fs: remove permissive rmdir recursive · nodejs/node@9948036 · GitHub
Skip to content

Commit 9948036

Browse files
aduh95aduh95
committed
fs: remove permissive rmdir recursive
PR-URL: #37216 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ian Sutherland <ian@iansutherland.ca> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
1 parent 3cc9aec commit 9948036

10 files changed

Lines changed: 167 additions & 43 deletions

‎doc/api/fs.md‎

Lines changed: 45 additions & 18 deletions

‎lib/fs.js‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -898,15 +898,20 @@ function rmdir(path, options, callback) {
898898
emitRecursiveRmdirWarning();
899899
validateRmOptions(
900900
path,
901-
{ ...options, force: true },
901+
{ ...options, force: false },
902902
true,
903903
(err, options) => {
904+
if (err === false) {
905+
const req = new FSReqCallback();
906+
req.oncomplete = callback;
907+
return binding.rmdir(path, req);
908+
}
904909
if (err) {
905910
return callback(err);
906911
}
907912

908913
lazyLoadRimraf();
909-
return rimraf(path, options, callback);
914+
rimraf(path, options, callback);
910915
});
911916
} else {
912917
validateRmdirOptions(options);
@@ -921,12 +926,15 @@ function rmdirSync(path, options) {
921926

922927
if (options?.recursive) {
923928
emitRecursiveRmdirWarning();
924-
options = validateRmOptionsSync(path, { ...options, force: true }, true);
925-
lazyLoadRimraf();
926-
return rimrafSync(pathModule.toNamespacedPath(path), options);
929+
options = validateRmOptionsSync(path, { ...options, force: false }, true);
930+
if (options !== false) {
931+
lazyLoadRimraf();
932+
return rimrafSync(pathModule.toNamespacedPath(path), options);
933+
}
934+
} else {
935+
validateRmdirOptions(options);
927936
}
928937

929-
validateRmdirOptions(options);
930938
const ctx = { path };
931939
binding.rmdir(pathModule.toNamespacedPath(path), undefined, ctx);
932940
return handleErrorFromBinding(ctx);

‎lib/internal/fs/promises.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,10 @@ async function rmdir(path, options) {
505505

506506
if (options.recursive) {
507507
emitRecursiveRmdirWarning();
508-
return rimrafPromises(path, options);
508+
const stats = await stat(path);
509+
if (stats.isDirectory()) {
510+
return rimrafPromises(path, options);
511+
}
509512
}
510513

511514
return binding.rmdir(path, kUsePromises);

‎lib/internal/fs/utils.js‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -698,39 +698,47 @@ const defaultRmdirOptions = {
698698
recursive: false,
699699
};
700700

701-
const validateRmOptions = hideStackFrames((path, options, warn, callback) => {
701+
const validateRmOptions = hideStackFrames((path, options, expectDir, cb) => {
702702
options = validateRmdirOptions(options, defaultRmOptions);
703703
validateBoolean(options.force, 'options.force');
704704

705705
lazyLoadFs().stat(path, (err, stats) => {
706706
if (err) {
707707
if (options.force && err.code === 'ENOENT') {
708-
return callback(null, options);
708+
return cb(null, options);
709709
}
710-
return callback(err, options);
710+
return cb(err, options);
711+
}
712+
713+
if (expectDir && !stats.isDirectory()) {
714+
return cb(false);
711715
}
712716

713717
if (stats.isDirectory() && !options.recursive) {
714-
return callback(new ERR_FS_EISDIR({
718+
return cb(new ERR_FS_EISDIR({
715719
code: 'EISDIR',
716720
message: 'is a directory',
717721
path,
718722
syscall: 'rm',
719723
errno: EISDIR
720724
}));
721725
}
722-
return callback(null, options);
726+
return cb(null, options);
723727
});
724728
});
725729

726-
const validateRmOptionsSync = hideStackFrames((path, options, warn) => {
730+
const validateRmOptionsSync = hideStackFrames((path, options, expectDir) => {
727731
options = validateRmdirOptions(options, defaultRmOptions);
728732
validateBoolean(options.force, 'options.force');
729733

730-
if (!options.force || warn || !options.recursive) {
734+
if (!options.force || expectDir || !options.recursive) {
731735
const isDirectory = lazyLoadFs()
732736
.statSync(path, { throwIfNoEntry: !options.force })?.isDirectory();
733737

738+
if (expectDir && !isDirectory) {
739+
return false;
740+
}
741+
734742
if (isDirectory && !options.recursive) {
735743
throw new ERR_FS_EISDIR({
736744
code: 'EISDIR',

‎test/parallel/test-fs-rmdir-recursive-sync-warns-not-found.js‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
22
const common = require('../common');
33
const tmpdir = require('../common/tmpdir');
4+
const assert = require('assert');
45
const fs = require('fs');
56
const path = require('path');
67

@@ -14,5 +15,9 @@ tmpdir.refresh();
1415
'will be removed. Use fs.rm(path, { recursive: true }) instead',
1516
'DEP0147'
1617
);
17-
fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'), { recursive: true });
18+
assert.throws(
19+
() => fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'),
20+
{ recursive: true }),
21+
{ code: 'ENOENT' }
22+
);
1823
}

‎test/parallel/test-fs-rmdir-recursive-sync-warns-on-file.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
'use strict';
22
const common = require('../common');
33
const tmpdir = require('../common/tmpdir');
4+
const assert = require('assert');
45
const fs = require('fs');
56
const path = require('path');
67

78
tmpdir.refresh();
89

910
{
10-
// Should warn when trying to delete a file
1111
common.expectWarning(
1212
'DeprecationWarning',
1313
'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' +
@@ -16,5 +16,8 @@ tmpdir.refresh();
1616
);
1717
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
1818
fs.writeFileSync(filePath, '');
19-
fs.rmdirSync(filePath, { recursive: true });
19+
assert.throws(
20+
() => fs.rmdirSync(filePath, { recursive: true }),
21+
{ code: common.isWindows ? 'ENOENT' : 'ENOTDIR' }
22+
);
2023
}

‎test/parallel/test-fs-rmdir-recursive-throws-not-found.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
'use strict';
2+
const common = require('../common');
3+
const tmpdir = require('../common/tmpdir');
4+
const assert = require('assert');
5+
const fs = require('fs');
6+
const path = require('path');
7+
8+
tmpdir.refresh();
9+
10+
{
11+
assert.throws(
12+
() =>
13+
fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'), { recursive: true }),
14+
{
15+
code: 'ENOENT',
16+
}
17+
);
18+
}
19+
{
20+
fs.rmdir(
21+
path.join(tmpdir.path, 'noexist.txt'),
22+
{ recursive: true },
23+
common.mustCall((err) => {
24+
assert.strictEqual(err.code, 'ENOENT');
25+
})
26+
);
27+
}
28+
{
29+
assert.rejects(
30+
() => fs.promises.rmdir(path.join(tmpdir.path, 'noexist.txt'),
31+
{ recursive: true }),
32+
{
33+
code: 'ENOENT',
34+
}
35+
).then(common.mustCall());
36+
}

‎test/parallel/test-fs-rmdir-recursive-throws-on-file.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
'use strict';
2+
const common = require('../common');
3+
const tmpdir = require('../common/tmpdir');
4+
const assert = require('assert');
5+
const fs = require('fs');
6+
const path = require('path');
7+
8+
tmpdir.refresh();
9+
10+
const code = common.isWindows ? 'ENOENT' : 'ENOTDIR';
11+
12+
{
13+
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
14+
fs.writeFileSync(filePath, '');
15+
assert.throws(() => fs.rmdirSync(filePath, { recursive: true }), { code });
16+
}
17+
{
18+
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
19+
fs.writeFileSync(filePath, '');
20+
fs.rmdir(filePath, { recursive: true }, common.mustCall((err) => {
21+
assert.strictEqual(err.code, code);
22+
}));
23+
}
24+
{
25+
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
26+
fs.writeFileSync(filePath, '');
27+
assert.rejects(() => fs.promises.rmdir(filePath, { recursive: true }),
28+
{ code }).then(common.mustCall());
29+
}

‎test/parallel/test-fs-rmdir-recursive-warns-on-file.js‎

Lines changed: 4 additions & 2 deletions

0 commit comments

Comments
 (0)