fix(oauth): bound handle resolution so a silent authority can't stall sign-in #54

Merged

Grandiras merged 2 commits from fix-52-handle-resolution-timeout into main 2026-07-25 02:00:14 +00:00

Conversation 5 Commits 2 Files changed 1 +147

Grandiras commented 2026-07-25 01:30:44 +00:00

Owner

Copy link

Closes #52.

What was broken

A handle whose domain drops packets on :443 — parked or firewalled apex domains do this routinely — neither answers nor refuses. Resolution therefore ran until HttpClient.Timeout elapsed, and since AtProtoOAuthService builds its client as a bare new HttpClient(), that was always the 100 s BCL default.

Two call sites, and the second one is fatal rather than merely slow:

ResolveHandleToDidAsync (login start) tried HTTPS before DNS, sequentially. A DNS TXT record that answers in ~100 ms went unread for a minute and a half:

$ time curl -sD- "/atproto/login?handle=grandiras.net"
HTTP/1.1 302 Found
Location: https://pds.grandiras.net/oauth/authorize?...
real    1m40.624s

ResolveHandleAuthoritativeAsync (OAuth callback) awaited both sources and let the eventual TaskCanceledException escape. CompleteAuthorizationAsync step 8 catches when (ex is not OperationCanceledException), so this was not absorbed as "handle unverified" — it propagated out of CompleteCallbackAsync and bounced the user back to the login page. Measured directly, before this change:

grandiras.net: THREW TaskCanceledException after 100.0s
  The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing.

Handle verification is not optional in the callback, so any account whose handle sits on such a domain could not sign in at all — it authorized on the PDS, waited 100 s on the redirect, and landed back at /login. _atproto.grandiras.net TXT had the answer the whole time.

The change

• New AuthorizationServerDiscovery.HandleResolutionTimeout (default 5 s, settable). Every handle lookup runs under it.
• A source that misses its deadline counts as silent, not failed. That's the substantive fix: the authoritative resolver can now fall back on the authority that did answer instead of failing the flow.
• Cancellation the caller passes in still propagates — the legs distinguish their own attempt budget from the caller's token, so OperationCanceledException semantics for callers are unchanged.
• ResolveHandleToDidAsync keeps its try-in-order precedence (HTTPS, then DNS, then appview); each attempt is simply bounded. Not switched to a race, to avoid querying dns.google on every login when HTTPS answers fine.
• Conflict detection is untouched: when both authorities answer they must still agree, and disagreement still fails closed with handle_resolution_conflict.
• AtProtoOAuthService's HttpClient gets a 30 s timeout, since AtProtoOAuthServerOptions gives a consuming app no way to supply one.

Verification

Against the reporting handle, calling the real ResolveHandleAuthoritativeAsync:

	before	after
grandiras.net (dead :443, DNS TXT present)	throws TaskCanceledException @ 100.0 s	resolves did:plc:vmdqoelhettxubov4hejncg2 @ 5.0 s

The residual 5 s is the authoritative resolver waiting out the silent source so it can still detect a DNS/HTTPS conflict — deliberate, and now bounded.

9 new tests in HandleResolutionTimeoutTests cover: silent HTTPS still resolving from DNS and vice versa, returning within budget, both-silent still raising handle_resolution_failed, conflicts still failing closed, caller cancellation still propagating, and the convenience resolver's fall-through order including the appview.

Full unit suite: 930 passed.

Not addressed here

ResolveHandleViaHttpsAsync still follows redirects with default settings and buffers the whole response before checking the did: prefix, on a host derived from untrusted input. Worth a separate look — flagged in #52.

🤖 Generated with Claude Code

