Input validation and bounds-checking hardening#30681
Conversation
Tightens validation across the runtime, package manager, SQL drivers, HTTP/2 server, shell, bundler, and JSC bindings.
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis PR implements security hardening across HTTP/2 protocol handling, SQL injection prevention, filesystem path traversal protection, command execution restrictions, output injection prevention, object serialization safety, parser recursion limits, and cryptographic buffer management to prevent protocol violations, injection attacks, resource exhaustion, and unsafe memory access patterns. ChangesSecurity Hardening
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/api/bun/h2_frame_parser.rs (1)
4056-4068:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't close a remotely-ended stream when the last CONTINUATION arrives.
Here
end_after_headersreflects the peer'sEND_STREAMon the originating HEADERS. Whenhandle_headers_frame()had to wait for CONTINUATION, it already moved the stream toHALF_CLOSED_REMOTE; converting that state toCLOSEDhere tears the stream down before the local side can send a response, and it can double-fireonStreamEnd.Suggested fix
- if stream.state == StreamState::HALF_CLOSED_REMOTE { + if stream.state == StreamState::HALF_CLOSED_LOCAL { // no more continuation headers we can call it closed stream.state = StreamState::CLOSED; stream.free_resources::<false>(self); } else { - stream.state = StreamState::HALF_CLOSED_LOCAL; + stream.state = StreamState::HALF_CLOSED_REMOTE; }🤖 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/api/bun/h2_frame_parser.rs` around lines 4056 - 4068, The code currently treats a CONTINUATION with END_HEADERS as if it should transition a stream in StreamState::HALF_CLOSED_REMOTE to StreamState::CLOSED and call free_resources::<false>(self); instead, stop tearing the stream down: when frame.flags has END_HEADERS and stream.end_after_headers is true, do not convert StreamState::HALF_CLOSED_REMOTE to CLOSED or call free_resources; leave the state as HALF_CLOSED_REMOTE so the local side can still send a response and avoid double-firing onStreamEnd (i.e., adjust the branch handling around stream.state == StreamState::HALF_CLOSED_REMOTE in the END_HEADERS block inside the CONTINUATION handling to skip closing/freeing and only set state = HALF_CLOSED_LOCAL for other states).src/runtime/cli/pm_view_command.rs (1)
425-579:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis still leaves a raw output bypass in the property lookup path.
These call sites sanitize the formatted summary, but Line 325 still prints registry-derived
EStringvalues with rawBStr. That meansbun pm view <pkg> descriptioncan still emit terminal control sequences unchanged. Please route the non-JSON string-property branch throughSanitizedtoo.🤖 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/cli/pm_view_command.rs` around lines 425 - 579, The property-lookup branch still prints registry-derived EString/BStr values without sanitization; find the prettyln! call sites that directly interpolate values from prop.key.as_string(&bump), prop.value.as_string(&bump), tagname_expr/val_expr, and any nm/em or published_time variables and wrap those raw BStr/EString values with Sanitized(...) (e.g., replace "{...}", Sanitized(tag) etc.) so every printed registry-derived string is passed through Sanitized; update the prettyln! invocations in the dependency/tag/maintainer/published branches (the code using prop.key/.value, tagname_expr, val_expr, nm, em, published_time) accordingly.
🤖 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/css/css_parser.rs`:
- Around line 3451-3457: The nesting guard internals are currently exposed
publicly; change the visibility of the `nesting_depth` field and the
`MAX_NESTING_DEPTH` constant to internal-only (either `pub(crate)` or private)
so they are not part of the public API; locate the `nesting_depth` field in the
struct definition and change `pub nesting_depth: u32` to `pub(crate)
nesting_depth: u32` (or `nesting_depth: u32`), and change `pub const
MAX_NESTING_DEPTH: u32 = 128;` to `pub(crate) const MAX_NESTING_DEPTH: u32 =
128;` (or private `const`) and run tests/build to ensure no external references
break.
- Around line 671-680: Replace the manual increment/decrement of
parser.nesting_depth with an RAII-based cleanup using scopeguard::defer!: when
entering the block (after checking MAX_NESTING_DEPTH and before calling
parser.parse_entirely) increment parser.nesting_depth and register
scopeguard::defer! to decrement it, so that parser.nesting_depth is always
decremented even if parser.parse_entirely or parsefn returns early; keep the
existing error branch
(Err(parser.new_custom_error(ParserError::maximum_nesting_depth))) unchanged and
still call parser.parse_entirely((), |(), p| parsefn(p)) for the normal path.
In `@src/http_jsc/websocket_client.rs`:
- Around line 313-327: The code currently rejects the connection if
SSL_get_servername() is NULL; instead, obtain the originally requested host (the
host/IP from the WebSocket URL supplied by the CppWebSocket instance or pass
that hostname into this callback) and use that value when calling
boringssl::check_server_identity rather than the negotiated SNI; keep the
existing ssl_ptr null checks and safety comments, build a byte-slice from the
requested host (like the current cstr handling) and call
check_server_identity(unsafe { &mut *ssl_ptr }, requested_host_bytes); only set
self.outgoing_websocket = None and call
ws_ref.did_abrupt_close(ErrorCode::FailedToConnect) if check_server_identity
returns false (not merely when servername is NULL).
In `@src/install/bin.rs`:
- Around line 740-749: bin_target_within_package_dir currently only checks
byte-prefix containment and can be bypassed by symlinks; update the installation
flow to validate the resolved target before performing operations like chmod on
EntryKind::SymLink: either resolve the final target and ensure it stays within
package_dir (canonicalize/realpath and compare) or reject symlinks up-front by
using lstat/open with O_NOFOLLOW and failing if the entry is a symlink, and
ensure the chmod call is only performed after this safe check.
In `@src/js/node/child_process.ts`:
- Around line 135-138: The early throw for options.windowsBatchFileError
bypasses option validation; ensure validateTimeout(options.timeout),
validateAbortSignal(options.signal, "options.signal"), and const killSignal =
sanitizeKillSignal(options.killSignal) are executed before checking or throwing
options.windowsBatchFileError so malformed options are always rejected—i.e.,
call validateTimeout, validateAbortSignal, and sanitizeKillSignal prior to the
windowsBatchFileError guard (leave the options.windowsBatchFileError throw in
place but move it after those validations).
- Around line 523-533: The early return when options.windowsBatchFileError is
set returns the cached async-style error (options.windowsBatchFileError) without
converting it to the sync form; before returning from both early-return sites,
mutate/retag that error exactly as the post-processing block does (the same
transformation used later in the spawnSync post-processing) — e.g., update the
error.message to replace the async prefix "spawn " with "spawnSync ",
preserve/adjust any metadata fields the post-processor sets, and return that
modified error object (apply the same fix in the other early-return branch that
checks options.windowsBatchFileError).
In `@src/jsc/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 1227-1237: The deserialize error paths after calling
sqlite3_deserialize(db, ...) leak the transient DB handle `db`; before each
early return on failure (both when status == SQLITE_BUSY and status !=
SQLITE_OK) call sqlite3_close_v2(db) to close the connection, then throw the
exception as currently done (i.e., insert sqlite3_close_v2(db) immediately prior
to the throwException(...) and return {} in the failure branches around the
existing `status` checks), leaving the success path unchanged.
In `@src/libarchive/lib.rs`:
- Around line 1793-1822: The code increments count before checking
traverses_created_symlink, so entries skipped by the created_symlink guard are
mistakenly tallied; update the logic in the loop around count, created_symlinks,
traverses_created_symlink and the options.log/Output::warn branch so that count
is only incremented after an entry is accepted/handled (move the count += 1 to
after the rejection check and continue, or conditionally increment inside the
path-handling branch) and ensure path_slice and created_symlinks are still used
to determine skipping before incrementing.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 4032-4045: The code currently uses
self.remote_settings.get().unwrap_or(self.local_settings.get()) to validate
inbound CONTINUATION frames, but inbound frames must be validated against our
local max_frame_size; change the comparison to use
self.local_settings.get().max_frame_size (or otherwise obtain local_settings
unconditionally) when checking frame.length, so that if frame.length >
self.local_settings.get().max_frame_size you call send_go_away
(frame.stream_identifier, ErrorCode::FRAME_SIZE_ERROR, b"invalid Continuation
frame size", self.last_stream_id.get(), true) and return Ok(data.len()); update
the logic around settings, remote_settings, local_settings and frame.length
accordingly so only the local max_frame_size governs this check.
In `@src/sql_jsc/mysql/MySQLConnection.rs`:
- Around line 1408-1418: The guard for ResultSetHeader.field_count is missing
for prepared-statement metadata: in handle_prepared_statement() where you
process StmtPrepareOK (the ok struct’s num_params / num_columns) you must apply
the same ceiling (e.g. reuse MAX_RESULT_SET_COLUMNS or a shared constant) before
allocating/resizing the parameter and column vectors; validate ok.num_params and
ok.num_columns against the cap, log and return
Err(AnyMySQLError::UnexpectedPacket) if they exceed it, and only then call
resize/allocate those metadata Vecs to prevent server-controlled large
allocations.
---
Outside diff comments:
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 4056-4068: The code currently treats a CONTINUATION with
END_HEADERS as if it should transition a stream in
StreamState::HALF_CLOSED_REMOTE to StreamState::CLOSED and call
free_resources::<false>(self); instead, stop tearing the stream down: when
frame.flags has END_HEADERS and stream.end_after_headers is true, do not convert
StreamState::HALF_CLOSED_REMOTE to CLOSED or call free_resources; leave the
state as HALF_CLOSED_REMOTE so the local side can still send a response and
avoid double-firing onStreamEnd (i.e., adjust the branch handling around
stream.state == StreamState::HALF_CLOSED_REMOTE in the END_HEADERS block inside
the CONTINUATION handling to skip closing/freeing and only set state =
HALF_CLOSED_LOCAL for other states).
In `@src/runtime/cli/pm_view_command.rs`:
- Around line 425-579: The property-lookup branch still prints registry-derived
EString/BStr values without sanitization; find the prettyln! call sites that
directly interpolate values from prop.key.as_string(&bump),
prop.value.as_string(&bump), tagname_expr/val_expr, and any nm/em or
published_time variables and wrap those raw BStr/EString values with
Sanitized(...) (e.g., replace "{...}", Sanitized(tag) etc.) so every printed
registry-derived string is passed through Sanitized; update the prettyln!
invocations in the dependency/tag/maintainer/published branches (the code using
prop.key/.value, tagname_expr, val_expr, nm, em, published_time) accordingly.
🪄 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: 9d997b40-0fa7-4ba7-a3ea-bfef55547a4d
📒 Files selected for processing (40)
packages/bun-uws/src/HttpParser.hpackages/bun-uws/src/HttpResponse.hsrc/base64/lib.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/options.rssrc/css/css_parser.rssrc/http_jsc/websocket_client.rssrc/install/bin.rssrc/js/bun/sql.tssrc/js/internal/debugger.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/sqlite.tssrc/js/node/child_process.tssrc/jsc/bindings/NodeHTTP.cppsrc/jsc/bindings/PathInlines.hsrc/jsc/bindings/node/crypto/JSCipherConstructor.cppsrc/jsc/bindings/node/http/NodeHTTPParser.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/jsc/bindings/webcore/JSDOMFormData.cppsrc/jsc/bindings/webcore/SerializedScriptValue.cppsrc/libarchive/lib.rssrc/parsers/yaml.rssrc/patch/lib.rssrc/resolver/package_json.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/bake/DevServer/HmrSocket.rssrc/runtime/cli/pm_view_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/node/net/BlockList.rssrc/runtime/server/RequestContext.rssrc/runtime/shell/states/Expansion.rssrc/runtime/webcore/Blob.rssrc/semver/lib.rssrc/shell_parser/parse.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/SASL.rssrc/url/lib.rs
| // Bound recursive descent so deeply nested blocks (e.g. `calc(calc(...))`) | ||
| // can't overflow the native stack; the error path leaves the parser recoverable. | ||
| let result = if parser.nesting_depth >= MAX_NESTING_DEPTH { | ||
| Err(parser.new_custom_error(ParserError::maximum_nesting_depth)) | ||
| } else { | ||
| parser.nesting_depth += 1; | ||
| let r = parser.parse_entirely((), |(), p| parsefn(p)); | ||
| parser.nesting_depth -= 1; | ||
| r | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C1 'scopeguard::defer!' src/css/css_parser.rsRepository: oven-sh/bun
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if scopeguard is imported
echo "=== Checking scopeguard imports ==="
rg -n 'use.*scopeguard|^extern crate scopeguard' src/css/css_parser.rs | head -20
# Get context around the proposed change location
echo -e "\n=== Context around lines 671-680 ==="
sed -n '665,685p' src/css/css_parser.rs
# Get context around the existing defer! usage at line 2165
echo -e "\n=== Context around line 2165 (existing defer! usage) ==="
sed -n '2160,2175p' src/css/css_parser.rsRepository: oven-sh/bun
Length of output: 91
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the file section around 671-680 directly
echo "=== Lines 665-690 of css_parser.rs ==="
head -690 src/css/css_parser.rs | tail -26
# Also check first 50 lines for imports
echo -e "\n=== First 50 lines (imports section) ==="
head -50 src/css/css_parser.rs
# Check if scopeguard is imported via module
echo -e "\n=== Search for scopeguard anywhere in file ==="
rg -n 'scopeguard' src/css/css_parser.rsRepository: oven-sh/bun
Length of output: 3499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search entire file for any scopeguard import or use statement
echo "=== Full file grep for scopeguard context ==="
rg -B5 -A2 'scopeguard' src/css/css_parser.rs
# Check if scopeguard is a dependency
echo -e "\n=== Checking Cargo.toml for scopeguard ==="
find . -name "Cargo.toml" -type f | head -5 | while read f; do
if rg -q 'scopeguard' "$f"; then
echo "Found in: $f"
rg -n 'scopeguard' "$f"
fi
done
# Also check if there's a module that might re-export it
echo -e "\n=== Check imports near line 2165 ==="
sed -n '2150,2170p' src/css/css_parser.rsRepository: oven-sh/bun
Length of output: 2582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the ENTIRE file for any use/import of scopeguard
echo "=== Searching entire file for scopeguard import ==="
rg -n '^use.*scopeguard|^extern.*scopeguard' src/css/css_parser.rs
# If not found, check the prelude or module structure
echo -e "\n=== Checking if scopeguard is in scope via prelude (check all use statements) ==="
rg -n '^use ' src/css/css_parser.rs | head -30
# Get full context around lines 671-680 to understand closure behavior
echo -e "\n=== Full context: lines 660-695 ==="
sed -n '660,695p' src/css/css_parser.rs
# Check if parse_entirely can return early
echo -e "\n=== Search for parse_entirely function signature ==="
rg -n 'fn parse_entirely' src/css/css_parser.rs -A3Repository: oven-sh/bun
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get broader context around line 2165 to see if it's gated
echo "=== Context lines 2140-2180 (checking for cfg gates) ==="
sed -n '2140,2180p' src/css/css_parser.rs
# Also check lines 650-700 for function signature and any gates
echo -e "\n=== Context lines 650-700 (around proposed change location) ==="
sed -n '650,700p' src/css/css_parser.rs
# Check if scopeguard is available via glob imports
echo -e "\n=== Check for pub use or glob imports that might bring scopeguard in ==="
rg -n 'pub use|use .*::\*' src/css/css_parser.rs | head -20Repository: oven-sh/bun
Length of output: 5476
Use RAII (scopeguard::defer!) for nesting_depth cleanup to match the pattern already used in this file.
The manual decrement-after-parse pattern is error-prone; the same issue with composes_refs cleanup is solved at line 2165 using scopeguard::defer!. This ensures nesting_depth is decremented on all exit paths (including early returns from parser.parse_entirely) and aligns with the coding guideline to prefer Drop-based cleanup.
Suggested change
let result = if parser.nesting_depth >= MAX_NESTING_DEPTH {
Err(parser.new_custom_error(ParserError::maximum_nesting_depth))
} else {
parser.nesting_depth += 1;
+ scopeguard::defer! {
+ parser.nesting_depth -= 1;
+ }
- let r = parser.parse_entirely((), |(), p| parsefn(p));
- parser.nesting_depth -= 1;
- r
+ parser.parse_entirely((), |(), p| parsefn(p))
};🤖 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/css/css_parser.rs` around lines 671 - 680, Replace the manual
increment/decrement of parser.nesting_depth with an RAII-based cleanup using
scopeguard::defer!: when entering the block (after checking MAX_NESTING_DEPTH
and before calling parser.parse_entirely) increment parser.nesting_depth and
register scopeguard::defer! to decrement it, so that parser.nesting_depth is
always decremented even if parser.parse_entirely or parsefn returns early; keep
the existing error branch
(Err(parser.new_custom_error(ParserError::maximum_nesting_depth))) unchanged and
still call parser.parse_entirely((), |(), p| parsefn(p)) for the normal path.
| /// Current nested-block depth; bounded by [`MAX_NESTING_DEPTH`] to prevent | ||
| /// stack overflow from deeply nested input. | ||
| pub nesting_depth: u32, | ||
| } | ||
|
|
||
| /// Hard cap on nested-block depth (matches WebKit's CSS tokenizer limit). | ||
| pub const MAX_NESTING_DEPTH: u32 = 128; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --type rust '\bMAX_NESTING_DEPTH\b|\bnesting_depth\b' -g '!src/css/css_parser.rs'Repository: oven-sh/bun
Length of output: 2313
Narrow the visibility of nesting guard internals to pub(crate) or private.
The nesting_depth field and MAX_NESTING_DEPTH constant are implementation details used only within this module and should not be part of the public API. No external code references them. Restricting their visibility prevents accidental dependence on what should remain an internal hardening mechanism.
🤖 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/css/css_parser.rs` around lines 3451 - 3457, The nesting guard internals
are currently exposed publicly; change the visibility of the `nesting_depth`
field and the `MAX_NESTING_DEPTH` constant to internal-only (either `pub(crate)`
or private) so they are not part of the public API; locate the `nesting_depth`
field in the struct definition and change `pub nesting_depth: u32` to
`pub(crate) nesting_depth: u32` (or `nesting_depth: u32`), and change `pub const
MAX_NESTING_DEPTH: u32 = 128;` to `pub(crate) const MAX_NESTING_DEPTH: u32 =
128;` (or private `const`) and run tests/build to ensure no external references
break.
| // SNI may be null (e.g. IP literal); with no hostname there is nothing | ||
| // to verify the certificate against, so reject rather than skip verification. | ||
| let hostname: &[u8] = if !servername.is_null() { | ||
| // SAFETY: servername is a NUL-terminated C string owned by the SSL session. | ||
| let hostname = unsafe { bun_core::ffi::cstr(servername) }.to_bytes(); | ||
| // SAFETY: ssl_ptr is non-null (connected SSL socket on the handshake path). | ||
| if !ssl_ptr.is_null() | ||
| && !boringssl::check_server_identity(unsafe { &mut *ssl_ptr }, hostname) | ||
| { | ||
| self.outgoing_websocket = None; | ||
| ws_ref.did_abrupt_close(ErrorCode::FailedToConnect); | ||
| } | ||
| unsafe { bun_core::ffi::cstr(servername) }.to_bytes() | ||
| } else { | ||
| b"" | ||
| }; | ||
| if ssl_ptr.is_null() | ||
| || hostname.is_empty() | ||
| // SAFETY: ssl_ptr is non-null (checked above) for the open socket's lifetime. | ||
| || !boringssl::check_server_identity(unsafe { &mut *ssl_ptr }, hostname) | ||
| { | ||
| self.outgoing_websocket = None; | ||
| ws_ref.did_abrupt_close(ErrorCode::FailedToConnect); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In BoringSSL/OpenSSL, does SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name) return NULL when the client connects to an IP literal or otherwise does not send SNI, and should certificate identity verification use the original requested host/IP (including IP SANs) rather than the negotiated SNI value?
💡 Result:
Yes, in both BoringSSL and OpenSSL, SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name) returns NULL when the client connects to an IP literal or otherwise does not send an SNI extension [1][2][3][4][5][6]. In BoringSSL source code, if no SNI extension is present in ClientHello, ssl->s3->hostname remains nullptr, and SSL_get_servername returns it [7][6]. OpenSSL documentation explicitly states it returns NULL if no servername was requested by the client [2][3][4][5]. When connecting to an IP literal, clients typically do not send SNI (as SNI must be a hostname per RFC 6066, not IP [8]), so NULL is returned [1][8]. Yes, certificate identity verification should use the original requested host/IP (including checking IP SANs) rather than the (possibly absent or different) negotiated SNI value [9][10][11]. OpenSSL's built-in verification via X509_check_host or SSL_CTX_set_verify uses the application-provided reference identity from functions like SSL_add1_host or X509_VERIFY_PARAM_set1_host, which is set to the originally requested endpoint (host or IP string) [9][10]. This checks against the peer certificate's CN/SAN DNS names (for hostnames) or IP SANs (for IPs) [9]. It does not use the SNI value, which is a separate server-side extension for virtual hosting and unavailable without client SNI [9][11]. Custom verification callbacks receive the original connection details independently of SNI [2].
Citations:
- 1: What exactly does "Can't use SSL_get_servername" mean? openssl/openssl#21523
- 2: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_tlsext_servername_callback.html
- 3: https://manpages.opensuse.org/Leap-15.6/openssl-1_1-doc/SSL_get_servername.3ssl.en.html
- 4: https://openssl.org/docs/man1.0.2/man3/SSL_get_servername_type.html
- 5: https://man.openbsd.org/OpenBSD-6.9/SSL_get_servername.3
- 6: https://github.com/google/boringssl/blob/master/ssl/handshake_server.cc
- 7: https://boringssl.googlesource.com/boringssl/+/main/ssl/extensions.cc
- 8:
SSL_set_tlsext_host_namedoes not validate its parameter to be a host name; causes standards non-compliance openssl/openssl#20041 - 9: https://www.openssl.org/docs/man1.1.1/man3/X509_check_host.html
- 10: Questionable
SSL_add1_host()semantics with IP address literals. openssl/openssl#28418 - 11: https://stackoverflow.com/questions/35604640/why-does-validation-fail-when-connecting-to-a-server-by-ip-address-instead-of-ho
🏁 Script executed:
# First, let's find the exact location and full context of the handle_handshake method
cd src/http_jsc && grep -n "fn handle_handshake" websocket_client.rsRepository: oven-sh/bun
Length of output: 88
🏁 Script executed:
# Let's examine the WebSocket struct definition and fields to see if it stores the original URL/host
rg "struct WebSocket" src/http_jsc/ -A 20Repository: oven-sh/bun
Length of output: 5485
🏁 Script executed:
# Check the full handle_handshake method signature and available context
sed -n '280,330p' src/http_jsc/websocket_client.rsRepository: oven-sh/bun
Length of output: 2642
🏁 Script executed:
# Check the Socket<SSL> definition to see if it stores the original host
rg "pub struct Socket" src/http_jsc/ -A 10Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for where handle_handshake is called and what context is available
rg "handle_handshake" src/http_jsc/ -B 3 -A 1Repository: oven-sh/bun
Length of output: 1763
🏁 Script executed:
# Check WebSocket initialization to see if original URL/host is stored
sed -n '1,100p' src/http_jsc/websocket_client.rs | head -60Repository: oven-sh/bun
Length of output: 2318
🏁 Script executed:
# Check if CppWebSocket stores the original host/URL
rg "impl CppWebSocket" src/http_jsc/ -A 30 | head -60Repository: oven-sh/bun
Length of output: 5254
🏁 Script executed:
# Look for WebSocket construction and initialization
rg "fn new\|fn init\|pub fn create" src/http_jsc/websocket_client.rs -A 5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for where the WebSocket URL/host originates from
rg "class WebSocket\|new WebSocket" src/ -t cpp -t h --max-count 20Repository: oven-sh/bun
Length of output: 37
Verify against the originally requested host, not the negotiated SNI value.
SSL_get_servername() returns NULL for IP-literal connections and clients that don't send SNI. This code incorrectly rejects those connections outright, breaking valid wss:// connections that rely on IP SANs in the certificate. Certificate identity verification should check against the original requested host/IP (from the WebSocket URL), not the absence of SNI. Query the requested hostname from the C++ CppWebSocket object or thread it into the callback, then verify the certificate against that rather than rejecting when SNI is absent.
🤖 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 313 - 327, The code currently
rejects the connection if SSL_get_servername() is NULL; instead, obtain the
originally requested host (the host/IP from the WebSocket URL supplied by the
CppWebSocket instance or pass that hostname into this callback) and use that
value when calling boringssl::check_server_identity rather than the negotiated
SNI; keep the existing ssl_ptr null checks and safety comments, build a
byte-slice from the requested host (like the current cstr handling) and call
check_server_identity(unsafe { &mut *ssl_ptr }, requested_host_bytes); only set
self.outgoing_websocket = None and call
ws_ref.did_abrupt_close(ErrorCode::FailedToConnect) if check_server_identity
returns false (not merely when servername is NULL).
| /// True when `abs_target` is contained in `package_dir`. `bin` targets are | ||
| /// untrusted package.json values; a `../../..` target would be symlinked + chmod 0o777. | ||
| fn bin_target_within_package_dir(abs_target: &[u8], package_dir: &[u8]) -> bool { | ||
| debug_assert!(matches!(package_dir.last(), Some(&b'/') | Some(&b'\\'))); | ||
| let dir = strings::without_trailing_slash(package_dir); | ||
| if !strings::has_prefix(abs_target, dir) { | ||
| return false; | ||
| } | ||
| matches!(abs_target.get(dir.len()), None | Some(&b'/') | Some(&b'\\')) | ||
| } |
There was a problem hiding this comment.
Lexical containment still allows symlink escapes.
bin_target_within_package_dir() only checks the joined path bytes. A package can still place a symlink inside the package that resolves outside the package root, and the later link path explicitly accepts EntryKind::SymLink and then calls chmod(abs_target, ... | 0o777) on POSIX. That preserves an external-target write/permission escalation path even though ../../ traversal is blocked. Resolve the final target (or lstat/open with O_NOFOLLOW and reject symlinks) before linking/chmodding.
🤖 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/install/bin.rs` around lines 740 - 749, bin_target_within_package_dir
currently only checks byte-prefix containment and can be bypassed by symlinks;
update the installation flow to validate the resolved target before performing
operations like chmod on EntryKind::SymLink: either resolve the final target and
ensure it stays within package_dir (canonicalize/realpath and compare) or reject
symlinks up-front by using lstat/open with O_NOFOLLOW and failing if the entry
is a symlink, and ensure the chmod call is only performed after this safe check.
| if (options.windowsBatchFileError) throw options.windowsBatchFileError; | ||
| validateTimeout(options.timeout); | ||
| validateAbortSignal(options.signal, "options.signal"); | ||
| const killSignal = sanitizeKillSignal(options.killSignal); |
There was a problem hiding this comment.
Preserve option validation before the batch-file guard.
This early throw now skips validateTimeout(), validateAbortSignal(), and sanitizeKillSignal() for the Windows .bat/.cmd path, so malformed options stop being rejected if the command hits this guard first.
Suggested fix
function spawn(file, args, options) {
options = normalizeSpawnArguments(file, args, options);
- if (options.windowsBatchFileError) throw options.windowsBatchFileError;
validateTimeout(options.timeout);
validateAbortSignal(options.signal, "options.signal");
const killSignal = sanitizeKillSignal(options.killSignal);
+ if (options.windowsBatchFileError) throw options.windowsBatchFileError;
const child = new ChildProcess();🤖 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/node/child_process.ts` around lines 135 - 138, The early throw for
options.windowsBatchFileError bypasses option validation; ensure
validateTimeout(options.timeout), validateAbortSignal(options.signal,
"options.signal"), and const killSignal = sanitizeKillSignal(options.killSignal)
are executed before checking or throwing options.windowsBatchFileError so
malformed options are always rejected—i.e., call validateTimeout,
validateAbortSignal, and sanitizeKillSignal prior to the windowsBatchFileError
guard (leave the options.windowsBatchFileError throw in place but move it after
those validations).
| count += 1; | ||
|
|
||
| // Reject entries whose path passes through (or equals) a | ||
| // symlink created earlier in this extraction (chained-symlink escape). | ||
| #[cfg(unix)] | ||
| { | ||
| // Case-insensitive on macOS (APFS default is case-insensitive). | ||
| #[cfg(target_os = "macos")] | ||
| fn component_eq(a: &[u8], b: &[u8]) -> bool { | ||
| a.eq_ignore_ascii_case(b) | ||
| } | ||
| #[cfg(all(unix, not(target_os = "macos")))] | ||
| fn component_eq(a: &[u8], b: &[u8]) -> bool { | ||
| a == b | ||
| } | ||
| let traverses_created_symlink = created_symlinks.iter().any(|prefix| { | ||
| path_slice.len() >= prefix.len() | ||
| && component_eq(&path_slice[..prefix.len()], &prefix[..]) | ||
| && (path_slice.len() == prefix.len() | ||
| || path_slice[prefix.len()] == b'/') | ||
| }); | ||
| if traverses_created_symlink { | ||
| if options.log { | ||
| Output::warn(&format_args!( | ||
| "Skipping entry that traverses a previously created symlink: {}\n", | ||
| bun_core::fmt::fmt_os_path(path_slice, Default::default()), | ||
| )); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Don't count entries that the new symlink guard skips.
count += 1 now runs before this continue, so blocked entries are still reported as extracted. Move the increment below the rejection branch, or increment only after the entry is actually handled.
Suggested fix
- count += 1;
-
// Reject entries whose path passes through (or equals) a
// symlink created earlier in this extraction (chained-symlink escape).
#[cfg(unix)]
{
// Case-insensitive on macOS (APFS default is case-insensitive).
@@
if traverses_created_symlink {
if options.log {
Output::warn(&format_args!(
"Skipping entry that traverses a previously created symlink: {}\n",
bun_core::fmt::fmt_os_path(path_slice, Default::default()),
));
}
continue;
}
}
+
+ count += 1;📝 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.
| count += 1; | |
| // Reject entries whose path passes through (or equals) a | |
| // symlink created earlier in this extraction (chained-symlink escape). | |
| #[cfg(unix)] | |
| { | |
| // Case-insensitive on macOS (APFS default is case-insensitive). | |
| #[cfg(target_os = "macos")] | |
| fn component_eq(a: &[u8], b: &[u8]) -> bool { | |
| a.eq_ignore_ascii_case(b) | |
| } | |
| #[cfg(all(unix, not(target_os = "macos")))] | |
| fn component_eq(a: &[u8], b: &[u8]) -> bool { | |
| a == b | |
| } | |
| let traverses_created_symlink = created_symlinks.iter().any(|prefix| { | |
| path_slice.len() >= prefix.len() | |
| && component_eq(&path_slice[..prefix.len()], &prefix[..]) | |
| && (path_slice.len() == prefix.len() | |
| || path_slice[prefix.len()] == b'/') | |
| }); | |
| if traverses_created_symlink { | |
| if options.log { | |
| Output::warn(&format_args!( | |
| "Skipping entry that traverses a previously created symlink: {}\n", | |
| bun_core::fmt::fmt_os_path(path_slice, Default::default()), | |
| )); | |
| } | |
| continue; | |
| } | |
| // Reject entries whose path passes through (or equals) a | |
| // symlink created earlier in this extraction (chained-symlink escape). | |
| #[cfg(unix)] | |
| { | |
| // Case-insensitive on macOS (APFS default is case-insensitive). | |
| #[cfg(target_os = "macos")] | |
| fn component_eq(a: &[u8], b: &[u8]) -> bool { | |
| a.eq_ignore_ascii_case(b) | |
| } | |
| #[cfg(all(unix, not(target_os = "macos")))] | |
| fn component_eq(a: &[u8], b: &[u8]) -> bool { | |
| a == b | |
| } | |
| let traverses_created_symlink = created_symlinks.iter().any(|prefix| { | |
| path_slice.len() >= prefix.len() | |
| && component_eq(&path_slice[..prefix.len()], &prefix[..]) | |
| && (path_slice.len() == prefix.len() | |
| || path_slice[prefix.len()] == b'/') | |
| }); | |
| if traverses_created_symlink { | |
| if options.log { | |
| Output::warn(&format_args!( | |
| "Skipping entry that traverses a previously created symlink: {}\n", | |
| bun_core::fmt::fmt_os_path(path_slice, Default::default()), | |
| )); | |
| } | |
| continue; | |
| } | |
| } | |
| count += 1; |
🤖 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/libarchive/lib.rs` around lines 1793 - 1822, The code increments count
before checking traverses_created_symlink, so entries skipped by the
created_symlink guard are mistakenly tallied; update the logic in the loop
around count, created_symlinks, traverses_created_symlink and the
options.log/Output::warn branch so that count is only incremented after an entry
is accepted/handled (move the count += 1 to after the rejection check and
continue, or conditionally increment inside the path-handling branch) and ensure
path_slice and created_symlinks are still used to determine skipping before
incrementing.
| let settings = self | ||
| .remote_settings | ||
| .get() | ||
| .unwrap_or(self.local_settings.get()); | ||
| if frame.length > settings.max_frame_size { | ||
| self.send_go_away( | ||
| frame.stream_identifier, | ||
| ErrorCode::FRAME_SIZE_ERROR, | ||
| b"invalid Continuation frame size", | ||
| self.last_stream_id.get(), | ||
| true, | ||
| ); | ||
| return Ok(data.len()); | ||
| } |
There was a problem hiding this comment.
Validate received CONTINUATION frames against local_settings.max_frame_size.
remote_settings.max_frame_size is the peer's advertised outbound limit for frames we send, not the limit the peer must obey when sending to us. If the peer advertises a larger value than we did, this hardening accepts oversized inbound CONTINUATION frames instead of tripping FRAME_SIZE_ERROR.
Suggested fix
- let settings = self
- .remote_settings
- .get()
- .unwrap_or(self.local_settings.get());
- if frame.length > settings.max_frame_size {
+ let max_frame_size = self.local_settings.get().max_frame_size;
+ if frame.length > max_frame_size {
self.send_go_away(
frame.stream_identifier,
ErrorCode::FRAME_SIZE_ERROR,📝 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.
| let settings = self | |
| .remote_settings | |
| .get() | |
| .unwrap_or(self.local_settings.get()); | |
| if frame.length > settings.max_frame_size { | |
| self.send_go_away( | |
| frame.stream_identifier, | |
| ErrorCode::FRAME_SIZE_ERROR, | |
| b"invalid Continuation frame size", | |
| self.last_stream_id.get(), | |
| true, | |
| ); | |
| return Ok(data.len()); | |
| } | |
| let max_frame_size = self.local_settings.get().max_frame_size; | |
| if frame.length > max_frame_size { | |
| self.send_go_away( | |
| frame.stream_identifier, | |
| ErrorCode::FRAME_SIZE_ERROR, | |
| b"invalid Continuation frame size", | |
| self.last_stream_id.get(), | |
| true, | |
| ); | |
| return Ok(data.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/runtime/api/bun/h2_frame_parser.rs` around lines 4032 - 4045, The code
currently uses self.remote_settings.get().unwrap_or(self.local_settings.get())
to validate inbound CONTINUATION frames, but inbound frames must be validated
against our local max_frame_size; change the comparison to use
self.local_settings.get().max_frame_size (or otherwise obtain local_settings
unconditionally) when checking frame.length, so that if frame.length >
self.local_settings.get().max_frame_size you call send_go_away
(frame.stream_identifier, ErrorCode::FRAME_SIZE_ERROR, b"invalid Continuation
frame size", self.last_stream_id.get(), true) and return Ok(data.len()); update
the logic around settings, remote_settings, local_settings and frame.length
accordingly so only the local max_frame_size governs this check.
| /// Like [`Self::handle_received_stream_id`], but bounds new-stream allocation | ||
| /// from inbound frames so a peer can't force unbounded `Stream` creation (RFC 7540 §5.1.2). | ||
| fn handle_remote_stream_id(&self, stream_identifier: u32) -> Option<*mut Stream> { | ||
| // Default SETTINGS_MAX_CONCURRENT_STREAMS is "unlimited"; apply a cap when | ||
| // the user hasn't explicitly configured `maxConcurrentStreams`. | ||
| const MAX_REMOTE_STREAMS_DEFAULT_CAP: u32 = 1000; | ||
| if stream_identifier != 0 && !self.streams.get().contains_key(&stream_identifier) { | ||
| let configured = self.local_settings.get().max_concurrent_streams; | ||
| let max = if configured == u32::MAX { | ||
| MAX_REMOTE_STREAMS_DEFAULT_CAP | ||
| } else { | ||
| configured | ||
| }; | ||
| // Count open streams; CLOSED ones stay in the map for pointer stability. | ||
| let mut open: u32 = 0; | ||
| for (_, stream) in self.streams.get().iter() { | ||
| // SAFETY: boxed Stream outlives the iteration (entries are never removed). | ||
| if unsafe { (**stream).state } != StreamState::CLOSED { | ||
| open += 1; | ||
| if open >= max { | ||
| return None; | ||
| } | ||
| } | ||
| } | ||
| if max == 0 { | ||
| return None; | ||
| } | ||
| } | ||
| self.handle_received_stream_id(stream_identifier) | ||
| } |
There was a problem hiding this comment.
Use a distinct outcome for "stream limit exceeded".
Returning None here makes callers treat an over-limit inbound stream exactly like stream 0 / "no stream". For HEADERS that falls into the "Headers frame on connection stream" path and sends GOAWAY PROTOCOL_ERROR, instead of rejecting just the offending stream. Please surface limit exhaustion explicitly so dispatch can take a stream-level refusal path.
| stream.state = StreamState::HALF_CLOSED_LOCAL; | ||
| this.dispatch(JSH2FrameParser::Gc::onWantTrailers, stream.get_identifier()); | ||
| return Ok(JSValue::js_number(stream_id as f64)); | ||
| } | ||
| // RFC 7540 §5.1: both sides END_STREAM → CLOSED, not HALF_CLOSED_LOCAL | ||
| // (mirrors `flush_queue_after_data_frame`). | ||
| if stream.state == StreamState::HALF_CLOSED_REMOTE { | ||
| stream.state = StreamState::CLOSED; | ||
| } else { | ||
| stream.state = StreamState::HALF_CLOSED_LOCAL; | ||
| } |
There was a problem hiding this comment.
This post-HEADERS state transition no longer matches the wire state.
In the wait_for_trailers branch, the client path has not sent END_STREAM yet, while the server path already set END_STREAM on the HEADERS frame earlier in this method, so forcing HALF_CLOSED_LOCAL and emitting onWantTrailers is inconsistent either way. In the non-trailer branch, if this moves the stream to CLOSED, it also needs free_resources::<false>(this) like send_data() does, otherwise header-only responses retain their stream context/signal state after completion.
Suggested fix
if end_stream {
stream.end_after_headers = true;
if wait_for_trailers {
- stream.state = StreamState::HALF_CLOSED_LOCAL;
- this.dispatch(JSH2FrameParser::Gc::onWantTrailers, stream.get_identifier());
+ if !this.is_server.get() {
+ this.dispatch(JSH2FrameParser::Gc::onWantTrailers, stream.get_identifier());
+ }
return Ok(JSValue::js_number(stream_id as f64));
}
// RFC 7540 §5.1: both sides END_STREAM → CLOSED, not HALF_CLOSED_LOCAL
// (mirrors `flush_queue_after_data_frame`).
if stream.state == StreamState::HALF_CLOSED_REMOTE {
stream.state = StreamState::CLOSED;
+ stream.free_resources::<false>(this);
} else {
stream.state = StreamState::HALF_CLOSED_LOCAL;
}| // `field_count` is a server-controlled length-encoded integer; cap | ||
| // it before it drives an allocation. MySQL caps tables at 4096 cols. | ||
| const MAX_RESULT_SET_COLUMNS: u64 = 0xFFFF; | ||
| if header.field_count > MAX_RESULT_SET_COLUMNS { | ||
| bun_core::scoped_log!( | ||
| MySQLConnection, | ||
| "result set header field count {} exceeds cap {}", | ||
| header.field_count, | ||
| MAX_RESULT_SET_COLUMNS | ||
| ); | ||
| return Err(AnyMySQLError::UnexpectedPacket); |
There was a problem hiding this comment.
Cap StmtPrepareOK metadata counts too.
This guard only covers ResultSetHeader.field_count. handle_prepared_statement() still trusts server-controlled ok.num_params / ok.num_columns and allocates both vectors directly at Lines 1189-1206, so a malicious server can still force large allocations through COM_STMT_PREPARE. Reuse the same ceiling there before resizing statement metadata.
🤖 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/mysql/MySQLConnection.rs` around lines 1408 - 1418, The guard for
ResultSetHeader.field_count is missing for prepared-statement metadata: in
handle_prepared_statement() where you process StmtPrepareOK (the ok struct’s
num_params / num_columns) you must apply the same ceiling (e.g. reuse
MAX_RESULT_SET_COLUMNS or a shared constant) before allocating/resizing the
parameter and column vectors; validate ok.num_params and ok.num_columns against
the cap, log and return Err(AnyMySQLError::UnexpectedPacket) if they exceed it,
and only then call resize/allocate those metadata Vecs to prevent
server-controlled large allocations.
|
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 40-file security hardening pass that changes behavior in TLS verification, child_process, SQL drivers, HTTP/2, archive extraction, and more — it warrants a human review for the user-facing behavioral tightening alone.
Extended reasoning...
Overview
This PR is a broad defensive-hardening sweep across ~40 files spanning the HTTP parser (uWS), WebSocket client TLS verification, SQL drivers (Postgres/MySQL/SQLite injection guards, SCRAM iteration cap), child_process (CVE-2024-27980 .bat/.cmd refusal), crypto cipher construction, structured-clone deserialization, libarchive symlink-escape protection, package-install bin path traversal, the bundler/linker, the CSS parser, the shell expander, the HTTP/2 frame parser, and assorted bounds checks in semver/url/base64/yaml.
Security risks
Almost every change here is the security surface: TLS hostname verification semantics (now rejects when no SNI hostname is available), SQL string interpolation allowlists, archive extraction path containment, pointer validation for structured-clone deserialize, sqlite file_control buffer sizing, header-injection escaping in Content-Disposition and multipart/form-data, terminal-escape stripping of untrusted output, and the BatBadBut mitigation. These are exactly the categories where a subtle off-by-one or over-tight regex breaks real users or, worse, leaves a gap.
Level of scrutiny
High. The changes are individually small but each one alters acceptance behavior on a hot, externally-reachable path. Several introduce new rejection cases that could regress legitimate workloads (e.g. Postgres array-type regex, transaction-options allowlist, WebSocket TLS-without-SNI now hard-fails, Windows .bat/.cmd spawn now EINVAL, HTTP/2 concurrent-stream cap). The HTTP/2 state-machine edits and the shell brace-escaping rework in particular are non-mechanical logic changes.
Other factors
No bugs were flagged by the automated hunt and the diff reads as carefully reasoned, but the sheer breadth, the security-critical nature of every touched path, and the user-visible behavioral tightening make this inappropriate for bot approval. A maintainer should sign off on the policy decisions (rejection thresholds, regex shapes, default caps) embedded here.
2 participants
What does this PR do?
Defensive hardening pass across the runtime — tightens input validation and adds bounds checks in a number of code paths. No new features or API surface.
How did you verify your code works?
bun bdbuilds cleancargo checkpasses