Dependency on insecure version of braces (Node security advisory 786)

[yhoiseth]

Is this a bug report?

Yes.

Did you try recovering your dependencies?

I don't think this step is necessary, due to the error being present in a brand new project.

Which terms did you search for in User Guide?

None.

Environment

Environment Info:

  System:
    OS: macOS 10.14.3
    CPU: x64 Intel(R) Core(TM) i5-4278U CPU @ 2.60GHz
  Binaries:
    Node: 10.15.0 - /usr/local/opt/node@10/bin/node
    Yarn: 1.13.0 - /usr/local/bin/yarn
    npm: 6.4.1 - /usr/local/opt/node@10/bin/npm
  Browsers:
    Chrome: 72.0.3626.109
    Firefox: 65.0
    Safari: 12.0.3
  npmPackages:
    react: ^16.8.2 => 16.8.2 
    react-dom: ^16.8.2 => 16.8.2 
    react-scripts: 2.1.5 => 2.1.5 
  npmGlobalPackages:
    create-react-app: Not Found

Steps to Reproduce

1. yarn create react-app my-app
2. cd my-app/
3. yarn audit

In addition, I've tried to add braces as a top-level dependency using yarn add braces. That didn't help.

Expected Behavior

Pass.

Actual Behavior

Fail:

➜  my-app git:(master) yarn audit
yarn audit v1.13.0
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ low           │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ braces                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.3.1                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ react-scripts                                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ react-scripts > babel-jest > babel-plugin-istanbul >         │
│               │ test-exclude > micromatch > braces                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/786                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
…
63 vulnerabilities found - Packages audited: 36332
Severity: 63 Low
✨  Done in 3.12s.

Reproducible Demo

I don't think this is necessary, due to the required steps being very few.

[bugzpodder]

This is a development only package and it should not affect your production build in anyway. It should go away once a newer version of a downstream package that addresses this issue is available.

[sbcreates]

Getting the same vulnerabilities from npm audit regarding braces.

The vulnerability is in versions earlier than 2.3.1 and it seems react-scripts has a nested dependency that uses an earlier version of braces

app
├─┬ http-proxy-middleware@0.19.0
│ └─┬ micromatch@3.1.10
│   └── braces@2.3.2 
└─┬ react-scripts@2.1.5
  ├─┬ babel-jest@23.6.0
  │ └─┬ babel-plugin-istanbul@4.1.6
  │   └─┬ test-exclude@4.2.3
  │     └─┬ micromatch@2.3.11
  │       └── braces@1.8.5 
  ├─┬ fork-ts-checker-webpack-plugin-alt@0.4.14
  │ └─┬ chokidar@2.1.1
  │   └── braces@2.3.2  deduped
  └─┬ jest@23.6.0
    └─┬ jest-cli@23.6.0
      ├─┬ jest-config@23.6.0
      │ └─┬ micromatch@2.3.11
      │   └── braces@1.8.5 
      ├─┬ jest-haste-map@23.6.0
      │ └─┬ micromatch@2.3.11
      │   └── braces@1.8.5 
      ├─┬ jest-message-util@23.4.0
      │ └─┬ micromatch@2.3.11
      │   └── braces@1.8.5 
      ├─┬ jest-runtime@23.6.0
      │ └─┬ micromatch@2.3.11
      │   └── braces@1.8.5 
      └─┬ micromatch@2.3.11
        └── braces@1.8.5 

More information on the issue is here.

Do we wait for react-scripts to update the dependency in question?

[bugzpodder]

Unless jest releases a new 23.x version with updated dependencies to micromatch@3 (unlikely since this is a breaking change), or CRA updates to jest 24, I don't think there is an easy workaround for this issue.

[sorahn]

jestjs/jest#7917

As it stands right now, Jest is not planning on releasing a patch for v23.

[alexch]

See #6064 #6109 #5777 #5618 for earlier times something like this happened; I think it's useful to draw the connection between these recurring issues.

#6064 (comment) explains the reasoning behind this project's tradeoff between dependency pessimism (pinning, delayed upgrades, and enduser frustration) and dependency optimism (allow users to use npm audit fix to apply upgrades with ^ or ~, but remain exposed to the risk that a nested dependency might push a patch upgrade that contains a hidden bug) and give readers of this current issue a pointer to previous discussions and explanations.

