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.

tests/backgroundApproval.test.js

@@ -32,6 +32,21 @@ const ORIGIN = "https://dapp.example";
 const HOSTNAME = "dapp.example";
 const EXT_URL = "chrome-extension://autistmask/";
 
+// An origin the persisted state has never allowed, so asking to connect from
+// it raises a prompt rather than being answered from allowedSites.
+const FRESH_ORIGIN = "https://fresh.example";
+
+// The approval id in the most recent popup URL of a list, or null when none
+// of them carries one. Takes both shapes: the absolute URL windows.create()
+// is given and the extension-relative one action.setPopup() is given.
+function approvalIdIn(urls) {
+    for (let i = urls.length - 1; i >= 0; i--) {
+        if (!urls[i] || !urls[i].includes("?approval=")) continue;
+        return new URL(urls[i], EXT_URL).searchParams.get("approval");
+    }
+    return null;
+}
+
 // What the dApp asks for: no nonce, no gas, no fees. This is the shape that
 // makes a duplicate broadcast possible at all.
 const TX_PARAMS = {
@@ -146,8 +161,13 @@ function loadBackground(options) {
 
     let messageListener = null;
     let windowRemovedListener = null;
+    let connectListener = null;
     const created = [];
     const removed = [];
+    // Every URL the background put on the browser action. A site approval
+    // raised through action.openPopup() opens no window at all, so this is
+    // the only place its id appears.
+    const actionPopups = [];
 
     global.chrome = {
         storage: {
@@ -163,7 +183,14 @@ function loadBackground(options) {
                     messageListener = fn;
                 },
             },
-            onConnect: { addListener: () => {} },
+            // Captured, not swallowed: the approval port is what carries a
+            // site connection's decision and the popup teardown that races
+            // it, so a no-op stub here hides the whole subject of #275.
+            onConnect: {
+                addListener: (fn) => {
+                    connectListener = fn;
+                },
+            },
             lastError: null,
         },
         windows: {
@@ -189,7 +216,17 @@ function loadBackground(options) {
             query: (q, cb) => cb([]),
             sendMessage: () => {},
         },
-        action: { setPopup: () => {} },
+        action: {
+            setPopup: (o) => {
+                actionPopups.push(o.popup);
+            },
+            // The production route for a site connection. Present only when
+            // a test asks for it, because with it the prompt is the toolbar
+            // popup: no window is created, so windows.onRemoved can never
+            // fire for it and the port disconnect is the only close signal
+            // that exists.
+            ...(opts.actionPopup ? { openPopup: () => Promise.resolve() } : {}),
+        },
     };
 
     require("../src/background/index");
@@ -248,6 +285,70 @@ function loadBackground(options) {
         };
     }
 
+    // A dApp asking to connect. The origin defaults to one the persisted
+    // state has never allowed, so the request really does raise a prompt
+    // instead of being answered from allowedSites.
+    function requestSite(origin) {
+        let rpcResult = null;
+        messageListener(
+            {
+                type: "AUTISTMASK_RPC",
+                method: "eth_requestAccounts",
+                params: [],
+            },
+            { origin: origin || FRESH_ORIGIN },
+            (r) => {
+                rpcResult = r;
+            },
+        );
+        return {
+            // Wherever the prompt went: the toolbar popup URL when
+            // action.openPopup() carried it, the created window otherwise.
+            id: () =>
+                approvalIdIn(actionPopups) ||
+                approvalIdIn(created.map((c) => c.url)),
+            result: () => rpcResult,
+        };
+    }
+
+    // The popup's approval port, as the browser delivers it. Messages posted
+    // on a port and that port's disconnect travel one channel in FIFO order,
+    // which is exactly the property the fix rests on, so this stub delivers
+    // them in the order the caller emits them and never reorders them.
+    function connectApproval(id, senderUrl) {
+        const onMessage = [];
+        const onDisconnect = [];
+        const port = {
+            name: "approval:" + id,
+            sender: {
+                url:
+                    senderUrl === undefined
+                        ? EXT_URL + "src/popup/index.html?approval=" + id
+                        : senderUrl,
+            },
+            onMessage: { addListener: (fn) => onMessage.push(fn) },
+            onDisconnect: { addListener: (fn) => onDisconnect.push(fn) },
+        };
+        connectListener(port);
+        return {
+            decide: (approved, remember) => {
+                for (const fn of onMessage) {
+                    fn(
+                        {
+                            type: "AUTISTMASK_APPROVAL_DECISION",
+                            approved,
+                            remember: !!remember,
+                        },
+                        port,
+                    );
+                }
+            },
+            disconnect: () => {
+                for (const fn of onDisconnect) fn(port);
+            },
+        };
+    }
+
     // The user closes the approval popup. `created` is index-aligned with the
     // ids the window stub hands back, so window 1 is the first popup opened.
     function closeWindow(windowId) {
@@ -258,6 +359,8 @@ function loadBackground(options) {
         send,
         requestTx,
         requestSign,
+        requestSite,
+        connectApproval,
         closeWindow,
         broadcastTransaction,
         loadState,
@@ -1058,3 +1161,136 @@ describe("popup-only messages", () => {
         });
     });
 });
