Skip to content
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

Jump to bottom

net/http: Unclear if modifying http.Request.URL in ServeHTTP is legal. #18952

Closed
dmitshur opened this issue Feb 6, 2017 · 7 comments
Closed

net/http: Unclear if modifying http.Request.URL in ServeHTTP is legal. #18952

dmitshur opened this issue Feb 6, 2017 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.9

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 6, 2017

As of https://golang.org/cl/21530 (/cc @bradfitz), which resolved #14940, http.Handler doc says about ServeHTTP(http.ResponseWriter, *http.Request):

// Except for reading the body, handlers should not modify the
// provided Request.

Yet here's what http.StripPrefix does:

return HandlerFunc(func(w ResponseWriter, r *Request) {
...
	r.URL.Path = p

Is modifying http.Request.URL not considered as modifying http.Request itself? If so, I believe the documentation needs to be more clear about that.

Or is there a bug in http.StripPrefix?
dmitshur added a commit to shurcooL/home that referenced this issue Feb 6, 2017
@dmitshur
Print original req.URL.Path in case ServeHTTP modifies it.
ee02a0c 
Unsure if ServeHTTP is allowed to modify req.URL.Path, related to
golang/go#18952.
@odeke-em
Copy link
Member

odeke-em commented Feb 6, 2017

/cc @bradfitz

@bradfitz
Copy link
Contributor

bradfitz commented Feb 6, 2017

Yeah, I suppose StripPrefix should follow the rules (which were written after StripPrefix was).

Anybody want to send a CL?

@bradfitz bradfitz added this to the Go1.9 milestone Feb 6, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 6, 2017
@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 6, 2017

So you're confirming that modifying req.URL is considered against the contract. I wonder if gorilla/mux and other libraries currently do that or not. If they do, they'll need to be updated.

I can send a CL if you want to help me understand the way you expect this resolved.

Would we need to make a shallow copy of *http.Request, and a copy of *url.URL, then modify the copied request's URL? Is there already a private helper for making a request copy I should use?

@gopherbot
Copy link

CL https://golang.org/cl/36483 mentions this issue.

dmitshur added a commit to shurcooL/home that referenced this issue Mar 8, 2017
@dmitshur
Make a copy of *http.Request before modifying req.URL.Path.
4556ed9 
Use http.StripPrefix from Go 1.9, since it has the bug fixed.

Related to golang/go#18952.
@quentinmit
Copy link
Contributor

I'm reopening this because apparently there is code out there that will break as a result of creating a new *http.Request instead of editing the existing one. (Code that builds a map[*http.Request]extraData.)

Not that we necessarily need to revert, but someone needs to think about which state is worse.

@quentinmit quentinmit reopened this Mar 21, 2017
@quentinmit quentinmit added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 21, 2017
@rsc
Copy link
Contributor

rsc commented Jun 26, 2017

ping @bradfitz

@bradfitz
Copy link
Contributor

There used to be somewhat valid reasons to have map[*http.Request]extraData but with per-Request contexts and net/http/httptrace, there are better ways.

Let's see how it goes with the beta and see if people are actually affected.

But @quentinmit, if you know of concrete examples of such code, let me know and we can reconsider. But if this is just hypothetical, I'm inclined to wait and see. We already have code which modifies the passed-through *Request. This is just one more.

@bradfitz bradfitz mentioned this issue Jul 7, 2017
Closed
kevinburke added a commit to kevinburke/handlers that referenced this issue Dec 15, 2017
@kevinburke
Make copies of HTTP requests
e569a51 
See
https://go-review.googlesource.com/c/go/+/36483/3/src/net/http/server.go,
golang/go#18952 for more information on why
this is necessary.
dmitshur added a commit to shurcooL/home that referenced this issue May 7, 2018
@dmitshur
Revert to StripPrefix from net/http rather than copy.
8dfdd4c 
Go 1.9 is long since out, so the copy isn't needed anymore.

This reverts a part of commit 4556ed9.

Updates golang/go#18952.
@golang golang locked and limited conversation to collaborators Jun 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
6 participants
@bradfitz @rsc @quentinmit @dmitshur @odeke-em @gopherbot