Split SRP input by word #14016

Gudahtt · 2022-03-17T14:53:38Z

Explanation

Split the secret recovery phrase input field into one field per word. Each word uses a type="password" field. There is a "show/hide toggle" next to each word allowing you to reveal it, but only one word can be revealed at a time.

This is a new design aimed at making our SRP input component more secure. This design makes it impossible to accidentally reveal your entire SRP to anybody nearby. It also acts as additional protection against the complete SRP being stored in a browser autocomplete database (though to be clear, we could find no evidence this was ever occurring).

More information

Figma design: https://www.figma.com/file/8z4Csc3wlOIkUz2OP1eTZW/Secure-UX-Improvements?node-id=1241%3A12046

There have been some changes from the original design (e.g. the addition of the tip about pasting). Also that design was for the "Onboarding V2" flow, so it was not followed exactly. For now this PR targets the old onboarding flow. I will add this new component to the new onboarding flow in the next PR after this.

Screenshots/Screencaps

Before

After

srp-input.mp4

Manual testing steps

The two screens that have changed are the "Import" page of onboarding, and the "Restore wallet" page accessible from the lock screen. The component and how it behaves should be identical in both places.

Suggestions on which behaviors to test:

It should allow manually typing in the SRP, or pasting the entire thing, or pasting each word individually.
The clipboard should be cleared on paste, unless the contents are too long (>24 words). In that case, the clipboard is preserved and the paste event is ignored (i.e. no fields are changed).
This should be fully navigable via keyboard, and the show/hide toggle should be toggle-able using the spacebar while focused.
All fields should be hidden on paste.
Only one field should be revealed at a time. Revealing any field hides the others.

metamaskbot · 2022-03-17T17:16:07Z

Builds ready [069506c]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1332 ± 48 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	1203	148	242	116
		domContentLoaded	1184	1630	1330	100	48
		load	1191	1630	1332	100	48
		domInteractive	1184	1630	1330	100	48

highlights:

storybook

metamaskbot · 2022-03-17T22:19:12Z

Builds ready [d6fdf01]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1408 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	66	147	95	17	8
		domContentLoaded	1193	1597	1404	122	59
		load	1193	1597	1408	123	59
		domInteractive	1193	1597	1404	122	59

highlights:

storybook

This new version of the SrpInput component uses a separate field for each word of the SRP. Only one field can be revealed at a time, making it less likely that it gets accidentally revealed to somebody.

… layout

…ort sizes

The "info" style of `ActionableMessage` is the default, so no type is required.

The method `toBeInTheDocument()` is now used over `not.toBeNull()` to improve the readability of tests. Likewise, the convenience method `.clear` is now used to clear fields rather than manually entering the key combination to clear a field.

adonesky1 · 2022-03-21T16:54:39Z

Revealing any field hides the others

Could this possibly be annoying for some users who want to be able to visually confirm the whole phrase? Or atleast in larger chunks than 1 at a time?

cc @holantonela

metamaskbot · 2022-03-21T17:01:16Z

Builds ready [d9eb3a5]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1374 ± 65 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	153	95	18	9
		domContentLoaded	1208	1732	1371	136	65
		load	1214	1732	1374	135	65
		domInteractive	1208	1732	1371	136	65

highlights:

storybook

Gudahtt · 2022-03-21T17:12:33Z

Could this possibly be annoying for some users who want to be able to visually confirm the whole phrase? Or atleast in larger chunks than 1 at a time?

Potentially, yes. But this was the tradeoff for making it safer to input. Unsure how we could make the process of checking the entire SRP easier. Right now the process would be "tab + tab + space" to navigate to the next word and reveal it.

holantonela · 2022-03-21T17:39:00Z

Or atleast in larger chunks than 1 at a time?

This is interesting.

Unsure how we could make the process of checking the entire SRP easier

We could do that by rendering the SRP as an image.

brad-decker

um okay i love this. Great work

brad-decker · 2022-03-21T17:50:19Z

I checked all steps on chrome and brave (linux) and it worked like a charm.

adonesky1

LGTM!

Gudahtt changed the title ~~Split srp input by word~~ Split SRP input by word Mar 17, 2022

Gudahtt force-pushed the split-srp-input-by-word branch from 787d63a to 069506c Compare March 17, 2022 16:26

Gudahtt added 21 commits March 21, 2022 13:27

Initial implementation of new SrpInput component

b25802d

This new version of the SrpInput component uses a separate field for each word of the SRP. Only one field can be revealed at a time, making it less likely that it gets accidentally revealed to somebody.

Fix copy mistakes

f7fc10b

Move container div from 'create vault' to 'srp-input', and setup grid…

f1bd4bd

… layout

Increase size of title

0e86cf2

Remove hard-coded width in Storybook to allow testing different viewp…

1f48b2c

…ort sizes

Improve layout

27d0e91

Improve margins

7ff9751

Update dropdown text

0998e76

Expand SRP input section

a6d7d78

Remove unused localized messages

dcc3f82

Update dropdown option names in unit tests

dc41b0f

Replace checkbox with show/hide toggle

ffa284d

Remove unused localized message

4637909

Fix 'data-testid' prop name

1fde09c

Fix e2e test imports using paste

965d7cf

Use 'ActionableMessage' component for error message

c4482b5

Convert error popover to actionable message

d8bbb37

Add tip about pasting the SRP

362dbba

Remove invalid prop

e3a3568

The "info" style of `ActionableMessage` is the default, so no type is required.

Use more readable test convenience methods

a4034d8

The method `toBeInTheDocument()` is now used over `not.toBeNull()` to improve the readability of tests. Likewise, the convenience method `.clear` is now used to clear fields rather than manually entering the key combination to clear a field.

Fix misspelled word

d9eb3a5

Gudahtt force-pushed the split-srp-input-by-word branch from d6fdf01 to d9eb3a5 Compare March 21, 2022 15:57

Gudahtt marked this pull request as ready for review March 21, 2022 15:59

Gudahtt requested a review from a team as a code owner March 21, 2022 15:59

Gudahtt requested a review from adonesky1 March 21, 2022 15:59

brad-decker approved these changes Mar 21, 2022

View reviewed changes

adonesky1 approved these changes Mar 21, 2022

View reviewed changes

Gudahtt merged commit 2931957 into develop Mar 21, 2022

Gudahtt deleted the split-srp-input-by-word branch March 21, 2022 19:09

github-actions bot locked and limited conversation to collaborators Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split SRP input by word #14016

Split SRP input by word #14016

Gudahtt commented Mar 17, 2022

metamaskbot commented Mar 17, 2022

metamaskbot commented Mar 17, 2022

adonesky1 commented Mar 21, 2022 •

edited

Loading

metamaskbot commented Mar 21, 2022

Gudahtt commented Mar 21, 2022

holantonela commented Mar 21, 2022 •

edited

Loading

brad-decker left a comment

brad-decker commented Mar 21, 2022 •

edited

Loading

adonesky1 left a comment

Split SRP input by word #14016

Split SRP input by word #14016

Conversation

Gudahtt commented Mar 17, 2022

Explanation

More information

Screenshots/Screencaps

Before

After

Manual testing steps

metamaskbot commented Mar 17, 2022

highlights:

metamaskbot commented Mar 17, 2022

highlights:

adonesky1 commented Mar 21, 2022 • edited Loading

metamaskbot commented Mar 21, 2022

highlights:

Gudahtt commented Mar 21, 2022

holantonela commented Mar 21, 2022 • edited Loading

brad-decker left a comment

Choose a reason for hiding this comment

brad-decker commented Mar 21, 2022 • edited Loading

adonesky1 left a comment

Choose a reason for hiding this comment

adonesky1 commented Mar 21, 2022 •

edited

Loading

holantonela commented Mar 21, 2022 •

edited

Loading

brad-decker commented Mar 21, 2022 •

edited

Loading