diff --git a/src/http/h2_client/dispatch.rs b/src/http/h2_client/dispatch.rs index 40ec9a6f32c9..c46c4bce3ea6 100644 --- a/src/http/h2_client/dispatch.rs +++ b/src/http/h2_client/dispatch.rs @@ -741,31 +741,9 @@ fn strip_padding(payload: &[u8]) -> Option<&[u8]> { /// hop-by-hop fields. Names from lshpack are already lowercase for table /// hits but a literal can carry anything. pub(crate) fn is_malformed_response_field(name: &[u8]) -> bool { - if name.is_empty() { + if name.is_empty() || !name.iter().all(|&c| wire::is_lower_tchar(c)) { return true; } - for &c in name { - match c { - b'a'..=b'z' - | b'0'..=b'9' - | b'!' - | b'#' - | b'$' - | b'%' - | b'&' - | b'\'' - | b'*' - | b'+' - | b'-' - | b'.' - | b'^' - | b'_' - | b'`' - | b'|' - | b'~' => {} - _ => return true, - } - } matches!( name, b"connection" @@ -777,13 +755,7 @@ pub(crate) 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(crate) fn is_malformed_response_value(value: &[u8]) -> bool { - bun_core::strings::contains_any(value, b"\0\r\n") -} +pub(crate) use wire::is_malformed_field_value as is_malformed_response_value; pub(crate) fn error_code_for(err: crate::Error) -> wire::ErrorCode { match err { diff --git a/src/http/lib.rs b/src/http/lib.rs index 57bfb2fccb51..4f1f99e5ba23 100644 --- a/src/http/lib.rs +++ b/src/http/lib.rs @@ -4792,6 +4792,48 @@ impl<'a> HTTPClient<'a> { } } + /// Shared tail of the `Location`-header arms in + /// `handle_response_metadata`: parse the rebuilt absolute href, compare + /// origins against the current URL, then swap the href into + /// `self.redirect`. Returns whether the redirect target is same-origin. + fn apply_redirect_url(&mut self, new_href: Vec) -> crate::Result { + let new_url = URL::parse(&new_href); + if !new_url.has_http_like_protocol() { + return Err(crate::Error::UnsupportedRedirectProtocol); + } + // SAFETY: self-borrow — `new_href` is moved into `self.redirect` + // below, which lives as long as `self` (≥ `'a`). + let new_url: URL<'a> = unsafe { new_url.erase_lifetime() }; + let is_same_origin = strings::eql_case_insensitive_ascii( + strings::without_trailing_slash(new_url.origin), + strings::without_trailing_slash(self.url.origin), + true, + ); + self.url = new_url; + // connected_url still borrows from the previous hop's buffer until + // doRedirect releases the socket, so park it in prev_redirect for + // doRedirect to free instead of leaking it. + debug_assert!(self.prev_redirect.is_empty()); + self.prev_redirect = core::mem::replace(&mut self.redirect, new_href); + Ok(is_same_origin) + } + + /// Normalize a fully-rebuilt redirect URL through the WHATWG parser and + /// apply it via [`Self::apply_redirect_url`]. + fn normalize_and_apply_redirect_url( + &mut self, + mut string_builder: StringBuilder, + ) -> crate::Result { + debug_assert!(string_builder.cap == string_builder.len); + let input = BunString::borrow_utf8(string_builder.allocated_slice()); + let normalized_url = OwnedString::new(bun_url::href_from_string(&input)); + if normalized_url.tag() == BunStringTag::Dead { + // URL__getHref failed, dont pass dead tagged string to toOwnedSlice. + return Err(crate::Error::RedirectURLInvalid); + } + self.apply_redirect_url(normalized_url.to_owned_slice()) + } + pub(crate) fn handle_response_metadata( &mut self, response: &mut picohttp::Response, @@ -5073,32 +5115,7 @@ impl<'a> HTTPClient<'a> { let _ = string_builder.append(location); - debug_assert!(string_builder.cap == string_builder.len); - - let input = BunString::borrow_utf8(string_builder.allocated_slice()); - let normalized_url = OwnedString::new(bun_url::href_from_string(&input)); - if normalized_url.tag() == BunStringTag::Dead { - // URL__getHref failed, dont pass dead tagged string to toOwnedSlice. - return Err(crate::Error::RedirectURLInvalid); - } - let normalized_url_str = normalized_url.to_owned_slice(); - - // SAFETY: self-borrow — `normalized_url_str` is moved into - // `self.redirect` below, which lives as long as `self` (≥ `'a`). - let new_url: URL<'a> = - unsafe { URL::parse(&normalized_url_str).erase_lifetime() }; - is_same_origin = strings::eql_case_insensitive_ascii( - strings::without_trailing_slash(new_url.origin), - strings::without_trailing_slash(self.url.origin), - true, - ); - self.url = new_url; - // connected_url still borrows from the previous hop's buffer - // until doRedirect releases the socket, so park it in - // prev_redirect for doRedirect to free instead of leaking it. - debug_assert!(self.prev_redirect.is_empty()); - self.prev_redirect = - core::mem::replace(&mut self.redirect, normalized_url_str); + is_same_origin = self.normalize_and_apply_redirect_url(string_builder)?; } else if location.starts_with(b"//") { let mut string_builder = StringBuilder::default(); @@ -5129,32 +5146,9 @@ impl<'a> HTTPClient<'a> { let _ = string_builder.append(location); - debug_assert!(string_builder.cap == string_builder.len); - - let input = BunString::borrow_utf8(string_builder.allocated_slice()); - let normalized_url = OwnedString::new(bun_url::href_from_string(&input)); - if normalized_url.tag() == BunStringTag::Dead { - return Err(crate::Error::RedirectURLInvalid); - } - let normalized_url_str = normalized_url.to_owned_slice(); - - // SAFETY: self-borrow — `normalized_url_str` is moved into - // `self.redirect` below, which lives as long as `self` (≥ `'a`). - let new_url: URL<'a> = - unsafe { URL::parse(&normalized_url_str).erase_lifetime() }; - is_same_origin = strings::eql_case_insensitive_ascii( - strings::without_trailing_slash(new_url.origin), - strings::without_trailing_slash(self.url.origin), - true, - ); - self.url = new_url; - debug_assert!(self.prev_redirect.is_empty()); - self.prev_redirect = - core::mem::replace(&mut self.redirect, normalized_url_str); + is_same_origin = self.normalize_and_apply_redirect_url(string_builder)?; } else { - let original_url = self.url.clone(); - - let base = BunString::borrow_utf8(original_url.href); + let base = BunString::borrow_utf8(self.url.href); let rel = BunString::borrow_utf8(location); let new_url_ = OwnedString::new(bun_url::join(&base, &rel)); @@ -5162,21 +5156,7 @@ impl<'a> HTTPClient<'a> { return Err(crate::Error::InvalidRedirectURL); } - let new_url = new_url_.to_owned_slice(); - let parsed_url = URL::parse(&new_url); - if !parsed_url.has_http_like_protocol() { - return Err(crate::Error::UnsupportedRedirectProtocol); - } - // SAFETY: self-borrow — `new_url` is moved into `self.redirect` - // below, which lives as long as `self` (≥ `'a`). - self.url = unsafe { parsed_url.erase_lifetime() }; - is_same_origin = strings::eql_case_insensitive_ascii( - strings::without_trailing_slash(self.url.origin), - strings::without_trailing_slash(original_url.origin), - true, - ); - debug_assert!(self.prev_redirect.is_empty()); - self.prev_redirect = core::mem::replace(&mut self.redirect, new_url); + is_same_origin = self.apply_redirect_url(new_url_.to_owned_slice())?; } } diff --git a/src/http_types/h2.rs b/src/http_types/h2.rs index 141e596b0831..eeef7c70575f 100644 --- a/src/http_types/h2.rs +++ b/src/http_types/h2.rs @@ -217,3 +217,104 @@ impl SettingsPayloadUnit { } } } + +// ─── field validation (RFC 9113 §8.2.1) ───────── + +/// RFC 9110 §5.6.2 `tchar`, restricted to lowercase: RFC 9113 §8.2.1 requires +/// HTTP/2 field names to be lowercase, so uppercase tchars are rejected (or +/// normalized) by callers rather than accepted here. +#[inline] +pub const fn is_lower_tchar(c: u8) -> bool { + matches!( + c, + b'a'..=b'z' + | b'0'..=b'9' + | b'!' + | b'#' + | b'$' + | b'%' + | b'&' + | b'\'' + | b'*' + | b'+' + | b'-' + | b'.' + | b'^' + | b'_' + | b'`' + | b'|' + | b'~' + ) +} + +/// 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. +#[inline] +pub fn is_malformed_field_value(value: &[u8]) -> bool { + bun_core::strings::contains_any(value, b"\0\r\n") +} + +#[cfg(test)] +mod tests { + use super::is_lower_tchar; + + /// Exhaustive parity check against the RFC 9110 §5.6.2 `tchar` grammar + /// with the uppercase range removed. + #[test] + fn lower_tchar_matches_grammar() { + // RFC 9110 defines tchar as any VCHAR except the delimiters below; + // deriving the oracle that way keeps it independent of the literal + // table in `is_lower_tchar`. + for c in 0..=u8::MAX { + let is_delimiter = matches!( + c, + b'"' | b'(' + | b')' + | b',' + | b'/' + | b':' + | b';' + | b'<' + | b'=' + | b'>' + | b'?' + | b'@' + | b'[' + | b'\\' + | b']' + | b'{' + | b'}' + ); + let expected = c.is_ascii_graphic() && !is_delimiter && !c.is_ascii_uppercase(); + assert_eq!(is_lower_tchar(c), expected, "byte {c:#04x}"); + } + } + + /// `contains_any` is backed by the highway objects, which a native + /// `cargo test` of this leaf crate does not link; this crate's unit tests + /// run in CI under Miri, where highway takes its scalar paths. + #[cfg(miri)] + #[test] + fn field_value_rejects_exactly_nul_cr_lf() { + use super::is_malformed_field_value; + + assert!(!is_malformed_field_value(b"")); + assert!(!is_malformed_field_value(b"text/plain; charset=utf-8")); + // Tabs, spaces, and obs-text are legal in values. + assert!(!is_malformed_field_value(b"\ta b\xff")); + for bad in [b'\0', b'\r', b'\n'] { + assert!(is_malformed_field_value(&[bad]), "lone {bad:#04x}"); + assert!(is_malformed_field_value(&[bad, b'x']), "leading {bad:#04x}"); + assert!( + is_malformed_field_value(&[b'x', bad]), + "trailing {bad:#04x}" + ); + } + let mut long = vec![b'a'; 100]; + assert!(!is_malformed_field_value(&long)); + long[99] = b'\n'; + assert!(is_malformed_field_value(&long)); + } +} diff --git a/src/js/internal/http.ts b/src/js/internal/http.ts index 19c94ee3fe6c..bf17608b70eb 100644 --- a/src/js/internal/http.ts +++ b/src/js/internal/http.ts @@ -550,6 +550,88 @@ function filterEnvForProxies(env) { }; } +// Stub members shared by `FakeSocket` (internal/http/FakeSocket.ts) and +// `NodeHTTPServerSocket` (node/_http_server.ts). They are copied onto each +// class's prototype (instead of using a base class) so the prototype chain +// stays `Socket.prototype -> Duplex.prototype`, matching `net.Socket`. +const { constructor: _socketStubConstructor, ...socketStubDescriptors } = Object.getOwnPropertyDescriptors( + class { + declare connecting: boolean; + declare readable: boolean; + declare writable: boolean; + declare writableLength: number; + declare address: () => any; + + connect(_port, _host, _connectListener) { + return this; + } + + get bufferSize() { + return this.writableLength; + } + + get pending() { + return this.connecting; + } + + get readyState() { + if (this.connecting) return "opening"; + if (this.readable) { + return this.writable ? "open" : "readOnly"; + } else { + return this.writable ? "writeOnly" : "closed"; + } + } + + ref() { + return this; + } + + get remoteAddress() { + return this.address()?.address; + } + + set remoteAddress(val) { + // initialize the object so that other properties wouldn't be lost + this.address().address = val; + } + + get remotePort() { + return this.address()?.port; + } + + set remotePort(val) { + // initialize the object so that other properties wouldn't be lost + this.address().port = val; + } + + get remoteFamily() { + return this.address()?.family; + } + + set remoteFamily(val) { + // initialize the object so that other properties wouldn't be lost + this.address().family = val; + } + + resetAndDestroy() {} + + setKeepAlive(_enable = false, _initialDelay = 0) {} + + setNoDelay(_noDelay = true) { + return this; + } + + unref() { + return this; + } + }.prototype, +); + +function installSocketStubs(SocketClass: { prototype: object }) { + Object.defineProperties(SocketClass.prototype, socketStubDescriptors); +} + export { METHODS, STATUS_CODES, @@ -570,6 +652,7 @@ export { hasServerResponseFinished, headerStateSymbol, headersSymbol, + installSocketStubs, isTlsSymbol, kAbortController, kAgent, diff --git a/src/js/internal/http/FakeSocket.ts b/src/js/internal/http/FakeSocket.ts index 3b7c9a08b767..1b7864f1468e 100644 --- a/src/js/internal/http/FakeSocket.ts +++ b/src/js/internal/http/FakeSocket.ts @@ -1,4 +1,4 @@ -const { kInternalSocketData, serverSymbol } = require("internal/http"); +const { kInternalSocketData, serverSymbol, installSocketStubs } = require("internal/http"); const { kAutoDestroyed } = require("internal/shared"); const { Duplex } = require("internal/stream"); @@ -24,13 +24,6 @@ var FakeSocket = class Socket extends Duplex { (internalData = this[kInternalSocketData])?.[0]?.[serverSymbol]?.requestIP(internalData[2]) ?? {}); } - get bufferSize() { - return this.writableLength; - } - - connect(_port, _host, _connectListener) { - return this; - } _onTimeout = function () { this.emit("timeout"); }; @@ -55,60 +48,8 @@ var FakeSocket = class Socket extends Duplex { return 80; } - get pending() { - return this.connecting; - } - _read(_size) {} - get readyState() { - if (this.connecting) return "opening"; - if (this.readable) { - return this.writable ? "open" : "readOnly"; - } else { - return this.writable ? "writeOnly" : "closed"; - } - } - - ref() { - return this; - } - - get remoteAddress() { - return this.address()?.address; - } - - set remoteAddress(val) { - // initialize the object so that other properties wouldn't be lost - this.address().address = val; - } - - get remotePort() { - return this.address()?.port; - } - - set remotePort(val) { - // initialize the object so that other properties wouldn't be lost - this.address().port = val; - } - - get remoteFamily() { - return this.address()?.family; - } - - set remoteFamily(val) { - // initialize the object so that other properties wouldn't be lost - this.address().family = val; - } - - resetAndDestroy() {} - - setKeepAlive(_enable = false, _initialDelay = 0) {} - - setNoDelay(_noDelay = true) { - return this; - } - setTimeout(timeout, callback) { const socketData = this[kInternalSocketData]; if (!socketData) return; // sometimes 'this' is Socket not FakeSocket @@ -118,10 +59,6 @@ var FakeSocket = class Socket extends Duplex { return this; } - unref() { - return this; - } - _write(_chunk, _encoding, _callback) {} destroy() { @@ -130,6 +67,7 @@ var FakeSocket = class Socket extends Duplex { } }; +installSocketStubs(FakeSocket); Object.defineProperty(FakeSocket, "name", { value: "Socket" }); export default { diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 3d32533df07a..6d2d8b92455f 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -54,6 +54,7 @@ const { setServerCustomOptions, setServerAppFlags, getMaxHTTPHeaderSize, + installSocketStubs, fakeSocketSymbol, noBodySymbol, kOutHeaders, @@ -1732,14 +1733,6 @@ const NodeHTTPServerSocket = class Socket extends NetSocket { return this[kHandle]?.remoteAddress || null; } - get bufferSize() { - return this.writableLength; - } - - connect(_port, _host, _connectListener) { - return this; - } - _destroy(err, callback) { const handle = this[kHandle]; if (!handle) { @@ -1782,10 +1775,6 @@ const NodeHTTPServerSocket = class Socket extends NetSocket { return this[kHandle]?.localAddress?.port; } - get pending() { - return this.connecting; - } - #resumeSocket() { const handle = this[kHandle]; const response = handle?.response; @@ -1828,15 +1817,6 @@ const NodeHTTPServerSocket = class Socket extends NetSocket { this.#resumeSocket(); } - get readyState() { - if (this.connecting) return "opening"; - if (this.readable) { - return this.writable ? "open" : "readOnly"; - } else { - return this.writable ? "writeOnly" : "closed"; - } - } - // SNI hostname the client sent in its ClientHello, or false when the TLS // client sent none (matches Node's server-side TLSSocket.servername). get servername() { @@ -1860,45 +1840,6 @@ const NodeHTTPServerSocket = class Socket extends NetSocket { return this[kHandle]?.authorizationError ?? null; } - ref() { - return this; - } - - get remoteAddress() { - return this.address()?.address; - } - - set remoteAddress(val) { - // initialize the object so that other properties wouldn't be lost - this.address().address = val; - } - - get remotePort() { - return this.address()?.port; - } - - set remotePort(val) { - // initialize the object so that other properties wouldn't be lost - this.address().port = val; - } - - get remoteFamily() { - return this.address()?.family; - } - - set remoteFamily(val) { - // initialize the object so that other properties wouldn't be lost - this.address().family = val; - } - - resetAndDestroy() {} - - setKeepAlive(_enable = false, _initialDelay = 0) {} - - setNoDelay(_noDelay = true) { - return this; - } - // Like Node.js's net.Socket#setTimeout (setStreamTimeout): an unref'd // inactivity timer that emits 'timeout' on this socket. server.setTimeout, // server.keepAliveTimeout, req.setTimeout and res.setTimeout all funnel here. @@ -1945,10 +1886,6 @@ const NodeHTTPServerSocket = class Socket extends NetSocket { throw err; } - unref() { - return this; - } - _write(_chunk, _encoding, _callback) { const handle = this[kHandle]; let err; @@ -2095,6 +2032,7 @@ function _writeHead(statusCode, reason, obj, response) { } } +installSocketStubs(NodeHTTPServerSocket); Object.defineProperty(NodeHTTPServerSocket, "name", { value: "Socket" }); function ServerResponse(req, options): void { diff --git a/src/runtime/api/bun/h2_frame_parser.rs b/src/runtime/api/bun/h2_frame_parser.rs index 332ac4dc9b42..d6db6f14d96f 100644 --- a/src/runtime/api/bun/h2_frame_parser.rs +++ b/src/runtime/api/bun/h2_frame_parser.rs @@ -23,8 +23,8 @@ use bstr::BStr; use bun_collections::{ByteVecExt, HashMap as BunHashMap, HiveArrayFallback, VecExt}; use bun_core::MutableString; use bun_core::String as BunString; -use bun_core::strings; use bun_http::lshpack; +use bun_http_types::h2::is_lower_tchar; use bun_jsc::AbortSignal; use bun_jsc::ErrorCode as JscErrorCode; use bun_jsc::StringJsc as _; @@ -652,9 +652,11 @@ fn is_valid_request_pseudo_header(name: &[u8]) -> bool { REQUEST_PSEUDO_HEADERS.contains(name) } +pub(crate) use bun_http_types::h2::is_malformed_field_value; + #[inline] fn is_valid_header_value(value: &[u8]) -> bool { - !strings::contains_any(value, b"\0\n\r") + !is_malformed_field_value(value) } #[inline] @@ -664,34 +666,7 @@ pub(crate) fn is_malformed_field_name(name: &[u8]) -> bool { Some((b':', rest)) => rest, Some(_) => name, }; - rest.is_empty() - || !rest.iter().all(|&c| { - matches!( - c, - b'a'..=b'z' - | b'0'..=b'9' - | b'!' - | b'#' - | b'$' - | b'%' - | b'&' - | b'\'' - | b'*' - | b'+' - | b'-' - | b'.' - | b'^' - | b'_' - | b'`' - | b'|' - | b'~' - ) - }) -} - -#[inline] -pub(crate) fn is_malformed_field_value(value: &[u8]) -> bool { - strings::contains_any(value, b"\0\r\n") + rest.is_empty() || !rest.iter().all(|&c| is_lower_tchar(c)) } const SINGLE_VALUE_HEADERS_LEN: usize = 40; @@ -7123,29 +7098,44 @@ impl H2FrameParser { Ok(JSValue::UNDEFINED) } - #[bun_jsc::host_fn(method)] - pub(crate) fn get_end_after_headers( - this: &Self, + /// Shared prologue for host fns that take a stream id argument: validates + /// the JS value, optionally rejects id 0 / ids above `MAX_STREAM_ID`, and + /// resolves the live `Stream` pointer in `self.streams`. `not_number_msg` + /// preserves each call site's user-visible error for a non-number argument. + #[inline] + fn stream_from_js_arg( + &self, global_object: &JSGlobalObject, - callframe: &CallFrame, - ) -> JsResult { - let [stream_arg] = callframe.arguments_as_array::<1>(); - if callframe.arguments_count() < 1 { - return Err(global_object.throw(format_args!("Expected stream argument"))); - } - + stream_arg: JSValue, + not_number_msg: &str, + ) -> JsResult<*mut Stream> { if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Invalid stream id"))); + return Err(global_object.throw(format_args!("{not_number_msg}"))); } let stream_id = stream_arg.to_u32(); - if stream_id == 0 { + if (CHECK_ZERO && stream_id == 0) || (CHECK_MAX && stream_id > MAX_STREAM_ID) { return Err(global_object.throw(format_args!("Invalid stream id"))); } - let Some(stream) = this.streams.get().get(&stream_id).copied() else { + let Some(stream) = self.streams.get().get(&stream_id).copied() else { return Err(global_object.throw(format_args!("Invalid stream id"))); }; + Ok(stream) + } + + #[bun_jsc::host_fn(method)] + pub(crate) fn get_end_after_headers( + this: &Self, + global_object: &JSGlobalObject, + callframe: &CallFrame, + ) -> JsResult { + let [stream_arg] = callframe.arguments_as_array::<1>(); + if callframe.arguments_count() < 1 { + return Err(global_object.throw(format_args!("Expected stream argument"))); + } + let stream = + this.stream_from_js_arg::(global_object, stream_arg, "Invalid stream id")?; // SAFETY: stream is *mut Stream from self.streams; valid while the map entry exists Ok(JSValue::from(unsafe { (*stream).end_after_headers })) @@ -7161,19 +7151,8 @@ impl H2FrameParser { if callframe.arguments_count() < 1 { return Err(global_object.throw(format_args!("Expected stream argument"))); } - - if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let stream_id = stream_arg.to_u32(); - if stream_id == 0 { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let Some(stream) = this.streams.get().get(&stream_id).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; + let stream = + this.stream_from_js_arg::(global_object, stream_arg, "Invalid stream id")?; // SAFETY: stream is a *mut Stream from self.streams (heap::alloc); valid while the map entry exists let stream = unsafe { &*stream }; @@ -7196,19 +7175,8 @@ impl H2FrameParser { if callframe.arguments_count() < 1 { return Err(global_object.throw(format_args!("Expected stream argument"))); } - - if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let stream_id = stream_arg.to_u32(); - if stream_id == 0 { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let Some(stream) = this.streams.get().get(&stream_id).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; + let stream = + this.stream_from_js_arg::(global_object, stream_arg, "Invalid stream id")?; // SAFETY: stream is a *mut Stream from self.streams (heap::alloc); valid while the map entry exists let stream = unsafe { &mut *stream }; let state = JSValue::create_empty_object(global_object, 6); @@ -7259,18 +7227,8 @@ impl H2FrameParser { return Err(global_object.throw(format_args!("Expected stream and options arguments"))); } - if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let stream_id = stream_arg.to_u32(); - if stream_id == 0 { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let Some(stream_ptr) = this.streams.get().get(&stream_id).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; + let stream_ptr = + this.stream_from_js_arg::(global_object, stream_arg, "Invalid stream id")?; // The `options` getters below can run user JS while `stream` is borrowed. let mut stream = this.enter_stream_dispatch(stream_ptr); @@ -7726,18 +7684,11 @@ impl H2FrameParser { ))); } - if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Expected stream to be a number"))); - } - - let stream_id = stream_arg.to_u32(); - if stream_id == 0 || stream_id > MAX_STREAM_ID { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let Some(stream) = this.streams.get().get(&stream_id).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; + let stream = this.stream_from_js_arg::( + global_object, + stream_arg, + "Expected stream to be a number", + )?; // SAFETY: stream is a *mut Stream from self.streams (heap::alloc); valid while the map entry exists let stream = unsafe { &mut *stream }; @@ -7827,23 +7778,7 @@ impl H2FrameParser { any = true; continue 'begin; } - b'a'..=b'z' - | b'0'..=b'9' - | b'!' - | b'#' - | b'$' - | b'%' - | b'&' - | b'\'' - | b'*' - | b'+' - | b'-' - | b'.' - | b'^' - | b'_' - | b'`' - | b'|' - | b'~' => {} + c if is_lower_tchar(c) => {} b':' => { // only allow pseudoheaders at the beginning if i != 0 || any { @@ -7893,18 +7828,11 @@ impl H2FrameParser { ))); } - if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Expected stream to be a number"))); - } - - let stream_id = stream_arg.to_u32(); - if stream_id == 0 || stream_id > MAX_STREAM_ID { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } - - let Some(stream_ptr) = this.streams.get().get(&stream_id).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; + let stream_ptr = this.stream_from_js_arg::( + global_object, + stream_arg, + "Expected stream to be a number", + )?; // The header/sensitive-object getters and value coercions below can run user JS // while `stream` is borrowed. let mut stream = this.enter_stream_dispatch(stream_ptr); @@ -7985,7 +7913,7 @@ impl H2FrameParser { value: &[u8], never_index: bool| -> JsResult> { - if !is_valid_header_value(value) { + if is_malformed_field_value(value) { let exception = global_object.to_type_error( bun_jsc::ErrorCode::HTTP2_INVALID_HEADER_VALUE, format_args!("Invalid value for header \"{}\"", BStr::new(validated_name)), @@ -8223,19 +8151,14 @@ impl H2FrameParser { defer_callback_arg, ] = args.ptr; - if !stream_arg.is_number() { - return Err(global_object.throw(format_args!("Expected stream to be a number"))); - } - - let stream_id = stream_arg.to_u32(); - if stream_id == 0 || stream_id > MAX_STREAM_ID { - return Err(global_object.throw(format_args!("Invalid stream id"))); - } + let stream_ptr = this.stream_from_js_arg::( + global_object, + stream_arg, + "Expected stream to be a number", + )?; + // ToBoolean is side-effect free, so reading `close` after the stream + // lookup is observably identical to the previous ordering. let close = close_arg.to_boolean(); - - let Some(stream_ptr) = this.streams.get().get(&stream_id).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; // Coercing `data_arg` (a String subclass's toString) can run user JS while `stream` // is borrowed. let mut stream = this.enter_stream_dispatch(stream_ptr); @@ -8643,13 +8566,11 @@ impl H2FrameParser { return Err(global_object.throw(format_args!("Expected stream_id argument"))); } - if !stream_id_arg.is_number() { - return Err(global_object.throw(format_args!("Expected stream_id to be a number"))); - } - - let Some(stream) = this.streams.get().get(&stream_id_arg.to_u32()).copied() else { - return Err(global_object.throw(format_args!("Invalid stream id"))); - }; + let stream = this.stream_from_js_arg::( + global_object, + stream_id_arg, + "Expected stream_id to be a number", + )?; // SAFETY: stream is *mut Stream from self.streams; valid while the map entry exists Ok(unsafe { (*stream).js_context.get() }.unwrap_or(JSValue::UNDEFINED)) @@ -9071,43 +8992,10 @@ impl H2FrameParser { return Err(global_object.throw_value(exception)); } - if js_value.js_type().is_array() { - bun_output::scoped_log!(H2FrameParser, "array header {}", BStr::new(name)); - let mut value_iter = js_value.array_iterator(global_object)?; - - if let Some(idx) = this.single_value_index_checked(validated_name) { - if value_iter.len > 1 || single_value_headers[idx] { - if !global_object.has_exception() { - let exception = global_object.to_type_error( - bun_jsc::ErrorCode::HTTP2_HEADER_SINGLE_VALUE, - format_args!( - "Header field \"{}\" must only have a single value", - BStr::new(validated_name) - ), - ); - return Err(global_object.throw_value(exception)); - } - return Ok(JSValue::ZERO); - } - single_value_headers[idx] = true; - } - - while let Some(item) = value_iter.next()? { - if item.is_empty_or_undefined_or_null() { - if !global_object.has_exception() { - return Err(global_object - .err( - JscErrorCode::HTTP2_INVALID_HEADER_VALUE, - format_args!( - "Invalid value for header \"{}\"", - BStr::new(validated_name) - ), - ) - .throw()); - } - return Ok(JSValue::ZERO); - } - + // closure shared by the array and single-value arms; `encode_err_return` + // preserves each arm's return value on a compression error + let mut encode_value = + |item: JSValue, encode_err_return: JSValue| -> JsResult> { let value_str = item.to_js_string(global_object)?; let never_index = if Self::is_index_like_name(validated_name) { @@ -9121,7 +9009,7 @@ impl H2FrameParser { let value_slice = value_str.to_slice(global_object); let value = value_slice.slice(); - if !is_valid_header_value(value) { + if is_malformed_field_value(value) { return Err(global_object .err( JscErrorCode::HTTP2_INVALID_HEADER_VALUE, @@ -9150,7 +9038,7 @@ impl H2FrameParser { .throw(format_args!("Failed to allocate header buffer"))); } let Some(stream) = this.handle_received_stream_id(stream_id) else { - return Ok(JSValue::js_number(-1.0)); + return Ok(Some(JSValue::js_number(-1.0))); }; // SAFETY: stream is a *mut Stream from self.streams (heap::alloc); valid while the map entry exists let stream = unsafe { &mut *stream }; @@ -9160,7 +9048,50 @@ impl H2FrameParser { stream.set_context(stream_ctx_arg, global_object); } this.schedule_header_compression_session_error(); - return Ok(JSValue::UNDEFINED); + return Ok(Some(encode_err_return)); + } + Ok(None) + }; + + if js_value.js_type().is_array() { + bun_output::scoped_log!(H2FrameParser, "array header {}", BStr::new(name)); + let mut value_iter = js_value.array_iterator(global_object)?; + + if let Some(idx) = this.single_value_index_checked(validated_name) { + if value_iter.len > 1 || single_value_headers[idx] { + if !global_object.has_exception() { + let exception = global_object.to_type_error( + bun_jsc::ErrorCode::HTTP2_HEADER_SINGLE_VALUE, + format_args!( + "Header field \"{}\" must only have a single value", + BStr::new(validated_name) + ), + ); + return Err(global_object.throw_value(exception)); + } + return Ok(JSValue::ZERO); + } + single_value_headers[idx] = true; + } + + while let Some(item) = value_iter.next()? { + if item.is_empty_or_undefined_or_null() { + if !global_object.has_exception() { + return Err(global_object + .err( + JscErrorCode::HTTP2_INVALID_HEADER_VALUE, + format_args!( + "Invalid value for header \"{}\"", + BStr::new(validated_name) + ), + ) + .throw()); + } + return Ok(JSValue::ZERO); + } + + if let Some(ret) = encode_value(item, JSValue::UNDEFINED)? { + return Ok(ret); } } } else if !js_value.is_empty_or_undefined_or_null() { @@ -9178,59 +9109,9 @@ impl H2FrameParser { } single_value_headers[idx] = true; } - let value_str = js_value.to_js_string(global_object)?; - - let never_index = if Self::is_index_like_name(validated_name) { - false - } else { - match sensitive_arg.get_truthy(global_object, validated_name)? { - Some(_) => true, - None => sensitive_arg.get_truthy(global_object, name)?.is_some(), - } - }; - - let value_slice = value_str.to_slice(global_object); - let value = value_slice.slice(); - if !is_valid_header_value(value) { - return Err(global_object - .err( - JscErrorCode::HTTP2_INVALID_HEADER_VALUE, - format_args!( - "Invalid value for header \"{}\"", - BStr::new(validated_name) - ), - ) - .throw()); - } - bun_output::scoped_log!( - H2FrameParser, - "encode header {} {}", - BStr::new(validated_name), - BStr::new(value) - ); - - if let Err(err) = this.encode_header_into_list( - &mut encoded_headers, - validated_name, - value, - never_index, - ) { - if matches!(err, crate::Error::Alloc(_)) { - return Err(global_object - .throw(format_args!("Failed to allocate header buffer"))); - } - let Some(stream) = this.handle_received_stream_id(stream_id) else { - return Ok(JSValue::js_number(-1.0)); - }; - // SAFETY: stream is a *mut Stream from self.streams (heap::alloc); valid while the map entry exists - let stream = unsafe { &mut *stream }; - if !stream_ctx_arg.is_empty_or_undefined_or_null() - && stream_ctx_arg.is_object() - { - stream.set_context(stream_ctx_arg, global_object); - } - this.schedule_header_compression_session_error(); - return Ok(JSValue::js_number(stream_id as f64)); + if let Some(ret) = encode_value(js_value, JSValue::js_number(stream_id as f64))? + { + return Ok(ret); } } } diff --git a/test/js/node/http/node-http-server-socket-surface.test.ts b/test/js/node/http/node-http-server-socket-surface.test.ts new file mode 100644 index 000000000000..a6567110c6c8 --- /dev/null +++ b/test/js/node/http/node-http-server-socket-surface.test.ts @@ -0,0 +1,51 @@ +import { expect, test } from "bun:test"; +import { createServer } from "node:http"; +import type { AddressInfo } from "node:net"; + +// The net.Socket compatibility members shared by the server socket and +// FakeSocket (installSocketStubs in src/js/internal/http.ts). Every assertion +// here also holds for a request socket under real Node. +test("server request socket exposes the net.Socket compatibility surface", async () => { + const { promise, resolve, reject } = Promise.withResolvers>(); + const server = createServer((req, res) => { + try { + const s = req.socket; + resolve({ + readyState: s.readyState, + pending: s.pending, + connecting: s.connecting, + bufferSizeEqualsWritableLength: s.bufferSize === s.writableLength, + refReturnsThis: s.ref() === s, + unrefReturnsThis: s.unref() === s, + setNoDelayReturnsThis: s.setNoDelay() === s, + remoteAddressType: typeof s.remoteAddress, + remotePortType: typeof s.remotePort, + remoteFamilyIsIP: s.remoteFamily === "IPv4" || s.remoteFamily === "IPv6", + }); + } catch (err) { + reject(err); + } finally { + res.end("ok"); + } + }); + try { + await new Promise(resolveListen => server.listen(0, "127.0.0.1", resolveListen)); + const { port } = server.address() as AddressInfo; + const res = await fetch(`http://127.0.0.1:${port}/`); + await res.text(); + expect(await promise).toEqual({ + readyState: "open", + pending: false, + connecting: false, + bufferSizeEqualsWritableLength: true, + refReturnsThis: true, + unrefReturnsThis: true, + setNoDelayReturnsThis: true, + remoteAddressType: "string", + remotePortType: "number", + remoteFamilyIsIP: true, + }); + } finally { + server.close(); + } +}); diff --git a/test/js/node/http2/node-http2-header-validation.test.ts b/test/js/node/http2/node-http2-header-validation.test.ts new file mode 100644 index 000000000000..99d9fba89a30 --- /dev/null +++ b/test/js/node/http2/node-http2-header-validation.test.ts @@ -0,0 +1,104 @@ +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +import http2 from "node:http2"; + +// Client-side header validation in the HTTP/2 frame parser: field names must +// be lowercase tchars, field values must not contain NUL/CR/LF (RFC 9113 +// section 8.2.1), and single-value headers must not repeat. Covers both the +// single-value and array encoding paths. +// +// Name and single-value violations are thrown synchronously from +// `client.request()`; value violations are detected when the header block is +// encoded and surface as an 'error' on the request. `requestError` captures +// whichever of the two delivers. +describe("client request header validation", () => { + let server: http2.Http2Server; + let url: string; + let lastHeaders: http2.IncomingHttpHeaders; + beforeAll(async () => { + server = http2.createServer(); + server.on("stream", (stream, headers) => { + lastHeaders = headers; + stream.respond({ ":status": 200 }, { endStream: true }); + }); + await new Promise(resolve => server.listen(0, resolve)); + url = `http://localhost:${(server.address() as { port: number }).port}`; + }); + afterAll(() => { + server.close(); + }); + + type CodedError = Error & { code?: string }; + + async function requestError(headers: Record): Promise { + const client = http2.connect(url); + client.on("error", () => {}); + try { + let req: http2.ClientHttp2Stream; + try { + req = client.request({ ":path": "/", ...headers }); + } catch (err) { + return err as CodedError; + } + const { promise, resolve, reject } = Promise.withResolvers(); + req.on("error", resolve); + req.on("response", () => reject(new Error("request unexpectedly succeeded"))); + req.end(); + return await promise; + } finally { + client.close(); + } + } + + it("rejects a control character in a single header value", async () => { + for (const bad of ["a\rb", "a\nb", "a\u0000b"]) { + const err = await requestError({ "x-bad": bad }); + expect(err).toBeInstanceOf(TypeError); + expect(err.code).toBe("ERR_HTTP2_INVALID_HEADER_VALUE"); + expect(err.message).toBe('Invalid value for header "x-bad"'); + } + }); + + it("rejects a control character in an array header value", async () => { + const err = await requestError({ "x-arr": ["good", "bad\u0000"] }); + expect(err).toBeInstanceOf(TypeError); + expect(err.code).toBe("ERR_HTTP2_INVALID_HEADER_VALUE"); + expect(err.message).toBe('Invalid value for header "x-arr"'); + }); + + it("rejects an invalid character in a header name", async () => { + const err = await requestError({ "bad header": "v" }); + expect(err).toBeInstanceOf(TypeError); + expect(err.code).toBe("ERR_INVALID_HTTP_TOKEN"); + }); + + it("rejects multiple values for a single-value header", async () => { + const err = await requestError({ "content-type": ["text/plain", "text/html"] }); + expect(err).toBeInstanceOf(TypeError); + expect(err.code).toBe("ERR_HTTP2_HEADER_SINGLE_VALUE"); + expect(err.message).toBe('Header field "content-type" must only have a single value'); + }); + + it("lowercases header names and accepts tchar names and array values", async () => { + const client = http2.connect(url); + const { promise, resolve, reject } = Promise.withResolvers(); + client.on("error", reject); + const req = client.request({ + ":path": "/", + "X-Mixed-CASE": "ok", + "x-multi": ["a", "b"], + "x-t0k3n!#$%&'*+-.^_`|~": "ok", + }); + req.on("response", resolve); + req.on("error", reject); + req.end(); + try { + const res = await promise; + expect(res[":status"]).toBe(200); + expect(lastHeaders["x-mixed-case"]).toBe("ok"); + expect(lastHeaders["x-multi"]).toBe("a, b"); + expect(lastHeaders["x-t0k3n!#$%&'*+-.^_`|~"]).toBe("ok"); + } finally { + client.close(); + } + }); +});