From 8ce694ecd36e3c6093ea48f9a65e82da4b303da0 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Jul 2026 16:24:53 +0100 Subject: [PATCH 1/2] fix(auth): ignore incomplete OTP submissions --- .../dont-flash-invalid-otp-on-empty-submit.md | 9 +++ AGENTS.md | 5 ++ e2e/step-definitions/auth.steps.ts | 60 ++++++++++++++++++- e2e/support/world.ts | 3 + features/passwordless-authentication.feature | 26 ++++++++ .../auth-service/src/routes/login-page.ts | 12 +++- 6 files changed, 113 insertions(+), 2 deletions(-) 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..a84d0a93 --- /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 code box and waits for you to type, without bothering anyone with a fake error. 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..5cf9fe19 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,61 @@ 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 +// --------------------------------------------------------------------------- + +async function clickVerifyWithIncompleteOtp( + 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.click('#form-verify-otp button[type=submit]') + await page.waitForTimeout(1_000) + } finally { + page.off('request', countVerifyRequest) + } + world.otpVerifyRequestCount = verifyRequestCount +} + +When( + 'the user clicks the Verify button without entering a code', + async function (this: EpdsWorld) { + await clickVerifyWithIncompleteOtp(this, 0) + }, +) + +When( + 'the user enters two OTP digits and clicks the Verify button', + async function (this: EpdsWorld) { + await clickVerifyWithIncompleteOtp(this, 2) + }, +) + +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..48a769c4 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -403,6 +403,32 @@ Feature: Passwordless authentication via email OTP When the user clicks "Use different email" Then the email input is empty and focused + # Clicking Verify 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. Cover both the empty and partial states. + @email @verify-incomplete-otp + Scenario: Submitting Verify with an empty code does not send a 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 clicks the Verify button without entering a code + Then no OTP verification request is sent + And no "Invalid OTP" error is shown + + @email @verify-incomplete-otp + Scenario: Submitting Verify with a partial code does not send a 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 clicks the Verify button + Then no OTP verification request is sent + And no "Invalid OTP" error is shown + @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 5bfb41ba..db8f1253 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -1346,12 +1346,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 48964db74aef68d4514712d1c00d7061eec8ac71 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Tue, 4 Aug 2026 15:46:23 +0100 Subject: [PATCH 2/2] fix(auth-service): disable Verify until the OTP is complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The incomplete-submit guard silently swallowed the click, so Verify looked live but did nothing until every box was filled. Render the button disabled and drive its state from the filled-slot count, so an incomplete code is visibly un-submittable rather than a dead click. The submit-handler guard stays as a backstop: Enter, autofill and requestSubmit() all reach submit without going through the button. Sync from updateHiddenCode()/clearOtpBoxes() — the two chokepoints where the code value changes — plus once at load, so a bfcache-restored page that repopulates the boxes without firing input can't strand the button disabled. The helper bails while verifying or after the flow is aborted; those owners disabled the button for reasons unrelated to length. That also replaces the unconditional re-enable on the failed-verify path, which would otherwise re-enable Verify over the boxes it just cleared. Drop the disabled button to 0.6 opacity so it reads as inert; 0.6 is the floor that keeps the white label at ~4.9:1 on the default brand colour (WCAG AA). Stop :hover brightening a disabled button, which read as clickable. The e2e steps now submit the form directly rather than clicking, since a disabled button is never actionable, and assert the disabled state. Co-Authored-By: Claude Opus 5 (1M context) --- .../dont-flash-invalid-otp-on-empty-submit.md | 4 +- e2e/step-definitions/auth.steps.ts | 31 ++++++-- features/passwordless-authentication.feature | 17 +++-- .../src/__tests__/login-page.test.ts | 76 +++++++++++++++++++ .../auth-service/src/routes/login-page.ts | 36 ++++++++- 5 files changed, 146 insertions(+), 18 deletions(-) diff --git a/.changeset/dont-flash-invalid-otp-on-empty-submit.md b/.changeset/dont-flash-invalid-otp-on-empty-submit.md index a84d0a93..3439d940 100644 --- a/.changeset/dont-flash-invalid-otp-on-empty-submit.md +++ b/.changeset/dont-flash-invalid-otp-on-empty-submit.md @@ -2,8 +2,8 @@ 'ePDS': patch --- -Empty Verify clicks no longer flash "Invalid OTP". +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 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 code box and waits for you to type, without bothering anyone with a fake error. +**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/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 5cf9fe19..86cb1083 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -974,7 +974,16 @@ Then('the email input is empty and focused', async function (this: EpdsWorld) { // Incomplete-OTP submit guard // --------------------------------------------------------------------------- -async function clickVerifyWithIncompleteOtp( +/** + * 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 { @@ -992,7 +1001,10 @@ async function clickVerifyWithIncompleteOtp( } page.on('request', countVerifyRequest) try { - await page.click('#form-verify-otp button[type=submit]') + 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) @@ -1001,19 +1013,26 @@ async function clickVerifyWithIncompleteOtp( } When( - 'the user clicks the Verify button without entering a code', + 'the user tries to submit the OTP form without entering a code', async function (this: EpdsWorld) { - await clickVerifyWithIncompleteOtp(this, 0) + await attemptSubmitWithIncompleteOtp(this, 0) }, ) When( - 'the user enters two OTP digits and clicks the Verify button', + 'the user enters two OTP digits and tries to submit the OTP form', async function (this: EpdsWorld) { - await clickVerifyWithIncompleteOtp(this, 2) + 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) }) diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index 48a769c4..28cab7f4 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -403,31 +403,36 @@ Feature: Passwordless authentication via email OTP When the user clicks "Use different email" Then the email input is empty and focused - # Clicking Verify before the code was complete used to flash "Invalid OTP", + # 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. Cover both the empty and partial states. + # 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: Submitting Verify with an empty code does not send a verification request + 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 - When the user clicks the Verify button without entering a code + 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: Submitting Verify with a partial code does not send a verification request + 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 clicks the Verify button + 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 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 db8f1253..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) { @@ -1454,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(); @@ -1508,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');