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

refactor!: debloat auth, hosting and Spaces #181

Merged
Grandiras merged 19 commits from debloat-b-auth into main 2026-09-27 16:19:10 +00:00
Conversation 15 Commits 19 Files changed 95 +1521 -3008
Grandiras commented 2026-09-27 12:47:28 +00:00
Owner
Copy link

Part of #180 (pass B: auth, hosting and Spaces).

Less code and surface in the auth, hosting and Spaces slice, with the same security behaviour. Items follow the plan's numbering.

What changed

Verification plumbing

  • [1] SpaceServiceAuthVerifier is folded into NotifyWriteEndpoint (VerifyCallerAsync). The endpoint builds a ServiceAuthVerifier from the space server's resolver, replay store, ClockSkew and MaxSingleUseTokenLifetime, and maps ServiceAuthException to NotAuthorized. [AuthenticatesItself] and the iss == repo check are unchanged.
  • [11] + A-Q5
    • AtProtoServiceAuthOptions.Verifier is a ServiceAuthVerifierOptions and replaces the scheme's copies of AllowedKeyIds, ClockSkew and MaxTokenLifetime.
    • The two classes share one internal Validate().
    • RequireLexiconMethod is gone; lxm is always required.
  • [5] Small duplicates now live in one place each:
    • one Authorization-header parser (AuthorizationHeader) for the scheme handler, notifyWrite and SpaceRequestAuthenticator;
    • one Jwt.RequireTokenId for the jti check and message in service auth, DPoP and SpaceTokens;
    • Jwt.EncodeHeader with a nullable typ, which replaces OAuthClientKey.EncodeHeader;
    • ReadJwtExpiry uses TryGetNumericDate;
    • ResolveServiceEndpoint was moved onto DidDocument and reverted after review (see Review fixes, item 5);
    • one NoLease;
    • one SweepSchedule for the in-memory and EF replay stores;
    • one SpaceServiceCall (resolve the endpoint, sign, send) for SpaceWriteNotifier and SimpleSpaceManagingAppClient.

