From 3be9f0062a19994128d3912695d5b6533be54816 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Mon, 23 Feb 2026 01:23:38 -0500 Subject: [PATCH] fix(matrix-js): harden sas verification flow --- .../src/matrix/sdk/crypto-bootstrap.test.ts | 36 ++++++++ .../src/matrix/sdk/crypto-bootstrap.ts | 14 +++ .../matrix/sdk/verification-manager.test.ts | 92 ++++++++++++++++++- .../src/matrix/sdk/verification-manager.ts | 68 +++++++++++++- 4 files changed, 208 insertions(+), 2 deletions(-) diff --git a/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.test.ts b/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.test.ts index bf0a540eb42..761fb4d3ccc 100644 --- a/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.test.ts +++ b/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.test.ts @@ -1,3 +1,4 @@ +import { VerificationPhase } from "matrix-js-sdk/lib/crypto-api/verification.js"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { MatrixCryptoBootstrapper, type MatrixCryptoBootstrapperDeps } from "./crypto-bootstrap.js"; import type { MatrixCryptoBootstrapApi, MatrixRawEvent } from "./types.js"; @@ -293,6 +294,41 @@ describe("MatrixCryptoBootstrapper", () => { expect(verificationRequest.accept).toHaveBeenCalledTimes(1); }); + it("skips auto-accept for requests that are no longer requested", async () => { + const deps = createBootstrapperDeps(); + const listeners = new Map void>(); + const crypto = createCryptoApi({ + getDeviceVerificationStatus: vi.fn(async () => ({ + isVerified: () => true, + })), + on: vi.fn((eventName: string, listener: (...args: unknown[]) => void) => { + listeners.set(eventName, listener); + }), + }); + const bootstrapper = new MatrixCryptoBootstrapper( + deps as unknown as MatrixCryptoBootstrapperDeps, + ); + + await bootstrapper.bootstrap(crypto); + + const verificationRequest = { + otherUserId: "@alice:example.org", + isSelfVerification: false, + initiatedByMe: false, + phase: VerificationPhase.Cancelled, + accepting: false, + declining: false, + accept: vi.fn(async () => {}), + }; + const listener = Array.from(listeners.entries()).find(([eventName]) => + eventName.toLowerCase().includes("verificationrequest"), + )?.[1]; + expect(listener).toBeTypeOf("function"); + await listener?.(verificationRequest); + + expect(verificationRequest.accept).not.toHaveBeenCalled(); + }); + it("registers verification listeners only once across repeated bootstrap calls", async () => { const deps = createBootstrapperDeps(); const crypto = createCryptoApi({ diff --git a/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.ts b/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.ts index af2a9138434..9af37da24ae 100644 --- a/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.ts +++ b/extensions/matrix-js/src/matrix/sdk/crypto-bootstrap.ts @@ -1,4 +1,5 @@ import { CryptoEvent } from "matrix-js-sdk/lib/crypto-api/CryptoEvent.js"; +import { VerificationPhase } from "matrix-js-sdk/lib/crypto-api/verification.js"; import type { MatrixDecryptBridge } from "./decrypt-bridge.js"; import { LogService } from "./logger.js"; import type { MatrixRecoveryKeyStore } from "./recovery-key-store.js"; @@ -233,6 +234,12 @@ export class MatrixCryptoBootstrapper { const otherUserId = verificationRequest.otherUserId; const isSelfVerification = verificationRequest.isSelfVerification; const initiatedByMe = verificationRequest.initiatedByMe; + const phase = + typeof verificationRequest.phase === "number" + ? verificationRequest.phase + : VerificationPhase.Requested; + const accepting = verificationRequest.accepting === true; + const declining = verificationRequest.declining === true; if (isSelfVerification || initiatedByMe) { LogService.debug( @@ -241,6 +248,13 @@ export class MatrixCryptoBootstrapper { ); return; } + if (phase !== VerificationPhase.Requested || accepting || declining) { + LogService.debug( + "MatrixClientLite", + `Skipping auto-accept for ${otherUserId} in phase=${phase} accepting=${accepting} declining=${declining}`, + ); + return; + } try { LogService.info( diff --git a/extensions/matrix-js/src/matrix/sdk/verification-manager.test.ts b/extensions/matrix-js/src/matrix/sdk/verification-manager.test.ts index 804db28989f..883d43cbbd6 100644 --- a/extensions/matrix-js/src/matrix/sdk/verification-manager.test.ts +++ b/extensions/matrix-js/src/matrix/sdk/verification-manager.test.ts @@ -1,5 +1,8 @@ import { EventEmitter } from "node:events"; -import { VerificationPhase } from "matrix-js-sdk/lib/crypto-api/verification.js"; +import { + VerificationPhase, + VerificationRequestEvent, +} from "matrix-js-sdk/lib/crypto-api/verification.js"; import { describe, expect, it, vi } from "vitest"; import { MatrixVerificationManager, @@ -154,6 +157,8 @@ describe("MatrixVerificationManager", () => { const started = await manager.startVerification(tracked.id, "sas"); expect(started.hasSas).toBe(true); + expect(started.sas?.decimal).toEqual([111, 222, 333]); + expect(started.sas?.emoji?.length).toBe(3); const sas = manager.getVerificationSas(tracked.id); expect(sas.decimal).toEqual([111, 222, 333]); @@ -166,6 +171,91 @@ describe("MatrixVerificationManager", () => { expect(mismatch).toHaveBeenCalledTimes(1); }); + it("auto-starts an incoming verifier exposed via request change events", async () => { + const verify = vi.fn(async () => {}); + const verifier = new MockVerifier( + { + sas: { + decimal: [6158, 1986, 3513], + emoji: [ + ["gift", "Gift"], + ["globe", "Globe"], + ["horse", "Horse"], + ], + }, + confirm: vi.fn(async () => {}), + mismatch: vi.fn(), + cancel: vi.fn(), + }, + null, + verify, + ); + const request = new MockVerificationRequest({ + transactionId: "txn-incoming-change", + verifier: undefined, + }); + const manager = new MatrixVerificationManager(); + const tracked = manager.trackVerificationRequest(request); + + request.verifier = verifier; + request.emit(VerificationRequestEvent.Change); + + await vi.waitFor(() => { + expect(verify).toHaveBeenCalledTimes(1); + }); + const summary = manager.listVerifications().find((item) => item.id === tracked.id); + expect(summary?.hasSas).toBe(true); + expect(summary?.sas?.decimal).toEqual([6158, 1986, 3513]); + expect(manager.getVerificationSas(tracked.id).decimal).toEqual([6158, 1986, 3513]); + }); + + it("auto-starts inbound SAS when request becomes ready without a verifier", async () => { + const verify = vi.fn(async () => {}); + const verifier = new MockVerifier( + { + sas: { + decimal: [1234, 5678, 9012], + emoji: [ + ["gift", "Gift"], + ["rocket", "Rocket"], + ["butterfly", "Butterfly"], + ], + }, + confirm: vi.fn(async () => {}), + mismatch: vi.fn(), + cancel: vi.fn(), + }, + null, + verify, + ); + const request = new MockVerificationRequest({ + transactionId: "txn-auto-start-sas", + initiatedByMe: false, + verifier: undefined, + }); + request.startVerification = vi.fn(async (_method: string) => { + request.phase = VerificationPhase.Started; + request.verifier = verifier; + return verifier; + }); + const manager = new MatrixVerificationManager(); + const tracked = manager.trackVerificationRequest(request); + + request.phase = VerificationPhase.Ready; + request.emit(VerificationRequestEvent.Change); + + await vi.waitFor(() => { + expect(request.startVerification).toHaveBeenCalledWith("m.sas.v1"); + }); + await vi.waitFor(() => { + expect(verify).toHaveBeenCalledTimes(1); + }); + const summary = manager.listVerifications().find((item) => item.id === tracked.id); + expect(summary?.hasSas).toBe(true); + expect(summary?.sas?.decimal).toEqual([1234, 5678, 9012]); + expect(manager.getVerificationSas(tracked.id).decimal).toEqual([1234, 5678, 9012]); + }); + it("prunes stale terminal sessions during list operations", () => { const now = new Date("2026-02-08T15:00:00.000Z").getTime(); const nowSpy = vi.spyOn(Date, "now"); diff --git a/extensions/matrix-js/src/matrix/sdk/verification-manager.ts b/extensions/matrix-js/src/matrix/sdk/verification-manager.ts index 87c8836461a..6638bb6fa7b 100644 --- a/extensions/matrix-js/src/matrix/sdk/verification-manager.ts +++ b/extensions/matrix-js/src/matrix/sdk/verification-manager.ts @@ -22,6 +22,10 @@ export type MatrixVerificationSummary = { chosenMethod?: string | null; canAccept: boolean; hasSas: boolean; + sas?: { + decimal?: [number, number, number]; + emoji?: Array<[string, string]>; + }; hasReciprocateQr: boolean; completed: boolean; error?: string; @@ -96,6 +100,7 @@ type MatrixVerificationSession = { activeVerifier?: MatrixVerifierLike; verifyPromise?: Promise; verifyStarted: boolean; + startRequested: boolean; sasCallbacks?: MatrixShowSasCallbacks; reciprocateQrCallbacks?: MatrixShowQrCodeCallbacks; }; @@ -179,6 +184,10 @@ export class MatrixVerificationManager { const methods = Array.isArray(methodsRaw) ? methodsRaw.filter((entry): entry is string => typeof entry === "string") : []; + const sasCallbacks = session.sasCallbacks ?? session.activeVerifier?.getShowSasCallbacks(); + if (sasCallbacks) { + session.sasCallbacks = sasCallbacks; + } const canAccept = phase < VerificationPhase.Ready && !accepting && !declining; return { id: session.id, @@ -194,7 +203,13 @@ export class MatrixVerificationManager { methods, chosenMethod: this.readRequestValue(request, () => request.chosenMethod ?? null, null), canAccept, - hasSas: Boolean(session.sasCallbacks), + hasSas: Boolean(sasCallbacks), + sas: sasCallbacks + ? { + decimal: sasCallbacks.sas.decimal, + emoji: sasCallbacks.sas.emoji, + } + : undefined, hasReciprocateQr: Boolean(session.reciprocateQrCallbacks), completed: phase === VerificationPhase.Done, error: session.error, @@ -229,9 +244,56 @@ export class MatrixVerificationManager { if (verifier) { this.attachVerifierToVerificationSession(session, verifier); } + this.maybeAutoStartInboundSas(session); }); } + private maybeAutoStartInboundSas(session: MatrixVerificationSession): void { + if (session.activeVerifier || session.verifyStarted || session.startRequested) { + return; + } + if (this.readRequestValue(session.request, () => session.request.initiatedByMe, true)) { + return; + } + const phase = this.readRequestValue( + session.request, + () => session.request.phase, + VerificationPhase.Requested, + ); + if (phase < VerificationPhase.Ready || phase >= VerificationPhase.Cancelled) { + return; + } + const methodsRaw = this.readRequestValue( + session.request, + () => session.request.methods, + [], + ); + const methods = Array.isArray(methodsRaw) + ? methodsRaw.filter((entry): entry is string => typeof entry === "string") + : []; + const chosenMethod = this.readRequestValue( + session.request, + () => session.request.chosenMethod, + null, + ); + const supportsSas = + methods.includes(VerificationMethod.Sas) || chosenMethod === VerificationMethod.Sas; + if (!supportsSas) { + return; + } + + session.startRequested = true; + void session.request + .startVerification(VerificationMethod.Sas) + .then((verifier) => { + this.attachVerifierToVerificationSession(session, verifier); + this.touchVerificationSession(session); + }) + .catch(() => { + session.startRequested = false; + }); + } + private attachVerifierToVerificationSession( session: MatrixVerificationSession, verifier: MatrixVerifierLike, @@ -250,6 +312,7 @@ export class MatrixVerificationManager { const verifierObj = verifier as unknown as object; if (this.trackedVerificationVerifiers.has(verifierObj)) { + this.ensureVerificationStarted(session); return; } this.trackedVerificationVerifiers.add(verifierObj); @@ -266,6 +329,7 @@ export class MatrixVerificationManager { session.error = err instanceof Error ? err.message : String(err); this.touchVerificationSession(session); }); + this.ensureVerificationStarted(session); } private ensureVerificationStarted(session: MatrixVerificationSession): void { @@ -316,6 +380,7 @@ export class MatrixVerificationManager { createdAtMs: now, updatedAtMs: now, verifyStarted: false, + startRequested: false, }; this.verificationSessions.set(session.id, session); this.ensureVerificationRequestTracked(session); @@ -323,6 +388,7 @@ export class MatrixVerificationManager { if (verifier) { this.attachVerifierToVerificationSession(session, verifier); } + this.maybeAutoStartInboundSas(session); return this.buildVerificationSummary(session); }