From b71384c689e046f6d9e4470fb15df1b99fd8626e Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Wed, 16 Sep 2026 19:25:00 -0700 Subject: [PATCH 01/11] test(analyzer): define shell truthiness core contract Signed-off-by: Christopher Kevin --- .../nodes/analyzers/test_shared_python_ast.py | 6 +- .../analyzers/test_tool_misuse_python_ast.py | 251 ++++++++++++++++++ tests/nodes/test_security_end_to_end.py | 75 ++++++ 3 files changed, 331 insertions(+), 1 deletion(-) create mode 100644 tests/nodes/analyzers/test_tool_misuse_python_ast.py diff --git a/tests/nodes/analyzers/test_shared_python_ast.py b/tests/nodes/analyzers/test_shared_python_ast.py index fa864ed60..1b152dfbe 100644 --- a/tests/nodes/analyzers/test_shared_python_ast.py +++ b/tests/nodes/analyzers/test_shared_python_ast.py @@ -16,6 +16,7 @@ behavioral_taint_tracking, static_patterns_data_exfiltration, static_patterns_output_handling, + static_patterns_tool_misuse, ) from skillspector.nodes.build_context import build_context from skillspector.nodes.deduplicate import deduplicate @@ -62,7 +63,8 @@ def test_preparsed_python_is_reused_by_all_ast_analyzers(tmp_path, monkeypatch) "import subprocess\n" "payload = input()\n" "environment = os.environ.copy()\n" - "subprocess.run(output)\n" + "enabled = True\n" + "subprocess.run(output, shell=enabled)\n" "exec(payload)\n", encoding="utf-8", ) @@ -90,11 +92,13 @@ def count_parse(*args, **kwargs): data_findings = static_patterns_data_exfiltration.node(state)["findings"] output_findings = static_patterns_output_handling.node(state)["findings"] + tool_misuse_findings = static_patterns_tool_misuse.node(state)["findings"] ast_findings = behavioral_ast.node(state)["findings"] taint_findings = behavioral_taint_tracking.node(state)["findings"] assert any(finding.rule_id == "E2" for finding in data_findings) assert any(finding.rule_id == "OH1" for finding in output_findings) + assert any(finding.rule_id == "TM1" for finding in tool_misuse_findings) assert any(finding.rule_id == "AST1" for finding in ast_findings) assert any(finding.rule_id == "TT5" for finding in taint_findings) assert parse_calls == 1 diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py new file mode 100644 index 000000000..64edd9841 --- /dev/null +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -0,0 +1,251 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Focused coverage for issue #475's ordinary-Python binding form.""" + +from __future__ import annotations + +import pytest + +from skillspector.nodes.analyzers import static_patterns_tool_misuse as tm_module + + +def _run(content: str, path: str = "run.py") -> dict: + return tm_module.node({"components": [path], "file_cache": {path: content}}) + + +def _tm1(content: str, path: str = "run.py") -> list: + return [finding for finding in _run(content, path)["findings"] if finding.rule_id == "TM1"] + + +@pytest.mark.parametrize( + "value", + [ + pytest.param("True", id="issue-body-boolean"), + pytest.param("'True'", id="reporter-attachment-string"), + ], +) +def test_issue_475_multiline_binding_matches_direct_tm1(value: str) -> None: + findings = _tm1( + "import subprocess\n" + "command = f'python a.py'\n" + f"enabled = {value}\n" + "result = subprocess.run(\n" + " command,\n" + " shell=enabled,\n" + " capture_output=True,\n" + " text=True,\n" + ")\n" + ) + + assert len(findings) == 1 + assert findings[0].start_line == 4 + assert findings[0].severity == "HIGH" + assert findings[0].confidence == pytest.approx(0.9) + assert "shell=enabled" in findings[0].matched_text + assert not findings[0].evidence + + +@pytest.mark.parametrize( + "value", + [ + pytest.param("1", id="integer"), + pytest.param("-1", id="negative-integer"), + pytest.param("(0,)", id="nonempty-tuple"), + pytest.param("not False", id="negation"), + ], +) +def test_simple_immutable_truthy_values_are_tracked(value: str) -> None: + assert ( + len(_tm1(f"import subprocess\nenabled = {value}\nsubprocess.run(cmd, shell=enabled)\n")) + == 1 + ) + + +@pytest.mark.parametrize( + "value", + [ + pytest.param("False", id="boolean"), + pytest.param("0", id="integer"), + pytest.param("''", id="string"), + pytest.param("()", id="tuple"), + pytest.param("None", id="none"), + ], +) +def test_definitely_false_values_are_not_tracked(value: str) -> None: + assert not _tm1(f"import subprocess\nenabled = {value}\nsubprocess.run(cmd, shell=enabled)\n") + + +def test_simple_alias_chain_and_bare_popen_are_tracked() -> None: + findings = _tm1( + "first = 'enabled'\nsecond = first\nthird = second\nPopen(command, shell=third)\n" + ) + + assert len(findings) == 1 + assert findings[0].start_line == 4 + + +@pytest.mark.parametrize( + "rebind", + [ + pytest.param("enabled = False", id="false-assignment"), + pytest.param("enabled = dynamic", id="unknown-assignment"), + pytest.param("import pathlib as enabled", id="import"), + pytest.param("from settings import enabled", id="from-import"), + ], +) +def test_rebinding_invalidates_truthy_fact(rebind: str) -> None: + assert not _tm1( + f"import subprocess\nenabled = True\n{rebind}\nsubprocess.run(command, shell=enabled)\n" + ) + + +def test_import_side_effect_boundary_clears_truth_facts() -> None: + assert not _tm1( + "import subprocess\nenabled = True\nimport attacker\n" + "subprocess.run(command, shell=enabled)\n" + ) + + +@pytest.mark.parametrize( + "shadow", + [ + pytest.param("subprocess = Proxy()", id="assignment"), + pytest.param("import other as subprocess", id="import-alias"), + pytest.param("for subprocess in values:\n pass", id="compound-binder"), + pytest.param("subprocess.run = Proxy()", id="attribute-mutation"), + pytest.param("subprocess, other = pair", id="unpacking"), + ], +) +def test_explicit_subprocess_shadow_rejects_bound_call(shadow: str) -> None: + assert not _tm1(f"{shadow}\nenabled = True\nsubprocess.run(cmd, shell=enabled)\n") + + +def test_explicit_import_reestablishes_direct_receivers() -> None: + assert ( + len( + _tm1( + "subprocess = Proxy()\n" + "import subprocess\n" + "Popen = Proxy()\n" + "from subprocess import Popen\n" + "enabled = True\n" + "subprocess.run(command, shell=enabled)\n" + "Popen(command, shell=enabled)\n" + ) + ) + == 2 + ) + + +def test_relative_import_does_not_establish_bare_popen() -> None: + assert not _tm1( + "Popen = proxy\nfrom .subprocess import Popen\nenabled = True\n" + "Popen(command, shell=enabled)\n" + ) + + +def test_function_local_binding_and_outer_fact_are_independent() -> None: + findings = _tm1( + "outer = True\n" + "def execute(command):\n" + " enabled = 'True'\n" + " subprocess.run(command, shell=enabled)\n" + "subprocess.run(command, shell=outer)\n" + ) + + assert [finding.start_line for finding in findings] == [4, 5] + + +def test_function_compile_time_receiver_shadow_rejects_earlier_lookup() -> None: + assert not _tm1( + "def execute(command):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + " subprocess = Proxy()\n" + ) + + +def test_later_global_receiver_mutation_suppresses_function_body() -> None: + assert not _tm1( + "def execute(command):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + "subprocess = Proxy()\n" + ) + + +def test_passive_function_definition_preserves_outer_fact() -> None: + assert ( + len( + _tm1( + "enabled = True\n" + "def helper(value=1):\n" + " pass\n" + "subprocess.run(command, shell=enabled)\n" + ) + ) + == 1 + ) + + +@pytest.mark.parametrize( + "compound", + [ + pytest.param("if condition:\n pass", id="if"), + pytest.param("for item in values:\n pass", id="for"), + pytest.param("with provider():\n pass", id="with"), + pytest.param("try:\n pass\nexcept Exception:\n pass", id="try"), + pytest.param("class Local:\n pass", id="class"), + ], +) +def test_compound_statement_conservatively_clears_truth_facts(compound: str) -> None: + assert not _tm1(f"enabled = True\n{compound}\nsubprocess.run(command, shell=enabled)\n") + + +def test_calls_inside_compound_statements_are_out_of_scope() -> None: + assert not _tm1( + "if condition:\n enabled = True\n subprocess.run(command, shell=enabled)\n" + ) + + +@pytest.mark.parametrize( + "argument", + [ + pytest.param("disable()", id="call"), + pytest.param("mutator.command", id="attribute"), + pytest.param("holder[0]", id="subscript"), + pytest.param("left + right", id="operator"), + pytest.param("f'{value}'", id="formatted-string"), + pytest.param("[item for item in items]", id="comprehension"), + pytest.param("*commands", id="starred-expansion"), + ], +) +def test_side_effect_capable_call_arguments_are_rejected(argument: str) -> None: + assert not _tm1(f"enabled = True\nsubprocess.run({argument}, shell=enabled)\n") + + +def test_unsupported_assignment_clears_existing_facts() -> None: + assert not _tm1("enabled = True\nresult = factory()\nsubprocess.run(cmd, shell=enabled)\n") + + +def test_annotated_assignment_is_outside_side_effect_free_contract() -> None: + assert not _tm1("enabled: bool = True\nsubprocess.run(command, shell=enabled)\n") + + +def test_assignment_rhs_direct_call_is_inspected_before_invalidation() -> None: + findings = _tm1("enabled = True\nresult = subprocess.run(command, shell=enabled)\n") + + assert len(findings) == 1 + assert findings[0].start_line == 2 + + +def test_true_prefixed_identifier_has_one_lexical_owner() -> None: + findings = _tm1("true_value = True\nsubprocess.run(command, shell=true_value)\n") + + assert len(findings) == 1 + + +@pytest.mark.parametrize("path", ["run.pyw", "run", "run.sh"]) +def test_non_py_surfaces_do_not_enable_ast_companion(path: str) -> None: + assert not _tm1("enabled = True\nsubprocess.run(command, shell=enabled)\n", path) diff --git a/tests/nodes/test_security_end_to_end.py b/tests/nodes/test_security_end_to_end.py index 76d47965d..d6bbdd81f 100644 --- a/tests/nodes/test_security_end_to_end.py +++ b/tests/nodes/test_security_end_to_end.py @@ -297,6 +297,81 @@ async def _assert_incomplete_across_public_surfaces( assert sc9["evidence"]["excluded_inspection_incomplete"] is True +@pytest.mark.parametrize( + "bound_value", + [ + pytest.param("True", id="boolean"), + pytest.param("'True'", id="reporter-truthy-string"), + ], +) +def test_tm1_bound_true_matches_literal_in_graph( + tmp_path: Path, + bound_value: str, +) -> None: + direct = tmp_path / "direct-shell" + bound = tmp_path / "bound-shell" + _write_bundle( + direct, + { + "SKILL.md": "# Shell helper", + "run.py": "import subprocess\nsubprocess.run(command, shell=True)\n", + }, + ) + _write_bundle( + bound, + { + "SKILL.md": "# Shell helper", + "run.py": ( + "import subprocess\n" + f"use_shell = {bound_value}\n" + "subprocess.run(command, shell=use_shell)\n" + ), + }, + ) + + direct_result = _scan(direct) + bound_result = _scan(bound) + direct_tm1 = _assert_rule(direct_result, "TM1", "run.py") + bound_tm1 = _assert_rule(bound_result, "TM1", "run.py") + + assert len(direct_tm1) == len(bound_tm1) == 1 + assert (bound_tm1[0].severity, bound_tm1[0].confidence) == ( + direct_tm1[0].severity, + direct_tm1[0].confidence, + ) + assert ( + bound_result["risk_score"], + bound_result["risk_severity"], + bound_result["risk_recommendation"], + ) == ( + direct_result["risk_score"], + direct_result["risk_severity"], + direct_result["risk_recommendation"], + ) + + +@pytest.mark.asyncio +async def test_tm1_bound_true_across_public_surfaces(tmp_path: Path) -> None: + bound = tmp_path / "bound-shell-public" + _write_bundle( + bound, + { + "SKILL.md": "# Shell helper", + "run.py": ( + "import subprocess\nuse_shell = True\nsubprocess.run(command, shell=use_shell)\n" + ), + }, + ) + + result = _scan(bound) + _assert_rule(result, "TM1", "run.py") + await _assert_rules_across_public_surfaces( + bound, + expected_locations={"TM1": {"run.py"}}, + python_result=result, + ) + + @pytest.mark.parametrize( ("finding", "normal_files", "bypass_files", "rule_id", "normal_path", "bypass_path"), [ From 96685159ff9cf04a71354c38771c9aeb33dcc3b4 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Wed, 16 Sep 2026 19:32:47 -0700 Subject: [PATCH 02/11] fix(analyzer): detect bound shell truthiness Signed-off-by: Christopher Kevin --- .../analyzers/static_patterns_tool_misuse.py | 71 +- .../static_python_shell_truthiness.py | 666 ++++++++++++++++++ 2 files changed, 722 insertions(+), 15 deletions(-) create mode 100644 src/skillspector/nodes/analyzers/static_python_shell_truthiness.py diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 6d8876a22..1d6a8da65 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -2550,6 +2550,26 @@ def _line_containing(content: str, start: int, end: int) -> str: return content[line_start:line_end] +def _classify_tm1( + context: str, + matched_text: str, + matched_line: str, + confidence: float, + file_type: str, +) -> tuple[Severity, float]: + """Apply the existing TM1 contextual classification to one candidate.""" + if ( + _is_safe_container_command(context) + or _is_safe_dockerfile_idiom(context, matched_text) + or _is_safe_cache_cleanup(matched_line) + ): + return Severity.LOW, min(confidence, 0.15) + adjusted = ( + min(1.0, confidence + 0.1) if file_type in ("python", "shell", "javascript") else confidence + ) + return Severity.HIGH, adjusted + + def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: """Analyze content for tool misuse patterns (TM1–TM3).""" findings: list[AnalyzerFinding] = [] @@ -2569,20 +2589,13 @@ def ctx(start: int) -> str: matched = matched_text[:200] matched_line = _line_containing(content, match_start, match_end) - if ( - _is_safe_container_command(context_text) - or _is_safe_dockerfile_idiom(context_text, matched) - or _is_safe_cache_cleanup(matched_line) - ): - adj = min(confidence, 0.15) - sev = Severity.LOW - else: - adj = ( - min(1.0, confidence + 0.1) - if file_type in ("python", "shell", "javascript") - else confidence - ) - sev = Severity.HIGH + sev, adj = _classify_tm1( + context_text, + matched, + matched_line, + confidence, + file_type, + ) candidate_key = (line_num, " ".join(matched.strip().split())) existing = tm1_findings_by_key.get(candidate_key) if existing is not None: @@ -2667,6 +2680,34 @@ def ctx(start: int) -> str: def node(state: SkillspectorState) -> AnalyzerNodeResponse: """Run tool_misuse patterns and return findings.""" - response = static_runner.run_static_patterns_with_ledger(state, [sys.modules[__name__]]) + from . import static_python_shell_truthiness + + response = static_runner.run_static_patterns_with_ledger( + state, + [sys.modules[__name__], static_python_shell_truthiness], + ) + file_cache = state.get("file_cache", {}) + for finding in response["findings"]: + if ( + finding.evidence.pop( + static_python_shell_truthiness.BOUND_SHELL_EVIDENCE, + None, + ) + is not True + ): + continue + content = file_cache.get(finding.file, "") + content_lines = content.splitlines() + line_index = max(0, finding.start_line - 1) + matched = (finding.matched_text or "")[:200] + matched_line = content_lines[line_index] if line_index < len(content_lines) else matched + severity, finding.confidence = _classify_tm1( + finding.context or "", + matched, + matched_line, + finding.confidence, + "python", + ) + finding.severity = severity.value logger.info("%s: %d findings", ANALYZER_ID, len(response["findings"])) return response diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py new file mode 100644 index 000000000..741a9d544 --- /dev/null +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -0,0 +1,666 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Find direct subprocess calls using a definitely truthy local name. + +This companion recognizes the straight-line ordinary-Python form reported in +issue #475. Call arguments must be passive, and unsupported expressions or +compound statements discard facts rather than guessing about Python execution. +""" + +from __future__ import annotations + +import ast + +from skillspector.models import AnalyzerFinding, Location, Severity +from skillspector.python_ast import ParsedPythonFile, parse_python_source + +from .common import get_context_from_lines, get_source_segment +from .pattern_defaults import PatternCategory + +ANALYZER_ID = "static_patterns_tool_misuse" +USES_PYTHON_AST = True +BOUND_SHELL_EVIDENCE = "_tm1_bound_shell_value" +_DIRECT_CALL_NAMES = frozenset({"subprocess", "Popen"}) + + +def _truth_value( + expression: ast.expr | None, + facts: dict[str, bool], +) -> bool | None: + """Return truth for a small, immutable, side-effect-free expression subset.""" + if expression is None: + return None + + resolved: dict[ast.expr, bool] = {} + pending: list[tuple[ast.expr, bool]] = [(expression, False)] + while pending: + current, expanded = pending.pop() + if isinstance(current, ast.Constant): + resolved[current] = bool(current.value) + elif isinstance(current, ast.Name): + if current.id not in facts: + return None + resolved[current] = facts[current.id] + elif isinstance(current, ast.Tuple): + if not current.elts: + resolved[current] = False + elif any(isinstance(item, ast.Starred) for item in current.elts): + return None + elif all(_is_passive_argument(item) for item in current.elts): + resolved[current] = True + else: + return None + elif isinstance(current, ast.UnaryOp): + if not isinstance(current.op, ast.Not) and not ( + isinstance(current.op, (ast.UAdd, ast.USub)) + and isinstance(current.operand, ast.Constant) + and type(current.operand.value) in (bool, int, float, complex) + ): + return None + if expanded: + operand = resolved[current.operand] + resolved[current] = not operand if isinstance(current.op, ast.Not) else operand + else: + pending.append((current, True)) + pending.append((current.operand, False)) + else: + return None + return resolved[expression] + + +def _update_trusted_names_from_import( + statement: ast.Import | ast.ImportFrom, + trusted_names: set[str], +) -> None: + """Update only direct receiver names that the import actually binds.""" + if isinstance(statement, ast.Import): + for imported in statement.names: + bound = imported.asname or imported.name.partition(".")[0] + if imported.name == "subprocess" and bound == "subprocess": + trusted_names.add(bound) + elif bound in trusted_names: + trusted_names.discard(bound) + return + + if any(imported.name == "*" for imported in statement.names): + trusted_names.clear() + return + for imported in statement.names: + bound = imported.asname or imported.name + if ( + statement.level == 0 + and statement.module == "subprocess" + and imported.name == "Popen" + and bound == "Popen" + ): + trusted_names.add(bound) + elif bound in trusted_names: + trusted_names.discard(bound) + + +class _DirectBindingCollector: + """Collect direct receiver bindings without entering nested scopes.""" + + def __init__(self, tracked_names: set[str] | frozenset[str]) -> None: + self.tracked_names = tracked_names + self.bound: set[str] = set() + self.mutated: set[str] = set() + self.nonlocal_names: set[str] = set() + + @staticmethod + def _function_header_nodes( + node: ast.FunctionDef | ast.AsyncFunctionDef, + ) -> list[ast.AST]: + nodes: list[ast.AST] = [*node.decorator_list, *node.args.defaults] + nodes.extend(item for item in node.args.kw_defaults if item is not None) + arguments = (*node.args.posonlyargs, *node.args.args, *node.args.kwonlyargs) + nodes.extend( + argument.annotation for argument in arguments if argument.annotation is not None + ) + if node.args.vararg is not None and node.args.vararg.annotation is not None: + nodes.append(node.args.vararg.annotation) + if node.args.kwarg is not None and node.args.kwarg.annotation is not None: + nodes.append(node.args.kwarg.annotation) + if node.returns is not None: + nodes.append(node.returns) + nodes.extend(getattr(node, "type_params", ())) + return nodes + + def visit(self, node: ast.AST) -> None: + pending = [node] + while pending: + current = pending.pop() + if isinstance(current, ast.Name): + if ( + isinstance(current.ctx, (ast.Store, ast.Del)) + and current.id in self.tracked_names + ): + self.bound.add(current.id) + continue + if isinstance(current, (ast.Attribute, ast.Subscript)): + if isinstance(current.ctx, (ast.Store, ast.Del)): + root: ast.expr = current.value + while isinstance(root, (ast.Attribute, ast.Subscript)): + root = root.value + if isinstance(root, ast.Name) and root.id in self.tracked_names: + self.mutated.add(root.id) + pending.extend(ast.iter_child_nodes(current)) + continue + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef)): + if current.name in self.tracked_names: + self.bound.add(current.name) + pending.extend(self._function_header_nodes(current)) + continue + if isinstance(current, ast.ClassDef): + if current.name in self.tracked_names: + self.bound.add(current.name) + pending.extend(current.decorator_list) + pending.extend(current.bases) + pending.extend(keyword.value for keyword in current.keywords) + continue + if isinstance(current, ast.Lambda): + pending.extend(current.args.defaults) + pending.extend(item for item in current.args.kw_defaults if item is not None) + continue + if isinstance(current, (ast.ListComp, ast.SetComp, ast.GeneratorExp)): + pending.append(current.elt) + for generator in current.generators: + pending.append(generator.iter) + pending.extend(generator.ifs) + continue + if isinstance(current, ast.DictComp): + pending.extend((current.key, current.value)) + for generator in current.generators: + pending.append(generator.iter) + pending.extend(generator.ifs) + continue + if isinstance(current, ast.Import): + for imported in current.names: + bound = imported.asname or imported.name.partition(".")[0] + if bound in self.tracked_names: + self.bound.add(bound) + continue + if isinstance(current, ast.ImportFrom): + if any(imported.name == "*" for imported in current.names): + self.bound.update(self.tracked_names) + continue + for imported in current.names: + bound = imported.asname or imported.name + if bound in self.tracked_names: + self.bound.add(bound) + continue + if isinstance(current, ast.ExceptHandler): + if isinstance(current.name, str) and current.name in self.tracked_names: + self.bound.add(current.name) + pending.extend(ast.iter_child_nodes(current)) + continue + if isinstance(current, (ast.Global, ast.Nonlocal)): + self.nonlocal_names.update(current.names) + continue + if isinstance(current, ast.MatchAs): + if isinstance(current.name, str) and current.name in self.tracked_names: + self.bound.add(current.name) + if current.pattern is not None: + pending.append(current.pattern) + continue + if isinstance(current, ast.MatchStar): + if isinstance(current.name, str) and current.name in self.tracked_names: + self.bound.add(current.name) + continue + if isinstance(current, ast.MatchMapping): + if isinstance(current.rest, str) and current.rest in self.tracked_names: + self.bound.add(current.rest) + pending.extend(current.patterns) + continue + pending.extend(ast.iter_child_nodes(current)) + + +def _function_bound_direct_names( + statement: ast.FunctionDef | ast.AsyncFunctionDef, + tracked_names: set[str], +) -> set[str]: + """Return compile-time local receiver names for one function scope.""" + arguments = statement.args + named = (*arguments.posonlyargs, *arguments.args, *arguments.kwonlyargs) + names = {argument.arg for argument in named} + if arguments.vararg is not None: + names.add(arguments.vararg.arg) + if arguments.kwarg is not None: + names.add(arguments.kwarg.arg) + collector = _DirectBindingCollector(tracked_names) + for child in statement.body: + collector.visit(child) + return names.intersection(tracked_names).union( + collector.bound.difference(collector.nonlocal_names) + ) + + +def _changed_direct_names(nodes: list[ast.AST], tracked_names: set[str]) -> set[str]: + """Return receiver names explicitly rebound or mutated by current-scope nodes.""" + collector = _DirectBindingCollector(tracked_names) + for node in nodes: + collector.visit(node) + return collector.bound.union(collector.mutated) + + +def _class_body_changed_direct_names( + statement: ast.ClassDef, + tracked_names: set[str], +) -> set[str]: + """Return explicit class-execution effects on outer receiver objects.""" + + def nested_classes(node: ast.AST) -> list[ast.ClassDef]: + classes: list[ast.ClassDef] = [] + pending = [node] + while pending: + current = pending.pop() + if isinstance(current, ast.ClassDef): + classes.append(current) + continue + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + continue + pending.extend(ast.iter_child_nodes(current)) + return classes + + affected: set[str] = set() + pending_classes = [statement] + while pending_classes: + current_class = pending_classes.pop() + declaration_collector = _DirectBindingCollector(tracked_names) + for child in current_class.body: + declaration_collector.visit(child) + global_names = declaration_collector.nonlocal_names.intersection(tracked_names) + + local_direct: dict[str, bool] = {} + affected.update(global_names.intersection(declaration_collector.bound)) + for child in current_class.body: + collector = _DirectBindingCollector(tracked_names) + collector.visit(child) + affected.update( + name + for name in collector.mutated + if name in global_names or local_direct.get(name, True) + ) + affected.update(collector.bound.intersection(global_names)) + pending_classes.extend(nested_classes(child)) + + local_bound = collector.bound.difference(global_names) + if isinstance(child, ast.Import): + for imported in child.names: + bound = imported.asname or imported.name.partition(".")[0] + if bound in local_bound: + local_direct[bound] = ( + imported.name == "subprocess" and bound == "subprocess" + ) + elif isinstance(child, ast.ImportFrom): + for imported in child.names: + bound = imported.asname or imported.name + if bound in local_bound: + local_direct[bound] = ( + child.level == 0 + and child.module == "subprocess" + and imported.name == "Popen" + and bound == "Popen" + ) + elif isinstance(child, ast.Assign): + prior_local_direct = dict(local_direct) + for name in local_bound: + local_direct[name] = False + for target in child.targets: + if isinstance(target, ast.Name) and target.id in local_bound: + local_direct[target.id] = ( + isinstance(child.value, ast.Name) + and child.value.id == target.id + and prior_local_direct.get( + child.value.id, + child.value.id in tracked_names, + ) + ) + elif isinstance(child, ast.AnnAssign) and child.value is not None: + prior_local_direct = dict(local_direct) + for name in local_bound: + local_direct[name] = False + if isinstance(child.target, ast.Name) and child.target.id in local_bound: + local_direct[child.target.id] = ( + isinstance(child.value, ast.Name) + and child.value.id == child.target.id + and prior_local_direct.get( + child.value.id, + child.value.id in tracked_names, + ) + ) + elif isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + if child.name in tracked_names and child.name not in global_names: + local_direct[child.name] = False + elif isinstance(child, ast.Delete): + for name in collector.bound: + local_direct.pop(name, None) + elif local_bound: + for name in local_bound: + local_direct.pop(name, None) + return affected + + +def _is_direct_subprocess_call(call: ast.Call, trusted_names: set[str]) -> bool: + function = call.func + if isinstance(function, ast.Name): + return function.id == "Popen" and function.id in trusted_names + return ( + isinstance(function, ast.Attribute) + and isinstance(function.value, ast.Name) + and function.value.id == "subprocess" + and function.value.id in trusted_names + ) + + +def _is_passive_argument(expression: ast.expr) -> bool: + """Return whether evaluation cannot invoke user-controlled Python code.""" + normal, hash_required, truth_required, numeric_required, integral_required = range(5) + pending: list[tuple[ast.expr, int]] = [(expression, normal)] + while pending: + current, requirement = pending.pop() + if isinstance(current, ast.Constant): + if requirement == numeric_required and type(current.value) not in ( + bool, + int, + float, + complex, + ): + return False + if requirement == integral_required and type(current.value) not in (bool, int): + return False + continue + if isinstance(current, ast.Name): + if requirement != normal: + return False + continue + if isinstance(current, ast.List): + if requirement in (hash_required, numeric_required, integral_required) or any( + isinstance(item, ast.Starred) for item in current.elts + ): + return False + pending.extend((item, normal) for item in current.elts) + continue + if isinstance(current, ast.Tuple): + if requirement in (numeric_required, integral_required): + return False + if any(isinstance(item, ast.Starred) for item in current.elts): + return False + nested_requirement = hash_required if requirement == hash_required else normal + pending.extend((item, nested_requirement) for item in current.elts) + continue + if isinstance(current, ast.Dict): + if requirement in (hash_required, numeric_required, integral_required) or any( + key is None for key in current.keys + ): + return False + pending.extend((key, hash_required) for key in current.keys if key is not None) + pending.extend((value, normal) for value in current.values) + continue + if isinstance(current, ast.Set): + if requirement in (hash_required, numeric_required, integral_required): + return False + pending.extend((item, hash_required) for item in current.elts) + continue + if isinstance(current, ast.UnaryOp): + if isinstance(current.op, ast.Not): + pending.append((current.operand, truth_required)) + elif isinstance(current.op, (ast.UAdd, ast.USub)): + operand_requirement = ( + integral_required if requirement == integral_required else numeric_required + ) + pending.append((current.operand, operand_requirement)) + elif isinstance(current.op, ast.Invert): + pending.append((current.operand, integral_required)) + else: + return False + continue + if isinstance(current, ast.JoinedStr) and all( + isinstance(item, ast.Constant) for item in current.values + ): + if requirement in (numeric_required, integral_required): + return False + continue + return False + return True + + +def _call_arguments_are_passive(call: ast.Call) -> bool: + return all(_is_passive_argument(argument) for argument in call.args) and all( + keyword.arg is not None and _is_passive_argument(keyword.value) for keyword in call.keywords + ) + + +def _annotation_is_passive(annotation: ast.expr) -> bool: + """Accept only annotation spellings whose evaluation cannot rebind a name.""" + return all( + isinstance(node, (ast.Name, ast.Constant, ast.Load)) for node in ast.walk(annotation) + ) + + +def _function_header_is_passive( + statement: ast.FunctionDef | ast.AsyncFunctionDef, +) -> bool: + """Reject definition-time expressions that could mutate tracked bindings.""" + if statement.decorator_list or getattr(statement, "type_params", []): + return False + defaults = (*statement.args.defaults, *(item for item in statement.args.kw_defaults if item)) + if any(not _is_passive_argument(default) for default in defaults): + return False + arguments = ( + *statement.args.posonlyargs, + *statement.args.args, + *statement.args.kwonlyargs, + ) + annotations = [argument.annotation for argument in arguments if argument.annotation is not None] + if statement.args.vararg is not None and statement.args.vararg.annotation is not None: + annotations.append(statement.args.vararg.annotation) + if statement.args.kwarg is not None and statement.args.kwarg.annotation is not None: + annotations.append(statement.args.kwarg.annotation) + if statement.returns is not None: + annotations.append(statement.returns) + return all(_annotation_is_passive(annotation) for annotation in annotations) + + +def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None: + """Apply one statement's explicit receiver-binding effects.""" + if isinstance(statement, (ast.Import, ast.ImportFrom)): + _update_trusted_names_from_import(statement, trusted_names) + return + if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): + if not _function_header_is_passive(statement): + trusted_names.clear() + trusted_names.discard(statement.name) + return + if isinstance(statement, ast.Assign): + changed = _changed_direct_names( + [statement.value, *statement.targets], + trusted_names, + ) + preserved = { + target.id + for target in statement.targets + if isinstance(target, ast.Name) + and isinstance(statement.value, ast.Name) + and statement.value.id == target.id + and target.id in trusted_names + } + trusted_names.difference_update(changed.difference(preserved)) + return + if isinstance(statement, ast.ClassDef): + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + trusted_names.difference_update(_class_body_changed_direct_names(statement, trusted_names)) + return + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + + +class _Analyzer: + def __init__(self, file_path: str, lines: list[str]) -> None: + self.file_path = file_path + self.lines = lines + self.findings: list[AnalyzerFinding] = [] + + def _inspect_call(self, call: ast.Call, facts: dict[str, bool]) -> None: + shell = next((item.value for item in call.keywords if item.arg == "shell"), None) + if ( + not isinstance(shell, ast.Name) + or shell.id.casefold().startswith("true") + or facts.get(shell.id) is not True + ): + return + line = getattr(call, "lineno", 1) + end_line = getattr(call, "end_lineno", None) + self.findings.append( + AnalyzerFinding( + rule_id="TM1", + message="Tool Parameter Abuse", + severity=Severity.HIGH, + location=Location(file=self.file_path, start_line=line, end_line=end_line), + confidence=0.8, + tags=[PatternCategory.TOOL_MISUSE.value], + context=get_context_from_lines(self.lines, line), + matched_text=get_source_segment(self.lines, line, end_line), + evidence={BOUND_SHELL_EVIDENCE: True}, + ) + ) + + def _scan_assignment( + self, + targets: list[ast.expr], + value: ast.expr, + facts: dict[str, bool], + trusted_names: set[str], + ) -> None: + if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): + resolved = None + safe_value = _call_arguments_are_passive(value) + if safe_value: + self._inspect_call(value, facts) + else: + resolved = _truth_value(value, facts) + safe_value = resolved is not None or _is_passive_argument(value) + + if not safe_value or any(not isinstance(target, ast.Name) for target in targets): + facts.clear() + trusted_names.difference_update(_changed_direct_names([value, *targets], trusted_names)) + return + for target in targets: + assert isinstance(target, ast.Name) + if resolved is None: + facts.pop(target.id, None) + else: + facts[target.id] = resolved + preserves_binding = ( + isinstance(value, ast.Name) and value.id == target.id and value.id in trusted_names + ) + if not preserves_binding: + trusted_names.discard(target.id) + + def _scan_block( + self, + statements: list[ast.stmt], + *, + trusted_names: set[str] | None = None, + ) -> None: + trusted_names = set(_DIRECT_CALL_NAMES if trusted_names is None else trusted_names) + facts: dict[str, bool] = {} + last_invalidation_by_name: dict[str, int] = {} + + def last_invalidation(name: str) -> int: + cached = last_invalidation_by_name.get(name) + if cached is not None: + return cached + last = -1 + for candidate_index, candidate in enumerate(statements): + probe = {name} + _advance_trusted_names(candidate, probe) + if name not in probe: + last = candidate_index + last_invalidation_by_name[name] = last + return last + + for index, statement in enumerate(statements): + if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): + passive_header = _function_header_is_passive(statement) + nested_trusted_names = { + name for name in trusted_names if last_invalidation(name) <= index + } + nested_trusted_names.difference_update( + _function_bound_direct_names(statement, nested_trusted_names) + ) + nested_trusted_names.discard(statement.name) + if not passive_header: + nested_trusted_names.clear() + self._scan_block(statement.body, trusted_names=nested_trusted_names) + if passive_header: + facts.pop(statement.name, None) + else: + facts.clear() + trusted_names.clear() + trusted_names.discard(statement.name) + elif isinstance(statement, (ast.Import, ast.ImportFrom)): + facts.clear() + _update_trusted_names_from_import(statement, trusted_names) + elif isinstance(statement, ast.Assign): + self._scan_assignment( + list(statement.targets), + statement.value, + facts, + trusted_names, + ) + elif isinstance(statement, ast.AnnAssign): + value = statement.value + if ( + isinstance(value, ast.Call) + and _is_direct_subprocess_call(value, trusted_names) + and _call_arguments_are_passive(value) + ): + self._inspect_call(value, facts) + facts.clear() + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + elif isinstance(statement, (ast.AugAssign, ast.Delete)): + facts.clear() + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + elif isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Call): + call = statement.value + if _is_direct_subprocess_call(call, trusted_names) and _call_arguments_are_passive( + call + ): + self._inspect_call(call, facts) + else: + facts.clear() + trusted_names.difference_update(_changed_direct_names([call], trusted_names)) + elif isinstance(statement, ast.Pass) or ( + isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant) + ): + continue + elif isinstance(statement, ast.ClassDef): + facts.clear() + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + trusted_names.difference_update( + _class_body_changed_direct_names(statement, trusted_names) + ) + else: + facts.clear() + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + + def run(self, tree: ast.Module) -> list[AnalyzerFinding]: + self._scan_block(tree.body) + return sorted(self.findings, key=lambda finding: finding.location.start_line) + + +def analyze( + content: str, + file_path: str, + file_type: str, + *, + python_ast: ParsedPythonFile | None = None, +) -> list[AnalyzerFinding]: + """Find straight-line truthy names passed to direct subprocess calls.""" + if file_type != "python": + return [] + parsed = python_ast or parse_python_source(content, file_path) + if parsed.tree is None: + return [] + return _Analyzer(file_path, parsed.lines).run(parsed.tree) From bd09251621e1edabcfb4615b04d420dbf83d02b6 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Wed, 16 Sep 2026 20:10:04 -0700 Subject: [PATCH 03/11] fix(analyzer): harden bound shell facts Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 251 ++++++++++++++++-- .../analyzers/test_tool_misuse_python_ast.py | 95 ++++++- 2 files changed, 318 insertions(+), 28 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 741a9d544..5622c5a55 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -15,7 +15,7 @@ from skillspector.models import AnalyzerFinding, Location, Severity from skillspector.python_ast import ParsedPythonFile, parse_python_source -from .common import get_context_from_lines, get_source_segment +from .common import get_complete_source_segment, get_context_from_lines from .pattern_defaults import PatternCategory ANALYZER_ID = "static_patterns_tool_misuse" @@ -216,6 +216,56 @@ def visit(self, node: ast.AST) -> None: pending.extend(ast.iter_child_nodes(current)) +def _direct_bound_names(node: ast.AST) -> set[str]: + """Return names bound by *node* without entering deferred nested scopes.""" + candidates: set[str] = set() + for current in ast.walk(node): + if isinstance(current, ast.Name) and isinstance(current.ctx, (ast.Store, ast.Del)): + candidates.add(current.id) + elif isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + candidates.add(current.name) + elif isinstance(current, ast.Import): + candidates.update( + imported.asname or imported.name.partition(".")[0] for imported in current.names + ) + elif isinstance(current, ast.ImportFrom): + candidates.update( + imported.asname or imported.name + for imported in current.names + if imported.name != "*" + ) + elif isinstance(current, ast.ExceptHandler) and isinstance(current.name, str): + candidates.add(current.name) + elif isinstance(current, (ast.MatchAs, ast.MatchStar)) and isinstance( + current.name, + str, + ): + candidates.add(current.name) + elif isinstance(current, ast.MatchMapping) and isinstance(current.rest, str): + candidates.add(current.rest) + collector = _DirectBindingCollector(candidates) + collector.visit(node) + return collector.bound + + +def _function_parameter_names(statement: ast.FunctionDef | ast.AsyncFunctionDef) -> set[str]: + """Return names that may already hold unsafe values on body entry.""" + arguments = statement.args + names = { + argument.arg + for argument in (*arguments.posonlyargs, *arguments.args, *arguments.kwonlyargs) + } + if arguments.vararg is not None: + names.add(arguments.vararg.arg) + if arguments.kwarg is not None: + names.add(arguments.kwarg.arg) + declarations = _DirectBindingCollector(set()) + for child in statement.body: + declarations.visit(child) + names.update(declarations.nonlocal_names) + return names + + def _function_bound_direct_names( statement: ast.FunctionDef | ast.AsyncFunctionDef, tracked_names: set[str], @@ -432,6 +482,23 @@ def _call_arguments_are_passive(call: ast.Call) -> bool: ) +def _is_finalizer_safe_value(expression: ast.expr, safe_names: set[str]) -> bool: + """Return whether releasing the resulting value cannot run user code.""" + if not _is_passive_argument(expression): + return False + return all( + not isinstance(node, ast.Name) or node.id in safe_names for node in ast.walk(expression) + ) + + +def _call_arguments_are_protocol_safe(call: ast.Call, safe_names: set[str]) -> bool: + """Return whether subprocess argument consumption cannot dispatch user code.""" + return all(_is_finalizer_safe_value(argument, safe_names) for argument in call.args) and all( + keyword.arg is not None and _is_finalizer_safe_value(keyword.value, safe_names) + for keyword in call.keywords + ) + + def _annotation_is_passive(annotation: ast.expr) -> bool: """Accept only annotation spellings whose evaluation cannot rebind a name.""" return all( @@ -463,6 +530,37 @@ def _function_header_is_passive( return all(_annotation_is_passive(annotation) for annotation in annotations) +def _is_immediate_function(statement: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: + """Return whether a direct call begins executing this function body.""" + if isinstance(statement, ast.AsyncFunctionDef): + return False + pending: list[ast.AST] = list(statement.body) + while pending: + current = pending.pop() + if isinstance(current, (ast.Yield, ast.YieldFrom)): + return False + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Lambda)): + continue + pending.extend(ast.iter_child_nodes(current)) + return True + + +def _passive_direct_call(statement: ast.stmt) -> ast.Call | None: + """Return a directly evaluated simple-name call with passive arguments.""" + value: ast.expr | None = None + if isinstance(statement, (ast.Expr, ast.Assign)): + value = statement.value + elif isinstance(statement, ast.AnnAssign): + value = statement.value + if ( + isinstance(value, ast.Call) + and isinstance(value.func, ast.Name) + and _call_arguments_are_passive(value) + ): + return value + return None + + def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None: """Apply one statement's explicit receiver-binding effects.""" if isinstance(statement, (ast.Import, ast.ImportFrom)): @@ -496,9 +594,10 @@ def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None class _Analyzer: - def __init__(self, file_path: str, lines: list[str]) -> None: + def __init__(self, file_path: str, python_ast: ParsedPythonFile) -> None: self.file_path = file_path - self.lines = lines + self.python_ast = python_ast + self.lines = python_ast.lines self.findings: list[AnalyzerFinding] = [] def _inspect_call(self, call: ast.Call, facts: dict[str, bool]) -> None: @@ -511,16 +610,34 @@ def _inspect_call(self, call: ast.Call, facts: dict[str, bool]) -> None: return line = getattr(call, "lineno", 1) end_line = getattr(call, "end_lineno", None) + start_byte_column = getattr(call, "col_offset", 0) + end_byte_column = getattr(call, "end_col_offset", start_byte_column) + start_column = self.python_ast.character_column(line, start_byte_column) + end_column = self.python_ast.character_column(end_line or line, end_byte_column) + complete_match = self.python_ast.source_segment(call) + if complete_match is None: + complete_match = get_complete_source_segment(self.lines, line, end_line) self.findings.append( AnalyzerFinding( rule_id="TM1", message="Tool Parameter Abuse", severity=Severity.HIGH, - location=Location(file=self.file_path, start_line=line, end_line=end_line), + location=Location( + file=self.file_path, + start_line=line, + end_line=end_line, + start_column=start_column, + end_column=end_column, + ), confidence=0.8, tags=[PatternCategory.TOOL_MISUSE.value], - context=get_context_from_lines(self.lines, line), - matched_text=get_source_segment(self.lines, line, end_line), + context=get_context_from_lines( + self.lines, + line, + column=start_column if start_column is not None else 0, + ), + matched_text=complete_match[:200], + complete_match=complete_match, evidence={BOUND_SHELL_EVIDENCE: True}, ) ) @@ -531,26 +648,59 @@ def _scan_assignment( value: ast.expr, facts: dict[str, bool], trusted_names: set[str], + bound_names: set[str], + finalizer_safe_names: set[str], ) -> None: + simple_targets = all(isinstance(target, ast.Name) for target in targets) + releases_unsafe_value = simple_targets and any( + target.id in bound_names + and target.id not in finalizer_safe_names + and not (isinstance(value, ast.Name) and value.id == target.id) + for target in targets + if isinstance(target, ast.Name) + ) + result_is_finalizer_safe = _is_finalizer_safe_value(value, finalizer_safe_names) + call_has_protocol_effects = False if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): resolved = None safe_value = _call_arguments_are_passive(value) if safe_value: self._inspect_call(value, facts) + call_has_protocol_effects = not _call_arguments_are_protocol_safe( + value, + finalizer_safe_names, + ) else: resolved = _truth_value(value, facts) safe_value = resolved is not None or _is_passive_argument(value) - if not safe_value or any(not isinstance(target, ast.Name) for target in targets): + if not safe_value or not simple_targets: facts.clear() + finalizer_safe_names.clear() + for target in targets: + if isinstance(target, ast.Name): + bound_names.add(target.id) trusted_names.difference_update(_changed_direct_names([value, *targets], trusted_names)) return + if releases_unsafe_value: + facts.clear() + finalizer_safe_names.clear() + trusted_names.clear() + if call_has_protocol_effects: + facts.clear() + finalizer_safe_names.clear() + trusted_names.clear() for target in targets: assert isinstance(target, ast.Name) - if resolved is None: + bound_names.add(target.id) + if releases_unsafe_value or call_has_protocol_effects or resolved is None: facts.pop(target.id, None) else: facts[target.id] = resolved + if releases_unsafe_value or call_has_protocol_effects or not result_is_finalizer_safe: + finalizer_safe_names.discard(target.id) + else: + finalizer_safe_names.add(target.id) preserves_binding = ( isinstance(value, ast.Name) and value.id == target.id and value.id in trusted_names ) @@ -562,29 +712,52 @@ def _scan_block( statements: list[ast.stmt], *, trusted_names: set[str] | None = None, + initial_bound_names: set[str] | None = None, ) -> None: trusted_names = set(_DIRECT_CALL_NAMES if trusted_names is None else trusted_names) facts: dict[str, bool] = {} - last_invalidation_by_name: dict[str, int] = {} + bound_names = set(initial_bound_names or ()) + finalizer_safe_names: set[str] = set() - def last_invalidation(name: str) -> int: - cached = last_invalidation_by_name.get(name) - if cached is not None: - return cached - last = -1 - for candidate_index, candidate in enumerate(statements): - probe = {name} - _advance_trusted_names(candidate, probe) - if name not in probe: - last = candidate_index - last_invalidation_by_name[name] = last - return last + last_invalidation_by_name: dict[str, int] = {} + receiver_trust = set(trusted_names) + for candidate_index, candidate in enumerate(statements): + before = set(receiver_trust) + _advance_trusted_names(candidate, receiver_trust) + for name in before.difference(receiver_trust): + last_invalidation_by_name[name] = candidate_index + + trusted_at_call_by_definition: dict[int, set[str]] = {} + receiver_trust = set(trusted_names) + active_functions: dict[str, int] = {} + for candidate_index, candidate in enumerate(statements): + call = _passive_direct_call(candidate) + if call is not None: + assert isinstance(call.func, ast.Name) + owner = active_functions.get(call.func.id) + if owner is not None: + trusted_at_call_by_definition.setdefault(owner, set()).update(receiver_trust) + + changed_names = _direct_bound_names(candidate) + for name in changed_names: + active_functions.pop(name, None) + if ( + isinstance(candidate, (ast.FunctionDef, ast.AsyncFunctionDef)) + and _function_header_is_passive(candidate) + and _is_immediate_function(candidate) + ): + active_functions[candidate.name] = candidate_index + _advance_trusted_names(candidate, receiver_trust) for index, statement in enumerate(statements): if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): passive_header = _function_header_is_passive(statement) + trusted_at_call = trusted_at_call_by_definition.get(index, set()) + nested_trusted_names = set(trusted_names).union(trusted_at_call) nested_trusted_names = { - name for name in trusted_names if last_invalidation(name) <= index + name + for name in nested_trusted_names + if last_invalidation_by_name.get(name, -1) <= index or name in trusted_at_call } nested_trusted_names.difference_update( _function_bound_direct_names(statement, nested_trusted_names) @@ -592,15 +765,27 @@ def last_invalidation(name: str) -> int: nested_trusted_names.discard(statement.name) if not passive_header: nested_trusted_names.clear() - self._scan_block(statement.body, trusted_names=nested_trusted_names) - if passive_header: + self._scan_block( + statement.body, + trusted_names=nested_trusted_names, + initial_bound_names=_function_parameter_names(statement), + ) + releases_unsafe_value = ( + statement.name in bound_names and statement.name not in finalizer_safe_names + ) + if passive_header and not releases_unsafe_value: facts.pop(statement.name, None) else: facts.clear() + finalizer_safe_names.clear() trusted_names.clear() + bound_names.add(statement.name) + finalizer_safe_names.discard(statement.name) trusted_names.discard(statement.name) elif isinstance(statement, (ast.Import, ast.ImportFrom)): facts.clear() + finalizer_safe_names.clear() + bound_names.update(_direct_bound_names(statement)) _update_trusted_names_from_import(statement, trusted_names) elif isinstance(statement, ast.Assign): self._scan_assignment( @@ -608,6 +793,8 @@ def last_invalidation(name: str) -> int: statement.value, facts, trusted_names, + bound_names, + finalizer_safe_names, ) elif isinstance(statement, ast.AnnAssign): value = statement.value @@ -618,9 +805,14 @@ def last_invalidation(name: str) -> int: ): self._inspect_call(value, facts) facts.clear() + finalizer_safe_names.clear() + if value is not None: + bound_names.update(_direct_bound_names(statement)) trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) elif isinstance(statement, (ast.AugAssign, ast.Delete)): facts.clear() + finalizer_safe_names.clear() + bound_names.update(_direct_bound_names(statement)) trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) elif isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Call): call = statement.value @@ -628,8 +820,13 @@ def last_invalidation(name: str) -> int: call ): self._inspect_call(call, facts) + if not _call_arguments_are_protocol_safe(call, finalizer_safe_names): + facts.clear() + finalizer_safe_names.clear() + trusted_names.clear() else: facts.clear() + finalizer_safe_names.clear() trusted_names.difference_update(_changed_direct_names([call], trusted_names)) elif isinstance(statement, ast.Pass) or ( isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant) @@ -637,12 +834,16 @@ def last_invalidation(name: str) -> int: continue elif isinstance(statement, ast.ClassDef): facts.clear() + finalizer_safe_names.clear() + bound_names.update(_direct_bound_names(statement)) trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) trusted_names.difference_update( _class_body_changed_direct_names(statement, trusted_names) ) else: facts.clear() + finalizer_safe_names.clear() + bound_names.update(_direct_bound_names(statement)) trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) def run(self, tree: ast.Module) -> list[AnalyzerFinding]: @@ -663,4 +864,4 @@ def analyze( parsed = python_ast or parse_python_source(content, file_path) if parsed.tree is None: return [] - return _Analyzer(file_path, parsed.lines).run(parsed.tree) + return _Analyzer(file_path, parsed).run(parsed.tree) diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 64edd9841..47bf9940b 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -8,6 +8,7 @@ import pytest from skillspector.nodes.analyzers import static_patterns_tool_misuse as tm_module +from skillspector.nodes.deduplicate import deduplicate def _run(content: str, path: str = "run.py") -> dict: @@ -130,8 +131,8 @@ def test_explicit_import_reestablishes_direct_receivers() -> None: "Popen = Proxy()\n" "from subprocess import Popen\n" "enabled = True\n" - "subprocess.run(command, shell=enabled)\n" - "Popen(command, shell=enabled)\n" + "subprocess.run('/usr/bin/true', shell=enabled)\n" + "Popen('/usr/bin/true', shell=enabled)\n" ) ) == 2 @@ -166,7 +167,19 @@ def test_function_compile_time_receiver_shadow_rejects_earlier_lookup() -> None: ) -def test_later_global_receiver_mutation_suppresses_function_body() -> None: +def test_later_global_receiver_mutation_does_not_suppress_earlier_function_call() -> None: + findings = _tm1( + "def execute(command):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + "execute(command)\n" + "subprocess = Proxy()\n" + ) + + assert [finding.start_line for finding in findings] == [3] + + +def test_later_global_receiver_mutation_suppresses_unobserved_function_body() -> None: assert not _tm1( "def execute(command):\n" " enabled = True\n" @@ -229,6 +242,59 @@ def test_unsupported_assignment_clears_existing_facts() -> None: assert not _tm1("enabled = True\nresult = factory()\nsubprocess.run(cmd, shell=enabled)\n") +def test_simple_name_store_with_unsafe_prior_binding_invalidates_truth_facts() -> None: + findings = _tm1( + "import subprocess\n" + "class Trigger:\n" + " def __del__(self):\n" + " global enabled\n" + " enabled = False\n" + "trigger = Trigger()\n" + "enabled = True\n" + "trigger = 0\n" + "subprocess.run('/usr/bin/true', shell=enabled)\n" + ) + + assert not findings + + +def test_external_name_store_treats_prior_binding_as_finalizer_capable() -> None: + findings = _tm1( + "import subprocess\n" + "class Trigger:\n" + " def __del__(self):\n" + " global enabled\n" + " enabled = False\n" + "trigger = Trigger()\n" + "enabled = False\n" + "def execute():\n" + " global enabled, trigger\n" + " enabled = True\n" + " trigger = 0\n" + " subprocess.run('/usr/bin/true', shell=enabled)\n" + "execute()\n" + ) + + assert not findings + + +def test_protocol_consuming_direct_call_invalidates_later_truth_fact() -> None: + findings = _tm1( + "import subprocess\n" + "class MutatingArgs:\n" + " def __iter__(self):\n" + " global enabled\n" + " enabled = False\n" + " return iter(('/usr/bin/true',))\n" + "mutator = MutatingArgs()\n" + "enabled = True\n" + "subprocess.run(mutator, shell=enabled)\n" + "subprocess.run('/usr/bin/true', shell=enabled)\n" + ) + + assert [finding.start_line for finding in findings] == [9] + + def test_annotated_assignment_is_outside_side_effect_free_contract() -> None: assert not _tm1("enabled: bool = True\nsubprocess.run(command, shell=enabled)\n") @@ -246,6 +312,29 @@ def test_true_prefixed_identifier_has_one_lexical_owner() -> None: assert len(findings) == 1 +def test_long_same_line_calls_keep_exact_coordinates_and_distinct_identity() -> None: + payload = "x" * 240 + first_call = f'subprocess.run("{payload}A", shell=enabled)' + second_call = f'subprocess.run("{payload}B", shell=enabled)' + call_line = f"first = {first_call}; second = {second_call}" + + findings = _tm1(f"import subprocess\nenabled = True\n{call_line}\n") + + assert len(findings) == 2 + assert [(finding.start_column, finding.end_column) for finding in findings] == [ + ( + call_line.index(first_call), + call_line.index(first_call) + len(first_call), + ), + ( + call_line.index(second_call), + call_line.index(second_call) + len(second_call), + ), + ] + assert findings[0].fingerprint() != findings[1].fingerprint() + assert len(deduplicate(findings)) == 2 + + @pytest.mark.parametrize("path", ["run.pyw", "run", "run.sh"]) def test_non_py_surfaces_do_not_enable_ast_companion(path: str) -> None: assert not _tm1("enabled = True\nsubprocess.run(command, shell=enabled)\n", path) From c3b0ff945013f2cc0eeaeb66515ba099d184478a Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Sun, 20 Sep 2026 19:39:02 -0700 Subject: [PATCH 04/11] fix(analyzer): honor shell argument evaluation order Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 33 +++++++-- .../analyzers/test_tool_misuse_python_ast.py | 69 +++++++++++++++++++ 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 5622c5a55..7b9323e01 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -4,8 +4,9 @@ """Find direct subprocess calls using a definitely truthy local name. This companion recognizes the straight-line ordinary-Python form reported in -issue #475. Call arguments must be passive, and unsupported expressions or -compound statements discard facts rather than guessing about Python execution. +issue #475. Arguments evaluated through ``shell=`` must be passive, and +unsupported expressions or compound statements discard facts rather than +guessing about Python execution. """ from __future__ import annotations @@ -482,6 +483,23 @@ def _call_arguments_are_passive(call: ast.Call) -> bool: ) +def _shell_argument_is_captured_before_effects(call: ast.Call) -> bool: + """Return whether evaluation reaches ``shell=`` without user-code effects. + + Python evaluates every positional argument, including starred expansions, + before keyword arguments. Keyword values are then evaluated in their stored + order. Effects after ``shell=`` cannot change the already captured value. + """ + if any(not _is_passive_argument(argument) for argument in call.args): + return False + for keyword in call.keywords: + if keyword.arg == "shell": + return _is_passive_argument(keyword.value) + if keyword.arg is None or not _is_passive_argument(keyword.value): + return False + return False + + def _is_finalizer_safe_value(expression: ast.expr, safe_names: set[str]) -> bool: """Return whether releasing the resulting value cannot run user code.""" if not _is_passive_argument(expression): @@ -664,8 +682,9 @@ def _scan_assignment( if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): resolved = None safe_value = _call_arguments_are_passive(value) - if safe_value: + if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) + if safe_value: call_has_protocol_effects = not _call_arguments_are_protocol_safe( value, finalizer_safe_names, @@ -801,7 +820,7 @@ def _scan_block( if ( isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names) - and _call_arguments_are_passive(value) + and _shell_argument_is_captured_before_effects(value) ): self._inspect_call(value, facts) facts.clear() @@ -816,10 +835,10 @@ def _scan_block( trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) elif isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Call): call = statement.value - if _is_direct_subprocess_call(call, trusted_names) and _call_arguments_are_passive( - call - ): + direct_call = _is_direct_subprocess_call(call, trusted_names) + if direct_call and _shell_argument_is_captured_before_effects(call): self._inspect_call(call, facts) + if direct_call and _call_arguments_are_passive(call): if not _call_arguments_are_protocol_safe(call, finalizer_safe_names): facts.clear() finalizer_safe_names.clear() diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 47bf9940b..a3fd3d38c 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -238,6 +238,75 @@ def test_side_effect_capable_call_arguments_are_rejected(argument: str) -> None: assert not _tm1(f"enabled = True\nsubprocess.run({argument}, shell=enabled)\n") +@pytest.mark.parametrize( + "statement", + [ + pytest.param( + "subprocess.run(command, shell=enabled, env=build_env())", + id="expression", + ), + pytest.param( + "result = subprocess.run(command, shell=enabled, env=build_env())", + id="assignment", + ), + pytest.param( + "result: object = subprocess.run(command, shell=enabled, env=build_env())", + id="annotated-assignment", + ), + ], +) +def test_later_keyword_effect_preserves_captured_shell_value(statement: str) -> None: + findings = _tm1(f"enabled = True\n{statement}\n") + literal_findings = _tm1(statement.replace("shell=enabled", "shell=True")) + + assert len(findings) == len(literal_findings) == 1 + assert findings[0].start_line == 2 + assert findings[0].severity == literal_findings[0].severity + assert findings[0].confidence == literal_findings[0].confidence + + +@pytest.mark.parametrize( + "call", + [ + pytest.param( + "subprocess.run(command, env=build_env(), shell=enabled)", + id="earlier-keyword", + ), + pytest.param( + "subprocess.run(build_command(), shell=enabled)", + id="earlier-positional", + ), + pytest.param( + "subprocess.run(shell=enabled, *build_args())", + id="starred-positional-written-later", + ), + pytest.param( + "subprocess.run(command, **build_options(), shell=enabled)", + id="earlier-keyword-expansion", + ), + ], +) +def test_earlier_argument_effect_keeps_shell_value_uncertain(call: str) -> None: + assert not _tm1(f"enabled = True\n{call}\n") + + +def test_later_keyword_expansion_preserves_captured_shell_value() -> None: + findings = _tm1("enabled = True\nsubprocess.run(command, shell=enabled, **build_options())\n") + + assert len(findings) == 1 + assert findings[0].start_line == 2 + + +def test_later_argument_effect_invalidates_fact_after_captured_call() -> None: + findings = _tm1( + "enabled = True\n" + "subprocess.run(command, shell=enabled, env=build_env())\n" + "subprocess.run(command, shell=enabled)\n" + ) + + assert [finding.start_line for finding in findings] == [2] + + def test_unsupported_assignment_clears_existing_facts() -> None: assert not _tm1("enabled = True\nresult = factory()\nsubprocess.run(cmd, shell=enabled)\n") From 995d746cbc1c615a7bf17c04ed4943c917ce9beb Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Mon, 21 Sep 2026 13:23:47 -0700 Subject: [PATCH 05/11] fix(analyzer): invalidate effectful subprocess receivers Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 49 ++++++++++++++----- .../analyzers/test_tool_misuse_python_ast.py | 45 +++++++++++++++++ 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 7b9323e01..e0c80d30b 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -580,7 +580,17 @@ def _passive_direct_call(statement: ast.stmt) -> ast.Call | None: def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None: - """Apply one statement's explicit receiver-binding effects.""" + """Apply one statement's receiver-trust effects.""" + value = ( + statement.value if isinstance(statement, (ast.Expr, ast.Assign, ast.AnnAssign)) else None + ) + if ( + isinstance(value, ast.Call) + and _is_direct_subprocess_call(value, trusted_names) + and not _call_arguments_are_passive(value) + ): + trusted_names.clear() + return if isinstance(statement, (ast.Import, ast.ImportFrom)): _update_trusted_names_from_import(statement, trusted_names) return @@ -679,9 +689,11 @@ def _scan_assignment( ) result_is_finalizer_safe = _is_finalizer_safe_value(value, finalizer_safe_names) call_has_protocol_effects = False + effectful_direct_call = False if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): resolved = None safe_value = _call_arguments_are_passive(value) + effectful_direct_call = not safe_value if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) if safe_value: @@ -699,7 +711,12 @@ def _scan_assignment( for target in targets: if isinstance(target, ast.Name): bound_names.add(target.id) - trusted_names.difference_update(_changed_direct_names([value, *targets], trusted_names)) + if effectful_direct_call: + trusted_names.clear() + else: + trusted_names.difference_update( + _changed_direct_names([value, *targets], trusted_names) + ) return if releases_unsafe_value: facts.clear() @@ -817,17 +834,21 @@ def _scan_block( ) elif isinstance(statement, ast.AnnAssign): value = statement.value - if ( - isinstance(value, ast.Call) - and _is_direct_subprocess_call(value, trusted_names) - and _shell_argument_is_captured_before_effects(value) - ): - self._inspect_call(value, facts) + effectful_direct_call = False + if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): + if _shell_argument_is_captured_before_effects(value): + self._inspect_call(value, facts) + effectful_direct_call = not _call_arguments_are_passive(value) facts.clear() finalizer_safe_names.clear() if value is not None: bound_names.update(_direct_bound_names(statement)) - trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + if effectful_direct_call: + trusted_names.clear() + else: + trusted_names.difference_update( + _changed_direct_names([statement], trusted_names) + ) elif isinstance(statement, (ast.AugAssign, ast.Delete)): facts.clear() finalizer_safe_names.clear() @@ -838,7 +859,8 @@ def _scan_block( direct_call = _is_direct_subprocess_call(call, trusted_names) if direct_call and _shell_argument_is_captured_before_effects(call): self._inspect_call(call, facts) - if direct_call and _call_arguments_are_passive(call): + arguments_are_passive = _call_arguments_are_passive(call) + if direct_call and arguments_are_passive: if not _call_arguments_are_protocol_safe(call, finalizer_safe_names): facts.clear() finalizer_safe_names.clear() @@ -846,7 +868,12 @@ def _scan_block( else: facts.clear() finalizer_safe_names.clear() - trusted_names.difference_update(_changed_direct_names([call], trusted_names)) + if direct_call: + trusted_names.clear() + else: + trusted_names.difference_update( + _changed_direct_names([call], trusted_names) + ) elif isinstance(statement, ast.Pass) or ( isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant) ): diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index a3fd3d38c..190eb6161 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -307,6 +307,51 @@ def test_later_argument_effect_invalidates_fact_after_captured_call() -> None: assert [finding.start_line for finding in findings] == [2] +@pytest.mark.parametrize( + "statement", + [ + pytest.param( + "subprocess.run(command, shell=enabled, env=replace_subprocess())", + id="expression", + ), + pytest.param( + "result = subprocess.run(command, shell=enabled, env=replace_subprocess())", + id="assignment", + ), + pytest.param( + "result: object = subprocess.run(command, shell=enabled, env=replace_subprocess())", + id="annotated-assignment", + ), + ], +) +def test_later_argument_effect_invalidates_receiver_after_captured_call(statement: str) -> None: + findings = _tm1( + "import subprocess\n" + "from helpers import replace_subprocess\n" + "enabled = True\n" + f"{statement}\n" + "later_enabled = True\n" + "subprocess.run(command, shell=later_enabled)\n" + ) + + assert [finding.start_line for finding in findings] == [4] + + +def test_later_argument_effect_invalidates_receiver_for_called_function() -> None: + findings = _tm1( + "import subprocess\n" + "from helpers import replace_subprocess\n" + "enabled = True\n" + "subprocess.run(command, shell=enabled, env=replace_subprocess())\n" + "def execute():\n" + " later_enabled = True\n" + " subprocess.run(command, shell=later_enabled)\n" + "execute()\n" + ) + + assert [finding.start_line for finding in findings] == [4] + + def test_unsupported_assignment_clears_existing_facts() -> None: assert not _tm1("enabled = True\nresult = factory()\nsubprocess.run(cmd, shell=enabled)\n") From 5b0b469be4ef0e627e27ac137a168b2aa9be3213 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Tue, 22 Sep 2026 22:25:52 -0700 Subject: [PATCH 06/11] fix(analyzer): invalidate receiver trust after generic calls Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 31 +++---- .../nodes/analyzers/test_shared_python_ast.py | 2 +- .../analyzers/test_tool_misuse_python_ast.py | 82 ++++++++++++++++++- 3 files changed, 93 insertions(+), 22 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index e0c80d30b..f62583097 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -584,13 +584,11 @@ def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None value = ( statement.value if isinstance(statement, (ast.Expr, ast.Assign, ast.AnnAssign)) else None ) - if ( - isinstance(value, ast.Call) - and _is_direct_subprocess_call(value, trusted_names) - and not _call_arguments_are_passive(value) - ): - trusted_names.clear() - return + if isinstance(value, ast.Call): + direct_subprocess_call = _is_direct_subprocess_call(value, trusted_names) + if not direct_subprocess_call or not _call_arguments_are_passive(value): + trusted_names.clear() + return if isinstance(statement, (ast.Import, ast.ImportFrom)): _update_trusted_names_from_import(statement, trusted_names) return @@ -689,11 +687,11 @@ def _scan_assignment( ) result_is_finalizer_safe = _is_finalizer_safe_value(value, finalizer_safe_names) call_has_protocol_effects = False - effectful_direct_call = False + effectful_call = isinstance(value, ast.Call) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): resolved = None safe_value = _call_arguments_are_passive(value) - effectful_direct_call = not safe_value + effectful_call = not safe_value if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) if safe_value: @@ -711,7 +709,7 @@ def _scan_assignment( for target in targets: if isinstance(target, ast.Name): bound_names.add(target.id) - if effectful_direct_call: + if effectful_call: trusted_names.clear() else: trusted_names.difference_update( @@ -834,16 +832,16 @@ def _scan_block( ) elif isinstance(statement, ast.AnnAssign): value = statement.value - effectful_direct_call = False + effectful_call = isinstance(value, ast.Call) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) - effectful_direct_call = not _call_arguments_are_passive(value) + effectful_call = not _call_arguments_are_passive(value) facts.clear() finalizer_safe_names.clear() if value is not None: bound_names.update(_direct_bound_names(statement)) - if effectful_direct_call: + if effectful_call: trusted_names.clear() else: trusted_names.difference_update( @@ -868,12 +866,7 @@ def _scan_block( else: facts.clear() finalizer_safe_names.clear() - if direct_call: - trusted_names.clear() - else: - trusted_names.difference_update( - _changed_direct_names([call], trusted_names) - ) + trusted_names.clear() elif isinstance(statement, ast.Pass) or ( isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant) ): diff --git a/tests/nodes/analyzers/test_shared_python_ast.py b/tests/nodes/analyzers/test_shared_python_ast.py index 1b152dfbe..ee32a2e6c 100644 --- a/tests/nodes/analyzers/test_shared_python_ast.py +++ b/tests/nodes/analyzers/test_shared_python_ast.py @@ -60,9 +60,9 @@ def test_preparsed_python_is_reused_by_all_ast_analyzers(tmp_path, monkeypatch) """One scan parses each eligible Python file once before analyzer fan-out.""" (tmp_path / "script.py").write_text( "import os\n" - "import subprocess\n" "payload = input()\n" "environment = os.environ.copy()\n" + "import subprocess\n" "enabled = True\n" "subprocess.run(output, shell=enabled)\n" "exec(payload)\n", diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 190eb6161..0eb8e80ac 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -8,6 +8,7 @@ import pytest from skillspector.nodes.analyzers import static_patterns_tool_misuse as tm_module +from skillspector.nodes.analyzers import static_python_shell_truthiness as python_tm_module from skillspector.nodes.deduplicate import deduplicate @@ -19,6 +20,10 @@ def _tm1(content: str, path: str = "run.py") -> list: return [finding for finding in _run(content, path)["findings"] if finding.rule_id == "TM1"] +def _tm1_ast(content: str, path: str = "run.py") -> list: + return python_tm_module.analyze(content, path, "python") + + @pytest.mark.parametrize( "value", [ @@ -127,8 +132,8 @@ def test_explicit_import_reestablishes_direct_receivers() -> None: len( _tm1( "subprocess = Proxy()\n" - "import subprocess\n" "Popen = Proxy()\n" + "import subprocess\n" "from subprocess import Popen\n" "enabled = True\n" "subprocess.run('/usr/bin/true', shell=enabled)\n" @@ -352,6 +357,79 @@ def test_later_argument_effect_invalidates_receiver_for_called_function() -> Non assert [finding.start_line for finding in findings] == [4] +@pytest.mark.parametrize( + "statement", + [ + pytest.param("replace_subprocess()", id="expression"), + pytest.param("result = replace_subprocess()", id="assignment"), + pytest.param("result: object = replace_subprocess()", id="annotated-assignment"), + ], +) +def test_generic_call_invalidates_receiver_trust(statement: str) -> None: + findings = _tm1( + "import subprocess\n" + "from helpers import replace_subprocess\n" + f"{statement}\n" + "enabled = True\n" + "subprocess.run(command, shell=enabled)\n" + ) + + assert not findings + + +@pytest.mark.parametrize( + "statement", + [ + pytest.param("replace_subprocess()", id="expression"), + pytest.param("result = replace_subprocess()", id="assignment"), + pytest.param("result: object = replace_subprocess()", id="annotated-assignment"), + ], +) +def test_generic_call_invalidates_receiver_trust_for_called_function(statement: str) -> None: + findings = _tm1( + "import subprocess\n" + "from helpers import replace_subprocess\n" + "def execute():\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + f"{statement}\n" + "execute()\n" + ) + + assert not findings + + +@pytest.mark.parametrize( + "statement", + [ + pytest.param( + "subprocess.run('/usr/bin/true', shell=enabled)", + id="expression", + ), + pytest.param( + "result = subprocess.run('/usr/bin/true', shell=enabled)", + id="assignment", + ), + pytest.param( + "result: object = subprocess.run('/usr/bin/true', shell=enabled)", + id="annotated-assignment", + ), + ], +) +def test_direct_subprocess_call_remains_detected(statement: str) -> None: + findings = _tm1_ast( + "import subprocess\n" + "def execute():\n" + " later_enabled = True\n" + " subprocess.run('/usr/bin/true', shell=later_enabled)\n" + "enabled = True\n" + f"{statement}\n" + "execute()\n" + ) + + assert [finding.location.start_line for finding in findings] == [4, 6] + + def test_unsupported_assignment_clears_existing_facts() -> None: assert not _tm1("enabled = True\nresult = factory()\nsubprocess.run(cmd, shell=enabled)\n") @@ -394,13 +472,13 @@ def test_external_name_store_treats_prior_binding_as_finalizer_capable() -> None def test_protocol_consuming_direct_call_invalidates_later_truth_fact() -> None: findings = _tm1( - "import subprocess\n" "class MutatingArgs:\n" " def __iter__(self):\n" " global enabled\n" " enabled = False\n" " return iter(('/usr/bin/true',))\n" "mutator = MutatingArgs()\n" + "import subprocess\n" "enabled = True\n" "subprocess.run(mutator, shell=enabled)\n" "subprocess.run('/usr/bin/true', shell=enabled)\n" From 75d182751602513aeb05cb8ffe471f806ae6a4cc Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Wed, 23 Sep 2026 01:39:40 -0700 Subject: [PATCH 07/11] fix(analyzer): reconcile bound shell ownership Signed-off-by: Christopher Kevin --- .../analyzers/static_patterns_tool_misuse.py | 604 +++++++++++++----- .../static_python_shell_truthiness.py | 202 +++++- .../nodes/analyzers/static_runner.py | 2 + .../analyzers/test_tool_misuse_python_ast.py | 274 ++++++++ 4 files changed, 917 insertions(+), 165 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index f27a9c37d..1e13ecee2 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -27,12 +27,13 @@ import ast import re import sys +from bisect import bisect_left, bisect_right from collections.abc import Callable, Iterator from dataclasses import dataclass from skillspector.logging_config import get_logger -from skillspector.models import AnalyzerFinding, Location, Severity -from skillspector.python_ast import parse_python_source +from skillspector.models import AnalyzerFinding, Finding, Location, Severity +from skillspector.python_ast import ParsedPythonFile, parse_python_source from skillspector.security_reconstruction import validated_json_string_spans from skillspector.state import AnalyzerNodeResponse, SkillspectorState @@ -49,6 +50,9 @@ logger = get_logger(__name__) ANALYZER_ID = "static_patterns_tool_misuse" +ANALYZE_USES_POSTPROCESS = True +POSTPROCESS_USES_PYTHON_AST = True +_VARIABLE_SHELL_FLAG_EVIDENCE = "_tm1_variable_shell_flag" _SHELL_COMMAND_WORD_START_RE = re.compile(r"[rRdDeE$'\"`\\]") _SHELL_COMMAND_WORD_CHARS = 4096 @@ -2127,168 +2131,354 @@ def _has_unsupported_brace_expansion(tokens: tuple[_ShellToken, ...]) -> bool: _SCOPE_NODE_TYPES = (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Lambda) +_COMPREHENSION_SCOPE_TYPES = (ast.ListComp, ast.SetComp, ast.DictComp, ast.GeneratorExp) -def _scope_chain(tree: ast.Module, target: ast.AST) -> tuple[ast.AST, ...] | None: - """Return the chain of enclosing scopes for *target*, outermost first. +@dataclass(frozen=True) +class _ScopedShellNode: + """One relevant AST node plus its source and lexical-scope identity.""" - The module itself is the outermost scope. Returns None when *target* is - not part of *tree*. - """ - parents: dict[ast.AST, ast.AST] = {} - stack: list[ast.AST] = [tree] - found = False - while stack: - node = stack.pop() - if node is target: - found = True - break - for child in ast.iter_child_nodes(node): - parents[child] = node - stack.append(child) - if not found: - return None - chain: list[ast.AST] = [] - current: ast.AST | None = target - while current is not None: - if isinstance(current, _SCOPE_NODE_TYPES) or current is tree: - chain.append(current) - current = parents.get(current) - chain.reverse() - return tuple(chain) + node: ast.Assign | ast.Call + scope_chain: tuple[ast.AST, ...] + start: int -def _scope_binds_name(scope_node: ast.AST, name: str) -> bool: - """Return whether *name* is bound directly in *scope_node*. +@dataclass(frozen=True) +class _VariableShellAstIndex: + """Linear-time index used to reconcile every bounded regex candidate.""" + + tree: ast.Module + line_character_starts: tuple[int, ...] + assignments: dict[tuple[int, str], tuple[_ScopedShellNode, ...]] + calls: dict[str, tuple[_ScopedShellNode, ...]] + call_starts: dict[str, tuple[int, ...]] + bindings: dict[ast.AST, frozenset[str]] + class_binding_starts: dict[ast.ClassDef, dict[str, tuple[int, ...]]] + declarations: dict[ast.AST, dict[str, str]] - Nested function/class/lambda bodies are not descended into: their bindings - belong to those scopes, not this one. - """ - if isinstance(scope_node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): - args = scope_node.args - named = [arg.arg for arg in (*args.posonlyargs, *args.args, *args.kwonlyargs)] - if args.vararg is not None: - named.append(args.vararg.arg) - if args.kwarg is not None: - named.append(args.kwarg.arg) - if name in named: - return True - bodies: list[ast.AST] = ( - [scope_node.body] if isinstance(scope_node, ast.Lambda) else list(scope_node.body) - ) - elif isinstance(scope_node, (ast.ClassDef, ast.Module)): - bodies = list(scope_node.body) - else: - return False - stack = list(bodies) - while stack: - node = stack.pop() - if isinstance(node, _SCOPE_NODE_TYPES): - continue - if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) and node.id == name: - return True - stack.extend(ast.iter_child_nodes(node)) - return False +@dataclass(frozen=True) +class _VariableShellCandidate: + """One raw variable-shell regex candidate resolved against the shared AST.""" + + name: str + call: ast.Call + same_scope: bool + visible: bool + + +def _resolved_name_scope( + index: _VariableShellAstIndex, + use_chain: tuple[ast.AST, ...], + name: str, + use_start: int, +) -> ast.AST: + """Return the Python scope that resolves *name* at one use site.""" + crossed_function = False + nonlocal_lookup = False + seen_class_scope = False + for scope in reversed(use_chain): + if scope is index.tree: + return index.tree + declaration = index.declarations.get(scope, {}).get(name) + if declaration == "global": + return index.tree + if declaration == "nonlocal": + nonlocal_lookup = True + crossed_function = True + continue + if isinstance(scope, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + if name in index.bindings.get(scope, frozenset()): + return scope + crossed_function = True + continue + if isinstance(scope, _COMPREHENSION_SCOPE_TYPES): + if name in index.bindings.get(scope, frozenset()): + return scope + # Comprehensions use an implicit function scope. Free names skip a + # surrounding class namespace just like names in a method body. + crossed_function = True + continue + if isinstance(scope, ast.ClassDef): + # A method does not close over its class namespace. A call evaluated + # directly in the class body sees assignments already executed, but + # a later class assignment does not create a compile-time local. + starts = index.class_binding_starts.get(scope, {}).get(name, ()) + if ( + not crossed_function + and not nonlocal_lookup + and not seen_class_scope + and bisect_left(starts, use_start) + ): + return scope + seen_class_scope = True + return index.tree + + +def _node_character_span(parsed: ParsedPythonFile, node: ast.AST) -> tuple[int, int] | None: + """Return one AST node's absolute character span.""" + line = getattr(node, "lineno", None) + end_line = getattr(node, "end_lineno", None) + byte_column = getattr(node, "col_offset", None) + end_byte_column = getattr(node, "end_col_offset", None) + if not all(isinstance(value, int) for value in (line, end_line, byte_column, end_byte_column)): + return None + assert isinstance(line, int) + assert isinstance(end_line, int) + assert isinstance(byte_column, int) + assert isinstance(end_byte_column, int) + start_column = parsed.character_column(line, byte_column) + end_column = parsed.character_column(end_line, end_byte_column) + if start_column is None or end_column is None: + return None + start = parsed.line_character_starts[line - 1] + start_column + end = parsed.line_character_starts[end_line - 1] + end_column + return start, end -def _direct_global_nonlocal(scope_node: ast.AST, name: str) -> str | None: - """Return 'global'/'nonlocal' when *scope_node* declares *name* as such. - Only declarations directly in the scope are considered; nested scopes are - not descended into. - """ - if isinstance(scope_node, ast.Lambda): +def _parameter_names(arguments: ast.arguments) -> tuple[str, ...]: + names = [ + argument.arg + for argument in (*arguments.posonlyargs, *arguments.args, *arguments.kwonlyargs) + ] + if arguments.vararg is not None: + names.append(arguments.vararg.arg) + if arguments.kwarg is not None: + names.append(arguments.kwarg.arg) + return tuple(names) + + +def _direct_shell_name(call: ast.Call) -> str | None: + function = call.func + direct = isinstance(function, ast.Name) and function.id == "Popen" + direct = direct or ( + isinstance(function, ast.Attribute) + and isinstance(function.value, ast.Name) + and function.value.id == "subprocess" + ) + if not direct: return None - bodies: list[ast.stmt] | None = None - if isinstance(scope_node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Module)): - bodies = scope_node.body - if bodies is None: + for keyword in call.keywords: + if keyword.arg == "shell" and isinstance(keyword.value, ast.Name): + return keyword.value.id + return None + + +def _build_variable_shell_ast_index(parsed: ParsedPythonFile) -> _VariableShellAstIndex | None: + """Index relevant nodes, bindings, and declarations in one AST traversal.""" + tree = parsed.tree + if tree is None: return None - stack: list[ast.AST] = list(bodies) + assignments: dict[tuple[int, str], list[_ScopedShellNode]] = {} + calls: dict[str, list[_ScopedShellNode]] = {} + bindings: dict[ast.AST, set[str]] = {tree: set()} + class_binding_starts: dict[ast.ClassDef, dict[str, list[int]]] = {} + declarations: dict[ast.AST, dict[str, str]] = {} + + def bind(scope: ast.AST, name: str, start: int) -> None: + bindings.setdefault(scope, set()).add(name) + if isinstance(scope, ast.ClassDef): + class_binding_starts.setdefault(scope, {}).setdefault(name, []).append(start) + + stack: list[tuple[ast.AST, tuple[ast.AST, ...], int | None]] = [(tree, (tree,), None)] while stack: - node = stack.pop() - if isinstance(node, _SCOPE_NODE_TYPES): + node, scope_chain, binding_start = stack.pop() + scope = scope_chain[-1] + span = _node_character_span(parsed, node) + start = span[0] if span is not None else 0 + end = span[1] if span is not None else start + + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + bind(scope, node.name, end) + nested_chain = (*scope_chain, node) + bindings.setdefault(node, set()).update(_parameter_names(node.args)) + header_nodes: list[ast.AST] = [*node.decorator_list, node.args] + if node.returns is not None: + header_nodes.append(node.returns) + header_nodes.extend(getattr(node, "type_params", [])) + stack.extend((child, scope_chain, None) for child in header_nodes) + stack.extend((child, nested_chain, None) for child in node.body) + continue + if isinstance(node, ast.Lambda): + nested_chain = (*scope_chain, node) + bindings.setdefault(node, set()).update(_parameter_names(node.args)) + stack.append((node.args, scope_chain, None)) + stack.append((node.body, nested_chain, None)) + continue + if isinstance(node, ast.ClassDef): + bind(scope, node.name, end) + nested_chain = (*scope_chain, node) + bindings.setdefault(node, set()) + header_nodes = [*node.decorator_list, *node.bases] + header_nodes.extend(keyword.value for keyword in node.keywords) + header_nodes.extend(getattr(node, "type_params", [])) + stack.extend((child, scope_chain, None) for child in header_nodes) + stack.extend((child, nested_chain, None) for child in node.body) + continue + if isinstance(node, _COMPREHENSION_SCOPE_TYPES): + nested_chain = (*scope_chain, node) + bindings.setdefault(node, set()) + generators = node.generators + if generators: + first, *remaining = generators + # The leftmost iterable is the only comprehension expression + # evaluated in the enclosing scope. + stack.append((first.iter, scope_chain, None)) + stack.append((first.target, nested_chain, None)) + stack.extend((condition, nested_chain, None) for condition in first.ifs) + for generator in remaining: + stack.append((generator.iter, nested_chain, None)) + stack.append((generator.target, nested_chain, None)) + stack.extend((condition, nested_chain, None) for condition in generator.ifs) + if isinstance(node, ast.DictComp): + stack.append((node.key, nested_chain, None)) + stack.append((node.value, nested_chain, None)) + else: + stack.append((node.elt, nested_chain, None)) continue - if isinstance(node, ast.Global) and name in node.names: - return "global" - if isinstance(node, ast.Nonlocal) and name in node.names: - return "nonlocal" - stack.extend(ast.iter_child_nodes(node)) - return None - -def _variable_shell_flag_same_scope(content: str, file_path: str, match: re.Match[str]) -> bool: - """Return whether a variable-shell-flag match is a same-scope data flow. + if isinstance(node, ast.Global): + for name in node.names: + declarations.setdefault(scope, {})[name] = "global" + elif isinstance(node, ast.Nonlocal): + for name in node.names: + declarations.setdefault(scope, {})[name] = "nonlocal" + elif isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store): + bind(scope, node.id, binding_start if binding_start is not None else start) + elif isinstance(node, (ast.Import, ast.ImportFrom)): + for alias in node.names: + if alias.name != "*": + bind(scope, alias.asname or alias.name.split(".", 1)[0], end) + elif isinstance(node, ast.ExceptHandler) and node.name: + type_span = _node_character_span(parsed, node.type) if node.type is not None else None + bind(scope, node.name, type_span[1] if type_span is not None else start) + elif isinstance(node, (ast.MatchAs, ast.MatchStar)) and node.name: + bind(scope, node.name, start) + elif isinstance(node, ast.MatchMapping) and node.rest: + bind(scope, node.rest, start) - The regex cannot see Python scopes, so ``use_shell = True`` in one - function followed by ``shell=use_shell`` in another still matches. Resolve - the matched assignment and the ``shell=`` use through the Python AST and - require the assignment to be visible from the use: identical scope chains, - a closure read from an enclosing scope, or a matching global/nonlocal - declaration. Unparseable content keeps the candidate so a syntax error - cannot silence the signal. - """ - var_name = match.group(1) - assign_line = content.count("\n", 0, match.start()) + 1 - use_line = content.count("\n", 0, match.end()) + 1 - tree = parse_python_source(content, file_path).tree - if tree is None: - return True - assign_node: ast.Assign | None = None - call_node: ast.Call | None = None - for node in ast.walk(tree): if ( - assign_node is None - and isinstance(node, ast.Assign) - and node.lineno == assign_line + isinstance(node, ast.Assign) + and span is not None and isinstance(node.value, ast.Constant) and node.value.value is True - and any( - isinstance(target, ast.Name) and target.id == var_name for target in node.targets - ) ): - assign_node = node - if ( - call_node is None - and isinstance(node, ast.Call) - and any( - keyword.arg == "shell" - and isinstance(keyword.value, ast.Name) - and keyword.value.id == var_name - and assign_line <= keyword.value.lineno <= use_line - for keyword in node.keywords - ) - ): - call_node = node - if assign_node is None or call_node is None: - return True - assign_chain = _scope_chain(tree, assign_node) - use_chain = _scope_chain(tree, call_node) - if assign_chain is None or use_chain is None: - return True - if assign_chain == use_chain: - return True - if len(assign_chain) < len(use_chain) and use_chain[: len(assign_chain)] == assign_chain: - # Closure read: the use sits in a scope nested inside the assignment's - # scope, so the name resolves to the assigned value. - return True - use_scope = use_chain[-1] - if use_scope is not tree: - declaration = _direct_global_nonlocal(use_scope, var_name) - if declaration == "global": - return assign_chain == (tree,) - if declaration == "nonlocal": - binding = next( - ( - scope - for scope in use_chain[-2::-1] - if isinstance(scope, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)) - and _scope_binds_name(scope, var_name) - ), - None, + for target in node.targets: + if isinstance(target, ast.Name): + scoped = _ScopedShellNode(node=node, scope_chain=scope_chain, start=start) + assignments.setdefault((node.lineno, target.id), []).append(scoped) + elif isinstance(node, ast.Call) and span is not None: + shell_name = _direct_shell_name(node) + if shell_name is not None: + scoped = _ScopedShellNode(node=node, scope_chain=scope_chain, start=start) + calls.setdefault(shell_name, []).append(scoped) + + if isinstance(node, ast.Assign): + stack.append((node.value, scope_chain, None)) + stack.extend((target, scope_chain, end) for target in node.targets) + elif isinstance(node, (ast.AnnAssign, ast.AugAssign)): + if node.value is not None: + stack.append((node.value, scope_chain, None)) + stack.append((node.target, scope_chain, end)) + if isinstance(node, ast.AnnAssign): + stack.append((node.annotation, scope_chain, None)) + elif isinstance(node, ast.NamedExpr): + stack.append((node.value, scope_chain, None)) + # PEP 572 makes a walrus target inside one or more + # comprehensions local to the nearest containing real scope. + # Comprehension ``for`` targets remain local to their implicit + # scope, but the named-expression target skips those scopes. + target_chain = scope_chain + while len(target_chain) > 1 and isinstance( + target_chain[-1], _COMPREHENSION_SCOPE_TYPES + ): + target_chain = target_chain[:-1] + stack.append((node.target, target_chain, end)) + elif isinstance(node, (ast.For, ast.AsyncFor)): + iteration_span = _node_character_span(parsed, node.iter) + iteration_end = iteration_span[1] if iteration_span is not None else end + stack.append((node.iter, scope_chain, None)) + stack.append((node.target, scope_chain, iteration_end)) + stack.extend((child, scope_chain, None) for child in (*node.body, *node.orelse)) + elif isinstance(node, ast.withitem): + context_span = _node_character_span(parsed, node.context_expr) + context_end = context_span[1] if context_span is not None else end + stack.append((node.context_expr, scope_chain, None)) + if node.optional_vars is not None: + stack.append((node.optional_vars, scope_chain, context_end)) + else: + stack.extend( + (child, scope_chain, binding_start) for child in ast.iter_child_nodes(node) ) - return binding is not None and assign_chain == _scope_chain(tree, binding) - return False + + frozen_calls = { + name: tuple(sorted(nodes, key=lambda item: item.start)) for name, nodes in calls.items() + } + return _VariableShellAstIndex( + tree=tree, + line_character_starts=parsed.line_character_starts, + assignments={key: tuple(nodes) for key, nodes in assignments.items()}, + calls=frozen_calls, + call_starts={ + name: tuple(item.start for item in nodes) for name, nodes in frozen_calls.items() + }, + bindings={scope: frozenset(names) for scope, names in bindings.items()}, + class_binding_starts={ + scope: {name: tuple(sorted(starts)) for name, starts in names.items()} + for scope, names in class_binding_starts.items() + }, + declarations=declarations, + ) + + +def _assignment_name_scope( + index: _VariableShellAstIndex, + scope_chain: tuple[ast.AST, ...], + name: str, +) -> ast.AST: + """Return the scope mutated by a simple-name assignment.""" + scope = scope_chain[-1] + declaration = index.declarations.get(scope, {}).get(name) + if declaration == "global": + return index.tree + if declaration == "nonlocal": + for outer in reversed(scope_chain[:-1]): + if isinstance(outer, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)) and name in ( + index.bindings.get(outer, frozenset()) + ): + return outer + return scope + + +def _resolve_variable_shell_candidate( + index: _VariableShellAstIndex, + match: re.Match[str], + assignment_line: int, +) -> _VariableShellCandidate | None: + """Resolve a bounded regex match to its exact assignment, call, and binding.""" + name = match.group(1) + assignment_nodes = [ + item + for item in index.assignments.get((assignment_line, name), ()) + if match.start() <= item.start < match.end() + ] + call_nodes = index.calls.get(name, ()) + starts = index.call_starts.get(name, ()) + call_index = bisect_left(starts, match.start()) + if len(assignment_nodes) != 1 or call_index >= len(call_nodes): + return None + assignment = assignment_nodes[0] + call = call_nodes[call_index] + if call.start >= match.end(): + return None + assignment_scope = _assignment_name_scope(index, assignment.scope_chain, name) + resolved_scope = _resolved_name_scope(index, call.scope_chain, name, call.start) + return _VariableShellCandidate( + name=name, + call=call.node, + same_scope=assignment.scope_chain == call.scope_chain, + visible=resolved_scope is assignment_scope, + ) def _tm1_candidates( @@ -2766,7 +2956,13 @@ def _classify_tm1( return Severity.HIGH, adjusted -def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: +def analyze( + content: str, + file_path: str, + file_type: str, + *, + defer_variable_reconciliation: bool = False, +) -> list[AnalyzerFinding]: """Analyze content for tool misuse patterns (TM1–TM3).""" findings: list[AnalyzerFinding] = [] @@ -2779,17 +2975,24 @@ def ctx(start: int) -> str: tag = [PatternCategory.TOOL_MISUSE.value] tm1_findings_by_key: dict[tuple[int, str], AnalyzerFinding] = {} - # The variable-shell-flag regex cannot see Python scopes, so an assignment - # in one function and a shell= use in another still match. Drop those - # cross-scope candidates for Python files. - cross_scope_starts: set[int] = set() - if file_type == "python": - for variable_match in _VARIABLE_SHELL_FLAG_RE.finditer(content): - if not _variable_shell_flag_same_scope(content, file_path, variable_match): - cross_scope_starts.add(variable_match.start()) + variable_matches = { + (match.start(), match.end()): (match.group(1), match) + for match in _VARIABLE_SHELL_FLAG_RE.finditer(content) + } + invisible_variable_matches: set[tuple[int, int]] = set() + if file_type == "python" and variable_matches and not defer_variable_reconciliation: + parsed = parse_python_source(content, file_path) + ast_index = _build_variable_shell_ast_index(parsed) + if ast_index is not None: + for span, (_, match) in variable_matches.items(): + assignment_line = bisect_right(ast_index.line_character_starts, match.start(1)) + candidate = _resolve_variable_shell_candidate(ast_index, match, assignment_line) + if candidate is not None and not candidate.visible: + invisible_variable_matches.add(span) for match_start, match_end, matched_text, confidence in _tm1_candidates(content): - if match_start in cross_scope_starts: + variable_match = variable_matches.get((match_start, match_end)) + if variable_match is not None and (match_start, match_end) in invisible_variable_matches: continue line_num = get_line_number(content, match_start) context_text = ctx(match_start) @@ -2810,6 +3013,9 @@ def ctx(start: int) -> str: existing.confidence = adj existing.severity = sev continue + evidence: dict[str, object] = {static_runner._VIEW_START_EVIDENCE: match_start} + if variable_match is not None and defer_variable_reconciliation: + evidence[_VARIABLE_SHELL_FLAG_EVIDENCE] = variable_match[0] finding = AnalyzerFinding( rule_id="TM1", message="Tool Parameter Abuse", @@ -2820,7 +3026,7 @@ def ctx(start: int) -> str: context=context_text, matched_text=matched, complete_match=matched_text, - evidence={static_runner._VIEW_START_EVIDENCE: match_start}, + evidence=evidence, ) tm1_findings_by_key[candidate_key] = finding findings.append(finding) @@ -2895,6 +3101,94 @@ def ctx(start: int) -> str: return findings +def _bound_shell_call_key(call: ast.Call) -> tuple[int, int, int, int]: + """Return the call key shared with the flow-sensitive companion.""" + return ( + getattr(call, "lineno", 1), + getattr(call, "col_offset", 0), + getattr(call, "end_lineno", getattr(call, "lineno", 1)), + getattr(call, "end_col_offset", getattr(call, "col_offset", 0)), + ) + + +def cleanup_path_findings(findings: list[Finding]) -> list[Finding]: + """Remove private reconciliation evidence when postprocessing times out.""" + for finding in findings: + finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) + return findings + + +def postprocess_path_findings( + content: str, + findings: list[Finding], + *, + python_ast: ParsedPythonFile | None, +) -> list[Finding]: + """Reconcile the legacy variable regex with the Python AST companion.""" + marked = [ + finding + for finding in findings + if isinstance(finding.evidence.get(_VARIABLE_SHELL_FLAG_EVIDENCE), str) + ] + if not marked: + return findings + + file_path = marked[0].file + file_type = static_runner._infer_file_type(file_path) + if file_type != "python": + marked_ids = {id(finding) for finding in marked} + for finding in marked: + finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) + return [finding for finding in findings if id(finding) not in marked_ids] + if python_ast is None or python_ast.tree is None: + return cleanup_path_findings(findings) + + from . import static_python_shell_truthiness + + ast_index = _build_variable_shell_ast_index(python_ast) + if ast_index is None: + return cleanup_path_findings(findings) + resolved: dict[tuple[int, str], list[_VariableShellCandidate]] = {} + for match in _VARIABLE_SHELL_FLAG_RE.finditer(content): + finding_line = bisect_right(ast_index.line_character_starts, match.start()) + assignment_line = bisect_right(ast_index.line_character_starts, match.start(1)) + candidate = _resolve_variable_shell_candidate(ast_index, match, assignment_line) + if candidate is None: + continue + key = (finding_line, candidate.name) + resolved.setdefault(key, []).append(candidate) + ownership = static_python_shell_truthiness.bound_shell_call_ownership( + file_path, + python_ast, + ) + + reconciled: list[Finding] = [] + for finding in findings: + variable_name = finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) + if not isinstance(variable_name, str): + reconciled.append(finding) + continue + candidates = resolved.get((finding.start_line, variable_name), []) + if len(candidates) != 1: + # Derived views and ambiguous recovery candidates retain the + # conservative lexical signal. + reconciled.append(finding) + continue + candidate = candidates[0] + if variable_name.casefold().startswith("true") or not candidate.visible: + continue + call_key = _bound_shell_call_key(candidate.call) + if candidate.same_scope: + # Parsed same-scope data flow belongs exclusively to the companion, + # including constructs it rejects as outside its straight-line + # contract. + continue + elif call_key in ownership and ownership[call_key] is False: + continue + reconciled.append(finding) + return reconciled + + def node(state: SkillspectorState) -> AnalyzerNodeResponse: """Run tool_misuse patterns and return findings.""" from . import static_python_shell_truthiness diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index f62583097..70b75ac7a 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -23,6 +23,17 @@ USES_PYTHON_AST = True BOUND_SHELL_EVIDENCE = "_tm1_bound_shell_value" _DIRECT_CALL_NAMES = frozenset({"subprocess", "Popen"}) +BoundShellCallKey = tuple[int, int, int, int] + + +def _bound_shell_call_key(call: ast.Call) -> BoundShellCallKey: + """Return a stable source key for one call node.""" + return ( + getattr(call, "lineno", 1), + getattr(call, "col_offset", 0), + getattr(call, "end_lineno", getattr(call, "lineno", 1)), + getattr(call, "end_col_offset", getattr(call, "col_offset", 0)), + ) def _truth_value( @@ -393,18 +404,111 @@ def nested_classes(node: ast.AST) -> list[ast.ClassDef]: return affected -def _is_direct_subprocess_call(call: ast.Call, trusted_names: set[str]) -> bool: +def _class_deferred_receiver_trust( + statement: ast.ClassDef, + trusted_names: set[str], +) -> tuple[set[str], dict[int, set[str]]]: + """Return final and observed-call outer trust for deferred class methods.""" + deferred = set(trusted_names) + trusted_at_call_by_definition: dict[int, set[str]] = {} + active_functions: dict[str, int] = {} + declarations = _DirectBindingCollector(_DIRECT_CALL_NAMES) + for child in statement.body: + declarations.visit(child) + global_names = declarations.nonlocal_names.intersection(_DIRECT_CALL_NAMES) + + for child_index, child in enumerate(statement.body): + call = _passive_direct_call(child) + if call is not None: + assert isinstance(call.func, ast.Name) + owner = active_functions.get(call.func.id) + if owner is not None: + trusted_at_call_by_definition.setdefault(owner, set()).update(deferred) + + pending = [child] + contains_call = False + while pending: + current = pending.pop() + if isinstance(current, ast.Call): + contains_call = True + break + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef)): + pending.extend(_DirectBindingCollector._function_header_nodes(current)) + continue + if isinstance(current, ast.Lambda): + pending.extend(current.args.defaults) + pending.extend(item for item in current.args.kw_defaults if item is not None) + continue + pending.extend(ast.iter_child_nodes(current)) + if contains_call: + # Class-body expressions run before any method can be called and may + # mutate the surrounding module/function receiver binding. + deferred.clear() + + collector = _DirectBindingCollector(_DIRECT_CALL_NAMES) + collector.visit(child) + deferred.difference_update(collector.mutated) + globally_bound = collector.bound.intersection(global_names) + if isinstance(child, ast.Import): + for imported in child.names: + bound = imported.asname or imported.name.partition(".")[0] + if bound not in globally_bound: + continue + if imported.name == "subprocess" and bound == "subprocess": + deferred.add(bound) + else: + deferred.discard(bound) + elif isinstance(child, ast.ImportFrom): + if any(imported.name == "*" for imported in child.names) and global_names: + deferred.clear() + for imported in child.names: + bound = imported.asname or imported.name + if bound not in globally_bound: + continue + if ( + child.level == 0 + and child.module == "subprocess" + and imported.name == "Popen" + and bound == "Popen" + ): + deferred.add(bound) + else: + deferred.discard(bound) + else: + deferred.difference_update(globally_bound) + + changed_names = _direct_bound_names(child) + for name in changed_names: + active_functions.pop(name, None) + if ( + isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)) + and _function_header_is_passive(child) + and _is_immediate_function(child) + ): + active_functions[child.name] = child_index + return deferred, trusted_at_call_by_definition + + +def _is_direct_subprocess_syntax(call: ast.Call) -> bool: + """Return whether a call uses one of the direct subprocess spellings.""" function = call.func if isinstance(function, ast.Name): - return function.id == "Popen" and function.id in trusted_names + return function.id == "Popen" return ( isinstance(function, ast.Attribute) and isinstance(function.value, ast.Name) and function.value.id == "subprocess" - and function.value.id in trusted_names ) +def _is_direct_subprocess_call(call: ast.Call, trusted_names: set[str]) -> bool: + if not _is_direct_subprocess_syntax(call): + return False + function = call.func + receiver = function.id if isinstance(function, ast.Name) else function.value.id + return receiver in trusted_names + + def _is_passive_argument(expression: ast.expr) -> bool: """Return whether evaluation cannot invoke user-controlled Python code.""" normal, hash_required, truth_required, numeric_required, integral_required = range(5) @@ -548,6 +652,17 @@ def _function_header_is_passive( return all(_annotation_is_passive(annotation) for annotation in annotations) +def _class_header_is_passive(statement: ast.ClassDef) -> bool: + """Return whether evaluating a class header cannot rebind a receiver.""" + expressions = [ + *statement.decorator_list, + *statement.bases, + *(keyword.value for keyword in statement.keywords), + *getattr(statement, "type_params", []), + ] + return all(_is_passive_argument(expression) for expression in expressions) + + def _is_immediate_function(statement: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: """Return whether a direct call begins executing this function body.""" if isinstance(statement, ast.AsyncFunctionDef): @@ -625,6 +740,21 @@ def __init__(self, file_path: str, python_ast: ParsedPythonFile) -> None: self.python_ast = python_ast self.lines = python_ast.lines self.findings: list[AnalyzerFinding] = [] + self.bound_shell_call_ownership: dict[BoundShellCallKey, bool] = {} + + def _record_bound_shell_call(self, call: ast.Call, trusted_names: set[str]) -> None: + """Record whether the companion owns one supported bound-shell call.""" + shell = next((item.value for item in call.keywords if item.arg == "shell"), None) + if ( + not _is_direct_subprocess_syntax(call) + or not isinstance(shell, ast.Name) + or shell.id.casefold().startswith("true") + ): + return + self.bound_shell_call_ownership[_bound_shell_call_key(call)] = bool( + _is_direct_subprocess_call(call, trusted_names) + and _shell_argument_is_captured_before_effects(call) + ) def _inspect_call(self, call: ast.Call, facts: dict[str, bool]) -> None: shell = next((item.value for item in call.keywords if item.arg == "shell"), None) @@ -688,6 +818,8 @@ def _scan_assignment( result_is_finalizer_safe = _is_finalizer_safe_value(value, finalizer_safe_names) call_has_protocol_effects = False effectful_call = isinstance(value, ast.Call) + if isinstance(value, ast.Call): + self._record_bound_shell_call(value, trusted_names) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): resolved = None safe_value = _call_arguments_are_passive(value) @@ -747,6 +879,8 @@ def _scan_block( *, trusted_names: set[str] | None = None, initial_bound_names: set[str] | None = None, + nested_function_trusted_names: set[str] | None = None, + nested_function_trusted_at_call: dict[int, set[str]] | None = None, ) -> None: trusted_names = set(_DIRECT_CALL_NAMES if trusted_names is None else trusted_names) facts: dict[str, bool] = {} @@ -786,13 +920,19 @@ def _scan_block( for index, statement in enumerate(statements): if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): passive_header = _function_header_is_passive(statement) - trusted_at_call = trusted_at_call_by_definition.get(index, set()) - nested_trusted_names = set(trusted_names).union(trusted_at_call) - nested_trusted_names = { - name - for name in nested_trusted_names - if last_invalidation_by_name.get(name, -1) <= index or name in trusted_at_call - } + if nested_function_trusted_names is None: + trusted_at_call = trusted_at_call_by_definition.get(index, set()) + nested_trusted_names = set(trusted_names).union(trusted_at_call) + nested_trusted_names = { + name + for name in nested_trusted_names + if last_invalidation_by_name.get(name, -1) <= index + or name in trusted_at_call + } + else: + nested_trusted_names = set(nested_function_trusted_names).union( + (nested_function_trusted_at_call or {}).get(index, set()) + ) nested_trusted_names.difference_update( _function_bound_direct_names(statement, nested_trusted_names) ) @@ -833,6 +973,8 @@ def _scan_block( elif isinstance(statement, ast.AnnAssign): value = statement.value effectful_call = isinstance(value, ast.Call) + if isinstance(value, ast.Call): + self._record_bound_shell_call(value, trusted_names) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) @@ -854,6 +996,7 @@ def _scan_block( trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) elif isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Call): call = statement.value + self._record_bound_shell_call(call, trusted_names) direct_call = _is_direct_subprocess_call(call, trusted_names) if direct_call and _shell_argument_is_captured_before_effects(call): self._inspect_call(call, facts) @@ -872,6 +1015,33 @@ def _scan_block( ): continue elif isinstance(statement, ast.ClassDef): + class_trusted_names = set(trusted_names) + passive_class_header = _class_header_is_passive(statement) + if not passive_class_header: + class_trusted_names.clear() + method_outer_trust = set( + trusted_names + if nested_function_trusted_names is None + else nested_function_trusted_names + ) + if nested_function_trusted_names is None: + method_outer_trust = { + name + for name in method_outer_trust + if last_invalidation_by_name.get(name, -1) <= index + } + if not passive_class_header: + method_outer_trust.clear() + method_trusted_names, method_trusted_at_call = _class_deferred_receiver_trust( + statement, + method_outer_trust, + ) + self._scan_block( + statement.body, + trusted_names=class_trusted_names, + nested_function_trusted_names=method_trusted_names, + nested_function_trusted_at_call=method_trusted_at_call, + ) facts.clear() finalizer_safe_names.clear() bound_names.update(_direct_bound_names(statement)) @@ -904,3 +1074,15 @@ def analyze( if parsed.tree is None: return [] return _Analyzer(file_path, parsed).run(parsed.tree) + + +def bound_shell_call_ownership( + file_path: str, + python_ast: ParsedPythonFile, +) -> dict[BoundShellCallKey, bool]: + """Return supported bound-shell calls and whether their receiver is trusted.""" + if python_ast.tree is None: + return {} + analyzer = _Analyzer(file_path, python_ast) + analyzer.run(python_ast.tree) + return dict(analyzer.bound_shell_call_ownership) diff --git a/src/skillspector/nodes/analyzers/static_runner.py b/src/skillspector/nodes/analyzers/static_runner.py index f8d7cc1b9..32cd4e5d7 100644 --- a/src/skillspector/nodes/analyzers/static_runner.py +++ b/src/skillspector/nodes/analyzers/static_runner.py @@ -878,6 +878,8 @@ def _scan_path( analyze_kwargs["python_ast"] = python_ast if _uses_runtime_check(module): analyze_kwargs["check_runtime"] = finding_budget.check_runtime + if _explicit_module_hook(module, "ANALYZE_USES_POSTPROCESS") is True: + analyze_kwargs["defer_variable_reconciliation"] = True raw = module.analyze(**analyze_kwargs) finding_budget.check_runtime() for af in raw: diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 0eb8e80ac..0934e0152 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -227,6 +227,173 @@ def test_calls_inside_compound_statements_are_out_of_scope() -> None: ) +def test_class_body_same_scope_binding_is_tracked() -> None: + findings = _tm1( + "class Runner:\n enabled = True\n subprocess.run(command, shell=enabled)\n" + ) + + assert [finding.start_line for finding in findings] == [3] + + +def test_generic_call_invalidates_receiver_trust_in_class_body() -> None: + assert not _tm1( + "class Runner:\n" + " replace_subprocess()\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + ) + + +def test_class_method_does_not_close_over_class_binding() -> None: + assert not _tm1( + "class Runner:\n" + " enabled = True\n" + " def run(self):\n" + " subprocess.run(command, shell=enabled)\n" + ) + + +def test_class_body_lookup_precedes_later_class_binding() -> None: + findings = _tm1( + "enabled = True\n" + "class Runner:\n" + " subprocess.run(command, shell=enabled)\n" + " enabled = False\n" + ) + + assert [finding.start_line for finding in findings] == [1] + + +@pytest.mark.parametrize( + "class_body", + [ + pytest.param( + " enabled = subprocess.run(command, shell=enabled)\n", + id="assignment-rhs-before-store", + ), + pytest.param( + " def enabled(value=subprocess.run(command, shell=enabled)):\n pass\n", + id="function-default-before-name-binding", + ), + pytest.param( + " with manager(subprocess.run(command, shell=enabled)) as enabled:\n pass\n", + id="with-target-after-context", + ), + pytest.param( + " try:\n" + " raise Error\n" + " except subprocess.run(command, shell=enabled) as enabled:\n" + " pass\n", + id="except-target-after-type", + ), + ], +) +def test_class_binding_is_installed_after_value_evaluation(class_body: str) -> None: + findings = _tm1("enabled = True\nclass Runner:\n" + class_body) + + assert [finding.start_line for finding in findings] == [1] + + +def test_class_local_import_does_not_establish_method_receiver_trust() -> None: + assert not _tm1( + "subprocess = proxy\n" + "class Runner:\n" + " import subprocess\n" + " def run(self):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + ) + + +def test_class_local_shadow_does_not_hide_global_method_receiver() -> None: + findings = _tm1( + "import subprocess\n" + "class Runner:\n" + " subprocess = proxy\n" + " def run(self):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + ) + + assert [finding.start_line for finding in findings] == [6] + + +def test_class_body_generic_call_invalidates_deferred_method_receiver() -> None: + assert not _tm1( + "import subprocess\n" + "class Runner:\n" + " replace_subprocess()\n" + " def run(self):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + ) + + +def test_called_class_method_preserves_receiver_before_later_invalidation() -> None: + findings = _tm1( + "import subprocess\n" + "class Runner:\n" + " def run():\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + " run()\n" + " replace_subprocess()\n" + ) + + assert [finding.start_line for finding in findings] == [5] + + +def test_class_method_call_after_invalidation_is_rejected() -> None: + assert not _tm1( + "import subprocess\n" + "class Runner:\n" + " def run():\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + " replace_subprocess()\n" + " run()\n" + ) + + +@pytest.mark.parametrize( + "class_header", + [ + pytest.param("@replace_subprocess()\nclass Runner:", id="decorator"), + pytest.param("class Runner(replace_subprocess()):", id="base"), + ], +) +def test_effectful_class_header_invalidates_deferred_method_receiver(class_header: str) -> None: + assert not _tm1( + "import subprocess\n" + f"{class_header}\n" + " def run(self):\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + ) + + +def test_class_for_target_is_bound_after_iterable_evaluation() -> None: + findings = _tm1( + "enabled = True\n" + "class Runner:\n" + " for enabled in subprocess.run(command, shell=enabled):\n" + " pass\n" + ) + + assert [finding.start_line for finding in findings] == [1] + + +def test_comprehension_target_shadows_outer_shell_flag() -> None: + assert not _tm1( + "enabled = True\n" + "class Runner:\n" + " values = [\n" + " subprocess.run(command, shell=enabled)\n" + " for enabled in items\n" + " ]\n" + ) + + @pytest.mark.parametrize( "argument", [ @@ -399,6 +566,19 @@ def test_generic_call_invalidates_receiver_trust_for_called_function(statement: assert not findings +def test_generic_call_invalidates_receiver_trust_for_nested_closure() -> None: + assert not _tm1( + "import subprocess\n" + "def outer():\n" + " enabled = True\n" + " def inner():\n" + " subprocess.run(command, shell=enabled)\n" + " replace_subprocess()\n" + " inner()\n" + "outer()\n" + ) + + @pytest.mark.parametrize( "statement", [ @@ -430,6 +610,13 @@ def test_direct_subprocess_call_remains_detected(statement: str) -> None: assert [finding.location.start_line for finding in findings] == [4, 6] +def test_blank_line_before_assignment_has_one_tm1_owner() -> None: + findings = _tm1("import subprocess\n\nenabled = True\nsubprocess.run(command, shell=enabled)\n") + + assert len(findings) == 1 + assert findings[0].start_line == 4 + + def test_unsupported_assignment_clears_existing_facts() -> None: assert not _tm1("enabled = True\nresult = factory()\nsubprocess.run(cmd, shell=enabled)\n") @@ -504,6 +691,93 @@ def test_true_prefixed_identifier_has_one_lexical_owner() -> None: assert len(findings) == 1 +def test_true_prefixed_closure_has_one_lexical_owner() -> None: + findings = _tm1( + "def outer():\n" + " true_value = True\n" + " def inner():\n" + " subprocess.run(command, shell=true_value)\n" + ) + + assert len(findings) == 1 + assert findings[0].start_line == 4 + + +@pytest.mark.parametrize( + "content", + [ + pytest.param( + "enabled = True\ndef run(enabled):\n subprocess.run(command, shell=enabled)\n", + id="parameter-shadow", + ), + pytest.param( + "enabled = True\ndef run():\n" + " subprocess.run(command, shell=enabled)\n enabled = False\n", + id="later-local-shadow", + ), + pytest.param( + "def outer():\n enabled = True\n def inner():\n" + " global enabled\n subprocess.run(command, shell=enabled)\n", + id="global-redirect", + ), + ], +) +def test_cross_scope_binding_must_resolve_to_literal_assignment(content: str) -> None: + assert not _tm1(content) + + +@pytest.mark.parametrize( + "content", + [ + pytest.param( + "import subprocess\n" + "enabled = True\n" + "values = [(subprocess.run(command, shell=enabled), (enabled := False)) " + "for item in items]\n", + id="module", + ), + pytest.param( + "import subprocess\n" + "def execute():\n" + " enabled = True\n" + " return [(subprocess.run(command, shell=enabled), " + "(enabled := False)) for item in items]\n", + id="function", + ), + pytest.param( + "import subprocess\n" + "enabled = True\n" + "values = [[(subprocess.run(command, shell=enabled), (enabled := False)) " + "for inner in inners] for outer in outers]\n", + id="nested-comprehension", + ), + ], +) +def test_comprehension_walrus_binds_in_containing_scope(content: str) -> None: + findings = _tm1(content) + + assert len(findings) == 1 + + +def test_malformed_python_keeps_bounded_lexical_fallback() -> None: + findings = _tm1("enabled = True\nsubprocess.run(command, shell=enabled)\nif:\n") + + assert len(findings) == 1 + assert "_tm1_variable_shell_flag" not in findings[0].evidence + + +def test_oversized_python_keeps_bounded_lexical_fallback( + monkeypatch: pytest.MonkeyPatch, +) -> None: + content = "enabled = True\nsubprocess.run(command, shell=enabled)\n" + "# padding\n" * 20 + monkeypatch.setattr(tm_module.static_runner, "MAX_FILE_CHARS", 80) + + findings = _tm1(content) + + assert len(findings) == 1 + assert "_tm1_variable_shell_flag" not in findings[0].evidence + + def test_long_same_line_calls_keep_exact_coordinates_and_distinct_identity() -> None: payload = "x" * 240 first_call = f'subprocess.run("{payload}A", shell=enabled)' From 650de7e29df712fcdcff580aff248fac74db04d9 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Wed, 23 Sep 2026 03:43:34 -0700 Subject: [PATCH 08/11] fix(analyzer): reconcile true-prefixed shell ownership Signed-off-by: Christopher Kevin --- .../analyzers/static_patterns_tool_misuse.py | 37 ++++++-- .../static_python_shell_truthiness.py | 6 +- .../analyzers/test_tool_misuse_python_ast.py | 92 +++++++++++++++++-- 3 files changed, 118 insertions(+), 17 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 1e13ecee2..ea463f68a 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -53,6 +53,8 @@ ANALYZE_USES_POSTPROCESS = True POSTPROCESS_USES_PYTHON_AST = True _VARIABLE_SHELL_FLAG_EVIDENCE = "_tm1_variable_shell_flag" +_TRUE_SHELL_DIRECT_EVIDENCE = "_tm1_true_shell_direct" +_TRUE_SHELL_ARGUMENT_RE = re.compile(r"\bshell\s*=\s*true", re.IGNORECASE) _SHELL_COMMAND_WORD_START_RE = re.compile(r"[rRdDeE$'\"`\\]") _SHELL_COMMAND_WORD_CHARS = 4096 @@ -2973,7 +2975,7 @@ def ctx(start: int) -> str: return get_context(content, start) tag = [PatternCategory.TOOL_MISUSE.value] - tm1_findings_by_key: dict[tuple[int, str], AnalyzerFinding] = {} + tm1_findings_by_key: dict[tuple[int, str, int], AnalyzerFinding] = {} variable_matches = { (match.start(), match.end()): (match.group(1), match) @@ -2992,6 +2994,10 @@ def ctx(start: int) -> str: for match_start, match_end, matched_text, confidence in _tm1_candidates(content): variable_match = variable_matches.get((match_start, match_end)) + if variable_match is not None and variable_match[0].casefold().startswith("true"): + # The case-insensitive direct ``shell=True`` pattern already owns + # true-prefixed names at the exact call location. + continue if variable_match is not None and (match_start, match_end) in invisible_variable_matches: continue line_num = get_line_number(content, match_start) @@ -3006,7 +3012,7 @@ def ctx(start: int) -> str: confidence, file_type, ) - candidate_key = (line_num, " ".join(matched.strip().split())) + candidate_key = (line_num, " ".join(matched.strip().split()), match_start) existing = tm1_findings_by_key.get(candidate_key) if existing is not None: if adj > existing.confidence: @@ -3016,6 +3022,8 @@ def ctx(start: int) -> str: evidence: dict[str, object] = {static_runner._VIEW_START_EVIDENCE: match_start} if variable_match is not None and defer_variable_reconciliation: evidence[_VARIABLE_SHELL_FLAG_EVIDENCE] = variable_match[0] + elif defer_variable_reconciliation and _TRUE_SHELL_ARGUMENT_RE.search(matched_text): + evidence[_TRUE_SHELL_DIRECT_EVIDENCE] = True finding = AnalyzerFinding( rule_id="TM1", message="Tool Parameter Abuse", @@ -3115,6 +3123,7 @@ def cleanup_path_findings(findings: list[Finding]) -> list[Finding]: """Remove private reconciliation evidence when postprocessing times out.""" for finding in findings: finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) + finding.evidence.pop(_TRUE_SHELL_DIRECT_EVIDENCE, None) return findings @@ -3129,6 +3138,7 @@ def postprocess_path_findings( finding for finding in findings if isinstance(finding.evidence.get(_VARIABLE_SHELL_FLAG_EVIDENCE), str) + or finding.evidence.get(_TRUE_SHELL_DIRECT_EVIDENCE) is True ] if not marked: return findings @@ -3136,10 +3146,13 @@ def postprocess_path_findings( file_path = marked[0].file file_type = static_runner._infer_file_type(file_path) if file_type != "python": - marked_ids = {id(finding) for finding in marked} - for finding in marked: - finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) - return [finding for finding in findings if id(finding) not in marked_ids] + reconciled: list[Finding] = [] + for finding in findings: + variable_name = finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) + finding.evidence.pop(_TRUE_SHELL_DIRECT_EVIDENCE, None) + if not isinstance(variable_name, str): + reconciled.append(finding) + return reconciled if python_ast is None or python_ast.tree is None: return cleanup_path_findings(findings) @@ -3161,10 +3174,22 @@ def postprocess_path_findings( file_path, python_ast, ) + ownership_by_start = { + (line, column): trusted + for (line, byte_column, _, _), trusted in ownership.items() + if (column := python_ast.character_column(line, byte_column)) is not None + } reconciled: list[Finding] = [] for finding in findings: variable_name = finding.evidence.pop(_VARIABLE_SHELL_FLAG_EVIDENCE, None) + direct_true = finding.evidence.pop(_TRUE_SHELL_DIRECT_EVIDENCE, None) + if direct_true is True: + location = (finding.start_line, finding.start_column) + if finding.start_column is not None and ownership_by_start.get(location) is False: + continue + reconciled.append(finding) + continue if not isinstance(variable_name, str): reconciled.append(finding) continue diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 70b75ac7a..4c9081e5b 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -745,11 +745,7 @@ def __init__(self, file_path: str, python_ast: ParsedPythonFile) -> None: def _record_bound_shell_call(self, call: ast.Call, trusted_names: set[str]) -> None: """Record whether the companion owns one supported bound-shell call.""" shell = next((item.value for item in call.keywords if item.arg == "shell"), None) - if ( - not _is_direct_subprocess_syntax(call) - or not isinstance(shell, ast.Name) - or shell.id.casefold().startswith("true") - ): + if not _is_direct_subprocess_syntax(call) or not isinstance(shell, ast.Name): return self.bound_shell_call_ownership[_bound_shell_call_key(call)] = bool( _is_direct_subprocess_call(call, trusted_names) diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 0934e0152..573011900 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -691,16 +691,94 @@ def test_true_prefixed_identifier_has_one_lexical_owner() -> None: assert len(findings) == 1 +@pytest.mark.parametrize("name", ["true", "TRUE", "TrUe"]) +def test_case_variant_true_has_one_direct_owner(name: str) -> None: + findings = _tm1(f"{name} = True\nsubprocess.run(command, shell={name})\n") + + assert len(findings) == 1 + assert findings[0].start_line == 2 + assert not findings[0].evidence + + def test_true_prefixed_closure_has_one_lexical_owner() -> None: findings = _tm1( - "def outer():\n" - " true_value = True\n" - " def inner():\n" - " subprocess.run(command, shell=true_value)\n" + "true_value = True\n" + "def inner():\n" + " # one\n" + " # two\n" + " # three\n" + " subprocess.run(command, shell=true_value)\n" ) assert len(findings) == 1 - assert findings[0].start_line == 4 + assert findings[0].start_line == 6 + snippet = str(findings[0].to_dict()["code_snippet"]) + assert "subprocess.run" in snippet + assert "true_value = True" not in snippet + assert findings[0].severity == "HIGH" + assert findings[0].confidence == pytest.approx(0.9) + + +def test_normalized_prefix_does_not_restore_invalidated_true_prefixed_call() -> None: + assert not _tm1( + "true_value = True\n" + "def inner():\n" + " ff; subprocess.run(command, shell=true_value)\n" + "replace_subprocess()\n" + "inner()\n" + ) + + +def test_normalized_prefix_does_not_duplicate_true_direct_fallback() -> None: + findings = _tm1("true = True\nff; subprocess.run(command, shell=true)\n") + + assert len(findings) == 1 + assert (findings[0].start_line, findings[0].start_column) == (2, 3) + assert not findings[0].evidence + + +@pytest.mark.parametrize( + "call_line", + [ + "subprocess.run(command, shell=true); " + "[subprocess.run(command, shell=true) for item in items]", + "[subprocess.run(command, shell=true) for item in items]; " + "subprocess.run(command, shell=true)", + ], +) +def test_true_direct_calls_on_one_line_keep_distinct_locations(call_line: str) -> None: + findings = _tm1(f"true = True\n{call_line}\n") + expected_columns = [ + index for index in range(len(call_line)) if call_line.startswith("subprocess.run", index) + ] + + assert len(findings) == 2 + assert sorted(finding.start_column for finding in findings) == expected_columns + + +def test_true_direct_ownership_invalidation_is_per_call() -> None: + findings = _tm1( + "true = True\n" + "subprocess.run(command, shell=true)\n" + "replace_subprocess()\n" + "subprocess.run(command, shell=true)\n" + ) + + assert [finding.start_line for finding in findings] == [2] + + +@pytest.mark.parametrize( + ("path", "suffix"), + [ + pytest.param("run.py", "if:\n", id="malformed-python"), + pytest.param("run.js", "", id="non-python"), + ], +) +def test_true_direct_fallback_survives_without_ast_ownership(path: str, suffix: str) -> None: + findings = _tm1(f"true = True\nsubprocess.run(command, shell=true)\n{suffix}", path) + + assert len(findings) == 1 + assert not findings[0].evidence @pytest.mark.parametrize( @@ -766,10 +844,12 @@ def test_malformed_python_keeps_bounded_lexical_fallback() -> None: assert "_tm1_variable_shell_flag" not in findings[0].evidence +@pytest.mark.parametrize("name", ["enabled", "true"]) def test_oversized_python_keeps_bounded_lexical_fallback( monkeypatch: pytest.MonkeyPatch, + name: str, ) -> None: - content = "enabled = True\nsubprocess.run(command, shell=enabled)\n" + "# padding\n" * 20 + content = f"{name} = True\nsubprocess.run(command, shell={name})\n" + "# padding\n" * 20 monkeypatch.setattr(tm_module.static_runner, "MAX_FILE_CHARS", 80) findings = _tm1(content) From c6f6b28e08c6d72458add73ece6767bccb6f0353 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Mon, 28 Sep 2026 13:12:28 -0700 Subject: [PATCH 09/11] fix(analyzer): invalidate nested RHS receiver effects Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 41 ++++++++++++++----- .../analyzers/test_tool_misuse_python_ast.py | 30 ++++++++++++++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 4c9081e5b..1dd821627 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -587,6 +587,17 @@ def _call_arguments_are_passive(call: ast.Call) -> bool: ) +def _value_preserves_receiver_trust(expression: ast.expr, trusted_names: set[str]) -> bool: + """Return whether evaluating a value cannot replace a trusted receiver.""" + if _is_passive_argument(expression): + return True + return ( + isinstance(expression, ast.Call) + and _is_direct_subprocess_call(expression, trusted_names) + and _call_arguments_are_passive(expression) + ) + + def _shell_argument_is_captured_before_effects(call: ast.Call) -> bool: """Return whether evaluation reaches ``shell=`` without user-code effects. @@ -699,11 +710,12 @@ def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None value = ( statement.value if isinstance(statement, (ast.Expr, ast.Assign, ast.AnnAssign)) else None ) - if isinstance(value, ast.Call): - direct_subprocess_call = _is_direct_subprocess_call(value, trusted_names) - if not direct_subprocess_call or not _call_arguments_are_passive(value): - trusted_names.clear() - return + if value is not None and not _value_preserves_receiver_trust(value, trusted_names): + trusted_names.clear() + return + if isinstance(statement, ast.AnnAssign) and not _annotation_is_passive(statement.annotation): + trusted_names.clear() + return if isinstance(statement, (ast.Import, ast.ImportFrom)): _update_trusted_names_from_import(statement, trusted_names) return @@ -813,13 +825,12 @@ def _scan_assignment( ) result_is_finalizer_safe = _is_finalizer_safe_value(value, finalizer_safe_names) call_has_protocol_effects = False - effectful_call = isinstance(value, ast.Call) + preserves_receiver_trust = _value_preserves_receiver_trust(value, trusted_names) if isinstance(value, ast.Call): self._record_bound_shell_call(value, trusted_names) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): resolved = None safe_value = _call_arguments_are_passive(value) - effectful_call = not safe_value if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) if safe_value: @@ -837,7 +848,7 @@ def _scan_assignment( for target in targets: if isinstance(target, ast.Name): bound_names.add(target.id) - if effectful_call: + if not preserves_receiver_trust: trusted_names.clear() else: trusted_names.difference_update( @@ -968,18 +979,26 @@ def _scan_block( ) elif isinstance(statement, ast.AnnAssign): value = statement.value - effectful_call = isinstance(value, ast.Call) + preserves_receiver_trust = value is None or _value_preserves_receiver_trust( + value, + trusted_names, + ) if isinstance(value, ast.Call): self._record_bound_shell_call(value, trusted_names) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) - effectful_call = not _call_arguments_are_passive(value) + preserves_receiver_trust = ( + preserves_receiver_trust + and _call_arguments_are_protocol_safe(value, finalizer_safe_names) + ) + if not _annotation_is_passive(statement.annotation): + preserves_receiver_trust = False facts.clear() finalizer_safe_names.clear() if value is not None: bound_names.update(_direct_bound_names(statement)) - if effectful_call: + if not preserves_receiver_trust: trusted_names.clear() else: trusted_names.difference_update( diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 573011900..a94a01b60 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -530,6 +530,21 @@ def test_later_argument_effect_invalidates_receiver_for_called_function() -> Non pytest.param("replace_subprocess()", id="expression"), pytest.param("result = replace_subprocess()", id="assignment"), pytest.param("result: object = replace_subprocess()", id="annotated-assignment"), + pytest.param("result = (replace_subprocess(),)", id="tuple-rhs"), + pytest.param("result = [replace_subprocess()]", id="list-rhs"), + pytest.param( + "result: object = (replace_subprocess(),)", + id="annotated-tuple-rhs", + ), + pytest.param( + "result: object = [replace_subprocess()]", + id="annotated-list-rhs", + ), + pytest.param("result = (mutator.value,)", id="tuple-protocol-rhs"), + pytest.param( + "result: object = [mutator[0]]", + id="annotated-list-protocol-rhs", + ), ], ) def test_generic_call_invalidates_receiver_trust(statement: str) -> None: @@ -550,6 +565,21 @@ def test_generic_call_invalidates_receiver_trust(statement: str) -> None: pytest.param("replace_subprocess()", id="expression"), pytest.param("result = replace_subprocess()", id="assignment"), pytest.param("result: object = replace_subprocess()", id="annotated-assignment"), + pytest.param("result = (replace_subprocess(),)", id="tuple-rhs"), + pytest.param("result = [replace_subprocess()]", id="list-rhs"), + pytest.param( + "result: object = (replace_subprocess(),)", + id="annotated-tuple-rhs", + ), + pytest.param( + "result: object = [replace_subprocess()]", + id="annotated-list-rhs", + ), + pytest.param("result = (mutator.value,)", id="tuple-protocol-rhs"), + pytest.param( + "result: object = [mutator[0]]", + id="annotated-list-protocol-rhs", + ), ], ) def test_generic_call_invalidates_receiver_trust_for_called_function(statement: str) -> None: From fce761c34f55ef0188508eb1a2ebe75ef6255711 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Mon, 28 Sep 2026 13:46:26 -0700 Subject: [PATCH 10/11] fix(analyzer): invalidate unsupported eager receiver effects Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 95 +++++++++++++++++-- .../analyzers/test_tool_misuse_python_ast.py | 40 ++++++++ 2 files changed, 126 insertions(+), 9 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 1dd821627..3746042dc 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -587,14 +587,47 @@ def _call_arguments_are_passive(call: ast.Call) -> bool: ) +def _comprehension_target_is_simple(target: ast.expr) -> bool: + """Return whether binding a comprehension target cannot dispatch user code.""" + if isinstance(target, ast.Name): + return True + if isinstance(target, (ast.Tuple, ast.List)): + return all( + not isinstance(item, ast.Starred) and _comprehension_target_is_simple(item) + for item in target.elts + ) + return False + + def _value_preserves_receiver_trust(expression: ast.expr, trusted_names: set[str]) -> bool: """Return whether evaluating a value cannot replace a trusted receiver.""" if _is_passive_argument(expression): return True - return ( + if ( isinstance(expression, ast.Call) and _is_direct_subprocess_call(expression, trusted_names) and _call_arguments_are_passive(expression) + ): + return True + if isinstance(expression, (ast.Tuple, ast.List)): + return all( + not isinstance(item, ast.Starred) + and _value_preserves_receiver_trust(item, trusted_names) + for item in expression.elts + ) + if isinstance(expression, (ast.ListComp, ast.SetComp, ast.GeneratorExp)): + values = [expression.elt] + elif isinstance(expression, ast.DictComp): + values = [expression.key, expression.value] + else: + return False + return all(_value_preserves_receiver_trust(value, trusted_names) for value in values) and all( + _comprehension_target_is_simple(generator.target) + and _value_preserves_receiver_trust(generator.iter, trusted_names) + and all( + _value_preserves_receiver_trust(condition, trusted_names) for condition in generator.ifs + ) + for generator in expression.generators ) @@ -725,6 +758,9 @@ def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None trusted_names.discard(statement.name) return if isinstance(statement, ast.Assign): + if not all(isinstance(target, ast.Name) for target in statement.targets): + trusted_names.clear() + return changed = _changed_direct_names( [statement.value, *statement.targets], trusted_names, @@ -739,11 +775,30 @@ def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None } trusted_names.difference_update(changed.difference(preserved)) return + if isinstance(statement, ast.AnnAssign): + if not isinstance(statement.target, ast.Name): + trusted_names.clear() + return + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + return if isinstance(statement, ast.ClassDef): trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) trusted_names.difference_update(_class_body_changed_direct_names(statement, trusted_names)) return - trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + if isinstance(statement, ast.Expr): + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + return + if isinstance(statement, ast.Assert): + expressions = [statement.test] + if statement.msg is not None: + expressions.append(statement.msg) + if all(_value_preserves_receiver_trust(item, trusted_names) for item in expressions): + return + trusted_names.clear() + return + if isinstance(statement, (ast.Global, ast.Nonlocal, ast.Pass)): + return + trusted_names.clear() class _Analyzer: @@ -825,7 +880,10 @@ def _scan_assignment( ) result_is_finalizer_safe = _is_finalizer_safe_value(value, finalizer_safe_names) call_has_protocol_effects = False - preserves_receiver_trust = _value_preserves_receiver_trust(value, trusted_names) + preserves_receiver_trust = simple_targets and _value_preserves_receiver_trust( + value, + trusted_names, + ) if isinstance(value, ast.Call): self._record_bound_shell_call(value, trusted_names) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): @@ -979,9 +1037,8 @@ def _scan_block( ) elif isinstance(statement, ast.AnnAssign): value = statement.value - preserves_receiver_trust = value is None or _value_preserves_receiver_trust( - value, - trusted_names, + preserves_receiver_trust = isinstance(statement.target, ast.Name) and ( + value is None or _value_preserves_receiver_trust(value, trusted_names) ) if isinstance(value, ast.Call): self._record_bound_shell_call(value, trusted_names) @@ -1008,7 +1065,7 @@ def _scan_block( facts.clear() finalizer_safe_names.clear() bound_names.update(_direct_bound_names(statement)) - trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + trusted_names.clear() elif isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Call): call = statement.value self._record_bound_shell_call(call, trusted_names) @@ -1025,10 +1082,30 @@ def _scan_block( facts.clear() finalizer_safe_names.clear() trusted_names.clear() - elif isinstance(statement, ast.Pass) or ( + elif isinstance(statement, (ast.Global, ast.Nonlocal, ast.Pass)) or ( isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant) ): continue + elif isinstance(statement, ast.Expr): + facts.clear() + finalizer_safe_names.clear() + bound_names.update(_direct_bound_names(statement)) + if _value_preserves_receiver_trust(statement.value, trusted_names): + trusted_names.difference_update( + _changed_direct_names([statement], trusted_names) + ) + else: + trusted_names.clear() + elif isinstance(statement, ast.Assert): + facts.clear() + finalizer_safe_names.clear() + expressions = [statement.test] + if statement.msg is not None: + expressions.append(statement.msg) + if not all( + _value_preserves_receiver_trust(item, trusted_names) for item in expressions + ): + trusted_names.clear() elif isinstance(statement, ast.ClassDef): class_trusted_names = set(trusted_names) passive_class_header = _class_header_is_passive(statement) @@ -1068,7 +1145,7 @@ def _scan_block( facts.clear() finalizer_safe_names.clear() bound_names.update(_direct_bound_names(statement)) - trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + trusted_names.clear() def run(self, tree: ast.Module) -> list[AnalyzerFinding]: self._scan_block(tree.body) diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index a94a01b60..9e19ec735 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -596,6 +596,46 @@ def test_generic_call_invalidates_receiver_trust_for_called_function(statement: assert not findings +@pytest.mark.parametrize( + "statement", + [ + pytest.param("assert (replace_subprocess(),)", id="assert"), + pytest.param("(replace_subprocess(),)", id="tuple-expression"), + pytest.param("[replace_subprocess()]", id="list-expression"), + ], +) +def test_unsupported_eager_statement_invalidates_receiver_trust(statement: str) -> None: + assert not _tm1( + "import subprocess\n" + "from helpers import replace_subprocess\n" + f"{statement}\n" + "enabled = True\n" + "subprocess.run(command, shell=enabled)\n" + ) + + +@pytest.mark.parametrize( + "statement", + [ + pytest.param("assert (replace_subprocess(),)", id="assert"), + pytest.param("(replace_subprocess(),)", id="tuple-expression"), + pytest.param("[replace_subprocess()]", id="list-expression"), + ], +) +def test_unsupported_eager_statement_invalidates_receiver_for_called_function( + statement: str, +) -> None: + assert not _tm1( + "import subprocess\n" + "from helpers import replace_subprocess\n" + "def execute():\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + f"{statement}\n" + "execute()\n" + ) + + def test_generic_call_invalidates_receiver_trust_for_nested_closure() -> None: assert not _tm1( "import subprocess\n" From 08a1caa49d7cdbff158fb0e8c4aa2e23082583b8 Mon Sep 17 00:00:00 2001 From: Christopher Kevin Date: Mon, 28 Sep 2026 14:25:36 -0700 Subject: [PATCH 11/11] fix(analyzer): track eager receiver state Signed-off-by: Christopher Kevin --- .../static_python_shell_truthiness.py | 567 +++++++++++++++--- .../nodes/analyzers/test_shared_python_ast.py | 6 +- .../analyzers/test_tool_misuse_python_ast.py | 37 +- 3 files changed, 501 insertions(+), 109 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py index 3746042dc..bb67a0f8c 100644 --- a/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py +++ b/src/skillspector/nodes/analyzers/static_python_shell_truthiness.py @@ -404,6 +404,156 @@ def nested_classes(node: ast.AST) -> list[ast.ClassDef]: return affected +def _nodes_contain_eager_call(nodes: list[ast.AST]) -> bool: + """Return whether eager evaluation reaches a call outside deferred bodies.""" + pending = list(nodes) + while pending: + current = pending.pop() + if isinstance(current, ast.Call): + return True + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef)): + if not _function_header_is_passive(current): + return True + continue + if isinstance(current, ast.ClassDef): + if ( + not _class_header_is_passive(current) + or _class_body_may_release_outer_value(current) + or _nodes_contain_eager_call(list(current.body)) + ): + return True + continue + if isinstance(current, ast.Lambda): + defaults = ( + *current.args.defaults, + *(item for item in current.args.kw_defaults if item is not None), + ) + if any(not _is_passive_argument(default) for default in defaults): + return True + continue + pending.extend(ast.iter_child_nodes(current)) + return False + + +def _class_body_may_release_outer_value(statement: ast.ClassDef) -> bool: + """Return whether class execution stores to a declared outer name.""" + declarations = _DirectBindingCollector(set()) + for child in statement.body: + declarations.visit(child) + outer_names = declarations.nonlocal_names + if outer_names: + bindings = _DirectBindingCollector(outer_names) + for child in statement.body: + bindings.visit(child) + if bindings.bound.intersection(outer_names): + return True + + pending: list[ast.AST] = list(statement.body) + while pending: + current = pending.pop() + if isinstance(current, ast.ClassDef): + if _class_body_may_release_outer_value(current): + return True + continue + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + continue + pending.extend(ast.iter_child_nodes(current)) + return False + + +def _class_body_has_eager_effects(statement: ast.ClassDef) -> bool: + """Return whether class execution can run user code or unsafe finalizers.""" + declarations = _DirectBindingCollector(set()) + for child in statement.body: + declarations.visit(child) + outer_names = declarations.nonlocal_names + bound_names: set[str] = set() + finalizer_safe_names: set[str] = set() + + for child in statement.body: + if isinstance(child, (ast.Global, ast.Nonlocal, ast.Pass)) or ( + isinstance(child, ast.Expr) and isinstance(child.value, ast.Constant) + ): + continue + if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)): + releases_unsafe_value = ( + child.name in bound_names and child.name not in finalizer_safe_names + ) + if ( + child.name in outer_names + or not _function_header_is_passive(child) + or releases_unsafe_value + ): + return True + bound_names.add(child.name) + finalizer_safe_names.add(child.name) + continue + if isinstance(child, ast.ClassDef): + releases_unsafe_value = ( + child.name in bound_names and child.name not in finalizer_safe_names + ) + if ( + child.name in outer_names + or not _class_header_is_passive(child) + or releases_unsafe_value + or _class_body_has_eager_effects(child) + ): + return True + bound_names.add(child.name) + finalizer_safe_names.add(child.name) + continue + if isinstance(child, ast.Assign): + if not all(isinstance(target, ast.Name) for target in child.targets): + return True + target_names = {target.id for target in child.targets if isinstance(target, ast.Name)} + if target_names.intersection(outer_names): + return True + releases_unsafe_value = any( + name in bound_names + and name not in finalizer_safe_names + and not (isinstance(child.value, ast.Name) and child.value.id == name) + for name in target_names + ) + if releases_unsafe_value or not _is_finalizer_safe_value( + child.value, + finalizer_safe_names, + ): + return True + bound_names.update(target_names) + finalizer_safe_names.update(target_names) + continue + if isinstance(child, ast.AnnAssign): + if not isinstance(child.target, ast.Name) or not _annotation_is_passive( + child.annotation + ): + return True + if child.value is None: + continue + target_name = child.target.id + if target_name in outer_names: + return True + releases_unsafe_value = ( + target_name in bound_names + and target_name not in finalizer_safe_names + and not (isinstance(child.value, ast.Name) and child.value.id == target_name) + ) + if releases_unsafe_value or not _is_finalizer_safe_value( + child.value, + finalizer_safe_names, + ): + return True + bound_names.add(target_name) + finalizer_safe_names.add(target_name) + continue + if isinstance(child, ast.Assert) and _is_finalizer_safe_value( + child.test, + finalizer_safe_names, + ): + continue + return True + return False + + def _class_deferred_receiver_trust( statement: ast.ClassDef, trusted_names: set[str], @@ -425,22 +575,7 @@ def _class_deferred_receiver_trust( if owner is not None: trusted_at_call_by_definition.setdefault(owner, set()).update(deferred) - pending = [child] - contains_call = False - while pending: - current = pending.pop() - if isinstance(current, ast.Call): - contains_call = True - break - if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef)): - pending.extend(_DirectBindingCollector._function_header_nodes(current)) - continue - if isinstance(current, ast.Lambda): - pending.extend(current.args.defaults) - pending.extend(item for item in current.args.kw_defaults if item is not None) - continue - pending.extend(ast.iter_child_nodes(current)) - if contains_call: + if _nodes_contain_eager_call([child]): # Class-body expressions run before any method can be called and may # mutate the surrounding module/function receiver binding. deferred.clear() @@ -587,32 +722,31 @@ def _call_arguments_are_passive(call: ast.Call) -> bool: ) -def _comprehension_target_is_simple(target: ast.expr) -> bool: - """Return whether binding a comprehension target cannot dispatch user code.""" - if isinstance(target, ast.Name): - return True - if isinstance(target, (ast.Tuple, ast.List)): - return all( - not isinstance(item, ast.Starred) and _comprehension_target_is_simple(item) - for item in target.elts - ) - return False - - -def _value_preserves_receiver_trust(expression: ast.expr, trusted_names: set[str]) -> bool: - """Return whether evaluating a value cannot replace a trusted receiver.""" +def _value_preserves_receiver_trust( + expression: ast.expr, + trusted_names: set[str], + protocol_safe_names: set[str] | None = None, +) -> bool: + """Return whether eager evaluation cannot replace a direct receiver.""" if _is_passive_argument(expression): return True - if ( - isinstance(expression, ast.Call) - and _is_direct_subprocess_call(expression, trusted_names) - and _call_arguments_are_passive(expression) - ): - return True + if isinstance(expression, ast.Call): + return ( + _is_direct_subprocess_call(expression, trusted_names) + and _call_arguments_are_passive(expression) + and ( + protocol_safe_names is None + or _call_arguments_are_protocol_safe(expression, protocol_safe_names) + ) + ) if isinstance(expression, (ast.Tuple, ast.List)): return all( not isinstance(item, ast.Starred) - and _value_preserves_receiver_trust(item, trusted_names) + and _value_preserves_receiver_trust( + item, + trusted_names, + protocol_safe_names, + ) for item in expression.elts ) if isinstance(expression, (ast.ListComp, ast.SetComp, ast.GeneratorExp)): @@ -621,13 +755,36 @@ def _value_preserves_receiver_trust(expression: ast.expr, trusted_names: set[str values = [expression.key, expression.value] else: return False - return all(_value_preserves_receiver_trust(value, trusted_names) for value in values) and all( - _comprehension_target_is_simple(generator.target) - and _value_preserves_receiver_trust(generator.iter, trusted_names) - and all( - _value_preserves_receiver_trust(condition, trusted_names) for condition in generator.ifs + for generator in expression.generators: + if generator.is_async or not _is_simple_comprehension_target(generator.target): + return False + if protocol_safe_names is not None and ( + not _is_finalizer_safe_value(generator.iter, protocol_safe_names) + or any( + not _is_finalizer_safe_value(condition, protocol_safe_names) + for condition in generator.ifs + ) + ): + return False + values.extend((generator.iter, *generator.ifs)) + # Comprehensions have historically retained ownership for their direct + # subprocess calls. Preserve that behavior while still rejecting generic + # nested calls and receiver stores. + return all( + _value_preserves_receiver_trust( + value, + trusted_names, + protocol_safe_names, ) - for generator in expression.generators + for value in values + ) + + +def _is_simple_comprehension_target(target: ast.expr) -> bool: + if isinstance(target, ast.Name): + return True + return isinstance(target, (ast.Tuple, ast.List)) and all( + _is_simple_comprehension_target(item) for item in target.elts ) @@ -698,13 +855,12 @@ def _function_header_is_passive( def _class_header_is_passive(statement: ast.ClassDef) -> bool: """Return whether evaluating a class header cannot rebind a receiver.""" - expressions = [ - *statement.decorator_list, - *statement.bases, - *(keyword.value for keyword in statement.keywords), - *getattr(statement, "type_params", []), - ] - return all(_is_passive_argument(expression) for expression in expressions) + return not ( + statement.decorator_list + or statement.bases + or statement.keywords + or getattr(statement, "type_params", []) + ) def _is_immediate_function(statement: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: @@ -738,28 +894,66 @@ def _passive_direct_call(statement: ast.stmt) -> ast.Call | None: return None -def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None: - """Apply one statement's receiver-trust effects.""" - value = ( - statement.value if isinstance(statement, (ast.Expr, ast.Assign, ast.AnnAssign)) else None - ) - if value is not None and not _value_preserves_receiver_trust(value, trusted_names): - trusted_names.clear() - return - if isinstance(statement, ast.AnnAssign) and not _annotation_is_passive(statement.annotation): - trusted_names.clear() - return +def _advance_trusted_names( + statement: ast.stmt, + trusted_names: set[str], + bound_names: set[str], + finalizer_safe_names: set[str], + unknown_unsafe_bindings: list[bool], +) -> None: + """Apply one eager statement's receiver-trust and value-release effects.""" if isinstance(statement, (ast.Import, ast.ImportFrom)): + imported_names = _direct_bound_names(statement) + releases_unsafe_value = any( + name in bound_names and name not in finalizer_safe_names for name in imported_names + ) + bound_names.update(imported_names) + finalizer_safe_names.difference_update(imported_names) _update_trusted_names_from_import(statement, trusted_names) + imports_unknown_names = isinstance(statement, ast.ImportFrom) and any( + imported.name == "*" for imported in statement.names + ) + if releases_unsafe_value or imports_unknown_names: + unknown_unsafe_bindings[0] = True + if releases_unsafe_value or unknown_unsafe_bindings[0]: + trusted_names.clear() return if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): - if not _function_header_is_passive(statement): + releases_unsafe_value = ( + statement.name in bound_names and statement.name not in finalizer_safe_names + ) + if not _function_header_is_passive(statement) or releases_unsafe_value: trusted_names.clear() + unknown_unsafe_bindings[0] = True + bound_names.add(statement.name) + finalizer_safe_names.discard(statement.name) trusted_names.discard(statement.name) return if isinstance(statement, ast.Assign): - if not all(isinstance(target, ast.Name) for target in statement.targets): + targets = list(statement.targets) + simple_targets = all(isinstance(target, ast.Name) for target in targets) + result_is_finalizer_safe = _is_finalizer_safe_value( + statement.value, + finalizer_safe_names, + ) + preserves_receiver_trust = simple_targets and _value_preserves_receiver_trust( + statement.value, + trusted_names, + finalizer_safe_names, + ) + releases_unsafe_value = simple_targets and any( + target.id in bound_names + and target.id not in finalizer_safe_names + and not (isinstance(statement.value, ast.Name) and statement.value.id == target.id) + for target in targets + if isinstance(target, ast.Name) + ) + if not preserves_receiver_trust or releases_unsafe_value: trusted_names.clear() + finalizer_safe_names.clear() + unknown_unsafe_bindings[0] = True + if not simple_targets: + bound_names.update(_direct_bound_names(statement)) return changed = _changed_direct_names( [statement.value, *statement.targets], @@ -774,31 +968,117 @@ def _advance_trusted_names(statement: ast.stmt, trusted_names: set[str]) -> None and target.id in trusted_names } trusted_names.difference_update(changed.difference(preserved)) + for target in targets: + assert isinstance(target, ast.Name) + bound_names.add(target.id) + if preserves_receiver_trust and not releases_unsafe_value and result_is_finalizer_safe: + finalizer_safe_names.add(target.id) + else: + finalizer_safe_names.discard(target.id) return if isinstance(statement, ast.AnnAssign): - if not isinstance(statement.target, ast.Name): + value = statement.value + target = statement.target + simple_target = isinstance(target, ast.Name) + preserves_receiver_trust = ( + simple_target + and _annotation_is_passive(statement.annotation) + and ( + value is None + or _value_preserves_receiver_trust( + value, + trusted_names, + finalizer_safe_names, + ) + ) + ) + releases_unsafe_value = ( + value is not None + and simple_target + and target.id in bound_names + and target.id not in finalizer_safe_names + and not (isinstance(value, ast.Name) and value.id == target.id) + ) + result_is_finalizer_safe = value is not None and _is_finalizer_safe_value( + value, + finalizer_safe_names, + ) + if not preserves_receiver_trust or releases_unsafe_value: trusted_names.clear() - return + finalizer_safe_names.clear() + unknown_unsafe_bindings[0] = True + if value is not None: + bound_names.update(_direct_bound_names(statement)) + if simple_target: + if ( + preserves_receiver_trust + and not releases_unsafe_value + and result_is_finalizer_safe + ): + finalizer_safe_names.add(target.id) + else: + finalizer_safe_names.discard(target.id) trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) return if isinstance(statement, ast.ClassDef): + releases_unsafe_value = ( + statement.name in bound_names and statement.name not in finalizer_safe_names + ) + unsafe_class_execution = ( + not _class_header_is_passive(statement) + or _class_body_has_eager_effects(statement) + or releases_unsafe_value + ) + if unsafe_class_execution: + trusted_names.clear() + unknown_unsafe_bindings[0] = True + finalizer_safe_names.clear() + else: + finalizer_safe_names.discard(statement.name) trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) trusted_names.difference_update(_class_body_changed_direct_names(statement, trusted_names)) + bound_names.update(_direct_bound_names(statement)) return if isinstance(statement, ast.Expr): - trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + value = statement.value + preserves_receiver_trust = _value_preserves_receiver_trust( + value, + trusted_names, + finalizer_safe_names, + ) + if not preserves_receiver_trust: + trusted_names.clear() + unknown_unsafe_bindings[0] = True + finalizer_safe_names.clear() + else: + trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) + bound_names.update(_direct_bound_names(statement)) return if isinstance(statement, ast.Assert): expressions = [statement.test] if statement.msg is not None: expressions.append(statement.msg) - if all(_value_preserves_receiver_trust(item, trusted_names) for item in expressions): - return - trusted_names.clear() + if not ( + _is_finalizer_safe_value(statement.test, finalizer_safe_names) + and all( + _value_preserves_receiver_trust( + item, + trusted_names, + finalizer_safe_names, + ) + for item in expressions + ) + ): + trusted_names.clear() + unknown_unsafe_bindings[0] = True + finalizer_safe_names.clear() return if isinstance(statement, (ast.Global, ast.Nonlocal, ast.Pass)): return + bound_names.update(_direct_bound_names(statement)) + finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True class _Analyzer: @@ -869,6 +1149,7 @@ def _scan_assignment( trusted_names: set[str], bound_names: set[str], finalizer_safe_names: set[str], + unknown_unsafe_bindings: list[bool], ) -> None: simple_targets = all(isinstance(target, ast.Name) for target in targets) releases_unsafe_value = simple_targets and any( @@ -883,6 +1164,7 @@ def _scan_assignment( preserves_receiver_trust = simple_targets and _value_preserves_receiver_trust( value, trusted_names, + finalizer_safe_names, ) if isinstance(value, ast.Call): self._record_bound_shell_call(value, trusted_names) @@ -908,6 +1190,7 @@ def _scan_assignment( bound_names.add(target.id) if not preserves_receiver_trust: trusted_names.clear() + unknown_unsafe_bindings[0] = True else: trusted_names.difference_update( _changed_direct_names([value, *targets], trusted_names) @@ -917,10 +1200,12 @@ def _scan_assignment( facts.clear() finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True if call_has_protocol_effects: facts.clear() finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True for target in targets: assert isinstance(target, ast.Name) bound_names.add(target.id) @@ -951,17 +1236,30 @@ def _scan_block( facts: dict[str, bool] = {} bound_names = set(initial_bound_names or ()) finalizer_safe_names: set[str] = set() + unknown_unsafe_bindings = [False] last_invalidation_by_name: dict[str, int] = {} receiver_trust = set(trusted_names) + receiver_bound_names = set(initial_bound_names or ()) + receiver_finalizer_safe_names: set[str] = set() + receiver_unknown_unsafe_bindings = [False] for candidate_index, candidate in enumerate(statements): before = set(receiver_trust) - _advance_trusted_names(candidate, receiver_trust) + _advance_trusted_names( + candidate, + receiver_trust, + receiver_bound_names, + receiver_finalizer_safe_names, + receiver_unknown_unsafe_bindings, + ) for name in before.difference(receiver_trust): last_invalidation_by_name[name] = candidate_index trusted_at_call_by_definition: dict[int, set[str]] = {} receiver_trust = set(trusted_names) + receiver_bound_names = set(initial_bound_names or ()) + receiver_finalizer_safe_names = set() + receiver_unknown_unsafe_bindings = [False] active_functions: dict[str, int] = {} for candidate_index, candidate in enumerate(statements): call = _passive_direct_call(candidate) @@ -980,7 +1278,13 @@ def _scan_block( and _is_immediate_function(candidate) ): active_functions[candidate.name] = candidate_index - _advance_trusted_names(candidate, receiver_trust) + _advance_trusted_names( + candidate, + receiver_trust, + receiver_bound_names, + receiver_finalizer_safe_names, + receiver_unknown_unsafe_bindings, + ) for index, statement in enumerate(statements): if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): @@ -1018,14 +1322,27 @@ def _scan_block( facts.clear() finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True bound_names.add(statement.name) finalizer_safe_names.discard(statement.name) trusted_names.discard(statement.name) elif isinstance(statement, (ast.Import, ast.ImportFrom)): + imported_names = _direct_bound_names(statement) + releases_unsafe_value = any( + name in bound_names and name not in finalizer_safe_names + for name in imported_names + ) facts.clear() - finalizer_safe_names.clear() - bound_names.update(_direct_bound_names(statement)) + finalizer_safe_names.difference_update(imported_names) + bound_names.update(imported_names) _update_trusted_names_from_import(statement, trusted_names) + imports_unknown_names = isinstance(statement, ast.ImportFrom) and any( + imported.name == "*" for imported in statement.names + ) + if releases_unsafe_value or imports_unknown_names: + unknown_unsafe_bindings[0] = True + if releases_unsafe_value or unknown_unsafe_bindings[0]: + trusted_names.clear() elif isinstance(statement, ast.Assign): self._scan_assignment( list(statement.targets), @@ -1034,29 +1351,57 @@ def _scan_block( trusted_names, bound_names, finalizer_safe_names, + unknown_unsafe_bindings, ) elif isinstance(statement, ast.AnnAssign): value = statement.value - preserves_receiver_trust = isinstance(statement.target, ast.Name) and ( - value is None or _value_preserves_receiver_trust(value, trusted_names) + target = statement.target + simple_target = isinstance(target, ast.Name) + releases_unsafe_value = ( + value is not None + and simple_target + and target.id in bound_names + and target.id not in finalizer_safe_names + and not (isinstance(value, ast.Name) and value.id == target.id) + ) + result_is_finalizer_safe = value is not None and _is_finalizer_safe_value( + value, + finalizer_safe_names, + ) + preserves_receiver_trust = ( + simple_target + and _annotation_is_passive(statement.annotation) + and ( + value is None + or _value_preserves_receiver_trust( + value, + trusted_names, + finalizer_safe_names, + ) + ) ) if isinstance(value, ast.Call): self._record_bound_shell_call(value, trusted_names) if isinstance(value, ast.Call) and _is_direct_subprocess_call(value, trusted_names): if _shell_argument_is_captured_before_effects(value): self._inspect_call(value, facts) - preserves_receiver_trust = ( - preserves_receiver_trust - and _call_arguments_are_protocol_safe(value, finalizer_safe_names) - ) - if not _annotation_is_passive(statement.annotation): - preserves_receiver_trust = False facts.clear() - finalizer_safe_names.clear() + if not preserves_receiver_trust or releases_unsafe_value: + finalizer_safe_names.clear() if value is not None: bound_names.update(_direct_bound_names(statement)) - if not preserves_receiver_trust: + if ( + simple_target + and preserves_receiver_trust + and not releases_unsafe_value + and result_is_finalizer_safe + ): + finalizer_safe_names.add(target.id) + elif simple_target: + finalizer_safe_names.discard(target.id) + if not preserves_receiver_trust or releases_unsafe_value: trusted_names.clear() + unknown_unsafe_bindings[0] = True else: trusted_names.difference_update( _changed_direct_names([statement], trusted_names) @@ -1066,6 +1411,7 @@ def _scan_block( finalizer_safe_names.clear() bound_names.update(_direct_bound_names(statement)) trusted_names.clear() + unknown_unsafe_bindings[0] = True elif isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Call): call = statement.value self._record_bound_shell_call(call, trusted_names) @@ -1078,35 +1424,53 @@ def _scan_block( facts.clear() finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True else: facts.clear() finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True elif isinstance(statement, (ast.Global, ast.Nonlocal, ast.Pass)) or ( isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant) ): continue elif isinstance(statement, ast.Expr): - facts.clear() - finalizer_safe_names.clear() + preserves_receiver_trust = _value_preserves_receiver_trust( + statement.value, + trusted_names, + finalizer_safe_names, + ) bound_names.update(_direct_bound_names(statement)) - if _value_preserves_receiver_trust(statement.value, trusted_names): + if preserves_receiver_trust: trusted_names.difference_update( _changed_direct_names([statement], trusted_names) ) else: + facts.clear() + finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True elif isinstance(statement, ast.Assert): - facts.clear() - finalizer_safe_names.clear() expressions = [statement.test] if statement.msg is not None: expressions.append(statement.msg) - if not all( - _value_preserves_receiver_trust(item, trusted_names) for item in expressions - ): + preserves_receiver_trust = all( + _value_preserves_receiver_trust( + item, + trusted_names, + finalizer_safe_names, + ) + for item in expressions + ) and _is_finalizer_safe_value(statement.test, finalizer_safe_names) + facts.clear() + if not preserves_receiver_trust: + finalizer_safe_names.clear() trusted_names.clear() + unknown_unsafe_bindings[0] = True elif isinstance(statement, ast.ClassDef): + releases_unsafe_value = ( + statement.name in bound_names and statement.name not in finalizer_safe_names + ) class_trusted_names = set(trusted_names) passive_class_header = _class_header_is_passive(statement) if not passive_class_header: @@ -1135,17 +1499,30 @@ def _scan_block( nested_function_trusted_at_call=method_trusted_at_call, ) facts.clear() - finalizer_safe_names.clear() bound_names.update(_direct_bound_names(statement)) - trusted_names.difference_update(_changed_direct_names([statement], trusted_names)) - trusted_names.difference_update( - _class_body_changed_direct_names(statement, trusted_names) + unsafe_class_execution = ( + not passive_class_header + or releases_unsafe_value + or _class_body_has_eager_effects(statement) ) + if unsafe_class_execution: + finalizer_safe_names.clear() + trusted_names.clear() + unknown_unsafe_bindings[0] = True + else: + finalizer_safe_names.discard(statement.name) + trusted_names.difference_update( + _changed_direct_names([statement], trusted_names) + ) + trusted_names.difference_update( + _class_body_changed_direct_names(statement, trusted_names) + ) else: facts.clear() finalizer_safe_names.clear() bound_names.update(_direct_bound_names(statement)) trusted_names.clear() + unknown_unsafe_bindings[0] = True def run(self, tree: ast.Module) -> list[AnalyzerFinding]: self._scan_block(tree.body) diff --git a/tests/nodes/analyzers/test_shared_python_ast.py b/tests/nodes/analyzers/test_shared_python_ast.py index 005e437b2..68d71ce1d 100644 --- a/tests/nodes/analyzers/test_shared_python_ast.py +++ b/tests/nodes/analyzers/test_shared_python_ast.py @@ -60,12 +60,12 @@ def code(tail: str) -> str: def test_preparsed_python_is_reused_by_all_ast_analyzers(tmp_path, monkeypatch) -> None: """One scan parses each eligible Python file once before analyzer fan-out.""" (tmp_path / "script.py").write_text( - "import os\n" - "payload = input()\n" - "environment = os.environ.copy()\n" "import subprocess\n" "enabled = True\n" "subprocess.run(output, shell=enabled)\n" + "import os\n" + "payload = input()\n" + "environment = os.environ.copy()\n" "exec(payload)\n", encoding="utf-8", ) diff --git a/tests/nodes/analyzers/test_tool_misuse_python_ast.py b/tests/nodes/analyzers/test_tool_misuse_python_ast.py index 9e19ec735..e88bfeb47 100644 --- a/tests/nodes/analyzers/test_tool_misuse_python_ast.py +++ b/tests/nodes/analyzers/test_tool_misuse_python_ast.py @@ -131,8 +131,8 @@ def test_explicit_import_reestablishes_direct_receivers() -> None: assert ( len( _tm1( - "subprocess = Proxy()\n" - "Popen = Proxy()\n" + "subprocess = None\n" + "Popen = None\n" "import subprocess\n" "from subprocess import Popen\n" "enabled = True\n" @@ -157,7 +157,7 @@ def test_function_local_binding_and_outer_fact_are_independent() -> None: "def execute(command):\n" " enabled = 'True'\n" " subprocess.run(command, shell=enabled)\n" - "subprocess.run(command, shell=outer)\n" + "subprocess.run('/usr/bin/true', shell=outer)\n" ) assert [finding.start_line for finding in findings] == [4, 5] @@ -729,19 +729,14 @@ def test_external_name_store_treats_prior_binding_as_finalizer_capable() -> None def test_protocol_consuming_direct_call_invalidates_later_truth_fact() -> None: findings = _tm1( - "class MutatingArgs:\n" - " def __iter__(self):\n" - " global enabled\n" - " enabled = False\n" - " return iter(('/usr/bin/true',))\n" - "mutator = MutatingArgs()\n" + "from helpers import mutator\n" "import subprocess\n" "enabled = True\n" "subprocess.run(mutator, shell=enabled)\n" "subprocess.run('/usr/bin/true', shell=enabled)\n" ) - assert [finding.start_line for finding in findings] == [9] + assert [finding.start_line for finding in findings] == [4] def test_annotated_assignment_is_outside_side_effect_free_contract() -> None: @@ -817,7 +812,7 @@ def test_normalized_prefix_does_not_duplicate_true_direct_fallback() -> None: ], ) def test_true_direct_calls_on_one_line_keep_distinct_locations(call_line: str) -> None: - findings = _tm1(f"true = True\n{call_line}\n") + findings = _tm1(f"items = [1]\ncommand = '/bin/true'\ntrue = True\n{call_line}\n") expected_columns = [ index for index in range(len(call_line)) if call_line.startswith("subprocess.run", index) ] @@ -826,6 +821,26 @@ def test_true_direct_calls_on_one_line_keep_distinct_locations(call_line: str) - assert sorted(finding.start_column for finding in findings) == expected_columns +def test_unknown_comprehension_protocol_invalidates_receiver_trust() -> None: + assert not _tm1( + "import subprocess\n" + "[subprocess.run('/bin/true', shell=False) for item in items]\n" + "enabled = True\n" + "subprocess.run(command, shell=enabled)\n" + ) + + +def test_unknown_comprehension_protocol_invalidates_called_function_trust() -> None: + assert not _tm1( + "import subprocess\n" + "def execute():\n" + " enabled = True\n" + " subprocess.run(command, shell=enabled)\n" + "[subprocess.run('/bin/true', shell=False) for item in items]\n" + "execute()\n" + ) + + def test_true_direct_ownership_invalidation_is_per_call() -> None: findings = _tm1( "true = True\n"