fix #154 (and fix #159 because that's a duplicate) (#165)

Author: dannym-arx

crates/mdk-core/CHANGELOG.md

@@ -25,6 +25,7 @@
 
 ### Added
 
+- **`PreviouslyFailed` Result Variant**: Added `MessageProcessingResult::PreviouslyFailed` variant to handle cases where a previously failed message arrives again but the MLS group ID cannot be extracted. This prevents crashes in client applications by returning a result instead of throwing an error. ([#165](https://github.com/marmot-protocol/mdk/pull/165), fixes [#154](https://github.com/marmot-protocol/mdk/issues/154), [#159](https://github.com/marmot-protocol/mdk/issues/159))
 - **Message Retry Support**: Implemented better handling for retryable message states. When a message fails processing, it now preserves the `message_event_id` and other context. Added logic to allow reprocessing of messages marked as `Retryable`, with automatic state recovery to `Processed` upon success. ([#161](https://github.com/marmot-protocol/mdk/pull/161))
 - Configurable `out_of_order_tolerance` and `maximum_forward_distance` in `MdkConfig` for MLS sender ratchet settings. Default `out_of_order_tolerance` increased from 5 to 100 for better handling of out-of-order message delivery on Nostr relays. ([`#155`](https://github.com/marmot-protocol/mdk/pull/155))
 

crates/mdk-core/src/messages/error_handling.rs

@@ -504,7 +504,7 @@ mod tests {
     /// of previously failed events, mitigating DoS attacks.
     ///
     /// When a previously failed message cannot provide a valid group_id (missing or
-    /// malformed h-tag), we return an error to be explicit about the failure.
+    /// malformed h-tag), we return PreviouslyFailed to avoid crashing client apps.
     #[test]
     fn test_repeated_validation_failure_rejected_immediately() {
         let mdk = create_test_mdk();
@@ -520,18 +520,15 @@ mod tests {
         assert!(result1.is_err(), "First attempt should fail validation");
 
         // Second attempt - should be rejected immediately via deduplication
-        // Returns error because group_id cannot be extracted from malformed event
+        // Returns PreviouslyFailed because group_id cannot be extracted from malformed event
         let result2 = mdk.process_message(&event);
         assert!(
-            result2.is_err(),
-            "Second attempt should return error for malformed event without valid h-tag"
+            result2.is_ok(),
+            "Second attempt should return Ok(PreviouslyFailed), not error"
         );
         assert!(
-            result2
-                .unwrap_err()
-                .to_string()
-                .contains("Message processing previously failed"),
-            "Should indicate message previously failed"
+            matches!(result2.unwrap(), MessageProcessingResult::PreviouslyFailed),
+            "Should return PreviouslyFailed variant"
         );
     }
 
@@ -584,7 +581,7 @@ mod tests {
     ///
     /// This test verifies the deduplication mechanism works for decryption failures,
     /// preventing expensive repeated decryption attempts. When the group doesn't exist
-    /// in storage, we return an error since we can't determine the MLS group ID.
+    /// in storage, we return PreviouslyFailed to avoid crashing client apps.
     #[test]
     fn test_repeated_decryption_failure_rejected_immediately() {
         let mdk = create_test_mdk();
@@ -602,26 +599,23 @@ mod tests {
         let result1 = mdk.process_message(&event);
         assert!(result1.is_err(), "First attempt should fail decryption");
 
-        // Second attempt - should return error because group isn't in storage
+        // Second attempt - should return PreviouslyFailed because group isn't in storage
         // (we can't determine the MLS group ID from just the Nostr group ID)
         let result2 = mdk.process_message(&event);
         assert!(
-            result2.is_err(),
-            "Second attempt should return error when group not in storage"
+            result2.is_ok(),
+            "Second attempt should return Ok(PreviouslyFailed), not error"
         );
         assert!(
-            result2
-                .unwrap_err()
-                .to_string()
-                .contains("Message processing previously failed"),
-            "Should indicate message previously failed"
+            matches!(result2.unwrap(), MessageProcessingResult::PreviouslyFailed),
+            "Should return PreviouslyFailed variant"
         );
     }
 
-    /// Test that previously failed message without group in storage returns error
+    /// Test that previously failed message without group in storage returns PreviouslyFailed
     ///
     /// This test verifies that when a previously failed message has a valid h-tag
-    /// but the group doesn't exist in storage, we return an error since we can't
+    /// but the group doesn't exist in storage, we return PreviouslyFailed since we can't
     /// determine the MLS group ID (Nostr group ID != MLS group ID).
     #[test]
     fn test_previously_failed_message_without_group_in_storage() {
@@ -655,26 +649,23 @@ mod tests {
             "State should be Failed"
         );
 
-        // Second attempt - should return error because we can't determine MLS group ID
+        // Second attempt - should return PreviouslyFailed because we can't determine MLS group ID
         let result2 = mdk.process_message(&event);
         assert!(
-            result2.is_err(),
-            "Second attempt should return error when group not in storage"
+            result2.is_ok(),
+            "Second attempt should return Ok(PreviouslyFailed), not error"
         );
         assert!(
-            result2
-                .unwrap_err()
-                .to_string()
-                .contains("Message processing previously failed"),
-            "Should indicate message previously failed"
+            matches!(result2.unwrap(), MessageProcessingResult::PreviouslyFailed),
+            "Should return PreviouslyFailed variant"
         );
     }
 
-    /// Test that previously failed message with oversized hex in h-tag returns error
+    /// Test that previously failed message with oversized hex in h-tag returns PreviouslyFailed
     ///
     /// This test verifies that when a previously failed message has an oversized hex string
     /// in the h-tag (potential DoS vector), the size check prevents decoding and returns
-    /// an explicit error.
+    /// PreviouslyFailed to avoid crashing client apps.
     #[test]
     fn test_previously_failed_message_with_oversized_hex() {
         let mdk = create_test_mdk();
@@ -703,25 +694,23 @@ mod tests {
             message_types::ProcessedMessageState::Failed
         );
 
-        // Second attempt - should return error due to malformed h-tag
+        // Second attempt - should return PreviouslyFailed due to malformed h-tag
         let result2 = mdk.process_message(&event);
         assert!(
-            result2.is_err(),
-            "Second attempt should return error for oversized hex"
+            result2.is_ok(),
+            "Second attempt should return Ok(PreviouslyFailed), not error"
         );
         assert!(
-            result2
-                .unwrap_err()
-                .to_string()
-                .contains("Message processing previously failed"),
-            "Should indicate message previously failed"
+            matches!(result2.unwrap(), MessageProcessingResult::PreviouslyFailed),
+            "Should return PreviouslyFailed variant"
         );
     }
 
-    /// Test that previously failed message with undersized hex in h-tag returns error
+    /// Test that previously failed message with undersized hex in h-tag returns PreviouslyFailed
     ///
     /// This test verifies that when a previously failed message has an undersized hex string
-    /// in the h-tag, the size check prevents decoding and returns an explicit error.
+    /// in the h-tag, the size check prevents decoding and returns PreviouslyFailed to avoid
+    /// crashing client apps.
     #[test]
     fn test_previously_failed_message_with_undersized_hex() {
         let mdk = create_test_mdk();
@@ -750,18 +739,15 @@ mod tests {
             message_types::ProcessedMessageState::Failed
         );
 
-        // Second attempt - should return error due to malformed h-tag
+        // Second attempt - should return PreviouslyFailed due to malformed h-tag
         let result2 = mdk.process_message(&event);
         assert!(
-            result2.is_err(),
-            "Second attempt should return error for undersized hex"
+            result2.is_ok(),
+            "Second attempt should return Ok(PreviouslyFailed), not error"
         );
         assert!(
-            result2
-                .unwrap_err()
-                .to_string()
-                .contains("Message processing previously failed"),
-            "Should indicate message previously failed"
+            matches!(result2.unwrap(), MessageProcessingResult::PreviouslyFailed),
+            "Should return PreviouslyFailed variant"
         );
     }
 
@@ -826,10 +812,10 @@ mod tests {
         }
     }
 
-    /// Test that previously failed message with invalid hex characters returns error
+    /// Test that previously failed message with invalid hex characters returns PreviouslyFailed
     ///
     /// This test verifies that when hex::decode fails due to invalid characters,
-    /// the code returns an explicit error.
+    /// the code returns PreviouslyFailed to avoid crashing client apps.
     #[test]
     fn test_previously_failed_message_with_invalid_hex_chars() {
         let mdk = create_test_mdk();
@@ -864,18 +850,15 @@ mod tests {
             message_types::ProcessedMessageState::Failed
         );
 
-        // Second attempt - should return error due to invalid hex
+        // Second attempt - should return PreviouslyFailed due to invalid hex
         let result2 = mdk.process_message(&event);
         assert!(
-            result2.is_err(),
-            "Second attempt should return error for invalid hex chars"
+            result2.is_ok(),
+            "Second attempt should return Ok(PreviouslyFailed), not error"
         );
         assert!(
-            result2
-                .unwrap_err()
-                .to_string()
-                .contains("Message processing previously failed"),
-            "Should indicate message previously failed"
+            matches!(result2.unwrap(), MessageProcessingResult::PreviouslyFailed),
+            "Should return PreviouslyFailed variant"
         );
     }
 
@@ -1007,6 +990,84 @@ mod tests {
         }
     }
 
+    /// Test that sanitize_error_reason covers all explicitly mapped error variants
+    /// and falls back to "processing_failed" for unmapped variants
+    #[test]
+    fn test_sanitize_error_reason_all_variants() {
+        use crate::MDK;
+        use mdk_memory_storage::MdkMemoryStorage;
+        use nostr::Kind;
+
+        // Test explicitly mapped error variants
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::UnexpectedEvent {
+                expected: Kind::MlsGroupMessage,
+                received: Kind::TextNote,
+            }),
+            "invalid_event_type"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::MissingGroupIdTag),
+            "invalid_event_format"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::InvalidGroupIdFormat(
+                "bad format".to_string()
+            )),
+            "invalid_event_format"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::MultipleGroupIdTags(3)),
+            "invalid_event_format"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::InvalidTimestamp(
+                "future timestamp".to_string()
+            )),
+            "invalid_event_format"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::GroupNotFound),
+            "group_not_found"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::CannotDecryptOwnMessage),
+            "own_message"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::AuthorMismatch),
+            "authentication_failed"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::CommitFromNonAdmin),
+            "authorization_failed"
+        );
+
+        // Test catch-all for unmapped variants (should return "processing_failed")
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::MessageNotFound),
+            "processing_failed"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::OwnLeafNotFound),
+            "processing_failed"
+        );
+
+        assert_eq!(
+            MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::ProcessMessageWrongEpoch(5)),
+            "processing_failed"
+        );
+    }
+
     #[test]
     fn test_record_failure_preserves_message_event_id() {
         let mdk = create_test_mdk();

crates/mdk-core/src/messages/mod.rs

@@ -105,6 +105,12 @@ pub enum MessageProcessingResult {
         /// The MLS group ID of the message that could not be processed
         mls_group_id: GroupId,
     },
+    /// Message was previously marked as failed and cannot be reprocessed
+    ///
+    /// This variant is returned when a message that previously failed processing
+    /// is received again. Unlike `Unprocessable`, this does not require an MLS group ID
+    /// because the group ID may not be extractable from malformed messages.
+    PreviouslyFailed,
 }
 
 impl std::fmt::Debug for MessageProcessingResult {
@@ -145,6 +151,7 @@ impl std::fmt::Debug for MessageProcessingResult {
                 .debug_struct("Unprocessable")
                 .field("mls_group_id", &"[REDACTED]")
                 .finish(),
+            Self::PreviouslyFailed => f.debug_struct("PreviouslyFailed").finish(),
         }
     }
 }