Public surface

  • [3] Removed or made internal:
    • DPoPProof keeps only KeyThumbprint.
    • DPoPProofValidator.ProofType and ProofAlgorithm, ClaimsSpaceCallerResolver.DidClaimType, SpaceTokens.TryParse, SpaceTokens.Verify(type, jwt, …), SpaceToken.ToSpaceUri(), services.AddAtProtoPdsAdmin(PdsAdminOptions) with CopyOptions, and WithSessionStore(factory) are removed.
    • SimpleSpaceAuthorityStore.Inner and SpaceServerOptions.BuildRequestUri are internal.
    • OAuthClient.CompleteAuthorizationAsync now returns the OAuthAuthorizationResult (session + app state), replacing CompleteAuthorizationWithAppStateAsync.
  • [6] + A-Q2 Hosted OAuth:
    • AtProtoOAuthServerOptions.HttpClient, HttpClientTimeout, HandleResolutionTimeout and AllowPrivateNetworks are removed.
    • WithOAuth() calls AddAtProtoIdentity() and builds its named client from IdentityResolverOptions.AllowPrivateNetworks: policy handler, no redirects, no proxy, UA, 30 s.
    • The AtProtoOAuthService constructor is internal, without its owned-client branch or the duplicate ClientKeys check. WithOAuth validation still refuses keys without metadata at startup.
    • AuthorizationServerDiscovery and OAuthClient.Discovery are internal.
  • [10] One internal InMemoryDefaultsWarning replaces InMemorySessionStoreWarning, InMemorySpaceStoreWarning, InMemorySessionStoreChoice and SpaceServerOptions.WarnOnInMemoryStores.
    • It warns only about an instance that a fallback registration created, so a store the app registers itself (the in-memory one included) is its choice.
    • AddAtProtoServiceAuth() now warns as well. It used to fall back to the in-process replay store silently.
  • [19] VerifiedCredentialCacheCapacity, ClientMetadataCacheLifetime and MaxClientMetadataBytes are internal. Tests still set them.
  • [17] IdentityAction and AtProtoScopes.Identity(attr, action) are removed.
  • [18] ISpaceCredentialIssuer is removed; the exchange takes SpaceCredentialIssuer.
  • A-Q7 MapTapWebhook keeps the delegate overload (the docs' main example). ITapEventHandler, MapTapWebhook<THandler>, OnUnreadableEvent and TapUnreadableEvent go. An unreadable delivery is still answered 200, with a logged warning.
  • A-Q8 ClaimsSpaceCallerResolver still accepts any scheme's did. It now documents why: no stored session or credential is used, so a verified caller acts only as itself.
  • [15] Aspire.Hosting: WithPlcUrl, WithCrawlers, WithInviteCodeRequired, WithBlobUploadLimit and WithReportService are one generic method each over AtProtoPdsContainerResourceBase. Each resource names its own environment variables (SharedSettingNames).

Mechanics

  • [8] The 27 CA2007 pragma pairs in the EF stores, plus 2 in ATProtoNet.Server, become var ctx = …; await using var ctxScope = ctx.ConfigureAwait(false);.
  • [14] XrpcQueryScratch (thread-static pool) is gone. The query binder allocates a 256-byte ArrayBufferWriter and a Utf8JsonWriter per bind, which is noise next to the request pipeline. No benchmark was run.
  • [2] Docs:
    • SpaceServerOptions and AtProtoOAuthServerOptions are cut to summary + default.
    • Each rationale is kept once and referenced: per-process replay store (IJtiReplayStore), no retrying handler (IAtProtoBuilder.HttpClient), account signer.
    • Class remarks that retold the method body are dropped: ServiceAuthVerifier checks, DPoP six-checks list.

Tests

  • [4] The JWS encoding cases are pinned once in JwtTests:
    • padded segments (new theory);
    • AAAAA/non-JSON/non-object rows per segment;
    • the standard alphabet (already there).
    • Each verifier keeps one "malformed → its own error" theory: service auth (BadJwt, existing), notifyWrite (NotAuthorized), DPoP (NotAuthorized), delegation (InvalidDelegationToken, new), attestation (InvalidClientAttestation, new), SpaceTokens.Parse (SpaceTokenException).
  • [13] DPoPProofGeneratorTests drops the tests DPoPInteropTests pins through the validator: thumbprint shape/consistency, header shape, unique jti, ath presence, export/import ×2. The nonce tests merge into one.
  • [9] Seven private HttpMessageHandler stubs are replaced by TestSupport/HttpStub. The warning tests use the shared CapturingLoggerProvider.

Where each security property is still tested

Property Tests
SSRF-safe named clients, no redirects AtProtoBuilderTests.WithOAuth_TheLoginSendsThroughItsNamedClient (no redirect, no proxy, ConnectCallback, UA, 30 s); …TheIdentityOptionsAllowPrivateNetworks_LiftsTheAddressCheck; AtProtoOAuthServiceHttpClientTests.WithoutAnHttpClient_TheOAuthClientsOwnKeepsTheIdentityFetchPolicy; MetadataFetchPolicyTests (unchanged)
Bounded reads HttpSpaceClientMetadataResolverTests (cap, now internal, still set by the test); TapWebhookTests 413 cases
Login-CSRF binding, local-only return URLs AtProtoOAuthCallbackBindingTests, CookieRelayTests (unchanged, retargeted to the internal ctor)
Issuer verification with fresh resolution OAuthCallbackHandleVerificationTests, AuthorizationServerMetadataValidationTests (unchanged apart from var (session, _))
Replay retention ≥ acceptance window ServiceAuthVerifierTests.…ReplayedAfterExpiryButWithinTheSkew_IsStillRefused; DPoPProofValidatorTests.…ReplayedAtTheInstantItAgesOut_IsRefused; DPoPInteropTests.RfcResourceRequestProof_Validates (now asserts the RFC jti is spent until iat + ProofLifetime); delegation/attestation …WithinTheSkew
jti spent last ServiceAuthVerifierTests.…ForgedToken_DoesNotSpendTheGenuineTokensJti, …RefusedForItsAudience_DoesNotSpendItsJti; NotifyWriteServiceAuthTests.VerifyCallerAsync_SameTokenTwice_IsRefusedTheSecondTime
[AuthenticatesItself] marker ServiceAuthWithSpacesTests (unchanged)
Refresh coordinator lease AtProtoBuilderTests.WithOAuth_AndTheClientFactory_ShareOneRefreshCoordinator, SessionRaceTests, the coordinator test in AtProtoOAuthCallbackBindingTests
notifyWrite iss == repo SpaceServerEndpointTests.NotifyWrite_SignedByAServiceAtTheWritersPdsOrigin_IsRefused

Where the removed duplicate tests' behaviours are pinned

  • SpaceServiceAuthVerifierTests (21 tests): 5 wrapper-specific tests remain as NotifyWriteServiceAuthTests:
    • each audience form + P-256;
    • no Bearer;
    • malformed → NotAuthorized with the ServiceAuthException cause;
    • replay;
    • the MaxSingleUseTokenLifetime ceiling.
      The rest map to ServiceAuthVerifierTests:
    • time claims out of range (rows iat long.MaxValue and iat -62135596801 added there);
    • missing jti/lxm/iat;
    • kid; iss fragment; not a DID;
    • expired; future; ceiling;
    • audience; wrong key; malformed; high-S/out-of-range S.
    • JWS padding, base64url and non-object cases are in JwtTests.
    • The SpaceIdentityRefreshTests service-auth refresh case is ServiceAuthVerifierTests.VerifyAsync_KeyRotatedSinceTheCachedDocument_VerifiesAgainstTheRefreshedOne.
  • DPoP JWS cases: JwtTests + DPoPProofValidatorTests.ValidateAsync_MalformedProof_IsNotAuthorized. The JWK-coordinate tests stay: they cover JWK decoding, not JWS decoding.
  • SpaceTokensTests TryParse and Verify-overload tests: retargeted to Parse (throws) and Verify(Parse(…)). The duplicate "already parsed" and "by jwt" variants are merged.
  • RequireLexiconMethod tests: removed with the knob. VerifyAsync_NoLxm_IsRefused pins the new always-required rule. The delegation-token-is-not-service-auth test now binds no method instead of disabling the requirement.
  • DPoP Method/Uri/TokenId asserts:
    • htu normalization is now read from the proof's own claims (TestJws.DecodeJson);
    • the RFC jti through the replay store;
    • htm/htu are enforced by the validator in the same tests.

Breaking changes (all in CHANGELOG Breaking changes and docs/migrating-to-0.7.md)

  • SpaceServiceAuthVerifier removed → ServiceAuthVerifier / AddAtProtoServiceAuth().
  • AtProtoServiceAuthOptions.{AllowedKeyIds,ClockSkew,MaxTokenLifetime} → o.Verifier.…; RequireLexiconMethod removed.
  • CompleteAuthorizationAsync returns OAuthAuthorizationResult → var (session, _) = …; CompleteAuthorizationWithAppStateAsync removed.
  • Test-only members listed under [3] → AtProtoClaimTypes.Did, Verify(Parse(…)), SpaceUri.Parse(token.Subject).
  • AddAtProtoPdsAdmin(PdsAdminOptions) → (url, password) + Configure<PdsAdminOptions>; WithSessionStore(factory) → WithSessionStore<T>().
  • Hosted OAuth knobs → AddAtProtoIdentity(o => …) and AddHttpClient(AtProtoOAuthExtensions.HttpClientName); AtProtoOAuthService ctor internal → resolve from DI.
  • AuthorizationServerDiscovery / OAuthClient.Discovery internal → IIdentityResolver.
  • WarnOnInMemoryStores, MaxClientMetadataBytes removed → register InMemoryJtiReplayStore yourself to keep it without the warning.
  • IdentityAction → Identity(attr).
  • ISpaceCredentialIssuer → SpaceCredentialIssuer.
  • ITapEventHandler, MapTapWebhook<T>, OnUnreadableEvent → the delegate overload.
  • Aspire With* pairs → generic; recompile.

PublicAPI.Shipped.txt is updated in ATProtoNet, ATProtoNet.Server and ATProtoNet.Aspire.Hosting (−87 lines). Every Unshipped.txt is still empty.

Measurements (loc.py origin/main HEAD, this slice)

before after Δ
src code (Auth, core Spaces, Server/*, EF, Aspire.Hosting) 12,191 11,816 −375
src XML doc 7,251 6,799 −452
tests/ATProtoNet.Tests 49,571 48,898 −673
PublicAPI txt 9,983 9,896 −87

Overall diff: 92 files, +1,260 / −3,036.

Verification

  • dotnet build --no-incremental: 0 warnings, 0 errors. This includes the compiled doc snippets, samples and integration tests.
  • dotnet test tests/ATProtoNet.Tests, run twice: 4,996 passed, 0 failed, 5 skipped (5,001). Before the change: 5,059 / 5 / 5,064. The difference is the duplicate tests removed above.

Review fixes

These commits sit on top of d61fb30. Each code fix has a regression test that fails without it.

  1. 6d92607 — the startup warning could crash the host (medium). InMemoryDefaultsWarning resolved the stores from the root provider to look at them.
    • Under scope validation, a scoped IJtiReplayStore stopped the host. A service-auth-only app also built its session store for nothing.
    • The fix: the warning records the fallback descriptors it added (InMemoryFallbacks) and checks whether each is still the last registration for its type. It resolves no store.
    • Tests: a scoped replay store under ValidateScopes starts cleanly (spaces and service auth); service auth never resolves a throwing session store; a store registered before the fallback adds no warning.
    • CHANGELOG: Fixed bullet. main's space warning had the same flaw.
  2. e998616 — the login used the container's identity stack.
    • The fix: WithOAuth no longer calls AddAtProtoIdentity(); it registers only the identity options. The login uses a registered IIdentityResolver (from AddAtProtoIdentity) as on main, and otherwise the OAuthClient's own CachingDidResolver. HandleResolutionTimeout and AllowPrivateNetworks still come from IdentityResolverOptions.
    • So a bare IDidResolver registered for other purposes no longer reaches the fresh issuer check.
    • Test: a custom IDidResolver without AddAtProtoIdentity leaves no IIdentityResolver in the container, and the login's resolver is a CachingDidResolver.
    • Docs: oauth.md states the InvalidateAsync requirement for a replacement resolver; the migration guide and CHANGELOG say "reads AddAtProtoIdentity()'s options".
  3. a9525d9 — the Verifier exception type, plus coverage.
    • An invalid o.Verifier again stops the host with InvalidOperationException, as on main, so no CHANGELOG bullet is needed.
    • Theory: ClockSkew (with the scheme's TimeProvider 70 s ahead), MaxTokenLifetime and AllowedKeyIds each refuse what the defaults accept.
  4. f070e3e — notifyWrite wiring. With the endpoint resolved from the container and a DI TimeProvider 70 s ahead, the default space skew (5 s) refuses and a 30 s SpaceServerOptions.ClockSkew accepts. Mutation-checked: dropping either the clock or the skew fails the test.
  5. f8f1b8e — endpoint move. ResolveServiceEndpoint is back to main's field-by-field read.
    • An unrelated malformed entry no longer keeps a password session on the entryway.
    • The first #atproto_pds entry of another type is passed over, as on main.
    • Two tests added. No CHANGELOG bullet, since the behaviour is main's.
  6. e998616, 2cbb5ab — stale docs. The migration guide no longer names AtProtoOAuthServerOptions.AllowPrivateNetworks, and the AtProtoClient / OAuthClient XML docs say the session is the result's .Session.
  7. 5128b17 — ISpaceCredentialIssuer migration. The migration guide and CHANGELOG now say plainly that credential signing is no longer pluggable, and why:
    • every SDK signer takes an in-process AtProtoKey;
    • the interface had no implementation or test;
    • ISpaceAccountSigner is not a substitute.
  8. 5140121 — notifier log cause. SpaceServiceCall.ResolveAsync lets the FormatException through again.
    • The notifier logs it as a failed delivery, with the exception.
    • The managing-app check wraps it in InvalidOperationException, as on main.
    • Test: a subscriber whose #atproto_space_host is not https logs one warning carrying a FormatException.

After the fixes: dotnet build --no-incremental gives 0 warnings. Unit tests, run twice: 5,012 passed, 0 failed, 5 skipped.

Deliberately left

  • Pass D items:
    • the verify-with-refresh helper; the refetch-once code in ServiceAuthVerifier, SpaceSyncer and SpaceDidResolution is untouched;
    • the bounded reader [12];
    • the caches [16];
    • DidDocument key getters.
  • SpaceCredentialIssuer.IssueAsync stays async (a Task.FromResult) to avoid more API churn.
  • Core CA2007 pragmas in XrpcClient, RepoClient, SyncClient, TapClient, RepoFetchers and SpaceSyncer are left alone: they belong to other passes, or to Pass D's SpaceSyncer bounded-copy area.
  • Test-only discovery members: AuthorizationServerDiscovery.ResolveFromIdentifierAsync and the two Fetch*MetadataAsync methods stay (now internal), because the SSRF and fetch-policy security tests use them.
  • WithSessionStore(factory), ITapEventHandler/OnUnreadableEvent, AtProtoServiceAuthOptions and RequireLexiconMethod never shipped in 0.6. They get Breaking bullets as instructed, but their migration entries are phrased for preview users.

🤖 Generated with Claude Code

Part of #180 (pass B: auth, hosting and Spaces). Less code and surface in the auth, hosting and Spaces slice, with the same security behaviour. Items follow the plan's numbering. ## What changed **Verification plumbing** - **[1]** `SpaceServiceAuthVerifier` is folded into `NotifyWriteEndpoint` (`VerifyCallerAsync`). The endpoint builds a `ServiceAuthVerifier` from the space server's resolver, replay store, `ClockSkew` and `MaxSingleUseTokenLifetime`, and maps `ServiceAuthException` to `NotAuthorized`. `[AuthenticatesItself]` and the `iss == repo` check are unchanged. - **[11] + A-Q5** - `AtProtoServiceAuthOptions.Verifier` is a `ServiceAuthVerifierOptions` and replaces the scheme's copies of `AllowedKeyIds`, `ClockSkew` and `MaxTokenLifetime`. - The two classes share one internal `Validate()`. - `RequireLexiconMethod` is gone; `lxm` is always required. - **[5]** Small duplicates now live in one place each: - one Authorization-header parser (`AuthorizationHeader`) for the scheme handler, `notifyWrite` and `SpaceRequestAuthenticator`; - one `Jwt.RequireTokenId` for the jti check and message in service auth, DPoP and `SpaceTokens`; - `Jwt.EncodeHeader` with a nullable `typ`, which replaces `OAuthClientKey.EncodeHeader`; - `ReadJwtExpiry` uses `TryGetNumericDate`; - `ResolveServiceEndpoint` was moved onto `DidDocument` and reverted after review (see Review fixes, item 5); - one `NoLease`; - one `SweepSchedule` for the in-memory and EF replay stores; - one `SpaceServiceCall` (resolve the endpoint, sign, send) for `SpaceWriteNotifier` and `SimpleSpaceManagingAppClient`. **Public surface** - **[3]** Removed or made internal: - `DPoPProof` keeps only `KeyThumbprint`. - `DPoPProofValidator.ProofType` and `ProofAlgorithm`, `ClaimsSpaceCallerResolver.DidClaimType`, `SpaceTokens.TryParse`, `SpaceTokens.Verify(type, jwt, …)`, `SpaceToken.ToSpaceUri()`, `services.AddAtProtoPdsAdmin(PdsAdminOptions)` with `CopyOptions`, and `WithSessionStore(factory)` are removed. - `SimpleSpaceAuthorityStore.Inner` and `SpaceServerOptions.BuildRequestUri` are internal. - `OAuthClient.CompleteAuthorizationAsync` now returns the `OAuthAuthorizationResult` (session + app state), replacing `CompleteAuthorizationWithAppStateAsync`. - **[6] + A-Q2** Hosted OAuth: - `AtProtoOAuthServerOptions.HttpClient`, `HttpClientTimeout`, `HandleResolutionTimeout` and `AllowPrivateNetworks` are removed. - `WithOAuth()` calls `AddAtProtoIdentity()` and builds its named client from `IdentityResolverOptions.AllowPrivateNetworks`: policy handler, no redirects, no proxy, UA, 30 s. - The `AtProtoOAuthService` constructor is internal, without its owned-client branch or the duplicate `ClientKeys` check. `WithOAuth` validation still refuses keys without metadata at startup. - `AuthorizationServerDiscovery` and `OAuthClient.Discovery` are internal. - **[10]** One internal `InMemoryDefaultsWarning` replaces `InMemorySessionStoreWarning`, `InMemorySpaceStoreWarning`, `InMemorySessionStoreChoice` and `SpaceServerOptions.WarnOnInMemoryStores`. - It warns only about an instance that a fallback registration created, so a store the app registers itself (the in-memory one included) is its choice. - `AddAtProtoServiceAuth()` now warns as well. It used to fall back to the in-process replay store silently. - **[19]** `VerifiedCredentialCacheCapacity`, `ClientMetadataCacheLifetime` and `MaxClientMetadataBytes` are internal. Tests still set them. - **[17]** `IdentityAction` and `AtProtoScopes.Identity(attr, action)` are removed. - **[18]** `ISpaceCredentialIssuer` is removed; the exchange takes `SpaceCredentialIssuer`. - **A-Q7** `MapTapWebhook` keeps the delegate overload (the docs' main example). `ITapEventHandler`, `MapTapWebhook<THandler>`, `OnUnreadableEvent` and `TapUnreadableEvent` go. An unreadable delivery is still answered 200, with a logged warning. - **A-Q8** `ClaimsSpaceCallerResolver` still accepts any scheme's `did`. It now documents why: no stored session or credential is used, so a verified caller acts only as itself. - **[15]** Aspire.Hosting: `WithPlcUrl`, `WithCrawlers`, `WithInviteCodeRequired`, `WithBlobUploadLimit` and `WithReportService` are one generic method each over `AtProtoPdsContainerResourceBase`. Each resource names its own environment variables (`SharedSettingNames`). **Mechanics** - **[8]** The 27 CA2007 pragma pairs in the EF stores, plus 2 in `ATProtoNet.Server`, become `var ctx = …; await using var ctxScope = ctx.ConfigureAwait(false);`. - **[14]** `XrpcQueryScratch` (thread-static pool) is gone. The query binder allocates a 256-byte `ArrayBufferWriter` and a `Utf8JsonWriter` per bind, which is noise next to the request pipeline. No benchmark was run. - **[2]** Docs: - `SpaceServerOptions` and `AtProtoOAuthServerOptions` are cut to summary + default. - Each rationale is kept once and referenced: per-process replay store (`IJtiReplayStore`), no retrying handler (`IAtProtoBuilder.HttpClient`), account signer. - Class remarks that retold the method body are dropped: `ServiceAuthVerifier` checks, DPoP six-checks list. **Tests** - **[4]** The JWS encoding cases are pinned once in `JwtTests`: - padded segments (new theory); - `AAAAA`/non-JSON/non-object rows per segment; - the standard alphabet (already there). - Each verifier keeps one "malformed → its own error" theory: service auth (`BadJwt`, existing), notifyWrite (`NotAuthorized`), DPoP (`NotAuthorized`), delegation (`InvalidDelegationToken`, new), attestation (`InvalidClientAttestation`, new), `SpaceTokens.Parse` (`SpaceTokenException`). - **[13]** `DPoPProofGeneratorTests` drops the tests `DPoPInteropTests` pins through the validator: thumbprint shape/consistency, header shape, unique jti, ath presence, export/import ×2. The nonce tests merge into one. - **[9]** Seven private `HttpMessageHandler` stubs are replaced by `TestSupport/HttpStub`. The warning tests use the shared `CapturingLoggerProvider`. ## Where each security property is still tested | Property | Tests | | --- | --- | | SSRF-safe named clients, no redirects | `AtProtoBuilderTests.WithOAuth_TheLoginSendsThroughItsNamedClient` (no redirect, no proxy, `ConnectCallback`, UA, 30 s); `…TheIdentityOptionsAllowPrivateNetworks_LiftsTheAddressCheck`; `AtProtoOAuthServiceHttpClientTests.WithoutAnHttpClient_TheOAuthClientsOwnKeepsTheIdentityFetchPolicy`; `MetadataFetchPolicyTests` (unchanged) | | Bounded reads | `HttpSpaceClientMetadataResolverTests` (cap, now internal, still set by the test); `TapWebhookTests` 413 cases | | Login-CSRF binding, local-only return URLs | `AtProtoOAuthCallbackBindingTests`, `CookieRelayTests` (unchanged, retargeted to the internal ctor) | | Issuer verification with fresh resolution | `OAuthCallbackHandleVerificationTests`, `AuthorizationServerMetadataValidationTests` (unchanged apart from `var (session, _)`) | | Replay retention ≥ acceptance window | `ServiceAuthVerifierTests.…ReplayedAfterExpiryButWithinTheSkew_IsStillRefused`; `DPoPProofValidatorTests.…ReplayedAtTheInstantItAgesOut_IsRefused`; `DPoPInteropTests.RfcResourceRequestProof_Validates` (now asserts the RFC jti is spent until iat + ProofLifetime); delegation/attestation `…WithinTheSkew` | | jti spent last | `ServiceAuthVerifierTests.…ForgedToken_DoesNotSpendTheGenuineTokensJti`, `…RefusedForItsAudience_DoesNotSpendItsJti`; `NotifyWriteServiceAuthTests.VerifyCallerAsync_SameTokenTwice_IsRefusedTheSecondTime` | | `[AuthenticatesItself]` marker | `ServiceAuthWithSpacesTests` (unchanged) | | Refresh coordinator lease | `AtProtoBuilderTests.WithOAuth_AndTheClientFactory_ShareOneRefreshCoordinator`, `SessionRaceTests`, the coordinator test in `AtProtoOAuthCallbackBindingTests` | | notifyWrite `iss == repo` | `SpaceServerEndpointTests.NotifyWrite_SignedByAServiceAtTheWritersPdsOrigin_IsRefused` | ### Where the removed duplicate tests' behaviours are pinned - **`SpaceServiceAuthVerifierTests`** (21 tests): 5 wrapper-specific tests remain as `NotifyWriteServiceAuthTests`: - each audience form + P-256; - no Bearer; - malformed → `NotAuthorized` with the `ServiceAuthException` cause; - replay; - the `MaxSingleUseTokenLifetime` ceiling. The rest map to `ServiceAuthVerifierTests`: - time claims out of range (rows `iat long.MaxValue` and `iat -62135596801` added there); - missing jti/lxm/iat; - kid; iss fragment; not a DID; - expired; future; ceiling; - audience; wrong key; malformed; high-S/out-of-range S. - JWS padding, base64url and non-object cases are in `JwtTests`. - The `SpaceIdentityRefreshTests` service-auth refresh case is `ServiceAuthVerifierTests.VerifyAsync_KeyRotatedSinceTheCachedDocument_VerifiesAgainstTheRefreshedOne`. - **DPoP JWS cases**: `JwtTests` + `DPoPProofValidatorTests.ValidateAsync_MalformedProof_IsNotAuthorized`. The JWK-coordinate tests stay: they cover JWK decoding, not JWS decoding. - **`SpaceTokensTests` TryParse and Verify-overload tests**: retargeted to `Parse` (throws) and `Verify(Parse(…))`. The duplicate "already parsed" and "by jwt" variants are merged. - **`RequireLexiconMethod` tests**: removed with the knob. `VerifyAsync_NoLxm_IsRefused` pins the new always-required rule. The delegation-token-is-not-service-auth test now binds no method instead of disabling the requirement. - **DPoP `Method`/`Uri`/`TokenId` asserts**: - htu normalization is now read from the proof's own claims (`TestJws.DecodeJson`); - the RFC jti through the replay store; - htm/htu are enforced by the validator in the same tests. ## Breaking changes (all in CHANGELOG `Breaking changes` and `docs/migrating-to-0.7.md`) - `SpaceServiceAuthVerifier` removed → `ServiceAuthVerifier` / `AddAtProtoServiceAuth()`. - `AtProtoServiceAuthOptions.{AllowedKeyIds,ClockSkew,MaxTokenLifetime}` → `o.Verifier.…`; `RequireLexiconMethod` removed. - `CompleteAuthorizationAsync` returns `OAuthAuthorizationResult` → `var (session, _) = …`; `CompleteAuthorizationWithAppStateAsync` removed. - Test-only members listed under [3] → `AtProtoClaimTypes.Did`, `Verify(Parse(…))`, `SpaceUri.Parse(token.Subject)`. - `AddAtProtoPdsAdmin(PdsAdminOptions)` → `(url, password)` + `Configure<PdsAdminOptions>`; `WithSessionStore(factory)` → `WithSessionStore<T>()`. - Hosted OAuth knobs → `AddAtProtoIdentity(o => …)` and `AddHttpClient(AtProtoOAuthExtensions.HttpClientName)`; `AtProtoOAuthService` ctor internal → resolve from DI. - `AuthorizationServerDiscovery` / `OAuthClient.Discovery` internal → `IIdentityResolver`. - `WarnOnInMemoryStores`, `MaxClientMetadataBytes` removed → register `InMemoryJtiReplayStore` yourself to keep it without the warning. - `IdentityAction` → `Identity(attr)`. - `ISpaceCredentialIssuer` → `SpaceCredentialIssuer`. - `ITapEventHandler`, `MapTapWebhook<T>`, `OnUnreadableEvent` → the delegate overload. - Aspire `With*` pairs → generic; recompile. `PublicAPI.Shipped.txt` is updated in `ATProtoNet`, `ATProtoNet.Server` and `ATProtoNet.Aspire.Hosting` (−87 lines). Every `Unshipped.txt` is still empty. ## Measurements (`loc.py origin/main HEAD`, this slice) | | before | after | Δ | | --- | --- | --- | --- | | src code (Auth, core Spaces, Server/*, EF, Aspire.Hosting) | 12,191 | 11,816 | −375 | | src XML doc | 7,251 | 6,799 | −452 | | tests/ATProtoNet.Tests | 49,571 | 48,898 | −673 | | PublicAPI txt | 9,983 | 9,896 | −87 | Overall diff: 92 files, +1,260 / −3,036. ## Verification - `dotnet build --no-incremental`: 0 warnings, 0 errors. This includes the compiled doc snippets, samples and integration tests. - `dotnet test tests/ATProtoNet.Tests`, run twice: 4,996 passed, 0 failed, 5 skipped (5,001). Before the change: 5,059 / 5 / 5,064. The difference is the duplicate tests removed above. ## Review fixes These commits sit on top of `d61fb30`. Each code fix has a regression test that fails without it. 1. **`6d92607` — the startup warning could crash the host (medium).** `InMemoryDefaultsWarning` resolved the stores from the root provider to look at them. - Under scope validation, a scoped `IJtiReplayStore` stopped the host. A service-auth-only app also built its session store for nothing. - The fix: the warning records the fallback descriptors it added (`InMemoryFallbacks`) and checks whether each is still the last registration for its type. It resolves no store. - Tests: a scoped replay store under `ValidateScopes` starts cleanly (spaces and service auth); service auth never resolves a throwing session store; a store registered before the fallback adds no warning. - CHANGELOG: `Fixed` bullet. `main`'s space warning had the same flaw. 2. **`e998616` — the login used the container's identity stack.** - The fix: `WithOAuth` no longer calls `AddAtProtoIdentity()`; it registers only the identity options. The login uses a registered `IIdentityResolver` (from `AddAtProtoIdentity`) as on `main`, and otherwise the `OAuthClient`'s own `CachingDidResolver`. `HandleResolutionTimeout` and `AllowPrivateNetworks` still come from `IdentityResolverOptions`. - So a bare `IDidResolver` registered for other purposes no longer reaches the fresh issuer check. - Test: a custom `IDidResolver` without `AddAtProtoIdentity` leaves no `IIdentityResolver` in the container, and the login's resolver is a `CachingDidResolver`. - Docs: `oauth.md` states the `InvalidateAsync` requirement for a replacement resolver; the migration guide and CHANGELOG say "reads `AddAtProtoIdentity()`'s options". 3. **`a9525d9` — the `Verifier` exception type, plus coverage.** - An invalid `o.Verifier` again stops the host with `InvalidOperationException`, as on `main`, so no CHANGELOG bullet is needed. - Theory: `ClockSkew` (with the scheme's `TimeProvider` 70 s ahead), `MaxTokenLifetime` and `AllowedKeyIds` each refuse what the defaults accept. 4. **`f070e3e` — notifyWrite wiring.** With the endpoint resolved from the container and a DI `TimeProvider` 70 s ahead, the default space skew (5 s) refuses and a 30 s `SpaceServerOptions.ClockSkew` accepts. Mutation-checked: dropping either the clock or the skew fails the test. 5. **`f8f1b8e` — endpoint move.** `ResolveServiceEndpoint` is back to `main`'s field-by-field read. - An unrelated malformed entry no longer keeps a password session on the entryway. - The first `#atproto_pds` entry of another type is passed over, as on `main`. - Two tests added. No CHANGELOG bullet, since the behaviour is `main`'s. 6. **`e998616`, `2cbb5ab` — stale docs.** The migration guide no longer names `AtProtoOAuthServerOptions.AllowPrivateNetworks`, and the `AtProtoClient` / `OAuthClient` XML docs say the session is the result's `.Session`. 7. **`5128b17` — `ISpaceCredentialIssuer` migration.** The migration guide and CHANGELOG now say plainly that credential signing is no longer pluggable, and why: - every SDK signer takes an in-process `AtProtoKey`; - the interface had no implementation or test; - `ISpaceAccountSigner` is not a substitute. 8. **`5140121` — notifier log cause.** `SpaceServiceCall.ResolveAsync` lets the `FormatException` through again. - The notifier logs it as a failed delivery, with the exception. - The managing-app check wraps it in `InvalidOperationException`, as on `main`. - Test: a subscriber whose `#atproto_space_host` is not https logs one warning carrying a `FormatException`. After the fixes: `dotnet build --no-incremental` gives 0 warnings. Unit tests, run twice: 5,012 passed, 0 failed, 5 skipped. ## Deliberately left - **Pass D items**: - the verify-with-refresh helper; the refetch-once code in `ServiceAuthVerifier`, `SpaceSyncer` and `SpaceDidResolution` is untouched; - the bounded reader [12]; - the caches [16]; - `DidDocument` key getters. - **`SpaceCredentialIssuer.IssueAsync`** stays async (a `Task.FromResult`) to avoid more API churn. - **Core CA2007 pragmas** in `XrpcClient`, `RepoClient`, `SyncClient`, `TapClient`, `RepoFetchers` and `SpaceSyncer` are left alone: they belong to other passes, or to Pass D's `SpaceSyncer` bounded-copy area. - **Test-only discovery members**: `AuthorizationServerDiscovery.ResolveFromIdentifierAsync` and the two `Fetch*MetadataAsync` methods stay (now internal), because the SSRF and fetch-policy security tests use them. - **`WithSessionStore(factory)`, `ITapEventHandler`/`OnUnreadableEvent`, `AtProtoServiceAuthOptions` and `RequireLexiconMethod`** never shipped in 0.6. They get Breaking bullets as instructed, but their migration entries are phrased for preview users. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
notifyWrite verifies its service auth with ServiceAuthVerifier directly;
the wrapper only forwarded to it. AtProtoServiceAuthOptions exposes a
ServiceAuthVerifierOptions instead of re-declaring its knobs, and
RequireLexiconMethod is gone since the spec requires lxm. One internal
Authorization-header parser serves the scheme handler, notifyWrite and
the space request authenticator.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DPoPProof keeps only its thumbprint; the validator's constants,
ClaimsSpaceCallerResolver.DidClaimType, SpaceTokens.TryParse, the
Verify(type, jwt) overload, SpaceToken.ToSpaceUri, the PdsAdminOptions
and factory registration overloads go, and SimpleSpaceAuthorityStore.Inner
and SpaceServerOptions.BuildRequestUri become internal: nothing but tests
used them. CompleteAuthorizationAsync returns the session with its app
state, replacing CompleteAuthorizationWithAppStateAsync. One Jwt helper
checks a token's jti for the three verifiers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
One NoLease, one replay-store sweep scheduler, one outbound space
service call (resolve the endpoint, sign, send), the JWT header encoder
for client assertions, TryGetNumericDate for a session's expiry and
DidDocument for its PDS. The EF stores dispose their contexts through
ConfigureAwait instead of 27 CA2007 pragma pairs, and the query binder
allocates its buffer per bind instead of keeping a thread-static pool.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WithOAuth registers AddAtProtoIdentity and always sends through its
SSRF-safe named client, so AtProtoOAuthServerOptions loses HttpClient,
HttpClientTimeout, HandleResolutionTimeout and AllowPrivateNetworks, and
the service's constructor becomes internal without its owned-client
branch or its second ClientKeys check. AuthorizationServerDiscovery and
OAuthClient.Discovery become internal: nothing outside OAuthClient used
them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The session-store and replay-store startup warnings become one hosted
service that warns only about an instance a fallback registration
created, so a store the application registers itself (the in-memory
one included) is its choice. That replaces WarnOnInMemoryStores and
InMemorySessionStoreChoice, and closes the gap where
AddAtProtoServiceAuth fell back to the in-process replay store without
a word. The credential and client-metadata cache bounds and the
metadata size cap become internal: nothing configures them but tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IdentityAction and Identity(attr, action) were already obsolete: the
permission spec dropped the parameter and servers reject it.
ISpaceCredentialIssuer had one implementation and no test double; the
credential exchange takes SpaceCredentialIssuer. MapTapWebhook keeps
its delegate overload, and an unreadable delivery is always logged as a
warning instead of going to OnUnreadableEvent.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WithPlcUrl, WithCrawlers, WithInviteCodeRequired, WithBlobUploadLimit and
WithReportService had one copy per server that differed only in the
environment variable; each resource now names its variables, and one
generic method over AtProtoPdsContainerResourceBase sets them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The JWS encoding cases (padding, alphabet, segment count, non-JSON
segments) are pinned once in JwtTests; each verifier keeps one theory
that a malformed token is its own error. DPoPProofGeneratorTests drops
what DPoPInteropTests already pins end to end through the validator.
The private HttpMessageHandler stubs of the Spaces, health-check and
OAuth service tests use TestSupport/HttpStub.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs: each auth and replay-store rationale once (#180)
All checks were successful
CI / pds-integration (pull_request) Successful in 32s
Details
CI / build-and-test (pull_request) Successful in 1m4s
Details
d61fb30c67 
The per-process replay store, the no-retrying-handler rule and the
account signer are explained in one place and referenced from the rest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-27 12:47:29 +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-181/run-2026-09-27T12-47-29-329Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-27 12:47:33 +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-27T17:47:29.741Z) — 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-27T17:47:29.741Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f1d17fba8ffeXNVP2h9ediWtct -->
Grandiras commented 2026-09-27 12:47:37 +00:00
Author
Owner
Copy link

@claude /review, please. This is debloat pass B of #180, covering auth, hosting and Spaces. It is a refactor that should leave the security behaviour identical. The description has a table showing where each security property is still tested, and where each removed duplicate test's behaviour is now pinned. The diff will be truncated, so please read these commits in full (git diff origin/main...HEAD), in particular:

  • the notifyWrite fold-in, which now calls ServiceAuthVerifier directly;
  • AtProtoServiceAuthOptions.Verifier;
  • the shared Authorization-header parser and jti check;
  • the CompleteAuthorizationAsync merge;
  • WithOAuth building its named client from IdentityResolverOptions, now that the AtProtoOAuthService constructor is internal;
  • the unified in-memory warning.
    Focus on any security check that got weaker, and on any behaviour a removed test pinned that is no longer pinned. The public API removals are listed with migration notes.
@claude /review, please. This is debloat pass B of #180, covering auth, hosting and Spaces. It is a refactor that should leave the security behaviour identical. The description has a table showing where each security property is still tested, and where each removed duplicate test's behaviour is now pinned. The diff will be truncated, so please read these commits in full (`git diff origin/main...HEAD`), in particular: - the `notifyWrite` fold-in, which now calls `ServiceAuthVerifier` directly; - `AtProtoServiceAuthOptions.Verifier`; - the shared Authorization-header parser and jti check; - the `CompleteAuthorizationAsync` merge; - `WithOAuth` building its named client from `IdentityResolverOptions`, now that the `AtProtoOAuthService` constructor is internal; - the unified in-memory warning. Focus on any security check that got weaker, and on any behaviour a removed test pinned that is no longer pinned. The public API removals are listed with migration notes.
claude-bot commented 2026-09-27 12:47:40 +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/633); 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-181/run-633.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-27 12:51:27 +00:00
Dismissed
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a large, mechanical debloat pass, and the parts I spot-checked against the target branch (origin/debloat-b-auth) hold up well.

Security-critical paths verified directly against the branch content (not just the diff):

  • NotifyWriteEndpoint.VerifyCallerAsync in SpaceAuthorityEndpoints.cs: the folded ServiceAuthVerifier is built once from the injected resolver/replay store/ClockSkew/MaxSingleUseTokenLifetime, the accepted-audience set correctly includes the space authority's bare DID, its host audience, and (when distinct) the service DID, ServiceAuthException is correctly mapped to SpaceErrors.NotAuthorized, and the critical caller.Issuer != repo writer check is untouched and still runs after verification.
  • ServiceAuthVerifier/ServiceAuthVerifierOptions/AtProtoServiceAuthOptions.Verifier: the grouped options, shared Validate(), and the always-required lxm behavior (ReadMethod throwing BadJwtLexiconMethod when method is non-null and the token names none) all match the PR's description; jti is still spent only after every other check passes.
  • SweepSchedule: the extracted scheduler is a faithful, correct generalization of the previous ad hoc SweepIfDue/Interlocked logic in EfCoreJtiReplayStore, now shared with InMemoryJtiReplayStore with identical semantics (not awaited, not tied to the triggering request's cancellation, CAS-guarded so only one sweep starts).
  • Aspire hosting: the new SharedSettingNames env-var names for both AtProtoPdsContainerResource (PDS_DID_PLC_URL, PDS_CRAWLERS, PDS_INVITE_REQUIRED, PDS_BLOB_UPLOAD_LIMIT, PDS_REPORT_SERVICE_URL, PDS_REPORT_SERVICE_DID) and AtProtoTranquilPdsContainerResource (PLC_DIRECTORY_URL, CRAWLERS, INVITE_CODE_REQUIRED, MAX_BLOB_SIZE, REPORT_SERVICE_URL, REPORT_SERVICE_DID) exactly match the environment variables the removed per-resource extension methods used to write, so the generic replacements are behavior-preserving; PublicAPI.Shipped.txt changes for both Aspire.Hosting classes line up with the removed/added signatures.
  • The hosted-OAuth HttpClient/identity wiring in AtProtoOAuthExtensions.WithOAuth: the named client still gets the identity fetch policy, a fixed 30s timeout and the SDK UA; the single AtProtoOAuthService singleton still ends up holding one lazily-built OAuthClient for its lifetime, so moving from PolicyHttpClientFactory + CreateClient on first use to a captured HttpClient resolved once at DI-singleton-construction time is not a behavioral regression.

CHANGELOG/migration-guide edits I checked (PdsAdmin overload removal, CompleteAuthorizationAsync tuple return, ISpaceServiceAuthVerifier removal, Aspire generic helpers, Tap webhook delegate-only mapping) accurately describe the corresponding code changes. I did not find correctness or security regressions in the portions I could inspect (the diff itself is truncated partway through, and I supplemented with direct reads of the head-branch files for the highest-risk pieces). Given the scope, I'd still suggest normal CI/build verification stands as the final gate, but I have no blocking findings.

⏱ 3m 29s · 38 turns · tokens: 132.5k in / 14.0k out (+4470.4k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, mechanical debloat pass, and the parts I spot-checked against the target branch (`origin/debloat-b-auth`) hold up well. **Security-critical paths verified directly against the branch content (not just the diff):** - `NotifyWriteEndpoint.VerifyCallerAsync` in `SpaceAuthorityEndpoints.cs`: the folded `ServiceAuthVerifier` is built once from the injected resolver/replay store/`ClockSkew`/`MaxSingleUseTokenLifetime`, the accepted-audience set correctly includes the space authority's bare DID, its host audience, and (when distinct) the service DID, `ServiceAuthException` is correctly mapped to `SpaceErrors.NotAuthorized`, and the critical `caller.Issuer != repo` writer check is untouched and still runs after verification. - `ServiceAuthVerifier`/`ServiceAuthVerifierOptions`/`AtProtoServiceAuthOptions.Verifier`: the grouped options, shared `Validate()`, and the always-required `lxm` behavior (`ReadMethod` throwing `BadJwtLexiconMethod` when `method` is non-null and the token names none) all match the PR's description; `jti` is still spent only after every other check passes. - `SweepSchedule`: the extracted scheduler is a faithful, correct generalization of the previous ad hoc `SweepIfDue`/`Interlocked` logic in `EfCoreJtiReplayStore`, now shared with `InMemoryJtiReplayStore` with identical semantics (not awaited, not tied to the triggering request's cancellation, CAS-guarded so only one sweep starts). - Aspire hosting: the new `SharedSettingNames` env-var names for both `AtProtoPdsContainerResource` (`PDS_DID_PLC_URL`, `PDS_CRAWLERS`, `PDS_INVITE_REQUIRED`, `PDS_BLOB_UPLOAD_LIMIT`, `PDS_REPORT_SERVICE_URL`, `PDS_REPORT_SERVICE_DID`) and `AtProtoTranquilPdsContainerResource` (`PLC_DIRECTORY_URL`, `CRAWLERS`, `INVITE_CODE_REQUIRED`, `MAX_BLOB_SIZE`, `REPORT_SERVICE_URL`, `REPORT_SERVICE_DID`) exactly match the environment variables the removed per-resource extension methods used to write, so the generic replacements are behavior-preserving; `PublicAPI.Shipped.txt` changes for both Aspire.Hosting classes line up with the removed/added signatures. - The hosted-OAuth `HttpClient`/identity wiring in `AtProtoOAuthExtensions.WithOAuth`: the named client still gets the identity fetch policy, a fixed 30s timeout and the SDK UA; the single `AtProtoOAuthService` singleton still ends up holding one lazily-built `OAuthClient` for its lifetime, so moving from `PolicyHttpClientFactory` + `CreateClient` on first use to a captured `HttpClient` resolved once at DI-singleton-construction time is not a behavioral regression. CHANGELOG/migration-guide edits I checked (PdsAdmin overload removal, `CompleteAuthorizationAsync` tuple return, `ISpaceServiceAuthVerifier` removal, Aspire generic helpers, Tap webhook delegate-only mapping) accurately describe the corresponding code changes. I did not find correctness or security regressions in the portions I could inspect (the diff itself is truncated partway through, and I supplemented with direct reads of the head-branch files for the highest-risk pieces). Given the scope, I'd still suggest normal CI/build verification stands as the final gate, but I have no blocking findings. --- ⏱ 3m 29s · 38 turns · tokens: 132.5k in / 14.0k out (+4470.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-181/run-633.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:d61fb30c6728af0f1a99c478b6a911ecdb4430c0 -->
opencode-bot commented 2026-09-27 15:57:21 +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-181/run-2026-09-27T15-57-20-613Z.md) — updating every ~5 minutes while the fix runs.
opencode-bot commented 2026-09-27 15:57:22 +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-27T20:54:08.259Z) — 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-27T20:54:08.259Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f1c6a2cddffeCCIfGdR3nk6AHM -->
Grandiras commented 2026-09-27 16:00:21 +00:00
Author
Owner
Copy link

@claude /review, please re-review. I rebased the branch onto main after #183 merged. The conflicts were in src/ATProtoNet/PublicAPI.Shipped.txt only; I resolved them by applying each commit's own line delta onto main's baseline. There are 8 new commits (0b845b7..ab92d69) that fix an independent adversarial review. See "Review fixes" in the description:

  • The in-memory-defaults warning reads service registrations instead of resolving stores, so a scoped IJtiReplayStore no longer crashes host start.
  • The hosted login keeps its own identity resolver unless AddAtProtoIdentity registered one, so a bare custom IDidResolver can't reach the fresh issuer check.
  • The DID document's PDS entry is read field by field again.
  • An invalid o.Verifier throws InvalidOperationException again, and new tests pin that the verifier options, clock and skew reach the handlers.
  • Docs, migration guide and logging fixes.
    Please read these commits in full.
@claude /review, please re-review. I rebased the branch onto main after #183 merged. The conflicts were in `src/ATProtoNet/PublicAPI.Shipped.txt` only; I resolved them by applying each commit's own line delta onto main's baseline. There are 8 new commits (0b845b7..ab92d69) that fix an independent adversarial review. See "Review fixes" in the description: - The in-memory-defaults warning reads service registrations instead of resolving stores, so a scoped `IJtiReplayStore` no longer crashes host start. - The hosted login keeps its own identity resolver unless `AddAtProtoIdentity` registered one, so a bare custom `IDidResolver` can't reach the fresh issuer check. - The DID document's PDS entry is read field by field again. - An invalid `o.Verifier` throws `InvalidOperationException` again, and new tests pin that the verifier options, clock and skew reach the handlers. - Docs, migration guide and logging fixes. Please read these commits in full.
Grandiras force-pushed debloat-b-auth from d61fb30c67
All checks were successful
CI / pds-integration (pull_request) Successful in 32s
Details
CI / build-and-test (pull_request) Successful in 1m4s
Details
to ab92d69ac7
Some checks failed
CI / pds-integration (pull_request) Successful in 31s
Details
CI / build-and-test (pull_request) Failing after 59s
Details
2026-09-27 16:00:21 +00:00 Compare
claude-bot commented 2026-09-27 16:00:24 +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/636); 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-181/run-636.md) — updating every ~5 minutes while the task runs.
opencode-bot commented 2026-09-27 16:01:23 +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-181/run-2026-09-27T16-01-23-203Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-27 16:01:26 +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-27T21:01:23.667Z) — 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-27T21:01:23.667Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f1c6676dcffeRAbichy4vBIKsA -->
claude-reviewer approved these changes 2026-09-27 16:01:41 +00:00
Dismissed
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

Re-review of the eight review-fix commits (6d92607/7157c68-equivalent … 5140121/ab92d69-equivalent, mapped onto this branch's actual commits e87a5b5..ab92d69) layered on top of the previously-approved base. I checked each fix against the issue it claims to resolve, and against its regression test:

  • In-memory defaults warning (7157c68): now records the fallback ServiceDescriptors it added and checks whether each is still the last registration for its service type, instead of resolving instances from the root provider. This correctly avoids building a scoped store under ValidateScopes and avoids constructing an unused session store in a service-auth-only app. New tests cover a scoped replay store under scope validation, a service-auth-only host never resolving a throwing session store, and a pre-registered store suppressing the hosted-service registration entirely. Logic checked and correct.
  • Hosted OAuth identity resolver (2608f34): WithOAuth() no longer calls AddAtProtoIdentity() (which would have composed any incidentally-registered IDidResolver into the login's fresh-issuer check); it now only registers IdentityResolverOptions, and AtProtoOAuthService takes IIdentityResolver? (nullable) plus the options object. New test confirms a bare IDidResolver registered for other purposes is not what the login's OAuthClient.Discovery resolves through, and that a CachingDidResolver is used instead. Correct.
  • Verifier validation exception type (8ab9651): AtProtoServiceAuthOptions.Validate() now wraps ServiceAuthVerifierOptions.Validate()'s ArgumentException into InvalidOperationException, matching the scheme's other startup failures. New theory test pins ClockSkew/MaxTokenLifetime/AllowedKeyIds each independently refusing what the defaults accept, plus a dedicated test for the exception type. Correct.
  • notifyWrite verifier wiring (01c421d): new test resolves NotifyWriteEndpoint from a real container with a TimeProvider 70s ahead and asserts the default 5s SpaceServerOptions.ClockSkew refuses while a 30s skew accepts — verifying the endpoint's inline-built ServiceAuthVerifierOptions actually reads the container's registered clock and configured skew rather than silently defaulting. Correct.
  • ResolveServiceEndpoint reverted to field-by-field JSON reads (0b845b7): replaces the DidDocument full-deserialize approach (which failed the whole lookup, and thus kept the session on the entryway, whenever any unrelated document entry didn't parse) with a targeted read of only id and the matching #atproto_pds/{did}#atproto_pds service entry of type AtprotoPersonalDataServer. New tests cover a malformed unrelated verification-method entry not blocking the move, and a first same-id-but-wrong-type entry being passed over in favor of the correctly-typed one later in the array. Matches main's behavior as claimed. Correct.
  • SpaceServiceCall.ResolveAsync no longer swallows FormatException (ab92d69): the malformed-endpoint case now propagates out of SpaceServiceCall.ResolveAsync instead of collapsing to null; SpaceWriteNotifier.DeliverAsync's existing catch-all (IsDeliveryFailure) picks it up and logs it with the exception as cause, and SimpleSpaceManagingAppClient explicitly catches FormatException to rewrap as InvalidOperationException, matching main's behavior. New SpaceWriteNotifierTests test confirms a malformed #atproto_space_host entry is skipped (still returns 0 delivered) with a single warning log carrying the FormatException. Correct.
  • Docs-only fix (038c8d1, ISpaceCredentialIssuer migration wording) is a straightforward clarification, low risk.

All the concrete issues raised in the earlier review round appear resolved, each with a regression test that specifically targets the failure mode described in the PR's "Review fixes" section. I did not find any new correctness or security regressions introduced by these fix commits. No blocking findings; approving.

⏱ 1m 8s · 23 turns · tokens: 114.8k in / 5.6k out (+2774.0k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review of the eight review-fix commits (`6d92607`/`7157c68`-equivalent … `5140121`/`ab92d69`-equivalent, mapped onto this branch's actual commits `e87a5b5`..`ab92d69`) layered on top of the previously-approved base. I checked each fix against the issue it claims to resolve, and against its regression test: - **In-memory defaults warning (`7157c68`)**: now records the fallback `ServiceDescriptor`s it added and checks whether each is still the last registration for its service type, instead of resolving instances from the root provider. This correctly avoids building a scoped store under `ValidateScopes` and avoids constructing an unused session store in a service-auth-only app. New tests cover a scoped replay store under scope validation, a service-auth-only host never resolving a throwing session store, and a pre-registered store suppressing the hosted-service registration entirely. Logic checked and correct. - **Hosted OAuth identity resolver (`2608f34`)**: `WithOAuth()` no longer calls `AddAtProtoIdentity()` (which would have composed any incidentally-registered `IDidResolver` into the login's fresh-issuer check); it now only registers `IdentityResolverOptions`, and `AtProtoOAuthService` takes `IIdentityResolver?` (nullable) plus the options object. New test confirms a bare `IDidResolver` registered for other purposes is not what the login's `OAuthClient.Discovery` resolves through, and that a `CachingDidResolver` is used instead. Correct. - **`Verifier` validation exception type (`8ab9651`)**: `AtProtoServiceAuthOptions.Validate()` now wraps `ServiceAuthVerifierOptions.Validate()`'s `ArgumentException` into `InvalidOperationException`, matching the scheme's other startup failures. New theory test pins `ClockSkew`/`MaxTokenLifetime`/`AllowedKeyIds` each independently refusing what the defaults accept, plus a dedicated test for the exception type. Correct. - **notifyWrite verifier wiring (`01c421d`)**: new test resolves `NotifyWriteEndpoint` from a real container with a `TimeProvider` 70s ahead and asserts the default 5s `SpaceServerOptions.ClockSkew` refuses while a 30s skew accepts — verifying the endpoint's inline-built `ServiceAuthVerifierOptions` actually reads the container's registered clock and configured skew rather than silently defaulting. Correct. - **`ResolveServiceEndpoint` reverted to field-by-field JSON reads (`0b845b7`)**: replaces the `DidDocument` full-deserialize approach (which failed the whole lookup, and thus kept the session on the entryway, whenever *any* unrelated document entry didn't parse) with a targeted read of only `id` and the matching `#atproto_pds`/`{did}#atproto_pds` service entry of type `AtprotoPersonalDataServer`. New tests cover a malformed unrelated verification-method entry not blocking the move, and a first same-id-but-wrong-type entry being passed over in favor of the correctly-typed one later in the array. Matches `main`'s behavior as claimed. Correct. - **`SpaceServiceCall.ResolveAsync` no longer swallows `FormatException` (`ab92d69`)**: the malformed-endpoint case now propagates out of `SpaceServiceCall.ResolveAsync` instead of collapsing to `null`; `SpaceWriteNotifier.DeliverAsync`'s existing catch-all (`IsDeliveryFailure`) picks it up and logs it with the exception as cause, and `SimpleSpaceManagingAppClient` explicitly catches `FormatException` to rewrap as `InvalidOperationException`, matching `main`'s behavior. New `SpaceWriteNotifierTests` test confirms a malformed `#atproto_space_host` entry is skipped (still returns `0` delivered) with a single warning log carrying the `FormatException`. Correct. - Docs-only fix (`038c8d1`, `ISpaceCredentialIssuer` migration wording) is a straightforward clarification, low risk. All the concrete issues raised in the earlier review round appear resolved, each with a regression test that specifically targets the failure mode described in the PR's "Review fixes" section. I did not find any new correctness or security regressions introduced by these fix commits. No blocking findings; approving. --- ⏱ 1m 8s · 23 turns · tokens: 114.8k in / 5.6k out (+2774.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-181/run-636.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:ab92d69ac79001d7d37dbef398092d019f73bd73 -->
It asserted that the host logged no warning at all. The service-auth
case calls AddAuthentication(), which brings Data Protection: on a
machine with no key ring yet, as in a fresh CI container, starting the
host creates one and warns that no XML encryptor is configured. The
test now looks only at InMemoryDefaultsWarning's own category; the
regression it pins (the host failing to start) is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test: the missing-ServiceDid host test builds under Development too (#180)
All checks were successful
CI / pds-integration (pull_request) Successful in 46s
Details
CI / build-and-test (pull_request) Successful in 1m32s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 8s
Details
40c0a890ac 
With DOTNET_ENVIRONMENT=Development the host validates every
registration on build, and the test's authority had no access policy,
so Build failed before the ServiceDid check it pins. Found while
stress-testing the CI failure; the test predates this PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Grandiras commented 2026-09-27 16:17:14 +00:00
Author
Owner
Copy link

@claude /review, please re-review. There are two new commits on top of ab92d69, which you approved:

  • 9427994: fixes the CI failure. InMemoryDefaultsWarningTests asserted that no warning at all was logged. The service-auth row brings in Data Protection, and on a fresh CI container Data Protection creates a key ring and logs "No XML encryptor configured". The assertion now filters on InMemoryDefaultsWarning's own category. I reproduced the failure with an empty HOME, and the product code is unchanged.
  • 40c0a89: fixes a test that failed under DOTNET_ENVIRONMENT=Development because it didn't register a space access policy.
    The full Release suite passed 12 times (fresh and real HOME, environment unset and Development).
@claude /review, please re-review. There are two new commits on top of ab92d69, which you approved: - **9427994: fixes the CI failure.** `InMemoryDefaultsWarningTests` asserted that no warning at all was logged. The service-auth row brings in Data Protection, and on a fresh CI container Data Protection creates a key ring and logs "No XML encryptor configured". The assertion now filters on `InMemoryDefaultsWarning`'s own category. I reproduced the failure with an empty `HOME`, and the product code is unchanged. - **40c0a89: fixes a test that failed under `DOTNET_ENVIRONMENT=Development`** because it didn't register a space access policy. The full Release suite passed 12 times (fresh and real `HOME`, environment unset and Development).
claude-bot commented 2026-09-27 16:17:18 +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/638); 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-181/run-638.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-27 16:18:27 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

Re-review: only two new commits landed since my last pass (9427994, 40c0a89), both test-only:

  • 9427994 narrows InMemoryDefaultsWarningTests' scoped-replay-store regression assertion from "no warning logged at all" to "no warning from InMemoryDefaultsWarning's own category", since AddAuthentication() pulls in Data Protection, which can log its own unrelated warning on a machine with no key ring yet (fresh CI container). The regression being pinned (host starting cleanly with a scoped replay store) is unchanged and still asserted.
  • 40c0a89 fixes a pre-existing (not introduced by this PR) test, AddSpaceAuthority_WithoutAServiceDid_StopsTheHostInOptionsValidation, by adding AddSimpleSpace<InMemorySimpleSpaceStore>() so the authority has an access policy and the host's container validation (which runs under DOTNET_ENVIRONMENT=Development) doesn't fail on a different, unrelated missing registration before reaching the ServiceDid check the test actually pins.

Both are narrowly scoped, correct, and touch no production code paths or security-relevant logic. Everything raised in my two earlier passes (the startup-warning root-provider crash, the hosted-login identity resolver leak, the Verifier exception type, notifyWrite wiring, the ResolveServiceEndpoint revert, and the SpaceServiceCall exception swallowing) remains fixed and covered by regression tests. No blocking issues found.

⏱ 0m 57s · 11 turns · tokens: 93.0k in / 3.4k out (+1167.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: only two new commits landed since my last pass (`9427994`, `40c0a89`), both test-only: - **`9427994`** narrows `InMemoryDefaultsWarningTests`' scoped-replay-store regression assertion from "no warning logged at all" to "no warning from `InMemoryDefaultsWarning`'s own category", since `AddAuthentication()` pulls in Data Protection, which can log its own unrelated warning on a machine with no key ring yet (fresh CI container). The regression being pinned (host starting cleanly with a scoped replay store) is unchanged and still asserted. - **`40c0a89`** fixes a pre-existing (not introduced by this PR) test, `AddSpaceAuthority_WithoutAServiceDid_StopsTheHostInOptionsValidation`, by adding `AddSimpleSpace<InMemorySimpleSpaceStore>()` so the authority has an access policy and the host's container validation (which runs under `DOTNET_ENVIRONMENT=Development`) doesn't fail on a different, unrelated missing registration before reaching the `ServiceDid` check the test actually pins. Both are narrowly scoped, correct, and touch no production code paths or security-relevant logic. Everything raised in my two earlier passes (the startup-warning root-provider crash, the hosted-login identity resolver leak, the `Verifier` exception type, notifyWrite wiring, the `ResolveServiceEndpoint` revert, and the `SpaceServiceCall` exception swallowing) remains fixed and covered by regression tests. No blocking issues found. --- ⏱ 0m 57s · 11 turns · tokens: 93.0k in / 3.4k out (+1167.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-181/run-638.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:40c0a890ac9879efbb47429cd0daa13fe36a8d6a -->
Grandiras deleted branch debloat-b-auth 2026-09-27 16:19:10 +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!181
No description provided.