From 184b9dfab6d789881c380ba7ef3209b729bb9e3a Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 14 Jul 2026 16:00:56 +0200 Subject: [PATCH] fix(528): ical and recurrence import use if ical and use ical::IcalParser prepare unit test --- Cargo.lock | 10 + Cargo.toml | 15 + src/domain/entities/calendar_event.rs | 464 +++++++++++++++++++++----- 3 files changed, 410 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed556920..7f61eefc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3155,6 +3155,15 @@ dependencies = [ "cc", ] +[[package]] +name = "ical" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b7cab7543a8b7729a19e2c04309f902861293dcdae6558dfbeb634454d279f6" +dependencies = [ + "thiserror 1.0.69", +] + [[package]] name = "icu_collections" version = "2.2.0" @@ -4247,6 +4256,7 @@ dependencies = [ "http-body 1.0.1", "http-body-util", "http-range-header", + "ical", "id3", "idna", "image", diff --git a/Cargo.toml b/Cargo.toml index e512b4db..c02862b1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,21 @@ flate2 = "1.1.9" tracing = "0.1.44" tracing-subscriber = { version = "0.3.23", features = ["env-filter"] } chrono = { version = "0.4.45", features = ["serde"] } +# RFC 5545 iCalendar parser + emitter. +# +# Adopted 2026-07-14 to replace the hand-rolled property-scan in +# `src/domain/entities/calendar_event.rs::extract_ical_property`, +# which used a naive `format!("\n{}:", name)` substring search and +# refused ANY property carrying parameters (`DTSTART;VALUE=DATE:...`, +# `RECURRENCE-ID;VALUE=DATE:...`, `ATTENDEE;CN=…;PARTSTAT=…:mailto:…`). +# That broke all-day events and made the domain unaware of exception +# instances (see AtalayaLabs/OxiCloud#528). +# +# The crate is the widely-used Rust parser (~1500 SLOC, MIT/Apache), +# actively maintained by @Peltoche as `ical-rs` on GitHub. It handles +# line-folding, escaped characters, parameter maps, and every standard +# component. If a spec conformance gap is found, we contribute upstream. +ical = "0.11" http-body = "1.0.1" serde = { version = "1.0.228", features = ["derive"] } serde_json = "1.0.150" diff --git a/src/domain/entities/calendar_event.rs b/src/domain/entities/calendar_event.rs index ccb301b0..65dbd804 100644 --- a/src/domain/entities/calendar_event.rs +++ b/src/domain/entities/calendar_event.rs @@ -237,24 +237,44 @@ impl CalendarEvent { ) })?; - let dtstart = Self::extract_ical_property(&ical_data, "DTSTART").ok_or_else(|| { - DomainError::new( - ErrorKind::InvalidInput, - "CalendarEvent", - "Missing DTSTART in iCalendar data", - ) - })?; + // DTSTART / DTEND: use the params-aware extractor so we can + // detect `VALUE=DATE` (all-day) from the property parameters + // rather than scanning the raw property line. The pre-parser- + // rewrite substring scan couldn't see param-carrying lines at + // all — see #528. + let (dtstart_value, dtstart_params) = + Self::extract_ical_property_with_params(&ical_data, "DTSTART").ok_or_else(|| { + DomainError::new( + ErrorKind::InvalidInput, + "CalendarEvent", + "Missing DTSTART in iCalendar data", + ) + })?; - let dtend = Self::extract_ical_property(&ical_data, "DTEND").ok_or_else(|| { - DomainError::new( - ErrorKind::InvalidInput, - "CalendarEvent", - "Missing DTEND in iCalendar data", - ) - })?; + let (dtend_value, _dtend_params) = + Self::extract_ical_property_with_params(&ical_data, "DTEND").ok_or_else(|| { + DomainError::new( + ErrorKind::InvalidInput, + "CalendarEvent", + "Missing DTEND in iCalendar data", + ) + })?; - // Parse dates (simplified) - let start_time = Self::parse_ical_datetime(&dtstart).map_err(|e| { + // All-day detection: `VALUE=DATE` parameter on DTSTART. + // Falls back to `false` when the parameter is absent, matching + // RFC 5545 §3.3.4 ("If the property permits, multiple 'VALUE' + // parameters can be specified as a comma-separated list") — + // we're strict: only "DATE" (case-insensitive) counts, "DATE-TIME" + // and anything else means timed. + let all_day = dtstart_params + .get("VALUE") + .map(|vs| { + vs.iter() + .any(|v| v.eq_ignore_ascii_case("DATE")) + }) + .unwrap_or(false); + + let start_time = Self::parse_ical_datetime(&dtstart_value, all_day).map_err(|e| { DomainError::new( ErrorKind::InvalidInput, "CalendarEvent", @@ -262,7 +282,7 @@ impl CalendarEvent { ) })?; - let end_time = Self::parse_ical_datetime(&dtend).map_err(|e| { + let end_time = Self::parse_ical_datetime(&dtend_value, all_day).map_err(|e| { DomainError::new( ErrorKind::InvalidInput, "CalendarEvent", @@ -270,9 +290,6 @@ impl CalendarEvent { ) })?; - // Determine if all-day event (simplified check) - let all_day = dtstart.contains("VALUE=DATE") && !dtstart.contains("T"); - // Extract optional fields let description = Self::extract_ical_property(&ical_data, "DESCRIPTION"); let location = Self::extract_ical_property(&ical_data, "LOCATION"); @@ -557,23 +574,30 @@ impl CalendarEvent { self.description = Self::extract_ical_property(&ical_data, "DESCRIPTION"); self.location = Self::extract_ical_property(&ical_data, "LOCATION"); - if let Some(dtstart) = Self::extract_ical_property(&ical_data, "DTSTART") - && let Ok(start_time) = Self::parse_ical_datetime(&dtstart) + // Extract DTSTART with parameters — needed for the all-day + // detection below AND for the DTSTART/DTEND datetime parsers + // (they need to know whether the value is a date or a datetime). + let dtstart_pair = Self::extract_ical_property_with_params(&ical_data, "DTSTART"); + let all_day = dtstart_pair + .as_ref() + .and_then(|(_v, params)| params.get("VALUE")) + .map(|vs| vs.iter().any(|v| v.eq_ignore_ascii_case("DATE"))) + .unwrap_or(false); + self.all_day = all_day; + + if let Some((value, _params)) = &dtstart_pair + && let Ok(start_time) = Self::parse_ical_datetime(value, all_day) { self.start_time = start_time; } - if let Some(dtend) = Self::extract_ical_property(&ical_data, "DTEND") - && let Ok(end_time) = Self::parse_ical_datetime(&dtend) + if let Some((value, _params)) = + Self::extract_ical_property_with_params(&ical_data, "DTEND") + && let Ok(end_time) = Self::parse_ical_datetime(&value, all_day) { self.end_time = end_time; } - // Update all-day status based on DTSTART - if let Some(dtstart) = Self::extract_ical_property(&ical_data, "DTSTART") { - self.all_day = dtstart.contains("VALUE=DATE") && !dtstart.contains("T"); - } - self.rrule = Self::extract_ical_property(&ical_data, "RRULE"); if let Some(uid) = Self::extract_ical_property(&ical_data, "UID") { @@ -620,17 +644,20 @@ impl CalendarEvent { // or if it ended after the start of our range if let Some(until_pos) = rrule.find("UNTIL=") { let until_start = until_pos + 6; // "UNTIL=" is 6 chars - if let Some(until_end) = rrule[until_start..].find(';') { - let until_str = &rrule[until_start..until_start + until_end]; - if let Ok(until_date) = Self::parse_ical_datetime(until_str) { - return until_date >= *start; - } + let until_str = if let Some(until_end) = rrule[until_start..].find(';') { + &rrule[until_start..until_start + until_end] } else { // UNTIL is the last part of the rule - let until_str = &rrule[until_start..]; - if let Ok(until_date) = Self::parse_ical_datetime(until_str) { - return until_date >= *start; - } + &rrule[until_start..] + }; + // RFC 5545 §3.3.10 — UNTIL is either a DATE (`YYYYMMDD`, + // 8 chars) or a DATE-TIME (`YYYYMMDDTHHMMSSZ`, 16 chars, + // trailing Z). Distinguish by shape: exactly 8 chars ⇒ + // date-only. Everything else is treated as datetime and + // parsed accordingly. + let is_date_only = until_str.len() == 8; + if let Ok(until_date) = Self::parse_ical_datetime(until_str, is_date_only) { + return until_date >= *start; } } else { // No UNTIL specified, so recurrence continues indefinitely @@ -646,60 +673,123 @@ impl CalendarEvent { /** * Extracts a property value from iCalendar data. * + * Backed by the `ical` crate's RFC 5545 parser (see `Cargo.toml` + * doc-comment on the dep). The pre-2026-07-14 hand-rolled scan + * looked for `\n:` and refused any parameter-carrying + * property (`DTSTART;VALUE=DATE:20260101`, + * `RECURRENCE-ID;VALUE=DATE:...`, `ATTENDEE;CN=…;PARTSTAT=…:…`) — + * see AtalayaLabs/OxiCloud#528. + * + * The current implementation reads the first VEVENT from the raw + * body via `IcalParser` and returns the named property's `value` + * (parameters discarded — use `extract_ical_property_with_params` + * for callers that care about `VALUE=DATE`, `TZID`, etc.). + * + * Returns `None` when the property is missing, has an empty value, + * or the body isn't parseable as iCalendar. Whole-body parse + * failures collapse to `None` rather than surface — same behaviour + * as the pre-rewrite hand-rolled scan, which just returned `None` + * on any mismatch. If callers need to distinguish "missing" from + * "unparseable body", they should use `parse_first_vevent` directly. + * * @param ical_data The iCalendar data to search in * @param property_name The name of the property to extract * @return Option containing the property value if found */ fn extract_ical_property(ical_data: &str, property_name: &str) -> Option { - // Find the property in the iCalendar data - let search_str = format!("\n{}:", property_name); - let search_str_alt = format!("\r\n{}:", property_name); + Self::extract_ical_property_with_params(ical_data, property_name).map(|(v, _p)| v) + } - let pos = ical_data - .find(&search_str) - .or_else(|| ical_data.find(&search_str_alt)); - - if let Some(pos) = pos { - // Find the start of the value - let value_start = pos + search_str.len(); - - // Find the end of the value (next line or end of string) - let value_end = ical_data[value_start..] - .find('\n') - .map(|p| value_start + p) - .unwrap_or_else(|| ical_data.len()); - - // Extract and return the value - let value = ical_data[value_start..value_end].trim(); - if !value.is_empty() { - return Some(value.to_string()); + /// Extract a property's value AND parameter map. Same lookup rules + /// as `extract_ical_property`; the second element is a map keyed by + /// parameter name (`"VALUE"`, `"TZID"`, `"CN"`, …) whose value is + /// the list of parameter values (parameters can be multi-valued — + /// `MEMBER="mailto:a@x","mailto:b@x"` — hence the `Vec` + /// per key). + /// + /// Callers that only need the value should use `extract_ical_property`; + /// this variant is for DTSTART / DTEND / RECURRENCE-ID which need + /// `VALUE=DATE` detection to distinguish all-day from timed events. + fn extract_ical_property_with_params( + ical_data: &str, + property_name: &str, + ) -> Option<(String, std::collections::HashMap>)> { + let event = Self::parse_first_vevent(ical_data)?; + let prop = event + .properties + .into_iter() + .find(|p| p.name.eq_ignore_ascii_case(property_name))?; + let value = prop.value?; + if value.trim().is_empty() { + return None; + } + let mut params: std::collections::HashMap> = + std::collections::HashMap::new(); + if let Some(param_list) = prop.params { + for (name, values) in param_list { + // RFC 5545 property parameter names are ASCII case-insensitive. + // Normalise to UPPER so callers key on a canonical form. + params.insert(name.to_ascii_uppercase(), values); } } + Some((value.trim().to_string(), params)) + } + /// Parse the raw iCalendar body and return the first VEVENT + /// component's properties. Returns `None` on any parse failure or + /// if the body carries zero events (e.g. a `VCALENDAR` with only + /// VTODOs — not our concern for the events surface). + /// + /// Delegated to the `ical` crate's `IcalParser`, which handles + /// line-folding, escaped characters, and RFC 5545 parameter syntax. + fn parse_first_vevent(ical_data: &str) -> Option { + use std::io::BufReader; + let reader = BufReader::new(ical_data.as_bytes()); + let parser = ical::IcalParser::new(reader); + for cal in parser { + let Ok(cal) = cal else { continue }; + if let Some(event) = cal.events.into_iter().next() { + return Some(event); + } + } None } /** * Parses an iCalendar datetime string into a DateTime object. * - * @param datetime The iCalendar datetime string to parse + * @param value The property value (already stripped of parameters + * by the ical-crate-backed extractor). + * @param is_date_only True when the source line carried + * `VALUE=DATE` (all-day event) — caller derives + * this from `extract_ical_property_with_params`. * @return Result containing the parsed DateTime or an error */ - fn parse_ical_datetime(datetime: &str) -> std::result::Result, String> { - // Handle VALUE=DATE format - if datetime.contains("VALUE=DATE") { - let date_str = datetime.split(':').next_back().unwrap_or(""); - if date_str.len() != 8 { - return Err("Invalid date format".to_string()); + fn parse_ical_datetime( + value: &str, + is_date_only: bool, + ) -> std::result::Result, String> { + // All-day form — YYYYMMDD, 8 chars, no time component. Caller + // signalled this via the `VALUE=DATE` parameter on the source + // property. Pre-2026-07-14 this was detected by scanning the + // raw property line for the substring `VALUE=DATE`, which + // failed because `extract_ical_property` refused to return + // param-carrying lines at all (see #528). + if is_date_only { + if value.len() != 8 { + return Err(format!( + "Invalid all-day date format: expected YYYYMMDD (8 chars), got {} chars", + value.len() + )); } - let year = date_str[0..4] + let year = value[0..4] .parse::() .map_err(|_| "Invalid year".to_string())?; - let month = date_str[4..6] + let month = value[4..6] .parse::() .map_err(|_| "Invalid month".to_string())?; - let day = date_str[6..8] + let day = value[6..8] .parse::() .map_err(|_| "Invalid day".to_string())?; @@ -709,29 +799,33 @@ impl CalendarEvent { }; } - // Handle standard UTC format (20230101T120000Z) - let datetime_str = datetime.split(':').next_back().unwrap_or(datetime); - if datetime_str.len() < 15 || !datetime_str.ends_with('Z') { - return Err("Invalid datetime format".to_string()); + // Standard UTC form: YYYYMMDDTHHMMSSZ, 16 chars, trailing 'Z'. + // Floating-time (no 'Z') and TZID-anchored forms aren't yet + // supported — future work when we tackle VTIMEZONE properly. + if value.len() < 15 || !value.ends_with('Z') { + return Err(format!( + "Invalid datetime format: expected YYYYMMDDTHHMMSSZ, got {:?}", + value + )); } - let year = datetime_str[0..4] + let year = value[0..4] .parse::() .map_err(|_| "Invalid year".to_string())?; - let month = datetime_str[4..6] + let month = value[4..6] .parse::() .map_err(|_| "Invalid month".to_string())?; - let day = datetime_str[6..8] + let day = value[6..8] .parse::() .map_err(|_| "Invalid day".to_string())?; - let hour = datetime_str[9..11] + let hour = value[9..11] .parse::() .map_err(|_| "Invalid hour".to_string())?; - let minute = datetime_str[11..13] + let minute = value[11..13] .parse::() .map_err(|_| "Invalid minute".to_string())?; - let second = datetime_str[13..15] + let second = value[13..15] .parse::() .map_err(|_| "Invalid second".to_string())?; @@ -816,3 +910,215 @@ impl CalendarEvent { } } } + +#[cfg(test)] +mod ical_parser_tests { + //! Regression tests for the `ical`-crate-backed property extractor. + //! + //! Every shape here failed under the pre-2026-07-14 hand-rolled + //! `find("\n:")` scan (see AtalayaLabs/OxiCloud#528). Fixtures + //! are RFC 5545-shaped; when we bundle real client bodies from + //! Thunderbird / DAVx⁵ / Gnome Calendar the mapping will follow the + //! same style — each case declares which shape it exercises. + //! + //! Fixture sources / attributions: + //! * RFC 5545 §3.6.1 (VEVENT baseline) — timed event example + //! * RFC 5545 §3.8.2.4 (DTSTART DATE form) — all-day event + //! * RFC 5545 §3.8.4.4 (RECURRENCE-ID) — exception instance + //! * Shape adapted from Radicale test fixtures — RRULE + UNTIL + //! with a DATE-form UNTIL for an all-day recurring event + //! + //! Everything is spec-shaped and byte-small; no network / no + //! external files. Real client bodies can be added later under + //! `tests/fixtures/ical/` and loaded via `include_str!`. + + use super::*; + + /// Simple timed VEVENT. Baseline sanity — this shape worked pre- + /// rewrite (no property parameters), so it's the regression floor. + const TIMED_EVENT: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:timed-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260101T120000Z\r +DTEND:20260101T130000Z\r +SUMMARY:Timed baseline\r +END:VEVENT\r +END:VCALENDAR\r +"; + + /// All-day VEVENT — the exact shape #528 flagged. Property line + /// carries `;VALUE=DATE:` which the old scan refused; the crate- + /// backed extractor now parses it and the all-day flag is derived + /// from the `VALUE` parameter. + const ALL_DAY_EVENT: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:allday-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260201\r +DTEND;VALUE=DATE:20260202\r +SUMMARY:All-day event\r +END:VEVENT\r +END:VCALENDAR\r +"; + + /// Timed recurring master with a modified single occurrence + /// (RECURRENCE-ID identifies which instance). The exception VEVENT + /// shares the master's UID and adds `RECURRENCE-ID:` to pinpoint + /// the overridden date. This is the #528 shape — parser must not + /// choke on the presence of RECURRENCE-ID even though we don't + /// route it into the domain yet (that's phase 2). + const RECURRING_WITH_EXCEPTION: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260101T090000Z\r +DTEND:20260101T100000Z\r +SUMMARY:Daily standup\r +RRULE:FREQ=DAILY;COUNT=10\r +END:VEVENT\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260103T110000Z\r +DTEND:20260103T120000Z\r +SUMMARY:Daily standup — rescheduled\r +RECURRENCE-ID:20260103T090000Z\r +END:VEVENT\r +END:VCALENDAR\r +"; + + /// All-day recurring with an all-day exception — the most-broken + /// case in #528 (RECURRENCE-ID;VALUE=DATE:...). Parser must accept + /// the parameter on both DTSTART and RECURRENCE-ID. + const ALL_DAY_RECURRING_WITH_EXCEPTION: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260105\r +DTEND;VALUE=DATE:20260106\r +SUMMARY:Weekly all-day\r +RRULE:FREQ=WEEKLY;COUNT=4\r +END:VEVENT\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260113\r +DTEND;VALUE=DATE:20260114\r +SUMMARY:Weekly all-day — rescheduled\r +RECURRENCE-ID;VALUE=DATE:20260112\r +END:VEVENT\r +END:VCALENDAR\r +"; + + fn parse_ok(body: &str) -> CalendarEvent { + CalendarEvent::from_ical(Uuid::new_v4(), body.to_string()) + .expect("expected successful parse") + } + + #[test] + fn timed_event_parses_and_is_not_all_day() { + let ev = parse_ok(TIMED_EVENT); + assert_eq!(ev.summary(), "Timed baseline"); + assert!(!ev.all_day()); + } + + #[test] + fn all_day_event_parses_and_flags_as_all_day() { + // Regression: DTSTART;VALUE=DATE:20260201 used to fail + // property-extraction ("Missing DTSTART") because the raw + // scan required a colon directly after the property name. + let ev = parse_ok(ALL_DAY_EVENT); + assert!(ev.all_day(), "VALUE=DATE parameter should flag all-day"); + assert_eq!( + ev.start_time().date_naive().to_string(), + "2026-02-01", + "DTSTART value should parse the YYYYMMDD payload" + ); + } + + #[test] + fn recurring_with_exception_still_returns_the_master() { + // The crate parses BOTH events from the VCALENDAR body; our + // `parse_first_vevent` returns the first, which is the master. + // Exception routing is phase 2 — this test locks the current + // "first event wins" behavior so phase 2 knows what it's + // extending. + let ev = parse_ok(RECURRING_WITH_EXCEPTION); + assert_eq!(ev.summary(), "Daily standup"); + assert_eq!(ev.ical_uid(), "daily-1@oxicloud.test"); + assert_eq!(ev.rrule().as_deref(), Some("FREQ=DAILY;COUNT=10")); + } + + #[test] + fn all_day_recurring_with_exception_master_parses() { + // The #528 shape end-to-end: parameterised DTSTART on both the + // master and the exception, plus a parameterised RECURRENCE-ID. + // Pre-rewrite this was a 400 (post the error-mapping fix) or 500 + // (before it); post-rewrite the master parses cleanly and the + // all_day flag is set from the master's DTSTART parameters. + let ev = parse_ok(ALL_DAY_RECURRING_WITH_EXCEPTION); + assert!(ev.all_day()); + assert_eq!(ev.ical_uid(), "weekly-allday@oxicloud.test"); + } + + #[test] + fn missing_dtstart_still_returns_a_useful_error() { + // Preserve the pre-rewrite error contract for the genuinely- + // missing case. `dav_error_mapping.hurl` asserts this shape. + let body = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:missing-dtstart@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTEND:20260101T130000Z\r +SUMMARY:No DTSTART\r +END:VEVENT\r +END:VCALENDAR\r +"; + let err = CalendarEvent::from_ical(Uuid::new_v4(), body.to_string()) + .expect_err("expected InvalidInput for missing DTSTART"); + assert_eq!(err.kind, ErrorKind::InvalidInput); + assert!( + err.message.contains("DTSTART"), + "message should mention DTSTART, got: {}", + err.message + ); + } + + #[test] + fn extract_property_with_params_returns_parameter_map() { + // Direct test of the params-aware extractor. Confirms + // parameter names are normalised to uppercase and preserved + // as a list (RFC 5545 §3.2 — parameters can carry multiple + // comma-separated values). + let (value, params) = + CalendarEvent::extract_ical_property_with_params(ALL_DAY_EVENT, "DTSTART") + .expect("DTSTART must extract"); + assert_eq!(value, "20260201"); + let vals = params.get("VALUE").expect("VALUE param must be present"); + assert_eq!(vals, &vec!["DATE".to_string()]); + } + + #[test] + fn extract_property_case_insensitive_property_name() { + // Property names are ASCII case-insensitive per RFC 5545 §3.1. + // The lookup must accept "dtstart" as well as "DTSTART". + let v = CalendarEvent::extract_ical_property(TIMED_EVENT, "dtstart"); + assert_eq!(v.as_deref(), Some("20260101T120000Z")); + } +}