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