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

S3 SDK has (too) many dependencies, and some optional peer dependencies which lead to errors / warnings, and some apparently contain vulnerabilities #4797

Open
3 tasks done
lukaselmer opened this issue Jun 6, 2023 · 7 comments
Labels
dependencies This issue is a problem in a dependency. feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue xl Effort estimation: very large

Comments

@lukaselmer
Copy link

lukaselmer commented Jun 6, 2023

Checkboxes for prior research

Describe the bug

When installing @aws-sdk/client-s3, 102 packages are installed. Some of them are not required in many scenarios IMO, e.g.

  • @aws-sdk/md5-js is available natively using nodejs
  • I suppose @aws-sdk/util-utf8-browser is not used on the server / in a node env
  • @aws-sdk/util-base64 is implemented in the browser
  • @aws-crypto/sha256-browser is used only in the browser, @aws-crypto/sha256-js is (probably?) only used on the server
  • @aws-crypto/ie11-detection is probably (hopefully!?) a thing of the past, and if it is used, it's only used in the browser
  • etc.

By installing so many dependencies, chances are that vulnerabilities are present. At the time of writing, fast-xml-parser is vulnerable (ok, this is extermely new, so it may not be a bit unfair to mention this).

Furthermore, when building a project with webpack, I get the following warnings / errors:

WARNING in ../../node_modules/@aws-sdk/signature-v4-multi-region/dist-es/SignatureV4MultiRegion.js 27:63-111
Module not found: Error: Can't resolve '@aws-sdk/signature-v4-crt' in '[...]/node_modules/@aws-sdk/signature-v4-multi-region/dist-es'
 @ ../../node_modules/@aws-sdk/signature-v4-multi-region/dist-es/index.js 1:0-41 1:0-41
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/runtimeConfig.shared.js 1:0-76 15:52-74
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/runtimeConfig.js 16:0-84 24:31-53
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/S3Client.js 15:0-73 18:26-44
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/index.js
 @ ./src/server/s3/deleteS3Object.ts 1:0-57 5:28-47
[...]

WARNING in ../../node_modules/@aws-sdk/util-user-agent-node/dist-es/is-crt-available.js 3:78-96
Module not found: Error: Can't resolve 'aws-crt' in '[...]/node_modules/@aws-sdk/util-user-agent-node/dist-es'
 @ ../../node_modules/@aws-sdk/util-user-agent-node/dist-es/index.js 4:0-52 14:25-39
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/runtimeConfig.js 15:0-65 33:12-28
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/S3Client.js 15:0-73 18:26-44
 @ ../../node_modules/@aws-sdk/client-s3/dist-es/index.js
 @ ./src/server/s3/deleteS3Object.ts 1:0-57 5:28-47
[...]

And yes, I've also noticed

but they don't really solve the core issue IMHO.

I don't know where these modules are used during production, but our tests / use cases apparently work without using these dependencies.

So now I have a lot of packages installed, I don't know which packages are used and which are unused, and our validation team, who validates these packages, have to validate more than 100 packages, and I expect that most of them are not even used. And we install packages in our server / CI, which are not even intended to be used on the server.

SDK version number

@aws-sdk/[email protected], see package-lock.json below for the full list

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v18.16.0

Reproduction Steps

npm init -y
npm i --save @aws-sdk/client-s3
npm audit
tree node_modules

Observed Behavior

(Too) Many dependencies are installed, some (most?) of them are unused.

There's a warning when compiling with webpack (missing peer dependencies). I don't know which features use these peer dependencies (missing docs), but apparently we don't use it.

Expected Behavior

Fewer packages are installed, all or almost all are used. I can install only what I need, and I need only a fraction of the features offered by the S3 sdk.

It is clearly documented which features use @aws-sdk/signature-v4-crt and aws-crt. Even better: this feature is extracted in a separate package, so if you don't need it, you don't install it.

Possible Solution

Do you think there's a better way to split up the sdk even more? E.g. separate a browser / server package, at least for the most popular packages like (I suppose) S3? Or have a "lite" version of some packages, which include the features used in 99% of the cases? And have a "extended" version of a package, which wrap the APIs that are only used in 1% of the libraries? Or convert more dependencies to peer dependencies, and describe which peer dependencies are used for which platforms / api calls? Or convert more peer dependencies, and create a meta package (e.g. @aws-sdk/client-s3-node) which requires the necessary peer dependencies?

I think it's a difficult problem, but there must be a better way! 🤔

Additional Information/Context

image image image
@lukaselmer lukaselmer added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 6, 2023
@basedwdev
Copy link

Commenting for visibility, seems is new as mentioned, and it present in many libraries [ses,sns, sqs, sts, credentials-provider].

fast-xml-parser vulnerable to Regex Injection via Doctype Entities - https://github.com/advisories/GHSA-6w63-h3fj-q4vw

https://snyk.io/advisor/npm-package/fast-xml-parser

@RanVaknin
Copy link
Contributor

Hi @lukaselmer ,

Thanks for opening this issue.
A lot of these dependencies are for compatibility with browser environment because Javascript is a language that is heavily used in web environments. The SDK has to support certain implementations out of the box for all environments and package size is a tradeoff when it comes to supporting a large subset of customers.

That being said, some of the dependencies you raised here can be removed, and others already have items on our roadmap to either unify, or remove altogether.

