Handle trailing slashes at the end of paths

[babolivier]

The HTTP routers Dendrite uses see /foo/bar and /foo/bar/ as two different routes. We'd ideally not want that, because Synapse and sytest sometimes add trailing slashes at the end of paths even though the specs doesn't use any.

This might require a bit of research but should be quite straightforward.

[brunnre8]

I could have a look if you'd like?

Just a question, gorillas/mux StrictSlash setting doesn't do what you want so we will need to write a non gorilla middleware for it.

Which endpoints do you want to handle in this way? All of them, including prometheus? Just the synapse ones in /api/... ?

[turt2live]

note that synapse is also picky about slashes, which may result in a fun time debugging if not considered carefully: matrix-org/synapse#3622

[brunnre8]

He, but I guess that doesn't mean that we can't "fix" it in dendrite does it? That just means that one also has to do the work in synapse if I understand correctly?

I mean, the core team needs to make the decision whether trailing slashes are significant or not. Then this can be implemented one way or the other.

[turt2live]

It should absolutely be fixed in Dendrite - that's a great thing to do. Just giving a head's up that going the other direction (Dendrite -> Synapse) is picky as well.

I'd hope that everyone agrees that trailing slashes are supposed to be insignificant.

[brunnre8]

I'd hope that everyone agrees that trailing slashes are supposed to be insignificant.

Oh boy, I just read a few blog posts / forum threads and people have very strong opinions about that.

Seems that there are strong voices against doing it in a REST interface.

Then again, I personally don't care either way so just tell me what to do and I'll happily do it 😉

[babolivier]

I could have a look if you'd like?

If you have some time and motivation to look into this, feel free to do so! :)

Which endpoints do you want to handle in this way? All of them, including prometheus? Just the synapse ones in /api/... ?

Ideally every endpoint managed by Dendrite, though we only really care about the public endpoints under /_matrix.

[anoadragon453]

I am up for removing all trailing slashes from all requests before passing them to handler functions, if that's possible.

Afaik no query parameters would have trailing slashes that would be worse off if they were chopped off.

[babolivier]

Well the issue is that it's gorilla/mux that does the matching: if you register a handler on a path without a trailing slash at the end then hit that path with a trailing slash the router will respond with a 404 code without passing the request to any of your handlers. We could do a very ugly thing and register two handlers for every path, one with a trailing slash and one without, but I'd like to avoid that as much as possible. We should look for another way to do it, if that's possible.

[anoadragon453]

https://godoc.org/github.com/gorilla/mux#Router.StrictSlash ?

…

On 1/18/19 1:50 PM, Brendan Abolivier wrote: Well the issue is that it's |gorilla/mux| that does the matching: if you register a handler on a path without a trailing slash at the end then hit that path with a trailing slash the router will respond with a |404| code without passing the request to any of your handlers. We could do a very ugly thing and register two handlers for every path, one with a trailing slash and one without, but I'd like to avoid that as much as possible. We should look for another way to do it, if that's possible. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#613 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABR7mBIjIGGr4GzRF-_-JgT2_gJuISg_ks5vEdE9gaJpZM4Z1Pnf>.

[babolivier]

As mentioned above, this doesn't fit our use case.

Possible ways to do this:

1. find a way to register some kind of middleware to catch the requests and remove any trailing slash from the URL
2. PR a "ignore trailing slash" feature to gorilla/mux

After irl discussions with @anoadragon453, we're likely going to go with option 2.

[brunnre8]

there's actually a nice blog post here https://natedenlinger.com/dealing-with-trailing-slashes-on-requesturi-in-go-with-mux/

[anoadragon453]

Huh, that should work! Might still send a PR to mux though to make other's lives easier.

[brunnre8]

Well, it's essentially option 1 babolivier proposed.

Regarding the PR to gorilla, that'd be great, however they objected to such a feature before gorilla/mux#30

[SUMUKHA-PK]

So its not option 2? Can I work on option 1?

[NJnisarg]

This issue seems to be still not resolved yet. Can I give it a shot?
I looked at the comment thread on this issue and seems like the article: https://natedenlinger.com/dealing-with-trailing-slashes-on-requesturi-in-go-with-mux/
might work. I was thinking of writing a middleware along the lines the common.WrapHandlerInCORS that simply drops the trailing slashes.

Correct me if my thinking path is wrong. I am new to dendrite and golang as well. Thanks!

[anoadragon453]

We need to do a comprehensive review of all the spec'd endpoints, but I believe Dendrite shouldn't just assume an endpoint with a trailing slash is the same as one without. For instance, there are some endpoints where a trailing slash means you're providing an empty path parameter: https://github.com/matrix-org/matrix-doc/issues/1916

As far as I can tell there shouldn't be any instances where we would want to handle an endpoint with and without a trailing slash as the exact same functionality, other than possibly keeping backwards compatibility with older Synapse versions that sent trailing slashes when they shouldn't have: matrix-org/synapse#4935 matrix-org/synapse#4840

[kegsay]

This happened, including weird fixes with SkipClean and UseEncodedPath. Related sytests (specifically with /rooms/{roomID}/state/{eventType}/ now pass.

[ghost]

var Router = mux.NewRouter().StrictSlash(true)
Just use this line of code instead of a regular router.

[nxvinh222]

var Router = mux.NewRouter().StrictSlash(true) Just use this line of code instead of a regular router.

this doesn't work with prefix path