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/httputil: reverseproxy.removeConnectionHeaders not removing all headers #30303

Closed
jonathon-l opened this issue Feb 18, 2019 · 7 comments

Comments

@jonathon-l
Copy link
Contributor

commented Feb 18, 2019

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 the strings.Split appears not do do anything, since only one value is returned).

Per the comment on (Header).Get:

To access multiple values of a key, or to use non-canonical keys, access the map directly.

Example

I've set this out in the playground. In this example, the upstream is returning the following headers:

Connection: Thing1, Thing2
Thing1: value1
Thing2: value2

However, the output shows that Thing2 header field is still present.

Thing1: 
Thing2: value2
Connection: 

Per RFC 7230, since Thing1 and Thing2 are both their own header fields, and also values of Connection, 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.

func removeConnectionHeaders(h http.Header) {
	if c := h["Connection"]; len(c) > 0 {
		for _, v := range c {
			if v = strings.TrimSpace(v); v != "" {
				h.Del(v)
			}
		}
	}
}

@dmitshur dmitshur added this to the Go1.13 milestone Feb 22, 2019

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

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 Header.Get instead of Header[key]. We introduced this in CL https://go-review.googlesource.com/c/go/+/28810/ but unfortunately our test only involved comma-concatenated header values

		w.Header().Set("Connection", "Upgrade, "+fakeConnectionToken)
		w.Header().Set("Upgrade", "should be deleted")
		w.Header().Set(fakeConnectionToken, "should be deleted")

instead of .Add values and this produced the subtle bug that you've noticed e.g.
https://play.golang.org/p/ionUhg2dYXp or inlined below in

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

func TestReverseProxyStripHeadersPresentInConnection(t *testing.T) {

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 .Get checks.

@dphan72

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@jonathon-l are you working on this? If not, I'd be glad to give it a shot.

@jonathon-l

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Hey @dphan72, I am working on this, I should have a PR ready tomorrow.

@kshitij10496

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@jonathon-l Gentle nudge for updates on this.
Let us know if you are facing any problem sending in the patch! 😄

@gopherbot

This comment has been minimized.

Copy link

commented Mar 8, 2019

Change https://golang.org/cl/166298 mentions this issue: net/http/httputil: remove all fields in Connection header

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Kindly paging you @jonathon-l to take a look at the comments that I left on your CL in March. Thank you!

@jonathon-l

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Will take a look. Thanks for the reminder, was off my radar.

@gopherbot gopherbot closed this in a5cea06 May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.