From ff4b1fdee3f56e2e24a2738dee30fb51b7c3e6b6 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 20:30:14 +0200 Subject: [PATCH 1/3] Feature | Recovery code login flow UI (CU-86ba2zp4f) Close the remaining gaps in the recovery-code MFA login mode. - onBackToOtp / onUseRecovery clear recoveryCode and errors.recovery along with the mode switch, so an abandoned attempt is not re-shown when the user toggles back into recovery mode. - Recovery field drops autoComplete="one-time-code": that hint makes the OS offer the e-mailed OTP in the recovery field, which is the wrong credential and the OTP/recovery confusion risk called out in the ticket. Added a format hint and copy that distinguishes it from the e-mailed code. Tests: - New recovery-code-form.test.js: autocomplete, format hint, disabled states, inline error, submit, back vs cancel, raw value handed to parent. - login.mfa.test.js: mode switching + state cleanup, input normalization, empty/in-flight submits, success redirect, low-codes warning (and its already-dismissed variant), invalid/used code, mfa_session_expired, mfa_rate_limit. - E2E TS-005 fixed (it filled a 16-char code that can never exist) and now asserts the dash never reaches the endpoint; new TS-009 back-to-OTP, TS-010 invalid/used code, TS-011 recovery rate limit, TS-012 recovery session expiry. CI seeds mfa-ts-009..012 for them. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/pull_request_frontend_tests.yml | 2 +- .../js/login/components/recovery_code_form.js | 11 +- resources/js/login/login.js | 7 +- tests/e2e/pages/LoginPage.ts | 4 + tests/e2e/tests/auth/login-mfa-flow.spec.ts | 115 ++++++++- .../components/recovery-code-form.test.js | 81 +++++++ tests/js/login/login.mfa.test.js | 227 ++++++++++++++++++ 7 files changed, 437 insertions(+), 10 deletions(-) create mode 100644 tests/js/login/components/recovery-code-form.test.js diff --git a/.github/workflows/pull_request_frontend_tests.yml b/.github/workflows/pull_request_frontend_tests.yml index f2f8df58..d38c2f43 100644 --- a/.github/workflows/pull_request_frontend_tests.yml +++ b/.github/workflows/pull_request_frontend_tests.yml @@ -102,7 +102,7 @@ jobs: php artisan db:seed --force php artisan idp:create-super-admin test@test.com '1Qaz2wsx!' php artisan idp:create-raw-user e2e@test.com '1Qaz2wsx!' - for i in 001 002 003 004 005 006 007 008; do + for i in 001 002 003 004 005 006 007 008 009 010 011 012; do php artisan idp:create-super-admin "mfa-ts-$i@test.com" '1Qaz2wsx!' done php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!' diff --git a/resources/js/login/components/recovery_code_form.js b/resources/js/login/components/recovery_code_form.js index 50830e04..7f54642c 100644 --- a/resources/js/login/components/recovery_code_form.js +++ b/resources/js/login/components/recovery_code_form.js @@ -34,7 +34,8 @@ const RecoveryCodeForm = ({
Enter a recovery code

- Enter one of the recovery codes you saved when you enabled two-step verification. + This is not the code we e-mailed you. Enter one of the recovery codes you saved + when you enabled two-step verification.

