fix: sign and export with each address's own derivation index (closes #510)

Sending, site transactions, site signatures and private key export
derived the key at the address's position in the wallet's list. After
an import whose scan passed over an unused index, or after removing an
address that was not the last, that position named another address's
key.

Each HD and xprv address record now stores the index it was derived at
(at wallet creation, by the import scan, by "+"). getSignerForAddress()
derives at that index, refuses a record without one, and refuses any key
whose address differs from the record's. The confirmation screen signs
as the From address it shows, not the selected one. The export screen
reports a refusal instead of calling the password wrong.

Model: opus-5-5

tests/addressKey.test.js

@@ -0,0 +1,490 @@
+// Sending, a site's transaction and signature requests and private key export
+// each use the key of the address they name: the key at the index that address
+// was derived at, not the one at its position in the wallet's list. The two
+// differ after an import whose scan passed over an unused index, and after an
+// address that is not the last is removed
+// (https://git.eeqj.de/sneak/AutistMask/issues/510). A send is signed by the
+// From address its confirmation screen shows, whichever address is selected.
+//
+// Driven through the real import, "+" button, address removal, confirmation
+// screen, transaction and signature approval screens and export screen, with
+// the node, the vault and the DOM stubbed.
+
+"use strict";
+
+// The addresses the stub node reports a balance for, and the signed
+// transactions it was handed.
+const mockNode = { funded: new Set(), broadcast: [] };
+
+jest.mock("ethers", () => {
+    const actual = jest.requireActual("ethers");
+    class StubProvider {
+        async getBalance(address) {
+            return mockNode.funded.has(address) ? 1n : 0n;
+        }
+        async getTransactionCount() {
+            return 0;
+        }
+        async estimateGas() {
+            return 21000n;
+        }
+        async getFeeData() {
+            return {
+                maxFeePerGas: 20000000000n,
+                maxPriorityFeePerGas: 1000000000n,
+                gasPrice: 10000000000n,
+            };
+        }
+        async getNetwork() {
+            return { chainId: 1n };
+        }
+        async getCode() {
+            return "0x";
+        }
+        async lookupAddress() {
+            return null;
+        }
+        async broadcastTransaction(signed) {
+            mockNode.broadcast.push(signed);
+            return { hash: "0x" + "ab".repeat(32) };
+        }
+    }
+    return { ...actual, JsonRpcProvider: StubProvider };
+});
+
+// No password is hashed: what the wallet stores is the phrase itself.
+jest.mock("../src/shared/vault", () => ({
+    encryptWithPassword: async (secret) => secret,
+    decryptWithPassword: async (secret) => secret,
+}));
+
+// The wait screen polls for a receipt; these tests stop at the broadcast.
+jest.mock("../src/popup/views/txStatus", () => ({
+    showWait: jest.fn(),
+    showError: jest.fn(),
+}));
+
+jest.mock("ethereum-blockies-base64", () => () => "data:image/png;base64,x");
+
+global.fetch = jest.fn(() => {
+    throw new Error("tests must not perform network requests");
+});
+
+const {
+    HDNodeWallet,
+    Mnemonic,
+    Transaction,
+    getBytes,
+    verifyMessage,
+} = require("ethers");
+
+// Publicly known development phrase; never fund it.
+const PHRASE = "test test test test test test test test test test test junk";
+// PHRASE's master extended private key, as the xprv import takes it.
+const XPRV = HDNodeWallet.fromSeed(
+    Mnemonic.fromPhrase(PHRASE).computeSeed(),
+).extendedKey;
+const PASSWORD = "correct horse battery staple";
+const RECIPIENT = "0xC0FfEE0000000000000000000000000000c0fFEe";
+// "Hello", as the hex a page sends.
+const MESSAGE = "0x48656c6c6f";
+
+// The address and key at derivation index `index` of PHRASE, worked out with
+// ethers directly rather than through the wallet's own code.
+function derived(index) {
+    return HDNodeWallet.fromPhrase(PHRASE, "", "m/44'/60'/0'/0/" + index);
+}
+
+// ------------------------------------------------------------ DOM stub
+
+function makeElement(id) {
+    const classes = new Set();
+    const el = {
+        id,
+        textContent: "",
+        title: "",
+        value: "",
+        innerHTML: "",
+        disabled: false,
+        style: {},
+        dataset: {},
+        listeners: {},
+        classList: {
+            add: (...names) => names.forEach((n) => classes.add(n)),
+            remove: (...names) => names.forEach((n) => classes.delete(n)),
+            contains: (n) => classes.has(n),
+            toggle: (n, force) => {
+                const on = force === undefined ? !classes.has(n) : force;
+                if (on) classes.add(n);
+                else classes.delete(n);
+                return on;
+            },
+        },
+        addEventListener: (name, fn) => {
+            el.listeners[name] = el.listeners[name] || [];
+            el.listeners[name].push(fn);
+        },
+        querySelectorAll: () => [],
+        appendChild: () => {},
+    };
+    // Views reach for .parentElement to hide whole sections.
+    Object.defineProperty(el, "parentElement", {
+        get: () => node(id + "-parent"),
+    });
+    return el;
+}
+
+function makeDocument() {
+    const els = new Map();
+    return {
+        getElementById(id) {
+            // The debug banner is created on demand by helpers.js; absent
+            // is the state a non-debug, non-testnet popup is in.
+            if (id === "debug-banner") return null;
+            if (!els.has(id)) els.set(id, makeElement(id));
+            return els.get(id);
+        },
+        createElement: () => makeElement("created"),
+        addEventListener: () => {},
+        body: { prepend: () => {} },
+    };
+}
+
+function node(id) {
+    return globalThis.document.getElementById(id);
+}
+
+function click(element) {
+    const event = { stopPropagation: () => {} };
+    return Promise.all((element.listeners.click || []).map((fn) => fn(event)));
+}
+
+// show() starts the fee estimate without awaiting it; this lets it settle.
+async function settle() {
+    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
+}
+
+// ------------------------------------------------------------ harness
+
+globalThis.chrome = {
+    storage: { local: { get: async () => ({}), set: async () => {} } },
+};
+
+const { state } = require("../src/shared/state");
+const { removeAddressFromState } = require("../src/shared/walletDelete");
+const addWallet = require("../src/popup/views/addWallet");
+const home = require("../src/popup/views/home");
+const confirmTx = require("../src/popup/views/confirmTx");
+const approval = require("../src/popup/views/approval");
+const exportPrivkey = require("../src/popup/views/exportPrivkey");
+const txStatus = require("../src/popup/views/txStatus");
+
+// What the background answers when the approval window asks which approval it
+// was opened for, and every message the popup sent it.
+let approvalDetails;
+let sent;
+
+// The "+" button of the wallet list's last render. Each render attaches the
+// handler to the buttons it drew, as the browser's fresh list would have.
+let plusButton;
+
+beforeEach(() => {
+    approvalDetails = null;
+    sent = [];
+    mockNode.broadcast = [];
+    globalThis.document = makeDocument();
+    globalThis.window = { close: () => {} };
+    globalThis.chrome = {
+        storage: { local: { get: async () => ({}), set: async () => {} } },
+        runtime: {
+            connect: () => ({ postMessage: () => {} }),
+            sendMessage: (msg, reply) => {
+                sent.push(msg);
+                if (!reply) return;
+                reply(
+                    msg.type === "AUTISTMASK_GET_APPROVAL"
+                        ? approvalDetails
+                        : null,
+                );
+            },
+        },
+    };
+    node("wallet-list").querySelectorAll = (selector) => {
+        if (selector !== ".btn-add-address") return [];
+        plusButton = makeElement("btn-add-address");
+        plusButton.dataset.wallet = "0";
+        return [plusButton];
+    };
+    // The explorer and price lookups these screens start fail offline, and
+    // say so; that is not under test.
+    for (const method of ["log", "warn", "error"]) {
+        jest.spyOn(console, method).mockImplementation(() => {});
+    }
+    txStatus.showError.mockClear();
+
+    state.wallets = [];
+    state.rpcUrl = "https://rpc.example.invalid";
+    state.networkId = "mainnet";
+    state.viewStack = [];
+    state.currentView = "main";
+
+    addWallet.init({
+        renderWalletList: () => {},
+        doRefreshAndRender: () => {},
+        pageClosed: new AbortController().signal,
+    });
+    confirmTx.init({});
+    approval.init({});
+    exportPrivkey.init();
+});
+
+afterEach(() => {
+    jest.restoreAllMocks();
+});
+
+// Import PHRASE on the add-wallet screen, with the stub node reporting a
+// balance at each of `fundedIndices`, and wait for the address scan.
+async function importPhrase(fundedIndices) {
+    mockNode.funded = new Set(fundedIndices.map((i) => derived(i).address));
+    await click(node("tab-mnemonic"));
+    node("wallet-mnemonic").value = PHRASE;
+    node("add-wallet-password").value = PASSWORD;
+    node("add-wallet-password-confirm").value = PASSWORD;
+    await click(node("btn-add-wallet-confirm"));
+}
+
+// The same, importing XPRV on the add-wallet screen's xprv tab.
+async function importXprv(fundedIndices) {
+    mockNode.funded = new Set(fundedIndices.map((i) => derived(i).address));
+    await click(node("tab-xprv"));
+    node("import-xprv-key").value = XPRV;
+    node("add-wallet-password").value = PASSWORD;
+    node("add-wallet-password-confirm").value = PASSWORD;
+    await click(node("btn-add-wallet-confirm"));
+}
+
+// Press the wallet's "+" button on Home.
+async function addAddress() {
+    home.render({
+        doRefreshAndRender: () => {},
+        pageClosed: new AbortController().signal,
+    });
+    await click(plusButton);
+}
+
+// The key the export screen reveals for the address at `position`.
+async function exportedKey(position) {
+    exportPrivkey.show(0, position);
+    node("export-privkey-password").value = PASSWORD;
+    await click(node("btn-export-privkey-confirm"));
+    return node("export-privkey-value").textContent;
+}
+
+// Confirm a send whose From line shows `from`, with the address at position
+// `selected` selected; resolves to the signed transaction the node was handed,
+// or undefined when none was.
+async function confirmSend(from, selected) {
+    state.selectedWallet = 0;
+    state.selectedAddress = selected;
+    confirmTx.show({
+        from,
+        to: RECIPIENT,
+        amount: "0.5",
+        token: "ETH",
+        balance: "1.0",
+    });
+    await settle();
+    node("confirm-tx-password").value = PASSWORD;
+    await click(node("btn-confirm-send"));
+    return mockNode.broadcast.pop();
+}
+
+// Send from the address at `position`, with that address selected, as the Send
+// button leaves it.
+function sendFrom(position) {
+    return confirmSend(state.wallets[0].addresses[position].address, position);
+}
+
+// A site asks the address at `position` to sign MESSAGE; resolves to the
+// response the approval window sent the background.
+async function signFor(position) {
+    const address = state.wallets[0].addresses[position].address;
+    approvalDetails = {
+        type: "sign",
+        origin: "https://dapp.example",
+        isPhishingDomain: false,
+        approvedFrom: address,
+        signParams: {
+            method: "personal_sign",
+            message: MESSAGE,
+            from: address,
+        },
+    };
+    await approval.show(1);
+    node("approve-sign-password").value = PASSWORD;
+    await click(node("btn-approve-sign"));
+    return sent.find((msg) => msg.type === "AUTISTMASK_SIGN_RESPONSE");
+}
+
+// A site asks the address at `position` to send RECIPIENT a transaction of no
+// value; resolves to the response the approval window sent the background.
+async function transactFor(position) {
+    const address = state.wallets[0].addresses[position].address;
+    approvalDetails = {
+        type: "tx",
+        origin: "https://dapp.example",
+        isPhishingDomain: false,
+        approvedFrom: address,
+        approvedTx: {
+            type: 2,
+            from: address,
+            chainId: "0x1",
+            nonce: "0x0",
+            gasLimit: "0x5208",
+            maxPriorityFeePerGas: "0x3b9aca00",
+            maxFeePerGas: "0x77359400",
+            to: RECIPIENT,
+            value: "0x0",
+            data: "0x",
+            accessList: [],
+        },
+    };
+    await approval.show(1);
+    node("approve-tx-password").value = PASSWORD;
+    await click(node("btn-approve-tx"));
+    return sent.find((msg) => msg.type === "AUTISTMASK_TX_RESPONSE");
+}
+
+describe.each([
+    [
+        "a recovery phrase import whose scan found only the first address",
+        async () => {
+            await importPhrase([0]);
+        },
+        // The address the import stores before its scan.
+        { position: 0, index: 0 },
+    ],
+    [
+        "an xprv import whose scan found only the first address",
+        async () => {
+            await importXprv([0]);
+        },
+        { position: 0, index: 0 },
+    ],
+    [
+        "an import whose scan passed over index 3",
+        async () => {
+            await importPhrase([0, 1, 2, 4]);
+        },
+        // Address 4 on Home, derived at index 4.
+        { position: 3, index: 4 },
+    ],
+    [
+        "an xprv import whose scan passed over index 3",
+        async () => {
+            await importXprv([0, 1, 2, 4]);
+        },
+        { position: 3, index: 4 },
+    ],
+    [
+        "removing the middle one of three addresses",
+        async () => {
+            await importPhrase([0]);
+            await addAddress();
+            await addAddress();
+            removeAddressFromState(state, 0, 1);
+        },
+        // Address 2 on Home after the removal, derived at index 2.
+        { position: 1, index: 2 },
+    ],
+])("after %s", (_name, setUp, { position, index }) => {
+    beforeEach(setUp);
+
+    test("the wallet lists the address derived at that index there", () => {
+        expect(state.wallets[0].addresses[position].address).toBe(
+            derived(index).address,
+        );
+    });
+
+    test("export reveals that address's key", async () => {
+        expect(await exportedKey(position)).toBe(derived(index).privateKey);
+    });
+
+    test("a send is signed by that address", async () => {
+        const signed = await sendFrom(position);
+        expect(Transaction.from(signed).from).toBe(derived(index).address);
+    });
+
+    test("a site's signature is made by that address", async () => {
+        const response = await signFor(position);
+        expect(verifyMessage(getBytes(MESSAGE), response.signature)).toBe(
+            derived(index).address,
+        );
+    });
+
+    test("a site's transaction is signed by that address", async () => {
+        const response = await transactFor(position);
+        expect(Transaction.from(response.rawSignedTx).from).toBe(
+            derived(index).address,
+        );
+    });
+});
+
+// The selection and the confirmation screen are saved apart, so the popup can
+// reopen onto the screen with another address selected than its From line
+// shows.
+describe("a confirmation screen whose From is not the selected address", () => {
+    beforeEach(async () => {
+        await importPhrase([0, 1]);
+    });
+
+    test("the send is signed by the From address shown", async () => {
+        const signed = await confirmSend(derived(1).address, 0);
+        expect(Transaction.from(signed).from).toBe(derived(1).address);
+    });
+
+    test("nothing is sent when the selected wallet does not hold it", async () => {
+        expect(await confirmSend(derived(5).address, 0)).toBeUndefined();
+        expect(node("confirm-tx-password-error").textContent).toBe(
+            "The selected wallet does not hold the From address above, " +
+                "so nothing was sent. Please go back and send again.",
+        );
+    });
+});
+
+describe("an address whose stored index derives a different address", () => {
+    function refusal() {
+        return (
+            "The key this wallet holds for " +
+            derived(0).address +
+            " belongs to a different address, so it was not used."
+        );
+    }
+
+    beforeEach(async () => {
+        await importPhrase([0]);
+        state.wallets[0].addresses[0].index = 1;
+    });
+
+    test("export reveals nothing and says why", async () => {
+        expect(await exportedKey(0)).toBe("");
+        expect(node("export-privkey-password-error").textContent).toBe(
+            refusal(),
+        );
+    });
+
+    test("nothing is sent", async () => {
+        expect(await sendFrom(0)).toBeUndefined();
+        expect(txStatus.showError).toHaveBeenCalledWith(
+            expect.anything(),
+            null,
+            refusal(),
+        );
+    });
+
+    test("nothing is signed", async () => {
+        const response = await signFor(0);
+        expect(response.signature).toBeUndefined();
+        expect(response.error).toBe(refusal());
+    });
+});

