Skip to content

refactor: state resolution error propagation #229

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

Closed
debdutdeb wants to merge 4 commits into from
Closed

refactor: state resolution error propagation #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8af5018
refactor: reduce forced assertions
debdutdeb Sep 26, 2025
1bce83e
refactor: clean up unused imports
debdutdeb Sep 26, 2025
4e5c897
refactor: StateResolverAuthorizationError correctly used everywhere
debdutdeb Sep 26, 2025
42c1974
ci: review coderabbitai
debdutdeb Sep 26, 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
7 changes: 3 additions & 4 deletions packages/federation-sdk/src/services/invite.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { EventBase, createLogger } from '@rocket.chat/federation-core';
import {
PduForType,
PersistentEventBase,
PersistentEventFactory,
RoomVersion,
} from '@rocket.chat/federation-room';
Expand Down Expand Up @@ -86,7 +85,7 @@ export class InviteService {
await stateService.persistStateEvent(inviteEvent);

if (inviteEvent.rejected) {
throw new Error(inviteEvent.rejectedReason);
throw new Error(inviteEvent.rejectReason);
}

// let all servers know of this state change
Expand Down Expand Up @@ -157,7 +156,7 @@ export class InviteService {

await this.stateService.persistStateEvent(inviteEvent);
if (inviteEvent.rejected) {
throw new Error(inviteEvent.rejectedReason);
throw new Error(inviteEvent.rejectReason);
}

// we do not send transaction here
Expand All @@ -174,7 +173,7 @@ export class InviteService {
// if we have the state we try to persist the invite event
await this.stateService.persistStateEvent(inviteEvent);
if (inviteEvent.rejected) {
throw new Error(inviteEvent.rejectedReason);
throw new Error(inviteEvent.rejectReason);
}
} catch (e) {
// don't have state copy yet
Expand Down
25 changes: 5 additions & 20 deletions packages/federation-sdk/src/services/message.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,8 @@
import {
type MessageAuthEvents,
type RoomMessageEvent,
roomMessageEvent,
} from '@rocket.chat/federation-core';
import { type SignedEvent } from '@rocket.chat/federation-core';

import { ForbiddenError } from '@rocket.chat/federation-core';
import {
type RedactionAuthEvents,
type RedactionEvent,
redactionEvent,
} from '@rocket.chat/federation-core';
import { createLogger } from '@rocket.chat/federation-core';
import { signEvent } from '@rocket.chat/federation-core';
import {
type EventID,
type PersistentEventBase,
PersistentEventFactory,
type RoomVersion,
} from '@rocket.chat/federation-room';
import { singleton } from 'tsyringe';
import { EventRepository } from '../repositories/event.repository';
Expand Down Expand Up @@ -97,7 +82,7 @@ export class MessageService {

await this.stateService.persistTimelineEvent(event);
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);
Expand Down Expand Up @@ -145,7 +130,7 @@ export class MessageService {

await this.stateService.persistTimelineEvent(event);
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);
Expand Down Expand Up @@ -181,7 +166,7 @@ export class MessageService {

await this.stateService.persistTimelineEvent(event);
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);
Expand Down Expand Up @@ -237,7 +222,7 @@ export class MessageService {

await this.stateService.persistTimelineEvent(event);
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);
Expand Down Expand Up @@ -288,7 +273,7 @@ export class MessageService {

await this.stateService.persistTimelineEvent(event);
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);
Expand Down
14 changes: 2 additions & 12 deletions packages/federation-sdk/src/services/room.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,10 @@
import {
EventBase,
EventStore,
RoomNameAuthEvents,
RoomPowerLevelsEvent,
RoomTombstoneEvent,
SignedEvent,
TombstoneAuthEvents,
generateId,
isRoomPowerLevelsEvent,
roomNameEvent,
roomPowerLevelsEvent,
roomTombstoneEvent,
signEvent,
} from '@rocket.chat/federation-core';
import { singleton } from 'tsyringe';
import { FederationService } from './federation.service';
Expand All @@ -21,18 +14,15 @@ import {
HttpException,
HttpStatus,
} from '@rocket.chat/federation-core';
import { type SigningKey } from '@rocket.chat/federation-core';

import { logger } from '@rocket.chat/federation-core';
import {
type EventID,
PduCreateEventContent,
PduForType,
PduJoinRuleEventContent,
PduType,
PersistentEventBase,
PersistentEventFactory,
RoomVersion,
} from '@rocket.chat/federation-room';
import { EventRepository } from '../repositories/event.repository';
import { RoomRepository } from '../repositories/room.repository';
Expand Down Expand Up @@ -810,7 +800,7 @@ export class RoomService {
await stateService.persistStateEvent(membershipEvent);

if (membershipEvent.rejected) {
throw new Error(membershipEvent.rejectedReason);
throw new Error(membershipEvent.rejectReason);
}

void federationService.sendEventToAllServersInRoom(membershipEvent);
Expand Down Expand Up @@ -993,7 +983,7 @@ export class RoomService {
);

if (joinEventFinal.rejected) {
throw new Error(joinEventFinal.rejectedReason);
throw new Error(joinEventFinal.rejectReason);
}
Comment on lines +986 to 987
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Return a structured HTTP error

Same issue on the remote join path: a plain Error becomes a 500 and leaks an "undefined" message when the join was legitimately rejected. Emit an HttpException with a safe fallback instead.

-		if (joinEventFinal.rejected) {
-			throw new Error(joinEventFinal.rejectReason);
+		if (joinEventFinal.rejected) {
+			throw new HttpException(
+				joinEventFinal.rejectReason || 'Join rejected',
+				HttpStatus.FORBIDDEN,
+			);
📝 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
throw new Error(joinEventFinal.rejectReason);
}
if (joinEventFinal.rejected) {
throw new HttpException(
joinEventFinal.rejectReason || 'Join rejected',
HttpStatus.FORBIDDEN,
);
}
🤖 Prompt for AI Agents 
In packages/federation-sdk/src/services/room.service.ts around lines 986-987,
replace throwing a plain Error with throwing a structured HttpException that
uses the joinEventFinal.rejectReason if present or a safe fallback message like
"Join request rejected", and set an appropriate HTTP status (e.g.,
HttpStatus.FORBIDDEN); also add the necessary imports (HttpException and
HttpStatus from @nestjs/common) and apply the same change to the remote join
path to avoid leaking "undefined" and to return a proper HTTP error.


return joinEventFinal.eventId;
Expand Down
2 changes: 1 addition & 1 deletion packages/federation-sdk/src/services/send-join.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export class SendJoinService {
await stateService.persistStateEvent(joinEvent);

if (joinEvent.rejected) {
throw new Error(joinEvent.rejectedReason);
throw new Error(joinEvent.rejectReason);
}

const configService = this.configService;
Expand Down
18 changes: 11 additions & 7 deletions packages/federation-sdk/src/services/state.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { RoomVersion } from '@rocket.chat/federation-room';
import { resolveStateV2Plus } from '@rocket.chat/federation-room';
import type { PduCreateEventContent } from '@rocket.chat/federation-room';
import { checkEventAuthWithState } from '@rocket.chat/federation-room';
import { RejectCodes } from '@rocket.chat/federation-room';
import { singleton } from 'tsyringe';
import { EventRepository } from '../repositories/event.repository';
import { StateRepository, StateStore } from '../repositories/state.repository';
Expand Down Expand Up @@ -53,12 +54,12 @@ export class StateService {
if (pdu.isState()) {
await this.persistStateEvent(pdu);
if (pdu.rejected) {
throw new Error(pdu.rejectedReason);
throw new Error(pdu.rejectReason);
}
} else {
await this.persistTimelineEvent(pdu);
if (pdu.rejected) {
throw new Error(pdu.rejectedReason);
throw new Error(pdu.rejectReason);
}
}
}
Expand Down Expand Up @@ -506,7 +507,7 @@ export class StateService {
if (!hasConflict) {
await checkEventAuthWithState(event, state, this._getStore(roomVersion));
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}

// save the state mapping
Expand Down Expand Up @@ -750,7 +751,7 @@ export class StateService {

// we need the auth events required to validate this event from our state
const requiredAuthEventsWeHaveSeenMap = new Map<
string,
EventID,
PersistentEventBase
>();
for (const auth of event.getAuthEventStateKeys()) {
Expand All @@ -777,24 +778,27 @@ export class StateService {
if (requiredAuthEventsWeHaveSeenMap.size !== authEventsReferencedMap.size) {
// incorrect length may mean either redacted event still referenced or event in state that wasn't referenced, both cases, reject the event
event.reject(
RejectCodes.AuthError,
`Auth events referenced in message do not match, expected ${requiredAuthEventsWeHaveSeenMap.size} but got ${authEventsReferencedMap.size}`,
);
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}
Comment on lines +781 to 785
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Preserve RejectCodes when failing auth prechecks

After calling event.reject(RejectCodes.AuthError, …), throwing new Error(event.rejectReason) discards the code and rejectedBy. Please throw/return the StateResolverAuthorizationError (or include its fields) so higher layers can differentiate auth errors from other failures.


for (const [eventId] of requiredAuthEventsWeHaveSeenMap) {
if (!authEventsReferencedMap.has(eventId)) {
event.reject(
RejectCodes.AuthError,
`wrong auth event in message, expected ${eventId} but not found in event`,
eventId,
);
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}
Comment on lines 787 to 795
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Propagate full rejection context

Same as above: we reject the event with RejectCodes.AuthError, but then throw away the structured metadata by emitting a plain Error. Please surface the structured error object instead.

🤖 Prompt for AI Agents 
In packages/federation-sdk/src/services/state.service.ts around lines 787 to
795, the code calls event.reject(RejectCodes.AuthError, ...) but then throws a
plain Error using event.rejectReason, losing the structured rejection metadata;
change this to capture and rethrow the structured rejection object returned by
event.reject (or construct and throw an object containing the RejectCode,
rejectReason and relevant metadata) instead of throwing new
Error(event.rejectReason) so callers receive the full rejection context.

}

// now we validate against auth rules
await checkEventAuthWithState(event, room, store);
if (event.rejected) {
throw new Error(event.rejectedReason);
throw new Error(event.rejectReason);
}
Comment on lines 798 to 802
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unhandled StateResolverAuthorizationError

checkEventAuthWithState can now throw StateResolverAuthorizationError. Catch it here, ensure event.reject is called, and propagate the structured error so callers can handle authorization failures deterministically.

🤖 Prompt for AI Agents 
In packages/federation-sdk/src/services/state.service.ts around lines 798 to
802, the call to checkEventAuthWithState can throw
StateResolverAuthorizationError but the code only checks event.rejected
afterwards; wrap the await checkEventAuthWithState(event, room, store) in a
try/catch, catch StateResolverAuthorizationError specifically, call
event.reject(...) (providing the error message/reason and setting
event.rejectReason as needed), then rethrow the caught
StateResolverAuthorizationError so callers receive the structured error; for any
other exceptions rethrow them unchanged.


// TODO: save event still but with mark
Expand Down
2 changes: 1 addition & 1 deletion packages/homeserver/src/controllers/internal/invite.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const internalInvitePlugin = (app: Elysia) => {
await stateService.persistStateEvent(membershipEvent);

if (membershipEvent.rejected) {
throw new Error(membershipEvent.rejectedReason);
throw new Error(membershipEvent.rejectReason);
}

return {
Expand Down
40 changes: 32 additions & 8 deletions packages/room/src/authorizartion-rules/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,47 @@
import type { PersistentEventBase } from '../manager/event-wrapper';
import { type EventID } from '../types/_common';

export const RejectCodes = {
AuthError: 'auth_error',
ValidationError: 'validation_error',
NotImplemented: 'not_implemented',
} as const;

export type RejectCode = (typeof RejectCodes)[keyof typeof RejectCodes];

class StateResolverAuthorizationError extends Error {
name = 'StateResolverAuthorizationError';

reason: string;

rejectedBy?: EventID;

constructor(
message: string,
public code: RejectCode,
{
eventFailed,
rejectedEvent,
reason,
rejectedBy,
}: {
eventFailed: PersistentEventBase;
reason?: PersistentEventBase;
rejectedEvent: PersistentEventBase;
reason: string;
rejectedBy?: PersistentEventBase;
},
) {
let error = `${message} for event ${eventFailed.eventId} in room ${eventFailed.roomId} type ${eventFailed.type} state_key ${eventFailed.stateKey}`;
if (reason) {
error += `, reason: ${reason.eventId} in room ${reason.roomId} type ${reason.type} state_key ${reason.stateKey}`;
// build the message
let message = `${code}: ${rejectedEvent.toStrippedJson()} failed authorization check`;

if (rejectedBy) {
message += ` against auth event ${rejectedBy.toStrippedJson()}`;
}
super(error);

message += `: ${reason}`;

super(message);

this.reason = reason;

this.rejectedBy = rejectedBy?.eventId;
}
}

Expand Down
Loading