From bbf43b8e12ff8479684fcfa9cf9651cdf3ae3156 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 09:47:42 -0700 Subject: [PATCH] Fix project-scoped grants never matching sub-agent worktree requests mintGrant stamps a project grant's cwd with the session root, but a sub-agent's request carries the cwd of its own git worktree (which can live outside the session root as a sibling directory, per CL-4929). The three sites that compared grant.cwd to request.cwd by strict equality (evaluateApprovals, isRequestCoveredByGrant, hasExactFullCommandGrant) meant a worktree path could never equal the session root, so no project-scoped grant ever matched a sub-agent request. Added a shared cwdMatchesGrant helper that resolves membership through the gate's existing rootsProvider (the authoritative registry for "which worktrees belong to this session," already used for path containment) instead of comparing raw path strings. A grant only gets this leniency when its stamped cwd matches the current gate's own session root, so a grant still never crosses project boundaries; membership within a matching project is exact equality against the resolved worktree roots, never a path-prefix. --- src/permission/authz-grants.ts | 41 ++++++- src/permission/gate.test.ts | 22 +++- src/permission/gate.ts | 33 +++-- src/permission/permission.test.ts | 195 +++++++++++++++++++++++++++++- src/permission/worktree-roots.ts | 6 +- 5 files changed, 277 insertions(+), 20 deletions(-) diff --git a/src/permission/authz-grants.ts b/src/permission/authz-grants.ts index 1bc5a7420..985df5e03 100644 --- a/src/permission/authz-grants.ts +++ b/src/permission/authz-grants.ts @@ -2,6 +2,7 @@ import { evaluateGrants, type GrantRule } from "@intx/authz"; import type { Approval } from "./types.js"; import { matchesPattern } from "./matcher.js"; +import { realpathOr } from "./worktree-roots.js"; // Exact-escaped patterns (backslash before metacharacters) cannot round-trip // through @intx/authz matchPattern, so those grants are filtered out of the @@ -25,12 +26,48 @@ export function approvalToGrantRule(approval: Approval, index: number): GrantRul }; } +// The gate's own project boundary: the session root it was constructed with, +// plus every git worktree registered against that root (which, per CL-4929, +// may live outside the root entirely — a sibling directory, not a +// subdirectory). Built once per gate from its closed-over resolvedCwd and +// rootsProvider and threaded through — never accept one built anywhere else, +// or "same project" quietly stops meaning "same gate's project." +export type GrantWorkspace = { resolvedCwd: string; roots: readonly string[] }; + +// A project-scoped grant (Approval.cwd set) is confined to the session that +// minted it: it may replay only for a request whose cwd is that same session +// root, or one of the root's registered worktrees. A worktree cwd never +// equals the session root by string identity (that's the bug this closes), +// so membership is resolved through `workspace` instead of a bare `===`. +// +// `grantCwd !== workspace.resolvedCwd` is the boundary: a grant stamped with +// some OTHER project's root is rejected before roots are ever consulted, so +// a request cwd that happens to coincide with a different project's worktree +// can never match. Membership within a matching project is exact equality +// against the resolved roots, never a path-prefix — a prefix check would let +// a maliciously named sibling directory (`/repo/wt-1-evil`) match a +// legitimate root (`/repo/wt-1`). `workspace.roots` already comes back +// realpath-resolved (see worktree-roots.ts); `requestCwd` is realpath'd here +// so a symlinked checkout (macOS /tmp vs /private/tmp) still compares equal. +export function cwdMatchesGrant( + grantCwd: string | undefined, + requestCwd: string | undefined, + workspace: GrantWorkspace, +): boolean { + if (grantCwd === undefined) return true; + if (requestCwd === undefined) return false; + if (grantCwd === requestCwd) return true; + if (grantCwd !== workspace.resolvedCwd) return false; + return workspace.roots.includes(realpathOr(requestCwd)); +} + export type EvaluateApprovalsInput = { tool: string; subject: string; approvals: readonly Approval[]; activeProviderModel?: string | undefined; requestCwd?: string | undefined; + workspace: GrantWorkspace; }; // Grant-store evaluation via @intx/authz. Filters provider-model and cwd the @@ -39,12 +76,12 @@ export type EvaluateApprovalsInput = { // matchesPattern (equality after unescape) first so a stored exact command is // never lost. export async function evaluateApprovals(input: EvaluateApprovalsInput): Promise { - const { tool, subject, approvals, activeProviderModel, requestCwd } = input; + const { tool, subject, approvals, activeProviderModel, requestCwd, workspace } = input; const scoped = approvals.filter( (a) => a.tool === tool && (a.providerModel === undefined || a.providerModel === activeProviderModel) && - (a.cwd === undefined || a.cwd === requestCwd), + cwdMatchesGrant(a.cwd, requestCwd, workspace), ); if (scoped.length === 0) return false; diff --git a/src/permission/gate.test.ts b/src/permission/gate.test.ts index fefed1d5d..8b769a212 100644 --- a/src/permission/gate.test.ts +++ b/src/permission/gate.test.ts @@ -60,7 +60,9 @@ describe("preGrantGuardReason / isRequestCoveredByGrant guard parity", () => { cwd, }; const grant: Approval = { tool: "run_shell", pattern: command }; - expect(isRequestCoveredByGrant(request, grant, undefined, isRestricted)).toBe(false); + expect( + isRequestCoveredByGrant(request, grant, undefined, isRestricted, { resolvedCwd: cwd, roots: [] }), + ).toBe(false); }); test(`${name}: evaluate() never allows outright`, async () => { @@ -85,7 +87,9 @@ describe("preGrantGuardReason / isRequestCoveredByGrant guard parity", () => { }; expect(preGrantGuardReason(request, isRestricted)).toBeUndefined(); const grant: Approval = { tool: "run_shell", pattern: "npm test" }; - expect(isRequestCoveredByGrant(request, grant, undefined, isRestricted)).toBe(true); + expect( + isRequestCoveredByGrant(request, grant, undefined, isRestricted, { resolvedCwd: cwd, roots: [] }), + ).toBe(true); }); }); @@ -122,7 +126,12 @@ describe("grant coverage rebinds relative paths to the request process cwd", () cwd: agentCwd, }; const grant: Approval = { tool: "run_shell", pattern: "cat *" }; - expect(isRequestCoveredByGrant(request, grant, undefined, sessionRestricted)).toBe(false); + expect( + isRequestCoveredByGrant(request, grant, undefined, sessionRestricted, { + resolvedCwd: sessionCwd, + roots: [], + }), + ).toBe(false); }); test("a relative path inside the registered worktree is not forced-restricted", () => { @@ -135,6 +144,11 @@ describe("grant coverage rebinds relative paths to the request process cwd", () cwd: agentCwd, }; const grant: Approval = { tool: "run_shell", pattern: "cat *" }; - expect(isRequestCoveredByGrant(request, grant, undefined, sessionRestricted)).toBe(true); + expect( + isRequestCoveredByGrant(request, grant, undefined, sessionRestricted, { + resolvedCwd: sessionCwd, + roots: [], + }), + ).toBe(true); }); }); diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 0b3929f34..5d0fb7713 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -14,7 +14,7 @@ import { autoShellRuleForCall } from "./auto-shell-policy.js"; import { commandReferencesSensitivePath } from "../plugins/secret-guard-plugin.js"; import { runShellAuthzBlockReason } from "../shell/run-shell-authz.js"; import { matchesPattern, escapeGlobLiteral } from "./matcher.js"; -import { evaluateApprovals } from "./authz-grants.js"; +import { evaluateApprovals, cwdMatchesGrant, type GrantWorkspace } from "./authz-grants.js"; import { splitChainedCommand, tokenize, isShellCommentOnly, stripCommentLines } from "./command.js"; import { createPathRestriction } from "./path-restriction.js"; import { createWorktreeRootsProvider, type RootsProvider } from "./worktree-roots.js"; @@ -51,6 +51,7 @@ function hasExactFullCommandGrant( approvals: readonly Approval[], activeProviderModel: string | undefined, requestCwd: string | undefined, + workspace: GrantWorkspace, ): boolean { // Comment-insensitive: a model-authored "# why" line prepended to an // otherwise-identical command must still replay against a grant minted @@ -62,7 +63,7 @@ function hasExactFullCommandGrant( a.tool === tool && a.pattern === normalized && (a.providerModel === undefined || a.providerModel === activeProviderModel) && - (a.cwd === undefined || a.cwd === requestCwd), + cwdMatchesGrant(a.cwd, requestCwd, workspace), ); } @@ -142,9 +143,10 @@ export function isRequestCoveredByGrant( approval: Approval, activeProviderModel: string | undefined, isRestricted: (path: string, isWrite: boolean) => boolean, + workspace: GrantWorkspace, ): boolean { if (request.tool !== approval.tool) return false; - if (approval.cwd !== undefined && approval.cwd !== request.cwd) return false; + if (!cwdMatchesGrant(approval.cwd, request.cwd, workspace)) return false; if ( approval.providerModel !== undefined && approval.providerModel !== activeProviderModel @@ -268,11 +270,15 @@ export function createPermissionGate(options: PermissionGateOptions): Permission const { requestApproval, persist, interactive, skipPermissions, providerName, model, cwd } = options; const mcpTiers = options.mcpTiers ?? createMcpToolPermissionRegistry(); const resolvedCwd = cwd ?? process.cwd(); - const pathRestriction = createPathRestriction( - resolvedCwd, - options.rootsProvider ?? createWorktreeRootsProvider(resolvedCwd), - ); + const rootsProvider = options.rootsProvider ?? createWorktreeRootsProvider(resolvedCwd); + const pathRestriction = createPathRestriction(resolvedCwd, rootsProvider); const isRestricted = pathRestriction.isRestricted; + // This gate's project boundary for grant matching (see cwdMatchesGrant): + // this session's root plus its currently-known registered worktrees. Built + // fresh per read from the same rootsProvider the gate already uses for + // path containment, so "same project" for a grant and "inside the + // workspace" for a path share one authority. + const grantWorkspace = (): GrantWorkspace => ({ resolvedCwd, roots: rootsProvider() }); let auto = options.auto; // Own a private copy so evaluating a grant never mutates the caller's array. const approvals: Approval[] = [...options.approvals]; @@ -309,7 +315,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission persist?.(approval, grant); } options.onGrant?.(approval, (request) => - isRequestCoveredByGrant(request, approval, activeProviderModel, isRestricted), + isRequestCoveredByGrant(request, approval, activeProviderModel, isRestricted, grantWorkspace()), ); }; @@ -405,7 +411,14 @@ export function createPermissionGate(options: PermissionGateOptions): Permission !fullReferencesSecret && !commandTargetsRestricted(fullCommand, isRestrictedHere) && segments.length > 1 && - hasExactFullCommandGrant(request.tool, fullCommand, approvals, activeProviderModel, effectiveCwd) + hasExactFullCommandGrant( + request.tool, + fullCommand, + approvals, + activeProviderModel, + effectiveCwd, + grantWorkspace(), + ) ) { continue; } @@ -432,6 +445,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission approvals, activeProviderModel, requestCwd: effectiveCwd, + workspace: grantWorkspace(), }) ) { continue; @@ -502,6 +516,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission approvals, activeProviderModel, requestCwd: effectiveCwd, + workspace: grantWorkspace(), }); if (alreadyApproved) { continue; diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index 22b03dd0a..4a546738a 100644 --- a/src/permission/permission.test.ts +++ b/src/permission/permission.test.ts @@ -260,16 +260,22 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { { tool: "write_file", pattern: "src/*" }, { tool: "run_shell", pattern: "rm -rf build/\\*" }, ]; + // No approval in these fixtures carries a cwd, so the workspace passed here + // is never actually consulted (cwdMatchesGrant short-circuits on + // grantCwd === undefined) — an explicit no-op value is threaded through + // instead of an optional param, so a future call site can't silently + // narrow the security check by forgetting to pass one. + const noWorkspace = { resolvedCwd: "/unused", roots: [] }; test("allows package-compatible wildcard grants", async () => { expect( - await evaluateApprovals({ tool: "run_shell", subject: "npm test", approvals }), + await evaluateApprovals({ tool: "run_shell", subject: "npm test", approvals, workspace: noWorkspace }), ).toBe(true); expect( - await evaluateApprovals({ tool: "run_shell", subject: "curl x", approvals }), + await evaluateApprovals({ tool: "run_shell", subject: "curl x", approvals, workspace: noWorkspace }), ).toBe(false); expect( - await evaluateApprovals({ tool: "write_file", subject: "src/a.ts", approvals }), + await evaluateApprovals({ tool: "write_file", subject: "src/a.ts", approvals, workspace: noWorkspace }), ).toBe(true); }); @@ -279,6 +285,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { tool: "run_shell", subject: "rm -rf build/*", approvals, + workspace: noWorkspace, }), ).toBe(true); expect( @@ -286,6 +293,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { tool: "run_shell", subject: "rm -rf build/../../etc", approvals, + workspace: noWorkspace, }), ).toBe(false); }); @@ -301,6 +309,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { subject: "npm test", approvals: scoped, activeProviderModel: "openai:gpt-4o", + workspace: noWorkspace, }), ).toBe(true); expect( @@ -309,6 +318,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { subject: "npm test", approvals: scoped, activeProviderModel: "anthropic:opus", + workspace: noWorkspace, }), ).toBe(false); expect( @@ -317,6 +327,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { subject: "git status", approvals: scoped, requestCwd: "/repo-a", + workspace: noWorkspace, }), ).toBe(true); expect( @@ -325,9 +336,69 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { subject: "git status", approvals: scoped, requestCwd: "/repo-b", + workspace: noWorkspace, }), ).toBe(false); }); + + test("a project grant minted at the session root matches a request whose cwd is a registered worktree of that root", async () => { + const scoped: Approval[] = [{ tool: "run_shell", pattern: "git *", cwd: "/session-root" }]; + const workspace = { resolvedCwd: "/session-root", roots: ["/sibling-dispatch-wts/agent-1"] }; + expect( + await evaluateApprovals({ + tool: "run_shell", + subject: "git status", + approvals: scoped, + requestCwd: "/sibling-dispatch-wts/agent-1", + workspace, + }), + ).toBe(true); + }); + + // Security test: a grant minted for one project must never authorize a + // request whose cwd belongs to a completely different project, even when + // that other project also happens to be a git worktree somewhere. Must + // pass both before and after the worktree-matching fix. + test("a project grant does not match a request from an unrelated project root", async () => { + const scoped: Approval[] = [{ tool: "run_shell", pattern: "git *", cwd: "/session-root" }]; + const workspace = { resolvedCwd: "/session-root", roots: ["/sibling-dispatch-wts/agent-1"] }; + expect( + await evaluateApprovals({ + tool: "run_shell", + subject: "git status", + approvals: scoped, + requestCwd: "/some-other-unrelated-project", + workspace, + }), + ).toBe(false); + }); + + test("session and provider-model scopes (no cwd) are unaffected by workspace membership", async () => { + const scoped: Approval[] = [ + { tool: "run_shell", pattern: "npm *" }, + { tool: "run_shell", pattern: "git *", providerModel: "openai:gpt-4o" }, + ]; + const workspace = { resolvedCwd: "/session-root", roots: ["/sibling-dispatch-wts/agent-1"] }; + expect( + await evaluateApprovals({ + tool: "run_shell", + subject: "npm test", + approvals: scoped, + requestCwd: "/anywhere-at-all", + workspace, + }), + ).toBe(true); + expect( + await evaluateApprovals({ + tool: "run_shell", + subject: "git status", + approvals: scoped, + activeProviderModel: "openai:gpt-4o", + requestCwd: "/anywhere-at-all", + workspace, + }), + ).toBe(true); + }); }); describe("classifyTool", () => { @@ -2916,6 +2987,124 @@ describe("sub-agent identity on permission requests", () => { }); }); +describe("project-scoped grants match sub-agent worktree requests (CL-5662)", () => { + const git = (cwd: string, ...args: string[]): void => { + execFileSync("git", args, { cwd, stdio: "ignore" }); + }; + + // A sibling worktree, not nested under the session root — mirrors CL-4929's + // real-world layout where a sub-agent's worktree lives outside the repo + // entirely (e.g. a dispatch worktrees directory next to the checkout). + const createRepoWithSiblingWorktree = (): { repo: string; worktree: string } => { + const base = mkdtempSync(join(tmpdir(), "corbits-project-grant-")); + const repo = join(base, "repo"); + const worktree = join(base, "sibling-worktree"); + mkdirSync(repo); + git(repo, "init", "-b", "main"); + git(repo, "-c", "user.email=t@t", "-c", "user.name=t", "commit", "--allow-empty", "-m", "init"); + git(repo, "worktree", "add", worktree); + return { repo, worktree }; + }; + + test("a project grant minted at the session root matches a sub-agent request whose cwd is a worktree under that root", async () => { + const { runWithSubAgentIdentity } = await import("../subagent/identity-context.js"); + const { repo, worktree } = createRepoWithSiblingWorktree(); + let asked = 0; + const gate = createPermissionGate({ + approvals: [], + cwd: repo, + requestApproval: async () => { + asked++; + return { allow: true, persist: { id: "exact", label: "Always allow", pattern: "npm *", grant: "project" } }; + }, + interactive: true, + skipPermissions: false, + }); + + // First call, from the session root, mints the project grant. + const first = await gate.evaluate(shellCall("npm test")); + expect(first.allowed).toBe(true); + expect(asked).toBe(1); + + // Second call, from a sub-agent running in the sibling worktree, must + // replay the same project grant instead of asking again. + const second = await runWithSubAgentIdentity({ description: "Worker", cwd: worktree }, () => + gate.evaluate(shellCall("npm run build")), + ); + expect(second.allowed).toBe(true); + expect(asked).toBe(1); + }); + + // Security test: a project grant must never leak to a request from a + // genuinely unrelated project's directory, even though that directory is + // just as "foreign" on disk as a legitimate worktree would look to a naive + // check. Must pass both before and after the worktree-matching fix. + test("a project grant does not match a request from an unrelated project root", async () => { + const { runWithSubAgentIdentity } = await import("../subagent/identity-context.js"); + const { repo } = createRepoWithSiblingWorktree(); + const unrelated = mkdtempSync(join(tmpdir(), "corbits-unrelated-project-")); + let asked = 0; + const gate = createPermissionGate({ + approvals: [], + cwd: repo, + requestApproval: async () => { + asked++; + return { allow: true, persist: { id: "exact", label: "Always allow", pattern: "npm *", grant: "project" } }; + }, + interactive: true, + skipPermissions: false, + }); + + const first = await gate.evaluate(shellCall("npm test")); + expect(first.allowed).toBe(true); + expect(asked).toBe(1); + + const second = await runWithSubAgentIdentity({ description: "Worker", cwd: unrelated }, () => + gate.evaluate(shellCall("npm run build")), + ); + expect(second.allowed).toBe(true); + // The unrelated cwd must still ask — the grant did not leak across + // projects — even though the operator happens to approve it again here. + expect(asked).toBe(2); + }); + + // Uses write_file rather than run_shell: every bare shell token is itself + // judged for path containment against the calling agent's cwd (a + // pre-existing, unrelated restriction — see classify.ts's + // commandTargetsRestricted), so a shell command issued from a genuinely + // foreign cwd always asks regardless of any grant. write_file's subject is + // the target path, not the agent's cwd, so it isolates the thing this test + // actually checks: that an unscoped (no-cwd) grant matches irrespective of + // where the request originated. + test("session and provider-model grants still match a sub-agent request regardless of cwd", async () => { + const { runWithSubAgentIdentity } = await import("../subagent/identity-context.js"); + const { repo } = createRepoWithSiblingWorktree(); + const unrelated = mkdtempSync(join(tmpdir(), "corbits-unrelated-project-")); + const target = join(repo, "notes.md"); + let asked = 0; + const gate = createPermissionGate({ + approvals: [], + cwd: repo, + requestApproval: async () => { + asked++; + return { allow: true, persist: { id: "exact", label: "Always allow", pattern: target, grant: "session" } }; + }, + interactive: true, + skipPermissions: false, + }); + + const first = await gate.evaluate({ id: "a", name: "write_file", arguments: { path: target } }); + expect(first.allowed).toBe(true); + expect(asked).toBe(1); + + const second = await runWithSubAgentIdentity({ description: "Worker", cwd: unrelated }, () => + gate.evaluate({ id: "b", name: "write_file", arguments: { path: target } }), + ); + expect(second.allowed).toBe(true); + expect(asked).toBe(1); + }); +}); + describe("sub-agent auto-allow uses the process cwd, not the session cwd", () => { test("a relative read inside the worktree auto-allows under the process cwd", async () => { // Nested worktree under the session so absolute paths stay inside the diff --git a/src/permission/worktree-roots.ts b/src/permission/worktree-roots.ts index 50ff13765..b174b3094 100644 --- a/src/permission/worktree-roots.ts +++ b/src/permission/worktree-roots.ts @@ -7,8 +7,10 @@ const execFileAsync = promisify(execFile); // Git prints realpaths; the caller's cwd may reach the same directory through a // symlink (e.g. macOS /tmp, /var). Canonicalize both sides so self-exclusion -// compares like with like. -function realpathOr(path: string): string { +// compares like with like. Exported so other permission-layer code comparing +// a path against these roots (e.g. grant cwd matching in authz-grants.ts) +// normalizes through the same function rather than reimplementing it. +export function realpathOr(path: string): string { try { return realpathSync(path); } catch {