-
Notifications
You must be signed in to change notification settings - Fork 1
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 CAIP-122 verification #193
Conversation
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.
Generated
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.
Generated
@@ -4,7 +4,7 @@ | |||
"encodedValue": "f6akufkq9Lex6rT8RCEDRuoZQRgo5pWiRzeo81nmKNGWGNJdJ", | |||
"encoding": "base58", | |||
"format": "ss58", | |||
"type": "SR25519" | |||
"type": "Sr25519" |
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.
Switch is due to how FA is doing things. No really matter either way
@@ -20,14 +21,17 @@ interface SiwxMessage { | |||
nonce: string; | |||
expired: boolean; | |||
issuedAt: Date; | |||
expirationTime: Date; | |||
expirationTime?: Date; |
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.
Not required
@@ -54,6 +58,36 @@ function parseMessage(message: string): SiwxMessage { | |||
}; | |||
} | |||
|
|||
// SIWA is switching away from this, but we should still support it for now | |||
function verifySignatureHashMaybeWrapped(publicKey: string, signature: string, message: Uint8Array): boolean { | |||
const unwrappedSigned = message.length > 256 ? blake2AsU8a(message) : message; |
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.
This is from how the Polkadot-SDK signs payloads. We might want to require it, but for now we support it either way.
@@ -84,4 +84,11 @@ describe('validateSiwfResponse', () => { | |||
'Response failed to correctly parse or invalid content: {"foo":"bad"}' | |||
); | |||
}); | |||
|
|||
it('throws on a bad domain', async () => { |
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.
Extra test
|
||
// Support both wrapped and unwrapped signatures | ||
const wrappedSignedBytes = u8aWrapBytes(message); | ||
const wrappedSigned = wrappedSignedBytes.length > 256 ? blake2AsU8a(wrappedSignedBytes) : wrappedSignedBytes; |
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.
👍
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.
👍 great!
Problem
Various issues found when testing the CAIP-122 signature verification
Solution
publicKey
should beSr
notSR
(verification doesn't care however)