diff --git a/packages/bun-uws/src/HttpParser.h b/packages/bun-uws/src/HttpParser.h index a835bea04f94..3ba319bc37a6 100644 --- a/packages/bun-uws/src/HttpParser.h +++ b/packages/bun-uws/src/HttpParser.h @@ -356,6 +356,13 @@ struct HttpResponseData; // Move past comma if present if (pos < value.length() && value[pos] == ',') { + /* llhttp HPE_INVALID_TRANSFER_ENCODING: any list element after "chunked" + * (including "chunked,", "chunked,chunked") is invalid, never framed as + * chunked — https://github.com/nodejs/llhttp (src/llhttp/http.ts). */ + if (te.chunked) [[unlikely]] { + te.invalid = true; + return te; + } pos++; } } diff --git a/src/js/internal/tls.ts b/src/js/internal/tls.ts index 897a6107407a..ded2a2bfd95b 100644 --- a/src/js/internal/tls.ts +++ b/src/js/internal/tls.ts @@ -135,6 +135,13 @@ function secureProtocolToVersionRange(secureProtocol) { return null; } +// Option-ingestion rule only (node v26.3.0 internal/tls/wrap.js:1368,1686,1762): +// `!== false`, so undefined keeps verification on. Read-sites of the stored +// field use plain truthiness there (:490,:845,:1220) — do not reuse this here. +function normalizeRejectUnauthorized(value) { + return value !== false; +} + let NativeSecureContext; /** @@ -182,6 +189,7 @@ export { VALID_TLS_ERROR_MESSAGE_TYPES, isValidTLSArray, isValidTLSItem, + normalizeRejectUnauthorized, processPfxOptions, secureProtocolToVersionRange, throwOnInvalidTLSArray, diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 38c6bc674baf..6de25d78967a 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -1013,7 +1013,7 @@ const ServerHandlers: SocketHandler = { data.destroy(error); } else if ( data.isServer && - data._rejectUnauthorized && + data._rejectUnauthorized !== false && /peer did not return a certificate/.test(error?.message) ) { // Ignore server's authorization errors diff --git a/src/js/node/tls.ts b/src/js/node/tls.ts index fcfbd5824864..d170eed083a5 100644 --- a/src/js/node/tls.ts +++ b/src/js/node/tls.ts @@ -10,6 +10,7 @@ const { tlsStringToProtocolVersion, secureProtocolToVersionRange, processPfxOptions, + normalizeRejectUnauthorized, validateSecureProtocol, } = require("internal/tls"); const { @@ -604,6 +605,13 @@ function newNativeSecureContext(options, cached = false) { options = { ...options, minVersion, maxVersion }; } } + // Node treats any value other than an explicit `false` as "verify"; the native converter + // only accepts real booleans, so normalize the falsy-but-not-false spellings Node accepts + // (0, "", null) before they can throw or silently disable verification. + const rejectUnauthorized = options.rejectUnauthorized; + if (rejectUnauthorized !== undefined && typeof rejectUnauthorized !== "boolean") { + options = { ...options, rejectUnauthorized: normalizeRejectUnauthorized(rejectUnauthorized) }; + } const ctx = (cached ? NativeSecureContext.intern : NativeSecureContext.createPrivate)(options); if (pfxExtraCAs) { for (const pem of pfxExtraCAs) ctx.addCACert(pem); diff --git a/src/jsc/bindings/node/http/JSConnectionsList.cpp b/src/jsc/bindings/node/http/JSConnectionsList.cpp index 0a2c017f6190..e595c470cb4e 100644 --- a/src/jsc/bindings/node/http/JSConnectionsList.cpp +++ b/src/jsc/bindings/node/http/JSConnectionsList.cpp @@ -89,7 +89,9 @@ JSArray* JSConnectionsList::idle(JSGlobalObject* globalObject) size_t i = 0; while (iter->next(globalObject, item)) { JSHTTPParser* parser = dynamicDowncast(item); - if (!parser) { + // A close()d parser has no impl but stays in the list (remove() after close() + // is a no-op, matching Node); it must be skipped, not dereferenced. + if (!parser || !parser->impl()) { continue; } @@ -143,7 +145,9 @@ JSArray* JSConnectionsList::expired(JSGlobalObject* globalObject, uint64_t heade size_t i = 0; while (iter->next(globalObject, item)) { JSHTTPParser* parser = dynamicDowncast(item); - if (!parser) { + // A close()d parser has no impl but stays in the list (remove() after close() + // is a no-op, matching Node); it must be skipped, not dereferenced. + if (!parser || !parser->impl()) { continue; } diff --git a/src/runtime/api/bun/h2/connection.rs b/src/runtime/api/bun/h2/connection.rs index 78de396b6065..0dce3862f599 100644 --- a/src/runtime/api/bun/h2/connection.rs +++ b/src/runtime/api/bun/h2/connection.rs @@ -1011,7 +1011,8 @@ impl Connection { let cap = (self.enforced_max_header_list_size as usize).max(65536); if self.header_block.len().saturating_add(payload.len()) > cap { // nghttp2's NGHTTP2_MAX_HEADERSLEN (65536) overflow returns NGHTTP2_ERR_HEADER_COMP, - // which node surfaces as a session COMPRESSION_ERROR. + // which node surfaces as a session COMPRESSION_ERROR + // (test-http2-options-max-headers-exceeds-nghttp2.js). self.send_go_away(sink, ErrorCode::CompressionError, b"header block too large"); return true; } @@ -1066,6 +1067,11 @@ impl Connection { let mut saw_connect = false; let mut saw_host = false; let mut informational = false; + // nghttp2 check_path() flags for the RFC 9113 §8.3.1 :path validation. + let mut path_regular = false; + let mut path_asterisk = false; + let mut scheme_http = false; + let mut meth_options = false; let mut content_length: Option = None; while off < block.len() { match self.hpack.decode(&block[off..]) { @@ -1106,11 +1112,14 @@ impl Connection { b"protocol" => pseudo::PROTOCOL, _ => pseudo::UNKNOWN, }; - // 8.3.1: requests never carry :status - a server seeing it inbound is - // a malformed block. (The client direction also constrains pseudo - // headers, but inbound PUSH_PROMISE blocks legitimately carry request - // pseudo-headers, so that check needs the push context first.) - let wrong_direction = self.is_server && rest == b"status"; + // RFC 9113 §8.3.1/§8.3.2: :status only in response blocks, request + // pseudo-headers only in request blocks. Key on `is_request` (not + // is_server) — a client-received PUSH_PROMISE is a request block. + let wrong_direction = if is_request { + bit == pseudo::STATUS + } else { + bit != pseudo::STATUS && bit != pseudo::UNKNOWN + }; // RFC 8441 §4: :protocol is only valid when SETTINGS_ENABLE_CONNECT_PROTOCOL // has been enabled by this endpoint. nghttp2 (and so node) checks the // submitted local value here, not the ACKed one — so a request that arrives @@ -1120,9 +1129,9 @@ impl Connection { let protocol_disabled = self.is_server && rest == b"protocol" && self.local_settings.enable_connect_protocol == 0; - // nghttp2 (check_pseudo_header) treats an empty pseudo-header value as - // malformed, so `:path: ""` never counts as a present :path (§8.3.1: - // `:path` "MUST NOT be empty" for http/https). + // RFC 9113 §8.1: pseudo-headers never appear in a trailer section. + // nghttp2 (check_pseudo_header) also treats an empty pseudo-header + // value as malformed, so `:path: ""` never counts as a present :path. if seen_regular || bit == pseudo::UNKNOWN || (seen_pseudo & bit) != 0 @@ -1137,15 +1146,43 @@ impl Connection { informational = true; } seen_pseudo |= bit; - if rest == b"method" && value_b == b"CONNECT" { - saw_connect = true; + // nghttp2 http_request_on_header: per-field flags for check_path()/ + // nghttp2_http_on_request_headers below; CONNECT on a pushed (even) + // stream is rejected up front ("we won't allow CONNECT for push"). + match rest { + b"method" => { + if value_b == b"CONNECT" { + if push_parent != 0 { + malformed = true; + } + saw_connect = true; + } + meth_options |= value_b == b"OPTIONS"; + } + b"path" => { + path_regular |= value_b.first() == Some(&b'/'); + path_asterisk |= value_b == b"*"; + } + b"scheme" => { + scheme_http |= value_b.eq_ignore_ascii_case(b"http") + || value_b.eq_ignore_ascii_case(b"https"); + } + _ => {} } } else { seen_regular = true; match name_b { b"connection" | b"keep-alive" | b"proxy-connection" | b"transfer-encoding" | b"upgrade" => malformed = true, - b"host" if is_request => saw_host = true, + // nghttp2 http_request_on_header: in request blocks Host is checked + // like :authority (empty/repeated => malformed); in a response it + // is an ordinary field and node delivers it. + b"host" if self.is_server || is_request => { + if value_b.is_empty() || saw_host { + malformed = true; + } + saw_host = true; + } b"te" => { // RFC 9110 10.1.4: field values are case-insensitive. if !value_b.eq_ignore_ascii_case(b"trailers") { @@ -1199,11 +1236,9 @@ impl Connection { sink.on_stream_reset(target, ErrorCode::StreamClosed.as_u32()); return false; } - // RFC 9113 §8.3.1 (nghttp2_http_on_request_headers): a request block needs exactly one - // non-empty :method, :scheme and :path plus an :authority or Host; plain CONNECT omits - // :scheme/:path and carries :authority; extended CONNECT (:protocol, RFC 8441) requires - // :method CONNECT and :authority. Without this a block with an empty or missing :path - // reaches JS as a request with an empty url (no compliant peer can produce that shape). + // RFC 9113 §8.3.1 / nghttp2_http_on_request_headers: request block = :method+:scheme + // +:path + (:authority|Host); plain CONNECT omits :scheme/:path with :authority; + // extended CONNECT (RFC 8441) needs :method CONNECT. Applies to HEADERS & PUSH_PROMISE. if is_request && !rejected && !malformed { use pseudo::{AUTHORITY, METHOD, PATH, PROTOCOL, SCHEME}; let extended_connect = (seen_pseudo & PROTOCOL) != 0; @@ -1213,8 +1248,19 @@ impl Connection { (seen_pseudo & (METHOD | SCHEME | PATH)) != (METHOD | SCHEME | PATH) || ((seen_pseudo & AUTHORITY) == 0 && !saw_host) || (extended_connect && (!saw_connect || (seen_pseudo & AUTHORITY) == 0)) + // nghttp2 check_path(): under http/https, :path must start with '/' + // (or be '*' for OPTIONS). + || (scheme_http && !(path_regular || (meth_options && path_asterisk))) }; - } + } else if !is_trailer && !rejected && !malformed && !informational { + // RFC 9113 §8.3.2 (nghttp2_http_on_response_headers): a final response block must + // carry exactly :status and no request pseudo-header. wrong_direction above already + // rejected a request pseudo per-field; this catches a block with :status omitted. + malformed = (seen_pseudo & pseudo::STATUS) == 0; + } + // RFC 9113 §8.1.1: an inbound request's content-length must be coherent — the declared + // value is attached to the stream and, at END_STREAM, must equal the DATA received + // (plain CONNECT is exempt). if push_parent == 0 && self.is_server && !malformed && !rejected { if let Some(s) = self.streams.get_mut(&target) { if !saw_connect && s.content_length.is_none() { @@ -1229,9 +1275,9 @@ impl Connection { } } if malformed && !rejected { - // node (Http2Session::OnInvalidFrame): every locally-rejected invalid frame counts - // against maxSessionInvalidFrames; exceeding it tears the session down with - // ERR_HTTP2_TOO_MANY_INVALID_FRAMES (same post-increment comparison as node). + // nghttp2 session_handle_invalid_stream2 / RFC 9113 §8.4.1: malformed HEADERS or + // PUSH_PROMISE → RST_STREAM(PROTOCOL_ERROR) on the target id + invalid-frame count. + // node Http2Session::OnInvalidFrame tears down on maxSessionInvalidFrames overflow. let count = self.invalid_frame_count; self.invalid_frame_count = count.saturating_add(1); if count > self.max_invalid_frames { diff --git a/src/runtime/api/bun/h2_frame_parser.rs b/src/runtime/api/bun/h2_frame_parser.rs index ce5e741ff1a6..f6237589b16e 100644 --- a/src/runtime/api/bun/h2_frame_parser.rs +++ b/src/runtime/api/bun/h2_frame_parser.rs @@ -5139,6 +5139,8 @@ impl H2FrameParser { if global.has_exception() { return Some(stream); } + // The `*mut Stream` above stays live across this call into JS. + let _dispatch = self.enter_dispatch(); match callback.call( &global, ctx_value, @@ -5498,6 +5500,34 @@ impl H2FrameParser { }); } + /// Free streams whose legacy lifecycle finished (queued by `free_resources`). Only runs + /// at a quiescent point — no JS dispatch on the stack and no in-progress receive() + /// borrowing the engine cell; otherwise ids stay queued for the next such point. + fn drain_pending_engine_stream_closes(&self) { + if self.dispatch_depth.get() != 0 || self.pending_engine_stream_closes.get().is_empty() { + return; + } + let Ok(mut engine_guard) = self.engine.try_borrow_mut() else { + return; + }; + let Some(engine) = engine_guard.as_mut() else { + return; + }; + self.pending_engine_stream_closes.with_mut(|v| { + for id in v.drain(..) { + engine.close_stream(id); + if let Some(stream) = self.streams.with_mut(|m| m.remove(&id)) { + // SAFETY: sole owner just removed from the map; free_resources already ran; + // dispatch-depth gate above proves no native frame still borrows it; stream + // ids never repeat in a session → frees exactly once. + unsafe { + drop(bun_core::heap::take(stream)); + } + } + } + }); + } + /// Feed inbound bytes through the rewrite engine, buffering the unconsumed tail (design B). fn rewrite_read(&self, bytes: &[u8]) { bun_output::scoped_log!(H2FrameParser, "rewriteRead {}", bytes.len()); @@ -5560,28 +5590,11 @@ impl H2FrameParser { engine.pending_local_settings_acks.push_back(w); } }); - // Streams whose legacy lifecycle finished since the last batch: evict the engine - // entry and free the legacy slot. free_resources already ran for these (it is the - // only producer of this queue); duplicate ids are fine — remove() yields None. - if self.dispatch_depth.get() == 0 { - self.pending_engine_stream_closes.with_mut(|v| { - for id in v.drain(..) { - engine.close_stream(id); - if let Some(stream) = self.streams.with_mut(|m| m.remove(&id)) { - // SAFETY: stream is the heap::alloc'd *mut Stream owned by the - // map entry just removed; free_resources ran when it was queued, - // dispatch_depth == 0 means no caller below us on the stack holds - // a `&mut Stream` across anything that can run user JS (every - // such site arms enter_dispatch), ids never repeat within a - // session, so this frees exactly once. - unsafe { - drop(bun_core::heap::take(stream)); - } - } - } - }); - } } + // Streams whose legacy lifecycle finished since the last batch: evict the engine entry + // and free the legacy slot at this quiescent point (the helper enforces the safety + // rules; deferred ids are also reclaimed at the next host-call boundary). + self.drain_pending_engine_stream_closes(); if self.rewrite_tail.get().is_empty() { let feed = { let mut guard = self.engine.borrow_mut(); @@ -7045,6 +7058,9 @@ impl H2FrameParser { callframe: &CallFrame, ) -> JsResult { bun_output::scoped_log!(H2FrameParser, "rstStream"); + // Quiescent host-call boundary: reclaim deferred stream closes before this frame + // materializes any `*mut Stream`. + this.drain_pending_engine_stream_closes(); let [stream_arg, error_arg] = callframe.arguments_as_array::<2>(); if callframe.arguments_count() < 2 { return Err(global_object.throw(format_args!("Expected stream and code arguments"))); @@ -7912,6 +7928,10 @@ impl H2FrameParser { defer_callback_arg, ] = args.ptr; + // Quiescent host-call boundary: reclaim deferred stream closes before this frame + // materializes any `*mut Stream`. + this.drain_pending_engine_stream_closes(); + if !stream_arg.is_number() { return Err(global_object.throw(format_args!("Expected stream to be a number"))); } @@ -7956,9 +7976,18 @@ impl H2FrameParser { } }; - let buffer = match StringOrBuffer::from_js_with_encoding(global_object, data_arg, encoding)? - { - Some(b) => b, + // send_data can re-enter JS mid-payload (batch flushes, prior writes' callbacks): pin + // + protect ArrayBuffer payloads so they can't be detached under the borrowed slice. + // Strings are immutable (zero-copy path); ThreadSafe's Drop releases the pin/protect. + let pin_payload = data_arg.is_cell() && data_arg.js_type().is_array_buffer_like(); + let buffer = match StringOrBuffer::from_js_with_encoding_maybe_async( + global_object, + data_arg, + encoding, + pin_payload, + true, + )? { + Some(b) => bun_jsc::ThreadSafe::adopt(b), None => { return Err(global_object.throw_invalid_argument_type_value( b"write", @@ -8347,6 +8376,8 @@ impl H2FrameParser { }; let mut _count: u32 = 0; let mut it = StreamResumableIterator::init(this); + // The iterator's `*mut Stream`s stay live across the callbacks below. + let _dispatch = this.enter_dispatch(); while let Some(stream) = it.next() { // SAFETY: stream is *mut Stream from self.streams; valid while the map entry exists let Some(value) = (unsafe { (*stream).js_context.get() }) else { diff --git a/src/runtime/server/NodeHTTPResponse.rs b/src/runtime/server/NodeHTTPResponse.rs index 86e0db34a795..a129a6255184 100644 --- a/src/runtime/server/NodeHTTPResponse.rs +++ b/src/runtime/server/NodeHTTPResponse.rs @@ -289,6 +289,19 @@ fn err_throw(global: &JSGlobalObject, code: ErrorCode, msg: &'static str) -> Err(err_throw_cold(global, code, msg)) } +/// Terminate a node:http response whose handler failed (threw or rejected). The wire must +/// never read as a complete success: status/body bytes already sent → close without valid +/// framing (RFC 9112 §7); nothing sent yet → a well-formed 500. +pub(crate) fn end_failed_node_http_response(raw: uws::AnyResponse, close_connection: bool) { + let state = raw.state(); + if state.is_http_status_called() || state.is_http_write_called() { + raw.force_close(); + } else { + raw.write_status(b"500 Internal Server Error"); + raw.end_stream(close_connection); + } +} + /// AnyResponse `is_ssl()` shim (upstream lacks this accessor). #[inline] fn any_response_is_ssl(r: &uws::AnyResponse) -> bool { @@ -495,16 +508,27 @@ impl NodeHTTPResponse { raw.resume(); } + /// Every precondition under which [`Self::upgrade`] refuses. Callers that must not commit + /// the one-shot 101 preamble to the socket for an upgrade that will fail check this first; + /// `upgrade()` itself starts with it, so the two can never drift. + pub(crate) fn can_upgrade(&self) -> bool { + // `AnyServer` is a `Copy` type-erased pointer to the long-lived server, not `*self`. + let mut server = self.server; + !self.upgrade_context.get().context.is_null() + && server.web_socket_handler().is_some() + && !self.get_server_socket_value().is_empty() + } + pub(crate) fn upgrade( &self, data_value: JSValue, sec_websocket_protocol: ZigString, sec_websocket_extensions: ZigString, ) -> bool { - let upgrade_ctx = self.upgrade_context.get().context; - if upgrade_ctx.is_null() { + if !self.can_upgrade() { return false; } + let upgrade_ctx = self.upgrade_context.get().context; // `AnyServer` is a `Copy` type-erased pointer; copy it so the // `&mut self`-taking accessor can be called from this `&self` body. // The pointee is the long-lived server, not `*self`. @@ -516,10 +540,6 @@ impl NodeHTTPResponse { // SAFETY: JS-thread only; the server (and its websocket config) outlives this call. let ws_handler: &mut crate::server::WebSocketServerHandler = unsafe { &mut *std::ptr::from_mut(ws_handler) }; - let socket_value = self.get_server_socket_value(); - if socket_value.is_empty() { - return false; - } self.resume_socket(); data_value.ensure_still_alive(); @@ -1535,10 +1555,10 @@ fn node_http_request_on_reject(global_object: &JSGlobalObject, callframe: &CallF raw_response.clear_on_data(); raw_response.clear_on_writable(); raw_response.clear_timeout(); - if !raw_response.state().is_http_status_called() { - raw_response.write_status(b"500 Internal Server Error"); - } - raw_response.end_stream(raw_response.state().is_http_connection_close()); + end_failed_node_http_response( + raw_response, + raw_response.state().is_http_connection_close(), + ); } this.on_request_complete(); diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 29e6bc1322b7..16395c38897e 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1386,12 +1386,7 @@ impl NewServer { if !nhr_flags.contains(NhrFlags::REQUEST_HAS_COMPLETED) && raw.state().is_response_pending() { - if raw.state().is_http_status_called() { - raw.write_status(b"500 Internal Server Error"); - raw.end_without_body(true); - } else { - raw.end_stream(true); - } + node_http_response::end_failed_node_http_response(raw, true); } } } diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 8e4c1e20bf90..64a97ed8cc78 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -1796,6 +1796,12 @@ where { return Ok(JSValue::FALSE); } + // upgrade() below cannot succeed for a request uWS never classified as a WebSocket + // upgrade: bail before the one-shot 101 status + headers are committed to the + // socket, so the app's fallback response stays well-formed (see #1339). + if !node_http_response.can_upgrade() { + return Ok(JSValue::FALSE); + } let mut data_value = JSValue::ZERO; @@ -1895,6 +1901,15 @@ where fetch_headers_to_use .fast_remove(HTTPHeaderName::SecWebSocketExtensions); } + // Option getters ran user JS (res.end()/destroy()/re-entrant upgrade() + // may have fired): re-check the guards (mirrors the native path below) + // so the one-shot 101 preamble isn't committed for a refusing upgrade(). + if node_http_response.flags.get().intersects( + NodeHTTPResponseFlags::ENDED | NodeHTTPResponseFlags::SOCKET_CLOSED, + ) || !node_http_response.can_upgrade() + { + return Ok(JSValue::FALSE); + } if let Some(raw_response) = node_http_response.raw_response.get() { // we must write the status first so that 200 OK isn't written raw_response.write_status(b"101 Switching Protocols"); diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index 4dac60f9de5d..aa0d3ad1a36f 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -930,21 +930,34 @@ impl FileSink { // contexts: no touching live JS cells (sweep), and no tearing down // state that in-flight IO still needs (close). - // Shutdown never unwinds the writer: the loop stops ticking, so the - // `onWrite`/`onClose`/EOF callbacks that balance these refs can no - // longer arrive, and a queued FlushPendingFileSinkTask never runs. - // Release them here (a piped stdout whose write once returned - // `.pending` otherwise strands its keep-alive ref forever and the sink - // leaks). Only under `is_shutting_down`: on a live VM those events - // still arrive and must keep the sink alive past the wrapper. + // Under `is_shutting_down` the loop stops ticking: onWrite/onClose/EOF and queued + // FlushPendingFileSinkTask never arrive to balance these refs, so release them here + // (else e.g. a piped stdout with a `.pending` write strands its keep-alive forever). if let Some(vm) = self.js_vm() { if vm.is_shutting_down() { let this = std::ptr::from_mut::(self); - // SAFETY: `this` is the canonical allocation pointer (finalize - // receives the wrapper's `m_ctx`); the wrapper's +1 is still - // held until the trailing `deref` below, so neither release - // can free `this` mid-body. `clear_keep_alive_ref` is - // flag-gated, so a (theoretical) late `onClose` is a no-op. + // SAFETY: `this` is the canonical alloc (finalize's `m_ctx`); the wrapper's +1 + // is held until the trailing `deref` so neither release frees `this` mid-body. + // `clear_keep_alive_ref` is flag-gated, so a late `onClose` is a no-op. + unsafe { FileSink::clear_keep_alive_ref(this) }; + if self.run_pending_later.has.get() { + self.run_pending_later.has.set(false); + // SAFETY: as above; balances the `ref_()` taken in + // `run_pending_later()` for a task that will never run. + unsafe { FileSink::deref(this) }; + } + } + } + + // Under `is_shutting_down` the loop stops ticking: onWrite/onClose/EOF and queued + // FlushPendingFileSinkTask never arrive to balance these refs, so release them here + // (else e.g. a piped stdout with a `.pending` write strands its keep-alive forever). + if let Some(vm) = self.js_vm() { + if vm.is_shutting_down() { + let this = std::ptr::from_mut::(self); + // SAFETY: `this` is the canonical alloc (finalize's `m_ctx`); the wrapper's +1 + // is held until the trailing `deref` so neither release frees `this` mid-body. + // `clear_keep_alive_ref` is flag-gated, so a late `onClose` is a no-op. unsafe { FileSink::clear_keep_alive_ref(this) }; if self.run_pending_later.has.get() { self.run_pending_later.has.set(false); diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index 1f779eae9f32..d1ca44f7152e 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -81,6 +81,44 @@ test("rejects Transfer-Encoding with chunked not last", async () => { }); }); +// llhttp (and so node) reject on the comma itself once "chunked" has been seen +// (HPE_INVALID_TRANSFER_ENCODING) — a following coding, empty list element, or trailing +// comma are all invalid rather than framed as chunked (a smuggling differential). +test.each(["chunked, chunked", "chunked, foo, chunked", "chunked,", "chunked, ", "chunked,,foo"])( + "rejects a comma after chunked in a single Transfer-Encoding header (%s)", + async te => { + await using server = Bun.serve({ + port: 0, + fetch(req) { + return new Response("OK"); + }, + }); + + const client = net.connect(server.port, "127.0.0.1"); + + const maliciousRequest = [ + "POST / HTTP/1.1", + "Host: localhost", + `Transfer-Encoding: ${te}`, + "", + "1", + "A", + "0", + "", + "", + ].join("\r\n"); + + const response = await new Promise((resolve, reject) => { + let raw = ""; + client.on("error", reject); + client.on("data", data => (raw += data.toString())); + client.on("close", () => resolve(raw)); + client.write(maliciousRequest); + }); + expect(response).toContain("HTTP/1.1 400"); + }, +); + test("rejects duplicate chunked in Transfer-Encoding", async () => { await using server = Bun.serve({ port: 0, diff --git a/test/js/node/http/node-http-parser.test.ts b/test/js/node/http/node-http-parser.test.ts index e89783fe5fc3..9e385e3c3cf6 100644 --- a/test/js/node/http/node-http-parser.test.ts +++ b/test/js/node/http/node-http-parser.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; const { HTTPParser, ConnectionsList, methods, allMethods } = process.binding("http_parser"); const { parsers } = require("node:_http_common"); @@ -263,6 +264,33 @@ describe("ConnectionsList", () => { // to remove it. expect(list.all()).toEqual([p1, p4, p3]); }); + + // close() frees the impl but leaves the parser in the list (see above): the idle()/expired() + // sweeps must skip it instead of dereferencing the freed impl. Spawned because the unfixed + // failure mode is a segfault, which would take down the test runner itself. + test("idle() and expired() skip a closed parser still in the list", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const { HTTPParser, ConnectionsList } = process.binding("http_parser"); + const list = new ConnectionsList(); + const p = new HTTPParser(); + p.initialize(HTTPParser.REQUEST, {}, 0, 0, list); + p.close(); + if (JSON.stringify(list.idle()) !== "[]") throw new Error("idle"); + if (JSON.stringify(list.expired(1, 1)) !== "[]") throw new Error("expired"); + if (list.all().length !== 1) throw new Error("all"); + console.log("OK");`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe("OK\n"); + expect(exitCode).toBe(0); + }); }); describe("parserOnHeaders maxHeaderPairs clamp (nodejs/node#61285)", () => { diff --git a/test/js/node/http/node-http-transfer-encoding.test.ts b/test/js/node/http/node-http-transfer-encoding.test.ts index 2d011563c854..832e59f1e2a9 100644 --- a/test/js/node/http/node-http-transfer-encoding.test.ts +++ b/test/js/node/http/node-http-transfer-encoding.test.ts @@ -3,6 +3,41 @@ import { once } from "events"; import { createServer, request } from "http"; import { AddressInfo, connect, Server } from "net"; +// node treats maxHeaderSize: 0 as "use the default limit", never "unlimited": a chunked +// trailer section that never terminates must error the connection, not buffer unboundedly. +// The 1024 variant proves the same cap applies when an explicit limit is configured. +test.each([0, 1024])("chunked trailer section is bounded when maxHeaderSize is %d", async maxHeaderSize => { + let sawRequestEnd = false; + const server = createServer({ maxHeaderSize }, (req, res) => { + req.resume(); + req.on("end", () => { + sawRequestEnd = true; + res.end("done"); + }); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + try { + const socket = connect((server.address() as AddressInfo).port, "127.0.0.1"); + await once(socket, "connect"); + let response = ""; + socket.on("data", chunk => (response += chunk.toString())); + socket.on("error", () => {}); + const closed = once(socket, "close"); + socket.write("POST / HTTP/1.1\r\nHost: a\r\nTransfer-Encoding: chunked\r\n\r\n0\r\n"); + // Trailer lines with no terminating blank line, far past the 16 KiB default cap. + const junkLine = Buffer.from("x-trailer-flood: yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy\r\n"); + for (let sent = 0; sent < 64 * 1024; sent += junkLine.length) { + socket.write(junkLine); + } + await closed; + expect(response).not.toContain("HTTP/1.1 200"); + expect(sawRequestEnd).toBe(false); + } finally { + server.close(); + } +}); + const fixture = "node-http-transfer-encoding-fixture.ts"; test(`should not duplicate transfer-encoding header in request`, async () => { const { resolve, promise } = Promise.withResolvers(); diff --git a/test/js/node/http/node-http-with-ws.test.ts b/test/js/node/http/node-http-with-ws.test.ts index a3ef8cac6a29..fb3bb1e64ff7 100644 --- a/test/js/node/http/node-http-with-ws.test.ts +++ b/test/js/node/http/node-http-with-ws.test.ts @@ -1,7 +1,8 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe, tls as options } from "harness"; import https from "https"; -import type { AddressInfo } from "node:net"; +import http from "node:http"; +import net, { type AddressInfo } from "node:net"; import tls from "tls"; import { WebSocketServer } from "ws"; @@ -59,6 +60,44 @@ test.concurrent("WebSocket upgrade should unref poll_ref from response", async ( expect(exitCode).toBe(0); }); +test.concurrent("server.upgrade(res) on a plain request returns false without corrupting the response", async () => { + // A plain GET has no uWS upgrade context, so upgrade() must refuse BEFORE the one-shot + // 101 preamble and the caller-supplied headers are committed to the socket — otherwise the + // app's documented fallback response is appended after a bogus 101 (a desynced exchange). + const kBunInternals = Symbol.for("::bunternal::"); + let upgradeResult: boolean | undefined; + const server = http.createServer((req, res) => { + const bunServer = (server as any)[kBunInternals]; + const handle = (req.socket as any)[kBunInternals]; + upgradeResult = bunServer.upgrade(handle, { headers: { "x-should-not-appear": "1" } }); + res.writeHead(200, { "content-type": "text/plain" }); + res.end("ok"); + }); + const { promise, resolve, reject } = Promise.withResolvers(); + server.listen(0, "127.0.0.1", () => { + const port = (server.address() as AddressInfo).port; + const socket = net.connect(port, "127.0.0.1", () => { + socket.write(`GET / HTTP/1.1\r\nHost: localhost:${port}\r\nConnection: close\r\n\r\n`); + }); + socket.setEncoding("latin1"); + let raw = ""; + socket.on("data", chunk => (raw += chunk)); + socket.on("error", reject); + socket.on("close", () => resolve(raw)); + }); + try { + const raw = await promise; + expect(upgradeResult).toBe(false); + expect(raw).toStartWith("HTTP/1.1 200 "); + expect(raw).not.toContain("101 Switching Protocols"); + expect(raw).not.toContain("x-should-not-appear"); + expect(raw.match(/HTTP\/1\.1 /g)).toHaveLength(1); + expect(raw.split("\r\n\r\n").at(-1)).toBe("ok"); + } finally { + server.close(); + } +}); + test.concurrent("should not crash when closing sockets after upgrade", async () => { const { promise, resolve } = Promise.withResolvers(); let http_sockets: tls.TLSSocket[] = []; diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index a204e6d37259..0b1a7d601780 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -1479,6 +1479,74 @@ it("should propagate exception in async data handler", async () => { expect(exitCode).toBe(0); }); +// A failing 'request' listener must never look like a success on the wire: nothing written +// yet => 500; headers/body already written => the framing is never completed, so clients, +// caches and gateways cannot mistake the failure for a complete response. +describe("a 'request' listener that fails", () => { + async function rawExchangeWithThrowingHandler( + handlerBody: string, + request = "GET / HTTP/1.1\\r\\nHost: a\\r\\nConnection: close\\r\\n\\r\\n", + ) { + const script = ` + process.on("uncaughtException", () => {}); + const http = require("node:http"); + const net = require("node:net"); + const server = http.createServer((req, res) => { + ${handlerBody} + }); + server.listen(0, "127.0.0.1", () => { + const socket = net.connect(server.address().port, "127.0.0.1", () => { + socket.write("${request}"); + }); + let raw = ""; + socket.setEncoding("latin1"); + socket.on("data", c => (raw += c)); + socket.on("error", () => {}); + socket.on("close", () => { + console.log(JSON.stringify({ raw })); + server.close(); + }); + }); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + // Drain stderr without asserting it is empty: debug/ASAN builds emit benign warnings there. + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(exitCode).toBe(0); + return JSON.parse(stdout).raw as string; + } + + it("throwing with nothing written responds 500, never an empty 200", async () => { + const raw = await rawExchangeWithThrowingHandler(`throw new Error("boom");`); + expect(raw).toStartWith("HTTP/1.1 500 "); + expect(raw).not.toContain("HTTP/1.1 200"); + }); + + it("throwing after body bytes were written never completes or corrupts the chunked framing", async () => { + const raw = await rawExchangeWithThrowingHandler( + ` + res.writeHead(200, { "Content-Type": "text/plain" }); + res.write("partial"); + res.flushHeaders(); + throw new Error("boom"); + `, + // keep-alive request: nothing but the handler failure may end this exchange + "GET / HTTP/1.1\\r\\nHost: a\\r\\n\\r\\n", + ); + // RFC 9112 section 7: no terminating chunk may follow a failed handler, and nothing + // (like a stray Connection: close header) may be injected into the chunked body — a + // truncated body must never read as a complete or differently-framed response. + expect(raw).not.toContain("\r\n0\r\n\r\n"); + const body = raw.split("\r\n\r\n").slice(1).join("\r\n\r\n"); + const afterFirstChunk = body.replace(/^7\r\npartial\r\n/, ""); + expect(afterFirstChunk === "" || /^[0-9a-fA-F]+[;\r]/.test(afterFirstChunk)).toBe(true); + }); +}); + // This test is disabled because it can OOM the CI it.skip("should be able to stream huge amounts of data", async () => { const buf = Buffer.alloc(1024 * 1024 * 256); diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index edd266b68268..f9f7c42da360 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -2733,6 +2733,72 @@ it("http2 server rejects requests carrying connection-specific or repeated pseud Buffer.from([0x01]), // :authority literal("localhost"), ]), + // RFC 9113 Section 8.3.1 (nghttp2_http_on_request_headers): :method, :scheme and :path + // are all mandatory (plus an :authority or Host), an empty pseudo-header value never + // counts as present, and plain CONNECT must omit :scheme/:path. nghttp2 (and so node) + // answers each of these with a stream PROTOCOL_ERROR before the request reaches JS. + "missing :method pseudo-header": Buffer.concat([ + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x84]), // :path: / + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + "missing :path pseudo-header": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + "missing :scheme pseudo-header": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x84]), // :path: / + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + "empty :path pseudo-header value": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x04]), // :path (literal without indexing, name index 4) + literal(""), + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + "no pseudo-headers at all": Buffer.concat([ + Buffer.from([0x00]), // literal header field without indexing, new name + literal("x-plain"), + literal("header"), + ]), + "missing :authority and host": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x84]), // :path: / + ]), + "CONNECT carrying :scheme and :path": Buffer.concat([ + Buffer.from([0x02]), // :method (literal without indexing, name index 2) + literal("CONNECT"), + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x84]), // :path: / + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + // nghttp2 check_path(): under an http/https scheme :path must start with '/', or be '*' + // for OPTIONS. `:path: foo` and `:method GET, :path *` are both PROTOCOL_ERROR. + ":path without leading slash": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x04]), // :path (literal without indexing, name index 4) + literal("foo"), + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + ":path * with non-OPTIONS method": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x04]), // :path + literal("*"), + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), }; async function exchange(headerBlock) { @@ -2812,6 +2878,440 @@ it("http2 server rejects requests carrying connection-specific or repeated pseud } }); +// RFC 8441 (nghttp2_http_on_request_headers extended-connect branch): once the server has +// advertised SETTINGS_ENABLE_CONNECT_PROTOCOL, `:protocol` still requires :method CONNECT +// and an :authority (a `host` header does not substitute here). +it("http2 server rejects malformed extended-CONNECT requests", async () => { + const deliveredRequests = []; + const server = http2.createServer({ settings: { enableConnectProtocol: true } }); + server.on("stream", (stream, headers) => { + deliveredRequests.push(headers); + stream.respond({ ":status": 200 }); + stream.end("ok"); + }); + const { promise: listening, resolve: onListening } = Promise.withResolvers(); + server.listen(0, "127.0.0.1", onListening); + await listening; + const port = server.address().port; + + const literal = str => { + const bytes = Buffer.from(str, "latin1"); + return Buffer.concat([Buffer.from([bytes.length]), bytes]); + }; + const cases = { + ":protocol on a non-CONNECT method": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x84]), // :path: / + Buffer.from([0x01]), // :authority + literal("localhost"), + Buffer.from([0x00]), + literal(":protocol"), + literal("websocket"), + ]), + "extended CONNECT with host but no :authority": Buffer.concat([ + Buffer.from([0x02]), // :method + literal("CONNECT"), + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x84]), // :path: / + Buffer.from([0x00]), + literal(":protocol"), + literal("websocket"), + Buffer.from([0x00]), + literal("host"), + literal("localhost"), + ]), + }; + const wellFormed = Buffer.concat([ + Buffer.from([0x02]), // :method + literal("CONNECT"), + Buffer.from([0x86]), // :scheme: http + Buffer.from([0x84]), // :path: / + Buffer.from([0x01]), // :authority + literal("localhost"), + Buffer.from([0x00]), + literal(":protocol"), + literal("websocket"), + ]); + + async function exchange(headerBlock) { + const frames = []; + const { promise: exchanged, resolve: onExchanged, reject: onSocketError } = Promise.withResolvers(); + const socket = net.connect(port, "127.0.0.1", () => { + socket.write(http2utils.kClientMagic); + socket.write(new http2utils.SettingsFrame(false).data); + socket.write(new http2utils.HeadersFrame(1, headerBlock, 0, true, true).data); + socket.write(new http2utils.PingFrame(false).data); + }); + socket.on("error", onSocketError); + let received = Buffer.alloc(0); + socket.on("data", chunk => { + received = Buffer.concat([received, chunk]); + while (received.length >= 9) { + const length = received.readUIntBE(0, 3); + if (received.length < 9 + length) break; + const frame = { + type: received[3], + flags: received[4], + streamId: received.readUInt32BE(5) & 0x7fffffff, + payload: Buffer.from(received.subarray(9, 9 + length)), + }; + received = received.subarray(9 + length); + frames.push(frame); + if ((frame.type === 6 && (frame.flags & 1) !== 0) || frame.type === 7) { + onExchanged(); + return; + } + } + }); + socket.on("close", () => onExchanged()); + try { + await exchanged; + } finally { + socket.destroy(); + } + return frames; + } + + try { + for (const [caseName, headerBlock] of Object.entries(cases)) { + const frames = await exchange(headerBlock); + expect({ caseName, delivered: deliveredRequests.length }).toEqual({ caseName, delivered: 0 }); + const rst = frames.find(f => f.type === 3 && f.streamId === 1); + expect({ caseName, rstCode: rst?.payload?.readUInt32BE(0) }).toEqual({ + caseName, + rstCode: http2.constants.NGHTTP2_PROTOCOL_ERROR, + }); + } + // Positive control: a well-formed extended CONNECT still reaches the application, so the + // rejections above are the extended_connect clause and not protocol_disabled. + const frames = await exchange(wellFormed); + expect(frames.find(f => f.type === 3 && f.streamId === 1)).toBeUndefined(); + expect(deliveredRequests.length).toBe(1); + expect(deliveredRequests[0][":protocol"]).toBe("websocket"); + } finally { + server.close(); + } +}); + +// RFC 9113 §8.3.2 (nghttp2_http_on_response_headers): a response block must carry :status +// and must not carry a request pseudo-header. node RST_STREAMs and never emits 'response'. +it("http2 client rejects a response missing :status or carrying a request pseudo-header", async () => { + const literal = str => { + const bytes = Buffer.from(str, "latin1"); + return Buffer.concat([Buffer.from([bytes.length]), bytes]); + }; + const cases = { + "no pseudo-headers": Buffer.concat([Buffer.from([0x00]), literal("x-foo"), literal("bar")]), + ":method in a response": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x00]), + literal("x-foo"), + literal("bar"), + ]), + }; + for (const [caseName, responseBlock] of Object.entries(cases)) { + const { promise: serverListening, resolve: onListening } = Promise.withResolvers(); + const server = net.createServer(socket => { + let received = Buffer.alloc(0); + let sawPreface = false; + let responded = false; + socket.write(new http2utils.SettingsFrame(false).data); + socket.on("data", chunk => { + received = Buffer.concat([received, chunk]); + if (!sawPreface) { + if (received.length < http2utils.kClientMagic.length) return; + received = received.subarray(http2utils.kClientMagic.length); + sawPreface = true; + } + while (received.length >= 9) { + const length = received.readUIntBE(0, 3); + if (received.length < 9 + length) break; + const type = received[3]; + const flags = received[4]; + const streamId = received.readUInt32BE(5) & 0x7fffffff; + received = received.subarray(9 + length); + if (type === 4 && (flags & 1) === 0) socket.write(new http2utils.SettingsFrame(true).data); + if (type === 1 && !responded) { + responded = true; + socket.write(new http2utils.HeadersFrame(streamId, responseBlock, 0, true, true).data); + } + } + }); + }); + server.listen(0, "127.0.0.1", () => onListening()); + await serverListening; + + let client; + try { + const { promise: closed, resolve: onClose, reject: onError } = Promise.withResolvers(); + const responses = []; + client = http2.connect(`http://127.0.0.1:${server.address().port}`); + client.on("error", () => {}); + const req = client.request({ ":path": "/" }); + req.on("response", headers => responses.push(headers)); + req.on("error", () => {}); + req.on("close", onClose); + req.resume(); + req.end(); + await closed; + expect({ caseName, responses: responses.length, rstCode: req.rstCode }).toEqual({ + caseName, + responses: 0, + rstCode: http2.constants.NGHTTP2_PROTOCOL_ERROR, + }); + } finally { + client?.close(); + server.close(); + } + } +}); + +// The Host rules above only apply to request blocks: nghttp2's http_response_on_header has +// no `host` case, so a response carrying an empty or repeated Host header is delivered to +// the client (node keeps the first value) instead of being answered with RST_STREAM. +it("http2 client delivers a response carrying an empty or repeated host header", async () => { + const literal = str => { + const bytes = Buffer.from(str, "latin1"); + return Buffer.concat([Buffer.from([bytes.length]), bytes]); + }; + const responseBlock = Buffer.concat([ + Buffer.from([0x88]), // :status: 200 (static table index 8) + Buffer.from([0x00]), // literal header field without indexing, new name + literal("host"), + literal(""), + Buffer.from([0x00]), + literal("host"), + literal("dup"), + ]); + const { promise: serverListening, resolve: onListening } = Promise.withResolvers(); + const server = net.createServer(socket => { + let received = Buffer.alloc(0); + let sawPreface = false; + let responded = false; + socket.write(new http2utils.SettingsFrame(false).data); + socket.on("data", chunk => { + received = Buffer.concat([received, chunk]); + if (!sawPreface) { + if (received.length < http2utils.kClientMagic.length) return; + received = received.subarray(http2utils.kClientMagic.length); + sawPreface = true; + } + while (received.length >= 9) { + const length = received.readUIntBE(0, 3); + if (received.length < 9 + length) break; + const type = received[3]; + const flags = received[4]; + const streamId = received.readUInt32BE(5) & 0x7fffffff; + received = received.subarray(9 + length); + if (type === 4 && (flags & 1) === 0) socket.write(new http2utils.SettingsFrame(true).data); + // Answer the client's HEADERS with END_HEADERS | END_STREAM response headers. + if (type === 1 && !responded) { + responded = true; + socket.write(new http2utils.HeadersFrame(streamId, responseBlock, 0, true, true).data); + } + } + }); + }); + server.listen(0, "127.0.0.1", () => onListening()); + await serverListening; + + let client; + try { + const { promise: closed, resolve: onClose, reject: onError } = Promise.withResolvers(); + const { promise: responded, resolve: onResponse } = Promise.withResolvers(); + client = http2.connect(`http://127.0.0.1:${server.address().port}`); + client.on("error", onError); + const req = client.request({ ":path": "/" }); + req.on("response", onResponse); + req.on("error", onError); + req.on("close", onClose); + req.resume(); + req.end(); + const headers = await responded; + await closed; + expect(headers[":status"]).toBe(200); + expect(headers.host).toBe(""); + expect(req.rstCode).toBe(http2.constants.NGHTTP2_NO_ERROR); + } finally { + client?.close(); + server.close(); + } +}); + +// A received PUSH_PROMISE is a request block: nghttp2 finalizes it with +// nghttp2_http_on_request_headers and answers a malformed one with RST_STREAM(PROTOCOL_ERROR) +// on the promised stream (nghttp2_session.c session_handle_invalid_stream2) — the session +// stays alive and the parent request still completes. nghttp2 also rejects :method CONNECT +// in a promised request per-header ("we won't allow CONNECT for push"). +it("http2 client rejects a malformed PUSH_PROMISE with RST_STREAM and keeps the session alive", async () => { + const literal = str => { + const bytes = Buffer.from(str, "latin1"); + return Buffer.concat([Buffer.from([bytes.length]), bytes]); + }; + const cases = { + valid: Buffer.concat([ + Buffer.from([0x82]), // :method: GET (static table index 2) + Buffer.from([0x86]), // :scheme: http (static table index 6) + Buffer.from([0x01]), // :authority (literal without indexing, name index 1) + literal("localhost"), + Buffer.from([0x04]), // :path (literal without indexing, name index 4) + literal("/pushed"), + ]), + "missing :scheme and :authority": Buffer.concat([ + Buffer.from([0x82]), // :method: GET + Buffer.from([0x04]), // :path + literal("/pushed"), + ]), + ":method CONNECT in a promised request": Buffer.concat([ + Buffer.from([0x02]), // :method (literal without indexing, name index 2) + literal("CONNECT"), + Buffer.from([0x01]), // :authority + literal("localhost"), + ]), + }; + for (const [caseName, pushBlock] of Object.entries(cases)) { + const clientFrames = []; + const { promise: listening, resolve: onListening } = Promise.withResolvers(); + const server = net.createServer(socket => { + let received = Buffer.alloc(0); + let sawPreface = false; + let responded = false; + socket.write(new http2utils.SettingsFrame(false).data); + socket.on("data", chunk => { + received = Buffer.concat([received, chunk]); + if (!sawPreface) { + if (received.length < http2utils.kClientMagic.length) return; + received = received.subarray(http2utils.kClientMagic.length); + sawPreface = true; + } + while (received.length >= 9) { + const length = received.readUIntBE(0, 3); + if (received.length < 9 + length) break; + const type = received[3]; + const flags = received[4]; + const streamId = received.readUInt32BE(5) & 0x7fffffff; + const payload = Buffer.from(received.subarray(9, 9 + length)); + received = received.subarray(9 + length); + clientFrames.push({ type, streamId, payload }); + if (type === 4 && (flags & 1) === 0) socket.write(new http2utils.SettingsFrame(true).data); + // Answer the client's PING so client.ping() can serve as a barrier below. + if (type === 6 && (flags & 1) === 0) + socket.write(Buffer.concat([new http2utils.Frame(8, 6, 0x1, 0).data, payload])); + if (type === 1 && !responded) { + responded = true; + // PUSH_PROMISE reserving stream 2 for the client's stream, then the stream response. + const promised = Buffer.alloc(4); + promised.writeUInt32BE(2, 0); + const pp = Buffer.concat([promised, pushBlock]); + socket.write(Buffer.concat([new http2utils.Frame(pp.length, 5, 0x4, streamId).data, pp])); + socket.write(new http2utils.HeadersFrame(streamId, Buffer.from([0x88]), 0, true, true).data); + } + } + }); + }); + server.listen(0, "127.0.0.1", () => onListening()); + await listening; + + let client; + try { + const pushed = []; + const { promise: responded, resolve: onResponse } = Promise.withResolvers(); + client = http2.connect(`http://127.0.0.1:${server.address().port}`); + client.on("error", () => {}); + client.on("stream", (pushStream, headers) => { + pushed.push(headers); + pushStream.on("error", () => {}); + }); + const req = client.request({ ":path": "/" }); + req.on("error", () => {}); + req.on("response", onResponse); + req.resume(); + req.end(); + // The parent request must complete regardless of whether the sibling push was accepted: + // nghttp2 answers a malformed PUSH_PROMISE with a stream error, not a connection error. + const headers = await responded; + expect({ caseName, status: headers[":status"] }).toEqual({ caseName, status: 200 }); + // Barrier: a PING round-trip guarantees any RST_STREAM the client wrote for the + // promised stream has already reached the server before clientFrames is inspected. + await new Promise((resolve, reject) => client.ping(err => (err ? reject(err) : resolve()))); + if (caseName === "valid") { + expect({ caseName, pushed: pushed.length }).toEqual({ caseName, pushed: 1 }); + expect(pushed[0][":path"]).toBe("/pushed"); + } else { + // No push event, and the promised stream was reset with PROTOCOL_ERROR. + const rst = clientFrames.find(f => f.type === 3 && f.streamId === 2); + expect({ caseName, pushed: pushed.length, rstCode: rst?.payload?.readUInt32BE(0) }).toEqual({ + caseName, + pushed: 0, + rstCode: http2.constants.NGHTTP2_PROTOCOL_ERROR, + }); + // The session survives: no GOAWAY carrying an error code. + const errorGoaways = clientFrames.filter(f => f.type === 7 && f.payload.readUInt32BE(4) !== 0); + expect({ caseName, errorGoaways: errorGoaways.length }).toEqual({ caseName, errorGoaways: 0 }); + } + } finally { + client?.destroy(); + server.close(); + } + } +}); + +it("allowHTTP1 fallback validates the status line like the native handle", async () => { + // The HTTP/1.1 fallback serializes `HTTP/1.1 ${statusCode} ${statusMessage}` itself, so it + // must enforce the same invariants node does on the implicit-header path (no writeHead()): + // a statusMessage carrying CR/LF throws ERR_INVALID_CHAR and an out-of-range statusCode + // throws ERR_HTTP_INVALID_STATUS_CODE instead of being written raw (response splitting). + const server = http2.createSecureServer({ ...TLS_CERT, allowHTTP1: true }); + const handlerErrors = []; + server.on("request", (req, res) => { + res.statusMessage = "OK\r\nx-injected: 1\r\n\r\nHTTP/1.1 200 OK"; + try { + res.end("split"); + } catch (err) { + handlerErrors.push(err.code); + } + res.statusCode = 100000; + try { + res.end("split"); + } catch (err) { + handlerErrors.push(err.code); + } + res.statusCode = 200; + res.statusMessage = "OK"; + res.end("clean"); + }); + const { promise: listening, resolve: onListening } = Promise.withResolvers(); + server.listen(0, "127.0.0.1", onListening); + await listening; + try { + const { promise: closed, resolve: onClose, reject: onError } = Promise.withResolvers(); + const socket = tls.connect({ + port: server.address().port, + host: "127.0.0.1", + ALPNProtocols: ["http/1.1"], + rejectUnauthorized: false, + }); + let raw = Buffer.alloc(0); + socket.on("secureConnect", () => { + socket.write("GET / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n"); + }); + socket.on("data", chunk => (raw = Buffer.concat([raw, chunk]))); + socket.on("error", onError); + socket.on("close", onClose); + await closed; + const text = raw.toString("latin1"); + expect(handlerErrors).toEqual(["ERR_INVALID_CHAR", "ERR_HTTP_INVALID_STATUS_CODE"]); + // Exactly one well-formed response, with nothing from the poisoned status line on the wire. + expect(text).toStartWith("HTTP/1.1 200 OK\r\n"); + expect(text).not.toContain("x-injected"); + expect(text.match(/HTTP\/1\.1 /g)).toHaveLength(1); + expect(text).toEndWith("clean"); + } finally { + server.close(); + } +}); + it("http2 client survives session teardown from a socket write while flushing queued DATA frames", async () => { // A flow-control-limited DATA frame sits in the native outbound queue until // the peer reopens the window. The flush that follows writes to the JS @@ -2903,6 +3403,182 @@ it("http2 client survives session teardown from a socket write while flushing qu expect(exitCode).toBe(0); }); +it("http2 client write payload cannot be transferred out from a socket write dispatch", async () => { + // The engine iterates the write's ArrayBuffer while synchronously re-entering JS (the JS + // socket's write, previous writes' callbacks): the payload is pinned for the duration, so + // JS running inside those dispatches can never free the bytes still being framed. Every + // transfer attempt from inside a write dispatch must throw and every DATA byte stay intact. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http2 = require("node:http2"); + const { Duplex } = require("node:stream"); + + function frame(type, flags, streamId, payload = Buffer.alloc(0)) { + const header = Buffer.alloc(9); + header.writeUIntBE(payload.length, 0, 3); + header[3] = type; + header[4] = flags; + header.writeUInt32BE(streamId, 5); + return Buffer.concat([header, payload]); + } + function windowUpdate(streamId, increment) { + const payload = Buffer.alloc(4); + payload.writeUInt32BE(increment, 0); + return frame(8, 0, streamId, payload); + } + // SETTINGS_INITIAL_WINDOW_SIZE = 4 MiB so the whole payload is framed in one + // writeStream call (socket writes happen inside it) instead of being queued. + const settingsPayload = Buffer.alloc(6); + settingsPayload.writeUInt16BE(0x4, 0); + settingsPayload.writeUInt32BE(4 * 1024 * 1024, 2); + + const TOTAL = 200 * 1024; + const ab = new ArrayBuffer(TOTAL); + new Uint8Array(ab).fill(0x61); + + let transferResult = "not-attempted"; + let wire = Buffer.alloc(0); + let prefaceSkipped = false; + let dataBytes = 0; + let corrupt = 0; + let sawEndStream = false; + + function consumeWire() { + if (!prefaceSkipped) { + if (wire.length < 24) return; + wire = wire.subarray(24); // client connection preface + prefaceSkipped = true; + } + while (wire.length >= 9) { + const length = wire.readUIntBE(0, 3); + if (wire.length < 9 + length) return; + const type = wire[3]; + const flags = wire[4]; + const streamId = wire.readUInt32BE(5) & 0x7fffffff; + const payload = wire.subarray(9, 9 + length); + wire = wire.subarray(9 + length); + if (type === 0x00 && streamId === 1) { + dataBytes += payload.length; + for (const byte of payload) if (byte !== 0x61) corrupt++; + if (flags & 0x01) sawEndStream = true; + } + } + } + + const socket = new Duplex({ + writableHighWaterMark: 16 * 1024 * 1024, + read() {}, + write(chunk, encoding, callback) { + wire = Buffer.concat([wire, chunk]); + if (chunk.length >= 9 && chunk[3] === 0x00) { + // A DATA-bearing socket write dispatched while the payload is still being + // framed: freeing (and overwriting) the payload here must be impossible. + try { + const moved = structuredClone(ab, { transfer: [ab] }); + new Uint8Array(moved).fill(0x42); + if (transferResult !== "threw") transferResult = "transferred"; + } catch { + transferResult = "threw"; + } + } + consumeWire(); + if (sawEndStream) { + console.log(JSON.stringify({ transferResult, dataBytes, corrupt })); + process.exit(0); + } + callback(); + }, + }); + + const client = http2.connect("http://localhost", { createConnection: () => socket }); + client.on("error", () => {}); + client.on("connect", () => { + socket.push( + Buffer.concat([frame(4, 0, 0, settingsPayload), frame(4, 1, 0), windowUpdate(0, 8 * 1024 * 1024)]), + ); + }); + client.once("remoteSettings", () => { + const req = client.request({ ":method": "POST", ":path": "/" }); + req.on("error", () => {}); + req.end(Buffer.from(ab)); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const result = JSON.parse(stdout.trim().split("\n").at(-1)); + expect(result).toEqual({ transferResult: "threw", dataBytes: 200 * 1024, corrupt: 0 }); + expect(exitCode).toBe(0); +}); + +it("http2 client survives a synchronous parser read from a closing stream's write callback", async () => { + // stream.close() concludes queued writes' callbacks synchronously while native code still + // holds the stream. A callback that synchronously feeds inbound bytes back into the parser + // must not let the deferred stream free run under that live reference (use-after-free). + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http2 = require("node:http2"); + const { Duplex } = require("node:stream"); + + function frame(type, flags, streamId, payload = Buffer.alloc(0)) { + const header = Buffer.alloc(9); + header.writeUIntBE(payload.length, 0, 3); + header[3] = type; + header[4] = flags; + header.writeUInt32BE(streamId, 5); + return Buffer.concat([header, payload]); + } + + const socket = new Duplex({ + writableHighWaterMark: 4 * 1024 * 1024, + read() {}, + write(chunk, encoding, callback) { + callback(); + }, + }); + + const client = http2.connect("http://localhost", { createConnection: () => socket }); + client.on("error", () => {}); + client.on("connect", () => { + socket.push(Buffer.concat([frame(4, 0, 0), frame(4, 1, 0)])); + }); + client.once("remoteSettings", () => { + const req = client.request({ ":method": "POST", ":path": "/" }); + req.on("error", () => {}); + // 65535 bytes fit the flow-control window; the remainder is queued with this callback. + req.write(Buffer.alloc(65535 + 32768, "a"), () => { + // Concluded synchronously by the close path below: feed inbound bytes so the parser + // re-enters read() while the closing stream is still referenced natively. + socket.push(frame(6, 0, 0, Buffer.alloc(8))); + }); + setImmediate(() => { + req.close(http2.constants.NGHTTP2_CANCEL); + setImmediate(() => { + console.log("REENTRANT_CLOSE_OK"); + process.exit(0); + }); + }); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toContain("REENTRANT_CLOSE_OK"); + expect(exitCode).toBe(0); +}); + it("http2 client keeps parsing a socket chunk whose ArrayBuffer is transferred by a frame event handler", async () => { // With a user-supplied connection (options.createConnection), the exact // Buffer handed to the socket "data" listener is fed to the native HTTP/2 diff --git a/test/js/node/test/common/index.js b/test/js/node/test/common/index.js index f1c4a8ba7688..16986ac64b3c 100644 --- a/test/js/node/test/common/index.js +++ b/test/js/node/test/common/index.js @@ -1430,6 +1430,38 @@ function installBunExposeInternalsShim() { }, }, })); + // node's internal/http: serve the very same symbols Bun's _http_outgoing + // attaches to OutgoingMessage instances, so tests poke at real state. + build.module("internal/http", () => { + const { kOutHeaders, kHighWaterMark } = require("node:_http_outgoing"); + return { + loader: "object", + exports: { kOutHeaders, kHighWaterMark }, + }; + }); + // node's internal/streams/state: getDefaultHighWaterMark is also part of + // the public node:stream API, so reuse that (same function in Bun). + build.module("internal/streams/state", () => ({ + loader: "object", + exports: { getDefaultHighWaterMark: require("node:stream").getDefaultHighWaterMark }, + })); + // node's internal/options: map the few CLI options vendored http tests ask + // about onto the equivalent runtime values. Unknown options return undefined. + build.module("internal/options", () => ({ + loader: "object", + exports: { + getOptionValue(name) { + switch (name) { + case "--max-http-header-size": + return require("node:http").maxHeaderSize; + case "--insecure-http-parser": + return false; + default: + return undefined; + } + }, + }, + })); }, }); } diff --git a/test/js/node/test/sequential/test-http2-ping-flood.js b/test/js/node/test/sequential/test-http2-ping-flood.js new file mode 100644 index 000000000000..0f324c30d06c --- /dev/null +++ b/test/js/node/test/sequential/test-http2-ping-flood.js @@ -0,0 +1,60 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const http2 = require('http2'); +const net = require('net'); + +const http2util = require('../common/http2'); + +// Test that ping flooding causes the session to be torn down + +const kSettings = new http2util.SettingsFrame(); +const kPing = new http2util.PingFrame(); + +const server = http2.createServer(); + +let interval; + +server.on('stream', common.mustNotCall()); +server.on('session', common.mustCall((session) => { + session.on('error', common.mustCallAtLeast((e) => { + assert.strictEqual(e.code, 'ERR_HTTP2_ERROR'); + assert(e.message.includes('Flooding was detected')); + clearInterval(interval); + }, 0)); + session.on('close', common.mustCall(() => { + server.close(); + })); +})); + +server.listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + + // nghttp2 uses a limit of 10000 items in it's outbound queue. + // If this number is exceeded, a flooding error is raised. + // TODO(jasnell): Unfortunately, this test is inherently flaky because + // it is entirely dependent on how quickly the server is able to handle + // the inbound frames and whether those just happen to overflow nghttp2's + // outbound queue. The threshold at which the flood error occurs can vary + // from one system to another, and from one test run to another. + client.on('connect', common.mustCall(() => { + client.write(http2util.kClientMagic, () => { + client.write(kSettings.data, () => { + interval = setInterval(() => { + for (let n = 0; n < 10000; n++) + client.write(kPing.data); + }, 1); + }); + }); + })); + + // An error event may or may not be emitted, depending on operating system + // and timing. We do not really care if one is emitted here or not, as the + // error on the server side is what we are testing for. Do not make this + // a common.mustCall() and there's no need to check the error details. + client.on('error', () => {}); +})); diff --git a/test/js/node/tls/node-tls-cert.test.ts b/test/js/node/tls/node-tls-cert.test.ts index 7717ae5949ad..86b82ef1a66d 100644 --- a/test/js/node/tls/node-tls-cert.test.ts +++ b/test/js/node/tls/node-tls-cert.test.ts @@ -550,6 +550,171 @@ it("tls.connect should not accept untrusted certificates", async () => { } }); +it("tls.connect with rejectUnauthorized: null still rejects untrusted certificates", async () => { + // Node normalizes rejectUnauthorized with `!== false`: only an explicit `false` disables + // verification. `null` (a common config-file "use the default") must keep it enforced. + const { promise, resolve, reject } = Promise.withResolvers(); + let server: Server | null = null; + let socket: TLSSocket | null = null; + + try { + server = tls + .createServer({ + key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), + cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), + passphrase: "123123123", + }) + .on("error", reject) + .listen(0, () => { + const address = server?.address() as AddressInfo; + socket = tls + .connect({ port: address.port, rejectUnauthorized: null as unknown as boolean }, () => { + reject(new Error("secureConnect must not fire when verification failed")); + }) + .on("error", resolve); + }); + + const err = await promise; + expect(err.code).toBe("UNABLE_TO_VERIFY_LEAF_SIGNATURE"); + } finally { + //@ts-ignore + socket?.end(); + server?.close(); + } +}); + +it("tls.connect with rejectUnauthorized: 0 keeps verification on like node", async () => { + // Node's normalization is `!== false`, so falsy non-`false` values (0, "") keep + // verification enforced; they must not blow up in the native strict-boolean conversion. + const { promise, resolve, reject } = Promise.withResolvers(); + let server: Server | null = null; + let socket: TLSSocket | null = null; + + try { + server = tls + .createServer({ + key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), + cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), + passphrase: "123123123", + }) + .on("error", reject) + .listen(0, () => { + const address = server?.address() as AddressInfo; + socket = tls + .connect({ port: address.port, rejectUnauthorized: 0 as unknown as boolean }, () => { + reject(new Error("secureConnect must not fire when verification failed")); + }) + .on("error", resolve); + }); + + const err = await promise; + expect(err.code).toBe("UNABLE_TO_VERIFY_LEAF_SIGNATURE"); + } finally { + //@ts-ignore + socket?.end(); + server?.close(); + } +}); + +it("tls.createServer with rejectUnauthorized: 0 still rejects a client with an untrusted certificate", async () => { + // The server-side gate runs in JS after the handshake (the native verify callback always + // defers to JS), so the raw option value must be normalized like Node (`!== false`): a + // falsy non-`false` value must not admit a client whose certificate failed verification. + let server: Server | null = null; + let socket: TLSSocket | null = null; + const secureConnections: string[] = []; + const clientErrors = Promise.withResolvers(); + const clientClosed = Promise.withResolvers(); + const serverTLS = { + key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), + cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), + passphrase: "123123123", + }; + + try { + server = tls + .createServer( + { + ...serverTLS, + requestCert: true, + rejectUnauthorized: 0 as unknown as boolean, + }, + s => { + secureConnections.push(s.authorizationError as string); + s.write("admitted"); + }, + ) + .on("tlsClientError", clientErrors.resolve) + .listen(0, () => { + const address = server?.address() as AddressInfo; + // The client presents a certificate the server cannot verify (self-signed, no CA). + socket = tls.connect({ port: address.port, rejectUnauthorized: false, ...serverTLS }); + let received = ""; + socket.on("data", chunk => (received += chunk)); + socket.on("error", () => {}); + socket.on("close", () => clientClosed.resolve(received)); + }); + + const [, received] = await Promise.all([clientErrors.promise, clientClosed.promise]); + // The unverified client is rejected: no 'secureConnection', no bytes served. + expect({ secureConnections, received }).toEqual({ secureConnections: [], received: "" }); + } finally { + //@ts-ignore + socket?.end(); + server?.close(); + } +}); + +it("tls.createServer with rejectUnauthorized: null still rejects unauthorized clients", async () => { + // Server side of the same normalization: with requestCert, a client that fails + // verification must be destroyed unless rejectUnauthorized is explicitly `false` + // (Node's `!== false`), so `null` must never silently admit an unverified peer. + let server: Server | null = null; + let socket: TLSSocket | null = null; + const secureConnections: string[] = []; + const clientErrors = Promise.withResolvers(); + const clientClosed = Promise.withResolvers(); + + try { + server = tls + .createServer( + { + key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), + cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), + passphrase: "123123123", + requestCert: true, + rejectUnauthorized: null as unknown as boolean, + }, + s => { + secureConnections.push(s.authorizationError as string); + s.write("admitted"); + }, + ) + .on("tlsClientError", clientErrors.resolve) + .listen(0, () => { + const address = server?.address() as AddressInfo; + // The client presents no certificate, so the server's verification fails. + socket = tls.connect({ port: address.port, rejectUnauthorized: false }); + let received = ""; + socket.on("data", chunk => (received += chunk)); + socket.on("error", () => {}); + socket.on("close", () => clientClosed.resolve(received)); + }); + + const [err, received] = await Promise.all([clientErrors.promise, clientClosed.promise]); + // The unauthorized client is rejected: no 'secureConnection', no bytes served. + expect({ secureConnections, received, code: (err as NodeJS.ErrnoException).code }).toEqual({ + secureConnections: [], + received: "", + code: "ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE", + }); + } finally { + //@ts-ignore + socket?.end(); + server?.close(); + } +}); + async function createTLSServer(options: tls.TlsOptions) { const server = await new Promise((resolve, reject) => { const server = tls