net/http: make Transport.RoundTrip close body on any invalid request

Fixes #35015

Change-Id: I7a1ed9cfa219ad88014aad033e3a01f9dffc3eb3
Reviewed-on: https://go-review.googlesource.com/c/go/+/202239
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index bd9717ea1501e414e6456a066f2b11de6ca87701..ceda34c741a19d9f4edca97dec18bc985438457a 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -469,10 +469,12 @@ func (t *Transport) roundTrip(req *Request) (*Response, error) {
        if isHTTP {
                for k, vv := range req.Header {
                        if !httpguts.ValidHeaderFieldName(k) {
+                               req.closeBody()
                                return nil, fmt.Errorf("net/http: invalid header field name %q", k)
                        }
                        for _, v := range vv {
                                if !httpguts.ValidHeaderFieldValue(v) {
+                                       req.closeBody()
                                        return nil, fmt.Errorf("net/http: invalid header field value %q for key %v", v, k)
                                }
                        }

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 0fe1283d97e8afda17358dcff1d747e9fbec4a59..c84d3ea1d6b5d361f913382229ab66250ecd19b1 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -5730,21 +5730,87 @@ func (bc *bodyCloser) Read(b []byte) (n int, err error) {
        return 0, io.EOF
 }
 
-func TestInvalidMethodClosesBody(t *testing.T) {
-       cst := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {}))
+// Issue 35015: ensure that Transport closes the body on any error
+// with an invalid request, as promised by Client.Do docs.
+func TestTransportClosesBodyOnInvalidRequests(t *testing.T) {
+       cst := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               t.Errorf("Should not have been invoked")
+       }))
        defer cst.Close()
-       var bc bodyCloser
+
        u, _ := url.Parse(cst.URL)
-       req := &Request{
-               Method: " ",
-               URL:    u,
-               Body:   &bc,
-       }
-       _, err := DefaultClient.Do(req)
-       if err == nil {
-               t.Fatal("Expected an error")
+
+       tests := []struct {
+               name    string
+               req     *Request
+               wantErr string
+       }{
+               {
+                       name: "invalid method",
+                       req: &Request{
+                               Method: " ",
+                               URL:    u,
+                       },
+                       wantErr: "invalid method",
+               },
+               {
+                       name: "nil URL",
+                       req: &Request{
+                               Method: "GET",
+                       },
+                       wantErr: "nil Request.URL",
+               },
+               {
+                       name: "invalid header key",
+                       req: &Request{
+                               Method: "GET",
+                               Header: Header{"💡": {"emoji"}},
+                               URL:    u,
+                       },
+                       wantErr: "invalid header field name",
+               },
+               {
+                       name: "invalid header value",
+                       req: &Request{
+                               Method: "POST",
+                               Header: Header{"key": {"\x19"}},
+                               URL:    u,
+                       },
+                       wantErr: "invalid header field value",
+               },
+               {
+                       name: "non HTTP(s) scheme",
+                       req: &Request{
+                               Method: "POST",
+                               URL:    &url.URL{Scheme: "faux"},
+                       },
+                       wantErr: "unsupported protocol scheme",
+               },
+               {
+                       name: "no Host in URL",
+                       req: &Request{
+                               Method: "POST",
+                               URL:    &url.URL{Scheme: "http"},
+                       },
+                       wantErr: "no Host",
+               },
        }
-       if !bc {
-               t.Fatal("Expected body to have been closed")
+
+       for _, tt := range tests {
+               t.Run(tt.name, func(t *testing.T) {
+                       var bc bodyCloser
+                       req := tt.req
+                       req.Body = &bc
+                       _, err := DefaultClient.Do(tt.req)
+                       if err == nil {
+                               t.Fatal("Expected an error")
+                       }
+                       if !bc {
+                               t.Fatal("Expected body to have been closed")
+                       }
+                       if g, w := err.Error(), tt.wantErr; !strings.Contains(g, w) {
+                               t.Fatalf("Error mismatch\n\t%q\ndoes not contain\n\t%q", g, w)
+                       }
+               })
        }
 }