diff --git a/.config/e2e-full-selection.txt b/.config/e2e-full-selection.txt index bf0a68683..a3d621f42 100644 --- a/.config/e2e-full-selection.txt +++ b/.config/e2e-full-selection.txt @@ -1,2 +1,2 @@ sha256-darwin=7c705942d3a9e583ebaa85845daa21c4f81fdbf36def24db931aaef38636c53e -sha256-linux=5c33dc395233671cb77452f8fb12900ecdbeb368632222b96835d310bce1aab8 +sha256-linux=01e1f5569be60674e30d9e30a9961c5c53a6cea6ca1f7913218f2adf7291d574 diff --git a/.config/e2e-smoke-selection.txt b/.config/e2e-smoke-selection.txt index c50ca40a8..aef666429 100644 --- a/.config/e2e-smoke-selection.txt +++ b/.config/e2e-smoke-selection.txt @@ -1 +1 @@ -sha256=00f8a56b75990b487a594b97c36610b94476a6944ba00473e37187ea654d42bf +sha256=f794c004e5952147c79dadbe6b7c84804bc986f34a8142731fac8864004e5130 diff --git a/.config/nextest.toml b/.config/nextest.toml index 0574fb73c..3aec9c525 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -701,7 +701,7 @@ test-group = 'ecstore-serial-flaky' [profile.e2e-smoke] default-filter = """ package(e2e_test) & ( - test(/^(delete_marker_migration_semantics|version_id_regression|list_objects_v2_pagination|list_object_versions_regression|list_objects_duplicates|list_buckets_double_slash|list_buckets_auth|list_buckets_iam_filter|leading_slash_key|special_chars|create_bucket_region|delete_objects_versioning|head_object_consistency|head_object_range|copy_object_metadata|copy_object_tagging|copy_source_invalid_date|content_encoding|compression|multipart_storage_class|storage_class_capability|ssec_copy|anonymous_access|bucket_policy_check|presigned_negative|negative_sigv4|admin_auth|notification_webhook|tls_hot_reload|console_smoke|admin_iam_crud|admin_pools|sts_query_compat|data_usage)_test::|^fake_s3_target::/) + test(/^(delete_marker_migration_semantics|version_id_regression|list_objects_v2_pagination|list_object_versions_regression|list_objects_duplicates|list_buckets_double_slash|list_buckets_auth|list_buckets_iam_filter|leading_slash_key|special_chars|create_bucket_region|delete_objects_versioning|head_object_consistency|head_object_range|copy_object_metadata|copy_object_tagging|copy_source_invalid_date|content_encoding|compression|multipart_storage_class|multipart_upload_id|storage_class_capability|ssec_copy|anonymous_access|bucket_policy_check|presigned_negative|negative_sigv4|admin_auth|notification_webhook|tls_hot_reload|console_smoke|admin_iam_crud|admin_pools|sts_query_compat|data_usage)_test::|^fake_s3_target::/) | test(/^replication_extension_test::(test_replication_check_succeeds_with_remote_target|test_replication_check_rejects_target_without_object_lock|test_set_remote_target_rejects_unversioned_source_bucket|test_replication_check_rejects_unversioned_source_bucket|test_replication_check_rejects_missing_replication_config|test_replication_check_rejects_invalid_bucket|test_set_remote_target_rejects_same_bucket_on_same_deployment|test_set_remote_target_rejects_unversioned_target_bucket|test_set_remote_target_update_requires_arn|test_set_remote_target_update_rejects_missing_target|test_set_remote_target_rejects_invalid_target_url|test_set_remote_target_rejects_self_signed_https_target_without_skip_tls_verify|test_set_remote_target_rejects_private_ca_https_target_without_ca_cert_pem|test_list_remote_targets_rejects_empty_bucket|test_list_remote_targets_rejects_invalid_bucket|test_remove_remote_target_rejects_missing_target|test_remove_remote_target_rejects_missing_arn|test_remove_remote_target_rejects_invalid_bucket|test_remove_remote_target_rejects_target_used_by_replication|test_delete_bucket_replication_removes_remote_target)$/) | test(/^reliant::lifecycle::/) | test(/^conditional_delete_objects_test::/) diff --git a/crates/e2e_test/src/lib.rs b/crates/e2e_test/src/lib.rs index 3d1ee362d..480b25885 100644 --- a/crates/e2e_test/src/lib.rs +++ b/crates/e2e_test/src/lib.rs @@ -283,6 +283,9 @@ mod ssec_copy_test; #[cfg(test)] mod multipart_storage_class_test; +#[cfg(test)] +mod multipart_upload_id_test; + #[cfg(test)] mod storage_class_capability_test; diff --git a/crates/e2e_test/src/multipart_upload_id_test.rs b/crates/e2e_test/src/multipart_upload_id_test.rs new file mode 100644 index 000000000..d0a6b9ae1 --- /dev/null +++ b/crates/e2e_test/src/multipart_upload_id_test.rs @@ -0,0 +1,175 @@ +// Copyright 2026 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. + +//! Multipart upload ID regression tests. +//! +//! Every operation that addresses an existing multipart upload must report an +//! upload ID that does not name a live upload as `404 NoSuchUpload`, whether +//! the ID is malformed or is a well-formed ID of an aborted upload. + +#[cfg(test)] +mod tests { + use crate::common::{RustFSTestEnvironment, init_logging}; + use aws_sdk_s3::Client; + use aws_sdk_s3::error::{ProvideErrorMetadata, SdkError}; + use aws_sdk_s3::primitives::ByteStream; + use aws_sdk_s3::types::{CompletedMultipartUpload, CompletedPart}; + + type TestResult = Result<(), Box>; + + const BUCKET: &str = "multipart-upload-id"; + const KEY: &str = "target.bin"; + const COPY_SOURCE_KEY: &str = "copy-source.bin"; + // RustFS upload IDs are URL-safe base64; '.' is outside that alphabet, so + // the object layer rejects this ID as malformed instead of looking it up. + const MALFORMED_UPLOAD_ID: &str = "malformed.upload.id"; + const PART_NUMBER: i32 = 1; + const PART_BODY: &[u8] = b"multipart upload id regression part"; + const PART_ETAG: &str = "\"00000000000000000000000000000000\""; + const NO_SUCH_UPLOAD_STATUS: u16 = 404; + const NO_SUCH_UPLOAD_CODE: &str = "NoSuchUpload"; + const OBJECT_NOT_FOUND_STATUS: u16 = 404; + + /// Describes how `result` differs from `404 NoSuchUpload`, or returns `None` when it matches. + fn no_such_upload_mismatch(operation: &str, result: Result>) -> Option + where + E: ProvideErrorMetadata + std::fmt::Debug, + { + let Err(error) = result else { + return Some(format!("{operation}: succeeded")); + }; + let status = error.raw_response().map(|response| response.status().as_u16()); + let code = error.as_service_error().and_then(ProvideErrorMetadata::code); + (status != Some(NO_SUCH_UPLOAD_STATUS) || code != Some(NO_SUCH_UPLOAD_CODE)) + .then(|| format!("{operation}: status {status:?}, code {code:?}: {error:?}")) + } + + async fn start_server_with_copy_source() -> Result<(RustFSTestEnvironment, Client), Box> + { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server(Vec::new()).await?; + env.create_test_bucket(BUCKET).await?; + let client = env.create_s3_client(); + client + .put_object() + .bucket(BUCKET) + .key(COPY_SOURCE_KEY) + .body(ByteStream::from_static(PART_BODY)) + .send() + .await?; + Ok((env, client)) + } + + /// Sends every operation that addresses an existing upload and asserts that all of them report `404 NoSuchUpload`. + async fn assert_every_upload_operation_reports_no_such_upload(client: &Client, upload_id: &str) { + let upload_part = client + .upload_part() + .bucket(BUCKET) + .key(KEY) + .upload_id(upload_id) + .part_number(PART_NUMBER) + .body(ByteStream::from_static(PART_BODY)) + .send() + .await; + let upload_part_copy = client + .upload_part_copy() + .bucket(BUCKET) + .key(KEY) + .upload_id(upload_id) + .part_number(PART_NUMBER) + .copy_source(format!("/{BUCKET}/{COPY_SOURCE_KEY}")) + .send() + .await; + let list_parts = client.list_parts().bucket(BUCKET).key(KEY).upload_id(upload_id).send().await; + let complete = client + .complete_multipart_upload() + .bucket(BUCKET) + .key(KEY) + .upload_id(upload_id) + .multipart_upload( + CompletedMultipartUpload::builder() + .parts(CompletedPart::builder().part_number(PART_NUMBER).e_tag(PART_ETAG).build()) + .build(), + ) + .send() + .await; + let abort = client + .abort_multipart_upload() + .bucket(BUCKET) + .key(KEY) + .upload_id(upload_id) + .send() + .await; + + let mismatches: Vec = [ + no_such_upload_mismatch("UploadPart", upload_part), + no_such_upload_mismatch("UploadPartCopy", upload_part_copy), + no_such_upload_mismatch("ListParts", list_parts), + no_such_upload_mismatch("CompleteMultipartUpload", complete), + no_such_upload_mismatch("AbortMultipartUpload", abort), + ] + .into_iter() + .flatten() + .collect(); + assert!( + mismatches.is_empty(), + "upload ID {upload_id:?}: operations not reporting {NO_SUCH_UPLOAD_STATUS} {NO_SUCH_UPLOAD_CODE}:\n{}", + mismatches.join("\n") + ); + } + + /// Asserts that the rejected operations left neither a multipart upload nor an object under `KEY`. + async fn assert_no_upload_or_object_published(client: &Client) -> TestResult { + let uploads = client.list_multipart_uploads().bucket(BUCKET).prefix(KEY).send().await?; + assert!(uploads.uploads().is_empty(), "rejected operations must not leave an upload: {uploads:?}"); + let head = client.head_object().bucket(BUCKET).key(KEY).send().await; + let status = head + .as_ref() + .err() + .and_then(|error| error.raw_response()) + .map(|response| response.status().as_u16()); + assert_eq!( + status, + Some(OBJECT_NOT_FOUND_STATUS), + "rejected operations must not publish {KEY}: {head:?}" + ); + Ok(()) + } + + #[tokio::test] + async fn malformed_upload_id_reports_no_such_upload_on_every_upload_operation() -> TestResult { + let (_env, client) = start_server_with_copy_source().await?; + + assert_every_upload_operation_reports_no_such_upload(&client, MALFORMED_UPLOAD_ID).await; + assert_no_upload_or_object_published(&client).await + } + + #[tokio::test] + async fn aborted_upload_id_reports_no_such_upload_on_every_upload_operation() -> TestResult { + let (_env, client) = start_server_with_copy_source().await?; + let created = client.create_multipart_upload().bucket(BUCKET).key(KEY).send().await?; + let upload_id = created.upload_id().ok_or("CreateMultipartUpload returned no upload ID")?; + client + .abort_multipart_upload() + .bucket(BUCKET) + .key(KEY) + .upload_id(upload_id) + .send() + .await?; + + assert_every_upload_operation_reports_no_such_upload(&client, upload_id).await; + assert_no_upload_or_object_published(&client).await + } +} diff --git a/rustfs/src/app/multipart_usecase.rs b/rustfs/src/app/multipart_usecase.rs index b6be3fb7a..c73846975 100644 --- a/rustfs/src/app/multipart_usecase.rs +++ b/rustfs/src/app/multipart_usecase.rs @@ -551,12 +551,9 @@ impl DefaultMultipartUsecase { return Err(S3Error::with_message(S3ErrorCode::InternalError, "Not init".to_string())); }; - // Special handling for abort_multipart_upload: Per AWS S3 API specification, this operation - // should return NoSuchUpload (404) when the upload_id doesn't exist, even if the format - // appears invalid. This differs from other multipart operations (upload_part, list_parts, - // complete_multipart_upload) which return InvalidArgument for malformed upload_ids. - // The lenient validation matches AWS S3 behavior where format validation is relaxed for - // abort operations to avoid leaking information about upload_id format requirements. + // Per the AWS S3 API reference, a malformed upload ID is reported as + // NoSuchUpload (404), the same as an upload that was aborted or + // completed. See multipart_upload_api_error. match store .abort_multipart_upload(bucket.as_str(), key.as_str(), upload_id.as_str(), &opts) .await @@ -569,13 +566,7 @@ impl DefaultMultipartUsecase { ); Ok(S3Response::new(AbortMultipartUploadOutput { ..Default::default() })) } - Err(err) => { - // Convert MalformedUploadID to NoSuchUpload for S3 API compatibility - if matches!(err, StorageError::MalformedUploadID(_)) { - return Err(S3Error::new(S3ErrorCode::NoSuchUpload)); - } - Err(ApiError::from(err).into()) - } + Err(err) => Err(multipart_upload_api_error(err)), } } @@ -700,7 +691,7 @@ impl DefaultMultipartUsecase { let multipart_info = match store.get_multipart_info(&bucket, &key, &upload_id, &opts).await { Ok(info) => Some(info), Err(err) if is_err_invalid_upload_id(&err) => None, - Err(err) => return Err(ApiError::from(err).into()), + Err(err) => return Err(multipart_upload_api_error(err)), }; // A ciphertext-passthrough session stores encrypted parts verbatim and // completes without the customer key (the replication client has none), @@ -1283,7 +1274,7 @@ impl DefaultMultipartUsecase { let fi = store .get_multipart_info(&bucket, &key, &upload_id, &opts) .await - .map_err(ApiError::from)?; + .map_err(multipart_upload_api_error)?; let max_total_object_size = multipart_max_total_object_size(&fi.user_defined)?; let mut size = require_upload_part_size(size, max_total_object_size.is_some())?; if let Some(limit) = max_total_object_size @@ -1678,7 +1669,7 @@ impl DefaultMultipartUsecase { let res = store .list_object_parts(&bucket, &key, &upload_id, params.part_number_marker, params.max_parts, &opts) .await - .map_err(ApiError::from)?; + .map_err(multipart_upload_api_error)?; Ok(S3Response::new(build_list_parts_output(res))) } @@ -1795,7 +1786,7 @@ impl DefaultMultipartUsecase { let mp_info = store .get_multipart_info(&bucket, &key, &upload_id, &dst_opts) .await - .map_err(ApiError::from)?; + .map_err(multipart_upload_api_error)?; let destination_size_limit = multipart_max_total_object_size(&mp_info.user_defined)?; EncryptionRequest { bucket: &bucket, @@ -2124,6 +2115,21 @@ fn normalize_presigned_part_checksums(req: &mut S3Request) -> S Ok(()) } +/// Maps a storage error from an operation that addresses an existing multipart +/// upload (UploadPart, UploadPartCopy, ListParts, CompleteMultipartUpload and +/// AbortMultipartUpload) to an S3 error. +/// +/// A malformed upload ID cannot name an existing upload, so it is reported as +/// `NoSuchUpload` (404), which the AWS S3 error reference describes as "The +/// upload ID might be invalid, or the multipart upload might have been aborted +/// or completed". +fn multipart_upload_api_error(err: StorageError) -> S3Error { + if matches!(err, StorageError::MalformedUploadID(_)) { + return S3Error::new(S3ErrorCode::NoSuchUpload); + } + ApiError::from(err).into() +} + #[cfg(test)] mod tests { use super::*; @@ -2797,6 +2803,22 @@ mod tests { assert!(multipart_max_total_object_size(&metadata).is_err()); } + #[test] + fn multipart_upload_api_error_reports_malformed_upload_id_as_no_such_upload() { + let malformed = multipart_upload_api_error(StorageError::MalformedUploadID("bucket/object-not-an-id".to_string())); + assert_eq!(malformed.code(), &S3ErrorCode::NoSuchUpload); + + let unknown = multipart_upload_api_error(StorageError::InvalidUploadID( + "bucket".to_string(), + "object".to_string(), + "dXBsb2FkLWlk".to_string(), + )); + assert_eq!(unknown.code(), &S3ErrorCode::NoSuchUpload); + + let other = multipart_upload_api_error(StorageError::ObjectNotFound("bucket".to_string(), "object".to_string())); + assert_eq!(other.code(), &S3ErrorCode::NoSuchKey); + } + #[tokio::test] async fn execute_complete_multipart_upload_rejects_missing_parts_payload() { let input = CompleteMultipartUploadInput::builder()