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

Override transitive semver dependency version to fix security vulnerability #604

Merged
merged 6 commits into from
Jul 13, 2023

Conversation

carolabadeer
Copy link
Contributor

Issue #, if available: #599

Description of changes: Use npm-force-resolutions to override the vulnerable semver dependency v5.7.1 being installed from cls-hooked v4.2.2. It is necessary to manually override the semver version as the cls-hooked package has not issued a release since 2017.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@carolabadeer carolabadeer requested a review from a team as a code owner July 13, 2023 20:50
@@ -3,6 +3,9 @@
"version": "3.5.0",
"private": true,
"license": "Apache-2.0",
"resolutions": {
"semver": "7.5.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering why is not

"resolutions": {
    "cls-hooked" : {
        "semver": "7.5.3"
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that initially but it seems that npm-force-resolutions doesn't support specifying the direct dependency -> transitive dependency. When I ran lerna bootstrap --hoist to install, the package-lock.json was not updated correctly in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know

@srprash
Copy link
Contributor

srprash commented Jul 13, 2023

How do we ensure that cls-hooked works with the "forced" v7.5.3 of semver in our SDK and not behave unexpectedly?

@carolabadeer
Copy link
Contributor Author

carolabadeer commented Jul 13, 2023

@srprash cls-hooked only uses semver in one line to utilize the gte function, and the module that exports the gte function in the semver repository has not had any changes in the last 4 years. So the single usage of the semver.gte() function in cls-hooked would behave the same for the current version in cls-hooked (5.7.1) and the forced version we are using (7.5.3) since 5.7.1 was released 4 years ago.

An additional note:
There are also many others who reported this vulnerability directly in the cls-hooked package, and some contributors even created PRs for upgrading the semver version to 7.5.3, but since the library is unmaintained, those PRs have not been addressed. Those authors of those issues and PRs report manually overriding semver version did not cause any issues and resolved the security risk: Jeff-Lewis/cls-hooked#78

@carolabadeer carolabadeer merged commit 3cab7bc into aws:master Jul 13, 2023
13 checks passed
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