From e74cc1be062bd48663588acfbb88cdde449776da Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 14:51:49 +0000 Subject: [PATCH] fix: typed data with a missing field or a bool that is not true or false is refused (closes #552) ethers signs a bool field by whether its value is truthy, so the string "false" was shown as false and signed as true, and a bool field left out was shown as nothing and signed as false. The sign screen now refuses typed data whose message leaves out a declared field, at any level of nesting, or holds a bool that is not true or false: the error line names the field and Sign is disabled. The background refuses a signature for it too. Presence is an own-key test, so a field named like a built-in property such as constructor is not found by accident. signedPrimaryType() moves to the shared module so the background reads the message as the same type. Model: opus-5-5 --- README.md | 7 ++ src/background/index.js | 25 ++++++-- src/popup/views/approval.js | 24 +++---- src/shared/approvalVerify.js | 60 +++++++++++++++++ tests/backgroundApproval.test.js | 44 +++++++++++-- tests/typedDataPermit.test.js | 106 ++++++++++++++++++++++++++++++- 6 files changed, 239 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index d08f843..5c5b35c 100644 --- a/README.md +++ b/README.md @@ -2221,6 +2221,13 @@ 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 diff --git a/src/background/index.js b/src/background/index.js index 2b55689..9e31536 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -22,6 +22,8 @@ const { debugFetch, log } = require("../shared/log"); const { verifySignedTx, verifySignature, + signedPrimaryType, + typedDataMessageRefusal, typedDataChainRefusal, failureIsRetryable, describeTxFailure, @@ -1787,18 +1789,27 @@ 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. + // 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. 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), - ); + const data = JSON.parse(sp.typedData); + const refusal = + typedDataMessageRefusal( + data.types, + signedPrimaryType(data.types), + data.message, + "", + ) || + typedDataChainRefusal( + data.domain, + networkById(s.networkId), + ); if (refusal) throw new ApprovalMismatchError(refusal); } settleApproval(msg.id, { signature }, { holdsClaim: true }); diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 21daae4..b49da47 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -27,7 +27,6 @@ const { Interface, MaxUint256, toUtf8String, - TypedDataEncoder, } = require("ethers"); const { getPrice, formatUsd } = require("../../shared/prices"); const { ERC20_ABI } = require("../../shared/constants"); @@ -48,6 +47,8 @@ const { getSignerForAddress } = require("../../shared/wallet"); const { walletDefect } = require("../../shared/walletDefects"); const { describeSigningFailure, + signedPrimaryType, + typedDataMessageRefusal, typedDataChainRefusal, } = require("../../shared/approvalVerify"); const txStatus = require("./txStatus"); @@ -432,22 +433,12 @@ 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 domain may name no chain but the active -// network's. +// 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. function typedDataRefusal(sp) { if (sp.method === "personal_sign" || sp.method === "eth_sign") return null; let data; @@ -470,7 +461,10 @@ function typedDataRefusal(sp) { ", so it cannot be signed." ); } - return typedDataChainRefusal(data.domain, currentNetwork()); + return ( + typedDataMessageRefusal(data.types, signed, data.message, "") || + typedDataChainRefusal(data.domain, currentNetwork()) + ); } // The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats diff --git a/src/shared/approvalVerify.js b/src/shared/approvalVerify.js index 4aa1a6a..4bce46d 100644 --- a/src/shared/approvalVerify.js +++ b/src/shared/approvalVerify.js @@ -56,6 +56,7 @@ const { getBigInt, getBytes, toQuantity, + TypedDataEncoder, verifyMessage, verifyTypedData, } = require("ethers"); @@ -636,6 +637,63 @@ 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. @@ -824,6 +882,8 @@ function describeSigningFailure(response, fallbackMessage) { module.exports = { verifySignedTx, verifySignature, + signedPrimaryType, + typedDataMessageRefusal, typedDataChainRefusal, assertNoForbiddenFields, assertNothingUnchecked, diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 07218fd..adf753b 100644 --- a/tests/backgroundApproval.test.js +++ b/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() { + function requestTypedData(typedData = TYPED_DATA) { let rpcResult = null; messageListener( { type: "AUTISTMASK_RPC", method: "eth_signTypedData_v4", - params: [signer.address, TYPED_DATA], + params: [signer.address, typedData], }, { 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) { - const { domain, types, message } = JSON.parse(TYPED_DATA); + async function answerTypedData(bg, pending, typedData = TYPED_DATA) { + const { domain, types, message } = JSON.parse(typedData); delete types.EIP712Domain; const signature = await signer.signTypedData(domain, types, message); const answer = bg.send( @@ -1539,6 +1539,42 @@ 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); diff --git a/tests/typedDataPermit.test.js b/tests/typedDataPermit.test.js index 3fb39ec..a560a1c 100644 --- a/tests/typedDataPermit.test.js +++ b/tests/typedDataPermit.test.js @@ -28,7 +28,7 @@ globalThis.chrome = { storage: { local: { get: async () => ({}), set: async () => {} } }, }; -const { getAddress, MaxUint256 } = require("ethers"); +const { getAddress, MaxUint256, TypedDataEncoder } = require("ethers"); const { state } = require("../src/shared/state"); const { decryptWithPassword } = require("../src/shared/vault"); const approval = require("../src/popup/views/approval"); @@ -662,3 +662,107 @@ 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.', + ); + }); +});