RocketChat / RocketChat/Rocket.Chat

Message_QuoteChainLimit has off-by-one error

Open
#27,206 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
46.1k
Forks
13.9k
Avg merge
3d 3h
Merged PRs (30d)
130

Description

Description:

Commit 5afe5ceb72888f0a0d51544d89fe1b4567ff5e85 ("Chore: Migrate oembed to ts (#25622)", released in RC v5.0.0) introduced a regression in the recursive processing of Message_QuoteChainLimit. The change creates an off-by-one error, cutting off the quote chain at the wrong point.

So, for example, if Message_QuoteChainLimit is at its default value of 2 and I quote a message that itself quotes a chain of other messages, RC v4.x would show:

  • my new message, quoting...
    • an older message, quoting...
      • a still older message

However, under RC v5.x, without changing any configuration, the same action would show:

  • my new message, quoting...
    • an older message, quoting...
      • a still older message, quoting...
        • a very old message

Additionally, Message_QuoteChainLimit has a bug in that setting it to 1 (or 0) isn't honored. The implementation always truncates at no less than 2.

As an aside, the documentation for this setting does not clearly define what constitutes a Quote Chain.
I wasn't certain whether "an older message" (in the above example) counted as part of the quote chain or not; that is, is the first quoted message not considered "chained" (excluding it from the count)? Or is the entire chain counted as a group (including it in the count)? I turned to the documentation for clarification, but https://docs.rocket.chat/guides/administration/admin-panel/settings/message says merely:

Maximum Number of Chained Quotes: Lets you set the maximum number of Chained quotes.

Steps to reproduce:
  1. Go to /admin/settings/Message and set Maximum Number of Chained Quotes to 2
  2. Enter a chat room and post a message.
  3. Click on the "Curly Quotes" Quote icon for the newly created message.
  4. Type in a message and post it.
  5. Repeat steps 3-4 several times, each time clicking [Quote] on the most recently created message.
Expected behavior:

The fourth message should not include the original message in the quote chain. Instead, message number 2 should be the oldest post in the chain.

Actual behavior:

The fourth message includes all 3 prior messages in the quote chain, including the original message, even though we have exceeded the limit of 2.

Screen Shot 2022-11-07 at 23 19 19
Server Setup Information:
  • Version of Rocket.Chat Server: 5.1.2
  • Operating System: Linux
  • Deployment Method: tar
  • Number of Running Instances: 5
  • DB Replicaset Oplog:
  • NodeJS Version: v14.19.3
  • MongoDB Version: 4.2.15 / wiredTiger (oplog Enabled)
Client Setup Information
  • Desktop App or Browser Version: Chrome Version 106.0.5249.119 (Official Build) (arm64)
  • Operating System: MacOS

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing the TypeScript implementation of Message_QuoteChainLimit from the Maximum Number of Chained Quotes setting and reproduce the issue using the listed quote steps. Verify the default limit of 2 and values of 1 and 0 against the expected chain length, then add regression coverage showing that the oldest quoted message is excluded when the limit is reached.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.