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: improve documentation for Request.Write behavior regarding headers it handles itself #68635

Open
conradsmi opened this issue Jul 29, 2024 · 2 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@conradsmi
Copy link

Go version

go version go1.22.5 darwin/arm64

Output of go env in your module/workspace:

Not relevant; documentation issue

What did you do?

I attempted to set the Host header via Request.Header.Set("Host", "www.example.org") and invoked the request.

See this modified playground link (credit to the original here); this just dumps the request info using httputil.DumpRequestOut.

What did you see happen?

Running the playground link reveals that the host which was actually sent was www.example.org, which was set via req.Host = "www.example.org". However, commenting this line out (and not req.Header.Set("Host", "www.example2.org") below it) and running it again results in the host becoming 1.2.3.4 instead, suggesting that whatever Host header is set using Header.Set is ignored.

What did you expect to see?

I expected Request.Header.Set("Host", "www.example2.org") to result in the request's Host header becoming www.example2.org, especially when it is not explicitly set otherwise.

The documentation is not very clear that the value of the Host header specifically is one that is ignored by Request.Write (well, it's there; just substantially hidden, in my opinion). Here are the steps I took to attempt to find this out starting from https://pkg.go.dev/net/http#Request:

// For client requests, the URL's Host specifies the server to
// connect to, while the Request's Host field optionally
// specifies the Host header value to send in the HTTP
// request.
URL *url

Okay, got it; I can use the provided Request.Host field to set the Host header. But still, why does Request.Header.Set("Host", "www.example2.org") not work?

// For client requests, certain headers such as Content-Length
// and Connection are automatically written when needed and
// values in Header may be ignored. See the documentation
// for the Request.Write method.
Header Header

Okay, there is a clue that this could indeed be happening with Host... let's check out Request.Write:

func (*Request) Write
func (r *Request) Write(w io.Writer) error
Write writes an HTTP/1.1 request, which is the header and body, in wire format. This method consults the following fields of the request:

Host
URL
Method (defaults to "GET")
Header
ContentLength
TransferEncoding
Body

If Body is present, Content-Length is <= 0 and [Request.TransferEncoding] hasn't been set to "identity", Write adds "Transfer-Encoding: chunked" to the header. Body is closed after it is sent.

Hm, there's nothing here that confirms what was said in the Header comments. The closest answer I can find in the documentation is actually right underneath Request.Write, in Request.WriteProxy:

func (r *Request) WriteProxy(w io.Writer) error
WriteProxy is like Request.Write but writes the request in the form expected by an HTTP proxy. In particular, Request.WriteProxy writes the initial Request-URI line of the request with an absolute URI, per section 5.3 of RFC 7230, including the scheme and host. In either case, WriteProxy also writes a Host header, using either r.Host or r.URL.Host.

So, after reading all of that, I guess the Host header can only come from r.Host and r.URL.Host. And looking at the source code confirms this. But it really would have been nice if, say, Request.Write outright stated something along the lines of "The following headers will be overridden: ...". I expected such a sentence since it was hinted at in the Request.Header documentation.


This is not a new problem by any means; plenty of people have happened upon this behavior in the past (see 1 and 2). I think a documentation change as I suggested would make this more clear.

@ianlancetaylor
Copy link
Contributor

CC @neild

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants