diff --git a/docs/lib/content/configuring-npm/package-json.md b/docs/lib/content/configuring-npm/package-json.md index 56724afe699ab..6ca2b8fa1bf0c 100644 --- a/docs/lib/content/configuring-npm/package-json.md +++ b/docs/lib/content/configuring-npm/package-json.md @@ -554,6 +554,9 @@ To prevent npm from automatically building your module with node-gyp, set `gypfi } ``` +This setting disables only the automatic `node-gyp rebuild` script. +It does not disable explicit lifecycle scripts in `package.json`. + This is useful for packages that include native addons but want to handle the build process differently, or packages that have a binding.gyp file but should not be built as a native addon. ### config @@ -1262,7 +1265,8 @@ npm will default some values based on package contents. * `"scripts":{"install": "node-gyp rebuild"}` - If there is a `binding.gyp` file in the root of your package and you have not defined an `install` or `preinstall` script, npm will default the `install` command to compile using node-gyp. + If the root of your package contains `binding.gyp`, npm uses `node-gyp rebuild` as the default `install` script. + An explicit `install` or `preinstall` script, or `"gypfile": false`, disables this default. * `"contributors": [...]` diff --git a/docs/lib/content/using-npm/scripts.md b/docs/lib/content/using-npm/scripts.md index 4db79686c2e19..3b0751edc9d3e 100644 --- a/docs/lib/content/using-npm/scripts.md +++ b/docs/lib/content/using-npm/scripts.md @@ -145,6 +145,8 @@ Because `preinstall` runs before reify, scripts cannot rely on packages from `no If there is a `binding.gyp` file in the root of your package and you haven't defined your own `install` or `preinstall` scripts, npm will default the `install` command to compile using node-gyp via `node-gyp rebuild` +Setting `"gypfile": false` disables this default and leaves explicit lifecycle scripts unchanged. + These are run from the scripts of `` #### [`npm pack`](/commands/npm-pack) diff --git a/lib/utils/check-allow-scripts.js b/lib/utils/check-allow-scripts.js index 47984803cde7b..1264ce0d360f8 100644 --- a/lib/utils/check-allow-scripts.js +++ b/lib/utils/check-allow-scripts.js @@ -15,13 +15,16 @@ const { collectUnreviewedScripts } = require('@npmcli/arborist/lib/unreviewed-sc // `includeWhenIgnored` keeps listing unreviewed packages even when // ignore-scripts is set, so approve/deny can show what you'd move from a // blanket ignore-scripts to an allowlist. Execution callers leave it false. -const checkAllowScripts = async ({ arb, npm, tree, includeWhenIgnored = false }) => +const checkAllowScripts = async ({ + arb, npm, tree, includeWhenIgnored = false, readFromDisk = true, +}) => collectUnreviewedScripts({ tree: tree || arb.actualTree, policy: arb.options?.allowScripts || null, ignoreScripts: !!arb.options?.ignoreScripts, dangerouslyAllowAllScripts: !!npm?.flatOptions?.dangerouslyAllowAllScripts, includeWhenIgnored, + readFromDisk, }) module.exports = checkAllowScripts diff --git a/lib/utils/strict-allow-scripts-preflight.js b/lib/utils/strict-allow-scripts-preflight.js index 0c500018184c2..34bfb7cd6cb50 100644 --- a/lib/utils/strict-allow-scripts-preflight.js +++ b/lib/utils/strict-allow-scripts-preflight.js @@ -33,7 +33,9 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => { tree = arb.actualTree } - const unreviewed = await checkAllowScripts({ arb, npm, tree }) + const unreviewed = await checkAllowScripts({ + arb, npm, tree, readFromDisk: idealTreeOpts === undefined, + }) if (unreviewed.length === 0) { return } diff --git a/test/lib/commands/ci.js b/test/lib/commands/ci.js index a90ca4b0cffe6..0cb157abed417 100644 --- a/test/lib/commands/ci.js +++ b/test/lib/commands/ci.js @@ -49,6 +49,66 @@ const abbrev = { test: 'test file', } +t.test('ci respects gypfile:false in installed package data', async t => { + require('../../fixtures/tspawk')(t) + const cases = [ + { name: 'default policy' }, + { name: 'stale presence flag', hasInstallScript: true }, + { name: 'approved', allowScripts: { 'abbrev@1.0.0': true } }, + { name: 'bypass', config: { 'dangerously-allow-all-scripts': true } }, + { name: 'ignored', config: { 'ignore-scripts': true } }, + { name: 'strict', config: { 'strict-allow-scripts': true } }, + ] + for (const lockfileVersion of [2, 3]) { + for (const { name, config, allowScripts, hasInstallScript } of cases) { + await t.test(`v${lockfileVersion}: ${name}`, async t => { + const lock = { + ...packageLock, + lockfileVersion, + packages: { + ...packageLock.packages, + 'node_modules/abbrev': { + ...packageLock.packages['node_modules/abbrev'], + ...(hasInstallScript ? { hasInstallScript } : {}), + }, + }, + } + if (lockfileVersion === 3) { + delete lock.dependencies + } + const { npm, registry, logs } = await loadMockNpm(t, { + config: { audit: false, ...config }, + prefixDir: { + 'package.json': JSON.stringify({ ...packageJson, allowScripts }), + 'package-lock.json': JSON.stringify(lock), + node_modules: { stale: 'removed by ci' }, + abbrev: { + 'package.json': JSON.stringify({ name: 'abbrev', version: '1.0.0', gypfile: false }), + 'binding.gyp': '', + }, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + await npm.exec('ci', []) + + t.ok(fs.existsSync(path.join(npm.prefix, 'node_modules/abbrev/binding.gyp')), + 'installs the native package') + t.notOk(fs.existsSync(path.join(npm.prefix, 'node_modules/stale')), 'cleans node_modules') + t.strictSame(logs.warn.byTitle('install-scripts'), [], 'no blocked-script warning') + t.strictSame( + JSON.parse(fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8')), + lock, + 'ci does not rewrite the project lockfile' + ) + }) + } + } +}) + t.test('reifies, but doesn\'t remove node_modules because --dry-run', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { config: { diff --git a/test/lib/commands/install-scripts.js b/test/lib/commands/install-scripts.js index fd7051c7e4419..24b2d5a659b0d 100644 --- a/test/lib/commands/install-scripts.js +++ b/test/lib/commands/install-scripts.js @@ -148,6 +148,52 @@ t.test('install-scripts ls with no unreviewed says so', async t => { t.match(joinedOutput(), /No packages with unreviewed install scripts/) }) +t.test('install-scripts respects gypfile:false through the hidden lockfile', async t => { + for (const hasInstallScript of [false, true]) { + for (const command of ['ls', 'approve', 'prune']) { + await t.test(`${command}, hasInstallScript=${hasInstallScript}`, async t => { + const fixture = setupProject({ + withScripts: [], + noScripts: ['native-pkg'], + allowScripts: command === 'prune' ? { 'native-pkg': true } : undefined, + }) + fixture.node_modules['native-pkg'] = { + 'package.json': JSON.stringify({ + name: 'native-pkg', + version: '1.0.0', + gypfile: false, + }), + 'binding.gyp': '', + } + const lock = JSON.parse(fixture['package-lock.json']) + if (hasInstallScript) { + lock.packages['node_modules/native-pkg'].hasInstallScript = true + } + fixture['package-lock.json'] = JSON.stringify(lock) + fixture.node_modules['.package-lock.json'] = JSON.stringify(lock) + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: fixture, + config: { all: command === 'approve' }, + }) + const Arborist = require('@npmcli/arborist') + const tree = await new Arborist({ path: prefix }).loadActual() + t.equal(tree.meta.hiddenLockfile, true, 'uses the hidden lockfile') + t.equal(tree.children.get('native-pkg').package.gypfile, undefined, + 'tree metadata omits the opt-out') + + await npm.exec('install-scripts', [command]) + + if (command === 'ls') { + t.match(joinedOutput(), /No packages with unreviewed install scripts/) + } else { + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk(pkg.allowScripts, 'does not retain an unnecessary approval') + } + }) + } + } +}) + t.test('install-scripts ls rejects positional args', async t => { const { npm } = await mockNpm(t, { prefixDir: setupProject({ withScripts: ['canvas'] }), diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 3961b7f02e0c1..2d9a76bc2a432 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -33,6 +33,173 @@ const packageJson = { }, } +t.test('gypfile:false survives fresh, lockfile, and repeated installs', async t => { + const cases = [ + { name: 'default policy' }, + { name: 'approved', allowScripts: { 'abbrev@1.0.0': true } }, + { name: 'bypass', config: { 'dangerously-allow-all-scripts': true } }, + { name: 'ignored', config: { 'ignore-scripts': true } }, + { name: 'strict', config: { 'strict-allow-scripts': true } }, + ] + for (const { name, config, allowScripts } of cases) { + await t.test(name, async t => { + const { npm, registry, logs } = await loadMockNpm(t, { + config: { audit: false, ...config }, + prefixDir: { + 'package.json': JSON.stringify({ ...packageJson, allowScripts }), + abbrev: { + 'package.json': JSON.stringify({ name: 'abbrev', version: '1.0.0', gypfile: false }), + 'binding.gyp': '', + }, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.package({ manifest }) + + for (const phase of ['fresh', 'lockfile', 'repeated']) { + if (phase === 'lockfile') { + fs.rmSync(path.join(npm.prefix, 'node_modules'), { recursive: true, force: true }) + fs.rmSync(path.join(npm.config.get('cache'), '_cacache'), { recursive: true, force: true }) + } + if (phase !== 'repeated') { + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + } + await npm.exec('install', []) + + const lock = JSON.parse(fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8')) + t.notOk(lock.packages['node_modules/abbrev'].hasInstallScript, + `${phase}: no false script-presence flag`) + t.notOk('gypfile' in lock.packages['node_modules/abbrev'], + `${phase}: lockfile format is unchanged`) + t.strictSame(logs.warn.byTitle('install-scripts'), [], `${phase}: no blocked-script warning`) + } + }) + } +}) + +t.test('transitive native opt-outs clear stale flags in both install strategies', async t => { + for (const installStrategy of ['hoisted', 'linked']) { + await t.test(installStrategy, async t => { + const { npm, registry, logs } = await loadMockNpm(t, { + config: { audit: false, 'install-strategy': installStrategy }, + prefixDir: { + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + dependencies: { 'parent-pkg': '1.0.0' }, + }), + parent: { + 'package.json': JSON.stringify({ + name: 'parent-pkg', + version: '1.0.0', + dependencies: { 'native-pkg': '1.0.0' }, + }), + }, + native: { + 'package.json': JSON.stringify({ name: 'native-pkg', version: '1.0.0', gypfile: false }), + 'binding.gyp': '', + }, + }, + }) + const parent = registry.manifest({ + name: 'parent-pkg', + packuments: [{ version: '1.0.0', dependencies: { 'native-pkg': '1.0.0' } }], + }) + const native = registry.manifest({ + name: 'native-pkg', + packuments: [{ version: '1.0.0', hasInstallScript: true }], + }) + await registry.package({ manifest: parent }) + await registry.package({ manifest: native }) + const lockPath = path.join(npm.prefix, 'package-lock.json') + + for (const phase of ['fresh', 'clean-lockfile', 'repeat']) { + if (phase === 'clean-lockfile') { + const staleLock = JSON.parse(fs.readFileSync(lockPath, 'utf8')) + staleLock.packages['node_modules/native-pkg'].hasInstallScript = true + fs.writeFileSync(lockPath, JSON.stringify(staleLock)) + } + if (phase === 'clean-lockfile') { + fs.rmSync(path.join(npm.prefix, 'node_modules'), { recursive: true, force: true }) + fs.rmSync(path.join(npm.config.get('cache'), '_cacache'), { recursive: true, force: true }) + } + if (phase !== 'repeat') { + await registry.tarball({ + manifest: parent.versions['1.0.0'], + tarball: path.join(npm.prefix, 'parent'), + }) + await registry.tarball({ + manifest: native.versions['1.0.0'], + tarball: path.join(npm.prefix, 'native'), + }) + } + await npm.exec('install', []) + + const lock = JSON.parse(fs.readFileSync(lockPath, 'utf8')) + t.notOk(lock.packages['node_modules/native-pkg'].hasInstallScript, + `${phase}: clears the stale project-lockfile flag`) + t.strictSame(logs.warn.byTitle('install-scripts'), [], `${phase}: no false warning`) + const parentPath = fs.realpathSync(path.join(npm.prefix, 'node_modules/parent-pkg/package.json')) + const nativePath = require('node:module').createRequire(parentPath).resolve('native-pkg/package.json') + t.equal(JSON.parse(fs.readFileSync(nativePath, 'utf8')).gypfile, false, + `${phase}: the transitive opted-out package is installed`) + } + }) + } +}) + +t.test('strict preflight rejects same-version replacements before changing installed files', async t => { + const { npm } = await loadMockNpm(t, { + config: { audit: false, 'allow-file': 'all', 'strict-allow-scripts': true }, + prefixDir: { + 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), + old: { + 'package.json': JSON.stringify({ name: 'native-pkg', version: '1.0.0', gypfile: false }), + 'binding.gyp': '', + 'contents.txt': 'old', + }, + incoming: { + 'package.json': JSON.stringify({ + name: 'native-pkg', + version: '1.0.0', + scripts: { postinstall: 'echo incoming' }, + }), + 'binding.gyp': '', + 'contents.txt': 'incoming', + }, + }, + }) + for (const source of ['old', 'incoming']) { + await require('tar').c({ + cwd: path.join(npm.prefix, source), + prefix: 'package', + gzip: true, + file: path.join(npm.prefix, `${source}.tgz`), + }, ['package.json', 'binding.gyp', 'contents.txt']) + } + await npm.exec('install', ['./old.tgz']) + const files = [ + 'package.json', + 'package-lock.json', + 'node_modules/native-pkg/package.json', + 'node_modules/native-pkg/contents.txt', + ] + const before = files.map(file => fs.readFileSync(path.join(npm.prefix, file), 'utf8')) + + await t.rejects(npm.exec('install', ['./incoming.tgz']), { + code: 'ESTRICTALLOWSCRIPTS', + message: /postinstall: echo incoming/, + }) + t.strictSame( + files.map(file => fs.readFileSync(path.join(npm.prefix, file), 'utf8')), + before, + 'strict rejection leaves installed contents and project manifests unchanged' + ) +}) + t.test('exec commands', async t => { await t.test('with args does not run lifecycle scripts', async t => { const { npm, registry } = await loadMockNpm(t, { diff --git a/test/lib/utils/strict-allow-scripts-preflight.js b/test/lib/utils/strict-allow-scripts-preflight.js index c67a6e4853a12..9a1e2a3dd87a5 100644 --- a/test/lib/utils/strict-allow-scripts-preflight.js +++ b/test/lib/utils/strict-allow-scripts-preflight.js @@ -164,6 +164,27 @@ t.test('uses actualTree when idealTreeOpts is not provided (rebuild path)', asyn ) }) +t.test('only rebuild preflight can recover a stale script flag from installed metadata', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'pkg', version: '1.0.0', gypfile: false }), + }) + const pending = tree([{ + ...node({ scripts: {} }), + path, + hasInstallScript: true, + }]) + const npm = { flatOptions: { strictAllowScripts: true } } + await t.rejects(preflight({ + arb: makeArb({ ideal: pending }), + npm, + idealTreeOpts: {}, + }), { code: 'ESTRICTALLOWSCRIPTS' }, 'incoming metadata remains unreviewed') + await t.resolves(preflight({ + arb: makeArb({ actual: pending }), + npm, + }), 'rebuild can inspect the installed package') +}) + t.test('error message includes script bodies', async t => { const arb = makeArb({ ideal: tree([node({ name: 'canvas', version: '2.11.0', scripts: { install: 'node-gyp rebuild' } })]), diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 784c389c43756..6e06648860ea5 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -381,6 +381,25 @@ module.exports = cls => class IsolatedReifier extends cls { return root } + syncIsolatedScriptMetadata () { + // Rebuild updates the store package copies. Preserve those updates on + // the logical nodes used for reporting and the project lockfile. + for (const [node, proxy] of this.#externalProxies) { + const location = join('node_modules', '.store', getKey(proxy), 'node_modules', proxy.packageName) + const installed = this.idealTree.inventory.get(location) + if (!installed) { + continue + } + node.package.scripts = installed.package.scripts + node.package.gypfile = installed.package.gypfile + if (installed.hasInstallScript) { + node.package.hasInstallScript = true + } else { + delete node.package.hasInstallScript + } + } + } + #processEdges (node, externalEdge, root) { const key = getKey(node) if (this.#processedEdges.has(key)) { diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index 53c6f63e77d94..7b10ab59bf293 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -250,45 +250,49 @@ module.exports = cls => class Builder extends cls { !(meta.originalLockfileVersion >= 2) } - const { package: pkg, hasInstallScript } = node.target + const { target } = node + const { package: pkg, hasInstallScript } = target const { gypfile, bin, scripts = {} } = pkg const { preinstall, install, postinstall, prepare } = scripts const anyScript = preinstall || install || postinstall || prepare - if (!refreshed && !anyScript && (hasInstallScript || this.#oldMeta)) { - // we either have an old metadata (and thus might have scripts) - // or we have an indication that there's install scripts (but - // don't yet know what they are) so we have to load the package.json - // from disk to see what the deal is. Failure here just means - // no scripts to add, probably borked package.json. + const isGyp = gypfile !== false && + !install && + !preinstall && + await isNodeGypPackage(target.path) + + if (!refreshed && (isGyp || !anyScript && (hasInstallScript || this.#oldMeta))) { + // Lockfiles omit scripts and gypfile. Native candidates need a disk + // read even without hasInstallScript, so an opt-out is not lost. + if (set.has(node)) { + return + } // add to the set then remove while we're reading the pj, so we // don't accidentally hit it multiple times. set.add(node) - const { content: pkg } = await PackageJson.normalize(node.path).catch(() => { - return { content: {} } + const { content: pkg } = await PackageJson.normalize(target.path).catch(() => { + return { content: null } }) set.delete(node) - const { scripts = {} } = pkg - node.package.scripts = scripts + if (pkg) { + const scripts = pkg.scripts || {} + target.package.scripts = scripts + target.package.gypfile = pkg.gypfile + if (pkg.gypfile === false && !scripts.preinstall && !scripts.install && !scripts.postinstall) { + delete target.package.hasInstallScript + } + } return this.#addToBuildSet(node, set, true) } - // Rebuild node-gyp dependencies lacking an install or preinstall script - // note that 'scripts' might be missing entirely, and the package may - // set gypfile:false to avoid this automatic detection. - const isGyp = gypfile !== false && - !install && - !preinstall && - await isNodeGypPackage(node.path) - if (bin || preinstall || install || postinstall || prepare || isGyp) { if (bin) { await this[_checkBins](node) } if (isGyp) { scripts.install = defaultGypInstallScript - node.package.scripts = scripts + target.package.scripts = scripts } set.add(node) } diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index b099d4d72c6a4..364eecfc9844f 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -142,6 +142,7 @@ module.exports = cls => class Reifier extends cls { // The sweep itself scopes to in-filter workspaces when a filter is active, so it's safe to run for filtered installs too. if (!this.options.dryRun && !this.options.packageLockOnly) { await this.#cleanOrphanedStoreEntries() + this.syncIsolatedScriptMetadata() } } else if (!this.options.dryRun && !this.options.packageLockOnly) { // The .store directory is exclusively a linked-strategy artifact, and load-actual ignores dot-directories, so the diff never sees it. @@ -755,6 +756,7 @@ module.exports = cls => class Reifier extends cls { if (node.isInStore) { const { content: pkg } = await PackageJson.normalize(node.path) node.package.scripts = pkg.scripts + node.package.gypfile = pkg.gypfile } await this.#applyPatch(node) return diff --git a/workspaces/arborist/lib/install-scripts.js b/workspaces/arborist/lib/install-scripts.js index 29e9eace8eb96..4d1fb8c1b91ee 100644 --- a/workspaces/arborist/lib/install-scripts.js +++ b/workspaces/arborist/lib/install-scripts.js @@ -41,12 +41,35 @@ const hasNonRegistryShape = (node) => { return !/^https?:\/\/[^/]+\/.+\/-\/[^/]+-\d/.test(node.resolved) } -const getInstallScripts = async (node) => { +const getInstallScripts = async (node, { readFromDisk = true } = {}) => { /* istanbul ignore next: arborist Nodes always carry a `package` object; defensive fallbacks for non-arborist callers. */ const pkg = node.package || {} - /* istanbul ignore next */ - const scripts = pkg.scripts || {} + let scripts = pkg.scripts || {} + let { gypfile } = pkg + const includePrepare = hasNonRegistryShape(node) + const anyScript = scripts.preinstall || scripts.install || scripts.postinstall || + (includePrepare && scripts.prepare) + let isGyp = readFromDisk && !scripts.preinstall && !scripts.install && gypfile !== false && + await isNodeGypPackage(node.path).catch(() => false) + let recoveredOptOut = false + + // Preflight cannot trust an older package at this path, even if its + // name and version match. Only installed-tree callers recover metadata. + if (readFromDisk && (isGyp || !anyScript && node.hasInstallScript === true)) { + const { content } = await PackageJson.normalize(node.path) + .catch(() => ({ content: null })) + const matchesPackage = content && pkg.name && pkg.version && + content.name === pkg.name && content.version === pkg.version + if (matchesPackage) { + scripts = { ...content.scripts, ...scripts } + gypfile = content.gypfile + recoveredOptOut = gypfile === false + } else if (node.hasInstallScript === true) { + isGyp = false + } + } + const collected = {} if (scripts.preinstall) { @@ -58,53 +81,18 @@ const getInstallScripts = async (node) => { if (scripts.postinstall) { collected.postinstall = scripts.postinstall } - if (scripts.prepare && hasNonRegistryShape(node)) { + if (scripts.prepare && includePrepare) { collected.prepare = scripts.prepare } - const hasExplicitGypGate = !!(collected.preinstall || collected.install) - if ( - !hasExplicitGypGate && - pkg.gypfile !== false && - await isNodeGypPackage(node.path).catch(() => false) - ) { + if (isGyp && gypfile !== false && !scripts.preinstall && !scripts.install) { collected.install = 'node-gyp rebuild' } - // Lockfile-only nodes carry `hasInstallScript: true` but no enumerated - // scripts: the lockfile records the presence flag, not the script bodies, - // so `node.package.scripts` is empty on a lockfile-driven install (`npm ci`, - // a repeat `npm install`). Before giving up, read the installed - // package.json from disk to recover the real script bodies. Builder#addToBuildSet - // does the same disk read to decide what to run, but unlike that path this - // one is read-only: we never mutate `node.package`. - if (Object.keys(collected).length === 0 && node.hasInstallScript === true) { - const { content } = await PackageJson.normalize(node.path) - .catch(() => ({ content: {} })) - /* istanbul ignore next: normalize resolves to an object with a scripts - object, or our catch fallback returns {}; defensive guard only. */ - const diskScripts = content?.scripts || {} - - if (diskScripts.preinstall) { - collected.preinstall = diskScripts.preinstall - } - if (diskScripts.install) { - collected.install = diskScripts.install - } - if (diskScripts.postinstall) { - collected.postinstall = diskScripts.postinstall - } - if (diskScripts.prepare && hasNonRegistryShape(node)) { - collected.prepare = diskScripts.prepare - } - - // Still nothing. The package isn't on disk yet (e.g. `npm ci` before - // reify) or its package.json is unreadable. Emit a sentinel so the - // advisory and the strict-allow-scripts preflight still surface that - // install scripts are present. - if (Object.keys(collected).length === 0) { - collected.install = '(install scripts present)' - } + // A confirmed opt-out can disprove a stale flag left by native-build + // detection. Otherwise keep the flag visible, including before extraction. + if (Object.keys(collected).length === 0 && node.hasInstallScript === true && !recoveredOptOut) { + collected.install = '(install scripts present)' } return collected diff --git a/workspaces/arborist/lib/unreviewed-scripts.js b/workspaces/arborist/lib/unreviewed-scripts.js index f7bed387845ec..d5919e61516a8 100644 --- a/workspaces/arborist/lib/unreviewed-scripts.js +++ b/workspaces/arborist/lib/unreviewed-scripts.js @@ -18,6 +18,7 @@ const collectUnreviewedScripts = async ({ ignoreScripts = false, dangerouslyAllowAllScripts = false, includeWhenIgnored = false, + readFromDisk = true, } = {}) => { // With ignore-scripts set, no scripts run, so execution callers bail out // here. approve/deny pass includeWhenIgnored so they keep listing @@ -63,7 +64,7 @@ const collectUnreviewedScripts = async ({ continue } - const scripts = await getInstallScripts(node) + const scripts = await getInstallScripts(node, { readFromDisk }) if (Object.keys(scripts).length === 0) { continue } diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index 400ff7b238a69..f8cd88b9da9da 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -66,6 +66,7 @@ const printIdeal = (path, opt) => buildIdeal(path, opt).then(printTree) t.test('fail on mismatched engine when engineStrict is set', async t => { const path = resolve(fixtures, 'engine-specification') + createRegistry(t, true) await t.rejects(buildIdeal(path, { nodeVersion: '12.18.4', diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index 6c3062be89a00..d85a16c0cad03 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -603,6 +603,180 @@ t.test('do not rebuild node-gyp dependencies with gypfile:false', async t => { await arb.rebuild() }) +t.test('lockfile-backed native packages recover the installed build metadata', async t => { + const cases = [ + { name: 'opt-out', gypfile: false, expected: {} }, + { name: 'opt-out with null scripts', gypfile: false, scripts: null, expected: {} }, + { + name: 'ignored opt-out with null scripts', + gypfile: false, + scripts: null, + ignoreScripts: true, + expected: {}, + }, + { name: 'stale presence flag', gypfile: false, hasInstallScript: true, expected: {} }, + { + name: 'preinstall', + gypfile: false, + hasInstallScript: true, + scripts: { preinstall: 'echo preinstall' }, + expected: { preinstall: 'echo preinstall' }, + }, + { + name: 'install', + gypfile: false, + hasInstallScript: true, + scripts: { install: 'echo install' }, + expected: { install: 'echo install' }, + }, + { + name: 'postinstall', + gypfile: false, + hasInstallScript: true, + scripts: { postinstall: 'echo postinstall' }, + expected: { postinstall: 'echo postinstall' }, + }, + { + name: 'known postinstall', + gypfile: false, + scripts: { postinstall: 'echo postinstall' }, + knownScripts: { postinstall: 'echo postinstall' }, + expected: { postinstall: 'echo postinstall' }, + }, + { + name: 'known registry prepare', + gypfile: false, + scripts: { prepare: 'echo prepare' }, + knownScripts: { prepare: 'echo prepare' }, + expected: { prepare: 'echo prepare' }, + expectedRuns: {}, + }, + { + name: 'unreadable manifest preserves known scripts', + invalidManifest: true, + knownScripts: { postinstall: 'echo postinstall' }, + expected: { install: 'node-gyp rebuild', postinstall: 'echo postinstall' }, + }, + { + name: 'default native build', + expected: { install: 'node-gyp rebuild' }, + }, + { + name: 'default native build with null scripts', + scripts: null, + expected: { install: 'node-gyp rebuild' }, + }, + { + name: 'ignored native build with null scripts', + scripts: null, + ignoreScripts: true, + expected: { install: 'node-gyp rebuild' }, + expectedRuns: {}, + }, + { + name: 'native build and postinstall', + gypfile: true, + hasInstallScript: true, + scripts: { postinstall: 'echo postinstall' }, + expected: { install: 'node-gyp rebuild', postinstall: 'echo postinstall' }, + }, + ] + + for (const lockfileVersion of [2, 3]) { + for (const testCase of cases) { + await t.test(`v${lockfileVersion}: ${testCase.name}`, async t => { + const { + gypfile, scripts, knownScripts, hasInstallScript, invalidManifest, expected, + ignoreScripts = false, expectedRuns = expected, + } = testCase + const runs = [] + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': async opts => { + runs.push([opts.event, opts.pkg.scripts[opts.event]]) + return { code: 0, signal: null } + }, + }) + const pkg = { name: 'project', version: '1.0.0', dependencies: { dep: '1.0.0' } } + const path = t.testdir({ + 'package.json': JSON.stringify(pkg), + 'package-lock.json': JSON.stringify({ + name: pkg.name, + version: pkg.version, + lockfileVersion, + packages: { + '': pkg, + 'node_modules/dep': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/dep/-/dep-1.0.0.tgz', + hasInstallScript, + }, + }, + }), + node_modules: { + dep: { + 'package.json': invalidManifest ? '{' + : JSON.stringify({ name: 'dep', version: '1.0.0', gypfile, scripts }), + 'binding.gyp': '', + }, + }, + }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true, ignoreScripts }) + const tree = await arb.loadVirtual() + const dep = tree.children.get('dep') + if (knownScripts) { + dep.package.scripts = { ...knownScripts } + } + await arb.rebuild({ nodes: [dep] }) + + t.strictSame(runs, Object.entries(expectedRuns), 'runs only the expected scripts') + t.strictSame(dep.package.scripts, expected, 'retains the correct scripts') + t.equal(dep.package.gypfile, gypfile, 'retains the installed gypfile value') + const meta = require('../../lib/shrinkwrap.js').metaFromNode(dep, path) + t.equal(!!meta.hasInstallScript, !!(expected.preinstall || expected.install || expected.postinstall), + 'does not persist a false script-presence flag') + }) + } + } +}) + +t.test('does not queue an opted-out bundled package twice', async t => { + const runs = [] + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': async ({ event, pkg }) => { + runs.push([event, pkg.name]) + return { code: 0, signal: null } + }, + }) + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'project', dependencies: { parent: '1.0.0' } }), + node_modules: { + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { dep: '1.0.0' }, + bundleDependencies: ['dep'], + }), + node_modules: { + dep: { + 'package.json': JSON.stringify({ + name: 'dep', + version: '1.0.0', + gypfile: false, + scripts: { preinstall: 'echo preinstall' }, + }), + 'binding.gyp': '', + }, + }, + }, + }, + }) + // The bundle is visited through both its parent and the tree inventory. + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) + await arb.rebuild() + t.strictSame(runs, [['preinstall', 'dep']]) +}) + // ref: https://github.com/npm/cli/issues/2905 t.test('do not run lifecycle scripts of linked deps twice', async t => { const testdir = t.testdir({ diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 943e340b22c1f..8d2a9060e4353 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -4700,6 +4700,67 @@ t.test('global install ignores a per-call linked strategy', async t => { t.strictSame(fs.readdirSync(nm), ['abbrev'], 'global package retained, no .store created') }) +t.test('linked native packages retain gypfile:false during extraction', async t => { + for (const scripts of [{}, { postinstall: 'echo postinstall' }]) { + await t.test(Object.keys(scripts).join(', ') || 'no scripts', async t => { + const runs = [] + const Arborist = t.mock('../../lib/index.js', { + '@npmcli/run-script': async opts => { + runs.push([opts.event, opts.pkg.scripts[opts.event]]) + t.equal(opts.pkg.gypfile, false, 'retains the opt-out alongside explicit scripts') + return { code: 0, signal: null } + }, + }) + const root = t.testdir({ + src: { + 'package.json': JSON.stringify({ + name: 'native-pkg', + version: '1.0.0', + gypfile: false, + scripts, + }), + 'binding.gyp': '', + }, + project: { + 'package.json': JSON.stringify({ + name: 'project', + version: '1.0.0', + dependencies: { 'native-pkg': '1.0.0' }, + }), + }, + }) + const registry = createRegistry(t) + const manifest = registry.manifest({ + name: 'native-pkg', + packuments: [{ version: '1.0.0', hasInstallScript: !!scripts.postinstall }], + }) + await registry.package({ manifest }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: resolve(root, 'src'), + }) + const arb = new Arborist({ + path: resolve(root, 'project'), + cache: resolve(root, 'cache'), + registry: 'https://registry.npmjs.org', + audit: false, + installStrategy: 'linked', + dangerouslyAllowAllScripts: true, + }) + await arb.reify() + + t.strictSame(runs, Object.entries(scripts), 'does not inject a native-build script') + const installed = fs.realpathSync(resolve(root, 'project/node_modules/native-pkg')) + t.match(installed, /[\\/]\.store[\\/]/, 'installs into the linked store') + t.ok(fs.existsSync(resolve(installed, 'binding.gyp')), 'installs binding.gyp') + + await arb.reify({ rm: ['native-pkg'] }) + t.notOk(fs.existsSync(resolve(root, 'project/node_modules/native-pkg')), + 'reusing Arborist does not restore metadata for a removed package') + }) + } +}) + t.test('linked strategy exposes store node_modules via NODE_PATH for lifecycle scripts', async t => { // Regression for #9549. In the linked strategy a store package's deps are symlinked siblings in its store node_modules. // A separate bin invoked by the script (e.g. napi-postinstall) resolves modules from its own store realpath and cannot see them, so npm exposes them via NODE_PATH. diff --git a/workspaces/arborist/test/install-scripts.js b/workspaces/arborist/test/install-scripts.js index 342e8e7d891d4..0bbb38172b2d6 100644 --- a/workspaces/arborist/test/install-scripts.js +++ b/workspaces/arborist/test/install-scripts.js @@ -1,4 +1,5 @@ const t = require('tap') +const PackageJson = require('@npmcli/package-json') const mockGetInstallScripts = (t, isNodeGypResult = () => false) => t.mock('../lib/install-scripts.js', { @@ -51,6 +52,13 @@ t.test('ignores unrelated scripts', async t => { ) }) +t.test('missing or null scripts are empty', async t => { + const getInstallScripts = mockGetInstallScripts(t) + for (const scripts of [undefined, null]) { + t.strictSame(await getInstallScripts(node({ scripts })), {}) + } +}) + t.test('prepare only counts for non-registry sources', async t => { const getInstallScripts = mockGetInstallScripts(t) // registry: prepare ignored @@ -95,6 +103,173 @@ t.test('synthetic node-gyp suppressed when gypfile: false', async t => { ) }) +t.test('native packages recover installed scripts and gypfile before enumeration', async t => { + const cases = [ + { name: 'opt-out', gypfile: false, scripts: {}, expected: {} }, + { + name: 'preinstall', + gypfile: false, + scripts: { preinstall: 'echo preinstall' }, + expected: { preinstall: 'echo preinstall' }, + }, + { + name: 'install', + gypfile: false, + scripts: { install: 'echo install' }, + expected: { install: 'echo install' }, + }, + { + name: 'postinstall', + gypfile: false, + scripts: { postinstall: 'echo postinstall' }, + expected: { postinstall: 'echo postinstall' }, + }, + { + name: 'known postinstall', + gypfile: false, + scripts: { postinstall: 'echo postinstall' }, + knownScripts: { postinstall: 'echo postinstall' }, + expected: { postinstall: 'echo postinstall' }, + }, + { + name: 'registry prepare', + gypfile: false, + scripts: { prepare: 'echo prepare' }, + expected: {}, + }, + { + name: 'non-registry prepare', + gypfile: false, + scripts: { prepare: 'echo prepare' }, + isRegistryDependency: false, + expected: { prepare: 'echo prepare' }, + }, + { + name: 'default native build', + scripts: {}, + expected: { install: 'node-gyp rebuild' }, + }, + { + name: 'native build and postinstall', + gypfile: true, + scripts: { postinstall: 'echo postinstall' }, + expected: { install: 'node-gyp rebuild', postinstall: 'echo postinstall' }, + }, + ] + + for (const hasInstallScript of [undefined, false, true]) { + for (const testCase of cases) { + const { + name, gypfile, scripts, knownScripts = {}, isRegistryDependency = true, expected, + } = testCase + await t.test(`${name}, hasInstallScript=${hasInstallScript}`, async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'pkg', version: '1.0.0', gypfile, scripts }), + 'binding.gyp': '', + }) + let reads = 0 + const getInstallScripts = t.mock('../lib/install-scripts.js', { + '@npmcli/package-json': { + normalize: async path => { + reads++ + return PackageJson.normalize(path) + }, + }, + }) + const pkg = Object.freeze({ + name: 'pkg', + version: '1.0.0', + scripts: Object.freeze(knownScripts), + }) + const lockfileNode = { + path, + isRegistryDependency, + hasInstallScript, + package: pkg, + } + + t.strictSame(await getInstallScripts(lockfileNode), expected) + t.equal(reads, 1, 'reads installed metadata once') + t.equal(lockfileNode.package, pkg, 'keeps the package object') + t.strictSame(pkg, { name: 'pkg', version: '1.0.0', scripts: knownScripts }, + 'does not modify package data') + }) + } + } +}) + +t.test('a confirmed opt-out clears a stale presence flag without binding.gyp', async t => { + const getInstallScripts = require('../lib/install-scripts.js') + const pkg = { name: 'pkg', version: '1.0.0', gypfile: false } + const path = t.testdir({ 'package.json': JSON.stringify(pkg) }) + t.strictSame(await getInstallScripts({ + path, + package: pkg, + hasInstallScript: true, + }), {}) +}) + +t.test('disk recovery does not replace known script bodies', async t => { + const getInstallScripts = require('../lib/install-scripts.js') + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'pkg', + version: '1.0.0', + gypfile: false, + scripts: { postinstall: 'old postinstall' }, + }), + 'binding.gyp': '', + }) + const pkg = Object.freeze({ + name: 'pkg', + version: '1.0.0', + scripts: Object.freeze({ postinstall: 'incoming postinstall' }), + }) + t.strictSame(await getInstallScripts({ + path, + package: pkg, + hasInstallScript: true, + }), { postinstall: 'incoming postinstall' }) +}) + +t.test('preflight does not recover an opt-out from an older installed package', async t => { + const getInstallScripts = require('../lib/install-scripts.js') + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'pkg', version: '1.0.0', gypfile: false }), + }) + t.strictSame(await getInstallScripts({ + path, + package: { name: 'pkg', version: '1.0.0' }, + hasInstallScript: true, + }, { readFromDisk: false }), { install: '(install scripts present)' }) +}) + +t.test('unavailable or mismatched metadata cannot clear a script-presence flag', async t => { + const getInstallScripts = require('../lib/install-scripts.js') + const cases = [ + ['missing', undefined], + ['malformed', '{'], + ['different version', { name: 'pkg', version: '0.9.0', gypfile: false }], + ['different name', { name: 'other', version: '1.0.0', gypfile: false }], + ['missing identity', { gypfile: false }], + ] + for (const [name, content] of cases) { + await t.test(name, async t => { + const path = t.testdir({ + 'binding.gyp': '', + ...(content === undefined ? {} : { + 'package.json': typeof content === 'string' ? content : JSON.stringify(content), + }), + }) + t.strictSame(await getInstallScripts({ + path, + package: { name: 'pkg', version: '1.0.0' }, + hasInstallScript: true, + }), { install: '(install scripts present)' }) + }) + } +}) + t.test('synthetic node-gyp suppressed when explicit install is present', async t => { const getInstallScripts = mockGetInstallScripts(t, () => true) t.strictSame( diff --git a/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js b/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js index 579fd0c0fddf2..036622bfa5c8b 100644 --- a/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js +++ b/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js @@ -24,6 +24,7 @@ const strictAllowScriptsPreflight = async (arb, opts) => { policy: opts.allowScripts || null, ignoreScripts: opts.ignoreScripts, dangerouslyAllowAllScripts: opts.dangerouslyAllowAllScripts, + readFromDisk: false, }) if (unreviewed.length === 0) { diff --git a/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js b/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js index 5fc803c26c8d1..072b0121935d3 100644 --- a/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js +++ b/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js @@ -64,6 +64,31 @@ t.test('throws when unreviewed scripts are present', async t => { ) }) +t.test('does not trust an installed opt-out for an incoming package with the same identity', async t => { + for (const scripts of [{ postinstall: 'incoming script' }, {}]) { + await t.test(Object.keys(scripts).join(', ') || 'presence flag only', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'pkg', version: '1.0.0', gypfile: false }), + 'binding.gyp': '', + }) + const arb = fakeArb({ + inventory: new Map([['node_modules/pkg', { + name: 'pkg', + location: 'node_modules/pkg', + path, + resolved: 'file:incoming.tgz', + package: { name: 'pkg', version: '1.0.0', scripts }, + hasInstallScript: true, + }]]), + }) + await t.rejects( + strictAllowScriptsPreflight(arb, { strictAllowScripts: true }), + { code: 'ESTRICTALLOWSCRIPTS' } + ) + }) + } +}) + t.test('resolves when the only unreviewed node is inert', async t => { // Inert deps (platform/engine-incompatible) are removed before any script // runs, so strict mode must not reject them (npm/cli#9562).