fix: open no approval window for a site-connection prompt already answered (closes #287)
When a site-connection prompt was decided before the toolbar popup raised for it had loaded, that popup was torn down, chrome.action.openPopup() rejected, and the background opened its fallback window for the answered approval and only then removed it. In the Chrome end-to-end suite the next test could take that window for its own prompt and lose it under its wait. openApprovalWindow() now returns before creating a window when the approval is no longer pending. The blocklist test clicked its self-closing Reject with a plain click; it now clicks it as the other site Reject does, with the click witnessed. README.md and the e2e workflow comment no longer name this issue as what keeps e2e-chrome from being a required check. Model: opus-5-5
This commit is contained in:
@@ -22,11 +22,10 @@ on: [push]
|
|||||||
# These jobs REPORT, they do not gate. Whether a check blocks a merge is
|
# These jobs REPORT, they do not gate. Whether a check blocks a merge is
|
||||||
# Gitea branch protection, which this repo does not configure, so a failure
|
# Gitea branch protection, which this repo does not configure, so a failure
|
||||||
# here is a red mark a reviewer has to account for rather than a hard
|
# here is a red mark a reviewer has to account for rather than a hard
|
||||||
# block. Making e2e-chrome a required check is blocked on the measured
|
# block. Making e2e-chrome a required check is blocked while reports of the
|
||||||
# flake in the dApp signing wait -- two of six runs of unmutated code on a
|
# Chrome suite failing under load are still open; the "In CI" section of
|
||||||
# loaded machine -- tracked as
|
# README.md names them. A gate that fails at random teaches people to merge
|
||||||
# https://git.eeqj.de/sneak/AutistMask/issues/287. A gate that fails at
|
# past red.
|
||||||
# random teaches people to merge past red.
|
|
||||||
#
|
#
|
||||||
# Nothing here may pass vacuously. There is no continue-on-error and no
|
# Nothing here may pass vacuously. There is no continue-on-error and no
|
||||||
# `|| true`. Both scripts exit non-zero when docker is missing, when the
|
# `|| true`. Both scripts exit non-zero when docker is missing, when the
|
||||||
|
|||||||
@@ -619,13 +619,14 @@ 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. The Chrome suite is
|
That is not only a statement about configuration. Reports of the Chrome suite
|
||||||
**measurably flaky under load** — two of six runs of unmutated code on a busy
|
**failing under load** are still open, among them
|
||||||
machine lost the approval popup out from under the dApp signing wait, always in
|
[#290](https://git.eeqj.de/sneak/AutistMask/issues/290), runs on a busy machine
|
||||||
the `#183` section, tracked as
|
failing with `the extension opened no approval window within 30000ms`, and
|
||||||
[#287](https://git.eeqj.de/sneak/AutistMask/issues/287). So a red `e2e-chrome`
|
[#446](https://git.eeqj.de/sneak/AutistMask/issues/446), the Settings round trip
|
||||||
has to be read before it is believed, and that flake is the blocker to ever
|
closing the popup before its network switch is saved. So a red `e2e-chrome` has
|
||||||
making this a required check. Do not answer it with a retry wrapper: a suite
|
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.
|
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
|
||||||
|
|||||||
@@ -45,6 +45,21 @@ but the review is broader than any of them.
|
|||||||
|
|
||||||
# Completed Steps
|
# Completed Steps
|
||||||
|
|
||||||
|
- 2026-10-05: The extension no longer opens a window for a site-connection
|
||||||
|
prompt already answered
|
||||||
|
([#287](https://git.eeqj.de/sneak/AutistMask/issues/287)). When the prompt was
|
||||||
|
decided before the toolbar popup raised for it had loaded, that popup was torn
|
||||||
|
down, `chrome.action.openPopup()` rejected, and the background opened its
|
||||||
|
fallback window for the answered approval and then removed it. In the Chrome
|
||||||
|
end-to-end suite the next test could take that window for its own prompt and
|
||||||
|
lose it under its wait. `openApprovalWindow()` now opens nothing for an
|
||||||
|
approval that is no longer pending. The blocklist test's Reject, whose window
|
||||||
|
closes itself, is clicked as the other site Reject is, with the click
|
||||||
|
witnessed. Making `e2e-chrome` a required check is still blocked: other
|
||||||
|
reports of the Chrome suite failing under load are open, among them
|
||||||
|
[#290](https://git.eeqj.de/sneak/AutistMask/issues/290) and
|
||||||
|
[#446](https://git.eeqj.de/sneak/AutistMask/issues/446), as `README.md` says.
|
||||||
|
|
||||||
- 2026-10-05: The lost-password delete confirmation refuses an empty field and
|
- 2026-10-05: The lost-password delete confirmation refuses an empty field and
|
||||||
ignores characters that paint nothing
|
ignores characters that paint nothing
|
||||||
([#336](https://git.eeqj.de/sneak/AutistMask/issues/336)). A wallet named only
|
([#336](https://git.eeqj.de/sneak/AutistMask/issues/336)). A wallet named only
|
||||||
|
|||||||
@@ -413,7 +413,7 @@ function releaseApproval(approval) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Open approval in a separate popup window.
|
// Open approval in a separate popup window, unless it is no longer pending.
|
||||||
// This is the primary mechanism for tx/sign approvals (triggered programmatically,
|
// This is the primary mechanism for tx/sign approvals (triggered programmatically,
|
||||||
// not from a user gesture) and the fallback for site-connection approvals.
|
// not from a user gesture) and the fallback for site-connection approvals.
|
||||||
// Never rejects. Its callers raise it from inside a Promise executor and drop
|
// Never rejects. Its callers raise it from inside a Promise executor and drop
|
||||||
@@ -446,6 +446,10 @@ async function openApprovalWindow(id) {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Already answered: a site-connection prompt decided before the toolbar
|
||||||
|
// popup raised for it had loaded, whose openPopup() rejects only now.
|
||||||
|
if (!pendingApprovals[id]) return;
|
||||||
|
|
||||||
let win = null;
|
let win = null;
|
||||||
try {
|
try {
|
||||||
win = await windowsCreate(opts);
|
win = await windowsCreate(opts);
|
||||||
|
|||||||
@@ -2235,6 +2235,27 @@ describe("a site connection decided as the popup closes", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// The prompt is decided before the toolbar popup raised for it has
|
||||||
|
// loaded; that popup is torn down and openPopup() rejects only after.
|
||||||
|
test("a toolbar prompt already decided opens no window when openPopup() rejects", async () => {
|
||||||
|
const bg = loadBackground({ actionPopup: true });
|
||||||
|
const opening = deferred();
|
||||||
|
bg.openPopup.mockImplementation(() => opening.promise);
|
||||||
|
const pending = bg.requestSite();
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
const port = bg.connectApproval(pending.id());
|
||||||
|
port.decide(true, false);
|
||||||
|
port.disconnect();
|
||||||
|
await settle();
|
||||||
|
expect(pending.result()).toEqual({ result: [signer.address] });
|
||||||
|
|
||||||
|
opening.reject(new Error("the toolbar popup closed before it loaded"));
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
expect(bg.created).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
// The port carries a decision now, so it carries the sender check the
|
// The port carries a decision now, so it carries the sender check the
|
||||||
// one-off message used to carry. A content script that guessed an
|
// one-off message used to carry. A content script that guessed an
|
||||||
// approval id must not be able to connect the site it is running on.
|
// approval id must not be able to connect the site it is running on.
|
||||||
|
|||||||
+4
-2
@@ -2738,7 +2738,7 @@ async function closeApprovalPages(ctx) {
|
|||||||
// #btn-reject on the site prompt — NOT self-proving. A page that went away
|
// #btn-reject on the site prompt — NOT self-proving. A page that went away
|
||||||
// without the click landing disconnects the approval port, the background
|
// without the click landing disconnects the approval port, the background
|
||||||
// settles that as 4001, and 4001 is exactly what assertUserRejection
|
// settles that as 4001, and 4001 is exactly what assertUserRejection
|
||||||
// accepts. That call site arms the click trace below and asserts it.
|
// accepts. Both call sites arm the click trace below and assert it.
|
||||||
//
|
//
|
||||||
// A button that is missing or unclickable raises a different error, which is
|
// A button that is missing or unclickable raises a different error, which is
|
||||||
// rethrown.
|
// rethrown.
|
||||||
@@ -3124,7 +3124,9 @@ test("a connect request from a blocklisted site is flagged (#219)", async (env)
|
|||||||
// Not remembered: a remembered decision for this origin would
|
// Not remembered: a remembered decision for this origin would
|
||||||
// outlive the test.
|
// outlive the test.
|
||||||
await popup.uncheck("#approve-remember");
|
await popup.uncheck("#approve-remember");
|
||||||
await popup.click("#btn-reject");
|
await armClickTrace(env, popup, "#btn-reject");
|
||||||
|
await clickAndClose(popup, "#btn-reject");
|
||||||
|
await assertClickLanded(env, "#btn-reject");
|
||||||
|
|
||||||
await assertUserRejection(
|
await assertUserRejection(
|
||||||
phishingDapp,
|
phishingDapp,
|
||||||
|
|||||||
Reference in New Issue
Block a user