fix(contacts): two PR #719 review bugs
- _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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
e1c776716c
commit
125ab40121
@@ -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":
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user