From f28f1d998fe7eb131d2055c0609386785d789539 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Mon, 30 Sep 2024 13:39:25 +0100
Subject: [PATCH] Improve error display for messages sent from insecure devices
 (#93)

* Add labs option to exclude unverified devices

Add a labs option which will, when set, switch into the "invisible crypto"
mode of refusing to send keys to, or decrypt messages from, devices that have
not been signed by their owner.

* DecryptionFailureBody: better error messages

Improve the error messages shown for messages from insecure devices.

* playwright: factor out `createSecondBotDevice` utility

* Playwright test for messages from insecure devices

* fixup! DecryptionFailureBody: better error messages

Use compound colour tokens, and add a background colour.

* fixup! DecryptionFailureBody: better error messages

Use compound spacing tokens
---
 playwright/e2e/crypto/event-shields.spec.ts   | 24 +++-----
 .../e2e/crypto/invisible-crypto.spec.ts       | 56 +++++++++++++++++++
 playwright/e2e/crypto/utils.ts                | 11 ++++
 .../messages/_DecryptionFailureBody.pcss      | 21 +++++++
 .../views/messages/DecryptionFailureBody.tsx  | 34 ++++++++++-
 src/i18n/strings/en_EN.json                   |  2 +
 .../messages/DecryptionFailureBody-test.tsx   | 28 ++++++++++
 .../DecryptionFailureBody-test.tsx.snap       | 15 +++++
 8 files changed, 174 insertions(+), 17 deletions(-)
 create mode 100644 playwright/e2e/crypto/invisible-crypto.spec.ts

diff --git a/playwright/e2e/crypto/event-shields.spec.ts b/playwright/e2e/crypto/event-shields.spec.ts
index 98142089fb..fa9d1959da 100644
--- a/playwright/e2e/crypto/event-shields.spec.ts
+++ b/playwright/e2e/crypto/event-shields.spec.ts
@@ -6,12 +6,16 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
 Please see LICENSE files in the repository root for full details.
 */
 
-import { Page } from "@playwright/test";
-
 import { expect, test } from "../../element-web-test";
-import { autoJoin, createSharedRoomWithUser, enableKeyBackup, logIntoElement, logOutOfElement, verify } from "./utils";
-import { Bot } from "../../pages/bot";
-import { HomeserverInstance } from "../../plugins/homeserver";
+import {
+    autoJoin,
+    createSecondBotDevice,
+    createSharedRoomWithUser,
+    enableKeyBackup,
+    logIntoElement,
+    logOutOfElement,
+    verify,
+} from "./utils";
 
 test.describe("Cryptography", function () {
     test.use({
@@ -296,13 +300,3 @@ test.describe("Cryptography", function () {
         });
     });
 });
-
-async function createSecondBotDevice(page: Page, homeserver: HomeserverInstance, bob: Bot) {
-    const bobSecondDevice = new Bot(page, homeserver, {
-        bootstrapSecretStorage: false,
-        bootstrapCrossSigning: false,
-    });
-    bobSecondDevice.setCredentials(await homeserver.loginUser(bob.credentials.userId, bob.credentials.password));
-    await bobSecondDevice.prepareClient();
-    return bobSecondDevice;
-}
diff --git a/playwright/e2e/crypto/invisible-crypto.spec.ts b/playwright/e2e/crypto/invisible-crypto.spec.ts
new file mode 100644
index 0000000000..c53bacd32c
--- /dev/null
+++ b/playwright/e2e/crypto/invisible-crypto.spec.ts
@@ -0,0 +1,56 @@
+/*
+Copyright 2024 New Vector Ltd.
+
+SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
+Please see LICENSE files in the repository root for full details.
+*/
+
+import { expect, test } from "../../element-web-test";
+import { autoJoin, createSecondBotDevice, createSharedRoomWithUser, verify } from "./utils";
+import { bootstrapCrossSigningForClient } from "../../pages/client.ts";
+
+/** Tests for the "invisible crypto" behaviour -- i.e., when the "exclude insecure devices" setting is enabled */
+test.describe("Invisible cryptography", () => {
+    test.use({
+        displayName: "Alice",
+        botCreateOpts: { displayName: "Bob" },
+        labsFlags: ["feature_exclude_insecure_devices"],
+    });
+
+    test("Messages fail to decrypt when sender is previously verified", async ({
+        page,
+        bot: bob,
+        user: aliceCredentials,
+        app,
+        homeserver,
+    }) => {
+        await app.client.bootstrapCrossSigning(aliceCredentials);
+        await autoJoin(bob);
+
+        // create an encrypted room
+        const testRoomId = await createSharedRoomWithUser(app, bob.credentials.userId, {
+            name: "TestRoom",
+            initial_state: [
+                {
+                    type: "m.room.encryption",
+                    state_key: "",
+                    content: {
+                        algorithm: "m.megolm.v1.aes-sha2",
+                    },
+                },
+            ],
+        });
+
+        // Verify Bob
+        await verify(app, bob);
+
+        // Bob logs in a new device and resets cross-signing
+        const bobSecondDevice = await createSecondBotDevice(page, homeserver, bob);
+        await bootstrapCrossSigningForClient(await bobSecondDevice.prepareClient(), bob.credentials, true);
+
+        /* should show an error for a message from a previously verified device */
+        await bobSecondDevice.sendMessage(testRoomId, "test encrypted from user that was previously verified");
+        const lastTile = page.locator(".mx_EventTile_last");
+        await expect(lastTile).toContainText("Verified identity has changed");
+    });
+});
diff --git a/playwright/e2e/crypto/utils.ts b/playwright/e2e/crypto/utils.ts
index a4d9769eb9..c198633733 100644
--- a/playwright/e2e/crypto/utils.ts
+++ b/playwright/e2e/crypto/utils.ts
@@ -377,3 +377,14 @@ export async function awaitVerifier(
         return verificationRequest.verifier;
     });
 }
+
+/** Log in a second device for the given bot user */
+export async function createSecondBotDevice(page: Page, homeserver: HomeserverInstance, bob: Bot) {
+    const bobSecondDevice = new Bot(page, homeserver, {
+        bootstrapSecretStorage: false,
+        bootstrapCrossSigning: false,
+    });
+    bobSecondDevice.setCredentials(await homeserver.loginUser(bob.credentials.userId, bob.credentials.password));
+    await bobSecondDevice.prepareClient();
+    return bobSecondDevice;
+}
diff --git a/res/css/views/messages/_DecryptionFailureBody.pcss b/res/css/views/messages/_DecryptionFailureBody.pcss
index 5dfdd7b7ae..64a09be7ef 100644
--- a/res/css/views/messages/_DecryptionFailureBody.pcss
+++ b/res/css/views/messages/_DecryptionFailureBody.pcss
@@ -10,3 +10,24 @@ Please see LICENSE files in the repository root for full details.
     color: $secondary-content;
     font-style: italic;
 }
+
+/* Formatting for the "Verified identity has changed" error */
+.mx_DecryptionFailureVerifiedIdentityChanged > span {
+    /* Show it in red */
+    color: var(--cpd-color-text-critical-primary);
+    background-color: var(--cpd-color-bg-critical-subtle);
+
+    /* With a red border */
+    border: 1px solid var(--cpd-color-border-critical-subtle);
+    border-radius: $font-16px;
+
+    /* Some space inside the border */
+    padding: var(--cpd-space-1x) var(--cpd-space-3x) var(--cpd-space-1x) var(--cpd-space-2x);
+
+    /* some space between the (!) icon and text */
+    display: inline-flex;
+    gap: var(--cpd-space-2x);
+
+    /* Center vertically */
+    align-items: center;
+}
diff --git a/src/components/views/messages/DecryptionFailureBody.tsx b/src/components/views/messages/DecryptionFailureBody.tsx
index d6e46267af..9cdc2eac7b 100644
--- a/src/components/views/messages/DecryptionFailureBody.tsx
+++ b/src/components/views/messages/DecryptionFailureBody.tsx
@@ -6,6 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
 Please see LICENSE files in the repository root for full details.
 */
 
+import classNames from "classnames";
 import React, { forwardRef, ForwardRefExoticComponent, useContext } from "react";
 import { MatrixEvent } from "matrix-js-sdk/src/matrix";
 import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api";
@@ -13,8 +14,9 @@ import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api";
 import { _t } from "../../../languageHandler";
 import { IBodyProps } from "./IBodyProps";
 import { LocalDeviceVerificationStateContext } from "../../../contexts/LocalDeviceVerificationStateContext";
+import { Icon as WarningBadgeIcon } from "../../../../res/img/compound/error-16px.svg";
 
