From 00b2f07f9ddeb8ffd2fb2108b0ed9ffa81ea000d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= <110401522+huseyinacacak-janea@users.noreply.github.com> Date: Thu, 10 Oct 2024 14:14:56 +0300 Subject: [PATCH] fs,win: fix bug in paths with trailing slashes Fixes: https://github.com/nodejs/node/issues/17801 Refs: https://github.com/nodejs/node/pull/33831 PR-URL: https://github.com/nodejs/node/pull/54160 Reviewed-By: Matteo Collina Reviewed-By: Yagiz Nizipli --- lib/fs.js | 26 +++-- lib/internal/fs/promises.js | 6 +- lib/internal/fs/streams.js | 4 +- lib/internal/fs/utils.js | 47 +++++++- test/sequential/test-fs-path-dir.js | 174 ++++++++++++++++++++++++++++ 5 files changed, 237 insertions(+), 20 deletions(-) create mode 100644 test/sequential/test-fs-path-dir.js diff --git a/lib/fs.js b/lib/fs.js index 9db60a4f89a3d9..e6ae9855a0ddef 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -385,7 +385,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) { @@ -437,7 +441,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)); } @@ -531,7 +537,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'; @@ -560,7 +566,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), ); @@ -2339,7 +2345,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( @@ -2984,8 +2992,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(); @@ -3003,8 +3011,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, ); } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 76460d2957bb0b..f5a9a3a854263b 100644 --- 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'), - 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( diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index 43f06d0104de61..6e7ae9d6804a04 100644 --- 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); + 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)); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index d34e2054cba46e..703a79371d867d 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -706,13 +706,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) || @@ -728,11 +753,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)) { diff --git a/test/sequential/test-fs-path-dir.js b/test/sequential/test-fs-path-dir.js new file mode 100644 index 00000000000000..8ba5e59fdc290e --- /dev/null +++ b/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 }); +}