fix: closing a transaction or signature approval window leaves the request pending (closes #673)

The browser can close the approval window when the user clicks away from it,
for example to copy the password from a password manager. Closing it used to
reject the request with 4001. Now the request stays pending and the site gets
no answer. Until it is answered, the toolbar popup opens the oldest such
approval. The toolbar popup skips an approval while its transaction is being
sent or its signature checked. A worker that starts holds no approvals, so it
points the toolbar popup back at the wallet.

Reject now closes its window only after the background has the rejection. A
rejection the background refuses leaves the window open with the background's
message.

Site-connection and network-switch windows still reject the request when
closed.

Model: opus-5-5

README.md

@@ -1505,11 +1505,12 @@ wrote last, the record says which build's shape it is in.
 Each write reads the record, changes it and writes it back, holding a lock from
 the read to the write: the browser's own (`navigator.locks`), taken through
 `withLock()` in `src/shared/browserApi.js`. Every page of the extension and its
-background share it, except a page in a Firefox private window, which Firefox
-gives locks of its own (see https://git.eeqj.de/sneak/AutistMask/issues/646).
-The popup's `saveState()`, the background's `updateState()` and the
-StateRecovery reset all take it, so among the pages and the background that
-share it, no write lands between another write's read and its write, where it
+background share it, except a page in a Firefox private window, which has locks
+of its own. `manifest/firefox.json` sets `"incognito": "not_allowed"`, so such a
+page cannot read or write extension storage. In Chrome, a page opened from an
+incognito window still runs outside incognito and shares the lock. The popup's
+`saveState()`, the background's `updateState()` and the StateRecovery reset all
+take it, so no write lands between another write's read and its write, where it
 would be lost. Chrome has it from version 69, older than any Chrome that runs a
 Manifest V3 extension, in pages and in the service worker; Firefox from version
 96, older than the 115 `manifest/firefox.json` declares. Where it is missing, a
@@ -2654,6 +2655,8 @@ view would leave a wallet one click from deletion.
       Data Model) → full-sentence error on the error line saying why, nothing
       signed (no screen change)
     - "Reject" → closes popup (returns rejection to background)
+    - "Reject" after "Confirm", while the transaction is being sent → the
+      background's message on the error line, no screen change
     - Popup window closed without answering → the request stays pending, and the
       site gets no answer. The browser can close this window when the user
       clicks away from it, for example to copy the password from a password
@@ -2748,6 +2751,8 @@ view would leave a wallet one click from deletion.
     - "Sign" (wrong password, or a signing failure) → error line, no screen
       change
     - "Reject" → closes popup (returns rejection to background)
+    - "Reject" after "Sign", while the signature is being checked → the
+      background's message on the error line, no screen change
     - Popup window closed without answering → the request stays pending, as on
       TxApproval. Until the request is answered, the toolbar button opens this
       screen instead of the wallet.

docs/README.md

@@ -208,6 +208,9 @@ password.
 3. Click "Load Temporary Add-on".
 4. Select `dist/firefox/manifest.json`.
 
+AutistMask does not work in Firefox private windows. If Firefox is set to "Never
+remember history", every window is private, so AutistMask does not work at all.
+
 ## Getting Started
 
 ### Creating a New Wallet

manifest/firefox.json

@@ -4,6 +4,7 @@
     "version": "0.1.0",
     "description": "Minimal Ethereum wallet for Firefox",
     "permissions": ["storage", "activeTab", "alarms", "<all_urls>"],
+    "incognito": "not_allowed",
     "content_security_policy": "default-src 'self'; script-src 'self' 'wasm-unsafe-eval'; object-src 'self'; style-src 'self'; img-src 'self' data:; connect-src 'self' https: http:; frame-src 'none'; form-action 'none'; base-uri 'none'",
     "icons": {
         "16": "icons/icon16.png",

src/background/index.js

@@ -390,10 +390,11 @@ let toolbarPopupApprovalId = null;
 // Point the toolbar popup at the oldest pending approval whose window closed,
 // or at the wallet when there is none. The user can then answer that approval
 // from the toolbar popup. pendingApprovals lists approvals in the order they
-// were added, so the first one found is the oldest.
+// were added, so the first one found is the oldest. An approval held by a
+// running attempt is skipped until that attempt ends.
 function updateToolbarPopup() {
     const closed = Object.values(pendingApprovals).find(
-        (approval) => approval.windowClosed,
+        (approval) => approval.windowClosed && !approval.attemptInFlight,
     );
     toolbarPopupApprovalId = closed ? closed.id : null;
     if (actionNs && typeof actionNs.setPopup === "function") {
@@ -478,6 +479,7 @@ const APPROVAL_WINDOW_FAILED_MESSAGE =
 function claimApproval(approval) {
     if (approval.attemptInFlight) return false;
     approval.attemptInFlight = true;
+    updateToolbarPopup();
     return true;
 }
 
@@ -486,6 +488,7 @@ function claimApproval(approval) {
 // its window while the attempt ran, the toolbar popup opens it for that retry.
 function releaseApproval(approval) {
     approval.attemptInFlight = false;
+    updateToolbarPopup();
 }
 
 // Open approval in a separate popup window, unless it is no longer pending.
@@ -1488,9 +1491,9 @@ updateToolbarPopup();
 // A transaction or signature approval stays pending when its window closes.
 // The browser can close that window when the user clicks away from it, for
 // example to copy the password from a password manager. The approval is marked
-// as closed, and from then on the toolbar popup opens it (see
-// updateToolbarPopup()). An attempt already running when the window closes
-// goes on and reports its outcome to the page.
+// as closed, so the toolbar popup can open it (see updateToolbarPopup()). An
+// attempt already running when the window closes goes on and reports its
+// outcome to the page.
 //
 // This listener rejects a site-connection or network-switch approval whose
 // window closes before its popup connected its port. Once the port is

src/background/state.js

@@ -18,11 +18,10 @@
 //   updateState(fn)   read fresh, apply fn to that fresh record, write it
 //                     back — all holding the lock every write of the stored
 //                     record takes, so no other write lands in between, by
-//                     the background or by any page except a page in a
-//                     Firefox private window (README, "Stored state and its
-//                     version"). The read is one storage round trip ahead of
-//                     the write rather than a page lifetime ahead of it
-//                     (which is what made the popup's saveState() need a
+//                     the background or by any page (README, "Stored state
+//                     and its version"). The read is one storage round trip
+//                     ahead of the write rather than a page lifetime ahead of
+//                     it (which is what made the popup's saveState() need a
 //                     per-field merge against a baseline at all).
 //
 // A handler that must both read and write therefore does its network work
@@ -73,8 +72,7 @@ async function updateStateOnce(mutate) {
 // src/shared/browserApi.js) is held from the read to the write: two
 // background turns, or a turn and a page's save, never interleave
 // (https://git.eeqj.de/sneak/AutistMask/issues/641). Every page's save waits
-// while it is held, except a page in a Firefox private window, which Firefox
-// gives locks of its own (README, "Stored state and its version").
+// while it is held (README, "Stored state and its version").
 //
 // `mutate` must also not call updateState() itself, directly or through
 // anything it awaits: the inner turn waits for the lock the outer one holds,

src/popup/views/approval.js

@@ -1144,13 +1144,19 @@ function init(_ctx) {
 
     // The window closes only once the background has the rejection. A message
     // sent as the window closes can be lost, and a closed window does not
-    // reject the request.
+    // reject the request. After Confirm, the background refuses a rejection
+    // while the transaction is being sent. The window then stays open and
+    // shows the background's message.
     $("btn-reject-tx").addEventListener("click", async () => {
-        await sendMessage({
+        const response = await sendMessage({
             type: "AUTISTMASK_TX_RESPONSE",
             id: approvalId,
             approved: false,
-        }).catch(() => {});
+        }).catch(() => null);
+        if (response && response.error) {
+            showError("approve-tx-error", response.error);
+            return;
+        }
         window.close();
     });
 
@@ -1273,13 +1279,18 @@ function init(_ctx) {
         if (outcome.retryable) setSignButtonBusy(false);
     });
 
-    // As for the transaction's Reject above.
+    // As for the transaction's Reject above. After Sign, the background
+    // refuses a rejection while the signature is being checked.
     $("btn-reject-sign").addEventListener("click", async () => {
-        await sendMessage({
+        const response = await sendMessage({
             type: "AUTISTMASK_SIGN_RESPONSE",
             id: approvalId,
             approved: false,
-        }).catch(() => {});
+        }).catch(() => null);
+        if (response && response.error) {
+            showError("approve-sign-error", response.error);
+            return;
+        }
         window.close();
     });
 }

src/popup/views/stateRecovery.js

@@ -143,8 +143,6 @@ async function resetProfile() {
     try {
         // Under the lock every write of the stored record takes, so the erase
         // cannot land between another write's read and its write and be undone.
-        // A page in a Firefox private window is the exception: Firefox gives
-        // it locks of its own (README, "Stored state and its version").
         await withLock("autistmask", () => storageRemove("autistmask"));
     } catch (e) {
         log.errorf("state recovery: reset failed:", e);

src/shared/state.js

@@ -581,9 +581,7 @@ function reportSaveFailure(err) {
 // Each save runs start to finish holding the lock every write of the stored
 // record takes (withLock() in src/shared/browserApi.js), so no write by
 // another page or the background lands between its read and its write and is
-// lost (https://git.eeqj.de/sneak/AutistMask/issues/641). A page in a Firefox
-// private window is the exception: Firefox gives it locks of its own (README,
-// "Stored state and its version").
+// lost (https://git.eeqj.de/sneak/AutistMask/issues/641).
 //
 // The same lock orders this page's own saves. showView() calls saveState() on
 // every navigation without awaiting it, so two saves from one page can be

tests/approvalWindow.test.js

@@ -176,17 +176,17 @@ test("a connection prompt in the toolbar popup leaves the browser window alone",
     expect(env.pageErrors).toEqual([]);
 });
 
-// A closed transaction or signature window leaves its request pending
-// (https://git.eeqj.de/sneak/AutistMask/issues/673). So Reject closes the
-// window only once the background has the rejection. A message sent as the
-// window closes can be lost, and the request would stay pending although the
-// user rejected it.
-test.each([
+// Each row holds a transaction or signature approval as the background
+// describes it, plus the element ids of the screen that shows it. `refusal` is
+// the message the background refuses a Reject with.
+const TX_AND_SIGN_APPROVALS = [
     {
         kind: "transaction",
         view: "approve-tx",
         reject: "btn-reject-tx",
+        error: "approve-tx-error",
         type: "AUTISTMASK_TX_RESPONSE",
+        refusal: "This transaction is already being sent.",
         approval: {
             type: "tx",
             approvedFrom: ADDRESS,
@@ -206,7 +206,9 @@ test.each([
         kind: "signature",
         view: "approve-sign",
         reject: "btn-reject-sign",
+        error: "approve-sign-error",
         type: "AUTISTMASK_SIGN_RESPONSE",
+        refusal: "This request is already being signed.",
         approval: {
             type: "sign",
             approvedFrom: ADDRESS,
@@ -217,7 +219,14 @@ test.each([
             },
         },
     },
-])(
+];
+
+// A closed transaction or signature window leaves its request pending
+// (https://git.eeqj.de/sneak/AutistMask/issues/673). So Reject closes the
+// window only once the background has the rejection. A message sent as the
+// window closes can be lost, and the request would stay pending although the
+// user rejected it.
+test.each(TX_AND_SIGN_APPROVALS)(
     "Reject on a $kind approval closes the window only once the background has answered",
     async ({ view, reject, type, approval }) => {
         let describeApproval = null;
@@ -259,3 +268,43 @@ test.each([
         expect(env.pageErrors).toEqual([]);
     },
 );
+
+// After Confirm or Sign, the background refuses a Reject while the transaction
+// is being sent or the signature checked. The request is not rejected then, so
+// the window stays open and shows the background's message.
+test.each(TX_AND_SIGN_APPROVALS)(
+    "Reject on a $kind approval that the background refuses shows the background's message and keeps the window open",
+    async ({ view, reject, error, type, refusal, approval }) => {
+        let describeApproval = null;
+        let answerReject = null;
+        const env = await bootPopup(unversionedValidProfile(), {
+            search: "?approval=approval-1",
+            runtime: {
+                connect: () => ({ postMessage: () => {} }),
+                sendMessage: (msg, reply) => {
+                    if (msg.type === "AUTISTMASK_GET_APPROVAL") {
+                        describeApproval = reply;
+                    }
+                    if (msg.type === type) answerReject = reply;
+                },
+            },
+        });
+        describeApproval({
+            ...approval,
+            origin: "https://dapp.example",
+            isPhishingDomain: false,
+        });
+        await settle();
+        const close = jest.fn();
+        globalThis.window.close = close;
+
+        const clicked = env.click(reject);
+        await settle();
+        answerReject({ error: refusal, retryable: false });
+        await clicked;
+        expect(close).not.toHaveBeenCalled();
+        expect(env.visibleViews()).toEqual([view]);
+        expect(env.text(error)).toBe(refusal);
+        expect(env.pageErrors).toEqual([]);
+    },
+);

tests/backgroundApproval.test.js

@@ -1989,6 +1989,90 @@ describe("a closed approval window leaves the request pending", () => {
         expect(toolbarPopup(bg)).toBe("src/popup/index.html");
     });
 
+    // The user can close the window while the attempt after Confirm runs. That
+    // attempt answers the request, so meanwhile the toolbar popup opens the
+    // wallet. Opened from the toolbar, the approval would look unanswered.
+    test("when the window closes during an attempt that sends, the toolbar popup keeps opening the wallet", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+
+        const inFlight = deferred();
+        bg.broadcastTransaction.mockReturnValue(inFlight.promise);
+        bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id: pending.id(),
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+        bg.closeWindow(1);
+        await settle();
+        expect(toolbarPopup(bg)).toBe("src/popup/index.html");
+
+        inFlight.resolve({ hash: "0xfeed" });
+        await settle();
+        expect(pending.result()).toEqual({ result: "0xfeed" });
+        expect(toolbarPopup(bg)).toBe("src/popup/index.html");
+    });
+
+    test("when the window closes during an attempt that fails retryably, the toolbar popup opens the wallet until the attempt ends, then the approval", async () => {
+        const stalled = deferred();
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+
+        bg.setStateReadHook(async () => {
+            await stalled.promise;
+            throw new Error("The wallet data could not be read.");
+        });
+        bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id: pending.id(),
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+        bg.closeWindow(1);
+        await settle();
+        expect(toolbarPopup(bg)).toBe("src/popup/index.html");
+
+        stalled.resolve();
+        await settle();
+        expect(pending.result()).toBeNull();
+        expect(toolbarPopup(bg)).toBe(
+            "src/popup/index.html?approval=" + pending.id(),
+        );
+
+        // A retry from the toolbar popup holds the approval again, so the
+        // toolbar popup opens the wallet while that retry runs.
+        bg.setStateReadHook(null);
+        const inFlight = deferred();
+        bg.broadcastTransaction.mockReturnValue(inFlight.promise);
+        bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id: pending.id(),
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+        expect(toolbarPopup(bg)).toBe("src/popup/index.html");
+
+        inFlight.resolve({ hash: "0xfeed" });
+        await settle();
+        expect(pending.result()).toEqual({ result: "0xfeed" });
+        expect(toolbarPopup(bg)).toBe("src/popup/index.html");
+    });
+
     test("an address switch still rejects a closed approval", async () => {
         const bg = loadBackground();
         const pending = bg.requestTx();

tests/manifest.test.js

@@ -165,6 +165,15 @@ describe("declared icons", () => {
     });
 });
 
+// Firefox gives a page in a private window locks of its own, so that page's
+// save could land between the background's read and its write (README,
+// "Stored state and its version").
+describe("private windows", () => {
+    test("a page in a firefox private window cannot read or write extension storage", () => {
+        expect(readManifest("firefox").incognito).toBe("not_allowed");
+    });
+});
+
 describe("shipped Content Security Policy", () => {
     // MV3 takes an object and applies extension_pages to the popup and the
     // background service worker, which is where libsodium runs.

tests/stateWriteLock.test.js

@@ -6,9 +6,8 @@
 // now holds a lock from its read to its write.
 //
 // The popup and the background run here as the real modules over one shared
-// storage stub, and share the lock the way the background and a page outside
-// a Firefox private window share the browser's
-// (tests/support/navigatorLocks.js).
+// storage stub, and share the lock the way the background and its pages share
+// the browser's (tests/support/navigatorLocks.js).
 
 const { makeStorageStub } = require("./support/storageStub");
 

tests/support/navigatorLocks.js

@@ -4,9 +4,8 @@
 // the callback settles, whether it resolved or rejected. That is what the
 // browser's own does for the one call the extension makes,
 // `navigator.locks.request(name, callback)`. Every module a test file loads
-// shares it, as the extension's background and its pages share the browser's,
-// except a page in a Firefox private window (README, "Stored state and its
-// version").
+// shares it, as the extension's background and its pages share the browser's
+// (README, "Stored state and its version").
 //
 // Run before each test file by `setupFiles` in package.json.