Skip to content

Commit 8eb33dc

Browse files
phatedphated
committed
Fix: Use explicit chmod to ensure setgid permission can be set & account for umask with default mode (closes #183, closes #185)
1 parent 79c7216 commit 8eb33dc

File tree

3 files changed

+78
-74
lines changed

3 files changed

+78
-74
lines changed

‎lib/file-operations.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,19 +201,19 @@ function mkdirp(dirpath, customMode, callback) {
201201
customMode = undefined;
202202
}
203203

204-
var mode = customMode || constants.DEFAULT_DIR_MODE;
204+
var mode = customMode || (constants.DEFAULT_DIR_MODE & ~process.umask());
205205
dirpath = path.resolve(dirpath);
206206

207207
fs.mkdir(dirpath, mode, onMkdir);
208208

209209
function onMkdir(mkdirErr) {
210210
if (!mkdirErr) {
211-
return callback();
211+
return fs.stat(dirpath, onStat);
212212
}
213213

214214
switch (mkdirErr.code) {
215215
case 'ENOENT': {
216-
return mkdirp(path.dirname(dirpath), mode, onRecurse);
216+
return mkdirp(path.dirname(dirpath), onRecurse);
217217
}
218218

219219
case 'EEXIST': {
@@ -234,11 +234,15 @@ function mkdirp(dirpath, customMode, callback) {
234234
return callback(mkdirErr);
235235
}
236236

237+
if (stats.mode === mode) {
238+
return callback();
239+
}
240+
237241
if (!customMode) {
238242
return callback();
239243
}
240244

241-
fs.chmod(dirpath, customMode, callback);
245+
fs.chmod(dirpath, mode, callback);
242246
}
243247
}
244248

‎test/dest-modes.js

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -336,12 +336,7 @@ describe('.dest() with custom modes', function() {
336336
var inputPath = path.join(__dirname, './fixtures/wow/suchempty');
337337
var expectedBase = path.join(__dirname, './out-fixtures/wow');
338338
var expectedPath = path.join(__dirname, './out-fixtures/wow/suchempty');
339-
// NOTE: Darwin does not set setgid
340-
var expectedDirMode = constants.DEFAULT_DIR_MODE;
341-
if (!isDarwin) {
342-
expectedDirMode |= parseInt('2000', 8);
343-
}
344-
expectedDirMode &= ~process.umask();
339+
var expectedDirMode = parseInt('2777', 8) & ~process.umask();
345340
var expectedFileMode = constants.DEFAULT_FILE_MODE & ~process.umask();
346341

347342
var firstFile = new File({

‎test/file-operations.js

Lines changed: 69 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ var buffer = require('buffer');
1111
var defaultResolution = require('default-resolution');
1212

1313
var fo = require('../lib/file-operations');
14+
var constants = require('../lib/constants');
1415

1516
var mkdirp = fo.mkdirp;
1617
var closeFd = fo.closeFd;
@@ -22,10 +23,15 @@ var updateMetadata = fo.updateMetadata;
2223

2324
var resolution = defaultResolution();
2425

25-
var MASK_MODE = parseInt('7777', 8);
26+
var DEFAULT_DIR_MODE = masked(constants.DEFAULT_DIR_MODE & ~process.umask()).toString(8);
2627

2728
function masked(mode) {
28-
return mode & MASK_MODE;
29+
return mode & constants.MASK_MODE;
30+
}
31+
32+
function statHuman(filepath) {
33+
var stats = fs.lstatSync(filepath);
34+
return masked(stats.mode).toString(8);
2935
}
3036

3137
function noop() {}
@@ -914,10 +920,15 @@ describe('updateMetadata', function() {
914920
});
915921

916922
describe('mkdirp', function() {
917-
var DEFAULT_DIR_MODE = parseInt('0777', 8);
918-
var MODE_MASK = parseInt('0777', 8);
919923

920-
var dir = path.join(__dirname, './fixtures/bar');
924+
var fixtures = path.join(__dirname, './fixtures');
925+
var dir = path.join(fixtures, './bar');
926+
927+
beforeEach(function(done) {
928+
// Linux inherits the setgid of the directory and it messes up our assertions
929+
// So we explixitly set the mode to 777 before each test
930+
fs.chmod(fixtures, '777', done);
931+
});
921932

922933
afterEach(function() {
923934
return del(dir);
@@ -926,49 +937,37 @@ describe('mkdirp', function() {
926937
it('makes a single directory', function(done) {
927938
mkdirp(dir, function(err) {
928939
expect(err).toNotExist();
940+
expect(statHuman(dir)).toExist();
929941

930-
fs.stat(dir, function(err2, stats) {
931-
expect(err2).toNotExist();
932-
expect(stats).toExist();
933-
934-
done();
935-
});
942+
done();
936943
});
937944
});
938945

939-
it('makes single directory w/ correct permissions', function(done) {
946+
it('makes single directory w/ default mode', function(done) {
940947
if (isWindows) {
941948
this.skip();
942949
return;
943950
}
944951

945952
mkdirp(dir, function(err) {
946953
expect(err).toNotExist();
954+
expect(statHuman(dir)).toEqual(DEFAULT_DIR_MODE);
947955

948-
fs.stat(dir, function(err2, stats) {
949-
expect(err2).toNotExist();
950-
expect(stats.mode & MODE_MASK).toEqual(DEFAULT_DIR_MODE & ~process.umask());
951-
952-
done();
953-
});
956+
done();
954957
});
955958
});
956959

957960
it('makes multiple directories', function(done) {
958961
var nestedDir = path.join(dir, './baz/foo');
959962
mkdirp(nestedDir, function(err) {
960963
expect(err).toNotExist();
964+
expect(statHuman(nestedDir)).toExist();
961965

962-
fs.stat(nestedDir, function(err2, stats) {
963-
expect(err2).toNotExist();
964-
expect(stats).toExist();
965-
966-
done();
967-
});
966+
done();
968967
});
969968
});
970969

971-
it('makes multiple directories w/ correct permissions', function(done) {
970+
it('makes multiple directories w/ default mode', function(done) {
972971
if (isWindows) {
973972
this.skip();
974973
return;
@@ -977,13 +976,9 @@ describe('mkdirp', function() {
977976
var nestedDir = path.join(dir, './baz/foo');
978977
mkdirp(nestedDir, function(err) {
979978
expect(err).toNotExist();
979+
expect(statHuman(nestedDir)).toEqual(DEFAULT_DIR_MODE);
980980

981-
fs.stat(nestedDir, function(err2, stats) {
982-
expect(err2).toNotExist();
983-
expect(stats.mode & MODE_MASK).toEqual(DEFAULT_DIR_MODE & ~process.umask());
984-
985-
done();
986-
});
981+
done();
987982
});
988983
});
989984

@@ -996,13 +991,24 @@ describe('mkdirp', function() {
996991
var mode = parseInt('0700', 8);
997992
mkdirp(dir, mode, function(err) {
998993
expect(err).toNotExist();
994+
expect(statHuman(dir)).toEqual(masked(mode).toString(8));
999995

1000-
fs.stat(dir, function(err2, stats) {
1001-
expect(err2).toNotExist();
1002-
expect(stats.mode & MODE_MASK).toEqual(mode & ~process.umask());
996+
done();
997+
});
998+
});
1003999

1004-
done();
1005-
});
1000+
it('can create a directory with setgid permission', function(done) {
1001+
if (isWindows) {
1002+
this.skip();
1003+
return;
1004+
}
1005+
1006+
var mode = parseInt('2700', 8);
1007+
mkdirp(dir, mode, function(err) {
1008+
expect(err).toNotExist();
1009+
expect(statHuman(dir)).toEqual(masked(mode).toString(8));
1010+
1011+
done();
10061012
});
10071013
});
10081014

@@ -1018,13 +1024,9 @@ describe('mkdirp', function() {
10181024

10191025
mkdirp(dir, function(err2) {
10201026
expect(err2).toNotExist();
1027+
expect(statHuman(dir)).toEqual(masked(mode).toString(8));
10211028

1022-
fs.stat(dir, function(err3, stats) {
1023-
expect(err3).toNotExist();
1024-
expect(stats.mode & MODE_MASK).toEqual(mode & ~process.umask());
1025-
1026-
done();
1027-
});
1029+
done();
10281030
});
10291031
});
10301032
});
@@ -1039,13 +1041,27 @@ describe('mkdirp', function() {
10391041
var mode = parseInt('0700',8);
10401042
mkdirp(nestedDir, mode, function(err) {
10411043
expect(err).toNotExist();
1044+
expect(statHuman(nestedDir)).toEqual(masked(mode).toString(8));
10421045

1043-
fs.stat(nestedDir, function(err2, stats) {
1044-
expect(err2).toNotExist();
1045-
expect(stats.mode & MODE_MASK).toEqual(mode & ~process.umask());
1046+
done();
1047+
});
1048+
});
10461049

1047-
done();
1048-
});
1050+
it('uses default mode on intermediate directories', function(done) {
1051+
if (isWindows) {
1052+
this.skip();
1053+
return;
1054+
}
1055+
1056+
var intermediateDir = path.join(dir, './baz');
1057+
var nestedDir = path.join(intermediateDir, './foo');
1058+
var mode = parseInt('0700',8);
1059+
mkdirp(nestedDir, mode, function(err) {
1060+
expect(err).toNotExist();
1061+
expect(statHuman(dir)).toEqual(DEFAULT_DIR_MODE);
1062+
expect(statHuman(intermediateDir)).toEqual(DEFAULT_DIR_MODE);
1063+
1064+
done();
10491065
});
10501066
});
10511067

@@ -1058,21 +1074,13 @@ describe('mkdirp', function() {
10581074
var mode = parseInt('0700',8);
10591075
mkdirp(dir, function(err) {
10601076
expect(err).toNotExist();
1077+
expect(statHuman(dir)).toEqual(DEFAULT_DIR_MODE);
10611078

1062-
fs.stat(dir, function(err2, stats) {
1079+
mkdirp(dir, mode, function(err2) {
10631080
expect(err2).toNotExist();
1064-
expect(stats.mode & MODE_MASK).toEqual(DEFAULT_DIR_MODE & ~process.umask());
1065-
1066-
mkdirp(dir, mode, function(err3) {
1067-
expect(err3).toNotExist();
1081+
expect(statHuman(dir)).toEqual(masked(mode).toString(8));
10681082

1069-
fs.stat(dir, function(err4, stats) {
1070-
expect(err2).toNotExist();
1071-
expect(stats.mode & MODE_MASK).toEqual(mode & ~process.umask());
1072-
1073-
done();
1074-
});
1075-
});
1083+
done();
10761084
});
10771085
});
10781086
});
@@ -1109,14 +1117,11 @@ describe('mkdirp', function() {
11091117
fs.writeFile(file, 'hello world', function(err2) {
11101118
expect(err2).toNotExist();
11111119

1112-
var stats = fs.statSync(file);
1113-
var expectedMode = stats.mode & MODE_MASK;
1120+
var expectedMode = statHuman(file);
11141121

11151122
mkdirp(file, mode, function(err3) {
11161123
expect(err3).toExist();
1117-
1118-
var stats = fs.statSync(file);
1119-
expect(stats.mode & MODE_MASK).toEqual(expectedMode);
1124+
expect(statHuman(file)).toEqual(expectedMode);
11201125

11211126
done();
11221127
});

0 commit comments

Comments
 (0)