feat(oauth)!: hardened OAuth client with confidential-client support #167
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!167
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-132-oauth"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
PostFormWithDpopAsync. It builds the form again on each attempt, so a client assertion is never reused. It retries once onuse_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 asOAuthException. 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 asJsonException.invalid_server_urlbefore anything is sent.DPoPNonceCacheis keyed by origin and bounded (1,024 origins, nonces up to 1,024 chars).XrpcClient(so the per-request factory clients too),SpaceReaderandOAuthClientall 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/keepDPoPNonceplumbing is gone.auth_server_mismatchand nothing is sent. The response must name the session DID (did_mismatch/missing_sub) and carryatproto(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'sauddoes.httpsURLs with no query, fragment or userinfo (httponly underAllowPrivateNetworks).resourcemust be the PDS, andauthorization_servers[0]must be a canonical issuer.issis compared exactly.OAuthAuthorizationOptions.ServerUrlthat serves no protected-resource metadata is tried as an issuer (upstream #4896). If that fails too, the original PDS error is reported.ServiceEndpointbecomes 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 exposesPublicJwkandCreateKeySetforjwks.OAuthOptions.ClientKeysandtoken_endpoint_auth_method: private_key_jwt, PAR, token, refresh and revoke carryclient_assertion_typeplus an ES256client_assertion. The header isalgandkidonly, as in the reference client. The claims areiss=sub= client_id,aud= issuer, a 128-bit hexjti,iat, andexp=iat+ 60.jwks/jwks_uri, a key missing from the inlinejwks, duplicate kids, or an unsupported method. Sign-in fails withunsupported_client_authif the AS doesn't listprivate_key_jwt/ES256.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 withclient_key_unavailable, and nothing is sent.State and scopes
IOAuthStateStoreholdsOAuthPendingAuthorization(state, requester, issuer, endpoints, PKCE verifier, DPoP key, client key id, resolved identity, expiry).InMemoryOAuthStateStoreis 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.DistributedCacheOAuthStateStorekeys entries by a SHA-256 of the state and sets the absolute expiry. It takes optionalProtect/Unprotect(for exampleIDataProtector), and unreadable entries count as absent.StartAuthorizationAsyncreturnsOAuthAuthorizationRequest(Uri AuthorizationUrl, string State, DateTimeOffset ExpiresAt)and takesOAuthAuthorizationOptions { ServerUrl, RequesterId }.IdentityActionandIdentity(attr, action)are[Obsolete]; the newIdentity(attr)replaces them.Rpc/Includevalidateaudas*(rpc only) ordid…#fragment, and a pre-encoded%23is accepted.BlueskyApp,BlueskyAppWithChat,BlueskyReadOnly,BlueskyPosting, plus theBlueskyAppView/BlueskyChataudiences. The presets are pinned against the builders, and the audiences againstServiceProxy.transition:*is documented as legacy, but the default is unchanged./oauth-client-metadata.json.GetPendingAuthorizationStateandOAuthAuthorizationState,AtProtoClientOptions.OAuth,OAuthOptions.DefaultPdsUrl.AtProtoOAuthServicepasses the remote IP asRequesterId, takes an optionalIOAuthStateStorefrom DI, creates its owned client under the fetch policy (still honouringHttpClientTimeout), and letsCompleteCallbackAsyncbuild the client viaTryGetClient(), so a callback after a restart completes.Breaking changes (migration)
new OAuthClient(options, httpClient, logger)becomesnew OAuthClient(options, logger).OAuthOptions.MetadataHttpClientis renamedHttpClientand 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)becomesvar (url, state, _) = await StartAuthorizationAsync(id, redirect, new OAuthAuthorizationOptions { ServerUrl = pdsUrl, RequesterId = ip }). Redirect tourl.AbsoluteUri.GetPendingAuthorizationState/OAuthAuthorizationStateare removed; useOAuthOptions.StateStore.AtProtoClientOptions.OAuthandOAuthOptions.DefaultPdsUrl, delete the assignments.AtProtoScopes.Rpc/Includethrow on a bare-DID aud and onincludewithaud=*. Append the service fragment (AtProtoScopes.BlueskyAppView).AtProtoOAuthServiceconstructor gainsIOAuthStateStore? stateStore = null, so code that constructs it must be recompiled.ServiceEndpointis 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.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 viaECDsaagainst 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.OAuthCallbackHandleVerificationTestswas 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.
IIdentityResolver.ResolveUncachedAsync(Did)(default interface member,IdentityResolveroverrides it by invalidating the DID resolver's cache first) andAuthorizationServerDiscovery.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.VerifyIssuerAsyncnow 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.OAuthOptions.RequestTimeout(30 s default) wraps send, retry and body read in one linkedCancellationTokenSource.CancelAfter;HttpRequestException, a mid-bodyIOException/InvalidDataException, and the timeout'sOperationCanceledExceptionall map toOAuthException(request_timeout/request_failed), while the caller's own cancellation is detected (!cancellationToken.IsCancellationRequested) and left asOperationCanceledException.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.UseForwardedHeadersis documented in bothdocs/oauth.mdanddocs/blazor.md. Blazor's_loginContextsis gone entirely — folded intoOAuthAuthorizationOptions.AppState(server-side, in the state store), whichCompleteAuthorizationWithAppStateAsynchands back; nothing keyed by state lives in the Blazor service's own memory any more./loginnow sets an HttpOnly,SameSite=Laxcookie (__Host-prefixed,Secure, on HTTPS) via the newOAuthLoginBinding, and keeps only its SHA-256 with the pending authorization as part ofAppState./callback(and the cross-origin loopback relay) requires and deletes it, failinglogin_not_boundand revoking the tokens when it is missing or does not match.returnUrlmoved out of any cookie intoAppState, and is accepted only asOAuthLoginBinding.IsLocalUrl— a single leading/, never//or/\.DistributedCacheOAuthStateStoreis secure by default. Theoptionsconstructor parameter is no longer optional; the constructor throws unlessProtect/Unprotectare both set or the explicit, loudly namedStoreSecretsUnencryptedis. 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.@atproto/oauth-client. Protected-resource metadata must now name exactly oneauthorization_serversentry (invalid_resource_metadataon two or more); an authorization server that publishesprotected_resources(RFC 9728 §4) must list the PDS; and tokens the client refuses (wrong account, wrong scope, non-DPoPtoken_type, a failed fresh-issuer check) are revoked best effort via the newRevokeUnusableTokensAsync.docs/blazor.md(next to theHttpClient/HttpClientTimeoutoptions) and in the CHANGELOG, since it is easy to assume an egress proxy is picked up automatically.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_nonceerror that carries aDPoP-Nonceheader not being retried),AtProtoOAuthCallbackBindingTests(the binding cookie: same browser signs in, another browser is refused and revoked, cross-instance completion, a non-localreturnUrlignored), plus inOAuthTransportTests:Refresh_ATokenThatIsNotDPoPBound_IsRejected/SignIn_ATokenThatIsNotDPoPBound_IsRejectedAndRevoked(non-DPoPtoken_typeon both flows),SignIn_StartedFromAServerUrl_ADidThatResolvesToAnotherAuthorizationServer_IsRejectedAndRevoked(a URL-started callback whosesubresolves to a different AS), andSignIn_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_IsRefusedcoversunsupported_scope/unsupported_dpop_alg, andDistributed_AnEntryForAnotherState_IsRefusedcovers the distributed store refusing a mismatched embedded state. No production check was weakened or removed to make a test pass.docs/oauth.mdanddocs/blazor.mdare updated throughout to match: the "State Parameter" section no longer calls the OAuthstateCSRF 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.mdgained a "Login CSRF protection" subsection, theRequestTimeout/no-proxy notes, and theUseForwardedHeadersguidance.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.Deferred / notes
AtProtoTokenDatain #124; nothing was left to remove.ClientKeysyet, so confidential clients in the hosted integration belong with #136 (AtProtoOAuthOptions.ClientAuthentication). Mapping/oauth-client-metadata.jsonand/jwks.jsonalso stays with #136.client_key_unavailabledoes not expire the session: it is recoverable by restoring the key. The reference client deletes the session instead.Http/XrpcClient.csandAuth/SessionManager.cs(nonce plumbing),Crypto/AtProtoCrypto.cs(an internalExportParameters),AtProtoClient.cs(the removed option),Identity/IDidResolver.csandIdentity/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 theHttpClientrename.🤖 Generated with Claude Code
🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
⏳ 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.
🤖 Resolving the merge conflicts — the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the fix runs.
⏳ 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.
🤖 Resolving the merge conflicts — the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the fix runs.
⏳ 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.
🤖 Resolving the merge conflicts — the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the fix runs.
⏳ 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.
@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,OAuthClientKeyand the client assertion,OAuthStateStore.cs(in-memory per-requester and global bounds with IPv6 /64 grouping, the distributed store secure by default), and the BlazorAtProtoOAuthService.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.19370abbf31a7b636256🤖 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 — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
⏳ 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.
✅ 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.csandAtProtoScopes.csfrom 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:PostFormWithDpopAsync/SendFormWithDpopAsync): single retry gated correctly onattempt == 1 && freshNonce is not null && error?.Error == "use_dpop_nonce"; every response is inside ausingso it's disposed each loop iteration; the request budget (RequestTimeout) wraps send + retry + body read via a linkedCancellationTokenSource, and thecatch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested)correctly distinguishes the internal budget from caller cancellation.VerifyIssuerAsync): used identically at code-exchange and at refresh, resolves viaResolveIdentityUncachedAsyncandbypassCache: true, so a stale cache genuinely can't confirm a moved account.identity.PdsEndpoint!inCompleteAuthorizationWithAppStateAsyncis safe —VerifyIssuerAsyncthrowspds_not_foundbefore returning if the PDS is missing, so the null-forgiving operator is justified.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, freshjti, 60sexp), header isalg+kidonly.ValidateClientAuthenticationcorrectly rejects every inconsistent combination described in the PR text (keys withnone, no keys withprivate_key_jwt, wrong alg, missing jwks/jwks_uri, unpublished key, duplicate kids).InMemoryOAuthStateStore's per-requester + global eviction under oneLockis correct (checked the linked-list bookkeeping inRemove);DistributedCacheOAuthStateStorecorrectly refuses unencrypted storage unless explicitly opted out, and re-checks the decrypted entry'sStateagainst 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.AtProtoScopes.ValidateAudience) and the metadata canonicalization helpers (IsCanonicalIssuer,ResourceMatches,IsEndpoint) inAuthorizationServerDiscovery.csall match the documented AT Protocol profile checks (exact issuer match including port/path, no query/fragment/userinfo, RFC 9728 §3.3/§4 resource checks).OAuthLoginBinding, the relay/redeem flow inAtProtoOAuthService): the binding cookie is verified with a constant-time comparison, cleared on success, and sessions are revoked before thelogin_not_boundexception 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), andIsIssuedRelayUrlcloses the open-redirect gap left by dropping the oldIsValidReturnUrl/absolute-URL trust.Two very minor, non-blocking observations, not worth blocking on:
ResolveFromServerUrlAsync's success path re-invokesResolveAuthorizationServerAsync, redoing the resource-metadata fetch already done in thetryblock; harmless because it hits the 5-minute metadata cache, but it's a wasted re-parse.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