From 5c0920da425a13e5b7d72461baab383266d61a0a Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 May 2020 11:05:30 +0100 Subject: [PATCH 1/4] Avoid soft crash if unknown device in verification Rageshakes from the wild indicate that device was null here which implies that we somehow did not know about the device when verifiying it? Log and null-check to avoid a soft crash. --- .../views/right_panel/EncryptionPanel.js | 5 +-- .../views/right_panel/VerificationPanel.js | 32 ++++++++++++++----- .../views/verification/VerificationShowSas.js | 7 ++-- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/components/views/right_panel/EncryptionPanel.js b/src/components/views/right_panel/EncryptionPanel.js index bc580c767b..e9f94729fa 100644 --- a/src/components/views/right_panel/EncryptionPanel.js +++ b/src/components/views/right_panel/EncryptionPanel.js @@ -45,9 +45,6 @@ const EncryptionPanel = (props) => { } }, [verificationRequest]); - const deviceId = request && request.channel.deviceId; - const device = MatrixClientPeg.get().getStoredDevice(MatrixClientPeg.get().getUserId(), deviceId); - useEffect(() => { async function awaitPromise() { setRequesting(true); @@ -143,7 +140,7 @@ const EncryptionPanel = (props) => { key={request.channel.transactionId} inDialog={layout === "dialog"} phase={phase} - device={device} /> + /> ); } }; diff --git a/src/components/views/right_panel/VerificationPanel.js b/src/components/views/right_panel/VerificationPanel.js index 67efd29d27..ff3700e143 100644 --- a/src/components/views/right_panel/VerificationPanel.js +++ b/src/components/views/right_panel/VerificationPanel.js @@ -17,6 +17,7 @@ limitations under the License. import React from "react"; import PropTypes from "prop-types"; +import {MatrixClientPeg} from "../../../MatrixClientPeg"; import * as sdk from '../../../index'; import {verificationMethods} from 'matrix-js-sdk/src/crypto'; import {SCAN_QR_CODE_METHOD} from "matrix-js-sdk/src/crypto/verification/QRCode"; @@ -161,6 +162,11 @@ export default class VerificationPanel extends React.PureComponent { this.state.reciprocateQREvent.cancel(); }; + _getDevice() { + const deviceId = this.props.request && this.props.request.channel.deviceId; + return MatrixClientPeg.get().getStoredDevice(MatrixClientPeg.get().getUserId(), deviceId); + } + renderQRReciprocatePhase() { const {member, request} = this.props; let Button; @@ -217,16 +223,26 @@ export default class VerificationPanel extends React.PureComponent { } } - const AccessibleButton = sdk.getComponent('elements.AccessibleButton'); - const description = request.isSelfVerification ? - _t("You've successfully verified %(deviceName)s (%(deviceId)s)!", { - deviceName: this.props.device.getDisplayName(), - deviceId: this.props.device.deviceId, - }): - _t("You've successfully verified %(displayName)s!", { + let description; + if (request.isSelfVerification) { + const device = this._getDevice(); + if (!device) { + // This shouldn't happen, although rageshake would indicate that it is: log a warn + // and leave the message slightly broken (avoid adding a translatable string for a + // case that shouldn't be happening). + console.warn("Verified device we don't know about: " + this.props.request.channel.deviceId); + } + description = _t("You've successfully verified %(deviceName)s (%(deviceId)s)!", { + deviceName: device ? device.getDisplayName() : '', + deviceId: this.props.request.channel.deviceId, + }); + } else { + description = _t("You've successfully verified %(displayName)s!", { displayName: member.displayName || member.name || member.userId, }); + } + const AccessibleButton = sdk.getComponent('elements.AccessibleButton'); return (

{_t("Verified")}

@@ -297,12 +313,12 @@ export default class VerificationPanel extends React.PureComponent { const emojis = this.state.sasEvent ? : ; return

{_t("Compare emoji")}

diff --git a/src/components/views/verification/VerificationShowSas.js b/src/components/views/verification/VerificationShowSas.js index edf860c4c2..251c8ca04f 100644 --- a/src/components/views/verification/VerificationShowSas.js +++ b/src/components/views/verification/VerificationShowSas.js @@ -30,6 +30,7 @@ export default class VerificationShowSas extends React.Component { static propTypes = { pending: PropTypes.bool, displayName: PropTypes.string, // required if pending is true + device: PropTypes.object, onDone: PropTypes.func.isRequired, onCancel: PropTypes.func.isRequired, sas: PropTypes.object.isRequired, @@ -116,9 +117,11 @@ export default class VerificationShowSas extends React.Component { let text; if (this.state.pending) { if (this.props.isSelf) { + // device shouldn't be null in this situation but it seems like it can be, so show + // slightly broken text rather than soft-crashing (we've already logged in VerificationPanel). text = _t("Waiting for your other session, %(deviceName)s (%(deviceId)s), to verify…", { - deviceName: this.props.device.getDisplayName(), - deviceId: this.props.device.deviceId, + deviceName: this.props.device ? this.props.device.getDisplayName() : '', + deviceId: this.props.device ? this.props.device.deviceId : '', }); } else { const {displayName} = this.props; From 3c5c7f56f3f4fffa08a17ee53402abc49250b1d6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 May 2020 11:14:05 +0100 Subject: [PATCH 2/4] Adjust comment --- src/components/views/right_panel/VerificationPanel.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/views/right_panel/VerificationPanel.js b/src/components/views/right_panel/VerificationPanel.js index ff3700e143..76507d8d4f 100644 --- a/src/components/views/right_panel/VerificationPanel.js +++ b/src/components/views/right_panel/VerificationPanel.js @@ -227,9 +227,9 @@ export default class VerificationPanel extends React.PureComponent { if (request.isSelfVerification) { const device = this._getDevice(); if (!device) { - // This shouldn't happen, although rageshake would indicate that it is: log a warn - // and leave the message slightly broken (avoid adding a translatable string for a - // case that shouldn't be happening). + // This can happen if the device is logged out while we're still showing verification + // UI for it. + // Leave the message slightly broken for this edge case (avoid adding a translatable string) console.warn("Verified device we don't know about: " + this.props.request.channel.deviceId); } description = _t("You've successfully verified %(deviceName)s (%(deviceId)s)!", { From ae2645b69ba5ec7b7a5dea0561021e5ee9da5741 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 May 2020 12:42:16 +0100 Subject: [PATCH 3/4] Provide separate translatable for case where we have no device --- .../views/right_panel/VerificationPanel.js | 11 ++++++----- .../views/verification/VerificationShowSas.js | 16 ++++++++++------ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/components/views/right_panel/VerificationPanel.js b/src/components/views/right_panel/VerificationPanel.js index 76507d8d4f..6bb2d3646b 100644 --- a/src/components/views/right_panel/VerificationPanel.js +++ b/src/components/views/right_panel/VerificationPanel.js @@ -229,13 +229,14 @@ export default class VerificationPanel extends React.PureComponent { if (!device) { // This can happen if the device is logged out while we're still showing verification // UI for it. - // Leave the message slightly broken for this edge case (avoid adding a translatable string) console.warn("Verified device we don't know about: " + this.props.request.channel.deviceId); + description = _t("You've successfully verified your device!"); + } else { + description = _t("You've successfully verified %(deviceName)s (%(deviceId)s)!", { + deviceName: device ? device.getDisplayName() : '', + deviceId: this.props.request.channel.deviceId, + }); } - description = _t("You've successfully verified %(deviceName)s (%(deviceId)s)!", { - deviceName: device ? device.getDisplayName() : '', - deviceId: this.props.request.channel.deviceId, - }); } else { description = _t("You've successfully verified %(displayName)s!", { displayName: member.displayName || member.name || member.userId, diff --git a/src/components/views/verification/VerificationShowSas.js b/src/components/views/verification/VerificationShowSas.js index 251c8ca04f..09374b91af 100644 --- a/src/components/views/verification/VerificationShowSas.js +++ b/src/components/views/verification/VerificationShowSas.js @@ -117,12 +117,16 @@ export default class VerificationShowSas extends React.Component { let text; if (this.state.pending) { if (this.props.isSelf) { - // device shouldn't be null in this situation but it seems like it can be, so show - // slightly broken text rather than soft-crashing (we've already logged in VerificationPanel). - text = _t("Waiting for your other session, %(deviceName)s (%(deviceId)s), to verify…", { - deviceName: this.props.device ? this.props.device.getDisplayName() : '', - deviceId: this.props.device ? this.props.device.deviceId : '', - }); + // device shouldn't be null in this situation but it can be, eg. if the device is + // logged out during verification + if (this.props.device) { + text = _t("Waiting for your other session, %(deviceName)s (%(deviceId)s), to verify…", { + deviceName: this.props.device ? this.props.device.getDisplayName() : '', + deviceId: this.props.device ? this.props.device.deviceId : '', + }); + } else { + text = _t("Waiting for your other session to verify…"); + } } else { const {displayName} = this.props; text = _t("Waiting for %(displayName)s to verify…", {displayName}); From a000be2b018d8f250f32530e5447962d52ed055c Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 May 2020 14:03:40 +0100 Subject: [PATCH 4/4] i18n --- src/i18n/strings/en_EN.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 27b4252f77..72cb0e20d8 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -492,6 +492,7 @@ "Verify this user by confirming the following number appears on their screen.": "Verify this user by confirming the following number appears on their screen.", "Unable to find a supported verification method.": "Unable to find a supported verification method.", "Waiting for your other session, %(deviceName)s (%(deviceId)s), to verify…": "Waiting for your other session, %(deviceName)s (%(deviceId)s), to verify…", + "Waiting for your other session to verify…": "Waiting for your other session to verify…", "Waiting for %(displayName)s to verify…": "Waiting for %(displayName)s to verify…", "Cancelling…": "Cancelling…", "They match": "They match", @@ -1278,6 +1279,7 @@ "Yes": "Yes", "Verify all users in a room to ensure it's secure.": "Verify all users in a room to ensure it's secure.", "In encrypted rooms, verify all users to ensure it’s secure.": "In encrypted rooms, verify all users to ensure it’s secure.", + "You've successfully verified your device!": "You've successfully verified your device!", "You've successfully verified %(deviceName)s (%(deviceId)s)!": "You've successfully verified %(deviceName)s (%(deviceId)s)!", "You've successfully verified %(displayName)s!": "You've successfully verified %(displayName)s!", "Verified": "Verified",