fix(taint): track sensitive data through reflective urllib sinks

[agentsope]

Addresses part of #586.

Summary

• Resolve bounded, statically-computable module and attribute strings used by importlib.import_module(...) and getattr(...).
• Map reflective handles such as opener = getattr(module, "url" + "open") back to existing network sinks at the exact call site.
• Feed those resolved calls into the existing taint engine, so credential or file-read data reaching urllib.request.urlopen produces the existing TT3/TT4 findings.
• Respect lexical function scope, source order, reassignment, argument shadowing, and class namespace boundaries.

This intentionally does not classify arbitrary runtime-only attribute names, ordinary public payloads, all dynamic URLs, or socket/chunk-stream behavior. It addresses the statically resolvable reflective urllib portion of #586.

Validation

• pytest -q -m "not integration and not provider" tests/ --tb=short: 5,983 passed, 14 skipped, 40 deselected, 4 xfailed
• pytest -q -m integration tests/ --ignore=tests/integration/test_agent_cli_live.py --tb=short: 30 passed
• Focused analyzer/report tests: 194 passed
• ruff check src/ tests/
• ruff format --check src/ tests/
• mypy src/skillspector/nodes/analyzers/behavioral_taint_tracking.py

The excluded live CLI integration file invokes locally installed external agents; the local claude command exited with code 1 and no stderr, which is unrelated to this change.

AI assistance was used to investigate, implement, and test this change; the resulting behavior and diff were manually reviewed and validated.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved scope, lookup, rebinding, class-body, lambda, and taint-isolation issues can cause missed findings or false positives.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Respect scope shadowing for importlib aliases · New

What changed in this PR

Extends behavioral taint tracking to detect statically resolvable reflective urllib sinks and integrate them with TT3/TT4 reporting.

Changes:

• Resolves bounded module and attribute strings.
• Tracks reflective sink handles and scope behavior.
• Adds analyzer and end-to-end report coverage.

File	Summary
tests/​nodes/​analyzers/​test_behavioral_taint_tracking.py	Tests reflective sink flows and scope behavior.
tests/​integration/​test_graph.py	Verifies reflective findings in generated reports.
src/​skillspector/​nodes/​analyzers/​behavioral_taint_tracking.py	Implements reflective sink resolution and taint integration.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved moderate findings affect reflective sink scope, shadowing, alias invalidation, and direct-call handling.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Respect scope shadowing for importlib aliases

Previously missed (4)

In code that hasn't changed since last review

Unqualified getattr calls ignore local shadowing

src/​skillspector/​nodes/​analyzers/​behavioral_taint_tracking.py:416

The resolver treats every unqualified call spelled getattr as the builtin, but it never checks the active scope for a parameter or local binding with that name. For example, def send(getattr): ... opener = getattr(module, "urlopen") ... opener(secret) is recorded as a urllib sink even though the call is user-defined; require the builtin/unshadowed getattr before creating the alias.

Alias invalidation misses non-assignment rebinding constructs

src/​skillspector/​nodes/​analyzers/​behavioral_taint_tracking.py:434

Alias invalidation is implemented only for Assign and AnnAssign. Rebinding through constructs such as for opener in ..., with ... as opener, an assignment expression, an exception target, or del opener is not processed, so the previous reflective sink identity can survive to a later opener(secret) after the runtime binding has changed or been removed. Handle these binding/deletion nodes (or conservatively clear the alias) in source order.

Lambda parameters do not shadow reflective aliases

src/​skillspector/​nodes/​analyzers/​behavioral_taint_tracking.py:493

Only named function definitions establish a new resolver scope; lambdas are traversed generically. A lambda parameter can therefore fail to shadow an outer reflective handle, e.g. lambda opener: opener(secret) is resolved against the module's urlopen alias and can produce a false TT3. Add lambda argument/body scope handling consistent with the named-function path.

Nested methods are omitted from reflective sink analysis

src/​skillspector/​nodes/​analyzers/​behavioral_taint_tracking.py:499

This handler never traverses node.body, so every class method is omitted from call_sinks, including a method that creates and invokes its own reflective urlopen handle. A valid class Client: def send(...): module = ...; opener = getattr(...); opener(secret) therefore remains unreported; the added regression only proves that a class-body handle does not leak into a method. Traverse nested function/async-function definitions with fresh function scopes while still skipping class assignments.

[yashrajp22]

Requesting changes for six additional issues reproduced on 1d8a648. The latest commit fixes importlib parameter shadowing and function-local imports. I also reproduced the class-method, rebinding/lambda, and shadowed-getattr issues already described in the existing review; those are referenced rather than duplicated inline.

Compared d162d9b to 1d8a648 using fresh wheels and isolated offline source/wheel runs. The selected regression tests pass (87 base, 98 head), while 15 of 33 focused behavior cases fail identically in both head modes. All eight concurrency checks and seven string-boundary checks pass. Nine CLI fixtures and 12 complete skill directories ran in all four lanes, with source/wheel parity. Nine corpus reports per lane remain partial because of missing references or obfuscated text; this does not establish global rule accuracy or live-provider coverage.

This review covers the reflective urllib scope of this PR, not the remaining dynamic-URL/socket/chunk-stream requirements of #586.

[yashrajp22]

Please keep this traversal safe for deep, valid Python expressions. A roughly 1.2 KB file containing a 600-term 1+1+... assignment followed by ordinary urllib.request.urlopen(os.environ.get("API_KEY")) hits RecursionError in this recursive visitor. The baseline detects TT3; this revision loses that finding and the full CLI reports the taint analyzer as failed. An iterative or explicitly bounded traversal needs to preserve normal direct-sink analysis too.

[agentsope]

Addressed in a797b06. Expression traversal is now iterative, including the local-binding collector used for function bodies, so deeply nested valid expressions no longer abort reflective resolution. Added regressions with a 600-term expression at module and function scope; the following ordinary urllib sink still produces TT3.

[yashrajp22]

Please preserve node.level when classifying imports. from .importlib import import_module as load imports a local package module, but this code records it as the standard-library importlib.import_module. Consequently module = load("urllib.request"); opener = getattr(module, "urlopen"); opener(os.environ.get("API_KEY")) is reported as TT3 even though the local loader has no established relationship to urllib. Relative imports should not acquire a known standard-library identity from their spelling.

[agentsope]

Addressed in a797b06. ImportFrom bindings are only assigned a standard-library identity when level == 0. Relative imports still shadow the local name, but cannot resolve as importlib.import_module. Added the reported relative-import false-positive regression.

[yashrajp22]

An annotation without a value does not reassign the variable. Here, opener = getattr(module, "urlopen"); opener: object; opener(os.environ.get("API_KEY")) loses TT3 because _bind clears the existing handle. Conversely, annotating a harmless lambda with opener: getattr(module, "urlopen") creates a false TT3. Please retain the existing value binding when node.value is None rather than using the annotation as an assigned expression.

[agentsope]

Addressed in a797b06. AnnAssign now visits the annotation for nested expressions but leaves the existing binding unchanged when value is None; annotations are never treated as assigned values. Added positive coverage for retaining an existing handle and negative coverage for an annotation-shaped getattr expression.

[yashrajp22]

Resolving a function body immediately freezes its free globals at definition time. If send() calls opener(secret), defining send before the module-level opener = getattr(module, "urlopen") and then calling send() misses TT3. Reversing the order and replacing opener with a harmless lambda before send() instead produces a false TT3. Please account for the bindings visible when the function can run, rather than treating def as execution of its body.

[agentsope]

Addressed in a797b06. Named function bodies are deferred until the enclosing block binding state is complete, then analyzed against an isolated snapshot. Added regressions for a global handle bound after the definition and for a previously bound handle replaced before the function can run.

[yashrajp22]

Both sides of a conditional currently mutate the same binding state in AST order. With if input(): opener = getattr(module, "urlopen") followed by else: opener = lambda value: value, the later opener(os.environ.get("API_KEY")) produces no TT3 because visiting the else branch erases the possible network sink. Please preserve feasible bindings across control-flow joins; visiting the last branch should not decide which runtime path occurred.

[agentsope]

Addressed in a797b06. If branches now execute from cloned input states and merge possible module/callable bindings at the join instead of letting AST visitation order choose the result. Added coverage for one feasible sink branch and for both branches replacing a stale handle.

[yashrajp22]

The collected Store names are not always locals of this function. In def send(): global opener; opener(os.environ.get("API_KEY")); opener = lambda value: value, the later assignment causes the valid outer urlopen handle to be shadowed before its first use, so TT3 is missed. A comprehension target also incorrectly hides an outer handle used after the comprehension. Please exclude global/nonlocal declarations and names belonging to nested expression scopes when collecting function locals.

[agentsope]

Addressed in a797b06. The local collector now excludes global/nonlocal declarations and nested comprehension scopes. Binding updates honor global/nonlocal targets, while comprehensions get an isolated target scope. Added regressions for the reported global-before-reassignment and comprehension-target cases.

[agentsope]

Pushed a797b06 to address the requested reflective-scope follow-ups.

In addition to the six inline cases, this update covers the four previously referenced findings:

• unqualified getattr must be the unshadowed builtin;
• reflective handles are invalidated by for/async for, with as, assignment expressions, exception targets, del, and augmented assignment;
• lambda arguments establish their own shadowing scope;
• class methods are analyzed with fresh function scope without closing over the class namespace.

The resolver now uses iterative expression traversal, deferred function-body analysis, isolated comprehension/lambda scopes, and branch-state joins. I added 19 regression tests for the reported cases and adjacent positive/negative controls.

Validation:

• behavioral taint tests: 95 passed
• analyzer-focused suite: 3,023 passed, 14 deselected, 4 xfailed
• full non-integration/non-provider suite: 6,003 passed, 14 skipped, 40 deselected, 4 xfailed
• non-live integration suite: 30 passed
• ruff check and format-check: passed
• mypy for the changed analyzer: passed

[Copilot]

Copilot review overview

🟡 Changes recommended

Four unresolved moderate analyzer correctness findings affect alias invalidation and builtin handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Missed local bindings cause false reflective sink findings · New