Accept localhost dynamic-port loopback redirect_uris

[jhrozek]

Summary

• Why: native MCP clients registered through DCR (VS Code, Claude Code) register a portless loopback redirect_uri like http://localhost/callback, then listen on an ephemeral port. RFC 8252 §7.3 requires the authorization server to allow any port for loopback redirects, but fosite's matcher recognises only IP literals (127.0.0.1, [::1]) — localhost is compared by exact string equality, so http://localhost:54321/callback is rejected and the flow can never complete.
• What: /authorize rewrites the form's redirect_uri to the client's registered portless literal so fosite's exact-match validation accepts it, then restores the requested dynamic-port URI for the pending authorization. The code stays bound to the exact requested port, so the /token check is unchanged.
• Error paths keep the client's real listener. A small wrapper hands fosite's error writer the dynamic-port URI and widens its redirect_uri validity check to cover the localhost loopback case it cannot recognise. The wrapper only ever widens that answer, never narrows it — so a client whose redirect_uri fosite would have accepted unaided still gets a proper error redirect instead of a bare JSON body.
• Matching is stricter than fosite's: escaped paths compared literally (an encoded separator can't impersonate a real one), a bare ? is significant, fragments and userinfo rejected. Exact registered matches take precedence over dynamic-port matches, so a client that pinned a port is never rewritten to a different registered entry. Dynamic-port matching is restricted to public clients.
• IP-literal loopback clients are untouched — fosite already matches those natively on both success and error paths.
• Net −42 lines of production code: a single-method interface with two implementations collapsed into one free function, and a wrapper type that no longer carried any behaviour deleted along with the branch that built it.

Fixes #6189

Type of change

• Bug fix

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

New and updated coverage:

• End-to-end through /authorize → callback → /token asserting the dynamic port survives all three legs, and that a different port and a different path are both rejected at token exchange.
• Error redirects on both /authorize (post-redirect_uri validation failure) and callback (upstream IdP denies consent) land on the client's real dynamic port. Both assert the full host:port prefix — a substring check would have passed before the fix too.
• The validity override: nil client, genuine dynamic-port match, unregistered path, and a confidential client whose exact-match redirect_uri must stay valid (this last one fails if the override ever narrows again).
• Exact-match precedence in both registration orders, so the test pins order-independence rather than list luck.
• Loopback matcher hardening: encoded path separators, ForceQuery, fragment and userinfo rejection, case-insensitive LOCALHOST, confidential-client rejection.
• A public client round-tripped through Redis still gets dynamic-port matching despite being reconstructed as a different concrete type.

Changes

File	Change
handlers/authorize.go	redirect_uri rewrite; loopbackAuthorizeRequester wrapper for error redirects; warn on a failed client lookup
handlers/callback.go	error requester carries the real dynamic-port URI from the pending authorization
registration/client.go	RegisteredLoopbackRedirectURI free function replacing a single-method interface; exact-match precedence; escaped-path/ForceQuery/fragment/userinfo hardening; behaviourless LoopbackClient deleted
storage/redis.go	per-backend matcher method dropped — the free function works on any fosite.Client
storage/cimd_decorator.go	dropped the branch (and helper) that existed only to build the deleted wrapper type
server/doc.go	package doc corrected for the removed type

Does this introduce a user-facing change?

Yes. Native OAuth clients that register a portless localhost loopback redirect_uri and listen on an ephemeral port can now complete the authorization flow against the embedded auth server. Previously these requests were rejected with redirect_uri mismatch. OAuth errors now also reach such a client's real listener rather than being delivered to whatever holds port 80 (or not delivered at all).

Special notes for reviewers

• Why the form rewrite rather than a fosite hook: fosite exposes no client-side extension point for loopback matching — MatchRedirectURIWithClientRedirectURIs never consults a Client's own matcher, and its loopback exception is IP-literal-only. Rewriting the form into its exact-match branch is the only seam available without forking fosite.
• The wrapper's widen-only invariant is load-bearing. An earlier revision let the override consult only the public-clients-only loopback matcher, which would have degraded error redirects to a JSON body for any confidential client. Not reachable today (both registrars mint public clients) but it is pinned by a test now.
• Known, accepted side effect: the wrapper doesn't satisfy fosite's G11NContext, so error messages fall back to language.English. Harmless here — no MessageCatalog is configured — but it is a real difference from an unwrapped requester.
• Pre-existing, not addressed here (worth its own issue): a client with exactly one registered redirect_uri that omits redirect_uri at /authorize gets invalid_grant at /token, because fosite's defaulted value is recorded into the pending authorization and then demanded back. Present on main; only reachable for non-OIDC scope sets, since fosite rejects an omitted redirect_uri whenever openid is requested.
• Separately worth a look: IP-literal loopback clients still go through fosite's own matcher, which compares decoded paths and ignores userinfo — so a client registering http://127.0.0.1/cb%2Fchild can have a code delivered to /cb/child. Mandatory S256 PKCE bounds the impact, and closing it means either extending the rewrite to IP literals (losing fosite's correct error-path port handling) or a reject-only pre-check. Out of scope here.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.06%. Comparing base (55feedf) to head (186a788).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/server/handlers/authorize.go	83.01%	6 Missing and 3 partials ⚠️
pkg/authserver/server/registration/client.go	96.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6215      +/-   ##
==========================================
+ Coverage   72.98%   73.06%   +0.08%     
==========================================
  Files         742      745       +3     
  Lines       78398    78787     +389     
