mirror of
https://github.com/morluto/rea.git
synced 2026-11-14 18:52:48 +00:00
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 <headmastereggy@gmail.com> Co-Authored-By: Harshit Sharma <66710144+harshitethic@users.noreply.github.com> Co-Authored-By: gensokyo-dev <342580529+gensokyo-dev@users.noreply.github.com>
This commit is contained in:
1 parent
15825208c8
commit
5e1d504664
6 files changed
+189
-13
No files matched your search
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<void> => {
|
||||
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<SafeOutputTree> {
|
||||
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);
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
},
|
||||
);
|
||||
Reference in new issue
Block a user