fix: closing a transaction or signature approval window leaves the request pending (closes #673)
check / check (push) Waiting to run
e2e / e2e-chrome (push) Waiting to run
e2e / e2e-firefox (push) Waiting to run

The browser can close the approval window when the user clicks away from it,
for example to copy the password from a password manager. Closing it used to
reject the request with 4001. Now the request stays pending and the site gets
no answer. Until it is answered, the toolbar popup opens the oldest such
approval. A worker that starts holds no approvals, so it points the toolbar
popup back at the wallet.

Reject now closes its window only after the background has the rejection. Sent
as the window closed, the rejection could be lost, which the old window-close
rejection hid.

Site-connection and network-switch windows still reject the request when
closed.

Model: opus-5-5
This commit is contained in:
2026-10-26 21:18:24 +00:00
parent bf8aa9f5dd
commit df0de00821
9 changed files with 592 additions and 162 deletions
+85
View File
@@ -12,6 +12,7 @@ const {
cleanupPopup,
settle,
unversionedValidProfile,
ADDRESS,
} = require("./support/popupBoot");
afterEach(cleanupPopup);
@@ -174,3 +175,87 @@ test("a connection prompt in the toolbar popup leaves the browser window alone",
expect(windows.updates).toEqual([]);
expect(env.pageErrors).toEqual([]);
});
// A closed transaction or signature window leaves its request pending
// (https://git.eeqj.de/sneak/AutistMask/issues/673). So Reject closes the
// window only once the background has the rejection. A message sent as the
// window closes can be lost, and the request would stay pending although the
// user rejected it.
test.each([
{
kind: "transaction",
view: "approve-tx",
reject: "btn-reject-tx",
type: "AUTISTMASK_TX_RESPONSE",
approval: {
type: "tx",
approvedFrom: ADDRESS,
approvedTx: {
chainId: 1,
to: ADDRESS,
value: "0",
data: "0x",
gasLimit: "21000",
maxFeePerGas: "1000000000",
maxPriorityFeePerGas: "1000000000",
nonce: 0,
},
},
},
{
kind: "signature",
view: "approve-sign",
reject: "btn-reject-sign",
type: "AUTISTMASK_SIGN_RESPONSE",
approval: {
type: "sign",
approvedFrom: ADDRESS,
signParams: {
method: "personal_sign",
message: "0x48656c6c6f",
from: ADDRESS,
},
},
},
])(
"Reject on a $kind approval closes the window only once the background has answered",
async ({ view, reject, type, approval }) => {
let describeApproval = null;
let answerReject = null;
const sent = [];
const env = await bootPopup(unversionedValidProfile(), {
search: "?approval=approval-1",
runtime: {
connect: () => ({ postMessage: () => {} }),
sendMessage: (msg, reply) => {
if (msg.type === "AUTISTMASK_GET_APPROVAL") {
describeApproval = reply;
}
if (msg.type === type) {
sent.push(msg);
answerReject = reply;
}
},
},
});
describeApproval({
...approval,
origin: "https://dapp.example",
isPhishingDomain: false,
});
await settle();
expect(env.visibleViews()).toEqual([view]);
const close = jest.fn();
globalThis.window.close = close;
const clicked = env.click(reject);
await settle();
expect(sent).toEqual([{ type, id: "approval-1", approved: false }]);
expect(close).not.toHaveBeenCalled();
answerReject(null);
await clicked;
expect(close).toHaveBeenCalledTimes(1);
expect(env.pageErrors).toEqual([]);
},
);
+210 -47
View File
@@ -241,10 +241,11 @@ function loadBackground(options) {
let connectListener = null;
const created = [];
const removed = [];
// Every URL the background put on the browser action. A site approval
// raised through action.openPopup() opens no window at all, so this is
// the only place its id appears.
const actionPopups = [];
// Every URL the background put on the browser action. When a test gives
// the URL the action holds before the worker starts, that URL comes
// first. A site approval raised through action.openPopup() opens no
// window at all, so this is the only place its id appears.
const actionPopups = opts.toolbarPopup ? [opts.toolbarPopup] : [];
const openPopup = jest.fn(() => Promise.resolve());
global.chrome = {
@@ -774,10 +775,9 @@ describe("one transaction approval at a time", () => {
// The user closes an approval window that looks hung while the attempt
// behind it is still running, and that attempt then fails in a way that
// would normally leave the approval standing for a retry. There is no
// window left to retry in, so leaving it standing answers the requesting
// page never — and holds the slot for the life of the worker with it.
test("an approval whose window closed under a failed attempt is answered, and frees the next request", async () => {
// leaves the approval standing for a retry. The toolbar popup opens it, so
// the user can still retry it, and answering it frees the slot.
test("an approval whose window closed under a failed attempt is retried from the toolbar popup, and frees the next request", async () => {
const stalled = deferred();
const bg = loadBackground();
@@ -808,11 +808,28 @@ describe("one transaction approval at a time", () => {
stalled.resolve();
await settle();
expect(first.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
expect(first.result()).toBeNull();
expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe(
"src/popup/index.html?approval=" + first.id(),
);
bg.setStateReadHook(null);
bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" });
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id: first.id(),
approved: true,
rawSignedTx: await signedAtNonce(NONCE),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(first.result()).toEqual({ result: "0xfeed" });
expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe(
"src/popup/index.html",
);
const second = bg.requestTx();
await settle();
expect(second.result()).toBeNull();
@@ -853,8 +870,10 @@ describe("one transaction approval at a time", () => {
const first = bg.requestTx();
await settle();
// The user closes the approval window, which rejects it.
bg.closeWindow(1);
bg.send(
{ type: "AUTISTMASK_TX_RESPONSE", id: first.id(), approved: false },
{ url: bg.fromPopup.url },
);
await settle();
expect(first.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
@@ -897,7 +916,7 @@ describe("the worker keeps running while an approval is pending", () => {
try {
const bg = loadBackground();
const calls = global.chrome.runtime.getPlatformInfo;
bg.requestTx();
const pending = bg.requestTx();
await settle();
expect(startTimer).toHaveBeenCalledTimes(1);
expect(startTimer.mock.calls[0][1]).toBe(20000);
@@ -906,11 +925,23 @@ describe("the worker keeps running while an approval is pending", () => {
tick();
expect(calls).toHaveBeenCalledTimes(1);
// The user closes the approval window, which rejects it.
// A closed window leaves the approval pending, so the calls go on.
bg.closeWindow(1);
await settle();
tick();
expect(calls).toHaveBeenCalledTimes(1);
expect(calls).toHaveBeenCalledTimes(2);
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id: pending.id(),
approved: false,
},
{ url: bg.fromPopup.url },
);
await settle();
tick();
expect(calls).toHaveBeenCalledTimes(2);
expect(stopTimer).toHaveBeenCalledWith(1);
} finally {
startTimer.mockRestore();
@@ -1841,6 +1872,153 @@ describe("the interlock releases a failed attempt", () => {
});
});
// A transaction or signature approval stays pending when its window closes.
// The browser can close that window when the user clicks away from it, for
// example to copy the password from a password manager. The toolbar popup then
// opens the approval, and the user answers it there
// (https://git.eeqj.de/sneak/AutistMask/issues/673).
describe("a closed approval window leaves the request pending", () => {
// The URL the toolbar popup opens now.
function toolbarPopup(bg) {
return bg.actionPopups[bg.actionPopups.length - 1];
}
// The timer that keeps the worker running while an approval is pending is
// stubbed, so no test here leaves that timer running when it ends.
let keepRunningTimer;
beforeEach(() => {
keepRunningTimer = jest
.spyOn(global, "setInterval")
.mockImplementation(() => 1);
});
afterEach(() => {
keepRunningTimer.mockRestore();
});
test("the toolbar popup opens a closed transaction approval, and approving it there sends", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
bg.closeWindow(1);
await settle();
expect(pending.result()).toBeNull();
expect(toolbarPopup(bg)).toBe(
"src/popup/index.html?approval=" + pending.id(),
);
bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" });
bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
id: pending.id(),
approved: true,
rawSignedTx: await signedAtNonce(NONCE),
},
{ url: bg.fromPopup.url },
);
await settle();
expect(pending.result()).toEqual({ result: "0xfeed" });
expect(toolbarPopup(bg)).toBe("src/popup/index.html");
});
test("Reject in the toolbar popup answers 4001, and the toolbar popup opens the wallet again", async () => {
const bg = loadBackground();
const pending = bg.requestSign();
await settle();
bg.closeWindow(1);
await settle();
expect(pending.result()).toBeNull();
expect(toolbarPopup(bg)).toBe(
"src/popup/index.html?approval=" + pending.id(),
);
const reject = bg.send(
{
type: "AUTISTMASK_SIGN_RESPONSE",
id: pending.id(),
approved: false,
},
{ url: bg.fromPopup.url },
);
await settle();
expect(pending.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
// The popup closes its window only once this answer arrives.
expect(reject.sendResponse).toHaveBeenCalledWith(null);
expect(toolbarPopup(bg)).toBe("src/popup/index.html");
});
test("with two closed, the toolbar popup opens the older one first, then the other", async () => {
const bg = loadBackground();
const sign = bg.requestSign();
await settle();
const signId = sign.id();
const tx = bg.requestTx();
await settle();
// The signature request is in window 1 and the transaction in window
// 2. The newer window is closed first.
bg.closeWindow(2);
bg.closeWindow(1);
await settle();
expect(toolbarPopup(bg)).toBe(
"src/popup/index.html?approval=" + signId,
);
bg.send(
{ type: "AUTISTMASK_SIGN_RESPONSE", id: signId, approved: false },
{ url: bg.fromPopup.url },
);
await settle();
expect(toolbarPopup(bg)).toBe(
"src/popup/index.html?approval=" + tx.id(),
);
const reject = bg.send(
{ type: "AUTISTMASK_TX_RESPONSE", id: tx.id(), approved: false },
{ url: bg.fromPopup.url },
);
await settle();
expect(tx.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
expect(reject.sendResponse).toHaveBeenCalledWith(null);
expect(toolbarPopup(bg)).toBe("src/popup/index.html");
});
test("an address switch still rejects a closed approval", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
bg.closeWindow(1);
await settle();
expect(pending.result()).toBeNull();
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(toolbarPopup(bg)).toBe("src/popup/index.html");
});
// The worker can stop while an approval waits with its window closed. The
// approval is lost with the worker, but the toolbar popup keeps its URL.
// The next worker points the toolbar popup back at the wallet.
test("a worker that starts points the toolbar popup back at the wallet", () => {
const bg = loadBackground({
toolbarPopup: "src/popup/index.html?approval=lost-approval",
});
expect(toolbarPopup(bg)).toBe("src/popup/index.html");
});
});
// 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
@@ -1961,20 +2139,6 @@ describe("a claimed approval outlives every other retirement path", () => {
});
// 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();
@@ -2584,15 +2748,12 @@ describe("a site connection decided as the popup closes", () => {
});
});
// The `isSite &&` half of that skip, which is what keeps it from reaching
// a tx or sign approval. The popup connects its port in show() before it
// knows the approval's type, and the background sets portConnected without
// looking at the type either, so a tx approval in the fallback window
// carries the flag too. Without the conjunct the window event would skip
// it, windowClosed would never be set, releaseApproval() would never settle
// it, and the page would hang — the #271 regression this guard is written
// around.
test("a tx window closed with the port connected still rejects", async () => {
// That skip must not reach a tx or sign approval. The popup connects its
// port in show() before it knows the approval's type, and the background
// sets portConnected without looking at the type either, so a tx approval
// carries the flag too. If the window event skipped it, windowClosed would
// never be set, and the toolbar popup would never open the approval.
test("a tx window closed with the port connected is still opened by the toolbar popup", async () => {
const bg = loadBackground();
const pending = bg.requestTx();
await settle();
@@ -2600,9 +2761,10 @@ describe("a site connection decided as the popup closes", () => {
await settle();
bg.closeWindow(1);
await settle();
expect(pending.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
expect(pending.result()).toBeNull();
expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe(
"src/popup/index.html?approval=" + pending.id(),
);
});
});
@@ -2994,13 +3156,14 @@ describe("where an approval window opens", () => {
expect(bg.created[1].left).toBeUndefined();
expect(bg.created[1].top).toBeUndefined();
// The request waits on the second window rather than failing:
// closing that window is refusing the prompt.
// The request waits on the second window rather than failing. Once
// that window closes, the toolbar popup opens the request.
expect(sign.result()).toBeNull();
bg.closeWindow(2);
await settle();
expect(sign.result()).toEqual({
error: { code: 4001, message: "User rejected the request." },
});
expect(sign.result()).toBeNull();
expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe(
"src/popup/index.html?approval=" + sign.id(),
);
});
});
+41 -8
View File
@@ -1035,11 +1035,14 @@ step(
},
);
// A closed approval window leaves its request pending, and the toolbar button
// opens it (https://git.eeqj.de/sneak/AutistMask/issues/673). WebDriver cannot
// see the toolbar popup, so its page is opened in a tab, at the URL the
// extension put on the button.
step(
"closing an approval window rejects the request with 4001",
"a closed approval window leaves the request pending until it is rejected",
async (env) => {
const d = env.driver;
const before = env.server.broadcast.length;
await d.switchToWindow(env.dappWindow);
await startRequest(d, "sign-closed", "personal_sign", [
@@ -1053,15 +1056,45 @@ step(
// Closed, not rejected: this is the windows.onRemoved path, which can
// only fire if windows.create() handed back a window id for the
// approval to be matched against — call site 4 in the issue, and the
// reason suppressing that write-back turns this step red.
// approval to be matched against. Without that id the toolbar button
// never opens the approval, and this step fails.
await d.closeWindow(env.dappWindow);
await assertUserRejection(d, "sign-closed", "a closed approval window");
const outcome = await settleRequest(d, "sign-closed", 2000);
assert(
env.server.broadcast.length === before,
"a closed approval window still put a transaction on the node",
outcome.settled === "pending",
"closing the approval window answered the page: " +
JSON.stringify(outcome),
);
await d.switchToWindow(env.popupWindow);
const url = await d.executeAsync(
`const done = arguments[arguments.length - 1];
browser.browserAction.getPopup({}).then(done, (e) => done(String(e)));`,
);
assert(
url.includes("?approval="),
"the toolbar button does not open the pending approval: " + url,
);
const reopened = await d.newWindow("tab");
await d.switchToWindow(reopened);
await d.navigate(url);
await d.waitVisible("#view-approve-sign");
await d.click("#btn-reject-sign");
await d.switchToWindow(env.dappWindow);
await assertUserRejection(
d,
"sign-closed",
"the signature request rejected from the toolbar button",
);
// A page may close only a window a script opened, so the tab can
// still be open.
const left = await d.findWindow((u) => u.includes("?approval="));
if (left) {
await d.switchToWindow(left);
await d.closeWindow(env.dappWindow);
}
},
);
+135 -16
View File
@@ -4110,11 +4110,12 @@ async function waitForApprovalWindow(ctx, timeout = 30000) {
// reservation exists.
// How long the tab creation above is given to take effect before the request
// that raises the next prompt is issued. Dismissing a browser-action popup
// disconnects its port, and src/background/index.js calls resetPopupUrl() on
// that disconnect; issued too early, the next request's setPopup() is undone
// by the previous popup's teardown, chrome.action.openPopup() then rejects,
// and the fallback window it opens dismisses the popup that was just raised —
// which settles the fresh approval as a rejection before it can be seen.
// disconnects its port, and src/background/index.js calls
// updateToolbarPopup() on that disconnect; issued too early, the next
// request's setPopup() is undone by the previous popup's teardown,
// chrome.action.openPopup() then rejects, and the fallback window it opens
// dismisses the popup that was just raised — which settles the fresh approval
// as a rejection before it can be seen.
// Every one of those steps was observed. 1.5s is comfortably past it.
const APPROVAL_TAB_SETTLE_MS = 1500;
@@ -4140,6 +4141,17 @@ async function reserveApprovalTab(env) {
return env.approvalTab;
}
// The URL the extension has put on the action: the page the toolbar button
// opens.
function toolbarPopupUrl(env) {
return env.page.evaluate(
() =>
new Promise((resolve) => {
chrome.action.getPopup({}, (u) => resolve(String(u)));
}),
);
}
// The site-connection prompt: the window the extension opened if it managed
// to open one, and the reserved tab at the action's approval URL otherwise.
async function openSiteApprovalPopup(env, timeout = 30000) {
@@ -4150,12 +4162,7 @@ async function openSiteApprovalPopup(env, timeout = 30000) {
.find((p) => !p.isClosed() && p.url().includes("?approval="));
if (existing) return existing;
const url = await env.page.evaluate(
() =>
new Promise((resolve) => {
chrome.action.getPopup({}, (u) => resolve(String(u)));
}),
);
const url = await toolbarPopupUrl(env);
if (url.includes("?approval=")) {
await env.approvalTab.goto(url);
return env.approvalTab;
@@ -5027,6 +5034,116 @@ test("eth_sendTransaction rejected broadcasts nothing (#183)", async (env) => {
);
});
// A transaction or signature approval stays pending when its window closes.
// The browser can close that window when the user clicks away from it, for
// example to copy the password from a password manager. The toolbar button then
// opens the approval, and the user answers it there
// (https://git.eeqj.de/sneak/AutistMask/issues/673).
//
// This closes the approval window of request `key` and checks that the page
// has no answer. It then opens the approval again from the toolbar button.
// Playwright cannot click that button, so its page is opened in a tab, at the
// URL the extension put on the button.
async function closeAndReopen(env, key, view) {
const approvalWindow = await waitForApprovalWindow(env.ctx);
await visible(approvalWindow, view);
await approvalWindow.close();
const outcome = await settleRequest(env.dapp, key, 2000);
assert(
outcome.settled === "pending",
"closing the approval window answered the page: " +
JSON.stringify(outcome),
);
const url = await toolbarPopupUrl(env);
assert(
url.includes("?approval="),
"the toolbar button does not open the pending approval: " + url,
);
const reopened = await env.ctx.newPage();
await reopened.goto(url);
await visible(reopened, view);
return reopened;
}
// The stub node answers every nonce request with 0, and the wallet refuses a
// nonce it has already broadcast. A run can therefore approve one dApp
// transaction only, so here the signature is approved and the transaction
// rejected.
test("a closed signature approval is signed from the toolbar button (#673)", async (env) => {
await startRequest(env.dapp, "sign-closed", "personal_sign", [
SIGN_HEX,
env.expectedAddress,
]);
const popup = await closeAndReopen(
env,
"sign-closed",
"#view-approve-sign",
);
let outcome;
try {
await popup.fill("#approve-sign-password", PASSWORD);
await clickAndClose(popup, "#btn-approve-sign");
outcome = await settleRequest(env.dapp, "sign-closed");
} finally {
await closeApprovalPages(env.ctx);
}
assert(
outcome.settled === "resolved",
"personal_sign did not resolve: " + JSON.stringify(outcome),
);
const recovered = getAddress(
verifyMessage(getBytes(SIGN_HEX), outcome.result),
);
assert(
recovered === env.expectedAddress,
"the personal_sign signature recovers to " +
recovered +
", not to the approved address " +
env.expectedAddress,
);
const url = await toolbarPopupUrl(env);
assert(
!url.includes("?approval="),
"the toolbar button still opens the answered approval: " + url,
);
});
test("a closed transaction approval is rejected from the toolbar button (#673)", async (env) => {
const before = env.routeOpts.broadcastTransactions.length;
await startRequest(env.dapp, "tx-closed", "eth_sendTransaction", [
{
from: env.expectedAddress,
to: STUB_COUNTERPARTY,
value: toQuantity(TX_VALUE_WEI),
data: TX_DATA,
},
]);
const popup = await closeAndReopen(env, "tx-closed", "#view-approve-tx");
try {
await clickAndClose(popup, "#btn-reject-tx");
await assertUserRejection(
env.dapp,
"tx-closed",
"the transaction rejected from the toolbar button",
);
} finally {
await closeApprovalPages(env.ctx);
}
assert(
env.routeOpts.broadcastTransactions.length === before,
"a rejected transaction still reached the RPC",
);
const url = await toolbarPopupUrl(env);
assert(
!url.includes("?approval="),
"the toolbar button still opens the answered approval: " + url,
);
});
// The extension used to centre every approval window on the last focused
// window. Centred on another approval window, the new one landed where the
// browser refused to create it, and the request failed with no window at all
@@ -5070,8 +5187,8 @@ test("a prompt raised while another approval window has focus opens its own (#29
});
await visible(txWindow, "#view-approve-tx");
// Closing a window is refusing its prompt, so both answer 4001.
await closeApprovalPages(env.ctx);
await clickAndClose(txWindow, "#btn-reject-tx");
await clickAndClose(signWindow, "#btn-reject-sign");
await assertUserRejection(env.dapp, "focus-tx", "the transaction prompt");
await assertUserRejection(env.dapp, "focus-sign", "the sign prompt");
});
@@ -5678,9 +5795,11 @@ async function main() {
// for the new one. After a test that passed, its windows get five
// seconds to close, and one still open then fails the test: it should
// have closed itself, and closing it here unreported would hide that.
// After a test that already failed, they are closed unreported, so
// that a prompt it left unanswered does not refuse the next one from
// its site.
// After a test that already failed, they are closed unreported. That
// rejects a site-connection or network-switch prompt it left
// unanswered, so the prompt does not refuse the next one from its
// site. A transaction or signature approval stays pending, and can
// fail the tests after it.
if (!failure) {
const deadline = Date.now() + 5000;
let open;