refactor: debloat the auth and server layers #206
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!206
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-204-auth-server-debloat"
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 #204.
A debloat of
src/ATProtoNet/Authandsrc/ATProtoNet.Server(outsideSpaces/), with no behaviour or public API change (PublicAPI.*.txtuntouched): net −203 lines across 28 files. A duplicate scan and a dead-member scan found nothing real to remove, so this is mostly owner-style compression.OAuthClient.Discovery/HttpClient,RefreshCoordinator,SessionManager.Store).AtProtoServiceAuthHandler,XrpcEndpointRegistration,KeyedLock<T>.Lease).OAuthClientform callbacks. The five() => { form; AddClientAuthentication; return form; }lambdas are gone, andPostFormWithDpopAsynctakes(form, clientKey, audience).AddClientAuthenticationruns at the top of each attempt and overwritesclient_id/client_assertion_type/client_assertion, so theuse_dpop_nonceretry still sends a fresh assertion with a new jti, as before. The DPoP proof is still generated per attempt with the current nonce.Security-adjacent spots to review are the guard brace removals in
ServiceAuthVerifier,SessionManager,AtProtoClientFactory,DPoPNonceCacheandAtProtoSessionJson, plus theOAuthClientform change above.Build has 0 warnings; 4964 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.
✅ Verdict: approve — no blocking issues found.
This is safe to merge. Verified the security-adjacent changes specifically:
AddClientAuthenticationnow runs at the top of every attempt inSendFormWithDpopAsync, overwritingclient_id,client_assertion_typeandclient_assertionbefore the request is built each iteration — so theuse_dpop_nonceretry still sends a fresh assertion with a new jti, matching the old per-attemptbuildForm()behavior. All five call sites pass the same client key and issuer the inline lambdas did. The DPoP proof is still generated per attempt with the current nonce.AtProtoServiceAuthHandler,XrpcEndpointRegistration,KeyedLock<T>.Lease): theAuthenticationHandlerbase invocation matches the old one; theXrpcEndpointRegistrationparameters only feed property initializers so nothing is captured long-lived; theLeasedoes exactly the oldentry.Semaphore.Release(); owner.Forget(key, entry)with the same dispose-once guard.OAuthClient.Discovery/HttpClient,RefreshCoordinator,SessionManager.Store): assigned once in construction and used read-only throughout; all former_store/_refreshCoordinator/_discovery/_httpClientreferences were rewritten consistently, including theStore!path that relies on the same non-null invariant as before.ServiceAuthVerifier,SessionManager,AtProtoClientFactory,DPoPNonceCache,AtProtoSessionJsonand the Server layer: each was a single-statement block (including the conditionalthrowexpression insidereturn); theawait usinglease scopes inSessionManagerand theleaseHeldif/else remain correct.Microsoft.Extensions.HostinginAtProtoBuilderExtensions,Microsoft.Extensions.DependencyInjection.ExtensionsinAtProtoServiceAuthentication).No behavior or public API change detected;
PublicAPI.*.txtuntouched as claimed.⏱ 3m 17s · 9 steps · $0.0147 · tokens: 53.7k in / 1.7k out (+143.7k cache-read) · Go: 5h 1% · wk 0% · mo 91%
📜 Full session transcript
🤖 Resolving the merge conflicts — the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the fix runs.
Merge committed. ✅
Conflict:
CHANGELOG.mdonly — both sides had added an entry under the## [Unreleased] → Changedsection: main's "Leaner Lexicon clients" bullet (#196) and this branch's "Leaner auth and server layers" bullet (#204).Resolution: kept both bullets in the unreleased section.
Verification: full solution builds with 0 errors/0 warnings, and the unit test suite passes (4964 passed, 0 failed). No conflict markers remain; the merge commit is in place and not pushed.
→ Merge resolved and pushed; a re-review follows.
⏱ 1m 20s · 8 steps · $0.0064 · tokens: 24.8k in / 703 out (+66.9k cache-read) · Go: 5h 1% · wk 0% · mo 91%
📜 Full session transcript
Independent adversarial review (read-only Sonnet reviewer): no defects found. Every brace removal covers a single-statement body, and no
elsebinding changed.AddClientAuthenticationoverwrites through the indexer at the top of each attempt, so every attempt, including theuse_dpop_nonceretry, gets a fresh assertion (no jti reuse). PAR fields, DPoP/nonce handling and theServiceAuthVerifierchecks are unchanged. Unit suite: 4964 passed.🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
💬 Verdict: comment — the reviewer did not submit a structured review; the raw output follows.
The workspace matches the PR head. Let me verify the security-adjacent spots in their current state, since new commits have landed since my last pass.
⏱ 30m 49s · 6 steps · $0.0131 · tokens: 72.5k in / 385 out (+58.8k cache-read)
📜 Full session transcript
@claude /review
🤖 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.
This is a behaviour-preserving refactor, and I found no problems.
AddClientAuthenticationsetsclient_id,client_assertion_typeandclient_assertionby indexer. Calling it at the top of each attempt on the shared dictionary therefore overwrites the old values, and theuse_dpop_nonceretry still gets a fresh assertion. The DPoP proof is still generated per attempt. The dictionary is not reused across calls, so there is no cross-request leakage.ServiceAuthVerifier,SessionManager,AtProtoClientFactory,DPoPNonceCacheandAtProtoSessionJson. Each guard still covers a single statement (athrow,returnorcontinue). The nestedforeach/ifandif/elsecases without braces also bind correctly.XrpcEndpointRegistrationprimary-constructor properties keep the private-constructor intent, because the type is internal.Minor, non-blocking style point: the unbraced
foreachwith a nestedifthat spans several lines (for example inAtProtoServiceAuthOptions.ValidateandOAuthClient) is harder to read and easier to break later. Braces on those would be safer.🧠 claude-sonnet-5-5 · ⏱ 0m 10s · 3 turns · tokens: 41.3k in / 877 out (+64.8k cache-read)
📜 Full session transcript
🤖 Resolving the merge conflicts — the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the fix runs.
The merge is committed (
a3c80b31). Confirmed the merge base (0ba4a207) predates both the branch's auth/server debloat and main's HTTP/2 identity work; the earlier merge owner (61061559) resolved the previous CHANGELOG only on this branch, and main then landed #198 which had never seen it. The new conflict is again the[Unreleased] / Changedbullet list, and both entries belong in it.Resolved CHANGELOG.md by keeping both bullets,
Leaner auth and server layersfirst, thenIdentity fetches use HTTP/2. No other files conflicted; the staged identity-side files are fine as main landed them, since the branch side doesn't touch them.Verified with a clean build of
src/ATProtoNet(0 errors, 0 warnings) and the matching Identity test suite undertests/ATProtoNet.Tests(--filter FullyQualifiedName~Identity: 1084 tests, all passing).→ Merge resolved and pushed; a re-review follows.
⏱ 26m 10s · 12 steps · $0.0194 · tokens: 95.4k in / 776 out (+133.6k cache-read) · Go: 5h 0% · wk 0% · mo 91%
📜 Full session transcript
🤖 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 refactor holds up, and I have no blocking issues. I reviewed the diff only; I did not build it or run the tests.
My earlier pass was an approval with one non-blocking style note, so there was nothing to confirm as resolved.
OAuthClientform change:AddClientAuthenticationnow runs at the top of each attempt inSendFormWithDpopAsync. It overwritesclient_id,client_assertion_typeandclient_assertionby indexer, so theuse_dpop_nonceretry still sends a fresh assertion with a new jti. The DPoP proof is still generated per attempt with the current nonce. Each call builds its own dictionary, so nothing carries over between requests.ServiceAuthVerifier,SessionManager,AtProtoClientFactory,DPoPNonceCacheandAtProtoSessionJson. Each guard still covers exactly one statement. The nestedforeach/ifandif/elsecases without braces also bind as before.AtProtoServiceAuthHandler,XrpcEndpointRegistrationandKeyedLock<T>.Leasekeep the same behaviour. The type-level access is unchanged and the public API files are untouched.Non-blocking, repeated from last time: the unbraced
foreachwith a nested multi-lineif/throwis easy to break with a later edit. Examples areAtProtoServiceAuthOptions.Validate,ServiceAuthVerifierOptionsandOAuthClient.ValidateClientKeys. Keeping braces on those would be safer, but it need not hold up the merge.🧠 claude-sonnet-5-5 · ⏱ 0m 16s · 3 turns · tokens: 42.2k in / 1.6k out (+65.6k cache-read)
📜 Full session transcript
✅ Verdict: approve — no blocking issues found.
Re-review of the current head: the behaviour-preserving refactor holds up and is safe to merge. Verified the security-adjacent spots specifically:
AddClientAuthenticationruns at the top of each attempt inSendFormWithDpopAsync, overwritingclient_id,client_assertion_typeandclient_assertion, so theuse_dpop_nonceretry still sends a fresh assertion with a new jti, exactly as the old per-attemptbuildForm()lambdas did. All five call sites pass the same client key and issuer; the DPoP proof is still generated per attempt with the current nonce, and the dictionary is never reused across requests.ServiceAuthVerifier(including the conditionalreturn/throwinReadKeyId),SessionManager(theawait usinglease scopes remain correctly scoped, including the nested two-condition guard),AtProtoClientFactory,DPoPNonceCache,AtProtoSessionJsonand the Server layer: each was a single-statement block; no condition changed.OAuthClient.Discovery/HttpClient,RefreshCoordinator,SessionManager.Store) preserve construction-time assignment and read-only use; theStore!path keeps the same non-null invariant as before.PublicAPI.*.txtuntouched as claimed, so no public API change.Non-blocking style note: the multi-line unbraced
foreach/nestedifblocks (e.g.AtProtoServiceAuthOptions.Validate,OAuthClient.FindClientKey) are harder to scan and easier to break with a later edit; braces on those would be more robust, but that is a matter of taste and does not block merging.⏱ 3m 33s · 5 steps · $0.0085 · tokens: 33.6k in / 1.0k out (+80.9k cache-read) · Go: 5h 0% · wk 0% · mo 91%
📜 Full session transcript