diff --git a/CHANGELOG.md b/CHANGELOG.md index affd221a..78d45eef 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 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 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..540f2293 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,9 +26,30 @@ */ final class ResourceDonut { - public const FORMAT = '[le:%s]'; + // `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 = '/\[le(2)?:([^\]]+)]/'; - 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 @@ -53,7 +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 { - $uri = $matches[1]; + // 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/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/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 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 diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index db2ef443..3a6ea76b 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -4,12 +4,18 @@ 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 htmlspecialchars; +use function http_build_query; + +use const ENT_QUOTES; class ResourceDonutTest extends TestCase { @@ -49,4 +55,98 @@ 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'), 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 + */ + public static function legacyQueryValueProvider(): array + { + return [ + 'encoded space (+)' => ['a b'], + 'encoded slash (%2F)' => ['a/b'], + 'non-ASCII' => ['日本語'], + 'reserved characters (&, =)' => ['a&b=c'], + ]; + } + + /** + * 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('

' . 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'); + } + + /** + * 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('

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

', (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); + } }