Fix host-only cookie shadowing OTP loop (#116) - #117
Merged
Conversation
Two failing e2e scenarios that exercise the OTP-loop trap diagnosed in GitHub issue #116. An affected user has a stale host-only dev-id/ses-id pair on the pds-core host (left over from a session predating the cookie-domain broadening in PR #103). The host-only pair shadows everything because the cookie parser keeps the first occurrence per name and per RFC 6265 host-only comes first in the Cookie header. Scenarios: - "Bounce response actually clears the host-only cookies" — drives the OAuth entry point and asserts the response that serves the email/OTP form has evicted the host-only pair from the jar. Currently fails: the welcome-page-guard's host-only clear is silently rewritten by the cookie-domain middleware to be Domain-scoped, so the host-only entry survives every bounce. - "User can complete OTP sign-in despite stale host-only cookies" — end-to-end loop reproduction. An affected user submits a valid OTP and must reach the consent screen / app rather than be bounced back to the OTP form. Currently fails: the OTP form is re-rendered after every submit because the stale host-only pair keeps shadowing the fresh Domain-scoped pair epds-callback sets. Both scenarios reuse the existing welcome-page-guard step harness and the Background's "returning user has a PDS account" step (which clears the browser context to a clean jar). A new Given plants the host-only stale pair directly via Playwright's addCookies API with domain: <host> (no leading dot). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rewriter Bug 2 from GitHub issue #116. The cookie-domain middleware rewrites device-cookie Set-Cookie headers to add Domain=<parent> so auth-service on the sibling subdomain can read upstream's session cookies. Until now it did this unconditionally, including for clearing cookies (Max-Age=0). That broke welcome-page-guard's bounce path: the guard intentionally emits two Set-Cookie lines per stale device cookie — one host-only (no Domain=) and one Domain-scoped — to evict cookies in both scopes. The middleware silently rewrote the host-only line to be Domain-scoped, identical to the second line, so the host-only stale entry survived every bounce. A user whose jar held a host-only dev-id/ses-id pair from before PR #103 (cookie-domain broadening) thus got trapped in an OTP loop with no way out. Fix: rewriteSetCookie now passes through clearing cookies untouched. A clear is identified by Max-Age=0 (canonical form upstream uses) or a past Expires date. Normal Set-Cookie values continue to get Domain= injected. Drop the standalone e2e scenario that asserted the host-only clear because it can't actually exercise the bounce path with only host-only cookies in the jar (auth-service can't see them on the subdomain, so the flow lands on the email form without ever reaching pds-core's guard). Unit tests cover the rewriter at the function level. The remaining e2e scenario in the feature file proves the user-facing outcome via the post-OTP path, where bug 1's fix (still pending) is the primary lever. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug 1 from GitHub issue #116. Browsers store host-only and Domain-scoped cookies of the same name as two distinct entries in the jar. A user whose jar held a host-only dev-id/ses-id pair from before PR #103 (cookie-domain broadening) accumulates two complete pairs after the post-OTP /oauth/epds-callback redirect — host-only stale alongside the fresh Domain-scoped one we just minted. The cookie parser used by both upstream and welcome-page-guard keeps the first occurrence per name, which per RFC 6265 ordering is the host-only stale value. The guard validates the wrong values, fails its check, and bounces to auth-service with prompt=login. The user loops on the OTP form forever. Fix: in the same response that emits the fresh Domain-scoped cookies, also emit explicit host-only Max-Age=0 clears for dev-id, ses-id, and their :hash sidecars. The browser evicts any host-only twin before the very next request, so the welcome-page-guard at /oauth/authorize sees only the fresh pair. Idempotent — emitting clears for cookies that don't exist is a no-op. Relies on the cookie-domain middleware's recent fix to leave Max-Age=0 lines untouched (commit before this one). Without that, the clears would be silently rewritten to Domain-scoped and never reach the host-only entry. Also strengthen the e2e scenario assertion: replace the weak "browser does not land back on the OTP form" check with the existing "browser is redirected back to the demo client" step, which proves the user reaches /welcome with a valid token rather than just leaving the OTP page. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change fixes an issue where ePDS fails to recover from host-only device/session cookies by adding cookie clearing logic, E2E test infrastructure, and a feature test. The implementation detects cookie eviction instructions and prevents domain-rewriting of clearing cookies, while the callback route explicitly emits host-scoped clears to evict stale cookie twins before continuing OAuth flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ePDS-pr-117 environment in ePDS
|
Coverage Report for CI Build 25082001533Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 47.832%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Bug fix to the bug fix in the previous commit. The unconditional host-only Max-Age=0 clears in /oauth/epds-callback evicted the freshly-minted device cookies on deployments where cookie-domain broadening is a no-op (Railway preview envs, where auth-service and pds-core live under unrelated *.up.railway.app hostnames). Without broadening, upstream's Set-Cookie emits the device cookies host-only — exactly the scope the clears target, so the browser sees the fresh cookies set and immediately deleted in the same response. Surfaced by CI's e2e suite against the PR-117 Railway preview: nine post-OTP scenarios timed out waiting for /welcome with the browser stuck on the bounce target /oauth/authorize?prompt=login. Local docker stack (where auth-service IS a subdomain of pds-core) was unaffected because broadening is active there. Fix: only emit the host-only clears when deriveCookieDomain returned non-null, i.e. when the cookie-domain middleware is actively rewriting device cookies to be Domain-scoped. On the no-broadening path the device cookies are already host-only and there is no twin to evict. Drive-by: hoist deriveCookieDomain to the top of main() so the same value is used for the welcome-page-guard, cookie-domain middleware, and the host-only clear gate, eliminating a duplicate computation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aspiers
commented
Apr 28, 2026
aspiers
commented
Apr 28, 2026
…ario Address review feedback on PR #117: the bespoke "the user submits a valid OTP for the existing account" step duplicated the email-fill, OTP-wait, mailpit-fetch, and code-fill mechanics already covered by existing steps in auth.steps.ts and email.steps.ts. Decompose the scenario into the existing four-step sequence: And the user enters the test email on the login page Then the login page shows an OTP verification form And an OTP email arrives in the mail trap When the user enters the OTP code Drop the now-unused "submits a valid OTP for the existing account" step definition. The reused steps are slightly more verbose at the feature level but match the phrasing every other OTP scenario uses, and they thread world.otpCode through the canonical mailpit-fetch helper instead of inlining it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…reuse-bugs Address review feedback on PR #117: the standalone session-reuse-host-only-shadowing.feature file is unnecessary; the scenario fits the existing session-reuse-bugs.feature thematically and sequencing-wise. Merge it back in alongside the other welcome-page-guard scenarios. The previous separation rationale was that the affected-user starting state (only host-only stale cookies, no Domain-scoped pair) couldn't coexist with the Feature-level Background's "user has completed one OAuth sign-in" step (which deposits a valid Domain-scoped pair). Resolved by having the new "browser jar holds only a stale host-only dev-id and ses-id pair" Given clear the cookie jar before planting, so it works regardless of whether the Background left it empty or populated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the infinite OTP loop diagnosed in #116. A user whose browser jar carries a stale host-only
dev-id/ses-idpair from before PR #103 (cookie-domain broadening) gets trapped on the OTP form: every OAuth attempt completes the OTP step, redirects through/oauth/epds-callback, then gets bounced from/oauth/authorizeback to OTP because the welcome-page-guard validates against the stale host-only values instead of the freshly-set Domain-scoped pair.Two narrow changes resolve it; either alone is insufficient.
Bug 2 —
cookie-domainmiddleware destroys host-only cookie clearsrewriteSetCookierewrote every device-cookieSet-Cookieto addDomain=<parent>, includingMax-Age=0clears. The welcome-page-guard's bounce response intentionally emits TWO clears per cookie name (one host-only, noDomain=; one Domain-scoped) so browsers evict cookies in both scopes. The middleware silently rewrote the host-only line to be Domain-scoped, identical to the second line, so the host-only stale entry survived every bounce.Fix:
rewriteSetCookienow skips clearing cookies (detected byMax-Age=0or pastExpires). NormalSet-Cookievalues continue to getDomain=injected. Unit tests added for each device cookie name, whitespace tolerance, pastExpires, and the no-double-inject guard.Bug 1 — Stale host-only entry parsed first, shadowing the freshly-set Domain-scoped pair
When a request carries both pairs in the Cookie header, the
cookiepackage'sparse()(used by both upstream'sparseHttpCookiesand welcome-page-guard'sparseDeviceCookies) keeps the first occurrence per name. Per RFC 6265 §5.4 ordering, the host-only stale entry comes first when its creation time predates the Domain-scoped pair (Ken's case — host-only existed pre-PR-#103). The guard validates the wrong values, fails, bounces.Fix: in
/oauth/epds-callback, the response that emits the fresh Domain-scoped cookies also emits explicit host-onlyMax-Age=0clears fordev-id,ses-id, and their:hashsidecars. The browser evicts any host-only twin before the very next request, so the welcome-page-guard at/oauth/authorizesees only the fresh pair. Idempotent — clearing cookies that don't exist is a no-op. Relies on Bug 2's fix to pass the host-only clears through unchanged.Tests
cookie-domain.test.tsfor the rewriter's clearing-cookie behaviour (Max-Age=0 host-only clears pass through unchanged for each device cookie name; positive Max-Age and future Expires still getDomain=injected; whitespace tolerated).features/session-reuse-host-only-shadowing.feature: an affected user (only host-only stale pair on the jar, no Domain-scoped, no live device session) starts an OAuth flow and submits a valid OTP. They must reach the demo client's/welcomepage rather than loop.Changeset
No new changeset. This is a fix for an unreleased feature (PR #103,
.changeset/cross-client-session-reuse.mdis unreleased). The existing changeset's user-facing claim ("If your browser's leftover sign-in cookies no longer match the server, you land on the familiar email code form rather than a generic sign-in screen") still holds post-fix — the bug was that the recovery path looped infinitely instead of recovering, not that the recovery existed at all.Architectural follow-up (NOT in this PR)
The welcome-page-guard runs on every GET to
/oauth/authorizeand/account*, so any future cookie-handling regression or new entry point can become an OTP loop the same way this bug did. Issue #116 flags a less invasive alternative (intercept upstream's welcome-page response and replace it, rather than pre-empting every request) for separate evaluation.Test plan
pnpm vitest run packages/pds-core/src/__tests__/cookie-domain.test.ts— 33/33 passingpnpm test:e2e:headless -p session-reuse— 16/16 passing against local docker stackdev-id/ses-idonepds-poc1.test.certified.app, log in via demo, reach/welcomewithout loopingepds1.test.certified.appwith Ken's account once deployedCloses #116
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests