fix: recognize allowScripts for local link targets (#9497)

Backport of #9490 to `release/v11`.

Co-authored-by: Rayan Salhab <r.salhab@aiyexpertsolutions.com>
Co-authored-by: cyphercodes <cyphercodes@users.noreply.github.com>

Author: github-actions[bot]

lib/utils/allow-scripts-writer.js

@@ -1,6 +1,9 @@
 const npa = require('npm-package-arg')
 const { log } = require('proc-log')
-const { getTrustedRegistryIdentity } = require('@npmcli/arborist/lib/script-allowed.js')
+const {
+  getTrustedRegistryIdentity,
+  resolvedSourceSpecs,
+} = require('@npmcli/arborist/lib/script-allowed.js')
 
 // Pure helpers that implement the RFC's pin-mismatch table for
 // `npm approve-scripts` and `npm deny-scripts`.
@@ -12,6 +15,8 @@ const { getTrustedRegistryIdentity } = require('@npmcli/arborist/lib/script-allo
 // Denying always writes `"<name>": false`, regardless of `--allow-scripts-pin`, per the
 // RFC's asymmetric-pin rule.
 
+const primaryResolvedSource = (node) => resolvedSourceSpecs(node)[0] || ''
+
 // Convert an arborist Node into the spec string used for a versioned policy
 // entry. Returns `null` if the node cannot be represented as a versioned key
 // derived from trusted sources (lockfile URL for registry, hosted shortcut
@@ -21,8 +26,7 @@ const versionedKeyFor = (node) => {
   if (!node) {
     return null
   }
-  /* istanbul ignore next: callers guarantee a string resolved */
-  const resolved = typeof node.resolved === 'string' ? node.resolved : ''
+  const resolved = primaryResolvedSource(node)
   if (resolved.startsWith('git')) {
     try {
       const parsed = npa(resolved)
@@ -69,8 +73,7 @@ const nameKeyFor = (node) => {
   if (!node) {
     return null
   }
-  /* istanbul ignore next: callers guarantee a string resolved */
-  const resolved = typeof node.resolved === 'string' ? node.resolved : ''
+  const resolved = primaryResolvedSource(node)
   if (resolved.startsWith('git')) {
     try {
       const parsed = npa(resolved)
@@ -164,7 +167,8 @@ const keyTargetsNode = (key, node) => {
     case 'git': {
       let resolvedParsed
       try {
-        resolvedParsed = node.resolved ? npa(node.resolved) : null
+        const resolved = primaryResolvedSource(node)
+        resolvedParsed = resolved ? npa(resolved) : null
       } catch {
         /* istanbul ignore next */
         return false
@@ -176,7 +180,8 @@ const keyTargetsNode = (key, node) => {
     case 'file':
     case 'directory':
     case 'remote':
-      return node.resolved === parsed.saveSpec || node.resolved === parsed.fetchSpec
+      return resolvedSourceSpecs(node)
+        .some(resolved => resolved === parsed.saveSpec || resolved === parsed.fetchSpec)
     default:
       return false
   }

test/lib/utils/allow-scripts-writer.js

@@ -45,6 +45,48 @@ t.test('nameKeyFor / versionedKeyFor — file', async t => {
   t.equal(versionedKeyFor(n), 'file:../local')
 })
 
+t.test('nameKeyFor / versionedKeyFor — local directory link target', async t => {
+  const targetPath = path.resolve('local')
+  const n = {
+    name: 'local',
+    packageName: 'local',
+    version: '1.0.0',
+    resolved: null,
+    path: targetPath,
+    realpath: targetPath,
+    linksIn: new Set([{ resolved: 'file:../local' }]),
+  }
+
+  t.equal(nameKeyFor(n), 'file:../local')
+  t.equal(versionedKeyFor(n), 'file:../local')
+
+  t.strictSame(
+    applyApprovalForPackage({}, [n], { pin: true }).allowScripts,
+    { 'file:../local': true }
+  )
+  t.match(
+    applyApprovalForPackage({ 'file:local': false }, [n], { pin: true }).warning,
+    /denied|versioned deny/
+  )
+})
+
+t.test('nameKeyFor / versionedKeyFor — empty link target has no portable file key', async t => {
+  const targetPath = path.resolve('local')
+  const n = {
+    name: 'local',
+    packageName: 'local',
+    version: '1.0.0',
+    resolved: null,
+    path: targetPath,
+    realpath: targetPath,
+    linksIn: new Set(),
+  }
+
+  t.equal(nameKeyFor(n), null)
+  t.equal(versionedKeyFor(n), null)
+  t.strictSame(applyApprovalForPackage({}, [n], { pin: true }).allowScripts, {})
+})
+
 t.test('isSingleVersionPin', async t => {
   t.ok(isSingleVersionPin('pkg@1.2.3'))
   t.notOk(isSingleVersionPin('pkg'))

workspaces/arborist/lib/script-allowed.js

@@ -97,6 +97,41 @@ const matches = (node, key) => {
   }
 }
 
+const resolvedSourceSpecs = (node) => {
+  const specs = []
+  const seen = new Set()
+  const add = (spec) => {
+    if (typeof spec !== 'string' || spec === '' || seen.has(spec)) {
+      return
+    }
+    seen.add(spec)
+    specs.push(spec)
+  }
+
+  add(node?.resolved)
+
+  if (!node?.resolved && node?.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') {
+    let hasIncomingLink = false
+    for (const link of node.linksIn) {
+      hasIncomingLink = true
+      add(link.resolved)
+    }
+
+    if (hasIncomingLink) {
+      // Link targets for local directory deps are separate inventory nodes
+      // whose own `resolved` is null. The incoming Link carries the saved spec
+      // (for example `file:../pkg`, relative to node_modules), while policy
+      // entries written by hand often use the dependency spec from package.json
+      // (for example `file:pkg`, resolved by npa to this target path). Include
+      // the real target paths so both forms can match the same local dep.
+      add(node.realpath)
+      add(node.path)
+    }
+  }
+
+  return specs
+}
+
 const matchRegistry = (node, parsed) => {
   // If this node is not a registry dep, refuse the match. A registry-style
   // key (`pkg`, `pkg@1`, `pkg@1 || 2`) must not match a tarball or git node
@@ -282,17 +317,13 @@ const matchGit = (node, parsed) => {
 }
 
 const matchFileOrDir = (node, parsed) => {
-  if (!node.resolved) {
-    return false
-  }
-  return node.resolved === parsed.saveSpec || node.resolved === parsed.fetchSpec
+  return resolvedSourceSpecs(node)
+    .some(resolved => resolved === parsed.saveSpec || resolved === parsed.fetchSpec)
 }
 
 const matchRemote = (node, parsed) => {
-  if (!node.resolved) {
-    return false
-  }
-  return node.resolved === parsed.fetchSpec || node.resolved === parsed.saveSpec
+  return resolvedSourceSpecs(node)
+    .some(resolved => resolved === parsed.fetchSpec || resolved === parsed.saveSpec)
 }
 
 const isRegistryNode = (node) => {
@@ -337,4 +368,5 @@ module.exports = isScriptAllowed
 module.exports.isScriptAllowed = isScriptAllowed
 module.exports.isExactVersionDisjunction = isExactVersionDisjunction
 module.exports.getTrustedRegistryIdentity = getTrustedRegistryIdentity
+module.exports.resolvedSourceSpecs = resolvedSourceSpecs
 module.exports.trustedDisplay = trustedDisplay

workspaces/arborist/test/script-allowed.js

@@ -118,6 +118,56 @@ t.test('file path — exact resolved match', t => {
   t.end()
 })
 
+t.test('file path — link target matches incoming link source', t => {
+  const targetPath = require('node:path').resolve('local-pkg')
+  const target = node({
+    name: 'local-pkg',
+    packageName: 'local-pkg',
+    version: '1.0.0',
+  })
+  target.resolved = null
+  target.path = targetPath
+  target.realpath = targetPath
+  target.linksIn = new Set([{ resolved: 'file:../local-pkg' }])
+
+  t.equal(isScriptAllowed(target, { 'file:../local-pkg': true }), true)
+  t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), true)
+  t.equal(isScriptAllowed(target, { 'file:../local-pkg': false }), false)
+  t.equal(isScriptAllowed(target, { 'file:../other': true }), null)
+  t.end()
+})
+
+t.test('file path — registry nodes do not match by install path', t => {
+  const reg = node({
+    name: 'sharp',
+    packageName: 'sharp',
+    version: '0.33.0',
+    path: 'node_modules/sharp',
+    realpath: require('node:path').resolve('node_modules/sharp'),
+    linksIn: new Set(),
+  })
+
+  t.equal(isScriptAllowed(reg, { 'file:node_modules/sharp': true }), null)
+  t.end()
+})
+
+t.test('file path — empty link sets do not add install paths', t => {
+  const targetPath = require('node:path').resolve('local-pkg')
+  const target = node({
+    name: 'local-pkg',
+    packageName: 'local-pkg',
+    version: '1.0.0',
+  })
+  target.resolved = null
+  target.path = targetPath
+  target.realpath = targetPath
+  target.linksIn = new Set()
+
+  t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), null)
+  t.equal(isScriptAllowed(target, { [targetPath]: true }), null)
+  t.end()
+})
+
 t.test('directory key — npa parses absolute paths as type=directory', t => {
   // npa treats absolute paths as { type: 'directory' }, which the
   // matcher shares with the 'file' case. path.resolve produces a

workspaces/arborist/test/unreviewed-scripts.js

@@ -118,6 +118,25 @@ t.test('collectUnreviewedScripts', async t => {
     t.equal(result[0].node.name, 'pending')
   })
 
+  t.test('skips reviewed local directory link targets', async t => {
+    const target = node({ name: 'local', scripts: { install: 'x' } })
+    target.resolved = null
+    target.isRegistryDependency = false
+    target.path = require('node:path').resolve('local')
+    target.realpath = target.path
+    target.linksIn = new Set([{ resolved: 'file:../local' }])
+
+    t.strictSame(await collectUnreviewedScripts({
+      tree: tree([target]),
+      policy: { 'file:../local': false },
+    }), [])
+
+    t.strictSame(await collectUnreviewedScripts({
+      tree: tree([target]),
+      policy: { 'file:local': true },
+    }), [])
+  })
+
   t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => {
     const collect = mockCollect(t, async (n) => {
       if (n.path === '/has-bindings') {