From 369cd3dfdc47709cc34d01c50f645a885c70ded7 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:36:42 +0800 Subject: [PATCH] fix(javascript): normalize package URLs before array selection (#1916) --- docs/javascript-artifact-reconstruction.md | 8 +- .../JavaScriptArtifactPathResolution.ts | 129 +++++++++++++---- .../packageImportAliasesCli.test.ts | 37 ++++- .../packageConditionalExports.test.ts | 45 ++++++ .../javascript/packageImportAliases.test.ts | 132 ++++++++++++++++++ 5 files changed, 318 insertions(+), 33 deletions(-) diff --git a/docs/javascript-artifact-reconstruction.md b/docs/javascript-artifact-reconstruction.md index 1ac08f88c..661244f82 100644 --- a/docs/javascript-artifact-reconstruction.md +++ b/docs/javascript-artifact-reconstruction.md @@ -482,8 +482,12 @@ and one percent-decoding pass. CommonJS relative paths and legacy package main fields retain literal filename punctuation. Exports targets must start with `./` and exclude `.`, `..`, and `node_modules` path segments, including their encoded forms, before URL normalization. Arrays skip invalid exports targets -in declared order; URL decoding and file lookup failures for a selected target -do not select a later entry. Rejected references retain the package metadata +in declared order, including URLs that escape the package root after normalization. +Selected package targets use the normalized URL path for lookup; tabs and +newlines in the URL text are removed by the URL parser. CommonJS package targets +reject encoded separators anywhere in the selected URL; ESM checks its pathname. +URL decoding and file lookup failures for a selected target do not select a +later entry. Rejected references retain the package metadata path, original target, failed constraint, and importing source location. HTML script references resolve to exact inventoried files after applying the diff --git a/src/application/javascript/JavaScriptArtifactPathResolution.ts b/src/application/javascript/JavaScriptArtifactPathResolution.ts index 9580424e4..94fc46645 100644 --- a/src/application/javascript/JavaScriptArtifactPathResolution.ts +++ b/src/application/javascript/JavaScriptArtifactPathResolution.ts @@ -74,6 +74,21 @@ const EXTENSIONS = [ ".node", ]; +// A private URL namespace for logical artifact paths, including root packages. +// Encode inventory segments so filename punctuation remains literal in the base. +const PACKAGE_ARTIFACT_ROOT_URL = new URL("file:///rea-artifact/"); +const packageMetadataUrl = (path: string): URL => + new URL( + path.split("/").map(encodeURIComponent).join("/"), + PACKAGE_ARTIFACT_ROOT_URL, + ); + +interface PackageFileTarget { + readonly value: string; + /** Normalized URL retained until loader-specific validation and file lookup. */ + readonly url: URL; +} + type CandidateResolution = | { readonly resolvedPath: string; @@ -313,7 +328,11 @@ const resolvePackageImportAlias = ( return unresolvedCandidate("external", [ `Wildcard package import aliases remain unresolved; package metadata ${metadata.path} has no exact imports key ${specifier}.`, ]); - const selected = selectPackageImportTarget(lookup.target, input.moduleKind); + const selected = selectPackageImportTarget( + lookup.target, + input.moduleKind, + metadata.path, + ); if (selected.status === "rejected") return unresolvedCandidate("rejected", [ `Package metadata ${metadata.path}: ${selected.limitation}`, @@ -334,14 +353,15 @@ const resolvePackageImportAlias = ( : `External-package import targets remain unresolved; package metadata ${metadata.path} maps ${specifier} to ${JSON.stringify(target.target)}.`, ]); } - return resolveExportTarget(input, metadata.path, selected.value, "imports"); + return resolveExportTarget(input, metadata.path, selected, "imports"); }; const selectPackageImportTarget = ( value: unknown, moduleKind: ResolveArtifactPathInput["moduleKind"], + packagePath: string, ): - | { readonly status: "value"; readonly value: string } + | ({ readonly status: "value" } & PackageFileTarget) | { readonly status: "non-relative"; readonly value: string } | { readonly status: "blocked"; readonly cause: PackageTargetBlockCause } | { readonly status: "unmatched"; readonly declared: readonly string[] } @@ -349,6 +369,7 @@ const selectPackageImportTarget = ( const selected = selectExportTarget( value, packageExportConditions(moduleKind), + packageMetadataUrl(packagePath), "imports", ); if (selected.kind === "invalid" || selected.kind === "invalid-config") @@ -362,7 +383,11 @@ const selectPackageImportTarget = ( return { status: "blocked", cause: selected.cause }; if (selected.kind === "non-relative") return { status: "non-relative", value: selected.value }; - return { status: "value", value: selected.value }; + return { + status: "value", + value: selected.value, + url: selected.url, + }; }; /** Self references use the nearest package scope before dependency lookup. */ @@ -430,7 +455,12 @@ const resolvePackageExports = ( subpath: string, rawExports: string | object, ): CandidateResolution => { - const exported = selectPackageExport(rawExports, subpath, input.moduleKind); + const exported = selectPackageExport( + rawExports, + subpath, + input.moduleKind, + packagePath, + ); if (exported.status === "rejected") return unresolvedCandidate("rejected", [ `Package metadata ${packagePath}: ${exported.limitation}`, @@ -443,25 +473,45 @@ const resolvePackageExports = ( return unresolvedCandidate("external", [ `Directory package metadata ${packagePath} blocks subpath ${subpath} with ${exported.cause === "null" ? "an explicit null exports target" : "an empty exports target array"}.`, ]); - return resolveExportTarget(input, packagePath, exported.value); + return resolveExportTarget(input, packagePath, exported); }; /** An exports or import target is a URL naming one exact file for both Node loaders. */ const resolveExportTarget = ( input: ResolveArtifactPathInput, packagePath: string, - target: string, + target: PackageFileTarget, mapKind: "exports" | "imports" = "exports", ): CandidateResolution => { - const rejected = rejectDeclaration(target); - if (rejected !== null) return rejected; - const decoded = decodeModuleUrlPath(stripQueryAndFragment(target)); - if (typeof decoded !== "string") return decoded; - const candidate = confineCandidate(relativeCandidate(packagePath, decoded)); - if (typeof candidate !== "string") return candidate; + const withTargetSource = ( + outcome: CandidateResolution, + ): CandidateResolution => ({ + ...outcome, + limitations: outcome.limitations.map( + (reason) => + `Package metadata ${packagePath}, ${mapKind} target ${JSON.stringify(target.value)}: ${reason}`, + ), + }); + // Node's CommonJS finalizer checks the serialized URL, including suffixes; + // its ESM finalizer checks only the pathname. Neither error retries arrays. + if (input.moduleKind === "require" && /%(?:2f|5c)/iu.test(target.url.href)) + return withTargetSource( + unresolvedCandidate("rejected", [ + "The selected package URL contains encoded separator bytes.", + ]), + ); + const encodedPath = target.url.pathname.slice( + PACKAGE_ARTIFACT_ROOT_URL.pathname.length, + ); + const rejected = rejectDeclaration(encodedPath); + if (rejected !== null) return withTargetSource(rejected); + const decoded = decodeModuleUrlPath(encodedPath); + if (typeof decoded !== "string") return withTargetSource(decoded); + const candidate = confineCandidate(decoded); + if (typeof candidate !== "string") return withTargetSource(candidate); return ( resolveFileCandidates(input, [candidate]) ?? - exactPackageTargetNotFound(candidate, mapKind) + withTargetSource(exactPackageTargetNotFound(candidate, mapKind)) ); }; @@ -776,7 +826,7 @@ const indexCandidates = (candidate: string): readonly string[] => type PackageTargetBlockCause = "null" | "empty-array"; type PackageExportOutcome = - | { readonly status: "value"; readonly value: string } + | ({ readonly status: "value" } & PackageFileTarget) | { readonly status: "rejected"; readonly limitation: string } | { readonly status: "blocked"; readonly cause: PackageTargetBlockCause } | { readonly status: "unmatched"; readonly declared: readonly string[] }; @@ -786,6 +836,7 @@ const selectPackageExport = ( value: unknown, subpath: string, moduleKind: ResolveArtifactPathInput["moduleKind"], + packagePath: string, ): PackageExportOutcome => { let target = value; let declared: string[] = []; @@ -812,6 +863,7 @@ const selectPackageExport = ( const selected = selectExportTarget( target, packageExportConditions(moduleKind), + packageMetadataUrl(packagePath), ); if (selected.kind === "invalid" || selected.kind === "invalid-config") return { status: "rejected", limitation: selected.limitation }; @@ -827,11 +879,15 @@ const selectPackageExport = ( status: "rejected", limitation: `The package exports target ${JSON.stringify(selected.value)} must start with "./".`, }; - return { status: "value", value: selected.value }; + return { + status: "value", + value: selected.value, + url: selected.url, + }; }; type ExportTargetOutcome = - | { readonly kind: "value"; readonly value: string } + | ({ readonly kind: "value" } & PackageFileTarget) | { readonly kind: "non-relative"; readonly value: string } | { readonly kind: "blocked"; readonly cause: PackageTargetBlockCause } | { readonly kind: "unmatched"; readonly declared: readonly string[] } @@ -866,6 +922,7 @@ const forbiddenExportSegment = (value: string): string | undefined => const selectStringPackageTarget = ( value: string, + packageUrl: URL, mapKind: "exports" | "imports", ): ExportTargetOutcome => { if (mapKind === "imports") { @@ -880,18 +937,33 @@ const selectStringPackageTarget = ( } else if (!value.startsWith("./")) return invalidExportTarget(value, 'must start with "./"', mapKind); const forbidden = forbiddenExportSegment(value); - return forbidden === undefined - ? { kind: "value", value } - : invalidExportTarget( - value, - `contains forbidden path segment ${JSON.stringify(forbidden)} before URL normalization`, - mapKind, - ); + if (forbidden !== undefined) + return invalidExportTarget( + value, + `contains forbidden path segment ${JSON.stringify(forbidden)} before URL normalization`, + mapKind, + ); + // Node constructs the URL before deciding whether this array entry is an + // invalid package target. Tabs/newlines and dot segments normalize here; + // percent decoding and existence checks belong after target selection. + const url = new URL(value, packageUrl); + if (!url.pathname.startsWith(new URL(".", packageUrl).pathname)) + return invalidExportTarget( + value, + "escapes the package root after URL normalization", + mapKind, + ); + return { + kind: "value", + value, + url, + }; }; const selectArrayPackageTarget = ( value: readonly unknown[], conditions: ReadonlySet, + packageUrl: URL, mapKind: "exports" | "imports", ): ExportTargetOutcome => { if (value.length === 0) return { kind: "blocked", cause: "empty-array" }; @@ -900,7 +972,7 @@ const selectArrayPackageTarget = ( | undefined; const declared = new Set(); for (const entry of value) { - const nested = selectExportTarget(entry, conditions, mapKind); + const nested = selectExportTarget(entry, conditions, packageUrl, mapKind); // Node's array fallback catches invalid targets, not invalid configuration. if (nested.kind === "invalid-config") return nested; if (nested.kind === "unmatched") { @@ -926,13 +998,14 @@ const selectArrayPackageTarget = ( const selectExportTarget = ( value: unknown, conditions: ReadonlySet, + packageUrl: URL, mapKind: "exports" | "imports" = "exports", ): ExportTargetOutcome => { if (typeof value === "string") - return selectStringPackageTarget(value, mapKind); + return selectStringPackageTarget(value, packageUrl, mapKind); if (value === null) return { kind: "blocked", cause: "null" }; if (Array.isArray(value)) - return selectArrayPackageTarget(value, conditions, mapKind); + return selectArrayPackageTarget(value, conditions, packageUrl, mapKind); if (typeof value !== "object") return invalidExportTarget( value, @@ -951,7 +1024,7 @@ const selectExportTarget = ( for (const [condition, target] of Object.entries(value)) { declared.add(condition); if (!conditions.has(condition)) continue; - const nested = selectExportTarget(target, conditions, mapKind); + const nested = selectExportTarget(target, conditions, packageUrl, mapKind); if (nested.kind !== "unmatched") return nested; for (const name of nested.declared) declared.add(name); } diff --git a/tests/acceptance/applications/packageImportAliasesCli.test.ts b/tests/acceptance/applications/packageImportAliasesCli.test.ts index 90927dfa1..7d3187a7f 100644 --- a/tests/acceptance/applications/packageImportAliasesCli.test.ts +++ b/tests/acceptance/applications/packageImportAliasesCli.test.ts @@ -13,6 +13,11 @@ import { cliTest } from "../../support/cli/cliFixture.js"; const literalAliases = ["#/x", "#x%2Fy", "#x%2Ey", "#x\\y", "#x\0y"]; const fallbackAliases = { + "#tab-escape": ["./.\t./outside.cjs", "./config.cjs"], + "#newline-escape": ["./.%\n2e/outside.cjs", "./config.cjs"], + "#normalized-url": "./con\tfig.cjs", + "#trimmed-url": "./config.cjs \t", + "#url-suffix": "./config.cjs?selected=one#fragment", "#inactive-array": { node: [{ browser: "./browser.cjs" }], default: "./config.cjs", @@ -30,29 +35,53 @@ const fallbackAliases = { }; const resolvedAliases = [...literalAliases, ...Object.keys(fallbackAliases)]; const refusedAliases = { - "#empty": { target: [], limitation: "an empty imports target array" }, + "#empty": { + target: [], + status: "external", + limitation: "an empty imports target array", + }, "#empty-active": { target: { node: [] }, + status: "external", limitation: "an empty imports target array", }, "#null-last": { target: [[], null], + status: "external", limitation: "an explicit null imports target", }, "#empty-last": { target: [null, []], + status: "external", limitation: "an empty imports target array", }, "#inactive": { target: [{ browser: "./browser.cjs" }], + status: "external", limitation: "it declares browser.", }, "#nested-inactive": { + status: "external", target: [ [{ browser: "./browser.cjs" }, { "rea-inactive": "./inactive.cjs" }], ], limitation: "it declares browser, rea-inactive.", }, + "#escape": { + target: "./.\t./outside.cjs", + status: "rejected", + limitation: "escapes the package root", + }, + "#encoded-separator": { + target: ["./con%2ffig.cjs", "./config.cjs"], + status: "rejected", + limitation: "separator bytes", + }, + "#missing-url": { + target: ["./miss\ting.cjs", "./config.cjs"], + status: "not-found", + limitation: JSON.stringify("./miss\ting.cjs"), + }, }; const allAliases = [...resolvedAliases, ...Object.keys(refusedAliases)]; @@ -168,7 +197,9 @@ cliTest.for(["CLI", "stdio MCP"] as const)( const imports = graph.edges.filter( ({ properties }) => properties.module_link_kind === "require", ); - for (const [specifier, { limitation }] of Object.entries(refusedAliases)) + for (const [specifier, { status, limitation }] of Object.entries( + refusedAliases, + )) for (const [moduleKind, sourcePath, source] of [ ["require", "main.cjs", mainSource], ["import", "main.mjs", moduleSource], @@ -179,7 +210,7 @@ cliTest.for(["CLI", "stdio MCP"] as const)( module_link_kind: moduleKind, specifier, resolved_path: null, - resolution_status: "external", + resolution_status: status, }), evidence: sourceEvidence(sourcePath, source, limitation), }), diff --git a/tests/boundary/javascript/packageConditionalExports.test.ts b/tests/boundary/javascript/packageConditionalExports.test.ts index b53fe8516..64b762072 100644 --- a/tests/boundary/javascript/packageConditionalExports.test.ts +++ b/tests/boundary/javascript/packageConditionalExports.test.ts @@ -741,6 +741,51 @@ describe("nested and array package export targets", () => { }, ); }); +describe("package export target URL normalization compared with Node", () => { + it.each([ + [["./.\t./outside.cjs", "./array.cjs"]], + [["./.%\n2e/outside.cjs", "./array.cjs"]], + [["./.\r./outside.cjs", "./array.cjs"]], + ["./arr\tay.cjs"], + ["./array.cjs \t"], + ["./array.cjs?selected=one#fragment"], + ["./.\t./example/array.cjs"], + ] as const)( + "resolves normalized target %j through both loaders", + async (target) => { + for (const moduleKind of ["import", "require"] as const) { + const { node, outcome } = await compareWithNode(target, moduleKind); + expect(node).toBe("array.cjs"); + expect(outcome).toMatchObject({ + resolution_status: "resolved", + resolved_path: "node_modules/example/array.cjs", + }); + } + }, + ); + + it.each([ + [["./arr%2fay.cjs", "./array.cjs"], "rejected"], + [["./miss\ting.cjs", "./array.cjs"], "not-found"], + ] as const)( + "retains the first selected target failure in %j", + async (target, status) => { + for (const moduleKind of ["import", "require"] as const) { + const { node, node_error, outcome } = await compareWithNode( + target, + moduleKind, + ); + expect(node).toBeNull(); + expect(node_error).not.toBeNull(); + expect(outcome).toMatchObject({ + resolution_status: status, + resolved_path: null, + }); + } + }, + ); +}); + describe("unmatched package export conditions", () => { it.each([ [[{ browser: "./default.cjs" }], "browser"], diff --git a/tests/boundary/javascript/packageImportAliases.test.ts b/tests/boundary/javascript/packageImportAliases.test.ts index b009c3ad4..64da6c235 100644 --- a/tests/boundary/javascript/packageImportAliases.test.ts +++ b/tests/boundary/javascript/packageImportAliases.test.ts @@ -455,6 +455,138 @@ describe("package import target arrays compared with Node", () => { }); }); +describe("package import target URL normalization compared with Node", () => { + it("normalizes targets before package-root validation and array selection", async () => { + const root = await createTestTempDirectory("rea-package-target-url-"); + const directory = "app space#?%λ"; + const imports = { + "#tab-escape": ["./.\t./outside.cjs", "./fallback.cjs"], + "#newline-escape": ["./.%\n2e/outside.cjs", "./fallback.cjs"], + "#cr-escape": ["./.\r./outside.cjs", "./fallback.cjs"], + "#normalized": "./fall\tback.cjs", + "#trimmed": "./fallback.cjs \t", + "#suffix": "./fallback.cjs?selected=one#fragment", + "#return": `./.\t./${encodeURIComponent(directory)}/fallback.cjs`, + }; + const contents = await writeTree(root, { + [`${directory}/package.json`]: JSON.stringify({ imports }), + [`${directory}/main.cjs`]: "module.exports = 1;", + [`${directory}/fallback.cjs`]: "module.exports = 1;", + }); + const inventoried = inventory(contents); + for (const specifier of Object.keys(imports)) { + const required = await nodeRequireTarget( + join(root, directory, "main.cjs"), + specifier, + ); + expect(required.error_code, specifier).toBeNull(); + expect(posixRelative(root, required.target ?? "")).toBe( + `${directory}/fallback.cjs`, + ); + const imported = await nodeImportTarget(join(root, directory), specifier); + expect(imported.error_code, specifier).toBeNull(); + expect(posixRelative(root, fileURLToPath(imported.target ?? ""))).toBe( + `${directory}/fallback.cjs`, + ); + for (const moduleKind of ["require", "import"] as const) + expect( + resolveAlias( + inventoried, + specifier, + `${directory}/main.cjs`, + moduleKind, + ), + specifier, + ).toMatchObject({ + declared_path: specifier, + resolution_status: "resolved", + resolved_path: `${directory}/fallback.cjs`, + }); + } + }); + + it("keeps URL decoding and missing-file failures after array selection", async () => { + const root = await createTestTempDirectory( + "rea-package-target-url-refusal-", + ); + const contents = await writeTree(root, { + "package.json": JSON.stringify({ + imports: { + "#escape": "./.\t./outside.cjs", + "#separator": ["./fall%2fback.cjs", "./fallback.cjs"], + "#missing": ["./miss\ting.cjs", "./fallback.cjs"], + "#query-separator": ["./first.cjs?selected=%2f", "./fallback.cjs"], + }, + }), + "main.cjs": "module.exports = 1;", + "fallback.cjs": "module.exports = 1;", + "first.cjs": "module.exports = 2;", + }); + const inventoried = inventory(contents); + for (const [specifier, code] of [ + ["#escape", "ERR_INVALID_PACKAGE_TARGET"], + ["#separator", "ERR_INVALID_MODULE_SPECIFIER"], + ] as const) { + expect( + (await nodeRequireTarget(join(root, "main.cjs"), specifier)).error_code, + ).toBe(code); + expect((await nodeImportTarget(root, specifier)).error_code).toBe(code); + for (const moduleKind of ["require", "import"] as const) + expect( + resolveAlias(inventoried, specifier, "main.cjs", moduleKind), + ).toMatchObject({ resolution_status: "rejected", resolved_path: null }); + } + expect( + (await nodeRequireTarget(join(root, "main.cjs"), "#missing")).error_code, + ).toBe("MODULE_NOT_FOUND"); + const imported = await nodeImportTarget(root, "#missing"); + expect(posixRelative(root, fileURLToPath(imported.target ?? ""))).toBe( + "missing.cjs", + ); + for (const moduleKind of ["require", "import"] as const) + expect( + resolveAlias(inventoried, "#missing", "main.cjs", moduleKind), + ).toMatchObject({ + resolution_status: "not-found", + resolved_path: null, + limitations: [expect.stringContaining("missing.cjs")], + }); + expect( + resolveAlias( + inventoried, + "#escape", + "main.cjs", + "require", + ).limitations.join(" "), + ).toContain(JSON.stringify("./.\t./outside.cjs")); + expect( + (await nodeRequireTarget(join(root, "main.cjs"), "#query-separator")) + .error_code, + ).toBe("ERR_INVALID_MODULE_SPECIFIER"); + expect( + resolveAlias(inventoried, "#query-separator", "main.cjs", "require"), + ).toMatchObject({ resolution_status: "rejected", resolved_path: null }); + const withQuery = await nodeImportTarget(root, "#query-separator"); + expect(posixRelative(root, fileURLToPath(withQuery.target ?? ""))).toBe( + "first.cjs", + ); + expect( + resolveAlias(inventoried, "#query-separator", "main.cjs", "import"), + ).toMatchObject({ + resolution_status: "resolved", + resolved_path: "first.cjs", + }); + expect( + resolveAlias( + inventoried, + "#missing", + "main.cjs", + "require", + ).limitations.join(" "), + ).toContain(JSON.stringify("./miss\ting.cjs")); + }); +}); + describe("package import aliases compared with Node", () => { it("matches Node require and import for an exact root alias", async () => { const root = await createTestTempDirectory("rea-import-alias-root-");