I do see that in this case, Jest fixed the issue with a major version bump, not a patch, so even a SemVer "minors and patches OK" "babel-jest": "^23.6.0" wouldn't have helped this time; >=23.6.0 would but that's probably too optimistic (even for me :-) ).

This is a development only package and it should not affect your production build in anyway.

@bugzpodder That is a very confident prediction! Unfortunately, in fact, I can't push to production via my CI pipeline when my tests fail, and this problem causes react-scripts test to fail (since my React client directory is nested under a project root directory containing a NodeJS server, and the server's dependencies are unpinned, so the server got Jest 24.x smoothly via npm audit fix). We're not in active release mode right now, so I'm not personally affected by this particular glitch, but I'm confident there are others who are.

UPDATE: For my nested-node-project setup, I had to add SKIP_PREFLIGHT_CHECK=true to a .env file in the child project dir to allow Jest 23.6.0 in the client to peacefully coexist with Jest 24.1.0 in the server.

ALSO: I know open source development can be a thankless task, so BIG THANKS to everyone who's working on all these interrelated projects and the tangled dependency webs they weave.

[SimenB]

FWIW we originally bumped Micromatch in a minor of Jest 23, but it broke a ton of setups (notably on windows), so we reverted it: jestjs/jest#6661

[bugzpodder]

@alexch, your production build (i.e. running react-scripts build) will not be affected by this security vulnerability in any way. You'll get a notice about this low risk vulnerability if you run npm install, but otherwise an end user should not be affected by this issue. There is no need to rush upgrading to Jest 24.

[Tasemu]

This is causing issues with us being able to deploy our front-end through our CI also. Going to keep an eye out here for any movement. :)

[SimenB]

Nothing will happen here unless a patch is released for an old version of micromatch or a new major of CRA is released. Using a newer major of micromatch is a breaking change.

Seems like your CI should just check production dependencies, not dev deps. Doesn't seem like npm supports it, though 🤷‍♂️

https://npm.community/t/please-support-production-or-only-production-in-npm-audit/3005
npm/rfcs#18

[aboutlo]

WORKAROUND

npm audit --registry=https://registry.npmjs.org --parseable | wc -l
returns the number of vulnerabilities found

you can exclude low vulnerabilities with
npm audit --registry=https://registry.npmjs.org --parseable | grep -V https://nodesecurity.io/advisories/786 | wc -l

[naugtur]

Instead of using a workaround (which ignores the advisory in all modules you installed) you can use https://www.npmjs.com/package/npm-audit-resolver that wraps audit in fine-grained tools to manage your security decision

[jdalegonzalez]

@bugzpodder

This is a development only package and it should not affect your production build in anyway. It should go away once a newer version of a downstream package that addresses this issue is available.

I've seen messages like this a number of times for a variety of reported security vulnerabilities in various packages and it's not a particularly satisfying response for me but maybe I'm thinking about things the wrong way.

Is the expected flow:

• see reported security vulnerability in npm (or yarn).
• lookup that flow
• attempt to determine if the vulnerability will impact the product env or not
• if it's determined it won't, come up with some workaround that suppresses that message from the log
• if it's determined it will, post a bug

That flow seems really likely to allow something that wasn't expected to cause a production issue to end up causing one.

I had been operating under the expectation that the flow was:

• see reported security vulnerability
• immediately attempt to implement a fix - without worrying about whether or not it was expected to cause a prod issue.
• end of flow

Under that flow, we're less likely to let flaws creep into our projects but it means responses like "it only impacts dev" or "it's only a command-line thing" are insufficient.

Maybe the issue with with npm and yarn? Maybe they should have some sort of "dev only, prod and dev" flags or "mark as 'ok for me and don't tell me again'" like other vuln scanners have?

[alexch]

@bugzpodder If react-scripts build were my entire process for doing a production build, I would agree with you. But my process for development and deployment also includes running a suite of unit and acceptance tests; to make them work I need to disable preflight checks... which is fine temporarily, but that allows other dependency issues to creep in, and is also one more thing to remember to undo later.

@jdalegonzalez I think you're right that different teams/projects have different workflow expectations, and different levels of reliance on so-called "dev-only" packages, and that your latter flow ("apply fix ASAP") is much preferred over a flow that requires every project owner to deeply understand vulnerabilities and updates and workarounds and release schedules.

[bugzpodder]

@alexch you don't need to disable preflight checks if you install the same version of jest (23.6 instead of 24) in your node code. If you decide to use Jest 24, then disabling preflight checks is a perfectly valid course of action for your decision.

