diff --git a/README.md b/README.md index cb0f278..2ed0dc3 100644 --- a/README.md +++ b/README.md @@ -436,7 +436,11 @@ The core hierarchy is **Wallets → Addresses**: multi-address behavior as an HD wallet, including the "+" button and the address scan on import, but imported from an extended private key rather than a recovery phrase. It therefore has no recovery phrase to display or - back up. + back up. Only a master key may be imported; an xprv wallet already in + storage that was imported from a non-master key is detected from the depth + 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. - The user can have multiple wallets, each with multiple addresses (HD) or a single address (key). diff --git a/TODO.md b/TODO.md index d9135ff..eb6dd07 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,11 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: An xprv wallet already in storage that was imported from a + non-master key is detected from the depth of its stored `xpub`, explained in + the wallet list, and blocked from signing, sending and private-key export + instead of throwing on the send screen + ([#234](https://git.eeqj.de/sneak/AutistMask/issues/234)). - 2026-08-12: An unreported `holders_count` is now parsed as `null` rather than `0`, so the low-holder rule declines to judge an unknown count instead of hiding a legitimate token as spam, in both the transaction history and the diff --git a/src/popup/views/addressDetail.js b/src/popup/views/addressDetail.js index aaf5864..d2b85ae 100644 --- a/src/popup/views/addressDetail.js +++ b/src/popup/views/addressDetail.js @@ -29,6 +29,16 @@ const { log } = require("../../shared/log"); const makeBlockie = require("ethereum-blockies-base64"); const { decryptWithPassword } = require("../../shared/vault"); const { getSignerForAddress } = require("../../shared/wallet"); +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. +function selectedWalletDefect() { + if (state.selectedWallet === null) return null; + return walletDefect(state.wallets[state.selectedWallet]); +} let ctx; @@ -254,6 +264,11 @@ function init(_ctx) { }); $("btn-send").addEventListener("click", () => { + const defect = selectedWalletDefect(); + if (defect) { + showFlash(defect.shortMessage); + return; + } const addr = state.wallets[state.selectedWallet].addresses[ state.selectedAddress @@ -298,6 +313,14 @@ 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. + const defect = selectedWalletDefect(); + if (defect) { + showFlash(defect.shortMessage); + return; + } pushCurrentView(); const wallet = state.wallets[state.selectedWallet]; const addr = wallet.addresses[state.selectedAddress]; diff --git a/src/popup/views/addressToken.js b/src/popup/views/addressToken.js index 2193632..7dba24c 100644 --- a/src/popup/views/addressToken.js +++ b/src/popup/views/addressToken.js @@ -35,6 +35,7 @@ const { } = require("./send"); const { log } = require("../../shared/log"); const makeBlockie = require("ethereum-blockies-base64"); +const { walletDefect } = require("../../shared/walletDefects"); let ctx; @@ -338,6 +339,11 @@ function init(_ctx) { }); $("btn-address-token-send").addEventListener("click", () => { + const defect = walletDefect(state.wallets[state.selectedWallet]); + if (defect) { + showFlash(defect.shortMessage); + return; + } const addr = state.wallets[state.selectedWallet].addresses[ state.selectedAddress diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index c9cc740..25ba926 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -21,6 +21,7 @@ const { ERC20_ABI } = require("../../shared/constants"); const { TOKEN_BY_ADDRESS } = require("../../shared/tokenList"); const { decryptWithPassword } = require("../../shared/vault"); const { getSignerForAddress } = require("../../shared/wallet"); +const { walletDefect } = require("../../shared/walletDefects"); const { getProvider } = require("../../shared/balances"); const txStatus = require("./txStatus"); const uniswap = require("../../shared/uniswap"); @@ -280,6 +281,7 @@ function showTxApproval(details) { showView("approve-tx"); attachCopyHandlers("view-approve-tx"); + gateOnWalletDefect("approve-tx-error", "btn-approve-tx"); } function decodeHexMessage(hex) { @@ -379,6 +381,7 @@ function showSignApproval(details) { showView("approve-sign"); attachCopyHandlers("view-approve-sign"); + gateOnWalletDefect("approve-sign-error", "btn-approve-sign"); } function show(id) { @@ -431,6 +434,20 @@ function setSignButtonBusy(busy) { $("btn-approve-sign").classList.toggle("text-muted", busy); } +// Say so on the approval screen itself, and disable the approve button, when +// the active address 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. Returns true when it gated. +function gateOnWalletDefect(errorId, buttonId) { + const active = findActiveWallet(); + const defect = active ? walletDefect(active.wallet) : null; + if (!defect) return false; + showError(errorId, defect.shortMessage); + $(buttonId).disabled = true; + $(buttonId).classList.add("text-muted"); + return true; +} + // Locate the wallet and the address index owning the currently active // address. Returns null when no wallet holds it. function findActiveWallet() { @@ -492,6 +509,14 @@ function init(ctx) { return; } + const defect = walletDefect(active.wallet); + if (defect) { + password = null; + showError("approve-tx-error", defect.shortMessage); + setTxButtonBusy(false); + return; + } + // Decrypt here, in the popup. The password must never cross the // extension messaging boundary; only the signed transaction does. let decryptedSecret; @@ -583,6 +608,14 @@ function init(ctx) { return; } + const defect = walletDefect(active.wallet); + if (defect) { + password = null; + showError("approve-sign-error", defect.shortMessage); + setSignButtonBusy(false); + return; + } + // Decrypt here, in the popup. The password must never cross the // extension messaging boundary; only the signature does. let decryptedSecret; diff --git a/src/popup/views/home.js b/src/popup/views/home.js index 52b631f..46d8b24 100644 --- a/src/popup/views/home.js +++ b/src/popup/views/home.js @@ -21,6 +21,10 @@ const { resetSendValidation, } = require("./send"); const { deriveAddressFromXpub } = require("../../shared/wallet"); +const { + walletDefect, + walletDefectHtml, +} = require("../../shared/walletDefects"); const { formatUsd, getPrice, @@ -214,25 +218,23 @@ async function loadHomeTxs(ctx) { } } -function render(ctx) { - const container = $("wallet-list"); - if (state.wallets.length === 0) { - container.innerHTML = - '

No wallets yet. Add one to get started.

'; - renderTotalValue(); - renderActiveAddress(); - return; - } - +// The wallet list markup. Pure: it reads state and returns a string, so the +// list can be asserted on without a DOM. +function walletListHtml() { let html = ""; state.wallets.forEach((wallet, wi) => { + const defect = walletDefect(wallet); html += `
`; html += `
`; html += `${wallet.name}`; - if (wallet.type === "hd" || wallet.type === "xprv") { + // No "+" on a defective wallet: deriving another address from that + // xpub would only add one more address the key does not produce + // under the standard path. + if (!defect && (wallet.type === "hd" || wallet.type === "xprv")) { html += ``; } html += `
`; + html += walletDefectHtml(wallet); wallet.addresses.forEach((addr, ai) => { html += `
`; @@ -260,7 +262,20 @@ function render(ctx) { html += `
`; }); - container.innerHTML = html; + return html; +} + +function render(ctx) { + const container = $("wallet-list"); + if (state.wallets.length === 0) { + container.innerHTML = + '

No wallets yet. Add one to get started.

'; + renderTotalValue(); + renderActiveAddress(); + return; + } + + container.innerHTML = walletListHtml(); container.querySelectorAll(".address-row").forEach((row) => { row.addEventListener("click", async () => { @@ -348,6 +363,13 @@ function render(ctx) { loadHomeTxs(ctx); } +// The defect of the wallet the selected address belongs to, or null. Call +// after selectActiveAddress(). +function selectedWalletDefect() { + if (state.selectedWallet === null) return null; + return walletDefect(state.wallets[state.selectedWallet]); +} + function selectActiveAddress() { for (let wi = 0; wi < state.wallets.length; wi++) { for (let ai = 0; ai < state.wallets[wi].addresses.length; ai++) { @@ -371,6 +393,13 @@ function init(ctx) { showFlash("No active address selected."); return; } + // Before the balance check and before any password is asked for: this + // wallet cannot sign at all, so the send screen is a dead end. + const defect = selectedWalletDefect(); + if (defect) { + showFlash(defect.shortMessage); + return; + } const addr = currentAddress(); if (!addr.balance || parseFloat(addr.balance) === 0) { showFlash("Cannot send \u2014 zero balance."); @@ -396,4 +425,4 @@ function init(ctx) { }); } -module.exports = { init, render }; +module.exports = { init, render, walletListHtml }; diff --git a/src/shared/wallet.js b/src/shared/wallet.js index 0d98a96..16d9db0 100644 --- a/src/shared/wallet.js +++ b/src/shared/wallet.js @@ -120,9 +120,24 @@ function getSignerForAddress(walletData, addrIndex, decryptedSecret) { return node.deriveChild(addrIndex); } if (walletData.type === "xprv") { - const node = - masterXprvOrThrow(decryptedSecret).derivePath(BIP44_ETH_PATH); - return node.deriveChild(addrIndex); + // 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 + // catches it at list-render time; this is the backstop behind that. + const node = parseExtendedKey(decryptedSecret); + if (!node || !node.privateKey) { + throw new Error( + "This wallet's stored key is not a valid extended private " + + "key, so it cannot sign.", + ); + } + if (node.depth !== MASTER_DEPTH) { + throw new Error( + "This wallet was imported from an extended private key that " + + "is not a master key, so it cannot sign.", + ); + } + return node.derivePath(BIP44_ETH_PATH).deriveChild(addrIndex); } return new Wallet(decryptedSecret); } @@ -142,6 +157,7 @@ function walletHasRecoveryPhrase(walletData) { module.exports = { generateMnemonic, + parseExtendedKey, deriveAddressFromXpub, hdWalletFromMnemonic, hdWalletFromXprv, diff --git a/src/shared/walletDefects.js b/src/shared/walletDefects.js new file mode 100644 index 0000000..afa069b --- /dev/null +++ b/src/shared/walletDefects.js @@ -0,0 +1,86 @@ +// Wallets already in stored state whose key cannot be used, and the copy that +// explains them. +// +// Refusing a non-master extended private key at import time does nothing for a +// wallet imported before that refusal existed. Such a wallet is detected here, +// at wallet-list render time, so the user meets the explanation on the list +// screen rather than an exception on the send screen. Nothing here modifies or +// removes a wallet: the record is the user's data. + +const { parseExtendedKey } = require("./wallet"); + +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. +const BIP44_ETH_XPUB_DEPTH = 4; + +const DEFECTS = { + [NON_MASTER_XPRV]: { + id: NON_MASTER_XPRV, + heading: "This wallet's addresses were derived incorrectly.", + paragraphs: [ + "This wallet was imported from an extended private key that is " + + "not a master key. An earlier version applied the Ethereum " + + "derivation path beneath that key instead of from a master " + + "key, so the addresses listed here are not the ones that key " + + "produces under the standard path.", + "Signing and sending are disabled for this wallet. The addresses " + + "do descend from the extended private key you imported, so " + + "anything they hold is still reachable by software that " + + "repeats the same non-standard derivation. Check them in a " + + "block explorer before deciding what to do.", + "To see the addresses this key produces under the standard path, " + + "import the master extended private key, or the recovery " + + "phrase it came from, as a new wallet. Nothing here has been " + + "changed or removed, and this wallet stays until you delete " + + "it yourself.", + ], + // One sentence for the places that have room for one: the flash on a + // blocked Send, the inline error on the approval screens. + shortMessage: + "This wallet cannot sign, because it was imported from an " + + "extended private key that is not a master key. The wallet list " + + "explains what happened.", + }, +}; + +// The defect record for a wallet, or null if there is nothing wrong with it +// that this module can see. Read-only. +// +// A wallet whose xpub will not parse gets null rather than a defect: there is +// no basis in that case to tell the user their key was not a master key, and a +// wrong explanation is worse than none. +function walletDefect(walletData) { + if (!walletData || walletData.type !== "xprv") return null; + const node = parseExtendedKey(walletData.xpub); + if (!node) return null; + if (node.depth === BIP44_ETH_XPUB_DEPTH) return null; + return DEFECTS[NON_MASTER_XPRV]; +} + +// The notice block for the wallet list, or "" for a wallet with no defect. +// The copy is fixed text from this module, so it needs no escaping. +function walletDefectHtml(walletData) { + const defect = walletDefect(walletData); + if (!defect) return ""; + let html = + '
'; + html += `
${defect.heading}
`; + for (const p of defect.paragraphs) { + html += `

${p}

`; + } + html += "
"; + return html; +} + +module.exports = { + NON_MASTER_XPRV, + walletDefect, + walletDefectHtml, +}; diff --git a/tests/walletDefects.test.js b/tests/walletDefects.test.js new file mode 100644 index 0000000..4ab804f --- /dev/null +++ b/tests/walletDefects.test.js @@ -0,0 +1,290 @@ +// Tests for the stored-state half of the non-master extended key problem. +// +// Refusing a non-master xprv at import time does nothing for a wallet that is +// 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. + +const { HDNodeWallet, Mnemonic } = require("ethers"); + +const wallet = require("../src/shared/wallet"); +const { + walletDefect, + walletDefectHtml, + NON_MASTER_XPRV, +} = require("../src/shared/walletDefects"); + +// BIP-39 test vector phrase, published; never used for real funds. +const VECTOR_PHRASE = + "test test test test test test test test test test test junk"; + +function seedNode(phrase) { + return HDNodeWallet.fromSeed(Mnemonic.fromPhrase(phrase, "").computeSeed()); +} + +// The master (depth-0) key, which is what the import flow accepts today. +function masterXprv(phrase) { + return seedNode(phrase).extendedKey; +} + +// The account-level (depth-3) key m/44'/60'/0'. A normal thing for a user to +// hold, and what the import flow used to accept. +function accountXprv(phrase) { + return seedNode(phrase).derivePath("m/44'/60'/0'").extendedKey; +} + +// The wallet record the CURRENT import path writes for a master key: the +// neutered m/44'/60'/0'/0 node, four levels below a depth-0 key. +function healthyXprvWallet(name = "Master") { + const { xpub, firstAddress } = wallet.hdWalletFromXprv( + masterXprv(VECTOR_PHRASE), + ); + return { + name, + type: "xprv", + xpub, + nextIndex: 1, + encryptedSecret: "irrelevant-to-these-tests", + addresses: [{ address: firstAddress, balance: "0.0000" }], + }; +} + +// The wallet record the PRE-#210 import path wrote for an account-level key: +// the same four levels, but derived as a relative path *beneath* the key, so +// the stored xpub sits at depth 3 + 4 = 7. Built here the way the old code +// built it rather than by calling the module under test, which now refuses. +function brokenXprvWallet(name = "Imported xprv") { + const node = HDNodeWallet.fromExtendedKey( + accountXprv(VECTOR_PHRASE), + ).derivePath("44'/60'/0'/0"); + return { + name, + type: "xprv", + xpub: node.neuter().extendedKey, + nextIndex: 1, + encryptedSecret: "irrelevant-to-these-tests", + addresses: [ + { address: node.deriveChild(0).address, balance: "0.0000" }, + ], + }; +} + +describe("the fixtures are what the two import paths actually produced", () => { + test("a master import stores a depth-4 xpub", () => { + expect( + HDNodeWallet.fromExtendedKey(healthyXprvWallet().xpub).depth, + ).toBe(4); + }); + + test("the pre-fix account-level import stored a depth-7 xpub", () => { + expect( + HDNodeWallet.fromExtendedKey(brokenXprvWallet().xpub).depth, + ).toBe(7); + }); +}); + +describe("walletDefect", () => { + test("names the defect on a stored non-master xprv wallet", () => { + const defect = walletDefect(brokenXprvWallet()); + + expect(defect).not.toBeNull(); + expect(defect.id).toBe(NON_MASTER_XPRV); + }); + + test("a depth-0 xprv wallet has no defect", () => { + expect(walletDefect(healthyXprvWallet())).toBeNull(); + }); + + test("hd and key wallets are never assessed", () => { + expect( + walletDefect({ type: "hd", xpub: brokenXprvWallet().xpub }), + ).toBe(null); + expect(walletDefect({ type: "key" })).toBeNull(); + }); + + test("an xprv wallet whose xpub cannot be parsed makes no claim", () => { + // No basis to say the key was non-master, so nothing is asserted + // about it rather than guessing. + expect(walletDefect({ type: "xprv", xpub: "not-a-key" })).toBeNull(); + expect(walletDefect({ type: "xprv" })).toBeNull(); + }); + + test("nothing about the wallet record is modified by the check", () => { + const w = brokenXprvWallet(); + const before = JSON.stringify(w); + + walletDefect(w); + + expect(JSON.stringify(w)).toBe(before); + }); +}); + +describe("the explanatory copy", () => { + const defect = walletDefect(brokenXprvWallet()); + + test("every sentence of it is a full sentence", () => { + for (const text of [defect.heading, ...defect.paragraphs]) { + expect(text).toMatch(/^[A-Z]/); + expect(text.trimEnd()).toMatch(/\.$/); + } + }); + + test("it says what was derived wrongly and that these are not the standard addresses", () => { + const body = defect.paragraphs.join(" "); + + expect(body).toContain("not a master key"); + expect(body).toMatch(/standard path/); + }); + + test("it does not claim the funds are safe and does not claim a loss", () => { + const all = [defect.heading, ...defect.paragraphs].join(" "); + + expect(all).not.toMatch(/\bsafe\b/i); + expect(all).not.toMatch(/\blost\b|\bstolen\b|\bgone\b/i); + }); + + test("it says the wallet is not deleted and what the user can do", () => { + const body = defect.paragraphs.join(" "); + + expect(body).toMatch(/until you delete it yourself/); + expect(body).toMatch(/recovery phrase/); + }); + + test("it uses the project's vocabulary", () => { + const all = [ + defect.heading, + ...defect.paragraphs, + defect.shortMessage, + ].join(" "); + + expect(all).not.toMatch(/seed phrase|mnemonic|passphrase/i); + expect(all).not.toMatch(/\baccounts?\b/i); + }); +}); + +describe("walletDefectHtml", () => { + test("renders the heading and every paragraph for a defective wallet", () => { + const defect = walletDefect(brokenXprvWallet()); + const html = walletDefectHtml(brokenXprvWallet()); + + expect(html).toContain(defect.heading); + for (const p of defect.paragraphs) { + expect(html).toContain(p); + } + }); + + test("renders nothing at all for a healthy wallet", () => { + expect(walletDefectHtml(healthyXprvWallet())).toBe(""); + }); +}); + +describe("the wallet list", () => { + let home; + let state; + + beforeAll(() => { + global.chrome = { + storage: { local: { get: async () => ({}), set: async () => {} } }, + runtime: { sendMessage: () => {} }, + }; + home = require("../src/popup/views/home"); + state = require("../src/shared/state").state; + }); + + afterEach(() => { + state.wallets = []; + state.activeAddress = null; + }); + + test("a stored depth-3 xprv wallet renders the explanation", () => { + state.wallets = [brokenXprvWallet("Imported xprv")]; + + const html = home.walletListHtml(); + + expect(html).toContain(walletDefect(state.wallets[0]).heading); + expect(html).toContain("Imported xprv"); + }); + + test("it does not offer to derive further addresses from that wallet", () => { + state.wallets = [brokenXprvWallet()]; + + expect(home.walletListHtml()).not.toContain("btn-add-address"); + }); + + test("a normal depth-0 xprv wallet renders exactly as it did before", () => { + state.wallets = [healthyXprvWallet("Master")]; + + const html = home.walletListHtml(); + + expect(html).not.toContain(walletDefect(brokenXprvWallet()).heading); + expect(html).toContain("btn-add-address"); + expect(html).toContain(state.wallets[0].addresses[0].address); + }); + + test("the defective wallet's notice does not bleed onto a healthy one", () => { + state.wallets = [brokenXprvWallet("Broken"), healthyXprvWallet("Fine")]; + + const html = home.walletListHtml(); + const healthyPart = html.slice(html.indexOf("Fine")); + + expect(html).toContain(walletDefect(state.wallets[0]).heading); + expect(healthyPart).not.toContain( + walletDefect(state.wallets[0]).heading, + ); + expect(healthyPart).toContain("btn-add-address"); + }); +}); + +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 + // key; deriving from it is what the list render already does. + const w = brokenXprvWallet(); + + expect(() => wallet.deriveAddressFromXpub(w.xpub, 0)).not.toThrow(); + expect(wallet.deriveAddressFromXpub(w.xpub, 0)).toBe( + w.addresses[0].address, + ); + }); + + test("the wallet list renders without throwing", () => { + const { state } = require("../src/shared/state"); + const home = require("../src/popup/views/home"); + state.wallets = [brokenXprvWallet()]; + + expect(() => home.walletListHtml()).not.toThrow(); + + state.wallets = []; + }); + + test("signing refuses with the named defect rather than a bare failure", () => { + // getSignerForAddress is the backstop behind the UI gate. It must + // still refuse, and it must say why in a sentence the user can read. + let thrown = null; + try { + wallet.getSignerForAddress( + { type: "xprv" }, + 0, + accountXprv(VECTOR_PHRASE), + ); + } catch (e) { + thrown = e; + } + + expect(thrown).not.toBeNull(); + expect(thrown.message).toMatch(/master key/); + expect(thrown.message.trimEnd()).toMatch(/\.$/); + }); + + test("a healthy xprv wallet signs as it always did", () => { + const signer = wallet.getSignerForAddress( + { type: "xprv" }, + 0, + masterXprv(VECTOR_PHRASE), + ); + + expect(signer.address).toBe(healthyXprvWallet().addresses[0].address); + }); +});