Skip to content

Commit b456cf5

Browse files
WebMadWebMad
committed
fix(read-file): preserve approval privacy and symlink allowlists
Defer filesystem resolution diagnostics until approval while retaining cancellation and fail-closed target identity checks. Match equivalent task workspace roots in one ordered read allowlist without granting external file aliases, changing write policy, or reanchoring escaping relative patterns. Split root normalization from prefix replacement and cover error, device identity, Windows, negation, and multi-root boundaries.
1 parent e1d7a61 commit b456cf5

7 files changed

Lines changed: 520 additions & 36 deletions

File tree

‎src/core/auto-approval/__tests__/windowsPaths.spec.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,4 +99,34 @@ describe("matching on Windows", () => {
9999
expect(matches("C:/path/to/repo/etc/passwd", ["passwd"])).toBe(true)
100100
expect(matches("C:/etc/passwd", ["passwd"])).toBe(false)
101101
})
102+
103+
it("matches equivalent workspace roots across drives with Windows case and ordered exclusions", () => {
104+
const rootMatch = (patterns: string[]) =>
105+
matcher().isFileMatchedByPatterns({
106+
filePath: "d:\\storage\\project\\DOCS\\NOTES.md",
107+
cwd: "C:\\Aliases\\Project",
108+
canonicalCwd: "D:\\Storage\\Project",
109+
patterns,
110+
})
111+
expect(rootMatch(["docs/**"])).toBe(true)
112+
expect(rootMatch(["c:/aliases/PROJECT/docs/**"])).toBe(true)
113+
expect(rootMatch(["d:/STORAGE/project/docs/**"])).toBe(true)
114+
expect(rootMatch(["docs/**", "!c:/aliases/project/docs/notes.md"])).toBe(false)
115+
expect(rootMatch(["docs/**", "!D:/Storage/Project/docs/notes.md"])).toBe(false)
116+
})
117+
118+
it("keeps Windows siblings and lexical-parent outside patterns separate from canonical roots", () => {
119+
const rootMatch = (filePath: string, patterns: string[]) =>
120+
matcher().isFileMatchedByPatterns({
121+
filePath,
122+
cwd: "C:\\Aliases\\Project",
123+
canonicalCwd: "D:\\Storage\\Project",
124+
patterns,
125+
})
126+
expect(rootMatch("C:/Aliases/shared/file.md", ["../shared/file.md"])).toBe(true)
127+
expect(rootMatch("D:/Storage/shared/file.md", ["../shared/file.md"])).toBe(false)
128+
expect(rootMatch("D:/Storage/shared/file.md", ["D:/Storage/shared/file.md"])).toBe(true)
129+
expect(rootMatch("D:/Storage/Project-other/docs/file.md", ["docs/**"])).toBe(false)
130+
expect(rootMatch("D:/Storage/Project-other/docs/file.md", ["C:/Aliases/Project-other/docs/**"])).toBe(false)
131+
})
102132
})

