npm / rfcs

Public change requests/proposals & ideation
Other
726 stars 238 forks source link

[RRFC] npm audit fix should consider package source in alert delivery #739

Open darakian opened 10 months ago

darakian commented 10 months ago

Motivation ("The Why")

At the moment npm audit does not seem to take into account the origin source of a package when delivering alerts. This results in alerts which are written specifically for packages hosted on npm to be delivered erroneously to users who pull packages of matching names from other locations.

Example

https://github.com/github/advisory-database/issues/2701#issuecomment-1772006702

How

Current Behaviour

Consider the package source when resolving npm audit alerts

Desired Behaviour

Alerts do not get erroneously delivered.

References

GitHub advisory database ecosystem definitions: https://github.com/github/advisory-database/#supported-ecosystems

ljharb commented 10 months ago

Non-scoped packages from other locations are generally inadvisable; is there a reason they're not scoped (with a scope you own on the public registry, to avoid a supply chain attack vector)?

darakian commented 10 months ago

Non-scoped packages from other locations are generally inadvisable

Agreed.

is there a reason they're not scoped

In the example provided it seems to be that the project simply doesn't publish to npm. Not my code, not my project, so I can't speak to the motivation ¯\_(ツ)_/¯

wesleytodd commented 10 months ago

Even if "they should be scoped" I think it is common enough that it is worth discussing a fix. Unfortunately a fix here will be hard. I think the main issue is that most companies should be pointing all of their installs through an internal proxy, so the tarball urls in the lockfiles will all point internally even if they are just proxied from the public registry.

And my guess is that most people are not implementing their own internal audit endpoints. I could be wrong, but likely most folks just proxy right on to public npm for that. I think the audit would need the some of the local config to be able to make correct decisions. And honestly it might even need the out of band information from the internal registry proxy.

wesleytodd commented 10 months ago

I just took a look at the linked report, and in the case of a git dependency I think the lockfile does have enough information to disambiguate from the registry. I am guessing that the audit needs to only report on tag, version, and range specs?

Still though, the problem is larger than just the git refs.

KateCatlin commented 10 months ago

Hey @wesleytodd just checking in on this issue. Anything we should report back to the user who posted it with?

wesleytodd commented 10 months ago

Hm, not sure. I would think that @lukekarrys or @wraithgar might need to answer that. I am guessing the GH advisory db or whatever reporting systems are involved have not been updated to take into account the spec type like I mentioned above?