From 6e73f6f1989bd0daea888fdb9a0c29c37e71d97b Mon Sep 17 00:00:00 2001
From: Gar <gar+gh@danger.computer>
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.
---
 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..128ee56b9d2c1 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)
 
     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)
 
     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)
 
     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)
 
     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)
 
       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)
 
       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)
 
       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)
 
       t.strictSame(OUTPUT, [
         ['abbrev-1.0.0-test.tgz'],