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

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

Comments

@dmitshur
Copy link
Member

@dmitshur 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
Unsure if ServeHTTP is allowed to modify req.URL.Path, related to
golang/go#18952.
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Feb 6, 2017

/cc @bradfitz

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz 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 label Feb 6, 2017
@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 7, 2017

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

@gopherbot gopherbot closed this in af59742 Feb 8, 2017
dmitshur added a commit to shurcooL/home that referenced this issue Mar 8, 2017
Use http.StripPrefix from Go 1.9, since it has the bug fixed.

Related to golang/go#18952.
@quentinmit

This comment has been minimized.

Copy link
Contributor

@quentinmit quentinmit commented Mar 21, 2017

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 and removed NeedsFix labels Mar 21, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jun 26, 2017

ping @bradfitz

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jun 26, 2017

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 closed this Jun 26, 2017
kevinburke added a commit to kevinburke/handlers that referenced this issue Dec 15, 2017
dmitshur added a commit to shurcooL/home that referenced this issue May 7, 2018
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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.