fix: handle trailing slashes on root route and avoid crashes due to AssertionErrors thrown by odd requests

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();
     }
 

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);
 };
 

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'
             ]
         };
 

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: {}