Closes #7947: add rocket_cache_http_headers filter so custom headers reach cached responses#8358
Closes #7947: add rocket_cache_http_headers filter so custom headers reach cached responses#8358Miraeld wants to merge 2 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Note Generated by the AI delivery pipeline (qa-engineer · claude-sonnet-4-6). Test Report — Closes #7947: add rocket_cache_http_headers filter so custom headers reach cached responsesBranch: fix/7947-wp-headers-is-not Strategy Selection
Acceptance Criteria
Test Run DetailsTest classes covered:
Code Analysis Findings
Smoke Tests
Overall: PASS Blockers: None. Recommendations:
Tests that could not be automated
|
| $rocket_early_hooks = WP_CONTENT_DIR . '/rocket-early-cache-hooks.php'; | ||
| if ( file_exists( $rocket_early_hooks ) ) { | ||
| $rocket_early_hooks = realpath( $rocket_early_hooks ); | ||
| if ( $rocket_early_hooks && 0 === strpos( $rocket_early_hooks, realpath( WP_CONTENT_DIR ) ) ) { |
There was a problem hiding this comment.
[CRITICAL] Path-containment check does not add a trailing directory separator before comparing, allowing a sibling directory like wp-content_evil to bypass the guard.
Proof: strpos('/var/www/html/wp-content_evil/rocket-early-cache-hooks.php', '/var/www/html/wp-content') === 0 returns true, so a symlink or a server where WP_CONTENT_DIR is a common prefix of another directory would allow inclusion of an out-of-tree file after realpath() resolution.
Fix: Append DIRECTORY_SEPARATOR to the realpath() result before the strpos() comparison:
if ( $rocket_early_hooks && 0 === strpos( $rocket_early_hooks, rtrim( realpath( WP_CONTENT_DIR ), DIRECTORY_SEPARATOR ) . DIRECTORY_SEPARATOR ) ) {|
|
||
| foreach ( $headers as $name => $value ) { | ||
| if ( is_string( $name ) && is_string( $value ) ) { | ||
| header( $name . ': ' . $value ); |
There was a problem hiding this comment.
[HIGH] send_headers() guards against non-string keys and values, but does NOT sanitize for CRLF characters within string values. A filter callback returning ['X-Foo' => "bar\r\nX-Injected: evil"] would pass the is_string() check. On PHP 7.3 (WP Rocket's minimum), header() does not block CRLF splitting — so response-splitting injection is possible if a misbehaving callback introduces newlines.
The spec doc says this 'prevents injection via a badly-written callback' — that claim is incorrect and the doc should be corrected.
Fix: Add a CRLF strip/guard inside the foreach loop, for example:
foreach ( $headers as $name => $value ) {
if ( is_string( $name ) && is_string( $value )
&& false === strpos( $name, "\r" ) && false === strpos( $name, "\n" )
&& false === strpos( $value, "\r" ) && false === strpos( $value, "\n" )
) {
header( $name . ': ' . $value );
}
}Also update the docblock in docs/api/cache-headers.md and send_headers() to reflect what is actually prevented (type checking only) versus what is not (CRLF injection on PHP < 7.4).
| } | ||
| ); | ||
|
|
||
| Functions\when( 'header' )->justReturn( null ); |
There was a problem hiding this comment.
[HIGH] Tests 2 and 5 do not assert that header() was actually called with the expected arguments. Test 2 captures the array state inside the filter callback and asserts the filter return value — it does not verify that header('X-Foo: foo') was invoked. Test 5 only asserts the filter ran, not that valid entries were dispatched to header() and invalid ones were not.
This leaves the core behavior of send_headers() — that it calls header($name . ': ' . $value) for each valid entry — untested at the header() level.
Fix: Use Functions\expect('header') instead of Functions\when('header') in tests 2 and 5, specifying the expected call arguments. For test 2:
Functions\expect( 'header' )
->once()
->with( 'X-Foo: foo' );For test 5, assert header is called exactly once (with 'X-Valid: ok') and never with the invalid entries. Apply the same fix to the mirror tests in ServeGzipCacheFile.php.
| [ | ||
| 'Last-Modified' => gmdate( 'D, d M Y H:i:s', filemtime( $cache_filepath ) ) . ' GMT', | ||
| ] | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Closing parenthesis on multi-line $this->send_headers(...) calls is indented at 3 tabs, but the opening statement is at 2 tabs (inside the method body). WordPress coding standards require the closing delimiter to align with the opening statement, not be indented further.
This pattern appears at lines 170, 192, 222, and 244.
Fix: Move the closing ) back one level to match the indentation of the $this->send_headers( call:
$this->send_headers(
\t[
\t\t'Last-Modified' => gmdate( 'D, d M Y H:i:s', filemtime( $cache_filepath ) ) . ' GMT',
\t]
);|
Note Generated by the AI delivery pipeline (lead-reviewer · claude-sonnet-4-6). Code Review — Issue #7947 / Branch: fix/7947-wp-headers-is-notSpec Compliance
Findings
Test CoverageFAIL — The 5 scenarios per method are present, but tests 2 and 5 in both test files do not assert the actual Overall: CHANGES REQUESTED Blockers (must fix before merge):
Follow-ups (non-blocking):
|
|
Note Generated by the AI delivery pipeline (qa-engineer · claude-sonnet-4-6). Re-run after fix commit 0a68acf. Test Report — fix/7947-wp-headers-is-not (post-fix re-run)Branch: fix/7947-wp-headers-is-not @ 0a68acf Strategy Selection
Acceptance Criteria
Security / Correctness VerificationCRLF guard ( Path-containment check ( 0 === strpos( $rocket_early_hooks, rtrim( realpath( WP_CONTENT_DIR ), DIRECTORY_SEPARATOR ) . DIRECTORY_SEPARATOR )The trailing Smoke Tests
Overall: PASS Blockers: None. Recommendations:
|
|
Note Generated by the AI delivery pipeline (lead-reviewer · Claude Sonnet 4.6). Code Review — Issue #7947 / Branch: fix/7947-wp-headers-is-not (review_loop=1)Blocker Verification (fix commit
|
| Previous blocker | Status | Evidence |
|---|---|---|
CRITICAL — advanced-cache.php line 99: path-containment check missing trailing DIRECTORY_SEPARATOR |
✅ Fixed | rtrim( realpath( WP_CONTENT_DIR ), DIRECTORY_SEPARATOR ) . DIRECTORY_SEPARATOR is now the needle. Docs updated to explain the sibling-directory bypass it prevents. |
HIGH — send_headers(): missing CRLF guard on header name/value |
✅ Fixed | Four strpos() checks guard \r and \n in both $name and $value before any header() call. Docs security section updated to describe PHP < 7.4 behaviour. |
HIGH — Tests 2 & 5 (both suites): passive Functions\when('header') stubs |
✅ Fixed | Both testFilterAddedHeaderIsSentViaHeader() and testNonStringKeyOrValueIsSkipped() now use Functions\expect('header')->once()->with(...). The call count and exact argument are both asserted. |
MEDIUM — Closing ) indentation on 4 send_headers() call sites |
✅ Fixed | All four closings are aligned with the 2-tab opening of the $this->send_headers( statement. |
Spec Compliance
| Spec item | Status | Notes |
|---|---|---|
send_headers() private method with rocket_cache_http_headers filter |
✅ Done | Correct docblock, apply_filters() pattern consistent with class, (array) cast in place. |
serve_cache_file() — Last-Modified routed through send_headers() |
✅ Done | |
serve_cache_file() — 304 path: Expires + Cache-Control through send_headers() |
✅ Done | Status line kept as direct header() call with code argument, as specified. |
serve_gzip_cache_file() — identical changes |
✅ Done | |
advanced-cache.php — early-hook drop-in mechanism |
✅ Done | file_exists + realpath + path-containment check + unset. Placed after class-existence guard, before new Config(...). |
Path-containment uses DIRECTORY_SEPARATOR suffix to prevent sibling-dir bypass |
✅ Done | |
CRLF guard in send_headers() |
✅ Done | |
| Non-string key/value guard | ✅ Done | is_string($name) && is_string($value) in the loop. |
| Non-array filter return handled | ✅ Done | (array) cast before loop. |
| Unit tests — Test 1: Last-Modified in filter input | ✅ Done | Both suites. |
Unit tests — Test 2: filter-added header sent via header() |
✅ Done | Functions\expect assertion confirms actual header() call. |
| Unit tests — Test 3: 304 path Expires + Cache-Control filterable | ✅ Done | Both suites. |
| Unit tests — Test 4: non-array filter return no fatal | ✅ Done | Both suites. |
Unit tests — Test 5: non-string key/value skipped, only valid entry calls header() |
✅ Done | Both suites. |
Documentation (docs/api/cache-headers.md) |
✅ Done | Security section now accurately describes all three defence layers and the DIRECTORY_SEPARATOR fix. |
| Out-of-scope: no Engine-layer changes | ✅ Confirmed | No files under inc/Engine/ changed. |
Findings
No new issues introduced by the fix commit. The implementation is clean:
- The CRLF guard correctly uses
false ===comparisons (avoidsstrposreturning0for a match at position 0 being falsy). unset( $rocket_early_hooks )cleans up the variable from global scope inadvanced-cache.php— good hygiene.- The
@group Bufferannotation is present on both test classes for targeted runs. ReflectionMethod::setAccessible(true)is the appropriate mechanism for testing a private method without changing visibility.
Test Coverage
PASS — 5 scenarios covered in both Test_ServeCacheFile and Test_ServeGzipCacheFile. Tests 2 and 5 are now behavioural assertions (call count + exact argument), not passive stubs.
Overall: PASS
No blockers remain. All four findings from review_loop=0 are resolved correctly with no regressions introduced.
…paths Introduce a new private send_headers() method in WP_Rocket\Buffer\Cache that applies the rocket_cache_http_headers filter before sending HTTP headers. This gives developers a hook point to inject custom headers on cached responses, which was previously impossible because the WordPress bootstrap never completes for early-exit cache serves. Route the Last-Modified, Expires, and Cache-Control headers in serve_cache_file() and serve_gzip_cache_file() through send_headers() so all three paths are filterable. Add an early-hook drop-in mechanism in advanced-cache.php so developers can register callbacks before plugins load via wp-content/rocket-early-cache-hooks.php. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add DIRECTORY_SEPARATOR to path-containment check in advanced-cache.php to prevent sibling-directory bypass (e.g. wp-content_evil) - Add CRLF guard in send_headers() to prevent response-splitting on PHP 7.3 - Strengthen tests 2 and 5: replace passive header() stubs with Functions\expect() call assertions in ServeCacheFile and ServeGzipCacheFile - Fix closing ) indentation on all four send_headers() call sites (PHPCS) - Correct docs/api/cache-headers.md security section to accurately describe what is and is not prevented Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
0a68acf to
7699a9d
Compare
Note
This PR was generated by the AI delivery pipeline (Claude Sonnet 4.6). Please review all changes carefully before merging.
Description
When WP Rocket serves a page from its file cache, it calls
exitimmediately after outputting content — deep insideadvanced-cache.php, before WordPress finishes loading. This means no plugin code ever runs, so thewp_headersfilter never fires and custom HTTP headers are absent from cached responses. This PR introduces a dedicatedrocket_cache_http_headersfilter that fires at the correct layer (insideserve_cache_file()/serve_gzip_cache_file()), and adds an early-hook drop-in mechanism so developers can register callbacks before plugins load. Fixes #7947Type of change
What was done
A new private
send_headers( array $headers ): voidmethod was added toWP_Rocket\Buffer\Cache. It applies therocket_cache_http_headersfilter to the incoming headers array before iterating and callingheader()for each valid string key/value pair. Bothserve_cache_file()andserve_gzip_cache_file()now route theirLast-Modified,Expires, andCache-Controlheaders throughsend_headers(), so all three named-header paths (200, 304 plain, 304 gzip) are filterable. Theadvanced-cache.phptemplate was updated to conditionally includewp-content/rocket-early-cache-hooks.php(with a path-traversal guard) before the cache is served, giving developers a supported place to calladd_filter()at PHP load time.Files changed
inc/classes/Buffer/class-cache.phpsend_headers()private method; routesLast-Modified,Expires, andCache-Controlthrough it in both serve methods.views/cache/advanced-cache.phpwp-content/rocket-early-cache-hooks.phpbefore cache serving, withrealpath-based path-traversal guard.tests/Unit/inc/classes/Buffer/Cache/ServeCacheFile.phpserve_cache_file(): filter receives correct headers, filter return is sent viaheader(), 304 path is filterable, non-string keys/values are skipped.tests/Unit/inc/classes/Buffer/Cache/ServeGzipCacheFile.phpserve_gzip_cache_file().tests/Fixtures/inc/classes/Buffer/Cache/ServeCacheFile.phpServeCacheFiletest class.tests/Fixtures/inc/classes/Buffer/Cache/ServeGzipCacheFile.phpServeGzipCacheFiletest class.docs/api/cache-headers.mdrocket_cache_http_headersfilter and therocket-early-cache-hooks.phpdrop-in.patchwork.jsonheaderto the list of patchwork-redefinable functions to support Brain\Monkey mocking in unit tests.Acceptance criteria
rocket_cache_http_headersfilter are present in the response.rocket_cache_http_headersby placingadd_filter()calls inwp-content/rocket-early-cache-hooks.php— this file is included byadvanced-cache.phpbefore any cache serving occurs, solving the bootstrap timing problem.Detailed scenario
What was tested
An e2e smoke test confirmed the fix end-to-end. A callback was registered on
rocket_cache_http_headersviawp-content/rocket-early-cache-hooks.phpto addX-WPRocket-Filter-Test: applied. On the first (uncached) request the header was absent, as expected. On the second (cached) request served byadvanced-cache.php, the headerX-WPRocket-Filter-Test: appliedwas present in the response, confirming the filter fires correctly on cached responses.How to test
wp-content/rocket-early-cache-hooks.phpwith:curl -I).X-WPRocket-Filter-Test: appliedis present in the cached response headers.Last-Modified,Expires, andCache-Controlheaders are still present and correct.curl -I --header "If-Modified-Since: <past date>") and verify the filter ran for the 304 path as well.Affected Features & Quality Assurance Scope
advanced-cache.phpdrop-in generation / early bootstrapTechnical description
Documentation
WP_Rocket\Buffer\Cache::send_headers()is a thin wrapper aroundapply_filters( 'rocket_cache_http_headers', $headers )followed by aforeachthat callsheader( "$name: $value" )for every entry where both key and value are strings. The string check is a security guard: a badly-written filter callback returning non-string values will be silently skipped rather than passed toheader(), which would otherwise produce a PHP warning or allow header injection.The bootstrap timing problem is solved by the early-hook drop-in:
advanced-cache.phpnow includeswp-content/rocket-early-cache-hooks.php(if it exists) at file scope beforemaybe_init_process()is called. WordPress'splugin.php(which definesadd_filter) is loaded beforeadvanced-cache.phpinwp-settings.php, soadd_filter()is available at that point. The included file runs at PHP parse time — not inside any WordPress action — so registered callbacks are in place whenserve_cache_file()later callsapply_filters( 'rocket_cache_http_headers', ... ).A
realpath()-based guard prevents path-traversal: the resolved path must haverealpath( WP_CONTENT_DIR )as a prefix before the file is included.New dependencies
None.
Risks
rocket-early-cache-hooks.phpthrows an uncaught exception, it will break cache serving for all requests. This is user responsibility; it is documented indocs/api/cache-headers.mdalongside the analogous risk forobject-cache.php.is_string()guard on both key and value insend_headers().file_exists()call inadvanced-cache.phpis a single stat per request and only triggers aninclude_oncewhen the file is present (which is the opt-in case).Follow-up tickets
None.
Mandatory Checklist
Code validation
Code style
Unticked items justification
N/A
Additional Checks