Skip to content

Commit 161465d

Browse files
deansheatherdeansheather
authored
fix: do not canonicalize Sec-WebSocket-* headers in apps (coder#5334)
* fix: do not canonicalize Sec-WebSocket-* headers in apps * chore: test for non-canonical header name subst
1 parent 85945af commit 161465d

2 files changed

Lines changed: 222 additions & 1 deletion

File tree

‎coderd/workspaceapps.go‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,23 @@ const (
3939
redirectURIQueryParam = "redirect_uri"
4040
)
4141

42+
// nonCanonicalHeaders is a map from "canonical" headers to the actual header we
43+
// should send to the app in the workspace. Some headers (such as the websocket
44+
// upgrade headers from RFC 6455) are not canonical according to the HTTP/1
45+
// spec. Golang has said that they will not add custom cases for these headers,
46+
// so we need to do it ourselves.
47+
//
48+
// Some apps our customers use are sensitive to the case of these headers.
49+
//
50+
// https://github.com/golang/go/issues/18495
51+
var nonCanonicalHeaders = map[string]string{
52+
"Sec-Websocket-Accept": "Sec-WebSocket-Accept",
53+
"Sec-Websocket-Extensions": "Sec-WebSocket-Extensions",
54+
"Sec-Websocket-Key": "Sec-WebSocket-Key",
55+
"Sec-Websocket-Protocol": "Sec-WebSocket-Protocol",
56+
"Sec-Websocket-Version": "Sec-WebSocket-Version",
57+
}
58+
4259
func (api *API) appHost(rw http.ResponseWriter, r *http.Request) {
4360
host := api.AppHostname
4461
if api.AccessURL.Port() != "" {
@@ -708,14 +725,24 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
708725
return
709726
}
710727
defer release()
728+
proxy.Transport = conn.HTTPTransport()
711729

712730
// This strips the session token from a workspace app request.
713731
cookieHeaders := r.Header.Values("Cookie")[:]
714732
r.Header.Del("Cookie")
715733
for _, cookieHeader := range cookieHeaders {
716734
r.Header.Add("Cookie", httpapi.StripCoderCookies(cookieHeader))
717735
}
718-
proxy.Transport = conn.HTTPTransport()
736+
737+
// Convert canonicalized headers to their non-canonicalized counterparts.
738+
// See the comment on `nonCanonicalHeaders` for more information on why this
739+
// is necessary.
740+
for k, v := range r.Header {
741+
if n, ok := nonCanonicalHeaders[k]; ok {
742+
r.Header.Del(k)
743+
r.Header[n] = v
744+
}
745+
}
719746

720747
// end span so we don't get long lived trace data
721748
tracing.EndHTTPSpan(r, http.StatusOK, trace.SpanFromContext(ctx))

‎coderd/workspaceapps_test.go‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package coderd_test
22

33
import (
4+
"bufio"
45
"context"
56
"encoding/json"
67
"fmt"
@@ -16,6 +17,7 @@ import (
1617
"github.com/google/uuid"
1718
"github.com/stretchr/testify/assert"
1819
"github.com/stretchr/testify/require"
20+
"golang.org/x/xerrors"
1921

2022
"cdr.dev/slog/sloggers/slogtest"
2123
"github.com/coder/coder/agent"
@@ -1024,3 +1026,195 @@ func TestAppSharing(t *testing.T) {
10241026
})
10251027
})
10261028
}
1029+
1030+
func TestWorkspaceAppsNonCanonicalHeaders(t *testing.T) {
1031+
t.Parallel()
1032+
1033+
setupNonCanonicalHeadersTest := func(t *testing.T, customAppHost ...string) (*codersdk.Client, codersdk.CreateFirstUserResponse, codersdk.Workspace, uint16) {
1034+
// Start a TCP server that manually parses the request. Golang's HTTP
1035+
// server canonicalizes all HTTP request headers it receives, so we
1036+
// can't use it to test that we forward non-canonical headers.
1037+
// #nosec
1038+
ln, err := net.Listen("tcp", ":0")
1039+
require.NoError(t, err)
1040+
go func() {
1041+
for {
1042+
c, err := ln.Accept()
1043+
if xerrors.Is(err, net.ErrClosed) {
1044+
return
1045+
}
1046+
require.NoError(t, err)
1047+
1048+
go func() {
1049+
s := bufio.NewScanner(c)
1050+
1051+
// Read request line.
1052+
assert.True(t, s.Scan())
1053+
reqLine := s.Text()
1054+
assert.True(t, strings.HasPrefix(reqLine, fmt.Sprintf("GET /?%s HTTP/1.1", proxyTestAppQuery)))
1055+
1056+
// Read headers and discard them. We collect the
1057+
// Sec-WebSocket-Key header (with a capital S) to respond
1058+
// with.
1059+
secWebSocketKey := "(none found)"
1060+
for s.Scan() {
1061+
if s.Text() == "" {
1062+
break
1063+
}
1064+
1065+
line := strings.TrimSpace(s.Text())
1066+
if strings.HasPrefix(line, "Sec-WebSocket-Key: ") {
1067+
secWebSocketKey = strings.TrimPrefix(line, "Sec-WebSocket-Key: ")
1068+
}
1069+
}
1070+
1071+
// Write response containing text/plain with the
1072+
// Sec-WebSocket-Key header.
1073+
res := fmt.Sprintf("HTTP/1.1 204 No Content\r\nSec-WebSocket-Key: %s\r\nConnection: close\r\n\r\n", secWebSocketKey)
1074+
_, err = c.Write([]byte(res))
1075+
assert.NoError(t, err)
1076+
err = c.Close()
1077+
assert.NoError(t, err)
1078+
}()
1079+
}
1080+
}()
1081+
t.Cleanup(func() {
1082+
_ = ln.Close()
1083+
})
1084+
tcpAddr, ok := ln.Addr().(*net.TCPAddr)
1085+
require.True(t, ok)
1086+
1087+
appHost := proxyTestSubdomainRaw
1088+
if len(customAppHost) > 0 {
1089+
appHost = customAppHost[0]
1090+
}
1091+
1092+
client := coderdtest.New(t, &coderdtest.Options{
1093+
AppHostname: appHost,
1094+
IncludeProvisionerDaemon: true,
1095+
AgentStatsRefreshInterval: time.Millisecond * 100,
1096+
MetricsCacheRefreshInterval: time.Millisecond * 100,
1097+
RealIPConfig: &httpmw.RealIPConfig{
1098+
TrustedOrigins: []*net.IPNet{{
1099+
IP: net.ParseIP("127.0.0.1"),
1100+
Mask: net.CIDRMask(8, 32),
1101+
}},
1102+
TrustedHeaders: []string{
1103+
"CF-Connecting-IP",
1104+
},
1105+
},
1106+
})
1107+
1108+
user := coderdtest.CreateFirstUser(t, client)
1109+
1110+
workspace := createWorkspaceWithApps(t, client, user.OrganizationID, appHost, uint16(tcpAddr.Port))
1111+
1112+
// Configure the HTTP client to not follow redirects and to route all
1113+
// requests regardless of hostname to the coderd test server.
1114+
client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
1115+
return http.ErrUseLastResponse
1116+
}
1117+
defaultTransport, ok := http.DefaultTransport.(*http.Transport)
1118+
require.True(t, ok)
1119+
transport := defaultTransport.Clone()
1120+
transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
1121+
return (&net.Dialer{}).DialContext(ctx, network, client.URL.Host)
1122+
}
1123+
client.HTTPClient.Transport = transport
1124+
t.Cleanup(func() {
1125+
transport.CloseIdleConnections()
1126+
})
1127+
1128+
return client, user, workspace, uint16(tcpAddr.Port)
1129+
}
1130+
1131+
t.Run("ProxyPath", func(t *testing.T) {
1132+
t.Parallel()
1133+
1134+
client, _, workspace, _ := setupNonCanonicalHeadersTest(t)
1135+
1136+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1137+
defer cancel()
1138+
1139+
u, err := client.URL.Parse(fmt.Sprintf("/@me/%s/apps/%s/?%s", workspace.Name, proxyTestAppNameOwner, proxyTestAppQuery))
1140+
require.NoError(t, err)
1141+
1142+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
1143+
require.NoError(t, err)
1144+
1145+
// Use a non-canonical header name. The S in Sec-WebSocket-Key should be
1146+
// capitalized according to the websocket spec, but Golang will
1147+
// lowercase it to match the HTTP/1 spec.
1148+
//
1149+
// Setting the header on the map directly will force the header to not
1150+
// be canonicalized on the client, but it will be canonicalized on the
1151+
// server.
1152+
secWebSocketKey := "test-dean-was-here"
1153+
req.Header["Sec-WebSocket-Key"] = []string{secWebSocketKey}
1154+
1155+
req.Header.Set(codersdk.SessionCustomHeader, client.SessionToken())
1156+
resp, err := client.HTTPClient.Do(req)
1157+
require.NoError(t, err)
1158+
defer resp.Body.Close()
1159+
1160+
// The response should be a 204 No Content with the Sec-WebSocket-Key
1161+
// header set to the value we sent.
1162+
res, err := httputil.DumpResponse(resp, true)
1163+
require.NoError(t, err)
1164+
t.Log(string(res))
1165+
require.Equal(t, http.StatusNoContent, resp.StatusCode)
1166+
require.Equal(t, secWebSocketKey, resp.Header.Get("Sec-WebSocket-Key"))
1167+
})
1168+
1169+
t.Run("Subdomain", func(t *testing.T) {
1170+
t.Parallel()
1171+
1172+
appHost := proxyTestSubdomainRaw
1173+
client, _, workspace, _ := setupNonCanonicalHeadersTest(t, appHost)
1174+
1175+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1176+
defer cancel()
1177+
1178+
user, err := client.User(ctx, codersdk.Me)
1179+
require.NoError(t, err)
1180+
1181+
u := fmt.Sprintf(
1182+
"http://%s--%s--%s--%s%s?%s",
1183+
proxyTestAppNameOwner,
1184+
proxyTestAgentName,
1185+
workspace.Name,
1186+
user.Username,
1187+
strings.ReplaceAll(appHost, "*", ""),
1188+
proxyTestAppQuery,
1189+
)
1190+
1191+
// Re-enable the default redirect behavior.
1192+
client.HTTPClient.CheckRedirect = nil
1193+
1194+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
1195+
require.NoError(t, err)
1196+
1197+
// Use a non-canonical header name. The S in Sec-WebSocket-Key should be
1198+
// capitalized according to the websocket spec, but Golang will
1199+
// lowercase it to match the HTTP/1 spec.
1200+
//
1201+
// Setting the header on the map directly will force the header to not
1202+
// be canonicalized on the client, but it will be canonicalized on the
1203+
// server.
1204+
secWebSocketKey := "test-dean-was-here"
1205+
req.Header["Sec-WebSocket-Key"] = []string{secWebSocketKey}
1206+
1207+
req.Header.Set(codersdk.SessionCustomHeader, client.SessionToken())
1208+
resp, err := client.HTTPClient.Do(req)
1209+
require.NoError(t, err)
1210+
defer resp.Body.Close()
1211+
1212+
// The response should be a 204 No Content with the Sec-WebSocket-Key
1213+
// header set to the value we sent.
1214+
res, err := httputil.DumpResponse(resp, true)
1215+
require.NoError(t, err)
1216+
t.Log(string(res))
1217+
require.Equal(t, http.StatusNoContent, resp.StatusCode)
1218+
require.Equal(t, secWebSocketKey, resp.Header.Get("Sec-WebSocket-Key"))
1219+
})
1220+
}

0 commit comments

Comments
 (0)