mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 02:06:43 +00:00
fix(mcp): clamp oversized MCP timeouts (#105784)
* fix(mcp): clamp oversized MCP timeouts * fix(mcp): centralize timer-safe timeout normalization Co-authored-by: 杨浩宇0668001029 <yang.haoyu@xydigit.com> --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
Peter Steinberger
parent
3383e35aa3
commit
1da345e9d3
@@ -95,6 +95,7 @@ describe("number-coercion", () => {
|
||||
expect(finiteSecondsToTimerSafeMilliseconds(1.5)).toBe(1_500);
|
||||
expect(finiteSecondsToTimerSafeMilliseconds(1.5, { floorSeconds: true })).toBe(1_000);
|
||||
expect(finiteSecondsToTimerSafeMilliseconds(10_000_000)).toBe(MAX_TIMER_TIMEOUT_MS);
|
||||
expect(finiteSecondsToTimerSafeMilliseconds(Number.MAX_VALUE)).toBe(MAX_TIMER_TIMEOUT_MS);
|
||||
expect(finiteSecondsToTimerSafeMilliseconds("10")).toBeUndefined();
|
||||
expect(finiteSecondsToTimerSafeMilliseconds(Number.POSITIVE_INFINITY)).toBeUndefined();
|
||||
expect(clampTimerTimeoutMs(0, 10)).toBe(10);
|
||||
|
||||
@@ -214,6 +214,9 @@ export function finiteSecondsToTimerSafeMilliseconds(
|
||||
return undefined;
|
||||
}
|
||||
const boundedSeconds = opts.floorSeconds ? Math.floor(seconds) : seconds;
|
||||
if (boundedSeconds >= MAX_TIMER_TIMEOUT_SECONDS) {
|
||||
return MAX_TIMER_TIMEOUT_MS;
|
||||
}
|
||||
const milliseconds = Math.floor(boundedSeconds * 1000);
|
||||
if (!Number.isFinite(milliseconds) || milliseconds <= 0) {
|
||||
return undefined;
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
// Verifies MCP transport config normalization and startup-safety filtering.
|
||||
import { MAX_TIMER_TIMEOUT_MS } from "@openclaw/normalization-core/number-coercion";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { logWarn } from "../logger.js";
|
||||
import { resolveMcpTransportConfig } from "./mcp-transport-config.js";
|
||||
@@ -48,6 +49,32 @@ describe("resolveMcpTransportConfig", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("clamps oversized MCP request timeout aliases to the Node timer maximum", () => {
|
||||
const seconds = resolveMcpTransportConfig("probe", {
|
||||
command: "node",
|
||||
connectTimeout: 1e306,
|
||||
timeout: 1e306,
|
||||
});
|
||||
const milliseconds = resolveMcpTransportConfig("probe", {
|
||||
command: "node",
|
||||
connectionTimeoutMs: 1e306,
|
||||
requestTimeoutMs: 1e306,
|
||||
});
|
||||
|
||||
expect(seconds).toEqual(
|
||||
expect.objectContaining({
|
||||
connectionTimeoutMs: MAX_TIMER_TIMEOUT_MS,
|
||||
requestTimeoutMs: MAX_TIMER_TIMEOUT_MS,
|
||||
}),
|
||||
);
|
||||
expect(milliseconds).toEqual(
|
||||
expect.objectContaining({
|
||||
connectionTimeoutMs: MAX_TIMER_TIMEOUT_MS,
|
||||
requestTimeoutMs: MAX_TIMER_TIMEOUT_MS,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("drops dangerous env overrides from stdio config", () => {
|
||||
// Stdio env is inherited executable process input. Block loader/shell hook
|
||||
// variables and child-process config pivots while preserving explicit MCP
|
||||
|
||||
@@ -1,6 +1,11 @@
|
||||
/**
|
||||
* Resolves MCP transport command, environment, and timeout configuration.
|
||||
*/
|
||||
import {
|
||||
clampPositiveTimerTimeoutMs,
|
||||
finiteSecondsToTimerSafeMilliseconds,
|
||||
resolvePositiveTimerTimeoutMs,
|
||||
} from "@openclaw/normalization-core/number-coercion";
|
||||
import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/string-coerce";
|
||||
import { sanitizeForLog } from "../../packages/terminal-core/src/ansi.js";
|
||||
import { resolveOpenClawMcpTransportAlias } from "../config/mcp-config-normalize.js";
|
||||
@@ -69,25 +74,28 @@ function getPositiveNumber(rawServer: unknown, keys: readonly string[]): number
|
||||
function getConnectionTimeoutMs(rawServer: unknown): number {
|
||||
const milliseconds = getPositiveNumber(rawServer, ["connectionTimeoutMs"]);
|
||||
if (milliseconds) {
|
||||
return Math.floor(milliseconds);
|
||||
return clampPositiveTimerTimeoutMs(milliseconds) ?? DEFAULT_CONNECTION_TIMEOUT_MS;
|
||||
}
|
||||
const seconds = getPositiveNumber(rawServer, ["connectTimeout", "connect_timeout"]);
|
||||
if (seconds) {
|
||||
return Math.floor(seconds * 1_000);
|
||||
return finiteSecondsToTimerSafeMilliseconds(seconds) ?? 1;
|
||||
}
|
||||
return DEFAULT_CONNECTION_TIMEOUT_MS;
|
||||
}
|
||||
|
||||
function getRequestTimeoutMs(rawServer: unknown): number {
|
||||
export function resolveMcpRequestTimeoutMs(
|
||||
rawServer: unknown,
|
||||
fallbackMs = DEFAULT_REQUEST_TIMEOUT_MS,
|
||||
): number {
|
||||
const milliseconds = getPositiveNumber(rawServer, ["requestTimeoutMs"]);
|
||||
if (milliseconds) {
|
||||
return Math.floor(milliseconds);
|
||||
return clampPositiveTimerTimeoutMs(milliseconds) ?? DEFAULT_REQUEST_TIMEOUT_MS;
|
||||
}
|
||||
const seconds = getPositiveNumber(rawServer, ["timeout"]);
|
||||
if (seconds) {
|
||||
return Math.floor(seconds * 1_000);
|
||||
return finiteSecondsToTimerSafeMilliseconds(seconds) ?? 1;
|
||||
}
|
||||
return DEFAULT_REQUEST_TIMEOUT_MS;
|
||||
return resolvePositiveTimerTimeoutMs(fallbackMs, DEFAULT_REQUEST_TIMEOUT_MS);
|
||||
}
|
||||
|
||||
function getBooleanField(rawServer: unknown, keys: readonly string[]): boolean | undefined {
|
||||
@@ -182,7 +190,7 @@ function resolveHttpTransportConfig(
|
||||
: {}),
|
||||
description: describeHttpMcpServerLaunchConfig(launch.config),
|
||||
connectionTimeoutMs: getConnectionTimeoutMs(rawServer),
|
||||
requestTimeoutMs: getRequestTimeoutMs(rawServer),
|
||||
requestTimeoutMs: resolveMcpRequestTimeoutMs(rawServer),
|
||||
supportsParallelToolCalls:
|
||||
getBooleanField(rawServer, ["supportsParallelToolCalls", "supports_parallel_tool_calls"]) ??
|
||||
false,
|
||||
@@ -217,7 +225,7 @@ export function resolveMcpTransportConfig(
|
||||
cwd: stdioLaunch.config.cwd,
|
||||
description: describeStdioMcpServerLaunchConfig(stdioLaunch.config),
|
||||
connectionTimeoutMs: getConnectionTimeoutMs(rawServer),
|
||||
requestTimeoutMs: getRequestTimeoutMs(rawServer),
|
||||
requestTimeoutMs: resolveMcpRequestTimeoutMs(rawServer),
|
||||
supportsParallelToolCalls:
|
||||
getBooleanField(rawServer, ["supportsParallelToolCalls", "supports_parallel_tool_calls"]) ??
|
||||
false,
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
import { ErrorCode, type CallToolResult, type Tool } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { expectDefined } from "@openclaw/normalization-core";
|
||||
import { MAX_TIMER_TIMEOUT_MS } from "@openclaw/normalization-core/number-coercion";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { OpenClawSchema } from "../config/zod-schema.js";
|
||||
import { startNodeHostMcpManager } from "./mcp.js";
|
||||
@@ -241,6 +242,20 @@ describe("node host MCP manager", () => {
|
||||
await manager.close();
|
||||
});
|
||||
|
||||
it("clamps oversized configured and requested MCP tool timeouts", async () => {
|
||||
const client = createClient({ tools: [tool("search")] });
|
||||
const manager = await startNodeHostMcpManager(
|
||||
{ docs: { command: "docs", timeout: 1e306 } },
|
||||
{ createClient: () => client, resolveTransport: () => transport, warn: vi.fn() },
|
||||
);
|
||||
|
||||
await manager.callMcpTool({ server: "docs", tool: "search", timeoutMs: 1e306 });
|
||||
expect(client.callTool).toHaveBeenCalledWith({ name: "search", arguments: {} }, undefined, {
|
||||
timeout: MAX_TIMER_TIMEOUT_MS,
|
||||
});
|
||||
await manager.close();
|
||||
});
|
||||
|
||||
it("bounds remote MCP error messages", async () => {
|
||||
const client = createClient({
|
||||
tools: [tool("fail")],
|
||||
|
||||
+4
-18
@@ -3,12 +3,14 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js";
|
||||
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js";
|
||||
import { ErrorCode, type CallToolResult, type Tool } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { redactSensitiveUrlLikeString } from "@openclaw/net-policy/redact-sensitive-url";
|
||||
import { clampPositiveTimerTimeoutMs } from "@openclaw/normalization-core/number-coercion";
|
||||
import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/string-coerce";
|
||||
import { truncateUtf16Safe } from "@openclaw/normalization-core/utf16-slice";
|
||||
import type { NodePluginToolDescriptor } from "../../packages/gateway-protocol/src/schema/nodes.js";
|
||||
import { matchesMcpToolFilterPattern } from "../agents/agent-bundle-mcp-filter.js";
|
||||
import { createMcpJsonSchemaValidator } from "../agents/mcp-json-schema-validator.js";
|
||||
import { sanitizeMcpMetadataText } from "../agents/mcp-metadata.js";
|
||||
import { resolveMcpRequestTimeoutMs } from "../agents/mcp-transport-config.js";
|
||||
import { resolveMcpTransport } from "../agents/mcp-transport.js";
|
||||
import { normalizeConfiguredMcpServers } from "../config/mcp-config-normalize.js";
|
||||
import type { McpServerConfig } from "../config/types.mcp.js";
|
||||
@@ -273,23 +275,7 @@ async function listAllTools(
|
||||
}
|
||||
|
||||
function resolveCallTimeoutMs(value: number | undefined): number {
|
||||
return typeof value === "number" && Number.isFinite(value) && value > 0
|
||||
? Math.floor(value)
|
||||
: NODE_MCP_TOOL_CALL_TIMEOUT_MS;
|
||||
}
|
||||
|
||||
function resolveServerToolCallTimeoutMs(config: McpServerConfig): number {
|
||||
if (
|
||||
typeof config.requestTimeoutMs === "number" &&
|
||||
Number.isFinite(config.requestTimeoutMs) &&
|
||||
config.requestTimeoutMs > 0
|
||||
) {
|
||||
return Math.floor(config.requestTimeoutMs);
|
||||
}
|
||||
if (typeof config.timeout === "number" && Number.isFinite(config.timeout) && config.timeout > 0) {
|
||||
return Math.floor(config.timeout * 1_000);
|
||||
}
|
||||
return NODE_MCP_TOOL_CALL_TIMEOUT_MS;
|
||||
return clampPositiveTimerTimeoutMs(value) ?? NODE_MCP_TOOL_CALL_TIMEOUT_MS;
|
||||
}
|
||||
|
||||
function isMcpTimeoutError(error: unknown): boolean {
|
||||
@@ -339,7 +325,7 @@ export async function startNodeHostMcpManager(
|
||||
client,
|
||||
connected: false,
|
||||
tools: new Set<string>(),
|
||||
toolCallTimeoutMs: resolveServerToolCallTimeoutMs(config),
|
||||
toolCallTimeoutMs: resolveMcpRequestTimeoutMs(config, NODE_MCP_TOOL_CALL_TIMEOUT_MS),
|
||||
detachStderr: resolved.detachStderr,
|
||||
};
|
||||
// MCP Client exposes callback properties rather than an EventTarget surface.
|
||||
|
||||
Reference in New Issue
Block a user