fix: redirect authorize errors per RFC 6749 §4.1.2.1, except to self-asserted clients; Server.Use chains - #285
Merged
Conversation
Closes #279. Closes #276 (the Use half; the resolver-interaction half is deliberately deferred — see below). ## #279 — the ordering fix /oauth2/authorize returned an RFC 6749 §5.2 JSON body for EVERY failure. §4.1.2.1 wants most of them redirected back to the client as error + state, and exempts only the two cases where redirecting would itself be the vulnerability: an invalid client_id, and a missing or unregistered redirect_uri. What blocked it was ordering, not effort. The handler resolved the principal before any client lookup, and client lookup + redirect_uri validation both lived inside IssueAuthCode — so access_denied, the failure a browser user actually hits by not being signed in, fired with no validated redirect_uri to send it to. Split ResolveAuthorizeClient out of IssueAuthCode (client lookup, client-state gate, grant-type allow-list, redirect_uri allow-list) and call it at a new step 3.5, before principal resolution. It reuses resolveClientRegistryOrCIMD and redirectURIAllowed verbatim rather than duplicating the registry-first/CIMD policy or the RFC 8252 loopback matching, and returns the same *OAuthError values, so no response that already worked changed shape. IssueAuthCodeRequest gains an optional Client field. Set, IssueAuthCode skips the lookup and the allow-list because they already ran; nil, it behaves exactly as before, so programmatic callers are untouched. It re-checks that a pre-resolved client matches the request client_id, so a mismatch cannot smuggle an unvalidated redirect_uri past the allow-list. Post-validation failures now redirect on GET: a declining resolver chain becomes access_denied (the client is fine — we resolved it — it is the resource owner who could not be authenticated, which is what §4.1.2.1 defines the code for), and grant-type and mint failures carry their own codes. state round-trips. POST keeps JSON, deliberately. Its caller is not a browser: it is a CLI, or a surface like Studio that authenticated the user itself and posts an RFC 7523 assertion. Those have parsed JSON since v1, there is no user agent in the exchange to redirect, and redirecting would break the hosted path for no benefit. Unchanged and tested as such: the availability gate and empty-resolver 503 (a deployer misconfiguration, not the client's problem), the parse and required-field 400s, and both §4.1.2.1 exemptions. ## #276 — Server.Use chains fn became fns; Use appends; the router closure composes first-registered-outermost, matching chi. AdminAuth keeps replace semantics — admin auth is one decision, not a stack — and is pinned by a test, since it shares middlewareHolder and silently stacking two auth gates would be worse than the bug being fixed. Every existing caller calls Use exactly once (tests/integration/helpers_test.go, highflame-authn/cmd/server/main.go), so this changes no current behaviour. It fixes a live footgun rather than a hypothetical one: AuthN's slot is already occupied by the trusted-service annotator paired with SetTrustedServiceValidator, so a deployer adding any second concern would have dropped the annotator and broken external-principal exchange instead of the thing they just added. Deferred: the resolver-interaction hook (#276's other half). Auth0 renders CIMD consent itself and explicitly FAILS CIMD logins when tenant extensibility is active, i.e. for an untrusted third-party client the AS owns consent and deployer code is excluded from the path. Our analogue of that is Studio, which already fronts the browser leg and posts an assertion. A resolver hook would solve a problem the reference implementation designed away. Now that Use chains, the advice already in RegisterPrincipalResolver's godoc — do interaction in middleware, which does have a ResponseWriter — is followable. ## Tests Rewrote two existing assertions whose MECHANISM changed while the property did not. TestAuthorizeGET_CredentialInQueryStringIsIgnored asserted "Location is empty"; it now asserts no code is issued, error=access_denied, and — newly relevant now that there is a Location — that the credential does not appear in it, since the redirect is built from the registered redirect_uri and never from the inbound URL. TestAuthorizeGET_NoCredentialIs401 became ...RedirectsAccessDenied. New: POST-stays-JSON, unknown-client-stays-JSON, unregistered-redirect_uri-stays-JSON, and five for the middleware chain. Three mutation checks, all confirmed to fail: - delete the GET gate -> POST-stays-JSON fails - revert Use to replace -> "the first-registered middleware did not run" - build the error redirect from the inbound URL -> the credential-leak assertion fails with "must not echo the inbound query string" go build, go vet, gofmt clean; go test ./... green across all 9 packages; golangci-lint under CI's invocation 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things previously listed as out of scope, fixed rather than deferred. ## Resolver interaction hook — closes the other half of #276 A PrincipalResolver returns (*Principal, error) and has no ResponseWriter, so a cookie-based resolver seeing no session had no way to START a login: declining reads as "wrong credential type", failing reads as "bad credential", and neither sends the user anywhere. ErrPrincipalInteractionRequired plus Server.SetInteractiveLoginURL closes it. The resolver says "this could be satisfied by logging in" and ZeroID performs the redirect on its behalf, so resolvers still never touch the transport — which is the property that made the hook worth having in this shape. Chose the sentinel-plus-configured-target shape over letting the resolver return its own URL. The resolver picking the target would put an open-redirect guard in the handler and make every deployer's redirect logic security-relevant; a func-valued setter keeps the per-request flexibility that was the only real argument for the alternative. The setter's godoc says to derive the target from configuration, not from the request, for exactly that reason. return_to is rebuilt from the VALIDATED protocol parameters, never copied from the inbound URL. Forwarding the raw query would hand the login surface — and its logs — whatever a caller appended, including a credential in the wrong channel, as a side effect of failing to sign in. Mutation-checked. Only honoured on GET (a POST caller has no user agent) and only after step 3.5, so an unresolved client cannot bounce users through the login surface. With no target configured it degrades to access_denied: a resolver cannot conjure a surface the deployment does not have. ## client_name is now REQUIRED It was RECOMMENDED, with a fallback to the client_id. That made the weakest case — a document declining to name itself — indistinguishable from a well-formed one, and put the string a human is asked to trust in the hands of whoever picked the URL. A CIMD publisher is anonymous by construction (no registration, no secret), so this label is most of what consent has to go on. Auth0 requires it non-empty for CIMD clients for the same reason. A deviation from the draft, so it is recorded as one in spec §12.4. ## SetTrustedServiceValidator(nil) actually disables The godoc has always said "when nil (default), external principal exchange is disabled". The setter wrapped its argument unconditionally, so nil became a NON-nil closure the service would call — turning "disabled" into a nil-deref panic on the first external-principal exchange. A crash on a request path, whenever someone first used the feature, rather than a no-op at startup. Pre-existing; found while exploring for #276. ## Tests Five integration tests for the hook (redirect target, return_to resumability, extraneous-param stripping, client-must-be-resolved-first, POST-not-redirected), three for client_name, one for the nil validator. The hook's tests are integration rather than unit because step 3.5 means the handler now needs a real OAuthService before it reaches the resolver — a bare &API{} panics, which is a consequence of the #279 reordering and not worth a test seam in production code. Three more mutation checks, all confirmed to fail: - wrap nil unconditionally -> "panics at request time instead of disabling" - restore the client_name fallback -> "expected ErrCIMDInvalidDocument" - forward the inbound query into return_to -> "must not forward a query-string credential" Docs updated in the same commit: RegisterPrincipalResolver's godoc (which told deployers a resolver simply cannot start an interaction), docs/cimd.md, and spec §12.4. go build, go vet, gofmt clean; go test ./... green across 9 packages; golangci-lint under CI's invocation 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rsharath
changed the title
The reasoning behind each of these stands on its own — a consent screen shows client_name so it has to be non-empty; middleware on the authorize path is security-relevant because CIMD clients are unregistered — and attributing it to another vendor's product adds nothing a reader needs. It also dates badly: their behaviour can change without notice, and a public repo naming a competitor's implementation as justification reads oddly whichever way their product moves. Four comments in server.go, internal/service/cimd.go, its test, and spec §12.4. Substance unchanged; no behaviour change. Four pre-existing mentions remain in files this PR does not touch (internal/service/oauth_client.go, docs/identity-model.md, docs/rar.md, tests/integration/external_idp_hardening_test.go) — left alone rather than widening this PR's diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eware chain Two defects in my own changes, found reviewing this branch. ## The redirect_uri allow-list could be skipped IssueAuthCodeRequest.Client lets a caller skip the client lookup because ResolveAuthorizeClient already did it. I also skipped the redirect_uri allow-list, and guarded only that the pre-resolved client's ClientID matched — while the field's own doc comment claimed both ClientID and RedirectURI were re-checked. They were not. Consequence for a programmatic caller: resolve one (client, redirect_uri) pair, then call IssueAuthCode with a redirect_uri the client never registered, and it is accepted. That is worse than an ordinary missing input check, because redirect_uri is baked into the code as the "ruri" claim and honoured at exchange — so the allow-list is defeated end to end, not just at issuance. Not reachable through authorizeHandler, which passes the same value it resolved with. But Client and RedirectURI are exported fields on an exported request type, IssueAuthCode's own godoc names programmatic issuance as a use case, and the doc comment told callers a check existed that did not. Fixed by re-running redirectURIAllowed in the pre-resolved branch rather than comparing against a remembered value: it re-establishes the actual invariant regardless of what the caller resolved with, and it is pure string work, so the skip still avoids the only expensive part (the DB read or CIMD fetch). Doc comment now says what the code does. Four tests: the resolved pair still issues, loopback ports still float per RFC 8252 §7.3 (a strict string compare would have broken this), an unregistered URI is refused with invalid_request, and a mismatched client_id is refused. Mutation-checked — dropping the re-check fails with "would be bound into the code as ruri and honoured at exchange". ## chain() allocated on every request Making Use append meant chain() composed the middleware per call: a slice copy plus a closure, on every request through every route, where the previous single-slot read allocated nothing. Now cached on the holder and rebuilt on mutation, so chain() is a read under RLock. Use is still callable after NewServer; the cache is invalidated rather than assumed immutable. go build, go vet, gofmt clean; go test ./... green across 9 packages; golangci-lint under CI's invocation 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Reworks OAuth authorization error handling, interactive login, middleware chaining, and CIMD validation.
Changes:
- Redirects eligible GET authorization errors and adds an interactive-login hook.
- Makes
Server.Useappend middleware and fixes nil trusted-service validators. - Requires non-empty CIMD
client_namevalues and expands tests/docs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
server.go |
Adds middleware chains and interactive-login configuration. |
hooks.go |
Exposes the interaction-required sentinel. |
middleware_chain_test.go |
Tests middleware and validator behavior. |
internal/handler/authorize.go |
Implements authorization error and login redirects. |
internal/handler/routes.go |
Wires the interactive-login callback. |
internal/service/oauth.go |
Extracts client resolution and supports pre-resolved clients. |
internal/service/principal.go |
Defines the interaction-required sentinel. |
internal/service/cimd.go |
Requires a non-empty CIMD client name. |
internal/service/cimd_test.go |
Updates CIMD validation tests. |
internal/service/authcode_issue_test.go |
Tests pre-resolved client validation. |
tests/integration/helpers_test.go |
Configures the integration login hook. |
tests/integration/oauth_authorize_get_test.go |
Tests GET error disposition. |
tests/integration/oauth_authorize_interactive_test.go |
Tests interactive login redirects. |
docs/cimd.md |
Documents interactive authorization. |
docs/spec/zeroid-oauth-extensions.md |
Documents the CIMD deviation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…clamp redirects Code review of this branch. The first item is the significant one: my previous commit fixed half of a bug and the review caught the other half. ## The pre-resolved-client skip had a second gate behind it Fixing redirect_uri by re-checking it in the else branch left the GRANT-TYPE allow-list skipped on that same path — it had moved into ResolveAuthorizeClient, so it only ran when Client was nil. IssueAuthCode's own godoc still listed it unconditionally. Concretely: Client with GrantTypes ["refresh_token"] minted a code, while the identical call with Client nil returned unauthorized_client. The lesson is the pattern, not the gate: "re-add the check to the other branch" does not survive the next gate anyone adds. Both cheap invariants now live in checkAuthorizeClientPolicy, called on EVERY path into IssueAuthCode — ResolveAuthorizeClient calls it, and IssueAuthCode calls it again after either branch. Running it twice on the nil path costs a slice scan and a string compare; that is the price of the skip being impossible. Grant-type subtest added alongside the redirect_uri ones. Mutation-checked: removing the shared call fails BOTH subtests together, which is the property that was missing before. ## S256 hoisted into the cheap gate code_challenge_method != "S256" was caught only inside IssueAuthCode. With the interactive-login hook in place that meant a request carrying plain could be sent through the deployer's login surface, make the user authenticate, and only then fail on a parameter the caller supplied. Making somebody sign in to be told their own request was malformed is a poor trade for a string comparison. IssueAuthCode still enforces it for programmatic callers. ## error_description clamped to the RFC 6749 §4.1.2.1 charset §4.1.2.1's NQCHAR is %x20-21 / %x23-5B / %x5D-7E — printable ASCII, no double quote, no backslash. Harmless while these strings only appeared in a JSON body (§5.2 sets no charset limit) but they now ride in a Location query, where a client validating strictly may reject a non-ASCII byte. One of my own descriptions carried an em dash. Clamped at the redirect boundary rather than by auditing every literal, because the descriptions reaching there include service-layer strings from extractOAuthError, and "keep this ASCII" does not survive the next error message. The em dash in my own string is also gone. The JSON body is left as UTF-8, where it reads better and is permitted. ## Docs my own reordering falsified The authorizeHandler header comment still said "EVERY failure here returns an RFC 6749 §5.2 JSON body", explained that access_denied "stays JSON until the lookup moves", listed client validation at step 6, and closed with "Tracked in #279" — while the step-3.5 comment pointed readers back at it for a rationale that was no longer true. resolvePrincipal's godoc still mapped both no-match and resolver-error to 401 with no mention of the new sentinel. Both rewritten. Also documented on SetInteractiveLoginURL: return_to is a reserved parameter name that will overwrite one already on the target, and there is NO loop guard, so a surface that bounces back without establishing a session will loop. go build, go vet, gofmt clean; go test ./... green across 9 packages; golangci-lint under CI's invocation 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ient can redirect Third review pass. Two real findings, and the first is the same class for the third time — which is the point worth recording. ## A third gate was skipped on the pre-resolved-client path The IsActive / ClientType == "public" gate lived in ResolveAuthorizeClient's switch, so IssueAuthCodeRequest.Client skipped it — while the comment I had just written two lines below claimed "Neither gate is safe to skip on the strength of 'the caller already resolved it'". A caller that cached a resolved client, or resolved one by another route, could mint codes for a DEACTIVATED or CONFIDENTIAL client, defeating the deactivation-is-a-kill-switch invariant #241 established. That is three gates found in three passes: redirect_uri, then grant-type, now client state. Each time the fix was "re-add this one to the other branch", and each time it left the next one behind. All three now live in checkAuthorizeClientPolicy, which every path into IssueAuthCode calls, and there is no branch left that can skip one. The gate was previously conditioned on !viaCIMD; it is now unconditional, which changes nothing because synthesizeCIMDClient always produces IsActive: true, ClientType: "public" (pinned by TestSynthesizeCIMDClient). ## unauthorized_client could never be redirected checkAuthorizeClientPolicy checked the grant-type allow-list BEFORE redirect_uri, and both errors came back from ResolveAuthorizeClient indistinguishably, so the handler answered both with JSON. RFC 6749 §4.1.2.1 lists unauthorized_client as redirectable and exempts only invalid client_id and bad redirect_uri. So a public client with a correctly registered redirect_uri whose grant_types omit authorization_code got a raw JSON blob in the browser while its callback never fired — precisely the dead end this PR exists to remove, reintroduced by my own gate ordering. Fixed by ordering the gates state -> redirect_uri -> grant-type and having checkAuthorizeClientPolicy report whether a redirect target was established before the failure. ResolveAuthorizeClient returns the client alongside the error in that case, and the handler reads a non-nil client as "this one may be redirected". Ordering is now itself a tested property: a client failing BOTH gates must report the redirect_uri refusal, since that is the one §4.1.2.1 exempts. Four new subtests (deactivated, confidential, grant-type, ordering). Mutation-checked: dropping the state gate fails the first two, and swapping the gate order back fails the ordering test with "got unauthorized_client". ## Also Use(nil) is now documented as ignored rather than clearing — an appending registry has no removal, and dropping every prior registration would be worse than a no-op. It differs from AdminAuth(nil), which does clear, because that is a single slot. go build, go vet, gofmt clean; go test ./... green across 9 packages; golangci-lint under CI's invocation 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the carve-out three review passes asked for. This is a deliberate deviation from RFC 6749 §4.1.2.1, so the reasoning is recorded in the code, docs/cimd.md and spec §12.5 rather than left to a commit message. ## What and why §4.1.2.1's redirect rule rests on "registered" meaning a party somebody vetted. CIMD removes registration by design: redirect_uris come from a document the requester published, CIMD is on by default, and allowed_domains ships empty. So for a CIMD client the destination is attacker-CHOSEN, not merely attacker-supplied, and honouring the rule makes /oauth2/authorize an unauthenticated redirector — reachable with no credential, because the failure being reported IS "you have no credential" — with our own origin as the first hop. Two paths now refuse for such a client: - failAuthorize answers the §5.2 JSON body instead of a 302. - redirectToInteractiveLogin declines. That is the more damaging half: sending a victim through the deployment's real login page on behalf of an unvetted client means they authenticate for real and the flow resumes toward an attacker-published redirect_uri. An unvetted client does not get to borrow the login surface's credibility. ## Provenance, not CIMD The discriminator is domain.OAuthClient.SelfAsserted() — registration_source == "cimd" — not a feature flag. Registered and RFC 7591 dynamically-registered clients are unaffected and keep the conformant redirect, and a deployer who sets cimd.allowed_domains re-establishes the vetting §4.1.2.1 assumes, which restores redirects for CIMD too. Moved the source constant into domain as RegistrationSourceCIMD so the predicate lives with the type rather than the handler string-matching. Also fails closed on a nil client: failAuthorize's premise is that a validated redirect_uri exists, and nil means it does not. ## The cost, stated plainly A browser-driven CIMD client can no longer learn its error from the callback and must read the JSON body — which is the ergonomic problem #279 set out to fix, kept for the one client class whose redirect target nobody vetted. That trade is the point of the change, not a side effect, and allowed_domains is the way out. Six tests, both branches, plus a nil case. Mutation-checked: removing either carve-out fails with the Location it would have emitted ("...?error=access_denied&...") and with "must NOT borrow the login surface". go build, go vet, gofmt clean; go test ./... green across 9 packages; golangci-lint under CI's invocation 0 issues; spec anchors intact after renumbering 12.5-12.8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rsharath
changed the title
Review raised that cimd.allowed_domains is deployment-wide while every other authorization decision here is scoped to (account_id, project_id). It is right, and it lands on wording this PR shipped. The carve-out offers "set cimd.allowed_domains" as the way to restore §4.1.2.1 error redirects for CIMD clients. domainAllowed takes no tenant and CIMDConfig hangs off Config with no tenant dimension anywhere, so on a multi-tenant deployment that hatch is all-or-nothing: restore redirects for everyone, accepting one customer's publishers on behalf of all, or for nobody. On the hosted product it therefore means CIMD clients do not get error redirects — the opposite of what the docs implied. Worse for a quick fix: the tenant is not known at the point CIMD resolves. This PR hoisted client resolution ahead of principal resolution on purpose, and a synthesized CIMD client carries no tenant fields because nothing in the request names one. So domainAllowed(tenant, host) has no tenant to pass, and a per-tenant allowlist is a design question rather than a missing config field. Filed as #286 with four options; the promising one is to split tenant-independent document validation from tenant-scoped publisher policy, or to hand the policy to a deployer hook the way PrincipalResolver and TrustedServiceValidator already are. Docs only — the behaviour was always this, the claim about it was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saucam
approved these changes
Aug 11, 2026
Review follow-up (PR #285). The response_type / PKCE-presence / S256 gates ran before client resolution, so a browser client with a valid registered redirect_uri still got a JSON dead end for its own malformed parameter. They now run at step 3.75 — after ResolveAuthorizeClient, so the failure redirects with error + state (unsupported_response_type for a non-code response_type, invalid_request otherwise), and before principal resolution, so a doomed request never bounces through a login surface. POST keeps the §5.2 JSON shape it has had since v1, including invalid_request as the JSON code for a non-code response_type. Also corrects the step-6 comment that said IssueAuthCode skips the redirect_uri allow-list for a pre-resolved client — it re-runs every policy gate via checkAuthorizeClientPolicy and skips only the lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rsharath
added a commit
that referenced
this pull request
Aug 21, 2026
#285 landed ErrPrincipalInteractionRequired + SetInteractiveLoginURL, so a cookie resolver no longer has to hand-roll the 302 to its login screen. It also refuses that redirect for a self-asserted (CIMD) client, which is the half that matters in this doc: on route 1 a CIMD authorization request only succeeds for a user who already has a session.
rsharath
added a commit
that referenced
this pull request
Aug 21, 2026
#285 shipped the §4.1.2.1 self-asserted carve-out with an escape hatch that three places document and no code implements. failAuthorize and redirectToInteractiveLogin both gated on client.SelfAsserted() alone, and RegistrationSource is set to "cimd" unconditionally at synthesis, so setting cimd.allowed_domains changed nothing. The handler never even received the allow-list — API carried only cimdEnabled. Both gates now go through one predicate, refusesRedirectTo, so the error redirect and the interactive-login redirect cannot drift apart: they answer the same question about the same client. Server.NewServer feeds it AllowedDomainCount() > 0 — the EFFECTIVE list, for the same reason the startup log uses it, since allowed_domains: [""] has length 1 and vets nothing. This is what makes the browser leg completable for an MCP CIMD client. An unvetted one is never sent to the login surface, so a user with no session cannot establish one and the flow cannot finish at all — the allow-list is the switch, and it was wired to nothing. The empty-allowlist startup warning now names that consequence too.
rsharath
added a commit
that referenced
this pull request
Aug 21, 2026
#285 shipped the §4.1.2.1 self-asserted carve-out with an escape hatch that three places document and no code implements. failAuthorize and redirectToInteractiveLogin both gated on client.SelfAsserted() alone, and RegistrationSource is set to "cimd" unconditionally at synthesis, so setting cimd.allowed_domains changed nothing. The handler never even received the allow-list — API carried only cimdEnabled. Both gates now go through one predicate, refusesRedirectTo, so the error redirect and the interactive-login redirect cannot drift apart: they answer the same question about the same client. Server.NewServer feeds it AllowedDomainCount() > 0 — the EFFECTIVE list, for the same reason the startup log uses it, since allowed_domains: [""] has length 1 and vets nothing. This is what makes the browser leg completable for an MCP CIMD client. An unvetted one is never sent to the login surface, so a user with no session cannot establish one and the flow cannot finish at all — the allow-list is the switch, and it was wired to nothing. The empty-allowlist startup warning now names that consequence too. Also ignores .gstack/, which is per-session browser audit output.
rsharath
added a commit
that referenced
this pull request
Aug 21, 2026
…publishers Review of the previous commit found the premise it rests on is not actually established. refusesRedirectTo reads "an allow-listed publisher is a vetted party, so redirects apply again" — but nothing tied redirect_uris to the allow-list, or even to the client_id host. synthesizeCIMDClient checks scheme rules only. So on any host where more than one party can publish a path — user content, a raw-file CDN, a broadly writable bucket, a shared internal app host, the config's own apps.acme.dev example — allow-listing it re-opened exactly what #285 closed: publish a document there naming redirect_uri https://evil.example/cb, and an unauthenticated GET /oauth2/authorize 302s to evil.example. Worse than before the allow-list, in fact, because redirectToInteractiveLogin now walks a victim through the real login page first, so the code lands at the attacker after a genuine sign-in. redirectHostsAllowed closes it: an https redirect_uri must be on the client_id's own host or on the allow-list. Loopback and private-use schemes stay exempt — they deliver to the caller's own machine, which is the native and MCP client shape. In open mode domainAllowed admits everything, so this is a no-op there, correctly: open mode refuses those redirects outright. Also from review: - refusesRedirectTo refused to answer for a nil client by returning false. Folded the nil case in and dropped the duplicated check at both call sites, which is what "one predicate so they cannot drift" was supposed to mean. - CIMDConfig.AllowedDomains' godoc and zeroid.yaml still described the field as a fetch/SSRF lever with empty as a fine default. It now also decides whether a browser CIMD client can sign a user in at all, and both say so — along with the new obligation that listing a host asserts you vet who publishes there. - docs/cimd.md's "Errors are not redirected to a CIMD client" heading and a spec cross-reference pointing at §12.6 (Caching) instead of §12.7.
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.
Closes #279. Closes #276 — both halves.
/oauth2/authorizereturned an RFC 6749 §5.2 JSON body for every failure. §4.1.2.1 wants most redirected back to the client aserror+state, and exempts only the two cases where redirecting would itself be the vulnerability: an invalidclient_id, and a missing or unregisteredredirect_uri.JSON was defensible while the endpoint was POST-only and the caller was a CLI parsing the body. #270 mounted GET. A browser now renders a raw
{"error":"invalid_client"}while the client blocks on a callback that never fires, with noerrorand nostateto correlate.The blocker was ordering, not effort. The handler resolved the principal at step 4, but client lookup and
redirect_urivalidation lived insideIssueAuthCodeat step 6 — soaccess_denied, the failure a browser user hits by not being signed in, fired with no validatedredirect_urito send it to.1. Step 3.5 — resolve the client before the resolver chain
ResolveAuthorizeClientis split out ofIssueAuthCodeand called before principal resolution. It reusesresolveClientRegistryOrCIMDandredirectURIAllowedverbatim rather than duplicating the registry-first/CIMD policy or the RFC 8252 loopback matching.IssueAuthCodeRequest.Clientcarries the result so the lookup runs once. Nil preserves the original behaviour exactly, so programmatic callers are untouched.2. Failure disposition
Two axes now: where the failure happened, and who registered the client.
invalid_clientaccess_deniedunauthorized_clientunauthorized_clientserver_errorserver_errorclient_id; unregisteredredirect_uriaccess_deniedrather thaninvalid_clientbecause the client is fine — we resolved it at 3.5 — it's the resource owner who couldn't be authenticated. The old code blamed the wrong party.POST keeps JSON. Its caller is not a browser: it's a CLI, or a surface like Studio that authenticated the user itself and posts an RFC 7523 assertion. Those have parsed JSON since v1 and there's no user agent in the exchange to redirect.
3. The self-asserted carve-out — a deliberate §4.1.2.1 deviation
This is the most security-salient decision in the PR.
§4.1.2.1's redirect rule rests on "registered" meaning a party somebody vetted. CIMD removes registration by design:
redirect_uriscome from a document the requester published, CIMD is on by default, andallowed_domainsships empty. So for a CIMD client the destination is attacker-chosen, not merely attacker-supplied — and honouring the rule makes this endpoint an unauthenticated redirector, reachable with no credential (the failure being reported is "you have no credential"), with the AS's own origin as the first hop.Two paths refuse for such a client:
failAuthorizeanswers the §5.2 JSON body instead of a 302.redirectToInteractiveLogindeclines. That's the more damaging half — sending a victim through the deployment's real login page for an unvetted client means they authenticate for real and the flow resumes toward an attacker-publishedredirect_uri.The discriminator is provenance, not the CIMD feature.
domain.OAuthClient.SelfAsserted()readsregistration_source == "cimd". Registered and RFC 7591 dynamically-registered clients are unaffected. Settingcimd.allowed_domainsrestores redirects for CIMD too, because vetting which hosts may publish restores the assumption the rule is built on — but only on a single-tenant deployment:allowed_domainshas no tenant dimension, so on a multi-tenant AS it is all-or-nothing and CIMD clients effectively never get redirects. The tenant is not even known when CIMD resolves. Tracked in #286.The cost, stated plainly: a browser-driven CIMD client can't learn its error from the callback and must read the JSON body — the exact ergonomic problem #279 set out to fix, kept for the one client class whose redirect target nobody vetted. Documented in
docs/cimd.mdand spec §12.5 so it reads as deliberate.4. One policy gate, because three separate gates leaked
The pre-resolved-
Clientpath skipped gates, and it took three review passes to find them all —redirect_uri, then grant-type, then client state (IsActive/ClientType == "public", i.e. deactivation-as-kill-switch). Each time the fix was "re-add this one to the other branch", and each time it left the next behind.All three now live in
checkAuthorizeClientPolicy, which every path intoIssueAuthCodecalls. Running it twice on the resolve path costs a slice scan and a string compare; that's the price of the skip being impossible.Gate order is also a tested property:
redirect_uriis settled first, so a client failing both it and grant-type reports theredirect_urirefusal — the one §4.1.2.1 exempts. Checking grant-type first is what madeunauthorized_clientpermanently non-redirectable.5.
Server.Usechains (#276)fn→fns, appending, composed first-registered-outermost, with the composition cached and rebuilt on mutation sochain()doesn't allocate per request.AdminAuthkeeps replace semantics — admin auth is one decision, not a stack — pinned by a test.Every existing caller calls
Useexactly once, so no current behaviour changes. It fixes a live footgun: AuthN's slot is already occupied by the trusted-service annotator paired withSetTrustedServiceValidator, so a deployer adding a second concern would have dropped the annotator and broken external-principal exchange.Use(nil)is documented as ignored — an appending registry has no removal.6. Interactive-login hook (#276's other half)
ErrPrincipalInteractionRequired+Server.SetInteractiveLoginURL. A resolver says "this could be satisfied by logging in"; ZeroID redirects on its behalf, so resolvers still never touch the transport.return_tois rebuilt from the validated protocol parameters, never copied from the inbound URL — forwarding the raw query would hand the login surface, and its logs, whatever a caller appended. GET only. Godoc documents thatreturn_tois a reserved parameter name and that there is no loop guard, so a surface bouncing back without establishing a session will loop.7. Two more, fixed rather than deferred
client_nameis REQUIRED and non-empty. It's the string a consent screen shows, and a CIMD publisher is anonymous by construction, so the old fallback toclient_idlet whoever chose the URL choose what the user reads. A deviation from the draft, recorded in spec §12.4.SetTrustedServiceValidator(nil)actually disables. It wrapped its argument unconditionally, so nil became a non-nil closure — turning "disabled" into a nil-deref panic on the first external-principal exchange. Pre-existing.8. Smaller corrections
IssueAuthCode,code_challenge_method=plaincould send a user through the login surface and fail after they authenticated.error_descriptionclamped to §4.1.2.1's NQCHAR (printable ASCII, no quote or backslash). Harmless in a JSON body; these strings now ride in aLocationquery. Clamped at the boundary, since service-layer strings reach there too.resolvePrincipal's godoc still mapped everything to 401.Tests
client_idrulesTwo existing assertions were rewritten where the mechanism changed but the property didn't — notably
TestAuthorizeGET_CredentialInQueryStringIsIgnored, which asserted "Locationis empty" and now asserts nocodeis issued and that the credential doesn't appear in theLocation, which is a stronger check than it replaced.Thirteen mutation checks, each confirmed to fail:
= 400, want 503logSafePath→r.RequestURIzid_sk_valueAllowedDomainCount() = 1, want 0Useto replaceclient_namefallbackreturn_toLocationit would have emittedVerification
go build ./...,go vet ./...,gofmtcleango test ./...green across all 9 packages, integration suite includedgolangci-lintunder CI's exact invocation (-c .golangci.yml --new-from-merge-base=origin/main): 0 issuesMCP_INTEROP_ZEROID_REPLACE=1: 24 passed, 12 xfailed, 0 failed — identical to the baseline, so theclient_namerequirement and the carve-out break nothing in the chain that authn#155 and regression#192 were verified against. All 12 published CIMD documents that reach resolution carryclient_name; the 12 xfails are the pre-existing documented gaps.Not in scope
access_deniedfor a resolver outage. ThePrincipalResolvercontract defines any non-sentinel error as "credential found but invalid", so an outage and a bad credential are indistinguishable. Separating them needs a new sentinel — a contract change worth its own decision.client_nameinterop cost for third parties. A real MCP client whose document omits it will now fail. Deliberate; the harness is unaffected.🤖 Generated with Claude Code