From 7a96caafaa6e8d6f6cd11c50fa8f1a93e9e1310a Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 16:56:36 -0700 Subject: [PATCH 01/21] IMAP: bound CONDSTORE missing-message work by the mailbox, not the request STORE ... (UNCHANGEDSINCE n) worked out which requested messages no longer exist by expanding every number in the client's sequence set into a hash set, so a single "UID STORE 1:4294967295 (UNCHANGEDSINCE 1)" from any logged-in user asked the server for billions of entries while it held the mailbox state lock. The missing set is now computed as ranges. Numbers up to the mailbox's highest UID (or message count) are checked one by one; everything above it is missing by definition and is emitted as a single range without being walked. MODIFIED carries ranges and is serialized as a sequence set, so its output is unchanged. Sequence::expand had no other caller and is removed. Tests: a range test that failed on main (expand(10) of 1:5000000 returned 5,000,000 entries) now passes against the new code, plus a reference comparison of the range result against per-number expansion. --- crates/imap-proto/src/lib.rs | 4 +- crates/imap-proto/src/protocol/mod.rs | 188 ++++++++++++++++++++------ crates/imap/src/core/message.rs | 42 +++--- crates/imap/src/op/store.rs | 13 +- 4 files changed, 176 insertions(+), 71 deletions(-) diff --git a/crates/imap-proto/src/lib.rs b/crates/imap-proto/src/lib.rs index a8d8b49b..432c7469 100644 --- a/crates/imap-proto/src/lib.rs +++ b/crates/imap-proto/src/lib.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use protocol::ObjectId; @@ -159,7 +161,7 @@ pub enum ResponseCode { // CONDSTORE Modified { - ids: Vec, + ranges: Vec<(u32, u32)>, }, HighestModseq { modseq: u64, diff --git a/crates/imap-proto/src/protocol/mod.rs b/crates/imap-proto/src/protocol/mod.rs index d3f4dab2..a31c4afe 100644 --- a/crates/imap-proto/src/protocol/mod.rs +++ b/crates/imap-proto/src/protocol/mod.rs @@ -2,14 +2,15 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{Command, ResponseCode, ResponseType, StatusResponse}; -use ahash::AHashSet; use base64::{Engine, engine::general_purpose::STANDARD}; use chrono::{DateTime, Utc}; use compact_str::CompactString; -use std::{cmp::Ordering, fmt::Display}; +use std::fmt::Display; use types::id::Id; use types::keyword::{ArchivedKeyword, Keyword}; use utils::chained_bytes::SliceRange; @@ -142,54 +143,52 @@ impl Sequence { } } - pub fn expand(&self, max_value: u32) -> AHashSet { - match self { - Sequence::Number { value } => AHashSet::from_iter([*value]), - Sequence::List { items } => { - let mut result = AHashSet::with_capacity(items.len()); - for item in items { - match item { - Sequence::Number { value } => { - result.insert(*value); - } - Sequence::Range { start, end } => { - let start = start.unwrap_or(max_value); - let end = end.unwrap_or(max_value); - match start.cmp(&end) { - Ordering::Equal => { - result.insert(start); - } - Ordering::Less => { - result.extend(start..=end); - } - Ordering::Greater => { - result.extend(end..=start); - } - } - } - _ => (), + /// The parts of this sequence that name no existing item, as sorted, + /// merged, inclusive ranges. `*` resolves to `max_value`; numbers up to + /// `max_value` are missing when `exists` says so, and every number above + /// it is missing. The part above `max_value` is never walked, so the cost + /// is bounded by `max_value` and the number of items in the command, not + /// by the numbers in it. + pub fn missing_ranges(&self, max_value: u32, exists: impl Fn(u32) -> bool) -> Vec<(u32, u32)> { + let mut missing = Vec::new(); + for (low, high) in self.ranges(max_value) { + let walk_high = high.min(max_value); + if low <= walk_high { + for value in low..=walk_high { + if !exists(value) { + missing.push((value, value)); } } - result } + if high > max_value { + missing.push((low.max(max_value + 1), high)); + } + } + merge_ranges(missing) + } + + /// The numbers this sequence covers, as sorted, merged, inclusive ranges. + /// `*` resolves to `max_value`; saved searches are not included. + pub fn ranges(&self, max_value: u32) -> Vec<(u32, u32)> { + let mut ranges = Vec::new(); + self.collect_ranges(max_value, &mut ranges); + merge_ranges(ranges) + } + + fn collect_ranges(&self, max_value: u32, ranges: &mut Vec<(u32, u32)>) { + match self { + Sequence::Number { value } => ranges.push((*value, *value)), Sequence::Range { start, end } => { - let mut result = AHashSet::new(); let start = start.unwrap_or(max_value); let end = end.unwrap_or(max_value); - match start.cmp(&end) { - Ordering::Equal => { - result.insert(start); - } - Ordering::Less => { - result.extend(start..=end); - } - Ordering::Greater => { - result.extend(end..=start); - } - } - result + ranges.push((start.min(end), start.max(end))); } - _ => AHashSet::new(), + Sequence::List { items } => { + for item in items { + item.collect_ranges(max_value, ranges); + } + } + Sequence::SavedSearch => (), } } } @@ -539,9 +538,9 @@ impl ResponseCode { ResponseCode::UidValidity => b"UIDVALIDITY", ResponseCode::Unavailable => b"UNAVAILABLE", ResponseCode::UnknownCte => b"UNKNOWN-CTE", - ResponseCode::Modified { ids } => { + ResponseCode::Modified { ranges } => { buf.extend_from_slice(b"MODIFIED "); - serialize_sequence(buf, ids); + serialize_ranges(buf, ranges); return; } ResponseCode::ObjectId(object_id) => { @@ -733,6 +732,32 @@ impl ProtocolVersion { } } +/// Sorts inclusive ranges and merges the ones that overlap or touch. +pub fn merge_ranges(mut ranges: Vec<(u32, u32)>) -> Vec<(u32, u32)> { + ranges.sort_unstable(); + let mut merged: Vec<(u32, u32)> = Vec::with_capacity(ranges.len()); + for (low, high) in ranges { + match merged.last_mut() { + Some(last) if low <= last.1.saturating_add(1) => last.1 = last.1.max(high), + _ => merged.push((low, high)), + } + } + merged +} + +pub fn serialize_ranges(buf: &mut Vec, ranges: &[(u32, u32)]) { + for (pos, (low, high)) in ranges.iter().enumerate() { + if pos > 0 { + buf.push(b','); + } + buf.extend_from_slice(low.to_string().as_bytes()); + if high != low { + buf.push(b':'); + buf.extend_from_slice(high.to_string().as_bytes()); + } + } +} + pub fn serialize_sequence(buf: &mut Vec, list: &[u32]) { let mut ids = list.iter().peekable(); while let Some(&id) = ids.next() { @@ -822,6 +847,79 @@ mod tests { use crate::protocol::{ObjectId, SerializeResponse}; use types::id::Id; + #[test] + fn sequence_work_is_bounded_by_max_value() { + // A client-sent range far above the mailbox's highest UID used to be + // expanded into a set holding every number in it. + let sequence = parse_sequence_set(b"1:4294967295").unwrap(); + assert_eq!( + sequence.missing_ranges(10, |uid| uid % 2 == 0), + vec![(1, 1), (3, 3), (5, 5), (7, 7), (9, 9), (11, 4294967295)] + ); + let sequence = parse_sequence_set(b"4294967295,5:*,4000000000:*").unwrap(); + // `*` is 10, so `4000000000:*` covers 10 through 4000000000. + assert_eq!( + sequence.ranges(10), + vec![(5, 4000000000), (4294967295, 4294967295)] + ); + assert_eq!(sequence.missing_ranges(u32::MAX, |_| true), vec![]); + } + + #[test] + fn sequence_missing_ranges_match_expansion() { + // Reference: expand every number, then keep the missing ones. + fn reference(sequence: &super::Sequence, max: u32, exists: &[u32]) -> Vec { + let mut out = Vec::new(); + for value in 0..=max + 20 { + if sequence.contains(value, max) && !exists.contains(&value) { + out.push(value); + } + } + out + } + fn flatten(ranges: &[(u32, u32)]) -> Vec { + ranges.iter().flat_map(|(low, high)| *low..=*high).collect() + } + let exists = [1, 2, 3, 5, 8, 13]; + for sequence in [ + "1", + "*", + "1:*", + "*:1", + "4:2", + "2:4,6:9,3", + "1:3,5,7:*", + "20:15", + "9,9,9", + "13:*,1", + "0:3", + ] { + let parsed = match parse_sequence_set(sequence.as_bytes()) { + Ok(parsed) => parsed, + Err(_) => continue, + }; + for max in [0, 1, 13, 15] { + let exists = exists + .iter() + .copied() + .filter(|v| *v <= max) + .collect::>(); + assert_eq!( + flatten(&parsed.missing_ranges(max, |v| exists.contains(&v))), + reference(&parsed, max, &exists), + "sequence {sequence:?}, max {max}" + ); + } + } + } + + #[test] + fn modified_ranges_serialize() { + let mut buf = Vec::new(); + super::serialize_ranges(&mut buf, &[(1, 1), (3, 7), (9, 9)]); + assert_eq!(String::from_utf8(buf).unwrap(), "1,3:7,9"); + } + #[test] fn serialize_error_uses_command_tag() { for (error, expected) in [ diff --git a/crates/imap/src/core/message.rs b/crates/imap/src/core/message.rs index 9eedb917..f090dfeb 100644 --- a/crates/imap/src/core/message.rs +++ b/crates/imap/src/core/message.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::{ @@ -11,7 +13,7 @@ use crate::core::ImapId; use ahash::AHashMap; use common::network::SessionStream; use email::cache::MessageCacheFetch; -use imap_proto::protocol::{Sequence, expunge, select::Exists}; +use imap_proto::protocol::{Sequence, expunge, merge_ranges, select::Exists}; use std::collections::BTreeMap; use store::{ValueKey, roaring::RoaringBitmap, write::ValueClass}; use trc::AddContext; @@ -280,33 +282,33 @@ impl SelectedMailbox { uids } - pub async fn sequence_expand_missing(&self, sequence: &Sequence, is_uid: bool) -> Vec { - let mut deleted_ids = Vec::new(); + pub async fn sequence_missing_ranges( + &self, + sequence: &Sequence, + is_uid: bool, + ) -> Vec<(u32, u32)> { if !sequence.is_saved_search() { let state = self.state.lock(); if is_uid { - for uid in sequence.expand(state.uid_max) { - if !state.uid_to_id.contains_key(&uid) { - deleted_ids.push(uid); - } - } + sequence.missing_ranges(state.uid_max, |uid| state.uid_to_id.contains_key(&uid)) } else { - for seqnum in sequence.expand(state.total_messages as u32) { - if seqnum > state.total_messages as u32 { - deleted_ids.push(seqnum); - } - } + sequence.missing_ranges(state.total_messages as u32, |_| true) } } else if let Some(saved_ids) = self.get_saved_search().await { let state = self.state.lock(); - for id in saved_ids.iter() { - if !state.uid_to_id.contains_key(&id.uid) { - deleted_ids.push(if is_uid { id.uid } else { id.seqnum }); - } - } + merge_ranges( + saved_ids + .iter() + .filter(|id| !state.uid_to_id.contains_key(&id.uid)) + .map(|id| { + let value = if is_uid { id.uid } else { id.seqnum }; + (value, value) + }) + .collect(), + ) + } else { + Vec::new() } - deleted_ids.sort_unstable(); - deleted_ids } pub fn append_messages(&self, ids: Vec, modseq: Option) { diff --git a/crates/imap/src/op/store.rs b/crates/imap/src/op/store.rs index a97e509a..7491871b 100644 --- a/crates/imap/src/op/store.rs +++ b/crates/imap/src/op/store.rs @@ -22,6 +22,7 @@ use imap_proto::{ protocol::{ Flag, ImapResponse, fetch::{DataItem, FetchItem}, + merge_ranges, store::{Arguments, Operation, Response}, }, receiver::Request, @@ -161,7 +162,7 @@ impl SessionData { .imap_ctx(&arguments.tag, trc::location!())?; let mut modified = mailbox - .sequence_expand_missing(&arguments.sequence_set, is_uid) + .sequence_missing_ranges(&arguments.sequence_set, is_uid) .await; // Add all IDs that changed in this mailbox @@ -175,9 +176,9 @@ impl SessionData { }) { if let Some(imap_id) = ids.remove(&id) { if is_uid { - modified.push(imap_id.uid); + modified.push((imap_id.uid, imap_id.uid)); } else { - modified.push(imap_id.seqnum); + modified.push((imap_id.seqnum, imap_id.seqnum)); if is_delete { unchanged_failed = true; } @@ -186,8 +187,10 @@ impl SessionData { } if !modified.is_empty() { - modified.sort_unstable(); - response_code = ResponseCode::Modified { ids: modified }.into(); + response_code = ResponseCode::Modified { + ranges: merge_ranges(modified), + } + .into(); } } -- 2.54.0 From e8d29ac15ed60fe06e895795d67cb568380afe9c Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:01:37 -0700 Subject: [PATCH 02/21] JMAP: place each create once when ordering creation references The topological sort of creation references (#id references between creates in one /set or Blob/upload) never remembered which creates it had already placed. When creates share references, e.g. each one referring to the next two, it walked the same nodes again and again, about fib(n) times: a request with 60-80 such creates kept a worker busy indefinitely. The 1000-deep stack check only catches cycles, and this graph has none. A create that has already been placed is now skipped. Its references were placed before it, so the order is unchanged. Test: sorting a 60-create chain did not finish within 2 seconds on main; it now returns at once, with every create after the ones it references. --- crates/jmap-proto/src/references/mod.rs | 38 +++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/crates/jmap-proto/src/references/mod.rs b/crates/jmap-proto/src/references/mod.rs index f4fa820f..c49a4bae 100644 --- a/crates/jmap-proto/src/references/mod.rs +++ b/crates/jmap-proto/src/references/mod.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use compact_str::format_compact; @@ -44,6 +46,12 @@ fn topological_sort( 'main: loop { while let Some(from_id) = it.next() { + // Already placed: its references were placed before it, so + // walking them again only repeats work (exponentially so when + // creates share references). + if !create.contains_key(from_id) { + continue; + } if let Some(to_ids) = graph.get(from_id) { it_stack.push((it, from_id)); if it_stack.len() > 1000 { @@ -85,6 +93,36 @@ fn topological_sort( #[cfg(test)] mod tests { + #[test] + fn topological_sort_visits_each_create_once() { + // b0 -> {b1, b2}, b1 -> {b2, b3}, ...: an acyclic graph that a walk + // without a visited set traverses about fib(n) times. + const N: usize = 60; + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let mut create = utils::map::vec_map::VecMap::new(); + let mut graph = std::collections::HashMap::new(); + for i in 0..N { + create.append(format!("b{i}"), i); + let refs = [i + 1, i + 2] + .into_iter() + .filter(|j| *j < N) + .map(|j| format!("b{j}")) + .collect::>(); + if !refs.is_empty() { + graph.insert(format!("b{i}"), refs); + } + } + let sorted = super::topological_sort(&mut create, graph).unwrap(); + let _ = tx.send(sorted.into_iter().map(|(_, v)| v).collect::>()); + }); + let order = rx + .recv_timeout(std::time::Duration::from_secs(2)) + .expect("topological sort did not finish within 2 seconds"); + // Every create comes after the ones it references. + assert_eq!(order, (0..N).rev().collect::>()); + } + use crate::references::Graph; use crate::references::eval::EvalObjectReferences; use crate::{ -- 2.54.0 From 062b3a6d0ce452173b482c5163c733d35c26e382 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:03:49 -0700 Subject: [PATCH 03/21] Bitmap::remove clears the bit instead of toggling it remove() XORed the bit, so removing an item that wasn't set added it. IMAP SETACL with "-" removes rights one at a time through it, so "SETACL mailbox user -x" against a user without "x" granted them "x". The JMAP callers remove from a full set, where both forms agree. Test: removing an unset item left it set on main; it now stays unset. --- crates/utils/src/map/bitmap.rs | 49 +++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/crates/utils/src/map/bitmap.rs b/crates/utils/src/map/bitmap.rs index ced6e201..11c911fa 100644 --- a/crates/utils/src/map/bitmap.rs +++ b/crates/utils/src/map/bitmap.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use std::ops::Deref; @@ -96,7 +98,8 @@ impl Bitmap { #[inline(always)] pub fn remove(&mut self, item: T) { debug_assert!(item.is_valid()); - self.bitmap ^= 1 << item.into(); + // inbuxa: clear the bit; XOR would set it when it wasn't set. + self.bitmap &= !(1 << item.into()); } #[inline(always)] @@ -314,3 +317,47 @@ impl Default for Bitmap { } } } + +#[cfg(test)] +mod tests { + use super::{Bitmap, BitmapItem}; + + #[derive(Clone, Copy)] + struct Bit(u64); + + impl From for Bit { + fn from(value: u64) -> Self { + Bit(value) + } + } + + impl From for u64 { + fn from(bit: Bit) -> Self { + bit.0 + } + } + + impl BitmapItem for Bit { + fn max() -> u64 { + 64 + } + + fn is_valid(&self) -> bool { + self.0 < 64 + } + } + + #[test] + fn remove_never_adds() { + // IMAP SETACL "-x" removes rights one by one: removing a right that + // isn't held must leave it unheld, not grant it. + let mut bitmap = Bitmap::::new(); + bitmap.insert(Bit(1)); + bitmap.remove(Bit(5)); + assert!(!bitmap.contains(Bit(5))); + assert!(bitmap.contains(Bit(1))); + bitmap.remove(Bit(1)); + bitmap.remove(Bit(1)); + assert!(bitmap.is_empty()); + } +} -- 2.54.0 From bac7578a6dd0e78a69771f80f9ad50a241f4d592 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 04/21] IMAP: CLOSE removes messages only with the rights EXPUNGE needs --- crates/imap/src/op/close.rs | 38 +++++++++++++++++++++++++++++++++++-- tests/src/imap/acl.rs | 14 ++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/crates/imap/src/op/close.rs b/crates/imap/src/op/close.rs index b5a938ff..2699d30b 100644 --- a/crates/imap/src/op/close.rs +++ b/crates/imap/src/op/close.rs @@ -2,21 +2,32 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use std::time::Instant; -use crate::core::{Session, State}; +use crate::core::{SelectedMailbox, Session, SessionData, State}; use common::network::SessionStream; use imap_proto::{Command, StatusResponse, receiver::Request}; +use registry::schema::enums::Permission; use trc::AddContext; +use types::acl::Acl; impl Session { pub async fn handle_close(&mut self, request: Request) -> trc::Result<()> { let op_start = Instant::now(); let (data, mailbox) = self.state.select_data(); - if mailbox.is_select { + // inbuxa: CLOSE removes \Deleted messages the way EXPUNGE does, so it + // takes the same rights. Without them the mailbox is closed and + // nothing is removed (RFC 3501 CLOSE has no failure response). + if mailbox.is_select + && may_expunge(&data, &mailbox) + .await + .caused_by(trc::location!())? + { data.expunge(mailbox.clone(), None, u32::MAX, op_start) .await .caused_by(trc::location!())?; @@ -39,3 +50,26 @@ impl Session { .await } } + +// inbuxa: the checks EXPUNGE makes before removing anything: the ImapExpunge +// permission, a delegation that may delete (AL-6), and RemoveItems on the +// mailbox. +async fn may_expunge( + data: &SessionData, + mailbox: &SelectedMailbox, +) -> trc::Result { + let access_token = data.refresh_access_token().await?; + if !access_token.has_permission(Permission::ImapExpunge) + || access_token + .delegation(mailbox.id.account_id) + .is_some_and(|delegation| !delegation.access.may_destroy()) + { + return Ok(false); + } + data.check_mailbox_acl( + mailbox.id.account_id, + mailbox.id.mailbox_id, + Acl::RemoveItems, + ) + .await +} diff --git a/tests/src/imap/acl.rs b/tests/src/imap/acl.rs index dcc5805b..9675cf87 100644 --- a/tests/src/imap/acl.rs +++ b/tests/src/imap/acl.rs @@ -191,6 +191,20 @@ pub async fn test( imap_john.send("UID EXPUNGE").await; imap_john.assert_read(Type::Tagged, ResponseType::No).await; + imap_john.send("UID FETCH 1 (PREVIEW)").await; + imap_john + .assert_read(Type::Tagged, ResponseType::Ok) + .await + .assert_contains("contents"); + + // inbuxa: CLOSE expunges too, so it needs the same rights as EXPUNGE. + // Without them the mailbox is closed and nothing is removed. + imap_john.send("CLOSE").await; + imap_john.assert_read(Type::Tagged, ResponseType::Ok).await; + imap_john + .send("SELECT \"Shared Folders/jane.smith@example.com/INBOX\"") + .await; + imap_john.assert_read(Type::Tagged, ResponseType::Ok).await; imap_john.send("UID FETCH 1 (PREVIEW)").await; imap_john .assert_read(Type::Tagged, ResponseType::Ok) -- 2.54.0 From 328b983cc2639e8a92080f1dfc7c0590c7ef5326 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 05/21] iTIP: accept organizer-only methods only from the organizer --- crates/dav/src/calendar/mod.rs | 3 + crates/groupware/src/calendar/itip.rs | 6 ++ crates/groupware/src/scheduling/inbound.rs | 4 ++ crates/groupware/src/scheduling/mod.rs | 7 +++ crates/groupware/src/scheduling/snapshot.rs | 27 +++++++- tests/resources/itip/itip_sender_role.txt | 70 +++++++++++++++++++++ 6 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 tests/resources/itip/itip_sender_role.txt diff --git a/crates/dav/src/calendar/mod.rs b/crates/dav/src/calendar/mod.rs index 064d3762..fdb0178a 100644 --- a/crates/dav/src/calendar/mod.rs +++ b/crates/dav/src/calendar/mod.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ pub mod copy_move; @@ -130,6 +132,7 @@ impl ItipPrecondition for ItipError { ItipError::MultipleOrganizer => Some(CalCondition::SameOrganizerInAllComponents), ItipError::OrganizerIsLocalAddress | ItipError::SenderIsNotParticipant(_) + | ItipError::SenderIsNotOrganizer(_) | ItipError::OrganizerMismatch => Some(CalCondition::ValidOrganizer), ItipError::CannotModifyProperty(_) | ItipError::CannotModifyInstance diff --git a/crates/groupware/src/calendar/itip.rs b/crates/groupware/src/calendar/itip.rs index d08d484f..301c7630 100644 --- a/crates/groupware/src/calendar/itip.rs +++ b/crates/groupware/src/calendar/itip.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -155,6 +157,10 @@ impl ItipIngest for Server { ItipError::SenderIsNotOrganizerNorAttendee, )); } + // inbuxa: also covers events the recipient doesn't have yet + itip_snapshots + .assert_organizer_sends(itip_method(&itip)?, sender) + .map_err(ItipIngestError::Message)?; // Obtain changedBy let changed_by = if let Some(id) = self.account_id_from_email(sender, true).await? { diff --git a/crates/groupware/src/scheduling/inbound.rs b/crates/groupware/src/scheduling/inbound.rs index 21521aac..175d3829 100644 --- a/crates/groupware/src/scheduling/inbound.rs +++ b/crates/groupware/src/scheduling/inbound.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::scheduling::{ @@ -88,6 +90,8 @@ pub fn itip_process_message( _ => return Err(ItipError::UnsupportedMethod(method.clone())), } } else { + itip_snapshots.assert_organizer_sends(method, &sender)?; + // Handle organizer and attendees updates match method { ICalendarMethod::Request => { diff --git a/crates/groupware/src/scheduling/mod.rs b/crates/groupware/src/scheduling/mod.rs index cdc7cb8c..4587fdce 100644 --- a/crates/groupware/src/scheduling/mod.rs +++ b/crates/groupware/src/scheduling/mod.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{decode_mailto_address, extract_addr_spec}; @@ -141,6 +143,7 @@ pub enum ItipError { OrganizerIsLocalAddress, SenderIsNotOrganizerNorAttendee, SenderIsNotParticipant(String), + SenderIsNotOrganizer(String), UnknownParticipant(String), UnsupportedMethod(ICalendarMethod), ICalendarParseError, @@ -368,6 +371,7 @@ impl ItipError { ItipError::MultipleOrganizer | ItipError::OrganizerIsLocalAddress | ItipError::SenderIsNotParticipant(_) + | ItipError::SenderIsNotOrganizer(_) | ItipError::OrganizerMismatch | ItipError::CannotModifyProperty(_) | ItipError::CannotModifyInstance @@ -424,6 +428,9 @@ impl Display for ItipError { ItipError::SenderIsNotParticipant(participant) => { write!(f, "Sender {participant:?} is not a participant") } + ItipError::SenderIsNotOrganizer(sender) => { + write!(f, "Sender {sender:?} is not the organizer") + } ItipError::SenderIsNotOrganizerNorAttendee => { write!(f, "Sender is neither organizer nor attendee") } diff --git a/crates/groupware/src/scheduling/snapshot.rs b/crates/groupware/src/scheduling/snapshot.rs index c2624319..36deab6c 100644 --- a/crates/groupware/src/scheduling/snapshot.rs +++ b/crates/groupware/src/scheduling/snapshot.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::scheduling::{ @@ -10,7 +12,7 @@ use crate::scheduling::{ }; use ahash::AHashMap; use calcard::icalendar::{ - ICalendar, ICalendarParameterName, ICalendarParameterValue, ICalendarProperty, + ICalendar, ICalendarMethod, ICalendarParameterName, ICalendarParameterValue, ICalendarProperty, ICalendarScheduleAgentValue, ICalendarValue, Uri, }; @@ -362,6 +364,29 @@ pub fn itip_snapshot<'x, 'y>( } impl ItipSnapshots<'_> { + // inbuxa: REQUEST, ADD, CANCEL and DECLINECOUNTER are the organizer's + // messages (RFC 5546 section 1.4), so only the organizer may send them. + // An attendee holds the UID too, and could otherwise rewrite or cancel the + // event in every other attendee's calendar. + pub fn assert_organizer_sends( + &self, + method: &ICalendarMethod, + sender: &str, + ) -> Result<(), ItipError> { + if matches!( + method, + ICalendarMethod::Request + | ICalendarMethod::Add + | ICalendarMethod::Cancel + | ICalendarMethod::Declinecounter + ) && !self.organizer.email.email.eq_ignore_ascii_case(sender) + { + Err(ItipError::SenderIsNotOrganizer(sender.to_string())) + } else { + Ok(()) + } + } + pub fn sender_is_organizer_or_attendee(&self, email: &str) -> bool { self.organizer.email.email == email || self.components.values().any(|snapshot| { diff --git a/tests/resources/itip/itip_sender_role.txt b/tests/resources/itip/itip_sender_role.txt new file mode 100644 index 00000000..87f6f74b --- /dev/null +++ b/tests/resources/itip/itip_sender_role.txt @@ -0,0 +1,70 @@ +# iTIP: only the organizer may update or cancel an event + +# Organizer invites two attendees (1) +> put a@example.com sender-role@example.com +BEGIN:VCALENDAR +PRODID:-//Example/ExampleCalendarClient//EN +VERSION:2.0 +BEGIN:VEVENT +UID:sender-role@example.com +SEQUENCE:0 +ORGANIZER:mailto:a@example.com +ATTENDEE;ROLE=CHAIR;PARTSTAT=ACCEPTED:mailto:a@example.com +ATTENDEE;RSVP=TRUE:mailto:b@example.com +ATTENDEE;RSVP=TRUE:mailto:c@example.com +SUMMARY:Planning +DTSTART:20270104T180000Z +DTEND:20270104T190000Z +DTSTAMP:20261027T120000Z +STATUS:CONFIRMED +END:VEVENT +END:VCALENDAR + +# Organizer invites two attendees (2) +> send + +# An attendee cancels the event for another attendee, posing as the organizer +> itip c@example.com b@example.com +BEGIN:VCALENDAR +PRODID:-//Example/ExampleCalendarClient//EN +METHOD:CANCEL +VERSION:2.0 +BEGIN:VEVENT +UID:sender-role@example.com +SEQUENCE:1 +ORGANIZER:mailto:a@example.com +ATTENDEE;ROLE=CHAIR;PARTSTAT=ACCEPTED:mailto:a@example.com +ATTENDEE:mailto:b@example.com +ATTENDEE:mailto:c@example.com +DTSTAMP:20261027T130000Z +STATUS:CANCELLED +END:VEVENT +END:VCALENDAR + +> send +SenderIsNotOrganizer("c@example.com") + +# An attendee reschedules the event for another attendee, posing as the organizer +> itip c@example.com b@example.com +BEGIN:VCALENDAR +PRODID:-//Example/ExampleCalendarClient//EN +METHOD:REQUEST +VERSION:2.0 +BEGIN:VEVENT +UID:sender-role@example.com +SEQUENCE:2 +ORGANIZER:mailto:a@example.com +ATTENDEE;ROLE=CHAIR;PARTSTAT=ACCEPTED:mailto:a@example.com +ATTENDEE:mailto:b@example.com +ATTENDEE:mailto:c@example.com +SUMMARY:Planning (moved) +DTSTART:20270105T030000Z +DTEND:20270105T040000Z +DTSTAMP:20261027T140000Z +STATUS:CONFIRMED +END:VEVENT +END:VCALENDAR + +> send +SenderIsNotOrganizer("c@example.com") + -- 2.54.0 From c4a15b1d669101a1eddad726895667103e1f0657 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 06/21] Sharing: a sharee passes on only the rights it holds --- crates/dav/src/common/acl.rs | 12 +++++++ crates/jmap/src/addressbook/set.rs | 21 ++++++++++++ crates/jmap/src/calendar/set.rs | 21 ++++++++++++ crates/jmap/src/file/copy.rs | 8 +++-- crates/jmap/src/file/set.rs | 28 +++++++++++++-- crates/jmap/src/mailbox/set.rs | 15 ++++++++ crates/types/src/acl.rs | 55 ++++++++++++++++++++++++++++++ tests/src/jmap/calendar/acl.rs | 50 +++++++++++++++++++++++++++ 8 files changed, 206 insertions(+), 4 deletions(-) diff --git a/crates/dav/src/common/acl.rs b/crates/dav/src/common/acl.rs index 1444fdc5..ede723a1 100644 --- a/crates/dav/src/common/acl.rs +++ b/crates/dav/src/common/acl.rs @@ -31,6 +31,7 @@ use store::{ }; use store::{ahash::AHashSet, roaring::RoaringBitmap, write::BatchBuilder}; use trc::AddContext; +use types::acl::grants_beyond; use types::{ acl::{Acl, AclGrant, ArchivedAclGrant}, collection::Collection, @@ -143,6 +144,17 @@ impl DavAclHandler for Server { .validate_and_map_aces(access_token, request, collection) .await?; + // inbuxa: a sharee passes on only the rights it holds itself. + if !access_token.is_member(account_id) + && grants_beyond( + &acls.effective_acl(access_token), + &grants, + &acls.iter().map(AclGrant::from).collect::>(), + ) + { + return Err(DavError::Code(StatusCode::FORBIDDEN)); + } + // inbuxa: MT-3: grants stay within the owner's tenant let tenant_id = self .try_account(account_id) diff --git a/crates/jmap/src/addressbook/set.rs b/crates/jmap/src/addressbook/set.rs index af7324ad..09f36b64 100644 --- a/crates/jmap/src/addressbook/set.rs +++ b/crates/jmap/src/addressbook/set.rs @@ -30,6 +30,7 @@ use store::{ write::{AlignedBytes, Archive, BatchBuilder, ValueClass}, }; use trc::AddContext; +use types::acl::{AclGrant, grants_beyond}; use types::{ acl::Acl, collection::{Collection, SyncCollection}, @@ -212,6 +213,26 @@ impl AddressBookSet for Server { ); continue 'update; } + // inbuxa: a sharee passes on only the rights it holds itself. + if has_acl_changes + && grants_beyond( + &acl, + &new_address_book.acls, + &address_book + .inner + .acls + .iter() + .map(AclGrant::from) + .collect::>(), + ) + { + response.not_updated.append( + id, + SetError::forbidden() + .with_description("You can only share rights you hold yourself."), + ); + continue 'update; + } } // inbuxa: MA-D0: a group's members don't share what it owns on. if has_acl_changes && access_token.is_group_member_only(account_id) { diff --git a/crates/jmap/src/calendar/set.rs b/crates/jmap/src/calendar/set.rs index c65a536b..a2bd529d 100644 --- a/crates/jmap/src/calendar/set.rs +++ b/crates/jmap/src/calendar/set.rs @@ -35,6 +35,7 @@ use store::{ write::{AlignedBytes, Archive, BatchBuilder, ValueClass}, }; use trc::AddContext; +use types::acl::{AclGrant, grants_beyond}; use types::{ acl::Acl, collection::{Collection, SyncCollection}, @@ -216,6 +217,26 @@ impl CalendarSet for Server { ); continue 'update; } + // inbuxa: a sharee passes on only the rights it holds itself. + if has_acl_changes + && grants_beyond( + &acl, + &new_calendar.acls, + &calendar + .inner + .acls + .iter() + .map(AclGrant::from) + .collect::>(), + ) + { + response.not_updated.append( + id, + SetError::forbidden() + .with_description("You can only share rights you hold yourself."), + ); + continue 'update; + } } // inbuxa: MA-D0: a group's members don't share what it owns on. if has_acl_changes && access_token.is_group_member_only(account_id) { diff --git a/crates/jmap/src/file/copy.rs b/crates/jmap/src/file/copy.rs index 27b37ff5..e553108e 100644 --- a/crates/jmap/src/file/copy.rs +++ b/crates/jmap/src/file/copy.rs @@ -39,6 +39,7 @@ use store::{ write::{AlignedBytes, Archive, BatchBuilder, now}, }; use trc::AddContext; +use types::acl::grants_beyond; use types::{ acl::Acl, collection::{Collection, SyncCollection}, @@ -357,8 +358,11 @@ impl FileNodeCopy for Server { file_node.acls = parent_acls; } } else if is_shared - && parent_acls - .is_none_or(|acls| !acls.effective_acl(access_token).contains(Acl::Share)) + && parent_acls.is_none_or(|acls| { + // inbuxa: and passes on only the rights it holds itself + let held = acls.effective_acl(access_token); + !held.contains(Acl::Share) || grants_beyond(&held, &file_node.acls, &acls) + }) { response.not_created.append( id, diff --git a/crates/jmap/src/file/set.rs b/crates/jmap/src/file/set.rs index d1c73024..4f577c80 100644 --- a/crates/jmap/src/file/set.rs +++ b/crates/jmap/src/file/set.rs @@ -32,6 +32,7 @@ use store::{ write::{AlignedBytes, Archive, BatchBuilder, now}, }; use trc::AddContext; +use types::acl::grants_beyond; use types::{ acl::{Acl, AclGrant}, blob::BlobId, @@ -276,8 +277,11 @@ impl FileNodeSet for Server { file_node.acls = parent_acls; } } else if is_shared - && parent_acls - .is_none_or(|acls| !acls.effective_acl(access_token).contains(Acl::Share)) + && parent_acls.is_none_or(|acls| { + // inbuxa: and passes on only the rights it holds itself + let held = acls.effective_acl(access_token); + !held.contains(Acl::Share) || grants_beyond(&held, &file_node.acls, &acls) + }) { response.not_created.append( id, @@ -520,6 +524,26 @@ impl FileNodeSet for Server { ); continue 'update; } + // inbuxa: a sharee passes on only the rights it holds itself. + if has_acl_changes + && grants_beyond( + &acl, + &new_file_node.acls, + &file_node + .inner + .acls + .iter() + .map(AclGrant::from) + .collect::>(), + ) + { + response.not_updated.append( + id, + SetError::forbidden() + .with_description("You can only share rights you hold yourself."), + ); + continue 'update; + } } // inbuxa: MA-D0: a group's members don't share what it owns on. if has_acl_changes && access_token.is_group_member_only(account_id) { diff --git a/crates/jmap/src/mailbox/set.rs b/crates/jmap/src/mailbox/set.rs index d181152d..db455667 100644 --- a/crates/jmap/src/mailbox/set.rs +++ b/crates/jmap/src/mailbox/set.rs @@ -39,6 +39,7 @@ use store::{ write::{AlignedBytes, Archive, BatchBuilder, assert::AssertValue}, }; use trc::AddContext; +use types::acl::grants_beyond; use types::{ acl::Acl, collection::Collection, field::MailboxField, id::Id, special_use::SpecialUse, }; @@ -646,6 +647,20 @@ impl MailboxSet for Server { ))); } + // inbuxa: a sharee passes on only the rights it holds itself. + if ctx.is_shared + && let Some(current) = current.as_ref() + && grants_beyond( + ¤t.inner.acls.effective_acl(ctx.access_token), + &changes.acls, + ¤t.inner.acls, + ) + { + return Ok(Err(SetError::forbidden() + .with_property(MailboxProperty::ShareWith) + .with_description("You can only share rights you hold yourself."))); + } + if !changes.acls.is_empty() && let Err(err) = self.acl_validate(ctx.account_id, &changes.acls).await { diff --git a/crates/types/src/acl.rs b/crates/types/src/acl.rs index a73bbdec..a38fa4e7 100644 --- a/crates/types/src/acl.rs +++ b/crates/types/src/acl.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use std::fmt::{self, Display}; @@ -146,3 +148,56 @@ impl From<&ArchivedAclGrant> for AclGrant { } } } + +/// inbuxa: whether `new` gives anyone a right that `held` lacks. Only rights +/// added since `old` count, so a sharee may keep or narrow what the owner +/// granted, and passes on only the rights it holds itself. +pub fn grants_beyond(held: &Bitmap, new: &[AclGrant], old: &[AclGrant]) -> bool { + new.iter().any(|grant| { + let had = old + .iter() + .find(|old| old.account_id == grant.account_id) + .map_or(0, |old| old.grants.bitmap); + grant.grants.bitmap & !had & !held.bitmap != 0 + }) +} + +#[cfg(test)] +mod tests { + use super::{Acl, AclGrant, grants_beyond}; + use utils::map::bitmap::Bitmap; + + fn grant(account_id: u32, acls: &[Acl]) -> AclGrant { + let mut grants = Bitmap::new(); + grants.insert_many(acls.iter().copied()); + AclGrant { account_id, grants } + } + + #[test] + fn sharee_passes_on_only_what_it_holds() { + let held = grant(1, &[Acl::Read, Acl::Modify, Acl::Share]).grants; + let old = [ + grant(1, &[Acl::Read, Acl::Modify, Acl::Share]), + grant(3, &[Acl::Delete]), + ]; + + // Granting itself Delete + assert!(grants_beyond( + &held, + &[grant(1, &[Acl::Read, Acl::Modify, Acl::Share, Acl::Delete])], + &old + )); + // Granting someone else Delete + assert!(grants_beyond(&held, &[grant(2, &[Acl::Delete])], &old)); + // Passing on what it holds, keeping what the owner gave someone, narrowing + assert!(!grants_beyond( + &held, + &[ + grant(1, &[Acl::Read]), + grant(2, &[Acl::Read, Acl::Share]), + grant(3, &[Acl::Delete]) + ], + &old + )); + } +} diff --git a/tests/src/jmap/calendar/acl.rs b/tests/src/jmap/calendar/acl.rs index 13af8792..c76f49d0 100644 --- a/tests/src/jmap/calendar/acl.rs +++ b/tests/src/jmap/calendar/acl.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::utils::{jmap::JmapUtils, server::TestServer}; @@ -575,6 +577,54 @@ pub async fn test(test: &TestServer) { "description": null })); + // inbuxa: a sharee holding Share passes on only the rights it holds itself. + // Jane may write and share, but not delete, so she can't grant Delete, + // not even to herself. + john.jmap_update( + MethodObject::Calendar, + [( + &john_calendar_id, + json!({ + format!("shareWith/{jane_id}/mayShare"): true, + format!("shareWith/{jane_id}/mayDelete"): false, + }), + )], + Vec::<(&str, &str)>::new(), + ) + .await + .updated(&john_calendar_id); + jane.jmap_update_account( + john, + MethodObject::Calendar, + [( + &john_calendar_id, + json!({ + format!("shareWith/{jane_id}/mayDelete"): true, + }), + )], + Vec::<(&str, &str)>::new(), + ) + .await + .not_updated(&john_calendar_id); + john.jmap_update( + MethodObject::Calendar, + [( + &john_calendar_id, + json!({ + format!("shareWith/{jane_id}/mayShare"): false, + format!("shareWith/{jane_id}/mayDelete"): true, + }), + )], + Vec::<(&str, &str)>::new(), + ) + .await + .updated(&john_calendar_id); + jane_share_change_id = jane + .jmap_changes(MethodObject::ShareNotification, &jane_share_change_id) + .await + .new_state() + .to_string(); + // Revoke Jane's access john.jmap_update( MethodObject::Calendar, -- 2.54.0 From 64919a7f9c1e8c5dec89a9862b7615853fa04fa3 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 07/21] OIDC: never publish or reuse the fallback signing secret --- crates/common/src/auth/oauth/config.rs | 158 +++++++++++++++++-------- 1 file changed, 108 insertions(+), 50 deletions(-) diff --git a/crates/common/src/auth/oauth/config.rs b/crates/common/src/auth/oauth/config.rs index 598d4c93..71728a97 100644 --- a/crates/common/src/auth/oauth/config.rs +++ b/crates/common/src/auth/oauth/config.rs @@ -2,19 +2,25 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ config::{EcKeyCurve, build_ecdsa_pem, build_rsa_keypair}, manager::application::Resource, }; +use aws_lc_rs::{ + rand::SystemRandom, + signature::{ECDSA_P256_SHA256_FIXED_SIGNING, EcdsaKeyPair}, +}; use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; use jsonwebtoken::{ Algorithm, EncodingKey, jwk::{ AlgorithmParameters, CommonParameters, EllipticCurve, EllipticCurveKeyParameters, - EllipticCurveKeyType, Jwk, JwkSet, KeyAlgorithm, OctetKeyParameters, OctetKeyType, - PublicKeyUse, RSAKeyParameters, RSAKeyType, + EllipticCurveKeyType, Jwk, JwkSet, KeyAlgorithm, PublicKeyUse, RSAKeyParameters, + RSAKeyType, }, }; use registry::schema::{enums::JwtSignatureAlgorithm, prelude::ObjectType, structs::OidcProvider}; @@ -61,35 +67,34 @@ impl OAuthConfig { JwtSignatureAlgorithm::Hs512 => Algorithm::HS512, }; - let rand_key = rng() - .sample_iter(Alphanumeric) - .take(64) - .map(char::from) - .collect::(); - - let signature_key = auth - .signature_key - .secret() - .await - .map_err(|err| { - bp.build_error(ObjectType::OidcProvider.singleton(), err); - }) - .unwrap_or(Cow::Borrowed(rand_key.as_str())); - - let fallback_key = || { - ( - EncodingKey::from_secret(rand_key.as_bytes()), - AlgorithmParameters::OctetKey(OctetKeyParameters { - key_type: OctetKeyType::Octet, - value: URL_SAFE_NO_PAD.encode(&rand_key), - }) - .into(), - ) + // inbuxa: every fallback key is its own random value, and an empty + // secret counts as missing. + let random_key = || { + rng() + .sample_iter(Alphanumeric) + .take(64) + .map(char::from) + .collect::() }; - let (oidc_signing_secret, algorithm) = match oidc_signature_algorithm { + let signature_key = match auth.signature_key.secret().await { + Ok(key) if !key.is_empty() => key, + Ok(_) => { + bp.build_error( + ObjectType::OidcProvider.singleton(), + "The OIDC signature key is empty", + ); + Cow::Owned(random_key()) + } + Err(err) => { + bp.build_error(ObjectType::OidcProvider.singleton(), err); + Cow::Owned(random_key()) + } + }; + + let configured = match oidc_signature_algorithm { Algorithm::HS256 | Algorithm::HS384 | Algorithm::HS512 => { - (EncodingKey::from_secret(signature_key.as_bytes()), None) + Ok((EncodingKey::from_secret(signature_key.as_bytes()), None)) } Algorithm::RS256 | Algorithm::RS384 @@ -98,24 +103,36 @@ impl OAuthConfig { | Algorithm::PS384 | Algorithm::PS512 => parse_rsa_key(&auth) .await - .map_err(|err| { - bp.build_error(ObjectType::OidcProvider.singleton(), err); - }) - .map(|(secret, alg)| (secret, Some(alg))) - .unwrap_or_else(|_| fallback_key()), + .map(|(secret, alg)| (secret, Some(alg))), Algorithm::ES256 | Algorithm::ES384 => parse_ecdsa_key(&auth, oidc_signature_algorithm) .await - .map_err(|err| { - bp.build_error(ObjectType::OidcProvider.singleton(), err); - }) - .map(|(secret, alg)| (secret, Some(alg))) - .unwrap_or_else(|_| fallback_key()), - _ => { - bp.build_error( - ObjectType::OidcProvider.singleton(), - format!("Unsupported OIDC signature algorithm {oidc_signature_algorithm:?}"), - ); - fallback_key() + .map(|(secret, alg)| (secret, Some(alg))), + _ => Err(format!( + "Unsupported OIDC signature algorithm {oidc_signature_algorithm:?}" + )), + }; + + // inbuxa: when the configured key can't be used, sign with a fresh + // ES256 key and publish only its public half. The old fallback was a + // random HMAC secret published as an "oct" key in the public JWKS, + // where anyone could take it and sign ID tokens. + let (oidc_signing_secret, algorithm, oidc_signature_algorithm) = match configured { + Ok((secret, algorithm)) => (secret, algorithm, oidc_signature_algorithm), + Err(err) => { + bp.build_error(ObjectType::OidcProvider.singleton(), err); + match ephemeral_es256_key() { + Ok((secret, algorithm)) => (secret, Some(algorithm), Algorithm::ES256), + Err(err) => { + // Nothing is published, so no relying party accepts + // what this key signs. + bp.build_error(ObjectType::OidcProvider.singleton(), err); + ( + EncodingKey::from_secret(random_key().as_bytes()), + None, + oidc_signature_algorithm, + ) + } + } } }; @@ -140,12 +157,20 @@ impl OAuthConfig { }; OAuthConfig { - oauth_key: auth - .encryption_key - .secret() - .await - .map_err(|err| bp.build_error(ObjectType::OidcProvider.singleton(), err)) - .map_or_else(|_| rand_key.clone(), Cow::into_owned), + oauth_key: match auth.encryption_key.secret().await { + Ok(key) if !key.is_empty() => key.into_owned(), + Ok(_) => { + bp.build_error( + ObjectType::OidcProvider.singleton(), + "The OAuth encryption key is empty", + ); + random_key() + } + Err(err) => { + bp.build_error(ObjectType::OidcProvider.singleton(), err); + random_key() + } + }, oauth_expiry_user_code: auth.user_code_expiry.as_secs(), oauth_expiry_auth_code: auth.auth_code_expiry.as_secs(), oauth_expiry_token: auth.access_token_expiry.as_secs(), @@ -162,6 +187,24 @@ impl OAuthConfig { } } +fn ephemeral_es256_key() -> Result<(EncodingKey, AlgorithmParameters), String> { + let pkcs8 = + EcdsaKeyPair::generate_pkcs8(&ECDSA_P256_SHA256_FIXED_SIGNING, &SystemRandom::new()) + .map_err(|err| format!("Failed to generate a fallback OIDC key: {err}"))?; + let pem = pem::encode(&pem::Pem::new("PRIVATE KEY", pkcs8.as_ref())); + let ecdsa_key = build_ecdsa_pem(EcKeyCurve::P256, &pem)?; + + Ok(( + EncodingKey::from_ec_der(&ecdsa_key.pkcs8_der), + AlgorithmParameters::EllipticCurve(EllipticCurveKeyParameters { + key_type: EllipticCurveKeyType::EC, + curve: EllipticCurve::P256, + x: URL_SAFE_NO_PAD.encode(&ecdsa_key.x), + y: URL_SAFE_NO_PAD.encode(&ecdsa_key.y), + }), + )) +} + async fn parse_rsa_key(auth: &OidcProvider) -> Result<(EncodingKey, AlgorithmParameters), String> { let rsa_key = build_rsa_keypair(auth.signature_key.secret().await?.as_ref())?; @@ -201,3 +244,18 @@ async fn parse_ecdsa_key( AlgorithmParameters::EllipticCurve(ecdsa_key_params), )) } + +#[cfg(test)] +mod tests { + use jsonwebtoken::jwk::AlgorithmParameters; + + #[test] + fn fallback_signing_key_is_asymmetric() { + // What the JWKS publishes for the fallback must be a public key only. + let (_, params) = super::ephemeral_es256_key().unwrap(); + assert!(matches!(params, AlgorithmParameters::EllipticCurve(_))); + let json = serde_json::to_string(¶ms).unwrap(); + assert!(!json.contains("\"oct\""), "{json}"); + assert!(!json.contains("\"d\""), "{json}"); + } +} -- 2.54.0 From 0a1c67ac3c4f29a9fa8da590016bb615c80411e4 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 08/21] Autoconfig: escape the caller's address in XML responses --- .../src/network/autoconfig/autodiscover.rs | 20 +++++++++++++++++++ .../network/autoconfig/legacy_autoconfig.rs | 5 +++++ tests/src/system/oidc.rs | 15 ++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/crates/common/src/network/autoconfig/autodiscover.rs b/crates/common/src/network/autoconfig/autodiscover.rs index 4ed38196..1065fc5f 100644 --- a/crates/common/src/network/autoconfig/autodiscover.rs +++ b/crates/common/src/network/autoconfig/autodiscover.rs @@ -9,6 +9,7 @@ use crate::{Server, manager::application::Resource}; use quick_xml::Reader; use quick_xml::XmlVersion; +use quick_xml::escape::escape; use quick_xml::events::Event; use registry::schema::{enums::ServiceProtocol, structs::Service}; use std::{borrow::Cow, fmt::Write}; @@ -98,6 +99,8 @@ fn build_autodiscover_response( services: &VecMap, switched_off: impl Fn(&ServiceProtocol) -> bool, ) -> String { + let emailaddress = escape(emailaddress); + // Build XML response let mut config = String::with_capacity(1024); let _ = writeln!(&mut config, ""); @@ -384,6 +387,23 @@ mod tests { } } + #[test] + fn autodiscover_escapes_address() { + use utils::map::vec_map::VecMap; + + let xml = super::build_autodiscover_response( + "z@example.org", + "mail.example.org", + &VecMap::new(), + |_| false, + ); + assert!(!xml.contains("\n"); diff --git a/tests/src/system/oidc.rs b/tests/src/system/oidc.rs index f240d1a5..a92c24db 100644 --- a/tests/src/system/oidc.rs +++ b/tests/src/system/oidc.rs @@ -153,6 +153,21 @@ pub async fn test(test: &mut TestServer) { get("https://127.0.0.1:8899/.well-known/openid-configuration").await; let jwk_set: JWKSet<()> = get(&oidc_metadata.jwks_uri).await; + // Autoconfig echoes the address it was given; it must come back as XML text + let xml = String::from_utf8( + get_bytes( + "https://127.0.0.1:8899/mail/config-v1.1.xml?emailaddress=\ + %3Cb%20x%3D%22y%22%3Ez%3C%2Fb%3E%40example.org", + ) + .await + .to_vec(), + ) + .unwrap(); + assert!( + !xml.contains(" Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 09/21] OAuth: keep device codes apart from authorization codes --- crates/common/src/lib.rs | 3 +++ crates/http/src/auth/oauth/auth.rs | 12 ++++++------ crates/http/src/auth/oauth/token.rs | 6 +++--- crates/services/src/task_manager/maintenance.rs | 12 +++++++----- tests/src/system/oidc.rs | 16 ++++++++++++++++ 5 files changed, 35 insertions(+), 14 deletions(-) diff --git a/crates/common/src/lib.rs b/crates/common/src/lib.rs index 68dd42b3..f31cc37b 100644 --- a/crates/common/src/lib.rs +++ b/crates/common/src/lib.rs @@ -130,6 +130,9 @@ pub const KV_LOCK_DAV: u8 = 25; pub const KV_SIEVE_ID: u8 = 26; // inbuxa: far above upstream's prefixes, so a new one of theirs never collides pub const KV_PORT_REACHABILITY: u8 = 200; +// inbuxa: device-flow codes live apart from authorization codes, so neither +// grant can redeem the other's code. +pub const KV_OAUTH_DEVICE: u8 = 201; #[derive(Clone)] pub struct Server { diff --git a/crates/http/src/auth/oauth/auth.rs b/crates/http/src/auth/oauth/auth.rs index 55d2b7eb..ce488cd5 100644 --- a/crates/http/src/auth/oauth/auth.rs +++ b/crates/http/src/auth/oauth/auth.rs @@ -11,7 +11,7 @@ use crate::auth::oauth::{ OAuthStatus, openid::OpenIdHandler, registration::ClientRegistrationHandler, }; use common::{ - KV_OAUTH, Server, + KV_OAUTH, KV_OAUTH_DEVICE, Server, auth::{ AuthRequest, authentication::UsernameParts, @@ -379,7 +379,7 @@ impl OAuthApiHandler for Server { if let Some(auth_code_) = self .in_memory_store() .key_get::>(KeyValue::<()>::build_key( - KV_OAUTH, + KV_OAUTH_DEVICE, code.as_bytes(), )) .await? @@ -420,7 +420,7 @@ impl OAuthApiHandler for Server { // Delete issued user code self.in_memory_store() .key_delete(KeyValue::<()>::build_key( - KV_OAUTH, + KV_OAUTH_DEVICE, code.as_bytes(), )) .await?; @@ -429,7 +429,7 @@ impl OAuthApiHandler for Server { self.in_memory_store() .key_set( KeyValue::with_prefix( - KV_OAUTH, + KV_OAUTH_DEVICE, oauth.params.as_bytes(), Archiver::new(new_oauth_code) .untrusted() @@ -529,7 +529,7 @@ impl OAuthApiHandler for Server { // Insert device code self.in_memory_store() .key_set( - KeyValue::with_prefix(KV_OAUTH, device_code.as_bytes(), oauth_code.clone()) + KeyValue::with_prefix(KV_OAUTH_DEVICE, device_code.as_bytes(), oauth_code.clone()) .expires(self.core.oauth.oauth_expiry_user_code), ) .await?; @@ -537,7 +537,7 @@ impl OAuthApiHandler for Server { // Insert user code self.in_memory_store() .key_set( - KeyValue::with_prefix(KV_OAUTH, user_code.as_bytes(), oauth_code) + KeyValue::with_prefix(KV_OAUTH_DEVICE, user_code.as_bytes(), oauth_code) .expires(self.core.oauth.oauth_expiry_user_code), ) .await?; diff --git a/crates/http/src/auth/oauth/token.rs b/crates/http/src/auth/oauth/token.rs index a191f5dc..be57fb9a 100644 --- a/crates/http/src/auth/oauth/token.rs +++ b/crates/http/src/auth/oauth/token.rs @@ -16,7 +16,7 @@ use base64::{ engine::general_purpose::{STANDARD, URL_SAFE_NO_PAD}, }; use common::{ - KV_OAUTH, Server, + KV_OAUTH, KV_OAUTH_DEVICE, Server, auth::{ AccessToken, oauth::{GrantType, oidc::StandardClaims}, @@ -161,7 +161,7 @@ impl TokenHandler for Server { if let Some(auth_code_) = self .in_memory_store() .key_get::>(KeyValue::<()>::build_key( - KV_OAUTH, + KV_OAUTH_DEVICE, device_code.as_bytes(), )) .await? @@ -187,7 +187,7 @@ impl TokenHandler for Server { // Mark this token as issued self.in_memory_store() .key_delete(KeyValue::<()>::build_key( - KV_OAUTH, + KV_OAUTH_DEVICE, device_code.as_bytes(), )) .await?; diff --git a/crates/services/src/task_manager/maintenance.rs b/crates/services/src/task_manager/maintenance.rs index 3f404e9a..186ca4e1 100644 --- a/crates/services/src/task_manager/maintenance.rs +++ b/crates/services/src/task_manager/maintenance.rs @@ -14,10 +14,10 @@ use crate::task_manager::{ }; use common::{ KV_ACME, KV_GREYLIST, KV_LOCK_DAV, KV_LOCK_QUEUE_MESSAGE, KV_LOCK_TASK, KV_OAUTH, - KV_QUOTA_BLOB, KV_RATE_LIMIT_AUTH, KV_RATE_LIMIT_CONTACT, KV_RATE_LIMIT_HTTP_ANONYMOUS, - KV_RATE_LIMIT_HTTP_AUTHENTICATED, KV_RATE_LIMIT_IMAP, KV_RATE_LIMIT_LOITER, KV_RATE_LIMIT_RCPT, - KV_RATE_LIMIT_SCAN, KV_RATE_LIMIT_SMTP, KV_SIEVE_ID, Server, - storage::index::ObjectIndexBuilder, + KV_OAUTH_DEVICE, KV_QUOTA_BLOB, KV_RATE_LIMIT_AUTH, KV_RATE_LIMIT_CONTACT, + KV_RATE_LIMIT_HTTP_ANONYMOUS, KV_RATE_LIMIT_HTTP_AUTHENTICATED, KV_RATE_LIMIT_IMAP, + KV_RATE_LIMIT_LOITER, KV_RATE_LIMIT_RCPT, KV_RATE_LIMIT_SCAN, KV_RATE_LIMIT_SMTP, KV_SIEVE_ID, + Server, storage::index::ObjectIndexBuilder, }; use email::{ cache::MessageCacheFetch, @@ -392,7 +392,9 @@ async fn store_maintenance( KV_RATE_LIMIT_IMAP, ][..], TaskStoreMaintenanceType::ResetBlobQuotas => &[KV_QUOTA_BLOB][..], - TaskStoreMaintenanceType::RemoveAuthTokens => &[KV_ACME, KV_OAUTH][..], + TaskStoreMaintenanceType::RemoveAuthTokens => { + &[KV_ACME, KV_OAUTH, KV_OAUTH_DEVICE][..] + } _ => unreachable!(), }; diff --git a/tests/src/system/oidc.rs b/tests/src/system/oidc.rs index a92c24db..bcbb56b8 100644 --- a/tests/src/system/oidc.rs +++ b/tests/src/system/oidc.rs @@ -459,6 +459,22 @@ pub async fn test(test: &mut TestServer) { } ); + // inbuxa: an authorization code is not a device code. Redeemed through the + // device grant it would skip the PKCE, redirect_uri and client checks above. + let device_params = AHashMap::from_iter([ + ( + "grant_type".to_string(), + "urn:ietf:params:oauth:grant-type:device_code".to_string(), + ), + ("device_code".to_string(), token_params["code"].clone()), + ("client_id".to_string(), client_id.to_string()), + ]); + let device_grant = post::(&metadata.token_endpoint, &device_params).await; + assert!( + !matches!(device_grant, TokenResponse::Granted(_)), + "an authorization code was redeemed through the device grant: {device_grant:?}" + ); + // Obtain token and verify the granted scope is echoed back token_params.insert("code_verifier".to_string(), PKCE_VERIFIER.to_string()); let granted = post::(&metadata.token_endpoint, &token_params).await; -- 2.54.0 From 86d17cd2ef9edc24f243beaeb4d25e88b3c22f0d Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 10/21] Calendar: refuse recurrence rules too large to expand --- crates/dav/src/calendar/update.rs | 16 ++++++- crates/groupware/src/calendar/dates.rs | 63 ++++++++++++++++++++++++- crates/groupware/src/calendar/expand.rs | 28 +++++++++++ crates/jmap/src/calendar_event/set.rs | 21 +++++++++ 4 files changed, 125 insertions(+), 3 deletions(-) diff --git a/crates/dav/src/calendar/update.rs b/crates/dav/src/calendar/update.rs index e6a6dd85..a4d60802 100644 --- a/crates/dav/src/calendar/update.rs +++ b/crates/dav/src/calendar/update.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::assert_is_unique_uid; @@ -28,7 +30,9 @@ use dav_proto::{ }; use groupware::{ cache::GroupwareCache, - calendar::{CalendarEvent, CalendarEventData, itip::ItipSendStatus}, + calendar::{ + CalendarEvent, CalendarEventData, dates::has_oversized_rrule, itip::ItipSendStatus, + }, scheduling::{ ItipMessages, event_create::itip_create, event_update::itip_update, itip::itip_set_unreachable_status, @@ -500,6 +504,16 @@ impl CalendarUpdateRequestHandler for Server { } fn validate_ical(ical: &ICalendar) -> crate::Result<&str> { + if has_oversized_rrule(ical) { + return Err(DavError::Condition( + DavErrorCondition::new( + StatusCode::PRECONDITION_FAILED, + CalCondition::ValidCalendarObjectResource, + ) + .with_details("Recurrence rule produces too many instances per period"), + )); + } + // Validate UIDs let mut uids = HashSet::with_capacity(1); diff --git a/crates/groupware/src/calendar/dates.rs b/crates/groupware/src/calendar/dates.rs index 44b92849..be30d03c 100644 --- a/crates/groupware/src/calendar/dates.rs +++ b/crates/groupware/src/calendar/dates.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::{ @@ -11,7 +13,10 @@ use super::{ use crate::calendar::{ComponentTimeRange, alarm::CalendarAlarmType}; use calcard::{ common::timezone::Tz, - icalendar::{ICalendar, ICalendarComponentType, dates::TimeOrDelta}, + icalendar::{ + ICalendar, ICalendarComponentType, ICalendarFrequency, ICalendarRecurrenceRule, + ICalendarValue, dates::TimeOrDelta, + }, }; use compact_str::ToCompactString; use indexmap::IndexMap; @@ -22,6 +27,50 @@ use store::{ const MAX_TIME_SPAN: i64 = u32::MAX as i64; +// calcard builds every candidate of one period (days x BYHOUR x BYMINUTE x +// BYSECOND) before the expansion limit applies, so a rule is refused when that +// set is larger than this. +pub const MAX_RRULE_PERIOD_CANDIDATES: u64 = 50_000; + +pub fn rrule_period_candidates(rule: &ICalendarRecurrenceRule) -> u64 { + fn distinct(values: &[u8]) -> u64 { + let mask = values + .iter() + .filter(|v| **v < 128) + .fold(0u128, |mask, v| mask | (1 << *v)); + (mask.count_ones() as u64).max(1) + } + + let days = match rule.freq { + ICalendarFrequency::Yearly => 366, + ICalendarFrequency::Monthly => 31, + ICalendarFrequency::Weekly => 7, + _ => 1, + }; + let seconds = distinct(&rule.bysecond); + let minutes = distinct(&rule.byminute); + let times = match rule.freq { + ICalendarFrequency::Secondly => 1, + ICalendarFrequency::Minutely => seconds, + ICalendarFrequency::Hourly => minutes * seconds, + _ => distinct(&rule.byhour) * minutes * seconds, + }; + days * times +} + +fn is_oversized_rrule(value: &ICalendarValue) -> bool { + matches!(value, ICalendarValue::RecurrenceRule(rule) + if rrule_period_candidates(rule) > MAX_RRULE_PERIOD_CANDIDATES) +} + +pub fn has_oversized_rrule(ical: &ICalendar) -> bool { + ical.components + .iter() + .flat_map(|comp| comp.entries.iter()) + .flat_map(|entry| entry.values.iter()) + .any(is_oversized_rrule) +} + impl CalendarEventData { pub fn new( ical: ICalendar, @@ -32,7 +81,17 @@ impl CalendarEventData { let mut ranges = TimeRanges::default(); let now = now() as i64; - let expanded = ical.expand_dates(default_tz, max_expansions); + let expanded = if has_oversized_rrule(&ical) { + // Index only the base instances of a rule that is too large to expand + let mut bounded = ical.clone(); + for comp in &mut bounded.components { + comp.entries + .retain(|entry| !entry.values.iter().any(is_oversized_rrule)); + } + bounded.expand_dates(default_tz, max_expansions) + } else { + ical.expand_dates(default_tz, max_expansions) + }; let mut groups: IndexMap<(u16, u16, u16, i32), Vec, RandomState> = IndexMap::with_capacity_and_hasher(16, RandomState::default()); let mut alarms = AHashMap::with_capacity(16); diff --git a/crates/groupware/src/calendar/expand.rs b/crates/groupware/src/calendar/expand.rs index 9314ada7..d736f3a2 100644 --- a/crates/groupware/src/calendar/expand.rs +++ b/crates/groupware/src/calendar/expand.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::ArchivedCalendarEventData; @@ -480,4 +482,30 @@ mod tests { assert_eq!(expansion.len(), 1); assert_eq!(keys.into_iter().collect::>(), [missing]); } + + #[test] + fn oversized_rrule_is_not_expanded() { + // A yearly rule on every day, hour, minute and second has ~31.6M candidates + // in its first year, which the expander buffers (~1.6 GB) before any limit. + let list = |n: u32| (0..n).map(|v| v.to_string()).collect::>().join(","); + let ical = format!( + "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//Test//EN\r\n\ + BEGIN:VEVENT\r\nUID:big@example.com\r\nDTSTAMP:20270101T000000Z\r\n\ + DTSTART:20270301T090000Z\r\nDTEND:20270301T100000Z\r\n\ + RRULE:FREQ=YEARLY;BYDAY=MO,TU,WE,TH,FR,SA,SU;BYHOUR={};BYMINUTE={};BYSECOND={}\r\n\ + END:VEVENT\r\nEND:VCALENDAR\r\n", + list(24), + list(60), + list(60) + ); + + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(event_data(&ical).time_ranges.len()); + }); + let ranges = rx + .recv_timeout(std::time::Duration::from_secs(2)) + .expect("expansion of an oversized RRULE must be refused, not attempted"); + assert_eq!(ranges, 1, "the base instance is still indexed"); + } } diff --git a/crates/jmap/src/calendar_event/set.rs b/crates/jmap/src/calendar_event/set.rs index 0a8264cb..ba994182 100644 --- a/crates/jmap/src/calendar_event/set.rs +++ b/crates/jmap/src/calendar_event/set.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::calendar_event::{CalendarSyntheticId, assert_is_unique_uid}; @@ -27,6 +29,7 @@ use groupware::{ ALERT_EMAIL, ALERT_RELATIVE_TO_END, ArchivedDefaultAlert, Calendar, CalendarEvent, CalendarEventData, EVENT_DRAFT, EVENT_HIDE_ATTENDEES, EVENT_INVITE_OTHERS, EVENT_INVITE_SELF, PREF_USE_DEFAULT_ALERTS, + dates::has_oversized_rrule, expand::{CalendarEventExpansion, ComponentRecurrenceId, RecurrenceKey, resolve_local}, itip::ItipSendStatus, }, @@ -412,6 +415,17 @@ impl CalendarEventSet for Server { ); continue 'update; } + if has_oversized_rrule(&new_calendar_event.data.event) { + update.fail( + &mut response, + SetError::invalid_properties() + .with_property(JSCalendarProperty::RecurrenceRule) + .with_description( + "Recurrence rule produces too many instances per period.", + ), + ); + continue 'update; + } // Obtain previous alarm let now = now() as i64; @@ -814,6 +828,13 @@ impl CalendarEventSet for Server { ), ))); } + if has_oversized_rrule(&ical) { + return Ok(Err(SetError::invalid_properties() + .with_property(JSCalendarProperty::RecurrenceRule) + .with_description( + "Recurrence rule produces too many instances per period.", + ))); + } // Build event let mut next_email_alarm = None; -- 2.54.0 From 6a3516188158e050d93ca9b2177ce479b7443a1b Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 17:55:03 -0700 Subject: [PATCH 11/21] Email/set: a sharee changes only messages it can read --- crates/jmap/src/email/set.rs | 10 ++++++++++ tests/src/jmap/mail/acl.rs | 16 ++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/crates/jmap/src/email/set.rs b/crates/jmap/src/email/set.rs index 68db77f1..083234c9 100644 --- a/crates/jmap/src/email/set.rs +++ b/crates/jmap/src/email/set.rs @@ -97,6 +97,9 @@ impl EmailSet for Server { } else { (None, None, None) }; + let can_read_message_ids = access_token + .is_shared(account_id) + .then(|| cache.shared_messages(access_token, Acl::ReadItems)); // Obtain import access token let import_access_token = if account_id != access_token.account_id() { @@ -823,6 +826,13 @@ impl EmailSet for Server { // Obtain message data let document_id = id.document_id(); + if can_read_message_ids + .as_ref() + .is_some_and(|ids| !ids.contains(document_id)) + { + response.not_updated.append(id, SetError::not_found()); + continue 'update; + } let data_ = match self .store() .get_value::>(ValueKey::archive( diff --git a/tests/src/jmap/mail/acl.rs b/tests/src/jmap/mail/acl.rs index 23a607f2..c4c09e7a 100644 --- a/tests/src/jmap/mail/acl.rs +++ b/tests/src/jmap/mail/acl.rs @@ -469,6 +469,22 @@ pub async fn test(test: &TestServer) { .await .unwrap(); + // AddItems on a mailbox doesn't let John file messages he can't read into it + let result = john_client + .set_default_account_id(jane.id_string()) + .email_set_mailbox(&jane_trash_email, &mailbox_id, true) + .await; + assert!( + matches!( + result, + Err(jmap_client::Error::Set(SetError { + type_: SetErrorType::NotFound, + .. + })) + ), + "Expected not found, got {result:?}" + ); + // Try deleting a mailbox assert_forbidden( john_client -- 2.54.0 From 1954198fe8ec0efc325b3ff23dc571383b195e00 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 18:47:32 -0700 Subject: [PATCH 12/21] HTTP: believe forwarding headers only from trusted proxies The client address came from the left-most X-Forwarded-For or Forwarded entry, which the client writes itself, so a banned or rate-limited client behind a proxy could pick any address. Headers are now read only when the connection comes from a trusted proxy, and the client is the right-most hop no trusted proxy accounts for. Trusted by default: loopback and the private ranges. INBUXA_HTTP_TRUSTED_PROXIES replaces that list. A direct client's headers are ignored, and scanner bans apply to it even with forwarding on. --- crates/common/src/config/network.rs | 54 ++++++- crates/http/src/client_ip.rs | 229 ++++++++++++++++++++++++++++ crates/http/src/lib.rs | 1 + crates/http/src/request.rs | 53 ++----- tests/src/system/security.rs | 40 +++++ 5 files changed, 337 insertions(+), 40 deletions(-) create mode 100644 crates/http/src/client_ip.rs diff --git a/crates/common/src/config/network.rs b/crates/common/src/config/network.rs index 876d63f2..0b95d210 100644 --- a/crates/common/src/config/network.rs +++ b/crates/common/src/config/network.rs @@ -26,7 +26,12 @@ use registry::schema::{ SystemSettings, TaskManager, }, }; -use std::{str::FromStr, time::Duration}; +use registry::types::ipmask::IpAddrOrMask; +use std::{ + net::{Ipv4Addr, Ipv6Addr}, + str::FromStr, + time::Duration, +}; use utils::map::vec_map::VecMap; #[derive(Clone)] @@ -71,6 +76,9 @@ pub struct Http { /// CORS is permissive (bootstrap, recovery, or `usePermissiveCors`). pub cors_origins: Vec, pub use_forwarded: bool, + /// inbuxa: peers whose X-Forwarded-For / Forwarded headers are believed. + /// Loopback and private networks, or `INBUXA_HTTP_TRUSTED_PROXIES`. + pub trusted_proxies: Vec, pub redirect_root: Option, /// inbuxa: HTTP Basic accepted on every endpoint, not only DAV (contract /// C-23). True in bootstrap and recovery mode, or with @@ -489,6 +497,26 @@ impl Http { Err(_) => false, }; + // inbuxa: which peers may say who the client is. A list replaces the + // default; an entry that isn't an address or network is left out. + let trusted_proxies = match types::branding::env_var("HTTP_TRUSTED_PROXIES") { + Ok(value) if !value.trim().is_empty() => value + .split(|c: char| c == ',' || c.is_whitespace()) + .filter(|entry| !entry.is_empty()) + .filter_map(|entry| match entry.parse::() { + Ok(net) => Some(net), + Err(err) => { + bp.build_warning( + ObjectType::Http.singleton(), + format!("INBUXA_HTTP_TRUSTED_PROXIES: {err}, left out."), + ); + None + } + }) + .collect(), + _ => default_trusted_proxies(), + }; + if use_permissive_cors { http_headers.push(( hyper::header::ACCESS_CONTROL_ALLOW_ORIGIN, @@ -547,12 +575,36 @@ impl Http { response_headers: http_headers, cors_origins, use_forwarded: http.use_x_forwarded, + trusted_proxies, redirect_root: http.redirect_root, basic_auth_everywhere, } } } +// inbuxa: loopback and the private ranges, where a reverse proxy on the same +// host, container network or LAN connects from. +fn default_trusted_proxies() -> Vec { + let v4 = |a, b, bits: u32| IpAddrOrMask::V4 { + addr: Ipv4Addr::new(a, b, 0, 0), + mask: u32::MAX << (32 - bits), + }; + let v6 = |addr: Ipv6Addr, bits: u32| IpAddrOrMask::V6 { + addr, + mask: u128::MAX << (128 - bits), + }; + vec![ + v4(127, 0, 8), + v4(10, 0, 8), + v4(172, 16, 12), + v4(192, 168, 16), + v4(100, 64, 10), + v6(Ipv6Addr::LOCALHOST, 128), + v6(Ipv6Addr::new(0xfc00, 0, 0, 0, 0, 0, 0, 0), 7), + v6(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 0), 10), + ] +} + impl AsnGeoLookupConfig { pub async fn parse(bp: &mut Bootstrap) -> Option { match bp.setting_infallible::().await { diff --git a/crates/http/src/client_ip.rs b/crates/http/src/client_ip.rs new file mode 100644 index 00000000..e2309f9a --- /dev/null +++ b/crates/http/src/client_ip.rs @@ -0,0 +1,229 @@ +/* + * SPDX-FileCopyrightText: 2026 Coffey Labs LLC + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +//! The client's address when inbuxa sits behind reverse proxies. +//! +//! Each proxy appends the address it received the request from, so the +//! client is the right-most address in the chain that no trusted proxy +//! accounts for. Everything to its left was written by the client and can +//! say anything. Forwarding headers are read only when the connection itself +//! comes from a trusted proxy. + +use hyper::HeaderMap; +use hyper::header::FORWARDED; +use registry::types::ipmask::IpAddrOrMask; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; + +pub fn is_trusted_proxy(trusted: &[IpAddrOrMask], ip: &IpAddr) -> bool { + let ip = ip.to_canonical(); + trusted.iter().any(|net| net.matches(&ip)) +} + +/// The client behind `peer`, or `None` when `peer` isn't a trusted proxy or +/// the headers name no usable address. +pub fn forwarded_client( + peer: IpAddr, + headers: &HeaderMap, + trusted: &[IpAddrOrMask], +) -> Option { + if !is_trusted_proxy(trusted, &peer) { + return None; + } + + let chain = forwarded_chain(headers).or_else(|| x_forwarded_chain(headers))?; + let mut client = None; + for hop in chain.into_iter().rev() { + // An address that can't be read ends the chain: what's left of it + // can't be told apart from what the client wrote. + let Some(ip) = hop else { + break; + }; + client = Some(ip); + if !is_trusted_proxy(trusted, &ip) { + break; + } + } + client +} + +// RFC 7239 `Forwarded: for=...` values in order, `None` for one that isn't an +// address (obfuscated or unknown). `None` overall when no element has `for`. +fn forwarded_chain(headers: &HeaderMap) -> Option>> { + let mut chain = Vec::new(); + for value in headers.get_all(FORWARDED) { + let Ok(value) = value.to_str() else { + chain.push(None); + continue; + }; + for element in value.split(',') { + if let Some(node) = element.split(';').find_map(|pair| { + pair.split_once('=') + .filter(|(name, _)| name.trim().eq_ignore_ascii_case("for")) + .map(|(_, node)| node) + }) { + chain.push(parse_node(node)); + } + } + } + (!chain.is_empty()).then_some(chain) +} + +fn x_forwarded_chain(headers: &HeaderMap) -> Option>> { + let mut chain = Vec::new(); + for value in headers.get_all("x-forwarded-for") { + match value.to_str() { + Ok(value) => chain.extend(value.split(',').map(parse_node)), + Err(_) => chain.push(None), + } + } + (!chain.is_empty()).then_some(chain) +} + +// An address with an optional port: `192.0.2.1`, `192.0.2.1:8080`, `2001:db8::1`, +// `[2001:db8::1]:8080`, any of them in double quotes. +fn parse_node(node: &str) -> Option { + let node = node.trim(); + let node = node + .strip_prefix('"') + .and_then(|node| node.strip_suffix('"')) + .unwrap_or(node); + let ip = if let Some(rest) = node.strip_prefix('[') { + let (ip, port) = rest.split_once(']')?; + if !port.is_empty() && !port.strip_prefix(':').is_some_and(is_port) { + return None; + } + IpAddr::V6(ip.parse::().ok()?) + } else if let Ok(ip) = node.parse::() { + ip + } else { + let (ip, port) = node.split_once(':')?; + if !is_port(port) { + return None; + } + IpAddr::V4(ip.parse::().ok()?) + }; + Some(ip.to_canonical()) +} + +fn is_port(port: &str) -> bool { + port.parse::().is_ok() +} + +#[cfg(test)] +mod tests { + use super::*; + use hyper::header::HeaderValue; + + fn trusted() -> Vec { + ["127.0.0.0/8", "10.0.0.0/8", "::1"] + .iter() + .map(|net| net.parse().unwrap()) + .collect() + } + + fn client(peer: &str, headers: &[(&'static str, &str)]) -> Option { + let mut map = HeaderMap::new(); + for (name, value) in headers { + map.append(*name, HeaderValue::from_str(value).unwrap()); + } + forwarded_client(peer.parse().unwrap(), &map, &trusted()) + } + + fn ip(ip: &str) -> Option { + Some(ip.parse().unwrap()) + } + + #[test] + fn right_most_untrusted_hop_is_the_client() { + let xff = "x-forwarded-for"; + // One proxy: the client's own entries in front are ignored + assert_eq!( + client("127.0.0.1", &[(xff, "198.51.100.1, 203.0.113.7")]), + ip("203.0.113.7") + ); + // Two proxies: the inner one is skipped + assert_eq!( + client("127.0.0.1", &[(xff, "198.51.100.1, 203.0.113.7, 10.0.0.5")]), + ip("203.0.113.7") + ); + // Every hop trusted: the left-most one + assert_eq!(client("127.0.0.1", &[(xff, "10.0.0.2")]), ip("10.0.0.2")); + // Several header lines read as one list + assert_eq!( + client("127.0.0.1", &[(xff, "198.51.100.1"), (xff, "203.0.113.7")]), + ip("203.0.113.7") + ); + // An unreadable hop ends the chain at the last address before it + assert_eq!( + client("127.0.0.1", &[(xff, "198.51.100.1, junk, 10.0.0.5")]), + ip("10.0.0.5") + ); + assert_eq!(client("127.0.0.1", &[(xff, "198.51.100.1, junk")]), None); + // IPv4-mapped peers and hops count as IPv4 + assert_eq!( + client("::ffff:127.0.0.1", &[(xff, "::ffff:203.0.113.7")]), + ip("203.0.113.7") + ); + } + + #[test] + fn headers_from_an_untrusted_peer_are_ignored() { + assert_eq!( + client("192.0.2.10", &[("x-forwarded-for", "203.0.113.7")]), + None + ); + assert_eq!( + client("192.0.2.10", &[("forwarded", "for=203.0.113.7")]), + None + ); + } + + #[test] + fn forwarded_header() { + let fwd = "forwarded"; + assert_eq!( + client( + "127.0.0.1", + &[(fwd, "for=198.51.100.1, for=203.0.113.7;proto=https")] + ), + ip("203.0.113.7") + ); + assert_eq!( + client( + "127.0.0.1", + &[(fwd, r#"For="[2001:db8::7]:4711", for=10.0.0.5"#)] + ), + ip("2001:db8::7") + ); + assert_eq!( + client("127.0.0.1", &[(fwd, "for=203.0.113.7:8080")]), + ip("203.0.113.7") + ); + // An obfuscated hop ends the chain + assert_eq!( + client("127.0.0.1", &[(fwd, "for=_hidden, for=10.0.0.5")]), + ip("10.0.0.5") + ); + // Forwarded is preferred; X-Forwarded-For is the fallback + assert_eq!( + client( + "127.0.0.1", + &[ + (fwd, "for=203.0.113.7"), + ("x-forwarded-for", "198.51.100.1") + ] + ), + ip("203.0.113.7") + ); + assert_eq!( + client( + "127.0.0.1", + &[(fwd, "proto=https"), ("x-forwarded-for", "198.51.100.1")] + ), + ip("198.51.100.1") + ); + } +} diff --git a/crates/http/src/lib.rs b/crates/http/src/lib.rs index b3c71e2d..6ee5d829 100644 --- a/crates/http/src/lib.rs +++ b/crates/http/src/lib.rs @@ -14,6 +14,7 @@ pub mod api; pub mod auth; pub mod branding; // inbuxa: branding +pub mod client_ip; // inbuxa: trusted proxies pub mod form; pub mod live; // inbuxa: monitoring (MON-20 to MON-24) pub mod request; diff --git a/crates/http/src/request.rs b/crates/http/src/request.rs index 49fcfdf6..570f01fb 100644 --- a/crates/http/src/request.rs +++ b/crates/http/src/request.rs @@ -17,6 +17,7 @@ use crate::{ }, token_only::{is_refused_basic, is_token_only_path}, }, + client_ip, form::FormHandler, }; use common::{ @@ -49,7 +50,7 @@ use jmap::{ use jmap_proto::request::{Request, capability::Session}; use percent_encoding::percent_decode_str; use registry::schema::enums::Permission; -use std::{net::IpAddr, str::FromStr, sync::Arc}; +use std::{str::FromStr, sync::Arc}; use store::dispatch::lookup::KeyValue; use trc::SecurityEvent; use types::{blob::BlobId, id::Id}; @@ -722,43 +723,11 @@ async fn handle_session(inner: Arc, session: SessionDat ); session.remote_ip - } else if let Some(forwarded_for) = req - .headers() - .get(header::FORWARDED) - .and_then(|h| h.to_str().ok()) - .and_then(|h| { - let h = h.to_ascii_lowercase(); - h.split_once("for=").and_then(|(_, rest)| { - let mut start_ip = usize::MAX; - let mut end_ip = usize::MAX; - - for (pos, ch) in rest.char_indices() { - match ch { - '0'..='9' | 'a'..='f' | ':' | '.' => { - if start_ip == usize::MAX { - start_ip = pos; - } - end_ip = pos; - } - '"' | '[' | ' ' if start_ip == usize::MAX => {} - _ => { - break; - } - } - } - - rest.get(start_ip..=end_ip) - .and_then(|h| h.parse::().ok()) - }) - }) - .or_else(|| { - req.headers() - .get("X-Forwarded-For") - .and_then(|h| h.to_str().ok()) - .map(|h| h.split_once(',').map_or(h, |(ip, _)| ip).trim()) - .and_then(|h| h.parse::().ok()) - }) - { + } else if let Some(forwarded_for) = client_ip::forwarded_client( + session.remote_ip, + req.headers(), + &server.core.network.http.trusted_proxies, + ) { trc::event!( Http(trc::HttpEvent::RequestUrl), SpanId = session.session_id, @@ -901,7 +870,13 @@ async fn handle_session(inner: Arc, session: SessionDat { if http_err.is_parse() { let server = inner.build_server(); - if !server.core.network.http.use_forwarded { + // inbuxa: a trusted proxy's address stands for many clients + if !(server.core.network.http.use_forwarded + && client_ip::is_trusted_proxy( + &server.core.network.http.trusted_proxies, + &session.remote_ip, + )) + { match server.is_scanner_fail2banned(session.remote_ip).await { Ok(true) => { trc::event!( diff --git a/tests/src/system/security.rs b/tests/src/system/security.rs index e00e4c1d..dbae92c8 100644 --- a/tests/src/system/security.rs +++ b/tests/src/system/security.rs @@ -236,6 +236,46 @@ pub async fn test(test: &mut TestServer) { "the expired ban's record is gone" ); + // inbuxa: behind a proxy the client is the last hop the proxies didn't + // add. A banned client can't pick another address by putting it first. + for _ in 0..105 { + validate_password_with_ip( + "unknown@example.org", + "wrong password", + "203.0.113.7", + false, + ) + .await; + } + validate_password_with_ip( + "user@example.org", + "this is a very strong password", + "198.51.100.1, 203.0.113.7", + false, + ) + .await; + let blocked_id = test + .server + .registry() + .primary_key( + ObjectType::BlockedIp.into(), + Property::Address, + IpAddrOrMask::from_ip(Ipv4Addr::new(203, 0, 113, 7).into()).to_index_key(), + ) + .await + .unwrap() + .expect("the ban is on the address the proxy saw"); + test.server + .registry() + .write(RegistryWrite::delete(blocked_id)) + .await + .unwrap() + .unwrap_id(trc::location!()); + test.server + .reload_registry(RegistryChange::Delete(blocked_id)) + .await + .unwrap(); + // inbuxa: personal-data catalog, D1: how long rotated log files are kept. // Unset keeps every file; a value is days; zero is refused; null goes back let using = &["urn:ietf:params:jmap:core", "urn:inbuxa:jmap"]; -- 2.54.0 From e45934fc8254f5e764325dda186cada358d5bc73 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 21:03:56 -0700 Subject: [PATCH 13/21] SMTP: never write an address with a line break into a command TLS-RPT report addresses come from DNS, and quoted-printable in a rua=mailto: URI can put CR/LF into one. The address went into RCPT TO as it was, so the line break started a command of the record owner's choosing. Addresses with control characters, spaces or angle brackets are now refused when they're queued, and any already in the queue fail instead of being sent. --- crates/smtp/src/outbound/session.rs | 25 +++++++++++++++- crates/smtp/src/queue/mod.rs | 32 +++++++++++++++++++++ crates/smtp/src/queue/spool.rs | 16 +++++++++-- crates/smtp/src/reporting/send.rs | 10 +++++++ tests/src/smtp/reporting/tls.rs | 44 ++++++++++++++++++++++++++++- 5 files changed, 122 insertions(+), 5 deletions(-) diff --git a/crates/smtp/src/outbound/session.rs b/crates/smtp/src/outbound/session.rs index 7f0d5c76..878df39c 100644 --- a/crates/smtp/src/outbound/session.rs +++ b/crates/smtp/src/outbound/session.rs @@ -2,13 +2,15 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::client::SmtpClient; use crate::outbound::DeliveryResult; use crate::outbound::client::{BoxResponse, from_error_status, from_mail_send_error}; use crate::outbound::error::ClientError; -use crate::queue::{Error, MessageWrapper, Recipient, Status}; +use crate::queue::{Error, MessageWrapper, Recipient, Status, is_smtp_path}; use crate::queue::{ErrorDetails, HostResponse, UnexpectedResponse}; use common::Server; use common::config::smtp::queue::ConnectionStrategy; @@ -172,6 +174,27 @@ impl MessageWrapper { continue; } + // inbuxa: queued before the queue refused such addresses + if !is_smtp_path(rcpt.address()) { + trc::event!( + Delivery(DeliveryEvent::RcptToRejected), + SpanId = params.session_id, + Hostname = params.hostname.to_string(), + To = rcpt.address().to_string(), + Details = "The address can't be written in an SMTP command.", + ); + statuses.push(DeliveryResult::account( + Status::PermanentFailure(ErrorDetails { + entity: params.hostname.into(), + details: Error::Io( + "The address can't be written in an SMTP command.".into(), + ), + }), + *rcpt_idx, + )); + continue; + } + let cmd = self.build_rcpt_to(rcpt, &capabilities); match smtp_client.cmd(cmd.as_bytes()).await { Ok(response) => match response.severity() { diff --git a/crates/smtp/src/queue/mod.rs b/crates/smtp/src/queue/mod.rs index d31323ca..4a89b14c 100644 --- a/crates/smtp/src/queue/mod.rs +++ b/crates/smtp/src/queue/mod.rs @@ -668,3 +668,35 @@ impl DisplayArchivedResponse for ArchivedResponse> { } } */ + +// inbuxa: whether an address can go into MAIL FROM or RCPT TO as it is. Report +// addresses come from DNS, where quoted-printable can put CR/LF in them, and a +// line break there would start a command of the sender's choosing. +pub fn is_smtp_path(address: &str) -> bool { + !address.is_empty() + && !address + .bytes() + .any(|byte| byte.is_ascii_control() || matches!(byte, b' ' | b'<' | b'>')) +} + +#[cfg(test)] +mod smtp_path_tests { + use super::is_smtp_path; + + #[test] + fn only_plain_addresses_are_paths() { + assert!(is_smtp_path("reports@example.org")); + assert!(is_smtp_path("jörg@exämple.org")); + for address in [ + "", + "x@example.org\r\nRSET", + "x@example.org\nRSET", + "x@example.org\0", + "x@example.org> NOTIFY=NEVER", + "x y@example.org", + ", server: &Server) { - self.message.recipients.push(Recipient::new(rcpt.as_ref())); + let rcpt = rcpt.as_ref(); + if !is_smtp_path(rcpt) { + trc::event!( + Delivery(DeliveryEvent::RcptToRejected), + SpanId = self.span_id, + To = rcpt.to_string(), + Details = "The address can't be written in an SMTP command.", + ); + return; + } + self.message.recipients.push(Recipient::new(rcpt)); let queue = server.get_queue_or_default( &server .eval_if::( diff --git a/crates/smtp/src/reporting/send.rs b/crates/smtp/src/reporting/send.rs index 7bde376a..d0b789b2 100644 --- a/crates/smtp/src/reporting/send.rs +++ b/crates/smtp/src/reporting/send.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -75,6 +77,10 @@ impl MtaReportSend for Server { for rcpt_ in rcpts { message.add_expanded_recipient(rcpt_.as_ref(), self).await; } + // inbuxa: every address was refused + if message.message.recipients.is_empty() { + return; + } // Schedule delivery at a random time between now and the next 3 hours if !deliver_now { @@ -122,6 +128,10 @@ impl MtaReportSend for Server { for rcpt in rcpts { message.add_expanded_recipient(rcpt, self).await; } + // inbuxa: every address was refused + if message.message.recipients.is_empty() { + return; + } // Queue message let dkim_signers = if let Some(sign_config) = sign_config { diff --git a/tests/src/smtp/reporting/tls.rs b/tests/src/smtp/reporting/tls.rs index e6f4801b..742b2d5e 100644 --- a/tests/src/smtp/reporting/tls.rs +++ b/tests/src/smtp/reporting/tls.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -12,7 +14,7 @@ use common::{config::smtp::report::AggregateFrequency, ipc::TlsEvent}; use mail_auth::{ common::parse::TxtRecordParser, flate2::read::GzDecoder, - mta_sts::TlsRpt, + mta_sts::{ReportUri, TlsRpt}, report::tlsrpt::{FailureDetails, PolicyType, ResultType, TlsReport}, }; use registry::schema::structs::{Expression, ReportSettings, TlsInternalReport, TlsReportSettings}; @@ -252,5 +254,45 @@ async fn report_tls() { assert_eq!(report.contact_info.unwrap(), "https://foobar.org/contact"); assert_eq!(report.policies.len(), 1); } + + // inbuxa: rua addresses are quoted-printable, so a record can put CR/LF in + // one. That address must never reach a RCPT TO. + let tls_record = Arc::new( + TlsRpt::parse(b"v=TLSRPTv1;rua=mailto:x@foobar.org=0D=0ARSET,mailto:reports@foobar.org") + .unwrap(), + ); + assert!( + matches!(&tls_record.rua[0], ReportUri::Mail(addr) if addr.contains("\r\n")), + "{:?}", + tls_record.rua + ); + test.server + .schedule_tls(Box::new(TlsEvent { + domain: "foobar.org".to_string(), + policy: common::ipc::PolicyType::None, + failure: None, + tls_record, + interval: AggregateFrequency::Daily, + span_id: 0, + })) + .await; + tokio::time::sleep(Duration::from_millis(200)).await; + let reports = test.read_report_events::().await; + assert_eq!(reports.len(), 1); + test.server + .send_tls_aggregate_report(reports.first().unwrap().0.id()) + .await + .unwrap(); + let message = test.expect_message().await; + assert_eq!( + message + .message + .recipients + .iter() + .map(|rcpt| rcpt.address().to_string()) + .collect::>(), + ["reports@foobar.org"] + ); + test.assert_report_is_empty::().await; } -- 2.54.0 From e028a0c091e284306e4eb77e59307824405ac6fc Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 21:06:44 -0700 Subject: [PATCH 14/21] Spam filter: stop looking for the end of an IPv6 literal after 47 characters The tokenizer that finds URLs and email addresses in a message looked for the closing bracket of an IPv6 host with no limit, buffering every token after an unclosed "[", and then took those tokens off the front of a Vec one at a time. A long "a@[1:1:1:..." run in inbound mail took time quadratic in its length: 7.4 s for 200,000 characters, hours for a full-size message. No IPv6 literal is longer than 47 characters with its brackets, so the search stops there, and the buffer is a VecDeque. --- crates/nlp/src/tokenizers/types.rs | 55 ++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 7 deletions(-) diff --git a/crates/nlp/src/tokenizers/types.rs b/crates/nlp/src/tokenizers/types.rs index 7818d3b6..eb2d4b84 100644 --- a/crates/nlp/src/tokenizers/types.rs +++ b/crates/nlp/src/tokenizers/types.rs @@ -2,9 +2,11 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ -use std::str::CharIndices; +use std::{collections::VecDeque, str::CharIndices}; use super::Token; @@ -12,7 +14,7 @@ use super::Token; pub struct TypesTokenizer<'x> { text: &'x str, iter: CharIndices<'x>, - tokens: Vec>>, + tokens: VecDeque>>, peek_pos: usize, last_ch_is_space: bool, last_token_is_dot: bool, @@ -101,12 +103,15 @@ impl<'x> Iterator for TypesTokenizer<'x> { } } +// inbuxa: "[ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255]" +const IPV6_LITERAL_MAX_LEN: usize = 47; + impl<'x> TypesTokenizer<'x> { pub fn new(text: &'x str) -> Self { Self { text, iter: text.char_indices(), - tokens: Vec::new(), + tokens: VecDeque::new(), eof: false, peek_pos: 0, last_ch_is_space: false, @@ -184,7 +189,7 @@ impl<'x> TypesTokenizer<'x> { if start_pos != usize::MAX { let text = &self.text[start_pos..end_pos]; - self.tokens.push(Token { + self.tokens.push_back(Token { word: if has_alpha && has_number { TokenType::Alphanumeric(text) } else if has_alpha { @@ -196,11 +201,11 @@ impl<'x> TypesTokenizer<'x> { to: end_pos, }); if let Some(stop_char) = stop_char { - self.tokens.push(stop_char); + self.tokens.push_back(stop_char); } true } else if let Some(stop_char) = stop_char { - self.tokens.push(stop_char); + self.tokens.push_back(stop_char); true } else { self.eof = true; @@ -213,7 +218,7 @@ impl<'x> TypesTokenizer<'x> { self.consume(); } if !self.tokens.is_empty() { - Some(self.tokens.remove(0)) + self.tokens.pop_front() } else { None } @@ -582,6 +587,10 @@ impl<'x> TypesTokenizer<'x> { let mut last_ch = u8::MAX; while let Some(token) = self.peek() { + // inbuxa: no IPv6 literal is longer than "[" + 45 characters + "]" + if token.to - start_pos > IPV6_LITERAL_MAX_LEN { + return None; + } match token.word { TokenType::Integer(_) | TokenType::Alphanumeric(_) => { last_ch = 0; @@ -2874,4 +2883,36 @@ mod test { println!("]),");*/ } } + + // inbuxa: a bracket that never closes used to buffer everything after it, + // and every token after that was taken off the front of the buffer one at a + // time, so a long run took time quadratic in its length. + #[test] + fn unclosed_ipv6_is_linear() { + let run = "1:".repeat(100_000); + for prefix in ["a@[", "http://["] { + let text = format!("{prefix}{run}"); + let started = std::time::Instant::now(); + let count = TypesTokenizer::new(&text).count(); + let elapsed = started.elapsed(); + assert!(count >= run.len(), "{prefix}: {count} tokens"); + assert!( + elapsed < std::time::Duration::from_secs(2), + "{prefix}: {elapsed:?}" + ); + } + + // The longest IPv6 literal still parses + let host = "[1234:1234:1234:1234:1234:1234:255.255.255.255]"; + let email = format!("a@{host}"); + let url = format!("http://{host}/"); + assert_eq!( + TypesTokenizer::new(&email).next().map(|t| t.word), + Some(TokenType::Email(email.as_str())) + ); + assert_eq!( + TypesTokenizer::new(&url).next().map(|t| t.word), + Some(TokenType::Url(url.as_str())) + ); + } } -- 2.54.0 From d0238a07572f2dff0bd7c5ea539b2429fdf2d8dc Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 21:10:05 -0700 Subject: [PATCH 15/21] Spam filter: compare the first 50 recipients for similar addresses The similar-recipients check ran an edit distance over every pair of To/Cc addresses. A To header with thousands of addresses took time quadratic in its length: 15 s for 5,000, hours for a large header in one inbound message. The check now looks at the first 50, which is plenty to tell a list of near-identical addresses from a real one. --- crates/spam-filter/src/analysis/recipient.rs | 8 ++++++ tests/src/smtp/inbound/antispam.rs | 26 ++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/crates/spam-filter/src/analysis/recipient.rs b/crates/spam-filter/src/analysis/recipient.rs index b04ec5d9..5269e331 100644 --- a/crates/spam-filter/src/analysis/recipient.rs +++ b/crates/spam-filter/src/analysis/recipient.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use std::future::Future; @@ -13,6 +15,9 @@ use store::ahash::HashSet; use crate::SpamFilterContext; +// inbuxa: how many To/Cc addresses the similar-recipients check compares +const SIMILAR_RECIPS_SAMPLE: usize = 50; + pub trait SpamFilterAnalyzeRecipient: Sync + Send { fn spam_filter_analyze_recipient( &self, @@ -243,6 +248,9 @@ impl SpamFilterAnalyzeRecipient for Server { if !is_sorted && rcpt_count >= 5 { // Look for similar recipients + // inbuxa: on the first SIMILAR_RECIPS_SAMPLE only; every pair of a + // long header would take time quadratic in its length + let rcpts = &rcpts[..rcpts.len().min(SIMILAR_RECIPS_SAMPLE)]; let mut hits = 0; let mut combinations = 0; for i in 0..rcpts.len() { diff --git a/tests/src/smtp/inbound/antispam.rs b/tests/src/smtp/inbound/antispam.rs index 0c1e3e18..95e76b88 100644 --- a/tests/src/smtp/inbound/antispam.rs +++ b/tests/src/smtp/inbound/antispam.rs @@ -699,6 +699,32 @@ async fn antispam() { } } } + + // inbuxa: the similar-recipients check used to compare every pair of To/Cc + // addresses, so a long To header took time quadratic in its length. + if filter_test + .as_ref() + .is_none_or(|s| s.eq_ignore_ascii_case("recipient")) + { + println!("===== recipient (many) ====="); + let to = (0..5_000u32) + .map(|i| format!("u{}@d{}.org", i.wrapping_mul(7919) % 5_000, i % 97)) + .collect::>() + .join(", "); + let message = format!("To: {to}\r\nSubject: Hi\r\n\r\nTest\r\n"); + let parsed_message = MessageParser::new().parse(&message).unwrap(); + let session = test.new_mta_session(); + let spam_input = session.build_spam_input(&parsed_message, &[], None, None, None, None); + let mut spam_ctx = test.server.spam_filter_init(spam_input); + let started = Instant::now(); + test.server + .spam_filter_analyze_recipient(&mut spam_ctx) + .await; + let elapsed = started.elapsed(); + assert!(spam_ctx.result.tags.contains("RCPT_COUNT_GT_50")); + assert!(!spam_ctx.result.tags.contains("SORTED_RECIPS")); + assert!(elapsed < Duration::from_secs(2), "{elapsed:?}"); + } } async fn classifier_features(server: &Server, contents: String) { -- 2.54.0 From 42b82f002c6c496be80fad863009f0763cdbe130 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 21:20:30 -0700 Subject: [PATCH 16/21] Spam filter: search only the start of the Subject for recipients Each recipient's address and local part were searched for in the whole Subject, and nothing limits how long a Subject or a To header can be. Both long took time proportional to the two lengths together: 7.4 s for 5,000 recipients and a 2 MB Subject, about a minute for a 1 MB To and a 1 MB Subject, per inbound message. Only the first 1,000 characters of the Subject are searched now. --- crates/spam-filter/src/analysis/recipient.rs | 14 +++++++++----- tests/src/smtp/inbound/antispam.rs | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/crates/spam-filter/src/analysis/recipient.rs b/crates/spam-filter/src/analysis/recipient.rs index 5269e331..b70156e2 100644 --- a/crates/spam-filter/src/analysis/recipient.rs +++ b/crates/spam-filter/src/analysis/recipient.rs @@ -17,6 +17,8 @@ use crate::SpamFilterContext; // inbuxa: how many To/Cc addresses the similar-recipients check compares const SIMILAR_RECIPS_SAMPLE: usize = 50; +// inbuxa: how much of the Subject is searched for each recipient +const SUBJECT_SCAN_LEN: usize = 1_000; pub trait SpamFilterAnalyzeRecipient: Sync + Send { fn spam_filter_analyze_recipient( @@ -155,6 +157,11 @@ impl SpamFilterAnalyzeRecipient for Server { let mut to_dn_count = 0; let mut to_match_envrcpt = 0; + // inbuxa: only the start of the Subject; all of it, once per recipient, + // took time proportional to both the Subject and the recipient list + let subject_lc = + &ctx.output.subject_lc[..ctx.output.subject_lc.floor_char_boundary(SUBJECT_SCAN_LEN)]; + for rcpt in &unique_recipients { // Validate name if let Some(rcpt_name) = &rcpt.name { @@ -172,13 +179,10 @@ impl SpamFilterAnalyzeRecipient for Server { // Check if the local part is present in the subject if !rcpt.email.local_part.is_empty() { - if ctx.output.subject_lc.contains(rcpt.email.address.as_str()) { + if subject_lc.contains(rcpt.email.address.as_str()) { ctx.result.add_tag("RCPT_IN_SUBJECT"); } else if rcpt.email.local_part.len() > 3 - && ctx - .output - .subject_lc - .contains(rcpt.email.local_part.as_str()) + && subject_lc.contains(rcpt.email.local_part.as_str()) { ctx.result.add_tag("RCPT_LOCAL_IN_SUBJECT"); } diff --git a/tests/src/smtp/inbound/antispam.rs b/tests/src/smtp/inbound/antispam.rs index 95e76b88..39ad6219 100644 --- a/tests/src/smtp/inbound/antispam.rs +++ b/tests/src/smtp/inbound/antispam.rs @@ -724,6 +724,21 @@ async fn antispam() { assert!(spam_ctx.result.tags.contains("RCPT_COUNT_GT_50")); assert!(!spam_ctx.result.tags.contains("SORTED_RECIPS")); assert!(elapsed < Duration::from_secs(2), "{elapsed:?}"); + + // inbuxa: and the Subject used to be searched end to end once or twice + // per recipient, so a long one took time proportional to both + let subject = format!("for first@example.org {}", "word ".repeat(400_000)); + let message = format!("To: first@example.org, {to}\r\nSubject: {subject}\r\n\r\nTest\r\n"); + let parsed_message = MessageParser::new().parse(&message).unwrap(); + let spam_input = session.build_spam_input(&parsed_message, &[], None, None, None, None); + let mut spam_ctx = test.server.spam_filter_init(spam_input); + let started = Instant::now(); + test.server + .spam_filter_analyze_recipient(&mut spam_ctx) + .await; + let elapsed = started.elapsed(); + assert!(spam_ctx.result.tags.contains("RCPT_IN_SUBJECT")); + assert!(elapsed < Duration::from_secs(2), "{elapsed:?}"); } } -- 2.54.0 From ffeb1aeb658916e49753afa948ce9749042a2c48 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 21:32:33 -0700 Subject: [PATCH 17/21] JMAP: parse escaped slashes in a key as nested pointers only 8 deep A property key whose first part holds an escaped "/" (~1) is parsed again as a pointer, and each level can escape the one inside it. A key built that way and nested a few thousand deep, well inside the default request size, recursed once per level: on a 2 MiB stack, like a tokio worker's, it overflowed and aborted the server. Past 8 levels the text is now kept as a plain key, which is also what RFC 6901 says "~1" means. --- crates/jmap-proto/src/object/addressbook.rs | 4 +- crates/jmap-proto/src/object/calendar.rs | 4 +- crates/jmap-proto/src/object/email.rs | 4 +- .../jmap-proto/src/object/email_submission.rs | 2 +- crates/jmap-proto/src/object/file_node.rs | 4 +- crates/jmap-proto/src/object/identity.rs | 4 +- crates/jmap-proto/src/object/mailbox.rs | 4 +- crates/jmap-proto/src/object/mod.rs | 68 ++++++++++++++++++- .../src/object/push_subscription.rs | 4 +- 9 files changed, 88 insertions(+), 10 deletions(-) diff --git a/crates/jmap-proto/src/object/addressbook.rs b/crates/jmap-proto/src/object/addressbook.rs index ffff813d..2c0dd735 100644 --- a/crates/jmap-proto/src/object/addressbook.rs +++ b/crates/jmap-proto/src/object/addressbook.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -138,7 +140,7 @@ impl AddressBookProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - AddressBookProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(AddressBookProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/calendar.rs b/crates/jmap-proto/src/object/calendar.rs index e17c6886..d612191c 100644 --- a/crates/jmap-proto/src/object/calendar.rs +++ b/crates/jmap-proto/src/object/calendar.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -246,7 +248,7 @@ impl CalendarProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - CalendarProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(CalendarProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/email.rs b/crates/jmap-proto/src/object/email.rs index 9bf62a64..c4651efd 100644 --- a/crates/jmap-proto/src/object/email.rs +++ b/crates/jmap-proto/src/object/email.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -266,7 +268,7 @@ impl EmailProperty { if let Some(header) = value.strip_prefix("header:") { HeaderProperty::parse(header).map(EmailProperty::Header) } else if allow_patch && value.contains('/') { - EmailProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(EmailProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/email_submission.rs b/crates/jmap-proto/src/object/email_submission.rs index 792a2463..42f1b99c 100644 --- a/crates/jmap-proto/src/object/email_submission.rs +++ b/crates/jmap-proto/src/object/email_submission.rs @@ -196,7 +196,7 @@ impl EmailSubmissionProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - EmailSubmissionProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(EmailSubmissionProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/file_node.rs b/crates/jmap-proto/src/object/file_node.rs index b8d24dbb..f114e486 100644 --- a/crates/jmap-proto/src/object/file_node.rs +++ b/crates/jmap-proto/src/object/file_node.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -281,7 +283,7 @@ impl FileNodeProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - FileNodeProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(FileNodeProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/identity.rs b/crates/jmap-proto/src/object/identity.rs index 4f9b43dd..3be42aae 100644 --- a/crates/jmap-proto/src/object/identity.rs +++ b/crates/jmap-proto/src/object/identity.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::object::{AnyId, JmapObject, JmapObjectId}; @@ -88,7 +90,7 @@ impl IdentityProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - IdentityProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(IdentityProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/mailbox.rs b/crates/jmap-proto/src/object/mailbox.rs index dbdfc964..47ed74c1 100644 --- a/crates/jmap-proto/src/object/mailbox.rs +++ b/crates/jmap-proto/src/object/mailbox.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::{ @@ -169,7 +171,7 @@ impl MailboxProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - MailboxProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(MailboxProperty::Pointer) } else { None } diff --git a/crates/jmap-proto/src/object/mod.rs b/crates/jmap-proto/src/object/mod.rs index 80a40f42..647a63e6 100644 --- a/crates/jmap-proto/src/object/mod.rs +++ b/crates/jmap-proto/src/object/mod.rs @@ -7,9 +7,9 @@ */ use crate::request::deserialize::DeserializeArguments; -use jmap_tools::{Element, Null, Property}; +use jmap_tools::{Element, JsonPointer, Null, Property}; use serde::Serialize; -use std::{fmt::Debug, str::FromStr}; +use std::{cell::Cell, fmt::Debug, str::FromStr}; use types::{acl::Acl, blob::BlobId, id::Id}; pub mod addressbook; @@ -237,3 +237,67 @@ impl TryFrom for Null { unreachable!() } } + +// inbuxa: how deep a key's escaped "/" is parsed as a pointer inside a pointer. +// Each level can escape the one inside it, so without a limit a request could +// nest them thousands deep. Past this, the text is kept as a plain key. +const MAX_POINTER_NESTING: u32 = 8; + +thread_local! { + static POINTER_NESTING: Cell = const { Cell::new(0) }; +} + +struct PointerNesting; + +impl Drop for PointerNesting { + fn drop(&mut self) { + POINTER_NESTING.with(|depth| depth.set(depth.get() - 1)); + } +} + +pub(crate) fn parse_nested_pointer(value: &str) -> Option> { + let level = POINTER_NESTING.with(Cell::get); + if level >= MAX_POINTER_NESTING { + return None; + } + POINTER_NESTING.with(|depth| depth.set(level + 1)); + let _nesting = PointerNesting; + Some(JsonPointer::parse(value)) +} + +// inbuxa: a key holding an escaped "/" is parsed again as a pointer, and each +// level can escape the one inside it. On a 2 MiB stack, like a tokio worker's, +// a key nested a few thousand deep used to overflow it and abort the process. +#[cfg(test)] +mod pointer_nesting_tests { + use super::email::EmailProperty; + use jmap_tools::{JsonPointer, JsonPointerItem, Key}; + + fn nesting(ptr: &JsonPointer) -> usize { + match ptr.first() { + Some(JsonPointerItem::Key(Key::Property(EmailProperty::Pointer(inner)))) => { + 1 + nesting(inner) + } + _ => 0, + } + } + + #[test] + fn nested_pointers_are_bounded() { + let mut key = "a/z".to_string(); + for _ in 0..2_000 { + key = key.replace('~', "~0").replace('/', "~1") + "/z"; + } + let depth = std::thread::Builder::new() + .stack_size(2 * 1024 * 1024) + .spawn(move || nesting(&JsonPointer::::parse(&key))) + .unwrap() + .join() + .unwrap(); + assert!(depth <= 16, "{depth}"); + + // One escaped level still parses as before + let ptr = JsonPointer::::parse("a~1b/c"); + assert_eq!(nesting(&ptr), 1); + } +} diff --git a/crates/jmap-proto/src/object/push_subscription.rs b/crates/jmap-proto/src/object/push_subscription.rs index 6137be54..b5380ec5 100644 --- a/crates/jmap-proto/src/object/push_subscription.rs +++ b/crates/jmap-proto/src/object/push_subscription.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::object::email::{EmailProperty, HeaderForm, HeaderProperty}; @@ -81,7 +83,7 @@ impl PushSubscriptionProperty { ) .or_else(|| { if allow_patch && value.contains('/') { - PushSubscriptionProperty::Pointer(JsonPointer::parse(value)).into() + super::parse_nested_pointer(value).map(PushSubscriptionProperty::Pointer) } else { None } -- 2.54.0 From d71a2ac3dcd5c6a2fda54cb384abdd99781704af Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 21:50:33 -0700 Subject: [PATCH 18/21] Files: FileNode/copy replaces only what the caller may delete, and only once the copy is made With onExists=replace, a copy that collided with an existing node put that node on the list to delete before checking whether the caller could add to the folder, and nothing took it off again when a later check refused the copy. The node and everything under it were then deleted with no Delete check. A sharee who knew a folder id and a name in it could delete any of the owner's files. The folder is now checked before names are looked at, replacing needs Delete on the node and all under it, as FileNode/set's destroy does, and the replaced node is deleted only when the copy is written. --- crates/jmap/src/file/copy.rs | 53 +++++++++++++++------- tests/src/jmap/files/acl.rs | 87 ++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 15 deletions(-) diff --git a/crates/jmap/src/file/copy.rs b/crates/jmap/src/file/copy.rs index e553108e..1dfced31 100644 --- a/crates/jmap/src/file/copy.rs +++ b/crates/jmap/src/file/copy.rs @@ -118,6 +118,9 @@ impl FileNodeCopy for Server { } else { None }; + // inbuxa: onExists=replace deletes what it replaces, as FileNode/set does + let can_delete = (!access_token.is_member(account_id)) + .then(|| cache.shared_containers(access_token, [Acl::Delete], false)); let on_exists = request.arguments.on_exists; let case_insensitive = request .arguments @@ -243,6 +246,23 @@ impl FileNodeCopy for Server { file_node.modified = now() as i64; } + // inbuxa: whether the node may go in this folder comes before any + // name collision there, so a refused copy can't replace what's there + if file_node.parent_id > 0 + && let Some(allowed) = &can_add_to + && !created_folders.contains_key(&(file_node.parent_id - 1)) + && !allowed.contains(file_node.parent_id - 1) + { + response.not_created.append( + id, + SetError::forbidden().with_description( + "You are not allowed to create file nodes in this folder.", + ), + ); + continue 'create; + } + + let mut replaces = None; let renamed = match find_sibling_collision( None, &file_node, @@ -298,8 +318,22 @@ impl FileNodeCopy for Server { .append(id, SetError::node_has_children()); continue 'create; } + if let Some(can_delete) = &can_delete + && !cache + .subtree(target.path()) + .all(|node| can_delete.contains(node.document_id())) + { + response.not_created.append( + id, + SetError::forbidden().with_description( + "You are not allowed to delete the file node this would replace.", + ), + ); + continue 'create; + } } - implicit_destroys.insert(existing); + // inbuxa: deleted only once the copy is made + replaces = Some(existing); false } OnExists::Newest => unreachable!(), @@ -333,20 +367,6 @@ impl FileNodeCopy for Server { if file_node.parent_id > 0 { let parent_id = file_node.parent_id - 1; - // The user must be allowed to add children to the destination parent - if let Some(allowed) = &can_add_to - && !created_folders.contains_key(&parent_id) - && !allowed.contains(parent_id) - { - response.not_created.append( - id, - SetError::forbidden().with_description( - "You are not allowed to create file nodes in this folder.", - ), - ); - continue 'create; - } - let parent_acls = created_folders.get(&parent_id).cloned().or_else(|| { cache .container_resource_by_id(parent_id) @@ -390,6 +410,9 @@ impl FileNodeCopy for Server { .caused_by(trc::location!())?; } + if let Some(existing) = replaces { + implicit_destroys.insert(existing); + } let document_id = self .store() .assign_document_ids(account_id, Collection::FileNode, 1) diff --git a/tests/src/jmap/files/acl.rs b/tests/src/jmap/files/acl.rs index 3df14bcc..d1ef3fb9 100644 --- a/tests/src/jmap/files/acl.rs +++ b/tests/src/jmap/files/acl.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::utils::{jmap::JmapUtils, server::TestServer}; @@ -552,6 +554,91 @@ pub async fn test(test: &TestServer) { let renamed_copy_id = renamed_entry.id().to_string(); assert_eq!(renamed_entry.text_field("name"), "copied-here (2)"); + // inbuxa: onExists=replace on copy deleted the existing node with no Delete + // check, and even when the copy itself was then refused + let john_private_id = john + .jmap_create( + MethodObject::FileNode, + [json!({"name": "private"})], + Vec::<(&str, &str)>::new(), + ) + .await + .created(0) + .id() + .to_string(); + let john_add_only_id = john + .jmap_create( + MethodObject::FileNode, + [json!({ + "name": "add-only", + "shareWith": { &jane_id: { "mayRead": true, "mayAddChildren": true } } + })], + Vec::<(&str, &str)>::new(), + ) + .await + .created(0) + .id() + .to_string(); + let response = john + .jmap_create( + MethodObject::FileNode, + [ + json!({"name": "secret", "parentId": &john_private_id}), + json!({"name": "keep", "parentId": &john_add_only_id}), + ], + Vec::<(&str, &str)>::new(), + ) + .await; + let secret_id = response.created(0).id().to_string(); + let keep_id = response.created(1).id().to_string(); + for (parent_id, name, existing_id) in [ + (&john_private_id, "secret", &secret_id), + (&john_add_only_id, "keep", &keep_id), + ] { + let response = jane + .jmap_method_calls(json!([[ + "FileNode/copy", + { + "fromAccountId": jane.id_string(), + "accountId": john.id_string(), + "onExists": "replace", + "create": { + &jane_folder_id: { "parentId": parent_id, "name": name } + } + }, + "0" + ]])) + .await; + assert_eq!( + john.jmap_get( + MethodObject::FileNode, + [FileNodeProperty::Id], + [existing_id.as_str()], + ) + .await + .list() + .len(), + 1, + "{name} was deleted: {response:?}" + ); + assert_eq!( + response + .method_response() + .pointer(&format!("/notCreated/{jane_folder_id}/type")) + .and_then(|v| v.as_str()), + Some("forbidden"), + "{name}: {response:?}" + ); + } + john.jmap_destroy( + MethodObject::FileNode, + [john_private_id.as_str(), john_add_only_id.as_str()], + [("onDestroyRemoveChildren", true)], + ) + .await + .destroyed() + .for_each(drop); + jane.jmap_destroy( MethodObject::FileNode, [&jane_folder_id], -- 2.54.0 From 389328470c2c6216e39750ef7051853a36bb1bbf Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 22:00:28 -0700 Subject: [PATCH 19/21] Files: a sharee creates and moves file nodes only where it may FileNode/set checked a sharee's rights on the node itself but never on the folder: anyone a folder was shared with could create files in it, read-only or not, and anyone who could rename a node could move it into any of the owner's folders or to the top of the account. FileNode/copy already checked the destination. Creating now needs mayAddChildren on the folder, and moving needs mayDelete on the folder it leaves and mayAddChildren on the one it goes to, never the top. A writing delegate (AL-7) is unaffected. --- crates/jmap/src/file/set.rs | 40 ++++++++++++++++ tests/src/jmap/files/acl.rs | 91 +++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/crates/jmap/src/file/set.rs b/crates/jmap/src/file/set.rs index 4f577c80..380fbb85 100644 --- a/crates/jmap/src/file/set.rs +++ b/crates/jmap/src/file/set.rs @@ -75,6 +75,17 @@ impl FileNodeSet for Server { .with_state(cache.assert_state(false, &request.if_in_state)?); let mut will_destroy = response.collect_will_destroy(request.unwrap_destroy()); let is_shared = access_token.is_shared(account_id); + // inbuxa: where a sharee may add and remove nodes (AL-7: a writing + // delegate may do both anywhere) + let (can_add_to, can_remove_from) = + if is_shared && !access_token.delegate_may_write(account_id) { + ( + Some(cache.shared_containers(access_token, [Acl::AddItems], true)), + Some(cache.shared_containers(access_token, [Acl::RemoveItems], true)), + ) + } else { + (None, None) + }; let on_destroy_remove_children = request .arguments .on_destroy_remove_children @@ -162,6 +173,19 @@ impl FileNodeSet for Server { response.not_created.append(id, err); continue 'create; } + if file_node.parent_id > 0 + && let Some(allowed) = &can_add_to + && !created_folders.contains_key(&(file_node.parent_id - 1)) + && !allowed.contains(file_node.parent_id - 1) + { + response.not_created.append( + id, + SetError::forbidden().with_description( + "You are not allowed to create file nodes in this folder.", + ), + ); + continue 'create; + } if file_node.modified == 0 { file_node.modified = now() as i64; @@ -428,6 +452,22 @@ impl FileNodeSet for Server { response.not_updated.append(id, err); continue 'update; } + let old_parent_id = file_node.inner.parent_id.to_native(); + if new_file_node.parent_id != old_parent_id + && let (Some(may_add), Some(may_remove)) = (&can_add_to, &can_remove_from) + && (old_parent_id == 0 + || !may_remove.contains(old_parent_id - 1) + || new_file_node.parent_id == 0 + || (!created_folders.contains_key(&(new_file_node.parent_id - 1)) + && !may_add.contains(new_file_node.parent_id - 1))) + { + response.not_updated.append( + id, + SetError::forbidden() + .with_description("You are not allowed to move this file node there."), + ); + continue 'update; + } let renamed = match find_sibling_collision( Some(document_id), diff --git a/tests/src/jmap/files/acl.rs b/tests/src/jmap/files/acl.rs index d1ef3fb9..39f53934 100644 --- a/tests/src/jmap/files/acl.rs +++ b/tests/src/jmap/files/acl.rs @@ -639,6 +639,97 @@ pub async fn test(test: &TestServer) { .destroyed() .for_each(drop); + // inbuxa: FileNode/set let a sharee create in any folder it could read, and + // move anything it could rename into any folder or to the top (Jane may + // remove from "rename-only", so only the destination is refused) + let john_read_only_id = john + .jmap_create( + MethodObject::FileNode, + [json!({ + "name": "read-only", + "shareWith": { &jane_id: { "mayRead": true } } + })], + Vec::<(&str, &str)>::new(), + ) + .await + .created(0) + .id() + .to_string(); + let john_rename_only_id = john + .jmap_create( + MethodObject::FileNode, + [json!({ + "name": "rename-only", + "shareWith": { &jane_id: { "mayRead": true, "mayRename": true, "mayDelete": true } } + })], + Vec::<(&str, &str)>::new(), + ) + .await + .created(0) + .id() + .to_string(); + let doc_id = john + .jmap_create( + MethodObject::FileNode, + [json!({"name": "doc", "parentId": &john_rename_only_id})], + Vec::<(&str, &str)>::new(), + ) + .await + .created(0) + .id() + .to_string(); + let response = jane + .jmap_create_account( + john, + MethodObject::FileNode, + [json!({"name": "dropped", "parentId": &john_read_only_id})], + Vec::<(&str, &str)>::new(), + ) + .await; + assert_eq!( + response + .method_response() + .pointer("/notCreated/i0/type") + .and_then(|v| v.as_str()), + Some("forbidden"), + "{response:?}" + ); + for parent_id in [json!(&john_read_only_id), json!(null)] { + let response = jane + .jmap_update_account( + john, + MethodObject::FileNode, + [(&doc_id, json!({ "parentId": parent_id }))], + Vec::<(&str, &str)>::new(), + ) + .await; + assert_eq!( + response + .method_response() + .pointer(&format!("/notUpdated/{doc_id}/type")) + .and_then(|v| v.as_str()), + Some("forbidden"), + "{parent_id}: {response:?}" + ); + } + // Renaming in place still works + jane.jmap_update_account( + john, + MethodObject::FileNode, + [(&doc_id, json!({ "name": "doc (2)" }))], + Vec::<(&str, &str)>::new(), + ) + .await + .updated(&doc_id); + john.jmap_destroy( + MethodObject::FileNode, + [john_read_only_id.as_str(), john_rename_only_id.as_str()], + [("onDestroyRemoveChildren", true)], + ) + .await + .destroyed() + .for_each(drop); + jane.jmap_destroy( MethodObject::FileNode, [&jane_folder_id], -- 2.54.0 From 74053a132b574840921f21fb1e5ac55e7ab20801 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 22:18:11 -0700 Subject: [PATCH 20/21] DAV: moving a whole calendar or address book needs the rights deleting it does A MOVE of a whole collection out of another account checked only that the caller could remove items from it, then took the collection from its owner. A JMAP calendar share with mayWriteAll gives that without mayDelete, so a sharee could move the owner's calendar into their own account. MOVE now needs Delete as well as RemoveItems on the collection, as DELETE does. Address books get the same check; there RemoveItems only ever comes with Delete. --- crates/dav/src/calendar/copy_move.rs | 58 ++++++++++++++++-------- crates/dav/src/card/copy_move.rs | 58 ++++++++++++++++-------- tests/src/webdav/acl.rs | 67 +++++++++++++++++++++++++++- 3 files changed, 146 insertions(+), 37 deletions(-) diff --git a/crates/dav/src/calendar/copy_move.rs b/crates/dav/src/calendar/copy_move.rs index 46e9e19c..66614fd7 100644 --- a/crates/dav/src/calendar/copy_move.rs +++ b/crates/dav/src/calendar/copy_move.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::assert_is_unique_uid; @@ -202,15 +204,25 @@ impl CalendarCopyMoveRequestHandler for Server { // Validate ACLs if !access_token.is_member(to_account_id) || (!access_token.is_member(from_account_id) - && !from_resources.has_access_to_container( - access_token, - from_resource.document_id(), - if is_move { - Acl::RemoveItems - } else { - Acl::ReadItems - }, - )) + // inbuxa: moving a whole collection takes it from its owner, so it + // needs the rights DELETE needs, not just RemoveItems. + && !(if is_move { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::Delete, + ) && from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::RemoveItems, + ) + } else { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::ReadItems, + ) + })) { return Err(DavError::Code(StatusCode::FORBIDDEN)); } @@ -378,15 +390,25 @@ impl CalendarCopyMoveRequestHandler for Server { // Validate ACLs if !access_token.is_member(from_account_id) - && !from_resources.has_access_to_container( - access_token, - from_resource.document_id(), - if is_move { - Acl::RemoveItems - } else { - Acl::ReadItems - }, - ) + // inbuxa: moving a whole collection takes it from its owner, so it + // needs the rights DELETE needs, not just RemoveItems. + && !(if is_move { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::Delete, + ) && from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::RemoveItems, + ) + } else { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::ReadItems, + ) + }) { return Err(DavError::Code(StatusCode::FORBIDDEN)); } diff --git a/crates/dav/src/card/copy_move.rs b/crates/dav/src/card/copy_move.rs index eb4bb9e8..c29dde9c 100644 --- a/crates/dav/src/card/copy_move.rs +++ b/crates/dav/src/card/copy_move.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use super::assert_is_unique_uid; @@ -186,15 +188,25 @@ impl CardCopyMoveRequestHandler for Server { // Validate ACLs if !access_token.is_member(to_account_id) || (!access_token.is_member(from_account_id) - && !from_resources.has_access_to_container( - access_token, - from_resource.document_id(), - if is_move { - Acl::RemoveItems - } else { - Acl::ReadItems - }, - )) + // inbuxa: moving a whole collection takes it from its owner, so it + // needs the rights DELETE needs, not just RemoveItems. + && !(if is_move { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::Delete, + ) && from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::RemoveItems, + ) + } else { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::ReadItems, + ) + })) { return Err(DavError::Code(StatusCode::FORBIDDEN)); } @@ -360,15 +372,25 @@ impl CardCopyMoveRequestHandler for Server { // Validate ACLs if !access_token.is_member(from_account_id) - && !from_resources.has_access_to_container( - access_token, - from_resource.document_id(), - if is_move { - Acl::RemoveItems - } else { - Acl::ReadItems - }, - ) + // inbuxa: moving a whole collection takes it from its owner, so it + // needs the rights DELETE needs, not just RemoveItems. + && !(if is_move { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::Delete, + ) && from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::RemoveItems, + ) + } else { + from_resources.has_access_to_container( + access_token, + from_resource.document_id(), + Acl::ReadItems, + ) + }) { return Err(DavError::Code(StatusCode::FORBIDDEN)); } diff --git a/tests/src/webdav/acl.rs b/tests/src/webdav/acl.rs index 08c1a772..a177df7f 100644 --- a/tests/src/webdav/acl.rs +++ b/tests/src/webdav/acl.rs @@ -6,10 +6,12 @@ * Modified by Coffey Labs in 2026 for INBUXA. */ -use crate::utils::{server::TestServer, webdav::GenerateTestDavResource}; +use crate::utils::{jmap::JmapUtils, server::TestServer, webdav::GenerateTestDavResource}; use dav_proto::schema::property::{DavProperty, WebDavProperty}; use groupware::DavResourceName; use hyper::StatusCode; +use jmap_proto::{object::calendar::CalendarProperty, request::method::MethodObject}; +use serde_json::json; pub async fn test(test: &TestServer) { let owner_client = test.account("bill@example.com").webdav_client(); @@ -443,6 +445,69 @@ pub async fn test(test: &TestServer) { .delete_default_containers_by_account("support@example.com") .await; + // inbuxa: MOVE of a whole calendar takes it from its owner, but it needed + // only RemoveItems, which a JMAP mayWriteAll share grants without mayDelete + let bill = test.account("bill@example.com"); + let john = test.account("john@example.com"); + let owner_calendar = format!( + "{}/bill%40example.com/team-calendar/", + DavResourceName::Cal.base_path() + ); + let sharee_calendar = format!( + "{}/john%40example.com/team-calendar/", + DavResourceName::Cal.base_path() + ); + owner_client + .mkcol( + "MKCALENDAR", + &owner_calendar, + [], + [("D:displayname", "Team calendar")], + ) + .await + .with_status(StatusCode::CREATED); + let calendar_id = bill + .jmap_get( + MethodObject::Calendar, + [CalendarProperty::Id, CalendarProperty::Name], + Vec::<&str>::new(), + ) + .await + .list() + .iter() + .find(|calendar| calendar.text_field("name") == "Team calendar") + .expect("Team calendar") + .id() + .to_string(); + bill.jmap_update( + MethodObject::Calendar, + [( + &calendar_id, + json!({ + format!("shareWith/{}", john.id_string()): { + "mayReadItems": true, + "mayWriteAll": true + } + }), + )], + Vec::<(&str, &str)>::new(), + ) + .await + .updated(&calendar_id); + sharee_client + .request_with_headers( + "MOVE", + &owner_calendar, + [("destination", sharee_calendar.as_str())], + "", + ) + .await + .with_status(StatusCode::FORBIDDEN); + owner_client + .request("DELETE", &owner_calendar, "") + .await + .with_status(StatusCode::NO_CONTENT); + sharee_client.delete_default_containers().await; owner_client.delete_default_containers().await; test.assert_is_empty().await; -- 2.54.0 From 32559d5bd0aef5d6d06ab94a21ef581f0abd27c4 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 27 Oct 2026 22:45:31 -0700 Subject: [PATCH 21/21] Accounts: change or delete an account only holding every permission it has Permissions were compared only when they were what a change touched, and not at all on delete, so an administrator with account-update rights could reset the password of an administrator holding more, or delete the account. Changing or deleting an account now needs every permission it has. SCIM keeps managing every account on its domains, administrators included (SCIM-13). Requests through the SCIM endpoint skip the new check, and the permission change they make is checked for what it adds rather than for everything the account holds, so SCIM can turn off an administrator's sign-in and still can't grant anything its key lacks. --- crates/common/src/auth/permissions.rs | 24 ++++- crates/jmap/src/registry/mapping/principal.rs | 32 ++++++- crates/jmap/src/registry/set.rs | 62 ++++++++++++- crates/scim/src/context.rs | 10 +- tests/src/scim/acceptance.rs | 9 ++ tests/src/system/authorization.rs | 92 +++++++++++++++++++ 6 files changed, 218 insertions(+), 11 deletions(-) diff --git a/crates/common/src/auth/permissions.rs b/crates/common/src/auth/permissions.rs index 7b7b0dac..a43dfa0e 100644 --- a/crates/common/src/auth/permissions.rs +++ b/crates/common/src/auth/permissions.rs @@ -126,6 +126,28 @@ impl Server { access_token: &AccessToken, account: &Account, ) -> trc::Result>> { + self.account_permissions(access_token, account) + .await + .map(|permissions| access_token.can_grant_permissions(permissions)) + } + + /// inbuxa: SCIM-13: only the permissions a change adds to an account + pub async fn can_change_permissions( + &self, + access_token: &AccessToken, + old_account: &Account, + account: &Account, + ) -> trc::Result>> { + let mut added = self.account_permissions(access_token, account).await?; + added.difference(&self.account_permissions(access_token, old_account).await?); + Ok(access_token.can_grant_permissions(added)) + } + + async fn account_permissions( + &self, + access_token: &AccessToken, + account: &Account, + ) -> trc::Result { let (permissions, role_ids, tenant_id) = match account { Account::User(account) => ( &account.permissions, @@ -158,7 +180,7 @@ impl Server { self.effective_permissions(permissions, role_ids, tenant_id) .await - .map(|permissions| access_token.can_grant_permissions(permissions.finalize())) + .map(|permissions| permissions.finalize()) } } diff --git a/crates/jmap/src/registry/mapping/principal.rs b/crates/jmap/src/registry/mapping/principal.rs index 18c4db5a..50285277 100644 --- a/crates/jmap/src/registry/mapping/principal.rs +++ b/crates/jmap/src/registry/mapping/principal.rs @@ -6,7 +6,10 @@ * Modified by Coffey Labs in 2026 for INBUXA. */ -use crate::registry::mapping::{ObjectResponse, ValidationResult}; +use crate::registry::{ + mapping::{ObjectResponse, ValidationResult}, + set::SetOrigin, +}; use common::{ Server, auth::{AccessToken, Permissions, PermissionsGroup, permissions::BuildPermissions}, @@ -37,6 +40,7 @@ pub async fn validate_account( access_token: &AccessToken, mut account: &mut Account, old_account: AccountUpdate<'_>, + origin: SetOrigin, ) -> ValidationResult { let is_external_directory = if let Account::User(account) = account { server @@ -236,10 +240,30 @@ pub async fn validate_account( } }; - let mut result = if validate_permissions { - Ok(server - .can_set_permissions(access_token, account) + // inbuxa: whoever changes an account must hold every permission it has, + // not only when its permissions are what changes; otherwise a limited + // administrator could reset a fuller one's password (SCIM-13: SCIM excepted) + if origin == SetOrigin::Jmap + && let AccountUpdate::Update(old_account) = old_account + && let Err(missing) = server + .can_set_permissions(access_token, old_account) .await? + { + return Ok(Err(build_set_error(missing))); + } + + let mut result = if validate_permissions { + let allowed = match old_account { + // inbuxa: SCIM-13: a change SCIM makes is checked for what it adds, + // so it can turn off an administrator's sign-in + AccountUpdate::Update(old_account) if origin == SetOrigin::Scim => { + server + .can_change_permissions(access_token, old_account, account) + .await? + } + _ => server.can_set_permissions(access_token, account).await?, + }; + Ok(allowed .map(|_| ObjectResponse::default()) .map_err(build_set_error)) } else { diff --git a/crates/jmap/src/registry/set.rs b/crates/jmap/src/registry/set.rs index 43edea29..5fbd3d5c 100644 --- a/crates/jmap/src/registry/set.rs +++ b/crates/jmap/src/registry/set.rs @@ -18,8 +18,8 @@ use crate::registry::{ domain::{validate_dns_server, validate_domain}, map_bootstrap_error, principal::{ - AccountUpdate, schedule_account_destruction, validate_account, validate_role, - validate_tenant_quota, + AccountUpdate, build_set_error, schedule_account_destruction, validate_account, + validate_role, validate_tenant_quota, }, public_key::validate_public_key, queued_message::queued_message_set, @@ -80,6 +80,24 @@ pub trait RegistrySet: Sync + Send { access_token: &AccessToken, session: &HttpSessionData, ) -> impl Future>> + Send; + + fn registry_set_from( + &self, + object_type: ObjectType, + request: SetRequest<'_, Registry>, + access_token: &AccessToken, + session: &HttpSessionData, + origin: SetOrigin, + ) -> impl Future>> + Send; +} + +/// inbuxa: where an `x:*/set` comes from. SCIM manages every account on its +/// domains, administrators included (SCIM-13), so it isn't held to having +/// every permission an account has, only to those it gives. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum SetOrigin { + Jmap, + Scim, } #[allow(clippy::large_enum_variant)] @@ -96,11 +114,29 @@ enum Modification { impl RegistrySet for Server { async fn registry_set( + &self, + object_type: ObjectType, + request: SetRequest<'_, Registry>, + access_token: &AccessToken, + session: &HttpSessionData, + ) -> trc::Result> { + Box::pin(self.registry_set_from( + object_type, + request, + access_token, + session, + SetOrigin::Jmap, + )) + .await + } + + async fn registry_set_from( &self, object_type: ObjectType, mut request: SetRequest<'_, Registry>, access_token: &AccessToken, session: &HttpSessionData, + origin: SetOrigin, ) -> trc::Result> { // Initial assertions if self.registry().is_bootstrap_mode() && !matches!(object_type, ObjectType::Bootstrap) { @@ -470,8 +506,14 @@ impl RegistrySet for Server { let mut tasks = Vec::new(); let result = match &mut new_object.inner { ObjectInner::Account(account) => { - validate_account(self, access_token, account, modification.as_account()) - .await? + validate_account( + self, + access_token, + account, + modification.as_account(), + origin, + ) + .await? } ObjectInner::Role(role) => { validate_role(self, access_token, role, modification.as_role()).await? @@ -837,6 +879,18 @@ impl RegistrySet for Server { && object.inner.account_id() != Some(Id::from(set.account_id)))) }) { + // inbuxa: and deletes it only holding every permission + // it has, as for changing it (SCIM-13: SCIM excepted) + if origin == SetOrigin::Jmap + && let ObjectInner::Account(account) = &object.inner + && let Err(missing) = + self.can_set_permissions(access_token, account).await? + { + set.response + .not_destroyed + .append(id, build_set_error(missing)); + continue; + } // inbuxa: personal-data catalog: a tenant's compliance // role, while nobody holds it, goes first let role_released = matches!(object.inner, ObjectInner::Tenant(_)) diff --git a/crates/scim/src/context.rs b/crates/scim/src/context.rs index 2a4ee041..d42140c7 100644 --- a/crates/scim/src/context.rs +++ b/crates/scim/src/context.rs @@ -13,7 +13,7 @@ use common::{ auth::{AccessToken, DomainCache}, }; use http_proto::HttpSessionData; -use jmap::registry::set::RegistrySet; +use jmap::registry::set::{RegistrySet, SetOrigin}; use jmap_proto::{method::set::SetRequest, object::registry::Registry}; use registry::{ schema::{ @@ -217,7 +217,13 @@ impl<'x> Ctx<'x> { .map_err(|err| server_error(trc::JmapEvent::InvalidArguments.into_err().reason(err)))?; let response = self .server - .registry_set(ObjectType::Account, request, self.token, self.session) + .registry_set_from( + ObjectType::Account, + request, + self.token, + self.session, + SetOrigin::Scim, + ) .await .map_err(|err| { if matches!( diff --git a/tests/src/scim/acceptance.rs b/tests/src/scim/acceptance.rs index a9bad849..b3548eed 100644 --- a/tests/src/scim/acceptance.rs +++ b/tests/src/scim/acceptance.rs @@ -1376,6 +1376,15 @@ async fn adoption(test: &TestServer, scim: &ScimTest) { assert_eq!(user.roles, structs::UserRoles::Admin, "SCIM-32"); assert_eq!(user.credentials.len(), 1, "SCIM-32"); imap_login(&format!("handmade@{SCIM_DOMAIN}"), true).await; + // inbuxa: SCIM-13: and its sign-in can be turned off, like anyone's + scim.client + .patch( + &format!("/Users/{id}"), + patch_body(json!([{"op": "replace", "path": "active", "value": false}])), + ) + .await + .assert_status(200); + imap_login(&format!("handmade@{SCIM_DOMAIN}"), false).await; scim.destroy(&format!("/Users/{id}")).await; } diff --git a/tests/src/system/authorization.rs b/tests/src/system/authorization.rs index 49222af5..11bc7ca0 100644 --- a/tests/src/system/authorization.rs +++ b/tests/src/system/authorization.rs @@ -355,6 +355,98 @@ pub async fn test(test: &mut TestServer) { assert_eq!(objects, vec![john_ids[&object_type]]); } + // inbuxa: an administrator holding fewer permissions than an account used + // to be able to change it (its password too) or delete it; permissions + // were compared only when they were the thing being changed + let limited_role_id = admin + .registry_create_object(Role { + description: "Limited administrator".to_string(), + enabled_permissions: Map::new(vec![ + Permission::Authenticate, + Permission::SysAccountGet, + Permission::SysAccountQuery, + Permission::SysAccountUpdate, + Permission::SysAccountDestroy, + ]), + ..Default::default() + }) + .await; + let minion_role_id = admin + .registry_create_object(Role { + description: "Minion".to_string(), + enabled_permissions: Map::new(vec![Permission::Authenticate]), + ..Default::default() + }) + .await; + let mut account_ids = Vec::new(); + for (name, roles) in [ + ( + "limited", + UserRoles::Custom(CustomRoles { + role_ids: Map::new(vec![limited_role_id]), + }), + ), + ( + "minion", + UserRoles::Custom(CustomRoles { + role_ids: Map::new(vec![minion_role_id]), + }), + ), + ("boss", UserRoles::Admin), + ] { + account_ids.push( + admin + .registry_create_object(structs::Account::User(UserAccount { + name: name.to_string(), + domain_id, + credentials: List::from_iter([Credential::Password(PasswordCredential { + secret: "this is a very strong password".to_string(), + ..Default::default() + })]), + roles, + ..Default::default() + })) + .await, + ); + } + let (limited_id, minion_id, boss_id) = (account_ids[0], account_ids[1], account_ids[2]); + let limited = crate::utils::account::Account::new( + "limited@example.org", + "this is a very strong password", + &[], + "Limited", + limited_id, + ); + // An account it outranks, it may change + limited + .registry_update_object( + ObjectType::Account, + minion_id, + json!({ Property::Description: "Changed by a limited admin" }), + ) + .await; + // One that holds more, it may neither change nor delete + limited + .registry_update_object_expect_err( + ObjectType::Account, + boss_id, + json!({ Property::Description: "Changed by a limited admin" }), + ) + .await + .assert_type(SetErrorType::Forbidden); + limited + .registry_destroy_object_expect_err(ObjectType::Account, boss_id) + .await + .assert_type(SetErrorType::Forbidden); + admin + .registry_destroy(ObjectType::Account, account_ids.clone()) + .await + .assert_destroyed(&account_ids); + admin + .registry_destroy(ObjectType::Role, [limited_role_id, minion_role_id]) + .await + .assert_destroyed(&[limited_role_id, minion_role_id]); + // Destroy test data for (account, ids) in [(&john, &john_ids), (&jane, &jane_ids)] { for (object_type, id) in ids { -- 2.54.0