From 4a23630e0695d0cc452e49f0aff22df7ede1e796 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 16 Sep 2022 09:03:17 +0100 Subject: [PATCH] Fix soft crash around inviting invalid MXIDs in start DM on first message flow (#9281) * Fix soft crash around inviting invalid MXIDs * Make ts --strict happier * Prevent suggesting invalid MXIDs * Add tests * Fix coverage * Fix coverage * Make tsc --strict happier * Fix test * Add tests --- cypress/e2e/crypto/crypto.spec.ts | 2 +- src/components/views/dialogs/InviteDialog.tsx | 81 ++++++++----- src/components/views/dialogs/ShareDialog.tsx | 8 -- src/utils/permalinks/Permalinks.ts | 38 +++--- .../views/dialogs/InviteDialog-test.tsx | 113 ++++++++++++++++++ test/utils/permalinks/Permalinks-test.ts | 9 ++ 6 files changed, 194 insertions(+), 57 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 d465f0c1e1..99bde7ad1d 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 d23c7f6a18..178c521bdc 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"; @@ -69,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"; @@ -243,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; @@ -283,14 +299,13 @@ interface IInviteDialogState { errorText: string; } -export default class InviteDialog extends React.PureComponent { +export default class InviteDialog extends React.PureComponent { static defaultProps = { kind: KIND_DM, initialText: "", }; - private closeCopiedTooltip: () => 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; @@ -316,7 +331,7 @@ export default class InviteDialog extends React.PureComponent { @@ -466,6 +478,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(); @@ -499,6 +512,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(); @@ -565,7 +579,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 +609,17 @@ 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 ( @@ -1107,14 +1125,15 @@ 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", { - spaceName: room.name || _t("Unnamed Space"), + spaceName: room?.name || _t("Unnamed Space"), }) : _t("Invite to %(roomName)s", { - roomName: room.name || _t("Unnamed Room"), + roomName: room?.name || _t("Unnamed Room"), }); let helpTextUntranslated; @@ -1140,14 +1159,14 @@ export default class InviteDialog extends React.PureComponent { userId }, a: (sub) => - { sub }, + { sub }, }); buttonText = _t("Invite"); goButtonFn = this.inviteUsers; if (cli.isRoomEncrypted(this.props.roomId)) { - const room = cli.getRoom(this.props.roomId); + const room = cli.getRoom(this.props.roomId)!; const visibilityEvent = room.currentState.getStateEvents( "m.room.history_visibility", "", ); @@ -1185,8 +1204,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 : { - 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; diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 404bf98769..c7c7de9c53 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]) => { @@ -189,9 +189,11 @@ export class RoomPermalinkCreator { return false; } const serverName = getServerName(userId); - return !isHostnameIpAddress(serverName) && - !isHostInRegex(serverName, this.bannedHostsRegexps) && - isHostInRegex(serverName, this.allowedHostsRegexps); + + const domain = getHostnameFromMatrixServerName(serverName) ?? serverName; + 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 +252,15 @@ 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]; + const serverName = serversByPopulation[i]; + const domain = getHostnameFromMatrixServerName(serverName) ?? ""; if ( - !candidates.has(server) && - !isHostnameIpAddress(server) && - !isHostInRegex(server, this.bannedHostsRegexps) && - isHostInRegex(server, this.allowedHostsRegexps) + !candidates.has(serverName) && + !isHostnameIpAddress(domain) && + !isHostInRegex(domain, this.bannedHostsRegexps) && + isHostInRegex(domain, this.allowedHostsRegexps) ) { - candidates.add(server); + candidates.add(serverName); } } @@ -441,17 +444,21 @@ export function parsePermalink(fullUrl: string): PermalinkParts { return null; // not a permalink we can handle } -function getServerName(userId: string): string { +export 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; +export function getHostnameFromMatrixServerName(serverName: string): string | null { + if (!serverName) return null; + 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 { - hostname = getHostnameFromMatrixDomain(hostname); if (!hostname) return true; // assumed if (regexps.length > 0 && !regexps[0].test) throw new Error(regexps[0].toString()); @@ -459,7 +466,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 diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx new file mode 100644 index 0000000000..8d10bb2357 --- /dev/null +++ b/test/components/views/dialogs/InviteDialog-test.tsx @@ -0,0 +1,113 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import React from "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"; + 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().mockRejectedValue({ errcode: "" }), + getIdentityServerUrl: jest.fn(), + searchUserDirectory: jest.fn().mockResolvedValue({}), + }); + + beforeEach(() => { + SdkConfig.put({ validated_server_config: {} as ValidatedServerConfig } as IConfigOptions); + 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 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(( + + )); + + expect(screen.queryByText("@localpart:server:tld")).toBeFalsy(); + }); +}); diff --git a/test/utils/permalinks/Permalinks-test.ts b/test/utils/permalinks/Permalinks-test.ts index 0c1104d285..85a80e966a 100644 --- a/test/utils/permalinks/Permalinks-test.ts +++ b/test/utils/permalinks/Permalinks-test.ts @@ -94,6 +94,15 @@ describe('Permalinks', function() { expect(creator.serverCandidates.length).toBe(0); }); + it('should gracefully handle invalid MXIDs', () => { + 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);