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

Update dependency tsconfig-paths #2639

Closed
Miljoen opened this issue Jan 2, 2023 · 19 comments
Closed

Update dependency tsconfig-paths #2639

Miljoen opened this issue Jan 2, 2023 · 19 comments

Comments

@Miljoen
Copy link

Miljoen commented Jan 2, 2023

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
Copy link

Teascade commented Jan 2, 2023

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
Copy link

air2 commented Jan 2, 2023

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
Copy link

Teascade commented Jan 2, 2023

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
Copy link

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

@air2
Copy link

air2 commented Jan 2, 2023

@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
Copy link

air2 commented Jan 2, 2023

@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
Copy link

Okay, thanks for the information @air2.

@gustaff-weldon
Copy link

gustaff-weldon commented Jan 2, 2023

@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
Copy link

@gustaff-weldon you may find the explanation in this issue #2631

@air2
Copy link

air2 commented Jan 2, 2023

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
Copy link

Teascade commented Jan 2, 2023

@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
Copy link
Member

ljharb commented Jan 2, 2023

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.

@ljharb ljharb closed this as not planned Won't fix, can't repro, duplicate, stale Jan 2, 2023
@import-js import-js deleted a comment from air2 Jan 2, 2023
@mashpie
Copy link

mashpie commented Jan 2, 2023

@ljharb please stop telling to ignore CVE per se!

@ljharb
Copy link
Member

ljharb commented Jan 2, 2023

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

@mashpie
Copy link

mashpie commented Jan 2, 2023

@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
Copy link

mashpie commented Jan 2, 2023

…sry for spelling: autocorrection on mobile ;)

@ljharb
Copy link
Member

ljharb commented Jan 2, 2023

@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
Copy link
Author

Miljoen commented Jan 4, 2023

@ljharb Hi man, I just wanted to thank you for the patient explanation given in #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
Copy link
Member

ljharb commented Jan 4, 2023

Thanks, I appreciate that.

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

No branches or pull requests

8 participants