refactor(auth): shared JWT, DPoP and base64url primitives #151

Merged

Grandiras merged 2 commits from issue-118-primitives into main 2026-09-24 18:58:33 +00:00

Conversation 5 Commits 2 Files changed 33 +2009 -614

Grandiras commented 2026-09-24 18:53:33 +00:00

Owner

Copy link

Closes #118.

Core and Server each had their own base64url, JWT signing and parsing, RFC 7638 thumbprint and DPoP htu normalization, and the client and server DPoP copies had drifted. Each primitive now has one internal implementation in core, which the Server uses through InternalsVisibleTo.

Two commits: the characterization tests first (test:, green against the old code), then the refactor.

What changed

Shared primitives

• InternalsVisibleTo ATProtoNet.Server on src/ATProtoNet/ATProtoNet.csproj. This is the only csproj change.
• base64url: System.Buffers.Text.Base64Url is used everywhere. That replaces 6 encoders and 5 decoders, including the internal Base64Url class in PlcOperationBuilder that shadowed the BCL type. Decoding goes through Jwt.TryDecodeBase64Url, a strict wrapper (details under Security below).
• Jwt (src/ATProtoNet/Auth/Jwt.cs, internal) has EncodeHeader, Sign(encodedHeader, payload, AtProtoKey), TryDecode, WriteTokenId and a GetStringOrNull extension. They replace the signers in DPoPProofGenerator, ServiceAuthGenerator and SpaceTokens.Create, and the parsers in SpaceTokens.Parse, DPoPProofValidator and SpaceServiceAuthVerifier. Each caller keeps its own exception type, its message prefix and every rationale comment.
• One jti scheme for every token: 128 bits from the system CSPRNG as 32 lower-case hex characters. DPoP and service auth used Guid.NewGuid().ToString("N"), which has the same shape.
• The alg label comes from KeyCurve.JwsAlgorithm(), which #150 added. It is now called only from Jwt.EncodeHeader.

DPoP

• DPoP (src/ATProtoNet/Auth/OAuth/DPoP.cs, internal) has NormalizeHtu, AccessTokenHash, the RFC 7638 Thumbprint and TokenType. The generator and DPoPProofValidator both use it, so the two thumbprint implementations and the three SHA-256→base64url copies are gone.
• htu drift, resolved the RFC way. htu is scheme://host[:port]/path: scheme and host lower-cased, no default port, and no query, fragment or userinfo (RFC 9449 §4.2 and §4.3, and RFC 9110 §4.2.4 for userinfo). It is implemented with Uri.GetComponents(SchemeAndServer | Path, UriEscaped). The client used to emit user:pw@ and so sent the credentials inside the proof. The server already stripped userinfo, and its output is unchanged for every other input I probed (IPv6, IDN, percent-encoding, dot segments). GenerateProof now throws ArgumentException for a URL that is not absolute with a host. Before, it sent the URL verbatim, and on Linux /xrpc/a became file:///xrpc/a.
• The K-256 mislabel (1.11) is fixed by refusing the key. new DPoPProofGenerator(byte[]) now throws ArgumentException for any key that is not P-256, because atproto DPoP is ES256 only. The generator wraps an AtProtoKey built from the imported ECDsa, keeps a typed JsonWebKey, and computes its thumbprint once.
• The validator deserializes the embedded key into JsonWebKey after the d-present check, which still runs first. JsonWebKeyVerifier.Verify(JsonElement…) is removed. JsonWebKeyVerifier stays separate from AtProtoCrypto, with its rationale comment unchanged, because JWS must accept high-S signatures.
• Allocation-light proofs: the header is pre-encoded once per key, ath is cached per access token, the payload is written with Utf8JsonWriter, and the signing input and output are built in one pooled buffer. The same pattern now applies to ServiceAuthGenerator and SpaceTokens.Create.

Space tokens

• SpaceTokens.Verify(SpaceToken parsed, …) is a new public overload. SpaceDelegationTokenVerifier and SpaceCredentialVerifier use it, so each token is parsed once.

Bounded reader

• BoundedContent.ReadBoundedAsync (src/ATProtoNet/Auth/OAuth/BoundedContent.cs, internal) replaces LengthLimitedStream in HttpSpaceClientMetadataResolver and ReadCappedStringAsync in AuthorizationServerDiscovery. It refuses a declared Content-Length over the cap before reading, reads at most cap + 1 bytes, and grows its buffer from the declared length instead of allocating the whole cap up front. The old attestation path would have streamed 256 KiB. Exceptions are unchanged: an oversized document is still SpaceVerificationException(InvalidClientAttestation) or an ignored atproto-did answer.

Security

• Stricter base64url at every JWT and JWK decoder. The standard alphabet's + and /, embedded whitespace (which Base64Url alone would skip) and non-zero trailing bits are rejected, so a token has exactly one spelling apart from padding. Correct padding is still accepted, as it was before, and is pinned by tests. A token's header and claims must be JSON objects, and the service-auth verifier now decodes the header as well.

