-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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/httputil: reverseproxy.removeConnectionHeaders not removing all headers #30303
Comments
Thank you for this report @jonathon-L and welcome to the Go project! In deed, as you've reported and noticed this is a minor oversight that comes from the usage of w.Header().Set("Connection", "Upgrade, "+fakeConnectionToken)
w.Header().Set("Upgrade", "should be deleted")
w.Header().Set(fakeConnectionToken, "should be deleted") instead of package main
import (
"bytes"
"fmt"
"net/http"
)
func main() {
buf := new(bytes.Buffer)
h1 := make(http.Header)
h1.Add("C", "a")
h1.Add("C", "b")
h1.Write(buf)
fmt.Printf("header by .Add on wire:\n%#v\n\n", buf.String())
buf.Reset()
h2 := make(http.Header)
h2.Add("C", "a, b")
h2.Write(buf)
fmt.Printf("header by ',' values on wire:\n%#v\n\n", buf.String())
hm1 := map[string][]string(h1)
fmt.Printf("1. Cast to map[string][]string: %#v\n", hm1)
hm2 := map[string][]string(h2)
fmt.Printf("2. Cast to map[string][]string: %#v\n", hm2)
} which produces header by .Add on wire:
"C: a\r\nC: b\r\n"
header by ',' values on wire:
"C: a, b\r\n"
1. Cast to map[string][]string: map[string][]string{"C":[]string{"a", "b"}}
2. Cast to map[string][]string: map[string][]string{"C":[]string{"a, b"}} Please feel free to send a patch( for Go1.13 and tag @bradfitz and myself, where the patch will involve your suggestion and updating the test in go/src/net/http/httputil/reverseproxy_test.go Line 153 in 01f34cb
to for instance w.Header().Add("Connection", "Upgrade, "+fakeConnectionToken)
w.Header().Add("Connection", "X-Extra")
w.Header().Set("Upgrade", "should be deleted")
w.Header().Set(fakeConnectionToken, "should be deleted")
w.Header().Set("X-Extra", "should be deleted") and perhaps a tighter test to check for the serialized output(number of expected headers and values) to augment the |
@jonathon-L are you working on this? If not, I'd be glad to give it a shot. |
Hey @dphan72, I am working on this, I should have a PR ready tomorrow. |
@jonathon-L Gentle nudge for updates on this. |
Change https://golang.org/cl/166298 mentions this issue: |
Kindly paging you @jonathon-L to take a look at the comments that I left on your CL in March. Thank you! |
Will take a look. Thanks for the reminder, was off my radar. |
Problem
In my testing, the reverse proxy doesn't remove all the header fields from a message that are listed in the
Connection
header.Relevant code lines
(Header).Get
only returns the first value if there are multiple values (also thestrings.Split
appears not do do anything, since only one value is returned).Per the comment on
(Header).Get
:Example
I've set this out in the playground. In this example, the upstream is returning the following headers:
However, the output shows that
Thing2
header field is still present.Per RFC 7230, since
Thing1
andThing2
are both their own header fields, and also values ofConnection
, my understanding is that all three fields should be removed.Potential Solution
If my understanding of this is correct, then we could do something like the following below, which would iterate over all the values of
Connection
.The text was updated successfully, but these errors were encountered: