In force-verify mode, prevent bypassing by cancelling device verification

playwright/e2e/login/login-consent.spec.ts

@@ -13,6 +13,7 @@ import { selectHomeserver } from "../utils";
 import { type Credentials, type HomeserverInstance } from "../../plugins/homeserver";
 import { consentHomeserver } from "../../plugins/homeserver/synapse/consentHomeserver.ts";
 import { isDendrite } from "../../plugins/homeserver/dendrite";
+import { createBot } from "../crypto/utils.ts";
 
 // This test requires fixed credentials for the device signing keys below to work
 const username = "user1234";
@@ -258,6 +259,34 @@ test.describe("Login", () => {
 
                     await expect(h1.locator(".mx_CompleteSecurity_skip")).toHaveCount(0);
                 });
+
+                test("Continues to show verification prompt after cancelling device verification", async ({
+                    page,
+                    homeserver,
+                    credentials,
+                }) => {
+                    // Create a different device which is cross-signed, meaning we need to verify this device
+                    await createBot(page, homeserver, credentials, true);
+
+                    // Wait to avoid homeserver rate limit on logins
+                    await page.waitForTimeout(100);
+
+                    // Load the page and see that we are asked to verify
+                    await page.goto("/#/welcome");
+                    await login(page, homeserver, credentials);
+                    let h1 = page.getByRole("heading", { name: "Verify this device", level: 1 });
+                    await expect(h1).toBeVisible();
+
+                    // Click "Verify with another device"
+                    await page.getByRole("button", { name: "Verify with another device" }).click();
+
+                    // Cancel the new dialog
+                    await page.getByRole("button", { name: "Close dialog" }).click();
+
+                    // Check that we are still being asked to verify
+                    h1 = page.getByRole("heading", { name: "Verify this device", level: 1 });
+                    await expect(h1).toBeVisible();
+                });
             });
         });
     });

src/components/structures/MatrixChat.tsx

@@ -1388,7 +1388,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
                 // so show the homepage.
                 dis.dispatch<ViewHomePagePayload>({ action: Action.ViewHomePage, justRegistered: true });
             }
-        } else {
+        } else if (!(await this.shouldForceVerification())) {
             this.showScreenAfterLogin();
         }
 
@@ -2003,9 +2003,17 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
     };
 
     // complete security / e2e setup has finished
