From e84649d97af50f69b13f34777947972e0e6a8c2a Mon Sep 17 00:00:00 2001 From: Andrew Gaul Date: Sun, 23 Aug 2026 19:26:18 -0700 Subject: [PATCH] fix(s3): apply the ACL and the source conditions a copy carries CopyObject dropped x-amz-acl, so a copy addressed public-read landed private and stayed that way -- the caller had no way to tell except by reading the ACL back. It now permissions the destination as a put does, and refuses an unknown value rather than ignoring it. A copy without one leaves the destination private rather than inheriting whatever the key it replaced carried. UploadPartCopy ignored all four x-amz-copy-source-if-* headers and copied the range regardless, which is the hazard a conditional read exists to prevent: the part is assembled from a source that has since changed. CopyObject already judged them correctly, so the block moves into a helper both operations ask -- one place to be right, and no chance of the two drifting apart. Co-Authored-By: Claude Opus 5 (1M context) --- ministack/services/s3.py | 86 ++++++++++++++++++++++++++++------------ tests/test_s3.py | 56 ++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 26 deletions(-) diff --git a/ministack/services/s3.py b/ministack/services/s3.py index 803f39be..aa28fb9c 100644 --- a/ministack/services/s3.py +++ b/ministack/services/s3.py @@ -4226,6 +4226,44 @@ def _object_mtime_dt(obj: dict): return dt.replace(microsecond=0) +def _check_copy_source_preconditions(headers: dict, src_obj: dict): + """Judge the x-amz-copy-source-if-* headers against the source. + + Per the AWS CopyObject reference (RFC 7232 precedence): + x-amz-copy-source-if-match takes precedence over -if-unmodified-since, + and -if-none-match over -if-modified-since, so the date header only + applies when its ETag counterpart is absent. UploadPartCopy carries the + same four headers and judges them the same way, so both ask here. + + Returns an error response, or None when every condition holds.""" + msg = "At least one of the pre-conditions you specified did not hold" + src_mtime = _object_mtime_dt(src_obj) + src_etag = (src_obj.get("etag") or "").strip('"') + + if_match = headers.get("x-amz-copy-source-if-match", "") + if if_match: + if if_match.strip('"') != src_etag: + return _error("PreconditionFailed", msg, 412) + else: + unmod = _parse_http_date( + headers.get("x-amz-copy-source-if-unmodified-since", "")) + # "Unmodified since" fails when the source was modified after it. + if unmod and src_mtime and src_mtime > unmod: + return _error("PreconditionFailed", msg, 412) + + if_none_match = headers.get("x-amz-copy-source-if-none-match", "") + if if_none_match: + if if_none_match.strip('"') == src_etag: + return _error("PreconditionFailed", msg, 412) + else: + mod = _parse_http_date( + headers.get("x-amz-copy-source-if-modified-since", "")) + # "Modified since" fails when the source has NOT changed since it. + if mod and src_mtime and src_mtime <= mod: + return _error("PreconditionFailed", msg, 412) + return None + + def _copy_object(bucket_name: str, dest_key: str, headers: dict): # Split the raw header at "?" before percent-decoding: a key legitimately # containing "?versionId" arrives encoded (%3FversionId) and must stay @@ -4258,6 +4296,13 @@ def _copy_object(bucket_name: str, dest_key: str, headers: dict): if dest_bucket is None: return _no_such_bucket(bucket_name) + # A canned ACL on the copy applies to the destination, as it does on a + # put; an unknown value rejects the whole request rather than being + # dropped on the floor. + canned_acl = headers.get("x-amz-acl") + if canned_acl and canned_acl not in _CANNED_OBJECT_ACLS: + return _error("InvalidArgument", f"Invalid x-amz-acl value: {canned_acl}", 400) + if src_version_id: ventry = next( (v for v in _object_versions.get((src_bucket_name, src_key), []) @@ -4304,32 +4349,9 @@ def _copy_object(bucket_name: str, dest_key: str, headers: dict): # AWS echoes the copied source version on a versioned source. copy_src_vid = src_version_id or src_obj.get("version_id") - # Copy-source preconditions. Per the AWS CopyObject reference (RFC 7232 - # precedence): x-amz-copy-source-if-match takes precedence over - # -if-unmodified-since, and -if-none-match over -if-modified-since, so the - # date header only applies when its ETag counterpart is absent. - _precond_msg = "At least one of the pre-conditions you specified did not hold" - src_mtime = _object_mtime_dt(src_obj) - - if_match = headers.get("x-amz-copy-source-if-match", "") - if if_match: - if if_match.strip('"') != src_obj["etag"].strip('"'): - return _error("PreconditionFailed", _precond_msg, 412) - else: - unmod = _parse_http_date(headers.get("x-amz-copy-source-if-unmodified-since", "")) - # "Unmodified since" fails when the source was modified after the given time. - if unmod and src_mtime and src_mtime > unmod: - return _error("PreconditionFailed", _precond_msg, 412) - - if_none_match = headers.get("x-amz-copy-source-if-none-match", "") - if if_none_match: - if if_none_match.strip('"') == src_obj["etag"].strip('"'): - return _error("PreconditionFailed", _precond_msg, 412) - else: - mod = _parse_http_date(headers.get("x-amz-copy-source-if-modified-since", "")) - # "Modified since" fails when the source has NOT changed since the given time. - if mod and src_mtime and src_mtime <= mod: - return _error("PreconditionFailed", _precond_msg, 412) + precond_err = _check_copy_source_preconditions(headers, src_obj) + if precond_err is not None: + return precond_err directive = headers.get("x-amz-metadata-directive", "COPY").upper() if directive == "REPLACE": @@ -4455,6 +4477,14 @@ def _copy_object(bucket_name: str, dest_key: str, headers: dict): else: _object_tags.pop((bucket_name, dest_key, dest_version_id), None) + if canned_acl: + _object_acl[(bucket_name, dest_key, dest_version_id)] = ( + _canned_acl_policy_xml(canned_acl, _canonical_owner_id())) + else: + # The destination is a new object: it does not inherit whatever the + # key it replaced was permissioned with. + _object_acl.pop((bucket_name, dest_key, dest_version_id), None) + root = Element("CopyObjectResult", xmlns=S3_NS) SubElement(root, "LastModified").text = last_modified SubElement(root, "ETag").text = new_etag @@ -5593,6 +5623,10 @@ def _upload_part_copy(bucket_name: str, dest_key: str, query_params: dict, heade if sse_src_err is not None: return sse_src_err + precond_err = _check_copy_source_preconditions(headers, ventry) + if precond_err is not None: + return precond_err + # Handle x-amz-copy-source-range copy_range = headers.get("x-amz-copy-source-range", "") if copy_range: diff --git a/tests/test_s3.py b/tests/test_s3.py index 0d0810c3..120477b2 100644 --- a/tests/test_s3.py +++ b/tests/test_s3.py @@ -5644,3 +5644,59 @@ def test_s3_restore_notifications_to_sqs(s3, sqs): red = completed["glacierEventData"]["restoreEventData"] assert red["lifecycleRestoreStorageClass"] == "DEEP_ARCHIVE" assert red["lifecycleRestorationExpiryTime"].endswith("Z") + + +def test_s3_copy_object_applies_canned_acl(s3): + """x-amz-acl on a copy permissions the destination, as it does on a put.""" + bkt = "s3-copy-acl" + s3.create_bucket(Bucket=bkt) + s3.put_object(Bucket=bkt, Key="src.txt", Body=b"body") + + s3.copy_object(Bucket=bkt, Key="dst.txt", ACL="public-read", + CopySource={"Bucket": bkt, "Key": "src.txt"}) + grants = s3.get_object_acl(Bucket=bkt, Key="dst.txt")["Grants"] + assert any(g.get("Grantee", {}).get("URI", "").endswith("AllUsers") + for g in grants) + + # A copy without one leaves the destination private. + s3.copy_object(Bucket=bkt, Key="plain.txt", + CopySource={"Bucket": bkt, "Key": "src.txt"}) + grants = s3.get_object_acl(Bucket=bkt, Key="plain.txt")["Grants"] + assert not any(g.get("Grantee", {}).get("URI", "").endswith("AllUsers") + for g in grants) + + with pytest.raises(ClientError) as exc: + s3.copy_object(Bucket=bkt, Key="bad.txt", ACL="nonsense", + CopySource={"Bucket": bkt, "Key": "src.txt"}) + assert exc.value.response["Error"]["Code"] == "InvalidArgument" + + +def test_s3_upload_part_copy_honours_source_preconditions(s3): + """UploadPartCopy carries the same copy-source conditions CopyObject does.""" + bkt = "s3-upc-precond" + s3.create_bucket(Bucket=bkt) + body = b"x" * (5 * 1024 * 1024) + etag = s3.put_object(Bucket=bkt, Key="src.bin", Body=body)["ETag"] + + upload = s3.create_multipart_upload(Bucket=bkt, Key="dst.bin")["UploadId"] + with pytest.raises(ClientError) as exc: + s3.upload_part_copy(Bucket=bkt, Key="dst.bin", UploadId=upload, + PartNumber=1, + CopySource={"Bucket": bkt, "Key": "src.bin"}, + CopySourceIfMatch='"00000000000000000000000000000000"') + assert exc.value.response["ResponseMetadata"]["HTTPStatusCode"] == 412 + + with pytest.raises(ClientError) as exc: + s3.upload_part_copy(Bucket=bkt, Key="dst.bin", UploadId=upload, + PartNumber=1, + CopySource={"Bucket": bkt, "Key": "src.bin"}, + CopySourceIfNoneMatch=etag) + assert exc.value.response["ResponseMetadata"]["HTTPStatusCode"] == 412 + + # The condition that holds copies the range. + part = s3.upload_part_copy(Bucket=bkt, Key="dst.bin", UploadId=upload, + PartNumber=1, + CopySource={"Bucket": bkt, "Key": "src.bin"}, + CopySourceIfMatch=etag) + assert part["CopyPartResult"]["ETag"] + s3.abort_multipart_upload(Bucket=bkt, Key="dst.bin", UploadId=upload)