fix(arborist): fix infinite loop with bundledDependencies and overrides

[everett1992]

Fixes #9227

npm install hangs when a project uses bundledDependencies and overrides targeting a transitive dep shared by multiple bundled deps.

In edge.js satisfiedBy(), the inBundle check (added in #4963) uses rawSpec for bundled nodes to prevent overrides from applying to pre-resolved deps inside a dependency's tarball. However, inBundle is also true for deps the root itself will bundle - these are freshly resolved from the registry and overrides should apply.

The override was always applied at placement time (correct version installed), but the edge stayed invalid because satisfiedBy checked rawSpec. Two bundled deps sharing the overridden transitive dep would endlessly re-queue each other via REPLACE.

The fix changes inBundle to inDepBundle, which is only true when the bundler is a non-root package. This preserves the #4963 behavior for deps pre-resolved inside a dependency's bundle/shrinkwrap while allowing the root's overrides to work.

Note: it is unclear whether overrides should be applied to deps that will be bundled or shrinkwrapped. The comment says that we explicitly don't, but I can't find supporting docs, and the existing behavior is that overrides are applied to dependencies that will be bundled/shrinkwrapped. I added tests asserting that behavior.

These new tests passed without the change:

• overrides do not apply inside a dependency that bundles
• node bundled inside a dependency uses rawSpec
• node inside a shrinkwrap uses rawSpec

These new tests failed, they produced the same tree, but the edges were marked invalid:

• node bundled by root uses overridden spec
• overrides apply to deps the root will bundle and edges are valid

This test hung forever:

• does not infinite loop

In both cases overrides that are 'baked into' dependnecies appear as 'invalid'. This happens because the root package doesn't read the bundler's overrides, and doesn't know why the shrinkwrap/bundle included the out-of-spec version. This commit doesn't affect that behavior.

References

[wraithgar]

Looks like you'll need npm run lintfix to be ran on the new tests.

[wraithgar]

Note: it is unclear whether overrides should be applied to deps that will be bundled or shrinkwrapped

This is something folks have wanted for some time, and I think it makes sense to give that kind of control to the end user. Bundled dependencies solve a small set of problems (npm itself ships with them because it needs those dependencies to install dependencies, classic bootstrapping problem), but folks who are including a package as a dependency should have total control over things.

We can be prudent here and take advantage of the fact that we are in the pre-release phase of npm 12 and consider this a breaking change, or we can consider this a bugfix. I can see the rationale for either decision.

[wraithgar]

#9221 is going to land before this PR so we should be sure to rebase once that one lands since they potentially have some overlap. I don't think there's any overlap that constitutes a bug but that's why we run tests.

[wraithgar]

The overrides that are configured in a dependency are not applied, only overrides in your base package.json.

What happens if I install a package with overridden bundled dependencies?

[wraithgar]

{
  "name": "bundled-test",
  "version": "1.0.0",
  "bundledDependencies": [
    "semver"
  ],
  "dependencies": {
    "semver": "7.5.0"
  },
  "overrides": {
    "lru-cache": "gist:021edb9564bbf3f924f380fb2372372f"
  }
}

If I have this package.json and run npm pack from this PR, I get a tarball that includes my overridden package just fine.

$ cat node_modules/lru-cache/package.json
{
  "name": "@gar/placeholder",
  "version": "0.0.0",
  "description": "Placeholder"
}      

If I install that tarball somewhere else, the bundled overridden package comes up as an error in the tree because of the name mismatch

$ node /Users/gar/Development/npm/cli/branches/gar_scratch ls -a
bundled-install@ /Users/gar/Development/scratch/bundled-install
└─┬ bundled-test@1.0.0
  └─┬ semver@7.5.0
    └── lru-cache@npm:@gar/placeholder@0.0.0 invalid: "^6.0.0" from node_modules/bundled-test/node_modules/semver

npm error code ELSPROBLEMS
npm error invalid: lru-cache@npm:@gar/placeholder@0.0.0 /Users/gar/Development/scratch/bundled-install/node_modules/bundled-test/node_modules/lru-cache

[everett1992]

That's an existing behavior - I felt that changing it either way was out of scope.

It's marked invalid because npm doesn't check bundled-tests overrides when it's considering bundled-install's tree. From that perspective npm doesn't know why @gar/placeholder was resolved instead of ^6.0.0.

One fix would be for npm to load the overrides of of nodes that have shrinkwrap or bundles so it can check if the pre-resolved dependencies are valid.
Another would be for npm to report a new state for pre-resolved invalid edges. "This is invalid, but it's determined by shrinkwrap or bundle so that's not my fault".

Anotherwould be to forbid overriding to-be-bundled or to-be-shrinkwrapped dependencies. If a dependency will be included in a bundle, or shrinkwrap, do not apply overrides. I couldn't tell if that was the intended meaning of the comment, but in practice npm allows overrides to dependencies the root package resolves, and blocks them in per-resolved dependencies - deps that are in-bundle or in-shrinkwrap of another package.

[wraithgar]

I think overriding your bundled dependencies is a valid use case, but publishing them is not. Perhaps we should simply refuse to pack/publish? Folks can still use the overrides to do local development/testing/whatever and then remove the override for publish?

[wraithgar]

Another would be for npm to report a new state for pre-resolved invalid edges. "This is invalid, but it's determined by shrinkwrap or bundle so that's not my fault".

This does seem like the easiest solution.

[everett1992]

Banning overrides in published bundles seems reasonable and consistent with npm's handling of non-bundled overrides. Tho it would be a breaking change. Actually, for consistency should npm ban overrides that affect prod dependencies in any published modules? npm will not use the overrides, except when globally installing the module.

I'm of the opinion that npm should actually respect all overrides in node_modules. If an npm library lib has overrides: { "foo": "npm:bar@^1.0.0" } then the override should apply within node_modules/lib when it's installed. libs dependencies should be hidden implementation details to the consumer, so lib should have control over the transitive dependencies (and responsibility for any bugs the overrides might cause). The alternative if a library has to fix a bug in a transitive dependency is to fork every package in the chain to the transitive.

npm/documentation#975

[wraithgar]

Allowing dependencies to define overrides is a security discussion I think we could have, but not today. The scanning infrastructure right now isn't even fully up to speed on shrinkwraps, from what I can tell.

Actually, for consistency should npm ban overrides that affect prod dependencies in any published modules

This seems like the right decision. And we are right in the middle of shipping npm 12 so the timing is perfect here.

[wraithgar]

I don't think that blocking publishes should be a prerequisite to THIS pr, and I think we should not backport this to npm 11 in anticipation of having npm 12 prevent this use case.

Thoughts?

[everett1992]

Agreed, I also don't think fixing npm ls invalid should be a blocker - you can see that with npm 11.12.1 on packages that bundle overriden dependencies.