Skip to content

fix(security): re-join the path a find traversal factors apart - #7298

Merged
iamwhatever merged 1 commit into
mainfrom
fix/bash-gate-find-exec-xargs-7034
Sep 4, 2026
Merged

fix(security): re-join the path a find traversal factors apart#7298
iamwhatever merged 1 commit into
mainfrom
fix/bash-gate-find-exec-xargs-7034

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

1. What is the problem?

The bash gate asks one question of every token: does this resolve to a fenced path? A find traversal answers no by CONSTRUCTION. It factors the path in two -- the directory is one argument, the leaf is another -- and produces the path itself at runtime, so the fenced path appears nowhere in the command text.

Measured on main:

MISSED   find ~/.kiro/crew -name '.env' -exec cat {} +
MISSED   find ~/.kiro/crew -name 'token_signing.key' -exec cat {} +
MISSED   find ~/.kiro/crew -name 'computer_use.json' -exec cat {} +
MISSED   find ~/.kiro/crew -name '*.lock' | xargs cat
MISSED   find ~ -name 'credentials' -exec cat {} +
BLOCKED  find ~/.ssh -type f -exec cat {} +

That last line is the tell the issue points at: ~/.ssh is fenced as a DIRECTORY, so the command names it directly and the existing matcher sees it. A leaf fenced by FILENAME is invisible, because the name appears only as a -name argument -- syntactically unrelated to the directory beside it.

Neighbourhood probing found the same hole with no -name at all (find ~/.kiro/crew -type f -exec cat {} + reads every keystone leaf), and through every other way a traversal hands a match onward: -execdir, -ok, -okdir, -delete, -fprint/-fls/-fprintf, a pipe into any xargs spelling, a command or process substitution, a plain redirect.

2. Why this issue matters to the user

The keystone floor exists so a prompt-injected agent cannot read or author its own authorizations. .env, token_signing.key, .local_secret and security_policy.json are all fenced by filename, so all of them are reachable this way, and ~/.aws/credentials is reachable too since .aws is fenced but the traversal starts at ~.

Unlike a race against a transient temp, this needs no timing and no guessing: the command is deterministic, the file is the permanent one, and the protected spelling and the unprotected one differ by moving the filename one argument to the left.

It was raised by the GPT review lane on #7024 and deliberately NOT fixed there, because a fix scoped to that PR's artifacts would have guarded the transient copy while leaving the permanent secret open to the identical command -- worse than the honest gap, because it reads as covered.

3. How our fix solves it

Re-join what find factored apart. A new pass (_check_find_traversal_reaches_fence) reads the traversal's roots and its name/path filters and asks whether the SET it names intersects the fence. Three bounds keep that from becoming an over-block, and the issue's own two candidate shapes are each rejected for the reason it gives:

Delivery. A find that only LISTS is left alone, so the newly-denied set is exactly the traversals that hand a match to a command. The inert primaries are an ALLOW-list and anything else denies -- the polarity _TRUST_ROOT_READ_LISTERS already documents, because enumerating the delivering primaries would fail OPEN on the one nobody thought of. This needs no knowledge of the CHILD command, which is the issue's objection to gating the sink: cat, base64, a script and xargs with any of -0/-I/-n/-P are all the same to it, because the gate denies NAMING a fenced path whatever is then done with it. That is also why there is no executor list here at all.

Framing. WHICH text to judge is its own question, and answering it by inspecting the command's characters was wrong twice. Capture was read off the two opener characters glued to the program word's token, so cat $( find ... ) was allowed where cat $(find ... ) was denied -- the same read, one space apart. And a bash -c '<traversal>' payload was never re-tokenized, so the traversal was invisible even though the argv floor already descends into payloads carrying a plain fenced path. Both are one defect: the pass judged the command's TEXT rather than the things the shell runs. _find_traversal_views enumerates those instead -- the outer line, every substitution body, every nested payload -- so capture stops being a spelling to detect and becomes a property of how a view was DERIVED, and no depth of wrapping is a special case. It reuses this module's own view idiom (_self_token_frames joins exactly these two extractors) and its termination discipline: no depth cap, because whatever number is chosen one more level defeats it; a view is a proper substring of its parent, and a visited set stops sibling wrappers re-walking the same text.

Certainty. The join is denied where the FENCE supplies the missing half, and left alone where the traversal merely might wander into one:

clause example why the fence answers it
the root is fenced, or holds fenced leaves find ~/.kiro/crew -type f -exec cat {} + the command names the credential directory outright and the leaf is the runtime wildcard -- the ~/.kiro/crew/$F shape _sensitive_under_unresolved_var already denies
a pattern matches a basename the fence DECLARES find ~ -name '.env' -exec cat {} + the fence lists that name; no filter at all means the pattern is *, which matches every entry under the root
the filter asks for a NAME that carries credentials wherever it sits, and a whole-directory credential store lies under the root find ~ -name credentials -exec cat {} + everything inside .aws/.ssh is fenced, so a request for one such name resolves a path rather than searching

The third clause is the one that needed care, and it is why the issue's other shape -- treat any root that CONTAINS a fence as the signal, via path_contains_sensitive -- is not what shipped: find ~ names the home directory, which contains everything, so that reading refuses unqualified traversals outright. Instead the clause asks what the FILTER names. _find_filter_names_a_credential_leaf (security.py:9168) compares the filter against a fixed vocabulary of leaf names that carry credentials wherever they sit -- _CREDENTIAL_LEAF_NAMES, 16 entries at security.py:7523, and _CREDENTIAL_LEAF_SUFFIXES, 7 entries at security.py:7543, both private. It performs no filesystem access at all: no stat, no directory listing. A literal name is tested directly; a glob is matched against that same vocabulary (each leaf name, plus one synthetic bearer per suffix), which is what makes -name 'id_*' answer like -name id_rsa and -name '*.py' match nothing.

Two earlier revisions asked the filesystem instead, and each way of asking was its own defect. An existence check joined onto the store could only ever see a store's DIRECT children, so a key one level down was read while the same name at the top denied. Matching a glob against a store's real entries cost 110 directory listings and 14.4ms on a single traversal, against 0 listings and 0.7ms for an ordinary fenced read, on a gate that is synchronous and in-process. Those two point in OPPOSITE directions -- probe deeper, stop enumerating -- so neither is reachable by extending the probe. Deciding from the name removes the depth question and the enumeration together, and drops a host-dependence that was never a feature, where the same command was allowed or denied according to whether a store happened to exist yet.

The cost is that the clause is strictly WIDER than a probe in one direction and NARROWER in another, and both are stated in the residual list below. Wider: a credential-looking name is refused whether or not that store exists on the machine. Narrower: a leaf whose own basename carries no credential signal is not reached from an ancestor root. What bounds the width is that the vocabulary is conservative -- known credential leaves and suffixes, nothing pattern-like -- because a broad rule reintroduces exactly the false positives it exists to avoid: the fenced tree includes OPERATIONAL directories that hold ordinary readable files (~/.kirocrew/run holds transient sandbox *.py wrappers, ~/.local/share/kiro-cli holds tui.js, several fenced directories hold a *.json). That is the entire difference between denying find ~ -name credentials -exec cat {} + and leaving find ~ -type d -name __pycache__ -exec rm -rf {} + alone -- no credential name is asked for, so the clause says no.

-path/-wholename/-ipath matches the whole path, so the pattern carries the fenced segments itself; dropping its wildcard SEGMENTS leaves something the ordinary path gate can answer (*/.aws/credentials reduces to .aws/credentials, while */node_modules/* reduces to node_modules).

gfind is parsed alongside find: macOS installs GNU findutils under that name, and the two share the grammar this pass reads.

A filter is agent-supplied text, and this gate is synchronous and in-process, so evaluating one is a denial-of-service surface before it is a correctness question. CPython's re has no timeout, and this module already records a watchdog-crossing hang from admitting a run into a pattern. Two rules bound it, and both fail CLOSED by widening the traversal rather than dropping the filter:

  • A -regex/-iregex pattern is never compiled and never run. Screening for catastrophic shapes was rejected rather than skipped: this module already argues that enumerating dangerous constructs cannot terminate against an untrusted string, and a hostile pattern author is exactly that case. _redos_prone exists here but was written to catch accidents in the repository's own deny patterns, which is a far weaker claim than defending against a crafted one. The cost is an over-block on a rare flag, bounded by the root -- see section 5.
  • A glob is bounded instead, because it can be. _glob_to_regex maps * and a brace group to .*, so -name '{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}b' compiled to fourteen adjacent .* and hung the gate outright -- measured as still running after 12 seconds, now 0.031s. Adjacent runs are collapsed, which is semantics-preserving (.*.* names exactly what .* names, so that pattern and * x40 both become a single .* and still match), and literal-separated runs, which cannot be collapsed, are capped at 8. Over the cap the matcher is refused, which the caller reads as opaque, so the bound can never become a bypass.

What makes the cap sufficient rather than a guess is that the SUBJECTS are not agent input: they are fence basenames and credential-store entry names, all short. Only the pattern is hostile.

The change can only ADD denials. It is a new pass returning a reason or None, appended after the existing ones; no existing pass is touched. The one deliberate over-trigger is on record as a test rather than left to be found: find ~ -name '*.json' -exec cat {} + is refused, because *.json covers security_policy.json and config.json, which the fence declares by name -- the glob carve-out is about names the fence does NOT declare, not about widening one it does.

The spellings that reach the pass were found by running it rather than by reading it. The cheap "find" not in command bail-out was itself a bypass twice over: fi''nd reaches shlex as find, so quote and escape characters are stripped first, and a shell expands f?nd against the filesystem, which no substring test on the text can see -- so a glob metacharacter now defeats the bail too, and the program word is matched through the same bounded matcher the filters use (_find_program_word_names_find). A word the wildcard bound refuses is read AS a match, so the bound widens the pass rather than opening a hole in it. A process substitution arrives with the redirect glued to the program word (<(find); a control operator glued to the program word (ls|find, true;find, true&&find) arrived as ONE shlex token whose basename matched no program name, so the whole pass was skipped -- the gate defeated by deleting one space. A captured substitution's closing paren arrives glued to the pattern ($(find ~ -regex '.*/id_rsa$')), which is why the strip lives where patterns are READ rather than per-list at the call site. And the brace-group glob hung the gate outright.

-files0-from is the mirror image on the other operand: GNU find reads its roots from a file, or from stdin for -, so the command names no root at all and a parse reading only operands defaulted to . while the traversal walked a fenced root it never saw. It is read as an unknowable root -- the same treatment a root carrying an unassigned expansion gets -- so the filter still decides and find -files0-from list.txt -name '*.o' -delete is untouched. Closed by construction: it is the only way find takes a root that is not an operand.

Delivery is likewise read from the invocation's own token span rather than the command line, because reading the whole line denied a listing whenever an unrelated LATER command carried a pipe (find ~/.kiro/crew -type f; cat notes | less).

Two shapes were fixed wider than the review that found them prescribed, both because the prescription would have left a sibling open: the store question replaced a literal-only probe that denied -name id_rsa while allowing -name 'id_*' for the identical read, and the wildcard bound covers the glob path, where the reproducible hang actually was, rather than only the -regex path that was reported.

What this covers, and what it deliberately does not. This pass recognises a traversal by its SPELLING, and that is the whole of the claim. An operand the shell COMPUTES is out of scope: $(printf find) ~/.kiro/crew -type f -exec cat {} + is allowed, because knowing it runs find means knowing what printf writes, which is not a property of the command text. The same holds for a glob-bearing root (~/.kir*/crew), brace expansion (f{i,}nd), a root supplied from outside the command line (-files0-from - with no filter left to judge), a genuinely unassigned expansion, and a root reached by a preceding cd rather than named. Review measured 17 such spellings across six rounds, and closing one revealed others every time -- the set of programs whose output is find is not enumerable by a pattern over the text, so a spelling-based recognizer does not terminate on this class. #8074 carries that argument and the fix (invert the polarity: require literal operands, fail closed on computed ones), which is a behaviour change on a security gate with its own false-positive surface to price and so is not folded in here. This is a descope, not a to-do list.

What the pass DOES resolve is what the text alone determines, and those cases are tested: a same-command assignment (F=find; $F, and the split F=fin; ${F}d), a glob-expanded program word (f?nd), a quote splice (fi''nd), a nested -c payload, and the body of a substitution -- each turns a computed spelling back into a literal one. A test pins BOTH sides of that line, so the description cannot drift from the behaviour.

Two further boundaries, both properties of the code rather than oversights. First, the credential-name vocabulary is the one clause whose polarity lets an OMISSION allow: find ~ -name known_hosts | xargs cat, -name 'pubring*' and -name trustdb.gpg are allowed because neither the vocabulary nor the fence's declared entry names cover them (measured), and so is a private key the user named themselves (find ~ -name github_work). No addition to the list closes the latter -- the name space is the user's -- so this is a boundary, and the choice between the two measured ways to invert it is tracked in #8074. A name the fence DECLARES is still caught by the clause above it, which is why -name config and -name config.json do deny (.kube/config, .docker/config.json are declared entries). Second, in the other direction, a credential-looking filter is denied whether or not the store exists on this machine: find ~ -name id_rsa -exec cat {} + is refused on a home with no .ssh at all. That is the fail-closed direction and it is deliberate, but it is a behaviour change worth naming rather than discovering. Outside the pass entirely: any OTHER program that factors a root and a name the same way -- fd/fdfind (-x/-X), locate/plocate, rg --files, du -a -- and grep -r <dir>, which needs no second command at all because the reader IS the traversal. Filed as #7309. The block comment says all of this where someone changing the code will read it.

4. What tests we did

TestFindTraversalReachesFence in test/test_security.py, 197 cases:

  • The five commands from the issue, each a read of a permanent secret. The fifth reaches ~/.aws/credentials, so it runs in a fake home with the store on disk rather than depending on the runner's own dotfiles.

  • The carrier grammar, 19 forms: -exec with ; and +, -execdir, -ok, -okdir, -delete, -fprint, -fls, -fprintf, -exec sh -c, four xargs spellings including -0 and -I{}, a while read loop, both substitution spellings, a process substitution, and a plain redirect. Plus an unknown primary, which must deny.

  • Traversal spellings: -L/-H/-P, -D tree, -O3, multiple roots, /usr/bin/find, fi''nd, $HOME and ${HOME}, depth flags, a parenthesised -o group, and a find after &&.

  • Zero false positives, 23 cases: project-rooted traversals for every delivery form, absolute non-home roots, home-rooted GLOB searches, the crew home's own non-secret subtrees (workspace, workspace/memory, skills -- which agents read constantly), listing-only forms, and commands that are not traversals. All 23 pass on main too, so none is a behaviour change.

  • The boundary of the name clause, in both directions: credentials and id_rsa deny by vocabulary and *.pem by suffix, while package.json, __pycache__, tsconfig.json, a *.py glob and every listing form stay allowed. A separate test pins the narrow side as a named residual -- known_hosts is fenced by WHERE it sits rather than by what it is called, so it is allowed from an ancestor root while cat ~/.ssh/known_hosts and find ~/.ssh -type f -exec cat {} + both still deny. Another pins the wide side: the same request is refused on a home with no store on disk. And one counts filesystem calls through the clause and asserts zero, because "it is fast now" is not a property and zero is.

  • A 61-command benign corpus and a 400-command carrier cross-product (4 root spellings x 10 filters x 10 delivery forms): 0 false positives, 0 misses.

  • The three round-1 findings, in both directions: six filter spellings of one store read all deny while two regex spellings of an ordinary read stay allowed; five glued-operator prefixes plus the glued-pipe-after case deny; four sequencing shapes stay allowed with | less as the control that must deny.

  • The pattern-evaluation bounds, in both directions: three adjacent-wildcard globs asserted to collapse to exactly one .* and still match, three literal-separated ones asserted refused, six ordinary globs asserted still compiling, the regex-widens clause with its root-bounded counterpart, and a wall-clock assertion over four previously-hanging commands with a deliberately enormous margin (measured ~30ms, asserted under 10s, so it cannot flake on a loaded box while still catching a return to super-polynomial matching).

Mutation-verified: with security.py reverted to main's version, 148 of the 197 cases fail -- direct evidence the bypasses were real -- while all 49 no-regression controls pass on both sides. Separately, each mechanism this revision adds was verified by breaking it on purpose: 15 targeted mutations, 15 killed (drop nested payloads from the view set; drop substitution bodies; make a body not carry capture; make a payload not inherit its wrapper's capture; revert the program word to an exact match; revert the bail-out to a substring test; make the wildcard cap fail OPEN; disable -files0-from; trust the defaulted .; always and never hypothesise; revert the store probe to literals; accept any store entry; widen the predicate to a substring rule; let the unreadable-store OSError escape). One mutation SURVIVED on the first run and exposed a test that did not bite -- an all-wildcard program word collapses to a single .* and so never exercised the refusal path -- which is why that assertion now uses a literal-separated pattern the cap genuinely refuses. The individual review findings were verified the same way but by probe: each was reproduced as a measured ALLOW (or a measured false DENY, or a measured hang) before the change and flipped after, with the controls unmoved.

Targeted runs, never the full suite: test_security.py + test_governance_self_protection.py + test_denied_commands_security.py -- 1985 passed, 1 skipped. test_hooks.py + test_connections_tool_aliases.py + test_app_sources_write_protection.py -- 416 passed. Also green earlier: test_computer_use_api.py, test_computer_use_enable_state.py, test_config_loader.py, test_aws_consent.py, test_snapshot_redaction_optout_ceiling.py, test_mcp_cron_security.py. flake8, isort and mypy clean; the baselined black gate passes in scope, and the added code is black-clean even though security.py is baselined.

5. Any other suggestions on the work

  • The computed-operand class is descoped to bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074, and that is the honest shape of this change. Six review rounds on this PR kept surfacing new spellings of one class (6 -> 10 -> 13 -> 17 distinct spellings on the record). Every finding that turned out to be closable was the parser assigning a literal token to the wrong ROLE -- a leading redirect read as a root, a filter read as bounding a delivery it does not bound, &> read as a control operator, a pre-filter that missed a spelling the pass could already resolve -- and each of those is fixed here. Every finding that stayed open needed a value the text does not carry. Continuing to patch spellings would have kept producing revisions without converging, so the class is now recorded with its existence proof rather than half-covered. bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074 also carries the credential-name polarity decision, with the price of each option measured (the narrower option flips 12 of 12 ordinary literal-name searches to DENY).

  • The sibling traversal tools are the honest remaining gap, and are now filed. fd -x cat is the same deterministic single-command form with a different grammar (positional regex pattern, -x/-X exec), locate | xargs has no root operand at all, and grep -r <dir> needs no sink because the reader and the traversal are one command. Bash gate misses a fenced path reached through fd, locate, rg --files or grep -r #7309 carries all of them, with the note that a producer allow-list is the wrong direction -- the module's comments already argue that enumerating programs fails open, so a half-done list would read as covered. The two reusable halves of this pass are already tool-independent, which is what that issue points at.

  • A keystone publish artifact named literally from outside the crew home is not covered. find ~ -name 'tmpAB12CD34.tmp' -exec cat {} + needs the mkstemp name, which is the timing race fix(security): fence a keystone leaf's atomic-write temp and lock sibling #7024 describes; rooted at the crew home it is denied by the first clause. Recorded rather than papered over.

  • The -regex over-block is the one deliberate cost of not evaluating agent patterns. A DELIVERING -regex traversal over a root that contains a fence is refused whatever the pattern says, find ~ -regex '.*[.]py$' -exec grep -l foo {} + included. It is bounded by the root -- find ~/Repos -regex ... -exec wc -l {} + and find /tmp -regex ... -delete are untouched, and a listing is untouched wherever it is rooted -- and both directions are pinned by tests. If the flag turns out to be common enough for that to bite, the fix is a linear matcher for a reduced subset of the regex grammar, not a screen for dangerous shapes.

  • The credential-filename predicate is deliberately conservative, and deliberately not yet shared. It lists known credential names and suffixes only; a broad rule (any name containing secret) would reintroduce exactly the false positives it exists to avoid, and a test pins the three families that must stay allowed. Two leaf modules carry their own copy of this list -- PROJECT_SECRET_NAMES in the design-tweak preview server and _EXCLUDE_NAMES in the cloud source packer -- and both already import DENIED_ROOT_PARTS from security.py, so they can be pointed at the new constant the same way. That migration is left out on purpose: it would move the behaviour of two unrelated surfaces (static-file serving, tarball packing) inside a security fix.

  • An unreadable credential store is not a hole, and the reason is simpler than it was. Nothing on this revision lists a store, so a directory the process cannot read cannot reach the verdict at all -- there is no fallback to get wrong. An earlier revision needed an explicit OSError guard; the test that injects PermissionError is kept because a future revision reaching for the filesystem again would need it back.

  • The depth flags are read but not modelled. -maxdepth 1 narrows what a traversal visits, and this pass ignores that, so it over-approximates the visited set. That is the fail-closed direction and cheap; modelling depth would only ever remove denials.

