fix(s3): honor the ACL headers on CopyObject, and share one resolver

crates/fakecloud-s3/src/service/mod.rs

@@ -194,6 +194,25 @@ pub(crate) fn reject_conflicting_acl_sources(
     }
 }
 
+/// The ACL an object write asks for: a canned value, or resolved grant headers,
+/// or neither.
+#[derive(Debug)]
+pub(crate) struct WriteAclHeaders {
+    pub(crate) canned: Option<String>,
+    pub(crate) grants: Option<Vec<AclGrant>>,
+}
+
+impl WriteAclHeaders {
+    /// The grants to store, given the owner to fall back to.
+    pub(crate) fn grants_for(&self, owner_id: &str) -> Option<Vec<AclGrant>> {
+        match (&self.grants, self.canned.as_deref()) {
+            (Some(grants), _) => Some(grants.clone()),
+            (None, Some(acl)) => Some(canned_acl_grants_for_object(acl, owner_id)),
+            (None, None) => None,
+        }
+    }
+}
+
 /// Whether a request body carries nothing an ACL could be read from.
 ///
 /// The mutual-exclusion checks and the "names no ACL at all" check have to agree
@@ -208,6 +227,121 @@ pub(crate) fn body_is_blank(body: &[u8]) -> bool {
     std::str::from_utf8(body).map_or(true, |s| s.trim().is_empty())
 }
 
+impl S3Service {
+    /// Validate and resolve the ACL headers of an object write.
+    ///
+    /// Every object-write path goes through here: PutObject,
+    /// CreateMultipartUpload and CopyObject each used to keep their own copy of
+    /// these four checks, and the ORDER matters -- the canned value is validated
+    /// before the grant headers are resolved, and both before the
+    /// "named two ways" rejection, so a request carrying an unresolvable grant
+    /// answers InvalidArgument rather than InvalidRequest. The conformance probe
+    /// populates the canned member and the grant members together, and only the
+    /// first of those codes is in the S3 error allowlist, so a path that
+    /// reordered its own copy would silently drop probe variants.
+    ///
+    /// Call it before any work the request would have to undo -- in particular
+    /// before the body is spooled to disk, since nothing unlinks the spool file
+    /// on an error path.
+    pub(crate) fn resolve_write_acl_headers(
+        &self,
+        account_id: &str,
+        bucket: &str,
+        headers: &HeaderMap,
+    ) -> Result<WriteAclHeaders, AwsServiceError> {
+        // A present-but-blank value is treated as absent, the same rule
+        // `has_grant_headers` applies to the `x-amz-grant-*` family and for the
+        // same reason: it is what a client sends for an unset config field, and
+        // testing presence alone turned that into a hard 400 on every
+        // ACL-accepting write.
+        let canned = headers
+            .get("x-amz-acl")
+            .and_then(|v| v.to_str().ok())
+            .filter(|s| !s.trim().is_empty())
+            .map(|s| s.to_string());
+        if let Some(acl) = canned.as_deref() {
+            validate_object_canned_acl(acl)?;
+        }
+        let grants = if has_grant_headers(headers) {
+            Some(resolved_grant_headers(headers)?)
+        } else {
+            None
+        };
+        if canned.is_some() && grants.is_some() {
+            return Err(AwsServiceError::aws_error(
+                StatusCode::BAD_REQUEST,
+                "InvalidRequest",
+                "Specifying both Canned ACLs and Header Grants is not allowed",
+            ));
+        }
+        // The grants the request resolves to, which is what the public-ACL check
+        // below has to judge.
+        let requested = match (&grants, canned.as_deref()) {
+            (Some(g), _) => Some(g.clone()),
+            (None, Some(acl)) => Some(canned_acl_grants_for_object(acl, account_id)),
+            (None, None) => None,
+        };
+
+        // BucketOwnerEnforced disables object ACLs. The model is precise about
+        // the one exception: such a bucket "only accept[s] PUT requests that
+        // don't specify an ACL or PUT requests that specify bucket owner full
+        // control ACLs, such as the bucket-owner-full-control canned ACL or an
+        // equivalent form of this ACL expressed in the XML format".
+        //
+        // So the test is the ACL the request NAMES, not the grants it resolves
+        // to. Judging by resolved shape would also accept `private`,
+        // `bucket-owner-read` and `aws-exec-read`, all of which AWS refuses
+        // here: `private` is owner-only by definition, and the other two
+        // collapse onto an owner-only grant only because their real grantees
+        // are not modeled. `private` in particular is neither "no ACL" nor
+        // "bucket owner full control".
+        let asks_for_owner_full_control = match (&grants, canned.as_deref()) {
+            // "an equivalent form of this ACL": explicit grants that give the
+            // owner full control and nobody anything.
+            (Some(g), _) => {
+                !g.is_empty()
+                    && g.iter().all(|grant| {
+                        grant.permission == "FULL_CONTROL"
+                            && grant.grantee_type == "CanonicalUser"
+                            && grant.grantee_id.as_deref() == Some(account_id)
+                    })
+            }
+            (None, Some(acl)) => acl == "bucket-owner-full-control",
+            (None, None) => false,
+        };
+        let specifies_an_acl = grants.is_some() || canned.is_some();
+        if specifies_an_acl
+            && !asks_for_owner_full_control
+            && self.bucket_owner_enforced(account_id, bucket)
+        {
+            return Err(AwsServiceError::aws_error(
+                StatusCode::BAD_REQUEST,
+                "AccessControlListNotSupported",
+                "The bucket does not allow ACLs",
+            ));
+        }
+
+        // BlockPublicAcls refuses a public grant at write time too. Only the
+        // Put*Acl paths enforced it, so `put-object --acl public-read` stored an
+        // AllUsers grant on a bucket that blocks exactly that while
+        // `put-object-acl --acl public-read` was refused; AWS refuses both.
+        if let Some(requested) = requested.as_deref() {
+            if crate::service::config::grants_are_public(requested) {
+                if let Some(flags) = self.pab_flags(account_id, bucket) {
+                    if flags.block_public_acls {
+                        return Err(AwsServiceError::aws_error(
+                            StatusCode::FORBIDDEN,
+                            "AccessDenied",
+                            "User is not authorized to perform: s3:PutObject. Reason: Public Access Block (BlockPublicAcls)",
+                        ));
+                    }
+                }
+            }
+        }
+        Ok(WriteAclHeaders { canned, grants })
+    }
+}
+
 /// Map a [`StoreError`] from the persistence layer to a 500 InternalError
 /// response. Invoked at every mutation site when the write-through persistence
 /// call fails: the in-memory mutation has already happened, but we surface the

