Skip to content

Commit

Permalink
Ensure user is prompted for password during reminder based backup (#2…
Browse files Browse the repository at this point in the history
…2307)

During the backup flow, we should prompt a user for a password. This
provides extra friction during a security sensitive step.

1. Build, install and onboard
2. Make sure to back up your seed phrase during onboarding. Onboarding
should work as normal
3. Once you get to the home screen, replace `home.html` in the url
browsers url bar with
`home.html#onboarding/secure-your-wallet/?isFromReminder=true` and press
enter
4. Click "Secure my wallet"
5. You should then be prompted to enter your password.
6. After entering your password, you should be able to proceed as normal

If you repeat those steps but click cancel when being prompted to enter
your password, you should be taken to the home screen.

If you repeat those steps, but on step 2 don't back up your seed phrase,
the remaining steps should work as described above.

https://github.com/MetaMask/metamask-extension/assets/7499938/ac8b5dfb-ca7b-4622-95b8-07db5405abfe

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Pedro Figueiredo <[email protected]>
  • Loading branch information
2 people authored and DDDDDanica committed Dec 18, 2023
1 parent c8e8a10 commit 92545de
Show file tree
Hide file tree
Showing 13 changed files with 1,759 additions and 865 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/dist/KeyringController.js b/dist/KeyringController.js
index da9523afe0a83e3fa298b47a518cb583eb509e52..8fe701103975335ff02ed8195f250f35eedccbab 100644
--- a/dist/KeyringController.js
+++ b/dist/KeyringController.js
@@ -662,7 +662,6 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
throw new Error('Seed phrase imported different accounts.');
}
});
- return seedWords;
});
}
// QR Hardware related methods
1,057 changes: 995 additions & 62 deletions CHANGELOG.md

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -2608,7 +2608,7 @@ export default class MetamaskController extends EventEmitter {

// primary keyring management
addNewAccount: this.addNewAccount.bind(this),
verifySeedPhrase: this.verifySeedPhrase.bind(this),
getSeedPhrase: this.getSeedPhrase.bind(this),
resetAccount: this.resetAccount.bind(this),
removeAccount: this.removeAccount.bind(this),
importAccountWithStrategy: this.importAccountWithStrategy.bind(this),
Expand Down Expand Up @@ -3798,12 +3798,13 @@ export default class MetamaskController extends EventEmitter {
*
* Called when the first account is created and on unlocking the vault.
*
* @param password
* @returns {Promise<number[]>} The seed phrase to be confirmed by the user,
* encoded as an array of UTF-8 bytes.
*/
async verifySeedPhrase() {
async getSeedPhrase(password) {
return this._convertEnglishWordlistIndicesToCodepoints(
await this.keyringController.verifySeedPhrase(),
await this.keyringController.exportSeedPhrase(password),
);
}

Expand Down
8 changes: 4 additions & 4 deletions app/scripts/metamask-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -925,10 +925,10 @@ describe('MetaMaskController', () => {
});
});

describe('#verifyseedPhrase', () => {
it('errors when no keying is provided', async () => {
await expect(metamaskController.verifySeedPhrase()).rejects.toThrow(
'No HD keyring found',
describe('#getSeedPhrase', () => {
it('errors when no password is provided', async () => {
await expect(metamaskController.getSeedPhrase()).rejects.toThrow(
'KeyringController - Cannot unlock without a previous vault.',
);
});

Expand Down
54 changes: 31 additions & 23 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "metamask-crx",
"version": "11.7.1",
"version": "11.7.0",
"private": true,
"repository": {
"type": "git",
Expand All @@ -15,6 +15,7 @@
"dist": "yarn build dist",
"dist:mv3": "ENABLE_MV3=true yarn build dist",
"dist:mmi": "yarn dist --build-type mmi",
"dist:mmi:debug": "yarn dist --build-type mmi --apply-lavamoat=false",
"build": "yarn lavamoat:build",
"build:dev": "node development/build/index.js",
"start:test": "BLOCKAID_FILE_CDN=static.metafi-dev.codefi.network/api/v1/confirmations/ppom SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' SENTRY_DSN_DEV=https://[email protected]/0000000 yarn build:dev testDev --apply-lavamoat=false --snow=false",
Expand Down Expand Up @@ -216,11 +217,13 @@
"[email protected]": "^7.5.4",
"[email protected]": "^7.5.4",
"@metamask/eth-keyring-controller@npm:^13.0.1": "patch:@metamask/eth-keyring-controller@npm%3A13.0.1#~/.yarn/patches/@metamask-eth-keyring-controller-npm-13.0.1-06ff83faad.patch",
"nonce-tracker@npm:^3.0.0": "patch:nonce-tracker@npm%3A3.0.0#~/.yarn/patches/nonce-tracker-npm-3.0.0-c5e9a93f9d.patch"
"nonce-tracker@npm:^3.0.0": "patch:nonce-tracker@npm%3A3.0.0#~/.yarn/patches/nonce-tracker-npm-3.0.0-c5e9a93f9d.patch",
"@metamask/accounts-controller@npm:^4.0.0": "patch:@metamask/accounts-controller@npm%3A4.0.0#~/.yarn/patches/@metamask-accounts-controller-npm-4.0.0-3127b56d14.patch",
"@trezor/connect-web": "9.0.11"
},
"dependencies": {
"@babel/runtime": "^7.23.2",
"@blockaid/ppom_release": "^1.2.8",
"@blockaid/ppom_release": "^1.3.3",
"@ensdomains/content-hash": "^2.5.6",
"@ethereumjs/common": "^3.1.1",
"@ethereumjs/tx": "^4.1.1",
Expand All @@ -242,40 +245,40 @@
"@metamask-institutional/rpc-allowlist": "^1.0.0",
"@metamask-institutional/sdk": "^0.1.23",
"@metamask-institutional/transaction-update": "^0.1.32",
"@metamask/accounts-controller": "^4.0.0",
"@metamask/address-book-controller": "^3.0.0",
"@metamask/announcement-controller": "^4.0.0",
"@metamask/approval-controller": "^3.4.0",
"@metamask/assets-controllers": "patch:@metamask/assets-controllers@npm%3A18.0.0#~/.yarn/patches/@metamask-assets-controllers-npm-18.0.0-48bf74a7fa.patch",
"@metamask/base-controller": "^3.2.0",
"@metamask/browser-passworder": "^4.1.0",
"@metamask/assets-controllers": "^21.0.0",
"@metamask/base-controller": "^4.0.0",
"@metamask/browser-passworder": "^4.3.0",
"@metamask/contract-metadata": "^2.3.1",
"@metamask/controller-utils": "^5.0.0",
"@metamask/design-tokens": "^1.12.0",
"@metamask/desktop": "^0.3.0",
"@metamask/eth-json-rpc-middleware": "^12.0.1",
"@metamask/eth-keyring-controller": "^13.0.1",
"@metamask/eth-keyring-controller": "^15.1.0",
"@metamask/eth-ledger-bridge-keyring": "^2.0.1",
"@metamask/eth-query": "^3.0.1",
"@metamask/eth-snap-keyring": "^1.0.0",
"@metamask/eth-snap-keyring": "^2.0.0",
"@metamask/eth-token-tracker": "^4.0.0",
"@metamask/eth-trezor-keyring": "^2.0.0",
"@metamask/eth-trezor-keyring": "^3.0.0",
"@metamask/etherscan-link": "^2.2.0",
"@metamask/ethjs-query": "^0.5.0",
"@metamask/gas-fee-controller": "^6.0.1",
"@metamask/gas-fee-controller": "^10.0.1",
"@metamask/jazzicon": "^2.0.0",
"@metamask/key-tree": "^9.0.0",
"@metamask/keyring-api": "^1.0.0",
"@metamask/keyring-controller": "^8.0.3",
"@metamask/keyring-controller": "patch:@metamask/keyring-controller@npm%3A9.0.0#~/.yarn/patches/@metamask-keyring-controller-npm-9.0.0-f57ed3ebea.patch",
"@metamask/logging-controller": "^1.0.1",
"@metamask/logo": "^3.1.2",
"@metamask/message-manager": "^7.3.0",
"@metamask/metamask-eth-abis": "^3.0.0",
"@metamask/name-controller": "^3.0.0",
"@metamask/network-controller": "^15.2.0",
"@metamask/network-controller": "^16.0.0",
"@metamask/notification-controller": "^3.0.0",
"@metamask/obs-store": "^8.1.0",
"@metamask/permission-controller": "^6.0.0",
"@metamask/phishing-controller": "^7.0.1",
"@metamask/phishing-controller": "^8.0.0",
"@metamask/polling-controller": "^1.0.1",
"@metamask/post-message-stream": "^7.0.0",
"@metamask/ppom-validator": "^0.10.0",
Expand All @@ -286,12 +289,11 @@
"@metamask/scure-bip39": "^2.0.3",
"@metamask/selected-network-controller": "^3.1.0",
"@metamask/signature-controller": "^6.1.2",
"@metamask/slip44": "^3.1.0",
"@metamask/smart-transactions-controller": "^6.2.2",
"@metamask/snaps-controllers": "^3.4.1",
"@metamask/snaps-rpc-methods": "^4.0.1",
"@metamask/snaps-controllers": "^3.5.1",
"@metamask/snaps-rpc-methods": "^4.0.3",
"@metamask/snaps-sdk": "^1.2.0",
"@metamask/snaps-utils": "^5.0.0",
"@metamask/snaps-utils": "^5.1.0",
"@metamask/transaction-controller": "^18.3.0",
"@metamask/utils": "^8.2.1",
"@ngraveio/bc-ur": "^1.1.6",
Expand Down Expand Up @@ -354,6 +356,7 @@
"qrcode-generator": "1.4.1",
"qrcode.react": "^1.0.1",
"react": "^16.12.0",
"react-beautiful-dnd": "^13.1.1",
"react-dom": "^16.12.0",
"react-focus-lock": "^2.9.4",
"react-idle-timer": "^4.2.5",
Expand Down Expand Up @@ -393,6 +396,7 @@
"@lavamoat/allow-scripts": "^2.3.1",
"@lavamoat/lavapack": "^5.2.4",
"@metamask/auto-changelog": "^2.1.0",
"@metamask/build-utils": "^1.0.0",
"@metamask/eslint-config": "^9.0.0",
"@metamask/eslint-config-jest": "^9.0.0",
"@metamask/eslint-config-mocha": "^9.0.0",
Expand Down Expand Up @@ -436,13 +440,15 @@
"@types/jest": "^29.1.2",
"@types/madge": "^5.0.0",
"@types/mocha": "^10.0.3",
"@types/node": "^17.0.21",
"@types/node": "^18",
"@types/pify": "^5.0.1",
"@types/pump": "^1.1.1",
"@types/react": "^16.9.53",
"@types/react-beautiful-dnd": "^13",
"@types/react-dom": "^17.0.11",
"@types/react-redux": "^7.1.25",
"@types/react-router-dom": "^5.3.3",
"@types/readable-stream": "^4.0.4",
"@types/remote-redux-devtools": "^0.5.5",
"@types/sass": "^1.43.1",
"@types/selenium-webdriver": "^4.1.19",
Expand Down Expand Up @@ -602,9 +608,6 @@
"eth-sig-util>ethereumjs-util>keccak": false,
"eth-sig-util>ethereumjs-util>secp256k1": false,
"eth-trezor-keyring>hdkey>secp256k1": false,
"eth-trezor-keyring>trezor-connect>@trezor/transport>protobufjs": false,
"eth-trezor-keyring>trezor-connect>@trezor/utxo-lib>blake-hash": false,
"eth-trezor-keyring>trezor-connect>@trezor/utxo-lib>tiny-secp256k1": false,
"ethereumjs-util>ethereum-cryptography>keccak": false,
"ganache>@trufflesuite/bigint-buffer": false,
"ganache>bufferutil": false,
Expand Down Expand Up @@ -640,7 +643,12 @@
"@storybook/manager-webpack5>core-js": false,
"@storybook/react-webpack5>@storybook/preset-react-webpack>@pmmmwh/react-refresh-webpack-plugin>core-js-pure": false,
"ethjs-contract>babel-runtime>core-js": false,
"ts-node>@swc/core": false
"ts-node>@swc/core": false,
"@trezor/connect-web>@trezor/connect>@trezor/transport>protobufjs": false,
"@trezor/connect-web>@trezor/connect>@trezor/transport>usb": false,
"@trezor/connect-web>@trezor/connect>@trezor/utxo-lib>blake-hash": false,
"@trezor/connect-web>@trezor/connect>@trezor/utxo-lib>tiny-secp256k1": false,
"@metamask/eth-trezor-keyring>@trezor/connect-web>@trezor/connect>@trezor/transport>usb": false
}
},
"packageManager": "[email protected]"
Expand Down
13 changes: 12 additions & 1 deletion test/e2e/tests/incremental-security.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ const { strict: assert } = require('assert');
const { convertToHexValue, withFixtures, openDapp } = require('../helpers');
const FixtureBuilder = require('../fixture-builder');

const WALLET_PASSWORD = 'correct horse battery staple';

describe('Incremental Security', function () {
const ganacheOptions = {
accounts: [
Expand Down Expand Up @@ -124,7 +126,16 @@ describe('Incremental Security', function () {

// reveals the Secret Recovery Phrase
await driver.clickElement('[data-testid="secure-wallet-recommended"]');
await driver.clickElement('[data-testid="recovery-phrase-reveal"]');

await driver.fill('[placeholder="Password"]', WALLET_PASSWORD);
await driver.clickElement({ text: 'Confirm', tag: 'button' });
await driver.waitForElementNotPresent('.mm-modal-overlay');

const recoveryPhraseRevealButton = await driver.findClickableElement(
'[data-testid="recovery-phrase-reveal"]',
);
await recoveryPhraseRevealButton.click();

const chipTwo = await (
await driver.findElement('[data-testid="recovery-phrase-chip-2"]')
).getText();
Expand Down
1 change: 1 addition & 0 deletions ui/components/app/reveal-SRP-modal/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './reveal-SRP-modal';
88 changes: 88 additions & 0 deletions ui/components/app/reveal-SRP-modal/reveal-SRP-modal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import PropTypes from 'prop-types';
import React, { useCallback, useState } from 'react';
import {
Display,
TextVariant,
FontWeight,
} from '../../../helpers/constants/design-system';
import { useI18nContext } from '../../../hooks/useI18nContext';
import { getSeedPhrase } from '../../../store/actions';
import {
Box,
Modal,
ModalContent,
ModalHeader,
ModalOverlay,
ButtonPrimary,
ButtonSecondary,
FormTextField,
} from '../../component-library';

export default function RevealSRPModal({
setSecretRecoveryPhrase,
onClose,
isOpen,
}) {
const t = useI18nContext();

const [password, setPassword] = useState('');

const onSubmit = useCallback(
async (_password) => {
const seedPhrase = await getSeedPhrase(_password);
setSecretRecoveryPhrase(seedPhrase);
},
[setSecretRecoveryPhrase],
);

return (
<Modal isOpen={isOpen} onClose={onClose}>
<ModalOverlay />
<ModalContent>
<ModalHeader onClose={onClose}>{t('revealSeedWords')}</ModalHeader>
<Box paddingLeft={4} paddingRight={4}>
<form
onSubmit={(e) => {
e.preventDefault();
onSubmit(password);
}}
>
<FormTextField
marginTop={6}
id="account-details-authenticate"
label={t('enterYourPassword')}
placeholder={t('password')}
onChange={(e) => setPassword(e.target.value)}
value={password}
variant={TextVariant.bodySm}
type="password"
labelProps={{ fontWeight: FontWeight.Medium }}
autoFocus
/>
</form>
<Box display={Display.Flex} marginTop={6} gap={2}>
<ButtonSecondary onClick={onClose} block>
{t('cancel')}
</ButtonSecondary>
<ButtonPrimary
onClick={() => onSubmit(password)}
disabled={password === ''}
block
>
{t('confirm')}
</ButtonPrimary>
</Box>
</Box>
</ModalContent>
</Modal>
);
}

RevealSRPModal.propTypes = {
/**
* A function to set a secret receovery phrase in the context that is rendering the RevealSRPModal
*/
setSecretRecoveryPhrase: PropTypes.func.isRequired,
onClose: PropTypes.func.isRequired,
isOpen: PropTypes.bool.isRequired,
};
29 changes: 14 additions & 15 deletions ui/pages/onboarding-flow/onboarding-flow.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ import {
createNewVaultAndGetSeedPhrase,
unlockAndGetSeedPhrase,
createNewVaultAndRestore,
verifySeedPhrase,
} from '../../store/actions';
import { getFirstTimeFlowTypeRoute } from '../../selectors';
import { MetaMetricsContext } from '../../contexts/metametrics';
import Button from '../../components/ui/button';
import RevealSRPModal from '../../components/app/reveal-SRP-modal';
import { useI18nContext } from '../../hooks/useI18nContext';
import {
MetaMetricsEventCategory,
Expand Down Expand Up @@ -60,7 +60,7 @@ const TWITTER_URL = 'https://twitter.com/MetaMask';
export default function OnboardingFlow() {
const [secretRecoveryPhrase, setSecretRecoveryPhrase] = useState('');
const dispatch = useDispatch();
const { pathName, search } = useLocation();
const { pathname, search } = useLocation();
const history = useHistory();
const t = useI18nContext();
const completedOnboarding = useSelector(getCompletedOnboarding);
Expand All @@ -74,18 +74,6 @@ export default function OnboardingFlow() {
}
}, [history, completedOnboarding, isFromReminder]);

useEffect(() => {
const verifyAndSetSeedPhrase = async () => {
if (completedOnboarding && !secretRecoveryPhrase) {
const verifiedSeedPhrase = await verifySeedPhrase();
if (verifiedSeedPhrase) {
setSecretRecoveryPhrase(verifiedSeedPhrase);
}
}
};
verifyAndSetSeedPhrase();
}, [completedOnboarding, secretRecoveryPhrase]);

const handleCreateNewAccount = async (password) => {
const newSecretRecoveryPhrase = await dispatch(
createNewVaultAndGetSeedPhrase(password),
Expand All @@ -105,8 +93,19 @@ export default function OnboardingFlow() {
return await dispatch(createNewVaultAndRestore(password, srp));
};

const showPasswordModalToAllowSRPReveal =
pathname === `${ONBOARDING_REVIEW_SRP_ROUTE}/` &&
completedOnboarding &&
!secretRecoveryPhrase &&
isFromReminder;

return (
<div className="onboarding-flow">
<RevealSRPModal
setSecretRecoveryPhrase={setSecretRecoveryPhrase}
onClose={() => history.push(DEFAULT_ROUTE)}
isOpen={showPasswordModalToAllowSRPReveal}
/>
<div className="onboarding-flow__wrapper">
<Switch>
<Route
Expand Down Expand Up @@ -203,7 +202,7 @@ export default function OnboardingFlow() {
<Route exact path="*" component={OnboardingFlowSwitch} />
</Switch>
</div>
{pathName === ONBOARDING_COMPLETION_ROUTE && (
{pathname === ONBOARDING_COMPLETION_ROUTE && (
<Button
className="onboarding-flow__twitter-button"
type="link"
Expand Down
Loading

0 comments on commit 92545de

Please sign in to comment.