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(vulnerabilty): switch tiny dependency and use same code #4336

Closed

Conversation

deleonio
Copy link
Contributor

Requirements

See #4332

The problem is the maintainment of two dependencies.
Main impact comes from lodash.
Second impact is the missing update from yargs-unparser.

Description of the Change

The current master code of yargs-unparser is okay. So I forked it an publish this code as npm packge named @care-for/yargs-unparser.

An replace yargs-unparser with @care-for/yargs-unparser in the package.json of mocha.

Alternate Designs

Why should this be in core?

Benefits

Zero vulnerabilities

Possible Drawbacks

Applicable issues

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.795% when pulling a04fdea on martinoppitz:fix/vulnerability-issue-4332 into 9d4a8ec on mochajs:master.

@deleonio
Copy link
Contributor Author

Hello, I dont know, whats going wrong.

-- netlify/mocha/deploy-preview

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@martinoppitz thank you.
IMO this is not a good solution:

  • see comment: Mocha => Yargs-unparser is not affected by this vulnerability.
  • publishing our own versions of dependencies is not a good option, for maintainance reasons and especially for an unjustified security warning.

I don't see any need for action in this matter and recommend to close this PR.

@deleonio
Copy link
Contributor Author

@juergba

Yes, this solution is not the best.

But how we will solve the lodash blocker?!

@Munter
Copy link
Member

Munter commented Jun 16, 2020

It's not a blocker. The problem is not in mocha but in your local tooling. Don't block on non-curated false positive security audits

@deleonio
Copy link
Contributor Author

@Munter

Sorry, thats not right. See here https://snyk.io/test/npm/mocha?tab=dependencies.

The problem is to fix the lodash vulnerability, because lodash is not maintained.

So I tried to fix yargs-unparser ... I hope anybody publish a new version.

@Munter
Copy link
Member

Munter commented Jun 16, 2020

@martinoppitz You did obviously not read @boneskull's reponse at #4332 (comment) properly.

There is a security vulnerability in lodash, but yargs-unparser is not using the vulnerable function. This means the vulnerability does actually not affect yargs-unparser and mocha.

Now, if you want to talk about security, asking us to switch our yargs-unparser dependency to a random fork that could very easily be compromised in other ways. Which does not have a history of active maintenance or a record of keeping up to date... That's a hard no.

This is a case of automated security checks causing false positives and resulting in generating more work for open source maintainers. You have now had 3 maintainers spend their time, not only on reviewing the security audit and follow the code path to conclude there's nothing we can do, but also on having to explain the same thing to you multiple times.

The problem that is blocking you is your CI configuration. Configure your CI differently.

I'm going to close these PR's

@Munter Munter closed this Jun 16, 2020
@deleonio deleonio deleted the fix/vulnerability-issue-4332 branch June 16, 2020 18:41
@deleonio
Copy link
Contributor Author

Hi @Munter ,

I have read the description from @boneskull - read for yourself: #4332 (comment)

And I would not discuss about security. I will say, that mocha depends on:

  • yargs-unparser@1.6.0 and this published package depends on
  • lodash@4.7.15

and both probably not maintained.

image

image

I tried my best to explain the vulnerability problem.

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

Successfully merging this pull request may close these issues.

None yet

4 participants