Skip to content
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.

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

@uppy/companion: switch from aws-sdk v2 to @aws-sdk/client-s3 (v3) #4285

Merged
merged 10 commits into from
Jun 19, 2023

Conversation

scottbessler
Copy link
Contributor

Upgrade to the more space-effecient aws-sdk v3 s3 client (@aws-sdk/client-s3).

This knocks down the included node_modules from the 76.4mb aws-sdk to a more modest 14.8mb. And its not just disk space and file count, but it significantly lowers the amount that gets bundled when esbuilding something that uses @uppy/companion.

Copy link
Member

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Reading through https://github.com/aws/aws-sdk-js-v3/blob/main/UPGRADING.md to check all the versions we are using:

  • signatureVersion: is deprecated. We should probably remove it.
  • endpoint: no changes documented (except that HTTP may no longer be supported out of the box)
  • region: no change.
  • useAccelerateEndpoint: no changes
  • s3BucketEndpoint: changed to bucketEndpoint. We should update this.
  • credentials: no change (except I suppose the one you made in this PR).

Could you take care of the only two problematic items here?

@scottbessler
Copy link
Contributor Author

Could you take care of the only two problematic items here?

yup! pushed an update addressing those

@mifi
Copy link
Contributor

mifi commented Feb 7, 2023

unfortunately it's not that easy because s3 v3 client API is completely different from v2. in v3 they use the new Command class format, and many things have been pulled out into separate packages.

so this PR breaks all s3 code in https://github.com/transloadit/uppy/blob/main/packages/@uppy/companion/src/server/controllers/s3.js and https://github.com/transloadit/uppy/blob/main/packages/@uppy/companion/src/server/Uploader.js

there's about 8 different s3 api calls that need to be refactored. anyone up for grabs?:)

Note:

aws sdk v3 has a lot of quirks and undocumented breaking changes, so we will want to test this refactor thoroughly. For example, we need to:

@scottbessler
Copy link
Contributor Author

unfortunately it's not that easy because s3 v3 client API is completely different from v2. in v3 they use the new Command class format, and many things have been pulled out into separate packages.

ah bummer. i'd be game but honestly my bigger issue is that we dont use AWS at all and end up with the massive aws-sdk dep. perhaps it would be easier/better to modularize that into an peer/optional dep in some way. is there an appetite for that?

@mifi
Copy link
Contributor

mifi commented Feb 8, 2023

The team will discuss whether we can somehow modularize the upload destinations. Out of curiosity, why is it a problem to drag in unneeded dependencies? Security concerns?

@scottbessler
Copy link
Contributor Author

The team will discuss whether we can somehow modularize the upload destinations. Out of curiosity, why is it a problem to drag in unneeded dependencies? Security concerns?

aws-sdk is one of, if not the, largest folders under node_modules for us. this has effects on devenv, as well as CI/CD. for example it accounts for 20% of size when bundling our api server.

@aduh95 aduh95 self-assigned this May 24, 2023
@socket-security
Copy link

socket-security bot commented May 26, 2023

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue Package Version Note Source
Network access @aws-sdk/fetch-http-handler 3.353.0 packages/@uppy/companion/package.json via @aws-sdk/client-s3@3.354.0, @aws-sdk/lib-storage@3.354.0, @aws-sdk/s3-presigned-post@3.354.0
Network access @aws-sdk/credential-provider-imds 3.354.0 packages/@uppy/companion/package.json via @aws-sdk/client-s3@3.354.0, @aws-sdk/lib-storage@3.354.0, @aws-sdk/s3-presigned-post@3.354.0

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that isn't functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore @aws-sdk/fetch-http-handler@3.353.0
  • @SocketSecurity ignore @aws-sdk/credential-provider-imds@3.354.0

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label May 26, 2023
@github-actions github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label May 26, 2023
@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label May 26, 2023
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels May 26, 2023
@aduh95 aduh95 added safe to test Add this label on trustworthy PRs to spawn the e2e test suite pending end-to-end tests and removed pending end-to-end tests labels May 26, 2023
@github-actions github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label May 26, 2023
@socket-security
Copy link

socket-security bot commented Jun 10, 2023

New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives1 Size Publisher
@aws-sdk/s3-presigned-post 🆕 3.354.0 network, shell, environment +88 6.97 MB aws-sdk-bot
@aws-sdk/s3-request-presigner 🆕 3.354.0 None +22 823 kB aws-sdk-bot
@aws-sdk/client-s3 🆕 3.354.0 network, shell, environment +86 6.93 MB aws-sdk-bot
@aws-sdk/lib-storage 🆕 3.354.0 network, filesystem, shell +89 7.08 MB aws-sdk-bot

