fix: do not canonicalize Sec-WebSocket-* headers in apps (#5334)

* fix: do not canonicalize Sec-WebSocket-* headers in apps

* chore: test for non-canonical header name subst

coderd/workspaceapps.go

@@ -39,6 +39,23 @@ const (
 	redirectURIQueryParam     = "redirect_uri"
 )
 
+// nonCanonicalHeaders is a map from "canonical" headers to the actual header we
+// should send to the app in the workspace. Some headers (such as the websocket
+// upgrade headers from RFC 6455) are not canonical according to the HTTP/1
+// spec. Golang has said that they will not add custom cases for these headers,
+// so we need to do it ourselves.
+//
+// Some apps our customers use are sensitive to the case of these headers.
+//
+// https://github.com/golang/go/issues/18495
+var nonCanonicalHeaders = map[string]string{
+	"Sec-Websocket-Accept":     "Sec-WebSocket-Accept",
+	"Sec-Websocket-Extensions": "Sec-WebSocket-Extensions",
+	"Sec-Websocket-Key":        "Sec-WebSocket-Key",
+	"Sec-Websocket-Protocol":   "Sec-WebSocket-Protocol",
+	"Sec-Websocket-Version":    "Sec-WebSocket-Version",
+}
+
 func (api *API) appHost(rw http.ResponseWriter, r *http.Request) {
 	host := api.AppHostname
 	if api.AccessURL.Port() != "" {
@@ -708,6 +725,7 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
 		return
 	}
 	defer release()
+	proxy.Transport = conn.HTTPTransport()
 
 	// This strips the session token from a workspace app request.
 	cookieHeaders := r.Header.Values("Cookie")[:]
@@ -715,7 +733,16 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
 	for _, cookieHeader := range cookieHeaders {
 		r.Header.Add("Cookie", httpapi.StripCoderCookies(cookieHeader))
 	}
-	proxy.Transport = conn.HTTPTransport()
+
+	// Convert canonicalized headers to their non-canonicalized counterparts.
+	// See the comment on `nonCanonicalHeaders` for more information on why this
+	// is necessary.
+	for k, v := range r.Header {
+		if n, ok := nonCanonicalHeaders[k]; ok {
+			r.Header.Del(k)
+			r.Header[n] = v
+		}
+	}
 
 	// end span so we don't get long lived trace data
 	tracing.EndHTTPSpan(r, http.StatusOK, trace.SpanFromContext(ctx))

coderd/workspaceapps_test.go

@@ -1,6 +1,7 @@
 package coderd_test
 
 import (
+	"bufio"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -16,6 +17,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/xerrors"
 
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/agent"
@@ -1024,3 +1026,195 @@ func TestAppSharing(t *testing.T) {
 		})
 	})
 }
