From d1235ea5d624c7d2f124ed49e51fbb7b7d7315da Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 05:49:03 +0900 Subject: [PATCH 1/6] Fix donut hole placeholder breaking under HTML escaping with 2+ query params Twig's default autoescape turns the placeholder's & into & when a hole's URI template carries two or more query parameters. DonutRenderer's str_replace never matched the escaped text, so the raw [le:...] placeholder leaked to the client on the first request, and ResourceDonut::refresh()'s regex-extracted URI carried a stray amp; into the second query parameter on the next request, throwing ParameterException for a required param. The placeholder now carries a percent-encoded URI (ResourceDonut::placeholder()), which has no HTML-significant characters left for any renderer's default escaping to touch. refresh() decodes it back before re-fetching; decoding a plain URI with no percent sequence is a no-op, so already-cached templates keep working. Closes #232 --- CHANGELOG.md | 1 + src/DonutRenderer.php | 3 +- src/DonutRequest.php | 4 +- src/ResourceDonut.php | 19 +++++- tests/DonutHolePlaceholderEscapingTest.php | 67 +++++++++++++++++++ tests/DonutRendererTest.php | 2 +- tests/DonutRequestTest.php | 10 +-- .../DonutCacheWithMultiParamQueryHole.php | 7 +- ...onutCacheWithMultiParamQueryHole.html.twig | 2 +- 9 files changed, 99 insertions(+), 16 deletions(-) create mode 100644 tests/DonutHolePlaceholderEscapingTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index affd221a..1bada9ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 no longer leaks a raw `[le:...]` placeholder to the client, and no longer throws `BEAR\Resource\Exception\ParameterException` on the next request - Twig's default autoescape was turning the placeholder's `&` into `&`, which neither `DonutRenderer`'s substitution nor `ResourceDonut::refresh()`'s re-fetch matched (#232). The placeholder now carries a percent-encoded URI, which no HTML renderer's default escaping alters. No action needed: an already-cached donut template transparently keeps working (percent-decoding a URI with no `%` is a no-op), though any entry that was already corrupted by this bug before upgrading should be purged or left to expire. ## [1.17.0] - 2026-09-17 diff --git a/src/DonutRenderer.php b/src/DonutRenderer.php index afb64bb8..f4be009e 100644 --- a/src/DonutRenderer.php +++ b/src/DonutRenderer.php @@ -6,7 +6,6 @@ use Override; -use function sprintf; use function str_replace; final class DonutRenderer implements DonutRendererInterface @@ -20,7 +19,7 @@ final class DonutRenderer implements DonutRendererInterface #[Override] public function setView(string $uri, string $view): void { - $this->searches[] = sprintf(ResourceDonut::FORMAT, $uri); + $this->searches[] = ResourceDonut::placeholder($uri); $this->views[] = $view; } diff --git a/src/DonutRequest.php b/src/DonutRequest.php index c8c49a65..104b4cc2 100644 --- a/src/DonutRequest.php +++ b/src/DonutRequest.php @@ -8,8 +8,6 @@ use Override; use Stringable; -use function sprintf; - /** * Stands in for an embedded `#[Embed]` request while a donut template is composed * @@ -47,6 +45,6 @@ public function __toString(): string $this->etags->addTag($this->request->resourceObject); $this->donutStorage->setView($uri, $view); - return sprintf(ResourceDonut::FORMAT, $uri); + return ResourceDonut::placeholder($uri); } } diff --git a/src/ResourceDonut.php b/src/ResourceDonut.php index 687d9ddd..54fdf0d4 100644 --- a/src/ResourceDonut.php +++ b/src/ResourceDonut.php @@ -15,6 +15,9 @@ use function is_iterable; use function max; use function preg_replace_callback; +use function rawurldecode; +use function rawurlencode; +use function sprintf; use function strtotime; use function time; @@ -23,10 +26,22 @@ */ final class ResourceDonut { + // The %s is a rawurlencode()'d URI (see placeholder()): percent-encoding leaves no + // '&', '<', '>', '"' or '\'' for an HTML renderer's default autoescaping to mangle, + // so the same placeholder text round-trips unchanged whether it is echoed raw, through + // Twig's default `{{ hole }}` escaping, or by a non-HTML renderer (issue #232). public const FORMAT = '[le:%s]'; private const URI_REGEX = '/\[le:([^\]]+)]/'; + /** + * Build the placeholder DonutRequest::__toString() embeds and DonutRenderer matches + */ + public static function placeholder(string $uri): string + { + return sprintf(self::FORMAT, rawurlencode($uri)); + } + /** * @param array $headers * @param int|null $lastModified Time when the composed content last actually changed @@ -53,7 +68,9 @@ public function refresh(ResourceInterface $resource, ResourceObject $ro): Resour { $etags = new SurrogateKeys($ro->uri); $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags): string { - $uri = $matches[1]; + // rawurldecode() is a no-op on a plain URI with no '%' sequence, so entries + // stored by older, un-encoded versions still refresh correctly (issue #232). + $uri = rawurldecode($matches[1]); $ro = $resource->get($uri); $ro->toString(); if (array_key_exists(Header::SURROGATE_KEY, $ro->headers)) { diff --git a/tests/DonutHolePlaceholderEscapingTest.php b/tests/DonutHolePlaceholderEscapingTest.php new file mode 100644 index 00000000..ec8eb399 --- /dev/null +++ b/tests/DonutHolePlaceholderEscapingTest.php @@ -0,0 +1,67 @@ +get($uri)` in `refresh()`; the stray `amp;` corrupts the second query + * parameter, which throws `BEAR\Resource\Exception\ParameterException` for a required + * param (quieter but equally wrong for an optional one). + */ +class DonutHolePlaceholderEscapingTest extends TestCase +{ + private ResourceInterface $resource; + + protected function setUp(): void + { + static $injector; + + $namespace = 'FakeVendor\HelloWorld'; + $module = new FakeEtagPoolModule(ModuleFactory::getInstance($namespace)); + $module->override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); + if (! $injector) { + $injector = new Injector($module, __DIR__ . '/tmp'); + } + + assert($injector instanceof Injector); + $this->resource = $injector->getInstance(ResourceInterface::class); + + parent::setUp(); + } + + public function testFirstRequestSubstitutesTheHoleInsteadOfLeakingThePlaceholder(): void + { + $ro = $this->resource->get('page://self/html/donut-cache-with-multi-param-query-hole'); + $view = (string) $ro; + + $this->assertStringNotContainsString('[le:', $view, 'the escaped placeholder must still be substituted, not served raw'); + $this->assertStringContainsString('1-2', $view, 'the hole renders with both query params, not a mangled one'); + } + + /** @depends testFirstRequestSubstitutesTheHoleInsteadOfLeakingThePlaceholder */ + public function testSecondRequestRefreshesWithTheSameQueryInsteadOfThrowing(): void + { + $ro = $this->resource->get('page://self/html/donut-cache-with-multi-param-query-hole'); + $view = (string) $ro; + + $this->assertStringNotContainsString('[le:', $view); + $this->assertStringContainsString('1-2', $view, 'refresh() must re-fetch the hole with BOTH original query params'); + } +} diff --git a/tests/DonutRendererTest.php b/tests/DonutRendererTest.php index 35dfa4c1..3607fc1d 100644 --- a/tests/DonutRendererTest.php +++ b/tests/DonutRendererTest.php @@ -15,7 +15,7 @@ public function testRender(): void $renderer = new DonutRenderer(); $renderer->setView('app://foo', 'Foo'); $renderer->setView('app://bar', 'Bar'); - $template = sprintf('template foo=%s, bar=%s', sprintf(ResourceDonut::FORMAT, 'app://foo'), sprintf(ResourceDonut::FORMAT, 'app://bar')); + $template = sprintf('template foo=%s, bar=%s', ResourceDonut::placeholder('app://foo'), ResourceDonut::placeholder('app://bar')); $view = $renderer->render($template); $this->assertSame('template foo=Foo, bar=Bar', $view); } diff --git a/tests/DonutRequestTest.php b/tests/DonutRequestTest.php index ff1312c7..ff8c8d4c 100644 --- a/tests/DonutRequestTest.php +++ b/tests/DonutRequestTest.php @@ -46,7 +46,7 @@ public function testToString(): DonutRenderer $storage = new DonutRenderer(); $request = new DonutRequest($this->request, $storage, new SurrogateKeys(new Uri('app://self/foo')), new UncacheableMark()); $view = (string) $request; - $this->assertSame('[le:page://self/html/comment]', $view); + $this->assertSame(ResourceDonut::placeholder('page://self/html/comment'), $view); return $storage; } @@ -54,7 +54,7 @@ public function testToString(): DonutRenderer /** @depends testToString */ public function testRender(DonutRenderer $storage): void { - $view = $storage->render('cmt=[le:page://self/html/comment]'); + $view = $storage->render('cmt=' . ResourceDonut::placeholder('page://self/html/comment')); $this->assertSame('cmt=comment01', $view); } @@ -110,8 +110,8 @@ public function testToStringCarriesAQueryStringSetViaTheRequestsOwnQueryArray(): $placeholder = (string) $donutRequest; $expectedUri = 'app://self/query-hole?' . http_build_query(['token' => 'public-9', 'visibility' => 'public']); - $this->assertSame('[le:' . $expectedUri . ']', $placeholder, 'the placeholder must carry the query string set via the request\'s own query array'); - $this->assertSame('public-9', $storage->render('[le:' . $expectedUri . ']'), 'the stored view must be keyed by the exact same full URI the placeholder carries'); + $this->assertSame(ResourceDonut::placeholder($expectedUri), $placeholder, 'the placeholder must carry the query string set via the request\'s own query array'); + $this->assertSame('public-9', $storage->render(ResourceDonut::placeholder($expectedUri)), 'the stored view must be keyed by the exact same full URI the placeholder carries'); $this->assertFalse($mark->isMarked(), 'visibility=public must not mark'); } @@ -136,7 +136,7 @@ public function testToStringCarriesAQueryStringEvenWhenTheHoleMarksItself(): voi $placeholder = (string) $donutRequest; $expectedUri = 'app://self/query-hole?' . http_build_query(['token' => 'secret-5', 'visibility' => 'private']); - $this->assertSame('[le:' . $expectedUri . ']', $placeholder, 'the placeholder must carry the query string even though the hole marked itself'); + $this->assertSame(ResourceDonut::placeholder($expectedUri), $placeholder, 'the placeholder must carry the query string even though the hole marked itself'); $this->assertTrue($mark->isMarked(), 'visibility=private must mark'); } diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMultiParamQueryHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMultiParamQueryHole.php index fa934a2a..9faff99c 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMultiParamQueryHole.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMultiParamQueryHole.php @@ -10,9 +10,10 @@ /** * A #[DonutCache] page embedding a hole through #[Embed]'s own URI-template expansion with - * two query parameters (issue #222 follow-up E2E coverage). `|raw` in the template sidesteps - * a separate, out-of-scope Twig-autoescaping issue (default `{{ hole }}` HTML-escapes the - * placeholder's `&` when 2+ params are present, reported separately, not fixed here). + * two query parameters, rendered with the project's standard `{{ hole }}` (issue #232: + * Twig's default autoescape turns the placeholder's `&` into `&`, so neither the + * first-request substitution nor the second-request `ResourceDonut::refresh()` re-fetch + * matched it before the fix). */ #[DonutCache] class DonutCacheWithMultiParamQueryHole extends ResourceObject diff --git a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMultiParamQueryHole.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMultiParamQueryHole.html.twig index cf9aae5a..eb72d5ba 100644 --- a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMultiParamQueryHole.html.twig +++ b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMultiParamQueryHole.html.twig @@ -1 +1 @@ -donut-cache-with-multi-param-query-hole:{{ article }}{{ hole|raw }} \ No newline at end of file +donut-cache-with-multi-param-query-hole:{{ article }}{{ hole }} \ No newline at end of file From 09aa046675a12243843365bc5c17e4b68dd00198 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 05:54:13 +0900 Subject: [PATCH 2/6] Harden donut placeholder fix per review - Reuse the existing DonutRepositoryTest::testDonutCacheWithAMultiParamEmbedRendersAndRefreshesBothQueryParams as the regression test for #232 instead of a new, duplicate, @depends-chained test file: it already drives the fixture end-to-end (render GET, then an explicit invalidateTags()-forced refresh GET) in one self-contained method, so a mutation of either half of the fix (encode or decode) fails or errors it directly, with no @depends skip-on-failure gap. - ResourceDonut::FORMAT is now private: nothing outside placeholder() builds a placeholder anymore, so there is no sanctioned reason to sprintf() it directly and silently reintroduce an un-encoded (bug-class) placeholder. --- src/ResourceDonut.php | 2 +- tests/DonutHolePlaceholderEscapingTest.php | 67 ---------------------- tests/DonutRepositoryTest.php | 10 ++-- 3 files changed, 6 insertions(+), 73 deletions(-) delete mode 100644 tests/DonutHolePlaceholderEscapingTest.php diff --git a/src/ResourceDonut.php b/src/ResourceDonut.php index 54fdf0d4..a22e27b4 100644 --- a/src/ResourceDonut.php +++ b/src/ResourceDonut.php @@ -30,7 +30,7 @@ final class ResourceDonut // '&', '<', '>', '"' or '\'' for an HTML renderer's default autoescaping to mangle, // so the same placeholder text round-trips unchanged whether it is echoed raw, through // Twig's default `{{ hole }}` escaping, or by a non-HTML renderer (issue #232). - public const FORMAT = '[le:%s]'; + private const FORMAT = '[le:%s]'; private const URI_REGEX = '/\[le:([^\]]+)]/'; diff --git a/tests/DonutHolePlaceholderEscapingTest.php b/tests/DonutHolePlaceholderEscapingTest.php deleted file mode 100644 index ec8eb399..00000000 --- a/tests/DonutHolePlaceholderEscapingTest.php +++ /dev/null @@ -1,67 +0,0 @@ -get($uri)` in `refresh()`; the stray `amp;` corrupts the second query - * parameter, which throws `BEAR\Resource\Exception\ParameterException` for a required - * param (quieter but equally wrong for an optional one). - */ -class DonutHolePlaceholderEscapingTest extends TestCase -{ - private ResourceInterface $resource; - - protected function setUp(): void - { - static $injector; - - $namespace = 'FakeVendor\HelloWorld'; - $module = new FakeEtagPoolModule(ModuleFactory::getInstance($namespace)); - $module->override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); - if (! $injector) { - $injector = new Injector($module, __DIR__ . '/tmp'); - } - - assert($injector instanceof Injector); - $this->resource = $injector->getInstance(ResourceInterface::class); - - parent::setUp(); - } - - public function testFirstRequestSubstitutesTheHoleInsteadOfLeakingThePlaceholder(): void - { - $ro = $this->resource->get('page://self/html/donut-cache-with-multi-param-query-hole'); - $view = (string) $ro; - - $this->assertStringNotContainsString('[le:', $view, 'the escaped placeholder must still be substituted, not served raw'); - $this->assertStringContainsString('1-2', $view, 'the hole renders with both query params, not a mangled one'); - } - - /** @depends testFirstRequestSubstitutesTheHoleInsteadOfLeakingThePlaceholder */ - public function testSecondRequestRefreshesWithTheSameQueryInsteadOfThrowing(): void - { - $ro = $this->resource->get('page://self/html/donut-cache-with-multi-param-query-hole'); - $view = (string) $ro; - - $this->assertStringNotContainsString('[le:', $view); - $this->assertStringContainsString('1-2', $view, 'refresh() must re-fetch the hole with BOTH original query params'); - } -} diff --git a/tests/DonutRepositoryTest.php b/tests/DonutRepositoryTest.php index a9375e39..6997701d 100644 --- a/tests/DonutRepositoryTest.php +++ b/tests/DonutRepositoryTest.php @@ -250,18 +250,18 @@ public function testDonutCacheTemplateStoresWhenOnlyAnEmbeddedHoleMarksItself(): public function testDonutCacheWithAMultiParamEmbedRendersAndRefreshesBothQueryParams(): void { // End-to-end through the real #[Embed] pipeline (URI-template expansion, not a - // manually built request): the donut placeholder/view mapping and a later + // manually built request), through the project's standard `{{ hole }}` (no + // `|raw`): the donut placeholder/view mapping and a later // ResourceDonut::refresh() must carry and re-resolve BOTH query parameters, not - // just a single one. `|raw` in the fixture's template sidesteps a separate, - // out-of-scope Twig-autoescaping issue (default `{{ hole }}` HTML-escapes the - // placeholder's `&` when 2+ params are present - reported separately, not fixed - // here). + // just a single one, and the placeholder itself must survive Twig's default + // HTML-escaping of its `&` (issue #232). $injector = $this->getInjector(); $resource = $injector->getInstance(ResourceInterface::class); $storage = $injector->getInstance(ResourceStorageInterface::class); $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); $first = $resource->get('page://self/html/donut-cache-with-multi-param-query-hole'); + $this->assertStringNotContainsString('[le:', (string) $first->view, 'the escaped placeholder must be substituted, not leaked raw to the client'); $this->assertStringContainsString('1-2', (string) $first->view, 'both query params resolved on the first (render) GET'); // Drop just the hole's own state, leaving the page's donut template behind, so the From ea0cf8b3e75cb4a976ae592cbc6105c93b76a59e Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 06:15:21 +0900 Subject: [PATCH 3/6] Version-mark the donut placeholder so refresh() only decodes new-form entries refresh()'s rawurldecode() of EVERY captured placeholder URI (introduced by the first #232 fix) silently corrupted an older, un-encoded [le:URI] entry whenever its query value had itself needed percent-encoding (a literal '&', '=', '+', '/', or a non-ASCII character): the extra decode stripped that encoding before BEAR\Resource\Uri ever parsed it, either mis-splitting the value (parse_str re-reads the now-literal '&'/'=') or failing URL validation outright for a raw non-ASCII byte (BEAR\Resource\Exception\UriException). New placeholders now carry a '[le2:URI]' marker; refresh() only rawurldecode()s that form. A legacy '[le:URI]' placeholder is used exactly as it always was: correct for 0/1 query params, and still broken for 2+ exactly as it already was (that entry was already corrupted by #232 and self-heals once it expires and is rewritten). Tests (ResourceDonutTest): a data-provider matrix refreshing a legacy placeholder whose query value needs a '+', '%2F', or non-ASCII encoding, asserting it round-trips unchanged; the same matrix for the new 'le2' form; a legacy duplicate-key (optional params) entry staying exactly as silently wrong as before; a legacy required-2-param entry still throwing the same ParameterException, not a new UriException. Each new test mutation-checked against a blanket-decode revert, a 'helpfully' unescape-amp;- for-legacy-too revert, and a legacy-regex-removed revert. CHANGELOG corrected: with the marker, an old-form entry keeps working exactly as before (no action needed, no longer merely 'no-op decode' handwaving); only an entry already corrupted by #232 itself needs purging or expiry. --- CHANGELOG.md | 2 +- src/ResourceDonut.php | 30 ++++++++---- tests/ResourceDonutTest.php | 92 +++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bada9ac..0e4b323e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 no longer leaks a raw `[le:...]` placeholder to the client, and no longer throws `BEAR\Resource\Exception\ParameterException` on the next request - Twig's default autoescape was turning the placeholder's `&` into `&`, which neither `DonutRenderer`'s substitution nor `ResourceDonut::refresh()`'s re-fetch matched (#232). The placeholder now carries a percent-encoded URI, which no HTML renderer's default escaping alters. No action needed: an already-cached donut template transparently keeps working (percent-decoding a URI with no `%` is a no-op), though any entry that was already corrupted by this bug before upgrading should be purged or left to expire. +- A `#[DonutCache]` page embedding an `#[Embed]` hole whose URI carries two or more query parameters no longer leaks a raw `[le:...]` placeholder to the client, and no longer throws `BEAR\Resource\Exception\ParameterException` on the next request - Twig's default autoescape was turning the placeholder's `&` into `&`, which neither `DonutRenderer`'s substitution nor `ResourceDonut::refresh()`'s re-fetch matched (#232). New placeholders carry a percent-encoded URI under a new `[le2:...]` marker, which no HTML renderer's default escaping alters; a stored `[le:...]` placeholder written before this version is left exactly as it was, byte for byte. No action needed: an old-form entry keeps working exactly as before (correct for 0/1 query params, same pre-existing behavior for 2+). An entry already corrupted by this bug before upgrading - a `[le:...]` placeholder whose stored text already contains `amp;` - stays corrupted the same way it already was and should be purged or left to expire. ## [1.17.0] - 2026-09-17 diff --git a/src/ResourceDonut.php b/src/ResourceDonut.php index a22e27b4..3ef09f3e 100644 --- a/src/ResourceDonut.php +++ b/src/ResourceDonut.php @@ -26,13 +26,22 @@ */ final class ResourceDonut { - // The %s is a rawurlencode()'d URI (see placeholder()): percent-encoding leaves no - // '&', '<', '>', '"' or '\'' for an HTML renderer's default autoescaping to mangle, - // so the same placeholder text round-trips unchanged whether it is echoed raw, through - // Twig's default `{{ hole }}` escaping, or by a non-HTML renderer (issue #232). - private const FORMAT = '[le:%s]'; - - private const URI_REGEX = '/\[le:([^\]]+)]/'; + // `le2` placeholders (written by this version onward) carry a rawurlencode()'d URI: + // percent-encoding leaves no '&', '<', '>', '"' or '\'' for an HTML renderer's default + // autoescaping to mangle, so the same placeholder text round-trips unchanged whether + // it is echoed raw, through Twig's default `{{ hole }}` escaping, or by a non-HTML + // renderer (issue #232). A legacy `le` placeholder (written before this version) is + // never decoded - its URI is used exactly as refresh() always used it, which stays + // correct for 0/1 query params (and still broken for 2+, as it already was: that + // entry was already corrupted and self-heals once it expires and is rewritten). + // Decoding it here too would be wrong, not merely redundant: a legacy query VALUE + // that itself needed percent-encoding (a literal '&', '=', '+', '/', non-ASCII bytes) + // would be stripped of that encoding before BEAR\Resource\Uri ever parses it, either + // corrupting the value (parse_str re-splits it on the now-literal '&'/'=') or failing + // URL validation outright (a raw non-ASCII byte is not a valid URL character). + private const FORMAT = '[le2:%s]'; + + private const URI_REGEX = '/\[le2:([^\]]+)]|\[le:([^\]]+)]/'; /** * Build the placeholder DonutRequest::__toString() embeds and DonutRenderer matches @@ -68,9 +77,10 @@ public function refresh(ResourceInterface $resource, ResourceObject $ro): Resour { $etags = new SurrogateKeys($ro->uri); $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags): string { - // rawurldecode() is a no-op on a plain URI with no '%' sequence, so entries - // stored by older, un-encoded versions still refresh correctly (issue #232). - $uri = rawurldecode($matches[1]); + // Group 1 is the rawurlencode()'d URI of a new `le2` match, present only when + // that alternative matched; group 2 is a legacy `le` match's URI, used verbatim. + $new = $matches[1]; + $uri = $new !== '' ? rawurldecode($new) : ($matches[2] ?? ''); $ro = $resource->get($uri); $ro->toString(); if (array_key_exists(Header::SURROGATE_KEY, $ro->headers)) { diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index db2ef443..59d5f078 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -4,12 +4,15 @@ namespace BEAR\QueryRepository; +use BEAR\Resource\Exception\ParameterException; use BEAR\Resource\ResourceInterface; use Madapaja\TwigModule\TwigModule; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; use function dirname; +use function http_build_query; class ResourceDonutTest extends TestCase { @@ -49,4 +52,93 @@ public function testRefreshResolvesPlaceholdersOnSeparateLines(): void $this->assertStringContainsString('like0', (string) $refreshed->view); $this->assertStringNotContainsString('[le:', (string) $refreshed->view); } + + /** + * A query value a legacy entry's placeholder needed http_build_query() to protect: an + * encoded space ('+'), an encoded slash ('%2F'), and a non-ASCII character (its own + * multi-byte '%XX' sequence). + * + * @return array + */ + public static function legacyQueryValueProvider(): array + { + return [ + 'encoded space (+)' => ['a b'], + 'encoded slash (%2F)' => ['a/b'], + 'non-ASCII' => ['日本語'], + ]; + } + + /** + * A legacy `[le:URI]` placeholder - written before this version introduced `le2` - + * must still be used verbatim, not rawurldecode()'d: its query value was already + * percent-encoded once (by http_build_query() when the placeholder was first built), + * and BEAR\Resource\Uri decodes it exactly once when it parses the re-fetched URI. An + * extra decode here would strip that encoding before parsing ever sees it, corrupting + * the value (or, for a raw non-ASCII byte, failing BEAR\Resource\Uri's own URL + * validation outright) instead of leaving an untouched legacy entry working exactly as + * it always did. + */ + #[DataProvider('legacyQueryValueProvider')] + public function testRefreshDoesNotDoubleDecodeALegacyPlaceholdersQueryValue(string $value): void + { + $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); + $query = http_build_query(['token' => $value]); + $donut = new ResourceDonut('

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

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

