Skip to content

fix(lib-storage): replace UploadPart spread with field allowlist - #8029

Open
Zelys-DFKH wants to merge 1 commit into
aws:mainfrom
Zelys-DFKH:fix/lib-storage-upload-uploadpart-params
Open

fix(lib-storage): replace UploadPart spread with field allowlist#8029
Zelys-DFKH wants to merge 1 commit into
aws:mainfrom
Zelys-DFKH:fix/lib-storage-upload-uploadpart-params

Conversation

@Zelys-DFKH

Copy link
Copy Markdown
Contributor

Issue

#8020

Description

Upload.__doConcurrentUpload spreads ...this.params (typed as PutObjectCommandInput) into every UploadPartCommand. PutObjectCommandInput includes object-creation-only fields — IfNoneMatch, ContentType, ContentMD5, Metadata, ACL, StorageClass, ObjectLock*, and others — and S3 rejects them on the UploadPart API with NotImplemented (501) or MalformedXML.

This PR replaces the spread with an explicit allowlist of the fields UploadPartRequest actually accepts: Bucket, Key, the full Checksum* family, SSECustomerAlgorithm/Key/KeyMD5, RequestPayer, and ExpectedBucketOwner. Body, PartNumber, UploadId, and ContentLength are set explicitly as before.

Two intentional omissions:

  • ContentMD5this.params.ContentMD5 is the whole-object MD5. Forwarding it to each part would be wrong. The UploadPartRequest.ContentMD5 field exists for per-part hashes, which callers can supply via request interceptors.
  • ContentLength — already explicitly set to undefined in the existing code; no change.

CompleteMultipartUploadCommand continues to receive ...this.params at the end of the flow, so IfNoneMatch and other CompleteMultipartUpload-valid fields still work correctly there.

Note on PR #7994: That PR strips ContentMD5 via a denylist. This PR uses a full allowlist instead — more defensive against future PutObjectCommandInput additions silently leaking through. Happy to coordinate if the team prefers the denylist approach.

Testing

Added a regression test in the large buffers describe block that:

  • forces the multipart path
  • confirms IfNoneMatch, ContentType, and ContentMD5 don't reach UploadPartCommand
  • confirms SSECustomerAlgorithm (valid for UploadPart) flows through
  • confirms CompleteMultipartUploadCommand still receives IfNoneMatch

All 53 existing tests pass.

Checklist

  • If the PR is a feature, add integration tests (*.integ.spec.ts) or E2E tests.
    • It's not a feature.
  • My E2E tests are resilient to concurrent i/o.
    • I didn't write any E2E tests.
  • I added access level annotations e.g. @public, @internal tags and enabled doc generation on the package. Remember that access level annotations go below the description, not above.
    • I didn't add any public functions.
  • Streams - how do they work?? My WebStream readers/locks are properly lifecycled. Node.js stream backpressure is handled. Error handling.
    • No streams here.

@Zelys-DFKH
Zelys-DFKH requested a review from a team as a code owner May 16, 2026 20:06
@danyalahmed1995

Copy link
Copy Markdown

I think this still has the same root issue in a slightly narrower form.

The PR removes the broad ...this.params spread into UploadPartCommand, which fixes the reported IfNoneMatch failure. But the new allowlist still forwards top-level checksum fields from Upload params into every part upload.

I tested this locally with a temporary regression test using:

ChecksumSHA256: "object-level-sha256-base64=="

on the Upload params. The focused test fails because both mocked UploadPartCommand calls receive that same ChecksumSHA256 value.

That looks like the same class of bug as ContentMD5: the value can be object-level at the Upload API boundary, but UploadPartCommand interprets checksum fields as per-part checksums. So the current allowlist can still create invalid multipart requests for users passing object-level checksum values.

Because of that, I don’t think the safe boundary is “fields accepted by UploadPartCommand.” Some fields are accepted by UploadPartCommand, but should only be passed if lib-storage computed them for that specific part. For user-supplied top-level params, the allowlist probably needs to exclude concrete Checksum* values too, not just ContentMD5.

…ing allowlist

Upload.__doConcurrentUpload spreads ...this.params (PutObjectCommandInput)
into every UploadPartCommand. That carries two classes of fields the
UploadPart API can't handle:

1. PUT-only fields S3 rejects with NotImplemented (501) or MalformedXML:
   IfNoneMatch, ContentType, Metadata, ACL, StorageClass, ObjectLock*.
2. Object-level checksums whose value is a whole-object hash and is wrong
   as a per-part value: ContentMD5 and the precomputed Checksum* family
   (ChecksumSHA*/CRC*/CRC64NVME). Sending them as per-part values causes
   BadDigest (aws#6742).

Replace the spread with an explicit allowlist of fields accepted by
UploadPart with the same semantic meaning as on PutObject: Bucket, Key,
ChecksumAlgorithm, SSECustomerAlgorithm/Key/KeyMD5, RequestPayer,
ExpectedBucketOwner. ChecksumAlgorithm is intentionally kept; it is a
directive (e.g. "SHA256"), not a precomputed hash, so the SDK checksum
middleware can still compute a per-part value from it.

Use inline this.params.X references instead of destructure-then-rebuild:
avoids shadowing the imported ChecksumAlgorithm enum at module scope and
halves the surrounding LOC.

CompleteMultipartUploadCommand continues to receive ...this.params, so
IfNoneMatch and other CompleteMultipartUpload-valid fields still work.

Regression coverage: a multipart test asserts IfNoneMatch, ContentType,
ContentMD5, and ChecksumSHA256 don't reach UploadPart and that
IfNoneMatch still reaches CompleteMultipartUpload; a single-part PUT
path test confirms object-level fields are preserved when multipart is
not used. All 54 tests pass.

Fixes aws#8020. Partially addresses aws#6742.

Credit: @danyalahmed1995 spotted the Checksum* leak on review.
@Zelys-DFKH
Zelys-DFKH force-pushed the fix/lib-storage-upload-uploadpart-params branch from ad69e64 to 22fc2bc Compare May 24, 2026 22:38
@Zelys-DFKH

Copy link
Copy Markdown
Contributor Author

Good catch. You're right that the allowlist still leaks object-level checksums into UploadPart. ChecksumSHA256 at the Upload boundary is a whole-object hash; sending it as a per-part value causes BadDigest. Same class of bug as ContentMD5.

Pushed 22fc2bc: dropped the concrete Checksum* values from the allowlist, kept only ChecksumAlgorithm. That one is a directive (e.g. "SHA256"), not a precomputed hash, so the SDK middleware can still compute a per-part value from it. Extended the regression test to assert ChecksumSHA256 doesn't reach UploadPart, plus a single-part PUT path test confirming object-level checksums are still preserved when multipart isn't used. 54/54 passing locally.

This complements #7990 (fail-fast on full-object Checksum* with multipart). Either this or your #8042 closes the bug; happy to defer to whichever the team prefers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants