Make recursive rmdir more strict

doc/api/errors.md

@@ -928,6 +928,11 @@ added: v14.0.0
 Used when a feature that is not available
 to the current platform which is running Node.js is used.
 
+<a id="ERR_FS_ENOTDIR"></a>
+### `ERR_FS_ENOTDIR`
+
+Path is not a directory.
+
 <a id="ERR_FS_FILE_TOO_LARGE"></a>
 ### `ERR_FS_FILE_TOO_LARGE`
 

doc/api/fs.md

@@ -3518,8 +3518,7 @@ changes:
     the number of retries. This option is ignored if the `recursive` option is
     not `true`. **Default:** `0`.
   * `recursive` {boolean} If `true`, perform a recursive directory removal. In
-    recursive mode, errors are not reported if `path` does not exist, and
-    operations are retried on failure. **Default:** `false`.
+    recursive mode operations are retried on failure. **Default:** `false`.
   * `retryDelay` {integer} The amount of time in milliseconds to wait between
     retries. This option is ignored if the `recursive` option is not `true`.
     **Default:** `100`.
@@ -3565,8 +3564,7 @@ changes:
     the number of retries. This option is ignored if the `recursive` option is
     not `true`. **Default:** `0`.
   * `recursive` {boolean} If `true`, perform a recursive directory removal. In
-    recursive mode, errors are not reported if `path` does not exist, and
-    operations are retried on failure. **Default:** `false`.
+    recursive mode operations are retried on failure. **Default:** `false`.
   * `retryDelay` {integer} The amount of time in milliseconds to wait between
     retries. This option is ignored if the `recursive` option is not `true`.
     **Default:** `100`.

lib/internal/errors.js

@@ -838,6 +838,7 @@ E('ERR_FEATURE_UNAVAILABLE_ON_PLATFORM',
   'The feature %s is unavailable on the current platform' +
   ', which is being used to run Node.js',
   TypeError);
