Add support for strict routing

[lukealbao]

Not sure if the idea was to have it opt-in, but that's what I've done here, since the original issue asked for an Express-like API – but I think that is the opposite of current behavior. Is that ok?

• Adds a new boolean parameter for createServer (strict)
• Compiles and matches paths using non-strict mode by default

Cf. #692

[DonutEspresso]

Thank you, this looks great! Love the tests. 👍

Honestly LGTM, if I can get another set of eyes here. My only suggestion would be possibly giving the option a more specific name, like strictRouting or something? Since as a server option it's less clear what strict means.

Now that I think about it, I think the implication of this new option is that the sanitizePath plugin should no longer strip trailing slashes. I think we'll probably want to rev restify-plugins with a similar change too. I'll add to the master ticket.

[micahr]

Implementation looks good to me. Thanks for the tests too!

[lukealbao]

@DonutEspresso I am not terribly familiar with the plugin, but I believe that this would obviate the need for the path sanitizer. The path regex gets compiled such that multiple slashes in the request will match the intended path (line 104), so the only functionality left over for the path sanitizer is the trailing slash.

[lukealbao]

Hmm, probably worth pointing out, then, that as of e4303e0 even in non-strict mode a trailing slash will match at most one trailing slash, so that may affect how you want to approach changes to sanitizePath (or change the regex operator used here).

[DonutEspresso]

Hmm that's a good observation, this means it's no longer possible to receive sloppy URLs if that's what you want. Given how barebones restify is I'm less comfortable with breaking that behavior for some of the stricter back end systems that use it. Here's the sanitize plugin:

https://github.com/restify/plugins/blob/master/lib/pre/prePath.js

I think it might be less surprising to reduce scope of the regex operator to handle only the trailing scenario, thus allowing users to opt-in to sanitization via the plugin. Does that sound reasonable?

[lukealbao]

The regex operator you refer to – if it's the one that's added in this PR – never affects any intermediate slashes. compileURL already builds a regex that will match one or more intermediate slashes, regardless of whether or not the path has been sanitized by a plugin (cf. line 104). (That is, for any slashes besides a trailing slash, sanitizePath is effectively noop.)

I think we should distinguish between sloppy (duplicate) slashes and trailing slashes. Prior to this PR, restify matches all sloppy intermediate slashes and rejects all trailing slashes in a request; if the sanitize plugin is active, then it also matches trailing slashes, whether sloppy or not. With the addition of this PR:

1. Non-strict will accept all sloppy intermediate slashes.
2. Non-strict will reject a sloppy trailing slash, due to an at-most-one regex operator. **
3. Strict mode will accept all sloppy intermediate slashes.
4. Strict mode will accept a trailing slash only if the path is defined to have one.
5. Strict mode will reject any request that contains sloppy trailing slashes.
6. Strict mode (with the sanitize plugin active) will always return 404 for a route defined on /foo/, because the plugin will strip the trailing slash from the request. **

I think only 2 and 6 are in need of attention. The rest seem reasonable enough to me. For (2), I think it does make sense to add \\/+ instead of \\/, because that's the way intermediate slashes are handled in compileUrl. For (6), that seems a question that is meant for the core team.

One suggestion for (6) would be to change the plugin to essentially be a deduplicator, rather than also removing any trailing slashes altogether. If we do that, and change the regex operator for the trailing slash, the behavior would (should) look like this:

Route /foo/bar/

request(/foo//bar//) -> (strict) -> reject
request(/foo//bar/) -> (strict) -> accept
request(/foo//bar//) -> (strict, sanitize) -> accept (fix #6)

request(/foo//bar) -> (non-strict) -> accept
request(/foo//bar//) -> (non-strict) -> accept (fix #2)

-------------

Route /foo/bar

request(/foo//bar/) -> (strict) -> reject
request(/foo//bar) -> (strict) -> accept
request(/foo//bar//) -> (strict, sanitize) -> reject

request(/foo//bar) -> (non-strict) -> accept
request(/foo//bar//) -> (non-strict) -> accept (fix #2)

Maybe that's helpful?

[DonutEspresso]

Apologies for not being very clear - as you figured out, I was really only talking about sloppy trailing slashes. I think both of your suggestions for 2) and 6) are spot on.

Would you mind making the updates for 2) in this PR? I'll tackle 6) separately in the other repo. Thanks for sticking with this despite the back and forth - your thoroughness is very much appreciated!

[DonutEspresso]

Also, any thoughts from the peanut gallery on renaming strict option to strictRouting or trailingSlash given the behavior expected from this option?

[lukealbao]

Hey, you're welcome! Glad I could contribute. :)

I'll keep an eye on this in case folks want to change that parameter name.

[DonutEspresso]

@lukealbao, we chatted offline and I think everyone likes strictRouting. When you get a chance to make that change, let's get this thing in. 👍

[DonutEspresso]

Hmm, looks like there's some conflicts - do you mind doing a quick rebase against 5.x branch?

[DonutEspresso]

🎉

[lukealbao]

💥