[6.x] Ensure share_errors replaces CSRFs - #15265
Conversation
jasonvarga
left a comment
There was a problem hiding this comment.
Reviewed this end-to-end and reproduced both the original bug and the fix in a local checkout against origin/6.x.
The reported bug is real and this PR does fix it. With errors.404 containing <input value="{{ csrf_token }}"> and a fresh session token per request:
R1 /nope-one |
R2 /nope-two |
R3 /nope-three |
R4 /nope-two (repeat) |
|
|---|---|---|---|---|
6.x |
gYPLL… |
gYPLL… ❌ |
gYPLL… ❌ |
gYPLL… ❌ |
| this PR | own token | own token ✅ | own token ✅ | own token ✅ |
On 6.x every visitor gets visitor #1's token, exactly as described. On this branch each session gets its own. tests/StaticCaching/ is 179/179 green on the branch.
However, running the full replacer set in getCachedError() also pulls in NoCacheReplacer, and that introduces a regression that trades the CSRF leak for a different cross-visitor leak. Details inline — that one needs fixing before this can go in. The rest are notes.
One pre-existing issue, not introduced here, but worth knowing since a fix will touch this code: a RegionNotFound thrown while serving a shared error escapes the exception handler and produces a 500 instead of the 404. I confirmed this behaves identically on origin/6.x, so it's not a regression from this PR — but the new replacer pass in getCachedError() sits directly on that path, so whatever shape the fix for the inline Critical takes will be adjacent to it.
|
|
||
| $response = $cacher->getCachedPage($request)->toResponse($request); | ||
|
|
||
| $this->applyReplacers($response); |
There was a problem hiding this comment.
Critical — this bakes expanded @nocache regions into the per-URL cache entry.
applyReplacers() runs all configured replacers, which includes NoCacheReplacer. Its replaceInCachedResponse() expands <span class="nocache" …>NOCACHE_PLACEHOLDER</span> into fully rendered region content. That expanded response is then returned from the exception handler and flows back into Cache::handleRequest(), where shouldBeCached() is still true — so makeReplacementsAndCacheResponse() clones it and caches the expanded content under the real URL. The placeholders are gone from that clone, so every later hit on that URL replays frozen region output forever.
Before this change, getCachedError() returned content with the placeholders intact, so the clone kept them and only the live response got expanded (via NoCacheReplacer::prepareResponseToCache() mutating $initial, which is the ApplicationCacher branch).
Confirmed with a differential probe — errors.404 = 404 {{ nocache }}COUNT:{{ probe_count }}{{ /nocache }}, counter incrementing per render, same fixtures, only the two src/ files differing:
R1 /nope-one |
R2 /nope-two |
R3 /nope-two |
R4 /nope-two |
|
|---|---|---|---|---|
6.x |
COUNT:1 |
COUNT:2 |
COUNT:3 ✅ |
COUNT:4 ✅ |
| this PR | COUNT:1 |
COUNT:2 |
COUNT:2 ❌ |
COUNT:2 ❌ |
@nocache exists specifically to hold per-visitor content (greetings, cart counts, auth state), so freezing it into a shared per-URL cache entry is the same class of cross-visitor leak this PR is fixing. The share_errors + @nocache combination is supported and was fixed only recently in #14729 / #14780, so this would be a step back there.
CI stays green because the existing nocache_session_is_written_under_the_real_url_for_shared_errors test asserts where the nocache session is written, not that region content stays dynamic.
Direction: the live response does need the replacers applied, but the copy destined for the per-URL cache must keep its placeholders. The ordering that gets you both is clone → cache the clone → then makeReplacements() on the live response, which is what attemptToGetCachedResponse() already does. Practically: getCachedError() shouldn't expand the placeholders itself; the middleware should apply them to the live response after makeReplacementsAndCacheResponse() has taken its clone. Note CsrfTokenReplacer::prepareResponseToCache() only touches the clone under ApplicationCacher, so the live response still needs that pass either way.
Whatever shape it takes, please add a test asserting a nocache region inside a shared error stays dynamic across repeat hits to the same URL.
| private function applyReplacers(Response $response): void | ||
| { | ||
| collect(config('statamic.static_caching.replacers')) | ||
| ->map(fn ($class) => app($class)) | ||
| ->each(fn (Replacer $replacer) => $replacer->replaceInCachedResponse($response)); |
There was a problem hiding this comment.
Note — this duplicates the replacer pipeline.
This is a copy of Cache::getReplacers() + Cache::makeReplacements(). Two copies will drift; worth extracting to one shared place (a small service, or a static on the middleware).
They already disagree slightly: Cache::makeReplacements() guards $response instanceof Response before iterating, this doesn't. The typed parameter covers it today, but the two implementations are no longer the same shape.
|
|
||
| $this->cacher->cachePage($request, $cachedResponse); | ||
|
|
||
| return $cachedResponse; |
There was a problem hiding this comment.
Note — this method silently gains a return value.
makeReplacementsAndCacheResponse() now returns $cachedResponse, but has no return type and a name that still reads as a command rather than a query. Either add : Response, or split building the prepared clone out from caching it so both call sites share it explicitly.
| public function replacers_run_when_serving_a_shared_error() | ||
| { | ||
| Cache::flush(); | ||
|
|
||
| $this->withStandardFakeViews(); | ||
| $this->viewShouldReturnRaw('errors.layout', '{{ template_content }}'); | ||
| $this->viewShouldReturnRaw('errors.404', '404 LIVE_VALUE'); |
There was a problem hiding this comment.
Note — the test doesn't exercise the actual bug, or the interaction that breaks.
Proving the mechanism with a synthetic SharedErrorTestReplacer is reasonable as far as it goes, but two gaps:
- Nothing covers the real
CsrfTokenReplacerend-to-end — i.e. "a second session hitting a shared 404 gets its own token". That's the reported symptom, and it's cheap to assert directly: render{{ csrf_token }}intoerrors.404, regenerate the session token between requests, and assert each response carries the current token rather than the first one. - Nothing covers
share_errors+@nocache, which is precisely where the Critical above hides.
|
I've pushed up fixes which answer the review, however its now needing to check instanceof ApplicationCacher which feels entirely wrong to me. Open to any suggestions on how to work around that. |
Summary
With
static_caching.share_errorsenabled (with the 'half' strategy), the first 404 (or other error) response is cached once and reused for every subsequent error across all URLs and visitors, indefinitely. Two things go wrong in that path:Middleware\Cache::copyError() stores the response for the shared cache before any replacer runs (prepareResponseToCache), so anything a replacer would normally swap for a placeholder — e.g. CsrfTokenReplacer — gets stored in the cache in a way that can't be changed.
Exceptions\Concerns\RendersHttpExceptions::getCachedError() returns that cached response directly with no replacer pass at all, so even a correctly-prepared placeholder is never swapped back to a live value on serve.
In practice this means every visitor landing on a repeat 404 gets served the CSRF token belonging to whoever's session first triggered that cached error page, until the cache is cleared. Any Livewire component (or other CSRF-protected request) on that page then fails with a 419 as soon as it makes a request.
Fix