From f283a5bb91b565aa7ac42cab970753b3d8f31022 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 14 Oct 2026 16:41:13 +0200 Subject: [PATCH] fix: the typed data screen shows only the message fields that are signed (closes #539) ethers signs only the fields a type declares and drops any other key, but the Message section listed every key the site sent, so an added key read as signed. The Message section now follows the signed type, at every level of nesting and in the type's order. Typed data with other keys is not refused: those keys are listed after the message under "Not part of the signature", by their place in the message. README SignApproval says so. The operator/spender test now also checks the Message section, and new tests cover an added key in a nested struct and in a list item. Model: opus-5-5 --- README.md | 7 +++- src/popup/views/approval.js | 54 +++++++++++++++++++++++++--- tests/typedDataPermit.test.js | 68 ++++++++++++++++++++++++++++++++++- 3 files changed, 123 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index b8fb070..8e64837 100644 --- a/README.md +++ b/README.md @@ -2156,7 +2156,12 @@ view would leave a wallet one click from deletion. Bytes that are not UTF-8 are shown as "This message is not text." For typed data, formatted domain/type/message fields (EIP-712). The primary type shown is the one ethers signs, derived from the typed data's `types`, - not the type the site states. + not the type the site states. The message shows only the fields that type + declares, at every level of nesting and in the type's order: ethers signs + those and drops any other key. Typed data carrying other keys is not + refused; each such key is listed after the message under "Not part of the + signature", by its place in the message (`details.note`, + `details[1].note`) and its value. - Token permission warning, at the top of the message (typed data whose primary type is `Permit`, as in EIP-2612, or one of Permit2's signature types): "⚠️ TOKEN PERMISSION: Signing this lets the spender below take the diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 4a65d90..d748112 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -626,10 +626,38 @@ function permitWarningHtml(types, primaryType, domain, message) { return html; } +// What ethers signs of a message value of type `type`: a struct keeps only +// the fields its type declares, in the type's order, and a list is read item +// by item. ethers drops every other key, so each one is added to `notSigned` +// with its place in the message and its value, for the screen to list apart. +function signedValue(types, type, value, path, notSigned) { + const itemType = type.replace(/\[\d*\]$/, ""); + if (itemType !== type) { + if (!Array.isArray(value)) return value; + return value.map((item, i) => + signedValue(types, itemType, item, `${path}[${i}]`, notSigned), + ); + } + const fields = types[type]; + if (!fields || !value || typeof value !== "object") return value; + const at = (name) => (path ? `${path}.${name}` : name); + for (const [key, val] of Object.entries(value)) { + if (!fields.some((f) => f.name === key)) notSigned.push([at(key), val]); + } + return Object.fromEntries( + fields.map((f) => [ + f.name, + signedValue(types, f.type, value[f.name], at(f.name), notSigned), + ]), + ); +} + // The typed data as the screen shows it. The primary type shown is the one // ethers signs, never the page's word for it; typedDataRefusal() keeps the two -// from differing on anything that can be signed. Only typed data that cannot -// be read at all is shown as raw text, and typedDataRefusal() refuses it. +// from differing on anything that can be signed. The Message section shows +// only what is signed, and any other key the page sent is listed after it as +// not part of the signature. Only typed data that cannot be read at all is +// shown as raw text, and typedDataRefusal() refuses it. function formatTypedDataHtml(jsonStr) { let data; let primaryType; @@ -663,12 +691,30 @@ function formatTypedDataHtml(jsonStr) { html += `
Primary type
`; html += `
${escapeHtml(primaryType)}
`; + const lineHtml = (key, val) => + `
${escapeHtml(key)}: ${escapeHtml(display(val))}
`; + if (data.message) { + const notSigned = []; + const message = signedValue( + data.types, + primaryType, + data.message, + "", + notSigned, + ); html += `
Message
`; - for (const [key, val] of Object.entries(data.message)) { - html += `
${escapeHtml(key)}: ${escapeHtml(display(val))}
`; + for (const [key, val] of Object.entries(message)) { + html += lineHtml(key, val); } html += `
`; + if (notSigned.length > 0) { + html += `
Not part of the signature
`; + for (const [key, val] of notSigned) { + html += lineHtml(key, val); + } + html += `
`; + } } return html; diff --git a/tests/typedDataPermit.test.js b/tests/typedDataPermit.test.js index a9fcf96..3fb39ec 100644 --- a/tests/typedDataPermit.test.js +++ b/tests/typedDataPermit.test.js @@ -233,6 +233,26 @@ function warningOf(data) { return html.slice(0, html.indexOf(">Domain<")); } +const NOT_SIGNED = ">Not part of the signature<"; + +// The Message section alone, without the keys listed after it as not part of +// the signature. +function messageOf(data) { + const html = formatTypedDataHtml(JSON.stringify(data)); + const end = html.indexOf(NOT_SIGNED); + return html.slice( + html.indexOf(">Message<"), + end === -1 ? html.length : end, + ); +} + +// The keys listed as not part of the signature, or "" when there are none. +function notSignedOf(data) { + const html = formatTypedDataHtml(JSON.stringify(data)); + const start = html.indexOf(NOT_SIGNED); + return start === -1 ? "" : html.slice(start); +} + function request(data) { return { method: "eth_signTypedData_v4", typedData: JSON.stringify(data) }; } @@ -312,7 +332,8 @@ describe("the warning reads only the fields the signed type declares", () => { }); // A Permit whose type names its spender `operator`; the page adds a - // `spender` key the type does not declare. + // `spender` key the type does not declare. The Message section names + // only the operator, and the added key is listed apart as not signed. test("an added key is not named as the spender", () => { const data = { ...PERMIT, @@ -331,6 +352,10 @@ describe("the warning reads only the fields the signed type declares", () => { const warning = warningOf(data); expect(warning).toContain(WARNING); expect(warning).not.toContain(DECOY); + expect(messageOf(data)).toContain(SPENDER); + expect(messageOf(data)).not.toContain(DECOY); + expect(notSignedOf(data)).toContain("spender:"); + expect(notSignedOf(data)).toContain(DECOY); }); // The permit for a Uniswap v3 position NFT: a `Permit` with no amount @@ -371,6 +396,47 @@ describe("the warning reads only the fields the signed type declares", () => { }); }); +// ethers signs only the declared fields at every level of nesting, so an +// added key inside a struct is not signed either. Such typed data is not +// refused; the added key is listed apart, with where it sits in the message. +describe("the Message section shows only the fields that are signed", () => { + const NOTE = "This signature only signs you in"; + + test("an added key inside a nested struct is listed as not signed", () => { + const data = { + ...PERMIT_SINGLE, + message: { + ...PERMIT_SINGLE.message, + details: { ...PERMIT_SINGLE.message.details, note: NOTE }, + }, + }; + expect(messageOf(data)).toContain("expiration"); + expect(messageOf(data)).not.toContain(NOTE); + expect(notSignedOf(data)).toContain("details.note:"); + expect(notSignedOf(data)).toContain(NOTE); + expect(typedDataRefusal(request(data))).toBeNull(); + }); + + test("an added key inside a list item is listed with its index", () => { + const [first, second] = PERMIT_BATCH.message.details; + const data = { + ...PERMIT_BATCH, + message: { + ...PERMIT_BATCH.message, + details: [first, { ...second, note: NOTE }], + }, + }; + expect(messageOf(data)).not.toContain(NOTE); + expect(notSignedOf(data)).toContain("details[1].note:"); + expect(notSignedOf(data)).toContain(NOTE); + }); + + test("nothing is listed apart when every key is declared", () => { + expect(notSignedOf(PERMIT_SINGLE)).toBe(""); + expect(notSignedOf(LOGIN)).toBe(""); + }); +}); + describe("the primary type shown is the one ethers signs", () => { // A drain stated as a sign-in: the page says Login, the types say // PermitSingle, and ethers would sign PermitSingle.