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

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

Comments

Projects
None yet
5 participants
@heschik
Copy link
Contributor

commented Jul 7, 2017

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

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

\cc @bradfitz

@dsnet dsnet added the NeedsDecision label Jul 7, 2017

@dsnet dsnet added this to the Go1.9 milestone Jul 7, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

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.

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2017

Perhaps a release note?

@rsc

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Jul 12, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.