harden(mail): address PR #935 round-4 review

No blockers raised; hardening + clarity:
- client/mail.py: URL-encode the caller-supplied attachment_id
  (quote(..., safe="")) — defense-in-depth against path traversal.
- server/mail.py: measure attachment content in UTF-8 bytes (not characters)
  for the size cap and the sentinel message.
- scanner.py: bound _mail_cap_logged (insertion-ordered dict + oldest-first
  eviction at 50k, mirroring _consent_backstop_done) so the cap-log dedup set
  can't leak in a long-running multi-tenant process; reword the cap log to not
  imply MAIL_SCAN_MAX_PER_MAILBOX is operator-tunable (it's the Mail OCS max).
- models/mail.py: comment why GetAttachmentResponse doesn't nest MailAttachment
  (different OCS endpoint shape).
- mail_content.py: document format_mail_addresses' empty-entry skip contract.

_potentially_deleted doc_type-in-key remains tracked as Deck #376 (pre-existing
cross-cutting; reviewer confirmed deferral).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/client/mail.py

@@ -14,6 +14,7 @@ OCS API controllers (Mail 5.x / Nextcloud 32+).
 
 import logging
 from typing import Any
+from urllib.parse import quote
 
 from httpx import HTTPStatusError, RequestError, Response
 
@@ -188,5 +189,10 @@ class MailClient(BaseNextcloudClient):
         Returns:
             Attachment object with name, mime, size, content.
         """
-        data = await self._ocs_get(f"/message/{message_id}/attachment/{attachment_id}")
+        # URL-encode the caller-supplied attachment id (defense-in-depth: keeps
+        # a value like "../.." from being normalised into a different path).
+        safe_attachment_id = quote(attachment_id, safe="")
+        data = await self._ocs_get(
+            f"/message/{message_id}/attachment/{safe_attachment_id}"
+        )
         return data or {}

nextcloud_mcp_server/models/mail.py

@@ -173,7 +173,13 @@ class GetMessageResponse(BaseResponse):
 
 
 class GetAttachmentResponse(BaseResponse):
-    """Response model for getting a single attachment."""
+    """Response model for getting a single attachment.
+
+    Intentionally does NOT nest ``MailAttachment``: the Mail OCS *get-attachment*
+    endpoint returns a different shape (``name``/``mime``/``size``/``content``)
+    than the attachment entries on a message listing, which ``MailAttachment``
+    models (``id``/``fileName``/``cid``/``disposition``, and no ``content``).
+    """
 
     name: str | None = Field(None, description="Attachment file name")
     mime: str | None = Field(None, description="MIME type")

nextcloud_mcp_server/server/mail.py

@@ -222,11 +222,13 @@ def configure_mail_tools(mcp: FastMCP):
         try:
             data = await client.mail.get_attachment(message_id, attachment_id)
             content = data.get("content")
-            if isinstance(content, str) and len(content) > MAX_ATTACHMENT_CONTENT_BYTES:
-                content = (
-                    f"[attachment too large to inline: {len(content)} bytes "
-                    f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]"
-                )
+            if isinstance(content, str):
+                content_bytes = len(content.encode("utf-8"))
+                if content_bytes > MAX_ATTACHMENT_CONTENT_BYTES:
+                    content = (
+                        f"[attachment too large to inline: {content_bytes} bytes "
+                        f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]"
+                    )
             return GetAttachmentResponse(
                 name=data.get("name"),
                 mime=data.get("mime"),

nextcloud_mcp_server/vector/mail_content.py

@@ -19,7 +19,13 @@ MAIL_SCAN_MAX_PER_MAILBOX = 100
 
 
 def format_mail_addresses(addrs: list[dict[str, Any]] | None) -> str:
-    """Render a list of {label, email} address objects as a display string."""
+    """Render a list of {label, email} address objects as a display string.
+
+    ``None`` and address objects with neither ``label`` nor ``email`` are
+    skipped (yielding ``""`` for an all-empty list) — IMAP envelope addresses
+    effectively always carry at least an email, so this only drops malformed
+    entries rather than losing real recipients.
+    """
     parts: list[str] = []
     for addr in addrs or []:
         label = addr.get("label")

nextcloud_mcp_server/vector/scanner.py

@@ -1366,8 +1366,26 @@ async def scan_news_items(
 # Per-process record of (user_id, mailbox_id) for which the newest-N cap has
 # already been logged, so the "older mail not indexed" notice is emitted once at
 # info level (discoverable) rather than on every scan tick (which would flood
-# multi-tenant logs).
-_mail_cap_logged: set[tuple[str, int]] = set()
+# multi-tenant logs). Insertion-ordered dict + bounded eviction (mirrors
+# _consent_backstop_done) so a long-running multi-tenant process can't leak it.
+_mail_cap_logged: dict[tuple[str, int], None] = {}
+_MAIL_CAP_LOGGED_MAX = 50_000
+
+
+def _mark_mail_cap_logged(key: tuple[str, int]) -> bool:
+    """Record a one-shot cap-log marker; return True if this is the first time.
+
+    Evicts oldest-first to half capacity on overflow (a re-log after eviction is
+    a harmless info line), so the dedup set stays bounded.
+    """
+    if key in _mail_cap_logged:
+        return False
+    if len(_mail_cap_logged) >= _MAIL_CAP_LOGGED_MAX:
+        overage = len(_mail_cap_logged) - _MAIL_CAP_LOGGED_MAX // 2
+        for stale_key in list(_mail_cap_logged)[:overage]:
+            del _mail_cap_logged[stale_key]
+    _mail_cap_logged[key] = None
+    return True
 
 
 async def scan_mail_messages(
@@ -1467,18 +1485,17 @@ async def scan_mail_messages(
                 )
                 continue
 
-            cap_key = (user_id, mailbox_id)
-            if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX and cap_key not in (
-                _mail_cap_logged
+            if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX and _mark_mail_cap_logged(
+                (user_id, mailbox_id)
             ):
-                _mail_cap_logged.add(cap_key)
                 logger.info(
-                    "[SCAN-%s] Mailbox %s hit the newest-%s cap; older messages "
-                    "are not indexed (set MAIL_SCAN_MAX_PER_MAILBOX higher for "
-                    "deeper history)",
+                    "[SCAN-%s] Mailbox %s contains more than %s messages; only "
+                    "the newest %s are indexed (cursor pagination not yet "
+                    "implemented)",
                     scan_id,
                     mailbox_id,
                     MAIL_SCAN_MAX_PER_MAILBOX,
+                    MAIL_SCAN_MAX_PER_MAILBOX,
                 )
 
             for message in messages: