import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.56k stars 1.57k forks source link

Update dependency tsconfig-paths #2639

Closed Miljoen closed 1 year ago

Miljoen commented 1 year ago

A high severity vulnerability in JSON5 was discovered.

tsconfig-paths relies on this package.

Fortunately, a fix was created for v4.1.2.

Request:

Update dependency

"tsconfig-paths": "^3.14.1"

To

"tsconfig-paths": "^4.1.2"
Teascade commented 1 year ago

Bump. This is very annoying for me personally because the place I work at, this dependency is actually pretty much at the bottom of our dependency hierarchy so it's impossible to get rid of, and also because npm audit lacks a feature to disable specific packages from auditing. The only way for us to currently work around this is to literally change our automatic audits to --audit-level critical or --omit=dev, neither of which is good or optimal.

air2 commented 1 year ago

Omitting CVE is a bad idea, because you could still trigger the CVE in JSON5 with a different route. So if an other package would depend on json5 the CVE could be very real, although through this package it will not be triggered. So only for this dependency tree it is a false positive. Also since a few days the CVE is fixed in the v1 of json5 but the CVE db is not yet updated so it is now a real false positive.

Teascade commented 1 year ago

You can use the eslint-plugins-i package

We can't, because we're also using eslint-config-airbnb-base which peer-depends on this package specifically.

desaisagar commented 1 year ago

I have created a fix. Can someone review and merge it?

air2 commented 1 year ago

@Teascade I use .npmrc with legacy-peer-deps=true to get around this issue. In my package.json I have: "eslint-plugin-import": "npm:eslint-plugin-i@^2.26.0-2",

air2 commented 1 year ago

@desaisagar The maintainer of this package unfortunately does not consider this a bug and will not merge fixes for it. (See all the other related and closed tickets)

desaisagar commented 1 year ago

Okay, thanks for the information @air2.

gustaff-weldon commented 1 year ago

@air2 any idea why maintainer is not concerned in helping to fix a CVE security issue? Is it because it was fixed in json5 1.0.2?

cbruchwaldnetfonds commented 1 year ago

@gustaff-weldon you may find the explanation in this issue https://github.com/import-js/eslint-plugin-import/issues/2631

air2 commented 1 year ago

It is because it is not a CVE for this package and ask everybody just to ignore this CVE, which is of course a bad idea, because the CVE could also be triggered if another packages uses json5. Also he wants to support node4, because not supporting node4 would be a breaking change, and needs a major version increment, which would be more work for him.

Teascade commented 1 year ago

@Teascade I use .npmrc with legacy-peer-deps=true to get around this issue. In my package.json I have: "eslint-plugin-import": "npm:eslint-plugin-i@^2.26.0-2",

I would as well, but for some weird reason this results in an error Cannot read property 'spec' of undefined from npm itself with some projects, not all. Probably a bug, but makes the feature completely unusable for me.

ljharb commented 1 year ago

json5 v1.0.2 has already been updated with this fix, and either way, it's not a valid vulnerability.

As is the case with almost every JS CVE, the best course of action is to do nothing until the ecosystem fixes it for you.

This is a duplicate of #2625; a duplicate of #2628; a duplicate of #2626; a duplicate of #2627; a duplicate of #2631; a duplicate of #2632; a duplicate of #2634; a duplicate of #2635; a duplicate of #2636; a duplicate of #2637.

Please stop filing issues about a vulnerability on "not the vulnerable package", it doesn't help.

mashpie commented 1 year ago

@ljharb please stop telling to ignore CVE per se!

ljharb commented 1 year ago

@mashpie why? it's a good one to ignore, and spreading FUD about it doesn't make anything more secure.

mashpie commented 1 year ago

@ljharb with your Package possibly not vulnerable doesn‘t mean there won‘t be any exploits for this CVE of json5.

ignoring it will ignore all vulnerable attack vectors beside of your package too. Like: chromiim, tap, nuxt, air-bnb, some pm2 etc.

json5 maintainer (@jordabtucker) himself committed to this CVE to be risky and mitigated it in all Majors.

So: Ok, assuming you package won‘t exploit that vulnerabilty, others might do so. Telling your community to ignore this CVE and generalizing it in way like „… As is the case with almost every JS CVE…“ is wrong, missleading and dangerous for the whole ecosystem you - on the other side - relay on to „… until the ecosystem fixes it for you…“

please don‘t get ne wrong: Fixes ARE applied by Json5 in v1 and v2 and it‘s not eslint-Plugin-Import to be blamed. Story finished. Keep CVEs serious and to investigated and mitigated rather than to be ignored.

Thanks! Have a nice 2023.

mashpie commented 1 year ago

…sry for spelling: autocorrection on mobile ;)

ljharb commented 1 year ago

@mashpie first of all, i have been very explicit in explaining that this code path isn't valid, but either way, Prototype Pollution is a class of CVEs that isn't actually that exploitable in practice - primarily only in theory.

If we all want CVEs to be considered seriously, then the entire system (one designed for boxed software and hardware) needs to be rethought from scratch for open source, which would include considering code paths, so that transitive warnings don't trigger unnecessarily - because in security, false negatives are MUCH WORSE than false positives, because they undermine confidence in the security systems themselves. The most dangerous thing for the ecosystem is treating every CVE as equally critical and scary.

Miljoen commented 1 year ago

@ljharb Hi man, I just wanted to thank you for the patient explanation given in https://github.com/import-js/eslint-plugin-import/issues/2631.

I've learned something about the npm ecosystem here.

And lastly, excuse us for making so many duplicate issues on your repo for what is, ultimately, out of your control.

See you!

ljharb commented 1 year ago

Thanks, I appreciate that.