fix: strip upstream CORS headers in proxy and tunnel

cmd/cloudx/proxy/helpers.go

@@ -13,7 +13,7 @@ import (
 	"net/url"
 	"os"
 	"path"
-	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -195,46 +195,19 @@ func runReverseProxy(ctx context.Context, h *client.CommandHelper, stdErr io.Wri
 			}, nil
 		},
 		proxy.WithReqMiddleware(reqMiddleware(conf, oryURL, apiKey)),
-		proxy.WithRespMiddleware(func(resp *http.Response, config *proxy.HostConfig, body []byte) ([]byte, error) {
-			l, err := resp.Location()
-			if err == nil {
-				// Redirect to main page if path is the default ui welcome page.
-				if l.Path == filepath.Join(conf.pathPrefix, "/ui/welcome") {
-					resp.Header.Set("Location", conf.defaultRedirectTo.String())
-				}
-			}
-
-			return body, nil
-		}),
+		proxy.WithRespMiddleware(respMiddleware(conf)),
 	))
 
 	cleanup := func() error {
 		return nil
 	}
 
-	var originFunc func(r *http.Request, origin string) bool
-	if len(conf.corsOrigins) == 0 {
-		originFunc = func(r *http.Request, origin string) bool {
-			return true
-		}
-	}
-
-	corsOrigins, err := corsx.NormalizeOriginStrings(append(conf.corsOrigins, conf.publicURL.String()))
+	corsOpts, err := corsOptions(conf)
 	if err != nil {
 		return err
 	}
 	addr := fmt.Sprintf(":%d", conf.port)
-	ch := cors.New(cors.Options{
-		AllowedOrigins:         corsOrigins,
-		AllowOriginRequestFunc: originFunc,
-		AllowedMethods:         corsx.CORSDefaultAllowedMethods,
-		AllowedHeaders:         append(corsx.CORSRequestHeadersSafelist, append(corsx.CORSRequestHeadersExtended, conf.additionalCorsHeaders...)...),
-		ExposedHeaders:         corsx.CORSResponseHeadersSafelist,
-		MaxAge:                 0,
-		AllowCredentials:       true,
-		OptionsPassthrough:     false,
-		Debug:                  conf.isDebug,
-	})
+	ch := cors.New(corsOpts)
 
 	server := graceful.WithDefaults(&http.Server{
 		Addr:    addr,
@@ -330,6 +303,83 @@ func reqMiddleware(conf *config, oryURL *url.URL, apiKey string) proxy.ReqMiddle
 	}
 }
 