Breaking changes

None binary- or source-breaking. The removed members were all internal: DPoPProofGenerator.Base64UrlEncode, ComputeS256Hash and NormalizeHtu; DPoPProofValidator.NormalizeUri; the Identity.Base64Url class; and JsonWebKeyVerifier.Verify(JsonElement…). Three behaviour changes are listed under Fixed and Security in the CHANGELOG:

• A non-P-256 key passed to DPoPProofGenerator(byte[]) throws ArgumentException.
• GenerateProof throws ArgumentException for a URL that is not absolute with a host.
• Base64url is strict.

Verification

• Unit tests: 2711 total, 2706 passed, 0 failed, 5 skipped (the environment-gated ones). The full solution build (--no-incremental) has 0 warnings and 0 errors.

• Characterization first. Commit 1 adds 85 test cases that pass against the unchanged source. That commit on its own runs 2636 tests: 2631 passed, 5 skipped. The refactor commit adds 75 more.

  • DPoP interop: DPoPProofGenerator output validated by DPoPProofValidator, with a bound thumbprint and ath. This covers the credential-exchange shape, a query-string URL, the wrong token, the wrong key, export/import and fresh jtis.
  • Base64url: padded (each segment and all three), unpadded and undecodable input at every decoder entry point. That is SpaceTokens, DPoPProofValidator, the service-auth verifier, JWK coordinates on both JsonWebKeyVerifier paths, and the bearer handler's pre-check (through the handler, with no network).
  • JWT: valid, expired, wrong-audience, wrong-key and malformed tokens for SpaceTokens and the service-auth verifier.
  • Size caps: HttpSpaceClientMetadataResolver's ceiling, which had no tests, declared, undeclared and at the exact boundary, for both the metadata and the JWKS fetch. Also the exact 1024/1025 boundary of the atproto-did cap.

• Pinned RFC vectors. Both example DPoP proofs from RFC 9449 validate through DPoPProofValidator with a fixed clock: §4.1 Figure 2, the token request, and §7.1 Figure 13, the resource request with ath and a bound jkt. Their signatures verify, which I checked independently before using them. The §6.1 thumbprint 0ZcOCORZNYy-DWpqq30jZyJGHTN0d2HglBV3uiguA4I and the §7.1 ath fUHyO2r2Z3DZ53EsNrWBb0xWXoaNy59IiKCAqksmQEo are asserted against DPoP.Thumbprint, DPoP.AccessTokenHash and the generator's own ath.

• Regression tests for the K-256 refusal, htu without userinfo (generator claim and interop), relative-URL rejection, ath cache invalidation when the token changes, and standard-alphabet rejection. Plus unit tests for Jwt (decode table, malformed reasons, sign/verify, ES256K label, jti shape), DPoP.NormalizeHtu, and ReadBoundedAsync, including the buffer-growth path and an understated Content-Length.

• Benchmarks. BenchmarkDotNet 0.14 ShortRun, in-process, .NET 10.0.12, Ryzen 5 7600X. "Old" is the origin/main code copied verbatim:

  Operation	Old	New
  DPoP proof with ath and nonce	18.78 µs / 10.9 KB	15.65 µs / 2.48 KB
  ServiceAuthGenerator.CreateToken	16.26 µs / 5.47 KB	15.45 µs / 1.62 KB
  SpaceTokens.Create (credential)	16.53 µs / 6.37 KB	15.67 µs / 2.99 KB

  These match the F18 prototype (18.6 → 15.0 µs, 11.2 → 2.1 KB). The remaining gap to the prototype comes from AtProtoKey.Sign returning a new array.

Deliberately deferred or flagged

