P2: fix(auth): filter unsupported OTP characters

.changeset/charset-filter-on-otp-forms.md

@@ -0,0 +1,9 @@
+---
+'ePDS': patch
+---
+
+Sign-in code boxes now ignore stray characters pasted or typed alongside your code.
+
+**Affects:** End users
+
+**End users:** every screen that asks for a sign-in code — the main sign-in screen, Account Settings sign-in, and account recovery — now drops characters your code can't contain, so a code pasted with surrounding quotes, brackets, or a trailing full stop is accepted instead of rejected. The main sign-in screen previously ignored only spaces and line breaks. Where the operator has turned on codes that mix letters and numbers, typing them in lowercase now works too.

e2e/step-definitions/otp-character-filtering.steps.ts

@@ -0,0 +1,105 @@
+import { Then, When } from '@cucumber/cucumber'
+import { expect } from '@playwright/test'
+import { testEnv } from '../support/env.js'
+import { getPage } from '../support/utils.js'
+import type { EpdsWorld } from '../support/world.js'
+
+When(
+  'the recovery OTP preview uses the {string} character policy',
+  async function (this: EpdsWorld, charset: string) {
+    await getPage(this).goto(
+      `${testEnv.authUrl}/preview/recovery-otp?otp_charset=${encodeURIComponent(charset)}`,
+    )
+  },
+)
+
+When(
+  'the user types {string} into the recovery OTP input',
+  async function (this: EpdsWorld, value: string) {
+    await getPage(this).getByLabel('One-time code').fill(value)
+  },
+)
+
+Then(
+  'the recovery OTP input contains {string}',
+  async function (this: EpdsWorld, expected: string) {
+    await expect(getPage(this).getByLabel('One-time code')).toHaveValue(
+      expected,
+    )
+  },
+)
+
+// ---------------------------------------------------------------------------
+// Segmented sign-in grid
+//
+// The grid is a different code path from the two single-input forms: its
+// filtering lives in JS input/paste handlers rather than an oninput
+// attribute, and the paste handler additionally spreads the cleaned
+// characters across boxes. Neither behaviour is observable in the rendered
+// HTML, so only a browser-level test can catch a regression there.
+// ---------------------------------------------------------------------------
+
+When(
+  'the sign-in OTP preview uses the {string} character policy',
+  async function (this: EpdsWorld, charset: string) {
+    const page = getPage(this)
+    await page.goto(
+      `${testEnv.authUrl}/preview/login-otp?otp_charset=${encodeURIComponent(charset)}`,
+    )
+    // The handlers under test are attached by the page's inline script, so
+    // wait for the boxes rather than racing the script with the first fill.
+    await page.locator('.otp-box').first().waitFor({ state: 'visible' })
+  },
+)
+
+When(
+  'the user types {string} into the first sign-in OTP box',
+  async function (this: EpdsWorld, value: string) {
+    await getPage(this).locator('.otp-box').first().fill(value)
+  },
+)
+
+When(
+  'the user pastes {string} into the first sign-in OTP box',
+  async function (this: EpdsWorld, value: string) {
+    // Playwright cannot portably seed the system clipboard, and the handler
+    // reads only event.clipboardData, so synthesise the event directly. This
+    // still drives the real listener, including its filtering and spreading.
+    await getPage(this)
+      .locator('.otp-box')
+      .first()
+      .evaluate((box, pasted) => {
+        const data = new DataTransfer()
+        data.setData('text/plain', pasted)
+        box.dispatchEvent(
+          new ClipboardEvent('paste', {
+            clipboardData: data,
+            bubbles: true,
+            cancelable: true,
+          }),
+        )
+      }, value)
+  },
+)
+
+Then(
+  'the sign-in OTP boxes spell {string}',
+  async function (this: EpdsWorld, expected: string) {
+    const boxes = getPage(this).locator('.otp-box')
+    const count = await boxes.count()
+    if (count < expected.length) {
+      throw new Error(
+        `Expected at least ${expected.length} OTP boxes, found ${count}`,
+      )
+    }
+    // Assert on every box rather than just the filled prefix: a spreading
+    // bug that scattered characters into later boxes would otherwise pass.
+    const padded = expected.padEnd(count, ' ')
+    for (let index = 0; index < count; index += 1) {
+      const character = padded[index]
+      await expect(boxes.nth(index)).toHaveValue(
+        character === ' ' ? '' : character,
+      )
+    }
+  },
+)

features/otp-character-filtering.feature

