Skip to main content

max / goingson

Security: fix vCard BDAY panic, scrub legacy plaintext email passwords - vcard.rs: take the BDAY date prefix with `get(..10)` instead of byte indexing, so a malformed multibyte value can't panic on a char boundary (parse_vcf runs in the import command, so a hostile .vcf would abort it). Add a multibyte regression test. - state.rs: on startup, move any plaintext password left in the email_accounts column by an older version into the OS keychain, then blank the column. New accounts are already keychain-only; this stops legacy secrets lingering in the database file. Retries on a future launch if the keychain write fails. Closes the remaining Security-axis MINORs. Clippy clean; vcard tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-22 02:12 UTC
Commit: 184f3a9601f0b7249bb8006b55f12a6efeada8d5
Parent: da9c526
2 files changed, +64 insertions, -2 deletions
M src-tauri/src/external_sync/vcard.rs +20 -2
@@ -175,8 +175,10 @@ fn parse_single_vcard(lines: &[&str]) -> Option<ParsedVCard> {
175 175 } else {
176 176 v.to_string()
177 177 };
178 - if normalized.len() >= 10 {
179 - birthday = Some(normalized[..10].to_string());
178 + // Take the YYYY-MM-DD prefix. `get` (not byte indexing) so a
179 + // malformed multibyte value can't panic on a char boundary.
180 + if let Some(prefix) = normalized.get(..10) {
181 + birthday = Some(prefix.to_string());
180 182 }
181 183 }
182 184 "TZ" => {
@@ -533,6 +535,22 @@ END:VCARD\r\n";
533 535 }
534 536
535 537 #[test]
538 + fn test_birthday_multibyte_does_not_panic() {
539 + // A hostile/garbled BDAY with multibyte UTF-8 must not panic on a
540 + // byte-slice boundary; it is simply not treated as a date.
541 + let vcf = "\
542 + BEGIN:VCARD\r\n\
543 + VERSION:3.0\r\n\
544 + FN:Test\r\n\
545 + BDAY:\u{4f60}\u{597d}\u{4f60}\u{597d}\u{4f60}\u{597d}\r\n\
546 + END:VCARD\r\n";
547 +
548 + let cards = parse_vcf(vcf).unwrap();
549 + // Either None or a safe ASCII prefix; the point is it does not panic.
550 + assert!(cards[0].birthday.as_deref() != Some("\u{4f60}"));
551 + }
552 +
553 + #[test]
536 554 fn test_pref_email() {
537 555 let vcf = "\
538 556 BEGIN:VCARD\r\n\
M src-tauri/src/state.rs +44
@@ -143,6 +143,9 @@ impl AppState {
143 143 .await
144 144 .map_err(|e| format!("Email ID migration failed: {e}"))?;
145 145
146 + // Move any plaintext passwords left by older versions into the keychain.
147 + scrub_legacy_email_passwords(&pool).await;
148 +
146 149 // Ensure desktop user exists (single-user mode)
147 150 ensure_desktop_user_exists(&pool).await?;
148 151
@@ -211,6 +214,47 @@ impl AppState {
211 214 }
212 215 }
213 216
217 + /// One-time scrub: move any plaintext password still in the `email_accounts`
218 + /// column into the OS keychain, then blank the column. New accounts already
219 + /// store secrets keychain-only (the column is `""`); this cleans up accounts
220 + /// created by older versions so plaintext secrets don't linger in the DB file.
221 + async fn scrub_legacy_email_passwords(pool: &SqlitePool) {
222 + let rows: Vec<(String, String)> =
223 + match sqlx::query_as("SELECT id, password FROM email_accounts WHERE password != ''")
224 + .fetch_all(pool)
225 + .await
226 + {
227 + Ok(rows) => rows,
228 + Err(e) => {
229 + warn!("Could not scan for legacy email passwords: {e}");
230 + return;
231 + }
232 + };
233 +
234 + for (id, password) in rows {
235 + let Ok(uuid) = uuid::Uuid::parse_str(&id) else {
236 + continue;
237 + };
238 + // Store in the keychain only if it isn't already there; if that fails,
239 + // leave the column intact and retry on a future launch.
240 + if crate::oauth::CredentialStore::get_password(uuid).is_none()
241 + && let Err(e) = crate::oauth::CredentialStore::store_password(uuid, &password)
242 + {
243 + warn!("Failed to migrate email password to keychain for {id}: {e}");
244 + continue;
245 + }
246 + if let Err(e) = sqlx::query("UPDATE email_accounts SET password = '' WHERE id = ?")
247 + .bind(&id)
248 + .execute(pool)
249 + .await
250 + {
251 + warn!("Failed to blank legacy email password column for {id}: {e}");
252 + } else {
253 + info!("Migrated a legacy email password to the keychain");
254 + }
255 + }
256 + }
257 +
214 258 /// Load a SyncKit API key from the keychain, migrating from plaintext file if needed.
215 259 fn load_api_key(data_dir: &std::path::Path) -> Option<String> {
216 260 // Migrate plaintext file to keychain (one-time)