Skip to content

Commit 642cd18

Browse files
committed
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: #12 Credit: @isaacs Close: #12 Reviewed-by: @isaacs
1 parent bc69419 commit 642cd18

File tree

4 files changed

+270
-18
lines changed

4 files changed

+270
-18
lines changed

index.js

+21-9
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@
33
const path = require('path')
44
const fs = require('graceful-fs')
55
const BB = require('bluebird')
6-
const linkIfExists = BB.promisify(require('gentle-fs').linkIfExists)
7-
const cmdShimIfExists = BB.promisify(require('cmd-shim').ifExists)
6+
const gentleFs = require('gentle-fs')
7+
const linkIfExists = BB.promisify(gentleFs.linkIfExists)
8+
const gentleFsBinLink = BB.promisify(gentleFs.binLink)
89
const open = BB.promisify(fs.open)
910
const close = BB.promisify(fs.close)
1011
const read = BB.promisify(fs.read, {multiArgs: true})
@@ -42,9 +43,9 @@ function isHashbangFile (file) {
4243
return read(fileHandle, Buffer.alloc(2), 0, 2, 0).spread((_, buf) => {
4344
if (!hasHashbang(buf)) return []
4445
return read(fileHandle, Buffer.alloc(2048), 0, 2048, 0)
45-
}).spread((_, buf) => buf && hasCR(buf), () => false)
46+
}).spread((_, buf) => buf && hasCR(buf), /* istanbul ignore next */ () => false)
4647
.finally(() => close(fileHandle))
47-
}).catch(() => false)
48+
}).catch(/* istanbul ignore next */ () => false)
4849
}
4950

5051
function hasHashbang (buf) {
@@ -109,18 +110,19 @@ function linkBins (pkg, folder, parent, gtop, opts) {
109110
opts.log.showProgress()
110111
}
111112
}).catch(err => {
113+
/* istanbul ignore next */
112114
if (err.code === 'ENOENT' && opts.ignoreScripts) return
113115
throw err
114116
})
115117
})
116118
}
117119

