Explorar el Código

fix: pin Twig autoescape strategy to 'html' (SEC_REVIEW F63)

The Twig factory in `ui/src/App/Container.php` relied on the Twig
default for the `autoescape` option. Twig 3 currently defaults to
`'html'`, so today every `{{ value }}` interpolation in a `.twig`
file is HTML-escaped. The SEC_REVIEW asked us not to lean on the
default in case a future Twig major bump or a Slim-twig wrapper
change silently flips it — un-escaped output across every
template would be a stored-XSS gold mine.

Pin `'autoescape' => 'html'` explicitly. If a future Twig version
ever rejects the option name (or renames it), `EscaperExtension`
throws on construction — a build-time error that fails CI rather
than a silent runtime regression.

Regression tests in `ui/tests/Integration/App/TwigConfigTest.php`:

  - `testAutoescapeStrategyIsExplicitlyHtml` boots the wired Twig
    environment, looks up the `EscaperExtension`, and asserts
    `getDefaultStrategy('any.twig')` returns `'html'` — directly
    pinning the option to the configured value.
  - `testRenderedTemplateAutoescapesUserInput` swaps in an
    in-memory `ArrayLoader` with a one-line template
    (`value=[{{ value }}]`), renders
    `'<script>alert(1)</script>'`, and asserts the output is
    `value=[&lt;script&gt;alert(1)&lt;/script&gt;]`. Together
    these prove both that the strategy is configured AND the
    pipeline applies it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa hace 3 días
padre
commit
d225bfe6b9
Se han modificado 2 ficheros con 67 adiciones y 0 borrados
  1. 9 0
      ui/src/App/Container.php
  2. 58 0
      ui/tests/Integration/App/TwigConfigTest.php

+ 9 - 0
ui/src/App/Container.php

@@ -117,6 +117,15 @@ final class Container
                     'cache' => false,
                     'auto_reload' => $isDev,
                     'strict_variables' => false,
+                    // SEC_REVIEW F63: pin the autoescape strategy to
+                    // `'html'` instead of relying on the Twig default.
+                    // The current major (Twig 3) defaults to `'html'`,
+                    // but a future major bump or a Slim-twig wrapper
+                    // change could silently flip it; an explicit pin
+                    // means a regression in the default would surface
+                    // as a build-time error (Twig refuses an unknown
+                    // strategy name) rather than as un-escaped output.
+                    'autoescape' => 'html',
                 ]);
                 $twig->getEnvironment()->addFunction(
                     new \Twig\TwigFunction('flag_emoji', static function (mixed $code): string {

+ 58 - 0
ui/tests/Integration/App/TwigConfigTest.php

@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Integration\App;
+
+use App\Tests\Integration\Support\AppTestCase;
+use Slim\Views\Twig;
+use Twig\Extension\EscaperExtension;
+
+/**
+ * SEC_REVIEW F63 — pin Twig's autoescape strategy to `'html'`. The
+ * current Twig major defaults to `'html'`, but the SEC_REVIEW asked
+ * us not to rely on the default in case a future major bump or a
+ * Slim-twig wrapper change silently flips it.
+ */
+final class TwigConfigTest extends AppTestCase
+{
+    protected function setUp(): void
+    {
+        $this->bootApp();
+    }
+
+    public function testAutoescapeStrategyIsExplicitlyHtml(): void
+    {
+        /** @var Twig $twig */
+        $twig = $this->container->get(Twig::class);
+        $escaper = $twig->getEnvironment()->getExtension(EscaperExtension::class);
+
+        // `getDefaultStrategy` takes the template name; for the
+        // pinned `'html'` policy it returns the strategy regardless
+        // of name.
+        self::assertSame('html', $escaper->getDefaultStrategy('any.twig'));
+        self::assertSame('html', $escaper->getDefaultStrategy('pages/login.twig'));
+    }
+
+    public function testRenderedTemplateAutoescapesUserInput(): void
+    {
+        // Defence-in-depth: render a string with a script tag through
+        // `{{ value }}` and confirm the output is HTML-escaped. The
+        // unit test above pins the configured strategy; this one
+        // proves the pipeline actually applies it.
+        /** @var Twig $twig */
+        $twig = $this->container->get(Twig::class);
+        $env = $twig->getEnvironment();
+
+        $env->setLoader(new \Twig\Loader\ArrayLoader([
+            'inline.twig' => 'value=[{{ value }}]',
+        ]));
+
+        $rendered = $env->render('inline.twig', ['value' => '<script>alert(1)</script>']);
+
+        self::assertSame(
+            'value=[&lt;script&gt;alert(1)&lt;/script&gt;]',
+            $rendered,
+        );
+    }
+}