Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
version: '3.8'

services:
zammad-mcp:
build:
context: .
dockerfile: Dockerfile
environment:
- ZAMMAD_URL=${ZAMMAD_URL}
- ZAMMAD_HTTP_TOKEN=${ZAMMAD_HTTP_TOKEN}
Comment thread
Erudition marked this conversation as resolved.
- MCP_TRANSPORT=http
- MCP_HOST=0.0.0.0
- MCP_PORT=8000
ports:
- "9146:8000"
restart: unless-stopped
5 changes: 4 additions & 1 deletion mcp_zammad/__main__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Entry point for the Zammad MCP server."""

from .config import TransportConfig, TransportType
from .server import mcp
from .server import _configure_logging, mcp


def main() -> None:
Expand All @@ -12,6 +12,9 @@ def main() -> None:
- MCP_HOST: Host for HTTP transport (default: 127.0.0.1)
- MCP_PORT: Port for HTTP transport (required if transport=http)
"""
# Configure logging first to prevent output leakage
_configure_logging()

# Load transport configuration from environment
config = TransportConfig.from_env()
config.validate()
Expand Down
16 changes: 16 additions & 0 deletions mcp_zammad/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ def add_article(
article_type: str = "note",
internal: bool = False,
sender: str = "Agent",
subject: str | None = None,
to: str | None = None,
cc: str | None = None,
content_type: str | None = None,
attachments: list[dict[str, str]] | None = None,
) -> dict[str, Any]:
"""Add an article (comment/note) to a ticket with optional attachments.
Expand All @@ -250,6 +254,10 @@ def add_article(
article_type: Article type (note, email, phone)
internal: Whether the article is internal
sender: Sender type (Agent, Customer, System)
subject: Optional article subject (for emails)
to: Optional email recipient (for email type)
cc: Optional email CC recipients
content_type: Optional content type (text/plain or text/html)
attachments: Optional list of attachments with keys:
- filename: str
- data: str (base64-encoded content)
Expand All @@ -266,6 +274,14 @@ def add_article(
"sender": sender,
}

if subject:
article_data["subject"] = subject
if to:
article_data["to"] = to
if cc:
article_data["cc"] = cc
if content_type:
article_data["content_type"] = content_type
if attachments:
article_data["attachments"] = attachments

Expand Down
4 changes: 4 additions & 0 deletions mcp_zammad/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,10 @@ class ArticleCreate(StrictBaseModel):
article_type: ArticleType = Field(default=ArticleType.NOTE, alias="type", description="Article type")
internal: bool = Field(default=False, description="Whether the article is internal")
sender: ArticleSender = Field(default=ArticleSender.AGENT, description="Sender type")
subject: str | None = Field(default=None, description="Article subject (for emails)")
to: str | None = Field(default=None, description="Email recipient (for email type)")
cc: str | None = Field(default=None, description="Email CC recipients")
content_type: str | None = Field(default=None, description="Content type (text/plain or text/html)")
Comment on lines +325 to +328
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

content_type support is incomplete at the model boundary.

The new header/content fields are all plain strings right now, and sanitize_body() still escapes every body before it leaves the model. That breaks text/html articles by sending literal tags, while unsupported content_type values still go straight to the API. Validate content_type explicitly and make body escaping conditional on the selected content type.

As per coding guidelines, "Validate all user inputs", "Check that all fields have appropriate defaults and constraints.", and "Verify HTML sanitization is applied where needed."

Also applies to: 333-337

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mcp_zammad/models.py` around lines 325 - 328, The content_type field
currently allows any string and always causes sanitize_body() to escape the
body, breaking text/html and allowing unsupported values; change content_type to
an explicit validated enum (accept only "text/plain" and "text/html" and default
to "text/plain") on the model (the Field named content_type) and add a validator
that rejects/normalizes other values, then update sanitize_body() to
conditionally process the body: if content_type == "text/plain" escape/encode as
before, if content_type == "text/html" run an HTML sanitizer (or a safe
whitelist/stripper) instead of escaping, and ensure unsupported values are
rejected before reaching the API; reference the Field content_type and the
sanitize_body() function when making these changes (also apply same validation
to the similar fields around lines 333-337).

attachments: list[AttachmentUpload] | None = Field(
default=None, description="Optional attachments to include", max_length=10
)
Expand Down
7 changes: 5 additions & 2 deletions mcp_zammad/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import json
import logging
import os
import sys
import time
from collections.abc import AsyncIterator
from contextlib import asynccontextmanager
Expand Down Expand Up @@ -819,7 +820,9 @@ async def lifespan(_app: FastMCP) -> AsyncIterator[None]:
def get_client(self) -> ZammadClient:
"""Get the Zammad client, ensuring it's initialized."""
if not self.client:
raise RuntimeError("Zammad client not initialized")
logger.debug("Zammad client not initialized, performing lazy initialization")
# Lazy initialization for cases where lifespan initialization hasn't finished
self.client = ZammadClient()
Comment on lines +823 to +825
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Lazy get_client() skips the startup setup path.

