feat: password-gated recovery phrase display for HD wallets (closes #161)
All checks were successful
check / check (push) Successful in 22s
All checks were successful
check / check (push) Successful in 22s
A user who created a wallet in AutistMask and did not write the phrase
down had no way to retrieve it. Adds a "Show recovery phrase" action on
the wallet row in Settings, next to the per-wallet actions that already
live there, mirroring the per-address private key export in structure,
password gate and warning treatment.
The screen displays the secret that owns every address in the wallet, so:
- Only HD wallets are offered it. walletHasRecoveryPhrase() is an
allowlist on type "hd", so the key and xprv types — which have no
phrase at all — are excluded, as is any type added later.
- Nothing is decrypted and nothing enters the page until
decryptWithPassword accepts the password. A wrong password produces a
full-sentence error and leaves the value node empty.
- Leaving the screen wipes it by any route, not just "Back": views that
hold a secret register a cleanup with showView() via onViewLeave(),
which also covers the settings gear.
- A decrypt still in flight when the screen is left is discarded rather
than written. reveal() captures a generation counter that every
clear() bumps and refuses to touch the DOM if it has moved: without
that check the write lands after the wipe, and nothing is scheduled to
wipe again, so the phrase stays in the hidden screen for the life of
the popup. crypto_pwhash is synchronous, so the reachable window is a
still-pending sodium.ready on the first vault use of a page load.
- The phrase is never assigned to state, so it cannot be persisted, and
the view is not in RESTORABLE_VIEWS — reopening the popup lands on
Home. That set moves to src/popup/restorableViews.js so the exclusion
can be asserted directly; the popup entry point cannot be required
outside a browser.
- The phrase cannot reach the logger: the view does not import
src/shared/log.js, and the failed-decrypt path reports a fixed
sentence rather than the caught error.
show() also pushes the navigation stack itself, because it can return
without navigating when the wallet has no phrase; pushing in the Settings
click handler left an entry no screen transition matched.
Tests: unit coverage for the type gate, the RESTORABLE_VIEWS exclusion
and the absence of any logger path; the DOM behaviour is driven against
the real popup in the e2e suite, which is where this repo tests views —
including a probe that leaves the screen while the decrypt is in flight
by dispatching both clicks in one page task, since a human cannot
interleave them once libsodium's wasm is warm.
This commit is contained in:
@@ -255,6 +255,11 @@ async function openPopup(ctx, popupUrl) {
|
||||
|
||||
// Full wallet creation through the real UI: BIP-39 generation, libsodium
|
||||
// vault encryption and extension storage persistence, for real.
|
||||
//
|
||||
// Returns the recovery phrase it generated. Tests that assert on a secret
|
||||
// need the real value — checking for "some 12 words" would pass against the
|
||||
// wrong wallet's phrase, and checking for nothing at all would pass against
|
||||
// a screen that shows the phrase it was supposed to hide.
|
||||
async function createWallet(page) {
|
||||
await page.click("#btn-welcome-add");
|
||||
await visible(page, "#view-add-wallet");
|
||||
@@ -263,10 +268,12 @@ async function createWallet(page) {
|
||||
const el = document.getElementById("wallet-mnemonic");
|
||||
return el && el.value.trim().split(/\s+/).length >= 12;
|
||||
});
|
||||
const phrase = (await page.inputValue("#wallet-mnemonic")).trim();
|
||||
await page.fill("#add-wallet-password", PASSWORD);
|
||||
await page.fill("#add-wallet-password-confirm", PASSWORD);
|
||||
await page.click("#btn-add-wallet-confirm");
|
||||
await visible(page, "#view-main", 60000);
|
||||
return phrase;
|
||||
}
|
||||
|
||||
// Reach the address detail screen from wherever the popup restored to.
|
||||
@@ -281,6 +288,7 @@ async function openAddressDetail(page) {
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
PASSWORD,
|
||||
createWallet,
|
||||
launch,
|
||||
openAddressDetail,
|
||||
|
||||
264
tests/e2e/run.js
264
tests/e2e/run.js
@@ -10,6 +10,7 @@
|
||||
"use strict";
|
||||
|
||||
const {
|
||||
PASSWORD,
|
||||
createWallet,
|
||||
launch,
|
||||
openAddressDetail,
|
||||
@@ -34,6 +35,10 @@ function assert(cond, message) {
|
||||
if (!cond) throw new Error(message);
|
||||
}
|
||||
|
||||
function sleep(ms) {
|
||||
return new Promise((resolve) => setTimeout(resolve, ms));
|
||||
}
|
||||
|
||||
function withTimeout(promise, name) {
|
||||
let timer;
|
||||
const timeout = new Promise((_, reject) => {
|
||||
@@ -56,7 +61,11 @@ test("popup loads and reaches the welcome view", async (env) => {
|
||||
});
|
||||
|
||||
test("wallet creation through the UI reaches the main view", async (env) => {
|
||||
await createWallet(env.page);
|
||||
env.phrase = await createWallet(env.page);
|
||||
assert(
|
||||
env.phrase.split(/\s+/).length >= 12,
|
||||
"wallet creation did not yield a recovery phrase",
|
||||
);
|
||||
const addrCount = await env.page
|
||||
.locator("#wallet-list .btn-addr-info")
|
||||
.count();
|
||||
@@ -117,6 +126,256 @@ test("transaction detail renders an ERC-20 transfer (#151)", async (env) => {
|
||||
assert(dots > 0, "token contract row rendered without its colour dot");
|
||||
});
|
||||
|
||||
// -------------------------------------------- recovery phrase (#161)
|
||||
|
||||
// The gear toggles, so pressing it while Settings is already up leaves it.
|
||||
async function openSettings(page) {
|
||||
if (!(await page.isVisible("#view-settings"))) {
|
||||
await page.click("#btn-settings");
|
||||
}
|
||||
await visible(page, "#view-settings");
|
||||
}
|
||||
|
||||
// Everything the recovery phrase screen is holding, read straight out of
|
||||
// the DOM whether or not that screen is the one on top. Reading it while it
|
||||
// is hidden is the point: "cleared on leave" means the node is empty, not
|
||||
// merely off-screen.
|
||||
async function phraseScreenState(page) {
|
||||
return page.evaluate(() => ({
|
||||
value: document.getElementById("show-phrase-value").textContent,
|
||||
error: document.getElementById("show-phrase-flash").textContent,
|
||||
html: document.getElementById("view-show-phrase").innerHTML,
|
||||
resultHidden: document
|
||||
.getElementById("show-phrase-result")
|
||||
.classList.contains("hidden"),
|
||||
viewHidden: document
|
||||
.getElementById("view-show-phrase")
|
||||
.classList.contains("hidden"),
|
||||
}));
|
||||
}
|
||||
|
||||
async function openPhraseScreen(page) {
|
||||
await openSettings(page);
|
||||
await page.click("#settings-wallet-list .btn-show-phrase");
|
||||
await visible(page, "#view-show-phrase");
|
||||
}
|
||||
|
||||
async function revealPhrase(page) {
|
||||
await page.fill("#show-phrase-password", PASSWORD);
|
||||
await page.click("#btn-show-phrase-reveal");
|
||||
await visible(page, "#show-phrase-result", 60000);
|
||||
}
|
||||
|
||||
function assertWiped(st, phrase, where) {
|
||||
assert(st.value === "", "phrase still in the DOM " + where);
|
||||
assert(st.resultHidden, "result section still shown " + where);
|
||||
assert(
|
||||
!st.html.includes(phrase),
|
||||
"the recovery phrase is still somewhere in the screen markup " + where,
|
||||
);
|
||||
}
|
||||
|
||||
test("only an HD wallet is offered the recovery phrase action (#161)", async (env) => {
|
||||
await openSettings(env.page);
|
||||
const offered = await env.page
|
||||
.locator("#settings-wallet-list .btn-show-phrase")
|
||||
.count();
|
||||
const wallets = await env.page
|
||||
.locator("#settings-wallet-list .btn-delete-wallet")
|
||||
.count();
|
||||
assert(wallets === 1, "expected exactly one wallet row, got " + wallets);
|
||||
assert(
|
||||
offered === 1,
|
||||
"the HD wallet was not offered the recovery phrase action",
|
||||
);
|
||||
});
|
||||
|
||||
// The other half of the gate, against the real UI: a wallet holding a bare
|
||||
// private key has no phrase to show, so no row of it may offer the action.
|
||||
// The key is generated here rather than committed — the repo holds no
|
||||
// private keys, test ones included.
|
||||
test("a key wallet is not offered the recovery phrase action (#161)", async (env) => {
|
||||
const { Wallet } = require("ethers");
|
||||
|
||||
await openSettings(env.page);
|
||||
await env.page.click("#btn-main-add-wallet");
|
||||
await visible(env.page, "#view-add-wallet");
|
||||
await env.page.click("#tab-privkey");
|
||||
await env.page.fill(
|
||||
"#import-private-key",
|
||||
Wallet.createRandom().privateKey,
|
||||
);
|
||||
await env.page.fill("#add-wallet-password", PASSWORD);
|
||||
await env.page.fill("#add-wallet-password-confirm", PASSWORD);
|
||||
await env.page.click("#btn-add-wallet-confirm");
|
||||
await visible(env.page, "#view-main", 60000);
|
||||
|
||||
await openSettings(env.page);
|
||||
const wallets = await env.page
|
||||
.locator("#settings-wallet-list .btn-delete-wallet")
|
||||
.count();
|
||||
const offered = await env.page
|
||||
.locator("#settings-wallet-list .btn-show-phrase")
|
||||
.count();
|
||||
assert(wallets === 2, "expected two wallet rows, got " + wallets);
|
||||
assert(
|
||||
offered === 1,
|
||||
"the key wallet was offered the recovery phrase action",
|
||||
);
|
||||
});
|
||||
|
||||
test("the recovery phrase screen holds nothing before the password (#161)", async (env) => {
|
||||
await openPhraseScreen(env.page);
|
||||
const st = await phraseScreenState(env.page);
|
||||
assertWiped(st, env.phrase, "before any password was entered");
|
||||
const passwordShown = await env.page.isVisible(
|
||||
"#show-phrase-password-section",
|
||||
);
|
||||
assert(passwordShown, "the password prompt is not shown");
|
||||
});
|
||||
|
||||
test("a wrong password reveals nothing (#161)", async (env) => {
|
||||
await env.page.fill("#show-phrase-password", "not-the-password");
|
||||
await env.page.click("#btn-show-phrase-reveal");
|
||||
await env.page.waitForFunction(
|
||||
() =>
|
||||
document.getElementById("show-phrase-flash").textContent.length > 0,
|
||||
null,
|
||||
{ timeout: 60000 },
|
||||
);
|
||||
|
||||
const st = await phraseScreenState(env.page);
|
||||
assertWiped(st, env.phrase, "after a wrong password");
|
||||
assert(
|
||||
/^[A-Z].*\.$/.test(st.error.trim()),
|
||||
"the wrong-password error is not a full sentence: " +
|
||||
JSON.stringify(st.error),
|
||||
);
|
||||
});
|
||||
|
||||
test("the correct password reveals the full phrase, and nothing logs it (#161)", async (env) => {
|
||||
const console_ = [];
|
||||
const listener = (msg) => console_.push(msg.text());
|
||||
env.page.on("console", listener);
|
||||
try {
|
||||
await revealPhrase(env.page);
|
||||
|
||||
const st = await phraseScreenState(env.page);
|
||||
assert(
|
||||
st.value === env.phrase,
|
||||
"the displayed phrase is not the wallet's phrase, verbatim",
|
||||
);
|
||||
const promptShown = await env.page.isVisible(
|
||||
"#show-phrase-password-section",
|
||||
);
|
||||
assert(!promptShown, "the password prompt is still shown after unlock");
|
||||
|
||||
// Full Identifiers Policy: shown whole, and copyable.
|
||||
const title = await env.page.getAttribute(
|
||||
"#show-phrase-value",
|
||||
"title",
|
||||
);
|
||||
assert(title === "Click to copy", "the phrase is not click-to-copy");
|
||||
|
||||
const leaked = console_.filter((line) => line.includes(env.phrase));
|
||||
assert(
|
||||
leaked.length === 0,
|
||||
"the recovery phrase reached the console: " +
|
||||
JSON.stringify(leaked),
|
||||
);
|
||||
} finally {
|
||||
env.page.off("console", listener);
|
||||
}
|
||||
});
|
||||
|
||||
test('"Back" wipes the revealed phrase (#161)', async (env) => {
|
||||
await env.page.click("#btn-show-phrase-back");
|
||||
await visible(env.page, "#view-settings");
|
||||
const st = await phraseScreenState(env.page);
|
||||
assert(st.viewHidden, "the recovery phrase screen is still on top");
|
||||
assertWiped(st, env.phrase, "after Back");
|
||||
});
|
||||
|
||||
// The settings gear leaves the screen without touching its Back button. A
|
||||
// clear wired only to Back would pass the test above and leak here.
|
||||
test("leaving by the settings gear wipes it too (#161)", async (env) => {
|
||||
await openPhraseScreen(env.page);
|
||||
await revealPhrase(env.page);
|
||||
await env.page.click("#btn-settings");
|
||||
await visible(env.page, "#view-settings");
|
||||
const st = await phraseScreenState(env.page);
|
||||
assertWiped(st, env.phrase, "after leaving via the settings gear");
|
||||
});
|
||||
|
||||
// The same leave, but taken while the decrypt is still running. Both
|
||||
// clicks are dispatched inside one page task on purpose: "Reveal" runs its
|
||||
// handler up to the await, the gear then runs the leave — and the wipe with
|
||||
// it — to completion, and the decrypt's continuation resumes afterwards.
|
||||
// Without a liveness check that continuation writes the phrase into the
|
||||
// hidden screen after the wipe, and nothing is left to wipe it again.
|
||||
//
|
||||
// A human cannot produce this interleaving by hand once libsodium's wasm is
|
||||
// warm, because crypto_pwhash is synchronous and the only suspension point
|
||||
// is a microtask; the window a user can actually hit is a still-pending
|
||||
// sodium.ready on the first vault use of a page load. Forcing it here is
|
||||
// the only way to test the guard deterministically.
|
||||
test("leaving while the decrypt is in flight reveals nothing (#161)", async (env) => {
|
||||
await openPhraseScreen(env.page);
|
||||
await env.page.fill("#show-phrase-password", PASSWORD);
|
||||
await env.page.evaluate(() => {
|
||||
document.getElementById("btn-show-phrase-reveal").click();
|
||||
document.getElementById("btn-settings").click();
|
||||
});
|
||||
await visible(env.page, "#view-settings");
|
||||
|
||||
// The Reveal button is disabled for exactly the duration of the
|
||||
// decrypt and re-enabled in the same continuation that would have
|
||||
// written the phrase, so waiting for it to come back is a precise
|
||||
// "the decrypt has settled and its handler has finished" signal
|
||||
// rather than a guess at a duration.
|
||||
await env.page.waitForFunction(
|
||||
() => !document.getElementById("btn-show-phrase-reveal").disabled,
|
||||
null,
|
||||
{ timeout: 60000 },
|
||||
);
|
||||
await sleep(2000);
|
||||
|
||||
const st = await phraseScreenState(env.page);
|
||||
// Printed on every run, pass or fail: "the phrase is not there" is
|
||||
// worth more as a measurement than as a silent assertion, and the
|
||||
// same line read from a build without the guard is what this test
|
||||
// exists to prevent.
|
||||
console.log(
|
||||
"# probe: len=" +
|
||||
st.value.length +
|
||||
" equalsPhrase=" +
|
||||
(st.value === env.phrase) +
|
||||
" resultHidden=" +
|
||||
st.resultHidden +
|
||||
" viewHidden=" +
|
||||
st.viewHidden,
|
||||
);
|
||||
assert(st.viewHidden, "the recovery phrase screen is still on top");
|
||||
assertWiped(st, env.phrase, "after leaving mid-decrypt");
|
||||
});
|
||||
|
||||
// Closing and reopening the page rather than reloading it: that is what
|
||||
// the toolbar popup actually does, and the persisted currentView is
|
||||
// "show-phrase" at the moment it happens, which is precisely the state
|
||||
// RESTORABLE_VIEWS has to refuse.
|
||||
test("reopening the popup never lands on the phrase screen (#161)", async (env) => {
|
||||
await openPhraseScreen(env.page);
|
||||
await revealPhrase(env.page);
|
||||
|
||||
await env.page.close();
|
||||
env.page = await openPopup(env.ctx, env.popupUrl);
|
||||
await visible(env.page, "#view-main");
|
||||
|
||||
const st = await phraseScreenState(env.page);
|
||||
assert(st.viewHidden, "the popup reopened onto the recovery phrase screen");
|
||||
assertWiped(st, env.phrase, "after reopening the popup");
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------- runner
|
||||
|
||||
async function main() {
|
||||
@@ -154,6 +413,9 @@ async function main() {
|
||||
popupUrl: session.popupUrl,
|
||||
routeOpts,
|
||||
page: null,
|
||||
// The recovery phrase of the wallet created in test 2, so later
|
||||
// tests can assert on the real secret rather than its shape.
|
||||
phrase: null,
|
||||
};
|
||||
|
||||
// Attribution of collected errors is total. session.errors has no
|
||||
|
||||
94
tests/showPhrase.test.js
Normal file
94
tests/showPhrase.test.js
Normal file
@@ -0,0 +1,94 @@
|
||||
// Tests for the recovery phrase display (issue #161).
|
||||
//
|
||||
// These cover the parts that do not need a DOM: which wallet types may be
|
||||
// offered the action at all, the exclusion of the screen from the set of
|
||||
// views the popup may reopen onto, and the absence of any path from this
|
||||
// module to the logger. The DOM behaviour it guards — nothing rendered
|
||||
// before the password is accepted, a wrong password revealing nothing, and
|
||||
// the wipe on leaving — is driven against the real popup in a real browser
|
||||
// by tests/e2e/run.js, which is where every other view behaviour is tested.
|
||||
|
||||
const fs = require("fs");
|
||||
const path = require("path");
|
||||
|
||||
const { walletHasRecoveryPhrase } = require("../src/shared/wallet");
|
||||
const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews");
|
||||
|
||||
const SHOW_PHRASE_VIEW = "show-phrase";
|
||||
|
||||
// helpers.js pulls in state.js, which reads chrome.storage.local at load.
|
||||
function loadHelpers() {
|
||||
globalThis.chrome = {
|
||||
storage: { local: { get: async () => ({}), set: async () => {} } },
|
||||
};
|
||||
return require("../src/popup/views/helpers");
|
||||
}
|
||||
|
||||
describe("which wallets have a recovery phrase", () => {
|
||||
test("an HD wallet does", () => {
|
||||
expect(walletHasRecoveryPhrase({ type: "hd" })).toBe(true);
|
||||
});
|
||||
|
||||
// A key wallet holds a bare private key and an xprv wallet an extended
|
||||
// private key. Neither can be turned back into words, so neither may be
|
||||
// offered the action.
|
||||
test("a key wallet does not", () => {
|
||||
expect(walletHasRecoveryPhrase({ type: "key" })).toBe(false);
|
||||
});
|
||||
|
||||
test("an xprv wallet does not", () => {
|
||||
expect(walletHasRecoveryPhrase({ type: "xprv" })).toBe(false);
|
||||
});
|
||||
|
||||
test("an unknown or missing wallet type does not", () => {
|
||||
expect(walletHasRecoveryPhrase({ type: "something-new" })).toBe(false);
|
||||
expect(walletHasRecoveryPhrase({})).toBe(false);
|
||||
expect(walletHasRecoveryPhrase(undefined)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("views the popup may reopen onto", () => {
|
||||
// Restoring onto a secret screen would put the phrase on screen with no
|
||||
// password prompt in front of it, on a popup the user may have reopened
|
||||
// by accident.
|
||||
test("the recovery phrase screen is not restorable", () => {
|
||||
expect(RESTORABLE_VIEWS.has(SHOW_PHRASE_VIEW)).toBe(false);
|
||||
});
|
||||
|
||||
test("the private key export screen is not restorable either", () => {
|
||||
expect(RESTORABLE_VIEWS.has("export-privkey")).toBe(false);
|
||||
});
|
||||
|
||||
test("the recovery phrase screen is still a registered view", () => {
|
||||
const { VIEWS } = loadHelpers();
|
||||
expect(VIEWS).toContain(SHOW_PHRASE_VIEW);
|
||||
});
|
||||
|
||||
// Guards the other direction: a restorable name that is not a real view
|
||||
// would leave restoreView() showing nothing at all.
|
||||
test("every restorable view is a registered view", () => {
|
||||
const { VIEWS } = loadHelpers();
|
||||
for (const view of RESTORABLE_VIEWS) {
|
||||
expect(VIEWS).toContain(view);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("the phrase cannot reach the logger", () => {
|
||||
const source = fs.readFileSync(
|
||||
path.join(__dirname, "..", "src", "popup", "views", "showPhrase.js"),
|
||||
"utf8",
|
||||
);
|
||||
|
||||
// The decrypted phrase only ever lives in a local and in the DOM node
|
||||
// that displays it. The module has no logger to hand it to, and this
|
||||
// pins that: src/shared/log.js writes to the console, and a console
|
||||
// record of a recovery phrase outlives the popup.
|
||||
test("the view does not import src/shared/log.js", () => {
|
||||
expect(source).not.toMatch(/require\(["'][^"']*shared\/log["']\)/);
|
||||
});
|
||||
|
||||
test("the view calls no logger method", () => {
|
||||
expect(source).not.toMatch(/\blog\.(debugf|infof|warnf|errorf)\b/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user