Closes #52. ## What was broken A handle whose domain drops packets on :443 — parked or firewalled apex domains do this routinely — neither answers nor refuses. Resolution therefore ran until `HttpClient.Timeout` elapsed, and since `AtProtoOAuthService` builds its client as a bare `new HttpClient()`, that was always the 100 s BCL default. Two call sites, and the second one is fatal rather than merely slow: **`ResolveHandleToDidAsync`** (login start) tried HTTPS *before* DNS, sequentially. A DNS TXT record that answers in ~100 ms went unread for a minute and a half: ``` $ time curl -sD- "/atproto/login?handle=grandiras.net" HTTP/1.1 302 Found Location: https://pds.grandiras.net/oauth/authorize?... real 1m40.624s ``` **`ResolveHandleAuthoritativeAsync`** (OAuth callback) awaited both sources and let the eventual `TaskCanceledException` escape. `CompleteAuthorizationAsync` step 8 catches `when (ex is not OperationCanceledException)`, so this was *not* absorbed as "handle unverified" — it propagated out of `CompleteCallbackAsync` and bounced the user back to the login page. Measured directly, before this change: ``` grandiras.net: THREW TaskCanceledException after 100.0s The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing. ``` Handle verification is not optional in the callback, so any account whose handle sits on such a domain could not sign in at all — it authorized on the PDS, waited 100 s on the redirect, and landed back at `/login`. `_atproto.grandiras.net` TXT had the answer the whole time. ## The change - New `AuthorizationServerDiscovery.HandleResolutionTimeout` (default 5 s, settable). Every handle lookup runs under it. - **A source that misses its deadline counts as silent, not failed.** That's the substantive fix: the authoritative resolver can now fall back on the authority that *did* answer instead of failing the flow. - Cancellation the *caller* passes in still propagates — the legs distinguish their own attempt budget from the caller's token, so `OperationCanceledException` semantics for callers are unchanged. - `ResolveHandleToDidAsync` keeps its try-in-order precedence (HTTPS, then DNS, then appview); each attempt is simply bounded. Not switched to a race, to avoid querying `dns.google` on every login when HTTPS answers fine. - Conflict detection is untouched: when both authorities answer they must still agree, and disagreement still fails closed with `handle_resolution_conflict`. - `AtProtoOAuthService`'s `HttpClient` gets a 30 s timeout, since `AtProtoOAuthServerOptions` gives a consuming app no way to supply one. ## Verification Against the reporting handle, calling the real `ResolveHandleAuthoritativeAsync`: | | before | after | |---|---|---| | `grandiras.net` (dead :443, DNS TXT present) | throws `TaskCanceledException` @ 100.0 s | resolves `did:plc:vmdqoelhettxubov4hejncg2` @ 5.0 s | The residual 5 s is the authoritative resolver waiting out the silent source so it can still detect a DNS/HTTPS conflict — deliberate, and now bounded. 9 new tests in `HandleResolutionTimeoutTests` cover: silent HTTPS still resolving from DNS and vice versa, returning within budget, both-silent still raising `handle_resolution_failed`, conflicts still failing closed, caller cancellation still propagating, and the convenience resolver's fall-through order including the appview. Full unit suite: **930 passed**. ## Not addressed here `ResolveHandleViaHttpsAsync` still follows redirects with default settings and buffers the whole response before checking the `did:` prefix, on a host derived from untrusted input. Worth a separate look — flagged in #52. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-07-25 01:30:44 +00:00

fix(oauth): bound handle resolution so a silent authority can't stall sign-in (#52) ... 85ae3cce19

A handle whose domain drops packets on :443 — parked or firewalled apex
domains do this routinely — neither answers nor refuses, so resolution ran
until HttpClient.Timeout elapsed. 100 s by default, and the client was
built as a bare `new HttpClient()`, so that default was always in force.

Two paths were affected, and the second one was fatal:

- ResolveHandleToDidAsync tried HTTPS before DNS, sequentially, so a DNS
  TXT record answering in ~100 ms went unread for a minute and a half.
- ResolveHandleAuthoritativeAsync awaited both sources and let the
  eventual TaskCanceledException escape. CompleteAuthorizationAsync step 8
  catches `when (ex is not OperationCanceledException)`, so this was not
  absorbed as "handle unverified" — it propagated and failed the OAuth
  callback, locking such accounts out of sign-in entirely.

Each lookup now runs under HandleResolutionTimeout (default 5 s, settable)
and a source that misses its deadline counts as silent, not failed, so the
other authority can still supply the answer. Cancellation the caller passes
in still propagates. Conflict detection between DNS and HTTPS is unchanged:
both are still required to agree when both answer.

The OAuth service's HttpClient also gets a 30 s timeout, since options
expose no hook for a consuming app to supply its own.

Verified against the reporting handle: ResolveHandleAuthoritativeAsync
("grandiras.net") went from throwing TaskCanceledException after 100.0 s to
resolving in 5.0s via the DNS record. 930 unit tests pass, 9 new.

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

claude-bot commented 2026-07-25 01:31:24 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

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

claude-bot reviewed 2026-07-25 01:33:35 +00:00

claude-bot left a comment

Copy link

No description provided.

