From ff801d9805f3a7e8b85eaa35eb97cb1ede0492a6 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 5 Feb 2024 17:44:08 +0100 Subject: [PATCH] Catch more gateway errors (#355) * Catch places where gateway calls may fail. * changelog * Fix a crash caused by users attempting to leave a room. * Fix test * Use a named type. --- changelog.d/355.bugfix | 1 + src/MatrixEventHandler.ts | 26 +++++++++++++++++++------- src/RoomSync.ts | 6 +++++- src/bifrost/Events.ts | 1 - src/xmppjs/XJSAccount.ts | 28 +++++++++++++++++++++------- test/xmppjs/test_XJSAccount.ts | 3 ++- 6 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 changelog.d/355.bugfix diff --git a/changelog.d/355.bugfix b/changelog.d/355.bugfix new file mode 100644 index 00000000..5db5cb9b --- /dev/null +++ b/changelog.d/355.bugfix @@ -0,0 +1 @@ +Fix several cases where Bifrost may crash if a gateway room was inaccessible. diff --git a/src/MatrixEventHandler.ts b/src/MatrixEventHandler.ts index d7c04edd..07af3906 100644 --- a/src/MatrixEventHandler.ts +++ b/src/MatrixEventHandler.ts @@ -626,7 +626,11 @@ Say \`help\` for more commands. const name: string = context.remote.get("room_name"); if (isGateway) { const msg = MessageFormatter.matrixEventToBody(event as MatrixMessageEvent, this.config.bridge); - this.gatewayHandler.sendMatrixMessage(name, event.sender, msg, context); + try { + await this.gatewayHandler.sendMatrixMessage(name, event.sender, msg, context); + } catch (ex) { + log.warn(`Failed to handle message for gateway:`, ex); + } return; } try { @@ -689,9 +693,13 @@ Say \`help\` for more commands. const name: string = context.remote.get("room_name"); const roomProtocol: string = context.remote.get("protocol_id"); if (isGateway) { - await this.gatewayHandler.sendMatrixMembership( - name, context, event, - ); + try { + await this.gatewayHandler.sendMatrixMembership( + name, context, event, + ); + } catch (ex) { + log.warn(`Failed to handle membership for gateway:`, ex); + } return; } @@ -728,9 +736,13 @@ Say \`help\` for more commands. const name = context.remote.get("room_name"); log.info(`Handling group state event for ${name}`); if (isGateway) { - await this.gatewayHandler.sendStateEvent( - name, event.sender, event, context, - ); + try { + await this.gatewayHandler.sendStateEvent( + name, event.sender, event, context, + ); + } catch (ex) { + log.warn(`Failed to handle state event for gateway:`, ex); + } return; } // XXX: Support state changes for non-gateways diff --git a/src/RoomSync.ts b/src/RoomSync.ts index 24647abf..552cb008 100644 --- a/src/RoomSync.ts +++ b/src/RoomSync.ts @@ -96,7 +96,11 @@ export class RoomSync { const isGateway = room.remote.get("gateway"); if (isGateway) { // The gateway handler syncs via roomState. - this.gateway.initialMembershipSync(room); + try { + await this.gateway.initialMembershipSync(room); + } catch (ex) { + log.warn(`Failed to sync gateway membership for room ${roomId}`); + } return; } let members: {[userId: string]: {display_name?: string}}; diff --git a/src/bifrost/Events.ts b/src/bifrost/Events.ts index 9e05d4a3..4078a33d 100644 --- a/src/bifrost/Events.ts +++ b/src/bifrost/Events.ts @@ -4,7 +4,6 @@ import { IBifrostAccount } from "./Account"; import { IBasicProtocolMessage } from "../MessageFormatter"; -import { IGatewayRoom } from "./Gateway"; import { IPublicRoomsResponse } from "../MatrixTypes"; export interface IChatJoinProperties {[key: string]: string; } diff --git a/src/xmppjs/XJSAccount.ts b/src/xmppjs/XJSAccount.ts index c105e0c2..159c5e2b 100644 --- a/src/xmppjs/XJSAccount.ts +++ b/src/xmppjs/XJSAccount.ts @@ -19,6 +19,8 @@ const LASTSTANZA_CHECK_MS = 2 * 60000; const LASTSTANZA_MAXDURATION = 10 * 60000; const log = new Logger("XmppJsAccount"); +type XmppChatJoinComponents = { handle: string }&({room: string, server: string}|{fullRoomName: string}) + export class XmppJsAccount implements IBifrostAccount { get name(): string { @@ -54,10 +56,17 @@ export class XmppJsAccount implements IBifrostAccount { this.lastStanzaTs.set(roomName, Date.now()); return; } + const handle = this.roomHandles.get(roomName); + if (!handle) { + // Shouldn't happen, but we must be careful. + return; + } this.joinChat({ fullRoomName: roomName, - handle: this.roomHandles.get(roomName)!, - }); + handle, + }).catch(ex => { + log.warn(`Attempted to join ${roomName} due to a failed self ping, but failed`, ex); + }) }); } }); @@ -186,18 +195,22 @@ export class XmppJsAccount implements IBifrostAccount { } public async joinChat( - components: IChatJoinProperties, + components: XmppChatJoinComponents, instance?: IBifrostInstance, timeout: number = 5000, setWaiting: boolean = true) : Promise { - if (!components.fullRoomName && (!components.room || !components.server)) { - throw Error("Missing fullRoomName OR room|server"); - } + let roomName: string; if (!components.handle) { throw Error("Missing handle"); } - const roomName = components.fullRoomName || `${components.room}@${components.server}`; + if ('fullRoomName' in components) { + roomName = components.fullRoomName; + } else if ('room' in components && 'server' in components) { + roomName = `${components.room}@${components.server}`; + } else { + throw Error("Missing fullRoomName OR room|server"); + } const to = `${roomName}/${components.handle}`; log.debug(`joinChat:`, this.remoteId, components); if (this.isInRoom(roomName)) { @@ -288,6 +301,7 @@ export class XmppJsAccount implements IBifrostAccount { `${components.room}@${components.server}/${components.handle}`, )); this.roomHandles.delete(room); + this.lastStanzaTs.delete(room); Metrics.remoteCall("xmpp.presence.left"); } diff --git a/test/xmppjs/test_XJSAccount.ts b/test/xmppjs/test_XJSAccount.ts index a74a3190..6e23b564 100644 --- a/test/xmppjs/test_XJSAccount.ts +++ b/test/xmppjs/test_XJSAccount.ts @@ -67,7 +67,8 @@ describe("XJSAccount", () => { await acct.joinChat({ room: "den", server: "remote.server", - }, instance as any, 50, true); + // Explicit any - we want to deliberately send wrong params + } as any, instance as any, 50, true); } catch (ex) { expect(ex.message).to.equal("Missing handle"); return;