chore: undo loadMissedMessages deprecation from #40711#40981
Merged
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughThe client ChangesloadMissedMessages reconnect flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ggazzo
force-pushed
the
feat/chat-history-endpoint
branch
from
c2546e8 to
6247a46
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40981 +/- ##
===========================================
+ Coverage 70.08% 70.15% +0.06%
===========================================
Files 3357 3358 +1
Lines 129543 129559 +16
Branches 22474 22468 -6
===========================================
+ Hits 90787 90886 +99
+ Misses 35456 35355 -101
- Partials 3300 3318 +18
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Add a new `GET /v1/chat.history` REST endpoint that returns a room's
message history by `roomId` regardless of room type (channel, private
group or DM), mirroring the behavior of `channels.history`,
`groups.history` and `im.history` without requiring the caller to know
the room type beforehand.
It reuses `getChannelHistory`, which resolves access via
`Authorization.canReadRoom`, so the same `{ rid, ts }`-indexed range
query is used for any room. This provides a performant REST replacement
for the deprecated `loadMissedMessages` DDP method, whose deprecation
notice now points to `/v1/chat.history`.
Replace the `chat.syncMessages` REST call in the reconnect hook with the new `GET /v1/chat.history` endpoint. On reconnection the hook now fetches only messages created after the newest loaded message's timestamp (`ts`) via a single indexed range query, instead of scanning `_updatedAt` plus the trash collection.
ggazzo
force-pushed
the
feat/chat-history-endpoint
branch
from
6247a46 to
daa2272
Compare
chat.history replaces the deprecated loadMissedMessages DDP method only. The reconnect hook must keep chat.syncMessages so it still reconciles edits and deletions made while offline.
Reconnect catch-up now uses /v1/chat.history. Page through with offset so the server-side count clamp (API_Upper_Count_Limit) cannot silently drop missed messages when many were sent while offline.
Reverts the loadMissedMessages deprecation added in #40711. Removes the new chat.history REST endpoint (types, tests) and points the reconnect catch-up back at the loadMissedMessages DDP method. chat.syncMessages would be the right long-term target (it also reconciles edits/deletions on reconnect), but its _updatedAt + trash-collection query is currently too slow for this path; left a TODO to optimize that query before migrating.
ggazzo
changed the title
sampaiodiego
approved these changes
Jun 17, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts (1)
31-31: 💤 Low valueRedundant
Array.from()call.
resultis alreadyIMessage[]when truthy (per the server method'sfalse | IMessage[]return type), soArray.from()is unnecessary.♻️ Suggested simplification
- await Promise.all(Array.from(result).map((msg) => upsertMessage({ msg, subscription }))); + await Promise.all(result.map((msg) => upsertMessage({ msg, subscription })));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts` at line 31, In the useLoadMissedMessages hook, remove the redundant Array.from() wrapper around result in the Promise.all mapping operation. Since result is already typed as IMessage[] (per the server method's false | IMessage[] return type), you can call map() directly on result without converting it to an array first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts`:
- Line 31: In the useLoadMissedMessages hook, remove the redundant Array.from()
wrapper around result in the Promise.all mapping operation. Since result is
already typed as IMessage[] (per the server method's false | IMessage[] return
type), you can call map() directly on result without converting it to an array
first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1480eedc-4887-4a78-b6bd-365d8074a91d
📒 Files selected for processing (2)
apps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/server/methods/loadMissedMessages.ts
💤 Files with no reviewable changes (1)
- apps/meteor/server/methods/loadMissedMessages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts
🧠 Learnings (5)
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts
🔇 Additional comments (2)
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts (2)
6-6: LGTM!
41-57: LGTM!
ggazzo
changed the title
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
There was a problem hiding this comment.
The server-side Meteor method loadMissedMessages fetches missed messages in a room after a given timestamp start without enforcing any limit or pagination. It calls Messages.findVisibleByRoomIdAfterTimestamp and converts the resulting cursor directly to an array via .toArray().
If an attacker (or any authorized user of a room) calls this method with a very old timestamp (e.g., new Date(0)) in a room/channel containing a large history of messages, the server will query and load all of those messages into memory at once before serializing and sending them to the client. This can lead to severe resource exhaustion, high CPU usage, and potentially an Out Of Memory (OOM) crash of the Node.js server process, resulting in a Denial of Service (DoS) for all users of the platform.
Steps to Reproduce
An attacker can execute the following DDP method call over WebSockets for any room they have access to (e.g., a large public channel like #general):
Meteor.call('loadMissedMessages', 'GENERAL_ROOM_ID', new Date(0), (err, res) => {
if (err) console.error(err);
else console.log(`Fetched ${res.length} messages`);
});If the channel has hundreds of thousands of messages, this triggers a massive database fetch and memory allocation, leading to server instability or crash.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/authorization/server/functions/canAccessRoom.ts"]
._Rocket.Chat_apps_meteor_app_authorization_server_functions_canAccessRoom.ts["Exports authorization functions and room access attributes for room access control."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/client/meteor/overrides/userAndUsers.ts"]
Meteor.userId["Overrides Meteor.userId to return the reactive userId from the local store."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/client/meteor/user.ts"]
watchUserId["Watches the userId store and returns the reactive current user ID."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/client/meteor/watch.ts"]
watch["watch"]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/server/methods/loadMissedMessages.ts"]
loadMissedMessages{{"Server-side Meteor method to fetch missed messages for a room after a timestamp."}}
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
loadMissedMessages --> Meteor.userId
loadMissedMessages --> ._Rocket.Chat_apps_meteor_app_authorization_server_functions_canAccessRoom.ts
Meteor.userId --> watchUserId
watchUserId --> watch
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/server/methods/loadMissedMessages.ts
Severity: medium
Vulnerability: Unbounded Database Query and Resource Exhaustion in loadMissedMessages Method
Description:
The server-side Meteor method `loadMissedMessages` fetches missed messages in a room after a given timestamp `start` without enforcing any limit or pagination. It calls `Messages.findVisibleByRoomIdAfterTimestamp` and converts the resulting cursor directly to an array via `.toArray()`.
If an attacker (or any authorized user of a room) calls this method with a very old timestamp (e.g., `new Date(0)`) in a room/channel containing a large history of messages, the server will query and load all of those messages into memory at once before serializing and sending them to the client. This can lead to severe resource exhaustion, high CPU usage, and potentially an Out Of Memory (OOM) crash of the Node.js server process, resulting in a Denial of Service (DoS) for all users of the platform.
Proof of Concept:
An attacker can execute the following DDP method call over WebSockets for any room they have access to (e.g., a large public channel like `#general`):
```javascript
Meteor.call('loadMissedMessages', 'GENERAL_ROOM_ID', new Date(0), (err, res) => {
if (err) console.error(err);
else console.log(`Fetched ${res.length} messages`);
});
```
If the channel has hundreds of thousands of messages, this triggers a massive database fetch and memory allocation, leading to server instability or crash.
Affected Code:
- [loadMissedMessages.ts L17-L36](https://github.com/RocketChat/Rocket.Chat/blob/develop/apps/meteor/server/methods/loadMissedMessages.ts#L17-L36)
```typescript
return Messages.findVisibleByRoomIdAfterTimestamp(rid, start, true, {
sort: {
ts: -1,
},
}).toArray();
```
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Reverts the
loadMissedMessagesdeprecation introduced in #40711.#40711 migrated the reconnect catch-up hook (
useLoadMissedMessages) from theloadMissedMessagesDDP method to/v1/chat.syncMessages, and marked the method deprecated.chat.syncMessagesis the right long-term target — it queries by_updatedAt(+ the trash collection), so on reconnect it also reconciles edits and deletions made while offline, not just new messages.However, that query is currently too slow for the reconnect path. This PR rolls the hook back to the
loadMissedMessagesDDP method and removes the premature deprecation notice. ATODO(ddp-removal)is left in the hook documenting that we should move tochat.syncMessagesonce the query is optimized.Changes
useLoadMissedMessages→ calls theloadMissedMessagesDDP method again (with aTODOto revisitchat.syncMessages).loadMissedMessagesmethod → deprecation notice removed.How to test
loadMissedMessagesdeprecation warning is logged on use.ARCH-2164
Summary by CodeRabbit