From 38d47ffc7e674a7557544eaacbe2a9151a8670a7 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Fri, 11 Sep 2026 19:50:08 -0400 Subject: [PATCH] fix(http): bound the bodies read from a host we do not control A DID document, a describeServer answer, an IMDS or Route53 response and an ACME response now all go through the bounded reader, which grows a blocking half for the two callers with no runtime. Co-Authored-By: Claude Opus 5 (1M context) --- crates/didbot-claim/src/describe.rs | 7 +- crates/didbot-claim/src/identify.rs | 6 +- crates/didbot-dns/src/route53.rs | 17 +++-- crates/didbot-http/src/lib.rs | 95 +++++++++++++++++++++++++++- crates/didbot-tls/src/http_client.rs | 6 +- 5 files changed, 115 insertions(+), 16 deletions(-) diff --git a/crates/didbot-claim/src/describe.rs b/crates/didbot-claim/src/describe.rs index 240fe3f7..bb1511ee 100644 --- a/crates/didbot-claim/src/describe.rs +++ b/crates/didbot-claim/src/describe.rs @@ -60,10 +60,11 @@ impl ServerDescriber for HttpServerDescriber { .map_err(|error| format!("fetching {url}: {error}"))? .error_for_status() .map_err(|error| format!("{url} answered: {error}"))?; - let body: DescribeServerResponse = response - .json() + let bytes = didbot_http::read_bounded(response, didbot_http::MAX_JSON_BODY) .await - .map_err(|error| format!("parsing {url}: {error}"))?; + .map_err(|error| format!("reading {url}: {error}"))?; + let body: DescribeServerResponse = + serde_json::from_slice(&bytes).map_err(|error| format!("parsing {url}: {error}"))?; Ok(body.did) } } diff --git a/crates/didbot-claim/src/identify.rs b/crates/didbot-claim/src/identify.rs index 18dc7c8b..9d380b17 100644 --- a/crates/didbot-claim/src/identify.rs +++ b/crates/didbot-claim/src/identify.rs @@ -217,13 +217,13 @@ impl DocumentFetcher for HttpDocumentFetcher { url: url.to_owned(), message: error.to_string(), })?; - response - .text() + let body = didbot_http::read_bounded(response, didbot_http::MAX_JSON_BODY) .await .map_err(|error| ResolveError::Transport { url: url.to_owned(), message: error.to_string(), - }) + })?; + Ok(didbot_http::body_text(body)) } } diff --git a/crates/didbot-dns/src/route53.rs b/crates/didbot-dns/src/route53.rs index aa42ff87..5233a880 100644 --- a/crates/didbot-dns/src/route53.rs +++ b/crates/didbot-dns/src/route53.rs @@ -376,13 +376,14 @@ fn api_client() -> reqwest::blocking::Client { } fn imds_request(request: reqwest::blocking::RequestBuilder) -> Result { - request + let response = request .send() .map_err(|err| ImdsError::Request(err.to_string()))? .error_for_status() - .map_err(|err| ImdsError::Request(err.to_string()))? - .text() - .map_err(|err| ImdsError::Request(err.to_string())) + .map_err(|err| ImdsError::Request(err.to_string()))?; + let body = didbot_http::read_bounded_blocking(response, didbot_http::MAX_JSON_BODY) + .map_err(|err| ImdsError::Request(err.to_string()))?; + Ok(didbot_http::body_text(body)) } /// Reads the credentials out of a @@ -1369,8 +1370,12 @@ impl Transport for HttpTransport { let response = request.send().map_err(|error| error.to_string())?; let status = response.status().as_u16(); - let body = response.text().map_err(|error| error.to_string())?; - Ok(Response { status, body }) + let body = didbot_http::read_bounded_blocking(response, didbot_http::MAX_JSON_BODY) + .map_err(|error| error.to_string())?; + Ok(Response { + status, + body: didbot_http::body_text(body), + }) } } diff --git a/crates/didbot-http/src/lib.rs b/crates/didbot-http/src/lib.rs index 78c50faa..f8c0f04a 100644 --- a/crates/didbot-http/src/lib.rs +++ b/crates/didbot-http/src/lib.rs @@ -69,9 +69,8 @@ pub fn client() -> reqwest::Client { builder().build().unwrap_or_default() } -/// The most of one response body this workspace will hold in memory to -/// parse it as JSON, when the response comes from a server nothing here -/// operates. +/// The most of one response body this workspace will hold in memory to read +/// it, when the response comes from a server nothing here operates. /// /// Two mebibytes -- the same magnitude `didbot-serve`'s own /// `MAX_REQUEST_BODY` already treats as a safe amount of one JSON payload @@ -95,6 +94,10 @@ pub enum ReadBoundedError { /// The connection or the HTTP exchange itself failed while reading. #[error("reading response body: {0}")] Transport(#[from] reqwest::Error), + /// The same, for a blocking response, which reports through + /// [`std::io`] rather than through `reqwest`. + #[error("reading response body: {0}")] + Io(#[from] std::io::Error), } /// Reads at most `max_bytes` of `response`'s body. @@ -124,6 +127,45 @@ pub async fn read_bounded( Ok(body) } +/// [`read_bounded`], for a caller with no async runtime. +#[cfg(feature = "blocking")] +pub fn read_bounded_blocking( + mut response: reqwest::blocking::Response, + max_bytes: usize, +) -> Result, ReadBoundedError> { + use std::io::Read; + + if response + .content_length() + .is_some_and(|len| len > max_bytes as u64) + { + return Err(ReadBoundedError::TooLarge { max_bytes }); + } + // One byte past the bound, so a body that declares a small length and + // then sends more is caught by what arrived rather than by what it said. + let mut body = Vec::new(); + response + .by_ref() + .take(max_bytes as u64 + 1) + .read_to_end(&mut body)?; + if body.len() > max_bytes { + return Err(ReadBoundedError::TooLarge { max_bytes }); + } + Ok(body) +} + +/// The body of a bounded read, as text. +/// +/// Lossy, like `reqwest`'s own `text()`: a document that is not UTF-8 is +/// already going to fail whatever parses it, and saying so there names the +/// document rather than the encoding. +pub fn body_text(body: Vec) -> String { + match String::from_utf8(body) { + Ok(text) => text, + Err(err) => String::from_utf8_lossy(err.as_bytes()).into_owned(), + } +} + /// A server that has stopped answering, for tests of a client's timeouts. #[cfg(feature = "test-support")] pub mod test_support { @@ -196,6 +238,53 @@ mod tests { assert_eq!(read, exact); } + /// A loopback server that answers every request with a chunked body it + /// never finishes. Chunked means no `Content-Length`, so there is no + /// header to refuse up front and the bound is the only thing stopping + /// the read. + #[cfg(feature = "blocking")] + fn endless_chunked_server() -> std::net::SocketAddr { + use std::io::Write; + + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("loopback binds"); + let addr = listener + .local_addr() + .expect("a bound listener has an address"); + std::thread::spawn(move || { + for mut stream in listener.incoming().flatten() { + if stream + .write_all(b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n") + .is_err() + { + continue; + } + let chunk = format!("2000\r\n{}\r\n", "x".repeat(0x2000)); + // Until the reader gives up and drops the connection. + while stream.write_all(chunk.as_bytes()).is_ok() {} + } + }); + addr + } + + #[cfg(feature = "blocking")] + #[test] + fn a_blocking_body_that_declares_no_length_is_still_cut_off_at_the_bound() { + let addr = endless_chunked_server(); + let response = blocking_builder() + .build() + .expect("the blocking client builds") + .get(format!("http://{addr}/")) + .send() + .expect("the server answers"); + assert!(response.content_length().is_none(), "chunked declares none"); + + let err = read_bounded_blocking(response, 64 * 1024).unwrap_err(); + assert!( + matches!(err, ReadBoundedError::TooLarge { max_bytes } if max_bytes == 64 * 1024), + "{err:?}" + ); + } + /// A server that accepts the connection and never answers costs the /// default client exactly [`DEFAULT_TIMEOUT`], then a timeout error. #[tokio::test(start_paused = true)] diff --git a/crates/didbot-tls/src/http_client.rs b/crates/didbot-tls/src/http_client.rs index 429aebbd..95c6a0c2 100644 --- a/crates/didbot-tls/src/http_client.rs +++ b/crates/didbot-tls/src/http_client.rs @@ -75,7 +75,11 @@ impl HttpClient for ReqwestAcmeClient { let response = builder.send().await.map_err(other)?; let status = response.status(); let headers = response.headers().clone(); - let response_bytes = response.bytes().await.map_err(other)?; + let response_bytes = Bytes::from( + didbot_http::read_bounded(response, didbot_http::MAX_JSON_BODY) + .await + .map_err(other)?, + ); let mut http_response = http::Response::builder().status(status); if let Some(response_headers) = http_response.headers_mut() { -- 2.51.2