diff --git a/README.md b/README.md index 54a5f07..443a08a 100644 --- a/README.md +++ b/README.md @@ -1192,7 +1192,16 @@ The core hierarchy is **Wallets → Addresses**: of its stored `xpub` by `src/shared/walletDefects.js`, explained in the wallet list, and blocked from signing, sending and private-key export. It is never deleted or rewritten. -- An **address** holds ETH and ERC-20 tokens. +- An **address** holds ETH and ERC-20 tokens. In an HD or xprv wallet each + address record also stores `index`, the derivation index it was made at: 0 for + the first address, the index the import scan found it at, or `nextIndex` when + "+" added it. An address's `index` and its position in the wallet's list + differ once the scan has passed over an unused index or an address has been + removed. Sending, site transactions, site signatures and private-key export + derive the key at `index`, and refuse it unless it is that address's own key; + an HD or xprv address with no valid `index` is refused before anything is + derived (`getSignerForAddress()` in `src/shared/wallet.js`). Each refusal is a + full sentence naming the address. - The user can have multiple wallets, each with multiple addresses (HD) or a single address (key). @@ -1518,6 +1527,9 @@ view would leave a wallet one click from deletion. change) - "Reveal" (wrong password) → full-sentence error on the error line, nothing revealed (no screen change) + - "Reveal" (correct password, but this address's key is refused, see Data + Model) → full-sentence error saying why, nothing revealed (no screen + change) - "Back" → previous screen (AddressDetail) - Settings gear → **Settings**, whose "Back" goes to AddressDetail: leaving drops the address the screen was showing, so it also takes the screen off @@ -1647,6 +1659,13 @@ view would leave a wallet one click from deletion. value (`src/shared/transferAmount.js`) - "Sign & Send" (wrong password) → "Wrong password." on the password error line, no screen change + - "Sign & Send" signs as the From address above, not as whichever address is + selected: the two are saved separately, so the popup can reopen onto this + screen with another address selected. When the selected wallet does not + hold the From address → full-sentence error on the password error line, + nothing sent (no screen change) + - "Sign & Send" (correct password, but the From address's key is refused, + see Data Model) → nothing is signed → **ErrorTx** saying why - "Back" → **Send** #### WaitTx (`wait-tx`) @@ -2110,6 +2129,9 @@ view would leave a wallet one click from deletion. shown, exactly as shown, hands the signed transaction to the background to broadcast, then → **WaitTx** in the same popup window - "Confirm" (wrong password) → error line, no screen change + - "Confirm" (correct password, but the From address's key is refused, see + Data Model) → full-sentence error on the error line saying why, nothing + signed (no screen change) - "Reject" → closes popup (returns rejection to background) - Popup window closed without answering → the request is rejected with EIP-1193 code 4001 diff --git a/src/popup/views/addWallet.js b/src/popup/views/addWallet.js index b6e0c3e..660061a 100644 --- a/src/popup/views/addWallet.js +++ b/src/popup/views/addWallet.js @@ -184,7 +184,12 @@ async function importMnemonic(ctx) { encryptedSecret: encrypted, nextIndex: 1, addresses: [ - { address: firstAddress, balance: "0.0000", tokenBalances: [] }, + { + address: firstAddress, + index: 0, + balance: "0.0000", + tokenBalances: [], + }, ], }; state.wallets.push(wallet); @@ -205,6 +210,7 @@ async function importMnemonic(ctx) { if (scan.addresses.length > 1) { wallet.addresses = scan.addresses.map((a) => ({ address: a.address, + index: a.index, balance: "0.0000", tokenBalances: [], })); @@ -299,7 +305,12 @@ async function importXprvKey(ctx) { encryptedSecret: encrypted, nextIndex: 1, addresses: [ - { address: firstAddress, balance: "0.0000", tokenBalances: [] }, + { + address: firstAddress, + index: 0, + balance: "0.0000", + tokenBalances: [], + }, ], }; state.wallets.push(wallet); @@ -320,6 +331,7 @@ async function importXprvKey(ctx) { if (scan.addresses.length > 1) { wallet.addresses = scan.addresses.map((a) => ({ address: a.address, + index: a.index, balance: "0.0000", tokenBalances: [], })); diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 823e465..c5b8c5a 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -867,15 +867,15 @@ function gateOnWalletDefect(errorId, buttonId, address) { return true; } -// Locate the wallet and the address index owning an address. Returns null when -// no wallet holds it. Approvals look up the address they were raised for, not -// whichever address is active now: the approval named one account, and signing -// with another is what verification refuses. +// Locate the wallet and the stored address record owning an address. Returns +// null when no wallet holds it. Approvals look up the address they were raised +// for, not whichever address is active now: the approval named one account, +// and signing with another is what verification refuses. function findWalletFor(address) { for (const wallet of state.wallets) { for (let i = 0; i < wallet.addresses.length; i++) { if (wallet.addresses[i].address === address) { - return { wallet, addrIndex: i }; + return { wallet, addr: wallet.addresses[i] }; } } } @@ -1002,7 +1002,7 @@ function init(_ctx) { try { const signer = getSignerForAddress( active.wallet, - active.addrIndex, + active.addr, decryptedSecret, ); // Sign the approved transaction exactly as it was displayed. The @@ -1130,7 +1130,7 @@ function init(_ctx) { try { const signer = getSignerForAddress( active.wallet, - active.addrIndex, + active.addr, decryptedSecret, ); const sp = pendingSignParams; diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index 30d1c4d..e6831c4 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -547,6 +547,19 @@ function init(_ctx) { return; } + // Sign as the From address this screen shows. The selection is saved + // apart from this screen, so the popup can reopen onto it with another + // address selected. + const addr = wallet.addresses.find((a) => a.address === pendingTx.from); + if (!addr) { + showError( + "confirm-tx-password-error", + "The selected wallet does not hold the From address above, " + + "so nothing was sent. Please go back and send again.", + ); + return; + } + const password = $("confirm-tx-password").value; if (!password) { showError( @@ -581,11 +594,7 @@ function init(_ctx) { let tx; try { - const signer = getSignerForAddress( - wallet, - state.selectedAddress, - decryptedSecret, - ); + const signer = getSignerForAddress(wallet, addr, decryptedSecret); const provider = getProvider(state.rpcUrl, state.networkId); const connectedSigner = signer.connect(provider); diff --git a/src/popup/views/exportPrivkey.js b/src/popup/views/exportPrivkey.js index 3860387..fb40635 100644 --- a/src/popup/views/exportPrivkey.js +++ b/src/popup/views/exportPrivkey.js @@ -135,7 +135,18 @@ async function reveal() { // decrypt ran, the wipe has already happened, so the key is not // even derived, let alone written. if (!isCurrentReveal(generation)) return; - const signer = getSignerForAddress(wallet, addressIndex, secret); + let signer; + try { + signer = getSignerForAddress( + wallet, + wallet.addresses[addressIndex], + secret, + ); + } catch (e) { + // The password was right; it is the key that was refused. + showError("export-privkey-password-error", e.message); + return; + } $("export-privkey-password").value = ""; $("export-privkey-password-section").classList.add("hidden"); $("export-privkey-value").textContent = signer.privateKey; diff --git a/src/popup/views/home.js b/src/popup/views/home.js index b77d106..dc542bc 100644 --- a/src/popup/views/home.js +++ b/src/popup/views/home.js @@ -354,6 +354,7 @@ function render(ctx) { ); wallet.addresses.push({ address: newAddr, + index: wallet.nextIndex, balance: "0.0000", tokenBalances: [], }); diff --git a/src/shared/wallet.js b/src/shared/wallet.js index 16d9db0..674e10e 100644 --- a/src/shared/wallet.js +++ b/src/shared/wallet.js @@ -110,16 +110,33 @@ function addressFromPrivateKey(key) { return w.address; } -function getSignerForAddress(walletData, addrIndex, decryptedSecret) { +// The signer for `addr`, one of the wallet's stored address records. An HD or +// xprv address is derived at its own `index`, never at its position in the +// wallet's list: the two differ once an import scan passes over an unused +// index, or once an address that is not the last is removed. An HD or xprv +// record without a valid `index` is refused before anything is derived. +// Whatever the wallet type, the key is refused unless it is the key of +// `addr.address`. +function getSignerForAddress(walletData, addr, decryptedSecret) { + if ( + (walletData.type === "hd" || walletData.type === "xprv") && + !(Number.isInteger(addr.index) && addr.index >= 0) + ) { + throw new Error( + "No derivation index is stored for " + + addr.address + + ", so its key cannot be derived.", + ); + } + let signer; if (walletData.type === "hd") { const node = HDNodeWallet.fromPhrase( decryptedSecret, "", BIP44_ETH_PATH, ); - return node.deriveChild(addrIndex); - } - if (walletData.type === "xprv") { + signer = node.deriveChild(addr.index); + } else if (walletData.type === "xprv") { // 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 @@ -137,9 +154,18 @@ function getSignerForAddress(walletData, addrIndex, decryptedSecret) { "is not a master key, so it cannot sign.", ); } - return node.derivePath(BIP44_ETH_PATH).deriveChild(addrIndex); + signer = node.derivePath(BIP44_ETH_PATH).deriveChild(addr.index); + } else { + signer = new Wallet(decryptedSecret); } - return new Wallet(decryptedSecret); + if (signer.address.toLowerCase() !== addr.address.toLowerCase()) { + throw new Error( + "The key this wallet holds for " + + addr.address + + " belongs to a different address, so it was not used.", + ); + } + return signer; } function isValidMnemonic(mnemonic) { diff --git a/tests/addressKey.test.js b/tests/addressKey.test.js new file mode 100644 index 0000000..f0a92b5 --- /dev/null +++ b/tests/addressKey.test.js @@ -0,0 +1,439 @@ +// Sending, a site's signature request 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, signature approval screen 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"); +} + +describe.each([ + [ + "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, + ); + }); +}); + +// 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()); + }); +}); diff --git a/tests/approvalNonce.test.js b/tests/approvalNonce.test.js index f23221d..7172a7c 100644 --- a/tests/approvalNonce.test.js +++ b/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({}); diff --git a/tests/approvalVerify.test.js b/tests/approvalVerify.test.js index 6b2a13c..17206b7 100644 --- a/tests/approvalVerify.test.js +++ b/tests/approvalVerify.test.js @@ -1455,7 +1455,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 }); } @@ -1550,7 +1554,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/); diff --git a/tests/wallet.test.js b/tests/wallet.test.js index b9035df..8940666 100644 --- a/tests/wallet.test.js +++ b/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"; diff --git a/tests/walletDefects.test.js b/tests/walletDefects.test.js index f7dec0e..d5e9ea5 100644 --- a/tests/walletDefects.test.js +++ b/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), );