Merge pull request #10409 from nicelocal/fix_backtick_analysis

Fix backtick analysis
vimeo · Nov 26, 2023 · 61405e4 · 61405e4
2 parents b654545 + 73a340f
commit 61405e4
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 81 deletions.
diff --git a/dictionaries/PropertyMap.php b/dictionaries/PropertyMap.php
@@ -384,7 +384,7 @@
         'items' => 'array<int, PhpParser\\Node\\Expr\\ArrayItem|null>',
     ],
     'phpparser\\node\\expr\\shellexec' => [
-        'parts' => 'list<PhpParser\\Node>',
+        'parts' => 'list<PhpParser\\Node\\Expr>',
     ],
     'phpparser\\node\\matcharm' => [
         'conds' => 'null|non-empty-list<PhpParser\\Node\\Expr>',

diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php
@@ -13,7 +13,6 @@
 use Psalm\Internal\Analyzer\Statements\Expression\BinaryOpAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\BitwiseNotAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\BooleanNotAnalyzer;
-use Psalm\Internal\Analyzer\Statements\Expression\Call\ArgumentAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\NewAnalyzer;
@@ -43,20 +42,18 @@
 use Psalm\Internal\Analyzer\Statements\Expression\YieldAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\YieldFromAnalyzer;
 use Psalm\Internal\Analyzer\StatementsAnalyzer;
-use Psalm\Internal\Codebase\TaintFlowGraph;
-use Psalm\Internal\DataFlow\DataFlowNode;
-use Psalm\Internal\DataFlow\TaintSink;
 use Psalm\Internal\FileManipulation\FileManipulationBuffer;
 use Psalm\Internal\Type\TemplateResult;
-use Psalm\Issue\ForbiddenCode;
 use Psalm\Issue\UnrecognizedExpression;
 use Psalm\Issue\UnsupportedReferenceUsage;
 use Psalm\IssueBuffer;
+use Psalm\Node\Expr\VirtualFuncCall;
+use Psalm\Node\Scalar\VirtualEncapsed;
+use Psalm\Node\VirtualArg;
+use Psalm\Node\VirtualName;
 use Psalm\Plugin\EventHandler\Event\AfterExpressionAnalysisEvent;
 use Psalm\Plugin\EventHandler\Event\BeforeExpressionAnalysisEvent;
-use Psalm\Storage\FunctionLikeParameter;
 use Psalm\Type;
-use Psalm\Type\TaintKind;
 
 use function get_class;
 use function in_array;
@@ -378,80 +375,20 @@ private static function handleExpression(
         }
 
         if ($stmt instanceof PhpParser\Node\Expr\ShellExec) {
-            if ($statements_analyzer->data_flow_graph) {
-                $call_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
-
-                if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) {
-                    $sink = TaintSink::getForMethodArgument(
-                        'shell_exec',
-                        'shell_exec',
-                        0,
-                        null,
-                        $call_location,
-                    );
-
-                    $sink->taints = [TaintKind::INPUT_SHELL];
-
-                    $statements_analyzer->data_flow_graph->addSink($sink);
-                }
-
-                foreach ($stmt->parts as $part) {
-                    if ($part instanceof PhpParser\Node\Expr\Variable) {
-                        if (self::analyze($statements_analyzer, $part, $context) === false) {
-                            break;
-                        }
-
-                        $expr_type = $statements_analyzer->node_data->getType($part);
-                        if ($expr_type === null) {
-                            break;
-                        }
-
-                        $shell_exec_param = new FunctionLikeParameter(
-                            'var',
-                            false,
-                        );
-
-                        if (ArgumentAnalyzer::verifyType(
-                            $statements_analyzer,
-                            $expr_type,
-                            Type::getString(),
-                            null,
-                            'shell_exec',
-                            null,
-                            0,
-                            $call_location,
-                            $stmt,
-                            $context,
-                            $shell_exec_param,
-                            false,
-                            null,
-                            true,
-                            true,
-                            new CodeLocation($statements_analyzer, $stmt),
-                        ) === false) {
-                            return false;
-                        }
-
-                        foreach ($expr_type->parent_nodes as $parent_node) {
-                            $statements_analyzer->data_flow_graph->addPath(
-                                $parent_node,
-                                new DataFlowNode('variable-use', 'variable use', null),
-                                'variable-use',
-                            );
-                        }
-                    }
-                }
-            }
-
-            IssueBuffer::maybeAdd(
-                new ForbiddenCode(
-                    'Use of shell_exec',
-                    new CodeLocation($statements_analyzer->getSource(), $stmt),
-                ),
-                $statements_analyzer->getSuppressedIssues(),
+            $concat = new VirtualEncapsed($stmt->parts, $stmt->getAttributes());
+            $virtual_call = new VirtualFuncCall(new VirtualName(['shell_exec']), [
+                new VirtualArg($concat),
+            ], $stmt->getAttributes());
+            return self::handleExpression(
+                $statements_analyzer,
+                $virtual_call,
+                $context,
+                $array_assignment,
+                $global_context,
+                $from_stmt,
+                $template_result,
+                $assigned_to_reference,
             );
-
-            return true;
         }
 
         if ($stmt instanceof PhpParser\Node\Expr\Print_) {

diff --git a/tests/ExpressionTest.php b/tests/ExpressionTest.php
@@ -51,6 +51,14 @@ public function providerValidCodeParse(): iterable
                 '$a===' => 'array{9223372036854775806: 0, 9223372036854775807: 1}',
             ],
         ];
+        yield 'shellExecConcatInt' => [
+            'code' => <<<'PHP'
+                <?php
+                $a = 123;
+                /** @psalm-suppress ForbiddenCode */
+                `ls $a`;
+                PHP,
+        ];
     }
 
     /**

diff --git a/tests/TaintTest.php b/tests/TaintTest.php
@@ -2306,6 +2306,14 @@ function foo(array $arr) : void {
                     ',
                 'error_message' => 'TaintedShell',
             ],
+            'shellExecBacktickConcat' => [
+                'code' => '<?php
+
+                    $input = $_GET["input"];
+                    $x = `ls $input`;
+                    ',
+                'error_message' => 'TaintedShell',
+            ],
             /*
             // TODO: Stubs do not support this type of inference even with $this->message = $message.
             // Most uses of getMessage() would be with caught exceptions, so this is not representative of real code.