From 42e0587a9ea6940a5d5be5903370ad1113feef21 Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 21 Apr 2021 09:14:08 -0700 Subject: [PATCH] fix(pack): refuse to pack invalid packument If name and version are missing, the filename won't make sense. This also slightly reorders operations so that it will bail early on multiple packages, not potentially packing some before hitting an invalid package and bailing. PR-URL: https://github.com/npm/cli/pull/3115 Credit: @wraithgar Close: #3115 Reviewed-by: @nlf --- lib/pack.js | 22 ++++++++++---- test/lib/pack.js | 76 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 68 insertions(+), 30 deletions(-) diff --git a/lib/pack.js b/lib/pack.js index 8e61efabb36e4..5c0da6be7b6e0 100644 --- a/lib/pack.js +++ b/lib/pack.js @@ -45,22 +45,34 @@ class Pack extends BaseCommand { args = ['.'] const unicode = this.npm.config.get('unicode') + const dryRun = this.npm.config.get('dry-run') - // clone the opts because pacote mutates it with resolved/integrity - const tarballs = await Promise.all(args.map(async (arg) => { + // Get the manifests and filenames first so we can bail early on manifest + // errors before making any tarballs + const manifests = [] + for (const arg of args) { const spec = npa(arg) - const dryRun = this.npm.config.get('dry-run') const manifest = await pacote.manifest(spec, this.npm.flatOptions) + if (!manifest._id) + throw new Error('Invalid package, must have name and version') + const filename = `${manifest.name}-${manifest.version}.tgz` .replace(/^@/, '').replace(/\//, '-') + manifests.push({ arg, filename, manifest }) + } + + // Load tarball names up for printing afterward to isolate from the + // noise generated during packing + const tarballs = [] + for (const { arg, filename, manifest } of manifests) { const tarballData = await libpack(arg, this.npm.flatOptions) const pkgContents = await getContents(manifest, tarballData) if (!dryRun) await writeFile(filename, tarballData) - return pkgContents - })) + tarballs.push(pkgContents) + } for (const tar of tarballs) { logTar(tar, { log, unicode }) diff --git a/test/lib/pack.js b/test/lib/pack.js index 64f4d1258b9ce..6706042b4c82e 100644 --- a/test/lib/pack.js +++ b/test/lib/pack.js @@ -15,10 +15,12 @@ const mockPacote = { manifest: (spec) => { if (spec.type === 'directory') return pacote.manifest(spec) - return { + const m = { name: spec.name || 'test-package', version: spec.version || '1.0.0-test', } + m._id = `${m.name}@${m.version}` + return m }, } @@ -43,9 +45,8 @@ t.test('should pack current directory with no arguments', (t) => { }) const pack = new Pack(npm) - pack.exec([], er => { - if (er) - throw er + pack.exec([], err => { + t.error(err, { bail: true }) const filename = `npm-${require('../../package.json').version}.tgz` t.strictSame(OUTPUT, [[filename]]) @@ -79,9 +80,8 @@ t.test('should pack given directory', (t) => { }) const pack = new Pack(npm) - pack.exec([testDir], er => { - if (er) - throw er + pack.exec([testDir], err => { + t.error(err, { bail: true }) const filename = 'my-cool-pkg-1.0.0.tgz' t.strictSame(OUTPUT, [[filename]]) @@ -115,9 +115,8 @@ t.test('should pack given directory for scoped package', (t) => { }) const pack = new Pack(npm) - return pack.exec([testDir], er => { - if (er) - throw er + return pack.exec([testDir], err => { + t.error(err, { bail: true }) const filename = 'cool-my-pkg-1.0.0.tgz' t.strictSame(OUTPUT, [[filename]]) @@ -150,9 +149,8 @@ t.test('should log pack contents', (t) => { }) const pack = new Pack(npm) - pack.exec([], er => { - if (er) - throw er + pack.exec([], err => { + t.error(err, { bail: true }) const filename = `npm-${require('../../package.json').version}.tgz` t.strictSame(OUTPUT, [[filename]]) @@ -160,6 +158,38 @@ t.test('should log pack contents', (t) => { }) }) +t.test('invalid packument', (t) => { + const mockPacote = { + manifest: () => { + return {} + }, + } + const Pack = t.mock('../../lib/pack.js', { + libnpmpack, + pacote: mockPacote, + npmlog: { + notice: () => {}, + showProgress: () => {}, + clearProgress: () => {}, + }, + }) + const npm = mockNpm({ + config: { + unicode: true, + json: true, + 'dry-run': true, + }, + output, + }) + const pack = new Pack(npm) + pack.exec([], err => { + t.match(err, { message: 'Invalid package, must have name and version' }) + + t.strictSame(OUTPUT, []) + t.end() + }) +}) + t.test('workspaces', (t) => { const testDir = t.testdir({ 'package.json': JSON.stringify({ @@ -201,9 +231,8 @@ t.test('workspaces', (t) => { const pack = new Pack(npm) t.test('all workspaces', (t) => { - pack.execWorkspaces([], [], er => { - if (er) - throw er + pack.execWorkspaces([], [], err => { + t.error(err, { bail: true }) t.strictSame(OUTPUT, [ ['workspace-a-1.0.0.tgz'], @@ -214,9 +243,8 @@ t.test('workspaces', (t) => { }) t.test('all workspaces, `.` first arg', (t) => { - pack.execWorkspaces(['.'], [], er => { - if (er) - throw er + pack.execWorkspaces(['.'], [], err => { + t.error(err, { bail: true }) t.strictSame(OUTPUT, [ ['workspace-a-1.0.0.tgz'], @@ -227,9 +255,8 @@ t.test('workspaces', (t) => { }) t.test('one workspace', (t) => { - pack.execWorkspaces([], ['workspace-a'], er => { - if (er) - throw er + pack.execWorkspaces([], ['workspace-a'], err => { + t.error(err, { bail: true }) t.strictSame(OUTPUT, [ ['workspace-a-1.0.0.tgz'], @@ -239,9 +266,8 @@ t.test('workspaces', (t) => { }) t.test('specific package', (t) => { - pack.execWorkspaces(['abbrev'], [], er => { - if (er) - throw er + pack.execWorkspaces(['abbrev'], [], err => { + t.error(err, { bail: true }) t.strictSame(OUTPUT, [ ['abbrev-1.0.0-test.tgz'],