From 3562205f865182a02ef282bf223526ee0e515496 Mon Sep 17 00:00:00 2001 From: nexdrew Date: Mon, 3 Jul 2023 15:23:24 -0400 Subject: [PATCH 1/5] add failing test originally authored by @mridgway --- test/plugins/plugins.test.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/plugins/plugins.test.js b/test/plugins/plugins.test.js index 83f8258c..fb70a9a1 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' ] }; From 20fab3e3bf1b02461653f2975a2a6ff705be9800 Mon Sep 17 00:00:00 2001 From: nexdrew Date: Mon, 3 Jul 2023 15:24:49 -0400 Subject: [PATCH 2/5] use root when stripped to empty string --- lib/plugins/pre/prePath.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plugins/pre/prePath.js b/lib/plugins/pre/prePath.js index 7361f34a..8325feae 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(); } From fd5ec1f220de05f9a78843427237a14cb1289e64 Mon Sep 17 00:00:00 2001 From: nexdrew Date: Mon, 3 Jul 2023 15:52:45 -0400 Subject: [PATCH 3/5] add failing test for defaultRoute with null pathname --- test/router.test.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/router.test.js b/test/router.test.js index d6277cd6..79e41a12 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -268,6 +268,28 @@ test('route handles 404', function(t) { ); }); +test('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: {} From 7e6fab42980f36bfd8b6e0ebc405a80a6a54f2e2 Mon Sep 17 00:00:00 2001 From: nexdrew Date: Mon, 3 Jul 2023 16:04:34 -0400 Subject: [PATCH 4/5] add failing test for router.lookup with null pathname --- test/router.test.js | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/test/router.test.js b/test/router.test.js index 79e41a12..a753e234 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,7 +288,7 @@ test('route handles 404', function(t) { ); }); -test('route handles 404 when pathname invalid', function(t) { +test('GH-1959: route handles 404 when pathname invalid', function(t) { var router = new Router({ log: {} }); From 606f7351f6ec36a549643b7bf3b54efafacffa36 Mon Sep 17 00:00:00 2001 From: nexdrew Date: Mon, 3 Jul 2023 16:12:33 -0400 Subject: [PATCH 5/5] handle null pathname in router methods --- lib/router.js | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/router.js b/lib/router.js index 8cadd000..6d775672 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); };