Pack and unpack preserving exec perms on all package bins

This addresses the problem brought up by @boneskull on
isaacs/node-tar#210 (comment)

If a package is created on a Windows system, the file will not have the
executable bit set, as Windows determines executable status by the
filename extension.

Thus, installing with '--no-bin-links' produces a package that can't be
used as expected.

This change does the following:

- While extracting, if the manifest has been loaded, then the mode of
  any bin targets is made executable.
- If the manifest is not loaded, then AFTER extraction, the mode of all
  bin targets is set appropriately.  (Only relevant for the FileFetcher
  class, as the others will all have access to the manifest earlier in
  the process.)
- When packing, all bin targets are given an executable mode in the
  archive.

Thus, newer npm will properly handle archives created without proper
modes, and will always produce proper modes for the benefit of
other/older clients regardless of what fs.stat says.

Strict in what we publish, liberal in what we install.

Author: isaacs

lib/dir.js

@@ -5,9 +5,11 @@ const Minipass = require('minipass')
 const { promisify } = require('util')
 const readPackageJson = promisify(require('read-package-json'))
 const npm = require('./util/npm.js')
+const isPackageBin = require('./util/is-package-bin.js')
 const packlist = require('npm-packlist')
 const tar = require('tar')
 const _prepareDir = Symbol('_prepareDir')
