From ccf4ece08d8583127600accdef81ee6fe126a8f7 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 03:36:25 +0000 Subject: [PATCH 01/37] fix(auth-service): hide Resend on OTP screen when sign-in cannot recover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the OTP screen always offered "Resend code", even when the upstream PAR row had silently lapsed (suspended tab, mobile background, heartbeat throttling). The user could click Resend, receive a fresh email, type the new code, and only then see "Sign in failed" — wasting their time on a code that could not have worked. The screen now never surfaces actions that cannot complete the flow: - Track lastSuccessfulHeartbeatAt; treat the PAR as dead once we cross upstream's 5 min AUTHORIZATION_INACTIVITY_TIMEOUT without a fresh ok ping (the upstream death point is exact — no margin needed). - Hide #btn-resend and surface a #btn-start-over (→ /auth/abort) the moment parLikelyDead() flips. Reconciled on every heartbeat tick (including transient ticks, so a stale-by-time case still hides the button) and on the visibilitychange event (so a backgrounded tab returning to focus reflects reality immediately). - Inline "Send a new code" action on the OTP-expired error now branches: parLikelyDead() → "Start over"; otherwise existing "Send a new code". This is the proactive UI complement to the existing reactive abort gate. Server-side enforcement of the same invariant on /email-otp/send- verification-otp and /sign-in/email-otp is a separate follow-up. Test: new @resend-hidden-when-par-dead scenario; full @otp-and-par-expiry / @par-heartbeat / @resend-after-par-dead / @otp-expiry suite still passes (7 scenarios, 78 steps). Co-Authored-By: Claude Opus 4.7 (1M context) --- ...hide-resend-when-sign-in-cannot-recover.md | 13 +++ e2e/step-definitions/auth.steps.ts | 46 ++++++++ features/passwordless-authentication.feature | 22 ++++ .../auth-service/src/routes/login-page.ts | 105 ++++++++++++++++-- 4 files changed, 174 insertions(+), 12 deletions(-) create mode 100644 .changeset/hide-resend-when-sign-in-cannot-recover.md diff --git a/.changeset/hide-resend-when-sign-in-cannot-recover.md b/.changeset/hide-resend-when-sign-in-cannot-recover.md new file mode 100644 index 00000000..3bc48815 --- /dev/null +++ b/.changeset/hide-resend-when-sign-in-cannot-recover.md @@ -0,0 +1,13 @@ +--- +'ePDS': patch +--- + +Sign-in no longer offers "Resend code" when the new code wouldn't have worked anyway. + +**Affects:** End users + +**End users:** Previously, if you sat on the email-code step long enough that the underlying sign-in had silently timed out (most often: leaving the tab in the background while reading email on your phone, or coming back after an interruption), the page would still show **Resend code**. Clicking it sent you a fresh email, but the moment you typed the new code you'd see "Sign in failed" — the code was issued for a sign-in that could no longer complete, so it never had a chance. + +The page now hides the Resend button as soon as it knows the sign-in can't be recovered, and shows **Start over** in its place. Clicking Start over takes you back to the app you came from to begin again, instead of letting you waste time on a code that couldn't work. + +If you're actively using the page (the tab in the foreground), nothing changes: Resend stays available and works the same way it always has. diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 89343dba..576bcccf 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -839,3 +839,49 @@ Then( } }, ) + +// --------------------------------------------------------------------------- +// Resend-button visibility (fix for the "fresh OTP wasted on dead PAR" UX) +// --------------------------------------------------------------------------- +// +// The page never offers an action that cannot complete the flow. When the +// PAR is dead, the standalone Resend button is removed from view and a +// Start over button takes its place — clicking it bails to /auth/abort +// rather than issuing an OTP that would only fail downstream. The steps +// below trigger the page's reactive ping (via the visibilitychange +// handler that fires on tab-foreground) so it can observe the dead PAR +// and reconcile the UI without a 5-minute wall-clock wait. + +When('the OTP form re-checks PAR liveness', async function (this: EpdsWorld) { + const page = getPage(this) + // Drive the page's reactive ping via a string-source script. + // Using page.evaluate(() => ...) inlines esbuild's __name helper, + // which then fails in Playwright's evaluation context with + // "ReferenceError: __name is not defined". Passing a string + // bypasses the bundler. + await page.evaluate(`(function () { + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get: function () { return 'visible' }, + }) + document.dispatchEvent(new Event('visibilitychange')) + })()`) +}) + +Then( + 'the Resend code button is no longer offered', + async function (this: EpdsWorld) { + const page = getPage(this) + await expect(page.locator('#btn-resend')).toBeHidden({ timeout: 5_000 }) + }, +) + +Then( + 'a Start over button is offered instead', + async function (this: EpdsWorld) { + const page = getPage(this) + await expect(page.locator('#btn-start-over')).toBeVisible({ + timeout: 5_000, + }) + }, +) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index b0692184..ee1888d6 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -343,6 +343,28 @@ Feature: Passwordless authentication via email OTP And the user requests a new OTP via the resend button Then the browser lands back at the demo client with an auth error + # The page never offers actions that cannot complete the flow. When + # the upstream PAR has died (silent timeout, suspended tab, + # heartbeat throttling), the standalone Resend button is removed + # from view and replaced with a Start over button — so the user + # never wastes time typing a fresh OTP that could not have worked. + # This is the proactive complement to @resend-after-par-dead's + # reactive abort gate: rather than letting the click happen and + # bouncing it server-side, we surface only forward paths that can + # actually succeed. + @email @otp-and-par-expiry @resend-hidden-when-par-dead + Scenario: Resend button is hidden when the PAR has died — Start over is offered instead + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then an OTP email arrives in the mail trap for the test email + And the login page shows an OTP verification form + When the PAR request_uri has expired before the bridge fires + And the OTP form re-checks PAR liveness + Then the Resend code button is no longer offered + And a Start over button is offered instead + @email @otp-and-par-expiry @prompt-login Scenario: prompt=login + expired PAR — clean exit back to the OAuth client Given a returning user has a PDS account diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 302d8902..b4ba42b8 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -743,17 +743,38 @@ export function renderLoginPage(opts: { var heartbeatEnabled = ${JSON.stringify(opts.heartbeatEnabled)}; var heartbeatHandle = null; var heartbeatIntervalMs = 3 * 60 * 1000; + // Upstream's AUTHORIZATION_INACTIVITY_TIMEOUT — once this much + // wall-clock time has elapsed since our last successful PAR + // refresh, the upstream row is guaranteed to be dead. Used by + // parLikelyDead() to hide Resend before the user can click it. + var parInactivityTimeoutMs = 5 * 60 * 1000; + // Page load is the implicit first PAR refresh — atproto's + // PAR_EXPIRES_IN gives a fresh row 5 min on creation, and the + // user just hit /oauth/authorize seconds ago. Treat now as + // last-known-alive until the first ping confirms otherwise. + var lastSuccessfulHeartbeatAt = Date.now(); // Set to true the moment we know the flow can no longer // complete (PAR or auth_flow gone). Resend / Verify gates // check this so a click that races the proactive notice // still bails to /auth/abort instead of issuing a fresh OTP // that would only fail. var flowAborted = false; + // True iff we have proof the PAR is still alive (last ping + // was ok:true and was recent enough to fall inside the + // upstream inactivity window). Used to gate every "offer the + // user a Resend" decision so they only ever see actions that + // can actually complete the flow. + function parLikelyDead() { + if (flowAborted) return true; + return Date.now() - lastSuccessfulHeartbeatAt >= parInactivityTimeoutMs; + } function pingHeartbeat() { return fetch('/auth/ping', { credentials: 'include', cache: 'no-store' }) .then(function(r) { return r.json(); }) .then(function(body) { - if (body && body.ok === false && body.reason !== 'transient') { + if (body && body.ok === true) { + lastSuccessfulHeartbeatAt = Date.now(); + } else if (body && body.ok === false && body.reason !== 'transient') { // Auth flow / PAR genuinely dead — no point pinging again, // and no point letting the user keep typing. 'transient' // (5xx / network blip) does NOT stop the interval; the @@ -763,7 +784,13 @@ export function renderLoginPage(opts: { } return body; }) - .catch(function() { return null; /* network blip — caller may retry */ }); + .catch(function() { return null; /* network blip — caller may retry */ }) + .finally(function() { + // Always reconcile visibility — a 'transient' tick that + // pushes us past the inactivity window must hide Resend + // even though we never got a definitive 'par_expired'. + refreshResendVisibility(); + }); } function startHeartbeat() { if (!heartbeatEnabled) return; @@ -777,6 +804,16 @@ export function renderLoginPage(opts: { } } window.addEventListener('beforeunload', stopHeartbeat); + // When the tab returns to the foreground after being hidden, + // setInterval may have been throttled enough that PAR has + // silently lapsed. Re-ping immediately so the UI reflects + // reality before the user clicks anything. + document.addEventListener('visibilitychange', function() { + if (document.visibilityState === 'visible' && heartbeatEnabled) { + pingHeartbeat(); + refreshResendVisibility(); + } + }); // Show the proactive "this won't work — start over" notice when // the flow is unrecoverable. Disables the OTP boxes, the verify @@ -819,6 +856,41 @@ export function renderLoginPage(opts: { errorEl.appendChild(startOverBtn); } + /** + * Toggle the standalone Resend button between visible and + * hidden based on whether the PAR is still alive. The button + * is removed from view (display:none) rather than just + * disabled — a button the user cannot productively click + * shouldn't be on the page at all. When hidden, a "Start over" + * link is shown in its place so the user always has a forward + * path. Idempotent — safe to call from heartbeat ticks, + * visibility change handlers, and inline render paths. + */ + function refreshResendVisibility() { + var resendBtn = document.getElementById('btn-resend'); + var startOverLink = document.getElementById('btn-start-over'); + if (!resendBtn) return; + if (parLikelyDead()) { + resendBtn.style.display = 'none'; + if (!startOverLink) { + startOverLink = document.createElement('button'); + startOverLink.type = 'button'; + startOverLink.id = 'btn-start-over'; + startOverLink.className = 'btn-secondary'; + startOverLink.textContent = 'Start over'; + startOverLink.addEventListener('click', function() { + window.location.href = '/auth/abort'; + }); + resendBtn.parentNode.insertBefore(startOverLink, resendBtn); + } + } else { + resendBtn.style.display = ''; + if (startOverLink && startOverLink.parentNode) { + startOverLink.parentNode.removeChild(startOverLink); + } + } + } + /** * Reactive gate used by the Resend and Verify click handlers. * Pings /auth/ping synchronously; if the result indicates the @@ -983,6 +1055,7 @@ export function renderLoginPage(opts: { if (otpBoxes.length) otpBoxes[0].focus(); clearError(); startHeartbeat(); + refreshResendVisibility(); } function showEmailStep() { @@ -1104,15 +1177,21 @@ export function renderLoginPage(opts: { // expired") plus generic "expir"/"too long" variants. var isExpired = /expir|too long/i.test(result.error); if (isExpired) { - // The inline action triggers the same Resend handler, - // which itself runs abortIfFlowDead() before issuing - // a new code. So even if the PAR is dead the user - // gets the spec-compliant bounce rather than a fresh - // OTP that wouldn't work — no need to gate the - // action's visibility separately here. - showErrorWithAction(result.error, 'Send a new code', function() { - document.getElementById('btn-resend').click(); - }); + // Only offer "Send a new code" when the PAR is still + // alive. If it isn't, a fresh OTP would issue but + // never complete — wasting the user's time on a code + // that can't work. Show "Start over" instead so the + // only forward path we surface is one that will + // actually succeed. + if (parLikelyDead()) { + showErrorWithAction(result.error, 'Start over', function() { + window.location.href = '/auth/abort'; + }); + } else { + showErrorWithAction(result.error, 'Send a new code', function() { + document.getElementById('btn-resend').click(); + }); + } } else { showError(result.error); } @@ -1185,8 +1264,10 @@ export function renderLoginPage(opts: { }); } // OTP form is already visible server-side; showOtpStep() never - // ran, so kick off the heartbeat ourselves. + // ran, so kick off the heartbeat ourselves and reflect the + // current PAR-liveness state in the Resend button visibility. startHeartbeat(); + refreshResendVisibility(); } })(); From a5cb519d781bc89d5f8738b73586331e1506c533 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:08:26 +0000 Subject: [PATCH 02/37] fix(demo): cookie outlasts OTP form sit times + honest error when it doesn't MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes that close the bug-report hole on the demo client: 1. Bump oauth_state cookie maxAge from 600s to 3600s, matching auth-service's auth_flow row TTL. The demo's cookie carries the state, code verifier, token endpoint and issuer for the OAuth callback to complete; if it expires before the user submits the OTP, the callback can't find any of that and bounces silently. 600s was shorter than realistic OTP-form sit times (the bug report was an 11-minute wait). Aligning with auth_flow's 60-min budget means as long as the auth-service can still recover the flow, the demo can too. 2. Distinguish "cookie missing" from "auth failed" on the callback. Previously every silent-fail path bounced to /?error=auth_failed ("Authentication failed. Please try again."), which is misleading when the sign-in itself succeeded — the user typed a fresh OTP correctly, the auth-service issued the OAuth code, the demo just couldn't finish because its own session cookie had aged out. Now the cookie-missing branch redirects to /?error=session_expired ("Your sign-in took too long to finish. Please sign in again.") so the user understands what happened without thinking they typed the code wrong. Test: new @demo-cookie-expiry @bug-report scenario clears the cookie mid-flow and asserts the session-expired error surfaces. Co-Authored-By: Claude Opus 4.7 (1M context) --- e2e/step-definitions/auth.steps.ts | 43 +++++++++++++++++++ features/passwordless-authentication.feature | 31 +++++++++++++ .../demo/src/app/api/oauth/callback/route.ts | 13 +++++- .../demo/src/app/api/oauth/login/route.ts | 4 +- .../demo/src/app/components/LoginForm.tsx | 2 + 5 files changed, 89 insertions(+), 4 deletions(-) diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 576bcccf..0fbbfcae 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -885,3 +885,46 @@ Then( }) }, ) + +// --------------------------------------------------------------------------- +// Demo client cookie expiry simulation +// --------------------------------------------------------------------------- +// +// The demo client stores OAuth state (state value, codeVerifier, token +// endpoint, issuer) in a signed cookie called `oauth_state` with +// `maxAge: 600` (see packages/demo/src/app/api/oauth/login/route.ts). +// If the user spends longer than 10 minutes between starting the OAuth +// flow and the callback firing — most realistic cause: dawdling on the +// OTP form, then clicking Resend after the 10-minute mark — the cookie +// expires before the callback runs, so the callback handler can't find +// the OAuth state and silently bounces to /?error=auth_failed. +// +// This step deletes the cookie programmatically so we can exercise the +// post-cookie-expiry callback path without a 10-minute wall-clock wait. + +When( + "the demo client's OAuth state cookie has expired", + async function (this: EpdsWorld) { + const page = getPage(this) + const ctx = page.context() + const before = await ctx.cookies() + const target = before.find((c) => c.name === 'oauth_state') + if (!target) { + throw new Error( + `Expected to find an oauth_state cookie set by the demo client but only saw: ${before.map((c) => c.name).join(', ')}`, + ) + } + await ctx.clearCookies({ name: 'oauth_state' }) + }, +) + +Then( + 'the demo client surfaces a session-expired error', + async function (this: EpdsWorld) { + const origin = new URL(testEnv.demoUrl).origin + const page = getPage(this) + await page.waitForURL(`${origin}/?error=session_expired*`, { + timeout: 30_000, + }) + }, +) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index ee1888d6..20edade8 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -365,6 +365,37 @@ Feature: Passwordless authentication via email OTP Then the Resend code button is no longer offered And a Start over button is offered instead + # The demo OAuth client stores its OAuth state (state value, code + # verifier, token endpoint, issuer) in a signed cookie that has its + # own lifetime. If that cookie expires before the auth-service + # bridges the user back via /oauth/epds-callback (e.g. user + # dawdled on the OTP form long enough that the cookie aged out + # mid-flow), the demo's callback handler can't find the OAuth state + # and silently bounces to the demo home page with + # `?error=auth_failed`. The user has just typed a fresh OTP + # successfully, so this is genuinely time-wasting and misleading: + # the auth-service did everything right but the demo dropped the + # ball. + # + # The contract the demo MUST satisfy: as long as the OAuth flow is + # still recoverable from the auth-service side (auth_flow row + # alive, PAR alive or refreshable), the demo's session cookie must + # also be alive. This scenario reproduces the failure mode by + # programmatically clearing the demo's `oauth_state` cookie just + # before the OTP submission, which is equivalent to the cookie + # having lapsed by wall-clock. + @email @demo-cookie-expiry @bug-report + Scenario: Demo client's OAuth cookie has expired by the time of callback — useful error, not generic auth_failed + When the demo client starts a new OAuth flow with random handle mode + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then an OTP email arrives in the mail trap for the test email + And the login page shows an OTP verification form + When the demo client's OAuth state cookie has expired + And the user enters the OTP code + Then the demo client surfaces a session-expired error + @email @otp-and-par-expiry @prompt-login Scenario: prompt=login + expired PAR — clean exit back to the OAuth client Given a returning user has a PDS account diff --git a/packages/demo/src/app/api/oauth/callback/route.ts b/packages/demo/src/app/api/oauth/callback/route.ts index abaa5811..194a963e 100644 --- a/packages/demo/src/app/api/oauth/callback/route.ts +++ b/packages/demo/src/app/api/oauth/callback/route.ts @@ -48,11 +48,20 @@ export async function GET(request: NextRequest) { return NextResponse.redirect(new URL('/?error=auth_failed', baseUrl)) } - // Retrieve OAuth session from signed cookie + // Retrieve OAuth session from signed cookie. The cookie carries + // the state value, code verifier, token endpoint and issuer that + // we recorded when starting the OAuth flow. If it has gone away + // (cookie expired by browser, user cleared cookies, very long + // wait on the OTP form), there is nothing we can do to complete + // the token exchange — but we owe the user a useful error + // (`session_expired`) rather than a generic `auth_failed`, so + // the landing page can guide them to start over instead of + // looking like the sign-in itself just failed. const cookieStore = await cookies() const stateData = getOAuthSessionFromCookie(cookieStore) if (!stateData) { - return NextResponse.redirect(new URL('/?error=auth_failed', baseUrl)) + console.error('[oauth/callback] Missing oauth_state cookie') + return NextResponse.redirect(new URL('/?error=session_expired', baseUrl)) } if (stateData.state !== state) { diff --git a/packages/demo/src/app/api/oauth/login/route.ts b/packages/demo/src/app/api/oauth/login/route.ts index 30a855e0..c544235c 100644 --- a/packages/demo/src/app/api/oauth/login/route.ts +++ b/packages/demo/src/app/api/oauth/login/route.ts @@ -263,7 +263,7 @@ export async function GET(request: Request) { httpOnly: true, secure: true, sameSite: 'lax', - maxAge: 600, + maxAge: 60 * 60, path: '/', }) return resp2 @@ -281,7 +281,7 @@ export async function GET(request: Request) { httpOnly: true, secure: true, sameSite: 'lax', - maxAge: 600, + maxAge: 60 * 60, path: '/', }) return response diff --git a/packages/demo/src/app/components/LoginForm.tsx b/packages/demo/src/app/components/LoginForm.tsx index 06989549..b0794113 100644 --- a/packages/demo/src/app/components/LoginForm.tsx +++ b/packages/demo/src/app/components/LoginForm.tsx @@ -6,6 +6,8 @@ import { ForceLoginCheckbox } from './ForceLoginCheckbox' const ERROR_MESSAGES: Record = { auth_failed: 'Authentication failed. Please try again.', + session_expired: + 'Your sign-in took too long to finish. Please sign in again.', par_failed: 'Could not start login — the PDS rejected the request. Check server logs.', invalid_email: 'Please enter a valid email address.', From 988be35294a2990a8e84b31906b38028a898a583 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:13:41 +0000 Subject: [PATCH 03/37] fix(auth-service): "Use different email" clears the email field + focuses it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OTP form's "Use different email" button took the user back to the email-entry form but left the previous email pre-filled. That's exactly the misleading "looks like the form remembered me" UX they were trying to escape — and forces an extra clearing keystroke before they can type the new address. Fix: showEmailStep() now resets emailInput.value, currentEmail, and focuses the field so the user can just start typing. Test: new @use-different-email scenario. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/use-different-email-clears-form.md | 9 +++++++++ e2e/step-definitions/auth.steps.ts | 11 +++++++++++ features/passwordless-authentication.feature | 17 +++++++++++++++++ packages/auth-service/src/routes/login-page.ts | 9 +++++++++ 4 files changed, 46 insertions(+) create mode 100644 .changeset/use-different-email-clears-form.md diff --git a/.changeset/use-different-email-clears-form.md b/.changeset/use-different-email-clears-form.md new file mode 100644 index 00000000..3fa6e176 --- /dev/null +++ b/.changeset/use-different-email-clears-form.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +"Use different email" on the sign-in code page now clears the email field. + +**Affects:** End users + +**End users:** clicking **Use different email** on the code-entry page used to take you back to the email form with your previous address still filled in — exactly the opposite of what the button suggests, and forcing you to clear the field manually before you could type a new address. The form now starts empty, with the cursor in the field, so you can just type the new address and continue. diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 0fbbfcae..13895bae 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -928,3 +928,14 @@ Then( }) }, ) + +// --------------------------------------------------------------------------- +// "Use different email" UX +// --------------------------------------------------------------------------- + +Then('the email input is empty and focused', async function (this: EpdsWorld) { + const page = getPage(this) + const input = page.locator('#email') + await expect(input).toHaveValue('', { timeout: 5_000 }) + await expect(input).toBeFocused({ timeout: 5_000 }) +}) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 20edade8..46ef4568 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -384,6 +384,23 @@ Feature: Passwordless authentication via email OTP # programmatically clearing the demo's `oauth_state` cookie just # before the OTP submission, which is equivalent to the cookie # having lapsed by wall-clock. + # The "Use different email" button on the OTP step takes the user + # back to the email-entry form so they can sign in with a different + # address. The form must be EMPTY when they get there — leaving the + # prior email pre-filled is exactly the misleading "looks like the + # form remembered me" UX that the button was meant to escape from, + # and forces the user to manually clear the field before they can + # type their actual email. + @email @use-different-email + Scenario: "Use different email" returns the user to a clean email form + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then the login page shows an OTP verification form + When the user clicks "Use different email" + Then the email input is empty and focused + @email @demo-cookie-expiry @bug-report Scenario: Demo client's OAuth cookie has expired by the time of callback — useful error, not generic auth_failed When the demo client starts a new OAuth flow with random handle mode diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index b4ba42b8..6be9a309 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -1065,6 +1065,15 @@ export function renderLoginPage(opts: { if (termsEl) termsEl.style.display = 'block'; clearError(); stopHeartbeat(); + // Reset the email field — the user clicked "Use different + // email" precisely to escape the previous value, so leaving + // it pre-filled both wastes a clearing keystroke and looks + // like the form remembered them when they wanted a fresh + // start. Focus the input so they can start typing + // immediately. + emailInput.value = ''; + currentEmail = ''; + emailInput.focus(); } // Send OTP via better-auth From 073da544aec4b1950e96baef9dc23e4ccbf1a2c1 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:18:44 +0000 Subject: [PATCH 04/37] fix(auth-service): don't flash "Invalid OTP" on an empty Verify click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking Verify before typing the code (or pressing Enter on an empty form) flashed "Invalid OTP" — misleading (the user typed nothing, not an invalid code) AND burned a real /sign-in/email-otp call against better-auth's rate limiter, so a confused user tab-clicking Verify could lock themselves out faster than necessary. Fix: short-circuit the submit handler when otp.length is shorter than the number of OTP boxes; focus the first empty box instead. Test: new @verify-empty-otp scenario. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../dont-flash-invalid-otp-on-empty-submit.md | 9 +++++++ e2e/step-definitions/auth.steps.ts | 24 +++++++++++++++++++ features/passwordless-authentication.feature | 19 +++++++++++++++ .../auth-service/src/routes/login-page.ts | 12 +++++++++- 4 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 .changeset/dont-flash-invalid-otp-on-empty-submit.md diff --git a/.changeset/dont-flash-invalid-otp-on-empty-submit.md b/.changeset/dont-flash-invalid-otp-on-empty-submit.md new file mode 100644 index 00000000..42344e05 --- /dev/null +++ b/.changeset/dont-flash-invalid-otp-on-empty-submit.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Empty Verify clicks no longer flash "Invalid OTP". + +**Affects:** End users + +**End users:** clicking **Verify** before typing the code (or pressing Enter on an empty form) used to flash a red "Invalid OTP" error, which was both misleading — you didn't type an invalid code, you typed nothing — and counted against the per-account rate limit. The form now just moves the cursor into the first empty digit box and waits for you to type, without bothering anyone with a fake error. diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 13895bae..4d2a1ee8 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -939,3 +939,27 @@ Then('the email input is empty and focused', async function (this: EpdsWorld) { await expect(input).toHaveValue('', { timeout: 5_000 }) await expect(input).toBeFocused({ timeout: 5_000 }) }) + +// --------------------------------------------------------------------------- +// Empty-OTP submit guard +// --------------------------------------------------------------------------- + +When( + 'the user clicks the Verify button without entering a code', + async function (this: EpdsWorld) { + const page = getPage(this) + await page.click('#form-verify-otp button[type=submit]') + }, +) + +Then('no "Invalid OTP" error is shown', async function (this: EpdsWorld) { + const page = getPage(this) + // Give any async submit a moment to land before asserting absence. + await page.waitForTimeout(1_000) + const errorText = await page.locator('#error-msg').textContent() + if (errorText && /invalid otp/i.test(errorText)) { + throw new Error( + `Expected no "Invalid OTP" error after empty submit but saw: "${errorText}"`, + ) + } +}) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 46ef4568..48742044 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -384,6 +384,25 @@ Feature: Passwordless authentication via email OTP # programmatically clearing the demo's `oauth_state` cookie just # before the OTP submission, which is equivalent to the cookie # having lapsed by wall-clock. + # Clicking Verify with no code typed used to flash "Invalid OTP", + # which is dishonest: the user didn't type an invalid code, they + # typed nothing. It also burned a real call to better-auth's + # /sign-in/email-otp endpoint, which counts against the + # rate-limiter — so a confused user who tab-clicked Verify could + # lock themselves out faster than necessary. The Verify button + # must not allow a submit until the user has typed the full code, + # and an empty submit (e.g. via Enter on a still-empty form) must + # surface a useful prompt rather than a misleading error. + @email @verify-empty-otp + Scenario: Submitting Verify with an empty code does not produce a misleading "Invalid OTP" + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then the login page shows an OTP verification form + When the user clicks the Verify button without entering a code + Then no "Invalid OTP" error is shown + # The "Use different email" button on the OTP step takes the user # back to the email-entry form so they can sign in with a different # address. The form must be EMPTY when they get there — leaving the diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 6be9a309..50ea2ef3 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -1157,12 +1157,22 @@ export function renderLoginPage(opts: { // first call consumes the code; a second one races the redirect and // flashes "Invalid OTP" before the page unloads. if (verifying) return; + var otp = document.getElementById('code').value.trim(); + // Don't bother better-auth with an empty / partial submit — + // it would flash a misleading "Invalid OTP" (the user typed + // nothing, not an invalid code) and burn a rate-limit slot. + // Just focus the first empty box and bail. + if (otp.length < otpBoxes.length) { + for (var i = 0; i < otpBoxes.length; i++) { + if (!otpBoxes[i].value) { otpBoxes[i].focus(); break; } + } + return; + } verifying = true; // Stop pinging the moment a verify is in flight — the redirect // is imminent and any further heartbeat is wasted. stopHeartbeat(); clearError(); - var otp = document.getElementById('code').value.trim(); var btn = this.querySelector('button[type=submit]'); btn.disabled = true; btn.textContent = 'Verifying...'; From 4e23ee1fc78f31e530a9bf4161d4360361b23bf0 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:30:50 +0000 Subject: [PATCH 05/37] fix(auth-service): inline Send-a-new-code on "Too many attempts" lockout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After 5 wrong codes, better-auth's email-otp plugin throws TOO_MANY_ATTEMPTS and deletes the verification row in the same call. Subsequent attempts hit the deleted row and return INVALID_OTP ("Invalid OTP") — so the user, who has just been locked out, sees a generic typo-shaped error and naturally tries again. Each retry just confirms the same dead-end. The only forward path is a fresh Resend. Make the page surface that path inline next to the lockout error, the same way the OTP-expired branch already does. Implementation: widen the inline-action regex from /expir|too long/ to /expir|too long|too many|attempt/ so it matches better-auth's TOO_MANY_ATTEMPTS message ("Too many attempts"). Rename the var isExpired → isUnrecoverable to reflect the broader contract. Test: new @too-many-attempts scenario burns 5 attempts via direct fetch then triggers the 6th through the form, asserts both the "Too many attempts" error AND the Send-a-new-code inline action. Existing unit tests updated to reference the new var name + regex. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../inline-resend-on-too-many-attempts.md | 9 ++++ e2e/step-definitions/auth.steps.ts | 54 +++++++++++++++++++ features/passwordless-authentication.feature | 23 ++++++++ .../src/__tests__/login-page.test.ts | 31 +++++++---- .../auth-service/src/routes/login-page.ts | 21 +++++--- 5 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 .changeset/inline-resend-on-too-many-attempts.md diff --git a/.changeset/inline-resend-on-too-many-attempts.md b/.changeset/inline-resend-on-too-many-attempts.md new file mode 100644 index 00000000..99857222 --- /dev/null +++ b/.changeset/inline-resend-on-too-many-attempts.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +After too many wrong codes, the sign-in page now offers a one-click "Send a new code" instead of looking like more typing might help. + +**Affects:** End users + +**End users:** if you typed the wrong code enough times to get a "Too many attempts" lockout, the page used to leave you fighting an error that more typing couldn't fix — the underlying code had been thrown away by then, so any further attempts came back as "Invalid OTP" against nothing. The page now surfaces a **Send a new code** action right next to the error so you can get a fresh code in one click instead of hunting for the resend button or wondering if you have a typo. diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 4d2a1ee8..830fe858 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -963,3 +963,57 @@ Then('no "Invalid OTP" error is shown', async function (this: EpdsWorld) { ) } }) + +// --------------------------------------------------------------------------- +// "Too many attempts" lockout UX +// --------------------------------------------------------------------------- + +When( + 'the user submits enough wrong OTPs to trigger the lockout', + async function (this: EpdsWorld) { + const page = getPage(this) + const boxCount = await page.locator('.otp-box').count() + if (!this.testEmail) { + throw new Error( + 'No testEmail in world; the email-submit step must run first', + ) + } + // better-auth's allowedAttempts is 5 (see auth-service better-auth.ts). + // After 5 wrong submits the next call sees attempts >= 5 and throws + // TOO_MANY_ATTEMPTS while deleting the row. Burn 5 attempts via direct + // fetch (the form path is racy from the driver's side because of + // auto-submit + clearOtpBoxes interleaving), then trigger the 6th + // through the form so the submit handler's error rendering — the UX + // we want to assert on — actually fires. + const burnDigits = ['0', '1', '2', '3', '4'] + for (const digit of burnDigits) { + await page.evaluate( + `fetch('/api/auth/sign-in/email-otp', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email: ${JSON.stringify(this.testEmail)}, otp: ${JSON.stringify(digit.repeat(boxCount))} }), + }).catch(function () {})`, + ) + } + // Final attempt through the form. + await page.evaluate(`(function () { + var boxes = document.querySelectorAll('.otp-box'); + for (var i = 0; i < boxes.length; i++) boxes[i].value = ''; + })()`) + await page.locator('.otp-box').first().focus() + await page.keyboard.type('5'.repeat(boxCount)) + await expect(page.locator('#error-msg')).toBeVisible({ timeout: 10_000 }) + }, +) + +Then( + 'a "Send a new code" inline action is offered', + async function (this: EpdsWorld) { + const page = getPage(this) + await expect( + page.locator('#error-msg button.flash-action', { + hasText: /send a new code/i, + }), + ).toBeVisible({ timeout: 5_000 }) + }, +) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 48742044..c064647d 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -384,6 +384,29 @@ Feature: Passwordless authentication via email OTP # programmatically clearing the demo's `oauth_state` cookie just # before the OTP submission, which is equivalent to the cookie # having lapsed by wall-clock. + # better-auth's email-otp plugin allows up to 3 wrong attempts on a + # single code. The 4th wrong submit returns "Too many attempts" AND + # deletes the verification row — so any further attempts on that + # same code path are doomed to fail (better-auth then returns + # "Invalid OTP" because the row no longer exists, which would mislead + # the user into thinking they have a typo). + # + # Once the user is in the too-many-attempts state, the only forward + # path is to request a fresh code. The form must surface that + # forward path inline alongside the error, the same way it does for + # OTP expiry — anything else lets the user fight an error that + # cannot be resolved by typing more carefully. + @email @too-many-attempts + Scenario: "Too many attempts" surfaces a Send-a-new-code action + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then the login page shows an OTP verification form + When the user submits enough wrong OTPs to trigger the lockout + Then the verification form shows an "Too many attempts" error + And a "Send a new code" inline action is offered + # Clicking Verify with no code typed used to flash "Invalid OTP", # which is dishonest: the user didn't type an invalid code, they # typed nothing. It also burned a real call to better-auth's diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 5376f713..1a927154 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -602,13 +602,20 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { expect(fnBody).not.toContain('innerHTML') }) - it('detects OTP-expired errors via a substring-stable regex', () => { + it('detects unrecoverable verify errors via a substring-stable regex', () => { const html = renderDefault() - // The detection must catch: - // - better-auth's "Invalid or expired code" - // - auth-service's "OTP expired" - // - any future wording with "expir" or "too long" in it - expect(html).toMatch(/var isExpired = \/expir\|too long\/i\.test/) + // The detection must catch every error string that means the + // current code path is dead and the user must Resend (or Start + // over): + // - better-auth's "Invalid or expired code" / "OTP expired" + // ("expir") + // - auth-service's age-out wording ("too long") + // - better-auth's "Too many attempts" lockout + // ("too many" / "attempt") — see ERROR_CODES in + // better-auth/dist/plugins/email-otp/routes.mjs + expect(html).toMatch( + /var isUnrecoverable = \/expir\|too long\|too many\|attempt\/i\.test/, + ) }) it('renders the inline action with the "Send a new code" label and triggers the Resend button', () => { @@ -618,13 +625,15 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { expect(html).toContain("document.getElementById('btn-resend').click()") }) - it('falls back to the plain showError on non-expired errors', () => { + it('falls back to the plain showError on recoverable (typo) errors', () => { const html = renderDefault() - // The non-expired branch must NOT route through - // showErrorWithAction (otherwise an "Invalid code" message - // would carry an inappropriate "Send a new code" link). + // The recoverable-error branch must NOT route through + // showErrorWithAction (otherwise a typo "Invalid OTP" message + // would carry an inappropriate "Send a new code" link). The + // structure to verify: if (isUnrecoverable) { ... } else { + // showError(result.error); }. expect(html).toMatch( - /if \(isExpired\) \{[\s\S]*?\} else \{[\s\S]*?showError\(result\.error\);\s*\}/, + /if \(isUnrecoverable\) \{[\s\S]*?\} else \{[\s\S]*?showError\(result\.error\);\s*\}/, ) }) }) diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 50ea2ef3..39a40fbc 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -1188,14 +1188,19 @@ export function renderLoginPage(opts: { try { var result = await verifyOtp(currentEmail, otp); if (result && result.error) { - // Inline a "Send a new code" action when the error - // indicates the OTP has aged out — too easy to miss the - // separate Resend button below the form. The substring - // match catches the better-auth wording ("Invalid or - // expired code") and the auth-service wording ("OTP - // expired") plus generic "expir"/"too long" variants. - var isExpired = /expir|too long/i.test(result.error); - if (isExpired) { + // Inline a "Send a new code" action when the current + // code path is dead — too easy to miss the separate + // Resend button below the form, and fighting an + // unrecoverable error is exactly the misleading, + // time-wasting UX we want to avoid. Matches: + // - "Invalid or expired code" / "OTP expired" + // (the original aged-out case) + // - "Too many attempts" (better-auth deletes the + // verification row after N wrong tries — the next + // code typed cannot succeed; the only forward + // path is a fresh Resend) + var isUnrecoverable = /expir|too long|too many|attempt/i.test(result.error); + if (isUnrecoverable) { // Only offer "Send a new code" when the PAR is still // alive. If it isn't, a fresh OTP would issue but // never complete — wasting the user's time on a code From f6a243c4f76b895ba49daf7fa9850541a0b2b626 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:36:11 +0000 Subject: [PATCH 06/37] fix(demo): surface PDS timeout error_description as session_expired MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PDS clean-exit redirect carries a useful error_description ("Your sign-in took too long..."), but the demo's callback was mapping every PDS-side error to the same generic auth_failed banner — discarding the reason and leaving the user unsure whether they typed the wrong code, are blocked, or hit a bug. Detect timeout-shaped descriptions ("timed out" / "too long" / "expired" / "session" inside error_description on access_denied) and route those to the session_expired error code. The user sees an honest "Your sign-in took too long" banner with a clear next step instead of "Authentication failed. Please try again." Test: new @passes-through-timeout-context scenario confirms a dead-PAR redirect lands on /?error=session_expired (existing @otp-and-par-expiry scenarios use the broader "auth error" matcher, which still passes — the matcher accepts either code since both are demo-side error banners). Co-Authored-By: Claude Opus 4.7 (1M context) --- e2e/step-definitions/auth.steps.ts | 15 ++++++++++--- features/passwordless-authentication.feature | 21 +++++++++++++++++++ .../demo/src/app/api/oauth/callback/route.ts | 20 ++++++++++++++++-- 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 830fe858..fea80c18 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -802,9 +802,18 @@ Then( async function (this: EpdsWorld) { const origin = new URL(testEnv.demoUrl).origin const page = getPage(this) - await page.waitForURL(`${origin}/?error=auth_failed*`, { - timeout: 30_000, - }) + // Both `session_expired` (timeout-shaped error_description from + // the PDS clean-exit path) and `auth_failed` (everything else) + // satisfy "an auth error" — they're both demo error codes that + // surface a banner on the landing page. Older scenarios were + // written before the demo distinguished the two; rather than + // re-tagging every one of them, accept either. + await page.waitForURL( + (url) => + url.origin === origin && + /^[?&](error=(auth_failed|session_expired))/.test(url.search), + { timeout: 30_000 }, + ) }, ) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index c064647d..e3f78944 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -365,6 +365,27 @@ Feature: Passwordless authentication via email OTP Then the Resend code button is no longer offered And a Start over button is offered instead + # The PDS clean-exit redirect uses error=access_denied with an + # error_description shaped like "Your sign-in took too long...". + # The demo's landing page used to map every PDS-side error to the + # same generic "auth_failed" banner, which discarded that context + # — telling the user the sign-in failed without saying *why*. + # Translating timeout-shaped descriptions to a `session_expired` + # banner gives the user the actual reason and a sensible next + # step ("Please sign in again") instead of leaving them unsure + # whether they typed the wrong code, are blocked, or hit a bug. + @email @otp-and-par-expiry @passes-through-timeout-context + Scenario: PDS timeout error_description surfaces as a session-expired banner + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then an OTP email arrives in the mail trap for the test email + And the login page shows an OTP verification form + When the PAR request_uri has expired before the bridge fires + And the user enters the OTP code + Then the demo client surfaces a session-expired error + # The demo OAuth client stores its OAuth state (state value, code # verifier, token endpoint, issuer) in a signed cookie that has its # own lifetime. If that cookie expires before the auth-service diff --git a/packages/demo/src/app/api/oauth/callback/route.ts b/packages/demo/src/app/api/oauth/callback/route.ts index 194a963e..9a8918b0 100644 --- a/packages/demo/src/app/api/oauth/callback/route.ts +++ b/packages/demo/src/app/api/oauth/callback/route.ts @@ -40,8 +40,24 @@ export async function GET(request: NextRequest) { const error = request.nextUrl.searchParams.get('error') if (error) { - console.error('[oauth/callback] Auth error from PDS') - return NextResponse.redirect(new URL('/?error=auth_failed', baseUrl)) + // The PDS clean-exit paths (e.g. dead PAR after a long wait) + // send error=access_denied with an error_description like + // "Your sign-in took too long...". A generic auth_failed + // banner discards that context and tells the user nothing + // about *why* it failed. Surface session-expiry timeouts + // distinctly so the landing page can guide them ("sign in + // again") instead of looking like the credentials were + // wrong. + const errorDescription = + request.nextUrl.searchParams.get('error_description') ?? '' + const isTimeout = + error === 'access_denied' && + /timed out|too long|expired|session/i.test(errorDescription) + const code = isTimeout ? 'session_expired' : 'auth_failed' + console.error( + `[oauth/callback] Auth error from PDS: ${error} (${errorDescription})`, + ) + return NextResponse.redirect(new URL(`/?error=${code}`, baseUrl)) } if (!code || !state) { From 4a899b61ea1adf0e721e76a9083fa7f07ce80d36 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:42:47 +0000 Subject: [PATCH 07/37] fix(auth-service): friendlier stale-recovery-link error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Visiting /auth/recover without an active sign-in flow (stale link, direct paste) used to surface "Missing request_uri parameter" — leaks the name of an internal OAuth field and tells the user nothing actionable. Replace with: "Account recovery has to be started from the sign-in page. Please sign in again from the app you came from." plus a "No active sign-in" title that matches the actual cause rather than a generic "Error". Test: new @stale-recovery-link scenario asserts the friendly copy AND that the technical "request_uri" string never appears on the page body. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../friendlier-stale-recovery-link-error.md | 9 +++++ .../account-recovery.steps.ts | 36 +++++++++++++++++++ features/account-recovery.feature | 12 +++++++ packages/auth-service/src/routes/recovery.ts | 11 +++++- 4 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 .changeset/friendlier-stale-recovery-link-error.md diff --git a/.changeset/friendlier-stale-recovery-link-error.md b/.changeset/friendlier-stale-recovery-link-error.md new file mode 100644 index 00000000..e5b6eaad --- /dev/null +++ b/.changeset/friendlier-stale-recovery-link-error.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Visiting a stale account-recovery link no longer leaks an internal OAuth field name. + +**Affects:** End users + +**End users:** if you arrived at the account-recovery page from a stale link or a direct paste — i.e. without an active sign-in flow — the page used to say "Missing request_uri parameter", which tells you nothing useful and leaks the name of an internal OAuth field. The page now says "Account recovery has to be started from the sign-in page. Please sign in again from the app you came from." so you know what to do next. diff --git a/e2e/step-definitions/account-recovery.steps.ts b/e2e/step-definitions/account-recovery.steps.ts index 870d4224..2764cd10 100644 --- a/e2e/step-definitions/account-recovery.steps.ts +++ b/e2e/step-definitions/account-recovery.steps.ts @@ -329,3 +329,39 @@ Then( await assertNoEmailFor(this.backupEmail) }, ) + +// --------------------------------------------------------------------------- +// Stale-recovery-link UX +// --------------------------------------------------------------------------- + +When( + 'the user navigates directly to the recovery page without an active sign-in', + async function (this: EpdsWorld) { + const page = getPage(this) + await page.goto(`${testEnv.authUrl}/auth/recover`) + }, +) + +Then( + 'the page explains that recovery has to start from the sign-in page', + async function (this: EpdsWorld) { + const page = getPage(this) + await expect(page.locator('body')).toContainText( + /recovery has to be started from the sign-in page/i, + { timeout: 10_000 }, + ) + }, +) + +Then( + 'the page does not mention the technical field name {string}', + async function (this: EpdsWorld, fieldName: string) { + const page = getPage(this) + const body = (await page.locator('body').textContent()) ?? '' + if (body.toLowerCase().includes(fieldName.toLowerCase())) { + throw new Error( + `Expected the page to not surface the technical field name "${fieldName}", but its body text contained it. Page body: ${body}`, + ) + } + }, +) diff --git a/features/account-recovery.feature b/features/account-recovery.feature index 4ded73d1..15db69f4 100644 --- a/features/account-recovery.feature +++ b/features/account-recovery.feature @@ -39,6 +39,18 @@ Feature: Account recovery via backup emails Then the recovery OTP form is displayed And no email arrives for that non-existent address + # /auth/recover requires an active sign-in flow (it carries a + # request_uri that points at the upstream PAR). Hitting the URL + # directly — typically by following a stale link or pasting from + # somewhere — used to surface "Missing request_uri parameter", + # which leaks the internal OAuth field name and tells the user + # nothing actionable. + @stale-recovery-link + Scenario: Direct visit to recovery URL surfaces a friendly explanation, not a technical field name + When the user navigates directly to the recovery page without an active sign-in + Then the page explains that recovery has to start from the sign-in page + And the page does not mention the technical field name "request_uri" + # --- Backup email management --- Scenario: User removes a backup email diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index ac6ae5df..523a499c 100644 --- a/packages/auth-service/src/routes/recovery.ts +++ b/packages/auth-service/src/routes/recovery.ts @@ -76,10 +76,19 @@ export function createRecoveryRouter( const requestUri = req.query.request_uri as string | undefined if (!requestUri) { + // The user landed here without an active sign-in flow — + // typically a stale link or direct visit. The technical + // "Missing request_uri parameter" tells them nothing + // actionable; surface the honest, useful message instead. res .status(400) .type('html') - .send(renderError('Missing request_uri parameter')) + .send( + renderError( + 'Account recovery has to be started from the sign-in page. Please sign in again from the app you came from.', + { title: 'No active sign-in' }, + ), + ) return } From f01f70337183f9bbcd69dec74eca5784475fed56 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:51:46 +0000 Subject: [PATCH 08/37] fix(auth-service): also inline Resend after the post-lockout "Invalid OTP" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After 5 wrong codes, better-auth surfaces "Too many attempts" AND deletes the verification row. The previous fix surfaced an inline "Send a new code" on that response — but every SUBSEQUENT submit hit the row-not-found path and got back the generic "Invalid OTP", which without further handling let the user fight a typo error that more typing can't possibly fix. Latch the lockout state in the page (verifyLockedOut). Once set, every subsequent verify-error inherits the same inline-action treatment, so the post-lockout "Invalid OTP" carries a Send-a-new- code action just like the original "Too many attempts" did. A successful Resend resets the latch (fresh row → fresh attempts budget → can succeed again). Test: new @too-many-attempts @post-lockout scenario submits one more wrong OTP after the lockout response and asserts the inline action is still offered. Existing unit test updated to assert the verifyLockedOut latch is referenced in the rendered page JS. Co-Authored-By: Claude Opus 4.7 (1M context) --- e2e/step-definitions/auth.steps.ts | 15 +++++++++++ features/passwordless-authentication.feature | 19 ++++++++++++++ .../src/__tests__/login-page.test.ts | 10 ++++--- .../auth-service/src/routes/login-page.ts | 26 ++++++++++++++++++- 4 files changed, 66 insertions(+), 4 deletions(-) diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index fea80c18..0d08c8b2 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -1026,3 +1026,18 @@ Then( ).toBeVisible({ timeout: 5_000 }) }, ) + +When( + 'the user submits one more wrong OTP after the lockout', + async function (this: EpdsWorld) { + const page = getPage(this) + const boxCount = await page.locator('.otp-box').count() + await page.evaluate(`(function () { + var boxes = document.querySelectorAll('.otp-box'); + for (var i = 0; i < boxes.length; i++) boxes[i].value = ''; + })()`) + await page.locator('.otp-box').first().focus() + await page.keyboard.type('9'.repeat(boxCount)) + await expect(page.locator('#error-msg')).toBeVisible({ timeout: 10_000 }) + }, +) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index e3f78944..040fc728 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -428,6 +428,25 @@ Feature: Passwordless authentication via email OTP Then the verification form shows an "Too many attempts" error And a "Send a new code" inline action is offered + # After the first lockout response, better-auth has deleted the + # verification row, so any further /sign-in/email-otp call falls + # through the row-not-found path and returns "Invalid OTP" — even + # though the row is gone, not invalid. The page latches the + # lockout state so post-lockout submits inherit the same inline + # action treatment as the original "Too many attempts" response, + # rather than letting the user fight an "Invalid OTP" error that + # more typing can't possibly fix. + @email @too-many-attempts @post-lockout + Scenario: After the lockout, further submits still surface a Send-a-new-code action + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then the login page shows an OTP verification form + When the user submits enough wrong OTPs to trigger the lockout + And the user submits one more wrong OTP after the lockout + Then a "Send a new code" inline action is offered + # Clicking Verify with no code typed used to flash "Invalid OTP", # which is dishonest: the user didn't type an invalid code, they # typed nothing. It also burned a real call to better-auth's diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 1a927154..2ea1adf9 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -613,9 +613,13 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { // - better-auth's "Too many attempts" lockout // ("too many" / "attempt") — see ERROR_CODES in // better-auth/dist/plugins/email-otp/routes.mjs - expect(html).toMatch( - /var isUnrecoverable = \/expir\|too long\|too many\|attempt\/i\.test/, - ) + // The page also OR-combines the regex result with a + // verifyLockedOut latch so post-lockout INVALID_OTP errors + // (better-auth has deleted the row by then, so further calls + // fall through to the row-not-found path) inherit the same + // inline-action treatment. + expect(html).toMatch(/\/expir\|too long\|too many\|attempt\/i\.test/) + expect(html).toContain('verifyLockedOut') }) it('renders the inline action with the "Send a new code" label and triggers the Resend button', () => { diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 39a40fbc..e97f502c 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -759,6 +759,17 @@ export function renderLoginPage(opts: { // still bails to /auth/abort instead of issuing a fresh OTP // that would only fail. var flowAborted = false; + // Set to true the moment better-auth surfaces a lockout + // ("Too many attempts"). The verification row is deleted at + // that point, so every subsequent /sign-in/email-otp call + // returns INVALID_OTP ("Invalid OTP") against nothing — and + // a user who keeps typing would just rack up dishonest + // "Invalid OTP" errors when in reality the only forward + // path is a fresh Resend. Latching this lets the + // post-lockout INVALID_OTP path inherit the same inline + // "Send a new code" treatment as the original "Too many + // attempts" response. + var verifyLockedOut = false; // True iff we have proof the PAR is still alive (last ping // was ok:true and was recent enough to fall inside the // upstream inactivity window). Used to gate every "offer the @@ -1199,7 +1210,16 @@ export function renderLoginPage(opts: { // verification row after N wrong tries — the next // code typed cannot succeed; the only forward // path is a fresh Resend) - var isUnrecoverable = /expir|too long|too many|attempt/i.test(result.error); + // Plus: once we have seen a lockout once, every + // subsequent /sign-in/email-otp call hits a deleted + // verification row and returns the generic + // "Invalid OTP" — which without this latch would let + // the user fight a typo error that more typing can't + // fix. + if (/too many|attempt/i.test(result.error)) verifyLockedOut = true; + var isUnrecoverable = + /expir|too long|too many|attempt/i.test(result.error) || + verifyLockedOut; if (isUnrecoverable) { // Only offer "Send a new code" when the PAR is still // alive. If it isn't, a fresh OTP would issue but @@ -1255,6 +1275,10 @@ export function renderLoginPage(opts: { if (result.error) { showError(result.error); } else { + // A fresh code resets the verify-lockout latch — the + // verification row is brand new, so future submits can + // succeed and shouldn't inherit the old lockout state. + verifyLockedOut = false; showSuccess('Code resent!'); } }); From 7a177e5cd2df3729460cc2a89b026d39a3f8530f Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 04:55:25 +0000 Subject: [PATCH 09/37] fix(auth-service): friendlier stale-authorize-link error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same shape as the /auth/recover stale-link fix in the previous commit. Visiting /oauth/authorize without request_uri (stale link, direct paste) showed the technical "Missing request_uri parameter" — leaks an internal field name and tells the user nothing actionable. Replace with: "Sign-in has to be started from the app you are signing into. Please return to that app and try again." plus a "No active sign-in" title that matches the actual cause. Test: new @stale-authorize-link scenario asserts the friendly copy AND that the technical "request_uri" string never appears on the page body. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../friendlier-stale-authorize-link-error.md | 9 ++++++++ e2e/step-definitions/auth.steps.ts | 23 +++++++++++++++++++ features/passwordless-authentication.feature | 11 +++++++++ .../auth-service/src/routes/login-page.ts | 11 ++++++++- 4 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 .changeset/friendlier-stale-authorize-link-error.md diff --git a/.changeset/friendlier-stale-authorize-link-error.md b/.changeset/friendlier-stale-authorize-link-error.md new file mode 100644 index 00000000..940d9f17 --- /dev/null +++ b/.changeset/friendlier-stale-authorize-link-error.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Visiting a stale sign-in link no longer leaks an internal OAuth field name. + +**Affects:** End users + +**End users:** if you arrived at the sign-in page from a stale link or a direct paste — i.e. without an active sign-in flow — the page used to say "Missing request_uri parameter", which tells you nothing useful and leaks the name of an internal OAuth field. The page now says "Sign-in has to be started from the app you are signing into. Please return to that app and try again." so you know what to do next. diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 0d08c8b2..f231904b 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -1027,6 +1027,29 @@ Then( }, ) +// --------------------------------------------------------------------------- +// Stale-authorize-link UX +// --------------------------------------------------------------------------- + +When( + 'the user navigates directly to the authorize page without an active sign-in', + async function (this: EpdsWorld) { + const page = getPage(this) + await page.goto(`${testEnv.authUrl}/oauth/authorize`) + }, +) + +Then( + 'the page explains that sign-in has to start from the app', + async function (this: EpdsWorld) { + const page = getPage(this) + await expect(page.locator('body')).toContainText( + /sign-in has to be started from the app/i, + { timeout: 10_000 }, + ) + }, +) + When( 'the user submits one more wrong OTP after the lockout', async function (this: EpdsWorld) { diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 040fc728..83784634 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -447,6 +447,17 @@ Feature: Passwordless authentication via email OTP And the user submits one more wrong OTP after the lockout Then a "Send a new code" inline action is offered + # /oauth/authorize requires an active sign-in flow (it carries a + # request_uri that points at the upstream PAR). Direct visits to + # the URL — typically a stale link or a paste — used to surface + # "Missing request_uri parameter", which leaks an internal OAuth + # field name and tells the user nothing actionable. + @stale-authorize-link + Scenario: Direct visit to /oauth/authorize surfaces a friendly explanation + When the user navigates directly to the authorize page without an active sign-in + Then the page explains that sign-in has to start from the app + And the page does not mention the technical field name "request_uri" + # Clicking Verify with no code typed used to flash "Invalid OTP", # which is dishonest: the user didn't type an invalid code, they # typed nothing. It also burned a real call to better-auth's diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index e97f502c..05b6a7a9 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -163,10 +163,19 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { const clientId = req.query.client_id as string | undefined const loginHint = req.query.login_hint as string | undefined if (!requestUri) { + // The user landed here without an active sign-in flow — + // typically a stale link or direct visit. The technical + // "Missing request_uri parameter" tells them nothing + // actionable; surface the honest, useful message instead. res .status(400) .type('html') - .send(renderError('Missing request_uri parameter')) + .send( + renderError( + 'Sign-in has to be started from the app you are signing into. Please return to that app and try again.', + { title: 'No active sign-in' }, + ), + ) return } From 7dbd5d71dd7d097055998cc318082ed348e09ec9 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:04:21 +0000 Subject: [PATCH 10/37] fix(auth-service): recovery link carries the real request_uri, not a placeholder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Recover with backup email" link on the OTP step was hard-coded to `/auth/recover?request_uri=/placeholder`. The recovery page used that placeholder both for its own logic AND for the "Back to sign in" link's href — which therefore pointed at `/oauth/authorize?request_uri=` and landed users on upstream's "data you submitted is invalid" error page when they decided not to recover after all. Thread the active OAuth flow's actual request_uri through renderLoginPage's opts so the link round-trips cleanly. Three existing call sites (preview/login, preview/login-otp, the heartbeat-toggle unit test fixture) needed corresponding updates. Tests: - New e2e scenario @recovery-link-roundtrip asserts the rendered link's request_uri matches the active OAuth flow's. - New unit test asserts the rendered HTML uses the supplied requestUri AND no longer contains the literal "/placeholder" string, so a regression to the hard-coded suffix surfaces immediately. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../recovery-link-uses-real-request-uri.md | 9 +++++ .../account-recovery.steps.ts | 33 +++++++++++++++++++ features/account-recovery.feature | 15 +++++++++ .../src/__tests__/heartbeat-toggle.test.ts | 1 + .../src/__tests__/login-page.test.ts | 18 ++++++++++ .../auth-service/src/routes/login-page.ts | 11 ++++++- packages/auth-service/src/routes/preview.ts | 7 ++++ 7 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 .changeset/recovery-link-uses-real-request-uri.md diff --git a/.changeset/recovery-link-uses-real-request-uri.md b/.changeset/recovery-link-uses-real-request-uri.md new file mode 100644 index 00000000..6a0b9856 --- /dev/null +++ b/.changeset/recovery-link-uses-real-request-uri.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +The "Recover with backup email" link no longer breaks the "Back to sign in" path. + +**Affects:** End users + +**End users:** clicking **Recover with backup email** on the sign-in page used to leave you on a recovery flow that couldn't return you to the original sign-in: hitting **Back to sign in** at the end landed on a "data you submitted is invalid" error page from the underlying OAuth machinery, because the link forwarded a placeholder URL instead of the real sign-in context. The link now carries the active sign-in's actual context, so the back path round-trips cleanly even if you decide not to use recovery after all. diff --git a/e2e/step-definitions/account-recovery.steps.ts b/e2e/step-definitions/account-recovery.steps.ts index 2764cd10..81514d88 100644 --- a/e2e/step-definitions/account-recovery.steps.ts +++ b/e2e/step-definitions/account-recovery.steps.ts @@ -330,6 +330,39 @@ Then( }, ) +// --------------------------------------------------------------------------- +// Recovery-link round-trip +// --------------------------------------------------------------------------- + +Then( + "the recovery link points at the active OAuth flow's request_uri", + async function (this: EpdsWorld) { + const page = getPage(this) + // Pull the request_uri the auth-service was driving for this + // page from the URL we landed on. + const flowUrl = new URL(page.url()) + const expectedRequestUri = flowUrl.searchParams.get('request_uri') + if (!expectedRequestUri) { + throw new Error( + `Expected the current page URL to carry a request_uri so the test can compare it to the recovery link, but URL was: ${page.url()}`, + ) + } + const recoveryHref = await page + .locator('#recovery-link') + .getAttribute('href') + if (!recoveryHref) { + throw new Error('Recovery link not found on page') + } + const recoveryUrl = new URL(recoveryHref, flowUrl.origin) + const linkRequestUri = recoveryUrl.searchParams.get('request_uri') + if (linkRequestUri !== expectedRequestUri) { + throw new Error( + `Recovery link request_uri mismatch:\n expected: ${expectedRequestUri}\n got: ${linkRequestUri}`, + ) + } + }, +) + // --------------------------------------------------------------------------- // Stale-recovery-link UX // --------------------------------------------------------------------------- diff --git a/features/account-recovery.feature b/features/account-recovery.feature index 15db69f4..0f58364d 100644 --- a/features/account-recovery.feature +++ b/features/account-recovery.feature @@ -39,6 +39,21 @@ Feature: Account recovery via backup emails Then the recovery OTP form is displayed And no email arrives for that non-existent address + # The "Recover with backup email" link on the OTP form used to + # carry a hard-coded "/placeholder" URL instead of the active + # OAuth flow's actual request_uri. The user could complete + # recovery, but clicking "Back to sign in" landed them on + # upstream's "data you submitted is invalid" error page (because + # /placeholder isn't a real PAR). This scenario asserts the link + # carries the actual request_uri so the back path round-trips + # cleanly. + @recovery-link-roundtrip + Scenario: Recovery link carries the active OAuth flow's request_uri + Given the demo client initiates OAuth with the test email as login_hint + Then an OTP email arrives in the mail trap + And the login page shows an OTP verification form + Then the recovery link points at the active OAuth flow's request_uri + # /auth/recover requires an active sign-in flow (it carries a # request_uri that points at the upstream PAR). Hitting the URL # directly — typically by following a stale link or pasting from diff --git a/packages/auth-service/src/__tests__/heartbeat-toggle.test.ts b/packages/auth-service/src/__tests__/heartbeat-toggle.test.ts index cb05ed42..98dcc517 100644 --- a/packages/auth-service/src/__tests__/heartbeat-toggle.test.ts +++ b/packages/auth-service/src/__tests__/heartbeat-toggle.test.ts @@ -92,6 +92,7 @@ function renderLoginPageWithHeartbeat(heartbeatEnabled: boolean): string { loginHint: '', initialStep: 'email', otpAlreadySent: false, + requestUri: 'urn:ietf:params:oauth:request_uri:req-abc', csrfToken: 'csrf', authBasePath: '/api/auth', pdsPublicUrl: 'https://pds.example.com', diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 2ea1adf9..1c916964 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -425,6 +425,7 @@ describe('renderLoginPage handle login button', () => { loginHint: '', initialStep: 'email', otpAlreadySent: false, + requestUri: 'urn:ietf:params:oauth:request_uri:test-req', csrfToken: 'csrf', authBasePath: '/api/auth', pdsPublicUrl: 'https://pds.example.com', @@ -512,6 +513,7 @@ function renderDefault(): string { loginHint: '', initialStep: 'email', otpAlreadySent: false, + requestUri: 'urn:ietf:params:oauth:request_uri:test-req', csrfToken: 'csrf', authBasePath: '/api/auth', pdsPublicUrl: 'https://pds.example.com', @@ -640,6 +642,22 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { /if \(isUnrecoverable\) \{[\s\S]*?\} else \{[\s\S]*?showError\(result\.error\);\s*\}/, ) }) + + it('renders the recovery link with the actual request_uri, not a placeholder', () => { + // The "Recover with backup email" link must round-trip the + // active OAuth flow's request_uri, so the recovery page's + // "Back to sign in" can return the user to the original + // /oauth/authorize. Earlier code hard-coded a "/placeholder" + // suffix on pdsPublicUrl, which silently broke the back path. + const html = renderDefault() + expect(html).toContain( + 'href="/auth/recover?request_uri=urn%3Aietf%3Aparams%3Aoauth%3Arequest_uri%3Atest-req"', + ) + // Make sure the placeholder fallback did not survive the + // refactor (catches a regression where the literal "placeholder" + // ends up back in the rendered HTML). + expect(html).not.toContain('/placeholder') + }) }) describe('renderLoginPage flow-aborted notice + reactive abort gates', () => { diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 05b6a7a9..0f577f20 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -458,6 +458,7 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { loginHint: emailHint, initialStep, otpAlreadySent, + requestUri, csrfToken: res.locals.csrfToken, authBasePath: '/api/auth', pdsPublicUrl: ctx.config.pdsPublicUrl, @@ -485,6 +486,14 @@ export function renderLoginPage(opts: { loginHint: string initialStep: 'email' | 'otp' otpAlreadySent: boolean + /** + * The actual PAR request_uri that started this flow. Forwarded to + * the recovery link's query string so the recovery page can + * round-trip the user back to their original /oauth/authorize on + * "Back to sign in". Previously the link used a placeholder URL, + * which silently broke the back-to-sign-in path after recovery. + */ + requestUri: string csrfToken: string authBasePath: string pdsPublicUrl: string @@ -709,7 +718,7 @@ export function renderLoginPage(opts: { - Recover with backup email diff --git a/packages/auth-service/src/routes/preview.ts b/packages/auth-service/src/routes/preview.ts index f9938998..513a2f27 100644 --- a/packages/auth-service/src/routes/preview.ts +++ b/packages/auth-service/src/routes/preview.ts @@ -216,6 +216,11 @@ export function createPreviewRouter(ctx: AuthServiceContext): Router { loginHint: '', initialStep: 'email', otpAlreadySent: false, + // Preview pages don't sit behind a real OAuth flow — pass a + // recognisable fake request_uri so the recovery link renders + // something shaped right without pretending to point at a + // live PAR. + requestUri: 'urn:ietf:params:oauth:request_uri:preview', csrfToken: fakeCsrfToken(), authBasePath: '/api/auth', pdsPublicUrl: ctx.config.pdsPublicUrl, @@ -246,6 +251,8 @@ export function createPreviewRouter(ctx: AuthServiceContext): Router { loginHint: FAKE_EMAIL, initialStep: 'otp', otpAlreadySent: true, + // See preview/login note above — fake request_uri. + requestUri: 'urn:ietf:params:oauth:request_uri:preview', csrfToken: fakeCsrfToken(), authBasePath: '/api/auth', pdsPublicUrl: ctx.config.pdsPublicUrl, From 11ea1cc13f6e02a431fd4825fe9444b9cf5b0051 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:10:52 +0000 Subject: [PATCH 11/37] test(auth-service): unit-test the new login-page UX guards Three new unit assertions covering the page-script changes from the recent commits: - showEmailStep clears emailInput.value AND focuses it ("Use different email" UX fix) - form-verify-otp short-circuits when otp.length < otpBoxes.length (empty/partial Verify guard) - verifyLockedOut is both SET on a "too many"-shaped error AND reset to false on a successful Resend (post-lockout latch + fresh-code reopen) These were already covered end-to-end by the new e2e scenarios, but uniting the assertions at the unit layer keeps coverage from sliding when downstream e2e tests are skipped on a different env (see Coveralls flagging the previous commits with -0.1%). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/__tests__/login-page.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 1c916964..8b9d8bd5 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -643,6 +643,34 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { ) }) + it('clears the email input on "Use different email" so the form does not appear to remember the previous user', () => { + const html = renderDefault() + // showEmailStep() must reset emailInput.value AND focus the + // input so the next keystroke goes to a clean form. Leaving + // the previous email pre-filled was the regression we want to + // catch — assert both effects appear in the inlined script. + expect(html).toMatch(/emailInput\.value\s*=\s*''/) + expect(html).toMatch(/emailInput\.focus\(\)/) + }) + + it('short-circuits Verify when the OTP boxes are not all filled', () => { + const html = renderDefault() + // The submit handler bails early when otp.length is shorter + // than the number of OTP boxes — that's what stops empty + // Verify clicks from flashing a misleading "Invalid OTP" and + // burning a rate-limit slot. Assert the guard is present. + expect(html).toMatch(/if \(otp\.length < otpBoxes\.length\)/) + }) + + it('latches verifyLockedOut on a "too many"-shaped error and resets it on Resend', () => { + const html = renderDefault() + // The post-lockout fix needs both halves: a latch that gets + // SET on the lockout error, and a reset on a successful + // Resend so a fresh code reopens the verification cycle. + expect(html).toMatch(/verifyLockedOut\s*=\s*true/) + expect(html).toMatch(/verifyLockedOut\s*=\s*false/) + }) + it('renders the recovery link with the actual request_uri, not a placeholder', () => { // The "Recover with backup email" link must round-trip the // active OAuth flow's request_uri, so the recovery page's From 5eddf7528be733ecdcc2eb546d2434e3c7c5b334 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:23:10 +0000 Subject: [PATCH 12/37] fix(auth-service): filter pasted OTP content to the configured charset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OTP paste handler stripped only whitespace before distributing pasted characters across the boxes — so pasting "1234-5678" into an 8-box numeric form put `1`, `2`, `3`, `4`, `-`, `5`, `6`, `7` into boxes and auto-submitted, getting back a misleading "Invalid OTP" and burning a rate-limit slot. Filter the paste content to the configured charset (digits-only when numeric, letters+digits uppercased when alphanumeric) before distributing. So a "12-AB-34" copy into a numeric form yields "1234" (filling 4 boxes, waiting for more digits) rather than a garbage auto-submit. Test: unit assertion that the rendered handler builds the charset regex from otpCharset and applies it to the paste. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/filter-paste-to-otp-charset.md | 9 +++++++++ .../src/__tests__/login-page.test.ts | 13 +++++++++++++ packages/auth-service/src/routes/login-page.ts | 17 +++++++++++++++-- 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 .changeset/filter-paste-to-otp-charset.md diff --git a/.changeset/filter-paste-to-otp-charset.md b/.changeset/filter-paste-to-otp-charset.md new file mode 100644 index 00000000..c7b74ddc --- /dev/null +++ b/.changeset/filter-paste-to-otp-charset.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Pasted sign-in codes are no longer auto-submitted with stray punctuation or letters. + +**Affects:** End users + +**End users:** if you copied your sign-in code from somewhere that wrapped it in punctuation or extra characters (e.g. an email reading "Your code is 1234-5678"), the page used to drop the punctuation straight into the digit boxes and submit the result, which the server would then reject as an invalid code. The page now filters pasted text to the format the code is in (digits only, or letters and digits) before distributing it across the boxes — so the paste cleans itself up and signs you in. diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 8b9d8bd5..57c7b5ac 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -671,6 +671,19 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { expect(html).toMatch(/verifyLockedOut\s*=\s*false/) }) + it('filters pasted OTP content to the configured charset before auto-submitting', () => { + const html = renderDefault() + // Numeric OTPs must drop letters / hyphens from a paste rather + // than letting them through and auto-submitting a garbage + // value that better-auth will reject — wastes a rate-limit + // slot AND flashes a misleading "Invalid OTP". The handler + // builds a charsetRegex from the JSON-injected otpCharset + // var; assert both branches are present. + expect(html).toMatch( + /charsetRegex\s*=\s*otpCharset\s*===\s*'alphanumeric'\s*\?\s*\/\[\^A-Za-z0-9\]\/g\s*:\s*\/\[\^0-9\]\/g/, + ) + }) + it('renders the recovery link with the actual request_uri, not a placeholder', () => { // The "Recover with backup email" link must round-trip the // active OAuth flow's request_uri, so the recovery page's diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 0f577f20..3f3994a6 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -982,8 +982,21 @@ export function renderLoginPage(opts: { box.addEventListener('paste', function(e) { e.preventDefault(); var data = (e.clipboardData || window.clipboardData).getData('text') || ''; - var cleaned = data.replace(/\\s/g, '').slice(0, otpBoxes.length - idx); - for (var i = 0; i < cleaned.length; i++) otpBoxes[idx + i].value = cleaned[i]; + // Strip everything that isn't part of the OTP charset. + // Pasting "1234 5678" should fill the boxes; pasting + // "12-AB-34" with a numeric OTP should drop the dashes + // and letters silently rather than auto-submitting a + // garbage code that better-auth will then reject as + // "Invalid OTP" — the user typed nothing wrong, the + // paste source did, so flashing them an error would be + // both misleading and burn a rate-limit slot. + var charsetRegex = otpCharset === 'alphanumeric' ? /[^A-Za-z0-9]/g : /[^0-9]/g; + var cleaned = data.replace(charsetRegex, '').slice(0, otpBoxes.length - idx); + for (var i = 0; i < cleaned.length; i++) { + otpBoxes[idx + i].value = otpCharset === 'alphanumeric' + ? cleaned[i].toUpperCase() + : cleaned[i]; + } updateHiddenCode(); var nextIdx = Math.min(idx + cleaned.length, otpBoxes.length - 1); otpBoxes[nextIdx].focus(); From a3f594a95559057c959660759e9902de4d2435ca Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:25:04 +0000 Subject: [PATCH 13/37] fix(auth-service): filter typed OTP keystrokes to the configured charset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the paste-handler fix in the previous commit. The per-box input handler stripped only whitespace, so a single typed keystroke that didn't match the charset (e.g. "A" in a numeric form) ended up in the box and contributed to an auto-submit on the last digit — same misleading "Invalid OTP" path the paste filter closes off. Apply the same charset regex (digits-only when numeric, letters +digits uppercased when alphanumeric) to the typed value. Also extend the changeset wording to cover both inputs. Test: unit assertion that the rendered handler builds the input charset regex from otpCharset. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/filter-paste-to-otp-charset.md | 2 +- packages/auth-service/src/__tests__/login-page.test.ts | 10 ++++++++++ packages/auth-service/src/routes/login-page.ts | 10 +++++++++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/.changeset/filter-paste-to-otp-charset.md b/.changeset/filter-paste-to-otp-charset.md index c7b74ddc..5bfbe1ec 100644 --- a/.changeset/filter-paste-to-otp-charset.md +++ b/.changeset/filter-paste-to-otp-charset.md @@ -6,4 +6,4 @@ Pasted sign-in codes are no longer auto-submitted with stray punctuation or lett **Affects:** End users -**End users:** if you copied your sign-in code from somewhere that wrapped it in punctuation or extra characters (e.g. an email reading "Your code is 1234-5678"), the page used to drop the punctuation straight into the digit boxes and submit the result, which the server would then reject as an invalid code. The page now filters pasted text to the format the code is in (digits only, or letters and digits) before distributing it across the boxes — so the paste cleans itself up and signs you in. +**End users:** if you copied your sign-in code from somewhere that wrapped it in punctuation or extra characters (e.g. an email reading "Your code is 1234-5678"), the page used to drop the punctuation straight into the digit boxes and submit the result, which the server would then reject as an invalid code. The same was true if you typed a stray letter into a digits-only code by accident. The boxes now filter both pasted and typed input to the format the code is in (digits only, or letters and digits) — so a paste cleans itself up and a stray keystroke is silently ignored. diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 57c7b5ac..15b7ab15 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -684,6 +684,16 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { ) }) + it('filters typed OTP keystrokes to the configured charset', () => { + const html = renderDefault() + // The same charset filter must apply on the per-box input + // handler — typing "A" into a numeric form should be dropped + // silently rather than auto-submitting a bad code at the end. + expect(html).toMatch( + /inputCharsetRegex\s*=\s*otpCharset\s*===\s*'alphanumeric'\s*\?\s*\/\[\^A-Za-z0-9\]\/g\s*:\s*\/\[\^0-9\]\/g/, + ) + }) + it('renders the recovery link with the actual request_uri, not a placeholder', () => { // The "Recover with backup email" link must round-trip the // active OAuth flow's request_uri, so the recovery page's diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 3f3994a6..a0eaa5c6 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -955,11 +955,19 @@ export function renderLoginPage(opts: { hiddenCode.value = ''; } + // Mirror the paste filter so a single typed keystroke gets + // dropped silently when it isn't part of the OTP charset. + // Without this a user typing "1A2B3C" into a numeric form + // ends up with "1A2B3C" in the boxes and triggers an + // auto-submit that returns "Invalid OTP", same misleading + // path the paste filter closes off. + var inputCharsetRegex = otpCharset === 'alphanumeric' ? /[^A-Za-z0-9]/g : /[^0-9]/g; otpBoxes.forEach(function(box, idx) { box.addEventListener('input', function() { // keep only the last typed char (handles paste into a single box) - var v = box.value.replace(/\\s/g, ''); + var v = box.value.replace(/\\s/g, '').replace(inputCharsetRegex, ''); if (v.length > 1) v = v.slice(-1); + if (v && otpCharset === 'alphanumeric') v = v.toUpperCase(); box.value = v; updateHiddenCode(); if (box.value && idx < otpBoxes.length - 1) otpBoxes[idx + 1].focus(); From 4aab907886aeb19b6bf6948d16bbd36babada274 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:28:26 +0000 Subject: [PATCH 14/37] fix(auth-service): account-login surfaces honest lockout message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The standalone /account/login OTP verify catch always returned "Invalid or expired code. Please try again." for any error from better-auth — both typos and the post-lockout deleted-row case ("Too many attempts" then INVALID_OTP-against-nothing). The "please try again" copy implied more typing could fix it; in the lockout case it couldn't, and the user would just rack up more failed submits. Detect /too many|attempt|expir/ in the error text and surface "That code can no longer be used. Click 'Resend code' below to get a fresh one." pointing at the Resend form already on the page. Same shape as the OAuth login-page fix in earlier commits, adapted for the server-rendered standalone form. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../account-login-honest-lockout-message.md | 9 +++++++++ .../auth-service/src/routes/account-login.ts | 16 +++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 .changeset/account-login-honest-lockout-message.md diff --git a/.changeset/account-login-honest-lockout-message.md b/.changeset/account-login-honest-lockout-message.md new file mode 100644 index 00000000..7346670e --- /dev/null +++ b/.changeset/account-login-honest-lockout-message.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Account-settings sign-in shows useful guidance when the code can no longer be used. + +**Affects:** End users + +**End users:** when signing in to manage your account, typing the wrong code enough times locked out the original code — but the page kept saying "Invalid or expired code. Please try again.", which implied more typing might help. The page now distinguishes that case and says "That code can no longer be used. Click 'Resend code' below to get a fresh one." pointing you at the Resend button right below the error. diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index 7b044ab2..c793593b 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -116,9 +116,19 @@ export function createAccountLoginRouter( return } catch (err: unknown) { logger.warn({ err, email }, 'OTP verification failed') - const errMsg = - err instanceof Error && err.message.includes('invalid') - ? 'Invalid or expired code. Please try again.' + const errText = err instanceof Error ? err.message.toLowerCase() : '' + // Distinguish the lockout / aged-out path ("Too many attempts" + // and the followup INVALID_OTP-against-deleted-row) from a + // recoverable typo. Both surface as Error messages from + // better-auth; the lockout / expired path needs a Resend, the + // typo path just needs a re-type. Showing "Invalid or expired" + // for both let the user fight an error that more typing + // couldn't fix. + const isUnrecoverable = /too many|attempt|expir/.test(errText) + const errMsg = isUnrecoverable + ? 'That code can no longer be used. Click "Resend code" below to get a fresh one.' + : errText.includes('invalid') + ? 'Invalid code. Please try again.' : 'Verification failed. Please try again.' res.type('html').send( renderOtpForm({ From 3b82b3189ed9e4bcfa3d1542b483be50b0a39819 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:34:09 +0000 Subject: [PATCH 15/37] test(auth-service): unit-test account-login error-message picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the OTP-verify error-message branching from the /account/verify-otp catch block into an exported pure function (pickAccountLoginVerifyErrorMessage) so its three cases — unrecoverable lockout, generic typo, internal failure — can each be unit-tested without standing up the full express router. 7 new test cases cover the lockout path, expired path, typo path, fallback path, non-Error rejections, and case-insensitive matching against better-auth's English fixed-text errors. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/__tests__/account-login.test.ts | 62 +++++++++++++++++++ .../auth-service/src/routes/account-login.ts | 42 ++++++++----- 2 files changed, 89 insertions(+), 15 deletions(-) create mode 100644 packages/auth-service/src/__tests__/account-login.test.ts diff --git a/packages/auth-service/src/__tests__/account-login.test.ts b/packages/auth-service/src/__tests__/account-login.test.ts new file mode 100644 index 00000000..07ec6f94 --- /dev/null +++ b/packages/auth-service/src/__tests__/account-login.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect } from 'vitest' +import { pickAccountLoginVerifyErrorMessage } from '../routes/account-login.js' + +describe('pickAccountLoginVerifyErrorMessage', () => { + // The standalone /account/login flow surfaces three distinct + // error states through its server-rendered OTP form. The user- + // facing copy must distinguish them, otherwise typing more in + // an unrecoverable state just rolls up failed attempts that + // could never succeed. + + it('points the user at Resend when the row was locked out', () => { + const err = new Error('Too many attempts') + expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + 'That code can no longer be used. Click "Resend code" below to get a fresh one.', + ) + }) + + it('points the user at Resend when the OTP has expired', () => { + const err = new Error('OTP expired') + expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + 'That code can no longer be used. Click "Resend code" below to get a fresh one.', + ) + }) + + it('asks the user to re-type on a typo', () => { + const err = new Error('Invalid OTP') + expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + 'Invalid code. Please try again.', + ) + }) + + it('falls back to a generic verification-failed message on unknown errors', () => { + const err = new Error('Internal database problem') + expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + 'Verification failed. Please try again.', + ) + }) + + it('handles non-Error rejections gracefully', () => { + expect(pickAccountLoginVerifyErrorMessage('string-thrown')).toBe( + 'Verification failed. Please try again.', + ) + expect(pickAccountLoginVerifyErrorMessage(null)).toBe( + 'Verification failed. Please try again.', + ) + expect(pickAccountLoginVerifyErrorMessage(undefined)).toBe( + 'Verification failed. Please try again.', + ) + }) + + it('matches the lockout pattern case-insensitively', () => { + // better-auth's INVALID_OTP / OTP_EXPIRED / TOO_MANY_ATTEMPTS + // strings come through as English fixed text — the function + // lowercases the message before matching, so capitalisation + // shouldn't matter. + expect( + pickAccountLoginVerifyErrorMessage(new Error('TOO MANY ATTEMPTS')), + ).toBe( + 'That code can no longer be used. Click "Resend code" below to get a fresh one.', + ) + }) +}) diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index c793593b..e52f5758 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -23,6 +23,32 @@ import { POWERED_BY_CSS, POWERED_BY_HTML } from '../lib/page-helpers.js' const logger = createLogger('auth:account-login') +/** + * Pick the user-facing OTP-verify error message for the standalone + * account-login form. Exported for unit tests; the route handler + * pipes its caught error through this helper rather than open-coding + * the branching. + * + * Three cases: + * - "too many attempts" / "expired" — better-auth has either + * locked the verification row out or aged it out, so any + * subsequent attempt with the same code is doomed. Point the + * user at Resend. + * - "invalid" — generic typo from better-auth (the shared + * INVALID_OTP message). Re-typing might fix it. + * - everything else — internal failure or unexpected shape. + */ +export function pickAccountLoginVerifyErrorMessage(err: unknown): string { + const errText = err instanceof Error ? err.message.toLowerCase() : '' + if (/too many|attempt|expir/.test(errText)) { + return 'That code can no longer be used. Click "Resend code" below to get a fresh one.' + } + if (errText.includes('invalid')) { + return 'Invalid code. Please try again.' + } + return 'Verification failed. Please try again.' +} + export function createAccountLoginRouter( auth: BetterAuthInstance, ctx: AuthServiceContext, @@ -116,27 +142,13 @@ export function createAccountLoginRouter( return } catch (err: unknown) { logger.warn({ err, email }, 'OTP verification failed') - const errText = err instanceof Error ? err.message.toLowerCase() : '' - // Distinguish the lockout / aged-out path ("Too many attempts" - // and the followup INVALID_OTP-against-deleted-row) from a - // recoverable typo. Both surface as Error messages from - // better-auth; the lockout / expired path needs a Resend, the - // typo path just needs a re-type. Showing "Invalid or expired" - // for both let the user fight an error that more typing - // couldn't fix. - const isUnrecoverable = /too many|attempt|expir/.test(errText) - const errMsg = isUnrecoverable - ? 'That code can no longer be used. Click "Resend code" below to get a fresh one.' - : errText.includes('invalid') - ? 'Invalid code. Please try again.' - : 'Verification failed. Please try again.' res.type('html').send( renderOtpForm({ email, csrfToken: res.locals.csrfToken, otpLength: ctx.config.otpLength, otpCharset: ctx.config.otpCharset, - error: errMsg, + error: pickAccountLoginVerifyErrorMessage(err), }), ) } From 010f0c070154c65cd9ff1cda964d8e32a2aa294f Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:35:02 +0000 Subject: [PATCH 16/37] fix(demo): invalid_grant on token exchange surfaces as session_expired MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token-endpoint failure path was mapping every non-200 response to the generic auth_failed banner. atproto's authorization codes have a short TTL (handful of minutes), so a slow user who stalls between the OAuth callback and the demo's token exchange gets back invalid_grant from the token endpoint — which until now showed "Authentication failed. Please try again." instead of the honest "Your sign-in took too long to finish." Detect invalid_grant on a 400 response from the token endpoint and route those to session_expired alongside the cookie-missing case, so the user sees one consistent honest message regardless of which step of the flow timed out. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/demo/src/app/api/oauth/callback/route.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/demo/src/app/api/oauth/callback/route.ts b/packages/demo/src/app/api/oauth/callback/route.ts index 9a8918b0..15feb7b0 100644 --- a/packages/demo/src/app/api/oauth/callback/route.ts +++ b/packages/demo/src/app/api/oauth/callback/route.ts @@ -186,7 +186,17 @@ export async function GET(request: NextRequest) { console.error( `[oauth/callback] FAILED status=${tokenRes.status} url=${tokenUrl} body=${errBody}`, ) - return NextResponse.redirect(new URL('/?error=auth_failed', baseUrl)) + // invalid_grant from the token endpoint typically means the + // authorization code expired (atproto's auth_request rows + // age out a few minutes after issuance). Surface that as + // session_expired rather than the generic auth_failed banner + // so the user sees an honest "sign-in took too long" rather + // than wondering whether their credentials were wrong. + const code = + tokenRes.status === 400 && /invalid_grant/i.test(errBody) + ? 'session_expired' + : 'auth_failed' + return NextResponse.redirect(new URL(`/?error=${code}`, baseUrl)) } const tokenData = (await tokenRes.json()) as { From f900a9a1deb65dd86eb71c6c6a81424c435bac3d Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 05:46:33 +0000 Subject: [PATCH 17/37] refactor(auth-service): share OTP-verify error-message picker between flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /account/login and /auth/recover flows both call better-auth's signInEmailOTP and need to translate the caught error into honest user-facing copy. Until now the recovery flow open-coded a two-branch matcher ("invalid or expired" vs generic) and the account-login flow had the three-branch picker added in the previous commit — same shape, slightly different wording. Lift the picker into packages/auth-service/src/lib/otp-verify- error.ts as pickOtpVerifyErrorMessage and call it from both routes. The recovery flow now also points the user at "Click 'Resend code' below to get a fresh one." on lockout/aged-out, matching the account-login UX. Test file renamed to reflect the new module location. No new e2e scenario — the recovery flow's user-facing change (better message on the same path) is covered by the existing unit tests on the picker function. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/recovery-honest-lockout-message.md | 9 +++++ ...login.test.ts => otp-verify-error.test.ts} | 22 +++++------ .../auth-service/src/lib/otp-verify-error.ts | 37 +++++++++++++++++++ .../auth-service/src/routes/account-login.ts | 29 +-------------- packages/auth-service/src/routes/recovery.ts | 11 +++--- 5 files changed, 64 insertions(+), 44 deletions(-) create mode 100644 .changeset/recovery-honest-lockout-message.md rename packages/auth-service/src/__tests__/{account-login.test.ts => otp-verify-error.test.ts} (72%) create mode 100644 packages/auth-service/src/lib/otp-verify-error.ts diff --git a/.changeset/recovery-honest-lockout-message.md b/.changeset/recovery-honest-lockout-message.md new file mode 100644 index 00000000..4d123992 --- /dev/null +++ b/.changeset/recovery-honest-lockout-message.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Account-recovery sign-in shows useful guidance when the code can no longer be used. + +**Affects:** End users + +**End users:** when recovering your account via a backup email, typing the wrong code enough times locked out the original code — but the page kept saying "Invalid or expired code. Please try again.", which implied more typing might help. The page now distinguishes that case and says "That code can no longer be used. Click 'Resend code' below to get a fresh one." pointing you at the Resend button right below the error, matching the standalone Account Settings sign-in flow. diff --git a/packages/auth-service/src/__tests__/account-login.test.ts b/packages/auth-service/src/__tests__/otp-verify-error.test.ts similarity index 72% rename from packages/auth-service/src/__tests__/account-login.test.ts rename to packages/auth-service/src/__tests__/otp-verify-error.test.ts index 07ec6f94..df811bec 100644 --- a/packages/auth-service/src/__tests__/account-login.test.ts +++ b/packages/auth-service/src/__tests__/otp-verify-error.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect } from 'vitest' -import { pickAccountLoginVerifyErrorMessage } from '../routes/account-login.js' +import { pickOtpVerifyErrorMessage } from '../lib/otp-verify-error.js' -describe('pickAccountLoginVerifyErrorMessage', () => { +describe('pickOtpVerifyErrorMessage', () => { // The standalone /account/login flow surfaces three distinct // error states through its server-rendered OTP form. The user- // facing copy must distinguish them, otherwise typing more in @@ -10,40 +10,40 @@ describe('pickAccountLoginVerifyErrorMessage', () => { it('points the user at Resend when the row was locked out', () => { const err = new Error('Too many attempts') - expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + expect(pickOtpVerifyErrorMessage(err)).toBe( 'That code can no longer be used. Click "Resend code" below to get a fresh one.', ) }) it('points the user at Resend when the OTP has expired', () => { const err = new Error('OTP expired') - expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + expect(pickOtpVerifyErrorMessage(err)).toBe( 'That code can no longer be used. Click "Resend code" below to get a fresh one.', ) }) it('asks the user to re-type on a typo', () => { const err = new Error('Invalid OTP') - expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + expect(pickOtpVerifyErrorMessage(err)).toBe( 'Invalid code. Please try again.', ) }) it('falls back to a generic verification-failed message on unknown errors', () => { const err = new Error('Internal database problem') - expect(pickAccountLoginVerifyErrorMessage(err)).toBe( + expect(pickOtpVerifyErrorMessage(err)).toBe( 'Verification failed. Please try again.', ) }) it('handles non-Error rejections gracefully', () => { - expect(pickAccountLoginVerifyErrorMessage('string-thrown')).toBe( + expect(pickOtpVerifyErrorMessage('string-thrown')).toBe( 'Verification failed. Please try again.', ) - expect(pickAccountLoginVerifyErrorMessage(null)).toBe( + expect(pickOtpVerifyErrorMessage(null)).toBe( 'Verification failed. Please try again.', ) - expect(pickAccountLoginVerifyErrorMessage(undefined)).toBe( + expect(pickOtpVerifyErrorMessage(undefined)).toBe( 'Verification failed. Please try again.', ) }) @@ -53,9 +53,7 @@ describe('pickAccountLoginVerifyErrorMessage', () => { // strings come through as English fixed text — the function // lowercases the message before matching, so capitalisation // shouldn't matter. - expect( - pickAccountLoginVerifyErrorMessage(new Error('TOO MANY ATTEMPTS')), - ).toBe( + expect(pickOtpVerifyErrorMessage(new Error('TOO MANY ATTEMPTS'))).toBe( 'That code can no longer be used. Click "Resend code" below to get a fresh one.', ) }) diff --git a/packages/auth-service/src/lib/otp-verify-error.ts b/packages/auth-service/src/lib/otp-verify-error.ts new file mode 100644 index 00000000..f86dd197 --- /dev/null +++ b/packages/auth-service/src/lib/otp-verify-error.ts @@ -0,0 +1,37 @@ +/** + * Shared OTP-verify error-message picker for the server-rendered + * sign-in flows (`/account/verify-otp` and `/auth/recover/verify`). + * + * Both routes call better-auth's `signInEmailOTP` and need to + * translate the caught error into user-facing copy. They have the + * same three meaningful cases: + * + * 1. Lockout / aged-out — "Too many attempts" or "OTP expired" + * from better-auth, OR the post-lockout INVALID_OTP that fires + * against a deleted verification row. The current code path is + * dead; more typing cannot succeed. Point the user at Resend. + * + * 2. Recoverable typo — "Invalid OTP" against a live row. Just + * ask the user to re-type. + * + * 3. Internal failure — anything else (network, DB, unexpected). + * Show a generic try-again message. + * + * Returning the message string rather than a structured kind keeps + * the call sites simple — they just feed it straight into their + * `renderOtpForm({ error: ... })` helper. + * + * The branching is exported as a pure function so unit tests can + * cover all three branches without standing up a router. + */ + +export function pickOtpVerifyErrorMessage(err: unknown): string { + const errText = err instanceof Error ? err.message.toLowerCase() : '' + if (/too many|attempt|expir/.test(errText)) { + return 'That code can no longer be used. Click "Resend code" below to get a fresh one.' + } + if (errText.includes('invalid')) { + return 'Invalid code. Please try again.' + } + return 'Verification failed. Please try again.' +} diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index e52f5758..caf132f0 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -20,35 +20,10 @@ import type { AuthServiceContext } from '../context.js' import { buildOtpInputProps } from '../otp-input.js' import type { BetterAuthInstance } from '../better-auth.js' import { POWERED_BY_CSS, POWERED_BY_HTML } from '../lib/page-helpers.js' +import { pickOtpVerifyErrorMessage } from '../lib/otp-verify-error.js' const logger = createLogger('auth:account-login') -/** - * Pick the user-facing OTP-verify error message for the standalone - * account-login form. Exported for unit tests; the route handler - * pipes its caught error through this helper rather than open-coding - * the branching. - * - * Three cases: - * - "too many attempts" / "expired" — better-auth has either - * locked the verification row out or aged it out, so any - * subsequent attempt with the same code is doomed. Point the - * user at Resend. - * - "invalid" — generic typo from better-auth (the shared - * INVALID_OTP message). Re-typing might fix it. - * - everything else — internal failure or unexpected shape. - */ -export function pickAccountLoginVerifyErrorMessage(err: unknown): string { - const errText = err instanceof Error ? err.message.toLowerCase() : '' - if (/too many|attempt|expir/.test(errText)) { - return 'That code can no longer be used. Click "Resend code" below to get a fresh one.' - } - if (errText.includes('invalid')) { - return 'Invalid code. Please try again.' - } - return 'Verification failed. Please try again.' -} - export function createAccountLoginRouter( auth: BetterAuthInstance, ctx: AuthServiceContext, @@ -148,7 +123,7 @@ export function createAccountLoginRouter( csrfToken: res.locals.csrfToken, otpLength: ctx.config.otpLength, otpCharset: ctx.config.otpCharset, - error: pickAccountLoginVerifyErrorMessage(err), + error: pickOtpVerifyErrorMessage(err), }), ) } diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index 523a499c..8c25f583 100644 --- a/packages/auth-service/src/routes/recovery.ts +++ b/packages/auth-service/src/routes/recovery.ts @@ -28,6 +28,7 @@ import { import { renderError } from '../lib/render-error.js' import { AUTH_FLOW_COOKIE, AUTH_FLOW_TTL_MS } from '../lib/auth-flow.js' import { heartbeatEnabledFor } from './login-page.js' +import { pickOtpVerifyErrorMessage } from '../lib/otp-verify-error.js' const logger = createLogger('auth:recovery') @@ -277,11 +278,11 @@ export function createRecoveryRouter( res.redirect(303, '/auth/complete') } catch (err: unknown) { logger.warn({ err, email }, 'Recovery OTP verification failed') - const errMsg = - err instanceof Error && - (err.message.includes('invalid') || err.message.includes('expired')) - ? 'Invalid or expired code. Please try again.' - : 'Verification failed. Please try again.' + // Same shape as the account-login flow: distinguish typo + // from lockout/aged-out so the user gets pointed at Resend + // rather than fighting an "Invalid or expired" error that + // more typing can't fix. + const errMsg = pickOtpVerifyErrorMessage(err) const { customCss, customFaviconUrl, customFaviconUrlDark, backUri } = await getFlowBranding(req) res.send( From e0c367bc0202a3eeb85ce661c2d2516f1cb4b90f Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:29:39 +0000 Subject: [PATCH 18/37] fix(auth-service): clear OTP boxes on Resend so old typing does not linger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the user starts typing the wrong code, realises mid-way, and clicks "Resend code" to start over, the boxes used to keep their typed digits — they'd have to delete what was there before the fresh code could go in. Adds an extra friction step exactly when the user is already self-correcting. The Resend success branch now runs clearOtpBoxes() and refocuses the first box, alongside the existing verifyLockedOut reset. Test: unit assertion that clearOtpBoxes() lives on the Code-resent success path (not just on the expired-OTP branch elsewhere). Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/clear-otp-boxes-on-resend.md | 9 +++++++++ .../src/__tests__/login-page.test.ts | 16 ++++++++++++++++ packages/auth-service/src/routes/login-page.ts | 6 ++++++ 3 files changed, 31 insertions(+) create mode 100644 .changeset/clear-otp-boxes-on-resend.md diff --git a/.changeset/clear-otp-boxes-on-resend.md b/.changeset/clear-otp-boxes-on-resend.md new file mode 100644 index 00000000..c77f8681 --- /dev/null +++ b/.changeset/clear-otp-boxes-on-resend.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Clicking "Resend code" now empties the digit boxes for the new code. + +**Affects:** End users + +**End users:** if you started typing your sign-in code, realised you'd made a mistake, and clicked **Resend code**, the boxes still held the digits you had typed. You had to delete them yourself before you could enter the fresh code. The boxes now reset on Resend so you can just type the new code straight away. diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 15b7ab15..7f5991e6 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -671,6 +671,22 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { expect(html).toMatch(/verifyLockedOut\s*=\s*false/) }) + it('clears the OTP boxes after a successful Resend so old typing does not linger', () => { + const html = renderDefault() + // When the user clicks Resend the verification row is rotated, + // so any digits they had typed for the old code are stale. + // The Resend success path must reset the boxes so the user + // can paste / type the fresh code without first deleting + // what's there. + const resendIdx = html.indexOf("'Code resent!'") + expect(resendIdx).toBeGreaterThan(-1) + // Look for clearOtpBoxes() near the success message — the + // success branch is what the test is asserting on, not the + // expired-otp branch which is much later in the file. + const window = html.slice(Math.max(0, resendIdx - 500), resendIdx + 200) + expect(window).toContain('clearOtpBoxes()') + }) + it('filters pasted OTP content to the configured charset before auto-submitting', () => { const html = renderDefault() // Numeric OTPs must drop letters / hyphens from a paste rather diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index a0eaa5c6..e5ee24ec 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -1318,6 +1318,12 @@ export function renderLoginPage(opts: { // verification row is brand new, so future submits can // succeed and shouldn't inherit the old lockout state. verifyLockedOut = false; + // Clear any digits the user typed for the old code so + // they can paste / type the fresh one without first + // having to delete what's there. Focus the first box so + // the next keystroke goes to the right place. + clearOtpBoxes(); + if (otpBoxes.length) otpBoxes[0].focus(); showSuccess('Code resent!'); } }); From 74459b88ce49de8617bb4965a3b826866b5f35c3 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:31:19 +0000 Subject: [PATCH 19/37] feat(auth-service): hint to check spam folder on OTP form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the user typo'd their email or the OTP landed in spam, they sat staring at the OTP form with no indication of what to do. The natural next move is "Use different email" or check the spam folder, but the page surfaced neither — so the user just waited and got frustrated. Add a small muted note below the digit boxes: "If you don't see the email, check your spam folder." Single line, doesn't compete with the "We sent a 8-digit code to xx@example.com" subtitle, lives above the Resend / Use different email actions so the visual reading order is: 1. We sent it to xx@example.com 2. (digit boxes + Verify) 3. If you don't see it, check spam 4. Otherwise, Resend / Use different email / Recover. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/spam-hint-on-otp-form.md | 9 +++++++++ packages/auth-service/src/routes/login-page.ts | 4 ++++ 2 files changed, 13 insertions(+) create mode 100644 .changeset/spam-hint-on-otp-form.md diff --git a/.changeset/spam-hint-on-otp-form.md b/.changeset/spam-hint-on-otp-form.md new file mode 100644 index 00000000..91965375 --- /dev/null +++ b/.changeset/spam-hint-on-otp-form.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Sign-in code page hints to check spam folder when the email doesn't arrive. + +**Affects:** End users + +**End users:** the page now shows a small "If you don't see the email, check your spam folder." note below the digit boxes. If you typo'd your email or the message landed in spam, you used to wait and wait and then get frustrated; the hint surfaces the most common reason early so you can either check spam or click "Use different email" sooner. diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index e5ee24ec..e305ae4d 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -632,6 +632,7 @@ export function renderLoginPage(opts: { .otp-box::placeholder { color: #d4d4d4; } .otp-box:focus { border-color: var(--focus-border); } .otp-actions { display: flex; gap: 32px; justify-content: center; margin-top: 12px; } + .otp-spam-hint { color: var(--muted-foreground); font-size: 13px; text-align: center; margin: 8px 0 16px; } .btn-primary { width: 100%; padding: 15px; background: ${brandColor}; color: white; border: none; border-radius: 9999px; font-size: 15px; font-weight: 500; cursor: pointer; transition: opacity 0.15s; } .btn-primary:hover { opacity: 0.9; } .btn-primary:disabled { opacity: 0.7; cursor: not-allowed; } @@ -714,6 +715,9 @@ export function renderLoginPage(opts: { +

