Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

CVE-2024-27286: Run usermessage modifications even for change_one. 

This `if new_stream is not None` block was improperly indented,
causing it to only run if the propagation mode was not `change_one`.
Since the block controlled creation and deletion of UserMessage rows,
this led to messages being improperly still visible to members of the
old stream if they were being moved from public to private streams.
Clients also failed to receive `delete_message` events, so the
messages remained visible in their feeds until they reloaded the
application.
This commit is contained in:
Alex Vandiver 2024-03-06 22:02:48 +00:00 committed by Alex Vandiver
parent c3488bfe76
commit e964536139
2 changed files with 79 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

106
zerver/actions/message_edit.py
View File

@ -687,65 +687,61 @@ def do_update_message(
changed_message_ids = list(changed_messages.values_list("id", flat=True)) changed_message_ids = list(changed_messages.values_list("id", flat=True))
changed_messages_count = len(changed_message_ids) changed_messages_count = len(changed_message_ids)
if new_stream is not None: if new_stream is not None:
assert stream_being_edited is not None assert stream_being_edited is not None
if gaining_usermessage_user_ids: if gaining_usermessage_user_ids:
ums_to_create = [] ums_to_create = []
for message_id in changed_message_ids: for message_id in changed_message_ids:
for user_profile_id in gaining_usermessage_user_ids: for user_profile_id in gaining_usermessage_user_ids:
# The fact that the user didn't have a UserMessage originally means we can infer that the user # The fact that the user didn't have a UserMessage originally means we can infer that the user
# was not mentioned in the original message (even if mention syntax was present, it would not # was not mentioned in the original message (even if mention syntax was present, it would not
# take effect for a user who was not subscribed). If we were editing the message's content, we # take effect for a user who was not subscribed). If we were editing the message's content, we
# would rerender the message and then use the new stream's data to determine whether this is # would rerender the message and then use the new stream's data to determine whether this is
# a mention of a subscriber; but as we are not doing so, we choose to preserve the "was this # a mention of a subscriber; but as we are not doing so, we choose to preserve the "was this
# mention syntax an actual mention" decision made during the original rendering for implementation # mention syntax an actual mention" decision made during the original rendering for implementation
# simplicity. As a result, the only flag to consider applying here is read. # simplicity. As a result, the only flag to consider applying here is read.
um = UserMessageLite( um = UserMessageLite(
user_profile_id=user_profile_id, user_profile_id=user_profile_id,
message_id=message_id, message_id=message_id,
flags=UserMessage.flags.read, flags=UserMessage.flags.read,
) )
ums_to_create.append(um) ums_to_create.append(um)
bulk_insert_ums(ums_to_create) bulk_insert_ums(ums_to_create)
# Delete UserMessage objects for users who will no # Delete UserMessage objects for users who will no
# longer have access to these messages. Note: This could be # longer have access to these messages. Note: This could be
# very expensive, since it's N guest users x M messages. # very expensive, since it's N guest users x M messages.
UserMessage.objects.filter( UserMessage.objects.filter(
user_profile_id__in=[sub.user_profile_id for sub in subs_losing_usermessages], user_profile_id__in=[sub.user_profile_id for sub in subs_losing_usermessages],
message_id__in=changed_messages, message_id__in=changed_messages,
).delete() ).delete()
delete_event: DeleteMessagesEvent = { delete_event: DeleteMessagesEvent = {
"type": "delete_message", "type": "delete_message",
"message_ids": changed_message_ids, "message_ids": changed_message_ids,
"message_type": "stream", "message_type": "stream",
"stream_id": stream_being_edited.id, "stream_id": stream_being_edited.id,
"topic": orig_topic_name, "topic": orig_topic_name,
} }
send_event(user_profile.realm, delete_event, losing_access_user_ids) send_event(user_profile.realm, delete_event, losing_access_user_ids)
# Reset the Attachment.is_*_public caches for all messages # Reset the Attachment.is_*_public caches for all messages
# moved to another stream with different access permissions. # moved to another stream with different access permissions.
if new_stream.invite_only != stream_being_edited.invite_only: if new_stream.invite_only != stream_being_edited.invite_only:
Attachment.objects.filter(messages__in=changed_messages.values("id")).update( Attachment.objects.filter(messages__in=changed_messages.values("id")).update(
is_realm_public=None, is_realm_public=None,
) )
ArchivedAttachment.objects.filter( ArchivedAttachment.objects.filter(messages__in=changed_messages.values("id")).update(
messages__in=changed_messages.values("id") is_realm_public=None,
).update( )
is_realm_public=None,
)
if new_stream.is_web_public != stream_being_edited.is_web_public: if new_stream.is_web_public != stream_being_edited.is_web_public:
Attachment.objects.filter(messages__in=changed_messages.values("id")).update( Attachment.objects.filter(messages__in=changed_messages.values("id")).update(
is_web_public=None, is_web_public=None,
) )
ArchivedAttachment.objects.filter( ArchivedAttachment.objects.filter(messages__in=changed_messages.values("id")).update(
messages__in=changed_messages.values("id") is_web_public=None,
).update( )
is_web_public=None,
)
# This does message.save(update_fields=[...]) # This does message.save(update_fields=[...])
save_message_for_edit_use_case(message=target_message) save_message_for_edit_use_case(message=target_message)

34
zerver/tests/test_message_move_stream.py
View File

@ -1460,6 +1460,7 @@ class MessageMoveStreamTest(ZulipTestCase):
history_public_to_subscribers: bool, history_public_to_subscribers: bool,
user_messages_created: bool, user_messages_created: bool,
to_invite_only: bool = True, to_invite_only: bool = True,
propagate_mode: str = "change_all",
) -> None: ) -> None:
admin_user = self.example_user("iago") admin_user = self.example_user("iago")
user_losing_access = self.example_user("cordelia") user_losing_access = self.example_user("cordelia")
@ -1484,6 +1485,9 @@ class MessageMoveStreamTest(ZulipTestCase):
) )
self.send_stream_message(admin_user, old_stream.name, topic_name="test", content="Second") self.send_stream_message(admin_user, old_stream.name, topic_name="test", content="Second")
self.assert_length(get_topic_messages(admin_user, old_stream, "test"), 2)
self.assert_length(get_topic_messages(admin_user, new_stream, "test"), 0)
self.assertEqual( self.assertEqual(
UserMessage.objects.filter( UserMessage.objects.filter(
user_profile_id=user_losing_access.id, user_profile_id=user_losing_access.id,
@ -1503,16 +1507,18 @@ class MessageMoveStreamTest(ZulipTestCase):
f"/json/messages/{msg_id}", f"/json/messages/{msg_id}",
{ {
"stream_id": new_stream.id, "stream_id": new_stream.id,
"propagate_mode": "change_all", "propagate_mode": propagate_mode,
}, },
) )
self.assert_json_success(result) self.assert_json_success(result)
messages = get_topic_messages(admin_user, old_stream, "test") # We gain one more message than we moved because of a notification-bot message.
self.assert_length(messages, 0) if propagate_mode == "change_one":
self.assert_length(get_topic_messages(admin_user, old_stream, "test"), 1)
messages = get_topic_messages(admin_user, new_stream, "test") self.assert_length(get_topic_messages(admin_user, new_stream, "test"), 2)
self.assert_length(messages, 3) else:
self.assert_length(get_topic_messages(admin_user, old_stream, "test"), 0)
self.assert_length(get_topic_messages(admin_user, new_stream, "test"), 3)
self.assertEqual( self.assertEqual(
UserMessage.objects.filter( UserMessage.objects.filter(
@ -1545,6 +1551,22 @@ class MessageMoveStreamTest(ZulipTestCase):
user_messages_created=False, user_messages_created=False,
) )
def test_move_one_message_from_public_to_private_stream_not_shared_history(self) -> None:
self.parameterized_test_move_message_involving_private_stream(
from_invite_only=False,
history_public_to_subscribers=False,
user_messages_created=True,
propagate_mode="change_one",
)
def test_move_one_message_from_public_to_private_stream_shared_history(self) -> None:
self.parameterized_test_move_message_involving_private_stream(
from_invite_only=False,
history_public_to_subscribers=True,
user_messages_created=False,
propagate_mode="change_one",
)
def test_move_message_from_private_to_private_stream_not_shared_history(self) -> None: def test_move_message_from_private_to_private_stream_not_shared_history(self) -> None:
self.parameterized_test_move_message_involving_private_stream( self.parameterized_test_move_message_involving_private_stream(
from_invite_only=True, from_invite_only=True,