-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Request validation should not throw if verifyingContract is not defined in typed signature #328
Changes from all commits
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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -432,6 +432,50 @@ describe('wallet', () => { | |||||||||
const promise = pify(engine.handle).call(engine, payload); | ||||||||||
await expect(promise).rejects.toThrow('Invalid input.'); | ||||||||||
}); | ||||||||||
|
||||||||||
it('should not throw if verifyingContract is undefined', async () => { | ||||||||||
const { engine } = createTestSetup(); | ||||||||||
const getAccounts = async () => testAddresses.slice(); | ||||||||||
const witnessedMsgParams: TypedMessageParams[] = []; | ||||||||||
const processTypedMessageV3 = async (msgParams: TypedMessageParams) => { | ||||||||||
witnessedMsgParams.push(msgParams); | ||||||||||
Comment on lines
+439
to
+441
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. Nit: This variable seems to serve no purpose:
Suggested change
|
||||||||||
// Assume testMsgSig is the expected signature result | ||||||||||
return testMsgSig; | ||||||||||
}; | ||||||||||
|
||||||||||
engine.push( | ||||||||||
createWalletMiddleware({ getAccounts, processTypedMessageV3 }), | ||||||||||
); | ||||||||||
|
||||||||||
const message = { | ||||||||||
types: { | ||||||||||
EIP712Domain: [ | ||||||||||
{ name: 'name', type: 'string' }, | ||||||||||
{ name: 'version', type: 'string' }, | ||||||||||
{ name: 'chainId', type: 'uint256' }, | ||||||||||
{ name: 'verifyingContract', type: 'address' }, | ||||||||||
], | ||||||||||
}, | ||||||||||
primaryType: 'EIP712Domain', | ||||||||||
message: {}, | ||||||||||
}; | ||||||||||
|
||||||||||
const stringifiedMessage = JSON.stringify(message); | ||||||||||
|
||||||||||
const payload = { | ||||||||||
method: 'eth_signTypedData_v3', | ||||||||||
params: [testAddresses[0], stringifiedMessage], // Assuming testAddresses[0] is a valid address from your setup | ||||||||||
}; | ||||||||||
|
||||||||||
const promise = pify(engine.handle).call(engine, payload); | ||||||||||
const result = await promise; | ||||||||||
Comment on lines
+470
to
+471
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. Nit: Unnecessary deferred Promise:
Suggested change
|
||||||||||
expect(result).toStrictEqual({ | ||||||||||
id: undefined, | ||||||||||
jsonrpc: undefined, | ||||||||||
result: | ||||||||||
'0x68dc980608bceb5f99f691e62c32caccaee05317309015e9454eba1a14c3cd4505d1dd098b8339801239c9bcaac3c4df95569dcf307108b92f68711379be14d81c', | ||||||||||
}); | ||||||||||
}); | ||||||||||
}); | ||||||||||
|
||||||||||
describe('signTypedDataV4', () => { | ||||||||||
|
@@ -524,6 +568,35 @@ describe('wallet', () => { | |||||||||
const promise = pify(engine.handle).call(engine, payload); | ||||||||||
await expect(promise).rejects.toThrow('Invalid input.'); | ||||||||||
}); | ||||||||||
|
||||||||||
it('should not throw if request is permit with undefined value for verifyingContract address', async () => { | ||||||||||
const { engine } = createTestSetup(); | ||||||||||
const getAccounts = async () => testAddresses.slice(); | ||||||||||
const witnessedMsgParams: TypedMessageParams[] = []; | ||||||||||
const processTypedMessageV4 = async (msgParams: TypedMessageParams) => { | ||||||||||
witnessedMsgParams.push(msgParams); | ||||||||||
Comment on lines
+575
to
+577
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. Nit: This variable isn't used either
Suggested change
|
||||||||||
// Assume testMsgSig is the expected signature result | ||||||||||
return testMsgSig; | ||||||||||
}; | ||||||||||
|
||||||||||
engine.push( | ||||||||||
createWalletMiddleware({ getAccounts, processTypedMessageV4 }), | ||||||||||
); | ||||||||||
|
||||||||||
const payload = { | ||||||||||
method: 'eth_signTypedData_v4', | ||||||||||
params: [testAddresses[0], JSON.stringify(getMsgParams())], | ||||||||||
}; | ||||||||||
|
||||||||||
const promise = pify(engine.handle).call(engine, payload); | ||||||||||
const result = await promise; | ||||||||||
Comment on lines
+591
to
+592
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. Nit: Unnecessary deferred Promise:
Suggested change
|
||||||||||
expect(result).toStrictEqual({ | ||||||||||
id: undefined, | ||||||||||
jsonrpc: undefined, | ||||||||||
result: | ||||||||||
'0x68dc980608bceb5f99f691e62c32caccaee05317309015e9454eba1a14c3cd4505d1dd098b8339801239c9bcaac3c4df95569dcf307108b92f68711379be14d81c', | ||||||||||
}); | ||||||||||
}); | ||||||||||
}); | ||||||||||
|
||||||||||
describe('sign', () => { | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -426,7 +426,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 429 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (18.x)
Check warning on line 429 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (20.x)
Check warning on line 429 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (22.x)
|
||
* an error | ||
*/ | ||
async function validateAndNormalizeKeyholder( | ||
|
@@ -463,10 +463,8 @@ | |
* @param data - The data passed in typedSign request. | ||
*/ | ||
function validateVerifyingContract(data: string) { | ||
const { | ||
domain: { verifyingContract }, | ||
} = parseTypedMessage(data); | ||
if (!isValidHexAddress(verifyingContract)) { | ||
const { domain: { verifyingContract } = {} } = parseTypedMessage(data); | ||
if (verifyingContract && !isValidHexAddress(verifyingContract)) { | ||
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. Does it matter if 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. An empty value will not throw error. 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 think @jiexi meant to ask should it throw an error. I've just tested with v12.0.6 and it looks like an empty |
||
throw rpcErrors.invalidInput(); | ||
} | ||
} | ||
|
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 we're testing the case where the
domain
is in the type definition given for the message, but missing from the message itself. It would also be useful to test these cases:verifyingContract
, anddomain
is missing from the messageverifyingContract
, anddomain
is included in the message, butverifyingContract
is missing