refactor(federation): add isAuthenticated and canAccessResource middlewares #37067
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds request signature verification and a unified resource-access middleware, removes legacy per-entity middlewares, wires authentication and ACL into federation Matrix routes (invite, media, profiles, send-join, transactions, rooms), passes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Federation Client
participant Router as Matrix Router
participant Auth as isAuthenticatedMiddleware
participant ACL as canAccessResourceMiddleware
participant Handler as Route Handler
Client->>Router: HTTP request (/_matrix/federation/v1/...)
Router->>Auth: verifyRequestSignature(Authorization, method, path, body)
alt verification failed
Auth-->>Client: 401 M_UNAUTHORIZED
else verification success
Auth-->>Router: store authenticatedServer
Router->>ACL: canAccessResource(entityType, resourceId, authenticatedServer)
alt invalid params or access denied
ACL-->>Client: 400 M_INVALID_PARAM or 403 M_FORBIDDEN
else access granted
Router->>Handler: execute handler (invite/media/transactions/...)
Handler-->>Client: 2xx / 4xx / 5xx
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ricardogarim
changed the title
ricardogarim
force-pushed
the
refactor/federation-middlewares
branch
2 times, most recently
from
a9908ec to
05982a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
311-317: Validation mismatch: “event_id” is a query param, not a path param.Both state_ids and state routes declare event_id under params, but you read it from the query. This can cause validation failures or lack of validation where needed.
Minimal fix in route options:
-{ - params: isGetStateIdsParamsProps, +{ + // params: roomId schema (optional to add) + query: isGetStateIdsParamsProps, response: { … } } … -{ - params: isGetStateParamsProps, +{ + // params: roomId schema (optional to add) + query: isGetStateParamsProps, response: { … } }Optionally add a dedicated RoomIdParamsSchema and use it in params for both routes.
Also applies to: 341-347
283-289: Typo in error payload key (“errorcode” → “errcode”).Matrix errors use “errcode”. This response currently returns “errorcode”.
- errorcode: 'M_UNKNOWN', + errcode: 'M_UNKNOWN',ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
139-152: Backoff bug: seconds vs milliseconds mix-up (retries trigger too early and escalate incorrectly).You compute delay in “seconds” but pass ms to the next cycle and seconds to setTimeout. Fix to keep units consistent and cap properly.
async function runWithBackoff(fn: () => Promise<void>, delaySec = 5) { try { await fn(); } catch (e) { - const delay = delaySec === 625 ? 625 : delaySec ** 2; - console.log(`error occurred, retrying in ${delay}ms`, e); - setTimeout(() => { - runWithBackoff(fn, delay * 1000); - }, delay); + const nextDelaySec = delaySec === 625 ? 625 : delaySec ** 2; // 5 -> 25 -> 625 -> 625 ... + console.log(`error occurred, retrying in ${nextDelaySec}s`, e); + setTimeout(() => { + void runWithBackoff(fn, nextDelaySec); + }, nextDelaySec * 1000); } }
🧹 Nitpick comments (11)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (2)
10-13: Confirm body format used for signature verification; handle bad JSON as 400.You parse JSON from a cloned body and pass the object to verifyRequestSignature. If the verifier expects canonical bytes, object reserialization may break signatures; if it expects parsed JSON, this is fine. Also, a JSON parse error currently becomes a 500 (M_UNKNOWN). Consider mapping SyntaxError to 400 (M_BAD_JSON) and passing raw text to the verifier if required.
Example adjustment:
- const body = c.req.raw.body ? await c.req.raw.clone().json() : undefined; + let body: unknown = undefined; + if (c.req.raw.body) { + try { + // If verifier expects raw, prefer: await c.req.text() + body = await c.req.raw.clone().json(); + } catch (e) { + return c.json({ errcode: 'M_BAD_JSON', error: 'Invalid JSON' }, 400); + } + }Please confirm what verifyRequestSignature expects and adjust accordingly.
Also applies to: 24-33
35-36: Type the context variable for safer downstream usage.Set a typed variable on Context to avoid any-typed c.get('authenticatedServer') consumers.
-export const isAuthenticatedMiddleware = (federationAuth: EventAuthorizationService) => - createMiddleware(async (c: Context, next) => { +type Vars = { authenticatedServer: { serverName: string; keyId: string } }; // shape per verifier result +export const isAuthenticatedMiddleware = (federationAuth: EventAuthorizationService) => + createMiddleware<{ Variables: Vars }>(async (c: Context, next) => { … - c.set('authenticatedServer', verificationResult); + c.set('authenticatedServer', verificationResult); return next();ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
31-36: Skip-on-missing-entity is fine; consider explicit allowlist of routes.Skipping when no entity param exists is convenient but can mask missing checks on new routes. Consider an allowlist or a per-route opt-in to make intent explicit.
ee/packages/federation-matrix/src/api/_matrix/media.ts (3)
100-115: Stream large media instead of buffering in memory.Buffering entire files risks high memory usage and GC pressure. Prefer a streaming response (Readable stream) with proper headers; multipart boundaries can be written chunked.
Example (illustrative):
- // TODO: Add file streaming support - const result = await getMediaFile(mediaId, serverName); + // Stream file to avoid buffering large payloads + const result = await MatrixMediaService.getLocalFileForMatrixNode(mediaId, serverName); if (!result) { return { statusCode: 404, body: { errcode: 'M_NOT_FOUND', error: 'Media not found' }, }; } - const { file, buffer } = result; + const file = result; + const fileStream = await MatrixMediaService.getLocalFileReadStream(file); // or equivalent API const mimeType = file.type || 'application/octet-stream'; - const fileName = file.name || mediaId; + const fileName = sanitizeFilename(file.name || mediaId); - const multipartResponse = createMultipartResponse(buffer, mimeType, fileName); + const { stream, contentType } = createMultipartStream(fileStream, mimeType, fileName); return { statusCode: 200, headers: { ...SECURITY_HEADERS, - 'content-type': multipartResponse.contentType, - 'content-length': String(multipartResponse.body.length), + 'content-type': contentType, + // omit content-length to allow chunked transfer when streaming }, - body: multipartResponse.body, + body: stream, };Additional helper signatures (outside diff scope):
function sanitizeFilename(name: string): string { return name.replace(/[\r\n"]/g, '_'); } function createMultipartStream(fileStream: NodeJS.ReadableStream, mime: string, name: string) { /* write preamble and boundary, pipe stream, write closing boundary */ }
103-107: Include security headers on error responses for consistency.Return SECURITY_HEADERS on 404/500 as well.
if (!result) { return { statusCode: 404, + headers: SECURITY_HEADERS, body: { errcode: 'M_NOT_FOUND', error: 'Media not found' }, }; } } catch (error) { return { statusCode: 500, + headers: SECURITY_HEADERS, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' }, }; }Also applies to: 126-129
86-93: Response schema for 200 doesn’t match a binary/multipart body.Using an Ajv “object” schema for a Buffer/multipart is misleading and brittle. Either drop validation for 200 here or model as string with contentEncoding=binary if your tooling supports it.
- 200: isBufferResponseProps, + // 200 is binary/multipart; skip Ajv validation to avoid false positives + // 200: isBinaryResponseProps,ee/packages/federation-matrix/src/api/_matrix/invite.ts (3)
306-313: Validate invite body with a concrete schema and expose 401/403 in spec.Replace the TODO with a schema that matches the handler’s
{ event, room_version }and include 401/403 produced by middlewares.- body: ajv.compile({ type: 'object' }), // TODO: add schema from room package. + body: ajv.compile({ + type: 'object', + properties: { + event: RoomMemberEventSchema, + room_version: { type: 'string' }, + }, + required: ['event', 'room_version'], + }), params: isProcessInviteParamsProps, response: { - 200: isProcessInviteResponseProps, + 200: isProcessInviteResponseProps, + 401: ajv.compile({ type: 'object', properties: { errcode: { type: 'string' }, error: { type: 'string' } }, required: ['errcode','error'] }), + 403: ajv.compile({ type: 'object', properties: { errcode: { type: 'string' }, error: { type: 'string' } }, required: ['errcode','error'] }), },
314-355: Wrap handler in try/catch and return Matrix error shape on failures.Current throws will bubble as generic 500s. Align with media route and middleware error style.
- async (c) => { - const { roomId, eventId } = c.req.param(); - const { event, room_version: roomVersion } = await c.req.json(); - ... - return { body: { event: inviteEvent.event }, statusCode: 200 }; - } + async (c) => { + try { + const { roomId, eventId } = c.req.param(); + const { event, room_version: roomVersion } = await c.req.json(); + ... + return { body: { event: inviteEvent.event }, statusCode: 200 }; + } catch (e) { + return { statusCode: 500, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' } }; + } + }
324-329: Harden Matrix ID parsing
Avoid brittlesplit(':')andslice(1)— explicitly parse and validate the full@user:domainformat. Extract or implement a sharedparseMatrixUserIdhelper that returns validated{ localpart, domain }, then use it here.ee/packages/federation-matrix/src/api/_matrix/send-join.ts (2)
235-242: Expose 401/403 in route response spec.Middlewares can return 401/403; reflect that in the schema.
response: { - 200: isSendJoinResponseProps, + 200: isSendJoinResponseProps, + 401: ajv.compile({ type: 'object', properties: { errcode: { type: 'string' }, error: { type: 'string' } }, required: ['errcode','error'] }), + 403: ajv.compile({ type: 'object', properties: { errcode: { type: 'string' }, error: { type: 'string' } }, required: ['errcode','error'] }), },
243-253: Add try/catch to map unexpected errors to Matrix error response.Keep handler consistent with other routes.
- async (c) => { - const { roomId, stateKey } = c.req.param(); - const body = await c.req.json(); - const response = await sendJoin.sendJoin(roomId, stateKey as EventID, body); - return { body: response, statusCode: 200 }; - } + async (c) => { + try { + const { roomId, stateKey } = c.req.param(); + const body = await c.req.json(); + const response = await sendJoin.sendJoin(roomId, stateKey as EventID, body); + return { body: response, statusCode: 200 }; + } catch (e) { + return { statusCode: 500, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' } }; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
ee/packages/federation-matrix/src/api/_matrix/invite.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(2 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(0 hunks)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares/index.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ee/packages/federation-matrix/src/api/middlewares.ts
🧰 Additional context used
🧬 Code graph analysis (7)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
ee/packages/federation-matrix/src/api/middlewares/index.ts (1)
canAccessResourceMiddleware(5-5)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(23-53)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (3)
ee/packages/federation-matrix/src/api/middlewares/index.ts (2)
isAuthenticatedMiddleware(4-4)canAccessResourceMiddleware(5-5)ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(23-53)
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(23-53)
ee/packages/federation-matrix/src/api/_matrix/send-join.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(23-53)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(23-53)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
ee/packages/federation-matrix/src/api/middlewares/index.ts (1)
isAuthenticatedMiddleware(4-4)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
[error] 38-38: TS2339: Property 'canAccessResource' does not exist on type 'EventAuthorizationService'.
🔇 Additional comments (5)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
349-352: Good middleware order; verify intended coverage for routes without entity params.Auth runs before resource access, which is correct. Note that routes like /v1/query/profile and /v1/user/devices/:userId have no eventId/roomId/mediaId, so canAccessResource will skip checks and only authentication applies. Confirm this matches your security model for these endpoints.
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
261-263: Auth + resource middleware applied correctly at router level.Order is correct. Be aware /v1/send/:txnId has no recognized entity param, so only authentication will run there; resource check will skip.
ee/packages/federation-matrix/src/api/middlewares/index.ts (1)
1-5: Re-export barrel looks good.Centralizing the middleware exports simplifies imports across routes.
ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
229-232: LGTM: middlewares applied at router level.AuthN/AuthZ gating is correctly wired ahead of the handler.
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
79-94: Incorrect resource-authorization concern:mediaIdis already handled.
ThecanAccessResourceMiddlewaremapsmediaIdto type"media"and enforces ACL checks, so no additional media‐specific guard is needed.Likely an incorrect or invalid review comment.
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
Outdated
Show resolved
Hide resolved
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
Outdated
Show resolved
Hide resolved
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
Outdated
Show resolved
Hide resolved
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
Outdated
Show resolved
Hide resolved
ricardogarim
force-pushed
the
refactor/federation-middlewares
branch
from
05ee026 to
f533048
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
110-114: Sanitize filename to prevent header injection.The filename from
file.namecan contain quotes or CR/LF characters that could corrupt MIME headers in the multipart response. This was previously flagged and remains unaddressed.Apply this diff to add sanitization:
+function sanitizeFilename(input: string): string { + return input.replace(/[\r\n"]/g, '_').slice(0, 200); +} + async function getMediaFile(mediaId: string, serverName: string): Promise<{ file: IUpload; buffer: Buffer } | null> {Then update line 111:
const mimeType = file.type || 'application/octet-stream'; -const fileName = file.name || mediaId; +const fileName = sanitizeFilename(file.name || mediaId);
🧹 Nitpick comments (3)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
351-351: Review layered authentication approach.The base router applies
isAuthenticatedMiddleware(federationAuth)(line 351), and specific routes also applycanAccessResourceMiddleware(federationAuth, 'room')(lines 426, 454, 478). SincecanAccessResourceMiddlewareinternally composesisAuthenticatedMiddleware(per relevant code snippets), routes withcanAccessResourceMiddlewarewill run authentication twice. This is redundant but not harmful.Consider either:
- Remove base-level
isAuthenticatedMiddlewareand applycanAccessResourceMiddlewareto all routes that need it- Apply only resource-specific checks to individual routes if base auth is sufficient
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
124-129: Consider logging errors before returning generic error response.The catch block returns a generic
M_UNKNOWNerror without logging the original error. This makes debugging difficult in production. Consider logging the error before returning the response.Apply this diff to add error logging:
} catch (error) { + console.error('Media download error:', error); return { statusCode: 500, body: { errcode: 'M_UNKNOWN', error: 'Internal server error' }, }; }
132-149: Consider using 501 Not Implemented for consistency.The thumbnail endpoint returns 404 to indicate it's not implemented, but 501 (Not Implemented) would be more semantically correct and consistent with the
/v1/user/devices/:userIdendpoint in profiles.ts (line 419).Apply this diff:
async () => ({ - statusCode: 404, + statusCode: 501, body: { errcode: 'M_UNRECOGNIZED', error: 'This endpoint is not implemented on the homeserver side', }, }),
📜 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 (8)
ee/packages/federation-matrix/src/api/_matrix/invite.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(5 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(5 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(0 hunks)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ee/packages/federation-matrix/src/api/middlewares.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/federation-matrix/src/api/_matrix/send-join.ts
- ee/packages/federation-matrix/src/api/_matrix/transactions.ts
- ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
🧰 Additional context used
🧬 Code graph analysis (3)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-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: ✅ Tests Done
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (2)
24-33: LGTM!Signature verification is correctly implemented with proper error handling and Matrix-compliant error responses.
38-38: Manual verification required: confirmerrCodes.M_UNKNOWNshape
Ensure thaterrCodes.M_UNKNOWNprovides botherrcodeanderrorproperties as expected by the JSON response schema; adjust the middleware to wrap or supplement those fields if it does not.ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
15-15: LGTM!The import and service extraction correctly integrate the new middleware infrastructure.
Also applies to: 298-298
303-303: Verify 'room' entity type for invite validation.The middleware uses
'room'as the entity type for access validation. Ensure thatcanAccessResourceproperly validates room access in the invite context. Based on a previous review comment on profiles.ts asking about resource validation, confirm that the authenticated server has permission to invite users to the specified room.ee/packages/federation-matrix/src/api/_matrix/profiles.ts (2)
413-421: LGTM!Returning 501 Not Implemented with
M_UNRECOGNIZEDis appropriate for endpoints that are not yet supported by the homeserver.
426-426: Verify 'room' entity type validation for these endpoints.All three routes use
canAccessResourceMiddleware(federationAuth, 'room'). Confirm that:
make_join- validates the requesting server can participate in the room join flowget_missing_events- validates the requesting server has access to the room's event historyevent_auth- validates the requesting server can query the room's authorization chainThis addresses the previous review comment asking "what resource is being validated here?"
Based on learnings (previous review comment on this file).
Also applies to: 454-454, 478-478
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
9-9: LGTM!Import and service extraction follow the established pattern for middleware integration.
Also applies to: 77-78
79-93: LGTM!Route configuration correctly applies media-specific access control and defines comprehensive response schemas.
ricardogarim
force-pushed
the
refactor/federation-middlewares
branch
from
f533048 to
3c66363
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
110-113: Sanitize filename to prevent header injection.The
file.namevalue is used directly in theContent-Dispositionheader within the multipart response (line 55 increateMultipartResponse). An attacker-controlled filename containing CR/LF (\r\n) or double quotes can inject additional headers or corrupt the MIME structure, potentially leading to security vulnerabilities such as header injection attacks.Apply this diff to sanitize the filename:
- const fileName = file.name || mediaId; + const fileName = sanitizeFilename(file.name || mediaId);Add a helper function at the module level (after imports, before route definitions):
function sanitizeFilename(input: string): string { // Remove CR/LF and dangerous quotes; clamp length return input.replace(/[\r\n"]/g, '_').slice(0, 200); }
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
262-381: Avoid double signature verification in federation transactions
canAccessResourceMiddlewarealready wrapsisAuthenticatedMiddleware, so adding a router-level.use(isAuthenticatedMiddleware(federationAuth))causes every state/event request to verify the Matrix signature twice. That doubles the crypto work on hot paths and adds latency without strengthening security. Consider limitingisAuthenticatedMiddlewareto the/v1/send/:txnIdroute (or introducing a version ofcanAccessResourceMiddlewarethat skips the auth wrapper) so other handlers don't incur the redundant verification.
📜 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 (6)
ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(6 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(5 hunks)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts
🧰 Additional context used
🧬 Code graph analysis (5)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-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 (9)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (4)
5-6: LGTM!The new middleware imports are correctly placed and will be used throughout the file.
348-351: LGTM! Router-level authentication applied correctly.The
isAuthenticatedMiddlewareat the router level ensures all federation endpoints require signature verification before proceeding to handlers. This centralizes authentication and eliminates per-route duplication.
413-420: LGTM! Correct Matrix error format for unimplemented endpoint.The 501 response with
M_UNRECOGNIZEDand a descriptive error message follows Matrix spec conventions for unsupported endpoints.
434-434: LGTM! Room resource access control correctly applied.The
canAccessResourceMiddleware(federationAuth, 'room')middleware correctly validates that the authenticated server has permission to access the specified room resource before executing the handler. This addresses the previous review question about what resource is being validated—theroomIdparameter extracted from the route is checked against the server's access rights.Each of these three endpoints (
make_join,get_missing_events,event_auth) operates on room-scoped data, so the'room'resource type is appropriate.Also applies to: 462-462, 485-485
ee/packages/federation-matrix/src/api/_matrix/media.ts (5)
9-9: LGTM!The middleware import is correctly placed and used later in the file.
78-93: LGTM! Media download route properly secured.The route definition correctly applies
canAccessResourceMiddleware(federationAuth, 'media')to ensure the authenticated server has permission to access the requested media resource. The schema validation and response type definitions are comprehensive.
95-106: LGTM! Proper 404 handling for missing media.The error response correctly returns a 404 status with the Matrix-spec-compliant
M_NOT_FOUNDerror code when media is not found.
115-123: LGTM! Appropriate security headers for media responses.The response includes comprehensive security headers (
X-Content-Type-Options,X-Frame-Options,CSP,HSTS) to mitigate common web vulnerabilities when serving user-uploaded media.
124-129: LGTM! Proper error handling with Matrix-compliant response.The catch block returns a 500 status with the Matrix
M_UNKNOWNerror code, maintaining consistency with the Matrix specification.
| .get( | ||
| '/v1/media/thumbnail/:mediaId', | ||
| { | ||
| params: isMediaDownloadParamsProps, | ||
| response: { | ||
| 404: isErrorResponseProps, | ||
| }, | ||
| tags: ['Federation', 'Media'], | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| return router; | ||
| canAccessResourceMiddleware(federationAuth, 'media'), | ||
| async (_c) => ({ | ||
| statusCode: 404, | ||
| body: { | ||
| errcode: 'M_UNRECOGNIZED', | ||
| error: 'This endpoint is not implemented on the homeserver side', | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Status code mismatch: use 501 for unimplemented endpoints.
The endpoint returns a 404 status code with an error message stating "This endpoint is not implemented on the homeserver side." This is semantically incorrect—404 indicates "resource not found," while 501 indicates "not implemented."
Apply this diff to use the correct status code:
.get(
'/v1/media/thumbnail/:mediaId',
{
params: isMediaDownloadParamsProps,
response: {
- 404: isErrorResponseProps,
+ 501: isErrorResponseProps,
},
tags: ['Federation', 'Media'],
},
canAccessResourceMiddleware(federationAuth, 'media'),
async (_c) => ({
- statusCode: 404,
+ statusCode: 501,
body: {
errcode: 'M_UNRECOGNIZED',
error: 'This endpoint is not implemented on the homeserver side',
},
}),
);📝 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.
| .get( | |
| '/v1/media/thumbnail/:mediaId', | |
| { | |
| params: isMediaDownloadParamsProps, | |
| response: { | |
| 404: isErrorResponseProps, | |
| }, | |
| tags: ['Federation', 'Media'], | |
| }, | |
| }), | |
| ); | |
| return router; | |
| canAccessResourceMiddleware(federationAuth, 'media'), | |
| async (_c) => ({ | |
| statusCode: 404, | |
| body: { | |
| errcode: 'M_UNRECOGNIZED', | |
| error: 'This endpoint is not implemented on the homeserver side', | |
| }, | |
| }), | |
| ); | |
| .get( | |
| '/v1/media/thumbnail/:mediaId', | |
| { | |
| params: isMediaDownloadParamsProps, | |
| response: { | |
| 501: isErrorResponseProps, | |
| }, | |
| tags: ['Federation', 'Media'], | |
| }, | |
| canAccessResourceMiddleware(federationAuth, 'media'), | |
| async (_c) => ({ | |
| statusCode: 501, | |
| body: { | |
| errcode: 'M_UNRECOGNIZED', | |
| error: 'This endpoint is not implemented on the homeserver side', | |
| }, | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/media.ts around lines 132 to
149, the thumbnail route returns a 404 while the handler message says the
endpoint is not implemented; change the response statusCode to 501 and ensure
the returned body remains the same so the HTTP status semantically matches "Not
Implemented" (501) instead of "Not Found" (404).
cc1bbf8 to
3d413f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts (1)
2-2: LGTM! Migration matches the HeaderV2 version.The migration is correct and identical to the one in
apps/meteor/client/views/room/HeaderV2/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts. Both hooks now use the REST endpoint/v1/livechat/inquiries.returnAsInquirywith the proper payload structure.Also applies to: 11-11, 17-17
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
132-149: Use 501 Not Implemented for thumbnail endpoint.Returning 404 with “not implemented” message is semantically wrong. Use 501.
Apply:
- response: { - 404: isErrorResponseProps, + response: { + 501: isErrorResponseProps, }, ... - async (_c) => ({ - statusCode: 404, + async (_c) => ({ + statusCode: 501, body: { errcode: 'M_UNRECOGNIZED', error: 'This endpoint is not implemented on the homeserver side', }, }),
110-114: Sanitize filename to prevent header injection in multipart.file.name can contain quotes or CR/LF. Sanitize or RFC5987-encode before embedding in Content-Disposition.
Apply:
- const fileName = file.name || mediaId; + const fileName = sanitizeFilename(file.name || mediaId);Add helper (outside selected range):
function sanitizeFilename(input: string): string { return input.replace(/[\r\n"]/g, '_').slice(0, 200); }ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
2-72: Fix auth compat, robust ID extraction, and deny-by-default on missing ID.
- EventAuthorizationService may not expose canAccessResource (TS2339). Add runtime fallback to canAccessEvent/Room/Media, deny-by-default.
- In our router, params can be empty in middlewares. Add fallbacks: use body.room_id for 'room' and parse mediaId from URL for 'media'.
- If no ID is determinable, return 400 instead of 500 to avoid accidental allow/deny drift.
Apply:
import { errCodes } from '@rocket.chat/federation-sdk'; -import type { EventAuthorizationService } from '@rocket.chat/federation-sdk'; +import type { EventAuthorizationService } from '@rocket.chat/federation-sdk'; import { every } from 'hono/combine'; import { createMiddleware } from 'hono/factory'; import { isAuthenticatedMiddleware } from './isAuthenticated'; +// Runtime-compatible auth surface (older SDKs may lack canAccessResource) +type ResourceAuth = EventAuthorizationService & { + canAccessResource?: (type: 'event' | 'media' | 'room', id: string, server: unknown) => Promise<boolean>; + canAccessEvent?: (id: string, server: unknown) => Promise<boolean>; + canAccessRoom?: (id: string, server: unknown) => Promise<boolean>; + canAccessMedia?: (id: string, server: unknown) => Promise<boolean>; +}; + function extractEntityId(params: { eventId?: string; mediaId?: string; roomId?: string }, entityType: 'event' | 'media' | 'room'): string { if (entityType === 'room') { const { roomId } = params; if (!roomId) { throw new Error('Room ID is required'); } return roomId; } if (entityType === 'media') { const { mediaId } = params; if (!mediaId) { throw new Error('Media ID is required'); } return mediaId; } if (entityType === 'event') { const { eventId } = params; if (!eventId) { throw new Error('Event ID is required'); } return eventId; } throw new Error('Invalid entity type'); } -const canAccessResource = (federationAuth: EventAuthorizationService, entityType: 'event' | 'media' | 'room') => +const canAccessResource = (federationAuth: ResourceAuth, entityType: 'event' | 'media' | 'room') => createMiddleware(async (c, next) => { try { const authenticatedServer = c.get('authenticatedServer'); if (!authenticatedServer) { return c.json({ errcode: 'M_UNAUTHORIZED', error: 'Missing Authorization header' }, 401); } - const mediaId = c.req.param('mediaId'); - const eventId = c.req.param('eventId'); - const roomId = c.req.param('roomId'); - - const resourceAccess = await federationAuth.canAccessResource( - entityType, - extractEntityId({ mediaId, eventId, roomId }, entityType), - authenticatedServer, - ); + // Params may be empty inside middlewares on this router; add robust extraction. + const paramsAll = (typeof (c.req as any).param === 'function' ? (c.req as any).param() : {}) as + | { mediaId?: string; eventId?: string; roomId?: string; stateKey?: string } + | undefined; + let mediaId = paramsAll?.mediaId; + let eventId = paramsAll?.eventId; + let roomId = paramsAll?.roomId; + + // Fallback to body where applicable + let body: any | undefined; + try { + body = c.req.raw.body ? await c.req.raw.clone().json() : undefined; + } catch { + // ignore body parse errors here + } + if (!roomId) { + roomId = body?.room_id || body?.roomId || body?.event?.room_id; + } + // Fallback for media: parse from URL if missing + if (!mediaId) { + const pathname = new URL(c.req.url).pathname; + const m = pathname.match(/\/media\/(?:download|thumbnail)\/([^/]+)/); + if (m) { + try { + mediaId = decodeURIComponent(m[1]); + } catch { + mediaId = m[1]; + } + } + } + + // Determine resource id based on the entityType + let resourceId: string | undefined; + if (entityType === 'room') resourceId = roomId; + else if (entityType === 'event') resourceId = eventId; + else if (entityType === 'media') resourceId = mediaId; + + if (!resourceId) { + return c.json( + { + errcode: 'M_BAD_JSON', + error: 'Missing resource identifier', + }, + 400, + ); + } + + // Compatibility: prefer canAccessResource, fallback to per-entity methods, deny-by-default + let allowed = false; + if (typeof federationAuth.canAccessResource === 'function') { + allowed = await federationAuth.canAccessResource(entityType, resourceId, authenticatedServer); + } else if (entityType === 'event' && typeof federationAuth.canAccessEvent === 'function') { + allowed = await federationAuth.canAccessEvent(resourceId, authenticatedServer); + } else if (entityType === 'room' && typeof federationAuth.canAccessRoom === 'function') { + allowed = await federationAuth.canAccessRoom(resourceId, authenticatedServer); + } else if (entityType === 'media' && typeof federationAuth.canAccessMedia === 'function') { + allowed = await federationAuth.canAccessMedia(resourceId, authenticatedServer); + } else { + allowed = false; + } + const resourceAccess = allowed; if (!resourceAccess) { return c.json( { errcode: 'M_FORBIDDEN', error: 'Access denied to resource', }, 403, ); } return next(); } catch (error) { return c.json(errCodes.M_UNKNOWN, 500); } }); -export const canAccessResourceMiddleware = (federationAuth: EventAuthorizationService, entityType: 'event' | 'media' | 'room') => +export const canAccessResourceMiddleware = (federationAuth: ResourceAuth, entityType: 'event' | 'media' | 'room') => every(isAuthenticatedMiddleware(federationAuth), canAccessResource(federationAuth, entityType));This also satisfies the suggestion to fail closed when we cannot determine the resource. Based on prior CI failure and reviewer notes.
🧹 Nitpick comments (7)
apps/meteor/client/views/room/HeaderV2/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts (1)
2-2: LGTM! Clean migration from deprecated method to REST endpoint.The changes correctly migrate from the deprecated
livechat:returnAsInquirymethod to the REST endpoint/v1/livechat/inquiries.returnAsInquiry. The payload structure change fromridto{ roomId: rid }aligns with REST API conventions.Note: This hook is duplicated in
apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts. Consider consolidating both into a shared location (e.g.,apps/meteor/client/lib/omnichannel/hooks/) to reduce maintenance burden.Also applies to: 11-11, 17-17
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/ChatInfo.tsx (1)
125-135: Excellent accessibility improvements, but refine list item display.The addition of semantic HTML (
<ul>/<li>) and ARIA attributes (aria-labelledby) significantly improves screen reader support. However, usingdisplay='inline'on list items can degrade accessibility because some screen readers may not properly announce list semantics when items are inline-styled.Apply this diff to use
inline-blockfor better screen reader compatibility:- <ul aria-labelledby={`${roomId}-tags`}> + <ul aria-labelledby={`${roomId}-tags`} style={{ display: 'flex', flexWrap: 'wrap', gap: '4px', listStyle: 'none', padding: 0 }}> {tags.map((tag) => ( - <Box is='li' key={tag} mie={4} display='inline'> - <Tag style={{ display: 'inline' }} disabled> + <Box is='li' key={tag}> + <Tag disabled> {tag} </Tag> </Box>This preserves the inline layout while maintaining list semantics for assistive technologies.
apps/meteor/tests/e2e/page-objects/omnichannel-room-info.ts (1)
56-66: Scope selectors to the Edit Room dialog and harden selectTag against closed dropdownsCurrent selectors search the whole page and can match unrelated elements. Also, selectTag assumes the dropdown is open. Scope to dialog and ensure the listbox is visible before clicking.
Apply:
get inputTags(): Locator { - return this.page.getByRole('textbox', { name: 'Select an option' }); + return this.dialogEditRoom.getByRole('textbox', { name: 'Select an option' }); } optionTags(name: string): Locator { - return this.page.getByRole('option', { name, exact: true }); + return this.page.getByRole('listbox').getByRole('option', { name, exact: true }); } async selectTag(name: string): Promise<void> { - await this.optionTags(name).click(); + const listbox = this.page.getByRole('listbox'); + if (!(await listbox.isVisible())) { + await this.inputTags.click(); + await listbox.waitFor(); + } + await this.optionTags(name).click(); }If the control uses role="combobox" with a different accessible name (e.g., “Tags”), prefer getByRole('combobox', { name: 'Tags' }) for inputTags. As per coding guidelines.
apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts (3)
84-89: Wait for the tag listbox after opening to reduce flakinessAfter clicking the input, wait for the options container to be visible before asserting option visibility.
await poOmnichannel.roomInfo.inputTags.click(); + await expect(poOmnichannel.page.getByRole('listbox')).toBeVisible();As per coding guidelines.
106-110: Ensure multi-select stability and confirm dialog close after SaveIf the dropdown closes after the first selection, the second click may fail. Either re-open before the second selection or rely on a hardened selectTag (see page-object suggestion). Also assert the Edit dialog closes after Save.
await poOmnichannel.roomInfo.selectTag('TagA'); + // If the dropdown closes on selection, re-open: + // await poOmnichannel.roomInfo.inputTags.click(); await poOmnichannel.roomInfo.selectTag('GlobalTag'); await poOmnichannel.roomInfo.btnSaveEditRoom.click(); + await expect(poOmnichannel.roomInfo.dialogEditRoom).not.toBeVisible();
119-125: Repeat the listbox visibility wait for the second scenarioMirror the wait to keep both scenarios equally robust.
await poOmnichannel.roomInfo.inputTags.click(); + await expect(poOmnichannel.page.getByRole('listbox')).toBeVisible();As per coding guidelines.
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
99-114: Consider streaming to reduce memory pressure.Building a single Buffer for large media doubles memory usage. Prefer streaming multipart to the response.
📜 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 (19)
.changeset/calm-hounds-look.md(1 hunks)apps/meteor/app/livechat/imports/server/rest/inquiries.ts(2 hunks)apps/meteor/app/livechat/server/lib/rooms.ts(2 hunks)apps/meteor/app/livechat/server/methods/returnAsInquiry.ts(2 hunks)apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/ChatInfo.tsx(2 hunks)apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts(2 hunks)apps/meteor/client/views/room/HeaderV2/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts(2 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts(1 hunks)apps/meteor/tests/e2e/page-objects/omnichannel-room-info.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/05-inquiries.ts(7 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/media.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(6 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(5 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(0 hunks)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts(1 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ee/packages/federation-matrix/src/api/middlewares.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts
- ee/packages/federation-matrix/src/api/_matrix/profiles.ts
- ee/packages/federation-matrix/src/api/_matrix/transactions.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-room-info.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-room-info.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-room-info.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts
🧬 Code graph analysis (10)
apps/meteor/app/livechat/server/lib/rooms.ts (1)
packages/core-services/src/index.ts (1)
Omnichannel(189-189)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts (3)
apps/meteor/tests/mocks/data.ts (1)
createFakeVisitor(310-315)apps/meteor/tests/e2e/page-objects/home-omnichannel.ts (1)
HomeOmnichannel(14-67)apps/meteor/tests/data/livechat/department.ts (1)
createDepartment(20-33)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)
apps/meteor/tests/end-to-end/api/livechat/05-inquiries.ts (1)
apps/meteor/tests/data/api-data.ts (2)
credentials(39-42)request(10-10)
ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(72-73)
apps/meteor/client/views/room/HeaderV2/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts (2)
packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)apps/meteor/app/livechat/server/methods/returnAsInquiry.ts (1)
rid(18-35)
apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useReturnChatToQueueMutation.ts (2)
packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)apps/meteor/app/livechat/server/methods/returnAsInquiry.ts (1)
rid(18-35)
apps/meteor/app/livechat/imports/server/rest/inquiries.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatInquiriesReturnAsInquirySuccessResponse(3508-3510)isPOSTLivechatInquiriesReturnAsInquiry(3490-3492)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)validateForbiddenErrorResponse(92-92)packages/models/src/index.ts (1)
LivechatRooms(176-176)apps/meteor/app/livechat/server/lib/rooms.ts (1)
returnRoomAsInquiry(212-260)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)
⏰ 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 (8)
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/ChatInfo.tsx (1)
56-56: LGTM!Destructuring
_idasroomIdimproves code clarity.apps/meteor/tests/end-to-end/api/livechat/05-inquiries.ts (3)
7-7: LGTM: Import updates align with REST migration.The removal of
parseMethodResponseand addition ofrequestandcredentialscorrectly reflect the shift from methodCall-based testing to REST endpoint testing.Also applies to: 22-22
327-452: LGTM: REST endpoint migration is complete and consistent.The test suite has been properly migrated from methodCall-based API to REST endpoint usage. All tests maintain their logical intent while using the new API structure:
- Correct HTTP method (POST) and endpoint path (
livechat/inquiries.returnAsInquiry)- Simplified payload structure (
{ roomId }instead of nested RPC-style)- Appropriate HTTP status codes (403 for unauthorized, 400 for validation errors, 200 for success)
- Proper assertions for REST response format
- Consistent error handling across all test cases
The business logic is preserved: returning
success: true, result: falsewhen no agent is serving (line 413-414) andsuccess: true, result: truewhen successfully moved to queue (line 435-436).
510-528: LGTM: Last message verification updated correctly.The test for verifying the correct last message when a room is returned to the queue has been properly updated to use the REST endpoint while maintaining the same validation logic.
apps/meteor/tests/e2e/page-objects/omnichannel-room-info.ts (1)
68-70: LGTM for tag assertionsGood use of role scoping to the Room Information dialog and exact text matching.
Confirm the list has accessible name “Tags” in the DOM to keep this selector stable across themes.
apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts (1)
1-135: Solid coverage and structureGood POM usage, web-first assertions, setup/teardown, and EE-only gating. Naming and steps are clear.
ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
227-241: Verify middleware can read route params in this context.Known issue: in our router, c.req.param() may be empty inside middlewares, causing canAccessResource to fail extracting roomId and return 500/skip checks. Ensure canAccessResource.ts falls back to body.room_id or URL parsing, or set the resource id into c context before this middleware.
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
320-336: Confirm param extraction and target entity.
- Ensure canAccessResource can read roomId inside middleware; otherwise it should fall back to body.event.room_id.
- This route has both roomId and eventId; you’re checking entityType 'room'. Please confirm that is the intended resource for authorization here.
ggazzo
force-pushed
the
chore/fed-branded
branch
from
c1f33ff to
ae1e270
Compare
ricardogarim
force-pushed
the
refactor/federation-middlewares
branch
from
381548e to
18de66e
Compare
ricardogarim
force-pushed
the
refactor/federation-middlewares
branch
2 times, most recently
from
f3e8b32 to
db60d28
Compare
ggazzo
force-pushed
the
chore/fed-branded
branch
2 times, most recently
from
c699bbf to
bc0ad58
Compare
ggazzo
force-pushed
the
chore/fed-branded
branch
8 times, most recently
from
89ebe54 to
94445d8
Compare
ggazzo
force-pushed
the
refactor/federation-middlewares
branch
from
e55889f to
00f58d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
323-323: Redundant authentication on routes with resource-level checks.The router-level
isAuthenticatedMiddleware(line 323) combined with route-levelcanAccessResourceMiddleware(lines 378, 409, 441, 476) causes duplicate authentication. According to the middleware implementation,canAccessResourceMiddlewareinternally includesisAuthenticatedMiddleware, so authentication runs twice for these routes—once at the router level and once within the resource middleware.Consider refactoring to avoid duplicate verification:
Option 1 (Recommended): Remove router-level middleware, apply explicitly per route
return ( new Router('/federation') - .use(isAuthenticatedMiddleware(federationAuth)) .put( '/v1/send/:txnId', { params: isSendTransactionParamsProps, body: isSendTransactionBodyProps, response: { }, tags: ['Federation'], license: ['federation'], }, + isAuthenticatedMiddleware(federationAuth), async (c) => {This ensures each route has authentication applied exactly once.
Option 2: Keep router-level middleware if a separate
canAccessResourcemiddleware existsIf the codebase provides a
canAccessResourcemiddleware (without embedded authentication), use that on GET routes instead ofcanAccessResourceMiddleware. However, this option requires changes outside the current file.Also applies to: 378-378, 409-409, 441-441, 476-476
📜 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)
ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)ee/packages/federation-matrix/src/api/_matrix/rooms.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/src/api/_matrix/invite.ts
🧰 Additional context used
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/api/_matrix/rooms.ts (1)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(61-62)
⏰ 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/api/_matrix/rooms.ts (1)
5-6: LGTM! Clean middleware integration.The authentication middleware is correctly applied at the router level, ensuring all public room endpoints require valid request signatures.
Also applies to: 127-130
ricardogarim
force-pushed
the
refactor/federation-middlewares
branch
2 times, most recently
from
a4d77a4 to
89df936
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ee/packages/federation-matrix/src/api/_matrix/invite.ts(5 hunks)ee/packages/federation-matrix/src/api/_matrix/rooms.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/packages/federation-matrix/src/api/_matrix/invite.ts
- ee/packages/federation-matrix/src/api/_matrix/rooms.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(6-40)ee/packages/federation-matrix/src/api/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(61-62)
⏰ 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
sampaiodiego
force-pushed
the
refactor/federation-middlewares
branch
from
89df936 to
165f855
Compare
sampaiodiego
force-pushed
the
refactor/federation-middlewares
branch
from
165f855 to
b00fa81
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37067 +/- ##
==================================================
- Coverage 66.37% 66.36% -0.02%
==================================================
Files 3386 3386
Lines 115619 115619
Branches 21351 21357 +6
==================================================
- Hits 76739 76725 -14
- Misses 36275 36285 +10
- Partials 2605 2609 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
As per FDR-159, this PR introduces two new middlewares for federation routes:
isAuthenticatedMiddleware./_matrix/federation/v1/event_auth/{roomId}/{eventId}/_matrix/federation/v1/backfill/{roomId}/_matrix/federation/v1/get_missing_events/{roomId}/_matrix/federation/v1/event/{eventId}/_matrix/federation/v1/make_join/{roomId}/{stateKey}/_matrix/federation/v1/send_join/{roomId}/{stateKey}/_matrix/federation/v2/invite/{roomId}/{eventId}/_matrix/federation/v1/publicRooms/_matrix/federation/v1/publicRooms/_matrix/federation/v1/query/profile/_matrix/federation/v1/user/devices/{userId}/_matrix/federation/v1/user/keys/query/_matrix/federation/v1/media/download/{mediaId}/_matrix/federation/v1/send/{txnId}/.well-known/matrix/server/_matrix/federation/v1/version* Note: The Server Access Control Lists (ACLs) section doesn’t explicitly mention enforcing ACLs on media, but we understand it as necessary and are applying it accordingly.
** Note: MP suggests handling ACLs and resource access per PDU/EDU internally rather than through middleware. We're delegating this responsibility to state resolution. TODO: Verify that all scenarios are properly covered.
*** Note: We're dealing with ACL on the state instead of a middleware. TODO: Verify that all scenarios are properly covered.
RC = Rocket.Chat; HS = Homeserver; MP = Matrix Protocol.
Relates to RocketChat/homeserver#224.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor