From d0061454446e607f836099f095cf301ba1cb424e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:24:24 +0200 Subject: [PATCH] harden(mail): address PR #935 round-5 review No blockers raised; residual cleanup: - processor.py: guard int(doc_task.doc_id) with is_valid_nextcloud_doc_id in the mail_message branch (consistent with search/context.py + the verifier). - mail metadata symmetry: store `bcc` in file_metadata and the Qdrant payload alongside cc (build_mail_content already emits a Bcc: line). - server/mail.py: extract _cap_attachment_content helper (byte-accurate cap) and unit-test it (small/None/oversized/multibyte). - client/mail.py: give the synthetic OCS-error Response an explicit empty body; add a test that a traversal-style attachment_id is percent-encoded. - models/mail.py: clarify ListMessagesResponse.total_count is the page count, not the mailbox total. Deferred (Deck #376): _potentially_deleted doc_type-in-key. It's pre-existing and spans ~30 sites across the notes/news/deck/file/mail scanners (whose deletion paths have no unit coverage), so it belongs in its own focused PR rather than expanding this mail PR's blast radius into other doc types. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/mail.py | 4 ++- nextcloud_mcp_server/models/mail.py | 6 +++- nextcloud_mcp_server/server/mail.py | 27 +++++++++++------ nextcloud_mcp_server/vector/processor.py | 8 +++++ tests/client/mail/test_mail_api.py | 19 ++++++++++++ tests/unit/test_mail_server_helpers.py | 38 ++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 tests/unit/test_mail_server_helpers.py diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index 22334bf2..fc006210 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/nextcloud_mcp_server/client/mail.py @@ -80,7 +80,9 @@ class MailClient(BaseNextcloudClient): except (TypeError, ValueError): status_code = 200 if status_code >= 400: - synthetic = Response(status_code=status_code, request=response.request) + synthetic = Response( + status_code=status_code, request=response.request, content=b"" + ) raise HTTPStatusError( f"Mail OCS error {status_code} for {path}: {meta.get('message')}", request=response.request, diff --git a/nextcloud_mcp_server/models/mail.py b/nextcloud_mcp_server/models/mail.py index b6d449c9..2887c9f7 100644 --- a/nextcloud_mcp_server/models/mail.py +++ b/nextcloud_mcp_server/models/mail.py @@ -162,7 +162,11 @@ class ListMessagesResponse(BaseResponse): """Response model for listing message envelopes.""" results: list[MailMessageSummary] = Field(description="List of message summaries") - total_count: int = Field(description="Number of messages returned") + total_count: int = Field( + description="Number of messages returned in this page (NOT the mailbox " + "total, which isn't known without a full scan); page with cursor and " + "stop on an empty result" + ) has_more: bool = Field(False, description="Whether more messages may exist") diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index 8c029092..b9ca5f9b 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/nextcloud_mcp_server/server/mail.py @@ -32,6 +32,23 @@ logger = logging.getLogger(__name__) MAX_ATTACHMENT_CONTENT_BYTES = 5 * 1024 * 1024 +def _cap_attachment_content(content: str | None) -> str | None: + """Replace oversized attachment content with a size sentinel. + + Measures UTF-8 byte length (what actually lands in the MCP response/LLM + context), not character count. Non-string content is returned unchanged. + """ + if not isinstance(content, str): + return content + content_bytes = len(content.encode("utf-8")) + if content_bytes > MAX_ATTACHMENT_CONTENT_BYTES: + return ( + f"[attachment too large to inline: {content_bytes} bytes " + f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]" + ) + return content + + def configure_mail_tools(mcp: FastMCP): """Configure Mail app MCP tools (read-only).""" @@ -221,19 +238,11 @@ def configure_mail_tools(mcp: FastMCP): client = await get_client(ctx) try: data = await client.mail.get_attachment(message_id, attachment_id) - content = data.get("content") - 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"), size=data.get("size"), - content=content, + content=_cap_attachment_content(data.get("content")), ) except RequestError as e: raise McpError( diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 1a1a1603..5b2ccf2c 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -42,6 +42,7 @@ from nextcloud_mcp_server.observability.metrics import ( from nextcloud_mcp_server.observability.tracing import trace_operation from nextcloud_mcp_server.search.pdf_highlighter import PDFHighlighter from nextcloud_mcp_server.usage import UsageEventStore +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector import payload_keys from nextcloud_mcp_server.vector._errors import format_exception_group from nextcloud_mcp_server.vector.dead_letter import ( @@ -848,6 +849,11 @@ async def _index_document( # Fetch the full message via the Mail OCS API. The Mail app handles # IMAP server-side; we only ever speak HTTP. build_mail_content is # shared with search/context.py so index- and query-time text match. + # Guard the cast before the network call (consistent with the same + # doc_type in search/context.py) so a malformed queue record produces + # a specific error rather than a bare ValueError. + if not is_valid_nextcloud_doc_id(doc_task.doc_id): + raise ValueError(f"Invalid mail_message doc_id: {doc_task.doc_id!r}") message = await nc_client.mail.get_message(int(doc_task.doc_id)) content = build_mail_content(message) @@ -861,6 +867,7 @@ async def _index_document( "from": format_mail_addresses(message.get("from")), "to": format_mail_addresses(message.get("to")), "cc": format_mail_addresses(message.get("cc")), + "bcc": format_mail_addresses(message.get("bcc")), "date_int": message.get("dateInt"), "has_attachments": bool(message.get("attachments")), "account_id": (doc_task.metadata or {}).get("account_id"), @@ -1640,6 +1647,7 @@ async def _index_document( "from": file_metadata.get("from"), "to": file_metadata.get("to"), "cc": file_metadata.get("cc"), + "bcc": file_metadata.get("bcc"), "date_int": file_metadata.get("date_int"), "has_attachments": file_metadata.get("has_attachments"), "account_id": file_metadata.get("account_id"), diff --git a/tests/client/mail/test_mail_api.py b/tests/client/mail/test_mail_api.py index 4ae1068e..07c9ee2a 100644 --- a/tests/client/mail/test_mail_api.py +++ b/tests/client/mail/test_mail_api.py @@ -196,6 +196,25 @@ async def test_get_attachment_unwraps_json(mocker): assert args == ("GET", "/ocs/v2.php/apps/mail/api/message/100/attachment/1.2") +async def test_get_attachment_url_encodes_attachment_id(mocker): + """A traversal-style attachment_id is percent-encoded in the URL path.""" + mock_response = _ocs_response({"name": "x", "content": "y"}) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + await client.get_attachment(100, "../../evil") + + args, _ = mock_make_request.call_args + # The "/" and ".." are encoded, so they can't escape the attachment path. + assert args == ( + "GET", + "/ocs/v2.php/apps/mail/api/message/100/attachment/..%2F..%2Fevil", + ) + + async def test_empty_data_returns_empty_list(mocker): """A null ocs.data payload degrades to an empty list for list endpoints.""" mock_response = _ocs_response(None) diff --git a/tests/unit/test_mail_server_helpers.py b/tests/unit/test_mail_server_helpers.py new file mode 100644 index 00000000..7d3027d3 --- /dev/null +++ b/tests/unit/test_mail_server_helpers.py @@ -0,0 +1,38 @@ +"""Unit tests for server/mail.py helper logic.""" + +import pytest + +from nextcloud_mcp_server.server.mail import ( + MAX_ATTACHMENT_CONTENT_BYTES, + _cap_attachment_content, +) + +pytestmark = pytest.mark.unit + + +def test_small_content_passes_through(): + assert _cap_attachment_content("hello") == "hello" + + +def test_none_content_passes_through(): + assert _cap_attachment_content(None) is None + + +def test_oversized_content_replaced_with_byte_sentinel(): + oversized = "a" * (MAX_ATTACHMENT_CONTENT_BYTES + 1) + result = _cap_attachment_content(oversized) + assert result != oversized + assert "too large to inline" in result + # Reports the actual UTF-8 byte count, not character count. + assert f"{MAX_ATTACHMENT_CONTENT_BYTES + 1} bytes" in result + + +def test_multibyte_counted_in_bytes_not_chars(): + # Each "€" is 3 UTF-8 bytes; a string just under the byte cap in characters + # can still exceed it in bytes. + char_count = (MAX_ATTACHMENT_CONTENT_BYTES // 3) + 1 + content = "€" * char_count + # Under the cap by character count, over it by byte count -> capped. + assert len(content) <= MAX_ATTACHMENT_CONTENT_BYTES + assert len(content.encode("utf-8")) > MAX_ATTACHMENT_CONTENT_BYTES + assert "too large to inline" in _cap_attachment_content(content)