Conversation
A custom mobile deep-link scheme containing an underscore, e.g. com.my_cool_app.example://callback, is rejected by url.Parse because an underscore is not a valid URI scheme character per RFC 3986. Two places broke on this: - IsRedirectURLValid bailed out on the parse error and returned false before consulting the allow list, so an explicitly allow-listed deep link was silently dropped and the request fell back to SiteURL. - prepPKCERedirectURL parsed the same URL again and returned an error, turning the PKCE callback into a 500. Only the same-site allowance and the IP/hostname safety checks require a parsed redirect URL; the admin-configured allow list matches the raw string, so evaluate it even when the redirect URL does not parse. SiteURL is still required to parse, and parseable URLs take the exact same path as before (regression tests cover that decimal-form IPs are still rejected). prepPKCERedirectURL now appends the PKCE code manually when url.Parse fails, keeping it in the query component ahead of any fragment. Fixes supabase#2447
2 tasks
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Address review feedback: allowing any redirect URL that fails url.Parse to fall through to the allow list let an obfuscated http(s) URL (e.g. "https://2130706433/%zz") skip the decimal-IP and hostname safety checks. A well-formed http:// or https:// URL always parses, so only non-HTTP schemes (custom mobile deep links such as com.my_cool_app.example://) may reach the allow list without a successful parse; a http(s) URL that fails to parse is now rejected outright.
Address follow-up review feedback: a leading ASCII control character such
as a tab ("\thttps://2130706433/") makes url.Parse fail and slips past the
http(s) scheme prefix check, but HTTP clients strip it and resolve the value
as a different, unchecked address.
Reject any redirect URL containing an ASCII control character or space up
front. These are never valid in a URL, so this closes the whole class of
leading-control-character obfuscation rather than only the tab case.
Address further review feedback: a redirect URL such as "https:/\2130706433/%zz" fails url.Parse and does not start with the literal "https://", so it slipped past the previous prefix check; browsers normalize the backslash to "/" and resolve it as https://2130706433/. Match the bare "http:" / "https:" scheme prefix instead of requiring "//", so any slash or backslash obfuscation of the authority on an unparseable http(s) URL is rejected. Together with the control-character rejection this covers the known bypass variants.
Replace the incremental http(s)-obfuscation checks with a single allowlist: when a redirect URL fails to parse it may only fall through to the allow list if it is a well-formed, non-http(s) custom scheme (the underscore deep links this branch exists for). Everything else that fails to parse — an obfuscated http(s) authority, a protocol-relative "//host" URL, or a missing/invalid scheme — is rejected, since a browser may still resolve it to an unchecked (e.g. loopback) address. This closes the "//2130706433/%zz" protocol-relative bypass and is robust against the earlier %zz / backslash / control-character variants by construction rather than case by case.
The manual PKCE fallback in prepPKCERedirectURL appended "code=" without removing a "code" already present in the redirect URL, so an allow-listed deep link carrying its own code produced two code parameters. The url.Parse path uses q.Set (overwrite); match that by dropping any existing code before appending the server-issued one, so the client cannot receive an attacker-selected code.
When url.ParseQuery fails on a malformed query (e.g. "?code=attacker&bad=%zz") the fallback previously preserved the raw query verbatim, keeping the attacker-controlled code ahead of the server-issued one. Discard an unparseable query instead so only the server code is emitted.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A custom mobile deep-link redirect whose scheme contains an underscore — e.g.
com.my_cool_app.example://callback— never works: it is silently dropped and the flow falls back to Site URL, even when the URL is explicitly on the redirect allow list.Root cause
Go's
url.Parserejects an underscore in the scheme (an underscore is not a valid URI scheme character per RFC 3986):Two code paths break on that error:
internal/utilities/request.go—IsRedirectURLValidreturnsfalseon the parse error before it ever consults the admin allow list, so an allow-listed deep link is rejected.internal/api/verify.go—prepPKCERedirectURLparses the same URL again and returns the error, turning the PKCE callback into a500.This matches the "redirect URLs with underscores always fail / Site URL overrides my redirect" reports, and the silent-failure symptom, in #2447.
Fix
IsRedirectURLValid: only the same-site allowance and the IP/hostname safety checks require a parsed redirect URL — the allow-list globs match the raw string. Evaluate the allow list even when the redirect URL does not parse.SiteURLis still required to parse, and parseable URLs take the exact same path as before, so the existing IP/hostname rejections are unchanged.prepPKCERedirectURL: whenurl.Parsefails, append the PKCEcodemanually, keeping it in the query component ahead of any fragment.A non-parseable URL still has to match an admin-configured allow-list entry to be accepted, so this does not widen what a project accepts beyond what its owner already allow-listed.
Testing
internal/utilities/request_test.go: allow-listed underscore scheme accepted (exact + glob), non-allow-listed rejected, empty allow list rejected, and — proving the safety checks are intact — a decimal-form IP is still rejected even when allow-listed.internal/api/verify_test.go:prepPKCERedirectURLfor https (with/without query) and underscore schemes, including correct code-ahead-of-fragment ordering and query escaping.gofmt,go vet, and theinternal/utilitiesandinternal/api(TestVerify) suites pass.Fixes #2447