Pattern harvest

Rule candidate: review-prompt
Pattern: a path-string fence judged only against the tokens a command NAMES, while the shell can compute the path at runtime.

This is the third member of one class, and all three were found the same way -- probe the gate with a command that reaches the fenced file without spelling it. cd ~/.kiro/crew && cat .env removes the ANCHOR (closed by the working-directory tracker); the Windows cd spelling of that same removal is the sibling comment on #7034 and is still open; find -exec removes the path ENTIRELY. Each arrived as its own bug report. The shared property is that the gate's question -- "does a token resolve to a fenced path?" -- is not the shell's question, which is "what file will be opened?".

So the harvest is a check to run when a fence is added or widened: ask which commands can reach the fenced path WITHOUT naming it -- by moving the anchor (cd, pushd, Set-Location), by generating the leaf (find -name, a glob, a variable), or by handing a produced path to a second program (xargs, a substitution, a while read loop). The three known members are all one of those three moves.

Deliberately NOT a semgrep or lint rule: the defect is a missing case in a semantic model, not a code shape a matcher can see -- there is no syntax common to cd, -exec and $F to match on. The module already documents the class in three places (bare_protected_path's comment, _sensitive_under_unresolved_var, and now this pass), which is itself the evidence that what was missing is the prompt-level question rather than a pattern.

Fixes #7034

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 31, 2026 16:32
@chenmingwei23
chenmingwei23 requested a review from dwu96 August 31, 2026 16:32
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 258bea54dba29aea99ea8c3c24baba8ed392864c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real, deterministic keystone-floor bypass closed by a fail-closed, additive-only pass; alternatives measured, residual class honestly descoped to filed issues.

[DESIGN-REVIEWED] 258bea5

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for 258bea54dba29aea99ea8c3c24baba8ed392864c; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 258bea54dba29aea99ea8c3c24baba8ed392864c: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 258bea54dba29aea99ea8c3c24baba8ed392864c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. Composing the review.

First-Principles-Verdict: CONCERNS

Every item closes a measured bypass of the reported defect, but the change ships the repo's third curated secret-filename vocabulary, with the two siblings named and left to diverge.

What this change ships

Intent: stop a prompt-injected agent reading fenced secrets by making find construct the path at runtime — a FIX (#7034).

  1. Delivering find over a fenced root, or asking a fence-declared basename, is denied — justified (five measured bypasses)
  2. find asking a credential-name leaf (id_rsa, credentials…) over a store-holding root is denied — justified, but third vocabulary copy (see Watch)
  3. Listing-only find stays allowed — justified bound
  4. Traversals inside bash -c payloads and substitutions are judged — justified (same read, one wrapper away)
  5. Obfuscated spellings resolved: braces, glob program words, += appends in the shared resolver — justified (each a demonstrated one-edit defeat)
  6. Commands past width/depth ceilings now deny (64 substitutions/roots, 32 brace forms, 16 nesting) — justified (measured 12 s hang, 28.9 s walk, RecursionError)
  7. -regex filters are never run, over-blocking delivering regex traversals of fence-holding roots — justified (CPython re has no timeout)
  8. Filters that don't bound delivery (-o, -not, ,, delivery-first) read as unfiltered — justified (find's evaluation order)
  9. Unknowable roots ($VAR, -files0-from) are decided by the filter — justified bound
  10. Computed-operand class ($(printf find), user-chosen key names) descoped to bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074 — declared residual

More granular differences exist (BSD pre-root flags, glued operators, redirect ordering); all serve items 1–2. No new public surface, config key, or flag; existing machinery (is_sensitive_path, _resolve_local_assignments, _substitution_bodies, _glob_to_regex, Pass 4 for non-find traversal tools) is reused rather than re-spelled.

Watch

  • Counted: three "does this FILENAME carry a secret" vocabularies now exist — new _CREDENTIAL_LEAF_NAMES/_CREDENTIAL_LEAF_SUFFIXES (security.py), PROJECT_SECRET_NAMES/PROJECT_SECRET_SUFFIXES (apps/builtins/design_tweak/backend/preview_files.py:149,173), _EXCLUDE_NAMES/_EXCLUDE_SUFFIXES (cloud/source.py:71,63). The suffix tuples are byte-identical; the name sets already diverge (the review-added accesstokens.json, application_default_credentials.json, access_tokens.db reached only the new copy). All three fail open on omission, so an entry added to one after an incident protects one surface out of three. The docstring defers the migration; a human should see it deferred.

Subtractions

  • Delete PROJECT_SECRET_SUFFIXES (preview_files.py:173) and _EXCLUDE_SUFFIXES' credential half (cloud/source.py:63) and import the security.py tuple both siblings' modules already import from — the docstring names this migration; doing the suffix half now removes two byte-identical copies at zero behavior change.

[FIRST-PRINCIPLES-REVIEWED] 258bea5

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 258bea54dba29aea99ea8c3c24baba8ed392864c — this comment is updated in place on each push.

Review details

I've analyzed the sole candidate against the code.

Candidate 1 — over-budget numeric brace sequence degrading to a literal. The divergence is real at the code level: _find_brace_sequence("1..100") returns None once the 33rd item is appended (line ~1109), and _find_brace_alternatives reads that None as "not a sequence" (the pass branch), falling through to the comma split, which returns the single literal ["1..100"] instead of propagating None up to a refusal. That does under-approximate relative to the shell.

But the finding needs (c) an observable wrong outcome, and that cannot be established:

  • For the degradation to become a missed denial, one of the unenumerated expansions (x1x100) would have to land on a fenced/credential path. Fence entries and credential-store names (_SENSITIVE_HOME_DIRS, _CREDENTIAL_LEAF_NAMES) are fixed literal strings, never pure digit runs, so no numeric-range item can equal one. Letter sequences cap at 26 items and never reach the 32 budget, so they are unaffected entirely.
  • Where a fence appears literally in the root (e.g. ~/.ssh/x{1..100}), the degraded literal reading ~/.ssh/x1..100 still startswith("~/.ssh/"), so is_sensitive_path on the root reading denies regardless of the sequence — the bug supplies no bypass there either.

So (c) resolves to "if any expansion resolves onto a fenced path" — a case the diff's own semantics make unconstructable, exactly the "could/might" the falsification bar rejects. The candidate's own confidence line concedes it could not build a concrete exploit. The item is a documented-contract divergence with no reachable wrong ALLOW; it does not survive.

No other finding grounds to the (a)/(b)/(c) bar within the changed lines.

No findings.

[OPUS-REVIEWED] 258bea5

Verdict parsed from the review's SHA-scoped output markers for commit 258bea54dba29aea99ea8c3c24baba8ed392864c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 258bea54dba29aea99ea8c3c24baba8ed392864c: <one-sentence reason>

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 1 disposition -- all findings acted on, none overridden

Every finding reproduced before it was touched. The probe results below come from a fake $HOME holding a real .aws/credentials and .ssh/id_rsa, so the before/after is a measured flip rather than an argument.

Opus 4.8 -- 3 findings, all REAL, all fixed

security.py:352 -- the -regex family got neither the store probe nor the segment reduction. Confirmed: find <home> -regex '.*/id_rsa$' -exec cat {} + was ALLOWED while find <home> -name id_rsa -exec cat {} + was DENIED. The same hole covered -name 'id_*', which the finding did not name but which fails identically: a glob matching a real store file was allowed too.

Fixed wider than prescribed, because the prescription (reduce the regex's fixed leaf) would have left the glob spelling open. _find_store_holds_match now asks the STORE what it holds: exact os.path.exists for a literal, a bounded os.scandir match for a glob or a regex. That removes the literal/glob distinction from the boundary entirely, which is the right shape -- the question was always "does this filter name a file that is really in a credential store", and only the answer path differs by spelling. All six spellings of the one read now deny; -regex '.*/[.]py$' and -name '*.py' still answer None, because no such file is in there.

security.py:396 -- a control operator glued to the program word bypassed the whole pass. Confirmed on all three spellings: ls|find ..., true;find ... and true&&find ... were ALLOWED. This was the most serious of the three: the gate was defeated by deleting one space. The program word is now read as the piece after the last control operator, alongside the substitution and redirect strippers that were already there. Five prefixes are pinned (ls|, true;, true&&, true&, echo hi|), plus the glued-pipe-AFTER case the finding did not mention (-type f|xargs cat, where the operator arrives glued to the last operand).

security.py:385 -- the whole-line composition scan over-blocked a listing. Confirmed: find <crew> -type f; cat notes | less was DENIED purely because of the sibling |. Fixed as prescribed: delivery is now read from the invocation's own token span. A | there is delivery; ;, && and & only sequence. Four sequencing shapes are pinned as allowed, with find <crew> -type f | less as the control that must still deny.

First Principles (CONCERNS) + Design Review -- the same ask, done

Both rounds asked for the residuals statement to name sibling traversal tools so the pass cannot be read as closing the traversal class. Agreed, and it is the failure mode #7034 itself warns about. The block comment now names fd/fdfind (-x/-X), locate/plocate, rg --files, du -a and grep -r, and says in as many words that this closes the deterministic find form and not the class. Filed as #7309 with the per-tool grammar problem for each, plus the note that grep -r is a different shape (no sink to detect, since the reader and the traversal are one command) and probably the higher-value one.

fd is not deferred out of convenience: its pattern is a positional regex and its exec is -x/-X, so it needs its own parser. The two reusable halves of this pass (_find_traversal_reaches_fence, _find_store_holds_match) are already tool-independent, which is what #7309 points at.

On the second First Principles concern, -regex shipping unpinned. Correct, and it was not merely unpinned but wrong, which is Opus's first finding. Six cases now exercise it in both directions.

gfind undeclared (minor). Now covered in the PR body. It is in _FIND_PROGRAM_NAMES because macOS installs GNU findutils under that name and the two share the grammar this pass parses.

GPT 5.6 -- review incomplete on the previous head

No verdict was produced for f1a430e19; that run was superseded rather than failed. Nothing to disposition and no override requested. A fresh run is in flight on b43f92ab6.

Verification on the new head

116 cases in TestFindTraversalReachesFence, up from 97. Benign corpus 61 commands / 0 false positives; carrier cross-product 400 commands / 0 misses. test_security.py + test_governance_self_protection.py + test_denied_commands_security.py: 1964 passed, 1 skipped. test_hooks.py + test_connections_tool_aliases.py + test_app_sources_write_protection.py: 416 passed. flake8, isort and mypy clean; the added code is black-clean.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ⏭️ skipped

Revision 258bea54dba29aea99ea8c3c24baba8ed392864c touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

@chenmingwei23
chenmingwei23 force-pushed the fix/bash-gate-find-exec-xargs-7034 branch from b43f92a to be815c2 Compare August 31, 2026 17:12
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 -- the undeclared frontend change is not in this PR

First Principles, "a full frontend change (#7272's content) rides in the diff undeclared, apparently from a stale base" -- the stale-base half of that reading is right, and the conclusion that follows from it is not. #7272's content was never in this PR's diff.

GitHub's own file list for the PR, at the head that was reviewed:

487+ 0-  src/kiro_crew/security.py
394+ 0-  test/test_security.py

Two files. No website/, no icon, no screenshots.

The mechanism is the one ci.yml documents on its own diff-base step -- "base.sha is the base branch tip when the PR event fired, not the branch-off point, so diffing it directly pulls in sibling commits." My branch point was c4320186e; main then merged three commits on top of it, f236a4b79 (#7272) among them. A review that diffs against main's TIP rather than the merge-base therefore sees those three sibling commits as part of this change. GitHub's PR diff is merge-base-based, which is why it stays clean while the review lane does not.

That also explains the UX round, which reviewed an icon change and passed it: there is no icon change here to pass. Both lanes were reading #7272.

Fixed at the source rather than argued: the branch is rebased onto current main (12fba75af), so it is now zero commits behind and every diff base -- merge-base, base tip, or branch point -- resolves to the same two files. Re-verified after the rebase: 1964 passed / 1 skipped on test_security.py + test_governance_self_protection.py + test_denied_commands_security.py, flake8 / isort / mypy clean, and the baselined black gate passes in scope.

One disclosure on the push itself: the local commit-message privacy hook refused the rebase because upstream commit 840aad557 (#7274, already merged and public on main) carries a developer desktop hostname in its message. My two commits have zero hits. I pushed with that hook's own audited one-time override, since re-pushing an already-public upstream commit under a branch ref discloses nothing new. No /ai-review override was used, on this or any round.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 disposition -- the BLOCKING finding was real, and it had a second half

Opus, security.py:493 -- -regex patterns are not stripped of a captured substitution's punctuation. Confirmed exactly as written, in a fake $HOME holding a real ~/.ssh/id_rsa:

ALLOW  cat $(find <home> -regex '.*/id_rsa$')
ALLOW  cat `find <home> -regex '.*/id_rsa$'`
DENY   cat $(find <home> -name id_rsa)            <- the parity control
DENY   cat $(find <home> -path '*/.ssh/id_rsa')   <- the other parity control

Two of the three filter families were stripped and -regex was not, so .*/id_rsa$) failed to compile and the read went through while the same read spelled -name was refused. That is the "every filter spelling of one read answers alike" invariant broken, and the finding named it correctly.

Fixed at the read site rather than by adding the third strip at the call site. _find_pattern_operand is now applied where each pattern is collected in the parser, so a filter family added later inherits it -- the per-list call-site strip is exactly the shape that let one family be forgotten in the first place.

The second half, which the finding did not name and which matters more. The reason a corrupt pattern was a PERMIT rather than a denial is that the matchers were collected with a filter that DROPPED whatever would not compile, while unfiltered was computed from the raw pattern lists -- so the clause ended up with neither a matcher nor the no-filter reading, and fell through to allow. That makes every malformed pattern a bypass, not just the paren spelling:

ALLOW  find <home> -regex '('        -exec cat {} +
ALLOW  find <home> -regex 'a{2,1}'   -exec cat {} +
ALLOW  find <home> -regex '*/id_rsa' -exec cat {} +

All three were allowed before this round and are denied now. An opaque pattern is read as *, which is this module's documented stance -- a maybe answers yes, since the gate may over-trigger but must never under-trigger. Had only the prescribed strip been applied, these three would still be open.

The boundary is pinned in both directions: fail-closed applies to UNCOMPILABLE, never to merely unmatched. find ~/Repos -regex '.*/package[.]json' -exec wc -l {} + and find ~/Repos -regex '.*[.]py$' | xargs wc -l still answer None.

Verification

127 cases in TestFindTraversalReachesFence, up from 116: five captured-substitution spellings including both parity controls, and five uncompilable patterns, plus the two compilable-but-unmatched controls. test_security.py + test_governance_self_protection.py + test_denied_commands_security.py: 1975 passed, 1 skipped. Benign corpus 61 commands / 0 false positives; carrier cross-product 400 / 0 misses. flake8, isort, mypy clean; added code black-clean.

No /ai-review override used on this or any round.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/bash-gate-find-exec-xargs-7034 branch from 9ca5932 to 22acd92 Compare August 31, 2026 17:42
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/bash-gate-find-exec-xargs-7034 branch from 22acd92 to bd08e22 Compare August 31, 2026 18:08
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3 disposition -- the DoS is real, and the reproducible hang is in the path the finding did not name

Opus, security.py:8382 -- an agent-controlled regex is compiled and run on the synchronous gate with no timeout. The mechanism is correct and the fix is applied: a -regex/-iregex pattern is now NEVER compiled and NEVER run. It is read as opaque, which widens the traversal instead of evaluating it.

Screening for catastrophic shapes was considered and rejected rather than skipped. This module already carries the argument against it -- enumerating dangerous punctuation does not terminate against an untrusted string -- and a hostile pattern author is exactly that case. _redos_prone exists here, but it was written to catch accidents in the repository's OWN deny patterns; using it as a boundary against a crafted pattern is a much stronger claim than it was built for. Refusing to run the pattern needs no such claim.

The half the finding did not name, which is the one that actually hangs. The prescribed fix scopes to -regex, so I measured the glob path too -- and that is where the reproducible hang is:

find ~ -name '{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}b' -exec cat {} +
  before:  *** still running after 12s ***
  after:   0.031s  DENY

_glob_to_regex maps a brace group to .*, so that pattern compiled to fourteen ADJACENT .* and wedged the gate. The finding's example patterns, by contrast, did not reproduce on this gate's subjects -- (.*)*z, (a+)+$ and (a|a)*z all answered in 0.025s, because CPython breaks a zero-width repeat and the subjects are short paths with no match to backtrack over. So the reported instance was theoretical while the unreported one was live. Both are closed now, but only because the glob path was measured rather than assumed to be covered by a regex-scoped fix.

Globs are bounded rather than refused, because they can be: adjacent .* runs are collapsed, which is semantics-preserving (.*.* names exactly what .* names), and what remains is capped at 8. That turns the pathological case linear instead of rejecting it -- {a}x14 and *x40 both collapse to a single .* and still match -- while runs separated by literals, which cannot be collapsed, are refused over the cap. Refusal returns None, which the caller reads as opaque, so the bound widens and can never become a bypass.

One property makes the cap sufficient rather than a guess: the SUBJECTS are not agent input. They are fence basenames and credential-store entry names, all short. Only the pattern is hostile.

The cost, stated plainly

A delivering -regex traversal over a root that contains a fence is now refused whatever the pattern says -- find ~ -regex '.*[.]py$' -exec grep -l foo {} + included. That is a real over-block on a rare flag, and it is the price of never running the pattern. It is bounded by the root: find ~/Repos -regex ... -exec wc -l {} + and find /tmp -regex ... -delete are unaffected, and a listing is unaffected wherever it is rooted. Two tests pin that boundary in both directions.

Verification

137 cases in TestFindTraversalReachesFence, up from 127. New: three collapsed-wildcard patterns asserted to compile to exactly one .*, three separated-wildcard patterns asserted refused, six ordinary globs asserted still compiling, the regex-widens clause with its root-bounded counterpart, and a wall-clock assertion over four previously-hanging commands with a deliberately enormous margin.

test_security.py + test_governance_self_protection.py + test_denied_commands_security.py: 1985 passed, 1 skipped. Benign corpus 61 commands / 0 false positives; carrier cross-product 400 / 0 misses. flake8, isort, mypy clean; added code black-clean. Amended into the single commit the hygiene gate wants rather than added as a fourth.

No /ai-review override used on this or any round.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 16: both lanes found the same crash in my own code, and I nearly dismissed it

Head be2043cd0, rebased onto current main (96 commits). GPT raised four and Opus one, and the one Opus raised is the same defect as GPT's fourth. Both were right.

Fixed: this pass let PathResolutionStalled escape the permission gate

_candidate_forms REFUSES rather than guessing when a canonical form cannot be established -- a wedged mount under the path raises PathResolutionStalled, and every sibling gate turns that into a denial (is_sensitive_path, _is_keystone_publish_artifact, path_contains_sensitive, _dir_holds_sensitive_leaf all catch it). This pass did not, so the exception propagated out of a synchronous gate instead of failing closed.

Worse, it was my own round-13 change that widened it. Hoisting the store resolution out of the root loop -- correct as an optimisation -- made the call eager for every command rather than only those carrying a name filter. So on a host whose home mount has stalled, a crash that needed a filtered traversal now needs any command at all.

Both _candidate_forms call sites now catch and deny. A test stubs the raise and asserts two things: this pass answers with a denial rather than raising, and the whole gate still answers rather than propagating.

How I nearly got this wrong

My first move was to grep for PathResolutionStalled in my working tree. It was absent -- along with _resolved_forms_bounded and the line numbers cited -- and _candidate_forms's docstring in my tree says the opposite of the claim ("the lexical forms are the fail-safe fallback when resolution cannot complete"). Two independent lanes citing an identical non-existent class looked like a shared hallucination, and I was drafting that rebuttal.

The class exists on kirocrew/main. My branch was 96 commits behind, so my base predated it; the reviewers judge the merge ref, which has it. Rebasing made all of it appear exactly as described, including the four sibling catch sites and the docstring Opus quoted.

Recording it because the failure mode generalises: "the symbol the reviewer cites does not exist" is not evidence of a hallucination until the branch is current. A stale base makes a correct finding unverifiable and, worse, makes it look fabricated. The check that would have caught it immediately is git rev-list --count HEAD..<remote>/main before reading any citation as false.

Not patched: three already answered

  • Opaque roots / unresolved program words fail open -- the descoped computed-operand class, #8074.
  • Quoted parentheses truncate substitution inspection -- _substitution_bodies is main's and unmodified here; every consumer inherits it. Reported in round 13, unchanged.
  • authorized_keys is missing from the credential-leaf names -- the coherence test says the gap is real (cat ~/.ssh/authorized_keys denies, find ~ -name authorized_keys allows), but the file is fenced by LOCATION and carries no credential itself, which is the category _CREDENTIAL_LEAF_NAMES deliberately excludes -- the block comment names known_hosts as exactly that case. Adding it would draw an arbitrary line between authorized_keys and known_hosts, which is the parked polarity decision. Added to bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074's coherence-gap set instead, which now reads: known_hosts, authorized_keys, trustdb.gpg, pubring.kbx.

That line is worth stating plainly, since two names went in and two did not: access_tokens.db and application_default_credentials.json ARE credentials, so the vocabulary is the right home for them. authorized_keys and known_hosts are public data whose protection comes from where they sit, so enumerating them is a polarity change, not a vocabulary addition.

And one thing this rebase resolved

data.sqlite3 now denies on BOTH routes -- main fenced the identity auth store since the last rebase. That was #8085, and it landed in the module's path vocabulary rather than in this pass's name list, which is where the coherence test said it belonged. Main's new TestIdentityAuthStoreFence even couples the traversal spelling to the direct read, which is the same invariant this PR argues for.

Verification

1532 passed / 1 skipped. 26 mutations, 26 killed, including one that lets the exception escape again. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1211 + 1535 insertions, 0 deletions, 2 files, 1 commit. The rebase conflict was again two independent classes appended at one position, resolved as a union -- and the resolver's own orientation self-check refused the first attempt because main's appended class had changed, which is what the check is for.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 17: Opus clean again; GPT's one finding is now filed as #8150

Head be2043cd0. The PathResolutionStalled crash both lanes found last round is fixed and Opus no longer blocks. Two reds remain and neither is a new defect in this change.

GPT: review incomplete, with one finding in the job log

Third consecutive head where the lane wrote no verdict. Per the rule this PR now follows, the log was read before re-running, and it held exactly one finding: a quoted or escaped ) truncates the captured traversal.

Re-measured on this head rather than re-cited, because main has moved 96 commits since the last time this was dispositioned:

ALLOW  echo "$(printf ')' ; find <fenced> -type f -exec cat {} +)"     body extracted: ["printf '"]
ALLOW  echo "$(echo ')' && find <fenced> -type f -exec cat {} +)"      body extracted: ["echo '"]
ALLOW  echo "$(printf \) ; find <fenced> -type f -exec cat {} +)"      body extracted: ["printf \\"]
DENY   echo "$(find <fenced> -type f -exec cat {} +)"                  body extracted in full

The bypass is real. It is also not this PR's: _substitution_bodies is at security.py:4188, this diff adds zero lines to it, and every consumer of that helper inherits the same truncation.

What changed is how it is being handled. It had been reported in a PR comment three times (rounds 10, 13, 17) and never tracked, which is why it kept coming back. It is now #8150, with the measurement, the fix (track quoting state while scanning for the closing paren), and the test list. That is the same route data.sqlite3 took: raised here, filed as its own issue, fixed by main in the right place.

I considered handling it at this pass's own call site and rejected it, which is worth stating since it is the obvious move. Detecting "this body looks truncated" without a quote-aware scan is not sound: an odd apostrophe count flags $(echo "it's fine"), and keying on "a quoted paren appears somewhere" over-blocks $(grep '(foo)' x). Both buy a partial fix with a real false-positive surface while the correct fix is small and exact.

Coverage Gate is derived from a cancelled shard

Backend Tests (3.12, 4) was cancelled and the coverage gate fails closed on it without downloading an artifact, so it is one red, not two.

The shard is worth describing precisely because it is not a plain failure. It reached 99%, then hung for 10.5 minutes and was cancelled at the job timeout, and the cleanup terminated an orphan pytest plus four python children -- a test that started subprocesses which never exited. Nothing in this change starts a subprocess, and test_security.py completes locally in 12 seconds.

Attribution so far: the shard was green on the two previous heads (ae4117d05, 939aa8678) and cancelled on this one, and main's own commits do not run that job, so there is no control from that side. The delta between the green head and this one is the 96-commit rebase plus two small changes in this pass. I have re-run the shard on the unchanged commit as a determinism check rather than asserting a cause; if it hangs again at 99% the next step is naming the test from the re-run's log.

Verification

Unchanged from the last push and re-confirmed on this head: 1532 passed / 1 skipped, 26/26 mutations killed, flake8 / isort / mypy / black gate clean, 1211 + 1535 insertions with 0 deletions across 2 files in 1 commit.

Nothing merged, approved or overridden. The override question remains with @chenmingwei23.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 18: the capture strip was defeating the opaque-filter mechanism

Head ed4b8d1dc. The GPT lane produced a real verdict on the re-run (attempt 2, after three heads of review incomplete) with four findings. One is a genuine defect in this change; three are already answered.

Fixed: a computed filter lost its opaque reading to my own strip

The reported command denies, so the finding looked unreproducible at first:

DENY  find <fenced> -name "$(printf .env)" -exec cat {} +

But that is the fenced root deciding on its own -- clause 1 answers before the filter matters. The filter only DECIDES when the root is not itself fenced, and there the finding is exact:

                                                    before        after
find ~ -name id_rsa -exec cat {} +                  DENY          DENY
find ~ -name "$(printf id_rsa)" -exec cat {} +      ALLOW  <--     DENY
find ~ -name credentials -exec cat {} +             DENY          DENY
find ~ -name "$(echo credentials)" -exec cat {} +   ALLOW  <--     DENY
find ~ -name .env / "$(printf .env)"                DENY / ALLOW   DENY / DENY
find ~ -name access_tokens.db / "$(echo ...)"       DENY / ALLOW   DENY / DENY

The mechanism for a computed filter was never missing: a pattern carrying an unresolved substitution sets opaque, which reads the traversal as unfiltered and fails closed. What broke it was _find_pattern_operand's capture-punctuation strip -- $(printf id_rsa) lost its closing paren, the leftover stopped looking like a substitution, and it was taken as the literal name $(printf id_rsa, matching nothing. ${LEAF} survived only because it has no trailing character to lose, which is exactly why it still denied and hid the defect:

'$(printf .env)'  -> '$(printf .env'   OPAQUE=False   <- was
'${LEAF}'         -> '${LEAF}'         OPAQUE=True
'$(printf .env)'  -> '$(printf .env)'  OPAQUE=True    <- now

This is the same shape as the pre-filter removed in round 10 and the escaped pattern in round 14: the capability existed and a text manipulation of my own defeated it. Not the descoped computed-operand class, which is about values this pass cannot compute at all.

The strip is now bounded by balance rather than applied unconditionally: a trailing ) comes off only while the token has more closers than openers, a trailing backtick only while their count is odd. An unbalanced one belongs to the outer capture; a balanced one is the token's own. That keeps the case the strip exists for (cat $(find ~ -regex '.*/id_rsa$') -> .*/id_rsa$) and stops it eating a parenthesised -regex group (.*(id_rsa|id_dsa)$) loses exactly one), both pinned by tests.

Not patched: three already answered

A note on the harness

The mutation run came back 25/26 with one ANCHOR-MISS rather than a survivor: round 14's mutation targeted the exact line this round rewrote, so it could no longer be applied. That is a harness failure, not a passing test -- an anchor that stops matching silently reduces coverage while still reporting a number. Retargeted at the escape substitution, which is what that mutation is actually about.

Verification

1537 passed / 1 skipped. 27 mutations, 27 killed, no anchor misses -- including one that restores the unconditional strip. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1233 + 1580 insertions, 0 deletions, 2 files, 1 commit.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 19: BSD pre-root options hid the root; the other four are already answered

Head 0f414a4a8. Five findings, one genuinely new and fixed.

Fixed: an unrecognised pre-root option ended the roots run

_FIND_GLOBAL_OPTS carried only GNU's -H -L -P --. BSD/macOS find takes more, and they bundle:

find [-H | -L | -P] [-EXdsx] [-f path] [path ...] [expression]

An option outside the set ends the roots walk, so the traversal was read as rooted at . and the fenced operand was never seen. Measured, twelve spellings allowed where the same command without the flag denies:

was ALLOW, now DENY   find -E <fenced> -type f -exec cat {} +
was ALLOW, now DENY   find -X / -s / -x <fenced> ...
was ALLOW, now DENY   find -EX / -Es / -dsx / -EXdsx <fenced> ...      <- the bundles
was ALLOW, now DENY   find -f <fenced> ... | find -E -f <fenced> ... | find -f <fenced> -f /tmp ...
already DENY          find -H / -L / -- / -D search / -O2 <fenced> ...

Two things worth separating. The [-EXdsx] group is matched as a character class rather than by enumerating the 31 bundle spellings, which closes the set instead of the reported example -- and it cannot swallow a primary, because every primary is a word carrying letters outside that set (-delete, -depth, -exec all fail the test, pinned by assertions).

-f is different in kind: it does not just precede the roots, it supplies one. Skipping the flag with its operand -- the obvious way to handle a flag that takes an argument -- would have discarded the very path that decides the verdict, so the operand is collected as a root. That required moving the roots list above the option run, and the first attempt raised UnboundLocalError on every -f command until it did; the probe caught it before the tests did.

Benign traversals through the same flags stay allowed (find -E . -name '*.py' -exec wc -l {} +, find -s /tmp -type f -delete, find -f /tmp -type f -exec cat {} +).

Not patched: four already answered

Verification

1548 passed / 1 skipped. 29 mutations, 29 killed, no anchor misses -- two new ones stop recognising the bundle and stop collecting -f's operand. flake8, isort, mypy and the baselined black gate clean (flake8 caught a missing blank line after the new helper before the commit). Scope against the merge base: 1268 + 1625 insertions, 0 deletions, 2 files, 1 commit.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 19 follow-up: my two new tests were wrong, and the failure proved the fix works

Head b27e57c3f. Backend Tests (3.12, 3) went red on the previous head, and it was mine -- two assertions I added, not the code they exercise.

FAILED test_a_preroot_option_over_a_benign_root_is_still_allowed
  AssertionError: find -s /tmp -type f -delete
  assert 'Blocked: ... (resolved via find:
    /tmp/pytest-of-runner/pytest-0/popen-gw0/i0/5034-kirocrew-home/workspace/md-note)' is None