@jdalegonzalez this is unfortunately the difference between reality and an ideal world. Ideally you would run a command that would fix everything, but the reality is that APIs change between versions (especially majors), we have deeply nested dependencies, and most open source projects don't have the resources to back port each fix to older versions. All of your npm modules have various version dependencies (eg Jest 23 uses micromatch 2 but eslint might use micromatch 3 at the same time) so you can't always have the latest packages for every nested dependency. So yes, it needs to be on a case-by-case basis. If you are on a team then one person should be the POC for security related issues. If you aren't sure about a particular vulnerability assessment, issues are one of the perfectly valid ways of asking for help.

[ztolley]

Handlebars just joined the party with a High priority vulnerability too

[jdalegonzalez]

Yep. That's why we pulled it out. We can live without test coverage reporting

…

On Mon, Feb 18, 2019, 4:15 PM Zac Tolley ***@***.*** wrote: Handlebars just joined the party with a High priority vulnerability too — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6443 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZ6hKAredtyt7L_aQCyipooTEOkJDXSks5vOxf3gaJpZM4a_CiD> .

[dlipeles]

Any ideas for suppressing this on yarn audit? Our CI is currently blocked even though jest is a dev dependency.

Looks like yarn doesn't allow for auditing production-only dependencies.

[emilymclean2]

I don't really understand why, but it seems like this is related to my perfect-scrollbar being on the left of my page instead of the right - I'm getting identical vulnerabilities after running npm audit, but everything else is working fine.

Edit: running Material Dashboard Pro

[bugzpodder]

@dlipeles why would this block a CI? Is it because of the preflight check or do you have a build step that hinges on yarn audit succeeding? In any case, if you are yarn, you could use yarn resolutions to pin micromatch to v3 but it might break something, YMMV.

[jitenderchand1]

NPM Audit is part of CI pipeline and due to this issue it is failing every time I pushed the code.

Why this issue started to occurred, I have not updated any package?

[bugzpodder]

Is it possible to change your CI pipeline to omit npm audit step? From my experience it takes a non-trivial amount of time to test/deploy/propagate a security fix. In most cases, when a security advisory is posted, either it doesn't affect your production build (such as this case), or that it is a real issue. In the latter case, either a fix might not be immediately available/feasible, or there could be workarounds that might mitigate risk while keeping the package as-is, and blocking all builds until the package is updated might not be the best course of action.

EDIT: This comment isn't meant as a call to ignore security warnings. npm audit has its place and it should be done as part of security audit for your application. It should be monitored and dealt on a case by case basis. IMO it doesn't belong in a CI pipeline for the reasons mentioned above.

[jdalegonzalez]

Is it possible to change your CI pipeline to omit npm audit step? From my experience it takes a non-trivial amount of time to test/deploy/propagate a security fix.

@bugzpodder With all due respect, this feels like bad advice to me. While I recognize that right this minute we're in a bit of a pickle as it relates to NPM audit, the idea that we all should ignore security warnings because most of the time they don't impact our app feels a little careless. Almost all high-profile breaches I've ever seen reported start with someone thinking it wasn't a high-priority to patch some previously reported vulnerability. A very low percentage of hacks are exploiting zero-day/unreported vulnerabilities. Within the last six months, there were packages on npm that had the ability to steal cryptocurrency - no impact to the "app" itself, it just made the app a conduit for theft.

If you feel like "for right now" removing the audit from the CI pipeline is necessary while solutions are explored - I would at least sympathize and maybe agree. If you're advocating (like some are) that the standard npm audit be wrapped in some script that filters out those warnings deemed as acceptable risk (again until npm fixes their audit system to support this natively), I would agree too.

Making the blanket suggestion that people stop depending on npm audit as a part of CI altogether because "in most cases" security advisories don't impact "most" production apps feels like a bridge too far.