+// corsOptions returns the CORS configuration the proxy terminates browser
+// requests with. respMiddleware strips exactly the headers this configuration
+// re-adds, so both must be derived from here — tests build the handler from
+// this function rather than restating the options.
+func corsOptions(conf *config) (cors.Options, error) {
+	var originFunc func(r *http.Request, origin string) bool
+	if len(conf.corsOrigins) == 0 {
+		originFunc = func(r *http.Request, origin string) bool {
+			return true
+		}
+	}
+
+	corsOrigins, err := corsx.NormalizeOriginStrings(append(conf.corsOrigins, conf.publicURL.String()))
+	if err != nil {
+		return cors.Options{}, err
+	}
+
+	return cors.Options{
+		AllowedOrigins:         corsOrigins,
+		AllowOriginRequestFunc: originFunc,
+		// HEAD is CORS-safelisted, so browsers send it without a preflight, but
+		// corsx.CORSDefaultAllowedMethods omits it. For a method it does not
+		// allow, rs/cors adds no Access-Control-Allow-Origin at all — and since
+		// respMiddleware strips the upstream's copy, such a response would reach
+		// the browser with no CORS headers whatsoever and be rejected.
+		AllowedMethods:     slices.Concat(corsx.CORSDefaultAllowedMethods, []string{http.MethodHead}),
+		AllowedHeaders:     slices.Concat(corsx.CORSRequestHeadersSafelist, corsx.CORSRequestHeadersExtended, conf.additionalCorsHeaders),
+		ExposedHeaders:     corsx.CORSResponseHeadersSafelist,
+		MaxAge:             0,
+		AllowCredentials:   true,
+		OptionsPassthrough: false,
+		Debug:              conf.isDebug,
+	}, nil
+}
+
+// upstreamCORSHeaders are the CORS response headers the proxy strips from the
+// upstream's response.
+//
+// The proxy terminates CORS itself: the browser talks to the proxy, so the
+// proxy's own configuration is what must apply. The upstream computed its
+// headers for a different client — the proxy — so they are not meaningful at
+// this boundary, and when the upstream sets them anyway both copies end up on
+// the wire. A duplicated Access-Control-Allow-Origin is a hard failure per the
+// Fetch spec, and a duplicated Access-Control-Allow-Credentials no longer reads
+// as exactly "true", which fails a credentialed request outright rather than
+// merely dropping the credentials. Either way the browser rejects a response
+// that both the upstream and the proxy answered correctly, and the developer
+// cannot fix it from their own application.
+//
+// Access-Control-Expose-Headers is deliberately not stripped: browsers merge
+// duplicates into a single list rather than failing, so removing it would
+// silently discard headers the upstream meant to expose.
+var upstreamCORSHeaders = []string{
+	"Access-Control-Allow-Origin",
+	"Access-Control-Allow-Credentials",
+}
+
+func respMiddleware(conf *config) proxy.RespMiddleware {
+	return func(resp *http.Response, _ *proxy.HostConfig, body []byte) ([]byte, error) {
+		for _, h := range upstreamCORSHeaders {
+			resp.Header.Del(h)
+		}
+
+		l, err := resp.Location()
+		if err == nil {
+			// Redirect to main page if path is the default ui welcome page.
+			// This compares URL paths, so it must use path.Join: filepath.Join
+			// would join with a backslash on Windows and never match.
+			if l.Path == path.Join(conf.pathPrefix, "/ui/welcome") {
+				resp.Header.Set("Location", conf.defaultRedirectTo.String())
+			}
+		}
+
+		return body, nil
+	}
+}
+
 func newJWTSigner() (jose.Signer, *jose.JSONWebKeySet, error) {
 	key, err := jwksx.GenerateSigningKeys(
 		uuid.Must(uuid.NewV4()).String(),

cmd/cloudx/proxy/helpers_test.go

@@ -4,17 +4,131 @@
 package proxy
 
 import (
+	"io"
 	"net/http"
+	"net/http/httptest"
 	"net/http/httputil"
 	"net/url"
 	"testing"
 
+	"github.com/rs/cors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/ory/x/cmdx"
 	"github.com/ory/x/proxy"
 )
 
+func TestRespMiddleware(t *testing.T) {
+	const (
+		headerAllowOrigin      = "Access-Control-Allow-Origin"
+		headerAllowCredentials = "Access-Control-Allow-Credentials"
+		headerExposeHeaders    = "Access-Control-Expose-Headers"
+	)
+
+	newResponse := func(header http.Header) *http.Response {
+		return &http.Response{StatusCode: http.StatusOK, Header: header}
+	}
+
+	t.Run("case=strips CORS headers the proxy sets itself", func(t *testing.T) {
+		resp := newResponse(http.Header{
+			headerAllowOrigin:      []string{"http://localhost:3000"},
+			headerAllowCredentials: []string{"true"},
+		})
+
+		_, err := respMiddleware(&config{})(resp, &proxy.HostConfig{}, nil)
+		require.NoError(t, err)
+
+		assert.Empty(t, resp.Header.Values(headerAllowOrigin))
+		assert.Empty(t, resp.Header.Values(headerAllowCredentials))
+	})
+
+	t.Run("case=keeps Access-Control-Expose-Headers from the upstream", func(t *testing.T) {
+		resp := newResponse(http.Header{headerExposeHeaders: []string{"X-Custom"}})
+
+		_, err := respMiddleware(&config{})(resp, &proxy.HostConfig{}, nil)
+		require.NoError(t, err)
+
+		assert.Equal(t, []string{"X-Custom"}, resp.Header.Values(headerExposeHeaders),
+			"browsers merge duplicates of this header, so stripping it would only lose information")
+	})
+
+	t.Run("case=rewrites the welcome page redirect", func(t *testing.T) {
+		conf := &config{pathPrefix: "/.ory", defaultRedirectTo: cmdx.URL{URL: url.URL{Scheme: "http", Host: "localhost:3000"}}}
+		resp := newResponse(http.Header{"Location": []string{"http://localhost:4000/.ory/ui/welcome"}})
+		resp.StatusCode = http.StatusFound
+
+		_, err := respMiddleware(conf)(resp, &proxy.HostConfig{}, nil)
+		require.NoError(t, err)
+
+		assert.Equal(t, "http://localhost:3000", resp.Header.Get("Location"))
+	})
+
+	t.Run("case=leaves other redirects alone", func(t *testing.T) {
+		conf := &config{pathPrefix: "/.ory", defaultRedirectTo: cmdx.URL{URL: url.URL{Scheme: "http", Host: "localhost:3000"}}}
+		resp := newResponse(http.Header{"Location": []string{"http://localhost:4000/.ory/ui/login"}})
+		resp.StatusCode = http.StatusFound
+
+		_, err := respMiddleware(conf)(resp, &proxy.HostConfig{}, nil)
+		require.NoError(t, err)
+
+		assert.Equal(t, "http://localhost:4000/.ory/ui/login", resp.Header.Get("Location"))
+	})
+}
+
+// TestCORSHeadersAreNotDuplicated is the regression test for
+// https://github.com/ory/cli/issues/344. An upstream that handles CORS itself
+// used to produce two Access-Control-Allow-Origin headers, which browsers
+// reject outright even though both values are correct.
+func TestCORSHeadersAreNotDuplicated(t *testing.T) {
+	const origin = "http://localhost:3000"
+
+	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Access-Control-Allow-Origin", origin)
+		w.Header().Set("Access-Control-Allow-Credentials", "true")
+		_, _ = io.WriteString(w, "ok")
+	}))
+	t.Cleanup(upstream.Close)
+
+	upstreamURL, err := url.Parse(upstream.URL)
+	require.NoError(t, err)
+
+	conf := &config{publicURL: &url.URL{Scheme: "http", Host: "localhost:4000"}}
+
+	rp := httputil.NewSingleHostReverseProxy(upstreamURL)
+	rp.ModifyResponse = func(resp *http.Response) error {
+		_, err := respMiddleware(conf)(resp, &proxy.HostConfig{}, nil)
+		return err
+	}
+
+	// Built from the same helper runReverseProxy uses, so the test cannot drift
+	// away from the CORS configuration the proxy actually runs.
+	corsOpts, err := corsOptions(conf)
+	require.NoError(t, err)
+	ch := cors.New(corsOpts)
+
+	srv := httptest.NewServer(ch.Handler(rp))
+	t.Cleanup(srv.Close)
+
+	// HEAD is CORS-safelisted, so it reaches the upstream without a preflight and
+	// must still come back with the proxy's own CORS headers.
+	for _, method := range []string{http.MethodGet, http.MethodHead} {
+		t.Run("method="+method, func(t *testing.T) {
+			req, err := http.NewRequest(method, srv.URL+"/api", nil)
+			require.NoError(t, err)
+			req.Header.Set("Origin", origin)
+
+			res, err := srv.Client().Do(req)
+			require.NoError(t, err)
+			t.Cleanup(func() { _ = res.Body.Close() })
+
+			assert.Equal(t, []string{origin}, res.Header.Values("Access-Control-Allow-Origin"),
+				"a duplicated Access-Control-Allow-Origin makes the browser reject the response, and none at all blocks it too")
+			assert.Equal(t, []string{"true"}, res.Header.Values("Access-Control-Allow-Credentials"))
+		})
+	}
+}
+
 func TestReqMiddleware(t *testing.T) {
 	oryURL := &url.URL{Scheme: "https", Host: "example.projects.oryapis.com"}
 	publicURL := &url.URL{Scheme: "http", Host: "localhost:4000"}