Skip to content

Commit 9d6c6b4

Browse files
TheGreatAxiosTheGreatAxios
committed
fix(permissions): stop update_plan grants covering manage_tasks
Stored update_plan grants canonicalized onto manage_tasks, so a create-only plan approval auto-allowed full task lifecycle calls. Coverage is now one-directional and seeders drop the narrow key.
1 parent 4b473a4 commit 9d6c6b4

5 files changed

Lines changed: 234 additions & 9 deletions

File tree

‎src/agent/canonical-tool-name.ts‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,59 @@ const DEFAULT_PREFIX = "default.";
77
// cache must use the same name so an alias cannot force a second ask/deny.
88
// Wire names (read/bash/…) and hidden aliases (shell/update_plan) collapse onto
99
// the registry engine id so a grant stored as run_shell covers bash.
10-
export function canonicalToolName(requested: string): string {
10+
function baseToolName(requested: string): string {
1111
let name = requested;
1212
if (name.startsWith(DEFAULT_PREFIX)) {
1313
const stripped = name.slice(DEFAULT_PREFIX.length);
1414
if (stripped.length > 0) name = stripped;
1515
}
16-
name = undoubledName(name) ?? name;
17-
return engineToolName(name);
16+
return undoubledName(name) ?? name;
17+
}
18+
19+
export function canonicalToolName(requested: string): string {
20+
return engineToolName(baseToolName(requested));
21+
}
22+
23+
// Grant coverage across the alias→engine collapse. Comparisons run in native
24+
// key space (both sides resolve onto the engine id first); a raw alias name is
25+
// never compared against a native id. Pure renames
26+
// (read/write/edit/delete/bash/glob, shell) are capability-identical, so a
27+
// grant stored under either name covers the other. update_plan is the
28+
// exception: it hidden-dispatches onto manage_tasks but only ever translates
29+
// to action:"create" with todo/doing/done statuses (see
30+
// translateUpdatePlanArgs), while manage_tasks spans the full lifecycle
31+
// (create/update, including cancelled). Coverage is therefore one-directional:
32+
// a stored manage_tasks (engine) grant covers update_plan use, but a stored
33+
// update_plan grant covers only update_plan-presenting requests — never a
34+
// manage_tasks request, which may carry update/cancel payloads the operator
35+
// never approved.
36+
export function grantToolCovers(
37+
storedTool: string,
38+
requestTool: string,
39+
): boolean {
40+
const storedBase = baseToolName(storedTool);
41+
const requestBase = baseToolName(requestTool);
42+
if (engineToolName(storedBase) !== engineToolName(requestBase)) return false;
43+
if (
44+
storedBase.toLowerCase() === "update_plan" &&
45+
requestBase.toLowerCase() !== "update_plan"
46+
) {
47+
return false;
48+
}
49+
return true;
50+
}
51+
52+
// Native key for a stored grant. Pure renames collapse onto the engine id
53+
// (capability-identical, so the collapse is behavior-preserving). update_plan
54+
// has no native key that preserves its narrow create-only capability:
55+
// collapsing it onto manage_tasks would read a stored plan approval as full
56+
// task lifecycle, so it maps to null and the seeder drops it fail-closed.
57+
// Live update_plan use auto-allows and never mints, so no reachable flow
58+
// needs the dropped key.
59+
export function canonicalGrantTool(storedTool: string): string | null {
60+
const base = baseToolName(storedTool);
61+
if (base.toLowerCase() === "update_plan") return null;
62+
return engineToolName(base);
1863
}
1964

2065
function undoubledName(requested: string): string | undefined {

‎src/agent/tool-aliases.test.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,71 @@ describe("grant aliases", () => {
101101
).toBe(true);
102102
});
103103

104+
test("pure renames stay bidirectional: read covers read_file and vice versa", async () => {
105+
expect(
106+
await evaluateApprovals({
107+
tool: "read_file",
108+
subject: "src/a.ts",
109+
approvals: [{ tool: "read", pattern: "src/*" }],
110+
workspace: noWorkspace,
111+
}),
112+
).toBe(true);
113+
expect(
114+
await evaluateApprovals({
115+
tool: "read",
116+
subject: "src/a.ts",
117+
approvals: [{ tool: "read_file", pattern: "src/*" }],
118+
workspace: noWorkspace,
119+
}),
120+
).toBe(true);
121+
});
122+
123+
// update_plan hidden-dispatches onto manage_tasks but is NOT
124+
// capability-identical: it only ever translates to action:"create" with
125+
// todo/doing/done statuses, while manage_tasks spans the full lifecycle
126+
// (create/update, including cancelled). Grant coverage is one-directional:
127+
// a stored update_plan grant must never cover a manage_tasks request.
128+
test("a stored update_plan grant does not cover manage_tasks", async () => {
129+
expect(
130+
await evaluateApprovals({
131+
tool: "manage_tasks",
132+
subject: "manage_tasks",
133+
approvals: [{ tool: "update_plan", pattern: "*" }],
134+
workspace: noWorkspace,
135+
}),
136+
).toBe(false);
137+
});
138+
139+
test("a stored update_plan grant still covers update_plan (create-equivalent) use", async () => {
140+
expect(
141+
await evaluateApprovals({
142+
tool: "update_plan",
143+
subject: "update_plan",
144+
approvals: [{ tool: "update_plan", pattern: "*" }],
145+
workspace: noWorkspace,
146+
}),
147+
).toBe(true);
148+
});
149+
150+
test("a stored manage_tasks grant covers update_plan (engine covers alias)", async () => {
151+
expect(
152+
await evaluateApprovals({
153+
tool: "update_plan",
154+
subject: "update_plan",
155+
approvals: [{ tool: "manage_tasks", pattern: "*" }],
156+
workspace: noWorkspace,
157+
}),
158+
).toBe(true);
159+
expect(
160+
await evaluateApprovals({
161+
tool: "manage_tasks",
162+
subject: "manage_tasks",
163+
approvals: [{ tool: "manage_tasks", pattern: "*" }],
164+
workspace: noWorkspace,
165+
}),
166+
).toBe(true);
167+
});
168+
104169
test("canonicalToolName maps wire and hidden aliases onto engines", () => {
105170
expect(canonicalToolName("bash")).toBe("run_shell");
106171
expect(canonicalToolName("shell")).toBe("run_shell");

‎src/permission/authz-grants.ts‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import { evaluateGrants, type GrantRule } from "@intx/authz";
22

3-
import { canonicalToolName } from "../agent/canonical-tool-name.js";
3+
import {
4+
canonicalGrantTool,
5+
canonicalToolName,
6+
grantToolCovers,
7+
} from "../agent/canonical-tool-name.js";
48
import type { Approval } from "./types.js";
59
import { matchesPattern } from "./matcher.js";
610
import { realpathOr } from "./worktree-roots.js";
@@ -68,6 +72,22 @@ export function cwdMatchesGrant(
6872
return workspace.roots.includes(realpathOr(requestCwd));
6973
}
7074

75+
// Seeded grants enter the gate in native key space: pure renames collapse
76+
// onto the engine id, and narrow update_plan keys (which no native key can
77+
// represent without overclaiming capability) are dropped fail-closed. Fresh
78+
// array; the gate owns it.
79+
export function normalizeSeededApprovals(
80+
seeded: readonly Approval[],
81+
): Approval[] {
82+
const out: Approval[] = [];
83+
for (const approval of seeded) {
84+
const tool = canonicalGrantTool(approval.tool);
85+
if (tool === null) continue;
86+
out.push(tool === approval.tool ? approval : { ...approval, tool });
87+
}
88+
return out;
89+
}
90+
7191
// The single place that decides whether a grant's tool/providerModel/cwd
7292
// scope covers a request, independent of whether the grant's pattern matches
7393
// the request's subject. Every live call site that needs to know "does this
@@ -82,7 +102,7 @@ export function grantScopeMatches(
82102
workspace: GrantWorkspace,
83103
): boolean {
84104
return (
85-
canonicalToolName(approval.tool) === canonicalToolName(tool) &&
105+
grantToolCovers(approval.tool, tool) &&
86106
(approval.providerModel === undefined ||
87107
approval.providerModel === activeProviderModel) &&
88108
cwdMatchesGrant(approval.cwd, requestCwd, workspace)
@@ -117,8 +137,13 @@ export async function approvalCoversSubject(
117137
workspace,
118138
} = input;
119139
const action = canonicalToolName(tool);
140+
// Scope-matching sees the raw request name: grantToolCovers is directional
141+
// for the update_plan/manage_tasks pair (a stored update_plan grant covers
142+
// only update_plan-presenting requests), so pre-canonicalizing here would
143+
// erase the alias and wrongly deny same-alias replay. The @intx/authz call
144+
// below still uses the canonical action on both sides.
120145
const scoped = approvals.filter((a) =>
121-
grantScopeMatches(a, action, activeProviderModel, requestCwd, workspace),
146+
grantScopeMatches(a, tool, activeProviderModel, requestCwd, workspace),
122147
);
123148
if (scoped.length === 0) return false;
124149

‎src/permission/gate.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import { matchesPattern, escapeGlobLiteral } from "./matcher.js";
3030
import {
3131
approvalCoversSubject,
3232
grantScopeMatches,
33+
normalizeSeededApprovals,
3334
type GrantWorkspace,
3435
} from "./authz-grants.js";
3536
import {
@@ -559,7 +560,9 @@ export function createPermissionGate(
559560
let auto = options.auto;
560561
let skipPermissions = options.skipPermissions;
561562
// Own a private copy so evaluating a grant never mutates the caller's array.
562-
const approvals: Approval[] = [...options.approvals];
563+
// Seeded grants enter in native key space (pure renames collapsed, narrow
564+
// update_plan keys dropped fail-closed).
565+
const approvals: Approval[] = normalizeSeededApprovals(options.approvals);
563566
let activeProviderModel =
564567
providerName !== undefined && model !== undefined
565568
? `${providerName}:${model}`
@@ -572,7 +575,9 @@ export function createPermissionGate(
572575
// scope-appropriate home: session grants stay in memory, everything else is
573576
// persisted. Both approval branches must mint identically — this is the
574577
// single place a grant comes into existence.
575-
const mintGrant = (tool: string, outcome: ApprovalOutcome): void => {
578+
const mintGrant = (requestedTool: string, outcome: ApprovalOutcome): void => {
579+
// Mint in native key space; live requests are already post-coercion.
580+
const tool = canonicalToolName(requestedTool);
576581
if (!outcome.persist || outcome.persist.pattern === null) return;
577582
const grant: GrantScope = outcome.persist.grant ?? "session";
578583
// A run_shell pattern may still carry a model-authored comment line (the
@@ -1205,7 +1210,7 @@ export function createPermissionGate(
12051210

12061211
const setSeededApprovals = (seeded: readonly Approval[]): void => {
12071212
approvals.length = 0;
1208-
approvals.push(...seeded, ...sessionGrants);
1213+
approvals.push(...normalizeSeededApprovals(seeded), ...sessionGrants);
12091214
// Re-seeded approvals can cover previously-denied requests — cached
12101215
// denies must re-evaluate instead of serving stale reasons.
12111216
denialMemory.clear();

‎src/permission/grant-scope.test.ts‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
approvalCoversSubject,
66
evaluateApprovals,
77
grantScopeMatches,
8+
normalizeSeededApprovals,
89
cwdMatchesGrant,
910
type GrantWorkspace,
1011
} from "./authz-grants.js";
@@ -27,6 +28,8 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () =>
2728
{ tool: "run_shell", pattern: "npm test", providerModel: "openai:gpt-5" },
2829
{ tool: "run_shell", pattern: "npm test", cwd: "/proj" },
2930
{ tool: "write_file", pattern: "npm test" },
31+
{ tool: "update_plan", pattern: "npm test" },
32+
{ tool: "manage_tasks", pattern: "npm test" },
3033
];
3134

3235
const requests: {
@@ -39,6 +42,8 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () =>
3942
{ tool: "run_shell", cwd: "/other", activeProviderModel: "openai:gpt-5" },
4043
{ tool: "run_shell", cwd: undefined, activeProviderModel: undefined },
4144
{ tool: "write_file", cwd: "/proj", activeProviderModel: undefined },
45+
{ tool: "manage_tasks", cwd: "/proj", activeProviderModel: undefined },
46+
{ tool: "update_plan", cwd: "/proj", activeProviderModel: undefined },
4247
];
4348

4449
for (const grant of grants) {
@@ -85,6 +90,86 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () =>
8590
});
8691
}
8792
}
93+
94+
test("stored update_plan grant never covers a manage_tasks request", async () => {
95+
const grant: Approval = { tool: "update_plan", pattern: "*" };
96+
expect(
97+
grantScopeMatches(grant, "manage_tasks", undefined, "/proj", workspace),
98+
).toBe(false);
99+
expect(
100+
await approvalCoversSubject({
101+
tool: "manage_tasks",
102+
subject: "manage_tasks",
103+
approvals: [grant],
104+
activeProviderModel: undefined,
105+
requestCwd: "/proj",
106+
workspace,
107+
}),
108+
).toBe(false);
109+
});
110+
111+
test("stored manage_tasks grant covers an update_plan-presenting request", async () => {
112+
const grant: Approval = { tool: "manage_tasks", pattern: "*" };
113+
expect(
114+
grantScopeMatches(grant, "update_plan", undefined, "/proj", workspace),
115+
).toBe(true);
116+
expect(
117+
await approvalCoversSubject({
118+
tool: "update_plan",
119+
subject: "update_plan",
120+
approvals: [grant],
121+
activeProviderModel: undefined,
122+
requestCwd: "/proj",
123+
workspace,
124+
}),
125+
).toBe(true);
126+
});
127+
});
128+
129+
// Seeded grants enter the gate in native key space. Pure renames collapse onto
130+
// the engine id (capability-identical, behavior-preserving); a stored
131+
// update_plan key has no native representation that preserves its narrow
132+
// create-only capability, so it is dropped fail-closed instead of widening
133+
// onto manage_tasks.
134+
describe("normalizeSeededApprovals", () => {
135+
test("collapses pure renames onto the engine id, preserving other fields", () => {
136+
expect(
137+
normalizeSeededApprovals([
138+
{
139+
tool: "bash",
140+
pattern: "npm test",
141+
providerModel: "openai:gpt-5",
142+
cwd: "/proj",
143+
},
144+
{ tool: "read", pattern: "README.md" },
145+
]),
146+
).toEqual([
147+
{
148+
tool: "run_shell",
149+
pattern: "npm test",
150+
providerModel: "openai:gpt-5",
151+
cwd: "/proj",
152+
},
153+
{ tool: "read_file", pattern: "README.md" },
154+
]);
155+
});
156+
157+
test("keeps native keys as-is", () => {
158+
const seeded: Approval[] = [
159+
{ tool: "run_shell", pattern: "*" },
160+
{ tool: "manage_tasks", pattern: "*" },
161+
];
162+
expect(normalizeSeededApprovals(seeded)).toEqual(seeded);
163+
});
164+
165+
test("drops update_plan keys fail-closed", () => {
166+
expect(
167+
normalizeSeededApprovals([
168+
{ tool: "update_plan", pattern: "*" },
169+
{ tool: "manage_tasks", pattern: "*" },
170+
]),
171+
).toEqual([{ tool: "manage_tasks", pattern: "*" }]);
172+
});
88173
});
89174

90175
// Grant minting decomposes a multi-segment chain scope into one grant per

0 commit comments

Comments
 (0)