+
+func TestWorkspaceAppsNonCanonicalHeaders(t *testing.T) {
+	t.Parallel()
+
+	setupNonCanonicalHeadersTest := func(t *testing.T, customAppHost ...string) (*codersdk.Client, codersdk.CreateFirstUserResponse, codersdk.Workspace, uint16) {
+		// Start a TCP server that manually parses the request. Golang's HTTP
+		// server canonicalizes all HTTP request headers it receives, so we
+		// can't use it to test that we forward non-canonical headers.
+		// #nosec
+		ln, err := net.Listen("tcp", ":0")
+		require.NoError(t, err)
+		go func() {
+			for {
+				c, err := ln.Accept()
+				if xerrors.Is(err, net.ErrClosed) {
+					return
+				}
+				require.NoError(t, err)
+
+				go func() {
+					s := bufio.NewScanner(c)
+
+					// Read request line.
+					assert.True(t, s.Scan())
+					reqLine := s.Text()
+					assert.True(t, strings.HasPrefix(reqLine, fmt.Sprintf("GET /?%s HTTP/1.1", proxyTestAppQuery)))
+
+					// Read headers and discard them. We collect the
+					// Sec-WebSocket-Key header (with a capital S) to respond
+					// with.
+					secWebSocketKey := "(none found)"
+					for s.Scan() {
+						if s.Text() == "" {
+							break
+						}
+
+						line := strings.TrimSpace(s.Text())
+						if strings.HasPrefix(line, "Sec-WebSocket-Key: ") {
+							secWebSocketKey = strings.TrimPrefix(line, "Sec-WebSocket-Key: ")
+						}
+					}
+
+					// Write response containing text/plain with the
+					// Sec-WebSocket-Key header.
+					res := fmt.Sprintf("HTTP/1.1 204 No Content\r\nSec-WebSocket-Key: %s\r\nConnection: close\r\n\r\n", secWebSocketKey)
+					_, err = c.Write([]byte(res))
+					assert.NoError(t, err)
+					err = c.Close()
+					assert.NoError(t, err)
+				}()
+			}
+		}()
+		t.Cleanup(func() {
+			_ = ln.Close()
+		})
+		tcpAddr, ok := ln.Addr().(*net.TCPAddr)
+		require.True(t, ok)
+
+		appHost := proxyTestSubdomainRaw
+		if len(customAppHost) > 0 {
+			appHost = customAppHost[0]
+		}
+
+		client := coderdtest.New(t, &coderdtest.Options{
+			AppHostname:                 appHost,
+			IncludeProvisionerDaemon:    true,
+			AgentStatsRefreshInterval:   time.Millisecond * 100,
+			MetricsCacheRefreshInterval: time.Millisecond * 100,
+			RealIPConfig: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{{
+					IP:   net.ParseIP("127.0.0.1"),
+					Mask: net.CIDRMask(8, 32),
+				}},
+				TrustedHeaders: []string{
+					"CF-Connecting-IP",
+				},
+			},
+		})
+
+		user := coderdtest.CreateFirstUser(t, client)
+
+		workspace := createWorkspaceWithApps(t, client, user.OrganizationID, appHost, uint16(tcpAddr.Port))
+
+		// Configure the HTTP client to not follow redirects and to route all
+		// requests regardless of hostname to the coderd test server.
+		client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		}
+		defaultTransport, ok := http.DefaultTransport.(*http.Transport)
+		require.True(t, ok)
+		transport := defaultTransport.Clone()
+		transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+			return (&net.Dialer{}).DialContext(ctx, network, client.URL.Host)
+		}
+		client.HTTPClient.Transport = transport
+		t.Cleanup(func() {
+			transport.CloseIdleConnections()
+		})
+
+		return client, user, workspace, uint16(tcpAddr.Port)
+	}
+
+	t.Run("ProxyPath", func(t *testing.T) {
+		t.Parallel()
+
+		client, _, workspace, _ := setupNonCanonicalHeadersTest(t)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		u, err := client.URL.Parse(fmt.Sprintf("/@me/%s/apps/%s/?%s", workspace.Name, proxyTestAppNameOwner, proxyTestAppQuery))
+		require.NoError(t, err)
+
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
+		require.NoError(t, err)
+
+		// Use a non-canonical header name. The S in Sec-WebSocket-Key should be
+		// capitalized according to the websocket spec, but Golang will
+		// lowercase it to match the HTTP/1 spec.
+		//
+		// Setting the header on the map directly will force the header to not
+		// be canonicalized on the client, but it will be canonicalized on the
+		// server.
+		secWebSocketKey := "test-dean-was-here"
+		req.Header["Sec-WebSocket-Key"] = []string{secWebSocketKey}
+
+		req.Header.Set(codersdk.SessionCustomHeader, client.SessionToken())
+		resp, err := client.HTTPClient.Do(req)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		// The response should be a 204 No Content with the Sec-WebSocket-Key
+		// header set to the value we sent.
+		res, err := httputil.DumpResponse(resp, true)
+		require.NoError(t, err)
+		t.Log(string(res))
+		require.Equal(t, http.StatusNoContent, resp.StatusCode)
+		require.Equal(t, secWebSocketKey, resp.Header.Get("Sec-WebSocket-Key"))
+	})
+
+	t.Run("Subdomain", func(t *testing.T) {
+		t.Parallel()
+
+		appHost := proxyTestSubdomainRaw
+		client, _, workspace, _ := setupNonCanonicalHeadersTest(t, appHost)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		user, err := client.User(ctx, codersdk.Me)
+		require.NoError(t, err)
+
+		u := fmt.Sprintf(
+			"http://%s--%s--%s--%s%s?%s",
+			proxyTestAppNameOwner,
+			proxyTestAgentName,
+			workspace.Name,
+			user.Username,
+			strings.ReplaceAll(appHost, "*", ""),
+			proxyTestAppQuery,
+		)
+
+		// Re-enable the default redirect behavior.
+		client.HTTPClient.CheckRedirect = nil
+
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
+		require.NoError(t, err)
+
+		// Use a non-canonical header name. The S in Sec-WebSocket-Key should be
+		// capitalized according to the websocket spec, but Golang will
+		// lowercase it to match the HTTP/1 spec.
+		//
+		// Setting the header on the map directly will force the header to not
+		// be canonicalized on the client, but it will be canonicalized on the
+		// server.
+		secWebSocketKey := "test-dean-was-here"
+		req.Header["Sec-WebSocket-Key"] = []string{secWebSocketKey}
+
+		req.Header.Set(codersdk.SessionCustomHeader, client.SessionToken())
+		resp, err := client.HTTPClient.Do(req)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		// The response should be a 204 No Content with the Sec-WebSocket-Key
+		// header set to the value we sent.
+		res, err := httputil.DumpResponse(resp, true)
+		require.NoError(t, err)
+		t.Log(string(res))
+		require.Equal(t, http.StatusNoContent, resp.StatusCode)
+		require.Equal(t, secWebSocketKey, resp.Header.Get("Sec-WebSocket-Key"))
+	})
+}