From 35f54d76fb91e76faa017325466989be1deef501 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:40:30 +0200 Subject: [PATCH] Fix client event whitelist and stored collection manage serialization The client event whitelist no longer drops the newer Bitwarden item-type and organization events that the previous range check still logged. The admin console collection lists echo a plain User's stored users_collections.manage bit again, so saving an unrelated dialog no longer strips it. --- src/api/core/events.rs | 61 +++++++++++++++++++++++++++++++---- src/db/models/collection.rs | 35 ++++++++++++++++++-- src/db/models/event.rs | 20 ++++++++++++ src/db/models/organization.rs | 5 +-- 4 files changed, 111 insertions(+), 10 deletions(-) diff --git a/src/api/core/events.rs b/src/api/core/events.rs index ba859d31..6aa6b954 100644 --- a/src/api/core/events.rs +++ b/src/api/core/events.rs @@ -247,6 +247,11 @@ fn validate_client_event_batch_size(event_count: usize) -> Result<(), crate::Err Ok(()) } +/// The client-generated event types upstream's `/events/collect` accepts. Anything else is ignored, +/// so that an authenticated client cannot write arbitrary event types into an organization's audit +/// log. Keep this in sync with upstream's `CollectController`: a type missing here is silently not +/// logged, which is why the newer item-type events below are listed explicitly rather than matched +/// by range. fn client_event_kind(event_type: i32) -> Option { match event_type { event_type if event_type == EventType::UserClientExportedVault as i32 => Some(ClientEventKind::User), @@ -259,11 +264,32 @@ fn client_event_kind(event_type: i32) -> Option { || event_type == EventType::CipherClientCopiedHiddenField as i32 || event_type == EventType::CipherClientCopiedCardCode as i32 || event_type == EventType::CipherClientAutofilled as i32 - || event_type == EventType::CipherClientToggledCardNumberVisible as i32 => + || event_type == EventType::CipherClientToggledCardNumberVisible as i32 + || event_type == EventType::CipherClientCopiedBankAccountNumber as i32 + || event_type == EventType::CipherClientCopiedBankAccountPin as i32 + || event_type == EventType::CipherClientToggledBankAccountNumberVisible as i32 + || event_type == EventType::CipherClientToggledBankAccountPinVisible as i32 + || event_type == EventType::CipherClientCopiedLicenseNumber as i32 + || event_type == EventType::CipherClientToggledLicenseNumberVisible as i32 + || event_type == EventType::CipherClientCopiedPassportNumber as i32 + || event_type == EventType::CipherClientToggledPassportNumberVisible as i32 + || event_type == EventType::CipherClientCopiedSwiftCode as i32 + || event_type == EventType::CipherClientToggledSwiftCodeVisible as i32 + || event_type == EventType::CipherClientCopiedIban as i32 + || event_type == EventType::CipherClientToggledIbanVisible as i32 + || event_type == EventType::CipherClientCopiedNationalIdentificationNumber as i32 + || event_type == EventType::CipherClientToggledNationalIdentificationNumberVisible as i32 => { Some(ClientEventKind::Cipher) } - event_type if event_type == EventType::OrganizationClientExportedVault as i32 => { + event_type + if event_type == EventType::OrganizationClientExportedVault as i32 + || event_type == EventType::OrganizationItemOrganizationAccepted as i32 + || event_type == EventType::OrganizationItemOrganizationDeclined as i32 + || event_type == EventType::OrganizationAutoConfirmEnabledAdmin as i32 + || event_type == EventType::OrganizationAutoConfirmDisabledAdmin as i32 + || event_type == EventType::OrganizationInviteLinkClientCopied as i32 => + { Some(ClientEventKind::Organization) } _ => None, @@ -584,13 +610,36 @@ mod tests { EventType::CipherClientCopiedCardCode, EventType::CipherClientAutofilled, EventType::CipherClientToggledCardNumberVisible, + EventType::CipherClientCopiedBankAccountNumber, + EventType::CipherClientCopiedBankAccountPin, + EventType::CipherClientToggledBankAccountNumberVisible, + EventType::CipherClientToggledBankAccountPinVisible, + EventType::CipherClientCopiedLicenseNumber, + EventType::CipherClientToggledLicenseNumberVisible, + EventType::CipherClientCopiedPassportNumber, + EventType::CipherClientToggledPassportNumberVisible, + EventType::CipherClientCopiedSwiftCode, + EventType::CipherClientToggledSwiftCodeVisible, + EventType::CipherClientCopiedIban, + EventType::CipherClientToggledIbanVisible, + EventType::CipherClientCopiedNationalIdentificationNumber, + EventType::CipherClientToggledNationalIdentificationNumberVisible, ] { assert_eq!(client_event_kind(event_type as i32), Some(ClientEventKind::Cipher)); } - assert_eq!( - client_event_kind(EventType::OrganizationClientExportedVault as i32), - Some(ClientEventKind::Organization) - ); + for event_type in [ + EventType::OrganizationClientExportedVault, + EventType::OrganizationItemOrganizationAccepted, + EventType::OrganizationItemOrganizationDeclined, + EventType::OrganizationAutoConfirmEnabledAdmin, + EventType::OrganizationAutoConfirmDisabledAdmin, + EventType::OrganizationInviteLinkClientCopied, + ] { + assert_eq!(client_event_kind(event_type as i32), Some(ClientEventKind::Organization)); + } + + // Upstream does not accept the TOTP seed toggle from clients either. + assert_eq!(client_event_kind(1118), None); for event_type in [ EventType::UserLoggedIn, diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index cdea34d6..e61e9bfc 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -55,6 +55,10 @@ pub struct CollectionCipher { /// Serialize the assignment-level `manage` capability using the same role boundary as the /// collection mutation guards. Read/write access is deliberately not management authority. +/// +/// This answers "may this member manage this collection?" and therefore belongs on the objects a +/// member receives about themselves. For the administrative lists that echo a *stored* grant back +/// to the client, use `stored_assignment_manage` instead. pub(super) fn assignment_manage_for_member(membership_type: i32, stored_manage: bool) -> bool { match MembershipType::from_i32(membership_type) { Some(MembershipType::Owner | MembershipType::Admin) => true, @@ -63,6 +67,18 @@ pub(super) fn assignment_manage_for_member(membership_type: i32, stored_manage: } } +/// Serialize a *stored* per-collection assignment row for the admin-console access lists. +/// +/// These lists describe the grant an administrator configured, and the client writes the very same +/// value back when the dialog is saved. Reporting anything other than the persisted bit would make +/// an unrelated save silently strip it — for a plain User that would also revoke the cipher write +/// access `users_collections.manage` still grants (see `Cipher::get_access_restrictions`). Admins +/// and Owners manage implicitly, so they are reported as managing regardless of the stored row. +pub(super) fn stored_assignment_manage(membership_type: i32, stored_manage: bool) -> bool { + matches!(MembershipType::from_i32(membership_type), Some(MembershipType::Owner | MembershipType::Admin)) + || stored_manage +} + /// Local methods impl Collection { pub fn new(org_uuid: OrganizationId, name: String, external_id: Option) -> Self { @@ -938,7 +954,7 @@ impl CollectionMembership { "id": self.membership_uuid, "readOnly": self.read_only, "hidePasswords": self.hide_passwords, - "manage": assignment_manage_for_member(membership_type, self.manage), + "manage": stored_assignment_manage(membership_type, self.manage), }) } } @@ -975,9 +991,24 @@ pub struct CollectionId(String); #[cfg(test)] mod tests { - use super::assignment_manage_for_member; + use super::{assignment_manage_for_member, stored_assignment_manage}; use crate::db::models::MembershipType; + // A stored `users_collections.manage` row must survive being listed in the admin console and + // written back unchanged. Reporting `false` for a plain User made an unrelated save strip the + // grant, which also revoked the cipher write access the row still confers. + #[test] + fn stored_assignment_manage_echoes_the_persisted_grant() { + for role in [MembershipType::Owner, MembershipType::Admin] { + assert!(stored_assignment_manage(role as i32, false)); + } + + for role in [MembershipType::Custom, MembershipType::User] { + assert!(stored_assignment_manage(role as i32, true)); + assert!(!stored_assignment_manage(role as i32, false)); + } + } + #[test] fn assignment_manage_matches_collection_guard_role_boundaries() { for role in [MembershipType::Owner, MembershipType::Admin] { diff --git a/src/db/models/event.rs b/src/db/models/event.rs index 3a6b610c..2554ddda 100644 --- a/src/db/models/event.rs +++ b/src/db/models/event.rs @@ -79,6 +79,21 @@ pub enum EventType { CipherSoftDeleted = 1115, CipherRestored = 1116, CipherClientToggledCardNumberVisible = 1117, + // CipherClientToggledTOTPSeedVisible = 1118, // Not accepted from clients by upstream either + CipherClientCopiedBankAccountNumber = 1119, + CipherClientCopiedBankAccountPin = 1120, + CipherClientToggledBankAccountNumberVisible = 1121, + CipherClientToggledBankAccountPinVisible = 1122, + CipherClientCopiedLicenseNumber = 1123, + CipherClientToggledLicenseNumberVisible = 1124, + CipherClientCopiedPassportNumber = 1125, + CipherClientToggledPassportNumberVisible = 1126, + CipherClientCopiedSwiftCode = 1127, + CipherClientToggledSwiftCodeVisible = 1128, + CipherClientCopiedIban = 1129, + CipherClientToggledIbanVisible = 1130, + CipherClientCopiedNationalIdentificationNumber = 1131, + CipherClientToggledNationalIdentificationNumberVisible = 1132, // Collection CollectionCreated = 1300, @@ -120,6 +135,11 @@ pub enum EventType { // OrganizationDisabledKeyConnector = 1607, // Not supported // OrganizationSponsorshipsSynced = 1608, // Not supported // OrganizationCollectionManagementUpdated = 1609, // Not supported + OrganizationItemOrganizationAccepted = 1618, + OrganizationItemOrganizationDeclined = 1619, + OrganizationAutoConfirmEnabledAdmin = 1620, + OrganizationAutoConfirmDisabledAdmin = 1621, + OrganizationInviteLinkClientCopied = 1627, // Policy PolicyUpdated = 1700, diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 4e63b2bc..3630baaf 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -25,7 +25,8 @@ use macros::UuidFromParam; use super::{ Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, - OrgPolicyType, TwoFactor, User, UserId, collection::assignment_manage_for_member as assignment_manage, + OrgPolicyType, TwoFactor, User, UserId, + collection::{assignment_manage_for_member as assignment_manage, stored_assignment_manage}, }; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -619,7 +620,7 @@ impl Membership { let (read_only, hide_passwords, manage) = if self.has_full_access() { (false, false, assignment_manage(self.atype, false)) } else if let Some(cu) = cu.get(&c.uuid) { - (cu.read_only, cu.hide_passwords, assignment_manage(self.atype, cu.manage)) + (cu.read_only, cu.hide_passwords, stored_assignment_manage(self.atype, cu.manage)) // If previous checks failed it might be that this user has access via a group, but we should not return those elements here // Those are returned via a special group endpoint } else if cg.contains(&c.uuid) {