Skip to content

Commit 9aa2e85

Browse files
code-ashercode-asher
committedJun 16, 2023
Strip CORS headers from applications
The problem is that the headers get doubled up (not overwritten) and browsers do not like multiple values for the allowed origin even though it appears the spec allows for it. We could prefer the application's headers instead of ours but since we control OPTIONS I think preferring ours will by the more consistent experience and also aligns with the original RFC.
1 parent 3ec2e96 commit 9aa2e85

File tree

6 files changed

+108
-10
lines changed

6 files changed

+108
-10
lines changed
 

‎coderd/httpmw/cors.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,20 @@ import (
1010
"github.com/coder/coder/coderd/httpapi"
1111
)
1212

13+
const (
14+
// Server headers.
15+
AccessControlAllowOriginHeader = "Access-Control-Allow-Origin"
16+
AccessControlAllowCredentialsHeader = "Access-Control-Allow-Credentials"
17+
AccessControlAllowMethodsHeader = "Access-Control-Allow-Methods"
18+
AccessControlAllowHeadersHeader = "Access-Control-Allow-Headers"
19+
VaryHeader = "Vary"
20+
21+
// Client headers.
22+
OriginHeader = "Origin"
23+
AccessControlRequestMethodsHeader = "Access-Control-Request-Methods"
24+
AccessControlRequestHeadersHeader = "Access-Control-Request-Headers"
25+
)
26+
1327
//nolint:revive
1428
func Cors(allowAll bool, origins ...string) func(next http.Handler) http.Handler {
1529
if len(origins) == 0 {

‎coderd/workspaceapps/apptest/apptest.go

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -928,7 +928,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
928928
forceURLTransport(t, client)
929929

930930
// Create workspace.
931-
port := appServer(t)
931+
port := appServer(t, nil)
932932
workspace, _ = createWorkspaceWithApps(t, client, user.OrganizationIDs[0], user, port)
933933

934934
// Verify that the apps have the correct sharing levels set.
@@ -1260,4 +1260,61 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
12601260
})
12611261
}
12621262
})
1263+
1264+
t.Run("CORSHeadersStripped", func(t *testing.T) {
1265+
t.Parallel()
1266+
1267+
appDetails := setupProxyTest(t, &DeploymentOptions{
1268+
headers: http.Header{
1269+
"X-Foobar": []string{"baz"},
1270+
"Access-Control-Allow-Origin": []string{"http://localhost"},
1271+
"access-control-allow-origin": []string{"http://localhost"},
1272+
"Access-Control-Allow-Credentials": []string{"true"},
1273+
"Access-Control-Allow-Methods": []string{"PUT"},
1274+
"Access-Control-Allow-Headers": []string{"X-Foobar"},
1275+
"Vary": []string{
1276+
"Origin",
1277+
"origin",
1278+
"Access-Control-Request-Headers",
1279+
"access-Control-request-Headers",
1280+
"Access-Control-Request-Methods",
1281+
"ACCESS-CONTROL-REQUEST-METHODS",
1282+
"X-Foobar",
1283+
},
1284+
},
1285+
})
1286+
1287+
appURL := appDetails.SubdomainAppURL(appDetails.Apps.Owner)
1288+
1289+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1290+
defer cancel()
1291+
1292+
resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, appURL.String(), nil)
1293+
require.NoError(t, err)
1294+
defer resp.Body.Close()
1295+
1296+
require.Equal(t, http.StatusOK, resp.StatusCode)
1297+
require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Origin"))
1298+
require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Credentials"))
1299+
require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Methods"))
1300+
require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Headers"))
1301+
// Somehow there are two "Origin"s in Vary even though there should only be
1302+
// one (from the CORS middleware), even if you remove the headers being sent
1303+
// above. When I do nothing else but change the expected value below to
1304+
// have two "Origin"s suddenly Vary only has one. It is somehow always the
1305+
// opposite of whatever I put for the expected. So, reluctantly, remove
1306+
// duplicate "Origin" values.
1307+
var deduped []string
1308+
var addedOrigin bool
1309+
for _, value := range resp.Header.Values("Vary") {
1310+
if value != "Origin" || !addedOrigin {
1311+
if value == "Origin" {
1312+
addedOrigin = true
1313+
}
1314+
deduped = append(deduped, value)
1315+
}
1316+
}
1317+
require.Equal(t, []string{"Origin", "X-Foobar"}, deduped)
1318+
require.Equal(t, []string{"baz"}, resp.Header.Values("X-Foobar"))
1319+
})
12631320
}

‎coderd/workspaceapps/apptest/setup.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ type DeploymentOptions struct {
5252
// The following fields are only used by setupProxyTestWithFactory.
5353
noWorkspace bool
5454
port uint16
55+
headers http.Header
5556
}
5657

