- Notifications
You must be signed in to change notification settings - Fork928
fix: handle urls with multiple slashes#16527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, but I'd like to get an OK from@mafredri as well. Multiple slashes istechnically 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 😄 |
b396408 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes:#9877
This PR introduces another middleware to rewrite URLs when multiple slashes are used.
Testing: