chore(federation): bring back domain setting #37033
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds a new Federation_Service_Domain setting, validates it at startup and on change, requires a valid domain to start the federation-matrix service, and stops/restarts the service when the domain changes. FederationMatrix now uses that setting for serverName and exposes getServerName(). i18n additions and minor .well-known comments included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin UI
participant Settings as Settings Store
participant Startup as Federation Startup
participant Matrix as Federation-Matrix Service
Admin->>Settings: update Federation_Service_Domain
Settings-->>Startup: settings.changed(Federation_Service_Domain)
Startup->>Startup: validateDomain(domain)
alt domain valid && hasLicense && enabled
alt Matrix running && domain changed
Startup->>Matrix: stop()
Startup->>Matrix: start(newDomain)
else Matrix not running
Startup->>Matrix: start(newDomain)
end
else not startable
opt Matrix running
Startup->>Matrix: stop()
end
end
sequenceDiagram
autonumber
participant App as App
participant FM as FederationMatrix
participant Bridge as Matrix Bridge
App->>FM: sendMessage(userId, message)
FM->>FM: resolve serverName (from Federation_Service_Domain)
FM->>FM: lookup/create bridged user (@user:serverName)
FM->>Bridge: send message as matrix user
Bridge-->>FM: ACK
FM-->>App: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/startup/federation.ts (1)
32-41: Unregister StreamerCentral 'broadcast' listener in stopService to prevent listener leaksFile: apps/meteor/ee/server/startup/federation.ts (around lines 32–41)
Each start registers StreamerCentral.on('broadcast', ...) but stopService never removes it — repeated starts stack listeners and cause duplicate typing notifications.let federationMatrixService: FederationMatrix | undefined; + let broadcastListener: + | ((name: string, eventName: string, args: any[]) => void) + | undefined; @@ - StreamerCentral.on('broadcast', (name, eventName, args) => { + broadcastListener = (name, eventName, args) => { if (!federationMatrixService) { return; } if (name === 'notify-room' && eventName.endsWith('user-activity')) { const [rid] = eventName.split('/'); const [user, activity] = args; void federationMatrixService.notifyUserTyping(rid, user, activity.includes('user-typing')); } - }); + }; + StreamerCentral.on('broadcast', broadcastListener);And in stopService:
logger.debug('Stopping federation-matrix service'); // TODO: Unregister routes // await unregisterFederationRoutes(federationMatrixService); + if (broadcastListener) { + // Prefer off if available, otherwise removeListener + (StreamerCentral as any).off?.('broadcast', broadcastListener) ?? + (StreamerCentral as any).removeListener?.('broadcast', broadcastListener); + broadcastListener = undefined; + }
🧹 Nitpick comments (7)
ee/packages/federation-matrix/src/api/.well-known/server.ts (1)
33-38: ETag/304 and caching polish.Consider quoting the ETag and returning 304 when If-None-Match matches to avoid re-sending the body. Optionally add Cache-Control.
Apply this diff:
- const etag = createHash('md5') - .update(JSON.stringify(responseData)) - .digest('hex'); + const etag = `"${createHash('md5') + .update(JSON.stringify(responseData)) + .digest('hex')}"`; + + // Optional: short-circuit if client cache is fresh + if (c.req.headers['if-none-match'] === etag) { + return { statusCode: 304 }; + } @@ c.header('ETag', etag); c.header('Content-Type', 'application/json'); + // Optional: public cache for a short period + c.header('Cache-Control', 'public, max-age=300');apps/meteor/server/settings/federation-service.ts (1)
16-23: Add lightweight validation and guidance.To prevent inputs like schemes/ports, consider validating against a host-like pattern or documenting examples via i18nDescription (e.g., “my.company.tld”, no scheme/port). Keeps config consistent across environments.
apps/meteor/ee/server/startup/federation.ts (2)
84-91: Serialize start/stop to avoid race conditions across watchers.Concurrent domain/license/enable toggles can interleave start/stop. Add a simple transition guard and use it in watchers.
Apply this diff to watchers (and add helpers as shown below):
- settings.watch('Federation_Service_Enabled', async (enabled) => { + settings.watch('Federation_Service_Enabled', async (enabled) => { logger.debug('Federation_Service_Enabled setting changed:', enabled); - if (shouldStartService()) { - await startService(); - } else { - await stopService(); - } + await reconcile(); }); settings.watch('Federation_Service_Domain', async (domain) => { logger.debug('Federation_Service_Domain setting changed:', domain); - if (shouldStartService()) { - if (domain !== federationMatrixService?.getServerName()) { - await stopService(); - } - await startService(); - } else { - await stopService(); - } + await reconcile(domain); });Add these helpers near start/stopService:
let transitioning = false; const reconcile = async (newDomain?: string) => { if (transitioning) return; transitioning = true; try { if (!shouldStartService()) { await stopService(); return; } if (newDomain && newDomain !== federationMatrixService?.getServerName()) { await stopService(); } await startService(); } finally { transitioning = false; } };Also applies to: 93-103
59-61: Unregister routes on stop to prevent duplicates after restarts.Route re-registration on every start without unregistering risks duplicate handlers or conflicting middleware. Please implement the TODO.
I can draft unregisterFederationRoutes if you share the expected router teardown API.
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
547-558: Auto-heal bridged user mapping on send: good; generalize across code paths.This ensures messages keep flowing after username/domain changes. Recommend extracting a helper (and using it in deleteMessage, leaveRoom, kickUser, etc.) to keep behavior consistent.
Apply this diff here (and repeat usage elsewhere):
- const existingMatrixUserId = await MatrixBridgedUser.getExternalUserIdByLocalUserId(user._id); - // TODO: Not sure if this should be made here, but, what happens if the user changed his username or his server domain changed? - if (!existingMatrixUserId || existingMatrixUserId !== matrixUserId) { - await MatrixBridgedUser.createOrUpdateByLocalId(user._id, matrixUserId, true, this.serverName); - } + const existingMatrixUserId = await this.ensureLocalUserBridged(user._id, user.username); @@ - const actualMatrixUserId = matrixUserId; + const actualMatrixUserId = existingMatrixUserId || matrixUserId;Add this method to the class:
private async ensureLocalUserBridged(localUserId: string, username?: string): Promise<string | undefined> { const current = await MatrixBridgedUser.getExternalUserIdByLocalUserId(localUserId); if (username && (!current || !current.startsWith(`@${username}:`))) { const newId = `@${username}:${this.serverName}`; await MatrixBridgedUser.createOrUpdateByLocalId(localUserId, newId, true, this.serverName); return newId; } return current; }
98-98: ENV flag always enables thumbnails.The expression
process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || trueis always true. Honor “false” to disable.Apply this diff:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS + ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true' + : true,packages/i18n/src/locales/en.i18n.json (1)
2153-2155: Good addition; tighten microcopy for consistency and clarity.
- Matches existing Federation copy patterns; HTML breaks are consistent.
- Minor wording/consistency nits: elsewhere we prefer “Example:” over “e.g.” and “Enter only the domain” over “Inform only…”. Also tighten the alert.
Apply this diff (copy-only; no behavioral impact):
- "Federation_Service_Domain_Description": "The domain that this server should respond to, for example: `acme.com`. This will be used as the suffix for user IDs (e.g., `@user:acme.com`).<br/>If your chat server is accessible from a different domain than the one you want to use for federation, you should follow our documentation to configure the `.well-known` file on your web server.", - "Federation_Service_Domain_Alert": "Inform only the domain, do not include http(s)://, slashes or any path after it.<br/>Use something like `acme.com` and not `https://acme.com/chat`.", + "Federation_Service_Domain_Description": "The domain this server should respond to. Example: `acme.com`. Used as the suffix for user IDs (Example: `@user:acme.com`).<br/>If your chat server is accessible from a different domain than the one you want to use for federation, see our documentation to configure the `.well-known` file on your web server.", + "Federation_Service_Domain_Alert": "Enter only the domain; do not include http(s)://, slashes, or any path.<br/>Use `acme.com`, not `https://acme.com/chat`.",Optional: consider “Federation Domain” instead of “Federated Domain” for the setting label to align with nearby “Federation …” labels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/meteor/ee/server/startup/federation.ts(2 hunks)apps/meteor/server/settings/federation-service.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)ee/packages/federation-matrix/src/api/.well-known/server.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/startup/federation.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/i18n/src/locales/en.i18n.jsonee/packages/federation-matrix/src/api/.well-known/server.tsapps/meteor/server/settings/federation-service.ts
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/core-services/src/index.ts (1)
Settings(182-182)packages/models/src/index.ts (1)
Settings(203-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
ee/packages/federation-matrix/src/api/.well-known/server.ts (1)
20-20: Clarify restart scope or make the route hot-reload the domain.The TODO says “server is restarted,” but this PR restarts only the federation-matrix service on domain change. If the route still serves the old domain post service restart, fetch the domain from a live source (e.g., settings) instead of a container-cached value; otherwise, update the comment to “service is restarted.”
apps/meteor/ee/server/startup/federation.ts (1)
19-21: Requiring a domain to start is correct.The extra guard avoids misconfigured starts.
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
63-66: Sourcing domain from Federation_Service_Domain is correct.Matches the new setting; startup already gates on presence. Just ensure empty string is never returned here.
150-151: Useful startup log.Good operational visibility for domain-bound instances.
189-191: Accessor looks good.Needed for the domain watcher to compare and reconcile.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37033 +/- ##
===========================================================
- Coverage 71.23% 69.75% -1.48%
===========================================================
Files 1456 3035 +1579
Lines 75179 103431 +28252
Branches 10719 18381 +7662
===========================================================
+ Hits 53552 72147 +18595
- Misses 21303 29409 +8106
- Partials 324 1875 +1551
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/server/settings/federation-service.ts (1)
18-25: Wire i18n label/description to ensure UI texts render.Explicitly set i18n keys for the new setting.
await this.add('Federation_Service_Domain', hostname, { type: 'string', + i18nLabel: 'Federation_Service_Domain', + i18nDescription: 'Federation_Service_Domain_Description', public: false, enterprise: true, modules: ['federation'], invalidValue: '', alert: 'Federation_Service_Domain_Alert', });apps/meteor/ee/server/startup/federation.ts (1)
32-35: Avoid calling .trim() on possibly undefined setting.Coerce to string before trimming.
- const domain = settings.get<string>('Federation_Service_Domain').trim(); + const domain = (settings.get<string>('Federation_Service_Domain') || '').trim();
📜 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)
apps/meteor/ee/server/startup/federation.ts(2 hunks)apps/meteor/server/settings/federation-service.ts(2 hunks)ee/packages/federation-matrix/src/api/.well-known/server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/src/api/.well-known/server.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/settings/federation-service.tsapps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/startup/federation.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(40-1013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/server/settings/federation-service.ts (1)
14-18: Guard URL parsing with try/catch to avoid startup crashes on malformed Site_Url.
new URL(siteUrl)will throw if Site_Url is invalid. Keep empty on failure.Apply this diff:
- const siteUrl = settings.get<string>('Site_Url'); - - const hostname = siteUrl ? new URL(siteUrl).hostname : ''; // since this setting controls service start up, better to set empty string than an invalid URL + const siteUrl = settings.get<string>('Site_Url'); + let hostname = ''; + try { + if (typeof siteUrl === 'string' && siteUrl.trim()) { + hostname = new URL(siteUrl).hostname; + } + } catch { + hostname = ''; + }
rodrigok
force-pushed
the
chore/federation-domain-config
branch
from
d166b7e to
e729df4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
63-63: Minor: Missing null check for domain setting.The code calls
.trim()on the result ofSettings.get<string>()without checking if it's null or undefined first.Apply this diff to add null safety:
- const serverHostname = await Settings.get<string>('Federation_Service_Domain').trim(); + const serverHostname = (await Settings.get<string>('Federation_Service_Domain') || '').trim();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/ee/server/startup/federation.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/core-services/src/index.ts (1)
Settings(182-182)packages/models/src/index.ts (1)
Settings(203-203)
apps/meteor/ee/server/startup/federation.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(40-1013)
🪛 Biome (2.1.2)
apps/meteor/ee/server/startup/federation.ts
[error] 33-33: Expected an expression but instead found ';'.
Expected an expression here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
apps/meteor/ee/server/startup/federation.ts (3)
46-52: LGTM! The domain validation is properly integrated into startup conditions.The function correctly requires a license, enabled federation, and a valid domain for the service to start.
16-41: Critical: Fix the syntax error and add input validation.The function has a syntax error on Line 33 (
throw;without a proper error) and lacks proper input validation. The previous review comment correctly identified this issue.The
validateDomainfunction needs to be fixed to handle invalid inputs and syntax errors:function validateDomain(domain: string): boolean { - const value = domain.trim(); + const value = (domain || '').trim(); if (!value) { logger.error('The Federation domain is not set'); return false; } if (value.toLowerCase() != value) { logger.error(`The Federation domain "${value}" cannot have uppercase letters`); return false; } try { const valid = new URL(`https://${value}`).hostname === value; if (!valid) { - throw; + logger.error(`The configured Federation domain "${value}" is not valid`); + return false; } + + return true; } catch { logger.error(`The configured Federation domain "${value}" is not valid`); return false; } - - return true; }
124-134: Major: Race conditions and inefficient domain comparison remain unaddressed.The previous review comment correctly identified multiple issues that need to be resolved for production reliability.
The domain watcher needs several improvements:
- Normalize domain comparison to avoid unnecessary restarts
- Serialize stop/start operations to prevent race conditions
- Manage StreamerCentral listeners to prevent duplication
Apply this fix for the domain comparison normalization:
settings.watch<string>('Federation_Service_Domain', async (domain) => { logger.debug('Federation_Service_Domain setting changed:', domain); if (shouldStartService()) { - if (domain.toLowerCase() !== federationMatrixService?.getServerName().toLowerCase()) { + const nextDomain = (domain || '').trim().toLowerCase(); + const currentDomain = federationMatrixService?.getServerName()?.toLowerCase(); + if (nextDomain !== currentDomain) { await stopService(); } await startService(); } else { await stopService(); } });Additionally, you should implement serialization for all lifecycle operations and proper StreamerCentral listener management as detailed in the previous review.
ee/packages/federation-matrix/src/FederationMatrix.ts (5)
150-150: LGTM! Clear startup logging for federation domain.The log message provides helpful information about which domain the Federation Matrix Homeserver is created for.
189-191: LGTM! Clean accessor method implementation.The
getServerName()method provides appropriate access to the server name for external consumers like the startup validation.
546-549: Good improvement for user mapping consistency.The logic now ensures that bridged user mappings are created or updated when the Matrix user ID differs from the existing mapping, which handles domain changes properly.
556-557: LGTM! Simplified and consistent Matrix user ID handling.Using
Based on my research, the documentation confirms that "Homeserver Domain" is a legitimate configuration setting that should represent "Your Matrix homeserver's name, the same as you've configured in your Matrix configuration file". The changes in this PR appear to be implementing the `Federation_Service_Domain` setting to replace the previous domain configuration mechanism. Let me continue with the review:matrixUserIddirectly is more reliable since it's now guaranteed to be current after the update above.
546-546: Good TODO comment regarding username/domain changes.The TODO comment raises an important architectural question about handling username changes or domain changes. This is a valid concern for federation systems where identity consistency is crucial.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/server/settings/federation-service.ts (1)
14-16: Improve defensive URL parsing against malformed Site_Url.The current implementation doesn't handle malformed URLs gracefully and could throw exceptions during startup if Site_Url contains invalid data.
Apply this diff to add try-catch protection:
- const siteUrl = settings.get<string>('Site_Url'); - - const hostname = siteUrl ? new URL(siteUrl).hostname : ''; // since this setting controls service start up, better to set empty string than an invalid URL + const siteUrl = settings.get<string>('Site_Url'); + let hostname = ''; + + if (siteUrl) { + try { + hostname = new URL(siteUrl).hostname; + } catch { + logger.warn(`Invalid Site_Url format: "${siteUrl}", defaulting Federation_Service_Domain to empty string`); + } + }ee/packages/federation-matrix/src/api/.well-known/server.ts (1)
20-21: Important architectural concerns documented.These TODO comments highlight critical issues with the current well-known implementation:
- Domain changes require server restart to take effect in well-known responses
- The well-known route serving logic may be incorrect when Site_Url differs from the federation domain
Based on the past review comments, these are known issues that need architectural fixes rather than simple code changes.
Consider implementing a domain change handler that can update the well-known responses without requiring a full server restart, and evaluate whether the well-known route should conditionally return 404 based on domain/Site_Url mismatch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/meteor/ee/server/startup/federation.ts(2 hunks)apps/meteor/server/settings/federation-service.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/.well-known/server.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/i18n/src/locales/en.i18n.json
- ee/packages/federation-matrix/src/FederationMatrix.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/settings/federation-service.tsapps/meteor/ee/server/startup/federation.tsee/packages/federation-matrix/src/api/.well-known/server.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/startup/federation.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-950)packages/core-services/src/index.ts (1)
License(160-160)
🪛 Biome (2.1.2)
apps/meteor/ee/server/startup/federation.ts
[error] 33-33: Expected an expression but instead found ';'.
Expected an expression here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/server/settings/federation-service.ts (2)
1-1: LGTM!The import statement correctly brings in the necessary settings module for the federation service configuration.
18-25: LGTM!The Federation_Service_Domain setting is properly configured with appropriate enterprise restrictions and validation behavior. The empty invalidValue ensures the service won't start with invalid domain configurations.
ee/packages/federation-matrix/src/api/.well-known/server.ts (1)
32-36: LGTM!Minor formatting improvements to the well-known route handler without functional changes.
apps/meteor/ee/server/startup/federation.ts (2)
49-51: LGTM!The updated shouldStartService logic correctly incorporates domain validation alongside license and enabled checks. The validateDomain function ensures the service only starts with a valid, properly formatted domain.
124-134: LGTM!The domain change watcher properly handles service restart when the domain setting changes. The logic correctly compares domains case-insensitively and stops/restarts the service as needed.
rodrigok
force-pushed
the
chore/federation-domain-config
branch
from
e729df4 to
42dd0d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/startup/federation.ts (1)
74-80: Clean up on partial start failures.If route/service registration fails, the service and listener remain set; ensure cleanup.
try { api.registerService(federationMatrixService); await registerFederationRoutes(federationMatrixService); } catch (error) { logger.error('Failed to start federation-matrix service:', error); + await stopService(); }
🧹 Nitpick comments (1)
apps/meteor/ee/server/startup/federation.ts (1)
49-52: Defensively coalesce the setting before validation.Even with a hardened validator, coalescing avoids accidental undefined propagation and keeps intent obvious.
- const domain = settings.get<string>('Federation_Service_Domain'); - const hasDomain = validateDomain(domain); + const domain = settings.get<string>('Federation_Service_Domain') ?? ''; + const hasDomain = validateDomain(domain);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/ee/server/startup/federation.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/startup/federation.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-950)packages/core-services/src/index.ts (1)
License(160-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/ee/server/startup/federation.ts (3)
97-134: Serialize lifecycle operations across watchers to avoid interleaved start/stop.License and settings watchers can race; serialize with a simple promise lock.
Minimal pattern (add near top of function and wrap watcher bodies):
let lifecycle = Promise.resolve(); const withLock = <T>(fn: () => Promise<T>) => (lifecycle = lifecycle.then(fn, fn)); // Example usage: settings.watch('Federation_Service_Enabled', async (enabled) => { logger.debug('Federation_Service_Enabled setting changed:', enabled); await withLock(async () => { if (shouldStartService()) { await startService(); } else { await stopService(); } }); }); settings.watch<string>('Federation_Service_Domain', async (domain) => { logger.debug('Federation_Service_Domain setting changed:', domain); await withLock(async () => { // ...normalized compare and start/stop as in previous comment }); }); void License.onLicense('federation', async () => { logger.debug('Federation license became available'); await withLock(async () => { if (shouldStartService()) { await startService(); } }); }); License.onInvalidateLicense(async () => { logger.debug('License invalidated, checking federation module'); await withLock(async () => { if (!shouldStartService()) { await stopService(); } }); });
124-134: Null-safe, normalized comparison; current code can throw when service is undefined.
federationMatrixService?.getServerName().toLowerCase()will calltoLowerCase()onundefinedwhen the service isn’t started. Normalize both sides and guard.- settings.watch<string>('Federation_Service_Domain', async (domain) => { - logger.debug('Federation_Service_Domain setting changed:', domain); - if (shouldStartService()) { - if (domain.toLowerCase() !== federationMatrixService?.getServerName().toLowerCase()) { - await stopService(); - } - await startService(); - } else { - await stopService(); - } - }); + settings.watch<string>('Federation_Service_Domain', async (domain) => { + logger.debug('Federation_Service_Domain setting changed:', domain); + if (shouldStartService()) { + const next = String(domain ?? '').trim().toLowerCase(); + const current = federationMatrixService?.getServerName()?.toLowerCase(); + if (next !== current) { + await stopService(); + } + await startService(); + } else { + await stopService(); + } + });
16-41: Fix undefined.trim()crash in validateDomain and simplify invalid-path handling.
settings.get<string>(...)can be undefined at runtime; callingtrim()will throw. Also, prefer returning false instead of throwing and catching immediately.Apply this diff:
-function validateDomain(domain: string): boolean { - const value = domain.trim(); +function validateDomain(domain: string): boolean { + const value = String(domain ?? '').trim(); if (!value) { logger.error('The Federation domain is not set'); return false; } if (value.toLowerCase() !== value) { logger.error(`The Federation domain "${value}" cannot have uppercase letters`); return false; } try { - const valid = new URL(`https://${value}`).hostname === value; - - if (!valid) { - throw new Error(); - } - } catch { + const url = new URL(`https://${value}`); + const valid = url.hostname === value; + if (!valid) { + logger.error(`The configured Federation domain "${value}" is not valid`); + return false; + } + } catch (error) { logger.error(`The configured Federation domain "${value}" is not valid`); return false; } return true; }
| let federationMatrixService: FederationMatrix | undefined; | ||
|
|
There was a problem hiding this comment.
Prevent broadcast-listener leaks across restarts; remove on stop and register once.
Each stop/start cycle adds another listener; they’re never removed. This can duplicate events and waste memory.
Apply these diffs:
@@
export const startFederationService = async (): Promise<void> => {
let federationMatrixService: FederationMatrix | undefined;
+ let broadcastHandler: ((name: string, eventName: string, args: any[]) => void) | undefined;
@@
- StreamerCentral.on('broadcast', (name, eventName, args) => {
- if (!federationMatrixService) {
- return;
- }
- if (name === 'notify-room' && eventName.endsWith('user-activity')) {
- const [rid] = eventName.split('/');
- const [user, activity] = args;
- void federationMatrixService.notifyUserTyping(rid, user, activity.includes('user-typing'));
- }
- });
+ broadcastHandler = (name, eventName, args) => {
+ if (!federationMatrixService) {
+ return;
+ }
+ if (name === 'notify-room' && eventName.endsWith('user-activity')) {
+ const [rid] = eventName.split('/');
+ const [user, activity] = args;
+ void federationMatrixService.notifyUserTyping(rid, user, activity.includes('user-typing'));
+ }
+ };
+ // @ts-expect-error: EventEmitter-like API
+ StreamerCentral.on?.('broadcast', broadcastHandler);
@@
const stopService = async (): Promise<void> => {
if (!federationMatrixService) {
logger.debug('Federation-matrix service not registered... skipping');
return;
}
logger.debug('Stopping federation-matrix service');
// TODO: Unregister routes
// await unregisterFederationRoutes(federationMatrixService);
+ if (broadcastHandler) {
+ // @ts-expect-error: removeListener/off exist on StreamerCentral emitter
+ StreamerCentral.removeListener?.('broadcast', broadcastHandler);
+ // @ts-expect-error
+ StreamerCentral.off?.('broadcast', broadcastHandler);
+ broadcastHandler = undefined;
+ }
+
await api.destroyService(federationMatrixService);
federationMatrixService = undefined;
};Also applies to: 63-74, 82-95
🤖 Prompt for AI Agents
In apps/meteor/ee/server/startup/federation.ts around lines 44-45 (and similarly
for blocks ~63-74 and ~82-95), the broadcast-listener is being added on every
start without being removed on stop, causing duplicate handlers across restart
cycles; fix by registering the listener exactly once and removing it on stop:
keep a single reference to the listener function (or use a boolean guard) when
adding it so repeated starts don’t re-register, and on the stop/shutdown path
call the corresponding removeListener/off with that same function reference (and
clear the reference) so the listener is fully deregistered between restarts.
Ensure the lifecycle uses the same federationMatrixService instance checks
before registering and that cleanup runs unconditionally on stop.
ggazzo
force-pushed
the
chore/federation-backup
branch
from
c20e2ec to
6fdad20
Compare
ggazzo
force-pushed
the
chore/federation-domain-config
branch
from
fc0dfd1 to
b2cefcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/ee/server/startup/federation.ts(2 hunks)apps/meteor/server/settings/federation-service.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/.well-known/server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/server/settings/federation-service.ts
- ee/packages/federation-matrix/src/api/.well-known/server.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/models/src/index.ts (1)
Settings(205-205)packages/core-services/src/index.ts (1)
Settings(185-185)
apps/meteor/ee/server/startup/federation.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-966)packages/core-services/src/index.ts (1)
License(162-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
73-73: LGTM!The change to derive serverName from
Federation_Service_Domainsetting (instead of parsingSite_Url) aligns perfectly with the PR objectives to restore a configurable domain/identity setting separate from siteurl.
168-169: LGTM!The startup log message provides good visibility into which domain the Federation Matrix Homeserver is using, which will be helpful for debugging domain-related configuration issues.
207-210: LGTM!The new
getServerName()public accessor is needed by the startup federation code to compare current vs. new domains when the setting changes.apps/meteor/ee/server/startup/federation.ts (4)
49-51: LGTM!The integration of domain validation into the service startup logic ensures that the federation service only starts when a valid domain is configured, which aligns with the PR objectives.
63-72: Broadcast listener accumulates across service restarts.Each time the service starts, a new broadcast listener is added to
StreamerCentralbut they are never removed when the service stops. This causes duplicate event handlers and memory leaks across restart cycles.This is a known issue from previous review comments that needs to be addressed by storing the listener reference and removing it during service shutdown.
The search results don't provide specific information about the `Federation_Service_Domain` setting. Let me search for more specific information about Matrix federation settings in Rocket.Chat.Based on the search results, I can see references to Homeserver Domain: Your Matrix homeserver's name, the same as you've configured in your Matrix configuration file and mentions of the Federation_Matrix_homeserver_domain setting in older documentation, but the new `Federation_Service_Domain` setting appears to be a newer implementation. The search results don't provide clear information about the specific validation rules or recent changes. Let me verify if the new setting name is correctly implemented.
13-41: Syntax error in throw statement requires fix.The bare
throw;statement on Line 33 is invalid syntax and will cause a compilation error.Apply this diff to fix the syntax error:
- if (!valid) { - throw new Error(); - } + if (!valid) { + logger.error(`The configured Federation domain "${value}" hostname does not match expected format`); + return false; + }
124-134: Check for potential null/undefined access and improve domain comparison.The code calls
domain.toLowerCase()andfederationMatrixService?.getServerName().toLowerCase()without null checks, which could cause runtime errors if the domain parameter is null/undefined or ifgetServerName()returns undefined.Apply this diff to add proper validation:
- if (shouldStartService()) { - if (domain.toLowerCase() !== federationMatrixService?.getServerName().toLowerCase()) { - await stopService(); - } + if (shouldStartService()) { + const newDomain = String(domain ?? '').trim().toLowerCase(); + const currentDomain = federationMatrixService?.getServerName()?.toLowerCase(); + if (newDomain !== currentDomain) { + await stopService(); + }
| function validateDomain(domain: string): boolean { | ||
| const value = domain.trim(); | ||
|
|
||
| if (!value) { | ||
| logger.error('The Federation domain is not set'); | ||
| return false; | ||
| } | ||
|
|
||
| if (value.toLowerCase() !== value) { | ||
| logger.error(`The Federation domain "${value}" cannot have uppercase letters`); | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| const valid = new URL(`https://${value}`).hostname === value; | ||
|
|
||
| if (!valid) { | ||
| throw new Error(); | ||
| } | ||
| } catch { | ||
| logger.error(`The configured Federation domain "${value}" is not valid`); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Verify the domain validation requirements against the latest Matrix specification.
The validateDomain function implements several validation rules (trimming, non-empty check, lowercase requirement, URL hostname validation), but it's unclear if these requirements align with the latest Matrix homeserver domain specifications.
Based on the search results, Matrix homeserver domains should match what you've configured in your Matrix configuration file, but the specific validation requirements for domain format aren't clearly documented. Please search for the latest Matrix specification requirements for homeserver domain validation:
🌐 Web query:
Matrix homeserver domain name validation requirements specification RFC
💡 Result:
Key sources that specify Matrix homeserver domain-name validation and requirements:
-
Matrix specification — Server name grammar & rules (server_name = hostname [ ":" port ], hostname may be IPv4/IPv6 literal or DNS name; recommendations on length/case; based on RFC 1123). [1][2]
-
Matrix Server-Server (federation) / discovery rules — how homeserver names are resolved: use /.well-known/matrix/server, then _matrix._tcp SRV, then connect to hostname:8448; certificate and Host header expectations for IP literals vs DNS names. [3]
-
Matrix historical/r0.1.2 federation text describing certificate/Host header handling and resolution order (useful for precise validation behaviour). [4]
-
Related discussion: SVCB/HTTPS records are not currently supported for homeserver discovery (implementation/issue thread). [5]
References
- Matrix spec — Appendices: Identifier grammar / Server Name. (spec.matrix.org)
- Matrix spec — Appendices (unstable/current). (spec.matrix.org)
- Matrix spec — Server-Server API / Server discovery (/.well-known, SRV, port and TLS behaviour). (spec.matrix.org)
- Matrix Federation API (historical r0.1.2) — resolution and certificate handling. (spec.matrix.org)
- Issue: Matrix does not support SVCB DNS records for homeserver discovery. (github.com)
If you want, I can extract the exact server_name grammar and the TLS/Host-header rules into a short spec excerpt.
Citations:
- 1: https://spec.matrix.org/legacy/appendices.html?utm_source=openai
- 2: https://spec.matrix.org/unstable/appendices/?utm_source=openai
- 3: https://spec.matrix.org/unstable/server-server-api/?utm_source=openai
- 4: https://spec.matrix.org/historical/server_server/r0.1.2?utm_source=openai
- 5: Matrix does not support SVCB DNS records for homeserver discovery matrix-org/matrix-spec#1220
Align validateDomain with Matrix server_name grammar
Current checks (rejecting uppercase; strict new URL(...).hostname === value) will reject valid Matrix server_name forms (IDNs, optional ports, IPv6 literals). Actions:
- apps/meteor/ee/server/startup/federation.ts (validateDomain, lines 16-41): remove the uppercase-only rejection; treat hostnames case‑insensitively and normalize to lowercase for storage/compare if needed.
- Replace the
URLequality check with validation that follows Matrix server_name = hostname [ ":" port ] — allow DNS names, IPv4, bracketed IPv6 literals, and optional port; support IDNs by converting to ASCII (punycode) before validation or use a robust host-parser library. - Confirm whether the configured value may include a port and ensure port-range validation and TLS/Host-header rules for IP literals match the Matrix Server‑Server spec.
🤖 Prompt for AI Agents
In apps/meteor/ee/server/startup/federation.ts around lines 16-41, the
validateDomain currently rejects uppercase and uses strict new URL(...).hostname
equality which incorrectly rejects valid Matrix server_name forms (IDNs,
optional ports, IPv6 literals); remove the uppercase-only rejection and treat
hostnames case-insensitively (normalize to lowercase for storage/compare), and
replace the URL equality check with a server_name-aware validation: parse
optional ":port" suffix, convert the host part to ASCII/punycode (using Node's
punycode.toASCII or a host-parser library) and validate it as either a DNS name,
IPv4 address, or bracketed IPv6 literal; if a port is present ensure it is an
integer in 1-65535; use a well-tested host parsing/validation library where
possible to handle IDNs and IPv6 correctly and return true only if host (after
punycode) and optional port meet these rules.
https://rocketchat.atlassian.net/browse/FDR-130
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Documentation