From 7829b1aeef8f43f186be2d2adbd6c67541306a7a Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 10 Oct 2026 04:14:28 +0800 Subject: [PATCH] fix(filesystem): reject unstable artifact targets --- src/application/BinaryTargetResolver.ts | 39 +++++++-- src/application/RegularFileRead.ts | 30 +------ src/application/WebScriptExportService.ts | 3 +- src/artifacts/inventory/classify.ts | 83 +++++++++++++++++-- .../inventory/hashStableRootArtifact.ts | 67 +++++++++++++++ src/artifacts/inventory/scanCanonical.ts | 18 ++-- src/cliJsonInput.ts | 6 +- src/filesystem/RegularFile.ts | 58 +++++++++++++ .../artifactInventoryRegularRoot.test.ts | 42 ++++++++++ .../boundary/filesystem/binaryTarget.test.ts | 23 +++++ tests/boundary/mcp/openBinaryFifo.test.ts | 48 +++++++++++ 11 files changed, 359 insertions(+), 58 deletions(-) create mode 100644 src/artifacts/inventory/hashStableRootArtifact.ts create mode 100644 src/filesystem/RegularFile.ts create mode 100644 tests/boundary/filesystem/artifactInventoryRegularRoot.test.ts create mode 100644 tests/boundary/mcp/openBinaryFifo.test.ts diff --git a/src/application/BinaryTargetResolver.ts b/src/application/BinaryTargetResolver.ts index f45a1d9a6..c1ec900bd 100644 --- a/src/application/BinaryTargetResolver.ts +++ b/src/application/BinaryTargetResolver.ts @@ -2,7 +2,7 @@ import { constants } from "node:fs"; import { createHash } from "node:crypto"; import { access, - open, + lstat, realpath, stat, type FileHandle, @@ -10,6 +10,10 @@ import { import { extname, isAbsolute, resolve } from "node:path"; import { BinaryTargetError } from "../domain/configurationErrors.js"; +import { + openRegularFile, + sameRegularFileState, +} from "../filesystem/RegularFile.js"; import { AnalysisCancelledError } from "../domain/analysisErrorCore.js"; import type { BinaryTarget } from "../domain/binaryTargetTypes.js"; import { err, ok, type Result } from "../domain/result.js"; @@ -69,11 +73,13 @@ export const parseBinaryTarget = async ( const { executable: path, infoPlist } = resolved.value; const bundle = infoPlist === undefined ? {} : { bundleInfoPlist: infoPlist }; - const handle = await open(path, "r"); + const handle = await openRegularFile(path, { + symlinks: "reject", + signal, + }); try { throwIfTargetResolutionCancelled(signal); - if (!(await handle.stat()).isFile()) - return err(new BinaryTargetError(path, "target is not a regular file")); + const metadata = await handle.stat(); throwIfTargetResolutionCancelled(signal); if (formatHint === "dos-com") { if (targetKind !== undefined && targetKind !== "executable") @@ -83,12 +89,12 @@ export const parseBinaryTarget = async ( "DOS COM format requires an executable target kind", ), ); - const length = validateDosComLength((await handle.stat()).size); + const length = validateDosComLength(metadata.size); if (!length.ok) return err(new BinaryTargetError(path, length.error)); return ok({ path, sourcePath: canonical, - sha256: await sha256Handle(handle, signal), + sha256: await sha256Handle(handle, path, metadata, signal), kind: "executable", format: "dos-com", architecture: "x86", @@ -102,7 +108,7 @@ export const parseBinaryTarget = async ( return ok({ path, sourcePath: process.platform === "win32" ? candidate : canonical, - sha256: await sha256Handle(handle, signal), + sha256: await sha256Handle(handle, path, metadata, signal), kind: "database", format: "analysis-database", }); @@ -111,7 +117,7 @@ export const parseBinaryTarget = async ( const identity = { path, sourcePath: process.platform === "win32" ? candidate : canonical, - sha256: await sha256Handle(handle, signal), + sha256: await sha256Handle(handle, path, metadata, signal), }; return isArchiveFormat(artifactFormat) ? ok({ ...identity, kind: "archive", format: artifactFormat }) @@ -127,7 +133,7 @@ export const parseBinaryTarget = async ( path, sourcePath: process.platform === "win32" ? candidate : canonical, ...bundle, - sha256: await sha256Handle(handle, signal), + sha256: await sha256Handle(handle, path, metadata, signal), kind: "executable", ...detected.value, }); @@ -136,6 +142,7 @@ export const parseBinaryTarget = async ( } } catch (cause: unknown) { if (cause instanceof AnalysisCancelledError) return err(cause); + if (cause instanceof BinaryTargetError) return err(cause); if (!(cause instanceof Error)) throw cause; const code = filesystemErrorCode(cause); if (code === undefined) throw cause; @@ -164,6 +171,10 @@ const targetFailureReason = (code: string, detail: string): string => { return `permission denied while reading target: ${detail}`; case "EISDIR": return `target is a directory, not a readable file: ${detail}`; + case "ENOTFILE": + return "target is not a regular file"; + case "ELOOP": + return `target changed while being opened: ${detail}`; default: return `target could not be read (${code}): ${detail}`; } @@ -235,6 +246,8 @@ const isArchiveFormat = ( const sha256Handle = async ( handle: FileHandle, + path: string, + initial: Awaited>, signal?: AbortSignal, ): Promise => { const hash = createHash("sha256"); @@ -248,6 +261,14 @@ const sha256Handle = async ( hash.update(chunk.subarray(0, observed.bytesRead)); position += observed.bytesRead; } + throwIfTargetResolutionCancelled(signal); + const [opened, currentPath] = await Promise.all([handle.stat(), lstat(path)]); + if ( + !sameRegularFileState(initial, opened) || + !sameRegularFileState(initial, currentPath) || + position !== initial.size + ) + throw new BinaryTargetError(path, "target changed while being read"); return hash.digest("hex"); }; diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index c5ad5f9e3..486e8c54d 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -1,38 +1,12 @@ -import { constants } from "node:fs"; -import { open } from "node:fs/promises"; - -/** The opened input cannot supply regular-file bytes. */ -export class NonRegularFileReadError extends Error { - readonly code: "EISDIR" | "ENOTFILE"; - - constructor( - readonly path: string, - directory: boolean, - ) { - super(`Selected input must be a regular file: ${path}`); - this.name = "NonRegularFileReadError"; - this.code = directory ? "EISDIR" : "ENOTFILE"; - } -} +import { openRegularFile } from "../filesystem/RegularFile.js"; /** Admit a regular file without waiting for a pipe, then read its verified handle. */ export const readRegularFile = async ( path: string, signal?: AbortSignal, ): Promise => { - signal?.throwIfAborted(); - const handle = await open( - path, - constants.O_RDONLY | - (constants.O_NONBLOCK ?? 0) | - (constants.O_NOCTTY ?? 0), - ); + const handle = await openRegularFile(path, { symlinks: "follow", signal }); try { - signal?.throwIfAborted(); - const stats = await handle.stat(); - signal?.throwIfAborted(); - if (!stats.isFile()) - throw new NonRegularFileReadError(path, stats.isDirectory()); const bytes = await handle.readFile({ signal }); signal?.throwIfAborted(); return bytes; diff --git a/src/application/WebScriptExportService.ts b/src/application/WebScriptExportService.ts index dfe54bb8e..1b3a47b7e 100644 --- a/src/application/WebScriptExportService.ts +++ b/src/application/WebScriptExportService.ts @@ -26,7 +26,8 @@ import { import { WebScriptExportError } from "../domain/webScriptExportError.js"; import type { ExecutionOptions } from "./AnalysisProvider.js"; import { WEB_SCRIPT_EXPORT_PROVIDER as PROVIDER } from "./InvestigationProviders.js"; -import { NonRegularFileReadError, readRegularFile } from "./RegularFileRead.js"; +import { readRegularFile } from "./RegularFileRead.js"; +import { NonRegularFileReadError } from "../filesystem/RegularFile.js"; const OPERATION = "export_web_scripts"; diff --git a/src/artifacts/inventory/classify.ts b/src/artifacts/inventory/classify.ts index 1cd6c754d..a8baf7a12 100644 --- a/src/artifacts/inventory/classify.ts +++ b/src/artifacts/inventory/classify.ts @@ -1,4 +1,10 @@ -import { open } from "node:fs/promises"; +import { + NonRegularFileReadError, + openRegularFile, + sameRegularFileState, +} from "../../filesystem/RegularFile.js"; +import type { FileHandle } from "node:fs/promises"; +import type { Stats } from "node:fs"; import { classifyArtifactContent } from "./ArtifactGraphConstruction.js"; import { ARTIFACT_CLASSIFICATION_PREFIX_BYTES } from "../ArtifactHash.js"; @@ -7,6 +13,9 @@ import { hasZipSignature, zipPackageFormatForPath, } from "../../domain/zipPackageFormat.js"; +import { ArtifactReaderFailure } from "../ArtifactReader.js"; +import type { HashResult } from "../ArtifactHash.js"; +import { hashStableRootArtifactHandle } from "./hashStableRootArtifact.js"; const classifyContainerExtension = ( path: string, @@ -26,15 +35,73 @@ export const classifyRoot = async ( if (directory) return "directory"; const extensionFormat = classifyContainerExtension(path); if (extensionFormat !== undefined) return extensionFormat; - const handle = await open(path, "r"); + const handle = await openRootFile(path); try { - const magic = Buffer.alloc(ARTIFACT_CLASSIFICATION_PREFIX_BYTES); - const observed = await handle.read(magic, 0, magic.length, 0); - const prefix = magic.subarray(0, observed.bytesRead); - if (hasZipSignature(prefix)) return "zip"; - return classifyArtifactContent(path, prefix, (await handle.stat()).size) - .format; + return await classifyOpenedRoot(path, handle); } finally { await handle.close(); } }; + +/** Classify and hash one file root through the same stable open descriptor. */ +export const classifyAndHashRoot = async ( + path: string, + directory: boolean, + expectedMetadata: Stats, + signal?: AbortSignal, +): Promise<{ + readonly format: ArtifactOccurrence["artifact_format"]; + readonly digest: HashResult | null; +}> => { + if (directory) return { format: "directory", digest: null }; + const handle = await openRootFile(path, signal); + try { + const initial = await handle.stat(); + if (!sameRegularFileState(expectedMetadata, initial)) + throw new ArtifactReaderFailure( + "integrity", + `Root artifact changed before inventory: ${path}`, + ); + const format = + classifyContainerExtension(path) ?? + (await classifyOpenedRoot(path, handle)); + const digest = await hashStableRootArtifactHandle( + path, + handle, + initial, + signal, + ); + return { format, digest }; + } finally { + await handle.close(); + } +}; + +const openRootFile = async ( + path: string, + signal?: AbortSignal, +): Promise => { + try { + return await openRegularFile(path, { symlinks: "reject", signal }); + } catch (cause: unknown) { + if (cause instanceof NonRegularFileReadError) + throw new ArtifactReaderFailure( + "format", + `Artifact root is not a regular file: ${path}`, + { cause }, + ); + throw cause; + } +}; + +const classifyOpenedRoot = async ( + path: string, + handle: FileHandle, +): Promise => { + const magic = Buffer.alloc(ARTIFACT_CLASSIFICATION_PREFIX_BYTES); + const observed = await handle.read(magic, 0, magic.length, 0); + const prefix = magic.subarray(0, observed.bytesRead); + if (hasZipSignature(prefix)) return "zip"; + return classifyArtifactContent(path, prefix, (await handle.stat()).size) + .format; +}; diff --git a/src/artifacts/inventory/hashStableRootArtifact.ts b/src/artifacts/inventory/hashStableRootArtifact.ts new file mode 100644 index 000000000..e441062d3 --- /dev/null +++ b/src/artifacts/inventory/hashStableRootArtifact.ts @@ -0,0 +1,67 @@ +import { lstat } from "node:fs/promises"; +import type { FileHandle } from "node:fs/promises"; + +import { ArtifactReaderFailure } from "../ArtifactReader.js"; +import { + NonRegularFileReadError, + openRegularFile, + sameRegularFileState, +} from "../../filesystem/RegularFile.js"; +import { + abortIfNeeded, + hashReadable, + type HashResult, +} from "../ArtifactHash.js"; + +/** Hash one stable regular root file from the same descriptor that was checked. */ +export const hashStableRootArtifact = async ( + path: string, + signal?: AbortSignal, +): Promise => { + let handle: FileHandle; + try { + handle = await openRegularFile(path, { + symlinks: "reject", + signal, + }); + } catch (cause: unknown) { + if (cause instanceof NonRegularFileReadError) + throw new ArtifactReaderFailure( + "format", + `Artifact root is not a regular file: ${path}`, + { cause }, + ); + throw cause; + } + try { + const initial = await handle.stat(); + return await hashStableRootArtifactHandle(path, handle, initial, signal); + } finally { + await handle.close(); + } +}; + +/** Hash and revalidate the same open descriptor used for root classification. */ +export const hashStableRootArtifactHandle = async ( + path: string, + handle: FileHandle, + initial: Awaited>, + signal?: AbortSignal, +): Promise => { + const digest = await hashReadable( + handle.createReadStream({ autoClose: false }), + signal, + ); + abortIfNeeded(signal); + const [opened, currentPath] = await Promise.all([handle.stat(), lstat(path)]); + if ( + !sameRegularFileState(initial, opened) || + !sameRegularFileState(initial, currentPath) || + digest.bytes !== initial.size + ) + throw new ArtifactReaderFailure( + "integrity", + `Root artifact changed during inventory: ${path}`, + ); + return digest; +}; diff --git a/src/artifacts/inventory/scanCanonical.ts b/src/artifacts/inventory/scanCanonical.ts index da046f18b..ca1324e17 100644 --- a/src/artifacts/inventory/scanCanonical.ts +++ b/src/artifacts/inventory/scanCanonical.ts @@ -1,7 +1,6 @@ import type { ArtifactInventorySnapshot } from "../../domain/artifactInventorySnapshot.js"; import { compareUnicodeCodePoints } from "../../domain/unicodeCodePointOrder.js"; -import { createReadStream } from "node:fs"; import { lstat } from "node:fs/promises"; import type { Stats } from "node:fs"; @@ -30,8 +29,9 @@ import { artifactGraphDigest, artifactManifestId, } from "../../domain/artifactIdentity.js"; -import { classifyRoot } from "./classify.js"; -import { hashReadable, type HashResult } from "../ArtifactHash.js"; +import { classifyAndHashRoot } from "./classify.js"; +import { type HashResult } from "../ArtifactHash.js"; +import { hashStableRootArtifact } from "./hashStableRootArtifact.js"; import { createReader, inventoryLimitations } from "./reader.js"; import { scanReader, @@ -49,10 +49,12 @@ export const scanCanonicalArtifactInventory = async ( ): Promise => { const integrity = options.integrity ?? STRICT_INTEGRITY_POLICY; const metadata = await lstat(path); - const rootFormat = await classifyRoot(path, metadata.isDirectory()); - const rootDigest = metadata.isDirectory() - ? null - : await hashReadable(createReadStream(path), options.signal); + const { format: rootFormat, digest: rootDigest } = await classifyAndHashRoot( + path, + metadata.isDirectory(), + metadata, + options.signal, + ); const reader = await readerFactory( path, rootFormat, @@ -240,7 +242,7 @@ const verifyRootDigest = async ( signal: AbortSignal | undefined, ): Promise => { if (rootDigest === null) return; - const verified = await hashReadable(createReadStream(path), signal); + const verified = await hashStableRootArtifact(path, signal); if ( verified.sha256 !== rootDigest.sha256 || verified.bytes !== rootDigest.bytes diff --git a/src/cliJsonInput.ts b/src/cliJsonInput.ts index 6b3947ffc..ebab099fe 100644 --- a/src/cliJsonInput.ts +++ b/src/cliJsonInput.ts @@ -1,10 +1,8 @@ import { constants as bufferConstants } from "node:buffer"; import { resolve } from "node:path"; -import { - NonRegularFileReadError, - readRegularFile, -} from "./application/RegularFileRead.js"; +import { readRegularFile } from "./application/RegularFileRead.js"; +import { NonRegularFileReadError } from "./filesystem/RegularFile.js"; import { parseUtf8Json } from "./application/Utf8JsonInput.js"; import { AnalysisAccessDeniedError, diff --git a/src/filesystem/RegularFile.ts b/src/filesystem/RegularFile.ts new file mode 100644 index 000000000..e5afac401 --- /dev/null +++ b/src/filesystem/RegularFile.ts @@ -0,0 +1,58 @@ +import { constants } from "node:fs"; +import type { BigIntStats, Stats } from "node:fs"; +import { open, type FileHandle } from "node:fs/promises"; + +export class NonRegularFileReadError extends Error { + readonly code: "EISDIR" | "ENOTFILE"; + + constructor( + readonly path: string, + directory: boolean, + ) { + super(`Selected input must be a regular file: ${path}`); + this.name = "NonRegularFileReadError"; + this.code = directory ? "EISDIR" : "ENOTFILE"; + } +} + +/** Open and verify the selected file descriptor without blocking on pipes. */ +export const openRegularFile = async ( + path: string, + options: { + readonly symlinks: "follow" | "reject"; + readonly signal?: AbortSignal | undefined; + }, +): Promise => { + options.signal?.throwIfAborted(); + const handle = await open( + path, + constants.O_RDONLY | + (constants.O_NONBLOCK ?? 0) | + (constants.O_NOCTTY ?? 0) | + (options.symlinks === "reject" ? (constants.O_NOFOLLOW ?? 0) : 0), + ); + try { + options.signal?.throwIfAborted(); + const metadata = await handle.stat(); + options.signal?.throwIfAborted(); + if (!metadata.isFile()) + throw new NonRegularFileReadError(path, metadata.isDirectory()); + return handle; + } catch (cause: unknown) { + await handle.close().catch(() => undefined); + throw cause; + } +}; + +/** Compare one regular file's identity and mutable metadata across a read. */ +export const sameRegularFileState = ( + expected: Stats | BigIntStats, + observed: Stats | BigIntStats, +): boolean => + observed.isFile() && + expected.dev === observed.dev && + expected.ino === observed.ino && + expected.size === observed.size && + expected.mode === observed.mode && + expected.mtimeMs === observed.mtimeMs && + expected.ctimeMs === observed.ctimeMs; diff --git a/tests/boundary/filesystem/artifactInventoryRegularRoot.test.ts b/tests/boundary/filesystem/artifactInventoryRegularRoot.test.ts new file mode 100644 index 000000000..b2e4f6d78 --- /dev/null +++ b/tests/boundary/filesystem/artifactInventoryRegularRoot.test.ts @@ -0,0 +1,42 @@ +import { execFile } from "node:child_process"; +import { lstat, rename, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { promisify } from "node:util"; + +import { expect, it } from "vitest"; + +import { inventoryArtifact } from "../../../src/artifacts/inventory/ArtifactInventory.js"; +import { classifyAndHashRoot } from "../../../src/artifacts/inventory/classify.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +it.skipIf(process.platform === "win32")( + "rejects a FIFO artifact root without waiting for a writer", + async () => { + const root = await createTestTempDirectory("rea-artifact-root-fifo-"); + const fifoPath = join(root, "input.asar"); + await promisify(execFile)("mkfifo", [fifoPath]); + + await expect(inventoryArtifact(fifoPath)).rejects.toMatchObject({ + reason: "format", + message: `Artifact root is not a regular file: ${fifoPath}`, + }); + }, +); + +it("rejects a root path replaced between its initial stat and open", async () => { + const root = await createTestTempDirectory("rea-artifact-root-replaced-"); + const path = join(root, "input.bin"); + const replacement = join(root, "replacement.bin"); + await writeFile(path, "first bytes\n"); + const expectedMetadata = await lstat(path); + await writeFile(replacement, "different bytes\n"); + await rm(path); + await rename(replacement, path); + + await expect( + classifyAndHashRoot(path, false, expectedMetadata), + ).rejects.toMatchObject({ + reason: "integrity", + message: `Root artifact changed before inventory: ${path}`, + }); +}); diff --git a/tests/boundary/filesystem/binaryTarget.test.ts b/tests/boundary/filesystem/binaryTarget.test.ts index 69ff26fab..669ee9265 100644 --- a/tests/boundary/filesystem/binaryTarget.test.ts +++ b/tests/boundary/filesystem/binaryTarget.test.ts @@ -24,6 +24,29 @@ import { } from "../../../src/domain/binaryTarget.fixture.js"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; + +it.skipIf(process.platform === "win32")( + "rejects a selected FIFO without waiting for a writer", + async () => { + const root = await createTestTempDirectory("rea-binary-target-fifo-"); + const fifoPath = join(root, "input.bin"); + await promisify(execFile)("mkfifo", [fifoPath]); + const outcome = await readWithoutFifoWriter(fifoPath, () => + parseBinaryTarget(fifoPath), + ); + expect(outcome.state).toBe("completed"); + if (outcome.state !== "completed") + throw new Error("Binary target resolution waited for a FIFO writer"); + expect(outcome.result).toMatchObject({ + ok: false, + error: { + _tag: "BinaryTargetError", + reason: "target is not a regular file", + }, + }); + }, +); describe("binary target I/O: app plist permissions and decoding", () => { it.each([ diff --git a/tests/boundary/mcp/openBinaryFifo.test.ts b/tests/boundary/mcp/openBinaryFifo.test.ts new file mode 100644 index 000000000..27a2ee1b7 --- /dev/null +++ b/tests/boundary/mcp/openBinaryFifo.test.ts @@ -0,0 +1,48 @@ +import { execFile } from "node:child_process"; +import { join } from "node:path"; +import { promisify } from "node:util"; + +import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; +import { expect, it } from "vitest"; + +import { ArtifactProvider } from "../../../src/artifacts/ArtifactProvider.js"; +import { createServer } from "../../../src/server/createServer.js"; +import { createTestBinarySession } from "../../fixtures/binarySession.js"; +import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; + +it.skipIf(process.platform === "win32")( + "rejects an open_binary FIFO without waiting for a writer", + async () => { + const root = await createTestTempDirectory("rea-open-binary-fifo-"); + const fifoPath = join(root, "input.bin"); + await promisify(execFile)("mkfifo", [fifoPath]); + const session = createTestBinarySession(new ArtifactProvider(process.env)); + const server = createServer({ kind: "session", session }); + const client = new Client({ name: "fifo-target-test", version: "1" }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + + try { + await server.connect(serverTransport); + await client.connect(clientTransport); + const outcome = await readWithoutFifoWriter(fifoPath, () => + client.callTool({ name: "open_binary", arguments: { path: fifoPath } }), + ); + expect(outcome.state).toBe("completed"); + if (outcome.state !== "completed") + throw new Error("MCP open_binary waited for a FIFO writer"); + expect(outcome.result.isError).toBe(true); + expect(parseMcpToolError(outcome.result)).toMatchObject({ + error: { message: expect.stringContaining("not a regular file") }, + }); + } finally { + await Promise.allSettled([ + client.close(), + server.close(), + session.close(), + ]); + } + }, +);