-
-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Add validation for types sign message primary type #350
Changes from all commits
f195c16
f4cb3b3
590b519
d2704f6
d0495dc
f983124
45b115b
04c3524
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { stripArrayTypeIfPresent } from './common'; | ||
|
||
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', () => { | ||
expect(stripArrayTypeIfPresent('string')).toBe('string'); | ||
expect(stripArrayTypeIfPresent('string []')).toBe('string []'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +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) => { | ||
Gudahtt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (typeString?.match(/\S\[\d*\]$/u) !== null) { | ||
return typeString.replace(/\[\d*\]$/gu, '').trim(); | ||
} | ||
return typeString; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
} from '@metamask/utils'; | ||
|
||
import type { Block } from './types'; | ||
import { stripArrayTypeIfPresent } from './utils/common'; | ||
import { normalizeTypedMessage, parseTypedMessage } from './utils/normalize'; | ||
|
||
/* | ||
|
@@ -243,6 +244,7 @@ | |
|
||
const address = await validateAndNormalizeKeyholder(params[0], req); | ||
const message = normalizeTypedMessage(params[1]); | ||
validatePrimaryType(message); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not want to validate this before signing process? Like we did it in the If this is a malformed signature and not applying the rules of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example if we change
This schema will be validated before signature is getting added in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we are doing same, rejecting before request reaches extension. Rejecting in RPC middleware. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, in this PR the validation is happening before the user is shown the request. Though I do wonder whether we should be using a schema instead of manually validating each expectation we have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with both approaches, just wanted to highlight that schema validation is also happening today. On both approaches, I think it make sense to centralise them in one place, do it on the middleware functions make sense as this PR do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree @OGPoyraz , but schema validation is currently happening in SignatureController, may be we move it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, there are multiple tasks in the backlog for that, we will handle that next year |
||
validateVerifyingContract(message); | ||
const version = 'V3'; | ||
const msgParams: TypedMessageParams = { | ||
|
@@ -274,6 +276,7 @@ | |
|
||
const address = await validateAndNormalizeKeyholder(params[0], req); | ||
const message = normalizeTypedMessage(params[1]); | ||
validatePrimaryType(message); | ||
validateVerifyingContract(message); | ||
const version = 'V4'; | ||
const msgParams: TypedMessageParams = { | ||
|
@@ -426,7 +429,7 @@ | |
* | ||
* @param address - The address to validate and normalize. | ||
* @param req - The request object. | ||
* @returns {string} - The normalized address, if valid. Otherwise, throws | ||
Check warning on line 432 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (18.x)
Check warning on line 432 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (20.x)
Check warning on line 432 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (22.x)
|
||
* an error | ||
*/ | ||
async function validateAndNormalizeKeyholder( | ||
|
@@ -457,6 +460,27 @@ | |
} | ||
} | ||
|
||
/** | ||
* 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. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It looks like
stripArrayTypeIfPresent
will ignore a case likestring []
, where there is a space before[]
. Perhaps we can add a test case for that as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was correct before. You've now updated it to allow
Type []
, which is unspecified behaviour (the spec only referencesType
orType[]
). I didn't mean to suggest you update the regex, just that there was a test case missingFrom https://eips.ethereum.org/EIPS/eip-712:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code and also changed regex to handle fixed size array like
string[5]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, never thought about fixed-sized arrays, and they were right in the quote. Good catch