feat: adds federation files support #36842
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #36842 +/- ##
==========================================================
Coverage ? 69.86%
==========================================================
Files ? 3033
Lines ? 103298
Branches ? 18363
==========================================================
Hits ? 72166
Misses ? 29261
Partials ? 1871
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
feat/federation
branch
from
2bf598a to
0131fdd
Compare
ricardogarim
force-pushed
the
feat/federation-files-2
branch
from
b7f0b7f to
5ef5ebb
Compare
ricardogarim
force-pushed
the
feat/federation-files-2
branch
2 times, most recently
from
b7f0b7f to
d46106d
Compare
WalkthroughAdds end-to-end Matrix media support and federated file handling: new MatrixMediaService, media routes and middleware, message flow refactors to handle media/threads/quotes, model/type updates for federation-aware uploads/messages, and E2EE type/guard adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RC as Rocket.Chat (send)
participant FM as FederationMatrix
participant MMS as MatrixMediaService
participant MX as Remote Matrix HS
RC->>FM: sendMessage({ msg, file?/files?/attachments? })
alt contains media
FM->>MMS: downloadAndStoreRemoteFile(mxcUri, metadata) / prepareLocalFileForMatrix(...)
MMS-->>FM: local fileId / mxc URI
FM->>MX: send m.room.message with mxc URI and msgtype (mapped)
else text / thread / quote
FM->>MX: send text/thread/quote message
end
MX-->>FM: event ack
FM->>RC: persist via saveMessageFromFederation(..., file?/files?/attachments?)
sequenceDiagram
autonumber
participant Client as Remote Matrix HS
participant API as Media Route (/v1/media/download/:mediaId)
participant FM as FederationMatrix
participant MMS as MatrixMediaService
participant DB as Uploads Model
Client->>API: GET /v1/media/download/:mediaId (Authorization)
API->>FM: canAccessMedia(auth, mediaId, path)
alt authorized
API->>MMS: getLocalFileForMatrixNode(mediaId, serverName)
MMS->>DB: findByFederationMediaIdAndServerName(...)
MMS-->>API: file + buffer
API-->>Client: 200 multipart/mixed (metadata + file)
else not authorized / not found
API-->>Client: 4xx/404/500 Matrix error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
packages/core-typings/src/IUpload.ts (1)
62-66: Clarify contract: if federation exists, all fields should exist.Current typing makes subfields optional, but all usages set all three and queries rely on them. Consider making them required for consistency (or document partial states).
Proposed:
- federation?: { - mxcUri?: string; - serverName?: string; - mediaId?: string; - }; + federation?: { + mxcUri: string; + serverName: string; + mediaId: string; + };If partials are expected, add a brief JSDoc describing which combinations are valid.
🧹 Nitpick comments (15)
apps/meteor/server/services/messages/service.ts (1)
98-106: Allow media-only federated messages (make msg optional).Federated events may carry attachments without text. executeSendMessage already tolerates missing msg; loosen the typing here to avoid forcing empty strings upstream.
- msg: string; + msg?: string;packages/core-services/src/types/IMessageService.ts (1)
18-21: Match service typing to accept media-only messages.Mirror the service implementation suggestion: make msg optional to support attachment-only federation events.
- msg, + msg, federation_event_id, tmid, file, files, attachments, }: { fromId: string; rid: string; - msg: string; + msg?: string; federation_event_id: string; tmid?: string; file?: IMessage['file']; files?: IMessage['files']; attachments?: IMessage['attachments'];Also applies to: 27-30
packages/model-typings/src/models/IUploadsModel.ts (1)
18-23: Return type consistency for setFederationInfo.UploadsRaw.setFederationInfo uses updateOne, which returns UpdateResult. Consider narrowing the interface return type to UpdateResult for clarity.
- setFederationInfo(fileId: string, info: { mxcUri: string; serverName: string; mediaId: string }): Promise<Document | UpdateResult>; + setFederationInfo(fileId: string, info: { mxcUri: string; serverName: string; mediaId: string }): Promise<UpdateResult>;packages/models/src/models/Uploads.ts (1)
58-63: Optional: validate info completeness at runtime.Type enforces strings, but adding a quick guard prevents partial writes from callers compiled with looser JS.
- setFederationInfo(fileId: string, info: { mxcUri: string; serverName: string; mediaId: string }): Promise<Document | UpdateResult> { + setFederationInfo(fileId: string, info: { mxcUri: string; serverName: string; mediaId: string }): Promise<Document | UpdateResult> { + if (!info?.mxcUri || !info?.serverName || !info?.mediaId) { + throw new Error('Invalid federation info'); + } return this.updateOne( { _id: fileId }, { $set: { 'federation.mxcUri': info.mxcUri, 'federation.serverName': info.serverName, 'federation.mediaId': info.mediaId } }, ); }ee/packages/federation-matrix/src/api/_matrix/media.ts (5)
47-66: Include minimal metadata in multipart header.You already support a JSON part; include filename/mimeType to help clients.
-const multipartResponse = createMultipartResponse(buffer, mimeType, fileName); +const multipartResponse = createMultipartResponse(buffer, mimeType, fileName, { fileName, mimeType });
82-132: Add security headers to error responses too.404/500 branches return JSON without SECURITY_HEADERS; align responses.
- return { statusCode: 404, body: { errcode: 'M_NOT_FOUND', error: 'Media not found' } }; + return { statusCode: 404, headers: SECURITY_HEADERS, body: { errcode: 'M_NOT_FOUND', error: 'Media not found' } }; ... - return { statusCode: 500, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' } }; + return { statusCode: 500, headers: SECURITY_HEADERS, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' } };
135-183: Thumbnail route serves full media; confirm intent.Current implementation returns the original file, not a resized thumbnail. If this is a placeholder, add a TODO or wire it to a real thumbnailer; otherwise, rename route to avoid confusion.
145-145: Type the handler context.Avoid any; use the Router’s context type for consistency.
- async (context: any) => { + async (c) => { + // c has the same typed shape as in the first route
68-76: Consider streaming large files instead of buffering.Reading full buffers can be memory-heavy for big media. Prefer a readable stream piped to the response with chunked transfer.
I can draft a streaming version using Upload.getReadStream if desired.
Also applies to: 113-121, 163-171
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
41-66: Server name source of truth.You pass
serverNamein; consider reading it fromhomeserverServices.config.serverNameto avoid mismatches and simplify call sites.ee/packages/federation-matrix/src/FederationMatrix.ts (2)
242-250: Msgtype mapping is clear; consider a tiny map for readability.Current checks are fine. Optionally use a map to avoid repeated
startsWith.const typeMap: [prefix: string, t: 'm.image'|'m.video'|'m.audio'][] = [ ['image/', 'm.image'], ['video/', 'm.video'], ['audio/', 'm.audio'], ]; return typeMap.find(([p]) => fileType?.startsWith(p))?.[1] ?? 'm.file';
252-285: Only the first attached file is sent. Support multi-file messages or document the constraint.If Matrix supports multiple file events per RC message, iterate all files and send each; return the last event id.
- const file = message.files[0]; - const mxcUri = await MatrixMediaService.prepareLocalFileForMatrix(file._id, matrixDomain); - const msgtype = this.determineFileMessageType(file.type); - const fileContent = { body: file.name, msgtype, url: mxcUri, info: { mimetype: file.type, size: file.size } }; - return this.homeserverServices.message.sendFileMessage(matrixRoomId, fileContent, matrixUserId); + let result: { eventId: string } | null = null; + for (const file of message.files) { + const mxcUri = await MatrixMediaService.prepareLocalFileForMatrix(file._id, matrixDomain); + const msgtype = this.determineFileMessageType(file.type); + const fileContent = { + body: file.name, + msgtype, + url: mxcUri, + info: { mimetype: file.type, size: file.size }, + }; + result = await this.homeserverServices.message.sendFileMessage(matrixRoomId, fileContent, matrixUserId); + } + return result;Do we want to send all attachments or just the first one for MVP?
ee/packages/federation-matrix/src/events/message.ts (3)
149-157: Extension derivation: handle empty/unknown MIME robustly.You already map
jpeg→jpg. Also guard when neither filename nor MIME yields an extension.-} else if (mimeType && mimeType.includes('/')) { +} else if (mimeType && mimeType.includes('/')) { fileExtension = mimeType.split('/')[1] || ''; if (fileExtension === 'jpeg') { fileExtension = 'jpg'; } +} else { + fileExtension = ''; }
229-242: Deduplicaterelates_toextraction.Both
replyToRelationandthreadRelationread the same path. Extract once to reduce mistakes.- const replyToRelation = content?.['m.relates_to']; - const threadRelation = content?.['m.relates_to']; + const relatesTo = content?.['m.relates_to']; + const replyToRelation = relatesTo; + const threadRelation = relatesTo;
325-342: Media branch relies onmessageBody; safe after fallback fix.Once the fallback filename is in place, this path is solid. Also consider logging
content.urlif download fails for easier triage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
apps/meteor/server/services/messages/service.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(6 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(1 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)packages/core-services/src/types/IMessageService.ts(1 hunks)packages/core-typings/src/IUpload.ts(1 hunks)packages/model-typings/src/models/IMessagesModel.ts(1 hunks)packages/model-typings/src/models/IUploadsModel.ts(2 hunks)packages/models/src/models/Messages.ts(1 hunks)packages/models/src/models/Uploads.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (2)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)packages/core-services/src/index.ts (1)
Upload(175-175)
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-144)
packages/model-typings/src/models/IMessagesModel.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
ee/packages/federation-matrix/src/FederationMatrix.ts (4)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-144)ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
getMatrixMediaRoutes(78-185)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
toExternalMessageFormat(201-212)
apps/meteor/server/services/messages/service.ts (2)
packages/apps-engine/src/definition/messages/IMessage.ts (1)
IMessage(11-36)apps/meteor/app/lib/server/methods/sendMessage.ts (1)
executeSendMessage(22-115)
packages/models/src/models/Uploads.ts (1)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)
packages/model-typings/src/models/IUploadsModel.ts (1)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)
ee/packages/federation-matrix/src/events/message.ts (4)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-144)ee/packages/federation-matrix/src/helpers/message.parsers.ts (2)
toInternalQuoteMessageFormat(125-151)toInternalMessageFormat(59-73)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/services/MatrixMediaService.ts
[error] 114-114: Step '@rocket.chat/federation-matrix:build' failed: TypeScript error TS2339: Property 'downloadFromRemoteServer' does not exist on type 'MediaService'.
🔇 Additional comments (4)
apps/meteor/server/services/messages/service.ts (1)
113-116: LGTM: safely forwarding media fields.Conditional spreads for file/files/attachments are clean and preserve existing validations in executeSendMessage.
packages/model-typings/src/models/IUploadsModel.ts (1)
2-2: Import looks good.mongodb UpdateResult import is needed for the new setter’s return type.
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
116-126: ```shell
#!/bin/bash
set -euo pipefailecho "=== Searching for literal 'federation' ==="
rg -n --hidden --no-ignore -S '\bfederation\b' || trueecho "=== Searching for Upload.uploadFile usages ==="
rg -n --hidden --no-ignore -S 'Upload.uploadFile' || trueecho "=== Searching for uploadFile occurrences/definitions ==="
rg -n --hidden --no-ignore -S '\buploadFile\b' || true
rg -n --hidden --no-ignore -S 'function\s+uploadFile|export\s+function\s+uploadFile|module.exports.uploadFile|exports.uploadFile' || trueecho "=== Searching for creation of a 'federation' user or system user ==="
rg -n --hidden --no-ignore -S 'createUser(|Meteor.users.insert|Users.insert|RocketChat.models.Users|username.*federation|username.*system|username.*rocket.cat' || trueecho "=== Searching for constants like DEFAULT_FEDERATION_USER or SYSTEM_USER ==="
rg -n --hidden --no-ignore -S 'DEFAULT_.*USER|SYSTEM_USER|FEDERATION_USER|federationUser' || true</blockquote></details> <details> <summary>ee/packages/federation-matrix/src/FederationMatrix.ts (1)</summary><blockquote> `171-173`: **Media routes wiring looks correct.** Mounting versions and media routes under `/_matrix` is consistent with the new service. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
ee/packages/federation-matrix/src/services/MatrixMediaService.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
ee/packages/federation-matrix/src/api/middlewares.ts (6)
29-29: Remove now-unused variable after URI construction changequery is no longer used once uriForSignature is built from c.req.url.
Apply this diff:
- const query = c.req.query();
38-38: Avoid consuming the request body in middleware; clone and read only when neededc.req.json() consumes the stream and will break downstream handlers for non-GET routes; it also fails for non-JSON bodies and HEAD requests.
Apply this diff:
- const body = method === 'GET' ? undefined : await c.req.json(); + let body: string | undefined; + if (!['GET', 'HEAD'].includes(method)) { + const raw = c.req.raw; + if (raw.body) { + // Read a clone so downstream can still consume the original body. + body = await raw.clone().text(); + } + }If EventAuthorizationService expects a parsed object instead of the raw string, confirm and adjust accordingly.
23-24: Type the middleware and await next()Return MiddlewareHandler for better typing and await next() for consistency with Hono patterns.
Apply this diff (plus import):
-import type { Context, Next } from 'hono'; +import type { Context, Next, MiddlewareHandler } from 'hono'; -export const canAccessMedia = (federationAuth: EventAuthorizationService) => { - return async (c: Context, next: Next) => { +export const canAccessMedia = (federationAuth: EventAuthorizationService): MiddlewareHandler => { + return async (c: Context, next: Next) => {And later:
- return next(); + await next(); + return;
25-26: Validate mediaId earlyDefensive check helps avoid confusing downstream errors if the route is misconfigured or the param is missing.
Apply this diff:
const mediaId = c.req.param('mediaId'); + if (!mediaId) { + return c.json({ errcode: 'M_INVALID_PARAM', error: 'Missing mediaId' }, 400); + }
5-21: Optional: strengthen errCodes typing and extendable mappingLock keys/types and make it easy to add codes like M_INVALID_PARAM without losing type safety.
Apply this diff:
-const errCodes: Record<string, { errcode: string; error: string; status: ContentfulStatusCode }> = { +const errCodes = { M_UNAUTHORIZED: { errcode: 'M_UNAUTHORIZED', error: 'Invalid or missing signature', status: 401, }, M_FORBIDDEN: { errcode: 'M_FORBIDDEN', error: 'Access denied', status: 403, }, + M_INVALID_PARAM: { + errcode: 'M_INVALID_PARAM', + error: 'Missing or invalid parameter', + status: 400, + }, M_UNKNOWN: { errcode: 'M_UNKNOWN', error: 'Internal server error while processing request', status: 500, }, -}; +} as const satisfies Record<string, { errcode: string; error: string; status: ContentfulStatusCode }>;
60-62: Log failures for observabilitySurface authorization failures and unexpected errors to your federation/media logger with request id/media id. Helps ops triage auth/signature issues.
If you have a logger on Context (e.g., c.get('logger')), add a log here before returning the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ee/packages/federation-matrix/src/api/_matrix/media.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(1 hunks)ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/packages/federation-matrix/src/services/MatrixMediaService.ts
- ee/packages/federation-matrix/src/api/_matrix/media.ts
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
ee/packages/federation-matrix/src/api/_matrix/media.ts (3)
101-101: Verify: File retrieval is limited to the configured server name.The function only retrieves files for the configured
serverName, which means it won't serve files originated from other federated servers. This may be intentional for security, but please verify if this aligns with federation requirements.#!/bin/bash # Description: Check if there are other media retrieval patterns in the codebase that handle cross-server federation # Search for other media file retrieval patterns in federation code rg -n -A3 -B3 "getLocalFileForMatrixNode|downloadFromRemoteServer" --type ts # Look for federation media handling patterns ast-grep --pattern 'downloadAndStoreRemoteFile($$$)'
134-150: Return appropriate error code for unimplemented thumbnail endpoint.The current implementation returns 404 with M_UNRECOGNIZED. Based on the Matrix spec and previous review discussions, consider using 413 status code with an appropriate error message for content that cannot be thumbnailed.
- async () => ({ - statusCode: 404, - body: { - errcode: 'M_UNRECOGNIZED', - error: 'This endpoint is not implemented on the homeserver side', - }, - }), + async () => ({ + statusCode: 413, + body: { + errcode: 'M_TOO_LARGE', + error: 'The local content is too large for the server to thumbnail', + }, + }),
100-109: Address TODO: Implement file streaming to avoid memory issues.Loading entire files into memory can cause OOM errors with large media files. This is a critical issue that needs to be addressed before production deployment.
Would you like me to generate a streaming implementation that efficiently handles large files without loading them entirely into memory?
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
111-112: Security: Validate MIME type to prevent content-type attacks.The MIME type from the database is used directly in the response without validation. A malicious or incorrect MIME type could lead to security issues.
- const mimeType = file.type || 'application/octet-stream'; + // Validate and sanitize MIME type + const rawMimeType = file.type || 'application/octet-stream'; + // Basic MIME type validation: type/subtype format + const mimeType = /^[a-zA-Z0-9][a-zA-Z0-9\-]*\/[a-zA-Z0-9][a-zA-Z0-9\-+.]*$/.test(rawMimeType) + ? rawMimeType + : 'application/octet-stream';
126-130: Improve error handling with proper logging.Generic error handling without logging makes debugging difficult. Consider logging the actual error for monitoring purposes.
} catch (error) { + console.error('Error handling media download request:', error); return { statusCode: 500, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' }, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ee/packages/federation-matrix/src/FederationMatrix.ts(6 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/src/FederationMatrix.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/api/_matrix/media.ts (3)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-147)ee/packages/federation-matrix/src/api/middlewares.ts (1)
canAccessMedia(5-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
38-43: LGTM! Good security headers implementation.The security headers are well-configured and follow best practices for serving media content securely.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/models/src/models/Uploads.ts (1)
14-16: Add sparse indexes for federation lookups (and consider uniqueness).Current queries on
federation.serverName/mediaIdwill scan. Add indexes; considerunique: trueif one remote media maps to exactly one local upload.protected modelIndexes(): IndexDescription[] { - return [...super.modelIndexes(), { key: { uploadedAt: -1 } }, { key: { rid: 1, _hidden: 1, typeGroup: 1 } }]; + return [ + ...super.modelIndexes(), + { key: { uploadedAt: -1 } }, + { key: { rid: 1, _hidden: 1, typeGroup: 1 } }, + // For Matrix federation media lookups + { key: { 'federation.serverName': 1, 'federation.mediaId': 1 }, sparse: true /* , unique: true (confirm before enabling) */ }, + // Optional: enable only if you introduce queries by MXC + // { key: { 'federation.mxcUri': 1 }, sparse: true }, + ]; }If you confirm 1:1 mapping, enable
unique: trueand plan a small migration to dedupe before creating the index.ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
114-117: Build error:downloadFromRemoteServerdoesn’t exist on MediaService. Add a compatibility shim and replace the call.This causes TS2339 in CI. Probe for known method names.
- const buffer = await this.homeserverServices.media.downloadFromRemoteServer(parts.serverName, parts.mediaId); + const buffer = await this.downloadRemoteMedia(parts.serverName, parts.mediaId);Add this helper inside the class (e.g., after
parseMXCUri):private static async downloadRemoteMedia(serverName: string, mediaId: string): Promise<Buffer> { if (!this.homeserverServices?.media) { throw new Error('Homeserver media service not initialized. Call setHomeserverServices first.'); } const media: any = this.homeserverServices.media; if (typeof media.downloadFromRemoteServer === 'function') { return media.downloadFromRemoteServer(serverName, mediaId); } if (typeof media.downloadRemote === 'function') { return media.downloadRemote(serverName, mediaId); } if (typeof media.download === 'function') { return media.download(serverName, mediaId); } throw new Error('Homeserver media service does not expose a download method (expected one of: downloadFromRemoteServer | downloadRemote | download)'); }
🧹 Nitpick comments (6)
packages/models/src/models/Uploads.ts (1)
54-59: Normalize and set federation as a single object (optional).Minor: set the whole
federationobject and normalizeserverNameto lowercase for consistent indexing.return this.updateOne( { _id: fileId }, - { $set: { 'federation.mxcUri': info.mxcUri, 'federation.serverName': info.serverName, 'federation.mediaId': info.mediaId } }, + { + $set: { + federation: { + mxcUri: info.mxcUri, + serverName: info.serverName.toLowerCase(), + mediaId: info.mediaId, + }, + }, + }, );If you adopt normalization, also lower-case in all finders.
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (5)
29-39: NormalizeserverNamein MXC parsing (minor).Lower-case
serverNameto avoid index misses due to case variance.- return { - serverName: match[1], - mediaId: match[2], - }; + return { + serverName: match[1].toLowerCase(), + mediaId: match[2], + };
119-129: Add size/MIME guardrails before persisting remote media.Enforce configured limits to prevent oversized/disallowed uploads.
const uploadedFile = await Upload.uploadFile({ userId: metadata.userId || 'federation', - buffer, + buffer, details: { name: metadata.name || 'unnamed', - size: buffer.length, - type: metadata.type || 'application/octet-stream', + size: buffer.length, + type: metadata.type || 'application/octet-stream', rid: metadata.roomId, userId: metadata.userId || 'federation', }, });Insert just before
Upload.uploadFile:const limits = (this.homeserverServices as any)?.config?.media; if (limits?.maxFileSize && buffer.length > limits.maxFileSize) { throw new Error(`Remote media exceeds max file size (${buffer.length} > ${limits.maxFileSize})`); } if (limits?.allowedMimeTypes && metadata.type && !limits.allowedMimeTypes.includes(metadata.type)) { throw new Error(`MIME type "${metadata.type}" not allowed by configuration`); }
119-129: Pass message linkage if supported by the upload service.
IUploadusesmessage_id; wire it if available.details: { name: metadata.name || 'unnamed', size: buffer.length, type: metadata.type || 'application/octet-stream', rid: metadata.roomId, userId: metadata.userId || 'federation', + // If supported by Upload.uploadFile details: + // message_id: metadata.messageId, },Confirm the expected key is
message_id(underscore) vsmessageId.
105-137: Handle duplicate races with a unique index fallback.Two workers can download the same remote media concurrently. After adding a unique sparse index on (
federation.serverName,federation.mediaId), catch E11000 and return the existing upload id.try { await Uploads.setFederationInfo(uploadedFile._id, { mxcUri, serverName: parts.serverName, mediaId: parts.mediaId }); return uploadedFile._id; } catch (e: any) { if (e?.code === 11000) { const existing = await Uploads.findByFederationMediaIdAndServerName(parts.mediaId, parts.serverName); if (existing?._id && existing._id !== uploadedFile._id) { // Optional: decide whether to keep or clean up the duplicate file content return existing._id; } } throw e; }
144-146: Provide a streaming variant for file reads.Add
getLocalFileStreamto avoid buffering whole files when responding over HTTP.static async getLocalFileStream(file: IUpload): Promise<NodeJS.ReadableStream> { return Upload.getReadStream({ file }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)packages/model-typings/src/models/IUploadsModel.ts(2 hunks)packages/models/src/models/Uploads.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/model-typings/src/models/IUploadsModel.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/models/src/models/Uploads.ts (1)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (2)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)packages/core-services/src/index.ts (1)
Upload(175-175)
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
packages/models/src/models/Uploads.ts (1)
50-52: LGTM: targeted finder for (serverName, mediaId).ee/packages/federation-matrix/src/services/MatrixMediaService.ts (2)
19-23: Static DI setup looks fine.
114-114: No other downloadFromRemoteServer call sites found. ripgrep returned a single match at ee/packages/federation-matrix/src/services/MatrixMediaService.ts:114 — no other matches under ee/ or packages/.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (3)
87-143: Consider streaming approach for large files.The current implementation buffers entire files in memory, which can cause memory issues for large media. Consider implementing streaming to handle large files more efficiently.
As noted in the TODO comment and previous discussions, this should be refactored to use streaming:
// Future implementation sketch: // const stream = await this.downloadRemoteMediaStream(parts.serverName, parts.mediaId); // await Upload.uploadFileStream({ userId, stream, details: { ... } });Consider creating a follow-up task to implement streaming support in both the download and upload pipelines.
114-114: Fix build: replace non-existentdownloadFromRemoteServerwith a compatibility shim.The build fails with TS2339 because
MediaServicedoesn't exposedownloadFromRemoteServer. Add a helper method that probes for available download methods.Apply this diff to fix the method call:
- const buffer = await this.homeserverServices.media.downloadFromRemoteServer(parts.serverName, parts.mediaId); + const buffer = await this.downloadRemoteMedia(parts.serverName, parts.mediaId);Add this helper method after
parseMXCUri(around line 40):private static async downloadRemoteMedia(serverName: string, mediaId: string): Promise<Buffer> { if (!this.homeserverServices?.media) { throw new Error('Homeserver media service not initialized. Call setHomeserverServices first.'); } const media: any = this.homeserverServices.media; if (typeof media.downloadFromRemoteServer === 'function') { return await media.downloadFromRemoteServer(serverName, mediaId); } if (typeof media.downloadRemote === 'function') { return await media.downloadRemote(serverName, mediaId); } if (typeof media.download === 'function') { return await media.download(serverName, mediaId); } throw new Error('Homeserver media service does not expose a download method (expected one of: downloadFromRemoteServer | downloadRemote | download)'); }
114-118: Add media size and MIME type validation before storing.Validate downloaded media against configured limits to prevent storing oversized or disallowed file types.
Apply this diff after obtaining the buffer:
const buffer = await this.downloadRemoteMedia(parts.serverName, parts.mediaId); if (!buffer) { throw new Error('Download from remote server returned null content.'); } + + const limits = this.homeserverServices?.config?.media; + if (limits?.maxFileSize && buffer.length > limits.maxFileSize) { + throw new Error(`Remote media exceeds max file size (${buffer.length} > ${limits.maxFileSize})`); + } + if (limits?.allowedMimeTypes && metadata.type && !limits.allowedMimeTypes.includes(metadata.type)) { + throw new Error(`MIME type "${metadata.type}" not allowed by configuration`); + }
🧹 Nitpick comments (3)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (3)
25-27: Validate serverName parameter in MXC URI generation.The
generateMXCUrimethod should validate the serverName to prevent malformed URIs.Add validation to ensure serverName doesn't contain invalid characters:
static generateMXCUri(fileId: string, serverName: string): string { + if (!serverName || serverName.includes('/')) { + throw new Error('Invalid serverName for MXC URI generation'); + } return `mxc://${serverName}/${fileId}`; }
121-129: Remove duplicate userId field in upload details.The
userIdfield is specified twice in the details object - once on line 122 and again on line 128.Apply this diff to remove the duplicate:
const uploadedFile = await Upload.uploadFile({ userId: metadata.userId || 'federation', buffer, details: { name: metadata.name || 'unnamed', size: buffer.length, type: metadata.type || 'application/octet-stream', rid: metadata.roomId, - userId: metadata.userId || 'federation', }, });
41-66: Add federation domain validation in prepareLocalFileForMatrix.When creating new federation metadata, validate that the serverName matches expected federation domains to prevent potential security issues.
Consider adding domain validation before creating the MXC URI:
const mxcUri = this.generateMXCUri(fileId, serverName); + + // Validate serverName against allowed federation domains if configured + const allowedDomains = this.homeserverServices?.config?.federation?.allowedDomains; + if (allowedDomains && !allowedDomains.includes(serverName)) { + throw new Error(`Server ${serverName} not in allowed federation domains`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (2)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)packages/core-services/src/index.ts (1)
Upload(175-175)
⏰ 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)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
18-24: LGTM! Clean dependency injection pattern.The static class design with explicit dependency injection via
setHomeserverServicesprovides good separation of concerns and testability.
sampaiodiego
force-pushed
the
feat/federation-files-2
branch
from
f5c636e to
b83f6cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
100-105: Bug: enableThumbnails always true.process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true forces true.
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true',Or default-on: process.env.MEDIA_ENABLE_THUMBNAILS !== 'false'.
ee/packages/federation-matrix/src/events/message.ts (1)
372-388: Resolve sender via bridge mapping, not raw username, for redactions.data.sender is a Matrix ID; Users.findOneByUsername(sender) will fail once usernames are sanitized.
- const internalUsername = data.sender; - const user = await Users.findOneByUsername(internalUsername); + const user = await getOrCreateFederatedUser(data.sender); if (!user) { - logger.debug(`User not found: ${internalUsername}`); + logger.debug(`User not found for Matrix sender: ${data.sender}`); return; }
♻️ Duplicate comments (8)
ee/packages/federation-matrix/src/api/_matrix/media.ts (3)
100-116: Avoid loading entire files into memory; add streaming now or track.Reading full Buffer and building multipart in memory won’t scale for large media.
- Short term: gate by size and return 413 for oversized payloads.
- Next step: switch to streamed responses (Readable/pipe) instead of Buffer.concat; add a Jira task and link here.
76-79: Guard required services upfront (fail fast, clearer errors).Validate homeserverServices.config.serverName and federationAuth before use.
export const getMatrixMediaRoutes = (homeserverServices: HomeserverServices) => { - const { config, federationAuth } = homeserverServices; + if (!homeserverServices?.config?.serverName) { + throw new TypeError('getMatrixMediaRoutes: homeserverServices.config.serverName is required'); + } + if (!homeserverServices?.federationAuth) { + throw new TypeError('getMatrixMediaRoutes: homeserverServices.federationAuth is required'); + } + const { config, federationAuth } = homeserverServices; const router = new Router('/federation');
45-65: Sanitize multipart headers to prevent CRLF/header injection.fileName and metadata are written into MIME headers without sanitization.
Apply:
function createMultipartResponse( buffer: Buffer, mimeType: string, fileName: string, metadata: Record<string, any> = {}, ): { body: Buffer; contentType: string } { const boundary = crypto.randomBytes(16).toString('hex'); const parts: string[] = []; - parts.push(`--${boundary}`, 'Content-Type: application/json', '', JSON.stringify(metadata)); - parts.push(`--${boundary}`, `Content-Type: ${mimeType}`, `Content-Disposition: attachment; filename="${fileName}"`, ''); + // harden inputs for header context + const safeName = String(fileName).replace(/[\r\n"]/g, ''); + let safeMetadata: unknown = {}; + try { + // drop non-serializable fields + safeMetadata = JSON.parse(JSON.stringify(metadata ?? {})); + } catch { + safeMetadata = {}; + } + parts.push(`--${boundary}`, 'Content-Type: application/json', '', JSON.stringify(safeMetadata)); + parts.push(`--${boundary}`, `Content-Type: ${mimeType}`, `Content-Disposition: attachment; filename="${safeName}"`, '');ee/packages/federation-matrix/src/FederationMatrix.ts (1)
569-574: Do not drop user-entered text when files are present.Send files and then, if message.msg has text, send a separate text event.
- if (message.files && message.files.length > 0) { - result = await this.handleFileMessage(message, matrixRoomId, actualMatrixUserId, this.serverName); - } else { - result = await this.handleTextMessage(message, matrixRoomId, actualMatrixUserId, this.serverName); - } + if (message.files && message.files.length > 0) { + const fileResult = await this.handleFileMessage(message, matrixRoomId, actualMatrixUserId, this.serverName); + if (message.msg && message.msg.trim().length > 0) { + // fire-and-forget text message; result maps to file event + void this.handleTextMessage(message, matrixRoomId, actualMatrixUserId, this.serverName); + } + result = fileResult; + } else { + result = await this.handleTextMessage(message, matrixRoomId, actualMatrixUserId, this.serverName); + }ee/packages/federation-matrix/src/events/message.ts (3)
229-232: Don’t drop edit events with no body — compute isEdited before early return.m.replace with only m.new_content gets ignored today.
- const msgtype = content?.msgtype; - const messageBody = content?.body?.toString(); - - if (!messageBody && !msgtype) { + const msgtype = content?.msgtype; + const messageBody = content?.body?.toString(); + const isEditedMessage = content?.['m.relates_to']?.rel_type === 'm.replace'; + + if (!messageBody && !msgtype && !isEditedMessage) { logger.debug('No message content found in event'); return; } @@ - const isEditedMessage = data.content['m.relates_to']?.rel_type === 'm.replace'; + // already computed aboveAlso applies to: 253-253
15-21: Parse Matrix IDs with first colon only (ports safe).split(':') drops port (e.g., @U:example.com:8448). Use first colon after '@'.
- const [userPart, domain] = matrixUserId.split(':'); - if (!userPart || !domain) { + const idx = matrixUserId.indexOf(':', 1); // skip leading '@' + const userPart = idx > 0 ? matrixUserId.slice(0, idx) : matrixUserId; + const domain = idx > 0 ? matrixUserId.slice(idx + 1) : ''; + if (!userPart || !domain) {
144-168: Null-safety for media fields; safe filename; avoid encodeURIComponent on undefined.Guard content.info/body and use deterministic fallback name.
- const fileInfo = content.info; - const mimeType = fileInfo.mimetype; - const fileName = messageBody; + const fileInfo = content?.info ?? {}; + const mimeType = fileInfo.mimetype ?? 'application/octet-stream'; + const fileName = (messageBody && String(messageBody).trim().length > 0) ? String(messageBody) : `${msgtype}-${eventId}`; @@ - const fileRefId = await MatrixMediaService.downloadAndStoreRemoteFile(content.url, { - name: messageBody, - size: fileInfo.size, - type: mimeType, + const fileRefId = await MatrixMediaService.downloadAndStoreRemoteFile(content.url, { + name: fileName, + size: typeof fileInfo.size === 'number' ? fileInfo.size : 0, + type: mimeType,packages/models/src/models/Messages.ts (1)
607-618: Add index to support sort by ts on tmid with federation filter.Current query can in-memory sort. Add a compound partial index.
Apply (outside this hunk) in modelIndexes():
protected modelIndexes(): IndexDescription[] { return [ ... + // threads: latest federated event by tmid + { key: { tmid: 1, ts: -1 }, partialFilterExpression: { 'federation.eventId': { $exists: true } } }, ]; }
🧹 Nitpick comments (4)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
251-253: Use logger, not console.log, for errors.- console.log(error); + this.logger.error(error);
431-455: Multi-file TODO — consider batching.Matrix supports multiple file sends; iterate files with ordered awaits and aggregate last eventId or store per-file mapping.
ee/packages/federation-matrix/src/events/message.ts (2)
166-173: encodeURIComponent on defined string only.- const fileUrl = `/file-upload/${fileRefId}/${encodeURIComponent(fileName)}`; + const fileUrl = `/file-upload/${fileRefId}/${encodeURIComponent(String(fileName))}`;
9-10: Avoid cyclic import with FederationMatrix.Importing fileTypes from ../FederationMatrix creates a module cycle. Move fileTypes/MatrixFileTypes to a small shared constants module and import from there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts(3 hunks)apps/meteor/client/lib/e2ee/rocketchat.e2e.ts(2 hunks)apps/meteor/server/services/messages/service.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(6 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(1 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)packages/core-services/src/types/IMessageService.ts(1 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/core-typings/src/IUpload.ts(1 hunks)packages/model-typings/src/models/IMessagesModel.ts(1 hunks)packages/model-typings/src/models/IUploadsModel.ts(2 hunks)packages/models/src/models/Messages.ts(1 hunks)packages/models/src/models/Uploads.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/core-typings/src/IMessage/IMessage.ts
- apps/meteor/client/lib/e2ee/rocketchat.e2e.ts
- packages/core-services/src/types/IMessageService.ts
- ee/packages/federation-matrix/src/api/middlewares.ts
- apps/meteor/server/services/messages/service.ts
- packages/model-typings/src/models/IUploadsModel.ts
- ee/packages/federation-matrix/src/services/MatrixMediaService.ts
- packages/models/src/models/Uploads.ts
- apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
- packages/core-typings/src/IUpload.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:09:18.041Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-09-16T22:09:18.041Z
Learning: Use Rocket.Chat primary documentation and provided reference files for guidance
Applied to files:
ee/packages/federation-matrix/src/events/message.ts
🧬 Code graph analysis (5)
packages/models/src/models/Messages.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)
packages/model-typings/src/models/IMessagesModel.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)
ee/packages/federation-matrix/src/events/message.ts (6)
packages/core-typings/src/IUser.ts (1)
IUser(186-253)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(34-39)apps/meteor/server/services/meteor/service.ts (1)
MeteorService(130-300)ee/packages/federation-matrix/src/helpers/message.parsers.ts (2)
toInternalQuoteMessageFormat(125-151)toInternalMessageFormat(59-73)
ee/packages/federation-matrix/src/api/_matrix/media.ts (3)
packages/core-typings/src/IUpload.ts (1)
IUpload(3-67)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/api/middlewares.ts (1)
canAccessMedia(6-33)
ee/packages/federation-matrix/src/FederationMatrix.ts (5)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
getMatrixMediaRoutes(76-153)ee/packages/federation-matrix/src/events/message.ts (1)
message(221-389)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
toExternalMessageFormat(201-212)
🔇 Additional comments (5)
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
116-124: Verify Matrix S2S response shape (multipart vs raw).Spec for GET /_matrix/federation/v1/media/download/{mediaId} typically serves the raw file with its content-type. Confirm multipart/mixed is acceptable for S2S downloads; otherwise serve the binary directly and move metadata to headers.
134-150: Thumbnail endpoint: status code and errcode OK as stub.Returning 404 with M_UNRECOGNIZED is acceptable while unimplemented.
Consider 413 (too large to thumbnail) once thumbnailing is added.
packages/model-typings/src/models/IMessagesModel.ts (1)
106-107: Signature aligns with current callers.tmid + messageId exclusion makes sense for latest thread event lookup.
Ensure all usages pass the current message _id (as in FederationMatrix.ts).
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
114-115: Wiring homeserverServices into MatrixMediaService — LGTM.
412-419: Graceful fallback for unknown MIME types — LGTM.
| const user = await Users.findOneByUsername(matrixUserId); | ||
| if (user) { | ||
| await MatrixBridgedUser.createOrUpdateByLocalId(user._id, matrixUserId, false, domain); | ||
| return user; | ||
| } |
There was a problem hiding this comment.
Lookup by sanitized local username or bridge mapping, not raw Matrix ID.
Users.findOneByUsername(matrixUserId) mismatches RC username rules.
- Use MatrixBridgedUser to resolve/create the local user by external ID.
- If keeping local usernames, derive a validated local username (no '@' or ':') and use that for lookups; keep the Matrix ID only in the bridge mapping.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 23-27, don't
call Users.findOneByUsername(matrixUserId) with the raw Matrix ID; instead
resolve/create the local user via the bridge mapping or a sanitized local
username. Replace the direct Users lookup with a MatrixBridgedUser call that
finds or creates the local account from the external Matrix ID (e.g.,
findOrCreateByExternalId(matrixUserId, domain)) and return the associated local
User; if you must look up by username, first derive and validate a sanitized
local username by stripping leading '@' and any ':domain' portion and validating
against Rocket.Chat username rules before calling Users.findOneByUsername, and
ensure the Matrix ID is stored only in the bridge mapping.
| const userData = { | ||
| username: matrixUserId, | ||
| name: username, // TODO: Fetch display name from Matrix profile | ||
| type: 'user', | ||
| status: UserStatus.ONLINE, | ||
| active: true, | ||
| roles: ['user'], | ||
| requirePasswordChange: false, | ||
| federated: true, | ||
| federation: { | ||
| version: 1, | ||
| }, | ||
| createdAt: new Date(), | ||
| _updatedAt: new Date(), | ||
| }; | ||
|
|
||
| if (!user) { | ||
| logger.info('Creating new federated user:', { username: internalUsername, externalId: data.sender }); | ||
|
|
||
| const userData: Partial<IUser> = { | ||
| username: internalUsername, | ||
| name: username, // TODO: Fetch display name from Matrix profile | ||
| type: 'user', | ||
| status: UserStatus.ONLINE, | ||
| active: true, | ||
| roles: ['user'], | ||
| requirePasswordChange: false, | ||
| federated: true, // Mark as federated user | ||
| federation: { | ||
| version: 1, | ||
| }, | ||
| createdAt: new Date(), | ||
| _updatedAt: new Date(), | ||
| }; | ||
| const { insertedId } = await Users.insertOne(userData); | ||
|
|
There was a problem hiding this comment.
Do not insert users directly — use vetted creation flow.
Users.insertOne bypasses validations/hooks/uniqueness.
Replace with the established user-creation API for federation (service/adapter) that enforces validateUsername, uniqueness fallback, roles, and denormalizations. I can draft the exact call once you confirm the preferred factory.
ggazzo
force-pushed
the
chore/federation-backup
branch
from
5d3431f to
76470a2
Compare
There was a problem hiding this comment.
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 (1)
ee/packages/federation-matrix/src/events/message.ts (1)
355-385: Redaction: resolve the user via bridge mapping (orgetOrCreateFederatedUser), not raw username.
Users.findOneByUsername(data.sender)repeats the Matrix‑ID‑as‑username issue and may fail.- const internalUsername = data.sender; - const user = await Users.findOneByUsername(internalUsername); + const user = await getOrCreateFederatedUser(data.sender); if (!user) { - logger.debug(`User not found: ${internalUsername}`); + logger.debug(`User not found for redaction sender: ${data.sender}`); return; }
♻️ Duplicate comments (4)
ee/packages/federation-matrix/src/events/message.ts (4)
23-27: Do not use raw Matrix IDs as RC usernames; derive a validated, unique local username.
@/:are invalid and cross‑homeserver collisions are likely. Use a sanitized username and keep the Matrix ID in the bridge mapping.- const user = await Users.findOneByUsername(matrixUserId); + // Derive a local username that avoids collisions across homeservers. + const safeDomain = domain.replace(/[^0-9a-zA-Z]/g, '_'); + const localUsername = `${baseLocalName}_${safeDomain}`; // e.g., "alice_example_com" + const user = await Users.findOneByUsername(localUsername); @@ - logger.info('Creating new federated user:', { username: matrixUserId, externalId: matrixUserId }); + logger.info('Creating new federated user:', { username: localUsername, externalId: matrixUserId }); @@ - username: matrixUserId, - name: username, // TODO: Fetch display name from Matrix profile + username: localUsername, + name: baseLocalName, // TODO: Fetch display name from Matrix profile @@ - federation: { - version: 1, - }, + // Avoid using deprecated "federation" fieldAlso applies to: 31-45
47-54: Don’t callUsers.insertOne— use the vetted user‑creation flow with validation and uniqueness fallback.Direct inserts bypass hooks/validation (username regex, denormalizations, roles). Replace with the established federation‑safe creation API.
Run to locate the canonical creation helper:
#!/bin/bash rg -nP -C2 '(createFederatedUser|createFederatedUserInternallyOnly|Accounts\.createUserAsync|Users\.create\()' --type=ts --type=js
221-229: Don’t drop edit events — computeisEditedMessagebefore early return.Edits may have no
content.bodybut carrym.new_content. Current guard discards them.- const msgtype = content?.msgtype; - const messageBody = content?.body?.toString(); - - if (!messageBody && !msgtype) { + const msgtype = content?.msgtype; + const messageBody = content?.body?.toString(); + const isEditedMessage = content?.['m.relates_to']?.rel_type === 'm.replace'; + + if (!messageBody && !msgtype && !isEditedMessage) { logger.debug('No message content found in event'); return; } @@ - const isEditedMessage = data.content['m.relates_to']?.rel_type === 'm.replace'; + // already computed aboveAlso applies to: 250-251
16-21: Parse Matrix IDs using the first colon (ports supported) and validate@prefix.Current split drops ports like
:8448and may accept malformed IDs.- const [userPart, domain] = matrixUserId.split(':'); - if (!userPart || !domain) { + const idx = matrixUserId.indexOf(':', 1); // skip leading '@' + const userPart = idx > 0 ? matrixUserId.slice(0, idx) : matrixUserId; + const domain = idx > 0 ? matrixUserId.slice(idx + 1) : ''; + if (!userPart || !domain || !userPart.startsWith('@')) { logger.error('Invalid Matrix sender ID format:', matrixUserId); return null; } - const username = userPart.substring(1); + const baseLocalName = userPart.slice(1);
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/events/message.ts (2)
25-26: Mark bridged Matrix users as remote consistently.
isRemoteshould betruefor external Matrix users, even when already existing.- await MatrixBridgedUser.createOrUpdateByLocalId(user._id, matrixUserId, false, domain); + await MatrixBridgedUser.createOrUpdateByLocalId(user._id, matrixUserId, true, domain);
81-85: Avoid relying on deprecatedroom.federated; use bridge mapping as the source of truth.
IRoom.federatedis deprecated. Prefer MatrixBridgedRoom mapping (presence implies federated) or a new flag owned by federation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/server/services/messages/service.ts(1 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)packages/core-services/src/types/IMessageService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/server/services/messages/service.ts
- packages/core-services/src/types/IMessageService.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T22:09:18.041Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-09-16T22:09:18.041Z
Learning: Use Rocket.Chat primary documentation and provided reference files for guidance
Applied to files:
ee/packages/federation-matrix/src/events/message.ts
📚 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:
ee/packages/federation-matrix/src/events/message.ts
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/events/message.ts (6)
packages/core-typings/src/IUser.ts (1)
IUser(186-253)packages/models/src/index.ts (6)
Users(209-209)MatrixBridgedUser(217-217)MatrixBridgedRoom(216-216)Rooms(200-200)Subscriptions(206-206)Messages(184-184)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(34-39)ee/packages/federation-matrix/src/helpers/message.parsers.ts (2)
toInternalQuoteMessageFormat(125-151)toInternalMessageFormat(59-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/events/message.ts (1)
111-120: Thread lookup helper looks good.Clear behavior and minimal DB touches.
| const fileInfo = content.info; | ||
| const mimeType = fileInfo.mimetype; | ||
| const fileName = messageBody; | ||
|
|
||
| const fileRefId = await MatrixMediaService.downloadAndStoreRemoteFile(content.url, { | ||
| name: messageBody, | ||
| size: fileInfo.size, | ||
| type: mimeType, | ||
| roomId: room._id, | ||
| userId: user._id, | ||
| }); |
There was a problem hiding this comment.
Null‑safety for media fields and safe filename; avoid encodeURIComponent on undefined.
Guard content.info/body, set sensible defaults, and always pass a defined filename.
- const fileInfo = content.info;
- const mimeType = fileInfo.mimetype;
- const fileName = messageBody;
+ const fileInfo = content?.info || {};
+ const mimeType = fileInfo.mimetype || 'application/octet-stream';
+ const fileName = (messageBody && String(messageBody).trim().length > 0)
+ ? String(messageBody).trim()
+ : `${msgtype || 'm.file'}-${eventId}`;
@@
- const fileRefId = await MatrixMediaService.downloadAndStoreRemoteFile(content.url, {
- name: messageBody,
- size: fileInfo.size,
- type: mimeType,
+ const fileRefId = await MatrixMediaService.downloadAndStoreRemoteFile(content.url, {
+ name: fileName,
+ size: typeof fileInfo.size === 'number' ? fileInfo.size : 0,
+ type: mimeType,
@@
- const fileUrl = `/file-upload/${fileRefId}/${encodeURIComponent(fileName)}`;
+ const fileUrl = `/file-upload/${fileRefId}/${encodeURIComponent(fileName)}`;Also applies to: 153-163
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 141-151 (and
likewise update 153-163), guard access to content.info and message body: use
content?.info and provide a fallback object, derive mimeType with a default like
'application/octet-stream', and compute a safe filename by preferring
messageBody or content.info.name and falling back to a deterministic default
(e.g., room id + timestamp). Only call encodeURIComponent when the filename is
defined; ensure the name passed to MatrixMediaService.downloadAndStoreRemoteFile
is always a defined string.
| messageToReplyToUrl, | ||
| formattedMessage: data.content.formatted_body || '', | ||
| rawMessage: message, | ||
| rawMessage: messageBody, | ||
| homeServerDomain: serverName, | ||
| senderExternalId: data.sender, | ||
| }); |
There was a problem hiding this comment.
Edited quote: use m.new_content.body as the raw text.
Using messageBody here is wrong for edits; it may be undefined or stale.
- rawMessage: messageBody,
+ rawMessage: data.content?.['m.new_content']?.body || '',📝 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.
| messageToReplyToUrl, | |
| formattedMessage: data.content.formatted_body || '', | |
| rawMessage: message, | |
| rawMessage: messageBody, | |
| homeServerDomain: serverName, | |
| senderExternalId: data.sender, | |
| }); | |
| messageToReplyToUrl, | |
| formattedMessage: data.content.formatted_body || '', | |
| rawMessage: data.content?.['m.new_content']?.body || '', | |
| homeServerDomain: serverName, | |
| senderExternalId: data.sender, |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 274-279, the
code sets rawMessage to messageBody which is incorrect for edited messages;
change the logic to detect edits (presence of data.content["m.new_content"]) and
use the edited plain-text payload (m.new_content.body) as rawMessage (and use
m.new_content.formatted_body for formattedMessage when present); fall back to
the existing messageBody/Content fields if m.new_content is absent and add
null/undefined guards so rawMessage and formattedMessage are never left
undefined.
| if (isMediaMessage && content?.url) { | ||
| const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, tmid); | ||
| await Message.saveMessageFromFederation(result); | ||
| } else { |
There was a problem hiding this comment.
tmid is undefined; compile/runtime error. Pass the computed thread (and keep tshow).
Also unifies media vs text thread handling.
- if (isMediaMessage && content?.url) {
- const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, tmid);
+ if (isMediaMessage && content?.url) {
+ const tmid = thread?.tmid;
+ const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, tmid);
await Message.saveMessageFromFederation(result);Recommended stronger refactor (unify on thread end‑to‑end):
-async function handleMediaMessage( /* ... */, tmid?: string, ): Promise<{ /* ... */ tmid?: string; /* ... */ }> {
+async function handleMediaMessage( /* ... */, thread?: { tmid: string; tshow: boolean }, ): Promise<{ /* ... */ thread?: { tmid: string; tshow: boolean }; /* ... */ }> {
@@
- federation_event_id: eventId,
- tmid,
+ federation_event_id: eventId,
+ thread,
@@
- const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, thread?.tmid);
+ const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, thread);📝 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.
| if (isMediaMessage && content?.url) { | |
| const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, tmid); | |
| await Message.saveMessageFromFederation(result); | |
| } else { | |
| if (isMediaMessage && content?.url) { | |
| const tmid = thread?.tmid; | |
| const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, tmid); | |
| await Message.saveMessageFromFederation(result); | |
| } else { |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 332 to 335,
the code passes an undefined variable tmid into handleMediaMessage which causes
compile/runtime errors; replace tmid with the computed thread object (and retain
tshow) so media message handling uses the same thread data as text messages, and
refactor the call sites so both media and text paths pass the unified thread
variable end-to-end (update handleMediaMessage signature/use if needed).
…at/federation-files-2
ggazzo
force-pushed
the
feat/federation-files-2
branch
from
d77bdcf to
32e15a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
ee/packages/federation-matrix/src/events/message.ts (6)
206-215: Media messages should preserve thread metadata consistently with text messages.Media path returns only
tmidwhile text path uses the fullthreadobject withtshow. This creates inconsistent message structures.return { fromId: user._id, rid: room._id, msg: '', federation_event_id: eventId, - tmid, + thread: tmid ? { tmid, tshow: false } : undefined, file: fileData, files: [fileData], attachments: [attachment], };
16-21: Parse Matrix IDs using the first colon only to preserve port numbers.The current
matrixUserId.split(':')incorrectly drops port components (e.g.,@u:example.com:8448becomes["@u", "example.com", "8448"]). UseindexOfto split on the first colon only.- const [userPart, domain] = matrixUserId.split(':'); - if (!userPart || !domain) { - logger.error('Invalid Matrix sender ID format:', matrixUserId); - return null; - } - const username = userPart.substring(1); + const idx = matrixUserId.indexOf(':'); + if (idx <= 1 || !matrixUserId.startsWith('@')) { + logger.error('Invalid Matrix sender ID format:', matrixUserId); + return null; + } + const userPart = matrixUserId.substring(0, idx); + const domain = matrixUserId.substring(idx + 1); + const username = userPart.substring(1);
23-27: Matrix IDs cannot be used directly as Rocket.Chat usernames — they violate username validation rules.
Users.findOneByUsername(matrixUserId)won't work because Matrix IDs contain@and:which are not allowed in Rocket.Chat usernames pervalidateUsernameregex/^[0-9a-zA-Z-_.]+$/.Use the bridge mapping to resolve users or derive a validated local username:
- const user = await Users.findOneByUsername(matrixUserId); - if (user) { - await MatrixBridgedUser.createOrUpdateByLocalId(user._id, matrixUserId, false, domain); - return user; - } + // First check if user exists via bridge mapping + const bridgedUser = await MatrixBridgedUser.findOneByExternalId(matrixUserId); + if (bridgedUser) { + const user = await Users.findOneById(bridgedUser.localId); + if (user) { + return user; + } + }
31-48: Don't useUsers.insertOne— it bypasses validation and triggers.Direct database inserts skip username validation, uniqueness checks, hooks, and denormalizations. Use the established user creation flow instead.
Replace with the proper creation API:
- const userData = { - username: matrixUserId, - name: username, // TODO: Fetch display name from Matrix profile - type: 'user', - status: UserStatus.ONLINE, - active: true, - roles: ['user'], - requirePasswordChange: false, - federated: true, - federation: { - version: 1, - }, - createdAt: new Date(), - _updatedAt: new Date(), - }; - - const { insertedId } = await Users.insertOne(userData); + // Derive a valid local username and ensure uniqueness + let localUsername = username.replace(/[^0-9a-zA-Z-_.]/g, '_'); + let suffix = 0; + while (await Users.findOneByUsername(localUsername)) { + suffix++; + localUsername = `${username.replace(/[^0-9a-zA-Z-_.]/g, '_')}_${suffix}`; + } + + // Use the proper creation flow + const userId = await Users.create({ + username: localUsername, + name: username, + type: 'user', + status: UserStatus.ONLINE, + active: true, + roles: ['user'], + requirePasswordChange: false, + federated: true, + federation: { + version: 1, + }, + }); + + const insertedId = userId;
141-151: Add null safety for media fields and ensure valid filename.
content.infocould be undefined, andmessageBodymight be empty/missing, causing runtime errors.- const fileInfo = content.info; - const mimeType = fileInfo.mimetype; - const fileName = messageBody; + const fileInfo = content?.info || {}; + const mimeType = fileInfo.mimetype || 'application/octet-stream'; + const fileName = (messageBody && String(messageBody).trim()) + ? String(messageBody).trim() + : `${msgtype || 'file'}-${eventId}`; const fileRefId = await MatrixMediaService.downloadAndStoreRemoteFile(content.url, { - name: messageBody, - size: fileInfo.size, + name: fileName, + size: typeof fileInfo.size === 'number' ? fileInfo.size : 0, type: mimeType,
274-279: Usem.new_content.bodyfor edited quotes, not the oldmessageBody.For edited messages,
messageBodycontains the old content. The raw text should come fromm.new_content.const formatted = await toInternalQuoteMessageFormat({ messageToReplyToUrl, formattedMessage: data.content.formatted_body || '', - rawMessage: messageBody, + rawMessage: data.content['m.new_content']?.body || '', homeServerDomain: serverName, senderExternalId: data.sender, });
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/events/message.ts (2)
111-120: Avoid database queries for disabled feature when threads not in use.When
threadRootEventIdis undefined, this function is called but immediately returns. Consider checking before calling.- const thread = await getThreadMessageId(threadRootEventId); + const thread = threadRootEventId ? await getThreadMessageId(threadRootEventId) : undefined;
332-335: Pass the full thread object tohandleMediaMessagefor consistency.Currently passing
thread?.tmidbut the function signature expects justtmid. This loses thetshowproperty.Update the function to accept the full thread object:
async function handleMediaMessage( // TODO improve typing content: any, msgtype: string, messageBody: string, user: IUser, room: IRoom, eventId: string, - tmid?: string, + thread?: { tmid: string; tshow: boolean }, ): Promise<{ @@ - tmid?: string; + thread?: { tmid: string; tshow: boolean }; @@ - tmid, + thread, @@ if (isMediaMessage && content?.url) { - const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, thread?.tmid); + const result = await handleMediaMessage(content, msgtype, messageBody, user, room, data.event_id, thread);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/server/services/messages/service.ts(1 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)packages/core-services/src/types/IMessageService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core-services/src/types/IMessageService.ts
- apps/meteor/server/services/messages/service.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T22:09:18.041Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-09-16T22:09:18.041Z
Learning: Use Rocket.Chat primary documentation and provided reference files for guidance
Applied to files:
ee/packages/federation-matrix/src/events/message.ts
📚 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:
ee/packages/federation-matrix/src/events/message.ts
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/events/message.ts (4)
packages/core-typings/src/IUser.ts (1)
IUser(186-253)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(34-39)ee/packages/federation-matrix/src/helpers/message.parsers.ts (2)
toInternalQuoteMessageFormat(125-151)toInternalMessageFormat(59-73)
⏰ 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)
ee/packages/federation-matrix/src/events/message.ts (1)
94-102: Good federation subscription handling.The logic properly creates subscriptions for federated users with minimal defaults, avoiding unwanted notifications while ensuring message visibility.
Co-authored-by: Diego Sampaio <chinello@gmail.com> Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Co-authored-by: Diego Sampaio <chinello@gmail.com> Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Works along with #36842.
Summary by CodeRabbit
New Features
Security
Bug Fixes