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.