‎src/core/auto-approval/__tests__/workspaceRootEquivalence.spec.ts‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
import * as fs from "fs/promises"
2+
import type { ClineSayTool } from "@roo-code/types"
3+
import { checkAutoApproval } from "../index"
4+
import { isFileMatchedByPatterns } from "../filePatterns"
5+
import { baseState, type State } from "./fixtures"
6+
7+
vi.mock("fs/promises", async (importOriginal) => {
8+
const actual = await importOriginal<typeof import("fs/promises")>()
9+
return { ...actual, realpath: vi.fn() }
10+
})
11+
12+
const CWD = "/aliases/project"
13+
const CANONICAL_CWD = "/storage/project"
14+
15+
function matches(filePath: string, patterns: string[]) {
16+
return isFileMatchedByPatterns({ filePath, cwd: CWD, canonicalCwd: CANONICAL_CWD, patterns, isWindows: false })
17+
}
18+
19+
function approval(tool: ClineSayTool, state: Partial<State>, cwd: string | undefined = CWD, isProtected = false) {
20+
return checkAutoApproval({
21+
state: { ...baseState, ...state },
22+
cwd,
23+
ask: "tool",
24+
text: JSON.stringify(tool),
25+
isProtected,
26+
})
27+
}
28+
29+
beforeEach(() => {
30+
vi.mocked(fs.realpath).mockReset().mockResolvedValue(CANONICAL_CWD)
31+
})
32+
33+
describe("read matcher workspace-root equivalence", () => {
34+
it.each([CWD, CANONICAL_CWD])("honors %s exclusions and later re-inclusion in one ordered matcher", (root) => {
35+
const candidate = `${CANONICAL_CWD}/docs/private.md`
36+
const exclusion = `!${root}/docs/private.md`
37+
38+
expect(matches(candidate, ["docs/**", exclusion])).toBe(false)
39+
expect(matches(candidate, [exclusion, "docs/**"])).toBe(true)
40+
expect(matches(candidate, ["docs/**", exclusion, `${CWD}/docs/private.md`])).toBe(true)
41+
})
42+
43+
it("resolves escaping patterns against the lexical parent, not the canonical parent", () => {
44+
expect(matches("/aliases/shared/file.md", ["../shared/file.md"])).toBe(true)
45+
expect(matches("/storage/shared/file.md", ["../shared/file.md"])).toBe(false)
46+
expect(matches("/storage/shared/file.md", ["/storage/shared/file.md"])).toBe(true)
47+
expect(matches("/aliases/shared/file.md", ["/storage/shared/file.md"])).toBe(false)
48+
expect(matches(`${CANONICAL_CWD}/docs/file.md`, ["../project/docs/file.md"])).toBe(true)
49+
})
50+
51+
it("does not translate similarly prefixed siblings or permit relative rules to cover other roots", () => {
52+
expect(matches(`${CANONICAL_CWD}-other/docs/file.md`, ["docs/**"])).toBe(false)
53+
expect(matches(`${CWD}-other/docs/file.md`, ["docs/**"])).toBe(false)
54+
expect(matches(`${CANONICAL_CWD}-other/docs/file.md`, [`${CWD}-other/docs/**`])).toBe(false)
55+
expect(matches(`${CANONICAL_CWD}/docs/file.md`, [`${CWD}-other/docs/**`])).toBe(false)
56+
expect(matches(`${CWD}-other/docs/file.md`, [`${CWD}-other/docs/**`])).toBe(true)
57+
})
58+
})
59+
60+
describe("read-only root equivalence approval", () => {
61+
it("keeps read and write allowlist exclusions independent while writes still imply reads", async () => {
62+
const tool: ClineSayTool = { tool: "readFile", path: `${CANONICAL_CWD}/docs/private.md` }
63+
const excludedRead = ["docs/**", `!${CWD}/docs/private.md`]
64+
const excludedWrite = ["docs/**", `!${CANONICAL_CWD}/docs/private.md`]
65+
66+
expect(await approval(tool, { allowedReadFiles: excludedRead, allowedWriteFiles: excludedWrite })).toEqual({
67+
decision: "ask",
68+
})
69+
expect(await approval(tool, { allowedReadFiles: excludedRead, allowedWriteFiles: ["docs/**"] })).toEqual({
70+
decision: "approve",
71+
})
72+
expect(await approval(tool, { allowedReadFiles: ["docs/**"], allowedWriteFiles: excludedWrite })).toEqual({
73+
decision: "approve",
74+
})
75+
})
76+
77+
it("does not extend write matching or protected-file permission when roots are equivalent", async () => {
78+
const canonicalTool: ClineSayTool = { tool: "newFileCreated", path: `${CANONICAL_CWD}/AGENTS.md` }
79+
expect(await approval(canonicalTool, { allowedReadFiles: ["*.md"], allowedWriteFiles: ["*.md"] })).toEqual({
80+
decision: "ask",
81+
})
82+
expect(await approval(canonicalTool, { allowedWriteFiles: [`${CANONICAL_CWD}/AGENTS.md`] }, CWD, true)).toEqual(
83+
{ decision: "ask" },
84+
)
85+
expect(
86+
await approval({ tool: "newFileCreated", path: "AGENTS.md" }, { allowedWriteFiles: ["*.md"] }, CWD, true),
87+
).toEqual({ decision: "ask" })
88+
expect(
89+
await approval(
90+
{ tool: "listFilesRecursive", path: `${CANONICAL_CWD}/docs` },
91+
{ allowedReadFiles: ["docs/**"] },
92+
),
93+
).toEqual({ decision: "ask" })
94+
expect(fs.realpath).not.toHaveBeenCalled()
95+
})
96+
97+
it("requires every named batch target to match and refuses unnamed targets", async () => {
98+
const state = { allowedReadFiles: ["docs/**", `!${CWD}/docs/private.md`] }
99+
const listed = { path: `${CANONICAL_CWD}/docs/notes.md`, lineSnippet: "", key: "notes" }
100+
const excluded = { path: `${CANONICAL_CWD}/docs/private.md`, lineSnippet: "", key: "private" }
101+
const outside = { path: "/elsewhere/docs/notes.md", lineSnippet: "", key: "outside" }
102+
103+
expect(
104+
await approval(
105+
{
106+
tool: "readFile",
107+
batchFiles: [listed, { path: `${CWD}/docs/other.md`, lineSnippet: "", key: "other" }],
108+
},
109+
state,
110+
),
111+
).toEqual({ decision: "approve" })
112+
expect(await approval({ tool: "readFile", batchFiles: [listed, excluded] }, state)).toEqual({ decision: "ask" })
113+
expect(await approval({ tool: "readFile", batchFiles: [listed, outside] }, state)).toEqual({ decision: "ask" })
114+
expect(await approval({ tool: "readFile", batchFiles: [listed], additionalFileCount: 1 }, state)).toEqual({
115+
decision: "ask",
116+
})
117+
})
118+
119+
it.each(["ENOENT", "EACCES"])(
120+
"does not infer root equivalence when task root resolution fails with %s",
121+
async (code) => {
122+
vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("root unavailable"), { code }))
123+
const tool: ClineSayTool = {
124+
tool: "readFile",
125+
path: `${CANONICAL_CWD}/docs/notes.md`,
126+
isOutsideWorkspace: true,
127+
}
128+
129+
expect(await approval(tool, { allowedReadFiles: ["docs/**"] })).toEqual({ decision: "ask" })
130+
expect(await approval(tool, { allowedReadFiles: [`${CWD}/docs/**`] })).toEqual({ decision: "ask" })
131+
expect(await approval(tool, { allowedReadFiles: [`${CANONICAL_CWD}/docs/**`] })).toEqual({
132+
decision: "approve",
133+
})
134+
},
135+
)
136+
137+
it("does not resolve workspace-relative grants without a task root", async () => {
138+
const text = JSON.stringify({
139+
tool: "readFile",
140+
path: `${CANONICAL_CWD}/docs/notes.md`,
141+
isOutsideWorkspace: true,
142+
})
143+
expect(
144+
await checkAutoApproval({ state: { ...baseState, allowedReadFiles: ["docs/**"] }, ask: "tool", text }),
145+
).toEqual({ decision: "ask" })
146+
expect(
147+
await checkAutoApproval({
148+
state: { ...baseState, allowedReadFiles: [`${CANONICAL_CWD}/docs/**`] },
149+
ask: "tool",
150+
text,
151+
}),
152+
).toEqual({ decision: "approve" })
153+
expect(fs.realpath).not.toHaveBeenCalled()
154+
})
155+
})

