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

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 https://github.com/TykTechnologies/tyk/blob/2a2a98461d86bd352aa8bb3b8904a62cca42c167/gateway/testutil.go#L461-L464.

+       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

https://github.com/TykTechnologies/tyk/blob/2a2a98461d86bd352aa8bb3b8904a62cca42c167/internal/httputil/streaming.go#L25-L37

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 :pray: .

TykTechnologies / tyk

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