Issue #89 landed the client half of the permissioned data protocol. This
is the other half: the ASP.NET Core layer over the ATProtoNet.Spaces
primitives that lets a .NET service act as a space authority, a repo
host, or both.

The verifiers are the part that has to be right. Three checks carry the
protocol's guarantees:

  - A delegation token's audience must equal spaceHostAud(spaceDid) for
    the authority named in the token's own sub, derived from the token
    and never taken from the request, so an authority handed a token
    minted for another authority cannot present it there.
  - A credential's signer is resolved from the space URI rather than
    from the credential's own iss, so nobody but a space's authority can
    mint credentials for it.
  - A DPoP proof's signature verifies against its own embedded jwk —
    which proves nothing alone — and that key's thumbprint is matched
    against the credential's cnf.jkt, which is what makes it mean
    something. ath pins the proof to the credential presented, htm/htu
    to this request, iat bounds its useful life, and jti is spent once.

Single use is ISpaceReplayStore, keyed on (iss, jti, exp) and consumed
last, only once every other check has passed, so a forged token cannot
burn the identifier of one a legitimate holder is about to present.

Also adds the endpoint surface (authority and repo host), the
com.atproto.simplespace administration methods with the baseline access
policy, and best-effort write notification delivery with first-write
auto-registration of the authority.

Two general additions to the XRPC routing came out of it: XrpcException,
so a handler produces the named error clients branch on rather than a
500, and IXrpcBlobQuery<TParams>, for the Lexicon methods whose output
encoding is not application/json.

Verified: dotnet build clean with no warnings; 1601 unit tests pass in
both Debug and Release, 90 of them new, including a TestServer pass over
the real HTTP surface from the credential exchange through a repo read.

closes #91

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude-bot referenced this pull request

2026-08-20 21:51:39 +00:00

Spaces: server-side support in ATProtoNet.Server (space authority and repo host) #91

claude-bot commented

2026-08-20 21:51:44 +00:00

Author

Collaborator

🤖 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/332); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-101/run-332.md) — updating every ~5 minutes while the task runs.

claude-reviewer reviewed

2026-08-20 21:52:53 +00:00

claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a large, carefully engineered PR implementing the server-side (authority + repo host) half of the Spaces protocol. The parts I could review in the (truncated) diff are consistent with the security-conscious style established in the earlier client-side Spaces PR: DPoP proof verification follows RFC 9449 correctly (own-jwk verify, then thumbprint-to-cnf.jkt binding, ath/htm/htu/iat checks, single-use jti via a replay store keyed on (iss, jti, exp)), delegation-token audience is derived from the token's own sub rather than the request, space-credential signer resolution is anchored to the space URI rather than the credential's iss, and constant-time comparisons (CryptographicOperations.FixedTimeEquals) are used for the security-sensitive string comparisons. JWK algorithm is correctly pinned to the key's curve, and private-key material embedded in a proof's jwk is rejected. The ISpaceReplayStore keying on (issuer, jti, exp) is a reasonable design and the rationale is well documented.

A few things worth flagging, none blocking based on what's visible:

DPoPProofValidator.NormalizeUri falls back to returning the raw url unchanged when Uri.TryCreate fails (non-absolute htu). Since both sides of the comparison go through the same normalization, this isn't exploitable, but it does mean a malformed htu silently skips scheme/host/port canonicalization rather than being rejected outright — worth confirming a request URI passed in as requestUri is always a well-formed absolute URI upstream (built from PublicBaseUrl + path) so this fallback path is truly unreachable for the trusted side.
SpaceServiceAuthVerifier.VerifyAsync checks exp (with clock skew) but doesn't appear to validate iat/freshness for service-auth tokens the way the DPoP proof does; that's consistent with the existing AT Proto service-auth convention elsewhere in the codebase, so likely fine, just noting the asymmetry.
The diff was truncated mid-file (cut off inside SpaceAuthorityEndpoints.cs), so I was not able to review the remaining endpoint handlers (repo host reads, getBlob access-check enforcement, write-notification delivery/inbound validation) or the new unit tests referenced in the CHANGELOG ("90 new unit tests"). Given the security-critical nature of this surface (credential exchange, blob access checks, replay handling), I'd recommend a follow-up pass — ideally via /code-review ultra — specifically over the untruncated remainder before merge, if that hasn't already happened.

