fix(artifacts): report which path constraint failed (#847)
Artifact path failures told callers to "inspect the reported path" but
reported none: `ArtifactReaderFailure` messages such as
`Artifact path collision: ...` were dropped when converted to
`ArtifactOperationError`, leaving only `{ operation, reason: "path" }`.
Carry the failure message into `details.detail` for artifact tools and
JavaScript application analysis. Name both spellings in case-fold
collisions and include the JSON-escaped input in unsafe and
non-normalized path messages.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
880cca1bca
commit
3f1a77f68e
|
|
@ -75,7 +75,12 @@ export const analyzeJavaScriptApplicationValidated = async (
|
|||
} catch (cause: unknown) {
|
||||
if (cause instanceof ArtifactReaderFailure)
|
||||
return err(
|
||||
new ArtifactOperationError(OPERATION, cause.reason, cause.details),
|
||||
new ArtifactOperationError(
|
||||
OPERATION,
|
||||
cause.reason,
|
||||
cause.details,
|
||||
cause.message,
|
||||
),
|
||||
);
|
||||
if (cause instanceof z.ZodError)
|
||||
return err(
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ export const normalizeArtifactPath = (input: string): string => {
|
|||
)
|
||||
throw new ArtifactReaderFailure(
|
||||
"path",
|
||||
"Artifact path is absolute or unsafe",
|
||||
`Artifact path is absolute or unsafe: ${JSON.stringify(input)}`,
|
||||
);
|
||||
const normalized = input.normalize("NFC").replace(/\/+$/u, "");
|
||||
const parts = normalized.split("/");
|
||||
|
|
@ -23,7 +23,10 @@ export const normalizeArtifactPath = (input: string): string => {
|
|||
parts.some((part) => part === "" || part === "." || part === "..") ||
|
||||
posix.normalize(normalized) !== normalized
|
||||
)
|
||||
throw new ArtifactReaderFailure("path", "Artifact path is not normalized");
|
||||
throw new ArtifactReaderFailure(
|
||||
"path",
|
||||
`Artifact path is not normalized: ${JSON.stringify(input)}`,
|
||||
);
|
||||
return normalized;
|
||||
};
|
||||
|
||||
|
|
@ -45,7 +48,7 @@ export class ArtifactPathRegistry {
|
|||
if (child !== undefined && child.spelling !== part)
|
||||
throw new ArtifactReaderFailure(
|
||||
"path",
|
||||
`Artifact path collision: ${path}`,
|
||||
`Artifact path collision: ${path} differs only in case from ${[...parts.slice(0, index), child.spelling].join("/")}`,
|
||||
);
|
||||
if (child === undefined) {
|
||||
child = { spelling: part, kind: undefined, children: new Map() };
|
||||
|
|
|
|||
|
|
@ -343,7 +343,12 @@ const translateFailure = (
|
|||
cause: unknown,
|
||||
): AnalysisError => {
|
||||
if (cause instanceof ArtifactReaderFailure)
|
||||
return new ArtifactOperationError(operation, cause.reason, cause.details);
|
||||
return new ArtifactOperationError(
|
||||
operation,
|
||||
cause.reason,
|
||||
cause.details,
|
||||
cause.message,
|
||||
);
|
||||
return new ArtifactOperationError(operation, "io");
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -216,6 +216,7 @@ const artifactStateErrorDetails = (
|
|||
operation: error.operation,
|
||||
reason: error.reason,
|
||||
...(error.reason === "limit" ? { truncated: true } : {}),
|
||||
...(error.detail === undefined ? {} : { detail: error.detail }),
|
||||
};
|
||||
if (error instanceof UnknownRegistryError) return { reason: error.reason };
|
||||
if (error instanceof EvidenceFileError)
|
||||
|
|
|
|||
|
|
@ -28,6 +28,8 @@ export class ArtifactOperationError extends AnalysisError {
|
|||
calculatedSha256: string | null;
|
||||
unpacked: boolean;
|
||||
}>,
|
||||
/** The specific constraint that failed, such as the colliding path. */
|
||||
readonly detail?: string,
|
||||
) {
|
||||
super(
|
||||
artifactDetails === undefined
|
||||
|
|
|
|||
|
|
@ -96,7 +96,7 @@ describe("artifact archive safety", () => {
|
|||
expect(unsafePathError).toBeInstanceOf(ArtifactReaderFailure);
|
||||
expect(unsafePathError).toMatchObject({
|
||||
reason: "path",
|
||||
message: "Artifact path is not normalized",
|
||||
message: 'Artifact path is not normalized: "../escape"',
|
||||
});
|
||||
const registry = new ArtifactPathRegistry();
|
||||
registry.add("A.js", "file");
|
||||
|
|
@ -109,7 +109,7 @@ describe("artifact archive safety", () => {
|
|||
expect(collisionError).toBeInstanceOf(ArtifactReaderFailure);
|
||||
expect(collisionError).toMatchObject({
|
||||
reason: "path",
|
||||
message: "Artifact path collision: a.js",
|
||||
message: "Artifact path collision: a.js differs only in case from A.js",
|
||||
});
|
||||
|
||||
for (const [filePath, childPath] of [
|
||||
|
|
|
|||
|
|
@ -0,0 +1,35 @@
|
|||
import { join } from "node:path";
|
||||
|
||||
import { expect, it } from "vitest";
|
||||
|
||||
import { writeOrderedZip } from "../../../fixtures/artifactEntryOrder.js";
|
||||
import { createTestTempDirectory } from "../../../fixtures/temporaryDirectory.js";
|
||||
|
||||
import { parseBinaryTarget } from "../../../../src/application/BinaryTargetResolver.js";
|
||||
import { ArtifactProvider } from "../../../../src/artifacts/ArtifactProvider.js";
|
||||
import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js";
|
||||
|
||||
it.each([
|
||||
[
|
||||
["Main.js", "main.js"],
|
||||
"Artifact path collision: main.js differs only in case from Main.js",
|
||||
],
|
||||
[["a\\b.js"], 'Artifact path is absolute or unsafe: "a\\\\b.js"'],
|
||||
] as const)(
|
||||
"reports the failed path constraint for %j",
|
||||
async (entries, detail) => {
|
||||
const root = await createTestTempDirectory("rea-artifact-detail-");
|
||||
const path = join(root, "fixture.zip");
|
||||
await writeOrderedZip(path, entries);
|
||||
const target = await parseBinaryTarget(path);
|
||||
if (!target.ok) throw new Error("expected a ZIP target");
|
||||
const result = await new ArtifactProvider()
|
||||
.createClient(target.value)
|
||||
.execute("inventory_artifact", {});
|
||||
if (result.ok) throw new Error("expected the path constraint to fail");
|
||||
expect(projectAnalysisError(result.error)).toMatchObject({
|
||||
code: "artifact_operation_failed",
|
||||
details: { operation: "inventory_artifact", reason: "path", detail },
|
||||
});
|
||||
},
|
||||
);
|
||||
Loading…
Reference in New Issue