Parsing redirect uri wildcard #3896
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends OAuth application redirect URI handling to support comma/space-separated lists and wildcard matching, addressing Issue #3895.
Changes:
- Added
APIApplication.redirect_uri_allowed()to match redirect URIs with support for comma/space delimiters and*wildcards in host/path. - Added unit tests covering exact matches, delimiter parsing, wildcard scenarios, scheme/port checks, and invalid inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
lego/apps/oauth/models.py |
Implements redirect URI allow-list matching with delimiter support and wildcard handling. |
lego/apps/oauth/tests/test_models.py |
Adds tests for the new redirect URI matching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Support both space and comma separated URIs | ||
| raw_uris = self.redirect_uris.replace(",", " ") | ||
| allowed_uris = [u.strip() for u in raw_uris.split() if u.strip()] |
There was a problem hiding this comment.
self.redirect_uris.replace(",", " ") treats any comma as a separator, which breaks valid redirect URIs that legitimately contain commas (e.g., in the query string). Consider splitting on commas only when they are actual delimiters (e.g., CSV-style parsing or splitting on "," with optional surrounding whitespace), or documenting/enforcing that commas are not allowed inside individual URIs (requiring percent-encoding instead).
| # Check to avoid universal links such as https://*, to avoid wildcard abuse | ||
| allowed_host = parsed_allowed.hostname or "" | ||
| if allowed_host == "*" or ( | ||
| allowed_host.startswith("*.") and len(allowed_host.split(".")) < 3 | ||
| ): | ||
| continue | ||
|
|
||
| if parsed_allowed.scheme != parsed_uri.scheme: | ||
| continue | ||
|
|
||
| if not fnmatch.fnmatch( | ||
| parsed_uri.hostname or "", parsed_allowed.hostname or "" | ||
| ): | ||
| continue |
There was a problem hiding this comment.
The hostname wildcard checks only block fully-universal patterns (*, *.tld), but patterns like *example.com or ex*ample.com are still accepted and will match unrelated registrable domains (e.g., evil-example.com). If the intent is to support only subdomain wildcards (*.example.com), restrict allowed_host to either an exact hostname or a single leading *. label (and reject any other * usage in the hostname).
| if parsed_allowed.port and parsed_allowed.port != parsed_uri.port: | ||
| continue |
There was a problem hiding this comment.
Port comparison is currently one-directional: if the allowed URI omits a port, any port in the incoming redirect URI will be accepted (e.g., allowed https://example.com/callback would allow https://example.com:444/callback). For OAuth redirect URI verification, the port is part of the authority and should match; consider comparing effective ports (explicit or scheme default) for both URIs and requiring equality.
| if not fnmatch.fnmatch( | ||
| parsed_uri.hostname or "", parsed_allowed.hostname or "" | ||
| ): | ||
| continue | ||
|
|
||
| allowed_path = parsed_allowed.path or "/" | ||
| uri_path = parsed_uri.path or "/" | ||
| if not fnmatch.fnmatch(uri_path, allowed_path): |
There was a problem hiding this comment.
fnmatch.fnmatch applies OS-specific normalization (os.path.normcase), which can make URL matching behave differently across platforms (notably Windows). Since these are URLs (not filesystem paths), use fnmatch.fnmatchcase or a regex-based match to ensure consistent behavior across environments.
| if not fnmatch.fnmatch( | |
| parsed_uri.hostname or "", parsed_allowed.hostname or "" | |
| ): | |
| continue | |
| allowed_path = parsed_allowed.path or "/" | |
| uri_path = parsed_uri.path or "/" | |
| if not fnmatch.fnmatch(uri_path, allowed_path): | |
| if not fnmatch.fnmatchcase( | |
| parsed_uri.hostname or "", parsed_allowed.hostname or "" | |
| ): | |
| continue | |
| allowed_path = parsed_allowed.path or "/" | |
| uri_path = parsed_uri.path or "/" | |
| if not fnmatch.fnmatchcase(uri_path, allowed_path): |
| def test_port_matching(self): | ||
| self.app.redirect_uris = "https://example.com:8080/callback" | ||
| self.assertTrue( | ||
| self.app.redirect_uri_allowed("https://example.com:8080/callback") | ||
| ) | ||
| self.assertFalse( | ||
| self.app.redirect_uri_allowed("https://example.com:9090/callback") | ||
| ) | ||
| self.assertFalse(self.app.redirect_uri_allowed("https://example.com/callback")) | ||
|
|
There was a problem hiding this comment.
Test coverage gap for the port-matching rules: there’s no test asserting that an allowed URI without an explicit port (e.g., https://example.com/callback) does not allow redirects to the same host/path on a different explicit port (e.g., https://example.com:444/callback). Adding this test will lock in the intended security behavior once the implementation is corrected.
1 participant
Description
Added wildcards and support for both space and comma separated links in application redirect uris, and supporting tests.
Warning
readme requested this feature, and even though i have some simple checks to avoid abuse of wildcards, this might become dangerous if combined with another request from readme which was that everyone can add applications
Testing
Added tests to check for new scenarios with wildcard, to make sure it works as expected and it catches the worst mistakes.
Resolves #3895