fix(federation): can't add local users to remote rooms #37246
Conversation
|
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 |
|
There was a problem hiding this comment.
Pull Request Overview
Fix updates federation invite handling so local users can be added to remote rooms by adjusting how joinUser is invoked.
- Updated joinUser calls to pass the full inviteEvent instead of only the roomId.
- Consistent change applied in joinRoom flow and acceptInvite handler.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WalkthroughUpdated two calls to joinUser in ee/packages/federation-matrix/src/api/_matrix/invite.ts to pass the full Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor RemoteServer
participant InviteHandler as invite.ts
participant RoomService as room.joinUser
RemoteServer->>InviteHandler: incoming inviteEvent
InviteHandler->>RoomService: joinUser(inviteEvent, inviteEvent.event.state_key)
Note right of RoomService: Previously called with roomId\nNow receives full inviteEvent
RoomService-->>InviteHandler: join result
InviteHandler-->>RemoteServer: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
sampaiodiego
added
the
stat: QA assured
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37246 +/- ##
=================================================
Coverage ? 66.37%
=================================================
Files ? 3386
Lines ? 115628
Branches ? 21360
=================================================
Hits ? 76745
Misses ? 36270
Partials ? 2613
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/server/services/messages/service.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(5 hunks)packages/core-services/src/types/IMessageService.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/server/services/messages/service.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)apps/meteor/app/lib/server/methods/sendMessage.ts (1)
executeSendMessage(22-109)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/core-services/src/types/IMessageService.ts (1)
12-37: LGTM!The addition of the
ts: Dateparameter tosaveMessageFromFederationcorrectly extends the API to accept timestamp information for federation messages. This aligns with theIMessageinterface wheretsis a required field (see packages/core-typings/src/IMessage/IMessage.ts line 142).apps/meteor/server/services/messages/service.ts (1)
88-131: LGTM with timestamp validation caveat.The implementation correctly threads the
tsparameter through toexecuteSendMessageand matches theIMessageServiceinterface definition. However, note the timestamp validation concern raised in the review of ee/packages/federation-matrix/src/events/message.ts regardingexecuteSendMessage's 60-second sync validation potentially rejecting legitimate federation messages with older timestamps.
| msg: formatted, | ||
| federation_event_id: data.event_id, | ||
| thread, | ||
| ts: new Date(data.origin_server_ts), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for origin_server_ts before Date conversion.
If data.origin_server_ts is undefined or not a valid number, new Date(data.origin_server_ts) will create an Invalid Date, which could cause issues in downstream validation or database operations.
Apply this pattern to validate the timestamp before conversion:
+ const timestamp = typeof data.origin_server_ts === 'number'
+ ? new Date(data.origin_server_ts)
+ : new Date();
+
await Message.saveMessageFromFederation({
fromId: user._id,
rid: room._id,
msg: formatted,
federation_event_id: data.event_id,
thread,
- ts: new Date(data.origin_server_ts),
+ ts: timestamp,
});Apply this pattern to all 5 locations where ts: new Date(data.origin_server_ts) is used (lines 227, 245, 259, 362, 376).
Also applies to: 245-245, 259-259, 362-362, 376-376
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 227, 245,
259, 362, and 376, the code constructs dates using new
Date(data.origin_server_ts) without validating the timestamp; change each
occurrence to first coerce and validate the timestamp (e.g. const tsVal =
Number(data.origin_server_ts); if (!Number.isFinite(tsVal)) handle invalid case)
and then use new Date(tsVal) only when valid, otherwise set ts to null/undefined
or throw an explicit error depending on caller expectations; apply this exact
validation pattern to all five locations so Invalid Date values are never
produced.
🧩 Analysis chain
Verify timestamp validation doesn't reject legitimate federation messages.
The origin_server_ts is converted to a Date and passed to executeSendMessage, which validates timestamps and throws an error if they're more than 60 seconds out of sync with the current time (see apps/meteor/app/lib/server/methods/sendMessage.ts lines 33-40). Federation messages during catchup, replay, or network delays may legitimately have timestamps older than 60 seconds, potentially causing a 500 Internal Server Error.
Consider whether federation messages should bypass or have relaxed timestamp validation, especially given the PR objective to fix "can't add local users to remote rooms" with 500 errors.
Run the following script to check how timestamp validation errors are handled in federation flows:
Also applies to: 245-245, 259-259, 362-362, 376-376
🏁 Script executed:
#!/bin/bash
# Description: Search for error handling around executeSendMessage in federation code
# Check how federation message saves handle timestamp validation errors
rg -n -A 10 -B 3 'saveMessageFromFederation|executeSendMessage' --type ts -g '*federation*'
# Look for error-message-ts-out-of-sync handling
rg -n -A 5 'error-message-ts-out-of-sync' --type tsLength of output: 543
Relax timestamp validation for federation messages
The executeSendMessage method (apps/meteor/app/lib/server/methods/sendMessage.ts:38) throws a 500 error when origin_server_ts is > 60 s behind the server clock (“error-message-ts-out-of-sync”), blocking federation catch-up and causing the 500s seen in “can’t add local users to remote rooms.” Bypass or widen this check for federation imports.
…ederation event handling" This reverts commit 3d6816d.
https://rocketchat.atlassian.net/browse/FDR-232
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features
Chores