Normalize CRLF in vCard escaping and ensure FN is never missing after updates

Author: bigcat88

src/nc_mcp_server/tools/contacts.py

@@ -44,7 +44,8 @@
 
 def _vcard_escape(text: str) -> str:
     """Escape a string for use as a vCard 3.0 text value (RFC 2426 Section 2.4.2)."""
-    return text.replace("\\", "\\\\").replace(";", "\\;").replace(",", "\\,").replace("\n", "\\n")
+    normalized = text.replace("\r\n", "\n").replace("\r", "\n")
+    return normalized.replace("\\", "\\\\").replace(";", "\\;").replace(",", "\\,").replace("\n", "\\n")
 
 
 def _normalize_entries(entries: list[dict[str, str]], default_type: str) -> list[dict[str, str]]:
@@ -320,28 +321,50 @@ def _strip_updated_fields(lines: list[str], skip_fields: set[str]) -> list[str]:
     return result
 
 
+def _synthesize_fn(card: Any) -> str:
+    """Derive a display name from a parsed vCard's existing N or FN fields."""
+    old_n = card.get("N")
+    if old_n and hasattr(old_n, "fields"):
+        fn = f"{old_n.fields.given} {old_n.fields.family}".strip()
+        if fn:
+            return fn
+    return str(card.get("FN", ""))
+
+
+def _apply_name_updates(new_lines: list[str], insert_before: int, card: Any, updates: dict[str, Any]) -> bool:
+    """Insert updated N and FN lines. Returns True if FN was inserted."""
+    fn_inserted = False
+    if updates.get("full_name"):
+        new_lines.insert(insert_before, f"FN:{_vcard_escape(updates['full_name'])}")
+        fn_inserted = True
+    if "given_name" not in updates and "family_name" not in updates:
+        return fn_inserted
+    old_n = card.get("N")
+    has_fields = old_n and hasattr(old_n, "fields")
+    family = updates.get("family_name", str(old_n.fields.family) if has_fields else "")
+    given = updates.get("given_name", str(old_n.fields.given) if has_fields else "")
+    additional = str(old_n.fields.additional) if has_fields and old_n.fields.additional else ""
+    prefix = str(old_n.fields.prefix) if has_fields and old_n.fields.prefix else ""
+    suffix = str(old_n.fields.suffix) if has_fields and old_n.fields.suffix else ""
+    n_parts = ";".join(_vcard_escape(p) for p in [family, given, additional, prefix, suffix])
+    new_lines.insert(insert_before, f"N:{n_parts}")
+    if not fn_inserted:
+        new_lines.insert(insert_before, f"FN:{_vcard_escape(f'{given} {family}'.strip())}")
+        fn_inserted = True
+    return fn_inserted
+
+
 def _apply_contact_updates(vcard_data: str, updates: dict[str, Any]) -> str:
     """Apply partial field updates to existing vCard data, return new vCard string."""
     skip_fields = {field for key, field in _UPDATE_FIELD_MAP if key in updates}
     if ("given_name" in updates or "family_name" in updates) and "full_name" not in updates:
         skip_fields.add("FN")
     new_lines = _strip_updated_fields(_unfold_vcard_lines(vcard_data), skip_fields)
     insert_before = len(new_lines) - 1
