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

@@ -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();

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<IDMRoomTileProps> {
     }
 }
 
-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
+    // Initial value to populate the filter with
-    // not provided.
+    initialText?: string;
-    kind: AnyInviteKind;
+}
+
+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<IInviteDialogProps, IInviteDialogState> {
+export default class InviteDialog extends React.PureComponent<Props, IInviteDialogState> {
     static defaultProps = {
         kind: KIND_DM,
         initialText: "",
     };
 
-    private closeCopiedTooltip: () => void;
+    private debounceTimer: number | null = null; // actually number because we're in the browser
-    private debounceTimer: number = null; // actually number because we're in the browser
     private editorRef = createRef<HTMLInputElement>();
     private numberEntryFieldRef: React.RefObject<Field> = createRef();
     private unmounted = false;
@@ -316,7 +331,7 @@ export default class InviteDialog extends React.PureComponent<IInviteDialogProps
 
         this.state = {
             targets: [], // array of Member objects (see interface above)
-            filterText: this.props.initialText,
+            filterText: this.props.initialText || "",
             recents: InviteDialog.buildRecents(alreadyInvited),
             numRecentsShown: INITIAL_ROOMS_SHOWN,
             suggestions: this.buildSuggestions(alreadyInvited),
@@ -343,9 +358,6 @@ export default class InviteDialog extends React.PureComponent<IInviteDialogProps
 
     componentWillUnmount() {
         this.unmounted = true;
-        // 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 onConsultFirstChange = (ev) => {
@@ -466,6 +478,7 @@ export default class InviteDialog extends React.PureComponent<IInviteDialogProps
     };
 
     private inviteUsers = async () => {
+        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<IInviteDialogProps
     };
 
     private transferCall = async () => {
+        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<IInviteDialogProps
         this.props.onFinished(false);
     };
 
-    private updateSuggestions = async (term) => {
+    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,15 +609,19 @@ export default class InviteDialog extends React.PureComponent<IInviteDialogProps
                     logger.warn("Non-fatal error trying to make an invite for a user ID");
                     logger.warn(e);
 
+                    // Reuse logic from Permalinks as a basic MXID validity check
+                    const serverName = getServerName(term);
+                    const domain = getHostnameFromMatrixServerName(serverName);
+                    if (domain) {
                         // Add a result anyways, just without a profile. We stick it at the
                         // top so it is most obviously presented to the user.
                         r.results.splice(0, 0, {
                             user_id: term,
                             display_name: term,
-                        avatar_url: null,
                         });
                     }
                 }
+            }
 
             this.setState({
                 serverResultsMixin: r.results.map(u => ({
@@ -940,7 +958,7 @@ export default class InviteDialog extends React.PureComponent<IInviteDialogProps
                 disabled={this.state.busy || (this.props.kind == KIND_CALL_TRANSFER && this.state.targets.length > 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<IInviteDialogProps
                 </CopyableText>
             </div>;
         } 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<IInviteDialogProps
                 userId: () =>
                     <a href={makeUserPermalink(userId)} rel="noreferrer noopener" target="_blank">{ userId }</a>,
                 a: (sub) =>
-                    <a href={makeRoomPermalink(this.props.roomId)} rel="noreferrer noopener" target="_blank">{ sub }</a>,
+                    <a href={makeRoomPermalink(roomId)} rel="noreferrer noopener" target="_blank">{ sub }</a>,
             });
 
             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<IInviteDialogProps
                     { _t("Transfer") }
                 </AccessibleButton>
             </div>;
-        } else {
-            logger.error("Unknown kind of InviteDialog: " + this.props.kind);
         }
 
         const goButton = this.props.kind == KIND_CALL_TRANSFER ? null : <AccessibleButton

src/components/views/dialogs/ShareDialog.tsx

@@ -71,8 +71,6 @@ interface IState {
 }
 
 export default class ShareDialog extends React.PureComponent<IProps, IState> {
-    protected closeCopiedTooltip: () => void;
-
     constructor(props) {
         super(props);
 
@@ -100,12 +98,6 @@ export default class ShareDialog extends React.PureComponent<IProps, IState> {
         });
     };
 
-    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;
 

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<string, number> = 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) &&
+                        const domain = getHostnameFromMatrixServerName(serverName) ?? serverName;
-                            isHostInRegex(serverName, this.allowedHostsRegexps);
+                        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) &&
+                !candidates.has(serverName) &&
-                !isHostnameIpAddress(server) &&
+                !isHostnameIpAddress(domain) &&
-                !isHostInRegex(server, this.bannedHostsRegexps) &&
+                !isHostInRegex(domain, this.bannedHostsRegexps) &&
-                isHostInRegex(server, this.allowedHostsRegexps)
+                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 {
+export function getHostnameFromMatrixServerName(serverName: string): string | null {
-    if (!domain) return null;
+    if (!serverName) return null;
-    return new URL(`https://${domain}`).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 {
-    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

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((
+            <InviteDialog
+                kind={KIND_INVITE}
+                roomId={roomId}
+                onFinished={jest.fn()}
+            />
+        ));
+
+        expect(screen.queryByText("Invite to Space")).toBeTruthy();
+    });
+
+    it("should label with room name", () => {
+        render((
+            <InviteDialog
+                kind={KIND_INVITE}
+                roomId={roomId}
+                onFinished={jest.fn()}
+            />
+        ));
+
+        expect(screen.queryByText("Invite to Room")).toBeTruthy();
+    });
+
+    it("should suggest valid MXIDs even if unknown", () => {
+        render((
+            <InviteDialog
+                kind={KIND_INVITE}
+                roomId={roomId}
+                onFinished={jest.fn()}
+                initialText="@localpart:server.tld"
+            />
+        ));
+
+        expect(screen.queryByText("@localpart:server.tld")).toBeFalsy();
+    });
+
+    it("should not suggest invalid MXIDs", () => {
+        render((
+            <InviteDialog
+                kind={KIND_INVITE}
+                roomId={roomId}
+                onFinished={jest.fn()}
+                initialText="@localpart:server:tld"
+            />
+        ));
+
+        expect(screen.queryByText("@localpart:server:tld")).toBeFalsy();
+    });
+});

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);