diff --git a/extensions/matrix/src/matrix/sdk/verification-manager.test.ts b/extensions/matrix/src/matrix/sdk/verification-manager.test.ts index d9313cf1286..072a1035cb0 100644 --- a/extensions/matrix/src/matrix/sdk/verification-manager.test.ts +++ b/extensions/matrix/src/matrix/sdk/verification-manager.test.ts @@ -188,6 +188,34 @@ describe("MatrixVerificationManager", () => { expect(secondSummary.chosenMethod).toBe("m.sas.v1"); }); + it("does not overwrite a different verification request with a colliding transaction ID", async () => { + const manager = new MatrixVerificationManager(); + const first = new MockVerificationRequest({ + transactionId: "txn-collision", + initiatedByMe: true, + otherUserId: "@alice:example.org", + otherDeviceId: "ALICE1", + }); + const second = new MockVerificationRequest({ + transactionId: "txn-collision", + initiatedByMe: true, + otherUserId: "@mallory:example.org", + otherDeviceId: "MALLORY1", + }); + + const firstSummary = manager.trackVerificationRequest(first); + const secondSummary = manager.trackVerificationRequest(second); + + expect(secondSummary.id).not.toBe(firstSummary.id); + expect(manager.listVerifications()).toHaveLength(2); + expect(() => manager.getVerificationSas("txn-collision")).toThrow( + "Matrix verification request id is ambiguous for transaction txn-collision", + ); + await manager.acceptVerification(firstSummary.id); + expect(first.accept).toHaveBeenCalledTimes(1); + expect(second.accept).not.toHaveBeenCalled(); + }); + it("starts SAS verification and exposes SAS payload/callback flow", async () => { const confirm = vi.fn(async () => {}); const mismatch = vi.fn(); diff --git a/extensions/matrix/src/matrix/sdk/verification-manager.ts b/extensions/matrix/src/matrix/sdk/verification-manager.ts index 2ffd071096c..29e78d3c6c8 100644 --- a/extensions/matrix/src/matrix/sdk/verification-manager.ts +++ b/extensions/matrix/src/matrix/sdk/verification-manager.ts @@ -133,6 +133,15 @@ type MatrixVerificationSession = { reciprocateQrCallbacks?: MatrixShowQrCodeCallbacks; }; +type MatrixVerificationRequestIdentity = { + transactionId: string; + roomId: string; + otherUserId: string; + otherDeviceId: string; + isSelfVerification: boolean; + initiatedByMe: boolean; +}; + const MAX_TRACKED_VERIFICATION_SESSIONS = 256; const TERMINAL_SESSION_RETENTION_MS = 24 * 60 * 60 * 1000; const SAS_AUTO_CONFIRM_DELAY_MS = 30_000; @@ -164,6 +173,36 @@ export class MatrixVerificationManager { return isMatrixVerificationPhase(phase) ? phase : fallback; } + private readVerificationRequestIdentity( + request: MatrixVerificationRequestLike, + ): MatrixVerificationRequestIdentity { + return { + transactionId: this.readRequestValue(request, () => request.transactionId?.trim() ?? "", ""), + roomId: this.readRequestValue(request, () => request.roomId ?? "", ""), + otherUserId: this.readRequestValue(request, () => request.otherUserId, ""), + otherDeviceId: this.readRequestValue(request, () => request.otherDeviceId ?? "", ""), + isSelfVerification: this.readRequestValue(request, () => request.isSelfVerification, false), + initiatedByMe: this.readRequestValue(request, () => request.initiatedByMe, false), + }; + } + + private isSameLogicalVerificationRequest( + left: MatrixVerificationRequestLike, + right: MatrixVerificationRequestLike, + ): boolean { + const leftIdentity = this.readVerificationRequestIdentity(left); + const rightIdentity = this.readVerificationRequestIdentity(right); + return ( + leftIdentity.transactionId !== "" && + leftIdentity.transactionId === rightIdentity.transactionId && + leftIdentity.roomId === rightIdentity.roomId && + leftIdentity.otherUserId === rightIdentity.otherUserId && + leftIdentity.otherDeviceId === rightIdentity.otherDeviceId && + leftIdentity.isSelfVerification === rightIdentity.isSelfVerification && + leftIdentity.initiatedByMe === rightIdentity.initiatedByMe + ); + } + private pruneVerificationSessions(nowMs: number): void { for (const [id, session] of this.verificationSessions) { const phase = this.readVerificationPhase(session.request, -1); @@ -277,11 +316,21 @@ export class MatrixVerificationManager { if (direct) { return direct; } - for (const session of this.verificationSessions.values()) { - const txId = this.readRequestValue(session.request, () => session.request.transactionId, ""); - if (txId === id) { - return session; - } + const transactionMatches = Array.from(this.verificationSessions.values()).filter((session) => { + const txId = this.readRequestValue( + session.request, + () => session.request.transactionId?.trim(), + "", + ); + return txId === id; + }); + if (transactionMatches.length === 1) { + return transactionMatches[0]; + } + if (transactionMatches.length > 1) { + throw new Error( + `Matrix verification request id is ambiguous for transaction ${id}; use the verification id instead`, + ); } throw new Error(`Matrix verification request not found: ${id}`); } @@ -482,15 +531,17 @@ export class MatrixVerificationManager { trackVerificationRequest(request: MatrixVerificationRequestLike): MatrixVerificationSummary { this.pruneVerificationSessions(Date.now()); - const txId = this.readRequestValue(request, () => request.transactionId?.trim(), ""); + const requestObj = request as unknown as object; + for (const existing of this.verificationSessions.values()) { + if ((existing.request as unknown as object) === requestObj) { + this.touchVerificationSession(existing); + return this.buildVerificationSummary(existing); + } + } + const txId = this.readVerificationRequestIdentity(request).transactionId; if (txId) { for (const existing of this.verificationSessions.values()) { - const existingTxId = this.readRequestValue( - existing.request, - () => existing.request.transactionId, - "", - ); - if (existingTxId === txId) { + if (this.isSameLogicalVerificationRequest(existing.request, request)) { existing.request = request; this.ensureVerificationRequestTracked(existing); const verifier = this.readRequestValue(request, () => request.verifier, null); 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 new file mode 100644 index 00000000000..b7ddbcb061b --- /dev/null +++ b/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from "vitest"; +import { formatMatrixQaCliCommand, redactMatrixQaCliOutput } from "./scenario-runtime-cli.js"; + +describe("Matrix QA CLI runtime", () => { + it("redacts secret CLI arguments in diagnostic command text", () => { + expect( + formatMatrixQaCliCommand([ + "matrix", + "verify", + "backup", + "restore", + "--recovery-key", + "abcdef1234567890ghij", + ]), + ).toBe("openclaw matrix verify backup restore --recovery-key [REDACTED]"); + expect(formatMatrixQaCliCommand(["matrix", "account", "add", "--access-token=token-123"])).toBe( + "openclaw matrix account add --access-token=[REDACTED]", + ); + }); + + it("redacts Matrix token output before diagnostics and artifacts", () => { + expect( + redactMatrixQaCliOutput("GET /_matrix/client/v3/sync?access_token=abcdef1234567890ghij"), + ).toBe("GET /_matrix/client/v3/sync?access_token=abcdef…ghij"); + }); +}); 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 f359737931f..460453ddc79 100644 --- a/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.ts +++ b/extensions/qa-matrix/src/runners/contract/scenario-runtime-cli.ts @@ -2,6 +2,7 @@ import { spawn as startOpenClawCliProcess } from "node:child_process"; import path from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime"; +import { redactSensitiveText } from "openclaw/plugin-sdk/logging-core"; export type MatrixQaCliRunResult = { args: string[]; @@ -23,8 +24,28 @@ export type MatrixQaCliSession = { kill: () => void; }; -function formatMatrixQaCliCommand(args: string[]) { - return `openclaw ${args.join(" ")}`; +const MATRIX_QA_CLI_SECRET_ARG_FLAGS = new Set(["--access-token", "--password", "--recovery-key"]); + +function redactMatrixQaCliArgs(args: string[]): string[] { + return args.map((arg, index) => { + const [flag] = arg.split("=", 1); + if (MATRIX_QA_CLI_SECRET_ARG_FLAGS.has(flag) && arg.includes("=")) { + return `${flag}=[REDACTED]`; + } + const previous = args[index - 1]; + if (previous && MATRIX_QA_CLI_SECRET_ARG_FLAGS.has(previous)) { + return "[REDACTED]"; + } + return arg; + }); +} + +export function redactMatrixQaCliOutput(text: string): string { + return redactSensitiveText(text); +} + +export function formatMatrixQaCliCommand(args: string[]) { + return `openclaw ${redactMatrixQaCliArgs(args).join(" ")}`; } function buildMatrixQaCliResult(params: { @@ -43,8 +64,8 @@ function buildMatrixQaCliResult(params: { function formatMatrixQaCliExitError(result: MatrixQaCliRunResult) { return [ `${formatMatrixQaCliCommand(result.args)} exited ${result.exitCode}`, - result.stderr.trim() ? `stderr:\n${result.stderr.trim()}` : null, - result.stdout.trim() ? `stdout:\n${result.stdout.trim()}` : null, + result.stderr.trim() ? `stderr:\n${redactMatrixQaCliOutput(result.stderr.trim())}` : null, + result.stdout.trim() ? `stdout:\n${redactMatrixQaCliOutput(result.stdout.trim())}` : null, ] .filter(Boolean) .join("\n"); @@ -150,7 +171,7 @@ export function startMatrixQaOpenClawCli(params: { settleWait = { reject, resolve }; }).catch((error) => { throw new Error( - `Matrix QA CLI command failed (${params.args.join(" ")}): ${formatErrorMessage(error)}`, + `Matrix QA CLI command failed (${formatMatrixQaCliCommand(params.args)}): ${redactMatrixQaCliOutput(formatErrorMessage(error))}`, ); }), waitForOutput: async (predicate, label, timeoutMs) => { @@ -168,7 +189,7 @@ export function startMatrixQaOpenClawCli(params: { } const output = readOutput(); throw new Error( - `openclaw ${params.args.join(" ")} did not print ${label} before timeout\nstdout:\n${output.stdout.trim()}\nstderr:\n${output.stderr.trim()}`, + `${formatMatrixQaCliCommand(params.args)} did not print ${label} before timeout\nstdout:\n${redactMatrixQaCliOutput(output.stdout.trim())}\nstderr:\n${redactMatrixQaCliOutput(output.stderr.trim())}`, ); }, writeStdin: async (text) => { diff --git a/extensions/qa-matrix/src/runners/contract/scenario-runtime-e2ee.ts b/extensions/qa-matrix/src/runners/contract/scenario-runtime-e2ee.ts index 46450f38853..ce552fc9c88 100644 --- a/extensions/qa-matrix/src/runners/contract/scenario-runtime-e2ee.ts +++ b/extensions/qa-matrix/src/runners/contract/scenario-runtime-e2ee.ts @@ -29,6 +29,8 @@ import { MATRIX_QA_IMAGE_ATTACHMENT_FILENAME, } from "./scenario-media-fixtures.js"; import { + formatMatrixQaCliCommand, + redactMatrixQaCliOutput, runMatrixQaOpenClawCli, startMatrixQaOpenClawCli, type MatrixQaCliRunResult, @@ -245,7 +247,7 @@ function parseMatrixQaCliJson(result: MatrixQaCliRunResult): unknown { const stderr = result.stderr.trim(); if (stdout && stderr) { throw new Error( - `openclaw ${result.args.join(" ")} printed JSON with extra output\nstdout:\n${stdout}\nstderr:\n${stderr}`, + `${formatMatrixQaCliCommand(result.args)} printed JSON with extra output\nstdout:\n${redactMatrixQaCliOutput(stdout)}\nstderr:\n${redactMatrixQaCliOutput(stderr)}`, ); } if (stdout) { @@ -253,24 +255,24 @@ function parseMatrixQaCliJson(result: MatrixQaCliRunResult): unknown { return parseMatrixQaCliJsonText(stdout); } catch (error) { throw new Error( - `openclaw ${result.args.join(" ")} printed invalid JSON: ${ + `${formatMatrixQaCliCommand(result.args)} printed invalid JSON: ${ error instanceof Error ? error.message : String(error) - }\nstdout:\n${stdout}`, + }\nstdout:\n${redactMatrixQaCliOutput(stdout)}`, { cause: error }, ); } } if (!stderr) { - throw new Error(`openclaw ${result.args.join(" ")} did not print JSON`); + throw new Error(`${formatMatrixQaCliCommand(result.args)} did not print JSON`); } try { return parseMatrixQaCliJsonText(stderr); } catch (error) { throw new Error( - `openclaw ${result.args.join(" ")} printed invalid JSON: ${ + `${formatMatrixQaCliCommand(result.args)} printed invalid JSON: ${ error instanceof Error ? error.message : String(error) - }\nstderr:\n${stderr}`, + }\nstderr:\n${redactMatrixQaCliOutput(stderr)}`, { cause: error }, ); } @@ -307,8 +309,8 @@ async function writeMatrixQaCliOutputArtifacts(params: { const stdoutPath = path.join(params.rootDir, `${prefix}.stdout.txt`); const stderrPath = path.join(params.rootDir, `${prefix}.stderr.txt`); await Promise.all([ - writeFile(stdoutPath, params.result.stdout, { mode: 0o600 }), - writeFile(stderrPath, params.result.stderr, { mode: 0o600 }), + writeFile(stdoutPath, redactMatrixQaCliOutput(params.result.stdout), { mode: 0o600 }), + writeFile(stderrPath, redactMatrixQaCliOutput(params.result.stderr), { mode: 0o600 }), ]); return { stderrPath, stdoutPath }; } diff --git a/extensions/qa-matrix/src/runners/contract/scenarios.test.ts b/extensions/qa-matrix/src/runners/contract/scenarios.test.ts index 7de634319df..570aa0d6d21 100644 --- a/extensions/qa-matrix/src/runners/contract/scenarios.test.ts +++ b/extensions/qa-matrix/src/runners/contract/scenarios.test.ts @@ -11,7 +11,14 @@ const { createMatrixQaE2eeScenarioClient, runMatrixQaE2eeBootstrap, startMatrixQ runMatrixQaE2eeBootstrap: vi.fn(), startMatrixQaFaultProxy: vi.fn(), })); -const { runMatrixQaOpenClawCli, startMatrixQaOpenClawCli } = vi.hoisted(() => ({ +const { + formatMatrixQaCliCommand, + redactMatrixQaCliOutput, + runMatrixQaOpenClawCli, + startMatrixQaOpenClawCli, +} = vi.hoisted(() => ({ + formatMatrixQaCliCommand: (args: string[]) => `openclaw ${args.join(" ")}`, + redactMatrixQaCliOutput: (text: string) => text, runMatrixQaOpenClawCli: vi.fn(), startMatrixQaOpenClawCli: vi.fn(), })); @@ -27,6 +34,8 @@ vi.mock("../../substrate/fault-proxy.js", () => ({ startMatrixQaFaultProxy, })); vi.mock("./scenario-runtime-cli.js", () => ({ + formatMatrixQaCliCommand, + redactMatrixQaCliOutput, runMatrixQaOpenClawCli, startMatrixQaOpenClawCli, })); diff --git a/src/logging/redact.test.ts b/src/logging/redact.test.ts index 3e3d0449850..dd24e150d44 100644 --- a/src/logging/redact.test.ts +++ b/src/logging/redact.test.ts @@ -45,6 +45,17 @@ describe("redactSensitiveText", () => { expect(output).toBe('{"token":"abcdef…ghij"}'); }); + it("masks Matrix access token fields and query parameters", () => { + const json = '{"access_token":"abcdef1234567890ghij"}'; + const url = "https://matrix.example/_matrix/client/v3/sync?access_token=zyxwv9876543210token"; + expect(redactSensitiveText(json, { mode: "tools", patterns: defaults })).toBe( + '{"access_token":"abcdef…ghij"}', + ); + expect(redactSensitiveText(url, { mode: "tools", patterns: defaults })).toBe( + "https://matrix.example/_matrix/client/v3/sync?access_token=zyxwv9…oken", + ); + }); + it("masks bearer tokens", () => { const input = "Authorization: Bearer abcdef1234567890ghij"; const output = redactSensitiveText(input, { diff --git a/src/logging/redact.ts b/src/logging/redact.ts index 2be750805e2..3689fb6a4a4 100644 --- a/src/logging/redact.ts +++ b/src/logging/redact.ts @@ -17,7 +17,7 @@ const DEFAULT_REDACT_PATTERNS: string[] = [ // ENV-style assignments. String.raw`\b[A-Z0-9_]*(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD)\b\s*[=:]\s*(["']?)([^\s"'\\]+)\1`, // JSON fields. - String.raw`"(?:apiKey|token|secret|password|passwd|accessToken|refreshToken)"\s*:\s*"([^"]+)"`, + String.raw`"(?:apiKey|token|secret|password|passwd|accessToken|refreshToken|access_token|refresh_token)"\s*:\s*"([^"]+)"`, // CLI flags. String.raw`--(?:api[-_]?key|hook[-_]?token|token|secret|password|passwd)\s+(["']?)([^\s"']+)\1`, // Authorization headers.