fixup(ews): address review blockers + substantive items

Blockers (from review):
- Migration 050_provider_type renumbered to 054_provider_type to avoid
  collision with upstream main's 051-053. src/db.rs migration array +
  CLAUDE.md index updated.
- Migration-count assertions in db.rs tests bumped from 50 to 54.

Substantive review items folded in:
- sync_folder_items now caps at 200 SOAP iterations; a server that
  never sets IncludesLastItemInRange=true returns partial results and
  is resumed from the latest cursor next sync. (olivierlambert#8)
- synth_vcalendar emits DTSTAMP per RFC 5545; EWS doesn't expose a
  stable last-modified, so we use Utc::now(). (olivierlambert#6)
- EWS sync_delta comment rewritten to acknowledge "EWS sources rely
  on full fetches" rather than claiming the cursor gets bootstrapped.
  (olivierlambert#3, option A)
- fetch_events_since wired into sync_ews_source with the same 90-day
  lookback used by the CalDAV path. The EWS impl uses CalendarView
  (server-side window). remove_orphaned_ews_events now takes the same
  since_prefix scoping as the CalDAV variant so events outside the
  window are not flagged as orphans. (olivierlambert#4)
- format_dt TODO documenting naive-local TZID drift on non-recurring
  items. (olivierlambert#7)
- Autodiscover redirect policy now carries an explicit comment about
  the SSRF residual risk on intermediate Location headers. (olivierlambert#5)
- 054_provider_type.sql explains the calendars.href / calendars.ctag
  reuse for EWS folder ItemId / ChangeKey. (olivierlambert#9)
- extract_vcalendar comment corrected: the \r\n /\r\n\t replacements
  are RFC 5545 §3.1 line-fold unfolding, not "stray indentation".

Rebase side-effects:
- SSRF guard now uses upstream's CALRS_ALLOW_PRIVATE_HOSTS allowlist
  (hostname-scoped) instead of the previous boolean toggle. private_
  host_allowlist() is exposed for the serve startup WARN log.
- Sources flow (commands/source.rs, commands/sync.rs, web/mod.rs)
  reconciled with upstream's OAuth2 dispatch + orphan-cancel work:
  EWS sources go through the provider trait, CalDAV sources keep the
  CaldavClient path (basic-auth or OAuth2 unchanged).
- new sync_ews_source/upsert_calendar_provider/upsert_provider_events
  helpers in commands/sync.rs to keep the EWS path off the CalDAV
  CaldavClient signature.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: Arthur Perrot

CLAUDE.md

@@ -93,7 +93,7 @@ calrs/
 │   ├── 042_event_transp.sql      ← TRANSP column on events (skip TRANSPARENT)
 │   ├── 043_event_type_watchers.sql ← event_type_watchers junction (team watches event type)
 │   ├── 044_booking_claim.sql     ← claimed_by_user_id/claimed_at on bookings + booking_claim_tokens
-│   └── 050_provider_type.sql     ← provider_type on caldav_sources (caldav/ews) for the calendar-provider abstraction
+│   └── 054_provider_type.sql     ← provider_type on caldav_sources (caldav/ews) for the calendar-provider abstraction
 ├── templates/
 │   ├── base.html                 ← base layout + CSS (light/dark mode)
 │   ├── dashboard_base.html       ← sidebar layout (extends base.html, all dashboard pages extend this)

migrations/054_provider_type.sql

@@ -1,5 +1,15 @@
 -- Add provider_type column to caldav_sources so a single sources table can
 -- describe multiple back-end protocols (CalDAV, EWS, ...). Existing rows keep
 -- the historical default of 'caldav'.
+--
+-- Schema reuse note: the `calendars` table keeps its CalDAV-era column names
+-- when populated by an EWS source. Specifically:
+--   * `calendars.href` holds the EWS folder ItemId (opaque base64-ish blob).
+--   * `calendars.ctag` holds the EWS ChangeKey, if any.
+-- The semantics are identical (opaque change marker / opaque resource id)
+-- and the rest of the codebase treats them that way via the provider trait
+-- (`crate::providers::RemoteCalendar`). Renaming the columns would force a
+-- table-rebuild migration on existing CalDAV deployments for no behavioural
+-- gain, so we accept the misleading names and surface them here.
 ALTER TABLE caldav_sources ADD COLUMN provider_type TEXT NOT NULL DEFAULT 'caldav';
 CREATE INDEX IF NOT EXISTS idx_caldav_sources_provider_type ON caldav_sources(provider_type);

src/commands/source.rs

@@ -362,31 +362,30 @@ pub async fn run(pool: &SqlitePool, key: &[u8; 32], cmd: SourceCommands) -> Resu
                     // OAuth2 sources are CalDAV-only (Google). Basic-auth
                     // sources may be CalDAV or EWS; let the provider factory
                     // pick the right back-end.
-                    let client: Box<dyn crate::providers::CalendarProvider> = if auth_type
-                        == "oauth2"
-                    {
-                        let caldav = crate::oauth2_caldav::build_client_for_source(
-                            pool,
-                            key,
-                            &source_id,
-                            &url,
-                            &auth_type,
-                            &username,
-                            password_enc.as_deref(),
-                            access_token_enc.as_deref(),
-                            token_expires_at.as_deref(),
-                        )
-                        .await?;
-                        Box::new(crate::providers::caldav::CaldavProvider::from_client(
-                            caldav,
-                        ))
-                    } else {
-                        let enc = password_enc.as_deref().ok_or_else(|| {
-                            anyhow::anyhow!("Basic auth source missing password")
-                        })?;
-                        let password = crate::crypto::decrypt_password(key, enc)?;
-                        build_provider(&provider_type, &url, &username, &password)?
-                    };
+                    let client: Box<dyn crate::providers::CalendarProvider> =
+                        if auth_type == "oauth2" {
+                            let caldav = crate::oauth2_caldav::build_client_for_source(
+                                pool,
+                                key,
+                                &source_id,
+                                &url,
+                                &auth_type,
+                                &username,
+                                password_enc.as_deref(),
+                                access_token_enc.as_deref(),
+                                token_expires_at.as_deref(),
+                            )
+                            .await?;
+                            Box::new(crate::providers::caldav::CaldavProvider::from_client(
+                                caldav,
+                            ))
+                        } else {
+                            let enc = password_enc.as_deref().ok_or_else(|| {
+                                anyhow::anyhow!("Basic auth source missing password")
+                            })?;
+                            let password = crate::crypto::decrypt_password(key, enc)?;
+                            build_provider(&provider_type, &url, &username, &password)?
+                        };
                     match client.check_connection().await {
                         Ok(true) => println!("{} Connection OK", "✓".green()),
                         Ok(false) => println!("{} Connected, partial detection", "⚠".yellow()),

src/commands/sync.rs

@@ -78,13 +78,14 @@ pub async fn run(pool: &SqlitePool, key: &[u8; 32], full: bool) -> Result<()> {
         // EWS sources go through the provider trait (no OAuth2, no CalDAV-only
         // sync-collection); CalDAV sources keep the existing flow.
         if provider_type == kinds::EWS {
-            let password = match crate::crypto::decrypt_password(key, password_enc.as_deref().unwrap_or("")) {
-                Ok(p) => p,
-                Err(e) => {
-                    println!("  {} Decrypt failed: {}", "✗".red(), e);
-                    continue;
-                }
-            };
+            let password =
+                match crate::crypto::decrypt_password(key, password_enc.as_deref().unwrap_or("")) {
+                    Ok(p) => p,
+                    Err(e) => {
+                        println!("  {} Decrypt failed: {}", "✗".red(), e);
+                        continue;
+                    }
+                };
             let provider =
                 match crate::providers::build_provider(provider_type, url, username, &password) {
                     Ok(p) => p,
@@ -370,22 +371,16 @@ pub async fn sync_if_stale(pool: &SqlitePool, key: &[u8; 32], user_id: &str) {
         }
 
         if provider_type == kinds::EWS {
-            let password = match crate::crypto::decrypt_password(
-                key,
-                password_enc.as_deref().unwrap_or(""),
-            ) {
-                Ok(p) => p,
-                Err(_) => continue,
-            };
-            let provider = match crate::providers::build_provider(
-                provider_type,
-                url,
-                username,
-                &password,
-            ) {
-                Ok(p) => p,
-                Err(_) => continue,
-            };
+            let password =
+                match crate::crypto::decrypt_password(key, password_enc.as_deref().unwrap_or("")) {
+                    Ok(p) => p,
+                    Err(_) => continue,
+                };
+            let provider =
+                match crate::providers::build_provider(provider_type, url, username, &password) {
+                    Ok(p) => p,
+                    Err(_) => continue,
+                };
             let _ = sync_ews_source(pool, key, provider.as_ref(), source_id).await;
             continue;
         }
@@ -501,15 +496,25 @@ pub async fn sync_ews_source(
 ) -> Result<()> {
     let calendars = provider.list_calendars().await?;
 
+    // Bounded fetch window. Matches the CalDAV path's FULL_FETCH_LOOKBACK_DAYS:
+    // 90 days back is plenty for orphan reconciliation and keeps EWS response
+    // sizes predictable. The provider's fetch_events_since uses CalendarView,
+    // which expands recurrences server-side within the window.
+    let since_dt = Utc::now() - chrono::Duration::days(FULL_FETCH_LOOKBACK_DAYS);
+    let since_iso = since_dt.to_rfc3339();
+    let since_prefix = since_dt.format("%Y%m%d").to_string();
+
     for cal_info in &calendars {
         let (cal_id, _stored_change_marker, _stored_sync_state) =
             upsert_calendar_provider(pool, source_id, cal_info).await?;
         let cal_label = cal_info.display_name.as_deref().unwrap_or(&cal_info.id);
 
-        match provider.fetch_events(&cal_info.id).await {
+        match provider.fetch_events_since(&cal_info.id, &since_iso).await {
             Ok(raw_events) => {
                 let count = upsert_provider_events(pool, &cal_id, &raw_events).await;
-                let deleted = remove_orphaned_ews_events(pool, key, &cal_id, &raw_events).await;
+                let deleted =
+                    remove_orphaned_ews_events(pool, key, &cal_id, &raw_events, &since_prefix)
+                        .await;
                 if deleted > 0 {
                     tracing::info!(
                         calendar_name = cal_label,
@@ -656,16 +661,21 @@ async fn upsert_provider_events(
     count
 }
 
-/// EWS variant of orphan reconciliation: full sweep without a time-range
-/// bound (EWS `FindItem` returns everything in the folder by default).
-/// `client = None` skips the HTTP confirm-before-cancel verification used in
-/// the CalDAV path — EWS double-booking deletes go through the provider
-/// trait, and the CalDAV `CaldavClient` is the wrong type here.
+/// EWS variant of orphan reconciliation, scoped to the fetched window.
+/// `since_prefix` is a `YYYYMMDD` lower bound matching the
+/// `fetch_events_since` call: events with `start_at` before it weren't in
+/// the response and must not be flagged as orphans. Pass an empty string to
+/// reconcile against every local event.
+///
+/// `client = None` is implied: EWS sources can't be HTTP-verified against a
+/// `CaldavClient`, so we go straight to DB cancellation when an event has
+/// vanished from the server.
 async fn remove_orphaned_ews_events(
     pool: &SqlitePool,
     key: &[u8; 32],
     cal_id: &str,
     raw_events: &[ProviderRawEvent],
+    since_prefix: &str,
 ) -> u32 {
     let mut seen_uids: Vec<(String, String)> = Vec::new();
     for raw in raw_events {
@@ -681,10 +691,17 @@ async fn remove_orphaned_ews_events(
         return 0;
     }
 
+    // Same window-scoping trick as the CalDAV path: compact ("YYYYMMDDTHHMMSS")
+    // and all-day ("YYYYMMDD") start_at values both sort against a YYYYMMDD
+    // lower bound.
     let local_events: Vec<(String, String, Option<String>)> = sqlx::query_as(
-        "SELECT id, uid, recurrence_id FROM events WHERE calendar_id = ?",
+        "SELECT id, uid, recurrence_id FROM events
+         WHERE calendar_id = ?
+           AND (? = '' OR start_at >= ?)",
     )
     .bind(cal_id)
+    .bind(since_prefix)
+    .bind(since_prefix)
     .fetch_all(pool)
     .await
     .unwrap_or_default();
@@ -697,9 +714,6 @@ async fn remove_orphaned_ews_events(
                 .bind(event_id)
                 .execute(pool)
                 .await;
-            // Same booking-cancel semantics as the CalDAV path, minus the
-            // CalDAV-only HTTP double-check: there's no `CaldavClient` to
-            // verify with for EWS sources.
             cancel_orphaned_booking_simple(pool, key, uid).await;
             deleted += 1;
         }
@@ -711,10 +725,12 @@ async fn remove_orphaned_ews_events(
 /// confirmed booking by UID and marks it cancelled. Skips the
 /// `cancel_orphaned_booking` HTTP confirm step (CalDAV-specific).
 async fn cancel_orphaned_booking_simple(pool: &SqlitePool, _key: &[u8; 32], uid: &str) {
-    let _ = sqlx::query("UPDATE bookings SET status = 'cancelled' WHERE uid = ? AND status = 'confirmed'")
-        .bind(uid)
-        .execute(pool)
-        .await;
+    let _ = sqlx::query(
+        "UPDATE bookings SET status = 'cancelled' WHERE uid = ? AND status = 'confirmed'",
+    )
+    .bind(uid)
+    .execute(pool)
+    .await;
 }
 
 // --- Helper functions ---

src/ews/autodiscover.rs

@@ -39,6 +39,15 @@ pub async fn discover_ews_url(email: &str, password: &str) -> Result<String> {
         .timeout(TIMEOUT)
         // Follow up to two redirects — Autodiscover often replies 302 to a
         // canonical URL on the same domain.
+        //
+        // Known limitation: the SSRF validator only runs on the initial
+        // candidate URL, not on intermediate Location headers. A malicious
+        // autodiscover responder could redirect to a private hostname mid
+        // chain. The chain is HTTPS, so the attacker would also need a
+        // valid cert for the private host; we accept the residual risk and
+        // rely on the egress firewall mitigation (see docs/src/security.md).
+        // The server-supplied EwsUrl from the final response is revalidated
+        // below before being persisted.
         .redirect(Policy::limited(2))
         .build()
         .context("HTTP client build failed")?;

src/ews/ical.rs

@@ -48,12 +48,18 @@ pub fn synth_vcalendar(item: &EwsCalendarItem) -> Option<String> {
         "CONFIRMED"
     };
 
+    // RFC 5545 requires DTSTAMP on every VEVENT. EWS doesn't expose a stable
+    // "last modified" we can map to it, so we emit "now" — strict consumers
+    // get a valid VEVENT, calrs availability code ignores DTSTAMP.
+    let dtstamp = chrono::Utc::now().format("%Y%m%dT%H%M%SZ").to_string();
+
     let mut buf = String::new();
     buf.push_str("BEGIN:VCALENDAR\r\n");
     buf.push_str("VERSION:2.0\r\n");
     buf.push_str("PRODID:-//calrs//ews-bridge//EN\r\n");
     buf.push_str("BEGIN:VEVENT\r\n");
     buf.push_str(&format!("UID:{uid}\r\n"));
+    buf.push_str(&format!("DTSTAMP:{dtstamp}\r\n"));
     buf.push_str(&format!("DTSTART{dtstart}\r\n"));
     buf.push_str(&format!("DTEND{dtend}\r\n"));
     if !summary.is_empty() {
@@ -73,6 +79,14 @@ pub fn synth_vcalendar(item: &EwsCalendarItem) -> Option<String> {
 /// `2026-05-08T00:00:00`) as the iCal property value, including the right
 /// VALUE/TZID hint. EWS stores naive-local-with-timezone or UTC; the
 /// generated iCal mirrors the source semantics.
+///
+/// TODO: naive-local datetimes are currently emitted as floating values
+/// (no `TZID` parameter). EWS normally returns UTC, but tenants with a
+/// non-UTC default may surface naive locals, which would be misread as
+/// the host's local time during availability checks. Recurring events
+/// already escape via the MIME path; the gap is limited to non-recurring
+/// naive-local items synthesised from `FindItem`. Tracking in the issue
+/// tracker for a follow-up.
 fn format_dt(value: &str, all_day: bool) -> String {
     if all_day {
         // All-day events use VALUE=DATE with YYYYMMDD.

src/ews/mod.rs

@@ -123,11 +123,12 @@ impl CalendarProvider for EwsProvider {
         // Cursor-seeding mode (see trait docs): the caller has already
         // populated the local cache via `fetch_events` and only wants a
         // starting cursor. EWS's `SyncFolderItems` without a state walks
-        // the entire folder before returning a cursor — extremely costly
-        // on busy mailboxes. We skip it: the next background full sync
-        // will bootstrap the cursor naturally on the first incremental
-        // call. Until then, EWS sources fall back to `fetch_events_since`,
-        // which uses a `CalendarView` window and is already efficient.
+        // the entire folder before returning one, which is prohibitively
+        // costly on large mailboxes (and there's no smaller cursor-only
+        // EWS call to swap in). For now, EWS sources rely on full fetches
+        // via `fetch_events` — `stored_sync_state` stays `None` and every
+        // sync re-fetches the folder. The follow-up is to swap in a
+        // `CalendarView`-based incremental sync, see issue tracker.
         if sync_state.is_none() {
             return Ok(DeltaResult::default());
         }

src/ews/operations.rs

@@ -318,17 +318,21 @@ pub async fn delete_item(
 ///
 /// `sync_state` is the opaque cursor returned by the previous call (or
 /// `None` for the initial sync). We request `MaxChangesReturned=512` and
-/// loop until the server reports `IncludesLastItemInRange=true`.
+/// loop until the server reports `IncludesLastItemInRange=true`, with a
+/// hard cap of `MAX_SYNC_PAGES` iterations as insurance against a server
+/// that never sets the terminator (200 pages × 512 changes = 102 400
+/// items, far above any realistic mailbox).
 pub async fn sync_folder_items(
     endpoint: &str,
     username: &str,
     password: &str,
     folder_id: &str,
     sync_state: Option<&str>,
 ) -> Result<EwsSyncDelta> {
+    const MAX_SYNC_PAGES: usize = 200;
     let mut state = sync_state.map(str::to_string);
     let mut all = EwsSyncDelta::default();
-    loop {
+    for _ in 0..MAX_SYNC_PAGES {
         let state_xml = state
             .as_deref()
             .map(|s| format!("      <m:SyncState>{}</m:SyncState>\n", escape(s)))
@@ -361,8 +365,17 @@ pub async fn sync_folder_items(
         state = all.new_sync_state.clone();
 
         if page.includes_last {
-            break;
+            return Ok(all);
         }
     }
+    // Hit the safety cap without seeing IncludesLastItemInRange=true: either
+    // the server is broken or the mailbox is genuinely enormous. Return what
+    // we have plus the latest cursor so the next sync can resume.
+    tracing::warn!(
+        folder_id = %folder_id,
+        pages = MAX_SYNC_PAGES,
+        "SyncFolderItems hit the safety cap without IncludesLastItemInRange=true; \
+         returning partial result, next sync will resume from cursor"
+    );
     Ok(all)
 }

src/ews/parse.rs

@@ -199,7 +199,9 @@ pub fn extract_vcalendar(mime: &str) -> Option<String> {
     let close = end + "END:VCALENDAR".len();
     let mut block = mime[begin..close].to_string();
     // MIME line endings are CRLF; iCal already requires CRLF, so we leave it
-    // alone — but normalise stray CR or accidental indentation.
+    // alone. RFC 5545 §3.1 long-content lines are folded with a CRLF followed
+    // by a single space or tab; unfold them here so downstream parsers see
+    // logical lines rather than the wire format.
     block = block.replace("\r\n ", "");
     block = block.replace("\r\n\t", "");
     Some(block)