Claude: changes for issue #89 #90

Merged
Grandiras merged 2 commits from claude/issue-89 into main 2026-08-20 21:16:05 +00:00
Collaborator

Closes #89.

Closes #89.
feat: add support for spaces (permissioned data)
All checks were successful
CI / pds-integration (pull_request) Successful in 25s
CI / build-and-test (pull_request) Successful in 56s
d53c33c069
Implements the client side of AT Protocol's permissioned data protocol
(proposal 0016), the second data protocol alongside public broadcast:
the same shape — DID-based authority, per-user repos, Lexicon-typed
records, applications crawling hosts — with an access perimeter around
it, called a space.

Protocol machinery under `Spaces/`:

- `SpaceUri` / `SpaceRecordUri` for the `at://.../space/...` grammar,
  where the URI's authority is the space authority that gates access
  while the record's authority remains its author.
- `LtHash`, the homomorphic set hash a permissioned repo commits to in
  place of an MST root, on a from-scratch BLAKE3 XOF implementation
  (.NET ships none, and the SDK carries no third-party crypto).
- `SpaceRepoCommit` / `SpaceCommitVerifier`: the signature covers only
  the context, never the digest, which is bound by a symmetric MAC — so
  a leaked commit is deniable and proves nothing about its contents.
- `SpaceRepoCar` for the two-root CAR, whose layout lets a consumer
  authenticate the whole index against the commit without reading a
  single record.
- `SpaceCredentialProvider` / `SpaceReader` for the DPoP-bound
  credential exchange, and `SpaceSyncer` for incremental sync with
  automatic full-state recovery on a digest mismatch.

Endpoints on `AtProtoClient.Space` (`com.atproto.space.*`) and
`AtProtoClient.SimpleSpace` (`com.atproto.simplespace.*`), plus
`AtProtoScopes.Space(...)` for `space:` OAuth scopes.

Also fixes two pre-existing bugs the feature depends on: DAG-CBOR map
keys were sorted bytewise rather than length-first, so every CID the SDK
computed for a record whose keys spanned more than one length disagreed
with the rest of the network; and DPoP proofs named the full request URL
in `htu` where RFC 9449 requires query and fragment stripped.

The LtHash digests, commit context encoding, and MAC are pinned against
values generated by the reference implementation's own libraries, the
BLAKE3 vectors come from the reference implementation, and the DAG-CBOR
ordering fix is pinned against a post fetched from a live PDS alongside
the CID that PDS reported for it.

closes #89

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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/315); 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-90/run-315.md) — updating every ~5 minutes while the task runs.
claude-reviewer requested changes 2026-08-20 21:10:58 +00:00
Dismissed
claude-reviewer left a comment

🛑 Verdict: request changes — blocking issues below.

This is a huge, well-documented PR implementing the permissioned-data (Spaces) protocol alongside two smaller unrelated-looking fixes bundled into the same CHANGELOG section (DAG-CBOR key ordering, DPoP htu normalization). The DPoP htu fix in DPoPProofGenerator.cs is correct per RFC 9449 §4.2 (strips query/fragment via GetLeftPart(UriPartial.Path)). The from-scratch BLAKE3 XOF implementation in Crypto/Blake3.cs was inspected against the reference algorithm structure (chunk state, compression rounds, message permutation, chaining-value stack folding, root output expansion) and looks structurally faithful — flags, round count (7 rounds/6 permutes), and G-function rotations all match the spec. I could not execute the 27-vector test suite the changelog references, so treat that as unverified rather than confirmed.

One concrete logic bug found by internal inspection of AtProtoScopes.Space(...): the actions parameter's default is SpaceAction.All, but the code elides emitting any action= query parameter when actions == SpaceAction.None (treating it the same as the All default), so a caller asking for zero record actions silently gets a scope string that grants full read/write. See inline finding.

