fix: open an approval window while another one has focus (closes #290)
The background centred each approval window on the last focused window,
which could be an earlier approval window still open. Headless Chrome
reports one as 1280x720, so the position came out where the browser refused
to create the window ("Bounds must be at least 50% within visible screen
space"), and the request failed with -32603 and no window. It now centres
only on a browser window and otherwise lets the browser place it.
Under load a test in the Chrome suite raised its prompt while the previous
test's window was still closing, and either hit that refusal or took the
closing window for its own. The runner now closes every approval window
between tests.
Model: opus-5-5
This commit is contained in:
@@ -381,11 +381,13 @@ handler on a reserved-TLD origin, gets `window.ethereum` from the shipped
|
|||||||
the runner and compared against the active address, the transaction assertions
|
the runner and compared against the active address, the transaction assertions
|
||||||
run against the raw signed transaction captured at `eth_sendRawTransaction`
|
run against the raw signed transaction captured at `eth_sendRawTransaction`
|
||||||
rather than against anything the extension reported, rejecting each prompt is
|
rather than against anything the extension reported, rejecting each prompt is
|
||||||
required to return a rejection to the page rather than hang or resolve, and the
|
required to return a rejection to the page rather than hang or resolve, a prompt
|
||||||
password is required to be absent from every message the approval window sends
|
raised while another approval window has focus is required to open a window of
|
||||||
to the background — with the message that would carry it required to be present,
|
its own, and the password is required to be absent from every message the
|
||||||
so that check cannot pass by observing nothing. That last one is the standing
|
approval window sends to the background — with the message that would carry it
|
||||||
floor under [#157](https://git.eeqj.de/sneak/AutistMask/issues/157).
|
required to be present, so that check cannot pass by observing nothing. That
|
||||||
|
last one is the standing floor under
|
||||||
|
[#157](https://git.eeqj.de/sneak/AutistMask/issues/157).
|
||||||
|
|
||||||
Two limits of that coverage, neither of them papered over. The RPC is stubbed
|
Two limits of that coverage, neither of them papered over. The RPC is stubbed
|
||||||
throughout, so this is **not** a real dApp against a real network with real
|
throughout, so this is **not** a real dApp against a real network with real
|
||||||
@@ -619,12 +621,9 @@ The jobs **report, they do not gate.** A failure is a red mark against the
|
|||||||
commit that a reviewer has to account for, not a hard block: whether a check
|
commit that a reviewer has to account for, not a hard block: whether a check
|
||||||
blocks a merge is Gitea branch protection, which this repo does not configure.
|
blocks a merge is Gitea branch protection, which this repo does not configure.
|
||||||
|
|
||||||
That is not only a statement about configuration. A report of the Chrome suite
|
That is not only a statement about configuration. No report of the Chrome suite
|
||||||
**failing under load** is still open:
|
**failing under load** is open now, but it has failed that way before, so a red
|
||||||
[#290](https://git.eeqj.de/sneak/AutistMask/issues/290), runs on a busy machine
|
`e2e-chrome` is read before it is believed. Do not answer one with a retry
|
||||||
failing with `the extension opened no approval window within 30000ms`. So a red
|
|
||||||
`e2e-chrome` has to be read before it is believed, and those failures are the
|
|
||||||
blocker to ever making this a required check. Do not answer them with a retry
|
|
||||||
wrapper: a suite that reruns until it is green stops being evidence.
|
wrapper: a suite that reruns until it is green stops being evidence.
|
||||||
|
|
||||||
Nothing in either job can pass vacuously. There is no `continue-on-error` and no
|
Nothing in either job can pass vacuously. There is no `continue-on-error` and no
|
||||||
@@ -1940,7 +1939,8 @@ view would leave a wallet one click from deletion.
|
|||||||
`eth_sendTransaction` arriving while one is unanswered is refused with
|
`eth_sendTransaction` arriving while one is unanswered is refused with
|
||||||
EIP-1193 code `-32002` rather than being populated at the same nonce. It opens
|
EIP-1193 code `-32002` rather than being populated at the same nonce. It opens
|
||||||
no window and takes no nonce, and the site can send it again once the pending
|
no window and takes no nonce, and the site can send it again once the pending
|
||||||
one is answered.
|
one is answered. The window is centred on the browser window the user was last
|
||||||
|
in; if that was another approval window, the browser picks the position.
|
||||||
- **Elements**:
|
- **Elements**:
|
||||||
- "Transaction Request" heading
|
- "Transaction Request" heading
|
||||||
- Phishing warning banner (shown when the hostname is on the phishing
|
- Phishing warning banner (shown when the hostname is on the phishing
|
||||||
|
|||||||
@@ -45,6 +45,17 @@ but the review is broader than any of them.
|
|||||||
|
|
||||||
# Completed Steps
|
# Completed Steps
|
||||||
|
|
||||||
|
- 2026-10-05: A prompt raised while another approval window has focus opens a
|
||||||
|
window of its own ([#290](https://git.eeqj.de/sneak/AutistMask/issues/290)).
|
||||||
|
The background centred each approval window on the last focused window, which
|
||||||
|
could be an earlier approval window still open; headless Chrome reports one as
|
||||||
|
1280x720, so the new window came out where the browser refused to create it,
|
||||||
|
and the request failed with no window at all. It now centres only on a browser
|
||||||
|
window and otherwise lets the browser place it. In the Chrome end-to-end suite
|
||||||
|
a test could raise its prompt while the previous test's window was still
|
||||||
|
closing, and then either hit that refusal or take the closing window for its
|
||||||
|
own; the runner now closes every approval window between tests.
|
||||||
|
|
||||||
- 2026-10-05: A token scale of zero decimals is tested
|
- 2026-10-05: A token scale of zero decimals is tested
|
||||||
([#325](https://git.eeqj.de/sneak/AutistMask/issues/325)).
|
([#325](https://git.eeqj.de/sneak/AutistMask/issues/325)).
|
||||||
`resolveTokenDecimals()` already used a scale of 0 from the bundled list or
|
`resolveTokenDecimals()` already used a scale of 0 from the bundled list or
|
||||||
|
|||||||
@@ -437,7 +437,13 @@ async function openApprovalWindow(id) {
|
|||||||
width: popupWidth,
|
width: popupWidth,
|
||||||
height: popupHeight,
|
height: popupHeight,
|
||||||
};
|
};
|
||||||
if (currentWin) {
|
// Centred on a browser window only. The last focused window can be
|
||||||
|
// another approval window still open, and centring on one can give a
|
||||||
|
// position the browser refuses ("Bounds must be at least 50% within
|
||||||
|
// visible screen space"): headless Chrome reports this 360x600 popup as
|
||||||
|
// 1280x720. The request then failed with no window at all. Over a popup,
|
||||||
|
// the browser picks the position.
|
||||||
|
if (currentWin && currentWin.type === "normal") {
|
||||||
opts.left = Math.round(
|
opts.left = Math.round(
|
||||||
currentWin.left + (currentWin.width - popupWidth) / 2,
|
currentWin.left + (currentWin.width - popupWidth) / 2,
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -267,7 +267,7 @@ function loadBackground(options) {
|
|||||||
lastError: null,
|
lastError: null,
|
||||||
},
|
},
|
||||||
windows: {
|
windows: {
|
||||||
getLastFocused: (cb) => cb(null),
|
getLastFocused: (cb) => cb(opts.lastFocused || null),
|
||||||
create: (options2, cb) => {
|
create: (options2, cb) => {
|
||||||
created.push(options2);
|
created.push(options2);
|
||||||
// A browser that answers with no window at all. The approval
|
// A browser that answers with no window at all. The approval
|
||||||
@@ -2716,3 +2716,47 @@ describe("removing a site in Settings disconnects it", () => {
|
|||||||
expect(await siteAccounts(bg)).toEqual({ result: [signer.address] });
|
expect(await siteAccounts(bg)).toEqual({ result: [signer.address] });
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// An approval window still open is often the last focused window, and headless
|
||||||
|
// Chrome reports one as 1280x720. Centred on that, the next approval window
|
||||||
|
// lands where the browser refuses to create it, and its request failed with no
|
||||||
|
// window at all (https://git.eeqj.de/sneak/AutistMask/issues/290).
|
||||||
|
describe("where an approval window opens", () => {
|
||||||
|
test("centred on the browser window the user was last in", async () => {
|
||||||
|
const bg = loadBackground({
|
||||||
|
lastFocused: {
|
||||||
|
type: "normal",
|
||||||
|
left: 0,
|
||||||
|
top: 0,
|
||||||
|
width: 1280,
|
||||||
|
height: 720,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
bg.requestSign();
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
expect(bg.created).toHaveLength(1);
|
||||||
|
expect(bg.created[0]).toMatchObject({ left: 460, top: 60 });
|
||||||
|
});
|
||||||
|
|
||||||
|
test("not centred on an approval window the user was last in", async () => {
|
||||||
|
const bg = loadBackground({
|
||||||
|
lastFocused: {
|
||||||
|
type: "popup",
|
||||||
|
left: 440,
|
||||||
|
top: 0,
|
||||||
|
width: 1280,
|
||||||
|
height: 720,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
bg.requestSign();
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
// Centred, it would be at left 900, the position the browser refused.
|
||||||
|
expect(bg.created).toHaveLength(1);
|
||||||
|
expect(bg.created[0].left).toBeUndefined();
|
||||||
|
expect(bg.created[0].top).toBeUndefined();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
+59
-1
@@ -2676,7 +2676,9 @@ function dappMessages(page, type) {
|
|||||||
|
|
||||||
// The approval window the background opened. Approvals are raised from an
|
// The approval window the background opened. Approvals are raised from an
|
||||||
// RPC call rather than from a user gesture, so the extension opens a real
|
// RPC call rather than from a user gesture, so the extension opens a real
|
||||||
// popup window for them; it is an ordinary page in this context.
|
// popup window for them; it is an ordinary page in this context. It is the
|
||||||
|
// first one found: the runner closes every approval window between tests, so
|
||||||
|
// within a test it can only be this test's.
|
||||||
async function waitForApprovalWindow(ctx, timeout = 30000) {
|
async function waitForApprovalWindow(ctx, timeout = 30000) {
|
||||||
const deadline = Date.now() + timeout;
|
const deadline = Date.now() + timeout;
|
||||||
for (;;) {
|
for (;;) {
|
||||||
@@ -3633,6 +3635,55 @@ test("eth_sendTransaction rejected broadcasts nothing (#183)", async (env) => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// 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
|
||||||
|
// (https://git.eeqj.de/sneak/AutistMask/issues/290).
|
||||||
|
test("a prompt raised while another approval window has focus opens its own (#290)", async (env) => {
|
||||||
|
await startRequest(env.dapp, "focus-sign", "personal_sign", [
|
||||||
|
SIGN_HEX,
|
||||||
|
env.expectedAddress,
|
||||||
|
]);
|
||||||
|
const signWindow = await waitForApprovalWindow(env.ctx);
|
||||||
|
await visible(signWindow, "#view-approve-sign");
|
||||||
|
await signWindow.bringToFront();
|
||||||
|
const focused = await env.page.evaluate(
|
||||||
|
() =>
|
||||||
|
new Promise((resolve) => {
|
||||||
|
chrome.windows.getLastFocused((w) => resolve(w.type));
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
assert(
|
||||||
|
focused === "popup",
|
||||||
|
"the sign window does not have focus, so this proves nothing: the " +
|
||||||
|
"last focused window is a " +
|
||||||
|
focused,
|
||||||
|
);
|
||||||
|
|
||||||
|
const opened = env.ctx.waitForEvent("page");
|
||||||
|
await startRequest(env.dapp, "focus-tx", "eth_sendTransaction", [
|
||||||
|
{
|
||||||
|
from: env.expectedAddress,
|
||||||
|
to: STUB_COUNTERPARTY,
|
||||||
|
value: toQuantity(TX_VALUE_WEI),
|
||||||
|
data: TX_DATA,
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
const txWindow = await opened.catch(async () => {
|
||||||
|
const outcome = await settleRequest(env.dapp, "focus-tx", 1000);
|
||||||
|
throw new Error(
|
||||||
|
"the extension opened no window for the transaction: " +
|
||||||
|
JSON.stringify(outcome),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
await visible(txWindow, "#view-approve-tx");
|
||||||
|
|
||||||
|
// Closing a window is refusing its prompt, so both answer 4001.
|
||||||
|
await closeApprovalPages(env.ctx);
|
||||||
|
await assertUserRejection(env.dapp, "focus-tx", "the transaction prompt");
|
||||||
|
await assertUserRejection(env.dapp, "focus-sign", "the sign prompt");
|
||||||
|
});
|
||||||
|
|
||||||
// The closing pass over both boundaries at once. Every message the section
|
// The closing pass over both boundaries at once. Every message the section
|
||||||
// put on either channel is re-read here and required to be free of the
|
// put on either channel is re-read here and required to be free of the
|
||||||
// password — and required to be there at all, method by method, so the
|
// password — and required to be there at all, method by method, so the
|
||||||
@@ -4002,6 +4053,13 @@ async function main() {
|
|||||||
failure = e.message;
|
failure = e.message;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// No test starts with an approval window open. One that closes itself,
|
||||||
|
// after a signature or on Reject, can still be closing when the next
|
||||||
|
// test raises its prompt, and waitForApprovalWindow() would take it
|
||||||
|
// for the new one; one a failed test left unanswered would refuse the
|
||||||
|
// next prompt from its site.
|
||||||
|
await closeApprovalPages(env.ctx);
|
||||||
|
|
||||||
// Any uncaught page error, console.error or unstubbed request
|
// Any uncaught page error, console.error or unstubbed request
|
||||||
// fails the test that provoked it, whether or not its assertions
|
// fails the test that provoked it, whether or not its assertions
|
||||||
// passed. This is the mechanism that caught #150.
|
// passed. This is the mechanism that caught #150.
|
||||||
|
|||||||
Reference in New Issue
Block a user