Validate app.bundlerOptions types in Bun.serve#30402
Conversation
WalkthroughRefactors BuildConfigSubset.fromJS to accept a comptime property_name for contextual error messages, changes boolean minify handling to explicitly set all minify flags (including false), updates UserOptions.fromJS to pass property names for server/client/ssr, and adds tests validating bundlerOptions shapes and minify behavior. ChangesBundler Options Validation Enhancement
🚥 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/bun-serve-args.test.ts`:
- Around line 675-709: Update the tests to assert the error class/code as well
as the message for at least one representative case (e.g., the "bundlerOptions
is not an object" case that calls serve). Instead of only using
expect(...).toThrow(message), call the function inside a try/catch (or capture
the thrown error via a helper) and assert the error is the expected type/code
(for example error instanceof TypeError or error.code ===
'ERR_INVALID_ARG_TYPE') and that error.message matches the existing message;
keep the other table entries as-is but tighten this single representative
assertion referencing serve and the specific test case description.
🪄 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: f88ebf4b-4d61-4262-a0a0-b339eb64a4a3
📒 Files selected for processing (2)
src/bake/bake.zigtest/js/bun/http/bun-serve-args.test.ts
|
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}
There was a problem hiding this comment.
LGTM — straightforward input validation that converts assertion crashes into proper ERR_INVALID_ARG_TYPE errors, with regression tests.
Extended reasoning...
Overview
This PR touches two files: src/bake/bake.zig adds isObject() guards before reading properties from app.bundlerOptions, bundlerOptions.{server,client,ssr}, and minify, converting a fuzzer-found debug-assertion crash into a proper TypeError [ERR_INVALID_ARG_TYPE]. It also fixes minify: false which previously fell through to the object-property path (the old check was isBoolean() and asBoolean()). test/js/bun/http/bun-serve-args.test.ts gains a test.each table covering each invalid shape plus a minify: false regression case. The CodeRabbit suggestion to assert error name/code (not just message) was addressed in a8653bf.
Security risks
None. This is purely additive input validation on a user-facing config API — it tightens behavior (rejecting bad input with a clear error) rather than loosening anything. No auth, crypto, filesystem, or network surface is touched.
Level of scrutiny
Low. The Zig change is ~30 lines of mechanical guard-clause additions following the exact pattern already used elsewhere in the same function (throwInvalidArguments, which I verified maps to jsc.Error.INVALID_ARG_TYPE → TypeError with code: ERR_INVALID_ARG_TYPE, matching the new test assertions). The comptime property_name parameter for contextual error messages is idiomatic Zig. The minify: false semantics now correctly set all three minify flags to false rather than crashing — this is the obviously-intended behavior and mirrors Bun.build.
Other factors
The robobun build-zig failures on 73dcb4a hit all six platforms simultaneously on scripts/build/ci.ts, which strongly suggests an infra flake rather than a code error in this ~30-line diff (and the file compiles cleanly in the checked-out tree). A duplicate-PR bot flagged #30125 as overlapping; that's a process/coordination matter for maintainers and doesn't affect the correctness of this change.
There was a problem hiding this comment.
LGTM — straightforward input validation fix that replaces a debug assertion with a proper ERR_INVALID_ARG_TYPE, with regression tests.
Extended reasoning...
Overview
This PR adds isObject() guards in src/bake/bake.zig before reading properties from bundlerOptions, bundlerOptions.{server,client,ssr}, and minify in Bun.serve({ app: ... }). It also fixes minify: false, which previously fell through the isBoolean() and asBoolean() check into the object-property path and hit the same assertion. A comptime property_name parameter is threaded through BuildConfigSubset.fromJS for clearer error messages, and regression tests are added to test/js/bun/http/bun-serve-args.test.ts.
Security risks
None. This is purely defensive input validation on a developer-facing config object. It converts a debug-build assertion into a thrown TypeError; no auth, crypto, filesystem, or network surface is touched.
Level of scrutiny
Low. The if (!x.isObject()) return global.throwInvalidArguments(...) pattern is already used throughout this same file (for config, plugin_config, framework options, etc.), and throwInvalidArguments resolves to jsc.Error.INVALID_ARG_TYPE — matching the name: "TypeError" / code: "ERR_INVALID_ARG_TYPE" contract asserted by the new tests. The only behavioral nuance is that minify: false now sets the three minify_* optionals to false rather than crashing, which is the correct interpretation. BuildConfigSubset.fromJS has no other callers, so the new comptime parameter is fully covered.
Other factors
The CodeRabbit suggestion to assert the error contract was addressed in a8653bf and the comment is resolved. CI failures (s3-storage-class, hot.test.ts, test-http-should-emit-close-when-connection-is-aborted, and the Windows x64-baseline build-zig flake that prompted the CI re-trigger commit) are unrelated to these files. No CODEOWNERS entries cover src/bake/ or this test file. A duplicate-PR bot flagged #30125 as covering similar ground, but that's a merge-coordination question for maintainers, not a correctness issue with this diff.
|
CI failures are unrelated to this change: Build #54228:
Build #52840 (pre-rebase):
|
robobun
force-pushed
the
farm/84665fac/bake-bundler-options-validation
branch
from
902bb59 to
c4fb8f4
Compare
There was a problem hiding this comment.
LGTM — straightforward input validation fix that converts a debug assertion crash into a proper ERR_INVALID_ARG_TYPE, with regression tests covering each path.
Extended reasoning...
Overview
This PR adds is_object() guards in src/runtime/bake/bake_body.rs before reading properties from bundlerOptions, bundlerOptions.{server,client,ssr}, and minify, and restructures the minify branch so minify: false no longer falls through to the object-property-reading path. BuildConfigSubset::from_js gains a property_name: &str parameter (only the three call sites in this file use it; verified no other callers). Regression tests are added in test/js/bun/http/bun-serve-args.test.ts asserting the error name, code, and message for each invalid shape, plus a positive test that minify: false parses without crashing.
Security risks
None. This is purely defensive input validation that turns a debug-assertion panic into a typed JS exception. No new attack surface, no auth/crypto/permissions involved.
Level of scrutiny
Low. The change is small (~50 LOC of logic), mechanical, and follows the existing throw_invalid_arguments pattern already used throughout this function for other invalid shapes. The one semantic change — minify: false now sets Some(false) for all three minify flags instead of crashing — is the obviously correct behavior and matches how minify: true was already handled. to_invalid_arguments uses JscError::INVALID_ARG_TYPE, so the test's { name: "TypeError", code: "ERR_INVALID_ARG_TYPE" } assertion is consistent with the implementation.
Other factors
- CodeRabbit's suggestion to assert the error contract (not just message) was addressed in a follow-up commit and the inline comment is resolved.
- robobun reports
bun-serve-args.test.tspassed on all platforms; the remaining CI failures are pre-existing flakes unrelated to this code path. - Neither modified file is covered by CODEOWNERS.
- A bot flagged PR #30125 as a potential duplicate; that's a coordination concern for the maintainers but doesn't affect the correctness of this change.
1 participant
What does this PR do?
Adds type validation for
app.bundlerOptionsinBun.serve()so that non-object values forbundlerOptions,bundlerOptions.{server,client,ssr}, and non-boolean/non-object values forminifythrow a properERR_INVALID_ARG_TYPEinstead of being silently misinterpreted via prototype-chain lookups.Also fixes
minify: false, which previously fell through to the object-property-reading path (the check wasis_boolean() && as_boolean()); it now correctly sets all three minify flags tofalse.Originally fixed a debug assertion crash (fuzzer fingerprint
c6994d6f8d88e64a) in the ZigJSValue.get()path; after #30412 moved this code to Rust the hard crash no longer reproduces, but the input was still accepted without validation. The behavior now matchesBun.build'sminifyhandling.How did you verify your code works?
Added regression tests to
test/js/bun/http/bun-serve-args.test.tscovering each invalid shape, assertingname: "TypeError"andcode: "ERR_INVALID_ARG_TYPE".