diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ad7481..aa325609 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,9 @@ If you are upgrading from v0.16.x, replace the binary (or run `docker pull`). If ## Changed ## Fixed -- JMAP: Parsing ids containing digits in JSON Pointers. +- JMAP: + - Patching ids containing digits in JSON Pointers failes. + - Patching nested objects with `null` values fails. ## [0.16.4] - 2026-05-05 diff --git a/crates/jmap/src/api/acl.rs b/crates/jmap/src/api/acl.rs index 5ce90248..2667bf72 100644 --- a/crates/jmap/src/api/acl.rs +++ b/crates/jmap/src/api/acl.rs @@ -80,6 +80,12 @@ impl JmapRights { .document_id(); if let Some(right) = path.next() { + if path.next().is_some() { + return Err(SetError::invalid_properties() + .with_property(T::SHARE_WITH_PROPERTY) + .with_description("Invalid path for ACL patch.")); + } + let is_set = match value { Value::Bool(is_set) => is_set, Value::Null => false, @@ -110,7 +116,7 @@ impl JmapRights { if is_set { acl_item.grants.insert_many(acl); } else { - acl_item.grants.insert_many(acl); + acl_item.grants.remove_many(acl); if acl_item.grants.is_empty() { grants.retain(|item| item.account_id != account_id); } diff --git a/crates/registry/src/jmap/patch.rs b/crates/registry/src/jmap/patch.rs index 13c695d3..b3c92399 100644 --- a/crates/registry/src/jmap/patch.rs +++ b/crates/registry/src/jmap/patch.rs @@ -310,26 +310,24 @@ impl RegistryJsonPatch for VecMap { value: JmapValue<'x>, ) -> PatchResult<'x> { match (pointer.next(), value) { - (Some(JsonPointerItem::Number(idx)), Value::Null) => { + (Some(JsonPointerItem::Number(idx)), value) => { if let Some(key) = K::try_from_integer(*idx) { - self.remove(&key); - return Ok(MaybeUnpatched::Patched); - } - } - (Some(JsonPointerItem::Key(key)), Value::Null) => { - if let Some(key) = K::try_from_string(key.to_string().as_ref()) { - self.remove(&key); - return Ok(MaybeUnpatched::Patched); + return if matches!(value, Value::Null) && !pointer.has_next() { + self.remove(&key); + Ok(MaybeUnpatched::Patched) + } else { + self.get_mut_or_insert(key).patch(pointer, value) + }; } } (Some(JsonPointerItem::Key(key)), value) => { if let Some(key) = K::try_from_string(key.to_string().as_ref()) { - return self.get_mut_or_insert(key).patch(pointer, value); - } - } - (Some(JsonPointerItem::Number(idx)), value) => { - if let Some(key) = K::try_from_integer(*idx) { - return self.get_mut_or_insert(key).patch(pointer, value); + return if matches!(value, Value::Null) && !pointer.has_next() { + self.remove(&key); + Ok(MaybeUnpatched::Patched) + } else { + self.get_mut_or_insert(key).patch(pointer, value) + }; } } (None, Value::Object(items)) => { diff --git a/crates/registry/src/types/list.rs b/crates/registry/src/types/list.rs index 8c5d6c7c..ff7f289d 100644 --- a/crates/registry/src/types/list.rs +++ b/crates/registry/src/types/list.rs @@ -95,34 +95,31 @@ impl RegistryJsonPatch for List { value: JmapValue<'x>, ) -> PatchResult<'x> { match (pointer.next(), value) { - (Some(JsonPointerItem::Number(key)), Value::Null) - if self.0.remove(&(*key as u32)).is_some() => - { - return Ok(MaybeUnpatched::Patched); - } - (Some(JsonPointerItem::Key(key)), Value::Null) => { - if let Ok(key) = key.to_string().parse::() - && self.0.remove(&key).is_some() - { - return Ok(MaybeUnpatched::Patched); + (Some(JsonPointerItem::Number(key)), value) => { + let key = *key as u32; + if matches!(value, Value::Null) && !pointer.has_next() { + if self.0.remove(&key).is_some() { + return Ok(MaybeUnpatched::Patched); + } + } else { + let result = self.0.get_mut_or_insert(key).patch(pointer, value); + self.0.sort_unstable_by_key(); + return result; } } (Some(JsonPointerItem::Key(key)), value) => { if let Ok(key) = key.to_string().parse::() { - let result = self.0.get_mut_or_insert(key).patch(pointer, value); - - self.0.sort_unstable_by_key(); - - return result; + if matches!(value, Value::Null) && !pointer.has_next() { + if self.0.remove(&key).is_some() { + return Ok(MaybeUnpatched::Patched); + } + } else { + let result = self.0.get_mut_or_insert(key).patch(pointer, value); + self.0.sort_unstable_by_key(); + return result; + } } } - (Some(JsonPointerItem::Number(key)), value) => { - let result = self.0.get_mut_or_insert(*key as u32).patch(pointer, value); - - self.0.sort_unstable_by_key(); - - return result; - } (None, Value::Object(items)) => { self.0.clear(); for (key, value) in items.into_vec() { diff --git a/crates/registry/src/types/map.rs b/crates/registry/src/types/map.rs index 5f03e13d..44d8925f 100644 --- a/crates/registry/src/types/map.rs +++ b/crates/registry/src/types/map.rs @@ -140,19 +140,31 @@ impl RegistryJsonPatch for Map { ) -> PatchResult<'x> { match (pointer.next(), value) { (Some(JsonPointerItem::Number(idx)), Value::Null | Value::Bool(false)) => { - if let Some(key) = T::try_from_integer(*idx) { + let key = T::try_from_integer(*idx); + + if !pointer.has_next() + && let Some(key) = key + { self.0.retain(|item| item != &key); return Ok(MaybeUnpatched::Patched); } } (Some(JsonPointerItem::Key(key)), Value::Null | Value::Bool(false)) => { - if let Some(key) = T::try_from_string(key.to_string().as_ref()) { + let key = T::try_from_string(key.to_string().as_ref()); + + if !pointer.has_next() + && let Some(key) = key + { self.0.retain(|item| item != &key); return Ok(MaybeUnpatched::Patched); } } (Some(JsonPointerItem::Key(key)), Value::Bool(true)) => { - if let Some(key) = T::try_from_string(key.to_string().as_ref()) { + let key = T::try_from_string(key.to_string().as_ref()); + + if !pointer.has_next() + && let Some(key) = key + { if !self.0.contains(&key) { self.0.push(key); } @@ -161,7 +173,10 @@ impl RegistryJsonPatch for Map { } } (Some(JsonPointerItem::Number(idx)), Value::Bool(true)) => { - if let Some(key) = T::try_from_integer(*idx) { + let key = T::try_from_integer(*idx); + if !pointer.has_next() + && let Some(key) = key + { if !self.0.contains(&key) { self.0.push(key); } diff --git a/crates/utils/src/map/bitmap.rs b/crates/utils/src/map/bitmap.rs index 51d86ae3..ced6e201 100644 --- a/crates/utils/src/map/bitmap.rs +++ b/crates/utils/src/map/bitmap.rs @@ -80,6 +80,13 @@ impl Bitmap { } } + pub fn remove_many(&mut self, items: impl IntoIterator) { + for item in items.into_iter() { + debug_assert!(item.is_valid()); + self.bitmap &= !(1 << item.into()); + } + } + #[inline(always)] pub fn with_item(mut self, item: T) -> Self { self.insert(item); diff --git a/tests/src/store/registry.rs b/tests/src/store/registry.rs index 02bdabc8..6a3f3b38 100644 --- a/tests/src/store/registry.rs +++ b/tests/src/store/registry.rs @@ -41,6 +41,8 @@ pub async fn test(test: &TestServer) { println!("Registry tests..."); + test_patch_regressions(); + // Pickle-unpickle test let mut account = Account::User(UserAccount { aliases: List::from_iter([ @@ -539,6 +541,200 @@ impl TestServer { } } +fn test_patch_regressions() { + fn fresh_account() -> Account { + Account::User(UserAccount { + credentials: List::from_iter([ + Credential::Password(PasswordCredential { + allowed_ips: Map::new(vec![ + IpAddrOrMask::from_str("192.168.1.1").unwrap(), + IpAddrOrMask::from_str("192.168.1.2").unwrap(), + ]), + credential_id: 3u64.into(), + expires_at: None, + otp_auth: None, + secret: "secret".into(), + }), + Credential::Password(PasswordCredential { + allowed_ips: Map::new(vec![IpAddrOrMask::from_str("10.0.0.1").unwrap()]), + credential_id: 4u64.into(), + expires_at: None, + otp_auth: None, + secret: "another".into(), + }), + ]), + domain_id: 1u64.into(), + name: "patch-target".into(), + ..Default::default() + }) + } + + fn user(account: &Account) -> &UserAccount { + match account { + Account::User(u) => u, + _ => panic!("expected user account"), + } + } + + fn user_mut(account: &mut Account) -> &mut UserAccount { + match account { + Account::User(u) => u, + _ => panic!("expected user account"), + } + } + + fn password_at(account: &Account, idx: u32) -> &PasswordCredential { + let cred = user(account) + .credentials + .0 + .get(&idx) + .expect("credential at index"); + match cred { + Credential::Password(p) => p, + _ => panic!("expected password credential at idx {idx}"), + } + } + + // Leaf-null patch into a List entry removes only the leaf not the whole entry. + let mut account = fresh_account(); + account.assert_patch( + "credentials/0/allowedIps/192.168.1.1", + JmapValue::Null, + trc::location!(), + ); + { + let cred = password_at(&account, 0); + assert_eq!(cred.allowed_ips.len(), 1, "one ip should remain"); + assert!( + cred.allowed_ips + .contains(&IpAddrOrMask::from_str("192.168.1.2").unwrap()), + "remaining ip survived" + ); + assert!( + !cred + .allowed_ips + .contains(&IpAddrOrMask::from_str("192.168.1.1").unwrap()), + "targeted ip removed" + ); + } + // The sibling credential is untouched. + { + let cred = password_at(&account, 1); + assert_eq!(cred.allowed_ips.len(), 1); + assert!( + cred.allowed_ips + .contains(&IpAddrOrMask::from_str("10.0.0.1").unwrap()) + ); + } + + // Removing every leaf still leaves the entry in place with an empty map. + let mut account = fresh_account(); + account.assert_patch( + "credentials/0/allowedIps/192.168.1.1", + JmapValue::Null, + trc::location!(), + ); + account.assert_patch( + "credentials/0/allowedIps/192.168.1.2", + JmapValue::Null, + trc::location!(), + ); + { + assert_eq!( + user(&account).credentials.len(), + 2, + "credential entry retained" + ); + let cred = password_at(&account, 0); + assert!(cred.allowed_ips.is_empty(), "leaf map drained"); + } + + // Direct removal of a list entry with no remaining segments still works. + let mut account = fresh_account(); + account.assert_patch("credentials/0", JmapValue::Null, trc::location!()); + { + assert_eq!(user(&account).credentials.len(), 1, "credential 0 removed"); + let cred = password_at(&account, 1); + assert_eq!(cred.allowed_ips.len(), 1); + } + + // Leaf-null patch into a scalar property of a list entry clears only that property. + let mut account = fresh_account(); + { + let cred = user_mut(&mut account) + .credentials + .inner_mut() + .get_mut(&0) + .expect("credential at 0"); + if let Credential::Password(p) = cred { + p.expires_at = Some(UTCDateTime::from_timestamp(now() as i64)); + } + } + account.assert_patch("credentials/0/expiresAt", JmapValue::Null, trc::location!()); + { + let cred = password_at(&account, 0); + assert!(cred.expires_at.is_none(), "expiresAt cleared"); + assert_eq!(cred.allowed_ips.len(), 2, "siblings untouched"); + } + + // Map set-style patches + fn account_with_groups() -> Account { + let mut account = match fresh_account() { + Account::User(u) => u, + _ => unreachable!(), + }; + account.member_group_ids = Map::new(vec![Id::new(2000), Id::new(2001)]); + Account::User(account) + } + + let mut account = account_with_groups(); + account.assert_patch( + &format!("memberGroupIds/{}", Id::new(2000)), + JmapValue::Null, + trc::location!(), + ); + assert_eq!( + user(&account).member_group_ids.len(), + 1, + "one member removed" + ); + assert!( + user(&account).member_group_ids.contains(&Id::new(2001)), + "sibling preserved" + ); + + let mut account = account_with_groups(); + let extra_path = format!("memberGroupIds/{}/extra", Id::new(2000)); + let ptr = JsonPointer::parse(&extra_path); + let outcome = account.patch(JsonPointerPatch::new(&ptr), JmapValue::Null); + assert!(outcome.is_err(), "extra segments must error on remove"); + assert_eq!( + user(&account).member_group_ids.len(), + 2, + "membership unchanged after rejected patch" + ); + + let mut account = account_with_groups(); + let ptr = JsonPointer::parse(&format!("memberGroupIds/{}/extra", Id::new(2002))); + let outcome = account.patch(JsonPointerPatch::new(&ptr), JmapValue::Bool(true)); + assert!(outcome.is_err(), "extra segments must error on add"); + assert_eq!( + user(&account).member_group_ids.len(), + 2, + "membership unchanged after rejected add" + ); + + // Direct adds and removes still work. + let mut account = account_with_groups(); + account.assert_patch( + &format!("memberGroupIds/{}", Id::new(2002)), + true, + trc::location!(), + ); + assert_eq!(user(&account).member_group_ids.len(), 3, "member added"); + assert!(user(&account).member_group_ids.contains(&Id::new(2002))); +} + trait AssertPatch { fn assert_patch(&mut self, patch: &str, value: impl Into>, location: &str); }