fix: explain a stored non-master xprv wallet instead of throwing at signing time (closes #234)

src/popup/views/addressDetail.js

@@ -29,6 +29,16 @@ const { log } = require("../../shared/log");
 const makeBlockie = require("ethereum-blockies-base64");
 const { decryptWithPassword } = require("../../shared/vault");
 const { getSignerForAddress } = require("../../shared/wallet");
+const { walletDefect } = require("../../shared/walletDefects");
+
+// The defect of the wallet the selected address belongs to, or null. Both the
+// send and the private-key export path check it before asking for a password,
+// so a wallet that cannot derive its keys says so instead of failing after the
+// user has typed one in.
+function selectedWalletDefect() {
+    if (state.selectedWallet === null) return null;
+    return walletDefect(state.wallets[state.selectedWallet]);
+}
 
 let ctx;
 
@@ -254,6 +264,11 @@ function init(_ctx) {
     });
 
     $("btn-send").addEventListener("click", () => {
+        const defect = selectedWalletDefect();
+        if (defect) {
+            showFlash(defect.shortMessage);
+            return;
+        }
         const addr =
             state.wallets[state.selectedWallet].addresses[
                 state.selectedAddress
@@ -298,6 +313,14 @@ function init(_ctx) {
     $("btn-export-privkey").addEventListener("click", () => {
         moreDropdown.classList.add("hidden");
         moreBtn.classList.remove("bg-fg", "text-bg");
+        // There is no private key to export for an address this wallet
+        // cannot derive. Without this the export screen would take a
+        // password and then report it as wrong.
+        const defect = selectedWalletDefect();
+        if (defect) {
+            showFlash(defect.shortMessage);
+            return;
+        }
         pushCurrentView();
         const wallet = state.wallets[state.selectedWallet];
         const addr = wallet.addresses[state.selectedAddress];

src/popup/views/addressToken.js

@@ -35,6 +35,7 @@ const {
 } = require("./send");
 const { log } = require("../../shared/log");
 const makeBlockie = require("ethereum-blockies-base64");
+const { walletDefect } = require("../../shared/walletDefects");
 
 let ctx;
 
@@ -338,6 +339,11 @@ function init(_ctx) {
     });
 
     $("btn-address-token-send").addEventListener("click", () => {
+        const defect = walletDefect(state.wallets[state.selectedWallet]);
+        if (defect) {
+            showFlash(defect.shortMessage);
+            return;
+        }
         const addr =
             state.wallets[state.selectedWallet].addresses[
                 state.selectedAddress

src/popup/views/approval.js

@@ -21,6 +21,7 @@ const { ERC20_ABI } = require("../../shared/constants");
 const { TOKEN_BY_ADDRESS } = require("../../shared/tokenList");
 const { decryptWithPassword } = require("../../shared/vault");
 const { getSignerForAddress } = require("../../shared/wallet");
+const { walletDefect } = require("../../shared/walletDefects");
 const { getProvider } = require("../../shared/balances");
 const txStatus = require("./txStatus");
 const uniswap = require("../../shared/uniswap");
@@ -280,6 +281,7 @@ function showTxApproval(details) {
 
     showView("approve-tx");
     attachCopyHandlers("view-approve-tx");
+    gateOnWalletDefect("approve-tx-error", "btn-approve-tx");
 }
 
 function decodeHexMessage(hex) {
@@ -379,6 +381,7 @@ function showSignApproval(details) {
 
     showView("approve-sign");
     attachCopyHandlers("view-approve-sign");
+    gateOnWalletDefect("approve-sign-error", "btn-approve-sign");
 }
 
 function show(id) {
@@ -431,6 +434,20 @@ function setSignButtonBusy(busy) {
     $("btn-approve-sign").classList.toggle("text-muted", busy);
 }
 
+// Say so on the approval screen itself, and disable the approve button, when
+// the active address belongs to a wallet whose keys cannot be derived. Without
+// this the screen would take a password and fail after deriving it. Reject
+// stays available; the wallet is not touched. Returns true when it gated.
+function gateOnWalletDefect(errorId, buttonId) {
+    const active = findActiveWallet();
+    const defect = active ? walletDefect(active.wallet) : null;
+    if (!defect) return false;
+    showError(errorId, defect.shortMessage);
+    $(buttonId).disabled = true;
+    $(buttonId).classList.add("text-muted");
+    return true;
+}
+
 // Locate the wallet and the address index owning the currently active
 // address. Returns null when no wallet holds it.
 function findActiveWallet() {
@@ -492,6 +509,14 @@ function init(ctx) {
             return;
         }
 
+        const defect = walletDefect(active.wallet);
+        if (defect) {
+            password = null;
+            showError("approve-tx-error", defect.shortMessage);
+            setTxButtonBusy(false);
+            return;
+        }
+
         // Decrypt here, in the popup. The password must never cross the
         // extension messaging boundary; only the signed transaction does.
         let decryptedSecret;
@@ -583,6 +608,14 @@ function init(ctx) {
             return;
         }
 
+        const defect = walletDefect(active.wallet);
+        if (defect) {
+            password = null;
+            showError("approve-sign-error", defect.shortMessage);
+            setSignButtonBusy(false);
+            return;
+        }
+
         // Decrypt here, in the popup. The password must never cross the
         // extension messaging boundary; only the signature does.
         let decryptedSecret;

src/popup/views/home.js

@@ -21,6 +21,10 @@ const {
     resetSendValidation,
 } = require("./send");
 const { deriveAddressFromXpub } = require("../../shared/wallet");
+const {
+    walletDefect,
+    walletDefectHtml,
+} = require("../../shared/walletDefects");
 const {
     formatUsd,
     getPrice,
@@ -214,25 +218,23 @@ async function loadHomeTxs(ctx) {
     }
 }
 
-function render(ctx) {
-    const container = $("wallet-list");
-    if (state.wallets.length === 0) {
-        container.innerHTML =
-            '<p class="text-muted py-2">No wallets yet. Add one to get started.</p>';
-        renderTotalValue();
-        renderActiveAddress();
-        return;
-    }
-
+// The wallet list markup. Pure: it reads state and returns a string, so the
+// list can be asserted on without a DOM.
+function walletListHtml() {
     let html = "";
     state.wallets.forEach((wallet, wi) => {
+        const defect = walletDefect(wallet);
         html += `<div>`;
         html += `<div class="flex justify-between items-center bg-section py-1 px-2" style="margin:0 -0.5rem">`;
         html += `<span class="font-bold cursor-pointer wallet-name underline decoration-dashed" data-wallet="${wi}">${wallet.name}</span>`;
-        if (wallet.type === "hd" || wallet.type === "xprv") {
+        // No "+" on a defective wallet: deriving another address from that
+        // xpub would only add one more address the key does not produce
+        // under the standard path.
+        if (!defect && (wallet.type === "hd" || wallet.type === "xprv")) {
             html += `<button class="btn-add-address border border-border px-1 hover:bg-fg hover:text-bg cursor-pointer text-xs" data-wallet="${wi}" title="Add another address to this wallet">+</button>`;
         }
         html += `</div>`;
+        html += walletDefectHtml(wallet);
 
         wallet.addresses.forEach((addr, ai) => {
             html += `<div class="address-row py-1 border-b border-border-light cursor-pointer hover:bg-hover" data-wallet="${wi}" data-address="${ai}">`;
@@ -260,7 +262,20 @@ function render(ctx) {
 
         html += `</div>`;
     });
-    container.innerHTML = html;
+    return html;
+}
+
+function render(ctx) {
+    const container = $("wallet-list");
+    if (state.wallets.length === 0) {
+        container.innerHTML =
+            '<p class="text-muted py-2">No wallets yet. Add one to get started.</p>';
+        renderTotalValue();
+        renderActiveAddress();
+        return;
+    }
+
+    container.innerHTML = walletListHtml();
 
     container.querySelectorAll(".address-row").forEach((row) => {
         row.addEventListener("click", async () => {
@@ -348,6 +363,13 @@ function render(ctx) {
     loadHomeTxs(ctx);
 }
 
+// The defect of the wallet the selected address belongs to, or null. Call
+// after selectActiveAddress().
+function selectedWalletDefect() {
+    if (state.selectedWallet === null) return null;
+    return walletDefect(state.wallets[state.selectedWallet]);
+}
+
 function selectActiveAddress() {
     for (let wi = 0; wi < state.wallets.length; wi++) {
         for (let ai = 0; ai < state.wallets[wi].addresses.length; ai++) {
@@ -371,6 +393,13 @@ function init(ctx) {
             showFlash("No active address selected.");
             return;
         }
+        // Before the balance check and before any password is asked for: this
+        // wallet cannot sign at all, so the send screen is a dead end.
+        const defect = selectedWalletDefect();
+        if (defect) {
+            showFlash(defect.shortMessage);
+            return;
+        }
         const addr = currentAddress();
         if (!addr.balance || parseFloat(addr.balance) === 0) {
             showFlash("Cannot send \u2014 zero balance.");
@@ -396,4 +425,4 @@ function init(ctx) {
     });
 }
 
-module.exports = { init, render };
+module.exports = { init, render, walletListHtml };

src/shared/wallet.js

@@ -120,9 +120,24 @@ function getSignerForAddress(walletData, addrIndex, decryptedSecret) {
         return node.deriveChild(addrIndex);
     }
     if (walletData.type === "xprv") {
-        const node =
-            masterXprvOrThrow(decryptedSecret).derivePath(BIP44_ETH_PATH);
-        return node.deriveChild(addrIndex);
+        // Checked here rather than through masterXprvOrThrow so the message
+        // fits the situation: nobody is importing anything at signing time,
+        // and this wallet is already in storage. src/shared/walletDefects.js
+        // catches it at list-render time; this is the backstop behind that.
+        const node = parseExtendedKey(decryptedSecret);
+        if (!node || !node.privateKey) {
+            throw new Error(
+                "This wallet's stored key is not a valid extended private " +
+                    "key, so it cannot sign.",
+            );
+        }
+        if (node.depth !== MASTER_DEPTH) {
+            throw new Error(
+                "This wallet was imported from an extended private key that " +
+                    "is not a master key, so it cannot sign.",
+            );
+        }
+        return node.derivePath(BIP44_ETH_PATH).deriveChild(addrIndex);
     }
     return new Wallet(decryptedSecret);
 }
@@ -142,6 +157,7 @@ function walletHasRecoveryPhrase(walletData) {
 
 module.exports = {
     generateMnemonic,
+    parseExtendedKey,
     deriveAddressFromXpub,
     hdWalletFromMnemonic,
     hdWalletFromXprv,

src/shared/walletDefects.js

@@ -0,0 +1,86 @@
+// Wallets already in stored state whose key cannot be used, and the copy that
+// explains them.
+//
+// Refusing a non-master extended private key at import time does nothing for a
+// wallet imported before that refusal existed. Such a wallet is detected here,
+// at wallet-list render time, so the user meets the explanation on the list
+// screen rather than an exception on the send screen. Nothing here modifies or
+// removes a wallet: the record is the user's data.
+
+const { parseExtendedKey } = require("./wallet");
+
+const NON_MASTER_XPRV = "non-master-xprv";
+
+// An "xprv" wallet stores the neutered BIP-44 Ethereum node, four levels below
+// the key that was imported: the current import path derives the absolute
+// m/44'/60'/0'/0 from a depth-0 key, and the pre-#210 path derived the same
+// four levels as a relative path beneath whatever depth it was given. A master
+// import therefore stores a depth-4 xpub and a depth-d import stores depth
+// d + 4, which makes the stored xpub an exact read on the imported key's
+// depth — and it is readable without the password, unlike the key itself.
+const BIP44_ETH_XPUB_DEPTH = 4;
+
+const DEFECTS = {
+    [NON_MASTER_XPRV]: {
+        id: NON_MASTER_XPRV,
+        heading: "This wallet's addresses were derived incorrectly.",
+        paragraphs: [
+            "This wallet was imported from an extended private key that is " +
+                "not a master key. An earlier version applied the Ethereum " +
+                "derivation path beneath that key instead of from a master " +
+                "key, so the addresses listed here are not the ones that key " +
+                "produces under the standard path.",
+            "Signing and sending are disabled for this wallet. The addresses " +
+                "do descend from the extended private key you imported, so " +
+                "anything they hold is still reachable by software that " +
+                "repeats the same non-standard derivation. Check them in a " +
+                "block explorer before deciding what to do.",
+            "To see the addresses this key produces under the standard path, " +
+                "import the master extended private key, or the recovery " +
+                "phrase it came from, as a new wallet. Nothing here has been " +
+                "changed or removed, and this wallet stays until you delete " +
+                "it yourself.",
+        ],
+        // One sentence for the places that have room for one: the flash on a
+        // blocked Send, the inline error on the approval screens.
+        shortMessage:
+            "This wallet cannot sign, because it was imported from an " +
+            "extended private key that is not a master key. The wallet list " +
+            "explains what happened.",
+    },
+};
+
+// The defect record for a wallet, or null if there is nothing wrong with it
+// that this module can see. Read-only.
+//
+// A wallet whose xpub will not parse gets null rather than a defect: there is
+// no basis in that case to tell the user their key was not a master key, and a
+// wrong explanation is worse than none.
+function walletDefect(walletData) {
+    if (!walletData || walletData.type !== "xprv") return null;
+    const node = parseExtendedKey(walletData.xpub);
+    if (!node) return null;
+    if (node.depth === BIP44_ETH_XPUB_DEPTH) return null;
+    return DEFECTS[NON_MASTER_XPRV];
+}
+
+// The notice block for the wallet list, or "" for a wallet with no defect.
+// The copy is fixed text from this module, so it needs no escaping.
+function walletDefectHtml(walletData) {
+    const defect = walletDefect(walletData);
+    if (!defect) return "";
+    let html =
+        '<div class="border border-red-500 border-dashed p-2 my-1 text-xs text-red-500">';
+    html += `<div class="font-bold mb-1">${defect.heading}</div>`;
+    for (const p of defect.paragraphs) {
+        html += `<p class="mb-1">${p}</p>`;
+    }
+    html += "</div>";
+    return html;
+}
+
+module.exports = {
+    NON_MASTER_XPRV,
+    walletDefect,
+    walletDefectHtml,
+};