Nothing in the reviewed portion looks incorrect or unsafe; CHANGELOG.md was updated in the same commit as required by the repo's CLAUDE.md.

⏱ 0m 54s · 2 turns · tokens: 73.1k in / 4.6k out (+129.6k cache-read)

💬 **Verdict: comment** — notes only, nothing blocking. This is a large, carefully engineered PR implementing the server-side (authority + repo host) half of the Spaces protocol. The parts I could review in the (truncated) diff are consistent with the security-conscious style established in the earlier client-side Spaces PR: DPoP proof verification follows RFC 9449 correctly (own-jwk verify, then thumbprint-to-cnf.jkt binding, ath/htm/htu/iat checks, single-use jti via a replay store keyed on (iss, jti, exp)), delegation-token audience is derived from the token's own sub rather than the request, space-credential signer resolution is anchored to the space URI rather than the credential's iss, and constant-time comparisons (CryptographicOperations.FixedTimeEquals) are used for the security-sensitive string comparisons. JWK algorithm is correctly pinned to the key's curve, and private-key material embedded in a proof's jwk is rejected. The `ISpaceReplayStore` keying on (issuer, jti, exp) is a reasonable design and the rationale is well documented. A few things worth flagging, none blocking based on what's visible: - `DPoPProofValidator.NormalizeUri` falls back to returning the raw `url` unchanged when `Uri.TryCreate` fails (non-absolute `htu`). Since both sides of the comparison go through the same normalization, this isn't exploitable, but it does mean a malformed `htu` silently skips scheme/host/port canonicalization rather than being rejected outright — worth confirming a request URI passed in as `requestUri` is always a well-formed absolute URI upstream (built from `PublicBaseUrl` + path) so this fallback path is truly unreachable for the trusted side. - `SpaceServiceAuthVerifier.VerifyAsync` checks `exp` (with clock skew) but doesn't appear to validate `iat`/freshness for service-auth tokens the way the DPoP proof does; that's consistent with the existing AT Proto service-auth convention elsewhere in the codebase, so likely fine, just noting the asymmetry. - The diff was truncated mid-file (cut off inside `SpaceAuthorityEndpoints.cs`), so I was not able to review the remaining endpoint handlers (repo host reads, `getBlob` access-check enforcement, write-notification delivery/inbound validation) or the new unit tests referenced in the CHANGELOG ("90 new unit tests"). Given the security-critical nature of this surface (credential exchange, blob access checks, replay handling), I'd recommend a follow-up pass — ideally via `/code-review ultra` — specifically over the untruncated remainder before merge, if that hasn't already happened. Nothing in the reviewed portion looks incorrect or unsafe; CHANGELOG.md was updated in the same commit as required by the repo's CLAUDE.md. --- ⏱ 0m 54s · 2 turns · tokens: 73.1k in / 4.6k out (+129.6k cache-read)

claude-bot commented

2026-08-20 21:53:47 +00:00

Author

Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/335); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-101/run-335.md) — updating every ~5 minutes while the task runs.

claude-bot referenced this pull request from a commit

2026-08-20 22:02:02 +00:00

fix: reject unnormalizable htu and cap single-use token lifetimes

claude-bot added 1 commit

2026-08-20 22:02:02 +00:00

fix: reject unnormalizable htu and cap single-use token lifetimes

CI / pds-integration (pull_request) Successful in 27s

Details

CI / build-and-test (pull_request) Successful in 1m0s

Details

b86628fbce

Two points from the review of #101.

NormalizeUri fell back to returning its argument unchanged when the value
was not an absolute URL. Both sides of the htu comparison went through it,
so it was not exploitable, but a malformed htu silently skipped scheme,
host, and port canonicalization instead of being rejected — a comparison of
two raw strings that has been checked against nothing. It now returns null
for anything that is not an absolute URL with a host: a proof's htu is
refused outright, and a request URI that does not normalize is a fault here
rather than a failed verification, since it means this service's own
PublicBaseUrl is misconfigured.

