volume server: reject non-ASCII input instead of panicking (#11406)

* volume server: reject non-ASCII input instead of panicking

Three parsers sliced attacker-supplied strings by byte offset, so a
multi-byte character split inside itself and panicked the task:

  - parse_needle_id_cookie took the last 8 bytes as the cookie and the
    rest as the needle id. Reachable from VolumeServer.BatchDelete,
    whose file_ids come straight off the wire as protobuf strings;
    that handler already answers 400 per bad fid, so the guard turns a
    panicked RPC into the error it was already written to return.

  - TTL::read took the unit as the last byte and the count as
    everything before it, so "?ttl=5<multi-byte>" split mid-character.
    The HTTP upload path does TTL::read(..).ok() and drops an invalid
    TTL; AllocateVolume maps the Err to InvalidArgument.

Both now reject non-ASCII up front. Hex and a digits-plus-unit TTL are
ASCII by definition, so no accepted input changes -- covered by tests
alongside the rejection cases.

The six response-* header overrides were inserted with
parse().unwrap(). They come from the query string, so
"?response-cache-control=%0Aevil" decodes to a value HeaderValue
rejects and the unwrap panicked the connection task,
unauthenticated. They now skip the override, matching the if-let the
chunked-response path in the same file already uses.

ReplicaPlacement::from_string was reported as a fourth site but is not
one: reaching chars[2] requires chars[0] and chars[1] to be ASCII
digits, which forces the padded string to be three single-byte
characters, so a multi-byte character always lands on a to_digit()
None first. Kept as a regression test rather than a change.

Each fix was confirmed against the unfixed code first: the parser
tests panic with "byte index N is not a char boundary", and the
integration tests panic at handlers.rs:1413 and ttl.rs:88.

Not a vector, contrary to the report: the HTTP request line. The path
is not percent-decoded before parsing, so "%C3%A9" stays ASCII and
fails the length check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* volume server: fall back to needle MIME when response-content-type is invalid

Skipping an unparseable override left the response without any
Content-Type because the override had already bypassed the normal MIME
selection. Also correct a test comment that described a chars[2] panic
which cannot be reached.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: chrislusf <chrislusf@users.noreply.github.com>

seaweed-volume/src/server/handlers.rs

@@ -1351,9 +1351,7 @@ async fn get_or_head_handler_inner(
 
     // H6: Determine Content-Type: filter application/octet-stream, use mime_guess
     // For chunk manifests, skip extension-based MIME override — use stored MIME as-is (Go parity)
-    let content_type = if let Some(ref ct) = query.response_content_type {
-        Some(ct.clone())
-    } else if n.is_chunk_manifest() {
+    let content_type = if n.is_chunk_manifest() {
         // Chunk manifests: use stored MIME but filter application/octet-stream (Go L334)
         if !n.mime.is_empty() {
             let mt = String::from_utf8_lossy(&n.mime).to_string();
@@ -1402,27 +1400,50 @@ async fn get_or_head_handler_inner(
             }
         }
     };
-    if let Some(ref ct) = content_type {
-        response_headers.insert(header::CONTENT_TYPE, ct.parse().unwrap());
+    // Every value below can come straight from the query string, so none of
+    // them may be unwrapped: `?response-cache-control=%0Aevil` decodes to a
+    // value with a newline, `HeaderValue::from_str` rejects it, and the unwrap
+    // would panic the connection task. An invalid `response-content-type`
+    // falls back to the needle MIME rather than dropping Content-Type.
+    if let Some(hval) = query
+        .response_content_type
+        .as_ref()
+        .and_then(|ct| ct.parse::<header::HeaderValue>().ok())
+    {
+        response_headers.insert(header::CONTENT_TYPE, hval);
+    } else if let Some(ref ct) = content_type
+        && let Ok(hval) = ct.parse()
+    {
+        response_headers.insert(header::CONTENT_TYPE, hval);
     }
 
     // Cache-Control override from query param
-    if let Some(ref cc) = query.response_cache_control {
-        response_headers.insert(header::CACHE_CONTROL, cc.parse().unwrap());
+    if let Some(ref cc) = query.response_cache_control
+        && let Ok(hval) = cc.parse()
+    {
+        response_headers.insert(header::CACHE_CONTROL, hval);
     }
 
     // S3 response passthrough headers
-    if let Some(ref ce) = query.response_content_encoding {
-        response_headers.insert(header::CONTENT_ENCODING, ce.parse().unwrap());
+    if let Some(ref ce) = query.response_content_encoding
+        && let Ok(hval) = ce.parse()
+    {
+        response_headers.insert(header::CONTENT_ENCODING, hval);
     }
-    if let Some(ref exp) = query.response_expires {
-        response_headers.insert(header::EXPIRES, exp.parse().unwrap());
+    if let Some(ref exp) = query.response_expires
+        && let Ok(hval) = exp.parse()
+    {
+        response_headers.insert(header::EXPIRES, hval);
     }
-    if let Some(ref cl) = query.response_content_language {
-        response_headers.insert("Content-Language", cl.parse().unwrap());
+    if let Some(ref cl) = query.response_content_language
+        && let Ok(hval) = cl.parse()
+    {
+        response_headers.insert("Content-Language", hval);
     }
-    if let Some(ref cd) = query.response_content_disposition {
-        response_headers.insert(header::CONTENT_DISPOSITION, cd.parse().unwrap());
+    if let Some(ref cd) = query.response_content_disposition
+        && let Ok(hval) = cd.parse()
+    {
+        response_headers.insert(header::CONTENT_DISPOSITION, hval);
     }
 
     // Last-Modified

seaweed-volume/src/storage/needle/needle.rs

@@ -749,6 +749,14 @@ pub fn parse_needle_id_cookie(s: &str) -> Result<(NeedleId, Cookie), String> {
         (s, None)
     };
 
+    // Every length check and the split below are in BYTES, so a multi-byte
+    // character would let `split` land inside one and panic the slice. Hex is
+    // ASCII by definition; reject anything else up front, as Go's ParseUint
+    // does a step later.
+    if !hex_part.is_ascii() {
+        return Err("KeyHash must be ASCII hex.".to_string());
+    }
+
     // Go: len(key_hash_string) <= CookieSize*2 => error (must be > 8 hex chars)
     if hex_part.len() <= COOKIE_SIZE * 2 {
         return Err("KeyHash is too short.".to_string());
@@ -828,6 +836,30 @@ pub enum NeedleError {
 mod tests {
     use super::*;
 
+    /// A fid whose hex part carries multi-byte UTF-8 must be rejected, not
+    /// panic. `split` is a byte offset into `hex_part`; before the ASCII guard
+    /// `&hex_part[..split]` could land inside a character. `GET /3,ééééa` is
+    /// nine bytes, so it passes the length checks and splits at byte 1 —
+    /// halfway through the first `é`. Go's `ParseUint` just errors.
+    #[test]
+    fn parse_needle_id_cookie_rejects_non_ascii_instead_of_panicking() {
+        for s in ["ééééa", "ééééaaaaa", "0123456é9abc", "ééééa_1"] {
+            assert!(
+                parse_needle_id_cookie(s).is_err(),
+                "non-ASCII fid {:?} must be an error",
+                s
+            );
+        }
+    }
+
+    /// The ASCII guard must not change any accepted input.
+    #[test]
+    fn parse_needle_id_cookie_still_accepts_ascii_hex() {
+        let (id, cookie) = parse_needle_id_cookie("01637037d6").unwrap();
+        assert_eq!(id, NeedleId(0x01));
+        assert_eq!(cookie, Cookie(0x637037d6));
+    }
+
     #[test]
     fn test_parse_header() {
         let mut buf = [0u8; NEEDLE_HEADER_SIZE];

seaweed-volume/src/storage/needle/ttl.rs

@@ -80,6 +80,12 @@ impl TTL {
         if s.is_empty() {
             return Ok(TTL::EMPTY);
         }
+        // The unit is read as the last BYTE and the count as everything before
+        // it, so a trailing multi-byte character would split inside itself and
+        // panic. A TTL is digits plus a one-letter unit; reject the rest.
+        if !s.is_ascii() {
+            return Err(format!("invalid TTL {:?}: must be ASCII", s));
+        }
         let last_byte = s.as_bytes()[s.len() - 1];
         let (num_str, unit_byte) = if last_byte.is_ascii_digit() {
             // All digits — default to minutes (matching Go)
@@ -240,6 +246,16 @@ impl fmt::Display for TTL {
 mod tests {
     use super::*;
 
+    /// `?ttl=5%C3%A9` must be an error, not a panic. The unit is taken as the
+    /// last *byte*, so a trailing multi-byte character made `&s[..s.len()-1]`
+    /// split inside it.
+    #[test]
+    fn ttl_read_rejects_non_ascii_instead_of_panicking() {
+        for s in ["5é", "é", "3🦀", "12é"] {
+            assert!(TTL::read(s).is_err(), "non-ASCII TTL {:?} must error", s);
+        }
+    }
+
     #[test]
     fn test_ttl_parse() {
         let ttl = TTL::read("3m").unwrap();

seaweed-volume/src/storage/super_block.rs

@@ -221,6 +221,35 @@ mod tests {
     use super::*;
     use crate::storage::types::*;
 
+    /// Multi-byte input must be an error, not a panic: `to_digit` on the
+    /// leading characters rejects it before `chars[2]` is ever indexed.
+    #[test]
+    fn replica_placement_rejects_non_ascii_instead_of_panicking() {
+        for s in ["é", "0é", "é0", "🦀", "ééé"] {
+            assert!(
+                ReplicaPlacement::from_string(s).is_err(),
+                "non-ASCII replication {:?} must error",
+                s
+            );
+        }
+    }
+
+    /// The ASCII guard must not change any accepted input, including the
+    /// zero-padding shorthands.
+    #[test]
+    fn replica_placement_still_accepts_ascii_shorthands() {
+        assert_eq!(
+            ReplicaPlacement::from_string("1").unwrap(),
+            ReplicaPlacement::from_string("001").unwrap()
+        );
+        assert_eq!(
+            ReplicaPlacement::from_string("01").unwrap(),
+            ReplicaPlacement::from_string("001").unwrap()
+        );
+        let rp = ReplicaPlacement::from_string("010").unwrap();
+        assert_eq!(rp.diff_rack_count, 1);
+    }
+
     #[test]
     fn test_super_block_round_trip() {
         let sb = SuperBlock {

seaweed-volume/tests/http_integration.rs

@@ -1142,3 +1142,116 @@ async fn delete_on_ec_volume_succeeds_when_the_needles_shard_is_not_mounted() {
         "the delete must have been journalled, not just answered 202"
     );
 }
+
+// ============================================================================
+// Hostile response-header override params must not panic the handler
+//
+// The `response-*` query params are attacker-controlled and were inserted with
+// `parse().unwrap()`. `%0A` decodes to a newline, `HeaderValue::from_str`
+// rejects it, and the unwrap panicked the connection task — unauthenticated.
+// The override must simply be skipped.
+// ============================================================================
+
+#[tokio::test]
+async fn hostile_response_header_overrides_are_skipped_not_panicked() {
+    let (state, _tmp) = test_state();
+    let uri = "/1,01637037d6";
+
+    let app = build_admin_router(state.clone());
+    let response = app
+        .oneshot(
+            Request::builder()
+                .method("POST")
+                .uri(uri)
+                .body(Body::from(b"payload".to_vec()))
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    assert_eq!(response.status(), StatusCode::CREATED);
+
+    // One request per override param, each carrying a raw newline.
+    for param in [
+        "response-cache-control",
+        "response-content-encoding",
+        "response-expires",
+        "response-content-language",
+        "response-content-disposition",
+        "response-content-type",
+    ] {
+        let app = build_admin_router(state.clone());
+        let response = app
+            .oneshot(
+                Request::builder()
+                    .uri(format!("{}?{}=%0Aevil", uri, param))
+                    .body(Body::empty())
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+        assert_eq!(
+            response.status(),
+            StatusCode::OK,
+            "{} with a newline must be ignored, not panic",
+            param
+        );
+        assert_eq!(body_bytes(response).await, b"payload".to_vec());
+    }
+}
+
+// ============================================================================
+// Non-ASCII in the fid and in ?ttl= must be rejected, not panic
+//
+// `parse_needle_id_cookie` split the hex by BYTE offset and `TTL::read` took
+// the unit as the last BYTE, so a multi-byte character split inside itself.
+// Both are reachable unauthenticated from the request line / query string.
+// ============================================================================
+
+#[tokio::test]
+async fn non_ascii_fid_and_ttl_are_rejected_not_panicked() {
+    let (state, _tmp) = test_state();
+
+    // A fid whose hex part is multi-byte UTF-8.
+    let app = build_admin_router(state.clone());
+    let response = app
+        .oneshot(
+            Request::builder()
+                .uri("/1,%C3%A9%C3%A9%C3%A9%C3%A9a")
+                .body(Body::empty())
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    assert!(
+        response.status().is_client_error() || response.status().is_server_error(),
+        "non-ASCII fid must produce an error status, got {}",
+        response.status()
+    );
+
+    // A TTL whose unit character is multi-byte. The upload path does
+    // `TTL::read(..).ok()`, so *any* unparseable TTL is simply dropped and the
+    // write succeeds — the point here is that a non-ASCII one now takes that
+    // same road instead of panicking. Assert it matches an ASCII-invalid TTL
+    // rather than inventing a stricter contract than the handler has.
+    let mut statuses = Vec::new();
+    // Distinct needle ids: reusing one id with a different cookie is a
+    // cookie-mismatch overwrite, which would mask what this test measures.
+    for (fid, ttl) in [("/1,03637037d7", "5%C3%A9"), ("/1,04637037d8", "5z")] {
+        let app = build_admin_router(state.clone());
+        let response = app
+            .oneshot(
+                Request::builder()
+                    .method("POST")
+                    .uri(format!("{}?ttl={}", fid, ttl))
+                    .body(Body::from(b"x".to_vec()))
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+        statuses.push(response.status());
+    }
+    assert_eq!(
+        statuses[0], statuses[1],
+        "a non-ASCII ttl must behave like any other invalid ttl, not panic"
+    );
+}