From 2a3eea8cc3285b5eb2a64df0300b35519443b7d8 Mon Sep 17 00:00:00 2001 From: "Timothy O. Peters" Date: Sun, 4 Mar 2018 09:34:44 +0100 Subject: [PATCH] create consistent behavior for fs module across platforms --- doc/api/errors.md | 5 + lib/fs.js | 26 +-- lib/internal/errors.js | 5 + lib/internal/fs.js | 14 +- lib/internal/url.js | 5 +- test/parallel/test-fs-path-err.js | 206 ++++++++++++++++++++++++ test/parallel/test-fs-readfile-error.js | 7 +- 7 files changed, 245 insertions(+), 23 deletions(-) create mode 100644 test/parallel/test-fs-path-err.js diff --git a/doc/api/errors.md b/doc/api/errors.md index b9c52ad0d9af89..2be8a8ec480f27 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1312,6 +1312,11 @@ A Node.js API was called in an unsupported manner, such as A given value is out of the accepted range. + +### ERR_PATH_IS_DIRECTORY + +Provided path ended with an unexpected character `/`. + ### ERR_REQUIRE_ESM diff --git a/lib/fs.js b/lib/fs.js index 2dcfb25348fe8c..3121fc229e6e71 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -235,8 +235,8 @@ fs.readFile = function(path, options, callback) { return; } - path = getPathFromURL(path); - validatePath(path); + path = getPathFromURL(path, true); + validatePath(path, 'path', true); binding.open(pathModule.toNamespacedPath(path), stringToFlags(options.flag || 'r'), 0o666, @@ -503,7 +503,7 @@ fs.open = function(path, flags, mode, callback_) { mode = modeNum(mode, 0o666); path = getPathFromURL(path); - validatePath(path); + validatePath(path, 'path', true); validateUint32(mode, 'mode'); const req = new FSReqWrap(); @@ -518,7 +518,7 @@ fs.open = function(path, flags, mode, callback_) { fs.openSync = function(path, flags, mode) { mode = modeNum(mode, 0o666); path = getPathFromURL(path); - validatePath(path); + validatePath(path, 'path', true); validateUint32(mode, 'mode'); const ctx = { path }; @@ -1879,10 +1879,10 @@ fs.copyFile = function(src, dest, flags, callback) { throw new errors.TypeError('ERR_INVALID_CALLBACK'); } - src = getPathFromURL(src); - dest = getPathFromURL(dest); - validatePath(src, 'src'); - validatePath(dest, 'dest'); + src = getPathFromURL(src, true); + dest = getPathFromURL(dest, true); + validatePath(src, 'src', true); + validatePath(dest, 'dest', true); src = pathModule._makeLong(src); dest = pathModule._makeLong(dest); @@ -1894,10 +1894,10 @@ fs.copyFile = function(src, dest, flags, callback) { fs.copyFileSync = function(src, dest, flags) { - src = getPathFromURL(src); - dest = getPathFromURL(dest); - validatePath(src, 'src'); - validatePath(dest, 'dest'); + src = getPathFromURL(src, true); + dest = getPathFromURL(dest, true); + validatePath(src, 'src', true); + validatePath(dest, 'dest', true); const ctx = { path: src, dest }; // non-prefixed @@ -1928,6 +1928,7 @@ function ReadStream(path, options) { if (!(this instanceof ReadStream)) return new ReadStream(path, options); + path = getPathFromURL(path, true); // a little bit bigger buffer and water marks by default options = copyObject(getOptions(options, {})); if (options.highWaterMark === undefined) @@ -2095,6 +2096,7 @@ function WriteStream(path, options) { if (!(this instanceof WriteStream)) return new WriteStream(path, options); + path = getPathFromURL(path, true); options = copyObject(getOptions(options, {})); Writable.call(this, options); diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a4a79d671e4938..1d668061d9fda9 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -823,6 +823,11 @@ E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU', TypeError); E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error); E('ERR_OUT_OF_RANGE', outOfRange, RangeError); +E('ERR_PATH_IS_DIRECTORY', (name, value) => { + const util = lazyUtil(); + return `The argument "${name}" must not end with "/". ` + + `Received ${util.inspect(value)}`; +}); E('ERR_REQUIRE_ESM', 'Must use import to load ES Module: %s', Error); E('ERR_SCRIPT_EXECUTION_INTERRUPTED', 'Script execution was interrupted by `SIGINT`.', Error); diff --git a/lib/internal/fs.js b/lib/internal/fs.js index 04844248b0b8e0..44896d586798c2 100644 --- a/lib/internal/fs.js +++ b/lib/internal/fs.js @@ -369,18 +369,24 @@ function validateOffsetLengthWrite(offset, length, byteLength) { } } -function validatePath(path, propName) { +function validatePath(path, propName, assertFilename) { let err; if (propName === undefined) { propName = 'path'; } - if (typeof path !== 'string' && !isUint8Array(path)) { + if (typeof path === 'string') { + err = nullCheck(path, propName, false); + if (assertFilename && err === undefined && path[path.length - 1] === '/') + err = new errors.Error('ERR_PATH_IS_DIRECTORY', propName, path); + } else if (isUint8Array(path)) { + err = nullCheck(path, propName, false); + if (assertFilename && err === undefined && path[path.length - 1] === 47) + err = new errors.Error('ERR_PATH_IS_DIRECTORY', propName, path); + } else { err = new errors.TypeError('ERR_INVALID_ARG_TYPE', propName, ['string', 'Buffer', 'URL']); - } else { - err = nullCheck(path, propName, false); } if (err !== undefined) { diff --git a/lib/internal/url.js b/lib/internal/url.js index 0bdfc4783e65f3..6a1eeca1e8956f 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -1365,13 +1365,16 @@ function getPathFromURLPosix(url) { return decodeURIComponent(pathname); } -function getPathFromURL(path) { +function getPathFromURL(path, assertFilename) { if (path == null || !path[searchParams] || !path[searchParams][searchParams]) { return path; } if (path.protocol !== 'file:') throw new errors.TypeError('ERR_INVALID_URL_SCHEME', 'file'); + + if (assertFilename && path.href[path.href.length - 1] === '/') + throw new errors.Error('ERR_PATH_IS_DIRECTORY', 'path', path.pathname); return isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path); } diff --git a/test/parallel/test-fs-path-err.js b/test/parallel/test-fs-path-err.js new file mode 100644 index 00000000000000..4b746363583313 --- /dev/null +++ b/test/parallel/test-fs-path-err.js @@ -0,0 +1,206 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const URL = require('url').URL; +const util = require('util'); + +function runPathTests(withSlash, withoutSlash, slashedPath, realName) { + + if (!slashedPath) { + slashedPath = withSlash; + } else if (typeof slashedPath === 'boolean') { + realName = slashedPath; + slashedPath = withSlash; + } + + common.expectsError( + () => { + fs.readFile(withSlash, common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.readFileSync(withSlash, { flag: 'r' }); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.open(withSlash, 'r', common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.openSync(withSlash, 'r'); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.appendFile(withSlash, 'test data', common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.appendFileSync(withSlash, 'test data'); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.createReadStream(withSlash); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + + common.expectsError( + () => { + fs.createWriteStream(withSlash); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.truncate(withSlash, 4, common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.truncateSync(withSlash, 4); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.writeFile(withSlash, 'test data', common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.writeFileSync(withSlash, 'test data'); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: 'The argument "path" must not end with "/". ' + + `Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.copyFile(withSlash, withoutSlash, common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: `The argument "${realName ? 'src' : 'path'}" must not end ` + + `with "/". Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.copyFile(withoutSlash, withSlash, common.mustNotCall()); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: `The argument "${realName ? 'dest' : 'path'}" must not end ` + + `with "/". Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.copyFileSync(withSlash, withoutSlash); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: `The argument "${realName ? 'src' : 'path'}" must not end ` + + `with "/". Received ${util.inspect(slashedPath)}` + }); + + common.expectsError( + () => { + fs.copyFileSync(withoutSlash, withSlash); + }, + { + code: 'ERR_PATH_IS_DIRECTORY', + type: Error, + message: `The argument "${realName ? 'dest' : 'path'}" must not end ` + + `with "/". Received ${util.inspect(slashedPath)}` + }); +} + +const path = '/tmp/test'; +const stringWithSlash = common.isWindows ? `file:///c:${path}/` : + `file://${path}/`; +const stringWithoutSlash = common.isWindows ? `file:///c:${path}` : + `file://${path}`; +const urlWithSlash = new URL(stringWithSlash); +const urlWithoutSlash = new URL(stringWithoutSlash); +const U8ABufferWithSlash = new Uint8Array(Buffer.from(stringWithSlash)); +const U8ABufferWithoutSlash = new Uint8Array(Buffer.from(stringWithoutSlash)); +runPathTests(urlWithSlash, urlWithoutSlash, `${path}/`); +runPathTests(stringWithSlash, stringWithoutSlash, true); +runPathTests(U8ABufferWithSlash, U8ABufferWithoutSlash, true); diff --git a/test/parallel/test-fs-readfile-error.js b/test/parallel/test-fs-readfile-error.js index 97633c9a887acc..c6e2488340d009 100644 --- a/test/parallel/test-fs-readfile-error.js +++ b/test/parallel/test-fs-readfile-error.js @@ -46,13 +46,8 @@ function test(env, cb) { })); } -test({ NODE_DEBUG: '' }, common.mustCall((data) => { - assert(/EISDIR/.test(data)); - assert(/test-fs-readfile-error/.test(data)); -})); - test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => { - assert(/EISDIR/.test(data)); + assert(/ERR_PATH_IS_DIRECTORY/.test(data)); assert(/test-fs-readfile-error/.test(data)); }));