From 463a7ffbc4f06fe2256165692fa3d29f87bdbdae Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 15:20:42 +0100 Subject: [PATCH 1/8] Add `enableUserSuffixCheck` option to allow disabling the checking of suffixes --- src/appservice/Appservice.ts | 39 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/appservice/Appservice.ts b/src/appservice/Appservice.ts index 48b074dd..45b50b0e 100644 --- a/src/appservice/Appservice.ts +++ b/src/appservice/Appservice.ts @@ -182,6 +182,14 @@ export interface IAppserviceOptions { */ maxAgeMs?: number; }; + + /** + * Check whether the users namespace of the appservice contains a regex + * which can be used by `*ForSuffix` functions. You can disable this check + * if you have multiple user namespaces, a namespace with a regex that does not + * end in .+ or .* and do not use user suffix functions + */ + enableUserSuffixCheck?: boolean; } /** @@ -198,7 +206,7 @@ export class Appservice extends EventEmitter { */ public readonly metrics: Metrics = new Metrics(); - private readonly userPrefix: string; + private readonly userPrefix?: string; private readonly aliasPrefix: string | null; private readonly registration: IAppserviceRegistration; private readonly storage: IAppserviceStorageProvider; @@ -217,6 +225,9 @@ export class Appservice extends EventEmitter { constructor(private options: IAppserviceOptions) { super(); + // On by default + options.enableUserSuffixCheck = options.enableUserSuffixCheck === undefined || options.enableUserSuffixCheck; + options.joinStrategy = new AppserviceJoinRoomStrategy(options.joinStrategy, this); if (!options.intentOptions) options.intentOptions = {}; @@ -268,11 +279,14 @@ export class Appservice extends EventEmitter { throw new Error("Too many user namespaces registered: expecting exactly one"); } - this.userPrefix = (this.registration.namespaces.users[0].regex || "").split(":")[0]; - if (!this.userPrefix.endsWith(".*") && !this.userPrefix.endsWith(".+")) { - throw new Error("Expected user namespace to be a prefix"); + // Only check if we've enabled + if (options.enableUserSuffixCheck) { + this.userPrefix = (this.registration.namespaces.users[0].regex || "").split(":")[0]; + if (!this.userPrefix.endsWith(".*") && !this.userPrefix.endsWith(".+")) { + throw new Error("Expected user namespace to be a prefix"); + } + this.userPrefix = this.userPrefix.substring(0, this.userPrefix.length - 2); // trim off the .* part } - this.userPrefix = this.userPrefix.substring(0, this.userPrefix.length - 2); // trim off the .* part if (!this.registration.namespaces || !this.registration.namespaces.aliases || this.registration.namespaces.aliases.length === 0 || this.registration.namespaces.aliases.length !== 1) { this.aliasPrefix = null; @@ -328,9 +342,9 @@ export class Appservice extends EventEmitter { /** * Starts the application service, opening the bind address to begin processing requests. - * @returns {Promise} resolves when started + * @returns {Promise} resolves when started */ - public begin(): Promise { + public begin(): Promise { return new Promise((resolve, reject) => { this.appServer = this.app.listen(this.options.port, this.options.bindAddress, () => resolve()); }).then(() => this.botIntent.ensureRegistered()); @@ -381,6 +395,9 @@ export class Appservice extends EventEmitter { * @returns {string} The user's ID. */ public getUserIdForSuffix(suffix: string): string { + if (!this.userPrefix) { + throw new Error(`Cannot use getUserIdForSuffix, enableUserSuffixCheck is off`); + } return `${this.userPrefix}${suffix}:${this.options.homeserverName}`; } @@ -405,6 +422,9 @@ export class Appservice extends EventEmitter { * @returns {string} The suffix from the user ID. */ public getSuffixForUserId(userId: string): string { + if (!this.userPrefix) { + throw new Error(`Cannot use getSuffixForUserId, enableUserSuffixCheck is off`); + } if (!userId || !userId.startsWith(this.userPrefix) || !userId.endsWith(`:${this.options.homeserverName}`)) { // Invalid ID return null; @@ -425,7 +445,10 @@ export class Appservice extends EventEmitter { * @returns {boolean} true if the user is namespaced, false otherwise */ public isNamespacedUser(userId: string): boolean { - return userId === this.botUserId || (userId.startsWith(this.userPrefix) && userId.endsWith(":" + this.options.homeserverName)); + return userId === this.botUserId || + !!this.registration.namespaces?.users.find(({regex}) => + new RegExp(regex).test(userId) + ); } /** From 80a83bdad0d8c4d83056f7becc0b27bc32311918 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 15:21:06 +0100 Subject: [PATCH 2/8] Add a test --- test/appservice/AppserviceTest.ts | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/appservice/AppserviceTest.ts b/test/appservice/AppserviceTest.ts index 8ee142fc..26f96629 100644 --- a/test/appservice/AppserviceTest.ts +++ b/test/appservice/AppserviceTest.ts @@ -165,6 +165,28 @@ describe('Appservice', () => { expect(appservice.getUserIdForSuffix('foo')).toEqual("@prefix_foo:localhost"); }); + it('should allow disabling the suffix check', async () => { + const appservice = new Appservice({ + port: 0, + bindAddress: '127.0.0.1', + homeserverName: 'localhost', + homeserverUrl: 'https://localhost', + registration: { + as_token: "", + hs_token: "", + sender_localpart: "", + namespaces: { + users: [{exclusive: true, regex: "@prefix_foo:localhost"}], + rooms: [], + aliases: [], + }, + }, + enableUserSuffixCheck: false, + }); + expect(() => appservice.getUserIdForSuffix('foo')).toThrowError("Cannot use getUserIdForSuffix, enableUserSuffixCheck is off"); + expect(appservice.isNamespacedUser('@prefix_foo:localhost')).toEqual(true); + }); + it('should return the right bot user ID', async () => { const appservice = new Appservice({ port: 0, From 3dbebea3b44bdb61ef44fb4dccec30260e1a77a8 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 15:21:23 +0100 Subject: [PATCH 3/8] Unrelated: Fixing a couple of places where TS complained of untyped Promises --- src/appservice/Intent.ts | 54 ++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/appservice/Intent.ts b/src/appservice/Intent.ts index b3d70298..9931419d 100644 --- a/src/appservice/Intent.ts +++ b/src/appservice/Intent.ts @@ -180,40 +180,40 @@ export class Intent { /** * Ensures the user is registered - * @returns {Promise} Resolves when complete + * @returns {Promise} Resolves when complete */ @timedIntentFunctionCall() - public async ensureRegistered() { - if (!(await Promise.resolve(this.storage.isUserRegistered(this.userId)))) { - try { - const result = await this.client.doRequest("POST", "/_matrix/client/r0/register", null, { - type: "m.login.application_service", - username: this.userId.substring(1).split(":")[0], - }); - - // HACK: Workaround for unit tests - if (result['errcode']) { - // noinspection ExceptionCaughtLocallyJS - throw {body: result}; - } - } catch (err) { - if (typeof (err.body) === "string") err.body = JSON.parse(err.body); - if (err.body && err.body["errcode"] === "M_USER_IN_USE") { - await Promise.resolve(this.storage.addRegisteredUser(this.userId)); - if (this.userId === this.appservice.botUserId) { - return null; - } else { - LogService.error("Appservice", "Error registering user: User ID is in use"); - return null; - } + public async ensureRegistered(): Promise { + if ((await Promise.resolve(this.storage.isUserRegistered(this.userId)))) { + return; + } + try { + const result = await this.client.doRequest("POST", "/_matrix/client/r0/register", null, { + type: "m.login.application_service", + username: this.userId.substring(1).split(":")[0], + }); + + // HACK: Workaround for unit tests + if (result['errcode']) { + // noinspection ExceptionCaughtLocallyJS + throw {body: result}; + } + } catch (err) { + if (typeof (err.body) === "string") err.body = JSON.parse(err.body); + if (err.body && err.body["errcode"] === "M_USER_IN_USE") { + await Promise.resolve(this.storage.addRegisteredUser(this.userId)); + if (this.userId === this.appservice.botUserId) { + return null; } else { LogService.error("Appservice", "Encountered error registering user: "); LogService.error("Appservice", extractRequestError(err)); } - throw err; + } else { + LogService.error("Appservice", "Encountered error registering user: "); + LogService.error("Appservice", err); } - - await Promise.resolve(this.storage.addRegisteredUser(this.userId)); + throw err; } + await Promise.resolve(this.storage.addRegisteredUser(this.userId)); } } From edcb9b200364f5d99243f96cd89a884d081c836d Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 15:26:40 +0100 Subject: [PATCH 4/8] Remove TODO comment --- src/appservice/Appservice.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/appservice/Appservice.ts b/src/appservice/Appservice.ts index 45b50b0e..5d1e3096 100644 --- a/src/appservice/Appservice.ts +++ b/src/appservice/Appservice.ts @@ -270,8 +270,6 @@ export class Appservice extends EventEmitter { // Everything else can 404 - // TODO: Should we permit other user namespaces and instead error when trying to use doSomethingBySuffix()? - if (!this.registration.namespaces || !this.registration.namespaces.users || this.registration.namespaces.users.length === 0) { throw new Error("No user namespaces in registration"); } From d58dba437c8557a2ffc9e9f3ccd22bf7f1c259ad Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 16:23:32 +0100 Subject: [PATCH 5/8] Remove parameter, instead throw on function call --- src/appservice/Appservice.ts | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/appservice/Appservice.ts b/src/appservice/Appservice.ts index 5d1e3096..f9d68734 100644 --- a/src/appservice/Appservice.ts +++ b/src/appservice/Appservice.ts @@ -182,14 +182,6 @@ export interface IAppserviceOptions { */ maxAgeMs?: number; }; - - /** - * Check whether the users namespace of the appservice contains a regex - * which can be used by `*ForSuffix` functions. You can disable this check - * if you have multiple user namespaces, a namespace with a regex that does not - * end in .+ or .* and do not use user suffix functions - */ - enableUserSuffixCheck?: boolean; } /** @@ -206,7 +198,7 @@ export class Appservice extends EventEmitter { */ public readonly metrics: Metrics = new Metrics(); - private readonly userPrefix?: string; + private readonly userPrefix: string | null; private readonly aliasPrefix: string | null; private readonly registration: IAppserviceRegistration; private readonly storage: IAppserviceStorageProvider; @@ -225,9 +217,6 @@ export class Appservice extends EventEmitter { constructor(private options: IAppserviceOptions) { super(); - // On by default - options.enableUserSuffixCheck = options.enableUserSuffixCheck === undefined || options.enableUserSuffixCheck; - options.joinStrategy = new AppserviceJoinRoomStrategy(options.joinStrategy, this); if (!options.intentOptions) options.intentOptions = {}; @@ -278,14 +267,14 @@ export class Appservice extends EventEmitter { } // Only check if we've enabled - if (options.enableUserSuffixCheck) { - this.userPrefix = (this.registration.namespaces.users[0].regex || "").split(":")[0]; - if (!this.userPrefix.endsWith(".*") && !this.userPrefix.endsWith(".+")) { - throw new Error("Expected user namespace to be a prefix"); - } - this.userPrefix = this.userPrefix.substring(0, this.userPrefix.length - 2); // trim off the .* part + let userPrefix = (this.registration.namespaces.users[0].regex || "").split(":")[0]; + if (!userPrefix.endsWith(".*") && !userPrefix.endsWith(".+")) { + this.userPrefix = null; + } else { + this.userPrefix = userPrefix.substring(0, userPrefix.length - 2); // trim off the .* part } + if (!this.registration.namespaces || !this.registration.namespaces.aliases || this.registration.namespaces.aliases.length === 0 || this.registration.namespaces.aliases.length !== 1) { this.aliasPrefix = null; } else { @@ -394,7 +383,7 @@ export class Appservice extends EventEmitter { */ public getUserIdForSuffix(suffix: string): string { if (!this.userPrefix) { - throw new Error(`Cannot use getUserIdForSuffix, enableUserSuffixCheck is off`); + throw new Error(`Cannot use getUserIdForSuffix, provided namespace did not include a valid suffix`); } return `${this.userPrefix}${suffix}:${this.options.homeserverName}`; } @@ -421,7 +410,7 @@ export class Appservice extends EventEmitter { */ public getSuffixForUserId(userId: string): string { if (!this.userPrefix) { - throw new Error(`Cannot use getSuffixForUserId, enableUserSuffixCheck is off`); + throw new Error(`Cannot use getUserIdForSuffix, provided namespace did not include a valid suffix`); } if (!userId || !userId.startsWith(this.userPrefix) || !userId.endsWith(`:${this.options.homeserverName}`)) { // Invalid ID From 294ab1616473ef5952560380e62f8b941c79ec0b Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 16:23:58 +0100 Subject: [PATCH 6/8] Update tests --- test/appservice/AppserviceTest.ts | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/test/appservice/AppserviceTest.ts b/test/appservice/AppserviceTest.ts index 26f96629..87e0a7ea 100644 --- a/test/appservice/AppserviceTest.ts +++ b/test/appservice/AppserviceTest.ts @@ -99,32 +99,6 @@ describe('Appservice', () => { } }); - it('should throw when there is no prefix namespace', async () => { - try { - new Appservice({ - port: 0, - bindAddress: '127.0.0.1', - homeserverName: 'localhost', - homeserverUrl: 'https://localhost', - registration: { - as_token: "", - hs_token: "", - sender_localpart: "", - namespaces: { - users: [{exclusive: true, regex: "@.*_suffix:.+"}], - rooms: [], - aliases: [], - }, - }, - }); - - // noinspection ExceptionCaughtLocallyJS - throw new Error("Did not throw when expecting it"); - } catch (e) { - expect(e.message).toEqual("Expected user namespace to be a prefix"); - } - }); - it('should accept a ".+" prefix namespace', async () => { const appservice = new Appservice({ port: 0, @@ -181,9 +155,9 @@ describe('Appservice', () => { aliases: [], }, }, - enableUserSuffixCheck: false, }); - expect(() => appservice.getUserIdForSuffix('foo')).toThrowError("Cannot use getUserIdForSuffix, enableUserSuffixCheck is off"); + expect(() => appservice.getUserIdForSuffix('foo')).toThrowError("Cannot use getUserIdForSuffix, provided namespace did not include a valid suffix"); + expect(() => appservice.getSuffixForUserId('foo')).toThrowError("Cannot use getUserIdForSuffix, provided namespace did not include a valid suffix"); expect(appservice.isNamespacedUser('@prefix_foo:localhost')).toEqual(true); }); From d438a9643dfe3711f1e5131cb34d7f2fbd392de1 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 19 Aug 2021 16:24:54 +0100 Subject: [PATCH 7/8] Revert changes to Intent.ts --- src/appservice/Intent.ts | 54 ++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/appservice/Intent.ts b/src/appservice/Intent.ts index 9931419d..b3d70298 100644 --- a/src/appservice/Intent.ts +++ b/src/appservice/Intent.ts @@ -180,40 +180,40 @@ export class Intent { /** * Ensures the user is registered - * @returns {Promise} Resolves when complete + * @returns {Promise} Resolves when complete */ @timedIntentFunctionCall() - public async ensureRegistered(): Promise { - if ((await Promise.resolve(this.storage.isUserRegistered(this.userId)))) { - return; - } - try { - const result = await this.client.doRequest("POST", "/_matrix/client/r0/register", null, { - type: "m.login.application_service", - username: this.userId.substring(1).split(":")[0], - }); - - // HACK: Workaround for unit tests - if (result['errcode']) { - // noinspection ExceptionCaughtLocallyJS - throw {body: result}; - } - } catch (err) { - if (typeof (err.body) === "string") err.body = JSON.parse(err.body); - if (err.body && err.body["errcode"] === "M_USER_IN_USE") { - await Promise.resolve(this.storage.addRegisteredUser(this.userId)); - if (this.userId === this.appservice.botUserId) { - return null; + public async ensureRegistered() { + if (!(await Promise.resolve(this.storage.isUserRegistered(this.userId)))) { + try { + const result = await this.client.doRequest("POST", "/_matrix/client/r0/register", null, { + type: "m.login.application_service", + username: this.userId.substring(1).split(":")[0], + }); + + // HACK: Workaround for unit tests + if (result['errcode']) { + // noinspection ExceptionCaughtLocallyJS + throw {body: result}; + } + } catch (err) { + if (typeof (err.body) === "string") err.body = JSON.parse(err.body); + if (err.body && err.body["errcode"] === "M_USER_IN_USE") { + await Promise.resolve(this.storage.addRegisteredUser(this.userId)); + if (this.userId === this.appservice.botUserId) { + return null; + } else { + LogService.error("Appservice", "Error registering user: User ID is in use"); + return null; + } } else { LogService.error("Appservice", "Encountered error registering user: "); LogService.error("Appservice", extractRequestError(err)); } - } else { - LogService.error("Appservice", "Encountered error registering user: "); - LogService.error("Appservice", err); + throw err; } - throw err; + + await Promise.resolve(this.storage.addRegisteredUser(this.userId)); } - await Promise.resolve(this.storage.addRegisteredUser(this.userId)); } } From 004d8047694144da389907df7e6f5d7a3a77e8d4 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Tue, 31 Aug 2021 09:21:34 +0100 Subject: [PATCH 8/8] Update Appservice.ts --- src/appservice/Appservice.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/appservice/Appservice.ts b/src/appservice/Appservice.ts index f9d68734..7091547c 100644 --- a/src/appservice/Appservice.ts +++ b/src/appservice/Appservice.ts @@ -266,7 +266,6 @@ export class Appservice extends EventEmitter { throw new Error("Too many user namespaces registered: expecting exactly one"); } - // Only check if we've enabled let userPrefix = (this.registration.namespaces.users[0].regex || "").split(":")[0]; if (!userPrefix.endsWith(".*") && !userPrefix.endsWith(".+")) { this.userPrefix = null;