Fetch the user's device info before processing a verification request (#5030)

* Use checked way to get OlmMachine

* Factor out two variables in onKeyVerificationEvent

* Make sure verification test waits for the request to be processed

* Fetch the user's device info before processing a verification request

If we don't have the device info for a user when we receive their
verification request, we ignore it. This change gives us the best
possible chance of having the right device data before we try to process
the verification.

Fixes #30693
Fixes #27819

Author: andybalaam

spec/integ/crypto/verification.spec.ts

@@ -46,8 +46,8 @@ import {
     type Verifier,
     VerifierEvent,
 } from "../../../src/crypto-api/verification";
-import { escapeRegExp } from "../../../src/utils";
-import { awaitDecryption, emitPromise, getSyncResponse, syncPromise } from "../../test-utils/test-utils";
+import { escapeRegExp, sleep } from "../../../src/utils";
+import { awaitDecryption, emitPromise, getSyncResponse, syncPromise, waitFor } from "../../test-utils/test-utils";
 import { SyncResponder } from "../../test-utils/SyncResponder";
 import {
     BACKUP_DECRYPTION_KEY_BASE64,
@@ -79,11 +79,6 @@ import {
 import { type KeyBackupInfo, CryptoEvent } from "../../../src/crypto-api";
 import { encodeBase64 } from "../../../src/base64";
 
-// The verification flows use javascript timers to set timeouts. We tell jest to use mock timer implementations
-// to ensure that we don't end up with dangling timeouts.
-// But the wasm bindings of matrix-sdk-crypto rely on a working `queueMicrotask`.
-jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
-
 beforeAll(async () => {
     // we use the libolm primitives in the test, so init the Olm library
     await Olm.init();
@@ -96,6 +91,13 @@ beforeAll(async () => {
     await RustSdkCryptoJs.initAsync();
 }, 10000);
 
+beforeEach(() => {
+    // The verification flows use javascript timers to set timeouts. We tell jest to use mock timer implementations
+    // to ensure that we don't end up with dangling timeouts.
+    // But the wasm bindings of matrix-sdk-crypto rely on a working `queueMicrotask`.
+    jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
+});
+
 afterEach(() => {
     // reset fake-indexeddb after each test, to make sure we don't leak connections
     // cf https://github.com/dumbmatter/fakeIndexedDB#wipingresetting-the-indexeddb-for-a-fresh-state
@@ -1080,6 +1082,13 @@ describe("verification", () => {
         });
 
         it("ignores old verification requests", async () => {
+            const debug = jest.fn();
+            const info = jest.fn();
+            const warn = jest.fn();
+
+            // @ts-ignore overriding RustCrypto's logger
+            aliceClient.getCrypto()!.logger = { debug, info, warn };
+
             const eventHandler = jest.fn();
             aliceClient.on(CryptoEvent.VerificationRequestReceived, eventHandler);
 
@@ -1094,6 +1103,16 @@ describe("verification", () => {
             const matrixEvent = room.getLiveTimeline().getEvents()[0];
             expect(matrixEvent.getId()).toEqual(verificationRequestEvent.event_id);
 
+            // Wait until the request has been processed. We use a real sleep()
+            // here to make sure any background async tasks are completed.
+            jest.useRealTimers();
+            await waitFor(async () => {
+                expect(info).toHaveBeenCalledWith(
+                    expect.stringMatching(/^Ignoring just-received verification request/),
+                );
+                sleep(100);
+            });
+
             // check that an event has not been raised, and that the request is not found
             expect(eventHandler).not.toHaveBeenCalled();
             expect(

spec/unit/rust-crypto/rust-crypto.spec.ts

@@ -2419,6 +2419,135 @@ describe("RustCrypto", () => {
             expect(mockOlmMachine.receiveRoomKeyBundle.mock.calls[0][1]).toEqual(new TextEncoder().encode("asdfghjkl"));
         });
     });
+
+    describe("Verification requests", () => {
+        it("fetches device details before room verification requests", async () => {
+            // Given a RustCrypto
+            const olmMachine = mockedOlmMachine();
+            const outgoingRequestProcessor = mockedOutgoingRequestProcessor();
+            const rustCrypto = makeRustCrypto(olmMachine, outgoingRequestProcessor);
+
+            // When we receive a room verification request
+            const event = mockedEvent("!r:s.co", "@u:s.co", "m.room.message", "m.key.verification.request");
+            await rustCrypto.onLiveEventFromSync(event);
+
+            // Then we first fetch device details
+            expect(outgoingRequestProcessor.makeOutgoingRequest).toHaveBeenCalled();
+
+            // And we handle the verification event as normal
+            expect(olmMachine.receiveVerificationEvent).toHaveBeenCalled();
+        });
+
+        it("does not fetch device details before other verification events", async () => {
+            // Given a RustCrypto
+            const olmMachine = mockedOlmMachine();
+            const outgoingRequestProcessor = mockedOutgoingRequestProcessor();
+            const rustCrypto = makeRustCrypto(olmMachine, outgoingRequestProcessor);
+
+            // When we receive some verification event that is not a room request
+            const event = mockedEvent("!r:s.co", "@u:s.co", "m.key.verification.start");
+            await rustCrypto.onLiveEventFromSync(event);
+
+            // Then we do not fetch device details
+            expect(outgoingRequestProcessor.makeOutgoingRequest).not.toHaveBeenCalled();
+
+            // And we handle the verification event as normal
+            expect(olmMachine.receiveVerificationEvent).toHaveBeenCalled();
+        });
+
+        it("throws an error if sender is missing", async () => {
+            // Given a RustCrypto
+            const olmMachine = mockedOlmMachine();
+            const outgoingRequestProcessor = mockedOutgoingRequestProcessor();
+            const rustCrypto = makeRustCrypto(olmMachine, outgoingRequestProcessor);
+
+            // When we receive a verification event without a sender
+            // Then we throw
+            const event = mockedEvent("!r:s.co", null, "m.key.verification.start");
+
+            await expect(async () => await rustCrypto.onLiveEventFromSync(event)).rejects.toThrow(
+                "missing sender in the event",
+            );
+
+            // And we do not fetch device details or handle the event
+            expect(outgoingRequestProcessor.makeOutgoingRequest).not.toHaveBeenCalled();
+            expect(olmMachine.receiveVerificationEvent).not.toHaveBeenCalled();
+        });
+
+        it("throws an error if room is missing", async () => {
+            // Given a RustCrypto
+            const olmMachine = mockedOlmMachine();
+            const outgoingRequestProcessor = mockedOutgoingRequestProcessor();
+            const rustCrypto = makeRustCrypto(olmMachine, outgoingRequestProcessor);
+
+            // When we receive a verification event without a sender
+            // Then we throw
+            const event = mockedEvent(null, "@u:s.co", "m.key.verification.start");
+
+            await expect(async () => await rustCrypto.onLiveEventFromSync(event)).rejects.toThrow(
+                "missing roomId in the event",
+            );
+
+            // And we do not fetch device details or handle the event
+            expect(outgoingRequestProcessor.makeOutgoingRequest).not.toHaveBeenCalled();
+            expect(olmMachine.receiveVerificationEvent).not.toHaveBeenCalled();
+        });
+
+        function mockedOlmMachine(): Mocked<OlmMachine> {
+            return {
+                queryKeysForUsers: jest.fn(),
+                getVerificationRequest: jest.fn(),
+                receiveVerificationEvent: jest.fn(),
+            } as unknown as Mocked<OlmMachine>;
+        }
+
+        function makeRustCrypto(
+            olmMachine: OlmMachine,
+            outgoingRequestProcessor: OutgoingRequestProcessor,
+        ): RustCrypto {
+            const rustCrypto = new RustCrypto(
+                new DebugLogger(debug("test Verification requests")),
+                olmMachine,
+                {} as unknown as MatrixHttpApi<IHttpOpts & { onlyData: true }>,
+                TEST_USER,
+                TEST_DEVICE_ID,
+                {} as ServerSideSecretStorage,
+                {} as CryptoCallbacks,
+            );
+
+            // @ts-ignore mocking outgoingRequestProcessor
+            rustCrypto.outgoingRequestProcessor = outgoingRequestProcessor;
+
+            return rustCrypto;
+        }
+
+        function mockedOutgoingRequestProcessor(): OutgoingRequestProcessor {
+            return {
+                makeOutgoingRequest: jest.fn(),
+            } as unknown as Mocked<OutgoingRequestProcessor>;
+        }
+
+        function mockedEvent(
+            roomId: string | null,
+            senderId: string | null,
+            eventType: string,
+            msgtype?: string | undefined,
+        ): MatrixEvent {
+            return {
+                isState: jest.fn().mockReturnValue(false),
+                getUnsigned: jest.fn().mockReturnValue({}),
+                isDecryptionFailure: jest.fn(),
+                isEncrypted: jest.fn(),
+                getType: jest.fn().mockReturnValue(eventType),
+                getRoomId: jest.fn().mockReturnValue(roomId),
+                getSender: jest.fn().mockReturnValue(senderId),
+                getId: jest.fn(),
+                getStateKey: jest.fn(),
+                getContent: jest.fn().mockReturnValue({ msgtype: msgtype }),
+                getTs: jest.fn(),
+            } as unknown as MatrixEvent;
+        }
+    });
 });
 
 /** Build a MatrixHttpApi instance */

src/rust-crypto/rust-crypto.ts

@@ -2053,32 +2053,47 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, CryptoEventH
      */
     private async onKeyVerificationEvent(event: MatrixEvent): Promise<void> {
         const roomId = event.getRoomId();
+        const senderId = event.getSender();
 
         if (!roomId) {
             throw new Error("missing roomId in the event");
         }
 
+        if (!senderId) {
+            throw new Error("missing sender in the event");
+        }
+
         this.logger.debug(
             `Incoming verification event ${event.getId()} type ${event.getType()} from ${event.getSender()}`,
         );
 
-        await this.olmMachine.receiveVerificationEvent(
+        const isRoomVerificationRequest =
+            event.getType() === EventType.RoomMessage && event.getContent().msgtype === MsgType.KeyVerificationRequest;
+
+        if (isRoomVerificationRequest) {
+            // Before processing an in-room verification request, we need to
+            // make sure we have the sender's device information - otherwise we
+            // will immediately abort verification. So we explicitly fetch it
+            // from /keys/query and wait for that request to complete before we
+            // call receiveVerificationEvent.
+            const req = this.getOlmMachineOrThrow().queryKeysForUsers([new RustSdkCryptoJs.UserId(senderId)]);
+            await this.outgoingRequestProcessor.makeOutgoingRequest(req);
+        }
+
+        await this.getOlmMachineOrThrow().receiveVerificationEvent(
             JSON.stringify({
                 event_id: event.getId(),
                 type: event.getType(),
-                sender: event.getSender(),
+                sender: senderId,
                 state_key: event.getStateKey(),
                 content: event.getContent(),
                 origin_server_ts: event.getTs(),
             }),
             new RustSdkCryptoJs.RoomId(roomId),
         );
 
-        if (
-            event.getType() === EventType.RoomMessage &&
-            event.getContent().msgtype === MsgType.KeyVerificationRequest
-        ) {
-            this.onIncomingKeyVerificationRequest(event.getSender()!, event.getId()!);
+        if (isRoomVerificationRequest) {
+            this.onIncomingKeyVerificationRequest(senderId, event.getId()!);
         }
 
         // that may have caused us to queue up outgoing requests, so make sure we send them.