Check against the inbox id derived against the checksum as well #1268
Conversation
codabrink
changed the title
There was a problem hiding this comment.
I think this makes the most sense to continue to support legacy addresses created from a checksum address. Would be good to know where these checksum inbox Ids are still being created. I'll do another pass on lowercasing in the sdk when I push this fix out through the sdks. And @rygine maybe you can as well whenever you push this out through node and web?
|
@nplasterer i reviewed the WASM/node bindings, and we're already lowercasing where necessary. 🎉 |
| @@ -381,6 +381,7 @@ impl IdentityAction for IdentityUpdate { | |||
| } | |||
|
|
|||
| let new_state = state.ok_or(AssociationError::NotCreated)?; | |||
| // Do we need to handle this case for checksum inboxIds? | |||
There was a problem hiding this comment.
@insipx I could hack some code here to check for mismatched inboxIds from checksums here but it's a lot less straight forward thoughts on this case?
| use sha2::{Digest, Sha256}; | ||
|
|
||
| use super::AssociationError; | ||
|
|
||
| /// Helper function to generate a SHA256 hash as a hex string. | ||
| fn sha256_string(input: String) -> String { | ||
| let normalized_input = input.to_lowercase(); |
There was a problem hiding this comment.
If we want to do this here we should rename the function. Will be a footgun to change the string before hashing, since callers won't expect that.
| @@ -104,7 +103,8 @@ impl IdentityStrategy { | |||
| legacy_signed_private_key, | |||
| ) => { | |||
| if let Some(stored_identity) = stored_identity { | |||
| if inbox_id != stored_identity.inbox_id { | |||
| let checksum_inbox_id = generate_inbox_id_from_checksum(&address, &nonce)?; | |||
There was a problem hiding this comment.
If the inbox ID was created with the checksum address, and not the lowercased one, it shouldn't be able to create a valid Create Inbox identity action
There was a problem hiding this comment.
We check that the inbox ID matches the signer as part of verifying that Identity Action, so it should blow up there.
So even if your client "works", everyone else would reject you
|
How many people do we think this issue is affecting? I'd much rather force them into creating a new identity and shed tech debt rather than having to support this bug forever. |
|
The only real person in particular that we know this is effecting is @rygine. But I think a lot of bot accounts and other fresh accounts @humanagent was testing with were also failing but those I think are less important. |
Fixes xmtp/xmtp-react-native#512
It appears that our SDKs at some point allowed or still allow checksum addresses to create inboxIds. Now we have inboxIds on the network that throw a mismatch id error because we handle most addresses as lowercase.
This is supposed to prevent any checksum inboxIds from being created ever again and help unblock users with inboxIds that were created from checksum addresses.