UniFFI don't panic on failed lift (#373)

dani-garcia · Thomas-Avery · web-flow · commit 91eadb56b8f3 · 2025-08-25T22:56:05.000+02:00
## 🎟️ Tracking  ## 📔 Objective UniFFI uses the `custom_type!` macro to convert request/response types. The request conversion is allowed to fail which should produce an error in the apps, but there are cases where it would produce a panic instead. The cases are: - If the FFI function doesn't return a result, then UniFFI produces a panic. - If the Error type of the function is different than the Error type in `custom_type!`, then UniFFI produces a panic. One way to fix this is to have the `custom_type!` calls return `bitwarden_uniffi::error::BitwardenError`, but this isn't possible as core and crypto can't depend on uniffi due to circular dependency issues. This PR makes the uniffi crate register an error conversion function during client initialization, thanks to the use of a new `uniffi-error` crate. It also updates all the calls to `custom_type!` to use this new error conversion. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines  - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes --------- Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -39,6 +39,7 @@ bitwarden-ssh = { path = "crates/bitwarden-ssh", version = "=1.0.0" }
 bitwarden-state = { path = "crates/bitwarden-state", version = "=1.0.0" }
 bitwarden-test = { path = "crates/bitwarden-test", version = "=1.0.0" }
 bitwarden-threading = { path = "crates/bitwarden-threading", version = "=1.0.0" }
+bitwarden-uniffi-error = { path = "crates/bitwarden-uniffi-error", version = "=1.0.0" }
 bitwarden-uuid = { path = "crates/bitwarden-uuid", version = "=1.0.0" }
 bitwarden-uuid-macro = { path = "crates/bitwarden-uuid-macro", version = "=1.0.0" }
 bitwarden-vault = { path = "crates/bitwarden-vault", version = "=1.0.0" }
diff --git a/crates/bitwarden-core/Cargo.toml b/crates/bitwarden-core/Cargo.toml
@@ -37,6 +37,7 @@ bitwarden-crypto = { workspace = true }
 bitwarden-encoding = { workspace = true }
 bitwarden-error = { workspace = true }
 bitwarden-state = { workspace = true }
+bitwarden-uniffi-error = { workspace = true }
 bitwarden-uuid = { workspace = true }
 chrono = { workspace = true, features = ["std"] }
 # We don't use this directly (it's used by rand), but we need it here to enable WASM support
diff --git a/crates/bitwarden-core/src/uniffi_support.rs b/crates/bitwarden-core/src/uniffi_support.rs
@@ -2,7 +2,7 @@
 
 use std::{num::NonZeroU32, str::FromStr};
 
-use bitwarden_crypto::CryptoError;
+use bitwarden_uniffi_error::convert_result;
 use uuid::Uuid;
 
 use crate::key_management::{PasswordProtectedKeyEnvelope, SignedSecurityState};
@@ -14,7 +14,7 @@ uniffi::custom_type!(DateTime, std::time::SystemTime, { remote });
 
 uniffi::custom_type!(Uuid, String, {
     remote,
-    try_lift: |val| Uuid::parse_str(val.as_str()).map_err(|e| e.into()),
+    try_lift: |val| convert_result(Uuid::parse_str(&val)),
     lower: |obj| obj.to_string(),
 });
 
