fix(contacts): close PR #719 second-pass review gaps
- _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) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
e2283ff28c
commit
e1c776716c
@@ -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")
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user