From 38f6807af1a547d1fc75a62bc7f24292bf81a34d Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Tue, 9 May 2023 20:45:42 +0200 Subject: [PATCH] scheduled-messages: Limit `to` parameter to user and stream IDs. For scheduled stream messages, we already limited the `to` parameter to be the stream ID, but here we return a JsonableError in the case of a ValueError when the passed value is not an integer. For scheduled direct messages, we limit the list for the `to` parameter to be user IDs. Previously, we accepted emails like we do when sending messages. --- zerver/actions/scheduled_messages.py | 26 ++++++++++++++ zerver/openapi/zulip.yaml | 8 ++--- zerver/tests/test_scheduled_messages.py | 48 +++++++++++++++++++++++-- zerver/views/scheduled_messages.py | 10 +++--- 4 files changed, 79 insertions(+), 13 deletions(-) diff --git a/zerver/actions/scheduled_messages.py b/zerver/actions/scheduled_messages.py index f3c2f25088..41f77a4f61 100644 --- a/zerver/actions/scheduled_messages.py +++ b/zerver/actions/scheduled_messages.py @@ -1,6 +1,7 @@ import datetime from typing import List, Optional, Sequence, Tuple, Union +import orjson from django.db import transaction from django.utils.translation import gettext as _ @@ -14,6 +15,31 @@ from zerver.models import Client, Realm, ScheduledMessage, UserProfile from zerver.tornado.django_api import send_event +def extract_stream_id(req_to: str) -> List[int]: + # Recipient should only be a single stream ID. + try: + stream_id = int(req_to) + except ValueError: + raise JsonableError(_("Invalid data type for stream ID")) + return [stream_id] + + +def extract_direct_message_recipient_ids(req_to: str) -> List[int]: + try: + user_ids = orjson.loads(req_to) + except orjson.JSONDecodeError: + user_ids = req_to + + if not isinstance(user_ids, list): + raise JsonableError(_("Invalid data type for recipients")) + + for user_id in user_ids: + if not isinstance(user_id, int): + raise JsonableError(_("Recipient list may only contain user IDs")) + + return list(set(user_ids)) + + def check_schedule_message( sender: UserProfile, client: Client, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index fde74fa27e..3f595f0172 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -5189,17 +5189,13 @@ paths: description: | The scheduled message's tentative target audience. - For stream messages, integer ID of the stream. For direct messages, - either a list containing integer user IDs or a list containing string - email addresses. + For stream messages, the integer ID of the stream. + For direct messages, a list containing integer user IDs. content: application/json: schema: oneOf: - type: integer - - type: array - items: - type: string - type: array items: type: integer diff --git a/zerver/tests/test_scheduled_messages.py b/zerver/tests/test_scheduled_messages.py index 5e05a6615c..57931e871f 100644 --- a/zerver/tests/test_scheduled_messages.py +++ b/zerver/tests/test_scheduled_messages.py @@ -5,6 +5,11 @@ from typing import TYPE_CHECKING, List, Union import orjson +from zerver.actions.scheduled_messages import ( + extract_direct_message_recipient_ids, + extract_stream_id, +) +from zerver.lib.exceptions import JsonableError from zerver.lib.test_classes import ZulipTestCase from zerver.lib.timestamp import timestamp_to_datetime from zerver.models import Attachment, ScheduledMessage @@ -70,7 +75,7 @@ class ScheduledMessageTest(ZulipTestCase): # Scheduling a private message is successful. othello = self.example_user("othello") result = self.do_schedule_message( - "direct", [othello.email], content + " 3", scheduled_delivery_timestamp + "direct", [othello.id], content + " 3", scheduled_delivery_timestamp ) scheduled_message = self.last_scheduled_message() self.assert_json_success(result) @@ -163,7 +168,7 @@ class ScheduledMessageTest(ZulipTestCase): othello = self.example_user("othello") result = self.do_schedule_message( - "direct", [othello.email], content + " 3", scheduled_delivery_timestamp + "direct", [othello.id], content + " 3", scheduled_delivery_timestamp ) # Multiple scheduled messages @@ -288,3 +293,42 @@ class ScheduledMessageTest(ZulipTestCase): [scheduled_message.id], ) self.assertEqual(scheduled_message.has_attachment, True) + + def test_extract_stream_id(self) -> None: + # Scheduled stream message recipient = single stream ID. + stream_id = extract_stream_id("1") + self.assertEqual(stream_id, [1]) + + with self.assertRaisesRegex(JsonableError, "Invalid data type for stream ID"): + extract_stream_id("1,2") + + with self.assertRaisesRegex(JsonableError, "Invalid data type for stream ID"): + extract_stream_id("[1]") + + with self.assertRaisesRegex(JsonableError, "Invalid data type for stream ID"): + extract_stream_id("general") + + def test_extract_recipient_ids(self) -> None: + # Scheduled direct message recipients = user IDs. + user_ids = "[3,2,1]" + result = sorted(extract_direct_message_recipient_ids(user_ids)) + self.assertEqual(result, [1, 2, 3]) + + # JSON list w/duplicates + user_ids = orjson.dumps([3, 3, 12]).decode() + result = sorted(extract_direct_message_recipient_ids(user_ids)) + self.assertEqual(result, [3, 12]) + + # Invalid data + user_ids = "1, 12" + with self.assertRaisesRegex(JsonableError, "Invalid data type for recipients"): + extract_direct_message_recipient_ids(user_ids) + + user_ids = orjson.dumps(dict(recipient=12)).decode() + with self.assertRaisesRegex(JsonableError, "Invalid data type for recipients"): + extract_direct_message_recipient_ids(user_ids) + + # Heterogeneous lists are not supported + user_ids = orjson.dumps([3, 4, "eeshan@example.com"]).decode() + with self.assertRaisesRegex(JsonableError, "Recipient list may only contain user IDs"): + extract_direct_message_recipient_ids(user_ids) diff --git a/zerver/views/scheduled_messages.py b/zerver/views/scheduled_messages.py index 161ab2e2fe..07ac02cdb0 100644 --- a/zerver/views/scheduled_messages.py +++ b/zerver/views/scheduled_messages.py @@ -1,13 +1,14 @@ -from typing import Optional, Sequence, Union +from typing import Optional from django.http import HttpRequest, HttpResponse from django.utils.timezone import now as timezone_now from django.utils.translation import gettext as _ -from zerver.actions.message_send import extract_private_recipients from zerver.actions.scheduled_messages import ( check_schedule_message, delete_scheduled_message, + extract_direct_message_recipient_ids, + extract_stream_id, ) from zerver.lib.exceptions import JsonableError from zerver.lib.request import REQ, RequestNotes, has_request_variables @@ -61,10 +62,9 @@ def scheduled_messages_backend( assert client is not None if recipient_type_name == "stream": - # req_to is ID of the recipient stream. - message_to: Union[Sequence[str], Sequence[int]] = [int(req_to)] + message_to = extract_stream_id(req_to) else: - message_to = extract_private_recipients(req_to) + message_to = extract_direct_message_recipient_ids(req_to) scheduled_message_id = check_schedule_message( sender,