From 9e050f49cc65483ae1488ca6b263bd1c19234634 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Thu, 23 Apr 2026 00:41:08 -0400 Subject: [PATCH] fix: harden Matrix recovery trust flow --- .../src/matrix/actions/verification.test.ts | 1 - .../matrix/src/matrix/actions/verification.ts | 1 - extensions/matrix/src/matrix/sdk.test.ts | 72 +++++++++++++++++++ extensions/matrix/src/matrix/sdk.ts | 26 ++++--- .../src/matrix/sdk/crypto-bootstrap.test.ts | 7 +- .../matrix/src/matrix/sdk/crypto-bootstrap.ts | 25 ------- .../contract/scenario-runtime-cli.test.ts | 20 +++++- .../runners/contract/scenario-runtime-cli.ts | 11 ++- .../src/runners/contract/scenarios.test.ts | 3 + 9 files changed, 124 insertions(+), 42 deletions(-) diff --git a/extensions/matrix/src/matrix/actions/verification.test.ts b/extensions/matrix/src/matrix/actions/verification.test.ts index 93fb0286655..63eeb7955c0 100644 --- a/extensions/matrix/src/matrix/actions/verification.test.ts +++ b/extensions/matrix/src/matrix/actions/verification.test.ts @@ -370,7 +370,6 @@ describe("matrix verification actions", () => { expect(crypto.confirmVerificationSas).toHaveBeenCalledWith("verification-1"); expect(bootstrapOwnDeviceVerification).toHaveBeenCalledWith({ allowAutomaticCrossSigningReset: false, - verifyOwnIdentity: true, }); expect(getOwnDeviceVerificationStatus).toHaveBeenCalled(); }); diff --git a/extensions/matrix/src/matrix/actions/verification.ts b/extensions/matrix/src/matrix/actions/verification.ts index 392614314cc..8c16875db35 100644 --- a/extensions/matrix/src/matrix/actions/verification.ts +++ b/extensions/matrix/src/matrix/actions/verification.ts @@ -210,7 +210,6 @@ async function completeMatrixSelfVerification(params: { }): Promise { const bootstrap = await params.client.bootstrapOwnDeviceVerification({ allowAutomaticCrossSigningReset: false, - verifyOwnIdentity: true, }); if (!bootstrap.verification.verified) { throw new Error( diff --git a/extensions/matrix/src/matrix/sdk.test.ts b/extensions/matrix/src/matrix/sdk.test.ts index 04f8ed94742..205f326194b 100644 --- a/extensions/matrix/src/matrix/sdk.test.ts +++ b/extensions/matrix/src/matrix/sdk.test.ts @@ -1779,6 +1779,78 @@ describe("MatrixClient crypto bootstrapping", () => { expect(persisted.encodedPrivateKey).toBe(previousEncoded); }); + it("does not persist a staged recovery key that secret storage did not validate", async () => { + const previousEncoded = encodeRecoveryKey( + new Uint8Array(Array.from({ length: 32 }, (_, i) => i + 5)), + ); + const attemptedEncoded = encodeRecoveryKey( + new Uint8Array(Array.from({ length: 32 }, (_, i) => i + 55)), + ); + + matrixJsClient.getUserId = vi.fn(() => "@bot:example.org"); + matrixJsClient.getDeviceId = vi.fn(() => "DEVICE123"); + matrixJsClient.getCrypto = vi.fn(() => ({ + on: vi.fn(), + bootstrapCrossSigning: vi.fn(async () => {}), + bootstrapSecretStorage: vi.fn(consumeMatrixSecretStorageKey), + requestOwnUserVerification: vi.fn(async () => null), + getSecretStorageStatus: vi.fn(async () => ({ + ready: true, + defaultKeyId: "SSSSKEY", + secretStorageKeyValidityMap: { SSSSKEY: false }, + })), + getDeviceVerificationStatus: vi.fn(async () => ({ + isVerified: () => true, + localVerified: true, + crossSigningVerified: false, + signedByOwner: false, + })), + checkKeyBackupAndEnable: vi.fn(async () => {}), + getActiveSessionBackupVersion: vi.fn(async () => "11"), + getSessionBackupPrivateKey: vi.fn(async () => new Uint8Array([1])), + getKeyBackupInfo: vi.fn(async () => ({ + algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", + auth_data: {}, + version: "11", + })), + isKeyBackupTrusted: vi.fn(async () => ({ + trusted: true, + matchesDecryptionKey: true, + })), + })); + + const recoveryDir = fs.mkdtempSync(path.join(os.tmpdir(), "matrix-sdk-verify-invalid-")); + const recoveryKeyPath = path.join(recoveryDir, "recovery-key.json"); + fs.writeFileSync( + recoveryKeyPath, + JSON.stringify({ + version: 1, + createdAt: new Date().toISOString(), + keyId: "SSSSKEY", + encodedPrivateKey: previousEncoded, + privateKeyBase64: Buffer.from( + new Uint8Array(Array.from({ length: 32 }, (_, i) => i + 5)), + ).toString("base64"), + }), + "utf8", + ); + + const client = new MatrixClient("https://matrix.example.org", "token", { + encryption: true, + recoveryKeyPath, + }); + + const result = await client.verifyWithRecoveryKey(attemptedEncoded as string); + + expect(result.success).toBe(false); + expect(result.recoveryKeyAccepted).toBe(false); + expect(result.backupUsable).toBe(true); + const persisted = JSON.parse(fs.readFileSync(recoveryKeyPath, "utf8")) as { + encodedPrivateKey?: string; + }; + expect(persisted.encodedPrivateKey).toBe(previousEncoded); + }); + it("fails recovery-key verification when backup remains untrusted after device verification", async () => { const encoded = encodeRecoveryKey(new Uint8Array(Array.from({ length: 32 }, (_, i) => i + 1))); diff --git a/extensions/matrix/src/matrix/sdk.ts b/extensions/matrix/src/matrix/sdk.ts index 571098bbbf3..db4769b5fcf 100644 --- a/extensions/matrix/src/matrix/sdk.ts +++ b/extensions/matrix/src/matrix/sdk.ts @@ -165,13 +165,11 @@ const MATRIX_AUTOMATIC_REPAIR_BOOTSTRAP_OPTIONS = { function createMatrixExplicitBootstrapOptions(params?: { allowAutomaticCrossSigningReset?: boolean; forceResetCrossSigning?: boolean; - verifyOwnIdentity?: boolean; }): MatrixCryptoBootstrapOptions { return { forceResetCrossSigning: params?.forceResetCrossSigning === true, allowAutomaticCrossSigningReset: params?.allowAutomaticCrossSigningReset !== false, allowSecretStorageRecreateWithoutRecoveryKey: true, - verifyOwnIdentity: params?.verifyOwnIdentity === true, strict: true, }; } @@ -1168,10 +1166,12 @@ export class MatrixClient { return await fail("Matrix recovery key is required"); } + let stagedKeyId: string | null = null; try { + stagedKeyId = (await this.resolveDefaultSecretStorageKeyId(crypto)) ?? null; this.recoveryKeyStore.stageEncodedRecoveryKey({ encodedPrivateKey: trimmedRecoveryKey, - keyId: await this.resolveDefaultSecretStorageKeyId(crypto), + keyId: stagedKeyId, }); } catch (err) { return await fail(formatMatrixErrorMessage(err)); @@ -1195,11 +1195,20 @@ export class MatrixClient { requireServerBackup: true, }) === null; const stagedRecoveryKeyUsed = this.recoveryKeyStore.hasStagedRecoveryKeyBeenUsed(); - const recoveryKeyAccepted = stagedRecoveryKeyUsed && (status.verified || backupUsable); + const secretStorageStatus = + typeof crypto.getSecretStorageStatus === "function" + ? await crypto.getSecretStorageStatus().catch(() => null) + : null; + const stagedRecoveryKeyValidated = Boolean( + stagedRecoveryKeyUsed && + stagedKeyId && + secretStorageStatus?.secretStorageKeyValidityMap?.[stagedKeyId] === true, + ); + const recoveryKeyAccepted = stagedRecoveryKeyValidated && (status.verified || backupUsable); if (!status.verified) { - if (backupUsable && stagedRecoveryKeyUsed) { + if (backupUsable && stagedRecoveryKeyValidated) { this.recoveryKeyStore.commitStagedRecoveryKey({ - keyId: await this.resolveDefaultSecretStorageKeyId(crypto), + keyId: stagedKeyId, }); } else { this.recoveryKeyStore.discardStagedRecoveryKey(); @@ -1228,7 +1237,7 @@ export class MatrixClient { ...status, }; } - if (!stagedRecoveryKeyUsed) { + if (!stagedRecoveryKeyValidated) { this.recoveryKeyStore.discardStagedRecoveryKey(); return { success: false, @@ -1242,7 +1251,7 @@ export class MatrixClient { } this.recoveryKeyStore.commitStagedRecoveryKey({ - keyId: await this.resolveDefaultSecretStorageKeyId(crypto), + keyId: stagedKeyId, }); const committedStatus = await this.getOwnDeviceVerificationStatus(); return { @@ -1477,7 +1486,6 @@ export class MatrixClient { allowAutomaticCrossSigningReset?: boolean; recoveryKey?: string; forceResetCrossSigning?: boolean; - verifyOwnIdentity?: boolean; }): Promise { const pendingVerifications = async (): Promise => this.crypto ? (await this.crypto.listVerifications()).length : 0; diff --git a/extensions/matrix/src/matrix/sdk/crypto-bootstrap.test.ts b/extensions/matrix/src/matrix/sdk/crypto-bootstrap.test.ts index 8197ba36dae..42639807c0d 100644 --- a/extensions/matrix/src/matrix/sdk/crypto-bootstrap.test.ts +++ b/extensions/matrix/src/matrix/sdk/crypto-bootstrap.test.ts @@ -253,7 +253,7 @@ describe("MatrixCryptoBootstrapper", () => { ); }); - it("can mark the own Matrix identity verified before cross-signing the current device", async () => { + it("does not mark the own Matrix identity verified before cross-signing the current device", async () => { const verifyOwnIdentity = vi.fn(async () => undefined); const freeOwnIdentity = vi.fn(); const setDeviceVerified = vi.fn(async () => {}); @@ -287,11 +287,10 @@ describe("MatrixCryptoBootstrapper", () => { await bootstrapper.bootstrap(crypto, { allowAutomaticCrossSigningReset: false, - verifyOwnIdentity: true, }); - expect(verifyOwnIdentity).toHaveBeenCalledTimes(1); - expect(freeOwnIdentity).toHaveBeenCalledTimes(1); + expect(verifyOwnIdentity).not.toHaveBeenCalled(); + expect(freeOwnIdentity).not.toHaveBeenCalled(); expect(setDeviceVerified).toHaveBeenCalledWith("@bot:example.org", "DEVICE123", true); expect(crossSignDevice).toHaveBeenCalledWith("DEVICE123"); }); diff --git a/extensions/matrix/src/matrix/sdk/crypto-bootstrap.ts b/extensions/matrix/src/matrix/sdk/crypto-bootstrap.ts index 20e1f6dd622..1c040e1ce29 100644 --- a/extensions/matrix/src/matrix/sdk/crypto-bootstrap.ts +++ b/extensions/matrix/src/matrix/sdk/crypto-bootstrap.ts @@ -28,7 +28,6 @@ export type MatrixCryptoBootstrapOptions = { forceResetCrossSigning?: boolean; allowAutomaticCrossSigningReset?: boolean; allowSecretStorageRecreateWithoutRecoveryKey?: boolean; - verifyOwnIdentity?: boolean; strict?: boolean; }; @@ -86,7 +85,6 @@ export class MatrixCryptoBootstrapper { } const ownDeviceVerified = await this.ensureOwnDeviceTrust(crypto, { strict, - verifyOwnIdentity: options.verifyOwnIdentity === true, }); return { crossSigningReady: crossSigning.ready, @@ -351,29 +349,10 @@ export class MatrixCryptoBootstrapper { LogService.info("MatrixClientLite", "Verification request handler registered"); } - private async verifyOwnIdentityTrust(crypto: MatrixCryptoBootstrapApi): Promise { - if (typeof crypto.getOwnIdentity !== "function") { - return; - } - const identity = await crypto.getOwnIdentity(); - if (!identity) { - return; - } - try { - if (identity.isVerified?.() === true) { - return; - } - await identity.verify?.(); - } finally { - identity.free?.(); - } - } - private async ensureOwnDeviceTrust( crypto: MatrixCryptoBootstrapApi, options: { strict: boolean; - verifyOwnIdentity: boolean; }, ): Promise { const deviceId = this.deps.getDeviceId()?.trim(); @@ -392,10 +371,6 @@ export class MatrixCryptoBootstrapper { return true; } - if (options.verifyOwnIdentity) { - await this.verifyOwnIdentityTrust(crypto); - } - if (typeof crypto.setDeviceVerified === "function") { await crypto.setDeviceVerified(userId, deviceId, true); } diff --git a/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.test.ts b/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.test.ts index b7ddbcb061b..ae104b6eab8 100644 --- a/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.test.ts +++ b/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.test.ts @@ -1,5 +1,12 @@ +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; import { describe, expect, it } from "vitest"; -import { formatMatrixQaCliCommand, redactMatrixQaCliOutput } from "./scenario-runtime-cli.js"; +import { + formatMatrixQaCliCommand, + redactMatrixQaCliOutput, + resolveMatrixQaOpenClawCliEntryPath, +} from "./scenario-runtime-cli.js"; describe("Matrix QA CLI runtime", () => { it("redacts secret CLI arguments in diagnostic command text", () => { @@ -23,4 +30,15 @@ describe("Matrix QA CLI runtime", () => { redactMatrixQaCliOutput("GET /_matrix/client/v3/sync?access_token=abcdef1234567890ghij"), ).toBe("GET /_matrix/client/v3/sync?access_token=abcdef…ghij"); }); + + it("prefers the ESM OpenClaw CLI entrypoint when present", async () => { + const root = await mkdtemp(path.join(tmpdir(), "matrix-qa-cli-entry-")); + try { + await mkdir(path.join(root, "dist")); + await writeFile(path.join(root, "dist", "index.mjs"), ""); + expect(resolveMatrixQaOpenClawCliEntryPath(root)).toBe(path.join(root, "dist", "index.mjs")); + } finally { + await rm(root, { force: true, recursive: true }); + } + }); }); diff --git a/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.ts b/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.ts index 460453ddc79..d6eb7edc902 100644 --- a/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.ts +++ b/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.ts @@ -1,4 +1,5 @@ import { spawn as startOpenClawCliProcess } from "node:child_process"; +import { existsSync } from "node:fs"; import path from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime"; @@ -48,6 +49,14 @@ export function formatMatrixQaCliCommand(args: string[]) { return `openclaw ${redactMatrixQaCliArgs(args).join(" ")}`; } +export function resolveMatrixQaOpenClawCliEntryPath(cwd: string): string { + const mjsEntryPath = path.join(cwd, "dist", "index.mjs"); + if (existsSync(mjsEntryPath)) { + return mjsEntryPath; + } + return path.join(cwd, "dist", "index.js"); +} + function buildMatrixQaCliResult(params: { args: string[]; exitCode: number; @@ -78,7 +87,7 @@ export function startMatrixQaOpenClawCli(params: { timeoutMs: number; }): MatrixQaCliSession { const cwd = params.cwd ?? process.cwd(); - const distEntryPath = path.join(cwd, "dist", "index.js"); + const distEntryPath = resolveMatrixQaOpenClawCliEntryPath(cwd); const stdout: Buffer[] = []; const stderr: Buffer[] = []; let closed = false; diff --git a/extensions/qa-matrix/src/runners/contract/scenarios.test.ts b/extensions/qa-matrix/src/runners/contract/scenarios.test.ts index 570aa0d6d21..f3a8c93b2fa 100644 --- a/extensions/qa-matrix/src/runners/contract/scenarios.test.ts +++ b/extensions/qa-matrix/src/runners/contract/scenarios.test.ts @@ -14,11 +14,13 @@ const { createMatrixQaE2eeScenarioClient, runMatrixQaE2eeBootstrap, startMatrixQ const { formatMatrixQaCliCommand, redactMatrixQaCliOutput, + resolveMatrixQaOpenClawCliEntryPath, runMatrixQaOpenClawCli, startMatrixQaOpenClawCli, } = vi.hoisted(() => ({ formatMatrixQaCliCommand: (args: string[]) => `openclaw ${args.join(" ")}`, redactMatrixQaCliOutput: (text: string) => text, + resolveMatrixQaOpenClawCliEntryPath: (cwd: string) => `${cwd}/dist/index.js`, runMatrixQaOpenClawCli: vi.fn(), startMatrixQaOpenClawCli: vi.fn(), })); @@ -36,6 +38,7 @@ vi.mock("../../substrate/fault-proxy.js", () => ({ vi.mock("./scenario-runtime-cli.js", () => ({ formatMatrixQaCliCommand, redactMatrixQaCliOutput, + resolveMatrixQaOpenClawCliEntryPath, runMatrixQaOpenClawCli, startMatrixQaOpenClawCli, }));