net/http: flesh out Transport's HTTP/1 CONNECT+bidi support to match HTTP/2

Fixes #17227

Change-Id: I0f8964593d69623b85d5759f6276063ee62b2915
Reviewed-on: https://go-review.googlesource.com/c/123156Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

src/net/http/request.go

@@ -545,6 +545,9 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF
 	} else if r.Method == "CONNECT" && r.URL.Path == "" {
 		// CONNECT requests normally give just the host and port, not a full URL.
 		ruri = host
+		if r.URL.Opaque != "" {
+			ruri = r.URL.Opaque
+		}
 	}
 	// TODO(bradfitz): escape at least newlines in ruri?
 

src/net/http/requestwrite_test.go

@@ -512,6 +512,38 @@ var reqWriteTests = []reqWriteTest{
 			"User-Agent: Go-http-client/1.1\r\n" +
 			"\r\n",
 	},
+
+	// CONNECT without Opaque
+	21: {
+		Req: Request{
+			Method: "CONNECT",
+			URL: &url.URL{
+				Scheme: "https", // of proxy.com
+				Host:   "proxy.com",
+			},
+		},
+		// What we used to do, locking that behavior in:
+		WantWrite: "CONNECT proxy.com HTTP/1.1\r\n" +
+			"Host: proxy.com\r\n" +
+			"User-Agent: Go-http-client/1.1\r\n" +
+			"\r\n",
+	},
+
+	// CONNECT with Opaque
+	22: {
+		Req: Request{
+			Method: "CONNECT",
+			URL: &url.URL{
+				Scheme: "https", // of proxy.com
+				Host:   "proxy.com",
+				Opaque: "backend:443",
+			},
+		},
+		WantWrite: "CONNECT backend:443 HTTP/1.1\r\n" +
+			"Host: proxy.com\r\n" +
+			"User-Agent: Go-http-client/1.1\r\n" +
+			"\r\n",
+	},
 }
 
 func TestRequestWrite(t *testing.T) {

src/net/http/transfer.go

@@ -184,6 +184,9 @@ func (t *transferWriter) shouldSendChunkedRequestBody() bool {
 	if t.ContentLength >= 0 || t.Body == nil { // redundant checks; caller did them
 		return false
 	}
+	if t.Method == "CONNECT" {
+		return false
+	}
 	if requestMethodUsuallyLacksBody(t.Method) {
 		// Only probe the Request.Body for GET/HEAD/DELETE/etc
 		// requests, because it's only those types of requests
@@ -357,7 +360,11 @@ func (t *transferWriter) writeBody(w io.Writer) error {
 				err = cw.Close()
 			}
 		} else if t.ContentLength == -1 {
-			ncopy, err = io.Copy(w, body)
+			dst := w
+			if t.Method == "CONNECT" {
+				dst = bufioFlushWriter{dst}
+			}
+			ncopy, err = io.Copy(dst, body)
 		} else {
 			ncopy, err = io.Copy(w, io.LimitReader(body, t.ContentLength))
 			if err != nil {
@@ -1050,3 +1057,18 @@ func isKnownInMemoryReader(r io.Reader) bool {
 	}
 	return false
 }
+
+// bufioFlushWriter is an io.Writer wrapper that flushes all writes
+// on its wrapped writer if it's a *bufio.Writer.
+type bufioFlushWriter struct{ w io.Writer }
+
+func (fw bufioFlushWriter) Write(p []byte) (n int, err error) {
+	n, err = fw.w.Write(p)
+	if bw, ok := fw.w.(*bufio.Writer); n > 0 && ok {
+		ferr := bw.Flush()
+		if ferr != nil && err == nil {
+			err = ferr
+		}
+	}
+	return
+}

src/net/http/transport.go

@@ -85,15 +85,6 @@ func init() {
 // To explicitly enable HTTP/2 on a transport, use golang.org/x/net/http2
 // and call ConfigureTransport. See the package docs for more about HTTP/2.
 //
-// The Transport will send CONNECT requests to a proxy for its own use
-// when processing HTTPS requests, but Transport should generally not
-// be used to send a CONNECT request. That is, the Request passed to
-// the RoundTrip method should not have a Method of "CONNECT", as Go's
-// HTTP/1.x implementation does not support full-duplex request bodies
-// being written while the response body is streamed. Go's HTTP/2
-// implementation does support full duplex, but many CONNECT proxies speak
-// HTTP/1.x.
-//
 // Responses with status codes in the 1xx range are either handled
 // automatically (100 expect-continue) or ignored. The one
 // exception is HTTP status code 101 (Switching Protocols), which is

src/net/http/transport_test.go

@@ -4887,3 +4887,68 @@ func TestTransportResponseBodyWritableOnProtocolSwitch(t *testing.T) {
 		t.Errorf("read %q; want %q", got, want)
 	}
 }
+
+func TestTransportCONNECTBidi(t *testing.T) {
+	defer afterTest(t)
+	const target = "backend:443"
+	cst := newClientServerTest(t, h1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		if r.Method != "CONNECT" {
+			t.Errorf("unexpected method %q", r.Method)
+			w.WriteHeader(500)
+			return
+		}
+		if r.RequestURI != target {
+			t.Errorf("unexpected CONNECT target %q", r.RequestURI)
+			w.WriteHeader(500)
+			return
+		}
+		nc, brw, err := w.(Hijacker).Hijack()
+		if err != nil {
+			t.Error(err)
+			return
+		}
+		defer nc.Close()
+		nc.Write([]byte("HTTP/1.1 200 OK\r\n\r\n"))
+		// Switch to a little protocol that capitalize its input lines:
+		for {
+			line, err := brw.ReadString('\n')
+			if err != nil {
+				if err != io.EOF {
+					t.Error(err)
+				}
+				return
+			}
+			io.WriteString(brw, strings.ToUpper(line))
+			brw.Flush()
+		}
+	}))
+	defer cst.close()
+	pr, pw := io.Pipe()
+	defer pw.Close()
+	req, err := NewRequest("CONNECT", cst.ts.URL, pr)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req.URL.Opaque = target
+	res, err := cst.c.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	if res.StatusCode != 200 {
+		t.Fatalf("status code = %d; want 200", res.StatusCode)
+	}
+	br := bufio.NewReader(res.Body)
+	for _, str := range []string{"foo", "bar", "baz"} {
+		fmt.Fprintf(pw, "%s\n", str)
+		got, err := br.ReadString('\n')
+		if err != nil {
+			t.Fatal(err)
+		}
+		got = strings.TrimSpace(got)
+		want := strings.ToUpper(str)
+		if got != want {
+			t.Fatalf("got %q; want %q", got, want)
+		}
+	}
+}