]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: send body or close connection on expect-100-continue requests
authorDamien Neil <dneil@google.com>
Thu, 6 Jun 2024 19:50:46 +0000 (12:50 -0700)
committerDamien Neil <dneil@google.com>
Thu, 6 Jun 2024 21:59:21 +0000 (21:59 +0000)
When sending a request with an "Expect: 100-continue" header,
we must send the request body before sending any further requests
on the connection.

When receiving a non-1xx response to an "Expect: 100-continue" request,
send the request body if the connection isn't being closed after
processing the response. In other words, if either the request
or response contains a "Connection: close" header, then skip sending
the request body (because the connection will not be used for
further requests), but otherwise send it.

Correct a comment on the server-side Expect: 100-continue handling
that implied sending the request body is optional. It isn't.

For #67555

Change-Id: Ia2f12091bee697771087f32ac347509ec5922d54
Reviewed-on: https://go-review.googlesource.com/c/go/+/591255
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
src/net/http/server.go
src/net/http/transport.go
src/net/http/transport_test.go

index 9deb308e8a5954fc1d260af5e99392b71b1476c1..b9a6edd7ad2508bf1d15b12d77c996c7042564d4 100644 (file)
@@ -1399,16 +1399,21 @@ func (cw *chunkWriter) writeHeader(p []byte) {
 
        // If the client wanted a 100-continue but we never sent it to
        // them (or, more strictly: we never finished reading their
-       // request body), don't reuse this connection because it's now
-       // in an unknown state: we might be sending this response at
-       // the same time the client is now sending its request body
-       // after a timeout.  (Some HTTP clients send Expect:
-       // 100-continue but knowing that some servers don't support
-       // it, the clients set a timer and send the body later anyway)
-       // If we haven't seen EOF, we can't skip over the unread body
-       // because we don't know if the next bytes on the wire will be
-       // the body-following-the-timer or the subsequent request.
-       // See Issue 11549.
+       // request body), don't reuse this connection.
+       //
+       // This behavior was first added on the theory that we don't know
+       // if the next bytes on the wire are going to be the remainder of
+       // the request body or the subsequent request (see issue 11549),
+       // but that's not correct: If we keep using the connection,
+       // the client is required to send the request body whether we
+       // asked for it or not.
+       //
+       // We probably do want to skip reusing the connection in most cases,
+       // however. If the client is offering a large request body that we
+       // don't intend to use, then it's better to close the connection
+       // than to read the body. For now, assume that if we're sending
+       // headers, the handler is done reading the body and we should
+       // drop the connection if we haven't seen EOF.
        if ecr, ok := w.req.Body.(*expectContinueReader); ok && !ecr.sawEOF.Load() {
                w.closeAfterReply = true
        }
index a1ff7ebe32bfd187967e3e63a0b55c439b9b9fe5..da9163a27ae6fff03b6b82cf51253f8baf2cb871 100644 (file)
@@ -2397,17 +2397,12 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
                        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 continueCh != nil && resCode == StatusContinue {
+                       if trace != nil && trace.Got100Continue != nil {
+                               trace.Got100Continue()
                        }
+                       continueCh <- struct{}{}
+                       continueCh = nil
                }
                is1xx := 100 <= resCode && resCode <= 199
                // treat 101 as a terminal status, see issue 26161
@@ -2430,6 +2425,25 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
        if resp.isProtocolSwitch() {
                resp.Body = newReadWriteCloserBody(pc.br, pc.conn)
        }
+       if continueCh != nil {
+               // We send an "Expect: 100-continue" header, but the server
+               // responded with a terminal status and no 100 Continue.
+               //
+               // If we're going to keep using the connection, we need to send the request body.
+               // Tell writeLoop to skip sending the body if we're going to close the connection,
+               // or to send it otherwise.
+               //
+               // The case where we receive a 101 Switching Protocols response is a bit
+               // ambiguous, since we don't know what protocol we're switching to.
+               // Conceivably, it's one that doesn't need us to send the body.
+               // Given that we'll send the body if ExpectContinueTimeout expires,
+               // be consistent and always send it if we aren't closing the connection.
+               if resp.Close || rc.treq.Request.Close {
+                       close(continueCh) // don't send the body; the connection will close
+               } else {
+                       continueCh <- struct{}{} // send the body
+               }
+       }
 
        resp.TLS = pc.tlsState
        return
index aa877b57c796dde87e9fb5411dc36d3151175bc3..ae7159dab00d399c4d440321e00b6af972c7ea1b 100644 (file)
@@ -1185,94 +1185,142 @@ func testTransportGzip(t *testing.T, mode testMode) {
        }
 }
 
