diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index f32d5764d5edb..41e35bd78ac46 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -80,6 +80,7 @@ const errorMessage = (er, npm) => { } case 'EALLOWGIT': + case 'EALLOWREMOTE': summary.push(['', er.message]) detail.push(['', `Refusing to fetch "${er.package}"`]) break diff --git a/tap-snapshots/test/lib/commands/config.js.test.cjs b/tap-snapshots/test/lib/commands/config.js.test.cjs index 57956a4f5171e..26420c296f493 100644 --- a/tap-snapshots/test/lib/commands/config.js.test.cjs +++ b/tap-snapshots/test/lib/commands/config.js.test.cjs @@ -18,8 +18,8 @@ exports[`test/lib/commands/config.js TAP config list --json > output matches sna "allow-same-version": false, "allow-directory": "all", "allow-file": "all", - "allow-git": "all", - "allow-remote": "all", + "allow-git": "none", + "allow-remote": "none", "allow-scripts": [ "" ], @@ -201,8 +201,8 @@ access = null all = false allow-directory = "all" allow-file = "all" -allow-git = "all" -allow-remote = "all" +allow-git = "none" +allow-remote = "none" allow-same-version = false allow-scripts = [""] allow-scripts-pending = false diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index f6dfe04f369d8..8d24e738ddf69 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -255,7 +255,7 @@ dependencies to be used for other commands like \`npm view\` #### \`allow-git\` -* Default: "all" +* Default: "none" * Type: "all", "none", or "root" Limits the ability for npm to fetch dependencies from git references. That @@ -264,6 +264,11 @@ range. Please note that this could leave your tree incomplete and some packages may not function as intended or designed. Changing this setting will not remove dependencies that are already installed. +As of npm 12 the default is \`none\`. Git dependencies run \`git\` against a +remote repo and may install configuration the project does not control. Opt +in explicitly per project (in \`.npmrc\`) or per command (on the CLI) when you +need git deps. + \`all\` allows any git dependencies to be fetched and installed. \`none\` prevents any git dependencies from being fetched and installed. \`root\` only allows git dependencies defined in your project's package.json to be fetched @@ -274,7 +279,7 @@ like \`npm view\` #### \`allow-remote\` -* Default: "all" +* Default: "none" * Type: "all", "none", or "root" Limits the ability for npm to fetch dependencies from urls. That is, @@ -283,6 +288,13 @@ range. Please note that this could leave your tree incomplete and some packages may not function as intended or designed. Changing this setting will not remove dependencies that are already installed. +As of npm 12 the default is \`none\`. Tarballs that share a hostname with the +configured registry (the typical case for the npm registry, GitHub Packages, +and most private registries) are still installed normally. If your registry +serves tarballs from a different host, set \`replace-registry-host\` or +override this setting. Opt in explicitly per project (in \`.npmrc\`) or per +command (on the CLI) when you intentionally install from a URL. + \`all\` allows any url to be installed. \`none\` prevents any url from being installed. \`root\` only allows urls defined in your project's package.json to be installed. Also allows url dependencies to be used for other commands @@ -2778,8 +2790,8 @@ Object { "all": false, "allowDirectory": "all", "allowFile": "all", - "allowGit": "all", - "allowRemote": "all", + "allowGit": "none", + "allowRemote": "none", "allowSameVersion": false, "allowScripts": Array [], "allowScriptsPending": false, diff --git a/test/lib/commands/exec.js b/test/lib/commands/exec.js index 92ea993e3edfb..30d8c2c046c37 100644 --- a/test/lib/commands/exec.js +++ b/test/lib/commands/exec.js @@ -268,6 +268,7 @@ t.test('packs from git spec', async t => { config: { audit: false, yes: true, + 'allow-git': 'all', }, }) try { diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 186e6f086cebf..3961b7f02e0c1 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -367,6 +367,20 @@ t.test('exec commands', async t => { ) }) + t.test('allow-git default rejects git deps', async t => { + const { npm } = await loadMockNpm(t, { + config: { audit: false }, + }) + await t.rejects( + npm.exec('install', ['npm/npm']), + { + code: 'EALLOWGIT', + package: 'github:npm/npm', + }, + 'no explicit allow-git config still blocks git installs' + ) + }) + t.test('allow-git=root refuses non-root git dependency', async t => { const { npm } = await loadMockNpm(t, { config: { @@ -507,6 +521,24 @@ t.test('exec commands', async t => { 'user-supplied remote URL is still blocked' ) }) + + t.test('allow-remote default rejects a user-supplied remote URL', async t => { + const { npm } = await loadMockNpm(t, { + config: { audit: false }, + prefixDir: { + 'package.json': JSON.stringify({ + name: '@npmcli/test-package', + version: '1.0.0', + dependencies: { abbrev: 'https://registry.npmjs.org/abbrev/-/abbrev-2.0.0.tgz' }, + }), + }, + }) + await t.rejects( + npm.exec('install', []), + { code: 'EALLOWREMOTE' }, + 'no explicit allow-remote config still blocks user-supplied tarball URLs' + ) + }) }) t.test('completion', async t => { diff --git a/test/lib/commands/view.js b/test/lib/commands/view.js index 84f1edcd96d10..dbf4560bf46ab 100644 --- a/test/lib/commands/view.js +++ b/test/lib/commands/view.js @@ -394,7 +394,9 @@ const loadMockNpm = async function (t, opts = {}) { } t.test('package from git', async t => { - const { view, joinedOutput } = await loadMockNpm(t, { config: { unicode: false } }) + const { view, joinedOutput } = await loadMockNpm(t, { + config: { unicode: false, 'allow-git': 'all' }, + }) await view.exec(['https://github.com/npm/green']) t.matchSnapshot(joinedOutput()) }) diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index 1fd88e9351953..8c7e2cfbb6438 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -225,7 +225,7 @@ const definitions = { flatten, }), 'allow-git': new Definition('allow-git', { - default: 'all', + default: 'none', type: ['all', 'none', 'root'], description: ` Limits the ability for npm to fetch dependencies from git references. @@ -233,6 +233,11 @@ const definitions = { Please note that this could leave your tree incomplete and some packages may not function as intended or designed. Changing this setting will not remove dependencies that are already installed. + As of npm 12 the default is \`none\`. Git dependencies run \`git\` + against a remote repo and may install configuration the project does + not control. Opt in explicitly per project (in \`.npmrc\`) or per + command (on the CLI) when you need git deps. + \`all\` allows any git dependencies to be fetched and installed. \`none\` prevents any git dependencies from being fetched and installed. \`root\` only allows git dependencies defined in your project's package.json to be fetched and installed. Also allows git dependencies to be fetched for other commands like \`npm view\` @@ -240,7 +245,7 @@ const definitions = { flatten, }), 'allow-remote': new Definition('allow-remote', { - default: 'all', + default: 'none', type: ['all', 'none', 'root'], description: ` Limits the ability for npm to fetch dependencies from urls. @@ -248,6 +253,14 @@ const definitions = { Please note that this could leave your tree incomplete and some packages may not function as intended or designed. Changing this setting will not remove dependencies that are already installed. + As of npm 12 the default is \`none\`. Tarballs that share a hostname + with the configured registry (the typical case for the npm registry, + GitHub Packages, and most private registries) are still installed + normally. If your registry serves tarballs from a different host, + set \`replace-registry-host\` or override this setting. Opt in + explicitly per project (in \`.npmrc\`) or per command (on the CLI) + when you intentionally install from a URL. + \`all\` allows any url to be installed. \`none\` prevents any url from being installed. \`root\` only allows urls defined in your project's package.json to be installed. Also allows url dependencies to be used for other commands like \`npm view\` diff --git a/workspaces/config/test/definitions/definitions.js b/workspaces/config/test/definitions/definitions.js index ad50d3e0111f2..37f4f033f4e13 100644 --- a/workspaces/config/test/definitions/definitions.js +++ b/workspaces/config/test/definitions/definitions.js @@ -1068,6 +1068,33 @@ t.test('node-gyp', t => { t.end() }) +t.test('allow-git defaults to none and flattens to allowGit', t => { + const defs = mockDefs() + t.equal(defs['allow-git'].default, 'none') + t.strictSame(defs['allow-git'].type, ['all', 'none', 'root']) + const flat = {} + defs['allow-git'].flatten('allow-git', { 'allow-git': 'root' }, flat) + t.strictSame(flat, { allowGit: 'root' }) + t.end() +}) + +t.test('allow-remote defaults to none and flattens to allowRemote', t => { + const defs = mockDefs() + t.equal(defs['allow-remote'].default, 'none') + t.strictSame(defs['allow-remote'].type, ['all', 'none', 'root']) + const flat = {} + defs['allow-remote'].flatten('allow-remote', { 'allow-remote': 'all' }, flat) + t.strictSame(flat, { allowRemote: 'all' }) + t.end() +}) + +t.test('allow-file and allow-directory still default to all', t => { + const defs = mockDefs() + t.equal(defs['allow-file'].default, 'all', 'allow-file unchanged') + t.equal(defs['allow-directory'].default, 'all', 'allow-directory unchanged') + t.end() +}) + t.test('allow-scripts', t => { t.test('defaults to empty string and flattens to []', t => { const defs = mockDefs() diff --git a/workspaces/libnpmdiff/lib/tarball.js b/workspaces/libnpmdiff/lib/tarball.js index e2738b58f11bc..a78b3e18ee8b5 100644 --- a/workspaces/libnpmdiff/lib/tarball.js +++ b/workspaces/libnpmdiff/lib/tarball.js @@ -29,10 +29,21 @@ const tarball = (manifest, opts) => { return nodeModulesTarball(manifest, opts) } - return pacote.tarball(manifest._resolved, { - ...opts, - Arborist, - }) + // pacote re-parses manifest._resolved as type=remote, so allow-remote=none + // would mis-fire on the tarball URL the registry just handed us. mirror + // arborist reify's carve-out: trust resolved tarballs for registry-typed specs. + const tarballOpts = { ...opts, Arborist } + let fromSpec + try { + fromSpec = manifest._from ? npa(manifest._from) : null + } catch { + fromSpec = null + } + if (fromSpec?.registry) { + tarballOpts.allowRemote = 'all' + } + + return pacote.tarball(manifest._resolved, tarballOpts) } module.exports = tarball diff --git a/workspaces/libnpmdiff/test/tarball.js b/workspaces/libnpmdiff/test/tarball.js index e182c159bc541..e5ababd3c2036 100644 --- a/workspaces/libnpmdiff/test/tarball.js +++ b/workspaces/libnpmdiff/test/tarball.js @@ -93,3 +93,51 @@ t.test('pkg not in a node_modules folder', async t => { 'should call regular pacote.tarball method instead' ) }) + +t.test('allowRemote carve-out for registry-mediated tarballs', async t => { + const originalTarball = pacote.tarball + let captured + pacote.tarball = async (resolved, opts) => { + captured = { resolved, opts } + return Buffer.alloc(0) + } + t.teardown(() => { + pacote.tarball = originalTarball + }) + + t.beforeEach(() => { + captured = undefined + }) + + t.test('registry-typed _from sets allowRemote=all', async t => { + await tarball({ + _from: 'abbrev@1.0.0', + _resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.0.0.tgz', + }, {}) + t.equal(captured.opts.allowRemote, 'all') + }) + + t.test('non-registry _from leaves allowRemote untouched', async t => { + await tarball({ + _from: 'https://example.com/foo.tgz', + _resolved: 'https://example.com/foo.tgz', + }, {}) + t.notOk(captured.opts.allowRemote) + }) + + t.test('missing _from leaves allowRemote untouched', async t => { + await tarball({ + _resolved: 'https://example.com/foo.tgz', + }, {}) + t.notOk(captured.opts.allowRemote) + }) + + t.test('unparseable _from leaves allowRemote untouched', async t => { + // npa throws on tags with reserved characters, exercising the catch branch + await tarball({ + _from: '@', + _resolved: 'https://example.com/foo.tgz', + }, {}) + t.notOk(captured.opts.allowRemote) + }) +}) diff --git a/workspaces/libnpmexec/lib/run-script.js b/workspaces/libnpmexec/lib/run-script.js index 13f16a74eb8a0..2de40454f9dcf 100644 --- a/workspaces/libnpmexec/lib/run-script.js +++ b/workspaces/libnpmexec/lib/run-script.js @@ -19,7 +19,9 @@ const run = async ({ // necessary for preventing bash/cmd keywords from overriding if (!isWindowsShell) { if (args.length > 0) { - args[0] = '"' + args[0] + '"' + // single-quote so shell metacharacters in the executable name are taken + // literally; double quotes still expand $(), backticks, $var and " + args[0] = `'${args[0].replace(/'/g, `'\\''`)}'` } } diff --git a/workspaces/libnpmexec/test/run-script.js b/workspaces/libnpmexec/test/run-script.js index 61937098b7e83..5c92ce19397f6 100644 --- a/workspaces/libnpmexec/test/run-script.js +++ b/workspaces/libnpmexec/test/run-script.js @@ -130,6 +130,20 @@ t.test('isWindows', async t => { await runScript() }) +t.test('escapes executable name to neutralize shell metacharacters', async t => { + let pkg + const { runScript } = await mockRunScript(t, { + 'ci-info': { isCI: true }, + '@npmcli/run-script': async (opts) => { + pkg = opts.pkg + }, + '../lib/is-windows.js': false, + }) + + await runScript({ args: [`evil'; touch pwned #`] }) + t.equal(pkg.scripts.npx, `'evil'\\''; touch pwned #'`) +}) + t.test('isNotWindows', async t => { const { runScript } = await mockRunScript(t, { 'ci-info': { isCI: true }, diff --git a/workspaces/libnpmpack/lib/index.js b/workspaces/libnpmpack/lib/index.js index 0c220bb7ceb2e..2ef6391e439d7 100644 --- a/workspaces/libnpmpack/lib/index.js +++ b/workspaces/libnpmpack/lib/index.js @@ -64,16 +64,23 @@ async function pack (spec = 'file:.', opts = {}) { } // packs tarball - const tarball = await pacote.tarball(manifest._resolved, { + const tarballOpts = { ...opts, Arborist, integrity: manifest._integrity, - }) + } + // pacote re-parses manifest._resolved as type=remote, so allow-remote=none + // would mis-fire on the tarball URL the registry just handed us. mirror + // arborist reify's carve-out: trust resolved tarballs for registry-typed specs. + if (spec.registry) { + tarballOpts.allowRemote = 'all' + } + const tarball = await pacote.tarball(manifest._resolved, tarballOpts) // check for explicit `false` so the default behavior is to skip writing to disk if (opts.dryRun === false) { const filename = `${manifest.name}-${manifest.version}.tgz` - .replace(/^@/, '').replace(/\//, '-') + .replace(/^@/, '').replace(/[/\\]/g, '-') const destination = path.resolve(opts.packDestination, filename) await writeFile(destination, tarball) } diff --git a/workspaces/libnpmpack/test/index.js b/workspaces/libnpmpack/test/index.js index 369c295428263..1dc07ac2295a7 100644 --- a/workspaces/libnpmpack/test/index.js +++ b/workspaces/libnpmpack/test/index.js @@ -36,6 +36,29 @@ t.test('packs from local directory', async t => { }) }) +t.test('flattens path separators in name so tarball stays in packDestination', async t => { + const testDir = t.testdir({ + src: { + 'package.json': JSON.stringify({ + name: 'x/../../../../../../escaped', + version: '1.0.0', + }, null, 2), + }, + dest: {}, + }) + + const dest = path.join(testDir, 'dest') + await pack(`file:${path.join(testDir, 'src')}`, { + dryRun: false, + packDestination: dest, + silent: true, + }) + + const written = fs.readdirSync(dest) + t.same(written, ['x-..-..-..-..-..-..-escaped-1.0.0.tgz'], 'separators flattened to a single filename') + t.notOk(fs.existsSync(path.join(testDir, 'escaped-1.0.0.tgz')), 'nothing escaped the destination') +}) + t.test('writes tarball to file when dryRun === false', async t => { const testDir = t.testdir({ 'package.json': JSON.stringify({