From 5e1d504664362649aec19c03f768d0bbf939b482 Mon Sep 17 00:00:00 2001 From: gensokyo-dev Date: Sat, 14 Nov 2026 09:43:22 +0000 Subject: [PATCH] fix(environment): admit the output root before creating its parents environment-build/export ran an unclassified recursive mkdir of the destination's parent before SafeOutputTree validated anything. A trailing `..` therefore created directories before it was refused, and a regular file in the parent path surfaced as unreadable_output instead of refusing the selection. SafeOutputTree.create gains a createParents option that admits the root's own syntax first, then creates the parents, mapping EEXIST and ENOTDIR to a destination refusal. The environment bundle writer uses it instead of its own mkdir. Fixes #7688 Fixes #7571 The non-directory mapping comes from #7734 by HeadmasterEggy, and the syntax-first check from #7588 by harshitethic; their tests are kept. Co-Authored-By: Avocado Co-Authored-By: Harshit Sharma <66710144+harshitethic@users.noreply.github.com> Co-Authored-By: gensokyo-dev <342580529+gensokyo-dev@users.noreply.github.com> --- .../environment/environmentBuild.test.ts | 13 +++ .../environment/environmentBundleWrite.ts | 7 +- .../environment/environmentExport.test.ts | 9 ++ src/artifacts/SafeOutputTree.ts | 54 ++++++++-- .../boundary/cli/environmentBuildCli.test.ts | 20 ++++ .../environmentBundleDestinationCli.test.ts | 99 +++++++++++++++++++ 6 files changed, 189 insertions(+), 13 deletions(-) create mode 100644 tests/boundary/cli/environmentBundleDestinationCli.test.ts diff --git a/src/application/environment/environmentBuild.test.ts b/src/application/environment/environmentBuild.test.ts index 5cabed56d2..d5be921e09 100644 --- a/src/application/environment/environmentBuild.test.ts +++ b/src/application/environment/environmentBuild.test.ts @@ -332,6 +332,19 @@ describe("buildEnvironment naming and refusals", () => { }); }); + it("does not create parents when the selected build root ends with dot-dot", async () => { + const root = await createTestTempDirectory( + "rea-environment-build-invalid-destination-", + ); + const parent = join(root, "untouched"); + const directory = `${join(parent, "a", "b", "c")}/..`; + const result = await buildEnvironment( + await buildInput(root, undefined, { directory }), + ); + expect(result.ok).toBe(false); + await expect(readdir(parent)).rejects.toMatchObject({ code: "ENOENT" }); + }); + it("refuses an occupied directory and leaves it as it was", async () => { const root = await createTestTempDirectory("rea-environment-build-busy-"); const input = await buildInput(root); diff --git a/src/application/environment/environmentBundleWrite.ts b/src/application/environment/environmentBundleWrite.ts index 16bf0f2210..e9d640ab38 100644 --- a/src/application/environment/environmentBundleWrite.ts +++ b/src/application/environment/environmentBundleWrite.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; -import { mkdir, type FileHandle } from "node:fs/promises"; -import { dirname, join } from "node:path"; +import type { FileHandle } from "node:fs/promises"; +import { join } from "node:path"; import { Readable } from "node:stream"; import { @@ -168,11 +168,10 @@ const writeBundleFiles = async ( } | { readonly kind: "failed"; readonly cause: unknown }; try { - if (input.exclusiveDestination !== true) - await mkdir(dirname(input.directory), { recursive: true, mode: 0o700 }); try { tree = await SafeOutputTree.create(input.directory, process.platform, { acceptExistingEmpty: input.exclusiveDestination !== true, + createParents: input.exclusiveDestination !== true, }); } catch (cause: unknown) { if (cause instanceof SafeOutputTreeCreationFailure) tree = cause.tree; diff --git a/src/application/environment/environmentExport.test.ts b/src/application/environment/environmentExport.test.ts index 1691944ab5..e6c84f6371 100644 --- a/src/application/environment/environmentExport.test.ts +++ b/src/application/environment/environmentExport.test.ts @@ -120,6 +120,15 @@ describe("readEnvironmentExportInputs", () => { }); }); +it("does not create parents for a rejected trailing dot-dot output selection", async () => { + const { root, input } = await prepare("rea-environment-export-invalid-root-"); + const parent = join(root, "untouched"); + const directory = `${join(parent, "a", "b", "c")}/..`; + const result = await exportEnvironmentBundle({ ...input, directory }); + expect(result.ok).toBe(false); + await expect(readdir(parent)).rejects.toMatchObject({ code: "ENOENT" }); +}); + const registerBundlePublicationCases = () => { it("writes the bundle into an absent directory with the read bytes copied unchanged", async () => { const { root, files, input, directory } = await prepare( diff --git a/src/artifacts/SafeOutputTree.ts b/src/artifacts/SafeOutputTree.ts index bf06c7e32b..db3a93c393 100644 --- a/src/artifacts/SafeOutputTree.ts +++ b/src/artifacts/SafeOutputTree.ts @@ -57,6 +57,44 @@ export type SafeOutputCleanup = readonly residualPaths: readonly [string, ...string[]]; }; +/** Admit an output root's own syntax before any filesystem effect. */ +const admitOutputRootSyntax = (outputRoot: string): string => { + if (!isAbsolute(outputRoot)) + throw new SafeOutputTreeDestinationRefusal("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 SafeOutputTreeDestinationRefusal("Invalid output directory"); + return requested; +}; + +/** + * Create a root's missing parents only after its syntax is admitted, so a + * refused selection leaves no directories behind. A file in the parent path + * refuses the selection instead of surfacing as a host write failure. + */ +const createOutputParents = async (outputRoot: string): Promise => { + const requested = admitOutputRootSyntax(outputRoot); + await mkdir(dirname(requested), { recursive: true, mode: 0o700 }).catch( + (cause: unknown) => { + const code = + cause instanceof Error && "code" in cause ? String(cause.code) : ""; + if (code === "EEXIST" || code === "ENOTDIR") + throw new SafeOutputTreeDestinationRefusal( + "Output parent must be a directory", + { cause }, + ); + if (code === "EACCES" || code === "EPERM") + throw new SafeOutputTreeDestinationRefusal( + "Output parent is unavailable", + { cause }, + ); + throw cause; + }, + ); +}; + /** * Materialize files in a tree created by this operation. * @@ -98,8 +136,13 @@ export class SafeOutputTree { static async create( outputRoot: string, platform: NodeJS.Platform = process.platform, - options: { readonly acceptExistingEmpty?: boolean } = {}, + options: { + readonly acceptExistingEmpty?: boolean; + /** Create missing parents, after the root's own syntax is admitted. */ + readonly createParents?: boolean; + } = {}, ): Promise { + if (options.createParents === true) await createOutputParents(outputRoot); const { requested, canonicalOutput } = await this.resolveDestination(outputRoot); let created = true; @@ -183,15 +226,8 @@ export class SafeOutputTree { /** Resolve an admissible parent without acquiring output; create rechecks it. */ static async resolveDestination(outputRoot: string) { - if (!isAbsolute(outputRoot)) - throw new SafeOutputTreeDestinationRefusal( - "Output root must be absolute", - ); - // Path syntax follows the host; `platform` selects only the name policy. - const requested = absoluteCallerPath(outputRoot); + const requested = admitOutputRootSyntax(outputRoot); const name = basename(requested); - if (name === "." || name === "..") - throw new SafeOutputTreeDestinationRefusal("Invalid output directory"); let parent: string; try { const selectedParent = dirname(requested); diff --git a/tests/boundary/cli/environmentBuildCli.test.ts b/tests/boundary/cli/environmentBuildCli.test.ts index 5c3482c568..65b890f24d 100644 --- a/tests/boundary/cli/environmentBuildCli.test.ts +++ b/tests/boundary/cli/environmentBuildCli.test.ts @@ -101,6 +101,26 @@ cliTest( expect(projected.exitCode, projected.stderr).toBe(0); const manifest = join(root, "manifest.json"); await writeFile(manifest, projected.stdout); + for (const command of [ + "environment-build", + "environment-export", + ] as const) { + const parent = join(root, `${command}-refused`); + await mkdir(parent); + // Do not use join for the last segment: the caller's spelling matters. + const destination = `${join(parent, "a", "b", "c")}${sep}..`; + const args = + command === "environment-build" + ? [command, har, destination, "--json"] + : [command, manifest, "--evidence", capture, destination, "--json"]; + const refused = await cli.run({ arguments: args }); + expect(refused.exitCode, refused.stderr).toBe(1); + expect(refused.json).toMatchObject({ + code: "invalid_request", + details: { issues: [{ path: ["directory"] }] }, + }); + expect(await readdir(parent)).toEqual([]); + } const exported = await cli.run({ arguments: [ "environment-export", diff --git a/tests/boundary/cli/environmentBundleDestinationCli.test.ts b/tests/boundary/cli/environmentBundleDestinationCli.test.ts new file mode 100644 index 0000000000..ea59edf08f --- /dev/null +++ b/tests/boundary/cli/environmentBundleDestinationCli.test.ts @@ -0,0 +1,99 @@ +import { mkdir, readFile, readdir, stat, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect } from "vitest"; +import { writeEnvironmentServeFiles } from "../../fixtures/environmentServe.js"; +import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest } from "../../support/cli/cliFixture.js"; + +cliTest.for(["environment-export", "environment-build"] as const)( + "%s attributes non-directory parents to the selected destination", + async (command, { cli }) => { + const root = await createTestTempDirectory("rea-bundle-parent-"); + const files = await writeEnvironmentServeFiles(root); + const occupied = join(root, "parentfile"); + await writeFile(occupied, "keep\n"); + const before = await stat(occupied); + const entries = await readdir(root); + const call = await cli.sharedMcpCall?.(); + if (call === undefined) throw new Error("Expected stdio MCP session"); + for (const directory of [ + join(occupied, "bundle"), + join(occupied, "missing", "bundle"), + ]) { + const arguments_ = + command === "environment-export" + ? [command, files.manifestPath, "--evidence", files.capturePath] + : [command, files.capturePath]; + const result = await cli.run({ + arguments: [...arguments_, directory, "--json"], + }); + expect(result.exitCode, result.stderr).toBe(1); + expect(result.json).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + details: { + issues: [{ path: ["directory"], reason: "invalid_value" }], + }, + recovery: { + kind: "inspect_input_contract", + operation: "environment-export", + }, + }); + const mcp = await call("export_environment_bundle", { + source: { kind: "evidence-file", path: files.capturePath }, + output_directory: directory, + }); + expect(mcp.isError).toBe(true); + expect(parseMcpToolError(mcp).error).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + details: { + issues: [{ path: ["output_directory"], reason: "invalid_value" }], + }, + recovery: { + kind: "inspect_input_contract", + operation: "export_environment_bundle", + }, + }); + expect(await readFile(occupied, "utf8")).toBe("keep\n"); + const after = await stat(occupied); + expect({ dev: after.dev, ino: after.ino }).toEqual({ + dev: before.dev, + ino: before.ino, + }); + expect(await readdir(root)).toEqual(entries); + } + }, +); + +cliTest.for(["environment-export", "environment-build"] as const)( + "%s still creates missing parents and accepts an empty destination", + async (command, { cli }) => { + const root = await createTestTempDirectory("rea-bundle-parent-control-"); + const files = await writeEnvironmentServeFiles(root); + const empty = join(root, "empty"); + await mkdir(empty); + for (const directory of [ + join(root, "missing", "nested", "bundle"), + empty, + ]) { + const arguments_ = + command === "environment-export" + ? [command, files.manifestPath, "--evidence", files.capturePath] + : [command, files.capturePath]; + const result = await cli.run({ + arguments: [...arguments_, directory, "--json"], + }); + expect(result.exitCode, result.stderr).toBe(0); + expect(result.json).toMatchObject({ + status: command === "environment-export" ? "exported" : "built", + }); + expect( + ( + await readFile(join(directory, "envs/grafana-example/evidence.json")) + ).equals(await readFile(files.capturePath)), + ).toBe(true); + } + }, +);