Enforce 42 characters for address on inboxId create (#1202)

* enforce 42 characters for address * add the error * update all of the tests * try and get closer to a solution * simple fix * one more error fix * fix up all the tests * fmt * fix the lint * fix another lint issue * update the test * cargo fmt * fix up the test * fix up another test * more test fixing
xmtp · Nov 1, 2024 · 71b47a2 · 71b47a2
1 parent c1066eb
commit 71b47a2
Show file tree

Hide file tree

Showing 20 changed files with 136 additions and 82 deletions.
diff --git a/bindings_ffi/src/lib.rs b/bindings_ffi/src/lib.rs
@@ -44,6 +44,8 @@ pub enum GenericError {
     Verifier(#[from] xmtp_id::scw_verifier::VerifierError),
     #[error("Failed to convert to u32")]
     FailedToConvertToU32,
+    #[error("Association error: {0}")]
+    Association(#[from] xmtp_id::associations::AssociationError),
     #[error(transparent)]
     DeviceSync(#[from] xmtp_mls::groups::device_sync::DeviceSyncError),
 }

diff --git a/bindings_ffi/src/mls.rs b/bindings_ffi/src/mls.rs
@@ -170,8 +170,8 @@ pub async fn get_inbox_id_for_address(
 
 #[allow(unused)]
 #[uniffi::export]
-pub fn generate_inbox_id(account_address: String, nonce: u64) -> String {
-    xmtp_id_generate_inbox_id(&account_address, &nonce)
+pub fn generate_inbox_id(account_address: String, nonce: u64) -> Result<String, GenericError> {
+    Ok(xmtp_id_generate_inbox_id(&account_address, &nonce)?)
 }
 
 #[derive(uniffi::Object)]
@@ -1817,7 +1817,7 @@ mod tests {
     ) -> Arc<FfiXmtpClient> {
         let ffi_inbox_owner = LocalWalletInboxOwner::with_wallet(wallet);
         let nonce = 1;
-        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce);
+        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce).unwrap();
 
         let client = create_client(
             Box::new(MockLogger {}),
@@ -1875,7 +1875,7 @@ mod tests {
         let account_address = "0x0bD00B21aF9a2D538103c3AAf95Cb507f8AF1B28".to_lowercase();
         let legacy_keys = hex::decode("0880bdb7a8b3f6ede81712220a20ad528ea38ce005268c4fb13832cfed13c2b2219a378e9099e48a38a30d66ef991a96010a4c08aaa8e6f5f9311a430a41047fd90688ca39237c2899281cdf2756f9648f93767f91c0e0f74aed7e3d3a8425e9eaa9fa161341c64aa1c782d004ff37ffedc887549ead4a40f18d1179df9dff124612440a403c2cb2338fb98bfe5f6850af11f6a7e97a04350fc9d37877060f8d18e8f66de31c77b3504c93cf6a47017ea700a48625c4159e3f7e75b52ff4ea23bc13db77371001").unwrap();
         let nonce = 0;
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
 
         let client = create_client(
             Box::new(MockLogger {}),
@@ -1899,7 +1899,7 @@ mod tests {
     async fn test_create_client_with_storage() {
         let ffi_inbox_owner = LocalWalletInboxOwner::new();
         let nonce = 1;
-        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce);
+        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce).unwrap();
 
         let path = tmp_path();
 
@@ -1950,7 +1950,7 @@ mod tests {
     async fn test_create_client_with_key() {
         let ffi_inbox_owner = LocalWalletInboxOwner::new();
         let nonce = 1;
-        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce);
+        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce).unwrap();
 
         let path = tmp_path();
 
@@ -2024,7 +2024,7 @@ mod tests {
         // Setup the initial first client
         let ffi_inbox_owner = LocalWalletInboxOwner::new();
         let nonce = 1;
-        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce);
+        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce).unwrap();
 
         let path = tmp_path();
         let key = static_enc_key().to_vec();
@@ -2090,7 +2090,7 @@ mod tests {
         // Setup the initial first client
         let ffi_inbox_owner = LocalWalletInboxOwner::new();
         let nonce = 1;
-        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce);
+        let inbox_id = generate_inbox_id(&ffi_inbox_owner.get_address(), &nonce).unwrap();
 
         let path = tmp_path();
         let key = static_enc_key().to_vec();
@@ -2176,7 +2176,7 @@ mod tests {
     async fn test_invalid_external_signature() {
         let inbox_owner = LocalWalletInboxOwner::new();
         let nonce = 1;
-        let inbox_id = generate_inbox_id(&inbox_owner.get_address(), &nonce);
+        let inbox_id = generate_inbox_id(&inbox_owner.get_address(), &nonce).unwrap();
         let path = tmp_path();
 
         let client = create_client(
@@ -2202,9 +2202,9 @@ mod tests {
     async fn test_can_message() {
         let amal = LocalWalletInboxOwner::new();
         let nonce = 1;
-        let amal_inbox_id = generate_inbox_id(&amal.get_address(), &nonce);
+        let amal_inbox_id = generate_inbox_id(&amal.get_address(), &nonce).unwrap();
         let bola = LocalWalletInboxOwner::new();
-        let bola_inbox_id = generate_inbox_id(&bola.get_address(), &nonce);
+        let bola_inbox_id = generate_inbox_id(&bola.get_address(), &nonce).unwrap();
         let path = tmp_path();
 
         let client_amal = create_client(

diff --git a/bindings_node/src/inbox_id.rs b/bindings_node/src/inbox_id.rs
@@ -30,9 +30,10 @@ pub async fn get_inbox_id_for_address(
 }
 
 #[napi]
-pub fn generate_inbox_id(account_address: String) -> String {
+pub fn generate_inbox_id(account_address: String) -> Result<Option<String>> {
   let account_address = account_address.to_lowercase();
   // ensure that the nonce is always 1 for now since this will only be used for the
   // create_client function above, which also has a hard-coded nonce of 1
-  xmtp_id_generate_inbox_id(&account_address, &1)
+  let result = xmtp_id_generate_inbox_id(&account_address, &1).map_err(ErrorWrapper::from)?;
+  Ok(Some(result))
 }
diff --git a/bindings_wasm/src/inbox_id.rs b/bindings_wasm/src/inbox_id.rs
@@ -24,9 +24,11 @@ pub async fn get_inbox_id_for_address(
 }
 
 #[wasm_bindgen(js_name = generateInboxId)]
-pub fn generate_inbox_id(account_address: String) -> String {
+pub fn generate_inbox_id(account_address: String) -> Result<String, JsError> {
   let account_address = account_address.to_lowercase();
   // ensure that the nonce is always 1 for now since this will only be used for the
   // create_client function above, which also has a hard-coded nonce of 1
-  xmtp_id_generate_inbox_id(&account_address, &1)
+  let result = xmtp_id_generate_inbox_id(&account_address, &1)
+    .map_err(|e| JsError::new(format!("{}", e).as_str()))?;
+  Ok(result)
 }
diff --git a/examples/cli/cli-client.rs b/examples/cli/cli-client.rs
@@ -37,7 +37,7 @@ use xmtp_cryptography::{
     signature::{RecoverableSignature, SignatureError},
     utils::rng,
 };
-use xmtp_id::associations::generate_inbox_id;
+use xmtp_id::associations::{generate_inbox_id, AssociationError};
 use xmtp_mls::{
     builder::ClientBuilderError,
     client::ClientError,
@@ -141,6 +141,8 @@ enum CliError {
     StorageError(#[from] StorageError),
     #[error("generic:{0}")]
     Generic(String),
+    #[error(transparent)]
+    Association(#[from] AssociationError),
 }
 
 impl From<String> for CliError {
@@ -506,7 +508,7 @@ where
     };
 
     let nonce = 0;
-    let inbox_id = generate_inbox_id(&w.get_address(), &nonce);
+    let inbox_id = generate_inbox_id(&w.get_address(), &nonce)?;
     let client = create_client(
         cli,
         IdentityStrategy::CreateIfNotFound(inbox_id, w.get_address(), nonce, None),

diff --git a/mls_validation_service/src/handlers.rs b/mls_validation_service/src/handlers.rs
@@ -362,7 +362,7 @@ mod tests {
         let wallet = LocalWallet::new(&mut rand::thread_rng());
         let address = format!("0x{}", hex::encode(wallet.address()));
 
-        let inbox_id = generate_inbox_id(&address, &0);
+        let inbox_id = generate_inbox_id(&address, &0).unwrap();
 
         (inbox_id, signing_key)
     }
@@ -413,7 +413,7 @@ mod tests {
     async fn test_get_association_state() {
         let account_address = rand_string();
         let nonce = rand_u64();
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
         let update = UnverifiedIdentityUpdate::new_test(
             vec![UnverifiedAction::new_test_create_inbox(
                 &account_address,

diff --git a/xmtp_id/src/associations/association_log.rs b/xmtp_id/src/associations/association_log.rs
@@ -36,6 +36,8 @@ pub enum AssociationError {
     MissingIdentityUpdate,
     #[error("Wrong chain id. Initially added with {0} but now signing from {1}")]
     ChainIdMismatch(u64, u64),
+    #[error("Invalid account address: Must be 42 hex characters, starting with '0x'.")]
+    InvalidAccountAddress,
 }
 
 pub trait IdentityAction: Send {
@@ -90,11 +92,11 @@ impl IdentityAction for CreateInbox {
             return Err(AssociationError::LegacySignatureReuse);
         }
 
-        Ok(AssociationState::new(
+        AssociationState::new(
             account_address,
             self.nonce,
             self.initial_address_signature.chain_id,
-        ))
+        )
     }
 
     fn signatures(&self) -> Vec<Vec<u8>> {
@@ -140,7 +142,7 @@ impl IdentityAction for AddAssociation {
             && existing_state.inbox_id().ne(&generate_inbox_id(
                 existing_member_identifier.to_string().as_str(),
                 &0,
-            ))
+            )?)
         {
             return Err(AssociationError::LegacySignatureReuse);
         }

diff --git a/xmtp_id/src/associations/builder.rs b/xmtp_id/src/associations/builder.rs
@@ -451,7 +451,7 @@ pub(crate) mod tests {
         let wallet = LocalWallet::new(&mut rand::thread_rng());
         let account_address = wallet.get_address();
         let nonce = 0;
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
 
         let mut signature_request = SignatureRequestBuilder::new(inbox_id)
             .create_inbox(account_address.into(), nonce)
@@ -474,7 +474,7 @@ pub(crate) mod tests {
         let account_address = wallet.get_address();
         let installation_key_id = installation_key.verifying_key().as_bytes().to_vec();
         let nonce = 0;
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
         let existing_member_identifier: MemberIdentifier = account_address.into();
         let new_member_identifier: MemberIdentifier = installation_key_id.into();
 
@@ -501,7 +501,7 @@ pub(crate) mod tests {
         let wallet = LocalWallet::new(&mut rand::thread_rng());
         let account_address = wallet.get_address();
         let nonce = 0;
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
         let existing_member_identifier: MemberIdentifier = account_address.clone().into();
 
         let mut signature_request = SignatureRequestBuilder::new(inbox_id)
@@ -524,9 +524,9 @@ pub(crate) mod tests {
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
     #[cfg_attr(not(target_arch = "wasm32"), tokio::test)]
     async fn attempt_adding_unknown_signer() {
-        let account_address = "account_address".to_string();
+        let account_address = "0x1234567890abcdef1234567890abcdef12345678".to_string();
         let nonce = 0;
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
         let mut signature_request = SignatureRequestBuilder::new(inbox_id)
             .create_inbox(account_address.into(), nonce)
             .build();

diff --git a/xmtp_id/src/associations/hashes.rs b/xmtp_id/src/associations/hashes.rs
@@ -1,12 +1,31 @@
 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 mut hasher = Sha256::new();
     hasher.update(input.as_bytes());
     let result = hasher.finalize();
     format!("{:x}", result)
 }
 
-pub fn generate_inbox_id(account_address: &str, nonce: &u64) -> String {
-    sha256_string(format!("{}{}", account_address.to_lowercase(), nonce))
+/// Validates that the account address is exactly 42 characters, starts with "0x",
+/// and contains only valid hex digits.
+fn is_valid_address(account_address: &str) -> bool {
+    account_address.len() == 42
+        && account_address.starts_with("0x")
+        && account_address[2..].chars().all(|c| c.is_ascii_hexdigit())
+}
+
+/// Generates an inbox ID if the account address is valid.
+pub fn generate_inbox_id(account_address: &str, nonce: &u64) -> Result<String, AssociationError> {
+    if !is_valid_address(account_address) {
+        return Err(AssociationError::InvalidAccountAddress);
+    }
+    Ok(sha256_string(format!(
+        "{}{}",
+        account_address.to_lowercase(),
+        nonce
+    )))
 }
diff --git a/xmtp_id/src/associations/mod.rs b/xmtp_id/src/associations/mod.rs
@@ -126,7 +126,8 @@ pub(crate) mod tests {
 
     pub fn new_test_inbox() -> AssociationState {
         let create_request = CreateInbox::default();
-        let inbox_id = generate_inbox_id(&create_request.account_address, &create_request.nonce);
+        let inbox_id =
+            generate_inbox_id(&create_request.account_address, &create_request.nonce).unwrap();
         let identity_update =
             IdentityUpdate::new_test(vec![Action::CreateInbox(create_request)], inbox_id);
 
@@ -159,7 +160,8 @@ pub(crate) mod tests {
     #[test]
     fn test_create_inbox() {
         let create_request = CreateInbox::default();
-        let inbox_id = generate_inbox_id(&create_request.account_address, &create_request.nonce);
+        let inbox_id =
+            generate_inbox_id(&create_request.account_address, &create_request.nonce).unwrap();
         let account_address = create_request.account_address.clone();
         let identity_update =
             IdentityUpdate::new_test(vec![Action::CreateInbox(create_request)], inbox_id.clone());
@@ -208,7 +210,7 @@ pub(crate) mod tests {
     fn create_and_add_together() {
         let create_action = CreateInbox::default();
         let account_address = create_action.account_address.clone();
-        let inbox_id = generate_inbox_id(&account_address, &create_action.nonce);
+        let inbox_id = generate_inbox_id(&account_address, &create_action.nonce).unwrap();
         let new_member_identifier: MemberIdentifier = rand_vec().into();
         let add_action = AddAssociation {
             existing_member_signature: VerifiedSignature::new(
@@ -254,7 +256,7 @@ pub(crate) mod tests {
                 None,
             ),
         };
-        let inbox_id = generate_inbox_id(&member_identifier.to_string(), &0);
+        let inbox_id = generate_inbox_id(&member_identifier.to_string(), &0).unwrap();
         let state = get_state(vec![IdentityUpdate::new_test(
             vec![Action::CreateInbox(create_action)],
             inbox_id.clone(),
@@ -393,7 +395,8 @@ pub(crate) mod tests {
     #[test]
     fn reject_if_signer_not_existing_member() {
         let create_inbox = CreateInbox::default();
-        let inbox_id = generate_inbox_id(&create_inbox.account_address, &create_inbox.nonce);
+        let inbox_id =
+            generate_inbox_id(&create_inbox.account_address, &create_inbox.nonce).unwrap();
         let create_request = Action::CreateInbox(create_inbox);
         // The default here will create an AddAssociation from a random wallet
         let update = Action::AddAssociation(AddAssociation {
@@ -665,7 +668,7 @@ pub(crate) mod tests {
 
         let initial_state = get_state(vec![IdentityUpdate::new_test(
             vec![Action::CreateInbox(action)],
-            generate_inbox_id(&signer, &0),
+            generate_inbox_id(&signer, &0).unwrap(),
         )])
         .expect("initial state should be OK");
 

diff --git a/xmtp_id/src/associations/serialization.rs b/xmtp_id/src/associations/serialization.rs
@@ -583,7 +583,7 @@ pub(crate) mod tests {
     fn test_round_trip_unverified() {
         let account_address = rand_string();
         let nonce = rand_u64();
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+        let inbox_id = generate_inbox_id(&account_address, &nonce).unwrap();
         let client_timestamp_ns = rand_u64();
         let signature_bytes = rand_vec();
 

diff --git a/xmtp_id/src/associations/state.rs b/xmtp_id/src/associations/state.rs
@@ -5,7 +5,9 @@
 
 use std::collections::{HashMap, HashSet};
 
-use super::{hashes::generate_inbox_id, member::Member, MemberIdentifier, MemberKind};
+use super::{
+    hashes::generate_inbox_id, member::Member, AssociationError, MemberIdentifier, MemberKind,
+};
 
 #[derive(Debug, Clone)]
 pub struct AssociationStateDiff {
@@ -179,16 +181,20 @@ impl AssociationState {
         }
     }
 
-    pub fn new(account_address: String, nonce: u64, chain_id: Option<u64>) -> Self {
-        let inbox_id = generate_inbox_id(&account_address, &nonce);
+    pub fn new(
+        account_address: String,
+        nonce: u64,
+        chain_id: Option<u64>,
+    ) -> Result<Self, AssociationError> {
+        let inbox_id = generate_inbox_id(&account_address, &nonce)?;
         let identifier = MemberIdentifier::Address(account_address.clone());
         let new_member = Member::new(identifier.clone(), None, None, chain_id);
-        Self {
+        Ok(Self {
             members: HashMap::from_iter([(identifier, new_member)]),
             seen_signatures: HashSet::new(),
             recovery_address: account_address.to_lowercase(),
             inbox_id,
-        }
+        })
     }
 }
 
@@ -204,7 +210,7 @@ pub(crate) mod tests {
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
     #[cfg_attr(not(target_arch = "wasm32"), test)]
     fn can_add_remove() {
-        let starting_state = AssociationState::new(rand_string(), 0, None);
+        let starting_state = AssociationState::new(rand_string(), 0, None).unwrap();
         let new_entity = Member::default();
         let with_add = starting_state.add(new_entity.clone());
         assert!(with_add.get(&new_entity.identifier).is_some());
@@ -214,7 +220,7 @@ pub(crate) mod tests {
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
     #[cfg_attr(not(target_arch = "wasm32"), test)]
     fn can_diff() {
-        let starting_state = AssociationState::new(rand_string(), 0, None);
+        let starting_state = AssociationState::new(rand_string(), 0, None).unwrap();
         let entity_1 = Member::default();
         let entity_2 = Member::default();
         let entity_3 = Member::default();