fix: empty-body requests without content length (#2888)

Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: OmX <omx@oh-my-codex.dev>

crates/e2e_test/src/delete_object_no_content_length_test.rs

@@ -0,0 +1,165 @@
+// Copyright 2024 RustFS Team
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Regression coverage for signed `DELETE Object?versionId` requests sent with
+//! no body and no `Content-Length` header.
+
+#[cfg(test)]
+mod tests {
+    use crate::common::{RustFSTestEnvironment, init_logging};
+    use aws_sdk_s3::primitives::ByteStream;
+    use aws_sdk_s3::types::{BucketVersioningStatus, VersioningConfiguration};
+    use http::header::HOST;
+    use rustfs_signer::constants::UNSIGNED_PAYLOAD;
+    use rustfs_signer::sign_v4;
+    use s3s::Body;
+    use serial_test::serial;
+    use std::error::Error;
+    use tokio::io::{AsyncReadExt, AsyncWriteExt};
+    use tokio::net::TcpStream;
+    use tokio::time::{Duration, timeout};
+    use tracing::info;
+
+    const RAW_RESPONSE_TIMEOUT: Duration = Duration::from_secs(10);
+
+    fn parse_status(raw_response: &str) -> Option<u16> {
+        raw_response.lines().next()?.split_whitespace().nth(1)?.parse().ok()
+    }
+
+    fn response_body(raw_response: &str) -> &str {
+        raw_response
+            .split_once("\r\n\r\n")
+            .map(|(_, body)| body)
+            .or_else(|| raw_response.split_once("\n\n").map(|(_, body)| body))
+            .unwrap_or("")
+    }
+
+    async fn signed_delete_without_content_length(
+        url: &str,
+        access_key: &str,
+        secret_key: &str,
+    ) -> Result<String, Box<dyn Error + Send + Sync>> {
+        let uri = url.parse::<http::Uri>()?;
+        let authority = uri.authority().ok_or("request URL missing authority")?.to_string();
+        let path_and_query = uri.path_and_query().ok_or("request URL missing path")?.as_str().to_string();
+
+        let request = http::Request::builder()
+            .method(http::Method::DELETE)
+            .uri(uri)
+            .header(HOST, authority.clone())
+            .header("x-amz-content-sha256", UNSIGNED_PAYLOAD)
+            .body(Body::empty())?;
+
+        let signed = sign_v4(request, 0, access_key, secret_key, "", "us-east-1");
+
+        let mut raw_request = format!("DELETE {path_and_query} HTTP/1.1\r\nHost: {authority}\r\nConnection: close\r\n");
+        for (name, value) in signed.headers() {
+            if name.as_str().eq_ignore_ascii_case("host") || name.as_str().eq_ignore_ascii_case("content-length") {
+                continue;
+            }
+            raw_request.push_str(name.as_str());
+            raw_request.push_str(": ");
+            raw_request.push_str(value.to_str()?);
+            raw_request.push_str("\r\n");
+        }
+        raw_request.push_str("\r\n");
+
+        assert!(
+            !raw_request.to_ascii_lowercase().contains("\r\ncontent-length:"),
+            "raw regression request must omit Content-Length; request was:\n{raw_request}"
+        );
+
+        let mut stream = TcpStream::connect(&authority).await?;
+        stream.write_all(raw_request.as_bytes()).await?;
+        stream.flush().await?;
+
+        let mut response = Vec::new();
+        timeout(RAW_RESPONSE_TIMEOUT, stream.read_to_end(&mut response))
+            .await
+            .map_err(|_| std::io::Error::new(std::io::ErrorKind::TimedOut, "timed out reading raw DELETE response"))??;
+        Ok(String::from_utf8_lossy(&response).into_owned())
+    }
+
+    #[tokio::test]
+    #[serial]
+    async fn test_delete_object_version_without_content_length_succeeds() -> Result<(), Box<dyn Error + Send + Sync>> {
+        init_logging();
+        info!("🧪 TEST: signed DELETE Object?versionId succeeds without Content-Length");
+
+        let mut env = RustFSTestEnvironment::new().await?;
+        env.start_rustfs_server(vec![]).await?;
+
+        let client = env.create_s3_client();
+        let bucket = "delete-version-no-content-length";
+        let key = "versioned-delete-target.txt";
+
+        client.create_bucket().bucket(bucket).send().await?;
+
+        client
+            .put_object()
+            .bucket(bucket)
+            .key(key)
+            .body(ByteStream::from_static(b"delete me after versioning is enabled"))
+            .send()
+            .await?;
+
+        client
+            .put_bucket_versioning()
+            .bucket(bucket)
+            .versioning_configuration(
+                VersioningConfiguration::builder()
+                    .status(BucketVersioningStatus::Enabled)
+                    .build(),
+            )
+            .send()
+            .await?;
+
+        let listed_versions = client.list_object_versions().bucket(bucket).prefix(key).send().await?;
+        let version_id = listed_versions
+            .versions()
+            .iter()
+            .find(|version| version.key() == Some(key))
+            .and_then(|version| version.version_id())
+            .ok_or("ListObjectVersions did not return the pre-versioning object version")?;
+
+        let url = format!("{}/{}/{}?versionId={}", env.url, bucket, key, urlencoding::encode(version_id));
+        let raw_response = signed_delete_without_content_length(&url, &env.access_key, &env.secret_key).await?;
+        info!("raw DELETE response:\n{}", raw_response);
+
+        assert_eq!(
+            parse_status(&raw_response),
+            Some(204),
+            "DELETE Object?versionId without Content-Length should succeed, got:\n{raw_response}"
+        );
+        assert!(
+            !raw_response.contains("MissingContentLength"),
+            "DELETE Object?versionId without Content-Length regressed to MissingContentLength: {raw_response}"
+        );
+        assert!(
+            response_body(&raw_response).trim().is_empty(),
+            "successful DELETE Object?versionId should not return an error body: {raw_response}"
+        );
+
+        let get_deleted_version = client
+            .get_object()
+            .bucket(bucket)
+            .key(key)
+            .version_id(version_id)
+            .send()
+            .await;
+        assert!(get_deleted_version.is_err(), "explicitly deleted version should no longer be readable");
+
+        Ok(())
+    }
+}

crates/e2e_test/src/lib.rs

@@ -74,6 +74,10 @@ mod compression_test;
 #[cfg(test)]
 mod delete_objects_versioning_test;
 
+// Regression test for signed DELETE Object?versionId requests without Content-Length.
+#[cfg(test)]
+mod delete_object_no_content_length_test;
+
 // Regression test for Issue #2252: ListObjectVersions misses newest version after put -> delete -> put
 #[cfg(test)]
 mod list_object_versions_regression_test;

rustfs/src/server/http.rs

@@ -22,7 +22,7 @@ use crate::server::{
     compress::{CompressionConfig, PathAwareCompressionPredicate, PathCategoryInjectionLayer},
     hybrid::hybrid,
     layer::{
-        AdminChunkedContentLengthCompatLayer, BodylessStatusFixLayer, ConditionalCorsLayer, HeadRequestBodyFixLayer,
+        BodylessStatusFixLayer, ConditionalCorsLayer, EmptyBodyContentLengthCompatLayer, HeadRequestBodyFixLayer,
         ObjectAttributesEtagFixLayer, PublicHealthEndpointLayer, RedirectLayer, RequestContextLayer, S3ErrorMessageCompatLayer,
     },
     tls_material::{TlsAcceptorHolder, TlsHandshakeFailureKind, TlsMaterialSnapshot, spawn_reload_loop},
@@ -680,7 +680,7 @@ fn process_connection(
         //  3. TrustedProxyLayer                       — conditional, parses X-Forwarded-For
         //  4. SetRequestIdLayer                       — generates X-Request-ID
         //  5. RequestContextLayer                    — creates RequestContext in extensions
-        //  6. AdminChunkedContentLengthCompatLayer    — admin API compat
+        //  6. EmptyBodyContentLengthCompatLayer       — adds Content-Length: 0 for known empty-body API routes
         //  7. CatchPanicLayer                        — panic → 500
         //  8. ReadinessGateLayer                     — blocks until ready
         //  9. KeystoneAuthLayer                      — X-Auth-Token validation
@@ -711,7 +711,7 @@ fn process_connection(
             .option_layer(trusted_proxy_layer)
             .layer(SetRequestIdLayer::x_request_id(MakeRequestUuid))
             .layer(RequestContextLayer)
-            .layer(AdminChunkedContentLengthCompatLayer)
+            .layer(EmptyBodyContentLengthCompatLayer)
             .layer(CatchPanicLayer::new())
             // CRITICAL: Insert ReadinessGateLayer before business logic
             // This stops requests from hitting IAMAuth or Storage if they are not ready.

rustfs/src/server/layer.rs

@@ -220,23 +220,29 @@ where
     }
 }
 
+/// Adds `Content-Length: 0` for routes whose requests are known to carry no
+/// body, but where some S3-compatible clients omit the header entirely.
+///
+/// The normalization runs before authentication so downstream request
+/// validation sees an explicit empty body length without requiring every
+/// handler to special-case absent `Content-Length`.
 #[derive(Clone)]
-pub struct AdminChunkedContentLengthCompatLayer;
+pub struct EmptyBodyContentLengthCompatLayer;
 
-impl<S> Layer<S> for AdminChunkedContentLengthCompatLayer {
-    type Service = AdminChunkedContentLengthCompatService<S>;
+impl<S> Layer<S> for EmptyBodyContentLengthCompatLayer {
+    type Service = EmptyBodyContentLengthCompatService<S>;
 
     fn layer(&self, inner: S) -> Self::Service {
-        AdminChunkedContentLengthCompatService { inner }
+        EmptyBodyContentLengthCompatService { inner }
     }
 }
 
 #[derive(Clone)]
-pub struct AdminChunkedContentLengthCompatService<S> {
+pub struct EmptyBodyContentLengthCompatService<S> {
     inner: S,
 }
 
-impl<S, ReqBody, ResBody> Service<HttpRequest<ReqBody>> for AdminChunkedContentLengthCompatService<S>
+impl<S, ReqBody, ResBody> Service<HttpRequest<ReqBody>> for EmptyBodyContentLengthCompatService<S>
 where
     S: Service<HttpRequest<ReqBody>, Response = Response<ResBody>> + Clone + Send + 'static,
     S::Future: Send + 'static,
@@ -253,7 +259,7 @@ where
     }
 
     fn call(&mut self, mut req: HttpRequest<ReqBody>) -> Self::Future {
-        if should_force_zero_content_length_for_admin_empty_body(&req) {
+        if should_force_zero_content_length_for_empty_body_route(&req) {
             req.headers_mut()
                 .insert(http::header::CONTENT_LENGTH, HeaderValue::from_static("0"));
         }
@@ -263,8 +269,20 @@ where
     }
 }
 
-fn should_force_zero_content_length_for_admin_empty_body<B>(req: &HttpRequest<B>) -> bool {
-    is_empty_body_admin_path(req.method(), req.uri().path()) && !req.headers().contains_key(http::header::CONTENT_LENGTH)
+fn should_force_zero_content_length_for_empty_body_route<B>(req: &HttpRequest<B>) -> bool {
+    if req.headers().contains_key(http::header::CONTENT_LENGTH) {
+        return false;
+    }
+
+    if req.headers().contains_key(http::header::TRANSFER_ENCODING) {
+        return false;
+    }
+
+    if is_empty_body_admin_path(req.method(), req.uri().path()) {
+        return true;
+    }
+
+    is_empty_body_s3_path(req.method(), req.uri())
 }
 
 fn is_empty_body_admin_path(method: &Method, path: &str) -> bool {
@@ -291,6 +309,10 @@ fn is_empty_body_admin_path(method: &Method, path: &str) -> bool {
     }
 }
 
+fn is_empty_body_s3_path(method: &Method, uri: &http::Uri) -> bool {
+    *method == Method::DELETE && ConditionalCorsLayer::is_s3_path(uri.path())
+}
+
 #[derive(Clone)]
 pub struct S3ErrorMessageCompatLayer;
 
@@ -1218,7 +1240,7 @@ mod tests {
             .body(())
             .expect("request");
 
-        assert!(should_force_zero_content_length_for_admin_empty_body(&request));
+        assert!(should_force_zero_content_length_for_empty_body_route(&request));
     }
 
     #[test]
@@ -1238,17 +1260,17 @@ mod tests {
             let request = Request::builder().method(Method::POST).uri(path).body(()).expect("request");
 
             assert!(
-                should_force_zero_content_length_for_admin_empty_body(&request),
+                should_force_zero_content_length_for_empty_body_route(&request),
                 "{path} should force Content-Length: 0"
             );
         }
     }
 
     #[tokio::test]
-    async fn admin_empty_body_post_layer_inserts_zero_content_length() {
+    async fn empty_body_layer_inserts_zero_content_length_for_admin_post() {
         let capture = HeaderCaptureService::default();
         let headers = capture.headers();
-        let mut service = AdminChunkedContentLengthCompatLayer.layer(capture);
+        let mut service = EmptyBodyContentLengthCompatLayer.layer(capture);
         let request = Request::builder()
             .method(Method::POST)
             .uri("/rustfs/admin/v3/rebalance/start")
@@ -1261,6 +1283,88 @@ mod tests {
         assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "0");
     }
 
+    #[tokio::test]
+    async fn empty_body_layer_inserts_zero_content_length_for_admin_put() {
+        let capture = HeaderCaptureService::default();
+        let headers = capture.headers();
+        let mut service = EmptyBodyContentLengthCompatLayer.layer(capture);
+        let request = Request::builder()
+            .method(Method::PUT)
+            .uri("/rustfs/admin/v3/set-group-status?group=test&status=enabled")
+            .body(())
+            .expect("request");
+
+        let _ = service.call(request).await.expect("service call");
+
+        let headers = headers.lock().expect("captured headers").take().expect("captured headers");
+        assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "0");
+    }
+
+    #[tokio::test]
+    async fn empty_body_layer_preserves_admin_transfer_encoding_without_content_length() {
+        let capture = HeaderCaptureService::default();
+        let headers = capture.headers();
+        let mut service = EmptyBodyContentLengthCompatLayer.layer(capture);
+        let request = Request::builder()
+            .method(Method::PUT)
+            .uri("/rustfs/admin/v3/set-group-status?group=test&status=enabled")
+            .header(http::header::TRANSFER_ENCODING, "chunked")
+            .body(())
+            .expect("request");
+
+        let _ = service.call(request).await.expect("service call");
+
+        let headers = headers.lock().expect("captured headers").take().expect("captured headers");
+        assert!(headers.get(http::header::CONTENT_LENGTH).is_none());
+        assert_eq!(headers.get(http::header::TRANSFER_ENCODING).unwrap(), "chunked");
+    }
+
+    #[test]
+    fn s3_delete_object_version_without_content_length_is_normalized() {
+        let request = Request::builder()
+            .method(Method::DELETE)
+            .uri("/bucket/object.txt?versionId=3HL4kqtJlcpXrof3Gj0OmxJnVBH40Nrjfkd")
+            .body(())
+            .expect("request");
+
+        assert!(should_force_zero_content_length_for_empty_body_route(&request));
+    }
+
+    #[tokio::test]
+    async fn empty_body_layer_inserts_zero_content_length_for_s3_delete_object_version() {
+        let capture = HeaderCaptureService::default();
+        let headers = capture.headers();
+        let mut service = EmptyBodyContentLengthCompatLayer.layer(capture);
+        let request = Request::builder()
+            .method(Method::DELETE)
+            .uri("/bucket/object.txt?versionId=3HL4kqtJlcpXrof3Gj0OmxJnVBH40Nrjfkd")
+            .body(())
+            .expect("request");
+
+        let _ = service.call(request).await.expect("service call");
+
+        let headers = headers.lock().expect("captured headers").take().expect("captured headers");
+        assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "0");
+    }
+
+    #[tokio::test]
+    async fn empty_body_layer_preserves_explicit_content_length_header() {
+        let capture = HeaderCaptureService::default();
+        let headers = capture.headers();
+        let mut service = EmptyBodyContentLengthCompatLayer.layer(capture);
+        let request = Request::builder()
+            .method(Method::PUT)
+            .uri("/minio/admin/v3/set-group-status?group=test&status=enabled")
+            .header(http::header::CONTENT_LENGTH, "7")
+            .body(())
+            .expect("request");
+
+        let _ = service.call(request).await.expect("service call");
+
+        let headers = headers.lock().expect("captured headers").take().expect("captured headers");
+        assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "7");
+    }
+
     #[test]
     fn admin_request_with_explicit_content_length_is_left_unchanged() {
         let request = Request::builder()
@@ -1270,18 +1374,75 @@ mod tests {
             .body(())
             .expect("request");
 
-        assert!(!should_force_zero_content_length_for_admin_empty_body(&request));
+        assert!(!should_force_zero_content_length_for_empty_body_route(&request));
     }
 
     #[test]
-    fn non_admin_chunked_put_is_not_normalized() {
+    fn s3_put_object_is_not_normalized() {
         let request = Request::builder()
             .method(Method::PUT)
             .uri("/bucket/object")
             .body(())
             .expect("request");
 
-        assert!(!should_force_zero_content_length_for_admin_empty_body(&request));
+        assert!(!should_force_zero_content_length_for_empty_body_route(&request));
+    }
+
+    #[test]
+    fn s3_delete_bucket_without_content_length_is_normalized() {
+        let request = Request::builder()
+            .method(Method::DELETE)
+            .uri("/bucket")
+            .body(())
+            .expect("request");
+
+        assert!(should_force_zero_content_length_for_empty_body_route(&request));
+    }
+
+    #[test]
+    fn s3_delete_object_without_version_id_is_normalized() {
+        let request = Request::builder()
+            .method(Method::DELETE)
+            .uri("/bucket/object")
+            .body(())
+            .expect("request");
+
+        assert!(should_force_zero_content_length_for_empty_body_route(&request));
+    }
+
+    #[test]
+    fn s3_delete_with_transfer_encoding_is_not_normalized() {
+        let request = Request::builder()
+            .method(Method::DELETE)
+            .uri("/bucket/object?versionId=3HL4kqtJlcpXrof3Gj0OmxJnVBH40Nrjfkd")
+            .header(http::header::TRANSFER_ENCODING, "chunked")
+            .body(())
+            .expect("request");
+
+        assert!(!should_force_zero_content_length_for_empty_body_route(&request));
+    }
+
+    #[test]
+    fn non_s3_delete_paths_are_not_normalized() {
+        let paths = [
+            "/minio/admin/v3/pools/cancel?versionId=unused",
+            "/rustfs/admin/v3/pools/cancel?versionId=unused",
+            "/rustfs/rpc/read_file_stream?versionId=unused",
+            "/rustfs/console/index.html?versionId=unused",
+            "/health?versionId=unused",
+            "/health/ready?versionId=unused",
+            "/profile/cpu?versionId=unused",
+            "/profile/memory?versionId=unused",
+        ];
+
+        for path in paths {
+            let request = Request::builder().method(Method::DELETE).uri(path).body(()).expect("request");
+
+            assert!(
+                !should_force_zero_content_length_for_empty_body_route(&request),
+                "{path} should not force Content-Length: 0"
+            );
+        }
     }
 
     #[test]
@@ -1292,7 +1453,7 @@ mod tests {
             .body(())
             .expect("request");
 
-        assert!(!should_force_zero_content_length_for_admin_empty_body(&request));
+        assert!(!should_force_zero_content_length_for_empty_body_route(&request));
     }
 
     #[test]