From b5dba08dba8a22994172f2bc8782c6832c91d53b Mon Sep 17 00:00:00 2001 From: Muhammad Hamza <32600505+mhamzasakhi@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:42:33 +0500 Subject: [PATCH] fix(web): refuse an existing script-export destination as invalid input (#6313) * fix(web): refuse an existing script-export destination as invalid input export_web_scripts refused an existing output directory or link correctly, but reported it as artifact_operation_failed / execution_failure with no field issue or recovery. SafeOutputTree.create raises that refusal as a path failure before any output tree exists, and the export mapper wrapped it as an artifact failure. Attribute pre-admission path refusals to output_directory as an AnalysisInputError, as environment export already does for its directory. Both now share one helper owned beside SafeOutputTree; failures after the tree is admitted keep their artifact semantics. Fixes #6029 * fix(artifacts): type SafeOutputTree destination refusals Admission conversion treated any cleanup-free path failure raised before admission as a refusal of the selected directory, so an acquired-tree setup failure, such as being unable to restrict a created root to 0700, blamed valid output_directory or directory input. SafeOutputTree.create now raises SafeOutputTreeDestinationRefusal for the refusals of the selection itself: a relative or reserved root, an existing root, and an accepted existing root that is not an empty directory. The shared conversion accepts only that type, directly or wrapped by SafeOutputTreeCreationFailure, so script and environment export no longer track admission separately. --- .../environment/environmentBundleWrite.ts | 31 ++----- src/artifacts/SafeOutputTree.ts | 30 +++++-- src/artifacts/SafeOutputTreeAdmission.test.ts | 83 +++++++++++++++++++ src/artifacts/SafeOutputTreeAdmission.ts | 43 ++++++++++ src/browser/assets/PublishWebScripts.ts | 9 +- .../cli/webScriptSelectionCli.test.ts | 53 +++++++++++- .../filesystem/webScriptExport.test.ts | 5 ++ .../mcp/webScriptSelectionMcp.test.ts | 58 ++++++++++++- 8 files changed, 277 insertions(+), 35 deletions(-) create mode 100644 src/artifacts/SafeOutputTreeAdmission.test.ts create mode 100644 src/artifacts/SafeOutputTreeAdmission.ts diff --git a/src/application/environment/environmentBundleWrite.ts b/src/application/environment/environmentBundleWrite.ts index 47f225e8a0..0f5c6cc2ed 100644 --- a/src/application/environment/environmentBundleWrite.ts +++ b/src/application/environment/environmentBundleWrite.ts @@ -15,6 +15,7 @@ import { streamChunkToBuffer } from "../../artifacts/StreamBytes.js"; import { ArtifactReaderFailure } from "../../artifacts/ArtifactReader.js"; import { ArtifactResourceScope } from "../../artifacts/ArtifactResourceScope.js"; import { SafeOutputTree } from "../../artifacts/SafeOutputTree.js"; +import { outputDirectoryAdmissionRefusal } from "../../artifacts/SafeOutputTreeAdmission.js"; import { SafeOutputTreeCreationFailure } from "../../artifacts/SafeOutputTreeCreationFailure.js"; import { RegularFileChangedError } from "../../filesystem/RegularFile.js"; import { withRegularFile } from "../RegularFileRead.js"; @@ -125,27 +126,6 @@ const writeTarball = async ( return { path: written.relativePath, bytes: written.bytesWritten }; }; -/** - * Only destination admission can blame the selected directory. Path failures - * after admission are host write failures on bundle files, not argument refusals. - */ -const directoryRefusal = (cause: unknown): unknown => - cause instanceof ArtifactReaderFailure && - cause.reason === "path" && - cause.cleanup === undefined - ? new AnalysisInputError( - ENVIRONMENT_EXPORT_OPERATION, - { cause, userMessage: cause.message }, - [ - { - path: ["directory"], - reason: "invalid_value", - message: cause.message, - }, - ], - ) - : cause; - /** The output owner anchors every create and verifies the complete tree before publication. */ const writeBundleFiles = async ( input: EnvironmentExportInput, @@ -155,7 +135,6 @@ const writeBundleFiles = async ( const resources = new ArtifactResourceScope(); let tree: SafeOutputTree | undefined; let written: WrittenBundleFile[] | undefined; - let admitted = false; let outcome: | { readonly kind: "completed"; @@ -172,7 +151,6 @@ const writeBundleFiles = async ( if (cause instanceof SafeOutputTreeCreationFailure) tree = cause.tree; throw cause; } - admitted = true; written = []; for (const file of plan.files) written.push( @@ -220,7 +198,12 @@ const writeBundleFiles = async ( } if (outcome.kind === "failed") { if (owner?.published === true && written !== undefined) return written; - throw admitted ? outcome.cause : directoryRefusal(outcome.cause); + // Only a destination refusal can blame the selected directory. + throw outputDirectoryAdmissionRefusal( + ENVIRONMENT_EXPORT_OPERATION, + "directory", + outcome.cause, + ); } return outcome.files; }; diff --git a/src/artifacts/SafeOutputTree.ts b/src/artifacts/SafeOutputTree.ts index 3141dc00fb..d113c42f2b 100644 --- a/src/artifacts/SafeOutputTree.ts +++ b/src/artifacts/SafeOutputTree.ts @@ -20,6 +20,7 @@ import { } from "./ArtifactPaths.js"; import { absoluteCallerPath } from "../filesystem/callerPath.js"; import { ArtifactReaderFailure } from "./ArtifactReader.js"; +import { SafeOutputTreeDestinationRefusal } from "./SafeOutputTreeAdmission.js"; import { SafeOutputTreeCreationFailure } from "./SafeOutputTreeCreationFailure.js"; import { hashOutputFile } from "./SafeOutputTreeReadback.js"; import { removeOwnedTree } from "./SafeOutputTreeCleanup.js"; @@ -95,15 +96,16 @@ export class SafeOutputTree { options: { readonly acceptExistingEmpty?: boolean } = {}, ): Promise { if (!isAbsolute(outputRoot)) - throw new ArtifactReaderFailure( - "path", + throw new SafeOutputTreeDestinationRefusal( "Extraction output root must be absolute", ); // Path syntax follows the host; `platform` selects only the name policy. const requested = absoluteCallerPath(outputRoot); const name = basename(requested); if (name === "." || name === "..") - throw new ArtifactReaderFailure("path", "Invalid extraction output root"); + throw new SafeOutputTreeDestinationRefusal( + "Invalid extraction output root", + ); const parent = await realpath(dirname(requested)).catch( (cause: unknown) => { throw new ArtifactReaderFailure( @@ -121,8 +123,7 @@ export class SafeOutputTree { created = false; return; } - throw new ArtifactReaderFailure( - "path", + throw new SafeOutputTreeDestinationRefusal( "Extraction output root already exists", { cause }, ); @@ -130,11 +131,24 @@ export class SafeOutputTree { const tree = new SafeOutputTree(canonicalOutput, platform, requested); tree.#createdRoot = created; try { - const rootIdentity = await readDirectoryIdentity(canonicalOutput); + const rootIdentity = await readDirectoryIdentity(canonicalOutput).catch( + (cause: unknown) => { + // An accepted existing root that is not a directory refuses the + // selection itself; a root this call created was replaced by a host race. + if ( + !created && + cause instanceof ArtifactReaderFailure && + cause.reason === "path" + ) + throw new SafeOutputTreeDestinationRefusal(cause.message, { + cause, + }); + throw cause; + }, + ); tree.#rootIdentity = rootIdentity; if (!created && (await readdir(canonicalOutput)).length > 0) - throw new ArtifactReaderFailure( - "path", + throw new SafeOutputTreeDestinationRefusal( "Output directory is not empty", ); if (created) diff --git a/src/artifacts/SafeOutputTreeAdmission.test.ts b/src/artifacts/SafeOutputTreeAdmission.test.ts new file mode 100644 index 0000000000..ac3d2a5a93 --- /dev/null +++ b/src/artifacts/SafeOutputTreeAdmission.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it } from "vitest"; + +import { AnalysisInputError } from "../domain/analysisErrorCore.js"; +import { ArtifactReaderFailure } from "./ArtifactReader.js"; +import type { SafeOutputTree } from "./SafeOutputTree.js"; +import { + outputDirectoryAdmissionRefusal, + SafeOutputTreeDestinationRefusal, +} from "./SafeOutputTreeAdmission.js"; +import { SafeOutputTreeCreationFailure } from "./SafeOutputTreeCreationFailure.js"; + +// Only the failure's type matters here; no tree method is called. +const acquiredTree = {} as SafeOutputTree; +const refuse = (cause: unknown) => + outputDirectoryAdmissionRefusal( + "export_web_scripts", + "output_directory", + cause, + ); + +describe("output directory admission refusal", () => { + it.each([ + [ + "a refusal before acquisition", + new SafeOutputTreeDestinationRefusal( + "Extraction output root already exists", + ), + ], + [ + "a refusal of an acquired non-empty root", + new SafeOutputTreeCreationFailure( + new SafeOutputTreeDestinationRefusal("Output directory is not empty"), + acquiredTree, + ), + ], + ])("blames the selected directory for %s", (_name, cause) => { + const refused = refuse(cause); + + expect(refused).toBeInstanceOf(AnalysisInputError); + expect(refused).toMatchObject({ + operation: "export_web_scripts", + issues: [ + { + path: ["output_directory"], + reason: "invalid_value", + message: cause.message, + }, + ], + }); + }); + + it.each([ + [ + "an acquired-tree setup failure", + new SafeOutputTreeCreationFailure( + new ArtifactReaderFailure( + "path", + "Cannot restrict output root to 0700", + ), + acquiredTree, + ), + ], + [ + "a path failure that is not a destination refusal", + new ArtifactReaderFailure("path", "Output file path changed"), + ], + [ + "a refusal whose cleanup is unresolved", + new ArtifactReaderFailure( + "path", + "Extraction output root already exists", + { + cause: new SafeOutputTreeDestinationRefusal( + "Extraction output root already exists", + ), + cleanup: { reason: "rollback failed", resources: ["/out"] }, + }, + ), + ], + ])("keeps artifact semantics for %s", (_name, cause) => { + expect(refuse(cause)).toBe(cause); + }); +}); diff --git a/src/artifacts/SafeOutputTreeAdmission.ts b/src/artifacts/SafeOutputTreeAdmission.ts new file mode 100644 index 0000000000..5c3976f4d8 --- /dev/null +++ b/src/artifacts/SafeOutputTreeAdmission.ts @@ -0,0 +1,43 @@ +import { AnalysisInputError } from "../domain/analysisErrorCore.js"; +import { ArtifactReaderFailure } from "./ArtifactReader.js"; + +/** + * `SafeOutputTree.create` refused the caller's selected destination itself: + * a relative or reserved root, an existing root, or an accepted existing root + * that is not an empty directory. + * Setup failures after a tree is acquired are not refusals of the selection. + */ +export class SafeOutputTreeDestinationRefusal extends ArtifactReaderFailure { + constructor(message: string, options?: { readonly cause?: unknown }) { + super("path", message, options); + this.name = "SafeOutputTreeDestinationRefusal"; + } +} + +/** + * Attribute a destination refusal to the caller's selected directory argument. + * A refusal raised after tree acquisition, such as a non-empty accepted root, + * arrives wrapped by `SafeOutputTreeCreationFailure`; every other failure, + * including acquired-tree setup failures, keeps its artifact semantics. + */ +export const outputDirectoryAdmissionRefusal = ( + operation: string, + field: string, + cause: unknown, +): unknown => { + if (!(cause instanceof ArtifactReaderFailure) || cause.cleanup !== undefined) + return cause; + const refusal = + cause instanceof SafeOutputTreeDestinationRefusal + ? cause + : cause.cause instanceof SafeOutputTreeDestinationRefusal + ? cause.cause + : undefined; + return refusal === undefined + ? cause + : new AnalysisInputError( + operation, + { cause, userMessage: refusal.message }, + [{ path: [field], reason: "invalid_value", message: refusal.message }], + ); +}; diff --git a/src/browser/assets/PublishWebScripts.ts b/src/browser/assets/PublishWebScripts.ts index f264776812..2a56d64d18 100644 --- a/src/browser/assets/PublishWebScripts.ts +++ b/src/browser/assets/PublishWebScripts.ts @@ -6,6 +6,7 @@ import { ArtifactReaderFailure } from "../../artifacts/ArtifactReader.js"; import { ArtifactResourceScope } from "../../artifacts/ArtifactResourceScope.js"; import { DestinationCaseCollisionFailure } from "../../artifacts/ArtifactPaths.js"; import { SafeOutputTree } from "../../artifacts/SafeOutputTree.js"; +import { outputDirectoryAdmissionRefusal } from "../../artifacts/SafeOutputTreeAdmission.js"; import { SafeOutputTreeCreationFailure } from "../../artifacts/SafeOutputTreeCreationFailure.js"; import { PLAYWRIGHT_RESPONSE_BODY_PROVENANCE_LIMITATION } from "../../domain/browserNetworkEvidence.js"; import { AnalysisCancelledError } from "../../domain/analysisErrorCore.js"; @@ -175,7 +176,13 @@ export const publishWebScripts = async ( if (tree.published && result !== undefined) return result; } - throw cancellation ?? cause; + if (cancellation !== undefined) throw cancellation; + // Only a destination refusal can blame the selected output directory. + throw outputDirectoryAdmissionRefusal( + OPERATION, + "output_directory", + cause, + ); } }); diff --git a/tests/boundary/cli/webScriptSelectionCli.test.ts b/tests/boundary/cli/webScriptSelectionCli.test.ts index e2d60e093d..48abd4830d 100644 --- a/tests/boundary/cli/webScriptSelectionCli.test.ts +++ b/tests/boundary/cli/webScriptSelectionCli.test.ts @@ -1,4 +1,4 @@ -import { access } from "node:fs/promises"; +import { access, mkdir, readdir, symlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; import pino from "pino"; @@ -10,6 +10,7 @@ import { CLI_COMMANDS } from "../../../src/cliCommandNames.js"; import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js"; import { err } from "../../../src/domain/result.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { scriptScenarioFixture } from "../../fixtures/webScriptCapture.js"; import { cliTest } from "../../support/cli/cliFixture.js"; it("forwards an interrupt into the export workflow and reports cancellation", async () => { @@ -89,3 +90,53 @@ cliTest.skipIf(process.platform === "win32")( }); }, ); + +cliTest( + "refuses an existing output directory or link as invalid output_directory input", + async ({ cli }) => { + const root = await createTestTempDirectory("rea-script-export-existing-"); + const capturePath = join(root, "capture.json"); + await writeFile(capturePath, JSON.stringify(scriptScenarioFixture())); + const retained = join(root, "retained"); + await mkdir(retained); + await writeFile(join(retained, "marker.txt"), "keep"); + const link = join(root, "output-link"); + await symlink( + retained, + link, + process.platform === "win32" ? "junction" : "dir", + ); + + for (const outputDirectory of [retained, link]) { + const response = await cli.run({ + arguments: [ + "export-web-scripts", + capturePath, + outputDirectory, + "--json", + ], + environment: { REA_LOG_LEVEL: "silent" }, + }); + expect(response.exitCode).toBe(1); + expect(response.json).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + recovery: { + kind: "inspect_input_contract", + operation: "export_web_scripts", + }, + details: { + operation: "export_web_scripts", + issues: [ + { + path: ["output_directory"], + reason: "invalid_value", + message: "Extraction output root already exists", + }, + ], + }, + }); + } + expect(await readdir(retained)).toEqual(["marker.txt"]); + }, +); diff --git a/tests/boundary/filesystem/webScriptExport.test.ts b/tests/boundary/filesystem/webScriptExport.test.ts index ecb173287c..74e08c6fce 100644 --- a/tests/boundary/filesystem/webScriptExport.test.ts +++ b/tests/boundary/filesystem/webScriptExport.test.ts @@ -312,6 +312,11 @@ describe("captured script publication failures and cleanup", () => { ); const result = await exportWebScripts(input, resources); if (result.ok) throw new Error("Expected exclusive output failure"); + expect(result.error).toMatchObject({ + _tag: "AnalysisInputError", + operation: "export_web_scripts", + issues: [{ path: ["output_directory"], reason: "invalid_value" }], + }); expect(result.error.userMessage).toContain("already exists"); expect(await readFile(marker, "utf8")).toBe("keep"); }); diff --git a/tests/boundary/mcp/webScriptSelectionMcp.test.ts b/tests/boundary/mcp/webScriptSelectionMcp.test.ts index 381c1dcb25..7b75e9d49e 100644 --- a/tests/boundary/mcp/webScriptSelectionMcp.test.ts +++ b/tests/boundary/mcp/webScriptSelectionMcp.test.ts @@ -1,5 +1,5 @@ import { execFile } from "node:child_process"; -import { access } from "node:fs/promises"; +import { access, mkdir, readdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { promisify } from "node:util"; @@ -15,6 +15,7 @@ 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"; +import { scriptScenarioFixture } from "../../fixtures/webScriptCapture.js"; it.skipIf(process.platform === "win32")( "returns a capture_path invalid_request for a named pipe without waiting or publishing output", @@ -80,3 +81,58 @@ it.skipIf(process.platform === "win32")( }); }, ); + +it("refuses an existing output directory as invalid output_directory input", async () => { + const root = await createTestTempDirectory("rea-script-export-existing-mcp-"); + const capturePath = join(root, "capture.json"); + await writeFile(capturePath, JSON.stringify(scriptScenarioFixture())); + const outputDirectory = join(root, "retained"); + await mkdir(outputDirectory); + await writeFile(join(outputDirectory, "marker.txt"), "keep"); + const server = new EvidenceMcpServer( + { name: "script-export-existing", version: "1" }, + {}, + undefined, + new ToolResultDelivery(STDIO_DEFAULT_MAX_BUFFER_SIZE), + ); + const artifactResources = new ArtifactResourceScope(); + registerWebScriptTool(server, { + logger: silentLogger, + recordEvidence: undefined, + artifactResources, + }); + const client = new Client({ name: "script-export-existing", version: "1" }); + onTestFinished(async () => { + await client.close(); + await server.close(); + await artifactResources.close(); + }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + const result = await client.callTool({ + name: "export_web_scripts", + arguments: { capture_path: capturePath, output_directory: outputDirectory }, + }); + expect(parseMcpToolError(result).error).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + recovery: { + kind: "inspect_input_contract", + operation: "export_web_scripts", + }, + details: { + operation: "export_web_scripts", + issues: [ + { + path: ["output_directory"], + reason: "invalid_value", + message: "Extraction output root already exists", + }, + ], + }, + }); + expect(await readdir(outputDirectory)).toEqual(["marker.txt"]); +});