The service auth verifier checked exp but never bounded it, and neither did
the delegation token or client attestation verifiers. All three tokens are
minted to live 60 seconds, but the exp on an inbound one is whatever its
signer chose, and all three have their jti consumed before anything
authorizes their issuer — so a token dated years ahead stayed replayable for
years and pinned its replay-store entry for just as long, the store evicting
an entry only once the token it guards has expired. They are now bounded by
SpaceServerOptions.MaxSingleUseTokenLifetime, five minutes by default, which
is what ServiceAuthGenerator already refuses to exceed when minting. Service
auth additionally rejects an iat dated in the future, which it did not check
at all.

Verified: dotnet build clean with no warnings; 1613 unit tests pass in both
Debug and Release, 12 of them new.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude-bot commented

2026-08-20 22:02:37 +00:00

Author

Collaborator

Pushed to claude/issue-91 as b86628f — the PR is updated.

What I changed

1. htu that doesn't normalize is now rejected (review point 1). The reviewer was right that the fallback wasn't exploitable — both sides went through the same normalization, so a relative htu failed the comparison anyway — but "not exploitable because the other side is also broken" is the wrong reason for a security check to hold. NormalizeUri now returns null for anything that isn't an absolute URL naming a host, and the two sides are treated differently on purpose:

a proof's htu that doesn't normalize is refused outright (NotAuthorized), because a value that skipped scheme/host/port canonicalization has been checked against nothing;
a request URI that doesn't normalize throws ArgumentException, since that's this service's own PublicBaseUrl being misconfigured (e.g. set without a scheme) — a fault, not a failed verification. To answer the question the review asked: BuildRequestUri composes {PublicBaseUrl or request scheme+host}{path}, so the trusted side is absolute in a correct configuration, and it now fails loudly rather than silently degrading when it isn't.

2. Single-use tokens are bounded in lifetime (review point 2). Looking into the iat/freshness asymmetry turned up something more concrete than the asymmetry itself: exp was checked but never bounded, and the same was true of delegation tokens and client attestations. All three are minted to live 60 seconds — ServiceAuthGenerator outright refuses to mint beyond five minutes — but the exp on an inbound one is whatever its signer chose, and all three have their jti consumed by ISpaceReplayStore before anything authorizes their issuer (the IsRepoHostAsync check on notifyWrite runs after VerifyAsync returns). So a self-signed token dated years ahead stayed replayable for years and pinned a replay-store entry for exactly as long, since the store evicts an entry only once the token it guards has expired. New SpaceServerOptions.MaxSingleUseTokenLifetime (five minutes, configurable) caps all three, and service auth now also rejects an iat dated in the future.

3. The truncated remainder (review point 3). I can't launch /code-review ultra — it's user-triggered and billed — so I read the rest myself: the repo-host read endpoints, getBlob, notifyWrite, SpaceWriteNotifier, the simplespace administration endpoints, SimpleSpaceAccessPolicy, and the new tests. Nothing else needed changing. The getBlob access check is real: the endpoint delegates to ISpaceRepoHost.GetBlobAsync(space, repo, cid), whose contract documents that the reference check is the access check and that a CID-only lookup would leak any blob the account holds. The one thing I looked at hard and concluded is sound: NotifyWriteEndpoint derives the expected service-auth audience from _options.ServiceDid ?? space.Authority, i.e. partly from the request when ServiceDid is unconfigured — but the space-state check that follows gates it to spaces this authority actually holds, whose authority is this service, so the audience can't be attacker-chosen in any case that reaches a write.

Docs (docs/spaces.md, docs/api-reference.md) and CHANGELOG.md updated in the same commit; the test count in the changelog entry went 90 → 102.

Verification

