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

fix: package-lock v3 missing sub-dependencies - UNIFY-258 #605

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

neil-snyk
Copy link
Contributor

@neil-snyk neil-snyk commented Sep 16, 2024

NPM lock-file V3 - missing transitive dependencies

Links

JIRA Ticket

UNIFY-258

Background

Problem

A customer discovered that when scanning a container that contains an NPM lock-file that is V3 no transitive dependencies are identified, only the direct
dependencies.

Impact

Due to that all transitive dependencies are missed there is a risk of not identifying serious vulnerabilities in customers containers.

Root cause

The cause of the issue was that when creating the depgraph from npm and yarn lock-files the snyk-docker-plugin was first creating a deptree using Snyk's
nodejs-lockfile-parser which was then converted to a depgraph. The nodejs-lockfile-parser function that creates deptree's has a bug and does not generate
the transitive dependencies for V3 npm lock-files. As deptrees are large structures, resulting in high memory usage and high CPU costs from serializing,
it is better to create depgraphs directly instead.

Change

Now deptrees are only created for npm V1 lock-files, for all other versions of npm and yarn lock-files, the deptree creation is skipped and a depgraph is directly created.
The logic in snyk-docker-plugin was also modified to mimic the CLI when scanning stand-alone node applications so
that container always produces the same result for containers containing node applications.

Testing

Unit tests were added for a couple of new functions and function tests were also added to check that the correct number of dependencies are identified in containers with
v3 npm lock files and that the number of identified dependencies is the same in V1, V2 and V3. Manual testing was also done to compare the result of the current version
of the CLI and a locally built CLI with a version of the snyk-docker-plugin with the changes mentioned above. There it could be seen that the local build of the CLI
identified the correct number of dependencies in a V3 npm lock-file and that that results from scanning a V1 and V2 npm lockfile was unchanged. Also containers with
yarn lock-files still returned the correct number of dependencies after the changes in snyk-docker-plugin.

Impact

This change will only impact customers that have container projects that contain node applications that use V3 npm loc-files. These customers will get a bump in identified
dependencies due to that not only direct dependencies will be detected but also transitive dependencies. This could also lead to new vulnerabilities being reported, which could
lead to pipelines failing. The amount of newly found dependencies and vulnerabilities that will be identified is hard to predict, but for example the container that the
customer added to the ticket was identified as having nearly 10 times the amount of dependencies compared to before these changes.

Benefits

The benefit is that customers will correctly identify all dependencies in containers that contain V3 npm lock-files and thereby correctly identify all vulnerabilities.

Future considerations

This change could lead to snapshot churn and that a number of customers pipelines could break due to that vulnerabilities are found in the transitive dependencies that are returned
in the depgraph. How much depends on how widespread the use of V3 npm lock-files is among our customers, something we currently don't have a metric, this is something that perhaps
should be added in the future.

@neil-snyk neil-snyk requested a review from a team as a code owner September 16, 2024 14:05
@SteveShani
Copy link
Contributor

Is there more information you can add to the error message?

I'm worried we might get "Failed to build dep graph from current project" in a zendesk ticket one day and we'd have no idea how to go from there.

@neil-snyk neil-snyk force-pushed the feat/packagelockv3 branch 2 times, most recently from d8eb2d6 to b33d93d Compare September 18, 2024 08:14

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
switch from creating deptree's for containers containing node
projects that use package-lock V2 and above to creating
depgraphs directly.
@neil-snyk neil-snyk merged commit fc1a6bb into main Sep 18, 2024
17 checks passed
@neil-snyk neil-snyk deleted the feat/packagelockv3 branch September 18, 2024 13:39
@snyk-team-unify
Copy link

🎉 This PR is included in version 6.13.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants