fix(security): stop scanning cron script bodies with the shell command gate by iamwhatever · Pull Request #9082 · kirodotdev/KiroCrew

iamwhatever · 2026-09-06T17:51:49Z

Problem / Motivation

Cron script jobs get refused on every fire for their documentation, their length, or a regex that redacts a credential path. Our own auto-pipeline crons were hit. Each refusal names a credential read that does not exist.

The cause is one wiring decision: mcp_cron._vet_script_contents hands the Python source body to security.is_sensitive_bash_command, whose every pass reads its subject as a shell command line.

Why it matters

Each shell-grammar pass added to the gate since #4243 turned into a new class of permanent false denial on ordinary scripts:

separator collapse read a re escape as a path (cron script body scanned with a shell heuristic: separator-run collapse denies a benign Python source body forever #7912, fixed by fix: scope the separator-run collapse to shell-grammar subjects #7913)
the traversal passes counted every line as a pipeline stage; any ~512-line script hit the budget (cron script body scanned as one shell command line: stage-budget refusal permanently blocks every ~512+ line script cron #8563, fixed by fix(security): point the traversal passes at a source body's command strings #8550)
the env rules assembled an env | grep pipeline from an os.environ read, a | in a regex literal and a comment hundreds of lines apart (cron script body scanned as one shell command line: stage-budget refusal permanently blocks every ~512+ line script cron #8563)
the find pass parsed a docstring that starts with "Find …" as a command line (Docstrings drawn into #8550's traversal subjects re-block real cron scripts #8643, fixed by fix(security): keep docstrings out of a source body's traversal subjects (#8643) #8811)

Each fix added another layer of AST analysis in security.py. That layer reached ~1500 lines and still cannot stop open(os.environ["LOCALAPPDATA"] + r"\kiro-cli\config.json") or open("~/" + ".a" + "ws/credentials") (verified on current main). Static text analysis of a Turing-complete body is not a fence. The sandbox is.

What changed (motivation → approach → change)

Symptom: a script is refused for prose or length. Root cause: a document is handed to a command-line matcher. Fix: stop handing it over.

_vet_script_contents now runs only whole-body, source-aware, linear detectors: a credential-path spelling anywhere, a protected secret env var by $NAME or bare name, an exfil URL, under its own 256 KiB ceiling. It does not call is_denied or is_sensitive_bash_command.

is_sensitive_bash_command is back to (command, *, enabled_ids). Deleted from security.py: the _subject_is_shell_grammar, _traversal_subjects, _env_subject, _max_chars knobs; is_sensitive_source_body; _source_command_subjects; _sensitive_run_in_source_literals; the re-authenticity guards (_re_module_is_authentic, _pattern_reextracted, _enclosing_call_slot, _compiled_name_escapes, …); the docstring-reflection audit (_docstring_constant_ids, _reads_dunder_doc, _DOC_REFLECTION_*). Net: security.py 22,607 → 21,240 lines; import ast is gone from it.

The runtime control for what a script may open is unchanged and already there: run_script spawns it under wrap_argv(mode="standard"), which bind-masks the crew home's credential leaves, the vault and the keystone; a secret-granted run uses strict.

Regression guards, so this does not creep back:

AGENTS.md gets two security invariants: a cron script body is never a shell-gate subject, and a regex spelling-chase is a review smell.
.github/review-prompts/gpt-repo-context.md and opus-validate.md tell both AI lanes not to propose "spelling Y also reaches the fence" against a change that narrows a text matcher when the sandbox masks the path, and not to propose handing a document to the shell gate. Rounds 1–4 of fix(security): gate traversals that reach a fence without find #7441 were exactly that loop.

Tests

test_the_shell_gate_has_no_source_body_entry_point (liveness): pins the is_sensitive_bash_command signature and asserts none of the removed source-body symbols exist.
test_script_body_is_never_a_shell_gate_subject (cron): monkeypatches every shell matcher to raise, then vets the benign and malicious corpora; a re-coupling fails it.
test_vet_script_contents_allows_source_that_only_names_a_fenced_store: eight bodies, one per false-denial class above (redactor, keyword pattern=, prose docstring naming the store, "Find …" docstring with 40 code spans, 700-line script, os.environ + | + comment far apart). All refused at some point on main; all allowed now.
test_vet_shell_command_still_blocks_a_separator_run: the shell path keeps pass 1b (Repeated path separator bypasses the Windows kiro-cli credential fence #6350 control).
test_the_source_body_ceiling_is_larger_and_owned_by_the_cron_reader: the 256 KiB ceiling still refuses, and reader and gate share one constant.
Deleted: test/test_security_source_command_subject.py (574 lines) and 40 tests in test_mcp_cron_security.py that pinned the removed AST layer. Their attack corpus (ATTACK_SCRIPTS_WITH_A_SEPARATOR_RUN) is gone with it: those 44 bodies are stopped by the sandbox mask, not by text, and the same bodies with + "" inserted were never stopped by text.

3464 passed across test_security*.py, test_denied_commands_security.py, test_llm_helpers*.py, test_mcp_cron*.py, test_cron_script.py. Wider -k "security or cron or sensitive or denied or llm_helpers or sandbox or hooks": 10007 passed, 12 failed, all in test_file_explorer_app.py and test_host_isolation_floor.py, which fail identically on clean origin/main in this environment.

Manual verification

Ran _vet_script_contents over the six assembled-path bypass bodies above on pristine main: all six allowed, confirming the removed layer was not the fence for them. flake8, isort, mypy, docs_lint and the black baseline gate are clean.

Related Issues

Closes #8563. Related: #8812 (llm_helpers._first_tool_input_denial is a separate entry point; a file write's content still goes through the shell matcher there, so an agent may still be refused writing a script cron can now run. Left for its own PR).

Pattern harvest

Rule candidate: agents-md, review-prompt
Pattern: a document (source body, file content, script) handed to a command-line matcher; each new shell pass becomes a new false-denial class, and the review loop proposes table entries instead of asking what the subject is.

Checklist

At most two commits (one is the norm), with a Conventional Commits title
Existing tests pass and new tests added for new functionality
Self-review completed; code follows project style guidelines
Documentation updated (security.md, learn-cron-dashboard.md, AGENTS.md)
No secrets, credentials, or internal references in the diff

…d gate A cron script body is Python source. Since #4243 it was handed to is_sensitive_bash_command, whose every pass reads its subject as a shell command line. Each shell-grammar pass added since then produced a new class of permanent false denial on ordinary scripts: separator collapse read regex escapes as paths (#7912), the traversal passes counted every line as a pipeline stage and refused any ~512-line script (#8563), the env rules assembled an `env | grep` pipeline from fragments hundreds of lines apart (#8563), and the find pass parsed a docstring starting with "Find ..." as a command line (#8643). Each was patched with another layer of AST analysis in security.py -- ~1500 lines -- that still could not stop `open(os.environ["LOCALAPPDATA"] + r"\kiro-cli\config.json")`. This removes that layer. `_vet_script_contents` now runs only whole-body, source-aware, linear detectors (credential path, secret env name, exfil URL) under its own size ceiling. `is_sensitive_bash_command` is back to `(command, *, enabled_ids)`; the `_subject_is_shell_grammar`, `_traversal_subjects`, `_env_subject` and `_max_chars` knobs, `is_sensitive_source_body`, `_source_command_subjects`, `_sensitive_run_in_source_literals`, the `re`-authenticity guards and the docstring-reflection audit are deleted. The runtime control for what a script may open is the sandbox `run_script` spawns it in. Two ratchet tests pin the decoupling (shell gate signature, no source-body entry point, the cron gate never reaches a shell matcher). AGENTS.md and the reviewer prompts record the invariant so review rounds stop proposing "one more spelling" table entries against a document subject. Closes #8563. Related: #8812 (the tool-input scan in llm_helpers is a separate entry point and is not changed here).

github-actions · 2026-09-06T17:55:55Z

Design Review (Fable 5) — 🟡 CONCERNS

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

Reviewed the full diff (mcp_cron.py rewiring, ~1500 lines of AST scaffolding deleted from security.py, new liveness/pin tests, AGENTS.md invariants, review-prompt additions) against the PR's stated intent.

Design-Verdict: CONCERNS

"The sandbox is the fence" is the load-bearing premise, and it is false on Windows — where the deleted separator-collapse pass was aimed in the first place.

Watch

The new design's second tier does not exist on Windows: sandbox.py states "Kiro Crew has no native Windows sandbox backend" and warns "no OS-level sandbox". The deleted literal-scan existed precisely for Win32 separator semantics; meanwhile _CRON_CRED_PATH_RE anchors on /, whitespace, and quotes, so a body containing a plainly-spelled \.aws\credentials or \kiro-cli\config.json (backslash separators) now vets clean → registers → runs with no mask underneath. Main refused that literal spelling; this PR allows it. The adversarial case (open(a + b)) was already unstoppable on main, so the marginal loss is only the literal-spelling case — but it is the keystone boundary, and neither the docstring, security.md, nor the new review-prompt text ("when the OS sandbox already masks the path") states the Windows caveat, so future reviewers following those prompts will assume a tier that isn't there.

Suggestions

Make _CRON_CRED_PATH_RE separator-agnostic ([/\\]+ plus a \\ prefix class) — a whole-body detector tweak fully inside this PR's design, restoring the literal-spelling refusal without any shell grammar.
State the Windows no-sandbox caveat explicitly in the _vet_script_contents docstring, security.md, and the two review prompts, so the "cite the mask that fails" instruction is answerable.

[DESIGN-REVIEWED] 56ca95c

github-actions · 2026-09-06T17:57:34Z

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for 56ca95c9d58669469a14481055bdae7218616833; 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 56ca95c9d58669469a14481055bdae7218616833: <one-sentence reason>

github-actions · 2026-09-06T17:57:35Z

Opus 4.8 Review — ✅ no blocking findings

Reviewed 56ca95c9d58669469a14481055bdae7218616833 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 56ca95c

Verdict parsed from the review's SHA-scoped output markers for commit 56ca95c9d58669469a14481055bdae7218616833.

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

github-actions · 2026-09-06T17:58:28Z

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 56ca95c9d58669469a14481055bdae7218616833 — 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.

First-Principles-Verdict: CONCERNS

The fix sits at cause level and deletes ~2,700 lines, but it also ships instructions that dull the automated reviewers toward every future text-matcher removal, not just this one.

What this change ships

Intent: stop cron script jobs from being permanently refused for their prose, length, or redaction regexes — a FIX.

A script cron refused for a docstring/length/regex now runs — justified, cause-level (the wrong subject is unhanded, not re-patched)
is_sensitive_source_body + ~1,400-line AST layer deleted — justified; 0 remaining consumers (grepped is_sensitive_source_body in src: only mcp_cron called it)
Shell-gate signature reverted; 4 internal knobs deleted, each with exactly 1 consumer (the deleted layer) — justified
256 KiB body ceiling moved into _vet_script_contents, same shared constant — justified move
Doubled-separator bodies no longer text-refused; sandbox mask is the control — declared, evidenced by the verified-on-main open(a + b) bypass
Two new AGENTS.md invariants — rides along, declared regression guard with named history (fix(security): credential paths were readable by respelling them with a variable or cd #4243→fix(security): keep docstrings out of a source body's traversal subjects (#8643) #8811)
GPT + Opus-validate lanes told not to raise matcher-narrowing findings — rides along, declared, broader than the case decided here
security.md / learn-cron-dashboard.md rewritten — mandated same-commit sync, one stale clause left
Ratchet tests pin the decoupling; ~1,900 test lines deleted — justified

Watch

Item 7's rule covers "a change that narrows or removes a text matcher" — ANY matcher, forever — shipped inside the PR that removes one. The named cause (fix(security): gate traversals that reach a fence without find #7441's four rounds) supports the document-subject case; the general form extends this PR's verdict to future changes nobody has examined. A human should confirm that breadth deliberately.
Sibling count for "document handed to the shell matcher": llm_helpers.py:995 (declared, deferred to Docstring prose still refused by the tool-input scan (_first_tool_input_denial) — same class as #8643, different entry point #8812) and computer_use/policy.py:265 (documented accepted trade) — declared/recorded, nothing hidden.

Subtractions

docs/system-specs/modules/security.md:155 still says the shell caller "opts out of it (value_already_scanned=True)" — that parameter is deleted (grepped value_already_scanned: 1 hit, this doc line, 0 in code; the behavior is now hard-coded in _fence_hit_in_collapsed). Delete the clause from the bullet this PR already rewrote.
Consider shrinking the new review-prompt rule's first bullet from "narrows or removes a text matcher" to the document-subject shape AGENTS.md actually pins — the smaller rule covers the recorded defects.

[FIRST-PRINCIPLES-REVIEWED] 56ca95c

bolichen97 · 2026-09-06T20:08:38Z

/ai-review override gpt 56ca95c: Ack. We are refactor security posture

github-actions · 2026-09-06T20:08:46Z

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 56ca95c9d58669469a14481055bdae7218616833.

Ack. We are refactor security posture

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

…its text A file edit's tool_input is the diff derive_edit_diff renders from the new content. _resolve_permission fed that document to the shell-command predicates (is_sensitive_bash_command, is_denied), so writing a page that says "git push origin main" or a docstring naming the gateway-restart command was refused by the regex rules, and any body over the 20 KiB command cap was refused for its length (#8812). This is the class #9082 closed for cron script bodies: a document is not the shell gate's subject. An event whose tool_kind is "edit" and whose raw_tool_params are present is now judged by where it writes. Every accepted path spelling (platform.tool_paths.target_paths) goes through is_sensitive_write_path, the read+write keystone plus the write-only tier; a truncated walk is denied as unverifiable, mirroring hooks.on_tool_call. The title tier still runs first. An edit with no params, and every non-edit kind, keeps the document scan unchanged. Pinned by test_llm_helpers_edit_gate.py (18 tests). Closes #8812.

…its text A file edit's tool_input is the diff derive_edit_diff renders from the new content. _resolve_permission fed that document to the shell-command predicates (is_sensitive_bash_command, is_denied), so writing a page that says "git push origin main" or a docstring naming the gateway-restart command was refused by the regex rules, and any body over the 20 KiB command cap was refused for its length (#8812). This is the class #9082 closed for cron script bodies: a document is not the shell gate's subject. An edit is now judged by where it writes, on client-derived provenance only: tool_kind "edit" AND shell_classified with is_shell False (the shell cache the preceding tool_call populated) AND raw_params_trusted (params from that same cache). A shell call forging kind="edit" keeps the command scan. The target set is the UNION of every accepted path spelling in the params (platform.tool_paths.target_paths) and the path the tool_call's diff content block named: _dispatch caches that path per scoped toolCallId (diff_path_cache, beside the params/shell/identity caches, in both AcpClient and AcpSessionHandle) and build_permission_event carries it as event.diff_path, because a backend may stream trusted params with no path key and name the file only in the block. Every candidate goes through is_sensitive_write_path (read+write keystone plus the write-only tier); a truncated walk is denied as unverifiable; an EMPTY union is denied outright rather than falling back to the document scan. The title tier still runs first; an edit with no params at all, and every non-edit kind, keeps the document scan unchanged. _EDIT_TOOL_KIND is imported from hooks instead of redeclared. Pinned by test_llm_helpers_edit_gate.py (29 tests). Closes #8812.

…its text (#9197) A file edit's tool_input is the diff derive_edit_diff renders from the new content. _resolve_permission fed that document to the shell-command predicates (is_sensitive_bash_command, is_denied), so writing a page that says "git push origin main" or a docstring naming the gateway-restart command was refused by the regex rules, and any body over the 20 KiB command cap was refused for its length (#8812). This is the class #9082 closed for cron script bodies: a document is not the shell gate's subject. An edit is now judged by where it writes, on client-derived provenance only: tool_kind "edit" AND shell_classified with is_shell False (the shell cache the preceding tool_call populated) AND raw_params_trusted (params from that same cache). A shell call forging kind="edit" keeps the command scan. The target set is the UNION of every accepted path spelling in the params (platform.tool_paths.target_paths) and the path the tool_call's diff content block named: _dispatch caches that path per scoped toolCallId (diff_path_cache, beside the params/shell/identity caches, in both AcpClient and AcpSessionHandle) and build_permission_event carries it as event.diff_path, because a backend may stream trusted params with no path key and name the file only in the block. Every candidate goes through is_sensitive_write_path (read+write keystone plus the write-only tier); a truncated walk is denied as unverifiable; an EMPTY union is denied outright rather than falling back to the document scan. The title tier still runs first; an edit with no params at all, and every non-edit kind, keeps the document scan unchanged. _EDIT_TOOL_KIND is imported from hooks instead of redeclared. Pinned by test_llm_helpers_edit_gate.py (29 tests). Closes #8812. Co-authored-by: zejiangg <zejiangg@amazon.com>

iamwhatever requested a review from a team as a code owner September 6, 2026 17:51

iamwhatever requested a review from buluoray September 6, 2026 17:51

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

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 6, 2026

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 6, 2026

bolichen97 approved these changes Sep 6, 2026

View reviewed changes

bolichen97 merged commit 0535471 into main Sep 6, 2026
63 of 65 checks passed

bolichen97 deleted the fix/cron-source-scan-decouple branch September 6, 2026 20:09

github-actions Bot removed the readiness: checking Automated validation is still running label Sep 6, 2026

bolichen97 mentioned this pull request Sep 6, 2026

refactor(security): drop the regex traversal simulation from the bash gate #9089

Merged

5 tasks

This was referenced Sep 6, 2026

fix(cron): vet script bodies as source + literals, not as one shell command #8564

Closed

Consolidate cron-vet source analyzers into security.py + per-forfeit refusal text #8763

Closed

bolichen97 mentioned this pull request Sep 7, 2026

refactor(security): split security.py into a package and drop path regex #9183

Merged

5 tasks

iamwhatever mentioned this pull request Sep 7, 2026

fix(security): judge a file edit by its target path, not by scanning its text #9197

Merged

5 tasks

dwu96 mentioned this pull request Sep 7, 2026

Bash gate refuses READS of the kiro agent-spec directory; move its write fence to the sandbox READONLY tier? #9198

Open

javenciu mentioned this pull request Sep 7, 2026

fix(security): scan doctest examples in docstrings as code #8870

Closed

5 tasks

CrysisDeu mentioned this pull request Sep 7, 2026

doctest >>> examples in docstrings are executable code but are never scanned as code (pre-existing, all treatments) #8824

Open

Navigation Menu

Uh oh!

fix(security): stop scanning cron script bodies with the shell command gate - #9082

fix(security): stop scanning cron script bodies with the shell command gate#9082
bolichen97 merged 1 commit into
mainfrom
fix/cron-source-scan-decouple

iamwhatever commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

bolichen97 commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iamwhatever commented Sep 6, 2026

Problem / Motivation

Why it matters

What changed (motivation → approach → change)

Tests

Manual verification

Related Issues

Pattern harvest

Checklist

Uh oh!

github-actions Bot commented Sep 6, 2026

Design Review (Fable 5) — 🟡 CONCERNS

Watch

Suggestions

Uh oh!

github-actions Bot commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ human override accepted

Uh oh!

github-actions Bot commented Sep 6, 2026

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 6, 2026

First Principles Review (Fable 5) — 🟡 CONCERNS

What this change ships

Watch

Subtractions

Uh oh!

bolichen97 commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026

Human judgment recorded

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Sep 6, 2026 •

edited

Loading