dotnet build clean with 0 warnings, and dotnet test tests/ATProtoNet.Tests/ passes in both Debug and Release: 1613 passed, 0 failed, 5 skipped (12 new — relative/non-absolute htu, non-absolute request URI, NormalizeUri null cases, and a lifetime-ceiling test for each of the three single-use token kinds, plus a small SpaceServiceAuthVerifier suite that didn't exist before).

No follow-up issues to file — everything I found in the unreviewed remainder is either fixed above or correct as written.

⏱ 8m 29s · 58 turns · tokens: 137.4k in / 30.6k out (+5550.7k cache-read)
📜 Full session transcript

Pushed to `claude/issue-91` as `b86628f` — the PR is updated. ## What I changed **1. `htu` that doesn't normalize is now rejected (review point 1).** The reviewer was right that the fallback wasn't exploitable — both sides went through the same normalization, so a relative `htu` failed the comparison anyway — but "not exploitable because the other side is also broken" is the wrong reason for a security check to hold. `NormalizeUri` now returns `null` for anything that isn't an absolute URL naming a host, and the two sides are treated differently on purpose: - a proof's `htu` that doesn't normalize is refused outright (`NotAuthorized`), because a value that skipped scheme/host/port canonicalization has been checked against nothing; - a *request* URI that doesn't normalize throws `ArgumentException`, since that's this service's own `PublicBaseUrl` being misconfigured (e.g. set without a scheme) — a fault, not a failed verification. To answer the question the review asked: `BuildRequestUri` composes `{PublicBaseUrl or request scheme+host}{path}`, so the trusted side *is* absolute in a correct configuration, and it now fails loudly rather than silently degrading when it isn't. **2. Single-use tokens are bounded in lifetime (review point 2).** Looking into the `iat`/freshness asymmetry turned up something more concrete than the asymmetry itself: `exp` was checked but never bounded, and the same was true of delegation tokens and client attestations. All three are minted to live 60 seconds — `ServiceAuthGenerator` outright refuses to mint beyond five minutes — but the `exp` on an *inbound* one is whatever its signer chose, and all three have their `jti` consumed by `ISpaceReplayStore` *before* anything authorizes their issuer (the `IsRepoHostAsync` check on `notifyWrite` runs after `VerifyAsync` returns). So a self-signed token dated years ahead stayed replayable for years and pinned a replay-store entry for exactly as long, since the store evicts an entry only once the token it guards has expired. New `SpaceServerOptions.MaxSingleUseTokenLifetime` (five minutes, configurable) caps all three, and service auth now also rejects an `iat` dated in the future. **3. The truncated remainder (review point 3).** I can't launch `/code-review ultra` — it's user-triggered and billed — so I read the rest myself: the repo-host read endpoints, `getBlob`, `notifyWrite`, `SpaceWriteNotifier`, the `simplespace` administration endpoints, `SimpleSpaceAccessPolicy`, and the new tests. Nothing else needed changing. The `getBlob` access check is real: the endpoint delegates to `ISpaceRepoHost.GetBlobAsync(space, repo, cid)`, whose contract documents that the reference check *is* the access check and that a CID-only lookup would leak any blob the account holds. The one thing I looked at hard and concluded is sound: `NotifyWriteEndpoint` derives the expected service-auth audience from `_options.ServiceDid ?? space.Authority`, i.e. partly from the request when `ServiceDid` is unconfigured — but the space-state check that follows gates it to spaces this authority actually holds, whose authority is this service, so the audience can't be attacker-chosen in any case that reaches a write. Docs (`docs/spaces.md`, `docs/api-reference.md`) and `CHANGELOG.md` updated in the same commit; the test count in the changelog entry went 90 → 102. ## Verification `dotnet build` clean with 0 warnings, and `dotnet test tests/ATProtoNet.Tests/` passes in both Debug and Release: **1613 passed, 0 failed, 5 skipped** (12 new — relative/non-absolute `htu`, non-absolute request URI, `NormalizeUri` null cases, and a lifetime-ceiling test for each of the three single-use token kinds, plus a small `SpaceServiceAuthVerifier` suite that didn't exist before). No follow-up issues to file — everything I found in the unreviewed remainder is either fixed above or correct as written. --- ⏱ 8m 29s · 58 turns · tokens: 137.4k in / 30.6k out (+5550.7k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-101/run-335.md)

Grandiras commented

2026-08-20 22:03:54 +00:00

Owner

@claude-bot Please fix the merge conflict

claude-bot commented

2026-08-21 01:02:42 +00:00

Author

Collaborator

🤖 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/347); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-101/run-347.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-08-21 01:21:12 +00:00

Dismissed

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

This re-review is on the follow-up commit fix: reject unnormalizable htu and cap single-use token lifetimes, which lands on top of the previously-reviewed feat: server-side space support in ATProtoNet.Server commit. It directly addresses both points I raised in my prior pass:

