fix: closing a transaction or signature approval window leaves the request pending (closes #673)
Closing a transaction or signature approval window no longer rejects the request; only Cancel does. The browser can close that window when the user clicks away, for example to copy the password from a password manager. The request stays pending and the site gets no answer. Pressing the toolbar button opens the oldest such request, unless an approved attempt still holds it. A Reject the background refuses keeps the window open with the background's message. The worker points the toolbar back at the wallet when it starts. While such a request waits, the transaction slot stays held, so another site's eth_sendTransaction gets -32002. An address switch still rejects pending approvals with 4001. Model: opus-5-5
This commit was merged in pull request #676.
This commit is contained in:
@@ -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
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user