tests/approvalNonce.test.js

@@ -136,7 +136,9 @@ async function openTxApproval(approvedTx) {
             xpub: "xpub-wallet-1",
             encryptedSecret: "encrypted-secret-1",
             nextIndex: 1,
-            addresses: [{ address: FROM, balance: "0", tokenBalances: [] }],
+            addresses: [
+                { address: FROM, index: 0, balance: "0", tokenBalances: [] },
+            ],
         },
     ];
     approval.init({});

tests/approvalVerify.test.js

@@ -1454,7 +1454,11 @@ describe("background population to popup signing to verification", () => {
 
     // What the popup does with it: signs it as given, populating nothing.
     async function popupSigns(approvedTx) {
-        const localSigner = getSignerForAddress(walletData, 0, SIGNER_KEY);
+        const localSigner = getSignerForAddress(
+            walletData,
+            { address: signer.address },
+            SIGNER_KEY,
+        );
         return localSigner.signTransaction({ ...approvedTx });
     }
 
@@ -1549,7 +1553,11 @@ describe("background population to popup signing to verification", () => {
     // address the user has switched to.
     test("the approved from stops the popup signing with another key", async () => {
         const approvedTx = await backgroundPrepares(TX_PARAMS);
-        const otherSigner = getSignerForAddress(walletData, 0, OTHER_KEY);
+        const otherSigner = getSignerForAddress(
+            walletData,
+            { address: other.address },
+            OTHER_KEY,
+        );
         await expect(
             otherSigner.signTransaction({ ...approvedTx }),
         ).rejects.toThrow(/from address mismatch/);

tests/wallet.test.js

@@ -416,7 +416,7 @@ describe("extended key depth", () => {
         expect(() =>
             wallet.getSignerForAddress(
                 { type: "xprv" },
-                0,
+                { address: VECTOR_ADDRESSES[0], index: 0 },
                 accountXprv(VECTOR_PHRASE),
             ),
         ).toThrow(/master/i);
@@ -504,7 +504,7 @@ describe("getSignerForAddress", () => {
     test.each([0, 1, 2])("hd wallet, address index %i", (index) => {
         const signer = wallet.getSignerForAddress(
             { type: "hd" },
-            index,
+            { address: VECTOR_ADDRESSES[index], index },
             VECTOR_PHRASE,
         );
 
@@ -515,7 +515,7 @@ describe("getSignerForAddress", () => {
     test.each([0, 1, 2])("xprv wallet, address index %i", (index) => {
         const signer = wallet.getSignerForAddress(
             { type: "xprv" },
-            index,
+            { address: VECTOR_ADDRESSES[index], index },
             masterXprv(VECTOR_PHRASE),
         );
 
@@ -527,7 +527,7 @@ describe("getSignerForAddress", () => {
         for (const index of [0, 1, 2]) {
             const signer = wallet.getSignerForAddress(
                 { type: "privkey" },
-                index,
+                { address: VECTOR_ADDRESSES[1], index },
                 VECTOR_PRIVATE_KEYS[1],
             );
 
@@ -535,10 +535,54 @@ describe("getSignerForAddress", () => {
         }
     });
 
+    // https://git.eeqj.de/sneak/AutistMask/issues/510: an address whose stored
+    // index does not derive it, or a private key that is not its key.
+    test.each([
+        ["hd", { address: VECTOR_ADDRESSES[2], index: 1 }, VECTOR_PHRASE],
+        [
+            "xprv",
+            { address: VECTOR_ADDRESSES[2], index: 1 },
+            masterXprv(VECTOR_PHRASE),
+        ],
+        ["privkey", { address: VECTOR_ADDRESSES[2] }, VECTOR_PRIVATE_KEYS[1]],
+    ])(
+        "%s wallet refuses a key that is not the address's own",
+        (type, addr, secret) => {
+            expect(() =>
+                wallet.getSignerForAddress({ type }, addr, secret),
+            ).toThrow(
+                "The key this wallet holds for " +
+                    VECTOR_ADDRESSES[2] +
+                    " belongs to a different address, so it was not used.",
+            );
+        },
+    );
+
+    // An HD or xprv address record without a usable `index` is refused in a
+    // sentence, not with what ethers says about the value it was handed.
+    test.each([
+        ["hd", VECTOR_PHRASE],
+        ["xprv", masterXprv(VECTOR_PHRASE)],
+    ])("%s wallet refuses an address with no valid index", (type, secret) => {
+        for (const index of [undefined, null, -1, 1.5, "1"]) {
+            expect(() =>
+                wallet.getSignerForAddress(
+                    { type },
+                    { address: VECTOR_ADDRESSES[1], index },
+                    secret,
+                ),
+            ).toThrow(
+                "No derivation index is stored for " +
+                    VECTOR_ADDRESSES[1] +
+                    ", so its key cannot be derived.",
+            );
+        }
+    });
+
     test("the returned signer signs recoverably as the expected address", async () => {
         const signer = wallet.getSignerForAddress(
             { type: "hd" },
-            1,
+            { address: VECTOR_ADDRESSES[1], index: 1 },
             VECTOR_PHRASE,
         );
         const message = "AutistMask derivation test";

tests/walletDefects.test.js

@@ -55,7 +55,7 @@ function healthyXprvWallet(name = "Master") {
         xpub,
         nextIndex: 1,
         encryptedSecret: "irrelevant-to-these-tests",
-        addresses: [{ address: firstAddress, balance: "0.0000" }],
+        addresses: [{ address: firstAddress, index: 0, balance: "0.0000" }],
     };
 }
 
@@ -74,7 +74,11 @@ function brokenXprvWallet(name = "Imported xprv") {
         nextIndex: 1,
         encryptedSecret: "irrelevant-to-these-tests",
         addresses: [
-            { address: node.deriveChild(0).address, balance: "0.0000" },
+            {
+                address: node.deriveChild(0).address,
+                index: 0,
+                balance: "0.0000",
+            },
         ],
     };
 }
@@ -570,7 +574,7 @@ describe("no path throws an unhandled error for a defective wallet", () => {
         try {
             wallet.getSignerForAddress(
                 { type: "xprv" },
-                0,
+                brokenXprvWallet().addresses[0],
                 accountXprv(VECTOR_PHRASE),
             );
         } catch (e) {
@@ -585,7 +589,7 @@ describe("no path throws an unhandled error for a defective wallet", () => {
     test("a healthy xprv wallet signs as it always did", () => {
         const signer = wallet.getSignerForAddress(
             { type: "xprv" },
-            0,
+            healthyXprvWallet().addresses[0],
             masterXprv(VECTOR_PHRASE),
         );