fix: DPoP nonce handling, did:web resolution, PAR request_uri encoding (P1-2, P1-3, P1-4, P2-3)

starsetbyte commented

2026-07-13 00:57:43 +00:00

Owner

Fixes P1-2, P1-3, P1-4, P2-3 from the 2026-07-11 adversarial delta review.

P1-3 / P1-4 — did:web resolution + DID document caching (commit `5e4efff`)

HandleResolver.buildWebDidUrl now does spec-compliant did:web URL construction (path DIDs no longer append /.well-known/did.json after the path; percent-encoded ports are decoded and validated via HttpUrl).
DID document cache is now TTL-bounded (5 min) with an explicit forceRefresh; refreshSession force-revalidates so a PDS migration/rotation is no longer ignored until process death.

P1-2 / P2-3 — DPoP nonce retries + PAR request_uri encoding (commits d3cffd0..6fddb50)

PAR/token-exchange/refresh responses that hit a DPoP nonce challenge are now closed (use { }) before retrying, and refresh no longer recurses unbounded — one explicit bounded retry.
The two previously-@Ignored nonce-retry tests are unignored and now assert exactly two requests, nonce presence on the retry, and response closure.
Authorization URLs built from a server-issued request_uri now go through HttpUrl.Builder.addQueryParameter instead of string concatenation, so reserved characters in the request_uri can't change query meaning.

Two trivial nits from task review (unused Ignore import, KDoc indentation) fixed in 0408422.

Gate: ./gradlew testDebugUnitTest lintDebug — 0 failures, 0 lint errors.

