Skip to content

Scope a session patch grant to the file it approved #6251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Hmbown merged 1 commit into from
Sep 16, 2026
+126 −21
Merged

Scope a session patch grant to the file it approved #6251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions crates/tui/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
114 changes: 93 additions & 21 deletions crates/tui/src/tools/approval_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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));

@codewhale-agent codewhale-agent Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Fail-closed guarantee depends on hash_json_value covering the whole input

Both new arms (unparsed_<digest> and no_target_<digest>) are only fail-closed if hash_json_value(input) is a function of the entire JSON value. If it hashes a filtered subset (for example only a specific key, or a canonicalised projection), then two different unparseable patches that agree on that subset would still share one key — i.e. the original escalation would survive in a smaller form. hash_json_value is defined outside this hunk, so please confirm it digests the full value; a short comment on the helper stating that requirement would keep future edits from weakening it.

};

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();

@codewhale-agent codewhale-agent Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fail-closed contract of both new arms rests on hash_json_value digesting the whole input value. Add a brief note at the helper's definition (or at the call sites here) stating that requirement, so a future change that narrows the digest cannot silently reintroduce a shared key for distinct unparseable patches. No literal replacement is offered because the helper is defined outside this hunk.


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

@codewhale-agent codewhale-agent Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Intentional UX change: path-less/unresolvable patches lose their family grant

Previously any unparseable or path-less patch shared one session key, so one approval covered all of them (the bug). Now each distinct byte input gets its own key, so a session approval for such a patch is effectively one-shot until the exact input repeats. That is the right fail-closed behavior and it is documented in the rustdoc and both changelogs, but it is a user-visible increase in prompt frequency for the shapes that previously relied on the shared constant, so it is worth calling out in the PR body/release notes beyond the security framing.

// the same reason as the error arm above: a shared key here is a
// shared grant.
return format!("no_target_{}", hash_json_value(input));

@codewhale-agent codewhale-agent Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] The no_files sentinel is gone; audit for other consumers

hash_patch_paths previously returned the literal "no_files" for every unparseable, path-less or non-patch input, and that string was part of the grouping key surfaced to the rest of the app. It is now never produced. Anything that keyed off it — approvals UI copy, telemetry/counters, an integration test in another module that asserts patch:no_files, or docs — will silently change meaning or start failing. The PR only reports a run over the tools::approval_cache + tools::apply_patch + approval filters, so a workspace-wide grep/test for no_files (and for patch:no_files) is worth doing before merge.

}

let mut hasher = DefaultHasher::new();
Expand Down Expand Up @@ -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}"
);
}
}

@codewhale-agent codewhale-agent Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Two of the four reported un-scoped shapes have no regression test

The PR description names four supported shapes that used to produce the shared no_files key: (1) apply_patch{path, patch}, (2) the same via File{action:"patch", path, patch}, (3) --no-prefix diffs, (4) delete-only diffs. The new tests cover (1) and (3) plus the fail-closed arm, but not (2) or (4). That matters because the fix is only as good as preflight_apply_patch's coverage of those inputs: if the resolver reports an empty touched_files for a delete-only diff (+++ /dev/null), such patches now land in the no_target_<hash> arm — safe (no escalation) but not scoped to the file as the changelog claims, and a later delete of the same file would still need a fresh card because the digest covers the whole input. Without a test the actual behavior for shape (4) is unverified in either direction, and shape (2) is asserted nowhere. A test tying preflight.touched_files to the executor's target set for these two shapes would pin the invariant the whole change rests on.

@codewhale-agent codewhale-agent Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add regression tests for the two shapes the PR description names but the new tests do not exercise: a delete-only diff (--- a/foo.rs / +++ /dev/null, and the prefix-less variant) and File{action:"patch", path, patch}. Assert what actually happens today (either that the key is scoped to foo.rs, or that it falls into the per-input no_target_ digest and therefore differs between two different delete targets). Without this, the claim in the changelog that delete-only diffs used to collapse to one key is untested in both the old and the new implementation. No literal replacement is provided because the expected key depends on whether preflight_apply_patch records deleted paths, which is not visible in this diff.


#[test]
fn grouping_key_collapses_patch_body_for_same_path() {
let key_a = build_approval_grouping_key(
Expand Down
7 changes: 7 additions & 0 deletions web/lib/changelog.generated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
Loading