diff --git a/internal/api/oauthserver/authorize_test.go b/internal/api/oauthserver/authorize_test.go index 13d82c803..b4df5ac2e 100644 --- a/internal/api/oauthserver/authorize_test.go +++ b/internal/api/oauthserver/authorize_test.go @@ -148,13 +148,15 @@ func TestValidateRequestOriginEdgeCases(t *testing.T) { tokenService := tokens.NewService(globalConfig, hooksMgr) server := NewServer(globalConfig, conn, tokenService) - t.Run("Origin with different port should be allowed (hostname matching)", func(t *testing.T) { + t.Run("Origin with different port on non-localhost should be rejected", func(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/test", nil) req.Header.Set("Origin", "https://example.com:8080") - // Should pass because hostname matches (IsRedirectURLValid allows different ports) + // Must be rejected: port mismatch on a non-loopback host. + // RFC 8252 Section 7.3 variable-port exception only applies to localhost. err := server.validateRequestOrigin(req) - assert.NoError(t, err) + assert.Error(t, err) + assert.Contains(t, err.Error(), "unauthorized request origin") }) t.Run("Case sensitivity in Origin header", func(t *testing.T) { diff --git a/internal/utilities/request.go b/internal/utilities/request.go index bd38c7381..f9988745e 100644 --- a/internal/utilities/request.go +++ b/internal/utilities/request.go @@ -98,15 +98,18 @@ func IsRedirectURLValid(config *conf.GlobalConfiguration, redirectURL string) bo base, berr := url.Parse(config.SiteURL) refurl, rerr := url.Parse(redirectURL) - - // As long as the referrer came from the site, we will redirect back there - if berr == nil && rerr == nil && base.Hostname() == refurl.Hostname() { - return true + if berr != nil || rerr != nil { + // either URL is for some reason invalid + return false } - if rerr != nil { - // redirect URL is for some reason invalid - return false + // Allow redirects back to the site: scheme, host and port must match. The port + // check is skipped for loopback addresses, since per RFC 8252 Section 7.3 native + // apps must be allowed to use variable port numbers. + if base.Hostname() == refurl.Hostname() && + base.Scheme == refurl.Scheme && + (base.Port() == refurl.Port() || isLocalhost(refurl.Hostname())) { + return true } scheme := strings.TrimSuffix(strings.ToLower(refurl.Scheme), ":") diff --git a/internal/utilities/request_test.go b/internal/utilities/request_test.go index 91ae97fac..b7b825fbb 100644 --- a/internal/utilities/request_test.go +++ b/internal/utilities/request_test.go @@ -10,6 +10,100 @@ import ( "github.com/supabase/auth/internal/sbff" ) +func TestIsRedirectURLValidSameOrigin(t *tst.T) { + cases := []struct { + desc string + siteURL string + redirectURL string + want bool + }{ + { + desc: "exact match", + siteURL: "https://example.com", + redirectURL: "https://example.com/path", + want: true, + }, + { + desc: "scheme downgrade https→http rejected", + siteURL: "https://example.com", + redirectURL: "http://example.com/path", + want: false, + }, + { + desc: "scheme upgrade http→https rejected", + siteURL: "http://example.com", + redirectURL: "https://example.com/path", + want: false, + }, + { + desc: "different port rejected", + siteURL: "https://example.com", + redirectURL: "https://example.com:8443/path", + want: false, + }, + { + desc: "explicit port matches SiteURL explicit port", + siteURL: "https://example.com:9000", + redirectURL: "https://example.com:9000/path", + want: true, + }, + { + desc: "no port vs explicit port rejected", + siteURL: "https://example.com:9000", + redirectURL: "https://example.com/path", + want: false, + }, + { + desc: "different explicit ports rejected", + siteURL: "https://example.com:9000", + redirectURL: "https://example.com:9001/path", + want: false, + }, + // RFC 8252 Section 7.3: variable ports must be allowed for localhost + { + desc: "localhost with different port allowed (RFC 8252 Section 7.3)", + siteURL: "http://localhost:3000", + redirectURL: "http://localhost:8080/callback", + want: true, + }, + { + desc: "127.0.0.1 with different port allowed (RFC 8252 Section 7.3)", + siteURL: "http://127.0.0.1:3000", + redirectURL: "http://127.0.0.1:8080/callback", + want: true, + }, + { + desc: "localhost without port in redirect allowed (RFC 8252 Section 7.3)", + siteURL: "http://localhost:3000", + redirectURL: "http://localhost/callback", + want: true, + }, + { + desc: "localhost scheme downgrade still rejected despite RFC 8252", + siteURL: "https://localhost:3000", + redirectURL: "http://localhost:8080/callback", + want: false, + }, + { + desc: "non-localhost variable port still rejected", + siteURL: "https://example.com:9000", + redirectURL: "https://example.com:9001/path", + want: false, + }, + } + + for _, c := range cases { + t.Run(c.desc, func(t *tst.T) { + config := conf.GlobalConfiguration{ + SiteURL: c.siteURL, + JWT: conf.JWTConfiguration{Secret: "testsecret"}, + } + require.NoError(t, config.ApplyDefaults()) + require.Equal(t, c.want, IsRedirectURLValid(&config, c.redirectURL)) + }) + } +} + func TestGetIPAddressWithSBFF(t *tst.T) { testCases := []struct { name string @@ -217,6 +311,21 @@ func TestGetReferrer(t *tst.T) { redirectURL: "http://[0:0:0:0:0:0:0:1]:12345/path", expected: "http://[0:0:0:0:0:0:0:1]:12345/path", }, + { + desc: "same origin allowed", + redirectURL: "https://example.com/dashboard", + expected: "https://example.com/dashboard", + }, + { + desc: "same hostname but http scheme rejected (scheme downgrade)", + redirectURL: "http://example.com/dashboard", + expected: config.SiteURL, + }, + { + desc: "same hostname and scheme but explicit non-default port rejected", + redirectURL: "https://example.com:8443/dashboard", + expected: config.SiteURL, + }, } for _, c := range cases {