From 37ca5f30c25f24a1263b85fc064aedbac35fbf19 Mon Sep 17 00:00:00 2001 From: dschom Date: Fri, 27 Feb 2026 11:26:25 -0800 Subject: [PATCH] bug(settings): Entering invalid ARK breaks reset flow --- .../databases/fxa/patches/patch-185-186.sql | 45 +++ .../databases/fxa/patches/patch-186-185.sql | 5 + .../databases/fxa/target-patch.json | 2 +- .../pages/confirmTotpResetPassword.ts | 9 + packages/functional-tests/pages/index.ts | 2 + .../functional-tests/pages/settings/totp.ts | 4 + .../resetPassword/resetPassword2FA.spec.ts | 280 ++++++++++++++++++ .../fxa-shared/db/models/auth/base-auth.ts | 2 +- 8 files changed, 347 insertions(+), 2 deletions(-) create mode 100644 packages/db-migrations/databases/fxa/patches/patch-185-186.sql create mode 100644 packages/db-migrations/databases/fxa/patches/patch-186-185.sql create mode 100644 packages/functional-tests/pages/confirmTotpResetPassword.ts diff --git a/packages/db-migrations/databases/fxa/patches/patch-185-186.sql b/packages/db-migrations/databases/fxa/patches/patch-185-186.sql new file mode 100644 index 00000000000..22dddca54c0 --- /dev/null +++ b/packages/db-migrations/databases/fxa/patches/patch-185-186.sql @@ -0,0 +1,45 @@ +SET NAMES utf8mb4 COLLATE utf8mb4_bin; + +CALL assertPatchLevel('185'); + +CREATE PROCEDURE `forgotPasswordVerified_10` ( + IN `inPasswordForgotTokenId` BINARY(32), + IN `inAccountResetTokenId` BINARY(32), + IN `inTokenData` BINARY(32), + IN `inUid` BINARY(16), + IN `inCreatedAt` BIGINT UNSIGNED, + IN `inVerificationMethod` INT + ) +BEGIN + DECLARE EXIT HANDLER FOR SQLEXCEPTION +BEGIN + -- ERROR +ROLLBACK; +RESIGNAL; +END; + +START TRANSACTION; + +-- Pass `inVerificationMethod` to the password forgot token table +REPLACE INTO accountResetTokens( + tokenId, + tokenData, + uid, + createdAt, + verificationMethod + ) + VALUES( + inAccountResetTokenId, + inTokenData, + inUid, + inCreatedAt, + inVerificationMethod + ); + +UPDATE accounts SET emailVerified = true WHERE uid = inUid; +UPDATE emails SET isVerified = true, verifiedAt = (UNIX_TIMESTAMP(NOW(3)) * 1000) WHERE isPrimary = true AND uid = inUid; + +COMMIT; +END; + +UPDATE dbMetadata SET value = '186' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa/patches/patch-186-185.sql b/packages/db-migrations/databases/fxa/patches/patch-186-185.sql new file mode 100644 index 00000000000..1a7625f78de --- /dev/null +++ b/packages/db-migrations/databases/fxa/patches/patch-186-185.sql @@ -0,0 +1,5 @@ +-- SET NAMES utf8mb4 COLLATE utf8mb4_bin; + +-- DROP PROCEDURE `forgotPasswordVerified_10`; + +-- UPDATE dbMetadata SET value = '185' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa/target-patch.json b/packages/db-migrations/databases/fxa/target-patch.json index ed079af43ec..de94ea15d05 100644 --- a/packages/db-migrations/databases/fxa/target-patch.json +++ b/packages/db-migrations/databases/fxa/target-patch.json @@ -1,3 +1,3 @@ { - "level": 185 + "level": 186 } diff --git a/packages/functional-tests/pages/confirmTotpResetPassword.ts b/packages/functional-tests/pages/confirmTotpResetPassword.ts new file mode 100644 index 00000000000..4d1ac66cf67 --- /dev/null +++ b/packages/functional-tests/pages/confirmTotpResetPassword.ts @@ -0,0 +1,9 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import { BaseTokenCodePage } from './baseTokenCode'; + +export class ConfirmTotpResetPassword extends BaseTokenCodePage { + readonly path = '/confirm_totp_reset_password'; +} diff --git a/packages/functional-tests/pages/index.ts b/packages/functional-tests/pages/index.ts index bf7b186ef04..31dae58f7c4 100644 --- a/packages/functional-tests/pages/index.ts +++ b/packages/functional-tests/pages/index.ts @@ -7,6 +7,7 @@ import { AvatarPage } from './settings/avatar'; import { BaseTarget } from '../lib/targets/base'; import { ConfigPage } from './config'; import { ConfirmSignupCodePage } from './confirmSignupCode'; +import { ConfirmTotpResetPassword } from './confirmTotpResetPassword'; import { ConnectAnotherDevicePage } from './connectAnotherDevice'; import { CookiesDisabledPage } from './cookiesDisabled'; import { ChangePasswordPage } from './settings/changePassword'; @@ -47,6 +48,7 @@ export function create(page: Page, target: BaseTarget) { changePassword: new ChangePasswordPage(page, target), configPage: new ConfigPage(page, target), confirmSignupCode: new ConfirmSignupCodePage(page, target), + confirmTotpResetPassword: new ConfirmTotpResetPassword(page, target), connectAnotherDevice: new ConnectAnotherDevicePage(page, target), cookiesDisabled: new CookiesDisabledPage(page, target), deleteAccount: new DeleteAccountPage(page, target), diff --git a/packages/functional-tests/pages/settings/totp.ts b/packages/functional-tests/pages/settings/totp.ts index d420fb415be..9185a0010f5 100644 --- a/packages/functional-tests/pages/settings/totp.ts +++ b/packages/functional-tests/pages/settings/totp.ts @@ -94,6 +94,10 @@ export class TotpPage extends SettingsLayout { return this.page.getByRole('button', { name: 'Finish' }); } + get confirmBackupCodeConfirmButton() { + return this.page.getByRole('button', { name: 'Confirm' }); + } + get confirmBackupCodeTextbox() { return ( this.page diff --git a/packages/functional-tests/tests/resetPassword/resetPassword2FA.spec.ts b/packages/functional-tests/tests/resetPassword/resetPassword2FA.spec.ts index b3c2fdab787..b95634ebc01 100644 --- a/packages/functional-tests/tests/resetPassword/resetPassword2FA.spec.ts +++ b/packages/functional-tests/tests/resetPassword/resetPassword2FA.spec.ts @@ -338,6 +338,176 @@ test.describe('severity-1 #smoke', () => { await expect(settings.recoveryKey.status).toHaveText('Not Set'); }); + test('provide invalid recovery key then reset with totp authenticator code', async ({ + page, + target, + pages: { + signin, + resetPassword, + settings, + totp, + confirmTotpResetPassword, + recoveryKey, + }, + testAccountTracker, + }) => { + const credentials = await testAccountTracker.signUp(); + + // Sign Into Settings + await signin.goto(); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.fillOutPasswordForm(credentials.password); + await expect(page).toHaveURL(/settings/); + + // Create Recovery Key + await settings.recoveryKey.createButton.click(); + await settings.confirmMfaGuard(credentials.email); + await recoveryKey.createRecoveryKey(credentials.password, 'hint'); + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.recoveryKey.status).toHaveText('Enabled'); + + // Enable 2FA + await expect(settings.totp.status).toHaveText('Disabled'); + await settings.totp.addButton.click(); + await settings.confirmMfaGuard(credentials.email); + const { secret } = + await totp.setUpTwoStepAuthWithQrAndBackupCodesChoice(credentials); + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.alertBar).toContainText( + 'Two-step authentication has been enabled' + ); + await expect(settings.totp.status).toHaveText('Enabled'); + + // Start Reset Password Flow + await settings.signOut(); + await signin.goto(); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.forgotPasswordLink.click(); + await resetPassword.fillOutEmailForm(credentials.email); + const code = await target.emailClient.getResetPasswordCode( + credentials.email + ); + await await resetPassword.fillOutResetPasswordCodeForm(code); + + // Enter Invalid Recovery Key + await expect(resetPassword.confirmRecoveryKeyHeading).toBeVisible(); + await resetPassword.recoveryKeyTextbox.fill( + '12345678-12345678-12345678-12345678' + ); + await resetPassword.confirmRecoveryKeyButton.click(); + await expect(resetPassword.errorBanner).toBeVisible(); + + // Note! This is the start of edge case this test validates. When we provided + // a recovery key, we took our password forgot token and exchange it for an + // account reset token, which resulted in the passwordForgotToken becoming + // invalid. We therefore must use the account reset token for the rest of + // the web requests in this flow. + + // Click Forgot Key Link + await resetPassword.forgotKeyLink.click(); + + // Provide TOTP Code from Authenticator + await page.waitForURL(/confirm_totp_reset_password/); + await expect(page.getByLabel('Enter 6-digit code')).toBeVisible(); + const totpCode = await getTotpCode(secret); + await confirmTotpResetPassword.fillOutCodeForm(totpCode); + + // Create a New Password + await expect(resetPassword.dataLossWarning).toBeVisible(); + const newPassword = testAccountTracker.generatePassword(); + await resetPassword.fillOutNewPasswordForm(newPassword); + testAccountTracker.updateAccountPassword(credentials.email, newPassword); + + // Observe Settings + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.alertBar).toHaveText('Your password has been reset'); + }); + + test('provide invalid recovery key then reset with totp back up code', async ({ + page, + target, + pages: { signin, resetPassword, settings, totp, recoveryKey }, + testAccountTracker, + }) => { + const credentials = await testAccountTracker.signUp(); + + // Sign Into Settings + await signin.goto(); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.fillOutPasswordForm(credentials.password); + + // Create Recovery Key + await expect(page).toHaveURL(/settings/); + await settings.recoveryKey.createButton.click(); + await settings.confirmMfaGuard(credentials.email); + await recoveryKey.createRecoveryKey(credentials.password, 'hint'); + + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.recoveryKey.status).toHaveText('Enabled'); + + // Enable 2FA + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.totp.status).toHaveText('Disabled'); + await settings.totp.addButton.click(); + await settings.confirmMfaGuard(credentials.email); + const { recoveryCodes } = + await totp.setUpTwoStepAuthWithQrAndBackupCodesChoice(credentials); + + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.alertBar).toContainText( + 'Two-step authentication has been enabled' + ); + await expect(settings.totp.status).toHaveText('Enabled'); + + // Start Reset Password Flow + await settings.signOut(); + await signin.goto(); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.forgotPasswordLink.click(); + await resetPassword.fillOutEmailForm(credentials.email); + const code = await target.emailClient.getResetPasswordCode( + credentials.email + ); + await resetPassword.fillOutResetPasswordCodeForm(code); + + // Enter Invalid Recovery Key + await expect(resetPassword.confirmRecoveryKeyHeading).toBeVisible(); + await resetPassword.recoveryKeyTextbox.fill( + '12345678-12345678-12345678-12345678' + ); + await resetPassword.confirmRecoveryKeyButton.click(); + await expect(resetPassword.errorBanner).toBeVisible(); + + /// Note! This is the start of edge case this test validates. When we provided + // a recovery key, we took our password forgot token and exchange it for an + // account reset token, which resulted in the passwordForgotToken becoming + // invalid. We therefore must use the account reset token for the rest of + // the web requests in this flow. + + // Click Forgot Key Link + await resetPassword.forgotKeyLink.click(); + + // Verify TOTP code entry page is shown + await page.waitForURL(/confirm_totp_reset_password/); + await expect(page.getByLabel('Enter 6-digit code')).toBeVisible(); + await resetPassword.clickTroubleEnteringCode(); + + // Provide a Backup TOTP Codes + await expect(totp.confirmBackupCodeHeading).toBeVisible(); + await totp.confirmBackupCodeTextbox.fill(recoveryCodes[0]); + await totp.confirmBackupCodeConfirmButton.click(); + + // Create a New Password + await expect(resetPassword.dataLossWarning).toBeVisible(); + const newPassword = testAccountTracker.generatePassword(); + await resetPassword.fillOutNewPasswordForm(newPassword); + testAccountTracker.updateAccountPassword(credentials.email, newPassword); + + // Observe Settings Page + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.alertBar).toHaveText('Your password has been reset'); + }); + test('can reset password with unverified 2FA and skip recovery key', async ({ page, target, @@ -570,4 +740,114 @@ test.describe('reset password with recovery phone', () => { await expect(settings.settingsHeading).toBeVisible(); }); + + test('provide invalid recovery key then reset with recovery phone', async ({ + page, + target, + pages: { + signin, + resetPassword, + settings, + totp, + recoveryKey, + recoveryPhone, + }, + testAccountTracker, + }) => { + const credentials = await testAccountTracker.signUp(); + const testNumber = target.smsClient.getPhoneNumber(); + + // Sign Into Settings + await signin.goto(); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.fillOutPasswordForm(credentials.password); + await expect(page).toHaveURL(/settings/); + + // Create Recovery Key + await settings.recoveryKey.createButton.click(); + await settings.confirmMfaGuard(credentials.email); + await recoveryKey.createRecoveryKey(credentials.password, 'hint'); + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.recoveryKey.status).toHaveText('Enabled'); + + // Enable 2FA + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.totp.status).toHaveText('Disabled'); + await settings.totp.addButton.click(); + await settings.confirmMfaGuard(credentials.email); + await totp.setUpTwoStepAuthWithQrAndBackupCodesChoice(credentials); + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.alertBar).toContainText( + 'Two-step authentication has been enabled' + ); + await expect(settings.totp.status).toHaveText('Enabled'); + + // Enable Recovery Phone + await settings.totp.addRecoveryPhoneButton.click(); + await page.waitForURL(/recovery_phone\/setup/); + await expect(recoveryPhone.addHeader()).toBeVisible(); + await recoveryPhone.enterPhoneNumber(testNumber); + await recoveryPhone.clickSendCode(); + await expect(recoveryPhone.confirmHeader).toBeVisible(); + let smsCode = await target.smsClient.getCode({ ...credentials }); + await recoveryPhone.enterCode(smsCode); + await recoveryPhone.clickConfirm(); + await page.waitForURL(/settings/); + await expect(settings.alertBar).toHaveText('Recovery phone added'); + + // Start Reset Password Flow + await settings.signOut(); + await signin.goto(); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.forgotPasswordLink.click(); + await resetPassword.fillOutEmailForm(credentials.email); + const code = await target.emailClient.getResetPasswordCode( + credentials.email + ); + await resetPassword.fillOutResetPasswordCodeForm(code); + await expect(resetPassword.confirmRecoveryKeyHeading).toBeVisible(); + + // Enter Invalid Recovery Key + await resetPassword.recoveryKeyTextbox.fill( + '12345678-12345678-12345678-12345678' + ); + await resetPassword.confirmRecoveryKeyButton.click(); + await expect(resetPassword.errorBanner).toBeVisible(); + + // Note! This is the start of edge case this test validates. When we provided + // a recovery key, we took our password forgot token and exchange it for an + // account reset token, which resulted in the passwordForgotToken becoming + // invalid. We therefore must use the account reset token for the rest of + // the web requests in this flow. + + // Click Forgot Key Link + await resetPassword.forgotKeyLink.click(); + + // Verify TOTP code entry page is shown + await page.waitForURL(/confirm_totp_reset_password/); + await expect(page.getByLabel('Enter 6-digit code')).toBeVisible(); + await resetPassword.clickTroubleEnteringCode(); + + // Choose Recovery Phone Option + await page.waitForURL(/reset_password_totp_recovery_choice/); + await resetPassword.clickChoosePhone(); + await resetPassword.clickContinueButton(); + + // Provide SMS Code + await page.waitForURL(/reset_password_recovery_phone/); + + smsCode = await target.smsClient.getCode({ ...credentials }); + await resetPassword.fillRecoveryPhoneCodeForm(smsCode); + await resetPassword.clickConfirmButton(); + + // Create a New Password + await expect(resetPassword.dataLossWarning).toBeVisible(); + const newPassword = testAccountTracker.generatePassword(); + await resetPassword.fillOutNewPasswordForm(newPassword); + testAccountTracker.updateAccountPassword(credentials.email, newPassword); + + // Observe Settings Page + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.alertBar).toHaveText('Your password has been reset'); + }); }); diff --git a/packages/fxa-shared/db/models/auth/base-auth.ts b/packages/fxa-shared/db/models/auth/base-auth.ts index beacb229297..7823c0a11ef 100644 --- a/packages/fxa-shared/db/models/auth/base-auth.ts +++ b/packages/fxa-shared/db/models/auth/base-auth.ts @@ -43,7 +43,7 @@ export enum Proc { DeviceFromRefreshTokenId = 'deviceFromRefreshTokenId_1', EmailBounces = 'fetchEmailBounces_4', FindLargeAccounts = 'findLargeAccounts_1', - ForgotPasswordVerified = 'forgotPasswordVerified_9', + ForgotPasswordVerified = 'forgotPasswordVerified_10', KeyFetchToken = 'keyFetchToken_1', KeyFetchTokenWithVerificationStatus = 'keyFetchTokenWithVerificationStatus_2', LimitSessions = 'limitSessions_3',