src/net/http/serve_test.go | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/net/http/server.go | 26 +++++++++++++++++++++++--- diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 5673dc1a9c079d5ebfe3342128db3be2166561f3..cc7c095de02ab07901b6f44cc9a4aa1bd69eb0d4 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -7685,3 +7685,98 @@ if !slices.Equal(nextProtos, wantNextProtos) { t.Fatalf("after running test: original NextProtos slice = %v, want %v", nextProtos, wantNextProtos) } } + +func TestServerCONNECTSuccess(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + const code = 200 + body := []byte("body") + srv := &Server{ + Handler: HandlerFunc(func(w ResponseWriter, req *Request) { + w.WriteHeader(code) + w.Write(body) + }), + } + l := fakeNetListen() + defer l.Close() + go srv.Serve(l) + + conn := l.connect() + defer conn.Close() + io.WriteString(conn, "CONNECT backend.example.tld:80 HTTP/1.1\r\nHost: example.tld\r\n\r\n") + + bufr := bufio.NewReader(conn) + synctest.Wait() + resp, err := ReadResponse(bufr, nil) + if err != nil { + t.Fatal(err) + } + if resp.StatusCode != code { + t.Errorf("response status = %v, want %v", resp.StatusCode, code) + } + if resp.ContentLength != -1 { + t.Errorf("Content-Length: %v; want absent", resp.ContentLength) + } + if len(resp.TransferEncoding) > 0 { + t.Errorf("Transfer-Encoding: %q; want absent", resp.TransferEncoding) + } + for _, h := range []string{"Content-Length", "Transfer-Encoding"} { + if got, ok := resp.Header[h]; ok { + t.Errorf("response header %q = %q; want absent", h, got) + } + } + got := make([]byte, len(body)) + if _, err := io.ReadFull(bufr, got); err != nil || !bytes.Equal(got, body) { + t.Fatalf("want bytes %q, got %q (err %v)", body, got, err) + } + if !conn.IsClosedByPeer() { + t.Errorf("connection not closed by peer") + } + if got, err := bufr.Peek(32); len(got) != 0 || err != io.EOF { + t.Errorf("read from conn: %q, %v; expect conn to be closed", got, err) + } + }) +} + +func TestServerCONNECTFailure(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + const code = 409 + body := []byte("body") + srv := &Server{ + Handler: HandlerFunc(func(w ResponseWriter, req *Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + w.WriteHeader(code) + w.Write(body) + }), + } + l := fakeNetListen() + defer l.Close() + go srv.Serve(l) + + conn := l.connect() + defer conn.Close() + io.WriteString(conn, "CONNECT backend.example.tld:80 HTTP/1.1\r\nHost: example.tld\r\n\r\n") + + bufr := bufio.NewReader(conn) + synctest.Wait() + resp, err := ReadResponse(bufr, nil) + if err != nil { + t.Fatal(err) + } + if resp.StatusCode != code { + t.Errorf("response status = %v, want %v", resp.StatusCode, code) + } + if !resp.Close { + t.Errorf("Connection: close not set; want it to be") + } + got := make([]byte, len(body)) + if _, err := io.ReadFull(bufr, got); err != nil || !bytes.Equal(got, body) { + t.Fatalf("want bytes %q, got %q (err %v)", body, got, err) + } + if !conn.IsClosedByPeer() { + t.Errorf("connection not closed by peer") + } + if got, err := bufr.Peek(32); len(got) != 0 || err != io.EOF { + t.Errorf("read from conn: %q, %v; expect conn to be closed", got, err) + } + }) +} diff --git a/src/net/http/server.go b/src/net/http/server.go index 0ea4ca3e47ff417e337860d5dcbecc8abaeaa9f1..f3e287c6039ec2407e8b1b987928723326f2873f 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -1253,7 +1253,11 @@ cw.wroteHeader = true w := cw.res keepAlivesEnabled := w.conn.server.doKeepAlives() - isHEAD := w.req.Method == "HEAD" + + // Consult w.conn.lastMethod instead of w.req.Method, + // just in case a middleware layer modified w.req. + isHEAD := w.conn.lastMethod == "HEAD" + isCONNECT := w.conn.lastMethod == "CONNECT" // header is written out to w.conn.buf below. Depending on the // state of the handler, we either own the map or not. If we @@ -1365,6 +1369,11 @@ if ecr, ok := w.req.Body.(*expectContinueReader); ok && !ecr.sawEOF.Load() { w.closeAfterReply = true } + if isCONNECT { + // Don't reuse a connection after a CONNECT, even if we reject it. + w.closeAfterReply = true + } + // We do this by default because there are a number of clients that // send a full request before starting to read the response, and they // can deadlock if we start writing the response with unconsumed body @@ -1464,7 +1473,13 @@ delHeader("Content-Length") hasCL = false } - if w.req.Method == "HEAD" || !bodyAllowedForStatus(code) || code == StatusNoContent { + isSuccessfulCONNECT := isCONNECT && code >= 200 && code < 300 + if isSuccessfulCONNECT { + // Tunnel established, connection is no longer HTTP. + delHeader("Transfer-Encoding") + delHeader("Content-Length") + setHeader.contentLength = nil + } else if isHEAD || !bodyAllowedForStatus(code) || code == StatusNoContent { // Response has no body. delHeader("Transfer-Encoding") } else if hasCL { @@ -1514,8 +1529,13 @@ (!keepAlivesEnabled || !hasToken(cw.header.get("Connection"), "close")) && !isProtocolSwitchResponse(w.status, header) if delConnectionHeader { delHeader("Connection") - if w.req.ProtoAtLeast(1, 1) { + // Don't set Connection: close on a 2xx CONNECT response, + // even though we will close the connection if the handler doesn't hijack it. + // If the handler does hijack the connection, the Connection: close is confusing. + if w.req.ProtoAtLeast(1, 1) && !isSuccessfulCONNECT { setHeader.connection = "close" + } else { + setHeader.connection = "" } }