From 5f16124a31fa4a82bd03037506809d6be959d768 Mon Sep 17 00:00:00 2001 From: You Wish <29498139+Y0UW1SH@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:49:54 -0400 Subject: [PATCH] fix(files): reject nonregular capture and JSON inputs Verify selected files through a nonblocking opened handle before reading. Preserve regular-file symlinks and return actionable CLI/MCP diagnostics without publishing output. Co-authored-by: YouWish <29498139+Y0UW1SH@users.noreply.github.com> --- docs/javascript-application-workflows.md | 4 + docs/website-script-export.md | 4 + src/application/RegularFileRead.ts | 42 ++++++ src/application/WebScriptExportService.ts | 20 ++- .../android/AndroidAnalysisService.ts | 7 +- src/cliJsonInput.ts | 14 +- .../cli/webScriptSelectionCli.test.ts | 46 +++++++ .../boundary/filesystem/cliJsonInput.test.ts | 80 ++++++++++- .../filesystem/webScriptExport.test.ts | 125 ++++++++++++++++++ .../mcp/webScriptSelectionMcp.test.ts | 78 +++++++++++ tests/fixtures/fifoInput.ts | 36 +++++ 11 files changed, 442 insertions(+), 14 deletions(-) create mode 100644 src/application/RegularFileRead.ts create mode 100644 tests/boundary/cli/webScriptSelectionCli.test.ts create mode 100644 tests/boundary/mcp/webScriptSelectionMcp.test.ts create mode 100644 tests/fixtures/fifoInput.ts diff --git a/docs/javascript-application-workflows.md b/docs/javascript-application-workflows.md index 5ff3886266..63be6935ff 100644 --- a/docs/javascript-application-workflows.md +++ b/docs/javascript-application-workflows.md @@ -175,6 +175,10 @@ All five CLI commands accept inline JSON or a path to a JSON file. The CLI returns an Evidence record directly. Put the full records in a later CLI input; a separate CLI process has no retained MCP connection state. +File inputs must be regular files; symlinks to regular files are accepted. +Directories, named pipes and device files produce an input error before JSON +parsing. + For a literal string trace, analyze your supplied tree once, then build the input from the saved Evidence (replace the target and seed): diff --git a/docs/website-script-export.md b/docs/website-script-export.md index 3745ff9aee..31ffaff45d 100644 --- a/docs/website-script-export.md +++ b/docs/website-script-export.md @@ -12,6 +12,10 @@ For passive inspection, select `include_script_sources: true` (CLI: `capture.network.response_body: true`. Save either the complete Evidence record or its normalized result as UTF-8 JSON. MCP `structuredContent` contains the complete Evidence record. +The capture must be a regular file. A symlink to a regular file is accepted; +directories, named pipes and device files are rejected before reading capture +bytes or creating output. + ```sh rea export-web-scripts /analysis/capture.json /analysis/exported-scripts --json rea analyze-javascript-application /analysis/exported-scripts/files --json diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts new file mode 100644 index 0000000000..c5ad5f9e34 --- /dev/null +++ b/src/application/RegularFileRead.ts @@ -0,0 +1,42 @@ +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"; + } +} + +/** 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), + ); + 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; + } finally { + await handle.close(); + } +}; diff --git a/src/application/WebScriptExportService.ts b/src/application/WebScriptExportService.ts index d35341f437..dfe54bb8e2 100644 --- a/src/application/WebScriptExportService.ts +++ b/src/application/WebScriptExportService.ts @@ -1,5 +1,4 @@ import { createHash } from "node:crypto"; -import { readFile } from "node:fs/promises"; import { isAbsolute } from "node:path"; import { z } from "zod"; @@ -27,6 +26,7 @@ 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"; const OPERATION = "export_web_scripts"; @@ -152,7 +152,7 @@ const readCapture = async ( signal: AbortSignal | undefined, ): Promise> => { try { - return ok(await readFile(path, { signal })); + return ok(await readRegularFile(path, signal)); } catch (cause: unknown) { const code = cause instanceof Error && "code" in cause ? String(cause.code) : ""; @@ -160,16 +160,24 @@ const readCapture = async ( return err( new AnalysisAccessDeniedError(OPERATION, path, code, { cause }), ); - if (code === "ENOENT" || code === "ENOTDIR" || code === "EISDIR") + if ( + cause instanceof NonRegularFileReadError || + code === "ENOENT" || + code === "ENOTDIR" || + code === "EISDIR" || + code === "ENXIO" + ) return err( new AnalysisInputError(OPERATION, { cause }, [ { path: ["capture_path"], reason: "invalid_value", message: - code === "EISDIR" - ? `Selected capture is a directory, not a file: ${path}` - : `Selected capture could not be read (${code}): ${path}`, + cause instanceof NonRegularFileReadError && code !== "EISDIR" + ? cause.message + : code === "EISDIR" + ? `Selected capture is a directory, not a file: ${path}` + : `Selected capture could not be read (${code}): ${path}`, }, ]), ); diff --git a/src/application/android/AndroidAnalysisService.ts b/src/application/android/AndroidAnalysisService.ts index a126df57c6..eae2efb8d8 100644 --- a/src/application/android/AndroidAnalysisService.ts +++ b/src/application/android/AndroidAnalysisService.ts @@ -29,9 +29,10 @@ export class AndroidAnalysisService { const request = androidRequestSchema.safeParse({ operation, input }); if (!request.success) return err(new AnalysisInputError(operation, { cause: request.error })); - const target = await parseBinaryTarget(request.data.input.path, { - ...(options?.signal === undefined ? {} : { signal: options.signal }), - }); + const target = await parseBinaryTarget( + request.data.input.path, + options?.signal === undefined ? {} : { signal: options.signal }, + ); if (!target.ok) return target; const executed = await this.provider.execute( target.value, diff --git a/src/cliJsonInput.ts b/src/cliJsonInput.ts index 9081b875e7..5c4397bf64 100644 --- a/src/cliJsonInput.ts +++ b/src/cliJsonInput.ts @@ -1,6 +1,9 @@ -import { readFile } from "node:fs/promises"; import { resolve } from "node:path"; +import { + NonRegularFileReadError, + readRegularFile, +} from "./application/RegularFileRead.js"; import { AnalysisInputError } from "./domain/analysisErrorCore.js"; import { projectAnalysisError } from "./domain/analysisErrorProjection.js"; import type { JsonValue } from "./domain/jsonValue.js"; @@ -19,7 +22,7 @@ export const parseCliJsonInput = async ( try { // Read raw bytes so invalid UTF-8 is rejected by parseJson instead of // being silently replaced by lossy "utf8" decoding. - const parsed = parseJson(await readFile(value)); + const parsed = parseJson(await readRegularFile(value)); return parsed === undefined ? jsonFileError(value, operation, "invalid-json") : { ok: true, value: parsed }; @@ -136,7 +139,7 @@ const jsonFileError = ( }, }); -/** Name the system error so the caller can tell a typo from a permission denial. */ +/** Preserve file-selection and system failures alongside the requested path. */ const readFailureIssue = (path: string | undefined, cause: unknown) => { const code = typeof cause === "object" && @@ -148,6 +151,9 @@ const readFailureIssue = (path: string | undefined, cause: unknown) => { return { path: [], reason: "invalid_value" as const, - message: `The JSON input file could not be read${code}: ${path ?? ""}`, + message: + cause instanceof NonRegularFileReadError && cause.code === "ENOTFILE" + ? `The JSON input must be a regular file${code}: ${path ?? ""}` + : `The JSON input file could not be read${code}: ${path ?? ""}`, }; }; diff --git a/tests/boundary/cli/webScriptSelectionCli.test.ts b/tests/boundary/cli/webScriptSelectionCli.test.ts new file mode 100644 index 0000000000..416c81ac1e --- /dev/null +++ b/tests/boundary/cli/webScriptSelectionCli.test.ts @@ -0,0 +1,46 @@ +import { access } from "node:fs/promises"; +import { join } from "node:path"; + +import { expect } from "vitest"; + +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest } from "../../support/cli/cliFixture.js"; + +cliTest.skipIf(process.platform === "win32")( + "exits with a capture_path invalid_request for a named pipe before publishing output", + async ({ cli, processes }) => { + const root = await createTestTempDirectory("rea-script-selection-cli-"); + const capturePath = join(root, "capture.pipe"); + const outputDirectory = join(root, "export"); + const created = await processes.run("mkfifo", [capturePath], { + timeoutMs: 5_000, + }); + expect(created.exitCode).toBe(0); + + const response = await cli.run({ + arguments: ["export-web-scripts", capturePath, outputDirectory, "--json"], + environment: { REA_LOG_LEVEL: "silent" }, + timeoutMs: 5_000, + }); + expect(response.exitCode).toBe(1); + expect(response.signal).toBeNull(); + expect(response.json).toMatchObject({ + error: "Script export failed", + code: "invalid_request", + details: { + operation: "export_web_scripts", + issues: [ + { + path: ["capture_path"], + reason: "invalid_value", + message: expect.stringContaining("regular file"), + }, + ], + }, + }); + expect(response.stdout).toContain(capturePath); + await expect(access(outputDirectory)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, +); diff --git a/tests/boundary/filesystem/cliJsonInput.test.ts b/tests/boundary/filesystem/cliJsonInput.test.ts index 1095840e0e..d75dac4e3d 100644 --- a/tests/boundary/filesystem/cliJsonInput.test.ts +++ b/tests/boundary/filesystem/cliJsonInput.test.ts @@ -1,11 +1,89 @@ -import { chmod, writeFile } from "node:fs/promises"; +import { execFile } from "node:child_process"; +import { chmod, symlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; +import { promisify } from "node:util"; import { describe, expect, it, onTestFinished } from "vitest"; import { parseCliJsonInput } from "../../../src/cliJsonInput.js"; +import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +describe("CLI JSON file selection", () => { + it("accepts a symlink to a regular JSON file", async () => { + const root = await createTestTempDirectory("rea-json-input-symlink-"); + const path = join(root, "input.json"); + const selected = join(root, "selected.json"); + await writeFile(path, '{"value":1}'); + await symlink(path, selected, "file"); + expect(await parseCliJsonInput(selected, "test-input")).toEqual({ + ok: true, + value: { value: 1 }, + }); + }); + + it + .skipIf(process.platform === "win32") + .each(["a named pipe", "a symlink to a named pipe"])( + "rejects %s without waiting for a writer", + async (kind) => { + const root = await createTestTempDirectory("rea-json-input-fifo-"); + const fifoPath = join(root, "input.pipe"); + await promisify(execFile)("mkfifo", [fifoPath]); + const selected = + kind === "a named pipe" ? fifoPath : join(root, "selected.json"); + if (selected !== fifoPath) await symlink(fifoPath, selected, "file"); + + const outcome = await readWithoutFifoWriter(fifoPath, () => + parseCliJsonInput(selected, "test-input"), + ); + expect(outcome.state).toBe("completed"); + if (outcome.state !== "completed") + throw new Error("JSON read waited for a FIFO writer"); + expect(outcome.result).toMatchObject({ + ok: false, + error: { + code: "invalid_request", + input_path: selected, + input_reason: "read-failed", + details: { + issues: [ + { + path: [], + reason: "invalid_value", + message: expect.stringContaining("(ENOTFILE)"), + }, + ], + }, + }, + }); + }, + ); + + it.skipIf(process.platform === "win32")( + "rejects a character device before parsing JSON bytes", + async () => { + expect(await parseCliJsonInput("/dev/null", "test-input")).toMatchObject({ + ok: false, + error: { + code: "invalid_request", + input_path: "/dev/null", + input_reason: "read-failed", + details: { + issues: [ + { + path: [], + reason: "invalid_value", + message: expect.stringContaining("(ENOTFILE)"), + }, + ], + }, + }, + }); + }, + ); +}); + describe("CLI JSON input", () => { it("distinguishes malformed inline text from files and preserves bracket-prefixed paths", async () => { expect(await parseCliJsonInput("[", "test-input")).toMatchObject({ diff --git a/tests/boundary/filesystem/webScriptExport.test.ts b/tests/boundary/filesystem/webScriptExport.test.ts index ebc13ca481..3a6ce47fdc 100644 --- a/tests/boundary/filesystem/webScriptExport.test.ts +++ b/tests/boundary/filesystem/webScriptExport.test.ts @@ -1,3 +1,4 @@ +import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; import { access, @@ -9,6 +10,7 @@ import { writeFile, } from "node:fs/promises"; import { join } from "node:path"; +import { promisify } from "node:util"; import { describe, expect, it, onTestFinished } from "vitest"; @@ -19,6 +21,7 @@ import { selectScriptCapture } from "../../../src/browser/assets/ScriptCaptureAd import { javascriptApplicationAnalysisResultSchema } from "../../../src/domain/javascript/javascriptApplicationAnalysis.js"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; import { webScriptExportResultSchema } from "../../../src/domain/webScriptExport.js"; +import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { scriptCaptureEvidenceFixture, @@ -272,6 +275,128 @@ describe("captured script publication failures and cleanup", () => { }); describe("captured script selection failures", () => { + it("accepts a capture symlink to a regular file and preserves its selected path", async () => { + const { root, input } = await setup(); + const selected = join(root, "selected-capture.json"); + await symlink(input.capture_path, selected, "file"); + + const result = await exportWebScripts({ ...input, capture_path: selected }); + if (!result.ok) throw result.error; + expect(result.value.subject?.local_path).toBe(selected); + expect( + webScriptExportResultSchema.parse(result.value.normalized_result) + .capture_sha256, + ).toBe( + createHash("sha256") + .update(await readFile(input.capture_path)) + .digest("hex"), + ); + await expect( + access(join(input.output_directory, "manifest.json")), + ).resolves.toBeUndefined(); + }); + + it + .skipIf(process.platform === "win32") + .each(["a named pipe", "a symlink to a named pipe"])( + "rejects %s without waiting for a writer or creating output", + async (kind) => { + const { root, input } = await setup(); + const fifoPath = join(root, "capture.pipe"); + await promisify(execFile)("mkfifo", [fifoPath]); + const selected = + kind === "a named pipe" + ? fifoPath + : join(root, "selected-capture.json"); + if (selected !== fifoPath) await symlink(fifoPath, selected, "file"); + + const outcome = await readWithoutFifoWriter(fifoPath, () => + exportWebScripts({ ...input, capture_path: selected }), + ); + expect(outcome.state).toBe("completed"); + if (outcome.state !== "completed") + throw new Error("Capture read waited for a FIFO writer"); + if (outcome.result.ok) + throw new Error("Expected invalid capture selection"); + expect(projectAnalysisError(outcome.result.error)).toMatchObject({ + code: "invalid_request", + details: { + issues: [ + { + path: ["capture_path"], + reason: "invalid_value", + message: expect.stringContaining("regular file"), + }, + ], + }, + }); + expect( + JSON.stringify(projectAnalysisError(outcome.result.error)), + ).toContain(selected); + await expect(access(input.output_directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, + ); + + it.skipIf(process.platform === "win32")( + "rejects a character device as a capture selection before parsing bytes", + async () => { + const { input } = await setup(); + const result = await exportWebScripts({ + ...input, + capture_path: "/dev/null", + }); + if (result.ok) throw new Error("Expected invalid capture selection"); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "invalid_request", + details: { + issues: [ + { + path: ["capture_path"], + reason: "invalid_value", + message: expect.stringContaining("regular file"), + }, + ], + }, + }); + expect(JSON.stringify(projectAnalysisError(result.error))).toContain( + "/dev/null", + ); + await expect(access(input.output_directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, + ); + + it.skipIf(process.platform === "win32")( + "honors a pre-aborted capture export before opening a named pipe", + async () => { + const { root, input } = await setup(); + const fifoPath = join(root, "capture.pipe"); + await promisify(execFile)("mkfifo", [fifoPath]); + const controller = new AbortController(); + controller.abort(); + + const outcome = await readWithoutFifoWriter(fifoPath, () => + exportWebScripts( + { ...input, capture_path: fifoPath }, + { signal: controller.signal }, + ), + ); + expect(outcome.state).toBe("completed"); + if (outcome.state !== "completed") + throw new Error("Aborted capture read waited for a FIFO writer"); + if (outcome.result.ok) throw new Error("Expected cancellation"); + expect(outcome.result.error._tag).toBe("AnalysisCancelledError"); + await expect(access(input.output_directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, + ); +}); + +describe("captured script path and permission failures", () => { it("reports a directory selected as the capture as invalid input", async () => { const { root, input } = await setup(); const directory = join(root, "captures"); diff --git a/tests/boundary/mcp/webScriptSelectionMcp.test.ts b/tests/boundary/mcp/webScriptSelectionMcp.test.ts new file mode 100644 index 0000000000..062718fc01 --- /dev/null +++ b/tests/boundary/mcp/webScriptSelectionMcp.test.ts @@ -0,0 +1,78 @@ +import { execFile } from "node:child_process"; +import { access } from "node:fs/promises"; +import { join } from "node:path"; +import { promisify } from "node:util"; + +import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; +import { STDIO_DEFAULT_MAX_BUFFER_SIZE } from "@modelcontextprotocol/server"; +import { expect, it, onTestFinished } from "vitest"; + +import { silentLogger } from "../../../src/logger.js"; +import { EvidenceMcpServer } from "../../../src/server/EvidenceMcpServer.js"; +import { registerWebScriptTool } from "../../../src/server/registerWebScriptTool.js"; +import { ToolResultDelivery } from "../../../src/server/toolResult.js"; +import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; +import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +it.skipIf(process.platform === "win32")( + "returns a capture_path invalid_request for a named pipe without waiting or publishing output", + async () => { + const root = await createTestTempDirectory("rea-script-selection-mcp-"); + const capturePath = join(root, "capture.pipe"); + const outputDirectory = join(root, "export"); + await promisify(execFile)("mkfifo", [capturePath]); + + const server = new EvidenceMcpServer( + { name: "script-selection", version: "1" }, + {}, + undefined, + new ToolResultDelivery(STDIO_DEFAULT_MAX_BUFFER_SIZE), + ); + registerWebScriptTool(server, { + logger: silentLogger, + recordEvidence: undefined, + }); + const client = new Client({ name: "script-selection-test", version: "1" }); + onTestFinished(async () => { + await client.close(); + await server.close(); + }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + const outcome = await readWithoutFifoWriter(capturePath, () => + client.callTool({ + name: "export_web_scripts", + arguments: { + capture_path: capturePath, + output_directory: outputDirectory, + }, + }), + ); + expect(outcome.state).toBe("completed"); + if (outcome.state !== "completed") + throw new Error("MCP capture selection waited for a FIFO writer"); + expect(parseMcpToolError(outcome.result)).toMatchObject({ + error: { + code: "invalid_request", + details: { + operation: "export_web_scripts", + issues: [ + { + path: ["capture_path"], + reason: "invalid_value", + message: expect.stringContaining("regular file"), + }, + ], + }, + }, + }); + expect(JSON.stringify(outcome.result)).toContain(capturePath); + await expect(access(outputDirectory)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, +); diff --git a/tests/fixtures/fifoInput.ts b/tests/fixtures/fifoInput.ts new file mode 100644 index 0000000000..f5b81c17bf --- /dev/null +++ b/tests/fixtures/fifoInput.ts @@ -0,0 +1,36 @@ +import { constants } from "node:fs"; +import { open } from "node:fs/promises"; + +type FifoReadOutcome = + | { state: "completed"; result: T } + | { state: "timed-out" }; + +/** Bound a writer-free FIFO regression and release a blocking reader on failure. */ +export const readWithoutFifoWriter = async ( + fifoPath: string, + read: () => Promise, +): Promise> => { + const pending = read(); + let timeout: NodeJS.Timeout | undefined; + try { + return await Promise.race([ + pending.then((result): FifoReadOutcome => ({ + state: "completed", + result, + })), + new Promise>((resolve) => { + timeout = setTimeout(() => resolve({ state: "timed-out" }), 1_000); + }), + ]); + } finally { + if (timeout !== undefined) clearTimeout(timeout); + // Release a blocking readFile open and await its EOF and cleanup. + // O_RDWR also completes when the fixed reader has already rejected. + const writer = await open( + fifoPath, + constants.O_RDWR | constants.O_NONBLOCK, + ); + await writer.close(); + await pending; + } +};