Refactor HttpClient request handling to support redirect logic.

mdegel · mdegel · commit 1acf3614e31d · 2025-11-02T15:07:35.000Z
diff --git a/src/net/http.rs b/src/net/http.rs
@@ -9,15 +9,17 @@ use core::ptr::NonNull;
 use std::io;
 
 use bytes::Bytes;
-use http::uri::Scheme;
-use http::{Request, Response};
+use http::uri::{PathAndQuery, Scheme};
+use http::{Method, Request, Response, StatusCode, Uri};
 use http_body::Body;
 use http_body_util::BodyExt;
 use nginx_sys::{ngx_log_t, ngx_resolver_t, NGX_LOG_WARN};
 use ngx::allocator::Box;
 use ngx::async_::resolver::Resolver;
 use ngx::async_::spawn;
 use ngx::ngx_log_error;
+use std::format;
+use std::string::ToString;
 use thiserror::Error;
 
 use super::peer_conn::PeerConnection;
@@ -40,6 +42,9 @@ const NGX_ACME_USER_AGENT: &str = constcat::concat!(
     NGINX_VER,
 );
 
+// Maximum number of redirects to follow for a single request.
+const MAX_REDIRECTS: usize = 10;
+
 #[allow(async_fn_in_trait)]
 pub trait HttpClient {
     type Error: StdError + Send + Sync + 'static;
@@ -101,26 +106,41 @@ impl<'a> NgxHttpClient<'a> {
 impl HttpClient for NgxHttpClient<'_> {
     type Error = HttpClientError;
 
-    async fn request<B>(&self, mut req: Request<B>) -> Result<Response<Bytes>, Self::Error>
+    async fn request<B>(&self, req: Request<B>) -> Result<Response<Bytes>, Self::Error>
     where
         B: Body + Send + 'static,
         <B as Body>::Data: Send,
         <B as Body>::Error: StdError + Send + Sync,
     {
-        let uri = req.uri().clone();
+        // Buffer the request body so it can be retransmitted across redirects safely.
+        let (mut parts, body) = req.into_parts();
+        let orig_method = parts.method.clone();
+        let mut body_bytes = BodyExt::collect(body)
+            .await
+            .map_err(|e| HttpClientError::Body(std::boxed::Box::new(e)))?
+            .to_bytes();
+
+        let mut current_uri = parts.uri.clone();
 
-        let authority = uri
+        // Basic validation of the starting URI.
+        let mut authority = current_uri
             .authority()
+            .cloned()
             .ok_or(HttpClientError::Uri("missing authority"))?;
 
-        let path_and_query = uri
-            .path_and_query()
-            .ok_or(HttpClientError::Uri("missing path"))?;
+        let mut redirects_followed = 0usize;
+
+        loop {
+            // Prepare request for the current target by setting origin-form URI and required headers.
+            let path_and_query: PathAndQuery = current_uri
+                .path_and_query()
+                .cloned()
+                .ok_or(HttpClientError::Uri("missing path"))?;
 
-        *req.uri_mut() = path_and_query.clone().into();
+            parts.uri = Uri::from(path_and_query.clone());
 
-        {
-            let headers = req.headers_mut();
+            // Build a fresh headers map for each attempt to avoid header leakage across hosts.
+            let mut headers = parts.headers.clone();
             headers.insert(
                 http::header::HOST,
                 http::HeaderValue::from_str(authority.as_str())
@@ -134,50 +154,207 @@ impl HttpClient for NgxHttpClient<'_> {
                 http::header::CONNECTION,
                 http::HeaderValue::from_static("close"),
             );
-        }
 
-        let ssl = if uri.scheme() == Some(&Scheme::HTTPS) {
-            Some(self.ssl.as_ref())
-        } else {
-            None
-        };
+            // Ensure Content-Length matches the body we will send; adjust for GET/HEAD.
+            let sending_method = parts.method.clone();
+            let mut send_body = http_body_util::Full::new(body_bytes.clone());
+            if sending_method == Method::GET || sending_method == Method::HEAD {
+                headers.insert(
+                    http::header::CONTENT_LENGTH,
+                    http::HeaderValue::from_static("0"),
+                );
+                // Empty the body for GET/HEAD attempts.
+                send_body = http_body_util::Full::new(Bytes::new());
+            } else {
+                headers.insert(
+                    http::header::CONTENT_LENGTH,
+                    http::HeaderValue::from_str(&body_bytes.len().to_string())
+                        .map_err(|_| HttpClientError::Uri("bad content length"))?,
+                );
+            }
+
+            let mut req = Request::new(send_body);
+            *req.method_mut() = parts.method.clone();
+            *req.uri_mut() = parts.uri.clone();
+            *req.version_mut() = parts.version;
+            {
+                let hm = req.headers_mut();
+                hm.clear();
+                for (k, v) in headers.iter() {
+                    hm.insert(k, v.clone());
+                }
+            }
+            *req.extensions_mut() = parts.extensions.clone();
 
-        let mut peer = Box::pin(PeerConnection::new(self.log)?);
+            // Establish connection for current target.
+            let ssl = if current_uri.scheme() == Some(&Scheme::HTTPS) {
+                Some(self.ssl.as_ref())
+            } else {
+                None
+            };
 
-        peer.as_mut()
-            .connect_to(authority.as_str(), &self.resolver, ssl)
-            .await?;
+            let mut peer = Box::pin(PeerConnection::new(self.log)?);
+            peer.as_mut()
+                .connect_to(authority.as_str(), &self.resolver, ssl)
+                .await?;
 
-        if ssl.is_some() && self.ssl_verify {
-            if let Err(err) = peer.verify_peer() {
-                let _ = future::poll_fn(|cx| peer.as_mut().poll_shutdown(cx)).await;
-                return Err(err.into());
+            if ssl.is_some() && self.ssl_verify {
+                if let Err(err) = peer.verify_peer() {
+                    let _ = future::poll_fn(|cx| peer.as_mut().poll_shutdown(cx)).await;
+                    return Err(err.into());
+                }
             }
-        }
 
-        if let Some(c) = peer.connection_mut() {
-            c.requests += 1;
-        }
+            if let Some(c) = peer.connection_mut() {
+                c.requests += 1;
+            }
+
+            let (mut sender, conn) = hyper::client::conn::http1::handshake(peer).await?;
+
+            let log = self.log;
+            spawn(async move {
+                if let Err(err) = conn.await {
+                    ngx_log_error!(NGX_LOG_WARN, log.as_ptr(), "connection error: {err}");
+                }
+            })
+            .detach();
 
-        let (mut sender, conn) = hyper::client::conn::http1::handshake(peer).await?;
+            let resp = sender.send_request(req).await?;
 
-        let log = self.log;
-        spawn(async move {
-            if let Err(err) = conn.await {
-                ngx_log_error!(NGX_LOG_WARN, log.as_ptr(), "connection error: {err}");
+            // If not a redirect, return the response with its body collected.
+            if !is_redirect(resp.status()) {
+                let (parts, body) = resp.into_parts();
+                let body = http_body_util::Limited::new(body, NGX_ACME_MAX_BODY_SIZE)
+                    .collect()
+                    .await
+                    .map_err(HttpClientError::Body)?
+                    .to_bytes();
+                return Ok(Response::from_parts(parts, body));
             }
-        })
-        .detach();
 
-        let resp = sender.send_request(req).await?;
-        let (parts, body) = resp.into_parts();
+            // For non-GET/HEAD, do not auto-follow redirects.
+            // ACME JWS 'url' must match the request URL; on redirect the client must re-sign and
+            // POST to the new Location itself (RFC 8555 §6.2). Return the 3xx so the ACME layer
+            // can handle it.
+            if !(parts.method == Method::GET || parts.method == Method::HEAD) {
+                let (parts_r, body) = resp.into_parts();
+                let body = http_body_util::Limited::new(body, NGX_ACME_MAX_BODY_SIZE)
+                    .collect()
+                    .await
+                    .map_err(HttpClientError::Body)?
+                    .to_bytes();
+                return Ok(Response::from_parts(parts_r, body));
+            }
 
-        let body = http_body_util::Limited::new(body, NGX_ACME_MAX_BODY_SIZE)
-            .collect()
-            .await
-            .map_err(HttpClientError::Body)?
-            .to_bytes();
+            // Handle redirect logic.
+            redirects_followed += 1;
+            if redirects_followed > MAX_REDIRECTS {
+                return Err(HttpClientError::Uri("too many redirects"));
+            }
+
+            let location = resp
+                .headers()
+                .get(http::header::LOCATION)
+                .ok_or(HttpClientError::Uri("redirect without location"))?;
 
-        Ok(Response::from_parts(parts, body))
+            let new_uri = resolve_location(&current_uri, location)?;
+
+            // Prevent HTTPS -> HTTP downgrade unless the original request was HTTP.
+            if current_uri.scheme() == Some(&Scheme::HTTPS)
+                && new_uri.scheme() == Some(&Scheme::HTTP)
+            {
+                return Err(HttpClientError::Uri("insecure redirect from https to http"));
+            }
+
+            // Adjust method and body according to status code per RFC 9110.
+            match resp.status() {
+                StatusCode::SEE_OTHER => {
+                    // 303: switch to GET, except HEAD remains HEAD
+                    parts.method = if orig_method == Method::HEAD {
+                        Method::HEAD
+                    } else {
+                        Method::GET
+                    };
+                    body_bytes = Bytes::new();
+                    // Drop content-type if any
+                    parts.headers.remove(http::header::CONTENT_TYPE);
+                }
+                StatusCode::MOVED_PERMANENTLY | StatusCode::FOUND => {
+                    // 301/302: preserve method and body
+                    parts.method = orig_method.clone();
+                }
+                StatusCode::PERMANENT_REDIRECT | StatusCode::TEMPORARY_REDIRECT => {
+                    // 308/307: MUST NOT change method
+                    // no-op; parts.method unchanged
+                }
+                _ => {}
+            }
+
+            // Update target for next loop iteration.
+            authority = new_uri
+                .authority()
+                .cloned()
+                .ok_or(HttpClientError::Uri("bad redirect authority"))?;
+            current_uri = new_uri;
+        }
+    }
+}
+
+fn is_redirect(status: StatusCode) -> bool {
+    matches!(
+        status,
+        StatusCode::MOVED_PERMANENTLY
+            | StatusCode::FOUND
+            | StatusCode::SEE_OTHER
+            | StatusCode::TEMPORARY_REDIRECT
+            | StatusCode::PERMANENT_REDIRECT
+    )
+}
+
+fn resolve_location(base: &Uri, location: &http::HeaderValue) -> Result<Uri, HttpClientError> {
+    let loc_str = location
+        .to_str()
+        .map_err(|_| HttpClientError::Uri("bad location header"))?;
+
+    // Absolute URI
+    if let Ok(uri) = loc_str.parse::<Uri>() {
+        if uri.scheme().is_some() && uri.authority().is_some() {
+            return Ok(uri);
+        }
+    }
+
+    // Relative forms
+    if loc_str.starts_with('/') {
+        // absolute-path reference
+        let mut builder = Uri::builder();
+        if let Some(scheme) = base.scheme() {
+            builder = builder.scheme(scheme.clone());
+        }
+        if let Some(auth) = base.authority() {
+            builder = builder.authority(auth.clone());
+        }
+        builder
+            .path_and_query(loc_str)
+            .build()
+            .map_err(|_| HttpClientError::Uri("bad redirect path"))
+    } else {
+        // path-relative; join with base path directory
+        let base_path = base.path_and_query().map(|pq| pq.as_str()).unwrap_or("/");
+        let joined = if let Some(idx) = base_path.rfind('/') {
+            format!("{}{}", &base_path[..=idx], loc_str)
+        } else {
+            format!("/{}", loc_str)
+        };
+        let mut builder = Uri::builder();
+        if let Some(scheme) = base.scheme() {
+            builder = builder.scheme(scheme.clone());
+        }
+        if let Some(auth) = base.authority() {
+            builder = builder.authority(auth.clone());
+        }
+        builder
+            .path_and_query(joined.as_str())
+            .build()
+            .map_err(|_| HttpClientError::Uri("bad relative redirect path"))
     }
 }
