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/httputil: ReverseProxy.handleUpgradeResponse should copy response header back to http.ResponseWriter #41634

Closed
aofei opened this issue Sep 26, 2020 · 5 comments
Closed

net/http/httputil: ReverseProxy.handleUpgradeResponse should copy response header back to http.ResponseWriter #41634

aofei opened this issue Sep 26, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@aofei
Copy link
Contributor

aofei commented Sep 26, 2020

In the ReverseProxy.ServeHTTP, we should always copy the response header returned by the upstream back to our http.ResponseWriter (when everything is fine). But currently we've missed this point in the implementation of WebSocket reverse proxy, which may affect users. For example, someone may want to log response headers in their middleware.
@gopherbot
Copy link

Change https://golang.org/cl/257777 mentions this issue: net/http/httputil: copy response header back to http.ResponseWriter in ReverseProxy.handleUpgradeResponse

@davecheney
Copy link
Contributor

@aofei thank you for raising this issue. Would you be able to explain why this change is needed. For example, what can you not do currently that you would be able to do with this change?

@aofei
Copy link
Contributor Author

aofei commented Sep 26, 2020

Hi @davecheney,

The main purpose of this change is to make the behavior of WebSocket reverse proxy consistent with other types.

In other types of reverse proxy (non-WebSocket) scenarios, we can directly access our http.ResponseWriter.Header to get all the response headers returned by the upstream. Thanks to this line:

go/src/net/http/httputil/reverseproxy.go

Line 313 in 8266570

copyHeader(rw.Header(), res.Header)

But for the WebSocket reverse proxy, we can't do this. Because the ReversePorxy.handleUpgradeResponse writes those response headers directly to the client through the hijacked connection, without copying them to our http.ResponseWriter.Header. In this case, those response headers are "invisible" to us (although we can still get them by hacking the ReverseProxy.ModifyResponse).

@andybons
Copy link
Member

@bradfitz

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 29, 2020
@andybons andybons added this to the Unplanned milestone Sep 29, 2020
@bradfitz
Copy link
Contributor

bradfitz commented Oct 6, 2020

Seems reasonable. Leaving for @neild for reviewing the implementation & test.

@golang golang locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@bradfitz @davecheney @andybons @aofei @gopherbot