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..3439d940 --- /dev/null +++ b/.changeset/dont-flash-invalid-otp-on-empty-submit.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Verify stays disabled until the code is complete, and incomplete submits no longer flash "Invalid OTP". + +**Affects:** End users + +**End users:** clicking **Verify** before typing the whole 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 **Verify** button is now greyed out until every box is filled, so it's clear up front that there's nothing to submit yet. If a submit still reaches the form another way (Enter, or your password manager autofilling), it's ignored: the cursor moves to the first empty box and no fake error appears. diff --git a/AGENTS.md b/AGENTS.md index 11c28450..e92ba34b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -131,6 +131,11 @@ The e2e suite uses two demo OAuth clients (trusted and untrusted) for trust-gated scenarios. See [`e2e/README.md`](e2e/README.md#two-demo-clients) for the full setup, tagging conventions, and step-definition patterns. +Cucumber tags are selectors for grouping or filtering scenarios, not +per-scenario identifiers. Add a tag only when it is reused by multiple +scenarios as a meaningful group or consumed by CI/tooling. Do not invent a +one-off tag merely to describe a scenario; use its name and comments instead. + ### Writing Tests Before designing or writing new tests, read diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 71abc40f..86cb1083 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -1,5 +1,5 @@ import { Given, Then, When } from '@cucumber/cucumber' -import { expect, type Route } from '@playwright/test' +import { expect, type Request, type Route } from '@playwright/test' import { testEnv } from '../support/env.js' import type { EpdsWorld } from '../support/world.js' import { @@ -969,3 +969,80 @@ 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 }) }) + +// --------------------------------------------------------------------------- +// Incomplete-OTP submit guard +// --------------------------------------------------------------------------- + +/** + * Fill `digitCount` OTP slots, then try to submit anyway. + * + * Verify is disabled while the code is incomplete, so a real click cannot + * reach the handler — Playwright would just wait for the button to become + * actionable. Submitting the form directly bypasses the disabled button and + * exercises the JS completeness guard, which is the layer that also covers + * Enter and autofill-triggered submits. + */ +async function attemptSubmitWithIncompleteOtp( + world: EpdsWorld, + digitCount: number, +): Promise { + const page = getPage(world) + const otpBoxes = page.locator('.otp-box') + for (let index = 0; index < digitCount; index += 1) { + await otpBoxes.nth(index).fill(String(index + 1)) + } + + let verifyRequestCount = 0 + const countVerifyRequest = (request: Request) => { + if (request.url().includes('/sign-in/email-otp')) { + verifyRequestCount += 1 + } + } + page.on('request', countVerifyRequest) + try { + await page.evaluate(() => { + const form = document.getElementById('form-verify-otp') + if (form instanceof HTMLFormElement) form.requestSubmit() + }) + await page.waitForTimeout(1_000) + } finally { + page.off('request', countVerifyRequest) + } + world.otpVerifyRequestCount = verifyRequestCount +} + +When( + 'the user tries to submit the OTP form without entering a code', + async function (this: EpdsWorld) { + await attemptSubmitWithIncompleteOtp(this, 0) + }, +) + +When( + 'the user enters two OTP digits and tries to submit the OTP form', + async function (this: EpdsWorld) { + await attemptSubmitWithIncompleteOtp(this, 2) + }, +) + +Then('the Verify button is disabled', async function (this: EpdsWorld) { + const page = getPage(this) + await expect( + page.locator('#form-verify-otp button[type=submit]'), + ).toBeDisabled({ timeout: 5_000 }) +}) + +Then('no OTP verification request is sent', function (this: EpdsWorld) { + expect(this.otpVerifyRequestCount).toBe(0) +}) + +Then('no "Invalid OTP" error is shown', async function (this: EpdsWorld) { + const page = getPage(this) + const errorText = await page.locator('#error-msg').textContent() + if (errorText && /invalid otp/i.test(errorText)) { + throw new Error( + `Expected no "Invalid OTP" error after incomplete submit but saw: "${errorText}"`, + ) + } +}) diff --git a/e2e/support/world.ts b/e2e/support/world.ts index 41e4c1fe..a426b98f 100644 --- a/e2e/support/world.ts +++ b/e2e/support/world.ts @@ -10,6 +10,9 @@ export class EpdsWorld extends World { /** OTP code extracted from the most recent email — set by email steps, read by auth steps. */ otpCode?: string + /** Number of verification requests observed after an incomplete OTP submit attempt. */ + otpVerifyRequestCount?: number + /** Subject line of the most recent email — set by email steps. */ lastEmailSubject?: string diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 09858b62..28cab7f4 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -403,6 +403,37 @@ Feature: Passwordless authentication via email OTP When the user clicks "Use different email" Then the email input is empty and focused + # Submitting before the code was complete used to flash "Invalid OTP", + # which is dishonest: the user had not entered an invalid full code. It also + # burned a real call to better-auth's /sign-in/email-otp endpoint, which + # counts against the rate-limiter. Verify is now disabled until every slot + # is filled, and the submit handler still guards the paths that bypass the + # button (Enter, autofill). Cover both the empty and partial states. + @email @verify-incomplete-otp + Scenario: Verify is disabled and an empty submit sends no verification request + 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 + And the Verify button is disabled + When the user tries to submit the OTP form without entering a code + Then no OTP verification request is sent + And no "Invalid OTP" error is shown + And the Verify button is disabled + + @email @verify-incomplete-otp + Scenario: Verify stays disabled and a partial submit sends no verification request + 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 enters two OTP digits and tries to submit the OTP form + Then no OTP verification request is sent + And no "Invalid OTP" error is shown + And the Verify button is disabled + @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__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 01a8784c..8805207b 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -1006,3 +1006,79 @@ describe('renderLoginPage link-affordance convention', () => { } }) }) + +describe('renderLoginPage incomplete-OTP guard', () => { + // Two layers: Verify renders disabled and only enables once every slot + // is filled (so an incomplete code is visibly un-submittable), and the + // submit handler still bails on a short code for the paths that reach + // submit without the button (Enter, autofill, requestSubmit). + + it('renders the Verify button disabled', () => { + const html = renderDefault() + expect(html).toContain( + '', + ) + }) + + it('styles the disabled button as inert without breaking contrast', () => { + const html = renderDefault() + // 0.6 is the floor that keeps the white label on the default brand + // colour at ~4.9:1 (WCAG AA); fading further makes it unreadable. + expect(html).toContain('.btn-primary:disabled { opacity: 0.6;') + // Hover must not brighten a disabled button — that reads as clickable. + expect(html).toContain('.btn-primary:hover:not(:disabled)') + }) + + it('drives the button state from the filled-slot count', () => { + const html = renderDefault() + const fnStart = html.indexOf('function syncVerifyButtonState()') + expect(fnStart).toBeGreaterThan(0) + const fnEnd = html.indexOf('function updateHiddenCode', fnStart) + expect(fnEnd).toBeGreaterThan(fnStart) + const fnBody = html.slice(fnStart, fnEnd) + expect(fnBody).toMatch( + /verifyBtn\.disabled = hiddenCode\.value\.length < otpBoxes\.length/, + ) + }) + + it('never re-enables Verify mid-verify or after the flow is aborted', () => { + const html = renderDefault() + const fnStart = html.indexOf('function syncVerifyButtonState()') + const fnEnd = html.indexOf('function updateHiddenCode', fnStart) + const fnBody = html.slice(fnStart, fnEnd) + // Those owners disabled the button for reasons unrelated to length; + // re-enabling would resurrect a control they deliberately killed. + expect(fnBody).toMatch(/if \(verifying \|\| flowAborted\) return/) + }) + + it('syncs the button whenever the code value changes', () => { + const html = renderDefault() + for (const fn of [ + 'function updateHiddenCode()', + 'function clearOtpBoxes()', + ]) { + const fnStart = html.indexOf(fn) + expect(fnStart).toBeGreaterThan(0) + const fnBody = html.slice(fnStart, html.indexOf('}', fnStart)) + expect(fnBody).toContain('syncVerifyButtonState()') + } + }) + + it('keeps the submit-handler completeness guard as a backstop', () => { + const html = renderDefault() + const handlerStart = html.indexOf("'form-verify-otp').addEventListener") + const handlerBody = html.slice(handlerStart) + // Enter / autofill / requestSubmit reach submit without the button. + expect(handlerBody).toMatch(/if \(otp\.length < otpBoxes\.length\)/) + }) + + it('re-enables via the sync helper after a failed verify, not blindly', () => { + const html = renderDefault() + const handlerStart = html.indexOf("'form-verify-otp').addEventListener") + const handlerBody = html.slice(handlerStart) + // The error path clears the boxes, so an unconditional + // btn.disabled = false would re-enable Verify on an empty code. + expect(handlerBody).not.toMatch(/btn\.disabled = false/) + expect(handlerBody).toContain('syncVerifyButtonState()') + }) +}) diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 5bfb41ba..e59cc444 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -630,9 +630,13 @@ export function renderLoginPage(opts: { .otp-box:focus { border-color: var(--focus-border); } .otp-actions { display: flex; gap: 32px; justify-content: center; margin-top: 12px; } .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:hover:not(:disabled) { 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; } + /* Verify sits disabled until the code is complete, so this is a resting + state the user reads, not just a momentary "Verifying..." flash. Fade + it enough to read as inert, but not past the point where the label + stops meeting WCAG AA (0.6 keeps white-on-brand at ~4.9:1). */ + .btn-primary:disabled { opacity: 0.6; cursor: not-allowed; } /* Link-affordance convention (see also .flash-action / .terms-link): STANDALONE actions sit in their own row, where position and spacing already read as actionable, so they carry no underline. IN-SENTENCE @@ -741,7 +745,7 @@ export function renderLoginPage(opts: { typing — instead of above the subtitle, and places the inline Resend action next to it. -->
${opts.initialStep === 'otp' ? flashRegionHtml : ''}
- +
@@ -1004,15 +1008,31 @@ export function renderLoginPage(opts: { return otpCharset === 'alphanumeric' ? cleaned.toUpperCase() : cleaned; } + // Keep Verify disabled until every slot is filled, so an incomplete + // code is visibly un-submittable rather than a click that silently + // does nothing. The submit handler keeps its own completeness guard: + // Enter, autofill and requestSubmit() can all reach submit without + // the button, so this is defence in depth, not a replacement. + function syncVerifyButtonState() { + var verifyBtn = document.querySelector('#form-verify-otp button[type=submit]'); + if (!verifyBtn) return; + // Never re-enable while a verify is in flight or the flow is dead — + // those owners disabled the button for reasons unrelated to length. + if (verifying || flowAborted) return; + verifyBtn.disabled = hiddenCode.value.length < otpBoxes.length; + } + function updateHiddenCode() { var v = ''; for (var i = 0; i < otpBoxes.length; i++) v += otpBoxes[i].value; hiddenCode.value = v; + syncVerifyButtonState(); } function clearOtpBoxes() { for (var i = 0; i < otpBoxes.length; i++) otpBoxes[i].value = ''; hiddenCode.value = ''; + syncVerifyButtonState(); } otpBoxes.forEach(function(box, idx) { @@ -1346,12 +1366,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...'; @@ -1444,8 +1474,11 @@ export function renderLoginPage(opts: { // and we don't want late events to re-open the form mid-navigation. if (!result || result.error) { verifying = false; - btn.disabled = false; btn.textContent = 'Verify'; + // Re-enable only if the code is still complete. The error path + // above clears the boxes, so this normally leaves Verify + // disabled until the user re-enters a full code. + syncVerifyButtonState(); // Verify failed (typo, expired OTP, etc.) — the form is // still live, so resume keeping the PAR alive. startHeartbeat(); @@ -1498,6 +1531,11 @@ export function renderLoginPage(opts: { var initialStep = ${JSON.stringify(opts.initialStep)}; var otpAlreadySent = ${JSON.stringify(opts.otpAlreadySent)}; + // Reconcile Verify with whatever is actually in the boxes at load — + // a back-navigation or bfcache restore can repopulate them without + // firing input, which would otherwise strand the button disabled. + updateHiddenCode(); + if (initialStep === 'otp' && loginHint) { currentEmail = loginHint; var masked = loginHint.replace(/(.{2})[^@]*(@.*)/, '$1***$2');