diff --git a/lib/plugins/pre/prePath.js b/lib/plugins/pre/prePath.js index 7361f34a3..8325feaec 100644 --- a/lib/plugins/pre/prePath.js +++ b/lib/plugins/pre/prePath.js @@ -44,7 +44,7 @@ function strip(path) { */ function sanitizePath() { function _sanitizePath(req, res, next) { - req.url = strip(req.url); + req.url = strip(req.url) || '/'; next(); } diff --git a/lib/router.js b/lib/router.js index 8cadd0001..6d7756728 100644 --- a/lib/router.js +++ b/lib/router.js @@ -72,6 +72,11 @@ util.inherits(Router, EventEmitter); Router.prototype.lookup = function lookup(req, res) { var pathname = req.getUrl().pathname; + // Handle unexpected url parsing result + if (!pathname) { + return undefined; + } + // Find route var registryRoute = this._registry.lookup(req.method, pathname); @@ -323,11 +328,16 @@ Router.prototype.defaultRoute = function defaultRoute(req, res, next) { } // Check for 405 instead of 404 - var allowedMethods = http.METHODS.filter(function some(method) { - return method !== req.method && self._registry.lookup(method, pathname); - }); + var allowedMethods; + if (pathname) { + allowedMethods = http.METHODS.filter(function some(method) { + return ( + method !== req.method && self._registry.lookup(method, pathname) + ); + }); + } - if (allowedMethods.length) { + if (allowedMethods && allowedMethods.length) { res.methods = allowedMethods; res.setHeader('Allow', allowedMethods.join(', ')); var methodErr = new MethodNotAllowedError( @@ -340,7 +350,10 @@ Router.prototype.defaultRoute = function defaultRoute(req, res, next) { // clean up the url in case of potential xss // https://github.com/restify/node-restify/issues/1018 - var err = new ResourceNotFoundError('%s does not exist', pathname); + var err = new ResourceNotFoundError( + '%s does not exist', + pathname || 'Requested path' + ); next(err, req, res); }; diff --git a/test/plugins/plugins.test.js b/test/plugins/plugins.test.js index 83f8258ca..fb70a9a1d 100644 --- a/test/plugins/plugins.test.js +++ b/test/plugins/plugins.test.js @@ -290,28 +290,40 @@ describe('all other plugins', function() { // Ensure it santizies potential edge cases correctly var tests = { input: [ + '/', // basic path + '///', // excess on basic path '////foo////', //excess padding on both ends 'bar/foo/', // trailing slash 'bar/foo/////', // multiple trailing slashes 'foo////bar', // multiple slashes inbetween '////foo', // multiple at beginning - '/foo/bar' // don't mutate + '/foo/bar', // don't mutate + '/?test=1', // basic with query + '///foo///?test=1' // excess with query ], output: [ + '/', + '/', '/foo', 'bar/foo', 'bar/foo', 'foo/bar', '/foo', - '/foo/bar' + '/foo/bar', + '/?test=1', + '/foo?test=1' ], description: [ + 'should give the root as a slash', + 'dont give empty string for excess on root', 'should clean excess padding on both ends', 'should clean trailing slash', 'should clean multiple trailing slashes', 'should clean multiple slashes inbetween', 'should clean multiple at beginning', - 'dont mutate correct urls' + 'dont mutate correct urls', + 'preserve query string with basic path', + 'preserve query string with excess slashes' ] }; diff --git a/test/router.test.js b/test/router.test.js index d6277cd63..a753e2346 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -246,6 +246,26 @@ test('lookup calls next with err', function(t) { }); }); +test('GH-1959: lookup returns undefined when no handler found', function(t) { + var router = new Router({ + log: {} + }); + var handlerFound = router.lookup( + Object.assign( + { + getUrl: function() { + return { pathname: null }; + }, + method: 'GET' + }, + mockReq + ), + mockRes + ); + t.notOk(handlerFound); + t.end(); +}); + test('route handles 404', function(t) { var router = new Router({ log: {} @@ -268,6 +288,28 @@ test('route handles 404', function(t) { ); }); +test('GH-1959: route handles 404 when pathname invalid', function(t) { + var router = new Router({ + log: {} + }); + router.defaultRoute( + Object.assign( + { + getUrl: function() { + return { pathname: null }; + }, + method: 'GET' + }, + mockReq + ), + mockRes, + function next(err) { + t.equal(err.statusCode, 404); + t.end(); + } + ); +}); + test('route handles method not allowed (405)', function(t) { var router = new Router({ log: {}