net/http/httputil: ReverseProxy req.Header copying is unclear

[FiloSottile]

ReverseProxy makes an attempt at not modifying the original req.Header

	// We are modifying the same underlying map from req (shallow
	// copied above) so we only copy it if necessary.
	copiedHeaders := false

However it skips the check for X-Forwarded-For

outreq.Header.Set("X-Forwarded-For", clientIP)

But more importantly, Director is called on the shallow copied req, and the NewSingleHostReverseProxy one modifies req.Header without making a new map

		if _, ok := req.Header["User-Agent"]; !ok {
			// explicitly disable User-Agent so it's not set to default value
			req.Header.Set("User-Agent", "")
		}

It looks to me like this is useless unless it's consistent. It should either make a new Header map before calling Director, or make no attempt at copying them at all.

See also #18326

[bradfitz]

I suppose it should just always copy.

This isn't a regression from Go 1.7 to master, is it? How is this related to #18326?

[FiloSottile]

Not a regression, even if 24d8f3f is a partial fix.

I linked #18326 because I expect it to end up into a discussion of whether you are supposed to modify req.Header, which is relevant to this issue. (And also because it's how I found #18326.)

[gopherbot]

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