harden: verify all approval fields and make failed signing retryable (closes #174)
All checks were successful
check / check (push) Successful in 27s

verifySignedTx compared only from, to, value and data, so a signed
transaction could differ from the approval in chain id, nonce, gas limit
or any fee field and still be broadcast. Worse, it named the fields it
checked and so admitted every field it did not name: a type 4 artifact
carrying an EIP-7702 authorization passed verification, paying the
approved amount to the approved recipient and, in the same transaction,
permanently installing another contract's code at the signer's own
account.

The check is now an allowlist in both directions. The transaction type
must be 0, 1 or 2 — the only types this wallet signs — so no later
EIP-2718 type can bring a field along; authorizationList, blobs, blob
commitments and blob gas fees are refused by name; and the access list
is compared with the approval. Every consequential field is compared and
any mismatch refuses outright: the chain id against the selected network
(and against the approval when the page fixed one), plus nonce, gas
limit, gasPrice, maxFeePerGas and maxPriorityFeePerGas wherever the
approval carries a value, together with the fee mechanism the approval
implies. Fields the approval does not carry are populated locally by the
popup and have no approved value to compare against, so they are held to
absolute ceilings instead. Verification then closes by rebuilding the
transaction from exactly those checked fields and comparing the unsigned
bytes, so an artifact carrying anything this module does not account for
is refused without having to be named first.

An approved value that is not a number now refuses like every other
quantity rather than escaping as a raw BigInt conversion error, which
was reported as retryable and left a live button that could never
succeed.

A failed signing attempt also left a button that could not succeed: the
background deleted the approval before it broadcast, so a retry found
nothing to sign. The approval is now retired once the request has an
outcome, and the background tells the popup which stage failed. A popup
that could not sign is retryable; a mismatch spends the approval; a
failed broadcast is terminal, because the node may have accepted the
transaction and still failed to answer and the popup's retry re-signs at
a freshly fetched nonce rather than re-broadcasting the same bytes,
which would send the approved transfer twice.

Keeping the approval alive for that retry cost it its single use: the
handler read it, then verified and broadcast asynchronously, so a second
AUTISTMASK_TX_RESPONSE carrying the same id started an independent
verify and broadcast instead of finding nothing. With the ordinary dApp
approval shape the page fixes no nonce, so two artifacts signed at
different nonces both verify and the approved transfer goes out twice; a
reloaded approval window during a slow broadcast is enough to send it,
since the only guard was popup-local button state. The approval is now
claimed synchronously, before the first await, and released only when an
attempt fails in a way the user may retry. Same interlock on
AUTISTMASK_SIGN_RESPONSE.

Surviving the whole verify-and-broadcast window put the approval within
reach of every other path that retires one, and those paths did not
consult the claim. Closing the approval popup, switching the active
address, or a reject arriving late each resolved the waiting promise
4001 while the attempt behind it ran to completion; the attempt's own
resolve then landed on a settled promise, so the transaction reached the
chain and the page was told the user rejected it. The user's natural
response is to redo the transfer from the site, which re-signs at a
fresh nonce and sends it twice — the outcome this change exists to
prevent, reached without an adversary, since the popup stays open across
the broadcast and a user closing an apparently-hung window is enough.

Every settlement now goes through one function. settleApproval() is the
only place an approval is resolved or removed, and it refuses a claimed
approval unless the caller holds the claim, so a path added later
inherits the interlock instead of having to remember it. The active-
address switch also leaves a claimed approval's window standing rather
than force-closing the window the attempt is reporting into. The
duplicate refusal on the sign path now carries a stage of its own, so
the popup stops telling the user to start again from the site while a
first attempt may still succeed.

Verification also compared only the decode against itself: both sides of
the closing byte comparison derive from one Transaction.from(), while
what is broadcast is the artifact string. An artifact re-encoded with a
leading zero byte on an RLP quantity therefore decoded to the approved
transaction, passed, and broadcast different bytes. The artifact is now
required to be the canonical encoding of its own decode, which is what
makes the claim that it *is* the approved transaction true.

The background's approval wiring had no tests, which is where these
defects lived. It has them now, driven through the real message listener
from eth_sendTransaction to broadcast, with windows.onRemoved captured
rather than stubbed away: each retirement path is asserted to leave a
mid-broadcast attempt alone and to still reject an approval no attempt
holds.
This commit is contained in:
2026-08-12 08:44:41 +00:00
parent bd4bdcafc7
commit 4e2ca87a06
6 changed files with 2227 additions and 123 deletions

View File

@@ -0,0 +1,679 @@
// The background's approval message wiring, driven end to end: a dApp
// eth_sendTransaction raises a pending approval, and the popup answers it with
// AUTISTMASK_TX_RESPONSE / AUTISTMASK_SIGN_RESPONSE.
//
// What this exists for is the duplicate response. The handler verifies and
// broadcasts asynchronously, and the approval deliberately survives a
// retryable failure so the user can try again with the transaction they
// already saw — which means the entry being present is not by itself proof
// that no attempt is running. A second response carrying the same id (a
// reloaded approval window re-rendering a live Approve button, a popup that
// emits the message twice) must not start a second verify and broadcast: with
// the ordinary dApp approval shape the page fixes no nonce, so two artifacts
// signed at different nonces both verify, and the approved transfer would go
// out twice.
const { Wallet } = require("ethers");
const SIGNER_KEY =
"0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
const signer = new Wallet(SIGNER_KEY);
const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
const ORIGIN = "https://dapp.example";
const HOSTNAME = "dapp.example";
const EXT_URL = "chrome-extension://autistmask/";
// What the dApp asks for: no nonce, no gas, no fees. This is the shape that
// makes a duplicate broadcast possible at all.
const TX_PARAMS = {
from: signer.address,
to: RECIPIENT,
value: "0x2386f26fc10000",
data: "0x",
};
// The fields the popup's populateTransaction() would fill in. The nonce is a
// parameter because the duplicate case turns on the two artifacts differing
// in exactly the field nothing constrains.
function populated(nonce) {
return {
type: 2,
chainId: 1,
nonce,
gasLimit: 100000n,
maxFeePerGas: 2000000000n,
maxPriorityFeePerGas: 1000000000n,
to: TX_PARAMS.to,
value: BigInt(TX_PARAMS.value),
data: TX_PARAMS.data,
};
}
function signedAtNonce(nonce) {
return signer.signTransaction(populated(nonce));
}
// A promise whose settlement the test controls, so a broadcast can be held in
// flight while the second response arrives.
function deferred() {
let resolve;
let reject;
const promise = new Promise((res, rej) => {
resolve = res;
reject = rej;
});
return { promise, resolve, reject };
}
// Load the background worker against stubbed browser and network APIs and
// return the handles the tests drive it through. Everything that would touch
// the network or the browser's own schedulers is mocked; the approval
// verification is the real module, because that is what the handler under
// test is wired to.
function loadBackground(options) {
const opts = options || {};
jest.resetModules();
const broadcastTransaction = jest.fn();
const loadState = jest.fn(opts.loadState || (async () => {}));
jest.doMock("../src/shared/state", () => ({
state: { rpcUrl: "https://rpc.invalid", wallets: [] },
loadState,
saveState: jest.fn(async () => {}),
currentNetwork: () => ({ chainId: "0x1" }),
}));
jest.doMock("../src/shared/balances", () => ({
getProvider: () => ({ broadcastTransaction }),
refreshBalances: jest.fn(async () => {}),
}));
jest.doMock("../src/shared/phishingDomains", () => ({
isPhishingDomain: () => false,
refreshPhishingListOnSchedule: jest.fn(async () => {}),
initPhishingList: jest.fn(async () => {}),
}));
jest.doMock("../src/shared/alarms", () => ({
BALANCE_REFRESH_ALARM: "balance",
PHISHING_REFRESH_ALARM: "phishing",
BALANCE_REFRESH_PERIOD_MINUTES: 1,
ensureRecurringAlarms: jest.fn(async () => {}),
registerAlarmHandlers: jest.fn(),
}));
const persisted = {
wallets: [
{ name: "Wallet 1", type: "hd", addresses: [signer.address] },
],
rpcUrl: "https://rpc.invalid",
activeAddress: signer.address,
allowedSites: { [signer.address]: [HOSTNAME] },
deniedSites: {},
};
let messageListener = null;
let windowRemovedListener = null;
const created = [];
const removed = [];
global.chrome = {
storage: {
local: {
get: jest.fn(async () => ({ autistmask: persisted })),
set: jest.fn(async () => {}),
},
},
runtime: {
getURL: (path) => EXT_URL + path,
onMessage: {
addListener: (fn) => {
messageListener = fn;
},
},
onConnect: { addListener: () => {} },
lastError: null,
},
windows: {
getLastFocused: (cb) => cb(null),
create: (options2, cb) => {
created.push(options2);
cb({ id: created.length });
},
remove: (id, cb) => {
removed.push(id);
if (cb) cb();
},
// Captured, not swallowed: closing the approval window is the
// event that used to retire an approval out from under a live
// broadcast, and a no-op stub here hides exactly that.
onRemoved: {
addListener: (fn) => {
windowRemovedListener = fn;
},
},
},
tabs: {
query: (q, cb) => cb([]),
sendMessage: () => {},
},
action: { setPopup: () => {} },
};
require("../src/background/index");
// Send a message the way the browser would, and hand back whatever the
// handler passed to sendResponse.
function send(msg, sender) {
const sendResponse = jest.fn();
const kept = messageListener(msg, sender || {}, sendResponse);
return { sendResponse, kept };
}
// Raise a pending transaction approval the way a dApp does, and dig the
// approval id back out of the popup URL the background opened.
function requestTx() {
let rpcResult = null;
const sendResponse = jest.fn((r) => {
rpcResult = r;
});
messageListener(
{
type: "AUTISTMASK_RPC",
method: "eth_sendTransaction",
params: [TX_PARAMS],
},
{ origin: ORIGIN },
sendResponse,
);
return {
id: () => new URL(created[0].url).searchParams.get("approval"),
result: () => rpcResult,
};
}
// The user closes the approval popup. `created` is index-aligned with the
// ids the window stub hands back, so window 1 is the first popup opened.
function closeWindow(windowId) {
windowRemovedListener(windowId);
}
return {
send,
requestTx,
closeWindow,
broadcastTransaction,
loadState,
created,
removed,
fromPopup: { url: EXT_URL + "src/popup/index.html" },
};
}
// Let the handler's promise chain run to the next suspension point.
async function settle() {
for (let i = 0; i < 10; i++) await Promise.resolve();
}
afterEach(() => {
delete global.chrome;
jest.resetModules();
});
describe("one approval, one broadcast", () => {
test("a second AUTISTMASK_TX_RESPONSE for the same id does not broadcast again", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
expect(id).toBeTruthy();
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
// The popup answers. Verification passes and the broadcast is held
// open, which is the whole window the second message arrives in.
const first = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
// A reloaded approval window signs the same approval again. Nothing
// in the approval fixes a nonce, so this artifact verifies just as
// well as the first one.
const second = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(8),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
expect(second.sendResponse).toHaveBeenCalledWith(
expect.objectContaining({
error: expect.stringMatching(/already being sent/),
retryable: false,
}),
);
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" });
expect(pending.result()).toEqual({ result: "0xfeed" });
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
});
test("the same artifact sent twice broadcasts once", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
const raw = await signedAtNonce(7);
const msg = {
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: raw,
};
bg.send(msg, { url: bg.fromPopup.url });
bg.send(msg, { url: bg.fromPopup.url });
await settle();
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
});
test("a response arriving after the broadcast finished finds nothing to send", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" });
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
const late = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(8),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
expect(late.sendResponse).not.toHaveBeenCalled();
});
test("a second AUTISTMASK_SIGN_RESPONSE for the same id is refused", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
// Hold the transaction approval in flight, then answer it a second
// time as if it were a sign approval: the sign handler must apply the
// same interlock rather than running its own verification.
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
const second = bg.send(
{
type: "AUTISTMASK_SIGN_RESPONSE",
id,
approved: true,
signature: "0x00",
},
{ url: bg.fromPopup.url },
);
await settle();
expect(second.sendResponse).toHaveBeenCalledWith(
expect.objectContaining({
error: expect.stringMatching(/already being signed/),
retryable: false,
}),
);
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
});
});
// The interlock must not cost the retry the approval exists to allow.
describe("the interlock releases a failed attempt", () => {
test("a retryable failure before the broadcast leaves the approval usable", async () => {
let failNext = true;
const bg = loadBackground({
loadState: async () => {
if (failNext) {
failNext = false;
throw new Error("storage unavailable");
}
},
});
const pending = bg.requestTx();
await settle();
const id = pending.id();
const first = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).not.toHaveBeenCalled();
expect(first.sendResponse).toHaveBeenCalledWith(
expect.objectContaining({ retryable: true }),
);
bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" });
const retry = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
expect(retry.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" });
expect(pending.result()).toEqual({ result: "0xfeed" });
});
test("a mismatched artifact spends the approval outright", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
// Signed for a different recipient than the one that was approved.
const wrong = await signer.signTransaction({
...populated(7),
to: "0xdAC17F958D2ee523a2206206994597C13D831ec7",
});
const first = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: wrong,
},
{ url: bg.fromPopup.url },
);
await settle();
expect(first.sendResponse).toHaveBeenCalledWith(
expect.objectContaining({ retryable: false, stage: "verify" }),
);
const retry = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).not.toHaveBeenCalled();
expect(retry.sendResponse).not.toHaveBeenCalled();
});
});
// The claim is what makes one approval one broadcast, so it has to hold
// against everything else that retires an approval, not just against a second
// AUTISTMASK_TX_RESPONSE. Each of these paths used to resolve the waiting
// promise 4001 while the attempt behind it ran to completion: the transaction
// reached the chain and the page was told the user rejected it, which invites
// the user to send it a second time at a fresh nonce.
describe("a claimed approval outlives every other retirement path", () => {
// The approval popup stays open across the broadcast it is waiting on, so
// a user closing an apparently-hung window needs no adversary at all.
test("closing the approval window mid-broadcast still reports the result", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
const first = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
// The user closes the window while the broadcast is still open.
bg.closeWindow(1);
await settle();
expect(pending.result()).toBeNull();
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(pending.result()).toEqual({ result: "0xfeed" });
expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" });
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
});
test("switching the active address mid-broadcast still reports the result", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
// The user switches account in the toolbar popup, which rejects and
// force-closes every pending approval.
bg.send(
{ type: "AUTISTMASK_ACTIVE_CHANGED" },
{ url: bg.fromPopup.url },
);
await settle();
expect(pending.result()).toBeNull();
// The window an in-flight attempt reports into is left standing too.
expect(bg.removed).toEqual([]);
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(pending.result()).toEqual({ result: "0xfeed" });
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
});
test("a reject arriving mid-broadcast is refused, not honoured", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
const reject = bg.send(
{ type: "AUTISTMASK_TX_RESPONSE", id, approved: false },
{ url: bg.fromPopup.url },
);
await settle();
expect(pending.result()).toBeNull();
expect(reject.sendResponse).toHaveBeenCalledWith(
expect.objectContaining({
retryable: false,
stage: "broadcast",
}),
);
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(pending.result()).toEqual({ result: "0xfeed" });
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
});
// The refusals above must not cost the rejection its ordinary meaning.
test("with no attempt running, closing the window still rejects", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
bg.closeWindow(1);
await settle();
expect(pending.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
expect(bg.broadcastTransaction).not.toHaveBeenCalled();
});
test("with no attempt running, an active-address switch still rejects and closes", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
bg.send(
{ type: "AUTISTMASK_ACTIVE_CHANGED" },
{ url: bg.fromPopup.url },
);
await settle();
expect(pending.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
expect(bg.removed).toEqual([1]);
});
// A sign approval held by a running verification is the same shape, and
// the refusal must not tell the user to start again from the site while
// the first attempt may still hand back a signature.
test("a reject during a sign attempt is refused with the in-flight stage", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
const inFlight = deferred();
bg.broadcastTransaction.mockReturnValue(inFlight.promise);
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: bg.fromPopup.url },
);
await settle();
const reject = bg.send(
{ type: "AUTISTMASK_SIGN_RESPONSE", id, approved: false },
{ url: bg.fromPopup.url },
);
await settle();
expect(reject.sendResponse).toHaveBeenCalledWith(
expect.objectContaining({ retryable: false, stage: "inflight" }),
);
inFlight.resolve({ hash: "0xfeed" });
await settle();
expect(pending.result()).toEqual({ result: "0xfeed" });
});
});
describe("popup-only messages", () => {
test("a page sender cannot answer an approval", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
const id = pending.id();
const spoof = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id,
approved: true,
rawSignedTx: await signedAtNonce(7),
},
{ url: ORIGIN + "/index.html" },
);
await settle();
expect(bg.broadcastTransaction).not.toHaveBeenCalled();
expect(spoof.sendResponse).toHaveBeenCalledWith({
error: "Unauthorized sender",
});
});
});