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] fix all cve warnings #138

Merged
merged 5 commits into from
Nov 3, 2023
Merged

[fix] fix all cve warnings #138

merged 5 commits into from
Nov 3, 2023

Conversation

allansson
Copy link
Contributor

This PR fixes all the CVE warnings when running ´trivyandnpm audit` on the repo.

The critical CVE was easy to fix. We just needed to upgrade babel to the latest version and it went away.

The other warnings were caused by an old version of ava, and that proved to be trickier to fix. The old version had support for mixing ESM and CJS, but that was dropped in favor of native esm support in node. But that has some problems:

  1. NODE_PATH doesn't work with ESM, so all paths would have to be converted to relative in the test suite.
  2. Node detects the module type based on the file extension and the type property in package.json: if type is "module" CJS modules need to have the extension .cjs and vice versa.

In the end, the simplest solution was to just convert all the import statements in the test suite to require statements.

How to test

  1. Install trivy
  2. Run trivy fs . --include-dev-deps in the repository
  3. Run npm audit in the reposity
  4. Run the CLI on some HAR file

Expected result

There should be no CVEs.

@allansson allansson requested a review from a team as a code owner October 30, 2023 15:22
@allansson allansson requested review from going-confetti, 2Steaks and w1kman and removed request for a team and going-confetti October 30, 2023 15:22
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

CR: I'm a bit sceptical against installing "D".

FYI: Since we are pushing a docker image on build (which is scanned by Grafana using trivy). I ran trivy image har-to-k6:local, which resulted in Total: 57 (UNKNOWN: 0, LOW: 50, MEDIUM: 5, HIGH: 1, CRITICAL: 1)

Not asking you to fix the image vulnerabilities, just pointing out that they are subjected to show up when grafana scans the docker image.

package.json Outdated
@@ -29,6 +29,7 @@
"bundle-collapser": "^1.3.0",
"caporal": "1.0.0",
"chalk": "^2.4.2",
"D": "^1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?
This package has been deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A guess: npm install D 😅

Copy link
Contributor Author

@allansson allansson Oct 31, 2023

Choose a reason for hiding this comment

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

I've updated the Dockerfile to use node:20-alpine to minimize the CVEs to 2 medium ones. It also shrinks the image by 60 MB.

D has been removed.

I also noticed a warning that babel-eslint was no longer updated, so I switched to @babel/eslint-parser to make any future updates easier.

@allansson allansson requested a review from w1kman October 31, 2023 08:52
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

LGTM 👌🏻

Copy link

@2Steaks 2Steaks left a comment

Choose a reason for hiding this comment

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

Tested with success and no warnings 👍

@allansson allansson merged commit b773f7d into master Nov 3, 2023
1 check passed
@allansson allansson deleted the fix/cves branch November 3, 2023 11:37
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.

3 participants