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

fix(spaces)!: protocol hardening, performance and a smaller server surface #172

Merged
Grandiras merged 10 commits from issue-133-spaces into main 2026-09-26 20:54:08 +00:00
Conversation 12 Commits 10 Files changed 72 +4350 -1235
Grandiras commented 2026-09-26 16:41:08 +00:00
Owner
Copy link

Closes #133.

Brings Spaces in line with proposal 0016's September clarifications and the reference implementation (bluesky-social/atproto #5187 / #5496, packages/space and pds/src/auth-verifier.ts), adds per-account signing for space service auth, makes the hot read path cheap, and shrinks the public server surface. Includes the two scope additions from the issue comments.

Protocol hardening

  • DID-document rules (G14). SpaceAuthority.GetSigningKey / GetHostEndpoint use the strict TryGetVerificationKey / TryGetServiceEndpoint lookups from #120: an entry that is absent falls back to #atproto / #atproto_pds; one that is published but unusable throws FormatException (unknown key type, no or undecodable key material; a #atproto_space_host not of type AtprotoSpaceHost or without an absolute https endpoint). SpaceCredentialProvider.ResolveHostAsync reports it as SpaceCredentialException, SpaceServiceAuthVerifier.IsRepoHostAsync as a refusal, and SpaceWriteNotifier as an undelivered notification.
  • Token kid (G15). Decision: a missing kid is refused, as the reference's resolveSpaceKey refuses it (every conforming issuer, SpaceTokens.Create included, sends one). Delegation tokens: #atproto only (proposal: "MUST be #atproto"; the reference also tolerates #atproto_space — see Flagged). Credentials: #atproto_space or #atproto, with or without #, never DID-qualified; the named entry is resolved with no fallback (SpaceDidResolution.RequireKeyId / ResolveKeyAsync). The old no-kid → #atproto_space-then-#atproto path is gone from the server. SpaceServerOptions.CredentialKeyId docs now say an authority with a dedicated key must set it.
  • DPoP (G16). DPoPProofValidator pins ES256 (the reference's SIGNING_ALG) and refuses an ath on the credential exchange (the reference's "must be omitted when obtaining a credential"). A nonce claim is ignored (0016: no server nonces).
  • SpaceRepoCar.Verify (G17). Index keys must split on exactly one / into a valid NSID and record key — checked in DecodeIndex for every entry, so index-only CARs are covered; index CIDs must be DAG-CBOR; each record block must be a DAG-CBOR map (major type 5, definite length, DagCborDecoder.TryValidate).
  • Space-type declarations (G18). FromLexicon refuses a wildcard in collections with a message naming it; name has no length limit (the doc line was already fixed by #169).
  • Open unions (G22). SimpleSpaceUserPolicy / SimpleSpaceAppAccess are [AtProtoUnion] open unions with UnknownSimpleSpaceUserPolicy / UnknownSimpleSpaceAppAccess; SpaceWriteOp is [AtProtoUnion(Closed = true)] (the Lexicon marks writes closed). Object models (SpaceView, SpaceRepoView, SpaceRecordView, SpaceRepoOpEntry, SpaceWriteOpResult, SimpleSpaceMember, the policies, SignedSpaceCommit) derive from LexObject. createSpace / updateSpace answer UnsupportedPolicy / UnsupportedAppAccess for an unknown variant. The OpenUnionTests guard no longer exempts Spaces.
  • Closed-union registration (issue comment 2). Decision: reject. LexiconTypeRegistry.RegisterUnionVariant throws ArgumentException for a closed [AtProtoUnion] base, and the converter no longer consults the registry for a closed union. Tests in LexiconTypeRegistryTests and SpaceUnionTests.
  • Docs (G23). docs/spaces.md says sync.getBlob answers BlobNotFound for a space-only blob.
  • Fix found on the way. SpaceSyncer verified commits with SpaceAuthority.GetSigningKey, i.e. a #atproto_space key when the author published one; commits are signed with the repo key, so it now uses #atproto (the integration fixture's helper too).

Per-account signing (issue comment 1)

  • New ISpaceAccountSigner.GetSignerAsync(Did) → ServiceAuthGenerator? (owned by the signer). SpaceWriteNotifier signs NotifyWriteAsync as the writer, ForwardWriteAsync and NotifySpaceDeletedAsync as the space authority; SimpleSpaceManagingAppClient signs checkUserAccess as the authority. Fallback to the service ServiceAuthGenerator when no signer is registered, it returns null, it throws (logged), or it returns a generator for another DID. Skipped when the account already is the service DID. DI passes sp.GetService<ISpaceAccountSigner>().
  • Integration coverage: SpaceAccountSigningTests ([RequiresSpacesFact]) registers a did:plc in the test network's PLC with a key the test holds, then has an SDK SpaceWriteNotifier notify the reference authority: signed as the writer it joins the writer set; signed as the host service DID it is refused. Not run (no spaces-alpha network here); it compiles.

Performance

  • Verified-credential cache. SpaceCredentialVerifier keys verified credentials by SHA-256(jwt) (the same hash is the expected ath, computed once) until min(exp, now + DidCache.StaleAfter); bounded by SpaceServerOptions.VerifiedCredentialCacheCapacity (10,000; 0 disables; when full of live entries a newcomer is verified per request rather than evicting). Per request it still checks the requested space, exp, and the whole DPoP proof including its jti. A mismatched space is refused before any key resolution.
  • JWK import cache. JsonWebKeyVerifier imports a JWK once (the import validates the point, so an off-curve Y cannot pass as the same-X key), keeps it by thumbprint as a did:key, and verifies through the existing pooled DidKeyCache JWS path (high-S accepted as before).
  • Client metadata / JWKS cache per client_id in SpaceClientAttestationVerifier (ClientMetadataCacheLifetime, 5 min); a kid miss or failed signature refetches at most every 30 s per client.
  • SpaceCredentialProvider: per-space SemaphoreSlim gate with a lock-free hit path (single-flight per space; a forced renewal replaces only the credential it saw); replaced credentials are kept for live readers and disposed once expired. The caching identity resolver was already in place (#120).
  • SpaceSyncer.RecoverAsync pre-sizes from Content-Length (capped at 32 MiB so a host cannot force a large allocation) and verifies from MemoryStream.GetBuffer() without the ToArray() copy.
  • EF upserts. RecordWriteAsync, RegisterNotifyAsync, PutMemberAsync, MarkDeletedAsync try ExecuteUpdateAsync first and fall back to the insert path; UpdateSpaceAsync, DeleteSpaceAsync, RemoveMemberAsync, UnregisterNotifyAsync are single ExecuteUpdate/ExecuteDelete statements. The writer-set revision guard moved into SQL (TIDs are [2-7a-z], which every collation orders bytewise).
  • Replay sweeps off the request path in both InMemoryJtiReplayStore and EfCoreJtiReplayStore<T> (background Task.Run, internal LastSweep for tests).

Measured with a Stopwatch harness (SpaceCredentialVerifier.VerifyAsync on the same credential with a fresh DPoP proof per call, cached DID document, 20,000 ops after 2,000 warm-up, Release, one thread; before = main at ef506f1, run back to back):

Authority key Before After Allocated/op
K-256 296.9 µs 61.3 µs 13.2 KB → 7.6 KB
P-256 127.3 µs 59.5 µs 13.2 KB → 7.6 KB

What remains per request is the DPoP proof's own P-256 signature check and its JSON parsing.

Smaller surface

  • Endpoint handlers (all 19), SpaceRepoEndpointBase<T>, SimpleSpaceEndpointBase and the query-parameter types are internal, with primary constructors and no DI null checks; a RequireLiveSpaceAsync helper replaces the repeated state checks.
  • Removed ISpaceServiceAuthVerifier (one implementation, no fakes). Kept ISimpleSpaceManagingAppClient and ISpaceCallerResolver: both are stubbed in tests (test-seam value), per the issue's criterion.
  • The seven hand-written DI lambdas (DPoPProofValidator, the four verifiers, SpaceRequestAuthenticator, SimpleSpaceAccessPolicy) are TryAddSingleton<T>(); the verifiers take the space resolver via [FromKeyedServices(DidResolverKey)], so a registered TimeProvider now reaches them (test added).
  • Unused members removed: VerifiedDelegationToken.Token, VerifiedSpaceCredential.Token/AuthorityDid, VerifiedClientAttestation.Token, SpaceCredentialRequestAuth.Space, DPoPProof.AccessTokenHash/Nonce, public SpaceRequestAuthenticator.BuildRequestUri, InMemorySpaceAuthorityStore.MarkDeleted, and SpaceCredentialIssuer's ownsKey/IDisposable. (CachingSpaceDidDocumentResolver.Invalidate was already gone; VerifiedServiceAuth.Audience is now used by #166's claims and stays.)
  • InMemorySpaceStoreWarning trimmed to the replay store (as the issue says; the simplespace member-list notice is gone too — see Flagged).
  • Paging shared: SpacePaging.After (in-memory) and SpacePaging.Page (all four stores).
  • LengthLimitedStream was already replaced by ReadBoundedAsync, with size-cap tests, in #151 — nothing left to do.

Breaking changes (migration)

  • Endpoint handlers / *Parameters internal → register through AddSpaceAuthority / AddSpaceRepoHost / AddSimpleSpace; implement your own IXrpcEndpoint for custom routes.
  • ISpaceServiceAuthVerifier removed → use SpaceServiceAuthVerifier.
  • Verified-record members removed → auth.Delegation.Space, credential.Space.Authority, SpaceServerOptions.BuildRequestUri; deletion via the simplespace store or EfCoreSpaceAuthorityStore.MarkDeletedAsync.
  • Constructors: SpaceCredentialVerifier(resolver, validator, options?, timeProvider?); SpaceWriteNotifier(…, accountSigner?, logger?); SimpleSpaceManagingAppClient(…, accountSigner?, logger?); SpaceCredentialIssuer(key, options) → pass timeProvider:/logger: by name; dispose the issuer's key yourself.
  • Space/SimpleSpace models: open unions and LexObject (binary break) → add an Unknown… arm to policy switches.
  • RegisterUnionVariant on a closed union throws → remove the registration.
  • Behavioural (Security): missing/other kid, ES256K or exchange-ath DPoP proofs, malformed #atproto_space(_host) entries, invalid repo CARs are now refused → an authority signing with #atproto_space sets CredentialKeyId = "#atproto_space".
  • EF space and replay stores need a provider with ExecuteUpdate/ExecuteDelete (not EF InMemory) → test on SQLite in-memory (now under Breaking changes).
  • SpaceServiceAuthVerifier.IsRepoHostAsync removed; notifyWrite must be signed by the writer.

Verification

  • dotnet build (solution, --no-incremental): 0 warnings, 0 errors.
  • Unit tests after the review fixes and the rebase onto a5f2489: 4887 passed, 0 failed, 5 skipped (4892); the Spaces and replay-store subset ran 5 more times with no failures. The original commit had 4678 passed (4683). New/extended: SpaceAuthorityTests (malformed entries throw, absent entries fall back), SpaceTokenKeyIdTests (kid allow-list, no fallback, missing kid), DPoPProofValidatorTests (ES256K on a real secp256k1 key refused, exchange ath refused, off-curve JWK Y refused, cached import still verifies each signature), SpaceCredentialCacheTests (one resolution for repeated presentations; replayed proof, foreign key, wrong space and expiry still refused; re-verification past StaleAfter against a rotated key; cache off / full / failed-credential cases), SpaceRepoCarTests (two-slash, empty and missing-slash paths, index-only CAR, non-map record blocks incl. indefinite-length, raw-codec CID), SpaceScopeTests (wildcard refusal, 200-char name), SpaceUnionTests + LexiconTypeRegistryTests (unknown policy round-trips byte-for-byte, extension data, closed SpaceWriteOp, closed-union registration rejected), SpaceWriteNotifierTests / SimpleSpaceManagingAppClientTests (iss per call, fallbacks, token verifies against the account key), SpaceCredentialMintTests (single-flight per space, no cross-space blocking, superseded retention/disposal), SpaceCredentialProviderTests (malformed space host throws; published one preferred), SpaceSyncerTests (commit verified against #atproto when #atproto_space is published; capped pre-size), SpaceClientKeyCacheTests, SpaceServerRegistrationTests (TimeProvider reaches verifiers; signer reaches the notifier), JtiReplayStoreTests (consumption does not wait for the sweep), SimpleSpaceEndpointTests (unknown variant → UnsupportedPolicy/UnsupportedAppAccess).
  • Integration tests (SpaceAccountSigningTests) compile but were not run here.

Review fixes

Rebased onto origin/main a5f2489 (#171); the only conflicts were in CHANGELOG, where both sides were kept and every bullet stayed in its section. 0a00095 became 2cc2dce. Every fix commit's regression test was checked to fail with that commit's src/ change reverted.

  • 6f12df9 (items 1, 6, 8, 10a) — notifyWrite requires iss == repo, the reference rule; IsRepoHostAsync is removed. Item 1 has both options: AddSpaceAuthority(signingKey, serviceAuthKey?), and ISpaceAccountSigner is also consulted for the service's own DID. A dedicated #atproto_space credential key with neither fails at startup. InMemorySpaceAuthorityStore.MarkDeleted is restored. A malformed managing-app space-host entry is refused rather than returning 500.
  • 3f5cedf (items 2, 3) — Item 2: LRU eviction with a per-authority quota of capacity/4 (a foreign authority minting 50 credentials holds 2 of 8 slots), rather than caching only after the endpoint confirms. Item 3: every cache hit re-checks the key against the DID document currently served (with a reference-equality fast path), so a cached credential stops verifying exactly when a fresh one would. Benchmark unchanged, about 60 µs per op.
  • e6cb40b (item 4) — client metadata is fetched at most once per 30 s, on success or failure; failures are cached; one shared fetch per client, which no single waiter's cancellation stops.
  • 14b9e6c (item 7) — one shared mint task per space, so all waiters get the same credential or the same failure; minted under a provider-level token; store, retire and dispose share one lock, and Dispose cancels a mint in flight.
  • 10b178e (item 5) — reads the stored revision, compares it ordinally in C#, and advances with ExecuteUpdate … WHERE Rev == <read>. Equality is collation-safe; ordering is not. A missing row takes the insert path, and the loop gives up with DbUpdateConcurrencyException after 5 attempts. The test puts SQLite's Rev column under a REVERSED collation. Cost: one primary-key read plus one UPDATE (was one UPDATE).
  • 624758c (item 9) — a strict record validator, Spaces/SpaceRecordCbor.cs: CborReader in Canonical mode, plus only tag 42 over 0x00…, text keys only, and no floats or undefined. It lives in Spaces/, so Repo/DagCborDecoder (#134) is untouched. SpaceSyncer.MaxRepoSize (256 MiB) bounds getRepo both by the declared Content-Length and during the read.
  • 75638a2 (item 11) — the EF InMemory incompatibility is moved to Breaking changes, with the SQLite in-memory migration.
  • fbc8437 (item 12) — both replay-store sweep tests await LastSweep before the replay check, and include an expired row to prove the sweep ran. The other item-12 gaps are covered in the commits above; the IsRepoHostAsync gap is moot because that method is removed.
  • 5034dc0 (item 10b) — no behaviour change. The reference unregisterNotify removes any service once the credential checks pass (verified at 787a730f and on the alpha branch), and a credential carries no subscriber identity to match against. Documented on the endpoint and in docs/spaces.md: the only impact is latency (the sweep plus a re-register).

Flagged / deferred

  • Delegation kid: follows the proposal (#atproto only), one notch stricter than the reference (which also accepts #atproto_space); no known issuer sends that.
  • InMemorySpaceStoreWarning no longer mentions in-memory simplespace member lists (trimmed per the issue); the store's own docs still say so.
  • AddSpaceAuthority's fallback ServiceAuthGenerator signs with the credential key — resolved in the review fixes (item 1).
  • SpaceCredentialProvider.ResolveHostAsync resolves repo hosts through #atproto_space_host too (pre-existing), so a malformed space-host entry on a writer's document now fails that read rather than falling back.
  • Redis replay store untouched (going away in #136).

🤖 Generated with Claude Code

Closes #133. Brings Spaces in line with proposal 0016's September clarifications and the reference implementation (bluesky-social/atproto #5187 / #5496, `packages/space` and `pds/src/auth-verifier.ts`), adds per-account signing for space service auth, makes the hot read path cheap, and shrinks the public server surface. Includes the two scope additions from the issue comments. ## Protocol hardening - **DID-document rules (G14).** `SpaceAuthority.GetSigningKey` / `GetHostEndpoint` use the strict `TryGetVerificationKey` / `TryGetServiceEndpoint` lookups from #120: an entry that is *absent* falls back to `#atproto` / `#atproto_pds`; one that is *published but unusable* throws `FormatException` (unknown key type, no or undecodable key material; a `#atproto_space_host` not of type `AtprotoSpaceHost` or without an absolute **https** endpoint). `SpaceCredentialProvider.ResolveHostAsync` reports it as `SpaceCredentialException`, `SpaceServiceAuthVerifier.IsRepoHostAsync` as a refusal, and `SpaceWriteNotifier` as an undelivered notification. - **Token `kid` (G15).** Decision: a missing `kid` is **refused**, as the reference's `resolveSpaceKey` refuses it (every conforming issuer, `SpaceTokens.Create` included, sends one). Delegation tokens: `#atproto` only (proposal: "MUST be #atproto"; the reference also tolerates `#atproto_space` — see Flagged). Credentials: `#atproto_space` or `#atproto`, with or without `#`, never DID-qualified; the named entry is resolved with **no fallback** (`SpaceDidResolution.RequireKeyId` / `ResolveKeyAsync`). The old no-kid → `#atproto_space`-then-`#atproto` path is gone from the server. `SpaceServerOptions.CredentialKeyId` docs now say an authority with a dedicated key must set it. - **DPoP (G16).** `DPoPProofValidator` pins `ES256` (the reference's `SIGNING_ALG`) and refuses an `ath` on the credential exchange (the reference's "must be omitted when obtaining a credential"). A `nonce` claim is ignored (0016: no server nonces). - **`SpaceRepoCar.Verify` (G17).** Index keys must split on exactly one `/` into a valid NSID and record key — checked in `DecodeIndex` for every entry, so index-only CARs are covered; index CIDs must be DAG-CBOR; each record block must be a DAG-CBOR map (major type 5, definite length, `DagCborDecoder.TryValidate`). - **Space-type declarations (G18).** `FromLexicon` refuses a wildcard in `collections` with a message naming it; `name` has no length limit (the doc line was already fixed by #169). - **Open unions (G22).** `SimpleSpaceUserPolicy` / `SimpleSpaceAppAccess` are `[AtProtoUnion]` open unions with `UnknownSimpleSpaceUserPolicy` / `UnknownSimpleSpaceAppAccess`; `SpaceWriteOp` is `[AtProtoUnion(Closed = true)]` (the Lexicon marks `writes` closed). Object models (`SpaceView`, `SpaceRepoView`, `SpaceRecordView`, `SpaceRepoOpEntry`, `SpaceWriteOpResult`, `SimpleSpaceMember`, the policies, `SignedSpaceCommit`) derive from `LexObject`. `createSpace` / `updateSpace` answer `UnsupportedPolicy` / `UnsupportedAppAccess` for an unknown variant. The `OpenUnionTests` guard no longer exempts Spaces. - **Closed-union registration (issue comment 2).** Decision: **reject**. `LexiconTypeRegistry.RegisterUnionVariant` throws `ArgumentException` for a closed `[AtProtoUnion]` base, and the converter no longer consults the registry for a closed union. Tests in `LexiconTypeRegistryTests` and `SpaceUnionTests`. - **Docs (G23).** `docs/spaces.md` says `sync.getBlob` answers `BlobNotFound` for a space-only blob. - **Fix found on the way.** `SpaceSyncer` verified commits with `SpaceAuthority.GetSigningKey`, i.e. a `#atproto_space` key when the author published one; commits are signed with the repo key, so it now uses `#atproto` (the integration fixture's helper too). ## Per-account signing (issue comment 1) - New `ISpaceAccountSigner.GetSignerAsync(Did)` → `ServiceAuthGenerator?` (owned by the signer). `SpaceWriteNotifier` signs `NotifyWriteAsync` as the **writer**, `ForwardWriteAsync` and `NotifySpaceDeletedAsync` as the **space authority**; `SimpleSpaceManagingAppClient` signs `checkUserAccess` as the **authority**. Fallback to the service `ServiceAuthGenerator` when no signer is registered, it returns `null`, it throws (logged), or it returns a generator for another DID. Skipped when the account already is the service DID. DI passes `sp.GetService<ISpaceAccountSigner>()`. - Integration coverage: `SpaceAccountSigningTests` (`[RequiresSpacesFact]`) registers a did:plc in the test network's PLC with a key the test holds, then has an SDK `SpaceWriteNotifier` notify the reference authority: signed as the writer it joins the writer set; signed as the host service DID it is refused. **Not run** (no spaces-alpha network here); it compiles. ## Performance - **Verified-credential cache.** `SpaceCredentialVerifier` keys verified credentials by `SHA-256(jwt)` (the same hash is the expected `ath`, computed once) until `min(exp, now + DidCache.StaleAfter)`; bounded by `SpaceServerOptions.VerifiedCredentialCacheCapacity` (10,000; 0 disables; when full of live entries a newcomer is verified per request rather than evicting). Per request it still checks the requested space, `exp`, and the whole DPoP proof including its `jti`. A mismatched space is refused before any key resolution. - **JWK import cache.** `JsonWebKeyVerifier` imports a JWK once (the import validates the point, so an off-curve Y cannot pass as the same-X key), keeps it by thumbprint as a did:key, and verifies through the existing pooled `DidKeyCache` JWS path (high-S accepted as before). - **Client metadata / JWKS cache** per `client_id` in `SpaceClientAttestationVerifier` (`ClientMetadataCacheLifetime`, 5 min); a kid miss or failed signature refetches at most every 30 s per client. - **`SpaceCredentialProvider`:** per-space `SemaphoreSlim` gate with a lock-free hit path (single-flight per space; a forced renewal replaces only the credential it saw); replaced credentials are kept for live readers and disposed once expired. The caching identity resolver was already in place (#120). - **`SpaceSyncer.RecoverAsync`** pre-sizes from `Content-Length` (capped at 32 MiB so a host cannot force a large allocation) and verifies from `MemoryStream.GetBuffer()` without the `ToArray()` copy. - **EF upserts.** `RecordWriteAsync`, `RegisterNotifyAsync`, `PutMemberAsync`, `MarkDeletedAsync` try `ExecuteUpdateAsync` first and fall back to the insert path; `UpdateSpaceAsync`, `DeleteSpaceAsync`, `RemoveMemberAsync`, `UnregisterNotifyAsync` are single `ExecuteUpdate`/`ExecuteDelete` statements. The writer-set revision guard moved into SQL (TIDs are `[2-7a-z]`, which every collation orders bytewise). - **Replay sweeps off the request path** in both `InMemoryJtiReplayStore` and `EfCoreJtiReplayStore<T>` (background `Task.Run`, `internal LastSweep` for tests). Measured with a Stopwatch harness (`SpaceCredentialVerifier.VerifyAsync` on the same credential with a fresh DPoP proof per call, cached DID document, 20,000 ops after 2,000 warm-up, Release, one thread; before = main at `ef506f1`, run back to back): | Authority key | Before | After | Allocated/op | |---|---|---|---| | K-256 | 296.9 µs | 61.3 µs | 13.2 KB → 7.6 KB | | P-256 | 127.3 µs | 59.5 µs | 13.2 KB → 7.6 KB | What remains per request is the DPoP proof's own P-256 signature check and its JSON parsing. ## Smaller surface - Endpoint handlers (all 19), `SpaceRepoEndpointBase<T>`, `SimpleSpaceEndpointBase` and the query-parameter types are `internal`, with primary constructors and no DI null checks; a `RequireLiveSpaceAsync` helper replaces the repeated state checks. - Removed `ISpaceServiceAuthVerifier` (one implementation, no fakes). Kept `ISimpleSpaceManagingAppClient` and `ISpaceCallerResolver`: both are stubbed in tests (test-seam value), per the issue's criterion. - The seven hand-written DI lambdas (`DPoPProofValidator`, the four verifiers, `SpaceRequestAuthenticator`, `SimpleSpaceAccessPolicy`) are `TryAddSingleton<T>()`; the verifiers take the space resolver via `[FromKeyedServices(DidResolverKey)]`, so a registered `TimeProvider` now reaches them (test added). - Unused members removed: `VerifiedDelegationToken.Token`, `VerifiedSpaceCredential.Token`/`AuthorityDid`, `VerifiedClientAttestation.Token`, `SpaceCredentialRequestAuth.Space`, `DPoPProof.AccessTokenHash`/`Nonce`, public `SpaceRequestAuthenticator.BuildRequestUri`, `InMemorySpaceAuthorityStore.MarkDeleted`, and `SpaceCredentialIssuer`'s `ownsKey`/`IDisposable`. (`CachingSpaceDidDocumentResolver.Invalidate` was already gone; `VerifiedServiceAuth.Audience` is now used by #166's claims and stays.) - `InMemorySpaceStoreWarning` trimmed to the replay store (as the issue says; the simplespace member-list notice is gone too — see Flagged). - Paging shared: `SpacePaging.After` (in-memory) and `SpacePaging.Page` (all four stores). - `LengthLimitedStream` was already replaced by `ReadBoundedAsync`, with size-cap tests, in #151 — nothing left to do. ## Breaking changes (migration) - Endpoint handlers / `*Parameters` internal → register through `AddSpaceAuthority` / `AddSpaceRepoHost` / `AddSimpleSpace`; implement your own `IXrpcEndpoint` for custom routes. - `ISpaceServiceAuthVerifier` removed → use `SpaceServiceAuthVerifier`. - Verified-record members removed → `auth.Delegation.Space`, `credential.Space.Authority`, `SpaceServerOptions.BuildRequestUri`; deletion via the simplespace store or `EfCoreSpaceAuthorityStore.MarkDeletedAsync`. - Constructors: `SpaceCredentialVerifier(resolver, validator, options?, timeProvider?)`; `SpaceWriteNotifier(…, accountSigner?, logger?)`; `SimpleSpaceManagingAppClient(…, accountSigner?, logger?)`; `SpaceCredentialIssuer(key, options)` → pass `timeProvider:`/`logger:` by name; dispose the issuer's key yourself. - Space/SimpleSpace models: open unions and `LexObject` (binary break) → add an `Unknown…` arm to policy switches. - `RegisterUnionVariant` on a closed union throws → remove the registration. - Behavioural (Security): missing/other `kid`, ES256K or exchange-`ath` DPoP proofs, malformed `#atproto_space(_host)` entries, invalid repo CARs are now refused → an authority signing with `#atproto_space` sets `CredentialKeyId = "#atproto_space"`. - EF space and replay stores need a provider with `ExecuteUpdate`/`ExecuteDelete` (not EF InMemory) → test on SQLite in-memory (now under Breaking changes). - `SpaceServiceAuthVerifier.IsRepoHostAsync` removed; `notifyWrite` must be signed by the writer. ## Verification - `dotnet build` (solution, `--no-incremental`): **0 warnings, 0 errors**. - Unit tests after the review fixes and the rebase onto `a5f2489`: **4887 passed, 0 failed, 5 skipped** (4892); the Spaces and replay-store subset ran 5 more times with no failures. The original commit had 4678 passed (4683). New/extended: `SpaceAuthorityTests` (malformed entries throw, absent entries fall back), `SpaceTokenKeyIdTests` (kid allow-list, no fallback, missing kid), `DPoPProofValidatorTests` (ES256K on a real secp256k1 key refused, exchange `ath` refused, off-curve JWK Y refused, cached import still verifies each signature), `SpaceCredentialCacheTests` (one resolution for repeated presentations; replayed proof, foreign key, wrong space and expiry still refused; re-verification past `StaleAfter` against a rotated key; cache off / full / failed-credential cases), `SpaceRepoCarTests` (two-slash, empty and missing-slash paths, index-only CAR, non-map record blocks incl. indefinite-length, raw-codec CID), `SpaceScopeTests` (wildcard refusal, 200-char name), `SpaceUnionTests` + `LexiconTypeRegistryTests` (unknown policy round-trips byte-for-byte, extension data, closed `SpaceWriteOp`, closed-union registration rejected), `SpaceWriteNotifierTests` / `SimpleSpaceManagingAppClientTests` (iss per call, fallbacks, token verifies against the account key), `SpaceCredentialMintTests` (single-flight per space, no cross-space blocking, superseded retention/disposal), `SpaceCredentialProviderTests` (malformed space host throws; published one preferred), `SpaceSyncerTests` (commit verified against `#atproto` when `#atproto_space` is published; capped pre-size), `SpaceClientKeyCacheTests`, `SpaceServerRegistrationTests` (TimeProvider reaches verifiers; signer reaches the notifier), `JtiReplayStoreTests` (consumption does not wait for the sweep), `SimpleSpaceEndpointTests` (unknown variant → `UnsupportedPolicy`/`UnsupportedAppAccess`). - Integration tests (`SpaceAccountSigningTests`) compile but were **not run** here. ## Review fixes Rebased onto `origin/main` `a5f2489` (#171); the only conflicts were in CHANGELOG, where both sides were kept and every bullet stayed in its section. `0a00095` became `2cc2dce`. Every `fix` commit's regression test was checked to fail with that commit's `src/` change reverted. - `6f12df9` (items 1, 6, 8, 10a) — `notifyWrite` requires `iss == repo`, the reference rule; `IsRepoHostAsync` is removed. Item 1 has both options: `AddSpaceAuthority(signingKey, serviceAuthKey?)`, and `ISpaceAccountSigner` is also consulted for the service's own DID. A dedicated `#atproto_space` credential key with neither fails at startup. `InMemorySpaceAuthorityStore.MarkDeleted` is restored. A malformed managing-app space-host entry is refused rather than returning 500. - `3f5cedf` (items 2, 3) — Item 2: LRU eviction with a per-authority quota of capacity/4 (a foreign authority minting 50 credentials holds 2 of 8 slots), rather than caching only after the endpoint confirms. Item 3: every cache hit re-checks the key against the DID document currently served (with a reference-equality fast path), so a cached credential stops verifying exactly when a fresh one would. Benchmark unchanged, about 60 µs per op. - `e6cb40b` (item 4) — client metadata is fetched at most once per 30 s, on success or failure; failures are cached; one shared fetch per client, which no single waiter's cancellation stops. - `14b9e6c` (item 7) — one shared mint task per space, so all waiters get the same credential or the same failure; minted under a provider-level token; store, retire and dispose share one lock, and `Dispose` cancels a mint in flight. - `10b178e` (item 5) — reads the stored revision, compares it ordinally in C#, and advances with `ExecuteUpdate … WHERE Rev == <read>`. Equality is collation-safe; ordering is not. A missing row takes the insert path, and the loop gives up with `DbUpdateConcurrencyException` after 5 attempts. The test puts SQLite's `Rev` column under a `REVERSED` collation. Cost: one primary-key read plus one `UPDATE` (was one `UPDATE`). - `624758c` (item 9) — a strict record validator, `Spaces/SpaceRecordCbor.cs`: `CborReader` in Canonical mode, plus only tag 42 over `0x00…`, text keys only, and no floats or `undefined`. It lives in `Spaces/`, so `Repo/DagCborDecoder` (#134) is untouched. `SpaceSyncer.MaxRepoSize` (256 MiB) bounds `getRepo` both by the declared `Content-Length` and during the read. - `75638a2` (item 11) — the EF InMemory incompatibility is moved to Breaking changes, with the SQLite in-memory migration. - `fbc8437` (item 12) — both replay-store sweep tests await `LastSweep` before the replay check, and include an expired row to prove the sweep ran. The other item-12 gaps are covered in the commits above; the `IsRepoHostAsync` gap is moot because that method is removed. - `5034dc0` (item 10b) — no behaviour change. The reference `unregisterNotify` removes any service once the credential checks pass (verified at `787a730f` and on the alpha branch), and a credential carries no subscriber identity to match against. Documented on the endpoint and in `docs/spaces.md`: the only impact is latency (the sweep plus a re-register). ## Flagged / deferred - Delegation `kid`: follows the proposal (`#atproto` only), one notch stricter than the reference (which also accepts `#atproto_space`); no known issuer sends that. - `InMemorySpaceStoreWarning` no longer mentions in-memory `simplespace` member lists (trimmed per the issue); the store's own docs still say so. - ~~`AddSpaceAuthority`'s fallback `ServiceAuthGenerator` signs with the credential key~~ — resolved in the review fixes (item 1). - `SpaceCredentialProvider.ResolveHostAsync` resolves repo hosts through `#atproto_space_host` too (pre-existing), so a malformed space-host entry on a *writer's* document now fails that read rather than falling back. - Redis replay store untouched (going away in #136). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(spaces)!: protocol hardening, performance and a smaller server surface (closes #133)
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 58s
Details
0a000952b9 
Brings Spaces in line with proposal 0016's 2026-09 clarifications and the
reference implementation (bluesky-social/atproto#5187/#5496): a malformed
fallback; space tokens must carry a kid (#atproto for delegation,
fallback; DPoP proofs are ES256 only and the credential exchange's proof
carries no ath; SpaceRepoCar.Verify checks paths, record maps and index
codecs as the reference does; space-type collections refuse wildcards.

Per-account signing (ISpaceAccountSigner) lets an SDK repo host notify a
reference authority as the writer, and an SDK authority ask a managing
app as the authority, which the reference requires. The simplespace
unions are open and the Space models keep unknown fields, following the

Performance: verified credentials are cached by hash (a K-256 read drops
from ~290 us to ~60 us of verification CPU), DPoP JWK imports and client
JWKS are cached, SpaceCredentialProvider mints per space and releases
replaced credentials, EF space stores update first, replay sweeps run in
the background, and SpaceSyncer recovery avoids a full-CAR copy.

Surface: endpoint handlers and query-parameter types are internal,
ISpaceServiceAuthVerifier is gone, DI activates the verifiers by type,
unused members are removed and the paging code is shared.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-26 16:41:10 +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-172/run-2026-09-26T16-41-10-290Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-26 16:41: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-26T21:41:10.767Z) — 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-26T21:41:10.767Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f216866f5ffeGgubTLPR4GdGTc -->
Grandiras commented 2026-09-26 16:41:21 +00:00
Author
Owner
Copy link

@claude /review — security-relevant Spaces hardening (permissioned data, proposal 0016). The diff will be truncated, so please read these in full from the branch: src/ATProtoNet.Server/Spaces/SpaceCredentialVerifier.cs (the new credential cache keyed by SHA-256(jwt): the per-request exp/subject/jti checks must still run on a cache hit, and cache lifetime ≤ min(exp, DID-cache TTL)), SpaceDidResolution.cs (a malformed entry is an error, an absent one falls back), JsonWebKeyVerifier.cs + SpaceClientAttestationVerifier.cs (client-key/JWKS caching per client_id, DPoP ES256 only, no ath on the credential exchange), SpaceServiceAuthVerifier.cs, ISpaceAccountSigner.cs and its use in SpaceWriteNotifier/SimpleSpaceAccessPolicy, src/ATProtoNet/Spaces/SpaceCredentialProvider.cs (per-space single-flight, disposal of superseded credentials), SpaceRepoCar verification, the EF stores' update-first upserts, and the LexiconTypeRegistry/AtProtoUnionConverter closed-union change. One point to judge in particular: an authority that signs credentials with a dedicated #atproto_space key signs its fallback service auth with that same key under the default #atproto kid. Is that a bug that should be refused or warned about at startup? Focus on cache-induced auth bypasses (revocation, key rotation, a token reused across subjects), token and kid confusion, and DoS bounds on the caches.

@claude /review — security-relevant Spaces hardening (permissioned data, proposal 0016). The diff will be truncated, so please read these in full from the branch: `src/ATProtoNet.Server/Spaces/SpaceCredentialVerifier.cs` (the new credential cache keyed by SHA-256(jwt): the per-request exp/subject/jti checks must still run on a cache hit, and cache lifetime ≤ min(exp, DID-cache TTL)), `SpaceDidResolution.cs` (a malformed entry is an error, an absent one falls back), `JsonWebKeyVerifier.cs` + `SpaceClientAttestationVerifier.cs` (client-key/JWKS caching per client_id, DPoP ES256 only, no ath on the credential exchange), `SpaceServiceAuthVerifier.cs`, `ISpaceAccountSigner.cs` and its use in `SpaceWriteNotifier`/`SimpleSpaceAccessPolicy`, `src/ATProtoNet/Spaces/SpaceCredentialProvider.cs` (per-space single-flight, disposal of superseded credentials), `SpaceRepoCar` verification, the EF stores' update-first upserts, and the `LexiconTypeRegistry`/`AtProtoUnionConverter` closed-union change. One point to judge in particular: an authority that signs credentials with a dedicated `#atproto_space` key signs its fallback service auth with that same key under the default `#atproto` kid. Is that a bug that should be refused or warned about at startup? Focus on cache-induced auth bypasses (revocation, key rotation, a token reused across subjects), token and kid confusion, and DoS bounds on the caches.
claude-bot commented 2026-09-26 16:41:25 +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/619); 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-172/run-619.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-26 16:44:22 +00:00
Dismissed
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a large, carefully-documented hardening/perf pass on the Spaces implementation, and the visible portion of the diff (the PR diff was truncated, so some referenced files — SpaceDidResolution, SpacePaging, SpaceRepoCar, LexiconTypeRegistry, the open-union plumbing — weren't shown and couldn't be fully verified) is consistent, internally coherent, and matches the PR description closely.

What I checked closely and found sound:

  • DPoPProofValidator: ES256 pinning happens before signature verification; the exchange-proof ath prohibition is enforced both ways (missing when expected, and rejected when present but not expected). The ValidateAsync → ValidateWithHashAsync split correctly hoists DPoP.AccessTokenHash computation for the credential-cache case.
  • SpaceCredentialVerifier: the cache-hit path still re-checks the requested space, re-validates expiry against the live clock, and still runs the full DPoP proof/jti check every request — matching the stated "only the signature check is skipped" design. The cache TTL is correctly capped at min(exp, now + StaleAfter), so a rotated authority key stops verifying cached credentials no later than fresh ones. The capacity-full behavior (verify-per-request rather than evict) matches the docs.
  • JsonWebKeyVerifier: algorithm-vs-curve pinning happens before any cache lookup or import, so an unusable key never pollutes the cache; the thumbprint (which covers x/y) makes cache-key collisions cryptographically implausible, and the off-curve-Y protection is preserved because the ECDSA import (which validates the point) happens before the did:key is cached.
  • SpaceClientAttestationVerifier: the cached-keys → refusal → forced refetch (rate-limited to 30s) → recheck flow correctly avoids both stale-key lockout and refetch-per-forged-attestation.
  • ISpaceAccountSigner / SpaceAccountSigning.ChooseAsync: fallback-to-service-key behavior on null signer, wrong-DID signer, exceptions, and same-DID short-circuit all match the description.
  • EF Core store changes: the update-first/insert-fallback pattern preserves prior semantics (no-op when the row doesn't exist, in the update-only endpoints; guarded revision-ordering preserved in RecordWriteAsync via the same <=/>= boundary check duplicated between the SQL WHERE and the fallback MutateAsync path).

One thing worth a sanity check post-merge rather than blocking: RecordWriteAsync's ExecuteUpdateAsync guard uses string.Compare(e.Rev, revValue) <= 0 inside the LINQ predicate. Two-argument string.Compare/CompareTo against a constant is a documented, provider-supported EF Core translation pattern on relational providers (confirmed via search), so this should translate rather than throw or fall back to client evaluation — but it's exactly the kind of thing where provider/collation quirks (Postgres vs SQL Server vs SQLite) bite in practice, so it's worth confirming the EF integration tests actually exercise this against a real relational provider (not just that they compile) if they haven't already run in CI.

No correctness or security issues found in the reviewed portion.

⏱ 2m 46s · 4 turns · tokens: 90.3k in / 15.1k out (+349.0k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, carefully-documented hardening/perf pass on the Spaces implementation, and the visible portion of the diff (the PR diff was truncated, so some referenced files — `SpaceDidResolution`, `SpacePaging`, `SpaceRepoCar`, `LexiconTypeRegistry`, the open-union plumbing — weren't shown and couldn't be fully verified) is consistent, internally coherent, and matches the PR description closely. What I checked closely and found sound: - **`DPoPProofValidator`**: ES256 pinning happens before signature verification; the exchange-proof `ath` prohibition is enforced both ways (missing when expected, and rejected when present but not expected). The `ValidateAsync` → `ValidateWithHashAsync` split correctly hoists `DPoP.AccessTokenHash` computation for the credential-cache case. - **`SpaceCredentialVerifier`**: the cache-hit path still re-checks the requested space, re-validates expiry against the live clock, and still runs the full DPoP proof/`jti` check every request — matching the stated "only the signature check is skipped" design. The cache TTL is correctly capped at `min(exp, now + StaleAfter)`, so a rotated authority key stops verifying cached credentials no later than fresh ones. The capacity-full behavior (verify-per-request rather than evict) matches the docs. - **`JsonWebKeyVerifier`**: algorithm-vs-curve pinning happens before any cache lookup or import, so an unusable key never pollutes the cache; the thumbprint (which covers `x`/`y`) makes cache-key collisions cryptographically implausible, and the off-curve-Y protection is preserved because the ECDSA import (which validates the point) happens before the did:key is cached. - **`SpaceClientAttestationVerifier`**: the cached-keys → refusal → forced refetch (rate-limited to 30s) → recheck flow correctly avoids both stale-key lockout and refetch-per-forged-attestation. - **`ISpaceAccountSigner` / `SpaceAccountSigning.ChooseAsync`**: fallback-to-service-key behavior on null signer, wrong-DID signer, exceptions, and same-DID short-circuit all match the description. - **EF Core store changes**: the update-first/insert-fallback pattern preserves prior semantics (no-op when the row doesn't exist, in the update-only endpoints; guarded revision-ordering preserved in `RecordWriteAsync` via the same `<=`/`>=` boundary check duplicated between the SQL `WHERE` and the fallback `MutateAsync` path). One thing worth a sanity check post-merge rather than blocking: `RecordWriteAsync`'s `ExecuteUpdateAsync` guard uses `string.Compare(e.Rev, revValue) <= 0` inside the LINQ predicate. Two-argument `string.Compare`/`CompareTo` against a constant is a documented, provider-supported EF Core translation pattern on relational providers (confirmed via search), so this should translate rather than throw or fall back to client evaluation — but it's exactly the kind of thing where provider/collation quirks (Postgres vs SQL Server vs SQLite) bite in practice, so it's worth confirming the EF integration tests actually exercise this against a real relational provider (not just that they compile) if they haven't already run in CI. No correctness or security issues found in the reviewed portion. --- ⏱ 2m 46s · 4 turns · tokens: 90.3k in / 15.1k out (+349.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-172/run-619.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:0a000952b9096988619923cf28ac2d63c1082e8d -->
opencode-bot commented 2026-09-26 16:57:28 +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-172/run-2026-09-26T16-57-27-062Z.md) — updating every ~5 minutes while the fix runs.
opencode-bot commented 2026-09-26 16:57:30 +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-26T21:57:27.503Z) — 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-26T21:57:27.503Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f21597f01ffeIPyE6JNuL4THQ6 -->
Grandiras force-pushed issue-133-spaces from 0a000952b9
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 58s
Details
to 5034dc04de
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 55s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s
Details
2026-09-26 20:49:25 +00:00 Compare
Grandiras commented 2026-09-26 20:49:25 +00:00
Author
Owner
Copy link

@claude /review — re-review please. The branch was rebased onto main (after #171, CHANGELOG-only conflicts). Nine new commits (6f12df9..5034dc0) on top of the approved patch fix the adversarial review's findings; see "Review fixes" in the description. Please read those commits in full (git diff 2cc2dce..5034dc0), especially:

  • the dedicated-key service auth: the new serviceAuthKey, ISpaceAccountSigner also consulted for the service's own DID, and the startup failure;
  • notifyWrite requiring iss == repo, with IsRepoHostAsync removed;
  • the credential cache: LRU, a per-authority quota, and entries bounded by the key's document age;
  • the client-metadata refetch rate limit and single-flight;
  • SpaceCredentialProvider failure sharing and dispose races;
  • the ordinal revision guard (ExecuteUpdate … WHERE Rev == read, with retries);
  • SpaceRecordCbor strict validation and the bounded getRepo.
@claude /review — re-review please. The branch was rebased onto main (after #171, CHANGELOG-only conflicts). Nine new commits (6f12df9..5034dc0) on top of the approved patch fix the adversarial review's findings; see "Review fixes" in the description. Please read those commits in full (`git diff 2cc2dce..5034dc0`), especially: - the dedicated-key service auth: the new `serviceAuthKey`, `ISpaceAccountSigner` also consulted for the service's own DID, and the startup failure; - `notifyWrite` requiring `iss == repo`, with `IsRepoHostAsync` removed; - the credential cache: LRU, a per-authority quota, and entries bounded by the key's document age; - the client-metadata refetch rate limit and single-flight; - `SpaceCredentialProvider` failure sharing and dispose races; - the ordinal revision guard (`ExecuteUpdate … WHERE Rev == read`, with retries); - `SpaceRecordCbor` strict validation and the bounded `getRepo`.
claude-bot commented 2026-09-26 20:49:28 +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/623); 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-172/run-623.md) — updating every ~5 minutes while the task runs.
opencode-bot commented 2026-09-26 20:50:26 +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-172/run-2026-09-26T20-50-26-110Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-26 20:50:29 +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-27T01:47:27.404Z) — 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-27T01:47:27.404Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f2084330cffeaNVnxnBc1i1Esq -->
claude-reviewer approved these changes 2026-09-26 20:53:15 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

Re-review after the "Review fixes" commits. The one non-blocking note from my last pass — whether RecordWriteAsync's revision comparison would translate/behave correctly across relational providers — is now fully resolved: EfCoreSpaceAuthorityStore.RecordWriteAsync reads the current Rev first, compares it ordinally in C#, and advances with a plain ExecuteUpdate(... WHERE Rev == <read>), retrying up to 5 times and throwing DbUpdateConcurrencyException if it can't win. No SQL-side string comparison remains, so provider/collation quirks are no longer a concern.

I checked out the PR branch directly (the diff in the description was truncated) and read the full, current versions of the files most central to this pass's security claims:

  • JsonWebKeyVerifier: algorithm/curve pinning happens before the thumbprint cache lookup; the cache key is the RFC 7638 thumbprint (DPoP.Thumbprint, same computation DPoPProofValidator uses), so an off-curve-Y key can never collide with a valid same-X key in the cache — the thumbprint covers x/y and a collision would require breaking SHA-256. The Import validation (which catches the off-curve point) still runs before anything is cached.
  • SpaceCredentialVerifier: cache-hit path still re-checks the requested space, re-validates the authority's current key against the cached DidKey/Document, and always re-runs the full DPoP proof/jti check. LRU + per-authority quota eviction (Remember/RemoveLocked) is bookkept consistently.
  • SpaceClientAttestationVerifier: cached-key → refusal → rate-limited (30s) forced refetch → recheck flow is implemented as described, with shared in-flight fetches via Lazy<Task<...>> keyed per client.
  • SpaceAuthority.GetSigningKey/GetHostEndpoint: correctly distinguish Absent (falls back to #atproto/#atproto_pds) from Malformed (throws FormatException), built on DidDocument.TryGetVerificationKey/TryGetServiceEndpoint, which already report Malformed for a wrong service type or non-absolute/wrong-scheme endpoint.
  • SpaceRepoCar: index-key parsing now requires exactly one / (ParsePath), index CIDs are required to be DAG-CBOR-coded, and SpaceRecordCbor.TryValidateRecord (new) enforces canonical DAG-CBOR (definite lengths, text-only keys, no floats, tag 42-only over a 0x00-prefixed byte string) via CborReader in Canonical mode.
  • LexiconTypeRegistry.RegisterUnionVariant now throws for a closed union base, and AtProtoUnionConverter<TBase> no longer consults the registry when shape.Closed — matches the described G22/closed-union-registration behavior exactly.
  • NotifyWriteEndpoint: now requires caller.Issuer == repo (the IsRepoHostAsync/document-based check is gone), matching the reference authority's rule and the item 1/6/8/10a fix description.
  • SpaceCredentialProvider: single-flight mint per space via Lazy<Task<...>>, double-check-after-await to skip a redundant mint, and superseded-credential retention/disposal under one lock all behave as documented; Dispose cancels an in-flight mint and disposes a credential that completes anyway.

Everything I inspected is internally consistent, matches the (very detailed) PR description, and no new correctness or security issues turned up. Nothing here blocks merging.

⏱ 3m 32s · 23 turns · tokens: 137.3k in / 12.4k out (+2793.6k cache-read)

✅ **Verdict: approve** — no blocking issues found. Re-review after the "Review fixes" commits. The one non-blocking note from my last pass — whether `RecordWriteAsync`'s revision comparison would translate/behave correctly across relational providers — is now fully resolved: `EfCoreSpaceAuthorityStore.RecordWriteAsync` reads the current `Rev` first, compares it ordinally in C#, and advances with a plain `ExecuteUpdate(... WHERE Rev == <read>)`, retrying up to 5 times and throwing `DbUpdateConcurrencyException` if it can't win. No SQL-side string comparison remains, so provider/collation quirks are no longer a concern. I checked out the PR branch directly (the diff in the description was truncated) and read the full, current versions of the files most central to this pass's security claims: - **`JsonWebKeyVerifier`**: algorithm/curve pinning happens before the thumbprint cache lookup; the cache key is the RFC 7638 thumbprint (`DPoP.Thumbprint`, same computation `DPoPProofValidator` uses), so an off-curve-Y key can never collide with a valid same-X key in the cache — the thumbprint covers `x`/`y` and a collision would require breaking SHA-256. The `Import` validation (which catches the off-curve point) still runs before anything is cached. - **`SpaceCredentialVerifier`**: cache-hit path still re-checks the requested space, re-validates the authority's current key against the cached `DidKey`/`Document`, and always re-runs the full DPoP proof/`jti` check. LRU + per-authority quota eviction (`Remember`/`RemoveLocked`) is bookkept consistently. - **`SpaceClientAttestationVerifier`**: cached-key → refusal → rate-limited (30s) forced refetch → recheck flow is implemented as described, with shared in-flight fetches via `Lazy<Task<...>>` keyed per client. - **`SpaceAuthority.GetSigningKey`/`GetHostEndpoint`**: correctly distinguish `Absent` (falls back to `#atproto`/`#atproto_pds`) from `Malformed` (throws `FormatException`), built on `DidDocument.TryGetVerificationKey`/`TryGetServiceEndpoint`, which already report `Malformed` for a wrong service type or non-absolute/wrong-scheme endpoint. - **`SpaceRepoCar`**: index-key parsing now requires exactly one `/` (`ParsePath`), index CIDs are required to be DAG-CBOR-coded, and `SpaceRecordCbor.TryValidateRecord` (new) enforces canonical DAG-CBOR (definite lengths, text-only keys, no floats, tag 42-only over a `0x00`-prefixed byte string) via `CborReader` in `Canonical` mode. - **`LexiconTypeRegistry.RegisterUnionVariant`** now throws for a closed union base, and `AtProtoUnionConverter<TBase>` no longer consults the registry when `shape.Closed` — matches the described G22/closed-union-registration behavior exactly. - **`NotifyWriteEndpoint`**: now requires `caller.Issuer == repo` (the `IsRepoHostAsync`/document-based check is gone), matching the reference authority's rule and the item 1/6/8/10a fix description. - **`SpaceCredentialProvider`**: single-flight mint per space via `Lazy<Task<...>>`, double-check-after-await to skip a redundant mint, and superseded-credential retention/disposal under one lock all behave as documented; `Dispose` cancels an in-flight mint and disposes a credential that completes anyway. Everything I inspected is internally consistent, matches the (very detailed) PR description, and no new correctness or security issues turned up. Nothing here blocks merging. --- ⏱ 3m 32s · 23 turns · tokens: 137.3k in / 12.4k out (+2793.6k cache-read) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:5034dc04de5bce4a2c9ec5129c708a109eac2601 -->
Grandiras deleted branch issue-133-spaces 2026-09-26 20:54:08 +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!172
No description provided.