diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index 49b70f72ae534..a99d328f5d294 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -199,10 +199,21 @@ module.exports = cls => class Builder extends cls { const { package: { bin, scripts = {} } } = node.target const { preinstall, install, postinstall, prepare } = scripts const tests = { bin, preinstall, install, postinstall, prepare } + // A denied package (allowScripts resolves to `false`) still gets its + // bins linked; only its lifecycle scripts are skipped, matching + // --ignore-scripts (npm/cli#9681). --dangerously-allow-all-scripts + // bypasses the gate. + const scriptsDenied = + !this.options.dangerouslyAllowAllScripts && + isScriptAllowed(node, this.options.allowScripts) === false for (const [key, has] of Object.entries(tests)) { - if (has) { - this.#queues[key].push(node) + if (!has) { + continue } + if (key !== 'bin' && scriptsDenied) { + continue + } + this.#queues[key].push(node) } } timeEnd() @@ -226,18 +237,6 @@ module.exports = cls => class Builder extends cls { return } - // Phase 1 allowScripts gate: a `false` verdict from the policy matcher - // means the user explicitly denied install scripts for this node, so skip - // it. `true` and `null` (unreviewed) both fall through to the existing - // detection logic — unreviewed nodes still run their scripts in Phase 1 - // and are surfaced via the post-reify advisory warning. The global - // --ignore-scripts kill switch in #build() still takes precedence, and - // --dangerously-allow-all-scripts bypasses this gate entirely. - if (!this.options.dangerouslyAllowAllScripts && - isScriptAllowed(node, this.options.allowScripts) === false) { - return - } - if (this.#oldMeta === null) { const { root: { meta } } = node this.#oldMeta = meta && meta.loadedFromDisk && diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index df2eac9eec25b..737d04d91545b 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -114,10 +114,9 @@ t.test('do not run scripts if ignoreScripts=true', async t => { t.throws(() => fs.statSync(file), 'bundle build script not run') }) -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 - // others continue to run. +t.test('allowScripts deny entry skips scripts for the denied node', async t => { + // Deny gate in #buildQueues: a `false` policy skips that node's + // scripts while others still run. const path = fixture(t, 'testing-rebuild-script-env-flags') const arb = newArb({ path, @@ -154,6 +153,46 @@ t.test('dangerouslyAllowAllScripts bypasses the deny gate', async t => { ) }) +t.test('allowScripts deny keeps the bin link (npm/cli#9681)', async t => { + // Denying a package's scripts must not drop its bin links: the script + // is skipped but `.bin/` is still created, like --ignore-scripts. + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'project', + version: '1.0.0', + dependencies: { 'denied-dep': '1.0.0' }, + }), + node_modules: { + 'denied-dep': { + 'package.json': JSON.stringify({ + name: 'denied-dep', + version: '1.0.0', + bin: { 'denied-bin': 'index.js' }, + scripts: { + postinstall: `node -e "require('fs').writeFileSync('ran', '1')"`, + }, + }), + 'index.js': '#!/usr/bin/env node\nconsole.log("hi")\n', + }, + }, + }) + const arb = newArb({ + path, + allowScripts: { 'denied-dep': false }, + }) + await arb.rebuild() + + t.throws( + () => fs.statSync(resolve(path, 'node_modules/denied-dep/ran')), + 'denied postinstall did not run' + ) + t.equal( + fs.statSync(resolve(path, 'node_modules/.bin/denied-bin')).isFile(), + true, + 'denied package bin link is still created' + ) +}) + t.test('do nothing if ignoreScripts=true and binLinks=false', async t => { 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')