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

[iamwhatever]

Problem / Motivation

Writing a file gets refused for what the file says. I replayed every policy block from 30 days of session logs (77 sessions, ~120 blocks) against main. The most frequent class left after #9082, #9089 and #9183: the agent writes a Markdown page or a Python file, and the always-on gate reads the file's text as a shell command. A doc that says git push origin main is refused by the git-publish rule. A docstring naming the gateway-restart command is refused by the self-protection rule. A file over 20 KiB is refused for its length. This is #8812.

Each refusal ends the turn. The agent retries with a reworded file, which costs a round trip and teaches it to dodge the gate.

Why it matters

Writing its own code and docs is the most common thing an agent does. A refusal that names a command the file never runs trains the model to treat the gate as noise, and the rewording it learns is exactly the spelling-chase AGENTS.md calls a review smell.

What changed (motivation → approach → change)

A file edit is a document, not a command. derive_edit_diff renders the new file content as a diff and that text lands in event.tool_input. _resolve_permission handed it to is_sensitive_bash_command and is_denied.

Now an edit goes through _edit_target_denial instead. The reroute keys on what the ACP client established from the preceding tool_call frame, not on the payload's own kind (which is agent-influenced): tool_kind == "edit" plus shell_classified with is_shell False plus raw_params_trusted. A shell call that forges kind="edit" still carries the cached is_shell=True and keeps the command scan.

The target is the union of two sources. One is every accepted path spelling in the params (target_paths). The other is the path the tool_call's diff content block named. A backend can send trusted params with no path key and name the file only in that block, so _dispatch now caches that path per toolCallId (diff_path_cache, next to the params and shell caches, in both AcpClient and AcpSessionHandle) and the permission event carries it as diff_path. Every candidate is checked with is_sensitive_write_path, the same gate hooks.on_tool_call applies. A truncated walk is denied as unverifiable. An empty union is denied outright, not handed 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 byte for byte.

An earlier revision of this PR also narrowed the curl exfil substrings (-d @, -F x=@) to lines naming curl. Four revisions each fell to a spelling the shell resolves after the text is read (c\url, "${x}"url, c{u..u}rl, a copied binary). That is the spelling-chase, so it is removed from this PR entirely; the substring rules are unchanged from main.

Tests

• test/test_llm_helpers_edit_gate.py (new, 29 tests): prose naming a denied command is writable; the document scan never runs on an edit; a body over the command cap passes; writes to ~/.ssh, ~/.aws, security_policy.json, config.json, ~/.kiro/agents/ are denied with always_deny_input; nested filePath aliases are judged; a truncated walk denies; the title tier runs first; a non-edit kind and an edit without params keep the document scan; Bash tool_input is unchanged; a forged kind="edit" on a shell call, an unclassified frame, and inline (untrusted) params all keep the command scan; a real tool_call + permission frame pair run through acp._dispatch shows a genuine edit carries the trusted flags and a forged one carries is_shell=True; a protected path named only by the diff content block is denied and a safe one approved; trusted params that name no target are denied; real tool_call / tool_call_update / request_permission frames through acp._dispatch show the block's path reaches the permission event under its origin scope, and a scope miss or missing cache leaves it empty.
• 1123 passed across test_llm_helpers_edit_gate.py, test_llm_helpers_tool_input_offload.py, test_security.py, test_hooks.py, test_workflow_security.py. On the current head: 1645 passed across every test file that touches build_permission_event, raw_params_trusted or diff_path, plus test_acp_tool_identity.py and test/metrics/test_tool_call_duration.py (the direct _build_tool_call_event callers); the 6 failures are in test_acp_client.py session-resume / tool-result-sync tests and reproduce without this change.

Manual verification

Full backend suite on the previous head: 88,661 passed, 271 failed; 235 of those reproduce identically on clean origin/main (7 storage / host-isolation files), none in security, hooks or llm_helpers. flake8, isort, mypy, docs_lint and the black baseline gate are clean.