-    if updates.get("full_name"):
-        new_lines.insert(insert_before, f"FN:{_vcard_escape(updates['full_name'])}")
-    if "given_name" in updates or "family_name" in updates:
-        card = ICal.from_ical(vcard_data)
-        old_n = card.get("N")
-        has_fields = old_n and hasattr(old_n, "fields")
-        family = updates.get("family_name", str(old_n.fields.family) if has_fields else "")
-        given = updates.get("given_name", str(old_n.fields.given) if has_fields else "")
-        additional = str(old_n.fields.additional) if has_fields and old_n.fields.additional else ""
-        prefix = str(old_n.fields.prefix) if has_fields and old_n.fields.prefix else ""
-        suffix = str(old_n.fields.suffix) if has_fields and old_n.fields.suffix else ""
-        n_parts = ";".join(_vcard_escape(p) for p in [family, given, additional, prefix, suffix])
-        new_lines.insert(insert_before, f"N:{n_parts}")
-        if not updates.get("full_name"):
-            new_lines.insert(insert_before, f"FN:{_vcard_escape(f'{given} {family}'.strip())}")
+    card = ICal.from_ical(vcard_data)
+    fn_inserted = _apply_name_updates(new_lines, insert_before, card, updates)
+    if "FN" in skip_fields and not fn_inserted:
+        new_lines.insert(insert_before, f"FN:{_vcard_escape(_synthesize_fn(card))}")
     for key, vcard_field in _SIMPLE_UPDATE_FIELDS:
         if updates.get(key):
             new_lines.insert(insert_before, f"{vcard_field}:{_vcard_escape(updates[key])}")

tests/integration/test_contacts.py

@@ -155,6 +155,18 @@ async def test_create_special_chars_roundtrip(self, nc_mcp: McpTestHelper) -> No
         assert fetched.get("organization") == "R&D <Team>"
         assert fetched.get("title") == 'VP "Sales"'
 
+    @pytest.mark.asyncio
+    async def test_create_note_with_crlf(self, nc_mcp: McpTestHelper) -> None:
+        """Notes with Windows line endings must roundtrip without bare \\r corruption."""
+        contact = await _create(nc_mcp, "crlf-note", note="Line1\r\nLine2\rLine3\nLine4")
+        fetched = json.loads(await nc_mcp.call("get_contact", uid=contact["uid"], book_id=BOOK_ID))
+        note = fetched.get("note", "")
+        assert "Line1" in note
+        assert "Line2" in note
+        assert "Line3" in note
+        assert "Line4" in note
+        assert "\r" not in note
+
     @pytest.mark.asyncio
     async def test_create_no_name_raises(self, nc_mcp: McpTestHelper) -> None:
         with pytest.raises((ToolError, ValueError)):
@@ -352,6 +364,30 @@ async def test_update_folded_note(self, nc_mcp: McpTestHelper) -> None:
         assert updated.get("note") == "Short note"
         assert updated.get("organization") == "Keep Corp"
 
+    @pytest.mark.asyncio
+    async def test_update_clear_full_name_keeps_fn_from_n(self, nc_mcp: McpTestHelper) -> None:
+        """Clearing full_name must not produce a vCard without FN — synthesize from N."""
+        uid = await _put_vcard_with_name(nc_mcp, "clr-fn", given="John", family="Doe")
+        contact = json.loads(await nc_mcp.call("get_contact", uid=uid, book_id=BOOK_ID))
+        updated = json.loads(
+            await nc_mcp.call("update_contact", uid=uid, etag=contact["etag"], full_name="", book_id=BOOK_ID)
+        )
+        assert updated["full_name"], "FN must not be empty after clearing full_name"
+        assert "John" in updated["full_name"]
+        assert "Doe" in updated["full_name"]
+
+    @pytest.mark.asyncio
+    async def test_update_clear_full_name_without_n(self, nc_mcp: McpTestHelper) -> None:
+        """Clearing full_name on a contact with only FN (no structured N) keeps old FN."""
+        created = await _create(nc_mcp, "clr-fn-only")
+        contact = json.loads(await nc_mcp.call("get_contact", uid=created["uid"], book_id=BOOK_ID))
+        original_fn = contact["full_name"]
+        updated = json.loads(
+            await nc_mcp.call("update_contact", uid=created["uid"], etag=contact["etag"], full_name="", book_id=BOOK_ID)
+        )
+        assert updated["full_name"], "FN must not be empty"
+        assert PREFIX in updated["full_name"] or updated["full_name"] == original_fn
+
 
 async def _put_vcard_with_categories(nc_mcp: McpTestHelper, suffix: str, categories: list[str]) -> str:
     """Create a test contact with CATEGORIES via direct CardDAV PUT. Returns UID."""