fix: a page with an opaque origin is refused, not treated as a site (closes #527)

A page with an opaque origin, such as one its server sandboxes with a
Content-Security-Policy, is reported to the background with the origin
"null". The background took that as a site, so every such page on every
website was one site: a connection remembered for one covered them all,
and the phishing check never matched. A request whose origin is "null" is
now refused with 4100, like one with no origin, before any method runs,
and is not credited to the site its URL names. tests/rpcOrigin.test.js
sends one, and README's SiteApproval says so.

Model: opus-5-5

README.md

@@ -2080,9 +2080,12 @@ view would leave a wallet one click from deletion.
   `wallet_requestPermissions` and is on neither the allowed nor the denied list.
   A site is its full origin, `scheme://host[:port]`, on both lists and for a
   connection allowed without "Remember": a choice for `https://dapp.example`
-  says nothing about `http://dapp.example` or another port of that host. The
-  background script prefers the toolbar popup (`action.openPopup()`) and falls
-  back to a separate popup window (`src/background/index.js`,
+  says nothing about `http://dapp.example` or another port of that host. A page
+  with an opaque origin, such as one its server sandboxes with a
+  Content-Security-Policy, is reported with the origin `null`, which is no site:
+  every request from it is refused with EIP-1193 code `4100` and opens no
+  prompt. The background script prefers the toolbar popup (`action.openPopup()`)
+  and falls back to a separate popup window (`src/background/index.js`,
   `requestApproval()`). Only one exists per site at a time: a further connection
   request from a site whose prompt is still unanswered is refused with EIP-1193
   code `-32002` and opens no new prompt. If that prompt was in a toolbar popup
@@ -2221,13 +2224,6 @@ view would leave a wallet one click from deletion.
     - Typed data that states no primary type, or one other than the type it
       would be signed as, or that cannot be read → shown with the error line
       saying so and "Sign" disabled; only "Reject" remains
-    - Typed data whose message leaves out a field its type declares, at any
-      level of nesting, or gives a `bool` field a value other than `true` or
-      `false` → shown with the error line naming the field and "Sign" disabled;
-      only "Reject" remains. ethers signs a `bool` by whether its value is
-      truthy, so the string `"false"` would be signed as true and a field left
-      out as false. The background refuses such a signature too and answers the
-      site with the same sentence
     - Typed data whose domain `chainId` names a chain other than the active
       network's → shown with the error line naming both chain ids and "Sign"
       disabled; only "Reject" remains. EIP-712 asks for this, because the

src/background/index.js

@@ -22,8 +22,6 @@ const { debugFetch, log } = require("../shared/log");
 const {
     verifySignedTx,
     verifySignature,
-    signedPrimaryType,
-    typedDataMessageRefusal,
     typedDataChainRefusal,
     failureIsRetryable,
     describeTxFailure,
@@ -1430,6 +1428,11 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
         // the origin of sender.url is used: the frame that sent the message,
         // not the tab's page, which may be another site embedding that
         // frame. With neither, the request is refused.
+        //
+        // A page with an opaque origin, such as one its server sandboxes with
+        // a Content-Security-Policy, is reported with the origin "null". That
+        // is no site, so the request is refused too, and not credited to the
+        // site its URL names.
         let trustedOrigin = sender.origin;
         if (!trustedOrigin && sender.url) {
             try {
@@ -1438,7 +1441,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                 // an unparseable URL leaves the origin unknown
             }
         }
-        if (!trustedOrigin) {
+        if (!trustedOrigin || trustedOrigin === "null") {
             sendResponse({
                 error: {
                     code: 4100,
@@ -1789,27 +1792,18 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                     signature,
                     approval.approvedFrom,
                 );
-                // The sign screen refuses typed data whose message ethers
-                // would sign differently from how it is shown, or whose
-                // domain names another chain. A signature for it that arrives
-                // anyway is refused here, against the network active now.
+                // The sign screen refuses typed data whose domain names
+                // another chain. A signature for it that arrives anyway is
+                // refused here, against the network active now.
                 const sp = approval.signParams;
                 if (
                     sp.method === "eth_signTypedData_v4" ||
                     sp.method === "eth_signTypedData"
                 ) {
-                    const data = JSON.parse(sp.typedData);
-                    const refusal =
-                        typedDataMessageRefusal(
-                            data.types,
-                            signedPrimaryType(data.types),
-                            data.message,
-                            "",
-                        ) ||
-                        typedDataChainRefusal(
-                            data.domain,
-                            networkById(s.networkId),
-                        );
+                    const refusal = typedDataChainRefusal(
+                        JSON.parse(sp.typedData).domain,
+                        networkById(s.networkId),
+                    );
                     if (refusal) throw new ApprovalMismatchError(refusal);
                 }
                 settleApproval(msg.id, { signature }, { holdsClaim: true });

src/popup/views/approval.js

@@ -27,6 +27,7 @@ const {
     Interface,
     MaxUint256,
     toUtf8String,
+    TypedDataEncoder,
 } = require("ethers");
 const { getPrice, formatUsd } = require("../../shared/prices");
 const { ERC20_ABI } = require("../../shared/constants");
@@ -47,8 +48,6 @@ const { getSignerForAddress } = require("../../shared/wallet");
 const { walletDefect } = require("../../shared/walletDefects");
 const {
     describeSigningFailure,
-    signedPrimaryType,
-    typedDataMessageRefusal,
     typedDataChainRefusal,
 } = require("../../shared/approvalVerify");
 const txStatus = require("./txStatus");
@@ -433,12 +432,22 @@ function markInvisibleCharacters(text) {
         );
 }
 
+// The type ethers will sign typed data as. ethers does not read the page's
+// `primaryType`: it takes the one struct in `types` that no other struct
+// refers to. Throws when the types name no such single struct, which ethers
+// would refuse to sign as well.
+function signedPrimaryType(types) {
+    const structs = { ...types };
+    // ethers derives EIP712Domain itself and rejects it as an input.
+    delete structs.EIP712Domain;
+    return TypedDataEncoder.getPrimaryType(structs);
+}
+
 // Why a signature request cannot be signed, as a sentence for the error line,
 // or null when it can. Only typed data is refused: the `primaryType` the page
 // states has to be the type ethers will sign, or this screen would name one
-// message while another is signed. Its message has to hold every declared
-// field, with each `bool` true or false, and its domain may name no chain but
-// the active network's.
+// message while another is signed. Its domain may name no chain but the active
+// network's.
 function typedDataRefusal(sp) {
     if (sp.method === "personal_sign" || sp.method === "eth_sign") return null;
     let data;
@@ -461,10 +470,7 @@ function typedDataRefusal(sp) {
             ", so it cannot be signed."
         );
     }
-    return (
-        typedDataMessageRefusal(data.types, signed, data.message, "") ||
-        typedDataChainRefusal(data.domain, currentNetwork())
-    );
+    return typedDataChainRefusal(data.domain, currentNetwork());
 }
 
 // The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats

src/shared/approvalVerify.js

@@ -56,7 +56,6 @@ const {
     getBigInt,
     getBytes,
     toQuantity,
-    TypedDataEncoder,
     verifyMessage,
     verifyTypedData,
 } = require("ethers");
@@ -637,63 +636,6 @@ function verifySignature(signParams, signature, expectedFrom) {
     return recovered;
 }
 
-// The type ethers will sign typed data as. ethers does not read the page's
-// `primaryType`: it takes the one struct in `types` that no other struct
-// refers to. Throws when the types name no such single struct, which ethers
-// would refuse to sign as well.
-function signedPrimaryType(types) {
-    const structs = { ...types };
-    // ethers derives EIP712Domain itself and rejects it as an input.
-    delete structs.EIP712Domain;
-    return TypedDataEncoder.getPrimaryType(structs);
-}
-
-// Why a typed data message cannot be signed as the screen shows it, as a
-// sentence, or null when it can. ethers signs a `bool` field by whether its
-// value is truthy, so the string "false" is signed as true and a field left
-// out as false. Each field that `type` declares, at every level of nesting,
-// must be one of the value's own keys, and each `bool` must be true or false.
-// Called with the type ethers signs, the message, and a `path` of "".
-function typedDataMessageRefusal(types, type, value, path) {
-    const itemType = type.replace(/\[\d*\]$/, "");
-    if (itemType !== type) {
-        // ethers refuses to sign a list field that is not a list.
-        if (!Array.isArray(value)) return null;
-        for (let i = 0; i < value.length; i++) {
-            const refusal = typedDataMessageRefusal(
-                types,
-                itemType,
-                value[i],
-                `${path}[${i}]`,
-            );
-            if (refusal) return refusal;
-        }
-        return null;
-    }
-    if (type === "bool" && value !== true && value !== false) {
-        return `The field "${path}" is not true or false, so this typed data cannot be signed.`;
-    }
-    for (const field of types[type] || []) {
-        const at = path ? `${path}.${field.name}` : field.name;
-        // An own key only: a plain lookup of `constructor` finds a function.
-        const present =
-            value !== null &&
-            typeof value === "object" &&
-            Object.prototype.hasOwnProperty.call(value, field.name);
-        if (!present) {
-            return `The field "${at}" is missing from the message, so this typed data cannot be signed.`;
-        }
-        const refusal = typedDataMessageRefusal(
-            types,
-            field.type,
-            value[field.name],
-            at,
-        );
-        if (refusal) return refusal;
-    }
-    return null;
-}
-
 // Why typed data cannot be signed while `network` is the active network, as a
 // sentence, or null when it can. EIP-712 has a wallet refuse typed data whose
 // domain names another chain, because the signature is valid on that chain.
@@ -882,8 +824,6 @@ function describeSigningFailure(response, fallbackMessage) {
 module.exports = {
     verifySignedTx,
     verifySignature,
-    signedPrimaryType,
-    typedDataMessageRefusal,
     typedDataChainRefusal,
     assertNoForbiddenFields,
     assertNothingUnchecked,

tests/backgroundApproval.test.js

@@ -384,13 +384,13 @@ function loadBackground(options) {
 
     // The same through eth_signTypedData_v4, whose params name the address
     // first and the typed data second.
-    function requestTypedData(typedData = TYPED_DATA) {
+    function requestTypedData() {
         let rpcResult = null;
         messageListener(
             {
                 type: "AUTISTMASK_RPC",
                 method: "eth_signTypedData_v4",
-                params: [signer.address, typedData],
+                params: [signer.address, TYPED_DATA],
             },
             { origin: ORIGIN },
             (r) => {
@@ -1494,8 +1494,8 @@ describe("what the approval is verified against", () => {
     // screen; a signature for it that reaches the background anyway is not
     // handed to the page (https://git.eeqj.de/sneak/AutistMask/issues/519).
     // TYPED_DATA names mainnet.
-    async function answerTypedData(bg, pending, typedData = TYPED_DATA) {
-        const { domain, types, message } = JSON.parse(typedData);
+    async function answerTypedData(bg, pending) {
+        const { domain, types, message } = JSON.parse(TYPED_DATA);
         delete types.EIP712Domain;
         const signature = await signer.signTypedData(domain, types, message);
         const answer = bg.send(
@@ -1539,42 +1539,6 @@ describe("what the approval is verified against", () => {
         expect(pending.result()).toEqual({ error: { message: refusal } });
     });
 
-    // ethers signs a `bool` field sent as the string "false" as true, and one
-    // left out as false. The sign screen refuses both; a signature for either
-    // that reaches the background anyway is not handed to the page
-    // (https://git.eeqj.de/sneak/AutistMask/issues/552).
-    test.each([
-        [
-            '"false"',
-            "false",
-            'The field "approved" is not true or false, so this typed data cannot be signed.',
-        ],
-        [
-            "left out",
-            undefined,
-            'The field "approved" is missing from the message, so this typed data cannot be signed.',
-        ],
-    ])(
-        "typed data with a bool %s is refused, not handed back",
-        async (_, approved, refusal) => {
-            const data = JSON.parse(TYPED_DATA);
-            data.types.Note.push({ name: "approved", type: "bool" });
-            data.message.approved = approved;
-            const typedData = JSON.stringify(data);
-            const bg = loadBackground();
-            const pending = bg.requestTypedData(typedData);
-            await settle();
-
-            const { answer } = await answerTypedData(bg, pending, typedData);
-
-            expect(answer.sendResponse).toHaveBeenCalledWith({
-                error: refusal,
-                retryable: false,
-            });
-            expect(pending.result()).toEqual({ error: { message: refusal } });
-        },
-    );
-
     test("a sign request from another address raises no approval at all", async () => {
         const bg = loadBackground();
         const pending = bg.requestSign(other.address);

tests/rpcOrigin.test.js

@@ -8,8 +8,14 @@
 // and a request the browser said nothing about was credited to whatever the
 // page wrote (https://git.eeqj.de/sneak/AutistMask/issues/407).
 //
-// Every sender here lacks sender.origin, as on old Firefox. The connection
-// check on eth_accounts is what shows which site a request was credited to.
+// A page with an opaque origin, such as one its server sandboxes with a
+// Content-Security-Policy, is reported with the origin "null". That names no
+// site, so its request is refused
+// (https://git.eeqj.de/sneak/AutistMask/issues/527).
+//
+// Every other sender here lacks sender.origin, as on old Firefox. The
+// connection check on eth_accounts is what shows which site a request was
+// credited to.
 
 const { makeStorageStub } = require("./support/storageStub");
 
@@ -144,3 +150,23 @@ describe("a request the browser does not say the sender of", () => {
         });
     });
 });
+
+describe("a request from a page with an opaque origin", () => {
+    test("is refused, whatever the page's URL says", async () => {
+        const accounts = loadBackground();
+
+        const result = await accounts({
+            origin: "null",
+            url: CONNECTED_ORIGIN + "/sandboxed.html",
+            tab: { url: CONNECTED_ORIGIN + "/sandboxed.html" },
+        });
+
+        expect(result).toEqual({
+            error: {
+                code: 4100,
+                message:
+                    "The wallet could not tell which site sent this request.",
+            },
+        });
+    });
+});

tests/typedDataPermit.test.js

@@ -28,7 +28,7 @@ globalThis.chrome = {
     storage: { local: { get: async () => ({}), set: async () => {} } },
 };
 
-const { getAddress, MaxUint256, TypedDataEncoder } = require("ethers");
+const { getAddress, MaxUint256 } = require("ethers");
 const { state } = require("../src/shared/state");
 const { decryptWithPassword } = require("../src/shared/vault");
 const approval = require("../src/popup/views/approval");
@@ -662,107 +662,3 @@ describe("the sign screen refuses typed data for another chain", () => {
         expect(node("btn-approve-sign").disabled).toBe(false);
     });
 });
-
-// ethers signs a `bool` field by whether its value is truthy in JavaScript:
-// the string "false" is signed as true, and a field left out as false. The
-// screen would show one value while another is signed, so such typed data is
-// refused, and so is any declared field left out of the message
-// (https://git.eeqj.de/sneak/AutistMask/issues/552). DAI's older permit
-// declares the `bool` field `allowed`.
-describe("the sign screen refuses a message ethers would sign differently", () => {
-    const NOT_A_BOOL =
-        'The field "allowed" is not true or false, so this typed data cannot be signed.';
-    const MISSING =
-        'The field "allowed" is missing from the message, so this typed data cannot be signed.';
-
-    function daiPermit(allowed) {
-        return { ...DAI_PERMIT, message: { ...DAI_PERMIT.message, allowed } };
-    }
-
-    const withoutAllowed = {
-        ...DAI_PERMIT,
-        message: { ...DAI_PERMIT.message },
-    };
-    delete withoutAllowed.message.allowed;
-
-    beforeEach(() => {
-        decryptWithPassword.mockClear();
-    });
-
-    test('ethers signs "false" as true, and a bool left out as false', () => {
-        const hash = (data) =>
-            TypedDataEncoder.hash(
-                data.domain,
-                { Permit: data.types.Permit },
-                data.message,
-            );
-        expect(hash(daiPermit("false"))).toBe(hash(daiPermit(true)));
-        expect(hash(withoutAllowed)).toBe(hash(daiPermit(false)));
-    });
-
-    test('a bool sent as the string "false" shows the error, with Sign disabled', async () => {
-        await openSignScreen(daiPermit("false"));
-        expect(node("approve-sign-error").textContent).toBe(NOT_A_BOOL);
-        expect(node("btn-approve-sign").disabled).toBe(true);
-    });
-
-    test("a bool left out shows the error, with Sign disabled", async () => {
-        await openSignScreen(withoutAllowed);
-        expect(node("approve-sign-error").textContent).toBe(MISSING);
-        expect(node("btn-approve-sign").disabled).toBe(true);
-    });
-
-    test("Sign clicked anyway decrypts and signs nothing", async () => {
-        const sent = await openSignScreen(daiPermit("false"));
-        node("approve-sign-password").value = "any password";
-        await click("btn-approve-sign");
-
-        expect(decryptWithPassword).not.toHaveBeenCalled();
-        expect(sent.map((msg) => msg.type)).not.toContain(
-            "AUTISTMASK_SIGN_RESPONSE",
-        );
-        expect(node("approve-sign-error").textContent).toBe(NOT_A_BOOL);
-    });
-
-    test("a well-formed message leaves Sign enabled", async () => {
-        for (const allowed of [true, false]) {
-            await openSignScreen(daiPermit(allowed));
-            expect(node("approve-sign-error").textContent).toBe("");
-            expect(node("btn-approve-sign").disabled).toBe(false);
-        }
-    });
-
-    test("a field left out inside a list item is named by its place", () => {
-        const [first, second] = PERMIT_BATCH.message.details;
-        const withoutNonce = { ...second };
-        delete withoutNonce.nonce;
-        const data = {
-            ...PERMIT_BATCH,
-            message: {
-                ...PERMIT_BATCH.message,
-                details: [first, withoutNonce],
-            },
-        };
-        expect(typedDataRefusal(request(data))).toBe(
-            'The field "details[1].nonce" is missing from the message, so this typed data cannot be signed.',
-        );
-    });
-
-    // A plain lookup of a field named after a built-in object property reads
-    // back a function, which ethers would sign as true.
-    test("a field named after a built-in object property is missing when left out", () => {
-        const data = {
-            ...LOGIN,
-            types: {
-                ...LOGIN.types,
-                Login: [
-                    ...LOGIN.types.Login,
-                    { name: "constructor", type: "bool" },
-                ],
-            },
-        };
-        expect(typedDataRefusal(request(data))).toBe(
-            'The field "constructor" is missing from the message, so this typed data cannot be signed.',
-        );
-    });
-});