Skip to content

Commit

Permalink
Show warnings during recovery and skip validation (#634)
Browse files Browse the repository at this point in the history
* Show warnings during recovery and skip validation

This addresses some long standing issues related to recovery phrase
verification/validation and BIP39, and (in my opinion) improves UX a
bit.

The two important changes are:

* Warnings are shown during recovery phrase input: these includes
  warning about words that aren't recognized, as well as other not so
  easy to spot typing errors.
* Invalid recovery phrase won't prevent user from submitting: this is
  pretty much the first sentence on [bitcoinjs' bip39
  repo](https://github.com/bitcoinjs/bip39#reminder-for-developers);
  instead, it is only a warning.

In a bit more details, the `inputSeedPhrase` page (where the user inputs
their recovery phrase) now shows the following warnings as the phrase is
being typed:

* Unexpected characters were entered (we're expecting letters and
  numbers only)
* Unexpected words were entered (we're expecting words from a list)
* Recovery phrase was generated for another anchor (already existed)
* Incorrect word count (we're expecting 24 words)
* Recovery phrase is not bip39-valid
* Double whitespaces were found (which _will_ prevent the user from
  recovering their anchor)

One thing to note is that, aside from trimming whitespaces at the
beginning and and of the recovery phrase, the implementation does _not_
modify the phrase input by the user, and will _not_ prevent the user
from submitting a phrase deemed invalid. As far as we know we've only
issued properly formatted, english bip-39 phrases but we can't be
certain.

This also adds a bunch of tests for `mnemonic.ts` which now also
regroups most of the operation on the recovery phrase (and fixes a bug
or two spotted by tests).

* Don't jitter but flash warnings

* Add comment

* 🤖 Selenium screenshots auto-update

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
nmattia and github-actions[bot] authored May 9, 2022
1 parent 29fd13b commit 3c7198e
Show file tree
Hide file tree
Showing 9 changed files with 464 additions and 62 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"generate:types": "didc bind ./src/internet_identity/internet_identity.did -t ts > src/frontend/generated/internet_identity_types.d.ts",
"generate:js": "didc bind ./src/internet_identity/internet_identity.did -t js > src/frontend/generated/internet_identity_idl.js",
"build": "NODE_ENV=production webpack",
"test": "jest --roots ./src/frontend --verbose --testPathIgnorePatterns=\"src/frontend/src/test-e2e\"",
"test": "jest --maxWorkers 1 --roots ./src/frontend --verbose --testPathIgnorePatterns=\"src/frontend/src/test-e2e\"",
"test:e2e": "./scripts/with-selenium-standalone jest --roots ./src/frontend --verbose --testPathPattern=\"src/frontend/src/test-e2e\" --detectOpenHandles",
"test:e2e-desktop": "SCREEN=desktop npm run test:e2e",
"test:e2e-mobile": "SCREEN=mobile npm run test:e2e",
Expand Down
Binary file modified screenshots/05-single-device-warning-mobile.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified screenshots/09-new-device-instructions-desktop.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified screenshots/11-new-device-alias-desktop.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified screenshots/11-new-device-alias-mobile.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
107 changes: 107 additions & 0 deletions src/frontend/src/crypto/mnemonic.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import {
Warning,
parseRecoveryPhrase,
getWarnings,
dropLeadingUserNumber,
} from "./mnemonic";

const expectLeadingUserNumberToBe = (
received: string,
expected: number | null
) => {
const parsed = parseRecoveryPhrase(received);
const lun = parsed.leadingUserNumber
? Number(parsed.leadingUserNumber)
: null;
expect(lun).toBe(expected);
};

test("Leading user number is parsed correctly", () => {
expectLeadingUserNumberToBe("10043", 10043);
expectLeadingUserNumberToBe(" 10043 ", 10043);
expectLeadingUserNumberToBe(" 10043 foo bar", 10043);
expectLeadingUserNumberToBe(" 10043 foo bar", 10043);
expectLeadingUserNumberToBe("10043n foo bar", null);
expectLeadingUserNumberToBe("foo 123 bar", null);
expectLeadingUserNumberToBe("", null);
});

test("Leading user number is dropped correctly", () => {
expect(dropLeadingUserNumber("10043")).toEqual("");
expect(dropLeadingUserNumber("10043 ")).toEqual("");
expect(dropLeadingUserNumber("10043 foo bar")).toEqual("foo bar");
expect(dropLeadingUserNumber("10043 foo bar")).toEqual("foo bar");
expect(dropLeadingUserNumber("foo bar")).toEqual("foo bar");
});

const expectWordsToEqual = (received: string, expected: string[]) => {
expect(parseRecoveryPhrase(received).words).toEqual(expected);
};

test("Word list is parsed correctly", () => {
expectWordsToEqual("about abandon", ["about", "abandon"]);
expectWordsToEqual(" about abandon ", ["about", "abandon"]);
expectWordsToEqual("", []);
expectWordsToEqual("about about about abandon 123", [
"about",
"about",
"about",
"abandon",
"123",
]);
});

const expectWarningsInclude = (
userNumber: number,
input: string,
warning: Warning
) => {
expect(getWarnings(BigInt(userNumber), input)).toEqual(
expect.arrayContaining([warning])
);
};

const expectNoWarnings = (userNumber: number, input: string) => {
expect(getWarnings(BigInt(userNumber), input)).toEqual([]);
};

test("Warnings are correctly generated", () => {
expectWarningsInclude(10001, "squirrel &", {
type: "bad_chars",
chars: ["&"],
});

expectWarningsInclude(10001, "about squirrel bar abandon", {
type: "repeated_whitespace",
between: ["squirrel", "bar"],
});

expectWarningsInclude(10001, "10042 about about", {
type: "bad_anchor",
anchor: BigInt(10042),
});

expectWarningsInclude(10001, Array(23).fill("about").join(" "), {
type: "bad_word_count",
count: 23,
});

expectWarningsInclude(10001, `10042 ${Array(23).fill("about").join(" ")}`, {
type: "bad_word_count",
count: 23,
});

expectWarningsInclude(10001, "about abut squirrel squirel abandon squirel", {
type: "bad_words",
words: ["abut", "squirel"],
});

expectWarningsInclude(10001, Array(24).fill("about").join(" "), {
type: "invalid",
});

expectNoWarnings(
10001,
"10001 trust dad oak bright arrow pipe omit provide material donkey hole worry parade test paper fix clutch state range census dust fan hurry almost"
);
});
171 changes: 168 additions & 3 deletions src/frontend/src/crypto/mnemonic.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,183 @@
import { entropyToMnemonic, wordlists, validateMnemonic } from "bip39";
import { toHexString } from "@dfinity/identity/lib/cjs/buffer";
import { isUserNumber } from "../utils/userNumber";

/**
* @returns A random BIP39 mnemonic with 256 bits of entropy.
* @returns A random BIP39 mnemonic with 256 bits of entropy (generates a list of 24 words)
*/
export function generate(): string {
const entropy = new Uint32Array(8);
const entropy = new Uint32Array(8); // NOTE: please change RECOVERYPHRASE_WORDCOUNT if this changes
crypto.getRandomValues(entropy);
return entropyToMnemonic(toHexString(entropy.buffer), wordlists.english);
return entropyToMnemonic(
toHexString(entropy.buffer),
RECOVERYPHRASE_WORDLIST
);
}

/** How many words are expected in the recovery phrase */
export const RECOVERYPHRASE_WORDCOUNT = 24;

/** The list of words used to make the recovery phrase */
// NOTE: "english" is the only one actually bundled in (see webpack config)
export const RECOVERYPHRASE_WORDLIST: string[] = wordlists.english;

/**
* @returns true if the mnemonic is valid, false otherwise.
*/
export function validate(mnemonic: string): boolean {
return validateMnemonic(mnemonic);
}

/** A recovery phrase that was parsed.
* An object of this type should not be used to reconstruct a recovery phrase, since
* some information (though most likely incorrectly inserted) like extra whitespaces
* is lost.
*/
type ParsedRecoveryPhrase = {
leadingUserNumber: string | null;
words: string[];
};

/** Parse recovery phrase into a structured object */
export const parseRecoveryPhrase = (s: string): ParsedRecoveryPhrase => {
/* Split nicely into words
* ' 2002 foo bar ' -> split(" ") -> [ '', '2002', 'foo', '', 'bar', '' ]
* [ '', '2002', 'foo', '', 'bar', '' ] -> filter(...) -> [ '2002', 'foo', 'bar' ]
*/
const words = s.split(" ").filter((w) => w !== "");

// check if there's a leading user (anchor) number
const leadingUserNumber: string | null =
words.length >= 1 && isUserNumber(words[0]) ? words[0] : null;

// If the first element is indeed a number (BigInt), drop it from the actual mnemonic
if (leadingUserNumber !== null) {
words.shift();
}

return { leadingUserNumber, words };
};

/** Drop the anchor number from a string
* Example: " 10005 foo bar" -> "foo bar"
*/
export const dropLeadingUserNumber = (input: string): string => {
const parsed = parseRecoveryPhrase(input);
return parsed.leadingUserNumber !== null
? input.replace(parsed.leadingUserNumber, "").trim()
: input;
};

/** The warning types that may be generated. For more information on each, see 'recoveryPhraseWarnings'.
*
* We generate the messages independently mostly so that we can programatically test the warnings generated
* (and avoid brittle regex-based testing).*/
export type Warning =
| { type: "bad_chars"; chars: string[] }
| { type: "repeated_whitespace"; between: [string, string] | null }
| { type: "bad_anchor"; anchor: bigint }
| { type: "bad_word_count"; count: number }
| { type: "bad_words"; words: string[] }
| { type: "invalid" };

/** Generates warnings for a given recovery phrase and user number */
export const recoveryPhraseWarnings: {
(userNumber: bigint, inputRaw: string): Warning | null;
}[] = [
// The input contains some characters that are not expected in a recovery phrase
(userNumber: bigint, inputRaw: string): Warning | null => {
// Check for anything that's not a letter, a number or a whitespace (' ', not \n)
const badChars = inputRaw.match(/[^a-z0-9\s]|[\n\t\r]/g);

if (badChars) {
return { type: "bad_chars", chars: [...new Set(badChars)] };
}

return null;
},

// Check for double (or triple) whitespaces between words
(userNumber: bigint, inputRaw: string): Warning | null => {
const repeatedSpace = inputRaw.match(/(\S+)\s\s+(\S+)/);
if (repeatedSpace) {
let between: [string, string] | null = null;
if (repeatedSpace.length === 3) {
between = [repeatedSpace[1], repeatedSpace[2]];
}

return { type: "repeated_whitespace", between };
}
return null;
},

// Check for an incorrect anchor number (e.g. "10005 foo bar" when recovery anchor 10004)
(userNumber: bigint, inputRaw: string): Warning | null => {
const input = parseRecoveryPhrase(inputRaw);
const leadingUserNumber =
(input.leadingUserNumber !== null && BigInt(input.leadingUserNumber)) ||
null;

if (leadingUserNumber !== null && leadingUserNumber !== userNumber) {
return { type: "bad_anchor", anchor: leadingUserNumber };
}

return null;
},

// We expect a specific word count
(userNumber: bigint, inputRaw: string): Warning | null => {
const input = parseRecoveryPhrase(inputRaw);

if (
input.words.length >= 1 &&
input.words.length !== RECOVERYPHRASE_WORDCOUNT
) {
return { type: "bad_word_count", count: input.words.length };
}

return null;
},

// As for characters, check for any words that we do not expect
(userNumber: bigint, inputRaw: string): Warning | null => {
const input = parseRecoveryPhrase(inputRaw);

const badWords = [];
for (const word of input.words) {
if (!RECOVERYPHRASE_WORDLIST.includes(word)) {
badWords.push(word);
}
}

if (badWords.length >= 1) {
return { type: "bad_words", words: [...new Set(badWords)] };
}

return null;
},

// We expect a specific word count
(userNumber: bigint, inputRaw: string): Warning | null => {
const input = parseRecoveryPhrase(inputRaw);

if (
input.words.length === RECOVERYPHRASE_WORDCOUNT &&
!validate(dropLeadingUserNumber(inputRaw))
) {
return { type: "invalid" };
}

return null;
},
];

export const getWarnings = (userNumber: bigint, input: string): Warning[] => {
const warnings = [];
for (const mkWarning of recoveryPhraseWarnings) {
const warning = mkWarning(userNumber, input);
if (warning) {
warnings.push(warning);
}
}
return warnings;
};
Loading

0 comments on commit 3c7198e

Please sign in to comment.