fix: address review findings on input hardening

- Return an explicit error when decompressed mbox hits the 50 MiB
  limit instead of silently truncating the stream
- Raise DefaultBodyLimit from 2 MiB to 25 MiB to accommodate large
  CLI mbox submissions and GitHub webhook payloads (up to 25 MB)
- Read decompressed data into Vec<u8> before UTF-8 conversion to
  avoid InvalidData errors when the byte limit splits a multi-byte
  character
- Extend SSRF blocklist with decimal (2130706433) and hex
  (0x7f000001) representations of loopback addresses
- Replace message-ID slash/backslash blocklist with percent-encoding
  via the percent-encoding crate, preserving RFC 5322 message-IDs
  that contain path-significant characters
- Replace full-URL string matching in is_safe_repo_url with parsed
  host-only checks via the url crate, eliminating false positives
  from blocklist patterns appearing in usernames or paths
- Restructure forge_webhook access control to make the security
  model explicit: when webhook_secret is configured, signature
  verification is the sole access control for ALL requests including
  loopback. The is_loopback bypass only applies when no secret is
  configured. This is critical for reverse proxy deployments where
  all traffic arrives from loopback.

Assisted-by: gemini-3.1-pro
Signed-off-by: derekbarbosa <derekasobrab@gmail.com>

Author: derekbarbosa

Cargo.lock

@@ -67,3 +67,5 @@ tree-sitter-c = "0.24.2"
 hmac = "0.13"
 base64 = "0.22"
 subtle = "2"
+url = "2.5.8"
+percent-encoding = "2.3.2"

Cargo.toml

@@ -66,8 +66,9 @@ if !is_loopback && !has_secret && !state.allow_all_submit {
 
 ### 3. Body Size Limits (`src/api.rs`)
 
-- Explicit `DefaultBodyLimit::max(2 MiB)` on the axum router (makes
-  the implicit axum 0.8 default auditable).
+- Explicit `DefaultBodyLimit::max(25 MiB)` on the axum router.
+  Sized to accommodate large CLI mbox submissions and GitHub
+  webhook payloads (up to 25 MB).
 - HTTP download cap (10 MiB) on `fetch_and_inject_thread` for
   lore.kernel.org mbox fetches.
 - Decompression cap (50 MiB) via `Read::take()` on the gzip decoder

designs/DESIGN_WEBHOOK_INPUT_HARDENING.md

@@ -361,7 +361,7 @@ The webhook payload failed validation.
 
 ### 413 Payload Too Large
 
-The request body exceeds the 2 MiB limit.
+The request body exceeds the 25 MiB limit.
 
 ## FAQ
 

docs/WEBHOOK_SECURITY.md

@@ -309,7 +309,7 @@ pub fn build_router(
         .route("/", get_service(ServeFile::new("static/index.html")))
         .nest_service("/static", ServeDir::new("static"))
         .layer(middleware::from_fn(redirect_www))
-        .layer(axum::extract::DefaultBodyLimit::max(2 * 1024 * 1024))
+        .layer(axum::extract::DefaultBodyLimit::max(25 * 1024 * 1024))
         .with_state(state)
 }
 
@@ -472,14 +472,22 @@ async fn submit_patch(
         }
         SubmitRequest::Thread { msgid } => {
             let id = generate_synthetic_id("thread");
-            let clean_msgid = msgid.trim_matches(|c| c == '<' || c == '>').to_string();
-
-            // Reject message-IDs with path separators to prevent URL path traversal
-            // when constructing the lore.kernel.org fetch URL. Note: ".." is valid
-            // in RFC 5322 local-parts and is not rejected here.
-            if clean_msgid.contains('/') || clean_msgid.contains('\\') {
-                return Err(StatusCode::BAD_REQUEST);
-            }
+            // Percent-encode path-significant characters in the message-ID
+            // for safe inclusion in the lore.kernel.org fetch URL. This
+            // handles RFC 5322 message-IDs that contain `/` or other
+            // path-sensitive characters without rejecting them.
+            const PATH_SEGMENT_ENCODE: &percent_encoding::AsciiSet = &percent_encoding::CONTROLS
+                .add(b'/')
+                .add(b'\\')
+                .add(b'?')
+                .add(b'#')
+                .add(b' ')
+                .add(b'%');
+            let clean_msgid = percent_encoding::utf8_percent_encode(
+                msgid.trim_matches(|c| c == '<' || c == '>'),
+                PATH_SEGMENT_ENCODE,
+            )
+            .to_string();
             info!(
                 "Received thread fetch request: {} (msgid: {})",
                 id, clean_msgid
@@ -556,14 +564,23 @@ async fn fetch_and_inject_thread(
         .into());
     }
 
-    // Decompress the gzip data using a blocking task to avoid blocking the async runtime
+    // Decompress into bytes first, then convert to UTF-8. Reading directly
+    // into a String via read_to_string would produce an InvalidData error
+    // if the byte limit splits a multi-byte UTF-8 character.
     let raw = tokio::task::spawn_blocking(move || -> Result<String, std::io::Error> {
         use std::io::Read;
         let decoder = flate2::read::GzDecoder::new(&bytes[..]);
         let mut limited = decoder.take(MAX_MBOX_DECOMPRESSED);
-        let mut raw = String::new();
-        limited.read_to_string(&mut raw)?;
-        Ok(raw)
+        let mut raw_bytes = Vec::new();
+        limited.read_to_end(&mut raw_bytes)?;
+        if raw_bytes.len() as u64 >= MAX_MBOX_DECOMPRESSED {
+            return Err(std::io::Error::other(format!(
+                "Decompressed mbox exceeds {} byte limit",
+                MAX_MBOX_DECOMPRESSED
+            )));
+        }
+        String::from_utf8(raw_bytes)
+            .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))
     })
     .await??;
 