Solid, well-targeted fix. The core mechanism is sound: each lookup attempt gets its own linked `CancellationTokenSource` capped by `HandleResolutionTimeout`, and the catch clauses use `when (callerToken.IsCancellationRequested)` to distinguish "attempt budget elapsed → silent, try the other authority" from "caller actually cancelled → propagate `OperationCanceledException`". I traced the token plumbing through both call sites (`ResolveHandleAuthoritativeAsync`'s shared budget for the concurrent HTTPS+DNS pair, and the sequential per-attempt budgets in `ResolveHandleToDidAsync`) and the parameter order/semantics are consistent everywhere. This correctly interacts with `OAuthClient.CompleteAuthorizationAsync`'s `catch (Exception ex) when (ex is not OperationCanceledException)` at the callback site — a real caller cancellation still surfaces as `OperationCanceledException` and isn't swallowed, while a silently-dead authority no longer takes the whole callback down with it. Conflict detection (both sources answer, disagree → fail closed) is untouched, matching the description. The new `HandleResolutionTimeout` default (5s) is comfortably shorter than both the new 30s `AtProtoOAuthService` HttpClient timeout and any bare-`HttpClient()` 100s default, so the attempt budget always wins the race in practice — no dead-letter scenario where `HttpClient.Timeout` fires first and produces an untagged cancellation. Worst case for `ResolveHandleToDidAsync` (all three sources silent) is now bounded at roughly 3×`HandleResolutionTimeout` instead of 3×100s, which is the intended improvement. Tests are thorough: silent-HTTPS/silent-DNS symmetry, within-budget timing, both-silent failure, conflict-still-fails-closed, caller-cancellation-propagates, and convenience-resolver fall-through order including the appview. `HandleResolutionTimeout` is a mutable public property on a class that's constructed once and cached for the lifetime of the (lazily-created, cached) `OAuthClient` in `AtProtoOAuthService`, so it's effectively shared/long-lived rather than per-call — fine given it's meant to be a one-time app-level configuration knob, just worth knowing if a future change tries to vary it per-request. CHANGELOG entries are correctly placed under `Fixed` with adequate detail. No blocking issues found. --- ⏱ 2m 2s · 7 turns · tokens: 47.3k in / 11.8k out (+395.1k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-54/run-25.md)

Grandiras commented 2026-07-25 01:55:56 +00:00

Author

Owner

Copy link

@claude-bot Please resolve the conflicts.

@claude-bot Please resolve the conflicts.

claude-bot commented 2026-07-25 01:56:01 +00:00

Collaborator

Copy link

🤖 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/30); the reply will link the full session transcript.

claude-bot added 1 commit 2026-07-25 01:59:28 +00:00

Merge main into fix-52-handle-resolution-timeout ... 262d7f2220

#53 landed the same fix on main first, and its implementation is a strict
superset of this branch's: the HandleResolutionTimeout budget, the "silent
source counts as no answer, not as failure" rule, caller-cancellation
passthrough and the OAuth HttpClient timeout are all there, plus the
well-known response cap and redirect-host check this PR explicitly deferred.
All three conflicted files (CHANGELOG.md, AtProtoOAuthService.cs,
AuthorizationServerDiscovery.cs) therefore resolve to main's side verbatim —
`git diff origin/main -- src/ CHANGELOG.md` is empty.

What survives from this branch is HandleResolutionTimeoutTests, trimmed to
the five cases main's HandleResolutionTests does not cover:

- silent DNS still resolving from HTTPS (main tests only the mirror case)
- both sources silent still raising handle_resolution_failed
- conflicting answers still failing closed with handle_resolution_conflict
- caller cancellation propagating out of ResolveHandleAuthoritativeAsync
- both authorities silent still falling through to the appview

Convenience_PrefersHttpsOverDns is dropped: main races the HTTPS and DNS
lookups rather than trying them in order, so HTTPS precedence is no longer a
guarantee to assert. The three remaining cases were straight duplicates of
main's coverage.

Test-only after the merge, so no CHANGELOG entry — main's #52 bullets already
describe the shipped behaviour. 939 unit tests pass.

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

claude-bot commented 2026-07-25 01:59:42 +00:00

Collaborator

Copy link

Conflicts resolved and pushed to fix-52-handle-resolution-timeout (262d7f2).

What the conflict actually was

