fix(router): handle non-trailing slashes correctly #1478

hekike · 2017-09-09T12:56:34Z

Issues

Closes:

#1464

Non-trailing slash leads to incorrect routing match (and parameter parsing).

Changes

Fix router regexp matching with non-trailing slash(es)

retrohacker · 2017-09-12T16:01:01Z

test/router.test.js

    t.ok(trailing.path.test('/trailing'));

    var noTrailing = server.router.routes.GET[1];
    t.ok(noTrailing.path.test('/no-trailing'));
-    t.ok(noTrailing.path.test('//no-trailing//'));


I'm not sure I have enough context to understand why this change is necessary. Why did we change an existing test as opposed to adding a separate test for a single leading /?

The original test case is not related to the code what it tests. This test should focus to double no-trailing slashes.

Sry, I mean it should focus to slashes at the end of the path :P

DonutEspresso · 2017-09-13T05:20:08Z

@hekike thanks for taking this on! I think you're right, the behavior should be to not remove double slashes by default - that should be taken care of by the dedupeSlashes plugin. This is going to be a breaking change, perhaps we can slide this in before we cut 6.x @retrohacker?

hekike · 2017-09-13T06:02:18Z

Apologize I didn't explain very well the context of this pr.
To be on the same page, the current PR:

Allows only one slash at the middle to prevent route pram parsing issues
Allows zero or more slashes at the end

I believe what @DonutEspresso proposes is a one step further and wouldn't allow, more than one slash at the end neither.

The new behavior after this proposal would be:

Allows only one slash at the middle to prevent route pram parsing issues
Allows zero or one slash at the end

retrohacker · 2017-09-14T22:37:38Z

Thanks for tackling this @hekike!

I'm 👍 on leaving the path exactly as is, and allowing the user to specify the dedupe behavior through a plugin. My main motivation is that we should do as little "magic" as possible, and changing paths before they get to the user seems like magic. I think our plugin logic should behave like merge_slashes in nginx. If users need a different behavior, they can specify that as their own plugin.

retrohacker · 2017-09-14T22:39:09Z

So, tl;dr, I propose:

We remove all url scrubbing logic from the router
We update our dedupe plugin to match NGinx's behavior
We document how you can specify your own url transformer through server.pre.

retrohacker · 2017-09-14T22:42:48Z

As for holding v6, I'd suggest we go ahead and ship. We've been in a state of perpetually staged breaking changes before (as @yunong pointed out yesterday) and it wasn't a great place to be.

Instead, I'd suggest we create a new branch called next that we merge BREAKING CHANGEs into until we think we have enough for a new major release. Then we can cut from there. This gives us two advantages:

We don't have to block bug/feature fixes because BREAKING is on the current master (like we are doing now)
Users who need the new BREAKING changes can opt into tracking the next branch by using restify/node-restify#next in their package.json.

BREAKING CHANGE: use server.pre(restify.plugins.pre.dedupeSlashes()) to deal with double slashes

…cases

hekike · 2017-09-28T07:56:31Z

@retrohacker I removed the double slash merging magic from the router, see my latest commit.
The restify.plugins.pre.dedupeSlashes() plugin is already documented.

I also refactored the dedupeSlashes test to cover cases with more than two slashes.
If I understand correctly restify's dedupleSlashes plugin already worked like nginx's merge_slashes option, just the test didn't point it out earlier:

Enables or disables compression of two or more adjacent slashes in a URI into a single slash. - merge_slashes

retrohacker

This LGTM 👍

retrohacker · 2017-10-09T15:19:42Z

lib/router.js

@@ -103,7 +103,7 @@ function compileURL(options) {
            return (false);
        }

-        pattern += '\\/+';
+        pattern += '\\/{1}';


I think this quantifier is unnecessary

retrohacker · 2017-10-09T15:20:18Z

lib/router.js

@@ -134,7 +134,7 @@ function compileURL(options) {
    }

    if (!options.strict) {
-        pattern += '[\\/]*';
+        pattern += '[\\/]?';


I think this list is unnecessary

retrohacker · 2017-10-09T19:52:41Z

This shouldn't merge into master unless we want to cut a new release since it is going to be a breaking change. Can we open this PR against a new branch next?

hekike · 2017-10-09T20:51:18Z

@retrohacker I've no permission to create a next branch in this repo, could you create one? I cannot open a PR to a non-existent branch.

hekike · 2017-10-09T20:57:31Z

Closed by the favor of #1512
cc @retrohacker

hekike force-pushed the fix/router-non-trailing-double-slash branch 2 times, most recently from 5c517b2 to 54f8430 Compare September 9, 2017 13:05

hekike mentioned this pull request Sep 9, 2017

Optional parameter issue with NON-trailing double slash #1464

Closed

hekike force-pushed the fix/router-non-trailing-double-slash branch 2 times, most recently from 765769e to 0c3862d Compare September 10, 2017 06:40

retrohacker requested changes Sep 12, 2017

View reviewed changes

Peter Marton added 2 commits September 28, 2017 08:58

fix(router): handle non-trailing slashes correctly

97a6e4c

fix(router): remove do not merge double slashes by default

d83ef0f

BREAKING CHANGE: use server.pre(restify.plugins.pre.dedupeSlashes()) to deal with double slashes

hekike force-pushed the fix/router-non-trailing-double-slash branch from 0c3862d to d83ef0f Compare September 28, 2017 07:31

test(plugins/dedupeSlashes): refactor test to cover multiple slashes …

56ec662

…cases

retrohacker approved these changes Oct 9, 2017

View reviewed changes

refactor(router): remove unecessary quantifier

b72f786

hekike mentioned this pull request Oct 9, 2017

Handle non-trailing slashes correctly #1512

Merged

hekike closed this Oct 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): handle non-trailing slashes correctly #1478

fix(router): handle non-trailing slashes correctly #1478

hekike commented Sep 9, 2017 •

edited

retrohacker Sep 12, 2017

hekike Sep 12, 2017

hekike Sep 12, 2017

DonutEspresso commented Sep 13, 2017

hekike commented Sep 13, 2017

retrohacker commented Sep 14, 2017

retrohacker commented Sep 14, 2017

retrohacker commented Sep 14, 2017

hekike commented Sep 28, 2017

retrohacker left a comment

retrohacker Oct 9, 2017

retrohacker Oct 9, 2017

retrohacker commented Oct 9, 2017

hekike commented Oct 9, 2017

hekike commented Oct 9, 2017

fix(router): handle non-trailing slashes correctly #1478

fix(router): handle non-trailing slashes correctly #1478

Conversation

hekike commented Sep 9, 2017 • edited

Issues

Changes

retrohacker Sep 12, 2017

Choose a reason for hiding this comment

hekike Sep 12, 2017

Choose a reason for hiding this comment

hekike Sep 12, 2017

Choose a reason for hiding this comment

DonutEspresso commented Sep 13, 2017

hekike commented Sep 13, 2017

retrohacker commented Sep 14, 2017

retrohacker commented Sep 14, 2017

retrohacker commented Sep 14, 2017

hekike commented Sep 28, 2017

retrohacker left a comment

Choose a reason for hiding this comment

retrohacker Oct 9, 2017

Choose a reason for hiding this comment

retrohacker Oct 9, 2017

Choose a reason for hiding this comment

retrohacker commented Oct 9, 2017

hekike commented Oct 9, 2017

hekike commented Oct 9, 2017

hekike commented Sep 9, 2017 •

edited