Merge pull request #8150 from muglug/track-taints-in-static-properties

Track taints in static properties

src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php

@@ -463,9 +463,6 @@ class InstancePropertyAssignmentAnalyzer
 
         $data_flow_graph = $statements_analyzer->data_flow_graph;
 
-        $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var);
-        $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
-
         if ($class_storage->specialize_instance) {
             $var_id = ExpressionIdentifier::getExtendedVarId(
                 $stmt->var,
@@ -487,6 +484,8 @@ class InstancePropertyAssignmentAnalyzer
                     return;
                 }
 
+                $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var);
+
                 $var_node = DataFlowNode::getForAssignment(
                     $var_id,
                     $var_location
@@ -494,6 +493,8 @@ class InstancePropertyAssignmentAnalyzer
 
                 $data_flow_graph->addNode($var_node);
 
+                $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
+
                 $property_node = DataFlowNode::getForAssignment(
                     $var_property_id ?: $var_id . '->$property',
                     $property_location
@@ -547,9 +548,39 @@ class InstancePropertyAssignmentAnalyzer
                 $statements_analyzer
             );
 
-            $localized_property_node = DataFlowNode::getForAssignment(
+            self::taintUnspecializedProperty(
+                $statements_analyzer,
+                $stmt,
+                $property_id,
+                $class_storage,
+                $assignment_value_type,
+                $context,
                 $var_property_id
-                    ?: $property_id . '-' . $property_location->file_name . ':' . $property_location->raw_file_start,
+            );
+        }
+    }
+
+    public static function taintUnspecializedProperty(
+        StatementsAnalyzer $statements_analyzer,
+        PhpParser\Node\Expr $stmt,
+        string $property_id,
+        ClassLikeStorage $class_storage,
+        Union $assignment_value_type,
+        Context $context,
+        ?string $var_property_id
+    ): void {
+        $codebase = $statements_analyzer->getCodebase();
+
+        $data_flow_graph = $statements_analyzer->data_flow_graph;
+
+        if (!$data_flow_graph) {
+            return;
+        }
+
+        $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
+
+        $localized_property_node = DataFlowNode::getForAssignment(
+            $var_property_id ?: $property_id,
             $property_location
         );
 
@@ -598,6 +629,8 @@ class InstancePropertyAssignmentAnalyzer
         if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph
             && $declaring_property_class
             && $declaring_property_class !== $class_storage->name
+            && ($stmt instanceof PhpParser\Node\Expr\PropertyFetch
+                || $stmt instanceof PhpParser\Node\Expr\StaticPropertyFetch)
             && $stmt->name instanceof PhpParser\Node\Identifier
         ) {
             $declaring_property_node = new DataFlowNode(
@@ -618,7 +651,6 @@ class InstancePropertyAssignmentAnalyzer
             );
         }
     }
-    }
 
     /**
      * @return list<AssignedProperty>

src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/StaticPropertyAssignmentAnalyzer.php

@@ -196,6 +196,16 @@ class StaticPropertyAssignmentAnalyzer
                 $context->vars_in_scope[$var_id] = $assignment_value_type;
             }
 
+            InstancePropertyAssignmentAnalyzer::taintUnspecializedProperty(
+                $statements_analyzer,
+                $stmt,
+                $property_id,
+                $class_storage,
+                $assignment_value_type,
+                $context,
+                null
+            );
+
             $class_property_type = $codebase->properties->getPropertyType(
                 $property_id,
                 true,

src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php

@@ -773,9 +773,6 @@ class AtomicPropertyFetchAnalyzer
 
         $data_flow_graph = $statements_analyzer->data_flow_graph;
 
-        $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var);
-        $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
-
         $added_taints = [];
         $removed_taints = [];
 
@@ -811,6 +808,9 @@ class AtomicPropertyFetchAnalyzer
                     return;
                 }
 
+                $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var);
+                $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
+
                 $var_node = DataFlowNode::getForAssignment(
                     $var_id,
                     $var_location
@@ -849,15 +849,47 @@ class AtomicPropertyFetchAnalyzer
                 $type->parent_nodes = [$property_node->id => $property_node];
             }
         } else {
+            self::processUnspecialTaints(
+                $statements_analyzer,
+                $stmt,
+                $type,
+                $property_id,
+                $in_assignment,
+                $added_taints,
+                $removed_taints
+            );
+        }
+    }
+
+    /**
+     * @param ?array<string> $added_taints
+     * @param ?array<string> $removed_taints
+     */
+    public static function processUnspecialTaints(
+        StatementsAnalyzer $statements_analyzer,
+        PhpParser\Node\Expr $stmt,
+        Union $type,
+        string $property_id,
+        bool $in_assignment,
+        ?array $added_taints,
+        ?array $removed_taints
+    ): void {
+        if (!$statements_analyzer->data_flow_graph) {
+            return;
+        }
+
+        $data_flow_graph = $statements_analyzer->data_flow_graph;
+
         $var_property_id = ExpressionIdentifier::getExtendedVarId(
             $stmt,
             null,
             $statements_analyzer
         );
 
+        $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
+
         $localized_property_node = DataFlowNode::getForAssignment(
-                $var_property_id
+            $var_property_id ?: $property_id,
-                    ?: $property_id . '-' . $property_location->file_name . ':' . $property_location->raw_file_start,
             $property_location
         );
 
@@ -892,7 +924,6 @@ class AtomicPropertyFetchAnalyzer
 
         $type->parent_nodes = [$localized_property_node->id => $localized_property_node];
     }
-    }
 
     private static function handleEnumName(
         StatementsAnalyzer $statements_analyzer,

src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/StaticPropertyFetchAnalyzer.php

@@ -231,6 +231,16 @@ class StaticPropertyFetchAnalyzer
                 );
             }
 
+            AtomicPropertyFetchAnalyzer::processUnspecialTaints(
+                $statements_analyzer,
+                $stmt,
+                $stmt_type,
+                $property_id,
+                false,
+                [],
+                []
+            );
+
             return true;
         }
 
@@ -389,6 +399,16 @@ class StaticPropertyFetchAnalyzer
                     $stmt_type->getId()
                 );
             }
+
+            AtomicPropertyFetchAnalyzer::processUnspecialTaints(
+                $statements_analyzer,
+                $stmt,
+                $stmt_type,
+                $property_id,
+                false,
+                [],
+                []
+            );
         } else {
             $statements_analyzer->node_data->setType($stmt, Type::getMixed());
         }

tests/TaintTest.php

@@ -2349,6 +2349,22 @@ class TaintTest extends TestCase
                     }',
                 'error_message' => 'TaintedHtml',
             ],
+            'checkMemoizedStaticMethodCallTaints' => [
+                'code' => '<?php
+                    class A {
+                        private static string $prev = "";
+
+                        public static function getPrevious(string $s): string {
+                            $prev = self::$prev;
+                            self::$prev = $s;
+                            return $prev;
+                        }
+                    }
+
+                    A::getPrevious($_GET["a"]);
+                    echo A::getPrevious("foo");',
+                'error_message' => 'TaintedHtml',
+            ],
         ];
     }