diff --git a/lib/fs.js b/lib/fs.js index 2119554c0e72c1..4d73c0b6aaec9d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -384,7 +384,7 @@ 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', true), flagsNumber, 0o666, req); } function tryStatSync(fd, isUserFd) { @@ -436,7 +436,7 @@ function readFileSync(path, options) { if (options.encoding === 'utf8' || options.encoding === 'utf-8') { if (!isInt32(path)) { - path = getValidatedPath(path); + path = getValidatedPath(path, 'path', true); } return binding.readFileUtf8(path, stringToFlags(options.flag)); } @@ -530,7 +530,7 @@ function closeSync(fd) { * @returns {void} */ function open(path, flags, mode, callback) { - path = getValidatedPath(path); + path = getValidatedPath(path, 'path', true); if (arguments.length < 3) { callback = flags; flags = 'r'; @@ -559,7 +559,7 @@ function open(path, flags, mode, callback) { */ function openSync(path, flags, mode) { return binding.open( - getValidatedPath(path), + getValidatedPath(path, 'path', true), stringToFlags(flags), parseFileMode(mode, 'mode', 0o666), ); @@ -2336,7 +2336,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 = getValidatedPath(path, 'path', true); } return binding.writeFileUtf8( @@ -2981,8 +2981,8 @@ function copyFile(src, dest, mode, callback) { mode = 0; } - src = getValidatedPath(src, 'src'); - dest = getValidatedPath(dest, 'dest'); + src = getValidatedPath(src, 'src', true); + dest = getValidatedPath(dest, 'dest', true); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -3000,8 +3000,8 @@ function copyFile(src, dest, mode, callback) { */ function copyFileSync(src, dest, mode) { binding.copyFile( - getValidatedPath(src, 'src'), - getValidatedPath(dest, 'dest'), + getValidatedPath(src, 'src', true), + getValidatedPath(dest, 'dest', true), mode, ); } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 1d1fdb8463ca8d..679d2afb62b597 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', 'path', true), + getValidatedPath(dest, 'dest', 'path', true), 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', true); 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..8fed7c4371bd92 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', true); } 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', true); } else { this.fd = getValidatedFd(importFd(this, options)); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 34634123fa84f0..49354133990a27 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -709,7 +709,7 @@ const validateOffsetLengthWrite = hideStackFrames( }, ); -const validatePath = hideStackFrames((path, propName = 'path') => { +const validatePath = hideStackFrames((path, propName = 'path', isFile) => { if (typeof path !== 'string' && !isUint8Array(path)) { throw new ERR_INVALID_ARG_TYPE.HideStackFramesError(propName, ['string', 'Buffer', 'URL'], path); } @@ -717,6 +717,22 @@ const validatePath = hideStackFrames((path, propName = 'path') => { const pathIsString = typeof path === 'string'; const pathIsUint8Array = isUint8Array(path); + if (isFile) { + 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: 'validatePath', + errno: ERR_FS_EISDIR, + }); + } + } + // We can only perform meaningful checks on strings and Uint8Arrays. if ((!pathIsString && !pathIsUint8Array) || (pathIsString && !StringPrototypeIncludes(path, '\u0000')) || @@ -731,11 +747,12 @@ const validatePath = hideStackFrames((path, propName = 'path') => { ); }); -const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => { - const path = toPathIfFileURL(fileURLOrPath); - validatePath(path, propName); - return path; -}); +const getValidatedPath = + hideStackFrames((fileURLOrPath, propName = 'path', isFile = false) => { + const path = toPathIfFileURL(fileURLOrPath); + validatePath(path, propName, isFile); + 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 }); +}