Skip to content

UniFFI don't panic on failed lift #373

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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" }
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ bitwarden-api-identity = { workspace = true }
bitwarden-crypto = { 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
Expand Down
7 changes: 3 additions & 4 deletions crates/bitwarden-core/src/uniffi_support.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::num::NonZeroU32;

use bitwarden_crypto::CryptoError;
use bitwarden_uniffi_error::convert_result;
use uuid::Uuid;

use crate::key_management::SignedSecurityState;
Expand All @@ -14,7 +15,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.as_str())),
lower: |obj| obj.to_string(),
});

Expand All @@ -29,9 +30,7 @@ struct UniffiConverterDummyRecord {

uniffi::custom_type!(SignedSecurityState, String, {
try_lift: |val| {
val.parse().map_err(|e| {
CryptoError::EncodingError(e).into()
})
convert_result(val.parse().map_err(CryptoError::EncodingError))
},
lower: |obj| obj.into(),
});
3 changes: 2 additions & 1 deletion crates/bitwarden-crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ argon2 = { version = ">=0.5.0, <0.6", features = [
], default-features = false }
base64 = ">=0.22.1, <0.23"
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" }
Expand All @@ -46,7 +47,7 @@ rayon = ">=1.8.1, <2.0"
rsa = ">=0.9.2, <0.10"
schemars = { workspace = true }
serde = { workspace = true }
serde_bytes = { workspace = true }
serde_bytes = { workspace = true }
serde_repr.workspace = true
sha1 = ">=0.10.5, <0.11"
sha2 = ">=0.10.6, <0.11"
Expand Down
12 changes: 6 additions & 6 deletions crates/bitwarden-crypto/src/uniffi_support.rs
Original file line number Diff line number Diff line change
@@ -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(val.parse().map_err(CryptoError::EncodingError))
},
lower: |obj| obj.into(),
});
25 changes: 25 additions & 0 deletions crates/bitwarden-uniffi-error/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions crates/bitwarden-uniffi-error/README.md
Original file line number Diff line number Diff line change
@@ -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.
38 changes: 38 additions & 0 deletions crates/bitwarden-uniffi-error/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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 conver errors in calls to [`uniffi::custom_type!`] into the main
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Configure an error converter to conver errors in calls to [`uniffi::custom_type!`] into the main
/// 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))
}
1 change: 1 addition & 0 deletions crates/bitwarden-uniffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,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"
Expand Down
3 changes: 3 additions & 0 deletions crates/bitwarden-uniffi/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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),
}
}
}
Expand All @@ -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()),
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions crates/bitwarden-uniffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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()
});
}
9 changes: 7 additions & 2 deletions crates/bitwarden-uniffi/src/vault/ciphers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,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(
Expand Down
Loading