message_edit: Check previous message content to prevent races.

Similar to group based setting values, we expect
the client to send the previous content alongwith
the edited content to the edit message endpoint.
We reject the request incase the previous content
doesn't match the current message content, which
could happen in case two users simultaneously edit
a message - which will be implemented in #33051.

api_docs/changelog.md

@@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with.
 
 ## Changes in Zulip 11.0
 
+**Feature level 379**
+
+* [`PATCH /messages/{message_id}`](/api/update-message): Added
+ optional parameter `prev_content_sha256`, which clients can use to
+ prevent races with the message being edited by another client.
+
 **Feature level 378**
 
 * [`GET /events`](/api/get-events): Archiving and unarchiving

web/src/click_handlers.ts

@@ -341,7 +341,7 @@ export function initialize(): void {
     });
     $("body").on("click", ".message_edit_save", function (e) {
         const $row = $(this).closest(".message_row");
-        message_edit.save_message_row_edit($row);
+        void message_edit.save_message_row_edit($row);
         e.stopPropagation();
     });
     $("body").on("click", ".message_edit_cancel", function (e) {

web/src/message_edit.ts

@@ -414,7 +414,7 @@ function handle_message_edit_enter(
             compose_validate.validate_message_length($row);
             return;
         }
-        save_message_row_edit($row);
+        void save_message_row_edit($row);
         e.stopPropagation();
         e.preventDefault();
     } else {
@@ -1197,7 +1197,7 @@ export function do_save_inline_topic_edit($row: JQuery, message: Message, new_to
     });
 }
 
