Revert "fs,win: fix bug in paths with trailing slashes"

This reverts commit 00b2f07f9d. PR-URL: https://github.com/nodejs/node/pull/55527 Fixes: https://github.com/nodejs/node/issues/17801 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
2024-11-21 10:59:27 +00:00 · 2024-11-03 03:24:29 +11:00 · 2024-11-03 03:24:29 +11:00 · 7b01758ded
commit 7b01758ded
parent 68dc15e400
5 changed files with 20 additions and 237 deletions
--- a/lib/fs.js
+++ b/lib/fs.js
@ -385,11 +385,7 @@ function readFile(path, options, callback) {
  const req = new FSReqCallback();
  req.context = context;
  req.oncomplete = readFileAfterOpen;
-  binding.open(
-    getValidatedPath(path, 'path', { expectFile: true, syscall: 'read' }),
-    flagsNumber,
-    0o666,
-    req);
+  binding.open(getValidatedPath(path), flagsNumber, 0o666, req);
 }

 function tryStatSync(fd, isUserFd) {
@ -441,9 +437,7 @@ function readFileSync(path, options) {

  if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
    if (!isInt32(path)) {
-      path = getValidatedPath(path,
-                              'path',
-                              { expectFile: true, syscall: 'read' });
+      path = getValidatedPath(path);
    }
    return binding.readFileUtf8(path, stringToFlags(options.flag));
  }
@ -537,7 +531,7 @@ function closeSync(fd) {
 * @returns {void}
 */
 function open(path, flags, mode, callback) {
-  path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' });
+  path = getValidatedPath(path);
  if (arguments.length < 3) {
    callback = flags;
    flags = 'r';
@ -566,7 +560,7 @@ function open(path, flags, mode, callback) {
 */
 function openSync(path, flags, mode) {
  return binding.open(
-    getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' }),
+    getValidatedPath(path),
    stringToFlags(flags),
    parseFileMode(mode, 'mode', 0o666),
  );
@ -2345,9 +2339,7 @@ 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',
-                              { expectFile: true, syscall: 'write' });
+      path = getValidatedPath(path);
    }

    return binding.writeFileUtf8(
@ -2992,8 +2984,8 @@ function copyFile(src, dest, mode, callback) {
    mode = 0;
  }

-  src = getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' });
-  dest = getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' });
+  src = getValidatedPath(src, 'src');
+  dest = getValidatedPath(dest, 'dest');
  callback = makeCallback(callback);

  const req = new FSReqCallback();
@ -3011,8 +3003,8 @@ function copyFile(src, dest, mode, callback) {
 */
 function copyFileSync(src, dest, mode) {
  binding.copyFile(
-    getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' }),
-    getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }),
+    getValidatedPath(src, 'src'),
+    getValidatedPath(dest, 'dest'),
    mode,
  );
 }
--- a/lib/internal/fs/promises.js
+++ b/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', { expectFile: true, syscall: 'cp' }),
-      getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }),
+      getValidatedPath(src, 'src'),
+      getValidatedPath(dest, 'dest'),
      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', { expectFile: true, syscall: 'open' });
+  path = getValidatedPath(path);
  const flagsNumber = stringToFlags(flags);
  mode = parseFileMode(mode, 'mode', 0o666);
  return new FileHandle(await PromisePrototypeThen(
--- a/lib/internal/fs/streams.js
+++ b/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, 'path', { expectFile: true, syscall: 'read' });
+    validatePath(this.path);
  } 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, 'path', { expectFile: true, syscall: 'write' });
+    validatePath(this.path);
  } else {
    this.fd = getValidatedFd(importFd(this, options));
  }
--- a/lib/internal/fs/utils.js
+++ b/lib/internal/fs/utils.js
@ -710,38 +710,13 @@ const validateOffsetLengthWrite = hideStackFrames(
  },
 );

-/**
- * 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) => {
+const validatePath = hideStackFrames((path, propName = 'path') => {
  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?.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) ||
@ -757,21 +732,11 @@ const validatePath = hideStackFrames((path, propName = 'path', options) => {
  );
 });

-/**
- * 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 getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
+  const path = toPathIfFileURL(fileURLOrPath);
+  validatePath(path, propName);
+  return path;
+});

 const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
  if (ObjectIs(fd, -0)) {
--- a/test/sequential/test-fs-path-dir.js
+++ b/test/sequential/test-fs-path-dir.js
@ -1,174 +0,0 @@
-'use strict';
-
-const common = require('../common');
-const tmpdir = require('../common/tmpdir');
-const assert = require('assert');
-const path = require('path');
-const fs = require('fs');
-const { pathToFileURL } = require('url');
-
-tmpdir.refresh();
-
-let fileCounter = 1;
-const nextFile = () => path.join(tmpdir.path, `file${fileCounter++}`);
-
-const generateStringPath = (file, suffix = '') => file + suffix;
-
-const generateURLPath = (file, suffix = '') =>
-  pathToFileURL(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;
-    });
-    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 });
-}