The path in the denial reason is the whole explanation: the fixture places the crew home under /tmp, so in CI a traversal rooted at /tmp really does reach a fenced store, and denying it is correct. My assertions called /tmp a benign root, which is true on this machine and false on the runner. They passed locally for that reason alone.

So the failure was the new option handling working -- -s and -f now carry the root through, and the root happened to be an ancestor of the fence. Reproduced deliberately by planting a crew home under a temp tree:

crew home planted under <tmproot>
DENY   find <tmproot> -type f -exec cat {} +
DENY   find -s <tmproot> -type f -delete          <- the flag no longer hides the root
DENY   find -f <tmproot> -type f -exec cat {} +   <- the operand is collected as a root

Both assertions now root at . instead, and the reason is recorded in the test docstrings so the next person does not "fix" them back. The comment explains that denying under /tmp is correct behaviour rather than a false positive.

The lesson is about the assertion, not the flag: a directory is only outside the fence relative to where the fence is, and the fence's location is environment-dependent. Picking a "clearly benign" absolute path is an assumption about the test environment, and /tmp is the worst choice available here because that is exactly where the harness builds its home.

Nothing else changed: the BSD pre-root option set, the bundles, and -f-as-root are as pushed in the previous head.

Verification

1548 passed / 1 skipped, and the corrected assertions re-verified under a planted fence rather than only on this machine. 29 mutations, 29 killed, no anchor misses. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1268 + 1633 insertions, 0 deletions, 2 files, 1 commit.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 20: a wall-clock assertion of mine was measuring main's cost, not this pass's

