From 85a8c21dc1d31ad5c88d21be5320a4ba090032bf Mon Sep 17 00:00:00 2001 From: "David Humphrey (:humph) david.humphrey@senecacollege.ca" Date: Wed, 2 Jan 2019 18:07:44 -0500 Subject: [PATCH] Support file:// URLs and Buffers for path args, throw when invalid --- README.md | 17 ++- src/filesystem/implementation.js | 64 --------- src/filesystem/interface.js | 124 ++++++++++++++---- tests/bugs/issue270.js | 20 +-- tests/spec/fs.write.spec.js | 10 +- .../spec/node-js/simple/test-fs-null-bytes.js | 19 +-- tests/spec/path-resolution.spec.js | 51 +++++++ 7 files changed, 178 insertions(+), 127 deletions(-) diff --git a/README.md b/README.md index 3d225a2..84e0f2e 100644 --- a/README.md +++ b/README.md @@ -232,7 +232,7 @@ The node.js [path module](http://nodejs.org/api/path.html) is available via the identical to the node.js (see [https://github.com/browserify/path-browserify](https://github.com/browserify/path-browserify)) version with the following differences: * The CWD always defaults to `/` -* No support for Windows style paths +* No support for Windows style paths (assume you are on a POSIX system) * Additional utility methods (see below) ```javascript @@ -267,6 +267,21 @@ Filer.Path also includes the following extra methods: * `addTrailing(p)` returns the path `p` with a single trailing slash added * `removeTrailing(p)` returns the path `p` with trailing slash(es) removed +[As with node.js](https://nodejs.org/api/fs.html#fs_file_paths), all methods below that +accept a `path` argument as a `String` can also take a [`file://` URL](https://nodejs.org/api/fs.html#fs_url_object_support) +or a `Buffer`. For example, all of the following cases will work the same way with Filer: + +```js +// 1. path as a String +fs.writeFile('/dir/file.txt', 'data', function(err) {...}); + +// 2. path as a URL +fs.writeFile(new URL('file:///dir/file.txt'), 'data', function(err) {...}); + +// 3. path as a Buffer +fs.writeFile(Buffer.from('/dir/file.txt'), 'data', function(err) {...}); +``` + #### Filer.Errors The error objects used internally by Filer are also exposed via the `Filer.Errors` object. As much as possible diff --git a/src/filesystem/implementation.js b/src/filesystem/implementation.js index d1b5eca..38e5c67 100644 --- a/src/filesystem/implementation.js +++ b/src/filesystem/implementation.js @@ -3,7 +3,6 @@ var normalize = Path.normalize; var dirname = Path.dirname; var basename = Path.basename; var isAbsolutePath = Path.isAbsolute; -var isNullPath = Path.isNull; var shared = require('../shared.js'); var Constants = require('../constants.js'); @@ -1627,30 +1626,6 @@ function validate_file_options(options, enc, fileMode){ return options; } -function pathCheck(path, allowRelative, callback) { - var err; - - if(typeof allowRelative === 'function') { - callback = allowRelative; - allowRelative = false; - } - - if(!path) { - err = new Errors.EINVAL('Path must be a string', path); - } else if(isNullPath(path)) { - err = new Errors.EINVAL('Path must be a string without null bytes.', path); - } else if(!allowRelative && !isAbsolutePath(path)) { - err = new Errors.EINVAL('Path must be absolute.', path); - } - - if(err) { - callback(err); - return false; - } - return true; -} - - function open(context, path, flags, mode, callback) { if (arguments.length < 5 ){ callback = arguments[arguments.length - 1]; @@ -1659,8 +1634,6 @@ function open(context, path, flags, mode, callback) { else { mode = validateAndMaskMode(mode, FULL_READ_WRITE_EXEC_PERMISSIONS, callback); } - - if(!pathCheck(path, callback)) return; function check_result(error, fileNode) { if(error) { @@ -1696,7 +1669,6 @@ function close(context, fd, callback) { } function mknod(context, path, type, callback) { - if(!pathCheck(path, callback)) return; make_node(context, path, type, callback); } @@ -1709,7 +1681,6 @@ function mkdir(context, path, mode, callback) { if(!mode) return; } - if(!pathCheck(path, callback)) return; make_directory(context, path, callback); } @@ -1719,7 +1690,6 @@ function access(context, path, mode, callback) { mode = Constants.fsConstants.F_OK; } - if (!pathCheck(path, callback)) return; mode = mode | Constants.fsConstants.F_OK; access_file(context, path, mode, callback); } @@ -1733,20 +1703,16 @@ function mkdtemp(context, prefix, options, callback) { let random = shared.randomChars(6); var path = prefix + '-' + random; - if(!pathCheck(path, callback)) return; make_directory(context, path, function(error) { callback(error, path); }); } function rmdir(context, path, callback) { - if(!pathCheck(path, callback)) return; remove_directory(context, path, callback); } function stat(context, path, callback) { - if(!pathCheck(path, callback)) return; - function check_result(error, result) { if(error) { callback(error); @@ -1778,13 +1744,10 @@ function fstat(context, fd, callback) { } function link(context, oldpath, newpath, callback) { - if(!pathCheck(oldpath, callback)) return; - if(!pathCheck(newpath, callback)) return; link_node(context, oldpath, newpath, callback); } function unlink(context, path, callback) { - if(!pathCheck(path, callback)) return; unlink_node(context, path, callback); } @@ -1823,8 +1786,6 @@ function readFile(context, path, options, callback) { callback = arguments[arguments.length - 1]; options = validate_file_options(options, null, 'r'); - if(!pathCheck(path, callback)) return; - var flags = validate_flags(options.flag || 'r'); if(!flags) { return callback(new Errors.EINVAL('flags is not valid', path)); @@ -1897,8 +1858,6 @@ function writeFile(context, path, data, options, callback) { callback = arguments[arguments.length - 1]; options = validate_file_options(options, 'utf8', 'w'); - if(!pathCheck(path, callback)) return; - var flags = validate_flags(options.flag || 'w'); if(!flags) { return callback(new Errors.EINVAL('flags is not valid', path)); @@ -1934,8 +1893,6 @@ function appendFile(context, path, data, options, callback) { callback = arguments[arguments.length - 1]; options = validate_file_options(options, 'utf8', 'a'); - if(!pathCheck(path, callback)) return; - var flags = validate_flags(options.flag || 'a'); if(!flags) { return callback(new Errors.EINVAL('flags is not valid', path)); @@ -2098,7 +2055,6 @@ function fchown_file(context, ofd, uid, gid, callback) { } function getxattr(context, path, name, callback) { - if (!pathCheck(path, callback)) return; getxattr_file(context, path, name, callback); } @@ -2118,7 +2074,6 @@ function setxattr(context, path, name, value, flag, callback) { flag = null; } - if (!pathCheck(path, callback)) return; setxattr_file(context, path, name, value, flag, callback); } @@ -2141,7 +2096,6 @@ function fsetxattr(context, fd, name, value, flag, callback) { } function removexattr(context, path, name, callback) { - if (!pathCheck(path, callback)) return; removexattr_file(context, path, name, callback); } @@ -2199,7 +2153,6 @@ function lseek(context, fd, offset, whence, callback) { } function readdir(context, path, callback) { - if(!pathCheck(path, callback)) return; read_directory(context, path, callback); } @@ -2213,8 +2166,6 @@ function toUnixTimestamp(time) { } function utimes(context, path, atime, mtime, callback) { - if(!pathCheck(path, callback)) return; - var currentTime = Date.now(); atime = (atime) ? toUnixTimestamp(atime) : toUnixTimestamp(currentTime); mtime = (mtime) ? toUnixTimestamp(mtime) : toUnixTimestamp(currentTime); @@ -2238,7 +2189,6 @@ function futimes(context, fd, atime, mtime, callback) { } function chmod(context, path, mode, callback) { - if(!pathCheck(path, callback)) return; mode = validateAndMaskMode(mode, callback); if(!mode) return; @@ -2260,7 +2210,6 @@ function fchmod(context, fd, mode, callback) { } function chown(context, path, uid, gid, callback) { - if(!pathCheck(path, callback)) return; if(!isUint32(uid)) { return callback(new Errors.EINVAL('uid must be a valid integer', uid)); } @@ -2290,9 +2239,6 @@ function fchown(context, fd, uid, gid, callback) { } function rename(context, oldpath, newpath, callback) { - if(!pathCheck(oldpath, callback)) return; - if(!pathCheck(newpath, callback)) return; - oldpath = normalize(oldpath); newpath = normalize(newpath); @@ -2407,23 +2353,14 @@ function rename(context, oldpath, newpath, callback) { function symlink(context, srcpath, dstpath, type, callback) { // NOTE: we support passing the `type` arg, but ignore it. callback = arguments[arguments.length - 1]; - - // Special Case: allow srcpath to be relative, which we normally don't permit. - // If the srcpath is relative, we assume it's relative to the dirpath of dstpath. - if(!pathCheck(srcpath, true, callback)) return; - if(!pathCheck(dstpath, callback)) return; - make_symbolic_link(context, srcpath, dstpath, callback); } function readlink(context, path, callback) { - if(!pathCheck(path, callback)) return; read_link(context, path, callback); } function lstat(context, path, callback) { - if(!pathCheck(path, callback)) return; - function check_result(error, result) { if(error) { callback(error); @@ -2441,7 +2378,6 @@ function truncate(context, path, length, callback) { callback = arguments[arguments.length - 1]; length = length || 0; - if(!pathCheck(path, callback)) return; if(validateInteger(length, callback) !== length) return; truncate_file(context, path, length, callback); diff --git a/src/filesystem/interface.js b/src/filesystem/interface.js index 9b9130c..aa14f74 100644 --- a/src/filesystem/interface.js +++ b/src/filesystem/interface.js @@ -1,6 +1,6 @@ var { promisify } = require('es6-promisify'); -var isNullPath = require('../path.js').isNull; +var Path = require('../path.js'); var nop = require('../shared.js').nop; var Constants = require('../constants.js'); @@ -45,6 +45,61 @@ function defaultCallback(err) { console.error('Filer error: ', err); } } +// Get a path (String) from a file:// URL. Support URL() like objects +// https://github.com/nodejs/node/blob/968e901aff38a343b1de4addebf79fd8fa991c59/lib/internal/url.js#L1381 +function toPathIfFileURL(fileURLOrPath) { + if(!(fileURLOrPath && + fileURLOrPath.protocol && + fileURLOrPath.pathname)) { + return fileURLOrPath; + } + + if(fileURLOrPath.protocol !== 'file:') { + throw new Errors.EINVAL('only file: URLs are supported for paths', fileURLOrPath); + } + + var pathname = fileURLOrPath.pathname; + for (var n = 0; n < pathname.length; n++) { + if (pathname[n] === '%') { + var third = pathname.codePointAt(n + 2) | 0x20; + if (pathname[n + 1] === '2' && third === 102) { + throw new Errors.EINVAL('file: URLs must not include encoded / characters', fileURLOrPath); + } + } + } + + return decodeURIComponent(pathname); +} + +// Allow Buffers for paths. Assumes we want UTF8. +function toPathIfBuffer(bufferOrPath) { + return Buffer.isBuffer(bufferOrPath) ? bufferOrPath.toString() : bufferOrPath; +} + +function validatePath(path, allowRelative) { + if(!path) { + return new Errors.EINVAL('Path must be a string', path); + } else if(Path.isNull(path)) { + return new Errors.EINVAL('Path must be a string without null bytes.', path); + } else if(!allowRelative && !Path.isAbsolute(path)) { + return new Errors.EINVAL('Path must be absolute.', path); + } +} + +function processPathArg(args, idx, allowRelative) { + var path = args[idx]; + path = toPathIfFileURL(path); + path = toPathIfBuffer(path); + + // Some methods specifically allow for rel paths (eg symlink with srcPath) + var err = validatePath(path, allowRelative); + if(err) { + throw err; + } + + // Overwrite path arg with converted and validated path + args[idx] = path; +} /** * FileSystem @@ -124,7 +179,7 @@ function FileSystem(options, callback) { // We support the optional `options` arg from node, but ignore it this.watch = function(filename, options, listener) { - if(isNullPath(filename)) { + if(Path.isNull(filename)) { throw new Error('Path must be a string without null bytes.'); } if(typeof options === 'function') { @@ -247,50 +302,53 @@ function FileSystem(options, callback) { } }); FileSystem.prototype.promises = {}; + /** - * Public API for FileSystem. All node.js methods that are - * exposed on fs.promises include `promise: true`. We also - * include our own extra methods, but skip the fd versions - * to match node.js, which puts these on a FileHandle object. + * Public API for FileSystem. All node.js methods that are exposed on fs.promises + * include `promise: true`. We also include our own extra methods, but skip the + * fd versions to match node.js, which puts these on a `FileHandle` object. + * Any method that deals with path argument(s) also includes the position of + * those args in one of `absPathArgs: [...]` or `relPathArgs: [...]`, so they + * can be processed and validated before being passed on to the method. */ [ - { name: 'open', promises: true }, - { name: 'access', promises: true }, - { name: 'chmod', promises: true }, + { name: 'open', promises: true, absPathArgs: [ 0 ] }, + { name: 'access', promises: true, absPathArgs: [ 0 ] }, + { name: 'chmod', promises: true, absPathArgs: [ 0 ] }, { name: 'fchmod' }, - { name: 'chown', promises: true }, + { name: 'chown', promises: true, absPathArgs: [ 0 ] }, { name: 'fchown' }, { name: 'close' }, - { name: 'mknod', promises: true }, - { name: 'mkdir', promises: true }, + { name: 'mknod', promises: true, absPathArgs: [ 0 ] }, + { name: 'mkdir', promises: true, absPathArgs: [ 0 ] }, { name: 'mkdtemp', promises: true }, - { name: 'rmdir', promises: true }, - { name: 'stat', promises: true }, + { name: 'rmdir', promises: true, absPathArgs: [ 0 ] }, + { name: 'stat', promises: true, absPathArgs: [ 0 ] }, { name: 'fstat' }, { name: 'fsync' }, - { name: 'link', promises: true }, - { name: 'unlink', promises: true }, + { name: 'link', promises: true, absPathArgs: [0, 1] }, + { name: 'unlink', promises: true, absPathArgs: [ 0 ] }, { name: 'read' }, - { name: 'readFile', promises: true }, + { name: 'readFile', promises: true, absPathArgs: [ 0 ] }, { name: 'write' }, - { name: 'writeFile', promises: true }, - { name: 'appendFile', promises: true }, - { name: 'exists' }, + { name: 'writeFile', promises: true, absPathArgs: [ 0 ] }, + { name: 'appendFile', promises: true, absPathArgs: [ 0 ] }, + { name: 'exists', absPathArgs: [ 0 ] }, { name: 'lseek' }, - { name: 'readdir', promises: true }, - { name: 'rename', promises: true }, - { name: 'readlink', promises: true }, - { name: 'symlink', promises: true }, + { name: 'readdir', promises: true, absPathArgs: [ 0 ] }, + { name: 'rename', promises: true, absPathArgs: [0, 1] }, + { name: 'readlink', promises: true, absPathArgs: [ 0 ] }, + { name: 'symlink', promises: true, relPathPargs: [ 0 ], absPathArgs: [ 1 ] }, { name: 'lstat', promises: true }, - { name: 'truncate', promises: true }, + { name: 'truncate', promises: true, absPathArgs: [ 0 ] }, { name: 'ftruncate' }, - { name: 'utimes', promises: true }, + { name: 'utimes', promises: true, absPathArgs: [ 0 ] }, { name: 'futimes' }, - { name: 'setxattr', promises: true }, - { name: 'getxattr', promises: true }, + { name: 'setxattr', promises: true, absPathArgs: [ 0 ] }, + { name: 'getxattr', promises: true, absPathArgs: [ 0 ] }, { name: 'fsetxattr' }, { name: 'fgetxattr' }, - { name: 'removexattr', promises: true }, + { name: 'removexattr', promises: true, absPathArgs: [ 0 ] }, { name: 'fremovexattr' } ].forEach(function(method) { var methodName = method.name; @@ -306,6 +364,14 @@ function FileSystem(options, callback) { var missingCallback = typeof args[lastArgIndex] !== 'function'; var callback = maybeCallback(args[lastArgIndex]); + // Deal with path arguments, validating and normalizing Buffer and file:// URLs + if(method.absPathArgs) { + method.absPathArgs.forEach(pathArg => processPathArg(args, pathArg, false)); + } + if(method.relPathPargs) { + method.relPathPargs.forEach(pathArg => processPathArg(args, pathArg, true)); + } + var error = fs.queueOrRun(function() { var context = fs.provider.openReadWriteContext(); diff --git a/tests/bugs/issue270.js b/tests/bugs/issue270.js index f7d1199..ea6ae2f 100644 --- a/tests/bugs/issue270.js +++ b/tests/bugs/issue270.js @@ -5,23 +5,15 @@ describe('undefined and relative paths, issue270', function() { beforeEach(util.setup); afterEach(util.cleanup); - it('should fail with EINVAL when called on an undefined path', function(done) { + it('should fail with EINVAL when called on an undefined path', function() { var fs = util.fs(); - - fs.writeFile(undefined, 'data', function(err) { - expect(err).to.exist; - expect(err.code).to.equal('EINVAL'); - done(); - }); + var fn = () => fs.writeFile(undefined, 'data'); + expect(fn).to.throw(); }); - it('should fail with EINVAL when called on a relative path', function(done) { + it('should fail with EINVAL when called on a relative path', function() { var fs = util.fs(); - - fs.writeFile('relpath/file.txt', 'data', function(err) { - expect(err).to.exist; - expect(err.code).to.equal('EINVAL'); - done(); - }); + var fn = () => fs.writeFile('relpath/file.txt', 'data'); + expect(fn).to.throw(); }); }); diff --git a/tests/spec/fs.write.spec.js b/tests/spec/fs.write.spec.js index c93936b..ec5da7b 100644 --- a/tests/spec/fs.write.spec.js +++ b/tests/spec/fs.write.spec.js @@ -10,14 +10,10 @@ describe('fs.write', function() { expect(fs.write).to.be.a('function'); }); - it('should error if file path is undefined',function(done) { + it('should error if file path is undefined', function() { var fs = util.fs(); - - fs.writeFile(undefined, 'data', function(error) { - expect(error).to.exist; - expect(error.code).to.equal('EINVAL'); - done(); - }); + var fn = () => fs.writeFile(undefined, 'data'); + expect(fn).to.throw(); }); it('should write data to a file', function(done) { diff --git a/tests/spec/node-js/simple/test-fs-null-bytes.js b/tests/spec/node-js/simple/test-fs-null-bytes.js index 62b9290..f2b2573 100644 --- a/tests/spec/node-js/simple/test-fs-null-bytes.js +++ b/tests/spec/node-js/simple/test-fs-null-bytes.js @@ -9,23 +9,18 @@ describe('node.js tests: https://github.com/joyent/node/blob/master/test/simple/ var checks = []; var fnCount = 0; var fnTotal = 16; - var expected = 'Path must be a string without null bytes.'; var fs = util.fs(); // Make sure function fails with null path error in callback. function check(fn) { var args = Array.prototype.slice.call(arguments, 1); - args = args.concat(function(err) { - checks.push(function(){ - expect(err).to.exist; - expect(err.message).to.equal(expected); - }); - fnCount++; - if(fnCount === fnTotal) { - done(); - } - }); - fn.apply(fs, args); + fn = () => fn.apply(fs, args); + expect(fn).to.throw(); + + fnCount++; + if(fnCount === fnTotal) { + done(); + } } check(fs.link, '/foo\u0000bar', 'foobar'); diff --git a/tests/spec/path-resolution.spec.js b/tests/spec/path-resolution.spec.js index f44f040..cbf9352 100644 --- a/tests/spec/path-resolution.spec.js +++ b/tests/spec/path-resolution.spec.js @@ -2,6 +2,9 @@ var Filer = require('../../src'); var util = require('../lib/test-utils.js'); var expect = require('chai').expect; +// Support global URL and node's url module +var URL = global.URL || require('url').URL; + describe('path resolution', function() { beforeEach(util.setup); afterEach(util.cleanup); @@ -245,4 +248,52 @@ describe('path resolution', function() { expect(Path.removeTrailing('/dir/')).to.equal('/dir'); expect(Path.removeTrailing('/dir//')).to.equal('/dir'); }); + + it('should allow using Buffer for paths', function(done) { + var fs = util.fs(); + var filePath = '/file'; + var bufferPath = Buffer.from(filePath); + var data = 'data'; + + fs.writeFile(bufferPath, data, function(err) { + if(err) throw err; + + fs.readFile(filePath, 'utf8', function(err, result) { + if(err) throw err; + expect(result).to.equal(data); + done(); + }); + }); + }); + + it('should allow using file: URLs for paths', function(done) { + var fs = util.fs(); + var filePath = '/file'; + var fileUrl = new URL(`file://${filePath}`); + var data = 'data'; + + fs.writeFile(fileUrl, data, function(err) { + if(err) throw err; + + fs.readFile(filePath, 'utf8', function(err, result) { + if(err) throw err; + expect(result).to.equal(data); + done(); + }); + }); + }); + + it('should error for non file: URLs for paths', function() { + var fs = util.fs(); + var fileUrl = new URL('http://file'); + var fn = () => fs.writeFile(fileUrl, 1); + expect(fn).to.throw(); + }); + + it('should error if file: URLs include escaped / characters', function() { + var fs = util.fs(); + var fileUrl = new URL('file:///p/a/t/h/%2F'); + var fn = () => fs.writeFile(fileUrl, 1); + expect(fn).to.throw(); + }); });