Skip to content

fix: change create DM room generic call to newCreateEvent #205

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

Merged
ggazzo merged 1 commit into from
Sep 19, 2025
Merged

fix: change create DM room generic call to newCreateEvent #205

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 2 additions & 15 deletions packages/federation-sdk/src/services/room.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1194,21 +1194,8 @@ export class RoomService {

const stateService = this.stateService;

const roomCreateEvent = PersistentEventFactory.newEvent<'m.room.create'>(
{
type: 'm.room.create',
content: {
room_version: PersistentEventFactory.defaultRoomVersion,
creator: creatorUserId,
},
room_id: '',
state_key: '',
auth_events: [],
depth: 0,
prev_events: [],
origin_server_ts: Date.now(),
sender: creatorUserId,
},
const roomCreateEvent = PersistentEventFactory.newCreateEvent(
creatorUserId,
PersistentEventFactory.defaultRoomVersion,
);
Comment on lines +1197 to 1200
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 19, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

🧩 Analysis chain

Swap to PersistentEventFactory.newCreateEvent looks correct; add a guard to ensure local creator and avoid bad roomId domains

newCreateEvent derives the roomId domain from creatorUserId. If creatorUserId is not local or contains a port/IPv6 (e.g., @U:example.org:8448), we may create a room hosted on the wrong “resident” server, which breaks subsequent logic that assumes a locally hosted room. Add a defensive check before creating the room.

Apply near this call:

@@
-    const stateService = this.stateService;
+    const stateService = this.stateService;
+    // Ensure the DM is created on this server; prevents room_id domain mismatch
+    const creatorServerName = creatorUserId.split(':').slice(1).join(':');
+    if (creatorServerName !== localServerName) {
+      throw new HttpException('Creator must be a local user to create a DM room on this server', HttpStatus.BAD_REQUEST);
+    }
 
-    const roomCreateEvent = PersistentEventFactory.newCreateEvent(
+    const roomCreateEvent = PersistentEventFactory.newCreateEvent(
       creatorUserId,
       PersistentEventFactory.defaultRoomVersion,
     );

Optional follow‑up: consider extending newCreateEvent to accept an explicit domain/serverName to avoid inferring it from a userId string.

Run to verify no remaining call sites use newEvent<'m.room.create'> and to inspect DM room creation call sites:

🏁 Script executed:

#!/bin/bash
rg -nP --type=ts "PersistentEventFactory\.newEvent<\s*'m\.room\.create'"
rg -n --type=ts -C3 "createDirectMessageRoom\("

Length of output: 1350

🏁 Script executed:

#!/bin/bash
# Show the implementation of createDirectMessageRoom to confirm where newCreateEvent is called and context variables
awk 'NR>=1170 && NR<=1250' packages/federation-sdk/src/services/room.service.ts || echo "File not found"

Length of output: 2083

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "Searching for PersistentEventFactory..."
rg -n --hidden "PersistentEventFactory" || true

echo
echo "Locating file that defines the factory..."
file=$(rg -n --hidden "export class PersistentEventFactory|class PersistentEventFactory" --hidden --files-with-matches | head -n1 || true)
echo "Found: $file"
if [ -n "$file" ]; then
  echo
  echo "Showing file contents around newCreateEvent..."
  rg -n "newCreateEvent" "$file" -n -N -S || true
  sed -n '1,240p' "$file"
fi

Length of output: 11441

Require creator to be local before calling PersistentEventFactory.newCreateEvent

newCreateEvent derives the room_id domain from the creator string (packages/room/src/manager/factory.ts uses creator.split(':').pop()). createDirectMessageRoom calls it without verifying the creator is local (packages/federation-sdk/src/services/room.service.ts ≈ line 1197) — this can produce a room hosted on the wrong server. Add a defensive guard.

@@
-    const stateService = this.stateService;
+    const stateService = this.stateService;
+    // Ensure the DM is created on this server; prevents room_id domain mismatch
+    const creatorServerName = creatorUserId.split(':').slice(1).join(':');
+    if (creatorServerName !== localServerName) {
+      throw new HttpException('Creator must be a local user to create a DM room on this server', HttpStatus.BAD_REQUEST);
+    }
 
-    const roomCreateEvent = PersistentEventFactory.newCreateEvent(
+    const roomCreateEvent = PersistentEventFactory.newCreateEvent(
       creatorUserId,
       PersistentEventFactory.defaultRoomVersion,
     );

Optional: extend newCreateEvent to accept an explicit serverName to avoid inferring the domain.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In packages/federation-sdk/src/services/room.service.ts around lines 1197-1200,
the code calls PersistentEventFactory.newCreateEvent(creatorUserId, ...) without
ensuring the creator is a local user, which lets newCreateEvent infer the
room_id domain from a non-local creator and can produce a room hosted on the
wrong server; add a defensive guard that verifies the creatorUserId is local
(use the existing isLocalUser/isLocalMxid utility or compare the MXID domain to
the local server name) and throw or return an error if not local before calling
newCreateEvent, or alternatively refactor to call newCreateEvent with an
explicit serverName parameter (update factory signature and call sites) so the
room domain is not inferred from the creator.


Expand Down