feat: Spaces credential revocation #221
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!221
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-214-spaces-revocation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #214.
Spaces credential revocation, as in the 2026-10-01 alpha (upstream #5567,
com.atproto.space.notifyCredentialRevoked).Repo host
NotifyCredentialRevokedEndpoint(registered byAddSpaceRepoHost) accepts{space, credentials: jti[1..100]}, authenticated with service auth:lxmmust be this method;issmust be the space's authority (403 otherwise);audmust be an account this host serves, checked after the signature so it can't be probed (403 otherwise);jtirefused).notifyWrite.(space, jti)pairs are kept for at least 3610 s (credential max lifetime + 2× clock skew).SpaceCredentialVerifierchecks them on every presentation, cached credentials included, and answers 401CredentialRevoked.ISpaceCredentialRevocationStore, with in-memory and EF Core implementations (AtProtoSpaceCredentialRevocationstable,AddAtProtoEfCoreSpaceCredentialRevocationStore<TContext>()).Authority
SpaceCredentialRevoker.RevokeAsync(space, jtis)(registered byAddSpaceAuthority):audset to the writer. Upstream has no reference fan-out, so this part is our design: best effort, one attempt.Client
SpaceClient.NotifyCredentialRevokedAsync(space, jtis).Breaking
ISpaceRepoHostgainsHostsAccountAsync. The reference requiresaudto be a hosted account, and the interface had no way to ask. The EF schema gains a table. Both are covered indocs/migrating-to-0.8.md.A startup warning flags the in-process default revocation store, as it does for the replay store.
Build has 0 warnings; 5143 unit tests pass.
🤖 Generated with Claude Code
🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
🤖 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.
✅ Verdict: approve — no blocking issues found.
The change matches the description and I found nothing that should block merging. I read the diff only and did not build or run the tests.
Endpoint auth order.
notifyCredentialRevokedchecks body limits first, then the service-auth signature,lxm, expiry and single use. After that it checksissagainst the space's authority. The hosted-account check onaudcomes last, after the signature, so an unauthenticated caller can't probe which accounts the host holds. A fragment audience such asdid#atproto_pdsis rejected with 401.Stores. The in-memory and EF Core stores share one contract test suite. Retention is raised but never lowered, and the sweep deletes only entries whose retention has passed. The EF retry after a concurrent insert looks safe.
Verifier, docs, API files. The verifier checks revocation on every presentation, cached credentials included, and does so last, so only credentials that otherwise verify cost a store read. The CHANGELOG entries, migration notes, docs and public API files are updated.
Minor, non-blocking.
SpaceCredentialRevoker.RevokeAsyncskips the "this service is the authority" check whenServiceDidis not configured. A caller could then record local revocations for another authority's space, and the notifications would go out signed as the wrong DID. Failing fast whenServiceDidis unset would be tidier.🧠 claude-sonnet-5-5 · ⏱ 0m 16s · 2 turns · tokens: 54.8k in / 2.0k out (+12.2k cache-read)
📜 Full session transcript
Independent adversarial review (read-only Sonnet reviewer): approve. Unit suite: 5143 passed.
issfrom the space URI, the store keyed by that space).lxmandaudare enforced, with the hosted-account lookup only after the signature. Replay is covered. Every credential presentation, cached or not, consults the store, keyed by the verified credential'sjti. The sweep can't delete a live entry. Fan-out sends onlyjtis, through the SSRF-guarded resolver.ClockSkew, but credentials are verified with the fixed 5 sDefaultClockSkew. WithClockSkew = 0a revocation lapsed up to 10 s before its credential expired. Retention now uses the larger of the two, asReplayRetentiondoes.Jwt.IsUsableTokenId(empty, control characters, over 255) are refused withInvalidRequest, since no credential can carry them and they would only take up store space.🤖 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.
✅ Verdict: approve — no blocking issues found.
The new commit is fine, and I found nothing that blocks merging. I read the diff and checked
Jwt.IsUsableTokenIdand theSpaceTokensconstants. I did not build or run the tests.SpaceCredentialRevocation.RetainUntilnow uses the larger ofoptions.ClockSkewandSpaceTokens.DefaultClockSkew. That matches the skew the credential checks use, and a test covers aClockSkewof zero.Last pass's minor note is still open and still non-blocking:
SpaceCredentialRevoker.RevokeAsyncskips the "this service is the authority" check whenServiceDidis unset.🧠 claude-sonnet-5-5 · ⏱ 0m 7s · 3 turns · tokens: 57.3k in / 599 out (+80.5k cache-read)
📜 Full session transcript
✅ Verdict: approve — no blocking issues found.
This is a well-built feature. The security-critical parts hold up:
notifyWrite), then the service auth signature is verified forlxm/audience/single-use, and only then isisscompared against the space's authority andHostsAccountAsyncconsulted — so an unauthenticated caller can't probe which accounts the host serves, and a non-authority can't lock readers out of a space. Accepting unknownjtis(revocation racing credential arrival) and idempotency are both correct.MaxCredentialLifetime + 2×ClockSkewfrom receipt covers a credential issued up to one skew in the past plus the skew at expiry; both stores only delete entries whose retention has passed, so sweeping can never revive a credential, and the conditional update (RetainUntil < until) never shortens an entry.aud= writer DID, batches of 100, one best-effort attempt) is a reasonable choice given upstream has no reference, and it's documented as such. TheAudience()mapping reuses the authority-host special case correctly.ISpaceRepoHost.HostsAccountAsyncand EF-schema breaks are justified by the reference's hosted-account requirement and are documented in the migration guide; the startup warning for the in-process default mirrors the replay store, and the tests cover it.The in-memory/EF contract tests plus endpoint tests (idempotency, cross-space isolation, non-authority refusal, audience probing, replay, batch bounds, retention window) give good coverage; the SQLite cross-instance test exercises what a shared store actually buys.
Two non-blocking notes: the endpoint accepts whitespace-only identifiers (
IsNullOrEmpty) whereSpaceClientandSpaceCredentialRevokerreject them (IsNullOrWhiteSpace) — harmless since such ajtican never match a minted credential, but the two validations could agree. And the fragment-form audience (did:plc:…#atproto_pds) is refused with 401 "not a repo DID" rather than the 403 path; since the reference requiresaudto be a hosted account, that's consistent, just worth knowing if a test harness ever sends the fragment form.⏱ 6m 7s · 7 steps · $0.0185 · tokens: 65.7k in / 1.1k out (+169.8k cache-read) · Go: 5h 4% · wk 2% · mo 92%
📜 Full session transcript
🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
de2354901a4d5970fda1🤖 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.
✅ Verdict: approve — no blocking issues found.
I found nothing that blocks merging. I read the diff only and did not build or run the tests.
My earlier non-blocking note is still open.
SpaceCredentialRevoker.RevokeAsyncskips the "this service is the authority" check whenServiceDidis unset. A caller could then record local revocations for another authority's space, and the notifications would go out signed as the wrong DID. Throwing whenServiceDidis null would be tidier.The endpoint checks the body limits and identifier validity first, then the service-auth signature,
iss, and the hosted account onaud. The retention window uses the larger ofClockSkewand the default skew. The verifier checks revocation on every presentation, and both stores run the same contract tests. The CHANGELOG entries, migration notes and public API files are updated.🧠 claude-sonnet-5-5 · ⏱ 0m 6s · 2 turns · tokens: 56.4k in / 548 out (+12.2k cache-read)
📜 Full session transcript
✅ Verdict: approve — no blocking issues found.
Re-review: both non-blocking notes from my last pass are resolved. The endpoint now validates each credential identifier with
Jwt.IsUsableTokenId(not-only-whitespace, no control characters, ≤ 255 chars, 400 otherwise), bringing it in line withSpaceClientandSpaceCredentialRevoker; tests cover blank, tab, and over-long identifiers. The fragment-form audience behavior is unchanged and as designed.The security-critical parts remain sound: body shape is refused before auth (matching
notifyWrite); the service-auth signature,lxm,audand single-usejtiare verified before theiss-is-the-authority andHostsAccountAsyncchecks, so an unauthenticated caller cannot probe which accounts the host serves and a non-authority cannot lock readers out; unknownjtisare recorded (revocation may race credential arrival) and the call is idempotent. Retention isMaxCredentialLifetime + 2×max(options.ClockSkew, SpaceTokens.DefaultClockSkew)from receipt, matching the skew the credential checks actually use; both stores never shorten an entry and sweep only rows past retention, so cleanup cannot revive a credential. The verifier checks revocation on every presentation, cached credentials included, and last, so only credentials that otherwise verify cost a store read.Breaking changes (
ISpaceRepoHost.HostsAccountAsync, theAtProtoSpaceCredentialRevocationsEF table) are justified and documented indocs/migrating-to-0.8.md; the startup warning for the in-process default store mirrors the replay store and its tests were updated. The in-memory and EF Core stores share one contract test suite, plus SQLite-specific concurrency and endpoint tests. No remaining issues; good to merge.⏱ 3m 27s · 4 steps · $0.0112 · tokens: 59.1k in / 835 out (+57.3k cache-read) · Go: 5h 4% · wk 2% · mo 92%
📜 Full session transcript
🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
✅ Verdict: approve — no blocking issues found.
Re-review: both non-blocking notes from my previous pass remain resolved — the endpoint validates each identifier with
Jwt.IsUsableTokenId(blank/control-character/over-long → 400), and the retention window uses the larger ofoptions.ClockSkewandSpaceTokens.DefaultClockSkew, with a test coveringClockSkew = 0.This pass I additionally checked the new fan-out code (
SpaceWriteNotifier.NotifyCredentialRevokedAsync) against the existing delivery paths. It reusesDeliverAsynccorrectly: failures (including DID resolution) are logged and counted as not-delivered, exceptions are absorbed per attempt, and only successful deliveries increment the counter. Each request is addressed (aud) to the writer's bare DID as the endpoint requires, signed viaSpaceAccountSigning.ChooseAsyncwith the same issuer-selection fallback the other notifications use, and batches of 100 respect the request limit. The endpoint path remains sound: body shape first, then the service-auth signature withlxm/aud/single-use, theniss== the space authority, thenHostsAccountAsync— so the hosted-account list cannot be probed by unauthenticated callers and a non-authority cannot lock readers out. The verifier still checks revocation on every presentation (cached credentials included) and last, so only otherwise-valid credentials cost a store read; retention isMaxCredentialLifetime + 2×max(skews)from receipt and neither store ever shortens an entry or sweeps rows still within retention.One non-blocking note stands open (explained on the code):
SpaceCredentialRevoker.RevokeAsyncdoes not require an explicitly configuredServiceDidbefore recording a local revocation, so withServiceDidunset it could record a revocation for a space this service does not author; the receiving host would still refuse it, so it is tidy-up rather than a blocker.Good to merge.
⏱ 1m 6s · 6 steps · $0.0115 · tokens: 39.6k in / 1.0k out (+151.6k cache-read) · Go: 5h 4% · wk 2% · mo 92%
📜 Full session transcript
@ -0,0 +55,4 @@if (credentialIds.Any(string.IsNullOrWhiteSpace))throw new ArgumentException("A credential is revoked by its non-blank jti.", nameof(credentialIds));if (_options.ServiceDid is { } serviceDid && space.Authority != serviceDid)throw new ArgumentException($"This service is not the authority for {space}.", nameof(space));Non-blocking, carried over from earlier passes: when
SpaceServerOptions.ServiceDidis unset, the "this service is the authority for the space" check is skipped, so a caller could record local revocations for another authority's space and the notifications would go out signed as this service (which receivers then refuse oniss). Failing fast whenServiceDidis null would be tidier, but it is not a blocker: a receiver only honors revocations signed by the space's authority, and the default used in production flows setsServiceDid.🔧 Let opencode-bot fix this — tick the box and opencode-bot picks up the review above, on this PR branch. Nothing runs until you do.