From 642cd1890d1e0ca32b5f9ceadc6a587d94a63d69 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 10 Dec 2019 18:13:08 -0800 Subject: [PATCH] fix: prevent improper clobbering of man/bin links This uses the updated version of gentle-fs which has the binLink method, so that we don't need to fork based on OS, and will always check to ensure that the bin or cmd-shim is a reference into the folder being linked. Also perform a similar check on linked man pages. Fix #11 PR-URL: https://github.com/npm/bin-links/pull/12 Credit: @isaacs Close: #12 Reviewed-by: @isaacs --- index.js | 30 +++++-- package-lock.json | 36 ++++++-- package.json | 6 +- test/link-bins.js | 216 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 270 insertions(+), 18 deletions(-) create mode 100644 test/link-bins.js diff --git a/index.js b/index.js index eda6abd..4f6d3c0 100644 --- a/index.js +++ b/index.js @@ -3,8 +3,9 @@ const path = require('path') const fs = require('graceful-fs') const BB = require('bluebird') -const linkIfExists = BB.promisify(require('gentle-fs').linkIfExists) -const cmdShimIfExists = BB.promisify(require('cmd-shim').ifExists) +const gentleFs = require('gentle-fs') +const linkIfExists = BB.promisify(gentleFs.linkIfExists) +const gentleFsBinLink = BB.promisify(gentleFs.binLink) const open = BB.promisify(fs.open) const close = BB.promisify(fs.close) const read = BB.promisify(fs.read, {multiArgs: true}) @@ -42,9 +43,9 @@ function isHashbangFile (file) { return read(fileHandle, Buffer.alloc(2), 0, 2, 0).spread((_, buf) => { if (!hasHashbang(buf)) return [] return read(fileHandle, Buffer.alloc(2048), 0, 2048, 0) - }).spread((_, buf) => buf && hasCR(buf), () => false) + }).spread((_, buf) => buf && hasCR(buf), /* istanbul ignore next */ () => false) .finally(() => close(fileHandle)) - }).catch(() => false) + }).catch(/* istanbul ignore next */ () => false) } function hasHashbang (buf) { @@ -109,6 +110,7 @@ function linkBins (pkg, folder, parent, gtop, opts) { opts.log.showProgress() } }).catch(err => { + /* istanbul ignore next */ if (err.code === 'ENOENT' && opts.ignoreScripts) return throw err }) @@ -116,11 +118,11 @@ function linkBins (pkg, folder, parent, gtop, opts) { } function linkBin (from, to, opts) { - if (process.platform !== 'win32') { - return linkIfExists(from, to, opts) - } else { - return cmdShimIfExists(from, to) + // do not clobber global bins + if (opts.globalBin && to.indexOf(opts.globalBin) === 0) { + opts.clobberLinkGently = true } + return gentleFsBinLink(from, to, opts) } function linkMans (pkg, folder, parent, gtop, opts) { @@ -132,17 +134,22 @@ function linkMans (pkg, folder, parent, gtop, opts) { // make sure that the mans are unique. // otherwise, if there are dupes, it'll fail with EEXIST var set = pkg.man.reduce(function (acc, man) { + if (typeof man !== 'string') { + return acc + } const cleanMan = path.join('/', man).replace(/\\|:/g, '/').substr(1) acc[path.basename(man)] = cleanMan return acc }, {}) var manpages = pkg.man.filter(function (man) { + if (typeof man !== 'string') { + return false + } const cleanMan = path.join('/', man).replace(/\\|:/g, '/').substr(1) return set[path.basename(man)] === cleanMan }) return BB.map(manpages, man => { - if (typeof man !== 'string') return opts.log.silly('linkMans', 'preparing to link', man) var parseMan = man.match(/(.*\.([0-9]+)(\.gz)?)$/) if (!parseMan) { @@ -165,6 +172,11 @@ function linkMans (pkg, folder, parent, gtop, opts) { var manDest = path.join(manRoot, 'man' + sxn, bn) + // man pages should always be clobbering gently, because they are + // only installed for top-level global packages, so never destroy + // a link if it doesn't point into the folder we're linking + opts.clobberLinkGently = true + return linkIfExists(manSrc, manDest, getLinkOpts(opts, gtop && folder)) }) } diff --git a/package-lock.json b/package-lock.json index e0b12bd..ba76228 100644 --- a/package-lock.json +++ b/package-lock.json @@ -487,6 +487,11 @@ "supports-color": "^2.0.0" } }, + "chownr": { + "version": "1.1.3", + "resolved": "https://registry.npmjs.org/chownr/-/chownr-1.1.3.tgz", + "integrity": "sha512-i70fVHhmV3DtTl6nqvZOnIjbY0Pe4kAUjwHj8z0zAdgBtYrJyYwLKCCuRBQ5ppkyL0AkN7HKRnETdmdp1zqNXw==" + }, "circular-json": { "version": "0.3.3", "resolved": "https://registry.npmjs.org/circular-json/-/circular-json-0.3.3.tgz", @@ -1683,18 +1688,32 @@ } }, "gentle-fs": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/gentle-fs/-/gentle-fs-2.0.1.tgz", - "integrity": "sha512-cEng5+3fuARewXktTEGbwsktcldA+YsnUEaXZwcK/3pjSE1X9ObnTs+/8rYf8s+RnIcQm2D5x3rwpN7Zom8Bew==", + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/gentle-fs/-/gentle-fs-2.3.0.tgz", + "integrity": "sha512-3k2CgAmPxuz7S6nKK+AqFE2AdM1QuwqKLPKzIET3VRwK++3q96MsNFobScDjlCrq97ZJ8y5R725MOlm6ffUCjg==", "requires": { "aproba": "^1.1.2", + "chownr": "^1.1.2", + "cmd-shim": "^3.0.3", "fs-vacuum": "^1.2.10", "graceful-fs": "^4.1.11", "iferr": "^0.1.5", + "infer-owner": "^1.0.4", "mkdirp": "^0.5.1", "path-is-inside": "^1.0.2", "read-cmd-shim": "^1.0.1", "slide": "^1.1.6" + }, + "dependencies": { + "cmd-shim": { + "version": "3.0.3", + "resolved": "https://registry.npmjs.org/cmd-shim/-/cmd-shim-3.0.3.tgz", + "integrity": "sha512-DtGg+0xiFhQIntSBRzL2fRQBnmtAVwXIDo4Qq46HPpObYquxMaZS4sb82U9nH91qJrlosC1wa9gwr0QyL/HypA==", + "requires": { + "graceful-fs": "^4.1.2", + "mkdirp": "~0.5.0" + } + } } }, "get-caller-file": { @@ -2033,6 +2052,11 @@ "integrity": "sha1-Sl/W0nzDMvN+VBmlBNu4NxBckok=", "dev": true }, + "infer-owner": { + "version": "1.0.4", + "resolved": "https://registry.npmjs.org/infer-owner/-/infer-owner-1.0.4.tgz", + "integrity": "sha512-IClj+Xz94+d7irH5qRyfJonOdfTzuDaifE6ZPWfx0N0+/ATZCbuTPq2prFl526urkQd90WyUKIh1DfBQ2hMz9A==" + }, "inflight": { "version": "1.0.6", "resolved": "https://registry.npmjs.org/inflight/-/inflight-1.0.6.tgz", @@ -4222,9 +4246,9 @@ "dev": true }, "read-cmd-shim": { - "version": "1.0.1", - "resolved": "https://registry.npmjs.org/read-cmd-shim/-/read-cmd-shim-1.0.1.tgz", - "integrity": "sha1-LV0Vd4ajfAVdIgd8MsU/gynpHHs=", + "version": "1.0.5", + "resolved": "https://registry.npmjs.org/read-cmd-shim/-/read-cmd-shim-1.0.5.tgz", + "integrity": "sha512-v5yCqQ/7okKoZZkBQUAfTsQ3sVJtXdNfbPnI5cceppoxEVLYA3k+VtV2omkeo8MS94JCy4fSiUwlRBAwCVRPUA==", "requires": { "graceful-fs": "^4.1.2" } diff --git a/package.json b/package.json index beffb2b..8cb14ef 100644 --- a/package.json +++ b/package.json @@ -6,9 +6,9 @@ "scripts": { "prerelease": "npm t", "postrelease": "npm publish && git push --follow-tags", - "pretest": "standard", + "posttest": "standard", "release": "standard-version -s", - "test": "tap -J --nyc-arg=--all --coverage test/*.js", + "test": "tap -J --nyc-arg=--all --coverage test/*.js --100", "update-coc": "weallbehave -o . && git add CODE_OF_CONDUCT.md && git commit -m 'docs(coc): updated CODE_OF_CONDUCT.md'", "update-contrib": "weallcontribute -o . && git add CONTRIBUTING.md && git commit -m 'docs(contributing): updated CONTRIBUTING.md'" }, @@ -30,7 +30,7 @@ "dependencies": { "bluebird": "^3.5.3", "cmd-shim": "^3.0.0", - "gentle-fs": "^2.0.1", + "gentle-fs": "^2.3.0", "graceful-fs": "^4.1.15", "npm-normalize-package-bin": "^1.0.0", "write-file-atomic": "^2.3.0" diff --git a/test/link-bins.js b/test/link-bins.js new file mode 100644 index 0000000..ce2719a --- /dev/null +++ b/test/link-bins.js @@ -0,0 +1,216 @@ +const t = require('tap') +const BB = require('bluebird') +const binLinks = BB.promisify(require('../')) + +// forking between cmd-shims and symlinks is already handled by +// the gentle-fs.binLink module. just test the unix handling here. +const _FAKE_PLATFORM_ = process.platform === 'win32' ? 'unix' : null + +const fs = require('fs') +const mkdirp = require('mkdirp').sync +const rimraf = require('rimraf').sync +const {basename, resolve} = require('path') +const me = resolve(__dirname, basename(__filename, '.js')) +rimraf(me) +mkdirp(me) +const globalDir = resolve(me, 'node_modules') +t.teardown(() => rimraf(me)) + +const log = { + clearProgress () {}, + info () {}, + showProgress () {}, + silly () {}, + verbose () {} +} + +// create some stuff that's already in the bin/man folders +const globalBin = resolve(me, 'bin') +mkdirp(globalBin) +fs.writeFileSync(globalBin + '/foo', 'pre-existing foo bin') +mkdirp(me + '/node_modules/bar/bin') +fs.writeFileSync(me + '/node_modules/bar/bin/bar.js', 'original bar') +fs.symlinkSync(me + '/node_modules/bar/bin/bar.js', me + '/bin/bar') + +const prefixes = [ + me, + globalBin, + globalDir, + resolve(me, 'share/man/man1') +] + +mkdirp(me + '/share/man/man1') +fs.writeFileSync(me + '/share/man/man1/foo.1', 'pre-existing foo man') +mkdirp(me + '/node_modules/bar/man') +fs.writeFileSync(me + '/node_modules/bar/man/bar.1', 'original bar man') +fs.symlinkSync(me + '/node_modules/bar/man/bar.1', me + '/share/man/man1/bar.1') + +t.test('foo package cannot link, pre-existing stuff there', t => { + const foo = resolve(me, 'node_modules/foo') + mkdirp(foo) + const pkg = { + name: 'foo', + version: '1.2.3', + bin: 'foo.js', + man: ['foo.1', { not: 'a manpage string' }] + } + fs.writeFileSync(foo + '/package.json', JSON.stringify(pkg)) + fs.writeFileSync(foo + '/foo.1', 'how to foo\n') + fs.writeFileSync(foo + '/foo.js', '#!/usr/bin/env node\nconsole.log("foo")') + + // might get it on the bin or the man, but one of these will EEXIST + return t.rejects(binLinks(pkg, foo, true, { + prefix: me, + global: true, + globalBin, + globalDir, + log, + pkgId: `${pkg.name}@${pkg.version}`, + name: pkg.name, + _FAKE_PLATFORM_, + prefixes: prefixes.concat(foo) + }), { code: 'EEXIST' }).then(() => { + // ensure we still have our preexisting bits + t.equal(fs.readFileSync(me + '/bin/foo', 'utf8'), 'pre-existing foo bin') + t.equal(fs.readFileSync(me + '/share/man/man1/foo.1', 'utf8'), 'pre-existing foo man') + }) +}) + +t.test('foo package can link with --force link', t => { + const foo = resolve(me, 'node_modules/foo') + mkdirp(foo) + const pkg = { + name: 'foo', + version: '1.2.3', + bin: 'foo.js', + man: ['foo.1'] + } + fs.writeFileSync(foo + '/package.json', JSON.stringify(pkg)) + fs.writeFileSync(foo + '/foo.1', 'how to foo\n') + fs.writeFileSync(foo + '/foo.js', '#!/usr/bin/env node\nconsole.log("foo")') + + return binLinks(pkg, foo, true, { + prefix: me, + global: true, + globalBin, + globalDir, + log, + pkgId: `${pkg.name}@${pkg.version}`, + name: pkg.name, + _FAKE_PLATFORM_, + force: true, + prefixes: prefixes.concat(foo) + }).then(() => { + // ensure we got our links made + t.equal(fs.readFileSync(me + '/bin/foo', 'utf8'), '#!/usr/bin/env node\nconsole.log("foo")') + if (process.platform !== 'win32') { + t.equal(fs.readFileSync(me + '/share/man/man1/foo.1', 'utf8'), 'how to foo\n') + } + }) +}) + +t.test('bar package can update, links are ours', t => { + const bar = resolve(me, 'node_modules/bar') + mkdirp(bar) + const pkg = { + name: 'bar', + version: '1.2.3', + bin: 'bar.js', + man: ['bar.1'] + } + fs.writeFileSync(bar + '/package.json', JSON.stringify(pkg)) + fs.writeFileSync(bar + '/bar.1', 'how to bar\n') + fs.writeFileSync(bar + '/bar.js', '#!/usr/bin/env node\nconsole.log("bar")') + + return binLinks(pkg, bar, true, { + prefix: me, + parseable: true, + global: true, + globalBin, + globalDir, + log, + pkgId: `${pkg.name}@${pkg.version}`, + name: pkg.name, + _FAKE_PLATFORM_, + prefixes: prefixes.concat(bar, bar + '/man') + }).then(() => { + // ensure we got our links made + t.equal(fs.readFileSync(me + '/bin/bar', 'utf8'), '#!/usr/bin/env node\nconsole.log("bar")') + if (process.platform !== 'win32') { + t.equal(fs.readFileSync(me + '/share/man/man1/bar.1', 'utf8'), 'how to bar\n') + } + }) +}) + +t.test('cannot overwrite with another package with the same bin', t => { + const baz = resolve(me, 'node_modules/@scope/baz') + mkdirp(baz) + const pkg = { + name: '@scope/baz', + version: '1.2.3', + bin: { bar: 'baz.js' } + } + fs.writeFileSync(baz + '/package.json', JSON.stringify(pkg)) + fs.writeFileSync(baz + '/baz.js', '#!/usr/bin/env node\nconsole.log("baz")') + + return t.rejects(binLinks(pkg, baz, true, { + global: true, + prefix: me, + globalBin, + globalDir, + log, + pkgId: `${pkg.name}@${pkg.version}`, + _FAKE_PLATFORM_, + name: pkg.name, + prefixes: prefixes.concat(baz) + }), { code: 'EEXIST' }).then(() => { + // ensure bar is still intact + t.equal(fs.readFileSync(me + '/bin/bar', 'utf8'), '#!/usr/bin/env node\nconsole.log("bar")') + }) +}) + +t.test('nothing to link', t => { + const qux = resolve(me, 'node_modules/qux') + mkdirp(qux) + const pkg = { + name: 'qux', + version: '1.2.3' + } + fs.writeFileSync(qux + '/package.json', JSON.stringify(pkg)) + + return binLinks(pkg, qux, true, { + global: true, + prefix: me, + globalBin, + globalDir, + log, + pkgId: `${pkg.name}@${pkg.version}`, + _FAKE_PLATFORM_, + name: pkg.name, + prefixes: prefixes.concat(qux) + }) +}) + +t.test('invalid man page name', t => { + const badman = resolve(me, 'node_modules/badman') + mkdirp(badman) + const pkg = { + name: 'badman', + version: '1.2.3', + man: ['manpage'] + } + fs.writeFileSync(badman + '/package.json', JSON.stringify(pkg)) + fs.writeFileSync(badman + '/manpage', JSON.stringify(pkg)) + + return t.rejects(binLinks(pkg, badman, true, { + global: true, + prefix: me, + globalBin, + globalDir, + log, + pkgId: `${pkg.name}@${pkg.version}`, + _FAKE_PLATFORM_, + name: pkg.name, + prefixes: prefixes.concat(badman) + }), { message: 'manpage is not a valid name for a man file.' }) +})