BREAKING CHANGE: Handle non-trailing slashes correctly (#1512)

* router - remove do not merge double slashes by default * use server.pre(restify.plugins.pre.dedupeSlashes()) to deal with double slashes * refactor test to cover multiple slashes cases
restify · Oct 9, 2017 · ac1eec5 · ac1eec5
1 parent e8efd6c
commit ac1eec5
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 39 deletions.
diff --git a/lib/router.js b/lib/router.js
@@ -103,7 +103,7 @@ function compileURL(options) {
             return (false);
         }
 
-        pattern += '\\/+';
+        pattern += '\\/';
 
         if (frag.charAt(0) === ':') {
             var label = frag;
@@ -134,7 +134,7 @@ function compileURL(options) {
     }
 
     if (!options.strict) {
-        pattern += '[\\/]*';
+        pattern += '[\\/]?';
     }
 
     if (pattern === '^') {

diff --git a/test/plugins/dedupeSlashes.js b/test/plugins/dedupeSlashes.js
@@ -67,18 +67,8 @@ describe('dedupe forward slashes in URL', function () {
             });
         });
 
-        it('should remove duplicate slashes', function (done) {
-            CLIENT.get('//foo//bar', function (err, _, res, data) {
-                assert.ifError(err);
-                assert.equal(res.statusCode, 200);
-                assert.equal(data, '/foo/bar');
-                done();
-            });
-        });
-
-        it('should remove duplicate slashes including trailing slashes',
-        function (done) {
-            CLIENT.get('//foo//bar//', function (err, _, res, data) {
+        it('should merge multiple slashes', function (done) {
+            CLIENT.get('//////foo///bar///////', function (err, _, res, data) {
                 assert.ifError(err);
                 assert.equal(res.statusCode, 200);
                 assert.equal(data, '/foo/bar/');
@@ -131,19 +121,8 @@ describe('dedupe forward slashes in URL', function () {
             });
         });
 
-        it('should remove duplicate slashes', function (done) {
-            CLIENT.get('//foo//bar//', function (err, _, res, data) {
-                assert.ifError(err);
-                assert.equal(res.statusCode, 200);
-                assert.equal(data, '/foo/bar/');
-                done();
-            });
-        });
-
-
-        it('should remove duplicate slashes including trailing slashes',
-        function (done) {
-            CLIENT.get('//foo//bar//', function (err, _, res, data) {
+        it('should merge multiple slashes', function (done) {
+            CLIENT.get('//////foo///bar///////', function (err, _, res, data) {
                 assert.ifError(err);
                 assert.equal(res.statusCode, 200);
                 assert.equal(data, '/foo/bar/');

diff --git a/test/router.test.js b/test/router.test.js
@@ -189,34 +189,48 @@ test('Strict routing distinguishes trailing slash', function (t) {
     server.get('/trailing/', noop);
     server.get('/no-trailing', noop);
 
-
     var trailing = server.router.routes.GET[0];
-    t.ok(trailing.path.test('/trailing/'));
-    t.notOk(trailing.path.test('/trailing'));
+    t.ok(trailing.path.test('/trailing/'), 'Single trailing slash is ok');
+    t.notOk(trailing.path.test('/trailing'), 'No trailing slash is not ok');
+    t.notOk(trailing.path.test('/trailing//'),
+            'Double trailing slash is not ok');
+    t.notOk(trailing.path.test('//trailing/'),
+            'Double heading slash is not ok');
 
     var noTrailing = server.router.routes.GET[1];
-    t.ok(noTrailing.path.test('/no-trailing'));
-    t.notOk(noTrailing.path.test('/no-trailing/'));
+    t.ok(noTrailing.path.test('/no-trailing', 'No trailing slash is ok'));
+    t.notOk(noTrailing.path.test('/no-trailing/'),
+            'Single trailing slash is not ok');
+    t.notOk(noTrailing.path.test('/no-trailing//'),
+            'Double trailing slash is not ok');
+    t.notOk(noTrailing.path.test('//no-trailing'),
+            'Double heading slash is not ok');
 
     t.end();
 });
 
-test('Default non-strict routing ignores trailing slash(es)', function (t) {
+test('Non-strict routing distinguishes trailing slash', function (t) {
     var server = restify.createServer();
     function noop () {}
 
     server.get('/trailing/', noop);
     server.get('/no-trailing', noop);
 
     var trailing = server.router.routes.GET[0];
-    t.ok(trailing.path.test('/trailing/'));
-    t.ok(trailing.path.test('//trailing//'));
-    t.ok(trailing.path.test('/trailing'));
+    t.ok(trailing.path.test('/trailing/', 'Single trailing slash is ok'));
+    t.ok(trailing.path.test('/trailing'), 'No trailing slash is not ok');
+    t.notOk(trailing.path.test('/trailing//'),
+            'Double trailing slash is not ok');
+    t.notOk(trailing.path.test('//trailing'),
+            'Double heading slash is not ok');
 
     var noTrailing = server.router.routes.GET[1];
-    t.ok(noTrailing.path.test('/no-trailing'));
-    t.ok(noTrailing.path.test('//no-trailing//'));
-    t.ok(noTrailing.path.test('/no-trailing/'));
+    t.ok(noTrailing.path.test('/no-trailing', 'No trailing slash is ok'));
+    t.ok(noTrailing.path.test('/no-trailing/'), 'Single trailing slash is ok');
+    t.notOk(noTrailing.path.test('/no-trailing//'),
+            'Double trailing slash is not ok');
+    t.notOk(noTrailing.path.test('//no-trailing'),
+            'Double heading slash is not ok');
 
     t.end();
 });