+const _tarcOpts = Symbol('_tarcOpts')
 
 const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
 class DirFetcher extends Fetcher {
@@ -47,20 +49,35 @@ class DirFetcher extends Fetcher {
     // pipe to the stream, and proxy errors the chain.
     this[_prepareDir]()
       .then(() => packlist({ path: this.resolved }))
-      .then(files => tar.c({
-        cwd: this.resolved,
-        prefix: 'package/',
-        portable: true,
-        gzip: true,
-
-        // Provide a specific date in the 1980s for the benefit of zip,
-        // which is confounded by files dated at the Unix epoch 0.
-        mtime: new Date('1985-10-26T08:15:00.000Z'),
-      }, files).on('error', er => stream.emit('error', er)).pipe(stream))
+      .then(files => tar.c(this[_tarcOpts](), files)
+        .on('error', er => stream.emit('error', er)).pipe(stream))
       .catch(er => stream.emit('error', er))
     return stream
   }
 
+  [_tarcOpts] () {
+    return {
+      cwd: this.resolved,
+      prefix: 'package/',
+      portable: true,
+      gzip: true,
+
+      // ensure that package bins are always executable
+      // Note that npm-packlist is already filtering out
+      // anything that is not a regular file, ignored by
+      // .npmignore or package.json "files", etc.
+      filter: (path, stat) => {
+        if (isPackageBin(this.package, path))
+          stat.mode |= 0o111
+        return true
+      },
+
+      // Provide a specific date in the 1980s for the benefit of zip,
+      // which is confounded by files dated at the Unix epoch 0.
+      mtime: new Date('1985-10-26T08:15:00.000Z'),
+    }
+  }
+
   manifest () {
     if (this.package)
       return Promise.resolve(this.package)

lib/fetcher.js

@@ -14,6 +14,7 @@ const retry = require('promise-retry')
 const fsm = require('fs-minipass')
 const cacache = require('cacache')
 const osenv = require('osenv')
+const isPackageBin = require('./util/is-package-bin.js')
 
 // we only change ownership on unix platforms, and only if uid is 0
 const selfOwner = process.getuid && process.getuid() === 0 ? {
@@ -333,25 +334,28 @@ class FetcherBase {
 
   // always ensure that entries are at least as permissive as our configured
   // dmode/fmode, but never more permissive than the umask allows.
-  [_entryMode] (mode, type) {
-    const m = type === 'Directory' ? this.dmode
-      : type === 'File' ? this.fmode
+  [_entryMode] (path, mode, type) {
+    const m = /Directory|GNUDumpDir/.test(type) ? this.dmode
+      : /File$/.test(type) ? this.fmode
       : /* istanbul ignore next - should never happen in a pkg */ 0
-    return (mode | m) & ~this.umask
+
+    // make sure package bins are executable
+    const exe = isPackageBin(this.package, path) ? 0o111 : 0
+    return ((mode | m) & ~this.umask) | exe
   }
 
   [_tarxOptions] ({ cwd, uid, gid }) {
     const sawIgnores = new Set()
     return {
       cwd,
       filter: (name, entry) => {
-        if (/^.*link$/i.test(entry.type))
+        if (/Link$/.test(entry.type))
           return false
-        entry.mode = this[_entryMode](entry.mode, entry.type)
+        entry.mode = this[_entryMode](entry.path, entry.mode, entry.type)
         // this replicates the npm pack behavior where .gitignore files
         // are treated like .npmignore files, but only if a .npmignore
         // file is not present.
-        if (entry.type === 'File') {
+        if (/File$/.test(entry.type)) {
           const base = basename(entry.path)
           if (base === '.npmignore')
             sawIgnores.add(entry.path)

lib/file.js

@@ -4,6 +4,9 @@ const cacache = require('cacache')
 const { promisify } = require('util')
 const readPackageJson = promisify(require('read-package-json'))
 const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
+const _exeBins = Symbol('_exeBins')
+const { resolve } = require('path')
+const fs = require('fs')
 
 class FileFetcher extends Fetcher {
   constructor (spec, opts) {
@@ -31,6 +34,36 @@ class FileFetcher extends Fetcher {
       }))
   }
 
+  [_exeBins] (pkg, dest) {
+    if (!pkg.bin)
+      return Promise.resolve()
+
+    return Promise.all(Object.keys(pkg.bin).map(k => new Promise(res => {
+      const script = resolve(dest, pkg.bin[k])
+      // Best effort.  Ignore errors here, the only result is that
+      // a bin script is not executable.  But if it's missing or
+      // something, we just leave it for a later stage to trip over
+      // when we can provide a more useful contextual error.
+      fs.stat(script, (er, st) => {
+        if (er)
+          return res()
+        const mode = st.mode | 0o111
+        if (mode === st.mode)
+          return res()
+        fs.chmod(script, mode, res)
+      })
+    })))
+  }
+
+  extract (dest) {
+    // if we've already loaded the manifest, then the super got it.
+    // but if not, read the unpacked manifest and chmod properly.
+    return super.extract(dest)
+      .then(result => this.package ? result
+        : readPackageJson(dest + '/package.json').then(pkg =>
+          this[_exeBins](pkg, dest)).then(() => result))
+  }
+
   [_tarballFromResolved] () {
     // create a read stream and return it
     return new fsm.ReadStream(this.resolved)

lib/util/is-package-bin.js

@@ -0,0 +1,24 @@
+// Function to determine whether a path is in the package.bin set.
+// Used to prevent issues when people publish a package from a
+// windows machine, and then install with --no-bin-links.
+//
+// Note: this is not possible in remote or file fetchers, since
+// we don't have the manifest until AFTER we've unpacked.  But the
+// main use case is registry fetching with git a distant second,
+// so that's an acceptable edge case to not handle.
+
+const binObj = (name, bin) =>
+  typeof bin === 'string' ? { [name]: bin } : bin
+
+const hasBin = (pkg, path) => {
+  const bin = binObj(pkg.name, pkg.bin)
+  const p = path.replace(/^[^\\\/]*\//, '')
+  for (const [k, v] of Object.entries(bin)) {
+    if (v === p)
+      return true
+  }
+  return false
+}
+
+module.exports = (pkg, path) =>
+  pkg && pkg.bin ? hasBin(pkg, path) : false

tap-snapshots/test-dir.js-TAP.test.js

@@ -165,6 +165,13 @@ Object {
 }
 `
 
+exports[`test/dir.js TAP make bins executable > results of unpack 1`] = `
+Object {
+  "integrity": "sha512-rlE32nBV7XgKCm0I7YqAewyVPbaRJWUQMZUFLlngGK3imG+som3Hin7d/zPTikWg64tHIxb8VXeeq6u0IRRfmQ==",
+  "resolved": "\${CWD}/test/fixtures/bin-object",
+}
+`
+
 exports[`test/dir.js TAP with prepare script > extract 1`] = `
 Object {
   "integrity": "sha512-HTzPAt8wmXNchUdisnGDSCuUgrFee5v8F6GsLc5mQd29VXiNzv4PGz71jjLSIF1wWQSB+UjLTmSJSGznF/s/Lw==",

tap-snapshots/test-fetcher.js-TAP.test.js

@@ -0,0 +1,13 @@
+/* IMPORTANT
+ * This snapshot file is auto-generated, but designed for humans.
+ * It should be checked into source control and tracked carefully.
+ * Re-generate by setting TAP_SNAPSHOT=1 and running tests.
+ * Make sure to inspect the output below.  Do not ignore changes!
+ */
+'use strict'
+exports[`test/fetcher.js TAP make bins executable > results of unpack 1`] = `
+Object {
+  "integrity": "sha512-TqzCjecWyQe8vqLbT0nv/OaWf0ptRZ2DnPmiuGUYJJb70shp02+/uu37IJSkM2ZEP1SAOeKrYrWPVIIYW+d//g==",
+  "resolved": "{CWD}/test/fixtures/bin-object.tgz",
+}
+`

tap-snapshots/test-fetcher.js-fake-sudo-TAP.test.js

@@ -0,0 +1,13 @@
+/* IMPORTANT
+ * This snapshot file is auto-generated, but designed for humans.
+ * It should be checked into source control and tracked carefully.
+ * Re-generate by setting TAP_SNAPSHOT=1 and running tests.
+ * Make sure to inspect the output below.  Do not ignore changes!
+ */
+'use strict'
+exports[`test/fetcher.js fake-sudo TAP make bins executable > results of unpack 1`] = `
+Object {
+  "integrity": "sha512-TqzCjecWyQe8vqLbT0nv/OaWf0ptRZ2DnPmiuGUYJJb70shp02+/uu37IJSkM2ZEP1SAOeKrYrWPVIIYW+d//g==",
+  "resolved": "{CWD}/test/fixtures/bin-object.tgz",
+}
+`

tap-snapshots/test-file.js-TAP.test.js

@@ -125,3 +125,24 @@ Object {
   },
 }
 `
+
+exports[`test/file.js TAP make bins executable bin-good > results of unpack 1`] = `
+Object {
+  "integrity": "sha512-Fx11OiHxV82CztnPk+k0S6H/66J4/eUzZEMGX2dJjP+Mxfrm8fSzE4SQG604zWk17ELZsOGENCdWSkvj4cpjUw==",
+  "resolved": "\${CWD}/test/fixtures/bin-good.tgz",
+}
+`
+
+exports[`test/file.js TAP make bins executable bin-object > results of unpack 1`] = `
+Object {
+  "integrity": "sha512-TqzCjecWyQe8vqLbT0nv/OaWf0ptRZ2DnPmiuGUYJJb70shp02+/uu37IJSkM2ZEP1SAOeKrYrWPVIIYW+d//g==",
+  "resolved": "\${CWD}/test/fixtures/bin-object.tgz",
+}
+`
+
+exports[`test/file.js TAP make bins executable bin-string > results of unpack 1`] = `
+Object {
+  "integrity": "sha512-iCc87DMYVMofO221ksAlMD88Zgsr4OIvqeX73KxTPikWaQPvBFZpzI9FGWnD4PTLTyJzOSETQh86+IwEidJRZg==",
+  "resolved": "\${CWD}/test/fixtures/bin-string.tgz",
+}
+`

test/dir.js

@@ -59,3 +59,13 @@ t.test('when read fails', t => {
   const f = new DirFetcher(preparespec, {})
   return t.rejects(f.extract(me + '/nope'), poop)
 })
+
+t.test('make bins executable', async t => {
+  const file = resolve(__dirname, 'fixtures/bin-object')
+  const spec = `file:${relative(process.cwd(), file)}`
+  const f = new DirFetcher(spec, {})
+  const target = resolve(me, basename(file))
+  const res = await f.extract(target)
+  t.matchSnapshot(res, 'results of unpack')
+  t.equal(fs.statSync(target + '/script.js').mode & 0o111, 0o111)
+})

test/fetcher.js

@@ -18,6 +18,7 @@ const { relative, resolve, basename } = require('path')
 const me = resolve(__dirname, basename(__filename, '.js'))
 const Fetcher = require('../lib/fetcher.js')
 const t = require('tap')
+t.cleanSnapshot = s => s.split(process.cwd()).join('{CWD}')
 if (!fakeSudo)
   t.teardown(() => require('rimraf').sync(me))
 
@@ -378,5 +379,17 @@ if (!fakeSudo) {
 
     t.end()
   })
-
 }
+
+t.test('make bins executable', async t => {
+  const file = resolve(__dirname, 'fixtures/bin-object.tgz')
+  const spec = `file:${relative(process.cwd(), file)}`
+  const f = new FileFetcher(spec, {})
+  // simulate a fetcher that already has a manifest
+  const manifest = require('./fixtures/bin-object/package.json')
+  f.package = manifest
+  const target = resolve(me, basename(file, '.tgz'))
+  const res = await f.extract(target)
+  t.matchSnapshot(res, 'results of unpack')
+  t.equal(fs.statSync(target + '/script.js').mode & 0o111, 0o111)
+})

test/file.js

@@ -26,3 +26,31 @@ t.test('basic', async t => {
   return t.resolveMatchSnapshot(f.extract(me + '/extract'), 'extract')
     .then(() => t.matchSnapshot(require(pj), 'package.json extracted'))
 })
+
+const binString = resolve(__dirname, 'fixtures/bin-string.tgz')
+const binObject = resolve(__dirname, 'fixtures/bin-object.tgz')
+// this one actually doesn't need any help
+const binGood = resolve(__dirname, 'fixtures/bin-good.tgz')
+
+t.test('make bins executable', t => {
+  const fs = require('fs')
+  const files = [binString, binObject, binGood]
+  t.plan(files.length)
+  files.forEach(file => t.test(basename(file, '.tgz'), async t => {
+    const spec = `file:${relative(process.cwd(), file)}`
+    const f = new FileFetcher(spec, {})
+    const target = resolve(me, basename(file, '.tgz'))
+    const res = await f.extract(target)
+    t.matchSnapshot(res, 'results of unpack')
+    t.equal(fs.statSync(target + '/script.js').mode & 0o111, 0o111)
+  }))
+})
+
+t.test('dont bork on missing script', async t => {
+  const file = resolve(__dirname, 'fixtures/bin-missing.tgz')
+  const spec = `file:${relative(process.cwd(), file)}`
+  const f = new FileFetcher(spec, {})
+  const target = resolve(me, basename(file, '.tgz'))
+  const res = await f.extract(target)
+  t.throws(() => fs.statSync(target + '/script.js'), 'should be missing')
+})

test/fixtures/bin-good.tgz

@@ -0,0 +1 @@
+{"name":"bin-object","version":"1.2.3","bin":{"bin-object":"script.js"}}

test/fixtures/bin-good/package.json

@@ -0,0 +1,5 @@
+#!/usr/bin/env node
+const fs = require('fs')
+const assert = require('assert')
+assert.equal(fs.statSync(__filename).mode & 0o111, 0o111)
+console.log('ok')

test/fixtures/bin-good/script.js

@@ -0,0 +1 @@
+{"name":"bin-string","version":"1.2.3","bin":"script.js"}

test/fixtures/bin-missing.tgz

@@ -0,0 +1 @@
+{"name":"bin-object","version":"1.2.3","bin":{"bin-object":"script.js"}}

test/fixtures/bin-missing/package.json

@@ -0,0 +1,5 @@
+#!/usr/bin/env node
+const fs = require('fs')
+const assert = require('assert')
+assert.equal(fs.statSync(__filename).mode & 0o111, 0o111)
+console.log('ok')

test/fixtures/bin-object.tgz

@@ -0,0 +1 @@
+{"name":"bin-string","version":"1.2.3","bin":"script.js"}

test/fixtures/bin-object/package.json

@@ -0,0 +1,5 @@
+#!/usr/bin/env node
+const fs = require('fs')
+const assert = require('assert')
+assert.equal(fs.statSync(__filename).mode & 0o111, 0o111)
+console.log('ok')

test/fixtures/bin-object/script.js

@@ -0,0 +1,8 @@
+const isPackageBin = require('../../lib/util/is-package-bin.js')
+const t = require('tap')
+
+t.ok(isPackageBin({bin:'foo'}, 'package/foo'), 'finds string')
+t.ok(isPackageBin({bin:{bar:'foo'}}, 'package/foo'), 'finds in obj')
+t.notOk(isPackageBin(null, 'anything'), 'return false if pkg is not')
+t.notOk(isPackageBin({bin:'foo'}, 'package/bar'), 'not the bin string')
+t.notOk(isPackageBin({bin:{bar:'foo'}}, 'package/bar'), 'not in obj')