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

lukaselmer commented 1 year ago

Checkboxes for prior research

[X] I've gone through Developer Guide and API reference
[X] I've checked AWS Forums and StackOverflow.
[X] I've searched for previous similar issues and didn't find any solution.

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/client-s3@3.347.0, 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

basedwdev commented 1 year ago

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 commented 1 year ago

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~

juvaly commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

nileshtrivedi commented 3 months ago

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 commented 3 months ago

GitHub has an advisory for it: https://github.com/advisories/GHSA-mpg4-rc92-vx8v

aws / aws-sdk-js-v3