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: #3115
Credit: @wraithgar
Close: #3115
Reviewed-by: @nlf

Author: wraithgar

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 })

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,16 +149,47 @@ 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]])
     t.end()
   })
 })
 
+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'],