feat(oauth)!: hardened OAuth client with confidential-client support #167

Merged
Grandiras merged 2 commits from issue-132-oauth into main 2026-09-26 15:49:47 +00:00
Owner

Closes #132.

Hardens the core OAuth client, adds confidential-client support, and picks up the items #120 and #124 deferred here. The Server hosting move, the DI redesign and the per-DID refresh coordinator stay with #136.

Transport and validation

  • One transport path. PAR, token exchange, refresh and revocation all go through PostFormWithDpopAsync. It builds the form again on each attempt, so a client assertion is never reused. It retries once on use_dpop_nonce (only that error, and only when a fresh nonce came back), reads responses up to 64 KiB, disposes every response, and reports error bodies as OAuthException. This replaces three copies. Before, PAR and token responses were never disposed, PAR retried on any 400 that carried a nonce, and a non-JSON error body escaped as JsonException.
  • SSRF on the token endpoints (#120). By default every request to a PDS or an authorization server follows the identity fetch policy: HTTPS, public addresses checked after DNS, no redirects. An endpoint that breaks the URL rules, including one read from a stored session, fails with invalid_server_url before anything is sent.
  • Process-wide DPoP nonce cache (#124). The internal DPoPNonceCache is keyed by origin and bounded (1,024 origins, nonces up to 1,024 chars). XrpcClient (so the per-request factory clients too), SpaceReader and OAuthClient all share it. A fresh client's first request carries a known nonce, and the token exchange reuses the nonce its PAR learned. The per-client _dpopNonce/keepDPoPNonce plumbing is gone.
  • Refresh validation (#124). Before the refresh token is spent, the DID is resolved again and its AS must still be the session's issuer. Otherwise the refresh fails with auth_server_mismatch and nothing is sent. The response must name the session DID (did_mismatch/missing_sub) and carry atproto (invalid_scope). The code exchange response is held to the same checks, now including the DPoP token type. A refresh also moves the session to the PDS the DID document names now, as the reference client's aud does.
  • AS metadata.
    • The issuer must be an exact, canonical match, port and path included (only scheme and host were compared before).
    • The authorization, token, PAR and revocation endpoints must be absolute https URLs with no query, fragment or userinfo (http only under AllowPrivateNetworks).
    • The three required booleans must be set.
    • The protected-resource resource must be the PDS, and authorization_servers[0] must be a canonical issuer.
    • The callback's iss is compared exactly.
  • Discovery.
    • A URL identifier or OAuthAuthorizationOptions.ServerUrl that serves no protected-resource metadata is tried as an issuer (upstream #4896). If that fails too, the original PDS error is reported.
    • Metadata documents are cached by URL for 5 minutes, as raw bytes, so callers get fresh copies.
    • A login started from a handle or DID stores the resolved PDS and verified handle in the pending state, so the callback fetches nothing again.
    • A login started from a URL resolves the DID at the callback, and the session's ServiceEndpoint becomes the DID document's PDS rather than the URL typed.

Confidential clients

  • OAuthClientKey (P-256 only) can be generated, imported from PKCS#8 and exported. It exposes PublicJwk and CreateKeySet for jwks.
  • With OAuthOptions.ClientKeys and token_endpoint_auth_method: private_key_jwt, PAR, token, refresh and revoke carry client_assertion_type plus an ES256 client_assertion. The header is alg and kid only, as in the reference client. The claims are iss = sub = client_id, aud = issuer, a 128-bit hex jti, iat, and exp = iat + 60.
  • The constructor refuses inconsistent setups: keys for a public client, no keys, an algorithm other than ES256, no jwks/jwks_uri, a key missing from the inline jwks, duplicate kids, or an unsupported method. Sign-in fails with unsupported_client_auth if the AS doesn't list private_key_jwt/ES256.
  • The key id is persisted per session as OAuthSession.ClientKeyId, so refresh and revoke use that key and key rotation works. The field is additive and nullable: stored sessions without it read as public-client sessions and are still sent as such (tested against the stored JSON). A session whose key is gone fails with client_key_unavailable, and nothing is sent.

State and scopes

  • IOAuthStateStore holds OAuthPendingAuthorization (state, requester, issuer, endpoints, PKCE verifier, DPoP key, client key id, resolved identity, expiry).
    • InMemoryOAuthStateStore is the default: 10 per requester, 10,000 in total, the oldest displaced rather than new ones refused, expired entries purged. It replaces the global cap of 100.
    • DistributedCacheOAuthStateStore keys entries by a SHA-256 of the state and sets the absolute expiry. It takes optional Protect/Unprotect (for example IDataProtector), and unreadable entries count as absent.
    • States longer than 256 chars are never looked up.
  • StartAuthorizationAsync returns OAuthAuthorizationRequest(Uri AuthorizationUrl, string State, DateTimeOffset ExpiresAt) and takes OAuthAuthorizationOptions { ServerUrl, RequesterId }.
  • Scopes.
    • IdentityAction and Identity(attr, action) are [Obsolete]; the new Identity(attr) replaces them.
    • Rpc/Include validate aud as * (rpc only) or did…#fragment, and a pre-encoded %23 is accepted.
    • Presets: BlueskyApp, BlueskyAppWithChat, BlueskyReadOnly, BlueskyPosting, plus the BlueskyAppView/BlueskyChat audiences. The presets are pinned against the builders, and the audiences against ServiceProxy.
    • transition:* is documented as legacy, but the default is unchanged.
    • The docs use /oauth-client-metadata.json.
  • Removed: GetPendingAuthorizationState and OAuthAuthorizationState, AtProtoClientOptions.OAuth, OAuthOptions.DefaultPdsUrl.
  • Blazor (minimal). AtProtoOAuthService passes the remote IP as RequesterId, takes an optional IOAuthStateStore from DI, creates its owned client under the fetch policy (still honouring HttpClientTimeout), and lets CompleteCallbackAsync build the client via TryGetClient(), so a callback after a restart completes.

Breaking changes (migration)

  • new OAuthClient(options, httpClient, logger) becomes new OAuthClient(options, logger). OAuthOptions.MetadataHttpClient is renamed HttpClient and now carries every AS request. Set it only for a client that is already safe, such as a test stub.
  • var (url, state) = await StartAuthorizationAsync(id, redirect, pdsUrl) becomes var (url, state, _) = await StartAuthorizationAsync(id, redirect, new OAuthAuthorizationOptions { ServerUrl = pdsUrl, RequesterId = ip }). Redirect to url.AbsoluteUri.
  • GetPendingAuthorizationState/OAuthAuthorizationState are removed; use OAuthOptions.StateStore.
  • For AtProtoClientOptions.OAuth and OAuthOptions.DefaultPdsUrl, delete the assignments.
  • AtProtoScopes.Rpc/Include throw on a bare-DID aud and on include with aud=*. Append the service fragment (AtProtoScopes.BlueskyAppView).
  • The AtProtoOAuthService constructor gains IOAuthStateStore? stateStore = null, so code that constructs it must be recompiled.
  • Behaviour: a URL-started login's ServiceEndpoint is the DID's PDS; metadata that breaks the profile is refused.

Verification

  • dotnet build (whole solution, including integration tests and samples): 0 warnings, 0 errors.
  • Unit tests: 4043 passed, 0 failed, 5 skipped (4048 total, +112 against main), stable across three consecutive runs.
  • New test classes:
    • OAuthTransportTests: nonce reuse PAR→token, retry rules, error mapping, oversized bodies, disposal of every response, URL rules, loopback refused without connecting, and the refresh checks.
    • AuthorizationServerMetadataValidationTests: issuer, endpoints, booleans, resource, cache, entryway.
    • ConfidentialClientTests: the RFC 7523 claim shape, with signatures verified independently via ECDsa against the published JWK. The key vector is RFC 7517 appendix A.2 (the same fixture indigo uses); its x/y must round-trip through PKCS#8 import. Also covers key rotation, the missing-key and public paths, and the constructor checks.
    • OAuthStateStoreTests: the in-memory limits and expiry, the flood regression (150 starts from one requester, then another requester still completes), state_expired, single use, and the distributed store (hashed keys, protector, cross-instance completion).
    • DPoPNonceCacheTests: a new transport's first request carries the known nonce; origin isolation; bounds.
    • AtProtoOAuthServiceStateTests: the Blazor requester id and flood handling.
  • OAuthCallbackHandleVerificationTests was reworked for the review round (see below): every callback, however the login started, now resolves the account afresh and pins that, rather than pinning identity reuse from the start.

(Superseded by the totals under Review fixes below, taken after the adversarial review and the rebase onto main.)

Review fixes

An adversarial review of the commit above found eight classes of issue. All eight are fixed, each with regression tests; nothing was deferred.

  1. Issuer verification now bypasses every cache, at both the code exchange and every refresh. IIdentityResolver.ResolveUncachedAsync(Did) (default interface member, IdentityResolver overrides it by invalidating the DID resolver's cache first) and AuthorizationServerDiscovery.ResolveAuthorizationServerAsync(pdsUrl, bypassCache: true, …) replace the old behaviour, where a handle/DID-started login reused the identity resolved at the start and a refresh's re-check could still read a cached AS document. VerifyIssuerAsync now runs unconditionally on every code exchange, not only when the login started from a server URL. Tokens that fail the check are revoked, best effort, before the exception.
  2. The authorization-server exchange is bounded end to end. OAuthOptions.RequestTimeout (30 s default) wraps send, retry and body read in one linked CancellationTokenSource.CancelAfter; HttpRequestException, a mid-body IOException/InvalidDataException, and the timeout's OperationCanceledException all map to OAuthException (request_timeout / request_failed), while the caller's own cancellation is detected (!cancellationToken.IsCancellationRequested) and left as OperationCanceledException.
  3. Pending-login limits. OAuthAuthorizationOptions.RequesterIdFor(IPAddress) groups an IPv6 requester by its /64 (the block one subscriber is usually given), on top of the existing per-requester (10) and global (10,000) bounds. Kept the original evict-oldest-rather-than-refuse design: a login only waits between redirect and callback, so the newest attempt is the one a real user is on, and refusing it would let a flood lock out genuine logins for the whole entry lifetime, where eviction only costs abandoned attempts. UseForwardedHeaders is documented in both docs/oauth.md and docs/blazor.md. Blazor's _loginContexts is gone entirely — folded into OAuthAuthorizationOptions.AppState (server-side, in the state store), which CompleteAuthorizationWithAppStateAsync hands back; nothing keyed by state lives in the Blazor service's own memory any more.
  4. Login CSRF. /login now sets an HttpOnly, SameSite=Lax cookie (__Host- prefixed, Secure, on HTTPS) via the new OAuthLoginBinding, and keeps only its SHA-256 with the pending authorization as part of AppState. /callback (and the cross-origin loopback relay) requires and deletes it, failing login_not_bound and revoking the tokens when it is missing or does not match. returnUrl moved out of any cookie into AppState, and is accepted only as OAuthLoginBinding.IsLocalUrl — a single leading /, never // or /\.
  5. DistributedCacheOAuthStateStore is secure by default. The options constructor parameter is no longer optional; the constructor throws unless Protect/Unprotect are both set or the explicit, loudly named StoreSecretsUnencrypted is. The non-atomic take (read then remove) is documented as accepted: the authorization server still only exchanges a code once, and a leaked state is useless without the browser's binding cookie.
  6. Metadata checks matching @atproto/oauth-client. Protected-resource metadata must now name exactly one authorization_servers entry (invalid_resource_metadata on two or more); an authorization server that publishes protected_resources (RFC 9728 §4) must list the PDS; and tokens the client refuses (wrong account, wrong scope, non-DPoP token_type, a failed fresh-issuer check) are revoked best effort via the new RevokeUnusableTokensAsync.
  7. Blazor's owned client has no proxy. This was already true of the SDK's identity fetch policy handler; it is now called out explicitly in docs/blazor.md (next to the HttpClient/HttpClientTimeout options) and in the CHANGELOG, since it is easy to assume an egress proxy is picked up automatically.
  8. New regression tests, on top of the classes above: OAuthRequestFailureTests (body-read timeout, a stalled connection, a body broken off mid-read, the caller's own cancellation staying a cancellation, a non-use_dpop_nonce error that carries a DPoP-Nonce header not being retried), AtProtoOAuthCallbackBindingTests (the binding cookie: same browser signs in, another browser is refused and revoked, cross-instance completion, a non-local returnUrl ignored), plus in OAuthTransportTests: Refresh_ATokenThatIsNotDPoPBound_IsRejected / SignIn_ATokenThatIsNotDPoPBound_IsRejectedAndRevoked (non-DPoP token_type on both flows), SignIn_StartedFromAServerUrl_ADidThatResolvesToAnotherAuthorizationServer_IsRejectedAndRevoked (a URL-started callback whose sub resolves to a different AS), and SignIn_TheMetadataCacheFromStartingTheLogin_IsBypassedAtTheCallback (issuer verification against a stale cache — asserts a second network fetch happens rather than trusting the entry cached at the start). AuthorizationServerMetadataValidationTests.MetadataWithoutAtprotoOrES256_IsRefused covers unsupported_scope/unsupported_dpop_alg, and Distributed_AnEntryForAnotherState_IsRefused covers the distributed store refusing a mismatched embedded state. No production check was weakened or removed to make a test pass.

docs/oauth.md and docs/blazor.md are updated throughout to match: the "State Parameter" section no longer calls the OAuth state CSRF protection on its own (it ties the callback to a pending authorization, not to a browser) and points at the Blazor cookie binding instead; docs/blazor.md gained a "Login CSRF protection" subsection, the RequestTimeout/no-proxy notes, and the UseForwardedHeaders guidance.

Verification (after the review round and rebasing onto main, which had gained #166 service-auth and #168 streaming in the meantime):

  • dotnet build (whole solution): 0 warnings, 0 errors.
  • Unit tests: 4387 passed, 0 failed, 5 skipped (4392 total).

Deferred / notes

  • The "unused nonce fields on the token data" were already removed with AtProtoTokenData in #124; nothing was left to remove.
  • Blazor has no option for ClientKeys yet, so confidential clients in the hosted integration belong with #136 (AtProtoOAuthOptions.ClientAuthentication). Mapping /oauth-client-metadata.json and /jwks.json also stays with #136.
  • client_key_unavailable does not expire the session: it is recoverable by restoring the key. The reference client deletes the session instead.
  • The distributed store cannot count per requester, so the docs recommend edge rate limiting.
  • Edits outside the OAuth area: Http/XrpcClient.cs and Auth/SessionManager.cs (nonce plumbing), Crypto/AtProtoCrypto.cs (an internal ExportParameters), AtProtoClient.cs (the removed option), Identity/IDidResolver.cs and Identity/IdentityResolver.cs (ResolveUncachedAsync, added for the review round's fresh-issuer checks). Parallel-owned files (ServiceAuthGenerator, Server/Spaces, Server/Xrpc, Streaming) are untouched. Two #120 CHANGELOG bullets were reworded for the HttpClient rename.

🤖 Generated with Claude Code

Closes #132. Hardens the core OAuth client, adds confidential-client support, and picks up the items #120 and #124 deferred here. The Server hosting move, the DI redesign and the per-DID refresh coordinator stay with #136. ## Transport and validation - **One transport path.** PAR, token exchange, refresh and revocation all go through `PostFormWithDpopAsync`. It builds the form again on each attempt, so a client assertion is never reused. It retries once on `use_dpop_nonce` (only that error, and only when a fresh nonce came back), reads responses up to 64 KiB, disposes every response, and reports error bodies as `OAuthException`. This replaces three copies. Before, PAR and token responses were never disposed, PAR retried on any 400 that carried a nonce, and a non-JSON error body escaped as `JsonException`. - **SSRF on the token endpoints (#120).** By default every request to a PDS or an authorization server follows the identity fetch policy: HTTPS, public addresses checked after DNS, no redirects. An endpoint that breaks the URL rules, including one read from a stored session, fails with `invalid_server_url` before anything is sent. - **Process-wide DPoP nonce cache (#124).** The internal `DPoPNonceCache` is keyed by origin and bounded (1,024 origins, nonces up to 1,024 chars). `XrpcClient` (so the per-request factory clients too), `SpaceReader` and `OAuthClient` all share it. A fresh client's first request carries a known nonce, and the token exchange reuses the nonce its PAR learned. The per-client `_dpopNonce`/`keepDPoPNonce` plumbing is gone. - **Refresh validation (#124).** Before the refresh token is spent, the DID is resolved again and its AS must still be the session's issuer. Otherwise the refresh fails with `auth_server_mismatch` and nothing is sent. The response must name the session DID (`did_mismatch`/`missing_sub`) and carry `atproto` (`invalid_scope`). The code exchange response is held to the same checks, now including the DPoP token type. A refresh also moves the session to the PDS the DID document names now, as the reference client's `aud` does. - **AS metadata.** - The issuer must be an exact, canonical match, port and path included (only scheme and host were compared before). - The authorization, token, PAR and revocation endpoints must be absolute `https` URLs with no query, fragment or userinfo (`http` only under `AllowPrivateNetworks`). - The three required booleans must be set. - The protected-resource `resource` must be the PDS, and `authorization_servers[0]` must be a canonical issuer. - The callback's `iss` is compared exactly. - **Discovery.** - A URL identifier or `OAuthAuthorizationOptions.ServerUrl` that serves no protected-resource metadata is tried as an issuer (upstream #4896). If that fails too, the original PDS error is reported. - Metadata documents are cached by URL for 5 minutes, as raw bytes, so callers get fresh copies. - A login started from a handle or DID stores the resolved PDS and verified handle in the pending state, so the callback fetches nothing again. - A login started from a URL resolves the DID at the callback, and the session's `ServiceEndpoint` becomes the DID document's PDS rather than the URL typed. ## Confidential clients - `OAuthClientKey` (P-256 only) can be generated, imported from PKCS#8 and exported. It exposes `PublicJwk` and `CreateKeySet` for `jwks`. - With `OAuthOptions.ClientKeys` and `token_endpoint_auth_method: private_key_jwt`, PAR, token, refresh and revoke carry `client_assertion_type` plus an ES256 `client_assertion`. The header is `alg` and `kid` only, as in the reference client. The claims are `iss` = `sub` = client_id, `aud` = issuer, a 128-bit hex `jti`, `iat`, and `exp` = `iat` + 60. - The constructor refuses inconsistent setups: keys for a public client, no keys, an algorithm other than ES256, no `jwks`/`jwks_uri`, a key missing from the inline `jwks`, duplicate kids, or an unsupported method. Sign-in fails with `unsupported_client_auth` if the AS doesn't list `private_key_jwt`/ES256. - The key id is persisted per session as `OAuthSession.ClientKeyId`, so refresh and revoke use that key and key rotation works. The field is additive and nullable: stored sessions without it read as public-client sessions and are still sent as such (tested against the stored JSON). A session whose key is gone fails with `client_key_unavailable`, and nothing is sent. ## State and scopes - **`IOAuthStateStore`** holds `OAuthPendingAuthorization` (state, requester, issuer, endpoints, PKCE verifier, DPoP key, client key id, resolved identity, expiry). - `InMemoryOAuthStateStore` is the default: 10 per requester, 10,000 in total, the oldest displaced rather than new ones refused, expired entries purged. It replaces the global cap of 100. - `DistributedCacheOAuthStateStore` keys entries by a SHA-256 of the state and sets the absolute expiry. It takes optional `Protect`/`Unprotect` (for example `IDataProtector`), and unreadable entries count as absent. - States longer than 256 chars are never looked up. - **`StartAuthorizationAsync` returns `OAuthAuthorizationRequest(Uri AuthorizationUrl, string State, DateTimeOffset ExpiresAt)`** and takes `OAuthAuthorizationOptions { ServerUrl, RequesterId }`. - **Scopes.** - `IdentityAction` and `Identity(attr, action)` are `[Obsolete]`; the new `Identity(attr)` replaces them. - `Rpc`/`Include` validate `aud` as `*` (rpc only) or `did…#fragment`, and a pre-encoded `%23` is accepted. - Presets: `BlueskyApp`, `BlueskyAppWithChat`, `BlueskyReadOnly`, `BlueskyPosting`, plus the `BlueskyAppView`/`BlueskyChat` audiences. The presets are pinned against the builders, and the audiences against `ServiceProxy`. - `transition:*` is documented as legacy, but the default is unchanged. - The docs use `/oauth-client-metadata.json`. - **Removed:** `GetPendingAuthorizationState` and `OAuthAuthorizationState`, `AtProtoClientOptions.OAuth`, `OAuthOptions.DefaultPdsUrl`. - **Blazor (minimal).** `AtProtoOAuthService` passes the remote IP as `RequesterId`, takes an optional `IOAuthStateStore` from DI, creates its owned client under the fetch policy (still honouring `HttpClientTimeout`), and lets `CompleteCallbackAsync` build the client via `TryGetClient()`, so a callback after a restart completes. ## Breaking changes (migration) - `new OAuthClient(options, httpClient, logger)` becomes `new OAuthClient(options, logger)`. `OAuthOptions.MetadataHttpClient` is renamed `HttpClient` and now carries every AS request. Set it only for a client that is already safe, such as a test stub. - `var (url, state) = await StartAuthorizationAsync(id, redirect, pdsUrl)` becomes `var (url, state, _) = await StartAuthorizationAsync(id, redirect, new OAuthAuthorizationOptions { ServerUrl = pdsUrl, RequesterId = ip })`. Redirect to `url.AbsoluteUri`. - `GetPendingAuthorizationState`/`OAuthAuthorizationState` are removed; use `OAuthOptions.StateStore`. - For `AtProtoClientOptions.OAuth` and `OAuthOptions.DefaultPdsUrl`, delete the assignments. - `AtProtoScopes.Rpc`/`Include` throw on a bare-DID aud and on `include` with `aud=*`. Append the service fragment (`AtProtoScopes.BlueskyAppView`). - The `AtProtoOAuthService` constructor gains `IOAuthStateStore? stateStore = null`, so code that constructs it must be recompiled. - Behaviour: a URL-started login's `ServiceEndpoint` is the DID's PDS; metadata that breaks the profile is refused. ## Verification - `dotnet build` (whole solution, including integration tests and samples): **0 warnings, 0 errors**. - Unit tests: **4043 passed, 0 failed, 5 skipped** (4048 total, +112 against main), stable across three consecutive runs. - New test classes: - `OAuthTransportTests`: nonce reuse PAR→token, retry rules, error mapping, oversized bodies, disposal of every response, URL rules, loopback refused without connecting, and the refresh checks. - `AuthorizationServerMetadataValidationTests`: issuer, endpoints, booleans, resource, cache, entryway. - `ConfidentialClientTests`: the RFC 7523 claim shape, with signatures verified independently via `ECDsa` against the published JWK. The key vector is **RFC 7517 appendix A.2** (the same fixture indigo uses); its x/y must round-trip through PKCS#8 import. Also covers key rotation, the missing-key and public paths, and the constructor checks. - `OAuthStateStoreTests`: the in-memory limits and expiry, the flood regression (150 starts from one requester, then another requester still completes), `state_expired`, single use, and the distributed store (hashed keys, protector, cross-instance completion). - `DPoPNonceCacheTests`: a new transport's first request carries the known nonce; origin isolation; bounds. - `AtProtoOAuthServiceStateTests`: the Blazor requester id and flood handling. - `OAuthCallbackHandleVerificationTests` was reworked for the review round (see below): every callback, however the login started, now resolves the account afresh and pins that, rather than pinning identity reuse from the start. (Superseded by the totals under **Review fixes** below, taken after the adversarial review and the rebase onto main.) ## Review fixes An adversarial review of the commit above found eight classes of issue. All eight are fixed, each with regression tests; nothing was deferred. 1. **Issuer verification now bypasses every cache, at both the code exchange and every refresh.** `IIdentityResolver.ResolveUncachedAsync(Did)` (default interface member, `IdentityResolver` overrides it by invalidating the DID resolver's cache first) and `AuthorizationServerDiscovery.ResolveAuthorizationServerAsync(pdsUrl, bypassCache: true, …)` replace the old behaviour, where a handle/DID-started login reused the identity resolved at the start and a refresh's re-check could still read a cached AS document. `VerifyIssuerAsync` now runs unconditionally on every code exchange, not only when the login started from a server URL. Tokens that fail the check are revoked, best effort, before the exception. 2. **The authorization-server exchange is bounded end to end.** `OAuthOptions.RequestTimeout` (30 s default) wraps send, retry and body read in one linked `CancellationTokenSource.CancelAfter`; `HttpRequestException`, a mid-body `IOException`/`InvalidDataException`, and the timeout's `OperationCanceledException` all map to `OAuthException` (`request_timeout` / `request_failed`), while the caller's own cancellation is detected (`!cancellationToken.IsCancellationRequested`) and left as `OperationCanceledException`. 3. **Pending-login limits.** `OAuthAuthorizationOptions.RequesterIdFor(IPAddress)` groups an IPv6 requester by its /64 (the block one subscriber is usually given), on top of the existing per-requester (10) and global (10,000) bounds. Kept the original evict-oldest-rather-than-refuse design: a login only waits between redirect and callback, so the newest attempt is the one a real user is on, and refusing it would let a flood lock out genuine logins for the whole entry lifetime, where eviction only costs abandoned attempts. `UseForwardedHeaders` is documented in both `docs/oauth.md` and `docs/blazor.md`. Blazor's `_loginContexts` is gone entirely — folded into `OAuthAuthorizationOptions.AppState` (server-side, in the state store), which `CompleteAuthorizationWithAppStateAsync` hands back; nothing keyed by state lives in the Blazor service's own memory any more. 4. **Login CSRF.** `/login` now sets an HttpOnly, `SameSite=Lax` cookie (`__Host-` prefixed, `Secure`, on HTTPS) via the new `OAuthLoginBinding`, and keeps only its SHA-256 with the pending authorization as part of `AppState`. `/callback` (and the cross-origin loopback relay) requires and deletes it, failing `login_not_bound` and revoking the tokens when it is missing or does not match. `returnUrl` moved out of any cookie into `AppState`, and is accepted only as `OAuthLoginBinding.IsLocalUrl` — a single leading `/`, never `//` or `/\`. 5. **`DistributedCacheOAuthStateStore` is secure by default.** The `options` constructor parameter is no longer optional; the constructor throws unless `Protect`/`Unprotect` are both set or the explicit, loudly named `StoreSecretsUnencrypted` is. The non-atomic take (read then remove) is documented as accepted: the authorization server still only exchanges a code once, and a leaked state is useless without the browser's binding cookie. 6. **Metadata checks matching `@atproto/oauth-client`.** Protected-resource metadata must now name exactly one `authorization_servers` entry (`invalid_resource_metadata` on two or more); an authorization server that publishes `protected_resources` (RFC 9728 §4) must list the PDS; and tokens the client refuses (wrong account, wrong scope, non-DPoP `token_type`, a failed fresh-issuer check) are revoked best effort via the new `RevokeUnusableTokensAsync`. 7. **Blazor's owned client has no proxy.** This was already true of the SDK's identity fetch policy handler; it is now called out explicitly in `docs/blazor.md` (next to the `HttpClient`/`HttpClientTimeout` options) and in the CHANGELOG, since it is easy to assume an egress proxy is picked up automatically. 8. **New regression tests**, on top of the classes above: `OAuthRequestFailureTests` (body-read timeout, a stalled connection, a body broken off mid-read, the caller's own cancellation staying a cancellation, a non-`use_dpop_nonce` error that carries a `DPoP-Nonce` header not being retried), `AtProtoOAuthCallbackBindingTests` (the binding cookie: same browser signs in, another browser is refused and revoked, cross-instance completion, a non-local `returnUrl` ignored), plus in `OAuthTransportTests`: `Refresh_ATokenThatIsNotDPoPBound_IsRejected` / `SignIn_ATokenThatIsNotDPoPBound_IsRejectedAndRevoked` (non-DPoP `token_type` on both flows), `SignIn_StartedFromAServerUrl_ADidThatResolvesToAnotherAuthorizationServer_IsRejectedAndRevoked` (a URL-started callback whose `sub` resolves to a different AS), and `SignIn_TheMetadataCacheFromStartingTheLogin_IsBypassedAtTheCallback` (issuer verification against a stale cache — asserts a second network fetch happens rather than trusting the entry cached at the start). `AuthorizationServerMetadataValidationTests.MetadataWithoutAtprotoOrES256_IsRefused` covers `unsupported_scope`/`unsupported_dpop_alg`, and `Distributed_AnEntryForAnotherState_IsRefused` covers the distributed store refusing a mismatched embedded state. No production check was weakened or removed to make a test pass. `docs/oauth.md` and `docs/blazor.md` are updated throughout to match: the "State Parameter" section no longer calls the OAuth `state` CSRF protection on its own (it ties the callback to a pending authorization, not to a browser) and points at the Blazor cookie binding instead; `docs/blazor.md` gained a "Login CSRF protection" subsection, the `RequestTimeout`/no-proxy notes, and the `UseForwardedHeaders` guidance. **Verification (after the review round and rebasing onto main, which had gained #166 service-auth and #168 streaming in the meantime):** - `dotnet build` (whole solution): **0 warnings, 0 errors**. - Unit tests: **4387 passed, 0 failed, 5 skipped (4392 total)**. ## Deferred / notes - The "unused nonce fields on the token data" were already removed with `AtProtoTokenData` in #124; nothing was left to remove. - Blazor has no option for `ClientKeys` yet, so confidential clients in the hosted integration belong with #136 (`AtProtoOAuthOptions.ClientAuthentication`). Mapping `/oauth-client-metadata.json` and `/jwks.json` also stays with #136. - `client_key_unavailable` does not expire the session: it is recoverable by restoring the key. The reference client deletes the session instead. - The distributed store cannot count per requester, so the docs recommend edge rate limiting. - Edits outside the OAuth area: `Http/XrpcClient.cs` and `Auth/SessionManager.cs` (nonce plumbing), `Crypto/AtProtoCrypto.cs` (an internal `ExportParameters`), `AtProtoClient.cs` (the removed option), `Identity/IDidResolver.cs` and `Identity/IdentityResolver.cs` (`ResolveUncachedAsync`, added for the review round's fresh-issuer checks). Parallel-owned files (`ServiceAuthGenerator`, `Server/Spaces`, `Server/Xrpc`, `Streaming`) are untouched. Two #120 CHANGELOG bullets were reworded for the `HttpClient` rename. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(oauth)!: hardened OAuth client with confidential-client support (closes #132)
All checks were successful
CI / pds-integration (pull_request) Successful in 26s
CI / build-and-test (pull_request) Successful in 48s
19370abbf3
Every request to an authorization server (PAR, token, refresh, revocation) now
takes one path: a DPoP proof with the origin's latest nonce from a process-wide
cache, one retry on use_dpop_nonce, capped and disposed responses, errors as
OAuthException, and the identity fetch policy for the endpoints, which come
from metadata whoever controls a DID document chooses.

Refresh responses are validated (sub, atproto scope) and the DID's
authorization server is re-checked before a refresh token is spent.
Authorization server metadata is held to the AT Protocol profile, and the
protected-resource `resource` must name the PDS. Sign-in can start at an
entryway, discovery metadata is cached, and a handle/DID login reuses the
identity resolved at the start.

Confidential clients sign an ES256 client assertion (RFC 7523) with a key the
session remembers. Pending logins move to an IOAuthStateStore, in memory with
per-requester limits by default or in an IDistributedCache, which replaces the
global cap of 100 that anyone could fill. Scopes gain presets built on
permission sets and aud validation; IdentityAction is obsolete. The dead
GetPendingAuthorizationState/OAuthAuthorizationState, AtProtoClientOptions.OAuth
and OAuthOptions.DefaultPdsUrl are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-2026-09-26T01-09-19-810Z.md) — updating every ~5 minutes while the review runs.
Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T06:09:20.264Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T06:09:20.264Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f24bd8745ffeEw5wCvwdUqXFEb -->
Collaborator

🤖 Resolving the merge conflicts — the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the fix runs.

🤖 Resolving the merge conflicts — the reply will link the full session transcript. <!-- opencode-run:conflict --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-2026-09-26T01-26-17-313Z.md) — updating every ~5 minutes while the fix runs.
Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T06:26:17.710Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T06:26:17.710Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f24ae004effeDMuH81ZdodORLO -->
Collaborator

🤖 Resolving the merge conflicts — the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the fix runs.

🤖 Resolving the merge conflicts — the reply will link the full session transcript. <!-- opencode-run:conflict --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-2026-09-26T06-26-47-736Z.md) — updating every ~5 minutes while the fix runs.
Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T11:26:48.152Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T11:26:48.152Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f239ae0e6ffenCeAAzSQf3xC1Q -->
Collaborator

🤖 Resolving the merge conflicts — the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the fix runs.

🤖 Resolving the merge conflicts — the reply will link the full session transcript. <!-- opencode-run:conflict --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-2026-09-26T11-27-18-165Z.md) — updating every ~5 minutes while the fix runs.
Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T16:27:18.589Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T16:27:18.589Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f2287c241ffen7slRDxiabpfbn -->
Author
Owner

@claude /review — security-critical and large; the description has the migration notes and a "Review fixes" section covering an independent adversarial review. The diff will be truncated, so please read these in full from the branch: src/ATProtoNet/Auth/OAuth/OAuthClient.cs (the shared request helper and its DPoP-nonce retry, the bounded send+read, issuer verification with fresh resolution on every exchange and refresh, refresh-response validation, best-effort revocation), AuthorizationServerDiscovery.cs (metadata validation: one AS, protected_resources, issuer), the DPoP nonce cache, OAuthClientKey and the client assertion, OAuthStateStore.cs (in-memory per-requester and global bounds with IPv6 /64 grouping, the distributed store secure by default), and the Blazor AtProtoOAuthService.cs / OAuthLoginBinding (CSRF cookie binding, local-only returnUrl carried in AppState). Compare with @atproto/oauth-client. Focus on account and session confusion, state fixation and replay, SSRF, nonce leakage across origins, and secrets in logs or exceptions.

@claude /review — security-critical and large; the description has the migration notes and a "Review fixes" section covering an independent adversarial review. The diff will be truncated, so please read these in full from the branch: `src/ATProtoNet/Auth/OAuth/OAuthClient.cs` (the shared request helper and its DPoP-nonce retry, the bounded send+read, issuer verification with fresh resolution on every exchange and refresh, refresh-response validation, best-effort revocation), `AuthorizationServerDiscovery.cs` (metadata validation: one AS, protected_resources, issuer), the DPoP nonce cache, `OAuthClientKey` and the client assertion, `OAuthStateStore.cs` (in-memory per-requester and global bounds with IPv6 /64 grouping, the distributed store secure by default), and the Blazor `AtProtoOAuthService.cs` / `OAuthLoginBinding` (CSRF cookie binding, local-only returnUrl carried in AppState). Compare with `@atproto/oauth-client`. Focus on account and session confusion, state fixation and replay, SSRF, nonce leakage across origins, and secrets in logs or exceptions.
Grandiras force-pushed issue-132-oauth from 19370abbf3
All checks were successful
CI / pds-integration (pull_request) Successful in 26s
CI / build-and-test (pull_request) Successful in 48s
to 1a7b636256
All checks were successful
CI / pds-integration (pull_request) Successful in 27s
CI / build-and-test (pull_request) Successful in 56s
Sync Closures to GitHub / sync-closure (pull_request) Successful in 8s
2026-09-26 15:45:55 +00:00
Compare
Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/614); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-614.md) — updating every ~5 minutes while the task runs.
Collaborator

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-2026-09-26T15-46-56-524Z.md) — updating every ~5 minutes while the review runs.
Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T20:46:56.945Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T20:46:56.945Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f219a0c12ffeiow5IwW9Ie3xjQ -->
claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

This is a large, carefully-built PR, and it holds up well under inspection. I read the full OAuthClient.cs, AuthorizationServerDiscovery.cs, OAuthStateStore.cs, OAuthClientKey.cs and AtProtoScopes.cs from the branch (the diff itself was truncated) to check the parts the PR description claims were hardened, since those are exactly the places a subtle bug would hide:

  • Transport (PostFormWithDpopAsync/SendFormWithDpopAsync): single retry gated correctly on attempt == 1 && freshNonce is not null && error?.Error == "use_dpop_nonce"; every response is inside a using so it's disposed each loop iteration; the request budget (RequestTimeout) wraps send + retry + body read via a linked CancellationTokenSource, and the catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) correctly distinguishes the internal budget from caller cancellation.
  • Issuer re-verification (VerifyIssuerAsync): used identically at code-exchange and at refresh, resolves via ResolveIdentityUncachedAsync and bypassCache: true, so a stale cache genuinely can't confirm a moved account. identity.PdsEndpoint! in CompleteAuthorizationWithAppStateAsync is safe — VerifyIssuerAsync throws pds_not_found before returning if the PDS is missing, so the null-forgiving operator is justified.
  • Confidential clients (OAuthClientKey): P-256 enforced, private scalar zeroed after use in the constructor, RFC 7523 claims match the description exactly (iss=sub=client_id, aud=issuer, fresh jti, 60s exp), header is alg+kid only. ValidateClientAuthentication correctly rejects every inconsistent combination described in the PR text (keys with none, no keys with private_key_jwt, wrong alg, missing jwks/jwks_uri, unpublished key, duplicate kids).
  • State stores: InMemoryOAuthStateStore's per-requester + global eviction under one Lock is correct (checked the linked-list bookkeeping in Remove); DistributedCacheOAuthStateStore correctly refuses unencrypted storage unless explicitly opted out, and re-checks the decrypted entry's State against the lookup key to guard against a hash collision or misplaced entry.
  • OAuthAuthorizationOptions.RequesterIdFor: the /64 IPv6 grouping (bytes[8..].Clear()) zeroes the correct half of the address.
  • Scope audience validation (AtProtoScopes.ValidateAudience) and the metadata canonicalization helpers (IsCanonicalIssuer, ResourceMatches, IsEndpoint) in AuthorizationServerDiscovery.cs all match the documented AT Protocol profile checks (exact issuer match including port/path, no query/fragment/userinfo, RFC 9728 §3.3/§4 resource checks).
  • Blazor login-CSRF binding (OAuthLoginBinding, the relay/redeem flow in AtProtoOAuthService): the binding cookie is verified with a constant-time comparison, cleared on success, and sessions are revoked before the login_not_bound exception on both the direct-callback and relay-redeem paths; the cross-origin relay is now correctly restricted to loopback↔loopback (narrowing the previous, broader behavior), and IsIssuedRelayUrl closes the open-redirect gap left by dropping the old IsValidReturnUrl/absolute-URL trust.

Two very minor, non-blocking observations, not worth blocking on:

  • ResolveFromServerUrlAsync's success path re-invokes ResolveAuthorizationServerAsync, redoing the resource-metadata fetch already done in the try block; harmless because it hits the 5-minute metadata cache, but it's a wasted re-parse.
  • A relay entry (AtProtoOAuthService's cross-origin loopback relay) that is never redeemed still isn't revoked when it expires — pre-existing behavior, not a regression from this PR.

I did not find correctness or security bugs in the sampled code, and the breaking-change/migration notes in the CHANGELOG and docs match the code they describe. Given the scope, I focused verification on the security-critical paths (transport, issuer/DID checks, confidential-client crypto, state stores, CSRF binding) rather than every doc/UI wording change, which appear consistent with the code on a spot check.


⏱ 2m 54s · 22 turns · tokens: 123.6k in / 13.8k out (+2536.9k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, carefully-built PR, and it holds up well under inspection. I read the full `OAuthClient.cs`, `AuthorizationServerDiscovery.cs`, `OAuthStateStore.cs`, `OAuthClientKey.cs` and `AtProtoScopes.cs` from the branch (the diff itself was truncated) to check the parts the PR description claims were hardened, since those are exactly the places a subtle bug would hide: - **Transport (`PostFormWithDpopAsync`/`SendFormWithDpopAsync`)**: single retry gated correctly on `attempt == 1 && freshNonce is not null && error?.Error == "use_dpop_nonce"`; every response is inside a `using` so it's disposed each loop iteration; the request budget (`RequestTimeout`) wraps send + retry + body read via a linked `CancellationTokenSource`, and the `catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested)` correctly distinguishes the internal budget from caller cancellation. - **Issuer re-verification (`VerifyIssuerAsync`)**: used identically at code-exchange and at refresh, resolves via `ResolveIdentityUncachedAsync` and `bypassCache: true`, so a stale cache genuinely can't confirm a moved account. `identity.PdsEndpoint!` in `CompleteAuthorizationWithAppStateAsync` is safe — `VerifyIssuerAsync` throws `pds_not_found` before returning if the PDS is missing, so the null-forgiving operator is justified. - **Confidential clients (`OAuthClientKey`)**: P-256 enforced, private scalar zeroed after use in the constructor, RFC 7523 claims match the description exactly (`iss`=`sub`=client_id, `aud`=issuer, fresh `jti`, 60s `exp`), header is `alg`+`kid` only. `ValidateClientAuthentication` correctly rejects every inconsistent combination described in the PR text (keys with `none`, no keys with `private_key_jwt`, wrong alg, missing jwks/jwks_uri, unpublished key, duplicate kids). - **State stores**: `InMemoryOAuthStateStore`'s per-requester + global eviction under one `Lock` is correct (checked the linked-list bookkeeping in `Remove`); `DistributedCacheOAuthStateStore` correctly refuses unencrypted storage unless explicitly opted out, and re-checks the decrypted entry's `State` against the lookup key to guard against a hash collision or misplaced entry. - **`OAuthAuthorizationOptions.RequesterIdFor`**: the /64 IPv6 grouping (`bytes[8..].Clear()`) zeroes the correct half of the address. - **Scope audience validation** (`AtProtoScopes.ValidateAudience`) and the metadata canonicalization helpers (`IsCanonicalIssuer`, `ResourceMatches`, `IsEndpoint`) in `AuthorizationServerDiscovery.cs` all match the documented AT Protocol profile checks (exact issuer match including port/path, no query/fragment/userinfo, RFC 9728 §3.3/§4 resource checks). - **Blazor login-CSRF binding** (`OAuthLoginBinding`, the relay/redeem flow in `AtProtoOAuthService`): the binding cookie is verified with a constant-time comparison, cleared on success, and sessions are revoked before the `login_not_bound` exception on both the direct-callback and relay-redeem paths; the cross-origin relay is now correctly restricted to loopback↔loopback (narrowing the previous, broader behavior), and `IsIssuedRelayUrl` closes the open-redirect gap left by dropping the old `IsValidReturnUrl`/absolute-URL trust. Two very minor, non-blocking observations, not worth blocking on: - `ResolveFromServerUrlAsync`'s success path re-invokes `ResolveAuthorizationServerAsync`, redoing the resource-metadata fetch already done in the `try` block; harmless because it hits the 5-minute metadata cache, but it's a wasted re-parse. - A relay entry (`AtProtoOAuthService`'s cross-origin loopback relay) that is never redeemed still isn't revoked when it expires — pre-existing behavior, not a regression from this PR. I did not find correctness or security bugs in the sampled code, and the breaking-change/migration notes in the CHANGELOG and docs match the code they describe. Given the scope, I focused verification on the security-critical paths (transport, issuer/DID checks, confidential-client crypto, state stores, CSRF binding) rather than every doc/UI wording change, which appear consistent with the code on a spot check. --- ⏱ 2m 54s · 22 turns · tokens: 123.6k in / 13.8k out (+2536.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-167/run-614.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:1a7b636256b5c77c7d28765146147e276b11ee8c -->
Grandiras deleted branch issue-132-oauth 2026-09-26 15:49:47 +00:00
Sign in to join this conversation.
No description provided.