‎src/core/auto-approval/filePatterns.ts‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,37 @@ function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boo
392392
return toRootRelativePath(path.posix.resolve(workspaceRoot, normalized))
393393
}
394394

395+
function normalizeAbsoluteWorkspaceRoot(root: string | undefined, isWindows: boolean): string | undefined {
396+
const normalized = pathsepsToPosix(root ?? "", isWindows)
397+
if (!isAbsolutePath(normalized, isWindows)) return undefined
398+
399+
return path.posix
400+
.resolve(toPosixAbsolutePath(normalized, driveSegmentOf(normalized, isWindows), isWindows))
401+
.replace(/\/$/, "")
402+
}
403+
404+
function replaceWorkspaceRootPrefix(value: string, root: string, canonicalRoot: string, isWindows: boolean): string {
405+
const comparableValue = isWindows ? value.toLowerCase() : value
406+
const comparableRoot = isWindows ? root.toLowerCase() : root
407+
return comparableValue === comparableRoot || comparableValue.startsWith(`${comparableRoot}/`)
408+
? `${canonicalRoot}${value.slice(root.length)}`
409+
: value
410+
}
411+
412+
/** Translate only a verified workspace-root spelling, never a file symlink. */
413+
function canonicalizeWorkspaceRoot(
414+
value: string,
415+
cwd: string | undefined,
416+
canonicalCwd: string | undefined,
417+
isWindows: boolean,
418+
): string {
419+
const root = normalizeAbsoluteWorkspaceRoot(cwd, isWindows)
420+
const canonicalRoot = normalizeAbsoluteWorkspaceRoot(canonicalCwd, isWindows)
421+
if (root === undefined || canonicalRoot === undefined) return value
422+
423+
return replaceWorkspaceRootPrefix(value, root, canonicalRoot, isWindows)
424+
}
425+
395426
/**
396427
* Check whether a file path is covered by any of the configured patterns.
397428
*
@@ -411,12 +442,15 @@ function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boo
411442
export function isFileMatchedByPatterns({
412443
filePath,
413444
cwd,
445+
canonicalCwd,
414446
patterns,
415447
isWindows = runningOnWindows(),
416448
homeDir = os.homedir(),
417449
}: {
418450
filePath?: string
419451
cwd?: string
452+
/** Verified realpath of cwd, used only by read approval. */
453+
canonicalCwd?: string
420454
patterns?: string[]
421455
isWindows?: boolean
422456
homeDir?: string
@@ -434,6 +468,12 @@ export function isFileMatchedByPatterns({
434468
const matcherPatterns = patterns
435469
.map((pattern) => toMatcherPattern(pattern, cwd, isWindows, homeDir))
436470
.filter((pattern): pattern is string => !!pattern)
471+
.map((pattern) => {
472+
const negation = pattern.startsWith("!") ? "!" : ""
473+
// Resolve against the original cwd BEFORE translation: ../shared names
474+
// its lexical parent, not the canonical workspace's parent.
475+
return `${negation}${canonicalizeWorkspaceRoot(pattern.slice(negation.length), cwd, canonicalCwd, isWindows)}`
476+
})
437477

438478
if (!matcherPatterns.length) {
439479
return false
@@ -442,7 +482,7 @@ export function isFileMatchedByPatterns({
442482
try {
443483
return ignore({ ignoreCase: isWindows })
444484
.add([...matcherPatterns, MATCH_DIRECTORIES_ONLY_PATTERN])
445-
.ignores(candidate)
485+
.ignores(toRootRelativePath(canonicalizeWorkspaceRoot(`/${candidate}`, cwd, canonicalCwd, isWindows)))
446486
} catch (error) {
447487
// A path the matcher rejects cannot be confirmed as matching, so treat it
448488
// as unmatched.

‎src/core/auto-approval/index.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from "@roo-code/types"
1010

1111
import type { DcgDecision } from "../../services/destructive-command-guard/runner"
12+
import { realpath } from "fs/promises"
1213

1314
import { ClineAskResponse } from "../../shared/WebviewMessage"
1415

@@ -105,20 +106,23 @@ function areAllNamedFilesMatched(tool: ClineSayTool, matchFun: (filePath: string
105106
* narrows the list it appears in. This also means that negating a read is
106107
* ineffective while a non-negated write pattern still matches the file.
107108
*/
108-
function isReadAllowedByPatterns(
109+
async function isReadAllowedByPatterns(
109110
tool: ClineSayTool,
110111
cwd: string | undefined,
111112
state: Pick<ExtensionState, "allowedReadFiles" | "allowedWriteFiles">,
112-
): boolean {
113-
if (tool.tool !== "readFile") {
113+
): Promise<boolean> {
114+
if (tool.tool !== "readFile" || (!state.allowedReadFiles?.length && !state.allowedWriteFiles?.length)) {
114115
return false
115116
}
116117

118+
// Task-local root only. Unknown roots keep lexical matching; never resolve
119+
// individual file aliases here, since those can point outside the workspace.
120+
const canonicalCwd = cwd ? await realpath(cwd).catch(() => undefined) : undefined
117121
return areAllNamedFilesMatched(
118122
tool,
119123
(filePath) =>
120-
isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedReadFiles }) ||
121-
isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedWriteFiles }),
124+
isFileMatchedByPatterns({ filePath, cwd, canonicalCwd, patterns: state.allowedReadFiles }) ||
125+
isFileMatchedByPatterns({ filePath, cwd, canonicalCwd, patterns: state.allowedWriteFiles }),
122126
)
123127
}
124128

@@ -392,7 +396,7 @@ export async function checkAutoApproval({
392396
// `alwaysAllowReadOnly` permission. Such a pattern names its
393397
// location, including outside the workspace, so it also stands in for
394398
// `alwaysAllowReadOnlyOutsideWorkspace`.
395-
const isAllowedReadFile = isReadAllowedByPatterns(tool, cwd, state)
399+
const isAllowedReadFile = await isReadAllowedByPatterns(tool, cwd, state)
396400

397401
const isReadAllowed =
398402
isAllowedReadFile ||

‎src/core/tools/ReadFileTool.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -752,8 +752,11 @@ export class ReadFileTool extends BaseTool<"read_file"> {
752752
const accessAllowed = task.rooIgnoreController?.validateAccess(relPath)
753753
let target: ReadFileTarget | undefined
754754
try {
755-
if (accessAllowed) target = await ReadFileTarget.resolve(fullPath)
755+
if (accessAllowed) {
756+
target = await ReadFileTarget.resolve(fullPath, () => throwIfReadFileCancelled(task))
757+
}
756758
} catch (error) {
759+
if (isReadFileCancelled(task)) return
757760
const errorMsg = error instanceof Error ? error.message : String(error)
758761
results.push(`File: ${relPath}\nError: ${errorMsg}`)
759762
await task.say("error", `Error reading file ${relPath}: ${errorMsg}`)

‎src/core/tools/file-reading/ReadFileTarget.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ export class ReadFileTarget {
2424
return new ReadFileTarget(fullPath, identity)
2525
} catch (error) {
2626
checkCancelled?.()
27-
if (error instanceof Error && "code" in error && error.code === "ENOENT") {
27+
// Filesystem diagnostics, including inaccessible parents, require approval.
28+
// Code-less identity races and programmer errors must still fail closed.
29+
if (error instanceof Error && "code" in error && typeof error.code === "string") {
2830
return new ReadFileTarget(lexicalPath, undefined, error)
2931
}
3032
throw error

0 commit comments

Comments
 (0)