Percent-decode ESM import specifiers before filesystem lookup#28746
Percent-decode ESM import specifiers before filesystem lookup#28746
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded ESM-style percent-decoding for relative/absolute import specifiers in the resolver: percent-decodes when Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolver/resolver.zig`:
- Around line 1104-1113: After percent-decoding import_path using
PercentEncoding.decode (where buf and fbs are used), check the decoded slice
(buf[0..decoded_len]) for any NUL (byte 0) and reject the import if any are
present; specifically, after computing decoded_len from PercentEncoding.decode
in the resolver code that assigns import_path = buf[0..decoded_len], scan that
slice for a zero byte and return .{ .not_found = {} } (or the existing error
path) if found so that encoded %00 can't bypass the earlier literal NUL check.
- Around line 1089-1094: The percent-decoding gate is too broad: currently it
triggers for any kind where !kind.isCommonJS() and !kind.isFromCSS(), which
unintentionally includes .entry_point_run and .entry_point_build; update the
condition so only true URL-like ESM import kinds are allowed to be
percent-decoded. Replace the current condition using kind.isCommonJS() and
kind.isFromCSS() with an explicit allowlist check (compare kind against the
specific enum variants that represent normal ESM import requests, e.g., the
import request variants used for module resolution) and ensure .entry_point_run
and .entry_point_build are excluded; keep the check that import_path contains
'%' before decoding. Use the same symbols from the diff (kind, import_path, and
the entry point enum names) to locate and change the logic in resolver.zig.
In `@test/regression/issue/28745.test.ts`:
- Line 6: The test only covers a relative ESM specifier ("test.mjs": `import
'./foo%2cbar.mjs';`) so add an absolute-specifier variant to exercise the
altered absolute-path decode/lookup path: add a new fixture entry (e.g.,
"test-abs.mjs") that imports the same module with an absolute specifier such as
`import '/foo%2cbar.mjs';` and ensure the corresponding module file
(foo%2cbar.mjs) exists at the absolute location; apply the same addition for the
other occurrences that use `./foo%2cbar.mjs` (the repeated entries noted in the
comment) so the suite exercises both relative and absolute ESM resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7186037-361e-408b-b745-f6bf4af6bfc0
📒 Files selected for processing (2)
src/resolver/resolver.zigtest/regression/issue/28745.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28745.test.ts`:
- Around line 129-132: The generated import uses an unescaped interpolated path
which breaks on Windows; change the write to quote/escape the absolute specifier
using JSON.stringify(absPath) when building the import line (e.g.,
writeFileSync(`${String(dir)}/test.mjs`, `import
${JSON.stringify(absPath)};\\n`)) and move the fs require (fs) out of the local
block to a module-scope import so fs.writeFileSync is not required inline;
update references to absPath/dir accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f3a7935-74da-44a5-8b4c-59e0e8c1c51a
📒 Files selected for processing (2)
src/resolver/resolver.zigtest/regression/issue/28745.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/28745.test.ts (1)
132-134:⚠️ Potential issue | 🟡 MinorAvoid inline
require()here and emit the import specifier withJSON.stringify().Line 133 uses
require("fs")inside a test that is not testingrequire()behavior, and Line 134 still builds a JS string literal via raw interpolation. Use a module-scope import and write the import statement withJSON.stringify(absPath).Suggested change
+import { writeFileSync } from "node:fs"; import { expect, test } from "bun:test"; import { bunEnv, bunExe, tempDir } from "harness"; ... - const { writeFileSync } = require("fs"); - writeFileSync(`${String(dir)}/test.mjs`, `import '${absPath}';\n`); + writeFileSync(`${String(dir)}/test.mjs`, `import ${JSON.stringify(absPath)};\n`);As per coding guidelines, in
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}only use dynamic import or require when the test is specifically testing dynamic import or require behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28745.test.ts` around lines 132 - 134, Replace the inline require("fs") and raw string interpolation when emitting the import in test.mjs: move to a module-scope import (import { writeFileSync } from "fs") instead of calling require within the test, and when writing the file use JSON.stringify(absPath) to produce a safe JS string literal for the import specifier (i.e. call writeFileSync(`${String(dir)}/test.mjs`, `import ${JSON.stringify(absPath)};\\n`), referencing the existing absPath and writeFileSync identifiers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28745.test.ts`:
- Around line 80-81: The two assertions in the spawned-process test are ordered
incorrectly; move the stderr assertion before the exitCode assertion so process
output is asserted first. Specifically, in test/regression/issue/28745.test.ts
change the expect calls so expect(stderr).toContain("Cannot find module");
appears before expect(exitCode).not.toBe(0); ensuring stderr is checked prior to
exitCode per spawned-process test convention.
---
Duplicate comments:
In `@test/regression/issue/28745.test.ts`:
- Around line 132-134: Replace the inline require("fs") and raw string
interpolation when emitting the import in test.mjs: move to a module-scope
import (import { writeFileSync } from "fs") instead of calling require within
the test, and when writing the file use JSON.stringify(absPath) to produce a
safe JS string literal for the import specifier (i.e. call
writeFileSync(`${String(dir)}/test.mjs`, `import
${JSON.stringify(absPath)};\\n`), referencing the existing absPath and
writeFileSync identifiers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72dfcbc2-c879-4ea1-8389-3cb31e5e0eeb
📒 Files selected for processing (1)
test/regression/issue/28745.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/28745.test.ts (1)
129-134:⚠️ Potential issue | 🟡 MinorReplace inline
require()and safely quote generated absolute specifier.Line 133 uses
require("fs")in a test that is not validatingrequire()behavior, and Line 134 embedsabsPathin single quotes directly. Prefer a module-scope import andJSON.stringify(absPath)when generatingimport ...;source.Suggested patch
-import { expect, test } from "bun:test"; +import { expect, test } from "bun:test"; +import { writeFileSync } from "node:fs"; import { bunEnv, bunExe, tempDir } from "harness"; @@ - const absPath = `${String(dir)}/foo%2cbar.mjs`.replaceAll("\\", "/"); - const { writeFileSync } = require("fs"); - writeFileSync(`${String(dir)}/test.mjs`, `import '${absPath}';\n`); + const absPath = `${String(dir)}/foo%2cbar.mjs`.replaceAll("\\", "/"); + writeFileSync(`${String(dir)}/test.mjs`, `import ${JSON.stringify(absPath)};\n`);#!/bin/bash # Verify the two patterns called out in this comment. rg -n 'require\("fs"\)|import '\''\$\{absPath\}'\'';' test/regression/issue/28745.test.tsAs per coding guidelines, "Only use dynamic import or require when the test is specifically testing dynamic import or require behavior. Otherwise, always use module-scope import statements."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28745.test.ts` around lines 129 - 134, Replace the inline require("fs") with a module-scope import of writeFileSync from "fs" and stop embedding absPath directly in single quotes; update the test so the top of the file imports { writeFileSync } from "fs" and the generated test.mjs content uses JSON.stringify(absPath) when creating the import statement (i.e., call writeFileSync(`${String(dir)}/test.mjs`, `import ${JSON.stringify(absPath)};\\n`), referencing the existing absPath and writeFileSync identifiers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28745.test.ts`:
- Around line 64-68: Update the test "ESM import with encoded slash (%2f) is
rejected per spec" to exercise all encoded-separator variants the resolver
rejects: add import paths using '%2F', '%5c', and '%5C' (in addition to '%2f')
in the generated test.mjs (or add separate test.mjs entries) so the resolver
branches for uppercase percent-encoding and backslash encodings are covered;
ensure each import is asserted to be rejected just like the existing '%2f' case
so Windows/backslash handling and case variants are tested.
---
Duplicate comments:
In `@test/regression/issue/28745.test.ts`:
- Around line 129-134: Replace the inline require("fs") with a module-scope
import of writeFileSync from "fs" and stop embedding absPath directly in single
quotes; update the test so the top of the file imports { writeFileSync } from
"fs" and the generated test.mjs content uses JSON.stringify(absPath) when
creating the import statement (i.e., call
writeFileSync(`${String(dir)}/test.mjs`, `import
${JSON.stringify(absPath)};\\n`), referencing the existing absPath and
writeFileSync identifiers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bddc87e8-a5a3-435a-be7c-71da40c4d9dd
📒 Files selected for processing (1)
test/regression/issue/28745.test.ts
|
@robobun What does node.js do? |
|
Node.js (v24.3.0) behavior for each case:
This PR matches Node.js on all cases. Per the ESM spec, relative/absolute specifiers are URL-like and percent-encoded characters are decoded before filesystem lookup. CJS |
There was a problem hiding this comment.
LGTM — all previously identified issues have been addressed.
Extended reasoning...
Overview
The PR modifies src/resolver/resolver.zig to percent-decode ESM import specifiers (static import and dynamic import()) before filesystem lookup, matching the Node.js ESM spec. It also adds a regression test at test/regression/issue/28745.test.ts covering comma, space, hash, encoded-slash rejection, CJS no-decode, dynamic import, and absolute-path variants.
Security risks
The only meaningful attack surface is path traversal via encoded separators (%2f/%5c), which the PR explicitly rejects. Encoded NULs (%00) are also rejected post-decode. Bare % that is not valid percent-encoding returns .not_found, matching Node.js URIError behavior. The thread-local buffer (percent_decode_buf) is correctly scoped per-thread and cannot be shared across concurrent resolutions.
Level of scrutiny
This touches the hot resolver path, but the change is small, focused, and well-bounded. The gate (kind == .stmt or kind == .dynamic) ensures entry points, CJS, and CSS imports are untouched. The critical dangling-reference bug (decoded path in percent_decode_buf returned as an external Path without copying) was found and fixed in this PR via dirname_store.append, matching the existing pattern in checkRelativePath.
Other factors
All bugs raised in prior review rounds (CodeRabbit and my own inline comments) have been confirmed resolved by the author. The test suite exercises all meaningful branches. No open comments remain.
Relative and absolute ESM import specifiers are URL-like per the spec, so percent-encoded characters (e.g. %2c for comma, %20 for space) should be decoded before filesystem lookup. CJS require() is unaffected. Encoded path separators (%2f, %5c) are rejected per the Node.js ESM spec.
…ath test - Only percent-decode for .stmt and .dynamic kinds (not entry points) - Reject decoded NUL bytes (%00) that would bypass the earlier literal check - Add absolute-path ESM import test case
Copy through dirname_store.append to avoid dangling reference to the threadlocal percent_decode_buf, matching the pattern used by the relative-path external branch in checkRelativePath.
robobun
force-pushed
the
farm/bb40d25a/fix-percent-encoded-imports
branch
from
5b3219d to
8aa5493
Compare
2 participants
Closes #28745
Problem
When an ESM import specifier contains percent-encoded characters (e.g.
%2cfor comma,%20for space,%23for hash), Bun looks for a file with the literal%XXin the name instead of decoding it first:Node.js resolves this correctly because per the ESM spec, relative and absolute import specifiers are URL-like and percent-encoded characters should be decoded before filesystem lookup.
Root Cause
In
src/resolver/resolver.zig,resolveWithoutSymlinkspasses the raw import specifier directly tocheckRelativePath/loadAsFileOrDirectorywithout percent-decoding it first.Fix
Added percent-decoding of import paths for ESM imports (not CJS
require()) when the specifier is a relative (./,../) or absolute path and contains%. Per the Node.js ESM spec, encoded path separators (%2f/%5c) are rejected.Verification