diff --git a/.changeset/otp-verify-double-submit-flash.md b/.changeset/otp-verify-double-submit-flash.md new file mode 100644 index 00000000..b2f8b333 --- /dev/null +++ b/.changeset/otp-verify-double-submit-flash.md @@ -0,0 +1,15 @@ +--- +'ePDS': patch +--- + +A smoother sign-in code experience: no false error flash on a successful sign-in, no rapid-fire failures when correcting a wrong code, and tidier-looking banners. + +**Affects:** End users, Client app developers + +**End users:** + +- A successful sign-in no longer briefly shows a red "Invalid OTP" message on its way to signing you in. +- After entering a wrong code, the boxes clear and focus jumps back to the first one, so retyping doesn't immediately resubmit the still-wrong code on every keystroke (which previously could lock you out for spamming the server). +- The red "Invalid OTP" and green "Code resent" banners are centred inside their coloured container instead of sitting in the corner of an empty wide box. + +**Client app developers:** the sign-in page's flash-message container now uses a stable `flash-msg` base class with `error` / `success` modifier classes, so custom client CSS can restyle either variant cleanly via `.flash-msg`, `.flash-msg.error`, and `.flash-msg.success`. diff --git a/packages/auth-service/src/__tests__/login-page.test.ts b/packages/auth-service/src/__tests__/login-page.test.ts index 420bfd66..4c28a586 100644 --- a/packages/auth-service/src/__tests__/login-page.test.ts +++ b/packages/auth-service/src/__tests__/login-page.test.ts @@ -485,3 +485,92 @@ describe('renderLoginPage handle login button', () => { expect(html).not.toContain(BUTTON_HTML) }) }) + +// Regression: the segmented OTP input auto-submits the verify form when the +// last digit lands (paste handler at the same site). If a second submit +// fires while the first is in flight — Enter after typing, OTP autofill +// dispatching input on every box, paste+input pair on some browsers — the +// second call hits /sign-in/email-otp with a now-consumed code, the +// response is "Invalid OTP", and that error renders briefly before the +// success-path redirect to /auth/complete unloads the page. The visible +// symptom is a red "Invalid OTP" flash followed by a successful login. +// +// The fix is an in-flight latch in the verify-form submit handler. These +// tests pin its structure so accidental refactors (removing the guard, +// moving it after the fetch, resetting the flag unconditionally on +// success) fail loudly. +function renderDefault(): string { + return renderLoginPage({ + flowId: 'flow-1', + clientId: 'https://example.com/client-metadata.json', + clientName: 'Example', + branding: {}, + customCss: null, + customFaviconUrl: null, + customFaviconUrlDark: null, + loginHint: '', + initialStep: 'email', + otpAlreadySent: false, + csrfToken: 'csrf', + authBasePath: '/api/auth', + pdsPublicUrl: 'https://pds.example.com', + otpLength: 6, + otpCharset: 'numeric', + }) +} + +describe('renderLoginPage OTP verify-form double-submit latch (regression)', () => { + it('declares the verifying flag at IIFE scope so input/paste/submit handlers share it', () => { + const html = renderDefault() + expect(html).toContain('var verifying = false;') + // Exactly one declaration — a second one would shadow the shared flag. + expect(html.match(/var verifying =/g)).toHaveLength(1) + }) + + it('guards the verify-form submit handler before any in-flight state is touched', () => { + const html = renderDefault() + // Order matters: the guard must short-circuit BEFORE we set + // verifying=true and BEFORE the verifyOtp() call. A guard placed + // after the fetch would not prevent a second request. + const guardIdx = html.indexOf('if (verifying) return;') + const setTrueIdx = html.indexOf('verifying = true;') + const verifyCallIdx = html.indexOf('await verifyOtp(currentEmail, otp)') + expect(guardIdx).toBeGreaterThan(0) + expect(setTrueIdx).toBeGreaterThan(guardIdx) + expect(verifyCallIdx).toBeGreaterThan(setTrueIdx) + }) + + it('resets the latch only on the error path, not on success', () => { + const html = renderDefault() + // The reset is wrapped in `if (!result || result.error) { ... }`. An + // unconditional reset would re-open the form during the post-success + // navigation and let a late input/Enter event fire a second verify + // on the consumed OTP — exactly the bug being prevented. + expect(html).toMatch( + /if \(!result \|\| result\.error\)\s*\{\s*verifying = false;/, + ) + // And there is exactly one place that sets the flag back to false (in + // the error branch). A second `verifying = false` somewhere else + // would defeat the latch. + const resetCount = html.split('verifying = false;').length - 1 + expect(resetCount).toBe(2) // initial declaration + single reset + }) + + it('clears the OTP boxes on verify error so re-entry does not auto-spam', () => { + const html = renderDefault() + // Without clearing, the boxes stay full at length 6 after an invalid + // code. The auto-submit handler fires whenever total length === 6, so + // the next keystroke (replacing one wrong digit) would immediately + // trigger another verify, again with a still-wrong code, on every + // edit — easily tripping the per-IP rate limiter. + const branchStart = html.indexOf('if (result && result.error) {') + expect(branchStart).toBeGreaterThan(0) + // The next `verifying = false;` (in the `finally` block) bounds the + // error branch — bounded slice, no unbounded regex backtracking. + const branchEnd = html.indexOf('verifying = false;', branchStart) + expect(branchEnd).toBeGreaterThan(branchStart) + const branch = html.slice(branchStart, branchEnd) + expect(branch).toContain('showError(result.error);') + expect(branch).toContain('clearOtpBoxes();') + }) +}) diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index a8be0fd6..62a3e959 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -571,7 +571,9 @@ export function renderLoginPage(opts: { .btn-atproto { margin-top: 12px; margin-bottom: 0; color: #1A130F !important; background: var(--input-bg) !important; border-color: var(--input-border) !important; } .divider { display: flex; align-items: center; gap: 12px; margin: 20px 0; color: #999; font-size: 13px; } .divider::before, .divider::after { content: ''; flex: 1; height: 1px; background: #ececec; } - .error { color: #dc3545; background: #fdf0f0; padding: 12px; border-radius: 10px; margin: 12px 0; font-size: 14px; text-align: left; } + .flash-msg { padding: 12px; border-radius: 10px; margin: 12px 0; font-size: 14px; text-align: center; } + .flash-msg.error { color: #dc3545; background: #fdf0f0; } + .flash-msg.success { color: #28a745; background: #f0fff4; } .step-otp { display: none; } .step-otp.active { display: block; } .step-email.hidden { display: none; } @@ -593,7 +595,7 @@ export function renderLoginPage(opts: { ${logoHtml}