Fix donut hole placeholder breaking under HTML escaping with 2+ query params

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
 

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;
     }
 

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);
     }
 }

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<string, string> $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)) {

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);
     }

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

tests/DonutRequestTest.php

@@ -46,15 +46,15 @@ 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;
     }
 
     /** @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');
     }
 

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

tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMultiParamQueryHole.html.twig

@@ -1 +1 @@
-donut-cache-with-multi-param-query-hole:{{ article }}<hole>{{ hole|raw }}</hole>
+donut-cache-with-multi-param-query-hole:{{ article }}<hole>{{ hole }}</hole>

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<string, array{0: string}>
+     */
+    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('<p>[le:app://self/query-hole?' . $query . ']</p>', [], null, false);
+
+        $refreshed = $donut->refresh($this->resource, $ro);
+
+        $this->assertSame('<p>' . htmlspecialchars($value, ENT_QUOTES, 'UTF-8') . '</p>', (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('<p>' . $placeholder . '</p>', [], null, false);
+
+        $refreshed = $donut->refresh($this->resource, $ro);
+
+        $this->assertSame('<p>' . htmlspecialchars($value, ENT_QUOTES, 'UTF-8') . '</p>', (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('<p>[le:app://self/query-hole?token=a&amp;token=b]</p>', [], null, false);
+
+        $refreshed = $donut->refresh($this->resource, $ro);
+
+        $this->assertSame('<p>a</p>', (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('<p>[le:app://self/multi-param-query-hole?id=1&amp;tag=2]</p>', [], null, false);
+
+        $this->expectException(ParameterException::class);
+        $donut->refresh($this->resource, $ro);
+    }
 }