-    private onCompleteSecurityE2eSetupFinished = (): void => {
-        // This is async but we making this function async to wait for it isn't useful
-        this.onShowPostLoginScreen().catch((e) => {
+    private onCompleteSecurityE2eSetupFinished = async (): Promise<void> => {
+        const forceVerify = await this.shouldForceVerification();
+        if (forceVerify) {
+            const isVerified = await MatrixClientPeg.safeGet().getCrypto()?.isCrossSigningReady();
+            if (!isVerified) {
+                // We must verify but we haven't yet verified - don't continue logging in
+                return;
+            }
+        }
+
+        await this.onShowPostLoginScreen().catch((e) => {
             logger.error("Exception showing post-login screen", e);
         });
     };

test/unit-tests/components/structures/MatrixChat-test.tsx

@@ -22,7 +22,12 @@ import { logger } from "matrix-js-sdk/src/logger";
 import { OidcError } from "matrix-js-sdk/src/oidc/error";
 import { type BearerTokenResponse } from "matrix-js-sdk/src/oidc/validate";
 import { defer, type IDeferred, sleep } from "matrix-js-sdk/src/utils";
-import { CryptoEvent, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api";
+import {
+    CryptoEvent,
+    type DeviceVerificationStatus,
+    UserVerificationStatus,
+    type CryptoApi,
+} from "matrix-js-sdk/src/crypto-api";
 
 import MatrixChat from "../../../../src/components/structures/MatrixChat";
 import * as StorageAccess from "../../../../src/utils/StorageAccess";
@@ -62,6 +67,7 @@ import { UIFeature } from "../../../../src/settings/UIFeature";
 import AutoDiscoveryUtils from "../../../../src/utils/AutoDiscoveryUtils";
 import { type ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig";
 import Modal from "../../../../src/Modal.tsx";
+import { SetupEncryptionStore } from "../../../../src/stores/SetupEncryptionStore.ts";
 
 jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({
     completeAuthorizationCodeGrant: jest.fn(),
@@ -894,13 +900,92 @@ describe("<MatrixChat />", () => {
         });
 
         describe("unskippable verification", () => {
-            it("should show the complete security screen if unskippable verification is enabled", async () => {
+            beforeEach(() => {
+                // Force verification is turned on in settings
                 defaultProps.config.force_verification = true;
+
+                // And this device is being force-verified (because it logged in after
+                // enforcement was turned on).
                 localStorage.setItem("must_verify_device", "true");
+
+                // lostKeys returns false, meaning there are other devices to verify against
+                const realStore = SetupEncryptionStore.sharedInstance();
+                jest.spyOn(realStore, "lostKeys").mockReturnValue(false);
+            });
+
+            afterEach(() => {
+                jest.restoreAllMocks();
+                // Reset things back to how they were before we started
+                defaultProps.config.force_verification = false;
+                localStorage.removeItem("must_verify_device");
+            });
+
+            it("should show the complete security screen if unskippable verification is enabled", async () => {
+                // Given we have force verification on, and an existing logged-in session
+                // that is not verified (see beforeEach())
+
+                // When we render MatrixChat
+                getComponent();
+
+                // Then we are asked to verify our device
+                await screen.findByRole("heading", { name: "Verify this device", level: 1 });
+
+                // Sanity: we are not racing with another screen update, so this heading stays visible
+                await screen.findByRole("heading", { name: "Verify this device", level: 1 });
+            });
+
+            it("should not open app after cancelling device verify if unskippable verification is on", async () => {
+                // See https://github.com/element-hq/element-web/issues/29230
+                // We used to allow bypassing force verification by choosing "Verify with
+                // another device" and not completing the verification.
+
+                // Given we have force verification on, and an existing logged-in session
+                // that is not verified (see beforeEach())
+
+                // And our crypto is set up
+                mockClient.getCrypto.mockReturnValue(createMockCrypto());
+
+                // And MatrixChat is rendered
                 getComponent();
 
-                await screen.findByRole("heading", { name: "Unable to verify this device", level: 1 });
+                // When we click "Verify with another device"
+                await screen.findByRole("heading", { name: "Verify this device", level: 1 });
+                const verify = screen.getByRole("button", { name: "Verify with another device" });
+                act(() => verify.click());
+
+                // And close the device verification dialog
+                const closeButton = await screen.findByRole("button", { name: "Close dialog" });
+                act(() => closeButton.click());
+
+                // Then we are not allowed in - we are still being asked to verify
+                await screen.findByRole("heading", { name: "Verify this device", level: 1 });
             });
+
+            function createMockCrypto(): CryptoApi {
+                return {
+                    getVersion: jest.fn().mockReturnValue("Version 0"),
+                    getVerificationRequestsToDeviceInProgress: jest.fn().mockReturnValue([]),
+                    getUserDeviceInfo: jest.fn().mockReturnValue({
+                        get: jest
+                            .fn()
+                            .mockReturnValue(
+                                new Map([
+                                    ["devid", { dehydrated: false, getIdentityKey: jest.fn().mockReturnValue("k") }],
+                                ]),
+                            ),
+                    }),
+                    getUserVerificationStatus: jest
+                        .fn()
+                        .mockResolvedValue(new UserVerificationStatus(true, true, false)),
+                    setDeviceIsolationMode: jest.fn(),
+                    isDehydrationSupported: jest.fn().mockReturnValue(false),
+                    getDeviceVerificationStatus: jest
+                        .fn()
+                        .mockResolvedValue({ signedByOwner: true } as DeviceVerificationStatus),
+                    isCrossSigningReady: jest.fn().mockReturnValue(false),
+                    requestOwnUserVerification: jest.fn().mockResolvedValue({ cancel: jest.fn() }),
+                } as any;
+            }
         });
     });