Skip to content

Commit f89fba1

Browse files
authored
fix(patches): sort endsWith checks by path length to prevent suffix collisions (#1166)
* fix(patches): sort endsWith checks by path length to prevent suffix collisions Routes whose paths are suffixes of other routes (e.g. `/test/app` vs `/`) were resolved to the wrong module because the shorter path matched first in the generated `.endsWith()` chain. Sort files by path length descending before generating the chain so that longer (more specific) paths are always checked first. Applied to all three `.endsWith()` chains: - `getRequires()` in dynamic-requires.ts (JS files for requirePage and NodeModuleLoader) - `htmlFiles` in dynamic-requires.ts (HTML files for requirePage) - `getEvalManifestRule()` in load-manifest.ts (client-reference-manifest files) * test: add unit tests for getRequires path sorting
1 parent da62708 commit f89fba1

4 files changed

Lines changed: 46 additions & 4 deletions

File tree

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@opennextjs/cloudflare": patch
3+
---
4+
5+
fix: sort `.endsWith()` checks by path length descending to prevent suffix collisions in dynamic requires
6+
7+
Routes whose paths are suffixes of other routes (e.g. `/test/app` vs `/`) were resolved incorrectly because the shorter path matched first in the generated `.endsWith()` chain. Sorting by path length descending ensures more specific (longer) paths are always checked first.
8+
9+
Fixes #1156.
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { describe, expect, test } from "vitest";
2+
3+
import { getRequires } from "./dynamic-requires.js";
4+
5+
describe("getRequires", () => {
6+
test("sorts paths by length descending so longer paths match first", () => {
7+
const files = ["app/page.js", "test/app/page.js", "page.js"];
8+
const result = getRequires("id", files, "/server");
9+
10+
const endsWithPattern = /\.endsWith\("([^"]+)"\)/g;
11+
const matches = [...result.matchAll(endsWithPattern)].map((m) => m[1]);
12+
13+
// Longer (more specific) paths should appear before shorter ones
14+
expect(matches).toEqual(["test/app/page.js", "app/page.js", "page.js"]);
15+
});
16+
17+
test("does not mutate the original files array", () => {
18+
const files = ["b.js", "aa.js", "c.js"];
19+
const original = [...files];
20+
getRequires("id", files, "/server");
21+
22+
expect(files).toEqual(original);
23+
});
24+
});

packages/cloudflare/src/cli/build/patches/plugins/dynamic-requires.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,13 @@ function getServerDir(buildOpts: BuildOptions) {
3030
return join(buildOpts.outputDir, "server-functions/default", getPackagePath(buildOpts), ".next/server");
3131
}
3232

33-
function getRequires(idVariable: string, files: string[], serverDir: string) {
33+
export function getRequires(idVariable: string, files: string[], serverDir: string) {
3434
// Inline fs access and dynamic requires that are not supported by workerd.
35-
return files
35+
// Sort by path length descending so longer (more specific) paths match first.
36+
// Without this, `/test/app/page.js` could match the `.endsWith("app/page.js")`
37+
// check for `/` before reaching the correct `.endsWith("test/app/page.js")` check.
38+
const sorted = [...files].sort((a, b) => b.length - a.length);
39+
return sorted
3640
.map(
3741
(file) => `
3842
if (${idVariable}.replaceAll(${JSON.stringify(sep)}, ${JSON.stringify(posix.sep)}).endsWith(${JSON.stringify(normalizePath(file))})) {
@@ -111,7 +115,9 @@ async function getRequirePageRule(buildOpts: BuildOptions) {
111115

112116
const manifests = pagesManifests.concat(appPathsManifests);
113117

114-
const htmlFiles = manifests.filter((file) => file.endsWith(".html"));
118+
// Sort html files by path length descending so longer (more specific) paths
119+
// match first, preventing suffix collisions in the `.endsWith()` chain (see #1156).
120+
const htmlFiles = manifests.filter((file) => file.endsWith(".html")).sort((a, b) => b.length - a.length);
115121
const jsFiles = manifests.filter((file) => file.endsWith(".js"));
116122

117123
return {

packages/cloudflare/src/cli/build/patches/plugins/load-manifest.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,10 @@ async function getEvalManifestRule(buildOpts: BuildOptions) {
100100
windowsPathsNoEscape: true,
101101
});
102102

103-
const returnManifests = manifests
103+
// Sort by path length descending so longer (more specific) paths match first,
104+
// preventing suffix collisions in the `.endsWith()` chain (see #1156).
105+
const sortedManifests = [...manifests].sort((a, b) => b.length - a.length);
106+
const returnManifests = sortedManifests
104107
.map((manifest) => {
105108
const endsWith = normalizePath(relative(baseDir, manifest));
106109
const key = normalizePath("/" + relative(appDir, manifest)).replace(

0 commit comments

Comments
 (0)