Skip to content

fs: create consistent behavior for fs module across platforms #17927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fs: create consistent behavior for fs module across platforms #17927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

<a id="ERR_PATH_IS_DIRECTORY"></a>
### ERR_PATH_IS_DIRECTORY

Provided path ended with an unexpected character `/`.

<a id="ERR_REQUIRE_ESM"></a>
### ERR_REQUIRE_ESM

Expand Down
26 changes: 14 additions & 12 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand All @@ -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 };
Expand Down Expand Up @@ -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);
Expand All @@ -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

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -2095,6 +2096,7 @@ function WriteStream(path, options) {
if (!(this instanceof WriteStream))
return new WriteStream(path, options);

path = getPathFromURL(path, true);
Copy link
Member

@joyeecheung joyeecheung Mar 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already one this.path = getPathFromURL(path); below. Also path is ignored if options.fd !== undefined so maybe we just need to delete this and change the line below to:

if (options.fd !== undefined) {
  this.path = getPathFromURL(path);
  validatePath(this.path, true);
}

Same about ReadStream

Copy link
Contributor Author

@timotew timotew Mar 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validatePath only accepts Uint8Array this will work if Stream path is always Uint8Array or string.

Copy link
Member

@joyeecheung joyeecheung Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timotew The path in the fs stream classes can be falsy if options.fd is provdided, e.g. fs.createReadStream(null, { fd: fd, encoding: 'utf8' }) is valid. From what I can tell, validatePath accepts both string and Uint8Array.

options = copyObject(getOptions(options, {}));

Writable.call(this, options);
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 10 additions & 4 deletions lib/internal/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

@joyeecheung joyeecheung Mar 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string being checked and reported here should be the result of isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path) for consistency, also because errors like ERR_INVALID_FILE_URL_HOST should be thrown first.

Copy link
Member

@joyeecheung joyeecheung Mar 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, maybe getPathFromURL does not even need to be changed since the path extracted from URLs will be eventually passed to validatePath anyway..from what I can tell if we update WriteStream and ReadStream to validatePath(path, true) then we don't need to a ssert the file name in getPathFromURL

return isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path);
}

Expand Down
206 changes: 206 additions & 0 deletions test/parallel/test-fs-path-err.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
7 changes: 1 addition & 6 deletions test/parallel/test-fs-readfile-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}));

Expand Down