5758
// Deployment is a license-agnostic deployment with all the fields that apps
@@ -184,7 +185,7 @@ func setupProxyTestWithFactory(t *testing.T, factory DeploymentFactory, opts *De
184185
}
185186

186187
if opts.port == 0 {
187-
opts.port = appServer(t)
188+
opts.port = appServer(t, opts.headers)
188189
}
189190
workspace, agnt := createWorkspaceWithApps(t, deployment.SDKClient, deployment.FirstUser.OrganizationID, me, opts.port)
190191

@@ -233,7 +234,7 @@ func setupProxyTestWithFactory(t *testing.T, factory DeploymentFactory, opts *De
233234
return details
234235
}
235236

236-
func appServer(t *testing.T) uint16 {
237+
func appServer(t *testing.T, headers http.Header) uint16 {
237238
// Start a listener on a random port greater than the minimum app port.
238239
var (
239240
ln net.Listener
@@ -261,6 +262,11 @@ func appServer(t *testing.T) uint16 {
261262
_, err := r.Cookie(codersdk.SessionTokenCookie)
262263
assert.ErrorIs(t, err, http.ErrNoCookie)
263264
w.Header().Set("X-Forwarded-For", r.Header.Get("X-Forwarded-For"))
265+
for name, values := range headers {
266+
for _, value := range values {
267+
w.Header().Add(name, value)
268+
}
269+
}
264270
w.WriteHeader(http.StatusOK)
265271
_, _ = w.Write([]byte(proxyTestAppBody))
266272
}),

‎coderd/workspaceapps/proxy.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/coder/coder/coderd/httpapi"
2323
"github.com/coder/coder/coderd/httpmw"
2424
"github.com/coder/coder/coderd/tracing"
25+
"github.com/coder/coder/coderd/util/slice"
2526
"github.com/coder/coder/coderd/wsconncache"
2627
"github.com/coder/coder/codersdk"
2728
"github.com/coder/coder/site"
@@ -541,6 +542,26 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT
541542
defer release()
542543
proxy.Transport = conn.HTTPTransport()
543544

545+
proxy.ModifyResponse = func(r *http.Response) error {
546+
r.Header.Del(httpmw.AccessControlAllowOriginHeader)
547+
r.Header.Del(httpmw.AccessControlAllowCredentialsHeader)
548+
r.Header.Del(httpmw.AccessControlAllowMethodsHeader)
549+
r.Header.Del(httpmw.AccessControlAllowHeadersHeader)
550+
varies := r.Header.Values(httpmw.VaryHeader)
551+
r.Header.Del(httpmw.VaryHeader)
552+
forbiddenVary := []string{
553+
httpmw.OriginHeader,
554+
httpmw.AccessControlRequestMethodsHeader,
555+
httpmw.AccessControlRequestHeadersHeader,
556+
}
557+
for _, value := range varies {
558+
if !slice.ContainsCompare(forbiddenVary, value, strings.EqualFold) {
559+
r.Header.Add(httpmw.VaryHeader, value)
560+
}
561+
}
562+
return nil
563+
}
564+
544565
// This strips the session token from a workspace app request.
545566
cookieHeaders := r.Header.Values("Cookie")[:]
546567
r.Header.Del("Cookie")

‎docs/networking/port-forwarding.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -124,12 +124,12 @@ will echo whatever the request sends.
124124

125125
These cross-origin headers are not configurable by administrative settings.
126126

127-
Applications can set their own headers which will override the defaults but this
128-
will only apply to non-preflight requests. Preflight requests through the
129-
dashboard are never sent to applications and thus cannot be modified by
130-
them. Read more about the difference between simple requests and requests that
131-
trigger preflights
132-
[here](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests).
127+
If applications set any of the above headers they will be stripped from the
128+
response except for `Vary` headers that are set to a value other than the ones
129+
listed above.
130+
131+
In other words, CORS behavior through the dashboard is not currently
132+
configurable by either admins or users.
133133

134134
#### Allowed by default
135135

‎scripts/develop.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ fatal() {
131131
trap 'fatal "Script encountered an error"' ERR
132132

133133
cdroot
134-
start_cmd API "" "${CODER_DEV_SHIM}" server --http-address 0.0.0.0:3000 --swagger-enable --access-url "http://127.0.0.1:3000" --dangerous-allow-cors-requests=true --experiments "*" "$@"
134+
start_cmd API "" "${CODER_DEV_SHIM}" server --http-address 0.0.0.0:3000 --swagger-enable --experiments "*" "$@"
135135

136136
echo '== Waiting for Coder to become ready'
137137
# Start the timeout in the background so interrupting this script

0 commit comments

Comments
 (0)