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

Find and fix vulnerable npm packages #8

Merged
merged 12 commits into from
Aug 4, 2024

Conversation

timtebeek
Copy link
Contributor

@timtebeek timtebeek commented Aug 3, 2024

What's changed?

Add a recipe to Find and fix vulnerable npm packages

Anything in particular you'd like reviewers to focus on?

  1. Do the test input and output align with what we hope to see? ^ versions specifically.
  2. Search markers are not yet implemented; do we need them for v1? That would show .minor and .major CVEs in code that now only show up in data table rows
  3. Like DependencyInsight this for now assumes a single package-lock.json. Should we support multiple?

Anyone you would like to review specifically?

@zieka

Have you considered any alternatives or workarounds?

  • This copies elements from rewrite-java-dependencies, in particular how we parse the GitHub advisories database. In the copy here we parse for the NPM ecosystem. Seemed odd to parse these in rewrite-java-dependencies, unless we rename that rewrite-dependencies.
  • Significant changes as to the DependencyVulnerabilityCheck for Java, producing different data table rows sans groupId/artifactId split, and without dependency depth, due to lacking data. This will require new visualizations, and separate recipe runs, but seemed a stretch to "make it fit" the existing model in rewrite-java-dependencies.

@timtebeek timtebeek self-assigned this Aug 3, 2024
@timtebeek
Copy link
Contributor Author

If we want to reduce the duplication, I've now made the ecosystem of the GitHub security advisory database configurable in

We could then push that file here from that repository, as we also do with a few other deploy keys such as for our docs.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link

@pstreef pstreef left a comment

Choose a reason for hiding this comment

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

Approved with some minor comments.
edit: Oh I guess this is still in draft. Feel free to re-request 😅

@timtebeek timtebeek marked this pull request as ready for review August 4, 2024 13:11
@timtebeek timtebeek merged commit d52c381 into main Aug 4, 2024
2 checks passed
@timtebeek timtebeek deleted the find-and-fix-vulnerable-npm-packages branch August 4, 2024 13:20
kmccarp pushed a commit to kmccarp/rewrite-nodejs that referenced this pull request Aug 22, 2024
* Parse vulnerabilities into local csv file

* Detect and bump NPM dependency versions

* Some minor comments to document what's done

* Parse a single advisory file in tests due to ordering

* Additional tests to document current behavior

* Find all vulnerabilities, but only bump patch versions

* Minor polish to get rid of warnings

* Apply suggestions from code review

Co-authored-by: Shannon Pamperl <[email protected]>

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Keep parsing advisories in rewrite-java-dependencies

---------

Co-authored-by: Shannon Pamperl <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants