fix(taint): detect inline urllib opener network sinks

[agentsope]

Summary

Partial improvement for #714; leaves the issue open. Recognize inline
urllib.request.build_opener(...).open(...) and stored opener instances as
network-output sinks using the analyzer's existing name/alias/type-map policy.

• Resolve an inline .open receiver's factory with resolve_call_name_typed.
• Add urllib.request.build_opener.open to the network-output sink set; the
  existing build_type_map also recognizes opener = build_opener(...).
• Remove the file-wide rebinding gate, extra traversal and arguments-only path.
• Inspect handler construction along with request arguments, consistently with
  existing network sinks. This detects possible credential-bearing handlers;
  it does not prove that every handler sends the provided value.
• Retain existing runtime limits and finding location/occurrence handling.

Scope and tradeoffs

The nested os.environ["API_KEY"] source-propagation gap remains: a subscript
inside Request(..., headers=...) is not recognized by the existing source
collector. This affects direct urlopen controls too. Dynamic/reflection and
precise lexical-scope resolution are outside this change.

The existing name-based policy may report a shadowed urllib spelling, just as
it does for other sinks. Handler inputs such as environment-derived proxy or
CA paths can also trigger a finding. Inline and stored forms now follow the
same policy rather than suppressing all sinks for an unrelated binding.

Validation

All sample code is parsed, not executed as network requests. No live or paid
provider calls are made. Updated regressions cover four import styles,
function-local/conditional imports, unrelated bindings and attribute stores,
definitions preceding imports, stored and inline credential/file/public
payloads, credential-bearing handlers, unrelated factories and locations.

The first targeted selection reproduced 15 failures and 4 passing controls
before the implementation change. The complete behavioral taint module passed
116 tests. The full non-integration/non-provider suite passed 8,543 tests,
with 14 skipped, 134 deselected and 4 expected failures in 349 seconds.
Repository-wide Ruff lint/format, targeted mypy and whitespace checks passed.
The full run bypassed pytest's local readline workaround in-process; capture
remained enabled and neither project nor Python installation files were changed.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[rng1995]

[SkillSpector Review]

Hi @agentsope, thank you for digging into #714 so carefully, and for being upfront about what this change does not cover!

Value and readiness: With this change, the inline form from the issue, urllib.request.build_opener(...).open(req), reaches TT3/TT4 when urllib is imported at module level. It adds no new class of false positive: the sink fires only when a source or tainted name reaches .open(...), the same rule urlopen follows. It is not ready yet, for two reasons. First, the new binding gate disables the sink for the whole file whenever the import root is bound anywhere, and two tests assert "no TT3" for code that does send the credential. Second, the PR body's wording will auto-close #714 on merge, although the stored-opener and nested os.environ[...] gaps remain. Resolving the factory with the analyzer's existing alias and type-map machinery would remove most of the new code. It would also cover opener = build_opener(...); opener.open(req), which is how most real hardening code is written.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/behavioral_taint_tracking.py:291-390: the binding gate turns the new sink off in ordinary code, and a single dead line turns it off on purpose.
   • The gate needs a module-level import above the call. Any binding of the root name anywhere in the file disables every inline opener in that file. That covers parameters, loop targets, except ... as names, nested imports and attribute stores, in any scope, even if the binding never runs.
   • I re-implemented the gate with the stdlib (no project code imported or run). Only the first shape below is recognized:
     • top-level import urllib.request and the inline call (recognized);
     • import urllib.request inside the function that makes the call;
     • import urllib.parse inside an unrelated helper;
     • from urllib import request plus any def handle(request): elsewhere;
     • import and call inside if __name__ == "__main__":;
     • urllib.request.HTTPRedirectHandler.max_repeats = 2 anywhere;
     • for urllib in (): pass or def _unused(urllib=None): pass, which never run;
     • the import placed after the def that uses it.
   • test_module_rebinding_does_not_create_a_false_sink pins two of these. After for ur in []: pass, or try: pass / except Exception as ur: pass, ur is still the urllib module at runtime. So ur.build_opener().open(os.getenv("KEY")) sends the key, yet the test asserts no TT3.
   • Every other sink in this analyzer is matched by spelled name plus import_aliases, with no scope check. My model of main resolves urllib.request.urlopen(...) in every shape above. Main also reports def f(requests): requests.post(url, data=os.getenv("K")) as TT3. So one file can report urlopen and silently miss the opener.
   • I understand the gate follows the scoping feedback on #589. There the sink is inferred from a value stored in a variable, so binding analysis is unavoidable. Here the call spells the module path, as urlopen calls do.
   • Expected fix: in _resolve_sink_name, when node.func is Attribute(attr="open") over a Call, resolve that inner call with resolve_call_name_typed(node.func.value, type_map, aliases). If it is urllib.request.build_opener, return urllib.request.build_opener.open, and add that name to _NETWORK_OUTPUT_SINKS. This mirrors how resolve_dynamic_import_call handles import_module(...) chains.
   • build_type_map already records opener = urllib.request.build_opener(...) as urllib.request.build_opener. I checked the four import styles in your tests plus a function-local import. So the same sink entry also covers opener.open(req).
   • The shadowing and relative-import negative tests then describe the imprecision every other sink already accepts: drop them, or mark them xfail with a reason. Please drop the dead-binding cases.
