diff --git a/TODO.md b/TODO.md index 62d8652..3ddf8e2 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 StateRecovery screen is driven in a real browser under the shipped CSP, in both end-to-end suites ([#361](https://git.eeqj.de/sneak/AutistMask/issues/361)). A stored record 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