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
jonathonlacher opened this issue Feb 18, 2019 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@jonathonlacher
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 22, 2019
@dmitshur dmitshur added this to the Go1.13 milestone Feb 22, 2019
@odeke-em
Copy link
Member

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.

@odeke-em odeke-em added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 24, 2019
@dphan72
Copy link
Contributor

dphan72 commented Mar 1, 2019

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

@jonathonlacher
Copy link
Contributor Author

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

@kshitij10496
Copy link
Contributor

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

@gopherbot
Copy link

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

@odeke-em
Copy link
Member

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

@jonathonlacher
Copy link
Contributor Author

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

@golang golang locked and limited conversation to collaborators May 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants