fix: settle a site approval on the port that carries its teardown (closes #275)
All checks were successful
check / check (push) Successful in 37s
e2e / e2e-chrome (push) Successful in 49s
e2e / e2e-firefox (push) Successful in 25s

Approve and window.close() left the popup on the next line, and the decision
and the disconnect the close caused travelled independent channels with nothing
ordering them. The disconnect handler settled a pending site approval as a
rejection, so whichever landed first decided the outcome. Driven in a tab the
teardown won every time: the user allowed the connection and the dApp was told
they had refused.

The decision now goes out on the approval port the popup already opens, which
is the same port the close disconnects. One channel is ordered -- a message
posted on a port is delivered before that port's own disconnect -- so the
approval is settled before the teardown is even seen, and the disconnect then
finds nothing pending to reject. Nothing waits, nothing is timed, and the popup
closes exactly as immediately as before.

windows.onRemoved no longer decides a site approval whose port is connected
either. In the fallback-window shape that event races the decision on a channel
of its own, which is the same defect one level over; the port disconnect says
the same thing in a defined order, so it is left to say it. A window that
closes before its popup ever connected has nothing else to speak for it and is
still rejected there, so no dApp is left waiting on a window that is gone.

Rejecting reports a rejection, and so does closing without deciding, in both
shapes. AUTISTMASK_APPROVAL_RESPONSE is gone; the port name carries the
approval id, so the popup no longer names one, and the sender check the message
carried moved to the port.

tests/backgroundApproval.test.js drives decide-then-disconnect with nothing
awaited in between, in the toolbar-popup shape that production uses and in the
fallback-window shape, and asserts every close-without-deciding path still
rejects. tests/e2e/run.js drops the deferred-window.close() accommodation it
carried for this bug, so the two site-prompt tests now drive the shipped
decide-then-close in a real Chromium.
This commit is contained in:
clawbot
2026-08-17 06:16:27 +00:00
committed by sneak
parent 7690fe6429
commit eb4b053f02
5 changed files with 509 additions and 71 deletions

View File

@@ -443,7 +443,7 @@ function showSignApproval(details) {
// describe the approval is the same outcome as an approval that is gone.
async function show(id) {
approvalId = id;
runtimeApi().connect({ name: "approval:" + id });
approvalPort = runtimeApi().connect({ name: "approval:" + id });
let details = null;
try {
@@ -476,6 +476,14 @@ async function show(id) {
}
let approvalId = null;
// The port this approval was opened on. Closing this window disconnects it,
// and the background treats that disconnect as "closed without deciding" for a
// site connection — so the decision goes out on this same port and not as a
// one-off message. One channel is ordered: a message posted on it is delivered
// before its own disconnect, however immediately the close follows. Two
// channels were not, and the close won, reporting a user who approved as
// having refused.
let approvalPort = null;
let pendingTxDetails = null;
// The exact objects shown to the user, kept so the popup signs what it
// displayed rather than re-fetching or re-populating anything at approval
@@ -543,6 +551,28 @@ function clearSignPassword() {
hideError("approve-sign-error");
}
// Answer a site-connection approval and close. The decision goes out on the
// approval port — see approvalPort above for why — and carries no approval id,
// because the port name already names the approval the background will settle.
// The post is guarded because a throw must not cost the close: posting on a
// port whose background worker has been torn down throws, and the approval it
// would have settled died with that worker, so the only thing left to do is
// what the user asked for — go away.
function decideSite(approved) {
if (approvalPort) {
try {
approvalPort.postMessage({
type: "AUTISTMASK_APPROVAL_DECISION",
approved,
remember: $("approve-remember").checked,
});
} catch {
// Nothing to report it to; the window closes either way.
}
}
window.close();
}
function init(_ctx) {
onViewLeave("approve-tx", clearTxPassword);
onViewLeave("approve-sign", clearSignPassword);
@@ -553,25 +583,11 @@ function init(_ctx) {
});
$("btn-approve").addEventListener("click", () => {
const remember = $("approve-remember").checked;
notify({
type: "AUTISTMASK_APPROVAL_RESPONSE",
id: approvalId,
approved: true,
remember,
});
window.close();
decideSite(true);
});
$("btn-reject").addEventListener("click", () => {
const remember = $("approve-remember").checked;
notify({
type: "AUTISTMASK_APPROVAL_RESPONSE",
id: approvalId,
approved: false,
remember,
});
window.close();
decideSite(false);
});
$("btn-approve-tx").addEventListener("click", async () => {