Skip to content

fix(server): bound DPoP replay state - #8845

Closed
solomonneas wants to merge 6 commits into
pingdotgg:mainfrom
solomonneas:fix/dpop-replay-retention
Closed

solomonneas wants to merge 6 commits into
pingdotgg:mainfrom
solomonneas:fix/dpop-replay-retention

Conversation

@solomonneas

@solomonneas solomonneas commented Aug 31, 2026 •

Copy link
Copy Markdown

Fixes #9040

DPoP authentication previously created a permanent secrets-store file for every accepted proof. This change records empty, digest-named markers in overlapping ten-minute buckets and prunes only expired buckets. Claims write and sync the future bucket before the current bucket so a partial write still blocks replay across a boundary.

Bootstrap credentials are validated without consuming them before replay state is allocated. The final atomic consume still decides whether the exchange succeeds. Replay rejection during token exchange includes the DPoP challenge header.

Upgrade requirement: stop all older servers sharing the secrets directory and drain their requests before starting the new version. Mixed-version operation is unsupported. Existing canonical legacy markers are always checked and never created or deleted by the new store. New traffic uses bounded bucket storage, while existing legacy files remain as fixed migration residue. Rollback requires stopping all new-version writers and waiting at least six minutes before starting an older server. See the migration guidance.

Verification:

  • Focused replay, bootstrap, environment-authentication, and server tests: 219 passed across six files.
  • Server typecheck and targeted lint/format checks: passed.
  • New migration tests reproduced five failures before the fix, including late legacy replay, marker deletion, and unexpected lookup errors.
  • Existing bucket-boundary and partial-write regression tests still pass. Independent-store concurrency permits only one successful claim.

Model and harness: GPT-6 via Codex.

Summary by CodeRabbit

  • New Features

    • Added stronger DPoP replay protection using time-based tracking.
    • Bootstrap credentials can now be validated without being consumed.
    • Invalid credential attempts no longer consume a DPoP proof’s replay state.
  • Bug Fixes

    • Replayed DPoP proofs are rejected consistently, including across time boundaries.
    • Rejected DPoP requests now include the appropriate authentication challenge.
  • Documentation

    • Added upgrade and rollback guidance for the new replay-protection behavior, including server coordination and clock requirements.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 478b5b59-605a-41ba-9522-eb05d3b0ef14

📥 Commits

Reviewing files that changed from the base of the PR and between af2bacc and 9eaf1d0.

📒 Files selected for processing (10)
  • apps/server/src/auth/DpopReplayStore.test.ts
  • apps/server/src/auth/DpopReplayStore.ts
  • apps/server/src/auth/EnvironmentAuth.ts
  • apps/server/src/auth/PairingGrantStore.test.ts
  • apps/server/src/auth/PairingGrantStore.ts
  • apps/server/src/auth/dpop.test.ts
  • apps/server/src/auth/dpop.ts
  • apps/server/src/auth/http.ts
  • apps/server/src/server.test.ts
  • docs/internals/environment-auth.md

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a bucketed filesystem-backed DPoP replay store, preserves compatibility with legacy markers, validates bootstrap credentials before claiming proofs, and integrates replay claims into token exchange authentication.

Changes

DPoP replay protection

Layer / File(s) Summary
Bucketed replay store
apps/server/src/auth/DpopReplayStore.ts, apps/server/src/auth/DpopReplayStore.test.ts
Adds typed replay-store errors, atomic markers in current and future ten-minute buckets, legacy-marker checks, pruning, scheduled cleanup, and filesystem failure coverage.
Bootstrap credential availability validation
apps/server/src/auth/PairingGrantStore.ts, apps/server/src/auth/PairingGrantStore.test.ts
Adds non-consuming credential validation for expiry, revocation, consumption, and proof-key matching.
DPoP authentication integration
apps/server/src/auth/dpop.ts, apps/server/src/auth/http.ts, apps/server/src/auth/EnvironmentAuth.ts, apps/server/src/auth/dpop.test.ts, apps/server/src/auth/server.test.ts
Moves replay claiming into the DPoP replay store, validates credentials before claiming proofs, maps replay failures, and tests DPoP response behavior.
Upgrade and rollback documentation
docs/internals/environment-auth.md
Documents version coordination, legacy marker compatibility, rollback timing, and clock requirements.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant TokenHandler
  participant EnvironmentAuth
  participant DpopReplayStore
  Client->>TokenHandler: Send token exchange with DPoP proof
  TokenHandler->>EnvironmentAuth: Validate bootstrap credential and proof thumbprint
  EnvironmentAuth-->>TokenHandler: Return validation result
  TokenHandler->>DpopReplayStore: Claim proof thumbprint and jti
  DpopReplayStore-->>TokenHandler: Return claim result
  TokenHandler-->>Client: Return token or DPoP challenge
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 9eaf1