• Signing into a stack buffer. AtProtoKey.Sign returns a byte[] and applies low-S normalization (harmless for JWS). Signing into a span needs an AtProtoKey.TrySign(ReadOnlySpan<byte>, Span<byte>) in Crypto/*, so I left it for a follow-up there rather than reaching into the key's ECDsa.
• AtProtoAuthenticationHandler: only its base64url decoder moved to the shared one, and that is pinned by new tests. #136 deletes the class, so its JWT parsing was not rebuilt on Jwt.TryDecode.
• Remaining GetString copies in Streaming/JetstreamEventParser and JetstreamSegmentReader belong to #117's files. They can switch to GetStringOrNull later.
• Pre-existing bug, not fixed here (it changes verifier semantics): DateTimeOffset.FromUnixTimeSeconds throws ArgumentOutOfRangeException for an exp or iat outside years 1–9999. This affects SpaceTokens.Parse (so TryParse throws instead of returning false), DPoPProofValidator (iat) and SpaceServiceAuthVerifier (exp, iat). A crafted token therefore produces a 500 instead of a 401 on the space endpoints. There is no bypass. A TryGetNumericDate helper next to GetStringOrNull would fix it; #133 or #125 is the natural home.

🤖 Generated with Claude Code

Closes #118. Core and Server each had their own base64url, JWT signing and parsing, RFC 7638 thumbprint and DPoP `htu` normalization, and the client and server DPoP copies had drifted. Each primitive now has one internal implementation in core, which the Server uses through `InternalsVisibleTo`. Two commits: the characterization tests first (`test:`, green against the old code), then the refactor. ## What changed ### Shared primitives - **`InternalsVisibleTo ATProtoNet.Server`** on `src/ATProtoNet/ATProtoNet.csproj`. This is the only csproj change. - **base64url**: `System.Buffers.Text.Base64Url` is used everywhere. That replaces 6 encoders and 5 decoders, including the internal `Base64Url` class in `PlcOperationBuilder` that shadowed the BCL type. Decoding goes through `Jwt.TryDecodeBase64Url`, a strict wrapper (details under *Security* below). - **`Jwt`** (`src/ATProtoNet/Auth/Jwt.cs`, internal) has `EncodeHeader`, `Sign(encodedHeader, payload, AtProtoKey)`, `TryDecode`, `WriteTokenId` and a `GetStringOrNull` extension. They replace the signers in `DPoPProofGenerator`, `ServiceAuthGenerator` and `SpaceTokens.Create`, and the parsers in `SpaceTokens.Parse`, `DPoPProofValidator` and `SpaceServiceAuthVerifier`. Each caller keeps its own exception type, its message prefix and every rationale comment. - **One `jti` scheme** for every token: 128 bits from the system CSPRNG as 32 lower-case hex characters. DPoP and service auth used `Guid.NewGuid().ToString("N")`, which has the same shape. - **The `alg` label** comes from `KeyCurve.JwsAlgorithm()`, which #150 added. It is now called only from `Jwt.EncodeHeader`. ### DPoP - **`DPoP`** (`src/ATProtoNet/Auth/OAuth/DPoP.cs`, internal) has `NormalizeHtu`, `AccessTokenHash`, the RFC 7638 `Thumbprint` and `TokenType`. The generator and `DPoPProofValidator` both use it, so the two thumbprint implementations and the three SHA-256→base64url copies are gone. - **`htu` drift, resolved the RFC way.** `htu` is `scheme://host[:port]/path`: scheme and host lower-cased, no default port, and no query, fragment or userinfo (RFC 9449 §4.2 and §4.3, and RFC 9110 §4.2.4 for userinfo). It is implemented with `Uri.GetComponents(SchemeAndServer | Path, UriEscaped)`. The client used to emit `user:pw@` and so sent the credentials inside the proof. The server already stripped userinfo, and its output is unchanged for every other input I probed (IPv6, IDN, percent-encoding, dot segments). `GenerateProof` now throws `ArgumentException` for a URL that is not absolute with a host. Before, it sent the URL verbatim, and on Linux `/xrpc/a` became `file:///xrpc/a`. - **The K-256 mislabel (1.11) is fixed by refusing the key.** `new DPoPProofGenerator(byte[])` now throws `ArgumentException` for any key that is not P-256, because atproto DPoP is ES256 only. The generator wraps an `AtProtoKey` built from the imported `ECDsa`, keeps a typed `JsonWebKey`, and computes its thumbprint once. - **The validator deserializes the embedded key into `JsonWebKey`** after the `d`-present check, which still runs first. `JsonWebKeyVerifier.Verify(JsonElement…)` is removed. `JsonWebKeyVerifier` stays separate from `AtProtoCrypto`, with its rationale comment unchanged, because JWS must accept high-S signatures. - **Allocation-light proofs**: the header is pre-encoded once per key, `ath` is cached per access token, the payload is written with `Utf8JsonWriter`, and the signing input and output are built in one pooled buffer. The same pattern now applies to `ServiceAuthGenerator` and `SpaceTokens.Create`. ### Space tokens - **`SpaceTokens.Verify(SpaceToken parsed, …)`** is a new public overload. `SpaceDelegationTokenVerifier` and `SpaceCredentialVerifier` use it, so each token is parsed once. ### Bounded reader - **`BoundedContent.ReadBoundedAsync`** (`src/ATProtoNet/Auth/OAuth/BoundedContent.cs`, internal) replaces `LengthLimitedStream` in `HttpSpaceClientMetadataResolver` and `ReadCappedStringAsync` in `AuthorizationServerDiscovery`. It refuses a declared `Content-Length` over the cap before reading, reads at most cap + 1 bytes, and grows its buffer from the declared length instead of allocating the whole cap up front. The old attestation path would have streamed 256 KiB. Exceptions are unchanged: an oversized document is still `SpaceVerificationException(InvalidClientAttestation)` or an ignored `atproto-did` answer. ### Security - **Stricter base64url** at every JWT and JWK decoder. The standard alphabet's `+` and `/`, embedded whitespace (which `Base64Url` alone would skip) and non-zero trailing bits are rejected, so a token has exactly one spelling apart from padding. Correct padding is still accepted, as it was before, and is pinned by tests. A token's header and claims must be JSON objects, and the service-auth verifier now decodes the header as well. ## Breaking changes None binary- or source-breaking. The removed members were all internal: `DPoPProofGenerator.Base64UrlEncode`, `ComputeS256Hash` and `NormalizeHtu`; `DPoPProofValidator.NormalizeUri`; the `Identity.Base64Url` class; and `JsonWebKeyVerifier.Verify(JsonElement…)`. Three behaviour changes are listed under *Fixed* and *Security* in the CHANGELOG: - A non-P-256 key passed to `DPoPProofGenerator(byte[])` throws `ArgumentException`. - `GenerateProof` throws `ArgumentException` for a URL that is not absolute with a host. - Base64url is strict. ## Verification - Unit tests: **2711 total, 2706 passed, 0 failed, 5 skipped** (the environment-gated ones). The full solution build (`--no-incremental`) has **0 warnings and 0 errors**. - **Characterization first.** Commit 1 adds 85 test cases that pass against the unchanged source. That commit on its own runs 2636 tests: 2631 passed, 5 skipped. The refactor commit adds 75 more. - DPoP interop: `DPoPProofGenerator` output validated by `DPoPProofValidator`, with a bound thumbprint and `ath`. This covers the credential-exchange shape, a query-string URL, the wrong token, the wrong key, export/import and fresh `jti`s. - Base64url: padded (each segment and all three), unpadded and undecodable input at every decoder entry point. That is `SpaceTokens`, `DPoPProofValidator`, the service-auth verifier, JWK coordinates on both `JsonWebKeyVerifier` paths, and the bearer handler's pre-check (through the handler, with no network). - JWT: valid, expired, wrong-audience, wrong-key and malformed tokens for `SpaceTokens` and the service-auth verifier. - Size caps: `HttpSpaceClientMetadataResolver`'s ceiling, which had no tests, declared, undeclared and at the exact boundary, for both the metadata and the JWKS fetch. Also the exact 1024/1025 boundary of the `atproto-did` cap. - **Pinned RFC vectors.** Both example DPoP proofs from RFC 9449 validate through `DPoPProofValidator` with a fixed clock: §4.1 Figure 2, the token request, and §7.1 Figure 13, the resource request with `ath` and a bound `jkt`. Their signatures verify, which I checked independently before using them. The §6.1 thumbprint `0ZcOCORZNYy-DWpqq30jZyJGHTN0d2HglBV3uiguA4I` and the §7.1 `ath` `fUHyO2r2Z3DZ53EsNrWBb0xWXoaNy59IiKCAqksmQEo` are asserted against `DPoP.Thumbprint`, `DPoP.AccessTokenHash` and the generator's own `ath`. - **Regression tests** for the K-256 refusal, `htu` without userinfo (generator claim and interop), relative-URL rejection, `ath` cache invalidation when the token changes, and standard-alphabet rejection. Plus unit tests for `Jwt` (decode table, malformed reasons, sign/verify, ES256K label, `jti` shape), `DPoP.NormalizeHtu`, and `ReadBoundedAsync`, including the buffer-growth path and an understated `Content-Length`. - **Benchmarks.** BenchmarkDotNet 0.14 ShortRun, in-process, .NET 10.0.12, Ryzen 5 7600X. "Old" is the `origin/main` code copied verbatim: | Operation | Old | New | |---|---:|---:| | DPoP proof with `ath` and nonce | 18.78 µs / 10.9 KB | **15.65 µs / 2.48 KB** | | `ServiceAuthGenerator.CreateToken` | 16.26 µs / 5.47 KB | 15.45 µs / 1.62 KB | | `SpaceTokens.Create` (credential) | 16.53 µs / 6.37 KB | 15.67 µs / 2.99 KB | These match the F18 prototype (18.6 → 15.0 µs, 11.2 → 2.1 KB). The remaining gap to the prototype comes from `AtProtoKey.Sign` returning a new array. ## Deliberately deferred or flagged - **Signing into a stack buffer.** `AtProtoKey.Sign` returns a `byte[]` and applies low-S normalization (harmless for JWS). Signing into a span needs an `AtProtoKey.TrySign(ReadOnlySpan<byte>, Span<byte>)` in `Crypto/*`, so I left it for a follow-up there rather than reaching into the key's `ECDsa`. - **`AtProtoAuthenticationHandler`**: only its base64url decoder moved to the shared one, and that is pinned by new tests. #136 deletes the class, so its JWT parsing was not rebuilt on `Jwt.TryDecode`. - **Remaining `GetString` copies** in `Streaming/JetstreamEventParser` and `JetstreamSegmentReader` belong to #117's files. They can switch to `GetStringOrNull` later. - **Pre-existing bug, not fixed here** (it changes verifier semantics): `DateTimeOffset.FromUnixTimeSeconds` throws `ArgumentOutOfRangeException` for an `exp` or `iat` outside years 1–9999. This affects `SpaceTokens.Parse` (so `TryParse` throws instead of returning `false`), `DPoPProofValidator` (`iat`) and `SpaceServiceAuthVerifier` (`exp`, `iat`). A crafted token therefore produces a 500 instead of a 401 on the space endpoints. There is no bypass. A `TryGetNumericDate` helper next to `GetStringOrNull` would fix it; #133 or #125 is the natural home. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 2 commits 2026-09-24 18:53:33 +00:00

test: characterize JWT, DPoP, base64url and bounded-read behaviour (#118) ... 309995bbac

Pins what the auth primitives do today before #118 folds the copies into
one implementation each:

- DPoP interop: proofs from the client DPoPProofGenerator validated by the
  server DPoPProofValidator with a bound thumbprint and ath, plus the two
  RFC 9449 example proofs (sections 4.1 and 7.1) as fixed vectors.
- base64url: padded, unpadded and undecodable segments at every decoder
  entry point (SpaceTokens, DPoPProofValidator, the service-auth verifier,
  JWK coordinates on both JsonWebKeyVerifier paths, and the bearer handler).
- JWT: wrong audience, wrong key and malformed tokens for SpaceTokens and
  the service-auth verifier.
- Size caps: the client-metadata/JWKS ceiling (untested until now) and the
  exact boundary of the atproto-did cap.

TestJws mints tokens independently of the SDK encoder so the vectors do
not share code with what they test.

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

refactor(auth): shared JWT, DPoP and base64url primitives (closes #118) ... 98a5cd3f7d

Core and Server each rolled their own base64url, JWT signing and parsing,
RFC 7638 thumbprint and htu normalization, and the client and server DPoP
copies had drifted: the client kept user:pw@ in htu while the validator
stripped it, and a stored K-256 key signed proofs labelled ES256.

- Core grants InternalsVisibleTo to ATProtoNet.Server, so the Server uses
  the core primitives instead of copies.
- Jwt: EncodeHeader, Sign (over AtProtoKey), TryDecode, WriteTokenId and
  a strict TryDecodeBase64Url over System.Buffers.Text.Base64Url, plus a
  GetStringOrNull JSON helper. It replaces the 3 signers and 4 parsers;
  each caller keeps its own exception type and security rationale.
- DPoP: one NormalizeHtu (scheme://host[:port]/path, no userinfo),
  AccessTokenHash, RFC 7638 Thumbprint and TokenType for the generator
  and DPoPProofValidator. The validator deserializes the embedded key
  into JsonWebKey after the private-key check; JsonWebKeyVerifier stays
  separate from AtProtoCrypto because JWS must accept high-S signatures.
- DPoPProofGenerator wraps a P-256 AtProtoKey and refuses any other
  curve, keeps a typed JWK, computes its thumbprint once, pre-encodes its
  header and caches ath per access token. Proofs, service-auth tokens and
  space tokens write claims with Utf8JsonWriter and share one jti format.
- SpaceTokens.Verify(SpaceToken, ...) lets the delegation and credential
  verifiers parse each token once.
- One ReadBoundedAsync replaces LengthLimitedStream and the discovery
  copy; the caps are pinned by tests.

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

opencode-bot commented 2026-09-24 18:53:34 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — the review will link the full session transcript.

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

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-151/run-2026-09-24T18-53-34-061Z.md) — updating every ~5 minutes while the review runs.

Grandiras commented 2026-09-24 18:53:41 +00:00

Author

Owner

Copy link

@claude /review

@claude /review

claude-bot commented 2026-09-24 18:53:44 +00:00

Collaborator

Copy link

🤖 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/586); 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-151/run-586.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-09-24 18:57:01 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Solid, well-scoped refactor. I checked out the PR head (origin/issue-118-primitives) and read the actual resulting files rather than relying on the diff alone, and spot-checked the highest-risk pieces:

• DPoP.NormalizeHtu uses uri.GetComponents(UriComponents.SchemeAndServer | UriComponents.Path, UriFormat.UriEscaped). I verified (docs + source) that SchemeAndServer = Scheme | Host | Port and does not include UserInfo — so the claimed userinfo-stripping behavior is correct, not a regression. Default-port dropping and lower-casing of scheme/host also check out against .NET's Uri normalization behavior, matching the new DPoPTests/DPoPProofGeneratorTests cases (including the RFC 9449 vectors).
• Jwt.TryDecodeBase64Url correctly rejects the standard alphabet, embedded whitespace, and non-zero trailing bits, while accepting both padded and unpadded input — confirmed against the System.Buffers.Text.Base64Url API surface (available since .NET 9, so net10.0 is fine) and the test table in JwtTests.
• Jwt.Sign builds the token into a single pooled buffer with sizes computed from Base64Url.GetEncodedLength, and validates the returned signature is exactly 64 bytes (AtProtoKey.Sign always returns a normalized fixed-width r || s signature), so there's no buffer-size mismatch.
• DPoPProofGenerator's new private constructor path disposes the ECDsa on every failure branch of ImportP256 (including the P-256 curve check), so the K-256 rejection doesn't leak a handle.
• BoundedContent.ReadBoundedAsync's growth loop correctly detects an oversized body whether or not Content-Length is declared or truthful, and the boundary tests (Cap/Cap+1) line up with the implementation.
• Removed/replaced members (JsonWebKeyVerifier.Verify(JsonElement…), the old NormalizeUri, the ad hoc decoders) are gone cleanly, and every call site (DPoPProofValidator, SpaceServiceAuthVerifier, SpaceTokens, AtProtoAuthenticationHandler) was updated consistently with the new shared primitives and produces the same exception types/messages as before.

I didn't find correctness or security issues. The CHANGELOG entries are properly categorized (Added/Changed/Fixed/Security) and cite #118, and the deferred items (signing into a stack buffer, the pre-existing FromUnixTimeSeconds range bug) are reasonably scoped out with clear rationale rather than silently ignored. Characterization-tests-first approach is a good practice and the interop tests (client generator against server validator, plus RFC 9449 pinned vectors) give real confidence the two independently-written sides still agree after the refactor.

---

⏱ 3m 8s · 31 turns · tokens: 121.4k in / 17.7k out (+3712.0k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Solid, well-scoped refactor. I checked out the PR head (origin/issue-118-primitives) and read the actual resulting files rather than relying on the diff alone, and spot-checked the highest-risk pieces: - **`DPoP.NormalizeHtu`** uses `uri.GetComponents(UriComponents.SchemeAndServer | UriComponents.Path, UriFormat.UriEscaped)`. I verified (docs + source) that `SchemeAndServer = Scheme | Host | Port` and does *not* include `UserInfo` — so the claimed userinfo-stripping behavior is correct, not a regression. Default-port dropping and lower-casing of scheme/host also check out against .NET's `Uri` normalization behavior, matching the new `DPoPTests`/`DPoPProofGeneratorTests` cases (including the RFC 9449 vectors). - **`Jwt.TryDecodeBase64Url`** correctly rejects the standard alphabet, embedded whitespace, and non-zero trailing bits, while accepting both padded and unpadded input — confirmed against the `System.Buffers.Text.Base64Url` API surface (available since .NET 9, so `net10.0` is fine) and the test table in `JwtTests`. - **`Jwt.Sign`** builds the token into a single pooled buffer with sizes computed from `Base64Url.GetEncodedLength`, and validates the returned signature is exactly 64 bytes (`AtProtoKey.Sign` always returns a normalized fixed-width `r || s` signature), so there's no buffer-size mismatch. - **`DPoPProofGenerator`**'s new private constructor path disposes the `ECDsa` on every failure branch of `ImportP256` (including the P-256 curve check), so the K-256 rejection doesn't leak a handle. - **`BoundedContent.ReadBoundedAsync`**'s growth loop correctly detects an oversized body whether or not `Content-Length` is declared or truthful, and the boundary tests (`Cap`/`Cap+1`) line up with the implementation. - Removed/replaced members (`JsonWebKeyVerifier.Verify(JsonElement…)`, the old `NormalizeUri`, the ad hoc decoders) are gone cleanly, and every call site (`DPoPProofValidator`, `SpaceServiceAuthVerifier`, `SpaceTokens`, `AtProtoAuthenticationHandler`) was updated consistently with the new shared primitives and produces the same exception types/messages as before. I didn't find correctness or security issues. The CHANGELOG entries are properly categorized (Added/Changed/Fixed/Security) and cite #118, and the deferred items (signing into a stack buffer, the pre-existing `FromUnixTimeSeconds` range bug) are reasonably scoped out with clear rationale rather than silently ignored. Characterization-tests-first approach is a good practice and the interop tests (client generator against server validator, plus RFC 9449 pinned vectors) give real confidence the two independently-written sides still agree after the refactor. --- ⏱ 3m 8s · 31 turns · tokens: 121.4k in / 17.7k out (+3712.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-151/run-586.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:98a5cd3f7df381118ed56f01653c84a11be16339 -->

Grandiras merged commit 06becd0356 into main 2026-09-24 18:58:33 +00:00

Grandiras deleted branch issue-118-primitives 2026-09-24 18:58:33 +00:00

Grandiras referenced this pull request from a commit 2026-09-24 18:58:34 +00:00

refactor(auth): shared JWT, DPoP and base64url primitives (#151)

Grandiras referenced this pull request 2026-09-24 19:05:04 +00:00

refactor(http)!: one error model and a robust XRPC transport #152

opencode-reviewer approved these changes 2026-09-24 19:18:26 +00:00

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Solid consolidation refactor — the primitives are correct, the two behavior fixes are real, and nothing is broken by the consolidation. Reviewed the final state of all touched files.

Verified against the .NET runtime source, not just the tests:

• Jwt.TryDecodeBase64Url's strictness claims all hold: Base64Url.DecodeFromChars runs with ignoreWhiteSpace: true (so the SearchValues pre-check is genuinely required to reject "Y Q"), accepts correct trailing padding, and rejects misplaced padding, +// (invalid in its decoding map), and non-zero trailing bits (Base64Helper.DecodeFrom checks (i0 & 0x8000F000) != 0 → InvalidData). Padding acceptance/strictness is what keeps RFC 7515 compat while pinning one spelling — correctly implemented. GetMaxDecodedLength + bytesWritten slicing is right.
• Jwt.TryDecode's signing input is the exact ASCII of {header}.{payload} as sent (header/payload segments pass the alphabet check, so ASCII is exact, and padding — when present — is covered by the signature as sent). JSON-object enforcement on header and payload with per-caller exception types and messages preserved.
• Jwt.Sign's pooled-buffer arithmetic is correct (86-char signature for a 64-byte fixed r||s, matching the JWS shape), the signature-length check is sound for both ES256 and ES256K, and the buffer is returned in finally.
• Jwt.EncodeHeader embeds exactly kty/crv/x/y: the SDK's JsonWebKey carries [JsonPropertyName] and JsonIgnore(WhenWritingNull) on Crv/X/Y/Kid/Use/Alg, so JsonSerializer.Serialize produces the minimal RFC-shaped header.
• DPoP.NormalizeHtu is the RFC-correct shape: SchemeAndServer (bits 1|4|8) excludes userinfo (bit 2), query and fragment; the userinfo drop is justified by RFC 9110 §4.2.4 and is a real fix (the old GetLeftPart(UriPartial.Path) did include user:pw@). Both generator and validator call the one implementation, so comparisons are self-consistent; the RFC 9449 §7.1 vector pins the exact output. Every in-repo call site (OAuthClient, XrpcClient via new Uri(BaseAddress, RequestUri), SpaceCredentialProvider) passes absolute URLs, so the new ArgumentException only fires for genuinely malformed input, and the behavior change is documented in CHANGELOG and docs/oauth.md.
• The K-256 fix is correct: 1.3.132.0.10 != 1.2.840.10045.3.1.7, the ECDsa is disposed on the failure path, and garbage input still surfaces CryptographicException.
• Validator changes keep the security-relevant ordering (the d-presence check still runs on the raw element before deserialization; typed JsonWebKey deserialization ignores unknown members and turns type mismatches into a 401, and wrong-length signatures stay a failed verification rather than a 500 via JsonWebKeyVerifier.VerifyData's catch).
• BoundedContent handles the boundaries correctly (declared-length pre-refusal, cap+1 headroom, doubling growth capped at maxBytes+1, exact-cap acceptance, understated Content-Length), and the metadata resolver's catch list correctly drops InvalidDataException, which the replacement no longer throws.
• InternalsVisibleTo is the only csproj change; grep confirms no remaining references to any removed member (DPoPProofGenerator.Base64UrlEncode/ComputeS256Hash/NormalizeHtu, DPoPProofValidator.NormalizeUri, Identity.Base64Url, the JsonElement verifier overload), so "no binary/source breaking" holds. Single net10.0 target, so Base64Url, SearchValues, Convert.TryToHexStringLower and ThrowIfGreaterThanOrEqual are all available.
• The CachedAth cache in DPoPProofGenerator is fine under concurrency: each call uses its own immutable snapshot, so a benign race can only duplicate a hash computation, never mix tokens.

Minor note, non-blocking: ImportP256 identifies the curve via Curve.Oid.Value, so a P-256 PKCS#8 that encodes explicit curve parameters instead of the named OID would be refused as "(unnamed)". Keys exported by the SDK (ExportPrivateKey) always carry the named OID, so the documented session-resumption round trip is unaffected; only third-party explicit-parameter encodings would be rejected, which is an acceptable narrowing for an ES256-only DPoP key.

The pre-existing numeric-date FromUnixTimeSeconds issue is correctly deferred (no bypass, verifier semantics unchanged, follow-up home named), and the deliberately-deferred span-based signing and AtProtoAuthenticationHandler notes are honest scoping.

---

⏱ 24m 51s · 42 steps · $0.2706 · tokens: 636.6k in / 24.6k out (+4777.3k cache-read) · Go: 5h 6% · wk 98% · mo 81%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Solid consolidation refactor — the primitives are correct, the two behavior fixes are real, and nothing is broken by the consolidation. Reviewed the final state of all touched files. **Verified against the .NET runtime source, not just the tests:** - `Jwt.TryDecodeBase64Url`'s strictness claims all hold: `Base64Url.DecodeFromChars` runs with `ignoreWhiteSpace: true` (so the `SearchValues` pre-check is genuinely required to reject `"Y Q"`), accepts correct trailing padding, and rejects misplaced padding, `+`/`/` (invalid in its decoding map), and non-zero trailing bits (`Base64Helper.DecodeFrom` checks `(i0 & 0x8000F000) != 0` → `InvalidData`). Padding acceptance/strictness is what keeps RFC 7515 compat while pinning one spelling — correctly implemented. `GetMaxDecodedLength` + `bytesWritten` slicing is right. - `Jwt.TryDecode`'s signing input is the exact ASCII of `{header}.{payload}` as sent (header/payload segments pass the alphabet check, so ASCII is exact, and padding — when present — is covered by the signature as sent). JSON-object enforcement on header and payload with per-caller exception types and messages preserved. - `Jwt.Sign`'s pooled-buffer arithmetic is correct (86-char signature for a 64-byte fixed r||s, matching the JWS shape), the signature-length check is sound for both ES256 and ES256K, and the buffer is returned in `finally`. - `Jwt.EncodeHeader` embeds exactly `kty/crv/x/y`: the SDK's `JsonWebKey` carries `[JsonPropertyName]` and `JsonIgnore(WhenWritingNull)` on `Crv/X/Y/Kid/Use/Alg`, so `JsonSerializer.Serialize` produces the minimal RFC-shaped header. - `DPoP.NormalizeHtu` is the RFC-correct shape: `SchemeAndServer` (bits 1|4|8) excludes userinfo (bit 2), query and fragment; the userinfo drop is justified by RFC 9110 §4.2.4 and is a real fix (the old `GetLeftPart(UriPartial.Path)` did include `user:pw@`). Both generator and validator call the one implementation, so comparisons are self-consistent; the RFC 9449 §7.1 vector pins the exact output. Every in-repo call site (`OAuthClient`, `XrpcClient` via `new Uri(BaseAddress, RequestUri)`, `SpaceCredentialProvider`) passes absolute URLs, so the new `ArgumentException` only fires for genuinely malformed input, and the behavior change is documented in CHANGELOG and `docs/oauth.md`. - The K-256 fix is correct: `1.3.132.0.10 != 1.2.840.10045.3.1.7`, the ECDsa is disposed on the failure path, and garbage input still surfaces `CryptographicException`. - Validator changes keep the security-relevant ordering (the `d`-presence check still runs on the raw element before deserialization; typed `JsonWebKey` deserialization ignores unknown members and turns type mismatches into a 401, and wrong-length signatures stay a failed verification rather than a 500 via `JsonWebKeyVerifier.VerifyData`'s catch). - `BoundedContent` handles the boundaries correctly (declared-length pre-refusal, cap+1 headroom, doubling growth capped at maxBytes+1, exact-cap acceptance, understated Content-Length), and the metadata resolver's catch list correctly drops `InvalidDataException`, which the replacement no longer throws. - `InternalsVisibleTo` is the only csproj change; grep confirms no remaining references to any removed member (`DPoPProofGenerator.Base64UrlEncode`/`ComputeS256Hash`/`NormalizeHtu`, `DPoPProofValidator.NormalizeUri`, `Identity.Base64Url`, the `JsonElement` verifier overload), so "no binary/source breaking" holds. Single `net10.0` target, so `Base64Url`, `SearchValues`, `Convert.TryToHexStringLower` and `ThrowIfGreaterThanOrEqual` are all available. - The `CachedAth` cache in `DPoPProofGenerator` is fine under concurrency: each call uses its own immutable snapshot, so a benign race can only duplicate a hash computation, never mix tokens. **Minor note, non-blocking:** `ImportP256` identifies the curve via `Curve.Oid.Value`, so a P-256 PKCS#8 that encodes explicit curve parameters instead of the named OID would be refused as "(unnamed)". Keys exported by the SDK (`ExportPrivateKey`) always carry the named OID, so the documented session-resumption round trip is unaffected; only third-party explicit-parameter encodings would be rejected, which is an acceptable narrowing for an ES256-only DPoP key. The pre-existing numeric-date `FromUnixTimeSeconds` issue is correctly deferred (no bypass, verifier semantics unchanged, follow-up home named), and the deliberately-deferred span-based signing and `AtProtoAuthenticationHandler` notes are honest scoping. --- ⏱ 24m 51s · 42 steps · $0.2706 · tokens: 636.6k in / 24.6k out (+4777.3k cache-read) · Go: 5h 6% · wk 98% · mo 81% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-151/run-2026-09-24T18-53-34-061Z.md) <!-- opencode-reviewed-head:98a5cd3f7df381118ed56f01653c84a11be16339 --> <!-- opencode-verdict:approve -->

Grandiras referenced this pull request 2026-09-26 16:41:08 +00:00

fix(spaces)!: protocol hardening, performance and a smaller server surface #172

Sign in to join this conversation.

Reviewers

No reviewers

claude-reviewer

opencode-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

5 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!151

No description provided.