initialize() loads .env files and eagerly verifies the connection, but the new lazy branch goes straight to ZammadClient(). A request that hits this path before lifespan now ignores file-based config and moves auth/connectivity failures from startup into the request path. Reuse the same setup helper here instead of constructing the client directly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mcp_zammad/server.py` around lines 823 - 825, The lazy path in get_client()
constructs ZammadClient() directly, bypassing the startup setup in initialize()
that loads .env and verifies connectivity; instead, call the same setup routine
used by initialize() (or extract that logic into a private helper like
_setup_client and invoke it from both initialize() and get_client()) so the lazy
branch loads file-based config and runs the same auth/connectivity verification
before assigning self.client, and preserve the same error handling/logging
behavior.

return self.client

async def initialize(self) -> None:
Expand Down Expand Up @@ -2548,7 +2551,7 @@ def _configure_logging() -> None:

# Add handler if none exists
if not root_logger.handlers:
handler = logging.StreamHandler()
handler = logging.StreamHandler(sys.stderr)
handler.setFormatter(logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s"))
root_logger.addHandler(handler)

Expand Down
86 changes: 79 additions & 7 deletions tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import pytest
import requests
from pydantic import ValidationError
from zammad_py.exceptions import ConfigException

from mcp_zammad.models import (
Article,
Expand Down Expand Up @@ -249,8 +250,8 @@ def test_tool_without_client():
server_inst = ZammadMCPServer()
server_inst.client = None

# Should raise RuntimeError when client is not initialized
with pytest.raises(RuntimeError, match="Zammad client not initialized"):
# Should raise ConfigException when client is not initialized and env vars are missing
with pytest.raises(ConfigException, match="Zammad URL is required"):
server_inst.get_client()


Expand Down Expand Up @@ -517,7 +518,16 @@ def test_add_article_tool(mock_zammad_client, sample_article_data, decorator_cap

# Verify the client was called with correct params (including attachments=None for backward compat)
mock_instance.add_article.assert_called_once_with(
ticket_id=1, article_type="note", attachments=None, body="New comment", internal=False, sender="Agent"
ticket_id=1,
article_type="note",
attachments=None,
body="New comment",
internal=False,
sender="Agent",
subject=None,
to=None,
cc=None,
content_type=None,
)


Expand Down Expand Up @@ -598,9 +608,71 @@ def test_add_article_with_attachments_tool(mock_zammad_client, decorator_capture
assert "attachments" in call_kwargs
assert call_kwargs["attachments"] is not None
assert len(call_kwargs["attachments"]) == 1
assert call_kwargs["attachments"][0]["filename"] == "doc.pdf"
assert call_kwargs["attachments"][0]["data"] == "dGVzdA=="
assert call_kwargs["attachments"][0]["mime-type"] == "application/pdf"


def test_add_article_with_email_fields(mock_zammad_client, decorator_capturer):
"""Test zammad_add_article tool with email-specific fields."""
mock_instance, _ = mock_zammad_client

# Mock client response
mock_instance.add_article.return_value = {
"id": 456,
"ticket_id": 123,
"body": "Email body",
"type": "email",
"internal": False,
"sender": "Agent",
"subject": "Re: Help",
"to": "customer@example.com",
"cc": "boss@example.com",
"content_type": "text/plain",
"created_at": "2024-01-15T16:30:00Z",
"updated_at": "2024-01-15T16:30:00Z",
"created_by_id": 1,
"updated_by_id": 1,
}

server_inst = ZammadMCPServer()
server_inst.client = mock_instance

# Capture tools using shared fixture
test_tools, capture_tool = decorator_capturer(server_inst.mcp.tool)
server_inst.mcp.tool = capture_tool # type: ignore[method-assign, assignment]
server_inst.get_client = lambda: server_inst.client # type: ignore[method-assign, assignment, return-value]
server_inst._setup_tools()

# Create params with email fields
params = ArticleCreate(
ticket_id=123,
body="Email body",
article_type=ArticleType.EMAIL,
subject="Re: Help",
to="customer@example.com",
cc="boss@example.com",
content_type="text/plain",
)

# Call tool
result = test_tools["zammad_add_article"](params)

# Verify result
assert result.id == 456
assert result.subject == "Re: Help"
assert result.to == "customer@example.com"

# Verify client.add_article was called with all email fields
mock_instance.add_article.assert_called_once_with(
ticket_id=123,
article_type="email",
body="Email body",
internal=False,
sender="Agent",
subject="Re: Help",
to="customer@example.com",
cc="boss@example.com",
content_type="text/plain",
attachments=None,
)


def test_add_article_without_attachments_backward_compat_tool(mock_zammad_client, decorator_capturer):
Expand Down Expand Up @@ -1277,7 +1349,7 @@ def test_get_client_error():
server = ZammadMCPServer()
server.client = None

with pytest.raises(RuntimeError, match="Zammad client not initialized"):
with pytest.raises(ConfigException, match="Zammad URL is required"):
server.get_client()


Expand Down