fix(security): hook edit gate judges the diff content block path too (#9297) - #9371
Conversation
|
Intent: Make the hook-tier edit gate ( |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound root-cause fix — single-sourced union plus a mechanical drift tripwire — but the new relative-path hard-deny rests on an unverified backend-behavior premise. WatchThe unanchored deny is a new fail-closed rule in the always-enforced tier, not just hook parity: before this PR a relative Suggestions
[DESIGN-REVIEWED] 815ea82 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth premises now verified against source. Candidate 1 (missing Candidate 2 ( No self-originated finding meets the 80+ bar: the change is additive/tightening on the governance plane, both edit gates share one extraction helper, and truncated/unanchored/empty-union all fail closed with test coverage. No findings. [OPUS-REVIEWED] 815ea82 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All verification done — counts run, premises checked against the repo. Final review: First-Principles-Verdict: CONCERNS Both tiers now hard-deny any relative diff-block path on an unverified "backends emit absolute paths" premise; wrong once, every edit on that backend dies. Not justified as shipped
What this change shipsInventory (8 items) — 5 justifiedIntent: close #9197's deferred finding #9297 — the hook edit gate (the only gate on channel dispatch paths) must judge the diff content block's path, not just params. FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] 815ea82 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
e617a7c to
68c7f5f
Compare
|
self-added: yes
|
68c7f5f to
374e181
Compare
|
self-added: yes
|
374e181 to
a7c1fce
Compare
|
self-added: yes
|
|
fixed — Undeclared provenance-stripping rider on a complete fix
|
|
rebutted — Watch: unanchored diff-path hard-deny rests on an unverified backend premise
|
a7c1fce to
16a1a5d
Compare
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
c4044d8 to
8996e4b
Compare
…9297) The always-enforced edit gate (llm_helpers._edit_target_denial) judges a file edit by the UNION of the params' path spellings and the path the tool call's diff content block named, and denies an empty union. The parallel gate in hooks.on_tool_call read only target_paths(raw_params): an edit naming its write-protected target only in the diff block was invisible to it, and an empty target set was not denied. The union now has a single source, platform.tool_paths.edit_target_candidates, used by BOTH gates so they cannot drift again. A call is on the write plane when it declares the edit kind OR its tool_call frame carried a diff content block naming a path (platform.tool_paths.is_edit_call): the diff block is the edit's target of record and only a call declaring a file change carries one, so the spec-optional, agent-influenced kind field is never the gate — a kindless or read-labelled call carrying a diff block is judged as an edit, while the read allowance stays keyed on the absence of a diff block. The hook's branch enters on 'raw_params is not None or diff_path' (an empty dict is judged, not skipped), denies an empty union, and keeps its own fail-closed reading of the truncated flag. A diff-block path still relative after tilde/env expansion is denied as unverifiable in both tiers: it is a verbatim backend field that resolves against the gateway CWD, not the agent workspace, so a workspace symlink could point it at a protected file. The governance plane consumes the same union and routing: classify_tool_args classifies every candidate as a filesystem.write item (an unanchored diff path becomes a never-permittable marker item, same construction as the truncation marker), so a diff-only edit is judged against an operator ALLOW-mode write confinement instead of reaching it pathless. Every enforcing caller that hands raw_params to on_tool_call now threads event.diff_path alongside it, pinned by an AST tripwire test. Closes #9297
…es require The base branch's comment-history cleanup lowered the baseline entries for these three files by exactly the markers this change rewords. On the current base, a tree carrying the old wording sits above its baseline and the gate reds, so the rewording rides with the fix that touches these files.
8996e4b to
815ea82
Compare
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
🤖 Kiro Crew Auto-Pipeline [operator: NicholasRBowers#a942f9ca] Review-ready at head
Remaining gate is the required human maintainer review. Auto-merge is NOT armed. |
bolichen97
left a comment
There was a problem hiding this comment.
Approving at 815ea82 after trying to construct a bypass.
Single-sourced union: platform/tool_paths.py edit_target_candidates is consumed by both hooks.py and llm_helpers.py, identity pinned by test_llm_helpers_denial_uses_the_shared_helper. The always-enforced tier only tightens: _edit_target_gated = _edit_params is not None or bool(event.diff_path and not event.is_shell), while document-scan suppression stays keyed on _edit_params is not None alone, so no shell/kind forgery path opens. The hook branch enters on raw_params is not None or diff_path, denies truncated, unanchored (relative) and empty unions before the sensitivity loop; keystone handling above is unchanged. Governance plane is additive (write pairs for the union, read pairs for kindless routes, network.egress kept when a url rides along). All 11 production on_tool_call(raw_params=…) sites thread diff_path; the one un-threaded site is the non-enforcing Slack EVENT_TOOL_CALL path, and an AST tripwire pins it. security.md updated; no denied-rule count restated.
Body correction: the Tests section says a kindless call carrying a diff_path is NOT judged by the edit branch — the diff does the opposite (is_edit_call returns True on any diff_path; test_a_kindless_call_carrying_a_diff_block_is_judged_as_an_edit asserts DENY). Stale sentence. Also undisclosed: .github/black-baseline.txt graduates test_governance_chokepoints.py with 4 unrelated reformat hunks. Pre-existing residual worth a follow-up: a params-relative path (not diff-relative) still resolves against the gateway CWD rather than being denied — asymmetric with the new diff-path rule.
Problem / Motivation
KiroCrew has two gates that judge a file-edit tool call. PR #9197 taught the always-enforced tier (
llm_helpers._edit_target_denial) to judge an edit by the UNION of the paths named in its params and the path named by the tool call's{"type": "diff"}content block (event.diff_path), and to deny an edit whose union is empty. The parallel gate inhooks.on_tool_call— thetool_kind == "edit"branch — still read onlytarget_paths(raw_params).A backend can stream trusted params that carry no path key at all and name the file only in the diff content block. Such an edit was invisible to the hook gate: a write to a protected config file could pass it unjudged, and an edit naming no target at all was approved blind instead of denied. The two gates judged different target sets. This was raised as a Design Review Watch item on #9197 and tracked as deferred-finding issue #9297.
Why it matters
The hook gate is the ONLY edit gate on the channel dispatch paths (Slack, Discord, Telegram, messaging, task executor/planner, subagent manager, CLI chat, auto-improvement) — those drive
TurnDriverdirectly, notllm_helpers._resolve_permission. On every one of those surfaces, an edit that names~/.kiro/crew/config.jsononly in its diff block skipped the write-protection that exists so a prompt-injected agent cannot rewrite its own resource ceilings. The gap was exactly the shape #9197 fixed one tier up.What changed (motivation → approach → change)
The defect was drift between two computations of the same fact, so the fix makes the fact single-sourced. A new helper,
platform.tool_paths.edit_target_candidates(raw_params, diff_path), returns the union of every accepted path spelling in the params plus the diff block's path. A second helper,is_edit_call(tool_kind, diff_path), is the single routing predicate for the write plane: a call is an edit when it declares theeditkind OR its tool_call frame carried a{"type": "diff"}content block naming a path. The diff block is the edit's target of record — thediff_pathcache is written only when a frame declares a file change, so no legitimate non-edit call carries one — and the ACPkindfield is spec-optional and agent-influenced on permission frames, so it is never the gate: a kindless or read-labelled call carrying a diff block is judged as an edit, and the read allowance is keyed on the absence of a diff block. Both helpers live inplatform.tool_pathsfor the same layering reason astarget_paths:llm_helpersimportshooks, sohookscannot import fromllm_helperswithout a cycle. A truncated walk returns as-is (no append past the work cap); the truncated flag keeps its hard-deny reading in both consumers.hooks.on_tool_callgains adiff_pathparameter. Its edit branch enters onraw_params is not None or diff_path(not truthiness —raw_params={}must be judged and denied on its empty union, exactly as_edit_target_denialjudges any dict; a truthiness guard would be the falsy-guard fail-open class). The branch judges the shared union, denies an empty union, and keeps its own fail-closed reading of the truncated flag so a reorder of the keystone above cannot silently turn a partial scan into a pass. A diff-block path that is still relative after~/env expansion is denied as unverifiable in BOTH tiers: the diff block's path is a verbatim backend field, and a relative one resolves against the gateway process CWD rather than the agent workspace, so a workspace symlink could point it at a protected file no gate would recognize under its unanchored spelling. An edit carryingraw_params=Noneand no diff block still falls through, matching the always-enforced tier, which such an edit never reaches. The deliberately-unmirrored empty/unknowntool_kindread allowance above the branch is untouched.Every enforcing caller that hands
raw_paramstoon_tool_callnow handsevent.diff_pathalongside it: dashboard chat runner, Slack handler and transport dispatch, Discord/Telegram transport dispatch, messaging dispatch, subagent manager, task executor, task planner, CLI chat, the auto-improvement agent runner, andllm_helpers._resolve_permission.llm_helpers._edit_target_denialnow consumes the shared helper instead of its own copy of the union. The governance plane consumes it too:classify_tool_argsclassifies every candidate in the union as afilesystem.writeitem (an unanchored diff path becomes a never-permittable marker item, the same construction as the truncated-scan marker), so a diff-only edit is judged against an operator ALLOW-mode write confinement instead of reaching it pathless. A tripwire test walks the source withastand fails if any productionon_tool_callcall site (direct or passed as a callable, theasyncio.to_threadshape) passesraw_paramswithoutdiff_path— this is the second round of this exact drift, so the invariant is now pinned mechanically.Tests
test/test_hooks_edit_gate_diff_path.py(new, red-verified against the pre-fix behavior — 6 of the tests fail with the union widening and empty-union deny reverted):raw_params=Noneis still judgedraw_params={}takes the same deny (not skipped by a truthiness guard);raw_params=Nonewith no diff block falls through~/env expansion is denied as unverifiable in both tiers (it would resolve against the gateway CWD); a~-spelled path expands deterministically and is judged on its merits; the shared helper sets theunanchoredflag and withholds the path from the candidate setfilesystem.writeconfinement is denied end-to-end throughon_tool_call(and one inside it still passes); an unanchored diff path emits the never-permittable marker item;read/diff-less classification is pinned unchangedtool_kindread allowance is regression-pinned: a kindless config read stays allowed, aread-kind config read stays allowed, and a kindless call carrying adiff_pathis not judged by the edit branchacp._dispatch: atool_callframe whose diff block names a protected path, followed by the permission frame, produces an event whose fields — handed toon_tool_callexactly as the dispatchers hand them — are deniedllm_helpersre-export is identity-checked); the truncated flag survives the union and the union never grows past the work capManual verification
N/A — unit coverage sufficient: the end-to-end test drives the real
acp._dispatchframe parsing and the realHookManager, which is the exact plumbing every dispatcher uses. Full backend suite run locally: zero failure delta against the base commit (the host's pre-existing sandbox-probe failures are identical on both).Related Issues
Closes #9297
Pattern harvest
Rule candidate: review-prompt
Pattern: "two parallel enforcement gates computing the same predicate independently — require the predicate to be single-sourced, and require every caller-threading change to carry a mechanical call-site tripwire (AST scan), because the same drift recurred one tier up within two months"
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...) — two here: the fix, plus a declaredchore(comments)commit that rewords the history-narration markers in three files the fix touches (the base branch's comment-history cleanup lowered those files' baseline entries, so a tree carrying the old wording sits above its baseline and CI's shrink-only comment ratchet reds it)