Strip CORS headers from applications (#8057)

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.

coderd/httpmw/cors.go

@@ -10,6 +10,20 @@ import (
 	"github.com/coder/coder/coderd/httpapi"
 )
 
+const (
+	// Server headers.
+	AccessControlAllowOriginHeader      = "Access-Control-Allow-Origin"
+	AccessControlAllowCredentialsHeader = "Access-Control-Allow-Credentials"
+	AccessControlAllowMethodsHeader     = "Access-Control-Allow-Methods"
+	AccessControlAllowHeadersHeader     = "Access-Control-Allow-Headers"
+	VaryHeader                          = "Vary"
+
+	// Client headers.
+	OriginHeader                      = "Origin"
+	AccessControlRequestMethodsHeader = "Access-Control-Request-Methods"
+	AccessControlRequestHeadersHeader = "Access-Control-Request-Headers"
+)
+
 //nolint:revive
 func Cors(allowAll bool, origins ...string) func(next http.Handler) http.Handler {
 	if len(origins) == 0 {

coderd/workspaceapps/apptest/apptest.go

@@ -928,7 +928,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
 			forceURLTransport(t, client)
 
 			// Create workspace.
-			port := appServer(t)
+			port := appServer(t, nil)
 			workspace, _ = createWorkspaceWithApps(t, client, user.OrganizationIDs[0], user, port)
 
 			// Verify that the apps have the correct sharing levels set.
@@ -1260,4 +1260,61 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
 			})
 		}
 	})
+
+	t.Run("CORSHeadersStripped", func(t *testing.T) {
+		t.Parallel()
+
+		appDetails := setupProxyTest(t, &DeploymentOptions{
+			headers: http.Header{
+				"X-Foobar":                         []string{"baz"},
+				"Access-Control-Allow-Origin":      []string{"http://localhost"},
+				"access-control-allow-origin":      []string{"http://localhost"},
+				"Access-Control-Allow-Credentials": []string{"true"},
+				"Access-Control-Allow-Methods":     []string{"PUT"},
+				"Access-Control-Allow-Headers":     []string{"X-Foobar"},
+				"Vary": []string{
+					"Origin",
+					"origin",
+					"Access-Control-Request-Headers",
+					"access-Control-request-Headers",
+					"Access-Control-Request-Methods",
+					"ACCESS-CONTROL-REQUEST-METHODS",
+					"X-Foobar",
+				},
+			},
+		})
+
+		appURL := appDetails.SubdomainAppURL(appDetails.Apps.Owner)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, appURL.String(), nil)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+		require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Origin"))
+		require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Credentials"))
+		require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Methods"))
+		require.Equal(t, []string(nil), resp.Header.Values("Access-Control-Allow-Headers"))
+		// Somehow there are two "Origin"s in Vary even though there should only be
+		// one (from the CORS middleware), even if you remove the headers being sent
+		// above.  When I do nothing else but change the expected value below to
+		// have two "Origin"s suddenly Vary only has one.  It is somehow always the
+		// opposite of whatever I put for the expected.  So, reluctantly, remove
+		// duplicate "Origin" values.
+		var deduped []string
+		var addedOrigin bool
+		for _, value := range resp.Header.Values("Vary") {
+			if value != "Origin" || !addedOrigin {
+				if value == "Origin" {
+					addedOrigin = true
+				}
+				deduped = append(deduped, value)
+			}
+		}
+		require.Equal(t, []string{"Origin", "X-Foobar"}, deduped)
+		require.Equal(t, []string{"baz"}, resp.Header.Values("X-Foobar"))
+	})
 }

coderd/workspaceapps/apptest/setup.go

@@ -52,6 +52,7 @@ type DeploymentOptions struct {
 	// The following fields are only used by setupProxyTestWithFactory.
 	noWorkspace bool
 	port        uint16
+	headers     http.Header
 }
 
 // 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
 	}
 
 	if opts.port == 0 {
-		opts.port = appServer(t)
+		opts.port = appServer(t, opts.headers)
 	}
 	workspace, agnt := createWorkspaceWithApps(t, deployment.SDKClient, deployment.FirstUser.OrganizationID, me, opts.port)
 
@@ -233,7 +234,7 @@ func setupProxyTestWithFactory(t *testing.T, factory DeploymentFactory, opts *De
 	return details
 }
 
-func appServer(t *testing.T) uint16 {
+func appServer(t *testing.T, headers http.Header) uint16 {
 	// Start a listener on a random port greater than the minimum app port.
 	var (
 		ln      net.Listener
@@ -261,6 +262,11 @@ func appServer(t *testing.T) uint16 {
 			_, err := r.Cookie(codersdk.SessionTokenCookie)
 			assert.ErrorIs(t, err, http.ErrNoCookie)
 			w.Header().Set("X-Forwarded-For", r.Header.Get("X-Forwarded-For"))
+			for name, values := range headers {
+				for _, value := range values {
+					w.Header().Add(name, value)
+				}
+			}
 			w.WriteHeader(http.StatusOK)
 			_, _ = w.Write([]byte(proxyTestAppBody))
 		}),

coderd/workspaceapps/proxy.go

@@ -22,6 +22,7 @@ import (
 	"github.com/coder/coder/coderd/httpapi"
 	"github.com/coder/coder/coderd/httpmw"
 	"github.com/coder/coder/coderd/tracing"
+	"github.com/coder/coder/coderd/util/slice"
 	"github.com/coder/coder/coderd/wsconncache"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/site"
@@ -541,6 +542,26 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT
 	defer release()
 	proxy.Transport = conn.HTTPTransport()
 
+	proxy.ModifyResponse = func(r *http.Response) error {
+		r.Header.Del(httpmw.AccessControlAllowOriginHeader)
+		r.Header.Del(httpmw.AccessControlAllowCredentialsHeader)
+		r.Header.Del(httpmw.AccessControlAllowMethodsHeader)
+		r.Header.Del(httpmw.AccessControlAllowHeadersHeader)
+		varies := r.Header.Values(httpmw.VaryHeader)
+		r.Header.Del(httpmw.VaryHeader)
+		forbiddenVary := []string{
+			httpmw.OriginHeader,
+			httpmw.AccessControlRequestMethodsHeader,
+			httpmw.AccessControlRequestHeadersHeader,
+		}
+		for _, value := range varies {
+			if !slice.ContainsCompare(forbiddenVary, value, strings.EqualFold) {
+				r.Header.Add(httpmw.VaryHeader, value)
+			}
+		}
+		return nil
+	}
+
 	// This strips the session token from a workspace app request.
 	cookieHeaders := r.Header.Values("Cookie")[:]
 	r.Header.Del("Cookie")