fix: eth_sign's warning and the README say what it signs (closes #409)

eth_sign is signed as an EIP-191 personal message over its data, which is
what the Ethereum JSON-RPC specification defines it to return. Its warning
on the signature prompt said the site was asking to sign a raw hash that
could drain funds, and the README said the same; neither was true. The
warning now says the site used the older eth_sign method and that the data
will be signed as a personal message, and the README's SignApproval section
says so too. A test pins that eth_sign is read address first and data
second, and that the page receives the personal message signature over the
data.

Model: opus-5-5

README.md

@@ -2124,7 +2124,12 @@ view would leave a wallet one click from deletion.
       blocklist)
     - Site origin (bold, scheme and port included) + "wants you to sign a
       message"
-    - Danger warning box (shown for `eth_sign`, which signs a raw hash)
+    - Warning box, shown for `eth_sign`: "This site used the older eth_sign
+      method. The data will be signed as a personal message." An `eth_sign`
+      request names the address first and the data second. Its data is signed
+      exactly as a `personal_sign` message is, as an EIP-191 personal message,
+      which is what the Ethereum JSON-RPC specification defines `eth_sign` to
+      return.
     - Type: "Personal message" or "Typed data (EIP-712)"
     - From: color dot + full address + etherscan link
     - Message: for `personal_sign` and `eth_sign`, the text the message's bytes
@@ -2160,13 +2165,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 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
-      signature is valid on the chain the domain names. Typed data with no
-      `chainId` is not refused. The background refuses such a signature too,
-      against the network active when the signature reaches it, and answers the
-      site with the same sentence
     - A `personal_sign` or `eth_sign` message that is not hex (`0x` or `0X` and
       an even number of hex digits, the form ethers' `getBytes` reads when
       signing) → shown as plain text, with the error line "This message is plain
@@ -2659,8 +2657,7 @@ plaintext list of anyone's domain names.
 When a dApp on a blocklisted domain requests a wallet connection, transaction
 approval, or signature, the approval popup displays a prominent red warning
 banner alerting the user. The domain checker matches exact hostnames and all
-parent domains (subdomain matching), and checks a hostname written with a
-trailing dot (`name.`) as `name`.
+parent domains (subdomain matching).
 
 #### Transaction Decoding
 

src/background/index.js

