fix(ext/node): fix module resolution for nested package.json files - #33767

Thanks for the AI-tool disclosure and the precise issue reference (#33766 → regression from #33170). The diagnosis is right: #33170 added a path-traversal guard using requestPath (the directory of the innermost package.json) as the boundary, which rejects the legitimate pattern of a nested package.json whose main points up into the outer package's tree. The new positive test (main_nested_package_cjs) sits in the existing byonm_main_path_traversal spec dir alongside the existing negative tests, so the regression check and the security check are anchored together — good.

Substance walk

• JS side (01_require.js:391–410) and Rust side (resolution.rs:2007–2026) implement the same shape: walk parent directories, stop at filesystem root or a node_modules basename, update packageRoot to the highest ancestor that has package.json. So for node_modules/nested-pkg/sub/ the loop visits nested-pkg/ (has package.json → packageRoot = nested-pkg/), then node_modules/ (basename match → break). End result: boundary is nested-pkg/, ../cjs/sub.js resolves cleanly, traversal still blocked at node_modules/. The two implementations stay in lockstep, which is right.
• stat(...) === 0 matches the existing tryFile convention (0 = regular file) and benefits from the existing statCache, so the per-require overhead is bounded.
• pnpm-style hoisted layouts still work: node_modules/.pnpm/[email protected]/node_modules/pkg/sub/ walks up to pkg/ and breaks at the inner node_modules, which is the right boundary.
• The negative case still fires: the existing main_cjs and main_esm tests in this same spec exercise an outer-package.json with main: "../../target.cjs" (genuine path-traversal attack), and after the fix the boundary for those is still the outer package directory, so the guard rejects them. That's preserved by both pre-existing assertions in __test__.jsonc:6–14.

Edge cases worth thinking about

• Workspace root package.json overlap. In a workspace layout /workspace/package.json + /workspace/packages/pkg-a/package.json + /workspace/packages/pkg-a/sub/package.json, the walk reaches /workspace/ (since none of the intermediates is node_modules), and packageRoot becomes the workspace root — meaning a malicious "main": "../../sibling-pkg/secret.json" from pkg-a/sub/package.json would pass the boundary check. That's probably fine in practice (workspaces are trusted code by definition), but worth confirming with bartlomieju since #33170's intent was specifically guarding the boundary at install scope.
• No outer package.json at all. If requestPath has no ancestor with package.json before the filesystem root, the walk runs all the way up and ends with packageRoot === requestPath (initial value), which preserves the old behavior — good.
• Symlinks. pathDirname/Path::parent() operate lexically, so symlinked package roots aren't followed. Existing behavior, just noting it doesn't get worse.

CI / process

CI is essentially empty so far (1 success, 1 license/cla pending) — no real test signal yet. Worth waiting for the full suite to come back before judging substance against existing module-resolution coverage. Author labels this a "draft implementation" so further iteration is expected.

The regression in node_resolver/resolution.rs traces back to #33170 (path traversal hardening), so cc @bartlomieju on the boundary semantics — any refinement of the package boundary in this file is the kind of call that warrants the original author's eye, especially since the workspace-root edge case noted above moves the boundary in a way the original guard didn't anticipate.

Significant rework since my prior pass at e5e2565. The new approach (derive package root from path structure rather than filesystem walks) is conceptually cleaner — find_package_root_from_node_modules walks the path components looking for node_modules, then takes the next 1 (unscoped) or 2 (scoped) components as the package root. No syscalls, just string parsing. The new spec test main_nested_escape_cjs correctly pins that nested-pkg/evil-sub with main: "../../../secret.json" is rejected by the path-traversal check.

Blocking concern: 6 CI shards regressed on tests this refactor's logic touches

test node_compat (1/3, 2/3, 3/3) debug linux-aarch64 and the same three shards on macos-aarch64 all fail on the same two tests:

• parallel/test-require-resolve.js — fails with Cannot find module '/home/runner/.../node_modules/no_index/lib/index.js'. Please verify that the package.json has a valid "main" entry
• parallel/test-require-extension-over-directory.js — same flavor

Both are upstream Node tests for the require() resolution path that this PR's refactor directly modifies. They would have been passing on main before this PR.

Walking through the no_index case to figure out where it breaks:

• package_json.path = .../node_modules/no_index/package.json
• package_path = .../node_modules/no_index
• find_package_root_from_node_modules returns .../node_modules/no_index (matches node_modules component, takes next = no_index, no @ prefix). So package_root == package_path.
• guess = .../node_modules/no_index/lib/index.js (assuming main is ./lib/index.js)
• guess.starts_with(&package_root) → true, path-traversal check passes
• self.sys.is_file(&guess) → presumably false (or the file is elsewhere)
• Falls through to extension probing loop

The error message points at the failure of the is_file check, but the message format Cannot find module '<path>'. Please verify that the package.json has a valid "main" entry is the JS-side error from 01_require.js:tryPackage. So the JS-side packageRoot derivation may differ from the Rust-side one in a way that's biting.

Possible JS-side bug

Looking at the JS 01_require.js changes:

let packageRoot = requestPath;
let current = requestPath;
while (true) {
  const parent = pathDirname(current);
  if (parent === current) break;
  const basename = op_require_path_basename(current);
  if (basename === "node_modules") {
    // current is the node_modules dir...

The loop checks basename(current), then advances current = parent. But the initial value is current = requestPath, and the first iteration checks basename(requestPath) — which for node_modules/no_index is no_index, not node_modules. The loop only advances on current = parent AFTER the body, so the first iteration's current is the deepest path. When current finally becomes node_modules, requestPath.slice(current.length + 1) correctly gives "no_index".

But — requestPath here is whatever was passed to tryPackage. For the no_index test, what is it? If it's the package directory node_modules/no_index/, then current walks [no_index, node_modules] and matches at iteration 2. If it's node_modules/no_index/lib/, then current walks [lib, no_index, node_modules] and matches at iteration 3 — same final result.

Actually I think the issue may be more subtle: for the extensions over directory and require resolve tests, the path may include a fixture path with node_modules embedded that happens to be the test fixture's own node_modules, not an actual npm install. If the fixture path is e.g. tests/.../fixtures/node_modules/no_index/lib, the new code would derive tests/.../fixtures/node_modules/no_index as packageRoot, then check that filename (which the test resolves) starts with it. If the test's resolved filename uses a different path representation (e.g., normalised differently or via symlinks), the startsWith check would fail and the error would be raised.

Worth running these two tests locally with cargo test --test node_compat -- test-require-resolve test-require-extension-over-directory and adding a debug log of packageRoot vs filename to see the actual mismatch. The most likely culprits are:

1. Trailing-slash inconsistency — packageRoot could be ".../no_index" while filename is ".../no_index/lib/index.js" and the + "/" concatenation should make them match, but if the OS uses \ on Windows and current + sep doesn't, that's a divergence.
2. Path separator detection — const sep = rel.indexOf("/") !== -1 ? "/" : "\\"; is brittle: if rel is just one component (no separator), defaults to "\\" even on Unix. For the no_index case with parts.length === 1, packageRoot = current + sep + parts[0] would use "\\" separator on Unix, producing node_modules\\no_index which doesn't match anything.

That's the most likely bug — separator default to backslash when the path has no separator after node_modules. Fix:

const sep = current.indexOf("\\") !== -1 ? "\\" : "/";

or better, normalize via op_require_path_join or similar.

CI is consistently failing on the aarch64 shards (linux + macos), which use Unix paths exclusively. That matches the suspected bug — separator detection failing when the rel string has no embedded /. ✓

Direction

Fix the JS-side separator detection and the 6 failing shards should clear. Substance of the refactor is otherwise the right shape — happy to come back once those pass.

LGTM. The new commit 1121c9e does the exact one-character clippy fix I called out at cbbafc0 (prefix.push(&components[nm_idx + 2]) -> prefix.push(components[nm_idx + 2]) - drops the & that triggers clippy::needless_borrows_for_generic_args). And the coordination question from my prior review is now resolved: #33804 is closed, so this is the canonical version going forward and credit-attribution stays with @mhrstmnn.

Substance recap (validated at cbbafc0, unchanged since)

• JS-side findPackageRootFromNodeModules (ext/node/polyfills/01_require.js): late lastIndexOf("/node_modules/") vs lastIndexOf("\\node_modules\\") to pick the rightmost node_modules separator, scoped-package handling for @scope/pkg paths, fallback to null when no node_modules ancestor exists.
• Rust-side find_package_root_from_node_modules (libs/node_resolver/resolution.rs:2781): rposition-based last-node_modules lookup, &components[..=nm_idx] prefix build, scoped-vs-unscoped branch on @-prefixed first segment.
• Spec test fixtures under tests/specs/npm/nested_package_json/: nested package + traversal-escape evil-sub/main test that pins the security boundary (a nested package can't main a path outside its own root).

CI

97 success, 1 skipped, 2 fail. Both failures are unrelated infrastructure:

• wpt release linux-x86_64: expired WPT test-asset TLS certificate - certificate is not valid after 1717209917 (60677885 seconds ago). Same flake biting many other PRs in the queue today.
• ci status: rollup of the wpt failure above.

No node_compat, unit, unit_node, specs, or integration test failures - the specific test surface for this resolver change is fully green.