Claude: changes for issue #89 #90
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
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!90
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-89"
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 #89.
🤖 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: 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
htunormalization). The DPoPhtufix inDPoPProofGenerator.csis correct per RFC 9449 §4.2 (strips query/fragment viaGetLeftPart(UriPartial.Path)). The from-scratch BLAKE3 XOF implementation inCrypto/Blake3.cswas 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(...): theactionsparameter's default isSpaceAction.All, but the code elides emitting anyaction=query parameter whenactions == SpaceAction.None(treating it the same as theAlldefault), 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 — onlyactionshas the asymmetric default that causes the bug). The diff was truncated before I could reviewLtHash,SpaceRepoCommit/SpaceCommitVerifier,SpaceCredentialProvider,SpaceSyncer,SpaceRepoCar, and theDagCborEncoderfix 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
@ -152,0 +311,4 @@if (collections is not null){foreach (var collection in NormalizeCollections(collections))AppendParam(sb, ref hasParams, "collection", collection);SpaceAction.Noneis silently treated the same asSpaceAction.Allhere: the guardif (actions is not SpaceAction.All and not SpaceAction.None)skips emitting anyaction=parameter whenactions == SpaceAction.None, producing the exact same scope string as theAlldefault (per the method's own doc: an omitted action list "Defaults toSpaceAction.All").Concretely,
AtProtoScopes.Space("com.example.foo", actions: SpaceAction.None, manage: SpaceManage.Update)yieldsspace: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.Noneis alsomanage's documented default, so eliding it there is correct — it's specificallyactionshaving a non-Nonedefault (All) that makes elidingNonewrong. Consider special-casingNoneto 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."🤖 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.
Pushed
177f72btoclaude/issue-89— PR #90 is updated.The finding was right, and it's fixed
AtProtoScopes.Space(...)elided theaction=parameters forSpaceAction.Noneon the same branch asSpaceAction.All. Since an omitted action list means the full default set,Space("com.example.foo", actions: SpaceAction.None, manage: SpaceManage.Update)producedspace: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=nonewould either be ignored (same bug) or reject the whole scope at the authorization server. SoSpace(...)now throwsArgumentException(paramName: "actions")forSpaceAction.None, with a message pointing atSpaceAction.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
manageasymmetry is correct and I left that path alone:SpaceManage.Noneismanage's documented default, so eliding it is right.Changed:
AtProtoScopes.cs(guard +<exception>and<param>docs), one new testSpace_NoActions_IsRejectedRatherThanSilentlyWidened, a paragraph indocs/spaces.md, and theAtProtoScopes.Space(...)bullet inCHANGELOG.mdamended in place (same unreleased feature, so not a separateFixedentry).Verified:
dotnet buildclean, 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
htufix 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 forRepoAction.None—Repo("app.bsky.feed.post", RepoAction.None)returnsrepo:app.bsky.feed.post, a full write grant. I did not fix it here:Repois shipped public API, so turning it into a throw is a behavioural break that needs its ownBreaking changesentry 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
🤖 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.
Re-review: the blocking issue from my previous pass is fixed.
AtProtoScopes.Space(...)now explicitly throwsArgumentExceptionwhenactions == SpaceAction.Noneinstead of silently falling through and emitting the same scope string as theSpaceAction.Alldefault — the over-broad-grant bug is gone, and the fix is documented consistently in the XML doc comments, the changelog entry, anddocs/spaces.md("SpaceAction.Noneis rejected rather than emitted").Spot-checked the rest of the newly-visible diff for regressions:
NormalizeCollections(dedup/sort,*short-circuit), themanageflag emission (unchanged, still correctly usingNoneas its own default so no elision bug there),DPoPProofGenerator.NormalizeHtu(still correct per RFC 9449 §4.2), and theSpaceClient/SimpleSpaceClientXRPC 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 theDagCborEncodermap-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