From f195c16f9216d8294098770fd3f3975ee9d0edcc Mon Sep 17 00:00:00 2001 From: jpuri Date: Mon, 16 Dec 2024 16:02:17 +0530 Subject: [PATCH 1/7] Add validation for types sign message primary type --- src/utils/common.test.ts | 13 ++++++++++ src/utils/common.ts | 6 +++++ src/wallet.test.ts | 54 ++++++++++++++++++++++++++++++++++++++++ src/wallet.ts | 24 ++++++++++++++++++ 4 files changed, 97 insertions(+) create mode 100644 src/utils/common.test.ts create mode 100644 src/utils/common.ts diff --git a/src/utils/common.test.ts b/src/utils/common.test.ts new file mode 100644 index 00000000..c2dd1909 --- /dev/null +++ b/src/utils/common.test.ts @@ -0,0 +1,13 @@ +import { stripArrayTypeIfPresent } from './common'; + +describe('CommonUtils', () => { + describe('stripArrayTypeIfPresent', () => { + it('remove array brackets from the type if present', () => { + expect(stripArrayTypeIfPresent('string[]')).toBe('string'); + }); + + it('return types which are not array without any change', () => { + expect(stripArrayTypeIfPresent('string')).toBe('string'); + }); + }); +}); diff --git a/src/utils/common.ts b/src/utils/common.ts new file mode 100644 index 00000000..648e82ae --- /dev/null +++ b/src/utils/common.ts @@ -0,0 +1,6 @@ +export const stripArrayTypeIfPresent = (typeString: string) => { + if(typeString && typeString.match(/\S\[\]$/u) !== null) { + return typeString.replace(/\[\]$/gu, '').trim(); + } + return typeString; +} diff --git a/src/wallet.test.ts b/src/wallet.test.ts index 27cd1fa8..fe5f1e68 100644 --- a/src/wallet.test.ts +++ b/src/wallet.test.ts @@ -626,6 +626,60 @@ describe('wallet', () => { '0x68dc980608bceb5f99f691e62c32caccaee05317309015e9454eba1a14c3cd4505d1dd098b8339801239c9bcaac3c4df95569dcf307108b92f68711379be14d81c', }); }); + + it('should throw if message does not have types defined', async () => { + const { engine } = createTestSetup(); + const getAccounts = async () => testAddresses.slice(); + const witnessedMsgParams: TypedMessageParams[] = []; + const processTypedMessageV4 = async (msgParams: TypedMessageParams) => { + witnessedMsgParams.push(msgParams); + // Assume testMsgSig is the expected signature result + return testMsgSig; + }; + + engine.push( + createWalletMiddleware({ getAccounts, processTypedMessageV4 }), + ); + + const messageParams = getMsgParams(); + const payload = { + method: 'eth_signTypedData_v4', + params: [ + testAddresses[0], + JSON.stringify({...messageParams, types: undefined}), + ], + }; + + const promise = pify(engine.handle).call(engine, payload); + await expect(promise).rejects.toThrow('Invalid input.'); + }); + + it('should throw if type of primaryType is not defined', async () => { + const { engine } = createTestSetup(); + const getAccounts = async () => testAddresses.slice(); + const witnessedMsgParams: TypedMessageParams[] = []; + const processTypedMessageV4 = async (msgParams: TypedMessageParams) => { + witnessedMsgParams.push(msgParams); + // Assume testMsgSig is the expected signature result + return testMsgSig; + }; + + engine.push( + createWalletMiddleware({ getAccounts, processTypedMessageV4 }), + ); + + const messageParams = getMsgParams(); + const payload = { + method: 'eth_signTypedData_v4', + params: [ + testAddresses[0], + JSON.stringify({...messageParams, types: {...messageParams.types, Permit: undefined}}), + ], + }; + + const promise = pify(engine.handle).call(engine, payload); + await expect(promise).rejects.toThrow('Invalid input.'); + }); }); describe('sign', () => { diff --git a/src/wallet.ts b/src/wallet.ts index e24f4086..936c9418 100644 --- a/src/wallet.ts +++ b/src/wallet.ts @@ -13,6 +13,7 @@ import { } from '@metamask/utils'; import type { Block } from './types'; +import { stripArrayTypeIfPresent } from './utils/common'; import { normalizeTypedMessage, parseTypedMessage } from './utils/normalize'; /* @@ -243,6 +244,7 @@ WalletMiddlewareOptions): JsonRpcMiddleware { const address = await validateAndNormalizeKeyholder(params[0], req); const message = normalizeTypedMessage(params[1]); + validatePrimaryType(message); validateVerifyingContract(message); const version = 'V3'; const msgParams: TypedMessageParams = { @@ -274,6 +276,7 @@ WalletMiddlewareOptions): JsonRpcMiddleware { const address = await validateAndNormalizeKeyholder(params[0], req); const message = normalizeTypedMessage(params[1]); + validatePrimaryType(message); validateVerifyingContract(message); const version = 'V4'; const msgParams: TypedMessageParams = { @@ -457,6 +460,27 @@ WalletMiddlewareOptions): JsonRpcMiddleware { } } +/** + * Validates primary of typedSignMessage, to ensure that it's type definition is present in message. + * + * @param data - The data passed in typedSign request. + */ +function validatePrimaryType(data: string) { + const { primaryType, types } = parseTypedMessage(data); + if (!types) { + throw rpcErrors.invalidInput(); + } + + // Primary type can be an array. + const baseType = stripArrayTypeIfPresent(primaryType) + + // Return if the base type is not defined in the types + const baseTypeDefinitions = types[baseType]; + if (!baseTypeDefinitions) { + throw rpcErrors.invalidInput(); + } +} + /** * Validates verifyingContract of typedSignMessage. * From f4cb3b3b234740aa06d25a28a2db5a20a20dd8ef Mon Sep 17 00:00:00 2001 From: jpuri Date: Mon, 16 Dec 2024 16:33:09 +0530 Subject: [PATCH 2/7] update --- src/utils/common.ts | 4 ++-- src/wallet.test.ts | 7 +++++-- src/wallet.ts | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/utils/common.ts b/src/utils/common.ts index 648e82ae..867b6aff 100644 --- a/src/utils/common.ts +++ b/src/utils/common.ts @@ -1,6 +1,6 @@ export const stripArrayTypeIfPresent = (typeString: string) => { - if(typeString && typeString.match(/\S\[\]$/u) !== null) { + if (typeString && typeString.match(/\S\[\]$/u) !== null) { return typeString.replace(/\[\]$/gu, '').trim(); } return typeString; -} +}; diff --git a/src/wallet.test.ts b/src/wallet.test.ts index fe5f1e68..4fab72f2 100644 --- a/src/wallet.test.ts +++ b/src/wallet.test.ts @@ -646,7 +646,7 @@ describe('wallet', () => { method: 'eth_signTypedData_v4', params: [ testAddresses[0], - JSON.stringify({...messageParams, types: undefined}), + JSON.stringify({ ...messageParams, types: undefined }), ], }; @@ -673,7 +673,10 @@ describe('wallet', () => { method: 'eth_signTypedData_v4', params: [ testAddresses[0], - JSON.stringify({...messageParams, types: {...messageParams.types, Permit: undefined}}), + JSON.stringify({ + ...messageParams, + types: { ...messageParams.types, Permit: undefined }, + }), ], }; diff --git a/src/wallet.ts b/src/wallet.ts index 936c9418..5216a89a 100644 --- a/src/wallet.ts +++ b/src/wallet.ts @@ -472,7 +472,7 @@ function validatePrimaryType(data: string) { } // Primary type can be an array. - const baseType = stripArrayTypeIfPresent(primaryType) + const baseType = stripArrayTypeIfPresent(primaryType); // Return if the base type is not defined in the types const baseTypeDefinitions = types[baseType]; From 590b519c62fbe7f33a94fe0f920e2072c4f33d76 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Tue, 17 Dec 2024 14:55:14 +0530 Subject: [PATCH 3/7] Update src/utils/common.ts Co-authored-by: Mark Stacey --- src/utils/common.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/common.ts b/src/utils/common.ts index 867b6aff..767405dc 100644 --- a/src/utils/common.ts +++ b/src/utils/common.ts @@ -1,5 +1,5 @@ export const stripArrayTypeIfPresent = (typeString: string) => { - if (typeString && typeString.match(/\S\[\]$/u) !== null) { + if (typeString?.match(/\S\[\]$/u) !== null) { return typeString.replace(/\[\]$/gu, '').trim(); } return typeString; From d2704f6e770ef488920e4683054b04636e23369b Mon Sep 17 00:00:00 2001 From: jpuri Date: Tue, 17 Dec 2024 15:15:09 +0530 Subject: [PATCH 4/7] update --- src/utils/common.test.ts | 1 + src/utils/common.ts | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/utils/common.test.ts b/src/utils/common.test.ts index c2dd1909..a3c425a4 100644 --- a/src/utils/common.test.ts +++ b/src/utils/common.test.ts @@ -4,6 +4,7 @@ describe('CommonUtils', () => { describe('stripArrayTypeIfPresent', () => { it('remove array brackets from the type if present', () => { expect(stripArrayTypeIfPresent('string[]')).toBe('string'); + expect(stripArrayTypeIfPresent('string []')).toBe('string'); }); it('return types which are not array without any change', () => { diff --git a/src/utils/common.ts b/src/utils/common.ts index 867b6aff..938d6d8b 100644 --- a/src/utils/common.ts +++ b/src/utils/common.ts @@ -1,6 +1,12 @@ +/** + * Function to stripe array brackets if string defining the type has it. + * + * @param typeString - String defining type from which array brackets are required to be removed. + * @returns Parameter string with array brackets [] removed. + */ export const stripArrayTypeIfPresent = (typeString: string) => { - if (typeString && typeString.match(/\S\[\]$/u) !== null) { - return typeString.replace(/\[\]$/gu, '').trim(); + if (typeString?.match(/\S\s*\[\]$/u) !== null) { + return typeString.replace(/\s*\[\]$/gu, '').trim(); } return typeString; }; From f98312448ce08838fc395f25dcd5a0dd29ffa40f Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 18 Dec 2024 20:02:06 +0530 Subject: [PATCH 5/7] Update src/utils/common.ts Co-authored-by: Mark Stacey --- src/utils/common.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/common.ts b/src/utils/common.ts index 938d6d8b..c8baa98e 100644 --- a/src/utils/common.ts +++ b/src/utils/common.ts @@ -5,8 +5,8 @@ * @returns Parameter string with array brackets [] removed. */ export const stripArrayTypeIfPresent = (typeString: string) => { - if (typeString?.match(/\S\s*\[\]$/u) !== null) { - return typeString.replace(/\s*\[\]$/gu, '').trim(); + if (typeString?.match(/\S\[\]$/u) !== null) { + return typeString.replace(/\[\]$/gu, '').trim(); } return typeString; }; From 45b115ba71bb6d19762b895c1622c751c0d6db04 Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 18 Dec 2024 20:03:20 +0530 Subject: [PATCH 6/7] update --- src/utils/common.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/common.test.ts b/src/utils/common.test.ts index a3c425a4..d89cd793 100644 --- a/src/utils/common.test.ts +++ b/src/utils/common.test.ts @@ -4,11 +4,11 @@ describe('CommonUtils', () => { describe('stripArrayTypeIfPresent', () => { it('remove array brackets from the type if present', () => { expect(stripArrayTypeIfPresent('string[]')).toBe('string'); - expect(stripArrayTypeIfPresent('string []')).toBe('string'); }); it('return types which are not array without any change', () => { expect(stripArrayTypeIfPresent('string')).toBe('string'); + expect(stripArrayTypeIfPresent('string []')).toBe('string []'); }); }); }); From 04c35242e0b2aed369b524d82495f396b7bb850a Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 18 Dec 2024 20:12:20 +0530 Subject: [PATCH 7/7] update --- src/utils/common.test.ts | 1 + src/utils/common.ts | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/utils/common.test.ts b/src/utils/common.test.ts index d89cd793..ba583226 100644 --- a/src/utils/common.test.ts +++ b/src/utils/common.test.ts @@ -4,6 +4,7 @@ describe('CommonUtils', () => { describe('stripArrayTypeIfPresent', () => { it('remove array brackets from the type if present', () => { expect(stripArrayTypeIfPresent('string[]')).toBe('string'); + expect(stripArrayTypeIfPresent('string[5]')).toBe('string'); }); it('return types which are not array without any change', () => { diff --git a/src/utils/common.ts b/src/utils/common.ts index c8baa98e..ae1b297c 100644 --- a/src/utils/common.ts +++ b/src/utils/common.ts @@ -5,8 +5,8 @@ * @returns Parameter string with array brackets [] removed. */ export const stripArrayTypeIfPresent = (typeString: string) => { - if (typeString?.match(/\S\[\]$/u) !== null) { - return typeString.replace(/\[\]$/gu, '').trim(); + if (typeString?.match(/\S\[\d*\]$/u) !== null) { + return typeString.replace(/\[\d*\]$/gu, '').trim(); } return typeString; };