(of course, that's just my opinion. I could be wrong)

[sorahn]

@jitenderchand1

Why this issue started to occurred, I have not updated any package?

The vulnerability was discovered in an existing package.

[jdalegonzalez]

A vulnerability was discovered and reported in a package that one of your packages ultimately depends on.  You didn’t need to update anything.  The vulnerability report is what is new. From: Daryl Roberts <notifications@github.com> Reply-To: facebook/create-react-app <reply@reply.github.com> Date: Tuesday, February 19, 2019 at 8:40 AM To: facebook/create-react-app <create-react-app@noreply.github.com> Cc: jdalegonzalez <dale.gonzalez@gmail.com>, Mention <mention@noreply.github.com> Subject: Re: [facebook/create-react-app] Dependency on insecure version of braces (Node security advisory 786) (#6443) @jitenderchand1 Why this issue started to occurred, I have not updated any package? The vulnerability was discovered in an existing package. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[iansu]

Let's focus on the issue here. This is not the place to debate the merits of npm audit or how your CI should be set up.

Since Jest isn't planning to backport this fix to Jest 23 the only thing we can do is release a major version of Create React App with Jest 24. We are currently working on that and you can follow the progress on upgrading to Jest 24 here: #6278

[SimenB]

Potentially, you can get a patch into braces/micromatch (although it's v2, with v3 being current major and v4 right around the corner).

Note that upgrading from micromatch 2 to 3 is a breaking change in and of itself, so there's really no way Jest can backport a fix to v23

[Waseemrajashaik]

any updates ??

[yhoiseth]

@Waseemrajashaik Based on @iansu's comment above, I think #6278 is relevant to your question.

[godmar]

Let's focus on the issue here. This is not the place to debate the merits of npm audit or how your CI should be set up.

Agreed. I shared my view on the merits of npm audit in the npm forum. I hope I represented the technical aspects correctly.

[ghost]

even thought installed latest CRA it still gives. Any updates?

[ianschmitz]

We haven't yet upgraded to Jest 24 which includes the fix but it is in progress. This will be part of the 3.0 release.

[methodbox]

FWIW, I went through my node_modules folder and searched out each instance of micromatch. I opened the respective folders and the package.json in each and manually changed the braces dependency version to 2.3.2. Jest was not the only culprit; after updating their package.json files I also had to update this in about 8 other packages edit nested inside Jest; so I guess you could say it's all Jest. They were all on 2.3.1, except one that was 1.8.5.

I did npm install to re-scan and install per the new package.jsons.

This resolved the warning (as the fix is >2.3.1) and CRA works fine.

Here's my npm tree now:

edit - I'm parsing this down. My npm tree was frickin huge. Hopefully this makes it easier for others to do the same, instead of digging folder by folder.

+-- react-scripts@2.1.8
| ...
| +-- jest@23.6.0
| | ...
| |   +-- jest-get-type@22.4.3
| |   +-- jest-haste-map@23.6.0
| |   | +-- fb-watchman@2.0.0
| |   | `-- sane@2.5.2
| |   |   ...
| |   |   +-- micromatch@3.1.10
| |   |   | ...
| |   |   | +-- braces@2.3.2 
| |   |   | ...
| |   +-- jest-message-util@23.4.0
| |   | ...
| |   | +-- braces@2.3.2 
...
| | +-- fork-ts-checker-webpack-plugin@1.0.0-alpha.6
| | | ...
| | | +-- micromatch@3.1.10
| | | | ...
| | | | +-- braces@2.3.2
| | | | | ...
| | +-- globby@8.0.2
| | | ...
| | | +-- fast-glob@2.2.6
| | | | +-- merge2@1.2.3
| | | | `-- micromatch@3.1.10
| | | |   +-- braces@2.3.2
| | | |   | ...
| | +-- micromatch@3.1.10
| | | ...
| | | +-- braces@2.3.2
| | | |...
| +-- webpack-dev-server@3.1.14
| | ...
| | +-- chokidar@2.1.5
| | | +...
| | | | ...
| | | +-- braces@2.3.2
...
| | | +-- readdirp@2.2.1
| | | | +-- graceful-fs@4.1.15 deduped
| | | | +-- micromatch@3.1.10
| | | | | ...
| | | | | +-- braces@2.3.2
| | | | | |...
| | +-- compression@1.7.4
| | |...
| | | +-- lodash@4.17.11 deduped
| | | `-- micromatch@3.1.10
| | |   ...
| | |   +-- braces@2.3.2
| | |   ...
`-- typescript@3.4.1

[marcinwasilewski]

Hmm ok so what would be the fastest way to continue working on my old project right now if npm audit fails, barring completely ignoring the issue by installing with --no-audit?

[ianschmitz]

For those that need this resolved immediately you can try out our alpha build by following the instructions over at #6475. Closing this for now as it should be resolved and will be part of the 3.0 release.