docs(crypto): clarify encryptable contract

crates/bitwarden-crypto/src/traits/decryptable.rs

@@ -3,8 +3,12 @@ use crate::{CryptoError, EncString, KeySlotId, KeySlotIds, store::KeyStoreContex
 /// A decryption operation that takes the input value and decrypts it into the output value.
 /// Implementations should generally consist of calling [Decryptable::decrypt] for all the fields of
 /// the type.
+///
+/// Decrypt must decrypt the item fully, and not leave parts encrypted. That is,
+/// decrypt(K1) -> encrypt(K2) -> decrypt(K2) MUST succed.
 pub trait Decryptable<Ids: KeySlotIds, Key: KeySlotId, Output> {
-    #[allow(missing_docs)]
+    /// Decrypts `self` into an `Output` under `key`. See the trait-level contract: the result must
+    /// contain no cipher text.
     fn decrypt(&self, ctx: &mut KeyStoreContext<Ids>, key: Key) -> Result<Output, CryptoError>;
 }
 

crates/bitwarden-crypto/src/traits/encryptable.rs

@@ -16,6 +16,21 @@
 //! a type that implements the `ConstContentFormat` trait. This allows for compile-time type
 //! checking of the content format, and the risk of using the wrong content format is limited to
 //! converting untyped bytes into a `Bytes<C>`
+//!
+//! # Round-trip contract
+//!
+//! The encrypt traits here and [`crate::Decryptable`] form a symmetric pair with a contract that
+//! callers rely on:
+//!
+//! - Decryption MUST yield a value that contains no encrypted material — no wrapped keys and no
+//!   residual [`EncString`]s that are still bound to the key that was used to decrypt. Everything
+//!   protected by that key must be fully decrypted.
+//! - Encryption MUST produce output that is fully encrypted under the provided `key`. No encrypted
+//!   items may be copied from the input. Plaintext data co-existing with encrypted data is allowed,
+//!   and may be copied through.
+//!
+//! Together these mean that decrypting with one key and re-encrypting with another round-trips: for
+//! any two valid keys `K` and `K1`, `decrypt(encrypt(decrypt(x, K), K1), K1) = decrypt(x, K)`.
 
 use crate::{ContentFormat, CryptoError, EncString, KeySlotId, KeySlotIds, store::KeyStoreContext};
 
@@ -32,6 +47,16 @@ pub trait CompositeEncryptable<Ids: KeySlotIds, Key: KeySlotId, Output> {
     ///
     /// For a struct made up of many small encstrings, such as a cipher, this takes the struct
     /// and recursively encrypts all the fields / sub-structs.
+    ///
+    /// # Contract
+    /// The returned value MUST be fully encrypted under `key`. Implementations MUST NOT reuse
+    /// ciphertext embedded in `self` (for example a still-wrapped per-item key carried over from a
+    /// previous decryption); any such key material must be re-derived and re-wrapped under `key`.
+    /// This guarantees that a value produced by [`Decryptable::decrypt`] can be re-encrypted under
+    /// any valid key and then decrypted with that same key — i.e. `decrypt(K)` then `encrypt(K1)`
+    /// then `decrypt(K1)` succeeds.
+    ///
+    /// [`Decryptable::decrypt`]: crate::Decryptable::decrypt
     fn encrypt_composite(
         &self,
         ctx: &mut KeyStoreContext<Ids>,
@@ -75,6 +100,13 @@ pub trait PrimitiveEncryptable<Ids: KeySlotIds, Key: KeySlotId, Output> {
     /// instead.
     ///
     /// Encrypts a primitive without requiring an externally provided content type
+    ///
+    /// # Contract
+    /// The returned value MUST be fully encrypted under `key`, so that the corresponding
+    /// [`Decryptable::decrypt`] yields the original primitive and a decrypt-then-encrypt under a
+    /// different key round-trips.
+    ///
+    /// [`Decryptable::decrypt`]: crate::Decryptable::decrypt
     fn encrypt(&self, ctx: &mut KeyStoreContext<Ids>, key: Key) -> Result<Output, CryptoError>;
 }
 

crates/bitwarden-vault/src/cipher/attachment.rs

@@ -252,6 +252,10 @@ impl Decryptable<KeySlotIds, SymmetricKeySlotId, Vec<u8>> for AttachmentFile {
     }
 }
 
+// ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::CompositeEncryptable`: `AttachmentView` retains
+// key-bound ciphertext (`key`, the attachment content key wrapped under the cipher key) and copies
+// it through unchanged (`key: self.key.clone()` below) instead of re-wrapping it under `key`. As a
+// result decrypt(K) -> encrypt(K1) -> decrypt(K1) does NOT round-trip.
 impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, Attachment> for AttachmentView {
     fn encrypt_composite(
         &self,
@@ -264,6 +268,8 @@ impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, Attachment> for Attach
             size: self.size.clone(),
             size_name: self.size_name.clone(),
             file_name: self.file_name.encrypt(ctx, key)?,
+            // ⚠️ pass-through of wrapped key-bound ciphertext — see the contract-violation note
+            // above.
             key: self.key.clone(),
         })
     }
@@ -296,6 +302,11 @@ impl Decryptable<KeySlotIds, SymmetricKeySlotId, AttachmentView> for Attachment
             size: self.size.clone(),
             size_name: self.size_name.clone(),
             file_name,
+            // ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::Decryptable`: the resulting
+            // `AttachmentView` is a decrypted DTO, yet `key` (the attachment content key wrapped
+            // under the cipher key) is copied through still encrypted (`self.key.clone()`) rather
+            // than decrypted. The wrapped key is therefore key-bound to the original cipher key,
+            // which is what makes the `CompositeEncryptable` pass-through above non-round-tripping.
             key: self.key.clone(),
             #[cfg(feature = "wasm")]
             decrypted_key: decrypted_key.map(|k| k.to_string()),

crates/bitwarden-vault/src/cipher/cipher.rs

@@ -647,6 +647,10 @@ impl CipherListView {
     }
 }
 
+// ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::CompositeEncryptable`: `CipherView` retains key-bound
+// ciphertext (`key`, the cipher content-encryption key wrapped under the decrypting key) and copies
+// it through unchanged (`key: cipher_view.key` below) instead of re-wrapping it under `key`. As a
+// result decrypt(K) -> encrypt(K1) -> decrypt(K1) does NOT round-trip.
 impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, Cipher> for CipherView {
     fn encrypt_composite(
         &self,
@@ -663,6 +667,8 @@ impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, Cipher> for CipherView
             organization_id: cipher_view.organization_id,
             folder_id: cipher_view.folder_id,
             collection_ids: cipher_view.collection_ids,
+            // ⚠️ pass-through of wrapped key-bound ciphertext — see the contract-violation note
+            // above.
             key: cipher_view.key,
             name: Some(cipher_view.name.encrypt(ctx, ciphers_key)?),
             notes: cipher_view.notes.encrypt(ctx, ciphers_key)?,
@@ -729,6 +735,12 @@ pub(crate) fn lenient_decrypt_cipher_view(
         organization_id: cipher.organization_id,
         folder_id: cipher.folder_id,
         collection_ids: cipher.collection_ids.clone(),
+        // ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::Decryptable`: the resulting `CipherView` is a
+        // decrypted DTO, yet `key` (the cipher's content key wrapped under the user/org key) is
+        // copied through still encrypted (`cipher.key.clone()`) rather than decrypted, because
+        // `CipherView` stores it as an `EncString`. The wrapped key is therefore key-bound to the
+        // original user/org key: a `CipherView` cannot be re-encrypted under a different user/org
+        // key without explicitly rewrapping `key`.
         key: cipher.key.clone(),
         name: cipher
             .name
@@ -1408,6 +1420,8 @@ pub(crate) fn lenient_decrypt_cipher_list_view(
         organization_id: cipher.organization_id,
         folder_id: cipher.folder_id,
         collection_ids: cipher.collection_ids.clone(),
+        // ⚠️ pass-through of the wrapped, key-bound cipher key — see the contract-violation note in
+        // `lenient_decrypt_cipher_view`.
         key: cipher.key.clone(),
         name: cipher
             .name
@@ -1599,6 +1613,8 @@ fn strict_decrypt_cipher_view(
         organization_id: cipher.organization_id,
         folder_id: cipher.folder_id,
         collection_ids: cipher.collection_ids.clone(),
+        // ⚠️ pass-through of the wrapped, key-bound cipher key — see the contract-violation note in
+        // `lenient_decrypt_cipher_view`.
         key: cipher.key.clone(),
         name: cipher
             .name
@@ -1691,6 +1707,8 @@ fn strict_decrypt_cipher_list_view(
         organization_id: cipher.organization_id,
         folder_id: cipher.folder_id,
         collection_ids: cipher.collection_ids.clone(),
+        // ⚠️ pass-through of the wrapped, key-bound cipher key — see the contract-violation note in
+        // `lenient_decrypt_cipher_view`.
         key: cipher.key.clone(),
         name: cipher
             .name

crates/bitwarden-vault/src/cipher/login.rs

@@ -430,6 +430,13 @@ impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, LoginUri> for LoginUri
     }
 }
 
+// ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::CompositeEncryptable`: `LoginView` is a decrypted
+// DTO, yet it stores `fido2_credentials` as `Vec<Fido2Credential>` (already-encrypted values)
+// rather than a decrypted view type. Encryption therefore copies the ciphertext through unchanged
+// (`fido2_credentials: self.fido2_credentials.clone()` below) instead of re-encrypting it under
+// `key`. As a result decrypt(K) -> encrypt(K1) -> decrypt(K1) does NOT round-trip the credentials:
+// they remain wrapped under the original key K. Callers that rewrap the cipher key must invoke
+// `LoginView::reencrypt_fido2_credentials` explicitly to keep the credentials decryptable.
 impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, Login> for LoginView {
     fn encrypt_composite(
         &self,
@@ -447,6 +454,8 @@ impl CompositeEncryptable<KeySlotIds, SymmetricKeySlotId, Login> for LoginView {
                 .filter(|s| !s.is_empty())
                 .encrypt(ctx, key)?,
             autofill_on_page_load: self.autofill_on_page_load,
+            // ⚠️ pass-through of already-encrypted credentials — see the contract-violation note
+            // above.
             fido2_credentials: self.fido2_credentials.clone(),
         })
     }
@@ -479,6 +488,11 @@ impl Decryptable<KeySlotIds, SymmetricKeySlotId, LoginView> for Login {
             uris: self.uris.decrypt(ctx, key).ok().flatten(),
             totp: self.totp.decrypt(ctx, key).ok().flatten(),
             autofill_on_page_load: self.autofill_on_page_load,
+            // ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::Decryptable`: the resulting `LoginView`
+            // is a decrypted DTO, but `fido2_credentials` are copied through still
+            // encrypted (`self.fido2_credentials.clone()`) rather than decrypted,
+            // because `LoginView` stores them as the encrypted `Vec<Fido2Credential>`.
+            // Consumers must decrypt each credential separately.
             fido2_credentials: self.fido2_credentials.clone(),
         })
     }
@@ -516,6 +530,11 @@ impl Decryptable<KeySlotIds, SymmetricKeySlotId, LoginView> for StrictDecrypt<&L
             uris: self.0.uris.decrypt(ctx, key)?,
             totp: self.0.totp.decrypt(ctx, key)?,
             autofill_on_page_load: self.0.autofill_on_page_load,
+            // ⚠️ CONTRACT VIOLATION of `bitwarden_crypto::Decryptable`: the resulting `LoginView`
+            // is a decrypted DTO, but `fido2_credentials` are copied through still
+            // encrypted (`self.0.fido2_credentials.clone()`) rather than decrypted,
+            // because `LoginView` stores them as the encrypted `Vec<Fido2Credential>`.
+            // Consumers must decrypt each credential separately.
             fido2_credentials: self.0.fido2_credentials.clone(),
         })
     }