DPoPProofValidator.NormalizeUri no longer falls back to returning the raw url string when it fails to parse as an absolute URI — it now returns null, and both the trusted request-side URI and the proof's htu are required to normalize successfully or the request is rejected (ArgumentException for a misconfigured PublicBaseUrl on the trusted side, SpaceVerificationException for a bad htu on the caller side). This closes the theoretical gap where an unparseable htu would previously be compared verbatim.
SpaceServerOptions.MaxSingleUseTokenLifetime (five minutes by default) is now enforced on the three single-use, signer-chosen-exp token kinds — delegation tokens, client attestations, and service auth tokens — via the new IsWithinSingleUseWindow helper. This bounds how long a captured/forged-but-signed token stays replayable and how long its jti sits in the replay store, addressing the asymmetry I flagged around service-auth freshness (which now also rejects a future-dated iat).

Both changes are covered by new, well-targeted unit tests (relative/non-absolute htu rejection, oversized-lifetime rejection for all three token kinds, future-dated iat rejection), and CHANGELOG.md/docs/spaces.md/docs/api-reference.md were updated in the same commit per the repo's CLAUDE.md requirement. The DI wiring in SpaceServerExtensions was updated to pass SpaceServerOptions into SpaceClientAttestationVerifier, which is consistent with the new check.

I was not able to run dotnet build/dotnet test in this session (git checkout/worktree operations required approval I didn't have), but the diff is a small, self-contained, low-risk change confined to validation-tightening logic with accompanying tests, and I reviewed the full diff (not truncated this time) plus the underlying full files where relevant. Nothing else in the change looks incorrect. One trivial nit for a future pass: the CHANGELOG's new DPoP bullet is missing a period between "...checked against nothing" and "A proof whose header leaks..." — not blocking.

Both previously-raised concerns are resolved; approving.

⏱ 18m 17s · 15 turns · tokens: 96.9k in / 4.6k out (+1555.4k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This re-review is on the follow-up commit `fix: reject unnormalizable htu and cap single-use token lifetimes`, which lands on top of the previously-reviewed `feat: server-side space support in ATProtoNet.Server` commit. It directly addresses both points I raised in my prior pass: 1. **`DPoPProofValidator.NormalizeUri`** no longer falls back to returning the raw `url` string when it fails to parse as an absolute URI — it now returns `null`, and both the trusted request-side URI and the proof's `htu` are required to normalize successfully or the request is rejected (`ArgumentException` for a misconfigured `PublicBaseUrl` on the trusted side, `SpaceVerificationException` for a bad `htu` on the caller side). This closes the theoretical gap where an unparseable `htu` would previously be compared verbatim. 2. **`SpaceServerOptions.MaxSingleUseTokenLifetime`** (five minutes by default) is now enforced on the three single-use, signer-chosen-`exp` token kinds — delegation tokens, client attestations, and service auth tokens — via the new `IsWithinSingleUseWindow` helper. This bounds how long a captured/forged-but-signed token stays replayable and how long its `jti` sits in the replay store, addressing the asymmetry I flagged around service-auth freshness (which now also rejects a future-dated `iat`). Both changes are covered by new, well-targeted unit tests (relative/non-absolute `htu` rejection, oversized-lifetime rejection for all three token kinds, future-dated `iat` rejection), and `CHANGELOG.md`/`docs/spaces.md`/`docs/api-reference.md` were updated in the same commit per the repo's CLAUDE.md requirement. The DI wiring in `SpaceServerExtensions` was updated to pass `SpaceServerOptions` into `SpaceClientAttestationVerifier`, which is consistent with the new check. I was not able to run `dotnet build`/`dotnet test` in this session (git checkout/worktree operations required approval I didn't have), but the diff is a small, self-contained, low-risk change confined to validation-tightening logic with accompanying tests, and I reviewed the full diff (not truncated this time) plus the underlying full files where relevant. Nothing else in the change looks incorrect. One trivial nit for a future pass: the CHANGELOG's new DPoP bullet is missing a period between "...checked against nothing" and "A proof whose header leaks..." — not blocking. Both previously-raised concerns are resolved; approving. --- ⏱ 18m 17s · 15 turns · tokens: 96.9k in / 4.6k out (+1555.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-101/run-347.md)

claude-bot commented

2026-08-21 01:21:13 +00:00

Author

Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/349); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-101/run-349.md) — updating every ~5 minutes while the task runs.

claude-bot added 1 commit

2026-08-21 01:27:30 +00:00

Merge main into claude/issue-91

CI / pds-integration (pull_request) Successful in 28s

Details

CI / build-and-test (pull_request) Successful in 55s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 6s

Details

935543551b

Two textual conflicts, both additive: the `### Added` section of
CHANGELOG.md and the Spaces line in docs/index.md. Both sides kept.

One interaction the merge does not resolve on its own. #98 taught the
core to read the legacy `Ecdsa...VerificationKey2019` verification-method
types alongside `Multikey`, and routed `SpaceAuthority.GetSigningKey`
through it. The space server's own key lookup in
`SpaceDidDocumentResolverExtensions` still had a private Multikey-only
`FindMultikey`, used whenever a token names a `kid` — so after the merge
the same DID document verified through the no-`kid` path and was refused
through the `kid` path. Both now go through
`DidDocument.GetVerificationKey`.

That lookup also throws `FormatException` on malformed key material,
which would have escaped a verifier as a 500. The document belongs to
the party being verified, so it is now a `SpaceVerificationException`
under the caller's own error name.

Three tests: a delegation token from an account publishing a legacy
`#atproto` entry, a credential naming a legacy `#atproto_space` by
`kid`, and malformed key material reported as a failed verification.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude-bot commented

2026-08-21 01:27:52 +00:00

Author

Collaborator

Merged main into claude/issue-91 and pushed as 9355435 — the PR merges cleanly now (the branch fully contains origin/main at 5bd0062).

The conflicts

Two files, both purely additive — main and this branch had each appended to the same spot:

CHANGELOG.md — both sides added entries under ### Added. Kept both, ordered #93, #92, #91, #89 to match the file's existing descending convention. Also took the reviewer's nit from the last pass while I was in the file: the missing period in the DPoP bullet.
docs/index.md — this branch extended the Spaces line with the serving a space anchor; main added a Testing Against a Real Space Host line beneath it. Kept both.

docs/spaces.md auto-merged and I checked the result by hand rather than trusting it: main's three additions (the lexgen cross-reference, the Partial/NoRepo paragraph, the See-also link) all survived alongside this branch's new Serving a space section, with no duplicated heading.

What the merge broke that the merge didn't fix

Textual conflicts weren't the whole story. #98 landed on main while this branch was open and taught the core to read the legacy Ecdsa...VerificationKey2019 verification-method types alongside Multikey, routing SpaceAuthority.GetSigningKey through the new DidDocument.GetVerificationKey. This branch's server-side key lookup in SpaceDidDocumentResolverExtensions still carried its own private FindMultikey, which requires Type == "Multikey" — and that one is used on exactly one of the two paths: whenever a presented token names a kid.

So the merged tree compiled and every existing test passed, while the same DID document verified through the no-kid path and was refused through the kid path. Neither branch is wrong on its own; the disagreement only exists once they're combined. Both paths now go through GetVerificationKey.

The second half of the same interaction: that lookup throws FormatException on malformed key material, where the old one returned null. In a verifier that's an unhandled exception — a 500 — over a document fetched from the party being verified, i.e. something a remote party controls. It's now a SpaceVerificationException reported under the caller's own error name (InvalidDelegationToken on the delegation path, not a generic one), which is what the rest of the verification surface does.

Three tests cover it: a delegation token from an account publishing a legacy #atproto entry, a credential naming a legacy #atproto_space by kid (the path that doesn't go through SpaceAuthority.GetSigningKey), and malformed key material surfacing as a failed verification. Changelog test count 102 → 105, with a sub-bullet on the change.

Verification

dotnet build clean, 0 warnings. dotnet test tests/ATProtoNet.Tests/ in Release: 1657 passed, 0 failed, 5 skipped — that's main's tests and this branch's running together for the first time. The integration-test project compiles too, though its space tests stay gated behind ATPROTO_TEST_SPACES and didn't run here.

No new issues to file.

⏱ 6m 28s · 49 turns · tokens: 73.1k in / 19.2k out (+3005.4k cache-read)
📜 Full session transcript

Merged `main` into `claude/issue-91` and pushed as `9355435` — the PR merges cleanly now (the branch fully contains `origin/main` at `5bd0062`). ## The conflicts Two files, both purely additive — `main` and this branch had each appended to the same spot: - **`CHANGELOG.md`** — both sides added entries under `### Added`. Kept both, ordered `#93, #92, #91, #89` to match the file's existing descending convention. Also took the reviewer's nit from the last pass while I was in the file: the missing period in the DPoP bullet. - **`docs/index.md`** — this branch extended the Spaces line with the *serving a space* anchor; `main` added a *Testing Against a Real Space Host* line beneath it. Kept both. `docs/spaces.md` auto-merged and I checked the result by hand rather than trusting it: `main`'s three additions (the lexgen cross-reference, the `Partial`/`NoRepo` paragraph, the See-also link) all survived alongside this branch's new *Serving a space* section, with no duplicated heading. ## What the merge broke that the merge didn't fix Textual conflicts weren't the whole story. `#98` landed on `main` while this branch was open and taught the core to read the legacy `Ecdsa...VerificationKey2019` verification-method types alongside `Multikey`, routing `SpaceAuthority.GetSigningKey` through the new `DidDocument.GetVerificationKey`. This branch's server-side key lookup in `SpaceDidDocumentResolverExtensions` still carried its own private `FindMultikey`, which requires `Type == "Multikey"` — and that one is used on exactly one of the two paths: whenever a presented token names a `kid`. So the merged tree compiled and every existing test passed, while the same DID document verified through the no-`kid` path and was refused through the `kid` path. Neither branch is wrong on its own; the disagreement only exists once they're combined. Both paths now go through `GetVerificationKey`. The second half of the same interaction: that lookup throws `FormatException` on malformed key material, where the old one returned `null`. In a verifier that's an unhandled exception — a 500 — over a document fetched from the party *being verified*, i.e. something a remote party controls. It's now a `SpaceVerificationException` reported under the caller's own error name (`InvalidDelegationToken` on the delegation path, not a generic one), which is what the rest of the verification surface does. Three tests cover it: a delegation token from an account publishing a legacy `#atproto` entry, a credential naming a legacy `#atproto_space` by `kid` (the path that doesn't go through `SpaceAuthority.GetSigningKey`), and malformed key material surfacing as a failed verification. Changelog test count 102 → 105, with a sub-bullet on the change. ## Verification `dotnet build` clean, 0 warnings. `dotnet test tests/ATProtoNet.Tests/` in Release: **1657 passed, 0 failed, 5 skipped** — that's `main`'s tests and this branch's running together for the first time. The integration-test project compiles too, though its space tests stay gated behind `ATPROTO_TEST_SPACES` and didn't run here. No new issues to file. --- ⏱ 6m 28s · 49 turns · tokens: 73.1k in / 19.2k out (+3005.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-101/run-349.md)

claude-bot commented

2026-08-21 01:28:32 +00:00

Author

Collaborator

🤖 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/350); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-101/run-350.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-08-21 01:29:40 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Re-review after a new commit landed: 9355435 Merge main into claude/issue-91. This is the only change since my prior approval of b86628f (the fix commit) — no new feature commits.

