diff --git a/docs/reference/terminal-artifact-grant-integrity.md b/docs/reference/terminal-artifact-grant-integrity.md new file mode 100644 index 00000000000..ea04d35deef --- /dev/null +++ b/docs/reference/terminal-artifact-grant-integrity.md @@ -0,0 +1,85 @@ +# Terminal artifact grant integrity + +Clicking a path in terminal output mints a short-lived grant over one file in a +world-writable temp directory. Between minting the grant and using it, anything on the +host can replace that file. This page is the contract for the checks that catch such a +replacement — and, more importantly, for the windows they do **not** close. + +Read this before weakening a check in +`src/main/runtime/runtime-file-commands-terminal-artifact-access.ts`, before assuming +the sequence is atomic, or before extending the guarantee to remote hosts. + +## The stat identity is weaker than it looks + +A grant pins the file as `dev:ino:nlink:size:mtimeMs`. Four of those five fields +survive an unlink-and-recreate at the same path, measured on Linux (Node 24) by +replaying that exact sequence: + +| Field | After `rm` + recreate in the same directory | +| --------- | ------------------------------------------------------------------------ | +| `dev` | unchanged — same filesystem | +| `ino` | **reused 100% of the time** (3000/3000); ext4 hands back the freed inode | +| `nlink` | `1` before and after | +| `size` | unchanged whenever the replacement is the same length | +| `mtimeMs` | **quantised to 1 ms** — the kernel's coarse clock advances once a tick | + +So for a same-length replacement the whole identity string collapses to a 1 ms +timestamp race. The full string collided in 63.7% of back-to-back iterations, 19.8% +with a 0.5 ms gap, and 0% at ≥1 ms. The same probe on macOS collided 0 times in 2000 — +no inode reuse, nanosecond mtimes — which is why this only ever showed up on Linux CI. + +`ino` contributes no discriminating power against the exact case it is there to catch. +Do not add `ctimeMs` or `birthtimeMs` hoping to fix this: they come from the same +coarse clock and collide in the same window. `bigint: true` stats do not help either — +the precision loss is in the stored kernel timestamp, not in Node's `Number`. + +## The content digest sits alongside the identity, and cannot replace it + +Local grants also pin a sha256 of the artifact's bytes, read from the **same open +handle** as the stat so the two describe one inode with no gap between them. + +The identity string stays exactly as it is because it is a wire contract, not a +host-local detail: `src/relay/fs-handler-terminal-artifact.ts` recomputes that same +`dev:ino:nlink:size:mtimeMs` format field-for-field from its own stat and compares it +against the `expectedStatIdentity` the host sends. Changing the format would have a new +host publishing a string an older relay can never reproduce, failing every remote +artifact read as `terminal_file_grant_stale` — a break that reaches old peers with no +wire-schema change at all. See [remote wire compatibility](./remote-wire-compatibility.md). + +## What is still open + +The digest narrows these checks. It does not make any of them atomic. + +- **Read and preview — closed.** The bytes returned to the caller are the same + in-memory buffer that was digested, with no re-read in between, and the handle pins + the inode for the whole operation. A swap landing after the `open` leaves the handle + on the granted inode, so the granted content is what is served; a swap landing before + it fails the digest. +- **Write — a window survives.** Between the final pre-rename verification and the + `rename()` itself, the target path can still be swapped, and the rename clobbers + whatever is there. POSIX `rename()` has no "only if the target is still inode X" + form; Linux's `renameat2(RENAME_EXCHANGE)` would close it but is not portable and is + not exposed by Node. Narrowing this further means changing the commit strategy, not + adding another check before it. +- **Artifacts over 10 MB fall back to stat-only.** They digest to `null`, so only the + identity guards them. Nothing leaks: every read, preview, and write path rejects on + size before reading. The weakness is unreachable, not fixed — a later cap change + could expose it. +- **Remote and SSH grants are untouched.** They keep the stat-only check, with the full + 1 ms weakness on whatever filesystem the relay runs. Closing it needs a negotiated + capability so an older relay is never sent a digest it cannot verify. + +## What is proven, and what is inferred + +The filesystem numbers above are direct measurements. That identical stats plus changed +content previously returned the swapped bytes, and now do not, is pinned by +`orca-runtime-files-terminal-artifact-swap-detection.test.ts`, which replays the first +stat seen for a path so the collision is deterministic rather than a 1 ms coin flip. + +The join between the two is a chain, not a single observation. This defect surfaced as +an intermittent failure of `orca-runtime-files-terminal-artifact-io.test.ts` on +`rejects stale absolute terminal artifact previews before returning changed content`, +which swaps an 8-byte artifact for 8 different bytes. No one has instrumented a Linux +runner to prove that a specific CI failure was a same-tick mtime collision; the +conclusion rests on every ingredient being measured separately. Treat it accordingly if +a future failure does not fit. diff --git a/src/main/runtime/orca-runtime-files-terminal-artifact-swap-detection.test.ts b/src/main/runtime/orca-runtime-files-terminal-artifact-swap-detection.test.ts new file mode 100644 index 00000000000..95c6121a51a --- /dev/null +++ b/src/main/runtime/orca-runtime-files-terminal-artifact-swap-detection.test.ts @@ -0,0 +1,153 @@ +import { describe, expect, it, vi } from 'vitest' +import { rm, writeFile } from 'node:fs/promises' +import type * as FsPromises from 'node:fs/promises' +import { openMock, resolveAuthorizedPathMock } from './orca-runtime-files-mock-registry' +import { + createRuntimeFileCommands, + useRuntimeFileCommandsLifecycle +} from './orca-runtime-files-test-harness' +import { + absoluteFileTarget, + resolveTerminalArtifactPath, + useTerminalArtifactTempFiles +} from './orca-runtime-files-terminal-artifact-fixtures' + +vi.mock('fs', async () => (await import('./orca-runtime-files-mock-registry')).fsModuleMock()) +vi.mock('fs/promises', async () => + (await import('./orca-runtime-files-mock-registry')).fsPromisesModuleMock() +) +vi.mock( + './file-watcher-host', + async () => (await import('./orca-runtime-files-mock-registry')).fileWatcherHostMock +) +vi.mock('../ipc/filesystem-auth', async () => + (await import('./orca-runtime-files-mock-registry')).filesystemAuthModuleMock() +) +vi.mock('../git/runner', async () => + (await import('./orca-runtime-files-mock-registry')).gitRunnerModuleMock() +) +vi.mock( + '../ipc/rg-availability', + async () => (await import('./orca-runtime-files-mock-registry')).rgAvailabilityMock +) +vi.mock( + '../ipc/local-worktree-runtime-options', + async () => (await import('./orca-runtime-files-mock-registry')).localWorktreeRuntimeOptionsMock +) +vi.mock( + '../ipc/filesystem-search-git', + async () => (await import('./orca-runtime-files-mock-registry')).filesystemSearchGitMock +) +vi.mock( + '../providers/ssh-filesystem-dispatch', + async () => (await import('./orca-runtime-files-mock-registry')).sshFilesystemDispatchMock +) + +/** + * Replays the first stat seen for a path on every later open of it. That is what Linux hands the + * grant check for free after an unlink+recreate: ext4 reuses the just-freed inode, nlink and size + * are unchanged for a same-size swap, and the coarse mtime clock only advances once per timer tick + * (measured: 1ms), so a swap inside one tick is byte-for-byte identical to the granted stat. + */ +function replayFirstStatPerPath(): void { + const frozen = new Map() + openMock.mockImplementation(async (...args) => { + const actual = await vi.importActual('fs/promises') + const handle = await actual.open(...args) + const key = String(args[0]) + return { + stat: async () => { + if (!frozen.has(key)) { + frozen.set(key, await handle.stat()) + } + return frozen.get(key) + }, + read: (...readArgs: Parameters) => handle.read(...readArgs), + close: () => handle.close() + } + }) +} + +describe('terminal artifact swap detection', () => { + useRuntimeFileCommandsLifecycle() + + const { tempFile } = useTerminalArtifactTempFiles() + + async function grantFor(artifactPath: string) { + const { commands } = createRuntimeFileCommands({ path: '/repo' }) + resolveAuthorizedPathMock.mockImplementation(async (p: string) => p) + const result = await resolveTerminalArtifactPath(commands, artifactPath) + return { commands, target: absoluteFileTarget(result) } + } + + it('rejects a same-size preview swap that the granted stat cannot tell apart', async () => { + replayFirstStatPerPath() + const artifactPath = await tempFile('result.png', 'fake-png') + const { commands, target } = await grantFor(artifactPath) + + await rm(artifactPath) + await writeFile(artifactPath, 'changed!') + + await expect( + commands.readTerminalArtifactPreview( + 'id:wt-1', + target.grantId, + target.absolutePath, + 'client-a' + ) + ).rejects.toThrow('terminal_file_grant_stale') + }) + + it('rejects a same-size read swap that the granted stat cannot tell apart', async () => { + replayFirstStatPerPath() + const artifactPath = await tempFile('result.json', '{"ok":true}') + const { commands, target } = await grantFor(artifactPath) + + await rm(artifactPath) + await writeFile(artifactPath, '{"ok":ext}') + + await expect( + commands.readTerminalArtifactFile('id:wt-1', target.grantId, target.absolutePath, 'client-a') + ).rejects.toThrow('terminal_file_grant_stale') + }) + + it('rejects a same-size write swap before the file is changed', async () => { + replayFirstStatPerPath() + const artifactPath = await tempFile('result.json', '{"ok":true}') + const { commands, target } = await grantFor(artifactPath) + + await rm(artifactPath) + await writeFile(artifactPath, '{"ok":ext}') + + await expect( + commands.writeTerminalArtifactFile( + 'id:wt-1', + target.grantId, + target.absolutePath, + '{"ok":no}', + 'client-a' + ) + ).rejects.toThrow('terminal_file_grant_stale') + }) + + // Control: the replayed stat alone must not reject, or the swap cases above prove nothing. + it('still serves an untouched artifact under the same replayed stat', async () => { + replayFirstStatPerPath() + const artifactPath = await tempFile('result.png', 'fake-png') + const { commands, target } = await grantFor(artifactPath) + + await expect( + commands.readTerminalArtifactPreview( + 'id:wt-1', + target.grantId, + target.absolutePath, + 'client-a' + ) + ).resolves.toMatchObject({ + content: Buffer.from('fake-png').toString('base64'), + isBinary: true, + isImage: true, + mimeType: 'image/png' + }) + }) +}) diff --git a/src/main/runtime/orca-runtime-files-terminal-link-host-translation.test.ts b/src/main/runtime/orca-runtime-files-terminal-link-host-translation.test.ts index 290b0a3c558..d4f2f4e25ae 100644 --- a/src/main/runtime/orca-runtime-files-terminal-link-host-translation.test.ts +++ b/src/main/runtime/orca-runtime-files-terminal-link-host-translation.test.ts @@ -295,6 +295,10 @@ describe('RuntimeFileCommands', () => { ino: 2, mtimeMs: 3 })), + read: vi.fn(async (buffer: Buffer) => { + const bytesRead = buffer.write('{}') + return { bytesRead, buffer } + }), close: vi.fn(async () => undefined) }) diff --git a/src/main/runtime/runtime-file-commands-mobile-file-list-limit.ts b/src/main/runtime/runtime-file-commands-mobile-file-list-limit.ts index 0f497715993..37c519137ba 100644 --- a/src/main/runtime/runtime-file-commands-mobile-file-list-limit.ts +++ b/src/main/runtime/runtime-file-commands-mobile-file-list-limit.ts @@ -169,6 +169,9 @@ export type TerminalFileGrant = { clientId?: string expiresAt: number statIdentity: string | null + // Why: on Linux an unlink+recreate in the same directory reuses the inode and the mtime clock is + // tick-quantized, so statIdentity alone cannot see a same-size swap. Local grants pin content too. + contentDigest: string | null readOnly: boolean provenance: 'terminal-output' | 'native-chat' expiryTimer?: ReturnType diff --git a/src/main/runtime/runtime-file-commands-resolve-allowed-terminal-artifact-path.ts b/src/main/runtime/runtime-file-commands-resolve-allowed-terminal-artifact-path.ts index 8b80da0e69d..63d65ae4bb5 100644 --- a/src/main/runtime/runtime-file-commands-resolve-allowed-terminal-artifact-path.ts +++ b/src/main/runtime/runtime-file-commands-resolve-allowed-terminal-artifact-path.ts @@ -8,6 +8,7 @@ import { assertTerminalArtifactNotHardLinked, canonicalPathForArtifactComparison, isTerminalArtifactHardLinked, + localTerminalArtifactContentDigest, terminalFileStatIdentity } from './runtime-file-commands-terminal-artifact-access' import { @@ -63,9 +64,15 @@ export class RuntimeFileCommandsWithResolveAllowedTerminalArtifactPath extends R readOnly?: boolean provenance?: TerminalFileGrant['provenance'] }): Promise { - const stats = args.connectionId - ? await this.statRemoteTerminalPath(args.artifactPath, args.connectionId) - : await this.statLocalTerminalPath(args.artifactPath) + let contentDigest: string | null = null + let stats: RuntimeFileStatLike & { isDirectory: () => boolean } + if (args.connectionId) { + stats = await this.statRemoteTerminalPath(args.artifactPath, args.connectionId) + } else { + const local = await this.statLocalTerminalArtifact(args.artifactPath) + stats = local.stats + contentDigest = local.contentDigest + } const isDirectory = stats.isDirectory() if (!isDirectory && isTerminalArtifactHardLinked(stats)) { return { @@ -86,7 +93,8 @@ export class RuntimeFileCommandsWithResolveAllowedTerminalArtifactPath extends R clientId: args.clientId, readOnly: args.readOnly === true, provenance: args.provenance ?? 'terminal-output', - stats + stats, + contentDigest }) return { worktree: args.worktreeId, @@ -135,10 +143,22 @@ export class RuntimeFileCommandsWithResolveAllowedTerminalArtifactPath extends R protected async statLocalTerminalPath( absolutePath: string ): Promise boolean }> { + return (await this.statLocalTerminalArtifact(absolutePath)).stats + } + + /** Stat and content digest taken from one handle, so nothing can swap the file between them. */ + protected async statLocalTerminalArtifact(absolutePath: string): Promise<{ + stats: RuntimeFileStatLike & { isDirectory: () => boolean } + contentDigest: string | null + }> { await assertLocalTerminalArtifactPathStillCanonical(absolutePath) const handle = await open(absolutePath, 'r') try { - return handle.stat() + const stats = await handle.stat() + const contentDigest = stats.isDirectory() + ? null + : await localTerminalArtifactContentDigest(handle, stats.size) + return { stats, contentDigest } } finally { await handle.close() } @@ -153,6 +173,7 @@ export class RuntimeFileCommandsWithResolveAllowedTerminalArtifactPath extends R readOnly?: boolean provenance: TerminalFileGrant['provenance'] stats: RuntimeFileStatLike + contentDigest?: string | null }): TerminalFileGrant { assertTerminalArtifactNotHardLinked(args.stats) const grant: TerminalFileGrant = { @@ -164,6 +185,7 @@ export class RuntimeFileCommandsWithResolveAllowedTerminalArtifactPath extends R ...(args.clientId ? { clientId: args.clientId } : {}), expiresAt: Date.now() + TERMINAL_FILE_GRANT_TTL_MS, statIdentity: terminalFileStatIdentity(args.stats), + contentDigest: args.contentDigest ?? null, readOnly: args.readOnly === true, provenance: args.provenance } diff --git a/src/main/runtime/runtime-file-commands-terminal-artifact-access.ts b/src/main/runtime/runtime-file-commands-terminal-artifact-access.ts index 03d1c9ff3f9..48379079894 100644 --- a/src/main/runtime/runtime-file-commands-terminal-artifact-access.ts +++ b/src/main/runtime/runtime-file-commands-terminal-artifact-access.ts @@ -1,5 +1,6 @@ // @ts-nocheck -- mechanically split declarations. import { tmpdir } from 'node:os' +import { createHash } from 'node:crypto' import { parseWslPath, toWindowsWslPath } from '../wsl' import { realpath } from 'node:fs/promises' import type { FileHandle } from 'node:fs/promises' @@ -7,7 +8,10 @@ import type { RuntimeFileStatLike, TerminalFileGrant } from './runtime-file-commands-mobile-file-list-limit' -import { MOBILE_FILE_READ_MAX_BYTES } from './runtime-file-commands-mobile-file-list-limit' +import { + LOCAL_PREVIEWABLE_BINARY_MAX_BYTES, + MOBILE_FILE_READ_MAX_BYTES +} from './runtime-file-commands-mobile-file-list-limit' export async function localTerminalArtifactRoots(worktreePath: string): Promise { const roots = new Set([tmpdir()]) @@ -42,6 +46,47 @@ export async function readFileHandleBufferBounded( return buffer.subarray(0, bytesRead) } +/** + * Content fingerprint pinned into a local grant. Bounded by the largest previewable size so any + * artifact an access path can return is covered whole; larger files digest to null because both + * read paths reject them on size before any content leaves the host. + */ +export async function localTerminalArtifactContentDigest( + handle: FileHandle, + size: number +): Promise { + if (size > LOCAL_PREVIEWABLE_BINARY_MAX_BYTES) { + return null + } + const buffer = await readFileHandleBufferBounded(handle, LOCAL_PREVIEWABLE_BINARY_MAX_BYTES + 1) + return terminalArtifactContentDigest(buffer) +} + +export function terminalArtifactContentDigest(buffer: Buffer): string { + return createHash('sha256').update(buffer).digest('hex') +} + +/** + * Rejects a local artifact whose bytes changed since the grant was minted. `buffer` must hold the + * whole file — every local access path size-checks before it reads, so a buffer that reaches here + * is the complete artifact. + */ +export function assertTerminalArtifactContentUnchanged( + grant: Pick, + buffer: Buffer +): void { + if ( + grant.contentDigest !== null && + grant.contentDigest !== terminalArtifactContentDigest(buffer) + ) { + throw new Error('terminal_file_grant_stale') + } +} + +// Wire contract: the relay recomputes this exact string from its own stat to honour +// expectedStatIdentity, so the format cannot change without a negotiated capability. +// What this identity cannot distinguish, and which windows stay open after the digest: +// docs/reference/terminal-artifact-grant-integrity.md export function terminalFileStatIdentity(stats: RuntimeFileStatLike): string | null { const dev = typeof stats.dev === 'number' ? stats.dev : null const ino = typeof stats.ino === 'number' ? stats.ino : null diff --git a/src/main/runtime/runtime-file-commands-terminal-file-paths.ts b/src/main/runtime/runtime-file-commands-terminal-file-paths.ts index cc69baed7dd..dca41999ad8 100644 --- a/src/main/runtime/runtime-file-commands-terminal-file-paths.ts +++ b/src/main/runtime/runtime-file-commands-terminal-file-paths.ts @@ -14,6 +14,7 @@ import { } from './runtime-file-commands-mobile-file-list-limit' import type { FileHandle } from 'node:fs/promises' import { + assertTerminalArtifactContentUnchanged, assertTerminalFileGrantFresh, canonicalPathForArtifactComparison, localTerminalArtifactRoots, @@ -84,6 +85,7 @@ export async function readLocalTerminalArtifactFileFromHandle( } assertTerminalFileGrantFresh(grant, fileStat) const buffer = await readFileHandleBufferBounded(handle, MOBILE_FILE_READ_MAX_BYTES + 1) + assertTerminalArtifactContentUnchanged(grant, buffer) if (isBinaryBuffer(buffer)) { throw new Error('binary_file') } @@ -113,6 +115,7 @@ export async function readLocalTerminalArtifactPreviewFromHandle( if (buffer.byteLength > binaryMaxBytes) { throw new Error('file_too_large') } + assertTerminalArtifactContentUnchanged(grant, buffer) return { content: buffer.toString('base64'), isBinary: true, diff --git a/src/main/runtime/runtime-file-commands-write-terminal-artifact-file.ts b/src/main/runtime/runtime-file-commands-write-terminal-artifact-file.ts index a37f53d3f34..26c41e66b5f 100644 --- a/src/main/runtime/runtime-file-commands-write-terminal-artifact-file.ts +++ b/src/main/runtime/runtime-file-commands-write-terminal-artifact-file.ts @@ -8,6 +8,7 @@ import { } from './runtime-file-commands-mobile-file-list-limit' import { isBinaryBuffer, isMobileBinaryPath } from './runtime-file-command-host' import { + assertTerminalArtifactContentUnchanged, assertTerminalFileGrantFresh, readFileHandleBufferBounded, terminalFileStatIdentity @@ -75,9 +76,9 @@ export class RuntimeFileCommandsWithWriteTerminalArtifactFile extends RuntimeFil throw new Error('file_too_large') } assertTerminalFileGrantFresh(grant, fileStats) - if ( - isBinaryBuffer(await readFileHandleBufferBounded(handle, MOBILE_FILE_READ_MAX_BYTES + 1)) - ) { + const buffer = await readFileHandleBufferBounded(handle, MOBILE_FILE_READ_MAX_BYTES + 1) + assertTerminalArtifactContentUnchanged(grant, buffer) + if (isBinaryBuffer(buffer)) { throw new Error('binary_file') } } finally { @@ -95,13 +96,17 @@ export class RuntimeFileCommandsWithWriteTerminalArtifactFile extends RuntimeFil const freshHandle = await openLocalTerminalArtifactGrant(grant, constants.O_RDONLY) try { assertTerminalFileGrantFresh(grant, await freshHandle.stat()) + assertTerminalArtifactContentUnchanged( + grant, + await readFileHandleBufferBounded(freshHandle, MOBILE_FILE_READ_MAX_BYTES + 1) + ) } finally { await freshHandle.close() } await rename(tempPath, grant.absolutePath) - grant.statIdentity = terminalFileStatIdentity( - await this.statLocalTerminalPath(grant.absolutePath) - ) + const committed = await this.statLocalTerminalArtifact(grant.absolutePath) + grant.statIdentity = terminalFileStatIdentity(committed.stats) + grant.contentDigest = committed.contentDigest this.refreshTerminalFileGrant(grant) return { ok: true } } finally {