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 vulnerabilities #8

Merged
merged 1 commit into from
Jul 29, 2022
Merged

fix vulnerabilities #8

merged 1 commit into from
Jul 29, 2022

Conversation

koto253
Copy link
Collaborator

@koto253 koto253 commented Jul 29, 2022

No description provided.

Copy link
Collaborator

@amyngb amyngb left a comment

Choose a reason for hiding this comment

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

Thanks for adding the Hello World part as well!

@amyngb amyngb merged commit c62d17e into dev Jul 29, 2022
@amyngb amyngb deleted the fix_vulnerabilities branch July 29, 2022 19:06
@readysetagile
Copy link
Collaborator

Was this tested??? I still see the message..

C:\repos\cbus-web>npm install
npm WARN deprecated [email protected]: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated
npm WARN deprecated [email protected]: This SVGO version is no longer supported. Upgrade to v2.x.x.

added 1441 packages, and audited 1444 packages in 15s

203 packages are looking for funding
  run `npm fund` for details

6 high severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.




C:\repos\cbus-web>npm audit
# npm audit report

nth-check  <2.0.1
Severity: high
Inefficient Regular Expression Complexity in nth-check - https://github.com/advisories/GHSA-rp65-9cf3-cjxr
fix available via `npm audit fix --force`
Will install [email protected], which is a breaking change
node_modules/svgo/node_modules/nth-check
  css-select  <=3.1.0
  Depends on vulnerable versions of nth-check
  node_modules/svgo/node_modules/css-select
    svgo  1.0.0 - 1.3.2
    Depends on vulnerable versions of css-select
    node_modules/svgo
      @svgr/plugin-svgo  <=5.5.0
      Depends on vulnerable versions of svgo
      node_modules/@svgr/plugin-svgo
        @svgr/webpack  4.0.0 - 5.5.0
        Depends on vulnerable versions of @svgr/plugin-svgo
        node_modules/@svgr/webpack
          react-scripts  >=2.1.4
          Depends on vulnerable versions of @svgr/webpack
          node_modules/react-scripts

6 high severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force




C:\repos\cbus-web>git show
commit c62d17ea79574ce8e4a8e392c9926325acb86d96 (HEAD -> dev, origin/dev, origin/HEAD)
Merge: aa077f7 a39841e
Author: Amy Bedinghaus <[email protected]>
Date:   Fri Jul 29 15:06:17 2022 -0400

    Merge pull request #8 from FCCColumbus/fix_vulnerabilities

    fix vulnerabilities

Simply running npm audit fix --force will not work. You will go into an infinite "fix" loop that will continually show vulnerabilities.

My research showed that this is caused by a known bug with nth-check. The workaround now is to put "nth-check": ">=2.0.1" in the dependencies section of package.json.

Unfortunately this bug has been around for months. No idea when it will be fixed.....

@amyngb
Copy link
Collaborator

amyngb commented Aug 2, 2022

@readysetagile My bad, I did not independently verify the fix. Just to clarify, is that one of the roles of the PR reviewer, to pull down the feature branch and manually test before merging? I know we don't currently have anyone in a QA role.

@koto253 are you still seeing the issue as well? See John's comment above about nth-check. We may need to create a new PR to fix this issue.

@readysetagile
Copy link
Collaborator

@amyngb up for discussion, however, my take is that the dev who is introducing the commit should at least check that the problem is solved before committing. We should establish these team agreements before going much farther. To your point, since we don't have any dedicated QA, we might want to also introduce a Peer Review with the Code Review. Netlify automatically creates an environment with the PR which would be nice in this situation. That's another discussion...

The other unique thing about this PR (not previously mentioned) is that it introduced 2 changes. This is dangerous, especially in this case because the second change "broke the build". You can see that with a npm run test that a variable is undefined.

@readysetagile
Copy link
Collaborator

Even though this is a issue/non-issue, we still need to keep on top of it, especially if we run automated tools to do security checks.

What I did was cherry-picked this commit, used the recommendations here, and got found 0 vulnerabilities. Let's try this and close this issue.

@readysetagile
Copy link
Collaborator

Looks like the latest build / audit has fixed this. closing this issue

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

Successfully merging this pull request may close these issues.

6 high severity vulnerabilities - Inefficient Regular Expression Complexity in nth-check
3 participants