feat: adds event ACL #161
feat: adds event ACL #161
Conversation
ricardogarim
force-pushed
the
feat/acl-methods
branch
from
a179a53 to
177e9a6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
- Coverage 81.18% 81.10% -0.08%
==========================================
Files 63 63
Lines 4676 4683 +7
==========================================
+ Hits 3796 3798 +2
- Misses 880 885 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
feat/acl-methods
branch
from
c134ce8 to
f496982
Compare
ricardogarim
force-pushed
the
feat/acl-methods
branch
from
f496982 to
7162b7d
Compare
ricardogarim
changed the base branch from
main
to
feat/get-event-by-id-route
ricardogarim
changed the title
|
Caution Review failedThe pull request is closed. WalkthroughAdds cryptographic, header-based federation event authorization with server ACL and history visibility checks; wires EventAuthorizationService into SDK exports and homeserver routes via middleware; introduces standardized error codes; updates state service server extraction; and adds type guards for ACL/history visibility events. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor RS as Remote Server
participant HS as Homeserver Controller
participant MW as Middleware (canAccessEvent)
participant EAS as EventAuthorizationService
participant SS as StateService
participant ES as EventService
participant KR as KeyRepository
participant RS2 as Remote HS (Key fetch)
RS->>HS: GET /_matrix/federation/v1/event/:eventId + Authorization
HS->>MW: onBeforeHandle (eventId, headers, method, uri)
MW->>EAS: canAccessEventFromAuthorizationHeader(eventId, auth, method, uri)
EAS->>EAS: extractSignaturesFromHeader / validateAuthorizationHeader
alt signer key known locally
EAS->>KR: getPublicKey(server, keyId)
else fetch from remote
EAS->>RS2: GET /_matrix/key/v2/server
RS2-->>EAS: server keys
EAS->>KR: persist/update key
end
EAS->>ES: load event(eventId)
EAS->>SS: resolve room state (members, ACL, visibility)
EAS-->>MW: {authorized}|{authorized:false, errCode}
alt authorized
MW-->>HS: continue
HS->>ES: getEvent(eventId)
ES-->>HS: event
HS-->>RS: 200 OK + event
else unauthorized/forbidden/error
MW-->>RS: status (401/403/500) + {errcode, error}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (7)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
ricardogarim
changed the base branch from
feat/get-event-by-id-route
to
main
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/federation-sdk/src/services/staging-area.service.ts (3)
359-372: Avoid spreading untrusted event content; sanitize or whitelist keys before emit.Blindly spreading
stagedEvent.event.contentcan leak unexpected keys and enable prototype-pollution vectors (e.g.,__proto__). Sanitize first or whitelist allowed fields.Apply a minimal sanitization before spreading:
- content: { - ...stagedEvent.event.content, + const safe = Object.fromEntries( + Object.entries(stagedEvent.event.content ?? {}).filter( + ([k]) => k !== '__proto__' && k !== 'constructor' && k !== 'prototype', + ), + ); + content: { + ...safe, body: stagedEvent.event.content?.body as string, msgtype: stagedEvent.event.content?.msgtype as string, 'm.relates_to': stagedEvent.event.content?.['m.relates_to'] as { rel_type: 'm.replace' | 'm.annotation' | 'm.thread'; event_id: string; }, 'm.new_content': stagedEvent.event.content?.['m.new_content'] as { body: string; msgtype: string; }, formatted_body: (stagedEvent.event.content?.formatted_body || '') as string, },
506-515: -Bug:Set.prototype.differencedoes not exist (runtime error).This will throw at runtime on Node/JS. Replace with explicit symmetric difference.
-const setOrUnsetPowerLevels = new Set( - usersInNewPowerLevelEvent, -).difference(new Set(usersInOldPowerLevelEvent)); +const newSet = new Set(usersInNewPowerLevelEvent); +const oldSet = new Set(usersInOldPowerLevelEvent); +const setOrUnsetPowerLevels = new Set([ + ...[...newSet].filter((u) => !oldSet.has(u)), + ...[...oldSet].filter((u) => !newSet.has(u)), +]);
204-207: Don’t hardcode'm.room.message'for auth queries.Use the incoming event’s type; otherwise non-message events pass wrong auth context.
-const authEvents = await this.eventService.getAuthEventIds( - 'm.room.message', +const authEvents = await this.eventService.getAuthEventIds( + event.event.type, { roomId: event.roomId, senderId: event.event.sender }, );packages/federation-sdk/src/services/signature-verification.service.ts (3)
97-131: Public key fetch is insecure and has no TTL; also ignores old keys.
- Trusts key response without verifying the server key response per Matrix spec.
- No timeout; potential hang.
- Caches by
${server}:${keyId}but stores whole key set; wastes memory and misses reuse.- Ignores
old_verify_keys/expired_ts; breaks historical signature checks.interface KeyData { server_name: string; + valid_until_ts?: number; verify_keys: { [keyId: string]: { key: string; }; }; old_verify_keys?: { [keyId: string]: { key: string; expired_ts: number; }; }; } export class SignatureVerificationService { private get logger() { return createLogger('SignatureVerificationService'); } - private cachedKeys = new Map<string, KeyData>(); + private cachedKeys = new Map<string, { keyData: KeyData; fetchedAt: number }>(); @@ - async getOrFetchPublicKey( - serverName: string, - keyId: string, - ): Promise<KeyData | null | undefined> { - const cacheKey = `${serverName}:${keyId}`; + async getOrFetchPublicKey( + serverName: string, + keyId: string, + ): Promise<KeyData | null> { + const cacheKey = serverName; if (this.cachedKeys.has(cacheKey)) { - return this.cachedKeys.get(cacheKey); + return this.cachedKeys.get(cacheKey)!.keyData; } try { - const response = await fetch( - `https://${serverName}/_matrix/key/v2/server`, - ); + const controller = new AbortController(); + const t = setTimeout(() => controller.abort(), 5000); + const response = await fetch(`https://${serverName}/_matrix/key/v2/server`, { + headers: { Accept: 'application/json' }, + signal: controller.signal, + }); + clearTimeout(t); if (!response.ok) { this.logger.error( `Failed to fetch keys from ${serverName}: ${response.status}`, ); return null; } const keyData = (await response.json()) as KeyData; - this.cachedKeys.set(cacheKey, keyData); + // TODO: verify the server response signature per spec and reject if invalid + this.cachedKeys.set(cacheKey, { keyData, fetchedAt: Date.now() }); return keyData; } catch (error: any) { this.logger.error( `Error fetching public key: ${error.message}`, error.stack, ); return null; } }Follow-ups:
- In
verifySignature, ifverify_keys[keyId]is missing, checkold_verify_keys[keyId]and ensureevent.origin_server_ts <= expired_ts.- Optionally honor
valid_until_tsto evict stale entries and refetch.I can wire these changes and add unit tests if helpful.
60-71: Fallback to old keys for historical events.If
verify_keys[keyId]is absent butold_verify_keys[keyId]exists and the event timestamp pre-datesexpired_ts, use the old key.-const keyData = await this.getOrFetchPublicKey(originServer, keyId); -if (!keyData || !keyData.verify_keys[keyId]) { +const keyData = await this.getOrFetchPublicKey(originServer, keyId); +const current = keyData?.verify_keys?.[keyId]?.key; +const old = keyData?.old_verify_keys?.[keyId]; +if (!current && old && typeof (event as any).origin_server_ts === 'number' && (event as any).origin_server_ts <= old.expired_ts) { + publicKey = old.key; +} else if (!current) { this.logger.warn(`Public key not found for ${originServer}:${keyId}`); return false; -} -publicKey = keyData.verify_keys[keyId].key; +} +publicKey = current ?? publicKey;
75-85: Use canonical JSON for signature verification — don’t use JSON.stringifyJSON.stringify is not canonical; Matrix/cryptographic signatures require a canonical JSON form. Replace the line
const canonicalJson = JSON.stringify(eventToVerify);
with a canonicalization call from the same canonicalizer the signer uses (e.g. canonicalize(eventToVerify) or toCanonicalJSONString(eventToVerify)). I couldn’t find an existing canonical JSON utility in the repo (only a comment in event-authorization.service.ts mentioning canonical json validation), so add/import a canonicalizer and use it in packages/federation-sdk/src/services/signature-verification.service.ts.packages/homeserver/src/homeserver.module.ts (1)
72-73: Bug:enableThumbnailsis alwaystrue.
X === 'true' || trueshort-circuits totrue. Respect the env var.-enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, +enableThumbnails: + process.env.MEDIA_ENABLE_THUMBNAILS === undefined + ? true + : process.env.MEDIA_ENABLE_THUMBNAILS === 'true',
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/state.service.ts (1)
804-807: Empty server part should be impossible; prefer enforcing or erroring.Changing
!to?? ''hides data issues and echoes a prior review concern.-return this.getMembersOfRoom(roomId).then((members) => - members.map((member) => member.split(':').pop() ?? ''), -); +return this.getMembersOfRoom(roomId).then((members) => { + const servers = members.map((m) => m.split(':').pop()); + if (servers.some((s) => !s)) { + throw new Error('Malformed Matrix IDs in room membership'); + } + return servers as string[]; +});
🧹 Nitpick comments (10)
packages/federation-sdk/src/services/staging-area.service.ts (3)
286-288: Use logger instead ofconsole.log.Keep logs consistent and structured.
-console.log('Skipping persistence stage, persisted in previous stage'); // TODO: revisit +this.logger.debug('Skipping persistence stage, persisted in previous stage'); // TODO: revisit
171-180: Backoff is linear, not exponential (comment mismatch).If exponential is desired, fix the wait calculation.
-}, 1000 * trackedEvent.retryCount); // Exponential backoff +}, 1000 * (2 ** (trackedEvent.retryCount - 1))); // Exponential backoff
81-83: Re-enable locking to prevent duplicate concurrent processing.The
@Lockis commented; concurrent queue replays can process the same room/event simultaneously.-// @Lock({ timeout: 10000, keyPath: 'event.room_id' }) +@Lock({ timeout: 10000, keyPath: 'event.room_id' })packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
1-6: Remove unused import.
Contextis not used.-import { Context, Elysia } from 'elysia'; +import { Elysia } from 'elysia';packages/federation-sdk/src/services/state.service.ts (1)
262-271: Type the return and simplify iteration.Specify the return type and leverage
state.values().-async getStateEventsByType(roomId: string, type: PduType) { - const state = await this.getFullRoomState(roomId); - const events = []; - for (const [, event] of state) { - if (event.type === type) { - events.push(event); - } - } - return events; -} +async getStateEventsByType(roomId: string, type: PduType): Promise<PersistentEventBase[]> { + const state = await this.getFullRoomState(roomId); + return Array.from(state.values()).filter((e) => e.type === type); +}packages/homeserver/src/middlewares/acl.middleware.ts (1)
32-33: Read the Authorization header case‑insensitively from Request.headers.Headers are case-insensitive; prefer
request.headers.get('authorization')over trusting a rawheaders.authorizationsnapshot.- const authorizationHeader = headers.authorization || ''; + const authorizationHeader = + request.headers.get('authorization') ?? + headers.authorization ?? + '';packages/federation-sdk/src/services/event-authorization.service.ts (4)
206-213: Avoid full room state fetch for history visibility; query the single state event.
getFullRoomState(roomId)is heavier than needed. Fetch onlym.room.history_visibility.- const roomState = await this.stateService.getFullRoomState(roomId); - const historyVisibility = this.getHistoryVisibility(roomState); + const [hv] = await this.stateService.getStateEventsByType( + roomId, + 'm.room.history_visibility', + ); + const historyVisibility = this.extractHistoryVisibility(hv);Add:
private extractHistoryVisibility( stateEvent?: { getContent?: () => any; content?: any }, ): string { const content = stateEvent?.getContent?.() ?? stateEvent?.content; return content?.history_visibility || 'shared'; }
228-238: Default should be 'shared', not 'joined'.Spec says when absent/unknown,
history_visibilitydefaults toshared. Adjust to avoid misleading behavior/comments. (spec.matrix.org)- return 'joined'; // default per Matrix spec + return 'shared'; // default per Matrix spec
351-370: Dynamic regex from user-controlled patterns: cap length to reduce DoS risk.Though you escape meta-chars, very long patterns can still be expensive. Add a small length guard.
private matchesServerPattern(serverName: string, pattern: string): boolean { + // sanity cap to reduce pathological inputs + if (pattern.length > 255) { + this.logger.warn(`ACL pattern too long; ignoring: length=${pattern.length}`); + return false; + }
240-288: Return-shape is clear and narrow. Consider unit tests for each branch.Please add tests for:
- invalid/missing signature → M_UNAUTHORIZED
- denied by ACL (deny hit, allow miss, allow list empty) → M_FORBIDDEN
- allowed when server is in room
- allowed when history is world_readable
I can scaffold these tests to recover the Codecov hit — want me to open a follow-up PR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/federation-sdk/src/container.ts(1 hunks)packages/federation-sdk/src/index.ts(3 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/media.service.ts(0 hunks)packages/federation-sdk/src/services/signature-verification.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(3 hunks)packages/homeserver/src/controllers/media.controller.ts(0 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/homeserver/src/controllers/media.controller.ts
- packages/federation-sdk/src/services/media.service.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/homeserver/src/middlewares/acl.middleware.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessEvent(179-226)packages/federation-sdk/src/index.ts (1)
EventAuthorizationService(40-40)
packages/homeserver/src/homeserver.module.ts (1)
packages/homeserver/src/controllers/federation/rooms.controller.ts (1)
roomPlugin(5-117)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/federation-sdk/src/services/signature-verification.service.ts (1)
SignatureVerificationService(20-132)packages/federation-sdk/src/services/config.service.ts (1)
serverName(92-94)
packages/federation-sdk/src/container.ts (1)
packages/federation-sdk/src/index.ts (4)
EventFetcherService(44-44)EventStateService(41-41)EventService(49-49)EventAuthorizationService(40-40)
packages/homeserver/src/controllers/federation/transactions.controller.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessEvent(179-226)packages/homeserver/src/middlewares/acl.middleware.ts (1)
canAccessEvent(28-51)
packages/federation-sdk/src/services/state.service.ts (4)
packages/federation-sdk/src/index.ts (1)
State(22-22)packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/event-wrapper.ts (3)
roomId(72-74)type(68-70)event(98-108)packages/room/src/types/v3-11.ts (1)
PduType(45-45)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 363-363: 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 (7)
packages/homeserver/src/homeserver.module.ts (1)
120-121: LGTM: plugin wiring for rooms.packages/homeserver/src/controllers/federation/transactions.controller.ts (2)
66-67: LGTM: route-level ACL viabeforeHandle.Authorization is enforced consistently ahead of handler execution.
59-63: Minor: ensureoriginon returned PDU matches spec.Current value is local server name; confirm this aligns with your spec target for GET /event.
If required by your Matrix compatibility target, we can adjust; otherwise ignore.
packages/federation-sdk/src/services/state.service.ts (1)
20-20: ExportingStatetype is fine.packages/federation-sdk/src/services/event-authorization.service.ts (1)
112-177: Minor: allow missing destination per spec; your code already does — keep.
destinationmay be omitted by older servers; you fallback to local server name. This is correct. (spec.matrix.org)packages/federation-sdk/src/index.ts (1)
95-109: Public API changed — HomeserverServices.media removed; federationAuth added. Verify downstream consumers.
- TS search found no internal
.mediausages. HomeserverServices defined at packages/federation-sdk/src/index.ts (lines ~95–109); getAllServices() exported there (line ~218). packages/federation-sdk/src/services/config.service.ts:getMediaConfig() returns this.config.media (line 105) — that’s config, not the removed service.- Action: update external consumers/docs and bump or gate the release to avoid breaking users.
packages/federation-sdk/src/container.ts (1)
93-97: Preserve string DI token 'EventFetcherService' for back-compatregisterSingleton(EventFetcherService) changes the DI token from the string "EventFetcherService" to the class — any constructor using @Inject('EventFetcherService') will stop resolving. Keep a temporary string alias while migrating injections.
File: packages/federation-sdk/src/container.ts (around lines 93–97)
container.registerSingleton(StateService); - container.registerSingleton(EventFetcherService); + // Prefer class token… + container.registerSingleton(EventFetcherService); + // …but keep string alias for back-compat while migrating injections. + container.register('EventFetcherService', { + useFactory: (c) => c.resolve(EventFetcherService), + });rg search skipped files; verify whether any code uses @Inject('EventFetcherService') or the literal 'EventFetcherService' token and add the alias above if found.
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
| const uri = new URL(request.url).pathname; | ||
|
|
There was a problem hiding this comment.
Signature verification must include the query string in the signed URI.
Per the Matrix federation auth spec, the signed uri includes ? and query parameters. Use pathname + search, not just pathname, to avoid false negatives. (spec.matrix.org)
Apply:
- const uri = new URL(request.url).pathname;
+ const url = new URL(request.url);
+ const uri = `${url.pathname}${url.search}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const uri = new URL(request.url).pathname; | |
| const url = new URL(request.url); | |
| const uri = `${url.pathname}${url.search}`; |
🤖 Prompt for AI Agents
In packages/homeserver/src/middlewares/acl.middleware.ts around lines 34-35, the
code builds the signed URI using only new URL(request.url).pathname which omits
query parameters; update it to include the query string by using the URL
object's pathname plus its search component (e.g. url.pathname + url.search or
pathname + (search || '')) so the signed URI matches the Matrix federation spec
and includes the '?' and any query parameters when verifying signatures.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
184-211: Membership/ACL checks should compare host-only (ignore port) to avoid false denies/allows.Using
serverNamewith ports will failserversInRoom.includes(...)and ACL matches; strip the port first.Apply this diff:
- private async canAccessEvent( + private async canAccessEvent( eventId: string, serverName: string, ): Promise<boolean> { try { const event = await this.eventService.getEventById(eventId); @@ - const isServerAllowed = await this.checkServerAcl(roomId, serverName); + const originHost = this.stripPort(serverName); + const isServerAllowed = await this.checkServerAcl(roomId, originHost); if (!isServerAllowed) { this.logger.warn( `Server ${serverName} is denied by room ACL for room ${roomId}`, ); return false; } const serversInRoom = await this.stateService.getServersInRoom(roomId); - if (serversInRoom.includes(serverName)) { + if (serversInRoom.includes(originHost)) { this.logger.debug(`Server ${serverName} is in room, allowing access`); return true; }
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
114-171: Normalize inputs for signature verification (method, URI path+query).To reduce signature-mismatch risk across clients, uppercase the method and ensure
uriis path+query (not a full URL).Apply this minimal diff and verify that
validateAuthorizationHeaderexpects path+query:- const isValid = await validateAuthorizationHeader( + const methodUpper = method.toUpperCase(); + let uriForSig = uri; + if (!uri.startsWith('/')) { + try { + const u = new URL(uri); + uriForSig = `${u.pathname}${u.search}`; + } catch { + // leave as-is; caller must supply path+query + } + } + const isValid = await validateAuthorizationHeader( origin, publicKey, actualDestination, - method, - uri, + methodUpper, + uriForSig, signature, body, );If
extractSignaturesFromHeadercan return multiple signatures, prefer the one whosedestinationmatchesthis.configService.serverName. Want me to adjust for multi-sig?Also applies to: 162-171
233-243: LGTM: default history visibility of 'joined' is reasonable.Optional: narrow return type to the visibility union to prevent typos in call sites.
type HistoryVisibility = 'invited' | 'joined' | 'shared' | 'world_readable'; private getHistoryVisibility(roomState: Map<any, any>): HistoryVisibility { ... }
300-307: Behavior when no ACL event exists is correct; consider adding tests.Returning
truewhen nom.room.server_aclis present aligns with current “no ACL state” behavior.Would you like test vectors for:
- allow missing vs empty,
- host vs host:port,
- IPv4/IPv6 literals with allow_ip_literals true/false,
- deny precedence over allow,
- wildcard edge cases (
*,*.example.org,example.*)? I can add a spec-driven test file to improve the patch coverage reported by Codecov.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.421Z
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.421Z
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 (1)
packages/federation-sdk/src/services/config.service.ts (1)
serverName(92-94)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 368-368: 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/federation-sdk/src/services/event-authorization.service.ts (2)
205-209: LGTM: fall back to history visibility for world-readable rooms.Access allowed for
world_readablewhen not in room is correct.
245-293: LGTM: clear auth result mapping (401 vs 403 vs unknown).Return structure and logging look good.
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
ricardogarim
force-pushed
the
feat/acl-methods
branch
from
d6a7f91 to
f8291ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/event-authorization.service.ts (4)
184-211: Use host-only for ACL and membership checks (ignore ports).Spec-wise ACL and membership comparisons operate on host. Using
serverNamewith:portcauses false denies/allows.Apply:
const roomId = event.event.room_id; + const originHost = this.stripPort(serverName); - const isServerAllowed = await this.checkServerAcl(roomId, serverName); + const isServerAllowed = await this.checkServerAcl(roomId, originHost); @@ - const serversInRoom = await this.stateService.getServersInRoom(roomId); - if (serversInRoom.includes(serverName)) { + const serversInRoom = await this.stateService.getServersInRoom(roomId); + if (serversInRoom.includes(originHost)) {
295-354: ACL semantics: treat missingallowas allow-all; operate on host-only; fix IP-literal detection.Current code denies-all when
allowis absent and matches against host:port. This diverges from spec and can over-block.Apply:
const serverAclContent = aclEvent.getContent() as { allow?: string[]; deny?: string[]; allow_ip_literals?: boolean; }; - const { - allow = [], - deny = [], - allow_ip_literals = true, - } = serverAclContent; + const allow = Array.isArray(serverAclContent.allow) + ? serverAclContent.allow + : undefined; + const deny = Array.isArray(serverAclContent.deny) ? serverAclContent.deny : []; + const allow_ip_literals = serverAclContent.allow_ip_literals ?? true; + const hostOnly = this.stripPort(serverName); - const isIpLiteral = - /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?$/.test(serverName) || - /^\[.*\](:\d+)?$/.test(serverName); // IPv6 + const isIpLiteral = + /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostOnly) || + /^\[[^\]]+\]$/.test(hostOnly); // IPv6 if (isIpLiteral && !allow_ip_literals) { this.logger.debug(`Server ${serverName} denied: IP literals not allowed`); return false; } for (const pattern of deny) { - if (this.matchesServerPattern(serverName, pattern)) { + if (this.matchesServerPattern(hostOnly, pattern)) { this.logger.debug( `Server ${serverName} matches deny pattern: ${pattern}`, ); return false; } } - // if allow list is empty, deny all servers (as per Matrix spec) - // empty allow list means no servers are allowed - if (allow.length === 0) { - this.logger.debug(`Server ${serverName} denied: allow list is empty`); - return false; - } + // If 'allow' is explicitly present and empty, deny all. If missing, allow-all (subject to 'deny'). + if (allow !== undefined && allow.length === 0) { + this.logger.debug(`Server ${serverName} denied: allow list is explicitly empty`); + return false; + } - for (const pattern of allow) { - if (this.matchesServerPattern(serverName, pattern)) { - this.logger.debug( - `Server ${serverName} matches allow pattern: ${pattern}`, - ); - return true; - } - } - - this.logger.debug(`Server ${serverName} not in allow list`); - return false; + if (allow !== undefined) { + for (const pattern of allow) { + if (this.matchesServerPattern(hostOnly, pattern)) { + this.logger.debug(`Server ${serverName} matches allow pattern: ${pattern}`); + return true; + } + } + this.logger.debug(`Server ${serverName} not in explicit allow list`); + return false; + } + // No explicit allow list -> allowed (not denied above) + return true;
356-375: ReDoS risk: avoid building RegExp from untrusted ACL patterns; implement simple '*' wildcard match.Regex-from-input enables catastrophic backtracking; spec only requires '*' wildcard.
Apply:
private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } + // Exact match or match-all + if (serverName === pattern || pattern === '*') return true; + // Support '*' only + const parts = pattern.split('*'); + let pos = 0; + if (!pattern.startsWith('*')) { + const first = parts.shift()!; + if (!serverName.startsWith(first)) return false; + pos = first.length; + } + for (let i = 0; i < parts.length - 1; i++) { + const seg = parts[i]; + if (!seg) continue; + const idx = serverName.indexOf(seg, pos); + if (idx === -1) return false; + pos = idx + seg.length; + } + const last = parts[parts.length - 1] ?? ''; + if (!pattern.endsWith('*')) { + return serverName.endsWith(last); + } + return true; }
376-376: Add host/port normalization helper.Required by fixes above.
Add inside the class:
+ private stripPort(serverName: string): string { + // [IPv6]:port -> [IPv6] + if (serverName.startsWith('[')) { + const m = serverName.match(/^\[([^\]]+)\](?::\d+)?$/); + return m ? `[${m[1]}]` : serverName; + } + // host:port -> host + return serverName.replace(/:\d+$/, ''); + }
🧹 Nitpick comments (3)
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
68-68: Signature check may use wrong URI (drop query) — include path + query in canonical request.Middleware builds
uriwithnew URL(request.url).pathname, excludingsearch. Signatures usually cover the full request-target (path + query). Pass path + query to avoid false 401 on signed requests with query params.Update in
packages/homeserver/src/middlewares/acl.middleware.ts:const u = new URL(request.url); const uri = u.pathname + u.search; // include query stringpackages/federation-sdk/src/services/event-authorization.service.ts (2)
129-137: Destination comparison should tolerate port/case differences (normalize or rely on validator).Early-rejecting when
destination !== serverNamecan spuriously fail if one has a port. Normalize host-only and case, or defer tovalidateAuthorizationHeader.Apply:
- if ( - !origin || - !key || - !signature || - (destination && destination !== this.configService.serverName) - ) { - return; - } + if (!origin || !key || !signature) { + return; + } + if ( + destination && + this.stripPort(destination.toLowerCase()) !== + this.stripPort(this.configService.serverName.toLowerCase()) + ) { + return; + }(Add
stripPorthelper below.)
245-293: Add tests for end-to-end authorization outcomes.Critical paths lack coverage (Codecov). Add unit tests for:
- valid signature + in-room
- valid signature + world_readable
- valid signature + ACL deny
- missing/invalid signature → 401
- dest mismatch → 401
- error path → 500 mapping
I can scaffold a test matrix for these cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/federation-sdk/src/container.ts(1 hunks)packages/federation-sdk/src/index.ts(3 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(3 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/homeserver/src/middlewares/acl.middleware.ts
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/index.ts
- packages/federation-sdk/src/container.ts
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.421Z
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.421Z
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/transactions.controller.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessEvent(184-231)packages/homeserver/src/middlewares/acl.middleware.ts (1)
canAccessEvent(28-51)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
packages/federation-sdk/src/services/config.service.ts (1)
serverName(94-96)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 368-368: 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/transactions.controller.ts (1)
1-5: Wiring looks good.Import + DI resolution for EventAuthorizationService and middleware integration are correct.
Also applies to: 16-16, 21-21
packages/federation-sdk/src/services/event-authorization.service.ts (1)
233-243: Default history_visibility may be ‘shared’, not ‘joined’.Double-check spec default to avoid over-restricting reads when state lacks this event.
Would you like me to open a follow-up to align default and add tests?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/homeserver/src/middlewares/acl.middleware.ts (1)
11-18: Include the query string in the signed URI (spec compliance).Use
pathname + search; current code signs only the path, causing false negatives with query params.- const method = request.method; - const uri = new URL(request.url).pathname; + const method = request.method; + const url = new URL(request.url); + const uri = `${url.pathname}${url.search}`;
🧹 Nitpick comments (4)
packages/federation-sdk/src/utils/response-codes.ts (1)
1-20: Make errCodes type-safe and immutable (derive ErrCode).Avoid
Record<string, …>; lock keys/values and export a typed union for safer indexing downstream.-export const errCodes: Record< - string, - { errcode: string; error: string; status: 401 | 403 | 500 } -> = { +export const errCodes = { M_UNAUTHORIZED: { errcode: 'M_UNAUTHORIZED', error: 'Invalid or missing signature', status: 401, }, M_FORBIDDEN: { errcode: 'M_FORBIDDEN', error: 'Access denied', status: 403, }, M_UNKNOWN: { errcode: 'M_UNKNOWN', error: 'Internal server error while processing request', status: 500, }, -}; +} as const; + +export type ErrCode = keyof typeof errCodes;packages/homeserver/src/middlewares/acl.middleware.ts (2)
26-32: Harden error mapping with a safe fallback to M_UNKNOWN.Protect against unexpected
errorCodevalues to avoid undefined access.- if (!result.authorized) { - set.status = errCodes[result.errorCode].status; - return { - errcode: errCodes[result.errorCode].errcode, - error: errCodes[result.errorCode].error, - }; - } + if (!result.authorized) { + const info = errCodes[result.errorCode as keyof typeof errCodes] ?? errCodes.M_UNKNOWN; + set.status = info.status; + return { errcode: info.errcode, error: info.error }; + }
15-16: Normalize Authorization header lookup (case-insensitive).Some frameworks may provide
Authorizationvsauthorization. Safer to check both.- const authorizationHeader = headers.authorization || ''; + const authorizationHeader = + headers.authorization ?? headers.Authorization ?? '';packages/federation-sdk/src/index.ts (1)
5-5: EventAuthorizationService is registered — remove duplicate registration.
container.registerSingleton(EventAuthorizationService) is present at packages/federation-sdk/src/container.ts:100 and packages/federation-sdk/src/container.ts:103; this prevents resolve errors but the duplicate registration should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/federation-sdk/src/index.ts(4 hunks)packages/federation-sdk/src/utils/response-codes.ts(1 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/federation-sdk/src/utils/response-codes.ts (1)
packages/federation-sdk/src/index.ts (1)
errCodes(69-69)
packages/homeserver/src/middlewares/acl.middleware.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessEvent(184-231)packages/federation-sdk/src/utils/response-codes.ts (1)
errCodes(1-20)
🔇 Additional comments (1)
packages/federation-sdk/src/index.ts (1)
69-69: LGTM: re-exporting errCodes from the SDK public API.This aligns homeserver usage with a single source of truth for status/errcode mapping.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/federation-sdk/src/services/state.service.ts (2)
289-301: Type the return based on PduType and avoid any[] inferenceReturn a typed array and leverage a generic so callers get
PersistentEventBase<..., T>for stronger correctness.Apply this diff:
-async getStateEventsByType( - roomId: string, - type: PduType, -): Promise<PersistentEventBase[]> { - const state = await this.getFullRoomState(roomId); - const events = []; - for (const [, event] of state) { - if (event.type === type) { - events.push(event); - } - } - return events; -} +async getStateEventsByType<T extends PduType>( + roomId: string, + type: T, +): Promise<Array<PersistentEventBase<RoomVersion, T>>> { + const state = await this.getFullRoomState(roomId); + const events: Array<PersistentEventBase<RoomVersion, T>> = []; + for (const [, event] of state) { + if (event.type === type) { + events.push(event as PersistentEventBase<RoomVersion, T>); + } + } + return events; +}
859-861: Good: enforce non‑empty member listThrowing when no members are found aligns with previous feedback to not “allow” empty membership sets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/state.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.ts (3)
packages/federation-sdk/src/index.ts (1)
State(23-23)packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/types/v3-11.ts (1)
PduType(45-45)
🔇 Additional comments (1)
packages/federation-sdk/src/services/state.service.ts (1)
26-26: Rename exportedStatetoResolvedStateto avoid cross-package type collisions@hs/room already exports
State = Map<StateMapKey, EventID>. Exporting anotherStatehere asMap<StateMapKey, PersistentEventBase>is a confusing, breaking public API — rename this export (e.g.,ResolvedState) and keep an internal alias to avoid churn.File: packages/federation-sdk/src/services/state.service.ts:26
Apply this diff locally and then re-export
ResolvedStatefrom the SDK index:-export type State = Map<StateMapKey, PersistentEventBase>; +// Avoid cross-package type-name collision with @hs/room's exported `State` +export type ResolvedState = Map<StateMapKey, PersistentEventBase>; +type State = ResolvedState;Run to find potential collisions and spots to update imports:
rg -nP -C2 "import\s*\{\s*State\s*\}\s*from\s*['\"]@hs/(room|federation-sdk)['\"]"
| return members.map((member) => { | ||
| const server = member.split(':').pop(); | ||
| if (!server) { | ||
| throw new Error(`Invalid member format of room ${roomId}: ${member}`); | ||
| } | ||
| return server; | ||
| }); |
There was a problem hiding this comment.
Bug: incorrect server parsing for ports/IPv6; dedupe servers
member.split(':').pop() returns only the last segment, breaking for servers with ports (example.org:8448) or IPv6 ([2001:db8::1]:8448). Parse everything after the first colon and return unique servers.
Apply this diff:
- return members.map((member) => {
- const server = member.split(':').pop();
- if (!server) {
- throw new Error(`Invalid member format of room ${roomId}: ${member}`);
- }
- return server;
- });
+ const servers = new Set<string>();
+ for (const member of members) {
+ const idx = member.indexOf(':');
+ if (idx < 0 || idx === member.length - 1) {
+ throw new Error(`Invalid member format in room ${roomId}: ${member}`);
+ }
+ // Everything after the first ":" is the server name; it may contain ":" (port/IPv6).
+ const server = member.slice(idx + 1);
+ servers.add(server);
+ }
+ return [...servers];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return members.map((member) => { | |
| const server = member.split(':').pop(); | |
| if (!server) { | |
| throw new Error(`Invalid member format of room ${roomId}: ${member}`); | |
| } | |
| return server; | |
| }); | |
| const servers = new Set<string>(); | |
| for (const member of members) { | |
| const idx = member.indexOf(':'); | |
| if (idx < 0 || idx === member.length - 1) { | |
| throw new Error(`Invalid member format in room ${roomId}: ${member}`); | |
| } | |
| // Everything after the first ":" is the server name; it may contain ":" (port/IPv6). | |
| const server = member.slice(idx + 1); | |
| servers.add(server); | |
| } | |
| return [...servers]; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/homeserver/src/middlewares/acl.middleware.ts (1)
13-14: Include query string in the signed URI (spec compliance).Matrix signs path + query; using only pathname will cause signature mismatches on requests with query params.
- const uri = new URL(request.url).pathname; + const url = new URL(request.url); + const uri = `${url.pathname}${url.search}`;
🧹 Nitpick comments (1)
packages/homeserver/src/middlewares/acl.middleware.ts (1)
9-12: Read Authorization header via Fetch API and normalize method.More robust across runtimes and avoids case/key pitfalls. Normalizing method is harmless and defensive.
- const { params, headers, request, set } = req; + const { params, request, set } = req; @@ - const authorizationHeader = headers.authorization || ''; - const method = request.method; + const authorizationHeader = request.headers.get('authorization') ?? ''; + const method = request.method.toUpperCase();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/homeserver/src/controllers/federation/transactions.controller.ts(3 hunks)packages/homeserver/src/middlewares/acl.middleware.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
🧬 Code graph analysis (1)
packages/homeserver/src/middlewares/acl.middleware.ts (3)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessEvent(184-231)packages/federation-sdk/src/index.ts (2)
EventAuthorizationService(40-40)errCodes(69-69)packages/federation-sdk/src/utils/response-codes.ts (1)
errCodes(1-20)
| const result = await federationAuth.canAccessEventFromAuthorizationHeader( | ||
| eventId, | ||
| authorizationHeader, | ||
| method, | ||
| uri, | ||
| ); | ||
|
|
||
| if (!result.authorized) { | ||
| set.status = errCodes[result.errorCode].status; | ||
| return { | ||
| errcode: errCodes[result.errorCode].errcode, | ||
| error: errCodes[result.errorCode].error, | ||
| }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Harden error handling and unknown error codes.
Guard against service exceptions and unknown codes to always return a spec’d body.
- const result = await federationAuth.canAccessEventFromAuthorizationHeader(
- eventId,
- authorizationHeader,
- method,
- uri,
- );
+ let result;
+ try {
+ result = await federationAuth.canAccessEventFromAuthorizationHeader(
+ eventId,
+ authorizationHeader,
+ method,
+ uri,
+ );
+ } catch {
+ set.status = errCodes.M_UNKNOWN.status;
+ return {
+ errcode: errCodes.M_UNKNOWN.errcode,
+ error: errCodes.M_UNKNOWN.error,
+ };
+ }
@@
- if (!result.authorized) {
- set.status = errCodes[result.errorCode].status;
- return {
- errcode: errCodes[result.errorCode].errcode,
- error: errCodes[result.errorCode].error,
- };
- }
+ if (!result.authorized) {
+ const code = errCodes[result.errorCode] ?? errCodes.M_UNKNOWN;
+ set.status = code.status;
+ return { errcode: code.errcode, error: code.error };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await federationAuth.canAccessEventFromAuthorizationHeader( | |
| eventId, | |
| authorizationHeader, | |
| method, | |
| uri, | |
| ); | |
| if (!result.authorized) { | |
| set.status = errCodes[result.errorCode].status; | |
| return { | |
| errcode: errCodes[result.errorCode].errcode, | |
| error: errCodes[result.errorCode].error, | |
| }; | |
| } | |
| }); | |
| let result; | |
| try { | |
| result = await federationAuth.canAccessEventFromAuthorizationHeader( | |
| eventId, | |
| authorizationHeader, | |
| method, | |
| uri, | |
| ); | |
| } catch { | |
| set.status = errCodes.M_UNKNOWN.status; | |
| return { | |
| errcode: errCodes.M_UNKNOWN.errcode, | |
| error: errCodes.M_UNKNOWN.error, | |
| }; | |
| } | |
| if (!result.authorized) { | |
| const code = errCodes[result.errorCode] ?? errCodes.M_UNKNOWN; | |
| set.status = code.status; | |
| return { errcode: code.errcode, error: code.error }; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/homeserver/src/middlewares/acl.middleware.ts around lines 15 to 29,
the call to federationAuth.canAccessEventFromAuthorizationHeader is not guarded
and the handler assumes result.errorCode always maps to errCodes; wrap the await
call in a try/catch to convert any thrown exceptions into a known error
response, and when result.authorized is false, defensively resolve
result.errorCode against errCodes with a safe fallback (e.g., a default INTERNAL
or FORBIDDEN mapping) so set.status is always assigned and the returned object
always contains errcode and error fields from the fallback when the code is
unknown; ensure no exceptions escape and the response body conforms to the spec
in all failure cases.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/homeserver/src/controllers/internal/room.controller.ts (1)
319-324: Restore DTO validation and structured error handling in ban routeThis handler no longer validates inputs or returns typed errors (unlike other endpoints here). Add back params/body validation and 400/500 handling to avoid 500s on bad input.
Apply this diff at the start of the handler:
- async ({ - params, - body, - }): Promise<InternalBanUserResponse | ErrorResponse> => { + async ({ + params, + body, + set, + }): Promise<InternalBanUserResponse | ErrorResponse> => { + const roomIdParse = RoomIdDto.safeParse(params.roomId); + const userIdParse = UsernameDto.safeParse(params.userIdToBan); + const bodyParse = InternalBanUserBodyDto.safeParse(body); + if (!roomIdParse.success || !userIdParse.success || !bodyParse.success) { + set.status = 400; + return { + error: 'Invalid request', + details: { + roomId: roomIdParse.error?.flatten(), + userId: userIdParse.error?.flatten(), + body: bodyParse.error?.flatten(), + }, + }; + } + const { senderUserId /*, reason, targetServers */ } = bodyParse.data;packages/room/src/authorizartion-rules/rules.ts (1)
60-76: Centralize recognized room versions; include '12' (spec updated Aug 11, 2025)Replace the inline ['1'…'11'] with a single exported Set/constant (or import) and add '12' — as of 2025-09-15 Matrix recognizes room versions 1–12. Location: packages/room/src/authorizartion-rules/rules.ts lines 60–76.
♻️ Duplicate comments (5)
packages/federation-sdk/src/services/event-authorization.service.ts (4)
184-211: Use host-only for ACL and membership checks (strip :port)
Using serverName with port causes false decisions for ACL/membership. Compare on host-only.- private async canAccessEvent( + private async canAccessEvent( eventId: string, serverName: string, ): Promise<boolean> { @@ - const roomId = event.event.room_id; + const roomId = event.event.room_id; + const originHost = this.stripPort(serverName); @@ - const isServerAllowed = await this.checkServerAcl(roomId, serverName); + const isServerAllowed = await this.checkServerAcl(roomId, originHost); @@ - const serversInRoom = await this.stateService.getServersInRoom(roomId); - if (serversInRoom.includes(serverName)) { + const serversInRoom = await this.stateService.getServersInRoom(roomId); + if (serversInRoom.includes(originHost)) { this.logger.debug(`Server ${serverName} is in room, allowing access`); return true; }Add helper inside the class:
private stripPort(serverName: string): string { if (serverName.startsWith('[')) { const m = serverName.match(/^\[([^\]]+)\](?::\d+)?$/); return m ? `[${m[1]}]` : serverName; } return serverName.replace(/:\d+$/, ''); }
284-287: Do not log full Authorization headerRedact the header to avoid credential leakage.
- this.logger.error( - { error, eventId, authorizationHeader, method, uri, body }, - 'Error checking event access', - ); + this.logger.error( + { error, eventId, method, uri, body, authorizationHeader: '<redacted>' }, + 'Error checking event access', + );
295-350: Fix ACL semantics and host handling per spec
- Missing allow should mean allow-all (subject to deny and allow_ip_literals).
- Evaluate patterns and IP-literal checks on host-only (no port).
- const serverAclContent = aclEvent.getContent(); - const { - allow = [], - deny = [], - allow_ip_literals = true, - } = serverAclContent; + const serverAclContent = aclEvent.getContent() as { + allow?: string[]; + deny?: string[]; + allow_ip_literals?: boolean; + }; + const allow = Array.isArray(serverAclContent.allow) + ? serverAclContent.allow + : undefined; + const deny = Array.isArray(serverAclContent.deny) ? serverAclContent.deny : []; + const allow_ip_literals = serverAclContent.allow_ip_literals ?? true; + + const hostOnly = this.stripPort(serverName); - const isIpLiteral = - /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?$/.test(serverName) || - /^\[.*\](:\d+)?$/.test(serverName); // IPv6 + const isIpLiteral = + /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostOnly) || + /^\[[^\]]+\]$/.test(hostOnly); // IPv6 literal in brackets if (isIpLiteral && !allow_ip_literals) { this.logger.debug(`Server ${serverName} denied: IP literals not allowed`); return false; } for (const pattern of deny) { - if (this.matchesServerPattern(serverName, pattern)) { + if (this.matchesServerPattern(hostOnly, pattern)) { this.logger.debug( `Server ${serverName} matches deny pattern: ${pattern}`, ); return false; } } - // if allow list is empty, deny all servers (as per Matrix spec) - // empty allow list means no servers are allowed - if (allow.length === 0) { - this.logger.debug(`Server ${serverName} denied: allow list is empty`); - return false; - } + // Explicit empty allow => deny all; missing allow => allow-all (subject to deny) + if (allow !== undefined) { + if (allow.length === 0) { + this.logger.debug(`Server ${serverName} denied: allow list is explicitly empty`); + return false; + } + for (const pattern of allow) { + if (this.matchesServerPattern(hostOnly, pattern)) { + this.logger.debug( + `Server ${serverName} matches allow pattern: ${pattern}`, + ); + return true; + } + } + this.logger.debug(`Server ${serverName} not in explicit allow list`); + return false; + } - for (const pattern of allow) { - if (this.matchesServerPattern(serverName, pattern)) { - this.logger.debug( - `Server ${serverName} matches allow pattern: ${pattern}`, - ); - return true; - } - } - - this.logger.debug(`Server ${serverName} not in allow list`); - return false; + return true;
352-371: Avoid regex-from-input; implement simple '*' wildcard matcherBuilding regex from untrusted ACL patterns risks ReDoS and goes beyond spec needs.
- private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } - } + private matchesServerPattern(serverName: string, pattern: string): boolean { + if (pattern === '*' || serverName === pattern) return true; + // Support '*' only; treat all other chars literally + const parts = pattern.split('*'); + let pos = 0; + if (!pattern.startsWith('*')) { + if (!serverName.startsWith(parts[0])) return false; + pos = parts[0].length; + parts.shift(); + } + for (let i = 0; i < parts.length - 1; i++) { + const seg = parts[i]; + if (!seg) continue; + const idx = serverName.indexOf(seg, pos); + if (idx === -1) return false; + pos = idx + seg.length; + } + const last = parts[parts.length - 1] ?? ''; + return pattern.endsWith('*') ? true : serverName.endsWith(last); + }packages/federation-sdk/src/services/state.service.ts (1)
858-869: Server parsing breaks for ports/IPv6; also dedupe resultsSplit on the first colon after localpart and keep everything after it; return unique servers.
- const members = await this.getMembersOfRoom(roomId); - if (!members.length) { - throw new Error(`No members found in room ${roomId}`); - } - return members.map((member) => { - const server = member.split(':').pop(); - if (!server) { - throw new Error(`Invalid member format of room ${roomId}: ${member}`); - } - return server; - }); + const members = await this.getMembersOfRoom(roomId); + if (members.length === 0) { + throw new Error(`No members found in room ${roomId}`); + } + const servers = new Set<string>(); + for (const member of members) { + const idx = member.indexOf(':'); // '@localpart:server' + if (idx < 0 || idx === member.length - 1) { + throw new Error(`Invalid member format in room ${roomId}: ${member}`); + } + const server = member.slice(idx + 1); // may include brackets for IPv6; no port in user_id + servers.add(server); + } + return [...servers];
🧹 Nitpick comments (10)
packages/federation-sdk/src/services/send-join.service.ts (1)
73-83: Guard membership when switching to untyped getContent()Avoid emitting undefined membership; validate once and reuse content locals.
this.emitterService.emit('homeserver.matrix.accept-invite', { event_id: eventId, room_id: roomId, sender: signedJoinEvent.sender, origin_server_ts: signedJoinEvent.originServerTs, - content: { - avatar_url: signedJoinEvent.getContent().avatar_url || null, - displayname: signedJoinEvent.getContent().displayname || '', - membership: signedJoinEvent.getContent().membership, - }, + content: { + // validate content once + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + ...(( + content => { + if (typeof content?.membership !== 'string') { + throw new Error('Invalid m.room.member content: missing membership'); + } + return { + avatar_url: content.avatar_url ?? null, + displayname: content.displayname ?? '', + membership: content.membership, + }; + } + )(signedJoinEvent.getContent())), + }, });packages/federation-sdk/src/services/state.service.ts (2)
26-26: Exported type name collides with @hs/room’s StateBoth packages export State with different shapes (EventID vs PersistentEventBase). This will confuse consumers.
Rename this to ResolvedState or StateMapResolved and re-export accordingly to avoid ambiguous imports.
289-301: Type the accumulator to avoid any[] leakageGive events a concrete type for safer inference.
- const events = []; + const events: PersistentEventBase<RoomVersion, P>[] = [];packages/room/src/state_resolution/definitions/definitions.ts (1)
26-27: Avoid double getMembership() callCache the value once; tiny win and clearer intent.
- (event.isMembershipEvent() && - (event.getMembership() === 'leave' || event.getMembership() === 'ban') && + (event.isMembershipEvent() && + ((() => { const m = event.getMembership(); return m === 'leave' || m === 'ban'; })()) && event.sender !== event.stateKey)packages/homeserver/src/controllers/internal/room.controller.ts (1)
397-401: Handle rejected auth before returningMirror invite flow: if the event was rejected by auth rules, surface that instead of returning 200.
Apply this diff:
- await stateService.persistStateEvent(membershipEvent); - - return { - eventId: membershipEvent.eventId, - }; + await stateService.persistStateEvent(membershipEvent); + if (membershipEvent.rejected) { + set.status = 403; + return { + error: membershipEvent.rejectedReason ?? 'Ban rejected by auth rules', + details: {}, + }; + } + return { eventId: membershipEvent.eventId };packages/room/src/manager/power-level-event-wrapper.ts (1)
24-29:toEventBasecan beundefined; fix the return type and narrow before castingCurrent cast can return
undefinedat runtime while the signature claims non-null. Return| undefinedand gate on type to avoid unsoundness.Apply this diff:
- toEventBase(): PersistentEventBase<RoomVersion, 'm.room.power_levels'> { - return this.event as PersistentEventBase< - RoomVersion, - 'm.room.power_levels' - >; - } + toEventBase(): + | PersistentEventBase<RoomVersion, 'm.room.power_levels'> + | undefined { + return this.event?.isPowerLevelEvent() + ? (this.event as PersistentEventBase<RoomVersion, 'm.room.power_levels'>) + : undefined; + }packages/room/src/authorizartion-rules/rules.ts (1)
172-223: PreferStateResolverAuthorizationErrorover plainErrorfor consistencySeveral branches throw
Error(...)instead ofStateResolverAuthorizationError, making error handling uneven.Example:
- if (sender !== invitee) { - throw new Error('state_key does not match the sender'); - } + if (sender !== invitee) { + throw new StateResolverAuthorizationError( + 'state_key does not match the sender', + { eventFailed: membershipEventToCheck } + ); + }Apply similarly in other branches that perform auth decisions.
packages/room/src/manager/event-wrapper.ts (3)
154-157: Nice: explicit, typed event-kind guardsThese predicates improve flow typing across the module. Consider adding guards for ACL and history visibility to align with this PR’s objectives.
Apply this diff to add two more guards:
isAliasEvent(): this is PersistentEventBase<T, 'm.room.aliases'> { return this.isState() && this.type === 'm.room.aliases'; } + + isServerAclEvent(): this is PersistentEventBase<T, 'm.room.server_acl'> { + return this.isState() && this.type === 'm.room.server_acl'; + } + + isHistoryVisibilityEvent(): this is PersistentEventBase< + T, + 'm.room.history_visibility' + > { + return this.isState() && this.type === 'm.room.history_visibility'; + }Also applies to: 158-160, 162-164, 166-168, 170-172, 174-176, 178-183, 185-187
189-194: Improve error context and return typing for getMembershipInclude the actual event type in the error to ease debugging, and surface the precise return type.
Apply this diff:
- getMembership() { - if (!this.isMembershipEvent()) - throw new Error('Event is not a membership event'); - - return (this.getContent() as PduMembershipEventContent).membership; - } + getMembership(): PduMembershipEventContent['membership'] { + if (!this.isMembershipEvent()) { + throw new Error(`Event is not a membership event; got type: ${this.type}`); + } + return (this.getContent() as PduMembershipEventContent).membership; + }
196-202: Mirror the membership improvements for getJoinRuleAdd precise return type and richer error message.
Apply this diff:
- getJoinRule() { - if (!this.isJoinRuleEvent()) { - throw new Error('Event is not a join rule event'); - } - - return (this.getContent() as PduJoinRuleEventContent).join_rule; - } + getJoinRule(): PduJoinRuleEventContent['join_rule'] { + if (!this.isJoinRuleEvent()) { + throw new Error(`Event is not a join rule event; got type: ${this.type}`); + } + return (this.getContent() as PduJoinRuleEventContent).join_rule; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(6 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(2 hunks)packages/homeserver/src/controllers/internal/room.controller.ts(2 hunks)packages/room/src/authorizartion-rules/rules.ts(7 hunks)packages/room/src/manager/event-wrapper.ts(2 hunks)packages/room/src/manager/power-level-event-wrapper.ts(2 hunks)packages/room/src/manager/room-state.ts(5 hunks)packages/room/src/state_resolution/definitions/definitions.ts(1 hunks)packages/room/src/types/v3-11.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.421Z
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.421Z
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/room/src/types/v3-11.tspackages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (7)
packages/room/src/manager/power-level-event-wrapper.ts (1)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/room/src/authorizartion-rules/rules.ts (3)
packages/room/src/authorizartion-rules/errors.ts (1)
StateResolverAuthorizationError(24-24)packages/room/src/manager/event-wrapper.ts (1)
event(102-112)packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-43)
packages/room/src/manager/room-state.ts (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
getStateMapKey(10-15)
packages/federation-sdk/src/services/state.service.ts (2)
packages/federation-sdk/src/index.ts (1)
State(23-23)packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (1)
PduForType(11-11)
packages/room/src/manager/event-wrapper.ts (1)
packages/room/src/types/v3-11.ts (3)
PduContent(731-731)PduMembershipEventContent(114-116)PduJoinRuleEventContent(186-188)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
packages/federation-sdk/src/services/config.service.ts (2)
ConfigService(76-153)serverName(94-96)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 364-364: 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 (25)
packages/federation-sdk/src/services/state.service.ts (3)
402-422: LGTM: generic signEvent improves type inferenceGood change; preserves E and returns same instance type.
429-431: LGTM: room_version via untyped content is consistent with the repo-wide shiftNo issues.
545-547: LGTM: untyped getContent for create eventsConsistent with newer content access.
packages/room/src/types/v3-11.ts (4)
229-247: ACL content schema looks correct (allow/deny/allow_ip_literals with default=true)Good addition; aligns with spec expectations.
655-659: Wiring m.room.server_acl into PDU union is correctEmpty state_key variant matches spec.
718-719: PDU union includes server ACLAll good.
731-731: PduContent becomes generic — confirm downstream typingThis should be backward-compatible via the default parameter, but please ensure external imports don’t rely on the old non-generic form in ways that now narrow unexpectedly.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
114-182: Signature verification flow: LGTM (happy-path and key lookup/cache)Clear separation of concerns; good early returns for invalid cases.
packages/homeserver/src/controllers/internal/room.controller.ts (2)
368-371: Good: stronger create-event guardValidating both presence and type with
isCreateEvent()prevents mis-typed state reads.
385-386: OK to use untypedgetContent()hereGiven the prior type guard, accessing
.room_versionvia untyped content is safe.packages/homeserver/src/controllers/internal/invite.controller.ts (2)
35-37: Good: runtime type guard for create eventPrevents invalid content access when state is corrupt or mismatched.
51-52: OK to use untypedgetContent()Safe given the guard. Keeps the code consistent with other modules.
packages/room/src/manager/room-state.ts (6)
25-30: Good: validate create event shapeEnsures we only read creator from a true m.room.create.
38-43: Good: membership accessor guardsReturning
undefinedwhen no valid membership event exists is appropriate.
83-88: Good default for privacyFalling back to 'public' if join rules missing or wrong type matches expected defaults.
107-112: Topic: safe fallbackReturning empty string for missing/invalid topic is consistent and non-throwing.
137-142: Power levels: tolerant accessReturning
undefinedwhen the event is missing or mistyped avoids crashes downstream.
148-153: Good: validate create event for version extractionPrevents invalid
.room_versionreads.packages/room/src/authorizartion-rules/rules.ts (7)
35-39: Good: requireisCreateEvent()in create checksAvoids accepting malformed “create” events.
121-122: Membership checks: correct to require a real membership eventThis avoids treating other state as membership.
168-169: Content access via untypedgetContent()is fineYou guarded type above; content read is safe.
188-191: Join path: correct creator short‑circuitAccepting creator’s initial join when the only prev is create matches the spec intent.
711-715: Good: assert the room create event’s type, not only presencePrevents reading create content from a wrong event.
719-721: OK:m.federatecheck via untyped contentGuarded by the type check above; this is correct and simple.
522-524: Optional chaining here pairs well with the proposedtoEventBase()fixOnce
toEventBase()returns| undefined, this access remains safe.
ggazzo
force-pushed
the
feat/acl-methods
branch
from
6a97e1d to
0479240
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/event-authorization.service.ts (4)
184-211: ACL/membership must ignore port; operate on host‑only to avoid false auth decisions.
UsingserverNamewith:portcauses mismatches vs room membership and ACL patterns.Apply this diff:
@@ - private async canAccessEvent( + private async canAccessEvent( eventId: string, serverName: string, ): Promise<boolean> { @@ - const roomId = event.event.room_id; + const roomId = event.event.room_id; + const originHost = this.stripPort(serverName); @@ - const isServerAllowed = await this.checkServerAcl(roomId, serverName); + const isServerAllowed = await this.checkServerAcl(roomId, originHost); @@ - const serversInRoom = await this.stateService.getServersInRoom(roomId); - if (serversInRoom.includes(serverName)) { + const serversInRoom = await this.stateService.getServersInRoom(roomId); + if (serversInRoom.includes(originHost)) { this.logger.debug(`Server ${serverName} is in room, allowing access`); return true; }Add the helper inside this class:
private stripPort(serverName: string): string { // [IPv6]:port -> [IPv6] if (serverName.startsWith('[')) { const m = serverName.match(/^\[([^\]]+)\](?::\d+)?$/); return m ? `[${m[1]}]` : serverName; } // host:port -> host return serverName.replace(/:\d+$/, ''); }
295-350: Fix ACL semantics per spec; also normalize to host‑only and avoid IP‑literal false negatives.
- Missing
allowmust mean “allow all” (subject todeny/allow_ip_literals), not deny‑all.- Evaluate ACL against host‑only (no port).
- Check IP‑literals on host‑only.
Apply:
@@ - const serverAclContent = aclEvent.getContent(); - const { - allow = [], - deny = [], - allow_ip_literals = true, - } = serverAclContent; + const serverAclContent = aclEvent.getContent() as { + allow?: string[]; + deny?: string[]; + allow_ip_literals?: boolean; + }; + // Preserve presence/absence to distinguish "missing" vs "present but empty" + const allow = Array.isArray(serverAclContent.allow) + ? serverAclContent.allow + : undefined; + const deny = Array.isArray(serverAclContent.deny) + ? serverAclContent.deny + : []; + const allow_ip_literals = serverAclContent.allow_ip_literals ?? true; + const hostOnly = this.stripPort(serverName); @@ - const isIpLiteral = - /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?$/.test(serverName) || - /^\[.*\](:\d+)?$/.test(serverName); // IPv6 + const isIpLiteral = + /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostOnly) || + /^\[[^\]]+\]$/.test(hostOnly); // IPv6 @@ - for (const pattern of deny) { - if (this.matchesServerPattern(serverName, pattern)) { + for (const pattern of deny) { + if (this.matchesServerPattern(hostOnly, pattern)) { this.logger.debug( `Server ${serverName} matches deny pattern: ${pattern}`, ); return false; } } @@ - // if allow list is empty, deny all servers (as per Matrix spec) - // empty allow list means no servers are allowed - if (allow.length === 0) { - this.logger.debug(`Server ${serverName} denied: allow list is empty`); - return false; - } + // If 'allow' is present and empty, deny all. If 'allow' is missing, allow-all (subject to deny rules). + if (allow !== undefined && allow.length === 0) { + this.logger.debug( + `Server ${serverName} denied: allow list is explicitly empty`, + ); + return false; + } @@ - for (const pattern of allow) { - if (this.matchesServerPattern(serverName, pattern)) { - this.logger.debug( - `Server ${serverName} matches allow pattern: ${pattern}`, - ); - return true; - } - } - - this.logger.debug(`Server ${serverName} not in allow list`); - return false; + if (allow !== undefined) { + for (const pattern of allow) { + if (this.matchesServerPattern(hostOnly, pattern)) { + this.logger.debug( + `Server ${serverName} matches allow pattern: ${pattern}`, + ); + return true; + } + } + this.logger.debug(`Server ${serverName} not in explicit allow list`); + return false; + } + // No explicit allow list -> allowed (not denied above) + return true;
352-371: Do not build RegExp from untrusted ACL patterns; use simple '*' wildcard matching.
Removes ReDoS risk and matches spec (only '*' supported).Apply:
private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } + // Fast paths + if (pattern === '*' || serverName === pattern) return true; + // Greedy linear '*' matcher + let i = 0; // serverName index + let j = 0; // pattern index + let star = -1; + let match = 0; + while (i < serverName.length) { + if (j < pattern.length && (pattern[j] === serverName[i])) { + i++; j++; + } else if (j < pattern.length && pattern[j] === '*') { + star = j++; + match = i; + } else if (star !== -1) { + j = star + 1; + i = ++match; + } else { + return false; + } + } + while (j < pattern.length && pattern[j] === '*') j++; + return j === pattern.length; }
284-287: Redact Authorization header in error logs.
Avoid leaking credentials/tokens.Apply:
- this.logger.error( - { error, eventId, authorizationHeader, method, uri, body }, - 'Error checking event access', - ); + this.logger.error( + { error, eventId, method, uri, body, authorizationHeader: '<redacted>' }, + 'Error checking event access', + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/federation-sdk/src/container.ts(1 hunks)packages/federation-sdk/src/index.ts(4 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/federation-sdk/src/utils/response-codes.ts(1 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(3 hunks)packages/homeserver/src/middlewares/acl.middleware.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/federation-sdk/src/utils/response-codes.ts
- packages/federation-sdk/src/container.ts
- packages/homeserver/src/middlewares/acl.middleware.ts
- packages/federation-sdk/src/index.ts
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.421Z
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.421Z
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/transactions.controller.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
canAccessEvent(184-231)packages/homeserver/src/middlewares/acl.middleware.ts (1)
canAccessEvent(5-30)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
packages/federation-sdk/src/services/state.service.ts (1)
singleton(35-931)packages/federation-sdk/src/services/event.service.ts (1)
singleton(44-808)packages/federation-sdk/src/services/config.service.ts (2)
ConfigService(76-153)serverName(94-96)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 364-364: 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 (3)
packages/homeserver/src/controllers/federation/transactions.controller.ts (3)
1-5: DI import for EventAuthorizationService looks correct.
Wires the SDK service into the controller cleanly.
16-16: Middleware import OK.
Interface matches the usage below.
21-21: Resolve once at plugin init (good).
Singleton resolve at load-time avoids per‑request overhead.
| }; | ||
| }, | ||
| { | ||
| use: canAccessEvent(eventAuthService), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Elysia supports per‑route use: in this project’s version.
If not supported, the middleware won’t run. Confirm the Elysia version or switch to a route‑scoped beforeHandle or a .group(..., { use: plugin }) wrapper for this GET only.
🌐 Web query:
Does Elysia support a `use` property in a single route’s options (to attach a plugin only to that route)? Cite docs and the minimum version that supports it.
💡 Result:
Short answer: No — Elysia does not provide a per-route "use" option to attach a plugin to a single route. The official route documentation describes route options for things like local schema/hooks but does not list a "use" property, and the plugin docs describe attaching plugins via app.use (instance‑level) or using guards/plugins as instances, not a per‑route options "use". [1][2]
Sources:
- Route docs (shows route signature and local schema/hooks in route options; no "use" per-route option). [1]
- Plugin docs (describes .use to register plugins on an instance and plugin patterns). [2]
Remove per‑route use — Elysia does not support use in route options.
Confirmed: route options expose a route-level beforeHandle for auth/validation; plugins are registered via .use on an instance and scoping is done with group/guard. (elysiajs.com)
File: packages/homeserver/src/controllers/federation/transactions.controller.ts (line ~68) — replace
- use: canAccessEvent(eventAuthService),
with either: - beforeHandle: canAccessEvent(eventAuthService) (preferred for per-route auth), or
- wrap the route in a group/guard and call app.use(...) inside that scope to apply the plugin only to that group.
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/transactions.controller.ts
around line 68, the route option incorrectly uses "use:
canAccessEvent(eventAuthService)" which Elysia does not support; replace that
option with "beforeHandle: canAccessEvent(eventAuthService)" to apply per-route
auth, or alternatively remove the per-route plugin and scope the plugin via
app.group/guard and app.use(...) to apply canAccessEvent to the route group.
0479240 to
7209530
Compare
| roomId: string, | ||
| serverName: string, | ||
| ): Promise<boolean> { | ||
| const [aclEvent] = await this.stateService.getStateEventsByType( |
There was a problem hiding this comment.
is it possible to have more than one state of type m.room.server_acl? if so this code is just considering the first one, is that ok?
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/room/src/manager/event-wrapper.ts (2)
121-123: Remove generic from getContent to prevent unsafe downcasts.Returning
PduContent<Type>avoids caller-supplied generics that bypass narrowing. See prior review; repeating for visibility.Apply:
- getContent<T extends PduContent<Type>>(): T { - return this.rawEvent.content as T; - } + getContent(): PduContent<Type> { + return this.rawEvent.content as PduContent<Type>; + }
371-383: Update membership-content call sites after getContent change.Stop using
getContent<...>(); narrow locally. Reiterating past comment so the new guards can be leveraged here.Apply:
- this.getContent<PduMembershipEventContent>().third_party_invite + (this.getContent() as PduMembershipEventContent).third_party_invite- this.getContent<PduMembershipEventContent>() - .join_authorised_via_users_server + (this.getContent() as PduMembershipEventContent) + .join_authorised_via_users_serverpackages/federation-sdk/src/services/event-authorization.service.ts (4)
195-211: Normalize to host-only (no :port) for membership/ACL checks.Using
serverNamewith:portcauses false negatives when comparing against room servers and ACL patterns.Apply:
const roomId = event.event.room_id; - const state = await this.stateService.getFullRoomState(roomId); + const state = await this.stateService.getFullRoomState(roomId); + const originHost = this.stripPort(serverName); @@ - const isServerAllowed = await this.checkServerAcl(aclEvent, serverName); + const isServerAllowed = await this.checkServerAcl(aclEvent, originHost); @@ - if (serversInRoom.includes(serverName)) { + if (serversInRoom.includes(originHost)) { this.logger.debug(`Server ${serverName} is in room, allowing access`); return true; }Add inside the class:
private stripPort(serverName: string): string { // [IPv6]:port -> [IPv6] if (serverName.startsWith('[')) { const m = serverName.match(/^\[([^\]]+)\](?::\d+)?$/); return m ? `[${m[1]}]` : serverName; } // host:port -> host return serverName.replace(/:\d+$/, ''); }
277-280: Do not log raw Authorization header. Redact it.Avoid credential leakage in logs.
Apply:
- this.logger.error( - { error, eventId, authorizationHeader, method, uri, body }, - 'Error checking event access', - ); + this.logger.error( + { error, eventId, method, uri, body, authorizationHeader: '<redacted>' }, + 'Error checking event access', + );
288-339: Fix ACL semantics and IP/host handling to match Matrix spec.
- Missing
allowmust mean “allow all” (subject todenyandallow_ip_literals), not deny-all.- Operate on host-only for pattern and IP-literal checks.
Apply:
- const serverAclContent = aclEvent.getContent(); - const { - allow = [], - deny = [], - allow_ip_literals = true, - } = serverAclContent; + const serverAclContent = aclEvent.getContent() as { + allow?: string[]; + deny?: string[]; + allow_ip_literals?: boolean; + }; + const allow = Array.isArray(serverAclContent.allow) + ? serverAclContent.allow + : undefined; + const deny = Array.isArray(serverAclContent.deny) + ? serverAclContent.deny + : []; + const allow_ip_literals = serverAclContent.allow_ip_literals ?? true; + + const hostOnly = this.stripPort(serverName); - const isIpLiteral = - /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?$/.test(serverName) || - /^\[.*\](:\d+)?$/.test(serverName); // IPv6 + const isIpLiteral = + /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostOnly) || + /^\[[^\]]+\]$/.test(hostOnly); // IPv6 if (isIpLiteral && !allow_ip_literals) { this.logger.debug(`Server ${serverName} denied: IP literals not allowed`); return false; } for (const pattern of deny) { - if (this.matchesServerPattern(serverName, pattern)) { + if (this.matchesServerPattern(hostOnly, pattern)) { this.logger.debug( `Server ${serverName} matches deny pattern: ${pattern}`, ); return false; } } - // if allow list is empty, deny all servers (as per Matrix spec) - // empty allow list means no servers are allowed - if (allow.length === 0) { - this.logger.debug(`Server ${serverName} denied: allow list is empty`); - return false; - } + // If 'allow' is present and empty -> deny all. If missing -> allow all (subject to deny). + if (allow !== undefined && allow.length === 0) { + this.logger.debug(`Server ${serverName} denied: allow list is explicitly empty`); + return false; + } - for (const pattern of allow) { - if (this.matchesServerPattern(serverName, pattern)) { - this.logger.debug( - `Server ${serverName} matches allow pattern: ${pattern}`, - ); - return true; - } - } + if (allow !== undefined) { + for (const pattern of allow) { + if (this.matchesServerPattern(hostOnly, pattern)) { + this.logger.debug( + `Server ${serverName} matches allow pattern: ${pattern}`, + ); + return true; + } + } + this.logger.debug(`Server ${serverName} not in explicit allow list`); + return false; + } - this.logger.debug(`Server ${serverName} not in allow list`); - return false; + // No explicit allow list -> allowed (not denied above) + return true;
341-360: Avoid regex-from-input; implement simple '*' wildcard matcher.Regex construction from ACL patterns risks ReDoS and over-implements the spec.
Apply:
- private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } - } + private matchesServerPattern(serverName: string, pattern: string): boolean { + // Exact or match-all + if (pattern === '*' || serverName === pattern) return true; + // Greedy '*' matcher; other chars are literal + let s = 0, p = 0, star = -1, match = 0; + while (s < serverName.length) { + if (p < pattern.length && pattern[p] === serverName[s]) { + s++; p++; + } else if (p < pattern.length && pattern[p] === '*') { + star = p++; + match = s; + } else if (star !== -1) { + p = star + 1; + s = ++match; + } else { + return false; + } + } + while (p < pattern.length && pattern[p] === '*') p++; + return p === pattern.length; + }
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
195-201: Avoid full-room state fetch on hot path.Fetching the entire state for ACL and history visibility is heavy. Prefer targeted reads (e.g., repository call by
StateMapKeyform.room.server_aclandm.room.history_visibility) or a newgetRoomStateByKeys(roomId, keys[]).Would you like me to draft a minimal
StateService.getRoomStateByKeysplus repository query?
📜 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(2 hunks)packages/federation-sdk/src/services/state.service.ts(1 hunks)packages/room/src/manager/event-wrapper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.421Z
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.421Z
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 (1)
packages/federation-sdk/src/services/config.service.ts (2)
ConfigService(76-153)serverName(94-96)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 353-353: 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/room/src/manager/event-wrapper.ts (1)
178-187: Good addition: precise type guards for ACL and history visibility events.These integrate cleanly with existing guards and enable safe narrowing downstream. No issues found.
ricardogarim
force-pushed
the
feat/acl-methods
branch
from
d14acf0 to
84d007d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/federation-sdk/src/services/state.service.ts (1)
912-922: Fix server parsing for ports/IPv6 and dedupe resultsmember.split(':').pop() breaks for IPv6 ([...]:port) and host:port; it also returns duplicates. Parse after the first ":" and return unique servers.
Apply:
- return members.map((member) => { - const server = member.split(':').pop(); - if (!server) { - throw new Error(`Invalid member format of room ${roomId}: ${member}`); - } - return server; - }); + const servers = new Set<string>(); + for (const member of members) { + const idx = member.indexOf(':'); + if (idx < 0 || idx === member.length - 1) { + throw new Error(`Invalid member format in room ${roomId}: ${member}`); + } + // Everything after the first ":" is the server name; it may include ":" (port/IPv6). + const server = member.slice(idx + 1); + servers.add(server); + } + return [...servers];packages/federation-sdk/src/services/event-authorization.service.ts (4)
288-339: ACL semantics bug (missing allow should not deny-all) and host handlingSpec: missing allow => allow all (subject to deny and allow_ip_literals). Also perform checks on host-only (no port). Current code denies-all and matches against host:port.
Apply:
- const serverAclContent = aclEvent.getContent(); - const { - allow = [], - deny = [], - allow_ip_literals = true, - } = serverAclContent; - - const isIpLiteral = - /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?$/.test(serverName) || - /^\[.*\](:\d+)?$/.test(serverName); // IPv6 + const serverAclContent = aclEvent.getContent() as { + allow?: string[]; + deny?: string[]; + allow_ip_literals?: boolean; + }; + // Distinguish "missing" vs "present but empty" + const allow = Array.isArray(serverAclContent.allow) + ? serverAclContent.allow + : undefined; + const deny = Array.isArray(serverAclContent.deny) + ? serverAclContent.deny + : []; + const allow_ip_literals = serverAclContent.allow_ip_literals ?? true; + + const hostOnly = this.stripPort(serverName); + const isIpLiteral = + /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostOnly) || + /^\[[^\]]+\]$/.test(hostOnly); // IPv6 [addr] if (isIpLiteral && !allow_ip_literals) { this.logger.debug(`Server ${serverName} denied: IP literals not allowed`); return false; } for (const pattern of deny) { - if (this.matchesServerPattern(serverName, pattern)) { + if (this.matchesServerPattern(hostOnly, pattern)) { this.logger.debug( `Server ${serverName} matches deny pattern: ${pattern}`, ); return false; } } - // if allow list is empty, deny all servers (as per Matrix spec) - // empty allow list means no servers are allowed - if (allow.length === 0) { - this.logger.debug(`Server ${serverName} denied: allow list is empty`); - return false; - } + // If 'allow' is present and empty, deny all. If 'allow' is missing, allow all (subject to deny above). + if (allow !== undefined && allow.length === 0) { + this.logger.debug(`Server ${serverName} denied: allow list explicitly empty`); + return false; + } - for (const pattern of allow) { - if (this.matchesServerPattern(serverName, pattern)) { - this.logger.debug( - `Server ${serverName} matches allow pattern: ${pattern}`, - ); - return true; - } - } - - this.logger.debug(`Server ${serverName} not in allow list`); - return false; + if (allow !== undefined) { + for (const pattern of allow) { + if (this.matchesServerPattern(hostOnly, pattern)) { + this.logger.debug( + `Server ${serverName} matches allow pattern: ${pattern}`, + ); + return true; + } + } + this.logger.debug(`Server ${serverName} not in explicit allow list`); + return false; + } + // No explicit allow -> allowed + return true;
341-360: Avoid regex from untrusted ACL patterns; implement simple '*' matcherBuilding RegExp from input risks ReDoS and over-implements spec (only '*' wildcard needed).
Apply:
- private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } - } + private matchesServerPattern(serverName: string, pattern: string): boolean { + // Exact or match-all + if (serverName === pattern || pattern === '*') return true; + // Linear '*' matcher (no regex) + const parts = pattern.split('*'); + let pos = 0; + // Prefix check + if (!pattern.startsWith('*')) { + if (!serverName.startsWith(parts[0])) return false; + pos = parts[0].length; + parts.shift(); + } + // Middle segments in order + for (let i = 0; i < parts.length - 1; i++) { + const seg = parts[i]; + if (!seg) continue; + const idx = serverName.indexOf(seg, pos); + if (idx === -1) return false; + pos = idx + seg.length; + } + // Suffix check + const last = parts[parts.length - 1] ?? ''; + return pattern.endsWith('*') ? true : serverName.endsWith(last); + }
277-280: Do not log full Authorization headerAvoid leaking credentials; redact the header value.
Apply:
- this.logger.error( - { error, eventId, authorizationHeader, method, uri, body }, + this.logger.error( + { error, eventId, method, uri, body, authorizationHeader: '<redacted>' }, 'Error checking event access', );
195-213: Normalize to host-only for checks; compare against normalized membership listComparing raw serverName (may include :port) against serversInRoom causes false negatives. Normalize once and use consistently.
Apply:
- const roomId = event.event.room_id; + const roomId = event.event.room_id; + const originHost = this.stripPort(serverName); @@ - const aclEvent = state.get('m.room.server_acl:'); - const isServerAllowed = await this.checkServerAcl(aclEvent, serverName); + const aclEvent = state.get('m.room.server_acl:'); + const isServerAllowed = await this.checkServerAcl(aclEvent, originHost); @@ - const serversInRoom = await this.stateService.getServersInRoom(roomId); - if (serversInRoom.includes(serverName)) { + const serversInRoom = await this.stateService.getServersInRoom(roomId); + // Normalize membership servers to host-only before comparing + if (serversInRoom.some((s) => this.stripPort(s) === originHost)) { this.logger.debug(`Server ${serverName} is in room, allowing access`); return true; }Add inside the class:
private stripPort(serverName: string): string { // [IPv6]:port -> [IPv6] if (serverName.startsWith('[')) { const m = serverName.match(/^\[([^\]]+)\](?::\d+)?$/); return m ? `[${m[1]}]` : serverName; } // host:port -> host return serverName.replace(/:\d+$/, ''); }
🧹 Nitpick comments (2)
packages/room/src/manager/event-wrapper.ts (1)
178-187: LGTM: precise type guards for ACL and history_visibilityRuntime checks align with existing isState() pattern matching.
Consider brief JSDoc on each guard to aid discoverability in IDEs.
Also applies to: 182-187
packages/federation-sdk/src/services/event-authorization.service.ts (1)
196-207: Avoid fetching full room state for ACL and history checksFetching the entire state is expensive. Consider a targeted query for just m.room.server_acl and m.room.history_visibility.
Do you want a follow-up patch that adds StateService helpers to fetch specific state keys?
📜 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(2 hunks)packages/federation-sdk/src/services/state.service.ts(1 hunks)packages/room/src/manager/event-wrapper.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 (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
packages/federation-sdk/src/services/config.service.ts (2)
ConfigService(76-153)serverName(94-96)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 353-353: 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)
sampaiodiego
force-pushed
the
feat/acl-methods
branch
from
003ea79 to
f02fdbb
Compare
5 participants
Works along with RocketChat/Rocket.Chat#36873.
Summary by CodeRabbit