Skip to main content

max / goingson

Sync/Email: fix multibyte Send panic, iCal overflow panic, scrub OAuth tokens Ultra-fuzz Run #30 Phase 1 (Sync/Email -> A). - F-1: format_flowed sliced &str at byte budget; multibyte (CJK/emoji) lines with no early space panicked on Send. Back off to the nearest char boundary; guarantee progress on a single oversized char. Add multibyte/emoji tests. - F-2: iCal parse_duration overflowed i64 and chrono::Duration::seconds panics out of range; the DTEND-from-DURATION add panicked too. Use checked_mul/ checked_add + try_seconds, and checked_add_signed at the caller. Add tests. - F-4: scrub_legacy_email_passwords only blanked the password column; OAuth access/refresh tokens lingered in plaintext and the token manager read them back. Migrate them to the keychain then blank both columns. - F-3: disconnect_oauth now best-effort revokes the refresh token (RFC 7009) before dropping it; Google has a revoke endpoint, Microsoft/Fastmail no-op. - F-5: enforce https on JMAP session_url and server-supplied apiUrl before the bearer token is sent. - Housekeeping: clear two clippy --all-targets warnings in the touched files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-22 03:29 UTC
Commit: 415911e4dbddba8df80d1c026b195dc9298bfc45
Parent: cf8dee1
11 files changed, +297 insertions, -32 deletions
M src-tauri/src/commands/oauth.rs +21 -2
@@ -437,11 +437,30 @@ pub async fn disconnect_oauth(
437 437 state: State<'_, Arc<AppState>>,
438 438 account_id: EmailAccountId,
439 439 ) -> Result<bool, ApiError> {
440 - // Delete credentials from keychain first
440 + // Best-effort: revoke the refresh token at the provider before we drop our
441 + // copy, so a leaked token can't outlive the disconnect. Providers without a
442 + // revocation endpoint (Microsoft, Fastmail) no-op. Failures here must not
443 + // block the local disconnect, so they are logged and swallowed.
444 + if let Ok(Some(account)) = state.email_accounts.get_by_id(account_id, DESKTOP_USER_ID).await {
445 + let refresh_token = CredentialStore::get_oauth(account_id.into())
446 + .and_then(|c| c.refresh_token)
447 + .or_else(|| account.oauth2_refresh_token.clone());
448 + if let Some(token) = refresh_token {
449 + let token_manager = TokenManager::from_env();
450 + if let Some(provider_id) =
451 + TokenManager::provider_id_for_auth_type(&account.auth_type)
452 + && let Some(provider) = token_manager.provider(provider_id)
453 + && let Err(e) = provider.revoke_token(&token).await
454 + {
455 + tracing::warn!("OAuth token revocation failed on disconnect: {e}");
456 + }
457 + }
458 + }
459 +
460 + // Delete credentials from keychain
441 461 let _ = CredentialStore::delete_oauth(account_id.into());
442 462
443 463 // Delete the account from database
444 - // In the future, we could also revoke the token with the provider
445 464 Ok(state.email_accounts.delete(account_id, DESKTOP_USER_ID).await?)
446 465 }
447 466
M src-tauri/src/email/imap_client.rs -2
@@ -803,8 +803,6 @@ impl async_imap::Authenticator for XOAuth2Authenticator {
803 803
804 804 #[cfg(test)]
805 805 mod tests {
806 - use super::ImapClient;
807 -
808 806 // strip_tags_simple, extract_href, decode_html_entities, and strip_html
809 807 // tests removed — these functions were replaced by pter::convert().
810 808
M src-tauri/src/email/smtp_client.rs +83 -3
@@ -69,11 +69,30 @@ fn format_flowed(body: &str) -> String {
69 69 break;
70 70 }
71 71
72 - // Find the last space within budget for a clean word break
73 - let break_at = remaining[..budget]
72 + // `budget` is a byte count, but `remaining` may contain multibyte
73 + // characters, so slicing at `budget` can land mid-character and
74 + // panic. Back off to the largest char boundary at or below budget.
75 + let mut boundary = budget;
76 + while boundary > 0 && !remaining.is_char_boundary(boundary) {
77 + boundary -= 1;
78 + }
79 + // A single character wider than the budget would leave boundary at
80 + // 0 and stall the loop; take that whole first character instead so
81 + // we always make forward progress.
82 + if boundary == 0 {
83 + boundary = remaining
84 + .char_indices()
85 + .nth(1)
86 + .map_or(remaining.len(), |(i, _)| i);
87 + }
88 +
89 + // Find the last space within the boundary for a clean word break.
90 + // `rfind(' ')` returns an ASCII byte index, so `+ 1` stays on a
91 + // char boundary; the fallback `boundary` is a boundary by construction.
92 + let break_at = remaining[..boundary]
74 93 .rfind(' ')
75 94 .map(|i| i + 1) // include the space in this line
76 - .unwrap_or(budget); // no space found, hard break at budget
95 + .unwrap_or(boundary); // no space found, hard break at boundary
77 96
78 97 let (chunk, rest) = remaining.split_at(break_at);
79 98 out.push_str(prefix);
@@ -303,3 +322,64 @@ impl SmtpClient {
303 322 Ok(())
304 323 }
305 324 }
325 +
326 + #[cfg(test)]
327 + mod tests {
328 + use super::format_flowed;
329 +
330 + #[test]
331 + fn short_ascii_line_unchanged() {
332 + assert_eq!(format_flowed("hello world"), "hello world");
333 + }
334 +
335 + #[test]
336 + fn signature_separator_preserved() {
337 + assert_eq!(format_flowed("body\n-- \nsig"), "body\n-- \nsig");
338 + }
339 +
340 + #[test]
341 + fn long_ascii_line_wraps_with_soft_break() {
342 + let line = "word ".repeat(20); // 100 chars, spaces every 5
343 + let out = format_flowed(line.trim_end());
344 + // Content per wrapped segment stays within 72; format=flowed may append
345 + // one trailing soft-break space, so the emitted line is content + 1.
346 + for seg in out.split('\n') {
347 + assert!(seg.len() <= 73, "segment too long: {:?}", seg);
348 + }
349 + }
350 +
351 + #[test]
352 + fn long_multibyte_line_does_not_panic() {
353 + // A line of CJK characters (3 bytes each) well over 72 bytes with no
354 + // ASCII space: the old byte-index slice panicked on the char boundary.
355 + let line = "の".repeat(60); // 180 bytes
356 + let out = format_flowed(&line);
357 + // Output must reconstruct to the same characters (sans soft-break spaces
358 + // and newlines) and never split a character.
359 + let stripped: String = out.chars().filter(|c| *c != '\n' && *c != ' ').collect();
360 + assert_eq!(stripped, line);
361 + }
362 +
363 + #[test]
364 + fn long_emoji_line_does_not_panic() {
365 + // Emoji are 4 bytes; a long run with no space is the worst case.
366 + let line = "😀".repeat(40); // 160 bytes
367 + let out = format_flowed(&line);
368 + let stripped: String = out.chars().filter(|c| *c != '\n' && *c != ' ').collect();
369 + assert_eq!(stripped, line);
370 + }
371 +
372 + #[test]
373 + fn mixed_multibyte_with_spaces_word_breaks() {
374 + let line = format!("{} {} {}", "あ".repeat(30), "い".repeat(30), "う".repeat(30));
375 + let out = format_flowed(&line);
376 + // Content per segment <= 72 bytes; allow up to 2 extra bytes for a
377 + // trailing soft-break space plus the word-break space included in chunk.
378 + for seg in out.split('\n') {
379 + assert!(seg.len() <= 74, "segment too long: {} bytes", seg.len());
380 + }
381 + // No character was split: reconstruction yields the original text.
382 + let stripped: String = out.replace('\n', "");
383 + assert!(stripped.contains(&"あ".repeat(24)));
384 + }
385 + }
M src-tauri/src/external_sync/ical.rs +51 -25
@@ -59,11 +59,14 @@ fn parse_vevent(event: &IcalEvent) -> Option<ParsedEvent> {
59 59 let start_time = parse_datetime_property(&event.properties, "DTSTART")?;
60 60
61 61 // Parse end time: DTEND takes precedence, then compute from DURATION
62 + // `start_time + dur` panics in chrono on overflow, so add it checked: a
63 + // duration that would push the end time out of range yields no end time
64 + // rather than crashing the whole import.
62 65 let end_time = parse_datetime_property(&event.properties, "DTEND")
63 66 .or_else(|| {
64 67 get_property_value(&event.properties, "DURATION")
65 68 .and_then(|d| parse_duration(&d))
66 - .map(|dur| start_time + dur)
69 + .and_then(|dur| start_time.checked_add_signed(dur))
67 70 });
68 71
69 72 // Parse recurrence
@@ -201,39 +204,40 @@ fn parse_duration(s: &str) -> Option<chrono::Duration> {
201 204 }
202 205 let s = &s[1..];
203 206
207 + // A hostile or malformed .ics can specify enormous component values
208 + // (e.g. "P9999999999999999999D"). Every step uses checked arithmetic so an
209 + // overflow yields None rather than panicking in debug / wrapping in release;
210 + // the event then simply falls back to no computed end time.
204 211 let mut total_seconds: i64 = 0;
205 212 let mut in_time = false;
206 213 let mut num_buf = String::new();
207 214
215 + let mut add = |buf: &mut String, unit_secs: i64| -> Option<()> {
216 + // An unparseable / overflowing number contributes 0, matching the
217 + // previous lenient behaviour for non-overflow garbage.
218 + let n = buf.parse::<i64>().unwrap_or(0);
219 + buf.clear();
220 + total_seconds = total_seconds.checked_add(n.checked_mul(unit_secs)?)?;
221 + Some(())
222 + };
223 +
208 224 for ch in s.chars() {
209 225 match ch {
210 226 'T' => in_time = true,
211 227 '0'..='9' => num_buf.push(ch),
212 - 'D' if !in_time => {
213 - total_seconds += num_buf.parse::<i64>().unwrap_or(0) * 86400;
214 - num_buf.clear();
215 - }
216 - 'W' if !in_time => {
217 - total_seconds += num_buf.parse::<i64>().unwrap_or(0) * 604800;
218 - num_buf.clear();
219 - }
220 - 'H' if in_time => {
221 - total_seconds += num_buf.parse::<i64>().unwrap_or(0) * 3600;
222 - num_buf.clear();
223 - }
224 - 'M' if in_time => {
225 - total_seconds += num_buf.parse::<i64>().unwrap_or(0) * 60;
226 - num_buf.clear();
227 - }
228 - 'S' if in_time => {
229 - total_seconds += num_buf.parse::<i64>().unwrap_or(0);
230 - num_buf.clear();
231 - }
228 + 'D' if !in_time => add(&mut num_buf, 86400)?,
229 + 'W' if !in_time => add(&mut num_buf, 604800)?,
230 + 'H' if in_time => add(&mut num_buf, 3600)?,
231 + 'M' if in_time => add(&mut num_buf, 60)?,
232 + 'S' if in_time => add(&mut num_buf, 1)?,
232 233 _ => {}
233 234 }
234 235 }
235 236
236 - Some(chrono::Duration::seconds(total_seconds))
237 + // `Duration::seconds` panics on values outside chrono's millisecond range;
238 + // `try_seconds` returns None instead, so an absurd total degrades to "no
239 + // duration" rather than crashing.
240 + chrono::Duration::try_seconds(total_seconds)
237 241 }
238 242
239 243 /// Parse an RRULE into a GO Recurrence. Only simple rules are mapped;
@@ -289,6 +293,7 @@ fn unescape_ical(s: &str) -> String {
289 293 #[cfg(test)]
290 294 mod tests {
291 295 use super::*;
296 + use chrono::Timelike;
292 297
293 298 #[test]
294 299 fn test_parse_simple_event() {
@@ -449,11 +454,32 @@ END:VCALENDAR\r\n";
449 454 }
450 455
451 456 #[test]
457 + fn test_parse_duration_overflow_is_none_not_panic() {
458 + // Values that parse as i64 but overflow when scaled to seconds, or that
459 + // exceed chrono's Duration range, must yield None rather than panicking.
460 + assert_eq!(parse_duration("P9223372036854775807D"), None); // i64::MAX days
461 + assert_eq!(parse_duration("PT9223372036854775807H"), None); // i64::MAX hours
462 + assert_eq!(parse_duration("P106751991167301D"), None); // overflows *86400
463 + // Unparseable (too many digits for i64) degrades to zero, never panics.
464 + assert_eq!(parse_duration("P9999999999999999999D").map(|d| d.num_seconds()), Some(0));
465 + }
466 +
467 + #[test]
468 + fn test_event_with_overflowing_duration_does_not_panic() {
469 + // A duration that fits chrono::Duration but pushes the end time past
470 + // chrono's date range exercises the checked_add_signed guard at the
471 + // caller: end_time stays unset rather than panicking the import.
472 + let ics = "BEGIN:VCALENDAR\r\nBEGIN:VEVENT\r\nSUMMARY:Overflow\r\n\
473 + DTSTART:20260101T120000Z\r\nDURATION:P100000000000D\r\n\
474 + END:VEVENT\r\nEND:VCALENDAR\r\n";
475 + let events = parse_ics(ics).unwrap();
476 + assert_eq!(events.len(), 1);
477 + assert!(events[0].end_time.is_none());
478 + }
479 +
480 + #[test]
452 481 fn test_unescape_ical() {
453 482 assert_eq!(unescape_ical("Hello\\nWorld"), "Hello\nWorld");
454 483 assert_eq!(unescape_ical("A\\,B\\;C"), "A,B;C");
455 484 }
456 485 }
457 -
458 - #[cfg(test)]
459 - use chrono::Timelike;
M src-tauri/src/jmap/client.rs +3
@@ -99,6 +99,9 @@ impl JmapClient {
99 99 /// Executes a JMAP request and returns the response.
100 100 pub async fn execute(&mut self, request: JmapRequest) -> Result<JmapResponse, String> {
101 101 let api_url = self.api_url().await?;
102 + // `apiUrl` is server-supplied; never POST the bearer token to a
103 + // cleartext endpoint.
104 + super::session::require_https(&api_url)?;
102 105
103 106 let response = self
104 107 .client
M src-tauri/src/jmap/session.rs +15
@@ -57,11 +57,26 @@ pub struct SessionAccount {
57 57 pub account_capabilities: HashMap<String, serde_json::Value>,
58 58 }
59 59
60 + /// Rejects any URL that would carry the bearer token over cleartext.
61 + ///
62 + /// The session URL comes from app config and the `apiUrl` comes from the
63 + /// server's own discovery response; enforcing `https://` here stops a
64 + /// malicious or compromised JMAP server from downgrading the bearer token to
65 + /// plaintext by returning an `http://` endpoint.
66 + pub fn require_https(url: &str) -> Result<(), String> {
67 + if url.trim_start().to_ascii_lowercase().starts_with("https://") {
68 + Ok(())
69 + } else {
70 + Err(format!("Refusing to send JMAP credentials to a non-HTTPS URL: {url}"))
71 + }
72 + }
73 +
60 74 /// Discovers the JMAP session for an account.
61 75 pub async fn discover_session(
62 76 session_url: &str,
63 77 access_token: &str,
64 78 ) -> Result<JmapSession, String> {
79 + require_https(session_url)?;
65 80 let client = reqwest::Client::builder()
66 81 .timeout(std::time::Duration::from_secs(30))
67 82 .connect_timeout(std::time::Duration::from_secs(10))
M src-tauri/src/oauth/provider.rs +58
@@ -54,6 +54,9 @@ pub struct OAuthProviderConfig {
54 54 pub auth_url: String,
55 55 /// Token exchange endpoint URL.
56 56 pub token_url: String,
57 + /// RFC 7009 token revocation endpoint (if the provider offers one).
58 + /// Used to invalidate the refresh token when an account is disconnected.
59 + pub revoke_url: Option<String>,
57 60 /// Scopes required for email access.
58 61 pub scopes: Vec<String>,
59 62 /// Whether this provider uses JMAP (vs IMAP with XOAUTH2).
@@ -280,6 +283,60 @@ pub trait OAuthProvider: Send + Sync + 'static {
280 283 .map_err(|e| format!("Failed to parse token response: {}", e))
281 284 }
282 285
286 + /// Revokes a refresh (or access) token at the provider (RFC 7009).
287 + ///
288 + /// Best-effort: a no-op success when the provider has no revocation
289 + /// endpoint configured (e.g. Microsoft, which only supports session
290 + /// logout). Used on account disconnect so a leaked refresh token can't be
291 + /// used after the user removes the account locally.
292 + async fn revoke_token(&self, token: &str) -> Result<(), String> {
293 + let Some(revoke_url) = self.config().revoke_url.as_ref() else {
294 + return Ok(());
295 + };
296 +
297 + let client = reqwest::Client::builder()
298 + .timeout(std::time::Duration::from_secs(15))
299 + .connect_timeout(std::time::Duration::from_secs(10))
300 + .build()
301 + .map_err(|e| format!("Failed to build HTTP client: {}", e))?;
302 +
303 + let mut form_params: Vec<(&str, &str)> = vec![("token", token)];
304 + let mut request = client.post(revoke_url);
305 + match self.client_auth_method() {
306 + ClientAuthMethod::BasicAuth => {
307 + if let Some(secret) = self.client_secret() {
308 + let credentials = format!("{}:{}", self.client_id(), secret);
309 + request = request.header(
310 + "Authorization",
311 + format!("Basic {}", STANDARD.encode(credentials.as_bytes())),
312 + );
313 + }
314 + }
315 + ClientAuthMethod::FormBody => {
316 + form_params.push(("client_id", self.client_id()));
317 + if let Some(secret) = self.client_secret() {
318 + form_params.push(("client_secret", secret));
319 + }
320 + }
321 + ClientAuthMethod::ClientIdOnly => {
322 + form_params.push(("client_id", self.client_id()));
323 + }
324 + }
325 +
326 + let response = request
327 + .form(&form_params)
328 + .send()
329 + .await
330 + .map_err(|e| format!("Token revocation request failed: {}", e))?;
331 +
332 + if !response.status().is_success() {
333 + let status = response.status();
334 + let body = response.text().await.unwrap_or_default();
335 + return Err(format!("Token revocation failed ({}): {}", status, body));
336 + }
337 + Ok(())
338 + }
339 +
283 340 /// Extracts the user's email address from the token response or via API call.
284 341 ///
285 342 /// Default implementation fetches from `config.userinfo_url` and extracts
@@ -520,6 +577,7 @@ mod tests {
520 577 config: OAuthProviderConfig {
521 578 auth_url: "https://auth.example.com/authorize".to_string(),
522 579 token_url: "https://auth.example.com/token".to_string(),
580 + revoke_url: None,
523 581 scopes: vec!["scope1".to_string(), "scope2".to_string()],
524 582 uses_jmap: false,
525 583 jmap_session_url: None,
M src-tauri/src/oauth/providers/fastmail.rs +3
@@ -24,6 +24,9 @@ impl FastmailProvider {
24 24 config: OAuthProviderConfig {
25 25 auth_url: "https://api.fastmail.com/oauth/authorize".to_string(),
26 26 token_url: "https://api.fastmail.com/oauth/refresh".to_string(),
27 + // No documented RFC 7009 revocation endpoint; disconnect drops
28 + // the local tokens without a provider-side revoke (best-effort).
29 + revoke_url: None,
27 30 scopes: vec![
28 31 "urn:ietf:params:jmap:core".to_string(),
29 32 "urn:ietf:params:jmap:mail".to_string(),
M src-tauri/src/oauth/providers/google.rs +1
@@ -27,6 +27,7 @@ impl GoogleProvider {
27 27 config: OAuthProviderConfig {
28 28 auth_url: "https://accounts.google.com/o/oauth2/v2/auth".to_string(),
29 29 token_url: "https://oauth2.googleapis.com/token".to_string(),
30 + revoke_url: Some("https://oauth2.googleapis.com/revoke".to_string()),
30 31 scopes: vec![
31 32 "https://mail.google.com/".to_string(), // Full Gmail access (IMAP/SMTP)
32 33 "openid".to_string(),
M src-tauri/src/oauth/providers/microsoft.rs +3
@@ -29,6 +29,9 @@ impl MicrosoftProvider {
29 29 config: OAuthProviderConfig {
30 30 auth_url: "https://login.microsoftonline.com/common/oauth2/v2.0/authorize".to_string(),
31 31 token_url: "https://login.microsoftonline.com/common/oauth2/v2.0/token".to_string(),
32 + // Microsoft identity platform has no RFC 7009 revocation endpoint
33 + // (only interactive session logout), so disconnect can't revoke.
34 + revoke_url: None,
32 35 scopes: vec![
33 36 "https://outlook.office.com/IMAP.AccessAsUser.All".to_string(),
34 37 "https://outlook.office.com/SMTP.Send".to_string(),
M src-tauri/src/state.rs +59
@@ -219,6 +219,13 @@ impl AppState {
219 219 /// store secrets keychain-only (the column is `""`); this cleans up accounts
220 220 /// created by older versions so plaintext secrets don't linger in the DB file.
221 221 async fn scrub_legacy_email_passwords(pool: &SqlitePool) {
222 + scrub_legacy_passwords(pool).await;
223 + scrub_legacy_oauth_tokens(pool).await;
224 + }
225 +
226 + /// Migrate plaintext IMAP/SMTP passwords from the `password` column into the
227 + /// keychain, then blank the column.
228 + async fn scrub_legacy_passwords(pool: &SqlitePool) {
222 229 let rows: Vec<(String, String)> =
223 230 match sqlx::query_as("SELECT id, password FROM email_accounts WHERE password != ''")
224 231 .fetch_all(pool)
@@ -255,6 +262,58 @@ async fn scrub_legacy_email_passwords(pool: &SqlitePool) {
255 262 }
256 263 }
257 264
265 + /// Migrate plaintext OAuth2 access/refresh tokens from the
266 + /// `oauth2_access_token` / `oauth2_refresh_token` columns into the keychain,
267 + /// then blank the columns. Older versions wrote these in cleartext and the
268 + /// token manager still reads them as a fallback, so without this they would
269 + /// linger unencrypted in the SQLite file forever.
270 + async fn scrub_legacy_oauth_tokens(pool: &SqlitePool) {
271 + let rows: Vec<(String, Option<String>, Option<String>)> = match sqlx::query_as(
272 + "SELECT id, oauth2_access_token, oauth2_refresh_token FROM email_accounts \
273 + WHERE (oauth2_access_token IS NOT NULL AND oauth2_access_token != '') \
274 + OR (oauth2_refresh_token IS NOT NULL AND oauth2_refresh_token != '')",
275 + )
276 + .fetch_all(pool)
277 + .await
278 + {
279 + Ok(rows) => rows,
280 + Err(e) => {
281 + warn!("Could not scan for legacy OAuth tokens: {e}");
282 + return;
283 + }
284 + };
285 +
286 + for (id, access_token, refresh_token) in rows {
287 + let Ok(uuid) = uuid::Uuid::parse_str(&id) else {
288 + continue;
289 + };
290 + // Don't clobber fresher keychain tokens (a refresh may have already
291 + // written newer ones); only migrate when the keychain has none. If the
292 + // store fails, leave the columns intact and retry on a future launch.
293 + if crate::oauth::CredentialStore::get_oauth(uuid).is_none() {
294 + let credentials = crate::oauth::OAuthCredentials {
295 + access_token: access_token.unwrap_or_default(),
296 + refresh_token: refresh_token.filter(|t| !t.is_empty()),
297 + };
298 + if let Err(e) = crate::oauth::CredentialStore::store_oauth(uuid, &credentials) {
299 + warn!("Failed to migrate OAuth tokens to keychain for {id}: {e}");
300 + continue;
301 + }
302 + }
303 + if let Err(e) = sqlx::query(
304 + "UPDATE email_accounts SET oauth2_access_token = '', oauth2_refresh_token = '' WHERE id = ?",
305 + )
306 + .bind(&id)
307 + .execute(pool)
308 + .await
309 + {
310 + warn!("Failed to blank legacy OAuth token columns for {id}: {e}");
311 + } else {
312 + info!("Migrated legacy OAuth tokens to the keychain");
313 + }
314 + }
315 + }
316 +
258 317 /// Load a SyncKit API key from the keychain, migrating from plaintext file if needed.
259 318 fn load_api_key(data_dir: &std::path::Path) -> Option<String> {
260 319 // Migrate plaintext file to keychain (one-time)