refactor: replace string types with branded types for RoomID, UserID,… #243
Conversation
WalkthroughReplaces many string identifiers with branded types (RoomID, UserID, EventID) across services, controllers, schemas, and factories; changes one core type guard to return boolean; removes some explicit casts in staging emissions. No runtime control-flow changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/federation-sdk/src/utils/event-schemas.ts (1)
🔇 Additional comments (6)
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 |
ggazzo
force-pushed
the
chore/more-branded-types
branch
2 times, most recently
from
2cc72ce to
3fb816f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #243 +/- ##
==========================================
- Coverage 81.86% 81.84% -0.02%
==========================================
Files 63 63
Lines 4709 4704 -5
==========================================
- Hits 3855 3850 -5
Misses 854 854 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… and EventID across services and schemas
ggazzo
force-pushed
the
chore/more-branded-types
branch
from
3fb816f to
1e827f3
Compare
packages/homeserver/src/controllers/federation/rooms.controller.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Diego Sampaio <chinello@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/homeserver/src/controllers/internal/direct-message.controller.ts (1)
9-10: Integrate userIdSchema into DTO schemaImport
userIdSchemafrom@rocket.chat/federation-room(it’s re-exported inpackages/room/src/index.ts) and replace botht.String()fields with transforms that enforce the branded format:+import { userIdSchema } from '@rocket.chat/federation-room'; export const InternalCreateDirectMessageBodyDto = t.Object({ - senderUserId: t.String(), - targetUserId: t.String(), + senderUserId: t.Transform(t.String()) + .Decode((value) => { + const result = userIdSchema.safeParse(value); + if (!result.success) throw new Error('Invalid senderUserId format'); + return result.data; + }) + .Encode((value) => value), + targetUserId: t.Transform(t.String()) + .Decode((value) => { + const result = userIdSchema.safeParse(value); + if (!result.success) throw new Error('Invalid targetUserId format'); + return result.data; + }) + .Encode((value) => value), });This enforces UserID validation at the API boundary—no custom validator needed.
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
56-59: Validate event ID format before casting.Consistent with other endpoints, validate the event ID before casting to the branded type.
+import { eventIdSchema } from '@rocket.chat/federation-room'; + .get( '/_matrix/federation/v1/event/:eventId', async ({ params, set }) => { + const validatedEventId = eventIdSchema.parse(params.eventId); const eventData = await eventService.getEventById( - params.eventId as EventID, + validatedEventId, );packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
113-120: Validate ID formats and arrays before casting.This endpoint casts
roomIdand two event ID arrays without validation. Array casts are particularly risky.+import { eventIdSchema, roomIdSchema } from '@rocket.chat/federation-room'; +import { z } from 'zod'; + .post( '/_matrix/federation/v1/get_missing_events/:roomId', async ({ params, body }) => { + const validatedRoomId = roomIdSchema.parse(params.roomId); + const validatedEarliestEvents = z.array(eventIdSchema).parse(body.earliest_events); + const validatedLatestEvents = z.array(eventIdSchema).parse(body.latest_events); + return profilesService.getMissingEvents( - params.roomId as RoomID, - body.earliest_events as EventID[], - body.latest_events as EventID[], + validatedRoomId, + validatedEarliestEvents, + validatedLatestEvents, body.limit, body.min_depth, ),packages/homeserver/src/controllers/internal/room.controller.ts (1)
378-402: Validate IDs before casting in ban implementation.The active ban implementation casts multiple IDs (
roomId,userIdToBan,senderUserId) without validation. This is particularly important since these IDs are used to construct events that affect user permissions.+import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room'; + const { roomId, userIdToBan } = params; const { senderUserId } = body; + + const validatedRoomId = roomIdSchema.parse(roomId); + const validatedUserIdToBan = userIdSchema.parse(userIdToBan); + const validatedSenderUserId = userIdSchema.parse(senderUserId); - const room = await stateService.getFullRoomState(roomId as RoomID); + const room = await stateService.getFullRoomState(validatedRoomId); // ... create event code ... const membershipEvent = PersistentEventFactory.newEvent<'m.room.member'>( { type: 'm.room.member', content: { membership: 'ban' }, - room_id: roomId as RoomID, - state_key: userIdToBan as UserID, + room_id: validatedRoomId, + state_key: validatedUserIdToBan, auth_events: [], depth: 0, prev_events: [], origin_server_ts: Date.now(), - sender: senderUserId as UserID, + sender: validatedSenderUserId, },
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/message.service.ts (1)
302-306: Return EventID-branded valuesNow that these methods accept branded IDs, keeping the return types as
Promise<string>drops the branding right away. Please returnPromise<EventID>so callers preserve the stronger typing.- async sendReaction( - roomId: RoomID, - eventId: EventID, - emoji: string, - senderUserId: UserID, - ): Promise<string> { + async sendReaction( + roomId: RoomID, + eventId: EventID, + emoji: string, + senderUserId: UserID, + ): Promise<EventID> { … - async unsetReaction( - roomId: RoomID, - eventIdReactedTo: EventID, - _emoji: string, - senderUserId: UserID, - ): Promise<string> { + async unsetReaction( + roomId: RoomID, + eventIdReactedTo: EventID, + _emoji: string, + senderUserId: UserID, + ): Promise<EventID> { … - async updateMessage( - roomId: RoomID, - rawMessage: string, - formattedMessage: string, - senderUserId: UserID, - eventIdToReplace: EventID, - ): Promise<string> { + async updateMessage( + roomId: RoomID, + rawMessage: string, + formattedMessage: string, + senderUserId: UserID, + eventIdToReplace: EventID, + ): Promise<EventID> { … - async redactMessage( - roomId: RoomID, - eventIdToRedact: EventID, - ): Promise<string> { + async redactMessage( + roomId: RoomID, + eventIdToRedact: EventID, + ): Promise<EventID> {Also applies to: 347-351, 379-385, 424-427
📜 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 (22)
packages/core/src/events/isRoomMemberEvent.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/message.service.ts(10 hunks)packages/federation-sdk/src/services/profiles.service.ts(4 hunks)packages/federation-sdk/src/services/room.service.ts(15 hunks)packages/federation-sdk/src/services/send-join.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(3 hunks)packages/federation-sdk/src/utils/event-schemas.ts(5 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(6 hunks)packages/homeserver/src/controllers/federation/rooms.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/send-join.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/state.controller.ts(3 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(2 hunks)packages/homeserver/src/controllers/internal/direct-message.controller.ts(2 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(3 hunks)packages/homeserver/src/controllers/internal/message.controller.ts(5 hunks)packages/homeserver/src/controllers/internal/room.controller.ts(12 hunks)packages/room/src/manager/event-wrapper.ts(1 hunks)packages/room/src/manager/factory.ts(3 hunks)packages/room/src/types/_common.ts(1 hunks)packages/room/src/types/v3-11.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (19)
packages/room/src/manager/factory.ts (3)
packages/room/src/manager/room-state.ts (1)
creator(21-31)packages/room/src/types/_common.ts (2)
UserID(16-16)roomIdSchema(10-10)packages/room/src/manager/event-wrapper.ts (1)
roomId(78-80)
packages/homeserver/src/controllers/federation/send-join.controller.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
roomId(78-80)packages/room/src/types/_common.ts (2)
RoomID(12-12)EventID(8-8)packages/room/src/manager/v3.ts (1)
eventId(17-29)
packages/core/src/events/isRoomMemberEvent.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(106-115)packages/room/src/types/v3-11.ts (1)
Pdu(728-728)
packages/federation-sdk/src/services/send-join.service.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(106-115)packages/room/src/types/_common.ts (2)
EventID(8-8)PduForType(22-22)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/room/src/types/_common.ts (3)
UserID(16-16)RoomID(12-12)EventID(8-8)packages/room/src/manager/v3.ts (1)
eventId(17-29)
packages/homeserver/src/controllers/internal/room.controller.ts (1)
packages/room/src/types/_common.ts (2)
UserID(16-16)RoomID(12-12)
packages/homeserver/src/controllers/federation/invite.controller.ts (2)
packages/room/src/types/_common.ts (2)
RoomID(12-12)EventID(8-8)packages/room/src/manager/v3.ts (1)
eventId(17-29)
packages/federation-sdk/src/services/profiles.service.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
roomId(78-80)packages/room/src/types/_common.ts (4)
RoomID(12-12)UserID(16-16)PduForType(22-22)EventID(8-8)
packages/federation-sdk/src/services/staging-area.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(106-115)
packages/homeserver/src/controllers/internal/invite.controller.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(78-80)sender(82-84)packages/room/src/types/_common.ts (2)
RoomID(12-12)UserID(16-16)packages/room/src/manager/power-level-event-wrapper.ts (1)
sender(190-196)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
packages/room/src/types/_common.ts (1)
RoomID(12-12)
packages/homeserver/src/controllers/internal/direct-message.controller.ts (1)
packages/room/src/types/_common.ts (1)
UserID(16-16)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (3)
eventIdSchema(6-6)roomIdSchema(10-10)userIdSchema(14-14)
packages/homeserver/src/controllers/internal/message.controller.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
roomId(78-80)packages/room/src/types/_common.ts (3)
RoomID(12-12)UserID(16-16)EventID(8-8)
packages/federation-sdk/src/utils/event-schemas.ts (1)
packages/room/src/types/_common.ts (3)
userIdSchema(14-14)roomIdSchema(10-10)eventIdSchema(6-6)
packages/federation-sdk/src/services/message.service.ts (2)
packages/room/src/types/_common.ts (3)
RoomID(12-12)UserID(16-16)EventID(8-8)packages/core/src/events/m.room.message.ts (1)
FileMessageContent(28-45)
packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
packages/room/src/types/_common.ts (3)
UserID(16-16)RoomID(12-12)EventID(8-8)
packages/federation-sdk/src/services/room.service.ts (3)
packages/room/src/types/_common.ts (3)
UserID(16-16)RoomID(12-12)EventID(8-8)packages/room/src/manager/event-wrapper.ts (2)
roomId(78-80)sender(82-84)packages/federation-sdk/src/index.ts (1)
EventID(23-23)
packages/homeserver/src/controllers/federation/state.controller.ts (1)
packages/room/src/types/_common.ts (2)
RoomID(12-12)EventID(8-8)
🔇 Additional comments (7)
packages/federation-sdk/src/services/staging-area.service.ts (4)
237-237: LGTM! Type cast removal is appropriate.Removing the explicit
as string | undefinedcast allows TypeScript to infer the type from the properly-typed schema. This aligns with the PR's goal of using branded types throughout the codebase.
248-248: LGTM! Unnecessary cast removed.The removal of
as stringis appropriate here. Form.room.memberevents,state_keyis always present and should now be properly typed (likely asUserIDbased on the PR's branded types refactor).
270-270: Confirm thatcontentis always defined form.room.topicevents
I couldn’t locate the PDU type for room-topic events in @rocket.chat/federation-core. Please verify that itscontentfield is declared as required (not optional); if it can be undefined, reintroduce optional chaining or add a runtime guard.
250-250: Verify external listeners handle full contentThe SDK now emits the full
event.event.contentform.room.memberinstead of an object with onlymembership. Ensure all consumers ofhomeserver.matrix.membershipcan process the complete content structure without breaking changes.packages/homeserver/src/controllers/internal/direct-message.controller.ts (1)
1-1: LGTM: Import aligns with branded type migration.The UserID import supports the controller's migration to strongly-typed IDs.
packages/federation-sdk/src/services/invite.service.ts (2)
38-47: LGTM! Service signature updates are appropriate.The service method signatures have been correctly updated to use branded types (
UserID,RoomID,EventID). This provides type safety at the service layer.However, ensure that all calling code (controllers) validates IDs before passing them to these methods—validation comments have been left on the controller files.
143-148: LGTM! Process invite signature updated correctly.The
processInvitemethod signature now usesRoomIDandEventIDbranded types, which improves type safety.As with
inviteUserToRoom, ensure controllers validate IDs before calling this method.
| export const isRoomMemberEvent = (event: Pdu): boolean => { | ||
| return event.type === 'm.room.member'; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify removal of type predicate doesn't break downstream type narrowing.
The function signature changed from a type predicate (event is RoomMemberEvent) to a plain boolean return. This removes compile-time type narrowing for callers who previously relied on TypeScript automatically treating event as RoomMemberEvent after this check.
While this may be intentional as part of the branded-types refactor, ensure that:
- Calling code that depended on the type predicate has been updated
- The Pdu type from
@rocket.chat/federation-roomnow provides sufficient type safety that makes the predicate redundant or incompatible - The simple
event.typecheck is sufficient given that RoomMemberEvent has additional required fields (content.membership, state_key, etc.)
Run this script to find usages and assess impact:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of isRoomMemberEvent to check if type narrowing was relied upon
# Search for isRoomMemberEvent usage with context
rg -n -C 5 'isRoomMemberEvent' --type tsLength of output: 3739
🏁 Script executed:
#!/bin/bash
sed -n '130,180p' packages/core/src/utils/pruneEventDict.tsLength of output: 1174
Restore type predicate on isRoomMemberEvent (packages/core/src/events/isRoomMemberEvent.ts:68)
Change the signature back to isRoomMemberEvent(event: Pdu): event is RoomMemberEvent so that TS correctly narrows callers (e.g. in pruneEventDict.ts) and drop the @ts-ignore annotations.
🤖 Prompt for AI Agents
In packages/core/src/events/isRoomMemberEvent.ts around lines 68 to 70, the
function currently returns boolean but should be a TypeScript type predicate to
allow proper narrowing; change the signature from (event: Pdu): boolean to
(event: Pdu): event is RoomMemberEvent, ensure RoomMemberEvent is imported or
referenced correctly, and remove any @ts-ignore annotations that were added
downstream (e.g., in pruneEventDict.ts) once callers can rely on the narrowed
type.
| return inviteService.processInvite( | ||
| body.event, | ||
| roomId, | ||
| eventId, | ||
| roomId as RoomID, | ||
| eventId as EventID, | ||
| body.room_version, | ||
| ); |
There was a problem hiding this comment.
Validate ID formats before casting to branded types.
Similar to send-join.controller.ts, this code casts route parameters to RoomID and EventID without runtime validation. The as assertions bypass format checks that branded types are meant to enforce.
Apply the same validation pattern as suggested for send-join.controller.ts:
+import { eventIdSchema, roomIdSchema } from '@rocket.chat/federation-room';
+
export const invitePlugin = (app: Elysia) => {
const inviteService = container.resolve(InviteService);
return app.put(
'/_matrix/federation/v2/invite/:roomId/:eventId',
async ({ body, params: { roomId, eventId } }) => {
+ const validatedRoomId = roomIdSchema.parse(roomId);
+ const validatedEventId = eventIdSchema.parse(eventId);
+
return inviteService.processInvite(
body.event,
- roomId as RoomID,
- eventId as EventID,
+ validatedRoomId,
+ validatedEventId,
body.room_version,
);
},📝 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.
| return inviteService.processInvite( | |
| body.event, | |
| roomId, | |
| eventId, | |
| roomId as RoomID, | |
| eventId as EventID, | |
| body.room_version, | |
| ); | |
| import { eventIdSchema, roomIdSchema } from '@rocket.chat/federation-room'; | |
| export const invitePlugin = (app: Elysia) => { | |
| const inviteService = container.resolve(InviteService); | |
| return app.put( | |
| '/_matrix/federation/v2/invite/:roomId/:eventId', | |
| async ({ body, params: { roomId, eventId } }) => { | |
| // Validate branded IDs at runtime | |
| const validatedRoomId = roomIdSchema.parse(roomId); | |
| const validatedEventId = eventIdSchema.parse(eventId); | |
| return inviteService.processInvite( | |
| body.event, | |
| validatedRoomId, | |
| validatedEventId, | |
| body.room_version, | |
| ); | |
| }, | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/invite.controller.ts around
lines 12 to 17, the code blindly casts route params to RoomID and EventID; add
runtime validation identical to send-join.controller.ts by checking the roomId
and eventId formats with the existing validator helpers (e.g., isValidRoomId /
isValidEventId or parse helpers) before casting; if validation fails, return a
400/BadRequest (with a brief message) instead of proceeding, and only then cast
to the branded types and call inviteService.processInvite with the validated
values.
| ({ query: { user_id } }) => | ||
| profilesService.queryProfile(user_id as UserID), |
There was a problem hiding this comment.
Validate user ID format before casting.
The queryProfile endpoint casts user_id from query parameters to UserID without validation.
+import { userIdSchema } from '@rocket.chat/federation-room';
+
.get(
'/_matrix/federation/v1/query/profile',
- ({ query: { user_id } }) =>
- profilesService.queryProfile(user_id as UserID),
+ ({ query: { user_id } }) => {
+ const validatedUserId = userIdSchema.parse(user_id);
+ return profilesService.queryProfile(validatedUserId);
+ },📝 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.
| ({ query: { user_id } }) => | |
| profilesService.queryProfile(user_id as UserID), | |
| import { userIdSchema } from '@rocket.chat/federation-room'; | |
| … // other imports and setup | |
| router | |
| .get( | |
| '/_matrix/federation/v1/query/profile', | |
| ({ query: { user_id } }) => { | |
| const validatedUserId = userIdSchema.parse(user_id); | |
| return profilesService.queryProfile(validatedUserId); | |
| }, | |
| ) | |
| … // other endpoints |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/profiles.controller.ts around
lines 40-41, the query handler directly casts query.user_id to UserID without
validating its format; instead, extract user_id from req.query as a string,
validate it with the project's existing user ID validator (or a canonical regex)
and if it fails return a 400 error response (with a short message), otherwise
cast to UserID and call profilesService.queryProfile(userId); ensure you handle
missing user_id similarly and keep types strict so the service only receives a
validated UserID.
| return profilesService.makeJoin( | ||
| roomId as RoomID, | ||
| userId as UserID, | ||
| ver ?? ['1'], | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Validate ID formats before casting.
The makeJoin endpoint casts roomId and userId without validation. Additionally, ensure ver array contains valid room version strings.
+import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room';
+
async ({ params, query }) => {
const { roomId, userId } = params;
const { ver } = query;
+
+ const validatedRoomId = roomIdSchema.parse(roomId);
+ const validatedUserId = userIdSchema.parse(userId);
return profilesService.makeJoin(
- roomId as RoomID,
- userId as UserID,
+ validatedRoomId,
+ validatedUserId,
ver ?? ['1'],
);
},📝 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.
| return profilesService.makeJoin( | |
| roomId as RoomID, | |
| userId as UserID, | |
| ver ?? ['1'], | |
| ); | |
| }, | |
| // At the top of packages/homeserver/src/controllers/federation/profiles.controller.ts | |
| import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room'; | |
| … | |
| // Around lines 91–96 | |
| async ({ params, query }) => { | |
| const { roomId, userId } = params; | |
| const { ver } = query; | |
| const validatedRoomId = roomIdSchema.parse(roomId); | |
| const validatedUserId = userIdSchema.parse(userId); | |
| return profilesService.makeJoin( | |
| validatedRoomId, | |
| validatedUserId, | |
| ver ?? ['1'], | |
| ); | |
| }, |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/profiles.controller.ts around
lines 91–96, do not blindly cast roomId and userId; validate their formats first
(use existing helpers like isValidRoomId/isValidUserId or a strict
regex/parser), return a 400/BadRequest when invalid, and only then cast to
RoomID/UserID; also validate the ver input is an array of allowed room version
strings (filter/validate against the supported versions list, default to ['1']
only if none valid) before passing to profilesService.makeJoin to avoid passing
malformed IDs or versions.
| const eventId = await roomService.joinUser( | ||
| roomId as RoomID, | ||
| userId as UserID, | ||
| ); |
There was a problem hiding this comment.
Add validation for ID parameters.
The joinUser endpoint casts both roomId and userId without any validation, creating inconsistency with other endpoints in this file.
+import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room';
+
.put(
'/internal/rooms/:roomId/join/:userId',
async ({ params }) => {
const { roomId, userId } = params;
+
+ const validatedRoomId = roomIdSchema.parse(roomId);
+ const validatedUserId = userIdSchema.parse(userId);
const eventId = await roomService.joinUser(
- roomId as RoomID,
- userId as UserID,
+ validatedRoomId,
+ validatedUserId,
);📝 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.
| const eventId = await roomService.joinUser( | |
| roomId as RoomID, | |
| userId as UserID, | |
| ); | |
| // at the top of the file, alongside your other imports | |
| import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room'; | |
| .put( | |
| '/internal/rooms/:roomId/join/:userId', | |
| async ({ params }) => { | |
| const { roomId, userId } = params; | |
| const validatedRoomId = roomIdSchema.parse(roomId); | |
| const validatedUserId = userIdSchema.parse(userId); | |
| const eventId = await roomService.joinUser( | |
| validatedRoomId, | |
| validatedUserId, | |
| ); | |
| // …rest of handler… |
| const room = await stateService.getFullRoomState( | ||
| params.roomId as RoomID, | ||
| ); |
There was a problem hiding this comment.
Validate room ID before casting.
The getFullRoomState call casts params.roomId without validation.
+import { roomIdSchema } from '@rocket.chat/federation-room';
+
}
- const room = await stateService.getFullRoomState(
- params.roomId as RoomID,
- );
+ const validatedRoomId = roomIdSchema.parse(params.roomId);
+ const room = await stateService.getFullRoomState(validatedRoomId);📝 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.
| const room = await stateService.getFullRoomState( | |
| params.roomId as RoomID, | |
| ); | |
| import { roomIdSchema } from '@rocket.chat/federation-room'; | |
| } | |
| const validatedRoomId = roomIdSchema.parse(params.roomId); | |
| const room = await stateService.getFullRoomState(validatedRoomId); |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/internal/room.controller.ts around lines
212 to 214, the code casts params.roomId to RoomID without validating it;
validate params.roomId before casting (e.g., check it's present, a string, and
matches the RoomID format/regex or use the existing RoomID parsing/validation
helper), return or throw a 400/422 error if invalid, and only call
stateService.getFullRoomState with a safely validated/parsed RoomID value.
| return roomService.markRoomAsTombstone( | ||
| roomIdParse.data, | ||
| bodyParse.data.sender, | ||
| roomIdParse.data as RoomID, | ||
| bodyParse.data.sender as UserID, | ||
| bodyParse.data.reason, | ||
| bodyParse.data.replacementRoomId, | ||
| bodyParse.data.replacementRoomId as RoomID, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant casts after validation.
Similar to other endpoints, this code validates with safeParse but then casts the already-validated data. Additionally, validate the sender field from the body.
+import { userIdSchema, roomIdSchema } from '@rocket.chat/federation-room';
+
}
+ const validatedSender = userIdSchema.parse(bodyParse.data.sender);
+ const validatedReplacementRoomId = bodyParse.data.replacementRoomId
+ ? roomIdSchema.parse(bodyParse.data.replacementRoomId)
+ : undefined;
+
return roomService.markRoomAsTombstone(
- roomIdParse.data as RoomID,
- bodyParse.data.sender as UserID,
+ roomIdParse.data,
+ validatedSender,
bodyParse.data.reason,
- bodyParse.data.replacementRoomId as RoomID,
+ validatedReplacementRoomId,
);🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/internal/room.controller.ts around lines
453 to 458, the code is casting values after a successful safeParse and also
does not explicitly validate the body sender; remove the redundant casts and use
the already-validated parsed values (e.g., roomIdParse.data and bodyParse.data)
directly, and ensure the request body schema includes and validates sender (or
perform a separate safeParse on sender) so you pass a validated sender to
markRoomAsTombstone instead of casting.
| const resp = await inviteService.inviteUserToRoom( | ||
| userId, | ||
| roomId, | ||
| sender, | ||
| userId as UserID, | ||
| roomId as RoomID, | ||
| sender as UserID, | ||
| ); |
There was a problem hiding this comment.
Validate IDs before casting.
The inviteUserToRoom endpoint casts all three ID parameters without validation.
+import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room';
+
async ({ params, body }) => {
const { roomId, userId } = params;
const { sender } = body;
+
+ const validatedUserId = userIdSchema.parse(userId);
+ const validatedRoomId = roomIdSchema.parse(roomId);
+ const validatedSender = userIdSchema.parse(sender);
const resp = await inviteService.inviteUserToRoom(
- userId as UserID,
- roomId as RoomID,
- sender as UserID,
+ validatedUserId,
+ validatedRoomId,
+ validatedSender,
);📝 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.
| const resp = await inviteService.inviteUserToRoom( | |
| userId, | |
| roomId, | |
| sender, | |
| userId as UserID, | |
| roomId as RoomID, | |
| sender as UserID, | |
| ); | |
| // At the top of the file | |
| import { roomIdSchema, userIdSchema } from '@rocket.chat/federation-room'; | |
| ... | |
| // In your async handler | |
| async ({ params, body }) => { | |
| const { roomId, userId } = params; | |
| const { sender } = body; | |
| // Validate IDs at runtime before calling the service | |
| const validatedUserId = userIdSchema.parse(userId); | |
| const validatedRoomId = roomIdSchema.parse(roomId); | |
| const validatedSender = userIdSchema.parse(sender); | |
| const resp = await inviteService.inviteUserToRoom( | |
| validatedUserId, | |
| validatedRoomId, | |
| validatedSender, | |
| ); | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/internal/room.controller.ts around lines
492 to 496, the code directly casts userId, roomId and sender to UserID/RoomID
without validating them first; add input validation before casting: check that
each ID exists and matches the expected format (e.g., non-empty string and/or
regex/type-guard used by your ID types), return a 400/422 error response when
any ID is missing or malformed, and only then cast and call
inviteService.inviteUserToRoom. Use existing validation helpers if available or
add small guard functions (isValidUserID/isValidRoomID) and short-circuit on
failure with an appropriate error message.
| const EventPduTypeRoomAliases = z.object({ | ||
| ...PduNoContentStateEventSchema, | ||
| ...PduNoContentEmptyStateKeyStateEventSchema, | ||
| type: z.literal('m.room.aliases'), | ||
| content: PduCanonicalAliasEventContentSchema, | ||
| }); |
There was a problem hiding this comment.
m.room.aliases state_key must remain the server domain
Per the Matrix spec, m.room.aliases events carry the owning server’s domain in state_key (e.g., example.com). Forcing it to '' will make us reject every legitimate aliases event received over federation. Please revert to a schema that accepts arbitrary server names again—for example:
-const EventPduTypeRoomAliases = z.object({
- ...PduNoContentEmptyStateKeyStateEventSchema,
+const EventPduTypeRoomAliases = z.object({
+ ...PduNoContentTimelineEventSchema,
+ state_key: z.string(),
type: z.literal('m.room.aliases'),
content: PduCanonicalAliasEventContentSchema,
});📝 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.
| const EventPduTypeRoomAliases = z.object({ | |
| ...PduNoContentStateEventSchema, | |
| ...PduNoContentEmptyStateKeyStateEventSchema, | |
| type: z.literal('m.room.aliases'), | |
| content: PduCanonicalAliasEventContentSchema, | |
| }); | |
| const EventPduTypeRoomAliases = z.object({ | |
| ...PduNoContentTimelineEventSchema, | |
| state_key: z.string(), | |
| type: z.literal('m.room.aliases'), | |
| content: PduCanonicalAliasEventContentSchema, | |
| }); |
🤖 Prompt for AI Agents
In packages/room/src/types/v3-11.ts around lines 629 to 633, the
EventPduTypeRoomAliases schema currently forces the state_key to an empty string
which is incorrect for m.room.aliases; change the schema so state_key accepts
arbitrary server domain strings again (e.g., z.string() or
z.string().nonempty(), optionally with a host/domain validation), instead of
''—update the spread or explicit property on the object to allow those server
names so federated aliases events are accepted.
There was a problem hiding this comment.
this is incorrect.. based on the spec https://spec.matrix.org/v1.16/client-server-api/#mroomcanonical_alias canonical alias event should have empty state_key
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Matrix spec m.room.aliases event state_key server domain requirement
💡 Result:
Short answer: the m.room.aliases state event uses the homeserver domain as its state_key (the domain that "owns" the listed aliases), and the event must be sent/signed by that same server (i.e. the event sender’s domain MUST match the state_key). Clients/servers should treat the lists as advisory and validate aliases by resolving them. [1][2][3]
Sources:
- m.room.aliases description (legacy spec): state_key is the homeserver domain which owns these aliases; client advice. [1]
- Room v3/v4 auth rules: events of type m.room.aliases MUST have a state_key and the sender’s domain MUST match the state_key (otherwise reject). [2][3]
@sampaiodiego, you're absolutely correct about m.room.canonical_alias requiring an empty state_key—that change on line 618 is correct! ✓
However, my comment was specifically about m.room.aliases (lines 629-633 in the diff), which is a different event type with different requirements. According to the Matrix spec, m.room.aliases events must have the homeserver domain as the state_key (e.g., "example.com"), and the sender's domain must match that state_key. [1][2][3]
Summary:
- ✅
m.room.canonical_alias→ emptystate_key(line 618 is correct) - ❌
m.room.aliases→ server domainstate_key(line 629 needs to allow non-empty strings)
The fix for m.room.aliases should be:
const EventPduTypeRoomAliases = z.object({
...PduNoContentTimelineEventSchema,
state_key: z.string(),
type: z.literal('m.room.aliases'),
content: PduCanonicalAliasEventContentSchema,
});Note: m.room.aliases is a legacy/deprecated event type, but if the codebase still supports it, the schema must match the spec requirements for proper federation compatibility.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
… and EventID across services and schemas
Summary by CodeRabbit