fix: dont send transaction to invited server#393
Conversation
WalkthroughUpdated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
=======================================
Coverage 50.80% 50.80%
=======================================
Files 101 101
Lines 11482 11484 +2
=======================================
+ Hits 5833 5835 +2
Misses 5649 5649 ☔ View full report in Codecov by Sentry. 🚀 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: 1
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/federation.service.ts (1)
208-211: Consider using aSetfor omitted-destination checks.
omitDestinations.includes(server)is linear per destination. Converting once to aSetmakes this path simpler and keeps lookups constant-time.♻️ Suggested refactor
async sendEventToAllServersInRoom(event: PersistentEventBase, omitDestinations: string[] = []): Promise<void> { + const omitted = new Set(omitDestinations); // TODO we need a map of rooms and destinations to avoid having to get rooms state just to send an event to all servers in the room. const servers = await this.stateService.getServerSetInRoom(event.roomId); @@ // Filter out the event origin, local server, and any additional omitted destinations const destinations = Array.from(servers).filter( - (server) => server !== event.origin && server !== this.configService.serverName && !omitDestinations.includes(server), + (server) => server !== event.origin && server !== this.configService.serverName && !omitted.has(server), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/federation.service.ts` around lines 208 - 211, Summary: Replace the linear includes check with a Set lookup to make omitted-destination checks constant-time. Fix: before building destinations, create a Set from omitDestinations (e.g., const omitDestinationsSet = new Set(omitDestinations)) and then change the filter on servers (the destinations computation) to use !omitDestinationsSet.has(server) instead of !omitDestinations.includes(server); keep the other checks (server !== event.origin and server !== this.configService.serverName) unchanged and preserve types/signatures around destinations and servers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/invite.service.ts`:
- Around line 128-129: The code is broadcasting and returning the singly-signed
inviteEvent instead of the doubly-signed inviteResponse.event that was
persisted; change uses of inviteEvent in
federationService.sendEventToAllServersInRoom(...) and the function return to
use inviteResponse.event (the event returned from the invited HS signing) so the
broadcasted/returned event is the doubly-signed version required by the SPEC;
update any local variables or parameters that expect inviteEvent accordingly
(e.g., replace sending/returning inviteEvent with inviteResponse.event and
ensure inviteResponse.event is passed to sendEventToAllServersInRoom and the
function's return value).
---
Nitpick comments:
In `@packages/federation-sdk/src/services/federation.service.ts`:
- Around line 208-211: Summary: Replace the linear includes check with a Set
lookup to make omitted-destination checks constant-time. Fix: before building
destinations, create a Set from omitDestinations (e.g., const
omitDestinationsSet = new Set(omitDestinations)) and then change the filter on
servers (the destinations computation) to use !omitDestinationsSet.has(server)
instead of !omitDestinations.includes(server); keep the other checks (server !==
event.origin and server !== this.configService.serverName) unchanged and
preserve types/signatures around destinations and servers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1eaadb27-6eaf-4641-a4d7-2f925bc6759b
📒 Files selected for processing (2)
packages/federation-sdk/src/services/federation.service.tspackages/federation-sdk/src/services/invite.service.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/federation.service.ts
🔇 Additional comments (1)
packages/federation-sdk/src/services/federation.service.ts (1)
197-225: Good change: destination omission is scoped cleanly without breaking existing callers.The optional
omitDestinationswith default[]preserves old behavior and enables the invite-specific exclusion flow cleanly.
| // let everyone else know | ||
| void federationService.sendEventToAllServersInRoom(inviteEvent, [invitedServer]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Matrix federation v2 invite flow, when the inviting server relays the invite event to other room servers, must the relayed event include both inviter and invitee server signatures?
💡 Result:
Yes, in the Matrix federation v2 invite flow, when the inviting server relays the invite event to other room servers (via /send), the relayed event must include both the inviter server signature (origin server) and the invitee server signature.
Citations:
- 1: https://spec.matrix.org/v1.6/server-server-api/
- 2: Add a note to the federation invite endpoints that invites can be sent twice matrix-org/matrix-spec#2067
- 3: Document that incoming invite events will reach a homeserver twice - once over /invite and again over /send matrix-org/matrix-spec#2062
- 4: https://spec.matrix.org/v1.15/server-server-api
🏁 Script executed:
cat -n packages/federation-sdk/src/services/invite.service.ts | sed -n '115,145p'Repository: RocketChat/homeserver
Length of output: 1168
🏁 Script executed:
cat -n packages/federation-sdk/src/services/invite.service.ts | sed -n '80,125p'Repository: RocketChat/homeserver
Length of output: 1892
🏁 Script executed:
cat -n packages/federation-sdk/src/services/invite.service.ts | sed -n '50,100p'Repository: RocketChat/homeserver
Length of output: 1979
🏁 Script executed:
cat -n packages/federation-sdk/src/services/invite.service.ts | sed -n '50,60p'Repository: RocketChat/homeserver
Length of output: 428
Broadcast and return the doubly-signed invite event, not the single-signed one.
On Line 126 you persist inviteResponse.event, but on Lines 129 and 132-133 you broadcast and return inviteEvent. The SPEC comment on Line 97 explicitly requires the event to be "signed by both the inviting homeserver and the invited homeserver" before being sent to all servers. Broadcasting the single-signed inviteEvent violates the Matrix specification and will cause federation rejection.
🐛 Proposed fix
- await stateService.handlePdu(PersistentEventFactory.createFromRawEvent(inviteResponse.event, roomVersion));
+ const signedInviteEvent = PersistentEventFactory.createFromRawEvent(inviteResponse.event, roomVersion);
+ await stateService.handlePdu(signedInviteEvent);
// let everyone else know
- void federationService.sendEventToAllServersInRoom(inviteEvent, [invitedServer]);
+ void federationService.sendEventToAllServersInRoom(signedInviteEvent, [invitedServer]);
return {
- event_id: inviteEvent.eventId,
- event: PersistentEventFactory.createFromRawEvent(inviteEvent.event, roomVersion),
+ event_id: signedInviteEvent.eventId,
+ event: signedInviteEvent,
room_id: roomId,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/invite.service.ts` around lines 128 -
129, The code is broadcasting and returning the singly-signed inviteEvent
instead of the doubly-signed inviteResponse.event that was persisted; change
uses of inviteEvent in federationService.sendEventToAllServersInRoom(...) and
the function return to use inviteResponse.event (the event returned from the
invited HS signing) so the broadcasted/returned event is the doubly-signed
version required by the SPEC; update any local variables or parameters that
expect inviteEvent accordingly (e.g., replace sending/returning inviteEvent with
inviteResponse.event and ensure inviteResponse.event is passed to
sendEventToAllServersInRoom and the function's return value).
3 participants
As per Matrix Spec:
https://spec.matrix.org/v1.18/server-server-api/#pdus
FGA-58
Summary by CodeRabbit