Skip to content
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

path: add type checking for path inputs #1153

Closed
wants to merge 1 commit into from
Closed

path: add type checking for path inputs #1153

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
68 changes: 49 additions & 19 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
'use strict';

const util = require('util');
const isWindows = process.platform === 'win32';

function assertPath(path) {
if (typeof path !== 'string')
throw new TypeError('Path must be a string. Received ' +
util.inspect(path));
Copy link
Member

Choose a reason for hiding this comment

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

Minor style nit: can you put braces around the body here? It doesn't fit on a single line.

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like this makes it more clear which argument is of wrong type:

function assertPaths() {
  [].slice.call(arguments).forEach(function (path, i) {
    if (typeof path !== 'string')
      throw new TypeError(util.format('Argument %d must be a string.' +
                          ' Received %s', i + 1, util.inspect(path));
  });
}
assertPaths(from, to);

Copy link
Contributor

Choose a reason for hiding this comment

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

[].slice.call(arguments) <-- slow

The forEach isn't particularly speedy either.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, probably better to keep it the way it is then.

I wish rest parameters were in V8 already 😭

Copy link
Contributor

Choose a reason for hiding this comment

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

The fastest way to write it is not that much harder actually ;p

function assertPaths() {
  for (var i = 0; i < arguments.length; ++i) {
    if (typeof arguments[i] !== 'string')
      throw new TypeError(util.format('Argument %d must be a string.' +
                          ' Received %s', i + 1, util.inspect(arguments[i])));
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that didn't occur to me of course, because I secretly hate classic for loops. I have to jsperf argument iteration sometimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not have this be over-engineered, we're just checking for argument types - anyways, as is, you could see which argument is bad via the stack trace.

// resolves . and .. elements in a path array with directory names there
// must be no slashes or device names (c:\) in the array
// (so also no leading and trailing slashes - it does not distinguish
Expand Down Expand Up @@ -84,10 +91,10 @@ win32.resolve = function() {
}
}

// Skip empty and invalid entries
if (typeof path !== 'string') {
throw new TypeError('Arguments to path.resolve must be strings');
} else if (!path) {
assertPath(path);

// Skip empty entries
if (path === '') {
continue;
}

Expand Down Expand Up @@ -137,6 +144,8 @@ win32.resolve = function() {


win32.normalize = function(path) {
assertPath(path);

var result = splitDeviceRe.exec(path),
device = result[1] || '',
isUnc = device && device.charAt(1) !== ':',
Expand Down Expand Up @@ -165,6 +174,8 @@ win32.normalize = function(path) {


win32.isAbsolute = function(path) {
assertPath(path);

var result = splitDeviceRe.exec(path),
device = result[1] || '',
isUnc = !!device && device.charAt(1) !== ':';
Expand Down Expand Up @@ -210,6 +221,9 @@ win32.join = function() {
// to = 'C:\\orandea\\impl\\bbb'
// The output of the function should be: '..\\..\\impl\\bbb'
win32.relative = function(from, to) {
assertPath(from);
assertPath(to);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a little weird because the error will always be "path" no matter which is missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should be changed to be like line #328?


from = win32.resolve(from);
to = win32.resolve(to);

Expand Down Expand Up @@ -287,6 +301,8 @@ win32._makeLong = function(path) {


win32.dirname = function(path) {
assertPath(path);

var result = win32SplitPath(path),
root = result[0],
dir = result[1];
Expand All @@ -306,6 +322,11 @@ win32.dirname = function(path) {


win32.basename = function(path, ext) {
assertPath(path);

if (ext !== undefined && typeof ext !== 'string')
Copy link

Choose a reason for hiding this comment

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

This causes breakage when used as an Array.map callback, which will pass in an index for ext.

Noticed because I had:

result = directories.map(path.basename);

And upgrading now throws:

>> TypeError: ext must be a string
>>     at posix.basename (path.js:550:11)
>>     at Array.map (native)

Technically an improper use of basename but still a small reduction in utility.

throw new TypeError('ext must be a string');

var f = win32SplitPath(path)[2];
// TODO: make this comparison case-insensitive on windows?
if (ext && f.substr(-1 * ext.length) === ext) {
Expand All @@ -316,6 +337,7 @@ win32.basename = function(path, ext) {


win32.extname = function(path) {
assertPath(path);
return win32SplitPath(path)[3];
};

Expand Down Expand Up @@ -351,11 +373,8 @@ win32.format = function(pathObject) {


win32.parse = function(pathString) {
if (typeof pathString !== 'string') {
throw new TypeError(
"Parameter 'pathString' must be a string, not " + typeof pathString
);
}
assertPath(pathString);

var allParts = win32SplitPath(pathString);
if (!allParts || allParts.length !== 4) {
throw new TypeError("Invalid path '" + pathString + "'");
Expand Down Expand Up @@ -395,10 +414,10 @@ posix.resolve = function() {
for (var i = arguments.length - 1; i >= -1 && !resolvedAbsolute; i--) {
var path = (i >= 0) ? arguments[i] : process.cwd();

// Skip empty and invalid entries
if (typeof path !== 'string') {
throw new TypeError('Arguments to path.resolve must be strings');
} else if (!path) {
assertPath(path);

// Skip empty entries
if (path === '') {
continue;
}

Expand All @@ -419,6 +438,8 @@ posix.resolve = function() {
// path.normalize(path)
// posix version
posix.normalize = function(path) {
assertPath(path);

var isAbsolute = posix.isAbsolute(path),
trailingSlash = path.substr(-1) === '/';

Expand All @@ -437,6 +458,7 @@ posix.normalize = function(path) {

// posix version
posix.isAbsolute = function(path) {
assertPath(path);
return path.charAt(0) === '/';
};

Expand All @@ -463,6 +485,9 @@ posix.join = function() {
// path.relative(from, to)
// posix version
posix.relative = function(from, to) {
assertPath(from);
assertPath(to);

from = posix.resolve(from).substr(1);
to = posix.resolve(to).substr(1);

Expand Down Expand Up @@ -510,6 +535,8 @@ posix._makeLong = function(path) {


posix.dirname = function(path) {
assertPath(path);

var result = posixSplitPath(path),
root = result[0],
dir = result[1];
Expand All @@ -529,8 +556,13 @@ posix.dirname = function(path) {


posix.basename = function(path, ext) {
assertPath(path);

if (ext !== undefined && typeof ext !== 'string')
throw new TypeError('ext must be a string');

var f = posixSplitPath(path)[2];
// TODO: make this comparison case-insensitive on windows?

if (ext && f.substr(-1 * ext.length) === ext) {
f = f.substr(0, f.length - ext.length);
}
Expand All @@ -539,6 +571,7 @@ posix.basename = function(path, ext) {


posix.extname = function(path) {
assertPath(path);
return posixSplitPath(path)[3];
};

Expand Down Expand Up @@ -566,11 +599,8 @@ posix.format = function(pathObject) {


posix.parse = function(pathString) {
if (typeof pathString !== 'string') {
throw new TypeError(
"Parameter 'pathString' must be a string, not " + typeof pathString
);
}
assertPath(pathString);

var allParts = posixSplitPath(pathString);
if (!allParts || allParts.length !== 4) {
throw new TypeError("Invalid path '" + pathString + "'");
Expand Down
10 changes: 5 additions & 5 deletions test/parallel/test-path-parse-format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ var unixPaths = [
];

var errors = [
{method: 'parse', input: [null], message: /Parameter 'pathString' must be a string, not/},
{method: 'parse', input: [{}], message: /Parameter 'pathString' must be a string, not object/},
{method: 'parse', input: [true], message: /Parameter 'pathString' must be a string, not boolean/},
{method: 'parse', input: [1], message: /Parameter 'pathString' must be a string, not number/},
{method: 'parse', input: [], message: /Parameter 'pathString' must be a string, not undefined/},
{method: 'parse', input: [null], message: /Path must be a string. Received null/},
{method: 'parse', input: [{}], message: /Path must be a string. Received {}/},
{method: 'parse', input: [true], message: /Path must be a string. Received true/},
{method: 'parse', input: [1], message: /Path must be a string. Received 1/},
{method: 'parse', input: [], message: /Path must be a string. Received undefined/},
// {method: 'parse', input: [''], message: /Invalid path/}, // omitted because it's hard to trigger!
{method: 'format', input: [null], message: /Parameter 'pathObject' must be an object, not/},
{method: 'format', input: [''], message: /Parameter 'pathObject' must be an object, not string/},
Expand Down
32 changes: 26 additions & 6 deletions test/parallel/test-path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,34 @@ joinTests.forEach(function(test) {
// assert.equal(actual, expected, message);
});
assert.equal(failures.length, 0, failures.join(''));
var joinThrowTests = [true, false, 7, null, {}, undefined, [], NaN];
joinThrowTests.forEach(function(test) {
assert.throws(function() {
path.join(test);
}, TypeError);

// Test thrown TypeErrors
var typeErrorTests = [true, false, 7, null, {}, undefined, [], NaN];

function fail(fn) {
var args = Array.prototype.slice.call(arguments, 1);

assert.throws(function() {
path.resolve(test);
fn.apply(null, args);
}, TypeError);
}

typeErrorTests.forEach(function(test) {
fail(path.join, test);
fail(path.resolve, test);
fail(path.normalize, test);
fail(path.isAbsolute, test);
fail(path.dirname, test);
fail(path.relative, test, 'foo');
fail(path.relative, 'foo', test);
fail(path.basename, test);
fail(path.extname, test);
fail(path.parse, test);

// undefined is a valid value as the second argument to basename
if (test !== undefined) {
fail(path.basename, 'foo', test);
}
});


Expand Down