[tainting] Twig print should not be an actual taint sink (#123)

* Twig print should not be a sink

* Add links to the test cases for tainting twig

* Update psalm

* Force typing of Request:: to ensure taint detection

* Fix test using old hooks mechanism.

README.md

@@ -92,6 +92,8 @@ To leverage the real Twig file analyzer, you have to configure a checker for the
 </fileExtensions>
 ```
 
+[See the currently supported cases.](https://github.com/psalm/psalm-plugin-symfony/blob/master/tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature)
+
 #### Cache Analyzer
 
 This approach is "dirtier", since it tries to connect the taints from the application code to the compiled PHP code representing a given template.
@@ -105,6 +107,8 @@ To allow the analysis through the cached template files, you have to add the `tw
 </pluginClass>
 ```
 
+[See the currently supported cases.](https://github.com/psalm/psalm-plugin-symfony/blob/master/tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature)
+
 ### Credits
 
 - Plugin created by [@seferov](https://github.com/seferov)

composer.json

@@ -13,7 +13,7 @@
         "php": "^7.1 || ^8.0",
         "ext-simplexml": "*",
         "symfony/framework-bundle": "^3.0 || ^4.0 || ^5.0",
-        "vimeo/psalm": "^4.1 <4.3.2"
+        "vimeo/psalm": "^4.4.1"
     },
     "require-dev": {
         "doctrine/orm": "^2.7",

src/Stubs/5/Request.stubphp

@@ -0,0 +1,11 @@
+<?php
+
+namespace Symfony\Component\HttpFoundation;
+
+class Request
+{
+    /**
+     * @psalm-var InputBag
+     */
+    public $request;
+}

src/Twig/AnalyzedTemplatesTainter.php

@@ -37,6 +37,8 @@ class AnalyzedTemplatesTainter implements AfterMethodCallAnalysisInterface
         }
 
         $templateName = TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source);
+
+        // Taints going _in_ the template
         $templateParameters = self::generateTemplateParameters($expr->args[1]->value, $statements_source);
         foreach ($templateParameters as $sourceNode) {
             $parameterName = $templateParameters[$sourceNode];
@@ -45,6 +47,14 @@ class AnalyzedTemplatesTainter implements AfterMethodCallAnalysisInterface
 
             $codebase->taint_flow_graph->addPath($sourceNode, $destinationNode, 'arg');
         }
+
+        // Taints going _out_ of the template
+        $source = new DataFlowNode($templateName, $templateName, null);
+        if (null !== $return_type_candidate) {
+            foreach ($return_type_candidate->parent_nodes as $sink) {
+                $codebase->taint_flow_graph->addPath($source, $sink, '=');
+            }
+        }
     }
 
     /**

src/Twig/Context.php

@@ -7,8 +7,6 @@ namespace Psalm\SymfonyPsalmPlugin\Twig;
 use Psalm\CodeLocation;
 use Psalm\Internal\Codebase\TaintFlowGraph;
 use Psalm\Internal\DataFlow\DataFlowNode;
-use Psalm\Internal\DataFlow\TaintSink;
-use Psalm\Internal\DataFlow\TaintSource;
 use Psalm\Type\TaintKind;
 use Twig\Node\Expression\FilterExpression;
 use Twig\Node\Expression\NameExpression;
@@ -30,6 +28,9 @@ class Context
     /** @var TaintFlowGraph */
     private $taint;
 
+    /** @var array<DataFlowNode> */
+    private $parentNodes = [];
+
     public function __construct(Source $sourceContext, TaintFlowGraph $taint)
     {
         $this->sourceContext = $sourceContext;
@@ -45,7 +46,7 @@ class Context
             $sinkName = 'twig_print';
         }
 
-        $sink = TaintSink::getForMethodArgument(
+        $sink = DataFlowNode::getForMethodArgument(
             $sinkName, $sinkName, 0, null, $codeLocation
         );
 
@@ -55,8 +56,9 @@ class Context
             TaintKind::SYSTEM_SECRET,
         ];
 
-        $this->taint->addSink($sink);
+        $this->taint->addNode($sink);
         $this->taint->addPath($source, $sink, 'arg');
+        $this->parentNodes[] = $sink;
     }
 
     public function taintVariable(NameExpression $expression): DataFlowNode
@@ -64,7 +66,7 @@ class Context
         /** @var string $variableName */
         $variableName = $expression->getAttribute('name');
 
-        $sinkNode = TaintSource::getForAssignment($variableName, $this->getNodeLocation($expression));
+        $sinkNode = DataFlowNode::getForAssignment($variableName, $this->getNodeLocation($expression));
 
         $this->taint->addNode($sinkNode);
         $sinkNode = $this->addVariableTaintNode($expression);
@@ -78,7 +80,7 @@ class Context
         $filterName = $expression->getNode('filter')->getAttribute('value');
 
         $returnLocation = $this->getNodeLocation($expression);
-        $taintDestination = TaintSource::getForMethodReturn('filter_'.$filterName, 'filter_'.$filterName, $returnLocation, $returnLocation);
+        $taintDestination = DataFlowNode::getForMethodReturn('filter_'.$filterName, 'filter_'.$filterName, $returnLocation, $returnLocation);
 
         $this->taint->addNode($taintDestination);
         $this->taint->addPath($taintSource, $taintDestination, 'arg');
@@ -110,18 +112,27 @@ class Context
     {
         foreach ($this->unassignedVariables as $variableName => $taintable) {
             $label = strtolower($templateName).'#'.strtolower($variableName);
-            $taintSource = new TaintSource($label, $label, null, null);
+            $taintSource = new DataFlowNode($label, $label, null);
 
             $this->taint->addNode($taintSource);
             $this->taint->addPath($taintSource, $taintable, 'arg');
         }
     }
 
+    public function taintSinks(string $templateName): void
+    {
+        $sink = new DataFlowNode($templateName, $templateName, null);
+        $this->taint->addNode($sink);
+        foreach ($this->parentNodes as $source) {
+            $this->taint->addPath($source, $sink, 'return');
+        }
+    }
+
     private function addVariableTaintNode(NameExpression $variableNode): DataFlowNode
     {
         /** @var string $variableName */
         $variableName = $variableNode->getAttribute('name');
-        $taintNode = TaintSource::getForAssignment($variableName, $this->getNodeLocation($variableNode));
+        $taintNode = DataFlowNode::getForAssignment($variableName, $this->getNodeLocation($variableNode));
 
         $this->taint->addNode($taintNode);
 

src/Twig/TemplateFileAnalyzer.php

@@ -20,7 +20,6 @@ class TemplateFileAnalyzer extends FileAnalyzer
 {
     public function analyze(
         PsalmContext $file_context = null,
-        bool $preserve_analyzers = false,
         PsalmContext $global_context = null
     ): void {
         $codebase = $this->project_analyzer->getCodebase();
@@ -52,6 +51,7 @@ class TemplateFileAnalyzer extends FileAnalyzer
         $traverser->traverse($tree);
 
         $twigContext->taintUnassignedVariables($local_file_name);
+        $twigContext->taintSinks($local_file_name);
     }
 
     public static function getTaintNodeForTwigNamedVariable(

tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature

@@ -54,7 +54,7 @@ Feature: Twig tainting with analyzer
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
-      twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
       """
     And I have the following "index.html.twig" template
       """
@@ -65,7 +65,7 @@ Feature: Twig tainting with analyzer
     When I run Psalm with taint analysis
     And I see no errors
 
-  Scenario: One tainted parameter of the twig template is displayed with only the raw filter
+  Scenario: One tainted parameter of the twig template is rendered with only the raw filter but the not displayed
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
@@ -78,6 +78,21 @@ Feature: Twig tainting with analyzer
       </h1>
       """
     When I run Psalm with taint analysis
+    And I see no errors
+
+  Scenario: One tainted parameter of the twig template is displayed with only the raw filter
+    Given I have the following code
+      """
+      $untrusted = $_GET['untrusted'];
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      """
+    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               |
       | TaintedHtml  | Detected tainted HTML |
@@ -89,7 +104,7 @@ Feature: Twig tainting with analyzer
       $untrustedParameters = ['untrusted' => $_GET['untrusted']];
       $template = 'index.html.twig';
 
-      twig()->render($template, $untrustedParameters);
+      echo twig()->render($template, $untrustedParameters);
       """
     And I have the following "index.html.twig" template
       """
@@ -107,7 +122,7 @@ Feature: Twig tainting with analyzer
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
-      twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
       """
     And I have the following "index.html.twig" template
       """
@@ -125,7 +140,7 @@ Feature: Twig tainting with analyzer
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
-      twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
       """
     And I have the following "index.html.twig" template
       """
@@ -140,7 +155,7 @@ Feature: Twig tainting with analyzer
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
-      twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
       """
     And I have the following "base.html.twig" template
       """
@@ -167,7 +182,7 @@ Feature: Twig tainting with analyzer
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
-      twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
       """
     And I have the following "index.html.twig" template
       """
@@ -187,7 +202,7 @@ Feature: Twig tainting with analyzer
     Given I have the following code
       """
       $untrusted = $_GET['untrusted'];
-      twig()->render('index.html.twig', ['untrusted' => $untrusted]);
+      echo twig()->render('index.html.twig', ['untrusted' => $untrusted]);
       """
     And I have the following "index.html.twig" template
       """

tests/unit/Symfony/TwigUtilsTest.php

@@ -4,10 +4,8 @@ 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;
@@ -17,8 +15,8 @@ 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\Plugin\EventHandler\AfterEveryFunctionCallAnalysisInterface;
+use Psalm\Plugin\EventHandler\Event\AfterEveryFunctionCallAnalysisEvent;
 use Psalm\Storage\FunctionStorage;
 use Psalm\SymfonyPsalmPlugin\Twig\TwigUtils;
 use RuntimeException;
@@ -34,9 +32,12 @@ class TwigUtilsTest extends TestCase
         $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
+            public static function afterEveryFunctionCallAnalysis(AfterEveryFunctionCallAnalysisEvent $event): void
             {
-                Assert::assertSame('expected.twig', TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source));
+                Assert::assertSame('expected.twig', TwigUtils::extractTemplateNameFromExpression(
+                    $event->getExpr()->args[0]->value,
+                    $event->getStatementsSource()
+                ));
             }
         };
 
@@ -61,9 +62,12 @@ class TwigUtilsTest extends TestCase
         $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
+            public static function afterEveryFunctionCallAnalysis(AfterEveryFunctionCallAnalysisEvent $event): void
             {
-                TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source);
+                TwigUtils::extractTemplateNameFromExpression(
+                    $event->getExpr()->args[0]->value,
+                    $event->getStatementsSource()
+                );
             }
         };
 
@@ -75,8 +79,9 @@ class TwigUtilsTest extends TestCase
 
     private static function createStatementsAnalyzer(AfterEveryFunctionCallAnalysisInterface $hook): StatementsAnalyzer
     {
+        /** @var Config $config */
         $config = (function () { return new self(); })->bindTo(null, Config::class)();
-        $config->after_every_function_checks[] = $hook;
+        $config->eventDispatcher->registerClass(get_class($hook));
 
         $nullFileAnalyzer = new FileAnalyzer(new ProjectAnalyzer($config, new Providers(new FileProvider())), '', '');
         $nullFileAnalyzer->codebase->functions->addGlobalFunction('dummy', new FunctionStorage());