+E('ERR_FS_ENOTDIR', 'not a directory', SystemError);
 E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than 2 GB', RangeError);
 E('ERR_FS_INVALID_SYMLINK_TYPE',
   'Symlink type must be one of "dir", "file", or "junction". Received "%s"',

lib/internal/fs/rimraf.js

@@ -29,6 +29,11 @@ const {
 const { sep } = require('path');
 const { setTimeout } = require('timers');
 const { sleep } = require('internal/util');
+const {
+  codes: {
+    ERR_FS_ENOTDIR
+  },
+} = require('internal/errors');
 const notEmptyErrorCodes = new Set(['ENOTEMPTY', 'EEXIST', 'EPERM']);
 const retryErrorCodes = new Set(
   ['EBUSY', 'EMFILE', 'ENFILE', 'ENOTEMPTY', 'EPERM']);
@@ -40,14 +45,33 @@ const separator = Buffer.from(sep);
 
 
 function rimraf(path, options, callback) {
+  stat(path, (err, stats) => {
+    if (err && err.code === 'ENOENT') {
+      callback(err);
+    } else if (stats && !stats.isDirectory()) {
+      callback(new ERR_FS_ENOTDIR({
+        code: 'ENOTDIR',
+        message: 'not a directory',
+        path,
+        syscall: 'rmdir',
+        errno: -20
+      }));
+    } else {
+      _rimraf(path, options, callback);
+    }
+  });
+}
+
+
+function _rimraf(path, options, callback) {
   let retries = 0;
 
-  _rimraf(path, options, function CB(err) {
+  __rimraf(path, options, function CB(err) {
     if (err) {
       if (retryErrorCodes.has(err.code) && retries < options.maxRetries) {
         retries++;
         const delay = retries * options.retryDelay;
-        return setTimeout(_rimraf, delay, path, options, CB);
+        return setTimeout(__rimraf, delay, path, options, CB);
       }
 
       // The file is already gone.
@@ -60,7 +84,7 @@ function rimraf(path, options, callback) {
 }
 
 
-function _rimraf(path, options, callback) {
+function __rimraf(path, options, callback) {
   // SunOS lets the root user unlink directories. Use lstat here to make sure
   // it's not a directory.
   lstat(path, (err, stats) => {
@@ -141,7 +165,7 @@ function _rmchildren(path, options, callback) {
     files.forEach((child) => {
       const childPath = Buffer.concat([pathBuf, separator, child]);
 
-      rimraf(childPath, options, (err) => {
+      _rimraf(childPath, options, (err) => {
         if (done)
           return;
 
@@ -174,6 +198,30 @@ function rimrafPromises(path, options) {
 function rimrafSync(path, options) {
   let stats;
 
+  try {
+    stats = statSync(path);
+  } catch (err) {
+    if (err.code === 'ENOENT')
+      throw err;
+  }
+
+  if (stats && !stats.isDirectory()) {
+    throw new ERR_FS_ENOTDIR({
+      code: 'ENOTDIR',
+      message: 'not a directory',
+      path,
+      syscall: 'rmdir',
+      errno: -20
+    });
+  }
+
+  _rimrafSync(path, options);
+}
+
+
+function _rimrafSync(path, options) {
+  let stats;
+
   try {
     stats = lstatSync(path);
   } catch (err) {
@@ -242,7 +290,7 @@ function _rmdirSync(path, options, originalErr) {
       readdirSync(pathBuf, readdirEncoding).forEach((child) => {
         const childPath = Buffer.concat([pathBuf, separator, child]);
 
-        rimrafSync(childPath, options);
+        _rimrafSync(childPath, options);
       });
 
       const tries = options.maxRetries + 1;

test/common/tmpdir.js

@@ -6,7 +6,11 @@ const path = require('path');
 const { isMainThread } = require('worker_threads');
 
 function rimrafSync(pathname) {
-  fs.rmdirSync(pathname, { maxRetries: 3, recursive: true });
+  try {
+    fs.rmdirSync(pathname, { maxRetries: 3, recursive: true });
+  } catch {
+    // do nothing
+  }
 }
 
 const testRoot = process.env.NODE_TEST_DIR ?

test/parallel/test-fs-rmdir-recursive.js

@@ -75,14 +75,9 @@ function removeAsync(dir) {
       fs.rmdir(dir, { recursive: true }, common.mustCall((err) => {
         assert.ifError(err);
 
-        // No error should occur if recursive and the directory does not exist.
-        fs.rmdir(dir, { recursive: true }, common.mustCall((err) => {
-          assert.ifError(err);
-
-          // Attempted removal should fail now because the directory is gone.
-          fs.rmdir(dir, common.mustCall((err) => {
-            assert.strictEqual(err.syscall, 'rmdir');
-          }));
+        // Attempted removal should fail now because the directory is gone.
+        fs.rmdir(dir, common.mustCall((err) => {
+          assert.strictEqual(err.syscall, 'rmdir');
         }));
       }));
     }));
@@ -105,6 +100,33 @@ function removeAsync(dir) {
   dir = nextDirPath();
   makeNonEmptyDirectory(1, 10, 2, dir, true);
   removeAsync(dir);
+
+  // Should fail if target does not exist
+  fs.rmdir(
+    path.join(tmpdir.path, 'noexist.txt'),
+    { recursive: true },
+    common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ENOENT');
+    })
+  );
+
+  // Should fail if target is a file
+  const filePath = path.join(tmpdir.path, 'rmdir-async-file.txt');
+  fs.writeFileSync(filePath, '');
+  fs.rmdir(filePath, { recursive: true }, common.mustCall((err) => {
+    try {
+      assert.strictEqual(err.code, 'ERR_FS_ENOTDIR');
+      assert.strictEqual(err.name, 'SystemError');
+      assert.match(err.message, /^not a directory/);
+      assert.strictEqual(err.info.code, 'ENOTDIR');
+      assert.strictEqual(err.info.message, 'not a directory');
+      assert.strictEqual(err.info.path, filePath);
+      assert.strictEqual(err.info.syscall, 'rmdir');
+      assert.strictEqual(err.info.errno, -20);
+    } finally {
+      fs.unlinkSync(filePath);
+    }
+  }));
 }
 
 // Test the synchronous version.
@@ -120,10 +142,40 @@ function removeAsync(dir) {
     fs.rmdirSync(dir, { recursive: false });
   }, { syscall: 'rmdir' });
 
-  // Recursive removal should succeed.
-  fs.rmdirSync(dir, { recursive: true });
+  // Should fail if target does not exist
+  assert.throws(() => {
+    fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'), { recursive: true });
+  }, {
+    code: 'ENOENT',
+    name: 'Error',
+    message: /^ENOENT: no such file or directory, stat/
+  });
+
+  // Should fail if target is a file
+  const filePath = path.join(tmpdir.path, 'rmdir-sync-file.txt');
+  fs.writeFileSync(filePath, '');
+
+  try {
+    assert.throws(() => {
+      fs.rmdirSync(filePath, { recursive: true });
+    }, {
+      code: 'ERR_FS_ENOTDIR',
+      name: 'SystemError',
+      message: /^not a directory/,
+      info: {
+        code: 'ENOTDIR',
+        message: 'not a directory',
+        path: filePath,
+        syscall: 'rmdir',
+        errno: -20
+      }
+    });
+  } finally {
+    fs.unlinkSync(filePath);
+  }
+
 
-  // No error should occur if recursive and the directory does not exist.
+  // Recursive removal should succeed.
   fs.rmdirSync(dir, { recursive: true });
 
   // Attempted removal should fail now because the directory is gone.
@@ -144,8 +196,39 @@ function removeAsync(dir) {
   // Recursive removal should succeed.
   await fs.promises.rmdir(dir, { recursive: true });
 
-  // No error should occur if recursive and the directory does not exist.
-  await fs.promises.rmdir(dir, { recursive: true });
+  // Should fail if target does not exist
+  assert.rejects(fs.promises.rmdir(
+    path.join(tmpdir.path, 'noexist.txt'),
+    { recursive: true }
+  ), {
+    code: 'ENOENT',
+    name: 'Error',
+    message: /^ENOENT: no such file or directory, stat/
+  });
+
+  // Should fail if target is a file
+  const filePath = path.join(tmpdir.path, 'rmdir-promises-file.txt');
+  fs.writeFileSync(filePath, '');
+
+  try {
+    await assert.rejects(fs.promises.rmdir(
+      filePath,
+      { recursive: true }
+    ), {
+      code: 'ERR_FS_ENOTDIR',
+      name: 'SystemError',
+      message: /^not a directory/,
+      info: {
+        code: 'ENOTDIR',
+        message: 'not a directory',
+        path: filePath,
+        syscall: 'rmdir',
+        errno: -20
+      }
+    });
+  } finally {
+    fs.unlinkSync(filePath);
+  }
 
   // Attempted removal should fail now because the directory is gone.
   assert.rejects(fs.promises.rmdir(dir), { syscall: 'rmdir' });

test/parallel/test-policy-parse-integrity.js

@@ -20,7 +20,13 @@ function hash(algo, body) {
 }
 
 const tmpdirPath = path.join(tmpdir.path, 'test-policy-parse-integrity');
-fs.rmdirSync(tmpdirPath, { maxRetries: 3, recursive: true });
+
+try {
+  fs.rmdirSync(tmpdirPath, { maxRetries: 3, recursive: true });
+} catch {
+  // do nothing
+}
+
 fs.mkdirSync(tmpdirPath, { recursive: true });
 
 const policyFilepath = path.join(tmpdirPath, 'policy');