From 712ddbfcfce2a4552ac161120736be5b56cc7b39 Mon Sep 17 00:00:00 2001 From: Vaibhav Acharya Date: Mon, 1 Jun 2026 15:01:23 +0530 Subject: [PATCH] fix: validate OAuth redirect URI against allowlist when strict --- api/external.go | 61 ++++++++++++++++++++++++++++++++++++-- api/external_test.go | 54 +++++++++++++++++++++++++++++++++ conf/configuration.go | 7 +++++ conf/configuration_test.go | 9 ++++++ 4 files changed, 128 insertions(+), 3 deletions(-) diff --git a/api/external.go b/api/external.go index 8ed9ad517..0aa00e23e 100644 --- a/api/external.go +++ b/api/external.go @@ -363,11 +363,66 @@ func getErrorQueryString(err error, errorID string, log logrus.FieldLogger) *url func (a *API) getExternalRedirectURL(r *http.Request) string { ctx := r.Context() config := a.getConfig(ctx) + + candidates := []string{} if config.External.RedirectURL != "" { - return config.External.RedirectURL + candidates = append(candidates, config.External.RedirectURL) } if er := getExternalReferrer(ctx); er != "" { - return er + candidates = append(candidates, er) + } + candidates = append(candidates, config.SiteURL) + + if !config.Security.Strict { + return candidates[0] + } + + allowed := config.Security.AllowedRedirectURIs + if len(allowed) == 0 { + allowed = []string{config.SiteURL} + } + + for _, candidate := range candidates { + if isAllowedRedirectURI(candidate, allowed) { + return candidate + } + } + // Nothing matched the allowlist. Returning config.SiteURL here would + // leak OAuth tokens to a non-allowlisted destination when the operator + // configured AllowedRedirectURIs without including SiteURL. Prefer the + // operator's primary allowlisted entry instead. + return allowed[0] +} + +// isAllowedRedirectURI matches candidate against allowed by exact scheme, +// case-insensitive host, and path-segment prefix. An allowlist entry with an +// empty path or path "/" matches any path on that host. A path-scoped entry +// matches the exact path or a sub-path, but not a path that merely shares a +// string prefix (entry "/auth" matches "/auth" and "/auth/cb" but not +// "/authorize"). Subdomains do NOT match. +func isAllowedRedirectURI(candidate string, allowed []string) bool { + cu, err := url.Parse(candidate) + if err != nil || cu.Scheme == "" || cu.Host == "" { + return false + } + for _, entry := range allowed { + au, err := url.Parse(entry) + if err != nil || au.Scheme == "" || au.Host == "" { + continue + } + if cu.Scheme != au.Scheme { + continue + } + if !strings.EqualFold(cu.Host, au.Host) { + continue + } + allowedPath := strings.TrimSuffix(au.Path, "/") + if allowedPath == "" { + return true + } + if cu.Path == allowedPath || strings.HasPrefix(cu.Path, allowedPath+"/") { + return true + } } - return config.SiteURL + return false } diff --git a/api/external_test.go b/api/external_test.go index 00c721cbd..f4f9665ea 100644 --- a/api/external_test.go +++ b/api/external_test.go @@ -1,6 +1,7 @@ package api import ( + "context" "net/http" "net/http/httptest" "net/url" @@ -9,10 +10,63 @@ import ( "github.com/gofrs/uuid" "github.com/netlify/gotrue/conf" "github.com/netlify/gotrue/models" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) +func TestIsAllowedRedirectURI(t *testing.T) { + cases := []struct { + name string + candidate string + allowed []string + want bool + }{ + {"exact host", "https://app.example.com/cb", []string{"https://app.example.com"}, true}, + {"empty path on entry matches any candidate path", "https://app.example.com/auth/cb?x=1", []string{"https://app.example.com"}, true}, + {"path prefix", "https://app.example.com/auth/cb", []string{"https://app.example.com/auth"}, true}, + {"path exact", "https://app.example.com/auth", []string{"https://app.example.com/auth"}, true}, + {"path prefix mismatch", "https://app.example.com/other/cb", []string{"https://app.example.com/auth"}, false}, + {"adjacent path prefix not allowed", "https://app.example.com/authorize", []string{"https://app.example.com/auth"}, false}, + {"trailing slash entry", "https://app.example.com/auth/cb", []string{"https://app.example.com/auth/"}, true}, + {"scheme mismatch", "http://app.example.com/cb", []string{"https://app.example.com"}, false}, + {"host mismatch", "https://evil.com/cb", []string{"https://app.example.com"}, false}, + {"subdomain not allowed", "https://evil.app.example.com/cb", []string{"https://app.example.com"}, false}, + {"empty candidate", "", []string{"https://app.example.com"}, false}, + {"candidate without scheme", "/relative/path", []string{"https://app.example.com"}, false}, + {"case-insensitive host", "https://APP.EXAMPLE.COM/cb", []string{"https://app.example.com"}, true}, + {"matches second entry", "https://preview.example.com/cb", []string{"https://app.example.com", "https://preview.example.com"}, true}, + {"empty allowlist", "https://app.example.com/cb", nil, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isAllowedRedirectURI(tc.candidate, tc.allowed)) + }) + } +} + +// TestGetExternalRedirectURL_StrictFallbackUsesAllowlist guards against the +// subtle hole where strict mode would still leak a redirect to SiteURL if the +// operator configured AllowedRedirectURIs without including it. With nothing +// matching the allowlist, the result must be an allowlisted entry, not the +// raw SiteURL. +func TestGetExternalRedirectURL_StrictFallbackUsesAllowlist(t *testing.T) { + api := &API{config: &conf.GlobalConfiguration{}} + config := &conf.Configuration{ + SiteURL: "https://app.example.com", + Security: conf.SecurityConfiguration{ + Strict: true, + AllowedRedirectURIs: []string{"https://allowed.example.com"}, + }, + } + config.ApplyDefaults() + ctx, err := WithInstanceConfig(context.Background(), config, uuid.Nil) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/callback", nil).WithContext(ctx) + assert.Equal(t, "https://allowed.example.com", api.getExternalRedirectURL(req)) +} + type ExternalTestSuite struct { suite.Suite API *API diff --git a/conf/configuration.go b/conf/configuration.go index 63fbdc172..1fea0640a 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -125,6 +125,13 @@ type MailerConfiguration struct { // follow-up changes. type SecurityConfiguration struct { Strict bool `json:"strict"` + + // AllowedRedirectURIs is the allowlist for OAuth redirect URIs when Strict. + // Entries match by exact scheme and case-insensitive host; the path is + // matched as a segment prefix, so "https://app.example.com/auth" allows + // "/auth" and "/auth/cb" but not "/authorize". Subdomains are not implied. + // Empty list means only the SiteURL host is accepted. + AllowedRedirectURIs []string `json:"allowed_redirect_uris" envconfig:"ALLOWED_REDIRECT_URIS"` } // Configuration holds all the per-instance configuration. diff --git a/conf/configuration_test.go b/conf/configuration_test.go index 5e15d29e8..9a290e765 100644 --- a/conf/configuration_test.go +++ b/conf/configuration_test.go @@ -79,6 +79,15 @@ func TestSecurityConfigurationDefaults(t *testing.T) { require.NoError(t, err) assert.True(t, c.Security.Strict) }) + + t.Run("loads allowed redirect URIs from env", func(t *testing.T) { + baseEnv() + os.Setenv("GOTRUE_SECURITY_STRICT", "true") + os.Setenv("GOTRUE_SECURITY_ALLOWED_REDIRECT_URIS", "https://app.example.com,https://app.example.com/cb") + c, err := LoadConfig("") + require.NoError(t, err) + assert.Equal(t, []string{"https://app.example.com", "https://app.example.com/cb"}, c.Security.AllowedRedirectURIs) + }) } func TestNewInstancesSecureByDefault(t *testing.T) {