fix(s3): ignore empty conditional ETag headers (#2592)

Co-authored-by: houseme <housemecn@gmail.com>

crates/e2e_test/src/archive_download_integrity_test.rs

@@ -218,6 +218,34 @@ mod tests {
         Ok(builder.send().await?)
     }
 
+    async fn signed_get_request_with_headers(
+        url: &str,
+        access_key: &str,
+        secret_key: &str,
+        extra_headers: &[(&str, &str)],
+    ) -> Result<reqwest::Response, Box<dyn Error + Send + Sync>> {
+        let uri = url.parse::<http::Uri>()?;
+        let authority = uri.authority().ok_or("request URL missing authority")?.to_string();
+        let mut request = http::Request::builder()
+            .method(http::Method::GET)
+            .uri(uri)
+            .header(HOST, authority)
+            .header("x-amz-content-sha256", UNSIGNED_PAYLOAD);
+        for (name, value) in extra_headers {
+            request = request.header(*name, *value);
+        }
+
+        let signed = sign_v4(request.body(Body::empty())?, 0, access_key, secret_key, "", "us-east-1");
+
+        let client = local_http_client();
+        let mut builder = client.get(url);
+        for (name, value) in signed.headers() {
+            builder = builder.header(name, value);
+        }
+
+        Ok(builder.send().await?)
+    }
+
     async fn assert_archive_object_content_encoding(
         client: &S3Client,
         bucket: &str,
@@ -655,6 +683,42 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    #[serial]
+    async fn test_multipart_get_ignores_empty_conditional_etag_headers() -> Result<(), Box<dyn Error + Send + Sync>> {
+        init_logging();
+        let mut env = RustFSTestEnvironment::new().await?;
+        env.start_rustfs_server(vec![]).await?;
+        env.create_test_bucket(MULTIPART_ARCHIVE_TEST_BUCKET).await?;
+
+        let client = env.create_s3_client();
+        let key = "multipart-empty-conditional-headers.zip";
+        let zip_bytes =
+            complete_archive_multipart_upload_with_content_encoding(&client, MULTIPART_ARCHIVE_TEST_BUCKET, key, None).await?;
+        let object_url = format!("{}/{}/{}", env.url, MULTIPART_ARCHIVE_TEST_BUCKET, key);
+
+        let response = signed_get_request_with_headers(
+            &object_url,
+            &env.access_key,
+            &env.secret_key,
+            &[("if-match", ""), ("if-none-match", "")],
+        )
+        .await?;
+        let status = response.status();
+        let body = response.bytes().await?;
+
+        assert_eq!(
+            status,
+            StatusCode::OK,
+            "unexpected multipart GET status {status}, body: {}",
+            String::from_utf8_lossy(body.as_ref())
+        );
+        assert_eq!(body.as_ref(), zip_bytes.as_slice());
+
+        env.stop_server();
+        Ok(())
+    }
+
     #[tokio::test]
     #[serial]
     async fn test_archive_multipart_with_aws_chunked_and_effective_encoding_roundtrips_by_default()

crates/ecstore/src/set_disk.rs

@@ -4296,15 +4296,21 @@ pub fn e_tag_matches(etag: &str, condition: &str) -> bool {
 }
 
 pub fn should_prevent_write(oi: &ObjectInfo, if_none_match: Option<String>, if_match: Option<String>) -> bool {
+    let if_none_match = if_none_match
+        .as_deref()
+        .map(str::trim)
+        .filter(|condition| !condition.is_empty());
+    let if_match = if_match.as_deref().map(str::trim).filter(|condition| !condition.is_empty());
+
     match &oi.etag {
         Some(etag) => {
             if let Some(if_none_match) = if_none_match
-                && e_tag_matches(etag, &if_none_match)
+                && e_tag_matches(etag, if_none_match)
             {
                 return true;
             }
             if let Some(if_match) = if_match
-                && !e_tag_matches(etag, &if_match)
+                && !e_tag_matches(etag, if_match)
             {
                 return true;
             }
@@ -5514,6 +5520,10 @@ mod tests {
         let if_none_match = None;
         let if_match = None;
         assert!(!should_prevent_write(&oi, if_none_match, if_match));
+
+        let if_none_match = Some(String::new());
+        let if_match = Some(" ".to_string());
+        assert!(!should_prevent_write(&oi, if_none_match, if_match));
     }
 
     #[test]

crates/ecstore/src/set_disk/write.rs

@@ -603,7 +603,9 @@ impl SetDisks {
                 if oi.delete_marker {
                     return None;
                 }
-                if should_prevent_write(&oi, http_preconditions.if_none_match, http_preconditions.if_match) {
+                let if_none_match = http_preconditions.if_none_match_value().map(str::to_owned);
+                let if_match = http_preconditions.if_match_value().map(str::to_owned);
+                if should_prevent_write(&oi, if_none_match, if_match) {
                     return Some(StorageError::PreconditionFailed);
                 }
             }
@@ -614,7 +616,7 @@ impl SetDisks {
                 // When the object is not found,
                 // - if If-Match is set, we should return 404 NotFound
                 // - if If-None-Match is set, we should be able to proceed with the request
-                if http_preconditions.if_match.is_some() {
+                if http_preconditions.if_match_value().is_some() {
                     return Some(StorageError::ObjectNotFound(bucket.to_string(), object.to_string()));
                 }
             }

crates/ecstore/src/store_api/types.rs

@@ -33,6 +33,16 @@ pub struct HTTPPreconditions {
     pub if_unmodified_since: Option<OffsetDateTime>,
 }
 
+impl HTTPPreconditions {
+    pub(crate) fn if_match_value(&self) -> Option<&str> {
+        non_empty_condition_value(self.if_match.as_deref())
+    }
+
+    pub(crate) fn if_none_match_value(&self) -> Option<&str> {
+        non_empty_condition_value(self.if_none_match.as_deref())
+    }
+}
+
 #[derive(Debug, Default, Clone)]
 pub struct ObjectOptions {
     // Use the maximum parity (N/2), used when saving server configuration files
@@ -146,7 +156,10 @@ impl ObjectOptions {
         }
 
         if let Some(pre) = &self.http_preconditions {
-            if let Some(if_none_match) = &pre.if_none_match
+            let if_none_match = pre.if_none_match_value();
+            let if_match = pre.if_match_value();
+
+            if let Some(if_none_match) = if_none_match
                 && let Some(etag) = &obj_info.etag
                 && is_etag_equal(etag, if_none_match)
             {
@@ -161,7 +174,7 @@ impl ObjectOptions {
                 return Err(Error::NotModified);
             }
 
-            if let Some(if_match) = &pre.if_match {
+            if let Some(if_match) = if_match {
                 if let Some(etag) = &obj_info.etag {
                     if !is_etag_equal(etag, if_match) {
                         return Err(Error::PreconditionFailed);
@@ -171,7 +184,7 @@ impl ObjectOptions {
                 }
             }
             if has_valid_mod_time
-                && pre.if_match.is_none()
+                && if_match.is_none()
                 && let Some(if_unmodified_since) = &pre.if_unmodified_since
                 && let Some(mod_time) = &obj_info.mod_time
                 && is_modified_since(mod_time, if_unmodified_since)
@@ -184,6 +197,10 @@ impl ObjectOptions {
     }
 }
 
+fn non_empty_condition_value(value: Option<&str>) -> Option<&str> {
+    value.map(str::trim).filter(|value| !value.is_empty())
+}
+
 fn is_etag_equal(etag1: &str, etag2: &str) -> bool {
     let e1 = etag1.trim_matches('"');
     let e2 = etag2.trim_matches('"');
@@ -1066,6 +1083,25 @@ mod tests {
         assert_eq!(info.get_actual_size().unwrap(), 77);
     }
 
+    #[test]
+    fn precondition_check_ignores_empty_etag_conditions() {
+        let opts = ObjectOptions {
+            http_preconditions: Some(HTTPPreconditions {
+                if_match: Some(String::new()),
+                if_none_match: Some(" ".to_string()),
+                ..Default::default()
+            }),
+            ..Default::default()
+        };
+        let info = ObjectInfo {
+            mod_time: Some(OffsetDateTime::now_utc()),
+            etag: Some("\"abc\"".to_string()),
+            ..Default::default()
+        };
+
+        assert!(opts.precondition_check(&info).is_ok());
+    }
+
     #[test]
     fn from_file_info_preserves_replication_decision() {
         let fi = rustfs_filemeta::FileInfo {