From 2cd01dc93b4842abd0352543c06b7d61ec97a369 Mon Sep 17 00:00:00 2001 From: "Julian Y. Richard Corbet" <48553615+julian-corbet@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:35:35 +0000 Subject: [PATCH 1/3] server: replace only items with identical attributes --- client/src/file/api/encrypted_item.rs | 5 ++ client/src/file/api/mod.rs | 17 +++++ client/src/file/mod.rs | 12 ++++ client/src/file/unlocked_keyring.rs | 8 +-- client/tests/file_unlocked_keyring.rs | 100 ++++++++++++++++++++++++++ server/src/collection/mod.rs | 22 +++++- server/src/collection/tests.rs | 67 +++++++++++++++++ 7 files changed, 226 insertions(+), 5 deletions(-) diff --git a/client/src/file/api/encrypted_item.rs b/client/src/file/api/encrypted_item.rs index bd99e512c..c46128cbf 100644 --- a/client/src/file/api/encrypted_item.rs +++ b/client/src/file/api/encrypted_item.rs @@ -41,6 +41,11 @@ impl EncryptedItem { } } + pub fn matches_exact(&self, attributes: &impl AsAttributes, key: Option<&Key>) -> bool { + let attributes = attributes.as_attributes(); + self.hashed_attributes.len() == attributes.len() && self.matches(&attributes, key) + } + fn try_decrypt_inner(&self, key: Option<&Key>) -> Result { match key { Some(key) => self.try_decrypt_encrypted(key), diff --git a/client/src/file/api/mod.rs b/client/src/file/api/mod.rs index f40adf6d2..8059d639d 100644 --- a/client/src/file/api/mod.rs +++ b/client/src/file/api/mod.rs @@ -235,6 +235,23 @@ impl Keyring { Ok(()) } + pub(crate) fn remove_items_exact( + &mut self, + attributes: &impl AsAttributes, + key: Option<&Key>, + ) -> Result<(), Error> { + for item in &self.items { + if item.matches_attributes_exact(attributes, key) && !item.is_valid(key) { + return Err(Error::MacError); + } + } + + self.items + .retain(|item| !item.matches_attributes_exact(attributes, key)); + + Ok(()) + } + fn as_bytes(&self) -> Result, Error> { let mut blob = FILE_HEADER.to_vec(); diff --git a/client/src/file/mod.rs b/client/src/file/mod.rs index 3263d35d8..0ea6a1f67 100644 --- a/client/src/file/mod.rs +++ b/client/src/file/mod.rs @@ -93,6 +93,18 @@ impl Item { Self::Locked(locked) => locked.inner.matches(attributes, key), } } + + /// Check if this item has exactly the given attributes. + pub fn matches_attributes_exact( + &self, + attributes: &impl AsAttributes, + key: Option<&Key>, + ) -> bool { + match self { + Self::Unlocked(unlocked) => unlocked.attributes() == &attributes.as_attributes(), + Self::Locked(locked) => locked.inner.matches_exact(attributes, key), + } + } } #[derive(Debug)] diff --git a/client/src/file/unlocked_keyring.rs b/client/src/file/unlocked_keyring.rs index 7daace776..c309068dc 100644 --- a/client/src/file/unlocked_keyring.rs +++ b/client/src/file/unlocked_keyring.rs @@ -465,10 +465,10 @@ impl UnlockedKeyring { let item = { let key = self.derive_key().await?; let mut keyring = self.keyring.write().await; + let item = UnlockedItem::new(label, attributes, secret); if replace { - keyring.remove_items(attributes, key.as_deref())?; + keyring.remove_items_exact(item.attributes(), key.as_deref())?; } - let item = UnlockedItem::new(label, attributes, secret); let encrypted_item = item.encrypt(key.as_deref())?; keyring.items.push(encrypted_item); item @@ -540,10 +540,10 @@ impl UnlockedKeyring { let _span = tracing::debug_span!("bulk_create", items_to_create = items.len()); for (label, attributes, secret, replace) in items { + let item = UnlockedItem::new(label, &attributes, secret); if replace { - keyring.remove_items(&attributes, key.as_deref())?; + keyring.remove_items_exact(item.attributes(), key.as_deref())?; } - let item = UnlockedItem::new(label, &attributes, secret); let encrypted_item = item.encrypt(key.as_deref())?; keyring.items.push(encrypted_item); } diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index 3bc4e2fb9..ee15273e9 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -549,6 +549,77 @@ async fn item_replacement_behavior() -> Result<(), Error> { Ok(()) } +#[tokio::test] +async fn item_replacement_matches_attributes_exactly() -> Result<(), Error> { + let temp_dir = tempdir().unwrap(); + let keyring_path = temp_dir.path().join("replace_exact_test.keyring"); + let keyring = UnlockedKeyring::load(&keyring_path, Some(strong_key())).await?; + + keyring + .create_item( + "Alice", + &[("app", "test"), ("user", "alice")], + "alice-secret", + false, + ) + .await?; + keyring + .create_item( + "Bob", + &[("app", "test"), ("user", "bob")], + "bob-secret", + false, + ) + .await?; + + // A coarser attribute set is not the same attribute set, so it must not + // replace either of the more specific items. + keyring + .create_item("Coarse", &[("app", "test")], "coarse-secret", true) + .await?; + + let items = keyring.search_items(&[("app", "test")]).await?; + assert_eq!(items.len(), 3); + assert_eq!( + keyring + .search_items(&[("app", "test"), ("user", "alice")]) + .await? + .len(), + 1 + ); + assert_eq!( + keyring + .search_items(&[("app", "test"), ("user", "bob")]) + .await? + .len(), + 1 + ); + + // Repeating the exact coarse attribute set still replaces its previous + // item rather than adding a duplicate. + keyring + .create_item( + "Updated Coarse", + &[("app", "test")], + "updated-coarse-secret", + true, + ) + .await?; + + let items = keyring.search_items(&[("app", "test")]).await?; + assert_eq!(items.len(), 3); + assert_eq!( + items + .iter() + .filter(|item| item.attributes().get("user").is_none()) + .map(|item| item.label()) + .collect::>(), + vec!["Updated Coarse"] + ); + + Ok(()) +} + #[tokio::test] async fn empty_keyring_operations() -> Result<(), Error> { let temp_dir = tempdir().unwrap(); @@ -815,6 +886,35 @@ async fn bulk_create_items() -> Result<(), Error> { // Verify the item was replaced - should still have 3 items total let all_items_after = keyring.search_items(&[("app", "bulk-app")]).await?; assert_eq!(all_items_after.len(), 3); + + // A bulk replacement follows the same exact-attribute rule as + // create_item: this coarser item must not remove the three specific ones. + keyring + .create_items(vec![( + "Coarse Item".to_string(), + HashMap::from([("app".to_string(), "bulk-app".to_string())]), + Secret::text("coarse-secret"), + true, + )]) + .await?; + assert_eq!( + keyring.search_items(&[("app", "bulk-app")]).await?.len(), + 4 + ); + + // Repeating that exact coarse attribute set replaces only the coarse item. + keyring + .create_items(vec![( + "Updated Coarse Item".to_string(), + HashMap::from([("app".to_string(), "bulk-app".to_string())]), + Secret::text("updated-coarse-secret"), + true, + )]) + .await?; + assert_eq!( + keyring.search_items(&[("app", "bulk-app")]).await?.len(), + 4 + ); Ok(()) } diff --git a/server/src/collection/mod.rs b/server/src/collection/mod.rs index 08704f207..68d4f1a7d 100644 --- a/server/src/collection/mod.rs +++ b/server/src/collection/mod.rs @@ -308,7 +308,7 @@ impl Collection { // Remove any existing items with the same attributes if replace { let existing_items = self - .search_items_with_key(&attributes, key.as_deref()) + .search_items_exact_with_key(&attributes, key.as_deref()) .await?; if !existing_items.is_empty() { let mut items = self.items.lock().await; @@ -531,6 +531,26 @@ impl Collection { Ok(matching_items) } + async fn search_items_exact_with_key( + &self, + attributes: &HashMap, + key: Option<&oo7::Key>, + ) -> Result, ServiceError> { + let mut matching_items = Vec::new(); + let items = self.items.lock().await; + + for item_wrapper in items.iter() { + let inner = item_wrapper.inner.lock().await; + let file_item = inner.as_ref().unwrap(); + + if file_item.matches_attributes_exact(attributes, key) { + matching_items.push(item_wrapper.clone()); + } + } + + Ok(matching_items) + } + pub async fn item_from_path(&self, path: &ObjectPath<'_>) -> Option { let items = self.items.lock().await; diff --git a/server/src/collection/tests.rs b/server/src/collection/tests.rs index 1e23609bc..4a1026630 100644 --- a/server/src/collection/tests.rs +++ b/server/src/collection/tests.rs @@ -226,6 +226,73 @@ async fn create_item_with_replace() -> Result<(), Box> { Ok(()) } +#[tokio::test] +async fn create_item_with_replace_matches_attributes_exactly() +-> Result<(), Box> { + let setup = TestServiceSetup::plain_session(true).await?; + + setup + .create_item( + "Alice", + &[("application", "myapp"), ("username", "alice")], + "alice-password", + false, + ) + .await?; + setup + .create_item( + "Bob", + &[("application", "myapp"), ("username", "bob")], + "bob-password", + false, + ) + .await?; + + // A coarser attribute set is not the same attribute set, so it must not + // replace either of the more specific items. + setup + .create_item( + "Coarse", + &[("application", "myapp")], + "coarse-password", + true, + ) + .await?; + + let items = setup.collections[0].items().await?; + assert_eq!(items.len(), 3); + assert_eq!( + setup.collections[0] + .search_items(&[("application", "myapp"), ("username", "alice")]) + .await? + .len(), + 1 + ); + assert_eq!( + setup.collections[0] + .search_items(&[("application", "myapp"), ("username", "bob")]) + .await? + .len(), + 1 + ); + + // Repeating the exact coarse attribute set still replaces its previous + // item rather than adding a duplicate. + setup + .create_item( + "Updated Coarse", + &[("application", "myapp")], + "updated-coarse-password", + true, + ) + .await?; + + let items = setup.collections[0].items().await?; + assert_eq!(items.len(), 3); + + Ok(()) +} + #[tokio::test] async fn label_property() -> Result<(), Box> { let setup = TestServiceSetup::plain_session(true).await?; From 125cd560b23c85b32d89794b1c21e14ec294b81c Mon Sep 17 00:00:00 2001 From: "Julian Y. Richard Corbet" <48553615+julian-corbet@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:41:42 +0000 Subject: [PATCH 2/3] fix exact replacement validation --- client/src/file/api/mod.rs | 4 ++-- client/tests/file_unlocked_keyring.rs | 10 ++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/client/src/file/api/mod.rs b/client/src/file/api/mod.rs index 8059d639d..cd73465a8 100644 --- a/client/src/file/api/mod.rs +++ b/client/src/file/api/mod.rs @@ -241,13 +241,13 @@ impl Keyring { key: Option<&Key>, ) -> Result<(), Error> { for item in &self.items { - if item.matches_attributes_exact(attributes, key) && !item.is_valid(key) { + if item.matches_exact(attributes, key) && !item.is_valid(key) { return Err(Error::MacError); } } self.items - .retain(|item| !item.matches_attributes_exact(attributes, key)); + .retain(|item| !item.matches_exact(attributes, key)); Ok(()) } diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index ee15273e9..5962172c5 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -897,10 +897,7 @@ async fn bulk_create_items() -> Result<(), Error> { true, )]) .await?; - assert_eq!( - keyring.search_items(&[("app", "bulk-app")]).await?.len(), - 4 - ); + assert_eq!(keyring.search_items(&[("app", "bulk-app")]).await?.len(), 4); // Repeating that exact coarse attribute set replaces only the coarse item. keyring @@ -911,10 +908,7 @@ async fn bulk_create_items() -> Result<(), Error> { true, )]) .await?; - assert_eq!( - keyring.search_items(&[("app", "bulk-app")]).await?.len(), - 4 - ); + assert_eq!(keyring.search_items(&[("app", "bulk-app")]).await?.len(), 4); Ok(()) } From 6d2bb9db194b671b6819b99e1491a1fa880fbf41 Mon Sep 17 00:00:00 2001 From: "Julian Y. Richard Corbet" <48553615+julian-corbet@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:00:39 +0200 Subject: [PATCH 3/3] file: align exact attribute matching --- client/src/file/mod.rs | 2 +- client/src/file/unlocked_item.rs | 5 +++++ client/tests/file_unlocked_keyring.rs | 15 ++++++--------- server/src/collection/mod.rs | 4 +++- server/src/collection/tests.rs | 9 +++------ 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/client/src/file/mod.rs b/client/src/file/mod.rs index 0ea6a1f67..0c521038c 100644 --- a/client/src/file/mod.rs +++ b/client/src/file/mod.rs @@ -101,7 +101,7 @@ impl Item { key: Option<&Key>, ) -> bool { match self { - Self::Unlocked(unlocked) => unlocked.attributes() == &attributes.as_attributes(), + Self::Unlocked(unlocked) => unlocked.matches_exact(attributes), Self::Locked(locked) => locked.inner.matches_exact(attributes, key), } } diff --git a/client/src/file/unlocked_item.rs b/client/src/file/unlocked_item.rs index 9cb3f50e1..52e8f5b55 100644 --- a/client/src/file/unlocked_item.rs +++ b/client/src/file/unlocked_item.rs @@ -62,6 +62,11 @@ impl UnlockedItem { &self.attributes } + /// Check whether the attribute maps match. + pub fn matches_exact(&self, attributes: &impl AsAttributes) -> bool { + self.attributes == attributes.as_attributes() + } + /// Retrieve the item attributes as a typed schema. /// /// # Example diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index 5962172c5..91b2f951a 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -550,9 +550,9 @@ async fn item_replacement_behavior() -> Result<(), Error> { } #[tokio::test] -async fn item_replacement_matches_attributes_exactly() -> Result<(), Error> { +async fn item_replacement_matches_attributes() -> Result<(), Error> { let temp_dir = tempdir().unwrap(); - let keyring_path = temp_dir.path().join("replace_exact_test.keyring"); + let keyring_path = temp_dir.path().join("replace_attributes_test.keyring"); let keyring = UnlockedKeyring::load(&keyring_path, Some(strong_key())).await?; keyring @@ -572,8 +572,7 @@ async fn item_replacement_matches_attributes_exactly() -> Result<(), Error> { ) .await?; - // A coarser attribute set is not the same attribute set, so it must not - // replace either of the more specific items. + // A coarser attribute set must not replace either item. keyring .create_item("Coarse", &[("app", "test")], "coarse-secret", true) .await?; @@ -595,8 +594,7 @@ async fn item_replacement_matches_attributes_exactly() -> Result<(), Error> { 1 ); - // Repeating the exact coarse attribute set still replaces its previous - // item rather than adding a duplicate. + // Replacing the coarse item must not add a duplicate. keyring .create_item( "Updated Coarse", @@ -887,8 +885,7 @@ async fn bulk_create_items() -> Result<(), Error> { let all_items_after = keyring.search_items(&[("app", "bulk-app")]).await?; assert_eq!(all_items_after.len(), 3); - // A bulk replacement follows the same exact-attribute rule as - // create_item: this coarser item must not remove the three specific ones. + // A coarse replacement must preserve the three specific items. keyring .create_items(vec![( "Coarse Item".to_string(), @@ -899,7 +896,7 @@ async fn bulk_create_items() -> Result<(), Error> { .await?; assert_eq!(keyring.search_items(&[("app", "bulk-app")]).await?.len(), 4); - // Repeating that exact coarse attribute set replaces only the coarse item. + // Replacing the coarse item removes only its predecessor. keyring .create_items(vec![( "Updated Coarse Item".to_string(), diff --git a/server/src/collection/mod.rs b/server/src/collection/mod.rs index 68d4f1a7d..460083a3f 100644 --- a/server/src/collection/mod.rs +++ b/server/src/collection/mod.rs @@ -541,7 +541,9 @@ impl Collection { for item_wrapper in items.iter() { let inner = item_wrapper.inner.lock().await; - let file_item = inner.as_ref().unwrap(); + let Some(file_item) = inner.as_ref() else { + continue; + }; if file_item.matches_attributes_exact(attributes, key) { matching_items.push(item_wrapper.clone()); diff --git a/server/src/collection/tests.rs b/server/src/collection/tests.rs index 4a1026630..a2dba90f5 100644 --- a/server/src/collection/tests.rs +++ b/server/src/collection/tests.rs @@ -227,8 +227,7 @@ async fn create_item_with_replace() -> Result<(), Box> { } #[tokio::test] -async fn create_item_with_replace_matches_attributes_exactly() --> Result<(), Box> { +async fn create_item_with_replace_matches_attributes() -> Result<(), Box> { let setup = TestServiceSetup::plain_session(true).await?; setup @@ -248,8 +247,7 @@ async fn create_item_with_replace_matches_attributes_exactly() ) .await?; - // A coarser attribute set is not the same attribute set, so it must not - // replace either of the more specific items. + // A coarser attribute set must not replace either item. setup .create_item( "Coarse", @@ -276,8 +274,7 @@ async fn create_item_with_replace_matches_attributes_exactly() 1 ); - // Repeating the exact coarse attribute set still replaces its previous - // item rather than adding a duplicate. + // Replacing the coarse item must not add a duplicate. setup .create_item( "Updated Coarse",