fix: enforce the base58 checksum and reject non-master extended keys (closes #210)

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", () => {