Skip to content

feat: add improved errors messaging #305

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
sampaiodiego merged 5 commits into from
Dec 19, 2025
Merged

feat: add improved errors messaging #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f407d6
feat: add improved errors messaging
ricardogarim Nov 24, 2025
5cf78a4
chore: remove httpCode from FederationValidationError
ricardogarim Nov 25, 2025
7e52690
chore: remove try catch block from checkRoomAcl
ricardogarim Nov 25, 2025
0c34985
disable validation during tests
sampaiodiego Dec 19, 2025
326cb5e
simplify config
sampaiodiego Dec 19, 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
5 changes: 5 additions & 0 deletions packages/federation-sdk/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ export {
} from './utils/event-schemas';
export { errCodes } from './utils/response-codes';
export { NotAllowedError } from './services/invite.service';
export {
FederationValidationService,
FederationValidationError,
} from './services/federation-validation.service';

export type HomeserverEventSignatures = {
'homeserver.ping': {
Expand Down Expand Up @@ -136,6 +140,7 @@ export {
roomIdSchema,
userIdSchema,
eventIdSchema,
extractDomainFromId,
} from '@rocket.chat/federation-room';

export async function init({
Expand Down
10 changes: 10 additions & 0 deletions packages/federation-sdk/src/sdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { EventAuthorizationService } from './services/event-authorization.servic
import { EventEmitterService } from './services/event-emitter.service';
import { EventService } from './services/event.service';
import { FederationRequestService } from './services/federation-request.service';
import { FederationValidationService } from './services/federation-validation.service';
import { FederationService } from './services/federation.service';
import { InviteService } from './services/invite.service';
import { MediaService } from './services/media.service';
Expand Down Expand Up @@ -39,6 +40,7 @@ export class FederationSDK {
private readonly federationRequestService: FederationRequestService,
private readonly federationService: FederationService,
public readonly eventEmitterService: EventEmitterService,
private readonly federationValidationService: FederationValidationService,
) {}

createDirectMessageRoom(
Expand Down Expand Up @@ -224,6 +226,14 @@ export class FederationSDK {
return this.wellKnownService.getWellKnownHostData(...args);
}

validateOutboundUser(
...args: Parameters<
typeof this.federationValidationService.validateOutboundUser
>
) {
return this.federationValidationService.validateOutboundUser(...args);
}

updateUserPowerLevel(
...args: Parameters<typeof this.roomService.updateUserPowerLevel>
) {
Expand Down
14 changes: 14 additions & 0 deletions packages/federation-sdk/src/services/config.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ export interface AppConfig {
processTyping: boolean;
processPresence: boolean;
};
userCheckTimeoutMs?: number;
networkCheckTimeoutMs?: number;
}

export const AppConfigSchema = z.object({
Expand Down Expand Up @@ -76,6 +78,18 @@ export const AppConfigSchema = z.object({
processTyping: z.boolean(),
processPresence: z.boolean(),
}),
networkCheckTimeoutMs: z
.number()
.int()
.min(1000, 'Network check timeout must be at least 1000ms')
.default(5000)
.optional(),
userCheckTimeoutMs: z
.number()
.int()
.min(1000, 'User check timeout must be at least 1000ms')
.default(10000)
.optional(),
});

@singleton()
Expand Down
2 changes: 1 addition & 1 deletion packages/federation-sdk/src/services/event-authorization.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export class EventAuthorizationService {
}

// as per Matrix spec: https://spec.matrix.org/v1.15/client-server-api/#mroomserver_acl
private async checkServerAcl(
async checkServerAcl(
aclEvent: PersistentEventBase | undefined,
serverName: string,
): Promise<boolean> {
Expand Down
116 changes: 116 additions & 0 deletions packages/federation-sdk/src/services/federation-validation.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import type { RoomID, UserID } from '@rocket.chat/federation-room';
import { extractDomainFromId } from '@rocket.chat/federation-room';
import { singleton } from 'tsyringe';
import { FederationEndpoints } from '../specs/federation-api';
import { ConfigService } from './config.service';
import { EventAuthorizationService } from './event-authorization.service';
import { FederationRequestService } from './federation-request.service';
import { StateService } from './state.service';

export class FederationValidationError extends Error {
public error: string;

constructor(
public code: 'POLICY_DENIED' | 'CONNECTION_FAILED' | 'USER_NOT_FOUND',
public userMessage: string,
) {
super(userMessage);
this.name = 'FederationValidationError';
this.error = `federation-${code.toLowerCase().replace(/_/g, '-')}`;
}
}

@singleton()
export class FederationValidationService {
constructor(
private readonly configService: ConfigService,
private readonly federationRequestService: FederationRequestService,
private readonly stateService: StateService,
private readonly eventAuthorizationService: EventAuthorizationService,
) {}

async validateOutboundUser(userId: UserID): Promise<void> {
const domain = extractDomainFromId(userId);
await this.checkDomainReachable(domain);
await this.checkUserExists(userId, domain);
}

async validateOutboundInvite(userId: UserID, roomId: RoomID): Promise<void> {
const domain = extractDomainFromId(userId);
await this.checkRoomAcl(roomId, domain);
await this.checkDomainReachable(domain);
await this.checkUserExists(userId, domain);
}

private async checkRoomAcl(roomId: RoomID, domain: string): Promise<void> {
const state = await this.stateService.getLatestRoomState(roomId);
const aclEvent = state.get('m.room.server_acl:');
if (!aclEvent || !aclEvent.isServerAclEvent()) {
return;
}

const isAllowed = await this.eventAuthorizationService.checkServerAcl(
aclEvent,
domain,
);
if (!isAllowed) {
throw new FederationValidationError(
'POLICY_DENIED',
"Action Blocked. The room's access control policy blocks communication with this domain.",
);
}
}

private async checkDomainReachable(domain: string): Promise<void> {
const timeoutMs =
this.configService.getConfig('networkCheckTimeoutMs') || 5000;

try {
const versionPromise = this.federationRequestService.get<{
server: { name?: string; version?: string };
}>(domain, FederationEndpoints.version);

await this.withTimeout(versionPromise, timeoutMs);
} catch (_error) {
throw new FederationValidationError(
'CONNECTION_FAILED',
'Connection Failed. The server domain could not be reached or does not support federation.',
);
}
}

private async checkUserExists(userId: UserID, domain: string): Promise<void> {
const timeoutMs =
this.configService.getConfig('userCheckTimeoutMs') || 10000;

try {
const uri = FederationEndpoints.queryProfile(userId);
const queryParams = { user_id: userId };

const profilePromise = this.federationRequestService.get<{
displayname?: string;
avatar_url?: string;
}>(domain, uri, queryParams);

await this.withTimeout(profilePromise, timeoutMs);
} catch (_error) {
throw new FederationValidationError(
'USER_NOT_FOUND',
"Invitation blocked. The specified user couldn't be found on their homeserver.",
);
}
}

private async withTimeout<T>(
promise: Promise<T>,
timeoutMs: number,
): Promise<T> {
const timeoutPromise = new Promise<never>((_, reject) => {
setTimeout(() => {
reject(new Error(`Operation timed out after ${timeoutMs}ms`));
}, timeoutMs);
});

return Promise.race([promise, timeoutPromise]);
}
}
10 changes: 8 additions & 2 deletions packages/federation-sdk/src/services/invite.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { EventRepository } from '../repositories/event.repository';
import { ConfigService } from './config.service';
import { EventAuthorizationService } from './event-authorization.service';
import { EventEmitterService } from './event-emitter.service';
import { FederationValidationService } from './federation-validation.service';
import { FederationService } from './federation.service';
import { StateService } from './state.service';
export class NotAllowedError extends Error {
Expand All @@ -35,6 +36,8 @@ export class InviteService {
private readonly emitterService: EventEmitterService,
@inject(delay(() => EventRepository))
private readonly eventRepository: EventRepository,
@inject(delay(() => FederationValidationService)) // need to delay to be able to inject during tests
private readonly federationValidationService: FederationValidationService,
) {}

/**
Expand Down Expand Up @@ -93,6 +96,11 @@ export class InviteService {
);
}

await this.federationValidationService.validateOutboundInvite(
userId,
roomId,
);

// if user invited belongs to our server
if (invitedServer === this.configService.serverName) {
await stateService.handlePdu(inviteEvent);
Expand All @@ -111,9 +119,7 @@ export class InviteService {
};
}

// invited user from another room
// get signed invite event

const inviteResponse = await federationService.inviteUser(
inviteEvent,
roomVersion,
Expand Down
15 changes: 13 additions & 2 deletions packages/federation-sdk/src/services/room.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
StateMapKey,
} from '@rocket.chat/federation-room';
import { container } from 'tsyringe';
import { federationSDK, init } from '..';
import { FederationValidationService, federationSDK, init } from '..';
import { AppConfig, ConfigService } from './config.service';
import { RoomService } from './room.service';
import { StateService } from './state.service';
Expand All @@ -29,7 +29,6 @@ describe('RoomService', async () => {
};

init({
emitter: undefined,
dbConfig: databaseConfig,
});
});
Expand All @@ -44,6 +43,18 @@ describe('RoomService', async () => {
useValue: configService,
});

// dont validate anything during tests
container.register(FederationValidationService, {
useValue: {
async validateOutboundUser() {
return true;
},
async validateOutboundInvite() {
return true;
},
} as unknown as FederationValidationService,
});

const stateService = container.resolve(StateService);
const roomService = container.resolve(RoomService);

Expand Down
7 changes: 7 additions & 0 deletions packages/federation-sdk/src/services/room.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import { EventAuthorizationService } from './event-authorization.service';
import { EventEmitterService } from './event-emitter.service';
import { EventFetcherService } from './event-fetcher.service';
import { EventService } from './event.service';
import { FederationValidationService } from './federation-validation.service';
import { FederationService } from './federation.service';
import { InviteService } from './invite.service';
import {
Expand All @@ -63,6 +64,7 @@ export class RoomService {
@inject(delay(() => EventStagingRepository))
private readonly eventStagingRepository: EventStagingRepository,
private readonly emitterService: EventEmitterService,
private readonly federationValidationService: FederationValidationService,
) {}

private validatePowerLevelChange(
Expand Down Expand Up @@ -1640,6 +1642,11 @@ export class RoomService {
await stateService.handlePdu(guestAccessEvent);

if (isExternalUser) {
await this.federationValidationService.validateOutboundInvite(
targetUserId,
roomCreateEvent.roomId,
);

Comment on lines 1642 to +1649
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Consider the room cleanup implications.

The validation occurs after the DM room has been fully created and persisted (lines 1406-1522). If validateOutboundInvite fails, the room remains in the database with only the creator as a member.

This design trade-off exists because the ACL check requires room state. However, it could lead to orphaned single-member rooms accumulating over time. Consider:

  • Whether orphaned rooms are acceptable for your use case
  • Adding cleanup logic to remove the room if validation fails
  • Documenting this behavior

🏁 Script executed:

#!/bin/bash
# Check if there's any cleanup logic for failed room creation elsewhere
rg -nP --type=ts -C3 'createDirectMessageRoom.*catch|try.*createDirectMessageRoom'

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Find the createDirectMessageRoom method implementation
ast-grep --pattern $'$$$createDirectMessageRoom($$$) {
  $$$
}'

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Search for validateOutboundInvite to understand what it does
rg -nP --type=ts -A10 'validateOutboundInvite\s*\(' -g '!node_modules'

Length of output: 3203

🏁 Script executed:

#!/bin/bash
# Check the broader createDirectMessageRoom implementation for transaction or error handling
rg -nP --type=ts -B5 -A30 'createDirectMessageRoom.*\{' packages/federation-sdk/src/services/room.service.ts | head -80

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Find who calls createDirectMessageRoom
rg -nP --type=ts 'createDirectMessageRoom\s*\(' -B3 -A3 -g '!node_modules' | grep -v test

Length of output: 2735

🏁 Script executed:

#!/bin/bash
# Check what errors checkDomainReachable and checkUserExists can throw
rg -nP --type=ts -A15 'private async checkDomainReachable\s*\(|private async checkUserExists\s*\(' packages/federation-sdk/src/services/federation-validation.service.ts

Length of output: 1304

🏁 Script executed:

#!/bin/bash
# Check if the entire createDirectMessageRoom is wrapped in a transaction or has error handling
rg -nP --type=ts -B10 'isExternalUser' packages/federation-sdk/src/services/room.service.ts | grep -A10 -B10 'isExternalUser.*{'

Length of output: 290

🏁 Script executed:

#!/bin/bash
# Get the full try-catch block from direct-message.controller.ts
rg -nP --type=ts -B5 -A20 'createDirectMessageRoom' packages/homeserver/src/controllers/internal/direct-message.controller.ts

Length of output: 721

🏁 Script executed:

#!/bin/bash
# Check what type of error response is returned
rg -nP --type=ts -A30 'catch.*error' packages/homeserver/src/controllers/internal/direct-message.controller.ts

Length of output: 701

Add cleanup logic or validate external user before room creation.

The concern is confirmed: if validateOutboundInvite fails (e.g., domain unreachable, user not found), the DM room persists in the database with only the creator as a member. The controller's error handler (direct-message.controller.ts:38-44) returns an error response but does not remove the orphaned room.

While the validation must occur after room creation (because the ACL check requires room state), consider:

  • Moving domain/user validation checks before room creation, keeping only the ACL check after
  • Adding transaction rollback or explicit cleanup if validation fails
  • Documenting this behavior if orphaned rooms are acceptable

await this.inviteService.inviteUserToRoom(
targetUserId,
roomCreateEvent.roomId,
Expand Down