sarbbottam / eslint-find-rules

Find built-in ESLint rules you don't have in your custom config
http://npm.im/eslint-find-rules
MIT License
201 stars 33 forks source link

Security warning for "mem" #307

Closed Xotic750 closed 4 years ago

Xotic750 commented 4 years ago
WS-2018-0236 More information
moderate severity
Vulnerable versions: < 4.0.0
Patched version: 4.0.0
In nodejs-mem before version 4.0.0 there is a memory leak due to old results not being removed from the cache despite reaching maxAge. Exploitation of this can lead to exhaustion of memory and subsequent denial of service.

npm ls mem shows:

└─┬ eslint-find-rules@3.3.1
  └─┬ yargs@8.0.2
    └─┬ os-locale@2.1.0
      └── mem@1.1.0 
ljharb commented 4 years ago

like many npm audit reports, this is a false positive, as eslint-find-rules isn’t long-running enough for this to be a problem.

ljharb commented 4 years ago

Also mem and nodejs-mem are two different packages.

Xotic750 commented 4 years ago

Ok, little confused here as to what github is on about?

Screenshot 2019-07-25 at 17 20 41
ljharb commented 4 years ago

ah, maybe it's just a poorly-written CVE explanation.

Either way, it's a false positive for this tool, and we'd have to update yargs to avoid it, which I suspect isn't worth the effort.

Xotic750 commented 4 years ago

I understand the effort part. :) I was not overly concerned, but explain that to my manager. :D

Xotic750 commented 4 years ago

I created a PR for a full dependency update: https://github.com/sarbbottam/eslint-find-rules/pull/308

All seems well with it, if you want to use it ;)

Xotic750 commented 4 years ago

New PR https://github.com/sarbbottam/eslint-find-rules/pull/309

ljharb commented 4 years ago

We can revisit updating yargs past v11 if node 4 is dropped in the future.

ronkorving commented 4 years ago

It's worth updating just to get rid of the false alarm for everybody who encounters it, who is currently forced to go investigate and waste a ton of time. I would suggest reopening.

ljharb commented 4 years ago

A better solution would be to direct efforts towards CVE factories and warning mechanisms (npm audit, github's vuln alerts) to find ways to minimize false positives rather than forcing churn on maintainers.

G-Rath commented 4 years ago

@ljharb Not quite sure why you think this is a false positive, as the dependency is quite clear:

└─┬ eslint-find-rules@3.3.1
  └─┬ yargs@8.0.2
    └─┬ os-locale@2.1.0
      └── mem@1.1.0 

I'm assuming you consider it a FP b/c it's not directly on eslint-find-rules, but instead one of its dependencies, but just want to ask in case I've missed something 🙂

That aside, since ESLint has now dropped Node4 support is there anything in particular that's keeping node@4 support around for you?

If it's a matter of time, I'm happy to do the bulk of the work if you'd be happy to review the PR 🙂 (also happy to wait until the new major of ESLint is released, so that node@8 support can be dropped at the same time)

ljharb commented 4 years ago

@G-Rath it's a false positive not because it's not in the dep graph, but because the CVE does not affect eslint-find-rule's usage of yargs and thus mem.

eslint-find-rules supports down to eslint 3; what eslint 6 has dropped isn't really relevant. There would need to be a very compelling reason to drop support of an old eslint, i think, and "there's a non-applicable audit warning" doesn't feel like one.