From 2b72d8637b2d5f3ae66570171d7ea896b0e88978 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 19 Apr 2022 18:17:01 -0230 Subject: [PATCH 1/3] Fix typed message schema mistake The exported JSON schema `TYPED_MESSAGE_SCHEMA` was changed in v4 to specify all supported Solidity types, but this accidentally prevented the use of reference types. This change was undone, so that the schema no longer validates that the types used are valid. I could not find a way to preserve this validation without disallowing reference types. JSON schema did not make this possible. Tests were added for the typed message schema to ensure this doesn't happen again. Fixes #242 --- package.json | 1 + src/sign-typed-data.test.ts | 215 ++++++++++++++++++++++++++++++++++++ src/sign-typed-data.ts | 22 +--- yarn.lock | 10 ++ 4 files changed, 227 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index 4b87f32e..52adae22 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "@types/node": "^14.14.25", "@typescript-eslint/eslint-plugin": "^4.28.2", "@typescript-eslint/parser": "^4.28.2", + "ajv": "^8.11.0", "eslint": "^7.30.0", "eslint-config-prettier": "^8.3.0", "eslint-plugin-import": "^2.23.4", diff --git a/src/sign-typed-data.test.ts b/src/sign-typed-data.test.ts index c8e8486a..9bab5117 100644 --- a/src/sign-typed-data.test.ts +++ b/src/sign-typed-data.test.ts @@ -1,10 +1,12 @@ import * as ethUtil from 'ethereumjs-util'; +import Ajv from 'ajv'; import { recoverTypedSignature, signTypedData, TypedDataUtils, typedSignatureHash, SignTypedDataVersion, + TYPED_MESSAGE_SCHEMA, } from './sign-typed-data'; const privateKey = Buffer.from( @@ -12,6 +14,219 @@ const privateKey = Buffer.from( 'hex', ); +/** + * Get a list of all Solidity types supported by EIP-712. + * + * @returns A list of all supported Solidity types. + */ +function getEip712SolidityTypes() { + const types = ['bool', 'address', 'string', 'bytes']; + const ints = Array.from(new Array(32)).map( + (_, index) => `int${(index + 1) * 8}`, + ); + const uints = Array.from(new Array(32)).map( + (_, index) => `uint${(index + 1) * 8}`, + ); + const bytes = Array.from(new Array(32)).map( + (_, index) => `bytes${index + 1}`, + ); + + return [...types, ...ints, ...uints, ...bytes]; +} + +const eip712SolidityTypes = getEip712SolidityTypes(); + +describe('TYPED_MESSAGE_SCHEMA', () => { + it('should match valid typed message', () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'object', + types: { + EIP712Domain: [], + }, + }; + + expect(validate(typedMessage)).toBe(true); + }); + + it('should allow custom types in addition to domain', () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'Message', + types: { + EIP712Domain: [], + Message: [], + }, + }; + + expect(validate(typedMessage)).toBe(true); + }); + + for (const solidityType of eip712SolidityTypes) { + // eslint-disable-next-line no-loop-func + it(`should allow custom type to have type of '${solidityType}'`, () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'Message', + types: { + EIP712Domain: [], + Message: [{ name: 'data', type: solidityType }], + }, + }; + + expect(validate(typedMessage)).toBe(true); + }); + } + + it('should allow custom type to have a custom type', () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'Message', + types: { + CustomValue: [{ name: 'value', type: 'string' }], + EIP712Domain: [], + Message: [{ name: 'data', type: 'CustomValue' }], + }, + }; + + expect(validate(typedMessage)).toBe(true); + }); + + const invalidStrings = [undefined, null, 0, 1, [], {}]; + + for (const invalidString of invalidStrings) { + // eslint-disable-next-line no-loop-func + it(`should disallow a primary type with value '${invalidString}'`, () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: invalidString, + types: { + EIP712Domain: [], + }, + }; + + expect(validate(typedMessage)).toBe(false); + }); + } + + const invalidObjects = [undefined, null, 0, 1, [], '', 'test']; + for (const invalidObject of invalidObjects) { + // eslint-disable-next-line no-loop-func + it(`should disallow a domain with value '${invalidObject}'`, () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: invalidObject, + message: {}, + primaryType: 'object', + types: { + EIP712Domain: [], + }, + }; + + expect(validate(typedMessage)).toBe(false); + }); + + // eslint-disable-next-line no-loop-func + it(`should disallow a message with value '${invalidObject}'`, () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: invalidObject, + primaryType: 'object', + types: { + EIP712Domain: [], + }, + }; + + expect(validate(typedMessage)).toBe(false); + }); + + // eslint-disable-next-line no-loop-func + it(`should disallow types with value '${invalidObject}'`, () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'object', + types: invalidObject, + }; + + expect(validate(typedMessage)).toBe(false); + }); + } + + it('should require custom type properties to have a name', () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'Message', + types: { + EIP712Domain: [], + Message: [{ type: 'string' }], + }, + }; + + expect(validate(typedMessage)).toBe(false); + }); + + it('should require custom type properties to have a type', () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'Message', + types: { + EIP712Domain: [], + Message: [{ name: 'name' }], + }, + }; + + expect(validate(typedMessage)).toBe(false); + }); + + const invalidTypes = [undefined, null, 0, 1, [], {}]; + + for (const invalidType of invalidTypes) { + // eslint-disable-next-line no-loop-func + it(`should disallow a type of '${invalidType}'`, () => { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + const typedMessage = { + domain: {}, + message: {}, + primaryType: 'Message', + types: { + EIP712Domain: [], + Message: [{ name: 'name', type: invalidType }], + }, + }; + + expect(validate(typedMessage)).toBe(false); + }); + } +}); + const encodeDataExamples = { // dynamic types supported by EIP-712: bytes: [10, '10', '0x10', Buffer.from('10', 'utf8')], diff --git a/src/sign-typed-data.ts b/src/sign-typed-data.ts index ca85d2cb..61ca1862 100644 --- a/src/sign-typed-data.ts +++ b/src/sign-typed-data.ts @@ -100,7 +100,7 @@ export const TYPED_MESSAGE_SCHEMA = { type: 'object', properties: { name: { type: 'string' }, - type: { type: 'string', enum: getSolidityTypes() }, + type: { type: 'string' }, }, required: ['name', 'type'], }, @@ -113,26 +113,6 @@ export const TYPED_MESSAGE_SCHEMA = { required: ['types', 'primaryType', 'domain', 'message'], }; -/** - * Get a list of all Solidity types. - * - * @returns A list of all Solidity types. - */ -function getSolidityTypes() { - const types = ['bool', 'address', 'string', 'bytes']; - const ints = Array.from(new Array(32)).map( - (_, index) => `int${(index + 1) * 8}`, - ); - const uints = Array.from(new Array(32)).map( - (_, index) => `uint${(index + 1) * 8}`, - ); - const bytes = Array.from(new Array(32)).map( - (_, index) => `bytes${index + 1}`, - ); - - return [...types, ...ints, ...uints, ...bytes]; -} - /** * Validate that the given value is a valid version string. * diff --git a/yarn.lock b/yarn.lock index b8ee7e8a..58f80c27 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1005,6 +1005,16 @@ ajv@^8.0.1: require-from-string "^2.0.2" uri-js "^4.2.2" +ajv@^8.11.0: + version "8.11.0" + resolved "https://registry.yarnpkg.com/ajv/-/ajv-8.11.0.tgz#977e91dd96ca669f54a11e23e378e33b884a565f" + integrity sha512-wGgprdCvMalC0BztXvitD2hC04YffAvtsUn93JbGXYLAtCUO4xd17mCCZQxUOItiBwZvJScWo8NIvQMQ71rdpg== + dependencies: + fast-deep-equal "^3.1.1" + json-schema-traverse "^1.0.0" + require-from-string "^2.0.2" + uri-js "^4.2.2" + ansi-colors@^4.1.1: version "4.1.1" resolved "https://registry.yarnpkg.com/ansi-colors/-/ansi-colors-4.1.1.tgz#cbb9ae256bf750af1eab344f229aa27fe94ba348" From b5f0d877f72fb36fd41d64eec7a8cb248aba33f5 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 20 Apr 2022 18:37:33 -0230 Subject: [PATCH 2/3] Reusing validation setup steps between tests --- src/sign-typed-data.test.ts | 63 ++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/src/sign-typed-data.test.ts b/src/sign-typed-data.test.ts index 9bab5117..2cedcf60 100644 --- a/src/sign-typed-data.test.ts +++ b/src/sign-typed-data.test.ts @@ -36,10 +36,22 @@ function getEip712SolidityTypes() { const eip712SolidityTypes = getEip712SolidityTypes(); +/** + * Validate the given message with the typed message schema. + * + * @param typedMessage - The typed message to validate. + * @returns Whether the message is valid. + */ +function validateTypedMessageSchema( + typedMessage: Record, +): boolean { + const ajv = new Ajv(); + const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); + return validate(typedMessage); +} + describe('TYPED_MESSAGE_SCHEMA', () => { it('should match valid typed message', () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -49,12 +61,10 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(true); + expect(validateTypedMessageSchema(typedMessage)).toBe(true); }); it('should allow custom types in addition to domain', () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -65,14 +75,11 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(true); + expect(validateTypedMessageSchema(typedMessage)).toBe(true); }); - for (const solidityType of eip712SolidityTypes) { - // eslint-disable-next-line no-loop-func + eip712SolidityTypes.forEach((solidityType) => { it(`should allow custom type to have type of '${solidityType}'`, () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -83,13 +90,11 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(true); + expect(validateTypedMessageSchema(typedMessage)).toBe(true); }); - } + }); it('should allow custom type to have a custom type', () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -101,7 +106,7 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(true); + expect(validateTypedMessageSchema(typedMessage)).toBe(true); }); const invalidStrings = [undefined, null, 0, 1, [], {}]; @@ -109,8 +114,6 @@ describe('TYPED_MESSAGE_SCHEMA', () => { for (const invalidString of invalidStrings) { // eslint-disable-next-line no-loop-func it(`should disallow a primary type with value '${invalidString}'`, () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -120,7 +123,7 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); } @@ -128,8 +131,6 @@ describe('TYPED_MESSAGE_SCHEMA', () => { for (const invalidObject of invalidObjects) { // eslint-disable-next-line no-loop-func it(`should disallow a domain with value '${invalidObject}'`, () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: invalidObject, message: {}, @@ -139,13 +140,11 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); // eslint-disable-next-line no-loop-func it(`should disallow a message with value '${invalidObject}'`, () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: invalidObject, @@ -155,13 +154,11 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); // eslint-disable-next-line no-loop-func it(`should disallow types with value '${invalidObject}'`, () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -169,13 +166,11 @@ describe('TYPED_MESSAGE_SCHEMA', () => { types: invalidObject, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); } it('should require custom type properties to have a name', () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -186,12 +181,10 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); it('should require custom type properties to have a type', () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -202,7 +195,7 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); const invalidTypes = [undefined, null, 0, 1, [], {}]; @@ -210,8 +203,6 @@ describe('TYPED_MESSAGE_SCHEMA', () => { for (const invalidType of invalidTypes) { // eslint-disable-next-line no-loop-func it(`should disallow a type of '${invalidType}'`, () => { - const ajv = new Ajv(); - const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); const typedMessage = { domain: {}, message: {}, @@ -222,7 +213,7 @@ describe('TYPED_MESSAGE_SCHEMA', () => { }, }; - expect(validate(typedMessage)).toBe(false); + expect(validateTypedMessageSchema(typedMessage)).toBe(false); }); } }); From 08b3b309198038fc1e123db764d6539a21bde0ab Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 20 Apr 2022 18:39:04 -0230 Subject: [PATCH 3/3] Add more invalid object tests --- src/sign-typed-data.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/sign-typed-data.test.ts b/src/sign-typed-data.test.ts index 2cedcf60..af858e97 100644 --- a/src/sign-typed-data.test.ts +++ b/src/sign-typed-data.test.ts @@ -129,6 +129,11 @@ describe('TYPED_MESSAGE_SCHEMA', () => { const invalidObjects = [undefined, null, 0, 1, [], '', 'test']; for (const invalidObject of invalidObjects) { + // eslint-disable-next-line no-loop-func + it(`should disallow a typed message with value'${invalidObject}'`, () => { + expect(validateTypedMessageSchema(invalidObject as any)).toBe(false); + }); + // eslint-disable-next-line no-loop-func it(`should disallow a domain with value '${invalidObject}'`, () => { const typedMessage = {