crates/mdk-core/src/messages/process.rs

@@ -279,30 +279,22 @@ where
                 processed.state == message_types::ProcessedMessageState::EpochInvalidated;
 
             if is_failed || is_epoch_invalidated {
-                let mls_group_id = match self.extract_mls_group_id_from_event(event) {
-                    Some(id) => {
+                match self.extract_mls_group_id_from_event(event) {
+                    Some(mls_group_id) => {
                         tracing::debug!(
                             target: "mdk_core::messages::process_message",
                             "Returning Unprocessable for previously failed/invalidated message with extracted group_id"
                         );
-                        id
+                        return Ok(MessageProcessingResult::Unprocessable { mls_group_id });
                     }
                     None => {
                         tracing::debug!(
                             target: "mdk_core::messages::process_message",
-                            "Cannot extract group_id from previously failed/invalidated message (missing or malformed h-tag)"
+                            "Returning PreviouslyFailed for message without extractable group_id"
                         );
-                        // Return the appropriate error message based on the prior state
-                        let error_msg = if is_failed {
-                            "Message processing previously failed"
-                        } else {
-                            "Message epoch was invalidated"
-                        };
-                        return Err(Error::Message(error_msg.to_string()));
+                        return Ok(MessageProcessingResult::PreviouslyFailed);
                     }
-                };
-
-                return Ok(MessageProcessingResult::Unprocessable { mls_group_id });
+                }
             }
 
             // Allow Retryable messages to be reprocessed after rollback
@@ -431,6 +423,8 @@ mod tests {
         let pending_proposal_result = MessageProcessingResult::PendingProposal {
             mls_group_id: test_group_id.clone(),
         };
+        // PreviouslyFailed: for when a message that previously failed cannot provide a group_id
+        let previously_failed_result = MessageProcessingResult::PreviouslyFailed;
 
         // Test that we can match on variants
         match app_result {
@@ -457,6 +451,11 @@ mod tests {
             MessageProcessingResult::PendingProposal { .. } => {}
             _ => panic!("Expected PendingProposal variant"),
         }
+
+        match previously_failed_result {
+            MessageProcessingResult::PreviouslyFailed => {}
+            _ => panic!("Expected PreviouslyFailed variant"),
+        }
     }
 
     #[test]

crates/mdk-uniffi/CHANGELOG.md

@@ -41,6 +41,7 @@
 
 ### Added
 
+- **`PreviouslyFailed` Result Variant**: Added `ProcessMessageResult.PreviouslyFailed` enum variant to handle cases where a previously failed message arrives again but the MLS group ID cannot be extracted. This prevents crashes in client applications (fixes [#153](https://github.com/marmot-protocol/mdk/issues/153)) by returning a result instead of throwing an exception. ([#165](https://github.com/marmot-protocol/mdk/pull/165), fixes [#154](https://github.com/marmot-protocol/mdk/issues/154), [#159](https://github.com/marmot-protocol/mdk/issues/159))
 - Added `MdkConfig` record for configuring MDK behavior, including `out_of_order_tolerance` and `maximum_forward_distance` settings for MLS sender ratchet configuration. All fields are optional and default to sensible values. ([`#155`](https://github.com/marmot-protocol/mdk/pull/155))
 - Exposed pagination control for `get_messages()` to foreign language bindings via optional `limit` and `offset` parameters. ([#111](https://github.com/marmot-protocol/mdk/pull/111))
 - Exposed pagination control for `get_pending_welcomes()` to foreign language bindings via optional `limit` and `offset` parameters. ([#119](https://github.com/marmot-protocol/mdk/pull/119))