-
Notifications
You must be signed in to change notification settings - Fork 223
Commits on Feb 27, 2026 bug(settings): Entering invalid ARK breaks reset flow #20121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
+347
−2
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
45 changes: 45 additions & 0 deletions
45
packages/db-migrations/databases/fxa/patches/patch-185-186.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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'; | ||
5 changes: 5 additions & 0 deletions
5
packages/db-migrations/databases/fxa/patches/patch-186-185.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| -- SET NAMES utf8mb4 COLLATE utf8mb4_bin; | ||
|
|
||
| -- DROP PROCEDURE `forgotPasswordVerified_10`; | ||
|
|
||
| -- UPDATE dbMetadata SET value = '185' WHERE name = 'schema-patch-level'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| { | ||
| "level": 185 | ||
| "level": 186 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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'; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment was a copy/paste from the other PR and is no longer correct here. |
||
| // 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'); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having a comment somewhere that the only difference between this migration and the previous one is we no longer delete the token?