From d124c0858da0b138cda2addcb0987b063ca86a47 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Thu, 21 May 2026 16:49:18 +0100 Subject: [PATCH 1/4] docs: Document `npm_old_version` and `npm_new_version` environment variables (#9385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit npm sets two additional environment variables `npm_old_version` and `npm_new_version` when running the `preversion`, `version`, `postversion` scripts, but these aren’t documented anywhere. Document the variables in the ‘Scripts’ docs, and cross-reference them from the documentation for the version command (and the libnpmversion readme). I've tried to match the existing formatting conventions for each document. Please let me know if anything needs to change. --- docs/lib/content/commands/npm-version.md | 2 ++ docs/lib/content/using-npm/scripts.md | 7 +++++++ workspaces/libnpmversion/README.md | 3 +++ 3 files changed, 12 insertions(+) diff --git a/docs/lib/content/commands/npm-version.md b/docs/lib/content/commands/npm-version.md index 23ce28763f6cd..cca8106a00831 100644 --- a/docs/lib/content/commands/npm-version.md +++ b/docs/lib/content/commands/npm-version.md @@ -69,6 +69,8 @@ The exact order of execution is as follows: 6. Run the `postversion` script. Use it to clean up the file system or automatically push the commit and/or tag. +For the `preversion`, `version` and `postversion` scripts, npm also sets the [environment variables](/using-npm/scripts#environment) `npm_old_version` and `npm_new_version`. + Take the following example: ```json diff --git a/docs/lib/content/using-npm/scripts.md b/docs/lib/content/using-npm/scripts.md index 380c4aa49e36f..b01c0bc7ffd77 100644 --- a/docs/lib/content/using-npm/scripts.md +++ b/docs/lib/content/using-npm/scripts.md @@ -290,6 +290,13 @@ For example, if you had `{"name":"foo", "version":"1.2.5"}` in your package.json See [`package.json`](/configuring-npm/package-json) for more on package configs. +#### versioning variables + +For versioning scripts (`preversion`, `version`, `postversion`), npm sets these environment variables: + +* `npm_old_version` - The version before being bumped +* `npm_new_version` – The version after being bumped + #### current lifecycle event Lastly, the `npm_lifecycle_event` environment variable is set to whichever stage of the cycle is being executed. diff --git a/workspaces/libnpmversion/README.md b/workspaces/libnpmversion/README.md index 3a91bb471ba56..8f3f89e453a9b 100644 --- a/workspaces/libnpmversion/README.md +++ b/workspaces/libnpmversion/README.md @@ -85,6 +85,9 @@ The exact order of execution is as follows: 6. Run the `postversion` script. Use it to clean up the file system or automatically push the commit and/or tag. +For the `preversion`, `version` and `postversion` scripts, npm also sets the +environment variables `npm_old_version` and `npm_new_version`. + Take the following example: ```json From c0fc54935af8e17a3a96cbdeac52bb4c597803b6 Mon Sep 17 00:00:00 2001 From: Zelys Date: Thu, 21 May 2026 11:10:20 -0500 Subject: [PATCH 2/4] fix(config): pause progress spinner during interactive editor spawn (#9372) Fixes #9142, Fixes #9184 When \`npm config edit\` or \`npm edit\` opens an interactive editor, the progress spinner keeps running and writes ANSI control codes into the buffer, garbling the display. Neither command called \`input.start()\` before opening the editor, though \`help.js\` and \`open-url.js\` already handle this correctly. Wrapping both editor spawns in \`input.start()\` lets the spinner step aside while the editor has control of the terminal, and closes both issues at once. Co-authored-by: Claude Sonnet 4.6 --- lib/commands/config.js | 6 +++--- lib/commands/edit.js | 10 ++++++---- test/lib/commands/config.js | 6 ++++++ test/lib/commands/edit.js | 6 ++++++ 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 015850c48304a..0a8b84aba2666 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -5,7 +5,7 @@ const { EOL } = require('node:os') const localeCompare = require('@isaacs/string-locale-compare')('en') const pkgJson = require('@npmcli/package-json') const { defaults, definitions, nerfDarts, proxyEnv } = require('@npmcli/config/lib/definitions') -const { log, output } = require('proc-log') +const { log, output, input } = require('proc-log') const BaseCommand = require('../base-cmd.js') const { redact } = require('@npmcli/redact') @@ -266,7 +266,7 @@ ${defData} `.split('\n').join(EOL) await mkdir(dirname(file), { recursive: true }) await writeFile(file, tmpData, 'utf8') - await new Promise((res, rej) => { + await input.start(() => new Promise((res, rej) => { const [bin, ...args] = e.split(/\s+/) const editor = spawn(bin, [...args, file], { stdio: 'inherit' }) editor.on('exit', (code) => { @@ -275,7 +275,7 @@ ${defData} } return res() }) - }) + })) } async fix () { diff --git a/lib/commands/edit.js b/lib/commands/edit.js index 1140c59efa3e4..0b1a200264d98 100644 --- a/lib/commands/edit.js +++ b/lib/commands/edit.js @@ -1,6 +1,7 @@ const { resolve } = require('node:path') const { lstat } = require('node:fs/promises') const cp = require('node:child_process') +const { input } = require('proc-log') const completion = require('../utils/installed-shallow.js') const BaseCommand = require('../base-cmd.js') @@ -46,16 +47,17 @@ class Edit extends BaseCommand { const dir = resolve(this.npm.dir, path) await lstat(dir) - await new Promise((res, rej) => { + await input.start(() => new Promise((res, rej) => { const [bin, ...spawnArgs] = this.npm.config.get('editor').split(/\s+/) const editor = cp.spawn(bin, [...spawnArgs, dir], { stdio: 'inherit' }) - editor.on('exit', async (code) => { + editor.on('exit', (code) => { if (code) { return rej(new Error(`editor process exited with code: ${code}`)) } - await this.npm.exec('rebuild', [dir]).then(res).catch(rej) + res() }) - }) + })) + await this.npm.exec('rebuild', [dir]) } } diff --git a/test/lib/commands/config.js b/test/lib/commands/config.js index fbcc58ba40153..56f46981a066a 100644 --- a/test/lib/commands/config.js +++ b/test/lib/commands/config.js @@ -574,6 +574,11 @@ t.test('config edit', async t => { }, }) + const inputEvents = [] + const inputListener = (level) => inputEvents.push(level) + process.on('input', inputListener) + t.teardown(() => process.off('input', inputListener)) + await npm.exec('config', ['edit']) t.ok(editor.called, 'editor was spawned') @@ -582,6 +587,7 @@ t.test('config edit', async t => { [join(home, '.npmrc')], 'editor opened the user config file' ) + t.same(inputEvents.slice(0, 2), ['start', 'end'], 'progress paused and resumed around editor') const contents = await fs.readFile(join(home, '.npmrc'), { encoding: 'utf8' }) t.ok(contents.includes('foo=bar'), 'kept foo') diff --git a/test/lib/commands/edit.js b/test/lib/commands/edit.js index b55bb2df218ba..915241c82f6da 100644 --- a/test/lib/commands/edit.js +++ b/test/lib/commands/edit.js @@ -58,8 +58,14 @@ t.test('npm edit', async t => { : ['-c', 'testinstall'] spawk.spawn(scriptShell, scriptArgs, { cwd: semverPath }) + const inputEvents = [] + const inputListener = (level) => inputEvents.push(level) + process.on('input', inputListener) + t.teardown(() => process.off('input', inputListener)) + await npm.exec('edit', ['semver']) t.match(joinedOutput(), 'rebuilt dependencies successfully') + t.same(inputEvents.slice(0, 2), ['start', 'end'], 'progress paused and resumed around editor') }) t.test('rebuild failure', async t => { From 979518dd198b9f2beb788c6c3cdcd1e055b03d22 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 21 May 2026 11:36:26 -0700 Subject: [PATCH 3/4] feat!: error on unknown configs, flags, and abbreviations (#9276) BREAKING CHANGE: unknown configs in .npmrc, unknown CLI flags, abbreviated flags, and single-hyphen multi-char shorthands now throw instead of warning. Closes npm/statusboard#1084. Unknown configs are now hard errors instead of warnings: - Unknown keys in `.npmrc` (project/user/global/builtin), aggregated across files - Unknown CLI flags (`--bogus`) - Abbreviated flags (`--dry` for `--dry-run`) - Single-hyphen multi-char flags (`-longflag`) Env (`npm_config_*`) and `publishConfig` unknowns still warn. `config`, `help`, `doctor`, `completion`, and `version` opt out via `static skipConfigValidation = true` so `npm config fix` and friends keep working when `.npmrc` is broken. ### Notable breaks - Legacy top-level auth keys (`email`, `_authToken`, `_password`, `username`, `certfile`, `keyfile`) must be in scoped/nerfdart form. Run `npm config fix` to migrate. - `-j` no longer expands to `--json`. Use `--json`. --- lib/base-cmd.js | 110 ++-- lib/commands/completion.js | 1 + lib/commands/doctor.js | 1 + lib/commands/help.js | 1 + lib/commands/version.js | 1 + lib/npm.js | 6 +- .../test/lib/commands/config.js.test.cjs | 29 +- .../test/lib/commands/diff.js.test.cjs | 18 +- test/lib/base-cmd.js | 204 ++++++- test/lib/commands/config.js | 46 +- test/lib/commands/diff.js | 8 +- test/lib/commands/login.js | 5 +- test/lib/commands/logout.js | 20 +- test/lib/commands/set.js | 8 +- workspaces/config/lib/index.js | 144 +++-- .../tap-snapshots/test/index.js.test.cjs | 100 +--- workspaces/config/test/index.js | 560 +++++++++++++----- 17 files changed, 839 insertions(+), 423 deletions(-) diff --git a/lib/base-cmd.js b/lib/base-cmd.js index 089ec680c7425..10a2fbb2f181c 100644 --- a/lib/base-cmd.js +++ b/lib/base-cmd.js @@ -1,5 +1,5 @@ const { log } = require('proc-log') -const { definitions, shorthands } = require('@npmcli/config/lib/definitions') +const { definitions } = require('@npmcli/config/lib/definitions') const nopt = require('nopt') class BaseCommand { @@ -323,10 +323,9 @@ class BaseCommand { delete parsed.argv } - // Validate flags - only if command has definitions (new system) - if (this.constructor.definitions && this.constructor.definitions.length > 0) { - this.#validateFlags(parsed, commandDefinitions, remains) - } + // Validate unknown CLI flags/configs and unexpected positionals. + // Runs for every command; command-specific flags are allow-listed here so they don't trip the global unknown-config collection from Config.loadCLI(). + this.validateCli(commandDefinitions, remains) // Check for conflicts between main flags and their aliases // Also map aliases back to their main keys @@ -363,64 +362,75 @@ class BaseCommand { return [{ ...defaults, ...filtered }, remains] } - // Validate flags and throw errors for unknown flags or unexpected positionals - #validateFlags (parsed, commandDefinitions, remains) { - // Build a set of all valid flag names (global + command-specific + shorthands) - const validFlags = new Set([ - ...Object.keys(definitions), + // Unified CLI validation — runs for every command (definitions-based and legacy). + // Reads collected unknown configs from Config (they were collected, not thrown, during Config.load()), subtracts any command-specific definitions, and throws a single aggregated error. + // Also enforces extra-positional errors for commands that set a finite `static positionals`. + // Shellout commands (run/exec/lifecycle) leave `static positionals = null` and are unaffected. + // Commands that set `static skipConfigValidation = true` (config, help, doctor, completion, version) bypass both unknown-config checks so they can operate against a broken .npmrc. + validateCli (commandDefinitions = this.constructor.definitions || [], remains = null) { + const allowlist = new Set([ ...commandDefinitions.map(d => d.key), - ...Object.keys(shorthands), // Add global shorthands like 'verbose', 'dd', etc. + ...commandDefinitions.flatMap(d => Array.isArray(d.alias) ? d.alias : []), ]) - // Add aliases to valid flags - for (const def of commandDefinitions) { - if (def.alias && Array.isArray(def.alias)) { - for (const alias of def.alias) { - validFlags.add(alias) - } - } - } + if (!this.constructor.skipConfigValidation) { + const cliUnknowns = this.npm.config.getUnknownConfigs('cli') + .filter(u => !allowlist.has(u.key) && !allowlist.has(u.baseKey)) - // Check parsed flags against valid flags - const unknownFlags = [] - for (const key of Object.keys(parsed)) { - if (!validFlags.has(key)) { - unknownFlags.push(key) + const fileUnknowns = [] + for (const where of ['builtin', 'project', 'user', 'global']) { + fileUnknowns.push(...this.npm.config.getUnknownConfigs(where)) } - } - // Throw error if unknown flags were found - if (unknownFlags.length > 0) { - const flagList = unknownFlags.map(f => `--${f}`).join(', ') - throw this.usageError(`Unknown flag${unknownFlags.length > 1 ? 's' : ''}: ${flagList}`) - } - - // Remove warnings for command-specific definitions that npm's global config doesn't know about (these were queued as "unknown" during config.load()) - for (const def of commandDefinitions) { - this.npm.config.removeWarning(def.key) - if (def.alias && Array.isArray(def.alias)) { - for (const alias of def.alias) { - this.npm.config.removeWarning(alias) + if (cliUnknowns.length > 0 || fileUnknowns.length > 0) { + const sections = [] + if (cliUnknowns.length > 0) { + const lines = cliUnknowns.map(u => + u.baseKey ? ` - --${u.baseKey} (${u.key})` : ` - --${u.key}` + ) + sections.push( + `Unknown cli config${cliUnknowns.length > 1 ? 's' : ''}:`, + ...lines, + 'Run `npm help config` for supported options.' + ) } + if (fileUnknowns.length > 0) { + if (sections.length > 0) { + sections.push('') + } + const lines = fileUnknowns.map(u => { + const display = u.baseKey ? `"${u.baseKey}" (${u.key})` : `"${u.key}"` + return ` - ${u.where} config ${display} from ${u.source}` + }) + sections.push( + `Unknown npm configuration key${fileUnknowns.length > 1 ? 's' : ''}:`, + ...lines, + 'See `npm help npmrc` for supported config options.' + ) + } + throw Object.assign(new Error(sections.join('\n')), { + code: 'EUNKNOWNCONFIG', + unknownConfigs: [...cliUnknowns, ...fileUnknowns], + }) } } - // Remove warnings for unknown positionals that were actually consumed as flag values by command-specific definitions (e.g., --id where --id is command-specific) - const remainsSet = new Set(remains) - for (const unknownPos of this.npm.config.getUnknownPositionals()) { - if (!remainsSet.has(unknownPos)) { - // This value was consumed as a flag value, not truly a positional - this.npm.config.removeUnknownPositional(unknownPos) + // Positionals consumed as flag values by command-specific definitions were queued as "unknown positional" warnings by Config.unknownHandler; drop those since they're actually flag arguments. + if (Array.isArray(remains)) { + const remainsSet = new Set(remains) + for (const unknownPos of this.npm.config.getUnknownPositionals()) { + if (!remainsSet.has(unknownPos)) { + this.npm.config.removeUnknownPositional(unknownPos) + } } } - // Warn about extra positional arguments beyond what the command expects - const expectedPositionals = this.constructor.positionals - if (expectedPositionals !== null && remains.length > expectedPositionals) { - const extraPositionals = remains.slice(expectedPositionals) - for (const extra of extraPositionals) { - throw new Error(`Unknown positional argument: ${extra}`) - } + const expected = this.constructor.positionals + if (expected !== null && remains !== null && remains.length > expected) { + const extra = remains.slice(expected) + throw this.usageError( + `Unknown positional argument${extra.length > 1 ? 's' : ''}: ${extra.join(', ')}` + ) } this.npm.config.logWarnings() diff --git a/lib/commands/completion.js b/lib/commands/completion.js index 499afcb5c68bc..712cf7358832f 100644 --- a/lib/commands/completion.js +++ b/lib/commands/completion.js @@ -37,6 +37,7 @@ class Completion extends BaseCommand { static name = 'completion' // Completion command uses args differently - they represent the command line being completed, not actual arguments to this command, so we use an empty definitions object to prevent flag validation static definitions = [] + static skipConfigValidation = true // completion for the completion command static async completion (opts) { diff --git a/lib/commands/doctor.js b/lib/commands/doctor.js index f01b05bead27a..203f6d156c5b8 100644 --- a/lib/commands/doctor.js +++ b/lib/commands/doctor.js @@ -99,6 +99,7 @@ class Doctor extends BaseCommand { static name = 'doctor' static params = ['registry'] static ignoreImplicitWorkspace = false + static skipConfigValidation = true static usage = [`[${checks.flatMap(s => s.groups) .filter((value, index, self) => self.indexOf(value) === index && value !== 'ping') .join('] [')}]`] diff --git a/lib/commands/help.js b/lib/commands/help.js index a684667e32411..d520dd1cceb45 100644 --- a/lib/commands/help.js +++ b/lib/commands/help.js @@ -25,6 +25,7 @@ class Help extends BaseCommand { static name = 'help' static usage = [' []'] static params = ['viewer'] + static skipConfigValidation = true static async completion (opts, npm) { if (opts.conf.argv.remain.length > 2) { diff --git a/lib/commands/version.js b/lib/commands/version.js index fe70322fd7cb9..67b17cfbf3967 100644 --- a/lib/commands/version.js +++ b/lib/commands/version.js @@ -23,6 +23,7 @@ class Version extends BaseCommand { static workspaces = true static ignoreImplicitWorkspace = false + static skipConfigValidation = true static usage = ['[ | major | minor | patch | premajor | preminor | prepatch | prerelease | from-git]'] diff --git a/lib/npm.js b/lib/npm.js index 2b4b2843e9218..b7317970771d1 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -291,8 +291,10 @@ class Npm { ? commandInstance.execWorkspaces(positionalArgs, flags) : commandInstance.exec(positionalArgs, flags)) } else { - // Legacy commands without definitions - this.config.logWarnings() + // Legacy commands without definitions: still validate unknown CLI configs/flags and (when finite) extra positionals. + if (typeof commandInstance.validateCli === 'function') { + commandInstance.validateCli([], args) + } return time.start(`command:${commandName}`, () => execWorkspaces ? commandInstance.execWorkspaces(args) : commandInstance.exec(args)) } diff --git a/tap-snapshots/test/lib/commands/config.js.test.cjs b/tap-snapshots/test/lib/commands/config.js.test.cjs index 42bd213ba473e..ec4841813d025 100644 --- a/tap-snapshots/test/lib/commands/config.js.test.cjs +++ b/tap-snapshots/test/lib/commands/config.js.test.cjs @@ -10,9 +10,9 @@ exports[`test/lib/commands/config.js TAP config list --json > output matches sna "cache": "{CACHE}", "color": {COLOR}, "json": true, - "projectloaded": "yes", - "userloaded": "yes", - "globalloaded": "yes", + "tag": "from-project", + "init-author-name": "from-user", + "init-license": "from-global", "access": null, "all": false, "allow-same-version": false, @@ -77,9 +77,7 @@ exports[`test/lib/commands/config.js TAP config list --json > output matches sna "include-workspace-root": false, "include-attestations": false, "init-author-email": "", - "init-author-name": "", "init-author-url": "", - "init-license": "ISC", "init-module": "{CWD}/home/.npm-init.js", "init-type": "commonjs", "init-version": "1.0.0", @@ -167,7 +165,6 @@ exports[`test/lib/commands/config.js TAP config list --json > output matches sna "sign-git-tag": false, "strict-peer-deps": false, "strict-ssl": true, - "tag": "latest", "tag-version-prefix": "v", "timing": false, "umask": 0, @@ -258,9 +255,9 @@ include-attestations = false include-staged = false include-workspace-root = false init-author-email = "" -init-author-name = "" +; init-author-name = "" ; overridden by user init-author-url = "" -init-license = "ISC" +; init-license = "ISC" ; overridden by global init-module = "{CWD}/home/.npm-init.js" init-private = false init-type = "commonjs" @@ -348,7 +345,7 @@ sign-git-commit = false sign-git-tag = false strict-peer-deps = false strict-ssl = true -tag = "latest" +; tag = "latest" ; overridden by project tag-version-prefix = "v" timing = false token-description = null @@ -369,15 +366,15 @@ yes = null ; "global" config from {CWD}/global/etc/npmrc -globalloaded = "yes" +init-license = "from-global" ; "user" config from {CWD}/home/.npmrc -userloaded = "yes" +init-author-name = "from-user" ; "project" config from {CWD}/prefix/.npmrc -projectloaded = "yes" +tag = "from-project" ; "cli" config from command line options @@ -389,19 +386,17 @@ long = true exports[`test/lib/commands/config.js TAP config list > output matches snapshot 1`] = ` ; "global" config from {CWD}/global/etc/npmrc -globalloaded = "yes" +init-license = "from-global" ; "user" config from {CWD}/home/.npmrc _auth = (protected) //nerfdart:_auth = (protected) -//nerfdart:auth = (protected) -auth = (protected) -userloaded = "yes" +init-author-name = "from-user" ; "project" config from {CWD}/prefix/.npmrc -projectloaded = "yes" +tag = "from-project" ; "cli" config from command line options diff --git a/tap-snapshots/test/lib/commands/diff.js.test.cjs b/tap-snapshots/test/lib/commands/diff.js.test.cjs index e87086d7d9b8f..0eb91154b2417 100644 --- a/tap-snapshots/test/lib/commands/diff.js.test.cjs +++ b/tap-snapshots/test/lib/commands/diff.js.test.cjs @@ -62,11 +62,13 @@ package.json ` exports[`test/lib/commands/diff.js TAP various options using diff option > must match snapshot 1`] = ` -diff --git a/index.js b/index.js +diff --git bar/index.js foo/index.js index v2.0.0..v3.0.0 100644 ---- a/index.js -+++ b/index.js -@@ -18,7 +18,7 @@ +--- bar/index.js ++++ foo/index.js +@@ -16,11 +16,11 @@ + 15 + 16 17 18 19 @@ -75,10 +77,12 @@ index v2.0.0..v3.0.0 100644 21 22 23 -diff --git a/package.json b/package.json + 24 + 25 +diff --git bar/package.json foo/package.json index v2.0.0..v3.0.0 100644 ---- a/package.json -+++ b/package.json +--- bar/package.json ++++ foo/package.json @@ -1,4 +1,4 @@ { "name": "bar", diff --git a/test/lib/base-cmd.js b/test/lib/base-cmd.js index 41bade7298b90..58573f9594cd4 100644 --- a/test/lib/base-cmd.js +++ b/test/lib/base-cmd.js @@ -118,7 +118,9 @@ t.test('flags() method with no definitions', async t => { }) t.test('flags() throws error for unknown flags', async t => { - const { npm } = await loadMockNpm(t) + const { npm } = await loadMockNpm(t, { + npm: { argv: ['test-command', '--unknown-flag'] }, + }) class TestCommand extends BaseCommand { static name = 'test-command' @@ -139,14 +141,14 @@ t.test('flags() throws error for unknown flags', async t => { } } - // Manually set config.argv to simulate command-line with unknown flag - npm.config.argv = ['node', 'npm', 'test-command', '--unknown-flag'] - const command = new TestCommand(npm) await t.rejects( command.exec(), - { message: /Unknown flag.*--unknown-flag/ }, - 'throws error for unknown flag' + { + code: 'EUNKNOWNCONFIG', + message: /Unknown cli config:[\s\S]*--unknown-flag/, + }, + 'throws EUNKNOWNCONFIG for unknown flag' ) }) @@ -416,7 +418,9 @@ t.test('flags() returns defaults when argv is empty', async t => { }) t.test('flags() throws error for multiple unknown flags with pluralization', async t => { - const { npm } = await loadMockNpm(t) + const { npm } = await loadMockNpm(t, { + npm: { argv: ['test-command', '--unknown-one', '--unknown-two'] }, + }) class TestCommand extends BaseCommand { static name = 'test-command' @@ -442,8 +446,11 @@ t.test('flags() throws error for multiple unknown flags with pluralization', asy const command = new TestCommand(npm) await t.rejects( command.exec(), - { message: /Unknown flags:.*--unknown-one.*--unknown-two/ }, - 'throws error with pluralized "flags" for multiple unknown flags' + { + code: 'EUNKNOWNCONFIG', + message: /Unknown cli configs:[\s\S]*--unknown-one[\s\S]*--unknown-two/, + }, + 'throws EUNKNOWNCONFIG with pluralized "configs" for multiple unknown flags' ) }) @@ -636,8 +643,8 @@ t.test('flags() throws error for extra positional arguments beyond expected coun // Should throw error for extra positional await t.rejects( command.exec(), - { message: 'Unknown positional argument: extra1' }, - 'throws error for first extra positional' + { message: 'Unknown positional arguments: extra1, extra2' }, + 'throws error for extra positionals' ) }) @@ -677,3 +684,178 @@ t.test('flags() does not throw when positionals is null (unlimited)', async t => t.same(remains, ['pkg1', 'extra1', 'extra2'], 'all positionals are in remains') t.equal(flags.id, null, 'id flag uses default') }) + +t.test('validateCli throws aggregated error for unknown file configs', async t => { + const { npm } = await loadMockNpm(t, { + homeDir: { + '.npmrc': [ + 'bogus-user-key=yes', + '@scope:bogus-scoped=no', + ].join('\n'), + }, + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + } + + const command = new TestCommand(npm) + await t.rejects( + (async () => command.validateCli())(), + { + code: 'EUNKNOWNCONFIG', + message: /Unknown npm configuration keys:[\s\S]*bogus-user-key[\s\S]*bogus-scoped[\s\S]*npm help npmrc/, + }, + 'throws EUNKNOWNCONFIG aggregating plain and scoped keys' + ) +}) + +t.test('validateCli uses singular "key" when only one unknown file config', async t => { + const { npm } = await loadMockNpm(t, { + homeDir: { + '.npmrc': 'only-one-bad-key=yes', + }, + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + } + + const command = new TestCommand(npm) + await t.rejects( + (async () => command.validateCli())(), + { + code: 'EUNKNOWNCONFIG', + message: /Unknown npm configuration key:\n/, + }, + 'singular phrasing when only one unknown key' + ) +}) + +t.test('validateCli bypasses checks when skipConfigValidation is set', async t => { + const { npm } = await loadMockNpm(t, { + homeDir: { + '.npmrc': 'bogus-user-key=yes', + }, + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + static skipConfigValidation = true + } + + const command = new TestCommand(npm) + t.doesNotThrow(() => command.validateCli(), 'skipConfigValidation bypasses unknown-file-config check') +}) + +t.test('validateCli combines cli and file unknowns into a single error', async t => { + const { npm } = await loadMockNpm(t, { + homeDir: { + '.npmrc': 'bogus-user-key=yes', + }, + npm: { argv: ['test-command', '--unknown-cli'] }, + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + async exec () { + return this.flags() + } + } + + const command = new TestCommand(npm) + let err + try { + command.validateCli() + t.fail('expected throw') + } catch (e) { + err = e + } + t.equal(err.code, 'EUNKNOWNCONFIG') + t.match(err.message, /Unknown cli config:[\s\S]*--unknown-cli/, + 'message contains the cli section') + t.match(err.message, /Unknown npm configuration key:[\s\S]*bogus-user-key/, + 'message contains the file section') + t.match(err.unknownConfigs, [ + { where: 'cli', key: 'unknown-cli' }, + { where: 'user', key: 'bogus-user-key' }, + ], 'structured payload contains both cli and file entries') +}) + +t.test('validateCli does not throw on unknown env (npm_config_*) configs', async t => { + // Env unknowns warn but never error — they are excluded from getUnknownConfigs() by default. + // This test locks in the carve-out at the validateCli boundary so the env→warn behavior cannot silently flip if the filter changes. + const { npm } = await loadMockNpm(t, { + globals: { + 'process.env.npm_config_bogus_env': 'yes', + }, + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + } + + const command = new TestCommand(npm) + t.doesNotThrow(() => command.validateCli(), + 'unknown env-set config does not throw at validateCli') + // sanity: the unknown was in fact seen by Config, just filtered from getUnknownConfigs() + t.ok( + npm.config.getUnknownConfigs('env').some(u => u.key === 'bogus-env'), + 'env unknown is still tracked internally' + ) +}) + +t.test('flags() throws with scoped (nerfdart) display for unknown cli config', async t => { + const { npm } = await loadMockNpm(t, { + argv: ['--@scope:bogus=yes'], + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + + async exec () { + return this.flags() + } + } + + npm.config.argv = ['node', 'npm', 'test-command', '--@scope:bogus=yes'] + const command = new TestCommand(npm) + await t.rejects( + command.exec(), + { + code: 'EUNKNOWNCONFIG', + message: /Unknown cli config:[\s\S]*--bogus \(@scope:bogus\)/, + }, + 'scoped display form uses baseKey (key) layout, throws EUNKNOWNCONFIG' + ) +}) + +t.test('flags() throws singular "argument" for a single extra positional', async t => { + const { npm } = await loadMockNpm(t, { + argv: ['pkg1', 'extra1'], + }) + + class TestCommand extends BaseCommand { + static name = 'test-command' + static description = 'Test command' + static positionals = 1 + + async exec () { + return this.flags() + } + } + + npm.config.argv = ['node', 'npm', 'test-command', 'pkg1', 'extra1'] + const command = new TestCommand(npm) + await t.rejects( + command.exec(), + { message: 'Unknown positional argument: extra1' }, + 'singular phrasing when only one extra positional' + ) +}) diff --git a/test/lib/commands/config.js b/test/lib/commands/config.js index 56f46981a066a..1ab9bbae67bd1 100644 --- a/test/lib/commands/config.js +++ b/test/lib/commands/config.js @@ -72,19 +72,17 @@ t.test('config ignores workspaces', async t => { t.test('config list', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { prefixDir: { - '.npmrc': 'projectloaded=yes', + '.npmrc': 'tag=from-project', }, globalPrefixDir: { etc: { - npmrc: 'globalloaded=yes', + npmrc: 'init-license=from-global', }, }, homeDir: { '.npmrc': [ - 'userloaded=yes', - 'auth=bad', + 'init-author-name=from-user', '_auth=bad', - '//nerfdart:auth=bad', '//nerfdart:_auth=bad', ].join('\n'), }, @@ -94,9 +92,9 @@ t.test('config list', async t => { const output = joinedOutput() - t.match(output, 'projectloaded = "yes"') - t.match(output, 'globalloaded = "yes"') - t.match(output, 'userloaded = "yes"') + t.match(output, 'tag = "from-project"') + t.match(output, 'init-license = "from-global"') + t.match(output, 'init-author-name = "from-user"') t.matchSnapshot(output, 'output matches snapshot') }) @@ -130,7 +128,7 @@ t.test('config list with proxy environment variables', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { prefixDir: { - '.npmrc': 'test=value', + '.npmrc': 'tag=value', }, }) @@ -147,15 +145,15 @@ t.test('config list with proxy environment variables', async t => { t.test('config list --long', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { prefixDir: { - '.npmrc': 'projectloaded=yes', + '.npmrc': 'tag=from-project', }, globalPrefixDir: { etc: { - npmrc: 'globalloaded=yes', + npmrc: 'init-license=from-global', }, }, homeDir: { - '.npmrc': 'userloaded=yes', + '.npmrc': 'init-author-name=from-user', }, config: { long: true, @@ -166,9 +164,9 @@ t.test('config list --long', async t => { const output = joinedOutput() - t.match(output, 'projectloaded = "yes"') - t.match(output, 'globalloaded = "yes"') - t.match(output, 'userloaded = "yes"') + t.match(output, 'tag = "from-project"') + t.match(output, 'init-license = "from-global"') + t.match(output, 'init-author-name = "from-user"') t.matchSnapshot(output, 'output matches snapshot') }) @@ -176,15 +174,15 @@ t.test('config list --long', async t => { t.test('config list --json', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { prefixDir: { - '.npmrc': 'projectloaded=yes', + '.npmrc': 'tag=from-project', }, globalPrefixDir: { etc: { - npmrc: 'globalloaded=yes', + npmrc: 'init-license=from-global', }, }, homeDir: { - '.npmrc': 'userloaded=yes', + '.npmrc': 'init-author-name=from-user', }, config: { json: true, @@ -195,9 +193,9 @@ t.test('config list --json', async t => { const output = joinedOutput() - t.match(output, '"projectloaded": "yes",') - t.match(output, '"globalloaded": "yes",') - t.match(output, '"userloaded": "yes",') + t.match(output, '"tag": "from-project"') + t.match(output, '"init-license": "from-global"') + t.match(output, '"init-author-name": "from-user"') t.matchSnapshot(output, 'output matches snapshot') }) @@ -569,9 +567,7 @@ t.test('config edit', async t => { homeDir: { '.npmrc': 'foo=bar\nbar=baz', }, - config: { - editor: EDITOR, - }, + npm: { argv: ['config', 'edit', '--editor=' + EDITOR] }, }) const inputEvents = [] @@ -643,6 +639,7 @@ t.test('config fix', (t) => { homeDir: { '.npmrc': '_authtoken=thisisinvalid\n_auth=beef', }, + npm: { argv: ['config', 'fix'] }, }) const registry = `//registry.npmjs.org/` @@ -686,6 +683,7 @@ t.test('config fix', (t) => { config: { location: 'user', }, + npm: { argv: ['config', 'fix', '--location=user'] }, }) const registry = `//registry.npmjs.org/` diff --git a/test/lib/commands/diff.js b/test/lib/commands/diff.js index 3d55cd7879b7a..0e30ac9b71cec 100644 --- a/test/lib/commands/diff.js +++ b/test/lib/commands/diff.js @@ -943,11 +943,11 @@ t.test('various options', async t => { t.test('using diff option', async t => { const { output } = await mockOptions(t, { - 'diff-context': 5, - 'diff-ignore-whitespace': true, + 'diff-unified': 5, + 'diff-ignore-all-space': true, 'diff-no-prefix': false, - 'diff-drc-prefix': 'foo/', - 'diff-fst-prefix': 'bar/', + 'diff-dst-prefix': 'foo/', + 'diff-src-prefix': 'bar/', 'diff-text': true, }) diff --git a/test/lib/commands/login.js b/test/lib/commands/login.js index 55568edd09f9d..f855fe683bd9f 100644 --- a/test/lib/commands/login.js +++ b/test/lib/commands/login.js @@ -49,7 +49,6 @@ t.test('legacy', t => { homeDir: { '.npmrc': [ '//registry.npmjs.org/:_authToken=user', - '//registry.npmjs.org/:always-auth=user', '//registry.npmjs.org/:email=test-email-old@npmjs.org', ].join('\n'), }, @@ -63,8 +62,8 @@ t.test('legacy', t => { t.same(npm.config.get('//registry.npmjs.org/:_authToken'), 'npm_test-token') t.same(rc(), { '//registry.npmjs.org/:_authToken': 'npm_test-token', - email: 'test-email-old@npmjs.org', - }, 'should only have token and un-nerfed old email') + '//registry.npmjs.org/:email': 'test-email-old@npmjs.org', + }, 'should only have token and nerfed email') }) t.test('scoped login default registry', async t => { diff --git a/test/lib/commands/logout.js b/test/lib/commands/logout.js index 840c92274bad0..b18b84ca48325 100644 --- a/test/lib/commands/logout.js +++ b/test/lib/commands/logout.js @@ -9,7 +9,7 @@ t.test('token logout - user config', async t => { homeDir: { '.npmrc': [ '//registry.npmjs.org/:_authToken=@foo/', - 'other-config=true', + 'fund=true', ].join('\n'), }, }) @@ -23,7 +23,7 @@ t.test('token logout - user config', async t => { 'should log message with correct registry' ) const userRc = await fs.readFile(join(home, '.npmrc'), 'utf-8') - t.equal(userRc.trim(), 'other-config=true') + t.equal(userRc.trim(), 'fund=true') }) t.test('token scoped logout - user config', async t => { @@ -60,7 +60,7 @@ t.test('user/pass logout - user config', async t => { '.npmrc': [ '//registry.npmjs.org/:username=foo', '//registry.npmjs.org/:_password=bar', - 'other-config=true', + 'fund=true', ].join('\n'), }, }) @@ -73,7 +73,7 @@ t.test('user/pass logout - user config', async t => { ) const userRc = await fs.readFile(join(home, '.npmrc'), 'utf-8') - t.equal(userRc.trim(), 'other-config=true') + t.equal(userRc.trim(), 'fund=true') }) t.test('missing credentials', async t => { @@ -95,7 +95,7 @@ t.test('ignore invalid scoped registry config', async t => { homeDir: { '.npmrc': [ '//registry.npmjs.org/:_authToken=@foo/', - 'other-config=true', + 'fund=true', ].join('\n'), }, @@ -111,7 +111,7 @@ t.test('ignore invalid scoped registry config', async t => { 'should log message with correct registry' ) const userRc = await fs.readFile(join(home, '.npmrc'), 'utf-8') - t.equal(userRc.trim(), 'other-config=true') + t.equal(userRc.trim(), 'fund=true') }) t.test('token logout - project config', async t => { @@ -119,13 +119,13 @@ t.test('token logout - project config', async t => { homeDir: { '.npmrc': [ '//registry.npmjs.org/:_authToken=@foo/', - 'other-config=true', + 'fund=true', ].join('\n'), }, prefixDir: { '.npmrc': [ '//registry.npmjs.org/:_authToken=@bar/', - 'other-config=true', + 'fund=true', ].join('\n'), }, }) @@ -142,7 +142,7 @@ t.test('token logout - project config', async t => { const userRc = await fs.readFile(join(home, '.npmrc'), 'utf-8') t.equal(userRc.trim(), [ '//registry.npmjs.org/:_authToken=@foo/', - 'other-config=true', + 'fund=true', ].join('\n'), 'leaves user config alone') t.equal( logs.verbose.byTitle('logout')[0], @@ -150,5 +150,5 @@ t.test('token logout - project config', async t => { 'should log message with correct registry' ) const projectRc = await fs.readFile(join(prefix, '.npmrc'), 'utf-8') - t.equal(projectRc.trim(), 'other-config=true', 'removes project config') + t.equal(projectRc.trim(), 'fund=true', 'removes project config') }) diff --git a/test/lib/commands/set.js b/test/lib/commands/set.js index 8574b80345e5c..fbb46571ecd1d 100644 --- a/test/lib/commands/set.js +++ b/test/lib/commands/set.js @@ -18,13 +18,13 @@ t.test('no args', async t => { t.test('test-config-item', async t => { const { npm, home, joinedOutput } = await mockNpm(t, { homeDir: { - '.npmrc': 'original-config-test=original value', + '.npmrc': 'tag=beta', }, }) t.equal( - npm.config.get('original-config-test'), - 'original value', + npm.config.get('tag'), + 'beta', 'original config is set from npmrc' ) @@ -46,7 +46,7 @@ t.test('test-config-item', async t => { t.equal( cleanNewlines(await fs.readFile(join(home, '.npmrc'), 'utf-8')), [ - 'original-config-test=original value', + 'tag=beta', 'fund=true', '', ].join('\n'), diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index a1acb7969b29f..2aae7efbefb09 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -60,6 +60,10 @@ class Config { // populated the first time we flatten the object #flatOptions = null #warnings = [] + // Unknown configs collected during load(), aggregated and thrown by `BaseCommand.validateCli` once command-specific definitions are known. + // Entries: { where, key, baseKey, source }. `baseKey` is non-null only for scoped keys (e.g. `:foo`) and holds the trailing segment. + // `env` and `publishConfig` entries warn instead of error. + #unknownConfigs = [] static get typeDefs () { return typeDefs @@ -359,7 +363,10 @@ class Config { loadCLI () { for (const s of Object.keys(this.shorthands)) { if (s.length > 1 && this.argv.includes(`-${s}`)) { - log.warn(`-${s} is not a valid single-hyphen cli flag and will be removed in the future`) + throw Object.assign( + new Error(`-${s} is not a valid single-hyphen cli flag. Did you mean --${s}?`), + { code: 'EUNKNOWNCONFIG' } + ) } } nopt.invalidHandler = (k, val, type) => @@ -431,6 +438,32 @@ class Config { } } } + + // Top-level `email`, `certfile`, and `keyfile` must be in nerfdart form. + // certfile/keyfile are an mTLS pair: drop a lone one rather than migrating it. + if (this.get('email', entryWhere)) { + authProblems.push({ + action: 'rename', + from: 'email', + to: `${nerfedReg}:email`, + where: entryWhere, + }) + } + for (const key of ['certfile', 'keyfile']) { + if (this.get(key, entryWhere)) { + const pair = key === 'certfile' ? 'keyfile' : 'certfile' + if (!this.get(pair, entryWhere)) { + authProblems.push({ action: 'delete', key, where: entryWhere }) + } else { + authProblems.push({ + action: 'rename', + from: key, + to: `${nerfedReg}:${key}`, + where: entryWhere, + }) + } + } + } } } @@ -481,10 +514,15 @@ class Config { if (problem.action === 'delete') { this.delete(problem.key, problem.where) } else if (problem.action === 'rename') { - const raw = this.data.get(problem.where).raw?.[problem.from] - const calculated = this.get(problem.from, problem.where) - this.set(problem.to, raw || calculated, problem.where) - this.delete(problem.from, problem.where) + // If the destination already exists, drop the source rather than clobber. + if (this.find(problem.to) !== null) { + this.delete(problem.from, problem.where) + } else { + const raw = this.data.get(problem.where).raw?.[problem.from] + const calculated = this.get(problem.from, problem.where) + this.set(problem.to, raw || calculated, problem.where) + this.delete(problem.from, problem.where) + } } } } @@ -535,7 +573,10 @@ class Config { } abbrevHandler (short, long) { - log.warn(`Expanding --${short} to --${long}. This will stop working in the next major version of npm.`) + throw Object.assign( + new Error(`Invalid abbreviated flag "--${short}". Did you mean "--${long}"?`), + { code: 'EUNKNOWNCONFIG' } + ) } unknownHandler (key, next) { @@ -603,30 +644,64 @@ class Config { } } if (where !== 'default' || key === 'npm-version') { - this.checkUnknown(where, key) + this.checkUnknown(where, key, source) } conf.data[k] = v } } } - checkUnknown (where, key) { - if (!this.definitions[key]) { - if (internalEnv.includes(key)) { - return - } - const hint = where !== 'cli' - ? ' See `npm help npmrc` for supported config options.' - : '' - if (!key.includes(':')) { - this.queueWarning(key, `Unknown ${where} config "${where === 'cli' ? '--' : ''}${key}". This will stop working in the next major version of npm.${hint}`) + checkUnknown (where, key, source = null) { + if (this.definitions[key]) { + return + } + if (internalEnv.includes(key)) { + return + } + const scoped = key.includes(':') + let baseKey = null + if (scoped) { + baseKey = key.split(':').pop() + if (this.definitions[baseKey] || this.nerfDarts.includes(baseKey)) { return } - const baseKey = key.split(':').pop() - if (!this.definitions[baseKey] && !this.nerfDarts.includes(baseKey)) { - this.queueWarning(baseKey, `Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.${hint}`) - } } + + const entry = { where, key, baseKey, source: source ?? this.data.get(where)?.source ?? null } + this.#unknownConfigs.push(entry) + + // publishConfig is handled by publish/unpublish/config commands and is out of scope for the npm 12 breaking change (tracked separately). + // Keep it as a queued warning so existing behavior is preserved. + if (where === 'publishConfig') { + const hint = ' See `npm help npmrc` for supported config options.' + const msg = scoped + ? `Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.${hint}` + : `Unknown ${where} config "${key}". This will stop working in the next major version of npm.${hint}` + this.queueWarning(scoped ? baseKey : key, msg) + return + } + + // env unknowns are an explicit carve-out for npm 12. + // setEnvs() exports npm_config_* for child processes and loadEnv() re-ingests them; erroring here would break npm-invoked-npm and many CI setups. + // Keep as warning. Planned to error in npm 13. + if (where === 'env') { + const hint = ' See `npm help npmrc` for supported config options.' + const msg = scoped + ? `Unknown ${where} config "${baseKey}" (${key}). This will error in a future major version of npm.${hint}` + : `Unknown ${where} config "${key}". This will error in a future major version of npm.${hint}` + this.queueWarning(scoped ? baseKey : key, msg) + } + + // cli + file sources (builtin/project/user/global): collected only. + // Aggregated and thrown by `BaseCommand.validateCli` after subcommand flag resolution so command-specific flags can be allow-listed first. + } + + // Returns unknown-config entries for a given source ('cli', 'builtin', 'project', 'user', 'global') or all entries except `env` and `publishConfig` when omitted. + getUnknownConfigs (where) { + if (where) { + return this.#unknownConfigs.filter(u => u.where === where) + } + return this.#unknownConfigs.filter(u => u.where !== 'env' && u.where !== 'publishConfig') } #checkDeprecated (key) { @@ -782,13 +857,8 @@ class Config { conf[_loadError] = null if (where === 'user') { - // if email is nerfed, then we want to de-nerf it - const nerfed = nerfDart(this.get('registry')) - const email = this.get(`${nerfed}:email`, 'user') - if (email) { - this.delete(`${nerfed}:email`, 'user') - this.set('email', email, 'user') - } + // Historically, save('user') would "de-nerf" email — move a scoped `:email` into a top-level `email` key — because npm used to warn on nerfed email. + // In npm 12 the top-level `email` key is a hard error, so we keep email in its scoped form. } // We need the actual raw data before we called parseField so that we are @@ -816,11 +886,7 @@ class Config { this.delete(`_auth`, level) this.delete(`_password`, level) this.delete(`username`, level) - // de-nerf email if it's nerfed to the default registry - const email = this.get(`${nerfed}:email`, level) - if (email) { - this.set('email', email, level) - } + // In npm 12, top-level `email` is a hard error — don't de-nerf it here either. } this.delete(`${nerfed}:_authToken`, level) this.delete(`${nerfed}:_auth`, level) @@ -839,7 +905,9 @@ class Config { // send auth if we have it, only to the URIs under the nerf dart. this.delete(`${nerfed}:always-auth`, 'user') - this.delete(`${nerfed}:email`, 'user') + // NOTE: we intentionally do NOT delete `${nerfed}:email` here anymore. + // In npm 11 and earlier, top-level `email` was the canonical form (with getCredentialsByURI copying scoped email up to top-level on read), and setCredentialsByURI cleared the scoped copy to enforce that invariant. + // In npm 12 top-level `email` is a hard error and the scoped nerfdart form is canonical, so preserve any existing scoped email across login. if (certfile && keyfile) { this.set(`${nerfed}:certfile`, certfile, 'user') this.set(`${nerfed}:keyfile`, keyfile, 'user') @@ -870,17 +938,13 @@ class Config { // this has to be a bit more complicated to support legacy data of all forms getCredentialsByURI (uri) { const nerfed = nerfDart(uri) - const def = nerfDart(this.get('registry')) const creds = {} - // email is handled differently, it used to always be nerfed and now it never should be - // if it's set nerfed to the default registry, then we copy it to the unnerfed key + // email is handled differently, it used to always be nerfed and now it never should be. + // In npm 12 the top-level `email` key is a hard error, so we stop copying scoped email back to the top-level here. // TODO: evaluate removing 'email' from the credentials object returned here const email = this.get(`${nerfed}:email`) || this.get('email') if (email) { - if (nerfed === def) { - this.set('email', email, 'user') - } creds.email = email } diff --git a/workspaces/config/tap-snapshots/test/index.js.test.cjs b/workspaces/config/tap-snapshots/test/index.js.test.cjs index eccdbee3abced..935f328ae5659 100644 --- a/workspaces/config/tap-snapshots/test/index.js.test.cjs +++ b/workspaces/config/tap-snapshots/test/index.js.test.cjs @@ -5,26 +5,6 @@ * Make sure to inspect the output below. Do not ignore changes! */ 'use strict' -exports[`test/index.js TAP credentials management def_auth > default registry 1`] = ` -Object { - "auth": "aGVsbG86d29ybGQ=", - "password": "world", - "username": "hello", -} -` - -exports[`test/index.js TAP credentials management def_auth > default registry after set 1`] = ` -Object { - "auth": "aGVsbG86d29ybGQ=", - "password": "world", - "username": "hello", -} -` - -exports[`test/index.js TAP credentials management def_auth > other registry 1`] = ` -Object {} -` - exports[`test/index.js TAP credentials management def_authEnv > default registry 1`] = ` Object { "auth": "\${PATH}", @@ -37,54 +17,6 @@ exports[`test/index.js TAP credentials management def_authEnv > other registry 1 Object {} ` -exports[`test/index.js TAP credentials management def_passNoUser > default registry 1`] = ` -Object { - "email": "i@izs.me", -} -` - -exports[`test/index.js TAP credentials management def_passNoUser > other registry 1`] = ` -Object { - "email": "i@izs.me", -} -` - -exports[`test/index.js TAP credentials management def_userNoPass > default registry 1`] = ` -Object { - "email": "i@izs.me", -} -` - -exports[`test/index.js TAP credentials management def_userNoPass > other registry 1`] = ` -Object { - "email": "i@izs.me", -} -` - -exports[`test/index.js TAP credentials management def_userpass > default registry 1`] = ` -Object { - "auth": "aGVsbG86d29ybGQ=", - "email": "i@izs.me", - "password": "world", - "username": "hello", -} -` - -exports[`test/index.js TAP credentials management def_userpass > default registry after set 1`] = ` -Object { - "auth": "aGVsbG86d29ybGQ=", - "email": "i@izs.me", - "password": "world", - "username": "hello", -} -` - -exports[`test/index.js TAP credentials management def_userpass > other registry 1`] = ` -Object { - "email": "i@izs.me", -} -` - exports[`test/index.js TAP credentials management nerfed_auth > default registry 1`] = ` Object { "auth": "aGVsbG86d29ybGQ=", @@ -174,7 +106,6 @@ exports[`test/index.js TAP credentials management nerfed_mtlsUserPass > default Object { "auth": "aGVsbG86d29ybGQ=", "certfile": "/path/to/cert", - "email": "i@izs.me", "keyfile": "/path/to/key", "password": "world", "username": "hello", @@ -182,9 +113,7 @@ Object { ` exports[`test/index.js TAP credentials management nerfed_mtlsUserPass > other registry 1`] = ` -Object { - "email": "i@izs.me", -} +Object {} ` exports[`test/index.js TAP credentials management nerfed_userpass > default registry 1`] = ` @@ -199,31 +128,12 @@ Object { exports[`test/index.js TAP credentials management nerfed_userpass > default registry after set 1`] = ` Object { "auth": "aGVsbG86d29ybGQ=", - "email": "i@izs.me", "password": "world", "username": "hello", } ` exports[`test/index.js TAP credentials management nerfed_userpass > other registry 1`] = ` -Object { - "email": "i@izs.me", -} -` - -exports[`test/index.js TAP credentials management none_authToken > default registry 1`] = ` -Object { - "token": "0bad1de4", -} -` - -exports[`test/index.js TAP credentials management none_authToken > default registry after set 1`] = ` -Object { - "token": "0bad1de4", -} -` - -exports[`test/index.js TAP credentials management none_authToken > other registry 1`] = ` Object {} ` @@ -235,14 +145,6 @@ exports[`test/index.js TAP credentials management none_emptyConfig > other regis Object {} ` -exports[`test/index.js TAP credentials management none_lcAuthToken > default registry 1`] = ` -Object {} -` - -exports[`test/index.js TAP credentials management none_lcAuthToken > other registry 1`] = ` -Object {} -` - exports[`test/index.js TAP credentials management none_noConfig > default registry 1`] = ` Object {} ` diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index d739b88fb5229..b5f59aab5768c 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -45,7 +45,26 @@ const fsMocks = { 'node:fs': mockFs, } -const { definitions, shorthands, nerfDarts, flatten } = t.mock('../lib/definitions/index.js', fsMocks) +const { definitions: realDefinitions, shorthands, nerfDarts, flatten } = + t.mock('../lib/definitions/index.js', fsMocks) + +// Extend the real definitions with stub entries for fake config keys used +// in the shared fixture below. Treating them as known lets merge-order, +// env-override, and similar tests exercise Config.load() without tripping +// the npm 12 unknown-config throw. Tests that want to exercise unknown +// behavior use keys NOT in this stub list (e.g. "totally-unknown-key"). +const stubDef = (key) => ({ key, type: [String, Boolean] }) +const definitions = { + ...realDefinitions, + 'builtin-config': stubDef('builtin-config'), + 'global-config': stubDef('global-config'), + 'user-config-from-builtin': stubDef('user-config-from-builtin'), + 'default-user-config-in-home': stubDef('default-user-config-in-home'), + 'project-config': stubDef('project-config'), + 'cli-config': stubDef('cli-config'), + foo: stubDef('foo'), + bAr: stubDef('bAr'), +} const Config = t.mock('../', fsMocks) // because we used t.mock above, the require cache gets blown and we lose our direct equality @@ -138,6 +157,7 @@ prefix = ${path}/global `, '.npmrc-from-builtin': ` user-config-from-builtin = true +default-user-config-in-home = true foo = from-custom-userconfig globalconfig = ${path}/global/etc/npmrc `, @@ -186,6 +206,7 @@ loglevel = yolo cwd: join(`${path}/project`), shorthands, definitions, + nerfDarts, }) await t.rejects(() => config.load(), { message: `double-loading config "${resolve(path, 'npm/npmrc')}" as "user",` + @@ -201,6 +222,7 @@ loglevel = yolo cwd: join(`${path}/project`), shorthands, definitions, + nerfDarts, }) await config.load() @@ -217,6 +239,7 @@ loglevel = yolo cwd: join(`${path}/project`), shorthands, definitions, + nerfDarts, }) await config.load() @@ -238,6 +261,7 @@ loglevel = yolo cwd: path, shorthands, definitions, + nerfDarts, }) logs.length = 0 await config.load() @@ -288,10 +312,10 @@ loglevel = yolo t.equal(config.get('global', 'env'), undefined, 'empty env is missing') t.equal(config.get('global'), false, 'empty env is missing') - config.set('asdf', 'quux', 'global') + config.set('foo', 'quux', 'global') await config.save('global') const gres = readFileSync(`${path}/global/etc/npmrc`, 'utf8') - t.match(gres, 'asdf=quux') + t.match(gres, 'foo=quux') const cliData = config.data.get('cli') t.throws(() => cliData.loadError = true, { @@ -381,8 +405,6 @@ loglevel = yolo // warn logs are emitted as a side effect of validate config.validate() t.strictSame(logs.filter(l => l[0] === 'warn'), [ - ['warn', 'Unknown builtin config "builtin-config". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown builtin config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], ['warn', 'invalid config', 'registry="hello"', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'full url with "http://"'], ['warn', 'invalid config', 'proxy="hello"', 'set in command line options'], @@ -399,13 +421,6 @@ loglevel = yolo ['warn', 'invalid config', 'prefix=true', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'valid filesystem path'], ['warn', 'config', 'also', 'Please use --include=dev instead.'], - ['warn', 'Unknown env config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown project config "project-config". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown project config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown user config "user-config-from-builtin". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown user config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown global config "global-config". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown global config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], ['warn', 'invalid config', 'loglevel="yolo"', `set in ${resolve(path, 'project/.npmrc')}`], ['warn', 'invalid config', 'Must be one of:', ['silent', 'error', 'warn', 'notice', 'http', 'info', 'verbose', 'silly'].join(', '), @@ -458,6 +473,7 @@ loglevel = yolo shorthands, definitions, + nerfDarts, }) await config.load() @@ -491,6 +507,7 @@ loglevel = yolo shorthands, definitions, + nerfDarts, }) await config.load() @@ -600,12 +617,6 @@ loglevel = yolo ['warn', 'invalid config', 'prefix=true', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'valid filesystem path'], ['warn', 'config', 'also', 'Please use --include=dev instead.'], - ['warn', 'Unknown env config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown user config "default-user-config-in-home". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown user config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown global config "global-config". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown global config "foo". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], - ['warn', 'Unknown global config "asdf". This will stop working in the next major version of npm. See `npm help npmrc` for supported config options.'], ]) logs.length = 0 }) @@ -627,6 +638,7 @@ t.test('cafile loads as ca (and some saving tests)', async t => { const config = new Config({ shorthands, definitions, + nerfDarts, npmPath: __dirname, env: { HOME: dir, PREFIX: dir }, flatten, @@ -665,6 +677,7 @@ fakey mc fakerson const config = new Config({ shorthands, definitions, + nerfDarts, npmPath: __dirname, env: { HOME: dir, @@ -686,6 +699,7 @@ t.test('ignore cafile if it does not load', async t => { const config = new Config({ shorthands, definitions, + nerfDarts, npmPath: __dirname, env: { HOME: dir }, }) @@ -704,6 +718,7 @@ t.test('raise error if reading ca file error other than ENOENT', async t => { const config = new Config({ shorthands, definitions, + nerfDarts, npmPath: __dirname, env: { HOME: dir }, flatten, @@ -718,8 +733,7 @@ t.test('credentials management', async t => { nerfed_userpass: { '.npmrc': `//registry.example/:username = hello //registry.example/:_password = ${Buffer.from('world').toString('base64')} -//registry.example/:email = i@izs.me -//registry.example/:always-auth = "false"`, +//registry.example/:email = i@izs.me`, }, nerfed_auth: { // note: does not load, because we don't do _auth per reg '.npmrc': `//registry.example/:_auth = ${Buffer.from('hello:world').toString('base64')}`, @@ -734,7 +748,6 @@ t.test('credentials management', async t => { nerfed_mtlsUserPass: { '.npmrc': `//registry.example/:username = hello //registry.example/:_password = ${Buffer.from('world').toString('base64')} //registry.example/:email = i@izs.me -//registry.example/:always-auth = "false" //registry.example/:certfile = /path/to/cert //registry.example/:keyfile = /path/to/key`, }, @@ -773,17 +786,40 @@ always-auth = true`, const defReg = 'https://registry.example/' const otherReg = 'https://other.registry/' + // Cases whose fixtures include top-level legacy auth keys that are no + // longer tolerated in npm 12 — Config collects them as unknowns; the + // error is raised by base-cmd.validateCli when the command runs. + const mustCollect = new Set([ + 'def_userpass', + 'def_userNoPass', + 'def_passNoUser', + 'def_auth', + 'none_authToken', + 'none_lcAuthToken', + ]) for (const testCase of Object.keys(fixtures)) { t.test(testCase, async t => { const c = new Config({ npmPath: path, shorthands, definitions, + nerfDarts, env: { HOME: resolve(path, testCase) }, argv: ['node', 'file', '--registry', defReg], }) + await c.load() + if (mustCollect.has(testCase)) { + const unknowns = [ + ...c.getUnknownConfigs('user'), + ...c.getUnknownConfigs('project'), + ...c.getUnknownConfigs('global'), + ] + t.ok(unknowns.length > 0, 'unknown top-level legacy auth keys collected') + return + } + // only have to do this the first time, it's redundant otherwise if (testCase === 'none_noConfig') { t.throws(() => c.setCredentialsByURI('http://x.com', { @@ -801,25 +837,20 @@ always-auth = true`, }) } - // the def_ and none_ prefixed cases have unscoped auth values and should throw - if (testCase.startsWith('def_') || - testCase === 'none_authToken' || - testCase === 'none_lcAuthToken') { + // def_authEnv still needs validate+repair (uses defined `_auth` at top + // level, which loads fine but validate flags as incomplete auth). + if (testCase === 'def_authEnv') { try { c.validate() - // validate should throw, fail the test here if it doesn't t.fail('validate should have thrown') } catch (err) { if (err.code !== 'ERR_INVALID_AUTH') { throw err } - - // we got our expected invalid auth error, so now repair it c.repair(err.problems) t.ok(c.valid, 'config is valid') } } else { - // validate won't throw for these ones, so let's prove it and repair are no-ops c.validate() c.repair() } @@ -878,6 +909,7 @@ t.test('finding the global prefix', t => { }, shorthands, definitions, + nerfDarts, npmPath, }) c.loadGlobalPrefix() @@ -893,6 +925,7 @@ t.test('finding the global prefix', t => { execPath: '/path/to/nodejs/node.exe', shorthands, definitions, + nerfDarts, npmPath, }) c.loadGlobalPrefix() @@ -905,6 +938,7 @@ t.test('finding the global prefix', t => { execPath: '/path/to/nodejs/bin/node', shorthands, definitions, + nerfDarts, npmPath, }) c.loadGlobalPrefix() @@ -918,6 +952,7 @@ t.test('finding the global prefix', t => { env: { DESTDIR: '/some/dest/dir' }, shorthands, definitions, + nerfDarts, npmPath, }) c.loadGlobalPrefix() @@ -943,6 +978,7 @@ t.test('finding the local prefix', t => { argv: [process.execPath, __filename, '-C', path], shorthands, definitions, + nerfDarts, npmPath: path, }) await c.load() @@ -953,6 +989,7 @@ t.test('finding the local prefix', t => { cwd: join(`${path}/hasNM/x/y/z`), shorthands, definitions, + nerfDarts, npmPath: path, }) await c.load() @@ -963,6 +1000,7 @@ t.test('finding the local prefix', t => { cwd: join(`${path}/hasPJ/x/y/z`), shorthands, definitions, + nerfDarts, npmPath: path, }) await c.load() @@ -973,6 +1011,7 @@ t.test('finding the local prefix', t => { cwd: join('/this/path/does/not/exist/x/y/z'), shorthands, definitions, + nerfDarts, npmPath: path, }) await c.load() @@ -992,26 +1031,29 @@ t.test('setting basic auth creds and email', async t => { definitions: { registry: { default: registry }, }, - npmPath: process.cwd(), + nerfDarts, + cwd: path, + excludeNpmCwd: true, + npmPath: path, } const c = new Config(opts) await c.load() - c.set('email', 'name@example.com', 'user') - t.equal(c.get('email', 'user'), 'name@example.com', 'email was set') + c.setCredentialsByURI(registry, { + username: 'admin', + password: 'admin', + }) + c.set('//registry.npmjs.org/:email', 'name@example.com', 'user') await c.save('user') - t.equal(c.get('email', 'user'), 'name@example.com', 'email still top level') - t.strictSame(c.getCredentialsByURI(registry), { email: 'name@example.com' }) + t.strictSame(c.getCredentialsByURI(registry), { + email: 'name@example.com', + username: 'admin', + password: 'admin', + auth: _auth, + }) const d = new Config(opts) await d.load() - t.strictSame(d.getCredentialsByURI(registry), { email: 'name@example.com' }) - d.set('_auth', _auth, 'user') - t.equal(d.get('_auth', 'user'), _auth, '_auth was set') - d.repair() - await d.save('user') - const e = new Config(opts) - await e.load() - t.equal(e.get('_auth', 'user'), undefined, 'un-nerfed _auth deleted') - t.strictSame(e.getCredentialsByURI(registry), { + t.equal(d.get('_auth', 'user'), undefined, 'un-nerfed _auth not present') + t.strictSame(d.getCredentialsByURI(registry), { email: 'name@example.com', username: 'admin', password: 'admin', @@ -1029,26 +1071,25 @@ t.test('setting username/password/email individually', async t => { definitions: { registry: { default: registry }, }, - npmPath: process.cwd(), + nerfDarts, + cwd: path, + excludeNpmCwd: true, + npmPath: path, } const c = new Config(opts) await c.load() - c.set('email', 'name@example.com', 'user') - t.equal(c.get('email'), 'name@example.com') - c.set('username', 'admin', 'user') - t.equal(c.get('username'), 'admin') - c.set('_password', Buffer.from('admin').toString('base64'), 'user') - t.equal(c.get('_password'), Buffer.from('admin').toString('base64')) + c.setCredentialsByURI(registry, { + username: 'admin', + password: 'admin', + }) + c.set('//registry.npmjs.org/:email', 'name@example.com', 'user') + t.equal(c.get('//registry.npmjs.org/:email'), 'name@example.com') + t.equal(c.get('//registry.npmjs.org/:username'), 'admin') t.equal(c.get('_auth'), undefined) - c.repair() await c.save('user') const d = new Config(opts) await d.load() - t.equal(d.get('email'), 'name@example.com') - t.equal(d.get('username'), undefined) - t.equal(d.get('_password'), undefined) - t.equal(d.get('_auth'), undefined) t.strictSame(d.getCredentialsByURI(registry), { email: 'name@example.com', username: 'admin', @@ -1057,7 +1098,7 @@ t.test('setting username/password/email individually', async t => { }) }) -t.test('nerfdart auths set at the top level into the registry', async t => { +t.test('nerfdart auths set at the top level now throw in npm 12', async t => { const registry = 'https://registry.npmjs.org/' const _auth = Buffer.from('admin:admin').toString('base64') const username = 'admin' @@ -1065,90 +1106,301 @@ t.test('nerfdart auths set at the top level into the registry', async t => { const email = 'i@izs.me' const _authToken = 'deadbeefblahblah' - // name: [ini, expect, wontThrow] - const cases = { - '_auth only, no email': [`_auth=${_auth}`, { - '//registry.npmjs.org/:_auth': _auth, - }], - '_auth with email': [`_auth=${_auth}\nemail=${email}`, { - '//registry.npmjs.org/:_auth': _auth, - email, - }], - '_authToken alone': [`_authToken=${_authToken}`, { - '//registry.npmjs.org/:_authToken': _authToken, - }], - '_authToken and email': [`_authToken=${_authToken}\nemail=${email}`, { - '//registry.npmjs.org/:_authToken': _authToken, - email, - }], - 'username and _password': [`username=${username}\n_password=${_password}`, { - '//registry.npmjs.org/:username': username, - '//registry.npmjs.org/:_password': _password, - }], - 'username, password, email': [`username=${username}\n_password=${_password}\nemail=${email}`, { - '//registry.npmjs.org/:username': username, - '//registry.npmjs.org/:_password': _password, - email, - }], - // handled invalid/legacy cases - 'username, no _password': [`username=${username}`, {}], - '_password, no username': [`_password=${_password}`, {}], - '_authtoken instead of _authToken': [`_authtoken=${_authToken}`, {}], - '-authtoken instead of _authToken': [`-authtoken=${_authToken}`, {}], - // de-nerfdart the email, if present in that way - 'nerf-darted email': [`//registry.npmjs.org/:email=${email}`, { - email, - }, true], + // All these legacy fixtures place auth-like keys at the top level of an + // .npmrc. In npm 11 these produced warnings and `repair()` migrated them + // into a nerfdart section. In npm 12 they are collected as unknown + // configs; base-cmd.validateCli throws on them when the command runs. + const throwingCases = { + '_auth only, no email': `_auth=${_auth}`, + '_auth with email': `_auth=${_auth}\nemail=${email}`, + '_authToken alone': `_authToken=${_authToken}`, + '_authToken and email': `_authToken=${_authToken}\nemail=${email}`, + 'username and _password': `username=${username}\n_password=${_password}`, + 'username, password, email': + `username=${username}\n_password=${_password}\nemail=${email}`, + 'username, no _password': `username=${username}`, + '_password, no username': `_password=${_password}`, + '_authtoken instead of _authToken': `_authtoken=${_authToken}`, + '-authtoken instead of _authToken': `-authtoken=${_authToken}`, } - const logs = [] - const logHandler = (...args) => logs.push(args) - process.on('log', logHandler) - t.teardown(() => { - process.removeListener('log', logHandler) - }) - const cwd = process.cwd() - for (const [name, [ini, expect, wontThrow]] of Object.entries(cases)) { + for (const [name, ini] of Object.entries(throwingCases)) { t.test(name, async t => { - t.teardown(() => { - process.chdir(cwd) - logs.length = 0 - }) const path = t.testdir({ '.npmrc': ini, 'package.json': JSON.stringify({}), }) - process.chdir(path) - const argv = [ - 'node', - __filename, - `--prefix=${path}`, - `--userconfig=${path}/.npmrc`, - `--globalconfig=${path}/etc/npmrc`, - ] const opts = { shorthands: {}, - argv, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], env: {}, definitions: { registry: { default: registry }, }, - npmPath: process.cwd(), + cwd: path, + excludeNpmCwd: true, + npmPath: path, } - const c = new Config(opts) await c.load() + const unknowns = [ + ...c.getUnknownConfigs('user'), + ...c.getUnknownConfigs('project'), + ...c.getUnknownConfigs('global'), + ...c.getUnknownConfigs('builtin'), + ] + t.ok(unknowns.length > 0, 'legacy top-level auth keys collected as unknown') + t.throws( + () => c.validate(), + { code: 'ERR_INVALID_AUTH' }, + 'validate() throws ErrInvalidAuth with a message describing each problem' + ) + }) + } - if (!wontThrow) { - t.throws(() => c.validate(), { code: 'ERR_INVALID_AUTH' }) - } + t.test('nerf-darted email still loads', async t => { + const path = t.testdir({ + '.npmrc': `//registry.npmjs.org/:email=${email}`, + 'package.json': JSON.stringify({}), + }) + const opts = { + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: {}, + definitions: { + registry: { default: registry }, + }, + nerfDarts, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + } + const c = new Config(opts) + await c.load() + c.repair() + await c.save('user') + t.same(c.data.get('user').data, { '//registry.npmjs.org/:email': email }) + }) +}) - // now we go ahead and do the repair, and save - c.repair() - await c.save('user') - t.same(c.data.get('user').data, expect) +t.test('checkUnknown and repair carve-outs', async t => { + const registry = 'https://registry.npmjs.org/' + const _authToken = 'deadbeef' + + t.test('repair() with no args validates and fixes auth problems', async t => { + const path = t.testdir({ + '.npmrc': `_authtoken=${_authToken}`, + 'package.json': JSON.stringify({}), }) - } + const c = new Config({ + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: {}, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + }) + await c.load() + t.throws(() => c.validate(), { code: 'ERR_INVALID_AUTH' }, 'pre-repair validate throws') + c.repair() + t.equal(c.get('_authtoken', 'user'), undefined, 'deleted from user config') + t.doesNotThrow(() => c.validate(), 'post-repair validate passes') + }) + + t.test('repair() migrates top-level email to nerfdart form', async t => { + const email = 'me@example.com' + const path = t.testdir({ + '.npmrc': `email=${email}`, + 'package.json': JSON.stringify({}), + }) + const c = new Config({ + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: {}, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + }) + await c.load() + t.throws(() => c.validate(), { code: 'ERR_INVALID_AUTH' }, + 'pre-repair validate flags top-level email') + c.repair() + t.equal(c.get('email', 'user'), undefined, 'top-level email deleted') + t.equal(c.get('//registry.npmjs.org/:email', 'user'), email, + 'email moved to nerfdart form') + t.doesNotThrow(() => c.validate(), 'post-repair validate passes') + }) + + t.test('repair() migrates certfile+keyfile pair to nerfdart form', async t => { + const path = t.testdir({ + '.npmrc': 'certfile=/path/to/cert\nkeyfile=/path/to/key', + 'package.json': JSON.stringify({}), + }) + const c = new Config({ + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: {}, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + }) + await c.load() + t.throws(() => c.validate(), { code: 'ERR_INVALID_AUTH' }, + 'pre-repair validate flags top-level certfile/keyfile') + c.repair() + t.equal(c.get('certfile', 'user'), undefined, 'top-level certfile deleted') + t.equal(c.get('keyfile', 'user'), undefined, 'top-level keyfile deleted') + t.equal(c.get('//registry.npmjs.org/:certfile', 'user'), '/path/to/cert', + 'certfile moved to nerfdart form') + t.equal(c.get('//registry.npmjs.org/:keyfile', 'user'), '/path/to/key', + 'keyfile moved to nerfdart form') + t.doesNotThrow(() => c.validate(), 'post-repair validate passes') + }) + + t.test('repair() drops orphan certfile (no matching keyfile)', async t => { + const path = t.testdir({ + '.npmrc': 'certfile=/path/to/cert', + 'package.json': JSON.stringify({}), + }) + const c = new Config({ + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: {}, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + }) + await c.load() + c.repair() + t.equal(c.get('certfile', 'user'), undefined, 'orphan certfile deleted') + t.equal(c.get('//registry.npmjs.org/:certfile', 'user'), undefined, + 'orphan certfile NOT moved (useless without keyfile)') + }) + + t.test('repair() preserves existing scoped destination on collision', async t => { + // Stale top-level + current scoped should keep the scoped value. + const stale = 'old@example.com' + const current = 'new@example.com' + const path = t.testdir({ + '.npmrc': `email=${stale}\n//registry.npmjs.org/:email=${current}`, + 'package.json': JSON.stringify({}), + }) + const c = new Config({ + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: {}, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + }) + await c.load() + c.repair() + t.equal(c.get('email', 'user'), undefined, 'stale top-level email deleted') + t.equal(c.get('//registry.npmjs.org/:email', 'user'), current, + 'existing scoped email preserved (not clobbered)') + }) + + t.test('publishConfig unknown warns but does not error', async t => { + const path = t.testdir({ 'package.json': JSON.stringify({}) }) + const c = new Config({ + shorthands: {}, + argv: ['node', __filename, `--prefix=${path}`], + env: {}, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + warn: false, + }) + await c.load() + c.checkUnknown('publishConfig', 'bogus-pub-key') + c.checkUnknown('publishConfig', '@scope:bogus-scoped-pub') + const pubUnknowns = c.getUnknownConfigs('publishConfig') + t.equal(pubUnknowns.length, 2, 'publishConfig unknowns collected') + t.notOk( + c.getUnknownConfigs().some(u => u.where === 'publishConfig'), + 'publishConfig unknowns excluded from default getUnknownConfigs()' + ) + }) + + t.test('getUnknownConfigs() returns all file + cli entries', async t => { + const path = t.testdir({ + '.npmrc': 'bogus-user-key=yes', + 'package.json': JSON.stringify({}), + }) + const c = new Config({ + shorthands: {}, + argv: [ + 'node', + __filename, + `--prefix=${path}`, + `--userconfig=${path}/.npmrc`, + `--globalconfig=${path}/etc/npmrc`, + ], + env: { + npm_config_bogus_env_key: 'x', + 'npm_config_@scope:bogus-scoped-env': 'y', + }, + definitions: { registry: { default: registry } }, + cwd: path, + excludeNpmCwd: true, + npmPath: path, + }) + await c.load() + const all = c.getUnknownConfigs() + t.ok(all.some(u => u.key === 'bogus-user-key' && u.where === 'user'), 'includes user file key') + t.notOk(all.some(u => u.where === 'env'), 'excludes env entries') + const envUnknowns = c.getUnknownConfigs('env') + t.ok(envUnknowns.some(u => !u.baseKey), 'plain env unknown collected') + t.ok(envUnknowns.some(u => u.baseKey), 'scoped env unknown collected') + }) }) t.test('workspaces', async (t) => { @@ -1199,6 +1451,7 @@ t.test('workspaces', async (t) => { cwd: join(`${path}/workspaces/one`), shorthands, definitions, + nerfDarts, }) await config.load() @@ -1222,6 +1475,7 @@ t.test('workspaces', async (t) => { cwd: join(`${path}/workspaces/one`), shorthands, definitions, + nerfDarts, }) await config.load() @@ -1478,6 +1732,7 @@ t.test('exclusive env option is skipped when sibling is set via CLI', async t => const path = t.testdir() const config = new Config({ env: { + HOME: path, npm_config_truth: 'true', }, npmPath: __dirname, @@ -1487,6 +1742,7 @@ t.test('exclusive env option is skipped when sibling is set via CLI', async t => '--lie=true', ], cwd: join(`${path}/project`), + excludeNpmCwd: true, shorthands, definitions: { ...definitions, @@ -1503,6 +1759,7 @@ t.test('exclusive env option is skipped when sibling is set via CLI', async t => exclusive: ['truth'], }), }, + nerfDarts, flatten, }) // should not throw — env `truth` is skipped because `lie` was set via CLI @@ -1519,8 +1776,12 @@ t.test('env-replaced config from files is not clobbered when saving', async (t) env: { TEST: 'test value' }, definitions: { registry: { default: 'https://registry.npmjs.org/' }, + test: { default: '' }, + other: { default: '' }, }, - npmPath: process.cwd(), + cwd: path, + excludeNpmCwd: true, + npmPath: path, } const c = new Config(opts) await c.load() @@ -1548,6 +1809,7 @@ t.test('umask', async t => { cwd: join(`${path}/project`), shorthands, definitions, + nerfDarts, flatten, }) await config.load() @@ -1582,6 +1844,7 @@ t.test('catch project config prefix error', async t => { cwd: join(`${path}/project`), shorthands, definitions, + nerfDarts, }) const logs = [] const logHandler = (...args) => logs.push(args) @@ -1596,12 +1859,8 @@ t.test('catch project config prefix error', async t => { ]], 'Expected error logged') }) -t.test('invalid single hyphen warnings', async t => { +t.test('invalid single hyphen errors', async t => { const path = t.testdir() - const logs = [] - const logHandler = (...args) => logs.push(args) - process.on('log', logHandler) - t.teardown(() => process.off('log', logHandler)) const config = new Config({ npmPath: `${path}/npm`, env: {}, @@ -1611,15 +1870,13 @@ t.test('invalid single hyphen warnings', async t => { definitions, nerfDarts, }) - await config.load() - const filtered = logs.filter(l => l[0] === 'warn') - t.match(filtered, [ - ['warn', '-iwr is not a valid single-hyphen cli flag and will be removed in the future'], - ['warn', '-ws is not a valid single-hyphen cli flag and will be removed in the future'], - ], 'Warns about single hyphen configs') + await t.rejects(config.load(), { + code: 'EUNKNOWNCONFIG', + message: /single-hyphen/, + }, 'Throws on invalid single-hyphen flag') }) -t.test('positional arg warnings', async t => { +t.test('positional arg is collected as unknown cli config', async t => { const path = t.testdir() const logs = [] const logHandler = (...args) => logs.push(args) @@ -1634,20 +1891,20 @@ t.test('positional arg warnings', async t => { definitions, nerfDarts, }) + // Unknown CLI flags are collected silently during load(); base-cmd throws + // later once command-scoped definitions are known. await config.load() const filtered = logs.filter(l => l[0] === 'warn') t.match(filtered, [ ['warn', '"extra" is being parsed as a normal command line argument.'], - ['warn', 'Unknown cli config "--something". This will stop working in the next major version of npm.'], - ], 'Warns about positional cli arg') + ], 'Still warns about positional cli arg being parsed as positional') + const unknowns = config.getUnknownConfigs('cli') + t.ok(unknowns.some(u => u.key === 'something'), + 'unknown cli flag collected for later validation') }) -t.test('abbreviation expansion warnings', async t => { +t.test('abbreviation expansion errors', async t => { const path = t.testdir() - const logs = [] - const logHandler = (...args) => logs.push(args) - process.on('log', logHandler) - t.teardown(() => process.off('log', logHandler)) const config = new Config({ npmPath: `${path}/npm`, env: {}, @@ -1657,11 +1914,10 @@ t.test('abbreviation expansion warnings', async t => { definitions, nerfDarts, }) - await config.load() - const filtered = logs.filter(l => l[0] === 'warn') - t.match(filtered, [ - ['warn', 'Expanding --bef to --before. This will stop working in the next major version of npm'], - ], 'Warns about expanded abbreviations') + await t.rejects(config.load(), { + code: 'EUNKNOWNCONFIG', + message: /--bef.*--before/, + }, 'Throws on abbreviation expansion') }) t.test('warning suppression and logging', async t => { @@ -1706,7 +1962,7 @@ t.test('warning suppression and logging', async t => { t.equal(finalWarnings.length, warningCount, 'no duplicate warnings after second logWarnings()') }) -t.test('warn false with invalid flag and warning removal', async t => { +t.test('warn false with unknown env flag and warning removal', async t => { const path = t.testdir() const logs = [] const logHandler = (...args) => logs.push(args) @@ -1715,8 +1971,8 @@ t.test('warn false with invalid flag and warning removal', async t => { const config = new Config({ npmPath: `${path}/npm`, - env: {}, - argv: [process.execPath, __filename, '--invalid-flag', 'value'], + env: { npm_config_invalid_flag: 'value' }, + argv: [process.execPath, __filename], cwd: path, shorthands, definitions, @@ -1726,18 +1982,17 @@ t.test('warn false with invalid flag and warning removal', async t => { config.warn = false await config.load() - // First logWarnings call - should log the queued warning + // First logWarnings call - should log the queued env-unknown warning const logsBeforeFirst = logs.filter(l => l[0] === 'warn').length config.logWarnings() const logsAfterFirst = logs.filter(l => l[0] === 'warn') - // Check we have warnings and the invalid-flag warning is there t.ok(logsAfterFirst.length > logsBeforeFirst, 'warnings were logged') const invalidFlagWarnings = logsAfterFirst.filter(w => w[1] && w[1].includes('invalid-flag')) t.ok(invalidFlagWarnings.length > 0, 'invalid-flag warning present') // Trigger the same warning again - config.checkUnknown('cli', 'invalid-flag') + config.checkUnknown('env', 'invalid-flag') // Remove the warning config.removeWarning('invalid-flag') @@ -1861,6 +2116,7 @@ t.test('before and min-release-age', async t => { argv: [process.execPath, __filename, '--min-release-age', '30'], cwd: path, definitions, + nerfDarts, shorthands, flatten, }) From 2a03860fcafe92b22770fc554b25994b29bacbdb Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 21 Apr 2026 14:26:17 -0700 Subject: [PATCH 4/4] fix!: run root preinstall before reify BREAKING CHANGE: root \`preinstall\` now runs before dependencies are installed. --- docs/lib/content/using-npm/scripts.md | 15 ++-- lib/commands/ci.js | 29 ++++--- lib/commands/install.js | 30 ++++--- test/lib/commands/ci.js | 109 +++++++++++++++++++++++++ test/lib/commands/install.js | 112 ++++++++++++++++++++++++++ 5 files changed, 267 insertions(+), 28 deletions(-) diff --git a/docs/lib/content/using-npm/scripts.md b/docs/lib/content/using-npm/scripts.md index b01c0bc7ffd77..4db79686c2e19 100644 --- a/docs/lib/content/using-npm/scripts.md +++ b/docs/lib/content/using-npm/scripts.md @@ -111,7 +111,7 @@ It is run AFTER the changes have been applied and the `package.json` and `packag #### [`npm ci`](/commands/npm-ci) -* `preinstall` +* `preinstall` (before dependencies are installed) * `install` * `postinstall` * `prepublish` @@ -119,8 +119,9 @@ It is run AFTER the changes have been applied and the `package.json` and `packag * `prepare` * `postprepare` -These all run after the actual installation of modules into - `node_modules`, in order, with no internal actions happening in between +`preinstall` runs before any dependencies are fetched or unpacked into `node_modules`, so scripts can prepare the environment (for example, setting up authentication for a private registry) before tarballs are fetched. For `npm ci`, `preinstall` fires *after* the lockfile has been validated against `package.json`, so it cannot influence dependency resolution — that remains locked to `package-lock.json`. The remaining scripts run after the installation of modules into `node_modules`, in order, with no internal actions happening in between. + +Because `preinstall` runs before reify, scripts cannot rely on packages from `node_modules`. `npm ci` wipes `node_modules` before `preinstall` fires, so `require()` of a dependency will always fail. Use `install` or `postinstall` for setup that depends on installed packages. #### [`npm diff`](/commands/npm-diff) @@ -128,9 +129,9 @@ These all run after the actual installation of modules into #### [`npm install`](/commands/npm-install) -These also run when you run `npm install -g ` +These run on a bare `npm install` in a local project (no package arguments). -* `preinstall` +* `preinstall` (before dependencies are installed) * `install` * `postinstall` * `prepublish` @@ -138,6 +139,10 @@ These also run when you run `npm install -g ` * `prepare` * `postprepare` +`preinstall` runs before any dependencies are fetched or unpacked into `node_modules`, so scripts can prepare the environment (for example, setting up authentication for a private registry) before resolution begins. The remaining scripts run after installation has completed. + +Because `preinstall` runs before reify, scripts cannot rely on packages from `node_modules`. On a fresh checkout, `require()` of a dependency will fail. On a repeat `npm install` against an existing `node_modules/`, it may incidentally succeed because the previously-installed tree is still on disk, but the version available is whatever was previously installed and may be removed or replaced by the upcoming install. Use `install` or `postinstall` for setup that depends on installed packages. + 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` These are run from the scripts of `` diff --git a/lib/commands/ci.js b/lib/commands/ci.js index 05514b441068e..87dccf908f174 100644 --- a/lib/commands/ci.js +++ b/lib/commands/ci.js @@ -99,12 +99,24 @@ class CI extends ArboristWorkspaceCmd { }) } + // Root lifecycle scripts for `npm ci` mirror those run by `npm install`. `preinstall` runs *before* reify so that scripts can bootstrap the environment (e.g. private-registry auth) before any dependency is fetched or unpacked. The remaining scripts run after reify as they did before. + const scriptShell = this.npm.config.get('script-shell') || undefined + const runRootScript = (event) => runScript({ + path: where, + args: [], + scriptShell, + stdio: 'inherit', + event, + }) + + if (!ignoreScripts) { + await runRootScript('preinstall') + } + await arb.reify(opts) - // run the same set of scripts that `npm install` runs. if (!ignoreScripts) { - const scripts = [ - 'preinstall', + const postReifyScripts = [ 'install', 'postinstall', 'prepublish', // XXX should we remove this finally?? @@ -112,15 +124,8 @@ class CI extends ArboristWorkspaceCmd { 'prepare', 'postprepare', ] - const scriptShell = this.npm.config.get('script-shell') || undefined - for (const event of scripts) { - await runScript({ - path: where, - args: [], - scriptShell, - stdio: 'inherit', - event, - }) + for (const event of postReifyScripts) { + await runRootScript(event) } } await reifyFinish(this.npm, arb) diff --git a/lib/commands/install.js b/lib/commands/install.js index 287b585f13231..c4463e3888094 100644 --- a/lib/commands/install.js +++ b/lib/commands/install.js @@ -145,12 +145,26 @@ class Install extends ArboristWorkspaceCmd { add: args, workspaces: this.workspaceNames, } + + // Root lifecycle scripts only run for a bare `npm install` in a local project. `preinstall` runs *before* Arborist touches the filesystem so that scripts can bootstrap the environment (e.g. set up private-registry auth, generate files consumed during resolution) before dependencies are fetched or unpacked. The remaining scripts run after reify as they did before. + const runRootLifecycle = !args.length && !isGlobalInstall && !ignoreScripts + const runRootScript = (event) => runScript({ + path: where, + args: [], + scriptShell, + stdio: 'inherit', + event, + }) + + if (runRootLifecycle) { + await runRootScript('preinstall') + } + const arb = new Arborist(opts) await arb.reify(opts) - if (!args.length && !isGlobalInstall && !ignoreScripts) { - const scripts = [ - 'preinstall', + if (runRootLifecycle) { + const postReifyScripts = [ 'install', 'postinstall', 'prepublish', // XXX(npm9) should we remove this finally?? @@ -158,14 +172,8 @@ class Install extends ArboristWorkspaceCmd { 'prepare', 'postprepare', ] - for (const event of scripts) { - await runScript({ - path: where, - args: [], - scriptShell, - stdio: 'inherit', - event, - }) + for (const event of postReifyScripts) { + await runRootScript(event) } } await reifyFinish(this.npm, arb) diff --git a/test/lib/commands/ci.js b/test/lib/commands/ci.js index 733b46a828133..e5639c3e04f8d 100644 --- a/test/lib/commands/ci.js +++ b/test/lib/commands/ci.js @@ -182,6 +182,115 @@ t.test('lifecycle scripts', async t => { ], 'runs appropriate scripts, in order') }) +// Regression test: `npm ci` must run root `preinstall` before reify populates node_modules, matching `npm install` behavior. +t.test('preinstall runs before reify for npm ci', async t => { + const events = [] + const { npm, registry } = await loadMockNpm(t, { + prefixDir: { + abbrev: abbrev, + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + preinstall: 'echo preinstall', + postinstall: 'echo postinstall', + }, + }), + 'package-lock.json': JSON.stringify(packageLock), + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix) { + const abbrevPkg = path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json') + events.push({ event: opts.event, depInstalled: fs.existsSync(abbrevPkg) }) + } + }, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {}) + await npm.exec('ci', []) + + const pre = events.find(e => e.event === 'preinstall') + const post = events.find(e => e.event === 'postinstall') + t.ok(pre, 'preinstall ran') + t.ok(post, 'postinstall ran') + t.equal(pre.depInstalled, false, 'preinstall runs before dependencies are installed') + t.equal(post.depInstalled, true, 'postinstall runs after dependencies are installed') +}) + +// Regression test: --ignore-scripts must suppress the new pre-reify `preinstall` path in `npm ci`, matching the symmetric guarantee in `npm install`. +t.test('--ignore-scripts skips preinstall entirely for npm ci', async t => { + const events = [] + const { npm, registry } = await loadMockNpm(t, { + config: { 'ignore-scripts': true, audit: false }, + prefixDir: { + abbrev: abbrev, + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + preinstall: 'echo preinstall', + postinstall: 'echo postinstall', + }, + }), + 'package-lock.json': JSON.stringify(packageLock), + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix) { + events.push(opts.event) + } + }, + }, + }) + 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.strictSame(events, [], 'no root lifecycle scripts run when --ignore-scripts is set') +}) + +// Regression test: symmetric to the install-side guarantee — a failing root `preinstall` must short-circuit before reify runs in `npm ci`, so dependencies never reach disk on failure. +t.test('a failing preinstall prevents reify for npm ci', async t => { + const events = [] + const { npm } = await loadMockNpm(t, { + prefixDir: { + abbrev: abbrev, + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + preinstall: 'exit 1', + postinstall: 'echo postinstall', + }, + }), + 'package-lock.json': JSON.stringify(packageLock), + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix) { + events.push(opts.event) + if (opts.event === 'preinstall') { + throw Object.assign(new Error('preinstall failed'), { code: 'ELIFECYCLE' }) + } + } + }, + }, + }) + + await t.rejects(npm.exec('ci', []), /preinstall failed/, 'ci rejects when preinstall fails') + t.strictSame(events, ['preinstall'], 'only preinstall ran; no post-reify scripts') + t.equal( + fs.existsSync(path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')), + false, + 'no dependency reached disk after preinstall failure' + ) +}) + t.test('should throw if package-lock.json is missing', async t => { const { npm } = await loadMockNpm(t, { prefixDir: { diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 2aa56a3dcd175..186e6f086cebf 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -101,6 +101,118 @@ t.test('exec commands', async t => { t.strictSame(lifecycleScripts, runOrder, 'all script ran in the correct order') }) + // Regression test: root `preinstall` must run before any dependency is fetched/unpacked, while `install` and `postinstall` run after reify has populated node_modules. + await t.test('preinstall runs before reify, post-reify scripts run after', async t => { + const events = [] + const { npm, registry } = await loadMockNpm(t, { + config: { audit: false }, + prefixDir: { + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + preinstall: 'echo preinstall', + install: 'echo install', + postinstall: 'echo postinstall', + }, + }), + abbrev, + }, + mocks: { + '@npmcli/run-script': async (opts) => { + // Only record scripts targeted at the project root, not any that arborist may run for dependencies during reify. + if (opts.path === npm.prefix) { + const abbrevPkg = path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json') + events.push({ event: opts.event, depInstalled: fs.existsSync(abbrevPkg) }) + } + }, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.package({ manifest }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + + await npm.exec('install') + + const pre = events.find(e => e.event === 'preinstall') + const post = events.find(e => e.event === 'postinstall') + t.ok(pre, 'preinstall ran') + t.ok(post, 'postinstall ran') + t.equal(pre.depInstalled, false, 'preinstall runs before dependencies are installed') + t.equal(post.depInstalled, true, 'postinstall runs after dependencies are installed') + }) + + await t.test('without args, --ignore-scripts skips preinstall entirely', async t => { + const events = [] + const { npm, registry } = await loadMockNpm(t, { + config: { audit: false, 'ignore-scripts': true }, + prefixDir: { + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + preinstall: 'echo preinstall', + postinstall: 'echo postinstall', + }, + }), + abbrev, + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix) { + events.push(opts.event) + } + }, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.package({ manifest }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + + await npm.exec('install') + t.strictSame(events, [], 'no root lifecycle scripts run when --ignore-scripts is set') + }) + + // Regression test: a failing root `preinstall` must short-circuit before reify runs, so dependencies never reach disk on failure. This is the cleaner failure mode the PR was motivated by; future refactors that swallow the rejection and still call reify must fail here. + await t.test('a failing preinstall prevents reify', async t => { + const events = [] + const { npm } = await loadMockNpm(t, { + config: { audit: false }, + prefixDir: { + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + preinstall: 'exit 1', + postinstall: 'echo postinstall', + }, + }), + abbrev, + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix) { + events.push(opts.event) + if (opts.event === 'preinstall') { + throw Object.assign(new Error('preinstall failed'), { code: 'ELIFECYCLE' }) + } + } + }, + }, + }) + + await t.rejects(npm.exec('install'), /preinstall failed/, 'install rejects when preinstall fails') + t.strictSame(events, ['preinstall'], 'only preinstall ran; no post-reify scripts') + t.equal( + fs.existsSync(path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')), + false, + 'no dependency reached disk after preinstall failure' + ) + }) + await t.test('should ignore scripts with --ignore-scripts', async t => { const { npm, registry } = await loadMockNpm(t, { config: {