diff --git a/cmd/cloudx/proxy/helpers.go b/cmd/cloudx/proxy/helpers.go index 79e4824f..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" @@ -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(), diff --git a/cmd/cloudx/proxy/helpers_test.go b/cmd/cloudx/proxy/helpers_test.go index 8a694ea2..db0dfc74 100644 --- a/cmd/cloudx/proxy/helpers_test.go +++ b/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"}