fix: write registry allowScripts keys under install-strategy=linked - #9941
fix: write registry allowScripts keys under install-strategy=linked#9941manzoorwanijk wants to merge 1 commit into
Conversation
fc5be25 to
4b40b17
Compare
|
@reggi this probably needs a label for v11 backport. |
|
Thanks for the quick fix! However it doesn't add the version number in // expected:
"allowScripts": {
"esbuild@0.28.1": true
}
// actual
"allowScripts": {
"esbuild": true
}I installed the fix locally using a local copy of the repo with this branch: ➜ npm -v
11.19.0
➜ node npm/bin/npm-cli.js -v
12.0.2Then, both of these commands gave me the output above: ➜ node npm/bin/npm-cli.js install-script approve esbuild@0.28.1
➜ node npm/bin/npm-cli.js install-script approve esbuild |
It works perfectly fine Screen.Recording.2026-09-01.at.3.15.02.PM.mov
|
Let's chalk it up to me not setting up the npm version from this PR properly |
|
I see that #9940 has a fix solely for the deduping (same fix as you have in |
Yes, it covers many other cases as well. |
| const found = [] | ||
| for (const node of arb.actualTree.inventory.values()) { | ||
| if (node.isProjectRoot || node.isWorkspace || node.inBundle) { | ||
| if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle) { |
There was a problem hiding this comment.
We should keep Links available for positional matching. For a dependency such as "foo": "file:./local-foo", the Link is named foo, but its target is named local-foo. Skipping the Link makes approve foo fail with ENOMATCH.
The change should be match first, then dereference and deduplicate. Keep the existing name/version matching and make these replacements:
const found = new Set()
...
const target = node.isLink ? node.target : node
if (!target ||
target.isProjectRoot ||
target.isWorkspace ||
target.inBundle) {
continue
}
found.add(target)
}Please also update the comment that currently says Links are skipped.
The regression case should use this local package layout:
package.json: dependencies = { "foo": "file:./local-foo" }
local-foo/package.json: includes an install script
node_modules/foo -> ../local-foo
Both npm install-scripts approve foo and deny foo should still find that dependency.
| return true | ||
| } | ||
| // A link target carries no edges of its own; they land on the incoming Links (e.g. the linked strategy's store packages, npm/cli#9939), so delegate the edge-based check to them. | ||
| if (node.edgesIn?.size === 0 && node.linksIn?.size > 0) { |
There was a problem hiding this comment.
An empty edgesIn plus incoming Links is not sufficient to identify a registry store package: ordinary local symlink targets can have the same topology.
Please add the store check to the delegation block in isRegistryNode:
if (
isStoreBacked(node) &&
node.edgesIn?.size === 0 &&
node.linksIn?.size > 0
) {
return [...node.linksIn].every(link => link.isRegistryDependency)
}| const nameFromEdges = (node) => { | ||
| if (!node.edgesIn || typeof node.edgesIn[Symbol.iterator] !== 'function') { | ||
| const name = nameFromEdgeSet(node?.edgesIn) | ||
| if (name) { |
There was a problem hiding this comment.
The same restriction needs to apply to nameFromEdges, because the writer calls getTrustedRegistryIdentity directly. Restricting only isRegistryNode would protect matching but still permit incorrect registry keys to be written.
Keep the existing nameFromEdgeSet parser, and change nameFromEdges to:
const nameFromEdges = (node) => {
const name = nameFromEdgeSet(node.edgesIn)
if (name || !isStoreBacked(node) || node.edgesIn?.size !== 0) {
return name
}
let linkName = null
if (node.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') {
for (const link of node.linksIn) {
if (!link.isRegistryDependency) {
return null
}
const name = nameFromEdgeSet(link.edgesIn)
if (!name || (linkName && linkName !== name)) {
return null
}
linkName = name
}
}
return linkName
}This preserves direct-edge behavior and refuses to infer a name when an incoming Link is non-registry, unidentified, or disagrees with another Link.
Please update the positive unit fixtures to represent actual store targets; for example, set target.isInStore = true. Add the inverse case outside .store: even with an incoming registry Link, both the registry policy match and trusted registry identity should remain null.
| } | ||
|
|
||
| // True when the node lives in the linked install strategy's `node_modules/.store` directory; such registry-managed packages must never derive identity from the store's internal `file:` link specs. | ||
| const isStoreBacked = (node) => { |
There was a problem hiding this comment.
Please anchor the path fallback to the current tree’s .store directory instead of accepting node_modules/.store anywhere in the path.
This matters because we’re now using this helper to decide whether incoming Links can supply registry identity. When isInStore is absent, a path inside an unrelated project’s .store should not qualify merely because it contains that directory name.
Keep the isInStore fast path, and replace the existing fallback as follows:
- const { sep } = require('node:path')
+ const { resolve, sep } = require('node:path')- const real = node?.realpath || node?.path
- return typeof real === 'string' && real.includes(`${sep}node_modules${sep}.store${sep}`)
+ // Nodes loaded from the hidden lockfile do not retain isInStore.
+ const paths = [node?.path, node?.realpath].filter(p => typeof p === 'string')
+ const roots = [node?.root?.path, node?.root?.realpath]
+ .filter(p => typeof p === 'string')
+ for (const root of roots) {
+ const store = resolve(root, 'node_modules', '.store')
+ if (paths.some(path => path.startsWith(`${store}${sep}`))) {
+ return true
+ }
+ }
+ return falsePlease cover these fallback cases without setting isInStore:
| Target path | Expected classification |
|---|---|
Inside the current root’s .store |
Store-backed |
Inside the root’s resolved physical .store when the root is symlinked |
Store-backed |
| Ordinary local directory | Not store-backed |
Inside another project’s .store, outside both root paths |
Not store-backed |
|
why i’m in this group?
null
martin ***@***.***> schrieb am Sa. 5. Sept. 2026 um 01:01:
… ***@***.**** commented on this pull request.
------------------------------
In lib/utils/allow-scripts-cmd.js
<#9941 (comment)>:
> const matched = []
const unmatched = []
for (const arg of args) {
const { name: wantName, range } = parsePositional(arg)
const found = []
for (const node of arb.actualTree.inventory.values()) {
- if (node.isProjectRoot || node.isWorkspace || node.inBundle) {
+ if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle) {
We should keep Links available for positional matching. For a dependency
such as "foo": "file:./local-foo", the Link is named foo, but its target
is named local-foo. Skipping the Link makes approve foo fail with
ENOMATCH.
The change should be match first, then dereference and deduplicate. Keep
the existing name/version matching and make these replacements:
const found = new Set()...const target = node.isLink ? node.target : nodeif (!target ||
target.isProjectRoot ||
target.isWorkspace ||
target.inBundle) {
continue
}
found.add(target)}
Please also update the comment that currently says Links are skipped.
The regression case should use this local package layout:
package.json: dependencies = { "foo": "file:./local-foo" }
local-foo/package.json: includes an install script
node_modules/foo -> ../local-foo
Both npm install-scripts approve foo and deny foo should still find that
dependency.
------------------------------
In workspaces/arborist/lib/script-allowed.js
<#9941 (comment)>:
> @@ -350,7 +383,14 @@ const isRegistryNode = (node) => {
// edge resolves to a registry spec, which is much harder to spoof than
// the URL.
if (typeof node.isRegistryDependency === 'boolean') {
- return node.isRegistryDependency
+ if (node.isRegistryDependency) {
+ return true
+ }
+ // A link target carries no edges of its own; they land on the incoming Links (e.g. the linked strategy's store packages, npm/cli#9939), so delegate the edge-based check to them.
+ if (node.edgesIn?.size === 0 && node.linksIn?.size > 0) {
An empty edgesIn plus incoming Links is not sufficient to identify a
registry store package: ordinary local symlink targets can have the same
topology.
Please add the store check to the delegation block in isRegistryNode:
if (
isStoreBacked(node) &&
node.edgesIn?.size === 0 &&
node.linksIn?.size > 0) {
return [...node.linksIn].every(link => link.isRegistryDependency)}
------------------------------
In workspaces/arborist/lib/script-allowed.js
<#9941 (comment)>:
> @@ -235,10 +246,32 @@ const getTrustedRegistryIdentity = (node) => {
}
const nameFromEdges = (node) => {
- if (!node.edgesIn || typeof node.edgesIn[Symbol.iterator] !== 'function') {
+ const name = nameFromEdgeSet(node?.edgesIn)
+ if (name) {
The same restriction needs to apply to nameFromEdges, because the writer
calls getTrustedRegistryIdentity directly. Restricting only isRegistryNode
would protect matching but still permit incorrect registry keys to be
written.
Keep the existing nameFromEdgeSet parser, and change nameFromEdges to:
const nameFromEdges = (node) => {
const name = nameFromEdgeSet(node.edgesIn)
if (name || !isStoreBacked(node) || node.edgesIn?.size !== 0) {
return name
}
let linkName = null
if (node.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') {
for (const link of node.linksIn) {
if (!link.isRegistryDependency) {
return null
}
const name = nameFromEdgeSet(link.edgesIn)
if (!name || (linkName && linkName !== name)) {
return null
}
linkName = name
}
}
return linkName}
This preserves direct-edge behavior and refuses to infer a name when an
incoming Link is non-registry, unidentified, or disagrees with another Link.
Please update the positive unit fixtures to represent actual store
targets; for example, set target.isInStore = true. Add the inverse case
outside .store: even with an incoming registry Link, both the registry
policy match and trusted registry identity should remain null.
------------------------------
In workspaces/arborist/lib/script-allowed.js
<#9941 (comment)>:
> @@ -99,6 +100,15 @@ const matches = (node, key, failClosed) => {
}
}
+// True when the node lives in the linked install strategy's `node_modules/.store` directory; such registry-managed packages must never derive identity from the store's internal `file:` link specs.
+const isStoreBacked = (node) => {
Please anchor the path fallback to the current tree’s .store directory
instead of accepting node_modules/.store anywhere in the path.
This matters because we’re now using this helper to decide whether
incoming Links can supply registry identity. When isInStore is absent, a
path inside an unrelated project’s .store should not qualify merely
because it contains that directory name.
Keep the isInStore fast path, and replace the existing fallback as
follows:
- const { sep } = require('node:path')+ const { resolve, sep } = require('node:path')
- const real = node?.realpath || node?.path- return typeof real === 'string' && real.includes(`${sep}node_modules${sep}.store${sep}`)+ // Nodes loaded from the hidden lockfile do not retain isInStore.+ const paths = [node?.path, node?.realpath].filter(p => typeof p === 'string')+ const roots = [node?.root?.path, node?.root?.realpath]+ .filter(p => typeof p === 'string')+ for (const root of roots) {+ const store = resolve(root, 'node_modules', '.store')+ if (paths.some(path => path.startsWith(`${store}${sep}`))) {+ return true+ }+ }+ return false
Please cover these fallback cases *without setting isInStore*:
Target path Expected classification
Inside the current root’s .store Store-backed
Inside the root’s resolved physical .store when the root is symlinked
Store-backed
Ordinary local directory Not store-backed
Inside another project’s .store, outside both root paths Not store-backed
—
Reply to this email directly, view it on GitHub
<#9941?email_source=notifications&email_token=CBCAR34CMTMWTGIEJ4SDIET5NNCS7A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJRHAZTSMBQG432M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#pullrequestreview-5118390077>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CBCAR367ZPI6OZRZUBJ4GMT5NNCS7AVCNFSNUABFKJSXA33TNF2G64TZHMYTGOJZGEYDEMRZHNEXG43VMU5TKMZRGAZTKNJZGIZ2C5QC>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
4 participants
Under
install-strategy=linked,npm install-scripts approve <pkg>wrote verbose, duplicatedfile:entries pointing intonode_modules/.store(one per incoming symlink depth) instead ofname@versionpins, and those store-path entries never matched at install time.There are two root causes.
In
findNodesForArgs(allow-scripts-cmd.js), positional args matched everyLinkpointing at the store package; each Link's relativefile:.store/...resolved spec became its own policy key and could even strip the correct pin as stale.In
script-allowed.js, a store package has noedgesIn(they land on its incoming Links), soisRegistryNoderefused registry keys, andls, the post-install advisory, and prune treated a correctname@versionentry as matching nothing.The fix skips
Linknodes when matching positional args, mirroringcollectUnreviewedScriptsand prune, so approvals key off the real package's trusted registry identity.isRegistryNodeandnameFromEdgesnow delegate edge-based checks to a link target's incoming Links, which also coversomit-lockfile-registry-resolved(approve by name, like the hoisted #9558 path).resolvedSourceSpecsno longer fabricatesfile:specs from links into the store, so store packages are never keyed by store paths and prune cleans up the buggy entries while keeping the valid pin.References
Fixes #9939