Conversation
121ca07 to
8b3ddf4
Compare
| } | ||
| }, | ||
| "node_modules/@npmcli/arborist/node_modules/tar": { | ||
| "version": "6.2.1", |
There was a problem hiding this comment.
The override's intention was to prevent us from installing tar in version 6.x due to known vulnerabilities. It seems like removing them returns that dependency.
There was a problem hiding this comment.
Thanks for looking into this! Since I got curious and looked a bit deeper, I thought I might as well share what I observed. It might well be that I am on a completely wrong track here. In that case, apologies for the long text.
--
It looks like pacote@20 in npm-shrinkwrap.json was not introduced by those overrides. (c5e1221)
Instead the change came with upgrade of @ui5/server dependency. (8722477)
The new version of @ui5/server was not even built with pacote@20 itself as it seems.
After the change, it's own package-lock.json still contained version 19.
(SAP/ui5-server@v4.0.10...v4.0.11)
Now I am not sure if the change was introduced deliberately, maybe to update to pacote@20 with fixed tar dependency, or if during upgrade of @ui5/server some transistive dependency switched it's dependency of pacote to the next version.
I see no pacote with version 20 in any other packages' dependencies though, according to new npm-shrinkwrap.json's content. So, I would not know why any conflict would have been resolved to that version.
There might be a third option: Could it be that dev/testing for the tar overrides fix left pacote version 20 in place in developer's local node_modules?
In that case, creating the npm-shrinkwrap.json for the following @ui5/server upgrade commit probably would have included what was installed locally (v20) at the time instead of what a fresh install would have included.
cc @matz3
There was a problem hiding this comment.
Thanks for sharing your observations. The strange thing is, even after removing node_modules and npm-shrinkwrap.json in this repo (ui5-cli), npm install would create a package-lock.json (later to be renamed to npm-shrinkwrap.json) with a pacote@20 dependency:
rm -rf node_modules npm-shrinkwrap.json && npm installnpm ls pacote:@ui5/cli@4.0.48 /xxx/ui5-cli ├─┬ @ui5/project@4.0.13 │ └── pacote@20.0.1 └─┬ licensee@11.1.1 └─┬ @npmcli/arborist@7.5.4 ├─┬ @npmcli/metavuln-calculator@7.1.1 │ └── pacote@20.0.1 deduped └── pacote@20.0.1 deduped
I checked the other listed dependents and none require pacote in version 20:
- https://github.com/npm/cli/blob/arborist-v7.5.4/workspaces/arborist/package.json#L30C6-L30C12
- https://github.com/npm/metavuln-calculator/blob/v7.1.1/package.json#L44
So I'm a bit puzzled why npm would decide to install.
That's with the latest npm version 11.11.1. I guess this needs more digging.
There was a problem hiding this comment.
Looks like my initial hunch was wrong.
Instead, overrides just does not behave as I would have expected:
Specifying a version range at the parent keySpec (pacote@<=20) does not only act as a condition for the child override, but also as an override for the parent itself.
So, maybe removing the version range would be an option?
{ "overrides": {
"pacote": {
"tar": "^7.5.6"
}
}}It would then apply to newer pacote versions too though.
What also seems to work, according to a quick test, would be to explicitly state that pacote's version should stay as is by referencing it's currently installed version:
{ "overrides": {
"pacote@<=20": {
".": "$pacote",
"tar": "^7.5.6"
}
}}According to docs that works for referencing direct dependencies. I am not sure about stable support for sub-dependencies though.
I created a npm/cli docs issue to get clarification: npm/cli#9121.
There was a problem hiding this comment.
Generally there are certain overrides that come from the other packages. That's why I cleaned everything up!
It's not that the repository would be clean from vulnerabilities, but at least the distributed NPM package is cleaned up. The vulnerable packages remain only for devDepenencies.
tar has been patched and for the productive distribution, we ship with a pathced version.
This is not the perfect situation but manageable.
Basically, we need to compromise between clean and working solution and overrides that come with certain compromises, like the non working npm ls --all flag.
Current solution seems to be okish:
- working
npm ls --all - clean distribution
- not all
devDependenciesare "clean"
You can test the scenarios in the following ways:
- Repeat the steps to reproduce from Invalid shrinkwrapped pacote version triggers npm ELSPROBLEMS UI5/cli#1325 and
npm linkwith the localfix-overridesbranch - Run
npm audit --omit devin the localfix-overridesbranch
Overrides now are redundant for productive code and may cause issues with ceratin npm commands i.e.
npm ls --all, altough the UI5 CLI is fully functional.Fixes: UI5/cli#1325