Skip to content

allowlist for copy request added #6352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Fred1155 merged 7 commits into from
Aug 26, 2025
+149 −18
Merged

allowlist for copy request added #6352

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cce84bf
allowlist for copy request added
Fred1155 Aug 18, 2025
c103284
changelog added
Fred1155 Aug 18, 2025
04136c4
change the validation condition to only check if the set field is dif…
Fred1155 Aug 25, 2025
65178b2
Merge branch 'master' into bole/add-allowlist-copy-upload
Fred1155 Aug 26, 2025
e19d68d
change to Objects.equal
Fred1155 Aug 26, 2025
a4ffc6f
Merge branch 'master' into bole/add-allowlist-copy-upload
Fred1155 Aug 26, 2025
a46c86f
Merge branch 'bole/add-allowlist-copy-upload' of github.com:aws/aws-s…
Fred1155 Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/next-release/bugfix-AWSSDKforJavav2-17f90b1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "bugfix",
"category": "AWS SDK for Java v2",
"contributor": "",
"description": "Added allowlist when copying PutObjectRequest and CopyObjectRequest to transfer manager request"
}
159 changes: 142 additions & 17 deletions ...in/java/software/amazon/awssdk/services/s3/internal/multipart/SdkPojoConversionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.core.SdkField;
import software.amazon.awssdk.core.SdkPojo;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.services.s3.model.AbortMultipartUploadRequest;
import software.amazon.awssdk.services.s3.model.ChecksumType;
import software.amazon.awssdk.services.s3.model.CompleteMultipartUploadRequest;
Expand Down Expand Up @@ -53,12 +54,104 @@ public final class SdkPojoConversionUtils {
new HashSet<>(Arrays.asList("ChecksumSHA1", "ChecksumSHA256", "ContentMD5", "ChecksumCRC32C", "ChecksumCRC32",
"ChecksumCRC64NVME", "ContentLength"));

private static final Set<String> PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS = new HashSet<>(Arrays.asList(
Copy link
Contributor

@alextwoods alextwoods Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we keep these allowlists up to date over time (ie, if/when there are service model updates that add new fields)? The PutObject to UploadPart translation uses an ignore list instead of allow list - is there a reason we choose allow rather than ignore for this?

Copy link
Contributor

@alextwoods alextwoods Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on offline discussion, I think the allow list here makes sense so I'm good moving forward with it.

"ACL",
"Bucket",
"CacheControl",
"ContentDisposition",
"ContentEncoding",
"ContentLanguage",
"ContentLength",
"ContentMD5",
"ContentType",
"ChecksumAlgorithm",
"ChecksumCRC32",
"ChecksumCRC32C",
"ChecksumCRC64NVME",
"ChecksumSHA1",
"ChecksumSHA256",
"Expires",
"IfMatch",
"IfNoneMatch",
"GrantFullControl",
"GrantRead",
"GrantReadACP",
"GrantWriteACP",
"Key",
"WriteOffsetBytes",
"Metadata",
"ServerSideEncryption",
"StorageClass",
"WebsiteRedirectLocation",
"SSECustomerAlgorithm",
"SSECustomerKey",
"SSECustomerKeyMD5",
"SSEKMSKeyId",
"SSEKMSEncryptionContext",
"BucketKeyEnabled",
"RequestPayer",
"Tagging",
"ObjectLockMode",
"ObjectLockRetainUntilDate",
"ObjectLockLegalHoldStatus",
"ExpectedBucketOwner"
));

private static final Set<String> COPY_OBJECT_TO_COPY_OBJECT_ALLOWED_FIELDS = new HashSet<>(Arrays.asList(
"ACL",
"CacheControl",
"ChecksumAlgorithm",
"ContentDisposition",
"ContentEncoding",
"ContentLanguage",
"ContentType",
"CopySource",
"CopySourceIfMatch",
"CopySourceIfModifiedSince",
"CopySourceIfNoneMatch",
"CopySourceIfUnmodifiedSince",
"CopySourceSSECustomerAlgorithm",
"CopySourceSSECustomerKey",
"CopySourceSSECustomerKeyMD5",
"DestinationBucket",
"DestinationKey",
"Expires",
"GrantFullControl",
"GrantRead",
"GrantReadACP",
"GrantWriteACP",
"Metadata",
"MetadataDirective",
"TaggingDirective",
"ServerSideEncryption",
"SourceBucket",
"SourceKey",
"SourceVersionId",
"StorageClass",
"WebsiteRedirectLocation",
"SSECustomerAlgorithm",
"SSECustomerKey",
"SSECustomerKeyMD5",
"SSEKMSKeyId",
"SSEKMSEncryptionContext",
"BucketKeyEnabled",
"RequestPayer",
"Tagging",
"ObjectLockMode",
"ObjectLockRetainUntilDate",
"ObjectLockLegalHoldStatus",
"ExpectedBucketOwner",
"ExpectedSourceBucketOwner"
));


private SdkPojoConversionUtils() {
}

public static UploadPartRequest toUploadPartRequest(PutObjectRequest putObjectRequest, int partNumber, String uploadId) {

UploadPartRequest.Builder builder = UploadPartRequest.builder();
validateRequestFields(putObjectRequest, builder, PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS);
setSdkFields(builder, putObjectRequest, PUT_OBJECT_REQUEST_TO_UPLOAD_PART_FIELDS_TO_IGNORE);
return builder.uploadId(uploadId).partNumber(partNumber).build();
}
Expand All @@ -67,6 +160,7 @@ public static CompleteMultipartUploadRequest toCompleteMultipartUploadRequest(Pu
String uploadId, CompletedPart[] parts,
long contentLength) {
CompleteMultipartUploadRequest.Builder builder = CompleteMultipartUploadRequest.builder();
validateRequestFields(putObjectRequest, builder, PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS);
setSdkFields(builder, putObjectRequest);

builder.mpuObjectSize(contentLength);
Expand All @@ -81,6 +175,7 @@ public static CompleteMultipartUploadRequest toCompleteMultipartUploadRequest(Pu
public static CreateMultipartUploadRequest toCreateMultipartUploadRequest(PutObjectRequest putObjectRequest) {

CreateMultipartUploadRequest.Builder builder = CreateMultipartUploadRequest.builder();
validateRequestFields(putObjectRequest, builder, PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS);
setSdkFields(builder, putObjectRequest);

if (S3ChecksumUtils.checksumValueSpecified(putObjectRequest)) {
Expand Down Expand Up @@ -130,29 +225,14 @@ public static CompletedPart toCompletedPart(Part part) {

public static ListPartsRequest toListPartsRequest(String uploadId, PutObjectRequest putObjectRequest) {
ListPartsRequest.Builder builder = ListPartsRequest.builder();
validateRequestFields(putObjectRequest, builder, PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS);
setSdkFields(builder, putObjectRequest);
return builder.uploadId(uploadId).build();
}

private static void setSdkFields(SdkPojo targetBuilder, SdkPojo sourceObject) {
setSdkFields(targetBuilder, sourceObject, new HashSet<>());
}

private static void setSdkFields(SdkPojo targetBuilder, SdkPojo sourceObject, Set<String> fieldsToIgnore) {
Map<String, Object> sourceFields = retrieveSdkFields(sourceObject, sourceObject.sdkFields());
List<SdkField<?>> targetSdkFields = targetBuilder.sdkFields();

for (SdkField<?> field : targetSdkFields) {
if (fieldsToIgnore.contains(field.memberName())) {
continue;
}
field.set(targetBuilder, sourceFields.getOrDefault(field.memberName(), null));
}
}

public static CreateMultipartUploadRequest toCreateMultipartUploadRequest(CopyObjectRequest copyObjectRequest) {
CreateMultipartUploadRequest.Builder builder = CreateMultipartUploadRequest.builder();

validateRequestFields(copyObjectRequest, builder, COPY_OBJECT_TO_COPY_OBJECT_ALLOWED_FIELDS);
setSdkFields(builder, copyObjectRequest);
builder.bucket(copyObjectRequest.destinationBucket());
builder.key(copyObjectRequest.destinationKey());
Expand Down Expand Up @@ -180,6 +260,7 @@ private static CopyObjectResult toCopyObjectResult(CompleteMultipartUploadRespon

public static AbortMultipartUploadRequest.Builder toAbortMultipartUploadRequest(CopyObjectRequest copyObjectRequest) {
AbortMultipartUploadRequest.Builder builder = AbortMultipartUploadRequest.builder();
validateRequestFields(copyObjectRequest, builder, COPY_OBJECT_TO_COPY_OBJECT_ALLOWED_FIELDS);
setSdkFields(builder, copyObjectRequest);
builder.bucket(copyObjectRequest.destinationBucket());
builder.key(copyObjectRequest.destinationKey());
Expand All @@ -188,6 +269,7 @@ public static AbortMultipartUploadRequest.Builder toAbortMultipartUploadRequest(

public static AbortMultipartUploadRequest.Builder toAbortMultipartUploadRequest(PutObjectRequest putObjectRequest) {
AbortMultipartUploadRequest.Builder builder = AbortMultipartUploadRequest.builder();
validateRequestFields(putObjectRequest, builder, PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS);
setSdkFields(builder, putObjectRequest);
return builder;
}
Expand Down Expand Up @@ -225,4 +307,47 @@ private static Map<String, Object> retrieveSdkFields(SdkPojo sourceObject, List<
field.getValueOrDefault(sourceObject)),
Map::putAll);
}

private static void setSdkFields(SdkPojo targetBuilder, SdkPojo sourceObject) {
setSdkFields(targetBuilder, sourceObject, new HashSet<>());
}

private static void setSdkFields(SdkPojo targetBuilder, SdkPojo sourceObject, Set<String> fieldsToIgnore) {
Map<String, Object> sourceFields = retrieveSdkFields(sourceObject, sourceObject.sdkFields());
List<SdkField<?>> targetSdkFields = targetBuilder.sdkFields();

for (SdkField<?> field : targetSdkFields) {
if (fieldsToIgnore.contains(field.memberName())) {
continue;
}
field.set(targetBuilder, sourceFields.getOrDefault(field.memberName(), null));
}
}

private static void validateRequestFields(SdkPojo sourceObject, SdkPojo targetBuilder, Set<String> allowedFields) {
Copy link
Contributor

@alextwoods alextwoods Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some test cases for this?

Copy link
Contributor Author

@Fred1155 Fred1155 Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To preserve backward compatibility, all current fields are on the the allow list. I can do a local test by changing the model and see if it fails

Set<String> invalidFields = new HashSet<>();

for (SdkField<?> sourceField : sourceObject.sdkFields()) {
String fieldName = sourceField.memberName();
Object sourceValue = sourceField.getValueOrDefault(sourceObject);

if (!allowedFields.contains(fieldName)) {
SdkField<?> targetField = targetBuilder.sdkFields()
.stream()
.filter(field -> field.memberName().equals(fieldName))
.findFirst()
.orElse(null);
if (targetField != null && !targetField.getValueOrDefault(targetBuilder).equals(sourceValue)) {
invalidFields.add(fieldName);
}
}
}

if (!invalidFields.isEmpty()) {
throw SdkClientException.create(
String.format("The following fields are not allowed: %s",
Copy link
Contributor

@alextwoods alextwoods Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there currently any fields not on the allow list and do we know if any of them have different default values? IE - is there a risk of now raising an exception for cases that was previously working for customers?

Copy link
Contributor Author

@Fred1155 Fred1155 Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All current fields are on the allow list so there shouldn't be cases that raise exception. The existing unit test for SdkPojoConversionUtils should already covered that.

String.join(", ", invalidFields))
);
}
}
}
Loading