Skip to content

Commit b1015ca

Browse files
WebMadWebMad
committed
fix(auto-approval): reject ambiguous Windows drive-relative paths
1 parent ca18838 commit b1015ca

6 files changed

Lines changed: 146 additions & 23 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ describe.each([
6565
}
6666
})
6767

68-
it.each([undefined, "", "relative/project", "C:relative"])(
68+
it.each([undefined, "", "relative/project", "C:", "C:relative"])(
6969
"does not infer equivalence from an unknown or relative root: %s",
7070
(invalidRoot) => {
7171
const value = `${root}/docs/file.md`
@@ -134,7 +134,7 @@ describe("Windows workspace-root strategy", () => {
134134

135135
it("supports drive roots and drive-less absolute roots in matcher notation", () => {
136136
const strategy = new WindowsWorkspaceRootStrategy()
137-
expect(WorkspaceRootCanonicalizer.forStrategy("C:", "D:/", strategy).canonicalize("/C/docs/file.md")).toBe(
137+
expect(WorkspaceRootCanonicalizer.forStrategy("C:/", "D:/", strategy).canonicalize("/C/docs/file.md")).toBe(
138138
"/D/docs/file.md",
139139
)
140140
expect(

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,18 @@ describe("matching off Windows", () => {
5858
expect(matches("/tmp/notes.md", ["C:/tmp/notes.md"])).toBe(false)
5959
})
6060

61+
it("keeps drive-relative-looking names usable as literal POSIX paths", () => {
62+
expect(matches("C:notes.md", ["C:notes.md"])).toBe(true)
63+
expect(matches("D:docs/notes.md", ["D:docs/**"])).toBe(true)
64+
expect(
65+
matcher().isFileMatchedByPatterns({
66+
filePath: "docs/notes.md",
67+
cwd: "/workspace/C:",
68+
patterns: ["docs/**"],
69+
}),
70+
).toBe(true)
71+
})
72+
6173
it("matches a bare pattern anywhere in the workspace, and nowhere else", () => {
6274
expect(matches("notes.md", ["notes.md"])).toBe(true)
6375
expect(matches("deeply/nested/notes.md", ["notes.md"])).toBe(true)

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import * as fs from "fs/promises"
2+
import { baseState } from "./fixtures"
3+
import { matcherForPlatform } from "./matcherForPlatform"
4+
5+
vi.mock("fs/promises", async (importOriginal) => {
6+
const actual = await importOriginal<typeof import("fs/promises")>()
7+
return { ...actual, realpath: vi.fn() }
8+
})
9+
10+
describe("approval with a drive-relative Windows workspace", () => {
11+
matcherForPlatform("win32")
12+
13+
beforeEach(() => {
14+
vi.mocked(fs.realpath).mockReset().mockResolvedValue("C:/repo")
15+
})
16+
17+
it.each(["readFile", "newFileCreated"] as const)(
18+
"does not approve %s outside the actual workspace through relative grants",
19+
async (tool) => {
20+
const { checkAutoApproval } = await import("../index")
21+
const state = { ...baseState, allowedReadFiles: ["docs/**"], allowedWriteFiles: ["docs/**"] }
22+
for (const path of ["C:/docs/private.md", "../docs/private.md"]) {
23+
expect(
24+
await checkAutoApproval({
25+
state,
26+
cwd: "C:",
27+
ask: "tool",
28+
text: JSON.stringify({ tool, path, isOutsideWorkspace: true }),
29+
}),
30+
).toEqual({ decision: "ask" })
31+
}
32+
33+
const text = JSON.stringify({ tool, path: "C:/docs/private.md", isOutsideWorkspace: true })
34+
expect(
35+
await checkAutoApproval({
36+
state: { ...state, allowedReadFiles: ["C:/docs/**"], allowedWriteFiles: ["C:/docs/**"] },
37+
cwd: "C:",
38+
ask: "tool",
39+
text,
40+
}),
41+
).toEqual({ decision: "approve" })
42+
expect(
43+
await checkAutoApproval({
44+
state: {
45+
...state,
46+
allowedReadFiles: ["C:/docs/**", "!C:/docs/private.md"],
47+
allowedWriteFiles: ["C:/docs/**", "!C:/docs/private.md"],
48+
},
49+
cwd: "C:",
50+
ask: "tool",
51+
text,
52+
}),
53+
).toEqual({ decision: "ask" })
54+
},
55+
)
56+
})

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

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,59 @@ describe("matching on Windows", () => {
4747
expect(matches("//other/share/docs/private.md", ["/docs/**"], cwd)).toBe(false)
4848
})
4949

50-
it.each(["C:", "C:/", "//server/share", "//server/share/"])(
51-
"matches relative grants at a volume root: %s",
52-
(cwd) => {
53-
const absolutePath = `${cwd.replace(/\/$/, "")}/docs/private.md`
54-
expect(matches("docs/private.md", ["docs/**"], cwd)).toBe(true)
55-
expect(matches(absolutePath, ["docs/**"], cwd)).toBe(true)
56-
expect(matches("docs/private.md", ["docs/**", "!docs/private.md"], cwd)).toBe(false)
57-
expect(matches(absolutePath, ["docs/**", "!docs/private.md"], cwd)).toBe(false)
50+
it.each(["C:/", "//server/share", "//server/share/"])("matches relative grants at a volume root: %s", (cwd) => {
51+
const absolutePath = `${cwd.replace(/\/$/, "")}/docs/private.md`
52+
expect(matches("docs/private.md", ["docs/**"], cwd)).toBe(true)
53+
expect(matches(absolutePath, ["docs/**"], cwd)).toBe(true)
54+
expect(matches("docs/private.md", ["docs/**", "!docs/private.md"], cwd)).toBe(false)
55+
expect(matches(absolutePath, ["docs/**", "!docs/private.md"], cwd)).toBe(false)
56+
})
57+
58+
it.each(["C:", "c:", "C:repo"])("does not anchor relative grants to a drive-relative workspace: %s", (cwd) => {
59+
expect(matcher().toMatcherPattern("docs/**", cwd)).toBeUndefined()
60+
for (const canonicalCwd of [undefined, "C:/repo"]) {
61+
for (const filePath of [
62+
"docs/private.md",
63+
"../docs/private.md",
64+
"C:/docs/private.md",
65+
"C:/repo/docs/private.md",
66+
]) {
67+
expect(matcher().isFileMatchedByPatterns({ filePath, cwd, canonicalCwd, patterns: ["docs/**"] })).toBe(
68+
false,
69+
)
70+
}
71+
expect(
72+
matcher().isFileMatchedByPatterns({
73+
filePath: "C:/docs/notes.md",
74+
cwd,
75+
canonicalCwd,
76+
patterns: ["C:/docs/**"],
77+
}),
78+
).toBe(true)
79+
expect(
80+
matcher().isFileMatchedByPatterns({
81+
filePath: "C:/docs/private.md",
82+
cwd,
83+
canonicalCwd,
84+
patterns: ["C:/docs/**", "!C:/docs/private.md"],
85+
}),
86+
).toBe(false)
87+
expect(
88+
matcher().isFileMatchedByPatterns({
89+
filePath: "../docs/notes.md",
90+
cwd,
91+
canonicalCwd,
92+
patterns: ["C:/docs/**"],
93+
}),
94+
).toBe(false)
95+
}
96+
})
97+
98+
it.each(["C:", "C:docs/private.md", "D:docs/private.md"])(
99+
"does not guess the location of a drive-relative candidate or pattern: %s",
100+
(filePath) => {
101+
expect(matches(filePath, ["**"])).toBe(false)
102+
expect(matcher().toMatcherPattern(filePath, WINDOWS_CWD)).toBeUndefined()
58103
},
59104
)
60105

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

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -159,13 +159,12 @@ function pathsepsToPosix(value: string, isWindows: boolean): string {
159159
}
160160

161161
/**
162-
* A Windows drive at the root of a path: the `C:` in `C:/tmp/x`, or `C:` alone.
162+
* A Windows drive at the root of an absolute path: the `C:` in `C:/tmp/x`.
163163
*
164-
* The lookahead is what limits this to a drive *root*. Windows also reads `C:tmp`
165-
* as naming a drive, but relative to the current directory on it, which is not a
166-
* place this module can resolve.
164+
* Require a separator after the colon. Both `C:` and `C:tmp` are relative to
165+
* the current directory on that drive, which this module cannot safely infer.
167166
*/
168-
const DRIVE_PREFIX = /^([a-zA-Z]):(?=\/|$)/
167+
const DRIVE_PREFIX = /^([a-zA-Z]):(?=\/)/
169168

170169
/**
171170
* The Windows volume in matcher notation: /C for a drive, /UNC/server/share
@@ -201,6 +200,11 @@ function isAbsolutePath(value: string, isWindows: boolean): boolean {
201200
return value.startsWith("/") || (isWindows && DRIVE_PREFIX.test(value))
202201
}
203202

203+
/** Windows drive-relative paths depend on per-drive process state, not the task workspace. */
204+
function isDriveRelativePath(value: string, isWindows: boolean): boolean {
205+
return isWindows && /^[a-zA-Z]:(?!\/)/.test(value)
206+
}
207+
204208
function escapesWorkspace(posixPath: string): boolean {
205209
return posixPath.split("/").includes("..")
206210
}
@@ -309,7 +313,13 @@ function toMatcherPatternWithStrategy(
309313
// as gitignore's escape character and a legal filename character.
310314
let normalized = pathsepsToPosix(pattern.slice(negation.length), isWindows)
311315

312-
if (!normalized.trim() || normalized === "." || normalized === "~" || normalized.endsWith("/")) {
316+
if (
317+
!normalized.trim() ||
318+
isDriveRelativePath(normalized, isWindows) ||
319+
normalized === "." ||
320+
normalized === "~" ||
321+
normalized.endsWith("/")
322+
) {
313323
return undefined
314324
}
315325

@@ -328,8 +338,8 @@ function toMatcherPatternWithStrategy(
328338
return `${negation}${strategy.toMatcherAbsolutePath(normalized, workspaceVolume)}`
329339
}
330340

331-
if (!cwd) {
332-
// No workspace for a relative pattern to be relative to; see the note above
341+
if (!cwd || isDriveRelativePath(workspacePosix, isWindows)) {
342+
// An unknown or drive-relative workspace cannot anchor a relative pattern; see the note above
333343
// on why this is not matched loosely instead.
334344
return undefined
335345
}
@@ -378,7 +388,7 @@ function toMatcherPath(
378388
// Not trimmed: whitespace can be part of a filename.
379389
const normalized = pathsepsToPosix(filePath, isWindows)
380390

381-
if (!normalized.trim() || normalized === ".") {
391+
if (!normalized.trim() || isDriveRelativePath(normalized, isWindows) || normalized === ".") {
382392
return undefined
383393
}
384394

@@ -390,8 +400,8 @@ function toMatcherPath(
390400
return toRootRelativePath(strategy.toMatcherAbsolutePath(normalized, workspaceVolume))
391401
}
392402

393-
if (!cwd) {
394-
// A relative path cannot be placed on the filesystem without a root, and
403+
if (!cwd || isDriveRelativePath(workspacePosix, isWindows)) {
404+
// A relative path cannot be placed without a known, non-drive-relative root, and
395405
// the only patterns that survive without one are absolute, which such a
396406
// path could never match anyway.
397407
return undefined

‎src/core/auto-approval/strategies/WindowsWorkspaceRootStrategy.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export class WindowsWorkspaceRootStrategy implements WorkspaceRootStrategy {
88
// Keep UNC paths independent of the current drive and distinct from drive letters.
99
if (absolutePath.startsWith("//")) return `/UNC/${absolutePath.replace(/^\/+/, "")}`
1010

11-
const drive = /^([a-zA-Z]):(?=\/|$)/.exec(absolutePath)
11+
const drive = /^([a-zA-Z]):(?=\/)/.exec(absolutePath)
1212
// Rooted paths belong to the workspace drive/share. Without a known volume,
1313
// keep them in a separate namespace so /UNC/... cannot impersonate a share.
1414
return drive
@@ -18,7 +18,7 @@ export class WindowsWorkspaceRootStrategy implements WorkspaceRootStrategy {
1818

1919
normalizeAbsoluteRoot(root: string | undefined): string | undefined {
2020
const normalized = (root ?? "").replace(/\\/g, "/")
21-
const drive = /^([a-zA-Z]):(?=\/|$)/.exec(normalized)
21+
const drive = /^([a-zA-Z]):(?=\/)/.exec(normalized)
2222
if (!normalized.startsWith("/") && !drive) return undefined
2323

2424
// Match the allowlist representation: a Windows drive is a POSIX path segment.

0 commit comments

Comments
 (0)