Skip to content

Approving one apply_patch for the session can silently pre-approve patches to any other file #6247

Description

@Hmbown

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:131hash_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. The contains(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:

  1. The documented path overrideapply_patch{path: "x.rs", patch: "@@ -1 +1 @@\n-a\n+b\n"}. A bare hunk has no +++ header at all. apply_patch.rs:592 reads optional_str(input, "path") precisely to support this, and :622 tells the model "Ensure the patch includes ---/+++ headers or provide path".
  2. The same shape via the canonical action family, File{action: "patch", path, patch}.
  3. Any --no-prefix unified diff: the executor's normalize_diff_path accepts +++ src/auth.rs, the fingerprint demands +++ b/.
  4. 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:

  1. 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_approved now contains patch:no_files.
  2. 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_tool returns 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-prefix diff produces the same key as its a/+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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions