diff --git a/lib/dir.js b/lib/dir.js index 5d109b9e..78478a31 100644 --- a/lib/dir.js +++ b/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) diff --git a/lib/fetcher.js b/lib/fetcher.js index d1bd6116..5d85c04e 100644 --- a/lib/fetcher.js +++ b/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,11 +334,14 @@ 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 }) { @@ -345,13 +349,13 @@ class FetcherBase { 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) diff --git a/lib/file.js b/lib/file.js index f808119e..c2334fdb 100644 --- a/lib/file.js +++ b/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) diff --git a/lib/util/is-package-bin.js b/lib/util/is-package-bin.js new file mode 100644 index 00000000..35cf0642 --- /dev/null +++ b/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 diff --git a/tap-snapshots/test-dir.js-TAP.test.js b/tap-snapshots/test-dir.js-TAP.test.js index 3310b91c..7bed8fdc 100644 --- a/tap-snapshots/test-dir.js-TAP.test.js +++ b/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==", diff --git a/tap-snapshots/test-fetcher.js-TAP.test.js b/tap-snapshots/test-fetcher.js-TAP.test.js new file mode 100644 index 00000000..3fd534cc --- /dev/null +++ b/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", +} +` diff --git a/tap-snapshots/test-fetcher.js-fake-sudo-TAP.test.js b/tap-snapshots/test-fetcher.js-fake-sudo-TAP.test.js new file mode 100644 index 00000000..9e19dcf2 --- /dev/null +++ b/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", +} +` diff --git a/tap-snapshots/test-file.js-TAP.test.js b/tap-snapshots/test-file.js-TAP.test.js index dcd587bb..b1e1dc34 100644 --- a/tap-snapshots/test-file.js-TAP.test.js +++ b/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", +} +` diff --git a/test/dir.js b/test/dir.js index d81d937c..177f42c8 100644 --- a/test/dir.js +++ b/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) +}) diff --git a/test/fetcher.js b/test/fetcher.js index 181486c7..c66f264c 100644 --- a/test/fetcher.js +++ b/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) +}) diff --git a/test/file.js b/test/file.js index d6aa5a55..b994e9af 100644 --- a/test/file.js +++ b/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') +}) diff --git a/test/fixtures/bin-missing.tgz b/test/fixtures/bin-missing.tgz new file mode 100644 index 00000000..54cc0b37 Binary files /dev/null and b/test/fixtures/bin-missing.tgz differ diff --git a/test/fixtures/bin-missing/package.json b/test/fixtures/bin-missing/package.json new file mode 100644 index 00000000..aaa75a24 --- /dev/null +++ b/test/fixtures/bin-missing/package.json @@ -0,0 +1 @@ +{"name":"bin-string","version":"1.2.3","bin":"script.js"} diff --git a/test/fixtures/bin-object.tgz b/test/fixtures/bin-object.tgz new file mode 100644 index 00000000..efba18dc Binary files /dev/null and b/test/fixtures/bin-object.tgz differ diff --git a/test/fixtures/bin-object/package.json b/test/fixtures/bin-object/package.json new file mode 100644 index 00000000..8ef131c7 --- /dev/null +++ b/test/fixtures/bin-object/package.json @@ -0,0 +1 @@ +{"name":"bin-object","version":"1.2.3","bin":{"bin-object":"script.js"}} diff --git a/test/fixtures/bin-object/script.js b/test/fixtures/bin-object/script.js new file mode 100644 index 00000000..7f473bc0 --- /dev/null +++ b/test/fixtures/bin-object/script.js @@ -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') diff --git a/test/fixtures/bin-string.tgz b/test/fixtures/bin-string.tgz new file mode 100644 index 00000000..0c088ad0 Binary files /dev/null and b/test/fixtures/bin-string.tgz differ diff --git a/test/fixtures/bin-string/package.json b/test/fixtures/bin-string/package.json new file mode 100644 index 00000000..aaa75a24 --- /dev/null +++ b/test/fixtures/bin-string/package.json @@ -0,0 +1 @@ +{"name":"bin-string","version":"1.2.3","bin":"script.js"} diff --git a/test/fixtures/bin-string/script.js b/test/fixtures/bin-string/script.js new file mode 100644 index 00000000..7f473bc0 --- /dev/null +++ b/test/fixtures/bin-string/script.js @@ -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') diff --git a/test/util/is-package-bin.js b/test/util/is-package-bin.js new file mode 100644 index 00000000..522b1a5b --- /dev/null +++ b/test/util/is-package-bin.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')