diff --git a/integration-tests/tsconfig.json b/integration-tests/tsconfig.json index 7b6ffce1bf9..d1383fb86e2 100644 --- a/integration-tests/tsconfig.json +++ b/integration-tests/tsconfig.json @@ -45,6 +45,9 @@ "@qwen-code/qwen-code-core/transcriptRecords": [ "../packages/core/src/utils/transcript-records.ts" ], + "@qwen-code/qwen-code-core/noFollowOpen": [ + "../packages/core/src/utils/no-follow-open.ts" + ], "@qwen-code/qwen-code-core/goalWire": [ "../packages/core/src/goals/goal-wire.ts" ], diff --git a/packages/acp-bridge/src/sessionArtifacts.test.ts b/packages/acp-bridge/src/sessionArtifacts.test.ts index f1014438747..51c2ca85bdd 100644 --- a/packages/acp-bridge/src/sessionArtifacts.test.ts +++ b/packages/acp-bridge/src/sessionArtifacts.test.ts @@ -21,6 +21,7 @@ import { type SessionArtifactEventRecordPayload, type SessionArtifactSnapshotRecordPayload, } from '@qwen-code/qwen-code-core'; +import { UNVERIFIABLE_IDENTITY_CODE } from '@qwen-code/qwen-code-core/noFollowOpen'; vi.mock('@xterm/headless', () => ({ Terminal: class Terminal {}, @@ -4087,6 +4088,93 @@ describe('SessionArtifactStore', () => { } }); + it('treats an unverifiable inode identity as missing, not an escape, on upsert', async () => { + // On volumes that never report inode numbers (ino 0: FAT/exFAT, some + // SMB shares) openNoFollow cannot prove the opened file matches the + // pre-open check and refuses with UNVERIFIABLE_IDENTITY_CODE. The + // upsert must then degrade to a plain missing artifact — it must not + // reject outright, and it must not raise the symlink-escape flag for a + // path the containment check already accepted (#8227 follow-up). + const store = new SessionArtifactStore({ + sessionId: 's7-unverifiable-upsert', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'report.txt'), 'hello'); + const originalOpen = fs.open.bind(fs); + const openSpy = vi + .spyOn(fs, 'open') + .mockImplementation(async (entry, flags, mode) => { + if (String(entry).endsWith('report.txt')) { + throw Object.assign(new Error('inode 0 cannot be verified'), { + code: UNVERIFIABLE_IDENTITY_CODE, + }); + } + return originalOpen(entry, flags, mode); + }); + + try { + const created = await store.upsertMany( + [{ title: 'Report', workspacePath: 'report.txt' }], + { strict: true }, + ); + expect(created.changes).toHaveLength(1); + expect(created.changes[0]).toMatchObject({ + action: 'created', + artifact: expect.objectContaining({ + status: 'missing', + workspacePath: 'report.txt', + }), + }); + } finally { + openSpy.mockRestore(); + } + }); + + it('keeps reporting unverifiable artifacts missing on refresh without an escape flag', async () => { + const store = new SessionArtifactStore({ + sessionId: 's7-unverifiable-refresh', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'report.txt'), 'hello'); + await store.upsertMany([{ title: 'Report', workspacePath: 'report.txt' }], { + strict: true, + }); + + vi.useFakeTimers(); + vi.setSystemTime(new Date(Date.now() + 6_000)); + const originalOpen = fs.open.bind(fs); + const openSpy = vi + .spyOn(fs, 'open') + .mockImplementation(async (entry, flags, mode) => { + if (String(entry).endsWith('report.txt')) { + throw Object.assign(new Error('inode 0 cannot be verified'), { + code: UNVERIFIABLE_IDENTITY_CODE, + }); + } + return originalOpen(entry, flags, mode); + }); + const stderr = vi + .spyOn(process.stderr, 'write') + .mockReturnValue(true as never); + + try { + const artifact = (await store.list()).artifacts[0]; + expect(artifact).toMatchObject({ + status: 'missing', + workspacePath: 'report.txt', + }); + expect(artifact).not.toHaveProperty('sizeBytes'); + // The degradation is a graceful missing status, not a refresh error: + // deleting the branch would re-throw the refusal and log this marker. + const logged = stderr.mock.calls.map((call) => String(call[0])).join(''); + expect(logged).not.toContain('status_refresh_failed'); + } finally { + vi.useRealTimers(); + openSpy.mockRestore(); + stderr.mockRestore(); + } + }); + it('rejects relative dangling symlinks that point outside the workspace', async () => { const store = new SessionArtifactStore({ sessionId: 's7-dangling-symlink', diff --git a/packages/acp-bridge/src/sessionArtifacts.ts b/packages/acp-bridge/src/sessionArtifacts.ts index 9aa294a3ea5..51b3d2fcd30 100644 --- a/packages/acp-bridge/src/sessionArtifacts.ts +++ b/packages/acp-bridge/src/sessionArtifacts.ts @@ -5,12 +5,7 @@ */ import { createHash } from 'node:crypto'; -import { - constants as fsConstants, - promises as fs, - type BigIntStats, - type Stats, -} from 'node:fs'; +import { promises as fs, type BigIntStats, type Stats } from 'node:fs'; import type { FileHandle } from 'node:fs/promises'; import path from 'node:path'; import { @@ -40,6 +35,10 @@ import type { SessionArtifactRetention, SessionArtifactSnapshotRecordPayload, } from '@qwen-code/qwen-code-core'; +import { + isUnverifiableIdentityError, + openNoFollow, +} from '@qwen-code/qwen-code-core/noFollowOpen'; import { writeStderrLine } from './internal/stderrLine.js'; export type DaemonSessionArtifactKind = @@ -3302,10 +3301,10 @@ async function getWorkspaceStatus( // Number spelling loses precision above 2^53, so two files created close // together can round to the SAME numeric ino and defeat the swap check. const preOpenStat = await fs.lstat(realPath, { bigint: true }); - const handle = await fs.open( - realPath, - fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW, - ); + // Where O_NOFOLLOW does not exist (Windows) the helper compensates + // with an lstat/open/fstat identity check instead of collapsing to a + // plain open that follows symlinks (#8227). + const handle = await openNoFollow(realPath); try { if (!isSameFile(preOpenStat, await handle.stat({ bigint: true }))) { return { status: 'missing', escaped: true }; @@ -3374,6 +3373,13 @@ async function getWorkspaceStatus( if (isNoFollowSymlinkError(error)) { return { status: 'missing', escaped: true }; } + if (isUnverifiableIdentityError(error)) { + // inode-0 volume: the file could not be proven identical to the one + // the pre-open check saw. Fail closed like a missing artifact, but + // do NOT flag a symlink escape we did not observe — the path passed + // the containment check above (#8227 follow-up). + return { status: 'missing' }; + } if (!isNotFoundError(error)) { throw error; } diff --git a/packages/acp-bridge/tsconfig.json b/packages/acp-bridge/tsconfig.json index 13546fa1b15..4bb585f54b8 100644 --- a/packages/acp-bridge/tsconfig.json +++ b/packages/acp-bridge/tsconfig.json @@ -15,6 +15,9 @@ "@qwen-code/qwen-code-core/transcriptRecords": [ "../core/src/utils/transcript-records.ts" ], + "@qwen-code/qwen-code-core/noFollowOpen": [ + "../core/src/utils/no-follow-open.ts" + ], "@qwen-code/qwen-code-core/*": ["../core/src/*"] } }, diff --git a/packages/acp-bridge/vitest.config.ts b/packages/acp-bridge/vitest.config.ts index e001edd2dbf..7b241c6ddcc 100644 --- a/packages/acp-bridge/vitest.config.ts +++ b/packages/acp-bridge/vitest.config.ts @@ -10,6 +10,10 @@ import path from 'node:path'; export default defineConfig({ resolve: { alias: { + '@qwen-code/qwen-code-core/noFollowOpen': path.resolve( + __dirname, + '../core/src/utils/no-follow-open.ts', + ), '@qwen-code/qwen-code-core/subSessionConstants': path.resolve( __dirname, '../core/src/tools/sub-session-constants.ts', diff --git a/packages/cli/src/serve/workspace-registration-store.test.ts b/packages/cli/src/serve/workspace-registration-store.test.ts index 7dab866bf62..a7ff4d75383 100644 --- a/packages/cli/src/serve/workspace-registration-store.test.ts +++ b/packages/cli/src/serve/workspace-registration-store.test.ts @@ -391,6 +391,57 @@ describe('WorkspaceRegistrationStore', () => { await expect(store.read()).rejects.toThrow(/regular file/); }); + it('reports an unverifiable store identity as a store error', async () => { + const home = await tempHome(); + vi.resetModules(); + vi.doMock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + const modifiedPromises = { + ...actual.promises, + lstat: vi.fn( + async (...args: Parameters) => { + const stats = await actual.promises.lstat(...args); + return new Proxy(stats, { + get: (target, property, receiver) => + property === 'ino' + ? 0 + : Reflect.get(target, property, receiver), + }); + }, + ), + }; + const modified = { + ...actual, + constants: { ...actual.constants, O_NOFOLLOW: undefined }, + promises: modifiedPromises, + }; + return { ...modified, default: modified }; + }); + try { + const storeModule = await import('./workspace-registration-store.js'); + const store = new storeModule.WorkspaceRegistrationStore( + '/work/primary', + home, + ); + await fs.mkdir(path.dirname(store.filePath), { recursive: true }); + await fs.writeFile( + store.filePath, + JSON.stringify({ + schemaVersion: 1, + primaryWorkspace: '/work/primary', + workspaces: [], + }), + ); + + await expect(store.read()).rejects.toThrow( + /identity could not be verified/, + ); + } finally { + vi.doUnmock('node:fs'); + vi.resetModules(); + } + }); + it('rejects an oversized store', async () => { const home = await tempHome(); const store = new WorkspaceRegistrationStore('/work/primary', home); @@ -472,9 +523,16 @@ describe('WorkspaceRegistrationStore', () => { }), }, })); - vi.doMock('@qwen-code/qwen-code-core', () => ({ - atomicWriteFile: vi.fn().mockRejectedValue(writeError), - })); + // The read path uses the leaf noFollowOpen export, so this barrel mock + // remains limited to the deferred write helper under test. + vi.doMock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + atomicWriteFile: vi.fn().mockRejectedValue(writeError), + }; + }); try { const storeModule = await import('./workspace-registration-store.js'); const store = new storeModule.WorkspaceRegistrationStore( diff --git a/packages/cli/src/serve/workspace-registration-store.ts b/packages/cli/src/serve/workspace-registration-store.ts index 59135329081..4b97a64d036 100644 --- a/packages/cli/src/serve/workspace-registration-store.ts +++ b/packages/cli/src/serve/workspace-registration-store.ts @@ -5,11 +5,14 @@ */ import { createHash } from 'node:crypto'; -import { constants } from 'node:fs'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import lockfile from 'proper-lockfile'; +import { + isUnverifiableIdentityError, + openNoFollow, +} from '@qwen-code/qwen-code-core/noFollowOpen'; import { MAX_WORKSPACE_PATH_LENGTH } from '@qwen-code/acp-bridge/workspacePaths'; import { getGlobalQwenDirLite } from '../config/storage-paths-lite.js'; import { MAX_REGISTERED_WORKSPACES } from './workspace-inputs.js'; @@ -355,14 +358,23 @@ export class WorkspaceRegistrationStore { } let file: Awaited>; try { - file = await fs.open( - this.filePath, - (constants.O_RDONLY ?? 0) | (constants.O_NOFOLLOW ?? 0), - ); + // Where O_NOFOLLOW does not exist (Windows) the helper compensates + // with an lstat/open/fstat identity check instead of collapsing to a + // plain open that follows symlinks (#8227). + file = await openNoFollow(this.filePath); } catch (err) { if ((err as NodeJS.ErrnoException).code === 'ENOENT') { return emptySnapshot(this.primaryWorkspace); } + if (isUnverifiableIdentityError(err)) { + // inode-0 volume: the store could not be proven identical to the + // file the pre-open check saw. Fail closed, but do not claim it + // "must be a regular file" — the lstat gate above already proved + // it is one (#8227 follow-up). + throw new WorkspaceRegistrationStoreError( + 'Workspace registration store identity could not be verified', + ); + } if ((err as NodeJS.ErrnoException).code === 'ELOOP') { throw new WorkspaceRegistrationStoreError( 'Workspace registration store must be a regular file', diff --git a/packages/cli/tsconfig.json b/packages/cli/tsconfig.json index 5b29feb7591..2bf90f9665e 100644 --- a/packages/cli/tsconfig.json +++ b/packages/cli/tsconfig.json @@ -14,6 +14,9 @@ "@qwen-code/qwen-code-core/transcriptRecords": [ "../core/src/utils/transcript-records.ts" ], + "@qwen-code/qwen-code-core/noFollowOpen": [ + "../core/src/utils/no-follow-open.ts" + ], "@qwen-code/qwen-code-core/*": ["../core/src/*"], "@qwen-code/acp-bridge": ["../acp-bridge/src/index.ts"], "@qwen-code/acp-bridge/transcriptReplay": [ diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 2a1ac87d7dd..3d0b902c8be 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -11,6 +11,10 @@ import path from 'node:path'; export default defineConfig({ resolve: { alias: { + '@qwen-code/qwen-code-core/noFollowOpen': path.resolve( + __dirname, + '../core/src/utils/no-follow-open.ts', + ), '@qwen-code/qwen-code-core/subSessionConstants': path.resolve( __dirname, '../core/src/tools/sub-session-constants.ts', diff --git a/packages/core/package.json b/packages/core/package.json index bc7cb5d1966..257a7e81585 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -37,6 +37,10 @@ "types": "./dist/src/hooks/user-prompt-submit-context.d.ts", "import": "./dist/src/hooks/user-prompt-submit-context.js" }, + "./noFollowOpen": { + "types": "./dist/src/utils/no-follow-open.d.ts", + "import": "./dist/src/utils/no-follow-open.js" + }, "./package.json": "./package.json", "./dist/*": "./dist/*", "./src/*": "./src/*" diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index bc5054567f7..1a47e05ec68 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -151,6 +151,12 @@ export { export { atomicWriteFile } from './utils/atomicFileWrite.js'; export { nextFireTime, parseCron } from './utils/cronParser.js'; export { isWsl } from './utils/terminal-env.js'; +export { + isUnverifiableIdentityError, + openNoFollow, + openSyncNoFollow, + UNVERIFIABLE_IDENTITY_CODE, +} from './utils/no-follow-open.js'; export * from './services/session-organization-service.js'; // Backward-compatible type re-exports for tool classes removed from eager loading. diff --git a/packages/core/src/services/backgroundShellRegistry.test.ts b/packages/core/src/services/backgroundShellRegistry.test.ts index 98ee01569ce..3dc9bd7592d 100644 --- a/packages/core/src/services/backgroundShellRegistry.test.ts +++ b/packages/core/src/services/backgroundShellRegistry.test.ts @@ -559,6 +559,51 @@ describe('BackgroundShellRegistry', () => { expect(modelText).toContain(' { + // Cross-product the test above misses: Windows has no O_NOFOLLOW + // (the constant is `undefined` and `| (O_NOFOLLOW ?? 0)` collapses + // to a plain open), so stub the constant away and pin that the + // compensating check still refuses to read through the link (#8227). + const dir = makeTempDir(); + const secretPath = join(dir, 'secret.txt'); + const outputPath = join(dir, 'shell.output'); + writeFileSync(secretPath, 'secret credentials'); + symlinkSync(secretPath, outputPath); + + vi.resetModules(); + vi.doMock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + const modified = { + ...actual, + constants: { ...actual.constants, O_NOFOLLOW: undefined }, + }; + return { ...modified, default: modified }; + }); + + try { + const { BackgroundShellRegistry: RegistryWithoutNoFollow } = + await import('./backgroundShellRegistry.js'); + const reg = new RegistryWithoutNoFollow(); + const callback = vi.fn(); + reg.setNotificationCallback(callback); + reg.register(makeEntry({ shellId: 'a', outputPath })); + + reg.complete('a', 0, 2000); + + const [, modelText] = callback.mock.calls[0]; + expect(modelText).not.toContain('secret credentials'); + expect(modelText).toContain(' { // Guards the catch branch in `readOutputTail`. If the try/catch // ever regresses to throwing, `complete()` would propagate the diff --git a/packages/core/src/services/backgroundShellRegistry.ts b/packages/core/src/services/backgroundShellRegistry.ts index 68b0941dca9..7ef829adf2b 100644 --- a/packages/core/src/services/backgroundShellRegistry.ts +++ b/packages/core/src/services/backgroundShellRegistry.ts @@ -23,6 +23,7 @@ import * as fs from 'node:fs'; import type { TaskBase, TaskRegistration } from '../agents/tasks/types.js'; import { atomicWriteFileSync } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; +import { openSyncNoFollow } from '../utils/no-follow-open.js'; import { todoWorkChainContext } from '../utils/promptIdContext.js'; import { isBidiControlChar, @@ -61,7 +62,10 @@ type OutputTailResult = function readOutputTail(outputFile: string): OutputTailResult { let fd: number | undefined; try { - fd = fs.openSync(outputFile, getReadOutputOpenFlags()); + // O_NOFOLLOW (or the compensating identity check where the flag does + // not exist, e.g. Windows) refuses a symlink planted over the output + // file, so the tail can never be read through it (#8227). + fd = openSyncNoFollow(outputFile); const stat = fs.fstatSync(fd); if (!stat.isFile() || stat.size <= 0) return undefined; @@ -107,11 +111,6 @@ function readOutputTail(outputFile: string): OutputTailResult { } } -function getReadOutputOpenFlags(): number { - const constants = fs.constants; - return (constants?.O_RDONLY ?? 0) | (constants?.O_NOFOLLOW ?? 0); -} - function truncateCommandForModel(command: string): { text: string; truncated: boolean; diff --git a/packages/core/src/services/sessionService.rename.test.ts b/packages/core/src/services/sessionService.rename.test.ts index c4bc875016a..042c2879393 100644 --- a/packages/core/src/services/sessionService.rename.test.ts +++ b/packages/core/src/services/sessionService.rename.test.ts @@ -83,6 +83,32 @@ describe('SessionService - rename and custom title', () => { vi.spyOn(fs, 'openSync').mockReturnValue(42); readSyncSpy = vi.spyOn(fs, 'readSync').mockReturnValue(0); vi.spyOn(fs, 'closeSync').mockImplementation(() => undefined); + // Platforms without O_NOFOLLOW (Windows) open session files through an + // lstat -> open -> fstat identity check (openSyncNoFollow). Spy both + // stats so that fallback accepts the fabricated paths above: a regular + // (non-symlink) file whose identity trivially matches itself. On + // platforms with the flag the spies stay inert. + vi.spyOn(fs, 'lstatSync').mockImplementation( + () => + ({ + dev: 1, + ino: 1, + isSymbolicLink: () => false, + isFile: () => true, + }) as unknown as fs.Stats, + ); + vi.spyOn(fs, 'fstatSync').mockImplementation( + () => + ({ + dev: 1, + ino: 1, + // size 0 keeps readLatestTailIfGrown's grown-tail pass inert, + // matching the pre-rerouting behavior where it never ran. + size: 0, + isSymbolicLink: () => false, + isFile: () => true, + }) as unknown as fs.Stats, + ); vi.mocked(jsonl.read).mockResolvedValue([]); vi.mocked(jsonl.readLines).mockResolvedValue([]); diff --git a/packages/core/src/tools/readManyFiles.ts b/packages/core/src/tools/readManyFiles.ts index 7eee0895ad5..828503c7d23 100644 --- a/packages/core/src/tools/readManyFiles.ts +++ b/packages/core/src/tools/readManyFiles.ts @@ -23,6 +23,7 @@ import { } from '../utils/fileUtils.js'; import { hasVerifiableInode } from '../utils/file-identity.js'; import { getFolderStructure } from '../utils/getFolderStructure.js'; +import { openNoFollow } from '../utils/no-follow-open.js'; /** * Options for reading multiple files. @@ -295,10 +296,11 @@ async function readValidatedTextFileContent( signal: AbortSignal | undefined, displayPath: string, ): ReturnType { - const source = await fs.promises.open( - filePath, - (fs.constants.O_RDONLY ?? 0) | (fs.constants.O_NOFOLLOW ?? 0), - ); + // Where O_NOFOLLOW does not exist (Windows) the helper compensates with + // an lstat/open/fstat identity check instead of collapsing to a plain + // open that follows symlinks (#8227); the validated-identity re-check + // below remains the second layer. + const source = await openNoFollow(filePath); try { const stats = await source.stat(); if (!fileStatsMatchValidatedIdentity(stats, expected)) { @@ -374,10 +376,9 @@ async function snapshotValidatedFile( | undefined; try { signal?.throwIfAborted(); - const source = await fs.promises.open( - filePath, - (fs.constants.O_RDONLY ?? 0) | (fs.constants.O_NOFOLLOW ?? 0), - ); + // See readValidatedTextFileContent: the helper keeps the no-follow + // guarantee on platforms without O_NOFOLLOW (#8227). + const source = await openNoFollow(filePath); try { const stats = await source.stat(); if (!fileStatsMatchValidatedIdentity(stats, expected)) { diff --git a/packages/core/src/utils/gitDiff.test.ts b/packages/core/src/utils/gitDiff.test.ts index d7be102751a..d14df420310 100644 --- a/packages/core/src/utils/gitDiff.test.ts +++ b/packages/core/src/utils/gitDiff.test.ts @@ -9,7 +9,7 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { promisify } from 'node:util'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { fetchGitDiff, fetchGitDiffHunks, @@ -28,6 +28,7 @@ import { parseStatusEntries, resolveGitDir, } from './gitDiff.js'; +import { UNVERIFIABLE_IDENTITY_CODE } from './no-follow-open.js'; const execFileAsync = promisify(execFile); @@ -2290,3 +2291,110 @@ describe('fetchGitLog range argument injection guard', () => { expect(log!.entries).toHaveLength(2); }); }); + +// openUntrackedForDiffRead consumes openNoFollow's refusals, so this suite +// stubs the helper at the seam. The helper's own rejection semantics (inode +// 0 -> UNVERIFIABLE_IDENTITY_CODE, symlink/race -> ELOOP) are covered in +// no-follow-open.test.ts; here we pin gitDiff's response to each code. +const noFollowRefusal = vi.hoisted(() => ({ + code: undefined as string | undefined, + message: '', +})); + +vi.mock('./no-follow-open.js', async (importActual) => { + const actual = await importActual(); + return { + ...actual, + openNoFollow: (filePath: string) => { + if (noFollowRefusal.code !== undefined) { + return Promise.reject( + Object.assign(new Error(noFollowRefusal.message), { + code: noFollowRefusal.code, + }), + ); + } + return actual.openNoFollow(filePath); + }, + }; +}); + +describe('untracked files on inode-unverifiable volumes (#8227 follow-up)', () => { + let repo: string; + + beforeEach(async () => { + repo = await makeRepo(); + }); + + afterEach(async () => { + noFollowRefusal.code = undefined; + await fs.rm(repo, { recursive: true, force: true }); + }); + + it('falls back to a plain open when inode identity is unverifiable', async () => { + // On inode-0 volumes (FAT/exFAT, some SMB shares) openNoFollow refuses + // with UNVERIFIABLE_IDENTITY_CODE because identity can never be proven + // there. Diff display is not identity-sensitive, so untracked text + // files must keep their line counts instead of collapsing to a binary + // row (#8227 follow-up). + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await fs.writeFile(path.join(repo, 'fat-volume.txt'), 'a\nb\nc\n'); + + noFollowRefusal.code = UNVERIFIABLE_IDENTITY_CODE; + noFollowRefusal.message = 'inode 0 cannot be verified'; + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.perFileStats.get('fat-volume.txt')).toEqual({ + added: 3, + removed: 0, + isBinary: false, + isUntracked: true, + truncated: false, + }); + // The fallback keeps the lines in the aggregate total too. + expect(result!.stats.linesAdded).toBe(3); + }); + + it('still synthesizes an all-added hunk when inode identity is unverifiable', async () => { + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await fs.writeFile(path.join(repo, 'new.txt'), 'x\ny\n'); + + noFollowRefusal.code = UNVERIFIABLE_IDENTITY_CODE; + noFollowRefusal.message = 'inode 0 cannot be verified'; + + const result = await fetchGitDiffHunksForFile(repo, 'new.txt'); + expect(result).not.toBeNull(); + expect(result!.truncated).toBe(false); + expect(result!.hunks).toHaveLength(1); + expect(result!.hunks[0].lines).toEqual(['+x', '+y']); + }); + + it('never falls back to a plain open on a symlink refusal', async () => { + // Any refusal OTHER than the inode-unverifiable one (a genuine symlink + // race, ELOOP) must NOT degrade to a plain open — that would follow + // the symlink the guard just refused. The file collapses to a binary + // row instead. + await fs.writeFile(path.join(repo, 'seed.txt'), 'x\n'); + await git(repo, 'add', '.'); + await git(repo, 'commit', '-q', '-m', 'init'); + await fs.writeFile(path.join(repo, 'raced.txt'), 'a\nb\n'); + + noFollowRefusal.code = 'ELOOP'; + noFollowRefusal.message = 'too many symbolic links'; + + const result = await fetchGitDiff(repo); + expect(result).not.toBeNull(); + expect(result!.perFileStats.get('raced.txt')).toEqual({ + added: 0, + removed: 0, + isBinary: true, + isUntracked: true, + truncated: false, + }); + expect(result!.stats.linesAdded).toBe(0); + }); +}); diff --git a/packages/core/src/utils/gitDiff.ts b/packages/core/src/utils/gitDiff.ts index 9b76a8a7f4c..6c30169b529 100644 --- a/packages/core/src/utils/gitDiff.ts +++ b/packages/core/src/utils/gitDiff.ts @@ -5,19 +5,13 @@ */ import { execFile } from 'node:child_process'; -// Namespace import (vs `import { constants }`) so vitest tests that -// `vi.mock('node:fs', ...)` without supplying every named export don't -// blow up in strict-mock mode just because they transitively load this -// file via `@qwen-code/qwen-code-core`. The `constants?.X ?? 0` accesses -// below absorb a missing `constants` field by falling through to plain -// `O_RDONLY` (= 0 on POSIX) — harmless in mock environments where no -// real `open()` ever runs. -import * as nodeFs from 'node:fs'; import { access, lstat, open, readFile, stat } from 'node:fs/promises'; +import type { FileHandle } from 'node:fs/promises'; import * as path from 'node:path'; import { promisify } from 'node:util'; import type { Hunk } from 'diff'; import { findGitRoot, readFirstLineNoFollow } from './gitUtils.js'; +import { isUnverifiableIdentityError, openNoFollow } from './no-follow-open.js'; /** Re-export so consumers don't need to depend on `diff` directly. */ export type GitDiffHunk = Hunk; @@ -90,24 +84,32 @@ const UNTRACKED_READ_CAP_BYTES = MAX_DIFF_SIZE_BYTES; const UNTRACKED_READ_CHUNK_BYTES = 64 * 1024; /** Scan the first N bytes for NUL to detect binary files (matches git's heuristic). */ const BINARY_SNIFF_BYTES = 8 * 1024; -/** Memoized open flags for line counting. `O_NOFOLLOW` closes the TOCTOU - * window between the `lstat` symlink check and `open` — if the path is - * replaced with a symlink in that gap, `open` rejects with `ELOOP` instead - * of silently dereferencing it. Falls back to plain `O_RDONLY` on platforms - * that don't expose the flag (Windows constants omit `O_NOFOLLOW`). - * - * Computed lazily on first call (rather than at module load) so test files - * that `vi.mock('node:fs', ...)` without supplying `constants` can still - * load this module transitively via `@qwen-code/qwen-code-core` without - * vitest's strict-mock proxy throwing on the property access. Tests that - * do not actually exercise `countUntrackedLines` never trigger the lookup. */ -let untrackedOpenFlagsCache: number | undefined; -function getUntrackedOpenFlags(): number { - if (untrackedOpenFlagsCache === undefined) { - untrackedOpenFlagsCache = - (nodeFs.constants?.O_RDONLY ?? 0) | (nodeFs.constants?.O_NOFOLLOW ?? 0); + +/** + * Open an untracked file for diff display through {@link openNoFollow}, + * degrading to a plain read only where the helper's fail-closed refusal is + * the inode-unverifiable one (ino 0: FAT/exFAT, some SMB shares on Windows). + * Every call site gates on `lstat(...).isFile()` immediately before the + * open, so the fallback cannot follow a symlink the gate did not already + * accept — it merely restores the pre-#8227 read for volumes where identity + * can never be proven. Without the degradation, EVERY untracked text file on + * such a volume would collapse to a binary row / dropped hunk even though + * diff display is not identity-sensitive. Any other refusal (a genuine + * symlink race) and any plain-open error return `undefined`. + */ +async function openUntrackedForDiffRead( + absPath: string, +): Promise { + try { + return await openNoFollow(absPath); + } catch (error) { + if (!isUnverifiableIdentityError(error)) return undefined; + } + try { + return await open(absPath); + } catch { + return undefined; } - return untrackedOpenFlagsCache; } /** @@ -447,12 +449,11 @@ async function synthesizeUntrackedHunk( } catch { return null; } - let fh; - try { - fh = await open(absPath, getUntrackedOpenFlags()); - } catch { - return null; - } + // O_NOFOLLOW closes the TOCTOU window between the lstat above and the + // open — where the flag does not exist (Windows) the helper compensates + // with an identity re-check (#8227). + const fh = await openUntrackedForDiffRead(absPath); + if (!fh) return null; try { const st = await fh.stat(); if (!st.isFile()) return null; @@ -966,10 +967,11 @@ async function countUntrackedLines( if (!st.isFile()) { return { added: 0, isBinary: true, truncated: false }; } - let fh; - try { - fh = await open(absPath, getUntrackedOpenFlags()); - } catch { + // O_NOFOLLOW closes the TOCTOU window between the lstat above and the + // open — where the flag does not exist (Windows) the helper compensates + // with an identity re-check (#8227). + const fh = await openUntrackedForDiffRead(absPath); + if (!fh) { // ELOOP from O_NOFOLLOW (path raced into a symlink between lstat and // open) and any other open error all collapse to a binary row so the // file appears once in the listing without contributing line counts. diff --git a/packages/core/src/utils/no-follow-open.test.ts b/packages/core/src/utils/no-follow-open.test.ts new file mode 100644 index 00000000000..231130804e7 --- /dev/null +++ b/packages/core/src/utils/no-follow-open.test.ts @@ -0,0 +1,493 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + closeSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import type { Stats } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + isUnverifiableIdentityError, + openNoFollow, + openSyncNoFollow, + UNVERIFIABLE_IDENTITY_CODE, +} from './no-follow-open.js'; + +let tmpDirs: string[] = []; + +function makeTempDir(): string { + const dir = mkdtempSync(join(tmpdir(), 'no-follow-open-')); + tmpDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of tmpDirs) { + rmSync(dir, { recursive: true, force: true }); + } + tmpDirs = []; + vi.restoreAllMocks(); +}); + +// Symlink creation needs developer mode on Windows; skip there like the +// other symlink planting tests in this repo. +const itNoSymlink = process.platform === 'win32' ? it.skip : it; + +// Copy a Stats object with identity fields patched, keeping the prototype +// so isSymbolicLink()/isFile() keep working on the perturbed result. +function perturbedStats( + stats: Stats, + patch: Partial>, +): Stats { + return Object.assign( + Object.create(Object.getPrototypeOf(stats)), + stats, + patch, + ); +} + +// Install a node:fs mock with O_NOFOLLOW removed so the module under test +// takes the lstat/open/fstat fallback path. The `default` member is +// LOAD-BEARING: no-follow-open.ts binds node:fs through a DEFAULT import, +// so a mock without it hands the helper the real O_NOFOLLOW and the +// fallback tests would silently pass on the native branch. +function mockNoFollowFs( + build: ( + actual: typeof import('node:fs'), + ) => Record = () => ({}), +): void { + vi.resetModules(); + vi.doMock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + const modified = { + ...actual, + ...build(actual), + constants: { ...actual.constants, O_NOFOLLOW: undefined }, + }; + return { ...modified, default: modified }; + }); +} + +describe('openNoFollow (native O_NOFOLLOW available)', () => { + it('opens a regular file for reading', async () => { + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + const handle = await openNoFollow(filePath); + try { + const buffer = Buffer.alloc(7); + await handle.read(buffer, 0, 7, 0); + expect(buffer.toString('utf8')).toBe('payload'); + } finally { + await handle.close(); + } + }); + + it('opens a regular file synchronously', () => { + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'sync-payload'); + + const fd = openSyncNoFollow(filePath); + try { + // readFileSync(fd) reads from offset 0 without closing the fd, so it + // proves the fd is a live read descriptor for the right file. + expect(readFileSync(fd, 'utf8')).toBe('sync-payload'); + } finally { + closeSync(fd); + } + }); + + itNoSymlink('refuses a symlinked path (async)', async () => { + const dir = makeTempDir(); + const targetPath = join(dir, 'target.txt'); + const linkPath = join(dir, 'link.txt'); + writeFileSync(targetPath, 'secret'); + symlinkSync(targetPath, linkPath); + + const error = await openNoFollow(linkPath).catch((e) => e); + expect(error).toBeInstanceOf(Error); + expect((error as NodeJS.ErrnoException).code).toBe('ELOOP'); + }); + + itNoSymlink('refuses a symlinked path (sync)', () => { + const dir = makeTempDir(); + const targetPath = join(dir, 'target.txt'); + const linkPath = join(dir, 'link.txt'); + writeFileSync(targetPath, 'secret'); + symlinkSync(targetPath, linkPath); + + expect(() => openSyncNoFollow(linkPath)).toThrow( + expect.objectContaining({ code: 'ELOOP' }), + ); + }); + + it('propagates ENOENT for missing paths', async () => { + const dir = makeTempDir(); + const error = await openNoFollow(join(dir, 'missing.txt')).catch((e) => e); + expect((error as NodeJS.ErrnoException).code).toBe('ENOENT'); + expect(() => openSyncNoFollow(join(dir, 'missing.txt'))).toThrow( + expect.objectContaining({ code: 'ENOENT' }), + ); + }); +}); + +describe('openNoFollow without O_NOFOLLOW (Windows flag set)', () => { + async function importWithoutNoFollow() { + mockNoFollowFs(); + const mockedFs = await import('node:fs'); + const { openNoFollow: openFallback, openSyncNoFollow: openSyncFallback } = + await import('./no-follow-open.js'); + return { mockedFs, openFallback, openSyncFallback }; + } + + afterEach(() => { + vi.doUnmock('node:fs'); + vi.resetModules(); + }); + + it('opens a regular file through the lstat/open/fstat fallback', async () => { + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'fallback-payload'); + + const { openFallback } = await importWithoutNoFollow(); + const handle = await openFallback(filePath); + try { + const buffer = Buffer.alloc(16); + const { bytesRead } = await handle.read(buffer, 0, 16, 0); + expect(buffer.toString('utf8', 0, bytesRead)).toBe('fallback-payload'); + } finally { + await handle.close(); + } + }); + + itNoSymlink('refuses a symlinked path via the pre-open lstat', async () => { + const dir = makeTempDir(); + const targetPath = join(dir, 'target.txt'); + const linkPath = join(dir, 'link.txt'); + writeFileSync(targetPath, 'secret'); + symlinkSync(targetPath, linkPath); + + const { openFallback, openSyncFallback } = await importWithoutNoFollow(); + const error = await openFallback(linkPath).catch((e) => e); + expect((error as NodeJS.ErrnoException).code).toBe('ELOOP'); + expect(() => openSyncFallback(linkPath)).toThrow( + expect.objectContaining({ code: 'ELOOP' }), + ); + }); + + it('refuses when the file identity changes between lstat and open', async () => { + // Simulates the TOCTOU race the fallback exists for: the path passes + // the lstat check, then gets swapped before the identity re-check on + // the opened fd. A real race is impractical to schedule in a unit + // test, so the re-check is fed a mismatched identity directly through + // the fs mock (the async FileHandle.stat() path bypasses fs.fstatSync + // and cannot be intercepted this way; the identity predicate is shared + // between the two variants). + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + const closeSpy = vi.fn(); + mockNoFollowFs((actual) => ({ + fstatSync: ((fd: number) => { + const stats = actual.fstatSync(fd); + return perturbedStats(stats, { ino: stats.ino + 1 }); + }) as typeof actual.fstatSync, + // Pin the rejection-path fd close: without it every sync fallback + // refusal leaks the raw fd it opened for the identity re-check. + closeSync: ((fd: number) => { + closeSpy(); + return actual.closeSync(fd); + }) as typeof actual.closeSync, + })); + + const { openSyncNoFollow: openSyncFallback } = await import( + './no-follow-open.js' + ); + expect(() => openSyncFallback(filePath)).toThrow( + expect.objectContaining({ code: 'ELOOP' }), + ); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('refuses when the device identity changes between lstat and open', async () => { + // dev half of the dev/ino identity re-check: inode numbers are unique + // only per-device, so a path swapped to a DIFFERENT device carrying a + // colliding inode (attacker-controlled second mount, bind mount) must + // still be refused. Mirrors the ino-mismatch variant with dev + 1. + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + mockNoFollowFs((actual) => ({ + fstatSync: ((fd: number) => { + const stats = actual.fstatSync(fd); + return perturbedStats(stats, { dev: stats.dev + 1 }); + }) as typeof actual.fstatSync, + })); + + const { openSyncNoFollow: openSyncFallback } = await import( + './no-follow-open.js' + ); + expect(() => openSyncFallback(filePath)).toThrow( + expect.objectContaining({ code: 'ELOOP' }), + ); + }); + + it('refuses when the file identity changes between lstat and open (async)', async () => { + // Async counterpart of the sync identity-change test. The real opened + // FileHandle's stat() cannot be intercepted through fs mocks, so the + // pre-open lstat is doctored instead (same prototype trick, ino + 1) + // and the identity re-check on the opened handle then mismatches it. + // This pins the async try/assertSameIdentity/catch-and-close block in + // openNoFollow: the symlink refusal tests all reject at the earlier + // isSymbolicLink() check, so deleting that block keeps them green + // while silently leaking the rejection-path handle unclosed. + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + let closeSpy: ReturnType | undefined; + + mockNoFollowFs((actual) => ({ + promises: { + ...actual.promises, + lstat: (async (p: string) => { + const stats = await actual.promises.lstat(p); + return perturbedStats(stats, { ino: stats.ino + 1 }); + }) as typeof actual.promises.lstat, + open: (async (...args: Parameters) => { + const handle = await actual.promises.open(...args); + closeSpy = vi.spyOn(handle, 'close'); + return handle; + }) as typeof actual.promises.open, + }, + })); + + const { openNoFollow: openFallback } = await import('./no-follow-open.js'); + const error = await openFallback(filePath).catch((e) => e); + expect((error as NodeJS.ErrnoException).code).toBe('ELOOP'); + expect(closeSpy).toBeDefined(); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + // The identity re-check must compare the opened fd against the PRE-OPEN + // lstat snapshot. These tests perturb every lstat AFTER the first call, + // so an implementation re-basing the comparison on a fresh post-open + // lstat would see the perturbed stats, mismatch the fd, and throw ELOOP; + // the correct single-lstat implementation opens and reads the payload. + function mockNoFollowFsWithPerturbedSnapshot(): void { + let lstatCalls = 0; + mockNoFollowFs((actual) => { + const snapshotStats = (stats: Stats): Stats => { + lstatCalls += 1; + return lstatCalls === 1 + ? stats + : perturbedStats(stats, { ino: stats.ino + 1 }); + }; + return { + lstatSync: ((p: string) => + snapshotStats(actual.lstatSync(p))) as typeof actual.lstatSync, + promises: { + ...actual.promises, + lstat: (async (p: string) => + snapshotStats( + await actual.promises.lstat(p), + )) as typeof actual.promises.lstat, + }, + }; + }); + } + + it('compares the opened fd against the PRE-OPEN lstat snapshot (sync)', async () => { + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'snapshot-payload'); + + mockNoFollowFsWithPerturbedSnapshot(); + + const { openSyncNoFollow: openSyncFallback } = await import( + './no-follow-open.js' + ); + const fd = openSyncFallback(filePath); + try { + expect(readFileSync(fd, 'utf8')).toBe('snapshot-payload'); + } finally { + closeSync(fd); + } + }); + + it('compares the opened handle against the PRE-OPEN lstat snapshot (async)', async () => { + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'snapshot-payload'); + + mockNoFollowFsWithPerturbedSnapshot(); + + const { openNoFollow: openFallback } = await import('./no-follow-open.js'); + const handle = await openFallback(filePath); + try { + const buffer = Buffer.alloc(16); + const { bytesRead } = await handle.read(buffer, 0, 16, 0); + expect(buffer.toString('utf8', 0, bytesRead)).toBe('snapshot-payload'); + } finally { + await handle.close(); + } + }); + + it('refuses when the filesystem cannot prove identity (inode 0)', async () => { + // FAT/exFAT/SMB volumes report ino 0 for every file; the comparison + // would be vacuous there, so the helper fails closed (#8290 posture). + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + const closeSpy = vi.fn(); + mockNoFollowFs((actual) => ({ + lstatSync: ((p: string) => + perturbedStats(actual.lstatSync(p), { + ino: 0, + })) as typeof actual.lstatSync, + // Same rejection-path fd close pin as the identity-change test. + closeSync: ((fd: number) => { + closeSpy(); + return actual.closeSync(fd); + }) as typeof actual.closeSync, + })); + + const { openSyncNoFollow: openSyncFallback } = await import( + './no-follow-open.js' + ); + // Distinct from a genuine symlink refusal: the code must NOT be + // 'ELOOP', or consumers' ELOOP-specific handling (symlink-escape + // flags, "not a regular file" errors, binary-row collapses) misfires + // on legitimate files that merely live on an inode-0 volume. + const error = (() => { + try { + openSyncFallback(filePath); + return undefined; + } catch (e) { + return e as NodeJS.ErrnoException; + } + })(); + expect(error).toBeDefined(); + expect(error?.code).toBe(UNVERIFIABLE_IDENTITY_CODE); + expect(error?.code).not.toBe('ELOOP'); + expect(isUnverifiableIdentityError(error)).toBe(true); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('still rejects with ELOOP when the rejection-path close fails (sync)', async () => { + // The identity-mismatch close is best-effort: if closeSync itself + // throws, the pinned ELOOP refusal must still surface, not the + // close error. Deleting the swallow around fs.closeSync in + // openSyncNoFollow makes this test fail with the close error. + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + mockNoFollowFs((actual) => ({ + fstatSync: ((fd: number) => { + const stats = actual.fstatSync(fd); + return perturbedStats(stats, { ino: stats.ino + 1 }); + }) as typeof actual.fstatSync, + closeSync: (() => { + throw Object.assign(new Error('close failed'), { code: 'EBADF' }); + }) as typeof actual.closeSync, + })); + + const { openSyncNoFollow: openSyncFallback } = await import( + './no-follow-open.js' + ); + expect(() => openSyncFallback(filePath)).toThrow( + expect.objectContaining({ code: 'ELOOP' }), + ); + }); + + it('still rejects with EUNVERIFIABLE when the rejection-path close fails (inode 0)', async () => { + // Same best-effort-close pin for the inode-0 refusal: a throwing + // closeSync must not mask the UNVERIFIABLE_IDENTITY_CODE error. + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + mockNoFollowFs((actual) => ({ + lstatSync: ((p: string) => + perturbedStats(actual.lstatSync(p), { + ino: 0, + })) as typeof actual.lstatSync, + closeSync: (() => { + throw Object.assign(new Error('close failed'), { code: 'EBADF' }); + }) as typeof actual.closeSync, + })); + + const { openSyncNoFollow: openSyncFallback } = await import( + './no-follow-open.js' + ); + const error = (() => { + try { + openSyncFallback(filePath); + return undefined; + } catch (e) { + return e as NodeJS.ErrnoException; + } + })(); + expect(error).toBeDefined(); + expect(error?.code).toBe(UNVERIFIABLE_IDENTITY_CODE); + }); + + it('still rejects with ELOOP when the rejection-path close fails (async)', async () => { + // Async best-effort-close pin: a rejecting handle.close() must not + // mask the pinned ELOOP refusal. Deleting the .catch(() => {}) on + // handle.close() in openNoFollow makes this test fail with the + // close rejection instead. + const dir = makeTempDir(); + const filePath = join(dir, 'data.txt'); + writeFileSync(filePath, 'payload'); + + mockNoFollowFs((actual) => ({ + promises: { + ...actual.promises, + lstat: (async (p: string) => { + const stats = await actual.promises.lstat(p); + return perturbedStats(stats, { ino: stats.ino + 1 }); + }) as typeof actual.promises.lstat, + open: (async (...args: Parameters) => { + const handle = await actual.promises.open(...args); + vi.spyOn(handle, 'close').mockRejectedValue( + Object.assign(new Error('close failed'), { code: 'EIO' }), + ); + return handle; + }) as typeof actual.promises.open, + }, + })); + + const { openNoFollow: openFallback } = await import('./no-follow-open.js'); + const error = await openFallback(filePath).catch((e) => e); + expect((error as NodeJS.ErrnoException).code).toBe('ELOOP'); + }); + + it('propagates ENOENT for missing paths', async () => { + const dir = makeTempDir(); + const { openFallback, openSyncFallback } = await importWithoutNoFollow(); + const error = await openFallback(join(dir, 'missing.txt')).catch((e) => e); + expect((error as NodeJS.ErrnoException).code).toBe('ENOENT'); + expect(() => openSyncFallback(join(dir, 'missing.txt'))).toThrow( + expect.objectContaining({ code: 'ENOENT' }), + ); + }); +}); diff --git a/packages/core/src/utils/no-follow-open.ts b/packages/core/src/utils/no-follow-open.ts new file mode 100644 index 00000000000..2dcef8ec089 --- /dev/null +++ b/packages/core/src/utils/no-follow-open.ts @@ -0,0 +1,197 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Cross-platform "open without following symlinks". + * + * POSIX provides `O_NOFOLLOW`: the kernel refuses to open a path whose final + * component is a symlink (`ELOOP`). Windows has no such flag — + * `fs.constants.O_NOFOLLOW` is `undefined` there — so flag expressions like + * `(flags | (O_NOFOLLOW ?? 0))` silently collapse into a plain open that + * follows symlinks (issue #8227). Callers must therefore never OR the flag + * in themselves; they should open through this helper instead. + * + * When `O_NOFOLLOW` is unavailable, the helper compensates with an + * lstat → open → fstat identity check: + * + * 1. `lstat` the path and refuse it when the final component is a symlink. + * 2. Open the path. + * 3. `fstat` the opened handle and require dev/ino to still match the + * `lstat` from step 1. If the path was replaced between the two calls + * (for example swapped for a symlink), the identities differ and the + * open is rejected after closing the handle again. + * + * Filesystems that do not expose inode numbers (`ino === 0`: FAT/exFAT, + * some SMB shares) make step 3 vacuous, so the helper refuses the open + * there rather than degrade to a plain open — the same fail-closed posture + * used for unverifiable inode identities elsewhere (#8290, #9857). + * + * Symlink refusals and identity races are reported as errors with + * `code: 'ELOOP'` — the same code POSIX `O_NOFOLLOW` produces — so + * existing `ELOOP` handling in callers applies to the fallback path + * unchanged. The inode-0 refusal, where identity was never provable in the + * first place, carries {@link UNVERIFIABLE_IDENTITY_CODE} instead, so a + * legitimate file on an inode-0 volume is not misclassified by + * ELOOP-specific handling as a symlink escape. + * + * `node:fs` is bound through the DEFAULT import (not a namespace import) + * so suites that spy the fs object — the way `sessionService.rename.test.ts` + * spies `openSync`/`readSync` for its fabricated session paths — intercept + * this helper's calls too: vitest hands namespace imports their own copy of + * an externalized CJS module, which escapes those spies (#8227). + */ + +import fs from 'node:fs'; +import type { FileHandle } from 'node:fs/promises'; + +import { hasVerifiableInode } from './file-identity.js'; + +/** + * Error code carried by the refusal raised when the fallback cannot even + * attempt the identity proof — the filesystem reports inode 0 (FAT/exFAT, + * some SMB shares), so the opened file cannot be proven identical to the + * one the pre-open check saw. + * + * The refusal itself is the documented fail-closed posture (#8290, #9857). + * What must stay distinguishable is the reason: callers with ELOOP-specific + * handling (symlink-escape flags, "not a regular file" errors, binary-row + * collapses) would otherwise misfire on LEGITIMATE files that merely live + * on an inode-0 volume. Genuine symlink refusals and identity races keep + * `code: 'ELOOP'`. + */ +export const UNVERIFIABLE_IDENTITY_CODE = 'EUNVERIFIABLE'; + +/** + * True iff `error` is the inode-unverifiable refusal described by + * {@link UNVERIFIABLE_IDENTITY_CODE}. + */ +export function isUnverifiableIdentityError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + (error as NodeJS.ErrnoException).code === UNVERIFIABLE_IDENTITY_CODE + ); +} + +function noFollowRejection( + filePath: string, + reason: string, + code: string = 'ELOOP', +): NodeJS.ErrnoException { + const error = new Error( + `Refusing to open '${filePath}' without a no-follow guarantee: ${reason}`, + ) as NodeJS.ErrnoException; + error.code = code; + return error; +} + +/** + * Verify that the handle opened in step (2) still refers to the file seen by + * the pre-open `lstat` (step 1). Throws an `ELOOP`-coded error when the + * identity changed, and an {@link UNVERIFIABLE_IDENTITY_CODE}-coded error + * when it was never provable (inode 0). + */ +function assertSameIdentity( + filePath: string, + before: fs.Stats, + after: fs.Stats, +): void { + if (!hasVerifiableInode(before.ino)) { + throw noFollowRejection( + filePath, + 'the filesystem reports inode 0, so the opened file cannot be ' + + 'proven identical to the one that was checked', + UNVERIFIABLE_IDENTITY_CODE, + ); + } + if (before.dev !== after.dev || before.ino !== after.ino) { + throw noFollowRejection( + filePath, + 'the file identity changed between the pre-open check and the open ' + + '(possible symlink race)', + ); + } +} + +/** + * The platform's O_NOFOLLOW flag, or `undefined` when the runtime does not + * expose it (Windows) — the caller then takes the compensating path. + */ +function getNoFollowFlag(): number | undefined { + return fs.constants?.O_NOFOLLOW; +} + +/** + * Synchronous variant of {@link openNoFollow}. Returns a raw fd; the caller + * owns closing it. + */ +export function openSyncNoFollow(filePath: string): number { + // Optional chain so strict vitest mocks of node:fs that omit `constants` + // degrade to plain O_RDONLY (= 0) instead of throwing at call time. + const baseFlags = fs.constants?.O_RDONLY ?? 0; + const noFollowFlag = getNoFollowFlag(); + if (typeof noFollowFlag === 'number') { + return fs.openSync(filePath, baseFlags | noFollowFlag); + } + + const before = fs.lstatSync(filePath); + if (before.isSymbolicLink()) { + throw noFollowRejection(filePath, 'the path is a symlink'); + } + const fd = fs.openSync(filePath, baseFlags); + try { + assertSameIdentity(filePath, before, fs.fstatSync(fd)); + } catch (error) { + try { + fs.closeSync(fd); + } catch { + // The rejection below is the primary error; closing is best-effort. + } + throw error; + } + return fd; +} + +/** + * Open `filePath` for reading without following a symlink in the final path + * component. + * + * On platforms with `O_NOFOLLOW` the guarantee is enforced by the kernel at + * open time. Elsewhere an lstat → open → fstat identity check compensates + * (see the module docs). Symlink refusals and identity races carry + * `code: 'ELOOP'`; an unverifiable identity carries + * {@link UNVERIFIABLE_IDENTITY_CODE}. The caller owns closing the returned + * handle. + * + * Deliberately read-only with no `flags`/`mode` parameters: no caller needs + * them, and an `fs.open`-shaped signature would invite write/create flags + * through a helper whose semantics and tests cover only the read-only case. + * Re-add them (with a caller and tests) in the PR that first needs them. + */ +export async function openNoFollow(filePath: string): Promise { + // Optional chain so strict vitest mocks of node:fs that omit `constants` + // degrade to plain O_RDONLY (= 0) instead of throwing at call time. + const baseFlags = fs.constants?.O_RDONLY ?? 0; + const noFollowFlag = getNoFollowFlag(); + if (typeof noFollowFlag === 'number') { + return fs.promises.open(filePath, baseFlags | noFollowFlag); + } + + const before = await fs.promises.lstat(filePath); + if (before.isSymbolicLink()) { + throw noFollowRejection(filePath, 'the path is a symlink'); + } + const handle = await fs.promises.open(filePath, baseFlags); + try { + assertSameIdentity(filePath, before, await handle.stat()); + } catch (error) { + await handle.close().catch(() => { + // The rejection below is the primary error; closing is best-effort. + }); + throw error; + } + return handle; +} diff --git a/packages/core/src/utils/sessionStorageUtils.test.ts b/packages/core/src/utils/sessionStorageUtils.test.ts index 5110368fdf6..765a6b52cc1 100644 --- a/packages/core/src/utils/sessionStorageUtils.test.ts +++ b/packages/core/src/utils/sessionStorageUtils.test.ts @@ -774,3 +774,101 @@ describe('sessionStorageUtils', () => { }); }); }); + +describe('sessionStorageUtils when O_NOFOLLOW is unavailable (Windows flag set)', () => { + // Windows has no O_NOFOLLOW; the constant is `undefined` there and flag + // expressions like `(O_RDONLY | (O_NOFOLLOW ?? 0))` silently collapse to a + // plain open that follows symlinks (#8227). Stub the constant away to run + // that exact path on Linux CI and pin the compensating refusal. + const itNoSymlink = process.platform === 'win32' ? it.skip : it; + + itNoSymlink( + 'does not read session metadata through a symlinked session file', + async () => { + const dir = fs.mkdtempSync( + path.join(os.tmpdir(), 'session-storage-nofollow-'), + ); + vi.resetModules(); + vi.doMock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + // sessionStorageUtils uses a DEFAULT import of node:fs, so the + // `default` property must carry the stubbed constants too. + const modified = { + ...actual, + constants: { ...actual.constants, O_NOFOLLOW: undefined }, + }; + return { ...modified, default: modified }; + }); + + try { + const secretPath = path.join(dir, 'secret.jsonl'); + const sessionPath = path.join(dir, 'session.jsonl'); + fs.writeFileSync( + secretPath, + '{"subtype":"custom_title","customTitle":"leaked-secret"}\n', + ); + fs.symlinkSync(secretPath, sessionPath); + + const { readLastJsonStringFieldSync: readFieldUnmocked } = await import( + './sessionStorageUtils.js' + ); + expect( + readFieldUnmocked(sessionPath, 'customTitle', 'custom_title'), + ).toBeUndefined(); + } finally { + vi.doUnmock('node:fs'); + vi.resetModules(); + fs.rmSync(dir, { recursive: true, force: true }); + } + }, + ); + + itNoSymlink( + 'does not read session metadata through a symlinked session file (multi-field)', + async () => { + // Mirror of the single-field refusal test for the plural variant, + // rerouted through the same helper in the same pass: a symlink + // planted over the session file must not leak customTitle / + // titleSource through readLastJsonStringFieldsSync either. + const dir = fs.mkdtempSync( + path.join(os.tmpdir(), 'session-storage-nofollow-fields-'), + ); + vi.resetModules(); + vi.doMock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + // sessionStorageUtils uses a DEFAULT import of node:fs, so the + // `default` property must carry the stubbed constants too. + const modified = { + ...actual, + constants: { ...actual.constants, O_NOFOLLOW: undefined }, + }; + return { ...modified, default: modified }; + }); + + try { + const secretPath = path.join(dir, 'secret.jsonl'); + const sessionPath = path.join(dir, 'session.jsonl'); + fs.writeFileSync( + secretPath, + '{"subtype":"custom_title","customTitle":"leaked-secret","titleSource":"auto"}\n', + ); + fs.symlinkSync(secretPath, sessionPath); + + const { readLastJsonStringFieldsSync: readFieldsUnmocked } = + await import('./sessionStorageUtils.js'); + expect( + readFieldsUnmocked( + sessionPath, + 'customTitle', + ['titleSource'], + 'custom_title', + ), + ).toEqual({ customTitle: undefined, titleSource: undefined }); + } finally { + vi.doUnmock('node:fs'); + vi.resetModules(); + fs.rmSync(dir, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/packages/core/src/utils/sessionStorageUtils.ts b/packages/core/src/utils/sessionStorageUtils.ts index 58dacc99e35..de22ea77e73 100644 --- a/packages/core/src/utils/sessionStorageUtils.ts +++ b/packages/core/src/utils/sessionStorageUtils.ts @@ -13,26 +13,11 @@ import fs from 'node:fs'; +import { openSyncNoFollow } from './no-follow-open.js'; + /** Size of the head/tail buffer for lite metadata reads (64KB). */ export const LITE_READ_BUF_SIZE = 64 * 1024; -/** - * Flags used when opening session files for metadata reads. `O_NOFOLLOW` - * refuses to follow symlinks — defense in depth so a symlink planted in - * `~/.qwen/tmp//chats/` (by another local user or an extension with - * filesystem access) can't redirect a metadata read to an unrelated file. - * Falls back to plain read-only when the flag isn't available (e.g. Windows - * doesn't expose O_NOFOLLOW; the constant is `undefined` there). - * - * Computed lazily so tests that stub out `fs` don't blow up at module-init - * time trying to read `fs.constants.O_RDONLY`. - */ -function getReadOpenFlags(): number { - const constants = fs.constants; - if (!constants) return 0; - return (constants.O_RDONLY ?? 0) | (constants.O_NOFOLLOW ?? 0); -} - function readLatestTailIfGrown( fd: number, previousSize: number, @@ -315,7 +300,11 @@ export function readLastJsonStringFieldSync( const fileSize = stats.size; if (fileSize === 0) return undefined; - fd = fs.openSync(filePath, getReadOpenFlags()); + // O_NOFOLLOW (or the compensating identity check where the flag does + // not exist, e.g. Windows) refuses a symlink planted over the session + // file — defense in depth so a planted link can't redirect a metadata + // read to an unrelated file (#8227). + fd = openSyncNoFollow(filePath); // Phase 1: tail window — fast path. This is where every well-behaved // session keeps its current title (ChatRecordingService re-anchors @@ -418,7 +407,11 @@ export function readLastJsonStringFieldsSync( const fileSize = stats.size; if (fileSize === 0) return emptyResult; - fd = fs.openSync(filePath, getReadOpenFlags()); + // O_NOFOLLOW (or the compensating identity check where the flag does + // not exist, e.g. Windows) refuses a symlink planted over the session + // file — defense in depth so a planted link can't redirect a metadata + // read to an unrelated file (#8227). + fd = openSyncNoFollow(filePath); // Phase 1: tail window fast path. See the single-field variant for // the head-or-tail invariant and buffer-pool semantics. diff --git a/scripts/tests/serve-fast-path-bundle-check.test.js b/scripts/tests/serve-fast-path-bundle-check.test.js index be53dcff8e7..a4bc0d6ddc9 100644 --- a/scripts/tests/serve-fast-path-bundle-check.test.js +++ b/scripts/tests/serve-fast-path-bundle-check.test.js @@ -255,6 +255,20 @@ describe('serve fast-path bundle check', () => { ]); }); + it('keeps leaf core imports from pulling the core barrel into serve', () => { + const metafile = makeMetafile({ + 'dist/chunks/run-qwen-serve.js': output({ + inputs: ['packages/cli/src/serve/run-qwen-serve.ts'], + imports: [staticImport('dist/chunks/no-follow-open.js')], + }), + 'dist/chunks/no-follow-open.js': output({ + inputs: ['packages/core/src/utils/no-follow-open.ts'], + }), + }); + + expect(findServeFastPathBundleOffenders(metafile)).toEqual([]); + }); + it('matches normalized source suffixes without accepting partial names', () => { const metafile = makeMetafile({ 'dist\\chunks\\run-qwen-serve.js': output({