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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ An MCP server that connects AI assistants to Zammad, providing tools for managin
- `zammad_update_ticket` - Update ticket properties
- `zammad_add_article` - Add comments/notes to tickets
- `zammad_add_ticket_tag` / `zammad_remove_ticket_tag` - Manage ticket tags
- `zammad_merge_tickets` - Merge a source ticket into a target ticket (moves all articles; irreversible)
- `zammad_get_ticket_tags` - Get tags assigned to a specific ticket
- `zammad_list_tags` - List all tags defined in the system (requires admin.tag permission)

Expand Down
39 changes: 39 additions & 0 deletions mcp_zammad/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,45 @@ def update_ticket(

return dict(self.api.ticket.update(ticket_id, update_data))

def merge_tickets(
self,
source_ticket_id: int,
target_ticket_number: str | None = None,
target_ticket_id: int | None = None,
) -> dict[str, Any]:
"""Merge a duplicate ticket into a target ticket (irreversible)."""
# The source ticket's articles are moved into the target ticket and the
# source ticket receives the state 'merged'. This operation cannot be undone.
#
# Uses the legacy REST endpoint PUT /api/v1/ticket_merge/{source_id}/{target_number}
# via zammad_py's internal session, since the library does not expose ticket
# merge and the newer PUT /tickets/{id}/merge route returns 404 on some instances.
#
# Exactly one of target_ticket_number / target_ticket_id must be provided.
# When only the target's internal ID is known, its display number is looked up first.
#
# Raises ValueError if both or neither target identifier is provided, if the
# target ticket number lookup fails (the error identifies the TARGET ticket), or
# if the API reports the merge as failed (the ticket_merge endpoint answers
# failures with HTTP 200 and {"result": "failed", "message": ...});
# requests.HTTPError if the API request itself fails.
if (target_ticket_number is None) == (target_ticket_id is None):
raise ValueError("Provide exactly one of target_ticket_number or target_ticket_id")

if target_ticket_number is None:
try:
target = self.api.ticket.find(target_ticket_id)
except Exception as e:
raise ValueError(f"Target ticket lookup failed for target_ticket_id={target_ticket_id}: {e}") from e
target_ticket_number = str(target["number"])
Comment thread
coderabbitai[bot] marked this conversation as resolved.

response = self.api.session.put(f"{self.url}/ticket_merge/{source_ticket_id}/{target_ticket_number}")

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.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate target_ticket_number before it reaches the URL path.

target_ticket_number is only constrained to length 1-50 in TicketMergeParams; it is not restricted to a specific character set. It is interpolated directly into f"{self.url}/ticket_merge/{source_ticket_id}/{target_ticket_number}" with no URL-encoding. A crafted value containing / or .. changes the request path sent through the authenticated session.

Restrict the field to the expected ticket-number format (e.g. digits) in the model, or URL-encode the value with urllib.parse.quote before building the request path.

As per path instructions for mcp_zammad/client.py, "Check for URL validation to prevent SSRF attacks."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mcp_zammad/client.py` at line 300, Validate target_ticket_number in
TicketMergeParams against the expected ticket-number format, preferably digits
only, before it is used by the ticket merge request. Ensure the validation
rejects path separators, traversal sequences, and other nonconforming values
while preserving valid ticket numbers; alternatively, URL-encode the value at
the request construction near the session.put call.

Source: Path instructions

response.raise_for_status()
payload = response.json()
if payload.get("result") != "success":
raise ValueError(f"Ticket merge failed: {payload.get('message', payload)}")
return dict(payload)

def add_article(
self,
ticket_id: int,
Expand Down
36 changes: 36 additions & 0 deletions mcp_zammad/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,42 @@ def sanitize_title(cls, v: str | None) -> str | None:
return html.escape(v) if v else v


class TicketMergeParams(StrictBaseModel):
"""Merge ticket request parameters (exactly one of target number/ID required)."""

source_ticket_id: int = Field(
gt=0,
description="Internal database ID of the duplicate ticket (it is merged away and gets state 'merged')",
)
target_ticket_number: str | None = Field(
None,
min_length=1,
max_length=50,
pattern=r"^\d+$",
description="Display number of the ticket to merge into (e.g. '65003')",
)
target_ticket_id: int | None = Field(
None,
gt=0,
description="Internal database ID of the ticket to merge into (its number is looked up automatically)",
)

@model_validator(mode="after")
def exactly_one_target(self) -> "TicketMergeParams":
"""Ensure exactly one target identifier is provided."""
if (self.target_ticket_number is None) == (self.target_ticket_id is None):
raise ValueError("Provide exactly one of target_ticket_number or target_ticket_id")
return self


class TicketMergeResult(BaseModel):
"""Result of a ticket merge operation."""

result: str = Field(description="Merge status ('success' on success)")
target_ticket: Ticket = Field(description="The surviving ticket after the merge")
source_ticket: Ticket | None = Field(None, description="The merged (duplicate) ticket, if returned by the API")


class GetArticleAttachmentsParams(StrictBaseModel):
"""Get article attachments request parameters."""

Expand Down
62 changes: 62 additions & 0 deletions mcp_zammad/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
Ticket,
TicketCreate,
TicketIdGuidanceError,
TicketMergeParams,
TicketMergeResult,
TicketPriority,
TicketSearchParams,
TicketState,
Expand Down Expand Up @@ -1173,6 +1175,66 @@ def zammad_update_ticket(params: TicketUpdateParams) -> Ticket:
except Exception as e:
_handle_ticket_not_found_error(params.ticket_id, e)

@self.mcp.tool(annotations=_write_annotations("Merge Tickets"))
def zammad_merge_tickets(params: TicketMergeParams) -> TicketMergeResult:
"""Merge a duplicate ticket into another ticket (irreversible).

The source ticket's articles are moved into the target ticket and the
source ticket receives the state 'merged'. The target ticket keeps its
owner, state, and conversation.

Args:
params (TicketMergeParams): Validated merge parameters containing:
- source_ticket_id (int): Internal database ID of the DUPLICATE ticket
(required, NOT display number - it is merged away)
- target_ticket_number (str | None): Display number of the surviving
ticket (e.g. "65003")
- target_ticket_id (int | None): Internal database ID of the surviving
ticket (its number is looked up automatically)

Exactly one of target_ticket_number / target_ticket_id must be provided.

Returns:
TicketMergeResult: The merge result with schema:

```json
{
"result": "success",
"target_ticket": {"id": 124, "number": "65004", "state_id": 2, "...": "..."},
"source_ticket": {"id": 123, "number": "65003", "state_id": 5, "...": "..."}
}
```

Examples:
- Use when: "Merge duplicate 123 into ticket 65004" -> source_ticket_id=123, target_ticket_number="65004"
- Use when: "Merge ticket 123 into ticket 124" -> source_ticket_id=123, target_ticket_id=124
- Don't use when: Only linking related tickets (merge is irreversible)
- Don't use when: Adding a comment (use zammad_add_article)

Error Handling:
- Returns TicketIdGuidanceError if the source ticket is not found, or if the
target ticket ID lookup fails (reported against the failing identifier;
suggests using search)
- Returns "Error: Validation failed" if both or neither target identifier is given
- Returns "Ticket merge failed: ..." if Zammad rejects the merge (the API
answers failures with HTTP 200 and result='failed'; surfaced as an error)
- Returns "Error: Permission denied" if no update permissions

Note:
source_ticket_id / target_ticket_id are internal database IDs, NOT display
numbers. Use the 'id' field from search results, not the 'number' field.
Both customers may see the merged conversation - merge only tickets that
truly belong together (same organization/issue).
"""
client = self.get_client()
try:
result = client.merge_tickets(**params.model_dump(exclude_none=True))
return TicketMergeResult(**result)
except Exception as e:
if "target ticket lookup failed" in str(e).lower() and params.target_ticket_id is not None:
_handle_ticket_not_found_error(params.target_ticket_id, e)
_handle_ticket_not_found_error(params.source_ticket_id, e)

@self.mcp.tool(annotations=_write_annotations("Add Ticket Article"))
def zammad_add_article(params: ArticleCreate) -> Article:
"""Add an article (comment/note/email) to an existing ticket with optional attachments.
Expand Down
101 changes: 101 additions & 0 deletions tests/test_client_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -652,3 +652,104 @@ def test_list_tags_permission_denied(self, mock_zammad_api: Mock) -> None:

with pytest.raises(requests.HTTPError, match="403"):
client.list_tags()


class TestMergeTickets:
"""Test ZammadClient.merge_tickets."""

@pytest.fixture
def mock_zammad_api(self) -> Generator[Mock, None, None]:
"""Mock the underlying zammad_py.ZammadAPI."""
with patch("mcp_zammad.client.ZammadAPI") as mock_api:
yield mock_api

def _make_client(self, mock_zammad_api: Mock) -> tuple[ZammadClient, Mock]:
"""Build a client with a mocked API and return (client, mock_instance)."""
mock_instance = Mock()
mock_zammad_api.return_value = mock_instance
client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token")
return client, mock_instance

def test_merge_with_target_number(self, mock_zammad_api: Mock) -> None:
"""Merge uses legacy ticket_merge route with the target number."""
client, mock_instance = self._make_client(mock_zammad_api)
mock_response = Mock()
mock_response.json.return_value = {
"result": "success",
"target_ticket": {"id": 124, "number": "65004"},
"source_ticket": {"id": 123, "number": "65003", "state_id": 5},
}
mock_response.raise_for_status = Mock()
mock_instance.session.put.return_value = mock_response

result = client.merge_tickets(source_ticket_id=123, target_ticket_number="65004")

assert result["result"] == "success"
assert result["target_ticket"]["id"] == 124
mock_instance.session.put.assert_called_once_with("https://test.zammad.com/api/v1/ticket_merge/123/65004")
mock_instance.ticket.find.assert_not_called()

def test_merge_with_target_id_looks_up_number(self, mock_zammad_api: Mock) -> None:
"""When only the target ID is given, its number is resolved first."""
client, mock_instance = self._make_client(mock_zammad_api)
mock_instance.ticket.find.return_value = {"id": 124, "number": "65004"}
mock_response = Mock()
mock_response.json.return_value = {"result": "success", "target_ticket": {"id": 124, "number": "65004"}}
mock_response.raise_for_status = Mock()
mock_instance.session.put.return_value = mock_response

result = client.merge_tickets(source_ticket_id=123, target_ticket_id=124)

mock_instance.ticket.find.assert_called_once_with(124)
mock_instance.session.put.assert_called_once_with("https://test.zammad.com/api/v1/ticket_merge/123/65004")
assert result["result"] == "success"

def test_merge_rejects_both_targets(self, mock_zammad_api: Mock) -> None:
"""Providing both target identifiers raises ValueError."""
client, mock_instance = self._make_client(mock_zammad_api)

with pytest.raises(ValueError, match="exactly one"):
client.merge_tickets(source_ticket_id=123, target_ticket_number="65004", target_ticket_id=124)

mock_instance.session.put.assert_not_called()

def test_merge_rejects_no_target(self, mock_zammad_api: Mock) -> None:
"""Providing no target identifier raises ValueError."""
client, mock_instance = self._make_client(mock_zammad_api)

with pytest.raises(ValueError, match="exactly one"):
client.merge_tickets(source_ticket_id=123)

mock_instance.session.put.assert_not_called()

def test_merge_http_error_propagates(self, mock_zammad_api: Mock) -> None:
"""HTTP errors from the merge endpoint propagate."""
client, mock_instance = self._make_client(mock_zammad_api)
mock_response = Mock()
mock_response.raise_for_status.side_effect = requests.HTTPError("404 Not Found")
mock_instance.session.put.return_value = mock_response

with pytest.raises(requests.HTTPError, match="404"):
client.merge_tickets(source_ticket_id=123, target_ticket_number="99999")

def test_merge_failed_result_raises(self, mock_zammad_api: Mock) -> None:
"""Zammad reports merge failures as HTTP 200 with result='failed' - must raise."""
client, mock_instance = self._make_client(mock_zammad_api)
mock_response = Mock()
mock_response.json.return_value = {"result": "failed", "message": "The source ticket could not be found."}
mock_response.raise_for_status = Mock()
mock_instance.session.put.return_value = mock_response

with pytest.raises(ValueError, match="source ticket could not be found"):
client.merge_tickets(source_ticket_id=99999999, target_ticket_number="65004")
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def test_merge_target_lookup_failure_is_distinguishable(self, mock_zammad_api: Mock) -> None:
"""A failing target-number lookup raises an error identifying the TARGET ticket."""
client, mock_instance = self._make_client(mock_zammad_api)
mock_instance.ticket.find.side_effect = Exception("Couldn't find Ticket with 'id'=99999")

with pytest.raises(ValueError, match=r"[Tt]arget") as exc_info:
client.merge_tickets(source_ticket_id=123, target_ticket_id=99999)

assert "99999" in str(exc_info.value)
mock_instance.session.put.assert_not_called()
44 changes: 44 additions & 0 deletions tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
GetTicketParams,
ResponseFormat,
TicketCreate,
TicketMergeParams,
TicketUpdate,
)

Expand Down Expand Up @@ -58,6 +59,49 @@ def test_html_sanitization_in_title(self):
update = TicketUpdate(title="<i>Important</i> Update") # type: ignore[call-arg]
assert update.title == "&lt;i&gt;Important&lt;/i&gt; Update"


class TestTicketMergeParams:
"""Test TicketMergeParams model validation."""

def test_valid_with_target_number(self):
"""Target by display number is accepted."""
params = TicketMergeParams(source_ticket_id=123, target_ticket_number="65004") # type: ignore[call-arg]
assert params.source_ticket_id == 123
assert params.target_ticket_number == "65004"
assert params.target_ticket_id is None

def test_valid_with_target_id(self):
"""Target by internal ID is accepted."""
params = TicketMergeParams(source_ticket_id=123, target_ticket_id=124) # type: ignore[call-arg]
assert params.target_ticket_id == 124
assert params.target_ticket_number is None

def test_rejects_both_targets(self):
"""Providing both target identifiers fails validation."""
with pytest.raises(ValidationError, match="exactly one"):
TicketMergeParams(source_ticket_id=123, target_ticket_number="65004", target_ticket_id=124) # type: ignore[call-arg]

def test_rejects_no_target(self):
"""Providing no target identifier fails validation."""
with pytest.raises(ValidationError, match="exactly one"):
TicketMergeParams(source_ticket_id=123) # type: ignore[call-arg]

def test_rejects_invalid_source_id(self):
"""Source ticket ID must be positive."""
with pytest.raises(ValidationError):
TicketMergeParams(source_ticket_id=0, target_ticket_number="65004") # type: ignore[call-arg]

def test_rejects_empty_target_number(self):
"""Target number must not be empty or whitespace-only."""
with pytest.raises(ValidationError):
TicketMergeParams(source_ticket_id=123, target_ticket_number=" ") # type: ignore[call-arg]

@pytest.mark.parametrize("bad_number", ["65004/../tickets", "..", "abc", "12a34", "65 004"])
def test_rejects_non_digit_target_number(self, bad_number: str):
"""Target number must be digits only (it is interpolated into a URL path)."""
with pytest.raises(ValidationError):
TicketMergeParams(source_ticket_id=123, target_ticket_number=bad_number) # type: ignore[call-arg]

def test_none_title_not_sanitized(self):
"""Test that None title is not processed."""
update = TicketUpdate(state="closed") # type: ignore[call-arg]
Expand Down
43 changes: 43 additions & 0 deletions tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
StateBrief,
Ticket,
TicketCreate,
TicketIdGuidanceError,
TicketMergeParams,
TicketPriority,
TicketSearchParams,
TicketState,
Expand Down Expand Up @@ -498,6 +500,47 @@ def test_create_ticket_customer_not_found_error(mock_zammad_client, decorator_ca
assert "zammad_create_user" in str(exc_info.value)


class TestMergeTicketsTool:
"""Test zammad_merge_tickets error attribution (source vs target)."""

def _setup_merge_tool(self, mock_zammad_client, decorator_capturer):
"""Build a server with a mocked client and return its merge tool."""
mock_instance, _ = mock_zammad_client
server_inst = ZammadMCPServer()
server_inst.client = mock_instance
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()
return mock_instance, test_tools["zammad_merge_tickets"]

def test_target_lookup_failure_reported_against_target(self, mock_zammad_client, decorator_capturer) -> None:
"""A failing target lookup must not be misreported as a missing source ticket."""
mock_instance, merge_tool = self._setup_merge_tool(mock_zammad_client, decorator_capturer)
mock_instance.merge_tickets.side_effect = ValueError(
"Target ticket lookup failed for target_ticket_id=124: Couldn't find Ticket with 'id'=124"
)

params = TicketMergeParams(source_ticket_id=123, target_ticket_id=124) # type: ignore[call-arg]

with pytest.raises(TicketIdGuidanceError) as exc_info:
merge_tool(params)

assert exc_info.value.ticket_id == 124

def test_source_failure_reported_against_source(self, mock_zammad_client, decorator_capturer) -> None:
"""A genuine source-ticket failure is still reported against the source ticket."""
mock_instance, merge_tool = self._setup_merge_tool(mock_zammad_client, decorator_capturer)
mock_instance.merge_tickets.side_effect = Exception("Couldn't find Ticket with 'id'=123")

params = TicketMergeParams(source_ticket_id=123, target_ticket_number="65004") # type: ignore[call-arg]

with pytest.raises(TicketIdGuidanceError) as exc_info:
merge_tool(params)

assert exc_info.value.ticket_id == 123


def test_add_article_tool(mock_zammad_client, sample_article_data, decorator_capturer):
"""Test the add_article tool with ArticleCreate params model."""
mock_instance, _ = mock_zammad_client
Expand Down