diff --git a/src/permission/auto-shell-policy.ts b/src/permission/auto-shell-policy.ts index cf190bd2a..d05176bef 100644 --- a/src/permission/auto-shell-policy.ts +++ b/src/permission/auto-shell-policy.ts @@ -523,7 +523,7 @@ export function autoShellRuleForCall( } for (const subject of subjects) { - if (commandReferencesSensitivePath(subject) !== undefined) + if (commandReferencesSensitivePath(subject, cwd) !== undefined) return SENSITIVE_PATH_ASK_RULE; } diff --git a/src/permission/classify.ts b/src/permission/classify.ts index 0fd024c1a..9e2f95ba9 100644 --- a/src/permission/classify.ts +++ b/src/permission/classify.ts @@ -15,7 +15,8 @@ import { import type { McpToolPermissionRegistry } from "../mcp/tool-permissions.js"; import { commandReferencesSensitivePath, - isSensitivePath, + isSensitiveShellToken, + PURE_DIRECTORY_LISTING_PROGRAMS, } from "../plugins/secret-guard-plugin.js"; import { runShellAuthzBlockReason, @@ -113,10 +114,10 @@ export function restrictedPathArg( return isRestricted(path, isWriteTool(call.name)) ? path : undefined; } -// Programs that only print directory names / metadata. Outside-workspace path -// arguments are fine for these — listing is not a content read. Content readers -// (cat, head, xxd, …) still fail the restricted-path check below. -const PURE_DIRECTORY_LISTING_PROGRAMS = new Set(["ls", "tree"]); +// Outside-workspace path arguments are fine for pure-listing programs — listing +// is not a content read. Content readers (cat, head, xxd, …) still fail the +// restricted-path check below. The program set itself is owned by +// secret-guard-plugin.ts (shared with the CL-7790 resolve-leg skip). // Cap accepted tree depth so `tree -L 999999 /` cannot auto-allow an OOM walk. const MAX_PURE_TREE_DEPTH = 10; @@ -405,7 +406,7 @@ function isAutoAllowedSegment( const trimmed = segment.trim(); if (trimmed.length === 0) return false; if (isShellCommentOnly(trimmed) || isShellNoOp(trimmed)) return true; - if (commandReferencesSensitivePath(trimmed)) return false; + if (commandReferencesSensitivePath(trimmed, cwd)) return false; // Same metacharacter gate as isAutoAllowedShellCommand: this classifier also // runs standalone per pipeline/chain segment (see isAutoAllowedShellSegment), // so a segment carrying its own command substitution or redirect must not @@ -429,7 +430,12 @@ function isAutoAllowedSegment( if (args.some((token) => WRITE_FLAG.test(token))) return false; if (args.some((token) => EXEC_FLAG.test(token))) return false; } - if (args.some((token) => isSensitivePath(token))) return false; + // CL-7790: resolve symlinks before the secret denylist — a benign-named + // symlink into a secret file (notes.txt -> .env) asks exactly like the + // secret name itself. Pure name-listings skip the resolve leg: `ls + // notes.txt` lists freely (CL-5420), and an impure listing fails above. + if (args.some((token) => isSensitiveShellToken(token, cwd, !pureListing))) + return false; // Pure directory listing may target outside-workspace paths (names only). // Content readers must stay inside the workspace. if ( @@ -457,7 +463,7 @@ export function isAutoAllowedShellCommand( (isShellCommentOnly(trimmed) || isShellNoOp(trimmed)) ) return true; - if (commandReferencesSensitivePath(trimmed)) return false; + if (commandReferencesSensitivePath(trimmed, cwd)) return false; // Never auto-allow a command the authz layer would hard-deny at execution. if (runShellAuthzBlockReason(trimmed) !== undefined) return false; // Reject anything with metacharacters that compose or redirect (& ; < > ` $ etc). diff --git a/src/permission/gate.ts b/src/permission/gate.ts index e977ea166..a3810da64 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -131,7 +131,7 @@ function segmentGuard( cwd?: string, rootsProvider?: RootsProvider, ): SegmentGuard | undefined { - if (commandReferencesSensitivePath(segment) !== undefined) + if (commandReferencesSensitivePath(segment, cwd) !== undefined) return { kind: "secret" }; if ( cwd !== undefined && @@ -654,7 +654,7 @@ export function createPermissionGate( // segment mentions a secret path. const shellReferencesSecret = shellCmd !== undefined && - commandReferencesSensitivePath(shellCmd) !== undefined; + commandReferencesSensitivePath(shellCmd, effectiveCwd) !== undefined; if (!restricted && classifyTool(call.name, mcpTiers) === "allow") { return { kind: "allow" }; } @@ -949,7 +949,8 @@ export function createPermissionGate( ) => { const anySecret = request.tool === "run_shell" && - commandReferencesSensitivePath(request.subject) !== undefined; + commandReferencesSensitivePath(request.subject, request.cwd) !== + undefined; return resolveInteractiveAsk( { kind: "ask", diff --git a/src/plugins/secret-guard-plugin.ts b/src/plugins/secret-guard-plugin.ts index 1caebdcc1..2c98745aa 100644 --- a/src/plugins/secret-guard-plugin.ts +++ b/src/plugins/secret-guard-plugin.ts @@ -1,4 +1,10 @@ -import { isAbsolute } from "node:path"; +import { lstatSync } from "node:fs"; +import { homedir } from "node:os"; +import { + isAbsolute, + join as joinPath, + resolve as resolvePath, +} from "node:path"; import type { ToolPlugin } from "@intx/tools-posix"; import { realpathNearestOr, @@ -132,14 +138,126 @@ function shellPathTokens(command: string): string[] { // dynamic construction of a path the matcher never sees as one token — e.g. // indirection through an unrelated variable (`F=.en; cat ${F}v`), character-by- // character assembly (`printf`), or reading via an interpreter that builds the -// name at runtime. Perfect shell sandboxing is out of scope; the goal is to -// force a prompt for the trivial, single-token references that make exfiltration -// easy. Tool-result secret scrub still redacts credential-shaped output. +// name at runtime. Unexpanded globs are the same class: `cat *` can open a +// symlink the matcher only ever saw as `*`. Perfect shell sandboxing is out +// of scope; the goal is to force a prompt for the trivial, single-token +// references that make exfiltration easy. Tool-result secret scrub still redacts credential-shaped output. +// Programs that only print directory names / metadata — listing a name never +// dumps file contents. Single owner for this set: the resolve-leg skip below +// and classify.ts's pure-listing exemption both read it, so a new names-only +// program cannot drift into one list without the other. +export const PURE_DIRECTORY_LISTING_PROGRAMS = new Set(["ls", "tree"]); + +// Worth spending a realpath on: shaped like a path the shell could open +// (a slash, an extension dot, or absolute), not a flag, variable, or fd +// number — those can never name a file the shell opens, so they skip the +// stat and the hot auto-allow path stays syscall-free for them. Globs are +// skipped here for a different reason: the matcher only sees the unexpanded +// pattern, so `cat *.txt` cannot resolve without running the shell — but a +// glob CAN expand into a symlink at runtime, which stays a stated residual +// (see the threat model below), not something this filter disproves. +function isPathLikeShellToken(token: string): boolean { + if ( + token.startsWith("-") || + token.includes("$") || + token.includes("*") || + token.includes("`") + ) + return false; + return ( + isAbsolute(token) || + token.includes("/") || + token.includes("\\") || + token.includes(".") + ); +} + +// `~` / `~/…` mean the operator's home to the shell, not a literal +// cwd-relative name — expand before both matcher legs so `cat ~/notes` +// resolves the home symlink instead of a (usually missing) cwd child. +// classify.ts's outside-workspace rule would ask anyway; the expansion fixes +// the *reason* (sensitive-path) rather than relying on that coincidence. +function expandHome(token: string): string { + if (token === "~") return homedir(); + if (token.startsWith("~/")) return joinPath(homedir(), token.slice(2)); + return token; +} + +// A bare token the shell could open as a cwd-relative file: not a flag, +// variable, glob, or command substitution — same exclusions as the path-like +// filter, minus the dot/slash shape requirement, so extensionless names +// (`notes`, or `notes` split out of `--file=notes` / `cat -n notes`) still +// get an existence probe below. +function isBareProbeCandidate(token: string): boolean { + return ( + token.length > 0 && + !token.startsWith("-") && + !token.includes("$") && + !token.includes("*") && + !token.includes("`") + ); +} + +// CL-7790: the ONE shell-token matcher both secret-guard call sites share — +// commandReferencesSensitivePath below and classify.ts's per-arg sensitive +// check. The cheap lexical denylist runs first so the hot auto-allow path +// never touches the filesystem; only survivors pay for filesystem access, in +// two bounded tiers: path-like tokens pay for a realpath via the CL-6971 +// helper, which catches a benign-named symlink into a secret file (notes.txt +// -> .env) exactly like the secret name itself, while bare extensionless +// tokens first pay a single lstat existence probe against the cwd-resolved +// path — a miss (the common `cat Makefile` case) costs exactly that one +// lstat and skips the resolve, a hit (file or symlink, dangling included) +// pays the realpath and matches on the target. Flags, variables, globs, and +// backticks never probe, so the worst case per command is one lstat per bare +// token plus one realpath per existing entry. Relative tokens resolve +// against cwd first because the helper takes absolute paths; `~` expands to +// the home directory before resolving for the same reason. That cwd is the +// session/process cwd, not a `cd` prefix inside the command — +// `cd sub && cat notes.txt` resolves `notes.txt` against the session cwd +// (absent) rather than cwd/sub (present). The chain still fails closed +// because `cd` is not a safe program, but no secret reason fires; +// per-segment `cd` modeling is deliberately out of scope. +// Pass resolveSymlinks=false for pure name-listings: listing a name is not +// dumping its contents (CL-5420), so `ls notes.txt` still lists freely while +// `cat notes.txt` asks. +export function isSensitiveShellToken( + token: string, + cwd: string = process.cwd(), + resolveSymlinks = true, +): boolean { + const expanded = expandHome(token); + if (isSensitivePath(expanded)) return true; + if (!resolveSymlinks) return false; + if (isPathLikeShellToken(expanded)) { + if (isAbsolute(expanded)) return isSensitivePathResolved(expanded); + return isSensitivePathResolved(resolvePath(cwd, expanded)); + } + if (!isBareProbeCandidate(expanded)) return false; + const abs = isAbsolute(expanded) ? expanded : resolvePath(cwd, expanded); + try { + lstatSync(abs); + } catch { + return false; + } + return isSensitivePathResolved(abs); +} + export function commandReferencesSensitivePath( command: string, + cwd: string = process.cwd(), ): string | undefined { - for (const token of shellPathTokens(command)) { - if (isSensitivePath(token)) return token; + const tokens = shellPathTokens(command); + // Dump vs list: a lone name-listing never dumps file contents, so only the + // cheap lexical leg applies and `ls notes.txt` still lists freely. Anything + // composed (pipes, chains, redirects, subshells) takes the resolve leg — + // `ls && cat notes.txt` must not ride the listing exemption. + const program = tokens[0] ?? ""; + const listingOnly = + PURE_DIRECTORY_LISTING_PROGRAMS.has(program) && + !/[;&|()<>\n]/.test(command); + for (const token of tokens) { + if (isSensitiveShellToken(token, cwd, !listingOnly)) return token; } return undefined; } diff --git a/src/plugins/secret-guard-shell-symlink.test.ts b/src/plugins/secret-guard-shell-symlink.test.ts new file mode 100644 index 000000000..7d453a613 --- /dev/null +++ b/src/plugins/secret-guard-shell-symlink.test.ts @@ -0,0 +1,187 @@ +import { describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + commandReferencesSensitivePath, + isSensitiveShellToken, +} from "./secret-guard-plugin.js"; +import { isAutoAllowedShellCommand } from "../permission/classify.js"; +import { autoShellRuleForCall } from "../permission/auto-shell-policy.js"; + +/** + * CL-7790: shell token matching ignores symlinks. A benign-named symlink + * into a secret file (notes.txt -> .env) must not auto-allow a content dump + * (`cat notes.txt`), identically to the direct name (`cat .env`). Pure + * name-listing (`ls notes.txt`) still lists freely — dumping contents is the + * threat, listing a name is not (dump-vs-list distinction, CL-5420). + */ + +async function withFixture( + run: (paths: { cwd: string }) => Promise, +): Promise { + const cwd = await mkdtemp(join(tmpdir(), "cl7790-shell-symlink-")); + try { + await writeFile(join(cwd, ".env"), "SECRET=fixture-env\n"); + await writeFile(join(cwd, "README.md"), "# fixture\n"); + await symlink(join(cwd, ".env"), join(cwd, "notes.txt")); + // Extensionless twin of notes.txt: no dot, no slash. + await symlink(join(cwd, ".env"), join(cwd, "notes")); + // A link visible only from a subdirectory, for the cd-prefix case. + await mkdir(join(cwd, "sub"), { recursive: true }); + await symlink(join(cwd, ".env"), join(cwd, "sub", "secret-link.txt")); + return await run({ cwd }); + } finally { + await rm(cwd, { recursive: true, force: true }); + } +} + +// A `~`-reachable secret and a benign-named link into it. Bun's homedir() +// does not follow a runtime-overridden $HOME, so the fixture lives in the +// real home directory under unique per-run names (never the real ~/.env) +// and is removed in `finally`. +async function withHomeFixture( + run: (paths: { linkName: string }) => Promise, +): Promise { + const { homedir } = await import("node:os"); + const home = homedir(); + const tag = `cl7790-probe-${process.pid}-${Math.floor(Math.random() * 1e9)}`; + const secretName = `${tag}.pem`; + const linkName = `${tag}-notes`; + try { + await writeFile(join(home, secretName), "SECRET=fixture-home-pem\n"); + await symlink(join(home, secretName), join(home, linkName)); + return await run({ linkName }); + } finally { + await rm(join(home, linkName), { force: true }); + await rm(join(home, secretName), { force: true }); + } +} + +const shellCall = (command: string) => ({ + id: "c", + name: "run_shell", + arguments: { command }, +}); + +describe("CL-7790 shell tokens resolve symlinks before the secret denylist", () => { + test("cat through a benign-named symlink does not auto-allow", async () => { + await withFixture(async ({ cwd }) => { + expect(isAutoAllowedShellCommand("cat notes.txt", cwd)).toBe(false); + }); + }); + + test("cat of the direct secret name still does not auto-allow", async () => { + await withFixture(async ({ cwd }) => { + expect(isAutoAllowedShellCommand("cat .env", cwd)).toBe(false); + }); + }); + + test("pure listing of the symlink still lists freely", async () => { + await withFixture(async ({ cwd }) => { + expect(isAutoAllowedShellCommand("ls notes.txt", cwd)).toBe(true); + expect(isAutoAllowedShellCommand("ls -la", cwd)).toBe(true); + }); + }); + + test("pure listing of the direct secret name still asks (CL-5420)", async () => { + await withFixture(async ({ cwd }) => { + expect(isAutoAllowedShellCommand("ls .env", cwd)).toBe(false); + }); + }); + + test("plugin flags the symlinked dump but not the listing", async () => { + await withFixture(async ({ cwd }) => { + expect(commandReferencesSensitivePath("cat notes.txt", cwd)).toBe( + "notes.txt", + ); + expect( + commandReferencesSensitivePath("ls notes.txt", cwd), + ).toBeUndefined(); + }); + }); + + test("shared helper matches the resolved target, not just the lexical name", async () => { + await withFixture(async ({ cwd }) => { + expect(isSensitiveShellToken("notes.txt", cwd)).toBe(true); + // The listing leg never resolves: names are not contents. + expect(isSensitiveShellToken("notes.txt", cwd, false)).toBe(false); + expect(isSensitiveShellToken("README.md", cwd)).toBe(false); + }); + }); + + test("auto mode asks for the symlinked dump, not the listing", async () => { + await withFixture(async ({ cwd }) => { + expect( + autoShellRuleForCall(shellCall("cat notes.txt"), () => false, cwd) + ?.name, + ).toBe("sensitive-path"); + expect( + autoShellRuleForCall(shellCall("ls notes.txt"), () => false, cwd), + ).toBeUndefined(); + }); + }); + + test("cat through an extensionless symlink does not auto-allow", async () => { + await withFixture(async ({ cwd }) => { + expect(isAutoAllowedShellCommand("cat notes", cwd)).toBe(false); + expect(commandReferencesSensitivePath("cat notes", cwd)).toBe("notes"); + expect(isSensitiveShellToken("notes", cwd)).toBe(true); + }); + }); + + test("flag-adjacent bare names do not auto-allow", async () => { + await withFixture(async ({ cwd }) => { + // `=` splits `--file=notes` into a bare `notes` token; `-n` is a flag. + expect(isAutoAllowedShellCommand("cat -n notes", cwd)).toBe(false); + expect(commandReferencesSensitivePath("cat -n notes", cwd)).toBe("notes"); + expect(isAutoAllowedShellCommand("grep --file=notes foo", cwd)).toBe( + false, + ); + }); + }); + + test("missing bare names still auto-allow (no false positive on a miss)", async () => { + await withFixture(async ({ cwd }) => { + // Nothing named Makefile exists in the fixture: the existence probe + // misses and the command stays auto-allowed. + expect(isAutoAllowedShellCommand("cat Makefile", cwd)).toBe(true); + expect( + commandReferencesSensitivePath("cat Makefile", cwd), + ).toBeUndefined(); + }); + }); + + test("home-relative link asks for the secret reason, not just outside-workspace", async () => { + await withFixture(async ({ cwd }) => { + await withHomeFixture(async ({ linkName }) => { + const command = `cat ~/${linkName}`; + // classify.ts would already ask here via the `~` outside-workspace + // rule; the point of `~` expansion is that the *secret* reason fires. + expect(commandReferencesSensitivePath(command, cwd)).toBe( + `~/${linkName}`, + ); + expect(isAutoAllowedShellCommand(command, cwd)).toBe(false); + expect( + autoShellRuleForCall(shellCall(command), () => false, cwd)?.name, + ).toBe("sensitive-path"); + }); + }); + }); + + test("cd-prefixed dump fails closed; per-segment cd tracking is out of scope", async () => { + await withFixture(async ({ cwd }) => { + // Relative tokens resolve against the session cwd, not a `cd` prefix + // inside the command — the shell would open cwd/sub/secret-link.txt but + // the secret leg only sees cwd/secret-link.txt (absent), so no secret + // reason fires here. The chain still asks because `cd` is not a safe + // program; per-segment `cd` modeling is deliberately not attempted. + expect( + isAutoAllowedShellCommand("cd sub && cat secret-link.txt", cwd), + ).toBe(false); + expect( + commandReferencesSensitivePath("cd sub && cat secret-link.txt", cwd), + ).toBeUndefined(); + }); + }); +}); diff --git a/src/plugins/tool-result-secret-scrub.ts b/src/plugins/tool-result-secret-scrub.ts index 0c28ec78c..3526f53a9 100644 --- a/src/plugins/tool-result-secret-scrub.ts +++ b/src/plugins/tool-result-secret-scrub.ts @@ -27,6 +27,11 @@ const JSON_CREDENTIAL_FIELD = // Grep/shell lines often look like path:line:KEY=value const ENV_ASSIGNMENT = /(?:^|:)([A-Z][A-Z0-9_]+)=([^\n]+)/gm; +// CL-7790 decision: connection-string keys (DATABASE_URL and friends) are +// deliberately NOT matched here. Widening this shape-classifier would redact +// every benign connection string in tool output — a false-positive blast +// radius on a scrub path, not a prompt path. That needs its own measured +// ticket; the gap stays documented, not silently fixed. function isSecretEnvKey(key: string): boolean { return ( key === "API_KEY" ||