s3mini use presigned URLs instead of signed headers #6130
Conversation
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6a77263d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR refactors the S3mini client to use presigned URLs instead of signed headers for authentication. Presigned URLs include the AWS signature in the query string rather than in request headers, simplifying the request flow and enabling easier URL sharing.
Changes:
- Replaced signed header authentication with presigned URL generation in the S3 signing mechanism
- Simplified the request flow by removing header-based signing logic and query parameter filtering
- Updated tests to verify presigned URL functionality instead of checksum validation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/@uppy/aws-s3/src/s3-client/types.ts | Replaced signableRequest/signedHeaders types with presignableRequest/presignedResponse types; removed SSECHeaders and AWSHeaders interfaces |
| packages/@uppy/aws-s3/src/s3-client/signer.ts | Completely rewrote signature generation to produce presigned URLs with query string signatures; added required endpoint parameter to SignerConfig |
| packages/@uppy/aws-s3/src/s3-client/S3.ts | Replaced _signedRequest with _presignedRequest method; removed support for custom headers and query parameters in favor of presigned URLs |
| packages/@uppy/aws-s3/tests/s3-client/minio.test.js | Replaced checksum validation tests with simpler presigned URL functionality tests |
| packages/@uppy/aws-s3/tests/s3-client/_shared.test.js | Updated to pass endpoint parameter to createSigV4Signer |
| packages/@uppy/aws-s3/src/index.ts | Added AwsS3Part interface export (appears unrelated to presigned URL changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80483a6d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const res = await this._presignedRequest('PUT', key, { | ||
| uploadId, | ||
| partNumber, | ||
| body, | ||
| headers: { | ||
| [C.HEADER_CONTENT_LENGTH]: U.getByteSize(data), | ||
| ...ssecHeaders, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
The uploadPart method still accepts an opts object, but the presigned request is generated only with uploadId, partNumber, and body. Any extra query parameters a caller previously relied on (e.g., backend-specific flags or pagination/checksum extensions) are now silently dropped and therefore not included in the signed URL, which can make the upload fail or behave differently. Consider threading opts into the presigning input or removing the parameter if it is no longer supported.
Useful? React with 👍 / 👎.
1 participant
This PR replaces signed headers with presigned urls , for signing.
signer.tsnow returns signed url , which is directly used by_presignedRequestmethod to make calls to s3