2. [Blocker] PR body: "does not fully resolve #714" contains the closing keyword resolve #714. GitHub already lists #714 as an issue this PR closes, so merging will close it. The nested os.environ["API_KEY"] gap you found is tracked only in #714's comments. Please reword the body (for example "Partial improvement for #714; leaves it open") and check that the Development sidebar no longer links #714. A separate issue for the nested-subscript gap would also help, since it affects every TT3 sink, e.g. requests.post(url, data=os.environ["K"]).
3. [Non-blocking] src/skillspector/nodes/analyzers/behavioral_taint_tracking.py:476-484: arguments_only=True skips the factory and its handler arguments.
   • A custom handler can attach the credential to every request, e.g. an http_request method that adds a header. So urllib.request.build_opener(Leak(os.getenv("K"))).open(url) is real exfiltration that stays silent. test_handler_construction_is_not_a_request_argument pins that.
   • Main walks the whole call for other sinks: requests.post(url, auth=Auth(os.getenv("K"))) reports TT3.
   • With the stored form covered through the type map, opener = build_opener(Leak(os.getenv("K"))) taints opener, so opener.open(url) would report TT3 while the inline form would not.
   • I suggest walking the whole call and dropping _walk_call_payload, unless the PIC decides otherwise (below).

PIC tradeoffs:

• Handler arguments (finding 3): walking them catches credentials injected by custom or auth handlers. The cost is TT3 when a handler receives a proxy URL or CA-bundle path from the environment. Main already accepts that cost for requests (proxies=, verify=, auth=). Whichever rule is chosen should apply to the inline and stored forms alike.
• Name-based resolution (finding 1) reports a shadowed urllib name, such as a parameter called ur, the same way main reports a shadowed requests. I think consistency and recall outweigh that rare false positive.

Verification and gaps:

• I read the full diff against merge-base 35270064, the analyzer at the PR head, common.py (resolve_call_name_typed, build_type_map) and python_ast.build_import_aliases.
• Stdlib models, written for this review: the gate on 11 shapes, and main's resolution of the stored form under 5 import styles plus the urlopen and requests.post controls.
• I traced the #714 reproduction. A req built with os.environ["API_KEY"] nested inside Request(...) is not tainted on main: _find_source_in_expr matches only calls, and the subscript branch handles only a bare right-hand side. So neither urlopen(req) nor the opener reports TT3, as you observed.
• git merge-tree with current main is clean. Main's later _emit changes (node_index, node-keyed seen) compose with this diff.
• #611 rewrites the same module. The two changes are independent in behavior: #611 changes how tainted is collected, and this PR changes sink identification. The conflicts are textual only. If this PR lands first, #611 gains one extra conflict on the collections import line (keep both deque and Iterator). #611 already conflicts with main in the tainted/seen setup and the sink-loop header, so it needs a rebase anyway. As the diff stands, landing #611 first is cleaner. With finding 1 applied, this PR no longer touches the imports or _analyze_python, and the overlap disappears, so either order works. #589 (reflective urllib handles) also conflicts textually with this PR.
• CI: all 6 checks are green on d3dcd81e. I did not run tests locally, per policy.

---

Decision: Changes Requested (reviewed head d3dcd81e7577df2205f3f758b8e3628ea0fa47bb)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[agentsope]

Addressed the review in cd9c406:

• Replaced the file-wide binding gate with resolve_call_name_typed in _resolve_sink_name and the canonical urllib.request.build_opener.open sink. Existing type-map resolution now also covers stored opener instances.
• Removed the arguments-only traversal. Inline and stored handlers now follow the existing whole-call source/taint policy; regressions cover credential-bearing handlers and ordinary import layouts.
• Reworded the PR description as a partial improvement for TT3: credential sent through urllib.request.build_opener(...).open() is not reported #714, leaving the nested os.environ[...] propagation gap explicitly out of scope.

Validation: 116 behavioral-taint tests passed; the full non-integration/non-provider suite passed 8,543 tests (14 skipped, 134 deselected, 4 xfailed). Ruff lint/format, targeted mypy and whitespace checks passed. Samples were parsed only; no live provider/network samples were executed.

The name-based shadowing and handler-input tradeoffs are documented in the description. Please re-review the updated head when convenient.