fix: settle a site approval on the port that carries its teardown (closes #275)
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:
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user