From 1f4473aaed629fb3592029c5db289533e89975b8 Mon Sep 17 00:00:00 2001 From: isaacs Date: Sat, 5 Oct 2019 23:02:06 -0700 Subject: [PATCH] Pack and unpack preserving exec perms on all package bins This addresses the problem brought up by @boneskull on https://github.com/npm/node-tar/issues/210#issuecomment-480994853 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. --- lib/dir.js | 37 +++++++++++++----- lib/fetcher.js | 18 +++++---- lib/file.js | 33 ++++++++++++++++ lib/util/is-package-bin.js | 24 ++++++++++++ tap-snapshots/test-dir.js-TAP.test.js | 7 ++++ tap-snapshots/test-fetcher.js-TAP.test.js | 13 ++++++ .../test-fetcher.js-fake-sudo-TAP.test.js | 13 ++++++ tap-snapshots/test-file.js-TAP.test.js | 21 ++++++++++ test/dir.js | 10 +++++ test/fetcher.js | 15 ++++++- test/file.js | 28 +++++++++++++ test/fixtures/bin-good.tgz | Bin 0 -> 354 bytes test/fixtures/bin-good/package.json | 1 + test/fixtures/bin-good/script.js | 5 +++ test/fixtures/bin-missing.tgz | Bin 0 -> 224 bytes test/fixtures/bin-missing/package.json | 1 + test/fixtures/bin-object.tgz | Bin 0 -> 354 bytes test/fixtures/bin-object/package.json | 1 + test/fixtures/bin-object/script.js | 5 +++ test/fixtures/bin-string.tgz | Bin 0 -> 350 bytes test/fixtures/bin-string/package.json | 1 + test/fixtures/bin-string/script.js | 5 +++ test/util/is-package-bin.js | 8 ++++ 23 files changed, 228 insertions(+), 18 deletions(-) create mode 100644 lib/util/is-package-bin.js create mode 100644 tap-snapshots/test-fetcher.js-TAP.test.js create mode 100644 tap-snapshots/test-fetcher.js-fake-sudo-TAP.test.js create mode 100644 test/fixtures/bin-good.tgz create mode 100644 test/fixtures/bin-good/package.json create mode 100755 test/fixtures/bin-good/script.js create mode 100644 test/fixtures/bin-missing.tgz create mode 100644 test/fixtures/bin-missing/package.json create mode 100644 test/fixtures/bin-object.tgz create mode 100644 test/fixtures/bin-object/package.json create mode 100644 test/fixtures/bin-object/script.js create mode 100644 test/fixtures/bin-string.tgz create mode 100644 test/fixtures/bin-string/package.json create mode 100644 test/fixtures/bin-string/script.js create mode 100644 test/util/is-package-bin.js 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-good.tgz b/test/fixtures/bin-good.tgz new file mode 100644 index 0000000000000000000000000000000000000000..ba501b5d3e9ed6b8a21afcde11cf60abbaaa6d38 GIT binary patch literal 354 zcmV-o0iFIIiwFSagPC0b1MSuAYQiuS24H{hDzXe_1(zo2hal`Kb^#J=D%F~}rV2y6 z_r=u}ak#zw|>gMjO0=N z6E2eu;pgAVS2v#jhx>5ls?cA9P@6R|cB1bxYg|ooN1jQgUspw?gKqA+y?uvrPFM9| zXVc;bG!M+Tt1`$P^M_WyyqPQ*jq;+@MlE!YE&Lkfk#H-7=r^I6+LoG?b{cf;+|PCG zlerCT{V!EES5xgj0M?v&4POsw9G|X#e*uo`KaM5p|1WIlt|;k{?k<8I&vaJPK3(g| z71sD>!6b`(i|?kx&EcpaZGRbbyG6MI000000000000000003OiJI2km^8hFS0FR!q A!2kdN literal 0 HcmV?d00001 diff --git a/test/fixtures/bin-good/package.json b/test/fixtures/bin-good/package.json new file mode 100644 index 00000000..8ef131c7 --- /dev/null +++ b/test/fixtures/bin-good/package.json @@ -0,0 +1 @@ +{"name":"bin-object","version":"1.2.3","bin":{"bin-object":"script.js"}} diff --git a/test/fixtures/bin-good/script.js b/test/fixtures/bin-good/script.js new file mode 100755 index 00000000..7f473bc0 --- /dev/null +++ b/test/fixtures/bin-good/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-missing.tgz b/test/fixtures/bin-missing.tgz new file mode 100644 index 0000000000000000000000000000000000000000..54cc0b371aabf141c11b987fa7b5ffd664120f0a GIT binary patch literal 224 zcmV<603ZJ!iwFSQd6``R1MSkyY63A3!0|o%6d`B3O|sby^ljp@N?MGvS$iqIyVEKn zf}WI8`u|;qe2`2svq<@6ok~f0^;$iNF($OF`ib2!8t0GkMA@eGoeMtLpp0#TYbw<~ zE1^rfGR09+NhQX<+}dMLlKFXTBw_kHr-JpzW-tH?H1(<000000000000000003|~Z^~Guvj8Xn03_I~ AMF0Q* literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..0c088ad0edf7cd591c223315f661a8cf263531c4 GIT binary patch literal 350 zcmV-k0ipgMiwFSXdYN4S1MSuAO2aS|2H^enDq;oK!Az4j?LhDXTLVvJs z&>