feat: request spec-aligned scopes, retry without resource indicators, and validate iss on remote login - #6109
Conversation
🦋 Changeset detectedLatest commit: 9ee890f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Running ultrareview automatically — Running ultrareview automatically — This change rewrites core remote-login OAuth behavior (scope resolution, RFC 8707 resource handling with retry, RFC 9207 iss validation) across the callback and issuer API, so a subtle bug could break auth or route grants.. I'll post findings when complete. |
d887aa1 to
64677dc
Compare
e1d8f0b to
273bc07
Compare
There was a problem hiding this comment.
Ultrareview completed in 25m 14s
Review completed against the latest diff
Linked issue analysis
Linked issue: AIM-202: Spec-aligned per-issuer scope and resource-indicator strategy for upstream authorization
| Status | Acceptance criteria | Notes |
|---|---|---|
| Resolve upstream scopes from the configured override or client/issuer scope source, and append advertised openid, email, profile, and offline_access scopes. | The PR documents and tests the override/base precedence and advertised standard-scope appending, but the provided evidence does not clearly demonstrate the issue's required merge semantics between ClientScope and issuer scopes_supported. | |
| ✅ | When an issuer rejects RFC 8707 resource with invalid_target, learn that capability, retry once without resource, and retain the resource on the resulting session. | The PR describes gating resource on both authorization and exchange, parsing invalid_target, persisting the issuer flag, retrying exactly once, and retaining the resource; an end-to-end test covers the strategy. |
| ✅ | Validate the RFC 9207 iss callback parameter when the issuer advertises authorization_response_iss_parameter_supported. | The callback compares the returned iss with the expected issuer and rejects missing or mismatched values; the strategy end-to-end test covers this behavior. |
| ✅ | Show a reconnect hint when a live grant lacks openid but reconnecting would request it. | Scopes are plumbed into session state and card rendering, the template adds the requested copy and reconnect action, and dedicated tests cover the behavior. |
| ✅ | Expose scope_override and resource_indicator_supported as issuer configuration across supported API forms and models. | The PR adds the fields to the issuer design, generated API schemas and models, create/update payloads, persistence, and view mapping. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
64677dc to
2d19fa6
Compare
273bc07 to
4f70ebc
Compare
2d19fa6 to
8f0eace
Compare
4f70ebc to
1980e25
Compare
8f0eace to
346a35d
Compare
1980e25 to
11ff909
Compare
346a35d to
50e14fc
Compare
11ff909 to
5c01fac
Compare
50e14fc to
c96f6e6
Compare
0ab74e3 to
d01a1a5
Compare
c96f6e6 to
0be7d0e
Compare
5dc471c to
85cc31c
Compare
This comment has been minimized.
This comment has been minimized.
|
Testing summary: Reviews. Three independent passes (security, behavioural correctness against AIM-202 and the RFCs, data and API surface). One blocker found and closed: the resource-indicator flag is now learned only from a token-endpoint Unit and end to end (remotesessions, mcp): scope precedence rows including NULL and empty Unchanged-behaviour suite (24 cases): exact authorize URL key set and Browser, local stack with dev-idp as the upstream issuer: baseline connect widened Suites. remotesessions and mcp: 1,430 tests. Full server suite: 15,455 (two contention flakes outside these packages, pass in isolation). Dashboard unit suite: 3,230. Server lint clean. |
85cc31c to
fb0ea22
Compare
0be7d0e to
3d8af5d
Compare
fb0ea22 to
c57e88d
Compare
This comment has been minimized.
This comment has been minimized.
3d8af5d to
cb4e77f
Compare
c57e88d to
3167847
Compare
…ssion_issuers Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cb4e77f to
665405e
Compare
…and validate iss on remote login Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3167847 to
e116c30
Compare
danielkov
left a comment
There was a problem hiding this comment.
Couple Astra findings. I didn't see anything else.
…e_override empty slices on the wire Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
AIM-202. Stacked on #6107.
Summary
Makes the upstream OAuth request adapt to each issuer.
Client.RequestedScopes:scope_overrideverbatim if set; else the client's stored scope, falling back to the issuer'sscopes_supported; thenopenid,email,profile,offline_accessappended when the issuer advertises them. Widening logs at debug; an empty override means none. The requested set is recorded on the session when the token response omitsscope(RFC 6749 §5.1).resourceis sent on authorize, exchange, and refresh unlessresource_indicator_supportedis false.resource_indicator_supportedis purely an operator override; Gram never writes it from a login. Oninvalid_targetthe callback retries that login once withoutresource, and the refresh grant gets the same one-shot retry inside its single-flight lease; any other error is final and nothing is generalised to the issuer or to other grants, since a rejection can be about one resource rather than RFC 8707 support. The resource stays on the session for routing.authorization_response_iss_parameter_supported, the callback rejects a response whoseissis missing or different, before the denial branch and the exchange. Compared to the discovery document's issuer, or the stored URL without its trailing slash when no document exists.codenorerrorleaves the login state alone. Denials echo only IETF-registered error codes. Upstream and browser text is capped before logging. Token-endpoint error bodies are parsed withoautherr.ParseTokenError.openidwhile a reconnect would request it; the first-party page no longer auto-closes over such a card.challenge_unchanged_regression_test.goand its mcp sibling pin the exact authorize URL, exchange and refresh bodies, callback handling without the iss capability, verbatim response-scope storage, pre-PR login states, and first-party auto-close for issuers the new rules do not touch. Two changes apply to every issuer and are pinned as such: a denial consumes the login state, and a token response withoutscoperecords the requested set.scope_overridewhen omitted ornulland clears it on[]; the form field serializes withoutomitemptyat every tier so generated clients can send[], and the OpenAPI marks it nullable. SDK regenerated. Dashboard exposure deferred; an omitted field keeps the stored value, so dashboard saves cannot clobber them.Motivation
Linear never returned an ID token because
openidwas never requested. Vercel sessions died after an hour becauseoffline_accesswas never requested. WorkOS never connected because it rejectsresource. The callback never checkediss. Scope and iss are per-issuer rules, so those knobs live on the issuer row. Resource-indicator support is learned per grant at runtime instead, with one retry so a misbehaving provider cannot loop a user.🤖 Generated with Claude Code