From a4db192f7301074b4f51040b407fdc3e14ed6399 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sat, 15 Aug 2026 12:14:15 -0400 Subject: [PATCH 1/8] fix: close confined-file TOCTOU with open-then-verify Open the confined path, verify the descriptor (POSIX O_NOFOLLOW; win32 ino identity), and read only through that handle so a symlink swap after realpath cannot exfiltrate outside-root content. --- src/agent/fileView.ts | 231 +++++++++++++++++++------------------ src/agent/renamePreview.ts | 115 +++++++++--------- src/util/confinedFile.ts | 104 ++++++++++++++++- tests/file-view.test.ts | 83 +++++++++++-- 4 files changed, 354 insertions(+), 179 deletions(-) diff --git a/src/agent/fileView.ts b/src/agent/fileView.ts index 268b0614..4f492487 100644 --- a/src/agent/fileView.ts +++ b/src/agent/fileView.ts @@ -2,7 +2,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { BuildOptions } from "../indexer/types.js"; -import { resolveProjectFile, resolveReadableFile } from "../util/confinedFile.js"; +import { openConfinedReadableFile, resolveProjectFile, type ConfinedReadableFile } from "../util/confinedFile.js"; import { fileIdentityKey, normalizePath, toProjectDisplayPath } from "../util/paths.js"; import { createAgentSession, @@ -135,59 +135,64 @@ export async function getCodegraphFileViewWithSession( ): Promise { const root = path.resolve(request.root); const realRoot = await fs.realpath(root); - const resolvedFile = await resolveReadableFile(realRoot, root, request.file); - const offset = boundedPositiveInteger(request.offset, 1, Number.MAX_SAFE_INTEGER); - const limit = boundedPositiveInteger(request.limit, DEFAULT_FILE_VIEW_LINES, MAX_FILE_VIEW_LINES); - const maxBytes = boundedPositiveInteger(request.maxBytes, DEFAULT_FILE_VIEW_BYTES, MAX_FILE_VIEW_BYTES); - const displaySensitiveKind = classifySensitiveFile(resolvedFile.displayPath); - let sensitiveKind = classifySensitiveFile(resolvedFile.realPath); - const keyMaterialAlias = displaySensitiveKind === "key-material" || sensitiveKind === "key-material"; - if (keyMaterialAlias) sensitiveKind = "key-material"; - else if (!sensitiveKind) sensitiveKind = displaySensitiveKind; - - let page: FilePage; - let truncated: boolean; - let sensitive: AgentFileViewSensitiveInfo | undefined; - if (sensitiveKind && !request.allowSensitive) { - const summary = await buildSensitiveSummary(resolvedFile.realPath, sensitiveKind); - page = paginateText(summary.text, offset, limit); - truncated = summary.scanTruncated; - sensitive = { kind: sensitiveKind, redacted: true, allowSensitiveRequired: true }; - } else { - page = await readTextFilePage(resolvedFile.realPath, offset, limit, maxBytes); - truncated = page.truncated; - if (sensitiveKind) { - sensitive = { kind: sensitiveKind, redacted: false, allowSensitiveRequired: true }; + const opened = await openConfinedReadableFile(realRoot, root, request.file); + try { + const offset = boundedPositiveInteger(request.offset, 1, Number.MAX_SAFE_INTEGER); + const limit = boundedPositiveInteger(request.limit, DEFAULT_FILE_VIEW_LINES, MAX_FILE_VIEW_LINES); + const maxBytes = boundedPositiveInteger(request.maxBytes, DEFAULT_FILE_VIEW_BYTES, MAX_FILE_VIEW_BYTES); + const displaySensitiveKind = classifySensitiveFile(opened.displayPath); + let sensitiveKind = classifySensitiveFile(opened.realPath); + const keyMaterialAlias = displaySensitiveKind === "key-material" || sensitiveKind === "key-material"; + if (keyMaterialAlias) sensitiveKind = "key-material"; + else if (!sensitiveKind) sensitiveKind = displaySensitiveKind; + + let page: FilePage; + let truncated: boolean; + let sensitive: AgentFileViewSensitiveInfo | undefined; + if (sensitiveKind && !request.allowSensitive) { + const summary = await buildSensitiveSummary(opened, sensitiveKind); + page = paginateText(summary.text, offset, limit); + truncated = summary.scanTruncated; + sensitive = { kind: sensitiveKind, redacted: true, allowSensitiveRequired: true }; + } else { + page = await readTextFilePage(opened, offset, limit, maxBytes); + truncated = page.truncated; + if (sensitiveKind) { + sensitive = { kind: sensitiveKind, redacted: false, allowSensitiveRequired: true }; + } } - } - let freshness: AgentFreshnessResult = { state: "fresh" }; - let graphContext: AgentFileGraphContext | undefined; - if (request.includeGraphContext) { - const activeSession = - session ?? - createAgentSession({ - root, - ...(request.buildOptions ? { buildOptions: request.buildOptions } : {}), - }); - if (activeSession.checkFreshness) { - freshness = await activeSession.checkFreshness(); + let freshness: AgentFreshnessResult = { state: "fresh" }; + let graphContext: AgentFileGraphContext | undefined; + if (request.includeGraphContext) { + const activeSession = + session ?? + createAgentSession({ + root, + ...(request.buildOptions ? { buildOptions: request.buildOptions } : {}), + }); + if (activeSession.checkFreshness) { + freshness = await activeSession.checkFreshness(); + } + // Path-only index lookup; byte reads already use the confined descriptor above. + const projectFile = await resolveProjectFile(realRoot, root, request.file); + const snapshot = await activeSession.loadProject({ symbolGraph: "skip" }); + graphContext = buildFileGraphContext(snapshot, projectFile); } - const projectFile = await resolveProjectFile(realRoot, root, request.file); - const snapshot = await activeSession.loadProject({ symbolGraph: "skip" }); - graphContext = buildFileGraphContext(snapshot, projectFile); - } - return buildResponse({ - file: resolvedFile.displayPath, - offset, - limit, - page, - truncated, - freshness, - ...(graphContext ? { graphContext } : {}), - ...(sensitive ? { sensitive } : {}), - }); + return buildResponse({ + file: opened.displayPath, + offset, + limit, + page, + truncated, + freshness, + ...(graphContext ? { graphContext } : {}), + ...(sensitive ? { sensitive } : {}), + }); + } finally { + await opened.handle.close(); + } } export function formatAgentFileViewResponse(response: AgentFileViewResponse): string { @@ -277,10 +282,15 @@ function buildResponse(args: { }; } -async function readTextFilePage(filePath: string, offset: number, limit: number, maxBytes: number): Promise { - await assertReadableTextFile(filePath); - - const handle = await fs.open(filePath, "r"); +async function readTextFilePage( + opened: ConfinedReadableFile, + offset: number, + limit: number, + maxBytes: number, +): Promise { + assertReadableTextFile(opened); + const filePath = opened.realPath; + const handle = opened.handle; const selectedLines: string[] = []; let lineNumber = 1; let remainingBytes = maxBytes; @@ -336,33 +346,29 @@ async function readTextFilePage(filePath: string, offset: number, limit: number, currentLineChunks = []; }; - try { - const buffer = Buffer.allocUnsafe(READ_BUFFER_BYTES); - while (true) { - const { bytesRead } = await handle.read(buffer, 0, buffer.length, null); - if (!bytesRead) break; - const chunk = buffer.subarray(0, bytesRead); - totalBytes += bytesRead; - assertFileViewSourceBytes(totalBytes, filePath); - if (chunk.includes(0)) { - throw new Error(`Binary files are not supported: ${filePath}`); - } - validateUtf8Chunk(chunk, utf8State, filePath); - let segmentStart = 0; - for (let index = 0; index < chunk.length; index += 1) { - if (chunk[index] !== 0x0a) continue; - consumeSegment(chunk.subarray(segmentStart, index)); - finishLine(); - lineNumber += 1; - segmentStart = index + 1; - } - consumeSegment(chunk.subarray(segmentStart)); + const buffer = Buffer.allocUnsafe(READ_BUFFER_BYTES); + while (true) { + const { bytesRead } = await handle.read(buffer, 0, buffer.length, null); + if (!bytesRead) break; + const chunk = buffer.subarray(0, bytesRead); + totalBytes += bytesRead; + assertFileViewSourceBytes(totalBytes, filePath); + if (chunk.includes(0)) { + throw new Error(`Binary files are not supported: ${filePath}`); } - assertUtf8Complete(utf8State, filePath); - finishLine(); - } finally { - await handle.close(); + validateUtf8Chunk(chunk, utf8State, filePath); + let segmentStart = 0; + for (let index = 0; index < chunk.length; index += 1) { + if (chunk[index] !== 0x0a) continue; + consumeSegment(chunk.subarray(segmentStart, index)); + finishLine(); + lineNumber += 1; + segmentStart = index + 1; + } + consumeSegment(chunk.subarray(segmentStart)); } + assertUtf8Complete(utf8State, filePath); + finishLine(); let nextOffset: number | undefined; if (lastReturnedLine !== undefined && lastReturnedLine < lineNumber) { @@ -497,19 +503,20 @@ export function classifySensitiveFile(filePath: string): AgentFileViewSensitiveK return undefined; } -async function buildSensitiveSummary(filePath: string, kind: AgentFileViewSensitiveKind): Promise { +async function buildSensitiveSummary( + opened: ConfinedReadableFile, + kind: AgentFileViewSensitiveKind, +): Promise { if (kind === "key-material") { - const stat = await fs.stat(filePath); - if (!stat.isFile()) throw new Error(`File view target is not a file: ${filePath}`); return { - text: `Sensitive key material omitted.\nSize: ${stat.size} bytes.`, + text: `Sensitive key material omitted.\nSize: ${opened.size} bytes.`, scanTruncated: false, }; } - const scan = await scanTextFilePrefix(filePath, SENSITIVE_SCAN_BYTES); + const scan = await scanTextFilePrefix(opened, SENSITIVE_SCAN_BYTES); - const text = UTF8_DECODER.decode(trimToUtf8Boundary(scan.prefix)); - const keys = extractSensitiveKeys(text).slice(0, SENSITIVE_KEY_LIMIT); + const decoded = UTF8_DECODER.decode(trimToUtf8Boundary(scan.prefix)); + const keys = extractSensitiveKeys(decoded).slice(0, SENSITIVE_KEY_LIMIT); const keySummary = keys.length ? keys.join(", ") : "No keys detected in bounded structural scan."; return { text: `Sensitive ${kind} values omitted.\nKeys: ${keySummary}`, @@ -518,48 +525,44 @@ async function buildSensitiveSummary(filePath: string, kind: AgentFileViewSensit } async function scanTextFilePrefix( - filePath: string, + opened: ConfinedReadableFile, prefixLimit: number, ): Promise<{ prefix: Buffer; totalBytes: number }> { - await assertReadableTextFile(filePath); - const handle = await fs.open(filePath, "r"); + assertReadableTextFile(opened); + const filePath = opened.realPath; + const handle = opened.handle; const prefixChunks: Buffer[] = []; let prefixBytes = 0; let totalBytes = 0; const utf8State = createUtf8ValidationState(); - try { - const buffer = Buffer.allocUnsafe(READ_BUFFER_BYTES); - while (true) { - const { bytesRead } = await handle.read(buffer, 0, buffer.length, null); - if (!bytesRead) break; - const chunk = buffer.subarray(0, bytesRead); - if (chunk.includes(0)) { - throw new Error(`Binary files are not supported: ${filePath}`); - } - validateUtf8Chunk(chunk, utf8State, filePath); - if (prefixBytes < prefixLimit) { - const bytesToKeep = Math.min(chunk.length, prefixLimit - prefixBytes); - prefixChunks.push(Buffer.from(chunk.subarray(0, bytesToKeep))); - prefixBytes += bytesToKeep; - } - totalBytes += chunk.length; - assertFileViewSourceBytes(totalBytes, filePath); + const buffer = Buffer.allocUnsafe(READ_BUFFER_BYTES); + while (true) { + const { bytesRead } = await handle.read(buffer, 0, buffer.length, null); + if (!bytesRead) break; + const chunk = buffer.subarray(0, bytesRead); + if (chunk.includes(0)) { + throw new Error(`Binary files are not supported: ${filePath}`); } - assertUtf8Complete(utf8State, filePath); - } finally { - await handle.close(); + validateUtf8Chunk(chunk, utf8State, filePath); + if (prefixBytes < prefixLimit) { + const bytesToKeep = Math.min(chunk.length, prefixLimit - prefixBytes); + prefixChunks.push(Buffer.from(chunk.subarray(0, bytesToKeep))); + prefixBytes += bytesToKeep; + } + totalBytes += chunk.length; + assertFileViewSourceBytes(totalBytes, filePath); } + assertUtf8Complete(utf8State, filePath); return { prefix: prefixChunks.length === 1 ? prefixChunks[0]! : Buffer.concat(prefixChunks), totalBytes, }; } -async function assertReadableTextFile(filePath: string): Promise { - assertTextFileExtension(filePath); - const stat = await fs.stat(filePath); - if (!stat.isFile()) throw new Error(`File view target is not a file: ${filePath}`); - assertFileViewSourceBytes(stat.size, filePath); +function assertReadableTextFile(opened: ConfinedReadableFile): void { + assertTextFileExtension(opened.displayPath); + assertTextFileExtension(opened.realPath); + assertFileViewSourceBytes(opened.size, opened.realPath); } function assertFileViewSourceBytes(totalBytes: number, filePath: string): void { diff --git a/src/agent/renamePreview.ts b/src/agent/renamePreview.ts index b6e2caee..3b291072 100644 --- a/src/agent/renamePreview.ts +++ b/src/agent/renamePreview.ts @@ -11,7 +11,7 @@ import { SymbolKind, type BuildOptions, type ProjectIndex, type Reference, type import { supportForFile } from "../languages.js"; import type { Range } from "../types.js"; import { ensureParsedContext } from "../indexer/parse-context.js"; -import { resolveReadableFile } from "../util/confinedFile.js"; +import { openConfinedReadableFile } from "../util/confinedFile.js"; import { errorMessage } from "../util/errors.js"; import { fileIdentityKey } from "../util/paths.js"; import { classifySensitiveFile } from "./fileView.js"; @@ -644,64 +644,71 @@ async function loadRenameFile( if (cached) return await cached; const load = (async (): Promise => { try { - const resolved = await resolveReadableFile(realRoot, snapshot.root, file); - const sensitiveKind = classifySensitiveFile(resolved.displayPath) ?? classifySensitiveFile(resolved.realPath); - if (sensitiveKind) { - unsafeSites.push({ - location: { file: resolved.displayPath, range: zeroRange() }, - text: "", - reason: "sensitive_file", - provenance: { - ...provenance, - confidence: "low", - reason: `Rename preview does not read ${sensitiveKind} files.`, - }, - }); - return null; - } - if (isGeneratedRenameFile(resolved.displayPath) || isGeneratedRenameFile(resolved.realPath)) { - unsafeSites.push({ - location: { file: resolved.displayPath, range: zeroRange() }, - text: "", - reason: "generated_file", - provenance: { - ...provenance, - confidence: "low", - reason: "Generated or vendored files are not edited by rename preview.", - }, - }); - return null; - } - const before = await fs.stat(resolved.realPath); - const indexedSignature = snapshot.fileSignatures?.get(file) ?? snapshot.fileSignatures?.get(resolved.realPath); - if (indexedSignature && (indexedSignature.size !== before.size || indexedSignature.mtimeMs !== before.mtimeMs)) { - unsafeSites.push({ - location: { file: resolved.displayPath, range: zeroRange() }, - text: "", - reason: "unresolved_reference", - provenance: { ...provenance, confidence: "low", reason: "File changed after indexing." }, - }); - return null; - } - const bytes = await fs.readFile(resolved.realPath); - if (bytes.includes(0)) throw new Error("Binary source contains NUL bytes."); - let source: string; + const opened = await openConfinedReadableFile(realRoot, snapshot.root, file); try { - source = strictUtf8Decoder.decode(bytes); - } catch { - throw new Error("Malformed UTF-8 source cannot be renamed safely."); + const sensitiveKind = classifySensitiveFile(opened.displayPath) ?? classifySensitiveFile(opened.realPath); + if (sensitiveKind) { + unsafeSites.push({ + location: { file: opened.displayPath, range: zeroRange() }, + text: "", + reason: "sensitive_file", + provenance: { + ...provenance, + confidence: "low", + reason: `Rename preview does not read ${sensitiveKind} files.`, + }, + }); + return null; + } + if (isGeneratedRenameFile(opened.displayPath) || isGeneratedRenameFile(opened.realPath)) { + unsafeSites.push({ + location: { file: opened.displayPath, range: zeroRange() }, + text: "", + reason: "generated_file", + provenance: { + ...provenance, + confidence: "low", + reason: "Generated or vendored files are not edited by rename preview.", + }, + }); + return null; + } + const before = await opened.handle.stat(); + const indexedSignature = snapshot.fileSignatures?.get(file) ?? snapshot.fileSignatures?.get(opened.realPath); + if ( + indexedSignature && + (indexedSignature.size !== before.size || indexedSignature.mtimeMs !== before.mtimeMs) + ) { + unsafeSites.push({ + location: { file: opened.displayPath, range: zeroRange() }, + text: "", + reason: "unresolved_reference", + provenance: { ...provenance, confidence: "low", reason: "File changed after indexing." }, + }); + return null; + } + const bytes = await opened.handle.readFile(); + if (bytes.includes(0)) throw new Error("Binary source contains NUL bytes."); + let source: string; + try { + source = strictUtf8Decoder.decode(bytes); + } catch { + throw new Error("Malformed UTF-8 source cannot be renamed safely."); + } + return { + displayPath: opened.displayPath, + realPath: opened.realPath, + source, + beforeSize: before.size, + beforeMtimeMs: before.mtimeMs, + }; + } finally { + await opened.handle.close(); } - return { - displayPath: resolved.displayPath, - realPath: resolved.realPath, - source, - beforeSize: before.size, - beforeMtimeMs: before.mtimeMs, - }; } catch (error: unknown) { const message = errorMessage(error); let reason: RenameUnsafeSite["reason"] = "unresolved_reference"; - if (/outside project root/i.test(message)) reason = "outside_root"; + if (/outside project root|changed between verification and open/i.test(message)) reason = "outside_root"; else if (/binary source|malformed UTF-8/i.test(message)) reason = "unsupported_syntax"; unsafeSites.push({ location: { file: normalizeAgentFilePath(snapshot.root, file), range: zeroRange() }, diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index 29925512..ed76c46b 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -1,8 +1,28 @@ -import fs from "node:fs/promises"; +import { constants as fsConstants, type Stats } from "node:fs"; +import fs, { type FileHandle } from "node:fs/promises"; import path from "node:path"; import { isFilePathWithinRoot, normalizePath, toProjectRelativePath } from "./paths.js"; +export type ConfinedReadableFile = { + handle: FileHandle; + realPath: string; + displayPath: string; + size: number; +}; + +type ConfinedFileTestHook = (realPath: string) => void | Promise; + +let afterConfinedPathVerifiedForTests: ConfinedFileTestHook | undefined; + +/** + * Test-only seam between path confinement and the open that binds a descriptor. + * Production code must leave this unset. + */ +export function setAfterConfinedPathVerifiedForTests(hook: ConfinedFileTestHook | undefined): void { + afterConfinedPathVerifiedForTests = hook; +} + export async function resolveReadableFile( realRoot: string, root: string, @@ -15,6 +35,35 @@ export async function resolveReadableFile( return { realPath, displayPath }; } +/** + * Resolve a project path, open it, and verify the opened descriptor before any read. + * + * Flow: realpath confinement → optional test hook → `lstat` (must be a regular file) → + * open the realpath'd target → `fstat` on that descriptor → identity check → callers read + * only through the returned handle (never re-resolve the path string). + * + * POSIX: open uses `O_RDONLY | O_NOFOLLOW` so a leaf symlink swap fails the open with ELOOP. + * win32: Node's `fs.open` has no portable `O_NOFOLLOW` (`fs.constants.O_NOFOLLOW` is absent). + * Guarantee there is post-open identity: `fstat.ino` must match `lstat.ino`; `dev` is compared + * only when both sides are non-zero because win32 `lstat` often reports `dev=0` while `fstat` + * has the volume serial. A symlink swap after `lstat` yields a different inode on follow. + * + * In-root symlinks still work: confinement realpaths them first, then the open targets the + * resolved regular file inside the root—not the symlink leaf. + */ +export async function openConfinedReadableFile( + realRoot: string, + root: string, + filePath: string, +): Promise { + const { realPath, displayPath } = await resolveReadableFile(realRoot, root, filePath); + if (afterConfinedPathVerifiedForTests) { + await afterConfinedPathVerifiedForTests(realPath); + } + const { handle, size } = await openVerifiedRegularFile(realPath); + return { handle, realPath, displayPath, size }; +} + export async function resolveProjectFile(realRoot: string, root: string, filePath: string): Promise { const candidatePath = path.isAbsolute(filePath) ? path.resolve(filePath) : path.resolve(root, filePath); const realPath = await assertRealPathCandidateWithinRoot(realRoot, candidatePath, "File"); @@ -62,6 +111,59 @@ export async function findNearestExistingPath(filePath: string): Promise return current; } +async function openVerifiedRegularFile(realPath: string): Promise<{ handle: FileHandle; size: number }> { + const preStat = await fs.lstat(realPath); + assertRegularFileStat(preStat, realPath); + + const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0); + let handle: FileHandle; + try { + handle = await fs.open(realPath, openFlags); + } catch (error) { + throw rewriteNoFollowOpenError(error, realPath); + } + + try { + const postStat = await handle.stat(); + assertRegularFileStat(postStat, realPath); + if (!sameFileIdentity(preStat, postStat)) { + throw new Error( + `File changed between verification and open: ${normalizePath(realPath)} (possible path confinement race)`, + ); + } + return { handle, size: postStat.size }; + } catch (error) { + await handle.close().catch(() => undefined); + throw error; + } +} + +function sameFileIdentity(preStat: Stats, postStat: Stats): boolean { + if (preStat.ino !== postStat.ino) return false; + // win32 lstat often reports dev=0 while fstat has the volume serial; only compare when both are set. + if (preStat.dev === 0 || postStat.dev === 0) return true; + return preStat.dev === postStat.dev; +} + +function assertRegularFileStat(stat: Stats, filePath: string): void { + if (stat.isFile()) return; + throw new Error(`File view target is not a file: ${filePath}`); +} + +function rewriteNoFollowOpenError(error: unknown, filePath: string): Error { + if ( + error instanceof Error && + "code" in error && + (error.code === "ELOOP" || error.code === "EMLINK" || error.code === "EINVAL") + ) { + return new Error( + `File changed between verification and open: ${normalizePath(filePath)} (possible path confinement race)`, + ); + } + if (error instanceof Error) return error; + return new Error(String(error)); +} + function isMissingPathError(error: unknown): boolean { return error instanceof Error && "code" in error && error.code === "ENOENT"; } diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 889900b4..1a7854ad 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import * as agentFacade from "../src/agent.js"; import { type AgentSession } from "../src/agent/session.js"; +import { setAfterConfinedPathVerifiedForTests } from "../src/util/confinedFile.js"; import { isSymlinkUnavailable } from "./helpers/filesystem.js"; const { createAgentSession, formatAgentFileViewResponse, getCodegraphFileView, getCodegraphFileViewWithSession } = @@ -42,6 +43,7 @@ async function writeSparseFile(root: string, relativePath: string, size: number) } afterEach(async () => { + setAfterConfinedPathVerifiedForTests(undefined); await Promise.all(Array.from(tempPaths, async (tempPath) => await fs.rm(tempPath, { recursive: true, force: true }))); tempPaths.clear(); }); @@ -227,6 +229,58 @@ describe("agent file view", () => { ); }); + it("refuses a TOCTOU symlink swap between confinement and open", async () => { + // Deterministic interleaving: setAfterConfinedPathVerifiedForTests runs after realpath + // confinement succeeds and before lstat/open, swapping the verified path for an outside symlink. + const root = await makeTempDir("cg-file-view-toctou-root-"); + const outside = await makeTempDir("cg-file-view-toctou-outside-"); + const victimRelative = "victim.txt"; + const victimPath = path.join(root, victimRelative); + const outsideFile = path.join(outside, "secret.txt"); + const outsideMarker = "TOCTOU_OUTSIDE_SECRET_CONTENT"; + await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); + await fs.writeFile(outsideFile, `${outsideMarker}\n`, "utf8"); + + const probeLink = path.join(root, "symlink-probe"); + try { + await fs.symlink(outsideFile, probeLink, "file"); + await fs.unlink(probeLink); + } catch (error) { + if (isSymlinkUnavailable(error)) return; + throw error; + } + + setAfterConfinedPathVerifiedForTests(async (realPath) => { + if (path.resolve(realPath) !== path.resolve(victimPath)) return; + await fs.unlink(victimPath); + await fs.symlink(outsideFile, victimPath, "file"); + }); + + await expect(getCodegraphFileView({ root, file: victimRelative, limit: 10, maxBytes: 100 })).rejects.toThrow( + /File view target is not a file:|File changed between verification and open:/, + ); + }); + + it("still reads ordinary text through an in-root symlink", async () => { + const root = await makeTempDir("cg-file-view-inroot-symlink-"); + const targetFile = path.join(root, "target-notes.txt"); + const linkedFile = path.join(root, "alias-notes.txt"); + await fs.writeFile(targetFile, "in-root-symlink-body\n", "utf8"); + try { + await fs.symlink(targetFile, linkedFile, "file"); + } catch (error) { + if (isSymlinkUnavailable(error)) return; + throw error; + } + + const result = await getCodegraphFileView({ root, file: "alias-notes.txt", limit: 10, maxBytes: 100 }); + expect(result).toMatchObject({ + file: "alias-notes.txt", + text: "in-root-symlink-body\n", + content: "1\tin-root-symlink-body\n2\t", + }); + }); + it("redacts a sensitive in-root symlink target even when the requested filename is benign", async () => { const root = await makeTempDir("cg-file-view-sensitive-symlink-"); const targetFile = path.join(root, ".env"); @@ -268,8 +322,10 @@ describe("agent file view", () => { throw error; } - const openSpy = vi.spyOn(fs, "open"); const readFileSpy = vi.spyOn(fs, "readFile"); + const probe = await fs.open(targetFile, "r"); + const handleReadSpy = vi.spyOn(Object.getPrototypeOf(probe), "read"); + await probe.close(); try { const redacted = await getCodegraphFileView({ root, file: "id_rsa", limit: 10, maxBytes: 100 }); @@ -282,11 +338,12 @@ describe("agent file view", () => { sensitive: { kind: "key-material", redacted: true, allowSensitiveRequired: true }, }); expect(JSON.stringify(redacted)).not.toContain(secretValue); - expect(openSpy).not.toHaveBeenCalled(); + // Descriptor open+fstat is required for TOCTOU-safe size metadata; content must stay unread. expect(readFileSpy).not.toHaveBeenCalled(); + expect(handleReadSpy).not.toHaveBeenCalled(); } finally { - openSpy.mockRestore(); readFileSpy.mockRestore(); + handleReadSpy.mockRestore(); } }); @@ -456,7 +513,7 @@ describe("agent file view", () => { let grewFile = false; openSpy.mockImplementation(async (target, flags) => { - if (!grewFile && target === filePath && flags === "r") { + if (!grewFile && path.resolve(String(target)) === path.resolve(filePath)) { grewFile = true; const growthHandle = await originalOpen(filePath, "w"); const chunk = Buffer.alloc(64 * 1024, 0x61); @@ -625,8 +682,9 @@ describe("agent file view", () => { sensitive: { kind: "key-material", redacted: true, allowSensitiveRequired: true }, }); expect(JSON.stringify(redacted)).not.toContain(marker); - expect(openSpy).not.toHaveBeenCalled(); expect(readFileSpy).not.toHaveBeenCalled(); + expect(openSpy).toHaveBeenCalled(); + openSpy.mockClear(); const allowed = await getCodegraphFileView({ root, @@ -644,7 +702,7 @@ describe("agent file view", () => { sensitive: { kind: "key-material", redacted: false, allowSensitiveRequired: true }, }); expect(openSpy).toHaveBeenCalledTimes(1); - expect(openSpy).toHaveBeenCalledWith(path.join(root, file), "r"); + expect(openSpy.mock.calls[0]?.[0]).toBe(path.join(root, file)); expect(readFileSpy).not.toHaveBeenCalled(); } finally { openSpy.mockRestore(); @@ -694,8 +752,10 @@ describe("agent file view", () => { }); expect(JSON.stringify(redacted)).not.toContain(marker); } - expect(openSpy).not.toHaveBeenCalled(); + // Metadata uses descriptor open+fstat; content bytes must remain unread. expect(readFileSpy).not.toHaveBeenCalled(); + expect(openSpy).toHaveBeenCalled(); + openSpy.mockClear(); const allowed = await getCodegraphFileView({ root, @@ -712,7 +772,7 @@ describe("agent file view", () => { sensitive: { kind: "key-material", redacted: false, allowSensitiveRequired: true }, }); expect(openSpy).toHaveBeenCalledTimes(1); - expect(openSpy).toHaveBeenCalledWith(path.join(root, "signing.key"), "r"); + expect(openSpy.mock.calls[0]?.[0]).toBe(path.join(root, "signing.key")); expect(readFileSpy).not.toHaveBeenCalled(); } finally { openSpy.mockRestore(); @@ -738,13 +798,16 @@ describe("agent file view", () => { content: `1\tSensitive key material omitted.\n2\tSize: ${oversizedBytes} bytes.`, sensitive: { kind: "key-material", redacted: true, allowSensitiveRequired: true }, }); - expect(openSpy).not.toHaveBeenCalled(); + // Descriptor open+fstat is required for TOCTOU-safe size metadata; content stays unread. + expect(openSpy).toHaveBeenCalledTimes(1); expect(readFileSpy).not.toHaveBeenCalled(); + openSpy.mockClear(); await expect( getCodegraphFileView({ root, file, limit: 10, maxBytes: 100, allowSensitive: true }), ).rejects.toThrow(`File exceeds the 16777216-byte file view input limit: ${filePath}`); - expect(openSpy).not.toHaveBeenCalled(); + // Open binds the descriptor for the size check; no content read follows. + expect(openSpy).toHaveBeenCalledTimes(1); expect(readFileSpy).not.toHaveBeenCalled(); } finally { openSpy.mockRestore(); From bf7b0b4c8a1864923c386ea195624a56f4641f62 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sat, 15 Aug 2026 22:54:21 -0400 Subject: [PATCH 2/8] fix: bind confined reads to verified identity --- src/util/confinedFile.ts | 48 +++++++++++++++++++++++++++---------- tests/file-view.test.ts | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index ed76c46b..932ab23d 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -12,11 +12,16 @@ export type ConfinedReadableFile = { }; type ConfinedFileTestHook = (realPath: string) => void | Promise; +type PreparedReadableFile = { + displayPath: string; + expectedStat: Stats; + realPath: string; +}; let afterConfinedPathVerifiedForTests: ConfinedFileTestHook | undefined; /** - * Test-only seam between path confinement and the open that binds a descriptor. + * Test-only seam after the trusted file identity is captured and before the descriptor opens. * Production code must leave this unset. */ export function setAfterConfinedPathVerifiedForTests(hook: ConfinedFileTestHook | undefined): void { @@ -27,20 +32,37 @@ export async function resolveReadableFile( realRoot: string, root: string, filePath: string, -): Promise<{ realPath: string; displayPath: string }> { +): Promise { const candidatePath = path.isAbsolute(filePath) ? path.resolve(filePath) : path.resolve(root, filePath); + const lexicalRelativePath = + toProjectRelativePath(root, candidatePath) ?? toProjectRelativePath(realRoot, candidatePath); + if (lexicalRelativePath === null) { + throw new Error(`File is outside project root: ${normalizePath(candidatePath)} (root: ${normalizePath(realRoot)})`); + } + const candidateStat = await fs.lstat(candidatePath); const realPath = await assertRealPathCandidateWithinRoot(realRoot, candidatePath, "File"); + const expectedStat = await fs.lstat(realPath); + assertRegularFileStat(expectedStat, realPath); + const finalRealPath = await fs.realpath(realPath); + if (!isFilePathWithinRoot(realRoot, finalRealPath)) { + throw new Error(`File is outside project root: ${normalizePath(finalRealPath)} (root: ${normalizePath(realRoot)})`); + } + if (candidateStat.isFile() && !sameFileIdentity(candidateStat, expectedStat)) { + throw new Error( + `File changed during confinement: ${normalizePath(candidatePath)} (possible path confinement race)`, + ); + } const displayPath = toProjectRelativePath(root, candidatePath) ?? toProjectRelativePath(realRoot, realPath) ?? normalizePath(realPath); - return { realPath, displayPath }; + return { realPath, displayPath, expectedStat }; } /** * Resolve a project path, open it, and verify the opened descriptor before any read. * - * Flow: realpath confinement → optional test hook → `lstat` (must be a regular file) → - * open the realpath'd target → `fstat` on that descriptor → identity check → callers read - * only through the returned handle (never re-resolve the path string). + * Flow: capture the lexical file identity -> realpath confinement -> capture the resolved regular + * file identity -> optional test hook -> open the realpath'd target -> `fstat` on that descriptor + * -> identity check -> callers read only through the returned handle (never re-resolve the path string). * * POSIX: open uses `O_RDONLY | O_NOFOLLOW` so a leaf symlink swap fails the open with ELOOP. * win32: Node's `fs.open` has no portable `O_NOFOLLOW` (`fs.constants.O_NOFOLLOW` is absent). @@ -56,11 +78,11 @@ export async function openConfinedReadableFile( root: string, filePath: string, ): Promise { - const { realPath, displayPath } = await resolveReadableFile(realRoot, root, filePath); + const { realPath, displayPath, expectedStat } = await resolveReadableFile(realRoot, root, filePath); if (afterConfinedPathVerifiedForTests) { await afterConfinedPathVerifiedForTests(realPath); } - const { handle, size } = await openVerifiedRegularFile(realPath); + const { handle, size } = await openVerifiedRegularFile(realPath, expectedStat); return { handle, realPath, displayPath, size }; } @@ -111,10 +133,10 @@ export async function findNearestExistingPath(filePath: string): Promise return current; } -async function openVerifiedRegularFile(realPath: string): Promise<{ handle: FileHandle; size: number }> { - const preStat = await fs.lstat(realPath); - assertRegularFileStat(preStat, realPath); - +async function openVerifiedRegularFile( + realPath: string, + expectedStat: Stats, +): Promise<{ handle: FileHandle; size: number }> { const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0); let handle: FileHandle; try { @@ -126,7 +148,7 @@ async function openVerifiedRegularFile(realPath: string): Promise<{ handle: File try { const postStat = await handle.stat(); assertRegularFileStat(postStat, realPath); - if (!sameFileIdentity(preStat, postStat)) { + if (!sameFileIdentity(expectedStat, postStat)) { throw new Error( `File changed between verification and open: ${normalizePath(realPath)} (possible path confinement race)`, ); diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 1a7854ad..2045f62e 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -261,6 +261,57 @@ describe("agent file view", () => { ); }); + it("refuses a TOCTOU hard-link replacement between confinement and open", async () => { + const root = await makeTempDir("cg-file-view-hardlink-race-root-"); + const outside = await makeTempDir("cg-file-view-hardlink-race-outside-"); + const victimRelative = "victim.txt"; + const victimPath = path.join(root, victimRelative); + const outsideFile = path.join(outside, "secret.txt"); + await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); + await fs.writeFile(outsideFile, "TOCTOU_OUTSIDE_SECRET_CONTENT\n", "utf8"); + + setAfterConfinedPathVerifiedForTests(async (realPath) => { + if (path.resolve(realPath) !== path.resolve(victimPath)) return; + await fs.unlink(victimPath); + await fs.link(outsideFile, victimPath); + }); + + await expect(getCodegraphFileView({ root, file: victimRelative, limit: 10, maxBytes: 100 })).rejects.toThrow( + /File changed between verification and open:/, + ); + }); + + it("refuses a TOCTOU parent-directory symlink swap between confinement and open", async () => { + const root = await makeTempDir("cg-file-view-parent-symlink-race-root-"); + const outside = await makeTempDir("cg-file-view-parent-symlink-race-outside-"); + const victimRelative = path.join("inside", "victim.txt"); + const insideDirectory = path.join(root, "inside"); + const victimPath = path.join(root, victimRelative); + const outsideVictim = path.join(outside, "victim.txt"); + await fs.mkdir(insideDirectory); + await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); + await fs.writeFile(outsideVictim, "TOCTOU_OUTSIDE_SECRET_CONTENT\n", "utf8"); + + const probeLink = path.join(root, "symlink-probe"); + try { + await fs.symlink(outside, probeLink, "junction"); + await fs.unlink(probeLink); + } catch (error) { + if (isSymlinkUnavailable(error)) return; + throw error; + } + + setAfterConfinedPathVerifiedForTests(async (realPath) => { + if (path.resolve(realPath) !== path.resolve(victimPath)) return; + await fs.rm(insideDirectory, { recursive: true }); + await fs.symlink(outside, insideDirectory, "junction"); + }); + + await expect(getCodegraphFileView({ root, file: victimRelative, limit: 10, maxBytes: 100 })).rejects.toThrow( + /File changed between verification and open:/, + ); + }); + it("still reads ordinary text through an in-root symlink", async () => { const root = await makeTempDir("cg-file-view-inroot-symlink-"); const targetFile = path.join(root, "target-notes.txt"); From f5ee839d7a188a27b7de4a38ae0e47ecd6dcfe15 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 16 Aug 2026 00:19:12 -0400 Subject: [PATCH 3/8] fix: verify all confined read identities --- src/util/confinedFile.ts | 44 ++++++++++++++--------------- tests/file-view.test.ts | 60 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 22 deletions(-) diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index 932ab23d..3b49c4a3 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -14,7 +14,7 @@ export type ConfinedReadableFile = { type ConfinedFileTestHook = (realPath: string) => void | Promise; type PreparedReadableFile = { displayPath: string; - expectedStat: Stats; + expectedStats: readonly Stats[]; realPath: string; }; @@ -39,7 +39,8 @@ export async function resolveReadableFile( if (lexicalRelativePath === null) { throw new Error(`File is outside project root: ${normalizePath(candidatePath)} (root: ${normalizePath(realRoot)})`); } - const candidateStat = await fs.lstat(candidatePath); + const candidateStat = await fs.stat(candidatePath); + assertRegularFileStat(candidateStat, candidatePath); const realPath = await assertRealPathCandidateWithinRoot(realRoot, candidatePath, "File"); const expectedStat = await fs.lstat(realPath); assertRegularFileStat(expectedStat, realPath); @@ -47,42 +48,42 @@ export async function resolveReadableFile( if (!isFilePathWithinRoot(realRoot, finalRealPath)) { throw new Error(`File is outside project root: ${normalizePath(finalRealPath)} (root: ${normalizePath(realRoot)})`); } - if (candidateStat.isFile() && !sameFileIdentity(candidateStat, expectedStat)) { - throw new Error( - `File changed during confinement: ${normalizePath(candidatePath)} (possible path confinement race)`, - ); - } const displayPath = toProjectRelativePath(root, candidatePath) ?? toProjectRelativePath(realRoot, realPath) ?? normalizePath(realPath); - return { realPath, displayPath, expectedStat }; + return { realPath, displayPath, expectedStats: [candidateStat, expectedStat] }; } /** * Resolve a project path, open it, and verify the opened descriptor before any read. * - * Flow: capture the lexical file identity -> realpath confinement -> capture the resolved regular - * file identity -> optional test hook -> open the realpath'd target -> `fstat` on that descriptor - * -> identity check -> callers read only through the returned handle (never re-resolve the path string). + * Flow: capture the lexical file identity (following any alias) -> realpath confinement -> capture + * the resolved regular file identity -> optional test hook -> open the realpath'd target -> `fstat` + * on that descriptor -> compare it to every pre-open identity -> callers read only through the + * returned handle (never re-resolve the path string). * * POSIX: open uses `O_RDONLY | O_NOFOLLOW` so a leaf symlink swap fails the open with ELOOP. * win32: Node's `fs.open` has no portable `O_NOFOLLOW` (`fs.constants.O_NOFOLLOW` is absent). - * Guarantee there is post-open identity: `fstat.ino` must match `lstat.ino`; `dev` is compared - * only when both sides are non-zero because win32 `lstat` often reports `dev=0` while `fstat` - * has the volume serial. A symlink swap after `lstat` yields a different inode on follow. + * Guarantee there is post-open identity: `fstat.ino` must match every pre-open identity. `dev` is + * compared when both sides expose it. When win32 `lstat.dev` is zero while `fstat.dev` has the + * volume serial, the creation time must also match, so a cross-volume junction swap cannot pass + * on a colliding inode alone. * * In-root symlinks still work: confinement realpaths them first, then the open targets the - * resolved regular file inside the root—not the symlink leaf. + * resolved regular file inside the root, not the symlink leaf. + * + * A pre-existing in-root hard link remains indistinguishable from one whose other directory entry + * is outside the root. Pathname confinement proves the opened descriptor, not hard-link provenance. */ export async function openConfinedReadableFile( realRoot: string, root: string, filePath: string, ): Promise { - const { realPath, displayPath, expectedStat } = await resolveReadableFile(realRoot, root, filePath); + const { realPath, displayPath, expectedStats } = await resolveReadableFile(realRoot, root, filePath); if (afterConfinedPathVerifiedForTests) { await afterConfinedPathVerifiedForTests(realPath); } - const { handle, size } = await openVerifiedRegularFile(realPath, expectedStat); + const { handle, size } = await openVerifiedRegularFile(realPath, expectedStats); return { handle, realPath, displayPath, size }; } @@ -135,7 +136,7 @@ export async function findNearestExistingPath(filePath: string): Promise async function openVerifiedRegularFile( realPath: string, - expectedStat: Stats, + expectedStats: readonly Stats[], ): Promise<{ handle: FileHandle; size: number }> { const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0); let handle: FileHandle; @@ -148,7 +149,7 @@ async function openVerifiedRegularFile( try { const postStat = await handle.stat(); assertRegularFileStat(postStat, realPath); - if (!sameFileIdentity(expectedStat, postStat)) { + if (!expectedStats.every((expectedStat) => sameFileIdentity(expectedStat, postStat))) { throw new Error( `File changed between verification and open: ${normalizePath(realPath)} (possible path confinement race)`, ); @@ -162,9 +163,8 @@ async function openVerifiedRegularFile( function sameFileIdentity(preStat: Stats, postStat: Stats): boolean { if (preStat.ino !== postStat.ino) return false; - // win32 lstat often reports dev=0 while fstat has the volume serial; only compare when both are set. - if (preStat.dev === 0 || postStat.dev === 0) return true; - return preStat.dev === postStat.dev; + if (preStat.dev !== 0 && postStat.dev !== 0) return preStat.dev === postStat.dev; + return preStat.birthtimeMs === postStat.birthtimeMs; } function assertRegularFileStat(stat: Stats, filePath: string): void { diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 2045f62e..d078a32f 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -281,6 +281,66 @@ describe("agent file view", () => { ); }); + it("refuses a symlink-alias target swap while resolving confinement", async () => { + const root = await makeTempDir("cg-file-view-alias-race-root-"); + const safeFile = path.join(root, "safe.txt"); + const replacementFile = path.join(root, "replacement.txt"); + const aliasFile = path.join(root, "alias.txt"); + await fs.writeFile(safeFile, "inside-safe-content\n", "utf8"); + await fs.writeFile(replacementFile, "TOCTOU_REPLACEMENT_CONTENT\n", "utf8"); + try { + await fs.symlink(safeFile, aliasFile, "file"); + } catch (error) { + if (isSymlinkUnavailable(error)) return; + throw error; + } + + const originalRealpath = fs.realpath.bind(fs); + const realpath = vi.spyOn(fs, "realpath"); + let swapped = false; + realpath.mockImplementation(async (candidate) => { + const resolved = await originalRealpath(candidate); + if (!swapped && path.resolve(String(candidate)) === path.resolve(aliasFile)) { + swapped = true; + await fs.unlink(aliasFile); + await fs.symlink(replacementFile, aliasFile, "file"); + } + return resolved; + }); + + try { + await expect(getCodegraphFileView({ root, file: "alias.txt", limit: 10, maxBytes: 100 })).rejects.toThrow( + /File changed during confinement:|File changed between verification and open:/, + ); + } finally { + realpath.mockRestore(); + } + }); + + it("rejects a mismatched fallback identity when lstat does not expose a device", async () => { + const root = await makeTempDir("cg-file-view-lstat-device-fallback-"); + const victimPath = path.join(root, "victim.txt"); + await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); + + const originalLstat = fs.lstat.bind(fs); + const lstat = vi.spyOn(fs, "lstat").mockImplementation(async (candidate) => { + const stat = await originalLstat(candidate); + if (path.resolve(String(candidate)) !== path.resolve(victimPath)) return stat; + return Object.assign(Object.create(stat), { + birthtimeMs: stat.birthtimeMs + 1, + dev: 0, + }); + }); + + try { + await expect(getCodegraphFileView({ root, file: "victim.txt", limit: 10, maxBytes: 100 })).rejects.toThrow( + /File changed between verification and open:/, + ); + } finally { + lstat.mockRestore(); + } + }); + it("refuses a TOCTOU parent-directory symlink swap between confinement and open", async () => { const root = await makeTempDir("cg-file-view-parent-symlink-race-root-"); const outside = await makeTempDir("cg-file-view-parent-symlink-race-outside-"); From d9ea174f75317197537aebb57b2dbae5452f0bca Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 16 Aug 2026 11:58:58 -0400 Subject: [PATCH 4/8] fix: classify rename file races correctly --- src/agent/renamePreview.ts | 2 +- src/util/confinedFile.ts | 6 +----- tests/file-view.test.ts | 5 +++-- tests/rename-preview.test.ts | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/agent/renamePreview.ts b/src/agent/renamePreview.ts index 3b291072..8baf7fbc 100644 --- a/src/agent/renamePreview.ts +++ b/src/agent/renamePreview.ts @@ -708,7 +708,7 @@ async function loadRenameFile( } catch (error: unknown) { const message = errorMessage(error); let reason: RenameUnsafeSite["reason"] = "unresolved_reference"; - if (/outside project root|changed between verification and open/i.test(message)) reason = "outside_root"; + if (/outside project root/i.test(message)) reason = "outside_root"; else if (/binary source|malformed UTF-8/i.test(message)) reason = "unsupported_syntax"; unsafeSites.push({ location: { file: normalizeAgentFilePath(snapshot.root, file), range: zeroRange() }, diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index 3b49c4a3..c4b4ff20 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -44,10 +44,6 @@ export async function resolveReadableFile( const realPath = await assertRealPathCandidateWithinRoot(realRoot, candidatePath, "File"); const expectedStat = await fs.lstat(realPath); assertRegularFileStat(expectedStat, realPath); - const finalRealPath = await fs.realpath(realPath); - if (!isFilePathWithinRoot(realRoot, finalRealPath)) { - throw new Error(`File is outside project root: ${normalizePath(finalRealPath)} (root: ${normalizePath(realRoot)})`); - } const displayPath = toProjectRelativePath(root, candidatePath) ?? toProjectRelativePath(realRoot, realPath) ?? normalizePath(realPath); return { realPath, displayPath, expectedStats: [candidateStat, expectedStat] }; @@ -169,7 +165,7 @@ function sameFileIdentity(preStat: Stats, postStat: Stats): boolean { function assertRegularFileStat(stat: Stats, filePath: string): void { if (stat.isFile()) return; - throw new Error(`File view target is not a file: ${filePath}`); + throw new Error(`File view target is not a file: ${normalizePath(filePath)}`); } function rewriteNoFollowOpenError(error: unknown, filePath: string): Error { diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index d078a32f..79b5e676 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -930,8 +930,9 @@ describe("agent file view", () => { const root = await makeTempDir("cg-file-view-key-directory-"); await fs.mkdir(path.join(root, "identity.pem")); - await expect(getCodegraphFileView({ root, file: "identity.pem", limit: 10, maxBytes: 100 })).rejects.toThrow( - /File view target is not a file:/, + const directoryPath = path.join(root, "identity.pem"); + await expect(getCodegraphFileView({ root, file: directoryPath, limit: 10, maxBytes: 100 })).rejects.toThrow( + `File view target is not a file: ${directoryPath.replace(/\\/g, "/")}`, ); }); diff --git a/tests/rename-preview.test.ts b/tests/rename-preview.test.ts index d34d0c01..e36bf08d 100644 --- a/tests/rename-preview.test.ts +++ b/tests/rename-preview.test.ts @@ -7,6 +7,7 @@ import { workspaceSymbolsInSnapshot, workspaceSymbolsWithSession } from "../src/ import { buildProjectIndexFromFiles } from "../src/indexer/build-index.js"; import { isSymlinkUnavailable, mkTmpDir } from "./helpers/filesystem.js"; import { fileIdentityKey } from "../src/util/paths.js"; +import { setAfterConfinedPathVerifiedForTests } from "../src/util/confinedFile.js"; async function renameFixture() { const root = await mkTmpDir("cg-rename-preview-"); @@ -483,6 +484,39 @@ describe("rename preview", () => { expect(deleted.unsafeSites.some((site) => site.reason === "unresolved_reference")).toBe(true); }); + it("reports a verified file replacement as an unresolved reference", async () => { + const { root, serviceFile, session, target } = await renameFixture(); + const replacement = path.join(root, "replacement.ts"); + await fsp.writeFile(replacement, "export function service(): number { return 2; }\n"); + let swapped = false; + setAfterConfinedPathVerifiedForTests(async (realPath) => { + if (swapped || path.resolve(realPath) !== path.resolve(serviceFile)) return; + swapped = true; + await fsp.rename(replacement, serviceFile); + }); + + try { + const result = await previewRenameWithSession(session, { + root, + handle: target.handle, + newName: "renamedService", + }); + + expect(result.safe).toBe(false); + expect(result.unsafeSites).toContainEqual( + expect.objectContaining({ + reason: "unresolved_reference", + provenance: expect.objectContaining({ + reason: expect.stringMatching(/changed between verification and open/i), + }), + }), + ); + expect(result.unsafeSites).not.toContainEqual(expect.objectContaining({ reason: "outside_root" })); + } finally { + setAfterConfinedPathVerifiedForTests(undefined); + } + }); + it("refuses source symlinks that resolve outside the project root", async (context) => { const root = await mkTmpDir("cg-rename-root-"); const outsideRoot = await mkTmpDir("cg-rename-outside-"); From 91df0952c6d3f52037874ad40e244890323751a1 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 16 Aug 2026 16:31:10 -0400 Subject: [PATCH 5/8] fix: generalize confined-file error message and TOCTOU test comment --- src/util/confinedFile.ts | 2 +- tests/file-view.test.ts | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index c4b4ff20..bdaaeeb3 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -165,7 +165,7 @@ function sameFileIdentity(preStat: Stats, postStat: Stats): boolean { function assertRegularFileStat(stat: Stats, filePath: string): void { if (stat.isFile()) return; - throw new Error(`File view target is not a file: ${normalizePath(filePath)}`); + throw new Error(`Confined file target is not a file: ${normalizePath(filePath)}`); } function rewriteNoFollowOpenError(error: unknown, filePath: string): Error { diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 79b5e676..00344ce9 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -231,7 +231,8 @@ describe("agent file view", () => { it("refuses a TOCTOU symlink swap between confinement and open", async () => { // Deterministic interleaving: setAfterConfinedPathVerifiedForTests runs after realpath - // confinement succeeds and before lstat/open, swapping the verified path for an outside symlink. + // confinement succeeds (lstat already captured) and before the descriptor opens, swapping the + // verified path for an outside symlink. const root = await makeTempDir("cg-file-view-toctou-root-"); const outside = await makeTempDir("cg-file-view-toctou-outside-"); const victimRelative = "victim.txt"; @@ -257,7 +258,7 @@ describe("agent file view", () => { }); await expect(getCodegraphFileView({ root, file: victimRelative, limit: 10, maxBytes: 100 })).rejects.toThrow( - /File view target is not a file:|File changed between verification and open:/, + /Confined file target is not a file:|File changed between verification and open:/, ); }); @@ -932,7 +933,7 @@ describe("agent file view", () => { const directoryPath = path.join(root, "identity.pem"); await expect(getCodegraphFileView({ root, file: directoryPath, limit: 10, maxBytes: 100 })).rejects.toThrow( - `File view target is not a file: ${directoryPath.replace(/\\/g, "/")}`, + `Confined file target is not a file: ${directoryPath.replace(/\\/g, "/")}`, ); }); From d8abb71498b5235747fd120933ca2a150fa5714f Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 16 Aug 2026 17:30:16 -0400 Subject: [PATCH 6/8] fix: use 64-bit inode identity and prevent FIFO-swap open hang - sameFileIdentity now compares BigIntStats (ino/dev/birthtimeMs) instead of Number-backed Stats, so inode values above Number.MAX_SAFE_INTEGER can no longer collide and let a swapped file pass identity verification. - openVerifiedRegularFile opens with O_NONBLOCK in addition to O_NOFOLLOW so a verified regular file swapped for a FIFO before the open cannot hang the caller; POSIX ignores O_NONBLOCK for regular files, so normal reads are unaffected. - Add a POSIX-only regression test that swaps a verified file for a FIFO via the test hook and asserts the open rejects instead of hanging. --- src/util/confinedFile.ts | 26 +++++++++++++++----------- tests/file-view.test.ts | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index bdaaeeb3..012280d0 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -1,4 +1,4 @@ -import { constants as fsConstants, type Stats } from "node:fs"; +import { constants as fsConstants, type BigIntStats } from "node:fs"; import fs, { type FileHandle } from "node:fs/promises"; import path from "node:path"; @@ -14,7 +14,7 @@ export type ConfinedReadableFile = { type ConfinedFileTestHook = (realPath: string) => void | Promise; type PreparedReadableFile = { displayPath: string; - expectedStats: readonly Stats[]; + expectedStats: readonly BigIntStats[]; realPath: string; }; @@ -39,10 +39,10 @@ export async function resolveReadableFile( if (lexicalRelativePath === null) { throw new Error(`File is outside project root: ${normalizePath(candidatePath)} (root: ${normalizePath(realRoot)})`); } - const candidateStat = await fs.stat(candidatePath); + const candidateStat = await fs.stat(candidatePath, { bigint: true }); assertRegularFileStat(candidateStat, candidatePath); const realPath = await assertRealPathCandidateWithinRoot(realRoot, candidatePath, "File"); - const expectedStat = await fs.lstat(realPath); + const expectedStat = await fs.lstat(realPath, { bigint: true }); assertRegularFileStat(expectedStat, realPath); const displayPath = toProjectRelativePath(root, candidatePath) ?? toProjectRelativePath(realRoot, realPath) ?? normalizePath(realPath); @@ -132,9 +132,13 @@ export async function findNearestExistingPath(filePath: string): Promise async function openVerifiedRegularFile( realPath: string, - expectedStats: readonly Stats[], + expectedStats: readonly BigIntStats[], ): Promise<{ handle: FileHandle; size: number }> { - const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0); + // O_NONBLOCK guards against a verified regular file being swapped for a FIFO before this open: + // without it, opening a FIFO for O_RDONLY blocks until a writer connects, hanging the caller + // before the fstat() below can reject the non-regular target. POSIX ignores O_NONBLOCK on + // regular files, so normal reads through the returned handle are unaffected. + const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0) | (fsConstants.O_NONBLOCK ?? 0); let handle: FileHandle; try { handle = await fs.open(realPath, openFlags); @@ -143,27 +147,27 @@ async function openVerifiedRegularFile( } try { - const postStat = await handle.stat(); + const postStat = await handle.stat({ bigint: true }); assertRegularFileStat(postStat, realPath); if (!expectedStats.every((expectedStat) => sameFileIdentity(expectedStat, postStat))) { throw new Error( `File changed between verification and open: ${normalizePath(realPath)} (possible path confinement race)`, ); } - return { handle, size: postStat.size }; + return { handle, size: Number(postStat.size) }; } catch (error) { await handle.close().catch(() => undefined); throw error; } } -function sameFileIdentity(preStat: Stats, postStat: Stats): boolean { +function sameFileIdentity(preStat: BigIntStats, postStat: BigIntStats): boolean { if (preStat.ino !== postStat.ino) return false; - if (preStat.dev !== 0 && postStat.dev !== 0) return preStat.dev === postStat.dev; + if (preStat.dev !== 0n && postStat.dev !== 0n) return preStat.dev === postStat.dev; return preStat.birthtimeMs === postStat.birthtimeMs; } -function assertRegularFileStat(stat: Stats, filePath: string): void { +function assertRegularFileStat(stat: BigIntStats, filePath: string): void { if (stat.isFile()) return; throw new Error(`Confined file target is not a file: ${normalizePath(filePath)}`); } diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 00344ce9..8c46f84d 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from "node:child_process"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -42,6 +43,14 @@ async function writeSparseFile(root: string, relativePath: string, size: number) return filePath; } +function isMkfifoUnavailable(error: unknown): boolean { + return ( + error instanceof Error && + "code" in error && + (error.code === "ENOENT" || error.code === "EPERM" || error.code === "ENOTSUP") + ); +} + afterEach(async () => { setAfterConfinedPathVerifiedForTests(undefined); await Promise.all(Array.from(tempPaths, async (tempPath) => await fs.rm(tempPath, { recursive: true, force: true }))); @@ -262,6 +271,37 @@ describe("agent file view", () => { ); }); + it("refuses a TOCTOU FIFO swap between confinement and open without hanging", async () => { + // Guards the O_NONBLOCK fix: opening a FIFO with O_RDONLY blocks until a writer connects, + // so without O_NONBLOCK a verified regular file swapped for a FIFO would hang this open + // forever instead of failing fast once fstat proves the descriptor is not a regular file. + // FIFOs are a POSIX filesystem feature; Windows has no equivalent node reachable through + // fs.open, so this race cannot be reproduced there. + if (process.platform === "win32") return; + const root = await makeTempDir("cg-file-view-fifo-race-root-"); + const victimRelative = "victim.txt"; + const victimPath = path.join(root, victimRelative); + const fifoPath = path.join(root, "victim.fifo"); + await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); + + try { + execFileSync("mkfifo", [fifoPath]); + } catch (error) { + if (isMkfifoUnavailable(error)) return; + throw error; + } + + setAfterConfinedPathVerifiedForTests(async (realPath) => { + if (path.resolve(realPath) !== path.resolve(victimPath)) return; + await fs.unlink(victimPath); + await fs.rename(fifoPath, victimPath); + }); + + await expect(getCodegraphFileView({ root, file: victimRelative, limit: 10, maxBytes: 100 })).rejects.toThrow( + /Confined file target is not a file:/, + ); + }, 5_000); + it("refuses a TOCTOU hard-link replacement between confinement and open", async () => { const root = await makeTempDir("cg-file-view-hardlink-race-root-"); const outside = await makeTempDir("cg-file-view-hardlink-race-outside-"); From 3af6c3c0f935e97c225110f125fd3d2cef37dd11 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 16 Aug 2026 17:57:25 -0400 Subject: [PATCH 7/8] fix: make lstat mock produce BigIntStats in fallback identity test The mock intercepted fs.lstat but called the original without forwarding the { bigint: true } options, so it returned Number-backed Stats while production now always requests BigIntStats. The mismatched numeric ino never matched the descriptor's bigint ino, so the test failed at the inode check before reaching the intended birth-time fallback comparison. Forward options through to the real lstat and use bigint literals for the injected overrides. --- tests/file-view.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 8c46f84d..5f062410 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -364,12 +364,12 @@ describe("agent file view", () => { await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); const originalLstat = fs.lstat.bind(fs); - const lstat = vi.spyOn(fs, "lstat").mockImplementation(async (candidate) => { - const stat = await originalLstat(candidate); + const lstat = vi.spyOn(fs, "lstat").mockImplementation(async (candidate, options) => { + const stat = await originalLstat(candidate, options as { bigint: true }); if (path.resolve(String(candidate)) !== path.resolve(victimPath)) return stat; return Object.assign(Object.create(stat), { - birthtimeMs: stat.birthtimeMs + 1, - dev: 0, + birthtimeMs: stat.birthtimeMs + 1n, + dev: 0n, }); }); From c922188baf86b4434d9fd952dd444cd9290a1611 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 16 Aug 2026 18:08:56 -0400 Subject: [PATCH 8/8] fix: require birth-time match for TOCTOU identity and close test blind spots - sameFileIdentity now always requires nanosecond-precision birthtimeNs equality in addition to ino, not just as a fallback when dev is unavailable. Without an open descriptor pinning identity across the pre-open checks, an unlink+recreate on the same filesystem can reuse the same inode; dev+ino equality alone cannot prove it is the same file. - Split the zero-device fallback test into a reject case (mismatched birthtimeNs) and a new accept case (matched birthtimeNs) so an implementation that rejects every dev-unavailable read would fail CI. - Spy on FileHandle.readFile in the sensitive-symlink metadata-only test, not just FileHandle.read and fs.readFile, so a future redacted-branch regression that reads via handle.readFile() would be caught. --- src/util/confinedFile.ts | 8 ++++++-- tests/file-view.test.ts | 29 +++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/util/confinedFile.ts b/src/util/confinedFile.ts index 012280d0..964e38d2 100644 --- a/src/util/confinedFile.ts +++ b/src/util/confinedFile.ts @@ -163,8 +163,12 @@ async function openVerifiedRegularFile( function sameFileIdentity(preStat: BigIntStats, postStat: BigIntStats): boolean { if (preStat.ino !== postStat.ino) return false; - if (preStat.dev !== 0n && postStat.dev !== 0n) return preStat.dev === postStat.dev; - return preStat.birthtimeMs === postStat.birthtimeMs; + // Birth time (nanosecond precision) must always match: an unlink+recreate on the same + // filesystem can reuse the same inode, and no descriptor is held open across the pre-open + // checks to pin identity, so ino/dev equality alone cannot prove it is the same file. + if (preStat.birthtimeNs !== postStat.birthtimeNs) return false; + if (preStat.dev === 0n || postStat.dev === 0n) return true; + return preStat.dev === postStat.dev; } function assertRegularFileStat(stat: BigIntStats, filePath: string): void { diff --git a/tests/file-view.test.ts b/tests/file-view.test.ts index 5f062410..ac30dbde 100644 --- a/tests/file-view.test.ts +++ b/tests/file-view.test.ts @@ -368,7 +368,7 @@ describe("agent file view", () => { const stat = await originalLstat(candidate, options as { bigint: true }); if (path.resolve(String(candidate)) !== path.resolve(victimPath)) return stat; return Object.assign(Object.create(stat), { - birthtimeMs: stat.birthtimeMs + 1n, + birthtimeNs: stat.birthtimeNs + 1n, dev: 0n, }); }); @@ -382,6 +382,27 @@ describe("agent file view", () => { } }); + it("accepts a matched fallback identity when lstat does not expose a device", async () => { + const root = await makeTempDir("cg-file-view-lstat-device-fallback-match-"); + const victimPath = path.join(root, "victim.txt"); + await fs.writeFile(victimPath, "inside-safe-content\n", "utf8"); + + const originalLstat = fs.lstat.bind(fs); + const lstat = vi.spyOn(fs, "lstat").mockImplementation(async (candidate, options) => { + const stat = await originalLstat(candidate, options as { bigint: true }); + if (path.resolve(String(candidate)) !== path.resolve(victimPath)) return stat; + // Birth time is unchanged; only the device id is unavailable, as on some lstat paths. + return Object.assign(Object.create(stat), { dev: 0n }); + }); + + try { + const view = await getCodegraphFileView({ root, file: "victim.txt", limit: 10, maxBytes: 100 }); + expect(view.content).toBe("1\tinside-safe-content\n2\t"); + } finally { + lstat.mockRestore(); + } + }); + it("refuses a TOCTOU parent-directory symlink swap between confinement and open", async () => { const root = await makeTempDir("cg-file-view-parent-symlink-race-root-"); const outside = await makeTempDir("cg-file-view-parent-symlink-race-outside-"); @@ -476,7 +497,9 @@ describe("agent file view", () => { const readFileSpy = vi.spyOn(fs, "readFile"); const probe = await fs.open(targetFile, "r"); - const handleReadSpy = vi.spyOn(Object.getPrototypeOf(probe), "read"); + const handleProto = Object.getPrototypeOf(probe); + const handleReadSpy = vi.spyOn(handleProto, "read"); + const handleReadFileSpy = vi.spyOn(handleProto, "readFile"); await probe.close(); try { @@ -493,9 +516,11 @@ describe("agent file view", () => { // Descriptor open+fstat is required for TOCTOU-safe size metadata; content must stay unread. expect(readFileSpy).not.toHaveBeenCalled(); expect(handleReadSpy).not.toHaveBeenCalled(); + expect(handleReadFileSpy).not.toHaveBeenCalled(); } finally { readFileSpy.mockRestore(); handleReadSpy.mockRestore(); + handleReadFileSpy.mockRestore(); } });