Skip to content

Commit f156d02

Browse files
committed
fix(read-file): bind approved reads to verified targets
Extract file-read safety from PR #1950 onto main without the reader architecture refactor. Approve canonical targets, verify descriptor identity, consume pinned bytes for text/images/documents, and discard cancelled ordinary results while retaining received feedback. Preserve the public singleton, string responses, legacy semantics, and formatted extraction API; add 60 regression and compatibility cases.
1 parent ecab519 commit f156d02

8 files changed

Lines changed: 1445 additions & 141 deletions

File tree

‎src/core/tools/ReadFileTool.ts‎

Lines changed: 204 additions & 120 deletions
Large diffs are not rendered by default.

‎src/core/tools/__tests__/readFileTool.spec.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,17 @@ vi.mock("path", async () => {
4646
vi.mock("fs/promises", () => ({
4747
readFile: vi.fn(),
4848
stat: vi.fn(),
49+
realpath: vi.fn(async (file: string) => file),
50+
lstat: vi.fn(async () => ({ dev: 1n, ino: 1n, isSymbolicLink: () => false })),
51+
open: vi.fn(async (file: string) => {
52+
const mocked = await import("fs/promises")
53+
return {
54+
stat: async () => ({ ...(await mocked.stat(file)), dev: 1n, ino: 1n }),
55+
read: async () => ({ bytesRead: 0 }),
56+
readFile: async (encoding?: "utf8") => (encoding ? mocked.readFile(file, encoding) : mocked.readFile(file)),
57+
close: vi.fn().mockResolvedValue(undefined),
58+
}
59+
}),
4960
}))
5061

5162
vi.mock("isbinaryfile")
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import * as fs from "fs/promises"
2+
import { constants, type BigIntStats } from "fs"
3+
import path from "path"
4+
import * as vscode from "vscode"
5+
6+
/** Metadata only until approval; content consumers must never reopen fullPath. */
7+
export class ReadFileTarget {
8+
private constructor(
9+
readonly fullPath: string,
10+
private readonly identity?: BigIntStats,
11+
private readonly missingError?: unknown,
12+
) {}
13+
14+
static async resolve(lexicalPath: string, checkCancelled?: () => void): Promise<ReadFileTarget> {
15+
try {
16+
const fullPath = await fs.realpath(lexicalPath)
17+
checkCancelled?.()
18+
const identity = await fs.lstat(fullPath, { bigint: true })
19+
checkCancelled?.()
20+
if (identity.isSymbolicLink() || (await fs.realpath(fullPath)) !== fullPath) {
21+
throw new Error(`File target changed before approval: ${lexicalPath}`)
22+
}
23+
checkCancelled?.()
24+
return new ReadFileTarget(fullPath, identity)
25+
} catch (error) {
26+
checkCancelled?.()
27+
if (error instanceof Error && "code" in error && error.code === "ENOENT") {
28+
return new ReadFileTarget(lexicalPath, undefined, error)
29+
}
30+
throw error
31+
}
32+
}
33+
34+
static async isOutsideWorkspace(fullPath: string): Promise<boolean> {
35+
const roots = await Promise.all(
36+
(vscode.workspace.workspaceFolders ?? []).map((folder) =>
37+
fs.realpath(folder.uri.fsPath).catch(() => undefined),
38+
),
39+
)
40+
return !roots.some((root) => {
41+
if (!root) return false
42+
const relative = path.relative(root, fullPath)
43+
return relative !== ".." && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)
44+
})
45+
}
46+
47+
async withHandle<T>(
48+
read: (file: fs.FileHandle, stats: BigIntStats) => Promise<T>,
49+
checkCancelled?: () => void,
50+
): Promise<T> {
51+
checkCancelled?.()
52+
if (!this.identity) throw this.missingError
53+
const file = await fs.open(this.fullPath, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0))
54+
try {
55+
checkCancelled?.()
56+
const stats = await file.stat({ bigint: true })
57+
checkCancelled?.()
58+
if (stats.dev !== this.identity.dev || stats.ino !== this.identity.ino) {
59+
throw new Error(`File target changed after approval: ${this.fullPath}`)
60+
}
61+
return await read(file, stats)
62+
} finally {
63+
await file.close()
64+
}
65+
}
66+
}

0 commit comments

Comments
 (0)