Compare commits
1 Commits
issue-275-
...
57e1fd0198
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
57e1fd0198 |
4
TODO.md
4
TODO.md
@@ -57,6 +57,10 @@ undefined identifiers, which is how
|
||||
from the wallet row in Settings, wiped on leaving the screen and excluded from
|
||||
the views the popup can reopen onto
|
||||
([#161](https://git.eeqj.de/sneak/AutistMask/issues/161)).
|
||||
- 2026-08-11: Extended-key import hardened — the base58 checksum is now enforced
|
||||
on every xprv and xpub, and a non-master key is refused with an explanation
|
||||
instead of being derived beneath
|
||||
([#210](https://git.eeqj.de/sneak/AutistMask/issues/210)).
|
||||
- 2026-08-11: Policy compliance sweep — conditional verbose test rerun, local
|
||||
Tailwind binary instead of `npx`, `--frozen-lockfile` on `make install`, and
|
||||
the Makefile-only targets documented in the README
|
||||
|
||||
@@ -136,7 +136,9 @@
|
||||
<div id="add-wallet-section-xprv" class="hidden">
|
||||
<p class="mb-2">
|
||||
Paste your extended private key (xprv) below. This will
|
||||
import the HD wallet and scan for used addresses.
|
||||
import the HD wallet and scan for used addresses. It
|
||||
must be the master key for the wallet; an account-level
|
||||
or child key is not supported.
|
||||
</p>
|
||||
<div class="mb-2">
|
||||
<input
|
||||
|
||||
@@ -6,6 +6,7 @@ const {
|
||||
addressFromPrivateKey,
|
||||
hdWalletFromXprv,
|
||||
isValidXprv,
|
||||
isMasterExtendedKey,
|
||||
} = require("../../shared/wallet");
|
||||
const { encryptWithPassword } = require("../../shared/vault");
|
||||
const { state, saveState } = require("../../shared/state");
|
||||
@@ -213,14 +214,25 @@ async function importXprvKey(ctx) {
|
||||
return;
|
||||
}
|
||||
if (!isValidXprv(xprv)) {
|
||||
showFlash("Invalid extended private key.");
|
||||
showFlash(
|
||||
"That extended private key is not valid. Please check it and try again.",
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (!isMasterExtendedKey(xprv)) {
|
||||
showFlash(
|
||||
"That is an account-level or child key, which cannot be imported. " +
|
||||
"Please paste the master extended private key for the wallet.",
|
||||
);
|
||||
return;
|
||||
}
|
||||
let result;
|
||||
try {
|
||||
result = hdWalletFromXprv(xprv);
|
||||
} catch (e) {
|
||||
showFlash("Invalid extended private key.");
|
||||
showFlash(
|
||||
"That extended private key is not valid. Please check it and try again.",
|
||||
);
|
||||
return;
|
||||
}
|
||||
const { xpub, firstAddress } = result;
|
||||
|
||||
@@ -16,8 +16,60 @@ function generateMnemonic() {
|
||||
return m.phrase;
|
||||
}
|
||||
|
||||
// Every extended key (xprv or xpub) entering the app goes through this.
|
||||
//
|
||||
// ethers' HDNodeWallet.fromExtendedKey does NOT verify the base58 checksum
|
||||
// when the decoded payload is the usual 82 bytes, which is exactly the case
|
||||
// the checksum exists to catch: a key with a one-character typo parses into a
|
||||
// *different* wallet instead of being rejected. Re-encoding the parsed node
|
||||
// reproduces a well-formed key byte for byte, checksum included, so comparing
|
||||
// the round trip against the input rejects any altered character. Measured by
|
||||
// the sweep in tests/wallet.test.js over every single-character substitution
|
||||
// of the BIP-32 vector 1 master key: 199 parse without the round-trip
|
||||
// comparison, 0 with it.
|
||||
//
|
||||
// Returns the parsed node, or null if the key is not a well-formed extended
|
||||
// key. Callers turn null into a user-facing error; none of them may fall back
|
||||
// to fromExtendedKey directly.
|
||||
function parseExtendedKey(key) {
|
||||
if (typeof key !== "string") return null;
|
||||
try {
|
||||
const node = HDNodeWallet.fromExtendedKey(key);
|
||||
return node.extendedKey === key ? node : null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
// A master key is at depth 0. Only from there is BIP44_ETH_PATH the absolute
|
||||
// path it names; deriving it under an account-level or child key yields
|
||||
// addresses that correspond to nothing the user holds.
|
||||
const MASTER_DEPTH = 0;
|
||||
|
||||
// Parse an extended private key that the BIP-44 Ethereum account path can be
|
||||
// derived from, or throw. Both callers derive BIP44_ETH_PATH from the result.
|
||||
function masterXprvOrThrow(key) {
|
||||
const node = parseExtendedKey(key);
|
||||
if (!node) {
|
||||
throw new Error("Not a valid extended private key (xprv).");
|
||||
}
|
||||
if (!node.privateKey) {
|
||||
throw new Error("Not an extended private key (xprv).");
|
||||
}
|
||||
if (node.depth !== MASTER_DEPTH) {
|
||||
throw new Error(
|
||||
"Not a master extended private key (xprv): an account-level or " +
|
||||
"child key cannot be imported.",
|
||||
);
|
||||
}
|
||||
return node;
|
||||
}
|
||||
|
||||
function deriveAddressFromXpub(xpub, index) {
|
||||
const node = HDNodeWallet.fromExtendedKey(xpub);
|
||||
const node = parseExtendedKey(xpub);
|
||||
if (!node) {
|
||||
throw new Error("Not a valid extended key.");
|
||||
}
|
||||
return node.deriveChild(index).address;
|
||||
}
|
||||
|
||||
@@ -29,23 +81,28 @@ function hdWalletFromMnemonic(mnemonic) {
|
||||
}
|
||||
|
||||
function hdWalletFromXprv(xprv) {
|
||||
const root = HDNodeWallet.fromExtendedKey(xprv);
|
||||
if (!root.privateKey) {
|
||||
throw new Error("Not an extended private key (xprv).");
|
||||
}
|
||||
const node = root.derivePath("44'/60'/0'/0");
|
||||
// BIP44_ETH_PATH is absolute ("m/..."), which ethers will only derive from
|
||||
// a depth-0 node. The relative form this used to derive would have been
|
||||
// applied *beneath* an account-level key instead of being refused.
|
||||
const node = masterXprvOrThrow(xprv).derivePath(BIP44_ETH_PATH);
|
||||
const xpub = node.neuter().extendedKey;
|
||||
const firstAddress = node.deriveChild(0).address;
|
||||
return { xpub, firstAddress };
|
||||
}
|
||||
|
||||
// Well-formed extended private key. Says nothing about depth: the import view
|
||||
// reports a non-master key separately, since "check it for a typo" is the
|
||||
// wrong advice for a key the user copied correctly.
|
||||
function isValidXprv(key) {
|
||||
try {
|
||||
const node = HDNodeWallet.fromExtendedKey(key);
|
||||
return !!node.privateKey;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
const node = parseExtendedKey(key);
|
||||
return !!(node && node.privateKey);
|
||||
}
|
||||
|
||||
// Whether an extended key is a master key, i.e. the one BIP44_ETH_PATH can be
|
||||
// derived from. False for anything parseExtendedKey rejects.
|
||||
function isMasterExtendedKey(key) {
|
||||
const node = parseExtendedKey(key);
|
||||
return !!node && node.depth === MASTER_DEPTH;
|
||||
}
|
||||
|
||||
function addressFromPrivateKey(key) {
|
||||
@@ -63,8 +120,8 @@ function getSignerForAddress(walletData, addrIndex, decryptedSecret) {
|
||||
return node.deriveChild(addrIndex);
|
||||
}
|
||||
if (walletData.type === "xprv") {
|
||||
const root = HDNodeWallet.fromExtendedKey(decryptedSecret);
|
||||
const node = root.derivePath("44'/60'/0'/0");
|
||||
const node =
|
||||
masterXprvOrThrow(decryptedSecret).derivePath(BIP44_ETH_PATH);
|
||||
return node.deriveChild(addrIndex);
|
||||
}
|
||||
return new Wallet(decryptedSecret);
|
||||
@@ -89,6 +146,7 @@ module.exports = {
|
||||
hdWalletFromMnemonic,
|
||||
hdWalletFromXprv,
|
||||
isValidXprv,
|
||||
isMasterExtendedKey,
|
||||
addressFromPrivateKey,
|
||||
getSignerForAddress,
|
||||
isValidMnemonic,
|
||||
|
||||
@@ -160,6 +160,31 @@ function masterXprv(phrase, passphrase = "") {
|
||||
).extendedKey;
|
||||
}
|
||||
|
||||
// The account-level (depth-3) extended private key m/44'/60'/0' for a phrase.
|
||||
// A normal thing for a user to hold, and not something the import flow can
|
||||
// derive the BIP-44 account path from.
|
||||
function accountXprv(phrase) {
|
||||
return HDNodeWallet.fromSeed(
|
||||
Mnemonic.fromPhrase(phrase, "").computeSeed(),
|
||||
).derivePath("m/44'/60'/0'").extendedKey;
|
||||
}
|
||||
|
||||
// Every single-character substitution of `key`, using base58 characters that
|
||||
// are not the original. Base58 has no visually ambiguous characters, so each
|
||||
// of these is a plausible typo rather than a contrived string.
|
||||
const TYPO_CHARS = ["a", "b", "2", "Z"];
|
||||
|
||||
function singleCharacterTypos(key) {
|
||||
const out = [];
|
||||
for (let i = 0; i < key.length; i++) {
|
||||
for (const c of TYPO_CHARS) {
|
||||
if (c === key[i]) continue;
|
||||
out.push(key.slice(0, i) + c + key.slice(i + 1));
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
describe("hdWalletFromMnemonic", () => {
|
||||
test("first address matches the published vector for m/44'/60'/0'/0/0", () => {
|
||||
expect(wallet.hdWalletFromMnemonic(VECTOR_PHRASE).firstAddress).toBe(
|
||||
@@ -299,19 +324,7 @@ describe("isValidXprv", () => {
|
||||
expect(wallet.isValidXprv(xpub)).toBe(false);
|
||||
});
|
||||
|
||||
// Skipped: this asserts the correct behaviour, which the code does not
|
||||
// currently have. isValidXprv gates the paste-your-extended-private-key
|
||||
// import in src/popup/views/addWallet.js:215, and it accepts a key with a
|
||||
// one-character typo: ethers' HDNodeWallet.fromExtendedKey skips base58
|
||||
// checksum verification whenever the decoded payload is the usual 82
|
||||
// bytes, which is the whole point of that checksum. Measured on this
|
||||
// vector: changing any one of the last 14 characters passes validation,
|
||||
// and for 9 of those 14 positions the import silently yields a *different*
|
||||
// wallet (e.g. 0x3F334f0a356d6B46B1d70B590E7437D77100d28D instead of
|
||||
// 0x022b971dFF0C43305e691DEd7a14367AF19D6407) with no error shown.
|
||||
// Tracked as https://git.eeqj.de/sneak/AutistMask/issues/210; out of scope
|
||||
// here, which is tests only. Unskip when it is fixed.
|
||||
test.skip("rejects an extended key with a one-character typo", () => {
|
||||
test("rejects an extended key with a one-character typo", () => {
|
||||
const index = BIP32_VECTOR_1_XPRV.length - 8;
|
||||
const typo =
|
||||
BIP32_VECTOR_1_XPRV.slice(0, index) +
|
||||
@@ -320,6 +333,125 @@ describe("isValidXprv", () => {
|
||||
|
||||
expect(wallet.isValidXprv(typo)).toBe(false);
|
||||
});
|
||||
|
||||
// The base58 checksum exists to make a mistyped key impossible to use, and
|
||||
// ethers does not enforce it: HDNodeWallet.fromExtendedKey skips checksum
|
||||
// verification whenever the decoded payload is the usual 82 bytes, which
|
||||
// is precisely the case it is there to catch. A typo anywhere in the key
|
||||
// must be refused, not silently turned into someone else's wallet.
|
||||
test("no single-character typo anywhere in the key is accepted", () => {
|
||||
const accepted = singleCharacterTypos(BIP32_VECTOR_1_XPRV).filter(
|
||||
(typo) => wallet.isValidXprv(typo),
|
||||
);
|
||||
|
||||
expect(accepted).toEqual([]);
|
||||
});
|
||||
|
||||
test("a typo never yields a wallet, let alone a different one", () => {
|
||||
const correct = wallet.hdWalletFromXprv(BIP32_VECTOR_1_XPRV);
|
||||
const derived = [];
|
||||
for (const typo of singleCharacterTypos(BIP32_VECTOR_1_XPRV)) {
|
||||
try {
|
||||
derived.push(wallet.hdWalletFromXprv(typo).firstAddress);
|
||||
} catch {
|
||||
// Rejected, which is the required behaviour.
|
||||
}
|
||||
}
|
||||
|
||||
expect(derived).toEqual([]);
|
||||
expect(correct.firstAddress).toBe(
|
||||
"0x022b971dFF0C43305e691DEd7a14367AF19D6407",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("extended key depth", () => {
|
||||
// hdWalletFromXprv derives the BIP-44 Ethereum account path from the key
|
||||
// it is given. That is only the path it names when the key is the master
|
||||
// key. Under an account-level key the same derivation lands at
|
||||
// m/44'/60'/0'/44'/60'/0'/0, whose addresses correspond to nothing the
|
||||
// user holds, so a non-master key is refused rather than derived from.
|
||||
test("a master key is a master key", () => {
|
||||
expect(wallet.isMasterExtendedKey(masterXprv(VECTOR_PHRASE))).toBe(
|
||||
true,
|
||||
);
|
||||
expect(wallet.isMasterExtendedKey(BIP32_VECTOR_1_XPRV)).toBe(true);
|
||||
});
|
||||
|
||||
test("an account-level key is not a master key", () => {
|
||||
expect(wallet.isMasterExtendedKey(accountXprv(VECTOR_PHRASE))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
test("a derived xpub is not a master key", () => {
|
||||
expect(
|
||||
wallet.isMasterExtendedKey(
|
||||
wallet.hdWalletFromMnemonic(VECTOR_PHRASE).xpub,
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
test("a mistyped key is not a master key either", () => {
|
||||
expect(wallet.isMasterExtendedKey(BIP32_VECTOR_1_XPRV + "a")).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
test("hdWalletFromXprv rejects an account-level key", () => {
|
||||
expect(() =>
|
||||
wallet.hdWalletFromXprv(accountXprv(VECTOR_PHRASE)),
|
||||
).toThrow(/master/i);
|
||||
});
|
||||
|
||||
test("getSignerForAddress rejects an account-level key", () => {
|
||||
expect(() =>
|
||||
wallet.getSignerForAddress(
|
||||
{ type: "xprv" },
|
||||
0,
|
||||
accountXprv(VECTOR_PHRASE),
|
||||
),
|
||||
).toThrow(/master/i);
|
||||
});
|
||||
|
||||
test("the account-level key is well-formed, so only depth rejects it", () => {
|
||||
expect(wallet.isValidXprv(accountXprv(VECTOR_PHRASE))).toBe(true);
|
||||
});
|
||||
|
||||
test("a master key still imports and derives the published addresses", () => {
|
||||
const { xpub, firstAddress } = wallet.hdWalletFromXprv(
|
||||
masterXprv(VECTOR_PHRASE),
|
||||
);
|
||||
|
||||
expect(firstAddress).toBe(VECTOR_ADDRESSES[0]);
|
||||
expect(
|
||||
[0, 1, 2].map((i) => wallet.deriveAddressFromXpub(xpub, i)),
|
||||
).toEqual(VECTOR_ADDRESSES);
|
||||
});
|
||||
});
|
||||
|
||||
describe("deriveAddressFromXpub checksum enforcement", () => {
|
||||
// The xpub path shares the hole: fromExtendedKey accepts a mistyped xpub
|
||||
// just as readily, and deriveAddressFromXpub would hand back addresses
|
||||
// from a different tree.
|
||||
const { xpub } = wallet.hdWalletFromMnemonic(VECTOR_PHRASE);
|
||||
|
||||
test("the correct xpub still derives the published addresses", () => {
|
||||
expect(wallet.deriveAddressFromXpub(xpub, 0)).toBe(VECTOR_ADDRESSES[0]);
|
||||
});
|
||||
|
||||
test("no single-character typo anywhere in an xpub is accepted", () => {
|
||||
const derived = [];
|
||||
for (const typo of singleCharacterTypos(xpub)) {
|
||||
try {
|
||||
derived.push(wallet.deriveAddressFromXpub(typo, 0));
|
||||
} catch {
|
||||
// Rejected, which is the required behaviour.
|
||||
}
|
||||
}
|
||||
|
||||
expect(derived).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isValidMnemonic", () => {
|
||||
|
||||
Reference in New Issue
Block a user