crates/fakecloud-s3/src/service/multipart.rs

@@ -14,7 +14,7 @@ use md5::{Digest, Md5};
 use super::{
     canned_acl_grants, compute_md5, extract_user_metadata, no_such_bucket, no_such_key,
     no_such_upload, parse_complete_multipart_xml, parse_url_encoded_tags, precondition_failed,
-    resolve_object, resolved_grant_headers, s3_xml, xml_escape, S3Service,
+    resolve_object, s3_xml, xml_escape, S3Service,
 };
 
 /// Build the `CompleteMultipartUploadResult` XML response for an object that
@@ -95,32 +95,7 @@ impl S3Service {
             .get("x-amz-tagging")
             .and_then(|v| v.to_str().ok())
             .map(|s| s.to_string());
-        let acl_header = req
-            .headers
-            .get("x-amz-acl")
-            .and_then(|v| v.to_str().ok())
-            .map(|s| s.to_string());
-        let has_grant_headers = super::has_grant_headers(&req.headers);
-        // Every sibling ACL-setting path rejects an ACL on a bucket whose
-        // ownership disables them; this one used to accept it and carry the
-        // grants into the completed object, which now persists them.
-        if (acl_header.is_some() || has_grant_headers)
-            && self.bucket_owner_enforced(account_id, bucket)
-        {
-            return Err(AwsServiceError::aws_error(
-                StatusCode::BAD_REQUEST,
-                "AccessControlListNotSupported",
-                "The bucket does not allow ACLs",
-            ));
-        }
-
-        if acl_header.is_some() && has_grant_headers {
-            return Err(AwsServiceError::aws_error(
-                StatusCode::BAD_REQUEST,
-                "InvalidRequest",
-                "Specifying both Canned ACLs and Header Grants is not allowed",
-            ));
-        }
+        let write_acl = self.resolve_write_acl_headers(account_id, bucket, &req.headers)?;
 
         let checksum_algorithm = req
             .headers
@@ -136,13 +111,9 @@ impl S3Service {
             .get_mut(bucket)
             .ok_or_else(|| no_such_bucket(bucket))?;
 
-        let acl_grants = if has_grant_headers {
-            resolved_grant_headers(&req.headers)?
-        } else {
-            let acl = acl_header.as_deref().unwrap_or("private");
-            super::validate_object_canned_acl(acl)?;
-            canned_acl_grants(acl, &b.acl_owner_id)
-        };
+        let acl_grants = write_acl
+            .grants_for(&b.acl_owner_id)
+            .unwrap_or_else(|| canned_acl_grants("private", &b.acl_owner_id));
 
         let upload = MultipartUpload {
             upload_id: upload_id.clone(),

crates/fakecloud-s3/src/service/objects/mod.rs

@@ -11,13 +11,12 @@ use crate::persistence::object_meta_snapshot;
 use crate::state::{AclGrant, S3Object};
 
 use super::{
-    canned_acl_grants_for_object, check_get_conditionals, check_head_conditionals,
-    check_object_lock_for_overwrite, compute_checksum, deliver_notifications, etag_matches,
-    extract_user_metadata, extract_xml_value, is_frozen, is_valid_storage_class,
-    make_delete_marker, no_such_bucket, no_such_key, parse_delete_objects_quiet,
-    parse_delete_objects_xml, parse_range_header, parse_url_encoded_tags, precondition_failed,
-    replicate_through_store, resolve_object, resolved_grant_headers, s3_xml, url_encode_s3_key,
-    xml_escape, RangeResult, S3Service,
+    check_get_conditionals, check_head_conditionals, check_object_lock_for_overwrite,
+    compute_checksum, deliver_notifications, etag_matches, extract_user_metadata,
+    extract_xml_value, is_frozen, is_valid_storage_class, make_delete_marker, no_such_bucket,
+    no_such_key, parse_delete_objects_quiet, parse_delete_objects_xml, parse_range_header,
+    parse_url_encoded_tags, precondition_failed, replicate_through_store, resolve_object, s3_xml,
+    url_encode_s3_key, xml_escape, RangeResult, S3Service,
 };
 
 mod delete;

crates/fakecloud-s3/src/service/objects/write.rs

@@ -40,47 +40,10 @@ impl S3Service {
             .and_then(|v| v.to_str().ok())
             .map(|s| s.to_string());
 
-        // Check for ACL header
-        let acl_header = req
-            .headers
-            .get("x-amz-acl")
-            .and_then(|v| v.to_str().ok())
-            .map(|s| s.to_string());
-
-        // Check for grant headers alongside canned ACL
-        let has_grant_headers = super::super::has_grant_headers(&req.headers);
-
-        // Validated here, before `take_body_stream` spools the payload to disk:
+        // Resolved here, before `take_body_stream` spools the payload to disk:
         // returning after the spool leaks the file, since nothing unlinks it on
         // the error paths.
-        if let Some(acl) = acl_header.as_deref() {
-            super::super::validate_object_canned_acl(acl)?;
-        }
-        if has_grant_headers {
-            resolved_grant_headers(&req.headers)?;
-        }
-        if acl_header.is_some() && has_grant_headers {
-            return Err(AwsServiceError::aws_error(
-                StatusCode::BAD_REQUEST,
-                "InvalidRequest",
-                "Specifying both Canned ACLs and Header Grants is not allowed",
-            ));
-        }
-
-        // BucketOwnerEnforced disables object ACLs at write-time too:
-        // any x-amz-acl or x-amz-grant-* header rejects with
-        // AccessControlListNotSupported. Plain PutObject without ACL
-        // headers continues to work — only attempts to set a grant
-        // are rejected.
-        if (acl_header.is_some() || has_grant_headers)
-            && self.bucket_owner_enforced(account_id, bucket)
-        {
-            return Err(AwsServiceError::aws_error(
-                StatusCode::BAD_REQUEST,
-                "AccessControlListNotSupported",
-                "The bucket does not allow ACLs",
-            ));
-        }
+        let write_acl = self.resolve_write_acl_headers(account_id, bucket, &req.headers)?;
 
         // Parse tags from header
         let tags = if let Some(tagging) = &tagging_header {
@@ -306,12 +269,8 @@ impl S3Service {
         });
 
         // Build ACL grants for object
-        let acl_grants = if has_grant_headers {
-            // Already validated before the body was spooled; this cannot fail
-            // here, but resolving again keeps one source for the grants.
-            resolved_grant_headers(&req.headers)?
-        } else if let Some(ref acl) = acl_header {
-            canned_acl_grants_for_object(acl, &acl_owner_id)
+        let acl_grants = if let Some(grants) = write_acl.grants_for(&acl_owner_id) {
+            grants
         } else {
             // Default: owner full control
             vec![AclGrant {
@@ -859,6 +818,15 @@ impl S3Service {
             .and_then(|v| v.to_str().ok())
             .map(|s| s.to_uppercase());
 
+        // CopyObject accepts the same ACL headers as PutObject: a canned
+        // `x-amz-acl` or the `x-amz-grant-*` pair, never both. Ignoring them --
+        // which is what this path used to do -- answered 200 for
+        // `copy-object --acl public-read` and produced a private object, so the
+        // caller believed it had published the copy.
+        // Resolved before the write lock, since the check reads bucket state of
+        // its own, and before the copy does any work.
+        let copy_acl = self.resolve_write_acl_headers(account_id, dest_bucket, &req.headers)?;
+
         let mut accts = self.state.write();
         let state = accts.get_or_create(account_id);
 
@@ -1152,14 +1120,18 @@ impl S3Service {
             None
         };
 
-        // Default ACL for destination (not copied from source)
-        let dest_acl_grants = vec![AclGrant {
-            grantee_type: "CanonicalUser".to_string(),
-            grantee_id: Some(db.acl_owner_id.clone()),
-            grantee_display_name: Some(db.acl_owner_id.clone()),
-            grantee_uri: None,
-            permission: "FULL_CONTROL".to_string(),
-        }];
+        // The destination's ACL comes from this request, never from the source:
+        // S3 treats a copy as a new object, so an unspecified ACL is the default
+        // private one rather than whatever the source carried.
+        let dest_acl_grants = copy_acl.grants_for(&db.acl_owner_id).unwrap_or_else(|| {
+            vec![AclGrant {
+                grantee_type: "CanonicalUser".to_string(),
+                grantee_id: Some(db.acl_owner_id.clone()),
+                grantee_display_name: Some(db.acl_owner_id.clone()),
+                grantee_uri: None,
+                permission: "FULL_CONTROL".to_string(),
+            }]
+        });
 
         let dest_obj = S3Object {
             key: dest_key.to_string(),