-// If a request has Expect:100-continue header, the request blocks sending body until the first response.
-// Premature consumption of the request body should not be occurred.
-func TestTransportExpect100Continue(t *testing.T) {
-       run(t, testTransportExpect100Continue, []testMode{http1Mode})
+// A transport100Continue test exercises Transport behaviors when sending a
+// request with an Expect: 100-continue header.
+type transport100ContinueTest struct {
+       t *testing.T
+
+       reqdone chan struct{}
+       resp    *Response
+       respErr error
+
+       conn   net.Conn
+       reader *bufio.Reader
 }
-func testTransportExpect100Continue(t *testing.T, mode testMode) {
-       ts := newClientServerTest(t, mode, HandlerFunc(func(rw ResponseWriter, req *Request) {
-               switch req.URL.Path {
-               case "/100":
-                       // This endpoint implicitly responds 100 Continue and reads body.
-                       if _, err := io.Copy(io.Discard, req.Body); err != nil {
-                               t.Error("Failed to read Body", err)
-                       }
-                       rw.WriteHeader(StatusOK)
-               case "/200":
-                       // Go 1.5 adds Connection: close header if the client expect
-                       // continue but not entire request body is consumed.
-                       rw.WriteHeader(StatusOK)
-               case "/500":
-                       rw.WriteHeader(StatusInternalServerError)
-               case "/keepalive":
-                       // This hijacked endpoint responds error without Connection:close.
-                       _, bufrw, err := rw.(Hijacker).Hijack()
-                       if err != nil {
-                               log.Fatal(err)
-                       }
-                       bufrw.WriteString("HTTP/1.1 500 Internal Server Error\r\n")
-                       bufrw.WriteString("Content-Length: 0\r\n\r\n")
-                       bufrw.Flush()
-               case "/timeout":
-                       // This endpoint tries to read body without 100 (Continue) response.
-                       // After ExpectContinueTimeout, the reading will be started.
-                       conn, bufrw, err := rw.(Hijacker).Hijack()
-                       if err != nil {
-                               log.Fatal(err)
-                       }
-                       if _, err := io.CopyN(io.Discard, bufrw, req.ContentLength); err != nil {
-                               t.Error("Failed to read Body", err)
-                       }
-                       bufrw.WriteString("HTTP/1.1 200 OK\r\n\r\n")
-                       bufrw.Flush()
-                       conn.Close()
-               }
 
-       })).ts
+const transport100ContinueTestBody = "request body"
 
-       tests := []struct {
-               path   string
-               body   []byte
-               sent   int
-               status int
-       }{
-               {path: "/100", body: []byte("hello"), sent: 5, status: 200},       // Got 100 followed by 200, entire body is sent.
-               {path: "/200", body: []byte("hello"), sent: 0, status: 200},       // Got 200 without 100. body isn't sent.
-               {path: "/500", body: []byte("hello"), sent: 0, status: 500},       // Got 500 without 100. body isn't sent.
-               {path: "/keepalive", body: []byte("hello"), sent: 0, status: 500}, // Although without Connection:close, body isn't sent.
-               {path: "/timeout", body: []byte("hello"), sent: 5, status: 200},   // Timeout exceeded and entire body is sent.
+// newTransport100ContinueTest creates a Transport and sends an Expect: 100-continue
+// request on it.
+func newTransport100ContinueTest(t *testing.T, timeout time.Duration) *transport100ContinueTest {
+       ln := newLocalListener(t)
+       defer ln.Close()
+
+       test := &transport100ContinueTest{
+               t:       t,
+               reqdone: make(chan struct{}),
        }
 
-       c := ts.Client()
-       for i, v := range tests {
-               tr := &Transport{
-                       ExpectContinueTimeout: 2 * time.Second,
-               }
-               defer tr.CloseIdleConnections()
-               c.Transport = tr
-               body := bytes.NewReader(v.body)
-               req, err := NewRequest("PUT", ts.URL+v.path, body)
-               if err != nil {
-                       t.Fatal(err)
-               }
+       tr := &Transport{
+               ExpectContinueTimeout: timeout,
+       }
+       go func() {
+               defer close(test.reqdone)
+               body := strings.NewReader(transport100ContinueTestBody)
+               req, _ := NewRequest("PUT", "http://"+ln.Addr().String(), body)
                req.Header.Set("Expect", "100-continue")
-               req.ContentLength = int64(len(v.body))
+               req.ContentLength = int64(len(transport100ContinueTestBody))
+               test.resp, test.respErr = tr.RoundTrip(req)
+               test.resp.Body.Close()
+       }()
 
-               resp, err := c.Do(req)
-               if err != nil {
-                       t.Fatal(err)
+       c, err := ln.Accept()
+       if err != nil {
+               t.Fatalf("Accept: %v", err)
+       }
+       t.Cleanup(func() {
+               c.Close()
+       })
+       br := bufio.NewReader(c)
+       _, err = ReadRequest(br)
+       if err != nil {
+               t.Fatalf("ReadRequest: %v", err)
+       }
+       test.conn = c
+       test.reader = br
+       t.Cleanup(func() {
+               <-test.reqdone
+               tr.CloseIdleConnections()
+               got, _ := io.ReadAll(test.reader)
+               if len(got) > 0 {
+                       t.Fatalf("Transport sent unexpected bytes: %q", got)
                }
-               resp.Body.Close()
+       })
 
-               sent := len(v.body) - body.Len()
-               if v.status != resp.StatusCode {
-                       t.Errorf("test %d: status code should be %d but got %d. (%s)", i, v.status, resp.StatusCode, v.path)
-               }
-               if v.sent != sent {
-                       t.Errorf("test %d: sent body should be %d but sent %d. (%s)", i, v.sent, sent, v.path)
+       return test
+}
+
+// respond sends response lines from the server to the transport.
+func (test *transport100ContinueTest) respond(lines ...string) {
+       for _, line := range lines {
+               if _, err := test.conn.Write([]byte(line + "\r\n")); err != nil {
+                       test.t.Fatalf("Write: %v", err)
                }
        }
+       if _, err := test.conn.Write([]byte("\r\n")); err != nil {
+               test.t.Fatalf("Write: %v", err)
+       }
+}
+
+// wantBodySent ensures the transport has sent the request body to the server.
+func (test *transport100ContinueTest) wantBodySent() {
+       got, err := io.ReadAll(io.LimitReader(test.reader, int64(len(transport100ContinueTestBody))))
+       if err != nil {
+               test.t.Fatalf("unexpected error reading body: %v", err)
+       }
+       if got, want := string(got), transport100ContinueTestBody; got != want {
+               test.t.Fatalf("unexpected body: got %q, want %q", got, want)
+       }
+}
+
+// wantRequestDone ensures the Transport.RoundTrip has completed with the expected status.
+func (test *transport100ContinueTest) wantRequestDone(want int) {
+       <-test.reqdone
+       if test.respErr != nil {
+               test.t.Fatalf("unexpected RoundTrip error: %v", test.respErr)
+       }
+       if got := test.resp.StatusCode; got != want {
+               test.t.Fatalf("unexpected response code: got %v, want %v", got, want)
+       }
+}
+
+func TestTransportExpect100ContinueSent(t *testing.T) {
+       test := newTransport100ContinueTest(t, 1*time.Hour)
+       // Server sends a 100 Continue response, and the client sends the request body.
+       test.respond("HTTP/1.1 100 Continue")
+       test.wantBodySent()
+       test.respond("HTTP/1.1 200", "Content-Length: 0")
+       test.wantRequestDone(200)
+}
+
+func TestTransportExpect100Continue200ResponseNoConnClose(t *testing.T) {
+       test := newTransport100ContinueTest(t, 1*time.Hour)
+       // No 100 Continue response, no Connection: close header.
+       test.respond("HTTP/1.1 200", "Content-Length: 0")
+       test.wantBodySent()
+       test.wantRequestDone(200)
+}
+
+func TestTransportExpect100Continue200ResponseWithConnClose(t *testing.T) {
+       test := newTransport100ContinueTest(t, 1*time.Hour)
+       // No 100 Continue response, Connection: close header set.
+       test.respond("HTTP/1.1 200", "Connection: close", "Content-Length: 0")
+       test.wantRequestDone(200)
+}
+
+func TestTransportExpect100Continue500ResponseNoConnClose(t *testing.T) {
+       test := newTransport100ContinueTest(t, 1*time.Hour)
+       // No 100 Continue response, no Connection: close header.
+       test.respond("HTTP/1.1 500", "Content-Length: 0")
+       test.wantBodySent()
+       test.wantRequestDone(500)
+}
+
+func TestTransportExpect100Continue500ResponseTimeout(t *testing.T) {
+       test := newTransport100ContinueTest(t, 5*time.Millisecond) // short timeout
+       test.wantBodySent()                                        // after timeout
+       test.respond("HTTP/1.1 200", "Content-Length: 0")
+       test.wantRequestDone(200)
 }
 
 func TestSOCKS5Proxy(t *testing.T) {