Skip to content

Commit 04fe79a

Browse files
committed
fs: validate the callback passed to fs.rm()
Every other callback based fs API runs its callback through makeCallback(), so a missing or non-function callback is reported synchronously as ERR_INVALID_ARG_TYPE. fs.rm() skipped that step, so the bad callback was not noticed until rimraf tried to invoke it, by which point the removal had already happened and the resulting "callback is not a function" TypeError was thrown from an async completion handler where it could not be caught. Refs: #65578 Signed-off-by: Paul Bouchon <mail@bitpshr.net>
1 parent 29c517f commit 04fe79a

2 files changed

Lines changed: 24 additions & 0 deletions

File tree

lib/fs.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1534,6 +1534,7 @@ function rm(path, options, callback) {
15341534
const h = vfsState.handlers;
15351535
if (h !== null && vfsVoid(h.rm(path, options), callback)) return;
15361536

1537+
callback = makeCallback(callback);
15371538
path = getValidatedPath(path);
15381539

15391540
validateRmOptions(path, options, false, (err, options) => {

test/parallel/test-fs-rm.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,29 @@ if (isGitPresent) {
485485
});
486486
}
487487

488+
// A missing callback is reported up front, the same way the other callback
489+
// based fs APIs report it, rather than throwing from the removal itself once
490+
// the work has already happened.
491+
// Refs: https://github.com/nodejs/node/issues/65578
492+
{
493+
const dirname = tmpdir.resolve('rm-missing-callback');
494+
fs.mkdirSync(dirname, { recursive: true });
495+
496+
for (const args of [
497+
[dirname],
498+
[dirname, { recursive: true }],
499+
[dirname, { recursive: true }, 'not a function'],
500+
]) {
501+
assert.throws(() => fs.rm(...args), {
502+
code: 'ERR_INVALID_ARG_TYPE',
503+
name: 'TypeError',
504+
});
505+
}
506+
507+
assert.ok(fs.existsSync(dirname), 'rm must not remove anything without a callback');
508+
fs.rmSync(dirname, { recursive: true });
509+
}
510+
488511
{
489512
// IBMi has a different access permission mechanism
490513
// This test should not be run as `root`

0 commit comments

Comments
 (0)