@@ -0,0 +1,43 @@
+Feature: OTP character filtering
+  The segmented sign-in grid and the recovery form must apply the configured
+  OTP character policy while the user types or pastes. The account-login form
+  applies the same policy, but has no preview route to drive it from a
+  browser, so it is not covered here.
+
+  Background:
+    Given the ePDS test environment is running
+
+  Scenario Outline: Recovery OTP input applies the configured character policy
+    When the recovery OTP preview uses the "<charset>" character policy
+    And the user types "<typed>" into the recovery OTP input
+    Then the recovery OTP input contains "<expected>"
+
+    Examples:
+      | charset      | typed | expected |
+      | numeric      | a1-!  | 1        |
+      | alphanumeric | a1-!  | A1       |
+
+  Scenario Outline: Sign-in OTP grid applies the configured character policy
+    When the sign-in OTP preview uses the "<charset>" character policy
+    And the user types "<typed>" into the first sign-in OTP box
+    Then the sign-in OTP boxes spell "<expected>"
+
+    Examples:
+      | charset      | typed | expected |
+      | numeric      | a     |          |
+      | numeric      | 7     | 7        |
+      | alphanumeric | -     |          |
+      | alphanumeric | b     | B        |
+
+  # The grid's paste handler both filters and distributes across boxes. A
+  # unit test asserting on rendered HTML can observe neither, so this is the
+  # only coverage that would catch a regression in the spreading.
+  Scenario Outline: Pasting into the sign-in OTP grid spreads the kept characters
+    When the sign-in OTP preview uses the "<charset>" character policy
+    And the user pastes "<pasted>" into the first sign-in OTP box
+    Then the sign-in OTP boxes spell "<expected>"
+
+    Examples:
+      | charset      | pasted | expected |
+      | numeric      | 1-2 3a | 123      |
+      | alphanumeric | 1-b 3! | 1B3      |

packages/auth-service/src/__tests__/login-page.test.ts

@@ -827,3 +827,64 @@ describe('renderLoginPage flow-aborted notice + reactive abort gates', () => {
     expect(verifyIdx).toBeGreaterThan(gateIdx)
   })
 })
+
+// The segmented OTP grid used to strip whitespace only, so a code copied
+// with surrounding punctuation, or a letter typed into a digits-only code,
+// reached the server verbatim and failed verification. It also never
+// upper-cased, while alphanumeric codes are generated as A-Z0-9 and the
+// other two OTP forms upper-case on the way in — so a desktop user typing
+// lowercase (where `autocapitalize` does nothing) submitted a code the
+// server would always reject. These pin the shared charset filter.
+describe('renderLoginPage OTP grid charset filter', () => {
+  it('emits the numeric filter from the shared helper', () => {
+    const html = renderDefault({ otpCharset: 'numeric' })
+    expect(html).toContain(`var otpCharFilter = ${/\D/g.toString()};`)
+  })
+
+  it('emits the alphanumeric filter from the shared helper', () => {
+    const html = renderDefault({ otpCharset: 'alphanumeric' })
+    expect(html).toContain(`var otpCharFilter = ${/[^A-Za-z0-9]/g.toString()};`)
+  })
+
+  it('upper-cases only under the alphanumeric policy', () => {
+    expect(renderDefault({ otpCharset: 'alphanumeric' })).toContain(
+      "otpCharset === 'alphanumeric' ? cleaned.toUpperCase() : cleaned",
+    )
+  })
+
+  it('routes the input handler through the filter, not a whitespace-only strip', () => {
+    const html = renderDefault()
+    expect(html).toContain('var v = filterOtpChars(box.value);')
+    // The old whitespace-only strip is a strict subset of every charset
+    // filter; leaving one behind would mean a handler was missed.
+    expect(html).not.toMatch(/replace\(\/\\s\/g, ''\)/)
+  })
+
+  it('routes the paste handler through the filter before slicing to the free boxes', () => {
+    const html = renderDefault()
+    expect(html).toContain(
+      'var cleaned = filterOtpChars(data).slice(0, otpBoxes.length - idx);',
+    )
+  })
+
+  it('defines the filter before the box handlers that call it', () => {
+    const html = renderDefault()
+    const defIdx = html.indexOf('function filterOtpChars(s)')
+    const inputIdx = html.indexOf('var v = filterOtpChars(box.value);')
+    const pasteIdx = html.indexOf('var cleaned = filterOtpChars(data)')
+    expect(defIdx).toBeGreaterThan(0)
+    expect(inputIdx).toBeGreaterThan(defIdx)
+    expect(pasteIdx).toBeGreaterThan(defIdx)
+  })
+
+  it('declares otpCharset once, above the filter that reads it', () => {
+    const html = renderDefault()
+    expect(html.match(/var otpCharset =/g)).toHaveLength(1)
+    expect(html.match(/var otpLength =/g)).toHaveLength(1)
+    // filterOtpChars is only ever called from event handlers, but keeping
+    // the declaration above it removes any reliance on var hoisting.
+    expect(html.indexOf('var otpCharset =')).toBeLessThan(
+      html.indexOf('function filterOtpChars(s)'),
+    )
+  })
+})

packages/auth-service/src/__tests__/otp-input.test.ts

@@ -9,7 +9,11 @@
  * 5. Alphanumeric pattern accepts both letters and digits
  */
 import { describe, it, expect } from 'vitest'
-import { buildOtpInputProps } from '../otp-input.js'
+import {
+  buildOtpInputFilter,
+  buildOtpInputProps,
+  resolvePreviewOtpCharset,
+} from '../otp-input.js'
 
 describe('Recovery flow: OTP input props', () => {
   it('numeric charset produces digit-only pattern and zero placeholder', () => {
@@ -63,3 +67,29 @@ describe('Recovery flow: OTP input props', () => {
     expect(re.test('A1B2C3D')).toBe(false) // one short
   })
 })
+
+describe('Server-rendered OTP character filter', () => {
+  it('removes unsupported characters under the numeric policy', () => {
+    expect('a1-!'.replace(buildOtpInputFilter('numeric'), '')).toBe('1')
+  })
+
+  it('preserves letters and digits under the alphanumeric policy', () => {
+    expect('a1-!'.replace(buildOtpInputFilter('alphanumeric'), '')).toBe('a1')
+  })
+})
+
+describe('Recovery preview: OTP charset override', () => {
+  it.each(['numeric', 'alphanumeric'] as const)(
+    'uses a supported %s override',
+    (requested) => {
+      expect(resolvePreviewOtpCharset(requested, 'numeric')).toBe(requested)
+    },
+  )
+
+  it('falls back to the configured policy for unsupported values', () => {
+    expect(resolvePreviewOtpCharset('unsupported', 'alphanumeric')).toBe(
+      'alphanumeric',
+    )
+    expect(resolvePreviewOtpCharset(undefined, 'numeric')).toBe('numeric')
+  })
+})

packages/auth-service/src/otp-input.ts

@@ -5,6 +5,8 @@
  * logic, and so the logic can be unit-tested without rendering or parsing HTML.
  */
 
+export type OtpCharset = 'numeric' | 'alphanumeric'
+
 export interface OtpInputProps {
   pattern: string
   placeholder: string
@@ -14,7 +16,7 @@ export interface OtpInputProps {
 
 export function buildOtpInputProps(
   otpLength: number,
-  otpCharset: 'numeric' | 'alphanumeric',
+  otpCharset: OtpCharset,
 ): OtpInputProps {
   if (otpCharset === 'alphanumeric') {
     return {
@@ -31,3 +33,18 @@ export function buildOtpInputProps(
     autocapitalize: 'off',
   }
 }
+
+/** Build the character-removal pattern shared by server-rendered OTP inputs. */
+export function buildOtpInputFilter(otpCharset: OtpCharset): RegExp {
+  return otpCharset === 'alphanumeric' ? /[^A-Za-z0-9]/g : /\D/g
+}
+
+/** Accept a supported preview override, otherwise preserve the configured policy. */
+export function resolvePreviewOtpCharset(
+  requested: string | undefined,
+  configured: OtpCharset,
+): OtpCharset {
+  return requested === 'numeric' || requested === 'alphanumeric'
+    ? requested
+    : configured
+}

packages/auth-service/src/routes/account-login.ts

@@ -17,7 +17,7 @@ import { Router, type Request, type Response } from 'express'
 import { escapeHtml, maskEmail, createLogger } from '@certified-app/shared'
 import { fromNodeHeaders } from 'better-auth/node'
 import type { AuthServiceContext } from '../context.js'
-import { buildOtpInputProps } from '../otp-input.js'
+import { buildOtpInputFilter, 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 {
@@ -185,6 +185,7 @@ function renderOtpForm(opts: {
 }): string {
   const maskedEmail = maskEmail(opts.email)
   const inputProps = buildOtpInputProps(opts.otpLength, opts.otpCharset)
+  const inputFilter = buildOtpInputFilter(opts.otpCharset)
 
   return `<!DOCTYPE html>
 <html lang="en">
@@ -216,6 +217,7 @@ function renderOtpForm(opts: {
                  autocapitalize="${inputProps.autocapitalize}"
                  placeholder="${inputProps.placeholder}"
                  class="otp-input"
+                 oninput="this.value=this.value.replace(${inputFilter.toString()},'')${opts.otpCharset === 'alphanumeric' ? '.toUpperCase()' : ''}"
                  style="letter-spacing: ${Math.max(2, Math.round(32 / opts.otpLength))}px">
         </div>
         <button type="submit" class="btn-primary">Verify</button>