fix: settle a site approval on the port that carries its teardown (closes #275)

tests/backgroundApproval.test.js

@@ -39,6 +39,21 @@ const HOSTNAME = "dapp.example";
 const UNCONNECTED_ORIGIN = "https://stranger.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 = {
@@ -173,8 +188,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: {
@@ -193,7 +213,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: {
@@ -221,7 +248,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");
@@ -289,6 +326,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) {
@@ -299,6 +400,8 @@ function loadBackground(options) {
         send,
         requestTx,
         requestSign,
+        requestSite,
+        connectApproval,
         closeWindow,
         broadcastTransaction,
         loadState,
@@ -1618,3 +1721,197 @@ 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.
+    //
+    // The event goes FIRST here, which is the interleaving the guard in the
+    // onRemoved listener exists for: the approval is still pending when the
+    // event arrives, so the listener really reaches it and really has to
+    // decline it. With the decision first there is nothing left in
+    // pendingApprovals and the listener finds no approval to spare.
+    test("approving in the fallback window survives a window event that lands first", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSite();
+        await settle();
+        expect(bg.created).toHaveLength(1);
+
+        const port = bg.connectApproval(pending.id());
+        bg.closeWindow(1);
+        port.decide(true, false);
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({ result: [signer.address] });
+    });
+
+    test("approving in the fallback window survives a window event that follows", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSite();
+        await settle();
+
+        const port = bg.connectApproval(pending.id());
+        port.decide(true, false);
+        bg.closeWindow(1);
+        port.disconnect();
+        await settle();
+
+        expect(pending.result()).toEqual({ result: [signer.address] });
+    });
+
+    // The connected port is what silences the window event, so connecting one
+    // must take the same sender check the decision takes. Otherwise a content
+    // script that guessed the id switches off the only settlement path a
+    // prompt whose real popup never connected has, and the dApp hangs.
+    test("a port from a page sender does not silence the window event", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSite();
+        await settle();
+
+        // Connected and held open — no disconnect, so nothing but the window
+        // event can settle this approval.
+        bg.connectApproval(pending.id(), FRESH_ORIGIN + "/x.html");
+        bg.closeWindow(1);
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+
+    // 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." },
+        });
+    });
+
+    // The `isSite &&` half of that skip, which is what keeps it from reaching
+    // a tx or sign approval. The popup connects its port in show() before it
+    // knows the approval's type, and the background sets portConnected without
+    // looking at the type either, so a tx approval in the fallback window
+    // carries the flag too. Without the conjunct the window event would skip
+    // it, windowClosed would never be set, releaseApproval() would never settle
+    // it, and the page would hang — the #271 regression this guard is written
+    // around.
+    test("a tx window closed with the port connected still rejects", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+        bg.connectApproval(pending.id());
+        await settle();
+        bg.closeWindow(1);
+        await settle();
+        expect(pending.result()).toEqual({
+            error: { code: 4001, message: "User rejected the request." },
+        });
+    });
+});

tests/e2e/run.js

@@ -2183,32 +2183,13 @@ async function reserveApprovalTab(env) {
     // one down with it.
     env.approvalTab = await env.ctx.newPage();
 
-    // The one accommodation this section makes to the shipped code, and the
-    // reason for it.
-    //
-    // Both approval buttons call runtime.sendMessage() and then window.close()
-    // on the next line. Closing this page disconnects the approval port, and
-    // the disconnect handler in src/background/index.js settles a pending
-    // site approval as a rejection. In a tab those two race and the teardown
-    // wins: the approve message is never acted on, and the page is told the
-    // user rejected. Measured — with the close left in place the approval
-    // resolves as a rejection every time; with it deferred it resolves as an
-    // approval every time.
-    //
-    // It is deferred, not removed: the harness closes the page itself once
-    // the outcome has been observed, which is what window.close() would have
-    // done, only after the message it was racing has been processed.
-    //
-    // This affects the site-connection prompt only. The sign and transaction
-    // prompts run in windows the extension opens itself, with window.close()
-    // untouched, and their disconnect handler deliberately keeps a tx or sign
-    // approval pending rather than rejecting it — so there is no race there
-    // to accommodate. Whether the same ordering holds in a real toolbar popup
-    // is not observable from a headless harness and is reported rather than
-    // assumed either way.
-    await env.approvalTab.addInitScript(() => {
-        window.close = function () {};
-    });
+    // This tab runs the shipped popup with nothing patched. The site
+    // approval buttons decide and then close on the next line, and the two
+    // site-approval tests below are therefore the real-browser
+    // approve-then-immediate-close and reject-then-immediate-close cases: the
+    // decision rides the approval port, which also carries the disconnect the
+    // close causes, so it is delivered ahead of it and the outcome does not
+    // depend on the teardown timing (#275).
     await env.approvalTab.goto("about:blank");
     await sleep(APPROVAL_TAB_SETTLE_MS);
     return env.approvalTab;
@@ -2257,6 +2238,95 @@ async function closeApprovalPages(ctx) {
     }
 }
 
