fix(allowScripts): close three enforcement gaps

lib/commands/link.js

@@ -5,6 +5,7 @@ const pkgJson = require('@npmcli/package-json')
 const semver = require('semver')
 const reifyFinish = require('../utils/reify-finish.js')
 const resolveAllowScripts = require('../utils/resolve-allow-scripts.js')
+const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js')
 const { patchRelaxOpts } = require('../utils/cli-only-flag.js')
 const ArboristWorkspaceCmd = require('../arborist-cmd.js')
 
@@ -69,13 +70,17 @@ class Link extends ArboristWorkspaceCmd {
     // load current packages from the global space, and then add symlinks installs locally
     const globalTop = resolve(this.npm.globalDir, '..')
     const Arborist = require('@npmcli/arborist')
+    // Resolve the policy up front so it also gates the global install of
+    // missing packages, not just the local link.
+    const { policy: allowScriptsPolicy } = await resolveAllowScripts(this.npm)
     const globalOpts = {
       ...this.npm.flatOptions,
       ...patchRelaxOpts(this.npm.config),
       Arborist,
       path: globalTop,
       global: true,
       prune: false,
+      allowScripts: allowScriptsPolicy,
     }
     const globalArb = new Arborist(globalOpts)
 
@@ -88,10 +93,17 @@ class Link extends ArboristWorkspaceCmd {
     // any extra arg that is missing from the current global space should be reified there first
     const missing = this.missingArgsFromTree(globals, args)
     if (missing.length) {
-      await globalArb.reify({
+      const globalReifyOpts = {
         ...globalOpts,
         add: missing,
+      }
+      // Gate the global install with the same preflight as `npm install`.
+      await strictAllowScriptsPreflight({
+        arb: globalArb,
+        npm: this.npm,
+        idealTreeOpts: globalReifyOpts,
       })
+      await globalArb.reify(globalReifyOpts)
     }
 
     // get a list of module names that should be linked in the local prefix
@@ -118,7 +130,6 @@ class Link extends ArboristWorkspaceCmd {
       )
     // create a new arborist instance for the local prefix and
     // reify all the pending names as symlinks there
-    const { policy: allowScriptsPolicy } = await resolveAllowScripts(this.npm)
     const localArb = new Arborist({
       ...this.npm.flatOptions,
       ...patchRelaxOpts(this.npm.config),

test/lib/commands/link.js

@@ -592,3 +592,73 @@ t.test('link threads allowScripts policy through to arborist', async t => {
   t.strictSame(localOpts.allowScripts, { canvas: true },
     'local arborist opts.allowScripts populated from package.json')
 })
+
+t.test('link threads allowScripts policy to the global install', async t => {
+  const capturedOpts = []
+  const FakeArborist = function (opts) {
+    capturedOpts.push(opts)
+    this.options = opts
+    this.actualTree = { inventory: new Map() }
+  }
+  FakeArborist.prototype.loadActual = async () => ({ isLink: false, children: new Map() })
+  FakeArborist.prototype.reify = async () => {}
+
+  const mock = await mockNpm(t, {
+    command: 'link',
+    prefixDir: {
+      'package.json': JSON.stringify({
+        name: 'host',
+        version: '1.0.0',
+        allowScripts: { canvas: true },
+      }),
+    },
+    mocks: {
+      '@npmcli/arborist': FakeArborist,
+      '{LIB}/utils/reify-finish.js': async () => {},
+    },
+  })
+  await mock.npm.exec('link', ['canvas'])
+  // the global Arborist is constructed first; its missing-package install
+  // must carry the project policy.
+  t.strictSame(capturedOpts[0].allowScripts, { canvas: true },
+    'global arborist opts.allowScripts populated from package.json')
+})
+
+t.test('link runs the strict-allow-scripts preflight before the global install', async t => {
+  // The mocked preflight stands in for strict mode finding an uncovered
+  // script. It must run and throw before the global reify.
+  const calls = []
+  const FakeArborist = function (opts) {
+    this.options = opts
+    this.actualTree = { inventory: new Map() }
+  }
+  FakeArborist.prototype.loadActual = async () => ({ isLink: false, children: new Map() })
+  FakeArborist.prototype.reify = async () => {
+    calls.push('reify')
+  }
+
+  const mock = await mockNpm(t, {
+    command: 'link',
+    prefixDir: {
+      'package.json': JSON.stringify({
+        name: 'host',
+        version: '1.0.0',
+      }),
+    },
+    mocks: {
+      '@npmcli/arborist': FakeArborist,
+      '{LIB}/utils/reify-finish.js': async () => {},
+      '{LIB}/utils/strict-allow-scripts-preflight.js': async () => {
+        calls.push('preflight')
+        throw Object.assign(new Error('blocked'), { code: 'ESTRICTALLOWSCRIPTS' })
+      },
+    },
+  })
+  await t.rejects(
+    mock.npm.exec('link', ['canvas']),
+    { code: 'ESTRICTALLOWSCRIPTS' },
+    'the strict preflight blocks the link'
+  )
+  t.strictSame(calls, ['preflight'],
+    'preflight ran and the global reify never executed')
+})

workspaces/arborist/lib/script-allowed.js

@@ -43,7 +43,9 @@ const isScriptAllowed = (node, policy) => {
   let anyDeny = false
 
   for (const [key, value] of Object.entries(policy)) {
-    if (!matches(node, key)) {
+    // Pass deny intent so matchRegistry can fail closed on an unverifiable
+    // version: a deny still blocks, an allow stays refused.
+    if (!matches(node, key, value === false)) {
       continue
     }
     if (value === false) {
@@ -66,7 +68,7 @@ const isScriptAllowed = (node, policy) => {
   return null
 }
 
-const matches = (node, key) => {
+const matches = (node, key, failClosed) => {
   let parsed
   try {
     parsed = npa(key)
@@ -78,7 +80,7 @@ const matches = (node, key) => {
     case 'tag':
     case 'range':
     case 'version':
-      return matchRegistry(node, parsed)
+      return matchRegistry(node, parsed, failClosed)
     case 'git':
       return matchGit(node, parsed)
     case 'file':
@@ -132,7 +134,7 @@ const resolvedSourceSpecs = (node) => {
   return specs
 }
 
-const matchRegistry = (node, parsed) => {
+const matchRegistry = (node, parsed, failClosed) => {
   // 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
   // even if their names happen to coincide.
@@ -168,16 +170,25 @@ const matchRegistry = (node, parsed) => {
     if (parsed.fetchSpec === '*' || parsed.rawSpec === '' || parsed.rawSpec === '*') {
       return true
     }
-    if (!trusted.version || !isExactVersionDisjunction(parsed.fetchSpec)) {
+    if (!isExactVersionDisjunction(parsed.fetchSpec)) {
       return false
     }
+    // Unverifiable version (omit-lockfile-registry-resolved): a deny blocks,
+    // an allow is refused.
+    if (!trusted.version) {
+      return failClosed
+    }
     return semver.satisfies(trusted.version, parsed.fetchSpec, { loose: true })
   }
 
   // `version` is an exact pin like `pkg@1.2.3`.
   /* istanbul ignore else: parsed.type at this point is always 'version';
      the istanbul-ignored fallback below handles the impossible case. */
   if (parsed.type === 'version') {
+    // Unverifiable version: a deny blocks, an allow is refused.
+    if (!trusted.version) {
+      return failClosed
+    }
     return trusted.version === parsed.fetchSpec
   }
 

workspaces/arborist/test/arborist/rebuild.js

@@ -118,6 +118,21 @@ t.test('do not run scripts if ignoreScripts=true', async t => {
   t.throws(() => fs.statSync(file), 'bundle build script not run')
 })
 
+t.test('bundled deps cannot be allowlisted: gate blocks their scripts', async t => {
+  // With the gate active, a bundled dep's script must not run even with an
+  // explicit allow: scripts only run when isScriptAllowed returns true, and
+  // bundled deps return null.
+  const path = fixture(t, 'testing-rebuild-bundle-reified')
+  const file = resolve(path, 'node_modules/@isaacs/testing-rebuild-bundle-a/node_modules/@isaacs/testing-rebuild-bundle-b/cwd')
+  t.throws(() => fs.statSync(file), 'file not there already (gut check)')
+  const arb = new Arborist({
+    path,
+    allowScripts: { '@isaacs/testing-rebuild-bundle-b': true },
+  })
+  await arb.rebuild()
+  t.throws(() => fs.statSync(file), 'bundled dep postinstall did not run despite explicit allow')
+})
+
 t.test('allowScripts deny entry skips the build set entry for that node', async t => {
   // Verifies the deny gate in #addToBuildSet: when `allowScripts` resolves
   // a node's policy to `false`, that node's scripts are skipped while

workspaces/arborist/test/script-allowed.js

@@ -229,6 +229,57 @@ t.test('omitLockfileRegistryResolved: name-only match via edges; version-pinned
   t.end()
 })
 
+t.test('omitLockfileRegistryResolved: version-pinned deny fails closed', t => {
+  // No resolved URL means no trusted version. A version-pinned deny must
+  // still block (fail closed); a matching allow stays refused.
+  const omitted = () => ({
+    name: 'evilpkg',
+    packageName: 'evilpkg',
+    version: '1.0.0',
+    resolved: undefined,
+    location: 'node_modules/evilpkg',
+    edgesIn: new Set([{ name: 'evilpkg', spec: '^1.0.0' }]),
+  })
+
+  // Exact-version deny: blocked.
+  t.equal(isScriptAllowed(omitted(), { 'evilpkg@1.0.0': false }), false,
+    'version-pinned deny blocks even without a trusted version')
+  // Exact-disjunction deny: blocked.
+  t.equal(isScriptAllowed(omitted(), { 'evilpkg@1.0.0 || 2.0.0': false }), false,
+    'exact-disjunction deny blocks even without a trusted version')
+  // The exploit: name-only allow + version-pinned deny. Deny still wins.
+  t.equal(isScriptAllowed(omitted(), { evilpkg: true, 'evilpkg@1.0.0': false }), false,
+    'deny wins over a name-only allow when the version is unverifiable')
+
+  // Allow stays strict: an unverifiable version is never authorized.
+  t.equal(isScriptAllowed(omitted(), { 'evilpkg@1.0.0 || 2.0.0': true }), null,
+    'exact-disjunction allow is refused without a trusted version')
+
+  // Name mismatch: fail-closed must not over-match a different package.
+  t.equal(isScriptAllowed(omitted(), { 'otherpkg@1.0.0': false }), null,
+    'a version-pinned deny for a different name does not match')
+
+  t.end()
+})
+
+t.test('omitLockfileRegistryResolved + alias: version-pinned deny fails closed', t => {
+  // `"trusted": "npm:naughty@1.0.0"`, resolved omitted. A deny on the
+  // underlying name must block; the alias name authorizes nothing.
+  const aliasOmitted = {
+    name: 'trusted',
+    packageName: 'naughty',
+    version: '1.0.0',
+    resolved: undefined,
+    location: 'node_modules/trusted',
+    edgesIn: new Set([{ name: 'trusted', spec: 'npm:naughty@1.0.0' }]),
+  }
+  t.equal(isScriptAllowed(aliasOmitted, { 'naughty@1.0.0': false }), false,
+    'underlying-name version deny blocks the aliased package')
+  t.equal(isScriptAllowed(aliasOmitted, { 'trusted@1.0.0': false }), null,
+    'alias-name version deny does not match the underlying package')
+  t.end()
+})
+
 t.test('omitLockfileRegistryResolved + alias: location is ignored; underlying name wins', t => {
   // Consumer's package.json has `"trusted": "npm:naughty@1.0.0"`. With
   // omitLockfileRegistryResolved, the resolved URL is absent. The install