- + Back to verification code {" · "} diff --git a/resources/js/login/login.js b/resources/js/login/login.js index 3aa2100c..16b16aee 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -587,15 +587,20 @@ class LoginPage extends React.Component { this.setState({ ...this.state, authFlow: FLOW.RECOVERY, + recoveryCode: "", errors: { ...this.state.errors, recovery: "" }, }); } onBackToOtp() { + // Drop the half-typed recovery code and its error along with the mode: the + // user is abandoning that attempt, and leaving them behind would re-show a + // stale code/error the next time "Use a recovery code instead" is clicked. this.setState({ ...this.state, authFlow: FLOW.MFA, - errors: { ...this.state.errors, twofactor: "" }, + recoveryCode: "", + errors: { ...this.state.errors, twofactor: "", recovery: "" }, }); } diff --git a/tests/e2e/pages/LoginPage.ts b/tests/e2e/pages/LoginPage.ts index d05b4be0..d67f5c37 100644 --- a/tests/e2e/pages/LoginPage.ts +++ b/tests/e2e/pages/LoginPage.ts @@ -21,6 +21,8 @@ export class LoginPage { readonly useRecoveryLink: Locator; // Recovery code step readonly recoveryForm: Locator; + readonly recoveryCodeInput: Locator; + readonly backToOtpLink: Locator; constructor(page: Page) { this.page = page; @@ -38,6 +40,8 @@ export class LoginPage { this.cancelLink = page.locator('[data-testid="cancel-link"]'); this.useRecoveryLink = page.locator('[data-testid="use-recovery-link"]'); this.recoveryForm = page.locator('[data-testid="recovery-form"]'); + this.recoveryCodeInput = page.locator('#recovery_code'); + this.backToOtpLink = page.locator('[data-testid="back-to-otp-link"]'); } async goto() { diff --git a/tests/e2e/tests/auth/login-mfa-flow.spec.ts b/tests/e2e/tests/auth/login-mfa-flow.spec.ts index b817dc6e..909f8b8c 100644 --- a/tests/e2e/tests/auth/login-mfa-flow.spec.ts +++ b/tests/e2e/tests/auth/login-mfa-flow.spec.ts @@ -18,6 +18,11 @@ const CANCEL_URL = '**/auth/login/cancel**'; // across all 8 tests would exhaust the limit well before the suite finishes. const MFA_USER_PASSWORD = '1Qaz2wsx!'; +// Recovery codes are generated as 8 chars from [A-Z0-9] and shown as XXXX-XXXX +// (RecoveryCodeService::regenerateCodesForUser), but hashed without the dash. +const RECOVERY_CODE_AS_DISPLAYED = 'ABCD-1234'; +const RECOVERY_CODE_NORMALIZED = 'ABCD1234'; + function mfaUserEmailFor(testTitle: string): string { const match = testTitle.match(/TS-(\d+)/); if (!match) { @@ -127,7 +132,7 @@ test.describe('MFA Login Flow', () => { }); // TS-005 ───────────────────────────────────────────────────────────────── - test('TS-005: use recovery code — recovery form shown and API called', + test('TS-005: use recovery code — recovery form shown and normalized code posted', async ({ loginPage, page }) => { await page.route(RECOVERY_URL, route => route.fulfill({ status: 200, contentType: 'application/json', body: '{}' }) @@ -137,13 +142,20 @@ test.describe('MFA Login Flow', () => { await expect(loginPage.recoveryForm).toBeVisible(); await expect(loginPage.twoFactorForm).not.toBeVisible(); - await page.locator('#recovery_code').fill('ABCD-1234-EFGH-5678'); + // Typed exactly as the code is displayed to the user (XXXX-XXXX); the + // separator is presentational only and must never reach the endpoint. + await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_AS_DISPLAYED); + await expect(loginPage.recoveryCodeInput).toHaveValue(RECOVERY_CODE_NORMALIZED); - const [response] = await Promise.all([ - page.waitForResponse(RECOVERY_URL), + const [request] = await Promise.all([ + page.waitForRequest(RECOVERY_URL), loginPage.verifyButton.click(), ]); - expect(response.status()).toBe(200); + + // postRawRequest() mirrors every param onto the query string, so the + // normalized value is assertable straight off the request URL. + expect(new URL(request.url()).searchParams.get('recovery_code')) + .toBe(RECOVERY_CODE_NORMALIZED); }); // TS-006 ───────────────────────────────────────────────────────────────── @@ -210,4 +222,97 @@ test.describe('MFA Login Flow', () => { await expect(loginPage.errorLabel).toBeVisible(); await expect(loginPage.errorLabel).toContainText('Too many attempts'); }); + + // TS-009 ───────────────────────────────────────────────────────────────── + test('TS-009: back to verification code — OTP mode restored with a clean recovery field', + async ({ loginPage }) => { + await loginPage.useRecoveryLink.click(); + await expect(loginPage.recoveryForm).toBeVisible(); + + await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED); + await loginPage.backToOtpLink.click(); + + // The OTP flow must be intact — same form, still able to submit a code. + await expect(loginPage.twoFactorForm).toBeVisible(); + await expect(loginPage.recoveryForm).not.toBeVisible(); + await expect(loginPage.passwordForm).not.toBeVisible(); + + // Re-entering recovery mode must not resurrect the abandoned code. + await loginPage.useRecoveryLink.click(); + await expect(loginPage.recoveryCodeInput).toHaveValue(''); + }); + + // TS-010 ───────────────────────────────────────────────────────────────── + test('TS-010: invalid/used recovery code — inline error, still in the recovery form', + async ({ loginPage, page }) => { + // A used code fails exactly like an unknown one: the backend answers + // mfa_invalid_recovery for both (AbstractMFAChallengeStrategy). + await page.route(RECOVERY_URL, route => + route.fulfill({ + status: 401, + contentType: 'application/json', + body: JSON.stringify({ error_code: 'mfa_invalid_recovery' }), + }) + ); + + await loginPage.useRecoveryLink.click(); + await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED); + await loginPage.verifyButton.click(); + + await expect(loginPage.errorLabel).toBeVisible(); + await expect(loginPage.errorLabel).toContainText('Invalid recovery code'); + // The MFA flow must not be abandoned on a bad code. + await expect(loginPage.recoveryForm).toBeVisible(); + await expect(loginPage.passwordForm).not.toBeVisible(); + }); + + // TS-011 ───────────────────────────────────────────────────────────────── + test('TS-011: recovery rate limit — 429 inline error shown', + async ({ loginPage, page }) => { + await page.route(RECOVERY_URL, route => + route.fulfill({ + status: 429, + contentType: 'application/json', + body: JSON.stringify({ + error_code: 'mfa_rate_limit', + error_message: 'Too many attempts. Please try again later.', + }), + }) + ); + + await loginPage.useRecoveryLink.click(); + await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED); + await loginPage.verifyButton.click(); + + await expect(loginPage.errorLabel).toBeVisible(); + await expect(loginPage.errorLabel).toContainText('Too many attempts'); + await expect(loginPage.recoveryForm).toBeVisible(); + }); + + // TS-012 ───────────────────────────────────────────────────────────────── + test('TS-012: recovery session expired — back to password form with warning snackbar', + async ({ loginPage, page }) => { + await page.route(RECOVERY_URL, route => + route.fulfill({ + status: 401, + contentType: 'application/json', + body: JSON.stringify({ error_code: 'mfa_session_expired' }), + }) + ); + // resetToPasswordFlow() fires cancelLogin() in the background; absorb it. + await page.route(CANCEL_URL, route => + route.fulfill({ status: 200, contentType: 'application/json', body: '{}' }) + ); + + await loginPage.useRecoveryLink.click(); + await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED); + await loginPage.verifyButton.click(); + + await expect(loginPage.recoveryForm).not.toBeVisible(); + await expect(loginPage.passwordForm).toBeVisible(); + + const snackbar = page.locator('[role="alert"]'); + await expect(snackbar).toBeVisible(); + await expect(snackbar).toContainText('session has expired'); + }); }); diff --git a/tests/js/login/components/recovery-code-form.test.js b/tests/js/login/components/recovery-code-form.test.js new file mode 100644 index 00000000..c0b8e26f --- /dev/null +++ b/tests/js/login/components/recovery-code-form.test.js @@ -0,0 +1,81 @@ +import React from 'react'; +import { render, screen, fireEvent } from '@testing-library/react'; +import RecoveryCodeForm from '../../../../resources/js/login/components/recovery_code_form'; + +const baseProps = { + recoveryCode: 'ABCD1234', + recoveryError: '', + disableInput: false, + onRecoveryCodeChange: jest.fn(), + onVerify: jest.fn(), + onBackToOtp: jest.fn(), + onCancel: jest.fn(), +}; + +describe('RecoveryCodeForm', () => { + + beforeEach(() => jest.clearAllMocks()); + + it('does not advertise the field as a one-time-code slot', () => { + // "one-time-code" would make the OS offer the e-mailed OTP here — the + // wrong credential for this field, and the OTP/recovery confusion risk. + render(); + expect(document.getElementById('recovery_code')).toHaveAttribute('autocomplete', 'off'); + }); + + it('renders the expected-format hint', () => { + render(); + expect(screen.getByText(/8 characters, shown as ABCD-1234/)).toBeInTheDocument(); + }); + + it('VERIFY button is disabled when the code is empty', () => { + render(); + expect(screen.getByTestId('verify-button')).toBeDisabled(); + }); + + it('VERIFY button is disabled while a submit is in flight', () => { + render(); + expect(screen.getByTestId('verify-button')).toBeDisabled(); + }); + + it('error paragraph renders when recoveryError is non-empty', () => { + const msg = 'Invalid recovery code. Please try again.'; + render(); + expect(screen.getByTestId('error-label')).toHaveTextContent(msg); + }); + + it('submitting the form calls onVerify without navigating', () => { + render(); + fireEvent.submit(screen.getByTestId('recovery-form')); + expect(baseProps.onVerify).toHaveBeenCalledTimes(1); + }); + + it('"Back to verification code" calls onBackToOtp', () => { + render(); + fireEvent.click(screen.getByTestId('back-to-otp-link')); + expect(baseProps.onBackToOtp).toHaveBeenCalledTimes(1); + expect(baseProps.onCancel).not.toHaveBeenCalled(); + }); + + it('"Cancel" calls onCancel', () => { + render(); + fireEvent.click(screen.getByTestId('cancel-link')); + expect(baseProps.onCancel).toHaveBeenCalledTimes(1); + expect(baseProps.onBackToOtp).not.toHaveBeenCalled(); + }); + + it('typing in the field reports the raw value to the parent', () => { + // Normalization lives in LoginPage.onRecoveryCodeChange(); the form must + // hand over the untouched event so that stays the single source of truth. + // Read the value inside the handler: the field is controlled, so React + // resets the DOM node back to the (unchanged) prop before the assertion runs. + let seen = null; + const onRecoveryCodeChange = jest.fn((ev) => { seen = ev.target.value; }); + + render(); + fireEvent.change(document.getElementById('recovery_code'), { target: { value: 'abcd-1234' } }); + + expect(onRecoveryCodeChange).toHaveBeenCalledTimes(1); + expect(seen).toBe('abcd-1234'); + }); +}); diff --git a/tests/js/login/login.mfa.test.js b/tests/js/login/login.mfa.test.js index ca8fdea2..89928a6d 100644 --- a/tests/js/login/login.mfa.test.js +++ b/tests/js/login/login.mfa.test.js @@ -1,5 +1,9 @@ import React from 'react'; import { FLOW, HTTP_CODES, MFA_ERROR_CODE } from '../../../resources/js/login/constants'; +import { + RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, + DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, +} from '../../../resources/js/shared/recovery_codes'; // actions.js makes real XHR calls — stub every export so the module loads cleanly. jest.mock('../../../resources/js/login/actions', () => ({ @@ -18,6 +22,7 @@ delete window.location; window.location = { href: '', reload: jest.fn() }; import { LoginPage } from '../../../resources/js/login/login'; +import { verifyRecoveryCode, cancelLogin } from '../../../resources/js/login/actions'; // ─── Minimal props that satisfy the LoginPage constructor ──────────────────── @@ -73,6 +78,9 @@ function makeError(status, body = null) { return { status, response: body ? { body } : null }; } +/** Let the pending .then()/.catch() callbacks of the mocked action run. */ +const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); + // ───────────────────────────────────────────────────────────────────────────── describe('LoginPage', () => { @@ -138,4 +146,223 @@ describe('LoginPage', () => { expect(inst.showAlert).not.toHaveBeenCalled(); }); }); + + // ─── Recovery-code login flow (CU-86ba2zp4f) ─────────────────────────────── + + describe('recovery code flow', () => { + let inst; + + beforeEach(() => { + inst = makeInstance(); + inst.state = { ...inst.state, authFlow: FLOW.MFA }; + window.location.href = ''; + window.location.reload.mockClear(); + verifyRecoveryCode.mockReset(); + cancelLogin.mockReset(); + sessionStorage.clear(); + }); + + describe('mode switching', () => { + + it('onUseRecovery — switches to the recovery flow with a clean field', () => { + inst.state = { + ...inst.state, + recoveryCode: 'STALE123', + errors: { ...inst.state.errors, recovery: 'Invalid recovery code. Please try again.' }, + }; + + inst.onUseRecovery(); + + expect(inst.state.authFlow).toBe(FLOW.RECOVERY); + expect(inst.state.recoveryCode).toBe(''); + expect(inst.state.errors.recovery).toBe(''); + }); + + it('onBackToOtp — returns to OTP mode and drops the abandoned recovery attempt', () => { + inst.state = { + ...inst.state, + authFlow: FLOW.RECOVERY, + recoveryCode: 'ABCD1234', + errors: { + ...inst.state.errors, + recovery: 'Invalid recovery code. Please try again.', + twofactor: 'Invalid or expired verification code. Please try again.', + }, + }; + + inst.onBackToOtp(); + + expect(inst.state.authFlow).toBe(FLOW.MFA); + expect(inst.state.recoveryCode).toBe(''); + expect(inst.state.errors.recovery).toBe(''); + expect(inst.state.errors.twofactor).toBe(''); + }); + + it('resetToPasswordFlow — clears the recovery code on the way back to password', async () => { + const real = makeInstance(); + real.resetToPasswordFlow = LoginPage.prototype.resetToPasswordFlow.bind(real); + cancelLogin.mockReturnValue(Promise.resolve({})); + real.state = { ...real.state, authFlow: FLOW.RECOVERY, recoveryCode: 'ABCD1234' }; + + real.resetToPasswordFlow(); + await flush(); + + expect(real.state.authFlow).toBe(FLOW.PASSWORD); + expect(real.state.recoveryCode).toBe(''); + expect(real.state.errors.recovery).toBe(''); + }); + }); + + describe('onRecoveryCodeChange', () => { + + it('normalizes the displayed dash and lowercase away', () => { + // Codes are shown as XXXX-XXXX but hashed without the separator. + inst.onRecoveryCodeChange({ target: { value: 'abcd-1234' } }); + expect(inst.state.recoveryCode).toBe('ABCD1234'); + }); + + it('clears a previous inline error as soon as the user edits the field', () => { + inst.state = { + ...inst.state, + errors: { ...inst.state.errors, recovery: 'Invalid recovery code. Please try again.' }, + }; + + inst.onRecoveryCodeChange({ target: { value: 'A' } }); + + expect(inst.state.errors.recovery).toBe(''); + }); + }); + + describe('onVerifyRecovery', () => { + + it('empty code — inline error, no request issued', () => { + inst.state = { ...inst.state, recoveryCode: '' }; + + inst.onVerifyRecovery(); + + expect(verifyRecoveryCode).not.toHaveBeenCalled(); + expect(inst.state.errors.recovery).toBe('Recovery code is empty'); + }); + + it('does not re-submit while a request is already in flight', () => { + inst.state = { ...inst.state, recoveryCode: 'ABCD1234', disableInput: true }; + + inst.onVerifyRecovery(); + + expect(verifyRecoveryCode).not.toHaveBeenCalled(); + }); + + it('valid code — posts the normalized code and navigates to redirect_url', async () => { + verifyRecoveryCode.mockReturnValue( + Promise.resolve({ response: { redirect_url: 'https://idp.test/authorize', recovery_codes_remaining: 7 } }), + ); + inst.state = { ...inst.state, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + expect(verifyRecoveryCode).toHaveBeenCalledWith('ABCD1234', PROPS.token); + await flush(); + + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + }); + + it('valid code with few codes left — shows the warning instead of navigating', async () => { + verifyRecoveryCode.mockReturnValue( + Promise.resolve({ + response: { + redirect_url: 'https://idp.test/authorize', + recovery_codes_remaining: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD - 1, + }, + }), + ); + inst.state = { ...inst.state, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + await flush(); + + expect(window.location.href).toBe(''); + expect(inst.state.lowRecoveryCodesWarning).toEqual({ + remaining: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD - 1, + redirectUrl: 'https://idp.test/authorize', + }); + }); + + it('low-code warning already dismissed this session — navigates straight through', async () => { + sessionStorage.setItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, '1'); + verifyRecoveryCode.mockReturnValue( + Promise.resolve({ response: { redirect_url: 'https://idp.test/authorize', recovery_codes_remaining: 1 } }), + ); + inst.state = { ...inst.state, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + await flush(); + + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + }); + + it('invalid/used code — inline error and the user stays in the recovery flow', async () => { + verifyRecoveryCode.mockReturnValue( + Promise.reject(makeError(HTTP_CODES.UNAUTHORIZED, { error_code: 'mfa_invalid_recovery' })), + ); + inst.state = { ...inst.state, authFlow: FLOW.RECOVERY, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + await flush(); + + expect(inst.state.errors.recovery).toBe('Invalid recovery code. Please try again.'); + expect(inst.state.authFlow).toBe(FLOW.RECOVERY); + expect(inst.state.disableInput).toBe(false); + expect(inst.resetToPasswordFlow).not.toHaveBeenCalled(); + expect(window.location.href).toBe(''); + }); + + it('mfa_session_expired — returns to the password flow with a warning', async () => { + verifyRecoveryCode.mockReturnValue( + Promise.reject( + makeError(HTTP_CODES.UNAUTHORIZED, { error_code: MFA_ERROR_CODE.MFA_SESSION_EXPIRED }), + ), + ); + inst.state = { ...inst.state, authFlow: FLOW.RECOVERY, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + await flush(); + + expect(inst.resetToPasswordFlow).toHaveBeenCalledTimes(1); + expect(inst.showAlert).toHaveBeenCalledWith( + 'Your verification session has expired. Please sign in again.', + 'warning', + ); + }); + + it('mfa_rate_limit — surfaces the server message inline', async () => { + const msg = 'Too many attempts. Please try again later.'; + verifyRecoveryCode.mockReturnValue( + Promise.reject( + makeError(HTTP_CODES.TOO_MANY_REQUESTS, { error_code: 'mfa_rate_limit', error_message: msg }), + ), + ); + inst.state = { ...inst.state, authFlow: FLOW.RECOVERY, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + await flush(); + + expect(inst.state.errors.recovery).toBe(msg); + expect(inst.state.authFlow).toBe(FLOW.RECOVERY); + expect(inst.state.disableInput).toBe(false); + }); + }); + + it('onContinueAfterLowRecoveryCodes — remembers the dismissal and resumes the redirect', () => { + inst.state = { + ...inst.state, + lowRecoveryCodesWarning: { remaining: 1, redirectUrl: 'https://idp.test/authorize' }, + }; + + inst.onContinueAfterLowRecoveryCodes(); + + expect(sessionStorage.getItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY)).toBe('1'); + expect(window.location.href).toBe('https://idp.test/authorize'); + }); + }); }); From 415c3a6f1a51fb448beedb13526b684c1a6ac528 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 20:53:52 +0200 Subject: [PATCH 2/3] chore: seed mfa-ts-009..012 in the push front-end workflow too The MFA e2e suite gives every TS-* test its own account because a real login burns that user's own OTP rate-limit window. TS-009..TS-012 were added with the seed loop widened only in pull_request_frontend_tests.yml, so "Front End Tests On Push" logged in as users that do not exist and the four new tests timed out waiting for the password step. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/push_frontend_tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/push_frontend_tests.yml b/.github/workflows/push_frontend_tests.yml index 5376f6f0..c70c48da 100644 --- a/.github/workflows/push_frontend_tests.yml +++ b/.github/workflows/push_frontend_tests.yml @@ -103,7 +103,7 @@ jobs: php artisan db:seed --force php artisan idp:create-super-admin test@test.com '1Qaz2wsx!' php artisan idp:create-raw-user e2e@test.com '1Qaz2wsx!' - for i in 001 002 003 004 005 006 007 008; do + for i in 001 002 003 004 005 006 007 008 009 010 011 012; do php artisan idp:create-super-admin "mfa-ts-$i@test.com" '1Qaz2wsx!' done php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!' From 3b8f243af6b9c8b9359a167a6cc049d1a55f867a Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 12 Aug 2026 15:33:17 +0200 Subject: [PATCH 3/3] fix: keep the recovery code out of the request URL verifyRecoveryCode() posted through postRawRequest(), which copies every param onto the query string in addition to the body (base_actions.js:71). That writes the recovery code - a credential that completes a login on its own - into any access log along the path. #146 hit the same trap with current_password and added the body-only postRawRequestFull() for it; switch this call to it as well. TS-005 asserted the code off the query string, which encoded the leak as the expected contract. It now asserts the body carries the code and the query string does not, so the fix cannot silently regress. Also from review: - the "all 8 tests" comment in the MFA spec had gone stale at 12 tests; reworded so it does not track a count, and it now states the seed loop lives in BOTH workflow files (the divergence that broke push CI). - onBackToOtp()'s comment justified its reset with a clean-field-on-reentry guarantee that onUseRecovery() already provides; state the real reason, which is not holding an unspent credential in component state. Co-Authored-By: Claude Opus 5 (1M context) --- resources/js/login/actions.js | 8 ++++++-- resources/js/login/login.js | 7 ++++--- tests/e2e/tests/auth/login-mfa-flow.spec.ts | 19 ++++++++++++------- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/resources/js/login/actions.js b/resources/js/login/actions.js index ebda8a6a..2a0e3285 100644 --- a/resources/js/login/actions.js +++ b/resources/js/login/actions.js @@ -1,4 +1,4 @@ -import {postRawRequest} from '../base_actions' +import {postRawRequest, postRawRequestFull} from '../base_actions' export const verifyAccount = (email, token) => { @@ -51,7 +51,11 @@ export const verifyRecoveryCode = (recoveryCode, token) => { recovery_code: recoveryCode }; - return postRawRequest(window.RECOVERY_2FA_ENDPOINT)(params, {'X-CSRF-TOKEN': token}); + // postRawRequestFull(), not postRawRequest(): the latter also copies every + // param onto the query string, which would write the recovery code - a + // credential that completes a login on its own - into every access log it + // passes through. Same reasoning as the current_password fix in #146. + return postRawRequestFull(window.RECOVERY_2FA_ENDPOINT)(params, {'X-CSRF-TOKEN': token}); } export const cancelLogin = (token) => { diff --git a/resources/js/login/login.js b/resources/js/login/login.js index 16b16aee..3d1ac13e 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -593,9 +593,10 @@ class LoginPage extends React.Component { } onBackToOtp() { - // Drop the half-typed recovery code and its error along with the mode: the - // user is abandoning that attempt, and leaving them behind would re-show a - // stale code/error the next time "Use a recovery code instead" is clicked. + // Drop the abandoned recovery code as we leave the mode so a credential the + // user chose not to spend is not kept in component state for the rest of the + // session. A clean field on re-entry is already guaranteed by onUseRecovery(); + // this is about not holding the value, not about what the next render shows. this.setState({ ...this.state, authFlow: FLOW.MFA, diff --git a/tests/e2e/tests/auth/login-mfa-flow.spec.ts b/tests/e2e/tests/auth/login-mfa-flow.spec.ts index 909f8b8c..1c418df4 100644 --- a/tests/e2e/tests/auth/login-mfa-flow.spec.ts +++ b/tests/e2e/tests/auth/login-mfa-flow.spec.ts @@ -6,16 +6,20 @@ import type { Page } from '@playwright/test'; // param onto the URL as a query string (in addition to the body), so the real // request is "?otp_value=...&method=..." - a glob without the trailing // wildcard requires an exact end-of-string match and silently never fires. +// The recovery endpoint is the exception: it posts through postRawRequestFull(), +// which is body-only, so its URL carries no query string ('**' still matches). const VERIFY_URL = '**/auth/login/2fa/verify**'; const RESEND_URL = '**/auth/login/2fa/resend**'; const RECOVERY_URL = '**/auth/login/2fa/recovery**'; const CANCEL_URL = '**/auth/login/cancel**'; -// Each TS-* test gets its own MFA-enforced super-admin (mfa-ts-NNN@test.com, -// seeded by CI via idp:create-super-admin - see pull_request_frontend_tests.yml). +// Each TS-* test gets its own MFA-enforced super-admin (mfa-ts-NNN@test.com), +// seeded by CI via idp:create-super-admin in BOTH pull_request_frontend_tests.yml +// and push_frontend_tests.yml - adding a TS-NNN here means widening the seed loop +// in both, or the new test logs in as a user that does not exist. // A real login issues a real OTP challenge and counts against that user's own // two_factor.rate_limit.max_otp_requests window, so sharing one fixed account -// across all 8 tests would exhaust the limit well before the suite finishes. +// across the whole suite would exhaust the limit well before it finishes. const MFA_USER_PASSWORD = '1Qaz2wsx!'; // Recovery codes are generated as 8 chars from [A-Z0-9] and shown as XXXX-XXXX @@ -152,10 +156,11 @@ test.describe('MFA Login Flow', () => { loginPage.verifyButton.click(), ]); - // postRawRequest() mirrors every param onto the query string, so the - // normalized value is assertable straight off the request URL. - expect(new URL(request.url()).searchParams.get('recovery_code')) - .toBe(RECOVERY_CODE_NORMALIZED); + // The code travels in the body only. verifyRecoveryCode() posts through + // postRawRequestFull() precisely so it never reaches the query string, + // where access logs would capture it - assert both halves of that. + expect(request.postDataJSON()).toMatchObject({ recovery_code: RECOVERY_CODE_NORMALIZED }); + expect(new URL(request.url()).searchParams.get('recovery_code')).toBeNull(); }); // TS-006 ─────────────────────────────────────────────────────────────────