-
Notifications
You must be signed in to change notification settings - Fork 4.3k
resolver: skip auto-install for invalid npm package names #29255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
2
commits into
main
Choose a base branch
from
farm/c3283de5/resolver-skip-autoinstall-invalid-name
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+115
−1
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, tempDir } from "harness"; | ||
|
|
||
| // When auto-install is enabled, the resolver must not attempt to install a | ||
| // package whose name is not a valid npm package name. Attempting to do so | ||
| // reentrantly ticks the JS event loop from inside the resolver, which has led | ||
| // to crashes when reached via mock.module() / vi.mock() / Bun.resolveSync() | ||
| // with arbitrary user-provided strings. | ||
| test("resolver does not attempt auto-install for invalid npm package names", async () => { | ||
| let requests = 0; | ||
| await using server = Bun.serve({ | ||
| port: 0, | ||
| fetch() { | ||
| requests++; | ||
| return new Response("{}", { status: 404, headers: { "content-type": "application/json" } }); | ||
| }, | ||
| }); | ||
|
|
||
| using dir = tempDir("resolve-autoinstall-invalid-name", { | ||
| "index.js": ` | ||
| const specifiers = [ | ||
| "function f2() {\\n const v6 = new ArrayBuffer();\\n}", | ||
| "has spaces", | ||
| "(parens)", | ||
| "{braces}", | ||
| "line1\\nline2", | ||
| "foo\\tbar", | ||
| "a\\u0000b", | ||
| ]; | ||
| for (const s of specifiers) { | ||
| try { | ||
| Bun.resolveSync(s, import.meta.dir); | ||
| } catch {} | ||
| } | ||
| console.log("ok"); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "--install=force", "index.js"], | ||
| env: { | ||
| ...bunEnv, | ||
| BUN_CONFIG_REGISTRY: server.url.href, | ||
| NPM_CONFIG_REGISTRY: server.url.href, | ||
| }, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toContain("ok"); | ||
| expect(requests).toBe(0); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("resolver still attempts auto-install for valid npm package names", async () => { | ||
| let requests = 0; | ||
| await using server = Bun.serve({ | ||
| port: 0, | ||
| fetch() { | ||
| requests++; | ||
| return new Response("{}", { status: 404, headers: { "content-type": "application/json" } }); | ||
| }, | ||
| }); | ||
|
|
||
| using dir = tempDir("resolve-autoinstall-valid-name", { | ||
| "index.js": ` | ||
| try { | ||
| Bun.resolveSync("some-package-that-does-not-exist-abc123", import.meta.dir); | ||
| } catch {} | ||
| console.log("ok"); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "--install=force", "index.js"], | ||
| env: { | ||
| ...bunEnv, | ||
| BUN_CONFIG_REGISTRY: server.url.href, | ||
| NPM_CONFIG_REGISTRY: server.url.href, | ||
| }, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toContain("ok"); | ||
| expect(requests).toBeGreaterThan(0); | ||
| expect(exitCode).toBe(0); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 The new gate at resolver.zig:1907 relies on
strings.isNPMPackageName, but that function has a pre-existing bug where empty-scope specifiers like@/pkgincorrectly pass the check. This means@/pkg-style specifiers still reach the auto-install path, potentially triggering the same reentrantEventLoop.tick()SIGSEGV this PR aims to prevent. The fix inisNPMPackageNameIgnoreLengthshould useslash_index > 1instead ofslash_index > 0to require at least one character between@and/.Extended reasoning...
What the bug is:
isNPMPackageNameIgnoreLengthinsrc/string/immutable.ziguses the conditionslash_index > 0when validating scoped package names, but this condition accepts empty-scope specifiers like@/pkg. A valid scoped npm package requires at least one character between@and/(e.g.,@scope/pkg), so the condition should beslash_index > 1(or equivalently>= 2).The specific code path:
In
isNPMPackageNameIgnoreLength, for input@/pkg:target[0] == '@'soscoped = true. The loop iterates overtarget[1..]=/pkg; ati=0, it encounters/and setsslash_index = i + 1 = 1. The final return is!scoped or slash_index > 0 and slash_index + 1 < target.len. Due to Zig's operator precedence (andbinds tighter thanor), this evaluates as(!true) or ((1 > 0) and (2 < 5))=false or (true and true)=true. So@/pkgpasses the validator despite having an empty scope name.Why existing code doesn't prevent it:
The bug is structural:
slash_indexis set toi + 1whereiis 0-based withintarget[1..]. When the slash is immediately after@(index 1 in the original string, index 0 in the slice),slash_indexbecomes 1. The conditionslash_index > 0is satisfied by any scoped name, including those with empty scope. The correct threshold isslash_index > 1(meaning the slash must be at least at position 2 in the original string, requiring one non-empty scope character).Impact:
The new guard at resolver.zig:1907 was added specifically to prevent garbage specifiers from reaching
enqueueDependencyToRoot → sleepUntil → EventLoop.tick()reentry. Specifiers matching@/anythingbypass this guard and still reach the auto-install path. This means: (1) wasteful registry requests for@/anythingpackages that can never exist, and (2) potential exposure to the same reentrant crash the PR aims to block, via@/pkg-style specifiers.How to fix it:
In
src/string/immutable.zig, insideisNPMPackageNameIgnoreLength, change the final return condition fromslash_index > 0toslash_index > 1. This ensures the scope portion has at least one character.Step-by-step proof with
@/pkg(length 5):ESModule.Package.parseName("@/pkg")— first char is@, finds first slash at index 1, slash2 at index 3 (end of "pkg"), returnsspecifier[0..5]="@/pkg".ESModule.Package.parse("@/pkg")— returns a non-nullPackagewithname = "@/pkg"(no leading dot, backslash, or percent).esm_ != null(step 2) andstrings.isNPMPackageName("@/pkg")is called.isNPMPackageNameIgnoreLength("@/pkg"):scoped = true; loop over"/pkg"setsslash_index = 1; return!true or (1>0 and 2<5)=true.enqueueDependencyToRootcalled →PackageManager.sleepUntil→ reentrantEventLoop.tick()→ potential SIGSEGV.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch —
isNPMPackageName("@/pkg")does return true becauseslash_indexis 1 when/immediately follows@, and the final return only checks> 0.That said, it's a pre-existing limitation of the shared validator (used by
bun add,bun link,bun pm view, pnpm import, the bundler, and now here), not something this PR introduces —@/pkgalready reachedenqueueDependencyToRootbefore this change. The Fuzzilli crash this PR targets (function f2() {\n...}) is blocked, and the gate is a strict improvement over no validation.Tightening the final return to
slash_index > 1is correct but touches all seven call sites and deserves its own unit coverage forisNPMPackageName— I'd rather land that as a separate one-liner than widen this crash fix.