' . $value . '

', (string) $refreshed->view, 'the legacy placeholder\'s URI must reach BEAR\Resource\Uri exactly as stored, not pre-decoded'); + } + + /** + * The new `le2` form (ResourceDonut::placeholder()) round-trips the same values a + * legacy entry carries, through its own rawurlencode()/rawurldecode() pair. + */ + #[DataProvider('legacyQueryValueProvider')] + public function testRefreshRoundTripsTheNewFormPlaceholdersQueryValue(string $value): void + { + $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); + $placeholder = ResourceDonut::placeholder('app://self/query-hole?' . http_build_query(['token' => $value])); + $donut = new ResourceDonut('

' . $placeholder . '

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

' . $value . '

', (string) $refreshed->view); + } + + /** + * A legacy `[le:URI]` placeholder for a hole with two OPTIONAL params and the same + * #232 `&`-to-`amp;` corruption: BEAR\Resource\Uri splits the query on the literal `&` + * left behind, so the second `token=b` becomes the unrelated key `amp;token`, and the + * first `token=a` silently wins - no required param is missing, so nothing throws, and + * the hole quietly recomposes with the wrong (first, not last) value. This is the + * "quieter" failure mode #232 describes for optional params, and it must stay exactly + * this wrong, not become something else, now that refresh() treats `le2` and `le` + * placeholders differently. + */ + public function testRefreshOfALegacyDuplicateKeyPlaceholderStaysExactlyAsWrongAsBeforeThisFix(): void + { + $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); + + $this->assertSame('

