fix(federation): allow to join non-private or encrypted rooms based on settings #280
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds invite configuration and enforcement for encrypted/public rooms, exports NotAllowedError, maps that error to 403 in the homeserver invite controller, introduces a federation error DTO, wires invite config into the homeserver module, emits m.room.encryption events from staging-area, and extends room event schemas/types to include encryption. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as HomeserverController
participant InviteService
participant Config as ConfigService
Client->>Controller: POST /federation/invite (member PDU)
Controller->>InviteService: processInvite(event, ...)
InviteService->>Config: getInviteConfig()
Config-->>InviteService: {allowedEncryptedRooms, allowedNonPrivateRooms}
alt Disallowed by config
InviteService-->>Controller: throw NotAllowedError
Controller-->>Client: 403 { errcode: M_FORBIDDEN, error }
else Allowed
InviteService-->>Controller: ProcessInviteResponseDto
Controller-->>Client: 200 ProcessInviteResponseDto
end
sequenceDiagram
participant StagingArea
participant EventBus
participant Consumers
StagingArea->>StagingArea: receive PDU
alt PDU.type == "m.room.encryption"
StagingArea->>EventBus: emit homeserver.matrix.encryption { event_id, event }
else PDU.type == "m.room.message" or "m.room.encrypted"
StagingArea->>EventBus: emit existing events (message/encrypted)
end
EventBus-->>Consumers: dispatch event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (10)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #280 +/- ##
==========================================
+ Coverage 60.46% 60.55% +0.09%
==========================================
Files 67 67
Lines 6673 6691 +18
==========================================
+ Hits 4035 4052 +17
- Misses 2638 2639 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
fix/block-encrypted-and-non-private-rooms-join
branch
from
b9c7af3 to
4c1889e
Compare
ricardogarim
force-pushed
the
fix/block-encrypted-and-non-private-rooms-join
branch
2 times, most recently
from
53687fc to
7faaf02
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/room/src/types/v3-11.ts (1)
788-796: Remove m.room.encryption from timeline event types.
m.room.encryptionis a state event, not a timeline event. Including it inisTimelineEventTypeis incorrect and will cause improper event handling.Apply this diff:
export function isTimelineEventType(type: PduType) { return ( type === 'm.room.message' || type === 'm.room.encrypted' || - type === 'm.room.encryption' || type === 'm.reaction' || type === 'm.room.redaction' ); }
📜 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 (10)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/services/config.service.ts(3 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(2 hunks)packages/homeserver/src/dtos/federation/error.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)packages/room/src/types/v3-11.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/federation-sdk/src/services/staging-area.service.ts (2)
packages/room/src/manager/event-wrapper.ts (2)
event(129-138)roomId(101-103)packages/room/src/manager/v3.ts (1)
eventId(17-29)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(129-138)packages/room/src/types/_common.ts (1)
PduForType(26-26)
packages/federation-sdk/src/index.ts (1)
packages/room/src/types/_common.ts (2)
EventID(8-8)PduForType(26-26)
packages/room/src/types/v3-11.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
type(97-99)
packages/homeserver/src/controllers/federation/invite.controller.ts (4)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/federation-sdk/src/services/invite.service.ts (1)
NotAllowedError(28-33)packages/homeserver/src/dtos/federation/invite.dto.ts (1)
ProcessInviteResponseDto(12-14)packages/homeserver/src/dtos/federation/error.dto.ts (2)
FederationErrorResponseDto(3-11)FederationErrorResponseDto(13-15)
🔇 Additional comments (11)
packages/federation-sdk/src/services/room.service.ts (1)
849-849: LGTM! Removing unnecessary type cast.The removal of the
as stringtype cast is a good change. It allows TypeScript to properly infer the type fromextractDomainFromId, improving type safety.packages/homeserver/src/dtos/federation/error.dto.ts (1)
1-15: LGTM! Clean federation error DTO implementation.The schema properly defines the federation error response structure with appropriate error codes for Matrix protocol compliance.
packages/homeserver/src/dtos/index.ts (1)
15-15: LGTM! Proper DTO re-export.The federation error DTO is correctly exported from the centralized DTO index.
packages/federation-sdk/src/services/staging-area.service.ts (1)
311-319: LGTM! Proper encryption event emission.The
m.room.encryptionevent handling follows the established pattern for other event types, with appropriate payload structure.packages/federation-sdk/src/index.ts (2)
93-93: LGTM! NotAllowedError properly exported.Exposing
NotAllowedErrorenables consumers (like the homeserver invite controller) to handle invite rejection scenarios.
167-173: LGTM! Encryption event signature properly defined.The
homeserver.matrix.encryptionevent signature follows the established pattern with correct typing.packages/federation-sdk/src/services/invite.service.ts (2)
28-33: LGTM! Clean error class definition.The
NotAllowedErrorproperly extendsErrorwith a descriptive name for invite rejection scenarios.
152-186: LGTM! Solid invite validation logic.The
shouldProcessInvitemethod correctly validates room properties against configuration:
- Detects public rooms via
join_rulecheck- Detects encrypted rooms via
m.room.encryptionstate event- Applies config-driven policy to reject invites when appropriate
The rejection logic properly uses AND/OR operators to enforce both constraints.
packages/federation-sdk/src/services/config.service.ts (3)
34-37: LGTM! Invite config properly typed.The
inviteconfiguration block is correctly defined in theAppConfiginterface with clear boolean fields for encryption and privacy controls.
76-79: LGTM! Invite schema validation added.The Zod schema properly validates the invite configuration structure, ensuring type safety at runtime.
124-126: LGTM! Clean accessor for invite config.The
getInviteConfigmethod provides type-safe access to the invite configuration.
ricardogarim
force-pushed
the
fix/block-encrypted-and-non-private-rooms-join
branch
5 times, most recently
from
6f04192 to
0af839b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/room/src/types/v3-11.ts (1)
788-796: Remove m.room.encryption from timeline event check.
m.room.encryptionis a state event (with an empty state key), not a timeline event. Including it inisTimelineEventTypeis incorrect and inconsistent with the schema definition at lines 716-720 which correctly usesPduNoContentEmptyStateKeyStateEventSchema.Apply this diff:
export function isTimelineEventType(type: PduType) { return ( type === 'm.room.message' || type === 'm.room.encrypted' || - type === 'm.room.encryption' || type === 'm.reaction' || type === 'm.room.redaction' ); }
♻️ Duplicate comments (2)
packages/room/src/types/v3-11.ts (1)
540-545: Fix the schema - m.room.encryption is a state event, not an encrypted message.The past review comment correctly identified that
PduEncryptionEventContentSchemashould NOT include aciphertextfield. According to the Matrix spec,m.room.encryptionis a state event that configures encryption settings for a room, not an encrypted message event. Theciphertextfield belongs tom.room.encryptedevents (which already exists asEncryptedContentSchemaat lines 520-538).The content should include:
algorithm: The encryption algorithm (required)rotation_period_ms: Optional rotation period in millisecondsrotation_period_msgs: Optional rotation period in message countApply this diff:
export const PduEncryptionEventContentSchema = z.object({ algorithm: z .enum(['m.megolm.v1.aes-sha2']) .describe('The algorithm used to encrypt the content.'), - ciphertext: z.string().describe('The encrypted content.'), + rotation_period_ms: z + .number() + .int() + .positive() + .optional() + .describe('How long the session should be used before changing it (in milliseconds).'), + rotation_period_msgs: z + .number() + .int() + .positive() + .optional() + .describe('How many messages should be sent before changing the session.'), });packages/homeserver/src/controllers/federation/invite.controller.ts (1)
28-54: Useerror.messageinstead of hard-coded text (per previous agreement).Lines 38-43 hard-code the error message as
'This server does not allow joining this type of room based on federation settings.'despite previous review feedback where you explicitly agreed to useerror.messageto preserve the descriptive policy reason (e.g., "encrypted" vs "public"). TheNotAllowedErrorcarries specific context that federation clients need to understand why the invite was rejected.Apply this diff:
} catch (error) { if (error instanceof NotAllowedError) { set.status = 403; return { errcode: 'M_FORBIDDEN', - error: - 'This server does not allow joining this type of room based on federation settings.', + error: error.message, }; } set.status = 500; return { errcode: 'M_UNKNOWN', error: error instanceof Error ? error.message : 'Internal server error while processing request', }; }
📜 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 (9)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/services/config.service.ts(3 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(2 hunks)packages/homeserver/src/dtos/federation/error.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)packages/room/src/types/v3-11.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/federation-sdk/src/services/config.service.ts
- packages/federation-sdk/src/index.ts
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/services/room.service.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/federation-sdk/src/services/invite.service.ts (1)
packages/federation-sdk/src/index.ts (4)
NotAllowedError(93-93)PduForType(22-22)PersistentEventBase(25-25)RoomVersion(26-26)
packages/homeserver/src/controllers/federation/invite.controller.ts (4)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/federation-sdk/src/services/invite.service.ts (1)
NotAllowedError(28-33)packages/homeserver/src/dtos/federation/invite.dto.ts (1)
ProcessInviteResponseDto(12-14)packages/homeserver/src/dtos/federation/error.dto.ts (2)
FederationErrorResponseDto(3-11)FederationErrorResponseDto(13-15)
packages/room/src/types/v3-11.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
type(97-99)
🔇 Additional comments (7)
packages/homeserver/src/dtos/federation/error.dto.ts (1)
1-15: LGTM!The FederationErrorResponseDto schema is correctly defined with appropriate Matrix error codes (M_UNRECOGNIZED, M_UNAUTHORIZED, M_FORBIDDEN, M_UNKNOWN) and follows Elysia conventions. The type alias is properly exported using
Static<typeof>.packages/federation-sdk/src/services/invite.service.ts (2)
28-33: LGTM!The NotAllowedError class follows standard error class patterns and correctly extends Error with a descriptive name property.
196-196: LGTM!The call to
shouldProcessInviteis correctly placed at the start ofprocessInviteto validate the invite before processing, and it's properly awaited.packages/room/src/types/v3-11.ts (1)
716-720: LGTM!The EventPduTypeRoomEncryption correctly uses
PduNoContentEmptyStateKeyStateEventSchemasince m.room.encryption is a state event with an empty state key. This addresses the previous review feedback.packages/homeserver/src/dtos/index.ts (1)
15-15: LGTM!The export for
federation/error.dtois correctly placed alongside other federation DTO exports, makingFederationErrorResponseDtopublicly accessible.packages/homeserver/src/controllers/federation/invite.controller.ts (2)
5-5: LGTM!The imports are correctly organized, bringing in
NotAllowedErrorfrom the federation-sdk and the necessary DTOs for error responses and request/response schemas.Also applies to: 10-15
63-67: LGTM!The response schema correctly maps status codes to their respective DTOs (200 → ProcessInviteResponseDto, 403/500 → FederationErrorResponseDto), providing proper OpenAPI documentation for the route.
ricardogarim
force-pushed
the
fix/block-encrypted-and-non-private-rooms-join
branch
from
0af839b to
cecfffb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/room/src/types/v3-11.ts (1)
540-545: Fix the schema to match Matrix spec for m.room.encryption.The
PduEncryptionEventContentSchemastill incorrectly includes aciphertextfield. According to the Matrix spec,m.room.encryptionis a state event that configures encryption settings for a room, not an encrypted message event (which ism.room.encryptedat lines 710-714).The content should only include:
algorithm: The encryption algorithm (required)rotation_period_ms: Optional rotation period in millisecondsrotation_period_msgs: Optional rotation period in message countApply this diff:
export const PduEncryptionEventContentSchema = z.object({ algorithm: z .enum(['m.megolm.v1.aes-sha2']) .describe('The algorithm used to encrypt the content.'), - ciphertext: z.string().describe('The encrypted content.'), + rotation_period_ms: z + .number() + .int() + .positive() + .optional() + .describe('How long the session should be used before changing it (in milliseconds).'), + rotation_period_msgs: z + .number() + .int() + .positive() + .optional() + .describe('How many messages should be sent before changing the session.'), });packages/federation-sdk/src/services/invite.service.ts (1)
152-186: Clarify the error message when multiple conditions are violated.When both
isRoomEncryptedandisRoomNonPrivateare true, the error message at Line 183 only mentions one reason. This could confuse federation clients about which policy was violated.Consider building a list of violated reasons and including all applicable policy violations in the error message. For example:
const shouldRejectInvite = (!allowedEncryptedRooms && isRoomEncrypted) || (!allowedNonPrivateRooms && isRoomNonPrivate); if (shouldRejectInvite) { + const reasons = []; + if (!allowedEncryptedRooms && isRoomEncrypted) { + reasons.push('encrypted'); + } + if (!allowedNonPrivateRooms && isRoomNonPrivate) { + reasons.push('public'); + } throw new NotAllowedError( - `Could not process invite due to room being ${isRoomEncrypted ? 'encrypted' : 'public'}`, + `Could not process invite due to room being ${reasons.join(' and ')}`, ); }
📜 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 (10)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/services/config.service.ts(3 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(2 hunks)packages/homeserver/src/dtos/federation/error.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)packages/room/src/types/v3-11.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/index.ts
- packages/homeserver/src/dtos/federation/error.dto.ts
- packages/homeserver/src/dtos/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/homeserver/src/controllers/federation/invite.controller.ts (4)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/federation-sdk/src/services/invite.service.ts (1)
NotAllowedError(28-33)packages/homeserver/src/dtos/federation/invite.dto.ts (1)
ProcessInviteResponseDto(12-14)packages/homeserver/src/dtos/federation/error.dto.ts (2)
FederationErrorResponseDto(3-11)FederationErrorResponseDto(13-15)
packages/federation-sdk/src/services/invite.service.ts (3)
packages/federation-sdk/src/index.ts (4)
NotAllowedError(93-93)PduForType(22-22)PersistentEventBase(25-25)RoomVersion(26-26)packages/room/src/manager/event-wrapper.ts (1)
event(129-138)packages/room/src/types/_common.ts (1)
PduForType(26-26)
packages/room/src/types/v3-11.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
type(97-99)
packages/federation-sdk/src/services/staging-area.service.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(129-138)packages/room/src/manager/v3.ts (1)
eventId(17-29)
🔇 Additional comments (8)
packages/federation-sdk/src/services/staging-area.service.ts (1)
311-316: LGTM! Clean implementation of encryption event handling.The new case for
m.room.encryptionfollows the established pattern and appropriately emits a minimal event payload. This is correct for encryption setup events where consumers need the full event object to extract encryption parameters.packages/federation-sdk/src/services/room.service.ts (1)
848-848: LGTM! Trailing comma follows best practices.The trailing comma addition aligns with modern JavaScript/TypeScript conventions and makes future parameter additions cleaner in version control diffs.
packages/room/src/types/v3-11.ts (2)
716-720: LGTM!The schema correctly uses
PduNoContentEmptyStateKeyStateEventSchema, which is appropriate form.room.encryptionas it's a state event with an empty state key per the Matrix specification.
766-766: LGTM!Correctly adds
EventPduTypeRoomEncryptiontoPduStateEventSchemasincem.room.encryptionis a state event.packages/federation-sdk/src/services/invite.service.ts (2)
28-33: LGTM!The
NotAllowedErrorclass is correctly implemented as a custom error type for authorization failures during invite processing.
196-196: LGTM!The call to
shouldProcessInviteat the start ofprocessInvitecorrectly enforces invite validation before proceeding with invite processing.packages/homeserver/src/controllers/federation/invite.controller.ts (2)
28-54: LGTM!The error handling implementation is correct:
NotAllowedErrorreturns 403 withM_FORBIDDENerrcode and a generic policy message (as confirmed by the maintainer).- Other errors return 500 with
M_UNKNOWNerrcode and the actual error message for debugging.
63-67: LGTM!The response schema definitions correctly document the possible response types (200, 403, 500) using the appropriate DTOs.
| invite: { | ||
| allowedEncryptedRooms: boolean; | ||
| allowedNonPrivateRooms: boolean; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify test coverage for the new invite configuration.
The implementation follows existing patterns and is correct. However, the codecov report indicates that this file has 50% patch coverage with 4 missing lines.
Please ensure that the new invite configuration block and the getInviteConfig() method have adequate test coverage. You can run the following script to check which lines are missing coverage:
Also applies to: 76-79, 124-126
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for config.service.ts
# Find and display test files for config.service
fd -e spec.ts -e test.ts config.service
# Check if the invite config is tested
rg -nP "getInviteConfig|invite.*config" --type=ts -g '*test*' -g '*spec*'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for config.service usages and config getters in test files
rg -n "config.service" -g "*spec.ts" -g "*test.ts"
rg -n "getDatabaseConfig" -g "*spec.ts" -g "*test.ts"
rg -n "getMediaConfig" -g "*spec.ts" -g "*test.ts"
rg -n "getInviteConfig" -g "*spec.ts" -g "*test.ts"Length of output: 347
Add unit tests for the new invite configuration
No tests currently cover getInviteConfig() or the invite block in config.service.ts; please add spec tests to exercise those code paths and restore full coverage.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/config.service.ts around lines 34 to 37,
there are no unit tests covering getInviteConfig() or the invite configuration
block; add Jest spec tests that import ConfigService (or the module exposing
getInviteConfig), mock the underlying configuration/source to supply variations,
and assert the returned invite object shape and values. Create tests for:
default behavior when invite is missing, explicit true/false values for
allowedEncryptedRooms and allowedNonPrivateRooms, and any edge/invalid inputs
the service tolerates; use beforeEach to set/restore config mocks and expect
exact booleans to restore coverage for these code paths.
cecfffb to
6c1db76
Compare
Changes:
shouldProcessInvitefails validation.NotAllowedErrorcustom error.FederationErrorResponseDto.m.room.encryptionMatrix event and enables propagation of thehomeserver.matrix.encryptionevent.allowedEncryptedRoomsandallowedNonPrivateRoomsto control invite behavior.Related to RocketChat/Rocket.Chat#37227.
Summary by CodeRabbit
New Features
Bug Fixes / API