[TT-13257] Websocket connection is not upgraded when keep-alive is added to Connection

[Darkness4]

Branch/Environment/Version

• Branch/Version: master
• Environment: On-Prem

Describe the bug
Connection header is deleted and not upgraded even if Upgrade is present, but with other Connection like keep-alive:

GET /ws HTTP/1.1
Host: <...>
Accept-Encoding: gzip, deflate, br, zstd
Sec-WebSocket-Version: 13
Sec-WebSocket-Extensions: permessage-deflate
Sec-WebSocket-Key: X62lCXELOHFcBBG72P2S2Q==
Connection: Upgrade, keep-alive
Upgrade: websocket

This notably affects Firefox users when trying to dial the tyk gateway.

Reproduction steps
Steps to reproduce the behavior:

Test case

Added a test at gateway/gateway_test.go.

func TestWebsockets(t *testing.T) {
	ts := StartTest(nil)
	defer ts.Close()

	globalConf := ts.Gw.GetConfig()
	globalConf.HttpServerOptions.EnableWebSockets = true
	ts.Gw.SetConfig(globalConf)

	ts.Gw.BuildAndLoadAPI(func(spec *APISpec) {
		spec.Proxy.ListenPath = "/"
	})

	baseURL := strings.Replace(ts.URL, "http://", "ws://", -1)
	url, err := url.Parse(baseURL)
	if err != nil {
		t.Fatalf("cannot parse url: %v", err)
	}

	conn, err := net.Dial("tcp", url.Host)
	if err != nil {
		t.Fatalf("cannot make connection: %v", err)
	}
	defer conn.Close()

	req := fmt.Sprintf(`GET %s/ws HTTP/1.1
Host: %s
Accept-Encoding: gzip, deflate, br, zstd
Sec-WebSocket-Version: 13
Sec-WebSocket-Extensions: permessage-deflate
Sec-WebSocket-Key: X62lCXELOHFcBBG72P2S2Q==
Connection: Upgrade, keep-alive
Upgrade: websocket

`, baseURL, url.Host)
	req = strings.Replace(req, "\n", "\r\n", -1)
	_, err = conn.Write([]byte(req))
	if err != nil {
		t.Fatalf("cannot write request: %v", err)
	}
	buf, err := bufio.NewReader(conn).ReadString('\n')
	if err != nil {
		t.Fatalf("cannot read response: %v", err)
	}
	if !strings.Contains(buf, "HTTP/1.1 101 Switching Protocols") {
		t.Error("Unexpected response:", buf)
	}

	_, _ = ts.Run(t, test.TestCase{
		Method: "GET",
		Path:   "/abc",
		Code:   http.StatusOK,
	})
}

Via Firefox

Firefox send a Connection: Upgrade, keep-alive when trying to connect to a websocket (GraphQL).

Actual behavior

Test panic. By applying a debug at

tyk/gateway/testutil.go

Lines 461 to 464 in 2a2a984

	conn, err := upgrader.Upgrade(w, req, nil)
	if err != nil {
	http.Error(w, fmt.Sprintf("cannot upgrade: %v", err), http.StatusInternalServerError)
	}

.

+		b, _ := httputil.DumpRequest(req, false)
+		fmt.Println(string(b))
		conn, err := upgrader.Upgrade(w, req, nil)
		if err != nil {
			http.Error(w, fmt.Sprintf("cannot upgrade: %v", err), http.StatusInternalServerError)
+			fmt.Println("cannot upgrade:", err)
		}

It prints:

time="Aug 06 12:44:59" level=info msg="starting test"
time="Aug 06 12:44:59" level=info msg="Rich plugins are disabled" prefix=coprocess
GET /ws HTTP/1.1
Host: 127.0.0.1:16500
Accept-Encoding: gzip, deflate, br, zstd
Sec-Websocket-Extensions: permessage-deflate
Sec-Websocket-Key: X62lCXELOHFcBBG72P2S2Q==
Sec-Websocket-Version: 13
User-Agent: Tyk/v5.5.0-dev
X-Forwarded-For: 127.0.0.1


cannot upgrade: websocket: the client is not using the websocket protocol: 'upgrade' token not found in 'Connection' header
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x46911ff]

goroutine 182 [running]:
github.com/gorilla/websocket.(*Conn).NextReader(0x0)
	github.com/gorilla/websocket@v1.5.3/conn.go:1000 +0x3f
github.com/gorilla/websocket.(*Conn).ReadMessage(0x0)
	github.com/gorilla/websocket@v1.5.3/conn.go:1093 +0x65
github.com/TykTechnologies/tyk/gateway.(*Test).testHttpHandler.func1.1()
	tyk/gateway/testutil.go:472 +0x55
created by github.com/TykTechnologies/tyk/gateway.(*Test).testHttpHandler.func1 in goroutine 180
	tyk/gateway/testutil.go:470 +0x4f6
Process 15407 has exited with status 2

Connection header has been filtered and the connection is not upgraded (conn is nil), causing a panic in the test case.

Expected behavior

Connection should be upgraded and the header should be passed.

Cause

tyk/internal/httputil/streaming.go

Lines 25 to 37 in 2a2a984

	func IsUpgrade(req *http.Request) (string, bool) {
	connection := strings.ToLower(strings.TrimSpace(req.Header.Get(headerConnection)))
	if connection != "upgrade" {
	return "", false
	}
	upgrade := strings.ToLower(strings.TrimSpace(req.Header.Get(headerUpgrade)))
	if upgrade != "" {
	return upgrade, true
	}
	return "", false
	}

Detections of "upgrade" in "Connection" header is too strict (!=) and should be more flexible (not contains).

Possible solutions

Using nhooyr.io's implementation style:

func IsUpgrade(req *http.Request) (string, bool) {
	if !headerContainsTokenIgnoreCase(req.Header, headerConnection, "Upgrade") {
		return "", false
	}

	upgrade := strings.ToLower(strings.TrimSpace(req.Header.Get(headerUpgrade)))
	if upgrade != "" {
		return upgrade, true
	}

	return "", false
}

func headerContainsTokenIgnoreCase(h http.Header, key, token string) bool {
	for _, t := range headerTokens(h, key) {
		if strings.EqualFold(t, token) {
			return true
		}
	}
	return false
}

func headerTokens(h http.Header, key string) []string {
	key = textproto.CanonicalMIMEHeaderKey(key)
	var tokens []string
	for _, v := range h[key] {
		v = strings.TrimSpace(v)
		for _, t := range strings.Split(v, ",") {
			t = strings.TrimSpace(t)
			tokens = append(tokens, t)
		}
	}
	return tokens
}

Nhooyr implementation seems pretty standard while gorilla/websocket seems "home-made".

If you could please fix this as this literally block all firefox users in using WS (including GraphQL subscriptions). Thank you 🙏 .

[WilliamGorge]

+1, having the same issue here