From d9a516e4892816bd93e70271f5a750430236f187 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 5 Oct 2026 09:52:02 +0000 Subject: [PATCH] test: cover every control that refuses a defective wallet (closes #254) Each control that leads to a signature or to the private key now has a test that it refuses a defective wallet before decrypting anything: Send on the main, address and token screens, Export Private Key, and both approval screens, as drawn and as clicked. Send on the confirmation screen had no such check. The Send buttons stand in front of it, but the popup reopens onto it from a saved view, so it now refuses the same way. The comments that said the wallet's key cannot be derived now say that getSignerForAddress refuses it, and the walletDefects module comment names both earlier import paths. Model: opus-5-5 --- TODO.md | 10 ++ src/popup/views/addressDetail.js | 11 +- src/popup/views/approval.js | 7 +- src/popup/views/confirmTx.js | 11 +- src/shared/walletDefects.js | 16 +- tests/walletDefects.test.js | 296 ++++++++++++++++++++++++++++++- 6 files changed, 335 insertions(+), 16 deletions(-) diff --git a/TODO.md b/TODO.md index 9382c64..32d1c76 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,16 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-05: Each control that leads to a signature or to the private key has a + test that it refuses a defective wallet before asking for a password + ([#254](https://git.eeqj.de/sneak/AutistMask/issues/254)): Send on the main, + address and token screens, Export Private Key, and both approval screens, as + drawn and as clicked. Send on the confirmation screen refuses it too now, + because the popup reopens onto that screen from a saved view. The comments + that said the wallet's key cannot be derived now say that + `getSignerForAddress` refuses it, and the module comment in + `src/shared/walletDefects.js` names both earlier import paths. + - 2026-10-05: The Chrome end-to-end suite drives the private key export screen as it drives the recovery phrase screen ([#253](https://git.eeqj.de/sneak/AutistMask/issues/253)): the correct diff --git a/src/popup/views/addressDetail.js b/src/popup/views/addressDetail.js index feeadde..bb4d847 100644 --- a/src/popup/views/addressDetail.js +++ b/src/popup/views/addressDetail.js @@ -33,8 +33,8 @@ 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. +// so a wallet whose key getSignerForAddress refuses 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]); @@ -259,9 +259,10 @@ 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. + // This address's private key can be derived from the stored key, + // but export goes through getSignerForAddress, which refuses a key + // that is not a master key. Without this the export screen would + // take a password and then report that refusal as a wrong password. const defect = selectedWalletDefect(); if (defect) { showFlash(defect.shortMessage); diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 3f8dddf..5f33d14 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -822,9 +822,10 @@ function setSignButtonBusy(busy) { } // Say so on the approval screen itself, and disable the approve button, when -// the address the approval was raised for 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. +// the address the approval was raised for belongs to a wallet whose key +// getSignerForAddress refuses. 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, address) { const owner = findWalletFor(address); diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index 942e951..30d1c4d 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -21,6 +21,7 @@ const { } = require("./helpers"); const { state, currentNetwork } = require("../../shared/state"); const { getSignerForAddress } = require("../../shared/wallet"); +const { walletDefect } = require("../../shared/walletDefects"); const { decryptWithPassword } = require("../../shared/vault"); const { formatUsd, getPrice } = require("../../shared/prices"); const { getProvider } = require("../../shared/balances"); @@ -537,6 +538,15 @@ function init(_ctx) { onViewLeave("confirm-tx", clearPassword); $("btn-confirm-send").addEventListener("click", async () => { + const wallet = state.wallets[state.selectedWallet]; + // Every Send button refuses a defective wallet before this screen, + // but the popup also reopens onto it from a saved view. + const defect = walletDefect(wallet); + if (defect) { + showError("confirm-tx-password-error", defect.shortMessage); + return; + } + const password = $("confirm-tx-password").value; if (!password) { showError( @@ -546,7 +556,6 @@ function init(_ctx) { return; } - const wallet = state.wallets[state.selectedWallet]; let decryptedSecret; hideError("confirm-tx-password-error"); diff --git a/src/shared/walletDefects.js b/src/shared/walletDefects.js index 8ed89e2..eb4ed27 100644 --- a/src/shared/walletDefects.js +++ b/src/shared/walletDefects.js @@ -13,11 +13,17 @@ 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. +// m/44'/60'/0'/0 from a depth-0 key, and the path before #210 (57959b7) +// 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. +// +// The first import path (7a7f9c5) does not fit: it stored the imported key's +// own xpub with no derivation, so a wallet it wrote is judged wrongly here (a +// master import as defective, a depth-4 import as sound). 57959b7 replaced it +// in the same push, and no tag contains it. const BIP44_ETH_XPUB_DEPTH = 4; const DEFECTS = { diff --git a/tests/walletDefects.test.js b/tests/walletDefects.test.js index 4ab804f..a9fd35d 100644 --- a/tests/walletDefects.test.js +++ b/tests/walletDefects.test.js @@ -4,8 +4,16 @@ // already in storage: the import that created it ran before the refusal // existed. Such a wallet used to sign for the wrong tree and now throws on the // send screen instead. These tests pin down that it is named and explained in -// the wallet list, that nothing on the way there throws, and that a wallet -// imported from a real master key is untouched by any of it. +// the wallet list, that every control leading to a signature or to the private +// key refuses it before asking for a password, that nothing on the way there +// throws, and that a wallet imported from a real master key is untouched by +// any of it. + +// Mocked so that no password has to be hashed: the controls below are checked +// for whether they decrypt at all. +jest.mock("../src/shared/vault", () => ({ + decryptWithPassword: jest.fn(), +})); const { HDNodeWallet, Mnemonic } = require("ethers"); @@ -237,6 +245,290 @@ describe("the wallet list", () => { }); }); +// A minimal DOM for driving the popup views: any element exists on first +// lookup, and click() runs the listeners a view attached to it. +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(id) { + return Promise.all((node(id).listeners.click || []).map((fn) => fn())); +} + +// getSignerForAddress refuses this wallet's key, but only once the password +// has been typed and spent, and the screens report that refusal as a wrong +// password or a failed send. So every control that leads to it refuses first. +describe("every way to a signature or the private key refuses a defective wallet first", () => { + const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + + let state; + let decryptWithPassword; + let home; + let addressDetail; + let addressToken; + let approval; + let confirmTx; + + let address; + let shortMessage; + // 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; + + beforeAll(() => { + state = require("../src/shared/state").state; + decryptWithPassword = + require("../src/shared/vault").decryptWithPassword; + home = require("../src/popup/views/home"); + addressDetail = require("../src/popup/views/addressDetail"); + addressToken = require("../src/popup/views/addressToken"); + approval = require("../src/popup/views/approval"); + confirmTx = require("../src/popup/views/confirmTx"); + }); + + beforeEach(() => { + const broken = brokenXprvWallet(); + // A balance, so that no Send button's zero-balance refusal can stand + // in for the defect check. + broken.addresses[0].balance = "1.0000"; + broken.addresses[0].tokenBalances = []; + address = broken.addresses[0].address; + shortMessage = walletDefect(broken).shortMessage; + + approvalDetails = null; + sent = []; + 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, + ); + }, + }, + }; + // What the wallet's stored secret decrypts to: the account-level key + // it was imported from. + decryptWithPassword.mockReset(); + decryptWithPassword.mockResolvedValue(accountXprv(VECTOR_PHRASE)); + + state.wallets = [broken]; + state.activeAddress = address; + state.selectedWallet = 0; + state.selectedAddress = 0; + state.selectedToken = "ETH"; + state.viewStack = []; + }); + + afterEach(() => { + state.wallets = []; + state.activeAddress = null; + state.selectedWallet = null; + state.selectedAddress = null; + state.selectedToken = null; + }); + + test("Send on the main screen", async () => { + state.currentView = "main"; + home.init({}); + + await click("btn-main-send"); + + expect(node("flash-msg").textContent).toBe(shortMessage); + expect(state.currentView).toBe("main"); + }); + + test("Send on the address screen", async () => { + state.currentView = "address"; + addressDetail.init({}); + + await click("btn-send"); + + expect(node("flash-msg").textContent).toBe(shortMessage); + expect(state.currentView).toBe("address"); + }); + + test("Export Private Key on the address screen", async () => { + state.currentView = "address"; + addressDetail.init({}); + + await click("btn-export-privkey"); + + expect(node("flash-msg").textContent).toBe(shortMessage); + expect(state.currentView).toBe("address"); + }); + + test("Send on a token's screen", async () => { + state.currentView = "address-token"; + addressToken.init({}); + + await click("btn-address-token-send"); + + expect(node("flash-msg").textContent).toBe(shortMessage); + expect(state.currentView).toBe("address-token"); + }); + + // The popup reopens onto this screen from a saved view, so the Send + // buttons above are not the only way onto it. The screen is not drawn, + // because drawing it starts a fee estimate against the network; with a + // decrypt that fails, a handler without the check stops at the password + // instead of going on to a transaction that was never set up. + test("Send on the confirmation screen", async () => { + decryptWithPassword.mockRejectedValue(new Error("wrong password")); + state.currentView = "confirm-tx"; + confirmTx.init({}); + node("confirm-tx-password").value = "any password"; + + await click("btn-confirm-send"); + + expect(decryptWithPassword).not.toHaveBeenCalled(); + expect(node("confirm-tx-password-error").textContent).toBe( + shortMessage, + ); + }); + + async function openTxApproval() { + approvalDetails = { + type: "tx", + origin: "https://dapp.example", + isPhishingDomain: false, + approvedFrom: address, + approvedTx: { + from: address, + to: RECIPIENT, + value: "0x0", + data: "0x", + chainId: 1, + nonce: 0, + gasLimit: "21000", + maxFeePerGas: "1000000000", + }, + }; + approval.init({}); + await approval.show(1); + } + + async function openSignApproval() { + approvalDetails = { + type: "sign", + origin: "https://dapp.example", + isPhishingDomain: false, + approvedFrom: address, + // "Hello", as the hex a page sends. + signParams: { + method: "personal_sign", + message: "0x48656c6c6f", + from: address, + }, + }; + approval.init({}); + await approval.show(1); + } + + test("the transaction approval screen says so and disables Approve", async () => { + await openTxApproval(); + + expect(node("approve-tx-error").textContent).toBe(shortMessage); + expect(node("btn-approve-tx").disabled).toBe(true); + }); + + // The stub runs a disabled button's listener, which a browser would not: + // what is asked here is whether the handler refuses on its own. + test("Approve on the transaction approval screen does not decrypt", async () => { + await openTxApproval(); + node("approve-tx-password").value = "any password"; + + await click("btn-approve-tx"); + + expect(decryptWithPassword).not.toHaveBeenCalled(); + expect(sent.map((msg) => msg.type)).not.toContain( + "AUTISTMASK_TX_RESPONSE", + ); + expect(node("approve-tx-error").textContent).toBe(shortMessage); + }); + + test("the signature approval screen says so and disables Approve", async () => { + await openSignApproval(); + + expect(node("approve-sign-error").textContent).toBe(shortMessage); + expect(node("btn-approve-sign").disabled).toBe(true); + }); + + test("Approve on the signature approval screen does not decrypt", async () => { + await openSignApproval(); + node("approve-sign-password").value = "any password"; + + await click("btn-approve-sign"); + + expect(decryptWithPassword).not.toHaveBeenCalled(); + expect(sent.map((msg) => msg.type)).not.toContain( + "AUTISTMASK_SIGN_RESPONSE", + ); + expect(node("approve-sign-error").textContent).toBe(shortMessage); + }); +}); + describe("no path throws an unhandled error for a defective wallet", () => { test("address derivation from the stored xpub still works", () => { // The stored xpub is at a non-standard depth but is a valid extended -- 2.54.0