+ If you don't see the email, check your spam folder. +

From 452eec32110c124c3903dda6811fb4093ec7cf11 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:47:40 +0000 Subject: [PATCH 20/37] test(auth-service): dedupe OTP-verify-error tests via shared constants + it.each Sonar flagged 3.7% line-duplication on new code, all of it inside the new otp-verify-error.test.ts. The duplicated lines were the expected-message strings repeated at every call site (5x for the Resend message alone). Lift them to module-level constants and fold the four lockout-shaped variants into a single it.each block, plus the three non-Error rejections into another it.each. Same coverage, no duplication, gate green. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/__tests__/otp-verify-error.test.ts | 79 ++++++++----------- 1 file changed, 33 insertions(+), 46 deletions(-) diff --git a/packages/auth-service/src/__tests__/otp-verify-error.test.ts b/packages/auth-service/src/__tests__/otp-verify-error.test.ts index df811bec..d94c2eac 100644 --- a/packages/auth-service/src/__tests__/otp-verify-error.test.ts +++ b/packages/auth-service/src/__tests__/otp-verify-error.test.ts @@ -1,60 +1,47 @@ import { describe, it, expect } from 'vitest' import { pickOtpVerifyErrorMessage } from '../lib/otp-verify-error.js' +// Expected user-facing messages for each error class. Lifted to +// constants so the tests don't repeat the exact copy at every call +// site (Sonar flags >3% line duplication on new code). +const RESEND_MSG = + 'That code can no longer be used. Click "Resend code" below to get a fresh one.' +const TYPO_MSG = 'Invalid code. Please try again.' +const FALLBACK_MSG = 'Verification failed. Please try again.' + describe('pickOtpVerifyErrorMessage', () => { - // The standalone /account/login flow surfaces three distinct - // error states through its server-rendered OTP form. The user- - // facing copy must distinguish them, otherwise typing more in + // The OTP verify flows (account-login, recovery) surface three + // distinct error states through their server-rendered OTP forms. + // The user-facing copy must distinguish them — typing more in // an unrecoverable state just rolls up failed attempts that // could never succeed. - it('points the user at Resend when the row was locked out', () => { - const err = new Error('Too many attempts') - expect(pickOtpVerifyErrorMessage(err)).toBe( - 'That code can no longer be used. Click "Resend code" below to get a fresh one.', - ) - }) - - it('points the user at Resend when the OTP has expired', () => { - const err = new Error('OTP expired') - expect(pickOtpVerifyErrorMessage(err)).toBe( - 'That code can no longer be used. Click "Resend code" below to get a fresh one.', - ) - }) + it.each([ + ['Too many attempts', RESEND_MSG], + ['OTP expired', RESEND_MSG], + ['Too many attempts on this code', RESEND_MSG], + ['TOO MANY ATTEMPTS', RESEND_MSG], // case-insensitive + ])( + 'points the user at Resend on lockout/aged-out: %s', + (errMessage, expected) => { + expect(pickOtpVerifyErrorMessage(new Error(errMessage))).toBe(expected) + }, + ) it('asks the user to re-type on a typo', () => { - const err = new Error('Invalid OTP') - expect(pickOtpVerifyErrorMessage(err)).toBe( - 'Invalid code. Please try again.', - ) + expect(pickOtpVerifyErrorMessage(new Error('Invalid OTP'))).toBe(TYPO_MSG) }) - it('falls back to a generic verification-failed message on unknown errors', () => { - const err = new Error('Internal database problem') - expect(pickOtpVerifyErrorMessage(err)).toBe( - 'Verification failed. Please try again.', - ) + it('falls back to generic verification-failed on unknown errors', () => { + expect( + pickOtpVerifyErrorMessage(new Error('Internal database problem')), + ).toBe(FALLBACK_MSG) }) - it('handles non-Error rejections gracefully', () => { - expect(pickOtpVerifyErrorMessage('string-thrown')).toBe( - 'Verification failed. Please try again.', - ) - expect(pickOtpVerifyErrorMessage(null)).toBe( - 'Verification failed. Please try again.', - ) - expect(pickOtpVerifyErrorMessage(undefined)).toBe( - 'Verification failed. Please try again.', - ) - }) - - it('matches the lockout pattern case-insensitively', () => { - // better-auth's INVALID_OTP / OTP_EXPIRED / TOO_MANY_ATTEMPTS - // strings come through as English fixed text — the function - // lowercases the message before matching, so capitalisation - // shouldn't matter. - expect(pickOtpVerifyErrorMessage(new Error('TOO MANY ATTEMPTS'))).toBe( - 'That code can no longer be used. Click "Resend code" below to get a fresh one.', - ) - }) + it.each([['string-thrown'], [null], [undefined]])( + 'falls back to generic verification-failed on non-Error rejections (%p)', + (err) => { + expect(pickOtpVerifyErrorMessage(err)).toBe(FALLBACK_MSG) + }, + ) }) From 453d9a57c6570ed6752d801fa90a8fc6241910f2 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:51:00 +0000 Subject: [PATCH 21/37] fix(auth-service): explicit autocomplete=email on every email input The email inputs on the login, account-login, and recovery forms relied on browsers heuristically autocompleting based on type=email + name=email. Heuristics work most of the time but explicit autocomplete=email makes browser autofill / password managers more reliable, especially in mobile webviews and privacy-stricter browsers that disable heuristic autofill. Three lines, one per route. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/auth-service/src/routes/account-login.ts | 1 + packages/auth-service/src/routes/login-page.ts | 1 + packages/auth-service/src/routes/recovery.ts | 1 + 3 files changed, 3 insertions(+) diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index caf132f0..2acaac98 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -154,6 +154,7 @@ function renderLoginForm(opts: { csrfToken: string; error?: string }): string {
diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index e305ae4d..2f8d1524 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -683,6 +683,7 @@ export function renderLoginPage(opts: {
diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index 8c25f583..a9230a5f 100644 --- a/packages/auth-service/src/routes/recovery.ts +++ b/packages/auth-service/src/routes/recovery.ts @@ -358,6 +358,7 @@ export function renderRecoveryForm(opts: {
From 35e76d294e004d582d9a7ec0513da4f86b2e0e4d Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:52:04 +0000 Subject: [PATCH 22/37] fix(auth-service): make sign-in errors announce to screen readers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error banners on the OAuth login, account-login, and recovery forms were plain divs / paragraphs. When an error appeared (or updated) screen-reader users got no announcement — the error silently changed and the user kept thinking the form had accepted their input. Add ARIA live-region semantics so updates announce naturally: - Main login flash banner (#error-msg): role=status aria-live=polite (the polite intent is correct — errors should announce on update without interrupting whatever the user was doing) - Server-rendered

banners on account-login and recovery: role=alert (these are static at render time, so the default assertive announcement on first render is appropriate) Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/auth-service/src/routes/account-login.ts | 4 ++-- packages/auth-service/src/routes/login-page.ts | 2 +- packages/auth-service/src/routes/recovery.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index 2acaac98..a9fd57ce 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -148,7 +148,7 @@ function renderLoginForm(opts: { csrfToken: string; error?: string }): string {

Account Settings

Sign in to manage your account

- ${opts.error ? '

' + escapeHtml(opts.error) + '

' : ''} + ${opts.error ? '' : ''}
@@ -191,7 +191,7 @@ function renderOtpForm(opts: {

Enter your code

We sent a ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to ${escapeHtml(maskedEmail)}

- ${opts.error ? '

' + escapeHtml(opts.error) + '

' : ''} + ${opts.error ? '' : ''} diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 2f8d1524..5ea1957a 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -673,7 +673,7 @@ export function renderLoginPage(opts: { ${logoHtml}

${opts.initialStep === 'otp' ? 'Enter your code' : 'Sign in'}

- + ${socialButtonsHtml} diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index a9230a5f..81a1af71 100644 --- a/packages/auth-service/src/routes/recovery.ts +++ b/packages/auth-service/src/routes/recovery.ts @@ -350,7 +350,7 @@ export function renderRecoveryForm(opts: {

Account Recovery

Enter the backup email address associated with your account.

- ${opts.error ? '

' + escapeHtml(opts.error) + '

' : ''} + ${opts.error ? '' : ''} @@ -445,7 +445,7 @@ export function renderRecoveryOtpForm(opts: {

Enter recovery code

If a backup email matches, we sent a ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to ${escapeHtml(maskedEmail)}

- ${opts.error ? '

' + escapeHtml(opts.error) + '

' : ''} + ${opts.error ? '' : ''} From c1cd111e8aab97fe90bf8ec419af0e42f696bb80 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:54:15 +0000 Subject: [PATCH 23/37] fix(demo): friendlier error banners, no developer-facing wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several of the demo's sign-in error banners used technical wording aimed at developers: - "Could not start login — the PDS rejected the request. Check server logs." - "Login could not be completed — token exchange failed." - "Login session expired or was tampered with." - "Invalid login hint format." End users have no idea what a "PAR" or "token exchange" is. Replace each with plain English that points at a next step: - par_failed: "Sign-in couldn't start right now. Please try again in a moment." - token_failed: "Sign-in couldn't be completed. Please sign in again." - state_mismatch: "Your sign-in session expired or was interrupted. Please sign in again." - invalid_login_hint: "We didn't recognise that account. Please sign in with your email instead." - auth_failed: "Sign-in failed. Please try again." (was "Authentication failed" — same length, less robotic) Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/friendlier-demo-error-banners.md | 9 +++++++++ packages/demo/src/app/components/LoginForm.tsx | 12 ++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) create mode 100644 .changeset/friendlier-demo-error-banners.md diff --git a/.changeset/friendlier-demo-error-banners.md b/.changeset/friendlier-demo-error-banners.md new file mode 100644 index 00000000..14128b02 --- /dev/null +++ b/.changeset/friendlier-demo-error-banners.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Demo client's error banners now use plain user-friendly language instead of developer wording. + +**Affects:** End users + +**End users:** several of the demo's sign-in error banners used technical wording aimed at developers ("PAR rejected the request — check server logs", "token exchange failed") that didn't tell you anything actionable. The wording is now plain English and points at the right next step ("Please try again in a moment", "Please sign in again"). diff --git a/packages/demo/src/app/components/LoginForm.tsx b/packages/demo/src/app/components/LoginForm.tsx index b0794113..dd00482f 100644 --- a/packages/demo/src/app/components/LoginForm.tsx +++ b/packages/demo/src/app/components/LoginForm.tsx @@ -5,17 +5,17 @@ import { useState } from 'react' import { ForceLoginCheckbox } from './ForceLoginCheckbox' const ERROR_MESSAGES: Record = { - auth_failed: 'Authentication failed. Please try again.', + auth_failed: 'Sign-in failed. Please try again.', session_expired: 'Your sign-in took too long to finish. Please sign in again.', - par_failed: - 'Could not start login — the PDS rejected the request. Check server logs.', + par_failed: "Sign-in couldn't start right now. Please try again in a moment.", invalid_email: 'Please enter a valid email address.', invalid_handle: 'Please enter a valid handle (e.g. you.bsky.social).', - invalid_login_hint: 'Invalid login hint format.', - token_failed: 'Login could not be completed — token exchange failed.', + invalid_login_hint: + "We didn't recognise that account. Please sign in with your email instead.", + token_failed: "Sign-in couldn't be completed. Please sign in again.", state_mismatch: - 'Login session expired or was tampered with. Please try again.', + 'Your sign-in session expired or was interrupted. Please sign in again.', } /** From 619eedf4ec30f0ca007689e93575110a967d39af Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 06:55:54 +0000 Subject: [PATCH 24/37] fix(auth-service): charset filter on account-login + recovery OTP inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The main login page's OTP boxes already filter typed/pasted content to the configured charset (numeric → digits-only; alphanumeric → letters+digits, uppercased). The account-login and recovery forms use a single-input variant of the same form and didn't have the same filter — pasting "1234-5678" or accidentally typing a letter into a numeric-only code would put stray characters in the input that better-auth would then reject as INVALID_OTP. Apply the same charset regex (built per-render from opts.otpCharset) on the inline `oninput` of both single-input forms, matching the segmented version's behaviour. Recovery's existing `oninput` already stripped whitespace + hyphens; the new regex supersedes it (digits-only includes both). Account-login had no inline filter; gains it now. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../charset-filter-on-server-rendered-otp-forms.md | 9 +++++++++ packages/auth-service/src/routes/account-login.ts | 1 + packages/auth-service/src/routes/recovery.ts | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 .changeset/charset-filter-on-server-rendered-otp-forms.md diff --git a/.changeset/charset-filter-on-server-rendered-otp-forms.md b/.changeset/charset-filter-on-server-rendered-otp-forms.md new file mode 100644 index 00000000..1ac88970 --- /dev/null +++ b/.changeset/charset-filter-on-server-rendered-otp-forms.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Account-settings and account-recovery code inputs also strip out characters that aren't part of the code. + +**Affects:** End users + +**End users:** the same paste/keystroke filter that the main sign-in form already had now applies to the standalone Account Settings sign-in and to the account recovery flow. If you copy your code from somewhere that wraps it in punctuation, or accidentally type a letter into a digits-only code, the input drops the stray characters silently instead of letting them through and rejecting the code as invalid. diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index a9fd57ce..62d3512f 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -206,6 +206,7 @@ function renderOtpForm(opts: { autocapitalize="${inputProps.autocapitalize}" placeholder="${inputProps.placeholder}" class="otp-input" + oninput="this.value=this.value.replace(${opts.otpCharset === 'alphanumeric' ? '/[^A-Za-z0-9]/g' : '/[^0-9]/g'},'')${opts.otpCharset === 'alphanumeric' ? '.toUpperCase()' : ''}" style="letter-spacing: ${Math.max(2, Math.round(32 / opts.otpLength))}px">
diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index 81a1af71..7cff0638 100644 --- a/packages/auth-service/src/routes/recovery.ts +++ b/packages/auth-service/src/routes/recovery.ts @@ -462,7 +462,7 @@ export function renderRecoveryOtpForm(opts: { autocapitalize="${inputProps.autocapitalize}" placeholder="${inputProps.placeholder}" class="otp-input" - oninput="this.value=this.value.replace(/[\\s-]/g,'')" + oninput="this.value=this.value.replace(${opts.otpCharset === 'alphanumeric' ? '/[^A-Za-z0-9]/g' : '/[^0-9]/g'},'')${opts.otpCharset === 'alphanumeric' ? '.toUpperCase()' : ''}" style="letter-spacing: ${Math.max(2, Math.round(32 / opts.otpLength))}px">
From d3e75a21e6ecf5248b8395124dafbc61de2004a0 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 07:11:35 +0000 Subject: [PATCH 25/37] fix(auth-service): handle picker error says "not available", not "just taken" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handle picker's error message for the handle_taken error code was "That handle was just taken — please choose another." That's accurate for the rare race where another user claims the handle in the same second, but most handle_taken responses fire because the handle is reserved (admin, www, support, etc.) — and "just taken" implies the user could try again later, when in reality a reserved handle will never be available. "That handle is not available" covers both cases honestly without implying transience. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/handle-not-available-message.md | 9 +++++++++ packages/auth-service/src/routes/choose-handle.ts | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 .changeset/handle-not-available-message.md diff --git a/.changeset/handle-not-available-message.md b/.changeset/handle-not-available-message.md new file mode 100644 index 00000000..6eb0bf43 --- /dev/null +++ b/.changeset/handle-not-available-message.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Handle-picker error wording is now accurate when the handle is reserved. + +**Affects:** End users + +**End users:** if you tried to claim a handle that was on the reserved list (admin, www, …) the picker said "That handle was just taken — please choose another." That's misleading — it wasn't just taken, it's permanently unavailable. The wording is now "That handle is not available — please choose another." which doesn't imply you can wait it out. diff --git a/packages/auth-service/src/routes/choose-handle.ts b/packages/auth-service/src/routes/choose-handle.ts index 555a912f..92019394 100644 --- a/packages/auth-service/src/routes/choose-handle.ts +++ b/packages/auth-service/src/routes/choose-handle.ts @@ -209,7 +209,7 @@ export function createChooseHandleRouter( } const KNOWN_ERROR_MESSAGES: Record = { - handle_taken: 'That handle was just taken — please choose another.', + handle_taken: 'That handle is not available — please choose another.', } const rawError = req.query.error as string | undefined const error = rawError From 97e5acb55e638733e5259e26b774dcd6465f2adf Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 07:35:09 +0000 Subject: [PATCH 26/37] test(e2e): de-flake @demo-cookie-expiry by skipping the email round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The @demo-cookie-expiry scenario flaked twice on CI (Railway preview env, OTP email never arrived within 60s). The original scenario walked the full email-OTP cycle just to land on the demo callback with no cookie — but the only thing under test is the demo's branching between session_expired vs auth_failed. Refactor: jump straight to /api/oauth/callback?code=...&state=... with no cookie set. The callback's first checks are "if (error)" → not present, "if (!code || !state)" → present; then it tries getOAuthSessionFromCookie which returns null, and routes to session_expired. That's the entire surface we care about for the bug-report fix. Faster (1s vs 30s+), deterministic, no Mailpit dependency. Co-Authored-By: Claude Opus 4.7 (1M context) --- e2e/step-definitions/auth.steps.ts | 20 +++++++++++++++ features/passwordless-authentication.feature | 27 ++++++++++++-------- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index f231904b..c2e77f33 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -911,6 +911,26 @@ Then( // This step deletes the cookie programmatically so we can exercise the // post-cookie-expiry callback path without a 10-minute wall-clock wait. +When( + 'the user navigates to the demo callback with code and state but no cookie', + async function (this: EpdsWorld) { + const page = getPage(this) + // Visit the demo origin first so document.cookie has the right + // origin context (needed for clearCookies to work reliably). + await page.goto(testEnv.demoUrl) + // Make sure no oauth_state cookie is present. + await page.context().clearCookies({ name: 'oauth_state' }) + // Land on /api/oauth/callback with plausible-looking query params. + // The callback's first thing it checks (after `error`) is + // !code || !state — both present here. Then it tries to read the + // signed cookie, which is gone, and routes to session_expired. + const url = new URL('/api/oauth/callback', testEnv.demoUrl) + url.searchParams.set('code', 'test-code') + url.searchParams.set('state', 'test-state') + await page.goto(url.toString()) + }, +) + When( "the demo client's OAuth state cookie has expired", async function (this: EpdsWorld) { diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 83784634..e17a0a0e 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -494,16 +494,23 @@ Feature: Passwordless authentication via email OTP When the user clicks "Use different email" Then the email input is empty and focused - @email @demo-cookie-expiry @bug-report - Scenario: Demo client's OAuth cookie has expired by the time of callback — useful error, not generic auth_failed - When the demo client starts a new OAuth flow with random handle mode - Then the browser is redirected to the auth service login page - And the login page displays an email input form - When the user enters a unique test email and submits - Then an OTP email arrives in the mail trap for the test email - And the login page shows an OTP verification form - When the demo client's OAuth state cookie has expired - And the user enters the OTP code + @demo-cookie-expiry @bug-report + Scenario: Demo client's OAuth callback with no oauth_state cookie surfaces a session-expired error, not generic auth_failed + # When the user has stalled long enough for the demo's OAuth state + # cookie to expire by the time they finish the OTP cycle, the + # demo callback can't find the cookie. Without the bug-report fix + # this used to surface as "Authentication failed" — misleading + # because the sign-in itself succeeded; the demo just dropped the + # ball at the end. This scenario hits the callback with valid- + # looking parameters but no cookie, asserts the session-expired + # branch fires. + # + # The scenario synthesises code+state on the URL because it's + # only testing the demo's callback branching — not the full + # sign-in flow. That avoids dependency on Mailpit (which can + # be slow on Railway preview envs and was flaking the older + # form of this scenario). + When the user navigates to the demo callback with code and state but no cookie Then the demo client surfaces a session-expired error @email @otp-and-par-expiry @prompt-login From fe13498568467df17e439f973abd3b9523ead479 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 08:24:14 +0000 Subject: [PATCH 27/37] fix(auth-service): SMS / email autofill distributes the OTP across boxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iOS and Android offer to autofill a verification code into the first input tagged autocomplete=one-time-code. They drop the entire code into that box as a single input event with the full string. Our handler kept only the LAST char ("v.slice(-1)") on multi-char input — meaning the user got the first 7 digits discarded and only the 8th in box 0, with no way to see what went wrong. When v.length > 1 on the input handler, distribute v[0..N] across otpBoxes starting at the current idx, mirroring the paste handler. Then focus the last filled box and auto-submit if the grid is full. Test: unit assertion that the rendered handler distributes v across boxes (v[i] → otpBoxes[idx + i].value) for v.length > 1. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...s-autofill-distributes-across-otp-boxes.md | 9 +++++++++ .../src/__tests__/login-page.test.ts | 12 +++++++++++ .../auth-service/src/routes/login-page.ts | 20 +++++++++++++++++-- 3 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 .changeset/sms-autofill-distributes-across-otp-boxes.md diff --git a/.changeset/sms-autofill-distributes-across-otp-boxes.md b/.changeset/sms-autofill-distributes-across-otp-boxes.md new file mode 100644 index 00000000..f5d4b94f --- /dev/null +++ b/.changeset/sms-autofill-distributes-across-otp-boxes.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +iOS and Android SMS / email autofill now fills the digit boxes correctly. + +**Affects:** End users + +**End users:** if your phone offered to autofill the sign-in code from a recent text message or email, the autofill used to put the whole code into the first box and discard everything but the last digit — leaving you to retype the rest. The boxes now distribute the autofilled code across them, the same way they do when you paste it manually. diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 7f5991e6..f5aa7be3 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -710,6 +710,18 @@ describe('renderLoginPage inline Resend action on expired OTP', () => { ) }) + it('distributes a multi-char input event across the OTP boxes (iOS SMS autofill)', () => { + const html = renderDefault() + // iOS / Android SMS autofill drops the whole code into the + // first box (the one tagged autocomplete=one-time-code) as a + // single input event with the full string. Without + // distribution the user would lose every digit but the last. + // The handler must spread v[0..N] across otpBoxes starting at + // the current idx, then focus the last filled box. + expect(html).toMatch(/if \(v\.length > 1\)/) + expect(html).toMatch(/otpBoxes\[idx \+ i\]\.value = v\[i\]/) + }) + it('renders the recovery link with the actual request_uri, not a placeholder', () => { // The "Recover with backup email" link must round-trip the // active OAuth flow's request_uri, so the recovery page's diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 5ea1957a..eb16ad23 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -969,10 +969,26 @@ export function renderLoginPage(opts: { var inputCharsetRegex = otpCharset === 'alphanumeric' ? /[^A-Za-z0-9]/g : /[^0-9]/g; otpBoxes.forEach(function(box, idx) { box.addEventListener('input', function() { - // keep only the last typed char (handles paste into a single box) var v = box.value.replace(/\\s/g, '').replace(inputCharsetRegex, ''); - if (v.length > 1) v = v.slice(-1); if (v && otpCharset === 'alphanumeric') v = v.toUpperCase(); + if (v.length > 1) { + // iOS / Android SMS autofill drops the entire code into + // the first box (the one tagged autocomplete=one-time-code) + // as a single input event with the full string. Without + // distribution the user would see only the last digit + // and lose the rest. Distribute across boxes starting + // here, mirroring the paste handler. + for (var i = 0; i < v.length && idx + i < otpBoxes.length; i++) { + otpBoxes[idx + i].value = v[i]; + } + updateHiddenCode(); + var nextIdx = Math.min(idx + v.length, otpBoxes.length - 1); + otpBoxes[nextIdx].focus(); + if (hiddenCode.value.length === otpBoxes.length) { + document.getElementById('form-verify-otp').requestSubmit(); + } + return; + } box.value = v; updateHiddenCode(); if (box.value && idx < otpBoxes.length - 1) otpBoxes[idx + 1].focus(); From 83de6942f7b6ca5c544995a0ba36ca1e92bcc512 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 08:32:58 +0000 Subject: [PATCH 28/37] fix(pds-core): handle picker live-check flags reserved handles as unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handle picker on auth-service runs a live availability check against pds-core's /_internal/check-handle endpoint. The endpoint only checked accountManager.getAccount() — it did not check upstream's reservedSubdomains list. So reserved handles like "admin", "www", "support" came back as ✓ Available on the live check, only to be rejected with HandleUnavailableError on Submit. The user pays for the round-trip with their typing time and gets bounced back to the picker with no indication of what went wrong upfront. Lift the upstream's reservedSubdomains object via a deep import into a small helper (`isReservedSubdomain`), and OR it into the `exists` field returned by /_internal/check-handle. The picker's existing client-side branch — `data.available` falsy → "✗ Not available" — now fires upfront for reserved handles, so the user sees the same disabled-Submit + helpful-error UX as for already-taken handles. Test: 4 unit tests on the reserved-handle helper covering sample reserved handles, case-insensitivity, normal handles, and the empty-string case. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../handle-picker-flags-reserved-handles.md | 9 +++++ .../src/__tests__/reserved-handle.test.ts | 33 +++++++++++++++++++ packages/pds-core/src/index.ts | 13 +++++++- packages/pds-core/src/lib/reserved-handle.ts | 22 +++++++++++++ 4 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 .changeset/handle-picker-flags-reserved-handles.md create mode 100644 packages/pds-core/src/__tests__/reserved-handle.test.ts create mode 100644 packages/pds-core/src/lib/reserved-handle.ts diff --git a/.changeset/handle-picker-flags-reserved-handles.md b/.changeset/handle-picker-flags-reserved-handles.md new file mode 100644 index 00000000..b415bf1f --- /dev/null +++ b/.changeset/handle-picker-flags-reserved-handles.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Handle picker now flags reserved handles as unavailable upfront. + +**Affects:** End users + +**End users:** the handle picker has a live availability check that says ✓ Available or ✗ Already taken as you type. For reserved handles (admin, support, www, …) it used to show ✓ Available, then refuse the handle on submit with a "not available" error — wasting the time you spent on the form. The live check now flags reserved handles as unavailable up front so you can keep going without losing what you typed. diff --git a/packages/pds-core/src/__tests__/reserved-handle.test.ts b/packages/pds-core/src/__tests__/reserved-handle.test.ts new file mode 100644 index 00000000..87f7e96c --- /dev/null +++ b/packages/pds-core/src/__tests__/reserved-handle.test.ts @@ -0,0 +1,33 @@ +import { describe, it, expect } from 'vitest' +import { isReservedSubdomain } from '../lib/reserved-handle.js' + +describe('isReservedSubdomain', () => { + it.each([ + // Common reserved subdomains from upstream's @atproto/pds list. + // The list is curated; we don't reproduce it here, just sample + // a few that should always be reserved. + ['admin'], + ['www'], + ['support'], + ['help'], + ['api'], + ['bsky'], + ])('flags reserved subdomain "%s" as reserved', (local) => { + expect(isReservedSubdomain(local)).toBe(true) + }) + + it('treats reserved subdomains case-insensitively', () => { + expect(isReservedSubdomain('ADMIN')).toBe(true) + expect(isReservedSubdomain('Www')).toBe(true) + }) + + it('returns false for normal handles', () => { + expect(isReservedSubdomain('alice')).toBe(false) + expect(isReservedSubdomain('bob42')).toBe(false) + expect(isReservedSubdomain('my-handle')).toBe(false) + }) + + it('returns false for the empty string (handle picker passes "" while still typing)', () => { + expect(isReservedSubdomain('')).toBe(false) + }) +}) diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index bdbe8c71..a8929a38 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -46,6 +46,7 @@ import { validateClientMetadataForPreview, } from '@certified-app/shared' import { shouldRewriteSecFetchSite } from './lib/sec-fetch-site-rewrite.js' +import { isReservedSubdomain } from './lib/reserved-handle.js' import { findInsertionIndex, installCssInjectionMiddleware, @@ -1022,7 +1023,17 @@ async function main() { } try { const account = await pds.ctx.accountManager.getAccount(handle) - res.json({ exists: !!account }) + // Reserved-subdomain check: upstream's createAccount throws + // HandleUnavailableError when the front part of the handle is + // in @atproto/pds's reservedSubdomains list (admin, www, support, + // ...). Without checking here, the live availability check on + // the picker said "✓ Available!" for those handles and the user + // only learned otherwise after clicking Create. Surface the + // unavailability inline so the picker disables Submit and shows + // an honest error instead. + const local = handle.split('.')[0] ?? '' + const isReserved = isReservedSubdomain(local) + res.json({ exists: !!account || isReserved }) } catch (err) { logger.error({ err, handle }, 'Failed to check handle availability') res.status(503).json({ error: 'handle_check_failed' }) diff --git a/packages/pds-core/src/lib/reserved-handle.ts b/packages/pds-core/src/lib/reserved-handle.ts new file mode 100644 index 00000000..ffe9c30a --- /dev/null +++ b/packages/pds-core/src/lib/reserved-handle.ts @@ -0,0 +1,22 @@ +/** + * Reserved-subdomain check that mirrors @atproto/pds's + * `ensureHandleServiceConstraints`. + * + * Upstream's reserved list lives at + * `@atproto/pds/dist/handle/reserved.js` and is not part of the + * public package exports — only `ensureHandleServiceConstraints` is. + * That helper throws an `HandleNotAvailable` error when the local + * part is reserved, but we want a cheap boolean for the live + * availability check on the handle picker (no need to throw + catch + * just to render a status string). + * + * Import the deep path explicitly. Pinned to the installed + * `@atproto/pds` version, so a major upgrade can break the import — + * caught immediately at typecheck / build time. + */ + +import { reservedSubdomains } from '@atproto/pds/dist/handle/reserved.js' + +export function isReservedSubdomain(local: string): boolean { + return local.toLowerCase() in reservedSubdomains +} From 50077c0424fa42b4a398923e5caa138590248e75 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 08:45:57 +0000 Subject: [PATCH 29/37] fix(auth-service): visible keyboard focus on Verify and Resend buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Buttons on the OTP form had :hover styles but no :focus-visible styles — so keyboard users tabbing through the form had no idea which button was focused (the browser default focus ring is suppressed by some user-agent stylesheets and the .btn-primary explicitly sets `border: none`). Add :focus-visible styles on .btn-primary and .btn-secondary that match the existing --focus-border CSS custom property the input fields already use, with a 2px outline + 2px offset so the ring sits clear of the rounded button edges. Pure CSS change; no behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/auth-service/src/routes/login-page.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index eb16ad23..eba4837b 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -635,9 +635,11 @@ export function renderLoginPage(opts: { .otp-spam-hint { color: var(--muted-foreground); font-size: 13px; text-align: center; margin: 8px 0 16px; } .btn-primary { width: 100%; padding: 15px; background: ${brandColor}; color: white; border: none; border-radius: 9999px; font-size: 15px; font-weight: 500; cursor: pointer; transition: opacity 0.15s; } .btn-primary:hover { opacity: 0.9; } + .btn-primary:focus-visible { outline: 2px solid var(--focus-border, #2563eb); outline-offset: 2px; } .btn-primary:disabled { opacity: 0.7; cursor: not-allowed; } - .btn-secondary { display: inline-block; color: #6b6b6b; background: none; border: none; font-size: 14px; font-weight: 500; cursor: pointer; padding: 4px 0; } + .btn-secondary { display: inline-block; color: #6b6b6b; background: none; border: none; font-size: 14px; font-weight: 500; cursor: pointer; padding: 4px 0; border-radius: 4px; } .btn-secondary:hover { color: #1A130F; } + .btn-secondary:focus-visible { outline: 2px solid var(--focus-border, #2563eb); outline-offset: 2px; } .btn-social { display: flex; align-items: center; justify-content: center; gap: 8px; width: 100%; padding: 13px 20px; border: 1px solid var(--btn-secondary-border); border-radius: 9999px; font-size: 15px; font-weight: 500; cursor: pointer; text-decoration: none; background: white; color: #333; margin-bottom: 8px; transition: background 0.15s; } .btn-social:hover { background: #fafafa; } .btn-atproto { margin-top: 12px; margin-bottom: 0; color: #1A130F !important; background: var(--input-bg) !important; border-color: var(--input-border) !important; } From 645a2fbf30e3a5bbfc3a00c7227b036e7b067656 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 08:47:37 +0000 Subject: [PATCH 30/37] fix(auth-service): visible keyboard focus on remaining route buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same focus-visible treatment that the main login page got in the previous commit, applied to the standalone account-login form, the recovery form, and the handle-picker buttons. All three had :hover styles and no :focus-visible — keyboard users tabbing through had no idea where focus was. Pure CSS change. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/auth-service/src/routes/account-login.ts | 4 +++- packages/auth-service/src/routes/choose-handle.ts | 2 ++ packages/auth-service/src/routes/recovery.ts | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/auth-service/src/routes/account-login.ts b/packages/auth-service/src/routes/account-login.ts index 62d3512f..e822403e 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -238,6 +238,8 @@ const CSS = ` .otp-input { font-size: 28px !important; text-align: center; font-family: 'SF Mono', Menlo, Consolas, monospace !important; padding: 14px !important; } .btn-primary { width: 100%; padding: 12px; background: #0f1828; color: white; border: none; border-radius: 8px; font-size: 16px; font-weight: 500; cursor: pointer; } .btn-primary:hover { background: #1a2a40; } - .btn-secondary { display: inline-block; color: #0f1828; background: none; border: none; font-size: 14px; cursor: pointer; text-decoration: underline; } + .btn-primary:focus-visible { outline: 2px solid #0f1828; outline-offset: 2px; } + .btn-secondary { display: inline-block; color: #0f1828; background: none; border: none; font-size: 14px; cursor: pointer; text-decoration: underline; border-radius: 4px; } + .btn-secondary:focus-visible { outline: 2px solid #0f1828; outline-offset: 2px; } .error { color: #dc3545; background: #fdf0f0; padding: 12px; border-radius: 8px; margin: 12px 0; } ` diff --git a/packages/auth-service/src/routes/choose-handle.ts b/packages/auth-service/src/routes/choose-handle.ts index 92019394..866493fd 100644 --- a/packages/auth-service/src/routes/choose-handle.ts +++ b/packages/auth-service/src/routes/choose-handle.ts @@ -536,9 +536,11 @@ export function renderChooseHandlePage( .error { color: #dc3545; background: #fdf0f0; padding: 12px; border-radius: 8px; margin-bottom: 16px; font-size: 14px; } .btn-primary { width: 100%; padding: 12px; background: #0f1828; color: white; border: none; border-radius: 8px; font-size: 16px; font-weight: 500; cursor: pointer; margin-top: 8px; } .btn-primary:hover:not(:disabled) { background: #1a2a40; } + .btn-primary:focus-visible { outline: 2px solid #0f1828; outline-offset: 2px; } .btn-primary:disabled { opacity: 0.5; cursor: not-allowed; } .btn-secondary { width: 100%; padding: 10px; background: white; color: #0f1828; border: 1px solid #0f1828; border-radius: 8px; font-size: 15px; font-weight: 500; cursor: pointer; margin-top: 8px; } .btn-secondary:hover:not(:disabled) { background: #f0f2f5; } + .btn-secondary:focus-visible { outline: 2px solid #0f1828; outline-offset: 2px; } .btn-secondary:disabled { opacity: 0.5; cursor: not-allowed; } ${renderOptionalStyleTag(customCss)} diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index 7cff0638..110e88ab 100644 --- a/packages/auth-service/src/routes/recovery.ts +++ b/packages/auth-service/src/routes/recovery.ts @@ -522,6 +522,8 @@ const CSS = ` .otp-input { font-size: 28px !important; text-align: center; font-family: 'SF Mono', Menlo, Consolas, monospace !important; padding: 14px !important; } .btn-primary { width: 100%; padding: 12px; background: #0f1828; color: white; border: none; border-radius: 8px; font-size: 16px; font-weight: 500; cursor: pointer; } .btn-primary:hover { background: #1a2a40; } - .btn-secondary { display: inline-block; margin-top: 12px; color: #0f1828; background: none; border: none; font-size: 14px; cursor: pointer; text-decoration: underline; } + .btn-primary:focus-visible { outline: 2px solid #0f1828; outline-offset: 2px; } + .btn-secondary { display: inline-block; margin-top: 12px; color: #0f1828; background: none; border: none; font-size: 14px; cursor: pointer; text-decoration: underline; border-radius: 4px; } + .btn-secondary:focus-visible { outline: 2px solid #0f1828; outline-offset: 2px; } .error { color: #dc3545; background: #fdf0f0; padding: 12px; border-radius: 8px; margin: 12px 0; } ` From 149380123f815106d92bcb8775eba0cd938e2a04 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 08:47:58 +0000 Subject: [PATCH 31/37] chore(changeset): keyboard focus-visible on buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers both prior commits (50077c0 main login + 645a2fb other routes) — single end-user change with one consolidated entry. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/keyboard-focus-visible-on-buttons.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/keyboard-focus-visible-on-buttons.md diff --git a/.changeset/keyboard-focus-visible-on-buttons.md b/.changeset/keyboard-focus-visible-on-buttons.md new file mode 100644 index 00000000..bf99df16 --- /dev/null +++ b/.changeset/keyboard-focus-visible-on-buttons.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Sign-in buttons now show a visible keyboard focus ring. + +**Affects:** End users + +**End users:** keyboard users tabbing through the sign-in pages couldn't tell where focus was — the buttons styled out the browser default focus ring without replacing it. Verify, Resend, Use different email, Recover, Send recovery code, Continue with email, Update handle, etc. all now show a clear outline when focused via the keyboard, while still hiding the ring on a mouse click. From 116f833e0429718bc44064c4dddbdaa9248b949e Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 08:49:18 +0000 Subject: [PATCH 32/37] fix(demo): error banner announces to screen readers via role=alert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The demo's sign-in error banner was a plain
. When an error appeared screen-reader users got no announcement — visual users saw the red banner, screen-reader users had no idea anything had changed. role="alert" matches the auth-service error banners in the previous commits (which got role=status / role=alert) and tells assistive tech to announce the message immediately. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/demo/src/app/components/LoginForm.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/demo/src/app/components/LoginForm.tsx b/packages/demo/src/app/components/LoginForm.tsx index dd00482f..1a86d809 100644 --- a/packages/demo/src/app/components/LoginForm.tsx +++ b/packages/demo/src/app/components/LoginForm.tsx @@ -40,6 +40,7 @@ export function LoginForm() { <> {errorMessage && (
Date: Thu, 7 May 2026 09:44:48 +0000 Subject: [PATCH 33/37] refactor(pds-core): extract handleIsUnavailable for the check-handle route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /_internal/check-handle handler open-coded the OR-shape that combines DB existence with reserved-subdomain match. Extract that into a pure helper (handleIsUnavailable) on the existing reserved-handle module so: - The route handler stays thin (one call instead of three lines). - Unit tests can cover the OR-shape directly without standing up a PDS instance — including the corner cases (account exists + not reserved, account missing + reserved, both, neither, case- insensitive matching, empty fullHandle). Six new unit tests on the helper. No behaviour change at the HTTP boundary. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/__tests__/reserved-handle.test.ts | 74 ++++++++++++++++++- packages/pds-core/src/index.ts | 22 +++--- packages/pds-core/src/lib/reserved-handle.ts | 26 +++++++ 3 files changed, 109 insertions(+), 13 deletions(-) diff --git a/packages/pds-core/src/__tests__/reserved-handle.test.ts b/packages/pds-core/src/__tests__/reserved-handle.test.ts index 87f7e96c..a1aa6c5b 100644 --- a/packages/pds-core/src/__tests__/reserved-handle.test.ts +++ b/packages/pds-core/src/__tests__/reserved-handle.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect } from 'vitest' -import { isReservedSubdomain } from '../lib/reserved-handle.js' +import { + isReservedSubdomain, + handleIsUnavailable, +} from '../lib/reserved-handle.js' describe('isReservedSubdomain', () => { it.each([ @@ -30,4 +33,73 @@ describe('isReservedSubdomain', () => { it('returns false for the empty string (handle picker passes "" while still typing)', () => { expect(isReservedSubdomain('')).toBe(false) }) + + it('returns false for whitespace-only or punctuation-only inputs', () => { + // The picker normalises before calling, but defence-in-depth on the + // helper. Reserved list doesn't contain spaces, so these all miss. + expect(isReservedSubdomain(' ')).toBe(false) + expect(isReservedSubdomain('---')).toBe(false) + }) +}) + +describe('handleIsUnavailable', () => { + // The handle picker's live availability check treats "exists" as + // "unavailable" — and "unavailable" has two sources: someone owns + // the handle in the DB, OR it's a reserved subdomain. The helper + // OR-combines them so the picker doesn't paint a misleading + // "✓ Available" on a reserved handle. + + it('returns true when the account already exists in the DB', () => { + expect( + handleIsUnavailable({ + fullHandle: 'someone.epds-poc1.test.certified.app', + accountExists: true, + }), + ).toBe(true) + }) + + it('returns true when the local part is a reserved subdomain', () => { + expect( + handleIsUnavailable({ + fullHandle: 'admin.epds-poc1.test.certified.app', + accountExists: false, + }), + ).toBe(true) + }) + + it('returns true when both the account exists AND the local part is reserved', () => { + // Pathological case: someone managed to get a row created with a + // reserved local part (e.g. via a config change after an account + // existed). Either condition alone makes the handle unavailable. + expect( + handleIsUnavailable({ + fullHandle: 'admin.epds-poc1.test.certified.app', + accountExists: true, + }), + ).toBe(true) + }) + + it('returns false for a fresh, non-reserved handle', () => { + expect( + handleIsUnavailable({ + fullHandle: 'alice.epds-poc1.test.certified.app', + accountExists: false, + }), + ).toBe(false) + }) + + it('matches reserved local parts case-insensitively', () => { + expect( + handleIsUnavailable({ + fullHandle: 'ADMIN.epds-poc1.test.certified.app', + accountExists: false, + }), + ).toBe(true) + }) + + it('handles an empty fullHandle gracefully (route handler rejects empty before calling)', () => { + expect(handleIsUnavailable({ fullHandle: '', accountExists: false })).toBe( + false, + ) + }) }) diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index a8929a38..f3cec47f 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -46,7 +46,7 @@ import { validateClientMetadataForPreview, } from '@certified-app/shared' import { shouldRewriteSecFetchSite } from './lib/sec-fetch-site-rewrite.js' -import { isReservedSubdomain } from './lib/reserved-handle.js' +import { handleIsUnavailable } from './lib/reserved-handle.js' import { findInsertionIndex, installCssInjectionMiddleware, @@ -1023,17 +1023,15 @@ async function main() { } try { const account = await pds.ctx.accountManager.getAccount(handle) - // Reserved-subdomain check: upstream's createAccount throws - // HandleUnavailableError when the front part of the handle is - // in @atproto/pds's reservedSubdomains list (admin, www, support, - // ...). Without checking here, the live availability check on - // the picker said "✓ Available!" for those handles and the user - // only learned otherwise after clicking Create. Surface the - // unavailability inline so the picker disables Submit and shows - // an honest error instead. - const local = handle.split('.')[0] ?? '' - const isReserved = isReservedSubdomain(local) - res.json({ exists: !!account || isReserved }) + // The handle picker treats "exists" as "unavailable" — see + // `handleIsUnavailable` for why both DB-collision and + // reserved-subdomain match feed the same boolean. + res.json({ + exists: handleIsUnavailable({ + fullHandle: handle, + accountExists: !!account, + }), + }) } catch (err) { logger.error({ err, handle }, 'Failed to check handle availability') res.status(503).json({ error: 'handle_check_failed' }) diff --git a/packages/pds-core/src/lib/reserved-handle.ts b/packages/pds-core/src/lib/reserved-handle.ts index ffe9c30a..3afa8aed 100644 --- a/packages/pds-core/src/lib/reserved-handle.ts +++ b/packages/pds-core/src/lib/reserved-handle.ts @@ -20,3 +20,29 @@ import { reservedSubdomains } from '@atproto/pds/dist/handle/reserved.js' export function isReservedSubdomain(local: string): boolean { return local.toLowerCase() in reservedSubdomains } + +/** + * Compute the `exists` field returned by /_internal/check-handle. + * + * The handle picker's live availability check treats "exists" as + * "this handle is unavailable to claim" — which has two distinct + * sources: + * + * 1. Someone already owns it (DB row in accountManager). + * 2. It's in the upstream reserved-subdomains list (admin, www, + * support, …). createAccount would later throw + * HandleUnavailableError; better to flag it here so the picker + * disables Submit and shows the same "not available" status as + * the already-owned case. + * + * Pure function so the route handler can stay thin and unit tests + * cover the OR-shape directly without standing up the full PDS. + */ +export function handleIsUnavailable(opts: { + fullHandle: string + accountExists: boolean +}): boolean { + if (opts.accountExists) return true + const local = opts.fullHandle.split('.')[0] ?? '' + return isReservedSubdomain(local) +} From 5fc98376be7605f1c03b300fc9aefa47c6efa0ee Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 09:57:39 +0000 Subject: [PATCH 34/37] fix(shared): renderError adds role=alert on the error

MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared renderError() helper produces every styled HTML fallback page (Sign-in session expired, internal failures, session not found, …). The error message

was a plain paragraph; screen-reader users got no announcement when these pages loaded. role="alert" tells assistive tech to announce immediately on page render. Matches the role=alert treatment we just gave the inline server-rendered error banners on account-login and recovery. One-character behaviour change; no test fixtures depend on the exact HTML. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/shared/src/render-error.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shared/src/render-error.ts b/packages/shared/src/render-error.ts index 22cf2593..56f7942e 100644 --- a/packages/shared/src/render-error.ts +++ b/packages/shared/src/render-error.ts @@ -102,7 +102,7 @@ export function renderError(

${escapeHtml(title)}

-

${escapeHtml(message)}

+ ${startOverHtml}
${bodyExtra} From 3ed32b945c81e06ba7e8175f1326a5e3781835d8 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 7 May 2026 10:27:18 +0000 Subject: [PATCH 35/37] fix(shared): malformed client_id no longer leaks into displayed app name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveClientMetadata returned `{ client_name: clientId }` when the clientId failed to parse as a URL, or used a non-http(s) protocol. The intent was probably "give the caller something to display" — but the upshot was that a malformed query-string client_id ("Sign in to not-a-url") or a schemeless one ("Sign in to javascript:…") showed up verbatim in: - the auth-service login page tag - the consent screen header - the OTP form's "Sign in to <appName>" heading — effectively letting the URL bar shape user-visible text. Return `{}` from those branches instead, so the caller's fallback chain (`client_name || extractDomain(clientId) || 'an application'`) ends in the static "an application" string. Tests: two existing assertions on the leaky behaviour are inverted to assert the fallback path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --- ...fallback-app-name-on-malformed-client-id.md | 9 +++++++++ .../src/__tests__/client-metadata.test.ts | 18 ++++++++++++++---- packages/shared/src/client-metadata.ts | 12 ++++++++++-- 3 files changed, 33 insertions(+), 6 deletions(-) create mode 100644 .changeset/fallback-app-name-on-malformed-client-id.md diff --git a/.changeset/fallback-app-name-on-malformed-client-id.md b/.changeset/fallback-app-name-on-malformed-client-id.md new file mode 100644 index 00000000..376284f8 --- /dev/null +++ b/.changeset/fallback-app-name-on-malformed-client-id.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Sign-in pages no longer leak a malformed client_id as the displayed app name. + +**Affects:** End users + +**End users:** if you arrived at a sign-in page with a broken `client_id` parameter on the URL (e.g. from a misconfigured app or a tampered link), the page would display that raw broken value as the app's name, e.g. "Sign in to not-a-url" or "Sign in to my-local-app". The page now falls back to "an application" in that case, so a malformed value in the URL doesn't leak into the visible page text or browser tab title. diff --git a/packages/shared/src/__tests__/client-metadata.test.ts b/packages/shared/src/__tests__/client-metadata.test.ts index 5a971c30..4c7330e5 100644 --- a/packages/shared/src/__tests__/client-metadata.test.ts +++ b/packages/shared/src/__tests__/client-metadata.test.ts @@ -23,9 +23,14 @@ beforeEach(() => { }) describe('resolveClientMetadata', () => { - it('returns client_name for non-URL client_id', async () => { + it('returns empty metadata for a non-URL client_id', async () => { + // Non-URL client_ids used to surface the raw value as + // metadata.client_name, which then ended up in page titles + // ("Sign in to not-a-url") on the auth-service login page. + // Returning empty metadata lets the caller's fallback chain + // (client_name || extractDomain → 'an application') win. const metadata = await resolveClientMetadata('my-local-app') - expect(metadata.client_name).toBe('my-local-app') + expect(metadata).toEqual({}) }) it('returns seeded metadata from cache', async () => { @@ -135,9 +140,14 @@ describe('resolveClientName', () => { expect(name).toBe('no-name.app') }) - it('returns client_id as-is for non-URL', async () => { + it('falls back to "an application" for a non-URL client_id', async () => { + // Non-URL client_ids used to leak through as the displayed + // app name (e.g. "Sign in to unnamed-client"). The empty- + // metadata change in resolveClientMetadata + extractDomain + // returning null on parse failure means resolveClientName's + // final fallback wins, yielding a generic "an application". const name = await resolveClientName('unnamed-client') - expect(name).toBe('unnamed-client') + expect(name).toBe('an application') }) }) diff --git a/packages/shared/src/client-metadata.ts b/packages/shared/src/client-metadata.ts index 1c3d0ded..fec97dc2 100644 --- a/packages/shared/src/client-metadata.ts +++ b/packages/shared/src/client-metadata.ts @@ -160,10 +160,18 @@ export async function resolveClientMetadata( try { parsedUrl = new URL(clientId) } catch { - return { client_name: clientId } + // Malformed clientId — return empty metadata so the caller's + // fallback chain (client_name || extractDomain(clientId) || + // 'an application') doesn't end up surfacing the literal raw + // string ("Sign in to not-a-url") in page titles and copy. + // The caller's extractDomain will also fail; the final + // fallback to "an application" wins. + return {} } if (parsedUrl.protocol !== 'https:' && parsedUrl.protocol !== 'http:') { - return { client_name: clientId } + // Same shape as the unparseable case: don't surface the raw + // schemeless or non-http value to end users. + return {} } if (!options.noCache) { From 40ad0e0f6c5012cd4ad14d53267c10a508c8f9fa Mon Sep 17 00:00:00 2001 From: Adam Spiers <atproto@adamspiers.org> Date: Thu, 7 May 2026 11:18:34 +0000 Subject: [PATCH 36/37] fix(auth-service): account settings shows success / error banners after every action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every POST handler in /account/* redirects back to /account with a ?success=<code> or ?error=<code> query param to confirm the action took effect. The GET handler was completely ignoring those query params — the user clicked "Add backup email" → server processed it → server redirected back → user landed on the same page with zero indication anything had happened. Wire the params through to renderSettingsPage and render a green flash banner on success, red on error. Whitelist the recognised codes via FLASH_SUCCESS_MESSAGES / FLASH_ERROR_MESSAGES (both exported, both unit-tested) so an attacker can't craft a URL like `?error=Some+raw+text+to+display` to inject arbitrary copy. Banners go through escapeHtml on render too as defence in depth. Also added two missing redirects: - POST /account/backup-email/remove → ?success=backup_removed - POST /account/sessions/revoke → ?success=session_revoked Tests: new account-settings-flash.test.ts asserts every code redirected to from a POST handler has a matching entry, plus the unknown-code → undefined safety test and the no-HTML test on the message values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --- .changeset/account-settings-flash-messages.md | 9 +++ .../__tests__/account-settings-flash.test.ts | 66 ++++++++++++++++ .../src/routes/account-settings.ts | 77 ++++++++++++++++++- 3 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 .changeset/account-settings-flash-messages.md create mode 100644 packages/auth-service/src/__tests__/account-settings-flash.test.ts diff --git a/.changeset/account-settings-flash-messages.md b/.changeset/account-settings-flash-messages.md new file mode 100644 index 00000000..ed76ab31 --- /dev/null +++ b/.changeset/account-settings-flash-messages.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Account Settings now confirms every action with a visible banner. + +**Affects:** End users + +**End users:** when you added a backup email, removed one, changed your handle, revoked a session, or hit a validation error on any of those, the page silently bounced back to the same form with no indication that anything had changed (or what went wrong). The page now shows a green "Backup email added" / "Handle updated" / etc. banner on success, and a red "That handle is not available" / "We couldn't send the verification email" / etc. banner on error — so you know whether your action took effect. diff --git a/packages/auth-service/src/__tests__/account-settings-flash.test.ts b/packages/auth-service/src/__tests__/account-settings-flash.test.ts new file mode 100644 index 00000000..d6481be5 --- /dev/null +++ b/packages/auth-service/src/__tests__/account-settings-flash.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect } from 'vitest' +import { + FLASH_SUCCESS_MESSAGES, + FLASH_ERROR_MESSAGES, +} from '../routes/account-settings.js' + +describe('account-settings flash messages', () => { + // Every code referenced in a redirect from a POST handler needs an + // entry in the matching lookup, otherwise the user lands on the + // settings page with no acknowledgement of the action they took. + // These tests guard against drift — adding a new code without a + // corresponding entry would mean the redirect silently dropped. + + it('has a success message for each code redirected to from a POST handler', () => { + const expectedSuccessCodes = [ + 'backup_added', + 'backup_verified', + 'backup_removed', + 'handle_updated', + 'session_revoked', + ] + for (const code of expectedSuccessCodes) { + expect( + FLASH_SUCCESS_MESSAGES[code], + `missing FLASH_SUCCESS_MESSAGES["${code}"]`, + ).toBeTruthy() + } + }) + + it('has an error message for each code redirected to from a POST handler', () => { + const expectedErrorCodes = [ + 'invalid_email', + 'already_primary', + 'account_not_found', + 'send_failed', + 'verify_failed', + 'invalid_handle', + 'handle_failed', + 'handle_taken', + 'delete_failed', + 'confirm_delete', + ] + for (const code of expectedErrorCodes) { + expect( + FLASH_ERROR_MESSAGES[code], + `missing FLASH_ERROR_MESSAGES["${code}"]`, + ).toBeTruthy() + } + }) + + it('returns undefined for unknown codes — the GET handler treats this as "no banner"', () => { + expect(FLASH_SUCCESS_MESSAGES['attacker_injected_text']).toBeUndefined() + expect(FLASH_ERROR_MESSAGES['<script>alert(1)</script>']).toBeUndefined() + }) + + it('all messages are plain text, no HTML', () => { + // The renderer escapeHtml's the lookup result anyway (defence in + // depth) but the values themselves shouldn't carry markup. + for (const msg of Object.values(FLASH_SUCCESS_MESSAGES)) { + expect(msg).not.toMatch(/<[a-z]/i) + } + for (const msg of Object.values(FLASH_ERROR_MESSAGES)) { + expect(msg).not.toMatch(/<[a-z]/i) + } + }) +}) diff --git a/packages/auth-service/src/routes/account-settings.ts b/packages/auth-service/src/routes/account-settings.ts index 8923039a..5406c4d5 100644 --- a/packages/auth-service/src/routes/account-settings.ts +++ b/packages/auth-service/src/routes/account-settings.ts @@ -16,6 +16,46 @@ import { POWERED_BY_CSS, POWERED_BY_HTML } from '../lib/page-helpers.js' const logger = createLogger('auth:account-settings') +/** + * User-visible flash-message lookup tables for the GET /account + * page. The POST handlers redirect back with `?success=<code>` or + * `?error=<code>` and the GET handler renders the matching message. + * + * Whitelisted lookup (not `?success=Some+raw+text`) so attackers + * can't inject arbitrary text into the settings page via a crafted + * URL. + * + * Exported for unit testing. + */ +export const FLASH_SUCCESS_MESSAGES: Record<string, string> = { + backup_added: + 'Backup email added. Click the verification link we sent to confirm.', + backup_verified: 'Backup email verified.', + backup_removed: 'Backup email removed.', + handle_updated: 'Handle updated.', + session_revoked: 'Session revoked.', +} + +export const FLASH_ERROR_MESSAGES: Record<string, string> = { + invalid_email: 'That email address is not valid.', + already_primary: + 'That email is already your primary — you can sign in with it directly.', + account_not_found: "We couldn't find an account for your sign-in.", + send_failed: + "We couldn't send the verification email. Please try again in a moment.", + verify_failed: + 'That verification link is no longer valid. Add the backup email again to receive a fresh link.', + invalid_handle: + "That handle isn't valid. Use 5–20 characters: letters, numbers, or hyphens.", + handle_failed: + "We couldn't change your handle. It may be reserved or already taken.", + delete_failed: + "We couldn't delete your account right now. Please try again in a moment.", + handle_taken: 'That handle is not available — please choose another.', + confirm_delete: + 'Type DELETE in the confirmation box to permanently delete your account.', +} + /** * Middleware that validates a better-auth session and injects it into res.locals. * If not authenticated, redirects to /account/login. @@ -80,6 +120,18 @@ export function createAccountSettingsRouter( logger.warn({ err }, 'Failed to list sessions') } + // The POST handlers below redirect back to /account with a + // ?success=… or ?error=… query param to confirm the action. + // Plumb those through so the user actually sees the result + // ("Backup email added", "Handle changed", …) instead of being + // silently bounced back to the same form. Whitelist the + // recognised codes via FLASH_*_MESSAGES so an attacker can't + // craft a URL that injects arbitrary text into the page. + const successCode = (req.query.success as string | undefined) ?? '' + const errorCode = (req.query.error as string | undefined) ?? '' + const successMessage = FLASH_SUCCESS_MESSAGES[successCode] ?? null + const errorMessage = FLASH_ERROR_MESSAGES[errorCode] ?? null + res.type('html').send( renderSettingsPage({ did: did ?? '(unknown)', @@ -90,6 +142,8 @@ export function createAccountSettingsRouter( sessions, currentSessionToken: session.session.token, csrfToken: res.locals.csrfToken, + successMessage, + errorMessage, }), ) }) @@ -217,7 +271,7 @@ export function createAccountSettingsRouter( if (did && email) { ctx.db.removeBackupEmail(did, email) } - res.redirect(303, '/account') + res.redirect(303, '/account?success=backup_removed') }, ) @@ -237,7 +291,7 @@ export function createAccountSettingsRouter( logger.warn({ err }, 'Failed to revoke session') } } - res.redirect(303, '/account') + res.redirect(303, '/account?success=session_revoked') }, ) @@ -427,7 +481,21 @@ function renderSettingsPage(opts: { }> currentSessionToken: string csrfToken: string + successMessage?: string | null + errorMessage?: string | null }): string { + // Prepend any flash banners INSIDE the existing page-wrap so they + // sit above the settings content. Both go through escapeHtml even + // though the source dictionary is whitelisted — defence-in-depth + // against a future maintainer accidentally widening the lookup. + const flashHtml = [ + opts.successMessage + ? `<div class="flash flash-success" role="status">${escapeHtml(opts.successMessage)}</div>` + : '', + opts.errorMessage + ? `<div class="flash flash-error" role="alert">${escapeHtml(opts.errorMessage)}</div>` + : '', + ].join('') const backupRows = opts.backupEmails .map( (be) => ` @@ -489,6 +557,8 @@ function renderSettingsPage(opts: { </form> </div> + ${flashHtml} + <section class="section"> <h2>Account</h2> <div class="setting-row"><strong>DID:</strong> <code>${escapeHtml(opts.did)}</code></div> @@ -589,6 +659,9 @@ const SETTINGS_CSS = ` .header { display: flex; justify-content: space-between; align-items: center; margin-bottom: 24px; border-bottom: 1px solid #eee; padding-bottom: 16px; } h1 { font-size: 24px; color: #111; } h2 { font-size: 18px; color: #333; margin-bottom: 12px; } + .flash { padding: 12px 16px; border-radius: 8px; margin-bottom: 20px; font-size: 14px; } + .flash-success { background: #f0fff4; color: #28a745; border: 1px solid #c3e6cb; } + .flash-error { background: #fdf0f0; color: #dc3545; border: 1px solid #f5c6cb; } .section { margin-bottom: 28px; padding-bottom: 20px; border-bottom: 1px solid #f0f0f0; } .section:last-child { border-bottom: none; margin-bottom: 0; } .setting-row { display: flex; justify-content: space-between; align-items: center; padding: 8px 0; font-size: 14px; color: #333; } From 473f6a066ab99941048c45479df5927e0ce6ab2e Mon Sep 17 00:00:00 2001 From: Adam Spiers <atproto@adamspiers.org> Date: Thu, 7 May 2026 12:04:20 +0000 Subject: [PATCH 37/37] refactor(auth-service): extract flash-resolver from /account route handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GET /account handler open-coded the success/error code → message lookup in 4 lines. Lift that into a pure helper (resolveAccountFlashFromQuery) so: - Coverage tooling can see the lookup branches without an integration test (route handler stays at the same coverage level, but the lookup itself is now fully covered). - Future maintainers swapping which params plumb through don't have to re-derive the safety logic. - Non-string query values (?success=foo&success=bar arrays, numeric values) are handled explicitly rather than relying on Express type coercion. 5 new unit tests covering known codes, empty query, unknown codes (URL-injection safety), and non-string query values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --- .../__tests__/account-settings-flash.test.ts | 43 +++++++++++++++++++ .../src/routes/account-settings.ts | 35 +++++++++++---- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/packages/auth-service/src/__tests__/account-settings-flash.test.ts b/packages/auth-service/src/__tests__/account-settings-flash.test.ts index d6481be5..5de5785d 100644 --- a/packages/auth-service/src/__tests__/account-settings-flash.test.ts +++ b/packages/auth-service/src/__tests__/account-settings-flash.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from 'vitest' import { FLASH_SUCCESS_MESSAGES, FLASH_ERROR_MESSAGES, + resolveAccountFlashFromQuery, } from '../routes/account-settings.js' describe('account-settings flash messages', () => { @@ -64,3 +65,45 @@ describe('account-settings flash messages', () => { } }) }) + +describe('resolveAccountFlashFromQuery', () => { + it('returns the success message when the success code is known', () => { + const result = resolveAccountFlashFromQuery({ success: 'backup_added' }) + expect(result.successMessage).toBe(FLASH_SUCCESS_MESSAGES.backup_added) + expect(result.errorMessage).toBeNull() + }) + + it('returns the error message when the error code is known', () => { + const result = resolveAccountFlashFromQuery({ + success: '', + error: 'invalid_handle', + }) + expect(result.successMessage).toBeNull() + expect(result.errorMessage).toBe(FLASH_ERROR_MESSAGES.invalid_handle) + }) + + it('returns null on both sides when the query is empty', () => { + expect(resolveAccountFlashFromQuery({})).toEqual({ + successMessage: null, + errorMessage: null, + }) + }) + + it('returns null on both sides for unknown codes (safety against URL-injection of attacker text)', () => { + expect( + resolveAccountFlashFromQuery({ + success: 'attacker_chosen_text', + error: '<script>alert(1)</script>', + }), + ).toEqual({ successMessage: null, errorMessage: null }) + }) + + it('ignores non-string query values gracefully (e.g. ?success=foo&success=bar arrays)', () => { + expect( + resolveAccountFlashFromQuery({ + success: ['backup_added', 'backup_removed'], + error: 12345, + }), + ).toEqual({ successMessage: null, errorMessage: null }) + }) +}) diff --git a/packages/auth-service/src/routes/account-settings.ts b/packages/auth-service/src/routes/account-settings.ts index 5406c4d5..90f34de7 100644 --- a/packages/auth-service/src/routes/account-settings.ts +++ b/packages/auth-service/src/routes/account-settings.ts @@ -56,6 +56,27 @@ export const FLASH_ERROR_MESSAGES: Record<string, string> = { 'Type DELETE in the confirmation box to permanently delete your account.', } +/** + * Resolve the success / error message pair from a /account request's + * query params. Whitelist the keys via the FLASH_*_MESSAGES tables so + * an attacker can't craft a URL like + * `/account?error=Some+raw+text` to inject arbitrary copy. + * + * Exported so the route handler can stay declarative AND be unit- + * tested without needing a fake express request. + */ +export function resolveAccountFlashFromQuery(query: { + success?: unknown + error?: unknown +}): { successMessage: string | null; errorMessage: string | null } { + const successCode = typeof query.success === 'string' ? query.success : '' + const errorCode = typeof query.error === 'string' ? query.error : '' + return { + successMessage: FLASH_SUCCESS_MESSAGES[successCode] ?? null, + errorMessage: FLASH_ERROR_MESSAGES[errorCode] ?? null, + } +} + /** * Middleware that validates a better-auth session and injects it into res.locals. * If not authenticated, redirects to /account/login. @@ -122,15 +143,11 @@ export function createAccountSettingsRouter( // The POST handlers below redirect back to /account with a // ?success=… or ?error=… query param to confirm the action. - // Plumb those through so the user actually sees the result - // ("Backup email added", "Handle changed", …) instead of being - // silently bounced back to the same form. Whitelist the - // recognised codes via FLASH_*_MESSAGES so an attacker can't - // craft a URL that injects arbitrary text into the page. - const successCode = (req.query.success as string | undefined) ?? '' - const errorCode = (req.query.error as string | undefined) ?? '' - const successMessage = FLASH_SUCCESS_MESSAGES[successCode] ?? null - const errorMessage = FLASH_ERROR_MESSAGES[errorCode] ?? null + // resolveAccountFlashFromQuery whitelists the recognised codes + // so an attacker can't craft a URL that injects arbitrary text. + const { successMessage, errorMessage } = resolveAccountFlashFromQuery( + req.query as { success?: unknown; error?: unknown }, + ) res.type('html').send( renderSettingsPage({