Vulnerability Advisory flaggs 1 high vulnerability in js-yaml for mocha@6.1.3

[dewwwald]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

According to the NPM vulnerability advisory there is a severe/ High vulnerability in the dependency js-yaml.
https://www.npmjs.com/advisories/813

Steps to Reproduce

Simply npm install the latest version of mocha@6.1.3

Expected behavior: [What you expect to happen]
No vulnerabilities.

Actual behavior: [What actually happens]
npm audit --parseable | awk -F $'\t' '{print $1,$2,$4,$5,$6}' will give
review js-yaml >=3.13.1 Code Injection https://npmjs.com/advisories/813

Reproduces how often: [What percentage of the time does it reproduce?]
100%

Versions

$ node_modules/mocha/bin/mocha --version # 6.1.3
$ node --version # v10.15.3

• The output of mocha --version and node node_modules/.bin/mocha --version:
• The output of node --version:
• Your operating system
  • name and version:
  • architecture (32 or 64-bit):
• Your shell (e.g., bash, zsh, PowerShell, cmd):
• Your browser and version (if running browser tests):
• Any third-party Mocha-related modules (and their versions):
• Any code transpiler (e.g., TypeScript, CoffeeScript, Babel) being used (and its version):

Additional Information

[sam-github]

@boneskull why is mocha doing "js-yaml": "3.13.0",? If you you had "js-yaml": "^3.13.0", there would be nothing to fix, mocha would be getting the patch update of js-yaml already.

[DarylThayil]

+1

[omrgrcn]

+1

[Zephilim]

+1

[dbosiers]

+1

[Zephilim]

If anyone at mocha is looking at this, please can you fix this ASAP. As @sam-github suggests, this need not have been an issue and that the resolution is trivial. However, whilst this isn't resolved (its strangely not even assigned yet) those of us with a security conscience are held up whilst our own workflow prevents us from checking in, due to a high vulnerability alert. It surprising that more people are not screaming at you because I know that mocha is widely used, but presumably are not that security aware (not using security checks in development workflow). Please fix this ASAP (I don't really want to be hacking my repo to bypass security checks!)

[dewwwald]

There is a PR, no need to thumbs up, +1. #3878

Also, before you start personally attacking hard working software engineers that most likely have a full time job apart from open source projects, try and show some respect to the developers that actually contribute to open source. Mocha is not a company here to service your open source needs. If you want to avoid these vulnerabilities go write Mocha yourself.

[Zephilim]

There is a PR, no need to thumbs up, +1. #3878

Also, before you start personally attacking hard working software engineers that most likely have a full time job apart from open source projects, try and show some respect to the developers that actually contribute to open source. Mocha is not a company here to service your open source needs. If you want to avoid these vulnerabilities go write Mocha yourself.

You've taken my comment in really bad form. I am not trying to attack anyone. Security is a really big problem in the modern age and it should be taken seriously. That is all I was trying to point out. I think mocha is fantastic and I love using it. I am sorry that you have taken my constructive comment so badly.

[dewwwald]

When you say someone does not take security serious you are being judgemental / destructive. When you say continue taking security more serious as you develop this product then you are being constructive. You where saying things in the former form. Specifically the part where you don't suggest, but make claims in a sarcastic tone caused by the placement of presumably. Your exact quote:

It surprising that more people are not screaming at you because I know that mocha is widely used, but presumably are not that security aware (not using security checks in development workflow).

Your intentions does not mean anything online because your exact words is everything. So it does not matter what you feel like you where saying, it matters what you actually said. I suggest that you read your comment again and think about how that from another persons perspective could be interpreted.

To your point on security, I totally agree. It is serious and should be taken seriously.

Lastly, I am not offended, I just despise the injustice of people who attack hard working developers online for a product that they don't even pay for. I am trying to inform you that the way you are conducting yourself is not the most helpful when you are trying to create a community that can teach young impressionable minds how to partake in open source.

[boneskull]

Historically, the problem has been that Mocha's "production" dependencies break stuff in minor and patch releases. This is why Mocha uses exact versions for production dependencies.

It's a tradeoff. It means that Mocha doesn't get fixes to vulnerabilities (or any other types of fixes) for free. But it also means that Mocha is less likely to break because a dependent package released a new bug, regression, or introduced breaking changes outside of a major release.

When something like that happens--and a fix is not yet available--Mocha has to pin use the exact release of the last known working version of that dependency, until the dependent package releases a fix.

Anyway, I'm happy to have a more in-depth discussion about the relative risks and benefits of changing this (in another issue)--but this is the reasoning.

cc @sam-github

[sam-github]

Thanks for the info. My own personal experience is that with free running dependencies I get automatic bug fixes with zero effort much more frequently than I get unwanted breakage. I recognize that experience is highly specific to a project's dependencies, and one poorly managed (from a semver perspective) dependency (or sub-sub dependency), can sour the whole experience. Perhaps in the future you might consider a white or black list approach, where you lock down deps that have introduced incompatibilities within a major, but not ones that have a history of good publishes.

[boneskull]

duplicate of #3876

[boneskull]

released as v6.1.4

[sam-github]

I don't know if this needs to be said, but since some noise followed on to a comment I made, I want to make clear that while I had a question about why mocha does what it does (which was well answered by @boneskull), I think its a well run, even exemplary project. A single day turn around for a (very mild, perhaps nonexistent) security issue is the kind of response time that money can't usually buy, and we get mocha for free.

[eight04]

I believe that you decided to pin-down prod-dependencies because it was hard for users to pin down a nested package historically. However, now we have package-lock.json, it is so easy to pin down a nested package by users. No matter the user wants to upgrade/downgrade the package. Therefore to pin down dependencies of a library becomes less meaningful and most of the time unexpected.

[sam-github]

package-lock.json can't be published: https://docs.npmjs.com/files/package-lock.json, so isn't relevant to mocha (well, could be for people developing mocha, but not consumers)

https://docs.npmjs.com/files/shrinkwrap.json.html is more relevant, but:

It’s strongly discouraged for library authors to publish this file, since that would prevent end users from having control over transitive dependency updates.

npm's assumption is that packages that will be used as dependencies use semver specs, and that top-level consumers of packages (app developers) use package lock, and top-level CLI developers publishing to npm use shrinkwrap (except they don't think those should exist, global tools should not be installed).

Leaving mocha in a grey area... its usually a dev dependency, but is not a "library", its used as a CLI.

[plroebuck]

If anyone at mocha is looking at this, please can you fix this ASAP. As @sam-github suggests, this need not have been an issue and that the resolution is trivial. However, whilst this isn't resolved (its strangely not even assigned yet) those of us with a security conscience are held up whilst our own workflow prevents us from checking in, due to a high vulnerability alert. It surprising that more people are not screaming at you because I know that mocha is widely used, but presumably are not that security aware (not using security checks in development workflow). Please fix this ASAP (I don't really want to be hacking my repo to bypass security checks!)

This issue was not assigned here because the issue had already been corrected (#3877) at least a day before you made your rather patronizing comment above. We do take security seriously, but the advisory has nothing to do with the code Mocha uses -- as noted in the advisory, the safeLoad function is unaffected.

mocha/lib/cli/config.js

Line 41 in 07ea876

require('js-yaml').safeLoad(fs.readFileSync(filepath, 'utf8')),

Release was waiting for clarification concerning whether upgrading "eleventy" could cause problems (as it also used the previous version of "js-yaml")... *sigh* YA release pending now...

As security issues occur constantly, perhaps you should consider making them an advisory part of your development workflow... IMO if they prevent you from checking in your code, your flow is borken.