From 7f9da293bfd681c0ff760d010bd84812d21fa297 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:21:50 +0900 Subject: [PATCH 01/12] Pin DonutRepository::refreshDonut()'s leak guard A hole that both changes content and marks the response unstorable during a donut refresh must not have that content persisted for the next visitor. DonutRepository::refreshDonut() already guards this with an early return in its marked branch, but nothing pinned it - mutating that return away let the marked content reach saveView() unnoticed. MutableComment gains an opt-in mark() call so a test can reproduce the exact scenario: changed + marked content recomposed during a refresh. --- tests/DonutQueryInterceptorPurgeTest.php | 48 +++++++++++++++++++ .../src/Resource/Page/Html/MutableComment.php | 12 +++++ 2 files changed, 60 insertions(+) diff --git a/tests/DonutQueryInterceptorPurgeTest.php b/tests/DonutQueryInterceptorPurgeTest.php index 19617240..217cd271 100644 --- a/tests/DonutQueryInterceptorPurgeTest.php +++ b/tests/DonutQueryInterceptorPurgeTest.php @@ -65,6 +65,7 @@ protected function configure(): void protected function tearDown(): void { MutableComment::$comment = 'comment-a'; + MutableComment::$markUnstorable = false; // Every emitted log entry must conform to the published schema (drift detection) $this->flushAndValidate($this->logger); } @@ -166,6 +167,53 @@ public function testRefreshPersistsChangedContentState(): void $this->assertNotSame($bLastModified, $a2->headers[Header::LAST_MODIFIED], 'B→A advances Last-Modified'); } + /** + * Pins DonutRepository::refreshDonut()'s leak guard: the `return $ro;` inside the + * marked branch of the refresh that stops a marked, changed recomposition from + * reaching saveView(). Mutating that line away (removing the early return) lets the + * marked content fall through to recordContentState()/saveView() and be persisted. + */ + public function testRefreshDoesNotPersistContentMarkedUnstorableDuringTheRefresh(): void + { + // Own pools: the mark this test sets is a per-injector singleton (UncacheableMark) + // and never unsets itself - it must not leak into the shared static injector the + // other test methods in this class reuse. + $injector = self::newInjector(); + $resource = $injector->getInstance(ResourceInterface::class); + $repository = $injector->getInstance(QueryRepository::class); + $mark = $injector->getInstance(UncacheableMarkInterface::class); + $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + + $pageUri = 'page://self/html/mutable-blog-posting'; + $commentUri = new Uri('page://self/html/mutable-comment'); + MutableComment::$comment = 'comment-a'; + MutableComment::$markUnstorable = false; + $resource->get($pageUri); + $logger->flush(); // drain the initial put: this test is about the refresh that follows + + // The hole both changes content and marks the recomposition unstorable - e.g. it + // issues a CSRF token while also returning fresh data - exactly the scenario the + // unprotected `return $ro;` in refreshDonut()'s marked branch guards against: + // removing it lets this changed, marked content fall through to saveView(). + MutableComment::$comment = 'comment-b-secret'; + MutableComment::$markUnstorable = true; + $purged = $repository->purge($commentUri); + assert($purged); + + $marked = $resource->get($pageUri); + $this->assertTrue($mark->isMarked(), 'sanity: the hole did mark this recomposition'); + $this->assertStringContainsString('comment-b-secret', (string) $marked->view, 'sanity: the recomposition did pick up the changed, marked content'); + $this->assertSame('private, no-store', $marked->headers[Header::CACHE_CONTROL]); + + // DonutRepository::get() consults exactly this state for EVERY later visitor, + // marked or not - if it now exists and carries the marked content, that content + // leaked into the shared cache for whoever requests this page next. + $state = $repository->get(new Uri($pageUri)); + $this->assertNull($state, 'content recomposed while marked unstorable must not be persisted for a later visitor'); + + $this->flushAndValidate($logger); + } + private function isStateCached(): bool { return $this->repository->get(new Uri('page://self/html/blog-posting')) instanceof ResourceState; diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php b/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php index bf22ee51..751123cb 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php @@ -5,6 +5,7 @@ namespace FakeVendor\HelloWorld\Resource\Page\Html; use BEAR\QueryRepository\Header; +use BEAR\QueryRepository\UncacheableMarkInterface; use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\Resource\ResourceObject; @@ -12,9 +13,20 @@ class MutableComment extends ResourceObject { public static string $comment = 'comment-a'; + /** Marks this recomposition unstorable, the way a CSRF token issuer would mid-render */ + public static bool $markUnstorable = false; + + public function __construct( + private readonly UncacheableMarkInterface $mark, + ) { + } public function onGet(): static { + if (self::$markUnstorable) { + $this->mark->mark('csrf-token-issued'); + } + $this->body = ['comment' => self::$comment]; $this->headers[Header::SURROGATE_KEY] = 'mutable-comment'; From 6fee6883b9e5c4351b702ce98b50db85b0f79b73 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:21:59 +0900 Subject: [PATCH 02/12] Scope a donut refresh's hole re-fetches with enterHole()/leaveHole() ResourceDonut::refresh() re-fetched each placeholder with $resource->get($uri) without wrapping it in UncacheableMarkInterface::enterHole()/leaveHole(), unlike DonutRequest::__toString() on the MISS path. A mark made during that re-fetch was therefore always attributed to TOP_LEVEL: an unrelated sibling hole marking itself during a refresh made an innocent nested #[DonutCache] hole's own reasonExcludingHoleMarks() check read as marked too, and that hole's own, otherwise-storable template was wrongly refused a re-save after a purge. DonutRepository::refreshDonut() is refresh()'s only caller; it now passes its own $mark. A nested hole that marks ITSELF during a refresh must still refuse its own template exactly as before - pinned by a new test alongside the fix for the unrelated-sibling case. --- src/DonutRepository.php | 2 +- src/ResourceDonut.php | 20 ++++-- tests/DonutCacheTest.php | 8 ++- tests/DonutRepositoryTest.php | 72 +++++++++++++++++++ ...acheWithMarkingAndNestedDonutCacheHole.php | 28 ++++++++ .../src/Resource/Page/Html/PlainDonutHole.php | 24 +++++++ ...thMarkingAndNestedDonutCacheHole.html.twig | 1 + .../Page/Html/PlainDonutHole.html.twig | 1 + tests/ResourceDonutTest.php | 14 ++-- 9 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/PlainDonutHole.php create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/PlainDonutHole.html.twig diff --git a/src/DonutRepository.php b/src/DonutRepository.php index d42b07cf..f52fc0ba 100644 --- a/src/DonutRepository.php +++ b/src/DonutRepository.php @@ -263,7 +263,7 @@ private function refreshDonut(ResourceObject $ro): ResourceObject|null $this->logger->event(new CacheHitContext('donut')); $this->logger->event(new RefreshDonutContext((string) $ro->uri)); - $donut->refresh($this->resource, $ro); + $donut->refresh($this->resource, $ro, $this->mark); $surrogateKeys = $ro->headers[Header::SURROGATE_KEY] ?? ''; // refresh() restores the stored headers before returning, discarding anything a diff --git a/src/ResourceDonut.php b/src/ResourceDonut.php index 540f2293..09ad5261 100644 --- a/src/ResourceDonut.php +++ b/src/ResourceDonut.php @@ -73,15 +73,27 @@ public function __construct( ) { } - public function refresh(ResourceInterface $resource, ResourceObject $ro): ResourceObject + public function refresh(ResourceInterface $resource, ResourceObject $ro, UncacheableMarkInterface $mark): ResourceObject { $etags = new SurrogateKeys($ro->uri); - $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags): string { + $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags, $mark): string { // Group 1 is "2" for a new `le2` match (its URI, group 2, is rawurlencode()'d), // or "" for a legacy `le` match (group 2 is its URI, used verbatim). $uri = $matches[1] === '2' ? rawurldecode($matches[2]) : $matches[2]; - $ro = $resource->get($uri); - $ro->toString(); + // Scoped the same way DonutRequest::__toString() scopes the MISS path: a mark + // made while this hole re-runs (onGet() or its own template render) must be + // attributed to the hole's own owner slot, not TOP_LEVEL - otherwise an + // unrelated sibling hole marking itself here is misread as "the page itself + // marked", and a nested #[DonutCache] hole's OWN template write is wrongly + // refused by reasonExcludingHoleMarks() for a mark that was never its own. + $mark->enterHole($uri); + try { + $ro = $resource->get($uri); + $ro->toString(); + } finally { + $mark->leaveHole($uri); + } + if (array_key_exists(Header::SURROGATE_KEY, $ro->headers)) { $etags->addTag($ro); } diff --git a/tests/DonutCacheTest.php b/tests/DonutCacheTest.php index 8beab91a..df6c058e 100644 --- a/tests/DonutCacheTest.php +++ b/tests/DonutCacheTest.php @@ -20,6 +20,7 @@ class DonutCacheTest extends TestCase { private ResourceInterface $resource; + private UncacheableMarkInterface $mark; protected function setUp(): void { @@ -29,6 +30,7 @@ protected function setUp(): void $module->override(new TwigModule([$path])); $injector = new Injector($module, __DIR__ . '/tmp'); $this->resource = $injector->getInstance(ResourceInterface::class); + $this->mark = new UncacheableMark(); parent::setUp(); } @@ -38,7 +40,7 @@ public function testGetState(): void $headers = ['Content-Type' => 'application/xml;']; $donut = new ResourceDonut('cmt=[le:page://self/html/comment]', $headers, null, true); $blog = $this->resource->get('page://self/html/blog-posting'); - $ro = $donut->refresh($this->resource, $blog); + $ro = $donut->refresh($this->resource, $blog, $this->mark); $this->assertInstanceOf(ResourceObject::class, $ro); $this->assertSame('cmt=comment01', $ro->view); $this->assertSame($headers['Content-Type'], $ro->headers['Content-Type']); @@ -179,7 +181,7 @@ public function testDonutReplaysTheStatusCodeItWasStoredWith(): void $donut = new ResourceDonut('tmpl', [], null, true, null, null, null, null, 301); $ro = $this->resource->get('page://self/html/comment'); - $donut->refresh($this->resource, $ro); + $donut->refresh($this->resource, $ro, $this->mark); $this->assertSame(301, $ro->code); } @@ -192,7 +194,7 @@ public function testLegacyDonutPayloadLeavesTheStatusCodeAlone(): void $ro = $this->resource->get('page://self/html/comment'); $ro->code = 301; - $this->legacyDonut()->refresh($this->resource, $ro); + $this->legacyDonut()->refresh($this->resource, $ro, $this->mark); $this->assertSame(301, $ro->code); } diff --git a/tests/DonutRepositoryTest.php b/tests/DonutRepositoryTest.php index 6997701d..776e9720 100644 --- a/tests/DonutRepositoryTest.php +++ b/tests/DonutRepositoryTest.php @@ -306,6 +306,78 @@ public function testNestedDonutCacheHoleDoesNotLeakItsOwnMarkedContentToTheShare $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); } + public function testRefreshReSavesAnUnrelatedNestedDonutCacheHolesTemplateAfterASiblingHoleMarksDuringTheRefresh(): void + { + // ResourceDonut::refresh() re-fetches each placeholder with $resource->get($uri); + // without scoping that re-fetch to its own URI the way DonutRequest::__toString() + // scopes the MISS path, a sibling hole's own mark (UnstorableValue marks every + // request) is attributed to TOP_LEVEL during the refresh - and an UNRELATED nested + // #[DonutCache] hole (PlainDonutHole, which never marks) reads + // isMarkedForCurrentScope() as true for its own write and wrongly refuses to + // re-save its own purged template (issue #222). + $injector = $this->getInjector(); + $resource = $injector->getInstance(ResourceInterface::class); + $storage = $injector->getInstance(ResourceStorageInterface::class); + $mark = $injector->getInstance(UncacheableMarkInterface::class); + $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + + // First GET: MISS. Composes the outer page's own template and, via its own + // nested write, PlainDonutHole's own template too. + $resource->get('page://self/html/donut-cache-with-marking-and-nested-donut-cache-hole'); + $logger->flush(); + + // Drop only the unrelated nested hole's own template, so the outer page's next + // GET - always a refresh for a #[DonutCache] page, which never stores its own + // state - re-fetches it as a fresh MISS that must re-save it. + $storage->invalidateTags([(new UriTag())(new Uri('page://self/html/plain-donut-hole'))]); + + $resource->get('page://self/html/donut-cache-with-marking-and-nested-donut-cache-hole'); + $tree = $this->flushAndValidate($logger); + $this->assertNotNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'the outer page is recomposed from its own cached donut'); + $this->assertTrue($mark->isMarked(), 'sanity: the sibling hole did mark during this refresh'); + + // A later visitor requesting the unrelated hole directly: if its own template was + // re-saved (the fix), this is a refresh (template hit); if the sibling's mark was + // misattributed to it instead (the bug), this is a fresh MISS again. + $logger->flush(); + $resource->get('page://self/html/plain-donut-hole'); + $laterTree = $this->flushAndValidate($logger); + $this->assertNotNull(self::eventContextJsonOf($laterTree, 'refresh_donut'), "the unrelated hole's own purged template was re-saved during the sibling's marked refresh, not skipped because of the sibling's mark"); + } + + public function testRefreshStillRefusesANestedDonutCacheHolesOwnTemplateWhenItMarksItselfDuringTheRefresh(): void + { + // Same safety property as testNestedDonutCacheHoleDoesNotLeakItsOwnMarkedContentToTheSharedCache + // above, but reached through ResourceDonut::refresh() instead of the initial + // compose (DonutRequest): enterHole()/leaveHole() around the refresh re-fetch must + // still let the hole's own render-time mark refuse ITS OWN template, the same way + // it does on the MISS path - the fix for the unrelated-sibling case above must not + // loosen this. + $injector = $this->getInjector(); + $resource = $injector->getInstance(ResourceInterface::class); + $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + + // First GET composes the outer page's own template (MISS); RenderTimeMark marks + // itself every time, so its own write is refused here too. + $resource->get('page://self/html/donut-cache-with-nested-donut-cache-hole'); + $logger->flush(); + + // A #[DonutCache] page never stores its own state, so this second GET is always a + // refresh - re-fetching RenderTimeMark through ResourceDonut::refresh(), not + // DonutRequest. + $resource->get('page://self/html/donut-cache-with-nested-donut-cache-hole'); + $tree = $this->flushAndValidate($logger); + $this->assertNotNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'the outer page is recomposed from its own cached donut'); + + // A later, different visitor requesting the hole's own URI directly. If the + // hole's template had been stored with the leaked content during the refresh + // above, this would be a hit serving it. + $ro = $resource->get('page://self/html/render-time-mark'); + $laterTree = $this->flushAndValidate($logger); + $this->assertNull(self::eventContextJsonOf($laterTree, 'refresh_donut'), "nothing was stored for this later visitor to recompose from, even though the hole's own mark fired during the OUTER page's refresh"); + $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + } + public function testDonutCacheTemplateSkipsSaveWhenMarkedWhileItsOwnTemplateRenders(): void { // The mark can also happen while the page's own template renders (e.g. a Twig CSRF diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php new file mode 100644 index 00000000..20a36027 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php @@ -0,0 +1,28 @@ +body['article'] = '1'; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/PlainDonutHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/PlainDonutHole.php new file mode 100644 index 00000000..35a50edf --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/PlainDonutHole.php @@ -0,0 +1,24 @@ +body['value'] = 'plain'; + + return $this; + } +} diff --git a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig new file mode 100644 index 00000000..9faf9029 --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig @@ -0,0 +1 @@ +marking-and-nested:{{ article }}{{ secret }}{{ plain }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/PlainDonutHole.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/PlainDonutHole.html.twig new file mode 100644 index 00000000..1caf4b69 --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/PlainDonutHole.html.twig @@ -0,0 +1 @@ +plain-donut-hole:{{ value }} \ No newline at end of file diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index 3a6ea76b..070333ee 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -20,6 +20,7 @@ class ResourceDonutTest extends TestCase { private ResourceInterface $resource; + private UncacheableMarkInterface $mark; protected function setUp(): void { @@ -28,6 +29,7 @@ protected function setUp(): void $module->override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); $injector = new Injector($module, __DIR__ . '/tmp'); $this->resource = $injector->getInstance(ResourceInterface::class); + $this->mark = new UncacheableMark(); parent::setUp(); } @@ -37,7 +39,7 @@ public function testRefreshResolvesTwoPlaceholdersOnTheSameLine(): void $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); $donut = new ResourceDonut('

[le:page://self/html/comment][le:page://self/html/like]

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro); + $refreshed = $donut->refresh($this->resource, $ro, $this->mark); $this->assertStringContainsString('comment01', (string) $refreshed->view); $this->assertStringContainsString('like0', (string) $refreshed->view); @@ -49,7 +51,7 @@ public function testRefreshResolvesPlaceholdersOnSeparateLines(): void $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); $donut = new ResourceDonut("

[le:page://self/html/comment]

\n

[le:page://self/html/like]

", [], null, false); - $refreshed = $donut->refresh($this->resource, $ro); + $refreshed = $donut->refresh($this->resource, $ro, $this->mark); $this->assertStringContainsString('comment01', (string) $refreshed->view); $this->assertStringContainsString('like0', (string) $refreshed->view); @@ -94,7 +96,7 @@ public function testRefreshDoesNotDoubleDecodeALegacyPlaceholdersQueryValue(stri $query = http_build_query(['token' => $value]); $donut = new ResourceDonut('

[le:app://self/query-hole?' . $query . ']

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro); + $refreshed = $donut->refresh($this->resource, $ro, $this->mark); $this->assertSame('

' . htmlspecialchars($value, ENT_QUOTES, 'UTF-8') . '

', (string) $refreshed->view, 'the legacy placeholder\'s URI must reach BEAR\Resource\Uri exactly as stored, not pre-decoded'); } @@ -110,7 +112,7 @@ public function testRefreshRoundTripsTheNewFormPlaceholdersQueryValue(string $va $placeholder = ResourceDonut::placeholder('app://self/query-hole?' . http_build_query(['token' => $value])); $donut = new ResourceDonut('

' . $placeholder . '

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro); + $refreshed = $donut->refresh($this->resource, $ro, $this->mark); $this->assertSame('

' . htmlspecialchars($value, ENT_QUOTES, 'UTF-8') . '

', (string) $refreshed->view); } @@ -130,7 +132,7 @@ public function testRefreshOfALegacyDuplicateKeyPlaceholderStaysExactlyAsWrongAs $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); $donut = new ResourceDonut('

[le:app://self/query-hole?token=a&token=b]

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro); + $refreshed = $donut->refresh($this->resource, $ro, $this->mark); $this->assertSame('

a

', (string) $refreshed->view, 'the first token "wins" exactly as it already did - not "b", and not an exception'); } @@ -147,6 +149,6 @@ public function testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBefore $donut = new ResourceDonut('

[le:app://self/multi-param-query-hole?id=1&tag=2]

', [], null, false); $this->expectException(ParameterException::class); - $donut->refresh($this->resource, $ro); + $donut->refresh($this->resource, $ro, $this->mark); } } From c6ec10e7a69c0937c790550ab6e11fc27ffb8b7d Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:26:51 +0900 Subject: [PATCH 03/12] Keep a hand-set private/no-store Cache-Control header on an unstorable response UnstorableGate::reason() returns the same 'unstorable' for a mark() and for a Cache-Control the response set itself (private/no-store), so HeaderSetter could not tell them apart and overwrote both the same way: Cache-Control: private, no-store plus Pragma: no-cache, clobbering whatever the developer wrote. HeaderSetter now injects UncacheableMarkInterface directly and checks isMarked() once reason() is non-null: a mark still gets the package's own refusal headers unchanged, but a response refused only because of its own hand-set header keeps that header exactly as written. The CDN-facing headers (Surrogate-Key, CDN-Cache-Control and the Fastly/ Akamai equivalents) are stripped either way - this package still never hands a shared cache a lifetime for a response it refuses to store. put_skipped's reason enum is unaffected (still 'unstorable' either way) - QueryRepository/DonutRepository's own store-skip decision never needed to distinguish the two triggers, only the header setters did. --- src/HeaderSetter.php | 45 ++++++++------ .../App/HandSetPrivateCacheControl.php | 37 ++++++++++++ tests/HandSetPrivateCacheControlTest.php | 58 +++++++++++++++++++ tests/HeaderSetterTest.php | 2 +- tests/ResourceRepositoryTest.php | 10 ++-- 5 files changed, 129 insertions(+), 23 deletions(-) create mode 100644 tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php create mode 100644 tests/HandSetPrivateCacheControlTest.php diff --git a/src/HeaderSetter.php b/src/HeaderSetter.php index e05635d0..e28d0366 100644 --- a/src/HeaderSetter.php +++ b/src/HeaderSetter.php @@ -17,15 +17,12 @@ final readonly class HeaderSetter { /** - * Every header unset on an unstorable response (issue #222): the CDN-facing ones this - * package's setters manage, across every flavor (default, Fastly, Akamai), plus - * Cache-Control/Pragma themselves - so one the application set directly in onGet(), - * bypassing the setters, does not survive the refusal under any header-name casing (RFC - * 7230 §3.2: $ro->headers is a plain PHP array, not an HTTP-aware collection). + * The CDN-facing headers unset on an unstorable response (issue #222), across every + * flavor (default, Fastly, Akamai) - unset unconditionally, whether the gate tripped on + * a mark or on a Cache-Control the response set itself, since this package never hands a + * shared cache a lifetime for a response it is itself refusing to store. */ - private const STRIPPED_HEADERS = [ - Header::CACHE_CONTROL, - Header::PRAGMA, + private const CDN_FACING_HEADERS = [ Header::CDN_CACHE_CONTROL, FastlyCacheControlHeaderSetter::CDN_CACHE_CONTROL_HEADER, AkamaiCacheControlHeaderSetter::CDN_CACHE_CONTROL_HEADER, @@ -36,23 +33,34 @@ public function __construct( private EtagSetterInterface $etagSetter, private UnstorableGateInterface $gate, + private UncacheableMarkInterface $mark, ) { } /** * Set the validator and Cache-Control max-age, or - when the gate says this response is - * unstorable for this request (issue #222) - strip every CDN-facing header and replace - * Cache-Control/Pragma with ones that tell every cache, shared or private, not to keep it. - * A Cache-Control value #[HttpCache] or #[NoHttpCache] itself declared is exempt from the - * gate's no-store/private check: #[Cacheable] combined with #[HttpCache(noStore: true)] - * still stores server-side while telling the client not to. + * unstorable for this request (issue #222) - strip every CDN-facing header under any + * casing (RFC 7230 §3.2: $ro->headers is a plain PHP array, not an HTTP-aware collection). + * + * A producer that called UncacheableMarkInterface::mark() gets its Cache-Control/Pragma + * replaced too, with ones that tell every cache, shared or private, not to keep the + * response - the response was never meant to carry a cache directive of its own. A + * response refused only because it already set its own Cache-Control to `private`/ + * `no-store` - no mark - keeps exactly the header the developer wrote: the gate decides + * whether THIS package stores it server-side, not what the client is told. A + * Cache-Control value #[HttpCache] or #[NoHttpCache] itself declared is exempt from the + * gate's no-store/private check either way: #[Cacheable] combined with + * #[HttpCache(noStore: true)] still stores server-side while telling the client not to. */ public function __invoke(ResourceObject $ro, int|null $cacheControlMaxAge, HttpCache|null $httpCache, int|null $time = null): void { if ($this->gate->reason($ro) !== null) { - $this->stripHeaders($ro); - $ro->headers[Header::CACHE_CONTROL] = 'private, no-store'; - $ro->headers[Header::PRAGMA] = 'no-cache'; + $this->stripHeaders($ro, self::CDN_FACING_HEADERS); + if ($this->mark->isMarked()) { + $this->stripHeaders($ro, [Header::CACHE_CONTROL, Header::PRAGMA]); + $ro->headers[Header::CACHE_CONTROL] = 'private, no-store'; + $ro->headers[Header::PRAGMA] = 'no-cache'; + } return; } @@ -64,9 +72,10 @@ public function __invoke(ResourceObject $ro, int|null $cacheControlMaxAge, HttpC } } - private function stripHeaders(ResourceObject $ro): void + /** @param list $canonicalNames */ + private function stripHeaders(ResourceObject $ro, array $canonicalNames): void { - foreach (self::STRIPPED_HEADERS as $canonical) { + foreach ($canonicalNames as $canonical) { foreach (array_keys($ro->headers) as $name) { if (strtolower($name) === strtolower($canonical)) { unset($ro->headers[$name]); diff --git a/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php b/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php new file mode 100644 index 00000000..6b1d664f --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php @@ -0,0 +1,37 @@ +mark->mark('csrf-token-issued'); + } + + $this->headers[Header::CACHE_CONTROL] = 'private'; + $this->body = ['token' => 'value']; + + return $this; + } +} diff --git a/tests/HandSetPrivateCacheControlTest.php b/tests/HandSetPrivateCacheControlTest.php new file mode 100644 index 00000000..198060b5 --- /dev/null +++ b/tests/HandSetPrivateCacheControlTest.php @@ -0,0 +1,58 @@ +resource = $injector->getInstance(ResourceInterface::class); + + parent::setUp(); + } + + public function testHandSetPrivateCacheControlIsNotStoredButKeepsItsOwnHeader(): void + { + $first = $this->resource->get('app://self/hand-set-private-cache-control'); + $this->assertSame('private', $first->headers[Header::CACHE_CONTROL], "the developer's own header is kept exactly - no max-age appended, no overwrite to private, no-store"); + $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $first->headers); + $this->assertArrayNotHasKey(Header::SURROGATE_KEY, $first->headers); + + // Not stored server-side: a second GET re-runs onGet() (Age is set only when a + // GET is served from a previously stored state - QueryRepository::get()). + $second = $this->resource->get('app://self/hand-set-private-cache-control'); + $this->assertSame('private', $second->headers[Header::CACHE_CONTROL]); + $this->assertArrayNotHasKey('Age', $second->headers, 'a cache hit would carry Age; onGet() ran again instead'); + } + + public function testHandSetPrivateCacheControlPlusAMarkGetsThePackagesOwnHeaders(): void + { + // A producer also calling mark() (e.g. the same request issuing a CSRF token + // elsewhere) still gets the package's own refusal headers, unchanged from before + // this fix: the hand-set header alone is the owner-approved exception, a mark is not. + $ro = $this->resource->get('app://self/hand-set-private-cache-control', ['alsoMark' => true]); + $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + $this->assertSame('no-cache', $ro->headers[Header::PRAGMA]); + $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); + $this->assertArrayNotHasKey(Header::SURROGATE_KEY, $ro->headers); + } +} diff --git a/tests/HeaderSetterTest.php b/tests/HeaderSetterTest.php index 51178354..4abb90cd 100644 --- a/tests/HeaderSetterTest.php +++ b/tests/HeaderSetterTest.php @@ -11,7 +11,7 @@ class HeaderSetterTest extends TestCase { private function headerSetter(UncacheableMarkInterface $mark): HeaderSetter { - return new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl())); + return new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl()), $mark); } public function testStripHeadersMatchesALowercaseCdnCacheControlHeaderName(): void diff --git a/tests/ResourceRepositoryTest.php b/tests/ResourceRepositoryTest.php index cd16cc46..02cda4cf 100644 --- a/tests/ResourceRepositoryTest.php +++ b/tests/ResourceRepositoryTest.php @@ -37,9 +37,10 @@ public function get() return $this->tagAwareAdapter; } }; + $mark = new UncacheableMark(); $this->repository = new Repository( new NullSemanticLogger(), - new HeaderSetter(new EtagSetter(), new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl())), + new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl()), $mark), new ResourceStorage( new NullSemanticLogger(), new NullPurger(), @@ -51,7 +52,7 @@ public function get() ), new Expiry(0, 0, 0), new CacheDependency(new UriTag()), - new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()), + new UnstorableGate($mark, new DeclaredCacheControl()), ); $this->ro = new Index(); $this->ro->uri = new Uri('page://self/user'); @@ -120,9 +121,10 @@ public function get() return $this->tagAwareAdapter; } }; + $mark = new UncacheableMark(); $repository = new Repository( new NullSemanticLogger(), - new HeaderSetter(new EtagSetter(), new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl())), + new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl()), $mark), new ResourceStorage( new NullSemanticLogger(), new NullPurger(), @@ -134,7 +136,7 @@ public function get() ), new Expiry(0, 0, 0), new CacheDependency(new UriTag()), - new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()), + new UnstorableGate($mark, new DeclaredCacheControl()), ); $this->assertInstanceOf(Repository::class, $repository); } From c938d1e7158dc891d3a99436156e7ac6f59d62ed Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:27:49 +0900 Subject: [PATCH 04/12] Rewrite the Unreleased CHANGELOG section for 1.18.0 Split the single dense #222 Added bullet into Added/Changed/Fixed entries matching this PR's final behaviour: the hand-set private/no-store exception (item 3), ResourceDonut::FORMAT going private, and the CacheVersionModule rolling-deploy guidance. Shortened the #232 entry to one sentence and dropped its 'no action needed' tail, superseded by the CacheVersionModule bullet. The refresh-path hole-scope fix (ResourceDonut::refresh()) is not listed under Fixed: UncacheableMark/enterHole()/leaveHole() are new in this same Unreleased cycle, so that bug never shipped. --- CHANGELOG.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78d45eef..63b41008 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,15 +8,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- `UncacheableMarkInterface`/`UncacheableMark`: a request-scoped service a producer of a per-user value (a CSRF token issuer, etc.) calls to mark the current response unstorable - issue #222. A `#[Cacheable]` or `#[CacheableResponse]` write that is marked, or whose `Cache-Control` already says `no-store`/`private`, is skipped (`put_skipped{reason: unstorable}`), and the response gets `Cache-Control: private, no-store` plus `Pragma: no-cache` and no CDN lifetime header instead. `$module->override(new DevUnstorableGateModule())` turns the skip into a thrown `UnstorableResponseDetected` during development - `install()` does not work here, since `QueryRepositoryModule` already binds `UnstorableGateInterface` and `install()` never replaces a binding the installer already holds. Existing cache entries written before this change may hold per-user content - purge on deploy. `UncacheableMark` is bound `Scope::SINGLETON` for the request's one container; a long-running worker (Swoole, RoadRunner) reusing a container across requests must rebind it per request, the same way it already must for `ServerContextInterface`. +- `UncacheableMarkInterface::mark($reason)`: code that produces a per-user value (a CSRF token, etc.) marks the current response unstorable; it is then not cached (`put_skipped{reason: unstorable}`) and is sent with `Cache-Control: private, no-store` and `Pragma: no-cache` and no CDN headers (#222). A mark also skips every `#[Cacheable]` write that happens later in the same request. +- `UncacheableMark` is request-scoped: a long-running worker (Swoole, RoadRunner) that reuses one container across requests must rebind it per request, as it already must for `ServerContextInterface`. +- `DevUnstorableGateModule`: `$module->override(new DevUnstorableGateModule())` turns such a skip into a thrown `Exception\UnstorableResponseDetected` during development (`install()` has no effect here). ### Changed +- Deploy this version with a new `CacheVersionModule` version: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache shows raw `[le2:...]` placeholders during a rolling deploy. +- A `#[Cacheable]`/`#[CacheableResponse]` response whose own code sets `Cache-Control` to `private` or `no-store` is no longer stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. +- `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. - Requires `koriym/semantic-logger` `^0.11`. Written logs use its `log` document shape, and diagnostic `kind` is now `code`. - `KeepMutationsAndFailures` also keeps a session cut at the record limit (`semantic_logger_truncated`): a `command` past the limit leaves no other trace. ### Fixed - `LogFileWriter`: two sessions flushed in the same microsecond no longer overwrite each other. The timestamped filename now carries a random suffix, so both survive (#221). -- A `#[DonutCache]` page embedding an `#[Embed]` hole whose URI carries two or more query parameters leaked a raw `[le:...]` placeholder to the client on the first request and could throw `BEAR\Resource\Exception\ParameterException` on the next - fixed (#232) by moving new placeholders to a `[le2:...]` marker. No action needed for existing entries: an old-form `[le:...]` entry keeps working exactly as before; only an entry already corrupted by this bug before upgrading should be purged or left to expire. +- A `#[DonutCache]` page embedding an `#[Embed]` hole whose URI carried two or more query parameters leaked a raw `[le:...]` placeholder to the client and could throw `ParameterException` on the next request - fixed by moving new placeholders to a `[le2:...]` marker (#232). ## [1.17.0] - 2026-09-17 From df475311eb8ef4839f6074397d078b34fb1349b7 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:28:53 +0900 Subject: [PATCH 05/12] Document the sibling-collateral-refusal limitation (not fixed here) A mark made by one #[DonutCache] hole also refuses an entirely unrelated sibling #[Cacheable] resource's own write in the same request, because QueryRepositoryInterface/DonutRepositoryInterface read UncacheableMarkInterface::isMarked() - 'marked anywhere in this request, by any owner' - and cannot see the resource call tree the owner stack is built from. This is deliberately conservative, not a regression, and is left for a later release; documented on both interfaces so the trade-off is visible where the gate is consulted. --- src/UncacheableMarkInterface.php | 10 ++++++++++ src/UnstorableGateInterface.php | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/src/UncacheableMarkInterface.php b/src/UncacheableMarkInterface.php index d11b2fb5..7f4fc96d 100644 --- a/src/UncacheableMarkInterface.php +++ b/src/UncacheableMarkInterface.php @@ -30,6 +30,16 @@ * slot; anything else - including two sibling holes on the same path with different query * arguments - gets its own. * + * Known limitation, not a regression: isMarked() answers "marked anywhere in this request, by + * any owner" - every check except a `#[DonutCache]` write's own template uses it, including + * `QueryRepository::doPut()` for an entirely unrelated `#[Cacheable]` resource embedded + * elsewhere in the same request tree. A sibling hole that marks itself therefore also refuses + * every OTHER `#[Cacheable]` write in that request, not just its own - because + * `QueryRepositoryInterface`/`DonutRepositoryInterface` cannot see the resource call tree + * `UncacheableMark`'s owner stack is built from, only "was anything marked". Scoping + * `QueryRepository::doPut()`'s own check the same way `reasonExcludingHoleMarks()` scopes a + * donut template write is deliberately left for a later release. + * * @see https://github.com/bearsunday/BEAR.QueryRepository/issues/222 */ interface UncacheableMarkInterface diff --git a/src/UnstorableGateInterface.php b/src/UnstorableGateInterface.php index 410ad0b4..154dda94 100644 --- a/src/UnstorableGateInterface.php +++ b/src/UnstorableGateInterface.php @@ -19,6 +19,10 @@ * directive, not a value a method or a render produced: `#[Cacheable]` combined with * `#[HttpCache(noStore: true)]` still stores server-side while telling the client not to. * + * reason() reads `UncacheableMarkInterface::isMarked()`, "marked anywhere in this request" - + * see that interface's docblock for the resulting, deliberate limitation: a mark made by one + * `#[DonutCache]` hole also refuses an entirely unrelated sibling resource's own write. + * * @see https://github.com/bearsunday/BEAR.QueryRepository/issues/222 */ interface UnstorableGateInterface From ce0dc498b8ef33e8e6fab454540e851f5494c293 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:33:28 +0900 Subject: [PATCH 06/12] Fix refreshDonut() comments overstated by item 3, align README deploy note DonutRepository::refreshDonut()'s comments described the unstorable branch as unconditionally setting Cache-Control: private, no-store - true before the hand-set-header fix, no longer true: HeaderSetter now only overwrites Cache-Control/Pragma when a mark fired, and otherwise keeps the developer's own header while still stripping CDN headers. README's Unstorable-Response Guard bullet said to 'purge them as part of the deploy' for pre-upgrade entries; aligned it with the CHANGELOG's actual guidance (bump CacheVersionModule's version), which also covers the rolling-deploy raw-placeholder case a plain purge would not. --- README.md | 2 +- src/DonutRepository.php | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 841cf21a..53e20fc9 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ - **CDN Integration**: Seamlessly integrates with modern CDNs (e.g., Fastly, Akamai) for shared cache management. - **Conditional Requests with ETag Support**: Reduces network overhead by leveraging `ETag` and `304 Not Modified` responses. - **Distributed Cache Support**: Works with server-side caches (e.g., Redis, APC), shared caches (e.g., CDNs), and client-side caches. -- **Unstorable-Response Guard**: A producer of a per-user value (a CSRF token, etc.) can mark the current response unstorable via `UncacheableMarkInterface`, so it is never written to the shared cache ([#222](https://github.com/bearsunday/BEAR.QueryRepository/issues/222)); the mark is per-request state and must be rebound each request under a long-running worker (Swoole, RoadRunner). This guard only stops *new* writes — deploying it does not clean cache entries already stored before the upgrade, which may hold per-user content; purge them as part of the deploy. +- **Unstorable-Response Guard**: A producer of a per-user value (a CSRF token, etc.) can mark the current response unstorable via `UncacheableMarkInterface`, so it is never written to the shared cache ([#222](https://github.com/bearsunday/BEAR.QueryRepository/issues/222)); the mark is per-request state and must be rebound each request under a long-running worker (Swoole, RoadRunner). This guard only stops *new* writes — deploy a new `CacheVersionModule` version alongside it so this release starts from a fresh cache namespace: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache would otherwise show raw placeholders during a rolling deploy. ## Documentation diff --git a/src/DonutRepository.php b/src/DonutRepository.php index f52fc0ba..b48002e0 100644 --- a/src/DonutRepository.php +++ b/src/DonutRepository.php @@ -278,12 +278,14 @@ private function refreshDonut(ResourceObject $ro): ResourceObject|null // cached and the page is never stored as a rendered view, so there is no // page-level entry to save after the refresh - and this package never hands // the CDN a lifetime header of its own for this kind, mirroring doPutDonut's - // MISS-path exactly: only overwrite the restored headers with Cache-Control: - // private, no-store when a hole marks itself on THIS recomposition, never add - // a lifetime header that was never there (issue #222 - a hole marking itself - // must still be caught on a later, recomposed hit, not just the one that first - // wrote the template; but a storable template-only hit must stay exactly as - // storable as the MISS that created it, no new CDN lifetime invented here). + // MISS-path exactly: only touch the restored headers (HeaderSetter overwrites + // Cache-Control/Pragma when a mark fired, keeps a hand-set private/no-store + // header as-is otherwise - issue #222) when a hole marks itself on THIS + // recomposition, never add a lifetime header that was never there (a hole + // marking itself must still be caught on a later, recomposed hit, not just the + // one that first wrote the template; but a storable template-only hit must + // stay exactly as storable as the MISS that created it, no new CDN lifetime + // invented here). if ($unstorable !== null) { ($this->donutHeaderSetter)($ro, null, null); } @@ -297,8 +299,9 @@ private function refreshDonut(ResourceObject $ro): ResourceObject|null // When the recomposed content is byte-identical to the recorded one, carry over // the original Last-Modified instead of advancing it to the recomposition time $lastModified = $unstorable === null ? $donut->getUnchangedLastModified((string) $ro->view) : null; - // Always run, unstorable or not: this is what replaces the restored, stale-but- - // "storable" headers with Cache-Control: private, no-store on an unstorable response. + // Always run, unstorable or not: this is what strips the restored, stale-but- + // "storable" CDN headers from an unstorable response and - when a mark fired - + // also replaces Cache-Control/Pragma (HeaderSetter, issue #222). ($this->donutHeaderSetter)($ro, $donut->ttl, null, $lastModified); $this->donutHeaderSetter->setCdnCacheControl($ro, $donut->ttl); $this->logCdnHeaders($ro); From c6025f7de1c20ac62ca4f3e6ebdb2b3f8564c4db Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:33:32 +0900 Subject: [PATCH 07/12] Add donut-path proof that CDN headers are actually stripped, not just absent The existing #[Cacheable] regression test's assertArrayNotHasKey checks for CDN-Cache-Control/Surrogate-Key hold vacuously there: a plain app resource never gets those headers regardless. Added the donut-path sibling (BlogPostingCacheControl, #[CacheableResponse] hand-setting Cache-Control: no-store): DonutRepository::setHeaders() sets Surrogate-Key unconditionally before HeaderSetter runs, so this proves the strip actually happens - confirmed by mutating the strip to run only when marked, which makes Surrogate-Key leak through and the new test fail. --- tests/HandSetPrivateCacheControlTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/HandSetPrivateCacheControlTest.php b/tests/HandSetPrivateCacheControlTest.php index 198060b5..116ebac2 100644 --- a/tests/HandSetPrivateCacheControlTest.php +++ b/tests/HandSetPrivateCacheControlTest.php @@ -55,4 +55,17 @@ public function testHandSetPrivateCacheControlPlusAMarkGetsThePackagesOwnHeaders $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); $this->assertArrayNotHasKey(Header::SURROGATE_KEY, $ro->headers); } + + public function testHandSetNoStoreOnADonutResponseKeepsItsHeaderAndStripsCdnHeaders(): void + { + // BlogPostingCacheControl (#[CacheableResponse]) hand-sets Cache-Control: no-store + // via a plain property default (bypassing #[HttpCache]) - the donut-path sibling of + // the #[Cacheable] case above. DonutRepository::setHeaders() sets Surrogate-Key + // unconditionally before HeaderSetter runs, so this is not a vacuous check: it + // proves the CDN-facing headers are actually stripped, not merely absent. + $ro = $this->resource->get('page://self/html/blog-posting-cache-control'); + $this->assertSame('no-store', $ro->headers[Header::CACHE_CONTROL], "the developer's own header is kept exactly"); + $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); + $this->assertArrayNotHasKey(Header::SURROGATE_KEY, $ro->headers); + } } From e159ebbd7d8bebdd22fe45033ecafd0f84778b1c Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:46:24 +0900 Subject: [PATCH 08/12] Pin the try/finally around ResourceDonut::refresh()'s hole re-fetch An exception from a hole's re-fetch ($resource->get($uri) during a donut refresh - a pool outage, a broken onGet()) must still leave the mark's owner stack balanced: without the finally, the throwing hole's uri stays stuck as the 'current' scope for the rest of the request, misattributing a later, unrelated mark() to it instead of TOP_LEVEL. Replacing finally with an unconditional leaveHole() after get() keeps every other test green - the new regression test pins exactly this: it fails only under that mutation (471/471 pass otherwise). --- tests/ResourceDonutTest.php | 103 ++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index 070333ee..2d6014bd 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -5,11 +5,15 @@ namespace BEAR\QueryRepository; use BEAR\Resource\Exception\ParameterException; +use BEAR\Resource\Method; +use BEAR\Resource\RequestInterface; use BEAR\Resource\ResourceInterface; +use BEAR\Resource\ResourceObject; use Madapaja\TwigModule\TwigModule; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; +use RuntimeException; use function dirname; use function htmlspecialchars; @@ -151,4 +155,103 @@ public function testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBefore $this->expectException(ParameterException::class); $donut->refresh($this->resource, $ro, $this->mark); } + + /** + * A re-fetch that throws must still leave the hole's scope closed, the same way + * `finally` guarantees `leaveHole()` runs: an exception from `$resource->get($uri)` + * (a pool outage, a broken onGet()) must not leave `$uri` stuck on the mark's owner + * stack for the rest of the request - which would misattribute a later, unrelated + * mark() to this hole's scope instead of TOP_LEVEL. + */ + public function testRefreshLeavesTheHoleScopeEvenWhenTheReFetchThrows(): void + { + $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); + $uri = 'app://self/query-hole?token=x'; + $donut = new ResourceDonut('

' . ResourceDonut::placeholder($uri) . '

', [], null, false); + $throwingResource = new class implements ResourceInterface { + public function newInstance($uri): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function object(ResourceObject $ro): RequestInterface + { + throw new RuntimeException('not used'); + } + + public function uri($uri): RequestInterface + { + throw new RuntimeException('not used'); + } + + public function newRequest(Method $method, string $uri, array $query = []): RequestInterface + { + throw new RuntimeException('not used'); + } + + public function crawl(string $uri, string $linkKey, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function href(string $rel, array $query = [], ResourceObject|null $ro = null): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function get(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('pool outage'); + } + + public function post(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function put(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function patch(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function delete(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function head(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + + public function options(string $uri, array $query = []): ResourceObject + { + throw new RuntimeException('not used'); + } + }; + + try { + $donut->refresh($throwingResource, $ro, $this->mark); + $this->fail('expected the re-fetch to throw'); + } catch (RuntimeException $e) { + $this->assertSame('pool outage', $e->getMessage()); + } + + // A sibling hole with the SAME uri visiting correctly later (its own balanced + // enterHole()/mark()/leaveHole()) must leave the stack exactly as it found it - + // which it only does if the aborted re-fetch above left no entry behind. + $this->mark->enterHole($uri); + $this->mark->mark('elsewhere'); + $this->mark->leaveHole($uri); + + $this->assertFalse( + $this->mark->isMarkedForCurrentScope(), + 'a later, unrelated top-level check must read TOP_LEVEL, not a scope the aborted re-fetch left stuck on the stack', + ); + } } From 02e3760cc064d827f809c054e7e910b55db70b58 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:48:06 +0900 Subject: [PATCH 09/12] Broaden the sibling-collateral-refusal limitation note to CacheableResponse too DonutRepository::doPutStatic() (#[CacheableResponse]) reads gate->reason() the same 'any owner, anywhere' way QueryRepository::doPut() (#[Cacheable]) does, so a sibling hole's mark also collaterally refuses an unrelated #[CacheableResponse] write, not just #[Cacheable] ones. The limitation docblocks and the CHANGELOG Added bullet said '#[Cacheable]' only; broadened both, and referenced the tracking issue (#234). --- CHANGELOG.md | 2 +- src/UncacheableMarkInterface.php | 13 +++++++------ src/UnstorableGateInterface.php | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63b41008..25959202 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- `UncacheableMarkInterface::mark($reason)`: code that produces a per-user value (a CSRF token, etc.) marks the current response unstorable; it is then not cached (`put_skipped{reason: unstorable}`) and is sent with `Cache-Control: private, no-store` and `Pragma: no-cache` and no CDN headers (#222). A mark also skips every `#[Cacheable]` write that happens later in the same request. +- `UncacheableMarkInterface::mark($reason)`: code that produces a per-user value (a CSRF token, etc.) marks the current response unstorable; it is then not cached (`put_skipped{reason: unstorable}`) and is sent with `Cache-Control: private, no-store` and `Pragma: no-cache` and no CDN headers (#222). A mark also skips every `#[Cacheable]`/`#[CacheableResponse]` write that happens later in the same request, including an unrelated sibling's (#234). - `UncacheableMark` is request-scoped: a long-running worker (Swoole, RoadRunner) that reuses one container across requests must rebind it per request, as it already must for `ServerContextInterface`. - `DevUnstorableGateModule`: `$module->override(new DevUnstorableGateModule())` turns such a skip into a thrown `Exception\UnstorableResponseDetected` during development (`install()` has no effect here). diff --git a/src/UncacheableMarkInterface.php b/src/UncacheableMarkInterface.php index 7f4fc96d..51136eff 100644 --- a/src/UncacheableMarkInterface.php +++ b/src/UncacheableMarkInterface.php @@ -32,13 +32,14 @@ * * Known limitation, not a regression: isMarked() answers "marked anywhere in this request, by * any owner" - every check except a `#[DonutCache]` write's own template uses it, including - * `QueryRepository::doPut()` for an entirely unrelated `#[Cacheable]` resource embedded - * elsewhere in the same request tree. A sibling hole that marks itself therefore also refuses - * every OTHER `#[Cacheable]` write in that request, not just its own - because + * `QueryRepository::doPut()` and `DonutRepository::doPutStatic()` for an entirely unrelated + * `#[Cacheable]` or `#[CacheableResponse]` resource embedded elsewhere in the same request + * tree. A sibling hole that marks itself therefore also refuses every OTHER `#[Cacheable]`/ + * `#[CacheableResponse]` write in that request, not just its own - because * `QueryRepositoryInterface`/`DonutRepositoryInterface` cannot see the resource call tree - * `UncacheableMark`'s owner stack is built from, only "was anything marked". Scoping - * `QueryRepository::doPut()`'s own check the same way `reasonExcludingHoleMarks()` scopes a - * donut template write is deliberately left for a later release. + * `UncacheableMark`'s owner stack is built from, only "was anything marked". Scoping those + * writes' own checks the same way `reasonExcludingHoleMarks()` scopes a donut template + * write is deliberately left for a later release (#234). * * @see https://github.com/bearsunday/BEAR.QueryRepository/issues/222 */ diff --git a/src/UnstorableGateInterface.php b/src/UnstorableGateInterface.php index 154dda94..ead6a315 100644 --- a/src/UnstorableGateInterface.php +++ b/src/UnstorableGateInterface.php @@ -21,7 +21,7 @@ * * reason() reads `UncacheableMarkInterface::isMarked()`, "marked anywhere in this request" - * see that interface's docblock for the resulting, deliberate limitation: a mark made by one - * `#[DonutCache]` hole also refuses an entirely unrelated sibling resource's own write. + * `#[DonutCache]` hole also refuses an entirely unrelated sibling resource's own write (#234). * * @see https://github.com/bearsunday/BEAR.QueryRepository/issues/222 */ From 6f145c3305963aeabf2b55280ec109065ec46a2a Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:48:19 +0900 Subject: [PATCH 10/12] Note CacheVersionModule's precondition in the deploy guidance CacheVersionModule only namespaces pools built through this package's own Storage*Modules (or a #[CacheNamespace]-aware adapter); an app binding its own cache adapter directly gets no namespace change from bumping the version and must clear those pools instead. Added this clause to both the CHANGELOG deploy bullet and README's guard bullet. --- CHANGELOG.md | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25959202..c9f5a371 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `DevUnstorableGateModule`: `$module->override(new DevUnstorableGateModule())` turns such a skip into a thrown `Exception\UnstorableResponseDetected` during development (`install()` has no effect here). ### Changed -- Deploy this version with a new `CacheVersionModule` version: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache shows raw `[le2:...]` placeholders during a rolling deploy. +- Deploy this version with a new `CacheVersionModule` version: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache shows raw `[le2:...]` placeholders during a rolling deploy. Only namespaces the pools this package's own `Storage*Module`s (or a `#[CacheNamespace]`-aware adapter) construct - an app binding its own cache adapter directly must clear those pools instead. - A `#[Cacheable]`/`#[CacheableResponse]` response whose own code sets `Cache-Control` to `private` or `no-store` is no longer stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. - `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. - Requires `koriym/semantic-logger` `^0.11`. Written logs use its `log` document shape, and diagnostic `kind` is now `code`. diff --git a/README.md b/README.md index 53e20fc9..ec6dce37 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ - **CDN Integration**: Seamlessly integrates with modern CDNs (e.g., Fastly, Akamai) for shared cache management. - **Conditional Requests with ETag Support**: Reduces network overhead by leveraging `ETag` and `304 Not Modified` responses. - **Distributed Cache Support**: Works with server-side caches (e.g., Redis, APC), shared caches (e.g., CDNs), and client-side caches. -- **Unstorable-Response Guard**: A producer of a per-user value (a CSRF token, etc.) can mark the current response unstorable via `UncacheableMarkInterface`, so it is never written to the shared cache ([#222](https://github.com/bearsunday/BEAR.QueryRepository/issues/222)); the mark is per-request state and must be rebound each request under a long-running worker (Swoole, RoadRunner). This guard only stops *new* writes — deploy a new `CacheVersionModule` version alongside it so this release starts from a fresh cache namespace: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache would otherwise show raw placeholders during a rolling deploy. +- **Unstorable-Response Guard**: A producer of a per-user value (a CSRF token, etc.) can mark the current response unstorable via `UncacheableMarkInterface`, so it is never written to the shared cache ([#222](https://github.com/bearsunday/BEAR.QueryRepository/issues/222)); the mark is per-request state and must be rebound each request under a long-running worker (Swoole, RoadRunner). This guard only stops *new* writes — deploy a new `CacheVersionModule` version alongside it so this release starts from a fresh cache namespace: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache would otherwise show raw placeholders during a rolling deploy. `CacheVersionModule` only namespaces pools built through this package's own `Storage*Module`s or a `#[CacheNamespace]`-aware adapter; an app binding its own cache adapter directly must clear those pools instead. ## Documentation From 5f8775fb2c94e126d27317936c9d335417d950ea Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:49:43 +0900 Subject: [PATCH 11/12] Clarify the deploy note and list the ResourceDonut::refresh() signature change --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9f5a371..40fa8da5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `DevUnstorableGateModule`: `$module->override(new DevUnstorableGateModule())` turns such a skip into a thrown `Exception\UnstorableResponseDetected` during development (`install()` has no effect here). ### Changed -- Deploy this version with a new `CacheVersionModule` version: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache shows raw `[le2:...]` placeholders during a rolling deploy. Only namespaces the pools this package's own `Storage*Module`s (or a `#[CacheNamespace]`-aware adapter) construct - an app binding its own cache adapter directly must clear those pools instead. +- Deploy this version with a new `CacheVersionModule` version (an app that binds its own cache adapter rather than this package's `Storage*Module`s must clear its pools instead): entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache shows raw `[le2:...]` placeholders during a rolling deploy. - A `#[Cacheable]`/`#[CacheableResponse]` response whose own code sets `Cache-Control` to `private` or `no-store` is no longer stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. -- `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. +- `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. `ResourceDonut::refresh()` takes an additional `UncacheableMarkInterface` argument. - Requires `koriym/semantic-logger` `^0.11`. Written logs use its `log` document shape, and diagnostic `kind` is now `code`. - `KeepMutationsAndFailures` also keeps a session cut at the record limit (`semantic_logger_truncated`): a `command` past the limit leaves no other trace. From 09fd04443301cf0d3e40c87eff2fc0c86aa2c9bb Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 13:51:31 +0900 Subject: [PATCH 12/12] Simplify the try/finally probe to a direct mark()+leaveHole() check The prior probe re-entered the throwing hole's uri (enterHole/mark/ leaveHole) to simulate a sibling visiting it correctly later - works, but indirect. Marking immediately after the aborted re-fetch and calling leaveHole($uri) directly tests the same invariant more plainly: a balanced stack makes that leaveHole() a no-op (the mark stays filed under TOP_LEVEL, visible); a leaked entry puts $uri back on top first, so the mark gets filed there instead and leaveHole() pops it, leaving TOP_LEVEL - which this mark() never touched - looking unmarked. Re-mutation-proved: fails only under the finally removal. --- tests/ResourceDonutTest.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index 2d6014bd..51ad14fa 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -242,16 +242,17 @@ public function options(string $uri, array $query = []): ResourceObject $this->assertSame('pool outage', $e->getMessage()); } - // A sibling hole with the SAME uri visiting correctly later (its own balanced - // enterHole()/mark()/leaveHole()) must leave the stack exactly as it found it - - // which it only does if the aborted re-fetch above left no entry behind. - $this->mark->enterHole($uri); - $this->mark->mark('elsewhere'); + // A balanced stack would make this leaveHole() a no-op (the uri is not on it), so + // the mark() just above stays filed under TOP_LEVEL and is still visible here. A + // leaked entry instead puts $uri back on top: mark() files under $uri, leaveHole() + // pops exactly that entry, and the current scope reverts to TOP_LEVEL - which this + // mark() never touched - making the check below read false instead. + $this->mark->mark('after the throw'); $this->mark->leaveHole($uri); - $this->assertFalse( + $this->assertTrue( $this->mark->isMarkedForCurrentScope(), - 'a later, unrelated top-level check must read TOP_LEVEL, not a scope the aborted re-fetch left stuck on the stack', + 'a mark right after the aborted re-fetch must stay filed under TOP_LEVEL, not get displaced by a scope the throw left stuck on the stack', ); } }