refactor(federation): Branded types #37131
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Warning Rate limit exceeded@ggazzo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
WalkthroughUse schema-validated Matrix IDs across federation flows, add an emitJoin path for membership joins, introduce a beforeAddUsersToRoom callback and hook to ensure federated users, widen room-topic user types, adjust DB upsert _id handling, change Livechat department removal to $unset, and update package federation-sdk deps/peers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as Matrix Invite API
participant FM as FederationMatrix
participant HS as Homeserver
C->>API: PUT /rooms/{roomId}/join?event_id={eventId}
API->>API: Parse roomId/eventId via roomIdSchema/eventIdSchema
API->>FM: processInvite(parsedRoomId, parsedEventId, inviteEvent)
FM->>HS: add user to room (uses parsed userId/roomId)
HS-->>FM: join success + latest room state
FM->>FM: iterate member join events
FM->>HS: emitJoin(membershipEvent, eventId)
HS-->>FM: ack
FM-->>API: join completed
API-->>C: 200 OK
sequenceDiagram
autonumber
participant U as Inviter
participant M as addUsersToRoom Method
participant CB as beforeAddUsersToRoom Callback
participant F as Federation Hook
participant FM as FederationMatrix
U->>M: addUsersToRoom({ users: [...] })
M->>CB: run({ usernames, inviter }, room)
CB->>F: shouldPerformFederationAction?
alt federated room
F->>FM: ensureFederatedUsersExistLocally(usernames)
FM-->>F: ensured
else non-federated
F-->>CB: no-op
end
CB-->>M: continue
M->>M: add users (batch)
M-->>U: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Matrix federation integration by adding schema validation for user and room IDs across various methods. The changes enhance data integrity by ensuring that Matrix-specific identifiers conform to expected formats before being passed to federation services.
- Adds schema imports (
eventIdSchema,roomIdSchema,userIdSchema) from the federation SDK - Integrates validation by wrapping parameters with schema parsing in federation method calls
- Updates multiple files to consistently validate Matrix identifiers throughout the codebase
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
ee/packages/federation-matrix/src/api/_matrix/profiles.ts |
Adds schema validation for room and user IDs in profile-related API endpoints |
ee/packages/federation-matrix/src/api/_matrix/invite.ts |
Integrates event and room ID validation in invite processing |
ee/packages/federation-matrix/src/FederationMatrix.ts |
Implements comprehensive schema validation across all federation operations including room creation, messaging, and user management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
rodrigok
changed the title
ggazzo
changed the title
ggazzo
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37131 +/- ##
==================================================
- Coverage 67.43% 67.41% -0.03%
==================================================
Files 3329 3329
Lines 113383 113381 -2
Branches 20573 20569 -4
==================================================
- Hits 76464 76433 -31
- Misses 34319 34347 +28
- Partials 2600 2601 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
167-172: Inconsistent state_key property path between guard and usage.Line 167 checks
inviteEvent.stateKey, but line 172 usesinviteEvent.event.state_key. If the event structure now nestsstate_keyunderevent, the guard should be updated to match:- if (!inviteEvent.stateKey) { + if (!inviteEvent.event.state_key) { throw new Error('join event has missing state key, unable to determine user to join'); }
280-315: Inconsistent state_key property path between guard and usage.Line 280 checks
inviteEvent.stateKey, but line 315 usesinviteEvent.event.state_key. The guard should be updated to match the new nested structure:- if (!inviteEvent.stateKey) { + if (!inviteEvent.event.state_key) { throw new Error('join event has missing state key, unable to determine user to join'); }
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
352-352: Consider explicit error handling for schema parsing.The inline schema parsing with
roomIdSchema.parse(roomId)andeventIdSchema.parse(eventId)will throw if validation fails. While this is appropriate for ensuring type safety, consider whether the HTTP framework handles these validation errors gracefully or if explicit try-catch with a user-friendly error response would improve the client experience.Example refactor:
try { const parsedRoomId = roomIdSchema.parse(roomId); const parsedEventId = eventIdSchema.parse(eventId); const inviteEvent = await invite.processInvite(event, parsedRoomId, parsedEventId, roomVersion); // ... rest of the code } catch (error) { if (error instanceof ZodError) { return { body: { error: 'Invalid roomId or eventId format' }, statusCode: 400, }; } throw error; }
📜 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 (4)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts(2 hunks)apps/meteor/ee/server/hooks/federation/index.ts(2 hunks)apps/meteor/lib/callbacks/beforeAddUserToRoom.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/ee/server/hooks/federation/index.ts (2)
apps/meteor/lib/callbacks/beforeAddUserToRoom.ts (1)
beforeAddUsersToRoom(6-7)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(141-978)
apps/meteor/lib/callbacks/beforeAddUserToRoom.ts (3)
apps/meteor/lib/callbacks/callbacksBase.ts (1)
Callbacks(24-234)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
apps/meteor/lib/callbacks/beforeAddUserToRoom.ts (1)
beforeAddUsersToRoom(6-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
3-3: LGTM!The import of schema validators from
@rocket.chat/federation-sdkis appropriate and aligns with the PR's goal of introducing schema-based parsing and validation for Matrix IDs.apps/meteor/lib/callbacks/beforeAddUserToRoom.ts (1)
6-7: LGTM! Clean batch-user hook addition.The new
beforeAddUsersToRoomcallback follows the established pattern and provides the batch equivalent of the existing single-user hook with appropriate type safety.apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
8-8: LGTM! Proper import of the new callback.
81-82: LGTM! Correct hook placement and invocation.The hook is invoked after validation but before the batch add operation, allowing federation and other systems to ensure prerequisites (e.g., federated users exist locally) before attempting to add users. If the hook throws, the entire operation aborts cleanly before any users are added.
apps/meteor/ee/server/hooks/federation/index.ts (2)
8-8: LGTM! Import updated to include the new callback.
73-77: LGTM! Proper batch-user hook registration.The new hook registration follows the established pattern, checks for federation action eligibility, and ensures federated users exist locally before batch-adding them to the room. This complements the existing single-user hook and maintains consistency across the federation integration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/models/src/models/BaseRaw.ts (1)
254-259: Same$setOnInsert._idoverwrite concern as infindOneAndUpdate.This code has the same issue as the
findOneAndUpdatemethod (lines 177-182): it does not check whetherupdate.$setOnInsert._idalready exists before overwriting it. If callers explicitly set an_idin$setOnInsert, it will be replaced with a new ObjectId.Apply the same fix suggested for
findOneAndUpdate:- if (options.upsert && !('_id' in update || (update.$set && '_id' in update.$set))) { + if (options.upsert && !('_id' in update || (update.$set && '_id' in update.$set) || (update.$setOnInsert && '_id' in update.$setOnInsert))) { update.$setOnInsert = { ...(update.$setOnInsert || {}), _id: new ObjectId().toHexString(), } as Partial<T> & { _id: string }; }
🧹 Nitpick comments (1)
packages/models/src/models/BaseRaw.ts (1)
177-182: Also guard against overwriting existing$setOnInsert._id
In the upsert condition, include a check forupdate.$setOnInsertso any explicit_idisn’t replaced:- if (options?.upsert && !('_id' in update || (update.$set && '_id' in update.$set))) { + if (options?.upsert && !('_id' in update || (update.$set && '_id' in update.$set) || (update.$setOnInsert && '_id' in update.$setOnInsert))) {
📜 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 (2)
packages/models/src/models/BaseRaw.ts(2 hunks)packages/models/src/models/LivechatDepartment.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/models/src/models/LivechatDepartment.ts (1)
228-230:removeDepartmentFromUnitusing$unsetis safe
Search found no null‐checks onparentIdorancestors(only one$exists: falsecheck forancestors, which covers the unset case). This aligns with other field‐removal methods. Confirm this change belongs in the federation‐branded‐types PR, as it appears unrelated to that objective.
| } | ||
|
|
||
| updateOne(filter: Filter<T>, update: UpdateFilter<T> | Partial<T>, options?: UpdateOptions): Promise<UpdateResult> { | ||
| updateOne(filter: Filter<T>, update: UpdateFilter<T>, options?: UpdateOptions): Promise<UpdateResult> { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that all callers have been updated for the stricter type signature.
The update parameter type has been changed from UpdateFilter<T> | Partial<T> to UpdateFilter<T>, removing support for passing Partial<T> directly. This is a stricter, more correct type that aligns with MongoDB's updateOne semantics, which expects update operators ($set, $inc, etc.) rather than raw documents.
Run the following script to find all callers of updateOne and verify they use proper update operators:
🏁 Script executed:
#!/bin/bash
# Description: Find all updateOne calls and verify they use update operators
# Find updateOne calls with context
rg -nP '\.updateOne\s*\(' -A 5 -B 2 --type=tsLength of output: 224941
Enforce UpdateFilter in updateOne calls
Calls passing raw update objects (e.g. LivechatVisitors.updateOne({ _id }, update) in LivechatVisitors.ts:361) will fail the new signature. Wrap such calls in an operator filter (for instance, replace updateOne(query, update) with updateOne(query, { $set: update })).
🤖 Prompt for AI Agents
packages/models/src/models/BaseRaw.ts around line 251: the updateOne signature
now requires an UpdateFilter<T>, but existing call sites pass raw update objects
which will break (e.g. LivechatVisitors.updateOne({ _id }, update)). Update
those calls to wrap plain update objects in an operator filter (for example
replace updateOne(query, update) with updateOne(query, { $set: update })) or
explicitly use an appropriate update operator ($inc, $unset, $push, etc.)
depending on the intent; ensure optional UpdateOptions are preserved when
forwarding the call.
ggazzo
force-pushed
the
chore/fed-branded
branch
from
c1f33ff to
ae1e270
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
239-257: Enforce schema.parse for all template‐generated Matrix IDs
Your search uncovered ~16 instances of@${username}:${this.serverName}assignments (e.g. lines 306, 615, 668, 708, 767, 787, 820, 853, 865, 889, 895, 931) that aren’t validated before use. Wrap each inuserIdSchema.parse(...)to catch malformed IDs early.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(24 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(5 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/packages/federation-matrix/package.json
- ee/packages/federation-matrix/src/api/_matrix/invite.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
ee/packages/federation-matrix/src/events/member.ts (1)
member(83-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
11-20: LGTM!The imports for branded types (
EventID,UserID) and schema validators (eventIdSchema,roomIdSchema,userIdSchema) align well with the refactoring objectives to add schema-based parsing and validation for Matrix IDs.
37-37: LGTM!The type predicate return type (
mxid is UserID) is appropriate and allows TypeScript to narrow the type after validation, which improves type safety throughout the codebase.
983-998: Use event’s original timestamp for origin_server_ts
TheemitJoinmethod currently setsorigin_server_tstoDate.now(), which can lead to ordering inconsistencies. Replace it with the timestamp provided by the incoming event (e.g.membershipEvent.originServerTs) to preserve the original ordering.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(24 hunks)packages/core-services/package.json(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
ee/packages/federation-matrix/src/events/member.ts (1)
member(83-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
packages/core-services/src/types/IFederationMatrixService.ts (2)
2-2: LGTM! Type imports enable proper method signature.The import of
PersistentEventBaseandRoomVersionfrom@rocket.chat/federation-sdkenables proper typing for the newemitJoinmethod, replacing the previous unsafeanytype.
27-27: LGTM! Type-safe method signature.The
emitJoinmethod now uses a specific type (PersistentEventBase<RoomVersion, 'm.room.member'>) instead ofany, improving type safety. The signature matches the implementation inFederationMatrix.tsline 983.ee/packages/federation-matrix/src/FederationMatrix.ts (4)
37-67: LGTM! Type predicate improves type safety.The change from
booleantomxid is UserIDenables type narrowing in TypeScript, allowing the compiler to treat validated strings asUserIDtypes in subsequent code. The validation logic correctly checks the Matrix user ID format.
100-141: Manually verify all createOrUpdateFederatedUser call sites
Automated search didn’t locate any usages; confirm every call tocreateOrUpdateFederatedUserpasses a validatedUserID(e.g. viavalidateFederatedUsername).
317-325: Validate constructed user IDs before parsing.Lines 318-320 construct Matrix user IDs by concatenating strings and then parse them. If the construction results in invalid formats, schema parsing will throw at runtime.
Add validation before parsing:
throw new Error('Other member is not federated'); } - const roomId = await this.homeserverServices.room.createDirectMessageRoom( - userIdSchema.parse(actualMatrixUserId), - userIdSchema.parse(otherMember.username), - ); + if (!validateFederatedUsername(actualMatrixUserId)) { + throw new Error(`Invalid Matrix user ID for creator: ${actualMatrixUserId}`); + } + if (!validateFederatedUsername(otherMember.username)) { + throw new Error(`Invalid Matrix user ID for member: ${otherMember.username}`); + } + const roomId = await this.homeserverServices.room.createDirectMessageRoom( + actualMatrixUserId, + otherMember.username, + ); matrixRoomResult = { room_id: roomId };Likely an incorrect or invalid review comment.
325-325: Validate constructed Matrix user ID.Same issue as earlier:
actualMatrixUserIdis constructed from string concatenation and should be validated before parsing.Apply validation:
- matrixRoomResult = await this.homeserverServices.room.createRoom(userIdSchema.parse(actualMatrixUserId), roomName, 'invite'); + if (!validateFederatedUsername(actualMatrixUserId)) { + throw new Error(`Invalid Matrix user ID: ${actualMatrixUserId}`); + } + matrixRoomResult = await this.homeserverServices.room.createRoom(actualMatrixUserId, roomName, 'invite');Likely an incorrect or invalid review comment.
packages/core-services/package.json (1)
38-38: Dependency check passed for @rocket.chat/federation-sdk@0.1.19: version exists, is latest, and no known security advisories.
| async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) { | ||
| if (!this.homeserverServices) { | ||
| this.logger.warn('Homeserver services not available, skipping user role room scoped'); | ||
| return; | ||
| } | ||
|
|
||
| this.homeserverServices.emitter.emit('homeserver.matrix.membership', { | ||
| event_id: membershipEvent.eventId, | ||
| event: membershipEvent.event, | ||
| room_id: membershipEvent.roomId, | ||
| state_key: membershipEvent.event.state_key, | ||
| content: { membership: 'join' }, | ||
| sender: membershipEvent.sender, | ||
| origin_server_ts: Date.now(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Use actual event data instead of hardcoded values.
The emitJoin method constructs a membership event with hardcoded values:
- Line 994:
content: { membership: 'join' }is hardcoded - Line 996:
origin_server_ts: Date.now()uses current time instead of the event's original timestamp
This may cause the emitted event to differ from the actual membership event.
Apply this diff to use the actual event data:
async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) {
if (!this.homeserverServices) {
this.logger.warn('Homeserver services not available, skipping user role room scoped');
return;
}
this.homeserverServices.emitter.emit('homeserver.matrix.membership', {
event_id: membershipEvent.eventId,
event: membershipEvent.event,
room_id: membershipEvent.roomId,
state_key: membershipEvent.event.state_key,
- content: { membership: 'join' },
+ content: membershipEvent.content,
sender: membershipEvent.sender,
- origin_server_ts: Date.now(),
+ origin_server_ts: membershipEvent.origin_server_ts || Date.now(),
});
}📝 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.
| async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) { | |
| if (!this.homeserverServices) { | |
| this.logger.warn('Homeserver services not available, skipping user role room scoped'); | |
| return; | |
| } | |
| this.homeserverServices.emitter.emit('homeserver.matrix.membership', { | |
| event_id: membershipEvent.eventId, | |
| event: membershipEvent.event, | |
| room_id: membershipEvent.roomId, | |
| state_key: membershipEvent.event.state_key, | |
| content: { membership: 'join' }, | |
| sender: membershipEvent.sender, | |
| origin_server_ts: Date.now(), | |
| }); | |
| } | |
| async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) { | |
| if (!this.homeserverServices) { | |
| this.logger.warn('Homeserver services not available, skipping user role room scoped'); | |
| return; | |
| } | |
| this.homeserverServices.emitter.emit('homeserver.matrix.membership', { | |
| event_id: membershipEvent.eventId, | |
| event: membershipEvent.event, | |
| room_id: membershipEvent.roomId, | |
| state_key: membershipEvent.event.state_key, | |
| content: membershipEvent.content, | |
| sender: membershipEvent.sender, | |
| origin_server_ts: membershipEvent.origin_server_ts || Date.now(), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 983 to
998, the emitted membership payload uses hardcoded content and a new timestamp;
replace those with the actual membership event data: use
membershipEvent.event.content (so the real membership field and any other
content) instead of { membership: 'join' }, and use
membershipEvent.event.origin_server_ts (the original event timestamp) instead of
Date.now(); keep the other fields (event_id, event, room_id, state_key, sender)
as-is so the emitted object matches the original event.
bc0ad58 to
4d0a63e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
ee/packages/federation-matrix/src/FederationMatrix.ts (4)
240-240: Handle schema parsing failures gracefully.Directly calling
userIdSchema.parse()on a constructed string can throw at runtime if the composition produces an invalid Matrix user ID. This was flagged in a previous review.Apply this diff to handle parsing failures:
- const matrixUserId = userIdSchema.parse(`@${owner.username}:${this.serverName}`); + const matrixUserIdString = `@${owner.username}:${this.serverName}`; + if (!validateFederatedUsername(matrixUserIdString)) { + throw new Error(`Invalid Matrix user ID constructed for owner: ${matrixUserIdString} (username: ${owner.username}, server: ${this.serverName})`); + } + const matrixUserId = matrixUserIdString;
984-999: Use actual event data instead of hardcoded values.The
emitJoinmethod uses hardcodedcontent: { membership: 'join' }(line 995) andorigin_server_ts: Date.now()(line 997) instead of the actual event data. This was flagged in a previous review and can cause the emitted event to differ from the actual membership event.Apply this diff to use actual event data:
async emitJoin(membershipEvent: PduForType<'m.room.member'>, eventId: EventID) { if (!this.homeserverServices) { this.logger.warn('Homeserver services not available, skipping user role room scoped'); return; } this.homeserverServices.emitter.emit('homeserver.matrix.membership', { event_id: eventId, event: membershipEvent, room_id: membershipEvent.room_id, state_key: membershipEvent.state_key, - content: { membership: 'join' }, + content: membershipEvent.content, sender: membershipEvent.sender, - origin_server_ts: Date.now(), + origin_server_ts: membershipEvent.origin_server_ts || Date.now(), }); }
258-258: Validate member strings before schema parsing.The
memberparameter frommembers: string[]is directly passed touserIdSchema.parse()without validation, which can throw at runtime if the member string is malformed. This was flagged in a previous review.Apply this diff to validate before parsing:
for await (const member of members) { if (member === owner.username) { continue; } // We are not generating bridged users for members outside of the current workspace // They will be created when the invite is accepted + if (!validateFederatedUsername(member)) { + this.logger.warn(`Skipping invalid Matrix user ID in room creation: ${member}`); + continue; + } - await this.homeserverServices.invite.inviteUserToRoom(userIdSchema.parse(member), matrixRoomResult.room_id, matrixUserId); + await this.homeserverServices.invite.inviteUserToRoom(member, matrixRoomResult.room_id, matrixUserId); }
339-343: Validate user IDs before inviting to DM.The code directly parses
member.usernameandactualMatrixUserIdwithout validation, which can throw at runtime. This was flagged in a previous review.Apply this diff:
try { + if (!isUserNativeFederated(member) || !validateFederatedUsername(member.username)) { + this.logger.error(`Invalid Matrix user ID for member in DM: ${member.username}`); + continue; + } + if (!validateFederatedUsername(actualMatrixUserId)) { + throw new Error(`Invalid Matrix user ID for creator: ${actualMatrixUserId}`); + } await this.homeserverServices.invite.inviteUserToRoom( - userIdSchema.parse(member.username), - roomIdSchema.parse(matrixRoomResult.room_id), - userIdSchema.parse(actualMatrixUserId), + member.username, + matrixRoomResult.room_id, + actualMatrixUserId, ); } catch (error) { this.logger.error('Error creating or updating bridged user for DM:', error); }
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
397-401: Schema parsing in file message handling.Multiple schema parsing calls for
matrixRoomIdandmatrixUserId. These should already be validated upstream, but consider adding safeguards.Consider validating before parsing:
+ if (!validateFederatedUsername(matrixUserId)) { + throw new Error(`Invalid Matrix user ID for file message: ${matrixUserId}`); + } lastEventId = await this.homeserverServices.message.sendFileMessage( - roomIdSchema.parse(matrixRoomId), + matrixRoomId, fileContent, - userIdSchema.parse(matrixUserId), + matrixUserId, );
433-438: Schema parsing in text message handling.Similar to file messages, validate before parsing.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
ee/packages/federation-matrix/package.json(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(24 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(5 hunks)packages/core-services/package.json(2 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/api/_matrix/invite.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
ee/packages/federation-matrix/src/events/member.ts (1)
member(83-99)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(146-1000)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (11)
packages/core-services/src/types/IFederationMatrixService.ts (2)
2-2: LGTM! Type imports added for improved type safety.The addition of
EventIDandPduForTypeimports enables strongly-typed federation event handling.
27-27: VerifyemitJoincall-site types
Confirm thatmatrixRoom.getMemberJoinEvents()yieldsevent.event: PduForType<'m.room.member'>andevent.eventId: EventIDbefore invokingFederationMatrix.emitJoin.ee/packages/federation-matrix/src/api/_matrix/invite.ts (6)
1-3: LGTM! Imports updated for schema validation.The addition of
eventIdSchemaandroomIdSchemaenables runtime validation of Matrix identifiers, and the inclusion ofFederationMatrixandRoomservices aligns with the new emitJoin flow.
319-319: Consistent with earlier state_key change.The use of
inviteEvent.event.state_keyis consistent with the change at line 172.
356-356: Schema parsing adds runtime validation.The addition of
roomIdSchema.parse(roomId)andeventIdSchema.parse(eventId)ensures that invalid identifiers are caught early before being passed to downstream processing. This aligns with the PR's objective of introducing branded types for safer Matrix ID handling.
268-271: Emitting join events for all members may cause race conditions.The code iterates over
matrixRoom.getMemberJoinEvents()and emits each join viaFederationMatrix.emitJoin. This could lead to:
- Race conditions if multiple join events are processed concurrently
- Duplicate processing if the same events are already being handled elsewhere
- Performance issues if there are many members in the room
Consider the following improvements:
- for await (const event of matrixRoom.getMemberJoinEvents()) { - await FederationMatrix.emitJoin(event.event, event.eventId); - } + // Emit joins sequentially to avoid race conditions + const memberJoinEvents = Array.from(matrixRoom.getMemberJoinEvents()); + this.logger.debug(`Emitting ${memberJoinEvents.length} member join events for room ${inviteEvent.roomId}`); + + for (const event of memberJoinEvents) { + try { + await FederationMatrix.emitJoin(event.event, event.eventId); + } catch (error) { + this.logger.error(`Failed to emit join for event ${event.eventId}:`, error); + // Continue processing other events rather than failing the entire flow + } + }Likely an incorrect or invalid review comment.
175-175: Confirm getLatestRoomState2 covers full room state. Verify that replacing getFullRoomState2 with getLatestRoomState2 still returns getMemberJoinEvents() and any other data consumed at line 269.
172-172: EnsureinviteEvent.event.state_keyis defined
Confirm that thePersistentEventBase<RoomVersion, 'm.room.member'>type from@rocket.chat/federation-sdkincludes anevent.state_keyproperty before accessing it here.ee/packages/federation-matrix/src/FederationMatrix.ts (3)
11-21: LGTM! Comprehensive type imports for schema validation.The imports include schema validators (
eventIdSchema,roomIdSchema,userIdSchema) and branded types (EventID,UserID,PduForType) that enable compile-time and runtime type safety for Matrix identifiers.
38-68: Type predicate improves type narrowing.The return type change from
booleantomxid is UserIDenables TypeScript to narrow the type after validation, improving type safety throughout the codebase. The validation logic correctly checks the Matrix ID format per the Matrix specification.
101-142: Ensure all calls to createOrUpdateFederatedUser receive a validated UserID
Automated search was inconclusive—manually verify each call site appliesvalidateFederatedUsernameor otherwise guarantees a validUserID.
| "@rocket.chat/federation-sdk": "*" | ||
| } |
There was a problem hiding this comment.
Tighten the peer dependency range.
Declaring the SDK peer requirement as "*" permits installing versions older than 0.1.19, which can break the new branded-type usage added in this PR. Please constrain the peer dependency to the minimum compatible version (e.g., ">=0.1.19" or a caret range) so consumers get a version that actually supports these features.
🤖 Prompt for AI Agents
In packages/core-services/package.json around lines 49 to 50, the peer
dependency for "@rocket.chat/federation-sdk" is declared as "*" which allows
installing older incompatible versions; update the peerDependencies entry to
require a minimum version that supports the new branded-type usage (for example
">=0.1.19" or "^0.1.19") so consumers are constrained to compatible SDK releases
and package managers will warn on older installs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
975-990: Use actual event data instead of hardcoded values.This issue has been flagged in previous reviews: the method hardcodes
content: { membership: 'join' }(line 986) and usesDate.now()(line 988) instead of the actual event'scontentandorigin_server_ts. This could cause the emitted event to differ from the original membership event.Apply the suggested fix from the previous review to use actual event data:
async emitJoin(membershipEvent: PduForType<'m.room.member'>, eventId: EventID) { if (!this.homeserverServices) { this.logger.warn('Homeserver services not available, skipping user role room scoped'); return; } this.homeserverServices.emitter.emit('homeserver.matrix.membership', { event_id: eventId, event: membershipEvent, room_id: membershipEvent.room_id, state_key: membershipEvent.state_key, - content: { membership: 'join' }, + content: membershipEvent.content, sender: membershipEvent.sender, - origin_server_ts: Date.now(), + origin_server_ts: membershipEvent.origin_server_ts || Date.now(), }); }
🧹 Nitpick comments (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
388-392: Consider validating IDs before parsing.The
roomIdSchema.parse(matrixRoomId)anduserIdSchema.parse(matrixUserId)calls can throw if the IDs (constructed earlier from user/room data) contain invalid characters. Consider usingsafeParseand handling failures gracefully, or validating beforehand.Example with safeParse:
+ const roomIdResult = roomIdSchema.safeParse(matrixRoomId); + if (!roomIdResult.success) { + throw new Error(`Invalid Matrix room ID: ${matrixRoomId}`); + } + const userIdResult = userIdSchema.safeParse(matrixUserId); + if (!userIdResult.success) { + throw new Error(`Invalid Matrix user ID: ${matrixUserId}`); + } + lastEventId = await this.homeserverServices.message.sendFileMessage( - roomIdSchema.parse(matrixRoomId), + roomIdResult.data, fileContent, - userIdSchema.parse(matrixUserId), + userIdResult.data, );
612-617: Clarify the validation-then-parse pattern.After
validateFederatedUsername(username)returns true (line 611),usernameis typed asUserID, yetuserIdSchema.parse(username)is still called (line 613). If the validation logic matches the schema, the parse is redundant. If the schema provides additional normalization or stricter checks, consider documenting this pattern.
662-667: Validate constructed user ID before parsing.Similar to other locations,
userMuiis constructed from user data (line 660) and then directly parsed (line 667) without validation. Ifuser.usernamecontains invalid characters,userIdSchema.parse()will throw.Consider validating the constructed ID:
const userMui = isUserNativeFederated(user) ? user.federation.mui : `@${user.username}:${this.serverName}`; + if (!validateFederatedUsername(userMui)) { + throw new Error(`Invalid Matrix user ID: ${userMui}`); + } const eventId = await this.homeserverServices.message.sendReaction( roomIdSchema.parse(room.federation.mrid), eventIdSchema.parse(matrixEventId), reactionKey, - userIdSchema.parse(userMui), + userMui, );
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
ee/packages/federation-matrix/src/FederationMatrix.ts(24 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
🔇 Additional comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
11-12: LGTM! Schema imports support type safety.The addition of
eventIdSchema,roomIdSchema, anduserIdSchemaimports along with their branded types (EventID,UserID,PduForType) enables schema-based validation throughout the file.
29-59: LGTM! Type predicate improves type safety.The return type
mxid is UserIDcorrectly narrows the type for TypeScript, enabling better compile-time checks when the validation passes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
ee/packages/federation-matrix/src/events/member.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(8-68)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
getUsernameServername(73-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
ggazzo
force-pushed
the
chore/fed-branded
branch
6 times, most recently
from
8d6d30d to
89ebe54
Compare
… for user and room IDs across various methods
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
…kages and others, to their latest versions for improved compatibility and performance
…m to ensure valid operation
ggazzo
force-pushed
the
chore/fed-branded
branch
from
89ebe54 to
94445d8
Compare
https://rocketchat.atlassian.net/browse/FDR-219
https://rocketchat.atlassian.net/browse/FDR-208
https://rocketchat.atlassian.net/browse/FDR-140
https://rocketchat.atlassian.net/browse/FDR-216
https://rocketchat.atlassian.net/browse/FDR-217
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Chores