Head aae422e8f. Backend Tests (Windows) (3) went red with one failure, and it was my own test:

FAILED test_the_view_walk_terminates_and_stays_cheap
  assert (1057.765 - 1047.515) < 10.0

That is seconds, not milliseconds -- the gate took 10.25 s against a 10 s budget. So not a threshold shaved too fine, but a real cost. The question was whose.

Attribution

The test bounded two shapes. Measured separately, with a tree lacking this pass as the control:

40 nested `bash -c` wrappers      this branch  17.2 ms   |  main  16.6 ms   (walk 0.4 ms, 3 views)
300 wide substitutions            this branch 2365.5 ms  |  main 2369.1 ms  (walk 10.1 ms)

The wide shape is the slow one, and it is identical with and without this pass. My pass contributes 10 ms of walk and then refuses the command at the 64-opener budget; the remaining 2.4 s is the other passes' quadratic behaviour on wide substitutions, which this change neither causes nor can guarantee. A slow Windows runner turns 2.4 s into more than 10 s, and the assertion failed on a cost that was never this PR's.

So the wall clock was the defect: a timing bound over is_sensitive_bash_command is a claim about every pass in it.

What replaced it

Deterministic assertions about the invariants the test is actually for:

  • the view walk terminates and collapses rather than enumerating a view per level (bounded view count);
  • a command carrying more substitutions than the budget is refused rather than walked (the budget comparison itself, plus the denial).

