diff --git a/src/security.ts b/src/security.ts index 12e0019e..65bcfad9 100644 --- a/src/security.ts +++ b/src/security.ts @@ -1,4 +1,4 @@ -import { readFileSync } from "node:fs"; +import { readFileSync, realpathSync } from "node:fs"; import { resolve } from "node:path"; import { homedir } from "node:os"; @@ -429,19 +429,54 @@ export function evaluateCommandDenyOnly( * * Normalizes backslashes to forward slashes before matching so that * Windows paths work with Unix-style glob patterns. + * + * When `projectRoot` is supplied, the path is also matched in its + * fully-resolved absolute form **and** — when the file exists — in + * its canonical form (`fs.realpathSync`). This prevents two classes + * of bypass: + * + * 1. `..` traversal: a relative path like `../../.ssh/id_rsa` no + * longer evades absolute-path deny rules. + * 2. Symlink escape: a project-local path whose realpath points + * outside the project (e.g. `safe.log -> ~/.ssh/id_rsa`) no + * longer evades absolute-path deny rules. + * + * realpath is best-effort: if the file does not exist yet (ENOENT) + * or the syscall fails for any reason, the lexical resolved form is + * still checked. This keeps the function usable for paths that will + * be created during execution. */ export function evaluateFilePath( filePath: string, denyGlobs: string[][], caseInsensitive: boolean = process.platform === "win32", + projectRoot?: string, ): { denied: boolean; matchedPattern?: string } { - // Normalize backslashes to forward slashes for cross-platform matching - const normalized = filePath.replace(/\\/g, "/"); + const toForward = (path: string): string => path.replace(/\\/g, "/"); + + // Match against the raw input, the lexically-resolved absolute path, + // and the canonical (symlink-resolved) path when the file exists. + // Deduplicated so absolute inputs and paths that don't cross symlinks + // don't pay the matching cost multiple times. + const candidates = new Set(); + candidates.add(toForward(filePath)); + if (projectRoot) { + const lexical = resolve(projectRoot, filePath); + candidates.add(toForward(lexical)); + try { + candidates.add(toForward(realpathSync(lexical))); + } catch { + // File does not exist yet, or realpath failed — rely on lexical form. + } + } for (const globs of denyGlobs) { for (const glob of globs) { - if (fileGlobToRegex(glob, caseInsensitive).test(normalized)) { - return { denied: true, matchedPattern: glob }; + const regex = fileGlobToRegex(glob, caseInsensitive); + for (const candidate of candidates) { + if (regex.test(candidate)) { + return { denied: true, matchedPattern: glob }; + } } } } diff --git a/src/server.ts b/src/server.ts index 93249a91..8ddeb61f 100644 --- a/src/server.ts +++ b/src/server.ts @@ -371,8 +371,14 @@ function checkFilePathDenyPolicy( toolName: string, ): ToolResult | null { try { - const denyGlobs = readToolDenyPatterns("Read", process.env.CLAUDE_PROJECT_DIR); - const result = evaluateFilePath(filePath, denyGlobs); + const projectDir = process.env.CLAUDE_PROJECT_DIR ?? process.cwd(); + const denyGlobs = readToolDenyPatterns("Read", projectDir); + const result = evaluateFilePath( + filePath, + denyGlobs, + process.platform === "win32", + projectDir, + ); if (result.denied) { return trackResponse(toolName, { content: [{ diff --git a/tests/hooks/tool-naming.test.ts b/tests/hooks/tool-naming.test.ts index 34afb550..0ce6bdcb 100644 --- a/tests/hooks/tool-naming.test.ts +++ b/tests/hooks/tool-naming.test.ts @@ -53,7 +53,7 @@ const mcpSentinel = resolve(tmpdir(), `context-mode-mcp-ready-${process.ppid}`); beforeEach(() => { if (typeof resetGuidanceThrottle === "function") resetGuidanceThrottle(); - writeFileSync(mcpSentinel, String(process.pid)); + writeFileSync(mcpSentinel, String(process.ppid)); }); afterEach(() => { diff --git a/tests/security.test.ts b/tests/security.test.ts index 9796c3a4..eeabc0e6 100644 --- a/tests/security.test.ts +++ b/tests/security.test.ts @@ -8,8 +8,8 @@ import { describe, test, beforeAll, afterAll } from "vitest"; import { strict as assert } from "node:assert"; import { writeFileSync, mkdirSync, rmSync } from "node:fs"; -import { join } from "node:path"; -import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { tmpdir, homedir } from "node:os"; import { parseBashPattern, @@ -474,6 +474,36 @@ describe("evaluateFilePath", () => { assert.equal(result.denied, true); assert.equal(result.matchedPattern, "**/.env"); }); + + test("evaluateFilePath: traversal does not bypass absolute deny glob when projectRoot is supplied", () => { + // An absolute deny rule for ~/.ssh/** should still match when the caller + // passes a ../-traversal relative path that resolves into ~/.ssh. + const home = homedir(); + const projectRoot = resolve(home, "some/project"); + const denyGlob = resolve(home, ".ssh").replace(/\\/g, "/") + "/**"; + + const result = evaluateFilePath( + "../../.ssh/id_rsa", + [[denyGlob]], + process.platform === "win32", + projectRoot, + ); + assert.equal(result.denied, true); + assert.equal(result.matchedPattern, denyGlob); + }); + + test("evaluateFilePath: without projectRoot, absolute deny glob is still bypassable (regression guard)", () => { + // Documents the pre-fix behavior: without projectRoot, `..` is not + // resolved, so the raw string doesn't match the absolute glob. + // This test exists so any change in behavior is intentional. + const absoluteSshGlob = resolve(homedir(), ".ssh").replace(/\\/g, "/") + "/**"; + const result = evaluateFilePath( + "../../.ssh/id_rsa", + [[absoluteSshGlob]], + process.platform === "win32", + ); + assert.equal(result.denied, false); + }); }); describe("Shell-Escape Scanner", () => {