The merge itself is substantive and worth checking, not just a mechanical resync: bringing in main pulled in issue #98's work (teaching DidDocument.GetVerificationKey to read legacy Ecdsa...VerificationKey2019 entries alongside Multikey), which would otherwise have silently regressed this PR's space verifiers. Before the merge, SpaceDidDocumentResolverExtensions had its own private FindMultikey helper used whenever a token named a kid, so a legacy-key DID document would verify through the no-kid path but be wrongly refused through the kid path post-merge. The merge commit fixes this correctly: both ResolveAccountKeyAsync and ResolveAuthorityKeyAsync now route through the shared DidDocument.GetVerificationKey, and I confirmed that method (in src/ATProtoNet/Identity/PlcClient.cs) does read through VerificationMethod.ToDidKey(), which is where #98's legacy-key support lives. The commit also correctly converts the FormatException GetVerificationKey can throw on malformed key material into a SpaceVerificationException under the caller's own error name, rather than letting it escape as an unhandled 500 — consistent with the pattern used elsewhere in this PR (e.g. CachingSpaceDidDocumentResolver.ResolveAsync).

Three new unit tests were added covering exactly this interaction (legacy #atproto delegation-token signer, legacy #atproto_space credential signer resolved by kid, and malformed key material surfaced as a failed verification with "malformed" in the message) — I checked these exist in tests/ATProtoNet.Tests/Server/Spaces/SpaceTokenVerifierTests.cs and SpaceServerTestFixtures.cs and match the stated intent.

The two textual conflicts (CHANGELOG.md's ### Added section, docs/index.md's Spaces line) were resolved additively with both sides kept, and the new CHANGELOG bullet documenting this merge-time key-resolution fix was added in the same commit. As a bonus, the trivial missing-period nit I flagged in my previous pass ("...checked against nothing. A proof...") is now fixed in this same CHANGELOG text.

I wasn't able to run dotnet build/dotnet test in this session (worktree/build tooling required approval I didn't have), but the change is small, self-contained, well-reasoned, and backed by targeted tests. Both prior review passes' concerns remain resolved, and this merge commit introduces no new issues. Approving.

