Skip to content

Commit 0397c58

Browse files
adrienlucasadrienlucas
authored
[tainting] Allow Twig\Environment::render to be tainted even with a variable 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
1 parent f75effe commit 0397c58

File tree

7 files changed

+221
-20
lines changed

7 files changed

+221
-20
lines changed

‎src/Twig/AnalyzedTemplatesTainter.php

Lines changed: 52 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,16 @@
77
use PhpParser\Node\Expr;
88
use PhpParser\Node\Expr\Array_;
99
use PhpParser\Node\Expr\MethodCall;
10-
use PhpParser\Node\Scalar\String_;
10+
use PhpParser\Node\Expr\Variable;
1111
use Psalm\Codebase;
1212
use Psalm\Context;
13+
use Psalm\Internal\DataFlow\DataFlowNode;
1314
use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface;
1415
use Psalm\StatementsSource;
16+
use Psalm\Type\Atomic\TKeyedArray;
1517
use Psalm\Type\Union;
18+
use RuntimeException;
19+
use SplObjectStorage;
1620
use Twig\Environment;
1721

1822
/**
@@ -26,25 +30,60 @@ public static function afterMethodCallAnalysis(Expr $expr, string $method_id, st
2630
if (
2731
null === $codebase->taint_flow_graph
2832
|| !$expr instanceof MethodCall || $method_id !== Environment::class.'::render' || empty($expr->args)
29-
|| !isset($expr->args[0]->value) || !$expr->args[0]->value instanceof String_
30-
|| !isset($expr->args[1]->value) || !$expr->args[1]->value instanceof Array_
33+
|| !isset($expr->args[0]->value)
34+
|| !isset($expr->args[1]->value)
3135
) {
3236
return;
3337
}
3438

35-
$template_name = $expr->args[0]->value->value;
36-
$twig_arguments_type = $statements_source->getNodeTypeProvider()->getType($expr->args[1]->value);
39+
$templateName = TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source);
40+
$templateParameters = self::generateTemplateParameters($expr->args[1]->value, $statements_source);
41+
foreach ($templateParameters as $sourceNode) {
42+
$parameterName = $templateParameters[$sourceNode];
43+
$label = $argumentId = strtolower($templateName).'#'.strtolower($parameterName);
44+
$destinationNode = new DataFlowNode($argumentId, $label, null, null);
3745

38-
if (null === $twig_arguments_type) {
39-
return;
46+
$codebase->taint_flow_graph->addPath($sourceNode, $destinationNode, 'arg');
47+
}
48+
}
49+
50+
/**
51+
* @return SplObjectStorage<DataFlowNode, string>
52+
*/
53+
private static function generateTemplateParameters(Expr $templateParameters, StatementsSource $source): SplObjectStorage
54+
{
55+
$type = $source->getNodeTypeProvider()->getType($templateParameters);
56+
if (null === $type) {
57+
throw new RuntimeException(sprintf('Can not retrieve type for the given expression (%s)', get_class($templateParameters)));
4058
}
4159

42-
foreach ($twig_arguments_type->parent_nodes as $source_taint) {
43-
preg_match('/array\[\'([a-zA-Z]+)\'\]/', $source_taint->label, $matches);
44-
$sink_taint = TemplateFileAnalyzer::getTaintNodeForTwigNamedVariable(
45-
$template_name, $matches[1]
46-
);
47-
$codebase->taint_flow_graph->addPath($source_taint, $sink_taint, 'arg');
60+
if ($templateParameters instanceof Array_) {
61+
/** @var SplObjectStorage<DataFlowNode, string> $parameters */
62+
$parameters = new SplObjectStorage();
63+
foreach ($type->parent_nodes as $node) {
64+
if (preg_match('/array\[\'([a-zA-Z]+)\'\]/', $node->label, $matches)) {
65+
$parameters[$node] = $matches[1];
66+
}
67+
}
68+
69+
return $parameters;
4870
}
71+
72+
if ($templateParameters instanceof Variable && array_key_exists('array', $type->getAtomicTypes())) {
73+
/** @var TKeyedArray $arrayValues */
74+
$arrayValues = $type->getAtomicTypes()['array'];
75+
76+
/** @var SplObjectStorage<DataFlowNode, string> $parameters */
77+
$parameters = new SplObjectStorage();
78+
foreach ($arrayValues->properties as $parameterName => $parameterType) {
79+
foreach ($parameterType->parent_nodes as $node) {
80+
$parameters[$node] = (string) $parameterName;
81+
}
82+
}
83+
84+
return $parameters;
85+
}
86+
87+
throw new RuntimeException(sprintf('Can not retrieve template parameters from given expression (%s)', get_class($templateParameters)));
4988
}
5089
}

