Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Fix soft crash around inviting invalid MXIDs in start DM on first mes…
Browse files Browse the repository at this point in the history 
…sage 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
  • Loading branch information
@t3chguy
t3chguy authored Sep 16, 2022
1 parent 4fec436 commit 4a23630
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 57 deletions.
2 changes: 1 addition & 1 deletion cypress/e2e/crypto/crypto.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
81 changes: 49 additions & 32 deletions src/components/views/dialogs/InviteDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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";

Expand Down Expand Up @@ -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
// 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;
Expand All @@ -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; // actually number because we're in the browser
private debounceTimer: number | null = null; // actually number because we're in the browser
private editorRef = createRef<HTMLInputElement>();
private numberEntryFieldRef: React.RefObject<Field> = createRef();
private unmounted = false;
Expand All @@ -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),
Expand All @@ -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) => {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -595,13 +609,17 @@ 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);

// 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,
});
// 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,
});
}
}
}

Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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;
Expand All @@ -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", "",
);
Expand Down Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions src/components/views/dialogs/ShareDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ interface IState {
}

export default class ShareDialog extends React.PureComponent<IProps, IState> {
protected closeCopiedTooltip: () => void;

constructor(props) {
super(props);

Expand Down Expand Up @@ -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;

Expand Down
38 changes: 22 additions & 16 deletions src/utils/permalinks/Permalinks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]) => {
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -441,25 +444,28 @@ 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());

return regexps.some(h => h.test(hostname));
}

function isHostnameIpAddress(hostname: string): boolean {
hostname = getHostnameFromMatrixDomain(hostname);
if (!hostname) return false;

// is-ip doesn't want IPv6 addresses surrounded by brackets, so
Expand Down
Loading

0 comments on commit 4a23630

Please sign in to comment.