Skip to content

fix: include banned and invited servers in event destination set #390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ggazzo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: include banned and invited servers in event destination set #390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c978a22
fix: include banned and invited servers in event destination set
ggazzo Mar 24, 2026
4136d6b
test: update getServerSetInRoom test to expect banned/invited servers
ggazzo Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/federation-sdk/src/services/state.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2262,7 +2262,7 @@ describe('StateService', async () => {
expect(servers.size).toBe(1);
});

it('should exclude servers with non-joined members', async () => {
it('should include servers with banned or invited members but exclude left', async () => {
const { roomCreateEvent } = await createRoom('public');

const creator = '@alice:example.com'; // Room creator with admin permissions
Expand All @@ -2284,9 +2284,9 @@ describe('StateService', async () => {

expect(servers.has('joined.com')).toBe(true);
expect(servers.has('left.com')).toBe(false);
expect(servers.has('banned.com')).toBe(false);
expect(servers.has('invited.com')).toBe(false);
expect(servers.size).toBe(2); // example.com (creator) + joined.com
expect(servers.has('banned.com')).toBe(true);
expect(servers.has('invited.com')).toBe(true);
expect(servers.size).toBe(4); // example.com (creator) + joined.com + banned.com + invited.com
});

it('should return creator server for room with only creator', async () => {
Expand Down
4 changes: 3 additions & 1 deletion packages/federation-sdk/src/services/state.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
if (!event.isMembershipEvent() || event.getMembership() !== 'join') {
if (!event.isMembershipEvent() || !residentMemberships.has(event.getMembership() ?? '')) {
continue;
Comment on lines +700 to 704
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Scope ban inclusion to federation destination usage; current change can authorize banned servers.

getServerSetInRoom is also used by EventAuthorizationService.serverHasAccessToResource (packages/federation-sdk/src/services/event-authorization.service.ts:254-256), where membership in this set immediately grants access. Including ban here can let banned servers pass authorization checks.

🔧 Proposed fix (make memberships explicit per call site) 
- async getServerSetInRoom(roomId: RoomID, roomState?: State) {
+ async getServerSetInRoom(
+   roomId: RoomID,
+   roomState?: State,
+   residentMemberships: ReadonlySet<string> = new Set(['join']),
+ ) {
   const state = roomState ?? (await this.getLatestRoomState(roomId));

   const servers = new Set<string>();

-  const residentMemberships = new Set(['join', 'invite', 'ban']);
-
   for (const event of state.values()) {
     if (!event.isMembershipEvent() || !residentMemberships.has(event.getMembership() ?? '')) {
       continue;
     }

Then only federation fanout paths that require state-chain continuity should pass:
new Set(['join', 'invite', 'ban']).

}

Expand Down
Loading