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

Critical vulnerability in socket.io-parser #801

Closed
jattasNI opened this issue Nov 2, 2022 · 4 comments
Closed

Critical vulnerability in socket.io-parser #801

jattasNI opened this issue Nov 2, 2022 · 4 comments
Assignees
Labels
discussion Questions, conversations, or announcements tech debt

Comments

@jattasNI
Copy link
Contributor

jattasNI commented Nov 2, 2022

🧹 Tech Debt

npm audit started failing due to a new critical vulnerability in socket.io-parser. This blocks the main PR workflow.

The package is pulled in by karma and @11y/eleventy:

npm ls socket.io-parser

├─┬ @ni/[email protected] -> ./packages/nimble-components
│ └─┬ [email protected]
│   └─┬ [email protected]
│     └── [email protected]
└─┬ @ni/[email protected] -> ./packages/site
  └─┬ @11ty/[email protected]
    └─┬ [email protected]
      ├─┬ [email protected]
      │ └─┬ [email protected]
      │   └── [email protected]
      └─┬ [email protected]
        └── [email protected]
@jattasNI jattasNI added tech debt triage New issue that needs to be reviewed labels Nov 2, 2022
jattasNI added a commit that referenced this issue Nov 2, 2022
# Pull Request

## 🤨 Rationale

As described in #801 we have a vulnerability that is causing `npm audit`
to fail the PR build, preventing submissions.

## 👩‍💻 Implementation

Comment out the audit step that's failing. We'll use the bug linked
above to track re-enabling it.

## 🧪 Testing

PR build

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Nov 8, 2022
@jattasNI jattasNI moved this from Backlog to In progress in Nimble Design System Priorities Mar 7, 2024
@rajsite
Copy link
Member

rajsite commented Mar 7, 2024

@fredvisser does Snyk replace the functionality that npm audit provided? If so then the action for this issue is to remove the audit steps from main.yml. Marking as "discussion" to see if we can make this issue go away.

@rajsite rajsite added discussion Questions, conversations, or announcements triage New issue that needs to be reviewed and removed discussion Questions, conversations, or announcements labels Mar 7, 2024
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Mar 12, 2024
@fredvisser
Copy link
Contributor

Snyk doesn't support monorepos by default, so isn't picking up any of the package specific package.json files. So the current setup isn't good enough.
While they have a CLI flag that will find the other package.json files, we're using a web hook which doesn't appear to expose the option.

We could switch to a their Github Action or we could add the Github Dependency Review Action which would keep the workflow entirely with Github.

@fredvisser fredvisser added the discussion Questions, conversations, or announcements label Mar 13, 2024
rajsite pushed a commit that referenced this issue Mar 26, 2024
# Pull Request

## 🤨 Rationale

#801 highlighted that our current use of `npm audit` is brittle. This
action should elevate any issues tracked by Github to the PR.

## 👩‍💻 Implementation

- Use the example config file from
https://github.com/actions/dependency-review-action?tab=readme-ov-file
- I'll remove the Snyk integration when this PR goes in

## 🧪 Testing

This PR only highlights the packages that are changed (the Github
Actions), but since the [Github dependency
graph](https://github.com/ni/nimble/network/dependencies) sees our other
NPM dependencies, I expect this will evaluate package.json issues when a
PR has those changes in it.

I could add known bad issues to this PR to validate, or we could just
submit this and validate over time.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@rajsite
Copy link
Member

rajsite commented Mar 26, 2024

Fixed in #1930

@rajsite rajsite closed this as completed Mar 26, 2024
@rajsite rajsite reopened this Mar 26, 2024
@rajsite
Copy link
Member

rajsite commented Mar 26, 2024

@fredvisser looking at the dependency review check instructions it says to check the following page: https://github.com/ni/nimble/security/dependabot

where there I see non-dismissed critical vulns, but it doesn't look like dependency review check is complaining about those, see build:
https://github.com/ni/nimble/actions/runs/8439038563

and log:
https://github.com/ni/nimble/actions/runs/8439038563/job/23112684298#step:3:8

Edit: Ohhh, it maybe is only detecting dependency changes based on the README: https://github.com/actions/dependency-review-action?tab=readme-ov-file#dependency-review-action

So that's great and it might be working fine. I'll re-close the issue

@rajsite rajsite closed this as completed Mar 26, 2024
@m-akinc m-akinc moved this from In progress to Done in Nimble Design System Priorities Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Questions, conversations, or announcements tech debt
Projects
Development

No branches or pull requests

4 participants