diff --git a/lib/utils/allow-scripts-cmd.js b/lib/utils/allow-scripts-cmd.js index f07dc8d1dc504..ebbaeb76bbad3 100644 --- a/lib/utils/allow-scripts-cmd.js +++ b/lib/utils/allow-scripts-cmd.js @@ -2,7 +2,7 @@ const { log, output } = require('proc-log') const npa = require('npm-package-arg') const semver = require('semver') const pkgJson = require('@npmcli/package-json') -const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') +const { trustedDisplay, isBundledByDependency } = require('@npmcli/arborist/lib/script-allowed.js') const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js') const checkAllowScripts = require('./check-allow-scripts.js') const resolveAllowScripts = require('./resolve-allow-scripts.js') @@ -188,9 +188,10 @@ class AllowScriptsCmd extends BaseCommand { output.standard('No packages with unreviewed install scripts.') return } - // Bundled dependencies never appear in `unreviewed` (checkAllowScripts - // skips them because they never run their install scripts and cannot - // be allowlisted), so there is nothing extra to filter here. + // Dep-bundled dependencies never appear in `unreviewed` (checkAllowScripts + // skips them because they never run their install scripts and cannot be + // allowlisted); root-bundled deps do appear and are approved like any other + // reviewable dep. Either way there is nothing extra to filter here. const groups = this.groupByPackage(unreviewed.map(({ node }) => node)) await this.writePolicyChanges(groups) } @@ -219,15 +220,19 @@ class AllowScriptsCmd extends BaseCommand { // Match positional args against each node's trusted name. Registry deps // use the URL-derived name; non-registry deps fall back to the dependency // edge name. A version or range on the arg narrows the match to installed - // versions that satisfy it. Bundled deps are excluded for the same reason - // as --all. Args that match nothing are returned in `unmatched`. + // versions that satisfy it. Dep-bundled deps are excluded for the same + // reason as --all (they never run their install scripts and can't be + // allowlisted); root-bundled deps are kept, matching the inDepBundle gate + // used by collectUnreviewedScripts so `approve ` can allow the same + // packages the pending list tells the user to approve (npm/cli#9679). Args + // that match nothing are returned in `unmatched`. 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 || isBundledByDependency(node)) { continue } const { name, version } = trustedDisplay(node) @@ -256,7 +261,7 @@ class AllowScriptsCmd extends BaseCommand { const groups = {} for (const node of nodes) { const key = nameKeyFor(node) - /* istanbul ignore if: callers prefilter via inBundle and trustedDisplay so untrusted nodes don't reach here */ + /* istanbul ignore if: callers prefilter via isBundledByDependency and trustedDisplay so untrusted nodes don't reach here */ if (!key) { log.warn( this.logTitle, @@ -355,10 +360,14 @@ class AllowScriptsCmd extends BaseCommand { // Candidate install nodes (mirrors collectUnreviewedScripts), tagged with // whether each has install scripts so the classifier can tell "gone" from - // "no longer has scripts". + // "no longer has scripts". Uses isBundledByDependency (not node.inBundle) + // so root-bundled deps stay in the candidate set: they are reviewable and + // may hold an allowScripts entry the user just approved, and pruning them + // out would silently delete that entry (npm/cli#9679). const nodes = [] for (const node of arb.actualTree.inventory.values()) { - if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle || node.inert) { + if (node.isProjectRoot || node.isWorkspace || node.isLink || + isBundledByDependency(node) || node.inert) { continue } const scripts = await getInstallScripts(node) diff --git a/test/lib/commands/approve-scripts.js b/test/lib/commands/approve-scripts.js index 4ae7a9875b6a1..941f983a3cf4d 100644 --- a/test/lib/commands/approve-scripts.js +++ b/test/lib/commands/approve-scripts.js @@ -859,3 +859,73 @@ t.test('approve-scripts --all with only bundled deps has nothing to review', asy const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) t.notOk(pkg.allowScripts, 'no allowScripts written') }) + +// Root-bundled deps (listed in the ROOT project's bundleDependencies) are +// fetched from the registry at install time, run their install scripts, and +// are reviewable — unlike deps bundled inside another package's tarball. The +// command layer must approve them by name, mirroring the inDepBundle gate the +// pending list / preflight use (npm/cli#9679). Before the fix, findNodesForArgs +// filtered on node.inBundle (true for root-bundled deps too), so +// `approve-scripts ` threw ENOMATCH even though `--all` and the pending +// list surfaced the dep. +const rootBundledFixture = () => ({ + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + dependencies: { 'root-bundled-dep': '*' }, + bundleDependencies: ['root-bundled-dep'], + }), + 'package-lock.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'host', + version: '1.0.0', + dependencies: { 'root-bundled-dep': '*' }, + bundleDependencies: ['root-bundled-dep'], + }, + 'node_modules/root-bundled-dep': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/root-bundled-dep/-/root-bundled-dep-1.0.0.tgz', + hasInstallScript: true, + }, + }, + }), + node_modules: { + 'root-bundled-dep': { + 'package.json': JSON.stringify({ + name: 'root-bundled-dep', + version: '1.0.0', + scripts: { install: 'echo install' }, + }), + }, + }, +}) + +t.test('approve-scripts by name is approved (npm/cli#9679)', async t => { + const { npm, prefix } = await _mockNpm(t, { + prefixDir: rootBundledFixture(), + }) + // The bug: this threw ENOMATCH because findNodesForArgs skipped the node + // on node.inBundle. It must now match and write the policy entry. + await npm.exec('approve-scripts', ['root-bundled-dep']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'root-bundled-dep@1.0.0': true }, + 'root-bundled dep is approved by name, not rejected with ENOMATCH') +}) + +t.test('approve-scripts --all approves a root-bundled dep (npm/cli#9679)', async t => { + const { npm, prefix } = await _mockNpm(t, { + prefixDir: rootBundledFixture(), + config: { all: true }, + }) + await npm.exec('approve-scripts', []) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'root-bundled-dep@1.0.0': true }, + 'root-bundled dep surfaces in --all and is approved') +}) diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 8c9b3fe118a8e..d8808895d776d 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -23,15 +23,72 @@ const versionFromTgz = require('./version-from-tgz.js') // - git: match on hosted.ssh() plus a short-SHA prefix of the // resolved committish +// True when the node was bundled by some dependency (not by the root +// project). The script-approval gate keys off this rather than +// node.inBundle so the user can review and allow scripts in deps the +// root explicitly bundles, while still blocking install scripts from +// tarballs nested inside another package (where manifest-confusion makes +// identity matching unsafe). +// +// Real Node (workspaces/arborist/lib/node.js): inDepBundle is a +// getBundler() walk that excludes the root project, so it is the +// trusted bit and we use it directly. +// +// IsolatedNode (workspaces/arborist/lib/isolated-classes.js): linked / +// isolated mode constructs bundled IsolatedNodes by walking from a +// bundle entry point and the class does not track root-vs-dep bundling, +// so its inDepBundle always returns false. To preserve the existing +// security gate (no manifest confusion via isolated bundled nodes), we +// treat any inBundle IsolatedNode as dep-bundled. The discriminator +// versus a real-Node root-bundled dep is getBundler(): real Nodes walk +// up to the root and return it; IsolatedNode.getBundler returns null. +const isBundledByDependency = (node) => { + if (!node) { + return false + } + if (node.inDepBundle) { + return true + } + if (!node.inBundle) { + return false + } + // inBundle is true and inDepBundle is false. For real Nodes this means + // root-bundled (allow review). For IsolatedNode it means "bundled in + // isolated mode, root/dep distinction not tracked" (must block). The + // discriminator is whether getBundler() can identify a real bundling + // parent; IsolatedNode.getBundler returns null. + if (typeof node.getBundler !== 'function') { + return false + } + return node.getBundler() === null +} + const isScriptAllowed = (node, policy) => { - // Bundled dependencies never run their install scripts and cannot be - // allowlisted. Matching by name@version from the bundled tarball would - // reintroduce manifest confusion (a bundled tarball can claim any name - // and version). Returning null marks them as not-allowed regardless of - // any policy entry, so their install scripts are blocked by the - // install-time gate. A package that needs a bundled dep's script must - // forward it as one of its own lifecycle scripts. - if (node.inBundle) { + // Dependencies bundled inside another package's tarball never run their + // install scripts and cannot be allowlisted. Matching by name@version + // from the bundled tarball would reintroduce manifest confusion (a + // bundled tarball can claim any name and version). Returning null marks + // them as not-allowed regardless of any policy entry, so their install + // scripts are blocked by the install-time gate. A package that needs a + // bundled dep's script must forward it as one of its own lifecycle + // scripts. + // + // Real Nodes: inDepBundle is the trusted bit. inBundle is also true + // when the root project itself bundles a dep, but root-bundled deps + // are still fetched from the registry at install time, have a trusted + // resolved URL, run their install scripts, and so must be reviewable + // (npm/cli#9679). Keying off inBundle would silently block them and + // hide them from the pending list. + // + // IsolatedNodes (linked/isolated mode): the class can't tell root- + // bundled from dep-bundled, so its inDepBundle getter always returns + // false. Anything an IsolatedNode reports as inBundle came through + // the isolated reifier's bundled tree and must keep being blocked, or + // a bundled tarball's identity match would bypass the gate. The + // getBundler()===null discriminator below catches that case without + // collapsing the root-bundled / dep-bundled distinction elsewhere + // (diff/reify/edge all key off inDepBundle in real-Node semantics). + if (isBundledByDependency(node)) { return null } @@ -382,3 +439,4 @@ module.exports.isExactVersionDisjunction = isExactVersionDisjunction module.exports.getTrustedRegistryIdentity = getTrustedRegistryIdentity module.exports.resolvedSourceSpecs = resolvedSourceSpecs module.exports.trustedDisplay = trustedDisplay +module.exports.isBundledByDependency = isBundledByDependency diff --git a/workspaces/arborist/lib/unreviewed-scripts.js b/workspaces/arborist/lib/unreviewed-scripts.js index f7bed387845ec..0c2f64ab2fc13 100644 --- a/workspaces/arborist/lib/unreviewed-scripts.js +++ b/workspaces/arborist/lib/unreviewed-scripts.js @@ -1,4 +1,4 @@ -const isScriptAllowed = require('./script-allowed.js') +const { isScriptAllowed, isBundledByDependency } = require('./script-allowed.js') const getInstallScripts = require('./install-scripts.js') // Shared allowScripts walk used by both the npm CLI @@ -42,12 +42,18 @@ const collectUnreviewedScripts = async ({ // Linked workspace dependencies are managed by the workspace owner. continue } - if (node.inBundle) { - // Bundled dependencies never run their install scripts and cannot be - // allowlisted, so they are never "pending". Skipping them keeps them - // out of the advisory warning and out of strict-allow-scripts. A - // package that needs a bundled dep's script must forward it as one of - // its own lifecycle scripts. + if (isBundledByDependency(node)) { + // Dependencies bundled inside another package's tarball never run + // their install scripts and cannot be allowlisted, so they are never + // "pending". Skipping them keeps them out of the advisory warning + // and out of strict-allow-scripts. A package that needs a bundled + // dep's script must forward it as one of its own lifecycle scripts. + // + // Uses isBundledByDependency (not node.inBundle): the root + // project's bundleDependencies list only controls what ships in + // the root's published tarball; at install time those deps come + // from the registry like any other direct dep, so their install + // scripts must still surface as pending review (npm/cli#9679). continue } if (node.inert) { diff --git a/workspaces/arborist/test/script-allowed.js b/workspaces/arborist/test/script-allowed.js index 218ccf1e28888..10c811a98e515 100644 --- a/workspaces/arborist/test/script-allowed.js +++ b/workspaces/arborist/test/script-allowed.js @@ -1,6 +1,6 @@ const t = require('tap') const isScriptAllowed = require('../lib/script-allowed.js') -const { trustedDisplay } = isScriptAllowed +const { trustedDisplay, isBundledByDependency } = isScriptAllowed // Test nodes default to a consistent registry-tarball shape: the resolved // URL's name+version match the supplied name+version. Tests that need to @@ -415,18 +415,19 @@ t.test('isRegistryNode — arborist isRegistryDependency true accepts even unusu t.equal(isScriptAllowed(arboristNode, { 'trusted@1.0.0': true }), true) }) -t.test('bundled deps cannot be allowlisted (never run)', async t => { - // Bundled dependencies have inBundle=true and no independent resolved - // URL. They can never be allowlisted because matching by name@version - // from the bundled tarball would reintroduce manifest confusion. They - // always return null, and their install scripts never run. +t.test('dep-bundled deps cannot be allowlisted (never run)', async t => { + // Dependencies bundled inside another package's tarball have + // inDepBundle=true and no independent resolved URL. They can never be + // allowlisted because matching by name@version from the bundled + // tarball would reintroduce manifest confusion. They always return + // null, and their install scripts never run. const bundled = { name: 'bundled-pkg', packageName: 'bundled-pkg', version: '1.0.0', resolved: undefined, - inBundle: true, + inDepBundle: true, } // Name-only allow: must NOT match a bundled dep. @@ -440,33 +441,115 @@ t.test('bundled deps cannot be allowlisted (never run)', async t => { t.equal(isScriptAllowed(bundled, null), null) }) -t.test('bundled deps: deny entry does not match either (returns null, not false)', async t => { - // A deny entry doesn't apply to bundled deps because they're outside - // the policy scope entirely. They're blocked because they never run, - // not via a policy entry. +t.test('dep-bundled deps: deny entry does not match either (returns null, not false)', async t => { + // A deny entry doesn't apply to dep-bundled deps because they're + // outside the policy scope entirely. They're blocked because they + // never run, not via a policy entry. const bundled = { name: 'bundled-pkg', packageName: 'bundled-pkg', version: '1.0.0', resolved: undefined, - inBundle: true, + inDepBundle: true, } t.equal(isScriptAllowed(bundled, { 'bundled-pkg': false }), null) }) -t.test('bundled dep with resolved field is still rejected', async t => { - // Defensive: even if a bundled dep somehow has a resolved URL, the - // inBundle flag wins over identity matching. +t.test('dep-bundled dep with resolved field is still rejected', async t => { + // Defensive: even if a dep-bundled dep somehow has a resolved URL, + // the inDepBundle flag wins over identity matching. const bundledWithResolved = { name: 'pkg', packageName: 'pkg', version: '1.0.0', resolved: 'https://registry.npmjs.org/pkg/-/pkg-1.0.0.tgz', - inBundle: true, + inDepBundle: true, } t.equal(isScriptAllowed(bundledWithResolved, { 'pkg@1.0.0': true }), null) }) +t.test('root-bundled deps remain allowlistable (npm/cli#9679)', async t => { + // The root project's bundleDependencies list only controls what ships + // in the root's published tarball. At install time those deps are + // fetched from the registry like any other direct dep, so they: + // - have a trusted resolved URL (no manifest confusion), + // - actually run install scripts during reify, and + // - must therefore be reviewable via allowScripts. + // + // Real Nodes report inBundle=true / inDepBundle=false for root-bundled + // deps. The gate must key off inDepBundle, not inBundle, or the user + // can never approve their own bundled deps' install scripts and the + // scripts are silently skipped. + const rootBundled = { + name: 'pkg', + packageName: 'pkg', + version: '1.0.0', + resolved: 'https://registry.npmjs.org/pkg/-/pkg-1.0.0.tgz', + inBundle: true, + inDepBundle: false, + isRegistryDependency: true, + } + + t.equal(isScriptAllowed(rootBundled, { pkg: true }), true, + 'name-only allow matches a root-bundled dep') + t.equal(isScriptAllowed(rootBundled, { 'pkg@1.0.0': true }), true, + 'versioned allow matches a root-bundled dep') + t.equal(isScriptAllowed(rootBundled, { pkg: false }), false, + 'deny entry blocks a root-bundled dep') + t.equal(isScriptAllowed(rootBundled, null), null, + 'unreviewed root-bundled dep returns null (blocks scripts, surfaces in pending)') +}) + +t.test('root-bundled deps: real Node getBundler()===root path (npm/cli#9679)', async t => { + // The plain-object fixture above only exercises the + // `typeof getBundler !== 'function'` fallback in isBundledByDependency. + // Build a genuine arborist Node whose root lists it in + // bundleDependencies so the real getBundler() walk runs: it returns the + // root, giving inBundle=true / inDepBundle=false and + // getBundler()===root (not null). That is the discriminator that keeps + // root-bundled deps reviewable while isolated bundled nodes + // (getBundler()===null) stay blocked. Modeled on the IsolatedNode-based + // test below, but with a real Node instance. + const Node = require('../lib/node.js') + + const root = new Node({ + pkg: { + name: 'root', + version: '1.0.0', + dependencies: { 'root-bundled-dep': '1.x' }, + bundleDependencies: ['root-bundled-dep'], + }, + path: '/project', + realpath: '/project', + }) + const rootBundled = new Node({ + pkg: { name: 'root-bundled-dep', version: '1.0.0' }, + resolved: 'https://registry.npmjs.org/root-bundled-dep/-/root-bundled-dep-1.0.0.tgz', + parent: root, + }) + + // Sanity-check the real getters that drive the gate. + t.equal(rootBundled.inBundle, true, 'real root-bundled Node reports inBundle') + t.equal(rootBundled.inDepBundle, false, + 'real root-bundled Node is not dep-bundled (bundler is the root)') + t.equal(rootBundled.getBundler(), root, + 'getBundler() walks to the root, not null (the real-Node path)') + t.equal(typeof rootBundled.getBundler, 'function', + 'real Node has a getBundler method (not the plain-object fallback)') + t.equal(isBundledByDependency(rootBundled), false, + 'isBundledByDependency is false for a real root-bundled Node') + + // The gate must let the user review/allow the dep's scripts. + t.equal(isScriptAllowed(rootBundled, { 'root-bundled-dep': true }), true, + 'name-only allow matches a real root-bundled Node') + t.equal(isScriptAllowed(rootBundled, { 'root-bundled-dep@1.0.0': true }), true, + 'versioned allow matches a real root-bundled Node') + t.equal(isScriptAllowed(rootBundled, { 'root-bundled-dep': false }), false, + 'deny entry blocks a real root-bundled Node') + t.equal(isScriptAllowed(rootBundled, null), null, + 'unreviewed real root-bundled Node returns null (blocks scripts, surfaces in pending)') +}) + t.test('inBundle: false does not affect normal matching', async t => { // Sanity check: explicit inBundle: false behaves identically to absent. const normal = { @@ -482,9 +565,9 @@ t.test('inBundle: false does not affect normal matching', async t => { t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => { // Regression guard: in isolated/linked mode the gate runs against // IsolatedNode instances, not real Nodes. A bundled IsolatedNode must - // report inBundle so the gate blocks it even when its resolved URL + // report inDepBundle so the gate blocks it even when its resolved URL // looks like a registry identity that a name entry would otherwise - // match. Without inBundle on IsolatedNode the guard is silently + // match. Without inDepBundle on IsolatedNode the guard is silently // skipped and the bundled install script runs. const { IsolatedNode } = require('../lib/isolated-classes.js') @@ -498,7 +581,10 @@ t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => { }) t.equal(bundled.inBundle, true, 'bundled IsolatedNode reports inBundle') - t.equal(isScriptAllowed(bundled, { 'bundled-pkg': true }), null) + t.equal(bundled.inDepBundle, false, + 'IsolatedNode inDepBundle is always false (class does not track it)') + t.equal(isScriptAllowed(bundled, { 'bundled-pkg': true }), null, + 'isolated bundled node is still blocked via getBundler()===null fallback') t.equal(isScriptAllowed(bundled, { 'bundled-pkg@1.0.0': true }), null) const store = new IsolatedNode({ @@ -512,6 +598,7 @@ t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => { }) t.equal(store.inBundle, false, 'external store IsolatedNode is not bundled') + t.equal(store.inDepBundle, false, 'external store IsolatedNode is not dep-bundled') t.equal(isScriptAllowed(store, { 'store-pkg@1.0.0': true }), true) }) diff --git a/workspaces/arborist/test/unreviewed-scripts.js b/workspaces/arborist/test/unreviewed-scripts.js index 34e4878b4625a..40767bfa90b09 100644 --- a/workspaces/arborist/test/unreviewed-scripts.js +++ b/workspaces/arborist/test/unreviewed-scripts.js @@ -28,6 +28,7 @@ const node = ({ isWorkspace = false, isLink = false, inBundle = false, + inDepBundle = false, inert = false, resolved, } = {}) => ({ @@ -40,6 +41,7 @@ const node = ({ isWorkspace, isLink, inBundle, + inDepBundle, inert, isRegistryDependency: true, package: { name, version, scripts }, @@ -71,19 +73,41 @@ t.test('collectUnreviewedScripts', async t => { t.strictSame(await collectUnreviewedScripts(), []) }) - t.test('skips project root, workspace, linked, and bundled nodes', async t => { + t.test('skips project root, workspace, linked, and dep-bundled nodes', async t => { const result = await collectUnreviewedScripts({ tree: tree([ node({ name: 'root', scripts: { install: 'x' }, isProjectRoot: true }), node({ name: 'ws', scripts: { install: 'x' }, isWorkspace: true }), node({ name: 'linked', scripts: { install: 'x' }, isLink: true }), - node({ name: 'bundled', scripts: { install: 'x' }, inBundle: true }), + node({ name: 'bundled', scripts: { install: 'x' }, inDepBundle: true }), ]), policy: null, }) t.strictSame(result, []) }) + t.test('root-bundled deps are still listed as pending (npm/cli#9679)', async t => { + // Deps listed in the root's bundleDependencies are fetched from the + // registry at install time, run their install scripts, and so must + // still surface in the unreviewed/pending list so the user can + // approve them. inBundle is true for these (any bundler), but + // inDepBundle is false (the bundler is the root project). + const result = await collectUnreviewedScripts({ + tree: tree([ + node({ + name: 'root-bundled', + scripts: { install: 'build.js' }, + inBundle: true, + inDepBundle: false, + }), + ]), + policy: null, + }) + t.equal(result.length, 1, 'root-bundled dep with install script is pending') + t.equal(result[0].node.name, 'root-bundled') + t.match(result[0].scripts, { install: 'build.js' }) + }) + t.test('skips inert (platform/engine-incompatible) optional nodes', async t => { const result = await collectUnreviewedScripts({ tree: tree([node({ name: 'fsevents', scripts: { install: 'x' }, inert: true })]),