feat: adds get state and state_id endpoints #36929
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 AJV schemas/validators and two new GET endpoints to the Matrix federation transactions routes for fetching room state IDs and state by event_id, wiring them to the existing event service. No exported/public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Transactions Routes
participant V as Validators (AJV)
participant E as Event Service
rect rgb(238,246,255)
note right of C: GET /v1/state_ids/:roomId?event_id=...
C->>R: HTTP GET /v1/state_ids/:roomId
R->>V: Validate params (event_id)
V-->>R: Params OK / 400 on fail
alt missing event_id
R-->>C: 404 { errcode: "M_NOT_FOUND" }
else event_id present
R->>E: getStateIds(roomId, event_id)
E-->>R: { stateIds: string[] }
R->>V: Validate 200 response
V-->>R: OK
R-->>C: 200 { stateIds }
end
end
rect rgb(241,255,238)
note right of C: GET /v1/state/:roomId?event_id=...
C->>R: HTTP GET /v1/state/:roomId
R->>V: Validate params (event_id)
V-->>R: Params OK / 400 on fail
alt missing event_id
R-->>C: 404 { errcode: "M_NOT_FOUND" }
else event_id present
R->>E: getState(roomId, event_id)
E-->>R: { state: object }
R->>V: Validate 200 response
V-->>R: OK
R-->>C: 200 { state }
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
86-86: Bug: enableThumbnails is always trueThe boolean expression forces true regardless of env. Fix to respect env.
Apply:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: + process.env.MEDIA_ENABLE_THUMBNAILS === undefined + ? true + : process.env.MEDIA_ENABLE_THUMBNAILS === 'true',ee/apps/federation-service/src/service.ts (1)
42-45: Missing await: license check always truthyLicense.hasModule is used with await elsewhere; without await this branch will never throw. Fix to ensure proper gating.
Apply:
- const hasLicense = License.hasModule('federation'); + const hasLicense = await License.hasModule('federation');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
apps/meteor/ee/server/startup/federation.ts(2 hunks)ee/apps/federation-service/package.json(1 hunks)ee/apps/federation-service/src/service.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/transactions.ts(2 hunks)ee/packages/federation-matrix/src/api/middlewares.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-services/src/index.ts (1)
FederationMatrix(194-194)
apps/meteor/ee/server/startup/federation.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(27-777)packages/models/src/index.ts (1)
InstanceStatus(165-165)
ee/apps/federation-service/src/service.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(27-777)packages/models/src/index.ts (1)
InstanceStatus(165-165)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
packages/http-router/src/Router.ts (1)
Router(78-412)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/api/_matrix/transactions.ts
[error] 378-378: TS2339: Property 'getStateIds' does not exist on type 'ProfilesService'.
🔇 Additional comments (3)
ee/apps/federation-service/package.json (1)
30-30: LGTM: dependency added correctlyAdding "@rocket.chat/instance-status" as a workspace dependency aligns with new usage in service/startup. No concerns.
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
45-46: No action required — InstanceStatus.id() is synchronous and call sites pass a string. InstanceStatus.id() (packages/instance-status/src/index.ts) returns a UUID string; FederationMatrix.create usages in ee/apps/federation-service/src/service.ts and apps/meteor/ee/server/startup/federation.ts are correct.ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
274-279: Fix compile error: use the correct service for state APIs (profile lacks getState/getStateIds)Repo search for getStateIds/getState returned no matches — I cannot confirm which service exposes those methods. Update the destructure in ee/packages/federation-matrix/src/api/_matrix/transactions.ts to use the service that provides state APIs (likely state or room) and replace profile.getStateIds/profile.getState at ~lines 274–279 and 376–401 accordingly. To verify, run: rg -nP 'getStateIds\s*(|getState\s*(' packages -C2.
|
|
||
| logger.debug('Starting federation-matrix service'); | ||
| federationMatrixService = await FederationMatrix.create(); | ||
| federationMatrixService = await FederationMatrix.create(InstanceStatus.id()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap create() and handle possibly-async InstanceStatus.id()
Avoid startup crash if create() throws and support async/sync id(). Also improves logging.
Apply:
- federationMatrixService = await FederationMatrix.create(InstanceStatus.id());
+ try {
+ const instanceId = await Promise.resolve(InstanceStatus.id());
+ federationMatrixService = await FederationMatrix.create(instanceId);
+ } catch (error) {
+ logger.error('Failed to create federation-matrix service:', error);
+ return;
+ }📝 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.
| federationMatrixService = await FederationMatrix.create(InstanceStatus.id()); | |
| try { | |
| const instanceId = await Promise.resolve(InstanceStatus.id()); | |
| federationMatrixService = await FederationMatrix.create(instanceId); | |
| } catch (error) { | |
| logger.error('Failed to create federation-matrix service:', error); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/ee/server/startup/federation.ts around line 28, ensure
InstanceStatus.id() is awaited (it may be async) into a local const before
calling FederationMatrix.create, and wrap the create call in a try/catch so any
thrown errors (sync or async) don’t crash startup; on error log a clear message
with the error and set federationMatrixService to null/undefined or otherwise
handle the failure path.
| const federationMatrix = await FederationMatrix.create(InstanceStatus.id()); | ||
| api.registerService(federationMatrix); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safer create(): support async InstanceStatus.id()
Handle both sync/async return and keep errors localized.
Apply:
- const federationMatrix = await FederationMatrix.create(InstanceStatus.id());
+ const federationMatrix = await FederationMatrix.create(await Promise.resolve(InstanceStatus.id()));📝 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.
| const federationMatrix = await FederationMatrix.create(InstanceStatus.id()); | |
| api.registerService(federationMatrix); | |
| const federationMatrix = await FederationMatrix.create(await Promise.resolve(InstanceStatus.id())); | |
| api.registerService(federationMatrix); |
🤖 Prompt for AI Agents
In ee/apps/federation-service/src/service.ts around lines 53–54, resolve
InstanceStatus.id() whether it returns a value or a Promise and keep errors
localized: first await the id (e.g. const instanceId = await
InstanceStatus.id()) inside a small try/catch to handle errors from fetching the
id, then pass the resolved instanceId into FederationMatrix.create() in its own
try/catch so errors creating the matrix are handled separately; update
api.registerService to use the created federationMatrix only after successful
creation.
| .post( | ||
| '/test', | ||
| { | ||
| response: { | ||
| 200: isPostTestBodyProps, | ||
| }, | ||
| }, | ||
| async (c) => { | ||
| try { | ||
| const { url, params } = await c.req.json(); | ||
|
|
||
| return { | ||
| statusCode: 200, | ||
| body: await request.get('hs1', url, params), | ||
| }; | ||
| } catch (error) { | ||
| console.log('error', error); | ||
| return { | ||
| statusCode: 500, | ||
| body: { | ||
| error, | ||
| }, | ||
| }; | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Remove /federation/test endpoint or guard behind TEST_MODE
Endpoint returns arbitrary upstream data and misuses response schema; don’t ship it.
Apply:
- .post(
- '/test',
- {
- response: {
- 200: isPostTestBodyProps,
- },
- },
- async (c) => {
- try {
- const { url, params } = await c.req.json();
-
- return {
- statusCode: 200,
- body: await request.get('hs1', url, params),
- };
- } catch (error) {
- console.log('error', error);
- return {
- statusCode: 500,
- body: {
- error,
- },
- };
- }
- },
- )Also delete PostTestBodySchema and its validator.
-const PostTestBodySchema = { ... };
-const isPostTestBodyProps = ajv.compile(PostTestBodySchema);📝 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.
| .post( | |
| '/test', | |
| { | |
| response: { | |
| 200: isPostTestBodyProps, | |
| }, | |
| }, | |
| async (c) => { | |
| try { | |
| const { url, params } = await c.req.json(); | |
| return { | |
| statusCode: 200, | |
| body: await request.get('hs1', url, params), | |
| }; | |
| } catch (error) { | |
| console.log('error', error); | |
| return { | |
| statusCode: 500, | |
| body: { | |
| error, | |
| }, | |
| }; | |
| } | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/transactions.ts around lines
293 to 318, remove the /test POST handler or wrap it behind a TEST_MODE feature
flag so it is not exposed in production; if using a flag, check
process.env.TEST_MODE (or existing config) before registering the route and
return 404/403 when disabled. Also delete the PostTestBodySchema and its
validator usage and any imports/exports solely related to that schema to avoid
dead code and ensure response schemas match actual returned data; update route
registration code to preserve surrounding flow after removal or guarding.
| '/v1/send/:txnId', | ||
| { | ||
| params: isSendTransactionParamsProps, | ||
| body: isSendTransactionBodyProps, | ||
| response: { | ||
| 200: isSendTransactionResponseProps, | ||
| 400: isErrorResponseProps, | ||
| }, | ||
| tags: ['Federation'], | ||
| license: ['federation'], | ||
| }, | ||
| async (c) => { | ||
| const body = await c.req.json(); | ||
|
|
||
| try { | ||
| await event.processIncomingTransaction(body); | ||
| } catch (error: any) { | ||
| // TODO custom error types? | ||
| if (error.message === 'too-many-concurrent-transactions') { | ||
| return { | ||
| statusCode: 429, | ||
| body: { | ||
| errorcode: 'M_UNKNOWN', | ||
| error: 'Too many concurrent transactions', | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| statusCode: 400, | ||
| body: {}, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error responses don’t match declared 400 schema
You declare 400: isErrorResponseProps but return {}. Return a consistent shape.
Apply:
- return {
- statusCode: 400,
- body: {},
- };
+ return {
+ statusCode: 400,
+ body: {
+ error: 'bad-request',
+ details: { message: (error as Error).message },
+ },
+ };Optionally add 429 to response map and reuse the same error schema.
📝 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.
| '/v1/send/:txnId', | |
| { | |
| params: isSendTransactionParamsProps, | |
| body: isSendTransactionBodyProps, | |
| response: { | |
| 200: isSendTransactionResponseProps, | |
| 400: isErrorResponseProps, | |
| }, | |
| tags: ['Federation'], | |
| license: ['federation'], | |
| }, | |
| async (c) => { | |
| const body = await c.req.json(); | |
| try { | |
| await event.processIncomingTransaction(body); | |
| } catch (error: any) { | |
| // TODO custom error types? | |
| if (error.message === 'too-many-concurrent-transactions') { | |
| return { | |
| statusCode: 429, | |
| body: { | |
| errorcode: 'M_UNKNOWN', | |
| error: 'Too many concurrent transactions', | |
| }, | |
| }; | |
| } | |
| return { | |
| statusCode: 400, | |
| body: {}, | |
| }; | |
| } | |
| '/v1/send/:txnId', | |
| { | |
| params: isSendTransactionParamsProps, | |
| body: isSendTransactionBodyProps, | |
| response: { | |
| 200: isSendTransactionResponseProps, | |
| 400: isErrorResponseProps, | |
| }, | |
| tags: ['Federation'], | |
| license: ['federation'], | |
| }, | |
| async (c) => { | |
| const body = await c.req.json(); | |
| try { | |
| await event.processIncomingTransaction(body); | |
| } catch (error: any) { | |
| // TODO custom error types? | |
| if (error.message === 'too-many-concurrent-transactions') { | |
| return { | |
| statusCode: 429, | |
| body: { | |
| errorcode: 'M_UNKNOWN', | |
| error: 'Too many concurrent transactions', | |
| }, | |
| }; | |
| } | |
| return { | |
| statusCode: 400, | |
| body: { | |
| error: 'bad-request', | |
| details: { message: (error as Error).message }, | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/transactions.ts around lines
320-352 the handler declares 400: isErrorResponseProps but returns an empty
object and therefore violates the response schema; update both error paths to
return the error shape defined by isErrorResponseProps (e.g. include errorcode
and error fields) and change the response map to also include 429 with the same
error schema for the too-many-concurrent-transactions case; ensure the generic
catch returns a 400 status with a populated error object and the specific branch
returns statusCode 429 with an error object matching isErrorResponseProps.
| if (error.message === 'too-many-concurrent-transactions') { | ||
| return { | ||
| statusCode: 429, | ||
| body: { | ||
| errorcode: 'M_UNKNOWN', | ||
| error: 'Too many concurrent transactions', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Typo in Matrix error shape: use errcode
Matrix errors use errcode. Current payload uses errorcode.
Apply:
- errorcode: 'M_UNKNOWN',
+ errcode: 'M_UNKNOWN',📝 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 (error.message === 'too-many-concurrent-transactions') { | |
| return { | |
| statusCode: 429, | |
| body: { | |
| errorcode: 'M_UNKNOWN', | |
| error: 'Too many concurrent transactions', | |
| }, | |
| }; | |
| if (error.message === 'too-many-concurrent-transactions') { | |
| return { | |
| statusCode: 429, | |
| body: { | |
| errcode: 'M_UNKNOWN', | |
| error: 'Too many concurrent transactions', | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/transactions.ts around lines
338-345 the error response object uses the incorrect key "errorcode"; replace
that key with "errcode" so the body has errcode: 'M_UNKNOWN' along with the
existing error message and statusCode 429; keep the rest of the payload
unchanged.
| { | ||
| params: isGetStateIdsParamsProps, | ||
| response: { | ||
| 200: isGetStateIdsResponseProps, | ||
| }, | ||
| }, | ||
| async (c) => { |
There was a problem hiding this comment.
Use query validators (not params) for event_id
event_id is a query param; current code places its validator under params so it never runs. Move to options.query.
Apply:
- {
- params: isGetStateIdsParamsProps,
+ {
+ query: isGetStateIdsParamsProps,
response: {
},
},And for state:
- {
- params: isGetStateParamsProps,
+ {
+ query: isGetStateParamsProps,
response: {
},
},Also applies to: 387-394
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/transactions.ts around lines
368-374 (and similarly around 387-394), the schema validators for event_id (and
the state validator) are incorrectly placed under params so they are never
applied to query parameters; move those validators into the options.query object
for the route definitions (replace params: isGetStateIdsParamsProps with query:
isGetStateIdsParamsProps or add query: { event_id: ... } as appropriate) and
remove or adjust the params entry so only path/URL params remain validated under
params.
| return { | ||
| body: stateIds, | ||
| statusCode: 200, | ||
| }; |
There was a problem hiding this comment.
Response body shape mismatch with declared schemas
- state_ids route should return { stateIds }
- state route should return { state }
Apply:
- return {
- body: stateIds,
- statusCode: 200,
- };
+ return {
+ body: { stateIds },
+ statusCode: 200,
+ };- return {
- statusCode: 200,
- body: state,
- };
+ return {
+ statusCode: 200,
+ body: { state },
+ };Also applies to: 399-401
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/transactions.ts around lines
380-383 (and similarly lines 399-401), the handlers are returning the raw
variables (e.g. body: stateIds) which does not match the declared response
schema; wrap the values in objects with the expected property names — return {
body: { stateIds } , statusCode: 200 } for the state_ids route and return {
body: { state } , statusCode: 200 } for the state route — ensure the response
keys exactly match the schemas.
| // GET /_matrix/federation/v1/event/{eventId} | ||
| .get( | ||
| '/v1/event/:eventId', | ||
| { | ||
| params: isGetEventParamsProps, | ||
| response: { | ||
| 200: isGetEventResponseProps, | ||
| }, | ||
| tags: ['Federation'], | ||
| license: ['federation'], | ||
| }, | ||
| async (c) => { | ||
| const eventData = await event.getEventById(c.req.param('eventId')); | ||
| if (!eventData) { | ||
| return { | ||
| body: { | ||
| errcode: 'M_NOT_FOUND', | ||
| error: 'Event not found', | ||
| }, | ||
| statusCode: 404, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| body: { | ||
| origin_server_ts: eventData.event.origin_server_ts, | ||
| origin: config.serverName, | ||
| pdus: [eventData.event], | ||
| }, | ||
| statusCode: 200, | ||
| }; | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ACL middleware to protected federation reads
Per PR title, guard event/state endpoints with signature+ACL verification.
After enabling export in middlewares.ts, wire it here (example using services.auth/eventAuth—adjust per SDK):
+ // Example: apply ACL middleware
+ // .use(aclMiddleware(services.auth))
.get(
'/v1/event/:eventId',If Router.use per-subrouter isn’t desired, pass middleware as an extra argument before the handler:
- .get('/v1/event/:eventId', { ... }, async (c) => { ... })
+ .get('/v1/event/:eventId', { ... }, aclMiddleware(services.auth), async (c) => { ... })Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/transactions.ts around lines
404 to 436, the GET /_matrix/federation/v1/event/:eventId route is currently
unprotected; add the ACL/signature verification middleware to this endpoint so
reads are guarded. Import the exported middleware from middlewares.ts (or use
services.auth.eventAuth / signature check from the SDK) and attach it to the
route—either by applying Router.use on the subrouter that contains this route or
by passing the middleware as an extra argument before the async handler—so the
request is validated (signature + ACL) before resolving getEventById and
returning the event or 404.
| // import type { EventAuthorizationService } from '@hs/federation-sdk'; | ||
| // import type { Context, Next } from 'hono'; | ||
|
|
||
| // const errCodes = { | ||
| // M_UNAUTHORIZED: { | ||
| // errcode: 'M_UNAUTHORIZED', | ||
| // error: 'Invalid or missing signature', | ||
| // }, | ||
| // M_FORBIDDEN: { | ||
| // errcode: 'M_FORBIDDEN', | ||
| // error: 'Access denied', | ||
| // }, | ||
| // M_UNKNOWN: { | ||
| // errcode: 'M_UNKNOWN', | ||
| // error: 'Internal server error while processing request', | ||
| // }, | ||
| // }; | ||
|
|
||
| // export const aclMiddleware = (federationAuth: EventAuthorizationService) => async (c: Context, next: Next) => { | ||
| // const { eventId } = c.req.param(); | ||
|
|
||
| // try { | ||
| // const verificationResult = await federationAuth.verifyRequestSignature({ | ||
| // method: c.req.method, | ||
| // uri: c.req.path, | ||
| // headers: Object.fromEntries(c.req.raw.headers.entries()), | ||
| // body: undefined, // GET requests don't have body | ||
| // }); | ||
|
|
||
| // if (!verificationResult.valid) { | ||
| // return c.json(errCodes.M_UNAUTHORIZED, 401); | ||
| // } | ||
|
|
||
| // const authResult = await federationAuth.canAccessEvent(eventId, verificationResult.serverName); | ||
| // if (!authResult.authorized) { | ||
| // return c.json(errCodes.M_FORBIDDEN, 403); | ||
| // } | ||
|
|
||
| // return next(); | ||
| // } catch (error) { | ||
| // return c.json(errCodes.M_UNKNOWN, 500); | ||
| // } | ||
| // }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ACL middleware is fully commented-out; wire it or remove
PR title mentions ACL, but nothing is active. Either delete the file or export and use it on federation routes.
Apply to enable export:
-// import type { EventAuthorizationService } from '@hs/federation-sdk';
-// import type { Context, Next } from 'hono';
+import type { EventAuthorizationService } from '@hs/federation-sdk';
+import type { Context, Next } from 'hono';
@@
-// const errCodes = {
+const errCodes = {
@@
-// };
+};
@@
-// export const aclMiddleware = (federationAuth: EventAuthorizationService) => async (c: Context, next: Next) => {
+export const aclMiddleware = (federationAuth: EventAuthorizationService) => async (c: Context, next: Next) => {
@@
-// };
+};Then apply it on state/event endpoints (see comment in transactions.ts).
📝 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.
| // import type { EventAuthorizationService } from '@hs/federation-sdk'; | |
| // import type { Context, Next } from 'hono'; | |
| // const errCodes = { | |
| // M_UNAUTHORIZED: { | |
| // errcode: 'M_UNAUTHORIZED', | |
| // error: 'Invalid or missing signature', | |
| // }, | |
| // M_FORBIDDEN: { | |
| // errcode: 'M_FORBIDDEN', | |
| // error: 'Access denied', | |
| // }, | |
| // M_UNKNOWN: { | |
| // errcode: 'M_UNKNOWN', | |
| // error: 'Internal server error while processing request', | |
| // }, | |
| // }; | |
| // export const aclMiddleware = (federationAuth: EventAuthorizationService) => async (c: Context, next: Next) => { | |
| // const { eventId } = c.req.param(); | |
| // try { | |
| // const verificationResult = await federationAuth.verifyRequestSignature({ | |
| // method: c.req.method, | |
| // uri: c.req.path, | |
| // headers: Object.fromEntries(c.req.raw.headers.entries()), | |
| // body: undefined, // GET requests don't have body | |
| // }); | |
| // if (!verificationResult.valid) { | |
| // return c.json(errCodes.M_UNAUTHORIZED, 401); | |
| // } | |
| // const authResult = await federationAuth.canAccessEvent(eventId, verificationResult.serverName); | |
| // if (!authResult.authorized) { | |
| // return c.json(errCodes.M_FORBIDDEN, 403); | |
| // } | |
| // return next(); | |
| // } catch (error) { | |
| // return c.json(errCodes.M_UNKNOWN, 500); | |
| // } | |
| // }; | |
| import type { EventAuthorizationService } from '@hs/federation-sdk'; | |
| import type { Context, Next } from 'hono'; | |
| const errCodes = { | |
| M_UNAUTHORIZED: { | |
| errcode: 'M_UNAUTHORIZED', | |
| error: 'Invalid or missing signature', | |
| }, | |
| M_FORBIDDEN: { | |
| errcode: 'M_FORBIDDEN', | |
| error: 'Access denied', | |
| }, | |
| M_UNKNOWN: { | |
| errcode: 'M_UNKNOWN', | |
| error: 'Internal server error while processing request', | |
| }, | |
| }; | |
| export const aclMiddleware = (federationAuth: EventAuthorizationService) => async (c: Context, next: Next) => { | |
| const { eventId } = c.req.param(); | |
| try { | |
| const verificationResult = await federationAuth.verifyRequestSignature({ | |
| method: c.req.method, | |
| uri: c.req.path, | |
| headers: Object.fromEntries(c.req.raw.headers.entries()), | |
| body: undefined, // GET requests don't have body | |
| }); | |
| if (!verificationResult.valid) { | |
| return c.json(errCodes.M_UNAUTHORIZED, 401); | |
| } | |
| const authResult = await federationAuth.canAccessEvent(eventId, verificationResult.serverName); | |
| if (!authResult.authorized) { | |
| return c.json(errCodes.M_FORBIDDEN, 403); | |
| } | |
| return next(); | |
| } catch (error) { | |
| return c.json(errCodes.M_UNKNOWN, 500); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/middlewares.ts lines 1-43 the ACL
middleware is completely commented out which means it is not exported or applied
to federation routes; either remove the file or restore and wire the middleware.
To fix: uncomment and restore the aclMiddleware implementation, re-enable the
EventAuthorizationService and Hono Context/Next imports, export aclMiddleware,
and then import and apply it to the federation state/event endpoints (per
transactions.ts comment) by passing the federationAuth instance when registering
routes so GET requests are signature-verified and authorization checked; if you
prefer removal, delete the file and remove any references in route setup.
ggazzo
force-pushed
the
chore/fed-mine
branch
from
219fdda to
6e015ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (7)
276-291: Error responses mismatch schema/spec; fix errcode key and 400 body.Return an error shape (with details) and use errcode for Matrix-style errors.
- if (error.message === 'too-many-concurrent-transactions') { + if (error.message === 'too-many-concurrent-transactions') { return { statusCode: 429, body: { - errorcode: 'M_UNKNOWN', - error: 'Too many concurrent transactions', + errcode: 'M_LIMIT_EXCEEDED', + error: 'Too many concurrent transactions', + details: { message: error.message }, }, }; } return { statusCode: 400, - body: {}, + body: { + error: 'bad-request', + details: { message: (error as Error).message }, + }, };Optionally declare 429 in the route spec:
response: { - 400: isErrorResponseProps, + 400: isErrorResponseProps, + 429: isErrorResponseProps, },
304-314: Use query validators and validate roomId; add tags/license and 400 mapping.Also see handler comment for required event_id check.
.get( '/v1/state_ids/:roomId', { - params: isGetStateIdsParamsProps, + params: isGetRoomParamsProps, + query: isGetStateIdsQueryProps, response: { - 200: isGetStateIdsResponseProps, + 200: isGetStateIdsResponseProps, + 400: isErrorResponseProps, }, + tags: ['Federation'], + license: ['federation'], },Add supporting schemas near the other schemas:
+const GetRoomParamsSchema = { + type: 'object', + properties: { + roomId: { type: 'string' }, + }, + required: ['roomId'], + additionalProperties: false, +}; +const isGetRoomParamsProps = ajv.compile(GetRoomParamsSchema);
316-323: Guard missing event_id and return the declared shape; avoid non‑null assertion.- const roomId = c.req.param('roomId'); - const eventId = c.req.query('event_id'); - const stateIds = await profile.getStateIds(roomId, eventId!); - - return { - body: stateIds, - statusCode: 200, - }; + const roomId = c.req.param('roomId'); + const eventId = c.req.query('event_id'); + if (!eventId) { + return { + statusCode: 400, + body: { error: 'bad-request', details: { message: 'event_id is required' } }, + }; + } + const stateIds = await profile.getStateIds(roomId, eventId); + return { + body: { stateIds }, + statusCode: 200, + };
326-333: Mirror the same fixes for /state: query validators, tags/license, and 400 mapping..get( '/v1/state/:roomId', { - params: isGetStateParamsProps, + params: isGetRoomParamsProps, + query: isGetStateQueryProps, response: { - 200: isGetStateResponseProps, + 200: isGetStateResponseProps, + 400: isErrorResponseProps, }, + tags: ['Federation'], + license: ['federation'], },
335-341: Return body shape that matches schema and guard event_id.- const roomId = c.req.param('roomId'); - const eventId = c.req.query('event_id'); - const state = await profile.getState(roomId, eventId!); - return { - statusCode: 200, - body: state, - }; + const roomId = c.req.param('roomId'); + const eventId = c.req.query('event_id'); + if (!eventId) { + return { + statusCode: 400, + body: { error: 'bad-request', details: { message: 'event_id is required' } }, + }; + } + const state = await profile.getState(roomId, eventId); + return { + statusCode: 200, + body: { state }, + };
230-239: Same as above: event_id must be required; schema should be query.-const GetStateParamsSchema = { +const GetStateQuerySchema = { type: 'object', properties: { event_id: { type: 'string', }, }, + required: ['event_id'], + additionalProperties: false, }; -const isGetStateParamsProps = ajv.compile(GetStateParamsSchema); +const isGetStateQueryProps = ajv.compile(GetStateQuerySchema);
205-216: event_id is a query param; fix schema naming/usage and tighten validation.Define this as a query schema (not params) and block extra fields.
-const GetStateIdsParamsSchema = { +const GetStateIdsQuerySchema = { type: 'object', properties: { event_id: { type: 'string', }, }, required: ['event_id'], + additionalProperties: false, }; -const isGetStateIdsParamsProps = ajv.compile(GetStateIdsParamsSchema); +const isGetStateIdsQueryProps = ajv.compile(GetStateIdsQuerySchema);Additionally add a params schema for roomId (see route comment below).
🧹 Nitpick comments (4)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (4)
217-229: Response schema is loose; require stateIds and disallow extras.const GetStateIdsResponseSchema = { type: 'object', properties: { stateIds: { type: 'array', items: { type: 'string', }, }, }, + required: ['stateIds'], + additionalProperties: false, };
240-249: Require state in response and disallow extras.const GetStateResponseSchema = { type: 'object', properties: { state: { type: 'object', }, }, + required: ['state'], + additionalProperties: false, }; const isGetStateResponseProps = ajv.compile(GetStateResponseSchema);
304-343: Guard new GETs with ACL/signature middleware per PR title.Attach your ACL middleware to both routes so reads are protected.
- .get( + .get( '/v1/state_ids/:roomId', { // ... }, + aclMiddleware(services.auth), // adjust import/DI as per your middleware async (c) => { // ... }, ) - .get( + .get( '/v1/state/:roomId', { // ... }, + aclMiddleware(services.auth), async (c) => { // ... }, )I can wire this once you confirm the middleware export and DI signature.
346-354: Add 404 schema and ACL to the event-by-id route for consistency..get( '/v1/event/:eventId', { params: isGetEventParamsProps, response: { + 404: isErrorResponseProps, }, tags: ['Federation'], license: ['federation'], }, + // aclMiddleware(services.auth),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/api/_matrix/transactions.ts
[error] 318-318: Step 'federation-matrix:build' failed: TS2339: Property 'getStateIds' does not exist on type 'ProfilesService' (src/api/_matrix/transactions.ts:318).
🔇 Additional comments (2)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
205-216: Add roomId params schema used by both new routes.[recommend_recommended_refactor]
+const GetRoomParamsSchema = { + type: 'object', + properties: { + roomId: { type: 'string' }, + }, + required: ['roomId'], + additionalProperties: false, +}; +const isGetRoomParamsProps = ajv.compile(GetRoomParamsSchema);
254-254: Verify 'profile' service — ensure getStateIds/getState exist or wire the correct service.HomeserverServices is imported from '@hs/federation-sdk' (type definition not in this repo). ee/packages/federation-matrix/src/api/_matrix/transactions.ts destructures { event, config, profile } and calls profile.getStateIds(roomId, eventId) and profile.getState(roomId, eventId) (lines 318 and 337). Confirm whether 'profile' actually exposes these methods; if not, replace with the correct service (e.g., state/room service) or add getStateIds/getState to the profile service/type in @hs/federation-sdk. Location: ee/packages/federation-matrix/src/api/_matrix/transactions.ts lines 253–254, 316–338.
ggazzo
changed the title
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit