Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Exceptions/Concerns/RendersHttpExceptions.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,11 @@ private function getCachedError(): ?Response

$request = Request::createFrom(request())->fakeStaticCacheStatus($status);

return $cacher->hasCachedPage($request)
? $cacher->getCachedPage($request)->toResponse($request)
: null;
if (! $cacher->hasCachedPage($request)) {
return null;
}

return $cacher->getCachedPage($request)->toResponse($request);
}

public static function renderUsing(Closure $callback): void
Expand Down
21 changes: 18 additions & 3 deletions src/StaticCaching/Middleware/Cache.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Statamic\StaticCaching\NoCache\Session;
use Statamic\StaticCaching\Replacer;
use Statamic\StaticCaching\ResponseStatus;
use Symfony\Component\HttpFoundation\Response as SymfonyResponse;

use function Statamic\trans as __;

Expand Down Expand Up @@ -88,9 +89,21 @@ private function handleRequest($request, Closure $next)
$response = $next($request);

if ($this->shouldBeCached($request, $response)) {
$this->copyError($request, $response);
$preparedResponse = $this->makeReplacementsAndCacheResponse($request, $response);

// The clone above is what gets cached, and keeps any replacer placeholders
// (e.g. nocache regions, CSRF tokens) intact for future requests to expand
// per-visitor. Under the ApplicationCacher, prepareResponseToCache() leaves
// those placeholders in the live response untouched too (it's the FileCacher
// that needs them left in place, for client-side JS to resolve), so they need
// expanding here - normally a no-op, except when this content came from a
// shared error cache (see RendersHttpExceptions::getCachedError), whose
// placeholders were never expanded on the way out.
if ($this->cacher instanceof ApplicationCacher) {
$this->makeReplacements($response);
}

$this->makeReplacementsAndCacheResponse($request, $response);
$this->copyError($request, $preparedResponse);

$this->nocache->write();

Expand Down Expand Up @@ -160,7 +173,7 @@ private function attemptToGetCachedResponse($request)
}
}

private function makeReplacementsAndCacheResponse($request, $response)
private function makeReplacementsAndCacheResponse($request, $response): SymfonyResponse
{
$cachedResponse = clone $response;

Expand All @@ -169,6 +182,8 @@ private function makeReplacementsAndCacheResponse($request, $response)
}

$this->cacher->cachePage($request, $cachedResponse);

return $cachedResponse;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private function makeReplacements($response)
Expand Down
150 changes: 150 additions & 0 deletions tests/StaticCaching/SharedErrorsStaticCachingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,22 @@

use Illuminate\Http\Request;
use Illuminate\Routing\Events\ResponsePrepared;
use Illuminate\Support\Facades\Cache;
use Orchestra\Testbench\Attributes\DefineEnvironment;
use PHPUnit\Framework\Attributes\Test;
use Statamic\Facades\Site;
use Statamic\StaticCaching\Cachers\ApplicationCacher;
use Statamic\StaticCaching\Replacer;
use Symfony\Component\HttpFoundation\Response;
use Tests\FakesViews;
use Tests\PreventSavingStacheItemsToDisk;
use Tests\TestCase;

class SharedErrorsStaticCachingTest extends TestCase
{
use FakesViews;
use PreventSavingStacheItemsToDisk;

private ApplicationCacher $cacher;

protected function setUp(): void
Expand Down Expand Up @@ -75,4 +84,145 @@ private function shareError(int $status, string $content): void
// the framework fires during the real response lifecycle.
event(new ResponsePrepared($request, $response));
}

public function withTestReplacer($app)
{
$app['config']->set('statamic.static_caching.strategy', 'half');
$app['config']->set('statamic.static_caching.share_errors', true);
$app['config']->set('statamic.static_caching.replacers', array_merge(
$app['config']->get('statamic.static_caching.replacers'),
['test' => SharedErrorTestReplacer::class]
));
}

#[Test]
#[DefineEnvironment('withTestReplacer')]
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');
Comment on lines +100 to +106

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CsrfTokenReplacer end-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 }} into errors.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.


// First 404 renders live and seeds the shared cache. The live response
// itself is untouched by the prepare step - only the clone that gets
// cached is.
$this->get('/this-does-not-exist')
->assertNotFound()
->assertSee('404 LIVE_VALUE', false);