‎src/Twig/CachedTemplatesTainter.php

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
use PhpParser\Node\Expr\MethodCall;
88
use PhpParser\Node\Expr\Variable;
99
use PhpParser\Node\Identifier;
10-
use PhpParser\Node\Scalar\String_;
1110
use Psalm\CodeLocation;
1211
use Psalm\Context;
1312
use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer;
@@ -59,12 +58,8 @@ public static function getMethodReturnType(
5958
isset($call_args[1]) ? [$call_args[1]] : []
6059
);
6160

62-
$firstArgument = $call_args[0]->value;
63-
if (!$firstArgument instanceof String_) {
64-
return null;
65-
}
66-
67-
$cacheClassName = CachedTemplatesMapping::getCacheClassName($firstArgument->value);
61+
$templateName = TwigUtils::extractTemplateNameFromExpression($call_args[0]->value, $source);
62+
$cacheClassName = CachedTemplatesMapping::getCacheClassName($templateName);
6863

6964
$context->vars_in_scope['$__fake_twig_env_var__'] = new Union([
7065
new TNamedObject($cacheClassName),

‎src/Twig/TemplateFileAnalyzer.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@
1111
use Twig\Loader\FilesystemLoader;
1212
use Twig\NodeTraverser;
1313

14+
/**
15+
* This class is to be used as a "checker" for the `.twig` files in the psalm configuration.
16+
*
17+
* @psalm-suppress UnusedClass
18+
*/
1419
class TemplateFileAnalyzer extends FileAnalyzer
1520
{
1621
public function analyze(

‎src/Twig/TwigUtils.php

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Psalm\SymfonyPsalmPlugin\Twig;
6+
7+
use PhpParser\Node\Expr;
8+
use PhpParser\Node\Expr\Variable;
9+
use PhpParser\Node\Scalar\String_;
10+
use Psalm\StatementsSource;
11+
use Psalm\Type\Atomic\TLiteralString;
12+
use Psalm\Type\Atomic\TNull;
13+
use Psalm\Type\Union;
14+
use RuntimeException;
15+
16+
class TwigUtils
17+
{
18+
public static function extractTemplateNameFromExpression(Expr $templateName, StatementsSource $source): string
19+
{
20+
if ($templateName instanceof Variable) {
21+
$type = $source->getNodeTypeProvider()->getType($templateName) ?? new Union([new TNull()]);
22+
$templateName = array_values($type->getAtomicTypes())[0];
23+
}
24+
25+
if (!$templateName instanceof String_ && !$templateName instanceof TLiteralString) {
26+
throw new RuntimeException(sprintf('Can not retrieve template name from given expression (%s)', get_class($templateName)));
27+
}
28+
29+
return $templateName->value;
30+
}
31+
}

‎tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,26 @@ Feature: Twig tainting with analyzer
8383
| TaintedInput | Detected tainted html |
8484
And I see no other errors
8585

86+
Scenario: One tainted parameter (in a variable) of the twig template (named in a variable) is displayed with only the raw filter
87+
Given I have the following code
88+
"""
89+
$untrustedParameters = ['untrusted' => $_GET['untrusted']];
90+
$template = 'index.html.twig';
91+
92+
twig()->render($template, $untrustedParameters);
93+
"""
94+
And I have the following "index.html.twig" template
95+
"""
96+
<h1>
97+
{{ untrusted|raw }}
98+
</h1>
99+
"""
100+
When I run Psalm with taint analysis
101+
Then I see these errors
102+
| Type | Message |
103+
| TaintedInput | Detected tainted html |
104+
And I see no other errors
105+
86106
Scenario: One tainted parameter of the twig rendering is displayed with some filter followed by the raw filter
87107
Given I have the following code
88108
"""

‎tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,27 @@ Feature: Twig tainting with cached templates
8686
| TaintedInput | Detected tainted html |
8787
And I see no other errors
8888

89+
Scenario: One tainted parameter (in a variable) of the twig template (named in a variable) is displayed with only the raw filter
90+
Given I have the following code
91+
"""
92+
$untrustedParameters = ['untrusted' => $_GET['untrusted']];
93+
$template = 'index.html.twig';
94+
95+
twig()->render($template, $untrustedParameters);
96+
"""
97+
And I have the following "index.html.twig" template
98+
"""
99+
<h1>
100+
{{ untrusted|raw }}
101+
</h1>
102+
"""
103+
And the "index.html.twig" template is compiled in the "cache/twig/" directory
104+
When I run Psalm with taint analysis
105+
Then I see these errors
106+
| Type | Message |
107+
| TaintedInput | Detected tainted html |
108+
And I see no other errors
109+
89110
Scenario: The template has a taint sink and is aliased
90111
Given I have the following code
91112
"""

‎tests/unit/Symfony/TwigUtilsTest.php

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Psalm\SymfonyPsalmPlugin\Tests\Symfony;
6+
7+
use PhpParser\Node\Expr\FuncCall;
8+
use PHPUnit\Framework\Assert;
9+
use PHPUnit\Framework\TestCase;
10+
use Psalm\Codebase;
11+
use Psalm\Config;
12+
use Psalm\Context;
13+
use Psalm\Internal\Analyzer\FileAnalyzer;
14+
use Psalm\Internal\Analyzer\ProjectAnalyzer;
15+
use Psalm\Internal\Analyzer\StatementsAnalyzer;
16+
use Psalm\Internal\Provider\FileProvider;
17+
use Psalm\Internal\Provider\NodeDataProvider;
18+
use Psalm\Internal\Provider\Providers;
19+
use Psalm\Internal\Provider\StatementsProvider;
20+
use Psalm\Plugin\Hook\AfterEveryFunctionCallAnalysisInterface;
21+
use Psalm\StatementsSource;
22+
use Psalm\Storage\FunctionStorage;
23+
use Psalm\SymfonyPsalmPlugin\Twig\TwigUtils;
24+
use RuntimeException;
25+
26+
class TwigUtilsTest extends TestCase
27+
{
28+
/**
29+
* @dataProvider provideExpressions
30+
*/
31+
public function testItCanExtractTheTemplateNameFromAnExpression(string $expression)
32+
{
33+
$code = '<?php'."\n".$expression;
34+
$statements = StatementsProvider::parseStatements($code, '7.1');
35+
36+
$assertionHook = new class() implements AfterEveryFunctionCallAnalysisInterface {
37+
public static function afterEveryFunctionCallAnalysis(FuncCall $expr, string $function_id, Context $context, StatementsSource $statements_source, Codebase $codebase): void
38+
{
39+
Assert::assertSame('expected.twig', TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source));
40+
}
41+
};
42+
43+
$statementsAnalyzer = self::createStatementsAnalyzer($assertionHook);
44+
$statementsAnalyzer->analyze($statements, new Context());
45+
}
46+
47+
public function provideExpressions(): array
48+
{
49+
return [
50+
['dummy("expected.twig");'],
51+
['dummy(\'expected.twig\');'],
52+
['$a = "expected.twig"; dummy($a);'],
53+
];
54+
}
55+
56+
public function testItThrowsAnExceptionWhenTryingToExtractTemplateNameFromAnUnsupportedExpression()
57+
{
58+
$code = '<?php'."\n".'dummy(123);';
59+
$statements = StatementsProvider::parseStatements($code, '7.1');
60+
61+
$assertionHook = new class() implements AfterEveryFunctionCallAnalysisInterface {
62+
public static function afterEveryFunctionCallAnalysis(FuncCall $expr, string $function_id, Context $context, StatementsSource $statements_source, Codebase $codebase): void
63+
{
64+
TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source);
65+
}
66+
};
67+
68+
$statementsAnalyzer = self::createStatementsAnalyzer($assertionHook);
69+
70+
$this->expectException(RuntimeException::class);
71+
$statementsAnalyzer->analyze($statements, new Context());
72+
}
73+
74+
private static function createStatementsAnalyzer(AfterEveryFunctionCallAnalysisInterface $hook): StatementsAnalyzer
75+
{
76+
$config = (function () { return new self(); })->bindTo(null, Config::class)();
77+
$config->after_every_function_checks[] = $hook;
78+
79+
$nullFileAnalyzer = new FileAnalyzer(new ProjectAnalyzer($config, new Providers(new FileProvider())), '', '');
80+
$nullFileAnalyzer->codebase->functions->addGlobalFunction('dummy', new FunctionStorage());
81+
$nullFileAnalyzer->codebase->file_storage_provider->create('');
82+
83+
$nodeData = new NodeDataProvider();
84+
(function () use ($nodeData) {
85+
$this->node_data = $nodeData;
86+
})->bindTo($nullFileAnalyzer, $nullFileAnalyzer)();
87+
88+
return new StatementsAnalyzer($nullFileAnalyzer, $nodeData);
89+
}
90+
}

0 commit comments

Comments
 (0)