Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(mux.go): use cleaned path as URL.Path #674

Closed
wants to merge 2 commits into from

Conversation

with-shrey
Copy link

Fixes #673

Summary of Changes

  1. removed the redirection on path mismatch after cleanup
  2. used the cleaned up path for finding Path match

PS: Make sure your PR includes/updates tests! If you need help with this part, just ask!
Will Update tests if the approach looks good

Copy link

@miicchelle miicchelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chaudyg
Copy link

chaudyg commented May 18, 2022

The go http servers follows the same convention to redirect when the request path isn't clean. See here. I am unsure why ? It could be to protect against path traversal? I am might be worth investigating further before diverging from this behaviour.

@with-shrey
Copy link
Author

@chaudyg that could be true for the HTML website server.

But for REST Api based servers, this is not very intuitive as per my understanding

@with-shrey
Copy link
Author

with-shrey commented May 30, 2022

@elithrar could use your advice here

@elithrar
Copy link
Contributor

@elithrar could use your advice here

Needs positive & negative tests - for the mentioned issue and the delated code.

Copy link
Contributor

@elithrar elithrar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need tests to demonstrate this fixes #673 and also doesn't break existing users.

@elithrar
Copy link
Contributor

@chaudyg that could be true for the HTML website server.

But for REST Api based servers, this is not very intuitive as per my understanding

There's no difference here: these are web servers. We can't break existing cases.

@amustaque97
Copy link
Contributor

I'm not discouraging anyone here. Changes proposed in the PR are not required and we can mark it as NA.
Let me know what do you think?

Reason is because as @chaudyg mentioned already in comment net/http/server.go file we have implemented Redirect and same we're doing it in our mux. Removing Redirect from mux will not align with the original implementation.

@with-shrey
Copy link
Author

with-shrey commented May 31, 2022 via email

@amustaque97
Copy link
Contributor

Makes sense

On Tue, 31 May, 2022, 3:31 pm Mustaque Ahmed, @.> wrote: I'm not discouraging anyone here. Changes proposed in the PR are not required and we can mark it as NA. Let me know what do you think? Reason is because as @chaudyg https://github.com/chaudyg mentioned already in comment <#674 (comment)> net/http/server.go file we have implemented Redirect and same we're doing it in our mux. Removing Redirect from mux will not align with the original implementation. — Reply to this email directly, view it on GitHub <#674 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGAVQE4MQOWMLQDOVHV3FJDVMXPRLANCNFSM5U5DKZRA . You are receiving this because you authored the thread.Message ID: @.>

@elithrar we can close the PR and its related issue.

@coreydaley
Copy link
Contributor

superseded by #578

@coreydaley coreydaley closed this Aug 17, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug] Wrong http method when clean path over an invalid path
6 participants