Skip to content

feat: adds Federation canAccessMedia middleware #36926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ricardogarim merged 3 commits into from
Sep 13, 2025
Merged

feat: adds Federation canAccessMedia middleware #36926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b399d40
feat: adds canAccessMedia middleware
ricardogarim Sep 12, 2025
5343239
refactor: rewrites the way in which signature is validated
ricardogarim Sep 12, 2025
9186a1e
chore: adds empty response check into downloadFromRemoteServer
ricardogarim Sep 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ee/packages/federation-matrix/src/api/_matrix/media.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Logger } from '@rocket.chat/logger';
import { ajv } from '@rocket.chat/rest-typings/dist/v1/Ajv';

import { MatrixMediaService } from '../../services/MatrixMediaService';
import { canAccessMedia } from '../middlewares';

const logger = new Logger('federation-matrix:media');

Expand Down Expand Up @@ -76,7 +77,7 @@ async function getMediaFile(mediaId: string, serverName: string): Promise<{ file
}

export const getMatrixMediaRoutes = (homeserverServices: HomeserverServices) => {
const { config } = homeserverServices;
const { config, federationAuth } = homeserverServices;
const router = new Router('/federation');

router.get(
Expand All @@ -86,12 +87,14 @@ export const getMatrixMediaRoutes = (homeserverServices: HomeserverServices) =>
response: {
200: isBufferResponseProps,
401: isErrorResponseProps,
403: isErrorResponseProps,
404: isErrorResponseProps,
429: isErrorResponseProps,
500: isErrorResponseProps,
},
tags: ['Federation', 'Media'],
},
canAccessMedia(federationAuth),
async (c) => {
try {
const { mediaId } = c.req.param();
Expand Down Expand Up @@ -122,7 +125,6 @@ export const getMatrixMediaRoutes = (homeserverServices: HomeserverServices) =>
body: multipartResponse.body,
};
} catch (error) {
logger.error('Federation media download error:', error);
return {
statusCode: 500,
body: { errcode: 'M_UNKNOWN', error: 'Internal server error' },
Expand All @@ -138,10 +140,12 @@ export const getMatrixMediaRoutes = (homeserverServices: HomeserverServices) =>
response: {
200: isBufferResponseProps,
401: isErrorResponseProps,
403: isErrorResponseProps,
404: isErrorResponseProps,
},
Comment on lines 141 to 145
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.

tags: ['Federation', 'Media'],
},
canAccessMedia(federationAuth),
async (context: any) => {
try {
const { mediaId } = context.req.param();
Expand Down
64 changes: 64 additions & 0 deletions ee/packages/federation-matrix/src/api/middlewares.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import type { EventAuthorizationService } from '@hs/federation-sdk';
import type { Context, Next } from 'hono';
import type { ContentfulStatusCode } from 'hono/utils/http-status';

const errCodes: Record<string, { errcode: string; error: string; status: ContentfulStatusCode }> = {
M_UNAUTHORIZED: {
errcode: 'M_UNAUTHORIZED',
error: 'Invalid or missing signature',
status: 401,
},
M_FORBIDDEN: {
errcode: 'M_FORBIDDEN',
error: 'Access denied',
status: 403,
},
M_UNKNOWN: {
errcode: 'M_UNKNOWN',
error: 'Internal server error while processing request',
status: 500,
},
};

export const canAccessMedia = (federationAuth: EventAuthorizationService) => {
return async (c: Context, next: Next) => {
const mediaId = c.req.param('mediaId');
const authHeader = c.req.header('Authorization') || '';
const { method } = c.req;
const { path } = c.req;
const query = c.req.query();

let uriForSignature = path;
const queryString = new URLSearchParams(query).toString();
if (queryString) {
uriForSignature = `${path}?${queryString}`;
}
Comment on lines +28 to +35
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.


try {
const body = method === 'GET' ? undefined : await c.req.json();

const verificationResult = await federationAuth.canAccessMediaFromAuthorizationHeader(
mediaId,
authHeader,
method,
uriForSignature, // use URI with query params for signature verification
body,
);

if (!verificationResult.authorized) {
const errorResponse = errCodes[verificationResult.errorCode];
return c.json(
{
errcode: errorResponse.errcode,
error: errorResponse.error,
},
errorResponse.status,
);
Comment on lines +48 to +56
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.

}

return next();
} catch (error) {
return c.json(errCodes.M_UNKNOWN, 500);
}
Comment on lines +60 to +62
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
} 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.

};
};
3 changes: 3 additions & 0 deletions ee/packages/federation-matrix/src/services/MatrixMediaService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ export class MatrixMediaService {
}

const buffer = await this.homeserverServices.media.downloadFromRemoteServer(parts.serverName, parts.mediaId);
if (!buffer) {
throw new Error('Download from remote server returned null content.');
}

const uploadedFile = await Upload.uploadFile({
userId: metadata.userId || 'federation',
Expand Down
Loading