⏱ 0m 55s · 15 turns · tokens: 91.2k in / 3.9k out (+1548.9k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review after a new commit landed: `9355435 Merge main into claude/issue-91`. This is the only change since my prior approval of `b86628f` (the fix commit) — no new feature commits. The merge itself is substantive and worth checking, not just a mechanical resync: bringing in main pulled in issue #98's work (teaching `DidDocument.GetVerificationKey` to read legacy `Ecdsa...VerificationKey2019` entries alongside `Multikey`), which would otherwise have silently regressed this PR's space verifiers. Before the merge, `SpaceDidDocumentResolverExtensions` had its own private `FindMultikey` helper used whenever a token named a `kid`, so a legacy-key DID document would verify through the no-`kid` path but be wrongly refused through the `kid` path post-merge. The merge commit fixes this correctly: both `ResolveAccountKeyAsync` and `ResolveAuthorityKeyAsync` now route through the shared `DidDocument.GetVerificationKey`, and I confirmed that method (in `src/ATProtoNet/Identity/PlcClient.cs`) does read through `VerificationMethod.ToDidKey()`, which is where #98's legacy-key support lives. The commit also correctly converts the `FormatException` `GetVerificationKey` can throw on malformed key material into a `SpaceVerificationException` under the caller's own error name, rather than letting it escape as an unhandled 500 — consistent with the pattern used elsewhere in this PR (e.g. `CachingSpaceDidDocumentResolver.ResolveAsync`). Three new unit tests were added covering exactly this interaction (legacy `#atproto` delegation-token signer, legacy `#atproto_space` credential signer resolved by `kid`, and malformed key material surfaced as a failed verification with "malformed" in the message) — I checked these exist in `tests/ATProtoNet.Tests/Server/Spaces/SpaceTokenVerifierTests.cs` and `SpaceServerTestFixtures.cs` and match the stated intent. The two textual conflicts (CHANGELOG.md's `### Added` section, docs/index.md's Spaces line) were resolved additively with both sides kept, and the new CHANGELOG bullet documenting this merge-time key-resolution fix was added in the same commit. As a bonus, the trivial missing-period nit I flagged in my previous pass ("...checked against nothing. A proof...") is now fixed in this same CHANGELOG text. I wasn't able to run `dotnet build`/`dotnet test` in this session (worktree/build tooling required approval I didn't have), but the change is small, self-contained, well-reasoned, and backed by targeted tests. Both prior review passes' concerns remain resolved, and this merge commit introduces no new issues. Approving. --- ⏱ 0m 55s · 15 turns · tokens: 91.2k in / 3.9k out (+1548.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-101/run-350.md)

Grandiras merged commit 4c0b9b88c0 into main

2026-08-21 01:29:53 +00:00

Grandiras deleted branch claude/issue-91

2026-08-21 01:29:53 +00:00

Grandiras referenced this pull request from a commit

2026-08-21 01:29:53 +00:00

Claude: changes for issue #91 (#101)