@baseddevblorp , The SDK team has already released a new version with an updated version of xml parser. The dependency has low risk of being exploited as the SDK uses https and "man in the middle" attacks are unlikely. Just pull the latest version of the SDK to mitigate this.

Thanks,
Ran~

@RanVaknin RanVaknin added p2 This is a standard priority issue needs-review This issue/pr needs review from an internal developer. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 11, 2023
@juvaly
Copy link

juvaly commented Jun 21, 2023

@RanVaknin what about the warnings that @lukaselmer shows in the original post? I can confirm these warnings as well, even though there is no real issue and everything works as intended in my project.

./node_modules/@aws-sdk/util-user-agent-node/dist-cjs/is-crt-available.js
Module not found: Can't resolve 'aws-crt' in '/Users/.../Documents/dev/.../node_modules/@aws-sdk/util-user-agent-node/dist-cjs'

Import trace for requested module:
./node_modules/@aws-sdk/util-user-agent-node/dist-cjs/is-crt-available.js
./node_modules/@aws-sdk/util-user-agent-node/dist-cjs/index.js
./node_modules/@aws-sdk/client-sts/dist-cjs/runtimeConfig.js
./node_modules/@aws-sdk/client-sts/dist-cjs/STSClient.js
./node_modules/@aws-sdk/client-sts/dist-cjs/index.js
...

@summaarum
Copy link

summaarum commented Jun 21, 2023

@RanVaknin Having the same issue Module not found: Can't resolve '@aws-sdk/signature-v4-crt'. Locally everything works as expected but deployment is failing because my build tool has strict error checking. Seems this issue is coming up again and again. Please help

@faizjamil
Copy link

faizjamil commented Aug 30, 2023

Can conform this (or a similar) webpack error(s) still shows up on @aws-sdk/client-s3 v3.400.0

WARNING in ./node_modules/@aws-sdk/signature-v4-multi-region/dist-es/SignatureV4MultiRegion.js 27:63-111
Module not found: Error: Can't resolve '@aws-sdk/signature-v4-crt' in '[...]/node_modules/@aws-sdk/signature-v4-multi-region/dist-es'
Did you miss the leading dot in 'resolve.extensions'? Did you mean '[".*",".js"]' instead of '["*",".js"]'?
 @ ./node_modules/@aws-sdk/signature-v4-multi-region/dist-es/index.js 1:0-41 1:0-41
 @ ./node_modules/@aws-sdk/client-s3/dist-es/runtimeConfig.shared.js 1:0-76 19:52-74
 @ ./node_modules/@aws-sdk/client-s3/dist-es/runtimeConfig.js 15:0-84 23:31-53
 @ ./node_modules/@aws-sdk/client-s3/dist-es/S3Client.js 15:0-73 20:26-44
 @ ./node_modules/@aws-sdk/client-s3/dist-es/index.js 1:0-27 1:0-27
 @ ./routes/api.js 8:0-54 141:35-51
 @ ./index.js 6:0-37 16:29-38

WARNING in ./node_modules/@aws-sdk/util-user-agent-node/dist-es/is-crt-available.js 3:78-96
Module not found: Error: Can't resolve 'aws-crt' in '[...]/node_modules/@aws-sdk/util-user-agent-node/dist-es'
Did you miss the leading dot in 'resolve.extensions'? Did you mean '[".*",".js"]' instead of '["*",".js"]'?
 @ ./node_modules/@aws-sdk/util-user-agent-node/dist-es/index.js 4:0-52 15:25-39
 @ ./node_modules/@aws-sdk/client-s3/dist-es/runtimeConfig.js 5:0-65 32:12-28
 @ ./node_modules/@aws-sdk/client-s3/dist-es/S3Client.js 15:0-73 20:26-44
 @ ./node_modules/@aws-sdk/client-s3/dist-es/index.js 1:0-27 1:0-27
 @ ./routes/api.js 8:0-54 141:35-51
 @ ./index.js 6:0-37 16:29-38

@ajredniwja ajredniwja added feature-request New feature or enhancement. May require GitHub community feedback. dependencies This issue is a problem in a dependency. and removed needs-review This issue/pr needs review from an internal developer. labels Sep 12, 2023
@RanVaknin RanVaknin added the queued This issues is on the AWS team's backlog label Feb 14, 2024
@kuhe kuhe added xl Effort estimation: very large and removed bug This issue is a bug. queued This issues is on the AWS team's backlog labels Jul 2, 2024
@RanVaknin RanVaknin removed their assignment Aug 5, 2024
@nileshtrivedi
Copy link

Our system reported a vulnerability in fast-xml-parser:

fast-xml-parser vulnerable to ReDOS at currency parsing

Summary: A ReDOS exists on currency.js was discovered by Gauss Security Labs R&D team.
Details : https://github.com/NaturalIntelligence/fast-xml-parser/blob/master/src/v5/valueParsers/currency.js#L10 contains a vulnerable regex PoC
pass the following string '\t'.repeat(13337) + '.'

Impact: Denial of service during currency parsing in experimental version 5 of fast-xml-parser-library

The patched version is 4.4.1 but aws-sdk is bringing in a vulnerable version 4.2.5.

@nileshtrivedi
Copy link

GitHub has an advisory for it: GHSA-mpg4-rc92-vx8v

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies This issue is a problem in a dependency. feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue xl Effort estimation: very large
Projects
None yet
Development

No branches or pull requests

10 participants