From a213d177f9e36f3b089e78fa89d47bdacb359fce Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 7 Oct 2025 09:13:31 +0100 Subject: [PATCH] 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 --- spec/integ/crypto/verification.spec.ts | 33 ++++-- spec/unit/rust-crypto/rust-crypto.spec.ts | 129 ++++++++++++++++++++++ src/rust-crypto/rust-crypto.ts | 29 +++-- 3 files changed, 177 insertions(+), 14 deletions(-) diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 0aefe75c9..d9cf48024 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/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( diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 952370d46..e9c77ef4e 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/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 { + return { + queryKeysForUsers: jest.fn(), + getVerificationRequest: jest.fn(), + receiveVerificationEvent: jest.fn(), + } as unknown as Mocked; + } + + function makeRustCrypto( + olmMachine: OlmMachine, + outgoingRequestProcessor: OutgoingRequestProcessor, + ): RustCrypto { + const rustCrypto = new RustCrypto( + new DebugLogger(debug("test Verification requests")), + olmMachine, + {} as unknown as MatrixHttpApi, + 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; + } + + 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 */ diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 8e8ccb67e..638bf37d1 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -2053,20 +2053,38 @@ export class RustCrypto extends TypedEventEmitter { 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(), @@ -2074,11 +2092,8 @@ export class RustCrypto extends TypedEventEmitter