-export function save_message_row_edit($row: JQuery): void {
+export async function save_message_row_edit($row: JQuery): Promise<void> {
     compose_tooltips.hide_compose_control_button_tooltips($row);
 
     assert(message_lists.current !== undefined);
@@ -1218,6 +1218,7 @@ export function save_message_row_edit($row: JQuery): void {
 
     let new_content;
     const old_content = message.raw_content;
+    assert(old_content !== undefined);
 
     const $edit_content_input = $row.find<HTMLTextAreaElement>("textarea.message_edit_content");
     const can_edit_content = $edit_content_input.attr("readonly") !== "readonly";
@@ -1263,8 +1264,11 @@ export function save_message_row_edit($row: JQuery): void {
         return;
     }
 
-    const request = {message_id: message.id, content: new_content};
-
+    const request = {
+        message_id: message.id,
+        content: new_content,
+        prev_content_sha256: await util.sha256_hash(old_content),
+    };
     if (!markdown.contains_backend_only_syntax(new_content ?? "")) {
         // If the new message content could have been locally echoed,
         // than we can locally echo the edit.
@@ -1362,6 +1366,17 @@ export function save_message_row_edit($row: JQuery): void {
                                 $container,
                             );
                             return;
+                        } else if (code === "EXPECTATION_MISMATCH") {
+                            const message = $t({
+                                defaultMessage:
+                                    "Error editing message: Message was edited by another client.",
+                            });
+                            compose_banner.show_error_message(
+                                message,
+                                compose_banner.CLASSNAMES.generic_compose_error,
+                                $container,
+                            );
+                            return;
                         }
                     }
 

web/src/util.ts

@@ -601,3 +601,17 @@ export function get_retry_backoff_seconds(
     }
     return Math.max(backoff_delay_secs, rate_limit_delay_secs);
 }
+
+export async function sha256_hash(text: string): Promise<string | undefined> {
+    // The Web Crypto API is only available in secure contexts (HTTPS or localhost).
+    if (!window.isSecureContext) {
+        return undefined;
+    }
+
+    const encoder = new TextEncoder();
+    const data = encoder.encode(text);
+    const hashBuffer = await crypto.subtle.digest("SHA-256", data);
+    const hashArray = [...new Uint8Array(hashBuffer)];
+    const hashHex = hashArray.map((byte) => byte.toString(16).padStart(2, "0")).join("");
+    return hashHex;
+}

web/tests/util.test.cjs

@@ -589,3 +589,13 @@ run_test("get_retry_backoff_seconds", () => {
     assert.ok(backoff >= 45);
     assert.ok(backoff <= 90);
 });
+
+run_test("sha256_hash", async ({override}) => {
+    const expected_hash = "f8e27cb511cd469712e3e0f2ac05a990481c0a39e11830b4f6aee729a894b769";
+    const data = "@*hamlet_and_cordelia* and #**channel>topic**";
+    let hash = await util.sha256_hash(data);
+    assert.equal(hash, undefined);
+    override(window, "isSecureContext", true);
+    hash = await util.sha256_hash(data);
+    assert.equal(hash, expected_hash);
+});

zerver/actions/message_edit.py

@@ -25,9 +25,11 @@ from zerver.actions.message_send import (
 )
 from zerver.actions.uploads import AttachmentChangeResult, check_attachment_reference_change
 from zerver.actions.user_topics import bulk_do_set_user_topic_visibility_policy
+from zerver.lib import utils
 from zerver.lib.exceptions import (
     JsonableError,
     MessageMoveError,
+    PreviousMessageContentMismatchedError,
     StreamWildcardMentionNotAllowedError,
     TopicWildcardMentionNotAllowedError,
 )
@@ -107,9 +109,10 @@ def validate_message_edit_payload(
     topic_name: str | None,
     propagate_mode: str | None,
     content: str | None,
+    prev_content_sha256: str | None,
 ) -> None:
     """
-    Checks that the data sent is well-formed. Does not handle editability, permissions etc.
+    Validates that a message edit request is well-formed. Does not handle permissions.
     """
     if topic_name is None and content is None and stream_id is None:
         raise JsonableError(_("Nothing to change"))
@@ -142,6 +145,14 @@ def validate_message_edit_payload(
     if content is not None and is_widget_message(message):
         raise JsonableError(_("Widgets cannot be edited."))
 
+    # We don't restrict this check to requests to edit content, to
+    # give the parameter a pure meaning. But clients sending this for
+    # topic moves are likely doing so in error.
+    if prev_content_sha256 is not None and prev_content_sha256 != utils.sha256_hash(
+        message.content
+    ):
+        raise PreviousMessageContentMismatchedError
+
 
 def validate_user_can_edit_message(
     user_profile: UserProfile, message: Message, edit_limit_buffer: int
@@ -1370,6 +1381,7 @@ def check_update_message(
     send_notification_to_old_thread: bool = True,
     send_notification_to_new_thread: bool = True,
     content: str | None = None,
+    prev_content_sha256: str | None = None,
 ) -> UpdateMessageResult:
     """This will update a message given the message id and user profile.
     It checks whether the user profile has the permission to edit the message
@@ -1397,7 +1409,9 @@ def check_update_message(
         if topic_name == message.topic_name():
             topic_name = None
 
-    validate_message_edit_payload(message, stream_id, topic_name, propagate_mode, content)
+    validate_message_edit_payload(
+        message, stream_id, topic_name, propagate_mode, content, prev_content_sha256
+    )
 
     message_edit_request = build_message_edit_request(
         message=message,

zerver/lib/utils.py

@@ -1,3 +1,4 @@
+import hashlib
 import re
 import secrets
 from collections.abc import Callable
@@ -68,3 +69,7 @@ def get_fk_field_name(model: type[models.Model], related_model: type[models.Mode
     assert len(foreign_key_fields_to_related_model) == 1
 
     return foreign_key_fields_to_related_model[0].name
+
+
+def sha256_hash(text: str) -> str:
+    return hashlib.sha256(text.encode()).hexdigest()

zerver/openapi/python_examples.py

@@ -1195,7 +1195,7 @@ def delete_saved_snippet(client: Client, saved_snippet_id: int) -> None:
 
 
 @openapi_test_function("/messages:post")
-def send_message(client: Client) -> int:
+def send_message(client: Client) -> tuple[int, str]:
     request: dict[str, Any] = {}
     # {code_example|start}
     # Send a channel message.
@@ -1231,7 +1231,7 @@ def send_message(client: Client) -> int:
     # Confirm the message was actually sent.
     message_id = result["id"]
     validate_message(client, message_id, request["content"])
-    return message_id
+    return message_id, request["content"]
 
 
 @openapi_test_function("/messages/{message_id}/reactions:post")
@@ -1298,7 +1298,11 @@ def test_private_message_invalid_recipient(client: Client) -> None:
 
 
 @openapi_test_function("/messages/{message_id}:patch")
-def update_message(client: Client, message_id: int) -> None:
+def update_message(client: Client, message_id: int, prev_content: str) -> None:
+    # We elect not to pass prev_content_sha256, because at present, it
+    # is likely to be experienced as clutter for almost all end users
+    # of this API.
+    #
     # {code_example|start}
     # Edit a message. Make sure that `message_id` is set to the ID of the
     # message you wish to update.
@@ -1799,11 +1803,11 @@ def test_invalid_stream_error(client: Client) -> None:
 
 def test_messages(client: Client, nonadmin_client: Client) -> None:
     render_message(client)
-    message_id = send_message(client)
+    message_id, content = send_message(client)
     set_message_edit_typing_status(client, message_id)
     add_reaction(client, message_id)
     remove_reaction(client, message_id)
-    update_message(client, message_id)
+    update_message(client, message_id, content)
     get_raw_message(client, message_id)
     get_messages(client)
     check_messages_match_narrow(client)

zerver/openapi/zulip.yaml

@@ -9002,6 +9002,20 @@ paths:
                   example: true
                 content:
                   $ref: "#/components/schemas/OptionalContent"
+                prev_content_sha256:
+                  description: |
+                    An optional SHA-256 hash of the previous raw content of the message
+                    that the client has at the time of the request.
+
+                    If provided, the server will return an error if it does not match the
+                    SHA-256 hash of the message's content stored in the database.
+
+                    Clients can use this feature to prevent races where multiple clients
+                    save conflicting edits to a message.
+
+                    **Changes**: New in Zulip 11.0 (feature level 379).
+                  type: string
+                  example: "6ae8a75555209fd6c44157c0aed8016e763ff435a19cf186f76863140143ff72" # "test content"
                 stream_id:
                   description: |
                     The channel ID to move the message(s) to, to request moving

zerver/tests/test_message_edit.py

@@ -15,6 +15,7 @@ from zerver.actions.realm_settings import (
 from zerver.actions.streams import do_change_stream_group_based_setting, do_deactivate_stream
 from zerver.actions.user_groups import add_subgroups_to_user_group, check_add_user_group
 from zerver.actions.user_topics import do_set_user_topic_visibility_policy
+from zerver.lib import utils
 from zerver.lib.message import messages_for_ids
 from zerver.lib.message_cache import MessageDict
 from zerver.lib.test_classes import ZulipTestCase
@@ -2257,3 +2258,43 @@ class EditMessageTest(ZulipTestCase):
         result_content = orjson.loads(result.content)
         self.assertEqual(result_content["result"], "success")
         self.assert_length(result_content["detached_uploads"], 0)
+
+    def test_edit_message_race_condition(self) -> None:
+        # If two users try to edit the same message at the same time,
+        # one of them should get an error message, as the `prev_content`
+        # parameter passed to the PATCH call would be outdated and not
+        # match.
+
+        # If `prev_content` does not match the expected value, this means
+        # the edit request is stale and should be rejected. We simulate
+        # that here.
+        self.login("hamlet")
+        msg_id = self.send_stream_message(
+            self.example_user("hamlet"),
+            "Denmark",
+            topic_name="editing",
+            content="Init message",
+        )
+        init_msg = utils.sha256_hash("Init message")
+        result = self.client_patch(
+            f"/json/messages/{msg_id}",
+            {
+                "content": "First user edit",
+                "prev_content_sha256": init_msg,
+            },
+        )
+        self.assert_json_success(result)
+        self.check_message(msg_id, topic_name="editing", content="First user edit")
+
+        result = self.client_patch(
+            f"/json/messages/{msg_id}",
+            {
+                "content": "Second user edit",
+                "prev_content_sha256": init_msg,
+            },
+        )
+        self.assert_json_error(
+            result,
+            "'prev_content_sha256' value does not match the expected value.",
+        )
+        self.check_message(msg_id, topic_name="editing", content="First user edit")

zerver/views/message_edit.py

@@ -153,6 +153,7 @@ def update_message_backend(
     send_notification_to_old_thread: Json[bool] = False,
     send_notification_to_new_thread: Json[bool] = True,
     content: str | None = None,
+    prev_content_sha256: str | None = None,
 ) -> HttpResponse:
     updated_message_result = check_update_message(
         user_profile,
@@ -163,6 +164,7 @@ def update_message_backend(
         send_notification_to_old_thread,
         send_notification_to_new_thread,
         content,
+        prev_content_sha256,
     )
 
     # Include the number of messages changed in the logs