[SECURITY] Fix a security issue where escaping was missing when using ??

Author: fabpot

CHANGELOG

@@ -1,5 +1,6 @@
 # 3.19.0 (2025-XX-XX)
 
+ * Fix a security issue where escaping was missing when using `??`
  * Deprecate `Token::getType()`, use `Token::test()` instead
  * Add `Token::toEnglish()`
  * Add `ForElseNode`

src/Node/Expression/Binary/NullCoalesceBinary.php

@@ -19,7 +19,6 @@
 use Twig\Node\Expression\Test\DefinedTest;
 use Twig\Node\Expression\Test\NullTest;
 use Twig\Node\Expression\Unary\NotUnary;
-use Twig\Node\Expression\Variable\ContextVariable;
 use Twig\TwigTest;
 
 final class NullCoalesceBinary extends AbstractBinary implements OperatorEscapeInterface
@@ -28,47 +27,31 @@ public function __construct(AbstractExpression $left, AbstractExpression $right,
     {
         parent::__construct($left, $right, $lineno);
 
-        if (!$left instanceof ContextVariable) {
-            $test = new DefinedTest(clone $left, new TwigTest('defined'), new EmptyNode(), $left->getTemplateLine());
-            // for "block()", we don't need the null test as the return value is always a string
-            if (!$left instanceof BlockReferenceExpression) {
-                $test = new AndBinary(
-                    $test,
-                    new NotUnary(new NullTest($left, new TwigTest('null'), new EmptyNode(), $left->getTemplateLine()), $left->getTemplateLine()),
-                    $left->getTemplateLine(),
-                );
-            }
-
-            $this->setNode('test', $test);
-        } else {
-            $left->setAttribute('always_defined', true);
+        $test = new DefinedTest(clone $left, new TwigTest('defined'), new EmptyNode(), $left->getTemplateLine());
+        // for "block()", we don't need the null test as the return value is always a string
+        if (!$left instanceof BlockReferenceExpression) {
+            $test = new AndBinary(
+                $test,
+                new NotUnary(new NullTest($left, new TwigTest('null'), new EmptyNode(), $left->getTemplateLine()), $left->getTemplateLine()),
+                $left->getTemplateLine(),
+            );
         }
+
+        $left->setAttribute('always_defined', true);
+        $this->setNode('test', $test);
     }
 
     public function compile(Compiler $compiler): void
     {
-        /*
-         * This optimizes only one case. PHP 7 also supports more complex expressions
-         * that can return null. So, for instance, if log is defined, log("foo") ?? "..." works,
-         * but log($a["foo"]) ?? "..." does not if $a["foo"] is not defined. More advanced
-         * cases might be implemented as an optimizer node visitor, but has not been done
-         * as benefits are probably not worth the added complexity.
-         */
-        if ($this->hasNode('test')) {
-            $compiler
-                ->raw('((')
-                ->subcompile($this->getNode('test'))
-                ->raw(') ? (')
-                ->subcompile($this->getNode('left'))
-                ->raw(') : (')
-                ->subcompile($this->getNode('right'))
-                ->raw('))')
-            ;
-
-            return;
-        }
-
-        parent::compile($compiler);
+        $compiler
+            ->raw('((')
+            ->subcompile($this->getNode('test'))
+            ->raw(') ? (')
+            ->subcompile($this->getNode('left'))
+            ->raw(') : (')
+            ->subcompile($this->getNode('right'))
+            ->raw('))')
+        ;
     }
 
     public function operator(Compiler $compiler): Compiler
@@ -78,6 +61,6 @@ public function operator(Compiler $compiler): Compiler
 
     public function getOperandNamesToEscape(): array
     {
-        return $this->hasNode('test') ? ['left', 'right'] : ['right'];
+        return ['left', 'right'];
     }
 }

tests/Fixtures/expressions/ternary_operator.test

@@ -5,14 +5,16 @@ Twig supports the ternary operator
 {{ 0 ? 'YES' : 'NO' }}
 {{ 0 ? 'YES' : (1 ? 'YES1' : 'NO1') }}
 {{ 0 ? 'YES' : (0 ? 'YES1' : 'NO1') }}
-{{ 1 == 1 ? 'foo<br />':'' }}
+{{ 1 == 1 ? 'foo<br />' : '' }}
 {{ foo ~ (bar ? ('-' ~ bar) : '') }}
+{{ true ? tag : 'KO' }}
 --DATA--
-return ['foo' => 'foo', 'bar' => 'bar']
+return ['foo' => 'foo', 'bar' => 'bar', 'tag' => '<br>']
 --EXPECT--
 YES
 NO
 YES1
 NO1
 foo<br />
 foo-bar
+&lt;br&gt;

tests/Fixtures/expressions/ternary_operator_noelse.test

@@ -3,8 +3,10 @@ Twig supports the ternary operator
 --TEMPLATE--
 {{ 1 ? 'YES' }}
 {{ 0 ? 'YES' }}
+{{ tag ? tag }}
 --DATA--
-return []
+return ['tag' => '<br>']
 --EXPECT--
 YES
 
+&lt;br&gt;

tests/Fixtures/expressions/ternary_operator_nothen.test

@@ -7,12 +7,14 @@ Twig supports the ternary operator
 {{ 0 ? : 'NO' }}
 {{ 'YES' ?  : 'NO' }}
 {{ 0 ?     : 'NO' }}
+{{ tag ?: 'KO' }}
 --DATA--
-return []
+return ['tag' => '<br>']
 --EXPECT--
 YES
 NO
 YES
 NO
 YES
 NO
+&lt;br&gt;

tests/Fixtures/tests/null_coalesce.test

@@ -15,8 +15,9 @@ Twig supports the ?? operator
 {{ 1 + (nope ?? 3) + (nada ?? 2) }}
 {{ obj.null() ?? 'OK' }}
 {{ obj.empty() ?? 'KO' }}
+{{ tag ?? 'KO' }}
 --DATA--
-return ['bar' => 'OK', 'foo' => ['bar' => 'OK'], 'obj' => new Twig\Tests\TwigTestFoo()]
+return ['bar' => 'OK', 'foo' => ['bar' => 'OK'], 'obj' => new Twig\Tests\TwigTestFoo(), 'tag' => '<br>']
 --EXPECT--
 OK
 OK
@@ -31,3 +32,5 @@ OK
 3
 6
 OK
+
+&lt;br&gt;

tests/Node/Expression/Binary/NullCoalesceTest.php

@@ -24,6 +24,6 @@ public static function provideTests(): iterable
         $right = new ConstantExpression(2, 1);
         $node = new NullCoalesceBinary($left, $right, 1);
 
-        return [[$node, "(// line 1\n\$context[\"foo\"] ?? 2)"]];
+        return [[$node, "(((// line 1\narray_key_exists(\"foo\", \$context) &&  !(null === \$context[\"foo\"]))) ? (\$context[\"foo\"]) : (2))"]];
     }
 }