// A different URL that also 404s is served from the shared cache.
// Regression: previously this replayed whatever copyError() captured
// before any replacer ran, and getCachedError() never ran replacers on
// the way out either - so a stale value (or, for CsrfTokenReplacer in
// production, a frozen CSRF token from a different session) leaked to
// every subsequent visitor forever.
$this->get('/this-also-does-not-exist')
->assertNotFound()
->assertSee('404 REPLACED_ON_SERVE', false);
}

public function shareErrorsWithHalfMeasure($app)
{
$app['config']->set('statamic.static_caching.strategy', 'half');
$app['config']->set('statamic.static_caching.share_errors', true);
}

#[Test]
#[DefineEnvironment('shareErrorsWithHalfMeasure')]
public function each_session_gets_its_own_csrf_token_on_a_shared_error()
{
Cache::flush();

$this->withStandardFakeViews();
$this->viewShouldReturnRaw('errors.layout', '{{ template_content }}');
$this->viewShouldReturnRaw('errors.404', '404 {{ csrf_token }}');

// First 404 renders live and seeds the shared cache.
$this->withSession(['_token' => 'session-one-token'])
->get('/nope-one')
->assertNotFound()
->assertSee('404 session-one-token', false);

// A different URL that also 404s is served from the shared cache, but
// for a different session. It must get its own token, not the one
// frozen into the shared cache by the first visitor.
$this->withSession(['_token' => 'session-two-token'])
->get('/nope-two')
->assertNotFound()
->assertSee('404 session-two-token', false)
->assertDontSee('session-one-token', false);

// A repeat hit on that second URL, now cached under its own URL,
// must still resolve session two's current token, not a frozen one.
$this->withSession(['_token' => 'session-two-token'])
->get('/nope-two')
->assertNotFound()
->assertSee('404 session-two-token', false);
}

#[Test]
#[DefineEnvironment('shareErrorsWithHalfMeasure')]
public function nocache_region_inside_a_shared_error_stays_dynamic_across_repeat_hits()
{
Cache::flush();

// Use a tag that outputs something dynamic. It just increments by
// one every time it's rendered.
app()->instance('example_count', 0);

(new class extends \Statamic\Tags\Tags
{
public static $handle = 'example_count';

public function index()
{
$count = app('example_count');
$count++;
app()->instance('example_count', $count);

return $count;
}
})::register();

$this->withStandardFakeViews();
$this->viewShouldReturnRaw('errors.layout', '{{ template_content }}');
$this->viewShouldReturnRaw('errors.404', '404 {{ nocache }}{{ example_count }}{{ /nocache }}');

// First 404 renders live and seeds the shared cache.
$this->get('/nope-one')->assertNotFound()->assertSee('404 1', false);

// A different URL that also 404s is served from the shared cache.
// Its nocache region must render fresh, not replay whatever the
// shared cache captured when it was seeded.
$this->get('/nope-two')->assertNotFound()->assertSee('404 2', false);

// Repeat hits to that second URL, now cached under its own URL, must
// keep rendering the region fresh too. Before the fix, the first
// request served from the shared cache baked the region's rendered
// output into the per-URL cache entry, so every later hit on that
// URL replayed the same frozen value forever - the same class of
// cross-visitor leak this PR fixes for CSRF tokens.
$this->get('/nope-two')->assertNotFound()->assertSee('404 3', false);
$this->get('/nope-two')->assertNotFound()->assertSee('404 4', false);
}
}

class SharedErrorTestReplacer implements Replacer
{
const PLACEHOLDER = 'TEST_TOKEN_PLACEHOLDER';

public function prepareResponseToCache(Response $response, Response $initial)
{
// Mirrors CsrfTokenReplacer's behaviour for the "half" strategy:
// only the clone that gets cached is touched, not $initial (which
// is what's returned to this first request).
$response->setContent(str_replace('LIVE_VALUE', self::PLACEHOLDER, $response->getContent()));
}

public function replaceInCachedResponse(Response $response)
{
$response->setContent(str_replace(self::PLACEHOLDER, 'REPLACED_ON_SERVE', $response->getContent()));
}
}
Loading