fix: avoid changing the AST assign node since it's not needed for propagation [backport 2.2] (#8109)

Backport 4e3cac1 from #8096 to 2.2.

## Description

We were doing several operations on the AST ASSIGN node that were not
needed for propagation like splitting the multiple assigments and
introducing temporary vars, which cause problems with introspection
tools. This removes all these operations and only keep the ignore tag to
left-side operands when they're a tuple or subscript.

## Checklist

- [X] Change(s) are motivated and described in the PR description.
- [X] Testing strategy is described if automated tests are not included
in the PR.
- [X] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [X] Change is maintainable (easy to change, telemetry, documentation).
- [X] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [X] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [X] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

Co-authored-by: Juanjo Alvarez Martinez <[email protected]>
Co-authored-by: Federico Mon <[email protected]>

Author: 3 people

ddtrace/appsec/_iast/_ast/visitor.py

@@ -607,27 +607,9 @@ def visit_AugAssign(self, augassign_node):  # type: (ast.AugAssign) -> Any
 
     def visit_Assign(self, assign_node):  # type: (ast.Assign) -> Any
         """
-        Decompose multiple assignment into single ones and
-        check if any item in the targets list is if type Subscript and if
-        that's the case further decompose it to use a temp variable to
-        avoid assigning to a function call.
+        Add the ignore marks for left-side subscripts or list/tuples to avoid problems
+        later with the visit_Subscript node.
         """
-        # a = b = c
-        # __dd_tmp = c
-        # a = __dd_tmp
-
-        ret_nodes = []
-
-        if len(assign_node.targets) > 1:
-            # Multiple assignments, assign the value to a temporal variable
-            tmp_var_left = self._name_node(assign_node, "__dd_tmp", ctx=ast.Store())
-            assign_value = self._name_node(assign_node, "__dd_tmp", ctx=ast.Load())
-            assign_to_tmp = self._assign_node(from_node=assign_node, targets=[tmp_var_left], value=assign_node.value)
-            ret_nodes.append(assign_to_tmp)
-            self.ast_modified = True
-        else:
-            assign_value = assign_node.value  # type: ignore
-
         for target in assign_node.targets:
             if isinstance(target, ast.Subscript):
                 # We can't assign to a function call, which is anyway going to rewrite
@@ -640,22 +622,8 @@ def visit_Assign(self, assign_node):  # type: (ast.Assign) -> Any
                         element.avoid_convert = True  # type: ignore[attr-defined]
 
             # Create a normal assignment. This way we decompose multiple assignments
-            # like (a = b = c) into a = b and a = c so the transformation above
-            # is possible.
-            # Decompose it into a normal, not multiple, assignment
-            new_assign_value = copy.copy(assign_value)
-
-            new_target = copy.copy(target)
-
-            single_assign = self._assign_node(assign_node, [new_target], new_assign_value)
-
-            self.generic_visit(single_assign)
-            ret_nodes.append(single_assign)
-
-        if len(ret_nodes) == 1:
-            return ret_nodes[0]
-
-        return ret_nodes
+        self.generic_visit(assign_node)
+        return assign_node
 
     def visit_Delete(self, assign_node):  # type: (ast.Delete) -> Any
         # del replaced_index(foo, bar) would fail so avoid converting the right hand side

releasenotes/notes/disable-multiple-assign-02565491d5d74083.yaml

@@ -0,0 +1,3 @@
+fixes:
+  - |
+    IAST: Don't split AST Assign nodes since it's not needed for propagation to work.

tests/appsec/iast/aspects/test_other_patching.py

@@ -0,0 +1,49 @@
+import pytest
+
+
+OriginType = pytest.importorskip("ddtrace.appsec._iast._taint_tracking.OriginType")
+get_tainted_ranges = pytest.importorskip("ddtrace.appsec._iast._taint_tracking.get_tainted_ranges")
+taint_pyobject = pytest.importorskip("ddtrace.appsec._iast._taint_tracking.taint_pyobject")
+_iast_patched_module = pytest.importorskip("tests.appsec.iast.aspects.conftest._iast_patched_module")
+
+mod = _iast_patched_module("tests.appsec.iast.fixtures.aspects.str_methods")
+
+
+def test_string_assigment():
+    string_input = taint_pyobject(
+        pyobject="foo",
+        source_name="test_add_aspect_tainting_left_hand",
+        source_value="foo",
+        source_origin=OriginType.PARAMETER,
+    )
+    assert get_tainted_ranges(string_input)
+
+    res = mod.do_string_assignment(string_input)
+    assert len(get_tainted_ranges(res)) == 1
+
+
+def test_multiple_string_assigment():
+    string_input = taint_pyobject(
+        pyobject="foo",
+        source_name="test_add_aspect_tainting_left_hand",
+        source_value="foo",
+        source_origin=OriginType.PARAMETER,
+    )
+    assert get_tainted_ranges(string_input)
+
+    results = mod.do_multiple_string_assigment(string_input)
+    for res in results:
+        assert len(get_tainted_ranges(res)) == 1
+
+
+def test_multiple_tuple_string_assigment():
+    string_input = taint_pyobject(
+        pyobject="foo",
+        source_name="test_add_aspect_tainting_left_hand",
+        source_value="foo",
+        source_origin=OriginType.PARAMETER,
+    )
+    assert get_tainted_ranges(string_input)
+
+    results = mod.do_tuple_string_assignment(string_input)
+    assert len(get_tainted_ranges(results[-1])) == 1

tests/appsec/iast/fixtures/aspects/str_methods.py

@@ -54,6 +54,21 @@ def do_operator_add_params(a, b):
     return a + b
 
 
+def do_string_assignment(a):
+    b = a
+    return b
+
+
+def do_multiple_string_assigment(a):
+    b = c = d = a
+    return b, c, d
+
+
+def do_tuple_string_assignment(a):
+    (b, c, d) = z = a
+    return b, c, d, z
+
+
 def uppercase_decorator(function):  # type: (Callable) -> Callable
     def wrapper(a, b):  # type: (str, str) -> str
         func = function(a, b)