The wide input is now sized just past the budget rather than pathologically wide, since the invariant is the comparison and extra width only re-imports the other passes' cost into this test's runtime.

And a false premise in the old test, which the rewrite exposed

Asserting a verdict on the 40-layer input failed, which is correct: naive single-quote wrapping does not nest in shell. bash -c 'bash -c '...'' flattens rather than nesting, which is why the walk yields 3 views and not 40. That string is a termination stress case, not a 40-deep command, and the old test never noticed because it only measured time. No verdict is asserted on it now, and the docstring says why. Genuinely nested payloads remain covered by the tests that build a real -c payload, including a two-level bash -c "sh -c '...'".

Verification

1548 passed / 1 skipped. 29 mutations, 29 killed, no anchor misses. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1268 + 1646 insertions, 0 deletions, 2 files, 1 commit.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 21: the closable set is now empty

Head aae422e8f. 55 green, 0 pending. Opus clean on this exact head. GPT is down to two findings and both are descoped, named verbatim in this PR's own scope boundary and in #8074:

  • f{i,}nd <fenced> -type f -exec cat {} + -- brace expansion of the program word.
  • printf ... | find -files0-from - -type f -exec cat {} + -- an opaque root with no filter to bound it.

No patch, per the descope. Both need a value this pass cannot compute from the text, which is the distinction the boundary rests on.

Worth recording the trajectory, because it is the argument that the boundary is holding rather than merely being invoked. GPT's finding count across the last four settled heads went 5 -> 4 -> 5 -> 2, and every finding that was closable got closed:

round closable finding outcome
16 PathResolutionStalled escaping the gate fixed, both call sites
18 capture strip defeating the opaque-filter reading fixed, bounded by balance
19 BSD pre-root options hiding the root fixed, whole option set + -f as root
19b my own test asserting /tmp is benign fixed, root changed to .
20 my own test bounding main's cost on a wall clock fixed, asserted structurally
21 -- nothing closable remains

Three of those six were defects in my own additions rather than in the original design, which is the honest shape of this PR's later rounds.

What remains is one decision, unchanged since it was raised: the two findings above are the descoped class, so the lane cannot go green without either the polarity inversion in #8074 (its own change, with the false-positive surface priced) or an override. The override is @chenmingwei23's -- asked at 06:05Z and still open -- and I will not press it.

Verification unchanged from the last push: 1548 passed / 1 skipped, 29/29 mutations killed with no anchor misses, flake8 / isort / mypy / black gate clean, 1268 + 1646 insertions with 0 deletions across 2 files in 1 commit.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Dependency Audit / Audit Production Dependencies — read the job log, and it is neither of the two options. Posting here because the same failure is live on #7298 and the investigation should only happen once.

There is no advisory and no package. The log names no GHSA, no severity and no dependency. Both PRs fail on one line, byte-identical:

ERROR: production dependency audit failed closed: npm audit timed out after 120s for website/package-lock.json

So the gate did not find a vulnerability. npm audit never finished, and the check fails closed on a tool failure — correctly, but that is what is being reported as a dependency failure.

Neither PR introduced it, and neither PR's own dependencies are involved. #7199 touches zero lines of website/package-lock.json. What changed is on main:

main commit when (UTC) resolved packages in website/package-lock.json
1a765b88c 09-02 19:03 1028
27fd68e96 (#8041, sketch pad) 09-03 18:10 1112 (+84, +1094 lines)
b206a9f9f (#8040, social cards) 09-04 00:21 1113

The lane passed on this PR at 15:09:22Z with 1028 packages, and has failed on every run after 27fd68e96 landed: 23:28Z, 00:10Z (#7298), 00:34Z. An 84-package jump pushed npm audit --package-lock-only past AUDIT_TIMEOUT_SECONDS = 120 in scripts/check_npm_audit.py:17.

Classification: main-owned, and a tooling budget rather than a dependency. Not a new advisory on a main dependency either — that was the natural hypothesis and the log rules it out.

Two consequences worth being explicit about, because both differ from the obvious action:

  1. A re-run will not clear it. This is not a transient runner flake like the ##[error]fetch failed shard earlier tonight. The lockfile is bigger than the budget allows, deterministically, so every re-run burns ~2 minutes and fails again.
  2. A rebase will not clear it either. The lanes review the base+head merge ref, so both PRs are already auditing main's current lockfile — which is precisely why this appeared without either branch changing anything. Rebasing changes nothing about the input to the audit.

What actually fixes it has to land on main: raise AUDIT_TIMEOUT_SECONDS, cache the audit, or trim what #8041 added. Until then this lane is red on every PR that runs it, and neither PR should touch dependencies or add a .vulnerability-exceptions.json entry — there is no vulnerability to except, and an exception entry would suppress a gate that is working correctly while hiding the real problem.

I have not changed anything in #7199 for this and will not.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • This PR is OVERLAPPING with PR #7441. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7298: CONTINUE_DEVELOPMENT. Complementary sibling pass already merged and already in this PR's base; it covers every traversal program EXCEPT find, so it leaves 7298's entire goal open. Files: src/kiro_crew/security.py.
  • This PR is OVERLAPPING with PR #7913. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7298: CONTINUE_DEVELOPMENT. Independent behaviour, shared insertion point in the same function and shared cost machinery; coordinate ordering rather than treating either as redundant. Files: src/kiro_crew/security.py.
  • This PR is OVERLAPPING with PR #8099. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7298: CONTINUE_DEVELOPMENT. Merged fence change whose live parity test this PR is expected to flip; the parity holds, and the only new question is a documented-class over-trigger worth naming in the description. Files: test/test_security.py, src/kiro_crew/security.py.
  • This PR is OVERLAPPING with PR #8282. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7298: CONTINUE_DEVELOPMENT. Same function, same liveness premise, non-overlapping bounds; worth coordinating landing order and confirming neither ceiling is presented as covering the other's case. Files: src/kiro_crew/security.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 24: two real findings, recovered from a lane that reported no verdict

Head 0007894b7. The previous head reached 54 green with GPT 5.6 the only red, and that red was review incomplete -- no verdict, not a finding. Reading the job log first (rather than re-running straight away) is what surfaced this: pass 2 had completed and published [GPT-REVIEWED] with [BLOCK-MERGE], and the gate discarded it because pass 1 never returned. Three findings were sitting in the log. Two are real and fixed here; one is the parked set.

1. Brace-expanded roots evaded the fence -- fixed, whole class

find <fenced-parent>/cre{w,w} -type f -exec cat {} + was allowed while its brace-free twin denied. The root names no fenced path as written; the shell hands find the fenced directory anyway.

This is not the globbed-root case parked in #8074, and the distinction is the whole reason it is in scope. A glob cannot be enumerated without asking the filesystem what exists. Brace expansion is finite and decided by the text alone -- same family as the quoting, backslash-escape and capture-strip gaps closed in earlier rounds. So the set closes rather than being sampled. Measured before the fix, all six spellings allowed where the plain root denied:

spelling (leaf shown as crew) before after
{crew,crew} ALLOW DENY
{crew,zzz} / {zzz,crew} ALLOW DENY
a group splitting the leaf, <c><r>{ew,x} ALLOW DENY
the same nested one level deeper ALLOW DENY
crew{,x} / crew{x,} (empty alternative) ALLOW DENY

_find_brace_expansions enumerates them as additional READINGS of the root, on the same footing as the literal spelling -- additive, so a wrong reading can produce a denial but never withdraw one. Expansion is multiplicative ({a,b} x 8 is 256 forms), so _FIND_BRACE_BUDGET = 32 caps it and fails closed: an expansion too wide to enumerate is not thereby known to name nothing fenced.

2. Root resolution was unbounded -- fixed, and the premise measured first

Each root costs a filesystem resolution per candidate form, and nothing upstream bounds how many roots a find names. Main is the control:

roots main this branch (before) delta after
100 3.4 ms 61.9 ms +58.4 ms +9.3 ms
500 31.0 ms 290.8 ms +259.7 ms +11.5 ms
1000 97.7 ms 607.6 ms +510.0 ms +17.2 ms
2000 398.4 ms 1390.4 ms +992.1 ms +25.0 ms

_FIND_ROOT_BUDGET = 64, matching _FIND_SUBSTITUTION_BUDGET deliberately -- both cap a dimension the text can inflate without limit, and both fail closed. The bound is applied to the READINGS rather than the parsed roots, because the readings are what get resolved; bounding the parsed roots alone would have left the real work unbounded.

One measurement corrected itself here and is worth recording. With 500 roots all naming the fenced directory the branch still read 726 ms after the fix, which looked like the bound failing. It was not: main costs 706.9 ms on that same command and the branch 738.5 ms, so the residual is main's own scan of a long line, and the branch's marginal cost is +31.6 ms. The first probe simply had no control for that case.

3. -files0-from opaque roots -- unchanged, parked

Still #8074. Needs a value the text cannot supply, which is the boundary this PR states.

Verification

1806 passed / 1 skipped. 33/33 mutations killed, zero anchor misses -- up from 29, with four new mutations covering brace expansion and both budgets, each pinning the fail-closed direction. Scope 1421 + 1711 insertions, 0 deletions, 2 files, 1 commit. flake8 / isort / mypy / black gate clean.

Two mutation anchors went stale in this round because inserting the root bound split the line they pointed at. Both printed ANCHOR-MISS and were repaired; without that check the run would have reported a healthy number while silently testing nothing at those two points.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 25: two more real findings, and a scope discipline that changed the fix

Head 38650a912. Round 24's two fixes are confirmed landed -- the brace-root and root-count findings are gone from GPT's output. It produced a complete verdict this time (the previous head's review incomplete had hidden completed findings in the job log) and reported two new ones. Both are real. Both are fixed.

1. NAME+= built a program word this pass never saw

F=fi; F+=nd; $F <fenced> -type f -exec cat {} + was allowed, while F=fin; ${F}d ... -- closed in an earlier round -- denied. The mechanism was present; only the += spelling escaped it, which is the shape this PR keeps hitting. Seven spellings did it, all now denied:

spelling before
F=fi; F+=nd; $F ALLOW
F=fi; F+=nd; ${F} ALLOW
F=; F+=find; $F (empty initial value) ALLOW
F=f; F+=i; F+=nd; $F (repeated append) ALLOW
F=fi; F+=n; ${F}d (append then split use) ALLOW
F=fi; F+='nd'; $F (quoted tail) ALLOW
F=fi ; F+=nd ; $F (spaced operators) ALLOW

An append tail is a LITERAL, so what the shell runs is decided by the text -- the same reason quoting, backslash escaping and brace expansion were in scope, and the same reason $(printf find) is not.

2. accessTokens.json -- added, after both premises were measured

The Azure CLI's bearer-token cache. This vocabulary requires two things before a name goes in, and both were checked rather than assumed:

Stored case-folded, so the accessTokens.json the CLI actually writes is covered.

The fix I wrote first was the wrong shape

Worth recording, because the better-looking change was the wrong one for this PR. Main already solves this exact problem on the path-tracking side with _SHELL_ASSIGN_RE = ...([A-Za-z_][A-Za-z0-9_]*)(\+?)=(.*), carrying the append form as an optional group -- and its docstring describes the same failure mode. So the tidy fix was to give _LOCAL_ASSIGN_RE the same optional group.

That worked and all 1816 tests passed, but the numstat told on it: 13 deletions, every one of them group-renumbering churn across three call sites that had nothing to do with this bug (the alias resolver and the payload scan both read group(2) as the value). This PR has been insertions-only for 24 rounds, and widening its blast radius into unrelated passes to save a regex is a bad trade on a change that is already large and hard to land.

So the append branch REUSES _SHELL_ASSIGN_RE instead of redefining anything -- one spelling of the rule in the module, matched only where the assignment pattern rejects the token. Same behaviour, 0 deletions, nothing else touched.

Verification

1816 passed / 1 skipped in test_security.py, plus 619 passed in test_denied_commands_security.py -- run because the append branch reuses a pattern shared with the alias and kill-by-name passes, and a regex shared across passes cannot be verified from one file. 36/36 mutations killed, zero anchor misses, up from 33: three added here, two of which had to be re-anchored after the rework above, which is exactly the failure the anchor check exists to catch. Scope 1472 + 1780 insertions, 0 deletions, 2 files, 1 commit. flake8 / isort / mypy / black gate clean.

A false-positive guard is pinned alongside the fix: appends that never spell a traversal (F=he; F+=llo), an append spelling a DIFFERENT program (P=fin; P+=ger), and an append used as an argument rather than as the program word all still allow.

Nothing merged, approved or overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Correcting my own note above, because following it would leave people stuck: the fix has landed on main, and a rerun-failed-jobs will never pick it up.

e786b086d (#8362, 2026-09-04 04:34Z) raised AUDIT_TIMEOUT_SECONDS to 180, added a retry for connection-level faults while keeping a real finding non-retryable, and moved the gate to workflow_call only — so it no longer runs on pull_request at all.

Why my PR is still red, and why re-running cannot fix it. I re-ran the audit on #7199 at 11:30Z, five hours after that commit landed. It failed with the old message, timed out after 120s. Both runs resolved the reusable workflow at the identical merge-ref commit:

05:11Z run   dependency-vulnerability.yml@refs/pull/7199/merge (9be1a638f3b4…)
11:30Z rerun  dependency-vulnerability.yml@refs/pull/7199/merge (9be1a638f3b4…)

refs/pull/<n>/merge is computed lazily — it was last recomputed at my 02:0xZ push, before the fix — and rerun-failed-jobs replays that same pinned SHA. So a rerun re-executes the pre-fix workflow no matter how long you wait.

Practical consequence, which reverses half of what I wrote earlier: I said neither a re-run nor a rebase clears this. The re-run half is now doubly confirmed and I can name the mechanism. The other half was wrong — any new push clears it, because that recomputes the merge ref against main's current tip, at which point the gate is workflow_call-only and simply does not run on the PR.

So the remaining red on a PR that has not pushed since ~04:34Z is a stale artifact of a workflow that no longer exists in this form, not a live signal. Nobody needs to re-run anything; the next push retires it. I am leaving #7199's red in place rather than pushing a no-op commit to refresh CI.

Thanks for the retry-and-budget work — worth noting your first suggestion turned out to be the right one for the electron-sized case, and the release-only move handles the large lockfile that a retry could not.

The bash gate asks one question of every token: does this resolve to a fenced
path? A `find` traversal answers no by construction, because it factors the path
in two -- the directory in one argument, the leaf in another -- and produces the
path itself at runtime:

    find ~/.kiro/crew -name '.env' -exec cat {} +
    find ~ -name credentials | xargs cat

A new pass re-joins the halves. It judges only a traversal that DELIVERS its
matches -- an action primary, a pipe, a redirect, or a substitution that captures
the output -- and denies it where the fence supplies the missing half: the root is
a credential directory, a filter matches a basename the fence declares, or a
filter resolves to a file that is really sitting in a fenced store.

The inert primaries are an allow-list, so a primary nobody enumerated denies
rather than permits -- the polarity _TRUST_ROOT_READ_LISTERS already documents.
Nothing here knows the child command, which is why xargs needs no flag grammar of
its own.

WHICH TEXT to judge is its own question, and answering it by inspecting the
command's characters was wrong twice. Capture was read off the two opener
characters glued to the program word's token, so `cat $( find ... )` was allowed
where `cat $(find ... )` was denied -- the same read, one space apart. And a
`bash -c '<traversal>'` payload was never re-tokenized at all, so the traversal
was invisible, even though the argv floor already descends into payloads when
they carry a plain fenced path. Both are one defect: the pass judged the
command's text rather than the things the shell runs. `_find_traversal_views`
enumerates those instead -- the outer line, every substitution body, every nested
payload -- so capture stops being a spelling to detect and becomes a property of
how a view was DERIVED, and no depth of wrapping is a special case. It reuses the
module's own view idiom and its termination discipline: no depth cap, because
whatever number is chosen one more level defeats it; a view is a proper substring
of its parent and a visited set stops sibling wrappers re-walking the same text.

The same reframing settles the program word. A shell expands `f?nd` against the
filesystem before running it, which no substring test on the command text can
see, so the word is matched through the pattern machinery instead -- and a word
the wildcard bound refuses is read AS a match, so the bound widens the pass
rather than opening a hole in it. `-files0-from` is the mirror image on the other
operand: find reads its roots from a file or from stdin, so the command names no
root and a parse reading only operands defaulted to `.`. It is read as an
unknowable root, which lets the filter still decide. And a program word or root
held in a variable the SAME command assigns is resolved through
`_resolve_local_assignments`, which the plain-path passes already consumed -- so
`D=<fenced>; cat $D/.env` was denied while `D=<fenced>; find "$D" -type f` was
allowed. That asymmetry was again machinery this pass had not been wired into.
Resolving it also SEPARATES two cases an unresolved-root reading conflated: an
assigned root is judged as the real path, while a genuinely unassigned `$SRC`
stays unknowable and is left to the filter.

A filter is agent-supplied text and this gate is synchronous, so evaluating one is
a denial-of-service surface before it is a correctness question. A `-regex`
pattern is never compiled and never run; a glob is bounded instead, by collapsing
adjacent `.*` runs and capping what remains. A filter or root carrying an
expansion the command never assigned is read as opaque. All of these fail closed
by widening the traversal rather than dropping the filter.

Whether a traversal RESOLVES a credential path is decided from the NAME it asks
for, with no filesystem access. Two earlier revisions asked the store instead and
each way of asking was its own defect: a direct-child `os.path.exists` could not
see `~/.ssh/archive/id_rsa` while denying the same name at the top, and an
`os.listdir` per store to match a glob measured 110 listdir calls and 14.4ms on
one traversal, against 0 and 0.7ms for an ordinary fenced read. Those two point in
opposite directions -- probe deeper, stop enumerating -- so neither is reachable by
extending the probe. Deciding from the name removes the depth question and the
enumeration together, and drops a host-dependence that was never a feature, where
the same command was allowed or denied according to whether a store happened to
exist yet. It is strictly wider than a probe, which is the fail-closed direction,
and what bounds it is a conservative predicate listing known credential leaves and
suffixes, nothing pattern-like -- the companion to `DENIED_ROOT_PARTS`, which
answers about a directory rather than a name. It is private: it has one consumer,
and a public spelling would exist only for a migration this change does not do.
A curated list does mean an OMISSION allows, which is the one decision here whose
polarity is not "anything unenumerated denies"; the alternative -- classifying
pure stores and denying any filter over one -- denies `find ~ -name '*.py'`
whenever `.ssh` exists. Neither is dominant, so the trade is recorded, not settled. Every filter spelling of one read answers alike -- `-name id_rsa`, `-name 'id_*'`,
`-path '*/id_rsa'` and `-path '*/id_*'` -- an invariant an earlier round
established and which each revision here had to be re-checked against. A `-path`
pattern is matched WHOLE against a synthetic fenced path rather than by its leaf,
because its earlier segments constrain what it can name: a trailing wildcard
matches any leaf yet `*/node_modules/*` can never name a store entry.

A filter bounds the delivery only where find will run the delivery after it and only
where it holds, so POSITION and the boolean operators decide the bound rather than
mere presence. `find ~ -exec cat {} + -name '*.py'` cats every file under the home
directory, and a disjunction, a negation or a comma each detach the filter from the
delivery; all of them were read as if the filter narrowed it. Establishing which
filters really bind would need find's own precedence grammar, so the traversal is
read as UNFILTERED instead -- no grammar, and it fails closed. An explicit `-a`
binds exactly as the implicit one does, and a `-prune -o ... -print` idiom is
untouched because it only lists.

There is no cheap pre-filter on the pass. A substring test for the program name has
to survive every rewriting the shell performs between the text and the program it
runs, and the one that stood here modelled quote removal and globs but not
parameter expansion, so `F=fin; ${F}d <fenced>` returned before the pass ran --
while the pass it guarded resolves exactly that spelling. Any pre-filter cheap
enough to be worth having is one spelling away from being the hole, so the pass
runs unconditionally and decides from the resolved program word. Measured at
45-190 us against the 136-844 us the surrounding checks already spend.

A redirect placed BEFORE the operands is delivery, not a root. Redirect spellings
carrying an `&` (`&>`, `&>>`, `>&`) also match the control-operator break, which
ran first in the primaries loop while the roots loop ordered the two the other way
-- so the pass disagreed with itself about the same redirect. Ordering the tests
consistently covers the operator set rather than one spelling. The roots loop
collected `>` and its target as traversal roots, so `find >out <fenced> -type f`
was allowed while `find <fenced> -type f >out` denied -- the same write with the
redirect moved to the front. Nothing there is expanded; the tokens were simply
classified in the wrong role, which is why it is a parse-order defect rather than
another spelling of the computed-operand class.

The `-path` probe is matched in both separator spellings, because the pattern uses
find's forward slashes while the join uses the platform's -- on Windows the clause
was inert. The separator is a parameter rather than read from `os.sep`, so the
behaviour is reachable from a test on either platform: taken from the global it was
unfalsifiable off Windows, and a mutation deleting the second spelling survived
every local test.

The tests interpolate every temp path into a command as POSIX. A backslash is an
escape to `shlex`, so a Windows temp root lost every separator and the gate parsed
a directory the test had not created -- 27 tests on the Windows shard, red from the
first revision. `find /` is likewise a POSIX-only literal, and the absolute-root case is computed from
the ANCHOR of the home the fence is anchored on -- deriving it from the working
directory's drive aimed the traversal at one drive while the fence sat on another.

The widened readings for an unknowable root are gated on there being a filter left
to reject them, because widening both at once denied an ordinary
`find "$TMPDIR" -type f -delete` outright.

The residuals are named where someone changing this code will read them: a pattern
held in a variable, a shell loop, a root reached by a preceding `cd` rather than
named (the working-directory emulation this module deliberately does not rest
security on), a leaf fenced only by LOCATION whose own name carries no credential
signal (`known_hosts`), an unknowable root with no filter left to decide, and the
sibling traversal tools (`fd`, `locate`, `rg --files`, `du -a`, `grep -r`) filed as
their own issue. Two leaf modules carry their own copy of the credential-filename
list and both already import `DENIED_ROOT_PARTS` from here, so they can be pointed
at the new constant; that migration is left out deliberately, because it moves the
behaviour of two unrelated surfaces.

SCOPE. This pass recognises a traversal by its SPELLING, and that is the whole of what
it claims. An operand the shell COMPUTES is deliberately NOT covered:

    $(printf find) ~/.kiro/crew -type f -exec cat {} +

Knowing that runs `find` means knowing what `printf` writes, which is not a property of
the command text. The same holds for a glob-bearing root (`~/.kir*/crew`), brace
expansion, a root supplied from outside the command line (`-files0-from -`), a genuinely
unassigned expansion, and a root reached by a preceding `cd`. Six review rounds measured
17 such spellings, and each one closed revealed others, because the set of programs whose
output is `find` is not enumerable by a pattern over the text. Issue #8074 carries the
argument and the fix -- invert the polarity, require literal operands, fail closed on
computed ones -- which is a behaviour change on a security gate with its own
false-positive surface to price, so it is not folded in here.

What this pass DOES resolve is what the text alone determines: a same-command assignment
(`F=find; $F`, and the split `F=fin; ${F}d`), a glob-expanded program word (`f?nd`), a
quote splice, a nested `-c` payload, and the body of a substitution -- each of which turns
a computed spelling back into a literal one.

The credential-name vocabulary is a documented boundary too, not a gap awaiting entries:
it is the one clause whose polarity lets an OMISSION allow, and a private key the user
named themselves (`github_work`) is outside any curated list. Both ways to invert it were
measured and both cost real false positives; the choice is tracked in #8074.

Fixes #7034
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Grounds for the /ai-review override gpt issued on 258bea54d

Recorded before the override so the reasoning is checkable independently of it. Written to be judged by someone reading it months from now: every claim below is verifiable from this repository or from the linked issue, not from this thread.

What the blocking verdict on this head actually consists of

Two findings, both complete ([GPT-REVIEWED] 258bea54d):

  1. find ~ -name deploy_key_prod -exec cat {} + -- a credential file whose NAME is not in the vocabulary. The proposed fix is to fail closed for any literal filter that could select an entry beneath a credential-store directory.
  2. -files0-from -, glob roots, or $(printf find) -- roots or a program word this pass cannot resolve from the text. The proposed fix is to refuse any delivering traversal whose program or roots cannot be statically proven outside every fence.

Both are the class this PR formally does not cover. Neither is a defect in the changed lines.

1. The blocked class is documented as out of scope, with an existence proof

#8074 carries it. The argument is not "a few more spellings remain": for any recognizer that enumerates N spellings of the program word, a substitution exists that produces find and is not among the N. Knowing that $(printf find) runs find requires knowing what printf writes, which is not a property of the command text. A spelling-based recognizer over that class does not terminate, and $(printf find) is the witness.

The issue also records the measured progression -- distinct computed-operand spellings on the record after each round: 6 after round 5, 10 after round 7, 13 after round 9, 17 after round 12. Closing one revealed others, every time.

Finding 2 above is that class verbatim. Finding 1 is the vocabulary's POLARITY, which #8074 also carries: it proposes inverting the polarity so a literal-name filter under a fence denies by default. That change was priced -- 12 of 12 benign literal-name searches flip to DENY -- which is why it is its own change with its own review rather than a patch here.

2. The two reviewers reach opposite conclusions on the same code path

This is the strongest ground and it is checkable in the tree.

_find_root_readings widens an unresolved root into fail-closed readings. Opus required one of those readings be WITHDRAWN as a false positive: the bare-home reading matched every fence and denied find "$TMPDIR" -type f -delete and find "$SRC" -type f -exec cat {} + outright. That withdrawal is recorded in the function's own docstring, which states the reading "contradict[ed] this docstring (found in review)", and a mutation pins it so the false positive cannot return.

GPT's finding 2 demands the opposite on the same path: refuse whenever roots cannot be statically proven outside every fence -- which is precisely the widened reading Opus had removed.

A blocking verdict constituted by an unresolved disagreement between two reviewers, on one code path, is not a basis for indefinite blocking. Resolving it is a policy decision about false-positive tolerance on a security gate, which is what #8074 exists to decide.

3. The PR claims only what it does

Section 3 of the description leads with the boundary and names the uncovered computed-operand class, pointing at #8074; section 5 explains why patching stopped. A test pins both sides of the boundary. An audit script was run against the description and the code specifically to find claims that overstate behaviour -- it caught one in a code comment of mine (-files0-from described as "closed by construction" on a path that measurably allowed) and that wording was corrected.

What this override does NOT do

It clears one automated gate. It is not an approval and not a merge. bolichen97's CHANGES_REQUESTED of 2026-09-02 still stands on this PR (against commit 62b323d6e, now many heads stale), and only he can clear it, so mergeable_state remains blocked after this override. That is the expected outcome, not a failed override.

Verification on this head

1836 passed / 1 skipped in test_security.py, 619 passed in test_denied_commands_security.py (run because the append branch reuses a pattern shared with other passes). 40/40 mutations killed, zero anchor misses. 1618 + 1868 insertions, 0 deletions, 2 files, 1 commit. flake8 / isort / mypy / black gate clean.

Authorisation: the decision was delegated by @chenmingwei23 and approved by the requester at 12:35Z today, after two earlier heads were checked and found to carry closable defects instead -- a RecursionError this pass introduced, and two gaps in its brace handling. Both were fixed rather than overridden. This head is the first whose blocking set is only the descoped class.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt 258bea5: Both blocking findings are the computed-operand and name-vocabulary-polarity class that this PR formally excludes and #8074 tracks with an existence proof, and the second contradicts a false-positive withdrawal Opus required on the same code path; grounds recorded in full at issuecomment-5541252977.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 258bea54dba29aea99ea8c3c24baba8ed392864c.

Both blocking findings are the computed-operand and name-vocabulary-polarity class that this PR formally excludes and #8074 tracks with an existence proof, and the second contradicts a false-positive withdrawal Opus required on the same code path; grounds recorded in full at issuecomment-5541252977.

This decision applies only to this commit. A new push requires a new judgment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bash gate misses a fenced path reached through find -exec or xargs

3 participants