[tainting] Allow Twig\Environment::render to be tainted even with a v…

…ariable as template name (#97)

Allow Twig\Environment::render to be tainted even with a variable as template parameters

Allow using a variable as template name for CachedTemplatesTainter too

Add TwigUtils::extractTemplateNameFromExpression tests

src/Twig/AnalyzedTemplatesTainter.php

@@ -7,12 +7,16 @@
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\Array_;
 use PhpParser\Node\Expr\MethodCall;
-use PhpParser\Node\Scalar\String_;
+use PhpParser\Node\Expr\Variable;
 use Psalm\Codebase;
 use Psalm\Context;
+use Psalm\Internal\DataFlow\DataFlowNode;
 use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface;
 use Psalm\StatementsSource;
+use Psalm\Type\Atomic\TKeyedArray;
 use Psalm\Type\Union;
+use RuntimeException;
+use SplObjectStorage;
 use Twig\Environment;
 
 /**
@@ -26,25 +30,60 @@ public static function afterMethodCallAnalysis(Expr $expr, string $method_id, st
         if (
             null === $codebase->taint_flow_graph
             || !$expr instanceof MethodCall || $method_id !== Environment::class.'::render' || empty($expr->args)
-            || !isset($expr->args[0]->value) || !$expr->args[0]->value instanceof String_
-            || !isset($expr->args[1]->value) || !$expr->args[1]->value instanceof Array_
+            || !isset($expr->args[0]->value)
+            || !isset($expr->args[1]->value)
         ) {
             return;
         }
 
-        $template_name = $expr->args[0]->value->value;
-        $twig_arguments_type = $statements_source->getNodeTypeProvider()->getType($expr->args[1]->value);
+        $templateName = TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source);
+        $templateParameters = self::generateTemplateParameters($expr->args[1]->value, $statements_source);
+        foreach ($templateParameters as $sourceNode) {
+            $parameterName = $templateParameters[$sourceNode];
+            $label = $argumentId = strtolower($templateName).'#'.strtolower($parameterName);
+            $destinationNode = new DataFlowNode($argumentId, $label, null, null);
 
-        if (null === $twig_arguments_type) {
-            return;
+            $codebase->taint_flow_graph->addPath($sourceNode, $destinationNode, 'arg');
+        }
+    }
+
+    /**
+     * @return SplObjectStorage<DataFlowNode, string>
+     */
+    private static function generateTemplateParameters(Expr $templateParameters, StatementsSource $source): SplObjectStorage
+    {
+        $type = $source->getNodeTypeProvider()->getType($templateParameters);
+        if (null === $type) {
+            throw new RuntimeException(sprintf('Can not retrieve type for the given expression (%s)', get_class($templateParameters)));
         }
 
-        foreach ($twig_arguments_type->parent_nodes as $source_taint) {
-            preg_match('/array\[\'([a-zA-Z]+)\'\]/', $source_taint->label, $matches);
-            $sink_taint = TemplateFileAnalyzer::getTaintNodeForTwigNamedVariable(
-                $template_name, $matches[1]
-            );
-            $codebase->taint_flow_graph->addPath($source_taint, $sink_taint, 'arg');
+        if ($templateParameters instanceof Array_) {
+            /** @var SplObjectStorage<DataFlowNode, string> $parameters */
+            $parameters = new SplObjectStorage();
+            foreach ($type->parent_nodes as $node) {
+                if (preg_match('/array\[\'([a-zA-Z]+)\'\]/', $node->label, $matches)) {
+                    $parameters[$node] = $matches[1];
+                }
+            }
+
+            return $parameters;
         }
+
+        if ($templateParameters instanceof Variable && array_key_exists('array', $type->getAtomicTypes())) {
+            /** @var TKeyedArray $arrayValues */
+            $arrayValues = $type->getAtomicTypes()['array'];
+
+            /** @var SplObjectStorage<DataFlowNode, string> $parameters */
+            $parameters = new SplObjectStorage();
+            foreach ($arrayValues->properties as $parameterName => $parameterType) {
+                foreach ($parameterType->parent_nodes as $node) {
+                    $parameters[$node] = (string) $parameterName;
+                }
+            }
+
+            return $parameters;
+        }
+
+        throw new RuntimeException(sprintf('Can not retrieve template parameters from given expression (%s)', get_class($templateParameters)));
     }
 }

src/Twig/CachedTemplatesTainter.php

@@ -7,7 +7,6 @@
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Identifier;
-use PhpParser\Node\Scalar\String_;
 use Psalm\CodeLocation;
 use Psalm\Context;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer;
@@ -59,12 +58,8 @@ public static function getMethodReturnType(
             isset($call_args[1]) ? [$call_args[1]] : []
         );
 
-        $firstArgument = $call_args[0]->value;
-        if (!$firstArgument instanceof String_) {
-            return null;
-        }
-
-        $cacheClassName = CachedTemplatesMapping::getCacheClassName($firstArgument->value);
+        $templateName = TwigUtils::extractTemplateNameFromExpression($call_args[0]->value, $source);
+        $cacheClassName = CachedTemplatesMapping::getCacheClassName($templateName);
 
         $context->vars_in_scope['$__fake_twig_env_var__'] = new Union([
             new TNamedObject($cacheClassName),

src/Twig/TemplateFileAnalyzer.php

@@ -11,6 +11,11 @@
 use Twig\Loader\FilesystemLoader;
 use Twig\NodeTraverser;
 
+/**
+ * This class is to be used as a "checker" for the `.twig` files in the psalm configuration.
+ *
+ * @psalm-suppress UnusedClass
+ */
 class TemplateFileAnalyzer extends FileAnalyzer
 {
     public function analyze(

src/Twig/TwigUtils.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\SymfonyPsalmPlugin\Twig;
+
+use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\Variable;
+use PhpParser\Node\Scalar\String_;
+use Psalm\StatementsSource;
+use Psalm\Type\Atomic\TLiteralString;
+use Psalm\Type\Atomic\TNull;
+use Psalm\Type\Union;
+use RuntimeException;
+
+class TwigUtils
+{
+    public static function extractTemplateNameFromExpression(Expr $templateName, StatementsSource $source): string
+    {
+        if ($templateName instanceof Variable) {
+            $type = $source->getNodeTypeProvider()->getType($templateName) ?? new Union([new TNull()]);
+            $templateName = array_values($type->getAtomicTypes())[0];
+        }
+
+        if (!$templateName instanceof String_ && !$templateName instanceof TLiteralString) {
+            throw new RuntimeException(sprintf('Can not retrieve template name from given expression (%s)', get_class($templateName)));
+        }
+
+        return $templateName->value;
+    }
+}

tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature

@@ -83,6 +83,26 @@ Feature: Twig tainting with analyzer
       | TaintedInput | Detected tainted html |
     And I see no other errors
 
+  Scenario: One tainted parameter (in a variable) of the twig template (named in a variable) is displayed with only the raw filter
+    Given I have the following code
+      """
+      $untrustedParameters = ['untrusted' => $_GET['untrusted']];
+      $template = 'index.html.twig';
+
+      twig()->render($template, $untrustedParameters);
+      """
+    And I have the following "index.html.twig" template
+      """
+      <h1>
+        {{ untrusted|raw }}
+      </h1>
+      """
+    When I run Psalm with taint analysis
+    Then I see these errors
+      | Type         | Message               |
+      | TaintedInput | Detected tainted html |
+    And I see no other errors
+
   Scenario: One tainted parameter of the twig rendering is displayed with some filter followed by the raw filter
     Given I have the following code
       """

tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature

@@ -86,6 +86,27 @@ Feature: Twig tainting with cached templates
       | TaintedInput | Detected tainted html |
     And I see no other errors
 
+  Scenario: One tainted parameter (in a variable) of the twig template (named in a variable) is displayed with only the raw filter
+    Given I have the following code
+      """
+      $untrustedParameters = ['untrusted' => $_GET['untrusted']];
+      $template = 'index.html.twig';
+
+      twig()->render($template, $untrustedParameters);
+      """
+    And I have the following "index.html.twig" template
+      """
+      <h1>
+        {{ untrusted|raw }}
+      </h1>
+      """
+    And the "index.html.twig" template is compiled in the "cache/twig/" directory
+    When I run Psalm with taint analysis
+    Then I see these errors
+      | Type         | Message               |
+      | TaintedInput | Detected tainted html |
+    And I see no other errors
+
   Scenario: The template has a taint sink and is aliased
     Given I have the following code
       """

tests/unit/Symfony/TwigUtilsTest.php

@@ -0,0 +1,90 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\SymfonyPsalmPlugin\Tests\Symfony;
+
+use PhpParser\Node\Expr\FuncCall;
+use PHPUnit\Framework\Assert;
+use PHPUnit\Framework\TestCase;
+use Psalm\Codebase;
+use Psalm\Config;
+use Psalm\Context;
+use Psalm\Internal\Analyzer\FileAnalyzer;
+use Psalm\Internal\Analyzer\ProjectAnalyzer;
+use Psalm\Internal\Analyzer\StatementsAnalyzer;
+use Psalm\Internal\Provider\FileProvider;
+use Psalm\Internal\Provider\NodeDataProvider;
+use Psalm\Internal\Provider\Providers;
+use Psalm\Internal\Provider\StatementsProvider;
+use Psalm\Plugin\Hook\AfterEveryFunctionCallAnalysisInterface;
+use Psalm\StatementsSource;
+use Psalm\Storage\FunctionStorage;
+use Psalm\SymfonyPsalmPlugin\Twig\TwigUtils;
+use RuntimeException;
+
+class TwigUtilsTest extends TestCase
+{
+    /**
+     * @dataProvider provideExpressions
+     */
+    public function testItCanExtractTheTemplateNameFromAnExpression(string $expression)
+    {
+        $code = '<?php'."\n".$expression;
+        $statements = StatementsProvider::parseStatements($code, '7.1');
+
+        $assertionHook = new class() implements AfterEveryFunctionCallAnalysisInterface {
+            public static function afterEveryFunctionCallAnalysis(FuncCall $expr, string $function_id, Context $context, StatementsSource $statements_source, Codebase $codebase): void
+            {
+                Assert::assertSame('expected.twig', TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source));
+            }
+        };
+
+        $statementsAnalyzer = self::createStatementsAnalyzer($assertionHook);
+        $statementsAnalyzer->analyze($statements, new Context());
+    }
+
+    public function provideExpressions(): array
+    {
+        return [
+            ['dummy("expected.twig");'],
+            ['dummy(\'expected.twig\');'],
+            ['$a = "expected.twig"; dummy($a);'],
+        ];
+    }
+
+    public function testItThrowsAnExceptionWhenTryingToExtractTemplateNameFromAnUnsupportedExpression()
+    {
+        $code = '<?php'."\n".'dummy(123);';
+        $statements = StatementsProvider::parseStatements($code, '7.1');
+
+        $assertionHook = new class() implements AfterEveryFunctionCallAnalysisInterface {
+            public static function afterEveryFunctionCallAnalysis(FuncCall $expr, string $function_id, Context $context, StatementsSource $statements_source, Codebase $codebase): void
+            {
+                TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source);
+            }
+        };
+
+        $statementsAnalyzer = self::createStatementsAnalyzer($assertionHook);
+
+        $this->expectException(RuntimeException::class);
+        $statementsAnalyzer->analyze($statements, new Context());
+    }
+
+    private static function createStatementsAnalyzer(AfterEveryFunctionCallAnalysisInterface $hook): StatementsAnalyzer
+    {
+        $config = (function () { return new self(); })->bindTo(null, Config::class)();
+        $config->after_every_function_checks[] = $hook;
+
+        $nullFileAnalyzer = new FileAnalyzer(new ProjectAnalyzer($config, new Providers(new FileProvider())), '', '');
+        $nullFileAnalyzer->codebase->functions->addGlobalFunction('dummy', new FunctionStorage());
+        $nullFileAnalyzer->codebase->file_storage_provider->create('');
+
+        $nodeData = new NodeDataProvider();
+        (function () use ($nodeData) {
+            $this->node_data = $nodeData;
+        })->bindTo($nullFileAnalyzer, $nullFileAnalyzer)();
+
+        return new StatementsAnalyzer($nullFileAnalyzer, $nodeData);
+    }
+}