diff --git a/CHANGELOG b/CHANGELOG index ca4aa2998..2dd9742af 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,6 @@ # 3.29.0 (2026-XX-XX) + * Fix the sandbox resolving `use` trait templates before checking that the `use` tag is allowed * Fix `html_attr` dropping `style` declarations whose value is `0`, `0.0` or `'0'` * Fix the `default` filter fallback emitting an undefined variable warning when it uses the null-safe operator * Fix the `matches` operator silently treating PCRE execution errors as non-matches diff --git a/src/Node/CheckSecurityCallNode.php b/src/Node/CheckSecurityCallNode.php index a248040b1..b5337aee8 100644 --- a/src/Node/CheckSecurityCallNode.php +++ b/src/Node/CheckSecurityCallNode.php @@ -15,6 +15,9 @@ use Twig\Attribute\YieldReady; use Twig\Compiler; /** + * Wires the security checker at the very top of the template constructor, as + * the constructor resolves `use` traits before the sandbox could check them. + * * @author Fabien Potencier */ #[YieldReady] @@ -26,7 +29,7 @@ class CheckSecurityCallNode extends Node public function compile(Compiler $compiler) { $compiler - ->write("\$this->sandbox = \$this->extensions[SandboxExtension::class]->getChecker();\n") + ->write("\$this->sandbox = \$env->getExtension(SandboxExtension::class)->getChecker();\n") ; } } diff --git a/src/Node/CheckSecurityNode.php b/src/Node/CheckSecurityNode.php index f3944cf50..0a7ed8ff3 100644 --- a/src/Node/CheckSecurityNode.php +++ b/src/Node/CheckSecurityNode.php @@ -47,6 +47,32 @@ class CheckSecurityNode extends Node public function compile(Compiler $compiler): void { + if (isset($this->usedTags['use'])) { + $compiler + ->write("\n") + ->write("protected function checkTraitsAllowed(): void\n") + ->write("{\n") + ->indent() + ->write("if (!\$this->sandbox->isSandboxed(\$this->source)) {\n") + ->indent() + ->write("return;\n") + ->outdent() + ->write("}\n\n") + ->write("try {\n") + ->indent() + ->write("\$this->sandbox->checkSecurity(['use'], [], [], [], \$this->source);\n") + ->outdent() + ->write("} catch (SecurityNotAllowedTagError \$e) {\n") + ->indent() + ->write('$e->setTemplateLine(')->repr($this->usedTags['use'])->raw(");\n\n") + ->write("throw \$e;\n") + ->outdent() + ->write("}\n") + ->outdent() + ->write("}\n") + ; + } + $compiler ->write("\n") ->write("public function ensureSecurityChecked(): void\n") diff --git a/src/Node/ModuleNode.php b/src/Node/ModuleNode.php index ad67886a2..ca2b0b9c2 100644 --- a/src/Node/ModuleNode.php +++ b/src/Node/ModuleNode.php @@ -218,7 +218,8 @@ final class ModuleNode extends Node implements CoercesChildrenToStringInterface $countTraits = \count($this->getNode('traits')); if ($countTraits) { - // traits + $compiler->write("\$this->ensureTraitsAllowed();\n\n"); + foreach ($this->getNode('traits') as $i => $trait) { $node = $trait->getNode('template'); diff --git a/src/NodeVisitor/SandboxNodeVisitor.php b/src/NodeVisitor/SandboxNodeVisitor.php index dba49c8b0..15001f1b2 100644 --- a/src/NodeVisitor/SandboxNodeVisitor.php +++ b/src/NodeVisitor/SandboxNodeVisitor.php @@ -125,7 +125,7 @@ final class SandboxNodeVisitor implements NodeVisitorInterface if ($node instanceof ModuleNode) { $this->inAModule = false; - $node->setNode('constructor_end', new Nodes([new CheckSecurityCallNode(), $node->getNode('constructor_end')])); + $node->setNode('constructor_start', new Nodes([new CheckSecurityCallNode(), $node->getNode('constructor_start')])); $node->setNode('class_end', new Nodes([new CheckSecurityNode($this->filters, $this->tags, $this->functions, $this->tests), $node->getNode('class_end')])); } diff --git a/src/Template.php b/src/Template.php index 8cd406b52..212c0f25e 100644 --- a/src/Template.php +++ b/src/Template.php @@ -508,6 +508,31 @@ abstract class Template { } + /** + * Checks the "use" tag against the sandbox policy. + * + * The constructor resolves "use" traits eagerly, which reaches the loader, + * so that tag alone is checked here; the rest of the policy still runs at + * render time. + * + * @internal + */ + public function ensureTraitsAllowed(): void + { + try { + $this->checkTraitsAllowed(); + } catch (\Throwable $e) { + $this->handleException($e); + } + } + + /** + * @internal + */ + protected function checkTraitsAllowed(): void + { + } + /** * Auto-generated method to display the template with the given context. * diff --git a/tests/Extension/SandboxTest.php b/tests/Extension/SandboxTest.php index 12f6f6349..3a975f116 100644 --- a/tests/Extension/SandboxTest.php +++ b/tests/Extension/SandboxTest.php @@ -293,6 +293,9 @@ class SandboxTest extends TestCase { yield ['extends', '{% extends "1_empty" %}']; yield ['use', '{% use "1_empty" %}']; + yield 'use of a missing template is rejected before the loader is reached' => ['use', '{% use "does_not_exist" %}']; + yield 'use of a missing block is rejected before the trait is resolved' => ['use', '{% use "1_layout" with does_not_exist as alias %}']; + yield 'use of a non-traitable template is rejected before the trait is resolved' => ['use', '{% use "1_child" %}']; } /** diff --git a/tests/Sandbox/SandboxTest.php b/tests/Sandbox/SandboxTest.php index 220b072a8..63923d0de 100644 --- a/tests/Sandbox/SandboxTest.php +++ b/tests/Sandbox/SandboxTest.php @@ -13,6 +13,7 @@ namespace Twig\Tests\Sandbox; use PHPUnit\Framework\TestCase; use Twig\Environment; +use Twig\Error\RuntimeError; use Twig\Extension\SandboxExtension; use Twig\Loader\ArrayLoader; use Twig\Markup; @@ -244,6 +245,65 @@ class SandboxTest extends TestCase $denying->render('index'); } + public function testTheUseTagMustBeAllowed(): void + { + $templates = [ + 'index' => '{% use "blocks" with content as base_content %}{{ block("base_content") }}', + 'blocks' => '{% block content %}trait content{% endblock %}', + ]; + + $allowing = new Sandbox(self::env($templates), self::strictPolicy(tags: ['use', 'block'], functions: ['block'])); + $this->assertSame('trait content', $allowing->render('index')); + + $denying = new Sandbox(self::env(['index' => '{% use "missing" with content as base_content %}']), self::strictPolicy()); + $this->expectException(SecurityNotAllowedTagError::class); + $this->expectExceptionMessage('Tag "use" is not allowed'); + $denying->render('index'); + } + + public function testTheUseTagIsCheckedWithoutCheckingTheRestOfThePolicy(): void + { + // "middle" is only reachable as a trait, and the block carrying the + // forbidden filter is overridden by "index", so it never renders. + $sandbox = new Sandbox(self::env([ + 'index' => '{% use "middle" %}{% block content %}SAFE{% endblock %}', + 'middle' => '{% use "leaf" %}{% block content %}{{ "bad"|upper }}{% endblock %}', + 'leaf' => '', + ]), self::strictPolicy(tags: ['use', 'block'])); + + $this->assertSame('SAFE', $sandbox->render('index')); + } + + public function testTheUseTagIsCheckedBeforeTheTraitTemplateIsLoaded(): void + { + $sandbox = new Sandbox(self::env([ + 'index' => '{{ block("b", "receiver") is defined ? "YES" : "NO" }}', + 'receiver' => '{% use "missing" %}', + ]), self::strictPolicy(tags: ['block'], functions: ['block'])); + + $this->expectException(SecurityNotAllowedTagError::class); + $this->expectExceptionMessage('Tag "use" is not allowed'); + $sandbox->render('index'); + } + + public function testAPolicyFailureWhileResolvingTraitsKeepsItsTwigContext(): void + { + $sandbox = new Sandbox(self::env([ + 'index' => "{% use \"empty\" %}\n{{ 'a'|upper }}", + 'empty' => '', + ]), new ThrowingOnUseSecurityPolicy()); + + try { + $sandbox->render('index'); + $this->fail('The policy failure should have been reported.'); + } catch (RuntimeError $e) { + $this->assertStringContainsString('Policy backend unreachable', $e->getMessage()); + $this->assertSame('index', $e->getSourceContext()?->getName()); + $this->assertSame(2, $e->getTemplateLine()); + $this->assertInstanceOf(\RuntimeException::class, $e->getPrevious()); + } + } + public function testARenderOnAnotherEnvironmentDuringASandboxedRenderIsNotSandboxed(): void { $app = new Environment(new ArrayLoader(['trusted' => '{{ value|upper }}']), ['autoescape' => false]); @@ -377,6 +437,24 @@ class SandboxTest extends TestCase } } +final class ThrowingOnUseSecurityPolicy implements SecurityPolicyInterface +{ + public function checkSecurity($tags, $filters, $functions, $tests = []): void + { + if (\in_array('use', $tags, true)) { + throw new \RuntimeException('Policy backend unreachable.'); + } + } + + public function checkMethodAllowed($obj, $method): void + { + } + + public function checkPropertyAllowed($obj, $property): void + { + } +} + final class SandboxTestObject implements \Stringable { public $name = 'fabien';