"Hide System Messages" not honored by stream-room-messages

[nmagedman]

Description:

Certain events create messages which are displayed in the chat conversation along with user-generated messages, e.g. "User Alice added by Bob". The settings page /admin/settings/Message has a section Hide System Messages, with a multiselect of Hide “Foo” messages. When we set, for example, Hide "User Added" messages, those messages still exist in the database (and perhaps are still sent to the client?), but they are not displayed to the user.

After a recent upgrade from 4.8.4 to 5.1.2, that hiding stopped working in certain situations. It works for messages that are loaded on the initial page load or when scrolling back, but the messages are not hidden when received as part of the websocket event feed, that is, while the user is actively online in the room.

Steps to reproduce:

1. Prepare two web browsers, so we can have multiple, concurrent sessions during this exercise, logged in as two different users.

2. In browser 1, log in as an Admin

3. Go to /admin/settings/Message and add “Hide "User Added" Messages” to the “Hide System Messages” list
   

4. Ensure that user "A" is in a given chat room and that user "B" is not.

5. In browser 2, log in as user "A" and sit in that same chat room

6. In browser 1 (Admin), add user "B" to the chat room

7. In browser 2 (user A), notice that the "User B added by Admin" message is posted to the room.
   

8. In browser 2 (user A), reload the page. Notice that the "User B added by Admin" is no longer visible.
   

Expected behavior:

The "User B added by Admin" message should not be visible at any time, neither by users who are logged in at the time of the "au" event nor by users who enter the chatroom afterward.

Actual behavior:

The "User B added by Admin" message is (incorrectly) shown to users who are logged in at the time of the "au" event. It is (correctly) not shown to users who enter the chatroom afterward.

Server Setup Information:

• Version of Rocket.Chat Server: 5.1.2
• Operating System: Linux
• Deployment Method: tar
• Number of Running Instances: 15
• DB Replicaset Oplog: oplog Enabled
• NodeJS Version: v14.19.3
• MongoDB Version: 4.2.15 / wiredTiger (oplog Enabled)

Client Setup Information

• Desktop App or Browser Version: Chrome Version 105.0.5195.125 (Official Build) (arm64)
• Operating System: MacOS

Additional context

Recently upgraded RC from v4.8.4 to v5.1.2. We started receiving reports of this "new" notification shortly afterward.

Relevant logs:

In Chrome's developer tools, the websocket shows the following message (formatted for readability)

{
  "msg": "changed",
  "collection": "stream-room-messages",
  "id": "id",
  "fields": {
    "eventName": "CLAryzmwwkidzN7fi",
    "args": [
      {
        "_id": "QAt3GG9qWGrSBXMCr",
        "t": "au",
        "rid": "CLAryzmwwkidzN7fi",
        "ts": {"$date": 1663837364689},
        "msg": "test.user.noach.3",
        "u": {
          "_id": "YAs2xng29uJwjRTnx",
          "username": "Seeking.Alpha"
        },
        "groupable": false,
        "_updatedAt": {"$date": 1663837364710}
      }
    ]
  }
}

[nmagedman]

I’ve looked through $ git diff 4.8.4..5.1.2 and could not find why these messages are no longer filtered out of the event stream. Some leads on relevant files:

• apps/meteor/app/ui/client/views/app/lib/roomHelpers.ts

  function messagesHistory() {  // heavily abridged
    hideMessagesOfType = new Set(settings.collection.findOne('Hide_System_Messages').value);
    const query: Mongo.Query<IMessage> = {
      rid,
      ...(hideMessagesOfType.size && { t: { $nin: Array.from(hideMessagesOfType.values()) } }),
    };
  

• apps/meteor/app/lib/server/lib/getHiddenSystemMessages.ts
  • Defines getHiddenSystemMessages (misnomered) : returns the types that should be hidden
  • if room.sysMes is an Array, it returns that array instead of what’s configured in Settings, however every room in the database either has sysMes set to true or undefined. Further, in both of the two usages of this function, the rooms are loaded from the db rather than created dynamically or mutated in memory.
• apps/meteor/app/lib/server/methods/getChannelHistory.ts
  • Defines getChannelHistory which calls getHiddenSystemMessages
• apps/meteor/app/lib/server/functions/loadMessageHistory.ts
  • Defines loadMessageHistory which calls getHiddenSystemMessages
• apps/meteor/app/models/server/models/Messages.js
  • Defines several functions findVisibleByRoomId…NotContainingTypes (called by the above two) which all contain:

    if (Match.test(types, [String]) && types.length > 0) {
      query.t = { $nin: types };
    }
    

AFAICT, these all work. I’m not sure which of these are for fetching message history during proactive fetches from the server (e.g. during pageload / scrollback) vs. received on the stream-room-messages websocket (while lurking in the room), but they all seem to check out; they correctly filter out messages of those types.

[nmagedman]

I've implemented an ugly workaround by filtering it out client-side in apps/meteor/app/ui-utils/client/lib/RoomManager.ts.

diff --git a/apps/meteor/app/ui-utils/client/lib/RoomManager.ts b/apps/meteor/app/ui-utils/client/lib/RoomManager.ts
index 85f281f944..5b2edc0360 100644
--- a/apps/meteor/app/ui-utils/client/lib/RoomManager.ts
+++ b/apps/meteor/app/ui-utils/client/lib/RoomManager.ts
@@ -174,6 +174,9 @@ const computation = Tracker.autorun(() => {
 				if (record.streamActive !== true) {
 					(
 						roomMessagesStream.on(record.rid, async (msg) => {
+							if (['au', 'ru'].includes(msg.t)) {
+								return;
+							}
 							// Should not send message to room if room has not loaded all the current messages
 							// if (RoomHistoryManager.hasMoreNext(record.rid) !== false) {
 							// 	return;

It's ugly for three reasons:

1. I'm just hardcoding the message types to filter, rather than using the actual Settings, so this code is just a workaround.
2. It seems to me that we should be filtering it server-side rather than having separate implementations of the filtering logic on the web and mobile clients.
3. The server-side code looks like it is attempting to filter it out in multiple places but is ineffective. If those codepaths are dead, they should be removed. At the very least, they should document in which scenarios they are meant to filter.

[hugocostadev]

Hi @nmagedman thank you for submitting the issue.

We fixed this Pull Request: #27151 and will be released in the next release.

Thank you!