Repository navigation
Approving one apply_patch for the session can silently pre-approve patches to any other file #6247
Description
build_approval_grouping_key stores an "approve for the session" decision under
patch:<hash_patch_paths(input)>. For a large class of real inputs that hash is
the literal constant "no_files" — so the grant is not scoped to the file the
user saw on the card.
The mechanism
crates/tui/src/tools/approval_cache.rs:131 — hash_patch_paths learns paths
from exactly two places:
Ok(NormalizedApplyPatchInput::Replacement { entries, .. }) => {
// entry["path"]
}
Ok(NormalizedApplyPatchInput::Patch(patch_text)) => {
for line in patch_text.lines() {
if let Some(rest) = line.strip_prefix("+++ b/") { paths.push(rest.trim()); }
}
}
Err(_) => {}It never reads the tool's own top-level path argument, it only accepts the
literal +++ b/ prefix, and it discards the normalizer's error. When it finds
nothing:
if paths.is_empty() {
return "no_files".to_string();
}A shared constant — indistinguishable from a real path set.
The only gate is a set membership test
(crates/tui/src/tui/ui/approval_routing.rs:10):
pub(super) fn is_session_approved_for_tool(app: &App, _tool_name: &str, grouping_key: &str) -> bool {
app.approval_session_approved.contains(grouping_key)
}Its own comment states the invariant this breaks:
Session grants match the grouping key only … A bare tool name is never
session-wide: approving one shell command used to auto-approve the entire
shell tool for the session. Thecontains(tool_name)clause was the
escalation (ops R2).
That escalation was fixed for shell. It is live for patches, through a constant
instead of a tool name.
Inputs that produce no_files
All four are supported shapes, not malformed input:
- The documented path override —
apply_patch{path: "x.rs", patch: "@@ -1 +1 @@\n-a\n+b\n"}. A bare hunk has no+++header at all.apply_patch.rs:592readsoptional_str(input, "path")precisely to support this, and:622tells the model "Ensure the patch includes ---/+++ headers or providepath". - The same shape via the canonical action family,
File{action: "patch", path, patch}. - Any
--no-prefixunified diff: the executor'snormalize_diff_pathaccepts+++ src/auth.rs, the fingerprint demands+++ b/. - Any pure-deletion diff (
+++ /dev/null).
Failure scenario
Ask posture, git workspace. The in-workspace write carve-out (#5185) excludes
.env*, so a card is shown:
- Model calls
apply_patch{path: ".env.example", patch: "@@ -1,2 +1,2 @@\n-A=1\n+A=2\n"}. Card reads "patch .env.example". User picks approve for the session.approval_session_approvednow containspatch:no_files. - Later,
apply_patch{path: ".codewhale/settings.json", patch: "@@ …"}— hook and MCP configuration, i.e. what runs on this machine. Same grouping key.is_session_approved_for_toolreturns true, the disposition resolves to auto-approve, the file is written.
No second card, no notice, no receipt. Same for .env, for absolute
out-of-workspace paths, and — in a non-git workspace, where the carve-out is
disabled entirely — for every file in the tree.
Fix
Reuse the resolver that already exists rather than keeping a second, weaker
parser. preflight_apply_patch is pub at apply_patch.rs:567 and already
folds the path override, prefix-less headers and tab-timestamps; both
core/engine.rs:7441 and auto_review.rs call it for exactly this purpose.
- Hash
preflight_apply_patch(input)?.touched_files. - On
Err, fail closed to a per-input digest (hash_json_value(input)), never to a shared constant. An unparseable patch should be its own family and match nothing.
Tests to add — the existing grouping tests
(approval_cache.rs:339-370) only exercise the replace/changes array shape,
which is why this is uncaught:
{path, patch}override with two different targets produces two different keys- a
--no-prefixdiff produces the same key as itsa/+b/equivalent - a delete-only diff does not share a key with an unrelated patch
Verification
Read on origin/main this session. The reported reachability was
independently checked by a second reviewer, who corrected the original
report: an earlier draft used .github/workflows/release.yml as the example,
which is wrong — that path is inside the #5185 carve-out and never shows a card
in a git workspace, so no escalation is observable there. The corrected set
above (.env*, .codewhale/settings.json, absolute paths, non-git workspaces)
is where the carve-out does not apply.
Activity
fix(approval): scope a session patch grant to the file it approved (c…