Screenshots / video

Why no screenshot: backend permission gate only.

Related Issues

Closes #8812

Follows #9082 (cron script bodies leave the shell gate) and #9183 (path regex dropped). The ~/.kiro/agents read refusal is #9198. The curl exfil substring false positives (date -d @<epoch>) are left on main's behavior; whether that rule should stay a bare substring or move to argv-structural parsing is a maintainer call, not another regex.

Pattern harvest

Rule candidate: review-prompt
Pattern: a document (file content, diff, script body) handed to a command-line matcher. The fix is to change the subject the gate judges, not to add exceptions to the matcher.

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 (docs/system-specs/modules/security.md)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Right subject fix — judges an edit by its write target, matching the sanctioned pattern already in hooks.on_tool_call, with provenance-gated rerouting and fail-closed fallbacks.

Watch

• The empty-union hard deny (Blocked: file edit names no target path to verify) means any future backend that spells its edit target under a key outside TARGET_PATH_KEYS and streams no diff content block will have every edit denied. The failure is loud and safe, but harness-parity work should know the fix is a TARGET_PATH_KEYS entry, not a gate change.
• The two edit gates now differ in strength: hooks.on_tool_call's write-protected branch neither consults the diff-block path nor fail-closes an empty target set, so an edit whose target is named only in the content block is still invisible to that gate. Worth a follow-up to feed the same diff_path cache there.

