From a751b3b55061708e35d8275341813d81e708e798 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Wed, 29 Jul 2026 14:25:33 +0200 Subject: [PATCH 1/2] fix: strip upstream CORS headers in proxy and tunnel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the upstream application handles CORS itself, its headers were forwarded alongside the ones the proxy's own CORS middleware sets, so the response carried two of each. 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", so credentials are dropped. The browser rejected a response that both the upstream and the proxy had answered correctly, and the developer could not fix it from their own application. The proxy terminates CORS: the browser talks to the proxy, so the proxy's 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 are now removed. Access-Control-Expose-Headers is deliberately left alone, because browsers merge duplicates of it into a single list rather than failing; stripping it would silently discard headers the upstream meant to expose. The response middleware is extracted into respMiddleware so it can be tested directly, mirroring reqMiddleware. Closes #344 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JYzGVwAKQ4ormxRHZg1eDu --- cmd/cloudx/proxy/helpers.go | 51 +++++++++++--- cmd/cloudx/proxy/helpers_test.go | 111 +++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 11 deletions(-) diff --git a/cmd/cloudx/proxy/helpers.go b/cmd/cloudx/proxy/helpers.go index 79e4824f..2d79df18 100644 --- a/cmd/cloudx/proxy/helpers.go +++ b/cmd/cloudx/proxy/helpers.go @@ -195,17 +195,7 @@ 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 { @@ -330,6 +320,45 @@ func reqMiddleware(conf *config, oryURL *url.URL, apiKey string) proxy.ReqMiddle } } +// 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", so credentials are dropped. 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. + if l.Path == filepath.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(), diff --git a/cmd/cloudx/proxy/helpers_test.go b/cmd/cloudx/proxy/helpers_test.go index 8a694ea2..ecfa0ae4 100644 --- a/cmd/cloudx/proxy/helpers_test.go +++ b/cmd/cloudx/proxy/helpers_test.go @@ -4,17 +4,128 @@ 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/corsx" "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) + + rp := httputil.NewSingleHostReverseProxy(upstreamURL) + rp.ModifyResponse = func(resp *http.Response) error { + _, err := respMiddleware(&config{})(resp, &proxy.HostConfig{}, nil) + return err + } + + // The same CORS options runReverseProxy configures. + ch := cors.New(cors.Options{ + AllowedOrigins: []string{origin}, + AllowOriginRequestFunc: func(*http.Request, string) bool { return true }, + AllowedMethods: corsx.CORSDefaultAllowedMethods, + AllowedHeaders: append(corsx.CORSRequestHeadersSafelist, corsx.CORSRequestHeadersExtended...), + ExposedHeaders: corsx.CORSResponseHeadersSafelist, + AllowCredentials: true, + }) + + srv := httptest.NewServer(ch.Handler(rp)) + t.Cleanup(srv.Close) + + req, err := http.NewRequest(http.MethodGet, 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") + 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"} From 9f3717967cc2cdf725e33135343a986d5ad18efe Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Wed, 29 Jul 2026 15:26:25 +0200 Subject: [PATCH 2/2] fix: keep CORS headers on HEAD requests after stripping the upstream's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stripping the upstream's Access-Control-Allow-Origin regressed cross-origin HEAD requests. HEAD is CORS-safelisted, so browsers send it without a preflight, but corsx.CORSDefaultAllowedMethods omits it — and for a method it does not allow, rs/cors returns from handleActualRequest before adding any header. The response therefore reached the browser with no CORS headers at all, where before this branch the upstream's copy made it work. HEAD is now allowed, and the CORS configuration is extracted into corsOptions so the strip list and the headers that replace it are derived from one place. The regression test builds its handler from that same helper instead of restating the options, which had already drifted from the real configuration. Also replaces filepath.Join with path.Join when matching the welcome page URL. filepath.Join joins with a backslash on Windows, so the redirect rewrite never fired there and --default-redirect-url was silently ignored. And folds the nested append chains into slices.Concat, which removes the risk of writing into a package-level slice's backing array. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JYzGVwAKQ4ormxRHZg1eDu --- cmd/cloudx/proxy/helpers.go | 69 +++++++++++++++++++++----------- cmd/cloudx/proxy/helpers_test.go | 47 ++++++++++++---------- 2 files changed, 70 insertions(+), 46 deletions(-) diff --git a/cmd/cloudx/proxy/helpers.go b/cmd/cloudx/proxy/helpers.go index 2d79df18..8ef0fa8e 100644 --- a/cmd/cloudx/proxy/helpers.go +++ b/cmd/cloudx/proxy/helpers.go @@ -13,7 +13,7 @@ import ( "net/url" "os" "path" - "path/filepath" + "slices" "strconv" "strings" "time" @@ -202,29 +202,12 @@ func runReverseProxy(ctx context.Context, h *client.CommandHelper, stdErr io.Wri 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, @@ -320,6 +303,41 @@ 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. // @@ -329,9 +347,10 @@ func reqMiddleware(conf *config, oryURL *url.URL, apiKey string) proxy.ReqMiddle // 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", so credentials are dropped. 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. +// 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 @@ -350,7 +369,9 @@ func respMiddleware(conf *config) proxy.RespMiddleware { 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") { + // 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()) } } diff --git a/cmd/cloudx/proxy/helpers_test.go b/cmd/cloudx/proxy/helpers_test.go index ecfa0ae4..db0dfc74 100644 --- a/cmd/cloudx/proxy/helpers_test.go +++ b/cmd/cloudx/proxy/helpers_test.go @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/require" "github.com/ory/x/cmdx" - "github.com/ory/x/corsx" "github.com/ory/x/proxy" ) @@ -94,36 +93,40 @@ func TestCORSHeadersAreNotDuplicated(t *testing.T) { 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(&config{})(resp, &proxy.HostConfig{}, nil) + _, err := respMiddleware(conf)(resp, &proxy.HostConfig{}, nil) return err } - // The same CORS options runReverseProxy configures. - ch := cors.New(cors.Options{ - AllowedOrigins: []string{origin}, - AllowOriginRequestFunc: func(*http.Request, string) bool { return true }, - AllowedMethods: corsx.CORSDefaultAllowedMethods, - AllowedHeaders: append(corsx.CORSRequestHeadersSafelist, corsx.CORSRequestHeadersExtended...), - ExposedHeaders: corsx.CORSResponseHeadersSafelist, - AllowCredentials: true, - }) + // 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) - req, err := http.NewRequest(http.MethodGet, 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") - assert.Equal(t, []string{"true"}, res.Header.Values("Access-Control-Allow-Credentials")) + // 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) {