@@ -1118,20 +1135,28 @@ async fn forge_webhook(
         return Err(StatusCode::FORBIDDEN);
     }
 
-    let is_loopback = addr.ip().to_canonical().is_loopback();
     let webhook_secret = state.settings.forge.webhook_secret.as_deref();
     let has_secret = webhook_secret.is_some();
 
-    // When webhook_secret is configured, the signature check in
-    // validate_event is the access control — no need for
-    // --enable-unsafe-all-submit. Without a secret, fall back to the
-    // existing localhost-only or explicit flag behavior.
-    if !is_loopback && !has_secret && !state.allow_all_submit {
-        info!(
-            "Refused {} webhook from {}: configure webhook_secret or use --enable-unsafe-all-submit",
-            provider, addr
-        );
-        return Err(StatusCode::FORBIDDEN);
+    // Access control: when webhook_secret is configured, signature
+    // verification in validate_event is the sole access control for ALL
+    // requests. This is critical for reverse proxy deployments where all
+    // traffic arrives from loopback — the signature check cannot be
+    // bypassed by source IP.
+    //
+    // When no secret is configured, fall back to localhost-only or the
+    // explicit --enable-unsafe-all-submit flag. Note: this is insecure
+    // behind a reverse proxy; operators MUST configure webhook_secret
+    // for proxied deployments.
+    if !has_secret {
+        let is_loopback = addr.ip().to_canonical().is_loopback();
+        if !is_loopback && !state.allow_all_submit {
+            info!(
+                "Refused {} webhook from {}: configure webhook_secret or use --enable-unsafe-all-submit",
+                provider, addr
+            );
+            return Err(StatusCode::FORBIDDEN);
+        }
     }
 
     let forge = state.forge_registry.get(&provider).ok_or_else(|| {

src/api.rs

@@ -35,23 +35,45 @@ pub fn is_valid_repo_url(url: &str) -> bool {
 }
 
 /// Check that a repository URL does not target known internal or metadata
-/// endpoints. Returns false for cloud metadata services, loopback addresses,
-/// and other destinations that should never be cloned.
+/// endpoints. Parses the URL and checks the host component only, avoiding
+/// false positives from blocklist patterns appearing in usernames or paths.
 ///
 /// This is a best-effort blocklist, not a complete SSRF mitigation. DNS
-/// rebinding and URL encoding can bypass string-based checks. The primary
-/// access control is webhook signature verification.
+/// rebinding can bypass host-based checks. The primary access control
+/// is webhook signature verification.
 pub fn is_safe_repo_url(url: &str) -> bool {
     if !is_valid_repo_url(url) {
         return false;
     }
-    let lower = url.to_lowercase();
-    !lower.contains("169.254.")
-        && !lower.contains("metadata.google")
-        && !lower.contains("localhost")
-        && !lower.contains("127.0.0.1")
-        && !lower.contains("[::1]")
-        && !lower.contains("0.0.0.0")
+    // For git@ URLs, extract the host portion. SSH interprets the LAST
+    // '@' as the user/host separator, so we must do the same to prevent
+    // injection via URLs like "git@github.com@127.0.0.1:repo.git".
+    let host = if let Some(rest) = url.strip_prefix("git@") {
+        let before_colon = rest.split(':').next().unwrap_or("");
+        // Use the portion after the last '@' — this is what SSH resolves
+        let ssh_host = before_colon.rsplit('@').next().unwrap_or(before_colon);
+        ssh_host.to_ascii_lowercase()
+    } else if let Ok(parsed) = url::Url::parse(url) {
+        parsed.host_str().unwrap_or("").to_ascii_lowercase()
+    } else {
+        return false;
+    };
+
+    // Blocklist applied to the host component only
+    !host.contains("169.254.")
+        && !host.contains("metadata.google")
+        && !host.starts_with("localhost")
+        && !host.starts_with("127.")
+        && host != "[::1]"
+        && host != "0.0.0.0"
+        // Decimal representations of loopback (127.0.0.0/8)
+        && !(2130706432..=2130706687).contains(&host.parse::<u64>().unwrap_or(0))
+        // Decimal representation of 169.254.169.254
+        && host != "2852039166"
+        // Hex and octal IP representations
+        && !host.starts_with("0x7f")
+        && !host.starts_with("0xa9fe")
+        && !host.starts_with("0177")
 }
 
 /// Decode a webhook secret. If prefixed with "whsec_", strip the prefix
@@ -106,10 +128,12 @@ fn verify_standard_webhook_signature(
 }
 
 /// Verify a GitHub HMAC-SHA256 signature. The header value has the format
-/// "sha256={hex_digest}".
+/// "sha256={hex_digest}". GitHub sends lowercase hex per their documentation.
+/// The received hex is normalized to lowercase before comparison for
+/// robustness against forges that may use uppercase.
 fn verify_github_signature(secret: &str, body: &[u8], signature_header: &str) -> bool {
     let hex_sig = match signature_header.strip_prefix("sha256=") {
-        Some(s) => s,
+        Some(s) => s.to_ascii_lowercase(),
         None => return false,
     };
     let mut mac = match HmacSha256::new_from_slice(secret.as_bytes()) {
@@ -118,7 +142,11 @@ fn verify_github_signature(secret: &str, body: &[u8], signature_header: &str) ->
     };
     mac.update(body);
     let result = mac.finalize().into_bytes();
-    let computed: String = result.iter().map(|b| format!("{:02x}", b)).collect();
+    let mut computed = String::with_capacity(64);
+    for b in result {
+        use std::fmt::Write;
+        let _ = write!(computed, "{:02x}", b);
+    }
     computed.as_bytes().ct_eq(hex_sig.as_bytes()).into()
 }
 
@@ -146,8 +174,11 @@ pub trait ForgeProvider: Send + Sync {
     fn name(&self) -> &str;
 
     /// Validate webhook event type and verify signature when a secret is
-    /// configured. Returns `UNAUTHORIZED` if the signature is missing or
-    /// invalid, `BAD_REQUEST` if the event type is wrong.
+    /// configured. When `secret` is `None`, only event-type validation is
+    /// performed and the request is treated as unauthenticated — callers
+    /// must enforce their own access control before calling this method.
+    /// Returns `UNAUTHORIZED` if the signature is missing or invalid,
+    /// `BAD_REQUEST` if the event type is wrong.
     fn validate_event(
         &self,
         headers: &HeaderMap,
@@ -487,9 +518,42 @@ mod tests {
         ));
         assert!(!is_safe_repo_url("http://metadata.google.internal/"));
         assert!(!is_safe_repo_url("http://localhost:5432/"));
+        assert!(!is_safe_repo_url("http://localhost.localdomain/repo"));
         assert!(!is_safe_repo_url("http://127.0.0.1:8080/repo"));
+        assert!(!is_safe_repo_url("http://127.1/repo"));
         assert!(!is_safe_repo_url("http://[::1]:8080/repo"));
         assert!(!is_safe_repo_url("http://0.0.0.0/repo"));
+        // Decimal and hex IP representations of 127.0.0.1
+        assert!(!is_safe_repo_url("http://2130706433/repo"));
+        assert!(!is_safe_repo_url("http://0x7f000001/repo"));
+        // Octal representation
+        assert!(!is_safe_repo_url("http://0177.0.0.1/repo"));
+        // Decimal representation of 169.254.169.254
+        assert!(!is_safe_repo_url("http://2852039166/latest/"));
+        // Hex representation of 169.254.x.x
+        assert!(!is_safe_repo_url("http://0xa9fea9fe/latest/"));
+        // Decimal for 127.0.0.2 (other loopback addresses in 127.0.0.0/8)
+        assert!(!is_safe_repo_url("http://2130706434/repo"));
+    }
+
+    #[test]
+    fn test_is_safe_repo_url_blocks_ssh_injection() {
+        // SSH resolves the LAST '@' as user/host separator
+        assert!(!is_safe_repo_url("git@github.com@127.0.0.1:repo.git"));
+        assert!(!is_safe_repo_url("git@github.com@localhost:repo.git"));
+        assert!(!is_safe_repo_url("git@legit.com@169.254.169.254:repo.git"));
+    }
+
+    #[test]
+    fn test_is_safe_repo_url_no_false_positives_on_path() {
+        // Blocklist patterns in username or path should NOT trigger rejection
+        assert!(is_safe_repo_url(
+            "https://github.com/user-127.0.0.1/repo.git"
+        ));
+        assert!(is_safe_repo_url(
+            "https://github.com/org/localhost-tools.git"
+        ));
+        assert!(is_safe_repo_url("git@github.com:0x7f-labs/project.git"));
     }
 
     #[test]