-function getErrorMessage(mxEvent: MatrixEvent, isVerified: boolean | undefined): string {
+function getErrorMessage(mxEvent: MatrixEvent, isVerified: boolean | undefined): string | React.JSX.Element {
     switch (mxEvent.decryptionFailureReason) {
         case DecryptionFailureCode.MEGOLM_KEY_WITHHELD_FOR_UNVERIFIED_DEVICE:
             return _t("timeline|decryption_failure|blocked");
@@ -32,16 +34,44 @@ function getErrorMessage(mxEvent: MatrixEvent, isVerified: boolean | undefined):
             break;
 
         case DecryptionFailureCode.HISTORICAL_MESSAGE_USER_NOT_JOINED:
+            // TODO: event should be hidden instead of showing this error.
+            //   To be revisited as part of /~https://github.com/element-hq/element-meta/issues/2449
             return _t("timeline|decryption_failure|historical_event_user_not_joined");
+
+        case DecryptionFailureCode.SENDER_IDENTITY_PREVIOUSLY_VERIFIED:
+            return (
+                <span>
+                    <WarningBadgeIcon className="mx_Icon mx_Icon_16" />
+                    {_t("timeline|decryption_failure|sender_identity_previously_verified")}
+                </span>
+            );
+
+        case DecryptionFailureCode.UNSIGNED_SENDER_DEVICE:
+            // TODO: event should be hidden instead of showing this error.
+            //   To be revisited as part of /~https://github.com/element-hq/element-meta/issues/2449
+            return _t("timeline|decryption_failure|sender_unsigned_device");
     }
     return _t("timeline|decryption_failure|unable_to_decrypt");
 }
 
+/** Get an extra CSS class, specific to the decryption failure reason */
+function errorClassName(mxEvent: MatrixEvent): string | null {
+    switch (mxEvent.decryptionFailureReason) {
+        case DecryptionFailureCode.SENDER_IDENTITY_PREVIOUSLY_VERIFIED:
+            return "mx_DecryptionFailureVerifiedIdentityChanged";
+
+        default:
+            return null;
+    }
+}
+
 // A placeholder element for messages that could not be decrypted
 export const DecryptionFailureBody = forwardRef<HTMLDivElement, IBodyProps>(({ mxEvent }, ref): React.JSX.Element => {
     const verificationState = useContext(LocalDeviceVerificationStateContext);
+    const classes = classNames("mx_DecryptionFailureBody", "mx_EventTile_content", errorClassName(mxEvent));
+
     return (
-        <div className="mx_DecryptionFailureBody mx_EventTile_content" ref={ref}>
+        <div className={classes} ref={ref}>
             {getErrorMessage(mxEvent, verificationState)}
         </div>
     );
diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
index 64680a93a3..5a0b9d7cac 100644
--- a/src/i18n/strings/en_EN.json
+++ b/src/i18n/strings/en_EN.json
@@ -3303,6 +3303,8 @@
             "historical_event_no_key_backup": "Historical messages are not available on this device",
             "historical_event_unverified_device": "You need to verify this device for access to historical messages",
             "historical_event_user_not_joined": "You don't have access to this message",
+            "sender_identity_previously_verified": "Verified identity has changed",
+            "sender_unsigned_device": "Encrypted by a device not verified by its owner.",
             "unable_to_decrypt": "Unable to decrypt message"
         },
         "disambiguated_profile": "%(displayName)s (%(matrixId)s)",
diff --git a/test/components/views/messages/DecryptionFailureBody-test.tsx b/test/components/views/messages/DecryptionFailureBody-test.tsx
index 8ba4503446..021e58d071 100644
--- a/test/components/views/messages/DecryptionFailureBody-test.tsx
+++ b/test/components/views/messages/DecryptionFailureBody-test.tsx
@@ -103,4 +103,32 @@ describe("DecryptionFailureBody", () => {
         // Then
         expect(container).toHaveTextContent("You don't have access to this message");
     });
+
+    it("should handle messages from users who change identities after verification", async () => {
+        // When
+        const event = await mkDecryptionFailureMatrixEvent({
+            code: DecryptionFailureCode.SENDER_IDENTITY_PREVIOUSLY_VERIFIED,
+            msg: "User previously verified",
+            roomId: "fakeroom",
+            sender: "fakesender",
+        });
+        const { container } = customRender(event);
+
+        // Then
+        expect(container).toMatchSnapshot();
+    });
+
+    it("should handle messages from unverified devices", async () => {
+        // When
+        const event = await mkDecryptionFailureMatrixEvent({
+            code: DecryptionFailureCode.UNSIGNED_SENDER_DEVICE,
+            msg: "Unsigned device",
+            roomId: "fakeroom",
+            sender: "fakesender",
+        });
+        const { container } = customRender(event);
+
+        // Then
+        expect(container).toHaveTextContent("Encrypted by a device not verified by its owner");
+    });
 });
diff --git a/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap b/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap
index 22e44fd16a..b2ba5b2a2e 100644
--- a/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap
+++ b/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap
@@ -19,3 +19,18 @@ exports[`DecryptionFailureBody Should display "Unable to decrypt message" 1`] =
   </div>
 </div>
 `;
+
+exports[`DecryptionFailureBody should handle messages from users who change identities after verification 1`] = `
+<div>
+  <div
+    class="mx_DecryptionFailureBody mx_EventTile_content mx_DecryptionFailureVerifiedIdentityChanged"
+  >
+    <span>
+      <div
+        class="mx_Icon mx_Icon_16"
+      />
+      Verified identity has changed
+    </span>
+  </div>
+</div>
+`;