http2: disable extended CONNECT by default

Browsers interpret a server advertising extended CONNECT support as indicating the server supports WebSockets-over-HTTP/2. However, WebSocket-over-HTTP/2 requires support from both the HTTP implementation and the WebSocket implementation, and existing Go WebSocket packages don't support HTTP/2. Disable extended CONNECT support by default, since advertising it is a non-backwards-compatible change. For golang/go#71128 For golang/go#49918 Change-Id: Ie7d3ee2cd48124836a00bad320752e78719ffc46 Reviewed-on: https://go-review.googlesource.com/c/net/+/641475 Auto-Submit: Damien Neil <[email protected]> Reviewed-by: Jonathan Amsterdam <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
golang · Jan 8, 2025 · 0a5dcdd · 0a5dcdd
1 parent 03179ce
commit 0a5dcdd
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 9 deletions.
diff --git a/http2/http2.go b/http2/http2.go
@@ -34,11 +34,19 @@ import (
 )
 
 var (
-	VerboseLogs                    bool
-	logFrameWrites                 bool
-	logFrameReads                  bool
-	inTests                        bool
-	disableExtendedConnectProtocol bool
+	VerboseLogs    bool
+	logFrameWrites bool
+	logFrameReads  bool
+	inTests        bool
+
+	// Enabling extended CONNECT by causes browsers to attempt to use
+	// WebSockets-over-HTTP/2. This results in problems when the server's websocket
+	// package doesn't support extended CONNECT.
+	//
+	// Disable extended CONNECT by default for now.
+	//
+	// Issue #71128.
+	disableExtendedConnectProtocol = true
 )
 
 func init() {
@@ -51,8 +59,8 @@ func init() {
 		logFrameWrites = true
 		logFrameReads = true
 	}
-	if strings.Contains(e, "http2xconnect=0") {
-		disableExtendedConnectProtocol = true
+	if strings.Contains(e, "http2xconnect=1") {
+		disableExtendedConnectProtocol = false
 	}
 }
 

diff --git a/http2/http2_test.go b/http2/http2_test.go
@@ -284,6 +284,15 @@ func TestNoUnicodeStrings(t *testing.T) {
 	}
 }
 
+// setForTest sets *p = v, and restores its original value in t.Cleanup.
+func setForTest[T any](t *testing.T, p *T, v T) {
+	orig := *p
+	t.Cleanup(func() {
+		*p = orig
+	})
+	*p = v
+}
+
 // must returns v if err is nil, or panics otherwise.
 func must[T any](v T, err error) T {
 	if err != nil {

diff --git a/http2/transport_test.go b/http2/transport_test.go
@@ -5856,7 +5856,7 @@ func TestTransportTLSNextProtoConnImmediateFailureUnused(t *testing.T) {
 }
 
 func TestExtendedConnectClientWithServerSupport(t *testing.T) {
-	disableExtendedConnectProtocol = false
+	setForTest(t, &disableExtendedConnectProtocol, false)
 	ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {
 		if r.Header.Get(":protocol") != "extended-connect" {
 			t.Fatalf("unexpected :protocol header received")
@@ -5892,7 +5892,7 @@ func TestExtendedConnectClientWithServerSupport(t *testing.T) {
 }
 
 func TestExtendedConnectClientWithoutServerSupport(t *testing.T) {
-	disableExtendedConnectProtocol = true
+	setForTest(t, &disableExtendedConnectProtocol, true)
 	ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {
 		io.Copy(w, r.Body)
 	})