diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index a661704df181..fa77571bda37 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -21,6 +21,7 @@ #include "libusockets.h" #include #include +#include /* These are in sni_tree.cpp */ void *sni_new(); @@ -799,7 +800,36 @@ static inline int ssl_gone(struct us_socket_t *s) { } static int ssl_renegotiate(struct us_socket_t *s) { + /* Server-forced renegotiation (HelloRequest -> SSL_ERROR_WANT_RENEGOTIATE). + * Enforce the per-context policy (default 3 per 600s, Node's + * CLIENT_RENEG_LIMIT/CLIENT_RENEG_WINDOW) before re-entering a full + * handshake — otherwise a malicious server can pin a core with + * back-to-back renegotiations. limit == 0 disables renegotiation; window + * == 0 means the per-connection counter never resets. Returning 0 makes + * the caller treat this as SSL_ERROR_SSL and close the connection. */ + uint32_t limit, window; + us_reneg_policy(s_ssl(s), &limit, &window); + struct us_ssl_reneg_state_t *st = us_reneg_state(s_ssl(s)); s->ssl_handshake_state = HANDSHAKE_RENEGOTIATION_PENDING; + if (!st) { + ssl_trigger_handshake(s, 0); + return 0; + } + /* Wall-clock time can step backwards (NTP, manual adjustment); the + * unsigned subtraction below would underflow and reset the window every + * time. Only treat the window as elapsed when time has moved forward. */ + uint64_t now_ms = (uint64_t)time(NULL) * 1000; + if (st->count == 0 || + (window && now_ms >= st->window_start_ms && + now_ms - st->window_start_ms >= (uint64_t)window * 1000)) { + st->window_start_ms = now_ms; + st->count = 0; + } + if (st->count >= limit) { + ssl_trigger_handshake(s, 0); + return 0; + } + st->count++; if (!SSL_renegotiate(s_ssl(s))) { ssl_trigger_handshake(s, 0); return 0; diff --git a/packages/bun-uws/src/HttpParser.h b/packages/bun-uws/src/HttpParser.h index dc08aa896efd..bdb118b32299 100644 --- a/packages/bun-uws/src/HttpParser.h +++ b/packages/bun-uws/src/HttpParser.h @@ -955,7 +955,12 @@ namespace uWS /* Go ahead and parse it (todo: better heuristics for emitting FIN to the app level) */ std::string_view dataToConsume(data, length); for (auto chunk : uWS::ChunkIterator(&dataToConsume, &remainingStreamingBytes)) { - dataHandler(user, chunk, chunk.length() == 0); + void *returnedUser = dataHandler(user, chunk, chunk.length() == 0); + if (returnedUser != user) { + /* The data handler closed or shut down the socket; stop parsing + * so we do not dispatch pipelined requests on a dead socket. */ + return HttpParserResult::success(consumedTotal, returnedUser); + } } if (isParsingInvalidChunkedEncoding(remainingStreamingBytes)) [[unlikely]] { // TODO: what happen if we already responded? @@ -969,12 +974,16 @@ namespace uWS } else if (contentLengthStringLen) { if constexpr (!ConsumeMinimally) { unsigned int emittable = (unsigned int) std::min(remainingStreamingBytes, length); - dataHandler(user, std::string_view(data, emittable), emittable == remainingStreamingBytes); + void *returnedUser = dataHandler(user, std::string_view(data, emittable), emittable == remainingStreamingBytes); remainingStreamingBytes -= emittable; data += emittable; length -= emittable; consumedTotal += emittable; + + if (returnedUser != user) { + return HttpParserResult::success(consumedTotal, returnedUser); + } } } else if(isConnectRequest) { // This only serves to mark that the connect request read all headers @@ -986,7 +995,10 @@ namespace uWS break; } else { /* If we came here without a body; emit an empty data chunk to signal no data */ - dataHandler(user, {}, true); + void *returnedUser = dataHandler(user, {}, true); + if (returnedUser != user) { + return HttpParserResult::success(consumedTotal, returnedUser); + } } /* Consume minimally should break as easrly as possible */ @@ -1011,7 +1023,10 @@ namespace uWS /* It's either chunked or with a content-length */ std::string_view dataToConsume(data, length); for (auto chunk : uWS::ChunkIterator(&dataToConsume, &remainingStreamingBytes)) { - dataHandler(user, chunk, chunk.length() == 0); + void *returnedUser = dataHandler(user, chunk, chunk.length() == 0); + if (returnedUser != user) { + return HttpParserResult::success(0, returnedUser); + } } if (isParsingInvalidChunkedEncoding(remainingStreamingBytes)) { return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_INVALID_CHUNKED_ENCODING); @@ -1074,7 +1089,10 @@ namespace uWS /* It's either chunked or with a content-length */ std::string_view dataToConsume(data, length); for (auto chunk : uWS::ChunkIterator(&dataToConsume, &remainingStreamingBytes)) { - dataHandler(user, chunk, chunk.length() == 0); + void *returnedUser = dataHandler(user, chunk, chunk.length() == 0); + if (returnedUser != user) { + return HttpParserResult::success(0, returnedUser); + } } if (isParsingInvalidChunkedEncoding(remainingStreamingBytes)) { return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_INVALID_CHUNKED_ENCODING); diff --git a/packages/bun-uws/src/PerMessageDeflate.h b/packages/bun-uws/src/PerMessageDeflate.h index e4ebaf0ac536..4c717e4b18d2 100644 --- a/packages/bun-uws/src/PerMessageDeflate.h +++ b/packages/bun-uws/src/PerMessageDeflate.h @@ -246,6 +246,9 @@ struct InflationStream { if (res == 0) { /* Fast path wins */ + if (written > maxPayloadLength) { + return std::nullopt; + } return std::string_view(buf, written); } #endif diff --git a/src/base64/lib.rs b/src/base64/lib.rs index dfa9ac5c89f0..d3ba74852f5f 100644 --- a/src/base64/lib.rs +++ b/src/base64/lib.rs @@ -327,7 +327,7 @@ pub mod vlq { let encoded_ = &encoded[start..][0..(encoded.len() - start).min(VLQ_MAX_IN_BYTES + 1)]; // inlining helps for the 1 or 2 byte case, hurts a little for larger - for i in 0..(VLQ_MAX_IN_BYTES + 1) { + for i in 0..encoded_.len() { if ASSERT_VALID { debug_assert!(encoded_[i] < U7_MAX); // invalid base64 character } diff --git a/src/bun_core/string/HashedString.rs b/src/bun_core/string/HashedString.rs index e05d463224d1..47a8f70ec885 100644 --- a/src/bun_core/string/HashedString.rs +++ b/src/bun_core/string/HashedString.rs @@ -43,7 +43,9 @@ impl HashedString { } pub fn eql_bytes(&self, other: &[u8]) -> bool { - (self.len as usize) == other.len() && (hash(other) as u32) == self.hash + (self.len as usize) == other.len() + && (hash(other) as u32) == self.hash + && self.str() == other } pub fn str(&self) -> &[u8] { diff --git a/src/glob/matcher.rs b/src/glob/matcher.rs index fe1e7818473c..f768fa0facfe 100644 --- a/src/glob/matcher.rs +++ b/src/glob/matcher.rs @@ -397,7 +397,9 @@ fn glob_match_impl( let pi = state.path_index as usize; let gi = state.glob_index as usize; let n = cc_len as usize; - pi + n <= path.len() && path[pi..pi + n] == glob[gi..gi + n] + pi + n <= path.len() + && gi + n <= glob.len() + && path[pi..pi + n] == glob[gi..gi + n] } else { path[state.path_index as usize] == cc }; diff --git a/src/http/HTTPContext.rs b/src/http/HTTPContext.rs index 64bd428ce3ac..14bb0d184541 100644 --- a/src/http/HTTPContext.rs +++ b/src/http/HTTPContext.rs @@ -772,10 +772,13 @@ impl HTTPContext { continue; } + // The hash covers the Host-header SNI override that the handshake + // was verified against (see get_tls_hostname / connect()). + if socket.proxy_auth_hash != proxy_auth_hash { + continue; + } + if want_tunnel { - if socket.proxy_auth_hash != proxy_auth_hash { - continue; - } if socket.target_port != target_port { continue; } @@ -970,7 +973,13 @@ impl HTTPContext { } else { 0 }; - let proxy_auth_hash: u64 = if want_tunnel { + // For a direct TLS connection the handshake verifies the peer + // against get_tls_hostname() — which prefers the Host-header + // override (client.hostname) over url.hostname — so the override + // must discriminate the pool key there too, not just for CONNECT + // tunnels. proxy_auth_hash() reduces to exactly the override hash + // (or 0) for a non-proxied request. + let proxy_auth_hash: u64 = if want_tunnel || (SSL && client.http_proxy.is_none()) { client.proxy_auth_hash() } else { 0 diff --git a/src/http/InternalState.rs b/src/http/InternalState.rs index d4e9e7ec2732..924ea1624b06 100644 --- a/src/http/InternalState.rs +++ b/src/http/InternalState.rs @@ -62,6 +62,12 @@ pub struct InternalStateFlags { pub is_redirect_pending: bool, pub is_libdeflate_fast_path_disabled: bool, pub resend_request_body_on_redirect: bool, + /// Cross-origin redirect: the per-request Host override must be dropped so + /// the follow-up connection re-derives SNI/Host from the redirect target. + /// The actual clear is deferred to `do_redirect`, after the old socket's + /// pool/close decision — that decision needs `hostname` still set to know + /// the handshake was verified against an override. + pub clear_hostname_on_redirect: bool, } impl InternalStateFlags { @@ -74,6 +80,7 @@ impl InternalStateFlags { is_redirect_pending: false, is_libdeflate_fast_path_disabled: false, resend_request_body_on_redirect: false, + clear_hostname_on_redirect: false, } } } diff --git a/src/http/h2_client/dispatch.rs b/src/http/h2_client/dispatch.rs index 57d032f3ebce..8cb34124cf17 100644 --- a/src/http/h2_client/dispatch.rs +++ b/src/http/h2_client/dispatch.rs @@ -619,7 +619,7 @@ pub fn decode_header_block(session: &mut ClientSession, stream: &mut Stream) { if stream.status_code != 0 || malformed { continue; } - if is_malformed_response_field(result.name) { + if is_malformed_response_field(result.name) || is_malformed_response_value(result.value) { malformed = true; continue; } @@ -754,6 +754,14 @@ pub fn is_malformed_response_field(name: &[u8]) -> bool { ) } +/// RFC 9113 §8.2.1: a field value MUST NOT contain NUL (0x00), LF (0x0a), or +/// CR (0x0d). HPACK is length-prefixed so these would otherwise pass through +/// verbatim, breaking the no-CR/LF invariant the HTTP/1.1 parser provides and +/// enabling header injection when values are forwarded downstream. +pub fn is_malformed_response_value(value: &[u8]) -> bool { + value.iter().any(|&c| c == 0 || c == b'\r' || c == b'\n') +} + pub fn error_code_for(err: bun_core::Error) -> wire::ErrorCode { // PORT NOTE: bun_core::Error is a NonZeroU16 interned tag; `err!()` yields // a const Error per name once the link-time table lands. Until then all diff --git a/src/http/lib.rs b/src/http/lib.rs index 504cb015e1c8..dea78105ed99 100644 --- a/src/http/lib.rs +++ b/src/http/lib.rs @@ -2308,6 +2308,11 @@ impl<'a> HTTPClient<'a> { } else if self.state.request_stage == RequestStage::Done && self.is_keep_alive_possible() && !socket.is_closed_or_has_error() + // A direct TLS socket verified against a Host-header override + // (get_tls_hostname) must not be pooled here: this.url has already + // been repointed at the redirect destination, so proxy_auth_hash() + // can no longer compute the correct pool key. Close it instead. + && (!IS_SSL || self.http_proxy.is_some() || self.hostname.is_none()) { // request_stage == .done: a 303 to a streaming POST can arrive before // the chunked upload's terminating 0\r\n\r\n is written. Pooling that @@ -2336,6 +2341,13 @@ impl<'a> HTTPClient<'a> { // (handleResponseMetadata already repointed this.url at the new one). self.prev_redirect = Vec::new(); + // Deferred until after the pool/close decision above — see + // `InternalStateFlags::clear_hostname_on_redirect`. + if self.state.flags.clear_hostname_on_redirect { + self.state.flags.clear_hostname_on_redirect = false; + self.hostname = None; + } + // TODO: should this check be before decrementing the redirect count? // the current logic will allow one less redirect than requested if self.remaining_redirect_count == 0 { @@ -3626,7 +3638,11 @@ impl<'a> HTTPClient<'a> { } else { 0 }, - if had_tunnel { + if had_tunnel || (IS_SSL && self.http_proxy.is_none()) { + // Direct TLS: the handshake verified the peer against + // the Host-header override (get_tls_hostname), so the + // override hash must be part of the pool key. Matches + // the lookup in HTTPContext::connect. self.proxy_auth_hash() } else { 0 @@ -3752,6 +3768,15 @@ impl<'a> HTTPClient<'a> { fn do_redirect_multiplexed(&mut self) { debug_assert!(self.flags.protocol != Protocol::Http1_1); bun_core::scoped_log!(fetch, "doRedirectMultiplexed"); + // See `do_redirect`: the cross-origin redirect must drop the + // per-request Host override before the follow-up connection derives + // its SNI / certificate-verification hostname. The h2/h3 path never + // reaches `do_redirect`'s consume-and-clear, so mirror it here before + // `state.reset()` discards the flag. + if self.state.flags.clear_hostname_on_redirect { + self.state.flags.clear_hostname_on_redirect = false; + self.hostname = None; + } if matches!(self.state.original_request_body, HTTPRequestBody::Stream(_)) { self.flags.is_streaming_request_body = false; } @@ -4240,10 +4265,35 @@ impl<'a> HTTPClient<'a> { 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") => { + // RFC 9110 section 9.3.6: a client MUST ignore + // Content-Length in a successful response to CONNECT — + // the connection becomes an opaque tunnel and is never + // pooled, so the framing-desync concern below does not + // apply. + if self.flags.proxy_tunneling + && self.proxy_tunnel.is_none() + && response.status_code == 200 + { + continue; + } // byte-level parse — header.value() is network bytes, not &str - let content_length = - bun_core::parse_unsigned::(header.value(), 10).unwrap_or(0); + // + // RFC 9112 section 6.3: an invalid or conflicting + // Content-Length is an unrecoverable framing error — + // falling back to 0 would release a desynchronized socket + // into the keep-alive pool. + let Ok(content_length) = bun_core::parse_unsigned::(header.value(), 10) + else { + return Err(err!(InvalidContentLength)); + }; if self.method.has_body() { + if self + .state + .content_length + .is_some_and(|prev| prev != content_length) + { + return Err(err!(InvalidContentLength)); + } self.state.content_length = Some(content_length); } else { // ignore body size for HEAD requests @@ -4273,6 +4323,15 @@ impl<'a> HTTPClient<'a> { } } h if h == hash_header_const(b"Transfer-Encoding") => { + // RFC 9110 section 9.3.6: as with Content-Length above, a + // client MUST ignore Transfer-Encoding in a successful + // response to CONNECT. + if self.flags.proxy_tunneling + && self.proxy_tunnel.is_none() + && response.status_code == 200 + { + continue; + } if header.value() == b"gzip" { if !self.flags.disable_decompression { self.state.transfer_encoding = Encoding::Gzip; @@ -4383,6 +4442,12 @@ impl<'a> HTTPClient<'a> { } } + // RFC 9110 §9.3.6: a non-200 response to CONNECT means the tunnel was + // not established. Surface the proxy's response to the caller, but + // never follow a Location header from it — a malicious proxy could + // otherwise redirect the request (body and custom headers included) + // to an attacker-chosen plaintext origin. + let mut is_proxy_connect_failure = false; if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() { if response.status_code == 200 { // signal to continue the proxing @@ -4392,6 +4457,7 @@ impl<'a> HTTPClient<'a> { // proxy denied connection so return proxy result (407, 403 etc) self.flags.proxy_tunneling = false; self.flags.disable_keepalive = true; + is_proxy_connect_failure = true; } let status_code = response.status_code; @@ -4404,7 +4470,8 @@ impl<'a> HTTPClient<'a> { // if is no redirect or if is redirect == "manual" just proceed let is_redirect = status_code >= 300 && status_code <= 399; if is_redirect { - if self.redirect_type == FetchRedirect::Follow + if !is_proxy_connect_failure + && self.redirect_type == FetchRedirect::Follow && !location.is_empty() && self.remaining_redirect_count > 0 { @@ -4643,6 +4710,13 @@ impl<'a> HTTPClient<'a> { } } + // Cross-origin redirect: re-derive SNI / cert + // verification / Host from the redirect target. See + // `InternalStateFlags::clear_hostname_on_redirect`. + if !is_same_origin { + self.state.flags.clear_hostname_on_redirect = true; + } + // https://fetch.spec.whatwg.org/#concept-http-redirect-fetch // If request's current URL's origin is not same origin with // locationURL's origin, then for each headerName of CORS @@ -4655,7 +4729,7 @@ impl<'a> HTTPClient<'a> { } // PORT NOTE: was a `const` table in Zig; LazyLock hashes // aren't const, so build at runtime. - let headers_to_remove: [H; 3] = [ + let headers_to_remove: [H; 4] = [ H { name: b"Authorization", hash: *AUTHORIZATION_HEADER_HASH, @@ -4668,6 +4742,13 @@ impl<'a> HTTPClient<'a> { name: b"Cookie", hash: *COOKIE_HEADER_HASH, }, + // A user-supplied Host header names the previous + // origin; keeping it would also suppress the + // default Host header derived from the new URL. + H { + name: HOST_HEADER_NAME, + hash: hash_header_const(HOST_HEADER_NAME), + }, ]; for to_remove in headers_to_remove.iter() { let mut i = 0; @@ -4690,7 +4771,7 @@ impl<'a> HTTPClient<'a> { } _ => {} } - } else if self.redirect_type == FetchRedirect::Error { + } else if !is_proxy_connect_failure && self.redirect_type == FetchRedirect::Error { // error out if redirect is not allowed return Err(err!(UnexpectedRedirect)); } diff --git a/src/http_types/URLPath.rs b/src/http_types/URLPath.rs index 4470758f077b..fa437d1a63d9 100644 --- a/src/http_types/URLPath.rs +++ b/src/http_types/URLPath.rs @@ -58,6 +58,16 @@ impl URLPath { out } + + /// Take ownership of the percent-decode buffer, if `parse()` had to + /// allocate one. The slice fields of `self` keep pointing into the + /// returned allocation — the caller must keep it alive for as long as any + /// of those slices (or sub-slices of them) are read; dropping it while + /// they are still in use leaves them dangling. + #[must_use = "dropping the returned storage dangles the slice fields of this URLPath"] + pub fn take_decoded_storage(&mut self) -> Option> { + self._decoded_storage.take() + } } // PORT NOTE: Zig uses two threadlocal fixed `[1024]u8`/`[16384]u8` buffers and diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index 1c89c8a663c9..49d7eaa8bb5c 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -1013,13 +1013,12 @@ pub fn enqueue_dependency_with_main_and_success_fn( // would pop their borrow-stack tags under SB. let cache_ctx = this.manifest_disk_cache_ctx(); let this_ptr: *mut PackageManager = this; - // SAFETY: `string_bytes` is not resized in the - // manifest-lookup path; every call below either copies - // `name_str` out or only reads it before any append. - // Detach the slice lifetime so the `&mut PackageManager` - // reborrows below do not conflict with it. - let name_str = this.lockfile.str_detached(&name); - let task_id = Task::Id::for_manifest(name_str); + // Owned copy: `get_or_put_resolved_package_with_find_result` + // below appends to `string_bytes` (and may reallocate it), + // and `name_str` is still read afterwards on the + // fall-through path. + let name_str: Vec = this.lockfile.str(&name).to_vec(); + let task_id = Task::Id::for_manifest(&name_str); if cfg!(debug_assertions) { debug_assert!(task_id.get() != 0); @@ -1051,7 +1050,7 @@ pub fn enqueue_dependency_with_main_and_success_fn( // from `manifests`. let scope: *const crate::npm::registry::Scope = unsafe { &(*this_ptr).options } - .scope_for_package_name(name_str); + .scope_for_package_name(&name_str); // SAFETY: `manifests` projected from // `this_ptr`; `cache_ctx` was snapshotted // before `this_ptr` so the lookup holds @@ -1160,14 +1159,14 @@ pub fn enqueue_dependency_with_main_and_success_fn( if verbose_install() { Output::pretty_errorln(format_args!( "Enqueue package manifest for download: {}", - bstr::BStr::new(name_str) + bstr::BStr::new(&name_str) )); } // `get_network_task` touches only the - // preallocated pool, not `string_bytes`; with - // `name_str` lifetime-detached above, `this` - // is free to reborrow `&mut`. + // preallocated pool, not `string_bytes`; + // `name_str` is an owned copy, so `this` is + // free to reborrow `&mut`. let network_task = this.get_network_task(); // SAFETY: `network_task` is the unique handle to a // freshly-vended pool slot. Zig's `network_task.* = .{ ... }` @@ -1177,11 +1176,11 @@ pub fn enqueue_dependency_with_main_and_success_fn( NetworkTask::write_init(network_task, task_id, this_ptr, None); } - let scope = this.scope_for_package_name(name_str); + let scope = this.scope_for_package_name(&name_str); // SAFETY: network_task points to a valid initialized NetworkTask slot unsafe { (*network_task).for_manifest( - name_str, + &name_str, scope, loaded_manifest.as_ref(), dependency.behavior.is_optional(), diff --git a/src/install/TarballStream.rs b/src/install/TarballStream.rs index e205a1b0520c..eabd129cfba2 100644 --- a/src/install/TarballStream.rs +++ b/src/install/TarballStream.rs @@ -1395,14 +1395,39 @@ fn make_symlink( return false; } { + // Normalize `symlink_dir/target` as a *relative* path with leading + // `..` preserved, and reject targets that climb above the extraction + // root. A fake absolute root cannot be used here: POSIX normalization + // clamps excess `..` at `/`, so a target like `../../packages/x` + // would normalize back under the fake root while the kernel still + // resolves the raw `..` components and escapes the extraction + // directory. let symlink_dir = bun_paths::dirname(path_slice).unwrap_or(b""); + let target_bytes = target.as_bytes(); let mut join_buf = PathBuffer::uninit(); - let resolved = resolve_path::join_abs_string_buf::( - b"/packages/", - &mut join_buf[..], - &[symlink_dir, target.as_bytes()], + if symlink_dir.len() + 1 + target_bytes.len() >= join_buf.len() { + return false; + } + let mut written = 0usize; + if !symlink_dir.is_empty() { + join_buf[..symlink_dir.len()].copy_from_slice(symlink_dir); + written = symlink_dir.len(); + join_buf[written] = b'/'; + written += 1; + } + join_buf[written..written + target_bytes.len()].copy_from_slice(target_bytes); + written += target_bytes.len(); + + let mut norm_buf = PathBuffer::uninit(); + let resolved = resolve_path::normalize_string_generic_t::( + &join_buf[..written], + &mut norm_buf[..], + b'/', + |c| c == b'/', ); - if !resolved.starts_with(b"/packages/") { + if bun_core::strings::eql(resolved, b"..") + || bun_core::strings::has_prefix_comptime(resolved, b"../") + { return false; } } diff --git a/src/install/dependency.rs b/src/install/dependency.rs index 2b75fb7954a7..02002e31d3cb 100644 --- a/src/install/dependency.rs +++ b/src/install/dependency.rs @@ -584,6 +584,28 @@ pub fn is_scoped_package_name(name: &[u8]) -> Result { Err(PackageNameError::InvalidPackageName) } +/// A dependency name/alias becomes a directory under `node_modules/`. Names +/// come from untrusted `package.json` / manifest keys, so reject anything that +/// could resolve outside that directory. `@scope/name` stays valid. +pub fn is_safe_install_folder_name(name: &[u8]) -> bool { + if name.is_empty() { + return false; + } + + for component in name.split(|&c| c == b'/') { + if component.is_empty() || component == b"." || component == b".." { + return false; + } + for &c in component { + if c == b'\\' || c == b':' || c == 0 { + return false; + } + } + } + + true +} + /// assumes version is valid pub fn without_build_tag(version: &[u8]) -> &[u8] { if let Some(plus) = strings::index_of_char(version, b'+') { diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index 74be4c7067a5..0d58177e7243 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2146,6 +2146,34 @@ pub fn install_isolated_packages( let pkg_name_hash = pkg_name_hashes[pkg_id as usize]; let pkg_res: Resolution = pkg_resolutions[pkg_id as usize]; + // Validate the package name and every dependency alias as + // `node_modules/` components before any filesystem work. + { + let mut unsafe_folder_name: Option<&[u8]> = None; + let name = pkg_name.slice(string_buf); + if !name.is_empty() && !crate::dependency::is_safe_install_folder_name(name) { + unsafe_folder_name = Some(name); + } else { + for dep in entry_dependencies[entry_id.get() as usize].slice() { + let dep_name = lockfile_ro.buffers.dependencies[dep.dep_id as usize] + .name + .slice(string_buf); + if !crate::dependency::is_safe_install_folder_name(dep_name) { + unsafe_folder_name = Some(dep_name); + break; + } + } + } + if let Some(name) = unsafe_folder_name { + Output::err_generic( + "\"{}\" is not a valid install folder name", + (BStr::new(name),), + ); + Output::flush(); + Global::exit(1); + } + } + match pkg_res.tag { ResolutionTag::Root => { if dep_id == invalid_dependency_id { diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index ed6dc191707a..8ec442a3efe0 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -300,6 +300,13 @@ impl<'a, const PATH_STYLE: IteratorPathStyle> Iterator<'a, PATH_STYLE> { } } +/// Tree folder names are joined into install destinations as +/// `node_modules//...`; this path and the tree builder must agree on the +/// same validator. +pub fn folder_name_is_safe(name: &[u8]) -> bool { + crate::dependency::is_safe_install_folder_name(name) +} + /// Returns relative path and the depth of the tree // PORT NOTE: reshaped — Zig takes `*const Lockfile`; here we take the three // buffer slices directly so callers from both `crate::lockfile` (stub) and @@ -365,6 +372,13 @@ pub fn relative_path_and_depth<'b, const PATH_STYLE: IteratorPathStyle>( let id = depth_buf[depth_buf_len]; let name = trees[id as usize].folder_name(dependencies, buf); + if !folder_name_is_safe(name) { + Output::err_generic( + "Lockfile is malformed (dependency name \"{}\" is not a valid folder name)", + (bstr::BStr::new(name),), + ); + bun_core::Global::crash(); + } let name_end = match path_written.checked_add(name.len()) { Some(end) if end < MAX_PATH_BYTES => end, _ => path_too_long(), @@ -806,6 +820,20 @@ impl Tree { let dependency = &dependencies[dep_id as usize]; + if !crate::dependency::is_safe_install_folder_name( + dependency + .name + .slice(lockfile.buffers.string_bytes.as_slice()), + ) { + builder.maybe_report_error(format_args!( + "Invalid dependency name \"{}\"", + dependency + .name + .fmt(lockfile.buffers.string_bytes.as_slice()), + )); + continue 'dep; + } + let hoisted: HoistDependencyResult = 'hoisted: { // don't hoist if it's a folder dependency or a bundled dependency. if dependency.behavior.is_bundled() { diff --git a/src/install/lockfile/bun.lock.rs b/src/install/lockfile/bun.lock.rs index 757740b89e93..ee11ed496ac4 100644 --- a/src/install/lockfile/bun.lock.rs +++ b/src/install/lockfile/bun.lock.rs @@ -67,6 +67,16 @@ fn string_array_hash_context(buf: &[u8]) -> bun_semver::string::ArrayHashContext } } +/// `true` if `url` points at a resource under `registry`: the registry href +/// (sans trailing slash) must be an exact prefix and the byte after it must be +/// a path separator, so `https://registry.example.com.evil.com/x.tgz` does not +/// count as being under a `https://registry.example.com` registry. +fn url_is_under_registry(url: &[u8], registry: &[u8]) -> bool { + let registry = strings::without_trailing_slash(registry); + strings::has_prefix(url, registry) + && (url.len() == registry.len() || url[registry.len()] == b'/') +} + // PORT NOTE: reshaped for borrowck. Zig keeps a single `var string_buf = // lockfile.stringBuf()` for the whole parser, but in Rust that locks out every // other `lockfile.*` access (the `string_buf()` method borrows the whole @@ -813,11 +823,9 @@ impl Stringifier { writer, "\"{}\", ", bstr::BStr::new( - if strings::has_prefix( + if url_is_under_registry( url_slice, - strings::without_trailing_slash( - Npm::Registry::DEFAULT_URL.as_bytes() - ), + Npm::Registry::DEFAULT_URL.as_bytes(), ) { b"" as &[u8] } else { @@ -2273,6 +2281,7 @@ pub fn parse_into_binary_lockfile( } }; + let mut npm_url_needs_integrity = false; if res.tag == ResolutionTag::Npm { if i >= (pkg_info.len_u32() as usize) { log.add_error(Some(source), value.loc, b"Missing npm registry"); @@ -2305,6 +2314,17 @@ pub fn parse_into_binary_lockfile( res.npm_mut().url = sbuf!(lockfile).append(url)?; } else { + let configured_registry = if let Some(mgr) = manager.as_deref() { + mgr.scope_for_package_name(name_str).url.href() + } else { + Npm::Registry::DEFAULT_URL.as_bytes() + }; + npm_url_needs_integrity = + !url_is_under_registry(registry_str, configured_registry) + && !url_is_under_registry( + registry_str, + Npm::Registry::DEFAULT_URL.as_bytes(), + ); res.npm_mut().url = sbuf!(lockfile).append(registry_str)?; } } @@ -2511,6 +2531,18 @@ pub fn parse_into_binary_lockfile( ); pkg.meta.integrity = Integrity::default(); } + + // Fail closed: otherwise a tampered lockfile could redirect + // the tarball URL off-registry and install arbitrary content + // under a trusted package name with verification disabled. + if npm_url_needs_integrity && !pkg.meta.integrity.tag.is_supported() { + log.add_error( + Some(source), + integrity_expr.loc, + b"Missing integrity hash for npm package resolved to a tarball URL outside the configured registry", + ); + return Err(ParseError::InvalidPackageInfo); + } } ResolutionTag::LocalTarball | ResolutionTag::RemoteTarball => { // integrity is optional for tarball deps (backward compat) diff --git a/src/install/lockfile/bun.lockb.rs b/src/install/lockfile/bun.lockb.rs index b517ce0131e0..85d9fb696f2a 100644 --- a/src/install/lockfile/bun.lockb.rs +++ b/src/install/lockfile/bun.lockb.rs @@ -116,6 +116,39 @@ const _: () = { assert!(size_of::() == 24 && align_of::() == 8); }; +/// On-disk layout of `PatchedDep` with the `bool` flag widened to `u8`. +/// +/// `read_array` reinterprets untrusted lockfile bytes as `T`, and +/// `PatchedDep::patchfile_hash_is_null` is a `bool` whose only valid byte +/// values are 0 and 1 — reinterpreting any other byte is immediate UB. Read +/// this invariant-free form instead and validate the flag before constructing +/// the real `PatchedDep`. +#[repr(C)] +#[derive(Clone, Copy)] +struct PatchedDepExternal { + path: SemverString, + _padding: [u8; 7], + patchfile_hash_is_null: u8, + patchfile_hash: u64, +} + +const _: () = { + assert!(size_of::() == size_of::()); + assert!(align_of::() == align_of::()); +}; + +impl PatchedDepExternal { + fn to_patched_dep(self) -> Result { + let mut dep = PatchedDep::with_path(self.path); + dep.set_patchfile_hash(match self.patchfile_hash_is_null { + 0 => Some(self.patchfile_hash), + 1 => None, + _ => return Err(bun_core::err!("InvalidLockfile")), + }); + Ok(dep) + } +} + /// Bridges `bun_semver::string::ArrayHashContext` (inherent `hash`/`eql`) to /// `bun_collections::ArrayHashAdapter` so `get_or_put_adapted` can use it. /// Can't `impl` the foreign trait for the foreign type directly (orphan rule). @@ -598,7 +631,8 @@ pub fn load( let map = &mut lockfile.patched_dependencies; map.ensure_total_capacity(patched_dependencies_name_and_version_hashes.len())?; - let patched_dependencies_paths: Vec = buffers::read_array(stream)?; + let patched_dependencies_paths: Vec = + buffers::read_array(stream)?; debug_assert_eq!( patched_dependencies_name_and_version_hashes.len(), @@ -609,7 +643,7 @@ pub fn load( .zip(patched_dependencies_paths.iter()) { // PERF(port): was assume_capacity - map.put_assume_capacity(*name_hash, *patch_path); + map.put_assume_capacity(*name_hash, patch_path.to_patched_dep()?); } } else { stream.pos -= 8; diff --git a/src/install/migration.rs b/src/install/migration.rs index 6a349d1e94e2..7b7d6e78fdf8 100644 --- a/src/install/migration.rs +++ b/src/install/migration.rs @@ -381,22 +381,21 @@ pub fn migrate_npm_lockfile<'a>( ); continue; } - if let Some(x) = pkg.get(b"inBundle") { - if matches!(x.data, ExprData::EBoolean(b) if b.value) { - id_map.put_assume_capacity( - pkg_path, - IdMapValue { - old_json_index: i as u32, - new_package_id: PACKAGE_ID_IS_BUNDLED, - }, - ); - continue; - } + // Counterpart of `is_skipped_pkg`: same per-flag truthiness, but + // bundled packages still get an id_map entry so dependency linking + // can recognize them. + if pkg_flag_is_true(pkg, b"inBundle") { + id_map.put_assume_capacity( + pkg_path, + IdMapValue { + old_json_index: i as u32, + new_package_id: PACKAGE_ID_IS_BUNDLED, + }, + ); + continue; } - if let Some(x) = pkg.get(b"extraneous") { - if matches!(x.data, ExprData::EBoolean(b) if b.value) { - continue; - } + if pkg_flag_is_true(pkg, b"extraneous") { + continue; } id_map.put_assume_capacity( @@ -605,12 +604,7 @@ pub fn migrate_npm_lockfile<'a>( continue; } - if pkg - .get(b"inBundle") - .or_else(|| pkg.get(b"extraneous")) - .map(|x| matches!(x.data, ExprData::EBoolean(b) if b.value)) - .unwrap_or(false) - { + if is_skipped_pkg(pkg) { continue; } @@ -919,13 +913,7 @@ pub fn migrate_npm_lockfile<'a>( // PORT NOTE: `StoreRef::get` shadows `E::Object::get`; deref-coerce. let pkg: &E::Object = pkg; - if pkg.get(b"link").is_some() - || pkg - .get(b"inBundle") - .or_else(|| pkg.get(b"extraneous")) - .map(|x| matches!(x.data, ExprData::EBoolean(b) if b.value)) - .unwrap_or(false) - { + if pkg.get(b"link").is_some() || is_skipped_pkg(pkg) { continue; } @@ -1577,6 +1565,19 @@ pub fn migrate_npm_lockfile<'a>( })) } +fn pkg_flag_is_true(pkg: &E::Object, key: &[u8]) -> bool { + pkg.get(key) + .map(|x| matches!(x.data, ExprData::EBoolean(b) if b.value)) + .unwrap_or(false) +} + +/// Skip predicate shared by the package counting, building, and linking +/// passes — all three must agree, otherwise the later passes append more +/// packages than the counting pass reserved. +fn is_skipped_pkg(pkg: &E::Object) -> bool { + pkg_flag_is_true(pkg, b"inBundle") || pkg_flag_is_true(pkg, b"extraneous") +} + fn package_name_from_path(pkg_path: &[u8]) -> &[u8] { if pkg_path.is_empty() { return b""; diff --git a/src/js/internal/sql/mysql.ts b/src/js/internal/sql/mysql.ts index 2df129b99da8..716683d38aa8 100644 --- a/src/js/internal/sql/mysql.ts +++ b/src/js/internal/sql/mysql.ts @@ -542,7 +542,15 @@ class MySQLAdapter }; } - validateTransactionOptions(_options: string): { valid: boolean; error?: string } { + validateTransactionOptions(options: string): { valid: boolean; error?: string } { + // The string is interpolated into `START TRANSACTION ${options}`, so refuse anything + // that could terminate the statement or start a new one. + if (!/^[A-Za-z ,]*$/.test(options)) { + return { + valid: false, + error: "Transaction options can only contain letters, spaces, and commas.", + }; + } return { valid: true }; } diff --git a/src/js/internal/sql/postgres.ts b/src/js/internal/sql/postgres.ts index d9271ffa4af0..e37da6c6b8f6 100644 --- a/src/js/internal/sql/postgres.ts +++ b/src/js/internal/sql/postgres.ts @@ -211,10 +211,16 @@ function getArrayType(typeNameOrID: number | ArrayType | undefined = undefined): } if (typeOfType === "string") { const type = (typeNameOrID as string).toUpperCase(); - // Allow `NUMERIC(10,2)`, `CHARACTER VARYING(255)`, `MYSCHEMA.MY_ENUM` - // — alnum, underscore, space, dot, comma, parens. The only goal is to - // refuse anything that could break out of the `$N::${type}[]` cast. - if (!/^[A-Z_][A-Z0-9_ .,()]*$/.test(type)) { + // Allow `NUMERIC(10,2)`, `CHARACTER VARYING(255)`, `MYSCHEMA.MY_ENUM`, + // `TIMESTAMP(3) WITH TIME ZONE`: identifier words separated by spaces or + // dots, each optionally followed by a `(digits[,digits])` modifier. + // Parentheses may only wrap digit lists so the value can never close the + // enclosing expression and break out of the `$N::${type}[]` cast. + if ( + !/^[A-Z_][A-Z0-9_]*(\( *[0-9]+( *, *[0-9]+)* *\))?([ .][A-Z_][A-Z0-9_]*(\( *[0-9]+( *, *[0-9]+)* *\))?)*$/.test( + type, + ) + ) { throw $ERR_INVALID_ARG_VALUE("type", typeNameOrID, "must be a valid PostgreSQL type name"); } return type; @@ -773,8 +779,15 @@ class PostgresAdapter }; } - validateTransactionOptions(_options: string): { valid: boolean; error?: string } { - // PostgreSQL accepts any transaction options + validateTransactionOptions(options: string): { valid: boolean; error?: string } { + // The string is interpolated into `BEGIN ${options}`, so refuse anything that + // could terminate the statement or start a new one. + if (!/^[A-Za-z ,]*$/.test(options)) { + return { + valid: false, + error: "Transaction options can only contain letters, spaces, and commas.", + }; + } return { valid: true }; } diff --git a/src/js/internal/sql/sqlite.ts b/src/js/internal/sql/sqlite.ts index 235f78a1214f..a7061616bd30 100644 --- a/src/js/internal/sql/sqlite.ts +++ b/src/js/internal/sql/sqlite.ts @@ -694,6 +694,15 @@ class SQLiteAdapter implements DatabaseAdapter { + // verify the result cannot escape that directory, either lexically + // ("..", absolute paths) or through a symlink that already exists on the + // host filesystem. + const RESOLVE_PATH = (stats, guestPath) => { if (!stats.path) { throw new types_1.WASIError(constants_1.WASI_EINVAL); } + // WASI paths are always interpreted relative to the directory fd. + // Re-root absolute guest paths under the preopen instead of letting + // them name an arbitrary host path. + let rel = String(guestPath); + while (rel.length !== 0 && (rel.charCodeAt(0) === 47 /* "/" */ || rel.charCodeAt(0) === 92) /* "\\" */) { + rel = rel.slice(1); + } const base = path.resolve(stats.path); - const resolved = path.resolve(base, p); - if (resolved !== base) { - const rel = path.relative(base, resolved); - if (rel === ".." || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel)) { + const resolved = path.resolve(base, rel); + const isContained = (parent, child) => + child === parent || child.startsWith(parent.endsWith(path.sep) ? parent : parent + path.sep); + if (!isContained(base, resolved)) { + throw new types_1.WASIError(constants_1.WASI_ENOTCAPABLE); + } + // A symlink that already exists inside the sandbox can still point + // outside of it. Resolve the closest existing ancestor with realpath + // and re-check containment. + let realBase = base; + try { + realBase = fs.realpathSync(base); + } catch {} + let probe = resolved; + let suffix = ""; + for (;;) { + let real; + try { + real = fs.realpathSync(probe); + } catch { + // Walk up on any resolution failure (ENOENT/ENOTDIR for + // not-yet-created components, but also ELOOP etc.) so `real` is + // always a *resolved* ancestor plus an unresolved suffix — + // comparing an unresolved path against the resolved preopen + // base would spuriously fail whenever the preopen itself + // traverses a symlink (e.g. macOS /tmp -> /private/tmp). + const parent = path.dirname(probe); + if (parent !== probe) { + suffix = path.sep + path.basename(probe) + suffix; + probe = parent; + continue; + } + real = probe; + } + if (!isContained(realBase, real + suffix)) { throw new types_1.WASIError(constants_1.WASI_ENOTCAPABLE); } + return resolved; } - return resolved; }; const CPUTIME_START = Bun.nanoseconds(); const timeOrigin = Math.trunc(performance.timeOrigin * 1e6); diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index d4ad51307aba..5be05149b305 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -951,6 +951,14 @@ impl JSValue { None } } + /// `as_array_buffer`, but pins the backing `JSC::ArrayBuffer` first so it + /// cannot be detached. The pin does not prevent collection. + pub fn as_pinned_arraybuffer(self, global: &JSGlobalObject) -> Option { + if !JSC__JSValue__pinArrayBuffer(self) { + return None; + } + self.as_array_buffer(global) + } /// Generic downcast (`as(comptime T)` in Zig). Dispatches via [`JsClass::from_js`]. #[inline] pub fn as_(self) -> Option<*mut T> { @@ -2087,6 +2095,7 @@ unsafe extern "C" { global: &JSGlobalObject, out: &mut ArrayBuffer, ) -> bool; + safe fn JSC__JSValue__pinArrayBuffer(this: JSValue) -> bool; safe fn JSC__JSValue__asPromise(this: JSValue) -> *mut JSPromise; safe fn JSC__JSValue__asInternalPromise(this: JSValue) -> *mut JSInternalPromise; safe fn Bun__attachAsyncStackFromPromise( diff --git a/src/jsc/array_buffer.rs b/src/jsc/array_buffer.rs index 3e773009673d..a4cf7bf06834 100644 --- a/src/jsc/array_buffer.rs +++ b/src/jsc/array_buffer.rs @@ -129,6 +129,16 @@ unsafe extern "C" { // `RefCounted` count mutation is interior to the opaque cell. safe fn JSC__ArrayBuffer__ref(self_: &JSCArrayBuffer); safe fn JSC__ArrayBuffer__deref(self_: &JSCArrayBuffer); + // safe: by-value `JSValue`; no-op for non-buffer values. + safe fn JSC__JSValue__unpinArrayBuffer(v: JSValue); +} + +impl JSValue { + /// Releases a pin taken on this value's backing `JSC::ArrayBuffer` by + /// [`JSValue::as_pinned_arraybuffer`] or a pinning collector. + pub fn unpin_array_buffer(self) { + JSC__JSValue__unpinArrayBuffer(self); + } } impl ArrayBuffer { @@ -136,6 +146,11 @@ impl ArrayBuffer { self.ptr.is_null() } + /// Releases the pin taken by [`JSValue::as_pinned_arraybuffer`]. + pub fn unpin(&self) { + self.value.unpin_array_buffer(); + } + // require('buffer').kMaxLength. // keep in sync with Bun::Buffer::kMaxLength pub const MAX_SIZE: c_uint = c_uint::MAX; diff --git a/src/jsc/bindings/BunString.cpp b/src/jsc/bindings/BunString.cpp index d6eb608b762d..1d10870009dd 100644 --- a/src/jsc/bindings/BunString.cpp +++ b/src/jsc/bindings/BunString.cpp @@ -419,7 +419,7 @@ extern "C" BunString BunString__fromUTF8(const char* bytes, size_t length) if (simdutf::validate_utf8(bytes, length)) { size_t u16Length = simdutf::utf16_length_from_utf8(bytes, length); std::span ptr; - auto impl = WTF::StringImpl::tryCreateUninitialized(static_cast(u16Length), ptr); + auto impl = WTF::StringImpl::tryCreateUninitialized(u16Length, ptr); if (!impl) [[unlikely]] { return { .tag = BunStringTag::Dead }; } diff --git a/src/jsc/bindings/NodeVMModule.cpp b/src/jsc/bindings/NodeVMModule.cpp index bba5326eb051..f547f38132ae 100644 --- a/src/jsc/bindings/NodeVMModule.cpp +++ b/src/jsc/bindings/NodeVMModule.cpp @@ -302,19 +302,40 @@ void NodeVMModulePrototype::finishCreation(JSC::VM& vm) JSC_DEFINE_CUSTOM_GETTER(jsNodeVmModuleGetterIdentifier, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, PropertyName propertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); - return JSValue::encode(JSC::jsString(globalObject->vm(), thisObject->identifier())); + VM& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue))) { + return JSValue::encode(JSC::jsString(vm, thisObject->identifier())); + } + + throwTypeError(globalObject, scope, "This function must be called on a SourceTextModule or SyntheticModule"_s); + return {}; } JSC_DEFINE_HOST_FUNCTION(jsNodeVmModuleGetStatusCode, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { - auto* thisObject = uncheckedDowncast(callFrame->thisValue()); - return JSValue::encode(JSC::jsNumber(static_cast(thisObject->status()))); + VM& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (auto* thisObject = dynamicDowncast(callFrame->thisValue())) { + return JSValue::encode(JSC::jsNumber(static_cast(thisObject->status()))); + } + + throwTypeError(globalObject, scope, "This function must be called on a SourceTextModule or SyntheticModule"_s); + return {}; } JSC_DEFINE_HOST_FUNCTION(jsNodeVmModuleGetStatus, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { - auto* thisObject = uncheckedDowncast(callFrame->thisValue()); + VM& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + auto* thisObject = dynamicDowncast(callFrame->thisValue()); + if (!thisObject) { + throwTypeError(globalObject, scope, "This function must be called on a SourceTextModule or SyntheticModule"_s); + return {}; + } using enum NodeVMModule::Status; switch (thisObject->status()) { @@ -353,7 +374,7 @@ JSC_DEFINE_HOST_FUNCTION(jsNodeVmModuleGetError, (JSC::JSGlobalObject * globalOb VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - if (auto* thisObject = uncheckedDowncast(callFrame->thisValue())) { + if (auto* thisObject = dynamicDowncast(callFrame->thisValue())) { if (JSC::Exception* exception = thisObject->evaluationException()) { return JSValue::encode(exception->value()); } @@ -370,7 +391,11 @@ JSC_DEFINE_HOST_FUNCTION(jsNodeVmModuleGetModuleRequests, (JSC::JSGlobalObject * auto& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto* thisObject = uncheckedDowncast(callFrame->thisValue()); + auto* thisObject = dynamicDowncast(callFrame->thisValue()); + if (!thisObject) { + throwTypeError(globalObject, scope, "This function must be called on a SourceTextModule or SyntheticModule"_s); + return {}; + } if (auto* sourceTextModule = dynamicDowncast(callFrame->thisValue())) { sourceTextModule->ensureModuleRecord(globalObject); @@ -493,8 +518,15 @@ JSC_DEFINE_HOST_FUNCTION(jsNodeVmModuleCreateCachedData, (JSC::JSGlobalObject * JSC_DEFINE_HOST_FUNCTION(jsNodeVmModuleCreateModuleRecord, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { - auto* thisObject = uncheckedDowncast(callFrame->thisValue()); - return JSValue::encode(thisObject->createModuleRecord(globalObject)); + VM& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (auto* thisObject = dynamicDowncast(callFrame->thisValue())) { + RELEASE_AND_RETURN(scope, JSValue::encode(thisObject->createModuleRecord(globalObject))); + } + + throwTypeError(globalObject, scope, "This function must be called on a SourceTextModule or SyntheticModule"_s); + return {}; } template diff --git a/src/jsc/bindings/bindings.cpp b/src/jsc/bindings/bindings.cpp index b004b4b3e484..b16ad16c1eb3 100644 --- a/src/jsc/bindings/bindings.cpp +++ b/src/jsc/bindings/bindings.cpp @@ -3345,8 +3345,9 @@ bool JSC__JSValue__asArrayBuffer( // Pin/unpin the backing ArrayBuffer of a JSArrayBuffer or JSArrayBufferView so // transfer()/detach() throw while a native borrower holds a slice into it. -// `pin` is a no-op on SharedArrayBuffer (already non-detachable). Returns -// false if `value` has no ArrayBuffer impl. +// SharedArrayBuffer is never detachable and never moves, so it is left +// unpinned rather than rejected. Returns false if `value` has no ArrayBuffer +// impl. static JSC::ArrayBuffer* arrayBufferImpl(JSC::JSValue value) { if (auto* jb = dynamicDowncast(value)) @@ -3358,15 +3359,18 @@ static JSC::ArrayBuffer* arrayBufferImpl(JSC::JSValue value) CPP_DECL bool JSC__JSValue__pinArrayBuffer(JSC::EncodedJSValue v) { if (auto* buf = arrayBufferImpl(JSC::JSValue::decode(v))) { - buf->pin(); + if (!buf->isShared()) + buf->pin(); return true; } return false; } CPP_DECL void JSC__JSValue__unpinArrayBuffer(JSC::EncodedJSValue v) { - if (auto* buf = arrayBufferImpl(JSC::JSValue::decode(v))) - buf->unpin(); + if (auto* buf = arrayBufferImpl(JSC::JSValue::decode(v))) { + if (!buf->isShared()) + buf->unpin(); + } } // Borrow `v`'s byte storage for off-thread reading. Splits out only the @@ -3406,7 +3410,8 @@ CPP_DECL int32_t JSC__JSValue__borrowBytesForOffThread(JSC::EncodedJSValue v, co // contract allows it). auto* buf = view->possiblySharedBuffer(); if (!buf) return 0; - buf->pin(); + if (!buf->isShared()) + buf->pin(); *out_ptr = static_cast(view->vector()); *out_len = view->byteLength(); return 2; @@ -3414,7 +3419,8 @@ CPP_DECL int32_t JSC__JSValue__borrowBytesForOffThread(JSC::EncodedJSValue v, co if (auto* jb = dynamicDowncast(value)) { auto* buf = jb->impl(); if (!buf || buf->isDetached()) return 0; - buf->pin(); + if (!buf->isShared()) + buf->pin(); *out_ptr = static_cast(buf->data()); *out_len = buf->byteLength(); return 2; @@ -6560,6 +6566,68 @@ extern "C" JSC::EncodedJSValue Bun__REPL__formatValue( return JSC::JSValue::encode(result); } +// Collects every ArrayBufferView in a JSArray and the (data, byteLength) span +// of each. Two passes, mirroring Buffer.concat: the first reads every element +// into a MarkedArgumentBuffer, so any user code an indexed read can run +// (getters, proxy traps) finishes before the second pass takes raw pointers. +// A backing store detached during the first pass reads back as a zero-length +// span. +// +// When `pinBuffers` is true, each view's backing ArrayBuffer is materialized +// and pinned before its data pointer is read, so the span stays valid after +// control returns to JS (an in-flight async I/O). The caller must balance +// every pinned element with `JSC__JSValue__unpinArrayBuffer`. SharedArrayBuffer +// is never detachable and never moves, so it is left unpinned. +// +// Returns 0 on success, 1 if the value is not a JSArray or an element is not +// an ArrayBufferView, 2 on allocation failure, -1 if an exception is pending. +extern "C" int32_t Bun__JSArray__collectBufferSpans( + JSC::JSGlobalObject* globalObject, + JSC::EncodedJSValue encodedValue, + bool pinBuffers, + void* ctx, + void (*append)(void* ctx, JSC::EncodedJSValue element, void* data, size_t byteLength)) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + JSC::JSValue value = JSC::JSValue::decode(encodedValue); + if (!value.isCell() || !JSC::isJSArray(value.asCell())) + return 1; + JSC::JSArray* array = uncheckedDowncast(value.asCell()); + + JSC::MarkedArgumentBuffer values; + values.ensureCapacity(array->length()); + if (values.hasOverflowed()) [[unlikely]] + return 2; + + JSC::forEachInArrayLike(globalObject, array, [&](JSC::JSValue element) -> bool { + values.append(element); + return true; + }); + RETURN_IF_EXCEPTION(scope, -1); + if (values.hasOverflowed()) [[unlikely]] + return 2; + + for (unsigned i = 0; i < unsigned(values.size()); i++) { + auto* view = dynamicDowncast(values.at(i)); + if (!view) + return 1; + if (pinBuffers) { + // possiblySharedBuffer() converts a FastTypedArray (GC-movable + // storage, no ArrayBuffer yet) into a malloc-backed one and can + // repoint m_vector, so it must run before vector() is read. + auto* buf = view->possiblySharedBuffer(); + if (!buf) [[unlikely]] + return 2; + if (!buf->isShared()) + buf->pin(); + } + append(ctx, JSC::JSValue::encode(view), view->vector(), view->byteLength()); + } + return 0; +} + extern "C" const JSC::EncodedJSValue* Bun__JSArray__getContiguousVector( JSC::EncodedJSValue encodedValue, uint32_t* outLength) diff --git a/src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp b/src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp index 0ed176ffae75..1e4891172c96 100644 --- a/src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp +++ b/src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp @@ -116,13 +116,19 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionNodeHTTPServerSocketEnd, (JSC::JSGlobalObject // Implementation of custom getters JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterIsSecureEstablished, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } return JSValue::encode(JSC::jsBoolean(thisObject->isAuthorized())); } JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterDuplex, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (thisObject->m_duplex) { return JSValue::encode(thisObject->m_duplex.get()); } @@ -132,7 +138,10 @@ JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterDuplex, (JSC::JSGlobalObjec JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterDuplex, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::EncodedJSValue encodedValue, JSC::PropertyName propertyName)) { auto& vm = globalObject->vm(); - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return false; + } JSValue value = JSC::JSValue::decode(encodedValue); if (auto* object = value.getObject()) { thisObject->m_duplex.set(vm, thisObject, object); @@ -146,7 +155,10 @@ JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterDuplex, (JSC::JSGlobalObjec JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterRemoteAddress, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { auto& vm = globalObject->vm(); - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (thisObject->m_remoteAddress) { return JSValue::encode(thisObject->m_remoteAddress.get()); } @@ -179,7 +191,10 @@ JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterRemoteAddress, (JSC::JSGlob JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterLocalAddress, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { auto& vm = globalObject->vm(); - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (thisObject->m_localAddress) { return JSValue::encode(thisObject->m_localAddress.get()); } @@ -211,7 +226,10 @@ JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterLocalAddress, (JSC::JSGloba JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterOnClose, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (thisObject->functionToCallOnClose) { return JSValue::encode(thisObject->functionToCallOnClose.get()); @@ -222,7 +240,10 @@ JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterOnClose, (JSC::JSGlobalObje JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterOnDrain, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (thisObject->functionToCallOnDrain) { return JSValue::encode(thisObject->functionToCallOnDrain.get()); @@ -236,7 +257,10 @@ JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterOnDrain, (JSC::JSGlobalObje auto& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return false; + } JSValue value = JSC::JSValue::decode(encodedValue); if (value.isUndefined() || value.isNull()) { @@ -254,7 +278,10 @@ JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterOnDrain, (JSC::JSGlobalObje JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterOnData, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (thisObject->functionToCallOnData) { return JSValue::encode(thisObject->functionToCallOnData.get()); @@ -268,7 +295,10 @@ JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterOnData, (JSC::JSGlobalObjec auto& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return false; + } JSValue value = JSC::JSValue::decode(encodedValue); if (value.isUndefined() || value.isNull()) { @@ -289,7 +319,10 @@ JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterOnClose, (JSC::JSGlobalObje auto& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return false; + } JSValue value = JSC::JSValue::decode(encodedValue); if (value.isUndefined() || value.isNull()) { @@ -307,19 +340,28 @@ JSC_DEFINE_CUSTOM_SETTER(jsNodeHttpServerSocketSetterOnClose, (JSC::JSGlobalObje JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterClosed, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } return JSValue::encode(JSC::jsBoolean(thisObject->isClosed())); } JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterBytesWritten, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } return JSValue::encode(JSC::jsNumber(thisObject->streamBuffer.totalBytesWritten())); } JSC_DEFINE_CUSTOM_GETTER(jsNodeHttpServerSocketGetterResponse, (JSC::JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName)) { - auto* thisObject = uncheckedDowncast(JSC::JSValue::decode(thisValue)); + auto* thisObject = dynamicDowncast(JSC::JSValue::decode(thisValue)); + if (!thisObject) [[unlikely]] { + return JSValue::encode(JSC::jsUndefined()); + } if (!thisObject->currentResponseObject) { return JSValue::encode(JSC::jsNull()); } diff --git a/src/jsc/bindings/node/crypto/JSCipherConstructor.cpp b/src/jsc/bindings/node/crypto/JSCipherConstructor.cpp index 6261433f5fb0..fef1972bcea1 100644 --- a/src/jsc/bindings/node/crypto/JSCipherConstructor.cpp +++ b/src/jsc/bindings/node/crypto/JSCipherConstructor.cpp @@ -205,6 +205,11 @@ JSC_DEFINE_HOST_FUNCTION(constructCipher, (JSC::JSGlobalObject * globalObject, J if (cipher.isSupportedAuthenticatedMode()) { initAuthenticated(globalObject, scope, ctx, cipherString, cipherKind, ivLen, authTagLength, maxMessageSize); RETURN_IF_EXCEPTION(scope, {}); + } else { + // Like Node, only keep authTagLength for authenticated modes. Keeping an + // unvalidated value here would let getAuthTag() memcpy past the 16-byte + // m_authTag buffer. + authTagLength = std::nullopt; } if (!ctx.setKeyLength(keyData.size())) { diff --git a/src/jsc/bindings/node/crypto/JSVerify.cpp b/src/jsc/bindings/node/crypto/JSVerify.cpp index 4b493c1878ef..89ce5170a83e 100644 --- a/src/jsc/bindings/node/crypto/JSVerify.cpp +++ b/src/jsc/bindings/node/crypto/JSVerify.cpp @@ -421,17 +421,20 @@ JSC_DEFINE_HOST_FUNCTION(jsVerifyProtoFuncVerify, (JSGlobalObject * globalObject if (dsaSigEnc == DSASigEnc::P1363 && keyPtr.isSigVariant()) { WTF::Vector derBuffer; - if (convertP1363ToDER(sigBuf, keyPtr, derBuffer)) { - // Conversion succeeded, perform verification with the converted signature - ncrypto::Buffer derSigBuf { - .data = derBuffer.begin(), - .len = derBuffer.size(), - }; - - bool result = pkctx.verify(derSigBuf, data); - return JSValue::encode(jsBoolean(result)); + // If the signature cannot be converted to DER (e.g. its length is not + // 2 * bytesOfRS), fail verification instead of reinterpreting the raw + // bytes as a DER signature, matching Node.js. + if (!convertP1363ToDER(sigBuf, keyPtr, derBuffer)) { + return JSValue::encode(jsBoolean(false)); } - // If conversion failed, fall through to use the original signature + + ncrypto::Buffer derSigBuf { + .data = derBuffer.begin(), + .len = derBuffer.size(), + }; + + bool result = pkctx.verify(derSigBuf, data); + return JSValue::encode(jsBoolean(result)); } // Perform verification with the original signature diff --git a/src/jsc/bindings/node/http/NodeHTTPParser.cpp b/src/jsc/bindings/node/http/NodeHTTPParser.cpp index 75e5ebd84620..5443c08b2c31 100644 --- a/src/jsc/bindings/node/http/NodeHTTPParser.cpp +++ b/src/jsc/bindings/node/http/NodeHTTPParser.cpp @@ -117,21 +117,36 @@ JSValue HTTPParser::execute(JSGlobalObject* globalObject, const char* data, size auto scope = DECLARE_THROW_SCOPE(vm); auto& builtinNames = WebCore::builtinNames(vm); - m_currentBufferLen = len; - m_currentBufferData = data; + // Forbid re-entrant execution of a new buffer while a previous execute() + // is still on the stack: llhttp keeps span pointers into the in-progress + // buffer, and a nested run over a different buffer corrupts them. + if (data != nullptr && m_inExecute) { + throwTypeError(globalObject, scope, "HTTPParser.execute is not reentrant"_s); + return {}; + } llhttp_errno_t err; if (data == nullptr) { err = llhttp_finish(&m_parserData); } else { + m_inExecute = true; + m_currentBufferLen = len; + m_currentBufferData = data; err = llhttp_execute(&m_parserData, data, len); save(); } size_t nread = len; if (err != HPE_OK) { - nread = llhttp_get_error_pos(&m_parserData) - data; + // On the finish() path `data` is nullptr, and after a lingering error + // `error_pos` may point into a previous buffer. Only derive nread from + // the error position when it provably lies within the current buffer, + // so a raw heap pointer is never exposed to JS via `bytesParsed`. + const char* errorPos = llhttp_get_error_pos(&m_parserData); + if (data != nullptr && errorPos >= data && errorPos <= data + len) { + nread = errorPos - data; + } if (err == HPE_PAUSED_UPGRADE) { err = HPE_OK; @@ -144,8 +159,11 @@ JSValue HTTPParser::execute(JSGlobalObject* globalObject, const char* data, size llhttp_pause(&m_parserData); } - m_currentBufferLen = 0; - m_currentBufferData = nullptr; + if (data != nullptr) { + m_currentBufferLen = 0; + m_currentBufferData = nullptr; + m_inExecute = false; + } RETURN_IF_EXCEPTION(scope, {}); diff --git a/src/jsc/bindings/node/http/NodeHTTPParser.h b/src/jsc/bindings/node/http/NodeHTTPParser.h index cef788de8275..88e2a70d6d4a 100644 --- a/src/jsc/bindings/node/http/NodeHTTPParser.h +++ b/src/jsc/bindings/node/http/NodeHTTPParser.h @@ -190,6 +190,9 @@ struct HTTPParser { const char* m_currentBufferData; bool m_headersCompleted = false; bool m_pendingPause = false; + // Set while execute() is running llhttp over a buffer. Owned exclusively + // by execute(); finish() must never clear it. + bool m_inExecute = false; uint64_t m_headerNread = 0; uint64_t m_chunkExtensionsNread = 0; uint64_t m_maxHttpHeaderSize = 0; diff --git a/src/jsc/bindings/sqlite/JSSQLStatement.cpp b/src/jsc/bindings/sqlite/JSSQLStatement.cpp index d09de269a820..74e6a57ce54c 100644 --- a/src/jsc/bindings/sqlite/JSSQLStatement.cpp +++ b/src/jsc/bindings/sqlite/JSSQLStatement.cpp @@ -176,7 +176,7 @@ static inline JSC::JSValue jsBigIntFromSQLite(JSC::JSGlobalObject* globalObject, #define DO_REBIND(param) \ if (param.isObject()) { \ - JSC::JSValue reb = castedThis->rebind(lexicalGlobalObject, param, true, castedThis->version_db->db); \ + JSC::JSValue reb = castedThis->rebind(lexicalGlobalObject, param, castedThis->version_db->db); \ RETURN_IF_EXCEPTION(scope, {}); \ if (!reb.isNumber()) [[unlikely]] { \ return JSValue::encode(reb); /* this means an error */ \ @@ -454,7 +454,7 @@ class JSSQLStatement : public JSC::JSDestructibleObject { static void analyzeHeap(JSCell*, JSC::HeapAnalyzer&); - JSC::JSValue rebind(JSGlobalObject* globalObject, JSC::JSValue values, bool clone, sqlite3* db); + JSC::JSValue rebind(JSGlobalObject* globalObject, JSC::JSValue values, sqlite3* db); bool need_update() { return version_db->version.load() != version; } void update_version() { version = version_db->version.load(); } @@ -815,7 +815,7 @@ void JSSQLStatement::destroy(JSC::JSCell* cell) thisObject->~JSSQLStatement(); } -static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3* db, sqlite3_stmt* stmt, int i, JSC::JSValue value, JSC::ThrowScope& scope, bool clone, bool isSafeInteger) +static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3* db, sqlite3_stmt* stmt, int i, JSC::JSValue value, JSC::ThrowScope& scope, bool isSafeInteger) { auto throwSQLiteError = [&]() -> void { throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, WTF::String::fromUTF8(sqlite3_errmsg(db)))); @@ -828,12 +828,11 @@ static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3 return false; \ } - // only clone if necessary - // SQLite has a way to call a destructor - // but there doesn't seem to be a way to pass a pointer? - // we can't use it if there's no pointer to ref/unref - auto transientOrStatic = (void (*)(void*))(clone ? SQLITE_TRANSIENT : SQLITE_STATIC); - + // Always copy string and blob payloads. With SQLITE_STATIC, sqlite3 keeps + // the raw pointer until sqlite3_step(), but a re-entrant property getter + // for a later parameter can free the backing store first (detaching an + // ArrayBuffer, or triggering a GC that collects an otherwise-unrooted + // string). if (value.isUndefinedOrNull()) { CHECK_BIND(sqlite3_bind_null(stmt, i)); } else if (value.isBoolean()) { @@ -861,9 +860,9 @@ static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3 } if (roped->is8Bit() && roped->containsOnlyASCII()) { - CHECK_BIND(sqlite3_bind_text(stmt, i, reinterpret_cast(roped->span8().data()), roped->length(), transientOrStatic)); + CHECK_BIND(sqlite3_bind_text(stmt, i, reinterpret_cast(roped->span8().data()), roped->length(), SQLITE_TRANSIENT)); } else if (!roped->is8Bit()) { - CHECK_BIND(sqlite3_bind_text16(stmt, i, roped->span16().data(), roped->length() * 2, transientOrStatic)); + CHECK_BIND(sqlite3_bind_text16(stmt, i, roped->span16().data(), roped->length() * 2, SQLITE_TRANSIENT)); } else { auto utf8 = roped->utf8(); CHECK_BIND(sqlite3_bind_text(stmt, i, utf8.data(), utf8.length(), SQLITE_TRANSIENT)); @@ -886,7 +885,7 @@ static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3 } } else if (JSC::JSArrayBufferView* buffer = dynamicDowncast(value)) { - CHECK_BIND(sqlite3_bind_blob(stmt, i, buffer->vector(), buffer->byteLength(), transientOrStatic)); + CHECK_BIND(sqlite3_bind_blob(stmt, i, buffer->vector(), buffer->byteLength(), SQLITE_TRANSIENT)); } else { throwException(lexicalGlobalObject, scope, createTypeError(lexicalGlobalObject, "Binding expected string, TypedArray, boolean, number, bigint or null"_s)); return false; @@ -896,10 +895,22 @@ static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3 #undef CHECK_BIND } -static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindingsMap& bindings, JSC::JSObject* target, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, bool clone, bool safeIntegers) +static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindingsMap& bindings, JSC::JSObject* target, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, bool safeIntegers, JSSQLStatement* statement) { int count = 0; + // Reading a property off `target` can run arbitrary JS (getters, Proxy + // traps), which can call statement.finalize() and free `stmt`. Re-validate + // before touching `stmt` again after any callback into JS. + const auto& statementStillAlive = [&]() -> bool { + if (statement && statement->stmt != stmt) [[unlikely]] { + if (!scope.exception()) + throwException(globalObject, scope, createError(globalObject, "Statement has finalized"_s)); + return false; + } + return true; + }; + auto& vm = JSC::getVM(globalObject); auto& structure = *target->structure(); bindings.ensureNamesLoaded(vm, stmt); @@ -951,6 +962,8 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin auto* name = sqlite3_bind_parameter_name(stmt, i + 1); JSValue value = getValue(name, i); + if (!statementStillAlive()) + return {}; if (!value && !scope.exception()) { if (throwOnMissing) { throwException(globalObject, scope, createError(globalObject, makeString("Missing parameter \""_s, WTF::String::fromUTF8ReplacingInvalidSequences({ reinterpret_cast(name), strlen(name) }), "\""_s))); @@ -960,7 +973,7 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin } RETURN_IF_EXCEPTION(scope, {}); - if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, clone, safeIntegers)) { + if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, safeIntegers)) { return {}; } @@ -975,6 +988,8 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin else if (bindings.isOnlyIndexed) [[unlikely]] { for (size_t i = 0; i < size; i++) { JSValue value = target->getDirectIndex(globalObject, i); + if (!statementStillAlive()) + return {}; if (!value && !scope.exception()) { if (throwOnMissing) { throwException(globalObject, scope, createError(globalObject, makeString("Missing parameter \""_s, i + 1, "\""_s))); @@ -985,7 +1000,7 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin RETURN_IF_EXCEPTION(scope, {}); - if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, clone, safeIntegers)) { + if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, safeIntegers)) { return {}; } @@ -1001,6 +1016,8 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin for (size_t i = 0; i < size; i++) { const auto& property = bindingNames[i]; JSValue value = property.isEmpty() ? target->getDirectIndex(globalObject, i) : target->fastGetOwnProperty(vm, structure, bindingNames[i]); + if (!statementStillAlive()) + return {}; if (!value && !scope.exception()) { if (throwOnMissing) { throwException(globalObject, scope, createError(globalObject, makeString("Missing parameter \""_s, property.isEmpty() ? String::number(i) : property.string(), "\""_s))); @@ -1011,7 +1028,7 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin RETURN_IF_EXCEPTION(scope, {}); - if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, clone, safeIntegers)) { + if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, safeIntegers)) { return {}; } @@ -1043,7 +1060,10 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin RETURN_IF_EXCEPTION(scope, {}); - if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, clone, safeIntegers)) { + if (!statementStillAlive()) + return {}; + + if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, safeIntegers)) { return {}; } @@ -1055,7 +1075,7 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin return jsNumber(count); } -static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, bool clone, SQLiteBindingsMap& bindings, bool safeIntegers) +static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, SQLiteBindingsMap& bindings, bool safeIntegers, JSSQLStatement* statement) { sqlite3_clear_bindings(stmt); JSC::JSArray* array = dynamicDowncast(values); @@ -1063,7 +1083,7 @@ static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JS if (!array) { if (JSC::JSObject* object = values.getObject()) { - auto res = rebindObject(lexicalGlobalObject, bindings, object, scope, db, stmt, clone, safeIntegers); + auto res = rebindObject(lexicalGlobalObject, bindings, object, scope, db, stmt, safeIntegers, statement); RETURN_IF_EXCEPTION(scope, {}); return res; } @@ -1087,7 +1107,7 @@ static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JS int i = 0; for (; i < count; i++) { JSC::JSValue value = array->getIndexQuickly(i); - if (!rebindValue(lexicalGlobalObject, db, stmt, i + 1, value, scope, clone, safeIntegers)) { + if (!rebindValue(lexicalGlobalObject, db, stmt, i + 1, value, scope, safeIntegers)) { return {}; } RETURN_IF_EXCEPTION(scope, {}); @@ -1453,7 +1473,7 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteFunction, (JSC::JSGlobalObject * l int count = sqlite3_bind_parameter_count(sql.stmt); SQLiteBindingsMap bindings { static_cast(count > -1 ? count : 0), strict }; - JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, sql.stmt, false, bindings, safeIntegers); + JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, sql.stmt, bindings, safeIntegers, nullptr); RETURN_IF_EXCEPTION(scope, {}); if (!reb.isNumber()) [[unlikely]] { @@ -2766,13 +2786,22 @@ void JSSQLStatement::analyzeHeap(JSCell* cell, HeapAnalyzer& analyzer) Base::analyzeHeap(cell, analyzer); } -JSC::JSValue JSSQLStatement::rebind(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, bool clone, sqlite3* db) +JSC::JSValue JSSQLStatement::rebind(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, sqlite3* db) { auto& vm = JSC::getVM(lexicalGlobalObject); auto scope = DECLARE_THROW_SCOPE(vm); auto* stmt = this->stmt; - auto val = rebindStatement(lexicalGlobalObject, values, scope, this->version_db->db, stmt, clone, this->m_bindingNames, this->useBigInt64); + auto val = rebindStatement(lexicalGlobalObject, values, scope, this->version_db->db, stmt, this->m_bindingNames, this->useBigInt64, this); + RETURN_IF_EXCEPTION(scope, {}); + + // A getter invoked while binding can finalize this statement; the callers + // cache `stmt` before binding and call sqlite3_step() on it afterwards. + if (this->stmt != stmt) [[unlikely]] { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s)); + return {}; + } + if (val.isNumber()) { RELEASE_AND_RETURN(scope, val); } else { diff --git a/src/jsc/bindings/webcore/SerializedScriptValue.cpp b/src/jsc/bindings/webcore/SerializedScriptValue.cpp index 180351b7888f..6a2e006c212c 100644 --- a/src/jsc/bindings/webcore/SerializedScriptValue.cpp +++ b/src/jsc/bindings/webcore/SerializedScriptValue.cpp @@ -3636,6 +3636,23 @@ class CloneDeserializer : public CloneBase { LengthType byteLength; if (!read(byteLength)) return false; + // The backing store of an ArrayBufferView can only be an ArrayBuffer (or a + // reference to one already in the object pool). Reject anything else before + // recursing into readTerminal() so a crafted payload of nested + // ArrayBufferViewTags can't consume one native stack frame per level and + // overflow the stack. + if (m_ptr >= m_end) + return false; + switch (static_cast(*m_ptr)) { + case ArrayBufferTag: + case ResizableArrayBufferTag: + case ArrayBufferTransferTag: + case SharedArrayBufferTag: + case ObjectReferenceTag: + break; + default: + return false; + } JSValue arrayBufferValue = readTerminal(); if (!arrayBufferValue || !arrayBufferValue.inherits()) return false; @@ -5019,7 +5036,7 @@ class CloneDeserializer : public CloneBase { } case ObjectReferenceTag: { auto index = readConstantPoolIndex(m_gcBuffer); - if (!index) { + if (!index || *index >= m_gcBuffer.size()) { fail(); return JSValue(); } diff --git a/src/resolver/package_json.rs b/src/resolver/package_json.rs index 4f638ac9920f..3ed254c65b80 100644 --- a/src/resolver/package_json.rs +++ b/src/resolver/package_json.rs @@ -2707,6 +2707,33 @@ impl<'a> ESModule<'a> { } } + // If the wildcard match (or trailing-slash remainder) taken from + // the import specifier contains any ".", ".." or "node_modules" + // segments, throw an Invalid Module Specifier error. Node's + // PACKAGE_TARGET_RESOLVE applies the same validation to + // patternMatch; without it the specifier can substitute "../" + // segments into the target and escape the package directory. + if !subpath.is_empty() { + if let Some(invalid) = find_invalid_subpath_segment(subpath) { + if let Some(log) = self.debug_logs.as_deref_mut() { + log.add_note_fmt(format_args!( + "The path \"{}\" is invalid because it contains an invalid segment \"{}\"", + bstr::BStr::new(subpath), + bstr::BStr::new(invalid) + )); + } + dedent!(); + return Resolution { + path: Box::<[u8]>::from(subpath), + status: Status::InvalidModuleSpecifier, + debug: ResolutionDebug { + token: target.first_token, + ..Default::default() + }, + }; + } + } + // If target does not start with "./", then... if !strings::starts_with(str, b"./") { if let Some(log) = self.debug_logs.as_deref_mut() { @@ -3126,6 +3153,29 @@ fn find_invalid_segment(path_: &[u8]) -> Option<&[u8]> { None } +// Like `find_invalid_segment`, but for the wildcard match (`patternMatch`) +// extracted from the import specifier rather than for a target string from +// package.json: every segment is validated, including the first, and a +// separator-less single-segment path is allowed. +fn find_invalid_subpath_segment(path_: &[u8]) -> Option<&[u8]> { + let mut path = path_; + while !path.is_empty() { + let mut segment = path; + if let Some(new_slash) = strings::index_any_comptime(path, b"/\\") { + segment = &path[0..new_slash]; + path = &path[new_slash + 1..]; + } else { + path = b""; + } + + if is_invalid_segment(segment) { + return Some(segment); + } + } + + None +} + // Node's PACKAGE_TARGET_RESOLVE rejects ".", "..", and "node_modules" segments // case-insensitively and including percent-encoded variants. Decode the segment // before comparing so spellings like "%2e%2e" or ".%2E" cannot survive the check diff --git a/src/runtime/api/bun/h2_frame_parser.rs b/src/runtime/api/bun/h2_frame_parser.rs index 2701a6506f5d..e938f117efb0 100644 --- a/src/runtime/api/bun/h2_frame_parser.rs +++ b/src/runtime/api/bun/h2_frame_parser.rs @@ -5918,20 +5918,19 @@ impl H2FrameParser { ); return Err(global_object.throw_value(exception)); } - let validated_name = - match Self::to_valid_header_name(name, &mut name_buffer[0..name.len()]) { - Ok(n) => n, - Err(_) => { - let exception = global_object.to_type_error( - bun_jsc::ErrorCode::INVALID_HTTP_TOKEN, - format_args!( - "The arguments Header name is invalid. Received {}", - BStr::new(name) - ), - ); - return Err(global_object.throw_value(exception)); - } - }; + let validated_name = match Self::to_valid_header_name(name, &mut name_buffer[..]) { + Ok(n) => n, + Err(_) => { + let exception = global_object.to_type_error( + bun_jsc::ErrorCode::INVALID_HTTP_TOKEN, + format_args!( + "The arguments Header name is invalid. Received {}", + BStr::new(name) + ), + ); + return Err(global_object.throw_value(exception)); + } + }; // closure for encode error handling let mut handle_encode = @@ -6550,20 +6549,19 @@ impl H2FrameParser { let name_slice = header_name.to_utf8(); let name = name_slice.slice(); - let validated_name = - match Self::to_valid_header_name(name, &mut name_buffer[0..name.len()]) { - Ok(n) => n, - Err(_) => { - let exception = global_object.to_type_error( - bun_jsc::ErrorCode::INVALID_HTTP_TOKEN, - format_args!( - "The arguments Header name is invalid. Received \"{}\"", - BStr::new(name) - ), - ); - return Err(global_object.throw_value(exception)); - } - }; + let validated_name = match Self::to_valid_header_name(name, &mut name_buffer[..]) { + Ok(n) => n, + Err(_) => { + let exception = global_object.to_type_error( + bun_jsc::ErrorCode::INVALID_HTTP_TOKEN, + format_args!( + "The arguments Header name is invalid. Received \"{}\"", + BStr::new(name) + ), + ); + return Err(global_object.throw_value(exception)); + } + }; if name.first() == Some(&b':') { if ignore_pseudo_headers == 1 { @@ -7017,10 +7015,15 @@ impl H2FrameParser { 0 }; let available_payload = actual_max_frame_size - priority_overhead; - let padding: u8 = if encoded_size > available_payload { + // Reserve one byte for the pad-length field so `encoded_size + + // padding_overhead` never exceeds `available_payload`; otherwise the + // CONTINUATION branch below would slice past the end of the encoded + // header block. CONTINUATION frames cannot carry padding, so it is + // disabled whenever the block does not fit in a single HEADERS frame. + let padding: u8 = if encoded_size >= available_payload { 0 } else { - stream.get_padding(encoded_size, available_payload) + stream.get_padding(encoded_size, available_payload - 1) }; let padding_overhead: usize = if padding != 0 { padding as usize + 1 diff --git a/src/runtime/api/filesystem_router.rs b/src/runtime/api/filesystem_router.rs index 7b5e4c22c37c..a55dbc325e50 100644 --- a/src/runtime/api/filesystem_router.rs +++ b/src/runtime/api/filesystem_router.rs @@ -597,9 +597,11 @@ impl FileSystemRouter { // `route`. Borrowck can't see that the allocation travels with the borrow, so we // detach the slice from `path`'s ownership here. The bytes stay valid: `path` is // never dropped on any path between here and `MatchedRoute::init` taking ownership - // (early returns above this point already dropped/replaced `path`). + // (early returns above this point already dropped/replaced `path`), except when + // `URLPath::parse` percent-decoded — in that case nothing borrows `path_bytes` + // anymore and `path` is swapped for the decode buffer below. let path_bytes: &[u8] = unsafe { bun_ptr::detach_lifetime(path.slice()) }; - let url_path = match URLPath::parse(path_bytes) { + let mut url_path = match URLPath::parse(path_bytes) { Ok(v) => v, Err(err) => { return Err(global_this.throw(format_args!( @@ -622,6 +624,17 @@ impl FileSystemRouter { return Ok(JSValue::NULL); }; + // If `URLPath::parse` had to percent-decode, `route.pathname`/`query_string` and + // the param values borrow the decode buffer — not `path` — and that buffer would + // be freed when `url_path` drops at the end of this call. Take ownership of it and + // make it the backing allocation instead. (`Box<[u8]>` -> `Vec` -> + // `ZigStringSlice::Owned` reuses the same heap allocation, so the borrowed slices + // stay valid; nothing in `route` points into the original encoded `path` once a + // decode happened.) + if let Some(decoded) = url_path.take_decoded_storage() { + path = ZigStringSlice::init_owned(decoded.into_vec()); + } + // PORT NOTE: Zig leaked `path` here (TODO comment in spec) and pointer-freed it // in `MatchedRoute.deinit` via `mi_free(pathname.ptr)`. We instead MOVE `path` // into `MatchedRoute` so the bytes that `route.pathname`/`query_string`/param diff --git a/src/runtime/api/zlib.classes.ts b/src/runtime/api/zlib.classes.ts index 0475f1d2b4da..37f80fc47638 100644 --- a/src/runtime/api/zlib.classes.ts +++ b/src/runtime/api/zlib.classes.ts @@ -10,7 +10,7 @@ function generate(name: string) { estimatedSize: true, klass: {}, JSType: "0b11101110", - values: ["writeCallback", "errorCallback", "dictionary"], + values: ["writeCallback", "errorCallback", "dictionary", "pendingInput", "pendingOutput"], proto: { init: { fn: "init" }, diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index 6e80d424d45d..bb6b975c88fc 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -2225,6 +2225,26 @@ pub mod internal { pub hash: u64, } + impl RequestKeyOwned { + /// Cache-lookup equality: same hash *and* same hostname bytes. The hash + /// (wyhash, fixed seed) is not collision resistant, so it is only a + /// fast reject — never the sole match criterion. + fn matches(&self, other: &RequestKey) -> bool { + if self.hash != other.hash { + return false; + } + match (self.host.as_ref(), other.host) { + (Some(a), Some(b)) => { + // SAFETY: `other.host` borrows the caller's NUL-terminated + // slice, which outlives the lookup (see `RequestKey.host`). + a.as_bytes() == unsafe { (*b).as_bytes() } + } + (None, None) => true, + _ => false, + } + } + } + impl RequestKey { pub fn init(name: Option<&ZStr>, port: u16) -> Self { let hash = if let Some(n) = name { @@ -2430,7 +2450,7 @@ pub mod internal { let entry = self.cache[i]; // SAFETY: entries 0..len are valid heap Requests unsafe { - if (*entry).key.hash == key.hash && (*entry).valid { + if (*entry).key.matches(key) && (*entry).valid { if (*entry).is_expired(timestamp_to_store) { bun_output::scoped_log!(dns, "get: expired entry"); if (*entry).refcount == 0 { diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index d264a864c36b..8fb6e416fde7 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -2885,6 +2885,7 @@ pub mod args { impl Unprotect for FdVectorIo { #[inline] fn unprotect(&mut self) { + self.buffers.release(); self.buffers.value.unprotect(); // Zig: `self.buffers.buffers.deinit()` — `Vec` frees on drop. } @@ -2896,11 +2897,14 @@ pub mod args { } pub fn from_js(ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice) -> JsResult { let fd = FD::from_js_required(ctx, arguments)?; - let buffers = VectorArrayBuffer::from_js( + let mut buffers = VectorArrayBuffer::from_js( ctx, arguments.protect_eat_next().ok_or_else(|| { ctx.throw_invalid_arguments(format_args!("Expected an ArrayBufferView[]")) })?, + // The iovec pointers outlive this call on the async path; root + // each element and pin its backing store until completion. + arguments.will_be_async, )?; let mut position: Option = None; if let Some(pos_value) = arguments.next_eat() { @@ -2908,6 +2912,9 @@ pub mod args { if pos_value.is_number() { position = Some(pos_value.to_int64() as u64); } else { + // `buffers` never reaches the Unprotect hook on this + // path; drop its element roots and pins here. + buffers.release(); return Err( ctx.throw_invalid_arguments(format_args!("position must be a number")) ); @@ -3927,6 +3934,9 @@ pub mod args { pub offset: u64, pub length: u64, pub position: Option, + /// True when `from_js` pinned `buffer` for the async path; balanced in + /// `unprotect()` (the JS-thread release hook). + pub pinned: bool, } impl Read { pub fn to_thread_safe(&self) { @@ -3936,6 +3946,9 @@ pub mod args { impl Unprotect for Read { #[inline] fn unprotect(&mut self) { + if self.pinned { + self.buffer.buffer.unpin(); + } self.buffer.buffer.value.unprotect(); } } @@ -3993,6 +4006,7 @@ pub mod args { length: 0, offset: 0, position: None, + pinned: false, }); } @@ -4105,12 +4119,28 @@ pub mod args { None }; + let (buffer, pinned) = if arguments.will_be_async { + match buffer_value.as_pinned_arraybuffer(ctx) { + Some(pinned) => ( + Buffer { + buffer: pinned, + owns_buffer: false, + }, + true, + ), + None => (buffer, false), + } + } else { + (buffer, false) + }; + Ok(Read { fd, buffer, offset, length, position, + pinned, }) } } diff --git a/src/runtime/node/node_fs_binding.rs b/src/runtime/node/node_fs_binding.rs index 9739a4a01137..4eb3177c1cf1 100644 --- a/src/runtime/node/node_fs_binding.rs +++ b/src/runtime/node/node_fs_binding.rs @@ -90,7 +90,7 @@ fn run_async( // Task completes), and `slice` is intentionally not dropped — its // `Drop`-unprotect would race that. - let args = match ::from_js(global, &mut slice) { + let mut args = match ::from_js(global, &mut slice) { Ok(a) => a, Err(err) => { // SAFETY: not yet dropped; only drop site for this path. @@ -100,6 +100,7 @@ fn run_async( }; if global.has_exception() { + args.unprotect(); drop(args); // SAFETY: not yet dropped; only drop site for this path. unsafe { ManuallyDrop::drop(&mut slice) }; @@ -114,6 +115,7 @@ fn run_async( global, reason.to_js(global), ); + args.unprotect(); drop(args); // SAFETY: not yet dropped; only drop site for this path. unsafe { ManuallyDrop::drop(&mut slice) }; diff --git a/src/runtime/node/node_zlib_binding.rs b/src/runtime/node/node_zlib_binding.rs index 3102790bc2a4..60836f339967 100644 --- a/src/runtime/node/node_zlib_binding.rs +++ b/src/runtime/node/node_zlib_binding.rs @@ -278,6 +278,10 @@ pub trait CompressionStreamImpl: Sized + Taskable + 'static { fn write_callback_get_cached(this_value: JSValue) -> Option; fn error_callback_get_cached(this_value: JSValue) -> Option; fn error_callback_set_cached(this_value: JSValue, global: &JSGlobalObject, cb: JSValue); + fn pending_input_set_cached(this_value: JSValue, global: &JSGlobalObject, value: JSValue); + fn pending_output_set_cached(this_value: JSValue, global: &JSGlobalObject, value: JSValue); + fn pending_input_get_cached(this_value: JSValue) -> Option; + fn pending_output_get_cached(this_value: JSValue) -> Option; } impl CompressionStream { @@ -300,7 +304,6 @@ impl CompressionStream { let in_off: u32; let in_len: u32; - let in_: Option<&[u8]>; let this_value = callframe.this(); @@ -322,15 +325,12 @@ impl CompressionStream { .throw()); } - // Hoisted so `in_` can borrow it past the `else` arm (mirrors `out_buf`). - let in_buf: jsc::ArrayBuffer; if arguments[1].is_null() { // just a flush - in_ = None; in_len = 0; in_off = 0; } else { - in_buf = match arguments[1].as_array_buffer(global_this) { + let in_buf = match arguments[1].as_array_buffer(global_this) { Some(b) => b, None => { return Err(global_this @@ -355,12 +355,9 @@ impl CompressionStream { ) .throw()); } - // Bounds checked above; `byte_slice` is the safe accessor for the JS - // ArrayBuffer's backing store (rooted via `arguments[1]` on the call stack). - in_ = Some(&in_buf.byte_slice()[in_off as usize..in_off as usize + in_len as usize]); } - let Some(mut out_buf) = arguments[4].as_array_buffer(global_this) else { + let Some(out_buf) = arguments[4].as_array_buffer(global_this) else { return Err(global_this .err( ErrorCode::INVALID_ARG_TYPE, @@ -382,11 +379,6 @@ impl CompressionStream { ) .throw()); } - // Bounds checked above; `byte_slice_mut` is the safe accessor for the JS - // ArrayBuffer's backing store (rooted via `arguments[4]` on the call stack). - let out: Option<&mut [u8]> = Some( - &mut out_buf.byte_slice_mut()[out_off as usize..out_off as usize + out_len as usize], - ); let _ = (in_off, in_len, out_off, out_len); if this.write_in_progress().get() { @@ -402,9 +394,35 @@ impl CompressionStream { .err(ErrorCode::INVALID_STATE, format_args!("Pending close")) .throw()); } + // Pin both buffers before mutating any state: materializing a + // FastTypedArray's backing store can fail on OOM, and failing here + // leaves nothing to unwind. + let in_buf: jsc::ArrayBuffer; + let in_: Option<&[u8]> = if arguments[1].is_null() { + None + } else { + let Some(buf) = arguments[1].as_pinned_arraybuffer(global_this) else { + return Err(global_this.throw_out_of_memory()); + }; + in_buf = buf; + Some(&in_buf.byte_slice()[in_off as usize..in_off as usize + in_len as usize]) + }; + let Some(mut out_buf) = arguments[4].as_pinned_arraybuffer(global_this) else { + if !arguments[1].is_null() { + arguments[1].unpin_array_buffer(); + } + return Err(global_this.throw_out_of_memory()); + }; + let out: Option<&mut [u8]> = Some( + &mut out_buf.byte_slice_mut()[out_off as usize..out_off as usize + out_len as usize], + ); + this.write_in_progress().set(true); this.ref_(); + T::pending_input_set_cached(this_value, global_this, arguments[1]); + T::pending_output_set_cached(this_value, global_this, arguments[4]); + this.stream().with_mut(|s| { s.set_buffers(in_, out); s.set_flush(i32::try_from(flush).expect("int cast")); @@ -505,6 +523,22 @@ impl CompressionStream { this_value.ensure_still_alive(); + for pinned in [ + T::pending_input_get_cached(this_value), + T::pending_output_get_cached(this_value), + ] + .into_iter() + .flatten() + { + if pinned.is_cell() { + if let Some(buf) = pinned.as_array_buffer(global) { + buf.unpin(); + } + } + } + T::pending_input_set_cached(this_value, global, JSValue::ZERO); + T::pending_output_set_cached(this_value, global, JSValue::ZERO); + if !Self::check_error(&this, global, this_value) { this.poll_ref().with_mut(|p| p.unref(vm)); // SAFETY: see above. @@ -955,11 +989,10 @@ macro_rules! __impl_compression_stream { } /// `T.js.*` — cached-property accessors emitted by - /// `generate-classes.ts` for `values: ["writeCallback", - /// "errorCallback", "dictionary"]`. + /// `generate-classes.ts` for the `values:` list in `zlib.classes.ts`. #[allow(unused)] pub mod js { - ::bun_jsc::codegen_cached_accessors!($type_name; writeCallback, errorCallback, dictionary); + ::bun_jsc::codegen_cached_accessors!($type_name; writeCallback, errorCallback, dictionary, pendingInput, pendingOutput); } impl $crate::node::node_zlib_binding::CompressionContext for $ctx { @@ -1015,6 +1048,18 @@ macro_rules! __impl_compression_stream { #[inline] fn error_callback_set_cached(this_value: ::bun_jsc::JSValue, global: &::bun_jsc::JSGlobalObject, cb: ::bun_jsc::JSValue) { js::error_callback_set_cached(this_value, global, cb) } + #[inline] fn pending_input_set_cached(this_value: ::bun_jsc::JSValue, global: &::bun_jsc::JSGlobalObject, value: ::bun_jsc::JSValue) { + js::pending_input_set_cached(this_value, global, value) + } + #[inline] fn pending_output_set_cached(this_value: ::bun_jsc::JSValue, global: &::bun_jsc::JSGlobalObject, value: ::bun_jsc::JSValue) { + js::pending_output_set_cached(this_value, global, value) + } + #[inline] fn pending_input_get_cached(this_value: ::bun_jsc::JSValue) -> Option<::bun_jsc::JSValue> { + js::pending_input_get_cached(this_value) + } + #[inline] fn pending_output_get_cached(this_value: ::bun_jsc::JSValue) -> Option<::bun_jsc::JSValue> { + js::pending_output_get_cached(this_value) + } } }; } diff --git a/src/runtime/node/types.rs b/src/runtime/node/types.rs index 580dfa798345..114cd92c5de1 100644 --- a/src/runtime/node/types.rs +++ b/src/runtime/node/types.rs @@ -1413,49 +1413,127 @@ pub struct VectorArrayBuffer { // Stored in a stack-local during writev; never heap-allocated. pub value: JSValue, pub buffers: Vec, + /// The collected elements, in order. Rooted (and their backing stores + /// pinned) for the lifetime of an async operation; see [`Self::release`]. + pub views: Vec, + pinned: bool, } impl VectorArrayBuffer { pub fn to_js(&self, _: &JSGlobalObject) -> JSValue { self.value } -} -impl VectorArrayBuffer { - pub fn from_js(global_object: &JSGlobalObject, val: JSValue) -> JsResult { - if !val.js_type().is_array_like() { - return Err( - global_object.throw_invalid_arguments(format_args!("Expected ArrayBufferView[]")) - ); + /// Release the per-element roots and pins taken by `from_js(.., pin: true)`. + /// Must run on the JS thread, exactly once, after the I/O completes. + pub fn release(&mut self) { + if !self.pinned { + return; } + self.pinned = false; + for view in self.views.drain(..) { + view.unpin_array_buffer(); + view.unprotect(); + } + } +} - let mut bufferlist: Vec = Vec::new(); - let mut i: usize = 0; - let len = val.get_length(global_object)? as usize; - bufferlist.reserve_exact(len); +unsafe extern "C" { + fn Bun__JSArray__collectBufferSpans( + global_object: &JSGlobalObject, + value: JSValue, + pin_buffers: bool, + ctx: *mut std::ffi::c_void, + append: unsafe extern "C" fn( + ctx: *mut std::ffi::c_void, + element: JSValue, + data: *mut u8, + byte_len: usize, + ), + ) -> i32; +} - while i < len { - let element = val.get_index(global_object, i as u32)?; +unsafe extern "C" fn append_buffer_span( + ctx: *mut std::ffi::c_void, + element: JSValue, + data: *mut u8, + byte_len: usize, +) { + // SAFETY: `ctx` is the `&mut VectorArrayBuffer` passed to + // `Bun__JSArray__collectBufferSpans` by `from_js` below, alive for the + // duration of the call. + let out = unsafe { &mut *ctx.cast::() }; + let slice: &mut [u8] = if data.is_null() || byte_len == 0 { + &mut [] + } else { + // SAFETY: `data..data + byte_len` is the byte range of `element`'s + // backing store, valid and unaliased for the duration of the callback. + unsafe { std::slice::from_raw_parts_mut(data, byte_len) } + }; + out.buffers.push(bun_sys::platform_iovec_create(slice)); + out.views.push(element); +} - if !element.is_cell() { - return Err(global_object - .throw_invalid_arguments(format_args!("Expected ArrayBufferView[]"))); +impl VectorArrayBuffer { + /// Collect an array of ArrayBufferViews into iovecs. Every element is read + /// before any raw pointer is taken, so user code run by an indexed read (a + /// getter, a proxy trap) cannot free a backing store that has already been + /// captured. + /// + /// `pin` is required when the spans outlive this call (async I/O): each + /// element is rooted and its backing store is pinned against detach until + /// [`Self::release`] runs. + pub fn from_js( + global_object: &JSGlobalObject, + val: JSValue, + pin: bool, + ) -> JsResult { + let mut out = VectorArrayBuffer { + value: val, + buffers: Vec::new(), + views: Vec::new(), + pinned: false, + }; + bun_jsc::validation_scope!(scope, global_object); + // SAFETY: `out` outlives the call; the callback only dereferences the + // ctx pointer it is handed. + let status = unsafe { + Bun__JSArray__collectBufferSpans( + global_object, + val, + pin, + (&raw mut out).cast(), + append_buffer_span, + ) + }; + scope.assert_exception_presence_matches(status == -1); + if pin { + // The C++ side already pinned each backing store; root the views + // themselves so a getter-returned element that is not reachable + // from `value` survives until completion. Set `pinned` even on + // failure so `release()` balances the elements collected before + // the error. + out.pinned = true; + for view in &out.views { + view.protect(); + } + } + match status { + 0 => Ok(out), + -1 => { + out.release(); + Err(jsc::JsError::Thrown) + } + 2 => { + out.release(); + Err(global_object.throw_out_of_memory()) + } + _ => { + out.release(); + Err(global_object + .throw_invalid_arguments(format_args!("Expected ArrayBufferView[]"))) } - - let Some(mut array_buffer) = element.as_array_buffer(global_object) else { - return Err(global_object - .throw_invalid_arguments(format_args!("Expected ArrayBufferView[]"))); - }; - - let buf = array_buffer.byte_slice_mut(); - bufferlist.push(bun_sys::platform_iovec_create(buf)); - i += 1; } - - Ok(VectorArrayBuffer { - value: val, - buffers: bufferlist, - }) } } diff --git a/src/runtime/node/win_watcher.rs b/src/runtime/node/win_watcher.rs index a26745e8e84e..6b774ef69730 100644 --- a/src/runtime/node/win_watcher.rs +++ b/src/runtime/node/win_watcher.rs @@ -45,10 +45,11 @@ bun_output::declare_scope!(fs_watch, visible); // pointer — and lets every load/store be safe code (`RacyCell` required an // `unsafe` block per access for the same single-word op). // -// NOTE: the manager binds to one VM's `uv_loop`, so even with the mutex this -// remains a per-VM resource — `watch()` debug-asserts the caller's `vm` -// matches the stored one. Promoting this to per-VM storage (e.g. `RareData`) -// is the longer-term fix; the mutex closes the UB window meanwhile. +// NOTE: the manager binds to one VM's `uv_loop`, so it is a per-VM resource — +// `watch()` allocates a fresh manager whenever the caller's `vm` differs from +// the one stored here (last caller wins the slot), so a Worker never mutates +// another VM's manager or drives its uv_loop cross-thread. Promoting this to +// true per-VM storage (e.g. `RareData`) is the longer-term fix. static DEFAULT_MANAGER: bun_core::AtomicCell<*mut PathWatcherManager> = bun_core::AtomicCell::new(ptr::null_mut()); static DEFAULT_MANAGER_MUTEX: Mutex = Mutex::new(); @@ -71,7 +72,12 @@ impl PathWatcherManager { bun_core::heap::into_raw(Box::new(PathWatcherManager { watchers: StringArrayHashMap::default(), vm, - deinit_on_last_watcher: false, + // A manager can be displaced from `DEFAULT_MANAGER` by a `watch()` + // call from a different VM; without this the displaced manager + // would never be freed. Set here — on the owning thread, before the + // pointer is published — to avoid a cross-thread write at + // displacement time. + deinit_on_last_watcher: true, })) } @@ -490,36 +496,35 @@ pub fn watch( #[cfg(not(windows))] compile_error!("win_watcher should only be used on Windows"); - let manager = { - let _g = DEFAULT_MANAGER_MUTEX.lock_guard(); - // DEFAULT_MANAGER is only read/written while holding - // DEFAULT_MANAGER_MUTEX (see static decl). `fs.watch()` is reachable - // from Worker threads, so an unguarded read+write here would be a data - // race — the prior "JS main thread only" claim was false. - let m = DEFAULT_MANAGER.load(); - if m.is_null() { - let m = PathWatcherManager::init(vm); - DEFAULT_MANAGER.store(m); - m - } else { - // The manager is bound to one VM's uv_loop; reusing it from a - // different VM (Worker) would drive libuv cross-thread. Catch - // that in debug until this becomes per-VM storage. - debug_assert!( - // SAFETY: `m` is a non-null pointer published under - // DEFAULT_MANAGER_MUTEX (which we hold) by `init` above on a - // prior call; the allocation lives until `deinit` clears the - // slot, so it is valid here. - core::ptr::eq(unsafe { (*m).vm }, vm), - "win_watcher PathWatcherManager reused across VMs (Worker fs.watch)", - ); - m - } + // DEFAULT_MANAGER is only read/written while holding DEFAULT_MANAGER_MUTEX + // (see static decl). The guard covers the whole registration — not just the + // slot load — because `PathWatcher::init` below mutates the manager's + // `watchers` map, and `fs.watch()` is reachable from Worker threads: two + // Workers releasing the lock before that mutation would alias `&mut *manager`. + let _g = DEFAULT_MANAGER_MUTEX.lock_guard(); + let existing = DEFAULT_MANAGER.load(); + // The manager is bound to one VM's uv_loop; reusing it from a different VM + // (Worker) would mutate its watcher map and drive libuv cross-thread. + // Allocate a fresh manager for this VM instead; the displaced one frees + // itself once its last watcher unregisters (`deinit_on_last_watcher`). + // SAFETY: `existing` is a non-null pointer published under + // DEFAULT_MANAGER_MUTEX (which we hold) by `init` below on a prior call; + // the allocation lives until `deinit` clears the slot, so it is valid here. + // `vm` is written once at construction and never mutated, so reading it + // cannot race with the owning VM's thread. + let manager = if existing.is_null() || !core::ptr::eq(unsafe { (*existing).vm }, vm) { + let m = PathWatcherManager::init(vm); + DEFAULT_MANAGER.store(m); + m + } else { + existing }; - // SAFETY: `manager` is a live heap-allocated pointer published under - // DEFAULT_MANAGER_MUTEX; per the debug_assert above all callers share the - // same VM thread, so this `&mut` is unaliased for the call. + // SAFETY: `manager` is a live heap-allocated pointer bound to the calling + // VM (created above or matched by `vm`). All other mutation of this manager + // happens on this VM's thread, and concurrent `watch()` calls from other + // Workers are serialized by DEFAULT_MANAGER_MUTEX (still held here), so + // this `&mut` is unaliased for the call. let watcher = match PathWatcher::init(unsafe { &mut *manager }, path, recursive) { sys::Result::Err(err) => return sys::Result::Err(err), sys::Result::Ok(w) => w, diff --git a/src/runtime/node/zlib/NativeBrotli.rs b/src/runtime/node/zlib/NativeBrotli.rs index f98dc7c63945..8bfe10b4c107 100644 --- a/src/runtime/node/zlib/NativeBrotli.rs +++ b/src/runtime/node/zlib/NativeBrotli.rs @@ -191,14 +191,55 @@ mod _impl { // this does not get gc'd because it is stored in the JS object's // `this._writeState`. and the JS object is tied to the native handle // as `_handle[owner_symbol]`. - let write_result = arguments.ptr[1] - .as_array_buffer(global_this) - .unwrap() - .as_u32() - .as_mut_ptr(); + // `flush_write_result` writes two u32s through this pointer, so the + // caller-supplied array must hold at least 2 elements. + let write_result_value = arguments.ptr[1]; + let Some(mut write_result_buf) = write_result_value.as_array_buffer(global_this) else { + return Err(global_this.throw_invalid_argument_type_value( + "writeResult", + "Uint32Array", + write_result_value, + )); + }; + if write_result_buf.typed_array_type != bun_jsc::JSType::Uint32Array { + return Err(global_this.throw_invalid_argument_type_value( + "writeResult", + "Uint32Array", + write_result_value, + )); + } + let write_result_slice = write_result_buf.as_u32(); + if write_result_slice.len() < 2 { + return Err(global_this + .err( + ErrorCode::INVALID_ARG_VALUE, + format_args!("writeResult must be a Uint32Array with at least 2 elements"), + ) + .throw()); + } + let write_result = write_result_slice.as_mut_ptr(); let write_callback = validators::validate_function(global_this, "writeCallback", arguments.ptr[2])?; + // Validate `params` before any native state is initialized so the + // error path needs no cleanup. `as_u32` reinterprets the view's + // bytes, so the element type must actually be Uint32Array. + let params_value = arguments.ptr[0]; + let Some(mut params_buf) = params_value.as_array_buffer(global_this) else { + return Err(global_this.throw_invalid_argument_type_value( + "params", + "Uint32Array", + params_value, + )); + }; + if params_buf.typed_array_type != bun_jsc::JSType::Uint32Array { + return Err(global_this.throw_invalid_argument_type_value( + "params", + "Uint32Array", + params_value, + )); + } + self.write_result.set(Some(write_result)); js::write_callback_set_cached( @@ -213,7 +254,6 @@ mod _impl { return Ok(JSValue::FALSE); } - let mut params_buf = arguments.ptr[0].as_array_buffer(global_this).unwrap(); let params_ = params_buf.as_u32(); for (i, &d) in params_.iter().enumerate() { diff --git a/src/runtime/node/zlib/NativeZlib.rs b/src/runtime/node/zlib/NativeZlib.rs index c40ca15102b4..cd0512f09820 100644 --- a/src/runtime/node/zlib/NativeZlib.rs +++ b/src/runtime/node/zlib/NativeZlib.rs @@ -142,11 +142,33 @@ mod _impl { let strategy = validators::validate_int32(global, arguments.ptr[3], "strategy", None, None)?; // this does not get gc'd because it is stored in the JS object's `this._writeState`. and the JS object is tied to the native handle as `_handle[owner_symbol]`. - let write_result = arguments.ptr[4] - .as_array_buffer(global) - .unwrap() - .as_u32() - .as_mut_ptr(); + // `flush_write_result` writes two u32s through this pointer, so the + // caller-supplied array must hold at least 2 elements. + let write_result_value = arguments.ptr[4]; + let Some(mut write_result_buf) = write_result_value.as_array_buffer(global) else { + return Err(global.throw_invalid_argument_type_value( + "writeResult", + "Uint32Array", + write_result_value, + )); + }; + if write_result_buf.typed_array_type != bun_jsc::JSType::Uint32Array { + return Err(global.throw_invalid_argument_type_value( + "writeResult", + "Uint32Array", + write_result_value, + )); + } + let write_result_slice = write_result_buf.as_u32(); + if write_result_slice.len() < 2 { + return Err(global + .err( + bun_jsc::ErrorCode::INVALID_ARG_VALUE, + format_args!("writeResult must be a Uint32Array with at least 2 elements"), + ) + .throw()); + } + let write_result = write_result_slice.as_mut_ptr(); let write_callback = validators::validate_function(global, "writeCallback", arguments.ptr[5])?; // Bind the ArrayBuffer view to a local so the borrowed byte_slice() outlives @@ -155,7 +177,17 @@ mod _impl { let dictionary = if arguments.ptr[6].is_undefined() { None } else { - dictionary_buf = arguments.ptr[6].as_array_buffer(global).unwrap(); + let dictionary_value = arguments.ptr[6]; + dictionary_buf = match dictionary_value.as_array_buffer(global) { + Some(buf) => buf, + None => { + return Err(global.throw_invalid_argument_type_value( + "dictionary", + "Buffer, TypedArray, or DataView", + dictionary_value, + )); + } + }; Some(dictionary_buf.byte_slice()) }; diff --git a/src/runtime/node/zlib/NativeZstd.rs b/src/runtime/node/zlib/NativeZstd.rs index 958ddf6d4b31..d3a7aefd4c42 100644 --- a/src/runtime/node/zlib/NativeZstd.rs +++ b/src/runtime/node/zlib/NativeZstd.rs @@ -160,8 +160,18 @@ mod _impl { write_state_value, )); } - self.write_result - .set(Some(write_state.as_u32().as_mut_ptr())); + // `flush_write_result` writes two u32s through this pointer, so the + // caller-supplied array must hold at least 2 elements. + let write_state_slice = write_state.as_u32(); + if write_state_slice.len() < 2 { + return Err(global + .err( + jsc::ErrorCode::INVALID_ARG_VALUE, + format_args!("writeState must be a Uint32Array with at least 2 elements"), + ) + .throw()); + } + self.write_result.set(Some(write_state_slice.as_mut_ptr())); let write_js_callback = validators::validate_function(global, "processCallback", process_callback_value)?; diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 5c4f6cf98bf9..21c8ff2421e1 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -184,6 +184,10 @@ pub struct RequestContext< pub request_body: Option, pub request_body_buf: Vec, pub request_body_content_len: usize, + /// Total bytes forwarded to the request-body `ReadableStream`. The + /// up-front `maxRequestBodySize` check only sees Content-Length, so + /// chunked / H3 bodies consumed as a stream are capped against this. + pub request_body_streamed_len: usize, pub sink: Option>>, pub byte_stream: Option>, @@ -1262,6 +1266,7 @@ where request_body: None, request_body_buf: Vec::new(), request_body_content_len: 0, + request_body_streamed_len: 0, sink: None, byte_stream: None, response_body_readable_stream_ref: readable_stream::Strong::default(), @@ -2282,7 +2287,8 @@ where resp.write_header_int(b"content-length", pair.size as u64); } this.end_without_body(this.should_close_connection()); - this.deref(); + // `end_without_body` released the base ref; the caller + // (`on_s3_size_resolved`) releases the ref taken for the S3 stat. } /// `S3::client::stat` callback shape: `fn(S3StatResult, *mut c_void) -> JsTerminatedResult<()>`. @@ -3566,6 +3572,50 @@ where // we can no longer hold the strong reference from the body value ref. if let Some(readable) = this.request_body_readable_stream_ref.get(global_this) { debug_assert!(this.request_body_buf.is_empty()); + + // Cap streamed bytes against maxRequestBodySize too — the up-front + // check only sees Content-Length (see the buffering branch below). + this.request_body_streamed_len = + this.request_body_streamed_len.saturating_add(chunk.len()); + if this.request_body_streamed_len > server.config().max_request_body_size { + this.resp.expect("infallible: resp bound").clear_on_data(); + this.flags.set_is_waiting_for_request_body(false); + + let _exit = vm.enter_event_loop_scope(); + + // Release the strong stream ref like the `last` arm does, then + // error the stream so a pending or future read rejects instead + // of hanging forever. + let _strong = core::mem::take(&mut this.request_body_readable_stream_ref); + + readable.value.ensure_still_alive(); + if let Some(bytes) = readable.ptr.bytes() { + let mut err = Body::ValueError::Message(BunString::static_( + "Request body exceeded maxRequestBodySize", + )); + let js_err = err.to_js(global_this); + js_err.ensure_still_alive(); + // TODO: properly propagate exception upwards + let _ = bytes.on_data(WebCore::streams::Result::Err( + WebCore::streams::StreamError::JSValue(js_err), + )); + err.reset(); + } + + // Route through the normal end path so this.resp is detached + // and the base ref released (see the buffering branch below). + // SAFETY: FFI handle + if let Some(resp) = this.resp { + if !resp.has_responded() { + this.flags.set_has_written_status(true); + // SAFETY: FFI handle + resp.write_status(b"413 Payload Too Large"); + } + } + this.end_without_body(!HTTP3); + return; + } + let _exit = vm.enter_event_loop_scope(); // `RawSlice` is non-owning; ownership of `chunk` stays with the @@ -3706,6 +3756,11 @@ where // This means we have received part of the body but not the whole thing if !self.request_body_buf.is_empty() { let emptied = core::mem::take(&mut self.request_body_buf); + // Count the drained pre-stream bytes against maxRequestBodySize so + // the streaming-path limit check sees the full body length, not + // just the chunks that arrive after the stream becomes active. + self.request_body_streamed_len = + self.request_body_streamed_len.saturating_add(emptied.len()); let cap = emptied.capacity(); return WebCore::DrainResult::Owned { list: emptied, diff --git a/src/runtime/shell/builtin/rm.rs b/src/runtime/shell/builtin/rm.rs index 0b0abd068709..705ea7f4d134 100644 --- a/src/runtime/shell/builtin/rm.rs +++ b/src/runtime/shell/builtin/rm.rs @@ -1074,7 +1074,11 @@ impl ShellRmTask { ); } - let flags = bun_sys::O::DIRECTORY | bun_sys::O::RDONLY; + // The entry was classified as a directory before this open (readdir + // type, or unlinkat returning EISDIR/EPERM). NOFOLLOW keeps a symlink + // swapped in between classification and open from redirecting the + // recursive delete into an unrelated tree (same as Dir::delete_tree). + let flags = bun_sys::O::DIRECTORY | bun_sys::O::RDONLY | bun_sys::O::NOFOLLOW; let fd = match shell_openat(dirfd, path, flags, 0) { Ok(fd) => fd, Err(e) => match e.get_errno() { diff --git a/src/runtime/shell/builtin/seq.rs b/src/runtime/shell/builtin/seq.rs index 44c3eb651679..e8567d705051 100644 --- a/src/runtime/shell/builtin/seq.rs +++ b/src/runtime/shell/builtin/seq.rs @@ -179,7 +179,15 @@ impl Seq { // TODO(port): verify Rust `{}` f32 formatting matches Zig `{d}`. let _ = write!(&mut out, "{}", current); out.extend_from_slice(sep.slice()); - current += incr; + let next = current + incr; + if next == current { + // f32 rounding can make `current + incr` equal `current` + // (e.g. `seq 1 99999999` saturates at 2^24, or a tiny + // increment relative to `current`). Without this check the + // loop never terminates and `out` grows without bound. + break; + } + current = next; } out.extend_from_slice(term.slice()); diff --git a/src/runtime/socket/uws_jsc.rs b/src/runtime/socket/uws_jsc.rs index 5d5b3be8e316..a798814aa0b5 100644 --- a/src/runtime/socket/uws_jsc.rs +++ b/src/runtime/socket/uws_jsc.rs @@ -129,8 +129,50 @@ pub unsafe extern "C" fn us_socket_buffered_js_write( // pointers (`*uws.us_socket_t` / `*us_socket_stream_buffer_t`) with no uniqueness // assertion, so we mirror that here. - // SAFETY: caller (JSNodeHTTPServerSocket.cpp) guarantees `buffer` is valid for the call; - // borrow is dropped before any JS execution below. + // PERF(port): was stack-fallback (std.heap.stackFallback(16 * 1024)) — profile if hot. + // + // Convert `data`/`encoding` BEFORE materializing the stream buffer into an owning + // `Vec`: the conversion can run arbitrary JS (toString/Symbol.toPrimitive, + // Request/Response body coercion) which can re-enter this function on the same + // socket. Taking the buffer first would leave two owning `Vec`s over the same + // `list_ptr`; the inner call's realloc would free the allocation out from under + // the outer frame (use-after-free). + let node_buffer: BlobOrStringOrBuffer = if data.is_undefined() { + BlobOrStringOrBuffer::StringOrBuffer(StringOrBuffer::EMPTY) + } else { + match BlobOrStringOrBuffer::from_js_with_encoding_value_allow_request_response( + global_object, + data, + encoding, + true, + ) { + Err(_) => return JSValue::ZERO, + Ok(Some(v)) => v, + Ok(None) => { + if !global_object.has_exception() { + let _ = global_object.throw_invalid_argument_type_value( + "data", + "string, buffer, or blob", + data, + ); + } + return JSValue::ZERO; + } + } + }; + + if let BlobOrStringOrBuffer::Blob(ref blob) = node_buffer { + if blob.needs_to_read_file() { + let _ = global_object.throw(format_args!( + "File blob not supported yet in this function." + )); + return JSValue::ZERO; + } + } + + // SAFETY: caller (JSNodeHTTPServerSocket.cpp) guarantees `buffer` is valid for the call. + // No JS executes between here and the `update()` below, so this owning `Vec` is the + // sole owner of `list_ptr` for the remainder of the function. let mut stream_buffer = unsafe { &mut *buffer }.to_stream_buffer(); let mut total_written: usize = 0; @@ -138,40 +180,6 @@ pub unsafe extern "C" fn us_socket_buffered_js_write( // reshaped as a labeled block + post-block cleanup so the side effects run on every // exit path without a scopeguard borrow conflict. let result: JSValue = 'body: { - // PERF(port): was stack-fallback (std.heap.stackFallback(16 * 1024)) — profile if hot. - let node_buffer: BlobOrStringOrBuffer = if data.is_undefined() { - BlobOrStringOrBuffer::StringOrBuffer(StringOrBuffer::EMPTY) - } else { - match BlobOrStringOrBuffer::from_js_with_encoding_value_allow_request_response( - global_object, - data, - encoding, - true, - ) { - Err(_) => break 'body JSValue::ZERO, - Ok(Some(v)) => v, - Ok(None) => { - if !global_object.has_exception() { - let _ = global_object.throw_invalid_argument_type_value( - "data", - "string, buffer, or blob", - data, - ); - } - break 'body JSValue::ZERO; - } - } - }; - - if let BlobOrStringOrBuffer::Blob(ref blob) = node_buffer { - if blob.needs_to_read_file() { - let _ = global_object.throw(format_args!( - "File blob not supported yet in this function." - )); - break 'body JSValue::ZERO; - } - } - let data_slice = node_buffer.slice(); // `us_socket_t` is an `opaque_ffi!` ZST — `opaque_mut` is the safe deref. // No JS executes between here and `JSValue::TRUE/FALSE` below, so the diff --git a/src/runtime/timer/timer_object_internals.rs b/src/runtime/timer/timer_object_internals.rs index 2f8d5b1dfe96..5d499f5eed34 100644 --- a/src/runtime/timer/timer_object_internals.rs +++ b/src/runtime/timer/timer_object_internals.rs @@ -855,7 +855,15 @@ impl TimerObjectInternals { // reclamation is omitted. Correctness is unaffected — the entry is // gone — only the high-watermark capacity lingers. // TODO(port): plumb a `shrink_to_fit` once `ArrayHashMap` grows one. - let _ = map.remove(&self.id); + if map.remove(&self.id).is_none() && kind == Kind::SetInterval { + // A `setTimeout` promoted to a `setInterval` by + // `convert_to_interval()` keeps the entry minted by + // `toPrimitive` in `maps.set_timeout`. Remove it from there + // too, or `remove_timer_by_id` would hand out a dangling + // `*mut EventLoopTimer` after the parent is freed. + // SAFETY: as above. + let _ = unsafe { (*state).timer.maps.set_timeout.remove(&self.id) }; + } } // (d) `setEnableKeepingEventLoopAlive(vm, false)` — without this a diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index 37287a4ba3cc..4ba830d728be 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -3504,6 +3504,48 @@ impl BlobExt for Blob { jsc::JSType::Array | jsc::JSType::DerivedArray => { let mut iter = jsc::JSArrayIterator::init(current, global)?; stack.reserve(iter.len as usize); + + // Decide up front whether processing any part (or any entry + // still pending on `stack`) can re-enter user JS (toString / + // Symbol.toPrimitive / proxy traps / getters) and detach a + // borrowed buffer before `joiner.done()` copies it out. If + // nothing can, typed-array parts are borrowed (`push_static`) + // instead of cloned, which would double peak memory for + // `new Blob(largeChunks)`. Non-fast arrays are conservatively + // treated as able to run user JS. + let mut parts_can_run_js = iter.fast.is_none() || !stack.is_empty(); + if !parts_can_run_js { + let mut prescan = jsc::JSArrayIterator::init(current, global)?; + while let Some(item) = prescan.next()? { + if item.is_undefined_or_null() { + continue; + } + match item.js_type_loose() { + jsc::JSType::String + | jsc::JSType::ArrayBuffer + | jsc::JSType::Int8Array + | jsc::JSType::Uint8Array + | jsc::JSType::Uint8ClampedArray + | jsc::JSType::Int16Array + | jsc::JSType::Uint16Array + | jsc::JSType::Int32Array + | jsc::JSType::Uint32Array + | jsc::JSType::Float16Array + | jsc::JSType::Float32Array + | jsc::JSType::Float64Array + | jsc::JSType::BigInt64Array + | jsc::JSType::BigUint64Array + | jsc::JSType::DataView => {} + jsc::JSType::DOMWrapper + if item.as_class_ref::().is_some() => {} + _ => { + parts_can_run_js = true; + break; + } + } + } + } + while let Some(item) = iter.next()? { if item.is_undefined_or_null() { continue; @@ -3538,7 +3580,13 @@ impl BlobExt for Blob { | jsc::JSType::DataView => { could_have_non_ascii = true; let buf = item.as_array_buffer(global).unwrap(); - joiner.push_static(buf.byte_slice()); + if parts_can_run_js { + // A later part may run user JS that detaches + // or resizes this buffer before `done()`. + joiner.push_cloned(buf.byte_slice()); + } else { + joiner.push_static(buf.byte_slice()); + } continue; } jsc::JSType::Array | jsc::JSType::DerivedArray => { @@ -3549,7 +3597,13 @@ impl BlobExt for Blob { if let Some(blob) = item.as_class_ref::() { could_have_non_ascii = could_have_non_ascii || blob.charset.get() != strings::AsciiStatus::AllAscii; - joiner.push_static(blob.shared_view()); + // A later part may run user JS that drops the + // last ref to this Blob's Store before `done()`. + if parts_can_run_js { + joiner.push_cloned(blob.shared_view()); + } else { + joiner.push_static(blob.shared_view()); + } continue; } else { let sliced = current.to_slice_clone(global)?; @@ -3571,7 +3625,10 @@ impl BlobExt for Blob { if let Some(blob) = current.as_class_ref::() { could_have_non_ascii = could_have_non_ascii || blob.charset.get() != strings::AsciiStatus::AllAscii; - joiner.push_static(blob.shared_view()); + // This arm only handles entries deferred onto the walk + // stack; other pending entries may still run user JS and + // free this Blob's Store before `done()`, so always copy. + joiner.push_cloned(blob.shared_view()); } else { let sliced = current.to_slice_clone(global)?; could_have_non_ascii = could_have_non_ascii || sliced.is_allocated(); diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index b089d9dda437..73a4581c6a7d 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -282,9 +282,18 @@ impl ByteStream { if self.pending.get().state == streams::PendingState::Pending { debug_assert!(self.buffer.get().is_empty()); - // SAFETY: pending_buffer is either dangling+len=0 or points into a live JS - // Uint8Array rooted by `pending_value`. - let pending_buf = unsafe { &mut *self.pending_buffer.get() }; + // Re-derive the destination from the GC-rooted view instead of trusting the + // raw pointer captured at pull time: JS can detach or transfer the backing + // ArrayBuffer between the pull and the data arriving, leaving + // `pending_buffer` dangling. A detached view re-derives to an empty slice. + let global = self.parent_const().global_this(); + let mut pending_view = self + .pending_value + .get() + .get() + .and_then(|view| view.as_array_buffer(global)) + .unwrap_or_default(); + let pending_buf = pending_view.slice_mut(); let to_copy_len = chunk.len().min(pending_buf.len()); let pending_buffer_len = pending_buf.len(); debug_assert!(pending_buf.as_ptr() != chunk.as_ptr()); diff --git a/src/runtime/webcore/FormData.rs b/src/runtime/webcore/FormData.rs index d6a9b82f5b4f..383f855fb145 100644 --- a/src/runtime/webcore/FormData.rs +++ b/src/runtime/webcore/FormData.rs @@ -282,7 +282,11 @@ pub fn to_js_from_multipart_data( ); // PORT NOTE: Zig `defer blob.detach()` — no early returns in // this branch, so call explicitly at scope end. + // `append_blob` dupes the content type, so the copy boxed above + // is solely owned by this stack-local and must be released here + // (Zig stored a borrowed slice and had nothing to free). blob.detach(); + blob.free_content_type(); } else { let value = ZigString::init_utf8( // > Each part whose `Content-Disposition` header does not @@ -433,7 +437,19 @@ pub fn for_each_multipart_entry( && field.content_type.is_empty() && strings::eql_case_insensitive_ascii(key, b"content-type", true) { - field.content_type = subslicer.sub(strings::trim(value, b"; \t")).value(); + let trimmed = strings::trim(value, b"; \t"); + // Only an exact `\r\n` terminates a header line above, so a bare + // CR or LF can survive into the value. Reject anything outside + // printable ASCII so it cannot reach `blob.content_type` and be + // reflected verbatim into outgoing request headers. HTAB stays + // allowed: it is valid optional whitespace inside a field value + // and cannot start a new header line. + if trimmed + .iter() + .all(|&b| b == b'\t' || (0x20..=0x7E).contains(&b)) + { + field.content_type = subslicer.sub(trimmed).value(); + } } } diff --git a/src/shell_parser/parse.rs b/src/shell_parser/parse.rs index b626a983e1f2..0d15d2baea0b 100644 --- a/src/shell_parser/parse.rs +++ b/src/shell_parser/parse.rs @@ -965,6 +965,9 @@ pub use ast as AST; pub struct Parser<'bump> { pub strpool: &'bump [u8], pub tokens: &'bump [Token], + /// Strpool ranges that came from interpolated JS values (`\x08__bunstr_N` + /// refs). See `Lexer::js_string_ranges`. + pub js_string_ranges: &'bump [TextRange], pub alloc: &'bump Bump, pub jsobjs: &'bump mut [JSValue], pub current: u32, @@ -1004,6 +1007,7 @@ impl<'bump> Parser<'bump> { Ok(Parser { strpool: lex_result.strpool, tokens: lex_result.tokens, + js_string_ranges: lex_result.js_string_ranges, alloc: bump, jsobjs, current: 0, @@ -1022,6 +1026,7 @@ impl<'bump> Parser<'bump> { Parser { strpool: self.strpool, tokens: self.tokens, + js_string_ranges: self.js_string_ranges, alloc: self.alloc, // PORT NOTE: reshaped for borrowck — Zig copies the slice value; we move the // exclusive borrow into the subparser and restore it in continue_from_subparser. @@ -1673,6 +1678,12 @@ impl<'bump> Parser<'bump> { if eq_idx == 0 { break 'var_decl None; } + // An `=` that came from an interpolated JS value is data, not + // shell syntax — it must not turn the word into an env + // assignment (e.g. interpolating "LD_PRELOAD=/evil.so"). + if self.is_interpolated_position(txtrng.start + eq_idx) { + break 'var_decl None; + } let label = &txt[..eq_idx as usize]; if !is_valid_var_name(label) { break 'var_decl None; @@ -1932,6 +1943,14 @@ impl<'bump> Parser<'bump> { &self.strpool[range.start as usize..range.end as usize] } + /// Whether the strpool position holds a byte that came from an + /// interpolated JS value (a `\x08__bunstr_N` ref spliced in by the lexer). + fn is_interpolated_position(&self, pos: u32) -> bool { + self.js_string_ranges + .iter() + .any(|r| pos >= r.start && pos < r.end) + } + fn advance(&mut self) -> Token { if !self.is_at_end() { self.current += 1; @@ -2372,6 +2391,7 @@ pub struct LexResult<'bump> { pub errors: &'bump [LexError], pub tokens: &'bump [Token], pub strpool: &'bump [u8], + pub js_string_ranges: &'bump [TextRange], } impl<'bump> LexResult<'bump> { @@ -2477,6 +2497,12 @@ pub struct Lexer<'bump, const ENCODING: StringEncoding> { pub subshell_depth: u32, pub errors: bun_alloc::ArenaVec<'bump, LexError>, + /// Strpool ranges that hold bytes spliced in from interpolated JS values + /// (`\x08__bunstr_N` refs). Interpolated bytes are data, not shell + /// syntax, so the parser must not reinterpret them (e.g. an `=` inside + /// one must not create an env assignment). + pub js_string_ranges: bun_alloc::ArenaVec<'bump, TextRange>, + /// Contains a list of strings we need to escape /// Not owned by this struct pub string_refs: &'bump mut [BunString], @@ -2499,6 +2525,7 @@ impl<'bump, const ENCODING: StringEncoding> Lexer<'bump, ENCODING> { tokens: bun_alloc::ArenaVec::new_in(bump), strpool: bun_alloc::ArenaVec::new_in(bump), errors: bun_alloc::ArenaVec::new_in(bump), + js_string_ranges: bun_alloc::ArenaVec::new_in(bump), word_start: 0, j: 0, delimit_quote: false, @@ -2514,6 +2541,7 @@ impl<'bump, const ENCODING: StringEncoding> Lexer<'bump, ENCODING> { tokens: self.tokens.into_bump_slice(), strpool: self.strpool.into_bump_slice(), errors: self.errors.into_bump_slice(), + js_string_ranges: self.js_string_ranges.into_bump_slice(), } } @@ -2540,6 +2568,10 @@ impl<'bump, const ENCODING: StringEncoding> Lexer<'bump, ENCODING> { strpool: core::mem::replace(&mut self.strpool, bun_alloc::ArenaVec::new_in(bump)), tokens: core::mem::replace(&mut self.tokens, bun_alloc::ArenaVec::new_in(bump)), errors: core::mem::replace(&mut self.errors, bun_alloc::ArenaVec::new_in(bump)), + js_string_ranges: core::mem::replace( + &mut self.js_string_ranges, + bun_alloc::ArenaVec::new_in(bump), + ), in_subshell: Some(kind), subshell_depth: self.subshell_depth + 1, word_start: self.word_start, @@ -2560,6 +2592,10 @@ impl<'bump, const ENCODING: StringEncoding> Lexer<'bump, ENCODING> { self.strpool = core::mem::replace(&mut sublexer.strpool, bun_alloc::ArenaVec::new_in(bump)); self.tokens = core::mem::replace(&mut sublexer.tokens, bun_alloc::ArenaVec::new_in(bump)); self.errors = core::mem::replace(&mut sublexer.errors, bun_alloc::ArenaVec::new_in(bump)); + self.js_string_ranges = core::mem::replace( + &mut sublexer.js_string_ranges, + bun_alloc::ArenaVec::new_in(bump), + ); self.chars = sublexer.chars; self.word_start = sublexer.word_start; @@ -3487,6 +3523,7 @@ impl<'bump, const ENCODING: StringEncoding> Lexer<'bump, ENCODING> { } let start = self.j; self.append_string_to_str_pool(bunstr)?; + self.js_string_ranges.push(TextRange { start, end: self.j }); // Interpolated values are data, not shell syntax. If the value would // begin its Text token with `~`, flush it as a quoted-text token so the // parser does not re-interpret it as tilde expansion. Values that diff --git a/src/spawn/static_pipe_writer.rs b/src/spawn/static_pipe_writer.rs index 0a5cbcbb579d..eb0e1bef6eaf 100644 --- a/src/spawn/static_pipe_writer.rs +++ b/src/spawn/static_pipe_writer.rs @@ -265,6 +265,11 @@ impl StaticPipeWriter

{ std::ptr::from_ref(self) as usize, err ); + // Clear the buffer before detaching: `buffer` aliases `self.source`'s + // storage, and `detach()` frees it. `drain_buffered_data` calls + // on_error() then Parent::on_write(), which would otherwise re-slice + // the freed allocation. + self.buffer = RawSlice::EMPTY; self.source.detach(); // Can't release start()'s +1 here: `drain_buffered_data` calls on_error() then // Parent::on_write(); freeing here would UAF. @@ -276,6 +281,9 @@ impl StaticPipeWriter

{ "StaticPipeWriter(0x{:x}) onClose()", std::ptr::from_ref(self) as usize ); + // `buffer` aliases `self.source`'s storage; clear it before detach() + // frees that storage so no dangling slice survives the close. + self.buffer = RawSlice::EMPTY; self.source.detach(); // SAFETY: `process` is a backref to the owning process, guaranteed alive // for the lifetime of this writer (the process owns/outlives its stdio writers). diff --git a/src/sql/mysql/protocol/NewWriter.rs b/src/sql/mysql/protocol/NewWriter.rs index b226ca9ca8c9..01fafeb39663 100644 --- a/src/sql/mysql/protocol/NewWriter.rs +++ b/src/sql/mysql/protocol/NewWriter.rs @@ -14,6 +14,10 @@ pub trait WriterContext: Copy { fn offset(self) -> usize; fn write(self, bytes: &[u8]) -> Result<(), AnyMySQLError>; fn pwrite(self, bytes: &[u8], offset: usize) -> Result<(), AnyMySQLError>; + /// Discard everything written at or after `offset` (a value previously + /// returned by `offset()`). Used to roll a partially-serialized packet + /// back out of the write buffer when it cannot be framed. + fn truncate(self, offset: usize); } #[derive(Clone, Copy)] @@ -32,6 +36,14 @@ impl Packet { let new_offset = self.ctx.wrapped.offset(); // fix position for packet header let length = new_offset - self.offset - PacketHeader::SIZE; + // The length field is only 24 bits and we don't implement multi-packet + // splitting on the write path; truncating would let the server reparse + // the tail as separate attacker-controlled packets. Roll the partial + // packet back out of the buffer and reject. + if length >= PacketHeader::MAX_PAYLOAD_LENGTH { + self.ctx.wrapped.truncate(self.offset); + return Err(AnyMySQLError::Overflow); + } self.header.length = u32::try_from(length).expect("int cast"); bun_core::scoped_log!(NewWriter, "writing packet header: {}", self.header.length); self.ctx.pwrite(&self.header.encode(), self.offset) diff --git a/src/sql/mysql/protocol/PacketHeader.rs b/src/sql/mysql/protocol/PacketHeader.rs index 5b131349c68e..dd1b2e65896f 100644 --- a/src/sql/mysql/protocol/PacketHeader.rs +++ b/src/sql/mysql/protocol/PacketHeader.rs @@ -8,6 +8,11 @@ pub struct PacketHeader { impl PacketHeader { pub const SIZE: usize = 4; + /// The header's length field is 24 bits. A single packet's payload must be + /// strictly smaller than this; a length of exactly 0xFFFFFF signals a + /// multi-packet continuation. + pub const MAX_PAYLOAD_LENGTH: usize = 0xFF_FF_FF; + pub fn decode(bytes: &[u8]) -> Option { if bytes.len() < 4 { return None; diff --git a/src/sql_jsc/mysql/MySQLConnection.rs b/src/sql_jsc/mysql/MySQLConnection.rs index 0f28e938a8fa..ee367250d356 100644 --- a/src/sql_jsc/mysql/MySQLConnection.rs +++ b/src/sql_jsc/mysql/MySQLConnection.rs @@ -603,7 +603,20 @@ impl MySQLConnection { // Process packet based on connection state match self.status { - ConnectionState::Handshaking => self.handle_handshake(reader)?, + ConnectionState::Handshaking => { + self.handle_handshake(reader)?; + // If the handshake negotiated TLS, the SSLRequest has been sent and + // everything after this packet must arrive over the encrypted channel. + // Any bytes already buffered behind the handshake packet are plaintext + // a man-in-the-middle could have injected (CVE-2021-23222 class), so + // reject them instead of feeding them to the auth/command handlers. + if self.tls_status == TLSStatus::MessageSent { + reader.set_offset_from_start(packet_length); + if !reader.peek().is_empty() { + return Err(AnyMySQLError::UnexpectedPacket); + } + } + } ConnectionState::Authenticating | ConnectionState::AuthenticationAwaitingPk => { self.handle_auth(reader, header_length)? } @@ -1595,6 +1608,12 @@ impl WriterContext for Writer { fn offset(self) -> usize { self.write_buffer().len() as usize } + + fn truncate(self, offset: usize) { + let buffer = self.write_buffer(); + let head = buffer.head as usize; + buffer.byte_list.truncate(head + offset); + } } #[derive(Clone, Copy)] diff --git a/src/sql_jsc/mysql/MySQLValue.rs b/src/sql_jsc/mysql/MySQLValue.rs index 74c30fed6e4f..a53bb6bdaea0 100644 --- a/src/sql_jsc/mysql/MySQLValue.rs +++ b/src/sql_jsc/mysql/MySQLValue.rs @@ -643,6 +643,22 @@ impl DateTime { ) } + /// `from_unix_timestamp`/`gregorian_date` can only represent + /// 1970-01-01T00:00:00Z through 9999-12-31T23:59:59Z (the MySQL DATETIME + /// maximum). Anything outside that window panics on an integer cast, so + /// reject it with a catchable error instead. + fn check_range(ts: i64, global_object: &JSGlobalObject) -> Result<(), any_mysql_error::Error> { + const MAX_DATETIME_UNIX_TIMESTAMP: i64 = 253_402_300_799; + if !(0..=MAX_DATETIME_UNIX_TIMESTAMP).contains(&ts) { + return Err(js_error_to_mysql(global_object.throw_invalid_arguments( + format_args!( + "MySQL DATE/DATETIME value must be between 1970-01-01T00:00:00Z and 9999-12-31T23:59:59Z" + ), + ))); + } + Ok(()) + } + pub fn from_js( value: JSValue, global_object: &JSGlobalObject, @@ -653,6 +669,7 @@ impl DateTime { let total_ms = value.get_unix_timestamp(); let ts: i64 = (total_ms / 1000.0).floor() as i64; let ms: u32 = (total_ms - (ts as f64 * 1000.0)) as u32; + Self::check_range(ts, global_object)?; return Ok(DateTime::from_unix_timestamp(ts, ms * 1000)); } @@ -660,6 +677,7 @@ impl DateTime { let total_ms = value.as_number(); let ts: i64 = (total_ms / 1000.0).floor() as i64; let ms: u32 = (total_ms - (ts as f64 * 1000.0)) as u32; + Self::check_range(ts, global_object)?; return Ok(DateTime::from_unix_timestamp(ts, ms * 1000)); } @@ -680,6 +698,19 @@ pub struct Time { } impl Time { + /// `from_unix_timestamp` stores whole days in a `u32`; negative or + /// oversized values panic on an integer cast. Reject them with a + /// catchable error instead. + fn check_range(ts: i64, global_object: &JSGlobalObject) -> Result<(), any_mysql_error::Error> { + const MAX_TIME_SECONDS: i64 = (u32::MAX as i64) * 86400 + 86399; + if !(0..=MAX_TIME_SECONDS).contains(&ts) { + return Err(js_error_to_mysql(global_object.throw_invalid_arguments( + format_args!("MySQL TIME value is out of range"), + ))); + } + Ok(()) + } + pub fn from_js( value: JSValue, global_object: &JSGlobalObject, @@ -689,11 +720,13 @@ impl Time { let total_ms = value.get_unix_timestamp(); let ts: i64 = (total_ms / 1000.0).floor() as i64; let ms: u32 = (total_ms - (ts as f64 * 1000.0)) as u32; + Self::check_range(ts, global_object)?; Ok(Time::from_unix_timestamp(ts, ms * 1000)) } else if value.is_number() { let total_ms = value.as_number(); let ts: i64 = (total_ms / 1000.0).floor() as i64; let ms: u32 = (total_ms - (ts as f64 * 1000.0)) as u32; + Self::check_range(ts, global_object)?; Ok(Time::from_unix_timestamp(ts, ms * 1000)) } else { Err(js_error_to_mysql(global_object.throw_invalid_arguments( diff --git a/src/sql_jsc/mysql/protocol/ResultSet.rs b/src/sql_jsc/mysql/protocol/ResultSet.rs index 6168fef15267..f3ecc335caaa 100644 --- a/src/sql_jsc/mysql/protocol/ResultSet.rs +++ b/src/sql_jsc/mysql/protocol/ResultSet.rs @@ -312,7 +312,11 @@ impl<'a> Row<'a> { for (index, value) in cells.iter_mut().enumerate() { if let Some(result) = decode_length_int(reader.peek()) { let column = &self.columns[index]; - if result.value == 0xfb { + // The NULL marker is the single literal byte 0xfb. A 251-byte + // value is length-encoded as `0xfc 0xfb 0x00` and also decodes + // to value 251, so the marker must be distinguished by its + // 1-byte encoding or row decoding desynchronizes. + if result.bytes_read == 1 && result.value == 0xfb { // NULL value reader.skip(result.bytes_read); // this dont matter if is raw because we will sent as null too like in postgres diff --git a/src/sql_jsc/postgres/DataCell.rs b/src/sql_jsc/postgres/DataCell.rs index 963e082b5a54..e15a40b34238 100644 --- a/src/sql_jsc/postgres/DataCell.rs +++ b/src/sql_jsc/postgres/DataCell.rs @@ -447,6 +447,7 @@ fn parse_array( slice = try_slice(slice, 5); continue; } + return Err(AnyPostgresError::UnsupportedArrayFormat); } else { array.push(SQLDataCell { tag: Tag::Bool, @@ -472,6 +473,7 @@ fn parse_array( slice = try_slice(slice, 4); continue; } + return Err(AnyPostgresError::UnsupportedArrayFormat); } else { array.push(SQLDataCell { tag: Tag::Bool, diff --git a/src/url/lib.rs b/src/url/lib.rs index c9020016e310..832eb091f3b6 100644 --- a/src/url/lib.rs +++ b/src/url/lib.rs @@ -1042,7 +1042,10 @@ impl QueryStringMap { debug_assert!(count > 0); // We should not call initWithScanner when there are no path params - while let Some(result) = scanner.query.next() { + while count < MAX_QUERY_STRING_PARAMS { + let Some(result) = scanner.query.next() else { + break; + }; if result.name_needs_decoding || result.value_needs_decoding { nothing_needs_decoding = false; } @@ -1054,7 +1057,7 @@ impl QueryStringMap { return Ok(None); } - list.reserve(count); // PERF(port): was ensureTotalCapacity + list.reserve(count.min(MAX_QUERY_STRING_PARAMS)); // PERF(port): was ensureTotalCapacity scanner.reset(); // this over-allocates @@ -1063,6 +1066,9 @@ impl QueryStringMap { let mut buf_writer_pos: u32 = 0; while let Some(result) = scanner.pathname.next() { + if list.len() >= MAX_QUERY_STRING_PARAMS { + break; + } let mut name = result.name; let mut value = result.value; let name_slice = result.raw_name(scanner.pathname.routename); @@ -1095,6 +1101,9 @@ impl QueryStringMap { let route_parameter_begin = list.len(); while let Some(result) = scanner.query.next() { + if list.len() >= MAX_QUERY_STRING_PARAMS { + break; + } let mut name = result.name; let mut value = result.value; let name_hash: u64; @@ -1169,7 +1178,10 @@ impl QueryStringMap { let mut estimated_str_len: usize = 0; let mut nothing_needs_decoding = true; - while let Some(result) = scanner.next() { + while count < MAX_QUERY_STRING_PARAMS { + let Some(result) = scanner.next() else { + break; + }; if result.name_needs_decoding || result.value_needs_decoding { nothing_needs_decoding = false; } @@ -1187,6 +1199,9 @@ impl QueryStringMap { if nothing_needs_decoding { scanner = Scanner::init(query_string); while let Some(result) = scanner.next() { + if list.len() >= MAX_QUERY_STRING_PARAMS { + break; + } debug_assert!(!result.name_needs_decoding); debug_assert!(!result.value_needs_decoding); @@ -1216,6 +1231,9 @@ impl QueryStringMap { // PORT NOTE: reshaped for borrowck — Zig captured `list.slice()` once outside // the loop; here we re-slice per iteration to avoid holding a borrow across push(). while let Some(result) = scanner.next() { + if list.len() >= MAX_QUERY_STRING_PARAMS { + break; + } let mut name = result.name; let mut value = result.value; let name_hash: u64; @@ -1275,12 +1293,15 @@ impl QueryStringMap { } } -// Assume no query string param map will exceed 2048 keys // Browsers typically limit URL lengths to around 64k // PORT NOTE: Zig `StaticBitSet(2048)` resolves to `ArrayBitSet(usize, 2048)`. // bun_collections::StaticBitSet currently aliases IntegerBitSet (≤64 bits), so // pick ArrayBitSet directly. 2048 / 64 == 32 masks. -type VisitedMap = ArrayBitSet<2048, { num_masks_for(2048) }>; +/// Hard cap on parsed query-string parameters, enforced in `init` / +/// `init_with_scanner` so the fixed-size `VisitedMap` bitset is never indexed +/// out of bounds. +const MAX_QUERY_STRING_PARAMS: usize = 2048; +type VisitedMap = ArrayBitSet; pub struct Iterator<'a> { pub i: usize, @@ -1295,6 +1316,7 @@ pub struct IteratorResult<'a, 't> { impl<'a> Iterator<'a> { pub fn init(map: &'a QueryStringMap) -> Iterator<'a> { + debug_assert!(map.list.len() <= MAX_QUERY_STRING_PARAMS); Iterator { i: 0, map, @@ -1307,7 +1329,7 @@ impl<'a> Iterator<'a> { where 'a: 't, { - while self.visited.is_set(self.i) { + while self.i < self.map.list.len() && self.visited.is_set(self.i) { self.i += 1; } if self.i >= self.map.list.len() { diff --git a/src/uws/lib.rs b/src/uws/lib.rs index ac7985d9e361..46eb31f90ed9 100644 --- a/src/uws/lib.rs +++ b/src/uws/lib.rs @@ -238,11 +238,22 @@ pub mod ssl_wrapper { /// writes we loop until we have no more data to write/backpressure. const BUFFER_SIZE: usize = 65536; + /// Cap on peer-initiated TLS renegotiations per + /// [`MAX_RENEGOTIATION_WINDOW`]. Mirrors the `us_reneg_policy` defaults in + /// the uSockets C path (openssl.c) and Node's + /// `CLIENT_RENEG_LIMIT`/`CLIENT_RENEG_WINDOW`. Unbounded renegotiation is + /// a CPU DoS (CVE-2011-1473). + const MAX_RENEGOTIATIONS: u8 = 3; + /// See [`MAX_RENEGOTIATIONS`]. + const MAX_RENEGOTIATION_WINDOW: core::time::Duration = core::time::Duration::from_secs(600); + pub struct SSLWrapper { pub handlers: Handlers, pub ssl: Option>, pub ctx: Option>, pub flags: Flags, + pub renegotiation_count: u8, + pub renegotiation_window_start: Option, } /// CamelCase alias for callers that imported the Zig name through the @@ -510,6 +521,8 @@ pub mod ssl_wrapper { flags, ctx: Some(ctx), ssl: Some(ssl), + renegotiation_count: 0, + renegotiation_window_start: None, }) } @@ -980,8 +993,26 @@ pub mod ssl_wrapper { if err == boring_sys::SSL_ERROR_WANT_RENEGOTIATE { self.flags .set_handshake_state(HandshakeState::HandshakeRenegotiationPending); + // An over-limit renegotiation request is treated + // like a failed SSL_renegotiate(). The count + // resets each MAX_RENEGOTIATION_WINDOW, matching + // the C path's `us_reneg_policy`. + let now = std::time::Instant::now(); + match self.renegotiation_window_start { + Some(start) + if now.duration_since(start) < MAX_RENEGOTIATION_WINDOW => {} + _ => { + self.renegotiation_window_start = Some(now); + self.renegotiation_count = 0; + } + } + let renegotiation_allowed = + self.renegotiation_count < MAX_RENEGOTIATIONS; + self.renegotiation_count = self.renegotiation_count.saturating_add(1); // SAFETY: ssl is still valid. - if unsafe { boring_sys::SSL_renegotiate(ssl.as_ptr()) } == 0 { + let renegotiated = renegotiation_allowed + && unsafe { boring_sys::SSL_renegotiate(ssl.as_ptr()) } != 0; + if !renegotiated { self.flags .set_handshake_state(HandshakeState::HandshakeCompleted); // we failed to renegotiate diff --git a/test/bundler/esbuild/packagejson.test.ts b/test/bundler/esbuild/packagejson.test.ts index bbb41c51bf77..0bc572881c4e 100644 --- a/test/bundler/esbuild/packagejson.test.ts +++ b/test/bundler/esbuild/packagejson.test.ts @@ -1987,3 +1987,29 @@ describe("bundler", () => { nodePaths: ["/usr/lib/pkg", "/lib/pkg", "/var/lib/pkg", "/tmp/pkg"], }); }); + +describe("bundler", () => { + // The wildcard match taken from the import specifier (the part substituted for "*" + // in an exports pattern) must be validated for ".", ".." and "node_modules" segments, + // including percent-encoded spellings, exactly like Node's PACKAGE_TARGET_RESOLVE does. + // "%2e%2e" percent-decodes to ".." after substitution, so without that validation the + // resolved target walks out of the package directory and loads /Users/user/project/escaped.js. + itBundled("packagejson/ExportsWildcardRejectsParentDirectorySegments", { + files: { + "/Users/user/project/src/entry.js": /* js */ ` + import 'pkg1/inside.js' + import 'pkg1/%2e%2e/%2e%2e/escaped.js' + `, + "/Users/user/project/node_modules/pkg1/package.json": `{ "exports": { "./*": "./*" } }`, + "/Users/user/project/node_modules/pkg1/inside.js": `console.log('SUCCESS')`, + // Lives outside the package directory. It must never be reachable through pkg1's + // exports map; if the wildcard subpath is substituted unvalidated, resolution lands here. + "/Users/user/project/escaped.js": `console.log('FAILURE')`, + }, + bundleErrors: { + "/Users/user/project/src/entry.js": [ + `Could not resolve: "pkg1/%2e%2e/%2e%2e/escaped.js". Maybe you need to "bun install"?`, + ], + }, + }); +}); diff --git a/test/cli/install/bun-install-registry.test.ts b/test/cli/install/bun-install-registry.test.ts index f3ba30a27fc5..67ae7845fc25 100644 --- a/test/cli/install/bun-install-registry.test.ts +++ b/test/cli/install/bun-install-registry.test.ts @@ -8722,3 +8722,79 @@ registry = "http://localhost:${port}/" }); } }); + +test("rejects dependency aliases containing relative path segments", async () => { + // A dependency alias is used verbatim as a folder name when building install + // paths (`node_modules//node_modules/...`). `one-fixed-dep@2.0.0` + // depends on `no-deps@2.0.0`, which conflicts with the root `no-deps@1.0.0` + // and therefore has to nest underneath the aliased folder. With the alias + // below, that nested install destination would resolve to + // `/escaped-target/node_modules/no-deps`, outside of + // `node_modules`. + await write( + packageJson, + JSON.stringify({ + name: "foo", + version: "1.0.0", + dependencies: { + "no-deps": "1.0.0", + "../escaped-target": "npm:one-fixed-dep@2.0.0", + }, + }), + ); + + let { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + + let err = await stderr.text(); + await stdout.text(); + expect(err).toContain('Invalid dependency name "../escaped-target"'); + // Nothing may be created outside of `node_modules`. `node_modules/../escaped-target` + // resolves to a sibling of `node_modules` inside the project directory. + expect(await exists(join(packageDir, "escaped-target"))).toBe(false); + expect(await exited).not.toBe(0); + + // The same dependency graph with a well-formed alias still installs, and the + // conflicting transitive dependency nests under the aliased folder. + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + await rm(join(packageDir, "bun.lockb"), { force: true }); + await rm(join(packageDir, "bun.lock"), { force: true }); + await write( + packageJson, + JSON.stringify({ + name: "foo", + version: "1.0.0", + dependencies: { + "no-deps": "1.0.0", + "escaped-target": "npm:one-fixed-dep@2.0.0", + }, + }), + ); + + ({ stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + })); + + err = await stderr.text(); + await stdout.text(); + expect(err).not.toContain("error:"); + expect( + await file(join(packageDir, "node_modules", "escaped-target", "node_modules", "no-deps", "package.json")).json(), + ).toMatchObject({ name: "no-deps", version: "2.0.0" }); + expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toMatchObject({ + name: "no-deps", + version: "1.0.0", + }); + expect(await exited).toBe(0); +}); diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 71c5a3b0b056..360754b60281 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -9172,3 +9172,43 @@ describe.concurrent("bun-install", () => { }); }); }); + +it("rejects dependency aliases containing '..' path segments", async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, dummyRegistryForContext(ctx, urls, { "0.0.3": {} })); + // The alias (the key in `dependencies`) becomes the folder name under + // node_modules/. An alias containing ".." segments must not be able to + // place the package outside the project directory. The name is unique per + // run so a previous (vulnerable) run's escape artifact can't fail this one. + const escapeName = + "bun-install-alias-escape-target-" + Date.now().toString(36) + Math.random().toString(36).slice(2); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { + [`../../${escapeName}`]: "npm:baz@0.0.3", + }, + }), + ); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const err = await stderr.text(); + const out = await stdout.text(); + // node_modules/../../ resolves to a sibling of the project + // directory; nothing may be materialized there. + expect(await exists(join(ctx.package_dir, "..", escapeName))).toBe(false); + // The alias is reported as invalid instead of being used as a path. + expect(err).toContain("Invalid dependency name"); + expect(out).not.toContain("1 package installed"); + expect(await exited).toBe(1); + }); +}); diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index 1d8183822eac..cb01e1d157c0 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -615,3 +615,92 @@ it("should include unused resolutions in the lockfile", async () => { // --frozen-lockfile works await runBunInstall(env, packageDir, { frozenLockfile: true }); }); + +it("requires an integrity hash when an npm package entry points at a tarball URL outside the configured registry", async () => { + const { packageDir, packageJson } = await registry.createTestDir(); + + // Stand-in for a host that is not the configured registry. A correct install + // must never contact it for this lockfile, because the entry carries no + // integrity hash that would pin the tarball contents. + let offRegistryRequests = 0; + await using offRegistry = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch() { + offRegistryRequests++; + return new Response("not found", { status: 404 }); + }, + }); + + await write( + packageJson, + JSON.stringify({ + name: "redirected-tarball-url", + dependencies: { + "no-deps": "1.0.0", + }, + }), + ); + + const lockfileWithUrl = (tarballUrl: string) => + JSON.stringify({ + lockfileVersion: 1, + configVersion: 1, + workspaces: { + "": { + name: "redirected-tarball-url", + dependencies: { + "no-deps": "1.0.0", + }, + }, + }, + packages: { + "no-deps": ["no-deps@1.0.0", tarballUrl, {}, ""], + }, + }); + + // The entry keeps the well-known name and version but points the tarball at + // a different host and provides no integrity hash. + await write( + join(packageDir, "bun.lock"), + lockfileWithUrl(`http://127.0.0.1:${offRegistry.port}/no-deps/-/no-deps-1.0.0.tgz`), + ); + + let { exited, stdout, stderr } = spawn({ + cmd: [bunExe(), "install", "--frozen-lockfile"], + cwd: packageDir, + env, + stdout: "pipe", + stderr: "pipe", + }); + + let [out, err] = await Promise.all([stdout.text(), stderr.text()]); + expect(err).toContain( + "Missing integrity hash for npm package resolved to a tarball URL outside the configured registry", + ); + expect(offRegistryRequests).toBe(0); + expect(await exists(join(packageDir, "node_modules", "no-deps"))).toBe(false); + expect(await exited).not.toBe(0); + + // The same entry with the tarball URL under the configured registry and no + // integrity hash is still accepted (backward compat for lockfiles that omit + // the hash for registry-hosted tarballs). + await write(join(packageDir, "bun.lock"), lockfileWithUrl(`${registry.registryUrl()}no-deps/-/no-deps-1.0.0.tgz`)); + + ({ exited, stdout, stderr } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + env, + stdout: "pipe", + stderr: "pipe", + })); + + [out, err] = await Promise.all([stdout.text(), stderr.text()]); + expect(err).not.toContain("Missing integrity hash"); + expect(offRegistryRequests).toBe(0); + expect(await exited).toBe(0); + expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toMatchObject({ + name: "no-deps", + version: "1.0.0", + }); +}); diff --git a/test/cli/install/bun-lockb.test.ts b/test/cli/install/bun-lockb.test.ts index be17176a3bf7..0ea87cc0c90a 100644 --- a/test/cli/install/bun-lockb.test.ts +++ b/test/cli/install/bun-lockb.test.ts @@ -175,3 +175,85 @@ it("recovers from a corrupted binary lockfile instead of panicking", async () => expect(await exists(join(packageDir, "node_modules", "no-deps"))).toBe(true); expect(await exists(join(packageDir, "node_modules", "a-dep"))).toBe(true); }); + +it("rejects a binary lockfile whose patched-dependency flag byte is out of range", async () => { + const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: false } }); + + // `optional-peer-deps@1.0.0` from the local registry, patched via + // `patchedDependencies` so the lockfile gains a `pAtChEdD` section. + const patch = `diff --git a/package.json b/package.json +index d156130662798530e852e1afaec5b1c03d429cdc..b4ddf35975a952fdaed99f2b14236519694f850d 100644 +--- a/package.json ++++ b/package.json +@@ -1,6 +1,7 @@ + { + "name": "optional-peer-deps", + "version": "1.0.0", ++ "hi": true, + "peerDependencies": { + "no-deps": "*" + }, +`; + + await write( + packageJson, + JSON.stringify({ + name: "patched-lockb", + version: "1.0.0", + dependencies: { + "optional-peer-deps": "1.0.0", + }, + patchedDependencies: { + "optional-peer-deps@1.0.0": "patches/optional-peer-deps@1.0.0.patch", + }, + }), + ); + await write(join(packageDir, "patches", "optional-peer-deps@1.0.0.patch"), patch); + + await runBunInstall(env, packageDir); + const lockbPath = join(packageDir, "bun.lockb"); + expect(await exists(lockbPath)).toBe(true); + + // A valid lockfile with a patched dependency still loads cleanly + // (runBunInstall asserts no "error:" / "warn:" on stderr). + await runBunInstall(env, packageDir, { savesLockfile: false }); + + // The patched-dependencies section is the `pAtChEdD` tag followed by two + // arrays, each stored as [start: u64][end: u64][type-name prefix][padding] + // [data] where start/end are absolute file offsets. The second array holds + // 24-byte PatchedDep records laid out as: + // path (8) | padding (7) | patchfile_hash_is_null (1) | patchfile_hash (8) + const lockb = Buffer.from(await file(lockbPath).arrayBuffer()); + const tagOff = lockb.indexOf("pAtChEdD"); + expect(tagOff).toBeGreaterThan(0); + const hashesEnd = Number(lockb.readBigUInt64LE(tagOff + 16)); + const depsStart = Number(lockb.readBigUInt64LE(hashesEnd)); + const depsEnd = Number(lockb.readBigUInt64LE(hashesEnd + 8)); + expect(depsEnd - depsStart).toBe(24); + // Sanity: the flag byte of the only entry is currently a valid bool + // (0 = patchfile hash present). + expect(lockb[depsStart + 15]).toBe(0); + // Any value other than 0 or 1 is not a valid bool and must be rejected by + // the parser, never reinterpreted. + lockb[depsStart + 15] = 0x42; + await write(lockbPath, lockb); + + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--no-progress"], + cwd: packageDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [out, rawErr, code] = await Promise.all([stdout.text(), stderr.text(), exited]); + const err = stderrForInstall(rawErr); + + // The out-of-range flag byte must fail lockfile parsing so the install + // falls back to a fresh resolve instead of consuming the bad byte. + expect(err).toContain("Ignoring lockfile"); + expect(out).toContain("optional-peer-deps@1.0.0"); + expect(code).toBe(0); + expect(await exists(join(packageDir, "node_modules", "optional-peer-deps"))).toBe(true); +}); diff --git a/test/cli/install/isolated-install.test.ts b/test/cli/install/isolated-install.test.ts index 76cee93e8459..d2565a0451aa 100644 --- a/test/cli/install/isolated-install.test.ts +++ b/test/cli/install/isolated-install.test.ts @@ -1993,3 +1993,36 @@ describe("global virtual store", () => { expect(await file(edited).text()).toBe("module.exports = 'USER_EDITS';\n"); }); }); + +test("rejects dependency aliases that traverse outside node_modules", async () => { + const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } }); + + // A (transitively) malicious package.json can use an arbitrary string as a + // dependency alias. The alias becomes a `node_modules/` path + // component in the isolated store layout, so a `..` segment lets it plant + // symlinks outside of node_modules. + await write( + packageJson, + JSON.stringify({ + name: "test-pkg-unsafe-alias", + dependencies: { + "../pwned-by-alias": "npm:no-deps@1.0.0", + }, + }), + ); + + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + + expect(stderr).toContain("is not a valid install folder name"); + // Nothing may be created outside of node_modules. `lstatSync` instead of + // `existsSync` because the escaped artifact would be a dangling symlink. + expect(() => lstatSync(join(packageDir, "pwned-by-alias"))).toThrow(); + expect(exitCode).not.toBe(0); +}); diff --git a/test/cli/install/migration/migrate.test.ts b/test/cli/install/migration/migrate.test.ts index 369aa84f0fc9..32929499de6d 100644 --- a/test/cli/install/migration/migrate.test.ts +++ b/test/cli/install/migration/migrate.test.ts @@ -164,3 +164,56 @@ for (const lockfile of lockfiles) { ).toEqual([true, false]); }); } + +test("npm lockfile migration skips extraneous packages that also declare inBundle: false", async () => { + // A package entry carrying both `"inBundle": false` and `"extraneous": true` must be + // excluded from every migration pass. The counting pass skips it (so its dependencies + // are never reserved); the building and linking passes must apply the exact same + // predicate, otherwise they append more package/dependency entries than were counted. + const phantomDependencies: Record = {}; + for (let i = 0; i < 200; i++) { + phantomDependencies[`phantom-dep-${i}`] = "1.0.0"; + } + + const testDir = tempDirWithFiles("migrate-extraneous-inbundle", { + "package.json": JSON.stringify({ + name: "extraneous-test", + workspaces: ["packages/pkg0"], + }), + "packages/pkg0/package.json": JSON.stringify({ name: "pkg0" }), + "package-lock.json": JSON.stringify({ + name: "extraneous-test", + lockfileVersion: 3, + requires: true, + packages: { + "": { + name: "extraneous-test", + workspaces: ["packages/pkg0"], + }, + "node_modules/pkg0": { + resolved: "packages/pkg0", + link: true, + }, + "packages/pkg0": {}, + "node_modules/not-actually-installed": { + version: "1.0.0", + inBundle: false, + extraneous: true, + dependencies: phantomDependencies, + }, + }, + }), + }); + + const { exitCode, stderr } = Bun.spawnSync([bunExe(), "install"], { + env: bunEnv, + cwd: testDir, + }); + + const err = stderr.toString(); + expect(err).toContain("migrated lockfile from package-lock.json"); + expect(err).not.toContain("InvalidNPMLockfile"); + expect(exitCode).toBe(0); + expect(await Bun.file(join(testDir, "node_modules", "pkg0", "package.json")).json()).toEqual({ name: "pkg0" }); + expect(fs.existsSync(join(testDir, "bun.lock"))).toBeTrue(); +}); diff --git a/test/cli/install/symlink-path-traversal.test.ts b/test/cli/install/symlink-path-traversal.test.ts index 2d06e73937c2..96df3e1c1a23 100644 --- a/test/cli/install/symlink-path-traversal.test.ts +++ b/test/cli/install/symlink-path-traversal.test.ts @@ -1,7 +1,9 @@ import { spawn } from "bun"; import { describe, expect, it, setDefaultTimeout } from "bun:test"; -import { access, lstat, readlink, rm, writeFile } from "fs/promises"; +import { access, lstat, readdir, readlink, rm, writeFile } from "fs/promises"; import { bunExe, bunEnv as env, tempDir } from "harness"; +import { createHash } from "node:crypto"; +import { createServer } from "node:http"; import { tmpdir } from "os"; import { join } from "path"; @@ -133,6 +135,136 @@ const isWindows = process.platform === "win32"; describe.concurrent.skipIf(isWindows)("symlink path traversal protection", () => { setDefaultTimeout(60000); + it("rejects symlink targets that climb above the package root before re-entering a 'packages' directory (streaming extraction)", async () => { + // The streaming extractor used to validate symlink targets by joining + // them onto a fake absolute root ("/packages/") and checking the prefix + // of the normalized result. POSIX normalization clamps excess ".." at + // "/", so a target of the form "(../)+packages/" normalized back + // under the fake root and passed the check, while the kernel resolves + // the raw ".." components from the symlink's real on-disk location and + // lands outside the extraction directory. Such targets must be rejected. + const escapeTarget = "../../../../packages/escape-target"; + + // Incompressible padding so the tarball body is delivered over many + // socket reads; the streaming extractor only takes over when the body + // arrives in multiple chunks. + let pad = ""; + let seed = "streaming-symlink-pad"; + while (pad.length < 256 * 1024) { + seed = createHash("sha256").update(seed).digest("hex"); + pad += seed; + } + + const tarball = createTarball([ + { name: "test-package/", type: "dir" }, + { + name: "test-package/package.json", + type: "file", + content: JSON.stringify({ name: "test-package", version: "1.0.0" }), + }, + { name: "test-package/escape-link", type: "symlink", linkname: escapeTarget }, + { name: "test-package/pad.bin", type: "file", content: pad }, + ]); + + // node:http rather than Bun.serve so the response carries an explicit + // Content-Length *and* can be drip-fed; each write is its own packet so + // the install's HTTP client sees multiple progress callbacks and commits + // to the streaming extractor. + const httpServer = createServer((req, res) => { + const url = new URL(req.url!, "http://localhost"); + if (url.pathname.includes("/tarball/")) { + res.setHeader("Content-Type", "application/gzip"); + res.setHeader("Content-Length", String(tarball.length)); + req.socket.setNoDelay(true); + let offset = 0; + const step = () => { + if (offset >= tarball.length) { + res.end(); + return; + } + res.write(Buffer.from(tarball.subarray(offset, Math.min(offset + 1024, tarball.length)))); + offset += 1024; + setImmediate(step); + }; + step(); + return; + } + if (url.pathname.includes("/repos/")) { + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify({ default_branch: "main" })); + return; + } + res.statusCode = 404; + res.end("Not Found"); + }); + await new Promise(resolve => httpServer.listen(0, "127.0.0.1", () => resolve())); + const port = (httpServer.address() as { port: number }).port; + + try { + using dir = tempDir("streaming-symlink-target-test", {}); + const installDir = String(dir); + + await writeFile( + join(installDir, "package.json"), + JSON.stringify({ + name: "test-app", + version: "1.0.0", + dependencies: { "test-package": "github:user/repo#main" }, + }), + ); + + const proc = spawn({ + cmd: [bunExe(), "install", "--verbose"], + cwd: installDir, + stdout: "pipe", + stderr: "pipe", + env: { + ...env, + GITHUB_API_URL: `http://127.0.0.1:${port}`, + BUN_INSTALL_CACHE_DIR: join(installDir, ".bun-cache"), + // Lower the streaming threshold so this tarball qualifies without + // having to be multiple megabytes. + BUN_INSTALL_STREAMING_MIN_SIZE: "1024", + }, + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + // Confirm the streaming extractor actually handled this tarball; if the + // buffered fallback ran instead this test would not be exercising the + // streaming symlink validation at all. + expect(stderr).toContain("Streamed "); + + if (exitCode !== 0) { + console.error("Install failed with exit code:", exitCode); + console.error("stdout:", stdout); + console.error("stderr:", stderr); + } + expect(exitCode).toBe(0); + + // No symlink anywhere under the install root (node_modules and the + // package cache included) may point at the escaping target. + const escapingSymlinks: string[] = []; + for (const entry of await readdir(installDir, { recursive: true, withFileTypes: true })) { + if (!entry.isSymbolicLink()) continue; + const linkPath = join(entry.parentPath, entry.name); + const target = await readlink(linkPath); + if (target.includes("escape-target")) { + escapingSymlinks.push(`${linkPath} -> ${target}`); + } + } + expect(escapingSymlinks).toEqual([]); + + // The legitimate entries are still extracted. + const pkgDir = join(installDir, "node_modules", "test-package"); + await access(join(pkgDir, "package.json")); + await access(join(pkgDir, "pad.bin")); + } finally { + httpServer.closeAllConnections?.(); + await new Promise(resolve => httpServer.close(() => resolve())); + } + }); + it("should skip symlinks with relative path traversal targets", async () => { // This reproduces the exact attack from the security report: // 1. Symlink test-package/symlink-to-tmp -> ../../../../../../../ diff --git a/test/js/bun/crypto/cipheriv-decipheriv.test.ts b/test/js/bun/crypto/cipheriv-decipheriv.test.ts index 3df7282b0f0b..fd9cb7f2733e 100644 --- a/test/js/bun/crypto/cipheriv-decipheriv.test.ts +++ b/test/js/bun/crypto/cipheriv-decipheriv.test.ts @@ -222,3 +222,21 @@ it("should not accept negative authTagLength, or other coercable values", () => }).toThrow(`The property 'options.authTagLength' is invalid. Received `); } }); + +it("should ignore authTagLength for non-authenticated cipher modes", () => { + // aes-128-cbc is not an authenticated mode. Node only retains + // options.authTagLength for authenticated modes, so getAuthTag() must report + // an invalid state here rather than returning a buffer of the requested size. + const cipher = createCipheriv("aes-128-cbc", randomBytes(16), randomBytes(16), { + authTagLength: 4096, + } as any) as CipherGCM; + cipher.update("hi"); + cipher.final(); + expect(() => cipher.getAuthTag()).toThrow(); + + // Authenticated modes still honor a valid authTagLength. + const gcm = createCipheriv("aes-128-gcm", randomBytes(16), randomBytes(12), { authTagLength: 12 }); + gcm.update("hi"); + gcm.final(); + expect(gcm.getAuthTag().length).toBe(12); +}); diff --git a/test/js/bun/http/bun-server.test.ts b/test/js/bun/http/bun-server.test.ts index 1b482f18bb0c..26b8d80e2bd2 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -1389,3 +1389,82 @@ test("should be able to redirect when using empty streams #15320", async () => { const response = await fetch(`http://localhost:${server.port}/redirect`); expect(await response.text()).toBe("Hello, World"); }); + +test("HEAD request for a Response with an S3 file body reports the object size and the server keeps serving", async () => { + // Answering a HEAD request whose Response body is an S3-backed Blob resolves + // the object size with an async S3 stat before writing headers. Run the + // server in a subprocess so a crash on that completion path shows up as a + // non-zero exit code instead of taking down the test runner. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + // Fake S3 origin: answers the stat (HEAD) with a fixed Content-Length. + const s3Origin = Bun.serve({ + port: 0, + fetch(req) { + if (req.method === "HEAD") { + return new Response(null, { + headers: { + "Content-Length": "11", + "ETag": '"abc123"', + "Content-Type": "text/plain", + }, + }); + } + return new Response("Hello World"); + }, + }); + + const s3 = new Bun.S3Client({ + accessKeyId: "test", + secretAccessKey: "test", + region: "us-east-1", + bucket: "my-bucket", + endpoint: s3Origin.url.href, + }); + + const app = Bun.serve({ + port: 0, + fetch(req) { + if (new URL(req.url).pathname === "/health") { + return new Response("alive"); + } + return new Response(s3.file("hello.txt")); + }, + }); + + for (let i = 0; i < 8; i++) { + const res = await fetch(new URL("/object", app.url), { method: "HEAD" }); + if (res.status !== 200) { + throw new Error("unexpected HEAD status: " + res.status); + } + const contentLength = res.headers.get("content-length"); + if (contentLength !== "11") { + throw new Error("unexpected content-length: " + contentLength); + } + await res.arrayBuffer(); + } + + // The request context for each HEAD request above has been released by + // now; a fresh request must still be served off the same pool. + const health = await fetch(new URL("/health", app.url)); + if ((await health.text()) !== "alive") { + throw new Error("server is no longer responding"); + } + + console.log("s3-head-ok"); + process.exit(0); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("s3-head-ok"); + expect(exitCode).toBe(0); +}); diff --git a/test/js/bun/http/proxy.test.ts b/test/js/bun/http/proxy.test.ts index 38d95741c1ef..0c530fe55819 100644 --- a/test/js/bun/http/proxy.test.ts +++ b/test/js/bun/http/proxy.test.ts @@ -1172,3 +1172,73 @@ describe.concurrent("NO_PROXY with explicit proxy option", () => { expect(exitCode).toBe(0); }); }); + +test("non-200 CONNECT response from proxy is surfaced and its Location header is not followed", async () => { + // RFC 9110 §9.3.6: a non-2xx response to CONNECT means the tunnel was not + // established. The proxy's response must be returned to the caller, but a + // Location header on it must never be followed — otherwise the original + // method, body, and custom headers would be re-sent to whatever plaintext + // origin the proxy names. + + // Records anything that reaches the address named in the proxy's Location + // header. Nothing should ever arrive here. + const reachedRedirectTarget: { method: string; apiKey: string | null; body: string }[] = []; + using redirectTarget = Bun.serve({ + port: 0, + async fetch(req) { + reachedRedirectTarget.push({ + method: req.method, + apiKey: req.headers.get("x-api-key"), + body: await req.text(), + }); + return new Response("redirect target reached"); + }, + }); + + // Proxy that refuses the CONNECT with a redirect pointing at the plaintext + // target above, instead of establishing the tunnel. + const proxySockets = new Set(); + const sawConnect: string[] = []; + const proxy = net.createServer(clientSocket => { + proxySockets.add(clientSocket); + clientSocket.on("close", () => proxySockets.delete(clientSocket)); + clientSocket.on("error", () => {}); + clientSocket.once("data", data => { + sawConnect.push(data.toString().split("\r\n")[0]); + clientSocket.write( + "HTTP/1.1 307 Temporary Redirect\r\n" + + `Location: ${redirectTarget.url.origin}/\r\n` + + "Content-Length: 0\r\n" + + "\r\n", + ); + }); + }); + proxy.listen(0); + await once(proxy, "listening"); + const proxyPort = (proxy.address() as net.AddressInfo).port; + + try { + const response = await fetch(httpsServer.url, { + method: "POST", + body: "secret request body", + headers: { "X-Api-Key": "super-secret" }, + proxy: `http://localhost:${proxyPort}`, + keepalive: false, + tls: { ca: tlsCert.cert, rejectUnauthorized: false }, + }); + + // The request did go through the proxy as a CONNECT... + expect(sawConnect.length).toBe(1); + expect(sawConnect[0]!.startsWith("CONNECT ")).toBe(true); + // ...the proxy's refusal is surfaced to the caller as-is... + expect(response.status).toBe(307); + // ...and the Location header on the failed CONNECT is never followed: + // the body and the X-Api-Key header must not reach the plaintext server + // it points at. + expect(reachedRedirectTarget).toEqual([]); + } finally { + for (const s of proxySockets) s.destroy(); + proxy.close(); + await once(proxy, "close"); + } +}); diff --git a/test/js/bun/http/serve-pending-promise-abort-leak.test.ts b/test/js/bun/http/serve-pending-promise-abort-leak.test.ts index b0c0e5acfa04..12b4f132f904 100644 --- a/test/js/bun/http/serve-pending-promise-abort-leak.test.ts +++ b/test/js/bun/http/serve-pending-promise-abort-leak.test.ts @@ -1,5 +1,6 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe } from "harness"; +import { connect } from "node:net"; import { join } from "node:path"; async function waitForPendingRequests(server: ReturnType, expected: number) { @@ -111,6 +112,111 @@ test("streaming 413 detaches the response so a late resolve/reject is a no-op", expect(exitCode).toBe(0); }, 30_000); +test("chunked request body consumed as a ReadableStream is capped at maxRequestBodySize", async () => { + // The up-front maxRequestBodySize check only sees Content-Length, and the + // buffering branch of onBufferedBodyChunk only caps req.text()/.arrayBuffer(). + // A chunked (no Content-Length) body consumed as a ReadableStream goes + // through the streaming branch, which must also count and cap forwarded + // bytes — otherwise a single request streams unbounded data past the limit. + const limit = 1024; + + let streamed = 0; + let streamError = ""; + let firstChunk = Promise.withResolvers(); + let handlerDone = Promise.withResolvers(); + + using server = Bun.serve({ + port: 0, + idleTimeout: 0, + maxRequestBodySize: limit, + async fetch(req) { + streamed = 0; + streamError = ""; + try { + for await (const chunk of req.body!) { + streamed += chunk.byteLength; + firstChunk.resolve(); + } + } catch (e) { + streamError = String((e as Error)?.message ?? e); + } finally { + firstChunk.resolve(); + handlerDone.resolve(); + } + return new Response(String(streamed)); + }, + }); + + // Sends a chunked POST with no Content-Length. Writes one small chunk, + // waits until the handler has started pulling from the stream (so later + // chunks take the streaming branch, not the pre-stream buffer), then + // writes the rest. + async function sendChunked(totalBytes: number): Promise { + firstChunk = Promise.withResolvers(); + handlerDone = Promise.withResolvers(); + + const sock = connect(Number(server.port), "127.0.0.1"); + await new Promise((resolve, reject) => { + sock.on("connect", resolve); + sock.on("error", reject); + }); + // Once the limit trips the server ends the connection while the client is + // still writing chunks; EPIPE/ECONNRESET here is the expected outcome. + sock.removeAllListeners("error"); + sock.on("error", () => {}); + + let received = ""; + const { promise: gotResponse, resolve: doneReceiving } = Promise.withResolvers(); + sock.on("data", d => { + received += d.toString("latin1"); + if (received.includes("\r\n\r\n")) doneReceiving(); + }); + sock.on("close", () => doneReceiving()); + + sock.write( + "POST / HTTP/1.1\r\n" + // + `Host: 127.0.0.1:${server.port}\r\n` + + "Transfer-Encoding: chunked\r\n" + + "\r\n", + ); + + const piece = Buffer.alloc(256, "A").toString("latin1"); + const writeChunk = () => + new Promise(resolve => { + if (sock.destroyed) return resolve(); + sock.write(piece.length.toString(16) + "\r\n" + piece + "\r\n", () => resolve()); + }); + + await writeChunk(); + await firstChunk.promise; + for (let sent = piece.length; sent < totalBytes && !sock.destroyed; sent += piece.length) { + await writeChunk(); + } + if (!sock.destroyed) sock.write("0\r\n\r\n"); + + await handlerDone.promise; + await gotResponse; + sock.destroy(); + return received.split("\r\n")[0]; + } + + // A chunked body under the limit still streams fully to the handler. + const okStatus = await sendChunked(512); + expect(streamError).toBe(""); + expect(streamed).toBe(512); + expect(okStatus).toBe("HTTP/1.1 200 OK"); + + // A chunked body over the limit is rejected: the stream read errors, the + // handler never sees the full payload, and the client gets a 413. + const overflowTotal = limit * 16; + const overflowStatus = await sendChunked(overflowTotal); + expect(overflowStatus).toBe("HTTP/1.1 413 Payload Too Large"); + expect(streamError).toBe("Request body exceeded maxRequestBodySize"); + expect(streamed).toBeLessThan(overflowTotal); + + await waitForPendingRequests(server, 0); +}, 15_000); + test("resolve() after abort does not crash and cleans up", async () => { // UAF safety: while the resolve function is reachable, the Promise stays // alive, the NativePromiseContext cell stays alive, and the RequestContext diff --git a/test/js/bun/http/serve.test.ts b/test/js/bun/http/serve.test.ts index 838857730aa0..2df2f84e0770 100644 --- a/test/js/bun/http/serve.test.ts +++ b/test/js/bun/http/serve.test.ts @@ -2231,3 +2231,66 @@ it.todo("Bun.serve hostname with interior NUL byte does not crash the process", exitCode: 0, }); }); + +// The HTTP parser shares HttpParser.h between Bun.serve and node:http. When a request +// handler tears the connection down from inside the request-body data callback, the +// parser must stop consuming the rest of the TCP segment instead of routing a request +// that was pipelined behind the body onto the already-closed socket. +it("does not dispatch a pipelined request after the connection is destroyed inside the body data callback", async () => { + const script = ` +const http = require("node:http"); +const net = require("node:net"); + +const seen = []; +const server = http.createServer((req, res) => { + seen.push(req.url); + if (req.url === "/first") { + req.on("data", () => { + // Reject the upload: finish the response and tear down the socket, + // synchronously, from inside the request body data callback. + res.writeHead(400); + res.end(); + req.socket.destroy(); + }); + return; + } + res.end("ok"); +}); + +server.listen(0, "127.0.0.1", () => { + const port = server.address().port; + const socket = net.connect(port, "127.0.0.1", () => { + // One TCP segment: a POST with a body, immediately followed by a pipelined GET. + socket.write( + "POST /first HTTP/1.1\\r\\nHost: 127.0.0.1\\r\\nContent-Length: 5\\r\\n\\r\\nhello" + + "GET /second HTTP/1.1\\r\\nHost: 127.0.0.1\\r\\n\\r\\n", + ); + }); + socket.on("error", () => {}); + socket.resume(); + socket.on("close", async () => { + // A fresh connection must still get a normal response afterwards. + const res = await fetch("http://127.0.0.1:" + port + "/after"); + await res.text(); + console.log(JSON.stringify({ seen, after: res.status })); + server.close(); + process.exit(0); + }); +}); +`; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + // "/second" arrived in the same TCP segment as the POST body, after the handler had + // already torn the connection down. It must never reach the request listener. + expect(stdout.trim()).toBe('{"seen":["/first","/after"],"after":200}'); + expect(exitCode).toBe(0); +}); diff --git a/test/js/bun/jsc/bun-jsc.test.ts b/test/js/bun/jsc/bun-jsc.test.ts index d69c8fd093e3..05abd1a9fc5d 100644 --- a/test/js/bun/jsc/bun-jsc.test.ts +++ b/test/js/bun/jsc/bun-jsc.test.ts @@ -24,7 +24,7 @@ import { totalCompileTime, } from "bun:jsc"; import { describe, expect, it } from "bun:test"; -import { isBuildKite, isWindows } from "harness"; +import { bunEnv, bunExe, isBuildKite, isWindows } from "harness"; describe("bun:jsc", () => { function count() { @@ -219,3 +219,89 @@ describe("bun:jsc", () => { expect(result3.stackTraces.traces.length).toBeGreaterThan(0); }); }); + +it("deserialize rejects an object reference index outside the deserialized object pool", async () => { + // A payload whose first value is ObjectReferenceTag must have its pool index + // validated against the number of objects deserialized so far (zero here), + // instead of indexing past the end of the pool. + const script = ` + import { serialize, deserialize } from "bun:jsc"; + // serialize(undefined) is [version header][UndefinedTag]; keep just the header. + const prefix = new Uint8Array(serialize(undefined)); + const header = prefix.subarray(0, prefix.length - 1); + const payload = new Uint8Array([...header, 19 /* ObjectReferenceTag */, 200 /* index into the (empty) object pool */]); + let outcome; + try { + const value = deserialize(payload); + outcome = value === null ? "rejected" : "accepted " + String(value); + } catch (error) { + outcome = error instanceof Error ? "rejected" : "threw non-error"; + } + console.log(outcome); + // A legitimate payload still round-trips. + console.log(JSON.stringify(deserialize(serialize({ a: 1 })))); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toBe('rejected\n{"a":1}\n'); + expect(exitCode).toBe(0); +}); + +it("deserialize rejects a typed array whose backing store is not an array buffer", async () => { + // A serialized ArrayBufferView must be backed by an ArrayBuffer (or a + // reference to one already in the object pool). A payload that nests + // ArrayBufferViewTag inside ArrayBufferViewTag thousands of levels deep must + // be rejected at the first level instead of being followed all the way down. + const script = ` + import { serialize, deserialize } from "bun:jsc"; + // serialize(undefined) is [version header][UndefinedTag]; keep just the header. + const prefix = new Uint8Array(serialize(undefined)); + const header = prefix.subarray(0, prefix.length - 1); + const depth = 200000; + // Each level is: ArrayBufferViewTag (22), Uint8Array subtag (2), + // byteOffset:uint64 = 0, byteLength:uint64 = 0. The next level's tag sits + // where the backing ArrayBuffer is supposed to be. + const unit = new Uint8Array(18); + unit[0] = 22; + unit[1] = 2; + const payload = new Uint8Array(header.length + unit.length * depth); + payload.set(header, 0); + for (let i = 0; i < depth; i++) { + payload.set(unit, header.length + i * unit.length); + } + let outcome; + try { + const value = deserialize(payload); + outcome = value === null ? "rejected" : "accepted " + String(value); + } catch (error) { + outcome = error instanceof Error ? "rejected" : "threw non-error"; + } + console.log(outcome); + // Real typed arrays still round-trip, including two views sharing one + // buffer (the second view's backing store is serialized as a reference + // into the object pool). + const shared = new ArrayBuffer(4); + const first = new Uint8Array(shared); + first.set([1, 2, 3, 4]); + const second = new Uint16Array(shared); + const out = deserialize(serialize({ first, second })); + console.log(out.first instanceof Uint8Array, Array.from(out.first).join(",")); + console.log(out.second instanceof Uint16Array, Array.from(out.second).join(",")); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toBe("rejected\ntrue 1,2,3,4\ntrue 513,1027\n"); + expect(exitCode).toBe(0); +}); diff --git a/test/js/bun/shell/bunshell.test.ts b/test/js/bun/shell/bunshell.test.ts index d00b30a8d70d..df6df37b8421 100644 --- a/test/js/bun/shell/bunshell.test.ts +++ b/test/js/bun/shell/bunshell.test.ts @@ -2729,3 +2729,31 @@ function sentinelByte(buf: Uint8Array): number { } throw new Error("No sentinel byte"); } + +describe("interpolated values in assignment position", () => { + // An `=` that arrives via an interpolated template value is data, not shell + // syntax. The value must remain a single inert command word instead of being + // reinterpreted as an environment-variable assignment applied to the rest of + // the command line. + TestBuilder.command`${"FOO_INJECTED=1"} echo hi` + .exitCode(1) + .stderr("bun: command not found: FOO_INJECTED=1\n") + .runAsTest("interpolated word containing equals stays a single command word"); + + // The assignment-shaped value must not land in a spawned child's environment + // with the following word promoted to the command name. + TestBuilder.command`${"SHELL_TEST_INJECTED=evil"} ${BUN} -e ${"console.log(process.env.SHELL_TEST_INJECTED)"}` + .exitCode(1) + .stderr("bun: command not found: SHELL_TEST_INJECTED=evil\n") + .runAsTest("interpolated word containing equals is not exported to the child environment"); + + // Legitimate uses keep working: a literal `=` in the template source still + // creates an assignment even when its value is interpolated, and an + // interpolated `=` in argument position passes through verbatim. + TestBuilder.command`FOO=${"bar"} ${BUN} -e ${"console.log(process.env.FOO)"}` + .stdout("bar\n") + .runAsTest("literal assignment with interpolated value still works"); + TestBuilder.command`echo ${"a=b"}` + .stdout("a=b\n") + .runAsTest("interpolated equals in argument position passes through"); +}); diff --git a/test/js/bun/shell/commands/rm.test.ts b/test/js/bun/shell/commands/rm.test.ts index f6701c01c780..10b575c4c6af 100644 --- a/test/js/bun/shell/commands/rm.test.ts +++ b/test/js/bun/shell/commands/rm.test.ts @@ -7,7 +7,7 @@ import { $ } from "bun"; import { beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test"; import { tempDirWithFiles } from "harness"; -import { mkdirSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, renameSync, symlinkSync, writeFileSync } from "node:fs"; import path from "path"; import { createTestBuilder, sortedShellOutput } from "../util"; const TestBuilder = createTestBuilder(import.meta.path); @@ -171,3 +171,55 @@ function packagejson() { "version": "0.0.0" }`; } + +// Recursive `rm -rf` classifies each entry as a directory from readdir, then +// later re-opens it by path on a worker thread. If that path is replaced by a +// symlink between classification and open, the open must not follow the link +// into an unrelated tree. Each iteration races a batch of directory->symlink +// swaps against the walker; the file behind the symlink must survive every +// time. The legitimate case (real directories that are not swapped in time) +// is exercised by the same loop: those entries are simply deleted. +test.skipIf(process.platform === "win32")( + "recursive rm does not follow a directory entry replaced by a symlink during deletion", + async () => { + const ENTRIES = 64; + const FILLER = 8; + const ITERATIONS = 10; + + for (let iter = 0; iter < ITERATIONS; iter++) { + const files: Record = { + "victim/keep.txt": "important", + "stash/.keep": "", + }; + for (let i = 0; i < ENTRIES; i++) { + for (let j = 0; j < FILLER; j++) { + files[`target/d${i}/f${j}.txt`] = ""; + } + } + const root = tempDirWithFiles(`rm-swap-${iter}`, files); + const victimDir = path.join(root, "victim"); + const victimFile = path.join(victimDir, "keep.txt"); + const target = path.join(root, "target"); + + // Start the recursive delete on the worker pool, then immediately + // replace each subdirectory with a symlink pointing at the victim + // directory while the walk is in flight. + const running = $`rm -rf ${target}`.nothrow().quiet().run(); + for (let i = 0; i < ENTRIES; i++) { + const entry = path.join(target, `d${i}`); + try { + renameSync(entry, path.join(root, "stash", `d${i}`)); + symlinkSync(victimDir, entry); + } catch { + // The walker may have already deleted this entry; that's fine. + } + } + await running; + + // The contents of the directory behind the symlink must never be + // deleted, no matter when the swap landed relative to the walk. + expect(existsSync(victimFile)).toBeTrue(); + expect(existsSync(victimDir)).toBeTrue(); + } + }, +); diff --git a/test/js/bun/shell/commands/seq.test.ts b/test/js/bun/shell/commands/seq.test.ts index 14b5443af296..fd889c1e2fae 100644 --- a/test/js/bun/shell/commands/seq.test.ts +++ b/test/js/bun/shell/commands/seq.test.ts @@ -108,6 +108,18 @@ describe("seq", async () => { .stdout("") .stderr("seq: needs negative decrement\n") .runAsTest("needs negative decrement"); + + TestBuilder.command`seq 16777216 16777218` + .exitCode(0) + .stdout("16777216\n") + .stderr("") + .runAsTest("terminates when adding the increment no longer changes the value"); + + TestBuilder.command`seq 1 0.00000001 2` + .exitCode(0) + .stdout("1\n") + .stderr("") + .runAsTest("terminates when the increment is too small to advance the accumulator"); }); describe("seq without stdout", async () => { diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js index 32c78f24fe97..659a00f0bdc6 100644 --- a/test/js/bun/sqlite/sqlite.test.js +++ b/test/js/bun/sqlite/sqlite.test.js @@ -1544,3 +1544,96 @@ it("internal SQL helpers reject out-of-range database handles", async () => { exitCode: 0, }); }); + +// Property getters on a bindings object run arbitrary JS in the middle of the +// bind loop. A getter for a later parameter must not be able to (1) change the +// bytes sqlite stores for an earlier blob parameter by mutating/detaching its +// ArrayBuffer after it was bound, or (2) keep the bind/step loop running on a +// statement it just finalized. Run in a subprocess because the unsafe variant +// of (2) operates on a freed sqlite3_stmt. +it("binds blob parameters by copy and rejects statements finalized while binding", async () => { + const src = ` + const { Database } = require("bun:sqlite"); + const out = {}; + + // 1. The getter for $b mutates and detaches the buffer that was already + // bound for $a. The stored blob must be the bytes as they were at bind + // time, not whatever the buffer's memory holds when the query runs. + { + const db = new Database(":memory:"); + db.run("CREATE TABLE t (a BLOB, b INT)"); + const ab = new ArrayBuffer(256); + const u8 = new Uint8Array(ab); + u8.fill(0xab); + db.run("INSERT INTO t VALUES ($a, $b)", { + get $a() { + return u8; + }, + get $b() { + u8.fill(0xee); + ab.transfer(); + return 1; + }, + }); + const row = db.query("SELECT a, b FROM t").get(); + out.blobLength = row.a.length; + out.blobIsOriginal = row.a.every(byte => byte === 0xab); + out.b = row.b; + db.close(); + } + + // 2. A getter that finalizes the statement whose parameters are being + // bound must result in an error, not continued use of the statement. + { + const db = new Database(":memory:"); + const q = db.query("SELECT $x AS x"); + let message = "did not throw"; + try { + q.get({ + get $x() { + q.finalize(); + return 1; + }, + }); + } catch (e) { + message = e.message; + } + out.finalizeDuringBind = message; + out.dbStillWorks = db.query("SELECT 123 AS y").get().y; + db.close(); + } + + // 3. Plain blob binding still round-trips. + { + const db = new Database(":memory:"); + db.run("CREATE TABLE t (a BLOB)"); + db.run("INSERT INTO t VALUES ($a)", { $a: new Uint8Array([1, 2, 3]) }); + out.plainBlob = Array.from(db.query("SELECT a FROM t").get().a); + db.close(); + } + + console.log(JSON.stringify(out)); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", src], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout.trim()).toBe( + JSON.stringify({ + blobLength: 256, + blobIsOriginal: true, + b: 1, + finalizeDuringBind: "Statement has finalized", + dbStillWorks: 123, + plainBlob: [1, 2, 3], + }), + ); + expect(exitCode).toBe(0); +}); diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index 739e2fe63bc2..4a5fa7b96789 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -571,3 +571,124 @@ it("throws a clean error for invalid route filenames (no use-after-free)", async expect(stdout.trim()).toBe("caught:Route is missing a closing bracket]"); expect(exitCode).toBe(0); }); + +it("decodes percent-encoded path segments and keeps params and pathname stable after later matches", async () => { + // The buffer that backs a MatchedRoute's decoded pathname, query string and + // param values must stay alive (and unshared) for as long as the MatchedRoute + // object does. Two back-to-back matches with equal-length encoded segments + // are used so that, if the first match's decode buffer were released or + // shared, the second match would immediately reuse and overwrite it. + // Run in a subprocess so a memory error in the child cannot take down the + // test runner. + using dir = tempDir("fsr-percent-decode", { + "pages/posts/[id].tsx": "export default 1;", + }); + + const code = /* ts */ ` + const router = new Bun.FileSystemRouter({ + dir: ${JSON.stringify(path.join(String(dir), "pages"))}, + style: "nextjs", + fileExtensions: [".tsx"], + }); + const enc = s => [...s].map(c => "%" + c.charCodeAt(0).toString(16).padStart(2, "0")).join(""); + + const a = "alpha-" + "a".repeat(58); + const b = "bravo-" + "b".repeat(58); + const ma = router.match("/posts/" + enc(a)); + const mb = router.match("/posts/" + enc(b)); + if (!ma || !mb) throw new Error("expected both URLs to match"); + if (ma.name !== "/posts/[id]") throw new Error("bad name: " + ma.name); + if (ma.params.id !== a) throw new Error("first param corrupted: " + JSON.stringify(ma.params.id)); + if (ma.pathname !== "/posts/" + a) throw new Error("first pathname corrupted: " + JSON.stringify(ma.pathname)); + if (mb.params.id !== b) throw new Error("second param corrupted: " + JSON.stringify(mb.params.id)); + if (mb.pathname !== "/posts/" + b) throw new Error("second pathname corrupted: " + JSON.stringify(mb.pathname)); + + // Un-encoded URLs must keep working. + const plain = router.match("/posts/hello-world"); + if (!plain || plain.params.id !== "hello-world") throw new Error("plain param: " + JSON.stringify(plain && plain.params.id)); + console.log("ok"); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", code], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ok"); + expect(exitCode).toBe(0); +}); + +it("caps the number of parsed query string parameters instead of crashing", async () => { + // A query string with more parameters than the iterator's fixed-size visited + // bitset (2048 entries) must not be able to take down the process when + // `.query` is read. Run in a subprocess so an abort is observable as output + // on stderr / a nonzero exit code instead of killing the test runner. + using dir = tempDir("fsr-many-query-params", { + "pages/posts.tsx": "export default 1;", + }); + + const code = /* ts */ ` + const router = new Bun.FileSystemRouter({ + dir: ${JSON.stringify(path.join(String(dir), "pages"))}, + style: "nextjs", + fileExtensions: [".tsx"], + }); + const qs = Array.from({ length: 3000 }, (_, i) => "k" + i + "=v" + i).join("&"); + const match = router.match("/posts?" + qs); + if (!match) throw new Error("expected /posts to match"); + const query = match.query; + const keys = Object.keys(query); + if (keys.length < 1 || keys.length > 3000) throw new Error("unexpected key count: " + keys.length); + if (query.k0 !== "v0") throw new Error("first param wrong: " + JSON.stringify(query.k0)); + console.log("ok " + keys.length); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", code], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toMatch(/^ok \d+$/); + expect(exitCode).toBe(0); +}); + +it("does not match a dynamic route whose static segment merely collides on length and 32-bit hash", () => { + // Route segment matching must compare bytes, not just (length, truncated + // 32-bit wyhash). Bun.hash.wyhash(s, 0) is the same hash the router stores + // for static route segments, so a birthday search over a few hundred + // thousand equal-length candidates finds a colliding pair with overwhelming + // probability (expected after ~80k candidates). + const seen = new Map(); + let pair: [string, string] | null = null; + for (let i = 0; i < 600_000; i++) { + const candidate = "s" + i.toString(36).padStart(9, "0"); + const h = Number(BigInt.asUintN(32, BigInt(Bun.hash.wyhash(candidate)))); + const prev = seen.get(h); + if (prev !== undefined) { + pair = [prev, candidate]; + break; + } + seen.set(h, candidate); + } + expect(pair).not.toBeNull(); + const [routeSegment, attackSegment] = pair!; + expect(attackSegment).not.toBe(routeSegment); + expect(attackSegment.length).toBe(routeSegment.length); + + const { dir } = make([`${routeSegment}/[id].tsx`]); + const router = new Bun.FileSystemRouter({ + dir, + style: "nextjs", + }); + + // The genuine segment matches its dynamic route. + expect(router.match(`/${routeSegment}/42`)?.name).toBe(`/${routeSegment}/[id]`); + // A different segment that only collides on (length, 32-bit hash) must not. + expect(router.match(`/${attackSegment}/42`)).toBeNull(); +}); diff --git a/test/js/bun/wasm/wasi.test.js b/test/js/bun/wasm/wasi.test.js index 5e6994a50457..1da7efca6d25 100644 --- a/test/js/bun/wasm/wasi.test.js +++ b/test/js/bun/wasm/wasi.test.js @@ -1,6 +1,9 @@ import { spawnSync } from "bun"; import { expect, it } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; +import fs from "node:fs"; +import path from "node:path"; +import { WASI } from "node:wasi"; it("Should support printing 'hello world'", () => { const { stdout, stderr, exitCode } = spawnSync({ @@ -20,3 +23,57 @@ it("Should support printing 'hello world'", () => { exitCode: 0, }); }); + +it("path_* syscalls cannot escape the preopened directory", () => { + using dir = tempDir("wasi-sandbox", { + "secret.txt": "outside", + "sandbox/inside.txt": "inside", + }); + const root = String(dir); + const sandbox = path.join(root, "sandbox"); + if (!isWindows) { + // a symlink that already exists inside the preopen and points outside of it + fs.symlinkSync(path.join("..", "secret.txt"), path.join(sandbox, "escape")); + } + + const wasi = new WASI({ preopens: { "/": sandbox } }); + wasi.setMemory(new WebAssembly.Memory({ initial: 1 })); + const memory = Buffer.from(wasi.memory.buffer); + + const WASI_ESUCCESS = 0; + const WASI_ENOTCAPABLE = 76; + const WASI_RIGHT_FD_READ = BigInt(2); + const preopenFd = 3; + const pathPtr = 1024; + const statBufPtr = 8192; + const fdPtr = 16384; + const writePath = p => memory.write(p, pathPtr); + + // (1) absolute guest path naming an arbitrary host file must not reach it + let len = writePath(path.join(root, "secret.txt")); + expect(wasi.wasiImport.path_filestat_get(preopenFd, 1, pathPtr, len, statBufPtr)).not.toBe(WASI_ESUCCESS); + + // (2) ".." traversal out of the preopen + len = writePath("../secret.txt"); + expect(wasi.wasiImport.path_filestat_get(preopenFd, 0, pathPtr, len, statBufPtr)).toBe(WASI_ENOTCAPABLE); + expect(wasi.wasiImport.path_unlink_file(preopenFd, pathPtr, len)).toBe(WASI_ENOTCAPABLE); + expect(fs.existsSync(path.join(root, "secret.txt"))).toBe(true); + + // (3) a pre-placed symlink inside the preopen that points outside of it + if (!isWindows) { + len = writePath("escape"); + expect(wasi.wasiImport.path_filestat_get(preopenFd, 1, pathPtr, len, statBufPtr)).toBe(WASI_ENOTCAPABLE); + expect(wasi.wasiImport.path_open(preopenFd, 0, pathPtr, len, 0, WASI_RIGHT_FD_READ, BigInt(0), 0, fdPtr)).toBe( + WASI_ENOTCAPABLE, + ); + expect(wasi.FD_MAP.has(4)).toBe(false); + } + + // a path that stays inside the preopen still works + len = writePath("inside.txt"); + expect(wasi.wasiImport.path_filestat_get(preopenFd, 0, pathPtr, len, statBufPtr)).toBe(WASI_ESUCCESS); + expect(wasi.wasiImport.path_open(preopenFd, 0, pathPtr, len, 0, WASI_RIGHT_FD_READ, BigInt(0), 0, fdPtr)).toBe( + WASI_ESUCCESS, + ); + expect(wasi.FD_MAP.has(4)).toBe(true); +}); diff --git a/test/js/node/crypto/sign-jwk-ieee-p1363.test.ts b/test/js/node/crypto/sign-jwk-ieee-p1363.test.ts index 8356208e46a1..acc7ef5930cc 100644 --- a/test/js/node/crypto/sign-jwk-ieee-p1363.test.ts +++ b/test/js/node/crypto/sign-jwk-ieee-p1363.test.ts @@ -112,3 +112,49 @@ test("crypto.Sign should handle JWK EC keys with different encodings", () => { expect(signature.length).toBe(64); } }); + +test("crypto.Verify with ieee-p1363 rejects signatures that are not in P1363 format", () => { + const jwkKey = { + kty: "EC", + crv: "P-256", + x: "UachlYxCg48kyuIpXA7RRci2bb99E7izkzDQfX1sc6U", + y: "umhCJJQF5niKkNIvna0egspwqEPc0XiuJ0vmpMOKdSg", + d: "g_AptXAXWjIrPcyXQWW16JZdSV65Np7DOQxTl-SNhDQ", + }; + const publicJwk = { kty: jwkKey.kty, crv: jwkKey.crv, x: jwkKey.x, y: jwkKey.y }; + const testData = "test data to verify"; + + // Produce a DER-encoded signature over the data (default dsaEncoding is 'der'). + const derSigner = crypto.createSign("sha256"); + derSigner.update(testData); + const derSignature = derSigner.sign({ key: jwkKey, format: "jwk" }); + // A DER ECDSA signature for P-256 is a SEQUENCE wrapper, not the raw 64-byte r||s. + expect(derSignature.length).not.toBe(64); + + // Sanity check: the DER signature verifies under the encoding it was produced in. + { + const verifier = crypto.createVerify("sha256"); + verifier.update(testData); + expect(verifier.verify({ key: publicJwk, format: "jwk", dsaEncoding: "der" }, derSignature)).toBe(true); + } + + // When the caller requests ieee-p1363, a signature that is not 2*n bytes of + // raw r||s must fail verification rather than being reinterpreted as DER. + { + const verifier = crypto.createVerify("sha256"); + verifier.update(testData); + expect(verifier.verify({ key: publicJwk, format: "jwk", dsaEncoding: "ieee-p1363" }, derSignature)).toBe(false); + } + + // The legitimate case still works: a real 64-byte P1363 signature verifies. + { + const p1363Signer = crypto.createSign("sha256"); + p1363Signer.update(testData); + const p1363Signature = p1363Signer.sign({ key: jwkKey, format: "jwk", dsaEncoding: "ieee-p1363" }); + expect(p1363Signature.length).toBe(64); + + const verifier = crypto.createVerify("sha256"); + verifier.update(testData); + expect(verifier.verify({ key: publicJwk, format: "jwk", dsaEncoding: "ieee-p1363" }, p1363Signature)).toBe(true); + } +}); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 5dce1fa754c1..5347ad74c85f 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -4057,3 +4057,132 @@ describe.skipIf(isWindows)("readFileSync on a FIFO larger than the stat size", ( expect(exitCode).toBe(0); }); }); + +it("fs.read keeps filling the caller's view when its ArrayBuffer is transferred while the read is pending", async () => { + using dir = tempDir("fs-read-transfer", { + "data.bin": Buffer.alloc(65536, 0x61).toString(), + }); + + // The async read snapshots the destination buffer before handing it to the + // work pool. Transferring the backing ArrayBuffer immediately afterwards + // must not leave the in-flight read writing into storage the caller's view + // no longer owns: the view must still be attached and contain the file's + // bytes once the read completes. + const script = ` + const fs = require("node:fs"); + const path = require("node:path"); + (async () => { + const fd = fs.openSync(path.join(process.cwd(), "data.bin"), "r"); + const ab = new ArrayBuffer(65536); + const view = new Uint8Array(ab); + const pending = new Promise((resolve, reject) => { + fs.read(fd, view, 0, 65536, 0, (err, bytesRead) => (err ? reject(err) : resolve(bytesRead))); + }); + // Attempt to detach the destination's backing store before the async + // read completes. Refusing the detach by throwing is also acceptable. + let transferred; + try { + transferred = ab.transfer(); + } catch {} + const bytesRead = await pending; + fs.closeSync(fd); + console.log( + JSON.stringify({ + bytesRead, + viewByteLength: view.byteLength, + first: view[0] ?? null, + last: view[65535] ?? null, + }), + ); + })().catch(err => { + console.error(err); + process.exit(1); + }); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(JSON.parse(stdout.trim())).toEqual({ + bytesRead: 65536, + viewByteLength: 65536, + first: 0x61, + last: 0x61, + }); + expect(exitCode).toBe(0); +}); + +it("writevSync does not write bytes from a buffer detached by an index getter during argument conversion", () => { + using dir = tempDir("fs-writev-detach", {}); + const file = join(String(dir), "out.bin"); + + // Legitimate case: a plain array of views writes every byte. + let fd = openSync(file, "w"); + expect(writevSync(fd, [Buffer.from("AAAA"), Buffer.from("BBBB")])).toBe(8); + closeSync(fd); + expect(readFileSync(file, "latin1")).toBe("AAAABBBB"); + + // An accessor on index 1 detaches element 0's ArrayBuffer while the + // argument array is still being converted. Every element is read before any + // data pointer is captured, so the detached element contributes zero bytes + // instead of a dangling pointer. + const first = new Uint8Array(new ArrayBuffer(16)).fill(0x41); + const second = new Uint8Array(8).fill(0x42); + const buffers: Uint8Array[] = [first]; + Object.defineProperty(buffers, 1, { + enumerable: true, + configurable: true, + get() { + first.buffer.transfer(); + return second; + }, + }); + expect(buffers.length).toBe(2); + + fd = openSync(file, "w"); + try { + expect(writevSync(fd, buffers)).toBe(8); + } finally { + closeSync(fd); + } + expect(first.buffer.detached).toBe(true); + expect(readFileSync(file, "latin1")).toBe("BBBBBBBB"); +}); + +it("fs.writev keeps buffers attached while the write is in flight", async () => { + using dir = tempDir("fs-writev-pin", {}); + const file = join(String(dir), "out.bin"); + const fd = openSync(file, "w"); + const buf = new Uint8Array(new ArrayBuffer(8)).fill(0x43); + const { promise, resolve, reject } = Promise.withResolvers(); + try { + fs.writev(fd, [buf], 0, (err, written) => (err ? reject(err) : resolve(written))); + + // The native write runs on the thread pool; the backing store cannot be + // detached out from under it. + buf.buffer.transfer(); + expect(buf.buffer.detached).toBe(false); + + expect(await promise).toBe(8); + + // Released once the write completes. + buf.buffer.transfer(); + expect(buf.buffer.detached).toBe(true); + + // A rejected call must not leave the buffers held either. + const other = new Uint8Array(new ArrayBuffer(8)); + expect(() => fs.writev(fd, [other], "not a position" as any, () => {})).toThrow(); + other.buffer.transfer(); + expect(other.buffer.detached).toBe(true); + } finally { + closeSync(fd); + } + expect(readFileSync(file, "latin1")).toBe("CCCCCCCC"); +}); diff --git a/test/js/node/http/node-http-parser.test.ts b/test/js/node/http/node-http-parser.test.ts index 3b06ab723187..15b2ec71e13b 100644 --- a/test/js/node/http/node-http-parser.test.ts +++ b/test/js/node/http/node-http-parser.test.ts @@ -36,6 +36,34 @@ describe("HTTPParser.prototype.close", () => { }); describe("HTTPParser.prototype.finish", () => { + test("reports bytesParsed of 0 when finish() fails after a paused parse", () => { + const parser = new HTTPParser(); + parser.initialize(HTTPParser.REQUEST, {}); + + // Returning HPE_PAUSED (21) from the headers-complete callback makes + // llhttp pause mid-message and record a position inside the input buffer + // as its error position. + parser[kOnHeadersComplete] = function () { + return 21; + }; + + const paused = parser.execute(Buffer.from("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n")); + expect(paused).toMatchObject({ code: "HPE_PAUSED" }); + + // Resuming clears the pause, but llhttp keeps the stale error position + // from the previous buffer. + parser.resume(); + + // finish() mid-message reports an EOF error. bytesParsed must be exactly + // 0 rather than a value derived from the stale error position. + const result = parser.finish(); + expect(result).toMatchObject({ + code: "HPE_INVALID_EOF_STATE", + reason: "Invalid EOF state", + }); + expect(result.bytesParsed).toBe(0); + }); + test("returns error for invalid state", async () => { const parser = new HTTPParser(); parser.initialize(HTTPParser.REQUEST, {}); @@ -62,6 +90,36 @@ describe("HTTPParser.prototype.finish", () => { }); }); +describe("HTTPParser.prototype.execute", () => { + test("rejects re-entrant execute, even after a nested finish()", async () => { + const parser = new HTTPParser(); + parser.initialize(HTTPParser.REQUEST, {}); + const input = Buffer.from("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n"); + const other = Buffer.from("GET /other HTTP/1.1\r\nHost: example.com\r\n\r\n"); + const { promise, resolve, reject } = Promise.withResolvers(); + let entered = false; + parser[kOnHeadersComplete] = function () { + if (entered) return; + entered = true; + try { + // Re-entering execute() while a buffer is still being parsed would + // corrupt llhttp's span pointers, so it must be rejected. + expect(() => this.execute(other)).toThrow("HTTPParser.execute is not reentrant"); + // A nested finish() must not disarm the re-entrancy guard. + this.finish(); + expect(() => this.execute(other)).toThrow("HTTPParser.execute is not reentrant"); + resolve(); + } catch (err) { + reject(err); + } + }; + expect(parser.execute(input)).toBe(input.length); + await promise; + // Once the outer execute() has returned, the parser accepts new data again. + expect(parser.execute(input)).toBe(input.length); + }); +}); + test("HTTPParser.prototype.getCurrentBuffer", async () => { const parser = new HTTPParser(); parser.initialize(HTTPParser.REQUEST, {}); diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index e7fac011c64c..bf36455cdba9 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -1786,3 +1786,165 @@ describe("HTTP Server Security Tests - Advanced", () => { expect(text).toBe("Hello World"); }); }); + +it("native server socket handle accessors return undefined for non-socket receivers", async () => { + // The custom getters/setters on the native server-socket prototype must verify the + // receiver type. Reflect.get(proto, name, {}) invokes the native accessor with an + // arbitrary object as `this`; it must return undefined instead of reading native + // fields out of the foreign object's storage. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const server = http.createServer((req, res) => { + let failure; + try { + const socket = req.socket; + const handleSym = Object.getOwnPropertySymbols(socket).find(s => s.description === "handle"); + const handle = handleSym && socket[handleSym]; + if (!handle || typeof handle.write !== "function") { + throw new Error("could not locate the native socket handle"); + } + const proto = Object.getPrototypeOf(handle); + const getters = [ + "closed", + "bytesWritten", + "secureEstablished", + "response", + "duplex", + "remoteAddress", + "localAddress", + "onclose", + "ondrain", + "ondata", + ]; + for (const name of getters) { + // Plain object with populated inline properties as the receiver. + const fake = { a: 1.1, b: 2.2, c: 3.3, d: 4.4, e: 5.5, f: 6.6 }; + const viaReflect = Reflect.get(proto, name, fake); + if (viaReflect !== undefined) { + throw new Error(name + " getter returned a value for a plain-object receiver: " + String(viaReflect)); + } + // The prototype object itself is also not a socket handle. + const viaProto = proto[name]; + if (viaProto !== undefined) { + throw new Error(name + " getter returned a value for the prototype receiver: " + String(viaProto)); + } + } + for (const name of ["duplex", "onclose", "ondrain", "ondata"]) { + // Setters must not write through a non-socket receiver. + Reflect.set(proto, name, function () {}, { a: 1.1, b: 2.2, c: 3.3 }); + } + // The real handle still works through the same accessors. + if (typeof handle.closed !== "boolean") throw new Error("handle.closed is not a boolean"); + if (typeof handle.bytesWritten !== "number") throw new Error("handle.bytesWritten is not a number"); + } catch (err) { + failure = err; + } + if (failure) { + console.error(failure && (failure.stack || failure.message || failure)); + res.end("FAIL"); + } else { + console.log("OK"); + res.end("PASS"); + } + server.close(); + }); + server.listen(0, "127.0.0.1", () => { + fetch("http://127.0.0.1:" + server.address().port + "/").then(r => r.text()); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toContain("OK"); + expect(exitCode).toBe(0); +}, 15_000); + +it("socket handle write keeps buffered data intact when encoding coercion re-enters write", async () => { + // Argument conversion for the native socket write can run arbitrary JS (an encoding + // object's toString). If that JS calls write() again on the same socket, both the + // re-entrant write's data and the outer write's data must survive; nothing may be + // dropped or written through a stale buffer. + // + // The raw handle.write()/streamBuffer path only has its drain machinery wired up for + // CONNECT-tunneled sockets (uWS HttpContext::onWritable gates onSocketDrain on + // isConnectRequest), so the scenario must be driven from a "connect" handler — on a + // plain GET the buffered bytes would never flush and the fixture would hang. + const MB = 1024 * 1024; + const expectedTotal = 8 * MB + 4 * MB + 4 * MB; + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const net = require("node:net"); + const MB = 1024 * 1024; + const A = Buffer.alloc(8 * MB, 0x61); + const B = Buffer.alloc(4 * MB, 0x62); + const C = Buffer.alloc(4 * MB, 0x63); + const server = http.createServer(); + server.on("connect", (req, socket) => { + const handleSym = Object.getOwnPropertySymbols(socket).find(s => s.description === "handle"); + const handle = handleSym && socket[handleSym]; + if (!handle || typeof handle.write !== "function") { + console.error("could not locate the native socket handle"); + process.exit(1); + } + // The CONNECT path already wired handle.ondrain (kEnableStreaming), so the native + // writable handler will flush the stream buffer as the client reads. + // The client cannot read while this handler runs synchronously on the same thread, + // so most of this 8 MB lands in the native stream buffer. + handle.write(A); + // The encoding object's toString() re-enters write() on the same socket while the + // outer call is still converting its arguments. + handle.write(B, { + toString() { + handle.write(C); + return "utf8"; + }, + }); + handle.end(); + }); + server.listen(0, "127.0.0.1", () => { + let received = 0; + let aCount = 0, bCount = 0, cCount = 0; + const client = net.connect(server.address().port, "127.0.0.1", () => { + client.write("CONNECT example.com:443 HTTP/1.1\\r\\nHost: example.com:443\\r\\n\\r\\n"); + }); + client.on("data", chunk => { + received += chunk.length; + for (let i = 0; i < chunk.length; i++) { + const b = chunk[i]; + if (b === 0x61) aCount++; + else if (b === 0x62) bCount++; + else if (b === 0x63) cCount++; + } + }); + client.on("end", () => { + console.log("received=" + received + " a=" + aCount + " b=" + bCount + " c=" + cCount); + process.exit(0); + }); + client.on("error", err => { + console.error(err); + process.exit(1); + }); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toContain("received=" + expectedTotal + " a=" + 8 * MB + " b=" + 4 * MB + " c=" + 4 * MB); + expect(exitCode).toBe(0); +}, 30_000); diff --git a/test/js/node/http2/node-http2-continuation.test.ts b/test/js/node/http2/node-http2-continuation.test.ts index c16576aee24e..3c106f8410ca 100644 --- a/test/js/node/http2/node-http2-continuation.test.ts +++ b/test/js/node/http2/node-http2-continuation.test.ts @@ -419,3 +419,66 @@ describe("HTTP/2 CONTINUATION frames - Server Side", () => { } }); }); + +// RFC 7540 Section 6.2: HEADERS frames may carry padding, but CONTINUATION +// frames may not. When the encoded header block lands just under the frame +// size limit, applying padding must not push the frame-splitting arithmetic +// past the end of the encoded header buffer. +describe("HTTP/2 HEADERS padding near the frame-size boundary", () => { + let paddingServer: ServerInfo; + + before(async () => { + paddingServer = await startNodeServer(); + }); + + after(() => { + paddingServer?.close(); + }); + + test( + "client with PADDING_STRATEGY_MAX sends header blocks just under the frame size", + { timeout: 30_000 }, + async () => { + // Sweep the HPACK-encoded header block size across the default + // 16384-byte frame boundary. "A" Huffman-encodes to 6 bits, so each + // 330-character step grows the encoded block by ~248 bytes. The range + // in which the maximum padding strategy yields non-zero padding while + // the padded block no longer fits in a single HEADERS frame is 255 + // bytes wide ([maxFrameSize - 255, maxFrameSize - 1]), so a sweep with + // a sub-255-byte step that starts below that range and ends above it is + // guaranteed to place at least one request inside it. Each probe uses a + // fresh connection so the HPACK dynamic table cannot shrink later + // encodings. + for (let valueLength = 21000; valueLength <= 22320; valueLength += 330) { + const client = http2.connect(paddingServer.url, { + ...TLS_OPTIONS, + rejectUnauthorized: false, + paddingStrategy: http2.constants.PADDING_STRATEGY_MAX, + settings: { + maxHeaderListSize: 256 * 1024, + }, + }); + + try { + const response = await makeRequest(client, { + ":method": "GET", + ":path": "/", + ":scheme": "https", + ":authority": `127.0.0.1:${paddingServer.port}`, + "x-filler": Buffer.alloc(valueLength, "A").toString(), + }); + + assert.ok(response.data, `Should receive response data for valueLength=${valueLength}`); + const parsed = JSON.parse(response.data); + assert.strictEqual( + parsed.receivedHeaders, + 1, + `Server should decode the single filler header for valueLength=${valueLength}`, + ); + } finally { + client.close(); + } + } + }, + ); +}); diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index dddf6aed6264..07488ff4bd2a 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -1979,3 +1979,60 @@ it("http2 request.destroy() with error", async () => { }); }); }); + +it("http2 client.request() rejects header names longer than 4096 bytes with a catchable error", async () => { + // A header name longer than the 4096-byte HPACK name buffer must surface as a + // thrown ERR_INVALID_HTTP_TOKEN, not terminate the process. Run in a + // subprocess so a crash shows up as a failed assertion instead of taking down + // the test runner. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http2 = require("node:http2"); + const server = http2.createServer(); + server.on("stream", stream => { + stream.respond({ ":status": 200 }); + stream.end("ok"); + }); + server.listen(0, "127.0.0.1", () => { + const client = http2.connect("http://127.0.0.1:" + server.address().port); + client.on("error", () => {}); + client.on("connect", () => { + try { + client.request({ ":path": "/", [Buffer.alloc(5000, "x").toString()]: "1" }); + console.log("NO_ERROR"); + } catch (err) { + console.log("CODE:" + err.code); + console.log("NAME:" + err.name); + } + // A legitimate request on the same session still succeeds afterwards. + const req = client.request({ ":path": "/" }); + req.on("response", headers => { + console.log("STATUS:" + headers[":status"]); + }); + req.resume(); + req.on("close", () => { + client.close(); + server.close(); + }); + req.end(); + }); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout).toContain("CODE:ERR_INVALID_HTTP_TOKEN"); + expect(stdout).toContain("NAME:TypeError"); + expect(stdout).not.toContain("NO_ERROR"); + expect(stdout).toContain("STATUS:200"); + expect(exitCode).toBe(0); +}); diff --git a/test/js/node/module/sourcemap.test.js b/test/js/node/module/sourcemap.test.js index d9ff675cdf1e..cb399fce1df9 100644 --- a/test/js/node/module/sourcemap.test.js +++ b/test/js/node/module/sourcemap.test.js @@ -1,5 +1,6 @@ const { test, expect } = require("bun:test"); const { SourceMap } = require("node:module"); +const { bunEnv, bunExe } = require("harness"); test("SourceMap class exists", () => { expect(SourceMap).toBeDefined(); @@ -175,3 +176,41 @@ test("SourceMap with invalid name index has undefined name property", () => { } `); }); + +test("SourceMap handles mappings with truncated VLQ segments without crashing", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const { SourceMap } = require("node:module"); +const truncated = [ + // 'g' decodes to a base64 value with the VLQ continuation bit set, so the + // decoder expects more bytes than the 1-byte input provides. + { version: 3, sources: [], mappings: "g" }, + // Both leading VLQ fields are consumed before the segment is complete, so + // the next field is decoded from an empty remainder. + { version: 3, sources: ["x.js"], mappings: "AA" }, +]; +for (const payload of truncated) { + try { + new SourceMap(payload); + } catch (err) { + // A clean SyntaxError for a malformed mapping is acceptable. + if (!(err instanceof SyntaxError)) throw err; + } +} +// A well-formed mapping still parses. +const ok = new SourceMap({ version: 3, sources: ["test.js"], mappings: "AAAA" }); +console.log(ok.findEntry(0, 0).originalSource); +console.log("done");`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toBe("test.js\ndone\n"); + expect(exitCode).toBe(0); +}); diff --git a/test/js/node/tls/renegotiation.test.ts b/test/js/node/tls/renegotiation.test.ts index dbc849d9745d..3a96abf3c59b 100644 --- a/test/js/node/tls/renegotiation.test.ts +++ b/test/js/node/tls/renegotiation.test.ts @@ -200,6 +200,86 @@ if (handshakes < 2) { console.log("ok"); `; +it("should terminate the connection when the peer exceeds the renegotiation limit over a duplex socket", async () => { + // tls.connect({ socket: }) is encrypted by the SSLWrapper path + // (UpgradedDuplex) rather than the uSockets C path. It must apply the same + // per-connection renegotiation cap: a malicious TLS 1.2 server that spams + // HelloRequest messages otherwise forces a full handshake each time + // (unbounded CPU per connection). + await using attacker = Bun.spawn({ + cmd: [ + "node", + "-e", + ` + const tls = require("tls"); + let renegs = 0; + const server = tls.createServer( + { + cert: process.env.SERVER_CERT, + key: process.env.SERVER_KEY, + minVersion: "TLSv1.2", + maxVersion: "TLSv1.2", + }, + socket => { + socket.on("error", () => {}); + const again = () => { + if (renegs >= 10) { + socket.write("DONE"); + return; + } + socket.renegotiate({ rejectUnauthorized: false }, err => { + if (err) return; + renegs++; + again(); + }); + }; + again(); + }, + ); + server.listen(0, () => console.log(server.address().port)); + `, + ], + stdout: "pipe", + stderr: "inherit", + stdin: "ignore", + env: { ...bunEnv, SERVER_CERT: tls.cert, SERVER_KEY: tls.key }, + }); + const { value } = await attacker.stdout.getReader().read(); + const port = Number(new TextDecoder().decode(value).trim()); + + const net = require("net"); + const { Duplex } = require("stream"); + const raw = net.connect(port, "127.0.0.1"); + const duplex = new Duplex({ + read() {}, + write(chunk, encoding, callback) { + raw.write(chunk, encoding, callback); + }, + final(callback) { + raw.end(); + callback(); + }, + }); + raw.on("data", (chunk: Buffer) => duplex.push(chunk)); + raw.on("end", () => duplex.push(null)); + raw.on("close", () => duplex.destroy()); + + const { promise: outcome, resolve } = Promise.withResolvers(); + let received = ""; + const socket = require("tls").connect({ socket: duplex, rejectUnauthorized: false }); + socket.on("data", (chunk: Buffer) => { + received += chunk.toString(); + if (received.includes("DONE")) resolve("got-response"); + }); + socket.on("error", () => {}); + socket.on("close", () => resolve("closed")); + + // The SSLWrapper must tear the connection down once the peer exceeds the + // renegotiation limit, before the attacker finishes its 10 renegotiations + // and delivers the response. + expect(await outcome).toBe("closed"); +}); + it("should fail if renegotiation fails using tls module", async () => { const { promise, resolve, reject } = Promise.withResolvers(); diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 1a37273b352c..97a68c1ffec1 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, normalizeBunSnapshot } from "harness"; import { compileFunction, constants, @@ -939,3 +940,70 @@ test("Loader is not defined in vm context", () => { // Ensure internal JSC Loader properties are not leaking through expect(runInContext("typeof Loader.registry;", customContext)).toBe("undefined"); }); + +test("node:vm native Module prototype methods reject non-module receivers", async () => { + // The native NodeVMModule prototype (reachable via the kNative own-symbol on a + // vm.SourceTextModule instance) must validate its receiver. Calling its methods + // with a plain object as `this` must throw a TypeError instead of reinterpreting + // the object's inline property storage as native module fields. + const fixture = ` + const vm = require("node:vm"); + const mod = new vm.SourceTextModule('import "./dep.js"; export const a = 1;'); + const kNative = Object.getOwnPropertySymbols(mod).find(s => s.description === "kNative"); + const native = mod[kNative]; + const proto = Object.getPrototypeOf(native); + const fake = { p1: 1n, p2: 0x41414141n }; + + const results = []; + for (const name of ["getStatus", "getStatusCode", "getModuleRequests", "createModuleRecord", "getError"]) { + if (typeof proto[name] !== "function") { + results.push(name + ": missing"); + continue; + } + try { + const value = proto[name].call(fake); + results.push(name + ": returned " + String(value)); + } catch (e) { + results.push(name + ": " + (e instanceof TypeError ? "TypeError" : "unexpected " + e)); + } + } + const identifierGetter = Object.getOwnPropertyDescriptor(proto, "identifier")?.get; + if (typeof identifierGetter !== "function") { + results.push("identifier: missing"); + } else { + try { + const value = identifierGetter.call(fake); + results.push("identifier: returned " + String(value)); + } catch (e) { + results.push("identifier: " + (e instanceof TypeError ? "TypeError" : "unexpected " + e)); + } + } + + // The legitimate receiver still works through the same native entry points. + results.push("status: " + proto.getStatus.call(native)); + results.push("requests: " + JSON.stringify(proto.getModuleRequests.call(native).map(r => r[0]))); + console.log(results.join("\\n")); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(normalizeBunSnapshot(stdout)).toMatchInlineSnapshot(` + "getStatus: TypeError + getStatusCode: TypeError + getModuleRequests: TypeError + createModuleRecord: TypeError + getError: TypeError + identifier: TypeError + status: unlinked + requests: [\"./dep.js\"]" + `); + expect(exitCode).toBe(0); +}); diff --git a/test/js/node/watch/fs.watch.test.ts b/test/js/node/watch/fs.watch.test.ts index 2c38aabf3193..9ebd22e1d8fe 100644 --- a/test/js/node/watch/fs.watch.test.ts +++ b/test/js/node/watch/fs.watch.test.ts @@ -900,3 +900,95 @@ test.skipIf(!isMacOS)("fs.watch(dir) on macOS does not leak the resolved FSEvent expect(exitCode).toBe(0); expect(stdout).toContain("RSS growth:"); }); + +// On Windows, fs.watch() registered every watcher into a single process-global +// PathWatcherManager bound to the first caller's VM/uv_loop. A Worker thread +// calling fs.watch() reused that manager: it mutated the watcher map and drove +// the main thread's uv_loop from a foreign thread (debug builds tripped a +// debug_assert and aborted; release builds raced). The manager is now +// re-allocated per VM, so a Worker's watcher never aliases the main thread's. +// +// Must run in a subprocess: on an unpatched debug build the Worker's +// fs.watch() call aborts the whole runtime. +test.skipIf(!isWindows)( + "fs.watch works from both the main thread and a Worker (windows)", + async () => { + using dir = tempDir("fswatch-worker", { + "main-watched/.keep": "", + "worker-watched/.keep": "", + "worker.js": /* js */ ` + import fs from "node:fs"; + import path from "node:path"; + import { parentPort } from "node:worker_threads"; + + const dir = path.join(import.meta.dir, "worker-watched"); + // Before the fix this call registered into the main thread's manager. + const watcher = fs.watch(dir, () => { + clearInterval(interval); + watcher.close(); + parentPort.postMessage("worker-saw-change"); + }); + const interval = setInterval(() => { + fs.writeFileSync(path.join(dir, "touch.txt"), String(Date.now())); + }, 20); + `, + "main.js": /* js */ ` + import fs from "node:fs"; + import path from "node:path"; + import { Worker } from "node:worker_threads"; + + const mainDir = path.join(import.meta.dir, "main-watched"); + + function watchForOneChange(dir) { + return new Promise((resolve, reject) => { + const watcher = fs.watch(dir, () => { + clearInterval(interval); + watcher.close(); + resolve(); + }); + watcher.on("error", err => { + clearInterval(interval); + reject(err); + }); + const interval = setInterval(() => { + fs.writeFileSync(path.join(dir, "touch.txt"), String(Date.now())); + }, 20); + }); + } + + // 1. The main thread registers the first watcher, creating the watcher + // manager bound to the main VM. + await watchForOneChange(mainDir); + + // 2. A Worker registers its own watcher and must observe a change. + const worker = new Worker(path.join(import.meta.dir, "worker.js")); + const msg = await new Promise((resolve, reject) => { + worker.on("message", resolve); + worker.on("error", reject); + }); + if (msg !== "worker-saw-change") throw new Error("unexpected worker message: " + msg); + await worker.terminate(); + + // 3. The main thread's watching must keep working after the Worker + // registered (and tore down) its own watcher. + await watchForOneChange(mainDir); + + console.log("OK"); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.js"], + cwd: String(dir), + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("OK"); + expect(exitCode).toBe(0); // unpatched debug builds abort in the Worker's fs.watch() + }, + 30000, +); diff --git a/test/js/node/zlib/zlib.test.js b/test/js/node/zlib/zlib.test.js index 7280089f36ee..fbf2e701c964 100644 --- a/test/js/node/zlib/zlib.test.js +++ b/test/js/node/zlib/zlib.test.js @@ -634,3 +634,58 @@ describe("zlib.zstd", () => { expect(all.length).toBeGreaterThanOrEqual(7); }, 15_000); }); + +describe("async write buffer lifetime", () => { + it("keeps the input and output buffers attached while a native write is in flight", async () => { + const { promise, resolve } = Promise.withResolvers(); + const deflate = zlib.createDeflate(); + try { + const handle = deflate._handle; + + const input = new Uint8Array(new ArrayBuffer(64)); + input.fill(97); + const out = new Uint8Array(new ArrayBuffer(1024)); + + // Mirror the bookkeeping processChunk() performs before calling + // handle.write(), so the native write callback can complete normally. + handle.buffer = input; + handle.cb = resolve; + handle.availOutBefore = out.byteLength; + handle.availInBefore = input.byteLength; + handle.inOff = 0; + handle.flushFlag = zlib.constants.Z_FINISH; + + handle.write( + zlib.constants.Z_FINISH, // flush + input, // in + 0, // in_off + input.byteLength, // in_len + out, // out + 0, // out_off + out.byteLength, // out_len + ); + + // The native worker thread reads `input` and writes compressed bytes into + // `out` through raw pointers until the write completes. Transferring + // either ArrayBuffer must not detach the backing store out from under + // the worker -- both buffers must stay attached and full-length. + out.buffer.transfer(); + input.buffer.transfer(); + expect(out.buffer.detached).toBe(false); + expect(out.byteLength).toBe(1024); + expect(input.buffer.detached).toBe(false); + expect(input.byteLength).toBe(64); + + await promise; + + // Once the write completes the buffers are released and can be + // transferred again. + out.buffer.transfer(); + input.buffer.transfer(); + expect(out.buffer.detached).toBe(true); + expect(input.buffer.detached).toBe(true); + } finally { + deflate.close(); + } + }); +}); diff --git a/test/js/sql/sql-mysql-raw-length-prefix.test.ts b/test/js/sql/sql-mysql-raw-length-prefix.test.ts index ab361efba3ea..531295530f51 100644 --- a/test/js/sql/sql-mysql-raw-length-prefix.test.ts +++ b/test/js/sql/sql-mysql-raw-length-prefix.test.ts @@ -287,3 +287,185 @@ test(".raw() strips length-prefix bytes (#30039) — binary protocol", async () await new Promise(r => server.close(() => r())); } }); + +// A COM_QUERY whose payload exceeds the 24-bit packet length limit cannot be +// framed as a single MySQL packet. It must be rejected client-side AND rolled +// back out of the connection's write buffer: leaving the partially-serialized +// packet behind desynchronizes the protocol stream, and the next query gets +// appended after the garbage and reparsed by the server as bogus packets. +test("oversized COM_QUERY is rejected and rolled back out of the write buffer", async () => { + const queries: string[] = []; + let desynced = false; + + const server = net.createServer(socket => { + let buffered = Buffer.alloc(0); + let authed = false; + socket.write(handshakeV10()); + socket.on("data", chunk => { + buffered = Buffer.concat([buffered, chunk]); + while (buffered.length >= 4) { + const len = buffered[0] | (buffered[1] << 8) | (buffered[2] << 16); + if (buffered.length < 4 + len) break; + const seq = buffered[3]; + const payload = buffered.subarray(4, 4 + len); + buffered = buffered.subarray(4 + len); + + if (!authed) { + authed = true; + socket.write(okPacket(seq + 1)); + continue; + } + if (payload[0] === 0x03 /* COM_QUERY */) { + queries.push(payload.subarray(1).toString("utf-8")); + socket.write(textResultSet(seq + 1, shortText, jsonText)); + } else if (payload[0] === 0x01 /* COM_QUIT */) { + socket.end(); + } else { + // A zero-length packet or one that does not start with a known + // command byte means the client's outgoing stream is no longer + // aligned on packet boundaries. Destroy the socket so the test + // fails fast instead of hanging. + desynced = true; + socket.destroy(); + } + } + }); + socket.on("error", () => {}); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const { port } = server.address() as net.AddressInfo; + + try { + await using sql = new SQL({ url: `mysql://root@127.0.0.1:${port}/db`, max: 1 }); + + // 1 command byte + 0xffffff bytes of query text = 0x1000000 — one past + // the largest payload a single MySQL packet can frame. + const oversized = Buffer.alloc(0xffffff, "-").toString(); + const first = await sql.unsafe(oversized).then( + () => "resolved", + e => e?.code ?? String(e), + ); + + // The same connection must still be usable: the rejected packet must not + // leave any bytes behind in the write buffer. + const second = await sql.unsafe("select 1").then( + () => "resolved", + e => e?.code ?? String(e), + ); + + expect({ first, second, queries, desynced }).toEqual({ + first: "ERR_MYSQL_OVERFLOW", + second: "resolved", + queries: ["select 1"], + desynced: false, + }); + } finally { + await new Promise(r => server.close(() => r())); + } +}); + +// --- 251-byte length-encoded values vs. the NULL marker --------------------- +// +// The text-protocol NULL marker is the single literal byte 0xfb. A column +// value that is exactly 251 bytes long is length-encoded as `0xfc 0xfb 0x00` +// followed by 251 payload bytes — and the decoded *length* is also 251 +// (0xfb). The decoder must distinguish the two by encoding width: if it only +// compares the decoded value, the column is misread as NULL, only the 3 +// length bytes are consumed, and the 251 payload bytes are re-parsed as the +// lengths/contents of the following columns. Whoever controls the first +// column then controls what the application sees in the rest of the row. + +// The first bytes of the 251-byte payload deliberately form a valid +// length-encoded string ("admin") so a desynchronized decoder would surface +// it as the *next* column's value instead of "user". +const bio251 = "\x05admin" + Buffer.alloc(251 - 6, "x").toString(); +const realRole = "user"; + +function textResultSet251(startSeq: number): Buffer { + const packets: Buffer[] = []; + let seq = startSeq; + + // Column count + packets.push(packet(seq++, Buffer.from([0x02]))); + packets.push(packet(seq++, columnDefinition("bio", MYSQL_TYPE_VAR_STRING))); + packets.push(packet(seq++, columnDefinition("role", MYSQL_TYPE_VAR_STRING))); + // Row 1: exactly-251-byte bio (3-byte lenenc prefix 0xfc 0xfb 0x00), then + // role = "user". + packets.push(packet(seq++, Buffer.concat([lenencStr(bio251), lenencStr(realRole)]))); + // Row 2: a genuine NULL bio (the single marker byte 0xfb), then + // role = "editor" — the legitimate NULL case must keep working. + packets.push(packet(seq++, Buffer.concat([Buffer.from([0xfb]), lenencStr("editor")]))); + // OK packet to close the result set (with CLIENT_DEPRECATE_EOF, header 0xfe). + packets.push(packet(seq++, Buffer.from([0xfe, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00]))); + + return Buffer.concat(packets); +} + +function startMock251Server() { + const server = net.createServer(socket => { + let buffered = Buffer.alloc(0); + let authed = false; + + socket.write(handshakeV10()); + + socket.on("data", chunk => { + buffered = Buffer.concat([buffered, chunk]); + while (buffered.length >= 4) { + const len = buffered[0] | (buffered[1] << 8) | (buffered[2] << 16); + if (buffered.length < 4 + len) break; + const seq = buffered[3]; + const payload = buffered.subarray(4, 4 + len); + buffered = buffered.subarray(4 + len); + + if (!authed) { + authed = true; + socket.write(okPacket(seq + 1)); + continue; + } + + const cmd = payload[0]; + if (cmd === 0x03 /* COM_QUERY */) { + socket.write(textResultSet251(seq + 1)); + } else { + // COM_QUIT / anything else — close. + socket.end(); + } + } + }); + }); + server.listen(0, "127.0.0.1"); + return server; +} + +test("text protocol decodes a 251-byte column value as data, not as NULL", async () => { + // Sanity: the payload is exactly 251 bytes and 251 really is the 3-byte + // lenenc form whose decoded value collides with the NULL marker byte. + expect(Buffer.byteLength(bio251, "utf-8")).toBe(251); + expect(Array.from(lenenc(251))).toEqual([0xfc, 0xfb, 0x00]); + + const server = startMock251Server(); + await once(server, "listening"); + const { port } = server.address() as net.AddressInfo; + try { + await using sql = new SQL({ url: `mysql://root@127.0.0.1:${port}/db`, max: 1 }); + // `.simple()` forces the text protocol → ResultSet decode_text, where the + // NULL-marker check lives. + const rows = (await sql`SELECT bio, role FROM users`.simple()) as unknown as { + bio: string | null; + role: string; + }[]; + expect(rows).toHaveLength(2); + // The 251-byte value must come back intact — not as NULL with the + // following column re-read out of the 251 payload bytes (which would + // make role === "admin"). + expect(rows[0].role).toBe(realRole); + expect(rows[0].bio).toBe(bio251); + // A genuine NULL marker (single 0xfb byte) still decodes as NULL and the + // row stays aligned. + expect(rows[1].bio).toBeNull(); + expect(rows[1].role).toBe("editor"); + } finally { + await new Promise(r => server.close(() => r())); + } +}); diff --git a/test/js/sql/sql-mysql-tls-plaintext-injection.test.ts b/test/js/sql/sql-mysql-tls-plaintext-injection.test.ts new file mode 100644 index 000000000000..c3185c15b5cb --- /dev/null +++ b/test/js/sql/sql-mysql-tls-plaintext-injection.test.ts @@ -0,0 +1,97 @@ +// Uses a minimal mock MySQL server so it can run without Docker. + +import { SQL } from "bun"; +import { expect, mock, test } from "bun:test"; +import net from "net"; + +test("MySQL TLS handshake rejects plaintext packets buffered behind the server greeting", async () => { + // A man-in-the-middle can append forged packets (e.g. an OK packet that marks + // the connection as authenticated) to the same TCP segment as the server + // greeting. Once the handshake negotiates TLS, everything after the greeting + // must arrive over the encrypted channel; bytes already buffered in plaintext + // must not be fed to the auth/command handlers. + function u16le(n: number) { + return Buffer.from([n & 0xff, (n >> 8) & 0xff]); + } + function u24le(n: number) { + return Buffer.from([n & 0xff, (n >> 8) & 0xff, (n >> 16) & 0xff]); + } + function u32le(n: number) { + return Buffer.from([n & 0xff, (n >> 8) & 0xff, (n >> 16) & 0xff, (n >>> 24) & 0xff]); + } + function packet(seq: number, payload: Buffer) { + return Buffer.concat([u24le(payload.length), Buffer.from([seq]), payload]); + } + + const CLIENT_PROTOCOL_41 = 1 << 9; + const CLIENT_SSL = 1 << 11; + const CLIENT_SECURE_CONNECTION = 1 << 15; + const CLIENT_PLUGIN_AUTH = 1 << 19; + const CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA = 1 << 21; + const CLIENT_DEPRECATE_EOF = 1 << 24; + const SERVER_CAPS = + CLIENT_PROTOCOL_41 | + CLIENT_SSL | + CLIENT_SECURE_CONNECTION | + CLIENT_PLUGIN_AUTH | + CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA | + CLIENT_DEPRECATE_EOF; + + const authData1 = Buffer.alloc(8, 0x61); + const authData2 = Buffer.alloc(13, 0x62); + authData2[12] = 0; + const greeting = packet( + 0, + Buffer.concat([ + Buffer.from([10]), // protocol version + Buffer.from("mock-8.0.0\0"), // server version, NUL-terminated + u32le(1), // connection id + authData1, // auth-plugin-data-part-1 (8 bytes) + Buffer.from([0]), // filler + u16le(SERVER_CAPS & 0xffff), // capability flags (lower) + Buffer.from([0x2d]), // character set + u16le(0x0002), // status flags (SERVER_STATUS_AUTOCOMMIT) + u16le((SERVER_CAPS >>> 16) & 0xffff), // capability flags (upper) + Buffer.from([21]), // auth-plugin-data length + Buffer.alloc(10, 0), // reserved + authData2, // auth-plugin-data-part-2 (13 bytes) + Buffer.from("caching_sha2_password\0"), + ]), + ); + // A forged OK packet. If the client keeps consuming the plaintext buffer + // after deciding to upgrade to TLS, this marks the connection as + // authenticated without any certificate ever being validated. + const forgedOk = packet(2, Buffer.from([0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00])); + + const server = net.createServer(socket => { + // Greeting and the injected packet arrive in a single segment, before the + // client has sent a byte. + socket.write(Buffer.concat([greeting, forgedOk])); + // Whatever the client sends next (SSLRequest, TLS ClientHello, auth + // response), close so a misbehaving client cannot hang waiting for more. + socket.on("data", () => socket.end()); + socket.on("error", () => {}); + }); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as import("node:net").AddressInfo; + + const onconnect = mock(); + try { + await using sql = new SQL({ + url: `mysql://root:pw@127.0.0.1:${port}/db`, + max: 1, + tls: { rejectUnauthorized: false }, + onconnect, + }); + const err = await sql`select 1`.then( + () => ({ code: "UNEXPECTED_SUCCESS" }), + e => ({ code: e?.code ?? String(e) }), + ); + // The connection must never be reported as established off the back of a + // plaintext packet, and the buffered bytes must be rejected outright. + expect(onconnect).not.toHaveBeenCalled(); + expect(err).toEqual({ code: "ERR_MYSQL_UNEXPECTED_PACKET" }); + } finally { + await new Promise(r => server.close(() => r())); + } +}); diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index 290856725b5e..344f5cecb48e 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -39,6 +39,26 @@ if (isDockerEnabled()) { concurrent: true, }, container => { + test("rejects a bind parameter that cannot be framed in a single wire packet", async () => { + await using db = new SQL({ ...getOptions(), max: 1 }); + + // A large but representable payload round-trips normally. + const ok = Buffer.alloc(1024 * 1024, 0x42); + expect((await db`select length(${ok}) as n`)[0].n).toBe(ok.length); + + // The MySQL packet header stores the payload length in 24 bits. A + // payload of >= 0xFFFFFF cannot be framed as a single packet; the + // client must refuse to send it instead of emitting a truncated + // length that the server would reparse as additional, independently + // framed client packets. + const oversized = Buffer.alloc(0xffffff + 64, 0x41); + const err = await db`select length(${oversized}) as n`.then( + () => ({ code: "UNEXPECTED_SUCCESS" }), + e => ({ code: (e as any)?.code ?? String(e) }), + ); + expect(err).toEqual({ code: "ERR_MYSQL_OVERFLOW" }); + }); + let sql: SQL; const password = image.image === "mysql_plain" ? "" : "bun"; const getOptions = (): Bun.SQL.Options => ({ diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index f3ac556de2c2..cf4d703dba0a 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -139,6 +139,30 @@ if (isDockerEnabled()) { }); describe("Array helpers", () => { + test("sql.array rejects type names with parentheses outside numeric modifiers", async () => { + await using sql = postgres(options); + + // The type name is interpolated verbatim into `$N::${type}[]`. Parentheses, + // commas and spaces are only legal as `(digits[,digits])` modifiers after an + // identifier; a value that closes the cast and appends further expression + // terms must be refused before it reaches the query text. + for (const type of [ + "INT) OR PG_SLEEP(10) IS NOT NULL OR ID IN (1", + "INT) OR ARRAY_LENGTH(CAST(NULL AS INT", + "TEXT(1)) , (SELECT PG_SLEEP(10", + "INT(1,2,X)", + ]) { + expect(() => sql.array([1, 2], type)).toThrow(/valid PostgreSQL type name/); + } + + // Legitimate parameterized and qualified type names are still accepted. + expect(() => sql.array([1.5], "NUMERIC(10,2)")).not.toThrow(); + expect(() => sql.array([new Date()], "TIMESTAMP(3) WITH TIME ZONE")).not.toThrow(); + expect(() => sql.array(["a"], "MYSCHEMA.MY_ENUM")).not.toThrow(); + const [{ x }] = await sql`select ${sql.array(["hello", "world"], "CHARACTER VARYING(255)")} as x`; + expect(x).toEqual(["hello", "world"]); + }); + test("SQL helper should support sql.array", async () => { await using sql = postgres(options); const random_name = "test_" + randomUUIDv7("hex").replaceAll("-", ""); @@ -12381,6 +12405,31 @@ CREATE TABLE ${table_name} ( }); }); }); // Close "Misc" describe + test("sql.begin rejects transaction option strings containing statement separators", async () => { + await using sql = postgres({ ...options, max: 1 }); + const marker = "inj_" + randomUUIDv7("hex").replaceAll("-", ""); + + // The transaction-mode string is interpolated into `BEGIN ${options}` and sent + // down the simple-query path, which accepts multiple semicolon-separated + // statements. Anything other than keyword lists must be refused up front. + const error = await sql + .begin(`; CREATE TABLE ${marker} (a int)`, async tx => { + await tx`select 1`; + }) + .catch(e => e); + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain("Transaction options can only contain letters, spaces, and commas."); + + // The statement embedded in the options string must not have executed. + const [{ found }] = await sql`SELECT to_regclass(${marker}) IS NOT NULL AS found`; + expect(found).toBe(false); + + // Legitimate transaction modes still work. + const [{ x }] = await sql.begin("read only", async tx => await tx`select 1 as x`); + expect(x).toBe(1); + const [{ y }] = await sql.begin("isolation level serializable, read only", async tx => await tx`select 2 as y`); + expect(y).toBe(2); + }); test("Handles empty integer array stored as {}", async () => { await using db = postgres(options); const tableName = `test_${randomUUIDv7("hex").replaceAll("-", "")}`; @@ -12425,3 +12474,105 @@ CREATE TABLE ${table_name} ( }); }); // Close "PostgreSQL tests" describe } // Close if (isDockerEnabled()) + +// A malicious or buggy Postgres server can send a text-format json[]/jsonb[] +// DataRow whose array literal contains an unquoted element starting with 'f' or +// 't' that is not exactly "false"/"true". The array parser must reject it; +// previously it consumed no input and re-entered the element loop forever, +// blocking the JS thread. The fixture runs in a subprocess so a regression +// shows up as a killed child instead of a hung test file. +test("text-format json[] with a malformed boolean literal returns an error instead of looping", async () => { + const fixtureDir = tempDirWithFiles("pg-json-array-bool-literal", { + "fixture.ts": ` +import { SQL } from "bun"; +import net from "node:net"; + +function pkt(type, body) { + const header = Buffer.alloc(5); + header.write(type, 0); + header.writeInt32BE(body.length + 4, 1); + return Buffer.concat([header, body]); +} +const int16 = n => { const b = Buffer.alloc(2); b.writeInt16BE(n, 0); return b; }; +const int32 = n => { const b = Buffer.alloc(4); b.writeInt32BE(n, 0); return b; }; +const cstr = s => Buffer.concat([Buffer.from(s), Buffer.from([0])]); + +// Single column "x" of type json[] (oid 199), format 0 (text). +const rowDescription = pkt("T", Buffer.concat([ + int16(1), + cstr("x"), int32(0), int16(0), int32(199), int16(-1), int32(-1), int16(0), +])); +function dataRow(text) { + const value = Buffer.from(text); + return pkt("D", Buffer.concat([int16(1), int32(value.length), value])); +} +const authenticationOk = pkt("R", int32(0)); +const readyForQuery = pkt("Z", Buffer.from("I")); +const commandComplete = pkt("C", cstr("SELECT 1")); + +async function run(arrayText) { + const server = net.createServer(socket => { + let startup = true; + socket.on("data", data => { + if (startup) { + startup = false; + socket.write(Buffer.concat([authenticationOk, readyForQuery])); + return; + } + if (data[0] !== 0x51 /* 'Q' */) return; + socket.write(Buffer.concat([rowDescription, dataRow(arrayText), commandComplete, readyForQuery])); + }); + socket.on("error", () => {}); + }); + await new Promise(r => server.listen(0, "127.0.0.1", () => r())); + const port = server.address().port; + const sql = new SQL({ + url: "postgres://u@127.0.0.1:" + port + "/db", + max: 1, + idleTimeout: 5, + connectionTimeout: 5, + }); + try { + const rows = await sql\`select x\`.simple(); + console.log("ROWS " + arrayText + " => " + JSON.stringify(rows[0] && rows[0].x)); + } catch (e) { + console.log("REJECTED " + arrayText + " => " + (e.code || e.message)); + } finally { + await sql.close().catch(() => {}); + await new Promise(r => server.close(() => r())); + } +} + +// Malformed boolean literals: must error, not spin forever. +await run("{falsy}"); +await run("{truthy}"); +// Well-formed booleans in a json[] must still parse. +await run("{false,true}"); +console.log("FIXTURE_DONE"); +`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.ts"], + cwd: fixtureDir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + // Bounds the child if the array parser regresses into an unbounded loop. + timeout: 10_000, + killSignal: "SIGKILL", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const filteredStderr = stderr + .split(/\r?\n/) + .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) + .join("\n"); + + expect(stdout).toContain("REJECTED {falsy} => ERR_POSTGRES_UNSUPPORTED_ARRAY_FORMAT"); + expect(stdout).toContain("REJECTED {truthy} => ERR_POSTGRES_UNSUPPORTED_ARRAY_FORMAT"); + expect(stdout).toContain("ROWS {false,true} => [false,true]"); + expect(stdout).toContain("FIXTURE_DONE"); + expect(filteredStderr).toBe(""); + expect(exitCode).toBe(0); +}, 30_000); diff --git a/test/js/web/fetch/blob.test.ts b/test/js/web/fetch/blob.test.ts index 0eb83a0ecb5a..f1fd9eef67c9 100644 --- a/test/js/web/fetch/blob.test.ts +++ b/test/js/web/fetch/blob.test.ts @@ -324,3 +324,101 @@ test("dupe() preserves allocated content_type for Body clone", () => { expect(originalType).toStartWith("multipart/form-data; boundary="); expect(clonedType).toBe(originalType); }); + +test("Blob part's bytes survive a later part freeing it during construction", async () => { + // Regression: the Blob constructor pushed Blob parts into the string joiner + // as *borrowed* views into their Store's bytes. A later part whose + // processing runs user JS (an object part's toString) could drop the last + // reference to that Blob and GC it, freeing the Store before the joiner + // copied the view out — a use-after-free. Blob parts must be copied at push + // time. Under ASAN the unfixed read is a use-after-poison crash. + using dir = tempDir("blob-part-uaf", { + "run.ts": ` + const SIZE = 1 << 18; + const expected = Buffer.alloc(SIZE, "A").toString() + "x"; + for (let i = 0; i < 16; i++) { + const parts: any[] = [ + new Blob([Buffer.alloc(SIZE, "A")]), + { + toString() { + // Drop the only reference to the Blob part, collect it, then + // reallocate same-sized buffers so the freed Store bytes get + // clobbered if the joiner still holds a borrowed view into them. + parts.length = 0; + Bun.gc(true); + const clobber = []; + for (let j = 0; j < 8; j++) clobber.push(Buffer.alloc(SIZE, "B")); + return "x"; + }, + }, + ]; + const text = await new Blob(parts).text(); + if (text !== expected) { + throw new Error("Blob part bytes were corrupted at iteration " + i); + } + } + process.stdout.write("OK"); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "run.ts"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout).toBe("OK"); + expect(exitCode).toBe(0); +}); + +test("Blob constructor copies typed array parts before later parts run user code", async () => { + // Constructing a Blob from [typedArray, objectWithToString] must snapshot the + // typed array's bytes when that part is visited. Stringifying a later part + // runs arbitrary user JS (toString / Symbol.toPrimitive / proxy traps) which + // can transfer or resize the earlier part's backing store before the Blob's + // contents are assembled. The resulting Blob must contain the bytes the view + // held at construction time, not whatever ends up at that address afterwards. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const ab = new ArrayBuffer(64, { maxByteLength: 1024 }); + const view = new Uint8Array(ab); + view.fill(0x41); // "A" + const blob = new Blob([ + view, + { + toString() { + // Detach the first part's buffer and overwrite the moved + // backing store while the Blob is still being assembled. + const moved = ab.transfer(); + new Uint8Array(moved).fill(0x42); // "B" + return "tail"; + }, + }, + ]); + const text = await blob.text(); + const expected = Buffer.alloc(64, 0x41).toString() + "tail"; + if (text !== expected) { + throw new Error("unexpected blob contents: " + JSON.stringify(text)); + } + console.log("OK", blob.size); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("OK 68"); + expect(exitCode).toBe(0); +}); diff --git a/test/js/web/fetch/fetch-keepalive.test.ts b/test/js/web/fetch/fetch-keepalive.test.ts index 3cf513c1488f..84aa951dd936 100644 --- a/test/js/web/fetch/fetch-keepalive.test.ts +++ b/test/js/web/fetch/fetch-keepalive.test.ts @@ -1,4 +1,5 @@ import { expect, test } from "bun:test"; +import { tls } from "harness"; test("keepalive", async () => { using server = Bun.serve({ @@ -34,3 +35,40 @@ test("keepalive", async () => { expect(headers.connection).toBe("HELLO!"); } }); + +test("fetch does not reuse a pooled TLS connection for a request with a different Host header", async () => { + using server = Bun.serve({ + port: 0, + tls, + fetch(req) { + // Identify which TCP connection served this request: a reused + // keep-alive socket keeps the same client ephemeral port, while a + // fresh connection must get a new one (the pooled socket still + // occupies the old 4-tuple). + return new Response(String(server.requestIP(req)?.port)); + }, + }); + + const url = `https://localhost:${server.port}/`; + const get = async (headers?: Record) => { + const res = await fetch(url, { + headers, + tls: { rejectUnauthorized: false }, + }); + return await res.text(); + }; + + // Two requests whose TLS handshake used the Host-header override + // "wrong.example" for SNI/certificate verification share one pooled + // connection (legitimate keep-alive still works). + const overrideA = await get({ Host: "wrong.example" }); + const overrideB = await get({ Host: "wrong.example" }); + expect(overrideB).toBe(overrideA); + + // A request without the override expects the server identity to match + // url.hostname ("localhost"), so it must not be handed the connection + // that was only ever negotiated as "wrong.example". It has to open a new + // connection, which cannot have the same client port. + const plain = await get(); + expect(plain).not.toBe(overrideA); +}); diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 1259952deafe..9a099e7f63b5 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -2462,3 +2462,84 @@ it("should allow to follow redirect if connection is closed, abort should work e } } }); + +it("rejects a response with an unparseable Content-Length instead of treating it as empty", async () => { + // RFC 9112 section 6.3: an invalid Content-Length (or duplicate Content-Length + // headers with differing values) is an unrecoverable framing error. Falling + // back to "0" would deliver an empty body and return a desynchronized socket + // to the keep-alive pool with the unread response bytes still in flight, + // where they would be read as the response to the next request. + await using server = net.createServer(socket => { + socket.once("data", data => { + const path = data.toString("utf8").split(" ")[1]; + if (path === "/invalid") { + socket.end( + "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: x\r\nConnection: keep-alive\r\n\r\n" + + "HTTP/1.1 200 OK\r\nContent-Length: 25\r\n\r\ninjected follow-up bytes!", + ); + } else if (path === "/conflicting") { + socket.end( + "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\nContent-Length: 6\r\n\r\nhello!", + ); + } else { + socket.end( + "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\nConnection: close\r\n\r\nhello", + ); + } + }); + }); + await once(server.listen(0, "localhost"), "listening"); + const { port } = server.address() as AddressInfo; + + for (const path of ["invalid", "conflicting"]) { + const result = await fetch(`http://localhost:${port}/${path}`) + .then(res => res.text()) + .catch(e => e); + expect(result).toBeInstanceOf(Error); + expect((result as any).code).toBe("InvalidContentLength"); + } + + // A well-formed Content-Length is still delivered normally. + const ok = await fetch(`http://localhost:${port}/valid`); + expect(await ok.text()).toBe("hello"); +}); + +it("drops a custom Host header when following a cross-origin redirect", async () => { + // A per-request Host override must not survive a change of origin: the + // follow-up request's Host header (and the TLS SNI / certificate identity + // derived from the same field) has to be re-computed from the redirect + // target's URL, not carried over from the previous origin. + await using target = Bun.serve({ + port: 0, + async fetch(request) { + return new Response(request.headers.get("host") ?? ""); + }, + }); + + await using origin = Bun.serve({ + port: 0, + async fetch(request) { + if (new URL(request.url).pathname === "/redirect") { + return new Response(null, { + status: 302, + headers: { "Location": `http://${target.hostname}:${target.port}/landed` }, + }); + } + return new Response(request.headers.get("host") ?? ""); + }, + }); + + // Cross-origin redirect: the redirect target must see its own authority, + // not the caller-supplied Host override naming the previous origin. + const redirected = await fetch(`http://${origin.hostname}:${origin.port}/redirect`, { + headers: { "Host": "tenant.shared-cdn.example" }, + }); + expect(redirected.redirected).toBe(true); + expect(await redirected.text()).toBe(`${target.hostname}:${target.port}`); + + // Without a redirect the explicit Host header is still honored. + const direct = await fetch(`http://${origin.hostname}:${origin.port}/direct`, { + headers: { "Host": "tenant.shared-cdn.example" }, + }); + expect(await direct.text()).toBe("tenant.shared-cdn.example"); +}); diff --git a/test/js/web/fetch/fetch.tls.test.ts b/test/js/web/fetch/fetch.tls.test.ts index 9287133b9be9..e92da8a22148 100644 --- a/test/js/web/fetch/fetch.tls.test.ts +++ b/test/js/web/fetch/fetch.tls.test.ts @@ -29,6 +29,66 @@ async function createServer(cert: TLSOptions, callback: (port: number) => Promis } describe.concurrent("fetch-tls", () => { + it("re-derives the Host header and TLS verification hostname from the redirect target on a cross-origin redirect", async () => { + // The redirect target records the Host header it actually receives. + const receivedHostHeaders: (string | null)[] = []; + using target = Bun.serve({ + port: 0, + tls: CERT_LOCALHOST_IP, + fetch(req) { + receivedHostHeaders.push(req.headers.get("host")); + return new Response("from-target"); + }, + }); + + // The origin issues a cross-origin redirect (different port => different origin). + using origin = Bun.serve({ + port: 0, + tls: CERT_LOCALHOST_IP, + fetch() { + return new Response(null, { + status: 302, + headers: { Location: `https://127.0.0.1:${target.port}/moved` }, + }); + }, + }); + + // An explicit Host header overrides both the wire Host header and the + // hostname used for TLS SNI / certificate verification. checkServerIdentity + // receives the verification hostname as its first argument. + // + // fetch() only invokes the JS checkServerIdentity callback for the + // connection that produced the final response: certificate info delivered + // before response metadata is held (FetchTasklet.on_progress_update returns + // early while metadata is null) and is overwritten by the next hop's + // certificate info when the redirect is followed internally. So a redirect + // chain yields exactly one observation - the verification hostname of the + // connection to the redirect target. + const verifiedHostnames: string[] = []; + const res = await fetch(`https://127.0.0.1:${origin.port}/`, { + keepalive: false, + headers: { Host: "localhost" }, + tls: { + ca: validTls.cert, + checkServerIdentity(hostname: string) { + verifiedHostnames.push(hostname); + return undefined; + }, + }, + }); + expect(await res.text()).toBe("from-target"); + + // The Host override names the previous origin, so on a cross-origin + // redirect it must be dropped and the verification hostname re-derived from + // the redirect target's URL ("127.0.0.1"). The vulnerable behavior carries + // the stale override and verifies the second connection against + // "localhost" instead. + expect(verifiedHostnames).toEqual(["127.0.0.1"]); + // The redirect target must see a Host header derived from its own URL, + // not the override that was supplied for the previous origin. + expect(receivedHostHeaders).toEqual([`127.0.0.1:${target.port}`]); + }); + it("can handle multiple requests with non native checkServerIdentity", async () => { await createServer(CERT_LOCALHOST_IP, async port => { async function request() { diff --git a/test/js/web/html/FormData.test.ts b/test/js/web/html/FormData.test.ts index 61f3be453c81..85cf489250b5 100644 --- a/test/js/web/html/FormData.test.ts +++ b/test/js/web/html/FormData.test.ts @@ -783,3 +783,52 @@ describe("Content-Type header propagation", () => { }); }); }); + +it("drops multipart part Content-Type values containing control characters", async () => { + // A part header line is only terminated by an exact \r\n, so a bare LF can + // survive inside a part's Content-Type value. That value becomes the + // resulting File's `type` and is later written verbatim into outgoing + // request headers, so it must never contain control bytes. + const body = + "--formboundary\r\n" + + "Content-Type: image/png\nX-Injected-Header: injected-value\r\n" + + 'Content-Disposition: form-data; name="evil"; filename="evil.bin"\r\n' + + "\r\n" + + "hello\r\n" + + "--formboundary\r\n" + + "Content-Type: text/plain\r\n" + + 'Content-Disposition: form-data; name="good"; filename="good.txt"\r\n' + + "\r\n" + + "world\r\n" + + "--formboundary\r\n" + + "Content-Type: text/plain;\tcharset=utf-8\r\n" + + 'Content-Disposition: form-data; name="tabbed"; filename="tabbed.txt"\r\n' + + "\r\n" + + "tabbed\r\n" + + "--formboundary--\r\n"; + + const response = new Response(body, { + headers: { "Content-Type": "multipart/form-data; boundary=formboundary" }, + }); + const formData = await response.formData(); + + const evil = formData.get("evil") as File; + expect(evil instanceof Blob).toBe(true); + // The part body itself is preserved; only the malformed Content-Type is discarded. + expect(await evil.text()).toBe("hello"); + expect(evil.type).not.toContain("\n"); + expect(evil.type).not.toContain("\r"); + expect(evil.type.toLowerCase()).not.toContain("x-injected-header"); + + // A well-formed part Content-Type is still honored. + const good = formData.get("good") as File; + expect(good instanceof Blob).toBe(true); + expect(await good.text()).toBe("world"); + expect(good.type).toBe("text/plain"); + + // An interior HTAB is valid optional whitespace, not an injection vector. + const tabbed = formData.get("tabbed") as File; + expect(tabbed instanceof Blob).toBe(true); + expect(await tabbed.text()).toBe("tabbed"); + expect(tabbed.type).toBe("text/plain;\tcharset=utf-8"); +}); diff --git a/test/js/web/timers/setTimeout.test.js b/test/js/web/timers/setTimeout.test.js index eb8f40357b48..243b567818aa 100644 --- a/test/js/web/timers/setTimeout.test.js +++ b/test/js/web/timers/setTimeout.test.js @@ -464,3 +464,68 @@ it("setTimeout does not leak a pending exception when emitting a timeout warning expect(stdout.trim()).toBe("survived"); expect(exitCode).toBe(0); }); + +it("clearTimeout with a numeric id is a no-op after a timeout promoted to an interval is cleared and collected", async () => { + // A setTimeout whose numeric id has been observed via `+timer` registers itself in the + // setTimeout id map. Assigning `_repeat` promotes it to a setInterval after its first + // fire. Once the timer is cleared and its wrapper is collected, the id-map entry must be + // gone from whichever map it was inserted into, so that a later clearTimeout(id) with the + // raw number is a harmless no-op instead of resolving to the freed timer. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + async function main() { + let fires = 0; + let resolveSecondFire; + const secondFire = new Promise(resolve => { + resolveSecondFire = resolve; + }); + let t = setTimeout(() => { + fires++; + if (fires === 2) resolveSecondFire(); + }, 1); + const id = +t; // register the numeric id in the setTimeout id map + t._repeat = 1; // promoted to an interval after the first fire + + // The second fire only happens because the timer became an interval. + await secondFire; + console.log("converted:", fires >= 2 ? "ok" : fires); + + clearInterval(t); + t = null; + Bun.gc(true); + await new Promise(resolve => setImmediate(resolve)); + Bun.gc(true); + + // The numeric id must no longer resolve to the collected timer. + clearTimeout(id); + clearTimeout(id); + clearInterval(id); + console.log("survived"); + } + main().then( + () => {}, + err => { + console.error(err); + process.exit(1); + }, + ); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const stderrLines = stderr + .split("\n") + .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) + .join("\n"); + expect(stderrLines).toBe(""); + expect(stdout).toBe("converted: ok\nsurvived\n"); + expect(exitCode).toBe(0); +}); diff --git a/test/js/web/websocket/websocket-permessage-deflate.test.ts b/test/js/web/websocket/websocket-permessage-deflate.test.ts index be4d618cf9e1..899ae13d06bf 100644 --- a/test/js/web/websocket/websocket-permessage-deflate.test.ts +++ b/test/js/web/websocket/websocket-permessage-deflate.test.ts @@ -248,3 +248,70 @@ test.skip("WebSocket client rejects compressed control frames", async () => { // This test would require a custom server that sends invalid compressed control frames // Skip for now as it requires low-level WebSocket frame manipulation }); + +test("server enforces maxPayloadLength on compressed messages inflated through the fast path", async () => { + // The server limits messages to 1024 bytes. A compressed frame is tiny on the + // wire but can inflate to 4000 bytes, which is over the configured limit yet + // still small enough to fit the inflater's 4096-byte fast-path output buffer. + // The server must drop the connection instead of delivering the oversized + // message to the handler. + const serverReceived: number[] = []; + + using server = serve({ + port: 0, + fetch(req, server) { + if (server.upgrade(req)) { + return; + } + return new Response("Not found", { status: 404 }); + }, + websocket: { + perMessageDeflate: true, + maxPayloadLength: 1024, + message(ws, message) { + const text = typeof message === "string" ? message : message.toString(); + serverReceived.push(text.length); + ws.send(text, true); + }, + }, + }); + + const client = new WebSocket(`ws://localhost:${server.port}`); + + await new Promise((resolve, reject) => { + client.onopen = resolve; + client.onerror = reject; + }); + expect(client.extensions).toContain("permessage-deflate"); + + // Tiny async event queue so we can await each client-side event in order + // without timers. + const events: string[] = []; + let notify = () => {}; + const record = (event: string) => { + events.push(event); + notify(); + }; + const waitForEventCount = async (count: number) => { + while (events.length < count) { + await new Promise(resolve => { + notify = resolve; + }); + } + }; + client.onmessage = event => record(`message:${event.data.length}`); + client.onclose = () => record("close"); + + // A compressible message within the limit is still delivered and echoed back. + client.send(Buffer.alloc(900, "B").toString()); + await waitForEventCount(1); + expect(events[0]).toBe("message:900"); + + // A compressible message that inflates past the limit must not be delivered; + // the server drops the connection instead of echoing it back. + client.send(Buffer.alloc(4000, "A").toString()); + await waitForEventCount(2); + expect(events[1]).toBe("close"); + + expect(serverReceived).toEqual([900]); +}); diff --git a/test/regression/issue/8254.test.ts b/test/regression/issue/8254.test.ts index 76a9195ade58..fac5f13234a9 100644 --- a/test/regression/issue/8254.test.ts +++ b/test/regression/issue/8254.test.ts @@ -14,11 +14,19 @@ test("Bun.write() should write past 2GB boundary without corruption", async () = const NUM_CHUNKS = Math.floor(TWO_GB / CHUNK_SIZE) + 1; const path = join(tmpbase, "large-file.bin"); + // Only 256 distinct fill values exist, so back the >2GB part list with 256 + // shared 1MB buffers instead of 2049 distinct ones. The blob is still >2GB + // and the boundary verification below is unchanged, but peak RSS drops by + // ~2GB, which keeps the test under the CI runners' memory ceiling. + const distinct: Uint8Array[] = []; + for (let i = 0; i < 256; i++) { + const chunk = new Uint8Array(CHUNK_SIZE); + chunk.fill(i); + distinct.push(chunk); + } const chunks: Uint8Array[] = []; for (let i = 0; i < NUM_CHUNKS; i++) { - const chunk = new Uint8Array(CHUNK_SIZE); - chunk.fill(i % 256); - chunks.push(chunk); + chunks.push(distinct[i % 256]); } const blob = new Blob(chunks);