Harden 36 reachable security findings across runtime, install, parsers, http

[Jarred-Sumner]

Hardens 36 reachable security findings across the runtime, package manager, parsers, HTTP client/server, and SQL drivers. Three auto-applied fixes (#61 SSL exception leak, #68 YAML merge dedup, #104 archive overwrite precheck) were dropped: #61 introduced a use-after-free, #68 stored a non-'static byte view in a 'static field, and #104 added dead gating that did not close the traversal.

Memory safety / lifetime

• Fix calling #private() functions in classes #2 — Dangling proxy slice across reentrant JS getter — copy process.env proxy href to an owned Vec before reentrant getters can free the env map (Blob.rs)
• Fix errors in bun bun (broke after threading) #15 — Rollback restores dangling editor name pointer — preserve and restore name_storage on detect_editor failure (BunObject.rs)
• Support github: dependencies #81 — Reentrant reconnect frees live handlers — only free previous handlers when active_connections == 0 (Listener.rs)
• Add test coverage for symlinks in static file dir #110 — Async randomFill uses stale resizable buffer pointer — fill a worker-owned scratch buffer; copy back on the JS thread after re-validating bounds (node_crypto_binding.rs)
• global install has been implemented, reflect in readme #119 — Null zero-length slice UB in DOMJIT fast path — use ffi::slice which tolerates (null, 0) (Crypto.rs)
• Create a build of Bun that doesn’t need AVX2 instructions #67 — Raw serialization reads struct padding bytes — add explicit _padding_* fields with offset_of! proof asserts (npm.rs)
• SyntaxError: Indirectly exported binding name 'default' cannot be resolved by star export entries #74 — TLS rejection path leaks websocket refcount — route SSL/auth failures through self.fail() which clears outgoing_websocket (websocket_client.rs)
• Improve error message when async is missing from function #108 — FD-backed fetch body leaks duplicated descriptor — close opened_fd unconditionally after read_file (fetch.rs)

Untrusted-input bounds / panics

• Rewrite the CLI to use subcommands and be clearer #10 — Invalid lockfile tag causes panic DoS — replace unreachable!() with logged error + Tag::Uninitialized (dependency.rs)
• Always replace global with globalThis #20 — Unchecked lockfile string offsets cause OOB slice — bounds-check non-inline String pointers against ctx.buffer (dependency.rs)
• github actions #91 — Panic on unvalidated resolution tag — validate ResolutionTag discriminants on lockfile load (Package.rs)
• Colorize log output for errors printed to STDERR #24 — Unwrap panic on unexpected 304 response — return UnexpectedNotModified when no cached manifest exists (npm.rs)
• "Imported binding name not found" error missing source file name #44 — UDP port getter unwrap panic on transient state — return undefined when socket is None (udp_socket.rs)
• Support macros in Bun.js SSR #36 — Close reason length mismatch causes panic — clamp body_len to 125 and bail on overlong UTF-8 transcode (websocket_client.rs)
• bun v0.0.66 #100 — Windows pipe name length panic DoS — debug_assert → real bounds check (Listener.rs)
• Remove another empty file #60 / Fix detecting index.html from root directory #111 — Windows shim stack buffer overflows — bounds-check argument and filename writes against BUF1_LEN/BUF2_U16_LEN before copy_nonoverlapping (bun_shim_impl.rs)
• Add experimental nix support #76 / Improve reliability of bun install #101 — Unchecked bin name/entry name copies — bounds-check before slicing into abs_dest_buf (bin.rs)
• Bun v0.0.55 #79 — if keyword misclassification causes parser panic — require a delimiter token before classifying (shell_parser/parse.rs)
• Fix macro loading so it doesn't need a symlink #32 — Bounds check occurs after UTF-16 write — pre-flight key/value lengths before convert_utf8_to_utf16_in_buffer (env_loader.rs)
• Zig stage1 compiler needs 11 GB of ram to compile a debug build of Bun #95 — PBKDF2 digest validation allows panic-only algorithm — reject digests with no EVP_MD (PBKDF2.rs)

DoS / resource caps

• Fix JSX parser bug: // comment after tagName and before closing tag is broken #17 — Unbounded recursion on deep TOML dotted keys — cap dotted-key segments at 512 (toml.rs)
• Allow build to output code that runs on Node.js #39 — Unbounded brace expansion preallocation — cap expansion count at 65536 in Bun.$ and Bun.braces (BunObject.rs, Expansion.rs)
• add npm install command before running bun #31 — SCRAM PBKDF2 parameters accepted from server — clamp iteration count to [4096, 10M], salt length to [1, 1024] (PostgresSQLConnection.rs)

Auth / injection / traversal

• test #19 — Cleartext password sent after TLS downgrade — require TLSStatus::SslOk, not just ssl_mode != Disable (MySQLConnection.rs)
• Support workspace: dependencies #83 — Strict TLS request reuses lax-verified pooled socket — track established_with_reject_unauthorized and refuse pool reuse for strict callers (HTTPContext.rs, lib.rs, ClientSession.rs)
• Add support for npm_package_config_* variables #73 — IPv6 loopback prefix auth bypass — exact-match ::1 instead of starts_with (server_body.rs)
• Add a fast npm install to Bun's CLI #56 — Unsanitized filename injects response headers — reject \r/\n/NUL/" in content-disposition filenames (RequestContext.rs)
• Windows Support #43 — Missing CRLF checks for signed host/auth headers — also validate region, access_key_id, and host (s3_signing/credentials.rs)
• Fix regression with "\0" #34 — Bucket slash enables S3 host confusion — reject buckets containing / (s3_signing/credentials.rs)
• Print import stack for errors during bun bun #25 — Lexical symlink check permits extraction escape — track created symlinks during extraction and refuse paths that traverse them (libarchive/lib.rs)
• documentation updates for react section to easily integrate with existing CRA #71 — bunx executes untrusted temp-cache binary — lstat cached binary; refuse symlinks and other-uid files (bunx_command.rs)

Permission hygiene

• Configurable feature map #6 — Bin target chmod always sets mode 0777 — 0o777 & !umask instead of umask | 0o777 (bin.rs)
• Disable reuseport, automatically choose next closest port on error.AddressInUse #23 — Process umask cleared and never restored — restore umask after probing it in ensure_umask (bin.rs)

Parser correctness

• Bare imports of CommonJS modules transformed to ESM should be eagerly-loaded to match Node.js behavior #22 — Sign-prefixed scalar misparsed as infinity — fix Zig→Rust &&/|| precedence transliteration (yaml.rs)

[robobun]

^{Updated 12:20 AM PT - May 15th, 2026}

❌ @Jarred-Sumner, your commit d9f2e8f has 1 failures in Build #54523 (All Failures):

• scripts/build/ci.ts - build failed on 🍎 x64 - build-cpp
• scripts/build/ci.ts - build failed on 🍎 aarch64 - build-cpp

---

🧪   To try this PR locally:

bunx bun-pr 30722

That installs a local version of the PR into your bun-30722 executable, so you can run:

bun-30722 --bun

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR hardens many subsystems: tracks TLS strictness for socket/session reuse, adds pre-write capacity checks, stages async randomFill output safely, validates lockfile/struct layouts, sanitizes headers and S3 inputs, prevents handler use-after-free, and enforces parser/explosion limits.

Changes

Security Hardening Across HTTP, Serialization, and System APIs

Layer / File(s)	Summary
HTTP/TLS request strictness and session reuse src/http/HTTPContext.rs, src/http/h2_client/PendingConnect.rs, src/http/h2_client/ClientSession.rs, src/http/lib.rs	Pooled sockets, h2 pending-connects, and client sessions record whether their TLS handshake used rejectUnauthorized. Release paths receive caller reject_unauthorized and reuse/multiplexing logic blocks strict callers from reusing sessions/sockets established without hostname verification.
Socket lifecycle and reconnect safety src/runtime/socket/socket_body.rs, src/runtime/socket/Listener.rs	Deferred cleanup guards capture the original handlers pointer and conditionally null/drop handlers only when still pointing to the captured allocation to avoid freeing callbacks during synchronous reconnects.
File-descriptor RAII in fetch read_file src/runtime/webcore/fetch.rs	Simplifies Path/FD selection, protects opened FD with a scopeguard to auto-close on early exit, wires the guarded FD into read_file, and eagerly closes the FD before producing the JS result.
Installer bounds & Windows shim validation src/install/bin.rs, src/install/windows-shim/bun_shim_impl.rs	Atomically probes/stores umask, adjusts chmod computation, and adds remaining-capacity/length checks when assembling installer paths and Windows shim command-lines, returning defined errors on overflow or invalid metadata.
WebSocket handshake and close-path fixes src/http_jsc/websocket_client.rs	Handshake SSL/identity failures now call fail(...) and return early; close-frame reason payloads are clamped to the control-frame limit before sending.
Shell parser token classification src/shell_parser/parse.rs	Refactors is_if_clause_text_token delegation and tightens peek_any_ifclausetok/IfClauseTok::from_tok so keywords are recognized only when followed by a delimiter.
bunx cached-binary trust check (Unix-only) src/runtime/cli/bunx_command.rs	bunx lstat-checks cached candidates and refuses untrusted cached binaries (not uid-owned or symlink-to-unowned), forcing cache-bust and reinstall.

Suggested reviewers

• alii
• dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Harden 36 reachable security findings across runtime, install, parsers, http' clearly and specifically summarizes the main purpose of the PR: hardening security vulnerabilities across multiple components.
Description check	✅ Passed	The PR description covers both required template sections with detailed information: it explains what the PR does (hardening 36 security findings with specific examples grouped by category) and how it was verified (through grouped security fixes with issue references).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 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/http_jsc/websocket_client.rs`:
- Around line 1322-1323: The close-frame reason cap is wrong: the WebSocket
close payload must include a 2-byte status code so the reason must be limited to
125 - 2 = 123 bytes. Update send_close_with_body (and the other similar sites
you noted) to use a 123-byte cap instead of 125: change the body buffer
type/size and any min(...) that currently uses 125 to 123 so reason_len + 2 <=
125, and ensure any code that writes the 2-byte status code still prepends those
two bytes before the reason. Also update the other functions/methods at the
other occurrences (the ones with body: Option<&mut [u8; 125]>, body_len: usize
or body_len.min(125)) to use 123 consistently.

In `@src/libarchive/lib.rs`:
- Around line 1325-1341: The current check in path_traverses_created_symlink
only looks at the in-memory created_symlinks list and misses symlink ancestors
that already exist on disk, allowing escapes; change the logic so that instead
of (or in addition to) consulting created_symlinks, you walk each path prefix
on-disk with no-follow semantics (e.g., use lstat/fstatat with
AT_SYMLINK_NOFOLLOW or equivalent) relative to the extraction dir FD and treat
any prefix that is a symlink as a traversal => return true. Update the function
(path_traverses_created_symlink) signature or add a new helper to accept the
target dir FD (or a closure to perform no-follow stat) and replace the callers
that currently rely only on created_symlinks (the checks around mkdirat/openat
where created_symlinks is passed) so they invoke the on-disk ancestor check
before creating directories/files. Ensure existing created_symlinks checks
remain for symlinks created earlier in this run, but always perform the on-disk
no-follow ancestor walk to close the vulnerability.

In `@src/runtime/node/node_crypto_binding.rs`:
- Around line 669-675: The allocation vec![0u8; size] inside the JobCtx creation
is fallible and can OOM panic; change it to a fallible allocation using
Vec::try_reserve_exact (or try_reserve) and handle the Err by returning a JS OOM
error instead of letting the process abort. Concretely, create a mutable
Vec<u8>, call try_reserve_exact(size) and if it returns Err convert/propagate
that into the function's JS OOM error path, otherwise resize the vec to size and
assign Some(scratch) to JobCtx.scratch; update the randomFill path that
constructs JobCtx to use this pattern instead of vec![0u8; size].

In `@src/sql_jsc/postgres/PostgresSQLConnection.rs`:
- Around line 2730-2744: Reject overly long base64-encoded salts before calling
bun_base64::decode_alloc to avoid large allocations: in the SASL handling code
that currently calls bun_base64::decode_alloc(cont.s.slice()) and then checks
server_salt_decoded_base64.len(), first check cont.s.slice().len() against a
safe encoded limit (compute encoded_max for decoded limit 1024, e.g.
ceil(4/3*1024)+padding) and return AnyPostgresError::InvalidMessage if the
encoded length exceeds that limit; only then call bun_base64::decode_alloc and
keep the existing decoded-length sanity check on server_salt_decoded_base64 and
the debug message.

🪄 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: 3bf516d6-b4c5-4a34-9f66-985e53caa19c

📥 Commits

Reviewing files that changed from the base of the PR and between 63035b3 and 8dccb2a.

📒 Files selected for processing (29)

• src/dotenv/env_loader.rs
• src/http/HTTPContext.rs
• src/http/h2_client/ClientSession.rs
• src/http/lib.rs
• src/http_jsc/websocket_client.rs
• src/install/bin.rs
• src/install/dependency.rs
• src/install/lockfile/Package.rs
• src/install/npm.rs
• src/install/windows-shim/bun_shim_impl.rs
• src/libarchive/lib.rs
• src/parsers/toml.rs
• src/parsers/yaml.rs
• src/runtime/api/BunObject.rs
• src/runtime/cli/bunx_command.rs
• src/runtime/crypto/PBKDF2.rs
• src/runtime/node/node_crypto_binding.rs
• src/runtime/server/RequestContext.rs
• src/runtime/server/server_body.rs
• src/runtime/shell/states/Expansion.rs
• src/runtime/socket/Listener.rs
• src/runtime/socket/udp_socket.rs
• src/runtime/webcore/Blob.rs
• src/runtime/webcore/Crypto.rs
• src/runtime/webcore/fetch.rs
• src/s3_signing/credentials.rs
• src/shell_parser/parse.rs
• src/sql_jsc/mysql/MySQLConnection.rs
• src/sql_jsc/postgres/PostgresSQLConnection.rs

[github-actions]

Found 3 issues this PR may fix:

1. Bun.serve mTLS: rejectUnauthorized: true is non-deterministically enforced — untrusted client certs intermittently accepted #27985 - Fix Support workspace: dependencies #83 tracks established_with_reject_unauthorized on pooled TLS sockets, directly addressing the non-deterministic mTLS enforcement caused by strict callers reusing lax-verified pooled connections
2. Header injection via unescaped Content-Disposition filename #26959 - Fix Add a fast npm install to Bun's CLI #56 rejects \r/\n/NUL/" in content-disposition filenames in RequestContext.rs, directly addressing this header injection vector
3. Segfault in the YAML parser #28917 - Fix Bare imports of CommonJS modules transformed to ESM should be eagerly-loaded to match Node.js behavior #22 corrects an operator precedence bug in YAML sign-prefixed scalar parsing (yaml.rs), which could contribute to parser crashes like this segfault

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #27985
Fixes #26959
Fixes #28917

🤖 Generated with Claude Code

[github-actions]

This PR may be a duplicate of:

1. Input validation and bounds-checking hardening #30681 - Broad security hardening PR with 9 overlapping files (websocket_client.rs, bin.rs, libarchive/lib.rs, yaml.rs, RequestContext.rs, Expansion.rs, Blob.rs, parse.rs, MySQLConnection.rs)
2. fix(sql): enforce minimum PBKDF2 iteration count in SCRAM-SHA-256 auth #26932 - Enforces minimum PBKDF2 iteration count in SCRAM-SHA-256 auth, duplicating finding add npm install command before running bun #31 (clamp SCRAM PBKDF2 iterations)
3. fix(shell): prevent integer overflow in brace expansion variant counting #26992 - Prevents integer overflow in brace expansion, duplicating finding Allow build to output code that runs on Node.js #39 (cap brace expansion count)
4. fix(install): normalize bin target paths to stay within package directory #27500 - Normalizes bin target paths to prevent traversal, duplicating findings Add experimental nix support #76/Improve reliability of bun install #101 and Configurable feature map #6/Disable reuseport, automatically choose next closest port on error.AddressInUse #23 in install/bin
5. Harden multipart/form-data encoding per WHATWG spec #27505 - Hardens multipart/form-data encoding, duplicating finding Add a fast npm install to Bun's CLI #56 (CR/LF/NUL in content-disposition filenames)
6. fix(sql): harden MySQL TLS handshake by clearing read buffer after upgrade #27508 - Hardens MySQL TLS handshake, duplicating finding test #19 (cleartext password after TLS downgrade)

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (2)

src/sql_jsc/postgres/PostgresSQLConnection.rs (1)

2730-2749: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Tighten the pre-decode salt cap to the actual 1 KiB limit.

2048 still lets decode_alloc reserve about 1536 decoded bytes before the later > 1024 check rejects it, so the pre-allocation guard is still bypassable. If the decoded ceiling is 1024 bytes, the encoded ceiling here should be the corresponding base64 maximum (1368), not a generous round-up.

🔧 Suggested fix

-                        // Bound the *encoded* length before allocating: a
-                        // malicious/MITM'd server can otherwise force an
-                        // arbitrarily large decode_alloc here. 1024 decoded
-                        // bytes ≈ 1368 base64 chars; round up generously.
+                        // Bound the *encoded* length before allocating: a
+                        // malicious/MITM'd server can otherwise force a
+                        // larger-than-allowed decode_alloc here.
+                        const MAX_SCRAM_SALT_LEN: usize = 1024;
+                        const MAX_SCRAM_SALT_B64_LEN: usize =
+                            ((MAX_SCRAM_SALT_LEN + 2) / 3) * 4;
                         let server_salt_b64 = cont.s.slice();
-                        if server_salt_b64.is_empty() || server_salt_b64.len() > 2048 {
+                        if server_salt_b64.is_empty()
+                            || server_salt_b64.len() > MAX_SCRAM_SALT_B64_LEN
+                        {
                             debug!(
                                 "SASLContinue encoded salt length out of range: {}",
                                 server_salt_b64.len()
                             );
                             return Err(AnyPostgresError::InvalidMessage);
                         }
@@
-                        if server_salt_decoded_base64.is_empty()
-                            || server_salt_decoded_base64.len() > 1024
+                        if server_salt_decoded_base64.is_empty()
+                            || server_salt_decoded_base64.len() > MAX_SCRAM_SALT_LEN
                         {

🤖 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/sql_jsc/postgres/PostgresSQLConnection.rs` around lines 2730 - 2749, The
pre-decode bound for the SASL server salt is too loose (uses 2048) allowing
decode_alloc to over-allocate; tighten the check in the SASLContinue handling by
replacing the server_salt_b64 length cap (the if that inspects
server_salt_b64.is_empty() || server_salt_b64.len() > 2048) with the correct
base64-encoded ceiling for 1024 decoded bytes (use 1368), so the check around
cont.s.slice()/server_salt_b64 prevents decode_alloc from reserving >1024 bytes
before bun_base64::decode_alloc and keeps the subsequent
server_salt_decoded_base64.len() > 1024 check meaningful.

src/http_jsc/websocket_client.rs (1)

1322-1344: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clamp close reasons to 123 bytes, not 125.

The 125-byte WebSocket control-frame limit includes the 2-byte status code, so the reason budget is only 123 bytes. The current min(125) / > 125 checks still allow invalid close frames with 2 + reason_len > 125.

🔧 Suggested fix

     fn send_close_with_body(&mut self, code: u16, body: Option<&mut [u8; 125]>, body_len: usize) {
-        let body_len = body_len.min(125);
+        let body_len = body_len.min(123);
         log!("Sending close with code {}", code);
@@
-                if wrote_len > 125 {
+                if wrote_len > 123 {
                     break 'inner;
                 }

Also applies to: 1668-1674

🤖 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/http_jsc/websocket_client.rs` around lines 1322 - 1344, The
send_close_with_body function incorrectly allows a 125-byte reason (plus 2-byte
status) which violates the 125-byte control-frame limit; change the clamping of
the reason budget from body_len.min(125) to body_len.min(123) (and any other
similar clamps) so that body_len + 2 <= 125, and ensure the header length
calculation still uses (body_len + 2) truncated to 7 bits when calling
header.set_len; update all analogous occurrences (e.g., the second close-frame
emitter around the later block) to use the same 123-byte cap to prevent
constructing invalid close frames.

🤖 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/http/HTTPContext.rs`:
- Around line 926-934: The pending-H2 enqueue path can coalesce strict callers
onto a lax PendingConnect because PendingConnect doesn't record
reject_unauthorized; update PendingConnect to store the initiating client's
flags.reject_unauthorized at creation (when constructing PendingConnect around
line ~1062), then add the same strictness guard when enqueuing onto
pending_h2_connects (check pc.matches(hostname, port, cfg_nn) &&
(!client.flags.reject_unauthorized ||
pc.established_with_reject_unauthorized_or_flag) — i.e. compare
client.flags.reject_unauthorized against the new PendingConnect field) so strict
callers never join a pending connect started with reject_unauthorized=false.

In `@src/install/bin.rs`:
- Around line 783-789: The current ensure_umask uses separate load/store
allowing multiple threads to run the umask(0)/umask(prev) sequence concurrently;
make it race-safe by performing an atomic compare-and-swap on HAS_SET_UMASK so
only the thread that successfully swaps false->true runs
sys::umask(0)/sys::umask(prev) and writes UMASK (use compare_exchange with
appropriate Acquire/Release orderings), and have other threads spin/wait (e.g.,
loop with an Acquire load) until HAS_SET_UMASK is true and UMASK is published so
they read the correct cached value; reference functions/vars: ensure_umask,
HAS_SET_UMASK, UMASK, and sys::umask.
- Around line 1650-1655: The new guard that checks unscoped_package_name.len()
against self.abs_dest_buf.len().saturating_sub(dest_off) and sets self.err =
Some(bun_core::err!("NameTooLong")) then returns is added for Tag::File but
missing for Tag::NamedFile and Tag::Map; add the exact same capacity check and
early return before any writes/copies into self.abs_dest_buf in the branches
handling Tag::NamedFile and Tag::Map (use the same variables:
unscoped_package_name, self.abs_dest_buf, dest_off, and set self.err to
bun_core::err!("NameTooLong") then return) so long bin names cannot panic during
unlink.

In `@src/install/windows-shim/bun_shim_impl.rs`:
- Around line 1149-1157: The shebang-related metadata parity and buffer-bounds
checks must be moved earlier and strengthened: in the metadata validation block
(around where shebang_arg_len_u8 and shebang_bin_path_len_bytes are parsed) add
parity checks for UTF-16 bytes (reject if (shebang_arg_len_u8 & 1) != 0 or
(shebang_bin_path_len_bytes & 1) != 0 and return LauncherMode::fail(...,
FailReason::InvalidShimBounds)); relocate the existing BUF2 capacity check that
compares sizes against BUF2_U16_LEN * 2 so it runs before any writes/copies
(i.e., before the operations at/around where the first copy occurs near the
previous line 1125); and change the raw store through write_ptr so it uses
write_ptr.write_unaligned(0) instead of unsafe deref assignment to avoid UB on
misaligned pointers—keep the same failure path (LauncherMode::fail with
FailReason::InvalidShimBounds) when checks fail.

In `@src/runtime/webcore/fetch.rs`:
- Around line 1747-1750: The unconditional opened_fd.close() is fragile; wrap
opened_fd in a scoped RAII guard (e.g., use scopeguard::guard or the project's
equivalent) immediately after the fd is created/duplicated so that the guard
calls opened_fd.close() in its drop handler; locate the code around opened_fd
and read_file (where opened_fd is passed as an Fd to read_file) and replace the
manual close() call with a scopeguard::guard that owns a closure calling
opened_fd.close(), ensuring the fd is always closed even on early returns or
panics.

---

Duplicate comments:
In `@src/http_jsc/websocket_client.rs`:
- Around line 1322-1344: The send_close_with_body function incorrectly allows a
125-byte reason (plus 2-byte status) which violates the 125-byte control-frame
limit; change the clamping of the reason budget from body_len.min(125) to
body_len.min(123) (and any other similar clamps) so that body_len + 2 <= 125,
and ensure the header length calculation still uses (body_len + 2) truncated to
7 bits when calling header.set_len; update all analogous occurrences (e.g., the
second close-frame emitter around the later block) to use the same 123-byte cap
to prevent constructing invalid close frames.

In `@src/sql_jsc/postgres/PostgresSQLConnection.rs`:
- Around line 2730-2749: The pre-decode bound for the SASL server salt is too
loose (uses 2048) allowing decode_alloc to over-allocate; tighten the check in
the SASLContinue handling by replacing the server_salt_b64 length cap (the if
that inspects server_salt_b64.is_empty() || server_salt_b64.len() > 2048) with
the correct base64-encoded ceiling for 1024 decoded bytes (use 1368), so the
check around cont.s.slice()/server_salt_b64 prevents decode_alloc from reserving
>1024 bytes before bun_base64::decode_alloc and keeps the subsequent
server_salt_decoded_base64.len() > 1024 check meaningful.

🪄 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: 5f8af78b-bd2f-4032-95a5-d9f5a1052388

📥 Commits

Reviewing files that changed from the base of the PR and between d5e15eb and 1c78d7d.

📒 Files selected for processing (29)

• src/dotenv/env_loader.rs
• src/http/HTTPContext.rs
• src/http/h2_client/ClientSession.rs
• src/http/lib.rs
• src/http_jsc/websocket_client.rs
• src/install/bin.rs
• src/install/dependency.rs
• src/install/lockfile/Package.rs
• src/install/npm.rs
• src/install/windows-shim/bun_shim_impl.rs
• src/libarchive/lib.rs
• src/parsers/toml.rs
• src/parsers/yaml.rs
• src/runtime/api/BunObject.rs
• src/runtime/cli/bunx_command.rs
• src/runtime/crypto/PBKDF2.rs
• src/runtime/node/node_crypto_binding.rs
• src/runtime/server/RequestContext.rs
• src/runtime/server/server_body.rs
• src/runtime/shell/states/Expansion.rs
• src/runtime/socket/Listener.rs
• src/runtime/socket/udp_socket.rs
• src/runtime/webcore/Blob.rs
• src/runtime/webcore/Crypto.rs
• src/runtime/webcore/fetch.rs
• src/s3_signing/credentials.rs
• src/shell_parser/parse.rs
• src/sql_jsc/mysql/MySQLConnection.rs
• src/sql_jsc/postgres/PostgresSQLConnection.rs

[coderabbitai]

Actionable comments posted: 3

🤖 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/runtime/cli/bunx_command.rs`:
- Around line 926-970: The trust check that uses bun_sys::lstat/stat to validate
ownership (the lstat_ok/stat_ok logic that sets trustworthy, flips
do_cache_bust, and breaks 'try_run_existing) is only applied to the initial
probe; apply the same validation to the alternate resolution/execution path (the
codepath that uses dest_or_cache2 / the later resolution of
`${bunx_cache_dir}/node_modules/.bin/<real-bin>`) so any resolved cached binary
is rejected unless owned by the current uid. Refactor the lstat/stat logic into
a small helper (e.g., is_trustworthy(destination, uid) or
validate_cached_binary) that encapsulates the existing lstat_ok/stat_ok checks
and symlink-follow behavior, then call that helper before allowing execution in
both the initial check and the dest_or_cache2 path and ensure you set
do_cache_bust / bail out the same way when it returns false.

In `@src/runtime/socket/socket_body.rs`:
- Around line 1506-1551: The guard that clears Flags::IS_ACTIVE for the captured
handlers can clear the shared IS_ACTIVE bit for a newly opened generation;
change the cleanup to only clear IS_ACTIVE when the current this_ref.handlers
still points to the captured_handlers (i.e., require
this_ref.handlers.get().map(|n| n.as_ptr()) == Some(h) before calling
update_flags to remove IS_ACTIVE); otherwise, do not touch this_ref.flags (but
still call Handlers::mark_inactive(h) for the captured h if the VM is not
shutting down) so the new generation's IS_ACTIVE bit and mark_active/on_open
logic remain correct.

🪄 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: 30d1197c-dd9b-4e20-9006-bbc3115bfef0

📥 Commits

Reviewing files that changed from the base of the PR and between 1c78d7d and 74bc9af.

📒 Files selected for processing (9)

• src/http/HTTPContext.rs
• src/http/h2_client/PendingConnect.rs
• src/http_jsc/websocket_client.rs
• src/install/bin.rs
• src/install/windows-shim/bun_shim_impl.rs
• src/runtime/cli/bunx_command.rs
• src/runtime/socket/socket_body.rs
• src/runtime/webcore/fetch.rs
• src/shell_parser/parse.rs

[claude]

🟡 nit: These three release_socket call sites pass self.flags.reject_unauthorized (the current request's flag) as established_with_reject_unauthorized, so a strict-established socket that gets reused by a lax client and re-pooled is downgraded to established_with_reject_unauthorized=false — a subsequent strict client is then needlessly blocked at HTTPContext.rs:805 from reusing a socket whose hostname was validated. Contrast ClientSession.rs:903, which correctly carries self.established_with_reject_unauthorized (set once at create). This errs conservative (extra TLS handshake, not a security hole), so it's just a keepalive-efficiency nit; the proper fix is to thread the original handshake's flag through ExistingSocket → HTTPClient and pass it back here instead of re-deriving from the current request.

Extended reasoning...

What the bug is

Fix #83 adds an established_with_reject_unauthorized: bool field to PooledSocket so that existing_socket() can refuse to hand a strict (rejectUnauthorized: true) caller a socket whose TLS handshake never ran checkServerIdentity. The field is populated from the new release_socket() parameter. ClientSession (h2) does this correctly: it stores established_with_reject_unauthorized once at creation time from the establishing client (ClientSession.rs:241) and passes that stored value back to release_socket (ClientSession.rs:903). But the three HTTP/1.1 release_socket() call sites in lib.rs (lines 2318, 3595, 3817) instead pass self.flags.reject_unauthorized — the current request's flag, not the original handshake's strictness.

The code path that triggers it

For a fresh connection, the current request is the establishing request, so self.flags.reject_unauthorized happens to be correct. The divergence appears on the second pool round-trip:

1. Strict client A establishes a TLS socket. on_handshake runs check_server_identity. A finishes and calls release_socket(..., established_with_reject_unauthorized = A.flags.reject_unauthorized = true, ...). Pooled with the flag = true. ✓
2. Lax client B (rejectUnauthorized: false) calls existing_socket(). The guard at HTTPContext.rs:797–808 only blocks strict→lax-socket, not lax→strict-socket, so B is handed the socket via ExistingSocket { socket, tunnel, h2_session }.
3. ExistingSocket (HTTPContext.rs:271–278) carries only { socket, tunnel, h2_session } — it does not carry established_with_reject_unauthorized out of the pool. The provenance is dropped on the floor.
4. B finishes and calls release_socket(..., established_with_reject_unauthorized = B.flags.reject_unauthorized = **false**, ...). The PooledSocket is now marked as if it were lax-established.
5. Strict client C calls existing_socket(). The guard at HTTPContext.rs:805 sees reject_unauthorized && !socket.established_with_reject_unauthorized → continue. C opens a fresh TLS connection even though the pooled socket's hostname was validated in step 1.

Why existing code doesn't prevent it

There is no field on HTTPClient that records the strictness of the handshake that produced the socket it's currently using; flags.reject_unauthorized is the request's policy, not the connection's provenance. And ExistingSocket doesn't carry the flag through the pool→client→pool round-trip, so once a lax client touches the socket the original strictness is unrecoverable. Contrast ClientSession, which is a per-connection object and stores the flag once at create().

Step-by-step proof

Consider a sequence of three fetch() calls to https://api.example.com over HTTP/1.1 keepalive:

1. fetch(url, { tls: { rejectUnauthorized: true } }) — fresh connect, checkServerIdentity runs, request completes, release_socket at lib.rs:3595 pools it with established_with_reject_unauthorized = self.flags.reject_unauthorized = true.
2. fetch(url, { tls: { rejectUnauthorized: false } }) — existing_socket() finds the pooled entry; reject_unauthorized=false so the guard at line 805 is skipped; socket reused. Request completes, release_socket at lib.rs:3595 pools it with established_with_reject_unauthorized = self.flags.reject_unauthorized = **false**.
3. fetch(url, { tls: { rejectUnauthorized: true } }) — existing_socket() finds the pooled entry; reject_unauthorized=true && !socket.established_with_reject_unauthorized → continue. The socket is left in the pool and a fresh TLS handshake is performed, even though the pooled socket's certificate was fully validated in step 1.

Impact

This errs on the safe side — it never lets a strict caller reuse an actually-unverified socket, so it is not a security hole. It is a keepalive-efficiency regression introduced by fix #83: in workloads that interleave strict and lax requests to the same origin (e.g. a default-strict app that occasionally probes with rejectUnauthorized: false), a strictly-validated socket is permanently "downgraded" after any lax reuse and can never serve a strict request again, costing one extra TLS handshake per such interleaving. This is a narrow scenario and the cost is bounded, hence nit severity — not a blocker for this PR.

How to fix

Thread the provenance through the reuse cycle the same way ClientSession does:

• Add established_with_reject_unauthorized: bool to ExistingSocket and populate it from the PooledSocket in existing_socket() before pending_sockets.put().
• Carry it on HTTPClient (or in state) when an existing socket is adopted; for a fresh connect, set it from self.flags.reject_unauthorized at handshake time (matching what ClientSession::create does at line 241).
• Pass that stored value, not self.flags.reject_unauthorized, at the three lib.rs release_socket call sites.

This is more invasive than the rest of fix #83 and is fine to defer to a follow-up.

[Jarred-Sumner]

Agreed on the analysis — this can downgrade a strictly-handshaked pooled socket after a lax client touches it, costing a redundant TLS handshake on the next strict reuse. It never lets a strict caller reuse an unverified socket, so it stays conservative.

Threading the original handshake's flag through ExistingSocket → HTTPClient and back to release_socket (mirroring ClientSession's established_with_reject_unauthorized field) is the right shape, but it touches the pool/client/release surface across three files. Deferring to a follow-up so this PR stays focused on the security-relevant pieces.

[dariogriffo]

The file is full of magic numbers +2 +5 +7.
Can be replaced with meaningful constants?