diff --git a/src/permission/authz-grants.ts b/src/permission/authz-grants.ts index 6c8bcef51..23adde94d 100644 --- a/src/permission/authz-grants.ts +++ b/src/permission/authz-grants.ts @@ -6,7 +6,7 @@ import { grantToolCovers, } from "../agent/canonical-tool-name.js"; import type { Approval } from "./types.js"; -import { matchesPattern } from "./matcher.js"; +import { directoryGrantAllows, matchesPattern } from "./matcher.js"; import { realpathOr } from "./worktree-roots.js"; // Exact-escaped patterns (backslash before metacharacters) cannot round-trip @@ -147,7 +147,17 @@ export async function approvalCoversSubject( ); if (scoped.length === 0) return false; - for (const a of scoped) { + // Directory Always grants (`/*`) cannot reach the package evaluator + // with a `..` walk-out subject: package `*` matches `..` lexically, so the + // same containment gate matchesPattern enforces applies here first. The + // gate only denies directory escapes; every other grant defers untouched. + const effectiveCwd = requestCwd ?? workspace.resolvedCwd; + const contained = scoped.filter((a) => + directoryGrantAllows(a.pattern, subject, effectiveCwd), + ); + if (contained.length === 0) return false; + + for (const a of contained) { if ( !isPackageCompatiblePattern(a.pattern) && matchesPattern(subject, a.pattern) @@ -156,7 +166,7 @@ export async function approvalCoversSubject( } } - const grants = scoped + const grants = contained .filter((a) => isPackageCompatiblePattern(a.pattern)) .map((a, i) => approvalToGrantRule(a, i)); if (grants.length === 0) return false; diff --git a/src/permission/classify.ts b/src/permission/classify.ts index f92f6c9dd..ff48530d5 100644 --- a/src/permission/classify.ts +++ b/src/permission/classify.ts @@ -24,6 +24,7 @@ import { runShellAuthzSegmentBlockReason, } from "../shell/run-shell-authz.js"; import { resolveWorkspacePath } from "./path-restriction.js"; +import { normalizeGrantPath } from "./matcher.js"; import type { RootsProvider } from "./worktree-roots.js"; import { isProductMutationTool, @@ -551,7 +552,7 @@ function fileScopes(path: string): ApprovalScope[] { ]; const slash = path.lastIndexOf("/"); if (slash > 0) { - const dir = path.slice(0, slash); + const dir = normalizeGrantPath(path.slice(0, slash)); scopes.push({ id: "dir", label: `Allow Always (this directory)`, diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 028598739..baa3f3262 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -327,7 +327,13 @@ function isRequestCoveredByApprovals( ); if (scoped.length === 0) return false; if (request.tool !== "run_shell") { - return scoped.some((a) => matchesPattern(request.subject, a.pattern)); + return scoped.some((a) => + matchesPattern( + request.subject, + a.pattern, + request.cwd ?? workspace.resolvedCwd, + ), + ); } if ( preGrantGuardReason(request, isRestricted, rootsProvider, isExtraDenied) !== @@ -340,7 +346,8 @@ function isRequestCoveredByApprovals( if (segments.length === 0) return false; const cwd = request.cwd ?? workspace.resolvedCwd; return segments.every((segment) => { - if (scoped.some((a) => matchesPattern(segment, a.pattern))) return true; + if (scoped.some((a) => matchesPattern(segment, a.pattern, cwd))) + return true; return isAutoAllowedShellSegment( segment, cwd, diff --git a/src/permission/matcher.ts b/src/permission/matcher.ts index 9fc5b2fed..54e41ca1a 100644 --- a/src/permission/matcher.ts +++ b/src/permission/matcher.ts @@ -1,3 +1,4 @@ +import { posix } from "node:path"; import { matchPattern } from "@intx/authz"; // Exact-command grants (see escapeGlobLiteral) store a backslash before every @@ -30,9 +31,56 @@ export function escapeGlobLiteral(text: string): string { return text.replace(/[\\*?]/g, "\\$&"); } -export function matchesPattern(subject: string, pattern: string): boolean { +export function matchesPattern( + subject: string, + pattern: string, + cwd?: string, +): boolean { if (isExactEscapedPattern(pattern)) { return subject === unescapeExactPattern(pattern); } + if (!directoryGrantAllows(pattern, subject, cwd)) return false; return matchPattern(pattern, subject); } + +// Lexically normalize a grant path (POSIX, no fs I/O): collapse `.`, `..`, +// and duplicate slashes. Absolute paths normalize in place; relative paths +// resolve against cwd when one is supplied, else collapse in place. Shared by +// the matcher gate below and file-scope minting so both sides agree on what +// a directory grant covers. +export function normalizeGrantPath(path: string, cwd?: string): string { + if (posix.isAbsolute(path)) return posix.normalize(path); + if (cwd !== undefined) return posix.normalize(posix.resolve(cwd, path)); + return posix.normalize(path); +} + +// Containment gate for Directory Always grants (`/*`): the package +// `*` matches `..` lexically, so a subject like `/proj/sub/../evil` would +// otherwise match `/proj/sub/*` and escape the granted directory. A +// normalized subject must sit strictly under the anchor (`anchor/` prefix), +// or the grant does not cover it — the grant covers directory contents, so +// even the anchor itself (`/proj/sub/.` normalizes to `/proj/sub`) does not +// match. Returns true for every non-directory pattern (exact-escaped, +// non-`/*`, or non-literal anchors) so those defer to their existing matcher +// untouched — the gate only ever denies, never allows. +export function directoryGrantAllows( + pattern: string, + subject: string, + cwd?: string, +): boolean { + if (pattern.includes("\\")) return true; + if (!pattern.endsWith("/*")) return true; + const anchor = pattern.slice(0, -2); + if (anchor.includes("*") || anchor.includes("?")) return true; + const base = anchor === "" ? "/" : anchor; + const normalizedAnchor = stripTrailingSlash(normalizeGrantPath(base, cwd)); + const normalizedSubject = stripTrailingSlash( + normalizeGrantPath(subject, cwd), + ); + const prefix = normalizedAnchor === "/" ? "/" : `${normalizedAnchor}/`; + return normalizedSubject.startsWith(prefix); +} + +function stripTrailingSlash(path: string): string { + return path.length > 1 ? path.replace(/\/+$/, "") : path; +} diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index 7bb19ca06..8dbb3f18c 100644 --- a/src/permission/permission.test.ts +++ b/src/permission/permission.test.ts @@ -354,6 +354,51 @@ describe("matchesPattern (@intx/authz + exact escapes)", () => { }); }); +describe("matchesPattern directory-grant traversal (CL-8989)", () => { + // A Directory Always grant (`/proj/sub/*`) must never authorize a subject + // that lexically escapes the granted directory, even though `*` matches + // `/`, `.`, and `..` at the string level. Legit depth semantics (`*` + // crosses `/`) are locked in first so the fix cannot narrow real grants. + const grant = "/proj/sub/*"; + + test("* still crosses directories for legit paths", () => { + expect(matchesPattern("/proj/sub/file.txt", grant)).toBe(true); + expect(matchesPattern("/proj/sub/nested/file.txt", grant)).toBe(true); + }); + + test(".. walk-out subjects do not match the directory grant", () => { + expect(matchesPattern("/proj/sub/../evil", grant)).toBe(false); + expect(matchesPattern("/proj/sub/../../etc/passwd", grant)).toBe(false); + expect(matchesPattern("/proj/sub/.", grant)).toBe(false); + expect(matchesPattern("/proj/sub//../evil", grant)).toBe(false); + }); + + test("trailing-slash anchor spellings do not match the directory grant", () => { + expect(matchesPattern("/proj/sub/", grant)).toBe(false); + expect(matchesPattern("/proj/sub//", grant)).toBe(false); + }); + + test("relative subjects resolve against cwd through the gate", () => { + expect(matchesPattern("sub/file.txt", "sub/*", "/proj")).toBe(true); + expect(matchesPattern("sub/", "sub/*", "/proj")).toBe(false); + expect(matchesPattern("sub", "sub/*", "/proj")).toBe(false); + }); + + test("a .. subject that normalizes back inside still matches", () => { + expect(matchesPattern("/proj/sub/../sub/file.txt", grant)).toBe(true); + }); + + test("sibling-prefix paths do not match the directory grant", () => { + expect(matchesPattern("/proj/sub-evil/file.txt", grant)).toBe(false); + expect(matchesPattern("/proj/subfile.txt", grant)).toBe(false); + }); + + test("non-path wildcard grants keep their existing semantics", () => { + expect(matchesPattern("npm exec vite", "npm *")).toBe(true); + expect(matchesPattern("src/a.ts", "src/*")).toBe(true); + }); +}); + describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { const approvals: Approval[] = [ { tool: "run_shell", pattern: "npm *" }, @@ -394,6 +439,38 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { ).toBe(true); }); + test("a directory grant denies .. walk-out through the package path (CL-8989)", async () => { + // evaluateApprovals delegates package-compatible grants to @intx/authz + // evaluateGrants, whose `*` matches `..` lexically — the containment gate + // must hold on this path too, while legit nested paths still match. + const dirApprovals: Approval[] = [ + { tool: "write_file", pattern: "/proj/sub/*" }, + ]; + const workspace = { resolvedCwd: "/proj", roots: [] as string[] }; + expect( + await evaluateApprovals({ + tool: "write_file", + subject: "/proj/sub/nested/file.txt", + approvals: dirApprovals, + workspace, + }), + ).toBe(true); + for (const subject of [ + "/proj/sub/../evil", + "/proj/sub/../../etc/passwd", + "/proj/sub//../evil", + ]) { + expect( + await evaluateApprovals({ + tool: "write_file", + subject, + approvals: dirApprovals, + workspace, + }), + ).toBe(false); + } + }); + test("a grant for read_file covers default.read_file", async () => { expect( await evaluateApprovals({