diff --git a/src/artifacts/ArtifactReader.ts b/src/artifacts/ArtifactReader.ts index 1278d815de..f072dd35cb 100644 --- a/src/artifacts/ArtifactReader.ts +++ b/src/artifacts/ArtifactReader.ts @@ -26,11 +26,35 @@ export interface ArtifactEntry { readonly adapterKey: string; /** Filesystem identity captured during traversal, when supplied by an adapter. */ readonly sourceIdentity?: { - readonly device: number; - readonly inode: number; + readonly device: bigint; + readonly inode: bigint; + readonly size: bigint; + readonly mode: bigint; + readonly mtimeNs: bigint; + readonly ctimeNs: bigint; }; } +/** Compare captured filesystem identity and mutable metadata at a read boundary. */ +export const sameArtifactSourceIdentity = ( + expected: ArtifactEntry["sourceIdentity"], + observed: { + readonly dev: bigint; + readonly ino: bigint; + readonly size: bigint; + readonly mode: bigint; + readonly mtimeNs: bigint; + readonly ctimeNs: bigint; + }, +): boolean => + expected !== undefined && + observed.dev === expected.device && + observed.ino === expected.inode && + observed.size === expected.size && + observed.mode === expected.mode && + observed.mtimeNs === expected.mtimeNs && + observed.ctimeNs === expected.ctimeNs; + /** Snapshot mutable entry metadata before the producer later validates open(). */ export const copyArtifactEntry = (entry: ArtifactEntry): ArtifactEntry => ({ ...entry, @@ -60,7 +84,11 @@ export const sameArtifactEntry = ( (limitation, index) => limitation === produced.limitations[index], ) && entry.sourceIdentity?.device === produced.sourceIdentity?.device && - entry.sourceIdentity?.inode === produced.sourceIdentity?.inode; + entry.sourceIdentity?.inode === produced.sourceIdentity?.inode && + entry.sourceIdentity?.size === produced.sourceIdentity?.size && + entry.sourceIdentity?.mode === produced.sourceIdentity?.mode && + entry.sourceIdentity?.mtimeNs === produced.sourceIdentity?.mtimeNs && + entry.sourceIdentity?.ctimeNs === produced.sourceIdentity?.ctimeNs; /** Read-only adapter over one directory, archive, or virtual container. */ export interface ArtifactReader { diff --git a/src/artifacts/DirectoryArtifactReader.ts b/src/artifacts/DirectoryArtifactReader.ts index cbbbbba69e..4a1d671352 100644 --- a/src/artifacts/DirectoryArtifactReader.ts +++ b/src/artifacts/DirectoryArtifactReader.ts @@ -1,4 +1,4 @@ -import { constants, type Stats } from "node:fs"; +import { constants, type BigIntStats } from "node:fs"; import { lstat, open, @@ -11,6 +11,7 @@ import { Readable } from "node:stream"; import { ArtifactReaderFailure, + sameArtifactSourceIdentity, type ArtifactEntry, type ArtifactReader, } from "./ArtifactReader.js"; @@ -95,9 +96,11 @@ export class DirectoryArtifactReader implements ArtifactReader { if (child === null) break; abortIfNeeded(signal); const absolute = join(directory, child.name); - const metadata = await lstat(absolute).catch((cause: unknown) => { - throw directoryIoFailure("inspect entry", absolute, cause); - }); + const metadata = await lstat(absolute, { bigint: true }).catch( + (cause: unknown) => { + throw directoryIoFailure("inspect entry", absolute, cause); + }, + ); const path = relative(root, absolute).split(sep).join("/"); const entry = artifactEntry(path, absolute, metadata); if (entry === undefined) continue; @@ -158,18 +161,18 @@ export class DirectoryArtifactReader implements ArtifactReader { const owner = new OwnedFileHandle(fileHandle); this.#files.set(owner, entry.adapterKey); try { - const observed = await owner.handle.stat().catch((cause: unknown) => { - throw directoryIoFailure( - "inspect opened entry", - entry.adapterKey, - cause, - ); - }); + const observed = await owner.handle + .stat({ bigint: true }) + .catch((cause: unknown) => { + throw directoryIoFailure( + "inspect opened entry", + entry.adapterKey, + cause, + ); + }); if ( !observed.isFile() || - entry.sourceIdentity === undefined || - observed.dev !== entry.sourceIdentity.device || - observed.ino !== entry.sourceIdentity.inode + !sameArtifactSourceIdentity(entry.sourceIdentity, observed) ) { throw new ArtifactReaderFailure( "integrity", @@ -178,7 +181,7 @@ export class DirectoryArtifactReader implements ArtifactReader { } abortIfNeeded(signal); const source = Readable.from( - mapFileReadCancellation(owner.handle, signal), + mapStableFileRead(owner.handle, entry, signal), { objectMode: false }, ); source.once("close", () => { @@ -256,7 +259,7 @@ export class DirectoryArtifactReader implements ArtifactReader { const artifactEntry = ( path: string, absolute: string, - metadata: Stats, + metadata: BigIntStats, ): ArtifactEntry | undefined => { const kind = metadata.isSymbolicLink() ? "symlink" @@ -269,7 +272,7 @@ const artifactEntry = ( return { path, kind, - declaredSize: kind === "file" ? metadata.size : null, + declaredSize: kind === "file" ? safeArtifactSize(metadata.size) : null, compressedSize: null, executable: hostExecutableMarking(absolute, metadata), encrypted: false, @@ -283,12 +286,24 @@ const artifactEntry = ( adapterKey: absolute, ...(kind === "file" ? { - sourceIdentity: { device: metadata.dev, inode: metadata.ino }, + sourceIdentity: { + device: metadata.dev, + inode: metadata.ino, + size: metadata.size, + mode: metadata.mode, + mtimeNs: metadata.mtimeNs, + ctimeNs: metadata.ctimeNs, + }, } : {}), }; }; +const safeArtifactSize = (size: bigint): number | null => { + if (size < 0n || size > BigInt(Number.MAX_SAFE_INTEGER)) return null; + return Number(size); +}; + const directoryIoFailure = ( operation: string, path: string, @@ -313,16 +328,45 @@ const directoryIoFailure = ( const failureMessage = (cause: unknown): string => cause instanceof Error ? cause.message : String(cause); -const mapFileReadCancellation = async function* ( +const mapStableFileRead = async function* ( handle: FileHandle, + entry: ArtifactEntry, signal?: AbortSignal, ): AsyncGenerator { + let bytes = 0n; try { - for await (const chunk of readFileHandleChunks(handle, { signal })) { + const expectedBytes = entry.sourceIdentity?.size; + if (expectedBytes === undefined) + throw new ArtifactReaderFailure( + "integrity", + `Directory entry has no captured file state: ${entry.path}`, + ); + for await (const chunk of readFileHandleChunks(handle, { + // Include one extra byte so a growing file cannot make this read unbounded. + end: expectedBytes, + signal, + })) { abortIfNeeded(signal); + bytes += BigInt(chunk.length); yield chunk; } abortIfNeeded(signal); + const [opened, current] = await Promise.all([ + handle.stat({ bigint: true }), + lstat(entry.adapterKey, { bigint: true }), + ]); + abortIfNeeded(signal); + if ( + !opened.isFile() || + !current.isFile() || + !sameArtifactSourceIdentity(entry.sourceIdentity, opened) || + !sameArtifactSourceIdentity(entry.sourceIdentity, current) || + bytes !== expectedBytes + ) + throw new ArtifactReaderFailure( + "integrity", + `Directory entry changed during read: ${entry.path}`, + ); } catch (cause: unknown) { if (signal?.aborted === true) throw new ArtifactReaderFailure( @@ -330,6 +374,18 @@ const mapFileReadCancellation = async function* ( "Directory artifact read cancelled", { cause }, ); + if ( + cause instanceof Error && + "code" in cause && + ["ENOENT", "ENOTDIR", "ELOOP"].includes(String(cause.code)) + ) + throw new ArtifactReaderFailure( + "integrity", + `Directory entry changed during read: ${entry.path}`, + { cause }, + ); + if (!(cause instanceof ArtifactReaderFailure)) + throw directoryIoFailure("validate read entry", entry.adapterKey, cause); throw cause; } }; diff --git a/src/artifacts/apple/AppleAssetCatalogAnalysis.ts b/src/artifacts/apple/AppleAssetCatalogAnalysis.ts index 1b469b6ef6..c91dc53124 100644 --- a/src/artifacts/apple/AppleAssetCatalogAnalysis.ts +++ b/src/artifacts/apple/AppleAssetCatalogAnalysis.ts @@ -1,7 +1,10 @@ import { createHash } from "node:crypto"; import { lstat } from "node:fs/promises"; import { DirectoryArtifactReader } from "../DirectoryArtifactReader.js"; -import { ArtifactReaderFailure } from "../ArtifactReader.js"; +import { + ArtifactReaderFailure, + sameArtifactSourceIdentity, +} from "../ArtifactReader.js"; import { analyzeInterfaceBuilderBundle } from "./InterfaceBuilderAnalysis.js"; import { appleAssetCatalogInputSchema, @@ -136,22 +139,20 @@ const inspectAssetCatalogEntry = async (input: { }) => { const { entry, reader, signal } = input; const sha256 = await digestBounded(reader, entry, signal); - const before = await lstat(entry.adapterKey); + const before = await lstat(entry.adapterKey, { bigint: true }); if ( - entry.sourceIdentity === undefined || - before.dev !== entry.sourceIdentity.device || - before.ino !== entry.sourceIdentity.inode + !before.isFile() || + !sameArtifactSourceIdentity(entry.sourceIdentity, before) ) throw new ArtifactReaderFailure( "integrity", `Asset catalog changed before inspection: ${entry.path}`, ); const output = await input.runAssetUtil(entry.adapterKey, signal); - const after = await lstat(entry.adapterKey); + const after = await lstat(entry.adapterKey, { bigint: true }); if ( - before.dev !== after.dev || - before.ino !== after.ino || - before.size !== after.size || + !after.isFile() || + !sameArtifactSourceIdentity(entry.sourceIdentity, after) || (await digestBounded(reader, entry, signal)) !== sha256 ) throw new ArtifactReaderFailure( diff --git a/src/artifacts/hostExecutableMarking.ts b/src/artifacts/hostExecutableMarking.ts index dcc4b4fb24..5549d099f6 100644 --- a/src/artifacts/hostExecutableMarking.ts +++ b/src/artifacts/hostExecutableMarking.ts @@ -1,4 +1,4 @@ -import type { Stats } from "node:fs"; +import type { BigIntStats, Stats } from "node:fs"; import { extname } from "node:path"; /** Extensions Windows launches directly through CreateProcess or cmd.exe. */ @@ -13,10 +13,12 @@ const windowsLaunchableExtensions = new Set([".bat", ".cmd", ".com", ".exe"]); */ export const hostExecutableMarking = ( path: string, - metadata: Pick, + metadata: Pick, platform: NodeJS.Platform = process.platform, ): boolean => platform === "win32" ? metadata.isFile() && windowsLaunchableExtensions.has(extname(path).toLowerCase()) - : (metadata.mode & 0o111) !== 0; + : typeof metadata.mode === "bigint" + ? (metadata.mode & 0o111n) !== 0n + : (metadata.mode & 0o111) !== 0; diff --git a/src/filesystem/readFileHandleChunks.ts b/src/filesystem/readFileHandleChunks.ts index f0e205891c..e9b9acc2f7 100644 --- a/src/filesystem/readFileHandleChunks.ts +++ b/src/filesystem/readFileHandleChunks.ts @@ -5,26 +5,30 @@ import { Readable } from "node:stream"; export const readFileHandleChunks = ( handle: Pick, options: { - readonly start?: number; - readonly end?: number; + readonly start?: number | bigint; + readonly end?: number | bigint; readonly signal?: AbortSignal | undefined; } = {}, ): Readable => { - const start = options.start ?? 0; - const end = options.end ?? Infinity; + const start = fileOffset(options.start ?? 0); + const unlimited = options.end === undefined || options.end === Infinity; + const end = unlimited ? undefined : fileOffset(options.end); if ( - !Number.isSafeInteger(start) || - start < 0 || - (end !== Infinity && (!Number.isSafeInteger(end) || end < start)) + start === undefined || + (!unlimited && end === undefined) || + (end !== undefined && end < start) ) throw new RangeError("Invalid file read range"); return Readable.from( (async function* () { let position = start; - while (position <= end) { + while (end === undefined || position <= end) { options.signal?.throwIfAborted(); + const remaining = end === undefined ? undefined : end - position + 1n; const chunk = Buffer.allocUnsafe( - Math.min(64 * 1024, end - position + 1), + remaining === undefined || remaining > 64n * 1024n + ? 64 * 1024 + : Number(remaining), ); const { bytesRead } = await handle.read( chunk, @@ -34,7 +38,7 @@ export const readFileHandleChunks = ( ); options.signal?.throwIfAborted(); if (bytesRead === 0) return; - position += bytesRead; + position += BigInt(bytesRead); yield chunk.subarray(0, bytesRead); } })(), @@ -44,3 +48,8 @@ export const readFileHandleChunks = ( }, ); }; + +const fileOffset = (value: number | bigint): bigint | undefined => { + if (typeof value === "bigint") return value >= 0n ? value : undefined; + return Number.isSafeInteger(value) && value >= 0 ? BigInt(value) : undefined; +}; diff --git a/tests/boundary/filesystem/artifactZipIntegrity.test.ts b/tests/boundary/filesystem/artifactZipIntegrity.test.ts index b2b8b179f5..946db4a867 100644 --- a/tests/boundary/filesystem/artifactZipIntegrity.test.ts +++ b/tests/boundary/filesystem/artifactZipIntegrity.test.ts @@ -61,6 +61,19 @@ it("binds ZIP opens to the emitted entry metadata", async () => { await expect( reader.open({ ...second, declaredSize: first.declaredSize }), ).rejects.toMatchObject({ reason: "integrity" }); + await expect( + reader.open({ + ...second, + sourceIdentity: { + device: 1n, + inode: 2n, + size: 3n, + mode: 4n, + mtimeNs: 5n, + ctimeNs: 6n, + }, + }), + ).rejects.toMatchObject({ reason: "integrity" }); const originalKey = second.adapterKey; Reflect.set(second, "adapterKey", first.adapterKey); await expect(reader.open(second)).rejects.toMatchObject({ diff --git a/tests/boundary/filesystem/directoryArtifactReaderOpen.test.ts b/tests/boundary/filesystem/directoryArtifactReaderOpen.test.ts index b8124dcf2f..554f23fbde 100644 --- a/tests/boundary/filesystem/directoryArtifactReaderOpen.test.ts +++ b/tests/boundary/filesystem/directoryArtifactReaderOpen.test.ts @@ -1,4 +1,4 @@ -import { lstat, open, rename, writeFile } from "node:fs/promises"; +import { lstat, open, rename, rm, utimes, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { buffer } from "node:stream/consumers"; @@ -29,27 +29,26 @@ describe("DirectoryArtifactReader.open", () => { const { path, reader } = await fixture(); try { const entry = await sampleEntry(reader); - const metadata = await lstat(path); + const metadata = await lstat(path, { bigint: true }); const handle = await open(path, "r"); try { - const observed = await handle.stat(); + const observed = await handle.stat({ bigint: true }); expect(entry.sourceIdentity).toEqual({ device: metadata.dev, inode: metadata.ino, + size: metadata.size, + mode: metadata.mode, + mtimeNs: metadata.mtimeNs, + ctimeNs: metadata.ctimeNs, }); - expect({ device: observed.dev, inode: observed.ino }).toEqual( - entry.sourceIdentity, - ); - console.info( - JSON.stringify({ - platform: process.platform, - node: process.version, - pathDevice: metadata.dev, - handleDevice: observed.dev, - pathInode: metadata.ino, - handleInode: observed.ino, - }), - ); + expect({ + device: observed.dev, + inode: observed.ino, + size: observed.size, + mode: observed.mode, + mtimeNs: observed.mtimeNs, + ctimeNs: observed.ctimeNs, + }).toEqual(entry.sourceIdentity); } finally { await handle.close(); } @@ -75,6 +74,73 @@ describe("DirectoryArtifactReader.open", () => { } }); + it.each(["changed in place with a new size\n", "TEST PAYLOAD\n"])( + "rejects an in-place file change after enumeration: %s", + async (replacement) => { + const { path, reader } = await fixture(); + try { + const entry = await sampleEntry(reader); + const before = await lstat(path); + await writeFile(path, replacement); + await utimes(path, before.atime, new Date(before.mtimeMs + 1000)); + const after = await lstat(path); + expect(after.ino).toBe(before.ino); + await expect(reader.open(entry)).rejects.toMatchObject({ + reason: "integrity", + message: expect.stringContaining("changed before open"), + }); + } finally { + await reader.close(); + } + }, + ); + + it.each(["truncate", "grow"])( + "rejects a same-file %s between streamed chunks within the captured byte bound", + async (mutation) => { + const root = await createTestTempDirectory("rea-dir-reader-mutate-"); + const path = join(root, "sample.txt"); + const originalBytes = 1024 * 1024; + await writeFile(path, Buffer.alloc(originalBytes, 0x61)); + const reader = new DirectoryArtifactReader(root); + try { + const entry = await sampleEntry(reader); + const stream = await reader.open(entry); + const iterator = stream[Symbol.asyncIterator](); + const first = await iterator.next(); + expect(first.done).toBe(false); + let observedBytes = + first.value === undefined ? 0 : Buffer.byteLength(first.value); + await writeFile( + path, + mutation === "truncate" + ? "short replacement\n" + : Buffer.alloc(2 * originalBytes, 0x62), + ); + let failure: unknown; + try { + for (;;) { + const next = await iterator.next(); + if (next.done) break; + observedBytes += Buffer.byteLength(next.value); + } + } catch (cause: unknown) { + failure = cause; + } + expect(failure).toMatchObject({ + reason: "integrity", + message: expect.stringContaining("changed during read"), + }); + expect(observedBytes).toBeLessThanOrEqual(originalBytes + 1); + } finally { + await reader.close(); + await rm(root, { recursive: true, force: true }); + } + }, + ); +}); + +describe("DirectoryArtifactReader input identity", () => { it.each(["zero", "another"] as const)( "requires the enumerated %s device to match the opened file", async (kind) => { @@ -84,13 +150,13 @@ describe("DirectoryArtifactReader.open", () => { const identity = entry.sourceIdentity; if (identity === undefined) throw new Error("Fixture omitted source identity"); - if (kind === "zero" && identity.device === 0) { + if (kind === "zero" && identity.device === 0n) { expect( (await buffer(await reader.open(entry))).toString("utf8"), ).toBe("test payload\n"); return; } - const device = kind === "zero" ? 0 : identity.device === 1 ? 2 : 1; + const device = kind === "zero" ? 0n : identity.device === 1n ? 2n : 1n; await expect( reader.open({ ...entry, sourceIdentity: { ...identity, device } }), ).rejects.toMatchObject({ reason: "integrity" });