Footnotes

  1. https://docs.socket.dev

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Jun 10, 2023
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels Jun 10, 2023
@aduh95 aduh95 added safe to test Add this label on trustworthy PRs to spawn the e2e test suite and removed safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels Jun 19, 2023
@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Jun 19, 2023
@github-actions github-actions bot removed safe to test Add this label on trustworthy PRs to spawn the e2e test suite pending end-to-end tests labels Jun 19, 2023
@aduh95 aduh95 merged commit 2966574 into transloadit:main Jun 19, 2023
18 of 19 checks passed
@github-actions github-actions bot mentioned this pull request Jun 19, 2023
github-actions bot added a commit that referenced this pull request Jun 19, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.2.0 | @uppy/status-bar       |   3.2.0 |
| @uppy/aws-s3-multipart |   3.4.0 | @uppy/transloadit      |   3.1.6 |
| @uppy/companion        |   4.5.1 | @uppy/tus              |   3.1.1 |
| @uppy/core             |   3.2.1 | @uppy/url              |   3.3.2 |
| @uppy/dashboard        |   3.4.1 | @uppy/utils            |   5.4.0 |
| @uppy/golden-retriever |   3.0.4 | @uppy/xhr-upload       |   3.3.0 |
| @uppy/locales          |   3.2.2 | uppy                   |  3.10.0 |
| @uppy/provider-views   |   3.3.1 |                        |         |

- @uppy/aws-s3-multipart: fix the chunk size calculation (Antoine du Hamel / #4508)
- @uppy/aws-s3: add `shouldUseMultipart` option (Antoine du Hamel / #4299)
- @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (Scott Bessler / #4285)
- @uppy/companion,@uppy/core,@uppy/dashboard,@uppy/golden-retriever,@uppy/status-bar,@uppy/utils: Migrate all lodash' per-method-packages usage to lodash. (LinusMain / #4274)
- @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (Artur Paikin / #4507)
- @uppy/companion: revert randomness from file names (Mikael Finstad / #4509)
- @uppy/companion: Custom provider fixes (Mikael Finstad / #4498)
- @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (Antoine du Hamel / #4487)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/utils,@uppy/xhr-upload: When file is removed (or all are canceled), controller.abort queued requests (Artur Paikin / #4504)
- @uppy/provider-views: Fix range selection not resetting and computing correctly (Terence C / #4415)
- meta: disallow use of `.only` in tests (Antoine du Hamel / #4494)
- @uppy/companion: fix 500 when file name contains non-ASCII chars (Antoine du Hamel / #4493)
- @uppy/locales: update `fr_FR.js` (Samuel De Backer / #4499)
- @uppy/aws-s3-multipart,@uppy/tus,@uppy/xhr-upload: Don't close socket while upload is still in progress (Artur Paikin / #4479)
- meta: bump `luxon` from 1.28.0 to 1.28.1 (dependabot[bot] / #4497)
- @uppy/utils: rename `EventTracker` -> `EventManager` (Stephen Wooten / #4481)
- meta: bump cookiejar from 2.1.3 to 2.1.4 (dependabot[bot] / #4496)
- meta: make `pre-commit` use `corepack yarn` instead of `npm run` (Antoine du Hamel / #4495)
- meta: bump ua-parser-js from 0.7.31 to 0.7.35 (dependabot[bot] / #4474)
- meta: bump @sideway/formula from 3.0.0 to 3.0.1 (dependabot[bot] / #4473)
- meta: bump http-cache-semantics from 4.1.0 to 4.1.1 (dependabot[bot] / #4472)
- @uppy/companion: Use filename from content-disposition instead of relying on url, with fallback (Artur Paikin / #4489)
- meta: bump `babel`, `esbuild`, and `vite` (dependabot[bot] / #4485)
- @uppy/dashboard: include the old state when setting new (Artur Paikin / #4490)
- @uppy/companion: fix companion implicitpath (Mikael Finstad / #4484)
- @uppy/companion: fix undefined protocol and example page (Mikael Finstad / #4483)
- meta: upgrade Cypress 12.9.0 -> 12.14.0 (Antoine du Hamel / #4491)
- @uppy/core: remove `state` getter from types (Antoine du Hamel / #4477)
- examples/php-xhr: Added filename sanitation and file size check before saving (neuronet77 / #4432)
- examples/php-xhr: update PHP dependencies (dependabot[bot])
- @uppy/xhr-upload: add support for arrays in metadata (Vasiliy Matyushin / #4431)
- @uppy/status-bar: Filtered ETA (stduhpf / #4458)
- @uppy/aws-s3-multipart: fix `getUploadParameters` option (Antoine du Hamel / #4465)
Murderlon added a commit that referenced this pull request Jun 20, 2023
* main: (61 commits)
  Release: uppy@3.10.0 (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
  @uppy/companion: fix 500 when file name contains non-ASCII chars (#4493)
  @uppy/locales: update `fr_FR.js` (#4499)
  Don't close socket while upload is still in progress (#4479)
  meta: bump `luxon` from 1.28.0 to 1.28.1 (#4497)
  @uppy/utils: rename `EventTracker` -> `EventManager` (#4481)
  meta: bump cookiejar from 2.1.3 to 2.1.4 (#4496)
  meta: make `pre-commit` use `corepack yarn` instead of `npm run` (#4495)
  meta: bump ua-parser-js from 0.7.31 to 0.7.35 (#4474)
  ...
Murderlon added a commit that referenced this pull request Jun 20, 2023
* main:
  Release: uppy@3.10.0 (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
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.

None yet

3 participants