a

', (string) $refreshed->view, 'the first token "wins" exactly as it already did - not "b", and not an exception'); + } + + /** + * A legacy `[le:URI]` placeholder for a hole with two REQUIRED query params, corrupted + * the same way, stays exactly as broken as it already was: still a ParameterException, + * the same failure a pre-#232-fix refresh() threw - not this fix's rawurldecode() + * turning it into a different, new failure. + */ + public function testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBeforeThisFix(): void + { + $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); + $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); + } } From 5fd2b4e87601325e5971527b8ee7d98a40726042 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 06:20:59 +0900 Subject: [PATCH 4/6] Simplify placeholder regex and add a silent-corruption test case - URI_REGEX becomes a single pattern with one optional marker group ('/\[le(2)?:([^\]]+)]/') instead of two alternated patterns: both capture groups now always exist, so refresh()'s decode branch no longer needs the isset()/?? workarounds PHPStan's offset-access checks forced on the alternated version. - Added a 'reserved characters (&, =)' row to the legacy-placeholder test matrix: a legacy single-param value containing '&'/'=' (http_build_query-encoded as '%26'/'%3D') is the case that corrupts SILENTLY under a blanket rawurldecode() (parse_str() re-splits the now-literal '&'/'=' into extra params) rather than the non-ASCII row's loud UriException - a second, independent mutation killer for the same class of bug, covering the 'quieter' failure mode directly instead of only by inference from the non-ASCII case. - The legacy-value assertions now compare against htmlspecialchars(, ...), not the raw value: the hole's own {{ token }} is itself autoescaped by Twig, so the new reserved-character row's '&' renders as '&' in the final view - asserting the raw value would have failed for an unrelated reason. - Verified 'same as before this PR' empirically, not just by reasoning: checked out upstream/1.x's pre-fix src/, ran the legacy-placeholder tests - all pass unchanged (only the new-form round-trip test errors with 'undefined method placeholder()', expected, since that method is new) - then restored. - Re-ran the full mutation set (blanket decode, amp;-unescape-for-legacy-too, legacy-marker-made-required) against the simplified regex and the new test row; all still catch their respective reverts. --- src/ResourceDonut.php | 9 ++++----- tests/ResourceDonutTest.php | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/ResourceDonut.php b/src/ResourceDonut.php index 3ef09f3e..540f2293 100644 --- a/src/ResourceDonut.php +++ b/src/ResourceDonut.php @@ -41,7 +41,7 @@ final class ResourceDonut // URL validation outright (a raw non-ASCII byte is not a valid URL character). private const FORMAT = '[le2:%s]'; - private const URI_REGEX = '/\[le2:([^\]]+)]|\[le:([^\]]+)]/'; + private const URI_REGEX = '/\[le(2)?:([^\]]+)]/'; /** * Build the placeholder DonutRequest::__toString() embeds and DonutRenderer matches @@ -77,10 +77,9 @@ public function refresh(ResourceInterface $resource, ResourceObject $ro): Resour { $etags = new SurrogateKeys($ro->uri); $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags): string { - // Group 1 is the rawurlencode()'d URI of a new `le2` match, present only when - // that alternative matched; group 2 is a legacy `le` match's URI, used verbatim. - $new = $matches[1]; - $uri = $new !== '' ? rawurldecode($new) : ($matches[2] ?? ''); + // 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(); if (array_key_exists(Header::SURROGATE_KEY, $ro->headers)) { diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index 59d5f078..3a6ea76b 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -12,8 +12,11 @@ use Ray\Di\Injector; use function dirname; +use function htmlspecialchars; use function http_build_query; +use const ENT_QUOTES; + class ResourceDonutTest extends TestCase { private ResourceInterface $resource; @@ -55,8 +58,12 @@ public function testRefreshResolvesPlaceholdersOnSeparateLines(): void /** * A query value a legacy entry's placeholder needed http_build_query() to protect: an - * encoded space ('+'), an encoded slash ('%2F'), and a non-ASCII character (its own - * multi-byte '%XX' sequence). + * encoded space ('+'), an encoded slash ('%2F'), a non-ASCII character (its own + * multi-byte '%XX' sequence), and reserved query-string characters within the value + * itself ('&', '=') - the case that corrupts silently rather than fatally: blanket- + * decoding it before BEAR\Resource\Uri parses it lets parse_str() re-split the + * now-literal '&'/'=' into extra, unrelated params instead of leaving them part of + * the value. * * @return array */ @@ -66,6 +73,7 @@ public static function legacyQueryValueProvider(): array 'encoded space (+)' => ['a b'], 'encoded slash (%2F)' => ['a/b'], 'non-ASCII' => ['日本語'], + 'reserved characters (&, =)' => ['a&b=c'], ]; } @@ -88,7 +96,7 @@ public function testRefreshDoesNotDoubleDecodeALegacyPlaceholdersQueryValue(stri $refreshed = $donut->refresh($this->resource, $ro); - $this->assertSame('

' . $value . '

', (string) $refreshed->view, 'the legacy placeholder\'s URI must reach BEAR\Resource\Uri exactly as stored, not pre-decoded'); + $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'); } /** @@ -104,7 +112,7 @@ public function testRefreshRoundTripsTheNewFormPlaceholdersQueryValue(string $va $refreshed = $donut->refresh($this->resource, $ro); - $this->assertSame('

' . $value . '

', (string) $refreshed->view); + $this->assertSame('

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

', (string) $refreshed->view); } /** From f02ff020ed3c54717c67f77a73540259151bf919 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 06:24:41 +0900 Subject: [PATCH 5/6] Trim CHANGELOG entry for #232 to 1-2 sentences --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e4b323e..36f8b181 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 no longer leaks a raw `[le:...]` placeholder to the client, and no longer throws `BEAR\Resource\Exception\ParameterException` on the next request - Twig's default autoescape was turning the placeholder's `&` into `&`, which neither `DonutRenderer`'s substitution nor `ResourceDonut::refresh()`'s re-fetch matched (#232). New placeholders carry a percent-encoded URI under a new `[le2:...]` marker, which no HTML renderer's default escaping alters; a stored `[le:...]` placeholder written before this version is left exactly as it was, byte for byte. No action needed: an old-form entry keeps working exactly as before (correct for 0/1 query params, same pre-existing behavior for 2+). An entry already corrupted by this bug before upgrading - a `[le:...]` placeholder whose stored text already contains `amp;` - stays corrupted the same way it already was and should be purged or left to expire. +- 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). No action needed for existing entries: an old-form entry keeps working exactly as before; only an entry already corrupted by this bug before upgrading should be purged or left to expire. ## [1.17.0] - 2026-09-17 From e469665cfadace7a385ea7c14cc63537ffe3395a Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sat, 24 Oct 2026 06:26:15 +0900 Subject: [PATCH 6/6] Note the on-disk placeholder marker change in the #232 CHANGELOG entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36f8b181..78d45eef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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). No action needed for existing entries: an old-form 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 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. ## [1.17.0] - 2026-09-17