118120
function linkBin (from, to, opts) {
119-
if (process.platform !== 'win32') {
120-
return linkIfExists(from, to, opts)
121-
} else {
122-
return cmdShimIfExists(from, to)
121+
// do not clobber global bins
122+
if (opts.globalBin && to.indexOf(opts.globalBin) === 0) {
123+
opts.clobberLinkGently = true
123124
}
125+
return gentleFsBinLink(from, to, opts)
124126
}
125127

126128
function linkMans (pkg, folder, parent, gtop, opts) {
@@ -132,17 +134,22 @@ function linkMans (pkg, folder, parent, gtop, opts) {
132134
// make sure that the mans are unique.
133135
// otherwise, if there are dupes, it'll fail with EEXIST
134136
var set = pkg.man.reduce(function (acc, man) {
137+
if (typeof man !== 'string') {
138+
return acc
139+
}
135140
const cleanMan = path.join('/', man).replace(/\\|:/g, '/').substr(1)
136141
acc[path.basename(man)] = cleanMan
137142
return acc
138143
}, {})
139144
var manpages = pkg.man.filter(function (man) {
145+
if (typeof man !== 'string') {
146+
return false
147+
}
140148
const cleanMan = path.join('/', man).replace(/\\|:/g, '/').substr(1)
141149
return set[path.basename(man)] === cleanMan
142150
})
143151

144152
return BB.map(manpages, man => {
145-
if (typeof man !== 'string') return
146153
opts.log.silly('linkMans', 'preparing to link', man)
147154
var parseMan = man.match(/(.*\.([0-9]+)(\.gz)?)$/)
148155
if (!parseMan) {
@@ -165,6 +172,11 @@ function linkMans (pkg, folder, parent, gtop, opts) {
165172

166173
var manDest = path.join(manRoot, 'man' + sxn, bn)
167174

175+
// man pages should always be clobbering gently, because they are
176+
// only installed for top-level global packages, so never destroy
177+
// a link if it doesn't point into the folder we're linking
178+
opts.clobberLinkGently = true
179+
168180
return linkIfExists(manSrc, manDest, getLinkOpts(opts, gtop && folder))
169181
})
170182
}

package-lock.json

+30-6
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

+3-3
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@
66
"scripts": {
77
"prerelease": "npm t",
88
"postrelease": "npm publish && git push --follow-tags",
9-
"pretest": "standard",
9+
"posttest": "standard",
1010
"release": "standard-version -s",
11-
"test": "tap -J --nyc-arg=--all --coverage test/*.js",
11+
"test": "tap -J --nyc-arg=--all --coverage test/*.js --100",
1212
"update-coc": "weallbehave -o . && git add CODE_OF_CONDUCT.md && git commit -m 'docs(coc): updated CODE_OF_CONDUCT.md'",
1313
"update-contrib": "weallcontribute -o . && git add CONTRIBUTING.md && git commit -m 'docs(contributing): updated CONTRIBUTING.md'"
1414
},
@@ -30,7 +30,7 @@
3030
"dependencies": {
3131
"bluebird": "^3.5.3",
3232
"cmd-shim": "^3.0.0",
33-
"gentle-fs": "^2.0.1",
33+
"gentle-fs": "^2.3.0",
3434
"graceful-fs": "^4.1.15",
3535
"npm-normalize-package-bin": "^1.0.0",
3636
"write-file-atomic": "^2.3.0"

test/link-bins.js

+216
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
const t = require('tap')
2+
const BB = require('bluebird')
3+
const binLinks = BB.promisify(require('../'))
4+
5+
// forking between cmd-shims and symlinks is already handled by
6+
// the gentle-fs.binLink module. just test the unix handling here.
7+
const _FAKE_PLATFORM_ = process.platform === 'win32' ? 'unix' : null
8+
9+
const fs = require('fs')
10+
const mkdirp = require('mkdirp').sync
11+
const rimraf = require('rimraf').sync
12+
const {basename, resolve} = require('path')
13+
const me = resolve(__dirname, basename(__filename, '.js'))
14+
rimraf(me)
15+
mkdirp(me)
16+
const globalDir = resolve(me, 'node_modules')
17+
t.teardown(() => rimraf(me))
18+
19+
const log = {
20+
clearProgress () {},
21+
info () {},
22+
showProgress () {},
23+
silly () {},
24+
verbose () {}
25+
}
26+
27+
// create some stuff that's already in the bin/man folders
28+
const globalBin = resolve(me, 'bin')
29+
mkdirp(globalBin)
30+
fs.writeFileSync(globalBin + '/foo', 'pre-existing foo bin')
31+
mkdirp(me + '/node_modules/bar/bin')
32+
fs.writeFileSync(me + '/node_modules/bar/bin/bar.js', 'original bar')
33+
fs.symlinkSync(me + '/node_modules/bar/bin/bar.js', me + '/bin/bar')
34+
35+
const prefixes = [
36+
me,
37+
globalBin,
38+
globalDir,
39+
resolve(me, 'share/man/man1')
40+
]
41+
42+
mkdirp(me + '/share/man/man1')
43+
fs.writeFileSync(me + '/share/man/man1/foo.1', 'pre-existing foo man')
44+
mkdirp(me + '/node_modules/bar/man')
45+
fs.writeFileSync(me + '/node_modules/bar/man/bar.1', 'original bar man')
46+
fs.symlinkSync(me + '/node_modules/bar/man/bar.1', me + '/share/man/man1/bar.1')
47+
48+
t.test('foo package cannot link, pre-existing stuff there', t => {
49+
const foo = resolve(me, 'node_modules/foo')
50+
mkdirp(foo)
51+
const pkg = {
52+
name: 'foo',
53+
version: '1.2.3',
54+
bin: 'foo.js',
55+
man: ['foo.1', { not: 'a manpage string' }]
56+
}
57+
fs.writeFileSync(foo + '/package.json', JSON.stringify(pkg))
58+
fs.writeFileSync(foo + '/foo.1', 'how to foo\n')
59+
fs.writeFileSync(foo + '/foo.js', '#!/usr/bin/env node\nconsole.log("foo")')
60+
61+
// might get it on the bin or the man, but one of these will EEXIST
62+
return t.rejects(binLinks(pkg, foo, true, {
63+
prefix: me,
64+
global: true,
65+
globalBin,
66+
globalDir,
67+
log,
68+
pkgId: `${pkg.name}@${pkg.version}`,
69+
name: pkg.name,
70+
_FAKE_PLATFORM_,
71+
prefixes: prefixes.concat(foo)
72+
}), { code: 'EEXIST' }).then(() => {
73+
// ensure we still have our preexisting bits
74+
t.equal(fs.readFileSync(me + '/bin/foo', 'utf8'), 'pre-existing foo bin')
75+
t.equal(fs.readFileSync(me + '/share/man/man1/foo.1', 'utf8'), 'pre-existing foo man')
76+
})
77+
})
78+
79+
t.test('foo package can link with --force link', t => {
80+
const foo = resolve(me, 'node_modules/foo')
81+
mkdirp(foo)
82+
const pkg = {
83+
name: 'foo',
84+
version: '1.2.3',
85+
bin: 'foo.js',
86+
man: ['foo.1']
87+
}
88+
fs.writeFileSync(foo + '/package.json', JSON.stringify(pkg))
89+
fs.writeFileSync(foo + '/foo.1', 'how to foo\n')
90+
fs.writeFileSync(foo + '/foo.js', '#!/usr/bin/env node\nconsole.log("foo")')
91+
92+
return binLinks(pkg, foo, true, {
93+
prefix: me,
94+
global: true,
95+
globalBin,
96+
globalDir,
97+
log,
98+
pkgId: `${pkg.name}@${pkg.version}`,
99+
name: pkg.name,
100+
_FAKE_PLATFORM_,
101+
force: true,
102+
prefixes: prefixes.concat(foo)
103+
}).then(() => {
104+
// ensure we got our links made
105+
t.equal(fs.readFileSync(me + '/bin/foo', 'utf8'), '#!/usr/bin/env node\nconsole.log("foo")')
106+
if (process.platform !== 'win32') {
107+
t.equal(fs.readFileSync(me + '/share/man/man1/foo.1', 'utf8'), 'how to foo\n')
108+
}
109+
})
110+
})
111+
112+
t.test('bar package can update, links are ours', t => {
113+
const bar = resolve(me, 'node_modules/bar')
114+
mkdirp(bar)
115+
const pkg = {
116+
name: 'bar',
117+
version: '1.2.3',
118+
bin: 'bar.js',
119+
man: ['bar.1']
120+
}
121+
fs.writeFileSync(bar + '/package.json', JSON.stringify(pkg))
122+
fs.writeFileSync(bar + '/bar.1', 'how to bar\n')
123+
fs.writeFileSync(bar + '/bar.js', '#!/usr/bin/env node\nconsole.log("bar")')
124+
125+
return binLinks(pkg, bar, true, {
126+
prefix: me,
127+
parseable: true,
128+
global: true,
129+
globalBin,
130+
globalDir,
131+
log,
132+
pkgId: `${pkg.name}@${pkg.version}`,
133+
name: pkg.name,
134+
_FAKE_PLATFORM_,
135+
prefixes: prefixes.concat(bar, bar + '/man')
136+
}).then(() => {
137+
// ensure we got our links made
138+
t.equal(fs.readFileSync(me + '/bin/bar', 'utf8'), '#!/usr/bin/env node\nconsole.log("bar")')
139+
if (process.platform !== 'win32') {
140+
t.equal(fs.readFileSync(me + '/share/man/man1/bar.1', 'utf8'), 'how to bar\n')
141+
}
142+
})
143+
})
144+
145+
t.test('cannot overwrite with another package with the same bin', t => {
146+
const baz = resolve(me, 'node_modules/@scope/baz')
147+
mkdirp(baz)
148+
const pkg = {
149+
name: '@scope/baz',
150+
version: '1.2.3',
151+
bin: { bar: 'baz.js' }
152+
}
153+
fs.writeFileSync(baz + '/package.json', JSON.stringify(pkg))
154+
fs.writeFileSync(baz + '/baz.js', '#!/usr/bin/env node\nconsole.log("baz")')
155+
156+
return t.rejects(binLinks(pkg, baz, true, {
157+
global: true,
158+
prefix: me,
159+
globalBin,
160+
globalDir,
161+
log,
162+
pkgId: `${pkg.name}@${pkg.version}`,
163+
_FAKE_PLATFORM_,
164+
name: pkg.name,
165+
prefixes: prefixes.concat(baz)
166+
}), { code: 'EEXIST' }).then(() => {
167+
// ensure bar is still intact
168+
t.equal(fs.readFileSync(me + '/bin/bar', 'utf8'), '#!/usr/bin/env node\nconsole.log("bar")')
169+
})
170+
})
171+
172+
t.test('nothing to link', t => {
173+
const qux = resolve(me, 'node_modules/qux')
174+
mkdirp(qux)
175+
const pkg = {
176+
name: 'qux',
177+
version: '1.2.3'
178+
}
179+
fs.writeFileSync(qux + '/package.json', JSON.stringify(pkg))
180+
181+
return binLinks(pkg, qux, true, {
182+
global: true,
183+
prefix: me,
184+
globalBin,
185+
globalDir,
186+
log,
187+
pkgId: `${pkg.name}@${pkg.version}`,
188+
_FAKE_PLATFORM_,
189+
name: pkg.name,
190+
prefixes: prefixes.concat(qux)
191+
})
192+
})
193+
194+
t.test('invalid man page name', t => {
195+
const badman = resolve(me, 'node_modules/badman')
196+
mkdirp(badman)
197+
const pkg = {
198+
name: 'badman',
199+
version: '1.2.3',
200+
man: ['manpage']
201+
}
202+
fs.writeFileSync(badman + '/package.json', JSON.stringify(pkg))
203+
fs.writeFileSync(badman + '/manpage', JSON.stringify(pkg))
204+
205+
return t.rejects(binLinks(pkg, badman, true, {
206+
global: true,
207+
prefix: me,
208+
globalBin,
209+
globalDir,
210+
log,
211+
pkgId: `${pkg.name}@${pkg.version}`,
212+
_FAKE_PLATFORM_,
213+
name: pkg.name,
214+
prefixes: prefixes.concat(badman)
215+
}), { message: 'manpage is not a valid name for a man file.' })
216+
})

0 commit comments

Comments
 (0)