Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 08801be

Browse files
committedSep 26, 2019
fix(fix-owner): chownr.sync quits on non-root uid
- Replicates logic that prevents non-root users from trying to change file ownership that is present in async version but missing in `chownr.sync` - test: Add test coverage to lib/utils/fix-owner.js module
1 parent 63ef08d commit 08801be

File tree

2 files changed

+258
-0
lines changed

2 files changed

+258
-0
lines changed
 

Diff for: ‎lib/util/fix-owner.js

+5
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@ function fixOwnerSync (cache, filepath) {
7777
}
7878
const { uid, gid } = inferOwner.sync(cache)
7979
getSelf()
80+
if (self.uid !== 0) {
81+
// almost certainly can't chown anyway
82+
return
83+
}
84+
8085
if (self.uid === uid && self.gid === gid) {
8186
// No need to override if it's already what we used.
8287
return

Diff for: ‎test/util.fix-owner.js

+253
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,253 @@
1+
'use strict'
2+
3+
const os = require('os')
4+
5+
const { test } = require('tap')
6+
const requireInject = require('require-inject')
7+
const uniqueFilename = require('unique-filename')
8+
9+
// defines reusable errors
10+
const genericError = new Error('ERR')
11+
genericError.code = 'ERR'
12+
const missingFileError = new Error('ENOENT')
13+
missingFileError.code = 'ENOENT'
14+
const pathExistsError = new Error('EEXIST')
15+
pathExistsError.code = 'EEXIST'
16+
17+
// helpers
18+
const CACHE = require('./util/test-dir')(__filename)
19+
const filename = uniqueFilename(os.tmpdir())
20+
const getuid = process.getuid
21+
const patchesGetuid = (t) => {
22+
process.getuid = () => 0
23+
t.teardown(() => { process.getuid = getuid })
24+
}
25+
const getFixOwner = (opts) => requireInject('../lib/util/fix-owner', opts)
26+
27+
// chownr and chownr.fix error handling tests
28+
29+
test('attempt to chownr existing path', (t) => {
30+
patchesGetuid(t)
31+
const fixOwner = getFixOwner({
32+
chownr: function chownr (path, uid, gid, cb) {
33+
cb(missingFileError)
34+
},
35+
'infer-owner': () => Promise.resolve({})
36+
})
37+
38+
t.plan(1)
39+
return fixOwner.chownr(CACHE, filename)
40+
.then(res => {
41+
t.notOk(res, 'should not throw if path exists')
42+
})
43+
})
44+
45+
test('attempt to chownr unknown error', (t) => {
46+
patchesGetuid(t)
47+
const fixOwner = getFixOwner({
48+
chownr: function chownr (path, uid, gid, cb) {
49+
cb(genericError)
50+
},
51+
'infer-owner': () => Promise.resolve({})
52+
})
53+
54+
t.plan(1)
55+
t.rejects(() => fixOwner.chownr(CACHE, filename), 'should throw unknown errors')
56+
})
57+
58+
test('attempt to chownr using same user', (t) => {
59+
patchesGetuid(t)
60+
const fixOwner = getFixOwner({
61+
'infer-owner': () => Promise.resolve({
62+
uid: process.getuid(),
63+
gid: process.getgid()
64+
})
65+
})
66+
67+
t.plan(1)
68+
return fixOwner.chownr(CACHE, filename)
69+
.then(res => {
70+
t.notOk(res, 'should not throw')
71+
})
72+
})
73+
74+
test('calls setuid setgid to replace user', (t) => {
75+
const setuid = process.setuid
76+
const setgid = process.setgid
77+
process.getuid = () => 0
78+
process.setuid = () => undefined
79+
process.setgid = () => undefined
80+
t.teardown(() => {
81+
process.getuid = getuid
82+
process.stuid = setuid
83+
process.stgid = setgid
84+
})
85+
const fixOwner = getFixOwner({
86+
'infer-owner': () => {
87+
process.setuid(process.getuid())
88+
process.setgid(process.getgid())
89+
return Promise.resolve({
90+
uid: process.getuid(),
91+
gid: process.getgid()
92+
})
93+
}
94+
})
95+
96+
t.plan(1)
97+
return fixOwner.chownr(CACHE, filename)
98+
.then(res => {
99+
t.notOk(res, 'should not throw')
100+
})
101+
})
102+
103+
test('attempt to chownr.sync on platforms that do not need ownership fix', (t) => {
104+
process.getuid = undefined
105+
t.teardown(() => { process.getuid = getuid })
106+
const fixOwner = require('../lib/util/fix-owner')
107+
108+
t.plan(1)
109+
return fixOwner.chownr(CACHE, filename)
110+
.then(res => {
111+
t.notOk(res, 'should not throw')
112+
})
113+
})
114+
115+
test('attempt to chownr.sync existing path', (t) => {
116+
patchesGetuid(t)
117+
function chownr () {}
118+
chownr.sync = () => {
119+
throw missingFileError
120+
}
121+
const fixOwner = getFixOwner({
122+
chownr,
123+
'infer-owner': { sync: () => ({}) }
124+
})
125+
126+
t.notOk(fixOwner.chownr.sync(CACHE, filename), 'should not throw if path exists')
127+
t.end()
128+
})
129+
130+
test('attempt to chownr.sync unknown error', (t) => {
131+
patchesGetuid(t)
132+
function chownr () {}
133+
chownr.sync = () => {
134+
throw genericError
135+
}
136+
const fixOwner = getFixOwner({
137+
chownr,
138+
'infer-owner': { sync: () => ({}) }
139+
})
140+
141+
t.throws(() => fixOwner.chownr.sync(CACHE, filename), genericError, 'should throw unknown errors')
142+
t.end()
143+
})
144+
145+
test('attempt to chownr.sync using same user', (t) => {
146+
patchesGetuid(t)
147+
const fixOwner = getFixOwner({
148+
'infer-owner': {
149+
sync: () => ({
150+
uid: process.getuid(),
151+
gid: process.getgid()
152+
})
153+
}
154+
})
155+
156+
t.notOk(fixOwner.chownr.sync(CACHE, filename), 'should not throw')
157+
t.end()
158+
})
159+
160+
test('attempt to chownr.sync on platforms that do not need ownership fix', (t) => {
161+
process.getuid = undefined
162+
t.teardown(() => { process.getuid = getuid })
163+
const fixOwner = require('../lib/util/fix-owner')
164+
165+
t.notOk(fixOwner.chownr.sync(CACHE, filename), 'should not throw')
166+
t.end()
167+
})
168+
169+
test('uses infer-owner ids instead of process-retrieved if valid', (t) => {
170+
const getgid = process.getgid
171+
process.getuid = () => 0
172+
process.getgid = () => 1
173+
t.teardown(() => {
174+
process.getuid = getuid
175+
process.getgid = getgid
176+
})
177+
t.plan(3)
178+
function chownr () {}
179+
chownr.sync = (path, uid, gid) => {
180+
t.equal(path, filename, 'should match filename')
181+
t.equal(uid, 501, 'should match uid')
182+
t.equal(gid, 20, 'should match gid')
183+
}
184+
const fixOwner = getFixOwner({
185+
chownr,
186+
'infer-owner': {
187+
sync: () => ({
188+
uid: 501,
189+
gid: 20
190+
})
191+
}
192+
})
193+
194+
fixOwner.chownr.sync(CACHE, filename)
195+
})
196+
197+
// mkdirfix and mkdirfix.sync error handling tests
198+
199+
test('attempt to mkdirfix existing path', (t) => {
200+
const fixOwner = getFixOwner({
201+
mkdirp: function mkdirp (path, cb) {
202+
cb(pathExistsError)
203+
}
204+
})
205+
206+
t.plan(1)
207+
return fixOwner.mkdirfix(CACHE, filename)
208+
.then(res => {
209+
t.notOk(res, 'should not throw if path exists')
210+
})
211+
})
212+
213+
test('attempt to mkdirfix unknown error', (t) => {
214+
const fixOwner = getFixOwner({
215+
mkdirp: function mkdirp (path, cb) {
216+
cb(genericError)
217+
}
218+
})
219+
220+
t.plan(1)
221+
t.rejects(() => fixOwner.mkdirfix(CACHE, filename), 'should throw unknown errors')
222+
})
223+
224+
test('attempt to mkdirfix.sync existing path', (t) => {
225+
function mkdirp () {}
226+
mkdirp.sync = () => {
227+
throw pathExistsError
228+
}
229+
const fixOwner = getFixOwner({ mkdirp })
230+
231+
t.notOk(fixOwner.mkdirfix.sync(CACHE, filename), 'should not throw if path exists')
232+
t.end()
233+
})
234+
235+
test('attempt to mkdirfix.sync unknown error', (t) => {
236+
function mkdirp () {}
237+
mkdirp.sync = () => {
238+
throw genericError
239+
}
240+
const fixOwner = getFixOwner({ mkdirp })
241+
242+
t.throws(() => fixOwner.mkdirfix.sync(CACHE, filename), genericError, 'should throw unknown errors')
243+
t.end()
244+
})
245+
246+
test('attempt to mkdirfix.sync but no dir created', (t) => {
247+
function mkdirp () {}
248+
mkdirp.sync = () => {}
249+
const fixOwner = getFixOwner({ mkdirp })
250+
251+
t.notOk(fixOwner.mkdirfix.sync(CACHE, filename), 'should not throw')
252+
t.end()
253+
})

0 commit comments

Comments
 (0)
Failed to load comments.