From 86956f71e62a6561b4a6412dec26dde992960dbf Mon Sep 17 00:00:00 2001 From: Natalie K <48805412+nklock@users.noreply.github.com> Date: Wed, 29 Apr 2026 11:42:45 -0700 Subject: [PATCH] fix(matrix): close owner-side device verification loop on SAS confirm (#74542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(matrix): close owner-side device verification loop on SAS confirm After SAS confirm via the `openclaw matrix verify confirm-sas` CLI, the operator's Element X stayed in "Verifying…" because three things on the bot side did not happen before the verb returned: 1. confirmVerificationSas didn't await the rust-crypto verifier promise. `Verifier.verify()` resolves only after both sides exchange MACs and the protocol fully settles, including cross-signing-key uploads triggered by `crossSignDevice`. Returning early meant Element X's next /keys/query saw an inconsistent state and the prompt persisted. 2. The 30s auto-confirm path (used when the operator initiates from their phone) explicitly passed `{ trustOwnDevice: false }`, so the bot never cross-signed its own device on this path. The check inside trustOwnDeviceAfterConfirmedSas already gates on isSelfVerification, so flipping the flag is safe — non-self requests remain a no-op. 3. The standalone `confirmMatrixVerificationSas` action did not call `trustOwnIdentityAfterSelfVerification` (only the higher-level `runMatrixSelfVerification` path did). Without that call, the bot had not signed the operator's master key, so Element X had no path to clear the prompt without a passive sync tick. Three additive edits: - verification-manager.ts (confirmVerificationSas): await session.verifyPromise after confirmSasForSession returns. verifyPromise is the .then().catch() chain set by ensureVerificationStarted, which already routes rejections into session.error, so awaiting it cannot double-throw. - verification-manager.ts (maybeAutoConfirmSas): pass { trustOwnDevice: true } so the auto-confirm path also cross-signs the bot device for self-verifications. - actions/verification.ts (confirmMatrixVerificationSas): mirror the trustOwnIdentityAfterSelfVerification call from completeMatrixSelfVerification when the returned summary indicates isSelfVerification. Tests: - verification-manager.test.ts: flipped the existing "auto-confirmed self-verification" assertion (now expects trustOwnDeviceAfterSas to be called); added two new tests for verifyPromise await and rejection-on-summary.error. - actions/verification.test.ts: two new tests asserting confirmMatrixVerificationSas calls trustOwnIdentityAfterSelfVerification on self-verifications and not on remote verifications. Verified end-to-end against matrix.thepolycule.ca (Synapse 1.145.0+ess.1, MAS-fronted): after `verify confirm-sas`, Element X's device-list view shows the bot device with a green shield and no pending Verify prompt. * fix(matrix): guard owner trust after failed SAS verification --------- Co-authored-by: Peter Steinberger --- extensions/matrix/CHANGELOG.md | 1 + .../src/matrix/actions/verification.test.ts | 72 ++++++++++++++++++ .../matrix/src/matrix/actions/verification.ts | 11 ++- .../matrix/sdk/verification-manager.test.ts | 73 ++++++++++++++++++- .../src/matrix/sdk/verification-manager.ts | 15 +++- 5 files changed, 168 insertions(+), 4 deletions(-) diff --git a/extensions/matrix/CHANGELOG.md b/extensions/matrix/CHANGELOG.md index 9276d92c199..7793ebf1044 100644 --- a/extensions/matrix/CHANGELOG.md +++ b/extensions/matrix/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixes +- Matrix/E2EE: close the owner-side device verification loop when SAS lands via the CLI. `verify confirm-sas` now (1) awaits the rust-crypto verifier promise so the done-exchange and any cross-signing uploads triggered by `crossSignDevice` settle before the verb returns, (2) cross-signs the bot device on the auto-confirmed inbound SAS path (previously skipped), and (3) calls `trustOwnIdentityAfterSelfVerification` from the standalone `confirmMatrixVerificationSas` action so the operator's Element X clears the "Verify" prompt without waiting for a passive sync tick [AI-assisted]. Thanks @nklock. - Matrix/E2EE: stabilize recovery and broken-device QA flows while avoiding device-cleanup sync races that could leave shutdown-time crypto work running. Thanks @gumadeiras. ## 2026.4.25 diff --git a/extensions/matrix/src/matrix/actions/verification.test.ts b/extensions/matrix/src/matrix/actions/verification.test.ts index d7b33a9de98..fbf5f6a2291 100644 --- a/extensions/matrix/src/matrix/actions/verification.test.ts +++ b/extensions/matrix/src/matrix/actions/verification.test.ts @@ -38,10 +38,12 @@ let getMatrixVerificationStatus: typeof import("./verification.js").getMatrixVer let restoreMatrixRoomKeyBackup: typeof import("./verification.js").restoreMatrixRoomKeyBackup; let runMatrixSelfVerification: typeof import("./verification.js").runMatrixSelfVerification; let startMatrixVerification: typeof import("./verification.js").startMatrixVerification; +let confirmMatrixVerificationSas: typeof import("./verification.js").confirmMatrixVerificationSas; describe("matrix verification actions", () => { beforeAll(async () => { ({ + confirmMatrixVerificationSas, getMatrixEncryptionStatus, getMatrixRoomKeyBackupStatus, getMatrixVerificationStatus, @@ -1001,4 +1003,74 @@ describe("matrix verification actions", () => { reason: "OpenClaw self-verification did not complete", }); }); + + it("confirmMatrixVerificationSas calls trustOwnIdentityAfterSelfVerification on a self-verification", async () => { + const crypto = { + confirmVerificationSas: vi.fn(async () => ({ + completed: true, + hasSas: true, + id: "verification-self", + isSelfVerification: true, + phaseName: "done", + transactionId: "tx-self", + })), + }; + const trustOwnIdentityAfterSelfVerification = vi.fn(async () => {}); + withStartedActionClientMock.mockImplementation(async (_opts, run) => { + return await run({ crypto, trustOwnIdentityAfterSelfVerification }); + }); + + const summary = await confirmMatrixVerificationSas("verification-self"); + + expect(crypto.confirmVerificationSas).toHaveBeenCalledWith("verification-self"); + expect(trustOwnIdentityAfterSelfVerification).toHaveBeenCalledTimes(1); + expect(summary.isSelfVerification).toBe(true); + }); + + it("confirmMatrixVerificationSas does not call trustOwnIdentityAfterSelfVerification on a non-self verification", async () => { + const crypto = { + confirmVerificationSas: vi.fn(async () => ({ + completed: true, + hasSas: true, + id: "verification-remote", + isSelfVerification: false, + phaseName: "done", + transactionId: "tx-remote", + })), + }; + const trustOwnIdentityAfterSelfVerification = vi.fn(async () => {}); + withStartedActionClientMock.mockImplementation(async (_opts, run) => { + return await run({ crypto, trustOwnIdentityAfterSelfVerification }); + }); + + const summary = await confirmMatrixVerificationSas("verification-remote"); + + expect(crypto.confirmVerificationSas).toHaveBeenCalledWith("verification-remote"); + expect(trustOwnIdentityAfterSelfVerification).not.toHaveBeenCalled(); + expect(summary.isSelfVerification).toBe(false); + }); + + it("confirmMatrixVerificationSas does not trust own identity when self-verification failed", async () => { + const crypto = { + confirmVerificationSas: vi.fn(async () => ({ + completed: false, + error: "verifier rejected mid-protocol", + hasSas: true, + id: "verification-self", + isSelfVerification: true, + phaseName: "started", + transactionId: "tx-self", + })), + }; + const trustOwnIdentityAfterSelfVerification = vi.fn(async () => {}); + withStartedActionClientMock.mockImplementation(async (_opts, run) => { + return await run({ crypto, trustOwnIdentityAfterSelfVerification }); + }); + + const summary = await confirmMatrixVerificationSas("verification-self"); + + expect(crypto.confirmVerificationSas).toHaveBeenCalledWith("verification-self"); + expect(trustOwnIdentityAfterSelfVerification).not.toHaveBeenCalled(); + expect(summary.error).toMatch(/verifier rejected mid-protocol/); + }); }); diff --git a/extensions/matrix/src/matrix/actions/verification.ts b/extensions/matrix/src/matrix/actions/verification.ts index c23425e2256..72bbe87050a 100644 --- a/extensions/matrix/src/matrix/actions/verification.ts +++ b/extensions/matrix/src/matrix/actions/verification.ts @@ -437,7 +437,16 @@ export async function confirmMatrixVerificationSas( return await withStartedActionClient(opts, async (client) => { const crypto = requireCrypto(client, opts); await ensureMatrixVerificationDmTracked(crypto, opts); - return await crypto.confirmVerificationSas(resolveVerificationId(requestId)); + const summary = await crypto.confirmVerificationSas(resolveVerificationId(requestId)); + // For self-verifications, mirror the trust-own-identity step that the + // higher-level runMatrixSelfVerification path already performs at + // completeMatrixSelfVerification: cross-sign the operator's master key + // from the bot side so Element X clears the "Verify" prompt without + // waiting for a passive sync tick. Non-self verifications are a no-op. + if (summary.isSelfVerification && summary.completed && !summary.error) { + await client.trustOwnIdentityAfterSelfVerification?.(); + } + return summary; }); } diff --git a/extensions/matrix/src/matrix/sdk/verification-manager.test.ts b/extensions/matrix/src/matrix/sdk/verification-manager.test.ts index 65b56777930..4c8dc321ecf 100644 --- a/extensions/matrix/src/matrix/sdk/verification-manager.test.ts +++ b/extensions/matrix/src/matrix/sdk/verification-manager.test.ts @@ -520,7 +520,7 @@ describe("MatrixVerificationManager", () => { } }); - it("does not cross-sign the other own device after auto-confirmed self-verification SAS", async () => { + it("cross-signs the other own device after auto-confirmed self-verification SAS", async () => { vi.useFakeTimers(); const { confirm, verifier } = createSasVerifierFixture({ decimal: [6158, 1986, 3513], @@ -541,12 +541,81 @@ describe("MatrixVerificationManager", () => { await vi.advanceTimersByTimeAsync(30_100); expect(confirm).toHaveBeenCalledTimes(1); - expect(trustOwnDeviceAfterSas).not.toHaveBeenCalled(); + expect(trustOwnDeviceAfterSas).toHaveBeenCalledWith("OTHERDEVICE"); } finally { vi.useRealTimers(); } }); + it("confirmVerificationSas awaits the verifier's verify promise before resolving", async () => { + let resolveVerify!: () => void; + const verifyPromise = new Promise((res) => { + resolveVerify = res; + }); + const verifyImpl = vi.fn(() => verifyPromise); + const { confirm, verifier } = createSasVerifierFixture({ + decimal: [111, 222, 333], + emoji: [["cat", "Cat"]], + verifyImpl, + }); + const trustOwnDeviceAfterSas = vi.fn(async () => {}); + const request = new MockVerificationRequest({ + isSelfVerification: true, + otherDeviceId: "OTHERDEVICE", + transactionId: "txn-await-verify", + initiatedByMe: true, + verifier, + }); + const manager = new MatrixVerificationManager({ trustOwnDeviceAfterSas }); + const tracked = manager.trackVerificationRequest(request); + + await manager.startVerification(tracked.id, "sas"); + expect(verifyImpl).toHaveBeenCalledTimes(1); + + let confirmResolved = false; + const confirmPromise = manager.confirmVerificationSas(tracked.id).then(() => { + confirmResolved = true; + }); + + // Yield once so confirmSasForSession + trustOwnDeviceAfterSas finish, but + // verifyPromise stays pending. confirmVerificationSas must still be + // blocked awaiting verifyPromise. + await Promise.resolve(); + await Promise.resolve(); + expect(confirm).toHaveBeenCalledTimes(1); + expect(trustOwnDeviceAfterSas).toHaveBeenCalledWith("OTHERDEVICE"); + expect(confirmResolved).toBe(false); + + resolveVerify(); + await confirmPromise; + expect(confirmResolved).toBe(true); + }); + + it("confirmVerificationSas surfaces a verifier-promise rejection on session.error", async () => { + const verifyImpl = vi.fn(async () => { + throw new Error("verifier rejected mid-protocol"); + }); + const { verifier } = createSasVerifierFixture({ + decimal: [111, 222, 333], + emoji: [["cat", "Cat"]], + verifyImpl, + }); + const request = new MockVerificationRequest({ + isSelfVerification: true, + otherDeviceId: "OTHERDEVICE", + transactionId: "txn-verify-rejects", + initiatedByMe: true, + verifier, + }); + const manager = new MatrixVerificationManager(); + const tracked = manager.trackVerificationRequest(request); + + await manager.startVerification(tracked.id, "sas"); + const summary = await manager.confirmVerificationSas(tracked.id); + + expect(summary.error).toMatch(/verifier rejected mid-protocol/); + }); + it("does not auto-confirm SAS for verifications initiated by this device", async () => { vi.useFakeTimers(); const confirm = vi.fn(async () => {}); diff --git a/extensions/matrix/src/matrix/sdk/verification-manager.ts b/extensions/matrix/src/matrix/sdk/verification-manager.ts index 08698a61cf8..cfdd9bc7f63 100644 --- a/extensions/matrix/src/matrix/sdk/verification-manager.ts +++ b/extensions/matrix/src/matrix/sdk/verification-manager.ts @@ -504,7 +504,11 @@ export class MatrixVerificationManager { return; } session.sasAutoConfirmStarted = true; - void this.confirmSasForSession(session, callbacks, { trustOwnDevice: false }) + // For self-verifications, trustOwnDeviceAfterConfirmedSas is gated on + // isSelfVerification, so non-self requests remain unaffected. Without + // this, the bot's own device never gets cross-signed when SAS lands + // via the auto-confirm timer (initiated remotely). + void this.confirmSasForSession(session, callbacks, { trustOwnDevice: true }) .then(() => { this.touchVerificationSession(session); }) @@ -732,6 +736,15 @@ export class MatrixVerificationManager { session.sasCallbacks = callbacks; session.sasAutoConfirmStarted = true; await this.confirmSasForSession(session, callbacks); + // Wait for the rust-crypto verifier to fully resolve (done-exchange + any + // pending cross-signing uploads triggered by trustOwnDeviceAfterSas) so + // the operator's client sees a settled state on the next /keys/query. + // verifyPromise is set inside ensureVerificationStarted and already + // funnels its own rejection into session.error, so awaiting it here + // cannot double-throw. + if (session.verifyPromise) { + await session.verifyPromise; + } this.touchVerificationSession(session); return this.buildVerificationSummary(session); }