bake: validate app.bundlerOptions values are objects#30468
Conversation
Previously, passing a non-object value for `app.bundlerOptions`,
`app.bundlerOptions.{server,client,ssr}`, or a non-boolean non-object
value for `minify` would hit a debug assertion in `JSValue.get` instead
of throwing a proper error. Additionally, `minify: false` would fall
through to the object path and crash.
Now these cases throw `InvalidArguments` errors.
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds context-aware validation for nested bundlerOptions subsets by passing a comptime name into BuildConfigSubset.fromJS, supports boolean or object minify values, and adds parameterized tests covering server/client/ssr and minify cases. ChangesBundler Options Validation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (!js_options.isObject()) { | ||
| return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions' must be an object", .{}); | ||
| } |
There was a problem hiding this comment.
🟣 Heads-up (pre-existing, not introduced by this PR): the same crash class still exists for framework.fileSystemRouterTypes — each fsr_opts array element goes straight into getOptionalString() → target.get() without an isObject() check, so Bun.serve({ app: { framework: { fileSystemRouterTypes: [42] } } }) still trips bun.debugAssert(target.isObject()). The adjacent builtInModules loop already guards this; might be worth adding the same if (!fsr_opts.isObject()) return global.throwInvalidArguments(...) while you're in this file.
Extended reasoning...
What the bug is
This PR fixes the debug-assert crash for app.bundlerOptions by adding isObject() guards before calling .get()/.getOptional() on user-supplied values. However, the identical crash pattern remains in the same file for framework.fileSystemRouterTypes array elements.
In Framework.fromJS (src/bake/bake.zig), the fileSystemRouterTypes loop iterates each element as fsr_opts and immediately passes it to getOptionalString(fsr_opts, global, "root", ...). getOptionalString() calls target.get(global, property) on its first line, and JSValue.get() (src/jsc/JSValue.zig:1534) contains bun.debugAssert(target.isObject()). There is no isObject() check on fsr_opts before this call.
Code path
while (try it.next()) |fsr_opts| : (i += 1) {
const root = try getOptionalString(fsr_opts, global, "root", refs, arena) orelse { ... };
// ^ no isObject() guard — crashes if fsr_opts is a primitiveCompare with the adjacent builtInModules loop in the same function, which does guard:
while (try it.next()) |file| : (i += 1) {
if (!file.isObject()) {
return global.throwInvalidArguments("'builtInModules[{d}]' is not an object", .{i});
}The asymmetry confirms this is an oversight rather than intentional.
Why existing code doesn't prevent it
opts.getArray(global, "fileSystemRouterTypes") only validates that the property itself is an array; it does not validate the element types. The iterator yields whatever JSValues are in the array, including primitives.
Step-by-step proof
- User calls
Bun.serve({ app: { framework: { fileSystemRouterTypes: [42] } } }). UserOptions.fromJSseesconfigis an object, skips the bundlerOptions branch (not present), and callsFramework.fromJSwith the framework object.Framework.fromJSseesoptsis an object, falls through the string/object checks, processesreactFastRefresh/serverComponents/builtInModules(all absent → defaults).- Reads
fileSystemRouterTypesviagetArray— it's an array of length 1, passes. - Iterator yields
fsr_opts = 42(a JS number, not an object). - Calls
getOptionalString(fsr_opts, global, "root", ...)→target.get(global, "root")→bun.debugAssert(target.isObject())fires on the number → crash in debug builds.
Impact
Same as the bug this PR fixes (fingerprint 86855f8bedcf582b): a debug-assert crash on malformed user input instead of a clean InvalidArguments throw. The fuzzer that found the bundlerOptions case will likely re-discover this via the framework path.
Fix
Add the same guard at the top of the loop:
while (try it.next()) |fsr_opts| : (i += 1) {
if (!fsr_opts.isObject()) {
return global.throwInvalidArguments("'fileSystemRouterTypes[{d}]' is not an object", .{i});
}
const root = try getOptionalString(fsr_opts, global, "root", refs, arena) orelse { ... };This is pre-existing — the PR does not touch these lines — so it's non-blocking, but worth doing while the author is in this file fixing the same root cause.
1 participant
Fuzzer found a debug assertion crash in
Bun.serve({ app: { bundlerOptions: ... } }).JSValue.getasserts that its target is an object, butbake.UserOptions.fromJSandBuildConfigSubset.fromJSwere calling.getOptional()on user-provided values without validating them first. This crashed when:app.bundlerOptionswas a primitive (number, string, boolean, symbol, bigint)app.bundlerOptions.{server,client,ssr}was a primitiveapp.bundlerOptions.*.minifywasfalseor any non-boolean primitive (the previous check wasisBoolean() and asBoolean(), sofalsefell through to the object path)These now throw
InvalidArgumentserrors instead.Fingerprint:
86855f8bedcf582b