Everything else in the visible diff (Lexicon clients, SimpleSpace models, docs, sample) is consistent, well-documented, and matches the stated design (e.g. manage's zero-value is also its documented default, so its elision logic is correct — only actions has the asymmetric default that causes the bug). The diff was truncated before I could review LtHash, SpaceRepoCommit/SpaceCommitVerifier, SpaceCredentialProvider, SpaceSyncer, SpaceRepoCar, and the DagCborEncoder fix referenced in the changelog, so those are unreviewed here.


⏱ 1m 45s · 3 turns · tokens: 83.8k in / 10.9k out (+230.6k cache-read)
📜 Full session transcript

🛑 **Verdict: request changes** — blocking issues below. This is a huge, well-documented PR implementing the permissioned-data (Spaces) protocol alongside two smaller unrelated-looking fixes bundled into the same CHANGELOG section (DAG-CBOR key ordering, DPoP `htu` normalization). The DPoP `htu` fix in `DPoPProofGenerator.cs` is correct per RFC 9449 §4.2 (strips query/fragment via `GetLeftPart(UriPartial.Path)`). The from-scratch BLAKE3 XOF implementation in `Crypto/Blake3.cs` was inspected against the reference algorithm structure (chunk state, compression rounds, message permutation, chaining-value stack folding, root output expansion) and looks structurally faithful — flags, round count (7 rounds/6 permutes), and G-function rotations all match the spec. I could not execute the 27-vector test suite the changelog references, so treat that as unverified rather than confirmed. One concrete logic bug found by internal inspection of `AtProtoScopes.Space(...)`: the `actions` parameter's default is `SpaceAction.All`, but the code elides emitting any `action=` query parameter when `actions == SpaceAction.None` (treating it the same as the `All` default), so a caller asking for zero record actions silently gets a scope string that grants full read/write. See inline finding. Everything else in the visible diff (Lexicon clients, SimpleSpace models, docs, sample) is consistent, well-documented, and matches the stated design (e.g. `manage`'s zero-value is also its documented default, so its elision logic is correct — only `actions` has the asymmetric default that causes the bug). The diff was truncated before I could review `LtHash`, `SpaceRepoCommit`/`SpaceCommitVerifier`, `SpaceCredentialProvider`, `SpaceSyncer`, `SpaceRepoCar`, and the `DagCborEncoder` fix referenced in the changelog, so those are unreviewed here. --- ⏱ 1m 45s · 3 turns · tokens: 83.8k in / 10.9k out (+230.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-90/run-315.md) <!-- claude-verdict:request_changes --> <!-- claude-reviewed-head:d53c33c0696858f08799a3d437d06bdac87924fb -->
@ -152,0 +311,4 @@
if (collections is not null)
{
foreach (var collection in NormalizeCollections(collections))
AppendParam(sb, ref hasParams, "collection", collection);
Collaborator

SpaceAction.None is silently treated the same as SpaceAction.All here: the guard if (actions is not SpaceAction.All and not SpaceAction.None) skips emitting any action= parameter when actions == SpaceAction.None, producing the exact same scope string as the All default (per the method's own doc: an omitted action list "Defaults to SpaceAction.All").

Concretely, AtProtoScopes.Space("com.example.foo", actions: SpaceAction.None, manage: SpaceManage.Update) yields space:com.example.foo?manage=update — which per the documented server-side default grants full read/create/update/delete on records, the opposite of the caller's intent (a manage-only, zero-record-access grant). This is an over-broad OAuth scope bug with real security impact, not just a doc nit.

Note the asymmetry with manage: SpaceManage.None is also manage's documented default, so eliding it there is correct — it's specifically actions having a non-None default (All) that makes eliding None wrong. Consider special-casing None to emit an explicit empty/no-op marker (or simply not special-casing it out of the emission block) so a caller can express "grant nothing here."

`SpaceAction.None` is silently treated the same as `SpaceAction.All` here: the guard `if (actions is not SpaceAction.All and not SpaceAction.None)` skips emitting any `action=` parameter when `actions == SpaceAction.None`, producing the exact same scope string as the `All` default (per the method's own doc: an omitted action list "Defaults to `SpaceAction.All`"). Concretely, `AtProtoScopes.Space("com.example.foo", actions: SpaceAction.None, manage: SpaceManage.Update)` yields `space:com.example.foo?manage=update` — which per the documented server-side default grants full read/create/update/delete on records, the opposite of the caller's intent (a manage-only, zero-record-access grant). This is an over-broad OAuth scope bug with real security impact, not just a doc nit. Note the asymmetry with `manage`: `SpaceManage.None` is *also* `manage`'s documented default, so eliding it there is correct — it's specifically `actions` having a non-`None` default (`All`) that makes eliding `None` wrong. Consider special-casing `None` to emit an explicit empty/no-op marker (or simply not special-casing it out of the emission block) so a caller can express "grant nothing here."
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/316); the reply will link the full session transcript. <!-- claude-run:comment --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-90/run-316.md) — updating every ~5 minutes while the task runs.
fix: reject SpaceAction.None instead of widening it to a full grant
All checks were successful
CI / pds-integration (pull_request) Successful in 21s
CI / build-and-test (pull_request) Successful in 52s
Sync Closures to GitHub / sync-closure (pull_request) Successful in 6s
177f72ba01
An omitted action list means SpaceAction.All in the scope grammar, and
AtProtoScopes.Space elided the action parameters for None as well as for
All — so asking for zero record actions produced the same scope string as
the default and handed back full read/write.

The grammar has no marker for an empty action list, so the request is
inexpressible rather than narrow: it now throws, pointing at
SpaceAction.ReadSelf as the narrowest record grant there is.

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

Pushed 177f72b to claude/issue-89 — PR #90 is updated.

The finding was right, and it's fixed

AtProtoScopes.Space(...) elided the action= parameters for SpaceAction.None on the same branch as SpaceAction.All. Since an omitted action list means the full default set, Space("com.example.foo", actions: SpaceAction.None, manage: SpaceManage.Update) produced space:com.example.foo?manage=update — a full read/create/update/delete grant handed to a caller asking for zero record access. Over-broad OAuth scope, exactly as described.

The fix is not to emit a marker, because the grammar has none: there is no way to write an empty action list, so "grant nothing here" is inexpressible rather than merely narrow. Emitting an invented action=none would either be ignored (same bug) or reject the whole scope at the authorization server. So Space(...) now throws ArgumentException(paramName: "actions") for SpaceAction.None, with a message pointing at SpaceAction.ReadSelf — the narrowest record grant that actually exists, and the one the manage-only example in the docs already uses.

The reviewer's note about the manage asymmetry is correct and I left that path alone: SpaceManage.None is manage's documented default, so eliding it is right.

Changed: AtProtoScopes.cs (guard + <exception> and <param> docs), one new test Space_NoActions_IsRejectedRatherThanSilentlyWidened, a paragraph in docs/spaces.md, and the AtProtoScopes.Space(...) bullet in CHANGELOG.md amended in place (same unreleased feature, so not a separate Fixed entry).

Verified: dotnet build clean, 0 warnings; dotnet test tests/ATProtoNet.Tests/ → 1511 passed, 0 failed, 5 skipped (Aspire manifest tests, skipped before this change too).

Not changed

The other review points needed no action. The DPoP htu fix and the DAG-CBOR ordering fix were confirmed or unchallenged; the reviewer's uncertainty about BLAKE3 and the truncated-diff files was "unverified," not "wrong" — the 27 reference vectors and the pinned LtHash/commit/MAC values do run in the suite above.

Filed separately

AppendRepoActions (AtProtoScopes.cs:567) has the identical defect for RepoAction.NoneRepo("app.bsky.feed.post", RepoAction.None) returns repo:app.bsky.feed.post, a full write grant. I did not fix it here: Repo is shipped public API, so turning it into a throw is a behavioural break that needs its own Breaking changes entry rather than being folded into a spaces PR.


⏱ 2m 38s · 17 turns · tokens: 39.1k in / 7.2k out (+725.6k cache-read)
📜 Full session transcript
→ Filed follow-up issue #94: AtProtoScopes.Repo silently widens RepoAction.None to a full write grant

Pushed `177f72b` to `claude/issue-89` — PR #90 is updated. ## The finding was right, and it's fixed `AtProtoScopes.Space(...)` elided the `action=` parameters for `SpaceAction.None` on the same branch as `SpaceAction.All`. Since an omitted action list means the full default set, `Space("com.example.foo", actions: SpaceAction.None, manage: SpaceManage.Update)` produced `space:com.example.foo?manage=update` — a full read/create/update/delete grant handed to a caller asking for zero record access. Over-broad OAuth scope, exactly as described. The fix is **not** to emit a marker, because the grammar has none: there is no way to write an empty action list, so "grant nothing here" is inexpressible rather than merely narrow. Emitting an invented `action=none` would either be ignored (same bug) or reject the whole scope at the authorization server. So `Space(...)` now throws `ArgumentException(paramName: "actions")` for `SpaceAction.None`, with a message pointing at `SpaceAction.ReadSelf` — the narrowest record grant that actually exists, and the one the manage-only example in the docs already uses. The reviewer's note about the `manage` asymmetry is correct and I left that path alone: `SpaceManage.None` *is* `manage`'s documented default, so eliding it is right. Changed: `AtProtoScopes.cs` (guard + `<exception>` and `<param>` docs), one new test `Space_NoActions_IsRejectedRatherThanSilentlyWidened`, a paragraph in `docs/spaces.md`, and the `AtProtoScopes.Space(...)` bullet in `CHANGELOG.md` amended in place (same unreleased feature, so not a separate `Fixed` entry). **Verified:** `dotnet build` clean, 0 warnings; `dotnet test tests/ATProtoNet.Tests/` → 1511 passed, 0 failed, 5 skipped (Aspire manifest tests, skipped before this change too). ## Not changed The other review points needed no action. The DPoP `htu` fix and the DAG-CBOR ordering fix were confirmed or unchallenged; the reviewer's uncertainty about BLAKE3 and the truncated-diff files was "unverified," not "wrong" — the 27 reference vectors and the pinned LtHash/commit/MAC values do run in the suite above. ## Filed separately `AppendRepoActions` (`AtProtoScopes.cs:567`) has the identical defect for `RepoAction.None` — `Repo("app.bsky.feed.post", RepoAction.None)` returns `repo:app.bsky.feed.post`, a full write grant. I did not fix it here: `Repo` is shipped public API, so turning it into a throw is a behavioural break that needs its own `Breaking changes` entry rather than being folded into a spaces PR. --- ⏱ 2m 38s · 17 turns · tokens: 39.1k in / 7.2k out (+725.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-90/run-316.md) → Filed follow-up issue [#94](https://git.grandiras.net/Grandiras/ATProto.NET/issues/94): AtProtoScopes.Repo silently widens RepoAction.None to a full write grant
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/317); 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-90/run-317.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

