diff --git a/docs/tools/slash-commands.md b/docs/tools/slash-commands.md index 7fafda3d27d1..5fb774436bf7 100644 --- a/docs/tools/slash-commands.md +++ b/docs/tools/slash-commands.md @@ -402,6 +402,11 @@ updates persist across restarts. ``` `/mcp` stores config in OpenClaw config, not embedded-agent project settings. +`/mcp show` redacts credential-bearing fields, recognized credential flag +values, and known secret-shaped arguments. When run from a group, the +configuration is sent to the owner privately; if no private owner route is +available, the command fails closed and asks the owner to retry from a direct +chat. ## `/debug`: runtime-only overrides diff --git a/src/auto-reply/reply/commands-mcp.test.ts b/src/auto-reply/reply/commands-mcp.test.ts index 5c333935d8a3..d44c93a60112 100644 --- a/src/auto-reply/reply/commands-mcp.test.ts +++ b/src/auto-reply/reply/commands-mcp.test.ts @@ -4,7 +4,7 @@ import type { OpenClawConfig } from "../../config/config.js"; import { withTempHome } from "../../config/home-env.test-harness.js"; import { REDACTED_SENTINEL } from "../../config/redact-snapshot.js"; import { createCommandWorkspaceHarness } from "./commands-filesystem.test-support.js"; -import { handleMcpCommand } from "./commands-mcp.js"; +import { createMcpCommandHandler, handleMcpCommand } from "./commands-mcp.js"; import { buildCommandTestParams } from "./commands.test-harness.js"; const mcpServers = vi.hoisted(() => new Map>()); @@ -164,14 +164,44 @@ describe("handleCommands /mcp", () => { }); }); - it("redacts credential-bearing headers and env from /mcp show in groups", async () => { + it("routes group /mcp show privately and redacts the delivered config", async () => { await withTempHome("openclaw-command-mcp-home-", async () => { const workspaceDir = await workspaceHarness.createWorkspace(); + const privateReplies: string[] = []; + const groupHandler = createMcpCommandHandler({ + resolvePrivateMcpTargets: async () => [{ channel: "telegram", to: "owner-1" }], + deliverPrivateMcpReply: async ({ reply }) => { + privateReplies.push(reply.text ?? ""); + return true; + }, + }); const headerSecret = "Bearer sk-test-secret-value"; const envSecret = "stdio-process-token-value"; + const separateArgSecret = "plain-separate-arg-secret"; + const inlineArgSecret = "plain-inline-arg-secret"; + const positionalArgSecret = "ghp_realgithubtoken1234567890ABCD"; + const secretKeyArg = "opaque-secret-key-value"; + const awsSecretAccessKeyArg = "opaque-aws-secret-access-key-value"; + const underscoreApiKeyArg = "opaque-underscore-api-key-value"; + const pluralCredentialsArg = "opaque-plural-credentials-value"; mcpServers.set("billing-server", { command: "uvx", - args: ["billing-mcp"], + args: [ + "billing-mcp", + "--api-key", + separateArgSecret, + `--token=${inlineArgSecret}`, + positionalArgSecret, + "--secret-key", + secretKeyArg, + `--aws-secret-access-key=${awsSecretAccessKeyArg}`, + "--openai_api_key", + underscoreApiKeyArg, + "--credentials", + pluralCredentialsArg, + "--region", + "us-east-1", + ], transport: "streamable-http", url: "https://billing.example.com/mcp", headers: { @@ -199,13 +229,34 @@ describe("handleCommands /mcp", () => { ); namedParams.command.senderIsOwner = true; namedParams.isGroup = true; - const namedResult = expectMcpResult(await handleMcpCommand(namedParams, true)); - const namedText = namedResult.reply?.text ?? ""; + const namedResult = expectMcpResult(await groupHandler(namedParams, true)); + const namedGroupText = namedResult.reply?.text ?? ""; + expect(namedGroupText).toContain("sent the details to the owner privately"); + expect(namedGroupText).not.toContain("billing-server"); + expect(namedGroupText).not.toContain("/tmp/openclaw.json"); + expect(namedGroupText).not.toContain(headerSecret); + expect(privateReplies).toHaveLength(1); + const namedText = privateReplies[0] ?? ""; expect(namedText).toContain('MCP server "billing-server"'); expect(namedText).toContain('"command": "uvx"'); + expect(namedText).toContain('"billing-mcp"'); + expect(namedText).toContain('"--api-key"'); + expect(namedText).toContain(`"--token=${REDACTED_SENTINEL}"`); + expect(namedText).toContain('"--secret-key"'); + expect(namedText).toContain(`"--aws-secret-access-key=${REDACTED_SENTINEL}"`); + expect(namedText).toContain('"--openai_api_key"'); + expect(namedText).toContain('"--region"'); + expect(namedText).toContain('"us-east-1"'); expect(namedText).toContain(REDACTED_SENTINEL); expect(namedText).not.toContain(headerSecret); expect(namedText).not.toContain(envSecret); + expect(namedText).not.toContain(separateArgSecret); + expect(namedText).not.toContain(inlineArgSecret); + expect(namedText).not.toContain(positionalArgSecret); + expect(namedText).not.toContain(secretKeyArg); + expect(namedText).not.toContain(awsSecretAccessKeyArg); + expect(namedText).not.toContain(underscoreApiKeyArg); + expect(namedText).not.toContain(pluralCredentialsArg); expect(namedText).not.toContain("sk-test-secret-value"); const allParams = buildCommandTestParams("/mcp show", buildCfg(), undefined, { @@ -213,14 +264,95 @@ describe("handleCommands /mcp", () => { }); allParams.command.senderIsOwner = true; allParams.isGroup = true; - const allResult = expectMcpResult(await handleMcpCommand(allParams, true)); - const allText = allResult.reply?.text ?? ""; + const allResult = expectMcpResult(await groupHandler(allParams, true)); + const allGroupText = allResult.reply?.text ?? ""; + expect(allGroupText).toContain("sent the details to the owner privately"); + expect(allGroupText).not.toContain("billing-server"); + expect(allGroupText).not.toContain("/tmp/openclaw.json"); + expect(privateReplies).toHaveLength(2); + const allText = privateReplies[1] ?? ""; expect(allText).toContain('"billing-server"'); expect(allText).toContain('"local-tools"'); expect(allText).toContain(REDACTED_SENTINEL); expect(allText).not.toContain(headerSecret); expect(allText).not.toContain(envSecret); + expect(allText).not.toContain(separateArgSecret); + expect(allText).not.toContain(inlineArgSecret); + expect(allText).not.toContain(positionalArgSecret); + expect(allText).not.toContain(secretKeyArg); + expect(allText).not.toContain(awsSecretAccessKeyArg); + expect(allText).not.toContain(underscoreApiKeyArg); + expect(allText).not.toContain(pluralCredentialsArg); expect(allText).not.toContain("local-env-secret-value"); }); }); + + it.each([ + { + name: "no private owner target", + resolvePrivateMcpTargets: async () => [], + deliverPrivateMcpReply: async () => true, + }, + { + name: "private delivery failure", + resolvePrivateMcpTargets: async () => [{ channel: "telegram", to: "owner-1" }], + deliverPrivateMcpReply: async () => false, + }, + ])("fails closed for group /mcp show with $name", async (route) => { + await withTempHome("openclaw-command-mcp-home-", async () => { + const workspaceDir = await workspaceHarness.createWorkspace(); + const secret = "group-route-secret-value"; + mcpServers.set("billing-server", { + command: "uvx", + args: ["billing-mcp", "--api-key", secret], + }); + const handler = createMcpCommandHandler(route); + const params = buildCommandTestParams("/mcp show billing-server", buildCfg(), undefined, { + workspaceDir, + }); + params.command.senderIsOwner = true; + params.isGroup = true; + + const result = expectMcpResult(await handler(params, true)); + const groupText = result.reply?.text ?? ""; + expect(groupText).toContain("Run /mcp show from an owner DM"); + expect(groupText).not.toContain("billing-server"); + expect(groupText).not.toContain("/tmp/openclaw.json"); + expect(groupText).not.toContain(secret); + }); + }); + + it("tries later private owner routes without exposing config to the group", async () => { + await withTempHome("openclaw-command-mcp-home-", async () => { + const workspaceDir = await workspaceHarness.createWorkspace(); + const attemptedTargets: string[] = []; + mcpServers.set("billing-server", { + command: "uvx", + args: ["billing-mcp", "--api-key", "private-route-secret"], + }); + const handler = createMcpCommandHandler({ + resolvePrivateMcpTargets: async () => [ + { channel: "telegram", to: "stale-owner-route" }, + { channel: "signal", to: "working-owner-route" }, + ], + deliverPrivateMcpReply: async ({ targets }) => { + const target = targets[0]?.to ?? ""; + attemptedTargets.push(target); + return target === "working-owner-route"; + }, + }); + const params = buildCommandTestParams("/mcp show billing-server", buildCfg(), undefined, { + workspaceDir, + }); + params.command.senderIsOwner = true; + params.isGroup = true; + + const result = expectMcpResult(await handler(params, true)); + const groupText = result.reply?.text ?? ""; + expect(attemptedTargets).toEqual(["stale-owner-route", "working-owner-route"]); + expect(groupText).toContain("sent the details to the owner privately"); + expect(groupText).not.toContain("billing-server"); + expect(groupText).not.toContain("private-route-secret"); + }); + }); }); diff --git a/src/auto-reply/reply/commands-mcp.ts b/src/auto-reply/reply/commands-mcp.ts index 3860c93ac764..ea6781d2ac5f 100644 --- a/src/auto-reply/reply/commands-mcp.ts +++ b/src/auto-reply/reply/commands-mcp.ts @@ -1,152 +1,270 @@ /** Handles /mcp commands for showing and mutating configured MCP servers. */ +import { resolveSessionAgentId } from "../../agents/agent-scope.js"; import { listConfiguredMcpServers, setConfiguredMcpServer, unsetConfiguredMcpServer, } from "../../config/mcp-config.js"; -import { redactConfigObject } from "../../config/redact-snapshot.js"; +import { redactSensitiveArgv } from "../../config/redact-argv.js"; +import { REDACTED_SENTINEL, redactConfigObject } from "../../config/redact-snapshot.js"; import { buildConfigSchema } from "../../config/schema.js"; +import type { ExecApprovalRequest } from "../../infra/exec-approvals.js"; +import type { ReplyPayload } from "../types.js"; import { rejectNonOwnerCommand, rejectUnauthorizedCommand, requireCommandFlagEnabled, requireGatewayClientScope, } from "./command-gates.js"; -import type { CommandHandler } from "./commands-types.js"; +import { + deliverPrivateCommandReply, + readCommandDeliveryTarget, + readCommandMessageThreadId, + resolvePrivateCommandApprovalRouteExpiresAtMs, + resolvePrivateCommandRouteTargets, + type PrivateCommandRouteTarget, +} from "./commands-private-route.js"; +import type { CommandHandler, HandleCommandsParams } from "./commands-types.js"; import { parseMcpCommand } from "./mcp-commands.js"; +const MCP_SHOW_PRIVATE_ROUTE_UNAVAILABLE = + "I couldn't find a private owner route for MCP configuration. Run /mcp show from an owner DM so sensitive server details are not posted in this chat."; +const MCP_SHOW_PRIVATE_ROUTE_ACK = + "MCP server configuration is sensitive. I sent the details to the owner privately."; + +type McpCommandDeps = { + resolvePrivateMcpTargets: (params: HandleCommandsParams) => Promise; + deliverPrivateMcpReply: (params: { + commandParams: HandleCommandsParams; + targets: PrivateCommandRouteTarget[]; + reply: ReplyPayload; + }) => Promise; +}; + +const defaultMcpCommandDeps: McpCommandDeps = { + resolvePrivateMcpTargets: resolvePrivateMcpTargetsForCommand, + deliverPrivateMcpReply: deliverPrivateCommandReply, +}; + function renderJsonBlock(label: string, value: unknown): string { return `${label}\n\`\`\`json\n${JSON.stringify(value, null, 2)}\n\`\`\``; } -/** Redact MCP server secrets (headers/env/url material) before chat display. */ +function redactMcpServerArgsForDisplay(server: unknown): unknown { + if (!server || typeof server !== "object" || Array.isArray(server)) { + return server; + } + const record = server as Record; + if (!Array.isArray(record.args) || !record.args.every((arg) => typeof arg === "string")) { + return server; + } + return { + ...record, + args: redactSensitiveArgv(record.args, REDACTED_SENTINEL), + }; +} + +/** Redact MCP server secrets before chat display. */ function redactMcpServersForDisplay(servers: Record): Record { - const redactedRoot = redactConfigObject({ mcp: { servers } }, buildConfigSchema().uiHints) as { + const argvRedacted = Object.fromEntries( + Object.entries(servers).map(([name, server]) => [name, redactMcpServerArgsForDisplay(server)]), + ); + const redactedRoot = redactConfigObject( + { mcp: { servers: argvRedacted } }, + buildConfigSchema().uiHints, + ) as { mcp?: { servers?: Record }; }; return redactedRoot.mcp?.servers ?? {}; } -/** Command handler for /mcp show/set/unset operations. */ -export const handleMcpCommand: CommandHandler = async (params, allowTextCommands) => { - if (!allowTextCommands) { - return null; +async function buildMcpShowReply(name?: string): Promise { + const loaded = await listConfiguredMcpServers(); + if (!loaded.ok) { + return { text: `⚠️ ${loaded.error}` }; } - const mcpCommand = parseMcpCommand(params.command.commandBodyNormalized); - if (!mcpCommand) { - return null; - } - const unauthorized = rejectUnauthorizedCommand(params, "/mcp"); - if (unauthorized) { - return unauthorized; - } - const nonOwner = rejectNonOwnerCommand(params, "/mcp"); - if (nonOwner) { - return nonOwner; - } - const disabled = requireCommandFlagEnabled(params.cfg, { - label: "/mcp", - configKey: "mcp", - }); - if (disabled) { - return disabled; - } - if (mcpCommand.action === "error") { + if (name) { + const server = loaded.mcpServers[name]; + if (!server) { + return { text: `🔌 No MCP server named "${name}" in ${loaded.path}.` }; + } + const redactedServer = redactMcpServersForDisplay({ + [name]: server, + })[name]; return { - shouldContinue: false, - reply: { text: `⚠️ ${mcpCommand.message}` }, + text: renderJsonBlock(`🔌 MCP server "${name}" (${loaded.path})`, redactedServer), }; } + if (Object.keys(loaded.mcpServers).length === 0) { + return { text: `🔌 No MCP servers configured in ${loaded.path}.` }; + } + return { + text: renderJsonBlock( + `🔌 MCP servers (${loaded.path})`, + redactMcpServersForDisplay(loaded.mcpServers), + ), + }; +} - if (mcpCommand.action === "show") { - const loaded = await listConfiguredMcpServers(); - if (!loaded.ok) { +async function resolvePrivateMcpTargetsForCommand( + params: HandleCommandsParams, +): Promise { + return await resolvePrivateCommandRouteTargets({ + commandParams: params, + request: buildMcpShowPrivateRouteRequest(params), + }); +} + +function buildMcpShowPrivateRouteRequest(params: HandleCommandsParams): ExecApprovalRequest { + const now = Date.now(); + const agentId = + params.agentId ?? + resolveSessionAgentId({ + sessionKey: params.sessionKey, + config: params.cfg, + }); + return { + id: "mcp-show-private-route", + request: { + command: params.command.commandBodyNormalized, + agentId, + ...(params.sessionKey ? { sessionKey: params.sessionKey } : {}), + turnSourceChannel: params.command.channel, + turnSourceTo: readCommandDeliveryTarget(params) ?? null, + turnSourceAccountId: params.ctx.AccountId ?? null, + turnSourceThreadId: readCommandMessageThreadId(params) ?? null, + }, + createdAtMs: now, + expiresAtMs: resolvePrivateCommandApprovalRouteExpiresAtMs(now), + }; +} + +async function deliverGroupMcpShowReplyPrivately( + deps: McpCommandDeps, + params: HandleCommandsParams, + name?: string, +) { + const targets = await deps.resolvePrivateMcpTargets(params); + if (targets.length === 0) { + return { + shouldContinue: false, + reply: { text: MCP_SHOW_PRIVATE_ROUTE_UNAVAILABLE }, + }; + } + const privateReply = await buildMcpShowReply(name); + for (const target of targets) { + if ( + await deps.deliverPrivateMcpReply({ + commandParams: params, + targets: [target], + reply: privateReply, + }) + ) { return { shouldContinue: false, - reply: { text: `⚠️ ${loaded.error}` }, + reply: { text: MCP_SHOW_PRIVATE_ROUTE_ACK }, }; } - if (mcpCommand.name) { - const server = loaded.mcpServers[mcpCommand.name]; - if (!server) { + } + return { + shouldContinue: false, + reply: { text: MCP_SHOW_PRIVATE_ROUTE_UNAVAILABLE }, + }; +} + +/** Creates an MCP command handler with injectable private-route dependencies. */ +export function createMcpCommandHandler(deps: Partial = {}): CommandHandler { + const resolvedDeps: McpCommandDeps = { + ...defaultMcpCommandDeps, + ...deps, + }; + return async (params, allowTextCommands) => { + if (!allowTextCommands) { + return null; + } + const mcpCommand = parseMcpCommand(params.command.commandBodyNormalized); + if (!mcpCommand) { + return null; + } + const unauthorized = rejectUnauthorizedCommand(params, "/mcp"); + if (unauthorized) { + return unauthorized; + } + const nonOwner = rejectNonOwnerCommand(params, "/mcp"); + if (nonOwner) { + return nonOwner; + } + const disabled = requireCommandFlagEnabled(params.cfg, { + label: "/mcp", + configKey: "mcp", + }); + if (disabled) { + return disabled; + } + if (mcpCommand.action === "error") { + return { + shouldContinue: false, + reply: { text: `⚠️ ${mcpCommand.message}` }, + }; + } + + if (mcpCommand.action === "show") { + if (params.isGroup) { + return await deliverGroupMcpShowReplyPrivately(resolvedDeps, params, mcpCommand.name); + } + return { + shouldContinue: false, + reply: await buildMcpShowReply(mcpCommand.name), + }; + } + + const missingAdminScope = requireGatewayClientScope(params, { + label: "/mcp write", + allowedScopes: ["operator.admin"], + missingText: "❌ /mcp set|unset requires operator.admin for gateway clients.", + }); + if (missingAdminScope) { + return missingAdminScope; + } + + if (mcpCommand.action === "set") { + const result = await setConfiguredMcpServer({ + name: mcpCommand.name, + server: mcpCommand.value, + }); + if (!result.ok) { return { shouldContinue: false, - reply: { text: `🔌 No MCP server named "${mcpCommand.name}" in ${loaded.path}.` }, + reply: { text: `⚠️ ${result.error}` }, }; } - const redactedServer = redactMcpServersForDisplay({ - [mcpCommand.name]: server, - })[mcpCommand.name]; return { shouldContinue: false, reply: { - text: renderJsonBlock( - `🔌 MCP server "${mcpCommand.name}" (${loaded.path})`, - redactedServer, - ), + text: `🔌 MCP server "${mcpCommand.name}" saved to ${result.path}.`, }, }; } - if (Object.keys(loaded.mcpServers).length === 0) { - return { - shouldContinue: false, - reply: { text: `🔌 No MCP servers configured in ${loaded.path}.` }, - }; - } - return { - shouldContinue: false, - reply: { - text: renderJsonBlock( - `🔌 MCP servers (${loaded.path})`, - redactMcpServersForDisplay(loaded.mcpServers), - ), - }, - }; - } - const missingAdminScope = requireGatewayClientScope(params, { - label: "/mcp write", - allowedScopes: ["operator.admin"], - missingText: "❌ /mcp set|unset requires operator.admin for gateway clients.", - }); - if (missingAdminScope) { - return missingAdminScope; - } - - if (mcpCommand.action === "set") { - const result = await setConfiguredMcpServer({ - name: mcpCommand.name, - server: mcpCommand.value, - }); + const result = await unsetConfiguredMcpServer({ name: mcpCommand.name }); if (!result.ok) { return { shouldContinue: false, reply: { text: `⚠️ ${result.error}` }, }; } + if (!result.removed) { + return { + shouldContinue: false, + reply: { text: `🔌 No MCP server named "${mcpCommand.name}" in ${result.path}.` }, + }; + } return { shouldContinue: false, - reply: { - text: `🔌 MCP server "${mcpCommand.name}" saved to ${result.path}.`, - }, + reply: { text: `🔌 MCP server "${mcpCommand.name}" removed from ${result.path}.` }, }; - } - - const result = await unsetConfiguredMcpServer({ name: mcpCommand.name }); - if (!result.ok) { - return { - shouldContinue: false, - reply: { text: `⚠️ ${result.error}` }, - }; - } - if (!result.removed) { - return { - shouldContinue: false, - reply: { text: `🔌 No MCP server named "${mcpCommand.name}" in ${result.path}.` }, - }; - } - return { - shouldContinue: false, - reply: { text: `🔌 MCP server "${mcpCommand.name}" removed from ${result.path}.` }, }; -}; +} + +/** Command handler for /mcp show/set/unset operations. */ +export const handleMcpCommand: CommandHandler = createMcpCommandHandler(); diff --git a/src/config/io.audit.test.ts b/src/config/io.audit.test.ts index f961b6e15346..04bdc1440b27 100644 --- a/src/config/io.audit.test.ts +++ b/src/config/io.audit.test.ts @@ -348,6 +348,15 @@ describe("config io audit helpers", () => { "--rotated-signing-key=PEM-LIKE-MATERIAL", "--ops-master-key", "ABCDEF1234567890", + "--secret-key", + "opaque-secret-key", + "--aws-secret-access-key=opaque-aws-secret", + "--openai_api_key", + "opaque-underscore-key", + "--aws_secret_access_key=opaque-underscore-aws-secret", + "--credentials", + "opaque-plural-credential", + "--service-credentials=opaque-inline-plural-credential", ]; const result = redactConfigAuditArgv(argv); expect(result).toEqual([ @@ -358,6 +367,15 @@ describe("config io audit helpers", () => { "--rotated-signing-key=***", "--ops-master-key", "***", + "--secret-key", + "***", + "--aws-secret-access-key=***", + "--openai_api_key", + "***", + "--aws_secret_access_key=***", + "--credentials", + "***", + "--service-credentials=***", ]); }); diff --git a/src/config/io.audit.ts b/src/config/io.audit.ts index 4dce1f081f44..c5ce21734c07 100644 --- a/src/config/io.audit.ts +++ b/src/config/io.audit.ts @@ -1,109 +1,13 @@ // Audits config paths and values for diagnostics and safety checks. import path from "node:path"; -import { redactSecrets, redactToolPayloadText } from "../logging/redact.js"; +import { redactSecrets } from "../logging/redact.js"; import { resolveStateDir } from "./paths.js"; +import { redactSensitiveArgv } from "./redact-argv.js"; const CONFIG_AUDIT_ARGV_CAP = 8; -// Conservative list of credential-bearing flags. The heuristic suffix -// classifier below catches the long tail (`--custom-api-key`, -// `--alibaba-model-studio-api-key`, plugin-defined `cliFlag` values, etc.) -// without needing every name enumerated here. -const SECRET_FLAG_NAMES = new Set([ - "--token", - "--api-key", - "--apikey", - "--secret", - "--password", - "--passwd", - "--auth-token", - "--access-token", - "--refresh-token", - "--client-secret", - "--hook-token", - "--gateway-token", - "--bot-token", - "--app-token", - "--remote-token", - "--push-token", - "--webhook-secret", - "--webhook-token", - "--service-account-token", - "--op-service-account-token", - "--bearer", - "--bearer-token", - "--pat", - "--personal-access-token", - "--oauth-token", - "--id-token", - "--identity-token", - "--session-token", - "--service-token", - "--private-key", - "--recovery-key", - "--gateway-key", - "--session-key", - "--active-key", -]); - -// Suffix-based heuristic. Any `--…-(token|secret|password|passwd|api-key| -// apikey|api-secret|webhook|credential|bearer|pat|private-key|recovery-key| -// signing-key|encryption-key|master-key|session-key|gateway-key|service-key| -// hook-key)` is treated as a secret flag in addition to the explicit list. -// The leading `--` is required so we don't mismatch arbitrary positional args. -const SECRET_FLAG_SUFFIX_PATTERN = - /^--(?:[a-z0-9]+(?:-[a-z0-9]+)*-)?(?:token|secret|password|passwd|api[-_]?key|api[-_]?secret|webhook|credential|bearer|pat|private[-_]?key|recovery[-_]?key|signing[-_]?key|encryption[-_]?key|master[-_]?key|session[-_]?key|gateway[-_]?key|service[-_]?key|hook[-_]?key)$/; - -function isSecretFlagName(flagName: string): boolean { - if (SECRET_FLAG_NAMES.has(flagName)) { - return true; - } - return SECRET_FLAG_SUFFIX_PATTERN.test(flagName); -} - -function parseFlagName(arg: string): string | null { - if (!arg.startsWith("--")) { - return null; - } - const eq = arg.indexOf("="); - return (eq === -1 ? arg : arg.slice(0, eq)).toLowerCase(); -} - -// Redacts CLI argv before it lands in the persistent config-audit log. -// Layers, applied per element: -// 1. `--flag=value` form for any name matching the explicit list or the -// suffix heuristic — mask the value half. -// 2. value following a bare `--flag` form — emit `***` instead of the -// next arg, even if it starts with `-`. Command parsers accept -// dash-leading values for required options, and this persistent audit -// log should fail closed. -// 3. fall back to redactToolPayloadText for everything else, which catches -// `KEY=VALUE` env-style assignments, raw token shapes (sk-, ghp_, xox*, -// gsk_, AIza*, npm_, Telegram bot tokens, PEM blocks, Bearer headers, -// URL query secrets) using the shared redaction patterns. export function redactConfigAuditArgv(argv: readonly string[]): string[] { - const result: string[] = []; - let redactNext = false; - for (const current of argv) { - if (redactNext) { - redactNext = false; - result.push("***"); - continue; - } - const currentFlag = parseFlagName(current); - if (currentFlag !== null && isSecretFlagName(currentFlag)) { - if (current.includes("=")) { - const eq = current.indexOf("="); - result.push(`${current.slice(0, eq + 1)}***`); - continue; - } - result.push(current); - redactNext = true; - continue; - } - result.push(redactToolPayloadText(current)); - } - return result; + return redactSensitiveArgv(argv); } function capArgv(argv: readonly string[] | undefined): string[] { diff --git a/src/config/mcp-config.test.ts b/src/config/mcp-config.test.ts index a77697ae994b..b6cd7344ed91 100644 --- a/src/config/mcp-config.test.ts +++ b/src/config/mcp-config.test.ts @@ -164,7 +164,15 @@ describe("config mcp config", () => { servers: { billing: { command: "uvx", - args: ["billing-mcp"], + args: [ + "billing-mcp", + "--api-key", + "real-argv-key", + "--token=real-inline-token", + "ghp_realgithubtoken1234567890ABCD", + "--region", + "us-east-1", + ], headers: { Authorization: "Bearer real-token", }, @@ -180,7 +188,15 @@ describe("config mcp config", () => { name: "billing", server: { command: "uvx", - args: ["billing-mcp", "--verbose"], + args: [ + "billing-mcp", + "--api-key", + REDACTED_SENTINEL, + `--token=${REDACTED_SENTINEL}`, + REDACTED_SENTINEL, + "--region", + "us-east-1", + ], headers: { Authorization: REDACTED_SENTINEL, }, @@ -198,7 +214,15 @@ describe("config mcp config", () => { } expect(loaded.mcpServers.billing).toEqual({ command: "uvx", - args: ["billing-mcp", "--verbose"], + args: [ + "billing-mcp", + "--api-key", + "real-argv-key", + "--token=real-inline-token", + "ghp_realgithubtoken1234567890ABCD", + "--region", + "us-east-1", + ], headers: { Authorization: "Bearer real-token", }, @@ -210,13 +234,68 @@ describe("config mcp config", () => { ); }); + it("rejects redacted MCP argv when its flag binding or shape changed", async () => { + await withMcpConfigHome( + { + mcp: { + servers: { + billing: { + command: "uvx", + args: ["billing-mcp", "--api-key", "real-argv-key"], + }, + }, + }, + }, + async () => { + const changedFlag = await setConfiguredMcpServer({ + name: "billing", + server: { + command: "uvx", + args: ["billing-mcp", "--output", REDACTED_SENTINEL], + }, + }); + expect(changedFlag.ok).toBe(false); + if (changedFlag.ok) { + throw new Error("expected changed argv binding to fail"); + } + expect(changedFlag.error).toContain(REDACTED_SENTINEL); + + const changedNonSecretArg = await setConfiguredMcpServer({ + name: "billing", + server: { + command: "uvx", + args: ["other-mcp", "--api-key", REDACTED_SENTINEL], + }, + }); + expect(changedNonSecretArg.ok).toBe(false); + if (changedNonSecretArg.ok) { + throw new Error("expected argv edit with a redacted value to fail"); + } + expect(changedNonSecretArg.error).toContain("Replace every redacted value explicitly"); + + const changedShape = await setConfiguredMcpServer({ + name: "billing", + server: { + command: "uvx", + args: ["--api-key", REDACTED_SENTINEL], + }, + }); + expect(changedShape.ok).toBe(false); + if (changedShape.ok) { + throw new Error("expected changed argv shape to fail"); + } + expect(changedShape.error).toContain(REDACTED_SENTINEL); + }, + ); + }); + it("rejects unrestorable redacted MCP secrets on set for a new server", async () => { await withMcpConfigHome({}, async () => { const setResult = await setConfiguredMcpServer({ name: "new-server", server: { command: "uvx", - args: ["new-mcp"], + args: ["new-mcp", "--api-key", REDACTED_SENTINEL], headers: { Authorization: REDACTED_SENTINEL, }, diff --git a/src/config/mcp-config.ts b/src/config/mcp-config.ts index 0c18dd45aa78..78f9db818ef5 100644 --- a/src/config/mcp-config.ts +++ b/src/config/mcp-config.ts @@ -6,13 +6,18 @@ import { normalizeConfiguredMcpServers, } from "./mcp-config-normalize.js"; import { replaceConfigFile } from "./mutate.js"; -import { restoreRedactedValues } from "./redact-snapshot.js"; +import { redactSensitiveArgv } from "./redact-argv.js"; +import { REDACTED_SENTINEL, restoreRedactedValues } from "./redact-snapshot.js"; import { buildConfigSchema } from "./schema.js"; import type { OpenClawConfig } from "./types.openclaw.js"; import { validateConfigObjectWithPlugins } from "./validation.js"; type ConfigMcpServers = ReturnType; +type McpArgvRestoreResult = + | { ok: true; server: Record } + | { ok: false; error: string }; + type ConfigMcpReadResult = | { ok: true; @@ -50,6 +55,55 @@ function normalizeToolSelectionList(value: readonly string[] | undefined): strin return normalized.length > 0 ? normalized : undefined; } +function restoreMcpServerArgvSentinels(params: { + incoming: Record; + original: Record | undefined; +}): McpArgvRestoreResult { + const incomingArgs = params.incoming.args; + if (!Array.isArray(incomingArgs)) { + return { ok: true, server: params.incoming }; + } + const hasSentinel = incomingArgs.some( + (arg) => typeof arg === "string" && arg.includes(REDACTED_SENTINEL), + ); + if (!hasSentinel) { + return { ok: true, server: params.incoming }; + } + + const originalArgs = params.original?.args; + if ( + !Array.isArray(originalArgs) || + !originalArgs.every((arg) => typeof arg === "string") || + incomingArgs.length !== originalArgs.length + ) { + return { + ok: false, + error: + 'Cannot restore MCP args containing "' + + REDACTED_SENTINEL + + '" without the same original argv shape.', + }; + } + + const displayedArgs = redactSensitiveArgv(originalArgs, REDACTED_SENTINEL); + if (incomingArgs.some((arg, index) => arg !== displayedArgs[index])) { + return { + ok: false, + error: + 'Cannot restore MCP args containing "' + + REDACTED_SENTINEL + + '" after argv changed. Replace every redacted value explicitly before editing args.', + }; + } + return { + ok: true, + server: { + ...params.incoming, + args: originalArgs, + }, + }; +} + export async function listConfiguredMcpServers(): Promise { const snapshot = await readSourceConfigSnapshot(); if (!snapshot.valid) { @@ -178,10 +232,22 @@ export async function setConfiguredMcpServer(params: { return loaded; } + const argvRestored = restoreMcpServerArgvSentinels({ + incoming: params.server, + original: loaded.mcpServers[name], + }); + if (!argvRestored.ok) { + return { + ok: false, + path: loaded.path, + error: argvRestored.error, + }; + } + // Restore redaction sentinels from the existing server entry so a show→set // round-trip cannot replace real credentials with the display placeholder. const restored = restoreRedactedValues( - { mcp: { servers: { [name]: params.server } } }, + { mcp: { servers: { [name]: argvRestored.server } } }, { mcp: { servers: loaded.mcpServers } }, buildConfigSchema().uiHints, ); diff --git a/src/config/redact-argv.ts b/src/config/redact-argv.ts new file mode 100644 index 000000000000..03ea39a20212 --- /dev/null +++ b/src/config/redact-argv.ts @@ -0,0 +1,88 @@ +// Redacts credential-bearing command arguments while preserving argv shape. +import { redactToolPayloadText } from "../logging/redact.js"; + +// Conservative list of credential-bearing flags. The heuristic suffix +// classifier below catches plugin-defined flags without enumerating each one. +const SECRET_FLAG_NAMES = new Set([ + "--token", + "--api-key", + "--apikey", + "--secret", + "--password", + "--passwd", + "--auth-token", + "--access-token", + "--refresh-token", + "--client-secret", + "--hook-token", + "--gateway-token", + "--bot-token", + "--app-token", + "--remote-token", + "--push-token", + "--webhook-secret", + "--webhook-token", + "--service-account-token", + "--op-service-account-token", + "--bearer", + "--bearer-token", + "--pat", + "--personal-access-token", + "--oauth-token", + "--id-token", + "--identity-token", + "--session-token", + "--service-token", + "--private-key", + "--recovery-key", + "--gateway-key", + "--session-key", + "--active-key", +]); + +const SECRET_FLAG_SUFFIX_PATTERN = + /^--(?:[a-z0-9]+(?:[-_][a-z0-9]+)*[-_])?(?:token|secret|password|passwd|passphrase|pin|api[-_]?key|api[-_]?secret|secret[-_]?key|secret[-_]?access[-_]?key|access[-_]?key(?:[-_]?id)?|account[-_]?key|client[-_]?key|consumer[-_]?key|license[-_]?key|subscription[-_]?key|webhook|credentials?|creds?|auth(?:orization)?|bearer|pat|cookie|private[-_]?key|recovery[-_]?key|signing[-_]?key|encryption[-_]?key|master[-_]?key|session[-_]?key|gateway[-_]?key|service[-_]?key|hook[-_]?key)$/; + +function parseFlagName(arg: string): string | null { + if (!arg.startsWith("--")) { + return null; + } + const equalsIndex = arg.indexOf("="); + return (equalsIndex === -1 ? arg : arg.slice(0, equalsIndex)).toLowerCase(); +} + +function isSecretFlagName(flagName: string): boolean { + return SECRET_FLAG_NAMES.has(flagName) || SECRET_FLAG_SUFFIX_PATTERN.test(flagName); +} + +/** + * Redacts recognized argv secrets without changing array length or non-secret flags. + * Known secret flags bind the next value even when it begins with a dash; other + * elements use the shared deterministic secret-text patterns. + */ +export function redactSensitiveArgv(argv: readonly string[], redactedValue?: string): string[] { + const replacement = redactedValue ?? "***"; + const result: string[] = []; + let redactNext = false; + for (const current of argv) { + if (redactNext) { + redactNext = false; + result.push(replacement); + continue; + } + const currentFlag = parseFlagName(current); + if (currentFlag !== null && isSecretFlagName(currentFlag)) { + const equalsIndex = current.indexOf("="); + if (equalsIndex !== -1) { + result.push(`${current.slice(0, equalsIndex + 1)}${replacement}`); + continue; + } + result.push(current); + redactNext = true; + continue; + } + const redacted = redactToolPayloadText(current); + result.push(redacted === current ? current : (redactedValue ?? redacted)); + } + return result; +} diff --git a/test/mcp-show-redact.e2e.test.ts b/test/mcp-show-redact.e2e.test.ts index 165361585137..4fecd13bfd15 100644 --- a/test/mcp-show-redact.e2e.test.ts +++ b/test/mcp-show-redact.e2e.test.ts @@ -16,6 +16,9 @@ const CHAT_FINAL_TIMEOUT_MS = 45_000; const HEADER_SECRET = "Bearer e2e-live-mcp-header-secret-value"; const ENV_SECRET = "e2e-live-mcp-env-secret-value"; +const ARG_SECRET = "e2e-live-mcp-argv-secret-value"; +const ARG_INLINE_SECRET = "e2e-live-mcp-inline-argv-secret-value"; +const ARG_POSITIONAL_SECRET = "ghp_e2elivemcpargvtoken1234567890ABCD"; const SERVER_NAME = "billing-server"; type ChatEventPayload = { @@ -93,7 +96,15 @@ describe("mcp show redaction e2e", () => { servers: { [SERVER_NAME]: { command: "uvx", - args: ["billing-mcp"], + args: [ + "billing-mcp", + "--api-key", + ARG_SECRET, + `--token=${ARG_INLINE_SECRET}`, + ARG_POSITIONAL_SECRET, + "--region", + "us-east-1", + ], transport: "streamable-http", url: "https://billing.example.com/mcp", headers: { @@ -151,6 +162,14 @@ describe("mcp show redaction e2e", () => { expect(namedText).toContain(REDACTED_SENTINEL); expect(namedText).not.toContain(HEADER_SECRET); expect(namedText).not.toContain(ENV_SECRET); + expect(namedText).not.toContain(ARG_SECRET); + expect(namedText).not.toContain(ARG_INLINE_SECRET); + expect(namedText).not.toContain(ARG_POSITIONAL_SECRET); + expect(namedText).toContain('"billing-mcp"'); + expect(namedText).toContain('"--api-key"'); + expect(namedText).toContain(`"--token=${REDACTED_SENTINEL}"`); + expect(namedText).toContain('"--region"'); + expect(namedText).toContain('"us-east-1"'); expect(namedText).not.toContain("e2e-live-mcp-header-secret-value"); expect(namedText).not.toContain("e2e-live-mcp-env-secret-value"); @@ -169,8 +188,60 @@ describe("mcp show redaction e2e", () => { expect(listText).toContain(REDACTED_SENTINEL); expect(listText).not.toContain(HEADER_SECRET); expect(listText).not.toContain(ENV_SECRET); + expect(listText).not.toContain(ARG_SECRET); + expect(listText).not.toContain(ARG_INLINE_SECRET); + expect(listText).not.toContain(ARG_POSITIONAL_SECRET); expect(listText).not.toContain("second-env-secret-value"); + // CLI show → set can safely round-trip redacted argv because it preserves JSON arrays. + // The chat set below intentionally omits args to avoid chat body bracket normalization. + const cliArgSet = await instance.cli([ + "mcp", + "set", + SERVER_NAME, + JSON.stringify({ + command: "uvx", + args: [ + "billing-mcp", + "--api-key", + REDACTED_SENTINEL, + `--token=${REDACTED_SENTINEL}`, + REDACTED_SENTINEL, + "--region", + "us-east-1", + ], + headers: { Authorization: REDACTED_SENTINEL }, + env: { BILLING_TOKEN: REDACTED_SENTINEL }, + }), + ]); + expect(cliArgSet.code).toBe(0); + expect(cliArgSet.stdout + cliArgSet.stderr).toContain(`Saved MCP server "${SERVER_NAME}"`); + const afterCliArgSet = JSON.parse(await fs.readFile(instance.configPath, "utf8")) as { + mcp?: { + servers?: Record< + string, + { + args?: string[]; + headers?: Record; + env?: Record; + } + >; + }; + }; + const argvRestored = afterCliArgSet.mcp?.servers?.[SERVER_NAME]; + expect(argvRestored?.args).toEqual([ + "billing-mcp", + "--api-key", + ARG_SECRET, + `--token=${ARG_INLINE_SECRET}`, + ARG_POSITIONAL_SECRET, + "--region", + "us-east-1", + ]); + expect(argvRestored?.headers?.Authorization).toBe(HEADER_SECRET); + expect(argvRestored?.env?.BILLING_TOKEN).toBe(ENV_SECRET); + expect(JSON.stringify(argvRestored)).not.toContain(REDACTED_SENTINEL); + // show → set with redacted secrets must restore live values, not write the sentinel. // Keep the JSON free of array brackets so chat body normalization cannot mangle it. const setPayload = {