From 57e1fd019832a55bfca0e3dc53fec97ca972de19 Mon Sep 17 00:00:00 2001
From: clawbot <clawbot@eeqj.de>
Date: Tue, 11 Aug 2026 13:14:35 +0000
Subject: [PATCH] fix: enforce the base58 checksum and reject non-master
 extended keys (closes #210)

Two ways an xprv import could silently produce a wallet that is not the
user's, with no error shown.

Checksum: ethers' HDNodeWallet.fromExtendedKey skips base58 checksum
verification when the decoded payload is the usual 82 bytes, which is
exactly the case the checksum exists to catch. Every extended key now
goes through parseExtendedKey, which re-encodes the parsed node and
requires the round trip to reproduce the input exactly. Over every
single-character substitution of the BIP-32 vector 1 master key, 199
parsed before and 0 parse now; the xpub path shared the hole (101
accepted typos) and is fixed by the same helper.

Depth: hdWalletFromXprv and getSignerForAddress derived the relative
path 44'/60'/0'/0, which is the BIP-44 Ethereum account path only for a
depth-0 master key. Under an account-level (depth-3) key it derived
m/44'/60'/0'/44'/60'/0'/0 instead of refusing. Both now derive the
absolute BIP44_ETH_PATH from a key checked to be at depth 0. The
extended-key format carries depth, parent fingerprint and child index
but not the path a key sits at, so a non-master key cannot be placed in
the tree without guessing; it is rejected, and the import screen says
an account-level or child key is not supported.

Import errors are full sentences per the README Language & Labeling
rules, and the skipped one-character-typo test is unskipped.
---
 TODO.md                      |   4 +
 src/popup/index.html         |   4 +-
 src/popup/views/addWallet.js |  16 +++-
 src/shared/wallet.js         |  86 +++++++++++++++----
 tests/wallet.test.js         | 158 ++++++++++++++++++++++++++++++++---
 5 files changed, 238 insertions(+), 30 deletions(-)

diff --git a/TODO.md b/TODO.md
index 8b17986..985e5b6 100644
--- a/TODO.md
+++ b/TODO.md
@@ -57,6 +57,10 @@ undefined identifiers, which is how
   from the wallet row in Settings, wiped on leaving the screen and excluded from
   the views the popup can reopen onto
   ([#161](https://git.eeqj.de/sneak/AutistMask/issues/161)).
+- 2026-08-11: Extended-key import hardened — the base58 checksum is now enforced
+  on every xprv and xpub, and a non-master key is refused with an explanation
+  instead of being derived beneath
+  ([#210](https://git.eeqj.de/sneak/AutistMask/issues/210)).
 - 2026-08-11: Policy compliance sweep — conditional verbose test rerun, local
   Tailwind binary instead of `npx`, `--frozen-lockfile` on `make install`, and
   the Makefile-only targets documented in the README
diff --git a/src/popup/index.html b/src/popup/index.html
index 361e864..b40e098 100644
--- a/src/popup/index.html
+++ b/src/popup/index.html
@@ -136,7 +136,9 @@
                 <div id="add-wallet-section-xprv" class="hidden">
                     <p class="mb-2">
                         Paste your extended private key (xprv) below. This will
-                        import the HD wallet and scan for used addresses.
+                        import the HD wallet and scan for used addresses. It
+                        must be the master key for the wallet; an account-level
+                        or child key is not supported.
                     </p>
                     <div class="mb-2">
                         <input
diff --git a/src/popup/views/addWallet.js b/src/popup/views/addWallet.js
index 5a746e1..063bbbe 100644
--- a/src/popup/views/addWallet.js
+++ b/src/popup/views/addWallet.js
@@ -6,6 +6,7 @@ const {
     addressFromPrivateKey,
     hdWalletFromXprv,
     isValidXprv,
+    isMasterExtendedKey,
 } = require("../../shared/wallet");
 const { encryptWithPassword } = require("../../shared/vault");
 const { state, saveState } = require("../../shared/state");
@@ -213,14 +214,25 @@ async function importXprvKey(ctx) {
         return;
     }
     if (!isValidXprv(xprv)) {
-        showFlash("Invalid extended private key.");
+        showFlash(
+            "That extended private key is not valid. Please check it and try again.",
+        );
+        return;
+    }
+    if (!isMasterExtendedKey(xprv)) {
+        showFlash(
+            "That is an account-level or child key, which cannot be imported. " +
+                "Please paste the master extended private key for the wallet.",
+        );
         return;
     }
     let result;
     try {
         result = hdWalletFromXprv(xprv);
     } catch (e) {
-        showFlash("Invalid extended private key.");
+        showFlash(
+            "That extended private key is not valid. Please check it and try again.",
+        );
         return;
     }
     const { xpub, firstAddress } = result;
diff --git a/src/shared/wallet.js b/src/shared/wallet.js
index 66d760a..0d98a96 100644
--- a/src/shared/wallet.js
+++ b/src/shared/wallet.js
@@ -16,8 +16,60 @@ function generateMnemonic() {
     return m.phrase;
 }
 
+// Every extended key (xprv or xpub) entering the app goes through this.
+//
+// ethers' HDNodeWallet.fromExtendedKey does NOT verify the base58 checksum
+// when the decoded payload is the usual 82 bytes, which is exactly the case
+// the checksum exists to catch: a key with a one-character typo parses into a
+// *different* wallet instead of being rejected. Re-encoding the parsed node
+// reproduces a well-formed key byte for byte, checksum included, so comparing
+// the round trip against the input rejects any altered character. Measured by
+// the sweep in tests/wallet.test.js over every single-character substitution
+// of the BIP-32 vector 1 master key: 199 parse without the round-trip
+// comparison, 0 with it.
+//
+// Returns the parsed node, or null if the key is not a well-formed extended
+// key. Callers turn null into a user-facing error; none of them may fall back
+// to fromExtendedKey directly.
+function parseExtendedKey(key) {
+    if (typeof key !== "string") return null;
+    try {
+        const node = HDNodeWallet.fromExtendedKey(key);
+        return node.extendedKey === key ? node : null;
+    } catch {
+        return null;
+    }
+}
+
+// A master key is at depth 0. Only from there is BIP44_ETH_PATH the absolute
+// path it names; deriving it under an account-level or child key yields
+// addresses that correspond to nothing the user holds.
+const MASTER_DEPTH = 0;
+
+// Parse an extended private key that the BIP-44 Ethereum account path can be
+// derived from, or throw. Both callers derive BIP44_ETH_PATH from the result.
+function masterXprvOrThrow(key) {
+    const node = parseExtendedKey(key);
+    if (!node) {
+        throw new Error("Not a valid extended private key (xprv).");
+    }
+    if (!node.privateKey) {
+        throw new Error("Not an extended private key (xprv).");
+    }
+    if (node.depth !== MASTER_DEPTH) {
+        throw new Error(
+            "Not a master extended private key (xprv): an account-level or " +
+                "child key cannot be imported.",
+        );
+    }
+    return node;
+}
+
 function deriveAddressFromXpub(xpub, index) {
-    const node = HDNodeWallet.fromExtendedKey(xpub);
+    const node = parseExtendedKey(xpub);
+    if (!node) {
+        throw new Error("Not a valid extended key.");
+    }
     return node.deriveChild(index).address;
 }
 
@@ -29,23 +81,28 @@ function hdWalletFromMnemonic(mnemonic) {
 }
 
 function hdWalletFromXprv(xprv) {
-    const root = HDNodeWallet.fromExtendedKey(xprv);
-    if (!root.privateKey) {
-        throw new Error("Not an extended private key (xprv).");
-    }
-    const node = root.derivePath("44'/60'/0'/0");
+    // BIP44_ETH_PATH is absolute ("m/..."), which ethers will only derive from
+    // a depth-0 node. The relative form this used to derive would have been
+    // applied *beneath* an account-level key instead of being refused.
+    const node = masterXprvOrThrow(xprv).derivePath(BIP44_ETH_PATH);
     const xpub = node.neuter().extendedKey;
     const firstAddress = node.deriveChild(0).address;
     return { xpub, firstAddress };
 }
 
+// Well-formed extended private key. Says nothing about depth: the import view
+// reports a non-master key separately, since "check it for a typo" is the
+// wrong advice for a key the user copied correctly.
 function isValidXprv(key) {
-    try {
-        const node = HDNodeWallet.fromExtendedKey(key);
-        return !!node.privateKey;
-    } catch {
-        return false;
-    }
+    const node = parseExtendedKey(key);
+    return !!(node && node.privateKey);
+}
+
+// Whether an extended key is a master key, i.e. the one BIP44_ETH_PATH can be
+// derived from. False for anything parseExtendedKey rejects.
+function isMasterExtendedKey(key) {
+    const node = parseExtendedKey(key);
+    return !!node && node.depth === MASTER_DEPTH;
 }
 
 function addressFromPrivateKey(key) {
@@ -63,8 +120,8 @@ function getSignerForAddress(walletData, addrIndex, decryptedSecret) {
         return node.deriveChild(addrIndex);
     }
     if (walletData.type === "xprv") {
-        const root = HDNodeWallet.fromExtendedKey(decryptedSecret);
-        const node = root.derivePath("44'/60'/0'/0");
+        const node =
+            masterXprvOrThrow(decryptedSecret).derivePath(BIP44_ETH_PATH);
         return node.deriveChild(addrIndex);
     }
     return new Wallet(decryptedSecret);
@@ -89,6 +146,7 @@ module.exports = {
     hdWalletFromMnemonic,
     hdWalletFromXprv,
     isValidXprv,
+    isMasterExtendedKey,
     addressFromPrivateKey,
     getSignerForAddress,
     isValidMnemonic,
diff --git a/tests/wallet.test.js b/tests/wallet.test.js
index 9054552..0adf376 100644
--- a/tests/wallet.test.js
+++ b/tests/wallet.test.js
@@ -160,6 +160,31 @@ function masterXprv(phrase, passphrase = "") {
     ).extendedKey;
 }
 
+// The account-level (depth-3) extended private key m/44'/60'/0' for a phrase.
+// A normal thing for a user to hold, and not something the import flow can
+// derive the BIP-44 account path from.
+function accountXprv(phrase) {
+    return HDNodeWallet.fromSeed(
+        Mnemonic.fromPhrase(phrase, "").computeSeed(),
+    ).derivePath("m/44'/60'/0'").extendedKey;
+}
+
+// Every single-character substitution of `key`, using base58 characters that
+// are not the original. Base58 has no visually ambiguous characters, so each
+// of these is a plausible typo rather than a contrived string.
+const TYPO_CHARS = ["a", "b", "2", "Z"];
+
+function singleCharacterTypos(key) {
+    const out = [];
+    for (let i = 0; i < key.length; i++) {
+        for (const c of TYPO_CHARS) {
+            if (c === key[i]) continue;
+            out.push(key.slice(0, i) + c + key.slice(i + 1));
+        }
+    }
+    return out;
+}
+
 describe("hdWalletFromMnemonic", () => {
     test("first address matches the published vector for m/44'/60'/0'/0/0", () => {
         expect(wallet.hdWalletFromMnemonic(VECTOR_PHRASE).firstAddress).toBe(
@@ -299,19 +324,7 @@ describe("isValidXprv", () => {
         expect(wallet.isValidXprv(xpub)).toBe(false);
     });
 
-    // Skipped: this asserts the correct behaviour, which the code does not
-    // currently have. isValidXprv gates the paste-your-extended-private-key
-    // import in src/popup/views/addWallet.js:215, and it accepts a key with a
-    // one-character typo: ethers' HDNodeWallet.fromExtendedKey skips base58
-    // checksum verification whenever the decoded payload is the usual 82
-    // bytes, which is the whole point of that checksum. Measured on this
-    // vector: changing any one of the last 14 characters passes validation,
-    // and for 9 of those 14 positions the import silently yields a *different*
-    // wallet (e.g. 0x3F334f0a356d6B46B1d70B590E7437D77100d28D instead of
-    // 0x022b971dFF0C43305e691DEd7a14367AF19D6407) with no error shown.
-    // Tracked as https://git.eeqj.de/sneak/AutistMask/issues/210; out of scope
-    // here, which is tests only. Unskip when it is fixed.
-    test.skip("rejects an extended key with a one-character typo", () => {
+    test("rejects an extended key with a one-character typo", () => {
         const index = BIP32_VECTOR_1_XPRV.length - 8;
         const typo =
             BIP32_VECTOR_1_XPRV.slice(0, index) +
@@ -320,6 +333,125 @@ describe("isValidXprv", () => {
 
         expect(wallet.isValidXprv(typo)).toBe(false);
     });
+
+    // The base58 checksum exists to make a mistyped key impossible to use, and
+    // ethers does not enforce it: HDNodeWallet.fromExtendedKey skips checksum
+    // verification whenever the decoded payload is the usual 82 bytes, which
+    // is precisely the case it is there to catch. A typo anywhere in the key
+    // must be refused, not silently turned into someone else's wallet.
+    test("no single-character typo anywhere in the key is accepted", () => {
+        const accepted = singleCharacterTypos(BIP32_VECTOR_1_XPRV).filter(
+            (typo) => wallet.isValidXprv(typo),
+        );
+
+        expect(accepted).toEqual([]);
+    });
+
+    test("a typo never yields a wallet, let alone a different one", () => {
+        const correct = wallet.hdWalletFromXprv(BIP32_VECTOR_1_XPRV);
+        const derived = [];
+        for (const typo of singleCharacterTypos(BIP32_VECTOR_1_XPRV)) {
+            try {
+                derived.push(wallet.hdWalletFromXprv(typo).firstAddress);
+            } catch {
+                // Rejected, which is the required behaviour.
+            }
+        }
+
+        expect(derived).toEqual([]);
+        expect(correct.firstAddress).toBe(
+            "0x022b971dFF0C43305e691DEd7a14367AF19D6407",
+        );
+    });
+});
+
+describe("extended key depth", () => {
+    // hdWalletFromXprv derives the BIP-44 Ethereum account path from the key
+    // it is given. That is only the path it names when the key is the master
+    // key. Under an account-level key the same derivation lands at
+    // m/44'/60'/0'/44'/60'/0'/0, whose addresses correspond to nothing the
+    // user holds, so a non-master key is refused rather than derived from.
+    test("a master key is a master key", () => {
+        expect(wallet.isMasterExtendedKey(masterXprv(VECTOR_PHRASE))).toBe(
+            true,
+        );
+        expect(wallet.isMasterExtendedKey(BIP32_VECTOR_1_XPRV)).toBe(true);
+    });
+
+    test("an account-level key is not a master key", () => {
+        expect(wallet.isMasterExtendedKey(accountXprv(VECTOR_PHRASE))).toBe(
+            false,
+        );
+    });
+
+    test("a derived xpub is not a master key", () => {
+        expect(
+            wallet.isMasterExtendedKey(
+                wallet.hdWalletFromMnemonic(VECTOR_PHRASE).xpub,
+            ),
+        ).toBe(false);
+    });
+
+    test("a mistyped key is not a master key either", () => {
+        expect(wallet.isMasterExtendedKey(BIP32_VECTOR_1_XPRV + "a")).toBe(
+            false,
+        );
+    });
+
+    test("hdWalletFromXprv rejects an account-level key", () => {
+        expect(() =>
+            wallet.hdWalletFromXprv(accountXprv(VECTOR_PHRASE)),
+        ).toThrow(/master/i);
+    });
+
+    test("getSignerForAddress rejects an account-level key", () => {
+        expect(() =>
+            wallet.getSignerForAddress(
+                { type: "xprv" },
+                0,
+                accountXprv(VECTOR_PHRASE),
+            ),
+        ).toThrow(/master/i);
+    });
+
+    test("the account-level key is well-formed, so only depth rejects it", () => {
+        expect(wallet.isValidXprv(accountXprv(VECTOR_PHRASE))).toBe(true);
+    });
+
+    test("a master key still imports and derives the published addresses", () => {
+        const { xpub, firstAddress } = wallet.hdWalletFromXprv(
+            masterXprv(VECTOR_PHRASE),
+        );
+
+        expect(firstAddress).toBe(VECTOR_ADDRESSES[0]);
+        expect(
+            [0, 1, 2].map((i) => wallet.deriveAddressFromXpub(xpub, i)),
+        ).toEqual(VECTOR_ADDRESSES);
+    });
+});
+
+describe("deriveAddressFromXpub checksum enforcement", () => {
+    // The xpub path shares the hole: fromExtendedKey accepts a mistyped xpub
+    // just as readily, and deriveAddressFromXpub would hand back addresses
+    // from a different tree.
+    const { xpub } = wallet.hdWalletFromMnemonic(VECTOR_PHRASE);
+
+    test("the correct xpub still derives the published addresses", () => {
+        expect(wallet.deriveAddressFromXpub(xpub, 0)).toBe(VECTOR_ADDRESSES[0]);
+    });
+
+    test("no single-character typo anywhere in an xpub is accepted", () => {
+        const derived = [];
+        for (const typo of singleCharacterTypos(xpub)) {
+            try {
+                derived.push(wallet.deriveAddressFromXpub(typo, 0));
+            } catch {
+                // Rejected, which is the required behaviour.
+            }
+        }
+
+        expect(derived).toEqual([]);
+    });
 });
 
 describe("isValidMnemonic", () => {
-- 
2.49.1