Validate mock.module() first argument is a string#28518
Conversation
|
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)
WalkthroughAdded a runtime type check in Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
robobun
force-pushed
the
farm/ca3e007d/fix-mock-module-non-string-crash
branch
from
79340bd to
e9dd4c3
Compare
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
mock.module() calls toString() on its first argument before checking its
type, then passes the result through the module resolver. When a
non-string value like SharedArrayBuffer is passed, toString() produces
a string like "function SharedArrayBuffer() { [native code] }" which
the resolver tries to auto-install as a package, crashing because the
package manager's logger allocator is uninitialized in this context.
Add an isString() check before the toString() call, matching the
validation pattern used by Jest.call() and other Bun APIs.
robobun
force-pushed
the
farm/ca3e007d/fix-mock-module-non-string-crash
branch
from
e9dd4c3 to
eb9df84
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR accidentally deletes the entire BunPlugin.cpp (965 lines) instead of adding an isString() guard — the build will fail at link time due to missing symbols, and the expected error message has no implementation.
Extended reasoning...
Overview
The PR touches two files: it deletes src/bun.js/bindings/BunPlugin.cpp entirely and adds a new test file test/js/bun/test/mock/mock-module-non-string.test.ts. The intent described in the PR description is to add an isString() check inside JSMock__jsModuleMock before the toString() call, but the actual diff shows the entire 965-line file was deleted with no replacement.
Security risks
No security risks in the intended change. The bug being fixed (crash on non-string argument) is a stability issue, not a security vulnerability.
Level of scrutiny
High — this modifies core C++ binding infrastructure for the plugin system, mock module support, and virtual module resolution. These are production-critical code paths used by Bun.plugin(), mock.module(), and the entire onLoad/onResolve plugin mechanism.
Why this cannot be approved
The bug report has been verified: src/bun.js/bindings/BunPlugin.cpp does not exist in the repository, only BunPlugin.h remains. grep finds JSMock__jsModuleMock only in jest.zig (as an extern fn declaration) with no C++ definition anywhere. The error string "mock(module, fn) requires a module name string" that the new test expects is not generated by any code in the codebase. This PR will produce a linker failure with undefined symbols for at minimum: JSMock__jsModuleMock, jsFunctionBunPlugin, jsFunctionBunPluginClear, Bun::runVirtualModule, Bun::createModuleMockStructure, Bun__runOnLoadPlugins, and Bun__runOnResolvePlugins. The fix needs to restore BunPlugin.cpp and add the isString() guard inside JSMock__jsModuleMock.
robobun
left a comment
robobun
left a comment
There was a problem hiding this comment.
The review above was against stale commit 79340bd where a blob upload issue temporarily corrupted the file. That was fixed by force-push to eb9df84 — the current HEAD.
Current diff is correct: BunPlugin.cpp is modified (+5 lines, not deleted), adding the isString() guard inside JSMock__jsModuleMock:
if (!callframe->argument(0).isString()) {
scope.throwException(lexicalGlobalObject, JSC::createTypeError(lexicalGlobalObject, "mock(module, fn) requires a module name string"_s));
return {};
}This is placed before the toString() call, matching the validation pattern in Jest.call().
There was a problem hiding this comment.
LGTM — the isString() guard is correct and the test file now has all required @ts-expect-error annotations.
Extended reasoning...
Overview
This PR modifies src/bun.js/bindings/BunPlugin.cpp to add a 5-line runtime type guard in JSMock__jsModuleMock, and adds a new test file test/js/bun/test/mock/mock-module-non-string.test.ts covering both invalid and valid inputs.
Security risks
None. The change only adds input validation that rejects non-string arguments early, preventing a downstream crash in the module resolver. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low. The change is a single focused guard matching the established pattern used elsewhere in the same file (e.g., jsFunctionAppendVirtualModulePluginBody already does if (!moduleIdValue.isString())). The fix is surgical and self-contained.
Other factors
Previous review runs flagged two issues: (1) an initial commit that accidentally deleted BunPlugin.cpp entirely, and (2) missing @ts-expect-error annotations on the non-string test calls. Both were corrected in the final commit eb9df84. The current diff is clean: the isString() check is in the right place, the TypeError message matches what the tests assert, and all four non-string calls in the test are properly annotated.
Jarred-Sumner
deleted the
farm/ca3e007d/fix-mock-module-non-string-crash
branch
`mock.module()` calls `toString()` on its first argument before checking
its type, then passes the result through the module resolver. When a
non-string value like `SharedArrayBuffer` is passed, `toString()`
produces `"function SharedArrayBuffer() { [native code] }"` which the
resolver tries to auto-install as a package, crashing because the
package manager's logger allocator is uninitialized in this context.
Add an `isString()` check before the `toString()` call, matching the
validation pattern used by `Jest.call()` and other Bun APIs.
Crash fingerprint: `Address:unknown-crash:bun-debug+0x90074c1`
`mock.module()` calls `toString()` on its first argument before checking
its type, then passes the result through the module resolver. When a
non-string value like `SharedArrayBuffer` is passed, `toString()`
produces `"function SharedArrayBuffer() { [native code] }"` which the
resolver tries to auto-install as a package, crashing because the
package manager's logger allocator is uninitialized in this context.
Add an `isString()` check before the `toString()` call, matching the
validation pattern used by `Jest.call()` and other Bun APIs.
Crash fingerprint: `Address:unknown-crash:bun-debug+0x90074c1`
2 participants
mock.module()callstoString()on its first argument before checking its type, then passes the result through the module resolver. When a non-string value likeSharedArrayBufferis passed,toString()produces"function SharedArrayBuffer() { [native code] }"which the resolver tries to auto-install as a package, crashing because the package manager's logger allocator is uninitialized in this context.Add an
isString()check before thetoString()call, matching the validation pattern used byJest.call()and other Bun APIs.Crash fingerprint:
Address:unknown-crash:bun-debug+0x90074c1