diff --git a/src/application/services/contact_service.rs b/src/application/services/contact_service.rs index efc0b42a..04ac2dde 100644 --- a/src/application/services/contact_service.rs +++ b/src/application/services/contact_service.rs @@ -103,7 +103,11 @@ impl ContactService { Ok(book) } - fn parse_vcard(&self, vcard_data: &str) -> Result { + // Associated function (no `&self`) so tests in this module + // can call `ContactService::parse_vcard(&body)` directly + // without instantiating a full service (which needs an + // Arc and an Arc). + fn parse_vcard(vcard_data: &str) -> Result { // This is a simplified vCard parser - a real implementation would use a proper vCard library // For now, we'll create a basic contact with minimal data @@ -123,11 +127,15 @@ impl ContactService { contact.set_first_name(Some(parts[1].to_string())); } } else if line.starts_with("EMAIL") { - let value = line.split(':').nth(1).unwrap_or(""); + // Split on the FIRST colon — same rationale as the + // TEL branch below; keeps parameter parsing separate + // from value parsing. + let value = line.split_once(':').map(|(_, v)| v.trim()).unwrap_or(""); if !value.is_empty() { - let email_type = if line.contains("TYPE=HOME") { + let params_upper = line.to_ascii_uppercase(); + let email_type = if params_upper.contains("TYPE=HOME") { "home" - } else if line.contains("TYPE=WORK") { + } else if params_upper.contains("TYPE=WORK") { "work" } else { "other" @@ -140,15 +148,35 @@ impl ContactService { }); } } else if line.starts_with("TEL") { - let value = line.split(':').nth(1).unwrap_or(""); + // Split on the FIRST colon so URI-form values survive. + // Apple Contacts / DAVx⁵ send: + // TEL;TYPE=cell;VALUE=uri:tel:+15551234567 + // The pre-fix `split(':').nth(1)` picked up "tel" + // (the middle segment), silently losing the actual + // phone number. `split_once(':')` splits ONCE at the + // property-name/value boundary; we then strip the + // `tel:` URI scheme if present. + let value = line.split_once(':').map(|(_, v)| v.trim()).unwrap_or(""); + let value = value.strip_prefix("tel:").unwrap_or(value); if !value.is_empty() { - let phone_type = if line.contains("TYPE=CELL") || line.contains("TYPE=MOBILE") { + // RFC 6350 §5.3: parameter values are + // case-insensitive. Match on the uppercase + // form of the whole property line so + // `TYPE=cell` and `TYPE=CELL` both route + // correctly. Pre-fix this was case-sensitive + // and dropped lowercase to "other" — matches + // the shape python-caldav / Apple Contacts + // emit. + let params_upper = line.to_ascii_uppercase(); + let phone_type = if params_upper.contains("TYPE=CELL") + || params_upper.contains("TYPE=MOBILE") + { "mobile" - } else if line.contains("TYPE=HOME") { + } else if params_upper.contains("TYPE=HOME") { "home" - } else if line.contains("TYPE=WORK") { + } else if params_upper.contains("TYPE=WORK") { "work" - } else if line.contains("TYPE=FAX") { + } else if params_upper.contains("TYPE=FAX") { "fax" } else { "other" @@ -160,6 +188,62 @@ impl ContactService { is_primary: contact.phone_is_empty(), // First one is primary }); } + } else if line.starts_with("ADR") { + // ADR (RFC 6350 §6.3.1). Structured value: 7 components + // separated by `;` — (pobox, extended, street, city, + // region, postal, country). Positions 0/1 are legacy + // and typically empty; we preserve positions 2–6 as + // (street, city, state, postal_code, country) which + // matches the emitter format at + // `carddav_adapter.rs::contact_to_vcard`. + // + // Pre-fix parse_vcard had NO ADR handler at all — + // every ADR line sent by a client was silently dropped + // at PUT time, so no address ever survived a + // round-trip. See bug_carddav_parser_gaps.md. + let value = line.split_once(':').map(|(_, v)| v).unwrap_or(""); + let parts: Vec<&str> = value.split(';').collect(); + let field = |i: usize| { + parts + .get(i) + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + }; + let params_upper = line.to_ascii_uppercase(); + let addr_type = if params_upper.contains("TYPE=HOME") { + "home" + } else if params_upper.contains("TYPE=WORK") { + "work" + } else { + "other" + }; + // Only push if AT LEAST one of the useful fields + // is populated — an all-empty ADR line is a + // no-op sent by some clients that "clear" the + // address; storing an empty row would confuse + // downstream UIs. + let street = field(2); + let city = field(3); + let state = field(4); + let postal_code = field(5); + let country = field(6); + if street.is_some() + || city.is_some() + || state.is_some() + || postal_code.is_some() + || country.is_some() + { + let is_primary = contact.address_is_empty(); + contact.push_address(Address { + street, + city, + state, + postal_code, + country, + r#type: addr_type.to_string(), + is_primary, + }); + } } else if let Some(stripped) = line.strip_prefix("ORG:") { contact.set_organization(Some(stripped.to_string())); } else if let Some(stripped) = line.strip_prefix("TITLE:") { @@ -535,7 +619,7 @@ impl ContactUseCase for ContactService { .await?; // Parse vCard data - let mut contact = self.parse_vcard(&dto.vcard)?; + let mut contact = Self::parse_vcard(&dto.vcard)?; // Set address book ID contact.set_address_book_id(address_book_id); @@ -1224,3 +1308,136 @@ impl UserLifecycleHook for DefaultAddressBookLifecycleHook { Ok(()) } } + +// ───────────────────────────────────────────────────────────── +// Tests — parse_vcard property surface +// ───────────────────────────────────────────────────────────── + +#[cfg(test)] +mod parse_vcard_tests { + use super::*; + + /// Wrap minimal vCard 3.0 header/footer around one or more + /// property lines. CRLF-normalise, matching wire format. + fn vcard(lines: &[&str]) -> String { + let mut body = + String::from("BEGIN:VCARD\r\nVERSION:3.0\r\nUID:parse-test\r\nFN:Parse Test\r\n"); + for l in lines { + body.push_str(l); + body.push_str("\r\n"); + } + body.push_str("END:VCARD\r\n"); + body + } + + // ── TEL ─────────────────────────────────────────────────── + + #[test] + fn tel_plain_form_still_parses() { + // Regression pin: pre-existing shape `TEL;TYPE=CELL:+1...` + // (no VALUE=uri) must continue to parse cleanly. + let body = vcard(&["TEL;TYPE=CELL:+15551234567"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.phone().len(), 1); + assert_eq!(c.phone()[0].number, "+15551234567"); + assert_eq!(c.phone()[0].r#type, "mobile"); + } + + #[test] + fn tel_uri_form_survives_first_colon_split() { + // The #528-adjacent bug the fix targets. Pre-fix the + // parser `split(':').nth(1)` would return "tel", losing + // the actual number. + let body = vcard(&["TEL;TYPE=cell;VALUE=uri:tel:+15551234567"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.phone().len(), 1); + assert_eq!( + c.phone()[0].number, + "+15551234567", + "URI-scheme prefix must be stripped so downstream UIs \ + show a clickable number, not `tel:+15551234567`." + ); + assert_eq!(c.phone()[0].r#type, "mobile"); + } + + #[test] + fn tel_uri_form_without_scheme_prefix_survives() { + // Some clients emit VALUE=uri but no explicit `tel:` in + // the value. Handle gracefully — we take everything after + // the first colon and only strip `tel:` if present. + let body = vcard(&["TEL;VALUE=uri:+15551234567"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.phone()[0].number, "+15551234567"); + } + + // ── ADR ─────────────────────────────────────────────────── + + #[test] + fn adr_full_structured_value_populates_all_fields() { + // The reference shape from RFC 6350 §6.3.1: + // ADR;TYPE=HOME:pobox;ext;street;city;region;postal;country + // Positions 0/1 (pobox, ext) are legacy and typically + // empty on real client output; we skip them by design. + let body = vcard(&["ADR;TYPE=HOME:;;42 Rue de Rivoli;Paris;Île-de-France;75001;France"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.address().len(), 1); + let a = &c.address()[0]; + assert_eq!(a.street.as_deref(), Some("42 Rue de Rivoli")); + assert_eq!(a.city.as_deref(), Some("Paris")); + assert_eq!(a.state.as_deref(), Some("Île-de-France")); + assert_eq!(a.postal_code.as_deref(), Some("75001")); + assert_eq!(a.country.as_deref(), Some("France")); + assert_eq!(a.r#type, "home"); + assert!(a.is_primary, "first ADR should be primary"); + } + + #[test] + fn adr_partial_value_only_populates_present_fields() { + // Client sends street + city only — the other structured + // components stay None (not "" — that would confuse the + // Address DTO's Option-based null semantics). + let body = vcard(&["ADR:;;42 Rue de Rivoli;Paris;;;"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.address().len(), 1); + let a = &c.address()[0]; + assert_eq!(a.street.as_deref(), Some("42 Rue de Rivoli")); + assert_eq!(a.city.as_deref(), Some("Paris")); + assert!(a.state.is_none()); + assert!(a.postal_code.is_none()); + assert!(a.country.is_none()); + assert_eq!(a.r#type, "other", "no TYPE param → 'other'"); + } + + #[test] + fn adr_all_empty_is_dropped() { + // Some clients emit `ADR:;;;;;;` as a "clear this + // address" operation. Storing an empty row would show as + // a blank address slot in UIs. Skip it. + let body = vcard(&["ADR:;;;;;;"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.address().len(), 0); + } + + #[test] + fn adr_type_work_recognized() { + let body = vcard(&["ADR;TYPE=WORK:;;5 Wall St;NYC;NY;10005;USA"]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.address()[0].r#type, "work"); + } + + #[test] + fn adr_and_tel_coexist() { + // Two independent fixes on the same PUT should both + // populate — proves neither branch consumes lines meant + // for the other via prefix ambiguity. + let body = vcard(&[ + "TEL;TYPE=CELL:+15551234567", + "ADR;TYPE=HOME:;;42 Rue de Rivoli;Paris;;75001;France", + ]); + let c = ContactService::parse_vcard(&body).expect("valid vcard"); + assert_eq!(c.phone().len(), 1); + assert_eq!(c.phone()[0].number, "+15551234567"); + assert_eq!(c.address().len(), 1); + assert_eq!(c.address()[0].city.as_deref(), Some("Paris")); + } +} diff --git a/src/domain/entities/contact.rs b/src/domain/entities/contact.rs index 7bbc47dc..c67246ac 100644 --- a/src/domain/entities/contact.rs +++ b/src/domain/entities/contact.rs @@ -402,6 +402,9 @@ impl Contact { pub fn push_phone(&mut self, p: Phone) { self.phone.push(p); } + pub fn push_address(&mut self, a: Address) { + self.address.push(a); + } pub fn set_email(&mut self, email: Vec) { self.email = email; } @@ -417,6 +420,9 @@ impl Contact { pub fn phone_is_empty(&self) -> bool { self.phone.is_empty() } + pub fn address_is_empty(&self) -> bool { + self.address.is_empty() + } // --- Consuming methods for ownership transfer --- pub fn into_email(self) -> Vec { diff --git a/src/infrastructure/services/nextcloud_chunked_upload_service.rs b/src/infrastructure/services/nextcloud_chunked_upload_service.rs index fff15b5e..f305fa19 100644 --- a/src/infrastructure/services/nextcloud_chunked_upload_service.rs +++ b/src/infrastructure/services/nextcloud_chunked_upload_service.rs @@ -1,6 +1,5 @@ use std::path::PathBuf; use tokio::fs; -use tokio::io::AsyncWriteExt; use crate::common::errors::{DomainError, Result}; @@ -76,6 +75,20 @@ impl NextcloudChunkedUploadService { /// `interfaces/upload_ingest::stream_body_to_path` helper to stream the /// HTTP body directly to disk and avoid materialising the whole chunk /// in RAM. + /// + /// Uses `tokio::fs::write` (single `spawn_blocking` around + /// `std::fs::write`) rather than manually driving + /// `create + write_all` and letting the tokio handle drop close the + /// fd. The manual shape leaked a race: `tokio::fs::File::drop` + /// dispatches `close(2)` to the blocking pool without awaiting it, + /// and until close completes the dirent update may not be visible + /// to a subsequent `read_dir` — on macOS APFS routinely, on Linux + /// under I/O contention. In practice that turned into + /// `ordered_chunk_paths` silently missing a just-uploaded chunk; + /// the NC assembly path (`handle_assemble` → `ordered_chunk_paths`) + /// would then produce a truncated file with no error to the client. + /// `std::fs::write` opens, writes, and synchronously closes before + /// returning, so the dirent is guaranteed visible on `.await`. pub async fn store_chunk( &self, user: &str, @@ -84,13 +97,9 @@ impl NextcloudChunkedUploadService { data: &[u8], ) -> Result<()> { let chunk_path = self.safe_chunk_path(user, upload_id, chunk_name)?; - let mut file = fs::File::create(&chunk_path) + fs::write(&chunk_path, data) .await - .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?; - file.write_all(data) - .await - .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?; - Ok(()) + .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string())) } /// List the session's chunk files in assembly (numeric) order. diff --git a/tests/api/carddav_vcard_properties.hurl b/tests/api/carddav_vcard_properties.hurl new file mode 100644 index 00000000..f727af56 --- /dev/null +++ b/tests/api/carddav_vcard_properties.hurl @@ -0,0 +1,134 @@ +# ============================================================= +# OxiCloud — CardDAV vCard property round-trip regression +# ============================================================= +# Regression pin for fix/carddav-parser-tel-adr. +# +# `contact_service.rs::parse_vcard` had two independent gaps +# and one case-sensitivity issue on the TYPE parameter: +# +# 1. TEL used `split(':').nth(1)` — a URI-form value like +# `TEL;TYPE=cell;VALUE=uri:tel:+15551234567` was sliced +# down to `"tel"`, losing the phone number entirely. +# 2. ADR had no parser branch at all — every address was +# silently dropped at PUT time. +# 3. TYPE param matching was case-sensitive; real clients +# (Apple Contacts, DAVx⁵, python-caldav) mix cases so +# `TYPE=cell` fell through to "other" instead of "mobile". +# +# Post-fix: `splitn(2, ':')` + `tel:` scheme strip, an ADR +# branch parsing the 7-part structured value into (street, +# city, state, postal_code, country), and case-insensitive +# TYPE matching (uppercased once, checked against upper). +# +# Test shape: PUT a vCard exercising all three fixes; GET it +# back; assert the emitter surfaces the parsed fields. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Admin login. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Discover admin's default address book. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/address-books +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Captures] +# See dav_error_mapping.hurl for why body regex vs jsonpath +# filter — same rationale (Hurl's scalar-vs-list handling on +# single-match jsonpath filters is brittle). +default_book_id: body regex "\"id\":\"([a-f0-9-]{36})\",\"name\":\"Contacts\"" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — PUT a vCard exercising all three fixes: +# * TEL URI form with lowercase TYPE=cell (URI-scheme + case +# insensitivity). +# * ADR with a full 7-field structured value and TYPE=HOME +# (parser branch existence + type detection). +# * EMAIL as a sanity anchor — the pre-existing path we did +# NOT change; must still round-trip cleanly. +# +# `dav-err-` UID prefix so a re-run inside the same DB (this +# file runs BEFORE contacts.hurl in run.sh, so its state +# doesn't collide with that suite). +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/carddav/{{default_book_id}}/dav-err-vcard-props.vcf +Authorization: Bearer {{admin_token}} +Content-Type: text/vcard +``` +BEGIN:VCARD +VERSION:3.0 +UID:dav-err-vcard-props +FN:Regression VCard +N:VCard;Regression;;; +EMAIL;TYPE=work:regression@example.com +TEL;TYPE=cell;VALUE=uri:tel:+15551234567 +ADR;TYPE=HOME:;;42 Rue de Rivoli;Paris;Île-de-France;75001;France +END:VCARD +``` + +HTTP * +[Asserts] +status >= 200 +status < 300 + + +# ───────────────────────────────────────────────────────────── +# Step 4 — GET the vCard back and assert the parser+emitter +# preserved each property. The emitter regenerates the body +# from DTO fields, so a value surfacing in the response body +# is proof it made it through parser → DB → emitter intact. +# +# NOTE: emitter uses vCard 3.0 uppercase TYPE values +# (`TEL;TYPE=MOBILE:`, `ADR;TYPE=HOME:`), so the response +# body's casing is normalised regardless of what the client +# sent. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/carddav/{{default_book_id}}/dav-err-vcard-props.vcf +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +# The bare number, with URI scheme stripped and unchanged +# through the parser's first-colon split. Pre-fix this would +# have been `+15551234567` in the input but the DB would +# store `"tel"` and the emitter would output that instead. +body contains "+15551234567" +# Case-insensitive TYPE detection: lowercase `TYPE=cell` on +# input → mapped to "mobile" internally → emitter writes +# uppercase `TYPE=MOBILE`. Pre-fix (case-sensitive) this fell +# through to "other" and emitted `TYPE=OTHER`. +body contains "TYPE=MOBILE" +# Address components — proves the ADR parser branch runs. +body contains "42 Rue de Rivoli" +body contains "Paris" +body contains "Île-de-France" +body contains "75001" +body contains "France" +# TYPE=HOME preserved from the input (uppercase in both +# directions). +body contains "TYPE=HOME" +# Sanity: pre-existing EMAIL path still round-trips. +body contains "regression@example.com" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Cleanup: delete the vCard so downstream files +# (contacts.hurl in particular) don't inherit the fixture. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/carddav/{{default_book_id}}/dav-err-vcard-props.vcf +Authorization: Bearer {{admin_token}} + +HTTP 204 diff --git a/tests/api/run.sh b/tests/api/run.sh index 3fc6ff98..0341c2f1 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -166,6 +166,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/dedup_blob_cleanup.hurl" \ "$API_DIR/default_caldav_carddav.hurl" \ "$API_DIR/dav_error_mapping.hurl" \ + "$API_DIR/carddav_vcard_properties.hurl" \ "$API_DIR/contacts.hurl" \ "$API_DIR/calendar.hurl" \ "$API_DIR/caldav_recurring.hurl" \