Hardening: input validation and bounds tightening across 31 subsystems (round 3)#31221
Merged
Conversation
Collaborator
|
Updated 1:10 PM PT - May 23rd, 2026
❌ @Jarred-Sumner, your commit f9bb063 has some failures in 🧪 To try this PR locally: bunx bun-pr 31221That installs a local version of the PR into your bun-31221 --bun |
Contributor
|
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 applies broad safety and validation hardening: socket pooling and redirect TLS handling, HTTP/H2 header and framing checks, parser early-stop on handler closure, buffer pinning for async/native paths, lockfile and install path validation, SQL/type bounds checks, JSC/FFI receiver and deserialization guards, and numerous parser/bounds fixes. ChangesHardened Security & Safety Across Sockets, Buffers, Lockfiles, Protocols, and Input Validation
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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_types/URLPath.rs`:
- Around line 61-68: The method take_decoded_storage() on URLPath relinquishes
the Box<[u8]> but leaves slice fields (path, pathname, extname, etc.) pointing
into that buffer, creating a dangling-slice hazard; mark the function
#[must_use] to warn callers if they ignore the returned storage and, immediately
after calling self._decoded_storage.take(), clear/invalidate the slice fields on
self (set them to empty slices/None as appropriate) so the URLPath cannot be
used while ownership of the backing storage has moved; refer to the
take_decoded_storage method, the _decoded_storage field, and the slice fields
(path, pathname, extname) when implementing this change.
In `@src/http/lib.rs`:
- Around line 4443-4445: The code currently converts proxy CONNECT failures into
UnexpectedRedirect in the FetchRedirect::Error handling, causing callers to lose
the proxy response/body; update the FetchRedirect::Error branch (the logic that
produces UnexpectedRedirect) to first check is_proxy_connect_failure and, if
true, preserve and return the proxy's response/status/body (or an error variant
that carries the proxy response) instead of mapping it to
UnexpectedRedirect—ensure references to is_proxy_connect_failure,
self.redirect_type/FetchRedirect::Follow, and the
FetchRedirect::Error/UnexpectedRedirect conversion are guarded so proxy CONNECT
failures are surfaced as-is.
- Around line 4254-4267: The header parsing arm that checks
hash_header_const(b"Content-Length") currently treats any invalid Content-Length
as an unrecoverable framing error (returning err!(InvalidHTTPResponse)), which
breaks CONNECT tunnel handling; change the logic so that when parsing response
headers for a successful 200 CONNECT (i.e. the code path that leads to proxy
tunneling / tunnel mode) you ignore framing headers like Content-Length and
Transfer-Encoding instead of validating them. Locate the header match arm that
uses hash_header_const(b"Content-Length") and
bun_core::parse_unsigned::<usize>() and add a conditional to skip the
parse-and-error behavior when the response is for a CONNECT tunnel (or when the
proxy_tunneling/tunnel-mode flag is set), leaving existing validation intact for
normal responses.
In `@src/install/lockfile/bun.lock.rs`:
- Around line 70-78: The serialization path in save_from_binary_inner is using a
raw prefix check to collapse npm registry URLs to "" which mismatches the
stricter registry-boundary logic in url_is_under_registry used by
parse_into_binary_lockfile, causing off-registry hosts like
"registry.npmjs.org.evil" to be mis-collapsed; update save_from_binary_inner to
call or reuse url_is_under_registry (the same boundary check) when deciding to
collapse a URL to the registry shorthand so only true under-registry URLs are
collapsed and npm_url_needs_integrity semantics are preserved.
In `@src/install/lockfile/Tree.rs`:
- Around line 303-317: The current folder_name_is_safe in Tree.rs is weaker than
crate::dependency::is_safe_install_folder_name (it allows empty names/segments
and misses colon checks) causing inconsistent validation between
relative_path_and_depth and process_subtree; fix by replacing the custom checks
in folder_name_is_safe to delegate to the canonical validator
(crate::dependency::is_safe_install_folder_name) so all callers (including
relative_path_and_depth) use the same stricter logic and avoid empty
segment/empty name/drive-colon edge cases.
In `@src/install/migration.rs`:
- Around line 608-620: Extract the duplicated skip logic (checking
ExprData::EBoolean for keys b"inBundle" or b"extraneous" on a pkg map) into a
small helper like should_skip_pkg(pkg: &ExprMap) -> bool (or is_skipped_pkg) and
replace the inline predicate in the counting, building, and linking passes with
a call to that helper (update the sites that currently inspect
pkg.get(b"inBundle")/... and pkg.get(b"extraneous")/... to call the new function
instead). Ensure the helper returns true when either key is present and true,
preserves the current unwrap_or(false) semantics, and is used consistently by
the three passes to prevent drift.
In `@src/runtime/node/win_watcher.rs`:
- Around line 509-512: Before storing the new PathWatcherManager returned by
PathWatcherManager::init into DEFAULT_MANAGER, detect the existing manager (the
pointer in DEFAULT_MANAGER or the `existing` variable) and retire it: if the old
manager is empty call its immediate teardown method (e.g., deinit/destroy)
otherwise mark it for deferred deinit (set deinit_on_last_watcher or call a
mark_for_deferred_deinit method) so unregister_watcher can finish cleanup later;
then store the new manager. Ensure you reference DEFAULT_MANAGER,
PathWatcherManager::init, unregister_watcher, and deinit_on_last_watcher in your
change so the previous manager is not leaked when replacing the global slot.
In `@src/runtime/node/zlib/NativeBrotli.rs`:
- Around line 198-210: The current code in NativeBrotli (around the handling of
arguments.ptr[1]) uses .unwrap() on arguments.ptr[1].as_array_buffer and then
accepts any view before treating it as a Uint32Array, which can panic; change
this to mirror the NativeZstd guard: validate that arguments.ptr[1] is present,
call as_array_buffer and if that returns None return a thrown Error
(INVALID_ARG_VALUE) instead of unwrapping, then check that the buffer view is a
Uint32Array (use the same type-check used in NativeZstd) and that its length >=
2 before calling as_u32/as_mut_ptr; on any failure return a descriptive Error
via global_this.err().throw() so bad JS input returns an error rather than
aborting (refer to flush_write_result, write_result,
arguments.ptr[1].as_array_buffer, and write_result_slice.as_u32).
In `@src/runtime/node/zlib/NativeZlib.rs`:
- Around line 147-159: The code currently unwraps the caller-provided buffer
when reading arguments.ptr[4] via as_array_buffer and then assumes as_u32
succeeds, which can panic; change this to return a JS error instead of panicking
by checking and handling both failures: validate that
arguments.ptr[4].as_array_buffer(global) returns Some and that the resulting
buffer can be viewed as a Uint32Array (as_u32) before using it; if either step
fails, return a bun_jsc::ErrorCode::INVALID_ARG_VALUE (or an appropriate JS
error) with a clear message about writeResult needing to be a Uint32Array of at
least 2 elements, and only then call as_mut_ptr() and proceed to
flush_write_result.
In `@src/runtime/server/RequestContext.rs`:
- Around line 3576-3582: The limit check for streaming bodies is missing bytes
already drained from request_body_buf when streaming starts: seed
request_body_streamed_len with the length of the drained buffer inside
on_start_streaming_request_body() (use request_body_buf.len() or the exact
drained byte count) so that request_body_streamed_len reflects buffered +
subsequent chunk bytes before comparing against
server.config().max_request_body_size; update the code paths that transition
from buffered→streaming to initialize request_body_streamed_len accordingly.
In `@src/url/lib.rs`:
- Around line 1066-1068: The pre-scan currently counts all parameters and bytes
before the capped second pass, allowing large allocations; update the pre-scan
loops that compute `count` and `estimated_str_len` so they stop incrementing
once `count` reaches `MAX_QUERY_STRING_PARAMS` (and similarly cap
`estimated_str_len` to a safe upper bound) and use those capped values when
calling `list.reserve(count)` / `Vec::with_capacity(estimated_str_len)`;
specifically modify the pre-scan logic that feeds `list.reserve` and
`Vec::with_capacity` (referencing `list.reserve`, `Vec::with_capacity`, and
`MAX_QUERY_STRING_PARAMS`) to bound both the parameter count and the estimated
allocation to the defined hard caps.
🪄 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: 49ce076c-32f3-4595-9e61-57bf39c42339
📒 Files selected for processing (57)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-uws/src/HttpParser.hpackages/bun-uws/src/PerMessageDeflate.hsrc/base64/lib.rssrc/bun_core/string/HashedString.rssrc/glob/matcher.rssrc/http/HTTPContext.rssrc/http/h2_client/dispatch.rssrc/http/lib.rssrc/http_types/URLPath.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/TarballStream.rssrc/install/dependency.rssrc/install/lockfile/Tree.rssrc/install/lockfile/bun.lock.rssrc/install/lockfile/bun.lockb.rssrc/install/migration.rssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/sqlite.tssrc/jsc/bindings/BunString.cppsrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cppsrc/jsc/bindings/node/crypto/JSCipherConstructor.cppsrc/jsc/bindings/node/crypto/JSVerify.cppsrc/jsc/bindings/node/http/NodeHTTPParser.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/jsc/bindings/webcore/SerializedScriptValue.cppsrc/resolver/package_json.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/filesystem_router.rssrc/runtime/dns_jsc/dns.rssrc/runtime/image/Image.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/types.rssrc/runtime/node/win_watcher.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/runtime/server/RequestContext.rssrc/runtime/shell/builtin/rm.rssrc/runtime/shell/builtin/seq.rssrc/runtime/socket/uws_jsc.rssrc/runtime/timer/timer_object_internals.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/ByteStream.rssrc/runtime/webcore/FormData.rssrc/shell_parser/parse.rssrc/spawn/static_pipe_writer.rssrc/sql/mysql/protocol/NewWriter.rssrc/sql/mysql/protocol/PacketHeader.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLValue.rssrc/sql_jsc/mysql/protocol/ResultSet.rssrc/sql_jsc/postgres/DataCell.rssrc/url/lib.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/cli/install/bun-lock.test.ts`:
- Around line 698-705: Reorder the assertions so the subprocess exit code is
checked before reading filesystem artifacts: after collecting stdout/stderr (the
[out, err] Promise.all call) keep the stderr/stdout checks and
offRegistryRequests assertion, then assert expect(await exited).toBe(0)
immediately, and only after that call file(join(packageDir, "node_modules",
"no-deps", "package.json")).json() and .toMatchObject; this prevents ENOENT from
masking process failures by ensuring exited is successful before inspecting
node_modules.
In `@test/cli/install/migration/migrate.test.ts`:
- Around line 213-218: Move the assertion of exitCode to immediately after the
stderr checks to fail fast before any filesystem reads: after computing err from
stderr (variable err / stderr.toString()) and the two expect(err)... assertions,
add expect(exitCode).toBe(0) and only then perform the reads/assertions that
touch node_modules and bun.lock (the Bun.file(...) json read and
fs.existsSync(...) checks); this ensures the exitCode is validated before calls
to Bun.file, join(testDir,...), and fs.existsSync.
In `@test/cli/install/symlink-path-traversal.test.ts`:
- Around line 231-261: Move the install success check to run immediately after
asserting stdout/stderr (i.e., after the expect(stderr).toContain(...) and
before iterating the filesystem); specifically, check exitCode (using the
exitCode variable and the existing guard that logs stdout/stderr) and
expect(exitCode).toBe(0) before calling readdir/installDir traversal, readlink,
or access; this prevents readdir/readlink/access (and the escapingSymlinks /
pkgDir checks) from throwing ENOENT and masking the real failure.
In `@test/js/bun/shell/commands/rm.test.ts`:
- Around line 207-223: The test currently launches the delete process into
"running" with $`rm -rf ${target}`.nothrow().quiet().run() but never asserts it
succeeded, allowing silent failures to produce false-positive victim-file
assertions; update the test to await running, capture its result (e.g., the
process/result returned by run()), and add an assertion that the delete command
exited successfully (check result.exitCode === 0 or result.ok depending on the
run() API) before asserting victimFile/victimDir existence so any rm failure
fails the test; reference the running variable and the $`rm -rf
${target}`.nothrow().quiet().run() invocation.
In `@test/js/bun/util/filesystem_router.test.ts`:
- Line 694: This test file contains a per-test explicit timeout expressed as the
trailing "}, 30_000);" on a test invocation; remove the explicit timeout
argument so the test ends with "});" instead and rely on the suite/global
timeouts (do not add any other per-test timeout in test/js/bun/** except the
documented exceptions like ffi/cc.test.ts and glob/leak.test.ts).
In `@test/js/node/fs/fs.test.ts`:
- Line 4063: Replace the test's repetitive string creation for the "data.bin"
entry: instead of using "a".repeat(65536), allocate a buffer and convert to
string (e.g. Buffer.alloc(65536, 'a').toString()) where the "data.bin" value is
set so tests follow the repo guideline for repetitive content.
In `@test/js/node/http/node-http.test.ts`:
- Around line 1864-1866: The test currently asserts exitCode without surfacing
stderr on failure; after the Promise.all that assigns stdout, stderr, exitCode
(from proc.stdout.text(), proc.stderr.text(), proc.exited) insert a guard: if
(exitCode !== 0) { expect(stderr).toBe(""); } immediately before the existing
expect(exitCode).toBe(0) so failing subprocesses print stderr; apply the same
change to the other occurrence that also reads stdout/stderr/exitCode.
In `@test/js/node/http2/node-http2-continuation.test.ts`:
- Line 468: Replace the repetitive-string generation for the "x-filler" header
payload to use Buffer.alloc for performance in debug builds: locate the header
assignment that uses "A".repeat(valueLength) (the "x-filler" key and the
valueLength variable) and change it to use Buffer.alloc(valueLength,
'A').toString() so the test follows the Buffer.alloc(...).toString() convention.
In `@test/js/node/vm/vm.test.ts`:
- Around line 891-903: The test currently assumes calling proto[name] or the
"identifier" getter will fail with a TypeError, but if the property is missing
the .call invocation itself throws and masks the real issue; update the loop and
getter check to first verify the native members exist and are callable: for the
list of method names ensure typeof proto[name] === "function" before invoking
proto[name].call(fake) (and record a clear failure/result if it's missing or not
callable), and for "identifier" use Object.getOwnPropertyDescriptor(proto,
"identifier") to confirm a .get exists before invoking descriptor.get.call(fake)
(again record missing getter if absent) so TypeError is only treated as success
when a valid member was present and the receiver check actually threw.
In `@test/js/node/zlib/zlib.test.js`:
- Around line 639-687: The test may throw before the final deflate.close(),
leaking the native handle; ensure the Deflate instance is always closed by
creating deflate before a try block and moving the existing test logic into try
{ ... } and calling deflate.close() in a finally block (use the existing deflate
variable and handle references so the finally can always call deflate.close()
even if an assertion or await promise throws).
In `@test/js/sql/sql-mysql-raw-length-prefix.test.ts`:
- Line 305: The test builds a long repeated string using "x".repeat(...) for the
bio251 variable; replace that with Buffer.alloc(...) per the test-performance
rule: keep the "\x05admin" prefix and construct the repeated 'x' sequence with
Buffer.alloc(251 - 6, 'x').toString() so bio251 equals "\x05admin" +
Buffer.alloc(251 - 6, 'x').toString(); update the initialization of bio251
accordingly.
In `@test/js/sql/sql-mysql.test.ts`:
- Around line 1086-1176: Extract the entire test case named "MySQL TLS handshake
rejects plaintext packets buffered behind the server greeting" (including helper
functions u16le/u24le/u32le/packet, constants, forgedOk, server setup and the
try/finally block) out of test/js/sql/sql-mysql.test.ts and paste it into a new
file matching the non-Docker pattern (e.g. test/js/sql/sql-mysql-mock.test.ts);
remove the original block from sql-mysql.test.ts so Docker-gated suite no longer
contains mock-socket tests; ensure the new file imports the same
helpers/fixtures (net, mock, using, SQL, expect) that the test needs and runs
under the existing test runner configuration.
In `@test/js/sql/sql.test.ts`:
- Around line 12566-12572: Add an assertion that the spawned fixture's stderr is
empty (after filtering known ASAN startup noise) before checking exitCode so
stderr-only regressions fail the test; specifically, capture proc.stderr.text()
into stderr (already present), apply the same ASAN-noise filtering used in other
bun tests, assert the filtered stderr is empty (or matches the expected
filtered-stderr helper) and place that assertion before the
expect(exitCode).toBe(0) check so any unexpected child diagnostics are caught.
In `@test/js/web/fetch/blob.test.ts`:
- Line 356: The test builds a long repetitive string using "A".repeat(64) when
assigning to the expected variable; replace that with creating the repeated A
sequence via Buffer.alloc and converting to string (i.e., use
Buffer.alloc(count, fill).toString()) and then append "tail" so the assignment
to expected uses Buffer.alloc(64, 'A').toString() + "tail" instead of repeat();
update the same assignment in blob.test.ts where expected is declared.
🪄 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: 0092c02d-507d-4330-961c-f1b25b9f5b75
📒 Files selected for processing (38)
test/bundler/esbuild/packagejson.test.tstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-lock.test.tstest/cli/install/bun-lockb.test.tstest/cli/install/migration/migrate.test.tstest/cli/install/symlink-path-traversal.test.tstest/js/bun/crypto/cipheriv-decipheriv.test.tstest/js/bun/http/bun-server.test.tstest/js/bun/http/proxy.test.tstest/js/bun/http/serve-pending-promise-abort-leak.test.tstest/js/bun/http/serve.test.tstest/js/bun/jsc/bun-jsc.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/commands/rm.test.tstest/js/bun/shell/commands/seq.test.tstest/js/bun/sqlite/sqlite.test.jstest/js/bun/util/filesystem_router.test.tstest/js/node/crypto/sign-jwk-ieee-p1363.test.tstest/js/node/fs/fs.test.tstest/js/node/http/node-http-parser.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2-continuation.test.tstest/js/node/http2/node-http2.test.jstest/js/node/module/sourcemap.test.jstest/js/node/vm/vm.test.tstest/js/node/watch/fs.watch.test.tstest/js/node/zlib/zlib.test.jstest/js/sql/sql-mysql-raw-length-prefix.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.tstest/js/web/fetch/blob.test.tstest/js/web/fetch/fetch-keepalive.test.tstest/js/web/fetch/fetch.test.tstest/js/web/fetch/fetch.tls.test.tstest/js/web/html/FormData.test.tstest/js/web/timers/setTimeout.test.jstest/js/web/websocket/websocket-permessage-deflate.test.ts
Jarred-Sumner
commented
May 22, 2026
Contributor
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/runtime/node/zlib/NativeZlib.rs (1)
149-183:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate
dictionarybefore unwrappingas_array_buffer
InNativeZlib::init(src/runtime/node/zlib/NativeZlib.rs:182),arguments.ptr[6]is only checked forundefined; if it’s any non-ArrayBuffervalue,arguments.ptr[6].as_array_buffer(global).unwrap()will panic instead of throwing a JS type error. Replace the unwrap with aNone→throw_invalid_argument_type_value("dictionary", "ArrayBuffer", arguments.ptr[6])path.🤖 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/node/zlib/NativeZlib.rs` around lines 149 - 183, The code in NativeZlib::init currently only checks if arguments.ptr[6] is undefined then unconditionally calls as_array_buffer(global).unwrap(), which can panic for non-ArrayBuffer values; change the logic that builds dictionary/dictionary_buf so that if arguments.ptr[6] is undefined you set dictionary = None, otherwise call arguments.ptr[6].as_array_buffer(global) and if that returns None return Err(global.throw_invalid_argument_type_value("dictionary", "ArrayBuffer", arguments.ptr[6])) instead of unwrap, and only on success bind dictionary_buf and set dictionary = Some(dictionary_buf.byte_slice()); keep references named dictionary_buf and dictionary so the borrow lifetime remains valid for the subsequent stream.init call.
♻️ Duplicate comments (1)
src/http/lib.rs (1)
4254-4267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore
Transfer-Encodingfor successful CONNECT too.This only fixes half of the tunnel-framing case. A
200 CONNECTresponse must ignoreTransfer-Encodingas well asContent-Length; otherwise the laterTransfer-Encodingbranch can still reject or mutate a successful tunnel setup.Suggested fix
+ let ignore_connect_framing = + self.flags.proxy_tunneling && self.proxy_tunnel.is_none() && response.status_code == 200; for (header_i, header) in response.headers.list.iter().enumerate() { match hash_header_name(header.name()) { h if h == hash_header_const(b"Content-Length") => { - if self.flags.proxy_tunneling - && self.proxy_tunnel.is_none() - && response.status_code == 200 - { + if ignore_connect_framing { continue; } let Ok(content_length) = bun_core::parse_unsigned::<usize>(header.value(), 10) else { return Err(err!(InvalidHTTPResponse)); @@ } h if h == hash_header_const(b"Transfer-Encoding") => { + if ignore_connect_framing { + continue; + } if header.value() == b"gzip" {🤖 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/lib.rs` around lines 4254 - 4267, The CONNECT success case currently ignores only Content-Length; extend the same logic to also ignore Transfer-Encoding for 200 CONNECT responses so the tunnel framing code can't be rejected or mutated later. In the header match handling (see the arm using hash_header_const(b"Content-Length")), apply the same conditional check (self.flags.proxy_tunneling && self.proxy_tunnel.is_none() && response.status_code == 200) to the Transfer-Encoding header branch (the branch that matches hash_header_const(b"Transfer-Encoding") or the later Transfer-Encoding handling) so that Transfer-Encoding is likewise skipped for successful CONNECT responses.
🤖 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/node/zlib/NativeBrotli.rs`:
- Around line 200-215: The init path in NativeBrotli is failing to validate
arguments.ptr[0] ("params") before calling as_u32(), which can panic or
misinterpret non-Uint32Array inputs; update NativeBrotli::init to mirror the
writeResult checks: call arguments.ptr[0].as_array_buffer(global_this) and
return Err(global_this.throw_invalid_argument_type_value("params",
"Uint32Array", params_value)) if it fails, then verify
params_buf.typed_array_type == bun_jsc::JSType::Uint32Array and return the same
throw_invalid_argument_type_value on mismatch, and only then call
params_buf.as_u32() to obtain the params slice.
---
Outside diff comments:
In `@src/runtime/node/zlib/NativeZlib.rs`:
- Around line 149-183: The code in NativeZlib::init currently only checks if
arguments.ptr[6] is undefined then unconditionally calls
as_array_buffer(global).unwrap(), which can panic for non-ArrayBuffer values;
change the logic that builds dictionary/dictionary_buf so that if
arguments.ptr[6] is undefined you set dictionary = None, otherwise call
arguments.ptr[6].as_array_buffer(global) and if that returns None return
Err(global.throw_invalid_argument_type_value("dictionary", "ArrayBuffer",
arguments.ptr[6])) instead of unwrap, and only on success bind dictionary_buf
and set dictionary = Some(dictionary_buf.byte_slice()); keep references named
dictionary_buf and dictionary so the borrow lifetime remains valid for the
subsequent stream.init call.
---
Duplicate comments:
In `@src/http/lib.rs`:
- Around line 4254-4267: The CONNECT success case currently ignores only
Content-Length; extend the same logic to also ignore Transfer-Encoding for 200
CONNECT responses so the tunnel framing code can't be rejected or mutated later.
In the header match handling (see the arm using
hash_header_const(b"Content-Length")), apply the same conditional check
(self.flags.proxy_tunneling && self.proxy_tunnel.is_none() &&
response.status_code == 200) to the Transfer-Encoding header branch (the branch
that matches hash_header_const(b"Transfer-Encoding") or the later
Transfer-Encoding handling) so that Transfer-Encoding is likewise skipped for
successful CONNECT responses.
🪄 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: efc7af89-4443-4af4-ad46-bb93b1483fa3
📒 Files selected for processing (25)
src/http/lib.rssrc/http_types/URLPath.rssrc/install/lockfile/Tree.rssrc/install/lockfile/bun.lock.rssrc/install/migration.rssrc/js/internal/sql/postgres.tssrc/runtime/node/win_watcher.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/server/RequestContext.rssrc/url/lib.rstest/cli/install/bun-lock.test.tstest/cli/install/migration/migrate.test.tstest/cli/install/symlink-path-traversal.test.tstest/js/bun/util/filesystem_router.test.tstest/js/node/fs/fs.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2-continuation.test.tstest/js/node/vm/vm.test.tstest/js/node/zlib/zlib.test.jstest/js/sql/sql-mysql-raw-length-prefix.test.tstest/js/sql/sql-mysql-tls-plaintext-injection.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.tstest/js/web/fetch/blob.test.ts
💤 Files with no reviewable changes (1)
- test/js/sql/sql-mysql.test.ts
Jarred-Sumner
force-pushed
the
claude/hardening-sweep-3
branch
from
54fb966 to
a24abe1
Compare
Jarred-Sumner
force-pushed
the
claude/hardening-sweep-3
branch
from
f7d9488 to
e6aaecb
Compare
Contributor
There was a problem hiding this comment.
No new issues found on this pass — all prior inline findings have been addressed. That said, this is a 109-file security-hardening sweep touching TLS/SNI pooling, crypto, deserialization bounds, install path traversal, and SQL/WASI input validation, with several subtle interactions already caught and fixed during review (pin/unpin balance, h2/h3 redirect hostname clear, throw-scope validator), so it warrants a human pass before merge. Also note Build #57268 still shows a build-rust failure on 🍎 x64.
Extended reasoning...
Overview
This PR is a 56-commit, 109-file input-validation and bounds-tightening sweep across 31 subsystems: TLS socket pooling and Host/SNI override keying (HTTPContext.rs, http/lib.rs), HTTP/H2 parser dispatch and header validation, permessage-deflate payload caps, JSC receiver checks and SerializedScriptValue deserialization bounds, ArrayBuffer pin/unpin for async zlib/fs I/O, install/lockfile folder-name and symlink-target validation, SQL transaction-option and array-type-name validation, MySQL packet framing and TLS handshake-tail checks, node:crypto authTagLength and P1363 conversion, shell rm NOFOLLOW and seq termination, WASI preopen containment, and more — each with new regression tests.
Security risks
The PR is itself the security surface: it changes TLS certificate-verification keying across redirects, crypto signature-encoding fallback, path-traversal guards in the installer, SQL string interpolation validation, structured-clone deserialization bounds, and WASI sandbox containment. Several review rounds already caught regressions in these exact areas (cross-origin redirect mis-pooling under proxy_auth_hash=0, h2/h3 not consuming clear_hostname_on_redirect, leaked ArrayBuffer pins on every async zlib write / fs.read, leaked pins on the position validation error path, throw-scope validator failures on x64-asan). All were fixed in follow-up commits, but the density of subtle cross-cutting interactions is high.
Level of scrutiny
High. This is production-critical, security-sensitive code across the HTTP client, TLS layer, package installer, crypto bindings, and JSC FFI boundary. The changes are individually small and well-commented, but the review history (12+ rounds of inline findings, multiple of which were genuine regressions introduced by earlier commits in the same PR) demonstrates that the interactions are non-obvious. This is well outside the "simple/mechanical" bar for auto-approval.
Other factors
- All 30+ prior inline comments (CodeRabbit and claude) are resolved, and the author confirmed sweeping the remaining stderr-assertion nits.
- The bug-hunting system found no new issues on this final pass.
- One open
DerivedArrayNode-compat narrowing note (types.rs:1433) was flagged as intentional by design and left for the author to confirm. - CI: the latest robobun update for commit 2d6811d still reports 1 failure (
scripts/build/ci.tsbuild-rust on 🍎 x64), which should be green before merge.
Adds JSValue::as_pinned_arraybuffer() and uses the generated cached-value slots on the zlib handle classes to keep in-flight write buffers reachable, replacing the per-handle Strong pair and the separate unpin path. fs.read reuses the same accessor. writev/readv now require a plain dense array.
A re-entrant property getter for a later parameter can run before sqlite3_step(), so a statically-bound string pointer can outlive its backing store. Bind text the same way blobs are now bound and drop the unused clone flag from the rebind chain.
Collect the elements of a writev/readv array before taking any data pointer, using the same MarkedArgumentBuffer pattern as Buffer.concat, and for the async path root each element and pin its backing store until the operation completes. SharedArrayBuffer-backed views are left unpinned everywhere since they can never be detached.
The buffer-span collector declares a throw scope, so its Rust caller has to discharge the simulated throw the way every other throwing binding call does. Release the collected buffers when a later argument fails validation, and unprotect the parsed arguments on the defensive early returns. Assert empty stderr in two subprocess tests that captured it without checking it.
Jarred-Sumner
force-pushed
the
claude/hardening-sweep-3
branch
from
4ee8320 to
749270b
Compare
An interior tab is valid optional whitespace in a field value and cannot start a new header line, so the printable-ASCII guard should not drop content types that contain one. Also keep the renegotiation window from resetting when the wall clock steps backwards, and back the 2GB write regression test with 256 shared chunks so its peak RSS fits the CI runners.
Pinning a write buffer can fail when the backing store cannot be materialized; throw instead of panicking, and do it before any write state is mutated so there is nothing to unwind.
This was referenced May 30, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jun 1, 2026
…y name (#31655) Closes #31652 ## Repro `bun install -g @openai/codex` on `1.4.0-canary` aborts with: ``` error: Invalid dependency name "" error: Invalid dependency name "" error: Invalid dependency name "" ``` On `1.3.x` the same command installs fine. Minimal reduction — a package whose manifest declares an **optional** dependency under an empty key whose target doesn't resolve on the current platform: ```jsonc // manifest for top@1.0.0 { "optionalDependencies": { "": "1.0.0" } } // target "" 404s / is platform-filtered ``` ``` $ bun install # 1.3.14 → exit 0, optional dep skipped $ bun install # 1.4.0-canary → error: Invalid dependency name "", exit 1 ``` ## Cause When a dependency alias (the key) is empty, the real package name is only substituted back into `Dependency.name` **if the dependency resolves** (`assign_resolution` / `assign_root_resolution` in `PackageManagerResolution.rs`). An *optional* dependency that fails to resolve keeps `pkg_id == invalid_package_id` and an **empty** name. That empty-named dependency then reaches the hoisting tree builder, where a name-safety check added in #31221 (`src/install/lockfile/Tree.rs`) treats the empty name as unsafe and calls `maybe_report_error("Invalid dependency name \"\"")`, aborting the install. The original Zig implementation had no such check here, which is why 1.3.x works. The check was also added **inconsistently**: the two sibling call sites already guard the empty case — - `src/install/lockfile/bun.lock.rs`: `if !name_str.is_empty() && !is_safe_install_folder_name(name_str)` - `src/install/isolated_install.rs`: `if !name.is_empty() && !is_safe_install_folder_name(name)` — while `Tree.rs` was the only one missing `!name.is_empty()`. ## Fix Guard `!name.is_empty()` before the safety check in `Tree.rs`, matching the sibling call sites. An empty alias has no `node_modules/<name>` folder to escape, so it is skipped like an unresolved optional dep should be, instead of failing the whole install. Genuinely unsafe *non-empty* names (e.g. `..`, `../..`) still error exactly as before. ## Verification `test/regression/issue/31652.test.ts` serves a package with `optionalDependencies: { "": "1.0.0" }` whose target 404s and asserts the install completes without `Invalid dependency name ""` and with exit 0. - Fails on `main`: `error: Invalid dependency name ""`, exit 1. - Passes with this change. - The existing alias-path-traversal rejection tests (`rejects dependency aliases containing '..' path segments`, `does not create a cache index entry outside the cache directory for a dependency alias of '..'`, and the two sibling `'..'` tests) still pass — the empty-name guard does not weaken the security checks. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 3 of the input-validation and bounds-tightening sweep. 56 commits across 31 subsystems, each commit scoped to one change:
Verified with
cargo check --workspaceon all 10 CI target triples and the per-subsystem test suites for each touched area; no new test failures relative to a baseline debug build.