@@ -22,7 +22,6 @@ const { debugFetch, log } = require("../shared/log");
 const {
     verifySignedTx,
     verifySignature,
-    typedDataChainRefusal,
     failureIsRetryable,
     describeTxFailure,
     sameAddress,
@@ -985,9 +984,8 @@ async function handleRpc(method, params, origin) {
 
         if (method === "eth_sign") {
             signParams.dangerWarning =
-                "\u26a0\ufe0f DANGER: This site is requesting to sign a raw hash. " +
-                "This can be used to sign transactions that drain your funds. " +
-                "Only proceed if you fully understand what you are signing.";
+                "This site used the older eth_sign method. " +
+                "The data will be signed as a personal message.";
         }
 
         if (findPendingApproval(origin, "sign")) {
@@ -1765,8 +1763,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
 
         (async () => {
             try {
-                const s = await getState();
-                const activeAddress = activeAddressOf(s);
+                const activeAddress = await getActiveAddress();
                 // Same as the transaction path: the address the approval named
                 // is the one that must have signed, and a switch since then is
                 // a refusal rather than a signature from another account.
@@ -1785,20 +1782,6 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                     signature,
                     approval.approvedFrom,
                 );
-                // 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 refusal = typedDataChainRefusal(
-                        JSON.parse(sp.typedData).domain,
-                        networkById(s.networkId),
-                    );
-                    if (refusal) throw new ApprovalMismatchError(refusal);
-                }
                 settleApproval(msg.id, { signature }, { holdsClaim: true });
                 sendResponse({ signature });
             } catch (e) {

src/popup/views/approval.js

@@ -12,7 +12,7 @@ const {
     formatFee,
     tokenLabel,
 } = require("./helpers");
-const { state, saveState, currentNetwork } = require("../../shared/state");
+const { state, saveState } = require("../../shared/state");
 const {
     networkById,
     networkByChainId,
@@ -46,10 +46,7 @@ const {
 const { decryptWithPassword } = require("../../shared/vault");
 const { getSignerForAddress } = require("../../shared/wallet");
 const { walletDefect } = require("../../shared/walletDefects");
-const {
-    describeSigningFailure,
-    typedDataChainRefusal,
-} = require("../../shared/approvalVerify");
+const { describeSigningFailure } = require("../../shared/approvalVerify");
 const txStatus = require("./txStatus");
 const uniswap = require("../../shared/uniswap");
 const { notify, runtimeApi, sendMessage } = require("../../shared/browserApi");
@@ -446,8 +443,7 @@ function signedPrimaryType(types) {
 // 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 domain may name no chain but the active
-// network's.
+// message while another is signed.
 function typedDataRefusal(sp) {
     if (sp.method === "personal_sign" || sp.method === "eth_sign") return null;
     let data;
@@ -470,7 +466,7 @@ function typedDataRefusal(sp) {
             ", so it cannot be signed."
         );
     }
-    return typedDataChainRefusal(data.domain, currentNetwork());
+    return null;
 }
 
 // The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats
@@ -725,7 +721,7 @@ function showSignApproval(details) {
             "This message is plain text, not hex, so it cannot be signed.";
     }
 
-    // Display danger warning for eth_sign (raw hash signing)
+    // The warning the background attaches to an eth_sign request.
     const warningEl = $("approve-sign-danger-warning");
     if (warningEl) {
         if (sp.dangerWarning) {

src/shared/approvalVerify.js

@@ -53,7 +53,6 @@ const {
     accessListify,
     formatEther,
     getAddress,
-    getBigInt,
     getBytes,
     toQuantity,
     verifyMessage,
@@ -636,33 +635,6 @@ function verifySignature(signParams, signature, expectedFrom) {
     return recovered;
 }
 
-// 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.
-// ethers signs no chain id when the domain's is null or absent, and reads any
-// other with getBigInt(), so 1, "1" and "0x1" are the same chain here too.
-function typedDataChainRefusal(domain, network) {
-    const chainId = domain?.chainId;
-    if (chainId === undefined || chainId === null) return null;
-    let named;
-    try {
-        named = getBigInt(chainId);
-    } catch {
-        return "This typed data cannot be read, so it cannot be signed.";
-    }
-    const active = getBigInt(network.chainId);
-    if (named === active) return null;
-    return (
-        "This typed data is for chain id " +
-        named +
-        ", but the active network is " +
-        network.name +
-        " (chain id " +
-        active +
-        "), so it cannot be signed."
-    );
-}
-
 // The stage a transaction approval failed at. Which stage it is decides
 // whether the approval survives the failure.
 const TX_STAGE_SIGN = "sign";
@@ -824,7 +796,6 @@ function describeSigningFailure(response, fallbackMessage) {
 module.exports = {
     verifySignedTx,
     verifySignature,
-    typedDataChainRefusal,
     assertNoForbiddenFields,
     assertNothingUnchecked,
     assertCanonicalBytes,

src/shared/phishingDomains.js

@@ -110,14 +110,11 @@ function hashListed(hash) {
  * Generate hostname variants for subdomain matching.
  * "sub.evil.com" yields ["sub.evil.com", "evil.com"].
  *
- * A trailing dot is dropped: "evil.com." is the fully qualified spelling of
- * "evil.com", and the browser loads the same site from either.
- *
  * @param {string} hostname
  * @returns {string[]}
  */
 function hostnameVariants(hostname) {
-    const h = hostname.toLowerCase().replace(/\.$/, "");
+    const h = hostname.toLowerCase();
     const variants = [h];
     const parts = h.split(".");
     // Parent domains: a.b.c.d -> b.c.d, c.d

tests/backgroundApproval.test.js

@@ -397,13 +397,7 @@ function loadBackground(options) {
                 rpcResult = r;
             },
         );
-        return {
-            id: () =>
-                new URL(created[created.length - 1].url).searchParams.get(
-                    "approval",
-                ),
-            result: () => rpcResult,
-        };
+        return { result: () => rpcResult };
     }
 
     // A dApp asking to connect. The origin defaults to one the persisted
@@ -1490,55 +1484,6 @@ describe("what the approval is verified against", () => {
         });
     });
 
-    // Typed data whose domain names another chain is refused on the sign
-    // 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) {
-        const { domain, types, message } = JSON.parse(TYPED_DATA);
-        delete types.EIP712Domain;
-        const signature = await signer.signTypedData(domain, types, message);
-        const answer = bg.send(
-            {
-                type: "AUTISTMASK_SIGN_RESPONSE",
-                id: pending.id(),
-                approved: true,
-                signature,
-            },
-            { url: bg.fromPopup.url },
-        );
-        await settle();
-        return { answer, signature };
-    }
-
-    test("typed data for the active chain is handed back signed", async () => {
-        const bg = loadBackground();
-        const pending = bg.requestTypedData();
-        await settle();
-
-        const { answer, signature } = await answerTypedData(bg, pending);
-
-        expect(answer.sendResponse).toHaveBeenCalledWith({ signature });
-        expect(pending.result()).toEqual({ result: signature });
-    });
-
-    test("typed data for another chain is refused, not handed back", async () => {
-        const bg = loadBackground();
-        bg.setNetwork(SEPOLIA);
-        const pending = bg.requestTypedData();
-        await settle();
-
-        const { answer } = await answerTypedData(bg, pending);
-
-        const refusal =
-            "This typed data is for chain id 1, but the active network is Sepolia Testnet (chain id 11155111), so it cannot be signed.";
-        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);
@@ -1606,6 +1551,53 @@ describe("what the approval is verified against", () => {
     });
 });
 
+// eth_sign names the address first and the data second, the reverse of
+// personal_sign, and is answered as the JSON-RPC specification defines it:
+// with an EIP-191 personal message signature over the data, the same
+// signature personal_sign returns
+// (https://git.eeqj.de/sneak/AutistMask/issues/409).
+describe("eth_sign", () => {
+    test("is read address first, and returns a personal message signature over the data", async () => {
+        const bg = loadBackground();
+        const page = bg.send(
+            {
+                type: "AUTISTMASK_RPC",
+                method: "eth_sign",
+                params: [signer.address, MESSAGE],
+            },
+            { origin: ORIGIN },
+        );
+        await settle();
+        const id = approvalIdIn(bg.created.map((c) => c.url));
+
+        const details = bg.send(
+            { type: "AUTISTMASK_GET_APPROVAL", id },
+            { url: bg.fromPopup.url },
+        );
+        const { signParams } = details.sendResponse.mock.calls[0][0];
+        expect(signParams).toMatchObject({
+            method: "eth_sign",
+            message: MESSAGE,
+            from: signer.address,
+        });
+        expect(signParams.dangerWarning).toBe(
+            "This site used the older eth_sign method. " +
+                "The data will be signed as a personal message.",
+        );
+
+        const signature = await signer.signMessage(
+            Buffer.from(MESSAGE.slice(2), "hex"),
+        );
+        bg.send(
+            { type: "AUTISTMASK_SIGN_RESPONSE", id, approved: true, signature },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(page.sendResponse).toHaveBeenCalledWith({ result: signature });
+    });
+});
+
 // The interlock must not cost the retry the approval exists to allow.
 describe("the interlock releases a failed attempt", () => {
     test("a retryable failure before the broadcast leaves the approval usable", async () => {

tests/phishingDomains.test.js

@@ -89,12 +89,6 @@ describe("vendored blocklist", () => {
         expect(isPhishingDomain(LISTED[0].toUpperCase())).toBe(true);
     });
 
-    test("a hostname ending in a dot is the same host", () => {
-        expect(isPhishingDomain(LISTED[1] + ".")).toBe(true);
-        expect(isPhishingDomain("wallet." + LISTED[1] + ".")).toBe(true);
-        expect(isPhishingDomain(CLEAN[0] + ".")).toBe(false);
-    });
-
     test("returns false for an empty or missing hostname", () => {
         expect(isPhishingDomain("")).toBe(false);
         expect(isPhishingDomain(null)).toBe(false);
@@ -139,13 +133,6 @@ describe("hostnameVariants", () => {
     test("lowercases hostnames", () => {
         expect(hostnameVariants("Evil.COM")).toEqual(["evil.com"]);
     });
-
-    test("drops the trailing dot of a fully qualified hostname", () => {
-        expect(hostnameVariants("sub.evil.com.")).toEqual([
-            "sub.evil.com",
-            "evil.com",
-        ]);
-    });
 });
 
 describe("domain hashing", () => {

tests/typedDataPermit.test.js

@@ -240,7 +240,6 @@ function request(data) {
 beforeEach(() => {
     state.trackedTokens = [];
     state.wallets = [];
-    state.networkId = "mainnet";
 });
 
 describe("a token permission is warned about, naming spender and amount", () => {
@@ -533,66 +532,3 @@ describe("the sign screen refuses a mismatched primary type", () => {
         expect(node("btn-approve-sign").disabled).toBe(true);
     });
 });
-
-// EIP-712: a wallet refuses typed data whose domain names a chain other than
-// the active one, because the signature is valid on the chain it names
-// (https://git.eeqj.de/sneak/AutistMask/issues/519). The wallet is on Sepolia
-// and the site asks for a mainnet USDC Permit.
-describe("the sign screen refuses typed data for another chain", () => {
-    const REFUSAL =
-        "This typed data is for chain id 1, but the active network is Sepolia Testnet (chain id 11155111), so it cannot be signed.";
-
-    function withChainId(chainId) {
-        return { ...PERMIT, domain: { ...PERMIT.domain, chainId } };
-    }
-
-    beforeEach(() => {
-        decryptWithPassword.mockClear();
-        state.networkId = "sepolia";
-    });
-
-    test("another chain shows the error, with Sign disabled", async () => {
-        await openSignScreen(PERMIT);
-        expect(node("approve-sign-error").textContent).toBe(REFUSAL);
-        expect(node("btn-approve-sign").disabled).toBe(true);
-    });
-
-    test("Sign clicked anyway decrypts and signs nothing", async () => {
-        const sent = await openSignScreen(PERMIT);
-        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(REFUSAL);
-    });
-
-    // ethers signs 1, "1" and "0x1" as the same chain id.
-    test("the chain id is refused in each form ethers signs", () => {
-        for (const chainId of [1, "1", "0x1"]) {
-            expect(typedDataRefusal(request(withChainId(chainId)))).toBe(
-                REFUSAL,
-            );
-        }
-    });
-
-    test("a chain id that is not a number is refused", () => {
-        expect(typedDataRefusal(request(withChainId("mainnet")))).toBe(
-            "This typed data cannot be read, so it cannot be signed.",
-        );
-    });
-
-    test("the active chain leaves Sign enabled", async () => {
-        await openSignScreen(withChainId("0xaa36a7"));
-        expect(node("approve-sign-error").textContent).toBe("");
-        expect(node("btn-approve-sign").disabled).toBe(false);
-    });
-
-    test("no chain id leaves Sign enabled", async () => {
-        await openSignScreen(withChainId(undefined));
-        expect(node("approve-sign-error").textContent).toBe("");
-        expect(node("btn-approve-sign").disabled).toBe(false);
-    });
-});