8000 feat: parsing & validating the new redirect url patterns by cemalkilic · Pull Request #2048 · supabase/auth · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: parsing & validating the new redirect url patterns #2048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cemalkilic
Copy link
Contributor

Summary

Implements the new redirect URI validation system as specified in the technical specification. This introduces a cleaner, more maintainable approach to redirect URI validation with automatic pattern categorization.

What kind of change does this PR introduce?

Feature - New redirect URI validation system with pattern categorization

What is the current behavior?

Supabase Auth currently uses an overly permissive redirect URL system that supports dangerous wildcard patterns like ** which create security vulnerabilities The validation logic is scattered and uses complex glob patterns that are difficult to maintain and debug.

What is the new behavior?

Implements Phase 1 of the redirect URI security enhancement by introducing a new categorized validation system in the internal/redirecturi package. This lays the foundation for migrating away from dangerous wildcard patterns to a more secure, OAuth-compliant approach.

New pattern categories:

  • Exact URLs: https://example.com/callback (literal matching)
  • Host-only: example.com (any HTTPS path on domain)
  • Wildcard subdomains: *.preview.app (subdomain matching only)
  • Custom schemes: myapp:// (mobile app deep links)

Additional context

Merging this PR won't have any affect on the existing behaviour.
The actual parsing & validating (shadow mode) will be enabled in a future PR.

@cemalkilic cemalkilic requested a review from a team as a code owner June 9, 2025 17:24
@coveralls
Copy link
coveralls commented Jun 9, 2025

Pull Request Test Coverage Report for Build 15561832509

Details

  • 132 of 161 (81.99%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 70.371%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/redirecturi/validator.go 34 43 79.07%
internal/redirecturi/patterns.go 98 118 83.05%
Totals Coverage Status
Change from base Build 15475144987: 0.1%
Covered Lines: 11469
Relevant Lines: 16298

💛 - Coveralls

Copy link
Contributor
@hf hf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally fine from a quick glance

}

// CategorizePattern determines the type of a redirect uri and returns a RedirectPattern
func CategorizePattern(pattern string) (RedirectPattern, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a few glob.Compile() calls in this function. It should be OK from a performance POV and definitely not a blocker, but maybe double-check (if you haven't already) what the library docs say about it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! We're only calling glob.Compile() during app startup when parsing the config, not during the requests (aka compile-once, match-many). Since it's a one-time initialization cost, there's no performance concern here. The library warns against compiling repeatedly in hot paths, which we're not doing.
We're good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0