==========================================
+ Hits        57216    57566     +350     
+ Misses      17193    17191       -2     
- Partials     3989     4030      +41     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alex-feel]

#6189 reporter here — this fix is what our deployment is waiting on (Claude Code is blocked at /oauth/authorize on exactly this scenario), so I did a careful review of the diff against the failure we filed, in case it helps the review along.

What I checked, and found sound:

• The integration point matches the root cause. fosite v0.49.0 offers no client-side hook that would ever invoke a matcher like the old LoopbackClient (that's why it was dead code), so the handler-level form rewrite before NewAuthorizeRequest, plus the widen-only requester wrapper for the error paths, looks like the right way in — and both mechanisms are exercised by tests running real fosite, so the "fosite never type-asserts past the interface" and "the rewritten form survives into validation" claims are proven rather than assumed.
• The type-independent matcher is a real improvement over reviving LoopbackClient. RegisteredLoopbackRedirectURI works against any fosite.Client, which the new tests pin for Redis-reconstructed clients and CIMD-built clients — a wrapper-type approach would have silently lost loopback matching for any client that round-trips through a storage backend.
• Our exact scenario is pinned end to end: portless http://localhost/callback registered, http://localhost:<dynamic port>/callback requested, through authorize → callback → token exchange, with the RFC 6749 §10.6 binding rejections for a different port/path. I also re-fetched https://claude.ai/oauth/claude-code-client-metadata today — it still declares exactly the portless http://localhost/callback + http://127.0.0.1/callback shape (and token_endpoint_auth_method: "none"), so the fix as written covers the current client.
• IP-literal loopback behavior is preserved (deliberately left to fosite's native matching) and test-pinned on both the success and error paths, and the added RFC 6749 §3.1.2 fragment/userinfo rejection plus the encoded-path/ForceQuery edge cases close real gaps the old matcher had.

One heads-up: the branch now shows a conflict with main — #6252 (merged today) touches the same registration package.

We run this in production (Redis-backed embedded auth server, CIMD enabled) and will gladly verify the fix end to end with the real Claude Code client as soon as it ships in a release, and report back. Anything we can do to help move the review forward?

[samuv]

Thanks for the careful work here. The matcher hardening and end-to-end coverage are strong, and the current CI suite is green. I found three issues worth addressing: later /authorize failures can still lose the dynamic port, unknown clients can generate unauthenticated WARN traffic, and the loopback matcher can ignore userinfo on the registered URI. I also left one documentation nit for the deleted wrapper.

Checklist

• Tests: Strong coverage overall; please add a regression for a post-validation failure such as StorePendingAuthorization.
• Docs: One stale LoopbackClient comment remains.
• Registry impact: None.
• Security: Please tighten the registered-URI userinfo check and avoid WARN amplification for normal unknown-client input.
• Backwards compatibility: Existing IP-literal handling appears preserved; the new localhost behavior is otherwise scoped to public clients.

[samuv]

Thanks for turning these around. The dynamic-port error handling, registered-userinfo check, and stale comment are fixed with focused tests, and CI is green. I could not approve yet because the WARN classification still misses CIMD lookup failures, so the unauthenticated log-flooding case remains for HTTPS client IDs. Also, the PR commits do not include the Signed-off-by trailer required by CONTRIBUTING.md.

Checklist

• Tests: The new regressions cover post-validation redirects and storage not-found logging; please add the CIMD/Fosite not-found case.
• Docs: The stale LoopbackClient comment is fixed.
• Registry impact: None.
• Security: One warning-amplification path remains in CIMD mode.
• Backwards compatibility: No regression found in the follow-up changes.

[jhrozek]

Thanks for catching the CIMD gap. Fixed in 186a788 — logClientLookupFailure now also checks errors.Is(err, fosite.ErrNotFound), since CIMDStorageDecorator.fetch wraps that (not storage.ErrNotFound) when a CIMD client_id URL fails to resolve. Added a case to TestLogClientLookupFailure for it.

On the Signed-off-by trailer: none of the CI checks on this PR currently enforce DCO, so I'll leave the existing commits as-is rather than rewrite history on a branch with an active review, but I've signed off the new commit and will keep doing so going forward.

[samuv]

Thanks for addressing the feedback. The requester wrapping now covers the full authorization error path, registered redirect URIs reject userinfo before loopback matching, and expected not-found cases from both storage and CIMD are logged quietly. The focused regression tests cover the reported cases, and all checks are green. Looks good to me.