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: StripPrefix creates a new request, confusing middleware #20948

Closed
heschi opened this issue Jul 7, 2017 · 5 comments
Closed

net/http: StripPrefix creates a new request, confusing middleware #20948

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

Comments

@heschi
Copy link
Contributor

heschi commented Jul 7, 2017 •
edited

As of http://golang.org/cl/36483, StripPrefix creates a new request with the modified URL rather than modifying the existing request. That means that the identify of the request object can change between different handlers. If an earlier handler used a map[*http.Request], and then a later handler tries to read that value, it won't find it. This broke one important Google-internal use case, and a Github search shows a lot of such maps, so it may be worth addressing. I don't know how given the conflicting requirement to not modify the request.
@dsnet
Copy link
Member

dsnet commented Jul 7, 2017

\cc @bradfitz

@dsnet dsnet added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 7, 2017
@dsnet dsnet added this to the Go1.9 milestone Jul 7, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Jul 7, 2017

@quentinmit had brought this up as a concern at #18952 (comment)

I'm still inclined to fix the affected code, though. Relying on *http.Request pointer equality down a chain of wrappers already seems dicey. I'll look at the internal bug.

@heschi
Copy link
Contributor Author

heschi commented Jul 7, 2017

Perhaps a release note?

@rsc
Copy link
Contributor

rsc commented Jul 11, 2017

Agree with Brad. It's fine to add a release note warning, but we've been making copies of Requests since at least Go 1.8's WithContext. Middleware should also be using context at this point I would imagine.

@gopherbot
Copy link

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

@romainmenke romainmenke mentioned this issue Jul 31, 2017
Open
@golang golang locked and limited conversation to collaborators Jul 12, 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
5 participants
@bradfitz @rsc @dsnet @gopherbot @heschi