diff --git a/CHANGELOG.md b/CHANGELOG.md index 78d45eef..40fa8da5 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]`/`#[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). ### Changed +- 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::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. ### 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 diff --git a/README.md b/README.md index 841cf21a..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 — 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. `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 diff --git a/src/DonutRepository.php b/src/DonutRepository.php index d42b07cf..b48002e0 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 @@ -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); 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/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/src/UncacheableMarkInterface.php b/src/UncacheableMarkInterface.php index d11b2fb5..51136eff 100644 --- a/src/UncacheableMarkInterface.php +++ b/src/UncacheableMarkInterface.php @@ -30,6 +30,17 @@ * 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()` 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 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 */ interface UncacheableMarkInterface diff --git a/src/UnstorableGateInterface.php b/src/UnstorableGateInterface.php index 410ad0b4..ead6a315 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 (#234). + * * @see https://github.com/bearsunday/BEAR.QueryRepository/issues/222 */ interface UnstorableGateInterface 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/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/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/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/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/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'; 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/HandSetPrivateCacheControlTest.php b/tests/HandSetPrivateCacheControlTest.php new file mode 100644 index 00000000..116ebac2 --- /dev/null +++ b/tests/HandSetPrivateCacheControlTest.php @@ -0,0 +1,71 @@ +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); + } + + 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); + } +} 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/ResourceDonutTest.php b/tests/ResourceDonutTest.php index 3a6ea76b..51ad14fa 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; @@ -20,6 +24,7 @@ class ResourceDonutTest extends TestCase { private ResourceInterface $resource; + private UncacheableMarkInterface $mark; protected function setUp(): void { @@ -28,6 +33,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 +43,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 +55,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 +100,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 +116,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 +136,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 +153,106 @@ 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); + } + + /** + * 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 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->assertTrue( + $this->mark->isMarkedForCurrentScope(), + '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', + ); } } 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); }