+// Click a button whose own handler closes the window it lives in — every
+// Reject, and Allow on the site prompt.
+//
+// page.click() dispatches the click and then waits for the renderer to
+// acknowledge it, and a page torn down by the handler never gets to. The
+// dispatch is what the test needs and the log shows it happening ("performing
+// click action") immediately before the failure; the page going away is the
+// button working, not the click failing. Observed on #btn-reject-sign and
+// #btn-reject-tx, whose windows have always closed themselves.
+//
+// What the swallow costs is not the same for every button, so neither is what
+// proves the click landed:
+//
+//   #btn-reject-sign, #btn-reject-tx — their disconnect leaves the approval
+//     pending, so a click that never landed leaves the dApp promise unsettled
+//     and the assertion after the call fails on its own.
+//   #btn-approve — only a decision resolves the promise, and a swallowed click
+//     cannot produce settled === "resolved".
+//   #btn-reject on the site prompt — NOT self-proving. A page that went away
+//     without the click landing disconnects the approval port, the background
+//     settles that as 4001, and 4001 is exactly what assertUserRejection
+//     accepts. That call site arms the click trace below and asserts it.
+//
+// A button that is missing or unclickable raises a different error, which is
+// rethrown.
+async function clickAndClose(page, selector) {
+    try {
+        await page.click(selector);
+    } catch (e) {
+        if (!String((e && e.message) || e).includes("has been closed")) throw e;
+    }
+}
+
+// Evidence that a click reached the button, for the button whose outcome
+// cannot tell.
+//
+// A capture-phase listener on the document runs ahead of the button's own
+// handler and writes one key with localStorage.setItem(), which is synchronous
+// and therefore already in the browser process when the handler tears the page
+// down a line later. Any other page of the extension origin can read it back,
+// and env.page is one. The listener only observes: nothing about the shipped
+// decide-then-close is deferred, patched or reordered.
+const CLICK_TRACE_KEY = "autistmask-e2e-click-landed";
+
+async function armClickTrace(env, page, selector) {
+    await env.page.evaluate(
+        (key) => localStorage.removeItem(key),
+        CLICK_TRACE_KEY,
+    );
+    await page.evaluate(
+        ({ key, sel }) => {
+            document.addEventListener(
+                "click",
+                (e) => {
+                    const target = e.target;
+                    if (target && target.closest && target.closest(sel)) {
+                        localStorage.setItem(key, sel);
+                    }
+                },
+                true,
+            );
+        },
+        { key: CLICK_TRACE_KEY, sel: selector },
+    );
+}
+
+// The write crosses processes to reach env.page's renderer, so it is waited
+// for rather than read once. Nothing else in the test is timed on this.
+async function assertClickLanded(env, selector, timeout = 5000) {
+    const deadline = Date.now() + timeout;
+    let seen;
+    for (;;) {
+        seen = await env.page.evaluate(
+            (key) => localStorage.getItem(key),
+            CLICK_TRACE_KEY,
+        );
+        if (seen === selector || Date.now() > deadline) break;
+        await sleep(25);
+    }
+    assert(
+        seen === selector,
+        "the click on " +
+            selector +
+            " never reached the button, so the outcome below proves nothing " +
+            "about it: trace was " +
+            JSON.stringify(seen),
+    );
+}
+
 // Record every message the approval window sends to the background worker.
 //
 // This is the direct observation the password check needs. It is installed
@@ -2477,7 +2547,11 @@ test("eth_requestAccounts rejected at the prompt returns a rejection (#183)", as
         // origin in deniedSites and every later test in this section is
         // auto-rejected with no prompt at all, which would look like a pass.
         await popup.uncheck("#approve-remember");
-        await popup.click("#btn-reject");
+        // The rejection this asserts is also what an unclicked prompt that
+        // simply went away produces, so the click itself is witnessed.
+        await armClickTrace(env, popup, "#btn-reject");
+        await clickAndClose(popup, "#btn-reject");
+        await assertClickLanded(env, "#btn-reject");
 
         await assertUserRejection(
             env.dapp,
@@ -2508,7 +2582,7 @@ test("eth_requestAccounts approved returns the selected address (#183)", async (
         // does not, and the sign and transaction tests below all require the
         // origin to still be authorized.
         await popup.check("#approve-remember");
-        await popup.click("#btn-approve");
+        await clickAndClose(popup, "#btn-approve");
 
         outcome = await settleRequest(env.dapp, "accounts");
     } finally {
@@ -2616,7 +2690,7 @@ test("personal_sign rejected returns a rejection to the page (#183)", async (env
     ]);
     const popup = await waitForApprovalWindow(env.ctx);
     await visible(popup, "#view-approve-sign");
-    await popup.click("#btn-reject-sign");
+    await clickAndClose(popup, "#btn-reject-sign");
 
     await assertUserRejection(
         env.dapp,
@@ -2719,7 +2793,7 @@ test("eth_signTypedData_v4 rejected returns a rejection to the page (#183)", asy
     ]);
     const popup = await waitForApprovalWindow(env.ctx);
     await visible(popup, "#view-approve-sign");
-    await popup.click("#btn-reject-sign");
+    await clickAndClose(popup, "#btn-reject-sign");
 
     await assertUserRejection(
         env.dapp,
@@ -2877,7 +2951,7 @@ test("eth_sendTransaction rejected broadcasts nothing (#183)", async (env) => {
     ]);
     const popup = await waitForApprovalWindow(env.ctx);
     await visible(popup, "#view-approve-tx");
-    await popup.click("#btn-reject-tx");
+    await clickAndClose(popup, "#btn-reject-tx");
 
     await assertUserRejection(
         env.dapp,