feat: password-gated recovery phrase display for HD wallets (closes #161)

tests/e2e/harness.js

@@ -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,

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

tests/showPhrase.test.js

@@ -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/);
+    });
+});