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

Updated argparse package reference #312

Closed
wants to merge 2 commits into from

Conversation

robotsrule
Copy link

Version of argparse being referenced is very old and has a security vulnerability via an old reference to underscore. argparse has been using lodash for a very long time which alleviates the vulnerability.

https://app.snyk.io/vuln/npm:underscore.string:20170908

Version of argparse being referenced is very old and has a security vulnerability via an old reference to underscore.  argparse has been using lodash for a very long time which alleviates the vulnerability. https://app.snyk.io/vuln/npm:underscore.string:20170908
Version of argparse being referenced is very old and has a security vulnerability via an old reference to underscore.  argparse has been using lodash for a very long time which alleviates the vulnerability. https://app.snyk.io/vuln/npm:underscore.string:20170908
@Rosey
Copy link

Rosey commented Dec 3, 2018

Thank you for opening this PR 🙌 here's hoping it gets merged and released soon :)

Repository owner deleted a comment from shellscape Dec 14, 2018
@jonschlinkert
Copy link
Owner

More detail is necessary describing how exactly this vulnerability would effect this library. Please describe how anyone would use this exploit.

A pull requet to replace argparse with a different library would be much preferred. Otherwise this is a seriously low priority as it's a "vulnerability" in the CLI.

@shellscape
Copy link

shellscape commented Dec 14, 2018

@jonschlinkert I'm a little confused as to how it can be dismissed as low priority when NPM itself is marking it as moderate. https://nodesecurity.io/advisories/745 Is there not a responsibility to mitigate these kinds of vulnerabilities with or without a reproduction or 0-day use-case, especially when the task is as simple as updating a dependency?

@jonschlinkert
Copy link
Owner

jonschlinkert commented Dec 14, 2018

I'm a little confused as to how it can be dismissed as low priority when NPM itself is marking it as moderate.

  1. Unless you are planning on attacking yourself by entering a 100k string in the terminal while running the CLI, this is not even remotely a vulnerability or security concern for remarkable.
  2. People get paid for reporting those "vulnerabilities" to Snyk
  3. NPM is attempting to create a competitive advantage over Yarn and other package managers by making "security" a top priority, which results in us having to see those annoying warnings in the terminal, long after any true vulnerabilities have already been addressed. Any experienced developer would know this, as you would be violating responsible disclosure otherwise.
  4. Despite that, NPM knows this strategy works because common sense never prevails, and devs like you will still say things like "Is there not a responsibility to mitigate these kinds of vulnerabilities".
  5. If you simply want to remove the noise from the terminal, just say that. But the real complaint should be to NPM.

If you want to continue the dialog in a thoughtful, productive way, I'd be happy to listen. If, however, you only want to troll the issue by arguing from authority, it's not going to help, as I will continue to look at facts, and not react by emotion.

@doowb
Copy link
Collaborator

doowb commented Dec 14, 2018

Affected versions of this package are vulnerable to Regular Expression Denial of Service (ReDoS). It parses dates using regex strings, which may cause a slowdown of 2 seconds per 50k characters.

This means that if you pass a long string (50k characters?), that might look like a date, to the remarkable cli, your experience might be degraded by about 2 seconds. I don't see why anyone would do this to themselves.

Edit: Jon answered above while I was typing this out.
I agree with him on the other points he makes too, especially the part about people getting paid to submit "vulnerabilities" to Snyk. When they're doing this, it never seems like anyone uses common sense and actually looks at how the code is being used nor think about if it would actually be exploitable.

@jonschlinkert
Copy link
Owner

your experience might be degraded by about 2 seconds. I don't see why anyone would do this to themselves.

Then again, people use Yeoman and Inquirer, which do the same thing but without the vulnerability ;)

shellscape added a commit to shellscape/webpack-plugin-serve that referenced this pull request Dec 14, 2018
the author of markdown-toc also owns remarkable, and has a policy
of not updating for reported security issues.
see: ttps://github.com/jonschlinkert/remarkable/pull/312
shellscape added a commit to shellscape/webpack-plugin-serve that referenced this pull request Dec 14, 2018
the author of markdown-toc also owns remarkable, and has a policy
of not updating for reported security issues.
see: jonschlinkert/remarkable#312
shellscape added a commit to shellscape/webpack-plugin-serve that referenced this pull request Dec 14, 2018
* fix: html-webpack-plugin child compiler. fixes #55

* chore: remove markdown-toc

the author of markdown-toc also owns remarkable, and has a policy
of not updating for reported security issues.
see: jonschlinkert/remarkable#312
Repository owner deleted a comment from shellscape Dec 14, 2018
Repository owner deleted a comment from TrySound Dec 14, 2018
Repository owner locked as too heated and limited conversation to collaborators Dec 14, 2018
@jonschlinkert
Copy link
Owner

We will merge this in, but FWIW, argparse should have released a patch to fix this. An issue should be created with that library, it shouldn't require a bump here.

@shockey
Copy link
Collaborator

shockey commented Jul 19, 2019

cc: #310

@TrySound TrySound closed this Jul 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants