refactor: add isAuthenticated and canAccessResource middlewares #224
Conversation
WalkthroughCentralized federation authorization: signature verification signature/order adjusted; added AclDeniedError and server ACL handling; introduced matchesServerPattern, serverHasAccessToResource and canAccessResource; promoted canAccessEvent/canAccessMedia/canAccessRoom; added isAuthenticated and canAccessResource middlewares, wired across federation controllers; removed legacy canAccessEvent middleware. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Homeserver Router
participant Auth as isAuthenticatedMiddleware
participant ACL as canAccessResourceMiddleware
participant Ctrl as Controller
participant Svc as EventAuthorizationService
C->>R: HTTP request (Authorization, method, path, body)
R->>Auth: run auth handler
Auth->>Svc: verifyRequestSignature(authorizationHeader, method, uri, body)
alt signature valid
Svc-->>Auth: authenticatedServer
Auth-->>R: attach authenticatedServer
R->>ACL: onBeforeHandle (entityType, params, authenticatedServer)
ACL->>Svc: canAccessResource(entityType, entityId, authenticatedServer)
alt access allowed
Svc-->>ACL: true
ACL-->>R: proceed to Controller
Ctrl-->>C: 200 / resource
else access denied
Svc-->>ACL: throws AclDeniedError or returns false
ACL-->>C: 403 M_FORBIDDEN
end
else signature missing/invalid
Auth-->>C: 401 M_UNAUTHORIZED or 500 M_UNKNOWN
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 #224 +/- ##
==========================================
- Coverage 60.31% 60.30% -0.01%
==========================================
Files 67 67
Lines 6672 6673 +1
==========================================
Hits 4024 4024
- Misses 2648 2649 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
c8aa81f to
30cf97f
Compare
ricardogarim
changed the title
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
30cf97f to
03ba043
Compare
ggazzo
force-pushed
the
refactor/middlewares
branch
from
03ba043 to
3e2107d
Compare
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
3e2107d to
aa8bbf2
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
25-33: Restore the request body after signature verification.Line 25:
isAuthenticated(eventAuthService)currently callsrequest.text()to compute the signature. Because this derives middleware runs before the route, it consumes the request stream, so the PUT handler no longer receives the JSON payload—bodybecomesundefinedandprocessIncomingTransactioncannot ingest the transaction. The same middleware use will also break other routes (e.g., the invite flow) that rely onbody. Please clone the request (or otherwise preserve/reinject the parsed payload) before reading it, so the downstream handlers still see the original body.Apply this diff to fix the issue:
--- a/packages/homeserver/src/middlewares/isAuthenticated.ts +++ b/packages/homeserver/src/middlewares/isAuthenticated.ts @@ - if (request.body) { + if (request.body) { try { - const text = await request.text(); + const text = await request.clone().text(); body = text ? JSON.parse(text) : undefined; } catch { body = undefined; } }
🧹 Nitpick comments (5)
packages/homeserver/src/middlewares/canAccessResource.ts (2)
58-64: Log error details for better debugging.Swallowing error details makes debugging difficult in production. Consider logging the actual error.
-} catch (_err) { +} catch (err) { + console.error('Resource access check failed:', err); set.status = errCodes.M_UNKNOWN.status; return { errcode: errCodes.M_UNKNOWN.errcode,
22-23: Consider making the undefined return more explicit.Returning
undefinedimplicitly could be confusing. Consider returningnullor throwing an error if no entity is found.-return; +return undefined; // No entity found in paramspackages/homeserver/src/controllers/federation/profiles.controller.ts (1)
31-32: Add error handling for dependency resolution.The
container.resolve()calls could throw if the services aren't registered. Consider adding error handling.-const profilesService = container.resolve(ProfilesService); -const eventAuthService = container.resolve(EventAuthorizationService); +let profilesService: ProfilesService; +let eventAuthService: EventAuthorizationService; + +try { + profilesService = container.resolve(ProfilesService); + eventAuthService = container.resolve(EventAuthorizationService); +} catch (error) { + console.error('Failed to resolve required services:', error); + throw new Error('Service initialization failed'); +}packages/homeserver/src/middlewares/isAuthenticated.ts (2)
27-29: Silent error handling might hide issues.Silently catching and ignoring body parsing errors could mask problems. Consider at least logging these errors in development.
try { const text = await request.text(); body = text ? JSON.parse(text) : undefined; -} catch { +} catch (error) { + // Log in development to catch parsing issues + if (process.env.NODE_ENV === 'development') { + console.warn('Failed to parse request body:', error); + } body = undefined; }
46-48: RenameisValidtoserverNamefor clarityIn packages/homeserver/src/middlewares/isAuthenticated.ts, update the binding and checks to reflect that
verifyRequestSignaturereturns a server name string (orundefined), not a boolean:- const isValid = await federationAuth.verifyRequestSignature( + const serverName = await federationAuth.verifyRequestSignature( authorizationHeader, method, uri, body, ); - if (!isValid) { + if (!serverName) { set.status = 401; return { authenticatedServer: null, }; } return { - authenticatedServer: isValid, + authenticatedServer: serverName, };
📜 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/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(1 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(0 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/index.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/middlewares/acl.middleware.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/homeserver/src/controllers/federation/media.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticated(4-68)packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessResource(312-330)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResource(25-66)
packages/homeserver/src/controllers/federation/invite.controller.ts (5)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticated(4-68)packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessResource(312-330)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResource(25-66)packages/homeserver/src/dtos/federation/invite.dto.ts (1)
ProcessInviteParamsDto(5-8)packages/homeserver/src/dtos/federation/profiles.dto.ts (1)
RoomVersionDto(57-69)
packages/homeserver/src/middlewares/canAccessResource.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessResource(312-330)packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)
packages/homeserver/src/middlewares/isAuthenticated.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/federation-sdk/src/services/federation-request.service.ts (1)
request(119-145)
packages/homeserver/src/controllers/federation/transactions.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticated(4-68)packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessResource(312-330)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResource(25-66)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticated(4-68)packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessResource(312-330)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResource(25-66)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 187-187: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (2)
packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
35-36: LGTM! Middleware order is correct.The middleware application order is appropriate - authentication (
isAuthenticated) runs before authorization (canAccessResource), which is the correct security flow.packages/homeserver/src/middlewares/isAuthenticated.ts (1)
23-30: No request body consumption conflictThis middleware is the sole consumer of the request body, so manually parsing with
request.text()/JSON.parsewon’t interfere with other handlers.
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
50-50: Plugin name mismatch already flagged.The plugin name 'homeserver/canAccessEvent' should be 'homeserver/canAccessResource' for consistency, but this has already been identified and acknowledged in previous review comments.
🧹 Nitpick comments (4)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
53-90: Consider consolidating the two/_matrixgroups.You have two separate
.group('/_matrix', ...)calls (lines 26 and 53). While this works, consolidating them into a single group would improve readability and reduce nesting.Optional refactor:
return app - .group('/_matrix', (app) => - app.use(isAuthenticatedMiddleware(eventAuthService)).put( - '/_matrix/federation/v1/send/:txnId', - ... - ), - ) - .group('/_matrix', (app) => - app.use(canAccessResourceMiddleware(eventAuthService, 'event')).get( - '/_matrix/federation/v1/event/:eventId', - ... - ), - ); + .group('/_matrix', (app) => + app + .group('/federation/v1/send', (sendApp) => + sendApp.use(isAuthenticatedMiddleware(eventAuthService)).put( + '/:txnId', + ... + ), + ) + .group('/federation/v1/event', (eventApp) => + eventApp.use(canAccessResourceMiddleware(eventAuthService, 'event')).get( + '/:eventId', + ... + ), + ), + );packages/homeserver/src/controllers/federation/rooms.controller.ts (2)
53-87: Incompleteroom_typesfilter implementation.The TODO comment on lines 76-78 indicates that
room_typesfiltering is not implemented. This means clients requesting specific room types will receive unfiltered results.Do you want me to generate an implementation for the
room_typesfilter or open a tracking issue?
88-115: Remove inline JSON comment.Line 89 contains an inline JSON example comment. While helpful during development, consider moving this to a JSDoc comment above the route or removing it entirely, as the schema is self-documenting.
}, { - // {"filter":{"generic_search_term":"","room_types":[null]},"include_all_networks":"false","limit":50} body: t.Object({packages/homeserver/src/middlewares/isAuthenticated.ts (1)
51-57: Consider structured logging for production observability.The middleware uses
console.errorfor error logging. For production environments, consider using a structured logger (e.g., pino, winston) that can be injected via the factory pattern, enabling better observability and log aggregation.Example refactor if a logger is available:
} catch (error) { - console.error('Authentication error:', error); + logger.error({ err: error, method, uri }, 'Authentication error'); set.status = 500; return { authenticatedServer: null, }; }
📜 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/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 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(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(2 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/homeserver/src/controllers/federation/transactions.controller.ts (4)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)packages/homeserver/src/dtos/federation/transactions.dto.ts (5)
SendTransactionBodyDto(8-21)SendTransactionResponseDto(23-30)GetEventParamsDto(32-34)GetEventResponseDto(36-42)GetEventErrorResponseDto(44-47)packages/homeserver/src/dtos/common/error.dto.ts (1)
ErrorResponseDto(3-6)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(46-82)
packages/homeserver/src/middlewares/isAuthenticated.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/federation-sdk/src/services/federation-request.service.ts (1)
request(119-145)
packages/homeserver/src/controllers/federation/media.controller.ts (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(46-82)
packages/homeserver/src/controllers/federation/state.controller.ts (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(46-82)
packages/homeserver/src/middlewares/canAccessResource.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)
packages/homeserver/src/controllers/federation/profiles.controller.ts (4)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)packages/homeserver/src/dtos/federation/profiles.dto.ts (6)
QueryProfileQueryDto(9-11)QueryProfileResponseDto(13-16)QueryKeysBodyDto(18-22)QueryKeysResponseDto(24-28)GetDevicesParamsDto(30-32)GetDevicesResponseDto(34-50)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(46-82)
packages/homeserver/src/controllers/federation/send-join.controller.ts (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(46-82)
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(46-82)
🔇 Additional comments (17)
packages/homeserver/src/controllers/federation/state.controller.ts (1)
2-24: LGTM! Clean integration of authorization middleware.The addition of
EventAuthorizationServiceandcanAccessResourceMiddlewareproperly gates access to federation state endpoints. The middleware application follows the consistent pattern across other federation controllers.packages/homeserver/src/controllers/federation/send-join.controller.ts (1)
2-19: LGTM! Proper authorization gate for send_join endpoint.The middleware integration correctly protects the send_join federation endpoint with resource-based access control.
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
1-14: LGTM! Consistent authorization pattern for invite endpoint.The middleware properly gates the federation invite endpoint with room-based access control.
packages/homeserver/src/controllers/federation/profiles.controller.ts (2)
33-100: LGTM! Appropriate middleware layering for profile endpoints.The authenticated group correctly applies
isAuthenticatedMiddlewarefor profile queries, which don't require resource-specific access. The intentional 501 responses for E2EE endpoints (keys, devices) are properly documented.
102-163: LGTM! Correct resource-based protection for room operations.The application of
canAccessResourceMiddlewarefor make_join, get_missing_events, and event_auth endpoints correctly enforces room-based access control.packages/homeserver/src/middlewares/canAccessResource.ts (3)
6-44: LGTM! Proper entity ID extraction with clear error handling.The
extractEntityIdhelper correctly throws errors when required IDs are missing, addressing past review feedback. The entityType-driven extraction eliminates ambiguity when multiple IDs are present in params.
52-60: Defensive authentication check is acceptable.The authentication check here is technically redundant since
isAuthenticatedMiddlewarealready rejects unauthenticated requests, but it serves as a defensive safeguard and documents the expectation clearly.
62-80: LGTM! Proper resource access validation and error handling.The resource access check correctly calls
canAccessResourceand handles both authorization failures (M_FORBIDDEN) and unexpected errors (M_UNKNOWN) appropriately.packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
7-8: LGTM! Clean middleware imports.The new middleware imports are correctly structured and align with the refactoring goals.
packages/homeserver/src/controllers/federation/media.controller.ts (4)
1-5: LGTM! Correct imports for the new middleware architecture.
18-40: LGTM! Properly documented not-implemented endpoint.The config endpoint correctly returns a 404 with the
M_UNRECOGNIZEDerror code and includes appropriate OpenAPI metadata.
63-149: LGTM! Comprehensive parameter validation for media routes.The media download and thumbnail endpoints include proper validation for path parameters and query strings, with reasonable constraints (e.g., width/height limits, method enum). The 404 responses are consistent and well-documented.
41-61: extractEntityId handles “media” entityType correctly
TheextractEntityIdfunction already includes a'media'branch that extractsparams.mediaIdand errors if missing (lines 25–32). No changes required.packages/homeserver/src/middlewares/isAuthenticated.ts (4)
1-6: LGTM: Clean factory pattern with proper dependency injection.The middleware factory correctly accepts the EventAuthorizationService dependency and maintains type safety.
7-9: LGTM: Proper Elysia instance creation.The named instance follows Elysia best practices and aids in debugging.
59-69: LGTM: Proper Matrix protocol error handling.The error responses correctly use Matrix protocol error codes (
M_UNAUTHORIZED,M_UNKNOWN) and differentiate between authentication failures and internal errors without leaking sensitive details.
34-50: No action required:verifyRequestSignaturereturnsPromise<string | undefined>, soisValidis the server identifier on success and falsy (undefined) on failure, matching the guard and return usage.Likely an incorrect or invalid review comment.
packages/homeserver/src/controllers/federation/transactions.controller.ts
Outdated
Show resolved
Hide resolved
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
cf0d010 to
23aff3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/homeserver/src/controllers/federation/rooms.controller.ts (2)
12-17: Extract duplicated defaultObj and fix "requried" typo.export const roomPlugin = (app: Elysia) => { const stateService = container.resolve(StateService); + const DEFAULT_PUBLIC_ROOM_FIELDS = { + join_rule: 'public', + // trying to reduce required endpoint hits + guest_can_join: false, + // ^^^ + world_readable: false, + // ?? don't have any yet + avatar_url: '', + }; return app .get( '/_matrix/federation/v1/publicRooms', async ({ query }) => { - const defaultObj = { - join_rule: 'public', - guest_can_join: false, // trying to reduce requried endpoint hits - world_readable: false, // ^^^ - avatar_url: '', // ?? don't have any yet - }; + const defaultObj = DEFAULT_PUBLIC_ROOM_FIELDS; ... async ({ body }) => { - const defaultObj = { - join_rule: 'public', - guest_can_join: false, // trying to reduce requried endpoint hits - world_readable: false, // ^^^ - avatar_url: '', // ?? don't have any yet - }; + const defaultObj = DEFAULT_PUBLIC_ROOM_FIELDS;Also applies to: 56-61
8-12: Require federation authentication for /_matrix/federation/v1/publicRooms (GET/POST).Other federation controllers in this PR use isAuthenticated; these endpoints should be gated similarly.
-import { StateService } from '@rocket.chat/federation-sdk'; +import { EventAuthorizationService, StateService } from '@rocket.chat/federation-sdk'; import { Elysia, t } from 'elysia'; import { container } from 'tsyringe'; +import { isAuthenticatedMiddleware } from '@rocket.chat/homeserver/middlewares/isAuthenticated'; export const roomPlugin = (app: Elysia) => { - const stateService = container.resolve(StateService); + const stateService = container.resolve(StateService); + const eventAuthService = container.resolve(EventAuthorizationService); - return app + return app + .use(isAuthenticatedMiddleware(eventAuthService))packages/homeserver/src/middlewares/isAuthenticated.ts (1)
25-32: Do not consume the original request body in middleware (clone before reading).Reading request.text() drains the fetch stream; downstream handlers will see an empty body. Clone the request and parse the clone instead.
- let body: Record<string, unknown> | undefined; - if (request.body) { - try { - const text = await request.text(); - body = text ? JSON.parse(text) : undefined; - } catch { - body = undefined; - } - } + let body: Record<string, unknown> | undefined; + try { + // Read from a clone so the original stream remains available downstream + const text = await request.clone().text().catch(() => ''); + if (text) { + try { + body = JSON.parse(text); + } catch { + body = undefined; // ignore parse errors + } + } + } catch { + body = undefined; + }packages/federation-sdk/src/services/event-authorization.service.ts (1)
249-257: Guard against missing room state from getFullRoomState().state can be undefined for unknown rooms; calling state.get(...) will throw and cause 500s. Short-circuit to deny access.
async serverHasAccessToResource( roomId: string, serverName: string, ): Promise<boolean> { - const state = await this.stateService.getFullRoomState(roomId); - - const aclEvent = state.get('m.room.server_acl:'); + const state = await this.stateService.getFullRoomState(roomId); + if (!state) { + this.logger.debug(`Room ${roomId} state not found`); + return false; + } + + const aclEvent = state.get('m.room.server_acl:');Based on learnings.
🧹 Nitpick comments (6)
packages/homeserver/src/middlewares/isAuthenticated.ts (3)
11-11: Prefer request.headers.get('Authorization') for case-insensitive lookup.Using the Headers API avoids relying on lower-cased copies and normalizes behavior across platforms.
- const authorizationHeader = headers.authorization; + const authorizationHeader = + request.headers.get('authorization') ?? request.headers.get('Authorization') ?? undefined;
34-50: Rename isValid to reflect returned value (server name).verifyRequestSignature returns the origin server string; naming it isValid is misleading.
- const isValid = await federationAuth.verifyRequestSignature( + const authenticatedServerName = await federationAuth.verifyRequestSignature( authorizationHeader, method, uri, body, ); - if (!isValid) { + if (!authenticatedServerName) { set.status = 401; return { authenticatedServer: null, }; } return { - authenticatedServer: isValid, + authenticatedServer: authenticatedServerName, };
51-53: Use project logger instead of console.error.Align with the logging approach used elsewhere (e.g., createLogger) to get structured logs and consistent formatting.
- } catch (error) { - console.error('Authentication error:', error); + } catch (error) { + // TODO: inject a logger or create one for this module + // logger.error(error, 'Authentication error');packages/homeserver/src/controllers/federation/rooms.controller.ts (2)
19-28: Make GET query params optional and honor limit.Avoid hard 400s on missing params and implement basic paging cap.
- const { limit: _limit } = query; + const limit = + typeof query.limit === 'number' + ? Math.max(1, Math.min(query.limit, 500)) + : undefined; ... - return { - chunk: publicRooms.map((room) => ({ + return { + chunk: (limit ? publicRooms.slice(0, limit) : publicRooms).map((room) => ({ ...defaultObj, ...room, })), }; ... - query: t.Object({ - include_all_networks: t.Boolean(), // we ignore this - limit: t.Number(), - }), + query: t.Object({ + include_all_networks: t.Optional(t.Boolean()), // ignored + limit: t.Optional(t.Number()), + }),Also applies to: 31-35
63-86: Harden POST filter and apply limit consistently.Guard against missing names and apply the optional body.limit.
- const publicRooms = await stateService.getAllPublicRoomIdsAndNames(); + const publicRooms = await stateService.getAllPublicRoomIdsAndNames(); + const limit = + typeof body.limit === 'number' + ? Math.max(1, Math.min(body.limit, 500)) + : undefined; ... - .filter((r) => { + .filter((r) => { if (filter.generic_search_term) { - return r.name - .toLowerCase() + return String(r.name ?? '') + .toLowerCase() .includes(filter.generic_search_term.toLowerCase()); } ... return true; }) - .map((room) => ({ + .slice(0, limit ?? undefined) + .map((room) => ({ ...defaultObj, ...room, })),Also applies to: 89-97
packages/federation-sdk/src/services/event-authorization.service.ts (1)
175-193: Add pattern length/complexity guard to prevent regex DoS.Even with escaping, attacker-controlled patterns like many wildcards can cause heavy backtracking. Reject overly long patterns and cap quantifiers.
private matchesServerPattern(serverName: string, pattern: string): boolean { + // Guard against pathological patterns + if (pattern.length > 512) { + this.logger.warn({ msg: 'ACL pattern too long', patternLength: pattern.length }); + return false; + } if (serverName === pattern) { return true; } let regexPattern = pattern .replace(/[.+^${}()|[\]\\]/g, '\\$&') .replace(/\*/g, '.*') .replace(/\?/g, '.'); regexPattern = `^${regexPattern}$`; try { - const regex = new RegExp(regexPattern); + const regex = new RegExp(regexPattern); return regex.test(serverName); } catch (error) { this.logger.warn({ msg: `Invalid ACL pattern: ${pattern}`, error }); return false; } }
📜 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 (11)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 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(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(6 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(0 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/middlewares/acl.middleware.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/homeserver/src/controllers/federation/transactions.controller.ts
- packages/homeserver/src/middlewares/canAccessResource.ts
- packages/homeserver/src/controllers/federation/send-join.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (5)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)packages/homeserver/src/dtos/federation/profiles.dto.ts (6)
QueryProfileQueryDto(9-11)QueryProfileResponseDto(13-16)QueryKeysBodyDto(18-22)QueryKeysResponseDto(24-28)GetDevicesParamsDto(30-32)GetDevicesResponseDto(34-50)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/controllers/federation/state.controller.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/middlewares/isAuthenticated.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/federation-sdk/src/services/federation-request.service.ts (1)
request(119-151)
packages/homeserver/src/controllers/federation/invite.controller.ts (2)
packages/federation-sdk/src/index.ts (2)
InviteService(61-61)EventAuthorizationService(56-56)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/controllers/federation/media.controller.ts (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 187-187: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: my-workflow
packages/federation-sdk/src/services/event-authorization.service.ts
[error] 275-279: File content differs from formatting output. Formatting check failed. Run the formatter (e.g., 'bun format' or the project-specific formatter) to fix code style issues in this file.
🔇 Additional comments (10)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
270-299: Run the project formatter and commit changes
CI indicates a formatting mismatch inpackages/federation-sdk/src/services/event-authorization.service.ts. Enable Corepack (corepack enable), ensurebun format(oryarn format) is available, then run the formatter and commit the updated file.packages/homeserver/src/controllers/federation/state.controller.ts (2)
2-6: LGTM: Imports and middleware integration.The imports are clean and properly scoped. The integration of
EventAuthorizationServiceandcanAccessResourceMiddlewarefollows the pattern established across other federation controllers.
21-24: LGTM: Middleware wiring.The DI resolution and middleware application are correct. Applying
canAccessResourceMiddleware(eventAuthService, 'room')before route definitions ensures that both state endpoints (state_ids and state) are properly gated by authentication and resource-access checks.packages/homeserver/src/controllers/federation/invite.controller.ts (2)
1-5: LGTM: Imports are clean.The imports correctly bring in
EventAuthorizationServiceandcanAccessResourceMiddlewareto support the new authorization flow.
12-14: LGTM: Middleware application.The DI resolution and chaining of
canAccessResourceMiddlewarebefore the PUT route is correct. This ensures the invite endpoint is protected by authentication and room-level resource checks.packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
102-102: LGTM: Room-level middleware application.Applying
canAccessResourceMiddleware(eventAuthService, 'room')after the authenticated group correctly gates the room-specific endpoints (make_join, get_missing_events, event_auth) with resource-level authorization.packages/homeserver/src/controllers/federation/media.controller.ts (4)
1-5: LGTM: Imports are properly structured.The imports correctly bring in the necessary authorization service and middlewares for the media endpoint protection.
21-40: LGTM: Unauthenticated config endpoint.The standalone GET
/media/v3/configendpoint outside the authenticated group is appropriate since it returns 404 regardless. The full path prefix is correct here since it's not inside the group.
63-149: LGTM: Comprehensive validation schemas.The media endpoints have proper parameter and query validation schemas:
- Path params correctly typed (serverName, mediaId)
- Query params properly validated (width, height with ranges, method as union, optional flags)
- Consistent 404 error responses across all endpoints
- Good route metadata (tags, summaries, descriptions)
The relative paths inside the
/_matrixgroup are correct and will not cause prefix duplication.
41-61: extractEntityId correctly handles mediaId; no action needed. extractEntityId(params, 'media') returns themediaIdfromparamsas expected.
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
23aff3d to
c581cc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
44-44: Fix naming inconsistency: update plugin name to 'homeserver/canAccessResource'.The plugin name still reads
'homeserver/canAccessEvent'but should be'homeserver/canAccessResource'to match the middleware function name.Apply this diff:
- return new Elysia({ name: 'homeserver/canAccessEvent' }) + return new Elysia({ name: 'homeserver/canAccessResource' })Based on past review comments.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
249-302: Add null check for room state before dereferencing.
getFullRoomStatecan returnundefinedwhen the room isn't known locally (e.g., remote probe for an unknown room, or media pointing to a purged room). Callingstate.get(...)on line 255 will throw, causing the middleware to return a 500 error instead of cleanly denying access.Apply this diff:
async serverHasAccessToResource( roomId: string, serverName: string, ): Promise<boolean> { const state = await this.stateService.getFullRoomState(roomId); + if (!state) { + this.logger.debug(`Room ${roomId} state not found, denying access`); + return false; + } const aclEvent = state.get('m.room.server_acl:');Based on past review comments.
🧹 Nitpick comments (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
48-54: Consider removing redundant authenticatedServer check.The
isAuthenticatedMiddleware(lines 56-66 in isAuthenticated.ts) already ensures that execution won't proceed ifauthenticatedServeris null—it returns early with an error response. This check on lines 48-54 is defensive but redundant.You can simplify by removing this block:
.onBeforeHandle(async ({ params, authenticatedServer, set }) => { try { - if (!authenticatedServer) { - set.status = errCodes.M_UNAUTHORIZED.status; - return { - errcode: errCodes.M_UNAUTHORIZED.errcode, - error: 'Authentication required', - }; - } - const resourceAccess = await federationAuth.canAccessResource( entityType, extractEntityId(params, entityType), authenticatedServer, );However, keeping it for defense-in-depth is acceptable.
📜 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/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (2)
packages/homeserver/src/middlewares/canAccessResource.ts (4)
packages/room/src/manager/event-wrapper.ts (1)
roomId(78-80)packages/room/src/manager/v3.ts (1)
eventId(17-29)packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(56-56)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/manager/event-wrapper.ts (3)
roomId(78-80)event(108-117)stateKey(96-100)packages/room/src/types/_common.ts (1)
EventID(8-8)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 187-187: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
328-346: LGTM: Clean resource access dispatcher.The
canAccessResourcemethod provides a clean entry point for the middleware, properly routing to entity-specific checks. The type cast on line 334 is safe given the middleware'sextractEntityIdensures correct ID types for each entity type.
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
c581cc0 to
57b3382
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/homeserver/src/controllers/federation/send-join.controller.ts (1)
19-33: Fix body exhaustion after middleware integration.Line [19]:
canAccessResourceMiddlewarewrapsisAuthenticatedMiddleware, which callsrequest.text()for signature verification and consumes the request stream. Once this middleware runs,bodyis alwaysundefinedhere, sosendJoinService.sendJoinprocesses an empty payload. Cache the parsed request body insideisAuthenticatedMiddleware(e.g., exposeparsedBody) and read that cached payload in this handler.- return app.use(canAccessResourceMiddleware(eventAuthService, 'room')).put( + return app.use(canAccessResourceMiddleware(eventAuthService, 'room')).put( '/_matrix/federation/v2/send_join/:roomId/:eventId', - async ({ - params, - body, - query: _query, // not destructuring this breaks the endpoint - }) => { + async ({ + params, + parsedBody, + body, + query: _query, // not destructuring this breaks the endpoint + }) => { const { roomId, eventId } = params; return sendJoinService.sendJoin( roomId as RoomID, eventId as EventID, - body as any, + (parsedBody ?? body) as SendJoinEventDto, ); },Also update
isAuthenticatedMiddlewareto returnparsedBodyalongsideauthenticatedServer.
♻️ Duplicate comments (9)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
29-55: Reuse the cached request body instead of the exhausted stream.Line [32]: This is the same regression previously reported—the new
isAuthenticatedMiddlewareconsumes the request stream viarequest.text(), sobodyis empty when we reach the handler. As a resultprocessIncomingTransactionreceives{}and the transaction fails. Please reuse the parsed payload that the middleware should expose..put( '/_matrix/federation/v1/send/:txnId', - async ({ body }) => { + async ({ body, parsedBody }) => { // TODO need to validate better the payload // biome-ignore lint/suspicious/noExplicitAny: - await eventService.processIncomingTransaction(body as any); + const payload = (parsedBody ?? body) as SendTransactionBodyDto; + // biome-ignore lint/suspicious/noExplicitAny: + await eventService.processIncomingTransaction(payload as any);Remember to extend
isAuthenticatedMiddlewareto stashparsedBodyin the context so downstream handlers can depend on it.packages/homeserver/src/controllers/federation/rooms.controller.ts (2)
12-26: Deduplicate the public-room defaults and fix the typo.Lines [12-26] and [56-61]: We’re still duplicating
defaultObj(with the “requried” typo) in both handlers, exactly as noted earlier—please hoist the structure once and reuse it.+const DEFAULT_PUBLIC_ROOM_FIELDS = { + join_rule: 'public', + guest_can_join: false, // trying to reduce required endpoint hits + world_readable: false, // ^^^ + avatar_url: '', // ?? don't have any yet +}; + export const roomPlugin = (app: Elysia) => { @@ - const defaultObj = { - join_rule: 'public', - guest_can_join: false, // trying to reduce requried endpoint hits - world_readable: false, // ^^^ - avatar_url: '', // ?? don't have any yet - }; + const defaultObj = DEFAULT_PUBLIC_ROOM_FIELDS; @@ - const defaultObj = { - join_rule: 'public', - guest_can_join: false, // trying to reduce requried endpoint hits - world_readable: false, // ^^^ - avatar_url: '', // ?? don't have any yet - }; + const defaultObj = DEFAULT_PUBLIC_ROOM_FIELDS;This keeps both handlers aligned and fixes the typo.
Also applies to: 56-61
8-115: Enforce federation authentication on/publicRooms.Lines [8-115]: Both federation
/publicRoomsendpoints still return data without verifying the calling server’s signature. The Matrix federation spec marks GET/POST/_matrix/federation/v1/publicRoomsas “Requires authentication: Yes”, so this change leaves the routes open to unauthenticated callers. Resolve by injectingEventAuthorizationServicehere and applyingisAuthenticatedMiddleware(eventAuthService)(orcanAccessResourceMiddlewareif you want per-room checks) before the GET/POST handlers so only authenticated servers can list rooms. Example:-import { StateService } from '@rocket.chat/federation-sdk'; +import { + EventAuthorizationService, + StateService, +} from '@rocket.chat/federation-sdk'; +import { isAuthenticatedMiddleware } from '@rocket.chat/homeserver/middlewares/isAuthenticated'; @@ export const roomPlugin = (app: Elysia) => { const stateService = container.resolve(StateService); + const eventAuthService = container.resolve(EventAuthorizationService); - return app + return app + .use(isAuthenticatedMiddleware(eventAuthService)) .get(Apply the same middleware to the POST route.
packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
41-104: Use group-relative paths to avoid/_matrix/_matrix/....Lines [41-104]: Inside
.group('/_matrix', …)the handlers still declare/_matrix/federation/..., so the effective routes become/_matrix/_matrix/...and none of these endpoints will resolve. This mirrors the previous review feedback—please switch to group-relative paths.- .get( - '/_matrix/federation/v1/query/profile', + .get( + '/federation/v1/query/profile', @@ - .post( - '/_matrix/federation/v1/user/keys/query', + .post( + '/federation/v1/user/keys/query', @@ - .get( - '/_matrix/federation/v1/user/devices/:userId', + .get( + '/federation/v1/user/devices/:userId',(You can also drop the leading slash if you prefer.)
packages/homeserver/src/middlewares/canAccessResource.ts (2)
44-44: Fix the plugin name to match the middleware function.The Elysia plugin name is still set to
'homeserver/canAccessEvent'but should be'homeserver/canAccessResource'to match the middleware function name.Apply this diff:
- return new Elysia({ name: 'homeserver/canAccessEvent' }) + return new Elysia({ name: 'homeserver/canAccessResource' })Based on past review comments.
6-38: Improve error handling for missing resource identifiers.The
extractEntityIdfunction throws errors when required parameters are missing, which are caught by the middleware's catch block (lines 68-73) and returned as generic 500 errors (M_UNKNOWN). This masks the actual problem from the caller—a missing parameter is a client error (400) not a server error (500).Additionally, if route params accidentally contain multiple IDs (e.g., both
roomIdandeventId), the priority order could cause incorrect extraction.Apply this diff to return
nullfor missing IDs and handle them explicitly:function extractEntityId( params: { roomId?: string; mediaId?: string; eventId?: string }, entityType: 'event' | 'media' | 'room', -): string { +): string | null { if (entityType === 'room') { - const roomId = params.roomId; - if (!roomId) { - throw new Error('Room ID is required'); - } - - return roomId; + return params.roomId ?? null; } if (entityType === 'media') { - const mediaId = params.mediaId; - if (!mediaId) { - throw new Error('Media ID is required'); - } - - return mediaId; + return params.mediaId ?? null; } if (entityType === 'event') { - const eventId = params.eventId; - if (!eventId) { - throw new Error('Event ID is required'); - } - - return eventId; + return params.eventId ?? null; } - throw new Error('Invalid entity type'); + return null; }Then update the middleware to handle
nullexplicitly:try { if (!authenticatedServer) { set.status = errCodes.M_UNAUTHORIZED.status; return { errcode: errCodes.M_UNAUTHORIZED.errcode, error: 'Authentication required', }; } + const resourceId = extractEntityId(params, entityType); + if (!resourceId) { + set.status = 400; + return { + errcode: 'M_INVALID_PARAM', + error: `Missing required ${entityType} identifier`, + }; + } + const resourceAccess = await federationAuth.canAccessResource( entityType, - extractEntityId(params, entityType), + resourceId, authenticatedServer, );Based on past review comments.
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
23-32: Fix body stream consumption to avoid breaking downstream handlers.Calling
await request.text()consumes the fetch body stream, which can only be read once. Controllers that destructurebodyfrom the route handler context (e.g., transactions.controller.ts, profiles.controller.ts) will receiveundefinedbecause the stream has already been drained.Apply this diff to clone the request before reading the body:
try { let body: Record<string, unknown> | undefined; if (request.body) { try { - const text = await request.text(); + const clone = request.clone(); + const text = await clone.text(); body = text ? JSON.parse(text) : undefined; } catch { body = undefined; } }Based on past review comments.
packages/federation-sdk/src/services/event-authorization.service.ts (2)
253-302: Add null check for room state before dereferencing.
getFullRoomStatecan returnundefinedwhen the room isn't known locally (e.g., a remote homeserver probes an event/room that was never seen, or media references a purged room). Callingstate.get(...)onundefinedwill throw, causing the middleware to return a generic 500 error instead of cleanly denying access.Apply this diff to check for
undefinedstate:async serverHasAccessToResource( roomId: string, serverName: string, ): Promise<boolean> { const state = await this.stateService.getFullRoomState(roomId); + if (!state) { + this.logger.debug(`Room ${roomId} state not found`); + return false; + } const aclEvent = state.get('m.room.server_acl:');Based on past review comments.
175-194: Mitigate Regular Expression Denial of Service (ReDoS) vulnerability.The regex pattern is constructed from ACL event content, which could be malicious. While special characters are escaped, patterns with nested quantifiers (e.g.,
"a*a*a*a*a*a*b") can still cause exponential backtracking, leading to ReDoS attacks that freeze the server.As per static analysis hints.
Apply this diff to validate pattern complexity:
private matchesServerPattern(serverName: string, pattern: string): boolean { if (serverName === pattern) { return true; } + // Limit pattern complexity to prevent ReDoS + if (pattern.length > 100 || (pattern.match(/[*?]/g) || []).length > 10) { + this.logger.warn(`ACL pattern too complex, rejecting: ${pattern}`); + return false; + } + let regexPattern = pattern .replace(/[.+^${}()|[\]\\]/g, '\\$&') .replace(/\*/g, '.*') .replace(/\?/g, '.');Alternatively, consider using a library like
safe-regexto validate patterns before constructing the regex.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
270-284: Consider performance optimization for invite checks.The loop iterates through all state events to check for pending invites. For rooms with large state maps (thousands of members), this could become a performance bottleneck.
Consider adding an index or caching mechanism in
StateServiceto quickly lookup invite events by server, or limit the iteration to only membership events:// Pseudocode - implementation depends on StateService structure const inviteEvents = await this.stateService.getMembershipEventsForServer(roomId, serverName); for (const event of inviteEvents) { if (event.getContent().membership === 'invite') { return true; } }
📜 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 (11)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 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(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(6 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(0 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/middlewares/acl.middleware.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/controllers/federation/invite.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (8)
packages/homeserver/src/middlewares/isAuthenticated.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(62-62)packages/federation-sdk/src/services/federation-request.service.ts (1)
request(119-151)
packages/homeserver/src/controllers/federation/state.controller.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(62-62)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/middlewares/canAccessResource.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
roomId(78-80)packages/room/src/manager/v3.ts (1)
eventId(17-29)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)
packages/homeserver/src/controllers/federation/media.controller.ts (1)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/controllers/federation/send-join.controller.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(62-62)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)packages/homeserver/src/dtos/federation/profiles.dto.ts (6)
QueryProfileQueryDto(9-11)QueryProfileResponseDto(13-16)QueryKeysBodyDto(18-22)QueryKeysResponseDto(24-28)GetDevicesParamsDto(30-32)GetDevicesResponseDto(34-50)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(40-76)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-70)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
packages/room/src/manager/event-wrapper.ts (3)
roomId(78-80)event(106-115)stateKey(96-98)packages/room/src/manager/v3.ts (1)
eventId(17-29)packages/room/src/types/_common.ts (1)
EventID(8-8)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 187-187: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: my-workflow
packages/homeserver/src/controllers/federation/profiles.controller.ts
[error] 1-1: File content differs from formatting output. Formatting tool reports differences in this file.
🔇 Additional comments (5)
packages/homeserver/src/controllers/federation/media.controller.ts (1)
44-149: LGTM!The media endpoints are properly protected with authentication and resource-level access controls. Parameter validation schemas are appropriately defined with proper types and constraints.
packages/homeserver/src/middlewares/canAccessResource.ts (1)
45-75: Verify type safety with isAuthenticatedMiddleware integration.The middleware correctly uses
isAuthenticatedMiddlewareto provide theauthenticatedServerproperty, ensuring proper typing without relying onany.packages/homeserver/src/middlewares/isAuthenticated.ts (1)
4-69: Verify authentication flow and error handling.The middleware correctly:
- Derives
authenticatedServerglobally via signature verification- Returns appropriate error responses (401 for unauthorized, 500 for internal errors)
- Uses
onBeforeHandleto gate requests without valid authenticationpackages/federation-sdk/src/services/event-authorization.service.ts (2)
118-123: LGTM!The signature update to include
uriandbodyparameters aligns with the Matrix federation specification for request signature verification.
304-346: LGTM!The centralized resource access control flow properly delegates to entity-specific checks and leverages the shared
serverHasAccessToResourcemethod. The implementation correctly handles events, media, and rooms.
packages/homeserver/src/controllers/federation/media.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/manager/event-wrapper.ts (3)
roomId(78-80)event(106-115)stateKey(96-98)packages/room/src/types/_common.ts (1)
EventID(8-8)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 192-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
| if (pattern.length > 200 || (pattern.match(/[*?]/g) || []).length > 20) { | ||
| this.logger.warn(`ACL pattern too complex, rejecting: ${pattern}`); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| async canAccessEventFromAuthorizationHeader( | ||
| eventId: EventID, | ||
| authorizationHeader: string, | ||
| method: string, | ||
| uri: string, | ||
| body?: Record<string, unknown>, | ||
| ): Promise< | ||
| | { authorized: true } | ||
| | { | ||
| authorized: false; | ||
| errorCode: 'M_UNAUTHORIZED' | 'M_FORBIDDEN' | 'M_UNKNOWN'; | ||
| } | ||
| > { | ||
| try { | ||
| const signatureResult = await this.verifyRequestSignature( | ||
| method, | ||
| uri, | ||
| authorizationHeader, | ||
| body, // keep body due to canonical json validation | ||
| ); | ||
| if (!signatureResult) { | ||
| return { | ||
| authorized: false, | ||
| errorCode: 'M_UNAUTHORIZED', | ||
| }; | ||
| } | ||
| let regexPattern = pattern | ||
| .replace(/[.+^${}()|[\]\\]/g, '\\$&') | ||
| .replace(/\*/g, '.*') | ||
| .replace(/\?/g, '.'); | ||
|
|
||
| const authorized = await this.canAccessEvent(eventId, signatureResult); | ||
| if (!authorized) { | ||
| return { | ||
| authorized: false, | ||
| errorCode: 'M_FORBIDDEN', | ||
| }; | ||
| } | ||
| regexPattern = `^${regexPattern}$`; | ||
|
|
||
| return { | ||
| authorized: true, | ||
| }; | ||
| try { | ||
| const regex = new RegExp(regexPattern); | ||
| return regex.test(serverName); | ||
| } catch (error) { | ||
| this.logger.error( | ||
| { error, eventId, authorizationHeader, method, uri, body }, | ||
| 'Error checking event access', | ||
| ); | ||
| return { | ||
| authorized: false, | ||
| errorCode: 'M_UNKNOWN', | ||
| }; | ||
| this.logger.warn({ msg: `Invalid ACL pattern: ${pattern}`, error }); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
ReDoS mitigation is still insufficient.
Even with the length and wildcard limits, crafted patterns like a*a*a*a*a*a*a*a*a*a* remain below the thresholds yet still trigger catastrophic backtracking. Because these ACL patterns come from remote federation input, we need a stronger safeguard (e.g., run safe-regex against the final pattern, or switch to a backtracking-free matcher) before constructing the RegExp. Without that, a malicious homeserver can still hang our process. Please harden this path.
🧰 Tools
🪛 ast-grep (0.39.5)
[warning] 192-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
packages/federation-sdk/src/services/event-authorization.service.ts around lines
180-198: the current RegExp construction is vulnerable to ReDoS even with
length/wildcard caps; before creating the RegExp either (a) validate the final
regexPattern with a ReDoS checker (e.g., run safe-regex on the escaped/converted
pattern and reject+log if unsafe) or (b) use a backtracking-free engine (e.g.,
construct the regex with the RE2 library and fall back to rejecting the pattern
if RE2 cannot compile it). Implement one of these: call safe-regex on
regexPattern and return false/log when it reports unsafe, or switch to using RE2
to build and test the pattern (with try/catch and a clear warn+reject path);
ensure any rejected/unsafe pattern logs the pattern and reason and never passes
it to the native RegExp constructor.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/homeserver/src/controllers/federation/rooms.controller.ts (1)
18-23: Extract duplicateddefaultObjconstant.The
defaultObjis duplicated between the GET handler (lines 18-23) and POST handler (lines 62-67). Extract it to a module-level constant to follow DRY principles.Additionally, both instances have a typo: "requried" should be "required" on lines 20 and 64.
Apply this diff:
export const roomPlugin = (app: Elysia) => { const stateService = container.resolve(StateService); const eventAuthService = container.resolve(EventAuthorizationService); + + const defaultObj = { + join_rule: 'public', + guest_can_join: false, // trying to reduce required endpoint hits + world_readable: false, // ^^^ + avatar_url: '', // ?? don't have any yet + }; return app .use(isAuthenticatedMiddleware(eventAuthService)) .get( '/_matrix/federation/v1/publicRooms', async ({ query }) => { - const defaultObj = { - join_rule: 'public', - guest_can_join: false, // trying to reduce requried endpoint hits - world_readable: false, // ^^^ - avatar_url: '', // ?? don't have any yet - };Apply the same removal in the POST handler.
Also applies to: 62-67
🧹 Nitpick comments (2)
packages/homeserver/src/controllers/federation/rooms.controller.ts (1)
82-84: Complete the room_types filter implementation.The TODO comment indicates incomplete filtering logic for
room_types. This could affect the accuracy of search results when clients request specific room types.Would you like me to generate the implementation for the
room_typesfilter, or open an issue to track this task?packages/federation-sdk/src/services/event-authorization.service.ts (1)
180-198: ReDoS mitigation helps but consider additional safeguards.The current complexity limits (200 chars, 20 wildcards) reduce the attack surface, but patterns like
a*a*a*a*a*a*a*a*a*a*b(under the limits) can still cause exponential backtracking. Since ACL patterns come from federation input, consider adding one of these stronger safeguards:
- Use a ReDoS-safe library like
safe-regexorrecheckto validate patterns before constructing the regex- Add a timeout mechanism for regex execution
- Switch to a backtracking-free engine (e.g., RE2)
Example using
safe-regex:import safeRegex from 'safe-regex'; // Before line 192: if (!safeRegex(regexPattern)) { this.logger.warn(`Unsafe ACL pattern detected: ${pattern}`); return false; }As per static analysis hints.
📜 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 (3)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/controllers/federation/rooms.controller.ts(1 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/middlewares/canAccessResource.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (2)
packages/homeserver/src/controllers/federation/rooms.controller.ts (2)
packages/federation-sdk/src/index.ts (2)
StateService(72-72)EventAuthorizationService(62-62)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
packages/room/src/manager/event-wrapper.ts (3)
roomId(78-80)event(106-115)stateKey(96-98)packages/federation-sdk/src/index.ts (1)
EventID(26-26)packages/room/src/types/_common.ts (1)
EventID(8-8)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 192-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (4)
packages/homeserver/src/controllers/federation/rooms.controller.ts (1)
1-14: Authentication middleware successfully added!The integration of
isAuthenticatedMiddlewareproperly addresses the federation authentication requirement flagged in previous reviews. The middleware is correctly applied to the route chain and will enforce server-to-server authentication for both publicRooms endpoints.packages/federation-sdk/src/services/event-authorization.service.ts (3)
118-123: LGTM: verifyRequestSignature properly exposed.Making this method public enables the
isAuthenticatedMiddlewareto verify federation request signatures. The signature change to includeuriandbodyparameters provides the necessary context for complete authentication validation.
254-316: Excellent centralized authorization logic!The
serverHasAccessToResourcemethod consolidates all federation access checks:
- ACL validation (lines 264-271)
- Server membership checks (lines 273-277)
- Pending invite detection with proper guards (lines 279-298)
- World-readable fallback (lines 300-310)
- Safe state handling (lines 258-262)
This centralization improves maintainability and consistency across the codebase. The guard conditions at lines 284-286 properly handle malformed membership events, addressing previous concerns.
318-360: Well-structured public API for resource access control.The new public methods provide a clean, consistent interface:
canAccessEvent,canAccessMedia,canAccessRoomdelegate to the centralizedserverHasAccessToResourcecanAccessResourceprovides a unified entry point for middleware integration- The entity-type routing pattern (lines 347-359) is clear and extensible
This design enables the
canAccessResourceMiddlewareto enforce authorization uniformly across all federation endpoints.
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
559b0cc to
b03f59e
Compare
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 (1)
packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
129-170: Federation endpoints lack authentication and authorization.The
get_missing_eventsandevent_authendpoints handle sensitive room data but are not protected by any authentication or authorization middleware. Remote servers can query these endpoints without proving their identity or room membership.Apply protection by wrapping these routes with
canAccessResourceMiddlewareor moving them inside the authenticated group:) + .use(canAccessResourceMiddleware(eventAuthService, 'room')) .post( '/_matrix/federation/v1/get_missing_events/:roomId',
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
175-199: ReDoS mitigation remains insufficient despite complexity limits.While the pattern length and wildcard count limits (lines 180-182) reduce the attack surface, crafted patterns like
a*a*a*a*a*a*a*a*a*a*remain below thresholds yet still trigger catastrophic backtracking. Since ACL patterns originate from federation input (remote homeservers), a malicious server can hang the process.Based on past review comments, the recommended mitigations are:
- Integrate
safe-regexto validate the finalregexPatternbefore constructing the RegExp, or- Use the RE2 library (backtracking-free engine) instead of native RegExp
As per static analysis hints.
🧹 Nitpick comments (4)
packages/homeserver/src/controllers/federation/transactions.controller.ts (2)
57-93: Enforce resource-level access on GET /event.Guard by event access, not only auth. Replace route-level
usewith canAccessResource('event').- use: isAuthenticatedMiddleware(eventAuthService), + use: canAccessResourceMiddleware(eventAuthService, 'event'),Rationale: ensures the requesting server is authorized to read the specific event, aligning with the new centralized resource checks. Please confirm service-layer checks aren’t already enforcing this to avoid duplication.
95-145: Enforce resource-level access on backfill.Backfill should require room access. Replace route-level
usewith canAccessResource('room').- use: isAuthenticatedMiddleware(eventAuthService), + use: canAccessResourceMiddleware(eventAuthService, 'room'),This ensures only permitted servers can read history for the given room.
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
2-6: Add room-level authorization to invite.Invites should validate server access to the target room, not just authentication. Chain canAccessResource('room') after isAuthenticated.
import { EventAuthorizationService, InviteService, } from '@rocket.chat/federation-sdk'; +import { canAccessResourceMiddleware } from '@rocket.chat/homeserver/middlewares/canAccessResource'; import { isAuthenticatedMiddleware } from '@rocket.chat/homeserver/middlewares/isAuthenticated'; @@ -return app.use(isAuthenticatedMiddleware(eventAuthService)).put( +return app + .use(isAuthenticatedMiddleware(eventAuthService)) + .use(canAccessResourceMiddleware(eventAuthService, 'room')) + .put(If EventAuthorizationService.processInvite already enforces room authorization, feel free to skip to avoid double checks.
Also applies to: 15-16
packages/homeserver/src/middlewares/canAccessResource.ts (1)
1-4: Optional: unify Elysia import style.Other files use named import. For consistency:
-import Elysia from 'elysia'; +import { Elysia } from 'elysia';
📜 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 (11)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 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(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(6 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(0 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/middlewares/acl.middleware.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/homeserver/src/middlewares/isAuthenticated.ts
- packages/homeserver/src/controllers/federation/state.controller.ts
- packages/homeserver/src/controllers/federation/rooms.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (7)
packages/homeserver/src/controllers/federation/send-join.controller.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(63-63)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/media.controller.ts (3)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(63-63)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/invite.controller.ts (2)
packages/federation-sdk/src/index.ts (2)
InviteService(68-68)EventAuthorizationService(63-63)packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/homeserver/src/dtos/federation/profiles.dto.ts (6)
QueryProfileQueryDto(9-11)QueryProfileResponseDto(13-16)QueryKeysBodyDto(18-22)QueryKeysResponseDto(24-28)GetDevicesParamsDto(30-32)GetDevicesResponseDto(34-50)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/manager/event-wrapper.ts (2)
roomId(100-102)event(128-137)packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 192-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (4)
packages/homeserver/src/controllers/federation/send-join.controller.ts (1)
19-20: Good authorization gate placement.Applying canAccessResource('room') before send-join is correct and matches params (roomId). Looks solid.
packages/homeserver/src/controllers/federation/media.controller.ts (1)
22-43: Config now behind authentication — good.Placing /media/v3/config under isAuthenticated while keeping other media routes behind canAccessResource('media') is consistent and safe.
packages/homeserver/src/middlewares/canAccessResource.ts (1)
29-69: Solid middleware: clear 401/400/403/500 paths and param extraction.Auth chaining and explicit error codes look correct. Good defensive handling.
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
31-36: Forward txnId or confirm idempotency handling
The controller ignores the txnId URL param; ensure the service deduplicates incoming transactions by PDU/event IDs or forward txnId to processIncomingTransaction for idempotency tracking.
sampaiodiego
force-pushed
the
refactor/middlewares
branch
from
13a7ef9 to
b93b9d0
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/homeserver/src/controllers/federation/transactions.controller.ts (1)
78-128: Apply resource ACL middleware to event/backfill routes.Authentication alone isn’t enough here—we now have
canAccessResourceMiddlewarewired elsewhere to enforce room/event ACLs. GET/event/:eventIdshould usecanAccessResourceMiddleware(..., 'event'), and/backfill/:roomIdshould use the'room'variant so servers denied by ACLs can’t pull history. Right now both endpoints bypass those checks.
📜 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 (11)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 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(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(6 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(0 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/middlewares/acl.middleware.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/homeserver/src/middlewares/isAuthenticated.ts
- packages/homeserver/src/controllers/federation/state.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (8)
packages/homeserver/src/controllers/federation/media.controller.ts (2)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/manager/event-wrapper.ts (3)
roomId(100-102)event(128-137)stateKey(118-120)packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/homeserver/src/controllers/federation/send-join.controller.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(63-63)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/homeserver/src/dtos/federation/profiles.dto.ts (6)
QueryProfileQueryDto(9-11)QueryProfileResponseDto(13-16)QueryKeysBodyDto(18-22)QueryKeysResponseDto(24-28)GetDevicesParamsDto(30-32)GetDevicesResponseDto(34-50)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/rooms.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 192-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
packages/homeserver/src/controllers/federation/transactions.controller.ts
Outdated
Show resolved
Hide resolved
ricardogarim
force-pushed
the
refactor/middlewares
branch
2 times, most recently
from
8647fbd to
9500b55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
175-198: ReDoS risk in dynamic ACL regex (consider RE2/safe-regex)Compiling user-controlled patterns with RegExp can still backtrack catastrophically despite length/wildcard caps. Validate with a ReDoS checker or use a backtracking‑free engine (e.g., RE2) before compiling.
🧹 Nitpick comments (4)
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
18-20: Redundant guard after auth middlewareonBeforeHandle already halts unauthenticated requests. This check is unreachable and can be removed.
- if (!authenticatedServer) { - throw new Error('Missing authenticated server from request'); - }packages/federation-sdk/src/services/invite.service.ts (1)
205-211: Avoid string matching for ACL denialComparing error.message is brittle. Throw a domain error (e.g., AccessDeniedError) from checkAclForInvite and check with instanceof here.
- } catch (error) { - // check if this is an ACL error or authorization error, rethrow if so - if ( - error instanceof Error && - error.message.includes('Sender server denied by room ACL') - ) { - throw error; - } + } catch (error) { + if (error instanceof AccessDeniedError) { + throw error; + } // typical noop; sign and return }Additionally define and use a typed error in the auth service:
// e.g., packages/federation-sdk/src/services/errors.ts export class AccessDeniedError extends Error {} // in event-authorization.service.ts import { AccessDeniedError } from './errors'; // ... throw new AccessDeniedError('Sender server denied by room ACL');packages/homeserver/src/middlewares/isAuthenticated.ts (2)
24-33: Parse body only for JSON requestsGate body parsing on Content-Type to avoid unnecessary text reads/parsing for non-JSON requests.
- let body: Record<string, unknown> | undefined; - if (request.body) { + let body: Record<string, unknown> | undefined; + const contentType = request.headers.get('content-type') ?? ''; + if (request.body && contentType.includes('application/json')) { try { const clone = request.clone(); const text = await clone.text(); body = text ? JSON.parse(text) : undefined; } catch { body = undefined; } }
52-54: Prefer structured logger over consoleUse the project logger for consistency and observability instead of console.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 (5)
packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/federation-sdk/src/services/invite.service.ts(5 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(6 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/controllers/federation/transactions.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (4)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/manager/event-wrapper.ts (3)
roomId(100-102)event(128-137)stateKey(118-120)packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
request(123-155)
packages/federation-sdk/src/services/invite.service.ts (1)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(63-63)
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
🪛 ast-grep (0.39.5)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 192-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
ricardogarim
force-pushed
the
refactor/middlewares
branch
from
9cec8ad to
43673d2
Compare
sampaiodiego
force-pushed
the
refactor/middlewares
branch
from
3b2df39 to
bfbdaef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
78-79: Switched to canAccessResourceMiddleware('event') — resolves prior feedbackThis centralizes authZ and aligns with the new middleware strategy.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
261-274: Fix NPE: Missing null check after getLatestRoomState.
getLatestRoomStatecan returnundefinedwhen the room isn't known locally, but line 264 directly callsstate.get(...)without checking. This will throw a null pointer exception.Apply this diff:
async checkAclForInvite(roomId: string, senderServer: string): Promise<void> { const state = await this.stateService.getLatestRoomState(roomId); + if (!state) { + this.logger.debug(`Room ${roomId} state not found; skipping ACL check`); + return; + } const aclEvent = state.get('m.room.server_acl:'); if (!aclEvent) { return; }Based on past review comments.
🧹 Nitpick comments (2)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
43-44: Auth on /send via isAuthenticatedMiddleware — OKCloning the request in the middleware avoids stream exhaustion; handler can safely use body.
Optional: expose parsedBody from isAuthenticatedMiddleware to reuse here and skip a second JSON parse.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
182-206: Consider additional ReDoS protection despite current guards.While the length and wildcard limits reduce risk, patterns like
a*a*a*a*a*a*a*a*a*a*can still trigger catastrophic backtracking (10 alternating captures). Since ACL patterns come from remote federation input, consider one of:
- Validate with safe-regex before compiling (recommended for quick fix)
- Switch to RE2 engine (if available, for guaranteed linear time)
Option 1: Add safe-regex validation
Install the package:
npm install safe-regexThen update the code:
+import safeRegex from 'safe-regex'; + private matchesServerPattern(serverName: string, pattern: string): boolean { if (serverName === pattern) { return true; } if (pattern.length > 200 || (pattern.match(/[*?]/g) || []).length > 20) { this.logger.warn(`ACL pattern too complex, rejecting: ${pattern}`); return false; } let regexPattern = pattern .replace(/[.+^${}()|[\]\\]/g, '\\$&') .replace(/\*/g, '.*') .replace(/\?/g, '.'); regexPattern = `^${regexPattern}$`; + if (!safeRegex(regexPattern)) { + this.logger.warn(`Unsafe ACL pattern detected, rejecting: ${pattern}`); + return false; + } + try { const regex = new RegExp(regexPattern); return regex.test(serverName); } catch (error) { this.logger.warn({ msg: `Invalid ACL pattern: ${pattern}`, error }); return false; } }Based on past review comments and static analysis hints.
📜 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 (15)
packages/federation-sdk/src/index.ts(1 hunks)packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)packages/federation-sdk/src/services/invite.service.ts(4 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 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(2 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(6 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(0 hunks)packages/homeserver/src/middlewares/canAccessResource.ts(1 hunks)packages/homeserver/src/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/middlewares/acl.middleware.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/homeserver/src/middlewares/isAuthenticated.ts
- packages/homeserver/src/controllers/federation/media.controller.ts
- packages/homeserver/src/controllers/federation/rooms.controller.ts
- packages/homeserver/src/controllers/federation/send-join.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (9)
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)
packages/federation-sdk/src/services/invite.service.ts (1)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(63-63)
packages/homeserver/src/controllers/federation/state.controller.ts (2)
packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(63-63)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/homeserver/src/controllers/federation/transactions.controller.ts (2)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
packages/federation-sdk/src/index.ts (1)
packages/room/src/types/_common.ts (2)
EventID(8-8)PduForType(26-26)
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
packages/room/src/manager/event-wrapper.ts (3)
roomId(101-103)event(129-138)stateKey(119-121)packages/federation-sdk/src/index.ts (1)
EventID(27-27)packages/room/src/types/_common.ts (1)
EventID(8-8)
packages/homeserver/src/middlewares/canAccessResource.ts (1)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-71)packages/homeserver/src/dtos/federation/profiles.dto.ts (6)
QueryProfileQueryDto(9-11)QueryProfileResponseDto(13-16)QueryKeysBodyDto(18-22)QueryKeysResponseDto(24-28)GetDevicesParamsDto(30-32)GetDevicesResponseDto(34-50)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-70)
🪛 ast-grep (0.39.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 199-199: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (22)
packages/federation-sdk/src/index.ts (1)
286-289: New server_acl signature export looks consistentMatches the minimal shape used by power_levels (event_id + event). No issues spotted.
packages/federation-sdk/src/services/staging-area.service.ts (1)
334-340: Emitter wiring for m.room.server_acl is correctEmits homeserver.matrix.room.server_acl with event_id and event, aligning with the new signature.
packages/homeserver/src/controllers/federation/state.controller.ts (1)
2-5: State endpoints gated by room access middleware — goodcanAccessResourceMiddleware('room') before state routes is appropriate and consistent with the route params (:roomId).
Please confirm extractEntityId expects params.roomId for entityType 'room' as used here.
Also applies to: 6-6, 21-21, 24-24
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
127-128: Backfill gated by canAccessResourceMiddleware('room') — goodCorrectly enforces room-level access before serving backfill.
packages/federation-sdk/src/services/invite.service.ts (3)
32-39: LGTM: Clean dependency injection.The EventAuthorizationService is properly injected and will be used for centralized ACL checks.
145-150: LGTM: Public API extended for authentication.The
authenticatedServerparameter is correctly added to support ACL checks. Verified that the controller (invite.controller.ts) has been updated to pass this parameter.
171-184: No NPE risk in checkAclForInvite
getLatestRoomStatethrows if no state exists, sostateis always defined andstate.get(…)cannot beundefined—no change needed.Likely an incorrect or invalid review comment.
packages/homeserver/src/middlewares/canAccessResource.ts (3)
6-23: LGTM: Clean null-safe parameter extraction.The helper correctly returns
nullfor missing parameters, allowing the middleware to respond with proper error codes (M_INVALID_PARAM) instead of generic 500 errors.
25-31: LGTM: Middleware correctly chains authentication.The middleware properly chains
isAuthenticatedMiddlewarebefore checking resource access, ensuringauthenticatedServeris available with correct types.
32-69: LGTM: Comprehensive error handling with proper Matrix error codes.The error handling correctly returns:
- M_UNAUTHORIZED (401) when authentication is missing
- M_INVALID_PARAM (400) when resource ID is missing
- M_FORBIDDEN (403) when access is denied
- M_UNKNOWN (500) for unexpected errors
Note: The
authenticatedServercheck on line 33 is technically redundant sinceisAuthenticatedMiddlewarealready validates this, but it serves as defensive programming and doesn't hurt.packages/homeserver/src/controllers/federation/invite.controller.ts (2)
11-16: LGTM: Authentication middleware correctly wired.The
isAuthenticatedMiddlewareis properly applied to the route, ensuring request signature validation occurs before invite processing.
17-28: LGTM: Defensive validation before invite processing.The handler correctly validates that
authenticatedServeris present before passing it toprocessInvite. The explicit error provides clear debugging information if the middleware somehow fails to populate this value.packages/homeserver/src/controllers/federation/profiles.controller.ts (4)
1-31: LGTM: Clean imports and DI setup.All necessary dependencies are imported and properly resolved from the DI container.
32-51: LGTM: Profile query endpoint properly authenticated.The route is correctly grouped under
/_matrixwith authentication middleware, and the path is relative to avoid duplication.
52-101: LGTM: E2EE stubs properly return 501 Not Implemented.Both
/user/keys/queryand/user/devices/:userIdendpoints correctly return 501 status with clear error messages indicating E2EE is not yet implemented. The response schemas properly include the 501 status type.
103-170: LGTM: Room access middleware correctly applied.The
canAccessResourceMiddlewarewith'room'type is properly applied before the room-related endpoints (make_join,get_missing_events,event_auth), ensuring proper authorization checks.packages/federation-sdk/src/services/event-authorization.service.ts (6)
19-24: LGTM: Clear custom error for ACL denials.The
AclDeniedErrorclass provides a well-structured error for signaling ACL violations, with a clear message including both the server and room identifiers.
125-180: LGTM: Signature verification correctly exposed as public API.The method is now properly public for use by authentication middleware, with a clean parameter list and comprehensive validation logic.
208-259: LGTM: ACL evaluation correctly implements Matrix spec.The logic properly follows the Matrix specification for server ACLs:
- IP literal handling respects
allow_ip_literals- Deny list checked first
- Empty allow list correctly denies all servers
- Allow list checked with proper pattern matching
276-338: LGTM: Comprehensive centralized resource access logic.This method correctly implements multi-layered authorization:
- Validates room state exists (null check on line 281)
- Enforces ACL rules
- Checks server membership in room
- Checks for pending invites with proper guards (lines 306-308)
- Allows world_readable rooms
- Denies by default
The invite loop guards prevent NPEs from malformed membership events.
340-358: LGTM: Clean public access methods for events and media.Both methods are now properly exposed as public APIs for middleware usage and correctly delegate to the centralized
serverHasAccessToResourcelogic.
360-382: LGTM: Unified resource access interface for middleware.The
canAccessResourcemethod provides a clean, type-based routing interface that the middleware can use, with all entity types correctly handled.
As per FDR-159, this PR introduces two new middlewares for federation routes:
isAuthenticatedMiddleware./_matrix/federation/v1/event_auth/{roomId}/{eventId}/_matrix/federation/v1/backfill/{roomId}/_matrix/federation/v1/get_missing_events/{roomId}/_matrix/federation/v1/event/{eventId}/_matrix/federation/v1/make_join/{roomId}/{stateKey}/_matrix/federation/v1/send_join/{roomId}/{stateKey}/_matrix/federation/v2/invite/{roomId}/{eventId}/_matrix/federation/v1/publicRooms/_matrix/federation/v1/publicRooms/_matrix/federation/v1/query/profile/_matrix/federation/v1/user/devices/{userId}/_matrix/federation/v1/user/keys/query/_matrix/federation/v1/media/download/{mediaId}/_matrix/federation/v1/send/{txnId}/.well-known/matrix/server/_matrix/federation/v1/version* Note: The Server Access Control Lists (ACLs) section doesn’t explicitly mention enforcing ACLs on media, but we understand it as necessary and are applying it accordingly.
** Note: MP suggests handling ACLs and resource access per PDU/EDU internally rather than through middleware. We're delegating this responsibility to state resolution. TODO: Verify that all scenarios are properly covered.
*** Note: We're dealing with ACL on the state instead of a middleware. TODO: Verify that all scenarios are properly covered.
RC = Rocket.Chat; HS = Homeserver; MP = Matrix Protocol.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests/Docs