-
Notifications
You must be signed in to change notification settings - Fork 498
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix sumdb/* paths when config.PathPrefix is set #1620
Conversation
http.StripPrefix will look at the entire request path when called, if we do not include config.PathPrefix then the StripPrefix call will never receive a valid path from the application and the user will always get a 404 error. There were no test where I could easily check this regression so I also added a few endpoint tests, the last test will fail with a 404 instead of 403 if this change in not applied.
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.
@elliotmr thank you for the fix and the tests look great!
Just a few comments. Let me know if you're busy and I'm happy to apply them myself, but otherwise take your time.
Appreciate the contribution 🎉
Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>
Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>
Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>
Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>
All changes looked good to me, I used TrimRight instead of TrimSuffix because I forgot that it existed. |
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.
This is great, thank you for the contribution 🎉
Fixes: #1619
http.StripPrefix will look at the entire request path when called,
if we do not include config.PathPrefix then the StripPrefix call
will never receive a valid path from the application and the user
will always get a 404 error.
There were no test where I could easily check this regression so
I also added a few endpoint tests, the last test will fail with
a 404 instead of 403 if this change in not applied.
What is the problem I am trying to address?
Describe the issue you have been trying to solve.
How is the fix applied?
Mention briefly how you have applied the fix.
What GitHub issue(s) does this PR fix or close?
Fixes #