diff --git a/internal/api/oauthserver/service.go b/internal/api/oauthserver/service.go index 3af95d064..7e13992df 100644 --- a/internal/api/oauthserver/service.go +++ b/internal/api/oauthserver/service.go @@ -9,17 +9,19 @@ import ( "fmt" "net/url" "slices" + "strings" "time" "github.com/gofrs/uuid" "github.com/pkg/errors" "github.com/supabase/auth/internal/api/apierrors" + "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/utilities" ) // validateRedirectURIList validates a list of redirect URIs -func validateRedirectURIList(redirectURIs []string, required bool) error { +func (s *Server) validateRedirectURIList(redirectURIs []string, required bool) error { if required && len(redirectURIs) == 0 { return apierrors.NewBadRequestError(apierrors.ErrorCodeValidationFailed, "redirect_uris is required") } @@ -33,7 +35,7 @@ func validateRedirectURIList(redirectURIs []string, required bool) error { } for _, uri := range redirectURIs { - if err := validateRedirectURI(uri); err != nil { + if err := s.validateRedirectURI(uri); err != nil { return apierrors.NewBadRequestError(apierrors.ErrorCodeValidationFailed, "invalid redirect_uri '%s': %v", uri, err) } } @@ -117,9 +119,9 @@ type OAuthServerClientRegisterParams struct { } // validate validates the OAuth client registration parameters -func (p *OAuthServerClientRegisterParams) validate() error { +func (p *OAuthServerClientRegisterParams) validate(s *Server) error { // Validate redirect URIs (required for registration) - if err := validateRedirectURIList(p.RedirectURIs, true); err != nil { + if err := s.validateRedirectURIList(p.RedirectURIs, true); err != nil { return err } @@ -170,8 +172,8 @@ func (p *OAuthServerClientRegisterParams) validate() error { return nil } -// validateRedirectURI validates OAuth 2.1 redirect URIs -func validateRedirectURI(uri string) error { +// validateRedirectURI validates OAuth 2.1 redirect URIs with enhanced security checks +func (s *Server) validateRedirectURI(uri string) error { if uri == "" { return fmt.Errorf("redirect URI cannot be empty") } @@ -181,30 +183,84 @@ func validateRedirectURI(uri string) error { return fmt.Errorf("invalid URL format") } - // Must have scheme and host - if parsedURL.Scheme == "" || parsedURL.Host == "" { - return fmt.Errorf("must have scheme and host") + // Must have scheme + if parsedURL.Scheme == "" { + return fmt.Errorf("must have scheme") } - // Check scheme requirements - if parsedURL.Scheme == "http" { + // Must not have fragment + if parsedURL.Fragment != "" { + return fmt.Errorf("fragment not allowed in redirect URI") + } + + // Block dangerous schemes that could be used for attacks + scheme := strings.ToLower(parsedURL.Scheme) + dangerousSchemes := []string{"javascript", "data", "file", "vbscript", "about"} + for _, dangerous := range dangerousSchemes { + if scheme == dangerous { + return fmt.Errorf("scheme '%s' is not allowed for security reasons", scheme) + } + } + + // For OAuth client registration, we need stricter validation than general redirects + // All redirect URIs must explicitly match the allow list (no hostname-matching shortcuts) + + // Special validation for HTTP scheme + if scheme == "http" { // HTTP only allowed for localhost host := parsedURL.Hostname() - if host != "localhost" && host != "127.0.0.1" { + if host != "localhost" && host != "127.0.0.1" && !strings.HasPrefix(host, "127.") { return fmt.Errorf("HTTP scheme only allowed for localhost") } - } else if parsedURL.Scheme != "https" { - return fmt.Errorf("scheme must be HTTPS or HTTP (localhost only)") } - // Must not have fragment - if parsedURL.Fragment != "" { - return fmt.Errorf("fragment not allowed in redirect URI") + // For OAuth Dynamic Client Registration, require strict allow-list matching + // This prevents exploitation of hostname-matching shortcuts that could allow + // arbitrary paths on the auth server (e.g., https://auth.example.com/arbitrary/path) + if !isURIExplicitlyAllowed(s.config, uri) { + return fmt.Errorf("redirect URI not allowed by configuration") } return nil } +// isURIExplicitlyAllowed checks if a URI is explicitly allowed in the configuration +// This provides strict validation for OAuth client registration by requiring exact +// pattern matches from the allow list, without hostname-matching shortcuts. +// This prevents attackers from registering arbitrary paths on the auth server. +func isURIExplicitlyAllowed(config *conf.GlobalConfiguration, uri string) bool { + if uri == "" { + return false + } + + parsedURL, err := url.Parse(uri) + if err != nil { + return false + } + + scheme := strings.ToLower(parsedURL.Scheme) + + // Check against allow list patterns + // Only match patterns that start with the same scheme to prevent cross-scheme exploitation + for pattern, glob := range config.URIAllowListMap { + patternURL, err := url.Parse(pattern) + if err != nil { + continue + } + + patternScheme := strings.ToLower(patternURL.Scheme) + if patternScheme == scheme { + // Match without fragment + matchAgainst, _, _ := strings.Cut(uri, "#") + if glob.Match(matchAgainst) { + return true + } + } + } + + return false +} + // generateClientSecret generates a secure random client secret func generateClientSecret() string { b := make([]byte, 32) @@ -235,7 +291,7 @@ func ValidateClientSecret(providedSecret, storedHash string) bool { // registerOAuthServerClient creates a new OAuth server client with generated credentials func (s *Server) registerOAuthServerClient(ctx context.Context, params *OAuthServerClientRegisterParams) (*models.OAuthServerClient, string, error) { // Validate all parameters - if err := params.validate(); err != nil { + if err := params.validate(s); err != nil { return nil, "", err } @@ -362,10 +418,10 @@ func (p *OAuthServerClientUpdateParams) isEmpty() bool { } // validate validates the OAuth client update parameters -func (p *OAuthServerClientUpdateParams) validate() error { +func (p *OAuthServerClientUpdateParams) validate(s *Server) error { // Validate redirect URIs if provided if p.RedirectURIs != nil { - if err := validateRedirectURIList(*p.RedirectURIs, false); err != nil { + if err := s.validateRedirectURIList(*p.RedirectURIs, false); err != nil { return err } } @@ -404,7 +460,7 @@ func (p *OAuthServerClientUpdateParams) validate() error { // updateOAuthServerClient updates an existing OAuth client func (s *Server) updateOAuthServerClient(ctx context.Context, clientID uuid.UUID, params *OAuthServerClientUpdateParams) (*models.OAuthServerClient, error) { // Validate all parameters - if err := params.validate(); err != nil { + if err := params.validate(s); err != nil { return nil, err } diff --git a/internal/api/oauthserver/service_test.go b/internal/api/oauthserver/service_test.go index d409128c5..c1b13809b 100644 --- a/internal/api/oauthserver/service_test.go +++ b/internal/api/oauthserver/service_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/gobwas/glob" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -59,6 +60,15 @@ func (ts *OAuthServiceTestSuite) SetupTest() { // Enable OAuth server and dynamic client registration for tests ts.Config.OAuthServer.Enabled = true ts.Config.OAuthServer.AllowDynamicRegistration = true + + // Add test URIs to allow list for testing + ts.Config.URIAllowList = append(ts.Config.URIAllowList, "https://example.com/**", "https://app.example.com/**", "http://localhost:3000/**", "http://127.0.0.1:8080/**") + // Rebuild the allow list map + ts.Config.URIAllowListMap = make(map[string]glob.Glob) + for _, uri := range ts.Config.URIAllowList { + g := glob.MustCompile(uri, '.', '/') + ts.Config.URIAllowListMap[uri] = g + } } // Helper function to create test OAuth client @@ -276,34 +286,46 @@ func (ts *OAuthServiceTestSuite) TestRedirectURIValidation() { errorMsg: "redirect URI cannot be empty", }, { - name: "Invalid scheme", + name: "Invalid URI with fragment", + uri: "https://example.com/callback#fragment", + shouldError: true, + errorMsg: "fragment not allowed in redirect URI", + }, + { + name: "Invalid URI format", + uri: "not-a-uri", + shouldError: true, + errorMsg: "must have scheme", + }, + { + name: "URI not in allow list", uri: "ftp://example.com/callback", shouldError: true, - errorMsg: "scheme must be HTTPS or HTTP (localhost only)", + errorMsg: "not allowed by configuration", }, { - name: "Invalid HTTP non-localhost", - uri: "http://example.com/callback", + name: "Dangerous javascript scheme blocked", + uri: "javascript:alert('xss')", shouldError: true, - errorMsg: "HTTP scheme only allowed for localhost", + errorMsg: "not allowed for security reasons", }, { - name: "Invalid URI with fragment", - uri: "https://example.com/callback#fragment", + name: "Dangerous data scheme blocked", + uri: "data:text/html,", shouldError: true, - errorMsg: "fragment not allowed in redirect URI", + errorMsg: "not allowed for security reasons", }, { - name: "Invalid URI format", - uri: "not-a-uri", + name: "Dangerous file scheme blocked", + uri: "file:///etc/passwd", shouldError: true, - errorMsg: "must have scheme and host", + errorMsg: "not allowed for security reasons", }, } for _, tc := range testCases { ts.T().Run(tc.name, func(t *testing.T) { - err := validateRedirectURI(tc.uri) + err := ts.Server.validateRedirectURI(tc.uri) if tc.shouldError { assert.Error(t, err) if tc.errorMsg != "" { @@ -336,3 +358,91 @@ func (ts *OAuthServiceTestSuite) TestGrantTypeDefaults() { assert.Contains(ts.T(), grantTypes, "refresh_token") assert.Len(ts.T(), grantTypes, 2) } + +func (ts *OAuthServiceTestSuite) TestCustomURISchemes() { + // Test custom URI schemes when they're in the allow list + // This tests the fix for issue #2285 + + // Save original allow list + originalAllowList := ts.Config.URIAllowList + originalAllowListMap := ts.Config.URIAllowListMap + defer func() { + ts.Config.URIAllowList = originalAllowList + ts.Config.URIAllowListMap = originalAllowListMap + }() + + // Configure allow list with custom schemes (keep existing + add custom) + ts.Config.URIAllowList = append([]string{}, originalAllowList...) + ts.Config.URIAllowList = append(ts.Config.URIAllowList, "cursor://**", "com.example.app://**", "exp://**") + // Rebuild the allow list map + ts.Config.URIAllowListMap = make(map[string]glob.Glob) + for _, uri := range ts.Config.URIAllowList { + g := glob.MustCompile(uri, '.', '/') + ts.Config.URIAllowListMap[uri] = g + } + + ctx := context.Background() + + // Test 1: cursor:// scheme (for Cursor IDE) + params := &OAuthServerClientRegisterParams{ + ClientName: "Cursor IDE", + RedirectURIs: []string{"cursor://anysphere.cursor-mcp/callback"}, + RegistrationType: "dynamic", + } + + client, secret, err := ts.Server.registerOAuthServerClient(ctx, params) + require.NoError(ts.T(), err, "Should allow cursor:// scheme when in allow list") + require.NotNil(ts.T(), client) + require.NotEmpty(ts.T(), secret) + assert.Equal(ts.T(), "Cursor IDE", *client.ClientName) + assert.Equal(ts.T(), []string{"cursor://anysphere.cursor-mcp/callback"}, client.GetRedirectURIs()) + + // Test 2: Mobile app scheme (com.example.app://) + params = &OAuthServerClientRegisterParams{ + ClientName: "Mobile App", + RedirectURIs: []string{"com.example.app://sign-in/v2"}, + RegistrationType: "dynamic", + } + + client, secret, err = ts.Server.registerOAuthServerClient(ctx, params) + require.NoError(ts.T(), err, "Should allow com.example.app:// scheme when in allow list") + require.NotNil(ts.T(), client) + require.NotEmpty(ts.T(), secret) + assert.Equal(ts.T(), "Mobile App", *client.ClientName) + + // Test 3: Expo scheme (exp://) + params = &OAuthServerClientRegisterParams{ + ClientName: "Expo App", + RedirectURIs: []string{"exp://192.168.1.1:19000/--/auth/callback"}, + RegistrationType: "dynamic", + } + + client, secret, err = ts.Server.registerOAuthServerClient(ctx, params) + require.NoError(ts.T(), err, "Should allow exp:// scheme when in allow list") + require.NotNil(ts.T(), client) + require.NotEmpty(ts.T(), secret) + + // Test 4: Unauthorized custom scheme should fail + params = &OAuthServerClientRegisterParams{ + ClientName: "Malicious App", + RedirectURIs: []string{"malicious://attack"}, + RegistrationType: "dynamic", + } + + _, _, err = ts.Server.registerOAuthServerClient(ctx, params) + assert.Error(ts.T(), err, "Should reject custom scheme not in allow list") + assert.Contains(ts.T(), err.Error(), "redirect URI not allowed by configuration") + + // Test 5: Mix of custom and standard schemes + params = &OAuthServerClientRegisterParams{ + ClientName: "Multi-Platform App", + RedirectURIs: []string{"https://example.com/callback", "cursor://app/callback"}, + RegistrationType: "dynamic", + } + + client, secret, err = ts.Server.registerOAuthServerClient(ctx, params) + require.NoError(ts.T(), err, "Should allow mix of standard and custom schemes") + require.NotNil(ts.T(), client) + require.NotEmpty(ts.T(), secret) + assert.Len(ts.T(), client.GetRedirectURIs(), 2) +}