From f075540232ea509d356199386841b2264fdb748e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 01:14:32 +0200 Subject: [PATCH] fix(talk): address remaining PR #741 reviewer feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the seven outstanding items from the @claude review on PR #741: 1. Add empty `tests/client/talk/__init__.py` for pytest discovery parity with `tests/client/{collectives,news}/`. 2. Standardise boolean query params to integers — `includeStatus` was the string `"true"` in `list_conversations`/`list_participants` while every other flag (`noStatusUpdate`, `lookIntoFuture`, `setReadMarker`, `includeLastKnown`) used `1`/`0`. 3. Replace the `app:install || app:enable` chain in the spreed install hook with `app:install --keep-disabled --force || true; app:enable spreed`, so unrelated install failures surface as a clear "app not found" from `app:enable` rather than being silently masked. 4. Add `_validate_token()` (alphanumeric whitelist) and call it from all six TalkClient methods that interpolate the token into a URL path — defence-in-depth against pathological tokens reaching httpx. 5. Rename `TalkConversation.type` to `room_type` with `Field(alias="type")` and `populate_by_name=True`, so the field no longer shadows Python's builtin while preserving spreed's wire format on input. MCP responses now serialize `room_type` (field name) instead of `type`. 6. `mark_as_read` now passes `json=body or None` so the bodyless "mark everything as read" call doesn't send a spurious `{}` body and `Content-Type: application/json` header. 7. `_validate_message_text` rejects whitespace-only messages, not just empty strings. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../10-install-spreed-app.sh | 22 +++++--- nextcloud_mcp_server/client/talk.py | 28 ++++++++-- nextcloud_mcp_server/models/talk.py | 20 ++++++- nextcloud_mcp_server/server/talk.py | 6 ++- tests/client/talk/__init__.py | 0 tests/client/talk/test_talk_api.py | 54 ++++++++++++++++--- tests/server/test_talk_mcp.py | 15 ++++-- 7 files changed, 119 insertions(+), 26 deletions(-) create mode 100644 tests/client/talk/__init__.py diff --git a/app-hooks/post-installation/10-install-spreed-app.sh b/app-hooks/post-installation/10-install-spreed-app.sh index f1df23b6..8992e379 100755 --- a/app-hooks/post-installation/10-install-spreed-app.sh +++ b/app-hooks/post-installation/10-install-spreed-app.sh @@ -4,11 +4,19 @@ set -euox pipefail # Talk (spreed) ships its appstore release on demand; on a fresh container the # app is not yet installed, so app:install pulls it. On rebuilds where the -# volume already has it, app:install would fail, so fall through to -# app:enable. +# volume already has it, app:install fails with "already installed" — that +# specific failure is benign, hence the trailing `|| true`. We then run +# app:enable separately, which is the action that actually has to succeed. # -# Caveat: this `||` also masks unrelated install failures (network outage, -# bad version pin, etc.) — they will fall through to app:enable, which -# will then fail with a clearer "app not found" error. Acceptable for a -# dev-fixture script; do not copy this pattern into production tooling. -php /var/www/html/occ app:install spreed || php /var/www/html/occ app:enable spreed +# This split (install || true; then enable) is preferred over the previous +# `app:install || app:enable` chain because: +# - `--keep-disabled` keeps install side-effects strictly to fetching/extracting +# the app, so the enable step is the single source of truth for whether the +# app is active. +# - `--force` skips the compatibility check, locking the script to spreed's +# current behaviour rather than the appstore's view of NC compatibility. +# - If app:install dies for an unrelated reason (network outage, appstore +# unreachable on a fresh install), app:enable now fails with the clearer +# "app not found" rather than the install-time error being masked entirely. +php /var/www/html/occ app:install spreed --keep-disabled --force || true +php /var/www/html/occ app:enable spreed diff --git a/nextcloud_mcp_server/client/talk.py b/nextcloud_mcp_server/client/talk.py index 4f2b1280..8673eb88 100644 --- a/nextcloud_mcp_server/client/talk.py +++ b/nextcloud_mcp_server/client/talk.py @@ -12,6 +12,7 @@ application/json`` is sent. """ import logging +import re from typing import Any from nextcloud_mcp_server.client.base import BaseNextcloudClient @@ -24,6 +25,18 @@ from nextcloud_mcp_server.models.talk import ( logger = logging.getLogger(__name__) +# Spreed conversation tokens are short alphanumeric strings (e.g. "a1b2c3d4"). +# httpx does not normalise path traversal sequences, so a pathological token +# like ``"../foo"`` would be sent verbatim. Validate up-front for clearer +# errors and defence-in-depth. +_TALK_TOKEN_RE = re.compile(r"^[A-Za-z0-9]+$") + + +def _validate_token(token: str) -> None: + if not _TALK_TOKEN_RE.fullmatch(token): + raise ValueError(f"Invalid Talk conversation token: {token!r}") + + class TalkClient(BaseNextcloudClient): """Client for Nextcloud Talk (spreed) app operations.""" @@ -68,7 +81,7 @@ class TalkClient(BaseNextcloudClient): if modified_since is not None: params["modifiedSince"] = modified_since if include_status: - params["includeStatus"] = "true" + params["includeStatus"] = 1 if no_status_update: params["noStatusUpdate"] = 1 response = await self._make_request( @@ -79,6 +92,7 @@ class TalkClient(BaseNextcloudClient): async def get_conversation(self, token: str) -> TalkConversation: """Fetch a single Talk conversation by its room token.""" + _validate_token(token) response = await self._make_request( "GET", f"{self._ROOM_BASE}/{token}", headers=self._talk_headers() ) @@ -112,6 +126,7 @@ class TalkClient(BaseNextcloudClient): async def delete_conversation(self, token: str) -> None: """Delete a conversation. Used by integration test cleanup.""" + _validate_token(token) await self._make_request( "DELETE", f"{self._ROOM_BASE}/{token}", headers=self._talk_headers() ) @@ -154,6 +169,7 @@ class TalkClient(BaseNextcloudClient): if the header was absent or unparseable), suitable for pagination. """ + _validate_token(token) clamped_limit = min(max(1, limit), 200) params: dict[str, Any] = { "limit": clamped_limit, @@ -208,6 +224,7 @@ class TalkClient(BaseNextcloudClient): silent: When True, the message is delivered without push notifications. """ + _validate_token(token) body: dict[str, Any] = {"message": message} if reply_to is not None: body["replyTo"] = reply_to @@ -232,13 +249,17 @@ class TalkClient(BaseNextcloudClient): that message; otherwise spreed marks everything currently in the room as read. """ + _validate_token(token) body: dict[str, Any] = {} if last_read_message is not None: body["lastReadMessage"] = last_read_message + # ``json=None`` makes httpx skip both the body and the + # ``Content-Type: application/json`` header — semantically correct + # for the bodyless "mark everything as read" call. await self._make_request( "POST", f"{self._CHAT_BASE}/{token}/read", - json=body, + json=body or None, headers=self._talk_headers(), ) @@ -248,9 +269,10 @@ class TalkClient(BaseNextcloudClient): self, token: str, *, include_status: bool = False ) -> list[TalkParticipant]: """List participants of a Talk conversation.""" + _validate_token(token) params: dict[str, Any] = {} if include_status: - params["includeStatus"] = "true" + params["includeStatus"] = 1 response = await self._make_request( "GET", f"{self._ROOM_BASE}/{token}/participants", diff --git a/nextcloud_mcp_server/models/talk.py b/nextcloud_mcp_server/models/talk.py index 0312bb03..9f5bf2f6 100644 --- a/nextcloud_mcp_server/models/talk.py +++ b/nextcloud_mcp_server/models/talk.py @@ -2,7 +2,7 @@ from typing import Any -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator from .base import BaseResponse, StatusResponse @@ -48,9 +48,25 @@ class TalkConversation(BaseModel): flows. """ + # ``populate_by_name=True`` lets us deserialize spreed's ``type`` key + # into the ``room_type`` field while still allowing internal callers + # to construct the model with ``room_type=...`` directly. + model_config = ConfigDict(populate_by_name=True) + id: int token: str - type: int + # The spreed JSON wire format uses ``type`` for the room kind, but + # ``type`` shadows Python's builtin within the class scope, which + # would silently call this int field if anyone wrote ``type(...)`` + # in a validator or method on this model. Map to ``room_type`` and + # alias the wire field instead. + room_type: int = Field( + alias="type", + description=( + "Conversation kind: 1=one-to-one, 2=group, 3=public, " + "4=changelog, 5=former one-to-one, 6=note-to-self." + ), + ) name: str displayName: str description: str = "" diff --git a/nextcloud_mcp_server/server/talk.py b/nextcloud_mcp_server/server/talk.py index a8bdfb36..52cf787d 100644 --- a/nextcloud_mcp_server/server/talk.py +++ b/nextcloud_mcp_server/server/talk.py @@ -27,8 +27,10 @@ _MESSAGE_MAX_LENGTH = 32000 def _validate_message_text(message: str) -> None: - if not message: - raise ValueError("Message text must not be empty") + # Reject both empty strings and whitespace-only strings — spreed + # would happily post the latter as a visually-blank message. + if not message or not message.strip(): + raise ValueError("Message text must not be empty or whitespace-only") if len(message) > _MESSAGE_MAX_LENGTH: raise ValueError( f"Message too long: {len(message)} characters (max {_MESSAGE_MAX_LENGTH})" diff --git a/tests/client/talk/__init__.py b/tests/client/talk/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/client/talk/test_talk_api.py b/tests/client/talk/test_talk_api.py index a38facbc..277bbbb3 100644 --- a/tests/client/talk/test_talk_api.py +++ b/tests/client/talk/test_talk_api.py @@ -5,7 +5,7 @@ import logging import httpx import pytest -from nextcloud_mcp_server.client.talk import TalkClient +from nextcloud_mcp_server.client.talk import TalkClient, _validate_token from nextcloud_mcp_server.models.talk import ( TalkConversation, TalkMessage, @@ -22,6 +22,44 @@ logger = logging.getLogger(__name__) pytestmark = pytest.mark.unit +# Token validation + + +@pytest.mark.parametrize( + "bad_token", + [ + "", + "../foo", + "a/b", + "a b", + "a.b", + "a-b", + "token!", + ], +) +def test_validate_token_rejects_invalid(bad_token): + """_validate_token rejects anything outside the alphanumeric whitelist.""" + with pytest.raises(ValueError, match="Invalid Talk conversation token"): + _validate_token(bad_token) + + +@pytest.mark.parametrize("good_token", ["a1b2c3d4", "ABC123", "abcdef", "1"]) +def test_validate_token_accepts_valid(good_token): + """Real spreed tokens — short alphanumeric strings — pass through.""" + _validate_token(good_token) + + +async def test_talk_get_conversation_rejects_path_traversal(mocker): + """Pathological tokens never reach the HTTP layer.""" + mock_make_request = mocker.patch.object(TalkClient, "_make_request") + + client = TalkClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(ValueError, match="Invalid Talk conversation token"): + await client.get_conversation("../etc/passwd") + + mock_make_request.assert_not_called() + + # Conversation tests @@ -64,7 +102,7 @@ async def test_talk_list_conversations(mocker): assert len(rooms) == 2 assert all(isinstance(r, TalkConversation) for r in rooms) assert rooms[0].token == "abc" - assert rooms[1].type == 3 + assert rooms[1].room_type == 3 mock_make_request.assert_called_once() call_args = mock_make_request.call_args @@ -89,7 +127,7 @@ async def test_talk_list_conversations_with_modified_since(mocker): params = mock_make_request.call_args[1]["params"] assert params["modifiedSince"] == 1700000000 - assert params["includeStatus"] == "true" + assert params["includeStatus"] == 1 async def test_talk_get_conversation(mocker): @@ -322,7 +360,7 @@ async def test_talk_send_message_with_reference_id_and_reply(mocker): async def test_talk_mark_as_read_no_message(mocker): - """mark_as_read with no message ID sends an empty body.""" + """mark_as_read with no message ID sends no body (json=None).""" mock_response = create_mock_response( status_code=200, json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, @@ -337,7 +375,9 @@ async def test_talk_mark_as_read_no_message(mocker): call_args = mock_make_request.call_args assert call_args[0][0] == "POST" assert "/chat/abc/read" in call_args[0][1] - assert call_args[1]["json"] == {} + # Empty body is sent as ``json=None`` so httpx skips both the body and + # the ``Content-Type: application/json`` header for this bodyless POST. + assert call_args[1]["json"] is None async def test_talk_mark_as_read_with_message(mocker): @@ -409,7 +449,7 @@ async def test_talk_list_participants(mocker): async def test_talk_list_participants_with_include_status(mocker): - """include_status=True forwards includeStatus=true as a query param.""" + """include_status=True forwards includeStatus=1 as a query param.""" mock_response = create_mock_response( status_code=200, json_data={"ocs": {"meta": {"status": "ok"}, "data": []}}, @@ -422,4 +462,4 @@ async def test_talk_list_participants_with_include_status(mocker): await client.list_participants("abc", include_status=True) params = mock_make_request.call_args[1]["params"] - assert params["includeStatus"] == "true" + assert params["includeStatus"] == 1 diff --git a/tests/server/test_talk_mcp.py b/tests/server/test_talk_mcp.py index f9dfe766..2eb52757 100644 --- a/tests/server/test_talk_mcp.py +++ b/tests/server/test_talk_mcp.py @@ -131,16 +131,21 @@ async def test_talk_list_participants( assert len(actor_ids) >= 1 -async def test_talk_send_message_validation_empty_text( - nc_mcp_client: ClientSession, temporary_conversation: dict +@pytest.mark.parametrize("blank_text", ["", " ", "\t\n", " \t \n "]) +async def test_talk_send_message_validation_blank_text( + nc_mcp_client: ClientSession, + temporary_conversation: dict, + blank_text: str, ): - """Empty message text is rejected client-side.""" + """Empty and whitespace-only message text are rejected client-side.""" token = temporary_conversation["token"] result = await nc_mcp_client.call_tool( - "talk_send_message", {"token": token, "message": ""} + "talk_send_message", {"token": token, "message": blank_text} + ) + assert result.isError is True, ( + f"Expected validation error for blank message {blank_text!r}" ) - assert result.isError is True, "Expected validation error for empty message" async def test_talk_send_message_validation_too_long(