[DESIGN-REVIEWED] 5665071

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 5665071e1f93762596e305bff341632503a9fcf4 — 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 evidence is in hand: the hook gate at hooks.py:784 already runs this path check on the HOOK_BASED channel; the new gate replaces the document scan at the always-enforced tier that also covers callers without hook wiring; AcpEvent.diff_path/LLMEvent.diff_path pre-exist on base (populated only on tool_call events for chat_runner's snapshot); is_sensitive_write_path is a verified superset of the read+write keystone. Two behaviors in the diff never appear in the description. Final review:

First-Principles-Verdict: CONCERNS

The fix earns its place, but the diff-block path plumbing and a brand-new "names no target path" hard deny ship undeclared in a fix PR.

What this change ships

Intent: stop the permission gate refusing file writes for what the file's text says (#8812) — a FIX.

1. Writing prose/code naming a denied command, or a large body, is no longer refused — justified (AGENTS.md's documented "document is not a shell-gate subject" invariant)
2. Edits to protected paths now denied even for callers without hook wiring — justified
3. A trusted edit naming no path anywhere is now denied outright — undeclared
4. Oversized edit params denied as unverifiable — justified, mirrors hooks.on_tool_call
5. Permission events now carry the tool_call diff block's path, cached in client, session handle, dispatch — undeclared
6. security.md bullet updated same commit — justified (mandated)
7. New 20-test file pinning the reroute — justified

Watch

• The description says "An edit with no params, and every non-edit kind, keeps the document scan byte for byte", but the diff adds "Blocked: file edit names no target path to verify (deny-by-default)" — a new refusal class in a PR whose stated job is removing refusals. Boundary-derived (agent untrusted), documented in security.md, but invisible to anyone reading only the description.
• The diff_path_cache threading (5 files) is likewise absent from "What changed"; it reuses the pre-existing AcpEvent.diff_path field (types.py:629, previously tool_call-only for chat_runner) and follows the sibling-cache pattern, so it is load-bearing for item 3, not surface — but it should have been declared.

Duplication check run: the path gate reuses target_paths (platform/tool_paths.py:66) and is_sensitive_write_path (security/paths.py:1855), the same two symbols hooks.py:748-792 uses — grepped both; no second spelling of the predicates, and the always-enforced tier is the only cover for AUTO_APPROVE callers where hooks.on_tool_call (llm_helpers.py:2267, HOOK_BASED-gated) never runs.

[FIRST-PRINCIPLES-REVIEWED] 5665071

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 5665071e1f93762596e305bff341632503a9fcf4 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5665071

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5665071e1f93762596e305bff341632503a9fcf4 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 5665071

Verdict parsed from the review's SHA-scoped output markers for commit 5665071e1f93762596e305bff341632503a9fcf4.

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

[iamwhatever]

self-added: yes
mechanism: none added this round; the curl-word binding is removed

• Escaped curl command bypasses exfiltration gate (span=3c5b15a2d41e) — fixed by removal in ab15094

The finding holds: binding the curl exfil substrings to the word curl opened c\url, and two further revisions opened "${x}"url, c{u..u}rl, c[u]rl and a copied binary in turn. A text test for how curl is spelled cannot be completed, so the mechanism is removed rather than patched. _BASH_EXFIL_PATTERNS and audit_bash_exfiltration are byte-identical to main on this head; every spelling this finding and its siblings named is denied exactly as before the PR.
Findings that a curl-flag exemption admits some spelling of curl are covered by this ruling: there is no exemption on this head. The remaining scope of the PR is the edit-kind target gate in llm_helpers.py, which touches no exfil rule.

[iamwhatever]

Intent: A file edit is judged by where it writes, not by reading the file's text as a shell command. Writing a doc or script that merely mentions a denied command or a fenced path must not be refused (#8812).
Not a goal: Changing any exfil / IMDS / env-credential rule, or any other shell-gate matcher. Reads of ~/.kiro/agents (#9198) and the curl-flag substring false positives are out of scope.

[iamwhatever]

self-added: yes
mechanism: reroute gated on client-derived provenance flags (shell_classified, is_shell, raw_params_trusted) instead of the payload's kind

• Untrusted tool kind bypasses the command gate (span=a59af492fbae) — fixed in bec3c8f

The finding holds: on a permission frame event.tool_kind is the payload's own kind, which _dispatch documents as agent-influenced display metadata, so a shell call could claim kind="edit" and skip the command scan. The reroute now requires what the client itself established from the preceding tool_call frame: shell_classified (the shell cache hit) with is_shell False, and raw_params_trusted (params from that same cache, not the inline fallback). A forged kind="edit" on a shell call carries the cached is_shell=True and keeps the document scan; a frame that missed either cache keeps it too.
Pinned by test_forged_edit_kind_on_a_shell_call_keeps_the_command_scan, test_unclassified_edit_keeps_the_document_scan, test_inline_params_without_cache_provenance_keep_the_document_scan, and TestProvenanceComesFromTheClientNotThePayload, which runs a real tool_call + permission frame pair through acp._dispatch to show a genuine edit carries the flags and a forged one carries is_shell=True.
Findings that the edit reroute trusts an agent-authored field are covered by this ruling: every input it keys on is written by the client from a frame the agent did not author.

[iamwhatever]

self-added: yes
mechanism: target set = params paths UNION the diff content block's path (new diff_path_cache in _dispatch, carried as event.diff_path); an empty union is denied

• Content-block-only edit paths bypass write protection (span=a59af492fbae) — fixed in 5665071

The finding holds: a backend can stream trusted rawInput with no path key and name the file only in the {"type": "diff", "path": ...} content block, so _edit_target_denial over the params alone checked zero paths and returned no denial. Same span as the round-1 forgery finding, so the same-span rule fired and a retrospective ran this round before any code was written. Its verdict: keep the provenance-gated reroute (the alternative, falling back to the document scan on an empty target set, re-admits the #8812 defect and still does not catch a write to ~/.kiro/agents/pwn.json, because a document that contains no denied text is not evidence the write is safe) and close the gap at the target-resolution step.
_dispatch now caches the diff block's path per scoped toolCallId (diff_path_cache, written by _build_tool_call_event and _build_tool_refinement_event, only when a block NAMED a path) beside the params/shell/identity caches in both AcpClient and AcpSessionHandle, and build_permission_event carries it as event.diff_path. _edit_target_denial(raw_params, diff_path) judges the UNION of target_paths(raw_params) and that path with is_sensitive_write_path, and an EMPTY union is denied outright (Blocked: file edit names no target path to verify (deny-by-default)) rather than approved or handed to the document scan.
Pinned by TestTheContentBlockPathIsATarget (protected path named only by the block: denied; safe path named only by the block: approved; either source protected: denied; trusted params naming no target: denied) and TestTheClientCarriesTheDiffPathOntoThePermissionEvent, which runs real tool_call / tool_call_update / request_permission frames through acp._dispatch and shows the block's path reaches the permission event under the same origin scope, and that a scope miss or a missing cache leaves it "" (denied downstream).
Findings that an edit's write target can be omitted from the params to skip the write-protected tier are covered by this ruling: there is no path an edit can take through the reroute that ends in approval without a resolved target having passed is_sensitive_write_path. _EDIT_TOOL_KIND is now imported from hooks (First Principles subtraction on the same head).

[iamwhatever]

self-added: yes
mechanism: none added; one literal removed

• Drop llm_helpers._EDIT_TOOL_KIND and import hooks._EDIT_TOOL_KIND (span=subtraction-edit-tool-kind) — taken in 5665071

The local redeclaration is deleted; llm_helpers imports _EDIT_TOOL_KIND from kiro_crew.hooks on the import line it already had for fire_tool_hooks. The literal now lives in hooks and the pre-existing inline "edit" at _dispatch.py, which this PR does not touch.

[iamwhatever]

self-added: yes
mechanism: none added

• Empty-union deny is a loud failure for a backend spelling its target under a new key (span=design-watch-empty-union) — rebutted (accepted as stated; no change)

The Watch describes the intended shape: the deny is fail-closed and loud, and the fix for a new backend is a TARGET_PATH_KEYS entry in platform/tool_paths.py, never a gate change. That is already how hooks.on_tool_call and governance resolve a target, and the deny reason names what was missing (file edit names no target path to verify) so harness-parity work sees it on the first denied edit. Findings that ask the edit gate to accept an unknown key spelling are covered by this ruling: the accepted spellings are one list, extended there.

[iamwhatever]

self-added: yes
mechanism: none added

• hooks.on_tool_call's write-protected branch does not consult diff_path or fail-close on an empty set (span=design-watch-hook-parity) — accepted-and-deferred to hooks.on_tool_call edit gate should judge the diff content block path too #9298

Holds. The hook gate still reads only target_paths(raw_params). It is not in this PR's intent (the always-enforced tier in _resolve_permission, the only cover for AUTO_APPROVE callers where the hook never runs), and widening it here would re-arm every lane on hooks.py. Tracked in #9298 (deferred-finding, assigned, due 2026-09-21): feed event.diff_path into the hook branch and decide its empty-set behaviour against the empty-kind comment above it.

[iamwhatever]

self-added: yes
mechanism: none added

• The description does not declare the new empty-union refusal (span=fp-watch-undeclared-deny) — fixed in the PR body (no code change)

Holds against the body the lane read. The body was rewritten after this head's review ran: "What changed" now states "An empty union is denied outright, not handed back to the document scan", and the sentence about the document scan is scoped to an edit with no params at all (where raw_params_trusted cannot be earned, so the reroute is never taken) and to non-edit kinds. security.md carries the same statement with the refusal string. A PR whose job is removing a false-positive refusal class declares the one boundary-derived refusal it adds.

[iamwhatever]

self-added: yes
mechanism: none added

• The diff_path_cache threading is absent from "What changed" (span=fp-watch-undeclared-cache) — fixed in the PR body (no code change)

Holds against the body the lane read. "What changed" now names diff_path_cache (in _dispatch, AcpClient and AcpSessionHandle, beside the params and shell caches) and that the permission event carries it as diff_path, reusing the pre-existing AcpEvent.diff_path field. The Tests section names the frame-pair tests that pin it. The duplication check's conclusion — target_paths + is_sensitive_write_path are the one spelling of the predicates, and this tier is the only cover for AUTO_APPROVE callers — matches the design and needs no change.

[bolichen97]

Approving after a security-focused Tech Lead review of 5665071e.

Verdict: this narrows the false-positive surface and WIDENS the real protection surface. Not a weakening.

What I verified rather than took from the description:

1. The subject change is the pre-existing invariant, not a new one. AGENTS.md "Security invariants" already states that a script body is never a shell-gate subject and that a spelling-chase is a review smell whose first question is "is the SUBJECT wrong (a document handed to a command-line matcher)". #8812 is cited there by number. An edit's tool_input is a rendered unified diff, so _first_tool_input_denial was running is_sensitive_bash_command / is_denied / the length cap over prose. hooks.py:784 has judged edits by target_paths + is_sensitive_write_path all along; this aligns the always-enforced tier with it.

2. The replacement predicate is strictly stronger on the axis that matters. is_sensitive_write_path (security/paths.py:1855) = _SENSITIVE_HOME_DIRS + _WRITE_PROTECTED_HOME_PATHS + keystone publish artifacts, a verified superset of is_sensitive_path. So the always-enforced tier now denies writes to config.json and ~/.kiro/agents/ that the old document scan only ever caught by coincidence of the path appearing in the diff header — and it now covers AUTO_APPROVE channels where hooks.on_tool_call never runs. That is added protection, not removed.

3. Every fork of the reroute fails closed, and each one is pinned by a named test. Truncated walk -> deny (test_truncated_walk_is_denied_as_unverifiable). Empty target union -> deny, no fallback to the document scan (test_trusted_params_naming_no_target_are_denied). Missing provenance -> keeps the document scan byte for byte (test_unclassified_edit_keeps_the_document_scan, test_inline_params_without_cache_provenance_keep_the_document_scan, test_edit_without_params_falls_back_to_the_document_scan, test_non_edit_kind_keeps_the_document_scan). Title tier still first (test_title_tier_still_runs_first). Bash unchanged (test_bash_tool_input_is_unchanged).

4. The forged-kind bypass I went looking for does not exist. The reroute keys on shell_classified and not is_shell and raw_params_trusted, all read from the origin-scoped tool_call caches, never from the permission payload's own kind. build_permission_event deliberately refuses to fall back to that field, and shell_cache is written only when the tool_call frame carried a resolved kind — so a shell call spelling kind="edit" on the permission frame still reads is_shell=True and keeps the command scan (test_forged_edit_kind_on_a_shell_call_keeps_the_command_scan). The residual trust in the adapter's tool_call kind is exactly the trust hooks.py:784 already places; this PR adds none.

5. diff_path is monotone. The union can only ADD candidates, and the loop denies on ANY hit, so a cached diff-block path cannot mask a sensitive params path. Its one grant-side effect is rescuing the empty-union deny, and only when the named path itself verifies safe. Cache writes are guarded on a non-empty path (no clobber with "") and keyed by the origin-scoped toolCallId (test_cache_miss_leaves_diff_path_empty).

6. The earlier curl-substring narrowing was correctly removed from this PR rather than carried through a fifth spelling. That is the AGENTS.md rule applied to the author's own diff.

What is genuinely given up is the coincidental scan of inert file bytes. Per the keystone invariant the enforcement point is the OS layer in sandbox.py, not a text matcher, so that scan was never the guarantee — and writes to a fenced path are now judged more strictly than before.

CI on this head: PR Readiness success, 5/5 AI lanes clear (Design PASS, GPT and Opus no blocking, UX success, First Principles CONCERNS advisory and description-completeness only), Coverage / Testpaths / PR Hygiene / Screenshot Evidence green. The three cancelled check-runs are superseded by successful re-runs at 19:14. Single commit, no unresolved review threads.

One non-blocking note for a follow-up, matching the First Principles finding: the new Blocked: file edit names no target path to verify refusal class and the diff_path_cache threading are in security.md but not in the PR description. Both are correct as shipped; only the description is short.