fix(auth-service): "Another account" lands on email form even with login_hint#141
fix(auth-service): "Another account" lands on email form even with login_hint#141
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🦋 Changeset detectedLatest commit: 592a7a6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
🚅 Deployed to the ePDS-pr-141 environment in ePDS
|
Coverage Report for CI Build 25223213611Coverage increased (+2.0%) to 53.156%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Fixes an auth-service /oauth/authorize edge case where clicking “Another account” from the enriched chooser could land users on the previous account’s OTP step when the original OAuth flow included a login_hint, by honoring prompt=login consistently in the login page step selection and prefill logic.
Changes:
- Gate
initialStep,otpAlreadySent, and email prefill onisForceLoginPrompt(req)soprompt=loginreliably starts at the email step. - Add an E2E regression scenario covering the “Another account + login_hint” path.
- Add a patch changeset describing the end-user impact.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/auth-service/src/routes/login-page.ts | Honors prompt=login in step selection, OTP idempotency hinting, and email prefill suppression. |
| features/session-reuse-bugs.feature | Adds an E2E repro scenario for “Another account” preserving login_hint while appending prompt=login. |
| .changeset/another-account-reaches-email-form.md | Documents the patch-level behavioral fix for release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address PR #141 review feedback: - Compute forceLogin up-front so fetchParLoginHint, resolveLoginHint and fetchDeviceAccountEmails are skipped on the "Another account" path. Those internal-API round trips were pure overhead — the result was unused for both rendering (initialStep / emailHint) and for shouldReuseSession (which already short-circuits on isForceLoginPrompt). - Tighten the issue #138 E2E repro: assert #email is empty and #step-otp.active is hidden, not just that the email form is reachable. This pins down the full set of behaviours described in the issue. - Drop the Client app developers section from the changeset — the fix requires no integration changes, and per the writing-changesets skill an audience-specific section is only worthwhile if the reader needs to adapt their code.
aspiers
force-pushed
the
fix/another-account-skips-email
branch
from
12d3a4b to
b8b4d38
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address PR #141 review feedback (copilot, @aspiers). The original isForceLoginPrompt() check `p === 'login'` only matched the exact single-token literal. Per OIDC Core 1.0 §3.1.2.1, prompt is a space-delimited list (e.g. `prompt=login consent`), and Express's req.query parser surfaces repeated keys as arrays (`?prompt=login&prompt=consent` -> `['login','consent']`). Either shape would have re-introduced the issue #138 behaviour for clients that pass them. pds-core's auth-ui-guard already had a correct parsePromptTokens() implementation (PR #129). Lift it to @certified-app/shared as oidc-prompt.ts so both packages consume the same canonical version, and rewrite isForceLoginPrompt to delegate to promptHasLogin. Keep the auth-ui-guard names as thin re-exports of the shared functions so existing imports (and the auth-ui-guard.test.ts test file) don't churn. Tighten the prompt=login test in login-page-prompt-login.test.ts: the original assertion that fetchDeviceAccountEmails wasn't called passed trivially because the request didn't carry a dev-id/ses-id cookie pair, and that call is gated on cookie presence anyway. Send a real cookie pair so the assertion now genuinely catches a regression that re-introduced device-email fetching on the prompt=login path. Verified via local sabotage: stripping the forceLogin gates causes 2/3 tests to fail as expected. Add session-reuse.test.ts coverage for the new behaviours: prompt as space-delimited list, prompt as array, and the negative case (list of non-login tokens).
Address PR #141 review feedback (copilot, @aspiers). The original isForceLoginPrompt() check `p === 'login'` only matched the exact single-token literal. Per OIDC Core 1.0 §3.1.2.1, prompt is a space-delimited list (e.g. `prompt=login consent`), and Express's req.query parser surfaces repeated keys as arrays (`?prompt=login&prompt=consent` -> `['login','consent']`). Either shape would have re-introduced the issue #138 behaviour for clients that pass them. pds-core's auth-ui-guard already had a correct parsePromptTokens() implementation (PR #129). Lift it to @certified-app/shared as oidc-prompt.ts so both packages consume the same canonical version, and rewrite isForceLoginPrompt to delegate to promptHasLogin. Keep the auth-ui-guard names as thin re-exports of the shared functions so existing imports (and the auth-ui-guard.test.ts test file) don't churn. Tighten the prompt=login test in login-page-prompt-login.test.ts: the original assertion that fetchDeviceAccountEmails wasn't called passed trivially because the request didn't carry a dev-id/ses-id cookie pair, and that call is gated on cookie presence anyway. Send a real cookie pair so the assertion now genuinely catches a regression that re-introduced device-email fetching on the prompt=login path. Verified via local sabotage: stripping the forceLogin gates causes 2/3 tests to fail as expected. Add session-reuse.test.ts coverage for the new behaviours: prompt as space-delimited list, prompt as array, and the negative case (list of non-login tokens).
… test Address PR #141 review feedback (copilot). The original beforeEach created its own `EpdsDb` against `dbPath`, then `Object.defineProperty`-overwrote `ctx.db` to point at it. The constructor-created instance was orphaned: never closed, holding a file descriptor + a SQLite write lock + the WAL/SHM companion files until process GC. Same dual-handle WAL antipattern that motivated 0f2b19d's switch from a second better-sqlite3 connection to reusing PDS's Kysely instance — two writers on one file is opaque trouble. The original "share state with the handler" rationale didn't apply: no test in the file actually seeds or inspects via the test-side db handle. Drop it. AuthServiceContext opens its own EpdsDb; we use ctx.db (transitively) for everything. One handle per test, closed cleanly by ctx.destroy(). Also tidy the afterEach: replace the empty-catch unlink with fs.rmSync(force:true), and clean up the WAL/SHM companions so the tmp directory isn't left with stale -wal/-shm files when WAL hasn't checkpointed by close time.
aspiers
force-pushed
the
fix/another-account-skips-email
branch
from
47a62e9 to
83e8012
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r account" Replace the prior approach that gated rendering on prompt=login with a dedicated signal carrying the actual user intent. The prior approach broke the @session-reuse e2e scenario "login_hint narrows to a stale binding on a multi-account device": pds-core's auth-ui-guard appends prompt=login on its sign-in-view bounce while still expecting auth-service to honour the PAR's login_hint and serve the OTP step. Conflating prompt=login with the "Another account" rebind made auth-service skip hint resolution on that bounce too, so no OTP email was sent and the user got stuck on an empty email form. Solution: - pds-core's chooser-enrichment "Another account" rebind now sets epds_skip_par_hint=1 (in addition to the existing prompt=login for shouldReuseSession bypass) and drops any URL login_hint. Comment block updated to spell out why prompt=login alone is ambiguous and the skip flag is needed. - auth-service's GET /oauth/authorize gates fetchParLoginHint on the absence of epds_skip_par_hint=1. Once the PAR hint is suppressed, resolvedEmail ends up null and the spec-correct rendering decision (hasLoginHint -> initialStep) drops out: email form for issue #138, OTP form for the auth-ui-guard bounce. - Drop the forceEmailForm override and the unused isForceLoginPrompt import — no longer needed once the input to the rendering decision is correctly suppressed. Unit tests in login-page-prompt-login.test.ts updated: - "renders the email step on the Another account rebind path" asserts fetchParLoginHint is NOT called when epds_skip_par_hint=1. - "honours PAR login_hint when prompt=login arrives without the skip flag" pins the auth-ui-guard bounce path so a future simplification doesn't re-conflate the two signals. Verified locally: - 20/20 session-reuse e2e (was 19/20 — the stale-binding scenario now passes again). - 897/897 unit tests, lint/format/typecheck clean.
This comment has been minimized.
This comment has been minimized.
The CI e2e workflow already polls /health for up to 5 minutes per service before invoking the suite, so by the time `pnpm test:e2e` runs the deploy is reachable. But a single transient Railway-edge or undici DNS blip 12 seconds later on the first scenario's `Given the ePDS test environment is running` step kills an entire 9-minute e2e run — observed on PR #141 commit dfee0f8. Budget 6 attempts × 2s = ~12s of retries on transient failures (undici TypeError, non-OK status). Real outages still surface once the budget is spent. Reproduced from CI logs: pre-step health check passed all 5 services at 15:52:47, scenario 1 fetch threw `TypeError: fetch failed` at 15:52:59, all 60 subsequent scenarios passed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/common.steps.ts`:
- Around line 29-41: The loop lacks a per-attempt fetch timeout so a hung
request can blow past the intended ~12s budget; update the fetch call inside the
retry loop to use AbortSignal.timeout(perAttemptMs) (perAttemptMs =
Math.ceil(12_000 / maxAttempts) or 2000) by passing the signal option to fetch
(replace fetch(url) with fetch(url, { signal: AbortSignal.timeout(perAttemptMs)
})); keep the existing error handling that assigns lastErr and backoff sleep
logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0adf3dd-acff-4982-b936-a53cc1c52bdf
📒 Files selected for processing (12)
.changeset/another-account-reaches-email-form.mde2e/step-definitions/common.steps.tse2e/step-definitions/session-reuse.steps.tsfeatures/session-reuse-bugs.featurepackages/auth-service/src/__tests__/login-page-prompt-login.test.tspackages/auth-service/src/__tests__/session-reuse.test.tspackages/auth-service/src/lib/session-reuse.tspackages/auth-service/src/routes/login-page.tspackages/pds-core/src/auth-ui-guard.tspackages/pds-core/src/chooser-enrichment.tspackages/shared/src/index.tspackages/shared/src/oidc-prompt.ts
✅ Files skipped from review due to trivial changes (2)
- packages/shared/src/index.ts
- .changeset/another-account-reaches-email-form.md
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/step-definitions/session-reuse.steps.ts
- packages/auth-service/src/lib/session-reuse.ts
- features/session-reuse-bugs.feature
- packages/shared/src/oidc-prompt.ts
…und retry fetch Address PR #141 review feedback (copilot, coderabbit). - AuthServiceContext's setInterval was never cleared in destroy(), so vitest workers constructing one context per test would accumulate timers across tests. Store the handle, clearInterval() in destroy(), and unref() it so a short-lived test process isn't held alive by the timer alone if destroy() is somehow missed. - The /health retry helper added in 15814d7 had no per-attempt fetch timeout, so a hung connection could blow past the documented ~12s budget. Add AbortSignal.timeout(2s) per attempt; the worst-case wall-clock is now bounded at 6 × (2s fetch + 2s backoff) = ~24s.
|
|
(reply generated by Claude Code, model claude-opus-4-7) Replying to coderabbit's "Out of Scope Changes" pre-merge check, which flagged the Fair flag, but the retry change is not unrelated — it was triggered by a CI failure that landed on this PR. Commit Splitting it into a separate PR would have left this PR's CI red on a flake unrelated to the actual fix, blocking merge and burning more CI cycles on rerun cycles. Adding it here keeps this PR's CI signal honest and prevents the same flake from blocking unrelated PRs in the same docker-compose / Railway environment until that separate PR landed. The change itself is also tightly scoped — it only touches the If reviewers prefer this be split anyway, happy to revert here and re-land in a follow-up PR. |
This comment has been minimized.
This comment has been minimized.
|
Tested to death and urgent need for a stable release -> merging as discussed in Telegram |
Summary
login_hint.epds_skip_par_hint=1) set bypds-core's "Another account" rebind, soauth-serviceknows to ignore any PAR-storedlogin_hinton that specific path.prompt=loginalone keeps its existing semantics (used by the auth-ui-guard's sign-in-view bounce, where the hint must still resolve and the OTP step must still render).isForceLoginPrompt()to handle space-delimited (prompt=login consent) and array (prompt=['login']) forms via a sharedoidc-promptmodule, lifted from pds-core's existingparsePromptTokensso both packages consume the same canonical implementation.features/session-reuse-bugs.feature(verified failing pre-fix, passing post-fix) plus route-level unit tests pinning the two distinguishable cases.Why
pds-core/chooser-enrichment.ts's "Another account" rebind navigates back toauth-service/oauth/authorize, preserving the OAuth request_uri so the flow can resume after the new sign-in. The pre-fix rebind only appendedprompt=login. That worked forshouldReuseSession()(which honouredprompt=loginto skip the chooser redirect) but the login-page handler still resolved the original PAR-bodylogin_hint, setinitialStep='otp'and pre-filled with the previous account's email — exactly the bug from issue #138.A first-cut fix gated
initialStep/otpAlreadySent/ email-prefill onisForceLoginPrompt(req). Local e2e revealed that broke the@session-reusescenario "login_hint narrows to a stale binding on a multi-account device":pds-core's auth-ui-guard sign-in-view bounce ALSO appendsprompt=login(to defeat session reuse) while still relying on the PAR-bodylogin_hintresolving so the OTP step renders. The two paths needed disambiguating.Fix
Two distinct signals, two distinct meanings:
prompt=login(OIDC standard, kept verbatim) — bypass session reuse. Set by both the chooser's "Another account" rebind AND the auth-ui-guard's sign-in-view bounce.epds_skip_par_hint=1(new, ePDS-private) — ignore anylogin_hintstored in the PAR for this request. Set ONLY by the chooser's "Another account" rebind.In
auth-service's/oauth/authorizehandler,fetchParLoginHintis gated on the absence ofepds_skip_par_hint=1. With the PAR hint suppressed and the URLlogin_hintalready stripped by the rebind,resolvedEmailends up null on the "Another account" path and the spec-correct rendering decision (hasLoginHint→initialStep) drops out: email form for issue #138, OTP form for the auth-ui-guard bounce. NoforceEmailFormoverride needed.isForceLoginPrompt()now delegates to a sharedpromptHasLogin()in@certified-app/shared, which correctly handles the OIDC space-delimited list shape and Express's array-of-strings shape from repeated query keys. The auth-ui-guard's existingparsePromptTokenswas lifted to the shared module to keep one canonical implementation.Test plan
"Another account" with login_hint must reach the email forminfeatures/session-reuse-bugs.feature— fails pre-fix, passes post-fix; tightened withemail input is emptyandOTP verification step is not activeassertions."Another account" on the chooser goes to the email form(no-hint path),Signed-in user can sign in as a different account from the chooser,Login hint matches a bound account — chooser still wins, and cruciallylogin_hint narrows to a stale binding on a multi-account device(pinned the case-B regression).Expired PAR shows a styled HTML page instead of leaking JSONstill passes locally.packages/auth-service/src/__tests__/login-page-prompt-login.test.tspin: "rebind path renders email step with empty input AND skips fetchParLoginHint" + "prompt=login without skip flag still resolves the PAR hint and serves OTP" + "no prompt=login still resolves hint normally". Verified by local sabotage.session-reuse.test.tsextended for space-delimited + arraypromptshapes.pnpm format:check,pnpm lint,pnpm typecheck,pnpm test(897/897), full local e2e in 3 phases (issue "Another account" in account chooser skips straight to OTP form #138 scenarios, PR fix: graceful OAuth error when PAR has expired (no more raw JSON leak) #128 PAR scenario, full session-reuse profile 20/20).🤖 Generated with Claude Code
Summary by CodeRabbit