fix: handle urls with multiple slashes #16527
Conversation
There was a problem hiding this comment.
LGTM, but I'd like to get an OK from @mafredri as well. Multiple slashes is technically valid in the case of /api/v2/something/$FOO/bar if $FOO can be an empty string. I don't see us needing this in the future, but I'm also no soothsayer.
| } | ||
|
|
||
| // Normalize multiple slashes to a single slash | ||
| newPath := multipleSlashesRe.ReplaceAllString(path, "/") |
There was a problem hiding this comment.
nit, non-blocking: I wonder what the cost of a regex replace is versus iterating over the string once?
There was a problem hiding this comment.
I had that thought to fiddle with chars, but blindly assumed that regexp is safer and easier for devs to comprehend. Thanks for the comment anyway!
coderd/coderd.go
Outdated
| if rctx != nil { | ||
| rctx.RoutePath = newPath | ||
| } | ||
| r.URL.Path = newPath |
There was a problem hiding this comment.
In some cases we may overwrite RoutePath with Path, and vice-versa. Depending on which is missing. Is there any risk with this approach?
Looking at existing middleware, they seem to take a conditional approach for the assignment of both values: https://github.com/go-chi/chi/blob/e846b8304c769c4f1a51c9de06bebfaa4576bd88/middleware/strip.go#L24-L28
There was a problem hiding this comment.
I don't think it is a great deal, but let's keep it consistent :) Thanks for raising this
This is a good point @johnstcn, I strongly hope we will never end up there as it's a use-case that's better not being supported IMO 😄 |
Fixes: #9877
This PR introduces another middleware to rewrite URLs when multiple slashes are used.
Testing: