Hardening round 11: input validation, bounds checks, lifetimes - #33072
Conversation
|
Updated 9:58 PM PT - Jul 3rd, 2026
❌ @Jarred-Sumner, your commit b42fbd3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33072That installs a local version of the PR into your bun-33072 --bun |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
|
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.
I didn't find any bugs, but this is a 200-file hardening pass touching security-sensitive paths (HTTP parsing, crypto, fs traversal, installer integrity, structured-clone deserialization), so it warrants a human reviewer.
Extended reasoning...
Overview
This PR is a broad hardening sweep across ~200 files: input validation and bounds checks in the HTTP/1 and HTTP/2 parsers, fetch/proxy request-line validation, package-installer path/tarball/integrity handling, lockfile deserialization, fs recursive copy symlink handling, crypto key validation (HKDF, WebCrypto OKP/RSA/Raw, structured-clone key revalidation), SQL/Valkey wire-protocol length checks, glob symlink-cycle detection, shell/markdown/lexer quadratic-input fixes, S3 region/host validation, inspector Host/Origin gating, and assorted lifetime/ordering fixes in Rust and C++ (AbortSignal native callbacks, h2 frame-parser stream lifetimes, BlockList structured-clone nonce table, native-plugin double-free removal). It also touches CI workflows, Dockerfiles (gpg --decrypt → --verify), and shell completions.
Security risks
Nearly every area here is security-relevant by design: request-smuggling defenses (Host validation, CONNECT precedence over Transfer-Encoding/Content-Length), redirect-scheme enforcement, decompression-bomb caps, tarball path-traversal and bin-link containment, integrity multi-hash selection, structured-clone recursion/length bounds, inspector DNS-rebinding checks, and N-API/V8 value handling. The changes are defensive (tightening validation), but any regression in these spots is high-impact, and several change observable behavior for previously-accepted-but-malformed inputs.
Level of scrutiny
High. Each change is individually small and the PR ships ~185 regression tests, but the aggregate surface is large, spans many independent subsystems, and includes behavior-visible tightening (e.g., 400 on Host with //@, glob self-symlink semantics, url.parse query prototype, mode_from_js no longer masking to 0o777, WebSocket subprotocol strictness). The diff was also truncated in my view (~447KB total), so I have not seen every hunk.
Other factors
No prior human reviews on the timeline yet, and the build is still in progress. The bug-hunting pass found nothing, but given the breadth, the security sensitivity, and the explicit behavior changes called out in the description, this should get human eyes rather than a bot approval.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates host/request validation, HTTP/2 stream accounting, installer safety, bundler ownership, SQL protocol handling, shell and URL parsing, and several runtime helpers. It also adds regression tests covering the new validation and framing behavior. ChangesTooling and install
HTTP, networking, and server
Bundler, resolver, glob, SQL, shell, and JS compat
Runtime, crypto, and WebCore
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Went through this area by area. Most of it is solid and a real improvement — the CONNECT reorder, the h2 framing checks, the lockfile/integrity tightening, and the pack/.git hardening are all good fixes. But there's one change I think can't ship in its current shape, and a cluster of items where the hardening diverges from the reference implementation (npm/ssri, md4c, esbuild, Web IDL), regresses a working path, or ships its load-bearing branch with zero test coverage.
Blocker
1. The parser-level Host-charset 400 fires for node:http and rejects requests Node delivers — with no node:http coverage
The new check in packages/bun-uws/src/HttpParser.h (~line 895 old) is gated only on !req->ancientHttp && requireHostHeader — and node:http wires requireHostHeader = true by default (src/js/node/_http_server.ts:2632 → :889 setServerCustomOptions → NodeHTTP.cpp → server_body.rs → App.h). Node v22 (llhttp) returns 200 and delivers req.headers.host for Host: a b, Host: example.com/path, Host: user@example.com, raw non-ASCII, and Host: a"b; Bun 1.3.14 matches Node on all five. The new RFC 3986 authority charset rejects all five with a connection-level 400 before the 'request' event — so Express/Fastify apps behind sloppy proxies/health-checkers break with no userland recourse (the only knob, requireHostHeader: false, also disables the presence check, and that option's Node semantics are presence-only).
The check also omits the !isConnectRequest && !req->getHeader("upgrade").data() exemption the adjacent missing-Host check carries (whose own comment explains Node dispatches those via 'upgrade'/'connect' before enforcing Host), so WebSocket upgrades and CONNECT with odd Hosts now die at the parser too. All six new tests in test/js/bun/http/request-smuggling.test.ts are Bun.serve-only.
Suggestion: don't gate the charset check on requireHostHeader. Either (a) scope the parser-level 400 to Bun.serve via a separate uws flag that node:http sets false — defensible there because Bun.serve synthesizes request.url from Host, and this PR already validates Host at the URL-synthesis layer (Request.rs), which covers Bun.serve anyway; or (b) if a parser-level reject stays, mirror the upgrade/CONNECT exemption and match llhttp's actual charset (reject control bytes only), not the RFC 3986 set. Either way, add node:http tests pinning parity with Node on the cases above.
(Related: item 20 below — the two Host validators and how they tie together.)
Should fix
2. Integrity: only the FIRST digest of the strongest algorithm is kept — order-dependent divergence from ssri/SRI
src/install/integrity.rs: if parsed.tag.0 > strongest.tag.0 (strictly greater) discards later same-algorithm entries, and the single 64-byte digest slot makes them unrecoverable at verify time (verify_by_tag). npm's ssri matches ANY digest of the strongest algorithm (checkData → find), per W3C SRI §3.3.4. So sha512-A sha512-B where the tarball matches B passes npm and hard-fails Bun, and the outcome depends on entry order — which the spec makes irrelevant. The new tests cover only mixed-algorithm strings; there's no sha512-A sha512-B case. (To be fair: pre-PR, multi-entry strings decayed to Tag::UNKNOWN and verification was silently skipped, so this is a real security improvement — it just over-tightens.)
Suggestion: keep all entries sharing the strongest algorithm; Streaming::init can hold a small bounded set (entries are bounded by string length) and verify compares against each. No lockfile format change needed. Add the two-same-algo test either way — and if pick-first is kept deliberately, note the deviation from npm in the description.
3. The glob symlink-cycle guard's --filter path is both untested and quadratic
- Coverage: the default-true
ENTRY_KIND_FOLLOWS_SYMLINKSbranch is live ONLY forDirEntryAccessor, whose sole instantiation isbun --filter(src/runtime/cli/filter_arg.rs:37). Every new cycle test (test/js/bun/glob/scan.test.ts,path-length.test.ts) goes throughBun.Glob=GlobWalker<SyscallAccessor>, whereshould_descend_resolved_dirconstant-folds toreturn true— so ~50 lines of new traversal logic (dupe_z,DirEntryAccessor::statat's relative join, theErr(_) => truefail-open, the ancestor prefix check) are dead in every tested configuration.test/cli/run/filter-workspace.test.tshas no symlink case. - Cost:
record_followed_linkonly pushes, never truncates;is_followed_link_ancestorscans the whole Vec per descent. On the DirEntryAccessor path this records EVERY directory and adds an uncachedstat(2)+ two heap allocations per descended directory — O(D²) over directory count for--filter-shaped walks in monorepos, on a cache-backed accessor whose whole point is avoiding syscalls. The resolver cache already knows symlink-ness (Entry.cache().symlink), so the stat recomputes cached information.
Suggestion: (a) snapshot followed_links.len() in each WorkItem at push and truncate on pop — the workbuf is LIFO so that's exactly the live ancestor chain; per-descent cost drops to O(depth) and the textual prefix check can go. (b) Surface the cache's symlink knowledge through AccessorDirEntry (an is_symlink() hook) so the walker only stats actual symlinks. (c) Add a --filter test with a self-referential directory symlink in a workspace — pre-fix behavior is infinite descent/ENAMETOOLONG, easy to prove with USE_SYSTEM_BUN=1.
4. validate_request_target rejects CR/LF/SP but not TAB and the rest of C0
src/http/lib.rs (~1124) checks only \r, \n, space. The guard is genuinely load-bearing — registry manifest tarball URLs, redirect Location (re-parsed with lenient URL::parse), and proxy-env URLs all bypass WHATWG normalization, as this PR's own tests show. But picohttpparser admits TAB in header values (vendor/picohttpparser/picohttpparser.c:166), so a server-controlled Location: /a\tb puts a raw TAB into the next request line — and RFC 9112 §3 lenient recipients split on whitespace including TAB. The PR description says "reject control characters"; the implementation rejects three bytes.
Suggestion: reject any byte <= 0x20 plus 0x7f (matches the scope of Node's ERR_UNESCAPED_CHARACTERS, same O(n) cost). Add \t/\x0b siblings next to the existing \n/space tests, plus a redirect-Location-with-TAB fetch test.
5. CONNECT-before-TE reorder: correct, but the two branches that actually moved are untested
The reorder closes a real smuggle path (old code chunk-decoded a CONNECT body then parsed subsequent bytes as a pipelined request). But the only new test is CONNECT + Content-Length: 0. CONNECT + Transfer-Encoding: chunked (raw bytes delivered un-decoded), CONNECT + nonzero CL with trailing GET /smuggled, and CONNECT + TE + CL → 400 (the ordering invariant at HttpParser.h:932-939) are pinned by nothing — a future "CONNECT ignores body framing" cleanup re-opens smuggling with the suite green. Three test-only additions in test/js/node/http/node-http-connect.test.ts cover it; all three match Node v22 behavior.
6. h2 dispatch_depth UAF guard is a comment-enforced global invariant — no assert, no helper, no test
src/runtime/api/bun/h2_frame_parser.rs ~5410: the drain gate is correct today (all six JS-entry wrappers plus the hand-guarded host fns arm enter_dispatch; the receive path is covered by the engine RefCell), but the audit only holds by hand. There are no debug_asserts in the h2 hunks and no test forces the depth>0 deferral — deleting the depth check or any single enter_dispatch() arm leaves the suite green. The next host fn that copy-pastes the prevailing unsafe { &mut *stream } pattern with an options getter reintroduces the exact UAF.
Suggestion: (1) a re-entrancy regression test: h2 over a JS-fed duplex, pass writeStream/request an options object whose getter synchronously feeds completion frames back into read() — removing the guard then crashes under ASAN. (2) Make the invariant structural: a guard-RAII helper that hands out the &mut Stream only inside an armed scope, replacing the scattered raw derefs.
7. Windows cpSync junction routing: right semantics, wrong symlink API — EPERM on non-elevated machines, zero Windows tests
The new reparse-point routing in node_fs.rs (~8299) correctly copies junctions/dir-symlinks as links (matching Node dereference:false and Bun's own async path). But it routes into the pre-existing branch at node_fs.rs:9102 that calls raw CreateSymbolicLinkW with only SYMBOLIC_LINK_FLAG_DIRECTORY — omitting SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE, which the in-tree sys::symlink_w/symlink_or_junction helpers (src/sys/lib.rs:8708+) already implement per the libuv pattern. Result: cpSync of a tree containing junctions (npm/pnpm/bun node_modules layouts) goes from succeeding pre-PR (dereference-copy) to EPERM on default non-elevated Windows, including Developer Mode where Node succeeds. The only new cp test is test.skipIf(!isLinux); Windows CI can create junctions unprivileged, so nothing catches this.
Suggestion: use the existing sys::symlink_w/symlink_or_junction helper (junction fallback is the highest-fidelity unprivileged copy), and add a Windows test in cp.test.ts: create a junction, cpSync recursive, assert the destination is a link to the original target.
8. The 4GiB decompression cap is wired to a drained buffer — different invariant per mode, corrupt-gzip error on trip
All three decoders check the LIVE output Vec length (src/zlib/lib.rs:1258, src/brotli/lib.rs:364, src/zstd/lib.rs:559), but that Vec aliases FetchTasklet.response_buffer, which is reset on every progress callback. In streaming mode the counter resets each drain — one socket read can't reach 4GiB through gzip's ~1032:1 ratio, while bomb output accumulates uncapped in scheduled_response_buffer. In one-shot mode it's a hard total-body cap Node/undici don't have: a legitimate >4GiB-decompressed download now fails with an error indistinguishable from corrupt gzip, no escape hatch. The security property is weakest exactly where the threat is and strictest where legitimate traffic lives.
Suggestion: (1) track cumulative decompressed bytes in the Decompressor itself (survives drains), or cap scheduled_response_buffer where accumulation actually happens; (2) a dedicated error (DecompressedBodyTooLarge) distinct from corrupt-data; (3) consider making the limit configurable (BUN_CONFIG_MAX_DECOMPRESSED_BODY_SIZE, following the max-header-size precedent).
9. url.ts: lookups hardened, but the three protocol tables still inherit from Object.prototype — and one divergence needs no pollution at all
The PR null-protos url.parse's outputs but leaves unsafeProtocol/hostlessProtocol/slashedProtocol (src/js/node/url.ts:78-99) as plain literals. Checked against Node three ways: resolveObject with protocol:'constructor' hits Object.prototype.constructor with NO pollution (Bun drops the host; Node keeps it); Object.prototype['evil:']=true changes parse structure; Object.prototype['weird:']=true defeats the unsafeProtocol guard and skips the XSS auto-escape loop. Node hardened these exact tables (SafeSet).
Suggestion: add __proto__: null to the three literals — three one-line edits in a file this PR already touches — plus a resolveObject-'constructor' regression test and one pollution test. Side note: Bun's slashedProtocol omits ws/wss, which Node includes.
10. md4c budget: matches neither upstream's formula nor — more importantly — its failure mode
Upstream md4c is min(16*size, 1MB) (md4c.c:7216) and on exhaustion renders refs as literal TEXT — parse succeeds (mity/md4c#238). This PR ships (16*size).max(1MB).min(16MB) (src/md/parser.rs:246, a shape that never existed upstream) and THROWS (src/md/links.rs:548,581 → MarkdownObject.rs:62). Charges accrue per use, so a ~30KB changelog with one long-URL definition referenced a few thousand times exceeds the floor — md4c/cmark/GitHub all render it, Bun.markdown.html() throws. The new test enshrines the divergent throw, and the PR description's "upstream md4c output budget" is wrong on both constants and semantics.
Suggestion: match md4c's degradation — zero the budget and return Ok(None) at the two charge sites (identical guards already sit adjacent), delete the new ParserError variant. Keep the looser clamp with an honest comment, or restore the exact upstream formula. Update the test to assert unlinked-text degradation, not a throw.
11. h2 refused-stream: benignity of pipelined frames hangs on an uncommented, untested invariant
All five frame types on a refused id are benign today, but RST_STREAM tolerance works ONLY because the last_stream_id advance stayed outside the !refused gate (connection.rs:797-801). The obvious-looking future cleanup — folding that block into the adjacent refusal-gated arm — turns pipelined RST_STREAM on a refused id into GOAWAY(PROTOCOL_ERROR), tearing down every live stream exactly when the server is under memory pressure. The sole refusal test sends no pipelined frames.
Suggestion: extend the maxSessionMemory test to pipeline DATA/WINDOW_UPDATE/PRIORITY/RST_STREAM behind the refused HEADERS in one write, assert no GOAWAY and that a subsequent stream completes; add a CONTINUATION variant asserting HPACK state stays in sync; one-line comment on the last_stream_id advance naming why it must stay refusal-independent.
12. __proto__ printer fix is at the wrong layer — printer-global flag instead of esbuild's per-property marker; the macro serializer is a live missed sibling
The was_lazy_export gate (src/js_printer/lib.rs:~4762) has to be re-wired at every printer entry point — this PR patches the THIRD such site (print_common_js), which is the fragility demonstrating itself. esbuild fixes this at the parser layer: json_parser.go:147-152 sets the computed-property flag on the "__proto__" property itself. And the flag approach misses src/js_parser_jsc/Macro.rs:791-796: a macro returning JSON.parse('{"__proto__":{...}}') serializes into the user's tree (flag false) as a prototype-setting literal — same bug class, unfixed, even though was_originally_macro exists right there.
Suggestion: set Property::IsComputed on "__proto__" string keys where the data parsers (json/json5/jsonc/toml/yaml) build EObject properties (small shared helper), and in the Macro.rs serialization. The printer flag then disappears and every current and future print path is correct.
13. Valkey: is_reply_kind is the only thing keeping psubscribe acks from desyncing the pipeline — and it's untested
The push-routing half is fine and tested. But SUBSCRIPTION_PUSH_MESSAGES covers only message/subscribe/unsubscribe (valkey_protocol.rs:805-809), so psubscribe/punsubscribe/ssubscribe/sunsubscribe acks rely solely on the new is_reply_kind() to consume their promise pair. Delete it or misspell a kind (there's a b"UNPSUBSCRIBE" typo precedent in ValkeyCommand.rs:170) and psubscribe's promise never settles while every subsequent pipelined reply resolves the wrong command — the exact desync class this PR fixes — with the suite green. Also: the b"subscribe"|b"unsubscribe" arms inside is_reply_kind are unreachable (those go through the push set).
Suggestion: one mock-server test sending a >3 psubscribe push ack followed by the next command's reply; assert both settle correctly. Drop the unreachable arms or derive the set from SUBSCRIPTION_PUSH_MESSAGES so there's one source of truth.
14. Record fast-path snapshot diverges from Web IDL/Node/Chrome/Safari/old-Bun — and the new tests codify it undisclosed
JSDOMConvertRecord.h: values are collected via getDirect BEFORE any Converter<V>::convert runs; the spec (and the untouched slow path, and upstream WebKit, which has no fast path at all) interleaves Get with conversion. A toString that mutates a sibling property now sees stale values and includes deleted properties — in new Headers(obj), URLSearchParams, fetch, WebSocket. Node returns second:"replaced" with third excluded; the new headers.test.ts asserts second:"second", third:"third". Worse, behavior now depends on hidden-class state (literal → fast path → snapshot; getter-bearing → slow path → spec order). Not mentioned in the Behavior notes.
Suggestion: keep the safety fix, preserve spec order — snapshot only the identifiers inside forEachProperty (no user JS runs there), convert interleaved with a per-property structure() comparison, falling back to the slow path on transition. Or mirror upstream WebKit and drop the fast path. Either way, flip the two new tests to assert spec-interleaved results.
15. bun pm pack: pre-include exclusion applies the full overridable default-ignore list, dropping files npm includes — tests bake the divergence in
The new is_excluded(..., &[]) call in iterate_included_project_tree (pack_command.rs) makes the entire DEFAULT_IGNORE_PATTERNS list a hard exclude that explicit "files" entries can't reach — can_override is dead with the empty slice. npm 10.9.3: files:["lib",".gitignore","bunfig.toml","npm-debug.log",".DS_Store"] packs all of those and excludes only the documented non-overridable set (.git/.npmrc/lockfiles). The two new bun-pack tests assert the opposite.
Suggestion: restrict the pre-include check to the can_override=false set (plus ROOT_DEFAULT_IGNORE_PATTERNS at depth 1) — that keeps the real hardening wins (.git/.npmrc/lockfile leak via "files", no traversal into .git) while preserving npm's documented override rule. Update the two tests.
16. wrapAnsi lastRowWidth cache assumes additive widths across the append boundary — grapheme clustering makes that false
The deleted recompute (rows.last().width() every iteration) became lastRowWidth = rowLength + wordLen, but GraphemeState::width() (stringWidth.cpp:455-490) deliberately overrides codepoint sums for clusters (keycap, RI pairs, ZWJ, VS16). On shipped Bun: Bun.stringWidth("ab ⃣xx") = 6 while the additive parts sum to 7 — the space clusters with a word-initial U+20E3. So a change billed as pure perf changes wrap decisions near the column limit for word-initial combining marks/keycap/VS16 — real decomposed-Unicode text. Only perf tests were added.
Suggestion: fall back to a full recompute whenever the appended word's first codepoint isn't a guaranteed cluster-break class (one fusedClassify check; preserves the ASCII fast path exactly). Add a differential equivalence test over the trim×wordWrap×hard matrix with ANSI + tabs + word-initial combining marks.
17. invalid_specifier_if_too_long! — the only guard against a release-build panic on multi-* pattern targets — is dead under both new tests
The pre-PR panic is real (slice-bounds panic in bun_core::replace writing into the fixed PathBuffer) and the two-site coverage in resolve_target is complete (package_json.rs:2612,2726; imports funnels through the same fn). But for a single * the coarse pre-check always fires first; the macro is live ONLY for multi-* targets — and both new tests use an 8,192-byte target that trips the pre-check before any replace. Delete both macro invocations and the suite stays green, leaving a user-reachable panic (DoS via package.json).
Suggestion: add a test sized to pass the pre-check but blow up on expansion — e.g. exports: {"./*": "./" + "*/".repeat(100) + "x"} with a few-hundred-byte specifier (under macOS's 1024-byte MAX_PATH_BYTES). Verify it panics with USE_SYSTEM_BUN=1. Add an imports sibling, and assert the error code instead of a bare catch.
18. The is_empty branch in leak_list_into_uint8array silently fixes a reachable pre-PR invalid free — undisclosed, untested for that reason
Pre-PR, Bun.gunzipSync(Bun.gzipSync("")) registered mi_free_ctx with a dangling NonNull::dangling() ctx freed at GC (BunObject.rs → array_buffer.rs → ArrayBuffer destructor; under ASAN it's free(0x1) → abort). The PR fixes this as an unlabeled side effect of a refactor billed as deduplication — Bun.gc appears zero times in the diff, and the only tests reaching empty output fail pre-PR for an unrelated ordering reason. Nothing prevents the next refactor from reintroducing it.
Suggestion: regression test next to the new zstd.test.ts block — empty gzip/deflate roundtrips, drop refs, loop Bun.gc(true); aborts pre-PR under ASAN/debug, passes post-PR. One sentence in the Behavior notes naming the fixed GC-time invalid free so the branch reads as load-bearing, not speculative.
Consider
19. http2 allowHTTP1 fallback: flat-array fix is correct and overdue, but it half-implements the NUL-sentinel convention
The old destructuring loop emitted garbage since #31584. The new code skips the "\0" pair but discards its "1" (close-delimited) / "2" (no-body) payload semantics that NodeHTTP.cpp:721-733 honors — so HEAD or close-delimited responses through Http2SecureServer({allowHTTP1:true}) get different wire framing than the same handler on plain https. Either implement the two flags (~10 lines mirroring NodeHTTP.cpp) or comment the skip site stating the omission is intentional; a HEAD-framing-parity test through the fallback would pin it.
20. Both Host validators are live but nothing documents why, and the two hand-written charsets have nothing binding them
The Rust check (Request.rs ~904) covers HTTP/1.0 and requireHostHeader=false where the parser check is skipped; the empty-Host divergence (parser accepts per RFC 3986 reg-name, Rust rejects because http:///path isn't synthesizable) is deliberate and load-bearing. Don't merge them across the FFI boundary — instead: short comments at each site naming the other and stating reachability, plus tests for empty Host: on HTTP/1.1 and a charset-parity test so the two sets can't drift silently. Resolve alongside item 1.
21. ServerRouteList param decoding: behavior change is correct but unpinned at the user-visible layer
Raw non-ASCII route-segment bytes now decode as UTF-8/U+FFFD instead of release-build Latin1 mojibake — the right fix, and CookieMap is pinned end-to-end. But req.params is public API and only byte-level bun:internal-for-testing tests exist. One raw-socket test in bun-serve-routes.test.ts against a /:id route would do it: raw UTF-8 bytes → "é", lone 0xE9 → "�".
22. WebSocket missing-protocol enforcement is the PR's most user-visible compat break, and it's absent from the Behavior notes
Clients requesting subprotocols now fail with close 1002 "Missing client protocol" when the server omits the echo (previously connected with protocol === ""), and protocol-stripping proxies are a known real-world pattern. The implementation checks out (both enforcement halves, the empty-value edge, the 1002 mapping). One sentence in the Behavior notes, no code change.
Checked and held up
For completeness, angles I dug into that turned out fine as shipped: the http.request CONNECT href-validation breadth (live, load-bearing, only rejects RFC-invalid input); the IPC serialize-then-write double buffer (justified by start_message's failure modes); h2 duplicate/HEAD/204 content-length handling and GOAWAY edges (match RFC 9113); lockfile script-byte rejection vs old lockfiles (migration unaffected); certerrors keepalive pool scoping; the relative-Location redirect scheme check; inspector auth ordering; zlib option-coercion error ordering; TextDecoder shared-copy completeness; napi byteOffset consistency; AbortSignal dispatch-list lifetime.
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Went through this area by area (reposted from the earlier single-comment review as per-hunk threads so they can be resolved independently). Most of it is solid and a real improvement — the CONNECT reorder, the h2 framing checks, the lockfile/integrity tightening, and the pack/.git hardening are all good fixes. 22 inline threads: 1 tagged [blocker] (the parser-level Host-charset 400 breaks node:http parity with Node and has no node:http coverage), 17 [should-fix] (mostly: divergence from the reference implementation — npm/ssri, md4c, esbuild, Web IDL — or load-bearing branches with zero coverage, or regressions on a working path), 4 [consider].
Angles I dug into that turned out fine as shipped, for completeness: http.request CONNECT href-validation breadth (live, load-bearing, only rejects RFC-invalid input); the IPC serialize-then-write double buffer (justified by start_message's failure modes); h2 duplicate/HEAD/204 content-length handling and GOAWAY edges (match RFC 9113); lockfile script-byte rejection vs old lockfiles (migration unaffected); certerrors keepalive pool scoping; the relative-Location redirect scheme check; inspector auth ordering; zlib option-coercion error ordering; TextDecoder shared-copy completeness; napi byteOffset consistency; AbortSignal dispatch-list lifetime.
| let mut strongest = Integrity::default(); | ||
| for entry in buf.split(|c: &u8| c.is_ascii_whitespace()) { | ||
| let parsed = Self::parse_entry(entry); | ||
| if parsed.tag.0 > strongest.tag.0 { |
There was a problem hiding this comment.
[should-fix] Strictly-greater keeps only the FIRST entry of the strongest algorithm — order-dependent divergence from ssri/SRI.
Later same-algorithm entries are discarded, and the single 64-byte digest slot makes them unrecoverable at verify time (verify_by_tag). npm's ssri matches ANY digest of the strongest algorithm (checkData → find), per W3C SRI §3.3.4. So sha512-A sha512-B where the tarball matches B passes npm and hard-fails Bun, and the outcome depends on entry order — which the spec makes irrelevant. The new tests cover only mixed-algorithm strings; there's no sha512-A sha512-B case. (To be fair: pre-PR, multi-entry strings decayed to Tag::UNKNOWN and verification was silently skipped, so this is a real improvement — it just over-tightens.)
Suggestion: keep all entries sharing the strongest algorithm; Streaming::init can hold a small bounded set (entries are bounded by string length) and verify compares against each — no lockfile format change needed. Add the two-same-algo test either way, and if pick-first is kept deliberately, note the deviation from npm in the PR description.
There was a problem hiding this comment.
Still unaddressed in the fix batch — no change to integrity.rs and no two-same-algo test (the new bun-install.test.ts block is the tarball-URL control-char test for the other thread). The ask stands: match ssri by keeping all digests of the strongest algorithm and verifying against each, or test + document pick-first as a deliberate divergence from npm.
There was a problem hiding this comment.
Bump — this is now the only thread untouched across all three fix waves (no code change, no test, no reply). Restating the ask: Integrity::parse keeps only the FIRST digest of the strongest algorithm, so sha512-A sha512-B where the tarball matches B fails in Bun but passes npm/ssri (any-match over the strongest set, per W3C SRI §3.3.4) — and the outcome depends on entry order, which the spec makes irrelevant. Either keep all strongest-algorithm digests and verify against each (Streaming::init can hold a small bounded set; no lockfile change needed), or add the two-same-algo test asserting pick-first and note the divergence in the PR description. The latter is fine if it's a deliberate call — it just needs to be visible.
There was a problem hiding this comment.
Yes, opened #33700 to track it.
On scope: holding a bounded set only in Streaming::init doesn't quite reach on its own. By the time Streaming::init runs, tarball.integrity is already the single 65-byte Integrity read back from PackageVersion (binary manifest cache) or Meta (lockfile); the raw SSRI string was discarded at Integrity::parse in npm.rs:2587 / bun.lock.rs / migration.rs / pnpm.rs / yarn.rs. So the manifest-cache side needs to carry the extra digests (format bump), which is the shape Jarred described above. The text lockfile can stay single-digest since we'd write the one that matched.
Happy to pick it up as the follow-up PR if you want; say the word on #33700.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Thanks for the depth here — every item was re-verified against the reference you named (ssri, md4c, esbuild, Web IDL, real Node, npm) before anything changed. Point by point:
|
| { | ||
| const spkacText = spkacValid.toString('utf8').trimEnd(); | ||
| const padded = Buffer.alloc(Buffer.byteLength(spkacText) + 1); | ||
| padded.write(spkacText); | ||
| const zeroLengthView = padded.subarray(0, 0); | ||
| assert.strictEqual(Certificate.verifySpkac(zeroLengthView), false); | ||
| assert.strictEqual(Certificate.exportPublicKey(zeroLengthView), ''); | ||
| assert.strictEqual(Certificate.exportChallenge(zeroLengthView), ''); | ||
|
|
||
| for (const input of [Buffer.alloc(0), Buffer.from(' \n\r\t'), '', ' \n\r\t']) { | ||
| assert.strictEqual(Certificate.verifySpkac(Buffer.from(input)), false); | ||
| assert.strictEqual(Certificate.exportPublicKey(input), ''); | ||
| assert.strictEqual(Certificate.exportChallenge(input), ''); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 This adds a Bun-authored regression block to test/js/node/test/parallel/test-crypto-certificate.js, but test/js/node/test/parallel/CLAUDE.md says "These are Node.js compatibility tests not written by Bun, so we cannot modify these tests" — local additions here will conflict with or be silently lost on the next upstream Node.js test sync. The assertions themselves are fine; just move the block to a Bun-owned file (e.g. test/js/node/crypto/crypto-certificate.test.ts).
Extended reasoning...
What this is. The PR's regression-test commit (13aff18) adds a 15-line block at test/js/node/test/parallel/test-crypto-certificate.js:109-123 covering the new empty/whitespace-only SPKAC input handling in ncrypto.cpp (VerifySpkac / ExportPublicKey / ExportChallenge). The block constructs a zero-length view, an empty buffer, and whitespace-only inputs and asserts that all three SPKAC helpers return the empty/false result instead of passing a zero-length pointer to NETSCAPE_SPKI_b64_decode.
Why placement matters. test/js/node/test/parallel/CLAUDE.md:7 reads, verbatim: "These are Node.js compatibility tests not written by Bun, so we cannot modify these tests". This directory mirrors nodejs/node/test/parallel and is periodically re-synced from upstream; git log confirms it is rarely touched outside vendor updates, so the convention is enforced in practice, not merely aspirational. A locally-added block in a vendored file is exactly what the next update-vendor / re-sync will clobber or turn into a merge conflict — and because the file is run as a standalone script (exits 0 on success) rather than through the Bun test runner, there is no describe/it scaffolding to make a 3-way merge trivial.
Step-by-step.
test/js/node/test/parallel/CLAUDE.mdexists and prohibits local edits to files in this directory.- Upstream
nodejs/node/test/parallel/test-crypto-certificate.jsdoes not contain the lines 109-123 block — it ends after the "Test static methods" /stripLineEndingssection. The new block is Bun-authored (commit 13aff18 "Add regression tests across the touched subsystems"). - On the next upstream sync, one of two things happens: (a) the file is overwritten wholesale and the regression test silently disappears, leaving the
ncrypto.cppif (length == 0) return ...;guards untested; or (b) the sync produces a textual conflict at lines 108-124 that someone has to resolve by hand. - Either outcome is the failure mode the directory's CLAUDE.md exists to prevent.
Why nothing else covers it. Every other regression test in this PR's test commit lives in a Bun-owned file (test/js/node/crypto/*.test.ts, test/js/bun/..., etc.); this is the only edit under test/js/node/test/parallel/. There is no existing test/js/node/crypto/crypto-certificate.test.ts, so the assertions have nowhere else to land without creating one — which is presumably why they ended up here.
Impact. No runtime impact — the assertions are correct and pass. This is a test-organization / maintainability issue: the regression coverage for the ncrypto.cpp SPKAC empty-input guards is at risk of being lost on the next vendor sync, and the edit violates an explicit, documented project convention.
Fix. Move lines 109-123 verbatim into a new Bun-owned file, e.g. test/js/node/crypto/crypto-certificate.test.ts:
import { expect, test } from "bun:test";
import { Certificate } from "node:crypto";
test("Certificate SPKAC helpers handle empty and whitespace-only input", () => {
for (const input of [Buffer.alloc(0), Buffer.from(" \n\r\t"), "", " \n\r\t"]) {
expect(Certificate.verifySpkac(Buffer.from(input))).toBe(false);
expect(Certificate.exportPublicKey(input)).toBe("");
expect(Certificate.exportChallenge(input)).toBe("");
}
});and revert test/js/node/test/parallel/test-crypto-certificate.js to its upstream content.
| if (i > 0) controller.write("'" + toSingleQuote(decoded) + "');__bun_f.push("); | ||
| controller.write('Uint8Array.from(atob("'); | ||
| for (; i < chunks.length; i++) { | ||
| const chunk = chunks[i]; |
There was a problem hiding this comment.
🟣 Pre-existing: the multi-chunk base64 fallback this catch block falls through to (line 358) writes base64.slice(1, -1) — stripping the first and last character of every chunk's base64 encoding before concatenating them inside one atob("…"), which is undecodable on the client (and even without the slice, concatenating per-chunk btoa() outputs is only valid when every prior chunk's byte length is a multiple of 3). The single-chunk sibling at line 326 does this correctly with no slice; since this PR already rewrites line 353 in the same catch block, it'd be a one-line fix to also drop the .slice(1, -1) and base64-encode the concatenated remaining bytes once.
Extended reasoning...
What the bug is. In writeManyFlightScriptData's catch branch — the very block this PR refactors at line 353 — the per-chunk base64 fallback loop at line 358 does controller.write(base64.slice(1, -1)). btoa() returns a plain base64 string with no surrounding delimiters (the " quotes are written separately on lines 354 and 360), so .slice(1, -1) strips the first and last data character of every chunk's encoding before concatenating them into a single atob("…") call on the client. The first character of a base64 encoding is always real data, never padding, so this is unconditionally wrong. The single-chunk sibling writeSingleFlightScriptData does the same operation at line 325–326 with no slice, confirming this is a bug rather than an intentional delimiter-strip.
Step-by-step proof. Take two buffered RSC chunks A = Uint8Array.of(0xFF, 0x61, 0x62) and B = Uint8Array.of(0x63, 0x64), with the streaming decoder constructed at line 144 as new TextDecoder('utf-8', { fatal: true }):
drainRscChunkscallswriteManyFlightScriptData([A, B], decoder, controller)(line 219).chunks.length === 2, so the early return at line 339 does not fire.- Loop iteration
i = 0:decoder.decode(A, {stream:true})throws (0xFFis never valid UTF-8). Control enters the catch withi === 0,decoded === "". - Line 353 (
i > 0) is false, so the catch writesUint8Array.from(atob("(line 354). - Iteration
i = 0:btoa(String.fromCodePoint(0xFF, 0x61, 0x62)) === "/2Fi";.slice(1, -1)→"2F"written. - Iteration
i = 1:btoa(String.fromCodePoint(0x63, 0x64)) === "Y2Q=";.slice(1, -1)→"2Q"written. - Line 360 closes with
"),m=>m.codePointAt(0))</script>. The client receivesatob("2F2Q"), which decodes to bytes[0xD8, 0x5D, 0x90]— nothing like the original[0xFF, 0x61, 0x62, 0x63, 0x64]. The inline flight payload is corrupted; React's client flight reader fails.
Even with the .slice(1, -1) removed there is a second layer of breakage: concatenating independent btoa() outputs into one atob() input is only valid when every non-final chunk's byte length is a multiple of 3 (otherwise the prior chunk emits = padding mid-string, which atob rejects). In the example above, chunk B is 2 bytes → "Y2Q=", and atob("/2FiY2Q=") happens to work only because B is last; swap the order and atob("Y2Q=/2Fi") throws.
Why nothing prevents it. The catch path is reachable: the shared decoder used at line 219 is constructed with { fatal: true } (line 144), so decode(chunk, {stream:true}) throws on any invalid-UTF-8 byte sequence in the RSC payload (React Flight can carry binary typed-array rows; the surrounding code's own comment at line 351 — "Since this is rare" — acknowledges the path exists). And chunks.length === 1 doesn't always short-circuit: rscChunks accumulates via push at line 249 whenever HTML is mid-flow, so multiple chunks routinely arrive before drainRscChunks runs. The PR's own change at line 353 sits one line above the broken loop in the same catch block, so this is squarely "code the PR interacts with".
Impact. When the catch path is hit with ≥2 buffered chunks, the inline __bun_f.push(Uint8Array.from(atob('…'))) script the SSR stream embeds is undecodable on the client — either atob throws (mid-string =), or it decodes to garbage bytes that React's flight client rejects. Hydration of that part of the page fails. Rare (RSC payloads are React-generated UTF-8; reaching this needs binary content in the flight stream), but a hard correctness fault when it does.
Fix. Mirror the single-chunk path: base64-encode the concatenated remaining bytes once, with no slice. E.g.
const remaining = Buffer.concat(chunks.slice(i));
const base64 = btoa(String.fromCodePoint(...remaining));
controller.write(`Uint8Array.from(atob("${base64}"),m=>m.codePointAt(0))</script>`);(or accumulate into one Uint8Array and call btoa once). This is a pre-existing bug the PR did not introduce — flagging it because the PR already rewrites the line immediately above it in the same function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 41
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/runtime/shell/shell_body.rs (1)
999-1026: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape keyword fragments, not only complete keywords.
Exact checks miss cases like
$`i${"f"} true; then echo x; fi`or$`el${"if"} ...`where an interpolated fragment combines with literal text into an if-clause keyword. The interpolated value should be routed throughappend_js_str_refwhen it can participate in formingif,else,elif,then, orfi, or the builder needs adjacent-literal context before deciding.🤖 Prompt for 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. In `@src/runtime/shell/shell_body.rs` around lines 999 - 1026, The keyword escaping check in append_utf8 only detects complete tokens, so interpolated fragments that combine with adjacent text can still form shell control keywords. Update the logic around needs_escape_utf8_ascii_latin1 and IfClauseTok::from_text in shell_body.rs so append_js_str_ref is used whenever the current fragment can contribute to forming if, else, elif, then, or fi with neighboring content, or make the builder inspect adjacent-literal context before choosing the UTF-8 fast path.test/js/bun/util/filesystem_router.test.ts (1)
685-711: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPrecompute the collision instead of searching for it in the test.
This 600k-iteration search is what forces the new
60_000timeout, andtest/js/bun/**explicitly avoids per-test timeouts. Check in a known colliding pair (or generate it once offline) so the regression stays deterministic and fast. As per coding guidelines,test/js/bun/**should not add explicit per-test timeout arguments.🤖 Prompt for 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. In `@test/js/bun/util/filesystem_router.test.ts` around lines 685 - 711, The filesystem router collision test currently brute-forces a hash collision with a long loop and adds an explicit per-test timeout, which should be avoided in test/js/bun/**. Replace the search in the Bun.FileSystemRouter test with a precomputed known colliding pair (or a fixed fixture generated offline) so the test is deterministic and fast. Keep the existing assertions around route matching and non-matching behavior, but remove the 60_000 timeout and the runtime collision search logic.Source: Coding guidelines
src/runtime/webview/WebViewHost.cpp (1)
717-732: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the outer
bunConsolepayload before sending Objective-C selectors.Lines 717-720 still call
objc::NSString(type).toWTF()andNSArray::count()before any guard ontypeorargs. A page can callwebkit.messageHandlers.bunConsole.postMessage(...)with arbitrary bridged objects, so this can still crash onunrecognized selectorbefore the per-element check runs. As per coding guidelines, "Validate untrusted input BEFORE any processing, allocation, or side effect" and "Assume userland is hostile on security-relevant paths."Suggested fix
- WTF::CString typeC = objc::NSString(type).toWTF().utf8(); + if (!objc::Ref(type).isKindOf(objc::NSString::cls)) type = nullptr; + if (!objc::Ref(args).isKindOf(objc::NSArray::cls)) args = nullptr; + WTF::CString typeC = objc::NSString(type).toWTF().utf8(); uint32_t typeLen = static_cast<uint32_t>(typeC.length()); objc::NSArray arr(args); uint32_t argCount = args ? static_cast<uint32_t>(arr.count()) : 0;🤖 Prompt for 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. In `@src/runtime/webview/WebViewHost.cpp` around lines 717 - 732, The outer bunConsole payload is still being dereferenced before validation in WebViewHost::postMessage handling, so arbitrary bridged objects can trigger Objective-C selector crashes. Add an upfront guard for both type and args before calling objc::NSString(type).toWTF() or NSArray::count(), and bail out early unless the payload is the expected NSString/NSArray shape. Keep the existing per-element checks in the loop, but make the initial payload validation happen first so no processing or allocation occurs on untrusted input.Source: Coding guidelines
src/runtime/bake/DevServer/HmrSocket.rs (1)
124-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnsubscribe branch is unreachable — uWS never unsubscribes dropped topics.
The
else ifon Line 168 repeats the exactifcondition from Line 126 (new_bits.contains(bit) && !self.subscriptions.contains(bit)), so thews.unsubscribe(&field.uws_topic())you updated on Line 172 can never run.on_unsubscribe(Line 323) only adjusts visualizer counters and does not callws.unsubscribe, so when a client drops a topic the socket stays subscribed at the uWS layer and keeps receiving that topic's messages even thoughself.subscriptionsis updated. The in-tree comment already marks this as likely a bug.🐛 Proposed fix to the unsubscribe condition
- } else if new_bits.contains(bit) && !self.subscriptions.contains(bit) { - // Note: this `else if` condition is identical to the `if` - // above and is therefore unreachable; likely a bug - // (intended: `!new && old` → unsubscribe). + } else if !new_bits.contains(bit) && self.subscriptions.contains(bit) { let _ = ws.unsubscribe(&field.uws_topic()); }🤖 Prompt for 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. In `@src/runtime/bake/DevServer/HmrSocket.rs` around lines 124 - 176, The unsubscribe path in HmrSocket::handle_subscription_changes is unreachable because the `else if` repeats the same condition as the subscribe branch, so dropped HmrTopic values never call `ws.unsubscribe`. Update that branch to check for topics present in `self.subscriptions` but absent from `new_bits`, and keep the unsubscribe call there. Make sure the logic still preserves the existing subscribe-side hooks for `feature_flags::BAKE_DEBUGGING_FEATURES` and leaves `self.on_unsubscribe` as counter cleanup only.
🤖 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 `@completions/bun.bash`:
- Around line 50-52: The script parsing in the bun.bash completion logic is
trimming using the last colon, which breaks script names that contain
colon-separated commands. Update the completion extraction in the scripts loop
to split on the first ":" in the command text before the regex check, so entries
like dev: bun --hot http://localhost:3000 are preserved and added to
package_json_compreply.
In `@packages/bun-uws/src/HttpParser.h`:
- Around line 707-713: `isValidHostFieldValue()` currently only checks allowed
bytes and can accept malformed host strings like extra colons or non-numeric
ports. Update the host validation logic in `HttpParser::isValidHostFieldValue`
to enforce the actual `uri-host [ ":" port ]` structure, including proper
bracketed IPv6 handling, rejecting invalid unbracketed colon placement, and
requiring a numeric port when present. Mirror the same structural validation in
`Request::is_valid_host_header` so malformed authorities cannot be used to
synthesize invalid `Request.url` values.
In `@src/glob/GlobWalker.rs`:
- Around line 967-973: The `GlobWalker` follow-link path is passing `Stat` by
value into `check_followed_link`, which triggers the clippy gate because `Stat`
is too large to copy eagerly. Update the `check_followed_link` call site in
`GlobWalker` to borrow the `Stat` for validation, then only copy/clone the
accepted link when assigning `followed_link = Some(link)`. Apply the same
pattern in the other affected `GlobWalker` match arms that use
`check_followed_link` so the borrow-only check is consistent.
In `@src/install/lockfile.rs`:
- Around line 3317-3321: The in_trusted_dependencies helper currently returns
true from the truncated hash lookup alone, which can incorrectly trust a
colliding package name. Update the trusted dependency check in
lockfile::in_trusted_dependencies to verify the stored exact name bytes,
matching the fail-closed behavior already used by has_trusted_dependency, so
only an exact name match can be treated as trusted before extraction logic runs.
In `@src/js_printer/lib.rs`:
- Line 8301: The dev-server lazy-export path is still missing the
`was_lazy_export` state, so `print_dev_server_module` can emit
`ast.has_lazy_export` through `print_expr` without the protection that
`print_common_js` already gets. Update the dev-server printer to wire
`tree.has_lazy_export` into the same `was_lazy_export` flag on the relevant
printer state, using the `print_dev_server_module` flow and the shared printer
setup around `printer.was_lazy_export`, so both code paths handle data-file
`__proto__` consistently.
In `@src/jsc/bindings/CookieMap.cpp`:
- Around line 197-200: The cookie prefix check in CookieMap::findOrCreateCookie
delete-tombstone handling is too permissive because it uses case-insensitive
matching for the "__Secure-" and "__Host-" prefixes. Update the secure-flag
detection in the CookieMap logic to use exact case-sensitive prefix checks so
only cookies with the canonical prefixes are treated as special, ensuring the
tombstone created via Cookie::create preserves the correct Secure attribute for
names like "__secure-id".
In `@src/jsc/bindings/decodeURIComponentSIMD.cpp`:
- Around line 22-31: The current decode path in decodeURIComponentSIMD appends
each literal run immediately, which breaks mixed byte sequences split between
raw bytes and %XX bytes. Update the shared decoding flow around appendLiteralRun
and the percent-decoding loop to keep a pending byte buffer for the active run,
append both literal bytes and decoded percent bytes into that buffer, and only
UTF-8-decode once per combined run before writing to the StringBuilder. Make the
same change in the other affected decode helpers so byte callers like mixed raw
0xC3 plus %A9 are emitted as one merged UTF-8 sequence instead of separate
replacement characters.
In `@src/jsc/bindings/node/crypto/JSECDHConstructor.cpp`:
- Around line 90-100: The key buffer obtained in JSECDHConstructor::construct
via getArrayBufferOrView is only kept as a raw view, but later coercions like
JSECDH::getFormat and curveValue.toWTFString can run JS and invalidate it. Root
the JSArrayBufferView (or copy its bytes into owned storage) before those calls,
then read the span only after liveness is guaranteed so the temporary buffer
cannot be reclaimed mid-construction.
In `@src/jsc/bindings/webcrypto/CryptoKeyOKP.cpp`:
- Around line 148-157: Update the shared OKP allowlist so X25519 is accepted
consistently across all consumers. In CryptoKeyOKP, extend isValidOKPAlgorithm()
to return true for X25519 as well as Ed25519, since
SerializedScriptValue::readOKPKey() depends on it before performing its
curve/algorithm cross-check. Keep the validation logic in sync with the new
X25519 branch so structured-cloned OKP keys are not rejected.
In `@src/md/parser.rs`:
- Around line 191-199: The budget check in charge_ref_def_output is too strict
because it rejects exact-fit reference definitions; update the comparison so a
reference whose combined dest_len and title_len exactly matches
max_ref_def_output is accepted, while still zeroing the budget and returning
false only when the combined size exceeds the remaining allowance.
In `@src/runtime/api/BunObject.rs`:
- Around line 2376-2406: The libdeflate success paths still manually convert a
Vec into a Uint8Array and register global_deallocator, which bypasses the
empty-Vec safety in leak_list_into_uint8array. Update the libdeflate branches in
BunObject::leak_list_into_uint8array callers to reuse
leak_list_into_uint8array(global_this, list) for successful decompression
results so empty outputs are handled without a dangling pointer or GC crash.
In `@src/runtime/bake/bun-framework-react/ssr.tsx`:
- Around line 342-349: The streaming decode path in ssr.tsx leaves the final
buffered UTF-8 bytes unflushed, so the combined payload can be truncated when
the last chunk ends mid-sequence. Update the chunk-combining logic in the
decoder loop to flush the TextDecoder state with a final decode after processing
the chunks, and make sure the fallback path that uses base64 also includes any
buffered tail bytes. Use the existing decoder and controller.write flow to keep
the output consistent.
In `@src/runtime/bake/DevServer/ErrorReportRequest.rs`:
- Around line 570-575: The comment in ErrorReportRequest should be shortened to
fit the 3-line limit while keeping the key behavior note. Trim the explanatory
text near the UTF-8 handling logic so the guidance stays concise, and preserve
only the essential distinction between well-formed UTF-8 continuation bytes and
the raw-byte fallback behavior in the report body.
In `@src/runtime/cli/upgrade_command.rs`:
- Line 73: The stable release checksum handling is currently failing open
because parse_asset_digest() can collapse bad metadata into Integrity::default()
and the extraction path later skips verification for unsupported/default tags.
Update the upgrade flow so stable release assets must carry a real digest and
refuse extraction when it is missing or malformed, while keeping the canary path
explicitly exempt only when canary metadata does not provide a digest. Use the
existing parse_asset_digest() and the asset verification/extraction logic in
upgrade_command.rs to ensure every stable path requires Some(digest) before
proceeding.
In `@src/runtime/webcore/Blob.rs`:
- Around line 3166-3175: The transfer path in Blob’s `get_array_buffer_transfer`
/ `get_bytes_transfer` is incorrectly falling back to the clone branch because
the local `_store` reference keeps the store from appearing uniquely owned.
Update the ownership check around the `self.store().is_some_and(|s|
!s.has_one_ref())` guard, or restructure the transfer-only flow so it does not
hold the extra ref before `to_array_buffer(..., Lifetime::Transfer)` /
`take_store()`. Ensure the uniquely-owned in-memory blob can still take the
transfer path instead of always calling `to_array_buffer_view_with_bytes::<{
Lifetime::Clone }, TYPED_ARRAY_VIEW>`.
In `@src/runtime/webcore/TextDecoder.rs`:
- Around line 286-294: The shared/resizable fast path in TextDecoder’s input
handling is copying the entire backing buffer instead of preserving the
JSUint8Array view. Update the match arm in TextDecoder.rs so the owned copy is
created from the typed array’s view data (respecting byteOffset and byteLength)
rather than array_buffer.slice(), and keep the non-shared path using
uint8array.slice() as-is.
In `@src/s3_signing/credentials.rs`:
- Line 1119: The checked bucket slice fallback in the endpoint parsing logic is
being evaluated eagerly by `unwrap_or`, triggering the Clippy warning. Update
the slice handling in the `credentials.rs` logic that returns from
`endpoint.get(...)` to use a lazy fallback instead, and apply the same change in
the other matching slice call referenced by the review so the fallback slice is
only computed when needed.
In `@src/shell_parser/parse.rs`:
- Around line 1915-1919: The if-clause token detection in if_clause_tok_at only
checks is_interpolated_position(range.start), so mixed literal/interpolated
spans can still be misclassified as if/elif/then/fi. Update if_clause_tok_at to
reject any TextRange that overlaps js_string_ranges across the entire range
before calling IfClauseTok::from_text, using the existing if_clause_tok_at and
is_interpolated_position logic as the entry point.
In `@src/sql_jsc/postgres/PostgresSQLConnection.rs`:
- Around line 2974-2976: The COPY handling in PostgresSQLConnection::reader is
only skipping CopyInResponse and CopyBothResponse, which can leave the
server-side COPY state open and pin the connection. Update the unsupported COPY
input-mode paths in the message dispatch to actively abort the operation by
either rejecting and closing the connection or sending CopyFail followed by Sync
after reader.skip_message() consumes the response, using the existing
PostgresSQLConnection and reader flow to locate the fix.
In `@src/sql_jsc/postgres/PostgresSQLQuery.rs`:
- Around line 681-685: The updated comments in
PostgresSQLQuery::get_or_put-style code paths are too long and exceed the repo’s
3-line limit. Shorten both affected comment blocks to at most three lines each
while preserving the same invariant and intent, and keep the wording concise
around the JsCell::with_mut / raw slot ptr borrow-lifetime explanation.
In `@test/cli/inspect/inspect.test.ts`:
- Around line 318-330: The inspector test helper exits on the "Listening:"
banner before the websocket URL is actually parsed, which can race on chunked
stderr output. In the stderr-reading loop in inspect.test.ts, keep collecting
chunks and only break once the URL parsed via new URL(line) yields a ws/wss
protocol, using the existing url variable as the observable condition instead of
the banner text. This should be fixed in the metadataInspectee.stderr reader
logic that currently checks stderr.includes("Listening:").
In `@test/cli/install/bun-install-tarball-integrity.test.ts`:
- Around line 721-727: The test currently reads bun.lock before confirming the
install succeeded, which can mask a real bun install failure with ENOENT. In
bun-install-tarball-integrity.test.ts, update the flow around the Promise.all
result to assert exitCode (and any failure state) before calling
file(join(String(dir), "bun.lock")).text(), using the existing subprocess
variables proc, exitCode, and lockContent to keep the failure source clear.
In `@test/cli/install/bun-pack.test.ts`:
- Around line 1065-1070: The bun-pack tests currently only verify that a few
expected tarball entries exist, so they can miss accidentally packed sensitive
files. Update the assertions in the bun-pack test cases around the tarball
entries checks to explicitly verify that excluded paths such as .git, .npmrc,
and package-lock.json are not present, using the existing tarball.entries data
in the same test blocks. Keep the references anchored on the bun-pack test
assertions so the fix applies to both affected cases.
In `@test/cli/install/symlink-path-traversal.test.ts`:
- Around line 717-724: The install test is reading node_modules/.bin before
asserting the bun install result, so a failed install can throw from readdir and
hide the real stdout/stderr/exitCode failure. In symlink-path-traversal.test.ts,
move the process-result check for exitCode (and its logging) ahead of the .bin
directory read, using the existing install result variables and the
readdir(join(..., ".bin")) assertion afterward so the test surfaces the actual
regression first.
In `@test/js/bun/http/bun-serve-routes.test.ts`:
- Around line 83-90: The raw-socket promise in bun-serve-routes.test.ts only
resolves on `end` and rejects on `error`, so premature connection shutdown can
hang the test. Update the socket handling around `Promise.withResolvers`,
`net.connect`, and the `socket.on(...)` listeners to reject on `close` (and
`abort` if applicable) as well, and make sure settlement is guarded so only the
first terminal event wins. This should make the fixture fail fast with a useful
error instead of timing out.
In `@test/js/bun/http/request-smuggling.test.ts`:
- Around line 1396-1406: The body assertions in the Host echo tests are flaky
because sendRawRequest() returns on the first data chunk before the full HTTP
response is received. Update the 200-path cases to use
sendRawRequestUntilClose() (or otherwise buffer using Content-Length/Connection:
close) in this request-smuggling test block so the echoed Host body is fully
read before asserting.
- Around line 1562-1593: The host-byte matrix in the request-smuggling test is
treating bare [ and ] as valid generic Host bytes, which incorrectly blesses
malformed authorities. Update the matcher logic around checkByte/isHostByte in
this test so brackets are excluded from the accepted reg-name byte set and only
covered where they are genuinely valid as IPv6 literal delimiters. Keep the
expected HTTP/1.1 vs HTTP/1.0 assertions aligned with the actual parser behavior
and req.url normalization for valid Host inputs only.
In `@test/js/bun/md/md-edge-cases.test.ts`:
- Around line 1121-1123: Tighten the link-cap assertion in the markdown
edge-case test so it catches regressions earlier; the current upper bound in the
resolved link count check is too loose. Update the expectation around the
html.match(/<a href=/g) count in md-edge-cases.test.ts to use a much smaller,
fixture-appropriate limit while keeping the existing lower-bound sanity check.
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 159-165: The shell test in bunshell.test.ts only round-trips tab
and question-mark interpolation, so it does not actually verify the
carriage-return case it names. Update the test in the quoted-values case to
include a\rb in the $`echo ...` execution and assert its output alongside the
existing checks, using the same test and $.escape coverage so the round-trip
path for carriage return is exercised too.
In `@test/js/bun/spawn/spawn.ipc.test.ts`:
- Around line 71-74: The IPC test in the spawn handler currently resolves as
soon as the 33rd message is received, so it can miss duplicate or extra
deliveries; update the `ipc` callback in `spawn.ipc.test.ts` to assert the exact
message count by rejecting or failing immediately when `messages.length` exceeds
33, while still resolving only when it reaches exactly 33. Use the existing
`messages`, `resolve`, and `reject` flow in the `ipc(message)` handler so the
test enforces the strongest invariant around delivery count.
In `@test/js/bun/util/wrapAnsi.test.ts`:
- Around line 488-505: The large-input subprocess checks in wrapAnsi.test.ts are
hiding native diagnostics by piping stderr and never using it. Update the
Bun.spawn calls in the affected wrapAnsi test cases so stderr remains visible,
ideally by switching to stderr: "inherit" or by explicitly asserting/logging the
collected stderr alongside stdout and exitCode. Make the change in the
subprocess setup around the large-input Bun.wrapAnsi cases so crash/overflow
regressions surface useful output.
In `@test/js/bun/wasm/wasi.test.js`:
- Around line 63-85: The WASI path_open failure test is only checking FD 4,
which can miss leaks if allocation changes. In the wasi.test.js case around
path_open() and FD_MAP, capture the current FD_MAP size or keys before calling
wasi.wasiImport.path_open, then assert that the FD table is unchanged after the
EEXIST result. Keep the existing sentinel and memory assertions, but replace the
hardcoded descriptor probe with a direct before/after comparison of FD_MAP.
In `@test/js/node/crypto/crypto.key-objects.test.ts`:
- Around line 355-366: The createPrivateKey detached-buffer regression
assertions are too broad because bare toThrow() accepts any failure, not just
the detached ArrayBuffer path. Update the affected expectations in
crypto.key-objects.test.ts around createPrivateKey (including the similar block
later in the file) to assert the specific thrown error shape, such as the exact
error code or message produced by the detachment failure, so the test proves the
intended path rather than generic DER parsing errors.
In `@test/js/node/fs/cp.test.ts`:
- Around line 637-677: The test in the fs cp watcher flow can hang because
`allCreated` is only resolved by file events and never rejected if the child
process exits early. Update the `fs.watch`/`Promise.withResolvers` setup to
route `proc.exited` (and watcher shutdown/error paths) into `onWatchError`
unless `modeAtCreation.size` has reached `destNames.length`, so the awaited
`Promise.all` fails fast instead of timing out.
In `@test/js/node/http/node-http-connect.test.ts`:
- Around line 289-325: The CONNECT test helpers are only resolving on the
happy-path events, so regression cases can hang until the outer timeout. Update
the `Promise.withResolvers(...)` flows in the CONNECT test blocks around
`proxyServer.on("connect")` and the `net.connect(...)` client handlers to reject
on unexpected `close`/missing-event paths as well as `error`, and make sure any
early disconnect or absent `connect`/`data` sequence fails the promise instead
of დარჩening pending. Apply the same failure wiring to the other CONNECT test
cases referenced by the shared helper pattern so every awaited condition has a
corresponding reject path.
In `@test/js/node/http2/h2-conformance.test.ts`:
- Around line 756-825: Remove the per-test timeout override from the Bun
subprocess regression test by updating the test case defined in
h2-conformance.test.ts and the named test that exercises re-entrant read() via
sendTrailers; keep the existing assertions and fixture behavior unchanged, but
rely on the file-level runner timeout instead of passing a local 30_000 timeout.
In `@test/js/node/http2/node-http2.test.js`:
- Around line 2955-2985: `requestOverHttp1()` only rejects on request errors or
after the response ends, so response-side aborts/errors can hang the test until
timeout. Update the helper to wire the response’s failure events, alongside the
existing request `"error"` handling, so any mid-body abort or stream error
immediately rejects the promise. Keep the change localized to
`requestOverHttp1()` and preserve the existing resolve path for normal `"end"`
completion.
In `@test/js/node/test/parallel/test-crypto-certificate.js`:
- Around line 109-122: The SPKAC regression assertions were added to the
vendored Node test suite, which should remain an upstream mirror. Remove the new
coverage from Certificate.verifySpkac, Certificate.exportPublicKey, and
Certificate.exportChallenge in the Node test and relocate the same
zero-length/whitespace input checks into a Bun-owned crypto test file so the
upstream-synced test stays unchanged.
In `@test/js/valkey/reliability/resp-nesting-depth.test.ts`:
- Around line 5-79: The RESP parsing in countRespCommands and
createMockRedisServer is too eager because it counts commands from each data
chunk without buffering incomplete frames. Update the socket handling to keep a
per-socket buffer, append incoming data across data events, and only
count/consume complete RESP command frames before replying so split
HELLO/PING/PSUBSCRIBE packets are handled correctly.
In `@test/js/web/websocket/websocket-subprotocol-strict.test.ts`:
- Around line 198-208: The WebSocket strict subprotocol test only waits for
close, so unexpected open or error paths can hang the test instead of failing
fast. Update the await setup around WebSocket and the Promise.withResolvers
usage to reject on any unexpected ws.onopen or ws.onerror while still resolving
on ws.onclose, and keep the assertions in the same test around onopenMock and
close code/reason.
In `@test/v8/v8.test.ts`:
- Around line 430-469: The standalone addon setup in
standaloneAddonFiles/buildStandaloneAddon is pulling node-gyp via bun install,
which makes the test depend on the live registry. Remove the external install
step and change the test to use only local, prechecked-in dependencies or
fixtures so buildStandaloneAddon can run hermetically without network access.
Keep the fix focused on the standalone addon test helpers and their package.json
generation.
---
Outside diff comments:
In `@src/runtime/bake/DevServer/HmrSocket.rs`:
- Around line 124-176: The unsubscribe path in
HmrSocket::handle_subscription_changes is unreachable because the `else if`
repeats the same condition as the subscribe branch, so dropped HmrTopic values
never call `ws.unsubscribe`. Update that branch to check for topics present in
`self.subscriptions` but absent from `new_bits`, and keep the unsubscribe call
there. Make sure the logic still preserves the existing subscribe-side hooks for
`feature_flags::BAKE_DEBUGGING_FEATURES` and leaves `self.on_unsubscribe` as
counter cleanup only.
In `@src/runtime/shell/shell_body.rs`:
- Around line 999-1026: The keyword escaping check in append_utf8 only detects
complete tokens, so interpolated fragments that combine with adjacent text can
still form shell control keywords. Update the logic around
needs_escape_utf8_ascii_latin1 and IfClauseTok::from_text in shell_body.rs so
append_js_str_ref is used whenever the current fragment can contribute to
forming if, else, elif, then, or fi with neighboring content, or make the
builder inspect adjacent-literal context before choosing the UTF-8 fast path.
In `@src/runtime/webview/WebViewHost.cpp`:
- Around line 717-732: The outer bunConsole payload is still being dereferenced
before validation in WebViewHost::postMessage handling, so arbitrary bridged
objects can trigger Objective-C selector crashes. Add an upfront guard for both
type and args before calling objc::NSString(type).toWTF() or NSArray::count(),
and bail out early unless the payload is the expected NSString/NSArray shape.
Keep the existing per-element checks in the loop, but make the initial payload
validation happen first so no processing or allocation occurs on untrusted
input.
In `@test/js/bun/util/filesystem_router.test.ts`:
- Around line 685-711: The filesystem router collision test currently
brute-forces a hash collision with a long loop and adds an explicit per-test
timeout, which should be avoided in test/js/bun/**. Replace the search in the
Bun.FileSystemRouter test with a precomputed known colliding pair (or a fixed
fixture generated offline) so the test is deterministic and fast. Keep the
existing assertions around route matching and non-matching behavior, but remove
the 60_000 timeout and the runtime collision search logic.
🪄 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: a5083408-d425-4996-9d0b-5939e7fcbe8f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (208)
.github/workflows/update-vendor.ymlcompletions/bun.bashcompletions/bun.zshdockerhub/alpine/Dockerfiledockerhub/debian-slim/Dockerfiledockerhub/debian/Dockerfiledockerhub/distroless/Dockerfilepackages/bun-debug-adapter-protocol/src/debugger/adapter.tspackages/bun-debug-adapter-protocol/src/debugger/sourcemap.test.tspackages/bun-release/src/npm/install.tspackages/bun-usockets/src/context.cpackages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/internal/internal.hpackages/bun-uws/src/App.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpContextData.hpackages/bun-uws/src/HttpParser.hpackages/bun-vscode/src/features/debug.tspackages/bun-vscode/src/features/lockfile/lockfile.style.tssrc/bundler/ParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/barrel_imports.rssrc/bundler/cache.rssrc/bundler/transpiler.rssrc/glob/GlobWalker.rssrc/http/lib.rssrc/http/ssl_config.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/runTasks.rssrc/install/bin.rssrc/install/extract_tarball.rssrc/install/integrity.rssrc/install/isolated_install.rssrc/install/lockfile.rssrc/install/lockfile/Package.rssrc/js/internal/debugger.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/js/node/http2.tssrc/js/node/https.tssrc/js/node/url.tssrc/js/node/wasi.tssrc/js_parser/lexer.rssrc/js_parser_jsc/Macro.rssrc/js_printer/lib.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/bindings/BunPlugin.cppsrc/jsc/bindings/CookieMap.cppsrc/jsc/bindings/ZigException.cppsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/decodeURIComponentSIMD.cppsrc/jsc/bindings/napi.cppsrc/jsc/bindings/ncrypto.cppsrc/jsc/bindings/node/crypto/CryptoHkdf.cppsrc/jsc/bindings/node/crypto/CryptoPrimes.cppsrc/jsc/bindings/node/crypto/CryptoSignJob.cppsrc/jsc/bindings/node/crypto/CryptoUtil.cppsrc/jsc/bindings/node/crypto/JSECDHConstructor.cppsrc/jsc/bindings/node/crypto/KeyObject.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/jsc/bindings/v8/V8Number.cppsrc/jsc/bindings/v8/V8String.cppsrc/jsc/bindings/webcore/AbortSignal.cppsrc/jsc/bindings/webcore/AbortSignal.hsrc/jsc/bindings/webcore/JSDOMConvertRecord.hsrc/jsc/bindings/webcore/SerializedScriptValue.cppsrc/jsc/bindings/webcrypto/CryptoAlgorithmEd25519.cppsrc/jsc/bindings/webcrypto/CryptoKeyOKP.cppsrc/jsc/bindings/webcrypto/CryptoKeyRSA.cppsrc/jsc/bindings/webcrypto/CryptoKeyRSA.hsrc/jsc/bindings/webcrypto/CryptoKeyRaw.cppsrc/jsc/bindings/webcrypto/CryptoKeyRaw.hsrc/jsc/bindings/wrapAnsi.cppsrc/jsc/ipc.rssrc/md/links.rssrc/md/parser.rssrc/node-fallbacks/url.jssrc/parsers/json_lexer.rssrc/paths/resolve_path.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/runtime/api/BunObject.rssrc/runtime/api/bun/h2/connection.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/DevServer/ErrorReportRequest.rssrc/runtime/bake/DevServer/HmrSocket.rssrc/runtime/bake/bun-framework-react/ssr.tsxsrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/mod.rssrc/runtime/cli/create_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/napi/napi_body.rssrc/runtime/node/net/BlockList.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/shell_body.rssrc/runtime/socket/socket_body.rssrc/runtime/valkey_jsc/valkey.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/TextDecoder.rssrc/runtime/webview/ObjCRuntime.cppsrc/runtime/webview/ObjCRuntime.hsrc/runtime/webview/WebViewHost.cppsrc/s3_signing/credentials.rssrc/semver/Version.rssrc/shell_parser/parse.rssrc/sourcemap/Mapping.rssrc/sourcemap/lib.rssrc/sql/mysql/protocol/AuthSwitchRequest.rssrc/sql/mysql/protocol/LocalInfileRequest.rssrc/sql/postgres/protocol/CopyData.rssrc/sql/postgres/protocol/NewReader.rssrc/sql_jsc/Cargo.tomlsrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/standalone_graph/StandaloneModuleGraph.rssrc/url/lib.rssrc/uws_sys/App.rssrc/uws_sys/libuwsockets.cppsrc/valkey/valkey_protocol.rstest/bake/dev/bundle.test.tstest/bake/dev/hot.test.tstest/bake/dev/html.test.tstest/bake/dev/production.test.tstest/bundler/bundler_barrel.test.tstest/bundler/bundler_browser.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/bundler_loader.test.tstest/bundler/native-plugin.test.tstest/bundler/transpiler/runtime-transpiler.test.tstest/cli/inspect/inspect.test.tstest/cli/install/bun-create.test.tstest/cli/install/bun-install-tarball-integrity.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-lockb.test.tstest/cli/install/bun-pack.test.tstest/cli/install/bun-upgrade.test.tstest/cli/install/isolated-install.test.tstest/cli/install/semver.test.tstest/cli/install/symlink-path-traversal.test.tstest/cli/run/filter-workspace.test.tstest/cli/run/run-quote.test.tstest/js/bun/cookie/cookie-map.test.tstest/js/bun/glob/path-length.test.tstest/js/bun/glob/scan.test.tstest/js/bun/http/bun-serve-routes.test.tstest/js/bun/http/decodeURIComponentSIMD.test.tstest/js/bun/http/proxy-stress-errors.test.tstest/js/bun/http/request-smuggling.test.tstest/js/bun/http/serve.test.tstest/js/bun/jsc/bun-jsc.test.tstest/js/bun/md/md-edge-cases.test.tstest/js/bun/net/socket.test.tstest/js/bun/plugin/plugins.test.tstest/js/bun/resolve/resolve.test.tstest/js/bun/s3/s3-list-encode-overflow.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/spawn/spawn.ipc.bun-node.test.tstest/js/bun/spawn/spawn.ipc.test.tstest/js/bun/sqlite/sqlite.test.jstest/js/bun/util/filesystem_router.test.tstest/js/bun/util/wrapAnsi.test.tstest/js/bun/util/zstd.test.tstest/js/bun/wasm/wasi.test.jstest/js/bun/webview/webview.test.tstest/js/node/crypto/crypto-oneshot.test.tstest/js/node/crypto/crypto-random.test.tstest/js/node/crypto/crypto.key-objects.test.tstest/js/node/crypto/hkdf-callback-null.test.tstest/js/node/fs/cp.test.tstest/js/node/fs/fs-mkdir.test.tstest/js/node/fs/fs.test.tstest/js/node/http/node-http-connect.test.tstest/js/node/http/node-http-proxy-url.test.tstest/js/node/http2/h2-conformance.test.tstest/js/node/http2/node-http2.test.jstest/js/node/test/parallel/test-crypto-certificate.jstest/js/node/tls/node-tls-server.test.tstest/js/node/url/url-parse-format.test.jstest/js/node/url/url-parse-query.test.jstest/js/node/v8/capture-stack-trace.test.jstest/js/sql/sql-helpers-validation.test.tstest/js/sql/sql-mysql-auth-short-nonce.test.tstest/js/sql/wire-frames.test.tstest/js/sql/wire-frames.tstest/js/valkey/reliability/connection-failures.test.tstest/js/valkey/reliability/resp-nesting-depth.test.tstest/js/web/crypto/web-crypto.test.tstest/js/web/encoding/text-decoder.test.jstest/js/web/fetch/blob-cow.test.tstest/js/web/fetch/fetch-redirect.test.tstest/js/web/fetch/fetch.tls.test.tstest/js/web/fetch/headers.test.tstest/js/web/url/url.test.tstest/js/web/websocket/websocket-subprotocol-strict.test.tstest/napi/napi-app/standalone_tests.cpptest/napi/napi.test.tstest/v8/v8.test.ts
💤 Files with no reviewable changes (3)
- src/sql_jsc/Cargo.toml
- src/bundler/cache.rs
- src/bundler/transpiler.rs
| ); | ||
| // `defer { if (generate_source_map) printer.source_map_builder.line_offset_tables.deinit(opts.allocator); }` | ||
| // — no longer needed: see `print_ast` above. | ||
| printer.was_lazy_export = tree.has_lazy_export; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Wire was_lazy_export through the dev-server lazy-export printer too.
Line 8301 only covers print_common_js; print_dev_server_module still prints ast.has_lazy_export via print_expr without enabling this flag, so data-file __proto__ can still be emitted as a prototype-setting literal on that path.
Proposed fix
- self.print_expr(
+ let previous_was_lazy_export = self.was_lazy_export;
+ self.was_lazy_export = true;
+ self.print_expr(
Expr {
data: *lazy,
loc: body_stmts[0].loc,
},
Level::Comma,
ExprFlagSet::empty(),
);
+ self.was_lazy_export = previous_was_lazy_export;As per coding guidelines, “Fix the whole class in the same PR.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| printer.was_lazy_export = tree.has_lazy_export; | |
| let previous_was_lazy_export = self.was_lazy_export; | |
| self.was_lazy_export = true; | |
| self.print_expr( | |
| Expr { | |
| data: *lazy, | |
| loc: body_stmts[0].loc, | |
| }, | |
| Level::Comma, | |
| ExprFlagSet::empty(), | |
| ); | |
| self.was_lazy_export = previous_was_lazy_export; |
🤖 Prompt for 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.
In `@src/js_printer/lib.rs` at line 8301, The dev-server lazy-export path is still
missing the `was_lazy_export` state, so `print_dev_server_module` can emit
`ast.has_lazy_export` through `print_expr` without the protection that
`print_common_js` already gets. Update the dev-server printer to wire
`tree.has_lazy_export` into the same `was_lazy_export` flag on the relevant
printer state, using the `print_dev_server_module` flow and the shared printer
setup around `printer.was_lazy_export`, so both code paths handle data-file
`__proto__` consistently.
Source: Coding guidelines
| bool secure = name.startsWithIgnoringASCIICase("__Secure-"_s) || name.startsWithIgnoringASCIICase("__Host-"_s); | ||
|
|
||
| // Add the new cookie | ||
| auto cookie_exception = Cookie::create(name, ""_s, domain, path, 1, false, CookieSameSite::Lax, false, std::numeric_limits<double>::quiet_NaN(), false); | ||
| auto cookie_exception = Cookie::create(name, ""_s, domain, path, 1, secure, CookieSameSite::Lax, false, std::numeric_limits<double>::quiet_NaN(), false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat cookie prefix matching as case-sensitive.
__Secure- and __Host- are exact cookie-name prefixes. Using startsWithIgnoringASCIICase here makes ordinary names like __secure-id look prefixed, so the delete tombstone can be emitted with the wrong Secure attribute and miss the original cookie.
Suggested fix
- bool secure = name.startsWithIgnoringASCIICase("__Secure-"_s) || name.startsWithIgnoringASCIICase("__Host-"_s);
+ bool secure = name.startsWith("__Secure-"_s) || name.startsWith("__Host-"_s);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool secure = name.startsWithIgnoringASCIICase("__Secure-"_s) || name.startsWithIgnoringASCIICase("__Host-"_s); | |
| // Add the new cookie | |
| auto cookie_exception = Cookie::create(name, ""_s, domain, path, 1, false, CookieSameSite::Lax, false, std::numeric_limits<double>::quiet_NaN(), false); | |
| auto cookie_exception = Cookie::create(name, ""_s, domain, path, 1, secure, CookieSameSite::Lax, false, std::numeric_limits<double>::quiet_NaN(), false); | |
| bool secure = name.startsWith("__Secure-"_s) || name.startsWith("__Host-"_s); | |
| // Add the new cookie | |
| auto cookie_exception = Cookie::create(name, ""_s, domain, path, 1, secure, CookieSameSite::Lax, false, std::numeric_limits<double>::quiet_NaN(), false); |
🤖 Prompt for 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.
In `@src/jsc/bindings/CookieMap.cpp` around lines 197 - 200, The cookie prefix
check in CookieMap::findOrCreateCookie delete-tombstone handling is too
permissive because it uses case-insensitive matching for the "__Secure-" and
"__Host-" prefixes. Update the secure-flag detection in the CookieMap logic to
use exact case-sensitive prefix checks so only cookies with the canonical
prefixes are treated as special, ensuring the tombstone created via
Cookie::create preserves the correct Secure attribute for names like
"__secure-id".
| function requestOverHttp1(port, headers) { | ||
| const { promise, resolve, reject } = Promise.withResolvers(); | ||
| const request = https.request( | ||
| { | ||
| host: "localhost", | ||
| port, | ||
| path: "/", | ||
| agent: false, | ||
| ca: TLS_CERT.cert, | ||
| headers: { connection: "close", ...headers }, | ||
| }, | ||
| async response => { | ||
| try { | ||
| let body = ""; | ||
| response.setEncoding("utf8"); | ||
| response.on("data", chunk => (body += chunk)); | ||
| await new Promise(done => response.on("end", done)); | ||
| resolve({ | ||
| statusCode: response.statusCode, | ||
| statusMessage: response.statusMessage, | ||
| headers: response.headers, | ||
| body, | ||
| }); | ||
| } catch (err) { | ||
| reject(err); | ||
| } | ||
| }, | ||
| ); | ||
| request.on("error", reject); | ||
| request.end(); | ||
| return promise; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject response-side failures in requestOverHttp1().
This helper only settles on request error or response end. If the fallback path aborts or errors the response mid-body, these tests hang until the outer timeout instead of failing immediately.
As per coding guidelines, await conditions and wire failure events to reject.
Suggested fix
function requestOverHttp1(port, headers) {
const { promise, resolve, reject } = Promise.withResolvers();
const request = https.request(
@@
async response => {
try {
let body = "";
response.setEncoding("utf8");
+ response.on("error", reject);
+ response.on("aborted", () => reject(new Error("response aborted")));
response.on("data", chunk => (body += chunk));
await new Promise(done => response.on("end", done));
resolve({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function requestOverHttp1(port, headers) { | |
| const { promise, resolve, reject } = Promise.withResolvers(); | |
| const request = https.request( | |
| { | |
| host: "localhost", | |
| port, | |
| path: "/", | |
| agent: false, | |
| ca: TLS_CERT.cert, | |
| headers: { connection: "close", ...headers }, | |
| }, | |
| async response => { | |
| try { | |
| let body = ""; | |
| response.setEncoding("utf8"); | |
| response.on("data", chunk => (body += chunk)); | |
| await new Promise(done => response.on("end", done)); | |
| resolve({ | |
| statusCode: response.statusCode, | |
| statusMessage: response.statusMessage, | |
| headers: response.headers, | |
| body, | |
| }); | |
| } catch (err) { | |
| reject(err); | |
| } | |
| }, | |
| ); | |
| request.on("error", reject); | |
| request.end(); | |
| return promise; | |
| function requestOverHttp1(port, headers) { | |
| const { promise, resolve, reject } = Promise.withResolvers(); | |
| const request = https.request( | |
| { | |
| host: "localhost", | |
| port, | |
| path: "/", | |
| agent: false, | |
| ca: TLS_CERT.cert, | |
| headers: { connection: "close", ...headers }, | |
| }, | |
| async response => { | |
| try { | |
| let body = ""; | |
| response.setEncoding("utf8"); | |
| response.on("error", reject); | |
| response.on("aborted", () => reject(new Error("response aborted"))); | |
| response.on("data", chunk => (body += chunk)); | |
| await new Promise(done => response.on("end", done)); | |
| resolve({ | |
| statusCode: response.statusCode, | |
| statusMessage: response.statusMessage, | |
| headers: response.headers, | |
| body, | |
| }); | |
| } catch (err) { | |
| reject(err); | |
| } | |
| }, | |
| ); | |
| request.on("error", reject); | |
| request.end(); | |
| return promise; | |
| } |
🤖 Prompt for 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.
In `@test/js/node/http2/node-http2.test.js` around lines 2955 - 2985,
`requestOverHttp1()` only rejects on request errors or after the response ends,
so response-side aborts/errors can hang the test until timeout. Update the
helper to wire the response’s failure events, alongside the existing request
`"error"` handling, so any mid-body abort or stream error immediately rejects
the promise. Keep the change localized to `requestOverHttp1()` and preserve the
existing resolve path for normal `"end"` completion.
Source: Coding guidelines
| { | ||
| const spkacText = spkacValid.toString('utf8').trimEnd(); | ||
| const padded = Buffer.alloc(Buffer.byteLength(spkacText) + 1); | ||
| padded.write(spkacText); | ||
| const zeroLengthView = padded.subarray(0, 0); | ||
| assert.strictEqual(Certificate.verifySpkac(zeroLengthView), false); | ||
| assert.strictEqual(Certificate.exportPublicKey(zeroLengthView), ''); | ||
| assert.strictEqual(Certificate.exportChallenge(zeroLengthView), ''); | ||
|
|
||
| for (const input of [Buffer.alloc(0), Buffer.from(' \n\r\t'), '', ' \n\r\t']) { | ||
| assert.strictEqual(Certificate.verifySpkac(Buffer.from(input)), false); | ||
| assert.strictEqual(Certificate.exportPublicKey(input), ''); | ||
| assert.strictEqual(Certificate.exportChallenge(input), ''); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep Bun-specific coverage out of upstream-synced Node tests.
test/js/node/test/parallel/ is the vendored Node suite. Adding local assertions here breaks upstream diffability and makes future syncs harder; please move this SPKAC regression coverage into a Bun-owned crypto test and leave the upstream port unchanged.
As per coding guidelines, test/js/node/test/parallel/ files are official Node.js tests that we cannot modify. Based on learnings, these files should remain immutable upstream mirrors.
🤖 Prompt for 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.
In `@test/js/node/test/parallel/test-crypto-certificate.js` around lines 109 -
122, The SPKAC regression assertions were added to the vendored Node test suite,
which should remain an upstream mirror. Remove the new coverage from
Certificate.verifySpkac, Certificate.exportPublicKey, and
Certificate.exportChallenge in the Node test and relocate the same
zero-length/whitespace input checks into a Bun-owned crypto test file so the
upstream-synced test stays unchanged.
Sources: Coding guidelines, Learnings
| /** | ||
| * Count the number of complete RESP commands in a buffer. | ||
| * Each command starts with '*' (array) followed by the element count. | ||
| * We count top-level '*' markers that begin a new command frame. | ||
| */ | ||
| function countRespCommands(data: Buffer): number { | ||
| const str = data.toString(); | ||
| let count = 0; | ||
| let pos = 0; | ||
| while (pos < str.length) { | ||
| if (str[pos] === "*") { | ||
| count++; | ||
| // Skip past this command: find the array length line | ||
| const crlfIdx = str.indexOf("\r\n", pos); | ||
| if (crlfIdx === -1) break; | ||
| const arrayLen = parseInt(str.substring(pos + 1, crlfIdx), 10); | ||
| if (isNaN(arrayLen) || arrayLen < 0) break; | ||
| // Skip past arrayLen bulk-string elements (each is $<len>\r\n<data>\r\n) | ||
| let elemPos = crlfIdx + 2; | ||
| for (let i = 0; i < arrayLen; i++) { | ||
| if (elemPos >= str.length || str[elemPos] !== "$") break; | ||
| const lenEnd = str.indexOf("\r\n", elemPos); | ||
| if (lenEnd === -1) break; | ||
| const bulkLen = parseInt(str.substring(elemPos + 1, lenEnd), 10); | ||
| if (isNaN(bulkLen) || bulkLen < 0) break; | ||
| elemPos = lenEnd + 2 + bulkLen + 2; // skip $<len>\r\n<data>\r\n | ||
| } | ||
| pos = elemPos; | ||
| } else { | ||
| pos++; | ||
| } | ||
| } | ||
| return count; | ||
| } | ||
|
|
||
| /** | ||
| * Creates a minimal mock Redis server that parses incoming RESP command | ||
| * frames. The first command (HELLO handshake) gets +OK; each subsequent | ||
| * command receives the next crafted payload (the last one is repeated when | ||
| * there are more commands than payloads). Handles the case where multiple | ||
| * commands arrive in a single TCP chunk. | ||
| */ | ||
| function createMockRedisServer(payload: Buffer | Buffer[]): Promise<{ server: net.Server; port: number }> { | ||
| const payloads = Array.isArray(payload) ? payload : [payload]; | ||
| return new Promise((resolve, reject) => { | ||
| const server = net.createServer(socket => { | ||
| let commandsSeen = 0; | ||
|
|
||
| socket.on("data", (data: Buffer) => { | ||
| const numCmds = countRespCommands(data); | ||
| for (let i = 0; i < numCmds; i++) { | ||
| if (commandsSeen === 0) { | ||
| // Respond to HELLO handshake with a simple OK | ||
| socket.write("+OK\r\n"); | ||
| } else { | ||
| // Each subsequent command gets the next crafted payload | ||
| socket.write(payloads[Math.min(commandsSeen - 1, payloads.length - 1)]); | ||
| } | ||
| commandsSeen++; | ||
| } | ||
| }); | ||
|
|
||
| socket.on("error", () => { | ||
| // Ignore socket errors from client disconnecting | ||
| }); | ||
| }); | ||
|
|
||
| server.listen(0, "127.0.0.1", () => { | ||
| const addr = server.address() as net.AddressInfo; | ||
| resolve({ server, port: addr.port }); | ||
| }); | ||
|
|
||
| server.on("error", reject); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Buffer RESP frames across data events before counting commands.
This helper counts a command as soon as it sees *, even if the frame is truncated, and it only looks at the current chunk. A single HELLO/PING/PSUBSCRIBE split across TCP packets will therefore get a reply before the full command arrives, which makes the new routing tests pass or fail for the wrong reason. Keep a per-socket buffer and only consume complete RESP frames.
As per coding guidelines, "Buffer raw socket/stdout chunks to the protocol's framing before asserting."
🤖 Prompt for 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.
In `@test/js/valkey/reliability/resp-nesting-depth.test.ts` around lines 5 - 79,
The RESP parsing in countRespCommands and createMockRedisServer is too eager
because it counts commands from each data chunk without buffering incomplete
frames. Update the socket handling to keep a per-socket buffer, append incoming
data across data events, and only count/consume complete RESP command frames
before replying so split HELLO/PING/PSUBSCRIBE packets are handled correctly.
Source: Coding guidelines
| const { promise: closePromise, resolve: resolveClose } = Promise.withResolvers<CloseEvent>(); | ||
|
|
||
| const ws = new WebSocket(`ws://localhost:${server.port}`, ["chat", "echo"]); | ||
| const onopenMock = mock(() => {}); | ||
| ws.onopen = onopenMock; | ||
| ws.onclose = close => resolveClose(close); | ||
|
|
||
| const close = await closePromise; | ||
| expect(close.code).toBe(1002); | ||
| expect(close.reason).toBe("Missing client protocol"); | ||
| expect(onopenMock).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject unexpected open/error paths while waiting for the close.
This promise only resolves on close, so an implementation that surfaces the regression as error or an unexpected open can leave the test hanging until the file timeout instead of failing immediately.
Suggested fix
- const { promise: closePromise, resolve: resolveClose } = Promise.withResolvers<CloseEvent>();
+ const { promise: closePromise, resolve: resolveClose, reject: rejectClose } =
+ Promise.withResolvers<CloseEvent>();
const ws = new WebSocket(`ws://localhost:${server.port}`, ["chat", "echo"]);
const onopenMock = mock(() => {});
- ws.onopen = onopenMock;
- ws.onclose = close => resolveClose(close);
+ ws.onopen = () => {
+ onopenMock();
+ rejectClose(new Error("unexpected open"));
+ };
+ ws.onerror = () => rejectClose(new Error("unexpected error"));
+ ws.onclose = resolveClose;As per coding guidelines, "Await the actual observable condition" and "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { promise: closePromise, resolve: resolveClose } = Promise.withResolvers<CloseEvent>(); | |
| const ws = new WebSocket(`ws://localhost:${server.port}`, ["chat", "echo"]); | |
| const onopenMock = mock(() => {}); | |
| ws.onopen = onopenMock; | |
| ws.onclose = close => resolveClose(close); | |
| const close = await closePromise; | |
| expect(close.code).toBe(1002); | |
| expect(close.reason).toBe("Missing client protocol"); | |
| expect(onopenMock).not.toHaveBeenCalled(); | |
| const { promise: closePromise, resolve: resolveClose, reject: rejectClose } = | |
| Promise.withResolvers<CloseEvent>(); | |
| const ws = new WebSocket(`ws://localhost:${server.port}`, ["chat", "echo"]); | |
| const onopenMock = mock(() => {}); | |
| ws.onopen = () => { | |
| onopenMock(); | |
| rejectClose(new Error("unexpected open")); | |
| }; | |
| ws.onerror = () => rejectClose(new Error("unexpected error")); | |
| ws.onclose = resolveClose; | |
| const close = await closePromise; | |
| expect(close.code).toBe(1002); | |
| expect(close.reason).toBe("Missing client protocol"); | |
| expect(onopenMock).not.toHaveBeenCalled(); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 199-199: Avoid insecure (ws://) WebSocket connections; use the encrypted wss:// scheme.
Context: new WebSocket(ws://localhost:${server.port}, ["chat", "echo"])
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(insecure-websocket-typescript)
🤖 Prompt for 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.
In `@test/js/web/websocket/websocket-subprotocol-strict.test.ts` around lines 198
- 208, The WebSocket strict subprotocol test only waits for close, so unexpected
open or error paths can hang the test instead of failing fast. Update the await
setup around WebSocket and the Promise.withResolvers usage to reject on any
unexpected ws.onopen or ws.onerror while still resolving on ws.onclose, and keep
the assertions in the same test around onopenMock and close code/reason.
Source: Coding guidelines
| function standaloneAddonFiles(targetName: string, addonCpp: string, runJs: string) { | ||
| return { | ||
| "package.json": JSON.stringify({ | ||
| name: `${targetName}-test`, | ||
| version: "1.0.0", | ||
| devDependencies: { "node-gyp": "~11.2.0" }, | ||
| }), | ||
| "binding.gyp": JSON.stringify({ | ||
| targets: [ | ||
| { | ||
| target_name: targetName, | ||
| sources: ["addon.cpp"], | ||
| cflags: ["-Wno-deprecated-declarations"], | ||
| cflags_cc: ["-Wno-deprecated-declarations"], | ||
| xcode_settings: { | ||
| OTHER_CFLAGS: ["-Wno-deprecated-declarations"], | ||
| OTHER_CPLUSPLUSFLAGS: ["-Wno-deprecated-declarations"], | ||
| }, | ||
| }, | ||
| ], | ||
| }), | ||
| "addon.cpp": addonCpp, | ||
| "run.js": runJs, | ||
| }; | ||
| } | ||
|
|
||
| async function buildStandaloneAddon(cwd: string) { | ||
| { | ||
| await using install = spawn({ | ||
| cmd: [bunExe(), "install", "--ignore-scripts"], | ||
| cwd, | ||
| env: bunEnv, | ||
| stdin: "inherit", | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| }); | ||
| const exitCode = await install.exited; | ||
| if (exitCode !== 0) { | ||
| throw new Error(`install failed: ${exitCode}`); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don’t fetch node-gyp from the live registry during the test run.
Creating a temp package.json and then running bun install here makes this suite depend on external npm availability, which breaks the repo’s hermetic-test requirement and can cause unrelated CI failures.
As per coding guidelines, "Tests must be hermetic and leave nothing behind. Never contact external network hosts or live registries."
🤖 Prompt for 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.
In `@test/v8/v8.test.ts` around lines 430 - 469, The standalone addon setup in
standaloneAddonFiles/buildStandaloneAddon is pulling node-gyp via bun install,
which makes the test depend on the live registry. Remove the external install
step and change the test to use only local, prechecked-in dependencies or
fixtures so buildStandaloneAddon can run hermetically without network access.
Keep the fix focused on the standalone addon test helpers and their package.json
generation.
Source: Coding guidelines
| static inline bool isValidHostFieldValue(std::string_view host) { | ||
| for (unsigned char c : host) { | ||
| if (!isHostFieldValueByte(c)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate Host structure, not just allowed characters.
isValidHostFieldValue() accepts any allowed byte in any position, so values like Host: example.com:abc or Host: a:b:c pass Line 936 even though they are not uri-host [ ":" port ]. This also mirrors into Request::is_valid_host_header, so Bun.serve can still synthesize invalid Request.url values from malformed authorities. Add structural checks for bracketed IPv6, unbracketed colon placement, and numeric ports, then mirror the same logic in Request.rs. As per coding guidelines, "Validate untrusted input BEFORE any processing, allocation, or side effect."
Also applies to: 933-937
🤖 Prompt for 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.
In `@packages/bun-uws/src/HttpParser.h` around lines 707 - 713,
`isValidHostFieldValue()` currently only checks allowed bytes and can accept
malformed host strings like extra colons or non-numeric ports. Update the host
validation logic in `HttpParser::isValidHostFieldValue` to enforce the actual
`uri-host [ ":" port ]` structure, including proper bracketed IPv6 handling,
rejecting invalid unbracketed colon placement, and requiring a numeric port when
present. Mirror the same structural validation in
`Request::is_valid_host_header` so malformed authorities cannot be used to
synthesize invalid `Request.url` values.
Source: Coding guidelines
|
Went through the 15 fix commits against the 22 review threads. 12 threads verified and resolved (Host-charset blocker via the
On the red CI: two of the three failures are this branch's. The glob rewrite recreated the deny-level |
|
Re the integrity thread (review item 2) — replying here since it's the one thread without a direct response: The maintainer's call is to keep strongest-algorithm-first, single-digest verification for now. The blocker isn't the comparison loop: the parsed |
433d2dd to
45dde10
Compare
|
Thanks for the second pass — everything below is on the branch. The branch is now rebased onto current 1. Integrity (multiple digests of one algorithm) — answered separately in that thread with the maintainer's decision; nothing further here. 2. Glob symlink cycles / clippy — 3. h2 4. cpSync Windows reparse targets — you're right on the UNC residual: 5. 6. Record conversion fast path — you're right that the getter test never entered the fast path (an accessor own property fails 7. 8. wrapAnsi no-space seam — your hand trace reproduces exactly (the PR branch kept one over-wide row where released Bun wraps). We did deviate from both suggested shapes, with measurements: "dirty when the row is non-empty" is O(n²) on the existing 100k zero-width-escape perf test in the same file (it failed), and "pass the row's real last codepoint" is still wrong when the width-0 row ends in an escape sequence whose ASCII terminator masks a Prepend ( 9. allowHTTP1 fallback writer — all three: the terminating chunk is gated 10. Host parity sweep — added the |
| seen.set(h, candidate); | ||
| } | ||
| expect(pair).not.toBeNull(); | ||
| const [routeSegment, attackSegment] = pair!; | ||
| expect(attackSegment).not.toBe(routeSegment); | ||
| expect(attackSegment.length).toBe(routeSegment.length); | ||
| const [routeSegment, collidingSegment] = pair!; | ||
| expect(collidingSegment).not.toBe(routeSegment); | ||
| expect(collidingSegment.length).toBe(routeSegment.length); | ||
|
|
||
| const { dir } = make([`${routeSegment}/[id].tsx`]); | ||
| const router = new Bun.FileSystemRouter({ | ||
| dir, | ||
| style: "nextjs", | ||
| }); | ||
|
|
||
| // The genuine segment matches its dynamic route. | ||
| expect(router.match(`/${routeSegment}/42`)?.name).toBe(`/${routeSegment}/[id]`); | ||
| // A different segment that only collides on (length, 32-bit hash) must not. | ||
| expect(router.match(`/${attackSegment}/42`)).toBeNull(); | ||
| }); | ||
| expect(router.match(`/${collidingSegment}/42`)).toBeNull(); | ||
| }, 60_000); |
There was a problem hiding this comment.
🟡 This hunk is a cosmetic-only refactor (rename attackSegment→collidingSegment, extract low32) that also (a) deletes the 5-line why-comment recording that Bun.hash.wyhash(s, 0) is the same hash the router stores for static segments — the one fact here not derivable from the test body (root CLAUDE.md:239: "Don't delete existing why-comments in cleanup passes"), and (b) adds a 60_000 per-test timeout where there was none (test/CLAUDE.md:120). Suggest restoring a one-line form of the wyhash-is-the-router's-hash note and dropping the timeout (or shrinking the 600k search if it's actually slow on debug).
Extended reasoning...
What changed. The only functional reason this PR touches filesystem_router.test.ts is the new .params decodes percent escapes test added above this hunk. The changes to this test are pure cosmetics — attackSegment is renamed to collidingSegment, and the inline Number(BigInt.asUintN(32, BigInt(Bun.hash.wyhash(...)))) expression is hoisted into a low32 helper. Riding alongside that cleanup are two unrelated edits: the existing 5-line block comment plus two inline assertion comments are deleted, and the closing }); becomes }, 60_000);.
The deleted comment. Root CLAUDE.md:239 reads "Comments carry only durable non-obvious content … Don't delete existing why-comments in cleanup passes." The removed block was:
// Route segment matching must compare bytes, not just (length, truncated
// 32-bit wyhash). Bun.hash.wyhash(s, 0) is the same hash the router stores
// for static route segments, so a birthday search over a few hundred
// thousand equal-length candidates finds a colliding pair with overwhelming
// probability (expected after ~80k candidates).
The non-obvious load-bearing clause is "Bun.hash.wyhash(s, 0) is the same hash the router stores for static route segments". That is a fact about src/url/lib.rs, not about this test, and it is the only thing that makes Bun.hash.wyhash the correct function to collide here — colliding any other hash would prove nothing. The test name says what (a (length, 32-bit hash) collision must not match); the comment said why this construction works. Without it, a future change to the router's segment-hash function silently turns this test into a no-op (the search would still find a wyhash collision, but the router would no longer be comparing that hash). The two deleted inline comments ("genuine segment matches" / "colliding segment must not") are admittedly narration and fine to drop.
Step-by-step. (1) Diff at lines 684–711 shows no behavioral change to this test — same 600k loop, same fixture, same three assertions. (2) The - lines remove exactly the 5-line block + 2 inline comments. (3) The + lines add only the rename, the low32 helper, and the , 60_000 timeout argument. (4) CLAUDE.md:238 separately says "don't ride file-wide standardization on a focused bugfix", which is what this hunk is.
The added timeout. test/CLAUDE.md:120 says "CRITICAL: Do not set a timeout on tests. Bun already has timeouts", and root CLAUDE.md:191 says "Don't raise per-test timeouts to make a slow test pass; shrink the workload." The workload here did not change (600k candidates before and after), so if the test was within budget before this PR there is no reason to add a timeout now, and if it was slow on debug builds the prescribed remedy is to lower the candidate count (the comment itself noted ~80k expected). One mitigating point the refuting verifier raised: the file already carries a pre-existing 60_000 at line 539 for another heavy-iteration test, so this addition is locally consistent and the no-timeout convention is evidently not strictly enforced in this file. That makes the timeout the weaker half of this finding.
Addressing the refutation. The dissenting verifier argued the deleted block itself violated the 3-line rule (CLAUDE.md:268) and "Regression tests get exactly one comment: the issue URL" (CLAUDE.md:239), so removing it was net-correct. But the "don't delete existing why-comments in cleanup passes" clause sits in the same sentence and specifically governs what to do with pre-existing explanatory comments during a cleanup — the 3-line/issue-URL norms describe what to write for new comments. And the refutation's "Bun.hash.wyhash being visible in the code" mitigation does not actually carry the lost fact: seeing Bun.hash.wyhash on line 685 tells the reader which hash the test computes, not that the router internally uses the same one with seed 0 for segment matching. That coupling to src/ is exactly the kind of "durable non-obvious content" the rule names.
Impact. None at runtime — test-only style/maintainability. The risk is forward-looking: the test's correctness depends on an undocumented coupling to the router's choice of hash function, and the per-test timeout sets a small precedent the convention asks to avoid. Hence nit.
Fix. Restore the why in compressed form (one or two lines is enough — e.g. // Bun.hash.wyhash(s, 0) is the router's static-segment hash; a birthday search over equal-length candidates finds a 32-bit collision in ~80k tries.), and either drop the 60_000 or, if debug-build timing is the concern, lower the 600k search bound instead. The rename and low32 extraction are fine to keep.
| } | ||
| } | ||
| } | ||
| if matches!(field, PackageField::Scripts) { |
There was a problem hiding this comment.
Deleted in b42fbd3. That block was the only mechanism behind the new "reads a registry package's lifecycle scripts from its package.json, not the binary lockfile" test, so that test is removed with it (a stored lockfile's script strings are used as-is again, as on main today); the neighboring scripts-flag byte validation you didn't flag is unchanged.
| ) | ||
| .expect("unreachable"), | ||
| skip_verify: false, | ||
| in_trusted_dependencies: this.lockfile.in_trusted_dependencies(name), |
There was a problem hiding this comment.
Deleted in b42fbd3 — the extractor never reads that field for tarball-tagged tasks (it always reads the package.json for those), so the lookup at this call site was pointless; it now passes a literal false.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/glob/path-length.test.ts`:
- Around line 87-137: Both glob path-length tests read scanStderr but never
validate it, so add an explicit stderr assertion in the overflow and Linux-only
matched-file cases. Update the test bodies around the Bun.spawn/scanProc checks
to verify stderr is empty or otherwise matches the expected diagnostics,
alongside the existing stdout and exitCode assertions, using the same scanStderr
variable in both tests.
In `@test/js/bun/util/filesystem_router.test.ts`:
- Around line 800-803: The new bunEnv subprocess tests collect stderr but never
verify it, unlike the file’s existing bunExe() convention. Update the relevant
test blocks around the referenced subprocess assertions to explicitly check that
stderr is empty with bunEnv, and apply the same fix in the matching second test
block so both use the established stderr expectation.
🪄 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: 5479f5f0-3ae2-4e3e-83fe-e501ab9f96c5
📒 Files selected for processing (11)
src/bundler/ParseTask.rssrc/glob/GlobWalker.rssrc/paths/resolve_path.rssrc/resolver/lib.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/webcore/Request.rstest/js/bun/glob/path-length.test.tstest/js/bun/http/serve.test.tstest/js/bun/util/filesystem_router.test.tstest/js/node/fs/fs.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/glob/path-length.test.ts`:
- Around line 87-137: Both glob path-length tests read scanStderr but never
validate it, so add an explicit stderr assertion in the overflow and Linux-only
matched-file cases. Update the test bodies around the Bun.spawn/scanProc checks
to verify stderr is empty or otherwise matches the expected diagnostics,
alongside the existing stdout and exitCode assertions, using the same scanStderr
variable in both tests.
In `@test/js/bun/util/filesystem_router.test.ts`:
- Around line 800-803: The new bunEnv subprocess tests collect stderr but never
verify it, unlike the file’s existing bunExe() convention. Update the relevant
test blocks around the referenced subprocess assertions to explicitly check that
stderr is empty with bunEnv, and apply the same fix in the matching second test
block so both use the established stderr expectation.
🪄 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: 5479f5f0-3ae2-4e3e-83fe-e501ab9f96c5
📒 Files selected for processing (11)
src/bundler/ParseTask.rssrc/glob/GlobWalker.rssrc/paths/resolve_path.rssrc/resolver/lib.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/webcore/Request.rstest/js/bun/glob/path-length.test.tstest/js/bun/http/serve.test.tstest/js/bun/util/filesystem_router.test.tstest/js/node/fs/fs.test.ts
🛑 Comments failed to post (2)
test/js/bun/glob/path-length.test.ts (1)
87-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Assert
stderrtoo, not just stdout/exitCode.Both new tests collect
scanStderrbut never assert on it. Since these tests specifically probe buffer-overflow-prone path-joining code, a native diagnostic/ASAN note on stderr (without necessarily flipping the exit code) would be valuable signal that's currently discarded.💡 Proposed fix (apply to both tests)
- expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG"); - expect(scanCode).toBe(0); + expect({ stdout: scanStdout.trim(), stderr: scanStderr, exitCode: scanCode }).toEqual({ + stdout: "ERR:ENAMETOOLONG", + stderr: "", + exitCode: 0, + });Apply the analogous change to the
OK:assertion in the Linux-only test (113-137).🤖 Prompt for 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. In `@test/js/bun/glob/path-length.test.ts` around lines 87 - 137, Both glob path-length tests read scanStderr but never validate it, so add an explicit stderr assertion in the overflow and Linux-only matched-file cases. Update the test bodies around the Bun.spawn/scanProc checks to verify stderr is empty or otherwise matches the expected diagnostics, alongside the existing stdout and exitCode assertions, using the same scanStderr variable in both tests.test/js/bun/util/filesystem_router.test.ts (1)
800-803: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Assert
stderrfor thesebunEnvsubprocess tests.
stderris collected but never checked in either new test (also applies to 833-837). Based on learnings, this file's own established convention is to assertexpect(stderr).toBe("")forbunExe()subprocesses spawned withbunEnv, sincebunEnvreliably suppresses benign ASAN/debug noise.💡 Proposed fix
- expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true"); - expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); + expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true"); + expect({ stderr, exitCode, signalCode: proc.signalCode }).toEqual({ stderr: "", exitCode: 0, signalCode: null });Apply the analogous change at 833-837.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true"); expect({ stderr, exitCode, signalCode: proc.signalCode }).toEqual({ stderr: "", exitCode: 0, signalCode: null }); }, 60_000);🤖 Prompt for 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. In `@test/js/bun/util/filesystem_router.test.ts` around lines 800 - 803, The new bunEnv subprocess tests collect stderr but never verify it, unlike the file’s existing bunExe() convention. Update the relevant test blocks around the referenced subprocess assertions to explicitly check that stderr is empty with bunEnv, and apply the same fix in the matching second test block so both use the established stderr expectation.Source: Learnings
|
Fixed in the latest push. On the row side: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/wrapAnsi.cpp (1)
150-181: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the shared ANSI parser in
trimLeadingSpaces().This scanner only recognizes
ESC ... mand BEL-terminated OSC. A leading valid sequence like"\x1b[2K foo"or an ST/C1 form leavesm_trimInEscapestuck, so the following spaces never trim and the cached row width stays wrong. ReusingANSI::isEscapeCharacter()plusANSI::consumeANSI()(or the existing terminator helper) here would keep trim behavior aligned with the rest of the file.🤖 Prompt for 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. In `@src/jsc/bindings/wrapAnsi.cpp` around lines 150 - 181, The trimLeadingSpaces scanner in wrapAnsi.cpp is using a custom escape-state loop that only handles a narrow set of ANSI sequences, causing m_trimInEscape and the cached width to get out of sync for valid leading escapes like CSI or ST/C1 forms. Update the trim logic to use the shared ANSI parser helpers already used elsewhere in this file, specifically ANSI::isEscapeCharacter() and ANSI::consumeANSI() (or the existing terminator helper), so trimLeadingSpaces() recognizes the same escape boundaries and correctly resumes trimming spaces after any valid ANSI sequence.Source: Coding guidelines
🤖 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 `@src/jsc/bindings/wrapAnsi.cpp`:
- Around line 62-70: skipLeadingAnsi() only strips the first ANSI escape, so a
word that begins with multiple adjacent escape sequences can still leave an ESC
at the seam and cause wordStartsNewCluster() and wordSeamIsAscii() to take the
wrong fast path. Update skipLeadingAnsi() in wrapAnsi.cpp to keep consuming
leading ANSI sequences until the first non-escape code unit is reached, and make
sure the seam classification logic uses that fully advanced pointer before
deciding whether to treat the word as ASCII.
---
Outside diff comments:
In `@src/jsc/bindings/wrapAnsi.cpp`:
- Around line 150-181: The trimLeadingSpaces scanner in wrapAnsi.cpp is using a
custom escape-state loop that only handles a narrow set of ANSI sequences,
causing m_trimInEscape and the cached width to get out of sync for valid leading
escapes like CSI or ST/C1 forms. Update the trim logic to use the shared ANSI
parser helpers already used elsewhere in this file, specifically
ANSI::isEscapeCharacter() and ANSI::consumeANSI() (or the existing terminator
helper), so trimLeadingSpaces() recognizes the same escape boundaries and
correctly resumes trimming spaces after any valid ANSI sequence.
🪄 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: f57fba34-418a-453b-a83f-8631c990e9c5
📒 Files selected for processing (2)
src/jsc/bindings/wrapAnsi.cpptest/js/bun/util/wrapAnsi.test.ts
| // A word may begin with ANSI escape sequences whose code units are all ASCII | ||
| // (ESC, '[', digits, 'm'), hiding the codepoint that actually lands on the seam. | ||
| // Skip them before classifying; a word not starting with ESC never enters the scan. | ||
| template<typename Char> | ||
| static inline const Char* skipLeadingAnsi(const Char* start, const Char* end) | ||
| { | ||
| if (start < end && ANSI::isEscapeCharacter(*start)) | ||
| return ANSI::consumeANSI(start, end); | ||
| return start; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip all leading ANSI sequences before classifying the seam.
skipLeadingAnsi() only consumes one escape. Inputs like "\x1b[31m\x1b[1m\u20E3bb" still leave the second ESC at the seam, so wordStartsNewCluster()/wordSeamIsAscii() take the additive fast path and keep the stale cached width.
Proposed fix
template<typename Char>
static inline const Char* skipLeadingAnsi(const Char* start, const Char* end)
{
- if (start < end && ANSI::isEscapeCharacter(*start))
- return ANSI::consumeANSI(start, end);
+ while (start < end && ANSI::isEscapeCharacter(*start)) {
+ const Char* next = ANSI::consumeANSI(start, end);
+ if (next == start)
+ break;
+ start = next;
+ }
return start;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A word may begin with ANSI escape sequences whose code units are all ASCII | |
| // (ESC, '[', digits, 'm'), hiding the codepoint that actually lands on the seam. | |
| // Skip them before classifying; a word not starting with ESC never enters the scan. | |
| template<typename Char> | |
| static inline const Char* skipLeadingAnsi(const Char* start, const Char* end) | |
| { | |
| if (start < end && ANSI::isEscapeCharacter(*start)) | |
| return ANSI::consumeANSI(start, end); | |
| return start; | |
| template<typename Char> | |
| static inline const Char* skipLeadingAnsi(const Char* start, const Char* end) | |
| { | |
| while (start < end && ANSI::isEscapeCharacter(*start)) { | |
| const Char* next = ANSI::consumeANSI(start, end); | |
| if (next == start) | |
| break; | |
| start = next; | |
| } | |
| return start; | |
| } |
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 66-66: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🤖 Prompt for 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.
In `@src/jsc/bindings/wrapAnsi.cpp` around lines 62 - 70, skipLeadingAnsi() only
strips the first ANSI escape, so a word that begins with multiple adjacent
escape sequences can still leave an ESC at the seam and cause
wordStartsNewCluster() and wordSeamIsAscii() to take the wrong fast path. Update
skipLeadingAnsi() in wrapAnsi.cpp to keep consuming leading ANSI sequences until
the first non-escape code unit is reached, and make sure the seam classification
logic uses that fully advanced pointer before deciding whether to treat the word
as ASCII.
|
Checked, but this one isn't reachable: |
| // Note: this `else if` condition is identical to the `if` | ||
| // above and is therefore unreachable; likely a bug | ||
| // (intended: `!new && old` → unsubscribe). | ||
| let _ = ws.unsubscribe(&[field as u8]); | ||
| let _ = ws.unsubscribe(&field.uws_topic()); |
There was a problem hiding this comment.
🟣 Pre-existing: this PR updates line 172 to the new uws_topic() encoding, but the surrounding else if at line 168 is byte-for-byte identical to the if at line 126 (new_bits.contains(bit) && !self.subscriptions.contains(bit)), so the line being edited is dead code — the in-tree comment at lines 169–171 already says exactly this. Since the PR is in this branch for the topic-encoding sweep anyway, swapping the condition to !new_bits.contains(bit) && self.subscriptions.contains(bit) (what the comment names) would make the edited ws.unsubscribe(...) actually fire when an HMR client drops a topic.
Extended reasoning...
What the bug is. In HmrSocket's SetSubscriptions handler, the if at line 126 and the else if at line 168 carry the identical condition — both are new_bits.contains(bit) && !self.subscriptions.contains(bit). An else if whose condition matches its preceding if is unreachable, so ws.unsubscribe(&field.uws_topic()) on line 172 never runs. The pre-existing inline comment at lines 169–171 already documents this ("this else if condition is identical to the if above and is therefore unreachable; likely a bug (intended: !new && old → unsubscribe)"). This PR's change to this file rewrites the call's argument from &[field as u8] to &field.uws_topic() (the 0xFF-prefixed encoding introduced in dev_server/mod.rs) without touching the dead condition, so it is updating a line that still cannot execute.
Step-by-step proof.
- The client sends a
SetSubscriptionsmessage;new_bitsis built from the payload (lines 119–123). - For each
fieldinHmrTopic::ALL, line 126 evaluatesnew_bits.contains(bit) && !self.subscriptions.contains(bit). - If that is true, control enters the
ifbody (subscribe) and theelse ifis never tested. - If that is false, the
else ifat line 168 evaluates the same expression, which is still false — the body is skipped. - Therefore line 172 is unreachable for every possible
(new_bits, self.subscriptions, bit)triple. - The bookkeeping at line 175,
self.on_unsubscribe(!new_bits & self.subscriptions), does run — buton_unsubscribe()(lines 323–344) only adjusts the DevServer's debug-visualizer counters and never callsws.unsubscribe(). Line 172 is the only place a per-topic uWS unsubscription would happen.
Why nothing else prevents it. on_unsubscribe() handles DevServer-side state (the emit_*_visualizer_events counters and the memory-visualizer timer), not the uWS pub/sub topic membership; and self.subscriptions = new_bits on line 176 only updates the local mask. So when a connected HMR client narrows its subscription set (e.g. drops MemoryVisualizer), the socket stays subscribed at the uWS layer to the topic it asked to drop, and DevServer::publish() continues delivering frames for it.
Why this PR interacts with it. The PR's purpose in this file is the HmrTopic::uws_topic() change (giving HMR topics a 0xFF-prefixed name no JS-side subscribe() string can collide with). The diff edits both the subscribe call (line 127) and the unsubscribe call (line 172) for that encoding sweep. The unsubscribe edit sits one line below a comment that names it as a bug and prescribes the fix; root CLAUDE.md:213 ("Fix the whole class in the same PR") applies — though it's reasonable to defer since fixing the condition is a behavior change (clients would actually start getting uWS-level unsubscriptions) orthogonal to a hardening pass.
Impact. Minor: an HMR socket that drops a topic from its subscription set keeps receiving published frames for that topic until disconnect. The local self.subscriptions mask is updated, so re-subscribing later still hits the subscribe arm correctly. This is dev-server-only and the leaked frames are small.
Fix. Swap the else if condition to the inverse named by the comment:
} else if !new_bits.contains(bit) && self.subscriptions.contains(bit) {
let _ = ws.unsubscribe(&field.uws_topic());
}and drop the now-redundant comment. The PR's mechanical update of the dead line is the correct thing to do for consistency regardless (if the condition is fixed later, the encoding should already match), so no objection to the PR's change as-is — this is a flag on the documented pre-existing bug it touches.
|
Agreed it's dead — the |
…always fires (#33998) Fixes `test/js/bun/jsc/bun-jsc.test.ts` going red on the debian 13 x64 lane in [build 72010](https://buildkite.com/bun/bun/builds/72010#019f546c-0875-422c-8f22-fa88eac52a1b): ``` 215 | expect(result2.stackTraces.traces.length).toBeGreaterThan(0); error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 ✗ bun:jsc > profile can be called multiple times [4.71ms] ``` ### Cause The test profiles `fib(26)` three times at a 50µs sample interval and asserts each run collected at least one trace. #33072 shrank the workload from `fib(30)` to `fib(26)` so three runs fit inside the per-test timeout on slow debug builds. On fast release hardware a JIT-compiled `fib(26)` can finish inside one 50µs interval (the whole 3-call test completed in 4.71ms there), so the sampler thread simply never fires during the second call. This is the same race #28873 addressed by lowering the sample interval; lowering the instruction count in #33072 reopened it. ### Fix Loop `fib(18)` for a fixed 10ms of wall-clock per `profile()` call instead of relying on a single `fib(n)` being slow enough. 10ms is ~200 sample intervals, so the sampler thread is guaranteed time to fire regardless of JIT tier or how slowly it wakes after `pause()`/`start()`. On debug builds the loop still exits after ~12ms of JS work (one `fib(18)` of overshoot), so the test is no slower there than before. The test still catches the original regression from #25939 (using `shutdown()` instead of `pause()`): a shut-down sampler returns zero traces no matter how long the workload runs. ### Verification - `USE_SYSTEM_BUN=1 bun test test/js/bun/jsc/bun-jsc.test.ts -t 'profile can be called multiple times'`: 30/30 pass, ~80 traces per call - `bun bd test test/js/bun/jsc/bun-jsc.test.ts`: 36 pass / 0 fail, profile test ~1.6s (dominated by the profiler's JSON reporting, same as before) This is a timing flake that does not reproduce on the dev container (`fib(26)` takes ~800µs there after JIT warmup, well above 50µs), so there is no deterministic fail-before to show; the CI failure above is the reproduction. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
4 participants
What
A hardening and robustness pass across the runtime: input validation, bounds checking, protocol-state handling, and object-lifetime correctness in ~200 files. It contains 122 individual fixes and ~190 new tests (171 new
test/itblocks, several parameterized, across 79 existing test files). No new API is introduced; every behavioral change below has a test unless explicitly noted, and each is aligned with Node, the relevant RFC/spec, or the upstream reference implementation.Potentially breaking / behavior-visible changes
Read this section first. Everything else in the PR preserves behavior for valid inputs.
Bun.serverequest.urlis only synthesized from a structurally validHost. For every HTTP/1.x request (Bun.serveand node-compat servers alike), aHostvalue that is empty or contains bytes outsideuri-host [":" port](RFC 3986 authority: alphanumerics,.-:_~%[]and sub-delims) is never used as the authority of the synthesizedrequest.url;request.urlfalls back to the request target (e.g./path) and the request is still served. No request is rejected on the basis of theHostfield value, and a validHoststill round-trips intorequest.urlexactly. Why:request.urlshould never carry an authority that cannot come back out ofnew URL().fetchrejects request lines it cannot legally serialize. A URL path/host (or, for proxied requests, the full href) containing a control character, space, or DEL now fails withInvalidURLbefore any bytes are written (RFC 9112 request-line grammar; normalfetch()input is percent-encoded by the URL parser and unaffected). Also: a redirect whoseLocationresolves to a non-http(s) scheme now fails withUnsupportedRedirectProtocol(Fetch spec, matches undici); a101arriving on the pre-tunnel leg of a proxied request is treated as an unrequested upgrade; connections whose identity was accepted by a per-requestcheckServerIdentitycallback are never entered into or taken from the keep-alive pool.__proto__key from data files and macros is printed as a computed key. Thejson,jsonc,json5,toml,yaml, and CSS-module loaders — and objects returned from Bun macros — now emit["__proto__"]: ...so importing such data yields an own__proto__property (likeJSON.parse) instead of a prototype assignment. Who notices: only code importing data with a__proto__key; matches esbuild's JSON loader and Node semantics.node:urllegacyurl.parselookup tables no longer inherit fromObject.prototype(bothnode:urland the browser fallback), the hostless/slashed lookups use the lowercased protocol, andurl.parse(s, true).queryis a null-prototype object (empty query included). All three match Node'slib/url.jsexactly. Who notices: code doingquery.hasOwnProperty(...)or parsing schemes named liketoString:.new WebSocket(url, ["a"])requested subprotocols and the server's 101 omitsSec-WebSocket-Protocol, the connection now closes with 1002 instead of opening withws.protocol === "". Matches browsers,ws, and undici. Connections that request no subprotocol are unaffected.content-lengthmust be1*DIGIT, non-duplicated, and equal to the DATA actually received (CONNECT exempt) — violations get RST_STREAM(PROTOCOL_ERROR) instead of being delivered. WithmaxSessionMemoryexceeded, new peer streams are refused with REFUSED_STREAM (retryable), and reset streams promptly release their native state — Node/nghttp2 parity throughout. The all-streams teardown helper now throws aTypeErrorfor a non-numeric error code instead of coercing it per stream.node:http2HTTP/1 fallback (allowHTTP1) frames responses like Node. Header-name matching is case-insensitive; HEAD and close-delimited responses don't get an autoTransfer-Encoding: chunked/terminating chunk;writeHeadnow throwsERR_HTTP_INVALID_STATUS_CODE/ERR_INVALID_CHARlike Node'sServerResponse. Re-entrantsendTrailers()raisesERR_HTTP2_TRAILERS_ALREADY_SENTin the same order Node does.node:http(s)proxyCONNECTendpoint is validated withvalidateHeaderValuein release builds (previously a debug-only assertion), so an invalid host/port surfaces as the same error Node throws.followSymlinks, a link that resolves to one of its own live ancestors is descended exactly once (likefind -L, glibcfts, node-glob); sibling/cousin links to the same target are still all visited. One pre-existing test changed: it previously asserted the walk failed withENAMETOOLONGafter the path grew past the limit; it now asserts the scan completes.exports/importstarget whose expansion would exceed the OS path limit is a normal resolution error (Invalid module specifier/Invalid package target, as Node models it) instead of a hard failure.if/then/elif/else/fiis treated as data, never as a reserved word (POSIX: reserved words are only recognized literally);$.escapenow quotes strings containing tab, CR, or?(word delimiters / glob metacharacters).bun pack/bun publishinclude/exclude matches npm-packlist. With a"files"field, the non-overridable defaults (.git,.npmrc,node_modules, lockfiles) are now applied inside thefilestraversal too; conversely.hgmoved to the overridable default-ignore list, so"files"can re-include it — exactly npm's split.bun upgradeverifies the downloaded artifact against the digest the GitHub Releases API reports for that asset, and fails with a retryable error on mismatch. If the API reports no (or an unrecognized) digest, behavior is unchanged.integritystring carrying several space-separated digests (legal SSRI) is now parsed correctly and verified against the strongest algorithm present (see Deviations); a storedbun.lockbwith a non-0/1 byte in a boolean slot fails validation instead of being reinterpreted; lifecycle scripts for registry packages always come from the installedpackage.json(never from lockfile bytes), matching what Bun writes and what npm does; isolated installs apply the same name/alias shape validation as hoisted installs; bin links reached through a subdirectory get the same resolved-containment check the dotted forms already had (npm only links files inside the package folder).node:fs:modearguments are no longer masked to0o777(setuid/setgid/sticky pass through to the syscall, like Node);copyFile/cpcreate the destination with the source's permission bits (libuv parity); on Windows,cpcopies directory junctions/symlinks via the unprivileged-create + junction-fallback helpers and rewrites\\?\UNC\targets to\\server\shareform (libuv parity), so copying a tree with junctions works without elevation; on macOS theclonefile/openatpaths useNOFOLLOWso the copy matches thelstatclassification (dereference:false).new Headers(obj), fetch init,URLSearchParams, …) snapshots the key list once and re-resolves keys mutated by a converter, exactly as Web IDL specifies (deleted keys skipped, replaced values re-read) — released Bun/Node/WebKit order preserved;TextDecoder.decodeover aSharedArrayBufferor resizable buffer view snapshots the bytes first; consuming aBlob/Responsebody no longer empties other objects sharing the same byte store (transfer only when sole owner); deeply nested serialized arrays instructuredClonedata hit the same recursion cap objects already had; the SIMDdecodeURIComponentfast path decodes non-ASCII input as UTF-8 (with U+FFFD for ill-formed sequences) instead of throwing/garbling.napi_create_arraybufferreturns zeroed memory (Node contract);napi_get_typedarray_info/napi_get_dataview_inforeport the view's realbyte_offset;v8::String::Utf8Lengthreturns the exact byte countWriteUtf8will produce for ill-formed UTF-16;v8::Number::Newcanonicalizes NaN payloads.Host/Origin: the inspector (bun --inspect) HTTP/WebSocket endpoint applies its Host/Origin checks before the/json* discovery routes and rejects non-matching DNS-nameHostvalues with 400 (Node inspector semantics); the bake/dev-server internal routes require an allowed Host and same-origin for the error-report/sourcemap endpoints; internal HMR pub/sub topics are namespaced so userpublish/subscribetopic strings can never collide with them.16 × min(input, 64 KiB)scale); once exhausted, further references degrade to literal bracketed text — no error — exactly as md4c does.checkPrimevalidates the candidate before the options (Node's order); HKDF rejects non-secretKeyObjects withERR_CRYPTO_INVALID_KEY_OBJECT_TYPE(current Node); Ed25519 sign/verify with a wrong-length key errors/returns false; X25519 JWK import honorskty/crv/use/key_ops/ext; SPKAC helpers return false/empty for empty or whitespace-only input (Node);CookieMap.deleteof a__Host-/__Secure-cookie emitsSecureso browsers accept the expiry; postgresescapeIdentifierrejects embedded NUL (pgparity); valkey/redis pending commands reject with "Connection closed" on disconnect and out-of-band push frames never consume an unrelated command's promise (RESP3); semver strings consisting only ofv/=/whitespace parse as*(node-semver);Bun.wrapAnsimeasures rows whose seam joins grapheme clusters (combining marks/ZWJ/VS16) the way npmwrap-ansidoes; bash/zsh completions handle script names containing:and other special characters; the Docker images verify (not just decode) the release checksum signature.Changes by area
Bun.serve/node:http) —packages/bun-uws/HttpParser.h,packages/bun-usockets,src/runtime/webcore/Request.rs,src/runtime/server: therequest.urlHosthandling above (URL synthesis inRequest.rsonly — the HTTP parser'sHosthandling is unchanged and every request is served); CONNECT requests are framed as an opaque tunnel regardless ofTransfer-Encoding/Content-Length(RFC 9110 §9.3.6); TLS socket relocation updates the loop's spill/last-error owner pointers so bookkeeping never points at a moved socket; the bake-only route additionally requires an allowedHost.src/http/lib.rs,src/http/ssl_config.rs,src/runtime/webcore/{Request,Blob,TextDecoder}.rs,src/jsc/bindings/webcore/*,src/jsc/bindings/decodeURIComponentSIMD.cpp,src/url/lib.rs,src/runtime/api/BunObject.rs: everything in the highlights, plus:write_requestfailures propagate their real error instead of being reported as out-of-memory; partial-header/1xx short reads no longer re-feed already-consumed bytes to the parser;SSLConfignow actually appliessecureOptionsand the client-renegotiation limit/window it was already accepting;URLSearchParamsno longer drops a pair whose value has a malformed percent sequence (WHATWG: never drop, decode lazily);AbortSignalnative listeners deregistered by an earlier abort callback are not invoked with a stale context; the compression helpers (Bun.gzipSyncet al.) read the options object before coercing the input buffer (so a getter can't invalidate the captured slice) and no longer register a deallocator for an empty result's dangling sentinel pointer (an invalid free at GC time under debug allocators).src/runtime/node/*,src/js/node/*,src/node-fallbacks/url.js,src/jsc/ipc.rs,src/runtime/socket: everything in the highlights, plus:fspath arguments from typed arrays are always pinned for the call's duration;BlockListstructured-clone carries only an opaque per-instance nonce resolved through a live table (round-trip unchanged); IPC advanced-mode frame lengths are range-checked before span arithmetic, serialization failures leave no partial frame in the send queue, and a received fd is closed if its message fails to parse; TLS-upgradeinitialDatais copied to an owned buffer before use;node:wasiinterprets rights bitfields as unsigned u64 (per the ABI) and no longer reports success for apath_openthat threw internally; the inspector/debugger endpoint changes above.src/runtime/api/bun/h2/connection.rs,h2_frame_parser.rs,src/http_jsc/*: everything in the highlights, plus: 1xx interim responses are not misclassified as trailers; refused streams still HPACK-decode the discarded block (RFC 9113 §4.3) and advancelast_stream_idso pipelined RST_STREAMs don't become connection errors; the frame parser no longer holds an exclusive stream reference across calls back into JS (options/header getters,toStringcoercions) — engine-side stream eviction is deferred until the dispatch unwinds; the trailers failure path still ends the stream with FRAME_SIZE_ERROR + graceful GOAWAY so in-flight streams stay retryable; the deflate plumbing gains a per-VM slot (no behavior change yet).src/install/*,src/runtime/cli/{pack,create,upgrade}_command.rs,src/semver,packages/bun-release,packages/bun-vscode: everything in the highlights, plus: SSRI option suffixes (?...) are stripped from digest payloads; a GitHub dependency whose resolved ref would not form a single well-formed folder name is refused with a clear error (real refs/SHAs always pass); the trusted-dependency lookup moved off the extraction worker thread (no user-visible change);bun create'spackage.jsonrewrite uses the CLI arena so the parsed AST outlives its uses; the npm installer package validates archive entry paths stay inside the destination; the VS Code lockfile preview escapes interpolated text and the debug adapter's session id comes fromcrypto.randomBytes.src/resolver,src/bundler,src/parsers/{json,json5,yaml}.rs,src/ast/e.rs,src/js_parser/lexer.rs,src/js_parser_jsc/Macro.rs,src/jsc/RuntimeTranspilerStore.rs,src/jsc/bindings/BunPlugin.cpp,src/sourcemap,src/md,src/paths,src/standalone_graph: the__proto__, resolver-limit, and markdown items above, plus: a native-pluginonLoadsource buffer now has exactly one owner (its free callback was registered twice);onResolvecallback lists are snapshotted (GC-visible) before user callbacks run, so a callback registering more plugins can't perturb the in-progress dispatch; barrel-import scheduling copies its alias seeds instead of holding references into a map the BFS mutates; embedded bytecode caches are handed toResolvedSourceas a genuinely owned allocation; the lazy sourcemap decompression cache becameOnceLock-based (shared-reference safe); the lexer's SIMD long-string fast path advances past scanned bytes (removes a quadratic re-scan on unterminated literals);is_parent_or_equaluses a true prefix check instead of substring containment.src/shell_parser,src/runtime/shell,src/glob/GlobWalker.rs,completions/bun.{bash,zsh},src/jsc/bindings/wrapAnsi.cpp: the highlights above; the glob walker's followed-link tracking is scoped to the live ancestor chain (DAG revisits still enumerate);wrapAnsialso caches row widths so the seam fix comes with fewer full-row rescans.src/jsc/bindings/{ncrypto.cpp,node/crypto/*,webcrypto/*}: the highlights above, plus: the sign/verify job copies signature bytes out of the JS view before the async job runs;ECDH.convertKeyandprepareAsymmetricKeycapture buffer spans only after argument coercions that can run user JS; deserializedCryptoKeys re-validate that the algorithm matches the key class (and an empty key payload is rejected); an OOM while encoding returns after throwing.src/sql,src/sql_jsc,src/js/internal/sql,src/runtime/valkey_jsc,src/valkey,src/s3_signing: the highlights above, plus: postgresCopyDatapayload length is computed per the wire protocol (was one byte short) andPortalSuspended/Copy*messages are consumed instead of desynchronizing the stream; MySQL zero-lengthAuthSwitchRequest/LocalInfileRequestpackets are clean protocol errors instead of a length underflow; prepared-statement caches are keyed by the full statement name, not a 64-bit hash (a hash hit is verified by equality); the distributed-transaction name is type-checked; the S3 region used to synthesize a host must be host-safe and endpoint parsing is index-safe on odd endpoint strings.src/jsc/bindings/{napi.cpp,v8/*,ZigException.cpp,ZigGlobalObject.cpp,CookieMap.cpp,sqlite/JSSQLStatement.cpp},src/jsc/rare_data.rs,src/runtime/webview: the N-API/V8 items above; stack-trace population skips out-of-range frame indices instead of asserting; the native microtask trampoline is hidden from stack traces;bun:sqlitedetects a database closed re-entrantly from inside a bind coercion and throws "Database has closed"; the macOS webview bridge type-checks the objects a page posts to its internal message handler.src/runtime/bake/*,dockerhub/*,.github/workflows/update-vendor.yml: the dev-endpoint gating and HMR topic namespacing above; the dev-server terminal error report blanks non-UTF-8 bytes (not just encoded C1); the React SSR flight-data inliner escapes the fully decoded string instead of per-chunk (a boundary-split escape character could previously produce a malformed inline script); Docker images usegpg --verify; the vendor-update workflow passes matrix values throughenv:.Deviations / decisions
integrityfield carries several digests, Bun verifies the strongest supported algorithm present; when several digests of that same algorithm are present, the first is the one verified. npm/ssri accepts a match on any digest of the chosen algorithm — keeping the full set needs plumbing through the manifest cache/lockfile types and is left for a follow-up.fetch. A per-response decompressed-body limit was implemented and then deliberately removed from this PR ("Keep fetch response decompression unbounded"): Node imposes none, and any cap is a behavior change for legitimate large responses. The net diff has no decompression change.[[GetOwnProperty]]/Getinterleaved with value conversion, so atoStringthat mutates a sibling property is observed and a deleted one skipped, the same as released Bun, Node, and WebKit. The property table is never held across user code.cache::Entry.external_free_function(plusEntry::newand the free branch ofEntry::deinit) andAlreadyBundled::bytecode_slice; thebun_wyhashdependency ofsql_jscis dropped.node:httpsTLS option set), websocket: do not hold an exclusive client reference across user callbacks #33055 (websocket dispatch re-entrancy), resolver: take the per-entry lock when rewriting cached directory entries #33056 (FileSystemRouter/resolver entry locking), child_process: honor uid and gid spawn options #33060 (child_processuid/gid), node:http: enforce server headersTimeout and requestTimeout #33061 (node:httpserver headers/request timeouts). Nothing from those PRs is claimed here.How it was verified
bun bd(full debug build) andcargo checkclean with zero warnings;bun run rust:check-allpasses 10/10 targets (the change set includes Windows- and macOS-gated code); clippy lints raised on the touched files were addressed.test/itblocks across 79 files, several parameterized). Each was verified to fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>(except the handful noted above with no JS-observable assertion); the touched suites were run in full to confirm no regressions.test/js/node/http2/h2-conformance.test.ts) and Node's own http2 tests; thenode:httpHost behavior was checked against Node's conformance test for accepted host values; the record-conversion ordering was checked against WebKit/Node observable order.