fs,win: fix bug in paths with trailing slashes

lib/fs.js

@@ -384,7 +384,11 @@ function readFile(path, options, callback) {
   const req = new FSReqCallback();
   req.context = context;
   req.oncomplete = readFileAfterOpen;
-  binding.open(getValidatedPath(path), flagsNumber, 0o666, req);
+  binding.open(
+    getValidatedPath(path, 'path', { expectFile: true, syscall: 'read' }),
+    flagsNumber,
+    0o666,
+    req);
 }
 
 function tryStatSync(fd, isUserFd) {
@@ -436,7 +440,9 @@ function readFileSync(path, options) {
 
   if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
     if (!isInt32(path)) {
-      path = getValidatedPath(path);
+      path = getValidatedPath(path,
+                              'path',
+                              { expectFile: true, syscall: 'read' });
     }
     return binding.readFileUtf8(path, stringToFlags(options.flag));
   }
@@ -530,7 +536,7 @@ function closeSync(fd) {
  * @returns {void}
  */
 function open(path, flags, mode, callback) {
-  path = getValidatedPath(path);
+  path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' });
   if (arguments.length < 3) {
     callback = flags;
     flags = 'r';
@@ -559,7 +565,7 @@ function open(path, flags, mode, callback) {
  */
 function openSync(path, flags, mode) {
   return binding.open(
-    getValidatedPath(path),
+    getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' }),
     stringToFlags(flags),
     parseFileMode(mode, 'mode', 0o666),
   );
@@ -2336,7 +2342,9 @@ function writeFileSync(path, data, options) {
   // C++ fast path for string data and UTF8 encoding
   if (typeof data === 'string' && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
     if (!isInt32(path)) {
-      path = getValidatedPath(path);
+      path = getValidatedPath(path,
+                              'path',
+                              { expectFile: true, syscall: 'write' });
     }
 
     return binding.writeFileUtf8(
@@ -2981,8 +2989,8 @@ function copyFile(src, dest, mode, callback) {
     mode = 0;
   }
 
-  src = getValidatedPath(src, 'src');
-  dest = getValidatedPath(dest, 'dest');
+  src = getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' });
+  dest = getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' });
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -3000,8 +3008,8 @@ function copyFile(src, dest, mode, callback) {
  */
 function copyFileSync(src, dest, mode) {
   binding.copyFile(
-    getValidatedPath(src, 'src'),
-    getValidatedPath(dest, 'dest'),
+    getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' }),
+    getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }),
     mode,
   );
 }

lib/internal/fs/promises.js

@@ -620,8 +620,8 @@ async function cp(src, dest, options) {
 async function copyFile(src, dest, mode) {
   return await PromisePrototypeThen(
     binding.copyFile(
-      getValidatedPath(src, 'src'),
-      getValidatedPath(dest, 'dest'),
+      getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' }),
+      getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }),
       mode,
       kUsePromises,
     ),
@@ -633,7 +633,7 @@ async function copyFile(src, dest, mode) {
 // Note that unlike fs.open() which uses numeric file descriptors,
 // fsPromises.open() uses the fs.FileHandle class.
 async function open(path, flags, mode) {
-  path = getValidatedPath(path);
+  path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' });
   const flagsNumber = stringToFlags(flags);
   mode = parseFileMode(mode, 'mode', 0o666);
   return new FileHandle(await PromisePrototypeThen(

lib/internal/fs/streams.js

@@ -182,7 +182,7 @@ function ReadStream(path, options) {
     this.flags = options.flags === undefined ? 'r' : options.flags;
     this.mode = options.mode === undefined ? 0o666 : options.mode;
 
-    validatePath(this.path);
+    validatePath(this.path, 'path', { expectFile: true, syscall: 'read' });
   } else {
     this.fd = getValidatedFd(importFd(this, options));
   }
@@ -337,7 +337,7 @@ function WriteStream(path, options) {
     this.flags = options.flags === undefined ? 'w' : options.flags;
     this.mode = options.mode === undefined ? 0o666 : options.mode;
 
-    validatePath(this.path);
+    validatePath(this.path, 'path', { expectFile: true, syscall: 'write' });
   } else {
     this.fd = getValidatedFd(importFd(this, options));
   }

lib/internal/fs/utils.js

@@ -709,13 +709,38 @@ const validateOffsetLengthWrite = hideStackFrames(
   },
 );
 
-const validatePath = hideStackFrames((path, propName = 'path') => {
+/**
+ * Validates the given path based on the expected type (file or directory).
+ * @param {string} path - The path to validate.
+ * @param {string} [propName='path'] - The name of the property being validated.
+ * @param {object} options - Additional options for validation.
+ * @param {boolean} [options.expectFile] - If true, expects the path to be a file.
+ * @param {string} [options.syscall] - The name of the syscall.
+ * @throws {TypeError} Throws if the path is not a string, Uint8Array, or URL without null bytes.
+ * @throws {Error} Throws if the path does not meet the expected type (file).
+ */
+const validatePath = hideStackFrames((path, propName = 'path', options) => {
   if (typeof path !== 'string' && !isUint8Array(path)) {
     throw new ERR_INVALID_ARG_TYPE.HideStackFramesError(propName, ['string', 'Buffer', 'URL'], path);
   }
 
   const pathIsString = typeof path === 'string';
   const pathIsUint8Array = isUint8Array(path);
+  if (options && options.expectFile) {
+    const lastCharacter = path[path.length - 1];
+    if (
+      lastCharacter === '/' || lastCharacter === 47 ||
+      (isWindows && (lastCharacter === '\\' || lastCharacter === 92))
+    ) {
+      throw new ERR_FS_EISDIR({
+        code: 'ERR_FS_EISDIR',
+        message: 'is a directory',
+        path,
+        syscall: options.syscall,
+        errno: ERR_FS_EISDIR,
+      });
+    }
+  }
 
   // We can only perform meaningful checks on strings and Uint8Arrays.
   if ((!pathIsString && !pathIsUint8Array) ||
@@ -731,11 +756,21 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
   );
 });
 
-const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
-  const path = toPathIfFileURL(fileURLOrPath);
-  validatePath(path, propName);
-  return path;
-});
+/**
+ * Validates and returns the given file URL or path based on the expected type (file or directory).
+ * @param {string|URL} fileURLOrPath - The file URL or path to validate.
+ * @param {string} [propName='path'] - The name of the property being validated.
+ * @param {object} options - Additional options for validation.
+ * @param {boolean} [options.expectFile] - If true, expects the path to be a file.
+ * @param {string} [options.syscall] - The name of the syscall.
+ * @returns {string} The validated path.
+ */
+const getValidatedPath =
+  hideStackFrames((fileURLOrPath, propName = 'path', options) => {
+    const path = toPathIfFileURL(fileURLOrPath);
+    validatePath(path, propName, options);
+    return path;
+  });
 
 const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
   if (ObjectIs(fd, -0)) {

test/sequential/test-fs-path-dir.js

@@ -0,0 +1,174 @@
+'use strict';
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const path = require('path');
+const fs = require('fs');
+const URL = require('url').URL;
+
+tmpdir.refresh();
+
+let fileCounter = 1;
+const nextFile = () => path.join(tmpdir.path, `file${fileCounter++}`);
+
+const generateStringPath = (file, suffix = '') => file + suffix;
+
+const generateURLPath = (file, suffix = '') =>
+  new URL('file://' + path.resolve(file) + suffix);
+
+const generateUint8ArrayPath = (file, suffix = '') =>
+  new Uint8Array(Buffer.from(file + suffix));
+
+const generatePaths = (file, suffix = '') => [
+  generateStringPath(file, suffix),
+  generateURLPath(file, suffix),
+  generateUint8ArrayPath(file, suffix),
+];
+
+const generateNewPaths = (suffix = '') => [
+  generateStringPath(nextFile(), suffix),
+  generateURLPath(nextFile(), suffix),
+  generateUint8ArrayPath(nextFile(), suffix),
+];
+
+function checkSyncFn(syncFn, p, args, fail) {
+  const result = fail ? 'must fail' : 'must succeed';
+  const failMsg = `failed testing sync ${p} ${syncFn.name} ${result}`;
+  if (!fail) {
+    try {
+      syncFn(p, ...args);
+    } catch (e) {
+      console.log(failMsg, e);
+      throw e;
+    }
+  } else {
+    assert.throws(() => syncFn(p, ...args), {
+      code: 'ERR_FS_EISDIR',
+    }, failMsg);
+  }
+}
+
+function checkAsyncFn(asyncFn, p, args, fail) {
+  const result = fail ? 'must fail' : 'must succeed';
+  const failMsg = `failed testing async ${p} ${asyncFn.name} ${result}`;
+  if (!fail) {
+    try {
+      asyncFn(p, ...args, common.mustCall());
+    } catch (e) {
+      console.log(failMsg, e);
+      throw e;
+    }
+  } else {
+    assert.throws(
+      () => asyncFn(p, ...args, common.mustNotCall()), {
+        code: 'ERR_FS_EISDIR',
+      },
+      failMsg
+    );
+  }
+}
+
+function checkPromiseFn(promiseFn, p, args, fail) {
+  const result = fail ? 'must fail' : 'must succeed';
+  const failMsg = `failed testing promise ${p} ${promiseFn.name} ${result}`;
+  if (!fail) {
+    const r = promiseFn(p, ...args).catch((err) => {
+      console.log(failMsg, err);
+      throw err;
+    });
+    if (r && r.close) r.close();
+  } else {
+    assert.rejects(
+      promiseFn(p, ...args), {
+        code: 'ERR_FS_EISDIR',
+      },
+      failMsg
+    ).then(common.mustCall());
+  }
+}
+
+function check(asyncFn, syncFn, promiseFn,
+               { suffix, fail, args = [] }) {
+  const file = nextFile();
+  fs.writeFile(file, 'data', (e) => {
+    assert.ifError(e);
+    const paths = generatePaths(file, suffix);
+    for (const p of paths) {
+      if (asyncFn) checkAsyncFn(asyncFn, p, args, fail);
+      if (promiseFn) checkPromiseFn(promiseFn, p, args, fail);
+      if (syncFn) checkSyncFn(syncFn, p, args, fail);
+    }
+  });
+}
+
+function checkManyToMany(asyncFn, syncFn, promiseFn,
+                         { suffix, fail, args = [] }) {
+  const source = nextFile();
+  fs.writeFile(source, 'data', (err) => {
+    assert.ifError(err);
+    if (asyncFn) {
+      generateNewPaths(suffix)
+        .forEach((p) => checkAsyncFn(asyncFn, source, [p, ...args], fail));
+    }
+    if (promiseFn) {
+      generateNewPaths(suffix)
+        .forEach((p) => checkPromiseFn(promiseFn, source, [p, ...args], fail));
+    }
+    if (syncFn) {
+      generateNewPaths(suffix)
+        .forEach((p) => checkSyncFn(syncFn, source, [p, ...args], fail));
+    }
+  });
+}
+check(fs.readFile, fs.readFileSync, fs.promises.readFile,
+      { suffix: '', fail: false });
+check(fs.readFile, fs.readFileSync, fs.promises.readFile,
+      { suffix: '/', fail: true });
+check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
+      { suffix: '', fail: false, args: ['data'] });
+check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
+      { suffix: '/', fail: true, args: ['data'] });
+check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
+      { suffix: '', fail: false, args: ['data'] });
+check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
+      { suffix: '/', fail: true, args: ['data'] });
+check(undefined, fs.createReadStream, undefined,
+      { suffix: '', fail: false });
+check(undefined, fs.createReadStream, undefined,
+      { suffix: '/', fail: true });
+check(undefined, fs.createWriteStream, undefined,
+      { suffix: '', fail: false });
+check(undefined, fs.createWriteStream, undefined,
+      { suffix: '/', fail: true });
+check(fs.truncate, fs.truncateSync, fs.promises.truncate,
+      { suffix: '', fail: false, args: [42] });
+check(fs.truncate, fs.truncateSync, fs.promises.truncate,
+      { suffix: '/', fail: true, args: [42] });
+
+check(fs.open, fs.openSync, fs.promises.open, { suffix: '', fail: false });
+check(fs.open, fs.openSync, fs.promises.open, { suffix: '/', fail: true });
+
+checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
+                { suffix: '', fail: false });
+
+checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
+                { suffix: '/', fail: true });
+
+if (common.isWindows) {
+  check(fs.readFile, fs.readFileSync, fs.promises.readFile,
+        { suffix: '\\', fail: true });
+  check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
+        { suffix: '\\', fail: true, args: ['data'] });
+  check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
+        { suffix: '\\', fail: true, args: ['data'] });
+  check(undefined, fs.createReadStream, undefined,
+        { suffix: '\\', fail: true });
+  check(undefined, fs.createWriteStream, undefined,
+        { suffix: '\\', fail: true });
+  check(fs.truncate, fs.truncateSync, fs.promises.truncate,
+        { suffix: '\\', fail: true, args: [42] });
+  check(fs.open, fs.openSync, fs.promises.open, { suffix: '\\', fail: true });
+  checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
+                  { suffix: '\\', fail: true });
+}