Revert "Refactor HttpClient request handling to support redirect logic."

mdegel · mdegel · commit a508f09c3ec8 · 2025-11-07T14:01:40.000Z
This reverts commit 1acf361.
diff --git a/src/net/http.rs b/src/net/http.rs
@@ -9,17 +9,15 @@ use core::ptr::NonNull;
 use std::io;
 
 use bytes::Bytes;
-use http::uri::{PathAndQuery, Scheme};
-use http::{Method, Request, Response, StatusCode, Uri};
+use http::uri::Scheme;
+use http::{Request, Response};
 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;
@@ -42,9 +40,6 @@ 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;
@@ -106,41 +101,26 @@ impl<'a> NgxHttpClient<'a> {
 impl HttpClient for NgxHttpClient<'_> {
     type Error = HttpClientError;
 
-    async fn request<B>(&self, req: Request<B>) -> Result<Response<Bytes>, Self::Error>
+    async fn request<B>(&self, mut 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,
     {
-        // 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 uri = req.uri().clone();
 
-        // Basic validation of the starting URI.
-        let mut authority = current_uri
+        let authority = uri
             .authority()
-            .cloned()
             .ok_or(HttpClientError::Uri("missing authority"))?;
 
-        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"))?;
+        let path_and_query = uri
+            .path_and_query()
+            .ok_or(HttpClientError::Uri("missing path"))?;
 
-            parts.uri = Uri::from(path_and_query.clone());
+        *req.uri_mut() = path_and_query.clone().into();
 
-            // Build a fresh headers map for each attempt to avoid header leakage across hosts.
-            let mut headers = parts.headers.clone();
+        {
+            let headers = req.headers_mut();
             headers.insert(
                 http::header::HOST,
                 http::HeaderValue::from_str(authority.as_str())
@@ -154,207 +134,50 @@ impl HttpClient for NgxHttpClient<'_> {
                 http::header::CONNECTION,
                 http::HeaderValue::from_static("close"),
             );
+        }
 
-            // 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();
-
-            // Establish connection for current target.
-            let ssl = if current_uri.scheme() == Some(&Scheme::HTTPS) {
-                Some(self.ssl.as_ref())
-            } else {
-                None
-            };
-
-            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 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 ssl = if uri.scheme() == Some(&Scheme::HTTPS) {
+            Some(self.ssl.as_ref())
+        } else {
+            None
+        };
 
-            let resp = sender.send_request(req).await?;
+        let mut peer = Box::pin(PeerConnection::new(self.log)?);
 
-            // 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));
-            }
+        peer.as_mut()
+            .connect_to(authority.as_str(), &self.resolver, ssl)
+            .await?;
 
-            // 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));
+        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());
             }
+        }
 
-            // 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"))?;
-
-            let new_uri = resolve_location(&current_uri, location)?;
+        if let Some(c) = peer.connection_mut() {
+            c.requests += 1;
+        }
 
-            // 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"));
-            }
+        let (mut sender, conn) = hyper::client::conn::http1::handshake(peer).await?;
 
-            // 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
-                }
-                _ => {}
+        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();
 
-            // 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"))?;
+        let resp = sender.send_request(req).await?;
+        let (parts, body) = resp.into_parts();
 
-    // Absolute URI
-    if let Ok(uri) = loc_str.parse::<Uri>() {
-        if uri.scheme().is_some() && uri.authority().is_some() {
-            return Ok(uri);
-        }
-    }
+        let body = http_body_util::Limited::new(body, NGX_ACME_MAX_BODY_SIZE)
+            .collect()
+            .await
+            .map_err(HttpClientError::Body)?
+            .to_bytes();
 
-    // 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"))
+        Ok(Response::from_parts(parts, body))
     }
 }
