From 0856d5995697be7e2c5b49ad684d744ecb5fafbe Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:12:15 +0200 Subject: [PATCH] harden(mail): address PR #935 round-4 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- nextcloud_mcp_server/client/mail.py | 8 ++++- nextcloud_mcp_server/models/mail.py | 8 ++++- nextcloud_mcp_server/server/mail.py | 12 ++++--- nextcloud_mcp_server/vector/mail_content.py | 8 ++++- nextcloud_mcp_server/vector/scanner.py | 35 +++++++++++++++------ 5 files changed, 54 insertions(+), 17 deletions(-) diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index a17e7493..22334bf2 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/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 {} diff --git a/nextcloud_mcp_server/models/mail.py b/nextcloud_mcp_server/models/mail.py index 0e4eb439..b6d449c9 100644 --- a/nextcloud_mcp_server/models/mail.py +++ b/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") diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index 13e59bb2..8c029092 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/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"), diff --git a/nextcloud_mcp_server/vector/mail_content.py b/nextcloud_mcp_server/vector/mail_content.py index 61f97f79..744ccf36 100644 --- a/nextcloud_mcp_server/vector/mail_content.py +++ b/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") diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 16023504..b4b0eea9 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/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: