refactor: adds authentication to media features #184
Conversation
WalkthroughAdds a multipart-aware typed fetch API and reworks federation request flow to return FetchResponse objects; introduces Upload and MatrixBridgedRoom repositories and media access authorization methods; replaces homeserver media controller with federation-scoped stub routes; adjusts staging message content spread and refines signature verification error text. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HS as Homeserver
participant FRS as FederationRequestService
participant CF as core.fetch<T>()
participant RS as Remote Server
rect rgba(220,235,255,0.25)
note right of HS: Binary media download flow (requestBinaryData)
HS->>FRS: requestBinaryData(GET, server, endpoint)
FRS->>CF: fetch<T>(URL, signed headers)
CF->>RS: HTTPS request
RS-->>CF: Response (status, headers, body)
CF-->>FRS: FetchResponse (ok,status,headers)
FRS->>FRS: response.multipart() / .buffer()
alt multipart contains CDN redirect
FRS->>CF: follow redirect (no Matrix auth)
CF->>RS: GET redirect URL
RS-->>CF: Final content
CF-->>FRS: FetchResponse buffer
end
FRS-->>HS: Buffer (binary)
end
sequenceDiagram
autonumber
participant Client as Caller
participant AS as EventAuthorizationService
participant UR as UploadRepository
participant BR as MatrixBridgedRoomRepository
participant SS as StateService
participant Core as Core Auth Utils
participant KR as KeyRepository
rect rgba(235,255,235,0.25)
Client->>AS: canAccessMediaFromAuthorizationHeader(mediaId, authHeader, ...)
AS->>Core: extractSignaturesFromHeader(authHeader)
AS->>KR: get cached key (or fetch)
AS->>Core: validateAuthorizationHeader(...signature...)
Core-->>AS: origin (or throws)
AS->>UR: findRocketChatRoomIdByMediaId(mediaId)
AS->>BR: findMatrixRoomId(rid)
AS->>SS: checkServerAcl/membership/history_visibility
alt allowed
AS-->>Client: { authorized: true }
else denied
AS-->>Client: { authorized: false, errorCode: ... }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
- Coverage 81.10% 81.01% -0.10%
==========================================
Files 63 63
Lines 4683 4692 +9
==========================================
+ Hits 3798 3801 +3
- Misses 885 891 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/homeserver/src/homeserver.module.ts (1)
72-73: Bug: MEDIA_ENABLE_THUMBNAILS env is ignored (always true).
=== 'true' || trueevaluates to true regardless of the env. Respect the env flag.Apply:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: + process.env.MEDIA_ENABLE_THUMBNAILS !== undefined + ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true' + : true,packages/federation-sdk/src/services/staging-area.service.ts (1)
506-509: Runtime error: Set.prototype.difference does not exist in JS.This will throw at runtime. Implement difference manually.
- const setOrUnsetPowerLevels = new Set( - usersInNewPowerLevelEvent, - ).difference(new Set(usersInOldPowerLevelEvent)); + const setOrUnsetPowerLevels = new Set<string>([ + // newly set + ...usersInNewPowerLevelEvent.filter( + (u) => !usersInOldPowerLevelEvent.includes(u), + ), + // unset + ...usersInOldPowerLevelEvent.filter( + (u) => !usersInNewPowerLevelEvent.includes(u), + ), + ]);packages/core/src/utils/fetch.ts (1)
90-93: SNI servername derivation breaks if Host is lowercase or absent.Headers may be lowercase; also fallback to URL hostname if Host missing.
-export async function fetch<T>(url: URL, options: RequestInit) { - const serverName = new URL( - `http://${(options.headers as OutgoingHttpHeaders).Host}` as string, - ).hostname; +export async function fetch<T>(url: URL, options: RequestInit) { + const headers = + (Array.isArray(options.headers) + ? Object.fromEntries(options.headers as [string, string][]) + : (options.headers as Record<string, string | number> | undefined)) || {}; + const hostHeader = + (headers['host'] as string) || + (headers['Host'] as string) || + (headers[':authority'] as string) || + ''; + const serverName = hostHeader ? new URL(`http://${hostHeader}`).hostname : url.hostname;packages/federation-sdk/src/services/federation-request.service.ts (1)
106-111: CI failure: response.multipart is not a function — wrong fetch imported.
The failing checks indicate the custom fetch wrapper (which provides multipart()) isn’t being used. Import it directly from '@hs/core/utils/fetch' and update call sites.Apply this diff:
@@ -import { - EncryptionValidAlgorithm, - authorizationHeaders, - computeAndMergeHash, - createLogger, - extractURIfromURL, - fetch, - signJson, -} from '@hs/core'; +import { + EncryptionValidAlgorithm, + authorizationHeaders, + computeAndMergeHash, + createLogger, + extractURIfromURL, + signJson, +} from '@hs/core'; +import { fetch as hsFetch } from '@hs/core/utils/fetch'; @@ - const response = await fetch<T>(url, { + const response = await hsFetch<T>(url, { method, ...(signedBody && { body: JSON.stringify(signedBody) }), headers, });
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/media.service.ts (1)
12-37: Stream instead of buffering entire files in memory.
At least add a TODO; ideally return a stream/pipe to the caller to avoid large buffers.- async downloadFromRemoteServer( + // TODO: stream result instead of buffering entire file in memory + async downloadFromRemoteServer(
🧹 Nitpick comments (15)
packages/homeserver/src/homeserver.module.ts (3)
61-71: Harden MIME types parsing (avoid empty entries).Empty env or stray commas will inject empty strings into the list.
Use trimming + filtering:
- allowedMimeTypes: process.env.MEDIA_ALLOWED_MIME_TYPES?.split(',') || [ + allowedMimeTypes: process.env.MEDIA_ALLOWED_MIME_TYPES + ? process.env.MEDIA_ALLOWED_MIME_TYPES.split(',').map((s) => s.trim()).filter(Boolean) + : [ 'image/jpeg', 'image/png', 'image/gif', 'image/webp', 'text/plain', 'application/pdf', 'video/mp4', 'audio/mpeg', 'audio/ogg', - ], + ],
94-107: Consider disabling Swagger in production.Reduce attack surface and overhead by enabling it conditionally.
If acceptable, move this
.use(swagger(...))into:if (process.env.NODE_ENV !== 'production') { app.use(swagger({ documentation: { /* ... */ }})); }
120-120: Media endpoints removal: verify upstream routing and docs.With media controller removed, ensure reverse proxies route media to Rocket.Chat, and 404s are expected on homeserver. Update deployment docs accordingly.
packages/federation-sdk/src/services/staging-area.service.ts (2)
359-372: Broad content spread may change downstream payload contract.Spreading
...stagedEvent.event.contentnow forwards additional keys; verify consumers tolerate extra fields.If you need a strict payload, pick only allowed keys:
- content: { - ...stagedEvent.event.content, - body: stagedEvent.event.content?.body as string, - msgtype: stagedEvent.event.content?.msgtype as string, - 'm.relates_to': stagedEvent.event.content?.['m.relates_to'] as { ... }, - 'm.new_content': stagedEvent.event.content?.['m.new_content'] as { ... }, - formatted_body: (stagedEvent.event.content?.formatted_body || '') as string, - }, + content: { + body: stagedEvent.event.content?.body as string, + msgtype: stagedEvent.event.content?.msgtype as string, + 'm.relates_to': stagedEvent.event.content?.['m.relates_to'] as { ... }, + 'm.new_content': stagedEvent.event.content?.['m.new_content'] as { ... }, + formatted_body: (stagedEvent.event.content?.formatted_body || '') as string, + },
287-287: Replace console.log with logger.Keep logging consistent.
- console.log('Skipping persistence stage, persisted in previous stage'); // TODO: revisit + this.logger.debug('Skipping persistence stage, persisted in previous stage'); // TODO: revisitpackages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
17-23: Add projection and index for efficiency.Only
mriis needed andridshould be indexed.- const bridgedRoom = await this.collection.findOne({ - rid: rocketChatRoomId, - }); + const bridgedRoom = await this.collection.findOne( + { rid: rocketChatRoomId }, + { projection: { mri: 1, _id: 0 } }, + );Outside this file, ensure index:
await collection.createIndex({ rid: 1 }, { name: 'rid_1' });packages/federation-sdk/src/repositories/upload.repository.ts (1)
19-25: Add projection and index on federation.mediaId.Speeds up lookups and reduces I/O.
- const upload = await this.collection.findOne({ - 'federation.mediaId': mediaId, - }); + const upload = await this.collection.findOne( + { 'federation.mediaId': mediaId }, + { projection: { rid: 1, _id: 0 } }, + );Outside this file, ensure index:
await collection.createIndex({ 'federation.mediaId': 1 }, { name: 'federation_mediaId_1', unique: true });packages/core/src/utils/fetch.ts (2)
6-11: Type the fetch response for better DX.Expose a concrete return type and keep generics on
json<T>().export type MultipartResult = { content: Buffer; headers?: Record<string, string>; redirect?: string; }; +export type FetchResponse<T> = { + ok: boolean; + status: number | undefined; + headers: OutgoingHttpHeaders; + buffer: () => Promise<Buffer>; + json: () => Promise<T | null>; + text: () => Promise<string>; + multipart: () => Promise<MultipartResult | null>; +}; @@ -export async function fetch<T>(url: URL, options: RequestInit) { +export async function fetch<T>(url: URL, options: RequestInit): Promise<FetchResponse<T>> { @@ - return { + return { ok: response.statusCode ? response.statusCode >= 200 && response.statusCode < 300 : false, buffer: () => Promise.resolve(response.body), json: () => handleJson<T>(contentType, response.body), text: () => handleText(contentType, response.body), multipart: () => handleMultipart(contentType, response.body), status: response.statusCode, headers: response.headers, };Also applies to: 88-88, 134-144
16-47: Multipart parser is fragile for large/binary parts.String-splitting the full body risks high memory and boundary collisions. Consider a Buffer-first parser that scans for boundaries and only stringifies headers.
I can provide a streaming-safe parser if needed.
packages/federation-sdk/src/container.ts (1)
89-101: Add DB indexes for new collections to avoid full scans.
Both lookups will be hot paths; create supporting indexes:
- rocketchat_uploads: federation.mediaId (unique, sparse)
- rocketchat_matrix_bridged_rooms: rid (unique), mri (unique)
Consider a lightweight migration to enforce these.
+// Migration (pseudo): +db.rocketchat_uploads.createIndex({ 'federation.mediaId': 1 }, { unique: true, sparse: true }); +db.rocketchat_matrix_bridged_rooms.createIndex({ rid: 1 }, { unique: true }); +db.rocketchat_matrix_bridged_rooms.createIndex({ mri: 1 }, { unique: true });packages/federation-sdk/src/services/federation-request.service.ts (3)
125-132: Be defensive if multipart isn’t available.
Even with the correct import, add a fallback for robustness.- const multipart = await response.multipart(); - if (multipart?.redirect) { + const hasMultipart = typeof (response as any).multipart === 'function'; + const multipart = hasMultipart ? await (response as any).multipart() : null; + if (multipart?.redirect) { return this.handleRedirect(multipart.redirect); } - if (multipart !== null) { + if (multipart !== null) { return multipart.content; }
200-214: Narrow method type and reuse query builder.
- Constrain method to HttpMethod.
- Reuse the same URLSearchParams flow for consistency.
- async requestBinaryData( - method: string, + async requestBinaryData( + method: HttpMethod, targetServer: string, endpoint: string, queryParams?: Record<string, string>, ) { - return this.makeSignedRequest({ + return this.makeSignedRequest({ method, domain: targetServer, uri: endpoint, - queryString: queryParams - ? new URLSearchParams(queryParams).toString() - : '', + queryString: queryParams ? new URLSearchParams(queryParams).toString() : '', }) as Promise<Buffer>; }
135-139: Error logging is clear — retain stack when available.
Optionally include err.stack for deeper triage in debug builds.packages/federation-sdk/src/services/event-authorization.service.ts (1)
369-425: Media access decisions: differentiate NotFound vs Forbidden.
Currently “not found” maps to false and later becomes M_FORBIDDEN. Consider returning a distinct “M_NOT_FOUND” to reduce leakage while keeping accurate semantics.- async canAccessMedia(mediaId: string, serverName: string): Promise<boolean> { + async canAccessMedia(mediaId: string, serverName: string): Promise<boolean> { // or return a tri-state enumOperational: ensure unique indexes on federation.mediaId (uploads) and room mappings as suggested in container.ts review.
packages/federation-sdk/src/services/media.service.ts (1)
1-1: Remove unused https import.
Lint will flag this; safe to delete.-import https from 'node:https';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/core/src/utils/authentication.ts(1 hunks)packages/core/src/utils/fetch.ts(2 hunks)packages/federation-sdk/src/container.ts(2 hunks)packages/federation-sdk/src/index.ts(3 hunks)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts(1 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(6 hunks)packages/federation-sdk/src/services/federation-request.service.ts(8 hunks)packages/federation-sdk/src/services/media.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/media.controller.ts(0 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/media.controller.ts
🧰 Additional context used
🧬 Code graph analysis (9)
packages/federation-sdk/src/container.ts (2)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
Upload(4-11)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
MatrixBridgedRoom(4-8)
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (2)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
singleton(13-26)packages/federation-sdk/src/services/event-authorization.service.ts (1)
singleton(16-476)
packages/federation-sdk/src/repositories/upload.repository.ts (2)
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
singleton(10-24)packages/federation-sdk/src/services/event-authorization.service.ts (1)
singleton(16-476)
packages/homeserver/src/homeserver.module.ts (1)
packages/homeserver/src/controllers/federation/rooms.controller.ts (1)
roomPlugin(5-117)
packages/core/src/utils/authentication.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
origin(82-84)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/core/src/utils/authentication.ts (1)
validateAuthorizationHeader(72-109)packages/federation-sdk/src/services/config.service.ts (1)
serverName(92-94)
packages/federation-sdk/src/services/federation-request.service.ts (1)
packages/core/src/utils/fetch.ts (1)
fetch(89-157)
packages/core/src/utils/fetch.ts (2)
packages/core/src/index.ts (1)
fetch(89-89)packages/federation-sdk/src/services/federation-request.service.ts (1)
request(142-166)
packages/federation-sdk/src/services/media.service.ts (2)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-215)packages/federation-sdk/src/services/config.service.ts (1)
serverName(92-94)
🪛 GitHub Check: Code Quality Checks(lint, test, tsc)
packages/federation-sdk/src/services/federation-request.service.ts
[failure] 125-125: TypeError: response.multipart is not a function. (In 'response.multipart()'
at makeSignedRequest (/home/runner/work/homeserver/homeserver/packages/federation-sdk/src/services/federation-request.service.ts:125:37)
[failure] 125-125: TypeError: response.multipart is not a function. (In 'response.multipart()'
at makeSignedRequest (/home/runner/work/homeserver/homeserver/packages/federation-sdk/src/services/federation-request.service.ts:125:37)
[failure] 125-125: TypeError: response.multipart is not a function. (In 'response.multipart()'
at makeSignedRequest (/home/runner/work/homeserver/homeserver/packages/federation-sdk/src/services/federation-request.service.ts:125:37)
🔇 Additional comments (10)
packages/federation-sdk/src/container.ts (2)
15-23: New repositories wired into DI — looks good.
Imports for MatrixBridgedRoom and Upload repos/types are correct and consistent with usage elsewhere.
107-109: Injection tokens verified — no changes required. UploadRepository uses @Inject('UploadCollection') (packages/federation-sdk/src/repositories/upload.repository.ts:16) and MatrixBridgedRoomRepository uses @Inject('MatrixBridgedRoomCollection') (packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts:13); keep the container.registerSingleton(...) calls as-is.packages/federation-sdk/src/index.ts (3)
49-49: Expose MediaService from SDK — OK.
Export aligns with DI container registration.
221-237: Service wiring — OK.
media resolved via container; consistent with other services.
97-102: Public API surface changed (HomeserverServices.media). Contract change — ensure dependents compile and update call sites. Verification not completed here (ripgrep returned no searchable files); confirm locally with: rg -n '\bHomeserverServices\b' && rg -n '.media\b'.packages/federation-sdk/src/services/federation-request.service.ts (2)
100-104: Minor: set Content-Type only when body is present — OK.
Header merge is correct and won’t clobber discovery headers.
47-54: Narrow the return type for JSON callers or split the APImakeSignedRequest currently returns Promise<T | Buffer | null> — confirm all callers that expect a JSON T-only result.
- Search found only internal callers + tests: definition at packages/federation-sdk/src/services/federation-request.service.ts (line ~47); callers at lines 159 and 206; many tests in packages/federation-sdk/src/services/federation-request.service.spec.ts. Also a similarly named util at packages/core/src/utils/makeRequest.ts:8 — ensure no external consumers rely on the union return.
- Options: add overloads to preserve Promise for JSON callers, or split into makeSignedJsonRequest(): Promise and makeSignedRequestBinary(): Promise (preferred).
- If keeping the union, add explicit runtime guards and update caller signatures where T is assumed.
packages/federation-sdk/src/services/event-authorization.service.ts (3)
154-164: Destination fallback fix — good catch.
Using destination || serverName aligns with canonical JSON validation.
427-475: Authorization-from-header flow — looks solid.
Signature verification + ACL + membership/world_readable checks are consistent with event path.
292-346: ACL semantics confirmed — empty allow list denies all (Matrix spec).
Verified: the Matrix spec treats an omitted/empty "allow" as an empty list (disallowing all servers); the current check (if allow.length === 0 => deny) matches the spec.
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/federation-sdk/src/services/media.service.ts (2)
11-11: Stream instead of buffering entire files; add TODO now if deferring.
Current API returns Buffer, which can blow memory with large media. Prefer a streaming variant (Readable stream) and piping to the caller; at minimum, add a TODO here.Minimal placeholder:
async downloadFromRemoteServer( serverName: string, mediaId: string, ): Promise<Buffer> { + // TODO: Stream/pipe media instead of buffering into memory (requires a streaming method in FederationRequestService).If helpful, I can sketch a
requestBinaryStream()in FederationRequestService and a correspondingdownloadStream()here.
11-15: Critical: missing await prevents endpoint fallback; return type shouldn’t include null.
The try/catch won’t catch async rejections, so the first failed request rejects the method. Also, the method never returns null; it throws on failure. This mirrors a prior review—resurfacing here.Apply:
- async downloadFromRemoteServer( + async downloadFromRemoteServer( serverName: string, mediaId: string, - ): Promise<Buffer | null> { + ): Promise<Buffer> { @@ - for (const endpoint of endpoints) { - try { - return this.federationRequest.requestBinaryData( - 'GET', - serverName, - endpoint, - ); - } catch (err) { + let lastErr: unknown; + for (const endpoint of endpoints) { + try { + const data = await this.federationRequest.requestBinaryData( + 'GET', + serverName, + endpoint, + ); + return data; + } catch (err) { + lastErr = err; this.logger.debug( `Endpoint ${endpoint} failed: ${err instanceof Error ? err.message : String(err)}`, ); } } @@ - throw new Error(`Failed to download media ${mediaId} from ${serverName}`); + throw new Error( + `Failed to download media ${mediaId} from ${serverName}${ + lastErr instanceof Error ? `: ${lastErr.message}` : '' + }`, + );Also applies to: 21-28, 35-35
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/media.service.ts (3)
15-19: Endpoint set: OK to prefer federation v1; r0/v3 fallbacks are legacy-only.
Matrix 1.11 introduced authenticated S2S media endpoints: GET /_matrix/federation/v1/media/download/{mediaId}. Client endpoints moved to /_matrix/client/v1, while /_matrix/media/{v3|r0} are deprecated. Keeping v3/r0 as last-resort fallbacks helps older homeservers, but note they’re unauthenticated and may be disabled on modern servers. The new /client/v1 endpoints require user access tokens and aren’t suitable for S2S. Consider guarding legacy fallbacks by config or feature flag. (spec.matrix.org)
21-32: Optional: add timeout and input sanity checks.
- Pass
timeout_msfor the federation call (spec default is 20s) if you want predictable behavior under load.- Validate
serverNameandmediaId(non-empty, no whitespace) before building URLs to reduce bad-request noise.
11-36: Tests: cover fallback and failure paths.
Add unit tests where the first endpoint rejects and the second succeeds, and one where all fail (assert thrown error message). This will also raise patch coverage.I can generate Jest tests stubbing FederationRequestService to simulate per-endpoint outcomes—want me to push a test PR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/media.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/media.service.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-215)
🔇 Additional comments (2)
packages/federation-sdk/src/services/media.service.ts (2)
3-3: LGTM: correct dependency import.
No issues with switching to FederationRequestService.
9-9: LGTM: DI via tsyringe is appropriate.
Constructor injection keeps the service testable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/federation-sdk/src/services/federation-request.service.spec.ts (3)
195-207: Fix false-positive error test: stub core.fetch and assert rejectionBecause
@hs/core.fetchis module-mocked earlier, overridingglobalThis.fetchwon’t affectmakeSignedRequest; this test can pass without asserting anything if no error is thrown. Replace the global fetch hack with acore.fetchstub and assert rejection viaexpect(...).rejects. Note:mock.restore()inafterEachdoes not resetmock.module()overrides, so the module mock persists.Apply:
- globalThis.fetch = Object.assign( - async () => { - return { - ok: false, - status: 404, - text: async () => 'Not Found', - multipart: async () => null, - } as Response; - }, - { preconnect: () => {} }, - ) as typeof fetch; - - try { - await service.makeSignedRequest({ - method: 'GET', - domain: 'target.example.com', - uri: '/test/path', - }); - } catch (error: unknown) { - if (error instanceof Error) { - expect(error.message).toContain( - 'Federation request failed: 404 Not Found', - ); - } else { - throw error; - } - } + spyOn(core, 'fetch').mockImplementation(async () => ({ + ok: false, + status: 404, + text: async () => 'Not Found', + multipart: async () => null, + } as Response)); + + await expect( + service.makeSignedRequest({ + method: 'GET', + domain: 'target.example.com', + uri: '/test/path', + }), + ).rejects.toThrow('Federation request failed: 404 Not Found');Also applies to: 208-223
226-238: Same issue: assert JSON error via rejects and stub core.fetchMirror the approach above to ensure the test actually fails if no error is thrown. Use
expect(...).rejectsinstead of try/catch.- globalThis.fetch = Object.assign( - async () => { - return { - ok: false, - status: 400, - text: async () => - '{"error":"Bad Request","code":"M_INVALID_PARAM"}', - multipart: async () => null, - } as Response; - }, - { preconnect: () => {} }, - ) as typeof fetch; - - try { - await service.makeSignedRequest({ - method: 'GET', - domain: 'target.example.com', - uri: '/test/path', - }); - } catch (error: unknown) { - if (error instanceof Error) { - expect(error.message).toContain( - 'Federation request failed: 400 {"error":"Bad Request","code":"M_INVALID_PARAM"}', - ); - } else { - throw error; - } - } + spyOn(core, 'fetch').mockImplementation(async () => ({ + ok: false, + status: 400, + text: async () => '{"error":"Bad Request","code":"M_INVALID_PARAM"}', + multipart: async () => null, + } as Response)); + + await expect( + service.makeSignedRequest({ + method: 'GET', + domain: 'target.example.com', + uri: '/test/path', + }), + ).rejects.toThrow( + 'Federation request failed: 400 {"error":"Bad Request","code":"M_INVALID_PARAM"}', + );Also applies to: 239-254
256-263: Same issue: network error test should stub core.fetch and use rejectsStub
core.fetchto reject and assert viarejects.toThrow.- globalThis.fetch = Object.assign( - async () => { - throw new Error('Network Error'); - }, - { preconnect: () => {} }, - ) as typeof fetch; - - try { - await service.makeSignedRequest({ - method: 'GET', - domain: 'target.example.com', - uri: '/test/path', - }); - } catch (error: unknown) { - if (error instanceof Error) { - expect(error.message).toBe('Network Error'); - } else { - throw error; - } - } + spyOn(core, 'fetch').mockImplementation(async () => { + throw new Error('Network Error'); + }); + + await expect( + service.makeSignedRequest({ + method: 'GET', + domain: 'target.example.com', + uri: '/test/path', + }), + ).rejects.toThrow('Network Error');Also applies to: 264-277
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/federation-request.service.spec.ts (2)
337-383: Strengthen requestBinaryData tests (buffer type + resilient query expectations)
- Assert the result is a Buffer.
- Don’t rely on query param ordering; assert presence of each pair.
expect(makeSignedRequestSpy).toHaveBeenCalledWith({ method: 'GET', domain: 'target.example.com', uri: '/media/download', queryString: '', }); - expect(result).toEqual(mockBuffer); + expect(Buffer.isBuffer(result)).toBe(true); + expect(result).toEqual(mockBuffer); @@ - expect(makeSignedRequestSpy).toHaveBeenCalledWith({ - method: 'GET', - domain: 'target.example.com', - uri: '/media/download', - queryString: 'width=100&height=100', - }); - expect(result).toEqual(mockBuffer); + expect(makeSignedRequestSpy).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'GET', + domain: 'target.example.com', + uri: '/media/download', + queryString: expect.stringContaining('width=100'), + }), + ); + expect(makeSignedRequestSpy).toHaveBeenCalledWith( + expect.objectContaining({ + queryString: expect.stringContaining('height=100'), + }), + ); + expect(Buffer.isBuffer(result)).toBe(true); + expect(result).toEqual(mockBuffer);
114-120: Remove duplicated config assertionsThese three expectations repeat the ones just above; trimming reduces noise.
- expect(configService.serverName).toBe(mockServerName); - expect(await configService.getSigningKeyBase64()).toBe(mockSigningKey); - expect(await configService.getSigningKeyId()).toBe(mockSigningKeyId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/federation-request.service.spec.ts(4 hunks)
🔇 Additional comments (3)
packages/federation-sdk/src/services/federation-request.service.spec.ts (3)
53-54: Good: mock Response.multipart is presentAdding
multipart: async () => nullprevents undefined-access in code paths expectingresponse.multipart().
202-204: Good: multipart stub included for error ResponseKeeping
multiparton error responses avoids test-time type/shape mismatches when code probes it.
233-235: Good: multipart stub included for JSON error ResponseConsistent with the fetch flow; prevents undefined access.
ricardogarim
force-pushed
the
refactor/media-service
branch
from
6d70a69 to
5816e69
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)
packages/federation-sdk/src/services/federation-request.service.ts (1)
159-166: Type safety: avoid casting away Buffer/null union.makeSignedRequest returns T | Buffer | null; forcing cast to Promise risks runtime type confusion. Consider splitting APIs (e.g., makeJsonRequest() and makeBinaryRequest()) or overloads keyed by an option.
packages/federation-sdk/src/services/staging-area.service.ts (1)
293-316: Bug: non-standard Set.difference and missing “unset” case (will throw at runtime)
Set.prototype.differenceis not standard → TypeError. Additionally, the logic should handle both users added and removed (symmetric difference).Apply this diff:
- const setOrUnsetPowerLevels = new Set( - usersInNewPowerLevelEvent, - ).difference(new Set(usersInOldPowerLevelEvent)); + const oldSet = new Set(usersInOldPowerLevelEvent); + const newSet = new Set(usersInNewPowerLevelEvent); + // symmetric difference: users added or removed + const setOrUnsetPowerLevels = new Set( + [...oldSet, ...newSet].filter((u) => oldSet.has(u) !== newSet.has(u)), + ); @@ - for (const userId of setOrUnsetPowerLevels) { - const newPowerLevel = changedUserPowers[userId]; // if unset, it's 0, if set, it's the power level + for (const userId of setOrUnsetPowerLevels) { + const newPowerLevel = (changedUserPowers as Record<string, number | undefined>)[userId] ?? 0; this.logger.debug( `Emitting event for ${userId} with new power level ${newPowerLevel ?? 0}`, ); this.eventEmitterService.emit('homeserver.matrix.room.role', { sender_id: event.event.sender, user_id: userId, room_id: roomId, role: getRole(newPowerLevel), }); }Please add/adjust unit tests to cover both “set” and “unset” scenarios.
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/federation-request.service.ts (2)
106-111: Timeouts/max body size still missing in core fetch.Calls here will inherit unbounded buffering and no timeouts from @hs/core fetch; please add/connect sane defaults there.
Would you like me to open a follow‑up PR to core’s fetch with connect/read timeouts and a maxBodySize guard?
32-45: Redirect fetch: enforce HTTPS and set Host header for SNI.Without a Host header, TLS SNI may fail (“undefined”) in core fetch; also allow‑listing only https prevents unsafe schemes.
Apply:
- private async handleRedirect(redirect: string): Promise<Buffer> { - const redirectResponse = await fetch(new URL(redirect), { - method: 'GET', - headers: {}, - }); + private async handleRedirect(redirect: string): Promise<Buffer> { + const u = new URL(redirect); + if (u.protocol !== 'https:') { + throw new Error(`Rejected non-HTTPS redirect: ${redirect}`); + } + const redirectResponse = await fetch(u, { + method: 'GET', + headers: { Host: u.host }, + });packages/federation-sdk/src/services/media.service.ts (2)
1-38: Consider streaming instead of buffering.Large media buffers increase memory pressure; add a TODO or switch to a pipe/stream path when core fetch supports it.
11-36: Bug: missing await prevents endpoint fallback; fix return type.The try/catch won’t catch async rejection; function never returns null. Await the call and return Promise.
- async downloadFromRemoteServer( + async downloadFromRemoteServer( serverName: string, mediaId: string, - ): Promise<Buffer | null> { + ): Promise<Buffer> { @@ - for (const endpoint of endpoints) { - try { - return this.federationRequest.requestBinaryData( + for (const endpoint of endpoints) { + try { + const data = await this.federationRequest.requestBinaryData( 'GET', serverName, endpoint, ); + return data; } catch (err) { this.logger.debug( `Endpoint ${endpoint} failed: ${err instanceof Error ? err.message : String(err)}`, ); } }
🧹 Nitpick comments (4)
packages/federation-sdk/src/services/federation-request.service.ts (1)
200-214: Narrow method type.Use HttpMethod for requestBinaryData’s method param.
- async requestBinaryData( - method: string, + async requestBinaryData( + method: HttpMethod,packages/federation-sdk/src/container.ts (1)
99-111: Add indexes for new collections.Queries use federation.mediaId and rid; create supporting indexes to avoid full scans.
- rocketchat_uploads: { 'federation.mediaId': 1 }, { rid: 1 }
- rocketchat_matrix_bridged_rooms: { rid: 1 } unique
packages/federation-sdk/src/services/event-authorization.service.ts (1)
182-185: Logging shape: use structured form consistently.Swap params to align with other error logs and preserve stack/fields.
- this.logger.error(error, 'Error verifying request signature'); + this.logger.error({ error }, 'Error verifying request signature');packages/federation-sdk/src/services/staging-area.service.ts (1)
148-161: Guard object spread against undefined and avoid potential runtime crashSpreading
event.event.contentwill throw if it’sundefined/null. Be defensive.Apply this diff:
- content: { - ...event.event.content, + content: { + ...(event.event.content ?? {}), body: event.event.content?.body as string, msgtype: event.event.content?.msgtype as string, 'm.relates_to': event.event.content?.['m.relates_to'] as { rel_type: 'm.replace' | 'm.annotation' | 'm.thread'; event_id: string; }, 'm.new_content': event.event.content?.['m.new_content'] as { body: string; msgtype: string; }, formatted_body: (event.event.content?.formatted_body || '') as string, },Also consider sanitizing dangerous keys like "proto", "constructor", "prototype" from user-controlled content to reduce prototype-pollution risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/core/src/utils/authentication.ts(1 hunks)packages/core/src/utils/fetch.ts(2 hunks)packages/federation-sdk/src/container.ts(2 hunks)packages/federation-sdk/src/index.ts(7 hunks)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts(1 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/federation-request.service.spec.ts(4 hunks)packages/federation-sdk/src/services/federation-request.service.ts(8 hunks)packages/federation-sdk/src/services/media.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/federation-sdk/src/utils/response-codes.ts(1 hunks)packages/homeserver/src/controllers/media.controller.ts(0 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/media.controller.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts
- packages/federation-sdk/src/utils/response-codes.ts
- packages/core/src/utils/authentication.ts
- packages/federation-sdk/src/repositories/upload.repository.ts
- packages/federation-sdk/src/services/federation-request.service.spec.ts
- packages/core/src/utils/fetch.ts
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/federation-sdk/src/services/staging-area.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(102-112)
packages/federation-sdk/src/services/federation-request.service.ts (1)
packages/core/src/utils/fetch.ts (1)
fetch(89-157)
packages/federation-sdk/src/container.ts (2)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
Upload(4-11)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
MatrixBridgedRoom(4-8)
packages/federation-sdk/src/services/event-authorization.service.ts (5)
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
singleton(10-24)packages/federation-sdk/src/repositories/upload.repository.ts (1)
singleton(13-26)packages/federation-sdk/src/services/state.service.ts (1)
singleton(29-835)packages/federation-sdk/src/services/config.service.ts (1)
serverName(94-96)packages/core/src/utils/authentication.ts (2)
extractSignaturesFromHeader(16-47)validateAuthorizationHeader(72-109)
packages/federation-sdk/src/services/media.service.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-215)
🪛 ast-grep (0.38.6)
packages/federation-sdk/src/services/event-authorization.service.ts
[warning] 273-273: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (9)
packages/federation-sdk/src/container.ts (2)
119-121: DI registrations look good.
126-128: Event service DI change LGTM.packages/federation-sdk/src/index.ts (3)
47-47: Export addition LGTM.
78-93: Summary inconsistency: media is still part of HomeserverServices.AI summary claimed media was removed from HomeserverServices; code keeps it. Proceeding with code as source of truth.
202-218: getAllServices wiring LGTM.packages/federation-sdk/src/services/event-authorization.service.ts (2)
282-338: Media ACL/membership flow LGTM.Room mapping → ACL → membership/world_readable checks are coherent.
340-388: Header‑based media auth flow LGTM.Clear Unauthorized vs Forbidden vs Unknown triage.
packages/federation-sdk/src/services/media.service.ts (1)
15-19: No change required — v1 federation media download endpoint is /_matrix/federation/v1/media/download/{mediaId} (no {serverName}). The first endpoint in the array is correct.packages/federation-sdk/src/services/staging-area.service.ts (1)
142-162: Tighten emitted payload for 'homeserver.matrix.message' — avoid spreading raw contentHomeserverEventSignatures is declared in packages/federation-sdk/src/index.ts and EventEmitterService is strongly typed, but the emit in packages/federation-sdk/src/services/staging-area.service.ts (lines ~142–162) spreads event.event.content, which can inject unexpected runtime fields that may break external/downstream consumers (no internal .on('homeserver.matrix.message') listeners found in this repo).
- Confirmed: typed signature (packages/federation-sdk/src/index.ts) and EventEmitterService type enforcement (packages/federation-sdk/src/services/event-emitter.service.ts).
- Action: replace the spread with an explicit whitelist (body, msgtype, 'm.relates_to', 'm.new_content', formatted_body) or validate/cast the payload to the declared event signature (or add zod runtime validation) before emitting; alternatively gate extra-fields behind a config flag.
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/homeserver/src/controllers/federation/media.controller.ts (7)
8-13: DRY the “not implemented” handler and message.You repeat the same 404 body and status across all routes. Centralize it to avoid divergence and ease future changes.
Apply this diff:
@@ /** * The group of media routes are not implemented on homeserver side * due to the fact that homeserver does not have a media repository. * All the medias are being handled by the Rocket.Chat instances. */ +const NOT_IMPLEMENTED = { + errcode: 'M_NOT_FOUND' as const, + error: 'This endpoint is not implemented on the homeserver side', +}; + +const notImplemented = ({ set }: { set: { status: number } }) => { + set.status = 404; + return NOT_IMPLEMENTED; +}; export const mediaPlugin = (app: Elysia) => {
16-33: Use the shared handler and add docs for consistency.Keep route docs consistent with the others and reuse the common handler.
- .get( - '/federation/v1/media/download/:mediaId', - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_NOT_FOUND', - error: 'This endpoint is not implemented on homeserver side', - }; - }, - { + .get( + '/federation/v1/media/download/:mediaId', + notImplemented, + { params: t.Object({ mediaId: t.String(), }), response: { }, + detail: { + tags: ['Media'], + summary: 'Federation media download', + description: 'Download a file from the Matrix media repository (federation).', + }, }, )
35-58: Replace inline handler with the shared one.Keeps behavior identical and reduces duplication.
- .get( - '/media/r0/download/:serverName/:mediaId', - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_NOT_FOUND', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + .get( + '/media/r0/download/:serverName/:mediaId', + notImplemented, {
60-87: Avoid pre-404 validation that changes the status to 400.With the current query schema, requests like width=10000 or non-numeric timeout_ms will fail validation and return 400, not your intended 404 “not implemented”. If the goal is a consistent 404, drop query validation on placeholder routes.
- .get( - '/media/v3/download/:serverName/:mediaId', - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_NOT_FOUND', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + .get( + '/media/v3/download/:serverName/:mediaId', + notImplemented, { params: t.Object({ serverName: t.String(), mediaId: t.String(), }), - query: t.Object({ - allow_remote: t.Optional(t.Boolean()), - timeout_ms: t.Optional(t.Number()), - }), response: { },
89-121: Same: remove thumbnail query constraints or they’ll return 400 instead of 404.If you prefer to keep docs without enforcing limits, omit the schema on placeholders.
- .get( - '/media/v3/thumbnail/:serverName/:mediaId', - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_NOT_FOUND', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + .get( + '/media/v3/thumbnail/:serverName/:mediaId', + notImplemented, { params: t.Object({ serverName: t.String(), mediaId: t.String(), }), - query: t.Object({ - width: t.Optional(t.Number({ minimum: 1, maximum: 800 })), - height: t.Optional(t.Number({ minimum: 1, maximum: 600 })), - method: t.Optional( - t.Union([t.Literal('crop'), t.Literal('scale')]), - ), - allow_remote: t.Optional(t.Boolean()), - timeout_ms: t.Optional(t.Number()), - }), response: { },
123-141: Replace inline handler with the shared one.Uniform implementation across routes.
- .get( - '/media/v3/config', - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_NOT_FOUND', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + .get( + '/media/v3/config', + notImplemented, {
13-144: Add minimal e2e tests for route contracts (404 + body).Lock in the 404 body for each path to prevent regressions and improve patch coverage.
I can scaffold Vitest/Supertest-style tests hitting each route and asserting status 404 and the JSON body. Want me to open a follow-up?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/homeserver/src/controllers/federation/media.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/transactions.controller.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/homeserver/src/controllers/federation/transactions.controller.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/homeserver.module.ts
🔇 Additional comments (1)
packages/homeserver/src/controllers/federation/media.controller.ts (1)
13-33: No global auth middleware found — 404s won't be masked. homeserver.module.ts instantiates the Elysia app and registers plugins (including mediaPlugin) with no auth/guard middleware; federation auth helpers live at packages/federation-sdk/src/services/event-authorization.service.ts but are not applied globally.
packages/homeserver/src/controllers/federation/media.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/utils/fetch.ts (1)
89-93: SNI hostname can become 'undefined' when Host header is missing/cased differently.Accesses
headers.Hostonly; if missing, SNI becomes'undefined'and TLS verification will fail. Handle both casings and fallback tourl.hostname.- const serverName = new URL( - `http://${(options.headers as OutgoingHttpHeaders).Host}` as string, - ).hostname; + const hdrs = options.headers as OutgoingHttpHeaders | undefined; + const hostHeader = + (hdrs?.host as string | undefined) ?? (hdrs?.Host as string | undefined); + const serverName = hostHeader + ? new URL(`http://${hostHeader}`).hostname + : url.hostname;
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/media.service.ts (2)
23-23: ACK: Streaming TODO addedThanks for adding the streaming TODO as requested earlier.
11-15: Fix missing await; tighten return type so fallback works correctlyWithout awaiting, errors bypass the try/catch and the loop never tries the next endpoint. Also the method never returns null; narrow to Buffer.
- ): Promise<Buffer | null> { + ): Promise<Buffer> { @@ - return this.federationRequest.requestBinaryData( + const data = await this.federationRequest.requestBinaryData( 'GET', serverName, endpoint, ); + return data;Run to check call sites don’t rely on
null:#!/bin/bash rg -nP --type=ts -C2 '\bdownloadFromRemoteServer\s*\('Also applies to: 24-28
packages/core/src/utils/fetch.ts (2)
137-147: Normalize content-type header to a string before use.
res.headers['content-type']can bestring | string[] | undefined. Normalize to a string to avoid false negatives in.includes(...).- const contentType = response.headers['content-type'] || ''; + const ct = response.headers['content-type']; + const contentType = Array.isArray(ct) ? (ct[0] ?? '') : (ct ?? '');
96-101: Default the port; avoid passing an empty string to https.request.
url.portis''when absent. Passing it can yield an invalid/ephemeral port. Default based on protocol and coerce to number.- port: url.port, + port: url.port ? Number(url.port) : (url.protocol === 'https:' ? 443 : 80),
🧹 Nitpick comments (5)
packages/core/src/utils/fetch.ts (5)
6-11: Define header type or drop unused field in MultipartResult.
headers?: Record<string, string>is never set byparseMultipart()and may confuse consumers. Either populate it with part headers or remove it.
61-67: text() deviates from WHATWG fetch; consider always returning text.Returning '' for non-
text/*can surprise callers who expectResponse.text()semantics. Either rename totextIfText()or always return UTF‑8 text:-function handleText(contentType: string, body: Buffer): Promise<string> { - if (!contentType.includes('text/')) { - return Promise.resolve(''); - } - return Promise.resolve(body.toString()); -} +function handleText(_contentType: string, body: Buffer): Promise<string> { + return Promise.resolve(body.toString('utf8')); +}
69-86: Boundary handling looks fine; quoted boundary unwrapped.Minor: consider lowercasing
contentTypebefore checks.
104-108: Use IncomingHttpHeaders for response headers.Type currently says
OutgoingHttpHeaders. Use the correct type forres.headers.- headers: OutgoingHttpHeaders; + headers: import('node:http').IncomingHttpHeaders;Additionally, consider exporting a typed response interface for this helper to improve downstream DX.
129-133: LGTM: timeout added; consider making it configurable via env/option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/utils/fetch.ts(2 hunks)packages/federation-sdk/src/services/media.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/media.service.ts (2)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-215)packages/federation-sdk/src/services/config.service.ts (1)
serverName(94-96)
🔇 Additional comments (3)
packages/federation-sdk/src/services/media.service.ts (1)
16-19: Incorrect — federation v1 media path is correct as writtenThe Server–Server v1 endpoint is GET /_matrix/federation/v1/media/download/{mediaId} (no serverName); keep the existing entry. Client downloads use /_matrix/client/v1/media/download/{serverName}/{mediaId}.
Likely an incorrect or invalid review comment.
packages/core/src/utils/fetch.ts (2)
49-59: LGTM: JSON guard with safe fallback.
150-161: Error fallback shape is sensible.Minor: returning raw error messages via
text()may leak low-level details if bubbled to callers. Ensure upstream logging/UX is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
packages/federation-sdk/src/services/media.service.ts (1)
23-23: Streaming is not optional here—buffering enables memory DoS on large media.
Buffering the entire payload can exhaust memory. Enforce a max size and switch to streaming with an abortable byte limit.Follow-up proposal:
- Enforce a configurable MAX_MEDIA_BYTES (e.g., via ConfigService).
- In FederationRequestService, prefer streaming (Readable/AsyncIterator) and hard-stop after N bytes; surface an error before buffering the whole body.
- Plumb an AbortController to cancel oversized/slow transfers.
I can draft the streaming changes in federation-request.service if helpful.
packages/core/src/utils/fetch.ts (2)
89-101: Guard Host header and default the port.This throws if Host is missing or lowercased; also avoid empty port.
-export async function fetch<T>(url: URL, options: RequestInit) { - const serverName = new URL( - `http://${(options.headers as OutgoingHttpHeaders).Host}` as string, - ).hostname; +export async function fetch<T>(url: URL, options: RequestInit) { + const raw = options.headers as OutgoingHttpHeaders | undefined; + const hostHeader = (raw?.Host as string | undefined) ?? (raw as any)?.host; + const serverName = hostHeader ? new URL(`http://${hostHeader}`).hostname : url.hostname; @@ const requestParams: RequestOptions = { host: url.hostname, // IP - port: url.port, + port: url.port || (url.protocol === 'https:' ? '443' : '80'), method: options.method, path: url.pathname + url.search, headers: options.headers as OutgoingHttpHeaders, servername: serverName, };
137-137: Normalize content-type to string before handlers.Header can be string[]; coerce before calling .includes/tests.
- const contentType = response.headers['content-type'] || ''; + const ct = response.headers['content-type']; + const contentType = Array.isArray(ct) ? (ct[0] ?? '') : (ct ?? '');
🧹 Nitpick comments (9)
packages/homeserver/src/controllers/federation/media.controller.ts (3)
8-12: Nit: wording/grammar in the comment.Minor copy tweak for clarity.
- * The group of media routes are unsupported on homeserver side - * due to the fact that homeserver does not have a media repository. - * All the medias are being handled by the Rocket.Chat instances. + * This group of media routes is unsupported on the homeserver side + * because the homeserver does not have a media repository. + * All media is handled by Rocket.Chat instances.
29-32: Add OpenAPI detail for the federation endpoint (consistency with others).response: { }, + detail: { + tags: ['Media'], + summary: 'Federation: download media', + description: + 'Download a file from the Matrix media repository (not implemented on homeserver)', + },
3-6: Optional: reuse centralized error constants if available.If the PR introduced centralized Matrix error codes, import and use them here to avoid string literals.
// e.g., import { MatrixErrcodes } from '@rocket.chat/federation-sdk/errors'; // adjust path if exists // then: errcode: MatrixErrcodes.M_UNRECOGNIZED,If that module doesn’t exist, ignore this comment.
packages/federation-sdk/src/services/media.service.ts (3)
24-31: Log the successful endpoint and bytes for observability.
Helps triage which path works in the wild and catch unexpected payload sizes.Apply this diff:
- const response = await this.federationRequest.requestBinaryData( + const data = await this.federationRequest.requestBinaryData( 'GET', serverName, endpoint, ); - - return response; + this.logger.debug( + `Downloaded media ${mediaId} from ${serverName} via ${endpoint} (${data.length} bytes)`, + ); + return data;
38-38: Surface the last error detail in the thrown error.
Makes failures actionable without hunting logs.Apply this diff:
- throw new Error(`Failed to download media ${mediaId} from ${serverName}`); + throw new Error( + `Failed to download media ${mediaId} from ${serverName} (all endpoints exhausted)`, + );If you prefer richer context, keep the last error:
- for (const endpoint of endpoints) { + let lastError: unknown; + for (const endpoint of endpoints) { try { // ... return data; } catch (err) { + lastError = err; this.logger.debug( `Endpoint ${endpoint} failed: ${err instanceof Error ? err.message : String(err)}`, ); } } - throw new Error(`Failed to download media ${mediaId} from ${serverName}`); + const detail = + lastError instanceof Error ? lastError.message : String(lastError ?? ''); + throw new Error( + `Failed to download media ${mediaId} from ${serverName}: ${detail}`, + );
11-14: Tighten return type to Promise and add a minimal arg guard.Method never returns null (returns a Buffer or throws). Update the signature and fail fast on empty inputs.
File: packages/federation-sdk/src/services/media.service.ts (around lines 11–14)
- async downloadFromRemoteServer( + async downloadFromRemoteServer( serverName: string, mediaId: string, - ): Promise<Buffer | null> { + ): Promise<Buffer> { + if (!serverName?.trim() || !mediaId?.trim()) { + throw new Error('serverName and mediaId are required'); + }No callers found in repo; safe to apply.
packages/core/src/utils/fetch.ts (3)
6-10: Clarify/implement MultipartResult.headers or remove it.The
headers?field is never populated; either extract part-2 headers or drop the field to avoid dead API surface.Apply either approach; if removing:
export type MultipartResult = { content: Buffer; - headers?: Record<string, string>; redirect?: string; };
143-149: Consider exposing a typed response interface.Return shape is implicit; define and export a Response-like type to stabilize public API.
154-160: Error-path text() returns internal messages.If these propagate to clients, sanitize or wrap in a generic message; keep the detailed error for logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/utils/fetch.ts(2 hunks)packages/federation-sdk/src/services/media.service.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/utils/fetch.ts (2)
packages/core/src/index.ts (1)
fetch(89-89)packages/federation-sdk/src/services/federation-request.service.ts (1)
request(142-166)
packages/federation-sdk/src/services/media.service.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-215)
🔇 Additional comments (5)
packages/federation-sdk/src/services/media.service.ts (3)
3-3: LGTM: correct dependency import.
No issues with the new dependency direction.
9-9: LGTM: DI-friendly constructor.
Constructor injection via tsyringe is appropriate.
15-19: Keep federation media endpoint first — current order is correct.
Matrix v1.11+ and major homeservers (Synapse, Dendrite) expect server→server media fetches to use /_matrix/federation/v1/media/download/{mediaId} (authenticated); only fall back to legacy /_matrix/media/... for very old homeservers.
File: packages/federation-sdk/src/services/media.service.ts Lines: 15-19packages/core/src/utils/fetch.ts (2)
61-67: text() behavior is nonstandard; verify callers.Returning empty string for non-text content differs from fetch semantics. Confirm all call sites expect this.
129-133: Timeout addition LGTM.20s default with destroy is a solid reliability improvement. Consider making it configurable per TODO.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/core/src/utils/fetch.ts (6)
49-59: Loosen JSON detection to match +json and case variantsCurrent check misses
application/ld+json, etc.-function handleJson<T>(contentType: string, body: Buffer): Promise<T | null> { - if (!contentType.includes('application/json')) { +function handleJson<T>(contentType: string, body: Buffer): Promise<T | null> { + if (!/\bjson\b/i.test(contentType)) { return Promise.resolve(null); }
69-86: Make multipart detection and boundary extraction case-insensitiveHeader tokens/params are case-insensitive.
-function handleMultipart( +function handleMultipart( contentType: string, body: Buffer, ): Promise<MultipartResult | null> { - if (!contentType.includes('multipart')) { + if (!/\bmultipart\b/i.test(contentType)) { return Promise.resolve(null); } - // extract boundary from content-type header - const boundaryMatch = contentType.match(/boundary=([^;,\s]+)/); + // extract boundary from content-type header + const boundaryMatch = contentType.match(/boundary=([^;,\s]+)/i); if (!boundaryMatch) { return Promise.resolve(null); } // remove quotes if present const boundary = boundaryMatch[1].replace(/^["']|["']$/g, ''); return Promise.resolve(parseMultipart(body, boundary)); }
94-101: Default the port and ensure numeric typeAvoid passing an empty string; choose protocol default when absent.
const requestParams: RequestOptions = { host: url.hostname, // IP - port: url.port, + port: url.port ? Number(url.port) : (url.protocol === 'https:' ? 443 : 80), method: options.method, path: url.pathname + url.search, headers: options.headers as OutgoingHttpHeaders, servername: serverName, };
104-108: Use IncomingHttpHeaders for response.headers
res.headersis Incoming, not Outgoing.-import { type OutgoingHttpHeaders } from 'node:http'; +import { type OutgoingHttpHeaders, type IncomingHttpHeaders } from 'node:http'; @@ - const response: { + const response: { statusCode: number | undefined; body: Buffer; - headers: OutgoingHttpHeaders; + headers: IncomingHttpHeaders; } = await new Promise((resolve, reject) => {Also change the catch return to
headers: {} as IncomingHttpHeaders.- headers: {}, + headers: {} as IncomingHttpHeaders,
157-167: Coerce content-type header to string before handlersHeaders may be arrays; normalize first.
- const contentType = response.headers['content-type'] || ''; + const ct = response.headers['content-type']; + const contentType = Array.isArray(ct) ? (ct[0] ?? '') : (ct ?? '');
16-47: Fix brittle multipart parsing (string conversion, incorrect slicing, redirect assumption)
- Converting the whole Buffer with
toString()risks corruption and waste.lastIndexOf('\r\n\r\n')can occur inside binary, truncating content.- Redirect detection should scan headers of each part, not assume “second part”.
Replace with a Buffer-based parser:
-function parseMultipart(buffer: Buffer, boundary: string): MultipartResult { - const bufferStr = buffer.toString(); - - // check if the second part contains a Location header (CDN redirect) - // pattern: after first boundary and JSON part, look for Location header - const parts = bufferStr.split(`--${boundary}`); - if (parts.length >= 3) { - const secondPart = parts[2]; - const locationMatch = secondPart.match(/\r?\nLocation:\s*(.+)\r?\n/i); - - if (locationMatch) { - return { - content: Buffer.from(''), - redirect: locationMatch[1].trim(), - }; - } - } - - // find where the last part's content starts (after the last \r\n\r\n) - const lastHeaderEnd = buffer.lastIndexOf('\r\n\r\n'); - if (lastHeaderEnd === -1) return { content: buffer }; - - const binaryStart = lastHeaderEnd + 4; - const closingBoundary = buffer.lastIndexOf(`\r\n--${boundary}`); - - const content = - closingBoundary > binaryStart - ? buffer.subarray(binaryStart, closingBoundary) - : buffer.subarray(binaryStart); - - return { content }; -} +function parseMultipart(buffer: Buffer, boundary: string): MultipartResult { + const dashBoundary = `--${boundary}`; + const boundaryBuf = Buffer.from(dashBoundary); + const closingBuf = Buffer.from(`${dashBoundary}--`); + const CRLF = Buffer.from('\r\n'); + + let offset = 0; + let lastContent: Buffer | null = null; + + while (true) { + // find next boundary (either at start or after CRLF) + let bIdx = buffer.indexOf(boundaryBuf, offset); + if (bIdx === -1) break; + + // closing boundary -> done + if (buffer.indexOf(closingBuf, bIdx) === bIdx) break; + + // skip boundary line + CRLF + let headersStart = bIdx + boundaryBuf.length; + if (buffer.indexOf(CRLF, headersStart) === headersStart) { + headersStart += 2; + } else { + // tolerate odd line endings + while (buffer[headersStart] === 13 || buffer[headersStart] === 10) headersStart++; + } + + const headersEnd = buffer.indexOf(Buffer.from('\r\n\r\n'), headersStart); + if (headersEnd === -1) break; + + // parse headers only (safe to decode) + const hdr = buffer.toString('latin1', headersStart, headersEnd); + const loc = hdr.match(/^[Ll]ocation:\s*([^\r\n]+)/m); + if (loc) { + return { content: Buffer.alloc(0), redirect: loc[1].trim() }; + } + + const bodyStart = headersEnd + 4; + // next boundary begins with CRLF + --boundary + let nextBoundary = buffer.indexOf(Buffer.from('\r\n' + dashBoundary), bodyStart); + if (nextBoundary === -1) { + // fallback if CRLF is missing before boundary + nextBoundary = buffer.indexOf(boundaryBuf, bodyStart); + } + const bodyEnd = nextBoundary !== -1 && nextBoundary >= 2 && + buffer[nextBoundary - 2] === 13 && buffer[nextBoundary - 1] === 10 + ? nextBoundary - 2 + : (nextBoundary !== -1 ? nextBoundary : buffer.length); + + lastContent = buffer.subarray(bodyStart, bodyEnd); + offset = nextBoundary === -1 ? buffer.length : nextBoundary + 2; // step over CRLF + } + + return { content: lastContent ?? Buffer.alloc(0) }; +}
🧹 Nitpick comments (3)
packages/core/src/utils/fetch.ts (3)
61-67: Case-insensitive text detectionMinor robustness improvement.
-function handleText(contentType: string, body: Buffer): Promise<string> { - if (!contentType.includes('text/')) { +function handleText(contentType: string, body: Buffer): Promise<string> { + if (!/^\s*text\//i.test(contentType)) { return Promise.resolve(''); } return Promise.resolve(body.toString()); }
89-93: Guard missing Host header and support lowercase 'host'Fallback to the URL hostname when Host isn’t provided.
-export async function fetch<T>(url: URL, options: RequestInit) { - const serverName = new URL( - `http://${(options.headers as OutgoingHttpHeaders).Host}` as string, - ).hostname; +export async function fetch<T>(url: URL, options: RequestInit) { + const hdrs = options.headers as OutgoingHttpHeaders; + const rawHost = (hdrs as any)?.Host ?? (hdrs as any)?.host; + const serverName = new URL(`http://${rawHost ?? url.hostname}`).hostname;
6-10: Remove or populate unused MultipartResult.headersheaders is declared in packages/core/src/utils/fetch.ts (lines 6–8) but never populated by parseMultipart (line 16) and has no external usages; remove the field or parse & return part headers from parseMultipart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/utils/fetch.ts(2 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/controllers/federation/media.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/utils/fetch.ts (1)
packages/core/src/index.ts (1)
fetch(89-89)
🔇 Additional comments (3)
packages/core/src/utils/fetch.ts (3)
112-125: Nice: stream error handling and size cap addedGood production hardening:
res.once('error', ...)and 50MB guard.
136-143: Nice: AbortSignal supportAbort wiring and cleanup on close look good.
149-152: Nice: request timeout20s default with destroy on expiry is reasonable; making it configurable later is fine.
packages/federation-sdk/src/services/federation-request.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/utils/fetch.ts (1)
96-96: Default HTTPS port when URL lacks an explicit port.The port can be empty for HTTPS URLs without an explicit port, which could cause connection issues.
Apply this fix to provide a default port:
- port: url.port, + port: url.port || (url.protocol === 'https:' ? '443' : '80'),
🧹 Nitpick comments (1)
packages/core/src/utils/fetch.ts (1)
157-157: Consider handling array-type headers.HTTP headers can be arrays in Node.js. While single content-type headers are the norm, defensive coding would be beneficial.
Consider normalizing the header value:
- const contentType = response.headers['content-type'] || ''; + const ct = response.headers['content-type']; + const contentType = Array.isArray(ct) ? (ct[0] ?? '') : (ct ?? '');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/utils/fetch.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-14T13:23:17.751Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/core/src/utils/fetch.ts:49-59
Timestamp: 2025-09-14T13:23:17.751Z
Learning: In packages/core/src/utils/fetch.ts, the handleJson function uses exact string matching for "application/json" content type detection. The maintainer ricardogarim prefers to keep this pattern for now rather than using case-insensitive regex matching, noting that lowercase "application/json" is the common standard.
Applied to files:
packages/core/src/utils/fetch.ts
📚 Learning: 2025-09-14T13:15:46.562Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.562Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/core/src/utils/fetch.ts
📚 Learning: 2025-09-14T13:30:07.775Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.775Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/core/src/utils/fetch.ts
🧬 Code graph analysis (1)
packages/core/src/utils/fetch.ts (1)
packages/core/src/index.ts (1)
fetch(89-89)
🔇 Additional comments (11)
packages/core/src/utils/fetch.ts (11)
1-1: LGTM! Correct type import for response headers.Using
IncomingHttpHeadersfor response headers is the proper approach, improving type safety.
6-10: Well-structured multipart result type.The type definition cleanly captures the possible outcomes of multipart parsing - content with optional headers or redirect.
16-47: Consider binary-safe parsing for production robustness.The multipart parsing implementation is functional and will work correctly for Matrix federation responses. While the current approach of converting the entire buffer to string works, I understand from our previous discussion that you prefer to keep it simple for now with a TODO for streaming support already in place.
For reference, the current implementation:
- Correctly handles CDN redirects via Location header detection
- Properly extracts binary content from the last part
- Will handle typical Matrix federation media responses
Given the learnings about keeping implementations simple, this approach is acceptable as an interim solution with planned streaming improvements.
49-59: JSON handler correctly validates content type.The exact match for "application/json" is appropriate as it's the standard lowercase format, as noted in previous discussions.
69-86: Robust multipart detection with case-insensitive matching.Good implementation of case-insensitive content-type and boundary detection, which properly handles HTTP header case variations per spec.
112-112: Good addition of response stream error handling.Properly handles response stream errors by rejecting the promise.
115-125: Excellent memory protection against large responses.The 50MB limit prevents OOM issues while being generous enough for typical media files. The TODO for configurability shows good forward planning.
136-143: Proper AbortSignal support implementation.Clean implementation that respects the abort signal and properly cleans up event listeners.
150-152: Good timeout protection with appropriate duration.The 20-second timeout prevents hung connections while allowing sufficient time for legitimate requests. The TODO for configurability is well-placed.
163-166: Consider why these methods now return Promises.The change from synchronous to Promise-returning methods (
buffer(),json(),text(),multipart()) is noteworthy. While this provides consistency in the API, it's worth documenting the rationale for future maintainers.As noted by @rodrigok in the previous review, this is a breaking change from the original implementation. Could you clarify if this aligns with the authentication requirements or if there's another architectural reason for this change?
88-182: Well-architected fetch implementation for federation needs.The overall implementation is solid with good error handling, proper resource management (timeouts, size limits, abort support), and clean separation of content-type handlers. The multipart support aligns well with Matrix federation requirements for media handling with authentication.
ggazzo
force-pushed
the
feat/acl-methods
branch
from
0479240 to
7209530
Compare
ggazzo
force-pushed
the
refactor/media-service
branch
2 times, most recently
from
b995362 to
f2f9372
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 (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
353-372: ReDoS risk: dynamic RegExp from untrusted ACL pattern. Replace with linear-time wildcard matcher.Regex built from user-controlled patterns can cause catastrophic backtracking (DoS). Use a simple '*'/'?' matcher.
Apply:
- private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } - } + private matchesServerPattern(serverName: string, pattern: string): boolean { + if (serverName === pattern) return true; + let s = 0, p = 0, star = -1, match = 0; + while (s < serverName.length) { + if (p < pattern.length && (pattern[p] === '?' || pattern[p] === serverName[s])) { + s++; p++; + } else if (p < pattern.length && pattern[p] === '*') { + star = p++; match = s; + } else if (star !== -1) { + p = star + 1; s = ++match; + } else { + return false; + } + } + while (p < pattern.length && pattern[p] === '*') p++; + return p === pattern.length; + }
♻️ Duplicate comments (8)
packages/homeserver/src/controllers/federation/media.controller.ts (3)
16-25: DRY the repeated 404 stub handler.Extract a shared unimplemented() and reuse. This avoids drift across routes.
Apply:
+const unimplemented = ({ set }: { set: any }) => { + set.status = 404; + return { + errcode: 'M_UNRECOGNIZED', + error: 'This endpoint is not implemented on homeserver side', + }; +}; @@ - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_UNRECOGNIZED', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + unimplemented, @@ - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_UNRECOGNIZED', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + unimplemented, @@ - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_UNRECOGNIZED', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + unimplemented, @@ - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_UNRECOGNIZED', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + unimplemented, @@ - async ({ set }) => { - set.status = 404; - return { - errcode: 'M_UNRECOGNIZED', - error: 'This endpoint is not implemented on homeserver side', - }; - }, + unimplemented,Also applies to: 35-44, 60-69, 89-98, 123-132
74-77: Constrain numeric query params to integers with bounds.Width/height/timeout should be integers; enforce min where applicable.
- query: t.Object({ - allow_remote: t.Optional(t.Boolean()), - timeout_ms: t.Optional(t.Number()), - }), + query: t.Object({ + allow_remote: t.Optional(t.Boolean()), + timeout_ms: t.Optional(t.Integer({ minimum: 0 })), + }), @@ - query: t.Object({ - width: t.Optional(t.Number({ minimum: 1, maximum: 800 })), - height: t.Optional(t.Number({ minimum: 1, maximum: 600 })), + query: t.Object({ + width: t.Optional(t.Integer({ minimum: 1, maximum: 800 })), + height: t.Optional(t.Integer({ minimum: 1, maximum: 600 })), method: t.Optional( t.Union([t.Literal('crop'), t.Literal('scale')]), ), allow_remote: t.Optional(t.Boolean()), - timeout_ms: t.Optional(t.Number()), + timeout_ms: t.Optional(t.Integer({ minimum: 0 })), }),Also applies to: 104-111
13-15: Map framework 405 to Matrix M_UNRECOGNIZED.Add a local onError for NOT_ALLOWED → 405 with Matrix body.
// add inside mediaPlugin before returning the group: app.onError(({ code, set }) => { if (code === 'NOT_ALLOWED') { set.status = 405; return { errcode: 'M_UNRECOGNIZED', error: 'Unsupported method' }; } });packages/core/src/utils/fetch.ts (2)
157-167: Normalize content-type header to string.IncomingHttpHeaders allows string|string[]; current code may pass an array to handlers causing runtime errors.
Apply:
- const contentType = response.headers['content-type'] || ''; + const ct = response.headers['content-type']; + const contentType = + Array.isArray(ct) ? (ct[0] ?? '') : (ct ?? '');
90-101: Guard Host header and default the port.If Host is missing, serverName derivation breaks; passing empty port can misbehave.
Apply:
-export async function fetch<T>(url: URL, options: RequestInit) { - const serverName = new URL( - `http://${(options.headers as IncomingHttpHeaders).Host}` as string, - ).hostname; +export async function fetch<T>(url: URL, options: RequestInit) { + const hostHeader = (options.headers as IncomingHttpHeaders)?.Host; + const serverName = hostHeader + ? new URL(`http://${hostHeader as string}`).hostname + : url.hostname; @@ - const requestParams: RequestOptions = { + const requestParams: RequestOptions = { host: url.hostname, // IP - port: url.port, + port: url.port || 443, method: options.method, path: url.pathname + url.search, headers: options.headers as IncomingHttpHeaders, servername: serverName, };packages/federation-sdk/src/services/federation-request.service.ts (2)
32-45: Redirect fetch: enforce HTTPS and set Host for SNI.Without Host, core fetch derives an invalid SNI; also reject non-HTTPS redirects.
Apply:
// the redirect URL should be fetched without Matrix auth // and will only occur for media downloads as per Matrix spec private async handleRedirect(redirect: string): Promise<Buffer> { - const redirectResponse = await fetch(new URL(redirect), { - method: 'GET', - headers: {}, - }); + const u = new URL(redirect); + if (u.protocol !== 'https:') { + throw new Error(`Rejected non-HTTPS redirect: ${redirect}`); + } + const redirectResponse = await fetch(u, { + method: 'GET', + headers: { Host: u.host }, + });
100-106: Ensure Host header is always present on signed requests.If discoveryHeaders lacks Host, SNI in core fetch breaks.
Apply:
- const headers = { + const headers: Record<string, string> = { Authorization: auth, ...discoveryHeaders, ...(signedBody && { 'Content-Type': 'application/json' }), }; + if (!headers.Host) { + headers.Host = url.host; + }packages/federation-sdk/src/services/media.service.ts (1)
25-33: ACK on streaming TODO; deferral is fine here.Noting the prior discussion and your preference to defer streaming/timeouts—keeping the TODO is acceptable for now.
🧹 Nitpick comments (5)
packages/federation-sdk/src/repositories/upload.repository.ts (2)
19-25: Disambiguate lookups: mediaId alone may collide across origins.Matrix mediaId is only unique per server. Recommend keying by federation.serverName + mediaId (or federation.mxcUri) to avoid false room mappings.
Minimal API addition to keep current callers working:
export class UploadRepository { @@ - async findRocketChatRoomIdByMediaId(mediaId: string): Promise<string | null> { - const upload = await this.collection.findOne({ - 'federation.mediaId': mediaId, - }); + async findRocketChatRoomIdByMediaId(mediaId: string): Promise<string | null> { + const upload = await this.collection.findOne( + { 'federation.mediaId': mediaId }, + { projection: { rid: 1 } }, + ); return upload?.rid || null; } + + async findRocketChatRoomIdByOriginAndMediaId( + serverName: string, + mediaId: string, + ): Promise<string | null> { + const upload = await this.collection.findOne( + { 'federation.serverName': serverName, 'federation.mediaId': mediaId }, + { projection: { rid: 1 } }, + ); + return upload?.rid || null; + }Would you be open to updating the authorization path to pass the origin server name when available?
4-11: Add an index on federation.mediaId (and ideally {serverName, mediaId}).Prevents collection scans on hot path authorization.
Example (migration/init script):
await collection.createIndex({ 'federation.mediaId': 1 }); await collection.createIndex( { 'federation.serverName': 1, 'federation.mediaId': 1 }, { unique: true, sparse: true }, );packages/core/src/utils/fetch.ts (1)
16-47: Reduce risk in multipart parsing without large refactor.Use latin1 for header-safe decoding and anchor Location match.
-function parseMultipart(buffer: Buffer, boundary: string): MultipartResult { - const bufferStr = buffer.toString(); +function parseMultipart(buffer: Buffer, boundary: string): MultipartResult { + const bufferStr = buffer.toString('latin1'); @@ - const locationMatch = secondPart.match(/\r?\nLocation:\s*(.+)\r?\n/i); + const locationMatch = secondPart.match(/(?:^|\r?\n)Location:\s*(\S+)(?:\r?\n|$)/i);packages/federation-sdk/src/services/federation-request.service.ts (1)
200-214: Narrow method type for requestBinaryData.Use the existing HttpMethod union for consistency.
- async requestBinaryData( - method: string, + async requestBinaryData( + method: HttpMethod, targetServer: string, endpoint: string, queryParams?: Record<string, string>, ) {packages/federation-sdk/src/services/media.service.ts (1)
36-42: Include the last failure cause in the thrown error for easier diagnosis.Surface the final error message to speed up debugging when all endpoints fail.
for (const endpoint of endpoints) { try { // TODO: Stream remote file downloads instead of buffering the entire file in memory. const response = await this.federationRequest.requestBinaryData( 'GET', serverName, endpoint, ); return response; } catch (err) { + lastErr = err; this.logger.debug( `Endpoint ${endpoint} failed: ${err instanceof Error ? err.message : String(err)}`, ); } } - throw new Error(`Failed to download media ${mediaId} from ${serverName}`); + const detail = + lastErr && (lastErr instanceof Error ? `: ${lastErr.message}` : `: ${String(lastErr)}`); + throw new Error(`Failed to download media ${mediaId} from ${serverName}${detail || ''}`);Add near the endpoints declaration:
const endpoints = [ `/_matrix/federation/v1/media/download/${mediaId}`, `/_matrix/media/v3/download/${serverName}/${mediaId}`, `/_matrix/media/r0/download/${serverName}/${mediaId}`, ]; + + let lastErr: unknown;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/core/src/utils/authentication.ts(1 hunks)packages/core/src/utils/fetch.ts(1 hunks)packages/federation-sdk/src/container.ts(2 hunks)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts(1 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)packages/federation-sdk/src/services/federation-request.service.spec.ts(4 hunks)packages/federation-sdk/src/services/federation-request.service.ts(8 hunks)packages/federation-sdk/src/services/media.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(1 hunks)packages/homeserver/src/controllers/media.controller.ts(0 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/media.controller.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/federation-sdk/src/services/staging-area.service.ts
- packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts
- packages/federation-sdk/src/container.ts
- packages/core/src/utils/authentication.ts
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/services/federation-request.service.spec.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-14T13:23:17.751Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/core/src/utils/fetch.ts:49-59
Timestamp: 2025-09-14T13:23:17.751Z
Learning: In packages/core/src/utils/fetch.ts, the handleJson function uses exact string matching for "application/json" content type detection. The maintainer ricardogarim prefers to keep this pattern for now rather than using case-insensitive regex matching, noting that lowercase "application/json" is the common standard.
Applied to files:
packages/core/src/utils/fetch.ts
📚 Learning: 2025-09-14T13:15:46.562Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.562Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/core/src/utils/fetch.tspackages/federation-sdk/src/services/media.service.ts
📚 Learning: 2025-09-14T13:30:07.775Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.775Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/core/src/utils/fetch.tspackages/federation-sdk/src/services/media.service.ts
📚 Learning: 2025-09-14T13:34:12.421Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.421Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (5)
packages/federation-sdk/src/repositories/upload.repository.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
singleton(18-481)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
singleton(10-24)
packages/core/src/utils/fetch.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
request(142-166)
packages/federation-sdk/src/services/federation-request.service.ts (1)
packages/core/src/utils/fetch.ts (1)
fetch(89-182)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
packages/federation-sdk/src/services/config.service.ts (1)
serverName(94-96)
packages/federation-sdk/src/services/media.service.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-215)
🔇 Additional comments (1)
packages/federation-sdk/src/services/media.service.ts (1)
15-19: Narrow return type to PromiseMethod either returns a Buffer from the retry loop or throws; returning null is unreachable.
File: packages/federation-sdk/src/services/media.service.ts
- ): Promise<Buffer | null> { + ): Promise<Buffer> {
| async canAccessMedia(mediaId: string, serverName: string): Promise<boolean> { | ||
| try { | ||
| const rcRoomId = | ||
| await this.uploadRepository.findRocketChatRoomIdByMediaId(mediaId); | ||
| if (!rcRoomId) { | ||
| this.logger.debug(`Media ${mediaId} not found in any room`); | ||
| return false; | ||
| } | ||
|
|
||
| const matrixRoomId = | ||
| await this.matrixBridgedRoomRepository.findMatrixRoomId(rcRoomId); | ||
| if (!matrixRoomId) { | ||
| this.logger.debug(`Media ${mediaId} not found in any room`); | ||
| return false; | ||
| } | ||
|
|
||
| const isServerAllowed = await this.checkServerAcl( | ||
| matrixRoomId, | ||
| serverName, | ||
| ); | ||
| if (!isServerAllowed) { | ||
| this.logger.warn( | ||
| `Server ${serverName} is denied by room ACL for media in room ${matrixRoomId}`, | ||
| ); | ||
| return false; | ||
| } | ||
|
|
||
| const serversInRoom = | ||
| await this.stateService.getServersInRoom(matrixRoomId); | ||
| if (serversInRoom.includes(serverName)) { | ||
| this.logger.debug( | ||
| `Server ${serverName} is in room ${matrixRoomId}, allowing media access`, | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| const roomState = await this.stateService.getFullRoomState(matrixRoomId); | ||
| const historyVisibility = this.getHistoryVisibility(roomState); | ||
| if (historyVisibility === 'world_readable') { | ||
| this.logger.debug( | ||
| `Room ${matrixRoomId} is world_readable, allowing media access to ${serverName}`, | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| this.logger.debug( | ||
| `Server ${serverName} not authorized for media ${mediaId}: not in room and room not world_readable`, | ||
| ); | ||
| return false; | ||
| } catch (error) { | ||
| this.logger.error( | ||
| { error, mediaId, serverName }, | ||
| 'Error checking media access', | ||
| ); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider origin server in media lookup to avoid collisions.
Authorization maps mediaId → RC room without origin context. If the DB can contain same mediaId from different servers, access could be misattributed.
Proposed direction:
- Pass originServerName into canAccessMedia(mediaId, originServerName, requesterServerName).
- Use UploadRepository.findRocketChatRoomIdByOriginAndMediaId(serverName, mediaId).
Would you like me to draft the end-to-end signature changes?
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root ==="
pwd
echo
echo "=== Searching for event-authorization.service.ts and canAccessMedia ==="
rg -n --hidden --no-ignore -S "event-authorization.service.ts" || true
rg -n --hidden --no-ignore -S "canAccessMedia\s*\(" || true
echo
echo "=== Search for repository method names referenced in the review ==="
rg -n --hidden --no-ignore -S "findRocketChatRoomIdByMediaId" || true
rg -n --hidden --no-ignore -S "findRocketChatRoomIdByOrigin" || true
rg -n --hidden --no-ignore -S "findRocketChatRoomIdByOriginAndMediaId" || true
rg -n --hidden --no-ignore -S "findRocketChatRoomIdBy" || true
echo
echo "=== Search for UploadRepository / uploadRepository usage ==="
rg -n --hidden --no-ignore -S "UploadRepository" || true
rg -n --hidden --no-ignore -S "uploadRepository" -C3 || true
echo
echo "=== Search for mediaId and origin usage across repo ==="
rg -n --hidden --no-ignore -S "\\bmediaId\\b" -C2 || true
rg -n --hidden --no-ignore -S "\\borigin\\b" -C2 || true
echo
echo "=== If event-authorization.service.ts exists, show context around the snippet ==="
FILE=$(rg -n --hidden --no-ignore -S "event-authorization.service.ts" -g '!node_modules' -l || true)
if [ -n "$FILE" ]; then
echo "Found: $FILE"
echo "---- head (lines 1-240) ----"
sed -n '1,240p' "$FILE" || true
echo "---- snippet (lines 360-460) ----"
sed -n '360,460p' "$FILE" || true
else
echo "event-authorization.service.ts not found"
fi
echo
echo "=== Done ==="Length of output: 107848
Require origin when authorizing media access — prevent mediaId collisions.
findRocketChatRoomIdByMediaId() only matches 'federation.mediaId' (packages/federation-sdk/src/repositories/upload.repository.ts:19), so canAccessMedia(mediaId, serverName) can return the wrong room when the same mediaId exists on different origins.
- Change canAccessMedia signature to include the media origin (e.g. canAccessMedia(mediaId: string, originServerName: string, requesterServerName: string)) and update caller at packages/federation-sdk/src/services/event-authorization.service.ts:459.
- Add UploadRepository.findRocketChatRoomIdByOriginAndMediaId(origin: string, mediaId: string) and use it in event-authorization.service.ts (around line 374) instead of findRocketChatRoomIdByMediaId.
- Update media download callers/controllers to pass the origin where available.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/event-authorization.service.ts around
lines 374–430, canAccessMedia currently looks up a room by mediaId only, causing
collisions when identical mediaIds exist on different origins; change
canAccessMedia signature to accept origin and requesterServerName (e.g.
canAccessMedia(mediaId: string, originServerName: string, requesterServerName:
string)), add a new
UploadRepository.findRocketChatRoomIdByOriginAndMediaId(origin: string, mediaId:
string) and call that instead of findRocketChatRoomIdByMediaId, update the
internal variable names and logging to reflect origin and requester, and update
the caller referenced at line ~459 plus any media download controllers to pass
the origin where available so the correct room is resolved.
| `/_matrix/federation/v1/media/download/${mediaId}`, | ||
| `/_matrix/media/v3/download/${serverName}/${mediaId}`, | ||
| `/_matrix/media/r0/download/${serverName}/${mediaId}`, | ||
| ]; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify first endpoint path and path-segment expectations.
Double‑check that /_matrix/federation/v1/media/download/${mediaId} is the intended S2S path (note: it lacks serverName unlike the other two). Also confirm whether any encoding/validation is required for serverName and mediaId as path segments.
🌐 Web query:
Matrix media download over federation: is there a server‑server endpoint at `/_matrix/federation/v1/media/download/{mediaId}` or should it include `{serverName}/{mediaId}`? Also confirm client‑server download paths for v3 and r0.
💡 Result:
Short answer:
- Federation: the spec defines GET /_matrix/federation/v1/media/download/{mediaId} (no {serverName} path component). [1][2]
- Client–Server: current authenticated path is GET /_matrix/client/v1/media/download/{serverName}/{mediaId}/{fileName} (and the matching thumbnail endpoint). The older/deprecated endpoints are /_matrix/media/v3/download/{serverName}/{mediaId}/{fileName} and earlier /_matrix/media/r0/download/{serverName}/{mediaId}/{fileName}. [3][4]
Sources:
- Server‑Server API (/_matrix/federation/v1/media/download/{mediaId}). [spec].
- Matrix v1.11 release notes (announcing federation /media/download/{mediaId}).
- Client‑Server API (/_matrix/client/v1/media/download/{serverName}/{mediaId}/{fileName}).
- Legacy r0 client/media download docs (/_matrix/media/r0/download/{serverName}/{mediaId}/{fileName}).
Federation path is correct; URL‑encode segments and include filename for client download paths
- Federation: /_matrix/federation/v1/media/download/{mediaId} is the correct S2S endpoint (no {serverName}).
- Client/legacy: client download paths expect a filename segment — use /_matrix/client/v1/media/download/{serverName}/{mediaId}/{fileName} (authenticated) or the legacy /_matrix/media/v3/download/{serverName}/{mediaId}/{fileName} and /_matrix/media/r0/download/{serverName}/{mediaId}/{fileName}. The v3/r0 entries in the snippet omit {fileName} and should be adjusted unless you intentionally rely on a server that accepts the shorter form.
- Always URL‑encode path segments (serverName, mediaId, fileName) when constructing these URLs (e.g., encodeURIComponent).
File: packages/federation-sdk/src/services/media.service.ts (lines 20–23).
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/media.service.ts around lines 20–23, the
endpoint list is incorrect: the federation S2S endpoint should remain
/_matrix/federation/v1/media/download/{mediaId} (no serverName) while
client/legacy download paths must include a filename segment; update the entries
to use /_matrix/client/v1/media/download/{serverName}/{mediaId}/{fileName} and
the legacy /_matrix/media/v3/download/{serverName}/{mediaId}/{fileName} and
/_matrix/media/r0/download/{serverName}/{mediaId}/{fileName} (remove the shorter
forms unless intentionally supported), and ensure every path segment
(serverName, mediaId, fileName) is URL-encoded (encodeURIComponent) when
constructing these URLs.
…ot moving the next endpoints
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
sampaiodiego
force-pushed
the
refactor/media-service
branch
from
55252a1 to
7640ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/homeserver/src/homeserver.module.ts (2)
76-76: MEDIA_ENABLE_THUMBNAILS is ignored; feature always enabled.
=== 'true' || truealways yieldstrue, making the env flag ineffective.Apply:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: + process.env.MEDIA_ENABLE_THUMBNAILS === undefined + ? true + : process.env.MEDIA_ENABLE_THUMBNAILS === 'true',
90-92: Don’t ignore provided container options.
options?.containerOptionsis declared but unused; onlyemitteris forwarded.Apply:
- const containerOptions: FederationContainerOptions = { - emitter: options?.emitter, - }; + const containerOptions: FederationContainerOptions = { + ...(options?.containerOptions ?? {}), + // Explicitly prefer the top-level emitter if provided + emitter: options?.emitter ?? options?.containerOptions?.emitter, + };
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
381-388: ReDoS risk via regex-based ACL wildcard matching. Replace with linear wildcard matcher.Use a non‑regex wildcard match (O(n)):
- private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - regexPattern = `^${regexPattern}$`; - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } - } + private matchesServerPattern(serverName: string, pattern: string): boolean { + if (serverName === pattern) return true; + let s = 0, p = 0, star = -1, match = 0; + while (s < serverName.length) { + if (p < pattern.length && (pattern[p] === '?' || pattern[p] === serverName[s])) { s++; p++; } + else if (p < pattern.length && pattern[p] === '*') { star = p++; match = s; } + else if (star !== -1) { p = star + 1; s = ++match; } + else { return false; } + } + while (p < pattern.length && pattern[p] === '*') p++; + return p === pattern.length; + }packages/federation-sdk/src/services/media.service.ts (2)
19-23: Encode path segments; add filename for client paths (spec).Encode serverName/mediaId to avoid path injection. Client media download paths typically include a filename segment; consider adding it if you rely on v3/r0 fallbacks.
- const endpoints = [ - `/_matrix/federation/v1/media/download/${mediaId}`, - `/_matrix/media/v3/download/${serverName}/${mediaId}`, - `/_matrix/media/r0/download/${serverName}/${mediaId}`, - ]; + const s = encodeURIComponent(serverName); + const m = encodeURIComponent(mediaId); + const endpoints = [ + `/_matrix/federation/v1/media/download/${m}`, + // If using client/legacy fallbacks, include a filename (e.g., mediaId) per spec. + `/_matrix/media/v3/download/${s}/${m}/${m}`, + `/_matrix/media/r0/download/${s}/${m}/${m}`, + ];Matrix Client-Server API: confirm download paths require filename for /_matrix/media/v3|r0 and the recommended authenticated path at /_matrix/client/v1/media/download/{serverName}/{mediaId}/{fileName}. Also confirm federation S2S path for /_matrix/federation/v1/media/download/{mediaId}.
25-35: Consume Buffer from requestBinaryData after switching to buffer().Reflects the return-type change proposed in FederationRequestService.
- const response = await this.federationRequest.requestBinaryData( + const data = await this.federationRequest.requestBinaryData( 'GET', serverName, endpoint, ); - - return response.content; + return data;packages/federation-sdk/src/services/federation-request.service.ts (1)
179-195: Binary responses: use buffer() instead of multipart(); also narrow verb type.Most media endpoints are single-part octet-stream; multipart() will fail or add complexity. Return Buffer directly and type the method.
- async requestBinaryData( - method: string, + async requestBinaryData( + method: HttpMethod, targetServer: string, endpoint: string, queryParams?: Record<string, string>, - ) { + ): Promise<Buffer> { const response = await this.makeSignedRequest({ method, domain: targetServer, uri: endpoint, queryString: queryParams ? new URLSearchParams(queryParams).toString() : '', }); - return response.multipart(); + return response.buffer(); }Follow-ups required in callers and tests are proposed in their respective comments.
🧹 Nitpick comments (17)
packages/homeserver/src/homeserver.module.ts (6)
62-64: Harden MEDIA_MAX_FILE_SIZE parsing (NaN/≤0 yields NaN).Invalid or non‑positive values produce
NaNand propagate.Apply:
- maxFileSize: process.env.MEDIA_MAX_FILE_SIZE - ? Number.parseInt(process.env.MEDIA_MAX_FILE_SIZE, 10) * 1024 * 1024 - : 100 * 1024 * 1024, + maxFileSize: (() => { + const mb = Number.parseInt(process.env.MEDIA_MAX_FILE_SIZE ?? '', 10); + return Number.isFinite(mb) && mb > 0 ? mb * 1024 * 1024 : 100 * 1024 * 1024; + })(),
65-75: Trim/filter MEDIA_ALLOWED_MIME_TYPES to avoid empty/spacey entries.Empty env or extra spaces can yield
['']or malformed items.Apply:
- allowedMimeTypes: process.env.MEDIA_ALLOWED_MIME_TYPES?.split(',') || [ + allowedMimeTypes: process.env.MEDIA_ALLOWED_MIME_TYPES + ? process.env.MEDIA_ALLOWED_MIME_TYPES.split(',').map((s) => s.trim()).filter(Boolean) + : [ 'image/jpeg', 'image/png', 'image/gif', 'image/webp', 'text/plain', 'application/pdf', 'video/mp4', 'audio/mpeg', 'audio/ogg', - ], + ],
78-86: Validate/clamp rate‑limit envs to sane defaults.Non‑numeric or non‑positive values will set invalid limits.
Apply:
- uploadPerMinute: Number.parseInt( - process.env.MEDIA_UPLOAD_RATE_LIMIT || '10', - 10, - ), - downloadPerMinute: Number.parseInt( - process.env.MEDIA_DOWNLOAD_RATE_LIMIT || '60', - 10, - ), + uploadPerMinute: (() => { + const n = Number.parseInt(process.env.MEDIA_UPLOAD_RATE_LIMIT ?? '', 10); + return Number.isFinite(n) && n > 0 ? n : 10; + })(), + downloadPerMinute: (() => { + const n = Number.parseInt(process.env.MEDIA_DOWNLOAD_RATE_LIMIT ?? '', 10); + return Number.isFinite(n) && n > 0 ? n : 60; + })(),
59-59: Possible env mismatch: CONFIG_FOLDER used as signing key path.
signingKeyPathexpects a file;CONFIG_FOLDERsounds like a directory. Prefer a dedicatedSIGNING_KEY_PATH, falling back to a file withinCONFIG_FOLDER.Apply:
- signingKeyPath: process.env.CONFIG_FOLDER || './rc1.signing.key', + signingKeyPath: + process.env.SIGNING_KEY_PATH + || (process.env.CONFIG_FOLDER ? path.join(process.env.CONFIG_FOLDER, 'rc1.signing.key') : './rc1.signing.key'),
98-111: Gate Swagger in prod.Avoid exposing API docs in production by default.
Suggested pattern (outside this hunk):
if (process.env.ENABLE_SWAGGER === 'true' || process.env.NODE_ENV !== 'production') { // @ts-ignore - Elysia is not typed correctly app.use(swagger({ documentation: { info: { title: 'Matrix Homeserver API', version: '1.0.0', description: 'Matrix Protocol Implementation - Federation and Internal APIs' } } })); }
61-87: Consider relocating media config if homeserver won’t serve media.Since media serving moved to Rocket.Chat and federation plugin stubs, this block may be unused here and confuse operators.
Option: keep only values required by federation stubs, or document that these are ignored on homeserver.
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (1)
17-23: Use projection to cut I/O; consider enforcing an index.Fetch only mri and rely on an index on rid for O(1) lookup.
Apply:
- const bridgedRoom = await this.collection.findOne({ - rid: rocketChatRoomId, - }); + const bridgedRoom = await this.collection.findOne( + { rid: rocketChatRoomId }, + { projection: { mri: 1 } }, + );Operationally: add an index (unique if domain allows one Matrix per RC room):
{ rid: 1 }.packages/federation-sdk/src/services/event-authorization.service.ts (2)
411-414: Tighten log messages to aid diagnosis.- this.logger.debug( - `Server ${serverName} not authorized for media ${mediaId}: not in room and room not world_readable`, - ); + this.logger.debug({ + msg: 'Media access denied', + mediaId, + matrixRoomId, + serverName, + reason: 'not in room and room not world_readable', + });
424-471: Origin already plumbed — no caller change required; add testsverifyRequestSignature returns the origin string and canAccessMediaFromAuthorizationHeader already forwards that value into canAccessMedia (types align).
- Add tests:
- origin mismatch → expect M_FORBIDDEN
- same mediaId on different origins → only the origin from the signature allowed
- world_readable room → allowed for non‑members
packages/federation-sdk/src/services/federation-request.service.spec.ts (5)
46-56: Mock the right response type (use FetchResponse shape).The stub returns a DOM Response-like object. Core’s fetch returns FetchResponse; add headers, buffer, and body to avoid type drift and future breakage.
await mock.module('@hs/core', () => ({ - fetch: async (_url: string, _options?: RequestInit) => { - return { - ok: true, - status: 200, - json: async () => ({ result: 'success' }), - text: async () => '{"result":"success"}', - multipart: async () => null, - } as Response; - }, + fetch: async (_url: string, _options?: RequestInit) => ({ + ok: true, + status: 200, + headers: {} as any, + buffer: async () => Buffer.from('{"result":"success"}'), + body: async () => Buffer.from('{"result":"success"}'), + json: async () => ({ result: 'success' }), + text: async () => '{"result":"success"}', + multipart: async () => null, + }), }));
116-122: Remove duplicate expectations.The three expectations on serverName/signingKey are repeated twice.
- expect(configService.serverName).toBe(mockServerName); - expect(await configService.getSigningKeyBase64()).toBe(mockSigningKey); - expect(await configService.getSigningKeyId()).toBe(mockSigningKeyId);
203-215: Don’t patch global fetch; stub core.fetch for error paths.makeSignedRequest uses @hs/core fetch. Stubbing globalThis.fetch won’t exercise the error branches reliably.
- globalThis.fetch = Object.assign( - async () => { - return { - ok: false, - status: 404, - text: async () => 'Not Found', - multipart: async () => null, - } as Response; - }, - { preconnect: () => {} }, - ) as typeof fetch; + spyOn(core, 'fetch').mockResolvedValue({ + ok: false, + status: 404, + headers: {} as any, + text: async () => 'Not Found', + json: async () => { throw new Error('Not JSON'); }, + buffer: async () => Buffer.alloc(0), + body: async () => Buffer.alloc(0), + multipart: async () => { throw new Error('no multipart'); }, + } as any);Apply a similar change in the 400 test block.
Also applies to: 233-246
293-300: Return full FetchResponse shape in spies.The mocked return lacks headers/buffer/body; add them for accuracy.
}).mockResolvedValue({ ok: true, status: 200, + headers: {} as any, json: async () => ({ result: 'success' }), text: async () => '{"result":"success"}', - multipart: async () => null, + buffer: async () => Buffer.from(''), + body: async () => Buffer.from(''), + multipart: async () => null, });Also applies to: 317-323, 344-349
365-389: Align tests with binary path returning a Buffer (not multipart container).Media endpoints generally return a single binary body. Assert on Buffer to match a safer implementation.
-).mockResolvedValue({ - ok: true, - status: 200, - multipart: async () => ({ content: mockBuffer }), -}); +).mockResolvedValue({ + ok: true, + status: 200, + headers: {} as any, + buffer: async () => mockBuffer, + body: async () => mockBuffer, + json: async () => ({} as any), + text: async () => '', + multipart: async () => { throw new Error('no multipart'); }, +} as any); @@ - expect(result).toEqual({ content: mockBuffer }); + expect(result).toEqual(mockBuffer);Apply the same changes to the “with query params” test below.
Also applies to: 391-417
packages/federation-sdk/src/services/federation-request.service.ts (2)
16-24: Narrow the HTTP verb type in SignedRequest.Use HttpMethod to prevent accidental unsupported verbs and signing mismatches.
-interface SignedRequest { - method: string; +interface SignedRequest { + method: HttpMethod;
67-74: Safer check for pre-hashed bodies.Avoid loose property access on Record<string, unknown>.
- if (body) { - signedBody = await signJson( - body.hashes ? body : computeAndMergeHash({ ...body, signatures: {} }), + if (body) { + const hasHashes = Object.prototype.hasOwnProperty.call(body, 'hashes'); + signedBody = await signJson( + hasHashes ? body : computeAndMergeHash({ ...body, signatures: {} }),packages/federation-sdk/src/services/media.service.ts (1)
15-19: Return type never null.The method either returns data or throws. Drop null from the signature.
-): Promise<Buffer | null> { +): Promise<Buffer> {
📜 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 (14)
packages/core/src/index.ts(1 hunks)packages/core/src/utils/authentication.ts(1 hunks)packages/core/src/utils/fetch.ts(1 hunks)packages/federation-sdk/src/container.ts(2 hunks)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts(1 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)packages/federation-sdk/src/services/federation-request.service.spec.ts(11 hunks)packages/federation-sdk/src/services/federation-request.service.ts(8 hunks)packages/federation-sdk/src/services/media.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/homeserver/src/controllers/federation/media.controller.ts(1 hunks)packages/homeserver/src/controllers/media.controller.ts(0 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/media.controller.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/federation-sdk/src/services/staging-area.service.ts
- packages/core/src/index.ts
- packages/homeserver/src/controllers/federation/media.controller.ts
- packages/federation-sdk/src/repositories/upload.repository.ts
- packages/core/src/utils/fetch.ts
- packages/core/src/utils/authentication.ts
- packages/federation-sdk/src/container.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/media.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/media.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (4)
packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (2)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
singleton(18-473)packages/federation-sdk/src/repositories/upload.repository.ts (1)
singleton(13-26)
packages/federation-sdk/src/services/media.service.ts (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
singleton(26-196)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
packages/federation-sdk/src/services/config.service.ts (1)
serverName(94-96)
packages/federation-sdk/src/services/federation-request.service.ts (1)
packages/core/src/utils/fetch.ts (2)
FetchResponse(129-138)fetch(141-271)
🔇 Additional comments (6)
packages/homeserver/src/homeserver.module.ts (1)
18-18: mediaPlugin export present; old controller references not verifiedFound
export const mediaPluginin packages/homeserver/src/controllers/federation/media.controller.ts (line 13). Ripgrep reported "No files were searched" for the old-path check, so I couldn't confirm removal of packages/homeserver/src/controllers/media.controller.ts or any remaining references — run: rg -nS -uu 'controllers/media\.controller' || true && test -f packages/homeserver/src/controllers/media.controller.ts && echo 'old file exists' || echo 'old file missing'packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts (2)
4-8: Confirm domain constraints; index accordingly.If multiple bridges per RC room are possible (e.g., by fromServer), prefer a compound unique index on { rid: 1, fromServer: 1 } and query with both fields when needed.
12-15: Resolved: 'MatrixBridgedRoomCollection' DI registration verified.
Container binds 'MatrixBridgedRoomCollection' to db.collection in packages/federation-sdk/src/container.ts (around line 99); injection in packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts is correct.packages/federation-sdk/src/services/event-authorization.service.ts (3)
12-13: Good repository wiring.
26-28: DI tokens and container wiring OK — no action required. UploadCollection and MatrixBridgedRoomCollection are registered in packages/federation-sdk/src/container.ts and both UploadRepository and MatrixBridgedRoomRepository are registered as singletons there; the constructor injections in event-authorization.service.ts will resolve.
362-371: Security: include origin in media lookup to prevent cross-origin media collisions
- Problem: lookup uses only federation.mediaId (packages/federation-sdk/src/repositories/upload.repository.ts) and can match uploads from other origins.
- Fix: pass the origin/serverName into canAccessMedia and query by both origin and mediaId (e.g., repository method findRocketChatRoomIdByOriginAndMediaId(origin, mediaId) that does a findOne({ 'federation.origin': origin, 'federation.mediaId': mediaId }, { projection: { rid: 1 } })). Adjust the field path to match your schema.
- Operational: add a unique index on { 'federation.origin': 1, 'federation.mediaId': 1 }.
- Verify: I could not find any federation.origin field in the repo — confirm the uploads collection field name for origin before applying the query/index.
Files: packages/federation-sdk/src/services/event-authorization.service.ts, packages/federation-sdk/src/repositories/upload.repository.ts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
299-304: ACL semantics: distinguish missing vs emptyallow(deny-all only when explicitly empty).Apply:
- const serverAclContent = aclEvent.getContent(); - const { - allow = [], - deny = [], - allow_ip_literals = true, - } = serverAclContent; + const serverAclContent = aclEvent.getContent() as { + allow?: string[]; + deny?: string[]; + allow_ip_literals?: boolean; + }; + const hasAllow = Object.prototype.hasOwnProperty.call(serverAclContent, 'allow'); + const allow = hasAllow ? serverAclContent.allow ?? [] : ['*']; + const deny = serverAclContent.deny ?? []; + const allow_ip_literals = serverAclContent.allow_ip_literals ?? true;This keeps “deny-all” only when
allow: []is explicitly set while treating missingallowas allow-all.Matrix spec question: In m.room.server_acl, if `allow` is missing, is the default equivalent to ["*"] (allow-all), and if `allow` is present but empty, does it deny-all?Also applies to: 322-327
342-361: ReDoS risk: replace dynamic RegExp with linear-time wildcard matcherReplace with safe '*'/'?' matcher:
private matchesServerPattern(serverName: string, pattern: string): boolean { - if (serverName === pattern) { - return true; - } - - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*') - .replace(/\?/g, '.'); - - regexPattern = `^${regexPattern}$`; - - try { - const regex = new RegExp(regexPattern); - return regex.test(serverName); - } catch (error) { - this.logger.warn(`Invalid ACL pattern: ${pattern}`, error); - return false; - } + if (serverName === pattern) return true; + let s = 0, p = 0, star = -1, match = 0; + while (s < serverName.length) { + if (p < pattern.length && (pattern[p] === '?' || pattern[p] === serverName[s])) { + s++; p++; + } else if (p < pattern.length && pattern[p] === '*') { + star = p++; match = s; + } else if (star !== -1) { + p = star + 1; s = ++match; + } else { + return false; + } + } + while (p < pattern.length && pattern[p] === '*') p++; + return p === pattern.length; }
363-423: MediaId collision risk across origins — require origin in lookupUse origin+mediaId to resolve the RC room; otherwise identical mediaIds from different origins can be misattributed.
- // TODO duplicated from canAccessEvent. need to refactor into a common method - async canAccessMedia(mediaId: string, serverName: string): Promise<boolean> { + // TODO duplicated from canAccessEvent. need to refactor into a common method + async canAccessMedia( + mediaId: string, + originServerName: string, + requesterServerName: string, + ): Promise<boolean> { try { - const rcRoomId = - await this.uploadRepository.findRocketChatRoomIdByMediaId(mediaId); + const rcRoomId = await this.uploadRepository + .findRocketChatRoomIdByOriginAndMediaId(originServerName, mediaId); if (!rcRoomId) { - this.logger.debug(`Media ${mediaId} not found in any room`); + this.logger.debug(`Media ${originServerName}/${mediaId} not mapped to any RC room`); return false; } const matrixRoomId = await this.matrixBridgedRoomRepository.findMatrixRoomId(rcRoomId); if (!matrixRoomId) { - this.logger.debug(`Media ${mediaId} not found in any room`); + this.logger.debug(`No Matrix mapping for RC room ${rcRoomId} (media ${originServerName}/${mediaId})`); return false; } const state = await this.stateService.getFullRoomState(matrixRoomId); const aclEvent = state.get('m.room.server_acl:'); - const isServerAllowed = await this.checkServerAcl(aclEvent, serverName); + const isServerAllowed = await this.checkServerAcl(aclEvent, requesterServerName); if (!isServerAllowed) { this.logger.warn( - `Server ${serverName} is denied by room ACL for media in room ${matrixRoomId}`, + `Server ${requesterServerName} is denied by room ACL for media in room ${matrixRoomId}`, ); return false; } const serversInRoom = await this.stateService.getServersInRoom(matrixRoomId); - if (serversInRoom.includes(serverName)) { + if (serversInRoom.includes(requesterServerName)) { this.logger.debug( - `Server ${serverName} is in room ${matrixRoomId}, allowing media access`, + `Server ${requesterServerName} is in room ${matrixRoomId}, allowing media access`, ); return true; } const historyVisibilityEvent = state.get('m.room.history_visibility:'); if ( historyVisibilityEvent?.isHistoryVisibilityEvent() && historyVisibilityEvent.getContent().history_visibility === 'world_readable' ) { this.logger.debug( - `Room ${matrixRoomId} is world_readable, allowing media access to ${serverName}`, + `Room ${matrixRoomId} is world_readable, allowing media access to ${requesterServerName}`, ); return true; } this.logger.debug( - `Server ${serverName} not authorized for media ${mediaId}: not in room and room not world_readable`, + `Server ${requesterServerName} not authorized for media ${originServerName}/${mediaId}: not in room and room not world_readable`, ); return false; } catch (error) { this.logger.error( - { error, mediaId, serverName }, + { error, mediaId, originServerName, requesterServerName }, 'Error checking media access', ); return false; } }Follow-up in repository (outside this file):
- Add UploadRepository.findRocketChatRoomIdByOriginAndMediaId(origin: string, mediaId: string).
I can draft the repo change and update callers if you want.
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
233-235: Unify logger.error call shape (meta first, message second)Elsewhere you pass meta first; here it’s reversed and loses context fields.
Apply:
- } catch (err) { - this.logger.error('Error checking event access', err); + } catch (error) { + this.logger.error({ error, eventId, serverName }, 'Error checking event access');
363-423: De-duplicate auth logic as per TODOBoth media and event access share: fetch state → check ACL → check membership → fallback to world_readable. Extract a private helper to reduce drift and improve testability.
Also applies to: 425-474
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
PR: RocketChat/homeserver#184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🔇 Additional comments (3)
packages/federation-sdk/src/services/event-authorization.service.ts (3)
12-13: Repo imports for media authorization — LGTMImporting MatrixBridgedRoomRepository and UploadRepository here makes sense for the new media access flow.
26-29: Constructor DI change — no action required (tsyringe injects by type)container.ts registers KeyRepository, MatrixBridgedRoomRepository, UploadRepository and EventAuthorizationService, and EventAuthorizationService constructor uses class-typed params (no @Inject tokens), so tsyringe will resolve by type and the added parameters are order-insensitive.
See packages/federation-sdk/src/container.ts:109-115 and packages/federation-sdk/src/services/event-authorization.service.ts:22-29.
425-474: Pass origin to media auth API (and propagate to lookup)Require the media origin on the header-based entry point and forward both media origin and requester to canAccessMedia (alternatively parse origin from uri, but explicit param is clearer).
- async canAccessMediaFromAuthorizationHeader( - mediaId: string, - authorizationHeader: string, - method: string, - uri: string, - body?: Record<string, unknown>, - ): Promise< + async canAccessMediaFromAuthorizationHeader( + mediaId: string, + originServerName: string, + authorizationHeader: string, + method: string, + uri: string, + body?: Record<string, unknown>, + ): Promise< | { authorized: true } | { authorized: false; errorCode: 'M_UNAUTHORIZED' | 'M_FORBIDDEN' | 'M_UNKNOWN'; } > { @@ - const authorized = await this.canAccessMedia(mediaId, signatureResult); + const authorized = await this.canAccessMedia( + mediaId, + originServerName, + signatureResult, // requester server + );Confirm/update all media-route call sites to pass originServerName (e.g. federation media download controllers).
Works along with RocketChat/Rocket.Chat#36842.
Note: The media controller was removed from the homeserver side since we don’t have a local file store and it can only operate alongside Rocket.Chat.
Summary by CodeRabbit
New Features
Improvements
Removed