Restore #6284: AWS S3 rewrite#6345
Open
qxprakash wants to merge 15 commits into
Open
Conversation
qxprakash
commented
Jun 17, 2026
qxprakash
commented
Collaborator
- restore AWS S3 rewrite #6284 to a6eddb1
- sign all the commits which were authored by me and were unsigned
- base branch cleanup and ran `yarn && yarn dedupe` - client copied and tests wired up
- added `getCredentials` callback in `S3Config` - simplified signer test util ( `sigv4-signer.js` ) and moved to `src/signer.ts` - all tests run in a new non root minio user ( required for sts ) , which gets created before the tests start see `tests/s3-client/setup.js` - tests added for sts , using `@aws-sdk/client-sts` for getting creds. - credential caching inspired from the current implementation in `@aws-s3` plugin , see `#getTemporarySecurityCredentials` inside `@uppy/aws-s3/src/index.ts` --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
#### This PR replaces signed headers with presigned urls , for signing. - I implemented this along with the uploader rewrite , but raised PR for this it separately to reduce diffs. - `signer.ts` now returns signed url , which is directly used by `_presignedRequest` method to make calls to s3 - tested it with uploader rewrite working fine. --------- Co-authored-by: Murderlon <merlijn@soverin.net>
Working : - Simple uploads (putObject) - Multipart uploads - Pause / Resume / Abort , Resume works using listParts as before - Progress tracking per chunk ( `#onProgress` ) - this is temporary , until we implement xhr based progress tracking as - before. **added new examples to examples/aws-nodejs to test the rewrite , old examples will eventually be removed** Not Yet Implemented - Rate limiting – Currently uploads are sequential - Retries - Remote file uploads – Files from Google Drive, Dropbox, etc. XHR progress - full wiring with uppy state ( I saw few state bugs while testing it manually ) --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Murderlon <merlijn@soverin.net>
Cherry-picked as the previous branch got messy ( from last PR ) - XHR-based uploads using `@uppy/utils/fetcher` - Silent offline detection (without treating it as an error) with automatic resume ( inspired from old plugin code ) - Retry with backoff using fetcher defaults: - Max retries: 3 - Delays: 300ms → 600ms → 1200ms - Retries on 5xx and 429; skips other 4xx errors - S3 Error Mappings
shouldUseMultipart: true was not working due to signature mismatch AI summary : This pull request improves support for specifying the `contentType` during S3 uploads, ensuring that the correct content type is set when generating presigned URLs for both single and multipart uploads. It also updates the client-side and server-side code to handle this new parameter. **Backend and API enhancements:** - Updated the `/s3/presign` endpoint in `index.js` to accept an optional `contentType` parameter and to include it in the S3 `PutObjectCommand` and `CreateMultipartUploadCommand` requests, defaulting to `'application/octet-stream'` if not provided. [[1]](diffhunk://#diff-0c4471088c62917198e1438777b4c7e78fd4d87d450d2a7c78b86b7c647ac97bL360-R360) [[2]](diffhunk://#diff-0c4471088c62917198e1438777b4c7e78fd4d87d450d2a7c78b86b7c647ac97bR384-R391) **Client-side and types improvements:** - Added `contentType` to the `presignableRequest` type to ensure type safety and clarity when passing this parameter. - Modified the S3 client (`S3.ts`) to include `contentType` when making presign requests. - Updated the example HTML (`rewrite-test.html`) to send `contentType` in presign requests and enabled multipart uploads for testing. [[1]](diffhunk://#diff-ca9bc28a71d2d8ac4e0aa6844698f2244594a12e695fc3e153db36674ccb2b52L103-R103) [[2]](diffhunk://#diff-ca9bc28a71d2d8ac4e0aa6844698f2244594a12e695fc3e153db36674ccb2b52R114)
- queue support added for file level concurrency.
- removed old examples , entirely remove withCustomEndpoints.html as every s3 operations are now handled by s3mini so no need of it. - add new examples and update README.md
right now whenever you upload any file through multipart upload you'll find empty body returned in `upload-success` event cause : response from completeMultipartRequest returned in CamelCase which was not parsed correctly ```xml <?xml version="1.0" encoding="UTF-8"?> <CompleteMultipartUploadResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Location>https://testbucketnewfix.s3.eu-north-1.amazonaws.com/13febe01-8d5b-4ed3-bda1-9620fc37c694-VSCode-darwin-universal.dmg</Location><Bucket>testbucketnewfix</Bucket><Key>13febe01-8d5b-4ed3-bda1-9620fc37c694-VSCode-darwin-universal.dmg</Key><ETag>"be4129821e06a893b453f1373995202b-49"</ETag><ChecksumCRC64NVME>Wjr7mcJOfU0=</ChecksumCRC64NVME><ChecksumType>FULL_OBJECT</ChecksumType></CompleteMultipartUploadResult> ``` --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- add support for remote uploads - add support for restore using golden-retriever - update examples - #6181 still persists --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Mikael Finstad <finstaden@gmail.com>
- update outdated examples based on latest api changes
|
![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: 49398593bd
ℹ️ 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".
| this.#setResumableUploadsCapability(true) | ||
| return upload | ||
| #generateKey(file: UppyFile<M, B>): string { | ||
| return this.opts.generateObjectKey?.(file) ?? file.name |
There was a problem hiding this comment.
With signRequest/getCredentials and no generateObjectKey, this uses the raw filename as the object key. S3 overwrites existing objects at the same key, so two uploads named image.jpg collide even though the option documentation above says the default is {randomId}-{filename}. Restore a unique default for direct S3 uploads.
Useful? React with 👍 / 👎.
Comment on lines
+195
to
+199
| request: { method: 'PUT', key }, | ||
| data, | ||
| onProgress, | ||
| signal, | ||
| contentType: fileType, |
There was a problem hiding this comment.
In signRequest mode, the signer sees only { method: 'PUT', key }, while the actual upload still sends Content-Type: fileType. If the signing backend includes ContentType in its AWS SDK command (the new Node example reads request.contentType and otherwise defaults to application/octet-stream), a non-octet file such as text/plain gets a URL signed for one content type but an XHR with another, causing S3 to reject it with a signature mismatch. Pass the content type through the presign request, and do the same for multipart creation.
Useful? React with 👍 / 👎.
Comment on lines
+287
to
+291
| await this.#options.s3Client.createMultipartUpload( | ||
| this.#options.key, | ||
| this.#options.file.type || 'application/octet-stream', | ||
| this.#options.metadata, | ||
| ) |
There was a problem hiding this comment.
When a user removes or cancels a file while createMultipartUpload() is in flight, abort() runs before #uploadId is set, so it cannot call abortMultipartUpload; because this request also does not receive the abort signal, it can still succeed afterward and leave an active multipart upload in S3 that nothing cleans up. Pass the signal into the create/list/complete client calls and clean up if it fires.
Useful? React with 👍 / 👎.
qxprakash
commented
Jun 17, 2026
Comment on lines
+73
to
+78
| - name: Debug packaged uppy dependencies | ||
| run: | | ||
| rm -rf /tmp/uppy-debug | ||
| mkdir -p /tmp/uppy-debug | ||
| tar -xzf /tmp/artifacts/uppy-${{ github.sha }}.tgz --strip-components 1 -C /tmp/uppy-debug | ||
| node -e 'const pkg=require("/tmp/uppy-debug/package.json");const fields=["dependencies","peerDependencies","optionalDependencies","devDependencies"];let found=false;for(const f of fields){if(!pkg[f])continue;for(const [k,v] of Object.entries(pkg[f])){if(String(v).startsWith("workspace:")){console.log(`${f}: ${k}=${v}`);found=true;}}}if(!found)console.log("No workspace:* entries in packaged uppy package.json");' |
Collaborator
Author
There was a problem hiding this comment.
I'm not sure whether this change is related to this aws-s3-rewrite PR , I'll check this
Collaborator
Author
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores and modernizes the @uppy/aws-s3 implementation (per the referenced AWS S3 rewrite), introducing a new browser-friendly S3 client/signing flow, Golden Retriever resume support for multipart uploads, and expanded test coverage (including MinIO-based integration tests).
Changes:
- Replaces the legacy multipart implementation with a new
S3Uploader+s3-client/*stack (client-side signing via STS credentials, server-side signing viasignRequest, or Companion-backed signing). - Adds MinIO-backed integration tests (including Playwright browser tests) and CI wiring for
VITE_MINIO_CONFIG. - Updates examples and dev dashboards to use the new option naming (
companionEndpoint) and new signing flows.
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Passes VITE_MINIO_CONFIG through Turbo test tasks to support MinIO tests. |
| private/dev/Dashboard.js | Updates AwsS3 dev usage to companionEndpoint. |
| packages/@uppy/utils/src/fetcher.ts | Adjusts body defaulting behavior in XHR fetcher. |
| packages/@uppy/core/src/Uppy.ts | Fixes comment typos and adds s3Multipart placeholder on file state. |
| packages/@uppy/aws-s3/vitest.config.ts | Introduces multi-project Vitest config (jsdom + Playwright browser) and global setup for MinIO. |
| packages/@uppy/aws-s3/tests/test-utils/browser-crypto.ts | Adds WebCrypto-based randomBytes helper for browser contexts. |
| packages/@uppy/aws-s3/tests/s3-client/setup.ts | Adds MinIO Docker compose global setup/teardown for tests. |
| packages/@uppy/aws-s3/tests/s3-client/minio.test.ts | Adds MinIO integration test suite for S3mini + STS signing. |
| packages/@uppy/aws-s3/tests/s3-client/docker.ts | Adds Docker helpers for starting/stopping/exec’ing in compose services. |
| packages/@uppy/aws-s3/tests/s3-client/config.ts | Reads VITE_MINIO_CONFIG and provides test credentials/config. |
| packages/@uppy/aws-s3/tests/s3-client/compose.minio.yaml | Adds MinIO docker-compose definition used by tests. |
| packages/@uppy/aws-s3/tests/index.test.ts | Adds extensive unit tests for new AwsS3 behavior and Golden Retriever resume state. |
| packages/@uppy/aws-s3/src/utils.ts | Removes AwsBody type from utils (moved to main entry). |
| packages/@uppy/aws-s3/src/S3Uploader.ts | New uploader handling simple + multipart uploads and resume state persistence (s3Multipart). |
| packages/@uppy/aws-s3/src/s3-client/utils.ts | Adds XML parsing, SigV4 helpers, URL sanitizers, and custom error types. |
| packages/@uppy/aws-s3/src/s3-client/types.ts | Adds types for presignable requests and credential/signing APIs. |
| packages/@uppy/aws-s3/src/s3-client/signer.ts | Adds SigV4 presigner (with backward-compatible alias). |
| packages/@uppy/aws-s3/src/s3-client/S3mini.ts | Adds browser-capable S3 client supporting signRequest or getCredentials. |
| packages/@uppy/aws-s3/src/s3-client/S3Client.ts | Adds shared XHR + retry/offline handling base class. |
| packages/@uppy/aws-s3/src/s3-client/index.ts | Exports S3mini and related types. |
| packages/@uppy/aws-s3/src/s3-client/consts.ts | Adds shared constants and error messages for S3 client/signing. |
| packages/@uppy/aws-s3/src/s3-client/CompanionS3.ts | Adds Companion-backed S3 client implementation. |
| packages/@uppy/aws-s3/src/MultipartUploader.ts | Deletes legacy multipart uploader implementation. |
| packages/@uppy/aws-s3/src/index.ts | Rewrites AwsS3 plugin API/implementation to use new uploader + signing modes. |
| packages/@uppy/aws-s3/src/index.test.ts | Deletes legacy AwsS3Multipart tests (replaced by tests/index.test.ts). |
| packages/@uppy/aws-s3/src/HTTPCommunicationQueue.ts | Deletes legacy HTTP communication queue. |
| packages/@uppy/aws-s3/src/createSignedURL.ts | Deletes legacy presigning implementation. |
| packages/@uppy/aws-s3/src/createSignedURL.test.ts | Deletes legacy presigning tests. |
| packages/@uppy/aws-s3/package.json | Adds @aws-sdk/client-sts dev dependency. |
| examples/companion-digitalocean-spaces/README.md | Updates docs links from aws-s3-multipart to aws-s3. |
| examples/companion-digitalocean-spaces/main.js | Updates example option to companionEndpoint. |
| examples/aws-php/s3-sign.php | Updates PHP signer example to new signRequest shape and improves CORS handling. |
| examples/aws-php/main.js | Switches example from getUploadParameters to signRequest. |
| examples/aws-nodejs/routes/sts.js | Adds STS credentials endpoint for client-side signing example. |
| examples/aws-nodejs/routes/presign.js | Adds server-side presign endpoint for signRequest example. |
| examples/aws-nodejs/README.md | Updates documentation to describe both signing modes. |
| examples/aws-nodejs/public/withCustomEndpoints.html | Removes the older “custom endpoints” example page. |
| examples/aws-nodejs/public/index.html | Updates browser example UI + wiring for both signing modes + GoldenRetriever. |
| examples/aws-nodejs/index.js | Refactors Node example server to use the new routes/* and injects bucket/region into HTML. |
| examples/aws-companion/main.js | Updates example option to companionEndpoint. |
| .github/workflows/ci.yml | Adds VITE_MINIO_CONFIG to CI env. |
| .github/workflows/bundlers.yml | Adds debug step to ensure packaged uppy doesn’t ship workspace:* deps. |
| .github/CONTRIBUTING.md | Documents how to run MinIO-backed tests locally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+6
| import path from 'node:path' | ||
| import { getConfig } from './config' | ||
| import { composeDown, composeUpWait, execDockerCommand } from './docker' | ||
|
|
||
| const composeFile = path.join(__dirname, 'compose.minio.yaml') | ||
|
|
Comment on lines
+42
to
+48
| export async function teardown() { | ||
| if (config == null) return | ||
|
|
||
| console.log(`⏬ stopping minio image …`) | ||
| await composeDown(composeFile) | ||
| console.log(`✅ minio stopped and cleaned up`) | ||
| } |
Comment on lines
+28
to
+33
| const EIGHT_MB = 8 * 1024 * 1024 | ||
| const key_bin = 'test-multipart.bin' | ||
|
|
||
| const large_buffer = randomBytes(EIGHT_MB * 3.2) | ||
| const content = 'some content' | ||
| const key = 'first-test-object.txt' |
Comment on lines
+336
to
338
| #generateKey(file: UppyFile<M, B>): string { | ||
| return this.opts.generateObjectKey?.(file) ?? file.name | ||
| } |
Comment on lines
63
to
67
| const { | ||
| body = null, | ||
| body, | ||
| headers = {}, | ||
| method = 'GET', | ||
| onBeforeRequest = noop, |
Comment on lines
+8
to
+14
| { | ||
| test: { | ||
| name: 's3-jsdom', | ||
| include: ['**/*.test.ts'], | ||
| environment: 'jsdom', | ||
| }, | ||
| }, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.