Skip to content

Conversation

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Sep 18, 2025

https://rocketchat.atlassian.net/browse/FDR-133

Organize the OLD and NEW federation settings:

  1. Removes the Federation Service group and move all new settings to Federation group
  2. Add NOT SUPPORTED to both old implementation sections
  3. Hide all settings of Rocket.Chat Federation but the enable one
  4. Add alerts of version not supported on both old implementations
  5. Disable all visible settings of both old implementations
  6. Split new signing key into 3 settings (algorithm, version and key)
  7. Change Signing Key to password type
  8. Add setting for maximium size of room when joining external rooms (TODO: implement on code)
  9. Add alert of Alfa version for the new federation

If running a previous version of development native federation remove the following settings:

db.rocketchat_settings.deleteOne({_id: 'Federation_Service_Matrix_Port'})
db.rocketchat_settings.deleteOne({_id: 'Federation Service'})

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features
    • Matrix Bridge admin settings added (signing algorithm, signing version/key) and a remote-public-room join size limit.
  • Changes
    • Federation settings consolidated under “Federation”; many options are now read-only or hidden and alerts/descriptions updated for Matrix.
  • Localization
    • Removed legacy federation enablement texts in many locales; English strings updated with Matrix-focused alerts/descriptions.
  • Chores
    • Added .env to .gitignore.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Sep 18, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Sep 18, 2025

⚠️ No Changeset found

Latest commit: c056235

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 18, 2025

Walkthrough

Renames and restructures Federation settings, adds Matrix signing algorithm/version/key and room-size controls, restricts legacy federation UI settings (readonly/hidden), changes settings initialization order with a new Matrix Bridge registrar, updates Federation i18n entries across locales, and adds .env to .gitignore.

Changes

Cohort / File(s) Summary
Federation service settings updates
apps/meteor/server/settings/federation-service.ts
Group renamed to "Federation"; Federation_Service_Enabled props updated; removed Matrix port; added Federation_Service_Matrix_Signing_Algorithm, Federation_Service_Matrix_Signing_Version, Federation_Service_Matrix_Signing_Key (password), and Federation_Service_max_allowed_size_of_public_rooms_to_join.
Legacy federation admin UI restrictions
apps/meteor/server/settings/federation.ts
Marked multiple FEDERATION_* settings readonly/hidden and updated FEDERATION_Enabled alert key.
Matrix bridge signing config consumption
ee/packages/federation-matrix/src/FederationMatrix.ts
Reads Signing_Algorithm, Signing_Version, and Signing_Key; composes composite signingKey as <algorithm>:<version>:<key> for initialization.
Matrix Bridge settings registration
apps/meteor/server/services/federation/Settings.ts
New exported async addMatrixBridgeFederationSettings() registers Matrix bridge settings (ids, hs/as tokens, HS/bridge URLs and domain, registration file, max public room size, config status and check action).
Settings initialization order and deprecations
apps/meteor/server/settings/index.ts
Imports new registrar, adjusts Promise.all ordering, and invokes deprecated createFederationSettings() and addMatrixBridgeFederationSettings() in a post-initialization step (kept for admin UI info).
i18n removals and additions (en)
packages/i18n/src/locales/en.i18n.json
Removed legacy FEDERATION enable/alerts and Matrix Port/Domain keys; added Old_Federation_Alert, "Rocket.Chat Federation", "Matrix Bridge", Matrix signing keys/descriptions, Federation_Service_max_allowed_size_of_public_rooms_to_join, and Federation_Service_Alert; updated Federation_Description.
i18n removals (many locales)
packages/i18n/src/locales/*
Removed FEDERATION_Enabled and related alert/description keys across many locale files (e.g., ar, ca, cs, da, de*, es, fi, fr, hi-IN, hu, ja, ka-GE, ko, nb, nl, nn, pl, pt-BR, pt, ru, sv, uk, zh*, ...).
Repo hygiene
.gitignore
Added .env to ignore list.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Admin
  participant SettingsInit as Settings Initialization
  participant Registry as Settings Registry
  participant LegacyFed as Legacy Federation Settings
  participant MatrixBridge as Matrix Bridge Registrar

  Admin->>SettingsInit: Server startup
  SettingsInit->>Registry: createFederationServiceSettings()
  SettingsInit->>Registry: (other settings)
  Note over SettingsInit,Registry: Post-initialization (deprecated UI info)
  SettingsInit->>LegacyFed: createFederationSettings() [deprecated]
  LegacyFed->>Registry: register readonly/hidden FEDERATION_* keys
  SettingsInit->>MatrixBridge: addMatrixBridgeFederationSettings()
  MatrixBridge->>Registry: register Matrix IDs/tokens/URLs/domain/limits/etc.
Loading
sequenceDiagram
  autonumber
  participant FM as FederationMatrix.create()
  participant Settings as Settings Store
  participant Config as Matrix Config

  FM->>Settings: get(Federation_Service_Matrix_Signing_Algorithm)
  FM->>Settings: get(Federation_Service_Matrix_Signing_Version)
  FM->>Settings: get(Federation_Service_Matrix_Signing_Key)
  FM->>Config: build signingKey = "<algorithm>:<version>:<key>"
  FM-->>Config: init with composite signingKey
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

stat: ready to merge

Suggested reviewers

  • sampaiodiego

Poem

I nibble wires, hop through keys,
I stitch algos, versions, pleas.
Old flags hide, new bridges sing,
Tokens roll and rooms stay trim.
Thump-thump! A rabbit's joyful ping. 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning Most changes match the PR objectives to reorganize federation settings, but the addition of ".env" to .gitignore is unrelated to federation and appears out-of-scope for this feature PR, and the widespread removal of i18n keys across many locales without corresponding replacements or a stated translation/migration plan risks missing UI text in non-English locales; the PR description references DB setting deletions but no migration script or runbook is present in the provided summary. These elements indicate out-of-scope or incomplete items. Move the .gitignore ".env" change into a separate housekeeping PR or justify its inclusion here, restore or provide replacement translations for affected locales or document the intentional removal, and add an explicit migration script or documented DB runbook for the referenced settings deletions so reviewers can validate the migration path.
Linked Issues Check ❓ Inconclusive The linked issue entry (FDR-133: [QA] Alpha Version 7.11) in the provided data contains only a short label and no explicit acceptance criteria or checklist to map against the code changes; the PR implements many alpha-related adjustments (alerts, hiding/deprecations, signing-key split, new limits) that appear consistent with an "alpha" rework but cannot be concretely verified against the issue. Because there are no detailed requirements in the linked_issues payload, I cannot conclusively confirm full compliance. Please attach FDR-133's acceptance criteria or provide a mapping (checklist) of required items to the specific settings/files changed so I can verify each requirement against the PR.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "chore: organize federation settings" is a concise, single-sentence summary that accurately reflects the primary change set—reorganizing and consolidating federation-related settings (group rename, visibility/readonly changes, splitting signing key, and adding remote-join limit). It is clear, focused, and appropriate for a chore/refactor-style PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/federation-settings

Comment @coderabbitai help to get the list of available commands and usage tips.

@rodrigok rodrigok changed the title Feat/federation settings chore: organize federation settings Sep 18, 2025
@rodrigok
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 18, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)

89-89: enableThumbnails is always true due to “|| true”.

Config cannot disable thumbnails.

Apply:

-				enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true,
+				enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true' : true,

517-525: Possible crash when no quote attachment is present.

Accessing message_link on undefined will throw.

Apply:

-		const messageLink = (
-			message.attachments.find((attachment) => isQuoteAttachment(attachment) && Boolean(attachment.message_link)) as MessageQuoteAttachment
-		).message_link;
+		const quoteAttachment = message.attachments.find(
+			(attachment) => isQuoteAttachment(attachment) && Boolean((attachment as MessageQuoteAttachment).message_link),
+		) as MessageQuoteAttachment | undefined;
+		const messageLink = quoteAttachment?.message_link;

62-95: Sanity-check: config defaults and envs.

  • Validate/whitelist signing algorithm and version instead of trusting Settings; code only strips the literal "ed25519 0 " prefix — implement explicit allowed-algorithms and robust parsing (Settings reads: lines 48–50; signing key assembled in the config block).
  • Confirm SERVER_PORT and SERVER_VERSION fallbacks ('8080', '1.0') are intended in your deployment manifests (env fallbacks at lines 67–68).
  • Fix enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true always evaluates to true — parse to boolean or use an explicit default (line 89).
apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts (2)

231-238: Treat HS token as secret; use password type.

Prevents accidental exposure in UI/logs.

Apply:

-		await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
-			readonly: true,
-			type: 'string',
+		await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
+			readonly: true,
+			type: 'password',

239-247: Treat AS token as secret; use password type.

Apply:

-		await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
-			readonly: true,
-			type: 'string',
+		await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
+			readonly: true,
+			type: 'password',
🧹 Nitpick comments (8)
Makefile (2)

12-15: Avoid directory juggling; use Yarn’s --cwd.

Apply:

 i18n-build:
-	cd packages/i18n && yarn build
-	cd ../..
+	yarn --cwd packages/i18n build

3-3: Satisfy checkmake: add all/clean/test phony targets.

Apply:

-.PHONY: help remove-key remove-key-except-en
+.PHONY: help remove-key remove-key-except-en all clean test
+
+all: help
+
+clean:
+	@echo "Nothing to clean"
+
+test: i18n-build
ee/packages/federation-matrix/src/FederationMatrix.ts (1)

235-238: Drop console.log; rely on logger only.

Apply:

-			console.log(error);
 			this.logger.error('Failed to create room:', error);
apps/meteor/server/settings/federation-service.ts (2)

14-23: Algorithm select has a single option; consider readonly or add more options when supported.

No code change required now.


3-13: Migration hygiene — remove stale Federation_Service_Matrix_Port translations and references

No Settings.add('Federation_Service_Matrix_Port') is present in apps/meteor/server/settings/federation-service.ts, but packages/i18n/src/locales/en.i18n.json still contains Federation_Service_Matrix_Port_Description and Federation_Service_Matrix_Port_Alert; remove those i18n keys (and any copies in other locales) and any leftover references, then re-run a global search for "Federation_Service_Matrix_Port".

packages/i18n/src/locales/en.i18n.json (3)

2055-2057: Tone/wording tweaks for deprecation alerts.

  • "not supported anymore" → "no longer supported"
  • Avoid ALL CAPS; use sentence case for consistency across UI.

Apply:

- "Old_Federation_Alert": "This Federation version is not supported anymore. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More Information about Matrix Federation support can be found here</a>",
+ "Old_Federation_Alert": "This federation version is no longer supported. Please configure the new Native Federation above.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More information about Matrix federation support can be found here</a>",
- "Rocket.Chat Federation": "Rocket.Chat Federation (NOT SUPPORTED)",
+ "Rocket.Chat Federation": "Rocket.Chat Federation (Not supported)",
- "Matrix Bridge": "Matrix Bridge (NOT SUPPORTED)",
+ "Matrix Bridge": "Matrix Bridge (Not supported)",

2147-2147: Fix typos and wording in alpha warning.

  • "alfa" → "alpha"
  • "performatic" → "performant"
  • Prefer "production use" over "usage".
- "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+ "Federation_Service_Alert": "<strong>This is an alpha feature not intended for production use!</strong><br/>It may not be stable and/or performant. Be aware it may change, break, or be removed in the future without notice.",

2152-2154: Grammar/polish for max-room-size texts.

Improve clarity and flow.

- "Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
+ "Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind that the larger the room users are allowed to join, the longer the join will take and the more resources it will consume. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
- "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them",
+ "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The maximum number of members allowed in a remote public room that users can still join. Rooms exceeding this limit will still be listed, but users won't be able to join them.",
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between c70e153 and 3f9f00c.

📒 Files selected for processing (31)
  • Makefile (1 hunks)
  • apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts (12 hunks)
  • apps/meteor/server/settings/federation-service.ts (1 hunks)
  • apps/meteor/server/settings/federation.ts (4 hunks)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (2 hunks)
  • packages/i18n/src/locales/ar.i18n.json (0 hunks)
  • packages/i18n/src/locales/ca.i18n.json (0 hunks)
  • packages/i18n/src/locales/cs.i18n.json (0 hunks)
  • packages/i18n/src/locales/da.i18n.json (0 hunks)
  • packages/i18n/src/locales/de-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/de.i18n.json (0 hunks)
  • packages/i18n/src/locales/en.i18n.json (4 hunks)
  • packages/i18n/src/locales/es.i18n.json (0 hunks)
  • packages/i18n/src/locales/fi.i18n.json (0 hunks)
  • packages/i18n/src/locales/fr.i18n.json (0 hunks)
  • packages/i18n/src/locales/hi-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/hu.i18n.json (0 hunks)
  • packages/i18n/src/locales/ja.i18n.json (0 hunks)
  • packages/i18n/src/locales/ka-GE.i18n.json (0 hunks)
  • packages/i18n/src/locales/ko.i18n.json (0 hunks)
  • packages/i18n/src/locales/nb.i18n.json (0 hunks)
  • packages/i18n/src/locales/nl.i18n.json (0 hunks)
  • packages/i18n/src/locales/nn.i18n.json (0 hunks)
  • packages/i18n/src/locales/pl.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt-BR.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt.i18n.json (0 hunks)
  • packages/i18n/src/locales/ru.i18n.json (0 hunks)
  • packages/i18n/src/locales/sv.i18n.json (0 hunks)
  • packages/i18n/src/locales/uk.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh-TW.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh.i18n.json (0 hunks)
💤 Files with no reviewable changes (25)
  • packages/i18n/src/locales/de-IN.i18n.json
  • packages/i18n/src/locales/pt.i18n.json
  • packages/i18n/src/locales/ja.i18n.json
  • packages/i18n/src/locales/sv.i18n.json
  • packages/i18n/src/locales/nl.i18n.json
  • packages/i18n/src/locales/es.i18n.json
  • packages/i18n/src/locales/ca.i18n.json
  • packages/i18n/src/locales/ko.i18n.json
  • packages/i18n/src/locales/cs.i18n.json
  • packages/i18n/src/locales/da.i18n.json
  • packages/i18n/src/locales/fr.i18n.json
  • packages/i18n/src/locales/pl.i18n.json
  • packages/i18n/src/locales/zh-TW.i18n.json
  • packages/i18n/src/locales/ka-GE.i18n.json
  • packages/i18n/src/locales/ru.i18n.json
  • packages/i18n/src/locales/zh.i18n.json
  • packages/i18n/src/locales/uk.i18n.json
  • packages/i18n/src/locales/hi-IN.i18n.json
  • packages/i18n/src/locales/fi.i18n.json
  • packages/i18n/src/locales/ar.i18n.json
  • packages/i18n/src/locales/de.i18n.json
  • packages/i18n/src/locales/hu.i18n.json
  • packages/i18n/src/locales/nn.i18n.json
  • packages/i18n/src/locales/pt-BR.i18n.json
  • packages/i18n/src/locales/nb.i18n.json
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile

[warning] 3-3: Missing required phony target "all"

(minphony)


[warning] 3-3: Missing required phony target "clean"

(minphony)


[warning] 3-3: Missing required phony target "test"

(minphony)

⏰ 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: 📦 Build Packages
🔇 Additional comments (8)
apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts (1)

183-194: UI lock-down looks consistent with the deprecation path.

Readonly/hidden flags align with the PR goal.

Also applies to: 195-214, 296-308, 310-334

apps/meteor/server/settings/federation-service.ts (2)

35-41: Good: signing key now password-typed and non-public.


25-33: Signing version readonly = '0' — confirm future-proofing.

apps/meteor/server/settings/federation-service.ts registers 'Federation_Service_Matrix_Signing_Version' as readonly with default/invalid value '0' (and is only read in ee/packages/federation-matrix/src/FederationMatrix.ts). If signing key versions may change (rotation/upgrade) either make the setting editable (with validation) or implement programmatic rotation; also confirm that default/invalidValue '0' is intentional.

apps/meteor/server/settings/federation.ts (1)

8-15: Locking legacy federation is clear and consistent.

Readonly + hidden + deprecation alert looks correct.

Also applies to: 17-23, 24-33, 34-44, 45-63, 64-69

packages/i18n/src/locales/en.i18n.json (4)

2099-2099: LGTM: clearer federation description.

Updated copy is accurate and concise.


2148-2151: Security/compliance: ensure signing key setting is secret.

The labels/descriptions are fine, but the backing setting for "Signing Key" must be marked secret/non-exposed in admin APIs, logs, exports, and UI placeholders (masked). Verify server-side setting metadata enforces this.


2271-2271: LGTM: minor copy tidy.

Description reads well; no issues.


2055-2055: Verify removed legacy federation keys and signing-key exposure

rg returned "No files were searched" — manual verification required.

  • Confirm removed/renamed keys are no longer referenced: FEDERATION_Enabled, FEDERATION_Enabled_Alert, Federation_Matrix_Enabled_Alert, Federation_Service_Matrix_Port.
  • Confirm only the new signing keys are used: Federation_Service_Matrix_Signing_Algorithm, Federation_Service_Matrix_Signing_Version, Federation_Service_Matrix_Signing_Key.
  • Confirm the signing-key setting is hidden/secret (not public) in server settings metadata.

Affected file: packages/i18n/src/locales/en.i18n.json (lines 2055, 2147–2154)

Run from repo root and share outputs:

rg -nP -S '(FEDERATION_Enabled|FEDERATION_Enabled_Alert|Federation_Matrix_Enabled_Alert|Federation_Service_Matrix_Port\b)' --hidden -g '!dist/**' -g '!build/**' || true
rg -nP -S '\bFederation_Service_Matrix_Signing_(Algorithm|Version|Key)\b' --hidden -g '!dist/**' -g '!build/**' || true
rg -n -C3 'Federation_Service_Matrix_Signing_Key' --hidden -g '!dist/**' -g '!build/**' || true
git grep -nE '(FEDERATION_Enabled|Federation_Service_Matrix_Signing_Key|Federation_Service_Matrix_Port)' || true

@codecov
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.31%. Comparing base (4b1ac41) to head (c056235).
⚠️ Report is 1 commits behind head on chore/federation-backup.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                     @@
##           chore/federation-backup   #36991      +/-   ##
===========================================================
- Coverage                    67.33%   67.31%   -0.03%     
===========================================================
  Files                         3342     3342              
  Lines                       113397   113397              
  Branches                     20704    20719      +15     
===========================================================
- Hits                         76361    76338      -23     
- Misses                       34435    34456      +21     
- Partials                      2601     2603       +2     
Flag Coverage Δ
e2e 56.91% <ø> (-0.03%) ⬇️
unit 71.24% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rodrigok rodrigok marked this pull request as ready for review September 19, 2025 15:19
@rodrigok rodrigok requested a review from a team as a code owner September 19, 2025 15:19
@ggazzo ggazzo changed the base branch from feat/federation to chore/federation-backup September 19, 2025 17:38
@ggazzo ggazzo requested review from a team as code owners September 19, 2025 17:38
@ggazzo ggazzo force-pushed the feat/federation-settings branch 2 times, most recently from df7c671 to 7d75249 Compare September 19, 2025 17:45
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/meteor/server/settings/index.ts (1)

41-57: Duplicate legacy federation initializer called; Matrix initializer not invoked

createFederationServiceSettings is imported from ./federation-service and called twice (apps/meteor/server/settings/index.ts — import: line 14; calls: lines 42 and 56). The Matrix initializer at ../services/federation/Settings.ts is not imported or invoked. Replace the duplicate call with a single import/call for each initializer:

-import { createFederationServiceSettings } from './federation-service';
+import { createFederationServiceSettings as createLegacyFederationServiceSettings } from './federation-service';
+import { createFederationServiceSettings as createFederationMatrixSettings } from '../services/federation/Settings';

@@
 await Promise.all([
-	createFederationServiceSettings(),
+	createFederationMatrixSettings(),
@@
-	createFederationServiceSettings(),
+	createLegacyFederationServiceSettings(),
🧹 Nitpick comments (1)
apps/meteor/server/services/federation/Settings.ts (1)

40-44: Ensure deterministic uniqueId source to avoid token drift if invoked early

If settings.get('uniqueID') is unset on a fresh instance, two concurrent initializations could derive different ephemeral IDs (and thus different tokens) before persistence. After fixing the duplicate invocation in index.ts, this becomes unlikely, but consider reading uniqueID only after the core settings bootstrap completes or persisting the generated value before computing tokens.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 3f9f00c and 7d75249.

📒 Files selected for processing (4)
  • apps/meteor/server/services/federation/Settings.ts (1 hunks)
  • apps/meteor/server/settings/federation-service.ts (1 hunks)
  • apps/meteor/server/settings/federation.ts (4 hunks)
  • apps/meteor/server/settings/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/meteor/server/settings/federation.ts
  • apps/meteor/server/settings/federation-service.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.613Z
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.613Z
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.613Z
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/services/federation/Settings.ts
  • apps/meteor/server/settings/index.ts
🧬 Code graph analysis (2)
apps/meteor/server/services/federation/Settings.ts (1)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-52)
apps/meteor/server/settings/index.ts (2)
apps/meteor/server/services/federation/Settings.ts (1)
  • createFederationServiceSettings (8-159)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-52)
⏰ 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: 📦 Build Packages

Comment on lines +55 to +70
await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
readonly: true,
type: 'string',
i18nLabel: 'Federation_Matrix_hs_token',
i18nDescription: 'Federation_Matrix_hs_token_desc',
group: 'Federation',
section: 'Matrix Bridge',
});

await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
readonly: true,
type: 'string',
i18nLabel: 'Federation_Matrix_as_token',
i18nDescription: 'Federation_Matrix_as_token_desc',
group: 'Federation',
section: 'Matrix Bridge',
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Treat Matrix tokens as secrets (use password type)

Federation_Matrix_hs_token and Federation_Matrix_as_token are shared-secret tokens. They should be stored as password to avoid accidental disclosure in the admin UI and logs.

 await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
-  readonly: true,
-  type: 'string',
+  readonly: true,
+  type: 'password',
@@
 });
 
 await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
-  readonly: true,
-  type: 'string',
+  readonly: true,
+  type: 'password',
@@
 });
📝 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.

Suggested change
await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
readonly: true,
type: 'string',
i18nLabel: 'Federation_Matrix_hs_token',
i18nDescription: 'Federation_Matrix_hs_token_desc',
group: 'Federation',
section: 'Matrix Bridge',
});
await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
readonly: true,
type: 'string',
i18nLabel: 'Federation_Matrix_as_token',
i18nDescription: 'Federation_Matrix_as_token_desc',
group: 'Federation',
section: 'Matrix Bridge',
});
await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
readonly: true,
type: 'password',
i18nLabel: 'Federation_Matrix_hs_token',
i18nDescription: 'Federation_Matrix_hs_token_desc',
group: 'Federation',
section: 'Matrix Bridge',
});
await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
readonly: true,
type: 'password',
i18nLabel: 'Federation_Matrix_as_token',
i18nDescription: 'Federation_Matrix_as_token_desc',
group: 'Federation',
section: 'Matrix Bridge',
});
🤖 Prompt for AI Agents
In apps/meteor/server/services/federation/Settings.ts around lines 55 to 71 the
two Matrix tokens are registered with type 'string' which exposes them in the
admin UI; change both setting registrations (Federation_Matrix_hs_token and
Federation_Matrix_as_token) to use type: 'password' (keep readonly, i18n keys,
group and section as-is) so the values are treated as secrets and hidden in the
UI/logs; review any code that renders these settings to ensure it does not
inadvertently log or reveal the secret values.

@ggazzo ggazzo force-pushed the chore/federation-backup branch from e003574 to cb6102c Compare September 19, 2025 18:09
@ggazzo ggazzo force-pushed the feat/federation-settings branch from 7d75249 to bbe8d42 Compare September 19, 2025 18:31
@ggazzo ggazzo added the stat: QA assured Means it has been tested and approved by a company insider label Sep 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)

91-91: Bug: enableThumbnails is always true.

The || true short‑circuits any env and forces thumbnails on.

-				enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true,
+				// default true if unset; allow explicit "false"
+				enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS
+					? process.env.MEDIA_ENABLE_THUMBNAILS === 'true'
+					: true,
♻️ Duplicate comments (3)
apps/meteor/server/services/federation/Settings.ts (2)

8-8: Rename initializer to avoid export collision and double invocation

This name collides with apps/meteor/server/settings/federation-service.ts and has been called twice from index.ts in prior commits. Rename to an intention‑revealing symbol and update call sites to ensure each initializer runs exactly once.

Apply:

-export const createFederationServiceSettings = async (): Promise<void> => {
+export const createFederationMatrixBridgeSettings = async (): Promise<void> => {

Run to verify no collisions and exactly one invocation remains:

#!/usr/bin/env bash
set -euo pipefail
echo "== Exports named createFederationServiceSettings =="
rg -nP 'export\s+(const|function)\s+createFederationServiceSettings\b' -C2

echo
echo "== Call sites of createFederationServiceSettings =="
rg -nP '\bcreateFederationServiceSettings\s*\(' -C2

echo
echo "== Call sites of createFederationMatrixBridgeSettings (after rename) =="
rg -nP '\bcreateFederationMatrixBridgeSettings\s*\(' -C2 || true

55-63: Treat Matrix tokens as secrets (password type, not string)

These are shared‑secret tokens; rendering them as strings risks disclosure in the admin UI and logs.

Apply:

 await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
   readonly: true,
-  type: 'string',
+  type: 'password',
   i18nLabel: 'Federation_Matrix_hs_token',
   i18nDescription: 'Federation_Matrix_hs_token_desc',
   group: 'Federation',
   section: 'Matrix Bridge',
 });

 await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
   readonly: true,
-  type: 'string',
+  type: 'password',
   i18nLabel: 'Federation_Matrix_as_token',
   i18nDescription: 'Federation_Matrix_as_token_desc',
   group: 'Federation',
   section: 'Matrix Bridge',
 });

Also applies to: 64-71

ee/packages/federation-matrix/src/FederationMatrix.ts (1)

50-53: Harden signing key construction: defaults, empty‑key guard, and safe legacy prefix normalization.

Unvalidated settings can yield "undefined undefined undefined" or crash later. Also, admins may paste a full "alg ver key" which should be normalized. Add defaults, trim, and strip only a leading " " prefix.

-		const settingsSigningAlg = await Settings.get<string>('Federation_Service_Matrix_Signing_Algorithm');
-		const settingsSigningVersion = await Settings.get<string>('Federation_Service_Matrix_Signing_Version');
-		const settingsSigningKey = await Settings.get<string>('Federation_Service_Matrix_Signing_Key');
+		const settingsSigningAlg = await Settings.get<string>('Federation_Service_Matrix_Signing_Algorithm');
+		const settingsSigningVersion = await Settings.get<string>('Federation_Service_Matrix_Signing_Version');
+		const settingsSigningKey = await Settings.get<string>('Federation_Service_Matrix_Signing_Key');
+		const alg = (settingsSigningAlg || 'ed25519').trim();
+		const ver = (settingsSigningVersion || '0').trim();
+		const key = (settingsSigningKey || '').trim();
+		if (!key) {
+			throw new Error('Federation signing key is not configured');
+		}
+		// If admin pasted "alg ver key", drop only the leading prefix
+		const normalizedKey = key.replace(new RegExp(`^\\s*${alg}\\s+${ver}\\s+`), '');
@@
-			signingKey: `${settingsSigningAlg} ${settingsSigningVersion} ${settingsSigningKey}`,
+			signingKey: `${alg} ${ver} ${normalizedKey}`,

Also applies to: 71-71

🧹 Nitpick comments (8)
apps/meteor/server/services/federation/Settings.ts (3)

40-40: Preserve full UUID entropy and simplify replace

Truncation reduces uniqueness unnecessarily. Keep the full UUID and use a regex literal.

-const uniqueId = settings.get('uniqueID') || uuidv4().slice(0, 15).replace(new RegExp('-', 'g'), '_');
+const uniqueId = settings.get('uniqueID') ?? uuidv4().replace(/-/g, '_');

73-81: Confirm http defaults; prefer https for production defaults

If these defaults can surface in production, consider defaulting to https or deriving scheme from Site_Url. If dev‑only, add that note to the i18n alert.

Also applies to: 93-101


111-119: Registration file is readonly—intended?

If admins need to paste/update the Matrix registration YAML, consider making it editable (or provide a regenerate/upload action). If it’s intentionally locked for Alpha, ignore.

packages/i18n/src/locales/en.i18n.json (1)

2063-2066: Copy tweak: “no longer supported” phrasing

Improve phrasing and consistency for user-facing alerts.

-  "Old_Federation_Alert": "This Federation version is not supported anymore. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More Information about Matrix Federation support can be found here</a>",
+  "Old_Federation_Alert": "This Federation version is no longer supported. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More information about Matrix Federation support can be found here</a>",
ee/packages/federation-matrix/src/FederationMatrix.ts (1)

240-241: Avoid console.log in server code; use the logger only.

Duplicate logging and potential PII leakage.

-			console.log(error);
 			this.logger.error('Failed to create room:', error);
apps/meteor/server/settings/federation-service.ts (3)

14-23: Matrix signing algorithm select: add gating and i18n polish.

Looks good. Consider showing it only when federation is enabled to reduce noise; also add i18nLabel/i18nDescription for clarity.

 		await this.add('Federation_Service_Matrix_Signing_Algorithm', 'ed25519', {
 			type: 'select',
 			public: false,
 			values: [
 				{ key: 'ed25519', i18nLabel: 'ed25519' },
 			],
+			enableQuery: { _id: 'Federation_Service_Enabled', value: true },
+			i18nLabel: 'Federation_Service_Matrix_Signing_Algorithm',
+			i18nDescription: 'Federation_Service_Matrix_Signing_Algorithm_Description',
 			enterprise: true,
 			modules: ['federation'],
 			invalidValue: 'ed25519',
 		});

25-33: Signing version: readonly is fine; gate visibility behind enable.

No functional issue; optionally hide unless enabled for UI cleanliness.

 		await this.add('Federation_Service_Matrix_Signing_Version', '0', {
 			type: 'string',
 			public: false,
 			readonly: true,
+			enableQuery: { _id: 'Federation_Service_Enabled', value: true },
 			enterprise: true,
 			modules: ['federation'],
 			invalidValue: '0',
 		});

34-41: Signing key (password): add enable gating and ensure secret handling.

Type=password should already mask it; still, gate it behind the enable flag.

 		await this.add('Federation_Service_Matrix_Signing_Key', '', {
 			type: 'password',
 			public: false,
+			enableQuery: { _id: 'Federation_Service_Enabled', value: true },
 			enterprise: true,
 			modules: ['federation'],
 			invalidValue: '',
 		});
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7d75249 and bbe8d42.

📒 Files selected for processing (31)
  • apps/meteor/server/services/federation/Settings.ts (1 hunks)
  • apps/meteor/server/settings/federation-service.ts (1 hunks)
  • apps/meteor/server/settings/federation.ts (4 hunks)
  • apps/meteor/server/settings/index.ts (1 hunks)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (2 hunks)
  • packages/i18n/src/locales/ar.i18n.json (0 hunks)
  • packages/i18n/src/locales/ca.i18n.json (0 hunks)
  • packages/i18n/src/locales/cs.i18n.json (0 hunks)
  • packages/i18n/src/locales/da.i18n.json (0 hunks)
  • packages/i18n/src/locales/de-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/de.i18n.json (0 hunks)
  • packages/i18n/src/locales/en.i18n.json (3 hunks)
  • packages/i18n/src/locales/es.i18n.json (0 hunks)
  • packages/i18n/src/locales/fi.i18n.json (0 hunks)
  • packages/i18n/src/locales/fr.i18n.json (0 hunks)
  • packages/i18n/src/locales/hi-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/hu.i18n.json (0 hunks)
  • packages/i18n/src/locales/ja.i18n.json (0 hunks)
  • packages/i18n/src/locales/ka-GE.i18n.json (0 hunks)
  • packages/i18n/src/locales/ko.i18n.json (0 hunks)
  • packages/i18n/src/locales/nb.i18n.json (0 hunks)
  • packages/i18n/src/locales/nl.i18n.json (0 hunks)
  • packages/i18n/src/locales/nn.i18n.json (0 hunks)
  • packages/i18n/src/locales/pl.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt-BR.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt.i18n.json (0 hunks)
  • packages/i18n/src/locales/ru.i18n.json (0 hunks)
  • packages/i18n/src/locales/sv.i18n.json (0 hunks)
  • packages/i18n/src/locales/uk.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh-TW.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh.i18n.json (0 hunks)
💤 Files with no reviewable changes (25)
  • packages/i18n/src/locales/ca.i18n.json
  • packages/i18n/src/locales/zh.i18n.json
  • packages/i18n/src/locales/pt.i18n.json
  • packages/i18n/src/locales/ko.i18n.json
  • packages/i18n/src/locales/ja.i18n.json
  • packages/i18n/src/locales/uk.i18n.json
  • packages/i18n/src/locales/cs.i18n.json
  • packages/i18n/src/locales/sv.i18n.json
  • packages/i18n/src/locales/fr.i18n.json
  • packages/i18n/src/locales/nb.i18n.json
  • packages/i18n/src/locales/nn.i18n.json
  • packages/i18n/src/locales/es.i18n.json
  • packages/i18n/src/locales/pt-BR.i18n.json
  • packages/i18n/src/locales/da.i18n.json
  • packages/i18n/src/locales/de-IN.i18n.json
  • packages/i18n/src/locales/ka-GE.i18n.json
  • packages/i18n/src/locales/hi-IN.i18n.json
  • packages/i18n/src/locales/pl.i18n.json
  • packages/i18n/src/locales/nl.i18n.json
  • packages/i18n/src/locales/zh-TW.i18n.json
  • packages/i18n/src/locales/de.i18n.json
  • packages/i18n/src/locales/hu.i18n.json
  • packages/i18n/src/locales/fi.i18n.json
  • packages/i18n/src/locales/ar.i18n.json
  • packages/i18n/src/locales/ru.i18n.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/meteor/server/settings/federation.ts
  • apps/meteor/server/settings/index.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.613Z
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.613Z
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.613Z
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/services/federation/Settings.ts
  • apps/meteor/server/settings/federation-service.ts
  • ee/packages/federation-matrix/src/FederationMatrix.ts
  • packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (2)
apps/meteor/server/services/federation/Settings.ts (1)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-52)
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)
🔇 Additional comments (4)
apps/meteor/server/services/federation/Settings.ts (2)

83-91: No changes requested: homeserver_domain is legacy/transitioned

Acknowledging team guidance: Federation_Matrix_homeserver_domain ties to legacy behavior and is being deprecated/removed; not flagging improvements here.


135-158: LGTM: hidden config status and check action

Good separation of UX concerns; hidden status/action avoids clutter while enabling diagnostics.

packages/i18n/src/locales/en.i18n.json (2)

2107-2107: LGTM: updated description is accurate and concise

The Matrix protocol reference reads well.


2157-2163: Fix typos and tighten wording in Federation service texts

-  "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+  "Federation_Service_Alert": "<strong>This is an alpha feature not intended for production use.</strong><br/>It may be unstable or not fully performant. It may change, break, or even be removed in the future without notice.",
-  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
+  "Federation_Service_Matrix_Signing_Key_Description": "The private Base64-encoded signing key used to authenticate federation requests. This is typically an Ed25519 key (version 4). It is required for secure communication between federated servers over the Matrix protocol and should be kept confidential.",
-  "Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room in a remote server",
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room on a remote server",
-  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind that the larger the room users can join, the longer the join will take and the more resources it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
  • Consider aligning key-name patterns between Matrix and Service settings (or add aliases) to reduce cognitive load.

Location: packages/i18n/src/locales/en.i18n.json (around lines 2157–2163).

Verification incomplete: ripgrep in the provided script returned a PCRE error and "No files were searched"; cannot confirm these keys are wired in the settings/UI — verify references in the repo.

const homeserverToken = crypto.createHash('sha256').update(`hs_${uniqueId}`).digest('hex');
const applicationServiceToken = crypto.createHash('sha256').update(`as_${uniqueId}`).digest('hex');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Generate cryptographically random tokens; don’t derive from uniqueId

Hashing a truncated/guessable identifier does not provide secrecy. Use crypto‑strong randomness so tokens are non‑derivable and rotation‑friendly.

Apply:

-const homeserverToken = crypto.createHash('sha256').update(`hs_${uniqueId}`).digest('hex');
-const applicationServiceToken = crypto.createHash('sha256').update(`as_${uniqueId}`).digest('hex');
+const homeserverToken = crypto.randomBytes(32).toString('hex');
+const applicationServiceToken = crypto.randomBytes(32).toString('hex');

Note: settingsRegistry.add should preserve existing values; random generation will be used only on first initialization.

📝 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.

Suggested change
const homeserverToken = crypto.createHash('sha256').update(`hs_${uniqueId}`).digest('hex');
const applicationServiceToken = crypto.createHash('sha256').update(`as_${uniqueId}`).digest('hex');
const homeserverToken = crypto.randomBytes(32).toString('hex');
const applicationServiceToken = crypto.randomBytes(32).toString('hex');
🤖 Prompt for AI Agents
In apps/meteor/server/services/federation/Settings.ts around lines 41-42,
replace the current tokens derived by hashing a predictable uniqueId with
cryptographically secure random tokens: generate homeserverToken and
applicationServiceToken using crypto.randomBytes (e.g., 32 bytes) and hex-encode
them so they are non-derivable and rotation-friendly; keep settingsRegistry.add
semantics so existing stored tokens are preserved and only generate and set
these random values when the settings are absent on first initialization.

Comment on lines +43 to 48
await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
type: 'int',
public: false,
alert: 'Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert',
enterprise: true,
modules: ['federation'],
invalidValue: false,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Type mismatch: invalidValue should be a number, not boolean. Add bounds.

invalidValue: false for an int setting is inconsistent and can cause coercion issues. Use a numeric invalidValue and bound the range.

-		await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
+		await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
 			type: 'int',
 			public: false,
 			alert: 'Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert',
 			enterprise: true,
 			modules: ['federation'],
-			invalidValue: false,
+			invalidValue: 100,
+			min: 1,
+			max: 100000, // adjust if you have a known upper bound
+			enableQuery: { _id: 'Federation_Service_Enabled', value: true },
 		});
📝 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.

Suggested change
await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
type: 'int',
public: false,
alert: 'Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert',
enterprise: true,
modules: ['federation'],
invalidValue: false,
});
await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
type: 'int',
public: false,
alert: 'Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert',
enterprise: true,
modules: ['federation'],
invalidValue: 100,
min: 1,
max: 100000, // adjust if you have a known upper bound
enableQuery: { _id: 'Federation_Service_Enabled', value: true },
});
🤖 Prompt for AI Agents
In apps/meteor/server/settings/federation-service.ts around lines 43 to 50, the
setting uses invalidValue: false for an int which is a type mismatch; change
invalidValue to a numeric value (e.g., 0) and add explicit numeric bounds (min
and max) to the setting definition (for example add min: 0 and max: 10000) so
the setting is validated and coercion issues are prevented.

@ggazzo ggazzo force-pushed the feat/federation-settings branch from bbe8d42 to d7c6531 Compare September 19, 2025 19:29
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (4)
apps/meteor/server/services/federation/Settings.ts (3)

7-7: Rename to avoid export-name collision with legacy initializer

This function name conflicts with the same-named export in apps/meteor/server/settings/federation-service.ts. Both functions are imported and called in the settings initialization sequence, leading to naming confusion and potential maintenance issues.


40-41: Generate cryptographically random tokens; don't derive from uniqueId

Hashing a truncated/guessable identifier does not provide secrecy for Matrix bridge tokens. Application service tokens (as_token and hs_token) are used for authentication between the Matrix homeserver and the application service, where the homeserver must verify the provided access_token matches their known hs_token.


54-70: Treat Matrix tokens as secrets (use password type)

The access_token should be supplied through the Authorization header where possible to prevent the token appearing in HTTP request logs by accident. Matrix bridge tokens should be stored as password type to avoid accidental disclosure in the admin UI.

apps/meteor/server/settings/federation-service.ts (1)

41-48: Type mismatch: invalidValue should be a number, not boolean. Add bounds.

The setting uses invalidValue: false for an int type, which creates type inconsistency and potential coercion issues.

🧹 Nitpick comments (2)
apps/meteor/server/services/federation/Settings.ts (2)

82-90: Avoid setting federation domain from Site_Url without validation

Using siteUrl directly for the Matrix homeserver domain assumes Site_Url is properly configured as a valid domain. This could lead to federation connectivity issues if Site_Url contains URL schemes, ports, or paths.

Based on the retrieved learnings, the old federation system is being deprecated, so this specific implementation may not require fixing.


120-132: Type mismatch: invalidValue should be a number, not boolean

For an int type setting, invalidValue: false creates type inconsistency and could cause validation issues.

Apply this diff to fix the type mismatch:

-    invalidValue: false,
+    invalidValue: 100,
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between bbe8d42 and d7c6531.

📒 Files selected for processing (3)
  • apps/meteor/server/services/federation/Settings.ts (1 hunks)
  • apps/meteor/server/settings/federation-service.ts (1 hunks)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • 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.613Z
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.613Z
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.613Z
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.ts
  • apps/meteor/server/services/federation/Settings.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/federation/Settings.ts (1)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-50)
🔇 Additional comments (4)
apps/meteor/server/settings/federation-service.ts (4)

4-4: LGTM: Settings group renamed appropriately

The change from "Federation Service" to "Federation" aligns with the PR objectives to consolidate federation settings organization.


14-21: LGTM: Matrix signing algorithm setting properly configured

The select type with restricted values and proper enterprise/modules configuration is appropriate for cryptographic algorithm selection.


23-30: LGTM: Version setting appropriately readonly

The readonly property prevents unintended modification of the signing version, which is correct for this Matrix specification parameter.


33-39: LGTM: Signing key properly secured

Using password type for the signing key ensures the cryptographic material is protected from accidental disclosure in the admin interface.

@rodrigok rodrigok force-pushed the feat/federation-settings branch from d7c6531 to a7f4dcd Compare September 19, 2025 23:37
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
apps/meteor/server/services/federation/Settings.ts (3)

7-7: Rename export to avoid collision with apps/meteor/server/settings/federation-service.ts and prevent double invocation on startup

There’s another export with the same name. This can cause ambiguous imports and, as observed, duplicate initialization calls. Rename to a distinct symbol.

-export const createFederationServiceSettings = async (): Promise<void> => {
+export const createFederationMatrixSettings = async (): Promise<void> => {

Also update its caller import in apps/meteor/server/settings/index.ts to use createFederationMatrixSettings so each initializer is invoked exactly once.

Run to verify duplicate exports/usages:

#!/bin/bash
set -euo pipefail
echo "=== Conflicting exports ==="
rg -n --hidden -S 'export\s+(const|function)\s+createFederationServiceSettings\b' -g '!node_modules' -C2
echo
echo "=== All call sites ==="
rg -n --hidden -S '\bcreateFederationServiceSettings\s*\(' -g '!node_modules' -C2

39-41: Use cryptographically strong, non-derivable tokens for hs/as secrets

Tokens derived from a predictable identifier are guessable. Generate securely.

-const homeserverToken = crypto.createHash('sha256').update(`hs_${uniqueId}`).digest('hex');
-const applicationServiceToken = crypto.createHash('sha256').update(`as_${uniqueId}`).digest('hex');
+const homeserverToken = crypto.randomBytes(32).toString('hex');
+const applicationServiceToken = crypto.randomBytes(32).toString('hex');

Note: settingsRegistry.add should only apply defaults when absent, so randomness is used on first init only.


54-70: Store Matrix tokens as password type to avoid accidental disclosure

These are secrets; render as password in admin UI/logs.

 await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
   readonly: true,
-  type: 'string',
+  type: 'password',
@@
 });
 
 await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
   readonly: true,
-  type: 'string',
+  type: 'password',
@@
 });
apps/meteor/server/settings/federation-service.ts (1)

41-47: Type mismatch for int setting: invalidValue must be numeric; add bounds and enableQuery.
This prevents coercion bugs and aligns with validation.

 await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
   type: 'int',
   public: false,
   alert: 'Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert',
   enterprise: true,
   modules: ['federation'],
-  invalidValue: false,
+  invalidValue: 100,
+  min: 1,
+  max: 100000, // adjust if you know a safer upper bound
+  enableQuery: { _id: 'Federation_Service_Enabled', value: true },
 });
🧹 Nitpick comments (8)
packages/i18n/src/locales/en.i18n.json (4)

2063-2066: Copy edits: “no longer supported” phrasing and capitalization

Improve clarity/consistency and sentence case for the link text.

Apply this diff:

-  "Old_Federation_Alert": "This Federation version is not supported anymore. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More Information about Matrix Federation support can be found here</a>",
+  "Old_Federation_Alert": "This federation version is no longer supported. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More information about Matrix Federation support can be found here</a>",
   "Rocket.Chat Federation": "Rocket.Chat Federation (NOT SUPPORTED)",
   "Matrix Bridge": "Matrix Bridge (NOT SUPPORTED)",

2156-2156: Fix typos/wording in alpha warning

Use “alpha”, “use”, and “performant”; tighten wording.

Apply this diff:

-  "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+  "Federation_Service_Alert": "<strong>This is an alpha feature not intended for production use.</strong><br/>It may not be stable or performant. It may change, break, or be removed in the future without notice.",

2158-2161: Tighten signing key labels and description

Minor phrasing improvements; clarify “base64‑encoded” and protocol preposition.

Apply this diff:

-  "Federation_Service_Matrix_Signing_Algorithm": "Signing Key Algorithm",
-  "Federation_Service_Matrix_Signing_Version": "Signing Key Version",
-  "Federation_Service_Matrix_Signing_Key": "Signing Key",
-  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
+  "Federation_Service_Matrix_Signing_Algorithm": "Signing key algorithm",
+  "Federation_Service_Matrix_Signing_Version": "Signing key version",
+  "Federation_Service_Matrix_Signing_Key": "Signing key",
+  "Federation_Service_Matrix_Signing_Key_Description": "The private signing key (base64‑encoded) used to authenticate federation requests. This is typically an Ed25519 key (version 4). It is essential for secure communication between federated servers over the Matrix protocol and must be kept confidential.",

2162-2164: Grammar/style for room size setting and alert

Improve readability: article “a”, remove comma splice, pluralize “resources”.

Apply this diff:

-  "Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room in a remote server",
-  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
-  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them",
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room on a remote server",
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind that the larger the room you allow users to join, the longer joining will take and the more resources it will consume. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit for a public room on a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them",
apps/meteor/server/services/federation/Settings.ts (1)

8-157: Optional: Register within addGroup/section for cohesion and less repetition

Wrapping these adds under settingsRegistry.addGroup('Federation') and a 'Matrix Bridge' section will de-duplicate group/section metadata and align with federation-service.ts style.

apps/meteor/server/settings/federation-service.ts (3)

14-21: Algorithm select OK; consider locking it or at least gating behind the main toggle.
If admins shouldn’t change the algorithm in Alpha, make it readonly; regardless, gate it behind the enable toggle.

Apply if desired:

 await this.add('Federation_Service_Matrix_Signing_Algorithm', 'ed25519', {
   type: 'select',
   public: false,
   values: [{ key: 'ed25519', i18nLabel: 'ed25519' }],
   enterprise: true,
   modules: ['federation'],
+  enableQuery: { _id: 'Federation_Service_Enabled', value: true },
   invalidValue: 'ed25519',
+  // readonly: true, // uncomment if admins must not change in UI
 });

23-30: Guard the version field behind the enable toggle.
It’s readonly (good); also add enableQuery so it’s disabled when federation is off. Confirm default '0' matches how key IDs are composed (e.g., ed25519:).

 await this.add('Federation_Service_Matrix_Signing_Version', '0', {
   type: 'string',
   public: false,
   readonly: true,
   enterprise: true,
   modules: ['federation'],
+  enableQuery: { _id: 'Federation_Service_Enabled', value: true },
   invalidValue: '0',
 });

32-39: Signing key: add enableQuery so it’s only editable when federation is enabled.

 await this.add('Federation_Service_Matrix_Signing_Key', '', {
   type: 'password',
   public: false,
   enterprise: true,
   modules: ['federation'],
+  enableQuery: { _id: 'Federation_Service_Enabled', value: true },
   invalidValue: '',
 });
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d7c6531 and a7f4dcd.

📒 Files selected for processing (31)
  • apps/meteor/server/services/federation/Settings.ts (1 hunks)
  • apps/meteor/server/settings/federation-service.ts (1 hunks)
  • apps/meteor/server/settings/federation.ts (4 hunks)
  • apps/meteor/server/settings/index.ts (1 hunks)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (2 hunks)
  • packages/i18n/src/locales/ar.i18n.json (0 hunks)
  • packages/i18n/src/locales/ca.i18n.json (0 hunks)
  • packages/i18n/src/locales/cs.i18n.json (0 hunks)
  • packages/i18n/src/locales/da.i18n.json (0 hunks)
  • packages/i18n/src/locales/de-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/de.i18n.json (0 hunks)
  • packages/i18n/src/locales/en.i18n.json (3 hunks)
  • packages/i18n/src/locales/es.i18n.json (0 hunks)
  • packages/i18n/src/locales/fi.i18n.json (0 hunks)
  • packages/i18n/src/locales/fr.i18n.json (0 hunks)
  • packages/i18n/src/locales/hi-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/hu.i18n.json (0 hunks)
  • packages/i18n/src/locales/ja.i18n.json (0 hunks)
  • packages/i18n/src/locales/ka-GE.i18n.json (0 hunks)
  • packages/i18n/src/locales/ko.i18n.json (0 hunks)
  • packages/i18n/src/locales/nb.i18n.json (0 hunks)
  • packages/i18n/src/locales/nl.i18n.json (0 hunks)
  • packages/i18n/src/locales/nn.i18n.json (0 hunks)
  • packages/i18n/src/locales/pl.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt-BR.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt.i18n.json (0 hunks)
  • packages/i18n/src/locales/ru.i18n.json (0 hunks)
  • packages/i18n/src/locales/sv.i18n.json (0 hunks)
  • packages/i18n/src/locales/uk.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh-TW.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh.i18n.json (0 hunks)
💤 Files with no reviewable changes (25)
  • packages/i18n/src/locales/ko.i18n.json
  • packages/i18n/src/locales/pt-BR.i18n.json
  • packages/i18n/src/locales/fr.i18n.json
  • packages/i18n/src/locales/zh-TW.i18n.json
  • packages/i18n/src/locales/ja.i18n.json
  • packages/i18n/src/locales/es.i18n.json
  • packages/i18n/src/locales/nl.i18n.json
  • packages/i18n/src/locales/nn.i18n.json
  • packages/i18n/src/locales/fi.i18n.json
  • packages/i18n/src/locales/pt.i18n.json
  • packages/i18n/src/locales/sv.i18n.json
  • packages/i18n/src/locales/ru.i18n.json
  • packages/i18n/src/locales/zh.i18n.json
  • packages/i18n/src/locales/pl.i18n.json
  • packages/i18n/src/locales/ka-GE.i18n.json
  • packages/i18n/src/locales/de-IN.i18n.json
  • packages/i18n/src/locales/ca.i18n.json
  • packages/i18n/src/locales/cs.i18n.json
  • packages/i18n/src/locales/hu.i18n.json
  • packages/i18n/src/locales/nb.i18n.json
  • packages/i18n/src/locales/de.i18n.json
  • packages/i18n/src/locales/ar.i18n.json
  • packages/i18n/src/locales/uk.i18n.json
  • packages/i18n/src/locales/da.i18n.json
  • packages/i18n/src/locales/hi-IN.i18n.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • ee/packages/federation-matrix/src/FederationMatrix.ts
  • apps/meteor/server/settings/index.ts
  • apps/meteor/server/settings/federation.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.613Z
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.613Z
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.613Z
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.ts
  • packages/i18n/src/locales/en.i18n.json
  • apps/meteor/server/services/federation/Settings.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/federation/Settings.ts (1)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-57)
⏰ 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 (2)
packages/i18n/src/locales/en.i18n.json (1)

2107-2107: LGTM: updated description

“Federation allows remote workspaces to communicate with each other through the Matrix protocol.” reads well and matches the new model.

apps/meteor/server/settings/federation-service.ts (1)

8-10: EE gating and module scoping on the toggle are appropriate.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
apps/meteor/server/settings/index.ts (1)

56-57: Track deprecation removal via issue/label.

Add a TODO with a link/ID to remove on 8.0 so it isn’t forgotten.

-  createFederationSettings(), // Deprecated and not used anymore. Kept for admin UI information purposes. Remove on 8.0
+  createFederationSettings(), // Deprecated; kept for admin UI. REMOVE in 8.0 — see ISSUE: FDR-133 follow-up
-  addMatrixBridgeFederationSettings(), // Deprecated and not used anymore. Kept for admin UI information purposes. Remove on 8.0
+  addMatrixBridgeFederationSettings(), // Deprecated; kept for admin UI. REMOVE in 8.0 — see ISSUE: FDR-133 follow-up
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a7f4dcd and 1c64a24.

📒 Files selected for processing (4)
  • .gitignore (1 hunks)
  • apps/meteor/app/settings/server/SettingsRegistry.ts (1 hunks)
  • apps/meteor/server/services/federation/Settings.ts (1 hunks)
  • apps/meteor/server/settings/index.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .gitignore
🧰 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.613Z
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.613Z
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.613Z
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/index.ts
  • apps/meteor/server/services/federation/Settings.ts
🧬 Code graph analysis (1)
apps/meteor/server/settings/index.ts (3)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-57)
apps/meteor/server/settings/federation.ts (1)
  • createFederationSettings (5-70)
apps/meteor/server/services/federation/Settings.ts (1)
  • addMatrixBridgeFederationSettings (7-158)
⏰ 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)
apps/meteor/server/settings/index.ts (2)

1-1: Distinct Matrix bridge initializer import looks good.


43-43: Single invocation of createFederationServiceSettings() is correct — approved.

Prevents duplicate registration; only the export and a single invocation at apps/meteor/server/settings/index.ts:43 were found.

apps/meteor/server/services/federation/Settings.ts (5)

7-17: LGTM: readonly “enabled” flag with deprecation alert.


110-118: LGTM: registration file as code and readonly is appropriate.


120-132: Confirm intent: readonly + public + enterprise for max size limit.

If admin control is planned post‑alpha, consider dropping readonly later; otherwise this is fine for a placeholder.


39-41: Generate tokens using CSPRNG, not a hash of a predictable ID.

Current derivation is guessable. Use crypto.randomBytes.

- const homeserverToken = crypto.createHash('sha256').update(`hs_${uniqueId}`).digest('hex');
- const applicationServiceToken = crypto.createHash('sha256').update(`as_${uniqueId}`).digest('hex');
+ const homeserverToken = crypto.randomBytes(32).toString('hex');
+ const applicationServiceToken = crypto.randomBytes(32).toString('hex');

54-70: Store secrets as password to avoid UI/log exposure.

Tokens are shared secrets; mask them.

 await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
-  readonly: true,
-  type: 'string',
+  readonly: true,
+  type: 'password',
@@
 });
 
 await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
-  readonly: true,
-  type: 'string',
+  readonly: true,
+  type: 'password',
@@
 });

Comment on lines 224 to 228
if (!this.store.has(_id)) {
options.ts = new Date();
await this.model.insertOne(options as ISetting);
this.store.set(options as ISetting);
await this.model.insertOne(options as ISetting);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid in-memory/DB divergence; write to DB first (idempotent), then cache.

Setting the store before persisting risks stale in‑memory state if insert fails, and diverges from add()’s order. Also, concurrent startups can race on insertOne. Use an upsert, then set the store.

-    if (!this.store.has(_id)) {
-      options.ts = new Date();
-      this.store.set(options as ISetting);
-      await this.model.insertOne(options as ISetting);
-    }
+    if (!this.store.has(_id)) {
+      options.ts = new Date();
+      await this.model.updateOne(
+        { _id },
+        { $setOnInsert: options as ISetting },
+        { upsert: true },
+      );
+      this.store.set(options as ISetting);
+    }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/meteor/server/settings/index.ts (1)

42-44: Order clarity: register the new Federation Service settings before the big Promise.all.
This ensures deterministic ordering for the 'Federation' group/sections in the admin UI and avoids any subtle interleaving with other group registrations.

Apply optionally:

-await Promise.all([
-	createFederationServiceSettings(),
+await createFederationServiceSettings();
+
+await Promise.all([
 	createAccountSettings(),

Please confirm UI section order remains as intended after this change (especially with “NOT SUPPORTED” banners).

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 986387f and 5a32c76.

📒 Files selected for processing (1)
  • apps/meteor/server/settings/index.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.613Z
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.613Z
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.613Z
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/index.ts
🧬 Code graph analysis (1)
apps/meteor/server/settings/index.ts (3)
apps/meteor/server/settings/federation-service.ts (1)
  • createFederationServiceSettings (3-57)
apps/meteor/server/settings/federation.ts (1)
  • createFederationSettings (5-70)
apps/meteor/server/services/federation/Settings.ts (1)
  • addMatrixBridgeFederationSettings (7-158)
⏰ 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 (2)
apps/meteor/server/settings/index.ts (2)

40-40: Import is correct and aligns with the deprecation flow.
Looks good; path and named export match the new helper.


83-87: Verify Site_Url and uniqueID exist before creating federation UI settings.

  • Confirm Site_Url is registered before the Promise.all invoking createFederationSettings() and addMatrixBridgeFederationSettings() runs (apps/meteor/server/settings/index.ts, ~lines 83–87).
  • Confirm uniqueID is present or that the UI paths handle its absence with an acceptable fallback.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/i18n/src/locales/en.i18n.json (2)

2063-2066: Old/new federation grouping looks correct; minor copy check on “NOT SUPPORTED”.

If “NOT SUPPORTED” should follow sentence‑case UI copy elsewhere (“Not supported”), consider aligning casing; otherwise LGTM.


2152-2160: Fix typos and improve clarity in alpha warning and Base64 capitalization.

  • “alfa” → “alpha”; “performatic” → “performant”; simplify “and/or”.
  • Capitalize “Base64” consistently.

Apply this diff:

-  "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+  "Federation_Service_Alert": "<strong>This is an alpha feature, not intended for production use.</strong><br/>It may not be stable or performant. Please be aware that it may change, break, or even be removed in the future without notice.",
@@
-  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
+  "Federation_Service_Matrix_Signing_Key_Description": "The private Base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as Base64. It is essential for secure communication between federated servers over the Matrix protocol and should be kept confidential.",

Please also confirm these new keys are added (or intentionally omitted) in other locale files to avoid fallback to English only.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 5a32c76 and b659f0b.

📒 Files selected for processing (1)
  • packages/i18n/src/locales/en.i18n.json (3 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:

  • packages/i18n/src/locales/en.i18n.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/i18n/src/locales/en.i18n.json (1)

2107-2107: Updated federation description reads well.

Accurately reflects Matrix-based federation. No changes needed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
packages/i18n/src/locales/en.i18n.json (3)

2063-2066: Minor copy tweaks for consistency

  • "More Information" → "More information".
-  "Old_Federation_Alert": "This Federation version is not supported anymore. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More Information about Matrix Federation support can be found here</a>",
+  "Old_Federation_Alert": "This Federation version is not supported anymore. Please configure the new alternative above named Native Federation.<br/><a target=\"_blank\" href=\"https://go.rocket.chat/i/matrix-federation\">More information about Matrix Federation support can be found here</a>",

2152-2160: Fix typos/grammar in alerts and signing-key description

  • “alfa” → “alpha”; “performatic” → “performant”.
  • Minor clarity/grammar in signing key description.
-  "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+  "Federation_Service_Alert": "<strong>This is an alpha feature not intended for production usage!</strong><br/>It may not be stable and/or performant. Please be aware that it may change, break, or even be removed in the future without any notice.",

-  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
+  "Federation_Service_Matrix_Signing_Key_Description": "The private base64-encoded signing key used to authenticate federation requests. This is typically an Ed25519 key (version 4). It is essential for secure communication between federated servers over the Matrix protocol and should be kept confidential.",

2158-2160: Wording consistency with Matrix setting

Optional: mirror the phrase “public room on a remote server” used in 2137–2139 to keep wording consistent across old/new settings.

-  "Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room in a remote server",
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room on a remote server",
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b659f0b and 37d217e.

📒 Files selected for processing (3)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (2 hunks)
  • packages/i18n/src/locales/en.i18n.json (3 hunks)
  • packages/i18n/src/locales/pt-BR.i18n.json (0 hunks)
💤 Files with no reviewable changes (1)
  • packages/i18n/src/locales/pt-BR.i18n.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • 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:

  • packages/i18n/src/locales/en.i18n.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/i18n/src/locales/en.i18n.json (1)

2107-2107: LGTM: updated description aligns with Matrix-based federation

Wording is clear and accurate.

@ggazzo ggazzo force-pushed the feat/federation-settings branch from 37d217e to d7c6531 Compare September 22, 2025 14:19
@ggazzo ggazzo force-pushed the feat/federation-settings branch from d7c6531 to c056235 Compare September 22, 2025 14:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
apps/meteor/server/settings/federation.ts (1)

19-20: Hide + readonly is good; drop unnecessary client exposure.
FEDERATION_Discovery_Method is hidden but still public: true. Set to non‑public to avoid publishing dead settings.

 await this.add('FEDERATION_Discovery_Method', 'dns', {
   type: 'select',
   readonly: true,
   hidden: true,
@@
-  public: true,
+  public: false,
 });

Also applies to: 26-28, 38-39, 47-49, 66-66

apps/meteor/server/services/federation/Settings.ts (1)

8-17: Alert copy seems mismatched.
Federation_Matrix_enabled shows Old_Federation_Alert. If this toggle is for the new Matrix path, use the new “Alpha/Not supported yet” alert key.

packages/i18n/src/locales/en.i18n.json (1)

2063-2066: Legacy sections clearly marked as not supported — LGTM, with a tiny tone nit

The NOT SUPPORTED callouts will help reduce confusion. If you want a softer, more consistent tone with the rest of the UI, consider “(Not supported)” instead of all caps — optional.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 37d217e and c056235.

📒 Files selected for processing (32)
  • .gitignore (1 hunks)
  • apps/meteor/server/services/federation/Settings.ts (1 hunks)
  • apps/meteor/server/settings/federation-service.ts (1 hunks)
  • apps/meteor/server/settings/federation.ts (4 hunks)
  • apps/meteor/server/settings/index.ts (2 hunks)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (2 hunks)
  • packages/i18n/src/locales/ar.i18n.json (0 hunks)
  • packages/i18n/src/locales/ca.i18n.json (0 hunks)
  • packages/i18n/src/locales/cs.i18n.json (0 hunks)
  • packages/i18n/src/locales/da.i18n.json (0 hunks)
  • packages/i18n/src/locales/de-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/de.i18n.json (0 hunks)
  • packages/i18n/src/locales/en.i18n.json (3 hunks)
  • packages/i18n/src/locales/es.i18n.json (0 hunks)
  • packages/i18n/src/locales/fi.i18n.json (0 hunks)
  • packages/i18n/src/locales/fr.i18n.json (0 hunks)
  • packages/i18n/src/locales/hi-IN.i18n.json (0 hunks)
  • packages/i18n/src/locales/hu.i18n.json (0 hunks)
  • packages/i18n/src/locales/ja.i18n.json (0 hunks)
  • packages/i18n/src/locales/ka-GE.i18n.json (0 hunks)
  • packages/i18n/src/locales/ko.i18n.json (0 hunks)
  • packages/i18n/src/locales/nb.i18n.json (0 hunks)
  • packages/i18n/src/locales/nl.i18n.json (0 hunks)
  • packages/i18n/src/locales/nn.i18n.json (0 hunks)
  • packages/i18n/src/locales/pl.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt-BR.i18n.json (0 hunks)
  • packages/i18n/src/locales/pt.i18n.json (0 hunks)
  • packages/i18n/src/locales/ru.i18n.json (0 hunks)
  • packages/i18n/src/locales/sv.i18n.json (0 hunks)
  • packages/i18n/src/locales/uk.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh-TW.i18n.json (0 hunks)
  • packages/i18n/src/locales/zh.i18n.json (0 hunks)
💤 Files with no reviewable changes (25)
  • packages/i18n/src/locales/ka-GE.i18n.json
  • packages/i18n/src/locales/pt.i18n.json
  • packages/i18n/src/locales/nl.i18n.json
  • packages/i18n/src/locales/nn.i18n.json
  • packages/i18n/src/locales/ru.i18n.json
  • packages/i18n/src/locales/pt-BR.i18n.json
  • packages/i18n/src/locales/sv.i18n.json
  • packages/i18n/src/locales/ca.i18n.json
  • packages/i18n/src/locales/pl.i18n.json
  • packages/i18n/src/locales/hu.i18n.json
  • packages/i18n/src/locales/ko.i18n.json
  • packages/i18n/src/locales/uk.i18n.json
  • packages/i18n/src/locales/hi-IN.i18n.json
  • packages/i18n/src/locales/da.i18n.json
  • packages/i18n/src/locales/fr.i18n.json
  • packages/i18n/src/locales/zh.i18n.json
  • packages/i18n/src/locales/ja.i18n.json
  • packages/i18n/src/locales/fi.i18n.json
  • packages/i18n/src/locales/nb.i18n.json
  • packages/i18n/src/locales/de.i18n.json
  • packages/i18n/src/locales/cs.i18n.json
  • packages/i18n/src/locales/zh-TW.i18n.json
  • packages/i18n/src/locales/es.i18n.json
  • packages/i18n/src/locales/ar.i18n.json
  • packages/i18n/src/locales/de-IN.i18n.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • ee/packages/federation-matrix/src/FederationMatrix.ts
  • apps/meteor/server/settings/index.ts
  • .gitignore
🧰 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.ts
  • apps/meteor/server/settings/federation-service.ts
  • apps/meteor/server/services/federation/Settings.ts
  • packages/i18n/src/locales/en.i18n.json
⏰ 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 Homeserver
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/server/settings/federation.ts (1)

10-14: Old federation toggle made read-only with proper alert — LGTM.
Matches the PR intent to deprecate legacy controls.

apps/meteor/server/settings/federation-service.ts (2)

4-4: Ensure no duplicate group/initializer wiring.
We previously found duplicate ‘Federation’ group registrations and multiple invocations of the initializer in index.ts. Re‑verify after these moves.

#!/bin/bash
set -euo pipefail
echo "== addGroup('Federation') sites =="
rg -nP "addGroup\\(['\"]Federation['\"]" -C2

echo
echo "== Initializer invocations =="
rg -nP "\\bcreateFederationServiceSettings\\s*\\(" -n -C2
rg -nP "\\baddMatrixBridgeFederationSettings\\s*\\(" -n -C2

41-48: Type mismatch: invalidValue for int should be numeric; add bounds.
Same issue flagged earlier.

 await this.add('Federation_Service_max_allowed_size_of_public_rooms_to_join', 100, {
   type: 'int',
   public: false,
   alert: 'Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert',
   enterprise: true,
   modules: ['federation'],
-  invalidValue: false,
+  invalidValue: 100,
+  min: 1,
+  max: 100000,
 });
apps/meteor/server/services/federation/Settings.ts (2)

54-70: Treat Matrix tokens as secrets (mask in admin UI).
Change type to password so values are masked and not leaked.

 await settingsRegistry.add('Federation_Matrix_hs_token', homeserverToken, {
   readonly: true,
-  type: 'string',
+  type: 'password',
@@
 });
 
 await settingsRegistry.add('Federation_Matrix_as_token', applicationServiceToken, {
   readonly: true,
-  type: 'string',
+  type: 'password',
@@
 });

39-41: Security: predictable tokens — generate cryptographically random secrets.
Deriving tokens from a guessable uniqueId is unsafe. Use CSPRNG.

-const homeserverToken = crypto.createHash('sha256').update(`hs_${uniqueId}`).digest('hex');
-const applicationServiceToken = crypto.createHash('sha256').update(`as_${uniqueId}`).digest('hex');
+const homeserverToken = crypto.randomBytes(32).toString('hex');
+const applicationServiceToken = crypto.randomBytes(32).toString('hex');

Note: settingsRegistry.add won’t override existing values, so randomness applies only on first init.

packages/i18n/src/locales/en.i18n.json (1)

2107-2107: Updated Federation description reads well

Clearer guidance that the new federation uses the Matrix protocol. No issues.

Comment on lines +120 to +132
await settingsRegistry.add('Federation_Matrix_max_size_of_public_rooms_users', 100, {
readonly: true,
type: 'int',
i18nLabel: 'Federation_Matrix_max_size_of_public_rooms_users',
i18nDescription: 'Federation_Matrix_max_size_of_public_rooms_users_desc',
alert: 'Federation_Matrix_max_size_of_public_rooms_users_Alert',
modules: ['federation'],
public: true,
enterprise: true,
invalidValue: false,
group: 'Federation',
section: 'Matrix Bridge',
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Type mismatch: invalidValue must match int type; add bounds.
Using false can trigger coercion bugs. Provide numeric sentinel and range.

 await settingsRegistry.add('Federation_Matrix_max_size_of_public_rooms_users', 100, {
   readonly: true,
   type: 'int',
@@
-  invalidValue: false,
+  invalidValue: 100,
+  min: 1,
+  max: 100000,
📝 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.

Suggested change
await settingsRegistry.add('Federation_Matrix_max_size_of_public_rooms_users', 100, {
readonly: true,
type: 'int',
i18nLabel: 'Federation_Matrix_max_size_of_public_rooms_users',
i18nDescription: 'Federation_Matrix_max_size_of_public_rooms_users_desc',
alert: 'Federation_Matrix_max_size_of_public_rooms_users_Alert',
modules: ['federation'],
public: true,
enterprise: true,
invalidValue: false,
group: 'Federation',
section: 'Matrix Bridge',
});
await settingsRegistry.add('Federation_Matrix_max_size_of_public_rooms_users', 100, {
readonly: true,
type: 'int',
i18nLabel: 'Federation_Matrix_max_size_of_public_rooms_users',
i18nDescription: 'Federation_Matrix_max_size_of_public_rooms_users_desc',
alert: 'Federation_Matrix_max_size_of_public_rooms_users_Alert',
modules: ['federation'],
public: true,
enterprise: true,
invalidValue: 100,
min: 1,
max: 100000,
group: 'Federation',
section: 'Matrix Bridge',
});
🤖 Prompt for AI Agents
In apps/meteor/server/services/federation/Settings.ts around lines 120 to 132,
the setting defines invalidValue as boolean false which mismatches the declared
type 'int' and can cause coercion bugs; change invalidValue to a numeric
sentinel (e.g. -1) that clearly indicates "no limit" and add integer bounds (min
and max) consistent with the setting (for example min: 0 or 1 and a sensible max
like 10000) so validation will enforce the int type and range.

Comment on lines 5 to 12
await this.add('Federation_Service_Enabled', false, {
type: 'boolean',
i18nLabel: 'Federation_Service_Enabled',
i18nDescription: 'Federation_Service_Enabled_Description',
public: true,
enterprise: true,
modules: ['federation'],
invalidValue: false,
alert: 'Federation_Service_Alert',
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Legacy service toggle should be read‑only per PR goal.
Objective says “disable all visible settings of both old implementations.” Make this non‑editable.

 await this.add('Federation_Service_Enabled', false, {
   type: 'boolean',
   public: true,
+  readonly: true,
   enterprise: true,
   modules: ['federation'],
   invalidValue: false,
   alert: 'Federation_Service_Alert',
 });
📝 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.

Suggested change
await this.add('Federation_Service_Enabled', false, {
type: 'boolean',
i18nLabel: 'Federation_Service_Enabled',
i18nDescription: 'Federation_Service_Enabled_Description',
public: true,
enterprise: true,
modules: ['federation'],
invalidValue: false,
alert: 'Federation_Service_Alert',
});
await this.add('Federation_Service_Enabled', false, {
type: 'boolean',
public: true,
readonly: true,
enterprise: true,
modules: ['federation'],
invalidValue: false,
alert: 'Federation_Service_Alert',
});
🤖 Prompt for AI Agents
In apps/meteor/server/settings/federation-service.ts around lines 5 to 12, the
legacy setting 'Federation_Service_Enabled' must be made non-editable; update
the options object passed to this.add to mark the setting as read-only (e.g.,
add readonly: true or the codebase's equivalent flag) while keeping the current
default and other metadata intact so the toggle is visible but cannot be changed
in the UI.

Comment on lines +2152 to +2159
"Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
"Federation_Service_Matrix_Signing_Algorithm": "Signing Key Algorithm",
"Federation_Service_Matrix_Signing_Version": "Signing Key Version",
"Federation_Service_Matrix_Signing_Key": "Signing Key",
"Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
"Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room in a remote server",
"Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
"Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

🧩 Analysis chain

Fix English and avoid misleading guidance in signing‑key copy

  • “alfa” → “alpha”; “performatic” → “performant”; simplify “and/or”.
  • Remove “typically ... (version 4)” from the key description; Matrix signing key “version” is an arbitrary label (e.g., ed25519:a_4), not a fixed “4”. The current text can mislead admins.

Apply this diff:

-  "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+  "Federation_Service_Alert": "<strong>This is an alpha feature not intended for production use.</strong><br/>It may not be stable or performant and may change, break, or be removed without notice.",

   "Federation_Service_Matrix_Signing_Algorithm": "Signing Key Algorithm",
   "Federation_Service_Matrix_Signing_Version": "Signing Key Version",
   "Federation_Service_Matrix_Signing_Key": "Signing Key",
-  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
+  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. Typically an Ed25519 key; specify its Algorithm and Version in the fields above. Keep this key confidential to ensure secure communication between federated servers over the Matrix protocol.",

-  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them"
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "Maximum allowed members in a remote public room that users may join. Rooms exceeding this limit will still be listed, but users won't be able to join them"

Run to confirm UI/validation expectations for Algorithm/Version so the copy aligns with accepted values (e.g., “ed25519”, version labels):


🏁 Script executed:

#!/bin/bash
# Where are the new signing key fields consumed and validated?
rg -n -C2 -F 'Federation_Service_Matrix_Signing_Algorithm'
rg -n -C2 -F 'Federation_Service_Matrix_Signing_Version'
rg -n -C2 -F 'Federation_Service_Matrix_Signing_Key'

# Look for allowed values or schemas (zod/joi/simple-schema/etc.)
rg -n -i -C2 -P 'Signing_Algorithm|ed25519|Signing_Version|server\\s*key\\s*id'

Length of output: 10637


Fix English and remove misleading "version 4" claim in signing‑key copy

Algorithm is a select (only "ed25519") and Signing Version is a free string (default "0"), so don't assert a fixed "version 4".

-  "Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
+  "Federation_Service_Alert": "<strong>This is an alpha feature not intended for production use.</strong><br/>It may not be stable or performant and may change, break, or be removed without notice.",

   "Federation_Service_Matrix_Signing_Algorithm": "Signing Key Algorithm",
   "Federation_Service_Matrix_Signing_Version": "Signing Key Version",
   "Federation_Service_Matrix_Signing_Key": "Signing Key",
-  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
+  "Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. Typically an Ed25519 key; specify its Algorithm and Version in the fields above. Keep this key confidential to ensure secure communication between federated servers over the Matrix protocol.",

-  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them"
+  "Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "Maximum allowed members in a remote public room that users may join. Rooms exceeding this limit will still be listed, but users won't be able to join them"
📝 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.

Suggested change
"Federation_Service_Alert": "<strong>This is an alfa feature not intended for production usage!</strong><br/>It may not be stable and/or performatic. Please be aware that it may change, break, or even be removed in the future without any notice.",
"Federation_Service_Matrix_Signing_Algorithm": "Signing Key Algorithm",
"Federation_Service_Matrix_Signing_Version": "Signing Key Version",
"Federation_Service_Matrix_Signing_Key": "Signing Key",
"Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. This is typically an Ed25519 algorithm key (version 4), encoded as base64. It is essential for secure communication between federated servers over Matrix protocol and should be kept confidential.",
"Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room in a remote server",
"Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
"Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them",
"Federation_Service_Alert": "<strong>This is an alpha feature not intended for production use.</strong><br/>It may not be stable or performant and may change, break, or be removed without notice.",
"Federation_Service_Matrix_Signing_Algorithm": "Signing Key Algorithm",
"Federation_Service_Matrix_Signing_Version": "Signing Key Version",
"Federation_Service_Matrix_Signing_Key": "Signing Key",
"Federation_Service_Matrix_Signing_Key_Description": "The private base64 signing key used to authenticate federation requests. Typically an Ed25519 key; specify its Algorithm and Version in the fields above. Keep this key confidential to ensure secure communication between federated servers over the Matrix protocol.",
"Federation_Service_max_allowed_size_of_public_rooms_to_join": "Maximum number of members when joining a public room in a remote server",
"Federation_Service_max_allowed_size_of_public_rooms_to_join_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. <a target=\"_blank\" href=\"https://matrix.org/blog/2022/10/18/testing-faster-remote-room-joins\">Read more</a>",
"Federation_Service_max_allowed_size_of_public_rooms_to_join_Description": "Maximum allowed members in a remote public room that users may join. Rooms exceeding this limit will still be listed, but users won't be able to join them",
🤖 Prompt for AI Agents
packages/i18n/src/locales/en.i18n.json around lines 2152 to 2159: the Signing
Key description wrongly asserts a fixed "version 4" and has awkward phrasing;
update the text to remove the misleading "version 4" claim and improve English
by stating that the key is typically an Ed25519 key (encoded in base64), that
the signing algorithm is usually Ed25519, and that the signing version is a
free-form string (default "0"); keep the warning to keep the private key
confidential and clarify it is used to authenticate federation requests over the
Matrix protocol.

@ggazzo ggazzo merged commit e7247a4 into chore/federation-backup Sep 22, 2025
21 checks passed
@ggazzo ggazzo deleted the feat/federation-settings branch September 22, 2025 14:33
ggazzo added a commit that referenced this pull request Sep 23, 2025
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
ggazzo added a commit that referenced this pull request Sep 25, 2025
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants