From c9c2428373b825a6352f0de353241942eedea496 Mon Sep 17 00:00:00 2001 From: houseme Date: Tue, 7 Apr 2026 16:25:34 +0800 Subject: [PATCH] fix(download): preserve archive download integrity (#2415) Co-authored-by: heihutu Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: houseme <4829346+houseme@users.noreply.github.com> --- Cargo.lock | 1 + crates/e2e_test/Cargo.toml | 1 + .../src/archive_download_integrity_test.rs | 329 ++++++++++++++++++ crates/e2e_test/src/lib.rs | 3 + crates/ecstore/src/compress.rs | 10 + crates/object-io/src/get.rs | 56 +++ rustfs/src/app/multipart_usecase.rs | 8 +- rustfs/src/app/object_usecase.rs | 15 + rustfs/src/app/object_usecase/app_adapters.rs | 4 + rustfs/src/server/compress.rs | 96 ++++- rustfs/src/server/http.rs | 99 +++++- rustfs/src/storage/options.rs | 95 ++++- 12 files changed, 699 insertions(+), 18 deletions(-) create mode 100644 crates/e2e_test/src/archive_download_integrity_test.rs diff --git a/Cargo.lock b/Cargo.lock index ffcd09ac1..3b52ae633 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3235,6 +3235,7 @@ dependencies = [ "urlencoding", "uuid", "walkdir", + "zip", "zstd", ] diff --git a/crates/e2e_test/Cargo.toml b/crates/e2e_test/Cargo.toml index cea060a54..1546d2f3d 100644 --- a/crates/e2e_test/Cargo.toml +++ b/crates/e2e_test/Cargo.toml @@ -80,3 +80,4 @@ suppaftp = { workspace = true, features = ["tokio", "rustls-aws-lc-rs"] } rcgen.workspace = true anyhow.workspace = true rustls.workspace = true +zip.workspace = true diff --git a/crates/e2e_test/src/archive_download_integrity_test.rs b/crates/e2e_test/src/archive_download_integrity_test.rs new file mode 100644 index 000000000..7bbc1d6dd --- /dev/null +++ b/crates/e2e_test/src/archive_download_integrity_test.rs @@ -0,0 +1,329 @@ +// 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. + +#[cfg(test)] +mod tests { + use crate::common::{RustFSTestEnvironment, init_logging, rustfs_binary_path}; + use aws_sdk_s3::primitives::ByteStream; + use aws_sdk_s3::types::{CompletedMultipartUpload, CompletedPart}; + use http::header::{CONTENT_TYPE, HOST}; + use reqwest::StatusCode; + use rustfs_signer::constants::UNSIGNED_PAYLOAD; + use rustfs_signer::{pre_sign_v4, sign_v4}; + use s3s::Body; + use serial_test::serial; + use sha2::{Digest, Sha256}; + use std::error::Error; + use std::io::{Cursor, Write}; + use std::process::Command; + use time::OffsetDateTime; + use zip::{CompressionMethod, ZipWriter, write::SimpleFileOptions}; + + const ARCHIVE_TEST_BUCKET: &str = "archive-download-integrity"; + const MULTIPART_ARCHIVE_TEST_BUCKET: &str = "archive-multipart-integrity"; + const MULTIPART_PART_SIZE: usize = 5 * 1024 * 1024; + + fn build_zip_bytes(files: &[(&str, &[u8])]) -> Result, Box> { + let cursor = Cursor::new(Vec::new()); + let mut zip = ZipWriter::new(cursor); + let options = SimpleFileOptions::default().compression_method(CompressionMethod::Stored); + + for (name, content) in files { + zip.start_file(*name, options)?; + zip.write_all(content)?; + } + + Ok(zip.finish()?.into_inner()) + } + + fn random_bytes(size: usize) -> Vec { + (0..size).map(|idx| (idx % 251) as u8).collect() + } + + async fn start_rustfs_server_with_env( + env: &mut RustFSTestEnvironment, + extra_env: &[(&str, &str)], + ) -> Result<(), Box> { + let binary_path = rustfs_binary_path(); + let mut command = Command::new(&binary_path); + command.env("RUST_LOG", "rustfs=info,rustfs_notify=debug"); + for (key, value) in extra_env { + command.env(key, value); + } + + let process = command + .args([ + "--address", + &env.address, + "--access-key", + &env.access_key, + "--secret-key", + &env.secret_key, + &env.temp_dir, + ]) + .spawn()?; + + env.process = Some(process); + env.wait_for_server_ready().await?; + Ok(()) + } + + async fn presigned_get_request_with_accept_encoding( + url: &str, + access_key: &str, + secret_key: &str, + accept_encoding: &str, + ) -> Result> { + let uri = url.parse::()?; + let authority = uri.authority().ok_or("request URL missing authority")?.to_string(); + let signed = pre_sign_v4( + http::Request::builder() + .method(http::Method::GET) + .uri(uri) + .header(HOST, authority) + .body(Body::empty())?, + access_key, + secret_key, + "", + "us-east-1", + 600, + OffsetDateTime::now_utc(), + ); + + let client = reqwest::Client::builder() + .no_proxy() + .no_gzip() + .no_brotli() + .no_zstd() + .no_deflate() + .build()?; + + Ok(client + .get(signed.uri().to_string()) + .header("Accept-Encoding", accept_encoding) + .send() + .await?) + } + + async fn signed_put_request_with_headers( + url: &str, + access_key: &str, + secret_key: &str, + body: Vec, + content_type: &str, + content_encoding: &str, + ) -> Result> { + let uri = url.parse::()?; + let authority = uri.authority().ok_or("request URL missing authority")?.to_string(); + let request = http::Request::builder() + .method(http::Method::PUT) + .uri(uri) + .header(HOST, authority) + .header(CONTENT_TYPE, content_type) + .header("content-encoding", content_encoding) + .header("x-amz-content-sha256", UNSIGNED_PAYLOAD) + .body(Body::empty())?; + let signed = sign_v4(request, body.len() as i64, access_key, secret_key, "", "us-east-1"); + + let client = reqwest::Client::builder().no_proxy().build()?; + let mut builder = client.put(url).body(body); + for (name, value) in signed.headers() { + builder = builder.header(name, value); + } + + Ok(builder.send().await?) + } + + #[tokio::test] + #[serial] + async fn test_archive_put_rejects_content_encoding_by_default() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server_without_cleanup(vec![]).await?; + env.create_test_bucket(ARCHIVE_TEST_BUCKET).await?; + let zip_bytes = build_zip_bytes(&[("alpha.txt", b"archive-body")])?; + let object_url = format!("{}/{}/{}", env.url, ARCHIVE_TEST_BUCKET, "bundle.zip"); + let response = + signed_put_request_with_headers(&object_url, &env.access_key, &env.secret_key, zip_bytes, "application/zip", "gzip") + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + let body = response.text().await?; + assert!( + body.contains("InvalidArgument") || body.contains("Content-Encoding"), + "unexpected error body: {body}" + ); + + env.stop_server(); + Ok(()) + } + + #[tokio::test] + #[serial] + async fn test_archive_download_roundtrip_with_http_compression_enabled() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + start_rustfs_server_with_env( + &mut env, + &[ + ("RUSTFS_COMPRESS_ENABLE", "on"), + ("RUSTFS_COMPRESS_MIME_TYPES", "text/*,application/json,application/zip"), + ("RUSTFS_COMPRESS_MIN_SIZE", "1"), + ], + ) + .await?; + env.create_test_bucket(ARCHIVE_TEST_BUCKET).await?; + + let client = env.create_s3_client(); + let zip_bytes = build_zip_bytes(&[ + ("docs/readme.txt", b"archive-download-integrity"), + ("docs/notes.txt", b"response-compression-must-not-alter-zip-bytes"), + ])?; + let expected_sha256 = Sha256::digest(&zip_bytes); + + client + .put_object() + .bucket(ARCHIVE_TEST_BUCKET) + .key("bundle.zip") + .content_type("application/zip") + .body(ByteStream::from(zip_bytes.clone())) + .send() + .await?; + + let object_url = format!("{}/{}/{}", env.url, ARCHIVE_TEST_BUCKET, "bundle.zip"); + let response = + presigned_get_request_with_accept_encoding(&object_url, &env.access_key, &env.secret_key, "gzip, br, zstd").await?; + + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response + .headers() + .get("content-encoding") + .and_then(|value| value.to_str().ok()), + None, + "archive download must not be HTTP-compressed" + ); + + let downloaded = response.bytes().await?; + assert_eq!( + downloaded.as_ref(), + zip_bytes.as_slice(), + "downloaded archive bytes must match uploaded bytes" + ); + assert_eq!( + Sha256::digest(downloaded.as_ref()).as_slice(), + expected_sha256.as_slice(), + "archive SHA256 mismatch" + ); + + env.stop_server(); + Ok(()) + } + + #[tokio::test] + #[serial] + async fn test_archive_multipart_roundtrip_preserves_bytes() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server_without_cleanup(vec![]).await?; + env.create_test_bucket(MULTIPART_ARCHIVE_TEST_BUCKET).await?; + + let client = env.create_s3_client(); + let payload = random_bytes(MULTIPART_PART_SIZE + 512 * 1024); + let zip_bytes = build_zip_bytes(&[("payload.bin", payload.as_slice())])?; + assert!(zip_bytes.len() > MULTIPART_PART_SIZE, "zip payload must exceed multipart threshold"); + let expected_sha256 = Sha256::digest(&zip_bytes); + + let create_output = client + .create_multipart_upload() + .bucket(MULTIPART_ARCHIVE_TEST_BUCKET) + .key("multipart-bundle.zip") + .content_type("application/zip") + .send() + .await?; + let upload_id = create_output.upload_id().expect("multipart upload id"); + + let first_part = zip_bytes[..MULTIPART_PART_SIZE].to_vec(); + let second_part = zip_bytes[MULTIPART_PART_SIZE..].to_vec(); + + let upload_part_1 = client + .upload_part() + .bucket(MULTIPART_ARCHIVE_TEST_BUCKET) + .key("multipart-bundle.zip") + .upload_id(upload_id) + .part_number(1) + .body(ByteStream::from(first_part)) + .send() + .await?; + + let upload_part_2 = client + .upload_part() + .bucket(MULTIPART_ARCHIVE_TEST_BUCKET) + .key("multipart-bundle.zip") + .upload_id(upload_id) + .part_number(2) + .body(ByteStream::from(second_part)) + .send() + .await?; + + let completed_upload = CompletedMultipartUpload::builder() + .parts( + CompletedPart::builder() + .part_number(1) + .e_tag(upload_part_1.e_tag().unwrap_or_default()) + .build(), + ) + .parts( + CompletedPart::builder() + .part_number(2) + .e_tag(upload_part_2.e_tag().unwrap_or_default()) + .build(), + ) + .build(); + + client + .complete_multipart_upload() + .bucket(MULTIPART_ARCHIVE_TEST_BUCKET) + .key("multipart-bundle.zip") + .upload_id(upload_id) + .multipart_upload(completed_upload) + .send() + .await?; + + let downloaded = client + .get_object() + .bucket(MULTIPART_ARCHIVE_TEST_BUCKET) + .key("multipart-bundle.zip") + .send() + .await? + .body + .collect() + .await? + .into_bytes(); + + assert_eq!( + downloaded.as_ref(), + zip_bytes.as_slice(), + "multipart archive bytes must match uploaded bytes" + ); + assert_eq!( + Sha256::digest(downloaded.as_ref()).as_slice(), + expected_sha256.as_slice(), + "multipart archive SHA256 mismatch" + ); + + env.stop_server(); + Ok(()) + } +} diff --git a/crates/e2e_test/src/lib.rs b/crates/e2e_test/src/lib.rs index 37b73d888..458c453ba 100644 --- a/crates/e2e_test/src/lib.rs +++ b/crates/e2e_test/src/lib.rs @@ -56,6 +56,9 @@ mod special_chars_test; #[cfg(test)] mod content_encoding_test; +#[cfg(test)] +mod archive_download_integrity_test; + // ListObjectsV2 pagination test (Issue #1596) #[cfg(test)] mod list_objects_v2_pagination_test; diff --git a/crates/ecstore/src/compress.rs b/crates/ecstore/src/compress.rs index 0886c84be..99c91b7fd 100644 --- a/crates/ecstore/src/compress.rs +++ b/crates/ecstore/src/compress.rs @@ -82,16 +82,26 @@ pub const STANDARD_EXCLUDE_COMPRESS_CONTENT_TYPES: &[&str] = &[ "video/*", "audio/*", "image/*", + // Archive formats (compressed) "application/zip", + "application/gzip", "application/x-gzip", "application/x-zip-compressed", "application/x-compress", "application/x-spoon", "application/x-rar-compressed", "application/x-7z-compressed", + "application/x-bzip", "application/x-bzip2", "application/x-xz", + "application/x-lzip", + "application/x-lzma", + "application/x-lzop", "application/zstd", + "application/x-zstd", + // Archive formats (uncompressed containers that are typically not further compressible) + "application/x-tar", + "application/tar", "application/pdf", "application/wasm", "font/*", diff --git a/crates/object-io/src/get.rs b/crates/object-io/src/get.rs index 90e49b992..f59aa1248 100644 --- a/crates/object-io/src/get.rs +++ b/crates/object-io/src/get.rs @@ -170,6 +170,7 @@ pub trait CachedGetObjectSource { fn content_type(&self) -> Option<&str>; fn e_tag(&self) -> Option<&str>; fn last_modified(&self) -> Option<&str>; + fn expires(&self) -> Option<&str>; fn cache_control(&self) -> Option<&str>; fn content_disposition(&self) -> Option<&str>; fn content_encoding(&self) -> Option<&str>; @@ -405,6 +406,10 @@ where .last_modified() .and_then(|s| OffsetDateTime::parse(s, &Rfc3339).ok()) .map(Timestamp::from); + let expires = cached + .expires() + .and_then(|s| OffsetDateTime::parse(s, &Rfc3339).ok()) + .map(Timestamp::from); let content_type = cached.content_type().and_then(|ct| ContentType::from_str(ct).ok()); @@ -416,6 +421,7 @@ where accept_ranges: Some("bytes".to_string()), e_tag: cached.e_tag().map(to_s3s_etag), last_modified, + expires, content_type, cache_control: cached.cache_control().map(str::to_string), content_disposition: cached.content_disposition().map(str::to_string), @@ -890,8 +896,12 @@ pub fn build_get_object_output( body, content_length: Some(response_content_length), last_modified, + expires: info.expires.map(Timestamp::from), content_type, + cache_control: info.user_defined.get(CACHE_CONTROL.as_str()).cloned(), + content_disposition: info.user_defined.get(CONTENT_DISPOSITION.as_str()).cloned(), content_encoding: info.content_encoding.clone(), + content_language: info.user_defined.get(CONTENT_LANGUAGE.as_str()).cloned(), accept_ranges: Some("bytes".to_string()), content_range, e_tag: info.etag.as_ref().map(|etag| to_s3s_etag(etag)), @@ -1052,6 +1062,7 @@ mod tests { content_type: Option, e_tag: Option, last_modified: Option, + expires: Option, cache_control: Option, content_disposition: Option, content_encoding: Option, @@ -1090,6 +1101,10 @@ mod tests { self.last_modified.as_deref() } + fn expires(&self) -> Option<&str> { + self.expires.as_deref() + } + fn cache_control(&self) -> Option<&str> { self.cache_control.as_deref() } @@ -1624,6 +1639,7 @@ mod tests { content_type: None, e_tag: None, last_modified: None, + expires: None, cache_control: None, content_disposition: None, content_encoding: None, @@ -1651,6 +1667,46 @@ mod tests { assert_eq!(result.output.checksum_type, Some(ChecksumType::from_static(ChecksumType::FULL_OBJECT))); } + #[test] + fn build_get_object_output_preserves_http_metadata_like_cached_path() { + let mut info = ObjectInfo { + content_type: Some("application/octet-stream".to_string()), + content_encoding: Some("zstd".to_string()), + etag: Some("etag".to_string()), + expires: Some(OffsetDateTime::UNIX_EPOCH), + ..Default::default() + }; + info.user_defined + .insert("cache-control".to_string(), "max-age=3600".to_string()); + info.user_defined + .insert("content-disposition".to_string(), "attachment; filename=\"bundle.zip\"".to_string()); + info.user_defined.insert("content-language".to_string(), "en-US".to_string()); + + let output = build_get_object_output( + None, + &info, + info.content_type + .as_ref() + .and_then(|content_type| ContentType::from_str(content_type).ok()), + info.mod_time.map(Timestamp::from), + 8, + None, + None, + None, + None, + None, + &GetObjectChecksums::default(), + None, + None, + ); + + assert_eq!(output.cache_control.as_deref(), Some("max-age=3600")); + assert_eq!(output.content_disposition.as_deref(), Some("attachment; filename=\"bundle.zip\"")); + assert_eq!(output.content_language.as_deref(), Some("en-US")); + assert_eq!(output.content_encoding.as_deref(), Some("zstd")); + assert_eq!(output.expires, Some(Timestamp::from(OffsetDateTime::UNIX_EPOCH))); + } + #[test] fn build_cors_wrapped_get_object_flow_result_uses_wrapped_mode() { let result = build_cors_wrapped_get_object_flow_result( diff --git a/rustfs/src/app/multipart_usecase.rs b/rustfs/src/app/multipart_usecase.rs index b370fc89e..7af30a263 100644 --- a/rustfs/src/app/multipart_usecase.rs +++ b/rustfs/src/app/multipart_usecase.rs @@ -23,7 +23,7 @@ use crate::storage::entity; use crate::storage::helper::OperationHelper; use crate::storage::options::{ copy_src_opts, extract_metadata, get_complete_multipart_upload_opts, get_content_sha256_with_query, get_opts, - parse_copy_source_range, put_opts, + parse_copy_source_range, put_opts, validate_archive_content_encoding, }; use crate::storage::request_context::spawn_traced; use crate::storage::s3_api::multipart::build_list_parts_output; @@ -559,6 +559,12 @@ impl DefaultMultipartUsecase { return Err(S3Error::with_message(S3ErrorCode::InternalError, "Not init".to_string())); }; + validate_archive_content_encoding( + &key, + req.headers.get("content-type").and_then(|value| value.to_str().ok()), + req.headers.get("content-encoding").and_then(|value| value.to_str().ok()), + )?; + let mut metadata = extract_metadata(&req.headers); if let Some(tags) = tagging { diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index c94347b5f..6f57f5507 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -38,6 +38,7 @@ use crate::storage::helper::OperationHelper; use crate::storage::options::{ copy_dst_opts, copy_src_opts, del_opts, extract_metadata, extract_metadata_from_mime_with_object_name, filter_object_metadata, get_content_sha256_with_query, get_opts, normalize_content_encoding_for_storage, put_opts, + validate_archive_content_encoding, }; use crate::storage::s3_api::multipart::parse_list_parts_params; use crate::storage::s3_api::{acl, restore, select}; @@ -315,6 +316,20 @@ fn apply_put_request_metadata( tagging: Option, storage_class: Option, ) -> S3Result<()> { + let request_content_type = content_type.as_ref().map(ToString::to_string).or_else(|| { + headers + .get("content-type") + .and_then(|value| value.to_str().ok()) + .map(ToOwned::to_owned) + }); + let request_content_encoding = content_encoding.as_ref().map(ToString::to_string).or_else(|| { + headers + .get("content-encoding") + .and_then(|value| value.to_str().ok()) + .map(ToOwned::to_owned) + }); + validate_archive_content_encoding(object_name, request_content_type.as_deref(), request_content_encoding.as_deref())?; + if let Some(cache_control) = cache_control { metadata.insert("cache-control".to_string(), cache_control.to_string()); } diff --git a/rustfs/src/app/object_usecase/app_adapters.rs b/rustfs/src/app/object_usecase/app_adapters.rs index d424764fb..667aa9ce9 100644 --- a/rustfs/src/app/object_usecase/app_adapters.rs +++ b/rustfs/src/app/object_usecase/app_adapters.rs @@ -104,6 +104,10 @@ impl ObjectIoCachedGetObjectSource for CachedGetObject { self.last_modified.as_deref() } + fn expires(&self) -> Option<&str> { + self.expires.as_deref() + } + fn cache_control(&self) -> Option<&str> { self.cache_control.as_deref() } diff --git a/rustfs/src/server/compress.rs b/rustfs/src/server/compress.rs index 09844ece3..c6e72cced 100644 --- a/rustfs/src/server/compress.rs +++ b/rustfs/src/server/compress.rs @@ -46,6 +46,8 @@ use rustfs_config::{ DEFAULT_COMPRESS_ENABLE, DEFAULT_COMPRESS_EXTENSIONS, DEFAULT_COMPRESS_MIME_TYPES, DEFAULT_COMPRESS_MIN_SIZE, ENV_COMPRESS_ENABLE, ENV_COMPRESS_EXTENSIONS, ENV_COMPRESS_MIME_TYPES, ENV_COMPRESS_MIN_SIZE, EnableState, }; +use rustfs_ecstore::compress::{STANDARD_EXCLUDE_COMPRESS_CONTENT_TYPES, STANDARD_EXCLUDE_COMPRESS_EXTENSIONS}; +use rustfs_utils::string::{has_pattern, has_string_suffix_in_slice}; use std::str::FromStr; use tower_http::compression::predicate::Predicate; use tracing::debug; @@ -196,6 +198,20 @@ impl CompressionConfig { None } + + pub(crate) fn is_excluded_filename(filename: &str) -> bool { + has_string_suffix_in_slice(&filename.to_ascii_lowercase(), STANDARD_EXCLUDE_COMPRESS_EXTENSIONS) + } + + pub(crate) fn is_excluded_mime_type(content_type: &str) -> bool { + let main_type = content_type + .split(';') + .next() + .unwrap_or(content_type) + .trim() + .to_ascii_lowercase(); + !main_type.is_empty() && has_pattern(STANDARD_EXCLUDE_COMPRESS_CONTENT_TYPES, &main_type) + } } impl Default for CompressionConfig { @@ -299,23 +315,37 @@ impl Predicate for CompressionPredicate { return false; } - // Check if the response matches configured extension via Content-Disposition + // Hard-stop archive/media/package MIME types even if the whitelist matches. + // This includes tar, gzip, bzip2, xz, zstd, zip, rar, 7z, lzip, lzma, lzop variants, + // plus video/*, audio/*, image/*, font/*, application/pdf, and application/wasm. + if let Some(content_type) = response.headers().get(http::header::CONTENT_TYPE) + && let Ok(ct) = content_type.to_str() + { + if CompressionConfig::is_excluded_mime_type(ct) { + debug!("Skipping compression for excluded Content-Type '{}'", ct); + return false; + } + + if self.config.matches_mime_type(ct) { + debug!("Compressing response: Content-Type '{}' matches configured MIME pattern", ct); + return true; + } + } + + // Hard-stop archive-like attachment downloads even if the whitelist matches. if let Some(content_disposition) = response.headers().get(http::header::CONTENT_DISPOSITION) && let Ok(cd) = content_disposition.to_str() && let Some(filename) = CompressionConfig::extract_filename_from_content_disposition(cd) - && self.config.matches_extension(&filename) { - debug!("Compressing response: filename '{}' matches configured extension", filename); - return true; - } + if CompressionConfig::is_excluded_filename(&filename) { + debug!("Skipping compression for excluded filename '{}'", filename); + return false; + } - // Check if the response matches configured MIME type - if let Some(content_type) = response.headers().get(http::header::CONTENT_TYPE) - && let Ok(ct) = content_type.to_str() - && self.config.matches_mime_type(ct) - { - debug!("Compressing response: Content-Type '{}' matches configured MIME pattern", ct); - return true; + if self.config.matches_extension(&filename) { + debug!("Compressing response: filename '{}' matches configured extension", filename); + return true; + } } // Default: don't compress (whitelist approach) @@ -405,7 +435,11 @@ use std::task::{Context, Poll}; use tower::{Layer, Service}; /// Tower layer that injects `RequestPathCategory` into each response's extensions -/// based on the incoming request URI path. Must be placed before `CompressionLayer`. +/// based on the incoming request URI path. +/// +/// It must be placed inside `CompressionLayer` so the category is available when +/// the outer compression middleware evaluates its response predicate. With +/// `tower::ServiceBuilder`, that means adding this layer after `CompressionLayer`. #[derive(Clone, Copy, Debug)] pub(crate) struct PathCategoryInjectionLayer; @@ -627,6 +661,42 @@ mod tests { assert_eq!(predicate.config.min_size, 1000); } + #[test] + fn test_compression_predicate_skips_archive_mime_type_even_when_whitelisted() { + let predicate = CompressionPredicate::new(CompressionConfig { + enabled: true, + extensions: vec![], + mime_patterns: vec!["application/zip".to_string()], + min_size: 0, + }); + + let response = Response::builder() + .header(http::header::CONTENT_TYPE, "application/zip") + .header(http::header::CONTENT_LENGTH, "4096") + .body(http_body_util::Empty::::new()) + .expect("response"); + + assert!(!predicate.should_compress(&response)); + } + + #[test] + fn test_compression_predicate_skips_archive_filename_even_when_whitelisted() { + let predicate = CompressionPredicate::new(CompressionConfig { + enabled: true, + extensions: vec![".zip".to_string()], + mime_patterns: vec![], + min_size: 0, + }); + + let response = Response::builder() + .header(http::header::CONTENT_DISPOSITION, r#"attachment; filename="bundle.zip""#) + .header(http::header::CONTENT_LENGTH, "4096") + .body(http_body_util::Empty::::new()) + .expect("response"); + + assert!(!predicate.should_compress(&response)); + } + #[test] fn test_path_category_classify_s3() { assert_eq!(PathCategory::classify("/"), PathCategory::S3DataPlane); diff --git a/rustfs/src/server/http.rs b/rustfs/src/server/http.rs index 90b940e86..6e0243d9a 100644 --- a/rustfs/src/server/http.rs +++ b/rustfs/src/server/http.rs @@ -603,8 +603,8 @@ fn process_connection( // 9. KeystoneAuthLayer — X-Auth-Token validation // 10. TraceLayer — request/response tracing + metrics // 11. PropagateRequestIdLayer — X-Request-ID → response - // 12. PathCategoryInjectionLayer — injects path category for compression - // 13. CompressionLayer — response compression (whitelist, path-aware) + // 12. CompressionLayer — response compression (whitelist, path-aware) + // 13. PathCategoryInjectionLayer — injects path category for compression predicate // 14. ObjectAttributesEtagFixLayer — ETag fix for GetObjectAttributes // 15. ConditionalCorsLayer — S3 API CORS // 16. RedirectLayer — console redirect (conditional) @@ -739,8 +739,8 @@ fn process_connection( .layer(PropagateRequestIdLayer::x_request_id()) // Compress responses based on whitelist configuration // Only compresses when enabled and matches configured extensions/MIME types - .layer(PathCategoryInjectionLayer) .layer(CompressionLayer::new().compress_when(PathAwareCompressionPredicate::new(compression_config))) + .layer(PathCategoryInjectionLayer) .layer(ObjectAttributesEtagFixLayer) // Conditional CORS layer: only applies to S3 API requests (not Admin, not Console) // Admin has its own CORS handling in router.rs @@ -927,8 +927,16 @@ fn get_default_tcp_keepalive() -> TcpKeepalive { #[cfg(test)] mod tests { use super::*; + use crate::server::compress::RequestPathCategory; + use bytes::Bytes; use http::HeaderMap; + use http::Request as HttpRequest; + use http_body_util::Empty; use opentelemetry::propagation::Extractor; + use std::convert::Infallible; + use std::future::Ready; + use std::task::{Context, Poll}; + use tower::{Layer, Service, ServiceBuilder}; /// Baseline constants — reference the authoritative config defaults. /// If a config default changes, tests automatically follow. @@ -1065,4 +1073,89 @@ mod tests { assert_eq!(carrier.get("Content-Type"), Some("application/json")); assert_eq!(carrier.get("CONTENT-TYPE"), Some("application/json")); } + + #[derive(Clone, Copy)] + struct ObserveCategoryLayer; + + #[derive(Clone)] + struct ObserveCategoryService { + inner: S, + } + + impl Layer for ObserveCategoryLayer { + type Service = ObserveCategoryService; + + fn layer(&self, inner: S) -> Self::Service { + ObserveCategoryService { inner } + } + } + + impl Service> for ObserveCategoryService + where + S: Service, Response = Response, Error = Infallible>, + { + type Response = Response; + type Error = Infallible; + type Future = Ready, Infallible>>; + + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn call(&mut self, req: HttpRequest) -> Self::Future { + let response = futures::executor::block_on(self.inner.call(req)).expect("infallible"); + let mut response = response; + let seen = response.extensions().get::().is_some(); + response + .headers_mut() + .insert("x-category-seen", if seen { "true" } else { "false" }.parse().expect("header")); + std::future::ready(Ok(response)) + } + } + + #[derive(Clone, Copy)] + struct OkService; + + impl Service> for OkService { + type Response = Response>; + type Error = Infallible; + type Future = Ready>, Infallible>>; + + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn call(&mut self, _req: HttpRequest) -> Self::Future { + std::future::ready(Ok(Response::new(Empty::new()))) + } + } + + #[test] + fn test_service_builder_order_regression_for_response_extensions() { + let request = HttpRequest::builder().uri("/bucket/archive.zip").body(()).expect("request"); + + let mut broken_order = ServiceBuilder::new() + .layer(PathCategoryInjectionLayer) + .layer(ObserveCategoryLayer) + .service(OkService); + + let broken_response = futures::executor::block_on(broken_order.call(request)).expect("response"); + assert_eq!( + broken_response.headers().get("x-category-seen").and_then(|v| v.to_str().ok()), + Some("false") + ); + + let request = HttpRequest::builder().uri("/bucket/archive.zip").body(()).expect("request"); + + let mut fixed_order = ServiceBuilder::new() + .layer(ObserveCategoryLayer) + .layer(PathCategoryInjectionLayer) + .service(OkService); + + let fixed_response = futures::executor::block_on(fixed_order.call(request)).expect("response"); + assert_eq!( + fixed_response.headers().get("x-category-seen").and_then(|v| v.to_str().ok()), + Some("true") + ); + } } diff --git a/rustfs/src/storage/options.rs b/rustfs/src/storage/options.rs index 6ad860be6..c54bc62bb 100644 --- a/rustfs/src/storage/options.rs +++ b/rustfs/src/storage/options.rs @@ -33,7 +33,7 @@ use rustfs_policy::service_type::ServiceType; use rustfs_utils::hash::EMPTY_STRING_SHA256_HASH; use rustfs_utils::http::AMZ_CONTENT_SHA256; use rustfs_utils::path::is_dir_object; -use s3s::{S3Result, s3_error}; +use s3s::{S3Error, S3ErrorCode, S3Result, s3_error}; use std::collections::HashMap; use std::sync::LazyLock; use tracing::error; @@ -349,6 +349,73 @@ pub(crate) fn normalize_content_encoding_for_storage(value: &str) -> Option bool { + let object_name = object_name.to_ascii_lowercase(); + ARCHIVE_CONTENT_ENCODING_BLOCKED_SUFFIXES + .iter() + .any(|suffix| object_name.ends_with(suffix)) +} + +fn is_archive_content_type_for_content_encoding(content_type: &str) -> bool { + let main_type = content_type + .split(';') + .next() + .unwrap_or(content_type) + .trim() + .to_ascii_lowercase(); + + ARCHIVE_CONTENT_ENCODING_BLOCKED_CONTENT_TYPES + .iter() + .any(|candidate| main_type == *candidate) +} + +pub(crate) fn validate_archive_content_encoding( + object_name: &str, + content_type: Option<&str>, + content_encoding: Option<&str>, +) -> S3Result<()> { + if rustfs_utils::get_env_bool(ENV_ALLOW_ARCHIVE_CONTENT_ENCODING, false) { + return Ok(()); + } + + let Some(content_encoding) = content_encoding.map(str::trim).filter(|value| !value.is_empty()) else { + return Ok(()); + }; + + let is_archive_like = is_archive_object_name_for_content_encoding(object_name) + || content_type.is_some_and(is_archive_content_type_for_content_encoding); + if !is_archive_like { + return Ok(()); + } + + Err(S3Error::with_message( + S3ErrorCode::InvalidArgument, + format!( + "Content-Encoding '{content_encoding}' is not allowed for archive objects by default; set {ENV_ALLOW_ARCHIVE_CONTENT_ENCODING}=true to allow legacy behavior" + ), + )) +} + /// Extracts metadata from headers and returns it as a HashMap with object name for MIME type detection. pub fn extract_metadata_from_mime_with_object_name( headers: &HeaderMap, @@ -692,6 +759,8 @@ fn get_content_sha256_cksum(headers: &HeaderMap, service_type: Serv #[cfg(test)] mod tests { + use temp_env; + use super::*; use http::{HeaderMap, HeaderValue}; use std::collections::HashMap; @@ -1358,6 +1427,30 @@ mod tests { assert_eq!(detect_content_type_from_object_name("noextension"), "application/octet-stream"); } + #[test] + fn test_validate_archive_content_encoding_rejects_archive_suffix_by_default() { + let err = validate_archive_content_encoding("bundle.tar.gz", Some("application/gzip"), Some("gzip")).unwrap_err(); + assert_eq!(err.code(), &S3ErrorCode::InvalidArgument); + } + + #[test] + fn test_validate_archive_content_encoding_rejects_archive_mime_by_default() { + let err = validate_archive_content_encoding("bundle", Some("application/zip"), Some("gzip")).unwrap_err(); + assert_eq!(err.code(), &S3ErrorCode::InvalidArgument); + } + + #[test] + fn test_validate_archive_content_encoding_allows_non_archive_precompressed_object() { + validate_archive_content_encoding("logs/app.log.zst", Some("text/plain"), Some("zstd")).expect("non-archive"); + } + + #[test] + fn test_validate_archive_content_encoding_allows_legacy_opt_in() { + temp_env::with_var(ENV_ALLOW_ARCHIVE_CONTENT_ENCODING, Some("true"), || { + validate_archive_content_encoding("bundle.zip", Some("application/zip"), Some("gzip")).expect("legacy opt-in"); + }); + } + #[test] fn test_parse_copy_source_range() { // Test complete range: bytes=0-1023