+
+// A site connection decided in a popup that closes on the next line.
+//
+// The decision and the teardown are two events the popup emits back to back,
+// and the background must not be able to reach different outcomes depending on
+// which of them it processes first. It cannot, because they are now one
+// channel: the decision is posted on the approval port that the close then
+// disconnects, so it is delivered first. Every test here therefore emits the
+// close IMMEDIATELY after the decision, with nothing awaited in between —
+// which is what the popup does, and what used to report a user who approved as
+// having refused (#275).
+describe("a site connection decided as the popup closes", () => {
+    // The production route: chrome.action.openPopup() put the prompt in the
+    // toolbar popup, which is not a window, so nothing but the port
+    // disconnect can tell the background this prompt is gone.
+    test("approving in the toolbar popup connects the site", async () => {
+        const bg = loadBackground({ actionPopup: true });
+        const pending = bg.requestSite();
+        await settle();
+        const id = pending.id();
+        expect(id).toBeTruthy();
+        expect(bg.created).toHaveLength(0);
+
+        const port = bg.connectApproval(id);
+        port.decide(true, false);
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({ result: [signer.address] });
+    });
+
+    test("closing the toolbar popup without deciding is a rejection", async () => {
+        const bg = loadBackground({ actionPopup: true });
+        const pending = bg.requestSite();
+        await settle();
+
+        const port = bg.connectApproval(pending.id());
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+
+    test("rejecting is a rejection, and the close that follows adds nothing", async () => {
+        const bg = loadBackground({ actionPopup: true });
+        const pending = bg.requestSite();
+        await settle();
+
+        const port = bg.connectApproval(pending.id());
+        port.decide(false, false);
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+
+    // 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
+    // approval id must not be able to connect the site it is running on.
+    test("a decision from a page sender is ignored, and the close rejects", async () => {
+        const bg = loadBackground({ actionPopup: true });
+        const pending = bg.requestSite();
+        await settle();
+
+        const port = bg.connectApproval(pending.id(), FRESH_ORIGIN + "/x.html");
+        port.decide(true, true);
+        await settle();
+        expect(pending.result()).toBeNull();
+
+        port.disconnect();
+        await settle();
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+
+    // The fallback shape, where openPopup() is unavailable and the prompt is
+    // a window the extension opened. Closing it fires windows.onRemoved as
+    // well, on a channel of its own that is ordered against nothing — so the
+    // window event must not be allowed to decide a site approval either.
+    test("approving in the fallback window survives the window event too", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSite();
+        await settle();
+        expect(bg.created).toHaveLength(1);
+
+        const port = bg.connectApproval(pending.id());
+        port.decide(true, false);
+        bg.closeWindow(1);
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({ result: [signer.address] });
+    });
+
+    // Same shape, and the same window event arriving before the popup has
+    // said anything at all — which is a user closing the window rather than
+    // deciding, and still has to reach the dApp as a rejection.
+    test("closing the fallback window without deciding is a rejection", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSite();
+        await settle();
+
+        const port = bg.connectApproval(pending.id());
+        bg.closeWindow(1);
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+
+    // The net under the paragraph above: a prompt whose page never got as far
+    // as connecting the port has no disconnect to reject it, so the window
+    // event has to. Otherwise the dApp waits forever on a window that is gone.
+    test("a window that closes before its popup ever connected still rejects", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSite();
+        await settle();
+
+        bg.closeWindow(1);
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+});