Repository navigation
fix!: hold a redirect URI's query, path and port to its sent form too - #867
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
v0.74.0 refused a redirect URI not written as it's sent, but net/url gives a query back byte for byte, so a query with a raw space, angle bracket or malformed %-encoding was accepted and sent unencoded; a "." or ".." path segment was accepted though a browser resolves it away; an empty or impossible port passed for https; and an uppercase private-use authority passed fapi.ParseRedirectURL (so client.New) while storage.NewRegisteredClient refused it. fapi.ParseRedirectURL now refuses all four, and the new fapi.CheckRedirectURIForm is the form check: written as sent, RFC 3986 query text, no dot segment, a port of 1 to 65535, whatever the scheme. storage.NewRegisteredClient applies it to every registered redirect URI, so registration and the request-time parse agree; whether a client may use a URI's scheme stays the server's decision per request. FuzzParseRedirectURL now checks that registration accepts every URI ParseRedirectURL accepts. BREAKING CHANGE: a redirect URI with a query RFC 3986 doesn't allow, a dot path segment, an empty or impossible port, or an uppercase private-use authority is refused by NewRegisteredClient, fapi.ParseRedirectURL and client.New, and a stored client with one no longer resolves. Register it encoded, resolved, with a real port or none, and lowercase; see UPGRADING.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
osanderson
force-pushed
the
fix/redirect-uri-query-dots-ports
branch
from
November 15, 2026 17:27
488fde6 to
d321cdc
Compare
|
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
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.
Fixes Lows 6 and 7 and two Info findings from the v0.74.0 security review. This is breaking:
fix!, with a v0.75.0 section in UPGRADING.md.What was let through
v0.74.0's "written as it's sent" check compares the URI with
url.URL.String(), which gives the query back byte for byte and never touches the path's meaning. So these were accepted:https://rp.example/cb?a=<x>,?a b,?a=%zzhttps://rp.example/a/../cb(or%2e%2e)/cbhttps://rp.example:/cb,:0,:99999com.example.app://Wallet/cbParseRedirectURL(soclient.New), butNewRegisteredClientrefused it, so it was configurable but never registrableNone of these redirected anywhere other than the registered string. They're gaps in the normalized-form guarantee, and an embedder rendering the URL unescaped is the only real risk.
Change
fapi.ParseRedirectURLrefuses all four forms, for every redirect form: https, loopback http, and both private-use forms. A private-use authority must be lowercase.fapi.CheckRedirectURIForm(raw)is new. It's the form check on its own: written as sent, RFC 3986 query text, no dot segment, a port of 1–65535, whatever the scheme. A string that doesn't parse at all passes and is left toParseRedirectURL, as before.storage.NewRegisteredClientnow usesCheckRedirectURIForm, so registration and the request-time parse can't disagree again. Whether a client may use a URI's scheme stays the server's decision per request.I tried having registration call
ParseRedirectURLitself, but that also refuses URIs that are only unusable per request, such as non-loopback http on a web client. Since stored clients are rebuilt throughNewRegisteredClient, one such URI would make a whole stored client fail to load, so I kept registration to form checks only.Tests
TestParseRedirectURL, and a newTestCheckRedirectURIForm, cover accepts and refusals for each rule.NewRegisteredClientrefuses each new form.FuzzParseRedirectURLnow also asserts that every URIParseRedirectURLaccepts passesCheckRedirectURIForm. That's the cross-check the review noted was missing, since the old property restated the check itself. A 90 s run: 16.4M execs, no failures.?bad=%zz) is no longer registrable. It now uses?plus=%2b, a valid escapeurl.Values.Encodewould still rewrite, so the byte-for-byte check stays meaningful.go testat Go 1.26.11; lint is clean.🤖 Generated with Claude Code