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

@@ -239,12 +239,8 @@ provide:
   `make check` stays non-mutating. Linting runs only in the container, because
   an ESLint result that depends on whichever ESLint the host happens to have is
   not a result, so docker is required to lint.
-- `script/fmt` — format all files (writes), on the host, with the `prettier`
-  that `script/bootstrap` installs in `node_modules/`; when that one is missing
-  it fails and says to run `script/bootstrap`, instead of using a `prettier`
-  from the `PATH`
-- `script/fmt-check` — check formatting (read-only), on the host, with the same
-  `prettier` and the same refusal
+- `script/fmt` — format all files (writes), on the host
+- `script/fmt-check` — check formatting (read-only), on the host
 - `script/check` — run `script/test`, `script/lint` and `script/fmt-check`
 - `script/check-censored` — assert the competitor name RULES.md bars appears
   nowhere in the working tree or under `dist/` outside its documented
@@ -2128,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
@@ -2164,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

script/fmt

@@ -25,13 +25,6 @@ run_yarn() {
 
 main() {
     cd "$ROOT"
-    # Without the repo's own prettier, yarn runs whatever prettier is on
-    # the PATH, which may be a version that formats files differently.
-    if [ ! -x node_modules/.bin/prettier ]; then
-        echo "fmt: this repo's prettier is not installed in node_modules/," \
-            "so run script/bootstrap first." >&2
-        exit 1
-    fi
     echo "Formatting..."
     run_yarn run fmt
 }

script/fmt-check

@@ -26,13 +26,6 @@ run_yarn() {
 
 main() {
     cd "$ROOT"
-    # Without the repo's own prettier, yarn runs whatever prettier is on
-    # the PATH, which may be a version that formats files differently.
-    if [ ! -x node_modules/.bin/prettier ]; then
-        echo "fmt-check: this repo's prettier is not installed in" \
-            "node_modules/, so run script/bootstrap first." >&2
-        exit 1
-    fi
     echo "Checking formatting..."
     run_yarn run fmt-check
 }

src/background/index.js

@@ -22,7 +22,6 @@ const { debugFetch, log } = require("../shared/log");
 const {
     verifySignedTx,
     verifySignature,
-    typedDataChainRefusal,
     failureIsRetryable,
     describeTxFailure,
     sameAddress,
@@ -633,9 +632,8 @@ function isExtensionSender(sender) {
 // had refused.
 //
 // TX and sign approvals do not decide here. They stay pending across a
-// disconnect. When their window closes, the windows.onRemoved listener below
-// rejects them, unless an approved attempt is still running, in which case
-// that attempt answers the page.
+// disconnect — the user can reopen the toolbar popup — and are rejected by the
+// windows.onRemoved listener below.
 runtime.onConnect.addListener((port) => {
     if (port.name.startsWith("approval:")) {
         const id = port.name.split(":")[1];
@@ -663,9 +661,7 @@ runtime.onConnect.addListener((port) => {
             const approval = pendingApprovals[id];
             if (approval) {
                 if (approval.type === "tx" || approval.type === "sign") {
-                    // When its window closes, windows.onRemoved rejects it,
-                    // unless an approved attempt is still running, in which
-                    // case that attempt answers the page.
+                    // Keep pending — user can reopen the toolbar popup
                     return;
                 }
                 settleApproval(id, { approved: false, remember: false });
@@ -988,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")) {
@@ -1768,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.
@@ -1788,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/index.html

@@ -1716,7 +1716,10 @@
                     <div class="text-xs text-muted mb-1">
                         Address that will be shared:
                     </div>
-                    <div id="approve-address" class="text-xs mb-2"></div>
+                    <div
+                        id="approve-address"
+                        class="text-xs flex items-center mb-2"
+                    ></div>
                 </div>
                 <div class="mb-3">
                     <label

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,

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/e2e/run.js

@@ -3857,9 +3857,6 @@ test("eth_requestAccounts approved returns the selected address (#183)", async (
             shown.toLowerCase().includes(env.expectedAddress.toLowerCase()),
             "the site prompt shows the wrong address: " + JSON.stringify(shown),
         );
-        // The text above is there even when the row is too narrow to show it
-        // all; this measures that the whole address is visible (#517).
-        await assertAddressesFit(popup, "the connection prompt");
 
         // Remembered, so the connection survives a background worker that MV3
         // terminates after 30 seconds idle. The in-memory connectedSites map

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);
-    });
-});