From 997e97f58f7a4ff95c24b7cfe23043714a79e0a1 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 14 Sep 2022 15:46:03 +0100 Subject: [PATCH 1/9] Fix soft crash around inviting invalid MXIDs --- src/utils/permalinks/Permalinks.ts | 42 ++++++++++++++++++------------ 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 404bf987699..cd301f8d4f7 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -189,9 +189,16 @@ export class RoomPermalinkCreator { return false; } const serverName = getServerName(userId); - return !isHostnameIpAddress(serverName) && - !isHostInRegex(serverName, this.bannedHostsRegexps) && - isHostInRegex(serverName, this.allowedHostsRegexps); + + let domain = serverName; + try { + domain = getHostnameFromMatrixServerName(serverName); + } catch (e) { + console.error("Error encountered while updating highest power level user", e); + } + return !isHostnameIpAddress(domain) && + !isHostInRegex(domain, this.bannedHostsRegexps) && + isHostInRegex(domain, this.allowedHostsRegexps); }); const maxEntry = allowedEntries.reduce((max, entry) => { return (entry[1] > max[1]) ? entry : max; @@ -250,14 +257,19 @@ export class RoomPermalinkCreator { .sort((a, b) => this.populationMap[b] - this.populationMap[a]); for (let i = 0; i < serversByPopulation.length && candidates.size < MAX_SERVER_CANDIDATES; i++) { - const server = serversByPopulation[i]; - if ( - !candidates.has(server) && - !isHostnameIpAddress(server) && - !isHostInRegex(server, this.bannedHostsRegexps) && - isHostInRegex(server, this.allowedHostsRegexps) - ) { - candidates.add(server); + const serverName = serversByPopulation[i]; + try { + const domain = getHostnameFromMatrixServerName(serverName); + if ( + !candidates.has(serverName) && + !isHostnameIpAddress(domain) && + !isHostInRegex(domain, this.bannedHostsRegexps) && + isHostInRegex(domain, this.allowedHostsRegexps) + ) { + candidates.add(serverName); + } + } catch (e) { + console.error("Encountered error while updating server candidates", e); } } @@ -445,13 +457,12 @@ function getServerName(userId: string): string { return userId.split(":").splice(1).join(":"); } -function getHostnameFromMatrixDomain(domain: string): string { - if (!domain) return null; - return new URL(`https://${domain}`).hostname; +function getHostnameFromMatrixServerName(serverName: string): string { + if (!serverName) return null; + return new URL(`https://${serverName}`).hostname; } function isHostInRegex(hostname: string, regexps: RegExp[]): boolean { - hostname = getHostnameFromMatrixDomain(hostname); if (!hostname) return true; // assumed if (regexps.length > 0 && !regexps[0].test) throw new Error(regexps[0].toString()); @@ -459,7 +470,6 @@ function isHostInRegex(hostname: string, regexps: RegExp[]): boolean { } function isHostnameIpAddress(hostname: string): boolean { - hostname = getHostnameFromMatrixDomain(hostname); if (!hostname) return false; // is-ip doesn't want IPv6 addresses surrounded by brackets, so From 83c2ce98aa32b6092de37caf12f1158be2f36c67 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 14 Sep 2022 15:59:57 +0100 Subject: [PATCH 2/9] Make ts --strict happier --- src/utils/permalinks/Permalinks.ts | 38 +++++++++++++----------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index cd301f8d4f7..0414cc586d6 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -180,7 +180,7 @@ export class RoomPermalinkCreator { if (plEvent) { const content = plEvent.getContent(); if (content) { - const users = content.users; + const users: Record = content.users; if (users) { const entries = Object.entries(users); const allowedEntries = entries.filter(([userId]) => { @@ -190,12 +190,7 @@ export class RoomPermalinkCreator { } const serverName = getServerName(userId); - let domain = serverName; - try { - domain = getHostnameFromMatrixServerName(serverName); - } catch (e) { - console.error("Error encountered while updating highest power level user", e); - } + const domain = getHostnameFromMatrixServerName(serverName) ?? serverName; return !isHostnameIpAddress(domain) && !isHostInRegex(domain, this.bannedHostsRegexps) && isHostInRegex(domain, this.allowedHostsRegexps); @@ -258,18 +253,14 @@ export class RoomPermalinkCreator { for (let i = 0; i < serversByPopulation.length && candidates.size < MAX_SERVER_CANDIDATES; i++) { const serverName = serversByPopulation[i]; - try { - const domain = getHostnameFromMatrixServerName(serverName); - if ( - !candidates.has(serverName) && - !isHostnameIpAddress(domain) && - !isHostInRegex(domain, this.bannedHostsRegexps) && - isHostInRegex(domain, this.allowedHostsRegexps) - ) { - candidates.add(serverName); - } - } catch (e) { - console.error("Encountered error while updating server candidates", e); + const domain = getHostnameFromMatrixServerName(serverName) ?? ""; + if ( + !candidates.has(serverName) && + !isHostnameIpAddress(domain) && + !isHostInRegex(domain, this.bannedHostsRegexps) && + isHostInRegex(domain, this.allowedHostsRegexps) + ) { + candidates.add(serverName); } } @@ -457,9 +448,14 @@ function getServerName(userId: string): string { return userId.split(":").splice(1).join(":"); } -function getHostnameFromMatrixServerName(serverName: string): string { +function getHostnameFromMatrixServerName(serverName: string): string | null { if (!serverName) return null; - return new URL(`https://${serverName}`).hostname; + try { + return new URL(`https://${serverName}`).hostname; + } catch (e) { + console.error("Error encountered while extracting hostname from server name", e); + return null; + } } function isHostInRegex(hostname: string, regexps: RegExp[]): boolean { From 45f8d66f33d9a66cea6e3d6e72fa4b36b8553e1a Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 14 Sep 2022 16:27:22 +0100 Subject: [PATCH 3/9] Prevent suggesting invalid MXIDs --- src/components/views/dialogs/InviteDialog.tsx | 28 +++++++++++++------ src/utils/permalinks/Permalinks.ts | 4 +-- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index d23c7f6a184..cedebdba974 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -25,7 +25,12 @@ import { Icon as InfoIcon } from "../../../../res/img/element-icons/info.svg"; import { Icon as EmailPillAvatarIcon } from "../../../../res/img/icon-email-pill-avatar.svg"; import { _t, _td } from "../../../languageHandler"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; -import { makeRoomPermalink, makeUserPermalink } from "../../../utils/permalinks/Permalinks"; +import { + getHostnameFromMatrixServerName, + getServerName, + makeRoomPermalink, + makeUserPermalink, +} from "../../../utils/permalinks/Permalinks"; import DMRoomMap from "../../../utils/DMRoomMap"; import SdkConfig from "../../../SdkConfig"; import * as Email from "../../../email"; @@ -565,7 +570,7 @@ export default class InviteDialog extends React.PureComponent { + private updateSuggestions = async (term: string) => { MatrixClientPeg.get().searchUserDirectory({ term }).then(async r => { if (term !== this.state.filterText) { // Discard the results - we were probably too slow on the server-side to make @@ -595,13 +600,18 @@ export default class InviteDialog extends React.PureComponent Date: Thu, 15 Sep 2022 10:38:08 +0100 Subject: [PATCH 4/9] Add tests --- cypress/e2e/crypto/crypto.spec.ts | 2 +- src/components/views/dialogs/InviteDialog.tsx | 41 +++++++---- .../views/dialogs/InviteDialog-test.tsx | 73 +++++++++++++++++++ 3 files changed, 100 insertions(+), 16 deletions(-) create mode 100644 test/components/views/dialogs/InviteDialog-test.tsx diff --git a/cypress/e2e/crypto/crypto.spec.ts b/cypress/e2e/crypto/crypto.spec.ts index d465f0c1e1b..99bde7ad1da 100644 --- a/cypress/e2e/crypto/crypto.spec.ts +++ b/cypress/e2e/crypto/crypto.spec.ts @@ -50,7 +50,7 @@ const checkDMRoom = () => { const startDMWithBob = function(this: CryptoTestContext) { cy.get('.mx_RoomList [aria-label="Start chat"]').click(); - cy.get('[data-test-id="invite-dialog-input"]').type(this.bob.getUserId()); + cy.get('[data-testid="invite-dialog-input"]').type(this.bob.getUserId()); cy.contains(".mx_InviteDialog_tile_nameStack_name", "Bob").click(); cy.contains(".mx_InviteDialog_userTile_pill .mx_InviteDialog_userTile_name", "Bob").should("exist"); cy.get(".mx_InviteDialog_goButton").click(); diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index cedebdba974..c13c61e23a2 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -74,7 +74,7 @@ import { startDmOnFirstMessage, ThreepidMember, } from "../../../utils/direct-messages"; -import { AnyInviteKind, KIND_CALL_TRANSFER, KIND_DM, KIND_INVITE } from './InviteDialogTypes'; +import { KIND_CALL_TRANSFER, KIND_DM, KIND_INVITE } from './InviteDialogTypes'; import Modal from '../../../Modal'; import dis from "../../../dispatcher/dispatcher"; @@ -248,26 +248,37 @@ class DMRoomTile extends React.PureComponent { } } -interface IInviteDialogProps { +interface BaseProps { // Takes a boolean which is true if a user / users were invited / // a call transfer was initiated or false if the dialog was cancelled // with no action taken. onFinished: (success: boolean) => void; - // The kind of invite being performed. Assumed to be KIND_DM if - // not provided. - kind: AnyInviteKind; + // Initial value to populate the filter with + initialText?: string; +} + +interface InviteDMProps extends BaseProps { + // The kind of invite being performed. Assumed to be KIND_DM if not provided. + kind?: typeof KIND_DM; +} + +interface InviteRoomProps extends BaseProps { + kind: typeof KIND_INVITE; // The room ID this dialog is for. Only required for KIND_INVITE. roomId: string; +} + +interface InviteCallProps extends BaseProps { + kind: typeof KIND_CALL_TRANSFER; // The call to transfer. Only required for KIND_CALL_TRANSFER. call: MatrixCall; - - // Initial value to populate the filter with - initialText: string; } +type Props = InviteDMProps | InviteRoomProps | InviteCallProps; + interface IInviteDialogState { targets: Member[]; // array of Member objects (see interface above) filterText: string; @@ -288,7 +299,7 @@ interface IInviteDialogState { errorText: string; } -export default class InviteDialog extends React.PureComponent { +export default class InviteDialog extends React.PureComponent { static defaultProps = { kind: KIND_DM, initialText: "", @@ -471,6 +482,7 @@ export default class InviteDialog extends React.PureComponent { + if (this.props.kind !== KIND_INVITE) return; this.setState({ busy: true }); this.convertFilter(); const targets = this.convertFilter(); @@ -504,6 +516,7 @@ export default class InviteDialog extends React.PureComponent { + if (this.props.kind !== KIND_CALL_TRANSFER) return; if (this.state.currentTabId == TabId.UserDirectory) { this.convertFilter(); const targets = this.convertFilter(); @@ -609,7 +622,6 @@ export default class InviteDialog extends React.PureComponent 0)} autoComplete="off" placeholder={hasPlaceholder ? _t("Search") : null} - data-test-id="invite-dialog-input" + data-testid="invite-dialog-input" /> ); return ( @@ -1117,7 +1129,8 @@ export default class InviteDialog extends React.PureComponent ; } else if (this.props.kind === KIND_INVITE) { - const room = MatrixClientPeg.get()?.getRoom(this.props.roomId); + const roomId = this.props.roomId; + const room = MatrixClientPeg.get()?.getRoom(roomId); const isSpace = room?.isSpaceRoom(); title = isSpace ? _t("Invite to %(spaceName)s", { @@ -1150,7 +1163,7 @@ export default class InviteDialog extends React.PureComponent { userId }, a: (sub) => - { sub }, + { sub }, }); buttonText = _t("Invite"); @@ -1195,8 +1208,6 @@ export default class InviteDialog extends React.PureComponent ; - } else { - logger.error("Unknown kind of InviteDialog: " + this.props.kind); } const goButton = this.props.kind == KIND_CALL_TRANSFER ? null : { + const roomId = "!111111111111111111:example.org"; + const aliceId = "@alice:example.org"; + const mockClient = getMockClientWithEventEmitter({ + getUserId: jest.fn().mockReturnValue(aliceId), + isGuest: jest.fn().mockReturnValue(false), + getVisibleRooms: jest.fn().mockReturnValue([]), + getRoom: jest.fn(), + getRooms: jest.fn(), + getAccountData: jest.fn(), + getPushActionsForEvent: jest.fn(), + mxcUrlToHttp: jest.fn().mockReturnValue(''), + isRoomEncrypted: jest.fn().mockReturnValue(false), + getProfileInfo: jest.fn().mockResolvedValue({ + displayname: 'Alice', + }), + getIdentityServerUrl: jest.fn(), + }); + + beforeEach(() => { + DMRoomMap.makeShared(); + jest.clearAllMocks(); + mockClient.getUserId.mockReturnValue("@bob:example.org"); + + const room = mkStubRoom(roomId, "Room", mockClient); + mockClient.getRooms.mockReturnValue([room]); + mockClient.getRoom.mockReturnValue(room); + }); + + afterAll(() => { + jest.spyOn(MatrixClientPeg, 'get').mockRestore(); + }); + + it("should not suggest invalid MXIDs", async () => { + render(( + + )); + + const input = screen.getByTestId("invite-dialog-input"); + fireEvent.change(input, { target: { value: "@localpart:server:tld" } }); + + expect(screen.queryByText("@localpart:server:tld")).toBeFalsy(); + }); +}); From 1e35449112865d50ac219773ae0f09b54b8a8ef5 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Sep 2022 11:09:35 +0100 Subject: [PATCH 5/9] Fix coverage --- test/components/views/dialogs/InviteDialog-test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx index f988a577ea0..626e638ec46 100644 --- a/test/components/views/dialogs/InviteDialog-test.tsx +++ b/test/components/views/dialogs/InviteDialog-test.tsx @@ -40,6 +40,7 @@ describe("InviteDialog", () => { displayname: 'Alice', }), getIdentityServerUrl: jest.fn(), + searchUserDirectory: jest.fn().mockResolvedValue({}), }); beforeEach(() => { From ef0d48c50be074730ba218a69c66f766922afee7 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Sep 2022 14:21:23 +0100 Subject: [PATCH 6/9] Fix coverage --- test/components/views/dialogs/InviteDialog-test.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx index 626e638ec46..c74c814f4fa 100644 --- a/test/components/views/dialogs/InviteDialog-test.tsx +++ b/test/components/views/dialogs/InviteDialog-test.tsx @@ -36,9 +36,7 @@ describe("InviteDialog", () => { getPushActionsForEvent: jest.fn(), mxcUrlToHttp: jest.fn().mockReturnValue(''), isRoomEncrypted: jest.fn().mockReturnValue(false), - getProfileInfo: jest.fn().mockResolvedValue({ - displayname: 'Alice', - }), + getProfileInfo: jest.fn().mockRejectedValue({ errcode: "" }), getIdentityServerUrl: jest.fn(), searchUserDirectory: jest.fn().mockResolvedValue({}), }); From 02103fb4b8a6606b44abb4d0e7c118b45e47900b Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Sep 2022 14:30:20 +0100 Subject: [PATCH 7/9] Make tsc --strict happier --- src/components/views/dialogs/InviteDialog.tsx | 14 +++++--------- src/components/views/dialogs/ShareDialog.tsx | 8 -------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index c13c61e23a2..178c521bdcb 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -305,8 +305,7 @@ export default class InviteDialog extends React.PureComponent void; - private debounceTimer: number = null; // actually number because we're in the browser + private debounceTimer: number | null = null; // actually number because we're in the browser private editorRef = createRef(); private numberEntryFieldRef: React.RefObject = createRef(); private unmounted = false; @@ -332,7 +331,7 @@ export default class InviteDialog extends React.PureComponent { @@ -1134,10 +1130,10 @@ export default class InviteDialog extends React.PureComponent { - protected closeCopiedTooltip: () => void; - constructor(props) { super(props); @@ -100,12 +98,6 @@ export default class ShareDialog extends React.PureComponent { }); }; - componentWillUnmount() { - // if the Copied tooltip is open then get rid of it, there are ways to close the modal which wouldn't close - // the tooltip otherwise, such as pressing Escape or clicking X really quickly - if (this.closeCopiedTooltip) this.closeCopiedTooltip(); - } - private getUrl() { let matrixToUrl; From ceac35372b2248c951c2cb377ad5e1c47dd60dc2 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Sep 2022 15:30:08 +0100 Subject: [PATCH 8/9] Fix test --- test/components/views/dialogs/InviteDialog-test.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx index c74c814f4fa..7bb199a8ea5 100644 --- a/test/components/views/dialogs/InviteDialog-test.tsx +++ b/test/components/views/dialogs/InviteDialog-test.tsx @@ -15,13 +15,16 @@ limitations under the License. */ import React from "react"; -import { fireEvent, render, screen } from "@testing-library/react"; +import { render, screen } from "@testing-library/react"; import InviteDialog from "../../../../src/components/views/dialogs/InviteDialog"; import { KIND_INVITE } from "../../../../src/components/views/dialogs/InviteDialogTypes"; import { getMockClientWithEventEmitter, mkStubRoom } from "../../../test-utils"; import { MatrixClientPeg } from "../../../../src/MatrixClientPeg"; import DMRoomMap from "../../../../src/utils/DMRoomMap"; +import SdkConfig from "../../../../src/SdkConfig"; +import { ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig"; +import { IConfigOptions } from "../../../../src/IConfigOptions"; describe("InviteDialog", () => { const roomId = "!111111111111111111:example.org"; @@ -42,6 +45,7 @@ describe("InviteDialog", () => { }); beforeEach(() => { + SdkConfig.put({ validated_server_config: {} as ValidatedServerConfig } as IConfigOptions); DMRoomMap.makeShared(); jest.clearAllMocks(); mockClient.getUserId.mockReturnValue("@bob:example.org"); @@ -61,12 +65,10 @@ describe("InviteDialog", () => { kind={KIND_INVITE} roomId={roomId} onFinished={jest.fn()} + initialText="@localpart:server:tld" /> )); - const input = screen.getByTestId("invite-dialog-input"); - fireEvent.change(input, { target: { value: "@localpart:server:tld" } }); - expect(screen.queryByText("@localpart:server:tld")).toBeFalsy(); }); }); From 26c428c908bdcbabe3738d7da74aa5c4b3013d96 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Sep 2022 15:56:35 +0100 Subject: [PATCH 9/9] Add tests --- .../views/dialogs/InviteDialog-test.tsx | 41 ++++++++++++++++++- test/utils/permalinks/Permalinks-test.ts | 9 ++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx index 7bb199a8ea5..8d10bb2357d 100644 --- a/test/components/views/dialogs/InviteDialog-test.tsx +++ b/test/components/views/dialogs/InviteDialog-test.tsx @@ -59,7 +59,46 @@ describe("InviteDialog", () => { jest.spyOn(MatrixClientPeg, 'get').mockRestore(); }); - it("should not suggest invalid MXIDs", async () => { + it("should label with space name", () => { + mockClient.getRoom(roomId).isSpaceRoom = jest.fn().mockReturnValue(true); + mockClient.getRoom(roomId).name = "Space"; + render(( + + )); + + expect(screen.queryByText("Invite to Space")).toBeTruthy(); + }); + + it("should label with room name", () => { + render(( + + )); + + expect(screen.queryByText("Invite to Room")).toBeTruthy(); + }); + + it("should suggest valid MXIDs even if unknown", () => { + render(( + + )); + + expect(screen.queryByText("@localpart:server.tld")).toBeFalsy(); + }); + + it("should not suggest invalid MXIDs", () => { render(( { + const roomId = "!fake:example.org"; + const alice50 = makeMemberWithPL(roomId, "@alice:pl_50:org", 50); + const room = mockRoom(roomId, [alice50]); + const creator = new RoomPermalinkCreator(room); + creator.load(); + expect(creator.serverCandidates).toBeTruthy(); + }); + it('should pick a candidate server for the highest power level user in the room', function() { const roomId = "!fake:example.org"; const alice50 = makeMemberWithPL(roomId, "@alice:pl_50", 50);