Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesBrowser fallback builds
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Browser fallback bundles now use browser-targeted polyfills, fixing Node crypto APIs in browser builds with coverage for hashing, HMAC, random bytes, PBKDF2, and AES-CBC. No merge-blocking risk is currently identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:36 AM PT - Sep 6th, 2026
❌ @robobun, your commit 33ca3a3 has 4 failures in
The baseline build contains instructions not available on Static scan violations |
|
Status: reproduced with CI: 176 of 181 jobs pass on the rebased branch. The bundler suites ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how every embedded --target=browser polyfill is produced (target node→browser, new regex-based __require stripping, +266 KB to crypto.js), a human look at the generated output would still be worthwhile.
What was reviewed:
keepBuiltinsExternalonResolve filter — matches bare andnode:-prefixed builtins, falls through for npm packages; entry files themselves aren't intercepted (onResolve fires on imports, not entrypoints).- Post-processing:
__require(→require(runs before the shim regex, and the leftover-__requireguard hard-fails the build if the shim shape drifts. - Snapshot diff in
bundler_browser.test.ts— onlypunycodechanged (gained named exports), no other module's export set was weakened. - New crypto test uses known-answer vectors (SHA-256 of "abc", HMAC-SHA1, PBKDF2) rather than self-round-trip alone.
Extended reasoning...
Overview
The PR rewrites src/node-fallbacks/build-fallbacks.ts, the build-time script that pre-bundles the ~30 Node polyfills embedded into release binaries for --target=browser. It swaps per-file Bun.$-spawned bun build --target=node invocations for in-process Bun.build({ target: "browser" }) with an onResolve plugin that keeps node builtins external. Post-processing now strips the browser __require shim via regex and hard-fails on any surviving __require. In test/bundler/bundler_browser.test.ts, a new browser/NodeCryptoPolyfill case exercises createHash/createHmac/randomBytes/pbkdf2Sync/createCipheriv with known-answer vectors, and the all-polyfills snapshot is updated because punycode now resolves through its module entry and gains named exports.
Security risks
None identified. This is build-time tooling operating on vendored npm packages under src/node-fallbacks/node_modules/; no untrusted input reaches it at runtime. The crypto polyfill is crypto-browserify — the standard browser shim — and the change makes its hash/HMAC/PBKDF2/cipher paths actually work (they previously self-recursed and threw). The known-answer test vectors are correct (verified SHA-256("abc") = ba7816bf…). No auth, permissions, or network surface is touched.
Level of scrutiny
Moderate-to-high. The script's output is zstd-compressed and embedded into every release binary, so a subtle regression here reaches every user who bundles for the browser. The target switch changes resolution semantics for every polyfill simultaneously — the PR description enumerates the observed differences (punycode exports, http.js/stream.js self-references removed, crypto.js 478 KB → 744 KB), and the NodePolyfills snapshot pins the export shapes, but a human diffing a few of the generated .js files (particularly crypto.js, http.js, stream.js) against the old output would catch anything the snapshot misses. The requireShim regex is intentionally brittle and depends on the exact minified shape of Bun's browser __require prelude; the follow-up includes("__require") guard turns drift into a build failure rather than silent breakage, which is the right design.
Other factors
The change is well-motivated (fixes a real user-facing bug where createHash et al. throw at runtime after a successful build), well-commented (the plugin comment explains why external alone can't work), and the test uses external known-answer vectors per REVIEW.md's codec guidance. The snapshot update is additive only — I diffed the old vs new lines and only punycode changed. No CODEOWNERS entry covers these paths. The PR description references #41315 (a parallel move to esbuild for this same script), which is a coordination point a maintainer should be aware of but not a blocker. The bug hunt exited on dry_streak with no findings and no ruled-out candidates.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Every node builtin stays a bare `require("x")` / `import "x"` in the output. | ||
| // The user's `--target=browser` build resolves it to the matching polyfill, so | ||
| // one `Buffer` (or `EventEmitter`, or `Stream`) is shared by all of them. | ||
| // | ||
| // The polyfills are bundled with `target: "browser"` so the npm packages | ||
| // resolve through their `browser` field. `crypto-browserify` depends on it: | ||
| // the node `main` of `randombytes`, `create-hash`, `create-hmac` and `pbkdf2` | ||
| // is `require("crypto").X`, which in a browser bundle is the polyfill itself. | ||
| // | ||
| // `external` cannot express this: with the browser target the resolver picks | ||
| // the polyfill before it checks the externals list. A plugin runs first. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `require` in the browser output is the runtime's `__require` shim. The text | ||
| // is rewritten to a plain `require(...)` below, which leaves the shim unused. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A plugin, not `external`: with the browser target the resolver picks the | ||
| // polyfill before it reads the externals list. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
build-fallbacks.ts bundled each polyfill with --target=node. That
ignores the package.json "browser" field, so the crypto-browserify
dependencies randombytes, create-hash, create-hmac and pbkdf2 resolved
to their node entry: require("crypto").X. In a browser bundle that is
the crypto polyfill itself, and createHash, createHmac, randomBytes,
pbkdf2Sync and createCipheriv threw "is not a function".
The script now calls Bun.build() with target "browser". A plugin keeps
every node builtin external, because with the browser target the
resolver picks the polyfill before it reads the externals list.
8b2ea73 to
33ca3a3
Compare
When `Bun.$.braces("")` is called with an empty string, the tokenizer
produces zero tokens. `flattenTokens` then accesses
`self.tokens.items[0]` unconditionally, causing an index-out-of-bounds
panic. Additionally, `Parser.advance()` unconditionally calls `prev()`
which underflows when `current == 0`.
The fix adds an early return when the token list is empty in
`flattenTokens`, and guards `advance()` to return `peek()` instead of
underflowing `prev()` when `current == 0`.
---
Verification (robobun, iteration 4): Build oven-sh#41524 (commit 7045ab1)
completed — 5 failing tests are all unrelated (bun-upgrade, issue/8254,
webview timeout, issue/24364, bun-types), none in shell/braces. Build
oven-sh#41555 (CI retry commit 477200b, empty) is pending. Diff: 2 files, 8
added / 1 removed — `flattenTokens` early-return guard at
braces.zig:591, `current > 0` guard in `advance()` at braces.zig:310,
regression test at brace.test.ts:53-57 covering default, parse:true, and
tokenize:true modes. Test would crash on main due to unchecked
`self.tokens.items[0]` and usize underflow in `self.current - 1`. No
TODO/FIXME/HACK in diff, no unrelated changes. CodeRabbit: no actionable
comments.
---------
Co-authored-by: SUZUKI Sosuke <sosuke@bun.com>
This branch has not been deployed
2 participants
Problem
bun build --target=browserbundle,createHash,createHmac,randomBytes,pbkdf2SyncandcreateCipherivfromnode:cryptothrowTypeError: createHash2 is not a function. Only the WebCrypto pass-throughs work. The build reports success.src/node-fallbacks/build-fallbacks.ts. It bundled each polyfill with--target=node. That ignores the package.jsonbrowserfield, so the crypto-browserify dependenciesrandombytes,create-hash,create-hmacandpbkdf2resolved to their node entry:module.exports = require("crypto").X. In the user's browser bundle thatrequire("crypto")is the crypto polyfill itself, so each function reads itself during its own initialization.Fix
Bun.build()withtarget: "browser". The npm packages now resolve through theirbrowserfield, which is how crypto-browserify is meant to be bundled.external: truefor every node builtin (buffer,stream,util, ...). With the browser target the resolver picks the polyfill before it reads theexternallist (resolver: check user externals before browser node builtin polyfills #35212), so the list alone cannot keep them external. The user's build still maps each builtin to one shared polyfill.__requireshim that the browser output carries is removed after the__require(torequire(rewrite. A leftover__requirefails the build.test/bundler/bundler_browser.test.ts(browser/NodeCryptoPolyfillfails on the old polyfills). Alsobundler_edgecase,bundler_regressions,bundler_loader,bun-build-api, and the browser regression tests.Background
src/node-fallbacks/*.jsare the modules thatbun build --target=browsersubstitutes for node builtins.build-fallbacks.tsbundles each one at build time intocodegen/node-fallbacks. Release builds embed them.browserfield in a package.json names a file to use in place ofmainin browsers, or maps a dependency to another file or tofalse. A bundler honors it for the browser target only.require("buffer")and friends as bare specifiers. If one polyfill inlined its own copy of the Buffer polyfill, a browser bundle would carry twoBufferclasses.Notes
import { createHash } from "node:crypto"; console.log(createHash("sha256").update("abc").digest("hex")), thenbun build --target=browser e.mjs --outfile=out.js && bun out.js. Before:TypeError: createHash2 is not a function. After:ba7816bf....punycoderesolves through itsmoduleentry and gains node's named exports (decode,encode,toASCII, ...). TheNodePolyfillssnapshot is updated for that.http.jsno longer containsrequire("http").STATUS_CODES(a self reference frombuiltin-status-codes) andstream.jsno longer containsrequire("stream")from readable-stream's node entry. The set of bare specifiers in every other polyfill is unchanged.crypto.jsgrows from 478 KB to 744 KB because it now carries the real hash, HMAC, PBKDF2 and cipher implementations instead ofrequire("crypto").X.createECDHandcreateSignusebrorand, which readsself.crypto. They work in a browser and under bun. Under node, which has noself, they fall back torequire("crypto")and hit the same self reference. Not changed here.assert(),util.formatandzlibin browser bundles are separate problems, covered by node-fallbacks: fix assert() and new EventEmitter() in browser bundles #41328 and node-fallbacks: fix the util polyfill and lint the polyfills for undeclared names #40072.platform: "browser"with anonResolveplugin for the builtins, ormainFields: ["browser", "module", "main"].bytecodetests inbun-build-api.test.tstime out at 5 s in the debug build with and without this change.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The browser fallbacks for Node built-ins were bundled through a pipeline that did not target the browser, so the embedded polyfills still carried a shared
__requireshim and unresolved references to Node modules, which made APIs likenode:cryptofail at runtime inbun build --target=browseroutput. The fix rebuilds the fallbacks with directBun.buildcalls using the browser target, externalizes the remaining Node built-ins, and removes the__requireshim so no Node-only resolution path survives in the emitted code. The build step now rejects any leftover reference to a Node built-…