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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulnerable dependencies in 1.1.4 #4374

Closed
rdebeasi opened this Issue Apr 27, 2018 · 16 comments

Comments

Projects
None yet
7 participants
@rdebeasi

rdebeasi commented Apr 27, 2018

Version 1.1.4 (the latest version as of this writing) has dependencies with known security vulnerabilities. Thank you in advance for looking into this! 馃槃

Is this a bug report?

Yes

Did you try recovering your dependencies?

Yes

Which terms did you search for in User Guide?

security, vulnerability, hoek

Environment

  1. node -v: v8.11.1
  2. npm -v: 6.0.0
  3. yarn --version (if you use Yarn): N/A
  4. npm ls react-scripts (if you haven鈥檛 ejected):
ganymede@0.1.0 /home/rdebeasi/Projects/ganymede
鈹斺攢鈹 react-scripts@1.1.4 
  1. Operating system: Fedora 27
  2. Browser and version (if relevant): N/A

Steps to Reproduce

  1. Test react-scripts 1.1.4 on Snyk
  2. Create a new project with create-react-app
  3. Run npm install
  4. Run npm ls hoek

Expected Behavior

  • Snyk finds no vulnerabilities in create-react-app.
  • react-scripts relies on a version of hoek newer than 5.0.3 or 4.2.1.

Actual Behavior

  • Snyk finds 2 medium severity vulnerabilities and 4 low severity vulnerabilities.
  • react-scripts relies on hoek 4.2.1, which is affected by CVE-2018-3728. (I noticed this issue because GitHub flagged hoek as a vulnerability in my create-react-app project.)
[rdebeasi@rdebeasi ganymede]$ npm ls hoek
ganymede@0.1.0 /home/rdebeasi/Projects/ganymede
鈹斺攢鈹 react-scripts@1.1.4
  鈹斺攢鈹 jest@20.0.4
    鈹斺攢鈹 jest-cli@20.0.4
      鈹斺攢鈹 jest-environment-jsdom@20.0.3
        鈹斺攢鈹 jsdom@9.12.0
          鈹斺攢鈹 request@2.85.0
            鈹斺攢鈹 hawk@6.0.2
              鈹溾攢鈹 boom@4.3.1
              鈹 鈹斺攢鈹 hoek@4.2.1  deduped
              鈹溾攢鈹 cryptiles@3.1.2
              鈹 鈹斺攢鈹 boom@5.2.0
              鈹   鈹斺攢鈹 hoek@4.2.1  deduped
              鈹溾攢鈹 hoek@4.2.1 
              鈹斺攢鈹 sntp@2.1.0
                鈹斺攢鈹 hoek@4.2.1  deduped

See also "Security vulnerability: hoek" in the Jest repo

Reproducible Demo

N/A

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 27, 2018

This dependency is only used in tests so I don't think it's practically relevant.

@rdebeasi

This comment has been minimized.

rdebeasi commented Apr 27, 2018

Makes sense. Thanks for the quick reply!

For what it's worth, a couple of those vulnerabilities are introduced through webpack-dev-server. This is still of course an issue that shouldn't affect production environments, but I bet there are at least a few people out there running the dev server in prod. :)

@rdebeasi

This comment has been minimized.

rdebeasi commented Apr 27, 2018

Quick update: it sounds like the vulnerability report on hoek 4.2.1 is a false positive. The issues in the Snyk test do seem to be legitimate, though.

@iddan

This comment has been minimized.

iddan commented May 16, 2018

Snyk is reporting macaddress is vulnerable as well. Are you aware of it? https://snyk.io/test/npm/create-react-app

@gaearon

This comment has been minimized.

Member

gaearon commented May 16, 2018

I don't see anything there.

@iddan

This comment has been minimized.

iddan commented May 16, 2018

Oops wrong link. Here is the correct one: https://snyk.io/test/npm/react-scripts/1.1.4 - can you see it now?

@pkrawc

This comment has been minimized.

pkrawc commented May 19, 2018

It would seem macaddress is the culprit, which is a dependency of cssnano which is a dependency of css-loader. Looks like there's a fix for cssnano already so updating dependencies should work here.

@pkrawc

This comment has been minimized.

pkrawc commented May 19, 2018

To be fair, this whole thing reeks of code smell. Why cssnano needed a unique id package when one can be written two dozen characters is beyond me.

@iddan

This comment has been minimized.

iddan commented May 19, 2018

Because common functionality should be abstracted. In scale small highly shared utils subtract source size significantly and allow sharing fixes to common problems be shared easily

@gaearon

This comment has been minimized.

Member

gaearon commented May 19, 2018

If you look at the vulnerability description, it says it only matters if the outside code has control over the argument (which it doesn鈥檛). So again, this doesn鈥檛 affect us in any way.

@gaearon

This comment has been minimized.

Member

gaearon commented May 20, 2018

As I have explained in #4479 (comment) there鈥檚 no actual vulnerability you鈥檙e being affected here.

@dannypurcell

This comment has been minimized.

dannypurcell commented May 22, 2018

Not exactly inspirational for a newcomer though. Need a way to either upgrade past it or silence the warning.

@gaearon

This comment has been minimized.

Member

gaearon commented May 22, 2018

Do you think we're happy this is the case? 馃槈 It's just as annoying to me to keep responding to five different threads about it, as it is to you to see a message like this.

I don't know what to suggest to you. We didn't turn these warnings on. Either you did it, or npm did it by default. (I don't know which one is the case.)

We can't fix it without the downstream dependency updating. When this happens, we'll happily cut a patch. You can help too!

@extempl

This comment has been minimized.

extempl commented May 24, 2018

I have same with macaddress:

鈹屸攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹
鈹 critical      鈹 Command Injection                                            鈹
鈹溾攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹尖攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹
鈹 Package       鈹 macaddress                                                   鈹
鈹溾攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹尖攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹
鈹 Dependency of 鈹 react-scripts                                                鈹
鈹溾攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹尖攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹
鈹 Path          鈹 react-scripts > css-loader > cssnano >                       鈹
鈹               鈹 postcss-filter-plugins > uniqid > macaddress                 鈹
鈹溾攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹尖攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹
鈹 More info     鈹 https://nodesecurity.io/advisories/654                       鈹
鈹斺攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹粹攢鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹鈹

Package was updated 3 years ago.

We didn't turn these warnings on. Either you did it, or npm did it by default.

That's right, npm added npm audit and returns vulnerability report on npm install.

If you look at the vulnerability description, it says it only matters if the outside code has control over the argument (which it doesn鈥檛). So again, this doesn鈥檛 affect us in any way.

In many companies npm audit or nsp check (before) used in deployment flow to block deployment if vulnerabilities was found. It is not always possible to switch checking off or allow deployment with a failed check.

@gaearon

This comment has been minimized.

Member

gaearon commented May 24, 2018

We鈥檙e happy to take a pull request that updates the dependency or switches it. It might be that you鈥檒l need to send it to a few underlying packages.

I don鈥檛 personally have the time to work on this right now. Are you willing to help out since it was your company that enabled these checks and is affected by the false positives?

@bugzpodder

This comment has been minimized.

Collaborator

bugzpodder commented Jun 1, 2018

both hoek and macaddress are no longer present in react-scripts@1.1.4 and @next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment