diff --git a/Cargo.lock b/Cargo.lock index 5fd0bb22c0..560931c68d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -845,7 +845,7 @@ dependencies = [ [[package]] name = "curl" version = "0.4.49" -source = "git+https://github.com/lexmount/curl-rust?branch=moli#f28fe3049962d1be917c2d4064f1c6574b331fac" +source = "git+https://github.com/lexmount/curl-rust?branch=moli#bcd4d9f9d7dcd4ec68cfa42f80e9e1f872d87229" dependencies = [ "curl-sys", "libc", @@ -859,7 +859,7 @@ dependencies = [ [[package]] name = "curl-sys" version = "0.4.87+curl-8.19.0" -source = "git+https://github.com/lexmount/curl-rust?branch=moli#f28fe3049962d1be917c2d4064f1c6574b331fac" +source = "git+https://github.com/lexmount/curl-rust?branch=moli#bcd4d9f9d7dcd4ec68cfa42f80e9e1f872d87229" dependencies = [ "brotlic-sys", "cc", diff --git a/moli-curl/src/lib.rs b/moli-curl/src/lib.rs index d60f843e95..e7c540e4ff 100644 --- a/moli-curl/src/lib.rs +++ b/moli-curl/src/lib.rs @@ -5,7 +5,6 @@ mod host_resolve; mod http; mod network_policy; mod proxy; -mod request_headers; mod runtime; mod tls; pub mod websocket; @@ -18,6 +17,5 @@ pub use proxy::{ ConnectionDnsEndpoint, ConnectionEndpointRole, ProxyRoute, ProxyScheme, ProxyTargetResolution, SelectedProxy, select_proxy_route, select_proxy_route_with_env, }; -pub use request_headers::RequestHeaderList; pub use runtime::{CurlMultiRuntime, CurlMultiRuntimeConfig, CurlTransferId}; pub use tls::CurlTlsConfig; diff --git a/moli-curl/src/request_headers.rs b/moli-curl/src/request_headers.rs deleted file mode 100644 index 93701f588e..0000000000 --- a/moli-curl/src/request_headers.rs +++ /dev/null @@ -1,55 +0,0 @@ -use std::ffi::CString; - -use anyhow::{Context, Result}; - -/// An owned libcurl header list that accepts encoded bytes. -/// -/// `curl::easy::List` only accepts UTF-8 strings. Retain this list in the Easy2 -/// handler so it outlives transfers that borrow its native pointer. -#[derive(Default)] -pub struct RequestHeaderList { - raw: *mut curl_sys::curl_slist, -} - -// SAFETY: the list has a single owner and no thread-affine state. Once attached, -// it moves with its Easy2 handler and is never mutated during a transfer. -unsafe impl Send for RequestHeaderList {} - -impl RequestHeaderList { - pub fn append(&mut self, line: &[u8]) -> Result<()> { - let line = CString::new(line).context("HTTP request header contains NUL")?; - // SAFETY: self.raw is null or a live list owned by self. libcurl copies - // the NUL-terminated string; failure leaves the existing list intact. - let raw = unsafe { curl_sys::curl_slist_append(self.raw, line.as_ptr()) }; - if raw.is_null() { - return Err(curl::Error::new(curl_sys::CURLE_OUT_OF_MEMORY).into()); - } - self.raw = raw; - Ok(()) - } - - pub fn as_ptr(&self) -> *mut curl_sys::curl_slist { - self.raw - } -} - -impl Drop for RequestHeaderList { - fn drop(&mut self) { - // SAFETY: this is the sole owner, and Easy2 cleans up the curl handle - // before dropping its handler and this list. - unsafe { curl_sys::curl_slist_free_all(self.raw) }; - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn rejects_nul_without_discarding_the_list() { - let mut headers = RequestHeaderList::default(); - headers.append(b"X-Valid: \xff").unwrap(); - assert!(headers.append(b"X-Invalid: before\0after").is_err()); - headers.append(b"X-Valid: after").unwrap(); - } -} diff --git a/moli-curl/src/websocket/request.rs b/moli-curl/src/websocket/request.rs index 617b05036f..c0a05df5c6 100644 --- a/moli-curl/src/websocket/request.rs +++ b/moli-curl/src/websocket/request.rs @@ -12,8 +12,6 @@ pub(super) struct Handshake { pub error: Option, #[cfg(test)] pub pool_waiting: Option>, - request_headers: crate::RequestHeaderList, - proxy_headers: crate::RequestHeaderList, proxy_connect: bool, header_bytes: usize, } @@ -101,35 +99,13 @@ pub(super) fn configure(request: &CurlWebSocketRequest) -> Result bool { .is_some_and(|(scheme, _)| scheme.eq_ignore_ascii_case("https")) } -fn headers(entries: &moli_header_field::HeaderFields) -> Result { - let mut list = crate::RequestHeaderList::default(); +fn headers(entries: &moli_header_field::HeaderFields) -> Result { + let mut list = List::new(); let mut size = 0usize; for (name, value) in entries { if name.is_empty() @@ -175,7 +151,7 @@ fn headers(entries: &moli_header_field::HeaderFields) -> Result (String, thread:: } fn read_request(stream: &mut TcpStream) -> String { + String::from_utf8(read_request_bytes(stream)).unwrap() +} + +fn read_request_bytes(stream: &mut TcpStream) -> Vec { let mut request = Vec::new(); while !request.ends_with(b"\r\n\r\n") { let mut byte = [0]; @@ -162,7 +166,7 @@ fn read_request(stream: &mut TcpStream) -> String { request.push(byte[0]); assert!(request.len() < 65536); } - String::from_utf8(request).unwrap() + request } fn assert_listener_stays_idle(listener: &TcpListener) { @@ -221,6 +225,64 @@ async fn opened(connection: &mut CurlWebSocketConnection) { } } +#[tokio::test] +async fn native_handshake_preserves_non_utf8_duplicate_and_empty_headers() { + let (finish_tx, finish_rx) = std::sync::mpsc::channel(); + let (url, task) = server(move |mut stream| { + let request = read_request_bytes(&mut stream); + let expected = b"X-Raw: \xe9\xff\r\nX-Raw: \xc3\xa9\r\nX-Empty:\r\n"; + assert!( + request.windows(expected.len()).any(|part| part == expected), + "request headers: {request:?}" + ); + let key = request + .split(|byte| *byte == b'\n') + .find_map(|line| line.strip_prefix(b"Sec-WebSocket-Key: ")) + .unwrap() + .trim_ascii(); + // Tungstenite serializes response headers through UTF-8; use raw bytes + // so the fixture can exercise opaque response headers as well. + let mut response = format!( + "HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Accept: {}\r\n", + derive_accept_key(key) + ).into_bytes(); + response.extend_from_slice(b"x-reply: \xff\x80\r\n\r\n"); + stream.write_all(&response).unwrap(); + finish_rx.recv_timeout(DEADLINE).unwrap(); + }); + let runtime = CurlWebSocketRuntime::new().unwrap(); + let mut request = CurlWebSocketRequest::new(url); + request.headers = moli_header_field::HeaderFields::from_bytes(vec![ + ("X-Raw".to_owned(), vec![0xe9, 0xff]), + ("X-Raw".to_owned(), vec![0xc3, 0xa9]), + ("X-Empty".to_owned(), Vec::new()), + ]); + let mut connection = runtime.connect(request).unwrap(); + match event(&mut connection).await { + CurlWebSocketEvent::Handshake { + request, + response, + result, + } => { + result.unwrap(); + assert!( + request + .windows(b"X-Raw: \xe9\xff\r\n".len()) + .any(|part| part == b"X-Raw: \xe9\xff\r\n") + ); + assert!( + response + .windows(b"x-reply: \xff\x80\r\n".len()) + .any(|part| part == b"x-reply: \xff\x80\r\n") + ); + } + unexpected => panic!("expected a handshake, got {unexpected:?}"), + } + finish_tx.send(()).unwrap(); + drop(connection); + task.join().unwrap(); +} + #[tokio::test] async fn native_upgrade_retains_socket_and_same_packet_empty_frame() { let (url, task) = server(|mut stream| { diff --git a/moli-fetch/src/blocking/mod.rs b/moli-fetch/src/blocking/mod.rs index bb96c84bc1..2db4cf95ee 100644 --- a/moli-fetch/src/blocking/mod.rs +++ b/moli-fetch/src/blocking/mod.rs @@ -1,8 +1,6 @@ mod cache; mod collectors; -pub(crate) use moli_curl::RequestHeaderList; - use std::{ ffi::{c_char, c_long}, net::IpAddr, @@ -677,7 +675,7 @@ pub(crate) fn configure_easy( .context("failed to configure curl host resolve overrides")?; } - let mut headers = RequestHeaderList::default(); + let mut headers = List::new(); let mut outgoing_headers = outgoing_request_header_bytes_for_url(config, request, request_url, cookie_header); // A 407 can come from a transparent proxy even when no explicit proxy @@ -707,7 +705,7 @@ pub(crate) fn configure_easy( header_line.extend_from_slice(value); } headers - .append(&header_line) + .append_bytes(&header_line) .context("failed to build request header")?; } if let Some(validation_headers) = validation_headers { @@ -718,7 +716,7 @@ pub(crate) fn configure_easy( let mut line = format!("{name}: ").into_bytes(); line.extend_from_slice(value); headers - .append(&line) + .append_bytes(&line) .context("failed to build cache validation request header")?; } } @@ -730,7 +728,7 @@ pub(crate) fn configure_easy( // for POST bodies and bodyless PUT requests. Browser requests only send Content-Type when // BodyInit or caller headers produce one, so suppress curl's transport default. headers - .append(b"Content-Type:") + .append("Content-Type:") .context("failed to suppress curl default content-type")?; } @@ -773,7 +771,8 @@ pub(crate) fn configure_easy( } } - crate::runtime::FetchTransferHandler::set_request_headers(easy, headers)?; + easy.http_headers(headers) + .context("failed to attach curl request headers")?; Ok(outgoing_headers.to_byte_strings()) } diff --git a/moli-fetch/src/runtime.rs b/moli-fetch/src/runtime.rs index 07ff03cf08..4c05dbf3f3 100644 --- a/moli-fetch/src/runtime.rs +++ b/moli-fetch/src/runtime.rs @@ -2659,7 +2659,6 @@ struct ActiveRawStreamingTransferContext { } pub(crate) struct FetchTransferHandler { - request_headers: crate::blocking::RequestHeaderList, response: FetchResponseCollector, network_observation_recorder: Option, proxy_connect_response_recorder: ProxyConnectResponseRecorder, @@ -2672,23 +2671,6 @@ enum FetchResponseCollector { } impl FetchTransferHandler { - pub(crate) fn set_request_headers( - easy: &mut Easy2, - headers: crate::blocking::RequestHeaderList, - ) -> Result<()> { - // SAFETY: libcurl borrows this list. On success, retain it in the - // handler until replacement or Easy2 cleanup (which precedes handler - // destruction). Keep the previous list alive if setopt fails. - let result = unsafe { - curl_sys::curl_easy_setopt(easy.raw(), curl_sys::CURLOPT_HTTPHEADER, headers.as_ptr()) - }; - if result != curl_sys::CURLE_OK { - return Err(curl::Error::new(result)).context("failed to attach curl request headers"); - } - easy.get_mut().request_headers = headers; - Ok(()) - } - fn new_buffered(collector: ResponseCollector) -> Self { Self::new(FetchResponseCollector::Buffered(collector)) } @@ -2703,7 +2685,6 @@ impl FetchTransferHandler { fn new(response: FetchResponseCollector) -> Self { Self { - request_headers: crate::blocking::RequestHeaderList::default(), response, network_observation_recorder: None, proxy_connect_response_recorder: ProxyConnectResponseRecorder::default(),