diff --git a/.agents/skills/writing-changesets/SKILL.md b/.agents/skills/writing-changesets/SKILL.md index c7444cb5..d7638c83 100644 --- a/.agents/skills/writing-changesets/SKILL.md +++ b/.agents/skills/writing-changesets/SKILL.md @@ -37,6 +37,12 @@ Skip changesets for: routes under `/_internal/`). These are contributor-facing, not consumer-facing. +Create one changeset per coherent user-facing change, not one per file, +route, or UI surface. Closely related fixes that ship together — such as +the same error-state improvement across sign-in and recovery forms — +share one concise changeset. Split them only when consumers could +meaningfully adopt or release them independently. + If in doubt, add one. An "empty" changeset (intentionally no release) can be created with `pnpm changeset add --empty` to document the decision. diff --git a/.changeset/otp-lockout-recovery.md b/.changeset/otp-lockout-recovery.md new file mode 100644 index 00000000..cd323eda --- /dev/null +++ b/.changeset/otp-lockout-recovery.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +After too many wrong codes, sign-in and account recovery now explain that the old code cannot be used and offer a way to get a fresh one. + +**Affects:** End users + +**End users:** Use **Send a new code** on the sign-in page, or the nearby **Resend code** button on account-management and recovery forms, instead of retrying a code that has been locked out. diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 71abc40f..dc49a123 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -611,7 +611,7 @@ Then( ) Then( - /^the verification form shows (?:an|the) "([^"]*)" error$/, + /^the verification form shows (?:an|the|a) "([^"]*)" error$/, async function (this: EpdsWorld, expected: string) { const page = getPage(this) await expect(page.locator('#error-msg')).toBeVisible({ timeout: 10_000 }) @@ -969,3 +969,70 @@ 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 }) }) + +// --------------------------------------------------------------------------- +// "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). + // Burn five attempts directly, then trigger the sixth through the form + // so the submit handler's error rendering is exercised. + 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 () {})`, + ) + } + 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 }) + await expect(page.locator('#error-msg')).toHaveText( + /\.\s*Send a new code$/i, + ) + }, +) + +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 09858b62..682abe5b 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -386,6 +386,7 @@ 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 @@ -403,6 +404,38 @@ Feature: Passwordless authentication via email OTP When the user clicks "Use different email" Then the email input is empty and focused + # better-auth allows five wrong attempts on one code. The sixth + # submit returns "Too many attempts" and deletes the verification + # row, so the only useful next action is requesting a fresh code. + # The banner asserts the rewritten end-user copy from otpErrorText(), + # not better-auth's raw developer string, because that mapped wording + # is what the user actually reads. + @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 a "Too many tries — that code is no longer usable." error + And a "Send a new code" inline action is offered + + # Once lockout deletes the verification row, later submissions return + # the generic "Invalid OTP" response. Retyping cannot recover that + # state, so the inline action must still be offered on the follow-up + # submit rather than stranding the user on a bare error. + @email @too-many-attempts + 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 + @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/__tests__/otp-verify-error.test.ts b/packages/auth-service/src/__tests__/otp-verify-error.test.ts new file mode 100644 index 00000000..d94c2eac --- /dev/null +++ b/packages/auth-service/src/__tests__/otp-verify-error.test.ts @@ -0,0 +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 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.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', () => { + expect(pickOtpVerifyErrorMessage(new Error('Invalid OTP'))).toBe(TYPO_MSG) + }) + + it('falls back to generic verification-failed on unknown errors', () => { + expect( + pickOtpVerifyErrorMessage(new Error('Internal database problem')), + ).toBe(FALLBACK_MSG) + }) + + it.each([['string-thrown'], [null], [undefined]])( + 'falls back to generic verification-failed on non-Error rejections (%p)', + (err) => { + expect(pickOtpVerifyErrorMessage(err)).toBe(FALLBACK_MSG) + }, + ) +}) 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..c7860ffe --- /dev/null +++ b/packages/auth-service/src/lib/otp-verify-error.ts @@ -0,0 +1,38 @@ +/** + * 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. The current code can no longer succeed, so + * point the user at Resend. + * + * 2. "Invalid OTP" — usually a recoverable typo. better-auth uses + * the same text after deleting a locked-out row, so these stateless + * server-rendered routes cannot distinguish that later case and must + * avoid claiming certainty they do not have. + * + * 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 attempts|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 786b795f..1008d0c1 100644 --- a/packages/auth-service/src/routes/account-login.ts +++ b/packages/auth-service/src/routes/account-login.ts @@ -25,6 +25,7 @@ import { renderEmailTypoGuardMarkup, renderEmailTypoGuardScript, } from '../lib/email-typo-guard.js' +import { pickOtpVerifyErrorMessage } from '../lib/otp-verify-error.js' const logger = createLogger('auth:account-login') @@ -121,17 +122,13 @@ 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.' - : '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: pickOtpVerifyErrorMessage(err), }), ) } diff --git a/packages/auth-service/src/routes/recovery.ts b/packages/auth-service/src/routes/recovery.ts index 70504f41..3f89ab87 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') @@ -268,11 +269,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(