Opened 4 months ago
Last modified 3 months ago
#58433 new enhancement
WP version in Etag header in load-styles.php file
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch needs-testing has-testing-info has-screenshots |
Focuses: | Cc: |
Description
Hi
I know there are plenty of plugins to hide WP version. You also decided to hide WP version from readme.html some time ago. When the user uses all possible methods to hide WP version, current version still can be checked by load-styles.php and load-scripts.php files. It is visible in eTag header.
I know, this is not a big issue but I think is worth considering to use here some hash or checksum.
Regards
Dawid
Attachments (2)
Change History (18)
#1
in reply to:
↑ description
@
4 months ago
#2
@
4 months ago
It might be worth changing how the ETAG header is generated, not to hide the WordPress version but in order to better calculate a value that changes with the scripts' versions rather than the WordPress version.
As is, a 304 HTTP response may be returned despite the scripts' version changing. I've attached a POC to demonstrate a more accurate response. This helps explain some caching issues I've had working on WordPress-Develop.
While the POC does hide the WP version behind a hash, it won't help hide the WordPress version from any bots as the ETAG header would continue to provide a method for bots to determine the current version of WP if they store the hashes.
This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.
3 months ago
#5
@
3 months ago
- Keywords needs-testing-info added
- Type changed from defect (bug) to enhancement
@dav4 Thanks for your report.
As per POC, we can leave the request as an enhancement.
Not all contributors will be clear on how to test ETAG headers, so enhancement testing instructions would be helpfull.
#6
@
3 months ago
- Keywords has-patch needs-testing added
As discussed during bi-weekly Test Team triage, adding a couple additional keywords to help move this forward.
#7
@
3 months ago
In the recent test meeting in slack, there was a question about the poc suffix on 58433-poc.diff. It's to indicate it's a proof of concept.
I've designated it as such as an equivalent change would be required in wp-includes/load-styles.php
prior to commit. If the concept is proven as workable for loading scripts, then I will follow up with a patch that covers loading styles too.
Steps to test:
- Apply the patch to your development environment
- Visit the dashboard of your development environment.
- Open the developer tools in your browser, switch to the network tab.
- Look for a URL that contains
load-scripts.php
- Click on the URL, this should open a panel with further details
- The panel will show a section called "Response Headers" or similar
- Look for the header
ETAG
, it should show an apparently random string made up of 0-9A-F - Look for a second URL that contains
load-scripts.php
- Follow the steps above to look for the
ETAG
header - Confirm the ETAG header differs from the first URL's
Advanced testing
- Look for a URL containing both
load-scripts.php
andjquery
- Observe the ETAG header following the steps anove
- Change the version number of jQuery as registered on line 825 of
script-loader.php
- Reload the page in your browser
- Look for the same URL
- Confirm the ETAG header has changed from the initial version
@ironprogrammer let me know if these steps help or you think they need clarifying.
#8
@
3 months ago
- Keywords needs-testing-info removed
Thanks, @peterwilsoncc! In running through these steps, it appears the current Etags emitted by load-scripts.php
and load-styles.php
are not double-quoted, which prevents them from being emitted (at least in Nginx).
The tweak in 58433-poc-2.diff should emit the tag as expected (see the Etag header in `ms-files.php` for comparison).
The steps provided are great 👍🏻 The only addition would be to ensure that your environment has define( 'SCRIPT_DEBUG', false );
, as that allows both load-scripts.php
and load-styles.php
to be used (or just apply the patch to a vanilla WP install).
#9
@
3 months ago
- Keywords has-testing-info has-screenshots added
Test Report - POC
Patch tested: attachment:58433-poc-2.diff. This only covers load-scripts.php
, as noted in comment:7.
Environment
- Hardware: MacBook Pro Apple M1 Pro
- OS: macOS 13.4
- Browser: Safari 16.5, Google Chrome 114.0.5735.106
- Server: nginx/1.25.0
- PHP: 7.4.33
- WordPress: 6.3-alpha-55505-src
- In
wp-config.php
:define( 'SCRIPT_DEBUG', false );
Actual Results
- ✅ The Etag emitted differs between the separate calls to
load-scripts.php
. - ✅ [Advanced] The Etag changes when a loaded script version changes, e.g. for
jquery-core
. (See the Inception-inspired screenshot in Figure 1.)
Additional Notes
- Nginx automatically sets Etags to "weak" for gzipped assets, hence the tag prefix
W/
in the screenshot.
Supplemental Artifacts
Figure 1: A) jquery-core
set to version 3.7.0
, B) 3.7.1
, and C) back to 3.7.0
(same as A):
This ticket was mentioned in PR #4618 on WordPress/wordpress-develop by @peterwilsoncc.
3 months ago
#10
#11
@
3 months ago
In the linked pull request:
- included quotes around etag per @ironprogrammer's comment
- modified where the quotes were added so they are detected in time for the
HTTP_IF_NONE_MATCH
header/304 response. - always set the comparison type to weak (again allows for the 304 header to be hit)
- duplicated it all in the load-styles.php file for CSS
- added a little code to account for
$wp_[ASSET]->registered[ $handle ]->ver === false
#12
@
3 months ago
Test Report
Environment
- Hardware: HP 11th Gen Intel(R) Core(TM) i5-1155G7 @ 2.50GHz 2.50 GHz
- OS: Windows 11
- Browser: Firefox 114.0.1 (64-bit)
- Server: Apache/2.4.56
- PHP: 8.0.29
- WordPress: 6.3-alpha-55905
- Theme: twentytwentythree v1.1
- Active Plugins: gutenberg v16.0.0-rc.4
Steps to Reproduce
- Start Docker Container by command
- Open WordPress files on VS Code.
- Change Patch Code
- WordPress Dashboard > Reload > Inspect > Network Tab
Expected Results
- ✅ Etag is the same. ( simple test )
Supplemental Artifacts
Image - https://www.awesomescreenshot.com/image/40766965?key=bf8c7cb678c471e77f07028b14ddeede
Video ( simple test )- https://www.awesomescreenshot.com/video/18344738?key=e31f0aa401bca7e237d881d692ebcd6a
#13
@
3 months ago
Thanks for the PR, @peterwilsoncc 🙌🏻
Test Report
Patch tested: https://github.com/WordPress/wordpress-develop/pull/4618. This covers both load-scripts.php
and load-styles.php
, reflecting updates noted in comment:11.
Environment
- Hardware: MacBook Pro Apple M1 Pro
- OS: macOS 13.4
- Browser: Safari 16.5, Google Chrome 114.0.5735.106
- Server: nginx/1.25.0
- PHP: 7.4.33
- WordPress: 6.3-alpha-55505-src
- Also, Postman v10.15.1 (see Additional Notes)
Actual Results
- Simple Test:
- ✅ Etag is now emitted and differs between separate calls to
load-scripts.php
. - ✅ Etag is now emitted for
load-styles.php
.
- ✅ Etag is now emitted and differs between separate calls to
- Advanced Test:
- ✅ Etag changes for the
load-scripts.php
response when a script version changes, e.g. bumpingjquery-core
to3.7.1
. - ✅ Etag changes for the
load-styles.php
response when a script version changes, e.g. adding a version tocommon
, a la$styles->add( 'common', "/wp-admin/css/common$suffix.css", array(), '1.0.0' );
.
- ✅ Etag changes for the
- Testing
HTTP 304 Not Modified
:- ✅ When
If-None-Match
is sent, the server responds with the expected304
. - ✅ When
If-None-Match
is sent with an old/different Etag, the server responds with200
, returning the latest resource.
- ✅ When
Additional Notes
For the 304
response test using Postman:
- Configure a GET request, such as
http://YOUR-SERVER/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=jquery-core,jquery-migrate,utils&ver=6.3-alpha-55505-src
. - Copy the Etag value returned.
- Add the
If-None-Match
header to the request, setting its value to the Etag. - Resend the request, and a
304
should return.
#14
@
3 months ago
Thanks for testing the patch Brian.
As we move toward Beta, I'm tempted to see if we can put this in for WP 6.3.
As I alluded too in comment #2, failing to account for the asset versions in the generated etag can lead to unusual behavior when developing on WordPress-Develop.
As external libraries and block-editor packages are updated, I sometimes get a 304 header even when the files have been modified. It was only once this ticket was logged that I realised why.
#15
follow-up:
↓ 16
@
3 months ago
It might be worth changing how the ETAG header is generated, not to hide the WordPress version but in order to better calculate a value that changes with the scripts' versions rather than the WordPress version.
The way this has been working in alpha/beta (for WP development) is by appending a timestamp to $wp_version
every time WP is built, see https://core.trac.wordpress.org/browser/tags/6.2.2/Gruntfile.js#L414.
a 304 HTTP response may be returned despite the scripts' version changing
As far as I see the only way the current ETAG (the $wp_version
that includes the last built timestamp) can become outdated is if some core scripts or stylesheets were updated but WP was not build. Not sure if that's possible?
The patch in PR #4618 looks good however I'm not sure if it will fix the problem (edge case) of updating WP scripts and not building WP if that is the actual problem here. $wp_version
is used to bust caching for many individual WP scripts and stylesheets, so the same problem will happen when not concatenating them.
#16
in reply to:
↑ 15
@
3 months ago
Replying to azaozz:
The way this has been working in alpha/beta (for WP development) is by appending a timestamp to
$wp_version
every time WP is built, see https://core.trac.wordpress.org/browser/tags/6.2.2/Gruntfile.js#L414.
It looks like this only works if running WordPress-Develop from the build
directory. As most people work in the src
directory (anecdotal but src
is the default in WordPress-Develop's Docker environment) the cache remains unbroken.
Thus the etag doesn't change when the editor packages are updated which can cause the caching issues I've seen.
It may be worth adding something like this in version.php
but it's probably best done on another ticket:
<?php /* * When running from the source directory of the WordPress-Develop repo, the * WordPress version needs to be modified for the purpose of cache-busting * JavaScript and CSS files. The following code will modify the version number * to include the current timestamp if the version number is not a release. */ if ( preg_match( '/-.*-\d{5,}-src$/', $wp_version ) ) { $wp_version = substr_replace( $wp_version, (string) time(), -3 ); }
Hi there, welcome back to WordPress Trac!
Replying to dav4:
Just to clarify a bit for anyone looking at the ticket, the version number was removed from
readme.html
not for security reasons, but rather as part of a general focus on de-emphasizing the version in the admin, except for a few places where it's relevant: the About page, the Updates screen, Site Health, etc. See [39583] / #35554 and #42386.Indeed, there are other ways to determine the version number.