From 04804a79e086714e70df1360f92091db65980362 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 17:03:22 -0700 Subject: [PATCH] fix(approval): scope a session patch grant to the file it approved (#6247) `build_approval_grouping_key` is the scope of an "approve for the session" decision: two patches share that grant exactly when they share the key. For apply_patch the key was built by a second, weaker parser than the one the executor uses - it read paths only from `+++ b/` headers and the `replace`/`changes` array, never from the tool's own top-level `path` argument, and discarded the normalizer's error with `Err(_) => {}`. When it found nothing it returned the literal constant "no_files". Four supported shapes hit that constant: the documented `apply_patch{path, patch}` override (a bare hunk has no `+++` line, and apply_patch.rs itself tells the model "Ensure the patch includes ---/+++ headers or provide `path`"), the same shape through `File{action:"patch"}`, any `--no-prefix` diff (the executor's normalize_diff_path accepts `+++ src/x.rs` while the fingerprint demanded the literal `b/`), and any delete-only diff. So approving a card that read "patch .env.example" also approved every later path-less patch - to `.env`, to `.codewhale/settings.json` which decides what runs on the machine, to any absolute path outside the workspace, and in a non-git workspace to every file in the tree, since the #5185 carve-out is disabled there. No second card, no notice, no receipt. is_session_approved_for_tool is a bare set lookup on this key, and its own comment names this exact class: "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 closed for shell. It was open for patches, through a constant instead of a tool name. The fix is reuse, not new parsing: hash `preflight_apply_patch(input)`'s touched_files - already `pub`, already what core/engine.rs and auto_review.rs call for the permission path - which folds the path override, prefix-less headers and tab timestamps. Both empty arms now fail closed to a digest of the input rather than to a shared constant, so an unparseable patch is its own family and matches nothing but a byte-identical repeat. Three regression tests, each confirmed failing without the fix by reverting the resolver and keeping the tests: grouping_key_scopes_a_path_override_to_its_own_file FAILED -> ok grouping_key_reads_prefix_less_diff_headers FAILED -> ok grouping_key_fails_closed_on_an_unresolvable_patch FAILED -> ok tools::approval_cache + tools::apply_patch + approval 323 passed; 0 failed (--test-threads=1) cargo fmt --all -- --check clean cargo clippy --workspace --all-targets --all-features --locked (CI's allow list) clean One note for the reviewer: at --test-threads=2 this set reported task_manager::tests::pending_approval_suspends_idle_and_timeout_denial_settles_failed as failing. It passes isolated and passes at --test-threads=1; it is the same load-sensitive timeout class as the compatibility_stream tests, not this change. check-blocking-calls-budget.py still fails on terminal_input.rs - that is main's existing red, fixed in #6243, not introduced here. Closes #6247 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AJENKJ2smviQW4FVGzUTk9 --- CHANGELOG.md | 13 +++ crates/tui/CHANGELOG.md | 13 +++ crates/tui/src/tools/approval_cache.rs | 114 ++++++++++++++++++++----- web/lib/changelog.generated.ts | 7 ++ 4 files changed, 126 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2654212127..3506205935 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- Approving an `apply_patch` "for the session" is now scoped to the file you + approved. The grouping key that scopes a session grant was built by a second, + weaker patch parser that read only `+++ b/` headers and the `replace` array: + it saw no target at all for the documented `apply_patch{path, patch}` + override, for `--no-prefix` diffs, or for delete-only diffs, and collapsed + every one of them to a single shared key. One approval therefore pre-approved + every later patch of that shape, to any file, with no card and no notice. The + key now comes from the same resolver the executor and the permission path + already use, and an input that cannot be resolved gets its own key rather + than a shared one (#6247). + ### Added - File edits are parse-gated before the write lands: Rust goes through diff --git a/crates/tui/CHANGELOG.md b/crates/tui/CHANGELOG.md index 55f75af3fc..dac93236ce 100644 --- a/crates/tui/CHANGELOG.md +++ b/crates/tui/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- Approving an `apply_patch` "for the session" is now scoped to the file you + approved. The grouping key that scopes a session grant was built by a second, + weaker patch parser that read only `+++ b/` headers and the `replace` array: + it saw no target at all for the documented `apply_patch{path, patch}` + override, for `--no-prefix` diffs, or for delete-only diffs, and collapsed + every one of them to a single shared key. One approval therefore pre-approved + every later patch of that shape, to any file, with no card and no notice. The + key now comes from the same resolver the executor and the permission path + already use, and an input that cannot be resolved gets its own key rather + than a shared one (#6247). + ### Added - File edits are parse-gated before the write lands: Rust goes through diff --git a/crates/tui/src/tools/approval_cache.rs b/crates/tui/src/tools/approval_cache.rs index 79bee75e14..02036aa9ab 100644 --- a/crates/tui/src/tools/approval_cache.rs +++ b/crates/tui/src/tools/approval_cache.rs @@ -36,7 +36,6 @@ use std::fmt::Write as _; use serde_json::Value; use sha2::{Digest, Sha256}; -use crate::tools::apply_patch::{NormalizedApplyPatchInput, normalize_apply_patch_input}; use codewhale_execpolicy::command_safety::classify_command; /// The fingerprint of a tool call — stable enough to match repeated @@ -129,35 +128,41 @@ fn command_prefix(input: &serde_json::Value) -> String { } /// Hash the sorted set of file paths referenced by a patch input. +/// +/// The paths come from [`preflight_apply_patch`] — the same resolver the +/// executor, the permission path (`core/engine.rs`) and auto-review already +/// use — rather than from a second, weaker parser. That matters because this +/// string *is* the scope of an "approve for the session" grant: two patches +/// share a grant exactly when they share this key. +/// +/// The previous implementation read only `+++ b/` headers and the +/// `replace`/`changes` array, so it saw no paths at all for the documented +/// `apply_patch{path, patch}` override, for `--no-prefix` diffs, or for +/// delete-only diffs — and collapsed all of them to one shared constant. +/// Approving any one of those pre-approved every later one, to any file +/// (#6247). +/// +/// An input the resolver cannot parse gets a digest of the input itself, not +/// a shared constant: an unparseable patch is its own family and matches +/// nothing but a byte-identical repeat. fn hash_patch_paths(input: &serde_json::Value) -> String { use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; - let mut paths: Vec<&str> = Vec::new(); + let Ok(preflight) = crate::tools::apply_patch::preflight_apply_patch(input) else { + return format!("unparsed_{}", hash_json_value(input)); + }; - match normalize_apply_patch_input(input) { - Ok(NormalizedApplyPatchInput::Replacement { entries, .. }) => { - for change in entries { - if let Some(path) = change.get("path").and_then(|v| v.as_str()) { - paths.push(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(_) => {} - } + let mut paths: Vec<&str> = preflight.touched_files.iter().map(String::as_str).collect(); - paths.sort(); + paths.sort_unstable(); paths.dedup(); if paths.is_empty() { - return "no_files".to_string(); + // The resolver parsed the input but found no target. Fail closed for + // the same reason as the error arm above: a shared key here is a + // shared grant. + return format!("no_target_{}", hash_json_value(input)); } let mut hasher = DefaultHasher::new(); @@ -337,6 +342,73 @@ mod tests { assert_ne!(key_a, key_b); } + /// #6247. The `path` override is the documented way to patch without + /// diff headers (`apply_patch.rs` tells the model "Ensure the patch + /// includes ---/+++ headers or provide `path`"), and a bare hunk has no + /// `+++` line at all. Before the fix both of these produced the constant + /// `patch:no_files`, so one session grant covered every later one. + #[test] + fn grouping_key_scopes_a_path_override_to_its_own_file() { + let hunk = "@@ -1 +1 @@\n-old\n+new\n"; + let benign = build_approval_grouping_key( + "apply_patch", + &json!({"path": ".env.example", "patch": hunk}), + ); + let sensitive = build_approval_grouping_key( + "apply_patch", + &json!({"path": ".codewhale/settings.json", "patch": hunk}), + ); + assert_ne!( + benign, sensitive, + "approving a patch to one file must never cover a patch to another" + ); + assert!( + !format!("{benign:?}").contains("no_files"), + "a resolvable target must never collapse to the shared constant" + ); + } + + /// The executor's `normalize_diff_path` accepts a prefix-less header, so + /// the fingerprint must too — otherwise a `--no-prefix` diff is a second + /// route to the shared key. + #[test] + fn grouping_key_reads_prefix_less_diff_headers() { + let prefixed = build_approval_grouping_key( + "apply_patch", + &json!({"patch": "--- a/src/auth.rs\n+++ b/src/auth.rs\n@@ -1 +1 @@\n-a\n+b\n"}), + ); + let bare = build_approval_grouping_key( + "apply_patch", + &json!({"patch": "--- src/auth.rs\n+++ src/auth.rs\n@@ -1 +1 @@\n-a\n+b\n"}), + ); + assert_eq!( + prefixed, bare, + "the same target written two legal ways is one approval family" + ); + let other = build_approval_grouping_key( + "apply_patch", + &json!({"patch": "--- src/billing.rs\n+++ src/billing.rs\n@@ -1 +1 @@\n-a\n+b\n"}), + ); + assert_ne!(bare, other, "different targets are different families"); + } + + /// Fail closed: an input the resolver cannot parse is its own family, not + /// a member of a shared one. Two different unparseable inputs must not + /// share a grant. + #[test] + fn grouping_key_fails_closed_on_an_unresolvable_patch() { + let a = build_approval_grouping_key("apply_patch", &json!({"patch": "not a diff at all"})); + let b = build_approval_grouping_key("apply_patch", &json!({"patch": "also not a diff"})); + assert_ne!(a, b, "unparseable inputs must not share an approval family"); + for key in [&a, &b] { + let rendered = format!("{key:?}"); + assert!( + !rendered.contains("no_files"), + "the shared constant must not survive anywhere: {rendered}" + ); + } + } + #[test] fn grouping_key_collapses_patch_body_for_same_path() { let key_a = build_approval_grouping_key( diff --git a/web/lib/changelog.generated.ts b/web/lib/changelog.generated.ts index f4f6e783e9..f31878cf99 100644 --- a/web/lib/changelog.generated.ts +++ b/web/lib/changelog.generated.ts @@ -27,6 +27,13 @@ export const CHANGELOG: ChangelogRelease[] = [ "unreleased": true, "compareUrl": "https://github.com/Hmbown/CodeWhale/compare/v0.9.12...HEAD", "sections": [ + { + "heading": "Security", + "items": [ + "Approving an apply_patch \"for the session\" is now scoped to the file you approved. The grouping key that scopes a session grant was built by a second, weaker patch parser that read only +++ b/ headers and the replace array: it saw no target at all for the documented apply_patch{path, patch} override, for --no-prefix diffs, or for delete-only diffs, and collapsed every one of them to a single shared key. One approval therefore pre-approved every later patch of that shape, to…" + ], + "itemCount": 1 + }, { "heading": "Added", "items": [