From 183c1d8cb4c616d214c7aafe7ac1525e339532d0 Mon Sep 17 00:00:00 2001
From: Zhengchao An <anzhengchao@gmail.com>
Date: Wed, 8 Jul 2026 16:05:37 +0800
Subject: [PATCH] fix(targets): disable HTTP redirects on webhook delivery
 client (#4420)

fix(targets): disable HTTP redirects on webhook delivery client (backlog#974)

The webhook reqwest client used the default redirect policy (follows up to
10 redirects). Even though the configured endpoint is validated against
outbound-egress rules, a malicious or compromised endpoint could return a
3xx response to bounce the outbound request to an internal address (e.g. the
cloud metadata service at 169.254.169.254), bypassing that validation (SSRF).

- Set `.redirect(reqwest::redirect::Policy::none())` on the delivery client
  so redirects are never followed.
- Treat a 3xx response as a delivery failure (error carries the status code)
  instead of silently succeeding.
- Add a regression test that stands up a loopback server returning a 302 to
  an internal metadata address and asserts the client surfaces the 3xx rather
  than following it.

Refs: https://github.com/rustfs/backlog/issues/974
---
 crates/targets/src/target/webhook.rs | 56 ++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/crates/targets/src/target/webhook.rs b/crates/targets/src/target/webhook.rs
index 1993b2fcd..ac840e54f 100644
--- a/crates/targets/src/target/webhook.rs
+++ b/crates/targets/src/target/webhook.rs
@@ -223,6 +223,12 @@ where
     fn build_http_client(args: &WebhookArgs) -> Result<Client, TargetError> {
         let mut client_builder = Client::builder()
             .timeout(Duration::from_secs(30))
+            // SSRF hardening (backlog#974): never follow HTTP redirects on webhook delivery.
+            // reqwest follows up to 10 redirects by default, which lets a malicious or
+            // compromised endpoint use a 3xx response to bounce the outbound request to an
+            // internal address (e.g. the cloud metadata service at 169.254.169.254),
+            // bypassing the outbound-endpoint validation performed on the configured URL.
+            .redirect(reqwest::redirect::Policy::none())
             .user_agent(crate::get_user_agent(crate::ServiceType::Basis));
         #[cfg(test)]
         {
@@ -458,6 +464,14 @@ where
             );
             self.delivery_counters.record_success();
             Ok(())
+        } else if status.is_redirection() {
+            // SSRF hardening (backlog#974): redirects are intentionally not followed
+            // (see build_http_client). Treat a 3xx response as a delivery failure rather
+            // than silently chasing the redirect to a potentially internal target.
+            Err(TargetError::Request(format!(
+                "{} returned redirect '{}'; redirects are not followed for webhook delivery",
+                self.args.endpoint, status
+            )))
         } else if status == StatusCode::FORBIDDEN {
             Err(TargetError::Authentication(format!(
                 "{} returned '{}', please check if your auth token is correctly set",
@@ -833,4 +847,46 @@ mod tests {
         };
         assert!(err.to_string().contains("not allowed"));
     }
+
+    // SSRF hardening regression (backlog#974): the delivery client must not follow HTTP
+    // redirects, otherwise a 3xx from the endpoint could bounce the outbound request to an
+    // internal address (e.g. the cloud metadata service) and bypass endpoint validation.
+    #[tokio::test]
+    async fn test_webhook_client_does_not_follow_redirects() {
+        use std::io::{Read, Write};
+        use std::net::TcpListener;
+
+        // Minimal HTTP server on an ephemeral loopback port.
+        let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock server");
+        let addr = listener.local_addr().expect("local addr");
+
+        // Serve exactly one request with a 302 pointing at an internal metadata address.
+        // If the client followed the redirect it would issue a second request to that
+        // (unreachable) target instead of returning the 3xx status.
+        let handle = std::thread::spawn(move || {
+            if let Ok((mut stream, _)) = listener.accept() {
+                let mut buf = [0u8; 1024];
+                let _ = stream.read(&mut buf);
+                let response =
+                    "HTTP/1.1 302 Found\r\nLocation: http://169.254.169.254/latest/meta-data/\r\nContent-Length: 0\r\n\r\n";
+                let _ = stream.write_all(response.as_bytes());
+                let _ = stream.flush();
+            }
+        });
+
+        let client = WebhookTarget::<serde_json::Value>::build_http_client(&base_args()).expect("build client");
+
+        let resp = client
+            .post(format!("http://{addr}/hook"))
+            .body("{}")
+            .send()
+            .await
+            .expect("request should complete without following the redirect");
+
+        // Redirects are disabled, so the 3xx is surfaced as-is rather than chased to the
+        // internal Location target.
+        assert_eq!(resp.status().as_u16(), 302, "webhook client must not follow redirects");
+
+        handle.join().expect("mock server thread");
+    }
 }