Claude: changes for issue #52

claude-bot commented

2026-07-25 01:19:52 +00:00

Collaborator

Requested in #52.

claude-bot added 1 commit

2026-07-25 01:19:52 +00:00

fix(oauth): bound handle resolution and race HTTPS against DNS (closes #52 )

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

Details

7aeb1a52f2

Starting the OAuth flow for a handle whose domain silently drops packets on
port 443 blocked for the full 100 s HttpClient default before the flow
continued, so sign-in appeared to hang. Two causes: ResolveHandleToDidAsync
tried HTTPS first and only fell back to DNS after it fully failed, and nothing
in the SDK set a timeout.

- AuthorizationServerDiscovery.HandleResolutionTimeout (default 5 s) bounds
  each resolution round with a CancellationTokenSource linked to the caller's
  token. Caller cancellation still propagates; only budget expiry is treated
  as "no answer". Timeout.InfiniteTimeSpan restores the old behaviour.
- ResolveHandleToDidAsync now races the HTTPS well-known lookup against the
  DNS TXT lookup (first DID wins) instead of running them in sequence; the
  appview fallback gets its own fresh budget. ResolveHandleAuthoritativeAsync
  keeps its fail-closed WhenAll, now under the same budget.
- AtProtoOAuthServerOptions gains HttpClient (caller-supplied, not disposed
  with the service, Timeout untouched), HttpClientTimeout (30 s for the
  SDK-created client) and HandleResolutionTimeout; OAuthOptions gains
  HandleResolutionTimeout for core-SDK callers. DidWebResolver's own client
  gets a 10 s timeout.
- Harden the well-known endpoint, whose host comes from untrusted input:
  responses are streamed and capped at 1 KiB (Content-Length and during the
  read) instead of buffered in full, and a response redirected to a different
  host is ignored rather than trusted.

AtProtoClient's own HttpClient is left at the default: it carries blob
uploads, where 100 s can be legitimate.

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

claude-bot referenced this pull request

2026-07-25 01:19:52 +00:00

OAuth handle resolution stalls 100 s when the handle's domain drops packets on :443 #52

Grandiras commented

2026-07-25 01:47:49 +00:00

Owner

@claude-bot Please review.

claude-bot commented

2026-07-25 01:47:52 +00:00

Author

Collaborator

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

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/29); the reply will link the full session transcript.

claude-bot added 1 commit

2026-07-25 01:52:58 +00:00

docs(changelog): restore Directory.Build.props entry to Fixed, note DoH change

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

Details

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

Details

f7ac34e08a

The new "### Security" heading was inserted directly above the pre-existing
`Directory.Build.props` `RepositoryUrl` bullet, silently reclassifying a build
fix as a security item. Move the heading below it so Security stays last.

Also record that racing the two lookups means dns.google is now contacted on
every handle resolution, not only after the HTTPS well-known lookup fails.

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

claude-bot commented

2026-07-25 01:53:26 +00:00

Author

Collaborator

Reviewed the branch, built it, and ran the full unit suite.

✅ Verified

dotnet build -p:EnableSourceControlManagerQueries=false — clean; no new warnings on any of the touched files (the CS1591s are pre-existing Lexicon/Tools/Ozone noise).
dotnet test tests/ATProtoNet.Tests/ — 934 passed, 0 failed.
Disposal ownership checked end-to-end: OAuthClient.Dispose() doesn't touch the HttpClient, and AtProtoOAuthService._httpClient stays null when a client is supplied, so there's no double-dispose or dispose-the-caller's-client path. The test asserting the caller's client survives service.Dispose() covers exactly that.
The race cleanup in RaceAuthoritativeLookupsAsync is correct: budget.Cancel() runs before the using disposes the source, so the loser's registrations are already fired and the ContinueWith observation prevents a late fault surfacing unobserved.

Overall this is solid — the diagnosis in the commit message matches the code, and the tests exercise the actual failure mode (a handler that hangs on :443) rather than just the plumbing.

🔧 Fixed on the branch (`claude/issue-52`, commit `f7ac34e`)

CHANGELOG.md — the new ### Security heading swallowed an unrelated entry. It was inserted directly above the pre-existing Directory.Build.props/RepositoryUrl bullet, which was under ### Fixed in HEAD~1. That NuGet-registry build fix was silently reclassified as a security item. Moved the heading below it so Security stays last.

I also added a Security bullet for a behaviour change the original entry didn't mention — see below.

💬 Review comments (your call, not changed)

1. DNS-over-HTTPS is now on the hot path for every resolution. Racing means dns.google is contacted on every ResolveHandleToDidAsync call, where previously it was only reached after the HTTPS well-known lookup failed. For *.bsky.social handles — which do serve the well-known endpoint — that used to be never. So every handle a user types now goes to a hardcoded third-party resolver. It's a reasonable trade for the latency win, but it deserved to be stated; I added a CHANGELOG bullet for it. Worth considering whether the DoH endpoint should be configurable.

2. AtProtoOAuthService.cs:115 mutates a caller-supplied HttpClient. The comment two lines above says "A caller-supplied client is theirs: don't touch its Timeout", and then the code writes to its DefaultRequestHeaders.UserAgent. For the headline use case in the docs — an IHttpClientFactory client — that instance may be handed out and in flight elsewhere; DefaultRequestHeaders mutation isn't thread-safe, and the UA then rides on all the caller's other requests through that client. Setting the UA per-request, or only on the SDK-created client, would match the stated contract.

3. ResolveHandleAuthoritativeAsync's fail-closed guarantee is now budget-dependent. With both lookups sharing one 5 s budget, an honest-but-slow HTTPS authority times out to null and the method accepts the DNS answer alone, logging "resolved via DNS only" — the conflict check never runs. I don't think it's exploitable in practice (an attacker who can stall one authority generally already controls it, and can't stall dns.google), but on slow networks it means the authoritative path can quietly degrade to single-source. There's no test for "both answer, one exceeds budget"; that case is worth pinning down.

4. Same-host redirect check may break apex→www setups. A handle domain that 301s example.com/.well-known/atproto-did → www.example.com/... previously resolved and now returns null. The DNS/appview fallbacks cover it, so the failure is a slower resolution rather than a broken one — just flagging that it's a real-world config, not only an attack shape.

5. Minor: the resolution order in ResolveHandleToDidAsync is now nondeterministic — with a stale DNS TXT record and a corrected well-known, which one you get varies per call. Fine for the documented convenience path, but a short HTTPS head start (or preferring HTTPS when both land inside a grace window) would keep it stable. Also, the commit message attributes AtProtoOAuthServerOptions to ATProtoNet.Server; it lives in ATProtoNet.Blazor.

⏱ 5m 25s · 28 turns · tokens: 74.6k in / 19.6k out (+1471.7k cache-read)
📜 Full session transcript

Reviewed the branch, built it, and ran the full unit suite. ## ✅ Verified - `dotnet build -p:EnableSourceControlManagerQueries=false` — clean; **no new warnings** on any of the touched files (the CS1591s are pre-existing `Lexicon/Tools/Ozone` noise). - `dotnet test tests/ATProtoNet.Tests/` — **934 passed, 0 failed**. - Disposal ownership checked end-to-end: `OAuthClient.Dispose()` doesn't touch the `HttpClient`, and `AtProtoOAuthService._httpClient` stays `null` when a client is supplied, so there's no double-dispose or dispose-the-caller's-client path. The test asserting the caller's client survives `service.Dispose()` covers exactly that. - The race cleanup in `RaceAuthoritativeLookupsAsync` is correct: `budget.Cancel()` runs *before* the `using` disposes the source, so the loser's registrations are already fired and the `ContinueWith` observation prevents a late fault surfacing unobserved. Overall this is solid — the diagnosis in the commit message matches the code, and the tests exercise the actual failure mode (a handler that hangs on `:443`) rather than just the plumbing. ## 🔧 Fixed on the branch (`claude/issue-52`, commit `f7ac34e`) **`CHANGELOG.md` — the new `### Security` heading swallowed an unrelated entry.** It was inserted directly above the pre-existing `Directory.Build.props`/`RepositoryUrl` bullet, which was under `### Fixed` in `HEAD~1`. That NuGet-registry build fix was silently reclassified as a security item. Moved the heading below it so `Security` stays last. I also added a Security bullet for a behaviour change the original entry didn't mention — see below. ## 💬 Review comments (your call, not changed) **1. DNS-over-HTTPS is now on the hot path for *every* resolution.** Racing means `dns.google` is contacted on every `ResolveHandleToDidAsync` call, where previously it was only reached after the HTTPS well-known lookup failed. For `*.bsky.social` handles — which do serve the well-known endpoint — that used to be never. So every handle a user types now goes to a hardcoded third-party resolver. It's a reasonable trade for the latency win, but it deserved to be stated; I added a CHANGELOG bullet for it. Worth considering whether the DoH endpoint should be configurable. **2. `AtProtoOAuthService.cs:115` mutates a caller-supplied `HttpClient`.** The comment two lines above says *"A caller-supplied client is theirs: don't touch its `Timeout`"*, and then the code writes to its `DefaultRequestHeaders.UserAgent`. For the headline use case in the docs — an `IHttpClientFactory` client — that instance may be handed out and in flight elsewhere; `DefaultRequestHeaders` mutation isn't thread-safe, and the UA then rides on all the caller's other requests through that client. Setting the UA per-request, or only on the SDK-created client, would match the stated contract. **3. `ResolveHandleAuthoritativeAsync`'s fail-closed guarantee is now budget-dependent.** With both lookups sharing one 5 s budget, an honest-but-slow HTTPS authority times out to `null` and the method accepts the DNS answer alone, logging "resolved via DNS only" — the conflict check never runs. I don't think it's exploitable in practice (an attacker who can stall one authority generally already controls it, and can't stall `dns.google`), but on slow networks it means the authoritative path can quietly degrade to single-source. There's no test for "both answer, one exceeds budget"; that case is worth pinning down. **4. Same-host redirect check may break apex→www setups.** A handle domain that 301s `example.com/.well-known/atproto-did` → `www.example.com/...` previously resolved and now returns `null`. The DNS/appview fallbacks cover it, so the failure is a slower resolution rather than a broken one — just flagging that it's a real-world config, not only an attack shape. **5. Minor:** the resolution order in `ResolveHandleToDidAsync` is now nondeterministic — with a stale DNS TXT record and a corrected well-known, which one you get varies per call. Fine for the documented convenience path, but a short HTTPS head start (or preferring HTTPS when both land inside a grace window) would keep it stable. Also, the commit message attributes `AtProtoOAuthServerOptions` to `ATProtoNet.Server`; it lives in `ATProtoNet.Blazor`. --- ⏱ 5m 25s · 28 turns · tokens: 74.6k in / 19.6k out (+1471.7k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-53/run-29.md)

Grandiras merged commit db6ab1146e into main

2026-07-25 01:54:36 +00:00

Grandiras deleted branch claude/issue-52

2026-07-25 01:54:37 +00:00

Grandiras referenced this pull request from a commit

2026-07-25 01:54:37 +00:00

Claude: changes for issue #52 (#53)

claude-bot referenced this pull request from a commit

2026-07-25 01:59:28 +00:00

Merge main into fix-52-handle-resolution-timeout

claude-bot referenced this pull request

2026-07-25 01:59:42 +00:00