feat: adds Federation canAccessMedia middleware #36926
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds a canAccessMedia middleware for federation media routes, wires it into download and thumbnail endpoints, returns 401/403/500 Matrix-style errors when appropriate, adds a null-content guard in remote file download, and updates route error schemas. No public API signatures change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant M as Media API Route
participant MW as canAccessMedia (middleware)
participant A as EventAuthorizationService
C->>M: Request /v1/media/(download|thumbnail)/:mediaId
M->>MW: Invoke middleware
MW->>MW: Build signature URI (path + query [+ body if non-GET])
MW->>A: canAccessMediaFromAuthorizationHeader(mediaId, auth, method, uri, body)
alt Authorized
A-->>MW: allowed
MW-->>M: next()
M-->>C: Proceed to handler (stream/thumbnail)
else Unauthorized
A-->>MW: unauthorized
MW-->>C: 401 { errcode: M_UNAUTHORIZED, error }
else Forbidden
A-->>MW: forbidden
MW-->>C: 403 { errcode: M_FORBIDDEN, error }
else Error
A-->>MW: throws/error
MW-->>C: 500 { errcode: M_UNKNOWN, error }
end
note over M: Download route catch no longer logs errors\nThumbnail route catch still logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ee/packages/federation-matrix/src/api/middlewares.ts (2)
38-38: Avoid JSON-only parsing for non-GET; use raw text and gate by method.HEAD/DELETE or non-JSON bodies will throw on json().
Apply:
- const body = method === 'GET' ? undefined : await c.req.json(); + const body = + method === 'GET' || method === 'HEAD' ? undefined : await c.req.text();
5-21: Tighten typing of errCodes.Use a discriminated constant map to prevent typos and enable exhaustiveness.
Apply:
-const errCodes: Record<string, { errcode: string; error: string; status: ContentfulStatusCode }> = { +const errCodes = { M_UNAUTHORIZED: { errcode: 'M_UNAUTHORIZED', error: 'Invalid or missing signature', status: 401 as ContentfulStatusCode, }, M_FORBIDDEN: { errcode: 'M_FORBIDDEN', error: 'Access denied', status: 403 as ContentfulStatusCode, }, M_UNKNOWN: { errcode: 'M_UNKNOWN', error: 'Internal server error while processing request', status: 500 as ContentfulStatusCode, }, -}; +} as const;ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
149-149: Unify handler param typing.Use the same variable/type as the download route for consistency.
Apply:
- async (context: any) => { + async (c) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ee/packages/federation-matrix/src/api/_matrix/media.ts(4 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
ee/packages/federation-matrix/src/api/middlewares.ts (1)
canAccessMedia(23-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/federation-matrix/src/api/_matrix/media.ts (2)
97-98: Middleware integration LGTM.Applying canAccessMedia before handlers on both routes is correct.
Also applies to: 148-149
127-132: Confirm logging intent (inconsistent between routes).Download no longer logs on 500; thumbnail still logs. If noise reduction was intentional, consider debug-level logging in both or neither for consistency.
Also applies to: 178-184
| 200: isBufferResponseProps, | ||
| 401: isErrorResponseProps, | ||
| 403: isErrorResponseProps, | ||
| 404: isErrorResponseProps, | ||
| }, |
There was a problem hiding this comment.
Thumbnail route schema missing 500 while handler returns 500.
This mismatches declared responses and can fail validation.
Apply:
response: {
+ 500: isErrorResponseProps,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 200: isBufferResponseProps, | |
| 401: isErrorResponseProps, | |
| 403: isErrorResponseProps, | |
| 404: isErrorResponseProps, | |
| }, | |
| 200: isBufferResponseProps, | |
| 401: isErrorResponseProps, | |
| 403: isErrorResponseProps, | |
| 404: isErrorResponseProps, | |
| 500: isErrorResponseProps, | |
| }, |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/media.ts around lines 141 to
145, the thumbnail route's response schema declares 200/401/403/404 but omits
500 while the handler can return 500; add a 500 entry mapping to
isErrorResponseProps in the responses object for the thumbnail route so the
schema matches the handler and validation passes.
| const { path } = c.req; | ||
| const query = c.req.query(); | ||
|
|
||
| let uriForSignature = path; | ||
| const queryString = new URLSearchParams(query).toString(); | ||
| if (queryString) { | ||
| uriForSignature = `${path}?${queryString}`; | ||
| } |
There was a problem hiding this comment.
Preserve raw query string (ordering and duplicates) for signature verification.
Rebuilding the query via URLSearchParams can reorder keys and drop multi-valued params, causing signature mismatches. Use the raw URL.
Apply:
- const { path } = c.req;
- const query = c.req.query();
-
- let uriForSignature = path;
- const queryString = new URLSearchParams(query).toString();
- if (queryString) {
- uriForSignature = `${path}?${queryString}`;
- }
+ const url = new URL(c.req.url);
+ const uriForSignature = `${url.pathname}${url.search}`;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/middlewares.ts around lines 28-35, the
code rebuilds the query string with URLSearchParams which can reorder keys and
drop repeated params; instead obtain the raw request URL including its original
query string (e.g. use c.req.url or c.req.rawUrl / c.req.originalUrl depending
on the framework) and use that for uriForSignature; if the raw URL may include
the origin, strip the origin leaving path + query, and also strip any fragment
(#...) before using it for signature verification so ordering and duplicates are
preserved.
| if (!verificationResult.authorized) { | ||
| const errorResponse = errCodes[verificationResult.errorCode]; | ||
| return c.json( | ||
| { | ||
| errcode: errorResponse.errcode, | ||
| error: errorResponse.error, | ||
| }, | ||
| errorResponse.status, | ||
| ); |
There was a problem hiding this comment.
Handle unknown errorCode to avoid runtime crash.
If the service returns an unexpected code, errorResponse is undefined and accessing its fields throws.
Apply:
- if (!verificationResult.authorized) {
- const errorResponse = errCodes[verificationResult.errorCode];
+ if (!verificationResult.authorized) {
+ const errorResponse = errCodes[verificationResult.errorCode] ?? errCodes.M_FORBIDDEN;
return c.json(
{
errcode: errorResponse.errcode,
error: errorResponse.error,
},
errorResponse.status,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!verificationResult.authorized) { | |
| const errorResponse = errCodes[verificationResult.errorCode]; | |
| return c.json( | |
| { | |
| errcode: errorResponse.errcode, | |
| error: errorResponse.error, | |
| }, | |
| errorResponse.status, | |
| ); | |
| if (!verificationResult.authorized) { | |
| const errorResponse = errCodes[verificationResult.errorCode] ?? errCodes.M_FORBIDDEN; | |
| return c.json( | |
| { | |
| errcode: errorResponse.errcode, | |
| error: errorResponse.error, | |
| }, | |
| errorResponse.status, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/middlewares.ts around lines 48 to 56,
the code assumes errCodes[verificationResult.errorCode] always exists and will
crash if undefined; guard against missing errorResponse by checking its
existence and using a sensible default (e.g., generic errcode, error message and
500 status) or map unknown codes to a fallback entry, then return that fallback
instead; optionally log the unexpected errorCode for debugging before returning.
| } catch (error) { | ||
| return c.json(errCodes.M_UNKNOWN, 500); | ||
| } |
There was a problem hiding this comment.
Return only Matrix error fields; drop extra “status” property.
Body currently includes “status,” which violates the route error schema.
Apply:
- } catch (error) {
- return c.json(errCodes.M_UNKNOWN, 500);
- }
+ } catch (error) {
+ return c.json({ errcode: errCodes.M_UNKNOWN.errcode, error: errCodes.M_UNKNOWN.error }, 500);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| return c.json(errCodes.M_UNKNOWN, 500); | |
| } | |
| } catch (error) { | |
| return c.json({ errcode: errCodes.M_UNKNOWN.errcode, error: errCodes.M_UNKNOWN.error }, 500); | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/middlewares.ts around lines 60 to 62,
the catch block returns a response body containing an extra "status" property
which violates the Matrix route error schema; update the error response so the
JSON body contains only the Matrix error fields (errcode and error) and remove
any "status" field while still returning the appropriate HTTP status code (500).
Ensure you build the response object with only the Matrix-standard keys and send
that with the 500 status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
115-117: Defensive check is good; also guard against zero-length and wrong type.Today this only catches null/undefined. Add a Buffer-type assertion and zero-length check to avoid uploading empty/invalid payloads.
- if (!buffer) { - throw new Error('Download from remote server returned null content.'); - } + if (!buffer || !Buffer.isBuffer(buffer) || buffer.length === 0) { + throw new Error( + `Download from remote server returned empty or invalid content for ${parts.serverName}/${parts.mediaId}.`, + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
apps/meteor/server/startup/initialData.js (1)
buffer(148-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Works along with RocketChat/homeserver#185.
For anyone testing with Synapse as HS1, remember to add these two properties to the YAML spec file:
Summary by CodeRabbit