@@ -29,17 +29,14 @@ struct UniffiConverterDummyRecord {
 
 uniffi::custom_type!(SignedSecurityState, String, {
     try_lift: |val| {
-        val.parse().map_err(|e| {
-            CryptoError::EncodingError(e).into()
-        })
+        convert_result(SignedSecurityState::from_str(&val))
     },
     lower: |obj| obj.into(),
 });
 
 uniffi::custom_type!(PasswordProtectedKeyEnvelope, String, {
     remote,
-    try_lift: |val| bitwarden_crypto::safe::PasswordProtectedKeyEnvelope::from_str(val.as_str())
-        .map_err(|e| e.into())
-        .map(PasswordProtectedKeyEnvelope),
+    try_lift: |val| convert_result(bitwarden_crypto::safe::PasswordProtectedKeyEnvelope::from_str(&val)
+        .map(PasswordProtectedKeyEnvelope)),
     lower: |obj| obj.0.into(),
 });
diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml
@@ -30,6 +30,7 @@ argon2 = { version = ">=0.5.0, <0.6", features = [
 base64 = ">=0.22.1, <0.23"
 bitwarden-encoding = { workspace = true }
 bitwarden-error = { workspace = true }
+bitwarden-uniffi-error = { workspace = true }
 cbc = { version = ">=0.1.2, <0.2", features = ["alloc", "zeroize"] }
 chacha20poly1305 = { version = "0.10.1" }
 ciborium = { version = ">=0.2.2, <0.3" }
diff --git a/crates/bitwarden-crypto/src/uniffi_support.rs b/crates/bitwarden-crypto/src/uniffi_support.rs
@@ -1,34 +1,34 @@
 use std::{num::NonZeroU32, str::FromStr};
 
+use bitwarden_uniffi_error::convert_result;
+
 use crate::{CryptoError, EncString, SignedPublicKey, UnsignedSharedKey};
 
 uniffi::custom_type!(NonZeroU32, u32, {
     remote,
     try_lift: |val| {
-        NonZeroU32::new(val).ok_or(CryptoError::ZeroNumber.into())
+        convert_result(NonZeroU32::new(val).ok_or(CryptoError::ZeroNumber))
     },
     lower: |obj| obj.get(),
 });
 
 uniffi::custom_type!(EncString, String, {
     try_lift: |val| {
-        EncString::from_str(&val).map_err(|e: CryptoError| e.into())
+        convert_result(EncString::from_str(&val))
     },
     lower: |obj| obj.to_string(),
 });
 
 uniffi::custom_type!(UnsignedSharedKey, String, {
     try_lift: |val| {
-        UnsignedSharedKey::from_str(&val).map_err(|e: CryptoError| e.into())
+        convert_result(UnsignedSharedKey::from_str(&val))
     },
     lower: |obj| obj.to_string(),
 });
 
 uniffi::custom_type!(SignedPublicKey, String, {
     try_lift: |val| {
-        val.parse().map_err(|e| {
-            CryptoError::EncodingError(e).into()
-        })
+        convert_result(SignedPublicKey::from_str(&val))
     },
     lower: |obj| obj.into(),
 });
diff --git a/crates/bitwarden-encoding/Cargo.toml b/crates/bitwarden-encoding/Cargo.toml
@@ -19,6 +19,7 @@ uniffi = ["dep:uniffi"]
 wasm = ["dep:tsify", "dep:wasm-bindgen"]
 
 [dependencies]
+bitwarden-uniffi-error = { workspace = true }
 data-encoding = { workspace = true }
 data-encoding-macro = "0.1.18"
 serde = { workspace = true }
diff --git a/crates/bitwarden-encoding/src/uniffi_support.rs b/crates/bitwarden-encoding/src/uniffi_support.rs
@@ -1,15 +1,19 @@
-use crate::{b64::NotB64Encoded, b64url::NotB64UrlEncoded, B64Url, B64};
+use std::str::FromStr;
+
+use bitwarden_uniffi_error::convert_result;
+
+use crate::{B64Url, B64};
 
 uniffi::custom_type!(B64, String, {
     try_lift: |val| {
-        val.parse().map_err(|e: NotB64Encoded| e.into())
+        convert_result(B64::from_str(&val))
     },
     lower: |obj| obj.to_string(),
 });
 
 uniffi::custom_type!(B64Url, String, {
     try_lift: |val| {
-        val.parse().map_err(|e: NotB64UrlEncoded| e.into())
+        convert_result(B64Url::from_str(&val))
     },
     lower: |obj| obj.to_string(),
 });
diff --git a/crates/bitwarden-uniffi-error/Cargo.toml b/crates/bitwarden-uniffi-error/Cargo.toml
@@ -0,0 +1,25 @@
+[package]
+name = "bitwarden-uniffi-error"
+description = """
+Internal crate for the bitwarden crate. Do not use.
+"""
+
+version.workspace = true
+authors.workspace = true
+edition.workspace = true
+rust-version.workspace = true
+readme.workspace = true
+homepage.workspace = true
+repository.workspace = true
+license-file.workspace = true
+keywords.workspace = true
+
+[features]
+uniffi = ["dep:uniffi"]
+
+[dependencies]
+anyhow = { version = ">=1.0, <2.0" }
+uniffi = { workspace = true, optional = true }
+
+[lints]
+workspace = true
diff --git a/crates/bitwarden-uniffi-error/README.md b/crates/bitwarden-uniffi-error/README.md
@@ -0,0 +1,4 @@
+# Bitwarden UniFFI error
+
+This crate provides some utilities to convert results inside `uniffi::custom_type!` calls, so that
+they don't produce panics when there is a parsing error.
diff --git a/crates/bitwarden-uniffi-error/src/lib.rs b/crates/bitwarden-uniffi-error/src/lib.rs
@@ -0,0 +1,38 @@
+#![doc = include_str!("../README.md")]
+
+use std::sync::OnceLock;
+
+#[allow(clippy::type_complexity)]
+static ERROR_TO_UNIFFI_ERROR: OnceLock<
+    Box<dyn Fn(Box<dyn std::error::Error + Send + Sync>) -> anyhow::Error + Send + Sync + 'static>,
+> = OnceLock::new();
+
+pub use anyhow::Error;
+
+/// Configure an error converter to convert errors in calls to [`uniffi::custom_type!`] into the
+/// main error of the application (`bitwarden_uniffi::error::BitwardenError). This is needed because
+/// if the errors don't match, Uniffi will panic instead of returning an error. This needs to be
+/// called by the `bitwarden_uniffi` crate before any other Uniffi code is run.
+pub fn set_error_to_uniffi_error<F>(f: F)
+where
+    F: Fn(Box<dyn std::error::Error + Send + Sync>) -> anyhow::Error + Send + Sync + 'static,
+{
+    let _ = ERROR_TO_UNIFFI_ERROR.set(Box::new(f));
+}
+
+fn convert_error<E: std::error::Error + Send + Sync + 'static>(error: E) -> anyhow::Error {
+    if let Some(f) = ERROR_TO_UNIFFI_ERROR.get() {
+        f(Box::new(error))
+    } else {
+        anyhow::Error::new(error)
+    }
+}
+
+/// Convert a `Result` into one that will not cause a panic when called inside
+/// [`uniffi::custom_type!`]. It is required that all the results created inside a `custom_type!`
+/// are converted using this function.
+pub fn convert_result<T, E: std::error::Error + Send + Sync + 'static>(
+    result: Result<T, E>,
+) -> Result<T, anyhow::Error> {
+    result.map_err(|e| convert_error(e))
+}
diff --git a/crates/bitwarden-uniffi/Cargo.toml b/crates/bitwarden-uniffi/Cargo.toml
@@ -30,6 +30,7 @@ bitwarden-generators = { workspace = true, features = ["uniffi"] }
 bitwarden-send = { workspace = true, features = ["uniffi"] }
 bitwarden-ssh = { workspace = true, features = ["uniffi"] }
 bitwarden-state = { workspace = true, features = ["uniffi"] }
+bitwarden-uniffi-error = { workspace = true, features = ["uniffi"] }
 bitwarden-vault = { workspace = true, features = ["uniffi"] }
 chrono = { workspace = true, features = ["std"] }
 env_logger = "0.11.1"
diff --git a/crates/bitwarden-uniffi/src/error.rs b/crates/bitwarden-uniffi/src/error.rs
@@ -9,6 +9,7 @@ use bitwarden_generators::{PassphraseError, PasswordError, UsernameError};
 #[uniffi(flat_error)]
 pub enum BitwardenError {
     E(Error),
+    ConversionError(Box<dyn std::error::Error + Send + Sync>),
 }
 
 impl From<Error> for BitwardenError {
@@ -21,6 +22,7 @@ impl Display for BitwardenError {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match self {
             Self::E(e) => Display::fmt(e, f),
+            Self::ConversionError(e) => Display::fmt(e, f),
         }
     }
 }
@@ -29,6 +31,7 @@ impl std::error::Error for BitwardenError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {
             BitwardenError::E(e) => Some(e),
+            BitwardenError::ConversionError(e) => Some(e.as_ref()),
         }
     }
 }
diff --git a/crates/bitwarden-uniffi/src/lib.rs b/crates/bitwarden-uniffi/src/lib.rs
@@ -41,6 +41,7 @@ impl Client {
     #[uniffi::constructor]
     pub fn new(settings: Option<ClientSettings>) -> Self {
         init_logger();
+        setup_error_converter();
 
         #[cfg(target_os = "android")]
         android_support::init();
@@ -123,3 +124,11 @@ fn init_logger() {
             .with_max_level(log::LevelFilter::Info),
     );
 }
+
+/// Setup the error converter to ensure conversion errors don't cause panics
+/// Check [`bitwarden_uniffi_error`] for more details
+fn setup_error_converter() {
+    bitwarden_uniffi_error::set_error_to_uniffi_error(|e| {
+        crate::error::BitwardenError::ConversionError(e).into()
+    });
+}
diff --git a/crates/bitwarden-uniffi/src/vault/ciphers.rs b/crates/bitwarden-uniffi/src/vault/ciphers.rs
@@ -29,8 +29,13 @@ impl CiphersClient {
 
     /// Decrypt cipher list with failures
     /// Returns both successfully decrypted ciphers and any that failed to decrypt
-    pub fn decrypt_list_with_failures(&self, ciphers: Vec<Cipher>) -> DecryptCipherListResult {
-        self.0.decrypt_list_with_failures(ciphers)
+    // Note that this function still needs to return a Result, as the parameter conversion can still
+    // fail
+    pub fn decrypt_list_with_failures(
+        &self,
+        ciphers: Vec<Cipher>,
+    ) -> Result<DecryptCipherListResult> {
+        Ok(self.0.decrypt_list_with_failures(ciphers))
     }
 
     pub fn decrypt_fido2_credentials(