Merge order: this is branch A of four (A → B → C → D per the review's dependency note — DM Phase A work should not proceed until A and C are merged).

Fixes P1-2, P1-3, P1-4, P2-3 from the [2026-07-11 adversarial delta review](https://durandal.exe.xyz/starsetbyte/peregrine/src/branch/main/docs/reviews/2026-07-11-adversarial-delta-review.md). ## P1-3 / P1-4 — did:web resolution + DID document caching (commit 5e4efff) - `HandleResolver.buildWebDidUrl` now does spec-compliant `did:web` URL construction (path DIDs no longer append `/.well-known/did.json` after the path; percent-encoded ports are decoded and validated via `HttpUrl`). - DID document cache is now TTL-bounded (5 min) with an explicit `forceRefresh`; `refreshSession` force-revalidates so a PDS migration/rotation is no longer ignored until process death. ## P1-2 / P2-3 — DPoP nonce retries + PAR request_uri encoding (commits d3cffd0..6fddb50) - PAR/token-exchange/refresh responses that hit a DPoP nonce challenge are now closed (`use { }`) before retrying, and refresh no longer recurses unbounded — one explicit bounded retry. - The two previously-`@Ignore`d nonce-retry tests are unignored and now assert exactly two requests, nonce presence on the retry, and response closure. - Authorization URLs built from a server-issued `request_uri` now go through `HttpUrl.Builder.addQueryParameter` instead of string concatenation, so reserved characters in the request_uri can't change query meaning. Two trivial nits from task review (unused `Ignore` import, KDoc indentation) fixed in `0408422`. **Gate:** `./gradlew testDebugUnitTest lintDebug` — 0 failures, 0 lint errors. **Merge order:** this is branch **A** of four (A → B → C → D per the review's dependency note — DM Phase A work should not proceed until A and C are merged).

starsetbyte added 4 commits

2026-07-13 00:57:43 +00:00

fix: spec-compliant did:web resolution + TTL-bounded DID document cache (P1-3, P1-4) 5e4efff94f

- buildWebDidUrl(): path DIDs resolve did.json under the decoded path
  (only hostname-only DIDs use /.well-known/did.json), percent-encoded
  ports land in the authority, hosts validated via HttpUrl, strict
  percent-decoding fails closed on malformed input
- DID document cache now has a 5-minute TTL and an explicit forceRefresh;
  refreshSession force-revalidates so a PDS migration or identity rotation
  is observed instead of reusing the login-time document
- fetchDidDocument closes its response (was leaked)
- tests: production did:web URL construction (hostname/path/port/malformed),
  cache TTL + forceRefresh, login-then-migration refresh test

Per docs/reviews/2026-07-11-adversarial-delta-review.md P1-3/P1-4.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lvg5FEfh2X5yFCnxBzKoCn

docs: implementation plan for remaining P1/P2 adversarial-review fixes d3cffd0da7

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lvg5FEfh2X5yFCnxBzKoCn

fix: close DPoP nonce-challenged responses, bound retries, encode authorization URL (P1-2, P2-3) 6fddb50ccf

- Add executeWithNonceDetection() helper that closes responses and detects nonce challenges
- Add buildAuthorizationUrl() helper that percent-encodes query values (fixes request_uri encoding)
- Implement bounded nonce retries (at most one) with no connection leaks in beginLogin, completeLogin, refreshSession
- Remove recursive refreshSession retry with discovery re-fetch; bound to single retry against validated metadata
- Update tests to verify nonce handling, bounded retries on repeated challenges, and percent-encoded request_uri
- Delete dead retryParWithNonce and retryTokenExchangeWithNonce functions

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lvg5FEfh2X5yFCnxBzKoCn

fix: remove dead Ignore import and re-indent KDoc (Task 1 review nits)

Peregrine CI / Signed Minified Release Build (pull_request) Has been cancelled

Details

Peregrine CI / Instrumented Tests (API 29+) (pull_request) Has been cancelled

Details

Peregrine CI / Build & Test (JDK 17) (pull_request) Has been cancelled

Details

peregrine-ci / assembleDebug Build succeeded (1m2s)

Details

0408422c15

Cleans up two Minor findings from Task 1's review: an unused
org.junit.Ignore import left over after unignoring the nonce-retry
tests, and a misindented KDoc comment on parseTokenResponse.

starsetbyte added 1 commit

2026-07-13 00:59:58 +00:00

docs: resume handoff for Tasks 10-13 of the P1/P2 fix effort

peregrine-ci / assembleDebug Build succeeded (1m28s)

Details

Peregrine CI / Build & Test (JDK 17) (pull_request) Successful in 5m3s

Details

Peregrine CI / Signed Minified Release Build (pull_request) Successful in 4m12s

Details

Peregrine CI / Instrumented Tests (API 29+) (pull_request) Has been skipped

Details

64de4af389

Records session state before compaction/handoff: Tasks 1-9 done, Task 10
(P1-6 backup flag) remaining on branch C, branch D not yet started.

starsetbyte commented

2026-07-13 01:30:20 +00:00

Author

Owner

Review pass 1 — revise

P1 — successful DPoP responses discard the server nonce. executeWithNonceDetection() returns only the body on success (AtProtoOAuthClient.kt:365-375), so the PAR nonce is not carried into PendingAuth and the first token request is constructed as tokenRequest(null) (:154-168, :229-243). The AT Protocol OAuth profile requires mandatory server-issued nonce tracking. Preserve the successful response header, use the PAR nonce for the first token exchange, persist rotated token/refresh nonces, and add success-path/missing-header tests.

P1 — did:web documents are consumed without binding doc.id to the requested DID. HandleResolver.fetchDidDocument() parses and caches any successful JSON document (HandleResolver.kt:220-237), then resolves its PDS service. Reject missing/mismatched id before caching or reading services; add negative MockWebServer coverage.

P2 — reject IP-literal did:web authorities. The current buildWebDidUrl() accepts them through HttpUrl.Builder.host() (:261-276), while the did:web specification prohibits IP addresses.

Targeted OAuth/HandleResolver unit suites compile and pass locally, but they do not exercise these required cases.

## Review pass 1 — revise **P1 — successful DPoP responses discard the server nonce.** `executeWithNonceDetection()` returns only the body on success (`AtProtoOAuthClient.kt:365-375`), so the PAR nonce is not carried into `PendingAuth` and the first token request is constructed as `tokenRequest(null)` (`:154-168`, `:229-243`). The AT Protocol OAuth profile requires mandatory server-issued nonce tracking. Preserve the successful response header, use the PAR nonce for the first token exchange, persist rotated token/refresh nonces, and add success-path/missing-header tests. **P1 — did:web documents are consumed without binding `doc.id` to the requested DID.** `HandleResolver.fetchDidDocument()` parses and caches any successful JSON document (`HandleResolver.kt:220-237`), then resolves its PDS service. Reject missing/mismatched `id` before caching or reading services; add negative MockWebServer coverage. **P2 — reject IP-literal did:web authorities.** The current `buildWebDidUrl()` accepts them through `HttpUrl.Builder.host()` (`:261-276`), while the did:web specification prohibits IP addresses. Targeted OAuth/HandleResolver unit suites compile and pass locally, but they do not exercise these required cases.

starsetbyte added 1 commit

2026-07-13 12:35:53 +00:00

fix: preserve successful OAuth DPoP nonces

peregrine-ci / assembleDebug Build succeeded (1m36s)

Details

Peregrine CI / Build & Test (JDK 17) (pull_request) Successful in 6m55s

Details

Peregrine CI / Signed Minified Release Build (pull_request) Successful in 4m16s

Details

Peregrine CI / Instrumented Tests (API 29+) (pull_request) Has been skipped

Details

5906047b5a

starsetbyte closed this pull request

2026-07-13 12:55:32 +00:00

starsetbyte commented

2026-07-13 13:01:21 +00:00

Author

Owner

Merged to main (`87933e8`)

Review pass 2 — remediation verified

Finding	Status
P1: DPoP nonce discarded on success	Fixed (`5906047`)
P1: did:web doc.id binding	Open — follow-up needed
P2: IP-literal did:web	Open — follow-up needed

Tests: AtProtoOAuthClientTest, HandleResolverTest, PendingAuthStoreTest all pass.

## Merged to main (87933e8) **Review pass 2 — remediation verified** | Finding | Status | |---------|--------| | P1: DPoP nonce discarded on success | Fixed (5906047) | | P1: did:web doc.id binding | Open — follow-up needed | | P2: IP-literal did:web | Open — follow-up needed | Tests: AtProtoOAuthClientTest, HandleResolverTest, PendingAuthStoreTest all pass.

starsetbyte commented

2026-07-26 03:22:31 +00:00

Author

Owner

Content from fix/p1-oauth-did-correctness shipped to main via a local --no-ff merge, which is why Forgejo shows this closed-unmerged rather than merged. Reconciled in docs/reviews/2026-07-25-branch-pr-reconciliation.md, which landed on main via #103. Branch deleted as part of that reconciliation.

Content from `fix/p1-oauth-did-correctness` shipped to `main` via a local `--no-ff` merge, which is why Forgejo shows this closed-unmerged rather than merged. Reconciled in `docs/reviews/2026-07-25-branch-pr-reconciliation.md`, which landed on `main` via #103. Branch deleted as part of that reconciliation.

peregrine-ci / assembleDebug Build succeeded (1m36s)

Required

Details

Peregrine CI / Build & Test (JDK 17) (pull_request) Successful in 6m55s

Details

Peregrine CI / Signed Minified Release Build (pull_request) Successful in 4m16s

Details

Peregrine CI / Instrumented Tests (API 29+) (pull_request) Has been skipped

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

fix: DPoP nonce handling, did:web resolution, PAR request_uri encoding (P1-2, P1-3, P1-4, P2-3) #97

P1-3 / P1-4 — did:web resolution + DID document caching (commit 5e4efff)

P1-2 / P2-3 — DPoP nonce retries + PAR request_uri encoding (commits d3cffd0..6fddb50)

Review pass 1 — revise

Merged to main (87933e8)

Pull request closed

P1-3 / P1-4 — did:web resolution + DID document caching (commit `5e4efff`)

Merged to main (`87933e8`)