From f9b5ad17a94ef60122df45f0371ffe2bedd20b38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sat, 18 Apr 2026 23:27:46 +0800 Subject: [PATCH] fix(s3): ignore empty conditional ETag headers (#2592) Co-authored-by: houseme --- .../src/archive_download_integrity_test.rs | 64 ++++++++++++++++ crates/ecstore/src/set_disk.rs | 14 +++- crates/ecstore/src/set_disk/write.rs | 6 +- crates/ecstore/src/store_api/types.rs | 42 ++++++++++- rustfs/src/storage/ecfs_extend.rs | 25 +++++-- rustfs/src/storage/ecfs_test.rs | 11 +++ rustfs/src/storage/options.rs | 75 ++++++++++++++----- 7 files changed, 205 insertions(+), 32 deletions(-) diff --git a/crates/e2e_test/src/archive_download_integrity_test.rs b/crates/e2e_test/src/archive_download_integrity_test.rs index 832a21559..aad253cb8 100644 --- a/crates/e2e_test/src/archive_download_integrity_test.rs +++ b/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> { + let uri = url.parse::()?; + 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> { + 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() diff --git a/crates/ecstore/src/set_disk.rs b/crates/ecstore/src/set_disk.rs index dcc97a609..9ab1f0828 100644 --- a/crates/ecstore/src/set_disk.rs +++ b/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, if_match: Option) -> 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] diff --git a/crates/ecstore/src/set_disk/write.rs b/crates/ecstore/src/set_disk/write.rs index d937ede3e..7e6254dbd 100644 --- a/crates/ecstore/src/set_disk/write.rs +++ b/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())); } } diff --git a/crates/ecstore/src/store_api/types.rs b/crates/ecstore/src/store_api/types.rs index 4b5b9f87d..096d00304 100644 --- a/crates/ecstore/src/store_api/types.rs +++ b/crates/ecstore/src/store_api/types.rs @@ -33,6 +33,16 @@ pub struct HTTPPreconditions { pub if_unmodified_since: Option, } +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 { diff --git a/rustfs/src/storage/ecfs_extend.rs b/rustfs/src/storage/ecfs_extend.rs index f4853b179..821be3435 100644 --- a/rustfs/src/storage/ecfs_extend.rs +++ b/rustfs/src/storage/ecfs_extend.rs @@ -16,6 +16,7 @@ use crate::config::{RustFSBufferConfig, WorkloadProfile, get_global_buffer_confi use crate::error::ApiError; use crate::server::cors; use crate::storage::ecfs::ListObjectUnorderedQuery; +use http::header::{IF_MATCH, IF_MODIFIED_SINCE, IF_NONE_MATCH, IF_UNMODIFIED_SINCE}; use http::{HeaderMap, HeaderValue, StatusCode}; use metrics::counter; use rustfs_ecstore::bucket::metadata_sys; @@ -384,13 +385,17 @@ pub(crate) async fn validate_bucket_object_lock_enabled(bucket: &str) -> S3Resul pub(crate) fn check_preconditions(headers: &HeaderMap, info: &ObjectInfo) -> S3Result<()> { let mod_time = info.mod_time; let etag = info.etag.as_deref(); + let if_match = non_empty_header_value(headers, IF_MATCH); + let if_none_match = non_empty_header_value(headers, IF_NONE_MATCH); + let if_modified_since = non_empty_header_value(headers, IF_MODIFIED_SINCE); + let if_unmodified_since = non_empty_header_value(headers, IF_UNMODIFIED_SINCE); if mod_time.is_none() && etag.is_none() { return Ok(()); } // If-Match: requires ETag to exist - if let Some(if_match_val) = headers.get("if-match").and_then(|v| v.to_str().ok()) { + if let Some(if_match_val) = if_match { match etag { Some(e) if is_etag_equal(e, if_match_val) => {} _ => return Err(S3Error::new(S3ErrorCode::PreconditionFailed)), @@ -398,9 +403,9 @@ pub(crate) fn check_preconditions(headers: &HeaderMap, info: &ObjectInfo) -> S3R } // If-Unmodified-Since (only when If-Match is absent) - if headers.get("if-match").is_none() + if if_match.is_none() && let Some(t) = mod_time - && let Some(if_unmodified_since) = headers.get("if-unmodified-since").and_then(|v| v.to_str().ok()) + && let Some(if_unmodified_since) = if_unmodified_since && let Ok(given_time) = time::PrimitiveDateTime::parse(if_unmodified_since, &RFC1123).map(|dt| dt.assume_utc()) && t > given_time.add(time::Duration::seconds(1)) { @@ -408,7 +413,7 @@ pub(crate) fn check_preconditions(headers: &HeaderMap, info: &ObjectInfo) -> S3R } // If-None-Match - if let Some(if_none_match) = headers.get("if-none-match").and_then(|v| v.to_str().ok()) + if let Some(if_none_match) = if_none_match && let Some(e) = etag && is_etag_equal(e, if_none_match) { @@ -431,9 +436,9 @@ pub(crate) fn check_preconditions(headers: &HeaderMap, info: &ObjectInfo) -> S3R } // If-Modified-Since (only when If-None-Match is absent — semantics per RFC 7232; dates use RFC 1123 format) - if headers.get("if-none-match").is_none() + if if_none_match.is_none() && let Some(t) = mod_time - && let Some(if_modified_since) = headers.get("if-modified-since").and_then(|v| v.to_str().ok()) + && let Some(if_modified_since) = if_modified_since && let Ok(given_time) = time::PrimitiveDateTime::parse(if_modified_since, &RFC1123).map(|dt| dt.assume_utc()) && t < given_time.add(time::Duration::seconds(1)) { @@ -460,6 +465,14 @@ pub(crate) fn check_preconditions(headers: &HeaderMap, info: &ObjectInfo) -> S3R Ok(()) } +fn non_empty_header_value(headers: &HeaderMap, name: http::header::HeaderName) -> Option<&str> { + headers + .get(name) + .and_then(|v| v.to_str().ok()) + .map(str::trim) + .filter(|v| !v.is_empty()) +} + /// Compares an object ETag with an ETag value from an HTTP header. /// /// This helper implements HTTP ETag comparison semantics for headers such as diff --git a/rustfs/src/storage/ecfs_test.rs b/rustfs/src/storage/ecfs_test.rs index e9c484afb..4b22a8bc6 100644 --- a/rustfs/src/storage/ecfs_test.rs +++ b/rustfs/src/storage/ecfs_test.rs @@ -1055,6 +1055,17 @@ mod tests { ..Default::default() }; assert!(check_preconditions(&headers17, &info17).is_ok()); + + // [18] Empty conditional ETag headers are ignored + let mut headers18 = HeaderMap::new(); + headers18.insert("if-match", HeaderValue::from_static("")); + headers18.insert("if-none-match", HeaderValue::from_static(" ")); + let info18 = ObjectInfo { + mod_time: Some(valid_mod_time), + etag: Some(valid_etag.to_string()), + ..Default::default() + }; + assert!(check_preconditions(&headers18, &info18).is_ok()); } #[test] diff --git a/rustfs/src/storage/options.rs b/rustfs/src/storage/options.rs index 78adef0a3..605808820 100644 --- a/rustfs/src/storage/options.rs +++ b/rustfs/src/storage/options.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use http::header::{IF_MATCH, IF_NONE_MATCH}; use http::{HeaderMap, HeaderValue}; use rustfs_ecstore::bucket::versioning_sys::BucketVersioningSys; use rustfs_ecstore::error::Result; @@ -170,31 +171,41 @@ pub async fn get_opts( } fn fill_conditional_writes_opts_from_header(headers: &HeaderMap, opts: &mut ObjectOptions) -> std::io::Result<()> { - if headers.contains_key("If-None-Match") || headers.contains_key("If-Match") { - let mut preconditions = HTTPPreconditions::default(); - if let Some(if_none_match) = headers.get("If-None-Match") { - preconditions.if_none_match = Some( - if_none_match - .to_str() - .map_err(|_| std::io::Error::other("Invalid If-None-Match header"))? - .to_string(), - ); - } - if let Some(if_match) = headers.get("If-Match") { - preconditions.if_match = Some( - if_match - .to_str() - .map_err(|_| std::io::Error::other("Invalid If-Match header"))? - .to_string(), - ); - } + let if_none_match = conditional_etag_header(headers, IF_NONE_MATCH, "If-None-Match")?; + let if_match = conditional_etag_header(headers, IF_MATCH, "If-Match")?; - opts.http_preconditions = Some(preconditions); + if if_none_match.is_some() || if_match.is_some() { + opts.http_preconditions = Some(HTTPPreconditions { + if_match, + if_none_match, + ..Default::default() + }); } Ok(()) } +fn conditional_etag_header( + headers: &HeaderMap, + name: http::header::HeaderName, + display_name: &str, +) -> std::io::Result> { + let Some(value) = headers.get(name) else { + return Ok(None); + }; + + let value = value + .to_str() + .map_err(|_| std::io::Error::other(format!("Invalid {display_name} header")))? + .trim(); + + if value.is_empty() { + Ok(None) + } else { + Ok(Some(value.to_owned())) + } +} + /// Creates options for putting an object in a bucket. pub async fn put_opts( bucket: &str, @@ -917,6 +928,32 @@ mod tests { assert_eq!(opts.version_id, None); } + #[tokio::test] + async fn test_get_opts_ignores_empty_conditional_headers() { + let mut headers = create_test_headers(); + headers.insert(http::header::IF_MATCH, HeaderValue::from_static("")); + headers.insert(http::header::IF_NONE_MATCH, HeaderValue::from_static(" ")); + + let result = get_opts("test-bucket", "test-object", None, None, &headers).await; + + assert!(result.is_ok()); + assert!(result.unwrap().http_preconditions.is_none()); + } + + #[tokio::test] + async fn test_get_opts_keeps_non_empty_conditional_headers() { + let mut headers = create_test_headers(); + headers.insert(http::header::IF_MATCH, HeaderValue::from_static(" \"etag-a\" ")); + headers.insert(http::header::IF_NONE_MATCH, HeaderValue::from_static("\"etag-b\"")); + + let result = get_opts("test-bucket", "test-object", None, None, &headers).await; + + assert!(result.is_ok()); + let preconditions = result.unwrap().http_preconditions.expect("conditional headers"); + assert_eq!(preconditions.if_match.as_deref(), Some("\"etag-a\"")); + assert_eq!(preconditions.if_none_match.as_deref(), Some("\"etag-b\"")); + } + #[tokio::test] async fn test_get_opts_with_part_number() { let headers = create_test_headers();