Verdict: approve — no blocking issues found.

Re-review: the blocking issue from my previous pass is fixed. AtProtoScopes.Space(...) now explicitly throws ArgumentException when actions == SpaceAction.None instead of silently falling through and emitting the same scope string as the SpaceAction.All default — the over-broad-grant bug is gone, and the fix is documented consistently in the XML doc comments, the changelog entry, and docs/spaces.md ("SpaceAction.None is rejected rather than emitted").

Spot-checked the rest of the newly-visible diff for regressions: NormalizeCollections (dedup/sort, * short-circuit), the manage flag emission (unchanged, still correctly using None as its own default so no elision bug there), DPoPProofGenerator.NormalizeHtu (still correct per RFC 9449 §4.2), and the SpaceClient/SimpleSpaceClient XRPC surface (parameter building, pagination, overloads) all look consistent and correct.

As before, the diff is truncated before reaching LtHash, SpaceRepoCommit/SpaceCommitVerifier, SpaceCredentialProvider, SpaceSyncer, SpaceRepoCar, and the DagCborEncoder map-key-ordering fix referenced in the changelog — those remain unreviewed by me across both passes. Nothing in the visible diff blocks merging.


⏱ 0m 25s · 3 turns · tokens: 71.1k in / 1.9k out (+230.6k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: the blocking issue from my previous pass is fixed. `AtProtoScopes.Space(...)` now explicitly throws `ArgumentException` when `actions == SpaceAction.None` instead of silently falling through and emitting the same scope string as the `SpaceAction.All` default — the over-broad-grant bug is gone, and the fix is documented consistently in the XML doc comments, the changelog entry, and `docs/spaces.md` ("`SpaceAction.None` is rejected rather than emitted"). Spot-checked the rest of the newly-visible diff for regressions: `NormalizeCollections` (dedup/sort, `*` short-circuit), the `manage` flag emission (unchanged, still correctly using `None` as its own default so no elision bug there), `DPoPProofGenerator.NormalizeHtu` (still correct per RFC 9449 §4.2), and the `SpaceClient`/`SimpleSpaceClient` XRPC surface (parameter building, pagination, overloads) all look consistent and correct. As before, the diff is truncated before reaching `LtHash`, `SpaceRepoCommit`/`SpaceCommitVerifier`, `SpaceCredentialProvider`, `SpaceSyncer`, `SpaceRepoCar`, and the `DagCborEncoder` map-key-ordering fix referenced in the changelog — those remain unreviewed by me across both passes. Nothing in the visible diff blocks merging. --- ⏱ 0m 25s · 3 turns · tokens: 71.1k in / 1.9k out (+230.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-90/run-317.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:177f72ba01fc02d9af6dd0ac786da8fb0092574b -->
Grandiras deleted branch claude/issue-89 2026-08-20 21:16:05 +00:00
Grandiras referenced this pull request from a commit 2026-08-20 21:16:05 +00:00
Sign in to join this conversation.
No description provided.