From 723aee9134e3b7fe63e4672c54283e4d16dcbc74 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 23 Apr 2026 07:28:05 +0200 Subject: [PATCH 1/7] fix(contacts): persist all documented fields on create (fixes #716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create_contact previously read only fn/email/tel from contact_data and silently dropped org, organization, note, title, nickname, bday, categories, url — and didn't accept phone as an alias for tel, so the reporter's exact call lost every field except fn and email. Introduce _build_contact_from_data, share it with update_contact's fallback, and normalise str→list inputs so pythonvCard4 doesn't iterate bare strings character-by-character for list-typed properties. Closes #716 Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 117 ++++++++++++++++-- nextcloud_mcp_server/server/contacts.py | 18 ++- .../contacts/test_contacts_operations.py | 44 +++++++ tests/server/test_contacts_mcp.py | 11 +- 4 files changed, 174 insertions(+), 16 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index a9a8e457..256a8c16 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -11,6 +11,108 @@ from .base import BaseNextcloudClient logger = logging.getLogger(__name__) +# Keys that _build_contact_from_data consumes. Used to warn (not error) on unknown keys. +_SUPPORTED_CONTACT_KEYS = frozenset( + { + "fn", + "email", + "tel", + "phone", + "org", + "organization", + "note", + "title", + "nickname", + "bday", + "categories", + "url", + } +) + + +def _wrap_contact_field(value) -> list[dict]: + """Normalize an email/tel/url input into pythonvCard4's list-of-dicts shape. + + Accepts a plain string, a dict already in ``{value, type}`` form, or a list of + either. Empty strings are dropped. Always returns a list (possibly empty). + """ + if value is None or value == "": + return [] + items = value if isinstance(value, list) else [value] + out: list[dict] = [] + for item in items: + if isinstance(item, dict) and item.get("value"): + types = item.get("type") or ["HOME"] + out.append({"value": item["value"], "type": list(types)}) + elif isinstance(item, str) and item: + out.append({"value": item, "type": ["HOME"]}) + return out + + +def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: + """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict. + + Maps every key documented on ``nc_contacts_create_contact`` onto the underlying + library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char + iteration of bare strings — see issue #716. + """ + + # pythonvCard4 iterates bare strings character-by-character for list-typed fields + # (ORG, NICKNAME, CATEGORIES, URL), producing garbage like ``ORG:A;c;m;e``. Wrap + # single strings in a list to keep the vCard well-formed. + def _as_list(value): + if isinstance(value, list): + return value + if isinstance(value, str) and "," in value: + return [v.strip() for v in value.split(",") if v.strip()] + return [value] + + kwargs: dict = {"fn": contact_data.get("fn"), "uid": uid} + + emails = _wrap_contact_field(contact_data.get("email")) + if emails: + kwargs["email"] = emails + + tels = _wrap_contact_field(contact_data.get("tel") or contact_data.get("phone")) + if tels: + kwargs["tel"] = tels + + org_value = contact_data.get("org") or contact_data.get("organization") + if org_value: + kwargs["org"] = _as_list(org_value) + + if contact_data.get("note"): + kwargs["note"] = contact_data["note"] + + if contact_data.get("title"): + kwargs["title"] = contact_data["title"] + + if contact_data.get("nickname"): + kwargs["nickname"] = _as_list(contact_data["nickname"]) + + if contact_data.get("categories"): + kwargs["categories"] = _as_list(contact_data["categories"]) + + if contact_data.get("url"): + kwargs["url"] = _as_list(contact_data["url"]) + + bday = contact_data.get("bday") + if bday: + if isinstance(bday, date): + kwargs["bday"] = bday + elif isinstance(bday, str): + try: + kwargs["bday"] = date.fromisoformat(bday) + except ValueError: + logger.warning("Ignoring non-ISO bday value: %r", bday) + + unknown = set(contact_data) - _SUPPORTED_CONTACT_KEYS + if unknown: + logger.debug("Ignoring unknown contact_data keys: %s", sorted(unknown)) + + return Contact(**kwargs) # type: ignore[arg-type] + + class ContactsClient(BaseNextcloudClient): """Client for NextCloud CardDAV contact operations.""" @@ -127,13 +229,7 @@ class ContactsClient(BaseNextcloudClient): carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{uid}.vcf" - contact = Contact(fn=contact_data.get("fn"), uid=uid) # type: ignore - if "email" in contact_data: - contact.email = [{"value": contact_data["email"], "type": ["HOME"]}] - if "tel" in contact_data: - contact.tel = [{"value": contact_data["tel"], "type": ["HOME"]}] - - vcard = contact.to_vcard() + vcard = _build_contact_from_data(contact_data, uid).to_vcard() headers = { "Content-Type": "text/vcard; charset=utf-8", @@ -177,12 +273,7 @@ class ContactsClient(BaseNextcloudClient): ) else: # Fallback to creating new vCard if we couldn't get existing - contact = Contact(fn=contact_data.get("fn"), uid=uid) # type: ignore - if "email" in contact_data: - contact.email = [{"value": contact_data["email"], "type": ["HOME"]}] - if "tel" in contact_data: - contact.tel = [{"value": contact_data["tel"], "type": ["HOME"]}] - vcard_content = contact.to_vcard() + vcard_content = _build_contact_from_data(contact_data, uid).to_vcard() headers = { "Content-Type": "text/vcard; charset=utf-8", diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 2becd064..90935b05 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -191,7 +191,23 @@ def configure_contacts_tools(mcp: FastMCP): not the display name. Use nc_contacts_list_addressbooks to find available URI slugs. uid: The unique ID for the contact. - contact_data: A dictionary with the contact's details, e.g. {"fn": "John Doe", "email": "john.doe@example.com"}. + contact_data: A dictionary with the contact's details. Supported keys: + + - ``fn`` (str, required): Formatted full name. + - ``email`` (str or list of str/dicts): Email address(es). + - ``tel`` / ``phone`` (str or list): Phone number(s). + - ``org`` / ``organization`` (str or list of str): Organization. + Lists become semicolon-separated ORG components per RFC 6350. + - ``title`` (str): Job title. + - ``note`` (str): Free-form note. + - ``nickname`` (str or list of str). + - ``bday`` (ISO date str ``"YYYY-MM-DD"`` or ``datetime.date``). + - ``categories`` (list of str, or comma-separated str). + - ``url`` (str or list of str). + + Unknown keys are ignored. Example: + ``{"fn": "John Doe", "email": "john@example.com", + "organization": "Acme", "note": "Met at conference"}``. """ client = await get_client(ctx) return await client.contacts.create_contact( diff --git a/tests/client/contacts/test_contacts_operations.py b/tests/client/contacts/test_contacts_operations.py index 85628dc9..63317174 100644 --- a/tests/client/contacts/test_contacts_operations.py +++ b/tests/client/contacts/test_contacts_operations.py @@ -86,3 +86,47 @@ async def test_full_contact_workflow( contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name) contact_uids = [c["vcard_id"] for c in contacts] assert contact_uid not in contact_uids + + +async def test_create_contact_persists_all_documented_fields( + nc_client: NextcloudClient, temporary_addressbook: str +): + """Regression for issue #716: org/note/phone/organization must persist to the vCard. + + Historically ``create_contact`` only handled fn/email/tel and silently dropped every + other key. Inspect the raw server-side vCard (not just the parsed list response) to + confirm each documented field round-trips. + """ + addressbook_name = temporary_addressbook + contact_uid = f"test-full-{uuid.uuid4().hex[:8]}" + contact_data = { + "fn": "Full Field User", + "email": "full@example.com", + "phone": "555-0716", # alias for tel + "organization": "Acme Corp", # alias for org + "note": "Issue 716 regression", + "title": "Engineer", + "url": "https://example.com", + } + + await nc_client.contacts.create_contact( + addressbook=addressbook_name, + uid=contact_uid, + contact_data=contact_data, + ) + try: + raw_vcard, _etag = await nc_client.contacts._get_raw_vcard( + addressbook_name, contact_uid + ) + assert "FN:Full Field User" in raw_vcard + assert "EMAIL" in raw_vcard and "full@example.com" in raw_vcard + assert "TEL" in raw_vcard and "555-0716" in raw_vcard + assert "ORG:Acme Corp" in raw_vcard + assert "NOTE:Issue 716 regression" in raw_vcard + assert "TITLE:Engineer" in raw_vcard + # Sabre rewrites bare URL: to URL;VALUE=URI: on PUT + assert "URL" in raw_vcard and "https://example.com" in raw_vcard + finally: + await nc_client.contacts.delete_contact( + addressbook=addressbook_name, uid=contact_uid + ) diff --git a/tests/server/test_contacts_mcp.py b/tests/server/test_contacts_mcp.py index f391fa82..c464ff83 100644 --- a/tests/server/test_contacts_mcp.py +++ b/tests/server/test_contacts_mcp.py @@ -24,6 +24,9 @@ async def test_mcp_contacts_workflow( "fn": f"MCP Contact {unique_suffix}", "email": f"mcp.contact.{unique_suffix}@example.com", "tel": "1234567890", + # Regression for issue #716 — these were silently dropped before + "organization": "MCP Test Corp", + "note": f"Created by test {unique_suffix}", } try: @@ -51,9 +54,13 @@ async def test_mcp_contacts_workflow( ) assert create_c_result.isError is False - # 4. Verify contact creation + # 4. Verify contact creation (and that all fields — #716 — actually persisted) contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name) - assert any(c["vcard_id"] == contact_uid for c in contacts) + created = next((c for c in contacts if c["vcard_id"] == contact_uid), None) + assert created is not None + raw_vcard = created.get("addressdata", "") + assert "ORG:MCP Test Corp" in raw_vcard + assert f"NOTE:Created by test {unique_suffix}" in raw_vcard # 5. Delete contact via MCP logger.info(f"Deleting contact {contact_uid} via MCP") From 0186b494f14210947fc0a256170f28800688cc05 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 23 Apr 2026 07:44:05 +0200 Subject: [PATCH 2/7] test(contacts): address PR #719 review feedback - Type-annotate _wrap_contact_field signature; drop stale "url" mention from its docstring (url is handled by the list-coercion helper, not this one). - Split the shape-coercion helper so comma-splitting only applies to categories: _as_str_list (no split) for org/nickname/url, _split_categories (comma split) for CATEGORIES. Fixes the case where organization="Smith, Jones & Associates" was mangled into a two- component ORG. - Share _normalize_contact_data between create and update so _merge_vcard_properties only sees canonical keys; add URL handlers in both update branches so the primary update path no longer drops URL silently. - Annotate the Contact(**kwargs) type:ignore with the reason (pythonvCard4 typeshed doesn't accept **dict[str, Any]). - Add tests/unit/client/test_contacts.py (pure unit, no HTTP) covering the #716 round-trip, comma-in-org regression, invalid-bday warning, tel/phone precedence, categories string-vs-list behaviour, and direct _normalize_contact_data cases. - Extend the MCP workflow test with an update-with-url step asserting the URL handler in _merge_vcard_properties. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 117 ++++++++++++---- tests/server/test_contacts_mcp.py | 18 +++ tests/unit/client/test_contacts.py | 179 ++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 31 deletions(-) create mode 100644 tests/unit/client/test_contacts.py diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 256a8c16..d037afb5 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -11,7 +11,8 @@ from .base import BaseNextcloudClient logger = logging.getLogger(__name__) -# Keys that _build_contact_from_data consumes. Used to warn (not error) on unknown keys. +# Canonical keys that _build_contact_from_data consumes. Aliases (``phone``, ``organization``) +# are normalised to their canonical form by _normalize_contact_data before lookup. _SUPPORTED_CONTACT_KEYS = frozenset( { "fn", @@ -30,8 +31,27 @@ _SUPPORTED_CONTACT_KEYS = frozenset( ) -def _wrap_contact_field(value) -> list[dict]: - """Normalize an email/tel/url input into pythonvCard4's list-of-dicts shape. +def _normalize_contact_data(contact_data: dict) -> dict: + """Map documented aliases to canonical keys. + + ``phone`` → ``tel``, ``organization`` → ``org``. The canonical key wins if both + are supplied, so callers who set ``tel`` don't lose it to a stray ``phone`` entry. + Returns a new dict — does not mutate the caller's argument. + """ + normalised = dict(contact_data) + if "phone" in normalised and "tel" not in normalised: + normalised["tel"] = normalised.pop("phone") + else: + normalised.pop("phone", None) + if "organization" in normalised and "org" not in normalised: + normalised["org"] = normalised.pop("organization") + else: + normalised.pop("organization", None) + return normalised + + +def _wrap_contact_field(value: str | dict | list | None) -> list[dict]: + """Normalize an email/tel input into pythonvCard4's list-of-dicts shape. Accepts a plain string, a dict already in ``{value, type}`` form, or a list of either. Empty strings are dropped. Always returns a list (possibly empty). @@ -49,6 +69,29 @@ def _wrap_contact_field(value) -> list[dict]: return out +def _as_str_list(value: str | list) -> list[str]: + """Wrap a bare string in a list. Does NOT split on commas. + + Used for ORG/NICKNAME/URL where commas are part of the value (e.g. + ``"Smith, Jones & Associates"``) and only the list wrapper is needed to + prevent pythonvCard4 from iterating the string character-by-character. + """ + return value if isinstance(value, list) else [value] + + +def _split_categories(value: str | list) -> list[str]: + """Normalise CATEGORIES input: a comma-separated string is split into a list. + + Unlike ORG/NICKNAME, CATEGORIES is canonically comma-separated in vCards + (``CATEGORIES:a,b,c``) so splitting a bare string is the expected shape. + Lists pass through unchanged — callers that already provide ``["a,b"]`` keep + their exact item, no double-splitting. + """ + if isinstance(value, list): + return value + return [v.strip() for v in value.split(",") if v.strip()] + + def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict. @@ -56,47 +99,37 @@ def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char iteration of bare strings — see issue #716. """ + data = _normalize_contact_data(contact_data) - # pythonvCard4 iterates bare strings character-by-character for list-typed fields - # (ORG, NICKNAME, CATEGORIES, URL), producing garbage like ``ORG:A;c;m;e``. Wrap - # single strings in a list to keep the vCard well-formed. - def _as_list(value): - if isinstance(value, list): - return value - if isinstance(value, str) and "," in value: - return [v.strip() for v in value.split(",") if v.strip()] - return [value] + kwargs: dict = {"fn": data.get("fn"), "uid": uid} - kwargs: dict = {"fn": contact_data.get("fn"), "uid": uid} - - emails = _wrap_contact_field(contact_data.get("email")) + emails = _wrap_contact_field(data.get("email")) if emails: kwargs["email"] = emails - tels = _wrap_contact_field(contact_data.get("tel") or contact_data.get("phone")) + tels = _wrap_contact_field(data.get("tel")) if tels: kwargs["tel"] = tels - org_value = contact_data.get("org") or contact_data.get("organization") - if org_value: - kwargs["org"] = _as_list(org_value) + if data.get("org"): + kwargs["org"] = _as_str_list(data["org"]) - if contact_data.get("note"): - kwargs["note"] = contact_data["note"] + if data.get("note"): + kwargs["note"] = data["note"] - if contact_data.get("title"): - kwargs["title"] = contact_data["title"] + if data.get("title"): + kwargs["title"] = data["title"] - if contact_data.get("nickname"): - kwargs["nickname"] = _as_list(contact_data["nickname"]) + if data.get("nickname"): + kwargs["nickname"] = _as_str_list(data["nickname"]) - if contact_data.get("categories"): - kwargs["categories"] = _as_list(contact_data["categories"]) + if data.get("categories"): + kwargs["categories"] = _split_categories(data["categories"]) - if contact_data.get("url"): - kwargs["url"] = _as_list(contact_data["url"]) + if data.get("url"): + kwargs["url"] = _as_str_list(data["url"]) - bday = contact_data.get("bday") + bday = data.get("bday") if bday: if isinstance(bday, date): kwargs["bday"] = bday @@ -106,10 +139,12 @@ def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: except ValueError: logger.warning("Ignoring non-ISO bday value: %r", bday) - unknown = set(contact_data) - _SUPPORTED_CONTACT_KEYS + unknown = set(data) - _SUPPORTED_CONTACT_KEYS if unknown: logger.debug("Ignoring unknown contact_data keys: %s", sorted(unknown)) + # kwargs built dynamically from contact_data; pythonvCard4's Contact typeshed + # has specific typed params and doesn't accept **dict[str, Any]. return Contact(**kwargs) # type: ignore[arg-type] @@ -251,6 +286,9 @@ class ContactsClient(BaseNextcloudClient): carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{uid}.vcf" + # Canonicalise aliases up front so both code paths (merge + fallback) agree. + contact_data = _normalize_contact_data(contact_data) + # Get raw vCard content to preserve all properties including extended ones raw_vcard_content = "" if not etag: @@ -480,6 +518,17 @@ class ContactsClient(BaseNextcloudClient): elif property_name == "TITLE" and "title" in contact_data: updated_lines.append(f"TITLE:{contact_data['title']}") updated_properties.add("title") + elif property_name == "URL" and "url" in contact_data: + if "url" not in updated_properties: + url_value = contact_data["url"] + if isinstance(url_value, list): + url_value = url_value[0] if url_value else "" + if url_value: + updated_lines.append(f"URL:{url_value}") + updated_properties.add("url") + else: + # Keep additional URLs unchanged + updated_lines.append(line) else: # Keep all other properties unchanged (preserves all extended/custom fields) updated_lines.append(line) @@ -511,6 +560,12 @@ class ContactsClient(BaseNextcloudClient): updated_lines.append(f"ORG:{value}") elif key == "title": updated_lines.append(f"TITLE:{value}") + elif key == "url": + url_value = ( + value[0] if isinstance(value, list) and value else value + ) + if url_value: + updated_lines.append(f"URL:{url_value}") # Add the END:VCARD line updated_lines.append("END:VCARD") diff --git a/tests/server/test_contacts_mcp.py b/tests/server/test_contacts_mcp.py index c464ff83..8b5d587d 100644 --- a/tests/server/test_contacts_mcp.py +++ b/tests/server/test_contacts_mcp.py @@ -62,6 +62,24 @@ async def test_mcp_contacts_workflow( assert "ORG:MCP Test Corp" in raw_vcard assert f"NOTE:Created by test {unique_suffix}" in raw_vcard + # 4b. Update with a URL — regression guard for PR #719 review: + # _merge_vcard_properties previously had no URL handler, silently dropping it. + update_result = await nc_mcp_client.call_tool( + "nc_contacts_update_contact", + { + "addressbook": addressbook_name, + "uid": contact_uid, + "contact_data": {"url": "https://mcp-test.example.com"}, + }, + ) + assert update_result.isError is False + contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name) + updated = next(c for c in contacts if c["vcard_id"] == contact_uid) + updated_vcard = updated.get("addressdata", "") + assert "mcp-test.example.com" in updated_vcard + # Prior properties must not have been clobbered by the merge. + assert "ORG:MCP Test Corp" in updated_vcard + # 5. Delete contact via MCP logger.info(f"Deleting contact {contact_uid} via MCP") delete_c_result = await nc_mcp_client.call_tool( diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py new file mode 100644 index 00000000..b72756ca --- /dev/null +++ b/tests/unit/client/test_contacts.py @@ -0,0 +1,179 @@ +"""Unit tests for the contacts client vCard builder. + +These exercise ``_build_contact_from_data`` in isolation — no HTTP, no fixtures — +so they cover the issue #716 regression surface and the edge cases flagged in +PR #719 review without standing up the compose stack. +""" + +from datetime import date + +import pytest + +from nextcloud_mcp_server.client.contacts import ( + _build_contact_from_data, + _normalize_contact_data, +) + +pytestmark = pytest.mark.unit + + +def _vcard(**kwargs) -> str: + """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text.""" + return _build_contact_from_data(kwargs, uid="unit-test-uid").to_vcard() + + +def test_issue_716_minimal_payload_keeps_all_fields(): + """Reporter's exact payload from issue #716: every field must survive.""" + vcard = _vcard( + fn="Repro User", + email="repro@example.com", + phone="555-0716", + organization="Acme Corp", + note="Issue 716", + ) + assert "FN:Repro User" in vcard + assert "EMAIL" in vcard and "repro@example.com" in vcard + assert "TEL" in vcard and "555-0716" in vcard + assert "ORG:Acme Corp" in vcard + assert "NOTE:Issue 716" in vcard + + +def test_org_preserves_comma_in_company_name(): + """Regression: ``_as_list`` used to comma-split ORG, mangling names like + "Smith, Jones & Associates" into a two-component ORG. After the fix the whole + string is a single ORG component (with the comma RFC-6350-escaped as ``\\,``). + """ + vcard = _vcard(fn="Alice", organization="Smith, Jones & Associates") + org_line = next(line for line in vcard.splitlines() if line.startswith("ORG")) + # Single component: no unescaped semicolon separator. + payload = org_line.split(":", 1)[1] + assert ";" not in payload + # Comma is escaped per RFC 6350 but the logical value is preserved. + assert payload.replace(r"\,", ",") == "Smith, Jones & Associates" + + +def test_org_list_input_produces_structured_org(): + """A list input is the opt-in shape for multi-component ORG (Company;Department).""" + vcard = _vcard(fn="Alice", org=["Acme", "Engineering"]) + assert "ORG:Acme;Engineering" in vcard + + +def test_invalid_bday_is_dropped_not_raised(caplog): + """An unparseable BDAY must warn and be omitted, not crash the call.""" + import logging + + with caplog.at_level( + logging.WARNING, logger="nextcloud_mcp_server.client.contacts" + ): + vcard = _vcard(fn="Alice", bday="not-a-date") + assert "BDAY" not in vcard + assert any("bday" in r.message.lower() for r in caplog.records) + + +def test_valid_iso_bday_is_persisted(): + vcard = _vcard(fn="Alice", bday="1990-05-01") + assert "BDAY:1990-05-01" in vcard + + +def test_date_object_bday_is_persisted(): + vcard = _vcard(fn="Alice", bday=date(1985, 12, 24)) + assert "BDAY:1985-12-24" in vcard + + +def test_tel_takes_precedence_over_phone_alias(): + """When the caller supplies both canonical and alias, canonical wins. Documents + the precedence so future callers aren't surprised. + """ + vcard = _vcard(fn="Alice", tel="111-1111", phone="222-2222") + assert "111-1111" in vcard + assert "222-2222" not in vcard + + +def test_organization_alias_fills_in_when_org_absent(): + vcard = _vcard(fn="Alice", organization="Acme") + assert "ORG:Acme" in vcard + + +def test_categories_string_is_split_on_commas(): + vcard = _vcard(fn="Alice", categories="friends,work,vip") + cat_line = next( + line for line in vcard.splitlines() if line.startswith("CATEGORIES") + ) + assert cat_line == "CATEGORIES:friends,work,vip" + + +def test_categories_list_passes_through_unchanged(): + """A caller that already supplied a list shouldn't have their entries split again + — ``["friends,work"]`` stays as one item (with the comma RFC-6350-escaped), not + two categories ``friends`` + ``work``. + """ + vcard = _vcard(fn="Alice", categories=["friends,work"]) + cat_line = next( + line for line in vcard.splitlines() if line.startswith("CATEGORIES") + ) + payload = cat_line.split(":", 1)[1] + assert payload == r"friends\,work" # one item, comma escaped + + +def test_nickname_bare_string_is_not_char_iterated(): + """Regression: pythonvCard4 iterates bare strings; we wrap to prevent that.""" + vcard = _vcard(fn="Alice", nickname="Bob") + nick_line = next(line for line in vcard.splitlines() if line.startswith("NICKNAME")) + assert nick_line == "NICKNAME:Bob" + + +def test_url_bare_string_is_not_char_iterated(): + vcard = _vcard(fn="Alice", url="https://example.com") + # Must appear as a single URL, not one URL: per character. + url_lines = [line for line in vcard.splitlines() if line.startswith("URL")] + assert url_lines == ["URL:https://example.com"] + + +def test_unknown_keys_are_ignored_without_error(caplog): + """Future-compat: callers sending unknown keys shouldn't blow up.""" + import logging + + with caplog.at_level(logging.DEBUG, logger="nextcloud_mcp_server.client.contacts"): + vcard = _vcard(fn="Alice", totally_made_up_field="ignored") + assert "FN:Alice" in vcard + assert "totally_made_up_field" not in vcard + # A debug log is expected but not required — main guarantee is that no exception is raised. + + +def test_empty_email_is_skipped(): + """An empty string for email must not emit an EMAIL: line.""" + vcard = _vcard(fn="Alice", email="") + assert "EMAIL" not in vcard + + +def test_dict_form_email_preserves_custom_type(): + vcard = _vcard( + fn="Alice", + email={"value": "work@example.com", "type": ["WORK"]}, + ) + assert "EMAIL;TYPE=WORK:work@example.com" in vcard + + +class TestNormalizeContactData: + """Direct tests for the alias helper — it's load-bearing for update_contact too.""" + + def test_phone_maps_to_tel(self): + assert _normalize_contact_data({"phone": "123"}) == {"tel": "123"} + + def test_organization_maps_to_org(self): + assert _normalize_contact_data({"organization": "Acme"}) == {"org": "Acme"} + + def test_canonical_wins_when_both_present(self): + """Caller intent: they set ``tel`` deliberately. A stray ``phone`` entry + must not clobber the canonical value. + """ + out = _normalize_contact_data({"tel": "canonical", "phone": "alias"}) + assert out == {"tel": "canonical"} + + def test_does_not_mutate_input(self): + original = {"phone": "123", "organization": "Acme"} + _normalize_contact_data(original) + assert original == {"phone": "123", "organization": "Acme"} + + def test_passthrough_for_unknown_keys(self): + assert _normalize_contact_data({"foo": "bar"}) == {"foo": "bar"} From ee1465c05cb927eadb86a37bed4bc83eb364f966 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 23 Apr 2026 16:21:15 +0200 Subject: [PATCH 3/7] test(contacts): pin NICKNAME/BDAY/CATEGORIES update behaviour in _merge_vcard_properties PR #719 review raised a claim that these three fields fall through to the "keep unchanged" catch-all in _merge_vcard_properties. The existing elif branches for NICKNAME/BDAY/CATEGORIES already prevent that, but the behaviour wasn't pinned by a test. Add a focused TestMergeVcardProperties class that calls the merge helper directly and asserts: - Existing NICKNAME/BDAY/CATEGORIES lines are overwritten by new values. - When the existing vCard has none of these lines, update adds them. - A URL update doesn't clobber unrelated ORG/NOTE/TEL properties. If the primary update path ever regresses for these fields, these tests will catch it immediately. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/unit/client/test_contacts.py | 63 ++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index b72756ca..af9da75e 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -177,3 +177,66 @@ class TestNormalizeContactData: def test_passthrough_for_unknown_keys(self): assert _normalize_contact_data({"foo": "bar"}) == {"foo": "bar"} + + +class TestMergeVcardProperties: + """Direct tests for ``_merge_vcard_properties`` — the primary update path. + + Written in response to PR #719 review claiming NICKNAME/BDAY/CATEGORIES are not + updatable via this function. These tests pin the actual behaviour so future + regressions (or claims) can be answered in one line. + """ + + @staticmethod + def _merge(raw: str, data: dict) -> str: + from nextcloud_mcp_server.client.contacts import ContactsClient + + client = ContactsClient.__new__(ContactsClient) # no HTTP / no __init__ + return client._merge_vcard_properties(raw, data, uid="merge-test") + + def test_nickname_overwrites_existing_line(self): + """Existing NICKNAME must be replaced with the new value, not preserved.""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nNICKNAME:Bob\nEND:VCARD\n" + result = self._merge(existing, {"nickname": "Robert"}) + assert "NICKNAME:Robert" in result + assert "NICKNAME:Bob" not in result + + def test_bday_overwrites_existing_line(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nBDAY:1990-05-01\nEND:VCARD\n" + result = self._merge(existing, {"bday": "1991-06-02"}) + assert "BDAY:1991-06-02" in result + assert "BDAY:1990-05-01" not in result + + def test_categories_overwrites_existing_line(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nCATEGORIES:old,stale\nEND:VCARD\n" + result = self._merge(existing, {"categories": ["vip", "new"]}) + assert "CATEGORIES:vip,new" in result + assert "old,stale" not in result + + def test_nickname_added_when_not_in_existing_vcard(self): + """If the existing vCard has no NICKNAME line, update must append one.""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"nickname": "Bob"}) + assert "NICKNAME:Bob" in result + + def test_bday_added_when_not_in_existing_vcard(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"bday": "1990-05-01"}) + assert "BDAY:1990-05-01" in result + + def test_categories_added_when_not_in_existing_vcard(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"categories": "a,b,c"}) + assert "CATEGORIES:a,b,c" in result + + def test_url_update_preserves_unrelated_properties(self): + """A URL update must not clobber ORG / NOTE / TEL from the existing vCard.""" + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "ORG:Acme\nTEL:555-1234\nNOTE:keep me\nEND:VCARD\n" + ) + result = self._merge(existing, {"url": "https://example.com"}) + assert "URL:https://example.com" in result + assert "ORG:Acme" in result + assert "TEL:555-1234" in result + assert "NOTE:keep me" in result From e2283ff28c60e282c3bd778303f5cfae150307e7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 24 Apr 2026 09:44:03 +0200 Subject: [PATCH 4/7] refactor(contacts): address PR #719 follow-up review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pulls the remaining review feedback into one commit: - Remove the double _normalize_contact_data call: the helper now assumes canonical keys, and create_contact normalises before calling it (update_contact already did). Docstring states the invariant. - Drop phone/organization from _SUPPORTED_CONTACT_KEYS; they never reach the unknown-key check post-normalisation. - Tighten generics to dict[str, Any] / list[str] across helpers and ContactsClient signatures. - Comment both URL-merge sites noting only the first URL is written. - Log a warning when fn is missing from contact_data. - Test coverage for _wrap_contact_field dropping value-less dicts and for the fn-missing warning; _vcard helper now mirrors the real call chain (normalise → build). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 60 ++++++++++++++++++------- tests/unit/client/test_contacts.py | 37 ++++++++++++++- 2 files changed, 78 insertions(+), 19 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index d037afb5..4a0f3d34 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -3,6 +3,7 @@ import logging import xml.etree.ElementTree as ET from datetime import date +from typing import Any from pythonvCard4.vcard import Contact @@ -11,16 +12,15 @@ from .base import BaseNextcloudClient logger = logging.getLogger(__name__) -# Canonical keys that _build_contact_from_data consumes. Aliases (``phone``, ``organization``) -# are normalised to their canonical form by _normalize_contact_data before lookup. +# Canonical keys accepted by _build_contact_from_data. Callers normalise aliases +# (``phone``→``tel``, ``organization``→``org``) via _normalize_contact_data beforehand +# so the set never needs to list them. _SUPPORTED_CONTACT_KEYS = frozenset( { "fn", "email", "tel", - "phone", "org", - "organization", "note", "title", "nickname", @@ -31,7 +31,7 @@ _SUPPORTED_CONTACT_KEYS = frozenset( ) -def _normalize_contact_data(contact_data: dict) -> dict: +def _normalize_contact_data(contact_data: dict[str, Any]) -> dict[str, Any]: """Map documented aliases to canonical keys. ``phone`` → ``tel``, ``organization`` → ``org``. The canonical key wins if both @@ -50,16 +50,19 @@ def _normalize_contact_data(contact_data: dict) -> dict: return normalised -def _wrap_contact_field(value: str | dict | list | None) -> list[dict]: +def _wrap_contact_field( + value: str | dict[str, Any] | list[str | dict[str, Any]] | None, +) -> list[dict[str, Any]]: """Normalize an email/tel input into pythonvCard4's list-of-dicts shape. Accepts a plain string, a dict already in ``{value, type}`` form, or a list of - either. Empty strings are dropped. Always returns a list (possibly empty). + either. Empty strings and dicts without a ``value`` key are dropped. Always + returns a list (possibly empty). """ if value is None or value == "": return [] items = value if isinstance(value, list) else [value] - out: list[dict] = [] + out: list[dict[str, Any]] = [] for item in items: if isinstance(item, dict) and item.get("value"): types = item.get("type") or ["HOME"] @@ -69,7 +72,7 @@ def _wrap_contact_field(value: str | dict | list | None) -> list[dict]: return out -def _as_str_list(value: str | list) -> list[str]: +def _as_str_list(value: str | list[str]) -> list[str]: """Wrap a bare string in a list. Does NOT split on commas. Used for ORG/NICKNAME/URL where commas are part of the value (e.g. @@ -79,7 +82,7 @@ def _as_str_list(value: str | list) -> list[str]: return value if isinstance(value, list) else [value] -def _split_categories(value: str | list) -> list[str]: +def _split_categories(value: str | list[str]) -> list[str]: """Normalise CATEGORIES input: a comma-separated string is split into a list. Unlike ORG/NICKNAME, CATEGORIES is canonically comma-separated in vCards @@ -92,16 +95,25 @@ def _split_categories(value: str | list) -> list[str]: return [v.strip() for v in value.split(",") if v.strip()] -def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: +def _build_contact_from_data(contact_data: dict[str, Any], uid: str) -> Contact: """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict. Maps every key documented on ``nc_contacts_create_contact`` onto the underlying library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char iteration of bare strings — see issue #716. - """ - data = _normalize_contact_data(contact_data) - kwargs: dict = {"fn": data.get("fn"), "uid": uid} + Callers must pre-normalise aliases via ``_normalize_contact_data`` before + invoking this helper; it assumes canonical keys only. + """ + data = contact_data + + if not data.get("fn"): + logger.warning( + "contact_data missing required 'fn' field; pythonvCard4 may reject or " + "produce an invalid vCard" + ) + + kwargs: dict[str, Any] = {"fn": data.get("fn"), "uid": uid} emails = _wrap_contact_field(data.get("email")) if emails: @@ -259,11 +271,15 @@ class ContactsClient(BaseNextcloudClient): url = f"{carddav_path}/{name}/" await self._make_request("DELETE", url) - async def create_contact(self, *, addressbook: str, uid: str, contact_data: dict): + async def create_contact( + self, *, addressbook: str, uid: str, contact_data: dict[str, Any] + ): """Create a new contact.""" carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{uid}.vcf" + # Normalise aliases here so the helper's invariant (canonical keys only) holds. + contact_data = _normalize_contact_data(contact_data) vcard = _build_contact_from_data(contact_data, uid).to_vcard() headers = { @@ -280,7 +296,12 @@ class ContactsClient(BaseNextcloudClient): await self._make_request("DELETE", url) async def update_contact( - self, *, addressbook: str, uid: str, contact_data: dict, etag: str = "" + self, + *, + addressbook: str, + uid: str, + contact_data: dict[str, Any], + etag: str = "", ): """Update an existing contact while preserving all existing properties.""" carddav_path = self._get_carddav_base_path() @@ -429,7 +450,7 @@ class ContactsClient(BaseNextcloudClient): raise def _merge_vcard_properties( - self, raw_vcard: str, contact_data: dict, uid: str + self, raw_vcard: str, contact_data: dict[str, Any], uid: str ) -> str: """Merge new contact data into existing raw vCard while preserving all properties.""" try: @@ -521,6 +542,9 @@ class ContactsClient(BaseNextcloudClient): elif property_name == "URL" and "url" in contact_data: if "url" not in updated_properties: url_value = contact_data["url"] + # Only the first URL from a list is written; multi-URL + # contacts are rare and this text merge doesn't attempt + # position-stable mapping to existing URL lines. if isinstance(url_value, list): url_value = url_value[0] if url_value else "" if url_value: @@ -561,6 +585,8 @@ class ContactsClient(BaseNextcloudClient): elif key == "title": updated_lines.append(f"TITLE:{value}") elif key == "url": + # Only the first URL is written on add-new; see note in the + # update-existing branch above. url_value = ( value[0] if isinstance(value, list) and value else value ) diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index af9da75e..8712cff8 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -12,14 +12,20 @@ import pytest from nextcloud_mcp_server.client.contacts import ( _build_contact_from_data, _normalize_contact_data, + _wrap_contact_field, ) pytestmark = pytest.mark.unit def _vcard(**kwargs) -> str: - """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text.""" - return _build_contact_from_data(kwargs, uid="unit-test-uid").to_vcard() + """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text. + + Mirrors ``create_contact``'s real call chain: normalise aliases first, then hand + canonical keys to ``_build_contact_from_data``. + """ + data = _normalize_contact_data(kwargs) + return _build_contact_from_data(data, uid="unit-test-uid").to_vcard() def test_issue_716_minimal_payload_keeps_all_fields(): @@ -154,6 +160,33 @@ def test_dict_form_email_preserves_custom_type(): assert "EMAIL;TYPE=WORK:work@example.com" in vcard +def test_wrap_field_dict_without_value_is_dropped(): + """Dict inputs lacking the ``value`` key are silently dropped so malformed + payloads don't emit an EMAIL/TEL line pointing at nothing. + """ + assert _wrap_contact_field({"type": ["WORK"]}) == [] + # Mixed list: the valid entry survives, the value-less dict is omitted. + out = _wrap_contact_field( + [{"value": "ok@example.com", "type": ["WORK"]}, {"type": ["HOME"]}] + ) + assert out == [{"value": "ok@example.com", "type": ["WORK"]}] + + +def test_missing_fn_logs_warning(caplog): + """A missing ``fn`` should log a warning so operators notice malformed payloads.""" + import logging + + with caplog.at_level( + logging.WARNING, logger="nextcloud_mcp_server.client.contacts" + ): + try: + _build_contact_from_data({"email": "x@example.com"}, uid="no-fn-uid") + except Exception: + # pythonvCard4 may raise on missing fn; we only care about the warning log. + pass + assert any("fn" in r.message.lower() for r in caplog.records) + + class TestNormalizeContactData: """Direct tests for the alias helper — it's load-bearing for update_contact too.""" From e1c776716c5623d102cfc7c3f6a047f3efef426d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 26 Apr 2026 02:24:16 +0200 Subject: [PATCH 5/7] fix(contacts): close PR #719 second-pass review gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _merge_vcard_properties no longer silently drops the existing EMAIL / TEL line when contact_data supplies a dict/list shape: the input is unhandled by the text merge, so the original line is preserved instead of being consumed and replaced with nothing. - Extracted _parse_bday so the update path validates ISO format the same way create does. Invalid → keep existing BDAY line (or skip on add-new) rather than writing a malformed one. - Added _safe_vcard_value to escape newlines per RFC 6350 §3.4 at every interpolation site in _merge_vcard_properties, blocking value-driven property injection (e.g. NOTE: containing a literal \n + EMAIL:). - Removed dead "organization" alias references from _merge_vcard_properties: unreachable since update_contact normalises before calling. - New regression tests pin all four behaviours (dict-email preserves existing line, list-tel ditto, invalid-bday-update preserves original, invalid-bday-add-new is dropped, newline-in-note no injection). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 132 +++++++++++++++++------- tests/unit/client/test_contacts.py | 59 +++++++++++ 2 files changed, 151 insertions(+), 40 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 4a0f3d34..d309d98f 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -95,6 +95,37 @@ def _split_categories(value: str | list[str]) -> list[str]: return [v.strip() for v in value.split(",") if v.strip()] +def _parse_bday(value: str | date | None) -> date | None: + """Parse a BDAY input to a ``date``. Logs and returns ``None`` if unparseable. + + Shared by the create path (``_build_contact_from_data``) and the update path + (``_merge_vcard_properties``) so a non-ISO BDAY is rejected consistently + instead of being written raw on update. + """ + if value is None or value == "": + return None + if isinstance(value, date): + return value + if isinstance(value, str): + try: + return date.fromisoformat(value) + except ValueError: + logger.warning("Ignoring non-ISO bday value: %r", value) + return None + + +def _safe_vcard_value(value: Any) -> Any: + """Escape newlines in a value so it can't inject additional vCard properties. + + Per RFC 6350 §3.4 newlines inside a property value are encoded as ``\\n``. + Unfolding this on the read side is pythonvCard4's job; we only need to make + sure ``contact_data`` strings don't terminate the line on the way out. + """ + if isinstance(value, str): + return value.replace("\r\n", "\\n").replace("\n", "\\n").replace("\r", "\\n") + return value + + def _build_contact_from_data(contact_data: dict[str, Any], uid: str) -> Contact: """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict. @@ -141,15 +172,9 @@ def _build_contact_from_data(contact_data: dict[str, Any], uid: str) -> Contact: if data.get("url"): kwargs["url"] = _as_str_list(data["url"]) - bday = data.get("bday") - if bday: - if isinstance(bday, date): - kwargs["bday"] = bday - elif isinstance(bday, str): - try: - kwargs["bday"] = date.fromisoformat(bday) - except ValueError: - logger.warning("Ignoring non-ISO bday value: %r", bday) + bday = _parse_bday(data.get("bday")) + if bday is not None: + kwargs["bday"] = bday unknown = set(data) - _SUPPORTED_CONTACT_KEYS if unknown: @@ -477,21 +502,27 @@ class ContactsClient(BaseNextcloudClient): # Handle updates for specific properties if property_name == "FN" and "fn" in contact_data: - updated_lines.append(f"FN:{contact_data['fn']}") + updated_lines.append(f"FN:{_safe_vcard_value(contact_data['fn'])}") updated_properties.add("fn") elif property_name == "EMAIL" and "email" in contact_data: # Replace first email with new one, preserve others if "email" not in updated_properties: if isinstance(contact_data["email"], str): + email_value = _safe_vcard_value(contact_data["email"]) # Try to preserve the original format as much as possible if ";TYPE=" in line: type_part = line.split(";TYPE=")[1].split(":")[0] updated_lines.append( - f"EMAIL;TYPE={type_part}:{contact_data['email']}" + f"EMAIL;TYPE={type_part}:{email_value}" ) else: - updated_lines.append(f"EMAIL:{contact_data['email']}") - updated_properties.add("email") + updated_lines.append(f"EMAIL:{email_value}") + updated_properties.add("email") + else: + # Dict / list inputs aren't translatable to a single + # text-merge replacement; keep the original line so we + # don't silently drop the contact's email. + updated_lines.append(line) else: # Keep additional emails unchanged updated_lines.append(line) @@ -499,45 +530,60 @@ class ContactsClient(BaseNextcloudClient): # Similar handling for phone numbers if "tel" not in updated_properties: if isinstance(contact_data["tel"], str): + tel_value = _safe_vcard_value(contact_data["tel"]) if ";TYPE=" in line: type_part = line.split(";TYPE=")[1].split(":")[0] updated_lines.append( - f"TEL;TYPE={type_part}:{contact_data['tel']}" + f"TEL;TYPE={type_part}:{tel_value}" ) else: - updated_lines.append(f"TEL:{contact_data['tel']}") - updated_properties.add("tel") + updated_lines.append(f"TEL:{tel_value}") + updated_properties.add("tel") + else: + # Same reasoning as the EMAIL branch above: don't drop. + updated_lines.append(line) else: # Keep additional phone numbers unchanged updated_lines.append(line) elif property_name == "NOTE" and "note" in contact_data: - updated_lines.append(f"NOTE:{contact_data['note']}") + updated_lines.append( + f"NOTE:{_safe_vcard_value(contact_data['note'])}" + ) updated_properties.add("note") elif property_name == "NICKNAME" and "nickname" in contact_data: nickname_value = contact_data["nickname"] if isinstance(nickname_value, list): nickname_value = ",".join(nickname_value) - updated_lines.append(f"NICKNAME:{nickname_value}") + updated_lines.append( + f"NICKNAME:{_safe_vcard_value(nickname_value)}" + ) updated_properties.add("nickname") elif property_name == "BDAY" and "bday" in contact_data: - updated_lines.append(f"BDAY:{contact_data['bday']}") - updated_properties.add("bday") + parsed_bday = _parse_bday(contact_data["bday"]) + if parsed_bday is not None: + updated_lines.append(f"BDAY:{parsed_bday.isoformat()}") + updated_properties.add("bday") + else: + # Invalid input — keep the existing BDAY rather than + # writing a malformed line or silently dropping it. + updated_lines.append(line) elif property_name == "CATEGORIES" and "categories" in contact_data: categories_value = contact_data["categories"] if isinstance(categories_value, list): categories_value = ",".join(categories_value) - updated_lines.append(f"CATEGORIES:{categories_value}") + updated_lines.append( + f"CATEGORIES:{_safe_vcard_value(categories_value)}" + ) updated_properties.add("categories") - elif property_name == "ORG" and ( - "org" in contact_data or "organization" in contact_data - ): - org_value = contact_data.get("org") or contact_data.get( - "organization" + elif property_name == "ORG" and "org" in contact_data: + updated_lines.append( + f"ORG:{_safe_vcard_value(contact_data['org'])}" ) - updated_lines.append(f"ORG:{org_value}") updated_properties.add("org") elif property_name == "TITLE" and "title" in contact_data: - updated_lines.append(f"TITLE:{contact_data['title']}") + updated_lines.append( + f"TITLE:{_safe_vcard_value(contact_data['title'])}" + ) updated_properties.add("title") elif property_name == "URL" and "url" in contact_data: if "url" not in updated_properties: @@ -548,7 +594,7 @@ class ContactsClient(BaseNextcloudClient): if isinstance(url_value, list): url_value = url_value[0] if url_value else "" if url_value: - updated_lines.append(f"URL:{url_value}") + updated_lines.append(f"URL:{_safe_vcard_value(url_value)}") updated_properties.add("url") else: # Keep additional URLs unchanged @@ -561,29 +607,35 @@ class ContactsClient(BaseNextcloudClient): for key, value in contact_data.items(): if key not in updated_properties: if key == "fn": - updated_lines.append(f"FN:{value}") + updated_lines.append(f"FN:{_safe_vcard_value(value)}") elif key == "email" and isinstance(value, str): - updated_lines.append(f"EMAIL:{value}") + updated_lines.append(f"EMAIL:{_safe_vcard_value(value)}") elif key == "tel" and isinstance(value, str): - updated_lines.append(f"TEL:{value}") + updated_lines.append(f"TEL:{_safe_vcard_value(value)}") elif key == "note": - updated_lines.append(f"NOTE:{value}") + updated_lines.append(f"NOTE:{_safe_vcard_value(value)}") elif key == "nickname": nickname_value = ( value if isinstance(value, str) else ",".join(value) ) - updated_lines.append(f"NICKNAME:{nickname_value}") + updated_lines.append( + f"NICKNAME:{_safe_vcard_value(nickname_value)}" + ) elif key == "bday": - updated_lines.append(f"BDAY:{value}") + parsed_bday = _parse_bday(value) + if parsed_bday is not None: + updated_lines.append(f"BDAY:{parsed_bday.isoformat()}") elif key == "categories": categories_value = ( value if isinstance(value, str) else ",".join(value) ) - updated_lines.append(f"CATEGORIES:{categories_value}") - elif key in ["org", "organization"]: - updated_lines.append(f"ORG:{value}") + updated_lines.append( + f"CATEGORIES:{_safe_vcard_value(categories_value)}" + ) + elif key == "org": + updated_lines.append(f"ORG:{_safe_vcard_value(value)}") elif key == "title": - updated_lines.append(f"TITLE:{value}") + updated_lines.append(f"TITLE:{_safe_vcard_value(value)}") elif key == "url": # Only the first URL is written on add-new; see note in the # update-existing branch above. @@ -591,7 +643,7 @@ class ContactsClient(BaseNextcloudClient): value[0] if isinstance(value, list) and value else value ) if url_value: - updated_lines.append(f"URL:{url_value}") + updated_lines.append(f"URL:{_safe_vcard_value(url_value)}") # Add the END:VCARD line updated_lines.append("END:VCARD") diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index 8712cff8..8616c617 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -273,3 +273,62 @@ class TestMergeVcardProperties: assert "ORG:Acme" in result assert "TEL:555-1234" in result assert "NOTE:keep me" in result + + def test_dict_email_input_preserves_existing_line(self): + """Regression: a dict-form email on update used to consume the existing + EMAIL: line and write nothing, silently deleting the contact's email. + Now the original line is preserved when the input shape isn't a plain str. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "EMAIL;TYPE=HOME:alice@example.com\nEND:VCARD\n" + ) + result = self._merge( + existing, {"email": {"value": "work@example.com", "type": ["WORK"]}} + ) + assert "EMAIL;TYPE=HOME:alice@example.com" in result + + def test_list_tel_input_preserves_existing_line(self): + """Same regression as the email branch but for TEL — a list-shaped tel + input must not silently drop the existing phone number. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "TEL;TYPE=HOME:555-0001\nEND:VCARD\n" + ) + result = self._merge( + existing, {"tel": [{"value": "555-9999", "type": ["WORK"]}]} + ) + assert "TEL;TYPE=HOME:555-0001" in result + + def test_invalid_bday_on_update_preserves_existing_line(self): + """A non-ISO BDAY string must not produce a malformed vCard line on update. + We share validation with the create path; invalid → keep the existing line. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "BDAY:1990-05-01\nEND:VCARD\n" + ) + result = self._merge(existing, {"bday": "not-a-date"}) + assert "BDAY:1990-05-01" in result + assert "BDAY:not-a-date" not in result + + def test_invalid_bday_on_add_new_is_dropped(self): + """No existing BDAY + invalid input → no BDAY line appended (vs. raw write).""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"bday": "not-a-date"}) + assert "BDAY" not in result + + def test_newline_in_note_does_not_inject_property(self): + """Regression: a literal newline in a value must not terminate the line + and inject a fresh vCard property. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge( + existing, {"note": "harmless\nEMAIL:attacker@evil.example"} + ) + # The injected property must not appear as a real EMAIL line. + lines = result.splitlines() + assert "EMAIL:attacker@evil.example" not in lines + # The note value is preserved with newlines escaped per RFC 6350. + assert any(line.startswith("NOTE:") and "\\n" in line for line in lines) From 125ab401216e56c33bf079b69dd442fde9dcb976 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 26 Apr 2026 03:24:15 +0200 Subject: [PATCH 6/7] fix(contacts): two PR #719 review bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _merge_vcard_properties: list-form ORG was passed through _safe_vcard_value unchanged, emitting a Python repr on the wire. Both branches now ;-join list components per RFC 6350 §6.6.4 (ORG is Company;Department;…) before interpolation. - _wrap_contact_field: a dict whose ``type`` was a bare string used to hit ``list("WORK")`` and explode into ``["W","O","R","K"]``. Wrap bare-string types into a single-element list before the list() call. Regression tests pin both shapes: - list-org overwrites and add-new produce ``ORG:Acme;Engineering`` - dict email with ``type="WORK"`` (bare str) emits ``EMAIL;TYPE=WORK:``, not ``EMAIL;TYPE=W,O,R,K:``. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 21 ++++++++++++--- tests/unit/client/test_contacts.py | 36 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index d309d98f..527af8df 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -66,6 +66,11 @@ def _wrap_contact_field( for item in items: if isinstance(item, dict) and item.get("value"): types = item.get("type") or ["HOME"] + # Wrap a bare string so ``list("WORK")`` doesn't iterate it into + # ``["W", "O", "R", "K"]`` — same char-iteration footgun this whole + # helper exists to avoid for the outer ``value``. + if isinstance(types, str): + types = [types] out.append({"value": item["value"], "type": list(types)}) elif isinstance(item, str) and item: out.append({"value": item, "type": ["HOME"]}) @@ -576,9 +581,13 @@ class ContactsClient(BaseNextcloudClient): ) updated_properties.add("categories") elif property_name == "ORG" and "org" in contact_data: - updated_lines.append( - f"ORG:{_safe_vcard_value(contact_data['org'])}" - ) + org_value = contact_data["org"] + # ORG is structured (Company;Department;…) per RFC 6350 §6.6.4; + # join list components with ';' so callers using the same shape + # ``_build_contact_from_data`` accepts don't get a Python repr. + if isinstance(org_value, list): + org_value = ";".join(org_value) + updated_lines.append(f"ORG:{_safe_vcard_value(org_value)}") updated_properties.add("org") elif property_name == "TITLE" and "title" in contact_data: updated_lines.append( @@ -633,7 +642,11 @@ class ContactsClient(BaseNextcloudClient): f"CATEGORIES:{_safe_vcard_value(categories_value)}" ) elif key == "org": - updated_lines.append(f"ORG:{_safe_vcard_value(value)}") + # See ORG note in update-existing branch above. + org_value = ( + ";".join(value) if isinstance(value, list) else value + ) + updated_lines.append(f"ORG:{_safe_vcard_value(org_value)}") elif key == "title": updated_lines.append(f"TITLE:{_safe_vcard_value(value)}") elif key == "url": diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index 8616c617..5784139c 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -160,6 +160,20 @@ def test_dict_form_email_preserves_custom_type(): assert "EMAIL;TYPE=WORK:work@example.com" in vcard +def test_dict_form_email_with_bare_string_type(): + """Regression: a dict with ``type="WORK"`` (bare string) used to be + char-iterated into ``["W","O","R","K"]`` because of an unguarded ``list()`` + call inside ``_wrap_contact_field``. + """ + vcard = _vcard( + fn="Alice", + email={"value": "work@example.com", "type": "WORK"}, + ) + assert "EMAIL;TYPE=WORK:work@example.com" in vcard + # The bug would emit something like ``EMAIL;TYPE=W,O,R,K:`` — guard against it. + assert "TYPE=W," not in vcard + + def test_wrap_field_dict_without_value_is_dropped(): """Dict inputs lacking the ``value`` key are silently dropped so malformed payloads don't emit an EMAIL/TEL line pointing at nothing. @@ -332,3 +346,25 @@ class TestMergeVcardProperties: assert "EMAIL:attacker@evil.example" not in lines # The note value is preserved with newlines escaped per RFC 6350. assert any(line.startswith("NOTE:") and "\\n" in line for line in lines) + + def test_list_org_overwrites_with_semicolon_join(self): + """Regression: list-form ORG used to fall through ``_safe_vcard_value`` + unchanged and emit a Python ``repr`` like ``ORG:['Acme', 'Engineering']``. + Per RFC 6350 §6.6.4 components are ``;``-separated. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nORG:OldCo\nEND:VCARD\n" + ) + result = self._merge(existing, {"org": ["Acme", "Engineering"]}) + assert "ORG:Acme;Engineering" in result + assert "ORG:OldCo" not in result + assert "[" not in result and "'Acme'" not in result + + def test_list_org_added_with_semicolon_join(self): + """Add-new branch: list ORG without an existing line still serialises + with ``;`` rather than as a Python list repr. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"org": ["Acme", "Engineering"]}) + assert "ORG:Acme;Engineering" in result + assert "[" not in result From c91be453742d8c323943dda0c315bd78d967eb33 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 26 Apr 2026 16:03:29 +0200 Subject: [PATCH 7/7] fix(contacts): warn on unsupported dict/list email/tel update inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surface the text-merge update path's limitation rather than silently no-op: when contact_data['email'] or ['tel'] arrives as a dict/list on update, log a warning at the top of _merge_vcard_properties pointing callers at plain str or create_contact. Existing EMAIL/TEL lines are still preserved unchanged. Bring nc_contacts_update_contact docstring into parity with create — the update tool now documents the same keys plus the explicit single-string limitation for email/tel and the BDAY validation / URL first-only behaviours. Three new TestMergeVcardProperties cases pin the warning: dict email warns, list tel warns, plain str email is silent (no false positives). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 24 ++++++++++++++- nextcloud_mcp_server/server/contacts.py | 25 ++++++++++++++- tests/unit/client/test_contacts.py | 41 +++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 527af8df..764fd695 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -482,7 +482,29 @@ class ContactsClient(BaseNextcloudClient): def _merge_vcard_properties( self, raw_vcard: str, contact_data: dict[str, Any], uid: str ) -> str: - """Merge new contact data into existing raw vCard while preserving all properties.""" + """Merge new contact data into existing raw vCard while preserving all properties. + + Limitation: dict / list-form ``email`` and ``tel`` inputs are not applied + by this text-merge path. Existing EMAIL/TEL lines are preserved unchanged, + and no new lines are written for the dict/list inputs. Pass plain strings + to update EMAIL/TEL here, or recreate via ``create_contact`` for full + multi-entry support with TYPE annotations. + """ + # Surface dict/list email/tel up front rather than silently no-op in the + # add-new loop below (where the isinstance(value, str) guard skips them). + for _key in ("email", "tel"): + _value = contact_data.get(_key) + if _value is not None and not isinstance(_value, str): + logger.warning( + "update_contact: %s=%r (dict/list shape) is not applied via " + "the text-merge update path; existing %s lines are preserved " + "unchanged. Use a plain string to update %s here, or recreate " + "via create_contact for multi-entry support.", + _key, + _value, + _key.upper(), + _key.upper(), + ) try: # Instead of using pythonvCard4 which has formatting issues, # let's do a simple text-based merge to preserve exact formatting diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 90935b05..587aa736 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -250,7 +250,30 @@ def configure_contacts_tools(mcp: FastMCP): not the display name. Use nc_contacts_list_addressbooks to find available URI slugs. uid: The unique ID of the contact to update. - contact_data: A dictionary with the contact's updated details, e.g. {"fn": "Jane Doe", "email": "jane.doe@example.com"}. + contact_data: A dictionary with the contact's updated details. Supported + keys mirror nc_contacts_create_contact: + + - ``fn`` (str): Formatted full name. + - ``email`` (str): Email address. **Update path supports plain + strings only**; dict / list-form inputs are not applied — the + existing EMAIL line is preserved unchanged and a warning is + logged. Use create_contact for multi-entry support with TYPE + annotations. + - ``tel`` / ``phone`` (str): Phone number. Same single-string + limitation as ``email`` above. + - ``org`` / ``organization`` (str or list of str): Organization. + Lists become semicolon-separated ORG components per RFC 6350. + - ``title`` (str): Job title. + - ``note`` (str): Free-form note. + - ``nickname`` (str or list of str). + - ``bday`` (ISO date str ``"YYYY-MM-DD"`` or ``datetime.date``). + Non-ISO strings are rejected with a warning; the existing + BDAY line is preserved. + - ``categories`` (list of str, or comma-separated str). + - ``url`` (str or list of str). Only the first URL is written + on update; multi-URL contacts should use create_contact. + + Example: ``{"fn": "Jane Doe", "email": "jane.doe@example.com"}``. etag: Optional ETag for optimistic concurrency control. """ client = await get_client(ctx) diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index 5784139c..a5deebb0 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -368,3 +368,44 @@ class TestMergeVcardProperties: result = self._merge(existing, {"org": ["Acme", "Engineering"]}) assert "ORG:Acme;Engineering" in result assert "[" not in result + + def test_dict_email_on_no_existing_line_warns(self, caplog): + """No existing EMAIL + dict input is a known limitation of the text-merge + path. Surface it as a warning so the silent no-op is at least observable. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + with caplog.at_level("WARNING"): + result = self._merge( + existing, {"email": {"value": "alice@work.com", "type": ["WORK"]}} + ) + # The dict input is not applied; no EMAIL line is added. + assert "EMAIL" not in result + # A warning specifically calls out the dict/list shape and recommends + # plain str / create_contact as alternatives. + assert any( + "email" in r.message and "dict/list shape" in r.message + for r in caplog.records + ) + + def test_list_tel_on_no_existing_line_warns(self, caplog): + """Same warning behaviour for TEL — list input on a contact without an + existing TEL line must not silently disappear. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + with caplog.at_level("WARNING"): + result = self._merge( + existing, {"tel": [{"value": "555-9999", "type": ["WORK"]}]} + ) + assert "TEL" not in result + assert any( + "tel" in r.message and "dict/list shape" in r.message + for r in caplog.records + ) + + def test_str_email_does_not_warn(self, caplog): + """Plain string email is the supported shape — no warning should fire.""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + with caplog.at_level("WARNING"): + result = self._merge(existing, {"email": "alice@work.com"}) + assert "EMAIL:alice@work.com" in result + assert not any("dict/list shape" in r.message for r in caplog.records)