test(journeys): waive each harness subprocess site, then enforce the reason - #430
Merged
Merged
Conversation
…reason
The semgrep tainted-env-args findings in the journeys harness are all the same
shape: a stand-config value from the operator's own env reaches a list argv.
Each site now carries a `# nosemgrep` with the reason that holds THERE, rather
than a path exclusion — a subprocess call added later fires fresh instead of
being silently pre-exempted.
The `limactl shell $FLEET_LIMA_INSTANCE -- argv` leg is called out explicitly:
it rides ssh semantics, which join argv into a command line the VM's shell
re-parses, so "list argv, never a shell" is not true end to end there.
Every waiver states "list argv, no shell", which is an assumption about code
nobody re-reads. test_z_meta_guard now makes it a property: an AST scan over
the whole journeys tree reds on `shell=True`, `os.system`/`os.popen`, or a
command built by f-string / `%` / `.format` / concatenation. Interpolation
inside ONE argv element stays legal — `f"name={cname}"` reaches the program as
a single argument — and the planted-violation test pins both sides of that
boundary so the guard can neither go vacuous nor force the waivers off.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…lling
The first cut of the guard matched a syntactic silhouette, so nine genuine
host-side hazards walked past it. An adversarial pass planted each one and the
detector returned nothing:
subprocess.getoutput("docker rm " + x) # /bin/sh by construction
subprocess.getstatusoutput(f"...") # likewise; semgrep misses it too
from subprocess import run; run(f"...") # callee not spelled subprocess.run
cmd = f"..."; subprocess.run(cmd) # command hoisted into a local
from os import system; system("..." + x)
subprocess.run(a, shell=sh) # non-literal shell=
subprocess.run(a, shell=1)
subprocess.run(a, shell=bool(os.getenv(..)))
sh = subprocess.run; sh("docker rm " + x) # alias
getoutput/getstatusoutput matter most: they take a command string and hand it
to /bin/sh, no list-argv form exists, and semgrep's python bundle does not flag
them either — this guard is their only backstop.
The detector now resolves the callee (from-imports and simple aliases) instead
of matching its spelling, tracks locals holding a built command string, treats
any non-False `shell=` as a hit, and carries a shell-by-construction callee set.
Interpolation inside ONE argv element stays legal, and the clean half of the
planted test pins that so the guard cannot force the waivers off.
The planted test previously exercised only the shapes the detector already
caught, which made it green by construction; it now plants every evasion above.
Two waiver reasons overstated their case and are corrected: test_k_admin's
`body` carries env-derived credentials via `_login` rather than literals, and
`_psql` takes one f-string (interpolating `int(value)` and a module constant).
Both remain safe because each value is one argv element, not because the values
are constants — which is the reason the comment should have given.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e form
A second adversarial pass found a false positive, which is worse than any of
the misses it also found: `string_locals` was a flat, file-global map, so a
local holding a LIST argv was flagged whenever another function in the same
file bound the same name to a built string.
def list_probe(name):
cmd = ["docker", "ps", "--filter", f"name={name}"]
return subprocess.run(cmd) # flagged, and clean
def other(x):
cmd = f"echo {x}"
return subprocess.getoutput(cmd) # the actual hazard
`cmd`, `args` and `argv` are what this harness names its list argv, so the next
helper added to an already-waived file would have reddened the safe form — and
a guard that reds on the safe form forces the waivers off instead of keeping
them honest, the exact failure the negative test claims to prevent. The tree is
clean today, so this was latent rather than visible.
String locals are now indexed per enclosing function (module level included),
and the lookup consults the scope of the call being inspected.
Two genuine hazards the pass also found are closed: `**{"shell": True}`, whose
keyword carries `arg is None` and so was never examined by the shell= loop, and
`import os as o; o.system(...)`, which `ast.Import` never bound (only
`ImportFrom` and attribute assignments were tracked).
The planted test pins all three, including the scope case as a line-number
assertion so a regression names the clean call it wrongly reds. 11/11 evasions
caught, harness green, semgrep tree still 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per-function scoping did not close the false positive; it moved it one level
down. `_index_scope` skipped a nested function with `ast.walk` + `continue`,
which prunes that one NODE and still descends into its body, so an inner
helper's built string landed in its parent's locals and reddened the parent's
clean list argv:
def outer(name):
cmd = ["docker", "ps"]
subprocess.run(cmd) # flagged, and clean
def inner(x):
cmd = f"docker rm {x}"
subprocess.run(cmd) # the actual hazard
Not hypothetical: conftest.py and test_f_agentic_load.py already nest helpers
inside waiver-bearing functions, so the first inner `cmd`/`args` holding a
string would have reddened the outer call.
Indexing now descends through direct children and stops at each nested scope,
carrying a nested definition's decorators and argument defaults with the
enclosing scope, where they actually evaluate.
Comprehensions and lambdas get their own scope for the same reason — a loop
target or a lambda argument binds inside and shadows the enclosing name — and
inherit the enclosing scope minus what they bind, so a comprehension that
merely READS an outer built string is still caught.
The planted test pins both as line-number assertions, so a regression names
the clean call it wrongly reds rather than just going red somewhere.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…first one
A comprehension inside a function resolved its names through the MODULE, so a
command string built in the function was invisible:
def t(x):
cmd = f"rm {x}"
return [subprocess.run(cmd) for _ in y] # missed
Two causes, both fixed. The parent map recorded whichever scope a walk reached
first and refused to overwrite it, which put every nested comprehension under
the module; it now records the nearest enclosing scope by descending from each
scope to its own children. And inheritance was precomputed in walk order, so a
comprehension could inherit before its enclosing function had been indexed; the
chain is now resolved at lookup, walking outward and stopping at any name the
scope binds itself.
A walrus binds in the enclosing scope rather than the comprehension (PEP 572),
so its assignment is collected there.
10/10 scope cases correct, including a comprehension shadowing its own loop
target, a lambda reading a function local, and a two-level nested def. All 12
evasions still caught, semgrep tree still 0.
The planted test pins the three shapes by line number. Its assertion sorts the
hits: they are not emitted in line order, and asserting the unsorted list would
have made the test depend on traversal order rather than on the finding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…inary ones
The inheritance lookup popped `args.args` alone, so a lambda whose own
positional-only, keyword-only, `*args` or `**kwargs` parameter shadowed an
enclosing built-string local still inherited that string, and the lambda's
clean call reddened:
cmd = f"echo {X}"
posonly = lambda cmd, /: subprocess.run(cmd) # flagged, and clean
All five parameter kinds now shadow. A lambda that READS an enclosing built
string is still caught, so the fix narrows nothing.
Two gaps stay open and are now written down in the detector rather than left
implied: a name rebound through `global`/`nonlocal` in another scope, and a
closure reading an enclosing function's local. Both need cross-scope name
resolution, and neither is a shell hazard — `subprocess.run("<str>")` without
`shell=` is a program-name lookup that fails, not a command line. The shell
surface is `shell=` and the always-shell callees, which resolve regardless of
scope.
Mutation-checked: reverting the pop to `args.args` reds the planted test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…journeys-subprocess-waivers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the semgrep half of #346.
What
Thirteen
dangerous-subprocess-use-tainted-env-argsfindings in the journeys harness are all one shape: a stand-config value from the operator's own env reaches a list argv. Each of the nine call sites now carries a per-site# nosemgrepstating the reason that holds there — not a path exclusion, not a rule disable.The
limactl shell $FLEET_LIMA_INSTANCE -- argvleg (conftest.py) is called out explicitly rather than waved through: it rides ssh semantics, which join argv into a command line the VM's shell re-parses, so "list argv, never a shell" is not true end to end there.Why the waivers are not just an assertion of good faith
Every waiver says "list argv, no shell" — an assumption about code nobody re-reads.
test_z_meta_guard.pyturns it into an enforced property: an AST scan over the whole journeys tree (includingconftest.pyandbackends/, not onlytest_*.py) reds onshell=True,os.system/os.popen, or a command built by f-string /%/.format/ concatenation.Interpolation inside one argv element stays legal —
f"name={cname}"reaches the program as a single argument, with no shell to re-parse it. The planted-violation test pins both sides of that boundary, so the guard can neither go vacuous nor force the waivers off.Measured, not read
subprocess.run([os.environ["NEW_THING"], "-x"])produces a finding — the waivers are per-site, not a blanket exemption.shell=True,f"docker rm {path}",os.system("echo " + path),"cat %s" % pathinto a live harness file reds the suite; all four caught.The first cut of the detector flagged 13 live sites — it conflated an f-string inside one argv element with a built command line. Narrowed to the latter, then re-mutated to confirm the narrowing did not blunt it.