Grandiras/ATProto.NET
1
1
Fork 0
Code Issues Pull requests Projects Releases 9 Packages 8 Wiki Activity Actions

feat(identity)!: resolver abstraction, identity caching and SSRF-safe fetching #160

Merged
Grandiras merged 1 commit from issue-120-identity into main 2026-09-26 00:23:38 +00:00
Conversation 14 Commits 1 Files changed 90 +7661 -2835
Grandiras commented 2026-09-25 20:47:35 +00:00
Owner
Copy link

Closes #120.

Identity resolution was uncached, interface-less, written three times (OAuth discovery, DidResolver, the space server's cache) and not SSRF-hardened. The worst consequence: FirehoseVerifier resolved the DID document over HTTP for every commit. This PR gives it one abstraction, one cache, one fetch policy and one exception type, and rewires every consumer.

Abstraction

  • IDidResolver (ResolveAsync, plus default-implemented RefreshAsync / InvalidateAsync), IHandleResolver, IIdentityResolver and ResolvedIdentity(Did, Handle?, HandleVerified, Uri? PdsEndpoint, DidDocument). Implementations: DidResolver (plc/web dispatch), PlcClient, DidWebResolver, HandleResolver, IdentityResolver (bidirectional check; Handle.Invalid = handle.invalid on failure; CreateDefault() builds the stack).
  • One immutable DidDocument (ATProtoNet.Identity): Id is a Did, init-only IReadOnlyLists, DidDocumentService (was ServiceEndpoint), GetServiceEndpoint(fragment, type?), GetVerificationKey(fragment), GetHandle() (the first at:// entry, validated), GetPdsEndpoint() → Uri?; lookups take the first entry by id, as the reference implementation does. Tolerates a single-string @context and structured serviceEndpoint; a null list reads as empty, a null entry is malformed. The OAuth DidDocument / DidService copies are deleted.
  • Strict lookups for callers that must not fall back (#133, proposal 0016): TryGetServiceEndpoint(fragment, type, out endpoint) and TryGetVerificationKey(fragment, out didKey) return DidDocumentEntryStatus.Found / Absent / Malformed (a non-http(s) or structured endpoint, or the first entry with the id having another type; an unknown key type, missing or undecodable key material). The Get… methods keep the lenient "unusable reads as absent" behaviour; Spaces does not enforce the strict rule yet (left to #133).
  • DidResolutionException(kind) (an AtProtoException) replaces PlcException / DidWebException, and covers network failures, timeouts and bodies that break off or do not decode too.
  • Consumers rewired: OAuth discovery (drops its private DID resolver, handle-format validator, dns.google DoH code, hard-coded plc.directory and the bsky.social AppView fallback), OAuthClient identity call sites (DID→PDS→AS consistency and handle verification now come from one ResolvedIdentity), FirehoseVerifier, TypedFirehoseConsumer, SpaceCredentialProvider, SpaceSyncer, SpaceAuthority (uses GetServiceEndpoint), and the space server (verifiers, SpaceWriteNotifier, SimpleSpaceManagingAppClient). ISpaceDidDocumentResolver / CachingSpaceDidDocumentResolver are replaced by an IDidResolver (see Caching for the space server's own cache); resolution failures still surface as SpaceVerificationException(NotAuthorized) / SpaceCredentialException, which fixes PlcException escaping the Spaces error contracts (debloat bug 1.7).
  • AddAtProtoIdentity() (Server) registers the resolvers as singletons; AddAtProtoSpaces calls it, and Blazor's AtProtoOAuthService takes a registered IIdentityResolver.

Caching

  • CachingDidResolver: bounded LRU (10k), stale-after 1 h (served + background refresh) / expire-after 1 day (defaults of @atproto/identity), failures remembered 1 min in a separate LRU (FailureCapacity, 1,000, so they never evict documents), single-flight per DID (a shared fetch survives one caller cancelling), InvalidateAsync (detaches an in-flight fetch so a pre-invalidation result is not stored), stale-on-error with a retry back-off, optional IDistributedCache (read on memory miss, written after fetch, errors logged and ignored, entries under the wrong key rejected; a write still in flight when the DID is invalidated is removed again once it lands; invalidation does not reach other instances' memory, documented).
  • Refetch once on signature failure in FirehoseVerifier, SpaceSyncer and the delegation-token, credential and service-auth verifiers. RefreshAsync is rate-limited per DID (MinRefreshInterval, 30 s, counted from the last attempt, failed ones included) so forged signatures cannot turn into a directory request each. TypedFirehoseConsumer invalidates on #identity (processing extracted into an internal AcceptAsync to test it).
  • Parsed-key cache (#112): every verification path already goes through AtProtoCrypto.VerifySignature → DidKeyCache; verified, no change needed.
  • Space server cache: the verifiers use their own CachingDidResolver, registered keyed under SpaceServerExtensions.DidResolverKey and configured by the new SpaceServerOptions.DidCache: a hard 5-minute lifetime (StaleAfter = ExpireAfter = 5 min), so a rotated-out key is never served past 5 minutes, because its documents back credential checks and a space server rarely follows #identity. Raising ExpireAfter opts into stale-while-revalidate (fetch off the request path, but the old key keeps verifying until the background fetch lands — up to ExpireAfter on an idle server); the option's docs and docs/spaces.md explain the trade-off. It fetches under the shared IdentityResolverOptions. Everything else shares AddAtProtoIdentity's general cache (1 h / 1 day).
  • SpaceServiceAuthVerifier.IsRepoHostAsync resolves both DIDs concurrently (keeps returning false without awaiting the service when the repo has no host).

Hardened fetching (SSRF)

  • IdentityNetworkPolicy: a shared SocketsHttpHandler whose ConnectCallback resolves the host, refuses if any address is non-public (loopback, RFC1918, link-local/metadata, CGNAT, multicast, documentation, benchmarking, 6to4/Teredo, ULA, …; IPv4-mapped and NAT64 judged by the IPv4) and connects to exactly the checked addresses (no rebinding window). No proxy and no redirects on the hardened handler (a caller client that followed one is refused too). HTTPS only. At most 64 identity fetches in flight per process.
  • did:web: hostname only (FQDN, no IP literal, no path), port only on localhost, and localhost only under the opt-out. did:web:internal.corp%3A6379 is now refused (debloat bug 1.6).
  • OAuth protected-resource and authorization-server metadata fetch under the same policy by default (HTTPS, no query/fragment, public addresses, no redirects, 64 KiB, 10 s; failures are OAuthExceptions), with OAuthOptions.AllowPrivateNetworks / MetadataHttpClient and AtProtoOAuthServerOptions.AllowPrivateNetworks. PAR/token/revocation requests are not yet covered (#132). The space server's AtProtoSpaces named client runs on the hardened handler too.
  • Caps: DID documents 64 KiB, handle well-known 2 KiB (bounded reader from #118), PLC logs 8 MiB, export pages 32 MiB, stream messages 1 MiB; per-fetch 5 s timeout; handle round 5 s budget.
  • Explicit development opt-out: IdentityResolverOptions.AllowPrivateNetworks (plain HTTP, private addresses, did:web:localhost%3Aport, .test handles). A caller-supplied HttpClient is used as is (the upstream "inject your own fetch" escape hatch), which is also how a single trusted private PLC mirror is reached without opening the policy globally.
  • Handle resolution: DNS TXT over a configurable DoH endpoint (DnsOverHttpsUrl, default https://dns.google/resolve, null disables) plus HTTPS well-known, concurrently under one budget; fail closed on DNS/HTTPS conflict (kept) and now also on two distinct did= TXT values; the well-known follows up to 3 same-host HTTPS redirects itself; split TXT character-strings reassembled; reserved TLDs never looked up.

Lexicon and PLC

  • IdentityClient.ResolveIdentityAsync, ResolveDidAsync, RefreshIdentityAsync; IdentityInfo (defs#identityInfo, a LexObject), ResolveDidResponse; XrpcErrors.HandleNotFound / DidNotFound / DidDeactivated. Models kept in Lexicon/Com/AtProto/Identity/.
  • PlcClient(IdentityResolverOptions?) / PlcClient(HttpClient, Uri directoryUrl, IdentityResolverOptions?); no BaseAddress reliance. Audit, log, last, data and health APIs kept and now tested against plc.directory fixtures. Added ExportAsync(after, count) (sequenced JSONL) and StreamExportAsync(cursor) (WebSocket through the hardened handler; PlcExportStreamException carries OutdatedCursor / FutureCursor / ConsumerTooSlow).
  • Duplicated PLC models removed: PlcData (→ PlcOperation), PlcOperationService (→ Lexicon DidService); PlcAuditEntry also carries export entries (Seq, Type, Nullified now bool?). PlcOperationBuilder typed (Handle, Did), and ATProtoNet.Identity is now covered by TypedIdentifierGuardTests.

Breaking changes (migration)

  • Auth.OAuth.DidDocument / DidService removed; ServiceEndpoint → DidDocumentService; DidDocument is init-only and typed → build with initializers; GetPdsEndpoint()?.OriginalString for a string.
  • PlcException / DidWebException (+ kinds) → DidResolutionException / DidResolutionErrorKind.
  • ResolveDidAsync(string) → ResolveAsync(Did); FirehoseVerifier(DidResolver) → FirehoseVerifier(IDidResolver), FirehoseVerifier() → FirehoseVerifier(IdentityResolverOptions? = null); SpaceCredentialProvider takes IDidResolver?.
  • PlcClient constructors (explicit directory URL), Did-typed methods, GetPlcDataAsync → PlcOperation, SubmitOperationAsync → Did, PlcData / PlcOperationService removed, PlcAuditEntry.Nullified → bool?.
  • PlcOperationBuilder.CreateGenesisOperation(…, Handle, …), DeriveDid → Did, PlcSignedOperation.Did → Did.
  • AuthorizationServerDiscovery: ResolveHandleToDidAsync, ResolveHandleAuthoritativeAsync, ResolvePdsFromDidAsync, FetchDidDocumentAsync, HandleResolutionTimeout removed → discovery.IdentityResolver.ResolveAsync(…); optional IIdentityResolver ctor arg; OAuthOptions.IdentityResolver; AtProtoOAuthService ctor gains an optional IIdentityResolver.
  • OAuth metadata requests no longer use the HttpClient passed to OAuthClient; AuthorizationServerDiscovery is IDisposable and takes HttpClient? (null = policy) plus allowPrivateNetworks; metadata failures are OAuthException (invalid_server_url / metadata_fetch_failed / invalid_metadata) instead of HttpRequestException / JsonException → set AllowPrivateNetworks for a local PDS, MetadataHttpClient for a custom/test client.
  • Space server: ISpaceDidDocumentResolver, CachingSpaceDidDocumentResolver, SpaceDidDocumentResolverExtensions, SpaceServerOptions.DidDocumentCacheLifetime removed → register a keyed IDidResolver under SpaceServerExtensions.DidResolverKey; set SpaceServerOptions.DidCache.StaleAfter where DidDocumentCacheLifetime was set.
  • SpaceSyncer(space, store, IDidResolver); SpaceSyncer.ResolveSigningKeyAsync removed; SpaceAuthority.GetHostEndpoint / GetServiceEndpoint → Uri?.

Behaviour changes (agreed with the coordinator): OAuth login no longer falls back to the bsky.social AppView and waits for both handle authorities within the budget (a parked HTTPS apex costs up to 5 s instead of first-answer-wins; listed under Changed); GetServiceEndpoint treats a value that is not an absolute http(s) URL as absent (so a malformed #atproto_space_host still falls back to #atproto_pds until #133 uses the strict lookup); the space server keeps its previous hard 5-minute document lifetime by default, with stale-while-revalidate available as an opt-in.

Rebased onto 06a4285 (#123, #124, #161, #162, #163, #164, #165). Merge notes: OAuthClient combines #124's session model (the callback returns an OAuthSession whose handle is handle.invalid when unverified) with this PR's identity resolution; #124's internal AtProtoSession.InvalidHandle is replaced by the public Handle.Invalid. The new identity models pass #123's drift and JSON-name convention tests.

Verification

  • dotnet build --no-incremental (solution): 0 warnings, 0 errors.
  • Unit tests (after rebasing onto 06a4285): 3914 passed, 0 failed, 5 skipped (3919), including #123's lexicon drift and convention tests. The identity/OAuth/spaces/firehose subset (1998 tests) was re-run 3× without flakes.
  • Table-driven SSRF policy test written first (IdentityFetchPolicyTests): 53 address cases (v4/v6/mapped/NAT64), 26 did:web URL cases with and without the opt-out, 9 service-URL cases, 12 handle-TLD cases, plus connect-callback tests against a real loopback TcpListener (a name resolving to loopback is refused before any connection; the opt-out reaches it).
  • Cache tests: fresh/stale/expired lifetimes, stale-on-error back-off, remembered failures, single flight (32 concurrent → 1 fetch), caller cancellation not failing the shared fetch, invalidate (incl. during a fetch), refresh floor, LRU eviction, distributed cache sharing / invalidation / wrong-key / failure.
  • Firehose: 50 verifications of real signed commit CARs through CachingDidResolver + PlcClient make 1 directory request (zero on cache hits); key rotation verifies after one refetch; 20 forged commits cost one refetch; #identity through TypedFirehoseConsumer invalidates.
  • Spaces: rotated-key delegation token / credential / service auth verify after one refresh; forged token refreshes once then refuses; expired token does not refresh; IsRepoHostAsync concurrency proven with a two-party barrier; SpaceSyncer rotation; the verifiers resolve through the keyed resolver (a custom one registered under the key is used), which is distinct from the shared one, and SpaceServerOptions.DidCache defaults to a hard 5-minute lifetime; a cache test pins that a hard lifetime never serves a document past it.
  • Strict lookups: TryGetServiceEndpoint (found, malformed string, non-http scheme, structured endpoint, absent, wrong type) and TryGetVerificationKey (valid Multikey, undecodable key, missing key material, unknown type, absent).
  • PLC tests use responses captured from plc.directory on 2026-09-25 (atproto.com's document, audit log, /data, /export?after=0&count=2); the export stream is tested over a TestServer WebSocket, including close reasons.
  • Live smoke run against the real network (not committed): atproto.com / bsky.app handles verify bidirectionally, did:web:api.bsky.app resolves, did:web:localhost → Blocked, /export pages and /export/stream delivers operations through the hardened handler.

Review follow-up

Every finding from the PR #160 review, with its fix and the regression test (most tests are the review's adversarial probes, made permanent: IdentityHardeningTests, MetadataFetchPolicyTests).

# Finding Fix Test
A OAuthClient leaked the IdentityResolver it created Ownership tracked: a resolver the client creates (and discovery's metadata client) is disposed with it; one from OAuthOptions is left to the caller MetadataFetchPolicyTests.Dispose_ResolverTheClientCreated_IsDisposedWithIt, …_ResolverTheOptionsSupplied_IsLeftToTheCaller
B1 Body-read IOException / HttpIOException and broken Content-Encoding escaped as non-DidResolutionExceptions, and were not cached IdentityFetch: read failures → NetworkError, undecodable bodies → InvalidDocument, so they are remembered like any failure; PLC error bodies read bounded DidWeb_BodyCutShort_IsANetworkErrorAndIsRemembered (real socket; 3 resolves → 1 connection), DidWeb_BodyThatDoesNotDecode_IsAnInvalidDocument (gzip/deflate/br)
B1 null lists / null entries in a DID document threw from lookups null list → empty (converter and init), null entry → JsonException → InvalidDocument; lookups skip nulls defensively DidDocument_NullList_ReadsAsEmptyAndLookupsWork, …_NullOrMisshapenEntry_FailsToParse, DidWeb_DocumentWithANullEntry_IsAnInvalidDocument, DidDocument_NullAssignedInCode_ReadsAsEmpty
B1 One subscriber ({"service":null}, a bad endpoint, a throwing handler) could make NotifySpaceDeletedAsync throw, so deleteSpace never deleted SpaceWriteNotifier.IsDeliveryFailure: any exception but the caller's cancellation is an undelivered notification NotifySpaceDeletedAsync_OneHostileSubscriber_DoesNotStopTheOthers (3 cases), NotifyWriteAsync_ResolverThatFailsAnyOtherWay_IsSkippedWithoutThrowing
B1 A resolver failing with anything but DidResolutionException surfaced as a 500 SpaceDidResolution.ResolveOrRefuseAsync: any failure but the caller's cancellation → NotAuthorized SpaceIdentityRefreshTests.DelegationToken_ResolverThatFailsAnyOtherWay_IsARefusalNotAFault
B1 HandleResolver / IdentityResolver could throw on a handle's authorities misbehaving Well-known and DNS failures of any kind are "no answer"; a DID's handle that cannot be verified is Handle.Invalid; a handle input whose resolver throws is HandleNotFound HandleResolver_WellKnownBodyCutShort_IsNoAnswer, IdentityResolver_HandleResolverThatThrows_IsHandleInvalidNotAFailure, …_HandleInputWhoseResolverThrows_IsHandleNotFound
B2 The refresh floor was measured from the last successful fetch: a failing directory allowed a refetch per call (probe: 11 fetches in 10 s) Entries carry LastAttemptAt; the floor counts from the last attempt, failed or not RefreshAsync_AfterAFailedRefresh_TheFloorStillHolds (1 resolve + 10 refreshes over 10 s → 2 fetches)
B3 OAuth metadata was fetched with the caller's plain HttpClient: any URL from a DID document, redirects followed, body uncapped Metadata under the identity policy by default (HTTPS, no query/fragment, public addresses after DNS, no redirects, 64 KiB, 10 s), all failures OAuthException; AllowPrivateNetworks and MetadataHttpClient options; Blazor passes its HttpClient through. PAR/token/revocation left to #132 StartAuthorization_PdsOnALoopbackAddress_IsRefusedWithoutConnecting, ResolveFromIdentifier_DocumentNamingAnInternalUrl_IsRefusedWithoutConnecting (the probe), FetchMetadata_UnusableUrl_IsRefusedWithoutARequest (http, query, fragment, ftp), FetchMetadata_UnusableAnswer_IsAnOAuthException (500, oversize, 302, followed redirect, malformed), FetchMetadata_DevelopmentOptOut_ReachesALocalPds
B3 AtProtoSpaces named client reached any address Runs on IdentityNetworkPolicy.CreateHandler under the shared opt-out AddAtProtoSpaces_OutboundClient_ReachesNoPrivateAddress, …_FollowsNoRedirects
C4 did:web followed redirects Hardened handler follows none; a caller client's followed redirect (final URI ≠ requested) is HttpError; the handle well-known follows ≤3 same-host HTTPS redirects itself DidWeb_RedirectToAnotherOrigin_IsRefusedWithoutFollowing (target never contacted), DidWeb_CallersClientThatFollowedARedirect_IsRefused, HandleWellKnown_OnlySameHostHttpsRedirectsAreFollowed (4 cases)
C5 An invalidation racing a distributed-cache write could leave the old document in the shared cache Pending writes registered under the same lock as the store; an invalidation marks them, and the write removes the key again once it lands. Docs state invalidation does not reach other instances' memory InvalidateAsync_WhileTheDistributedWriteIsInFlight_TheOldDocumentDoesNotSurvive
C6 Duplicate ids: a later matching entry was used; GetHandle fell through to later entries and matched AT:// First entry by id, then its type checked (mismatch → Malformed / null); GetHandle reads only the first at:// entry (ordinal) Lookups_WithDuplicateIds_TakeTheFirstEntry, DidDocumentTests.GetHandle_ReadsOnlyTheFirstAtUriEntry
C6 No global bound on identity fetches Process-wide cap of 64 in-flight fetches, waited for within each fetch's own budget Fetches_AreCappedProcessWide (128 concurrent → peak ≤ 64)
C6 Remembered failures shared the document LRU, so bogus DIDs evicted real documents Failures in their own LRU (DidCacheOptions.FailureCapacity, 1,000) Failures_DoNotEvictDocuments (50 failures, capacity 2 documents: both still cached)
D Rebase Rebased onto 06a4285; CHANGELOG merged with changelog-merge.py (3 blocks, all 24 #120 bullets kept) Full build --no-incremental 0/0, unit suite green

Also found while turning the probes into tests: AuthorizationServerDiscovery prefixed https:// to any URL not starting with http(s)://, so ftp://host was fetched as https://ftp//host; only a bare host is prefixed now, and other schemes are invalid_server_url.

Deferred / not done

  • PAR, token and revocation requests still go through the caller's HttpClient without the address policy (#132). Spaces does not yet use the strict TryGet… lookups (#133).
  • Handle-resolution results are not cached (only DID documents are); OAuth logins and DID→identity lookups re-verify the handle each time.
  • GetRecommendedDidCredentialsResponse / SignPlcOperationRequest share a shape with PlcOperation (debloat 10.9 tail) — left alone, they are #123's Lexicon models.
  • No local PLC op-chain validation, no bounded parallel verification stage in TypedFirehoseConsumer (perf F1 "optional"), and SpaceCredentialProvider's global mint lock / superseded-credential growth (F17, non-host part) are untouched.
  • PLC export has no Enumerate* helper (the shared paginator is string-cursor and Lexicon-only); the docs show the page loop.

🤖 Generated with Claude Code

Closes #120. Identity resolution was uncached, interface-less, written three times (OAuth discovery, `DidResolver`, the space server's cache) and not SSRF-hardened. The worst consequence: `FirehoseVerifier` resolved the DID document over HTTP for every commit. This PR gives it one abstraction, one cache, one fetch policy and one exception type, and rewires every consumer. ## Abstraction - `IDidResolver` (`ResolveAsync`, plus default-implemented `RefreshAsync` / `InvalidateAsync`), `IHandleResolver`, `IIdentityResolver` and `ResolvedIdentity(Did, Handle?, HandleVerified, Uri? PdsEndpoint, DidDocument)`. Implementations: `DidResolver` (plc/web dispatch), `PlcClient`, `DidWebResolver`, `HandleResolver`, `IdentityResolver` (bidirectional check; `Handle.Invalid` = `handle.invalid` on failure; `CreateDefault()` builds the stack). - **One immutable `DidDocument`** (`ATProtoNet.Identity`): `Id` is a `Did`, init-only `IReadOnlyList`s, `DidDocumentService` (was `ServiceEndpoint`), `GetServiceEndpoint(fragment, type?)`, `GetVerificationKey(fragment)`, `GetHandle()` (the first `at://` entry, validated), `GetPdsEndpoint()` → `Uri?`; lookups take the first entry by id, as the reference implementation does. Tolerates a single-string `@context` and structured `serviceEndpoint`; a `null` list reads as empty, a `null` entry is malformed. The OAuth `DidDocument` / `DidService` copies are deleted. - **Strict lookups** for callers that must not fall back (#133, proposal 0016): `TryGetServiceEndpoint(fragment, type, out endpoint)` and `TryGetVerificationKey(fragment, out didKey)` return `DidDocumentEntryStatus.Found` / `Absent` / `Malformed` (a non-http(s) or structured endpoint, or the first entry with the id having another type; an unknown key type, missing or undecodable key material). The `Get…` methods keep the lenient "unusable reads as absent" behaviour; Spaces does not enforce the strict rule yet (left to #133). - **`DidResolutionException(kind)`** (an `AtProtoException`) replaces `PlcException` / `DidWebException`, and covers network failures, timeouts and bodies that break off or do not decode too. - **Consumers rewired**: OAuth discovery (drops its private DID resolver, handle-format validator, `dns.google` DoH code, hard-coded `plc.directory` and the `bsky.social` AppView fallback), `OAuthClient` identity call sites (DID→PDS→AS consistency and handle verification now come from one `ResolvedIdentity`), `FirehoseVerifier`, `TypedFirehoseConsumer`, `SpaceCredentialProvider`, `SpaceSyncer`, `SpaceAuthority` (uses `GetServiceEndpoint`), and the space server (verifiers, `SpaceWriteNotifier`, `SimpleSpaceManagingAppClient`). `ISpaceDidDocumentResolver` / `CachingSpaceDidDocumentResolver` are replaced by an `IDidResolver` (see Caching for the space server's own cache); resolution failures still surface as `SpaceVerificationException(NotAuthorized)` / `SpaceCredentialException`, which fixes `PlcException` escaping the Spaces error contracts (debloat bug 1.7). - `AddAtProtoIdentity()` (Server) registers the resolvers as singletons; `AddAtProtoSpaces` calls it, and Blazor's `AtProtoOAuthService` takes a registered `IIdentityResolver`. ## Caching - `CachingDidResolver`: bounded LRU (10k), stale-after 1 h (served + background refresh) / expire-after 1 day (defaults of `@atproto/identity`), failures remembered 1 min in a separate LRU (`FailureCapacity`, 1,000, so they never evict documents), single-flight per DID (a shared fetch survives one caller cancelling), `InvalidateAsync` (detaches an in-flight fetch so a pre-invalidation result is not stored), stale-on-error with a retry back-off, optional `IDistributedCache` (read on memory miss, written after fetch, errors logged and ignored, entries under the wrong key rejected; a write still in flight when the DID is invalidated is removed again once it lands; invalidation does not reach other instances' memory, documented). - **Refetch once on signature failure** in `FirehoseVerifier`, `SpaceSyncer` and the delegation-token, credential and service-auth verifiers. `RefreshAsync` is rate-limited per DID (`MinRefreshInterval`, 30 s, counted from the last attempt, failed ones included) so forged signatures cannot turn into a directory request each. `TypedFirehoseConsumer` invalidates on `#identity` (processing extracted into an internal `AcceptAsync` to test it). - Parsed-key cache (#112): every verification path already goes through `AtProtoCrypto.VerifySignature` → `DidKeyCache`; verified, no change needed. - **Space server cache**: the verifiers use their own `CachingDidResolver`, registered keyed under `SpaceServerExtensions.DidResolverKey` and configured by the new `SpaceServerOptions.DidCache`: a **hard 5-minute lifetime** (`StaleAfter = ExpireAfter = 5 min`), so a rotated-out key is never served past 5 minutes, because its documents back credential checks and a space server rarely follows `#identity`. Raising `ExpireAfter` opts into stale-while-revalidate (fetch off the request path, but the old key keeps verifying until the background fetch lands — up to `ExpireAfter` on an idle server); the option's docs and `docs/spaces.md` explain the trade-off. It fetches under the shared `IdentityResolverOptions`. Everything else shares `AddAtProtoIdentity`'s general cache (1 h / 1 day). - `SpaceServiceAuthVerifier.IsRepoHostAsync` resolves both DIDs concurrently (keeps returning `false` without awaiting the service when the repo has no host). ## Hardened fetching (SSRF) - `IdentityNetworkPolicy`: a shared `SocketsHttpHandler` whose `ConnectCallback` resolves the host, refuses if **any** address is non-public (loopback, RFC1918, link-local/metadata, CGNAT, multicast, documentation, benchmarking, 6to4/Teredo, ULA, …; IPv4-mapped and NAT64 judged by the IPv4) and connects to exactly the checked addresses (no rebinding window). No proxy and no redirects on the hardened handler (a caller client that followed one is refused too). HTTPS only. At most 64 identity fetches in flight per process. - `did:web`: hostname only (FQDN, no IP literal, no path), port only on `localhost`, and `localhost` only under the opt-out. `did:web:internal.corp%3A6379` is now refused (debloat bug 1.6). - OAuth protected-resource and authorization-server metadata fetch under the same policy by default (HTTPS, no query/fragment, public addresses, no redirects, 64 KiB, 10 s; failures are `OAuthException`s), with `OAuthOptions.AllowPrivateNetworks` / `MetadataHttpClient` and `AtProtoOAuthServerOptions.AllowPrivateNetworks`. PAR/token/revocation requests are not yet covered (#132). The space server's `AtProtoSpaces` named client runs on the hardened handler too. - Caps: DID documents 64 KiB, handle well-known 2 KiB (bounded reader from #118), PLC logs 8 MiB, export pages 32 MiB, stream messages 1 MiB; per-fetch 5 s timeout; handle round 5 s budget. - Explicit development opt-out: `IdentityResolverOptions.AllowPrivateNetworks` (plain HTTP, private addresses, `did:web:localhost%3Aport`, `.test` handles). A caller-supplied `HttpClient` is used as is (the upstream "inject your own fetch" escape hatch), which is also how a single trusted private PLC mirror is reached without opening the policy globally. - Handle resolution: DNS TXT over a **configurable** DoH endpoint (`DnsOverHttpsUrl`, default `https://dns.google/resolve`, `null` disables) plus HTTPS well-known, concurrently under one budget; fail closed on DNS/HTTPS conflict (kept) and now also on two distinct `did=` TXT values; the well-known follows up to 3 same-host HTTPS redirects itself; split TXT character-strings reassembled; reserved TLDs never looked up. ## Lexicon and PLC - `IdentityClient.ResolveIdentityAsync`, `ResolveDidAsync`, `RefreshIdentityAsync`; `IdentityInfo` (`defs#identityInfo`, a `LexObject`), `ResolveDidResponse`; `XrpcErrors.HandleNotFound` / `DidNotFound` / `DidDeactivated`. Models kept in `Lexicon/Com/AtProto/Identity/`. - `PlcClient(IdentityResolverOptions?)` / `PlcClient(HttpClient, Uri directoryUrl, IdentityResolverOptions?)`; no `BaseAddress` reliance. Audit, log, last, data and health APIs kept and now tested against plc.directory fixtures. Added `ExportAsync(after, count)` (sequenced JSONL) and `StreamExportAsync(cursor)` (WebSocket through the hardened handler; `PlcExportStreamException` carries `OutdatedCursor` / `FutureCursor` / `ConsumerTooSlow`). - Duplicated PLC models removed: `PlcData` (→ `PlcOperation`), `PlcOperationService` (→ Lexicon `DidService`); `PlcAuditEntry` also carries export entries (`Seq`, `Type`, `Nullified` now `bool?`). `PlcOperationBuilder` typed (`Handle`, `Did`), and `ATProtoNet.Identity` is now covered by `TypedIdentifierGuardTests`. ## Breaking changes (migration) - `Auth.OAuth.DidDocument` / `DidService` removed; `ServiceEndpoint` → `DidDocumentService`; `DidDocument` is init-only and typed → build with initializers; `GetPdsEndpoint()?.OriginalString` for a string. - `PlcException` / `DidWebException` (+ kinds) → `DidResolutionException` / `DidResolutionErrorKind`. - `ResolveDidAsync(string)` → `ResolveAsync(Did)`; `FirehoseVerifier(DidResolver)` → `FirehoseVerifier(IDidResolver)`, `FirehoseVerifier()` → `FirehoseVerifier(IdentityResolverOptions? = null)`; `SpaceCredentialProvider` takes `IDidResolver?`. - `PlcClient` constructors (explicit directory URL), `Did`-typed methods, `GetPlcDataAsync` → `PlcOperation`, `SubmitOperationAsync` → `Did`, `PlcData` / `PlcOperationService` removed, `PlcAuditEntry.Nullified` → `bool?`. - `PlcOperationBuilder.CreateGenesisOperation(…, Handle, …)`, `DeriveDid` → `Did`, `PlcSignedOperation.Did` → `Did`. - `AuthorizationServerDiscovery`: `ResolveHandleToDidAsync`, `ResolveHandleAuthoritativeAsync`, `ResolvePdsFromDidAsync`, `FetchDidDocumentAsync`, `HandleResolutionTimeout` removed → `discovery.IdentityResolver.ResolveAsync(…)`; optional `IIdentityResolver` ctor arg; `OAuthOptions.IdentityResolver`; `AtProtoOAuthService` ctor gains an optional `IIdentityResolver`. - OAuth metadata requests no longer use the `HttpClient` passed to `OAuthClient`; `AuthorizationServerDiscovery` is `IDisposable` and takes `HttpClient?` (null = policy) plus `allowPrivateNetworks`; metadata failures are `OAuthException` (`invalid_server_url` / `metadata_fetch_failed` / `invalid_metadata`) instead of `HttpRequestException` / `JsonException` → set `AllowPrivateNetworks` for a local PDS, `MetadataHttpClient` for a custom/test client. - Space server: `ISpaceDidDocumentResolver`, `CachingSpaceDidDocumentResolver`, `SpaceDidDocumentResolverExtensions`, `SpaceServerOptions.DidDocumentCacheLifetime` removed → register a keyed `IDidResolver` under `SpaceServerExtensions.DidResolverKey`; set `SpaceServerOptions.DidCache.StaleAfter` where `DidDocumentCacheLifetime` was set. - `SpaceSyncer(space, store, IDidResolver)`; `SpaceSyncer.ResolveSigningKeyAsync` removed; `SpaceAuthority.GetHostEndpoint` / `GetServiceEndpoint` → `Uri?`. Behaviour changes (agreed with the coordinator): OAuth login no longer falls back to the `bsky.social` AppView and waits for both handle authorities within the budget (a parked HTTPS apex costs up to 5 s instead of first-answer-wins; listed under Changed); `GetServiceEndpoint` treats a value that is not an absolute http(s) URL as absent (so a malformed `#atproto_space_host` still falls back to `#atproto_pds` until #133 uses the strict lookup); the space server keeps its previous hard 5-minute document lifetime by default, with stale-while-revalidate available as an opt-in. Rebased onto `06a4285` (#123, #124, #161, #162, #163, #164, #165). Merge notes: `OAuthClient` combines #124's session model (the callback returns an `OAuthSession` whose handle is `handle.invalid` when unverified) with this PR's identity resolution; #124's internal `AtProtoSession.InvalidHandle` is replaced by the public `Handle.Invalid`. The new identity models pass #123's drift and JSON-name convention tests. ## Verification - `dotnet build --no-incremental` (solution): 0 warnings, 0 errors. - Unit tests (after rebasing onto `06a4285`): 3914 passed, 0 failed, 5 skipped (3919), including #123's lexicon drift and convention tests. The identity/OAuth/spaces/firehose subset (1998 tests) was re-run 3× without flakes. - Table-driven SSRF policy test written first (`IdentityFetchPolicyTests`): 53 address cases (v4/v6/mapped/NAT64), 26 `did:web` URL cases with and without the opt-out, 9 service-URL cases, 12 handle-TLD cases, plus connect-callback tests against a real loopback `TcpListener` (a name resolving to loopback is refused before any connection; the opt-out reaches it). - Cache tests: fresh/stale/expired lifetimes, stale-on-error back-off, remembered failures, single flight (32 concurrent → 1 fetch), caller cancellation not failing the shared fetch, invalidate (incl. during a fetch), refresh floor, LRU eviction, distributed cache sharing / invalidation / wrong-key / failure. - Firehose: 50 verifications of real signed commit CARs through `CachingDidResolver` + `PlcClient` make **1** directory request (zero on cache hits); key rotation verifies after one refetch; 20 forged commits cost one refetch; `#identity` through `TypedFirehoseConsumer` invalidates. - Spaces: rotated-key delegation token / credential / service auth verify after one refresh; forged token refreshes once then refuses; expired token does not refresh; `IsRepoHostAsync` concurrency proven with a two-party barrier; `SpaceSyncer` rotation; the verifiers resolve through the keyed resolver (a custom one registered under the key is used), which is distinct from the shared one, and `SpaceServerOptions.DidCache` defaults to a hard 5-minute lifetime; a cache test pins that a hard lifetime never serves a document past it. - Strict lookups: `TryGetServiceEndpoint` (found, malformed string, non-http scheme, structured endpoint, absent, wrong type) and `TryGetVerificationKey` (valid Multikey, undecodable key, missing key material, unknown type, absent). - PLC tests use responses captured from plc.directory on 2026-09-25 (atproto.com's document, audit log, `/data`, `/export?after=0&count=2`); the export stream is tested over a TestServer WebSocket, including close reasons. - Live smoke run against the real network (not committed): `atproto.com` / `bsky.app` handles verify bidirectionally, `did:web:api.bsky.app` resolves, `did:web:localhost` → `Blocked`, `/export` pages and `/export/stream` delivers operations through the hardened handler. ## Review follow-up Every finding from the PR #160 review, with its fix and the regression test (most tests are the review's adversarial probes, made permanent: `IdentityHardeningTests`, `MetadataFetchPolicyTests`). | # | Finding | Fix | Test | |---|---|---|---| | A | `OAuthClient` leaked the `IdentityResolver` it created | Ownership tracked: a resolver the client creates (and discovery's metadata client) is disposed with it; one from `OAuthOptions` is left to the caller | `MetadataFetchPolicyTests.Dispose_ResolverTheClientCreated_IsDisposedWithIt`, `…_ResolverTheOptionsSupplied_IsLeftToTheCaller` | | B1 | Body-read `IOException` / `HttpIOException` and broken `Content-Encoding` escaped as non-`DidResolutionException`s, and were not cached | `IdentityFetch`: read failures → `NetworkError`, undecodable bodies → `InvalidDocument`, so they are remembered like any failure; PLC error bodies read bounded | `DidWeb_BodyCutShort_IsANetworkErrorAndIsRemembered` (real socket; 3 resolves → 1 connection), `DidWeb_BodyThatDoesNotDecode_IsAnInvalidDocument` (gzip/deflate/br) | | B1 | `null` lists / `null` entries in a DID document threw from lookups | `null` list → empty (converter and init), `null` entry → `JsonException` → `InvalidDocument`; lookups skip nulls defensively | `DidDocument_NullList_ReadsAsEmptyAndLookupsWork`, `…_NullOrMisshapenEntry_FailsToParse`, `DidWeb_DocumentWithANullEntry_IsAnInvalidDocument`, `DidDocument_NullAssignedInCode_ReadsAsEmpty` | | B1 | One subscriber (`{"service":null}`, a bad endpoint, a throwing handler) could make `NotifySpaceDeletedAsync` throw, so `deleteSpace` never deleted | `SpaceWriteNotifier.IsDeliveryFailure`: any exception but the caller's cancellation is an undelivered notification | `NotifySpaceDeletedAsync_OneHostileSubscriber_DoesNotStopTheOthers` (3 cases), `NotifyWriteAsync_ResolverThatFailsAnyOtherWay_IsSkippedWithoutThrowing` | | B1 | A resolver failing with anything but `DidResolutionException` surfaced as a 500 | `SpaceDidResolution.ResolveOrRefuseAsync`: any failure but the caller's cancellation → `NotAuthorized` | `SpaceIdentityRefreshTests.DelegationToken_ResolverThatFailsAnyOtherWay_IsARefusalNotAFault` | | B1 | `HandleResolver` / `IdentityResolver` could throw on a handle's authorities misbehaving | Well-known and DNS failures of any kind are "no answer"; a DID's handle that cannot be verified is `Handle.Invalid`; a handle input whose resolver throws is `HandleNotFound` | `HandleResolver_WellKnownBodyCutShort_IsNoAnswer`, `IdentityResolver_HandleResolverThatThrows_IsHandleInvalidNotAFailure`, `…_HandleInputWhoseResolverThrows_IsHandleNotFound` | | B2 | The refresh floor was measured from the last *successful* fetch: a failing directory allowed a refetch per call (probe: 11 fetches in 10 s) | Entries carry `LastAttemptAt`; the floor counts from the last attempt, failed or not | `RefreshAsync_AfterAFailedRefresh_TheFloorStillHolds` (1 resolve + 10 refreshes over 10 s → 2 fetches) | | B3 | OAuth metadata was fetched with the caller's plain `HttpClient`: any URL from a DID document, redirects followed, body uncapped | Metadata under the identity policy by default (HTTPS, no query/fragment, public addresses after DNS, no redirects, 64 KiB, 10 s), all failures `OAuthException`; `AllowPrivateNetworks` and `MetadataHttpClient` options; Blazor passes its `HttpClient` through. PAR/token/revocation left to #132 | `StartAuthorization_PdsOnALoopbackAddress_IsRefusedWithoutConnecting`, `ResolveFromIdentifier_DocumentNamingAnInternalUrl_IsRefusedWithoutConnecting` (the probe), `FetchMetadata_UnusableUrl_IsRefusedWithoutARequest` (http, query, fragment, ftp), `FetchMetadata_UnusableAnswer_IsAnOAuthException` (500, oversize, 302, followed redirect, malformed), `FetchMetadata_DevelopmentOptOut_ReachesALocalPds` | | B3 | `AtProtoSpaces` named client reached any address | Runs on `IdentityNetworkPolicy.CreateHandler` under the shared opt-out | `AddAtProtoSpaces_OutboundClient_ReachesNoPrivateAddress`, `…_FollowsNoRedirects` | | C4 | `did:web` followed redirects | Hardened handler follows none; a caller client's followed redirect (final URI ≠ requested) is `HttpError`; the handle well-known follows ≤3 same-host HTTPS redirects itself | `DidWeb_RedirectToAnotherOrigin_IsRefusedWithoutFollowing` (target never contacted), `DidWeb_CallersClientThatFollowedARedirect_IsRefused`, `HandleWellKnown_OnlySameHostHttpsRedirectsAreFollowed` (4 cases) | | C5 | An invalidation racing a distributed-cache write could leave the old document in the shared cache | Pending writes registered under the same lock as the store; an invalidation marks them, and the write removes the key again once it lands. Docs state invalidation does not reach other instances' memory | `InvalidateAsync_WhileTheDistributedWriteIsInFlight_TheOldDocumentDoesNotSurvive` | | C6 | Duplicate ids: a later matching entry was used; `GetHandle` fell through to later entries and matched `AT://` | First entry by id, then its type checked (mismatch → `Malformed` / `null`); `GetHandle` reads only the first `at://` entry (ordinal) | `Lookups_WithDuplicateIds_TakeTheFirstEntry`, `DidDocumentTests.GetHandle_ReadsOnlyTheFirstAtUriEntry` | | C6 | No global bound on identity fetches | Process-wide cap of 64 in-flight fetches, waited for within each fetch's own budget | `Fetches_AreCappedProcessWide` (128 concurrent → peak ≤ 64) | | C6 | Remembered failures shared the document LRU, so bogus DIDs evicted real documents | Failures in their own LRU (`DidCacheOptions.FailureCapacity`, 1,000) | `Failures_DoNotEvictDocuments` (50 failures, capacity 2 documents: both still cached) | | D | Rebase | Rebased onto `06a4285`; CHANGELOG merged with `changelog-merge.py` (3 blocks, all 24 #120 bullets kept) | Full build `--no-incremental` 0/0, unit suite green | Also found while turning the probes into tests: `AuthorizationServerDiscovery` prefixed `https://` to any URL not starting with `http(s)://`, so `ftp://host` was fetched as `https://ftp//host`; only a bare host is prefixed now, and other schemes are `invalid_server_url`. ## Deferred / not done - PAR, token and revocation requests still go through the caller's `HttpClient` without the address policy (#132). Spaces does not yet use the strict `TryGet…` lookups (#133). - Handle-resolution results are not cached (only DID documents are); OAuth logins and DID→identity lookups re-verify the handle each time. - `GetRecommendedDidCredentialsResponse` / `SignPlcOperationRequest` share a shape with `PlcOperation` (debloat 10.9 tail) — left alone, they are #123's Lexicon models. - No local PLC op-chain validation, no bounded parallel verification stage in `TypedFirehoseConsumer` (perf F1 "optional"), and `SpaceCredentialProvider`'s global mint lock / superseded-credential growth (F17, non-host part) are untouched. - PLC export has no `Enumerate*` helper (the shared paginator is string-cursor and Lexicon-only); the docs show the page loop. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(identity)!: resolver abstraction, identity caching and SSRF-safe fetching (closes #120)
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 53s
Details
d4139ab8b5 
DID and handle resolution was written three times (OAuth discovery, DidResolver,
the space server's cache), had no interface and no cache, and was not SSRF-hardened:
FirehoseVerifier fetched a DID document for every commit, and the resolver the
space server used accepted a port on any did:web host.

- IDidResolver / IHandleResolver / IIdentityResolver and ResolvedIdentity, with
  bidirectional handle verification (handle.invalid on failure), one immutable
  DidDocument, and DidResolutionException(kind) replacing PlcException and
  DidWebException.
- CachingDidResolver: bounded LRU with stale/max TTL, negative caching, single
  flight, InvalidateAsync, rate-limited RefreshAsync and an optional
  IDistributedCache. Consumers refetch once when a signature fails, and
  TypedFirehoseConsumer invalidates on #identity.
- One fetch policy for did:web, PLC, handle well-knowns and DoH: HTTPS only, a
  connect callback that refuses non-public addresses after DNS, hostname-only
  did:web, size caps, short timeouts, and an explicit development opt-out.
- Handle resolution over a configurable DNS-over-HTTPS endpoint plus the HTTPS
  well-known, failing closed on conflict.
- OAuth discovery, FirehoseVerifier, SpaceCredentialProvider, SpaceSyncer,
  SpaceAuthority and the space server rewired onto the interfaces;
  ISpaceDidDocumentResolver is replaced by a keyed CachingDidResolver with a hard
  5-minute lifetime (SpaceServerOptions.DidCache), since its documents back
  credential checks; everything else shares AddAtProtoIdentity's cache.
- DidDocument.TryGetServiceEndpoint / TryGetVerificationKey tell an absent entry
  from a malformed one, for callers that must not fall back.
- com.atproto.identity.resolveIdentity / resolveDid / refreshIdentity, and
  PlcClient with an explicit directory URL, /export and /export/stream.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-25 20:47:37 +00:00
Collaborator
Copy link

🤖 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-160/run-2026-09-25T20-47-36-405Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-25 20:47:40 +00:00
Collaborator
Copy link

⏳ 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-26T01:47:36.895Z) — 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-26T01:47:36.895Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f25ad24c9ffeIdhvn0jpdupDOC -->
Grandiras commented 2026-09-25 20:47:43 +00:00
Author
Owner
Copy link

@claude /review — this is security-sensitive. Please read these in full from the branch: the SSRF policy (the ConnectCallback address filtering after DNS, did:web port/path rules, size caps), the caching resolver (single-flight, negative cache, stale/expire, invalidation), and the refetch-once-on-signature-failure paths in FirehoseVerifier and the Spaces verifiers.

@claude /review — this is security-sensitive. Please read these in full from the branch: the SSRF policy (the `ConnectCallback` address filtering after DNS, did:web port/path rules, size caps), the caching resolver (single-flight, negative cache, stale/expire, invalidation), and the refetch-once-on-signature-failure paths in `FirehoseVerifier` and the Spaces verifiers.
claude-bot commented 2026-09-25 20:47:45 +00:00
Collaborator
Copy link

🤖 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/601); 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-160/run-601.md) — updating every ~5 minutes while the task runs.
claude-reviewer requested changes 2026-09-25 20:50:22 +00:00
Dismissed
claude-reviewer left a comment
Copy link

🛑 Verdict: request changes — blocking issues below.

This is a large, well-executed consolidation: one IDidResolver/IHandleResolver/IIdentityResolver abstraction, a real bounded LRU cache with single-flight and stale-on-error handling, and a genuinely careful SSRF policy (IdentityNetworkPolicy) that checks every resolved address in the connect callback and connects only to the addresses it checked. The space-server refresh-once-on-signature-failure logic (SpaceDidResolution.VerifyWithKeyRefreshAsync, SpaceServiceAuthVerifier.VerifyServiceAuthAsync) and the concurrent IsRepoHostAsync resolution are correct and match the PR description. I verified several call sites against the actual PR branch (origin/issue-120-identity) rather than relying on the diff alone, since the diff was truncated.

One real defect: OAuthClient leaks the IdentityResolver it creates for itself. When OAuthOptions.IdentityResolver is null (the documented default), OAuthClient's constructor calls IdentityResolver.CreateDefault(...), which builds and owns a CachingDidResolver → DidResolver → PlcClient + DidWebResolver, each with its own HttpClient/SocketsHttpHandler (confirmed in PlcClient.PlcClient(IdentityResolverOptions?) and IdentityResolver.CreateDefault, whose own doc says "Dispose it to release its clients"). OAuthClient stores this only inside _discovery and never disposes it — OAuthClient.Dispose() (line ~868 of OAuthClient.cs) only disposes pending DPoP keys. This silently contradicts the disposal discipline the rest of this release cycle established (AtProtoClient "disposes only what it created"). It's not catastrophic in the common singleton usage (AtProtoOAuthService builds one OAuthClient and reuses it), but any code path that constructs more than one OAuthClient without supplying a shared IdentityResolver (tests, multi-tenant apps, samples) leaks a SocketsHttpHandler/connection pool per instance for the process lifetime.

Everything else I could verify — the DidDocument.SigningKeyId constant, SpaceTokenException.IsSignatureFailure, the DI registrations in IdentityServiceCollectionExtensions and SpaceServerExtensions, and the address-checking logic in IdentityNetworkPolicy.ConnectToPublicAddressAsync — matches what the PR description claims and looks correct.

⏱ 2m 20s · 34 turns · tokens: 108.0k in / 13.7k out (+3797.5k cache-read)
📜 Full session transcript

🛑 **Verdict: request changes** — blocking issues below. This is a large, well-executed consolidation: one `IDidResolver`/`IHandleResolver`/`IIdentityResolver` abstraction, a real bounded LRU cache with single-flight and stale-on-error handling, and a genuinely careful SSRF policy (`IdentityNetworkPolicy`) that checks every resolved address in the connect callback and connects only to the addresses it checked. The space-server refresh-once-on-signature-failure logic (`SpaceDidResolution.VerifyWithKeyRefreshAsync`, `SpaceServiceAuthVerifier.VerifyServiceAuthAsync`) and the concurrent `IsRepoHostAsync` resolution are correct and match the PR description. I verified several call sites against the actual PR branch (`origin/issue-120-identity`) rather than relying on the diff alone, since the diff was truncated. One real defect: `OAuthClient` leaks the `IdentityResolver` it creates for itself. When `OAuthOptions.IdentityResolver` is null (the documented default), `OAuthClient`'s constructor calls `IdentityResolver.CreateDefault(...)`, which builds and owns a `CachingDidResolver` → `DidResolver` → `PlcClient` + `DidWebResolver`, each with its own `HttpClient`/`SocketsHttpHandler` (confirmed in `PlcClient.PlcClient(IdentityResolverOptions?)` and `IdentityResolver.CreateDefault`, whose own doc says "Dispose it to release its clients"). `OAuthClient` stores this only inside `_discovery` and never disposes it — `OAuthClient.Dispose()` (line ~868 of `OAuthClient.cs`) only disposes pending DPoP keys. This silently contradicts the disposal discipline the rest of this release cycle established (`AtProtoClient` "disposes only what it created"). It's not catastrophic in the common singleton usage (`AtProtoOAuthService` builds one `OAuthClient` and reuses it), but any code path that constructs more than one `OAuthClient` without supplying a shared `IdentityResolver` (tests, multi-tenant apps, samples) leaks a `SocketsHttpHandler`/connection pool per instance for the process lifetime. Everything else I could verify — the `DidDocument.SigningKeyId` constant, `SpaceTokenException.IsSignatureFailure`, the DI registrations in `IdentityServiceCollectionExtensions` and `SpaceServerExtensions`, and the address-checking logic in `IdentityNetworkPolicy.ConnectToPublicAddressAsync` — matches what the PR description claims and looks correct. --- ⏱ 2m 20s · 34 turns · tokens: 108.0k in / 13.7k out (+3797.5k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-160/run-601.md) <!-- claude-verdict:request_changes --> <!-- claude-reviewed-head:d4139ab8b5a9e64f616507f8712a8c5161bc04df -->
src/ATProtoNet/Auth/OAuth/OAuthClient.cs Outdated
@ -60,3 +59,1 @@
{
HandleResolutionTimeout = _options.HandleResolutionTimeout,
};
_discovery = new AuthorizationServerDiscovery(
claude-reviewer commented 2026-09-25 20:50:22 +00:00
Collaborator
Copy link

When _options.IdentityResolver is null, this constructs an owned IdentityResolver.CreateDefault(...), which per its own XML doc (src/ATProtoNet/Identity/IdentityResolver.cs) owns a CachingDidResolver and HandleResolver, each of which owns its own HttpClient/SocketsHttpHandler (see PlcClient, DidWebResolver). OAuthClient.Dispose() (further down in this file) never disposes _discovery or its IdentityResolver — it only disposes pending DPoP keys. Every OAuthClient built without an explicit shared IdentityResolver therefore leaks a connection pool for the process lifetime. Either dispose the resolver in OAuthClient.Dispose() when this constructor created it (tracking ownership like AtProtoClient does elsewhere in this release), or expose/document the leak explicitly.

When `_options.IdentityResolver` is null, this constructs an owned `IdentityResolver.CreateDefault(...)`, which per its own XML doc (`src/ATProtoNet/Identity/IdentityResolver.cs`) owns a `CachingDidResolver` and `HandleResolver`, each of which owns its own `HttpClient`/`SocketsHttpHandler` (see `PlcClient`, `DidWebResolver`). `OAuthClient.Dispose()` (further down in this file) never disposes `_discovery` or its `IdentityResolver` — it only disposes pending DPoP keys. Every `OAuthClient` built without an explicit shared `IdentityResolver` therefore leaks a connection pool for the process lifetime. Either dispose the resolver in `OAuthClient.Dispose()` when this constructor created it (tracking ownership like `AtProtoClient` does elsewhere in this release), or expose/document the leak explicitly.
opencode-bot commented 2026-09-25 21:03:11 +00:00
Collaborator
Copy link

🤖 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-160/run-2026-09-25T21-03-11-559Z.md) — updating every ~5 minutes while the fix runs.
opencode-bot commented 2026-09-25 21:03:13 +00:00
Collaborator
Copy link

⏳ 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-26T02:00:33.556Z) — 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-26T02:00:33.556Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f259ee28fffeOjwk7cLYtX7Qb7 -->
opencode-bot commented 2026-09-25 23:59:10 +00:00
Collaborator
Copy link

🤖 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-160/run-2026-09-25T23-59-09-910Z.md) — updating every ~5 minutes while the fix runs.
opencode-bot commented 2026-09-25 23:59:12 +00:00
Collaborator
Copy link

⏳ 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-26T04:57:05.468Z) — 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-26T04:57:05.468Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f24fdc6a3ffeqNeiuBWI7VraJa -->
Grandiras commented 2026-09-26 00:22:13 +00:00
Author
Owner
Copy link

Follow-up in 95b8eca, rebased onto main. It addresses the REQUEST_CHANGES finding and an independent adversarial security review of the SSRF policy, caching and refetch paths. That review found nothing blocking and confirmed the core address filtering across 34 address classes, rebinding and proxy bypass. See Review follow-up in the description for each finding with its fix and regression test. In short:

  • A: OAuthClient now disposes the identity resolver (and the metadata client) it creates.
  • B1: Body-read and decompression failures and null DID-document collections become DidResolutionException. A hostile registerNotify target can no longer block deleteSpace. The Spaces verifiers answer NotAuthorized instead of 500. Handle failures yield handle.invalid.
  • B2: The refresh rate limit now counts attempts, including failed ones.
  • B3: The OAuth metadata fetches follow the identity fetch policy: HTTPS only, public addresses only, no redirects, 64 KiB cap. The AtProtoSpaces client uses the hardened handler.
  • C4–C6: No redirects for DID documents. The distributed-cache invalidation race is closed. Lookups match the reference behaviour. A global fetch cap and a separate negative cache are in place.

@claude /review

Follow-up in `95b8eca`, rebased onto main. It addresses the REQUEST_CHANGES finding and an **independent adversarial security review** of the SSRF policy, caching and refetch paths. That review found nothing blocking and confirmed the core address filtering across 34 address classes, rebinding and proxy bypass. See *Review follow-up* in the description for each finding with its fix and regression test. In short: - **A:** `OAuthClient` now disposes the identity resolver (and the metadata client) it creates. - **B1:** Body-read and decompression failures and `null` DID-document collections become `DidResolutionException`. A hostile `registerNotify` target can no longer block `deleteSpace`. The Spaces verifiers answer `NotAuthorized` instead of 500. Handle failures yield `handle.invalid`. - **B2:** The refresh rate limit now counts attempts, including failed ones. - **B3:** The OAuth metadata fetches follow the identity fetch policy: HTTPS only, public addresses only, no redirects, 64 KiB cap. The `AtProtoSpaces` client uses the hardened handler. - **C4–C6:** No redirects for DID documents. The distributed-cache invalidation race is closed. Lookups match the reference behaviour. A global fetch cap and a separate negative cache are in place. @claude /review
Grandiras force-pushed issue-120-identity from d4139ab8b5
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 53s
Details
to 95b8eca335
All checks were successful
CI / pds-integration (pull_request) Successful in 28s
Details
CI / build-and-test (pull_request) Successful in 52s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s
Details
2026-09-26 00:22:13 +00:00 Compare
claude-bot commented 2026-09-26 00:22:16 +00:00
Collaborator
Copy link

🤖 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/611); 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-160/run-611.md) — updating every ~5 minutes while the task runs.
opencode-bot commented 2026-09-26 00:23:15 +00:00
Collaborator
Copy link

🤖 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-160/run-2026-09-26T00-23-14-847Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-26 00:23:17 +00:00
Collaborator
Copy link

⏳ 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-26T05:23:15.312Z) — 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-26T05:23:15.312Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f24e7b806ffedsHh83Ute0SOLM -->
claude-reviewer approved these changes 2026-09-26 00:23:23 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a re-review after my previous request_changes pass. The one blocking issue I raised then — OAuthClient leaking the IdentityResolver it creates internally (via IdentityResolver.CreateDefault), never disposed, silently leaking a SocketsHttpHandler/connection pool per instance — is now fixed. I verified this directly against origin/issue-120-identity (rebased onto current main at 06a4285, confirmed via git merge-base):

  • OAuthClient now tracks ownership with a _ownedIdentityResolver field, set only when _options.IdentityResolver is null, and Dispose() calls both _discovery.Dispose() and _ownedIdentityResolver?.Dispose().
  • AuthorizationServerDiscovery mirrors the same pattern for its own metadata HttpClient (_ownsMetadataClient) and, separately, an identity resolver it might own when constructed directly without one.
  • No double-dispose risk: when OAuthClient passes its already-resolved identityResolver into AuthorizationServerDiscovery's constructor, that value is non-null, so AuthorizationServerDiscovery's own _ownedIdentityResolver stays null and only OAuthClient disposes it.
  • The regression tests described in the PR's review-follow-up table exist and do the right thing: MetadataFetchPolicyTests.Dispose_ResolverTheClientCreated_IsDisposedWithIt asserts a ResolveAsync call on the client-created resolver throws ObjectDisposedException after client.Dispose(); Dispose_ResolverTheOptionsSupplied_IsLeftToTheCaller asserts a caller-supplied resolver's Dispose() is never called.

I also spot-checked IdentityNetworkPolicy (the SSRF connect-callback and address classification) against the PR's description and it matches: DNS resolved once, every returned address checked before connecting to exactly those addresses, no proxy, IPv4-mapped/NAT64 addresses judged by their embedded IPv4, and a solid set of reserved ranges (RFC1918, CGNAT, link-local/metadata, documentation, benchmarking, 6to4/Teredo, multicast) is excluded on both v4 and v6.

I was unable to run a full build/test pass in this environment (sandbox denied git fetch/git worktree/network commands), so I relied on direct source inspection of the actual PR branch rather than test execution. Given the previously blocking issue is resolved with real regression coverage and nothing new I inspected raised a concern, this is good to merge.

⏱ 0m 57s · 21 turns · tokens: 93.2k in / 5.2k out (+2212.6k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a re-review after my previous request_changes pass. The one blocking issue I raised then — `OAuthClient` leaking the `IdentityResolver` it creates internally (via `IdentityResolver.CreateDefault`), never disposed, silently leaking a `SocketsHttpHandler`/connection pool per instance — is now fixed. I verified this directly against `origin/issue-120-identity` (rebased onto current `main` at `06a4285`, confirmed via `git merge-base`): - `OAuthClient` now tracks ownership with a `_ownedIdentityResolver` field, set only when `_options.IdentityResolver` is null, and `Dispose()` calls both `_discovery.Dispose()` and `_ownedIdentityResolver?.Dispose()`. - `AuthorizationServerDiscovery` mirrors the same pattern for its own metadata `HttpClient` (`_ownsMetadataClient`) and, separately, an identity resolver it might own when constructed directly without one. - No double-dispose risk: when `OAuthClient` passes its already-resolved `identityResolver` into `AuthorizationServerDiscovery`'s constructor, that value is non-null, so `AuthorizationServerDiscovery`'s own `_ownedIdentityResolver` stays null and only `OAuthClient` disposes it. - The regression tests described in the PR's review-follow-up table exist and do the right thing: `MetadataFetchPolicyTests.Dispose_ResolverTheClientCreated_IsDisposedWithIt` asserts a `ResolveAsync` call on the client-created resolver throws `ObjectDisposedException` after `client.Dispose()`; `Dispose_ResolverTheOptionsSupplied_IsLeftToTheCaller` asserts a caller-supplied resolver's `Dispose()` is never called. I also spot-checked `IdentityNetworkPolicy` (the SSRF connect-callback and address classification) against the PR's description and it matches: DNS resolved once, every returned address checked before connecting to exactly those addresses, no proxy, IPv4-mapped/NAT64 addresses judged by their embedded IPv4, and a solid set of reserved ranges (RFC1918, CGNAT, link-local/metadata, documentation, benchmarking, 6to4/Teredo, multicast) is excluded on both v4 and v6. I was unable to run a full build/test pass in this environment (sandbox denied `git fetch`/`git worktree`/network commands), so I relied on direct source inspection of the actual PR branch rather than test execution. Given the previously blocking issue is resolved with real regression coverage and nothing new I inspected raised a concern, this is good to merge. --- ⏱ 0m 57s · 21 turns · tokens: 93.2k in / 5.2k out (+2212.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-160/run-611.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:95b8eca335bf28b3cf2da290083a0cd06029bc66 -->
Grandiras deleted branch issue-120-identity 2026-09-26 00:23:38 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
claude-reviewer
Labels
Clear labels   
breaking-change
Introduces a breaking API change

  
bug
Something is not working correctly

  
documentation
Documentation improvements

  
duplicate
This issue or PR already exists

  
enhancement
New feature or improvement

  
good first issue
Good for newcomers to the project

  
help wanted
Extra attention is needed

  
performance
Performance-related issue or improvement

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Grandiras/ATProto.NET!160
No description provided.