The upgrade documentation already requires the operational sequencing needed to preserve replay protection across versions.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #9040 requires atomic replay state that remains bounded and bootstrap validation before replay-state writes. DpopReplayStore uses digest-named markers under dpop-replay, writes the next buck…
Out of Scope Changes check ✅ Passed The changed store, authentication integration, tests, DPoP challenge behavior, and migration documentation all support issue #9040. No unrelated change is demonstrated by the reviewed evidence.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9…
Title check ✅ Passed The title clearly and concisely describes the main change: bounding DPoP replay state.
Description check ✅ Passed The description explains what changed, why it changed, compatibility and rollback requirements, verification results, and the migration guidance. It does not use the template headings or include the c…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Aug 31, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the new DpopReplayStore service and its call sites against the Effect service conventions. Module layout (imports → errors → Context.Service tag → make → layer), namespace imports, Foo["Service"] typing, environment-based dependency acquisition and the exported Schema.is union predicate all look right. Three error-modeling/catch findings below.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/auth/dpop.ts Outdated
Comment thread apps/server/src/auth/DpopReplayStore.ts Outdated
Comment thread apps/server/src/auth/DpopReplayStore.ts Outdated
Comment thread apps/server/src/auth/DpopReplayStore.ts Outdated
Comment thread apps/server/src/auth/http.ts
@macroscopeapp

macroscopeapp Bot commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR adds substantial persistent replay-state machinery and changes production DPoP authentication and bootstrap-token flows in a security-sensitive auth package. Unresolved findings also concern rolling-upgrade replay protection and DPoP challenge responses, so the change warrants human review.

Not approved because:

  • 2 blocking correctness issues found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@github-actions github-actions Bot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Aug 31, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the new DpopReplayStore service and its call sites against the Effect service conventions. The earlier findings (structural replay-detection error, structural claim/prune error attributes, catchIf with a schema predicate) are addressed in this revision. One remaining item: DpopReplayStoreSetupError drops the path context available at its wrapping site.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/auth/DpopReplayStore.ts Outdated
Comment thread apps/server/src/auth/DpopReplayStore.ts
@shivamhwp

Copy link
Copy Markdown
Collaborator

Note: GPT-6 on behalf of shivam (@shivamhwp).

The compatibility lifetime is still tied to this instance's startup rather than the lifetime of proofs accepted by every writer sharing secretsDir.

  • Start the new store at t=0. At t=359s, an old server accepts a fresh proof and atomically creates its legacy marker. At t=361s, the new store skips claimLegacyMarker and accepts the same thumbprint/jti. That proof is only two seconds old, well inside the five-minute acceptance window.
  • Start another new-version store at t=420s and let it claim a proof. Its six-minute compatibility period writes a fresh legacy marker, but calling prune on the first store immediately deletes that marker. An old server can then accept the same proof during the newer store's advertised compatibility window.

pruneLegacy cannot erase every canonical legacy marker merely because one process is six minutes old, and claim cannot stop consulting that namespace while older writers may still accept proofs. This needs a coordinated migration boundary covering all writers, or an explicit requirement to stop old servers before transitioning. A longer process-local timer leaves the same failure at a later cutoff. Avoid bringing back unprotected stat-then-delete cleanup of reusable canonical marker names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL 500-999 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: DPoP replay markers grow without bound

2 participants