mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 11:10:45 +00:00
fix: harden Matrix verification diagnostics
This commit is contained in:
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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) => {
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
}));
|
||||
|
||||
@@ -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, {
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user