fix: handle federation events for banned/invited users#384
fix: handle federation events for banned/invited users#384
Conversation
WalkthroughAdds documentation describing two federated Matrix-room edge cases and implements corresponding fixes: (1) emitting membership rejection events when m.room.member validation fails due to missing room state, and (2) extending server set routing to include servers with users in ban and invite membership states. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
…vited users When a server receives a membership event (e.g., ban) for a room where only an invite exists, event validation fails because no m.room.create event is stored locally. Previously this silently discarded the event, leaving the invite stuck. Now emits a `homeserver.matrix.membership.rejected` event so consumers can handle the case (e.g., clean up the pending invite). Includes documentation on the partial room state gap compared to Synapse.
After banning a user, their server is excluded from event destinations because getServerSetInRoom only considers 'join' membership. This causes the unban (leave) event to never reach the remote server, breaking the state chain and preventing re-invites.
getServerSetInRoom only considered servers with 'join' membership, excluding servers whose users were banned or had pending invites. This caused unban (leave) events to never reach the banned user's server, breaking the state chain and preventing re-invites. Include 'ban' and 'invite' memberships so these servers continue receiving room events needed to maintain a consistent state.
ggazzo
force-pushed
the
fix/ban-invited-user-federation
branch
from
dc3addc to
91e5cc4
Compare
The test previously asserted that banned and invited servers were excluded from the destination set. Updated to match the new behavior where these servers are included to ensure they receive room events.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
==========================================
+ Coverage 50.37% 50.83% +0.46%
==========================================
Files 99 101 +2
Lines 11336 11477 +141
==========================================
+ Hits 5710 5834 +124
- Misses 5626 5643 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/federation-sdk/src/services/state.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/state.service.ts:700">
P1: `getServerSetInRoom` now includes `ban`/`invite` servers globally, which broadens EDU fanout and access authorization to non-joined servers.</violation>
</file>
<file name="packages/federation-sdk/src/services/event.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/event.service.ts:171">
P1: Do not let membership-rejection handler failures abort PDU processing; guard the emitted hook so validation failures still get skipped.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const servers = new Set<string>(); | ||
|
|
||
| const residentMemberships = new Set(['join', 'invite', 'ban']); |
There was a problem hiding this comment.
P1: getServerSetInRoom now includes ban/invite servers globally, which broadens EDU fanout and access authorization to non-joined servers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/state.service.ts, line 700:
<comment>`getServerSetInRoom` now includes `ban`/`invite` servers globally, which broadens EDU fanout and access authorization to non-joined servers.</comment>
<file context>
@@ -697,8 +697,10 @@ export class StateService {
const servers = new Set<string>();
+ const residentMemberships = new Set(['join', 'invite', 'ban']);
+
for (const event of state.values()) {
</file context>
| await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', { | ||
| event, | ||
| reason: err.message, | ||
| }); |
There was a problem hiding this comment.
P1: Do not let membership-rejection handler failures abort PDU processing; guard the emitted hook so validation failures still get skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/event.service.ts, line 171:
<comment>Do not let membership-rejection handler failures abort PDU processing; guard the emitted hook so validation failures still get skipped.</comment>
<file context>
@@ -167,12 +167,19 @@ export class EventService {
- err,
- });
+ if (err instanceof Error && err.name === 'UnknownRoomError' && event.type === 'm.room.member') {
+ await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', {
+ event,
+ reason: err.message,
</file context>
| await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', { | |
| event, | |
| reason: err.message, | |
| }); | |
| try { | |
| await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', { | |
| event, | |
| reason: err.message, | |
| }); | |
| } catch (emitErr) { | |
| this.logger.error({ | |
| msg: 'Failed to emit membership rejection event', | |
| origin, | |
| event, | |
| err: emitErr, | |
| }); | |
| } |
![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)
docs/partial-room-state-for-invites.md (1)
5-26: Optional wording polish: prefer “invitation” in prose.For formal documentation text, “invitation” reads cleaner than noun-form “invite” (keep protocol field names like
invite_room_stateunchanged).Also applies to: 74-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/partial-room-state-for-invites.md` around lines 5 - 26, The document uses the noun "invite" in prose; change these prose occurrences to "invitation" while keeping protocol field and event names unchanged (e.g., preserve `invite_room_state`, `m.room.create`, `m.room.join_rules` as-is). Update all prose mentions throughout the file (including the section describing Synapse behavior and any other occurrences) to use "invitation" for readability, but do not modify code/protocol identifiers or example event names.
🤖 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/event.service.ts`:
- Around line 170-175: The UnknownRoomError branch currently calls
this.eventEmitterService.emit('homeserver.matrix.membership.rejected', ...)
directly which can throw and bubble up, aborting PDU processing; wrap that emit
call in its own try/catch so any exceptions from listeners are swallowed or
logged (e.g. via this.logger.warn/error) and do not rethrow, ensuring processing
continues; change the block inside the if that calls eventEmitterService.emit to
catch and handle errors locally while preserving the existing payload and
control flow.
---
Nitpick comments:
In `@docs/partial-room-state-for-invites.md`:
- Around line 5-26: The document uses the noun "invite" in prose; change these
prose occurrences to "invitation" while keeping protocol field and event names
unchanged (e.g., preserve `invite_room_state`, `m.room.create`,
`m.room.join_rules` as-is). Update all prose mentions throughout the file
(including the section describing Synapse behavior and any other occurrences) to
use "invitation" for readability, but do not modify code/protocol identifiers or
example event names.
🪄 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: 00207b9b-d2bd-4868-a633-327b25a42481
📒 Files selected for processing (5)
docs/partial-room-state-for-invites.mdpackages/federation-sdk/src/index.tspackages/federation-sdk/src/services/event.service.tspackages/federation-sdk/src/services/state.service.spec.tspackages/federation-sdk/src/services/state.service.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
📚 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/event.service.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/state.service.spec.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
docs/partial-room-state-for-invites.md
🪛 LanguageTool
docs/partial-room-state-for-invites.md
[style] ~5-~5: The noun “invitation” is usually used instead of ‘invite’ in formal writing.
Context: ...rgeting users who have only received an invite but have not yet joined the room. This ...
(AN_INVITE)
[style] ~26-~26: The noun “invitation” is usually used instead of ‘invite’ in formal writing.
Context: ...s where the server has only received an invite. When a server receives an invite, the ...
(AN_INVITE)
[style] ~26-~26: The noun “invitation” is usually used instead of ‘invite’ in formal writing.
Context: ...ed an invite. When a server receives an invite, the unsigned.invite_room_state field...
(AN_INVITE)
[style] ~74-~74: The noun “invitation” is usually used instead of ‘invite’ in formal writing.
Context: ...tries to re-invite the user — builds an invite event whose prev_events reference the...
(AN_INVITE)
🔇 Additional comments (3)
packages/federation-sdk/src/index.ts (1)
127-130: Typed event contract addition looks good.The new
homeserver.matrix.membership.rejectedsignature is clear and aligns well with the event payload shape used by the emitter flow.packages/federation-sdk/src/services/state.service.spec.ts (1)
2265-2290: Good coverage for the membership-set expansion scenario.This test cleanly validates the intended
join + invite + baninclusion while still excludingleave.packages/federation-sdk/src/services/state.service.ts (1)
700-704: This membership expansion was intentionally added per commit 50be08a.The change to include
inviteandbanstates in resident memberships is documented in the commit message as intentional: "Updated to match the new behavior where these servers are included to ensure they receive room events." The accompanying test confirms this expectation (lines 2287-2288 assert that banned.com and invited.com are included).If you believe the spec or security requirements should exclude invited/banned servers from receiving ephemeral events (typing, presence, receipts), that's a valid design concern—but it should be addressed by reconsidering the intentional feature, not as an unintended side effect.
> Likely an incorrect or invalid review comment.
| if (err instanceof Error && err.name === 'UnknownRoomError' && event.type === 'm.room.member') { | ||
| await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', { | ||
| event, | ||
| reason: err.message, | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Guard the rejection emit path so listener failures don’t abort PDU processing.
If eventEmitterService.emit(...) throws, this catch branch now bubbles and can fail the whole transaction. Validation failures here should remain fail-soft and continue.
💡 Suggested hardening
- if (err instanceof Error && err.name === 'UnknownRoomError' && event.type === 'm.room.member') {
- await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', {
- event,
- reason: err.message,
- });
+ if (err instanceof Error && err.name === 'UnknownRoomError' && event.type === 'm.room.member') {
+ try {
+ await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', {
+ event,
+ reason: err.message,
+ });
+ } catch (emitErr) {
+ this.logger.error({
+ msg: 'Failed to emit membership rejection event',
+ origin,
+ event,
+ err: emitErr,
+ });
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (err instanceof Error && err.name === 'UnknownRoomError' && event.type === 'm.room.member') { | |
| await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', { | |
| event, | |
| reason: err.message, | |
| }); | |
| } else { | |
| if (err instanceof Error && err.name === 'UnknownRoomError' && event.type === 'm.room.member') { | |
| try { | |
| await this.eventEmitterService.emit('homeserver.matrix.membership.rejected', { | |
| event, | |
| reason: err.message, | |
| }); | |
| } catch (emitErr) { | |
| this.logger.error({ | |
| msg: 'Failed to emit membership rejection event', | |
| origin, | |
| event, | |
| err: emitErr, | |
| }); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/event.service.ts` around lines 170 -
175, The UnknownRoomError branch currently calls
this.eventEmitterService.emit('homeserver.matrix.membership.rejected', ...)
directly which can throw and bubble up, aborting PDU processing; wrap that emit
call in its own try/catch so any exceptions from listeners are swallowed or
logged (e.g. via this.logger.warn/error) and do not rethrow, ensuring processing
continues; change the block inside the if that calls eventEmitterService.emit to
catch and handle errors locally while preserving the existing payload and
control flow.
There was a problem hiding this comment.
this document could actually be two issues right? they don't have value as "documentation"
3 participants
Summary
Fixes two related issues where our homeserver fails to properly route and process federation events involving banned or invited users.
Bug 1: Banning a user who has a pending invite
When a user is banned before accepting a federation invite, the remote server cannot process the ban event because it has no room state (only an outlier invite event). The SDK's
validateEventrequires them.room.createevent to determine the room version, which doesn't exist on a server that never joined the room. The ban event is silently discarded and the invite gets stuck in the UI.Fix: Emit a new
homeserver.matrix.membership.rejectedevent whenvalidateEventfails withUnknownRoomErrorfor membership events, so the application layer can clean up the stale invite.Bug 2: Re-inviting a user after ban+unban cycle
After banning and unbanning a user, re-inviting them fails with a 500 error.
getServerSetInRoomonly includes servers withjoinmembership when computing event destinations. After a ban, the user's membership changes toban, so their server is excluded from the destination set. The unban (leave) event is never delivered to the remote server, breaking the state chain. When a new invite arrives, the remote server cannot resolveprev_eventsit never received.Fix: Include
banandinvitememberships ingetServerSetInRoomso these servers continue receiving room events.Documentation
Added
docs/partial-room-state-for-invites.mddocumenting both issues, the gap compared to Synapse (which stores partial room state frominvite_room_state), and a proposed long-term solution.Summary by CodeRabbit
Bug Fixes
Documentation