resolver: skip auto-install for invalid npm package names#29255
resolver: skip auto-install for invalid npm package names#29255
Conversation
The auto-install path in loadNodeModules would attempt to install any
specifier that parsed as an ESM package, including arbitrary strings
reached via Bun.resolveSync, require.resolve, import, or
mock.module/vi.mock. For a string like "function f2() {\n...}",
enqueueDependencyToRoot would spin up the package manager and
synchronously pump the JS event loop via sleepUntil from inside the
resolver, reentrantly ticking ConcurrentTask.Queue from a call that is
itself running inside EventLoop.tick.
Gate the auto-install branch on isNPMPackageName(esm.name) so specifiers
that can never resolve against a registry fall straight through to
.not_found instead of initializing the package manager and issuing a
network request.
|
Updated 5:54 AM PT - Apr 13th, 2026
❌ @robobun, your commit 6dbd60a has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29255That installs a local version of the PR into your bun-29255 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughResolver's auto-global-cache loading was restricted: Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Looked at the three linked issues — not adding
All three share the downstream machinery ( |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicates: #28311 is the closest — it also uses #28945 / #28956 are narrower guards inside #28500 / #28310 fix the stale |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted one-line fix with solid test coverage.
Extended reasoning...
Overview
The PR touches three files: one line changed in src/resolver/resolver.zig (the auto-install gate), and two test files — a new integration test for the resolver and an additional case in an existing mock-module test.
Security Risks
The change reduces the attack surface: previously any user-controlled string (e.g. from Bun.resolveSync, import(), require.resolve, or mock.module) could trigger event-loop re-entrancy via PackageManager.sleepUntil, causing a SIGSEGV. The fix closes this path by rejecting strings that are not valid npm package names before the package manager is ever initialized. No new security risks are introduced.
Level of Scrutiny
Low. The code change is a single boolean guard appended to an existing condition. strings.isNPMPackageName is a pre-existing, well-tested utility with clear semantics; the PR description confirms it is already used in bun link, bun pm view, the bundler, and pnpm import. The logic is additive and non-breaking for valid package names.
Other Factors
Tests cover the exact crash-inducing inputs from the Fuzzilli report as well as a positive control (valid name still reaches the registry). The fix is resolver-level, so it covers all entry points rather than patching only mock.module. No CODEOWNERS or security-sensitive subsystems are affected beyond the resolver guard.
|
|
||
| // this is the magic! | ||
| if (global_cache.canUse(any_node_modules_folder) and r.usePackageManager() and esm_ != null) { | ||
| if (global_cache.canUse(any_node_modules_folder) and r.usePackageManager() and esm_ != null and strings.isNPMPackageName(esm_.?.name)) { |
There was a problem hiding this comment.
🟣 The new gate at resolver.zig:1907 relies on strings.isNPMPackageName, but that function has a pre-existing bug where empty-scope specifiers like @/pkg incorrectly pass the check. This means @/pkg-style specifiers still reach the auto-install path, potentially triggering the same reentrant EventLoop.tick() SIGSEGV this PR aims to prevent. The fix in isNPMPackageNameIgnoreLength should use slash_index > 1 instead of slash_index > 0 to require at least one character between @ and /.
Extended reasoning...
What the bug is:
isNPMPackageNameIgnoreLength in src/string/immutable.zig uses the condition slash_index > 0 when 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 be slash_index > 1 (or equivalently >= 2).
The specific code path:
In isNPMPackageNameIgnoreLength, for input @/pkg: target[0] == '@' so scoped = true. The loop iterates over target[1..] = /pkg; at i=0, it encounters / and sets slash_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 (and binds tighter than or), this evaluates as (!true) or ((1 > 0) and (2 < 5)) = false or (true and true) = true. So @/pkg passes the validator despite having an empty scope name.
Why existing code doesn't prevent it:
The bug is structural: slash_index is set to i + 1 where i is 0-based within target[1..]. When the slash is immediately after @ (index 1 in the original string, index 0 in the slice), slash_index becomes 1. The condition slash_index > 0 is satisfied by any scoped name, including those with empty scope. The correct threshold is slash_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 @/anything bypass this guard and still reach the auto-install path. This means: (1) wasteful registry requests for @/anything packages 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, inside isNPMPackageNameIgnoreLength, change the final return condition from slash_index > 0 to slash_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).- At resolver.zig:1907 —
esm_ != null(step 2) andstrings.isNPMPackageName("@/pkg")is called. - Inside
isNPMPackageNameIgnoreLength("@/pkg"):scoped = true; loop over"/pkg"setsslash_index = 1; return!true or (1>0 and 2<5)=true. - Gate passes →
enqueueDependencyToRootcalled →PackageManager.sleepUntil→ reentrantEventLoop.tick()→ potential SIGSEGV.
There was a problem hiding this comment.
Good catch — isNPMPackageName("@/pkg") does return true because slash_index is 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 — @/pkg already reached enqueueDependencyToRoot before 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 > 1 is correct but touches all seven call sites and deserves its own unit coverage for isNPMPackageName — I'd rather land that as a separate one-liner than widen this crash fix.
1 participant
Problem
Fuzzilli hit a flaky SIGSEGV (fingerprint
2519cad1804eace1) from:JSMock__jsModuleMockcallsBun__resolveSyncWithSourceon the specifier before validating the callback, which sends the garbage string through the resolver. The resolver's auto-install gate atloadNodeModulesonly checksesm_ != null;ESModule.Package.parseaccepts anything that doesn't start with.or contain\/%, so the whole function source is treated as a package name.enqueueDependencyToRootthen callsPackageManager.sleepUntil, which re-entersEventLoop.tick()from inside a call that is itself running inside an event-loop tick:The same path is reachable from
Bun.resolveSync,import(), andrequire.resolvewith any user-provided string.Fix
Gate the auto-install branch on
strings.isNPMPackageName(esm_.?.name). That validator already exists and is used bybun link,bun pm view, and the bundler; it rejects newlines, spaces, braces, and anything else that could never be a registry package. Specifiers failing the check fall straight through to.not_found— the same result the registry fetch would eventually produce — without initializing the package manager or ticking the event loop.This is a resolver-level fix, so it covers every entry point (not just
mock.module). It also avoids spurious network requests for garbage specifiers; on this container a single resolve of a multi-line specifier dropped from ~275ms to ~16ms.Tests
test/js/bun/resolve/resolve-autoinstall-invalid-name.test.tsstands up a local registry and verifies zero manifest requests for a set of invalid names with--install=force, plus a positive control that a valid name still hits the registry.test/js/bun/test/mock/mock-module-non-string.test.tsgains a case formock.modulewith newline / whitespace / bracket specifiers (with and without a callback).test/cli/run/run-autoinstall.test.ts(11 tests) andtest/js/bun/test/mock/mock-module.test.tsall pass.Related: #28945, #28956, #28500, #28511.
Fingerprint:
2519cad1804eace1