While this PR was open, #53 landed the same fix on main (db6ab11), and its implementation is a strict superset of this branch's:

	this branch	main (#53)
HandleResolutionTimeout budget	✅ 5 s	✅ 5 s, plus validation and Timeout.InfiniteTimeSpan opt-out
silent source ⇒ "no answer", not failure	✅	✅
caller cancellation still propagates	✅	✅
conflict detection preserved	✅	✅
OAuth HttpClient timeout	✅ 30 s hardcoded	✅ 30 s, configurable via AtProtoOAuthServerOptions.HttpClientTimeout/.HttpClient
well-known 1 KiB cap + redirect-host check	❌ deferred, flagged in #52	✅ done
ResolveHandleToDidAsync	sequential, each attempt bounded	races HTTPS vs DNS

So all three conflicted files — CHANGELOG.md, AtProtoOAuthService.cs, AuthorizationServerDiscovery.cs — resolve to main's side verbatim. git diff origin/main -- src/ CHANGELOG.md is now empty.

What's left of the PR

Only HandleResolutionTimeoutTests, trimmed from 9 cases to the 5 that main's HandleResolutionTests doesn't cover:

• silent DNS still resolving from HTTPS (main only tests the mirror case)
• both sources silent → still handle_resolution_failed
• conflicting answers → still fails closed with handle_resolution_conflict
• caller cancellation propagating out of ResolveHandleAuthoritativeAsync
• both authorities silent → still falls through to the appview

Convenience_PrefersHttpsOverDns is dropped, not merged — main races the two lookups, so HTTPS precedence is no longer a property to assert; keeping it would have been a flaky test asserting superseded behaviour. Three others were straight duplicates of main's coverage. No CHANGELOG entry added since the merge leaves this test-only and main's #52 bullets already describe the shipped behaviour.

Verified

Installed the .NET 10 SDK in the container (none was present), then:

• dotnet build -p:EnableSourceControlManagerQueries=false — 0 errors. Also fixed a CS8602 the test stub was emitting on the touched file.
• dotnet test tests/ATProtoNet.Tests/ — 939 passed, 0 failed; the ~HandleResolution filter runs 15 of them green.

Worth deciding: the PR is now a test-only delta on top of a fix that already shipped. Merging it adds the five extra assertions; closing it as superseded by #53 loses only those. Your call — I've left it mergeable either way.

---

⏱ 3m 33s · 23 turns · tokens: 48.1k in / 10.9k out (+1116.6k cache-read)
📜 Full session transcript

Conflicts resolved and pushed to `fix-52-handle-resolution-timeout` (`262d7f2`). ## What the conflict actually was While this PR was open, **#53 landed the same fix on main** (`db6ab11`), and its implementation is a strict superset of this branch's: | | this branch | main (#53) | |---|---|---| | `HandleResolutionTimeout` budget | ✅ 5 s | ✅ 5 s, plus validation and `Timeout.InfiniteTimeSpan` opt-out | | silent source ⇒ "no answer", not failure | ✅ | ✅ | | caller cancellation still propagates | ✅ | ✅ | | conflict detection preserved | ✅ | ✅ | | OAuth `HttpClient` timeout | ✅ 30 s hardcoded | ✅ 30 s, configurable via `AtProtoOAuthServerOptions.HttpClientTimeout`/`.HttpClient` | | well-known 1 KiB cap + redirect-host check | ❌ deferred, flagged in #52 | ✅ done | | `ResolveHandleToDidAsync` | sequential, each attempt bounded | races HTTPS vs DNS | So all three conflicted files — `CHANGELOG.md`, `AtProtoOAuthService.cs`, `AuthorizationServerDiscovery.cs` — resolve to main's side verbatim. `git diff origin/main -- src/ CHANGELOG.md` is now empty. ## What's left of the PR Only `HandleResolutionTimeoutTests`, trimmed from 9 cases to the 5 that main's `HandleResolutionTests` doesn't cover: - silent **DNS** still resolving from HTTPS (main only tests the mirror case) - both sources silent → still `handle_resolution_failed` - conflicting answers → still fails closed with `handle_resolution_conflict` - caller cancellation propagating out of `ResolveHandleAuthoritativeAsync` - both authorities silent → still falls through to the appview `Convenience_PrefersHttpsOverDns` is **dropped**, not merged — main races the two lookups, so HTTPS precedence is no longer a property to assert; keeping it would have been a flaky test asserting superseded behaviour. Three others were straight duplicates of main's coverage. No CHANGELOG entry added since the merge leaves this test-only and main's #52 bullets already describe the shipped behaviour. ## Verified Installed the .NET 10 SDK in the container (none was present), then: - `dotnet build -p:EnableSourceControlManagerQueries=false` — 0 errors. Also fixed a `CS8602` the test stub was emitting on the touched file. - `dotnet test tests/ATProtoNet.Tests/` — **939 passed, 0 failed**; the `~HandleResolution` filter runs 15 of them green. **Worth deciding:** the PR is now a test-only delta on top of a fix that already shipped. Merging it adds the five extra assertions; closing it as superseded by #53 loses only those. Your call — I've left it mergeable either way. --- ⏱ 3m 33s · 23 turns · tokens: 48.1k in / 10.9k out (+1116.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-54/run-30.md)

Grandiras merged commit 669b8519b0 into main 2026-07-25 02:00:14 +00:00

Grandiras deleted branch fix-52-handle-resolution-timeout 2026-07-25 02:00:14 +00:00

Grandiras referenced this pull request from a commit 2026-07-25 02:00:15 +00:00

fix(oauth): bound handle resolution so a silent authority can't stall sign-in (#54)

claude-bot referenced this pull request 2026-07-25 02:09:00 +00:00

feat: Jetstream consumer — JSON event streaming with server-side filtering (closes #43) #44

Sign in to join this conversation.

Reviewers

No reviewers

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

2 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!54

No description provided.