]> Cypherpunks repositories - gostls13.git/commitdiff
net/http, net/http/httptrace: make Transport support 1xx responses properly
authorBrad Fitzpatrick <bradfitz@golang.org>
Wed, 6 Jun 2018 22:50:01 +0000 (22:50 +0000)
committerBrad Fitzpatrick <bradfitz@golang.org>
Tue, 12 Jun 2018 13:42:28 +0000 (13:42 +0000)
Previously the Transport had good support for 100 Continue responses,
but other 1xx informational responses were returned as-is.

But per https://tools.ietf.org/html/rfc7231#section-6.2:

> A client MUST be able to parse one or more 1xx responses received
> prior to a final response, even if the client does not expect one. A
> user agent MAY ignore unexpected 1xx responses.

We weren't doing that. Instead, we were returning any 1xx that wasn't
100 as the final result.

With this change we instead loop over up to 5 (arbitrary) 1xx
responses until we find the final one, returning an error if there's
more than 5. The limit is just there to guard against malicious
servers and to have _some_ limit.

By default we ignore the 1xx responses, unless the user defines the
new httptrace.ClientTrace.Got1xxResponse hook, which is an expanded
version of the previous ClientTrace.Got100Continue.

Still remaining:

* httputil.ReverseProxy work. (From rfc7231#section-6.2: "A proxy MUST
  forward 1xx responses unless the proxy itself requested the
  generation of the 1xx response."). Which would require:

* Support for an http.Handler to generate 1xx informational responses.

Those can happen later. Fixing the Transport to be resilient to others
using 1xx in the future without negotiation (as is being discussed
with HTTP status 103) is most important for now.

Updates #17739

Change-Id: I55aae8cd978164643fccb9862cd60a230e430486
Reviewed-on: https://go-review.googlesource.com/116855
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
src/go/build/deps_test.go
src/net/http/httptrace/trace.go
src/net/http/transport.go
src/net/http/transport_test.go

index 67d1115017d65a332e7420a6232ce9967d3d8945..9d667b610728746110ae58483d3a0b6f82daae28 100644 (file)
@@ -416,7 +416,7 @@ var pkgDeps = map[string][]string{
                "syscall/js",
        },
        "net/http/internal":  {"L4"},
-       "net/http/httptrace": {"context", "crypto/tls", "internal/nettrace", "net", "reflect", "time"},
+       "net/http/httptrace": {"context", "crypto/tls", "internal/nettrace", "net", "net/textproto", "reflect", "time"},
 
        // HTTP-using packages.
        "expvar":             {"L4", "OS", "encoding/json", "net/http"},
index ea7b38c8fc6e0be959e65aa7e2f1648774cb16da..8033535670333b959297b7ca6c999f8fce3ae8ea 100644 (file)
@@ -11,6 +11,7 @@ import (
        "crypto/tls"
        "internal/nettrace"
        "net"
+       "net/textproto"
        "reflect"
        "time"
 )
@@ -107,6 +108,12 @@ type ClientTrace struct {
        // Continue" response.
        Got100Continue func()
 
+       // Got1xxResponse is called for each 1xx informational response header
+       // returned before the final non-1xx response. Got1xxResponse is called
+       // for "100 Continue" responses, even if Got100Continue is also defined.
+       // If it returns an error, the client request is aborted with that error value.
+       Got1xxResponse func(code int, header textproto.MIMEHeader) error
+
        // DNSStart is called when a DNS lookup begins.
        DNSStart func(DNSStartInfo)
 
index 3890f19af38dee28ed7b01597cc06519a485e6e8..9b5ea52c9b4c51eebaa78e0a78a25fadecf67aea 100644 (file)
@@ -21,6 +21,7 @@ import (
        "log"
        "net"
        "net/http/httptrace"
+       "net/textproto"
        "net/url"
        "os"
        "strings"
@@ -1641,26 +1642,42 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
                        trace.GotFirstResponseByte()
                }
        }
-       resp, err = ReadResponse(pc.br, rc.req)
-       if err != nil {
-               return
-       }
-       if rc.continueCh != nil {
-               if resp.StatusCode == 100 {
-                       if trace != nil && trace.Got100Continue != nil {
-                               trace.Got100Continue()
-                       }
-                       rc.continueCh <- struct{}{}
-               } else {
-                       close(rc.continueCh)
-               }
-       }
-       if resp.StatusCode == 100 {
-               pc.readLimit = pc.maxHeaderResponseSize() // reset the limit
+       num1xx := 0               // number of informational 1xx headers received
+       const max1xxResponses = 5 // arbitrary bound on number of informational responses
+
+       continueCh := rc.continueCh
+       for {
                resp, err = ReadResponse(pc.br, rc.req)
                if err != nil {
                        return
                }
+               resCode := resp.StatusCode
+               if continueCh != nil {
+                       if resCode == 100 {
+                               if trace != nil && trace.Got100Continue != nil {
+                                       trace.Got100Continue()
+                               }
+                               continueCh <- struct{}{}
+                               continueCh = nil
+                       } else if resCode >= 200 {
+                               close(continueCh)
+                               continueCh = nil
+                       }
+               }
+               if 100 <= resCode && resCode <= 199 {
+                       num1xx++
+                       if num1xx > max1xxResponses {
+                               return nil, errors.New("net/http: too many 1xx informational responses")
+                       }
+                       pc.readLimit = pc.maxHeaderResponseSize() // reset the limit
+                       if trace != nil && trace.Got1xxResponse != nil {
+                               if err := trace.Got1xxResponse(resCode, textproto.MIMEHeader(resp.Header)); err != nil {
+                                       return nil, err
+                               }
+                       }
+                       continue
+               }
+               break
        }
        resp.TLS = pc.tlsState
        return
index 57309bbac1fd5f3da6b32ea4ed376806ddc3960d..01a209c6330c6251f3faf62a772ea06cbb0ca2c2 100644 (file)
@@ -31,6 +31,7 @@ import (
        "net/http/httptrace"
        "net/http/httputil"
        "net/http/internal"
+       "net/textproto"
        "net/url"
        "os"
        "reflect"
@@ -2287,6 +2288,7 @@ Content-Length: %d
        c := &Client{Transport: tr}
 
        testResponse := func(req *Request, name string, wantCode int) {
+               t.Helper()
                res, err := c.Do(req)
                if err != nil {
                        t.Fatalf("%s: Do: %v", name, err)
@@ -2309,13 +2311,67 @@ Content-Length: %d
                req.Header.Set("Request-Id", reqID(i))
                testResponse(req, fmt.Sprintf("100, %d/%d", i, numReqs), 200)
        }
+}
 
-       // And some other informational 1xx but non-100 responses, to test
-       // we return them but don't re-use the connection.
-       for i := 1; i <= numReqs; i++ {
-               req, _ := NewRequest("POST", "http://other.tld/", strings.NewReader(reqBody(i)))
-               req.Header.Set("X-Want-Response-Code", "123 Sesame Street")
-               testResponse(req, fmt.Sprintf("123, %d/%d", i, numReqs), 123)
+// Issue 17739: the HTTP client must ignore any unknown 1xx
+// informational responses before the actual response.
+func TestTransportIgnore1xxResponses(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+       cst := newClientServerTest(t, h1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+               conn, buf, _ := w.(Hijacker).Hijack()
+               buf.Write([]byte("HTTP/1.1 123 OneTwoThree\r\nFoo: bar\r\n\r\nHTTP/1.1 200 OK\r\nBar: baz\r\nContent-Length: 5\r\n\r\nHello"))
+               buf.Flush()
+               conn.Close()
+       }))
+       defer cst.close()
+       cst.tr.DisableKeepAlives = true // prevent log spam; our test server is hanging up anyway
+
+       var got bytes.Buffer
+
+       req, _ := NewRequest("GET", cst.ts.URL, nil)
+       req = req.WithContext(httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
+               Got1xxResponse: func(code int, header textproto.MIMEHeader) error {
+                       fmt.Fprintf(&got, "1xx: code=%v, header=%v\n", code, header)
+                       return nil
+               },
+       }))
+       res, err := cst.c.Do(req)
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer res.Body.Close()
+
+       res.Write(&got)
+       want := "1xx: code=123, header=map[Foo:[bar]]\nHTTP/1.1 200 OK\r\nContent-Length: 5\r\nBar: baz\r\n\r\nHello"
+       if got.String() != want {
+               t.Errorf(" got: %q\nwant: %q\n", got.Bytes(), want)
+       }
+}
+
+func TestTransportLimits1xxResponses(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+       cst := newClientServerTest(t, h1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+               conn, buf, _ := w.(Hijacker).Hijack()
+               for i := 0; i < 10; i++ {
+                       buf.Write([]byte("HTTP/1.1 123 OneTwoThree\r\n\r\n"))
+               }
+               buf.Write([]byte("HTTP/1.1 204 No Content\r\n\r\n"))
+               buf.Flush()
+               conn.Close()
+       }))
+       defer cst.close()
+       cst.tr.DisableKeepAlives = true // prevent log spam; our test server is hanging up anyway
+
+       res, err := cst.c.Get(cst.ts.URL)
+       if res != nil {
+               defer res.Body.Close()
+       }
+       got := fmt.Sprint(err)
+       wantSub := "too many 1xx informational responses"
+       if !strings.Contains(got, wantSub) {
+               t.Errorf("Get error = %v; want substring %q", err, wantSub)
        }
 }