From 6f229f78d9911b4734f0a19c6afdc5454034c759 Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Mon, 9 Dec 2019 15:38:47 -0800
Subject: [PATCH] fix: sanitize and normalize package bin field

---
 lib/fetchers/directory.js |  3 ++-
 lib/finalize-manifest.js  | 14 +++-----------
 package-lock.json         |  5 +++++
 package.json              |  1 +
 test/finalize-manifest.js | 25 +------------------------
 5 files changed, 12 insertions(+), 36 deletions(-)

diff --git a/lib/fetchers/directory.js b/lib/fetchers/directory.js
index 3d4ec24c..fc9c46cd 100644
--- a/lib/fetchers/directory.js
+++ b/lib/fetchers/directory.js
@@ -9,6 +9,7 @@ const readJson = require('../util/read-json')
 const path = require('path')
 const pipe = BB.promisify(require('mississippi').pipe)
 const through = require('mississippi').through
+const normalizePackageBin = require('npm-normalize-package-bin')
 
 const readFileAsync = BB.promisify(require('fs').readFile)
 
@@ -63,7 +64,7 @@ Fetcher.impl(fetchDirectory, {
       } else {
         return pkg
       }
-    })
+    }).then(pkg => normalizePackageBin(pkg))
   },
 
   // As of npm@5, the npm installer doesn't pack + install directories: it just
diff --git a/lib/finalize-manifest.js b/lib/finalize-manifest.js
index d1d0f4e5..80b9cda7 100644
--- a/lib/finalize-manifest.js
+++ b/lib/finalize-manifest.js
@@ -14,6 +14,7 @@ const pipe = BB.promisify(require('mississippi').pipe)
 const ssri = require('ssri')
 const tar = require('tar')
 const readJson = require('./util/read-json')
+const normalizePackageBin = require('npm-normalize-package-bin')
 
 // `finalizeManifest` takes as input the various kinds of manifests that
 // manifest handlers ('lib/fetchers/*.js#manifest()') return, and makes sure
@@ -105,17 +106,8 @@ function Manifest (pkg, fromTarball, fullMetadata) {
   this._shrinkwrap = pkg._shrinkwrap || fromTarball._shrinkwrap || null
   this.bin = pkg.bin || fromTarball.bin || null
 
-  if (this.bin && Array.isArray(this.bin)) {
-    // Code yanked from read-package-json.
-    const m = (pkg.directories && pkg.directories.bin) || '.'
-    this.bin = this.bin.reduce((acc, mf) => {
-      if (mf && mf.charAt(0) !== '.') {
-        const f = path.basename(mf)
-        acc[f] = path.join(m, mf)
-      }
-      return acc
-    }, {})
-  }
+  // turn arrays and strings into a legit object, strip out bad stuff
+  normalizePackageBin(this)
 
   this._id = null
 
diff --git a/package-lock.json b/package-lock.json
index 6e362b20..15b07c0c 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -3618,6 +3618,11 @@
       "resolved": "https://registry.npmjs.org/npm-bundled/-/npm-bundled-1.0.5.tgz",
       "integrity": "sha512-m/e6jgWu8/v5niCUKQi9qQl8QdeEduFA96xHDDzFGqly0OOjI7c+60KM/2sppfnUU9JJagf+zs+yGhqSOFj71g=="
     },
+    "npm-normalize-package-bin": {
+      "version": "1.0.0",
+      "resolved": "https://registry.npmjs.org/npm-normalize-package-bin/-/npm-normalize-package-bin-1.0.0.tgz",
+      "integrity": "sha512-q5spmGV/CMb/glA+ziZT3ZNbZKnua8SRQLWxQK5cpQik+l4YxgElAw72z6ZrwMrnJtzoEs259Vl3neEfxKQmDw=="
+    },
     "npm-package-arg": {
       "version": "6.1.0",
       "resolved": "https://registry.npmjs.org/npm-package-arg/-/npm-package-arg-6.1.0.tgz",
diff --git a/package.json b/package.json
index 95e4e120..863773db 100644
--- a/package.json
+++ b/package.json
@@ -58,6 +58,7 @@
     "mississippi": "^3.0.0",
     "mkdirp": "^0.5.1",
     "normalize-package-data": "^2.4.0",
+    "npm-normalize-package-bin": "^1.0.0",
     "npm-package-arg": "^6.1.0",
     "npm-packlist": "^1.1.12",
     "npm-pick-manifest": "^3.0.0",
diff --git a/test/finalize-manifest.js b/test/finalize-manifest.js
index 11f1f293..f2e26efe 100644
--- a/test/finalize-manifest.js
+++ b/test/finalize-manifest.js
@@ -62,7 +62,7 @@ test('returns a manifest with the right fields', t => {
       peerDependencies: {},
       peerDependenciesMeta: {},
       bin: {
-        testing: './foo.js'
+        testing: 'foo.js'
       },
       _shasum: 'deadbeef1',
       _resolved: 'resolved.to.this',
@@ -96,7 +96,6 @@ test('defaults all field to expected types + values', t => {
       bundleDependencies: false, // because npm does boolean checks on this
       peerDependencies: {},
       peerDependenciesMeta: {},
-      bin: null,
       _resolved: base._resolved,
       _integrity: base._integrity,
       _shasum: base._shasum,
@@ -212,28 +211,6 @@ test('fills in `bin` if `directories.bin` string', t => {
   })
 })
 
-test('fills in `bin` if original was an array', t => {
-  const tarballPath = 'testing/tarball-1.2.3.tgz'
-  const base = {
-    name: 'testing',
-    version: '1.2.3',
-    bin: ['my/bin1', 'bin2.js'],
-    directories: {
-      bin: 'foo'
-    },
-    _integrity: 'sha1-deadbeefc0ffeebad1dea',
-    _shasum: '75e69d6de79f7347df79e6da77575e',
-    _resolved: OPTS.registry + tarballPath,
-    _hasShrinkwrap: false
-  }
-  return finalizeManifest(base, npa(base.name), OPTS).then(manifest => {
-    t.deepEqual(manifest.bin, {
-      'bin1': path.join('foo', 'my', 'bin1'),
-      'bin2.js': path.join('foo', 'bin2.js')
-    }, 'bins successfully calculated')
-  })
-})
-
 test('uses package.json as base if passed null', t => {
   const tarballPath = 'testing/tarball-1.2.3.tgz'
   const base = {