fix: classify /tools/invoke errors and sanitize 500s (#13185) (thanks @davidrudduck)
This commit is contained in:
@@ -13,6 +13,7 @@ Docs: https://docs.openclaw.ai
|
|||||||
- Security/Canvas: serve A2UI assets via the shared safe-open path (`openFileWithinRoot`) to close traversal/TOCTOU gaps, with traversal and symlink regression coverage. (#10525) Thanks @abdelsfane.
|
- Security/Canvas: serve A2UI assets via the shared safe-open path (`openFileWithinRoot`) to close traversal/TOCTOU gaps, with traversal and symlink regression coverage. (#10525) Thanks @abdelsfane.
|
||||||
- Security/WhatsApp: enforce `0o600` on `creds.json` and `creds.json.bak` on save/backup/restore paths to reduce credential file exposure. (#10529) Thanks @abdelsfane.
|
- Security/WhatsApp: enforce `0o600` on `creds.json` and `creds.json.bak` on save/backup/restore paths to reduce credential file exposure. (#10529) Thanks @abdelsfane.
|
||||||
- Security/Gateway + ACP: block high-risk tools (`sessions_spawn`, `sessions_send`, `gateway`, `whatsapp_login`) from HTTP `/tools/invoke` by default with `gateway.tools.{allow,deny}` overrides, and harden ACP permission selection to fail closed when tool identity/options are ambiguous while supporting `allow_always`/`reject_always`. (#15390) Thanks @aether-ai-agent.
|
- Security/Gateway + ACP: block high-risk tools (`sessions_spawn`, `sessions_send`, `gateway`, `whatsapp_login`) from HTTP `/tools/invoke` by default with `gateway.tools.{allow,deny}` overrides, and harden ACP permission selection to fail closed when tool identity/options are ambiguous while supporting `allow_always`/`reject_always`. (#15390) Thanks @aether-ai-agent.
|
||||||
|
- Gateway/Tools Invoke: sanitize `/tools/invoke` execution failures while preserving `400` for tool input errors and returning `500` for unexpected runtime failures, with regression coverage and docs updates. (#13185) Thanks @davidrudduck.
|
||||||
- MS Teams: preserve parsed mention entities/text when appending OneDrive fallback file links, and accept broader real-world Teams mention ID formats (`29:...`, `8:orgid:...`) while still rejecting placeholder patterns. (#15436) Thanks @hyojin.
|
- MS Teams: preserve parsed mention entities/text when appending OneDrive fallback file links, and accept broader real-world Teams mention ID formats (`29:...`, `8:orgid:...`) while still rejecting placeholder patterns. (#15436) Thanks @hyojin.
|
||||||
- Security/Audit: distinguish external webhooks (`hooks.enabled`) from internal hooks (`hooks.internal.enabled`) in attack-surface summaries to avoid false exposure signals when only internal hooks are enabled. (#13474) Thanks @mcaxtr.
|
- Security/Audit: distinguish external webhooks (`hooks.enabled`) from internal hooks (`hooks.internal.enabled`) in attack-surface summaries to avoid false exposure signals when only internal hooks are enabled. (#13474) Thanks @mcaxtr.
|
||||||
- Security/Onboarding: clarify multi-user DM isolation remediation with explicit `openclaw config set session.dmScope ...` commands in security audit, doctor security, and channel onboarding guidance. (#13129) Thanks @VintLin.
|
- Security/Onboarding: clarify multi-user DM isolation remediation with explicit `openclaw config set session.dmScope ...` commands in security audit, doctor security, and channel onboarding guidance. (#13129) Thanks @VintLin.
|
||||||
|
|||||||
@@ -89,11 +89,12 @@ To help group policies resolve context, you can optionally set:
|
|||||||
## Responses
|
## Responses
|
||||||
|
|
||||||
- `200` → `{ ok: true, result }`
|
- `200` → `{ ok: true, result }`
|
||||||
- `400` → `{ ok: false, error: { type, message } }` (invalid request or tool error)
|
- `400` → `{ ok: false, error: { type, message } }` (invalid request or tool input error)
|
||||||
- `401` → unauthorized
|
- `401` → unauthorized
|
||||||
- `429` → auth rate-limited (`Retry-After` set)
|
- `429` → auth rate-limited (`Retry-After` set)
|
||||||
- `404` → tool not available (not found or not allowlisted)
|
- `404` → tool not available (not found or not allowlisted)
|
||||||
- `405` → method not allowed
|
- `405` → method not allowed
|
||||||
|
- `500` → `{ ok: false, error: { type, message } }` (unexpected tool execution error; sanitized message)
|
||||||
|
|
||||||
## Example
|
## Example
|
||||||
|
|
||||||
|
|||||||
@@ -18,6 +18,15 @@ export type ActionGate<T extends Record<string, boolean | undefined>> = (
|
|||||||
defaultValue?: boolean,
|
defaultValue?: boolean,
|
||||||
) => boolean;
|
) => boolean;
|
||||||
|
|
||||||
|
export class ToolInputError extends Error {
|
||||||
|
readonly status = 400;
|
||||||
|
|
||||||
|
constructor(message: string) {
|
||||||
|
super(message);
|
||||||
|
this.name = "ToolInputError";
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export function createActionGate<T extends Record<string, boolean | undefined>>(
|
export function createActionGate<T extends Record<string, boolean | undefined>>(
|
||||||
actions: T | undefined,
|
actions: T | undefined,
|
||||||
): ActionGate<T> {
|
): ActionGate<T> {
|
||||||
@@ -49,14 +58,14 @@ export function readStringParam(
|
|||||||
const raw = params[key];
|
const raw = params[key];
|
||||||
if (typeof raw !== "string") {
|
if (typeof raw !== "string") {
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
const value = trim ? raw.trim() : raw;
|
const value = trim ? raw.trim() : raw;
|
||||||
if (!value && !allowEmpty) {
|
if (!value && !allowEmpty) {
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
@@ -80,7 +89,7 @@ export function readStringOrNumberParam(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
@@ -106,7 +115,7 @@ export function readNumberParam(
|
|||||||
}
|
}
|
||||||
if (value === undefined) {
|
if (value === undefined) {
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
@@ -137,7 +146,7 @@ export function readStringArrayParam(
|
|||||||
.filter(Boolean);
|
.filter(Boolean);
|
||||||
if (values.length === 0) {
|
if (values.length === 0) {
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
@@ -147,14 +156,14 @@ export function readStringArrayParam(
|
|||||||
const value = raw.trim();
|
const value = raw.trim();
|
||||||
if (!value) {
|
if (!value) {
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
return [value];
|
return [value];
|
||||||
}
|
}
|
||||||
if (required) {
|
if (required) {
|
||||||
throw new Error(`${label} required`);
|
throw new ToolInputError(`${label} required`);
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
@@ -181,7 +190,7 @@ export function readReactionParams(
|
|||||||
allowEmpty: true,
|
allowEmpty: true,
|
||||||
});
|
});
|
||||||
if (remove && !emoji) {
|
if (remove && !emoji) {
|
||||||
throw new Error(options.removeErrorMessage);
|
throw new ToolInputError(options.removeErrorMessage);
|
||||||
}
|
}
|
||||||
return { emoji, remove, isEmpty: !emoji };
|
return { emoji, remove, isEmpty: !emoji };
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import type { IncomingMessage, ServerResponse } from "node:http";
|
import type { IncomingMessage, ServerResponse } from "node:http";
|
||||||
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
|
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
|
||||||
|
import { ToolInputError } from "../agents/tools/common.js";
|
||||||
import { createTestRegistry } from "../test-utils/channel-plugins.js";
|
import { createTestRegistry } from "../test-utils/channel-plugins.js";
|
||||||
import { resetTestPluginRegistry, setTestPluginRegistry, testState } from "./test-helpers.mocks.js";
|
import { resetTestPluginRegistry, setTestPluginRegistry, testState } from "./test-helpers.mocks.js";
|
||||||
import { installGatewayTestHooks, getFreePort, startGatewayServer } from "./test-helpers.server.js";
|
import { installGatewayTestHooks, getFreePort, startGatewayServer } from "./test-helpers.server.js";
|
||||||
@@ -50,6 +51,31 @@ const invokeAgentsList = async (params: {
|
|||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const invokeTool = async (params: {
|
||||||
|
port: number;
|
||||||
|
tool: string;
|
||||||
|
args?: Record<string, unknown>;
|
||||||
|
action?: string;
|
||||||
|
headers?: Record<string, string>;
|
||||||
|
sessionKey?: string;
|
||||||
|
}) => {
|
||||||
|
const body: Record<string, unknown> = {
|
||||||
|
tool: params.tool,
|
||||||
|
args: params.args ?? {},
|
||||||
|
};
|
||||||
|
if (params.action) {
|
||||||
|
body.action = params.action;
|
||||||
|
}
|
||||||
|
if (params.sessionKey) {
|
||||||
|
body.sessionKey = params.sessionKey;
|
||||||
|
}
|
||||||
|
return await fetch(`http://127.0.0.1:${params.port}/tools/invoke`, {
|
||||||
|
method: "POST",
|
||||||
|
headers: { "content-type": "application/json", ...params.headers },
|
||||||
|
body: JSON.stringify(body),
|
||||||
|
});
|
||||||
|
};
|
||||||
|
|
||||||
describe("POST /tools/invoke", () => {
|
describe("POST /tools/invoke", () => {
|
||||||
let sharedPort = 0;
|
let sharedPort = 0;
|
||||||
let sharedServer: Awaited<ReturnType<typeof startGatewayServer>>;
|
let sharedServer: Awaited<ReturnType<typeof startGatewayServer>>;
|
||||||
@@ -330,4 +356,78 @@ describe("POST /tools/invoke", () => {
|
|||||||
});
|
});
|
||||||
expect(resMain.status).toBe(200);
|
expect(resMain.status).toBe(200);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("maps tool input errors to 400 and unexpected execution errors to 500", async () => {
|
||||||
|
const registry = createTestRegistry();
|
||||||
|
registry.tools.push({
|
||||||
|
pluginId: "tools-invoke-test",
|
||||||
|
source: "test",
|
||||||
|
names: ["tools_invoke_test"],
|
||||||
|
optional: false,
|
||||||
|
factory: () => ({
|
||||||
|
label: "Tools Invoke Test",
|
||||||
|
name: "tools_invoke_test",
|
||||||
|
description: "Test-only tool.",
|
||||||
|
parameters: {
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
mode: { type: "string" },
|
||||||
|
},
|
||||||
|
required: ["mode"],
|
||||||
|
additionalProperties: false,
|
||||||
|
},
|
||||||
|
execute: async (_toolCallId, args) => {
|
||||||
|
const mode = (args as { mode?: unknown }).mode;
|
||||||
|
if (mode === "input") {
|
||||||
|
throw new ToolInputError("mode invalid");
|
||||||
|
}
|
||||||
|
if (mode === "crash") {
|
||||||
|
throw new Error("boom");
|
||||||
|
}
|
||||||
|
return { ok: true };
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
setTestPluginRegistry(registry);
|
||||||
|
const { writeConfigFile } = await import("../config/config.js");
|
||||||
|
await writeConfigFile({
|
||||||
|
plugins: { enabled: true },
|
||||||
|
// oxlint-disable-next-line typescript/no-explicit-any
|
||||||
|
} as any);
|
||||||
|
|
||||||
|
const token = resolveGatewayToken();
|
||||||
|
|
||||||
|
try {
|
||||||
|
const inputRes = await invokeTool({
|
||||||
|
port: sharedPort,
|
||||||
|
tool: "tools_invoke_test",
|
||||||
|
args: { mode: "input" },
|
||||||
|
headers: { authorization: `Bearer ${token}` },
|
||||||
|
sessionKey: "main",
|
||||||
|
});
|
||||||
|
expect(inputRes.status).toBe(400);
|
||||||
|
const inputBody = await inputRes.json();
|
||||||
|
expect(inputBody.ok).toBe(false);
|
||||||
|
expect(inputBody.error?.type).toBe("tool_error");
|
||||||
|
expect(inputBody.error?.message).toBe("mode invalid");
|
||||||
|
|
||||||
|
const crashRes = await invokeTool({
|
||||||
|
port: sharedPort,
|
||||||
|
tool: "tools_invoke_test",
|
||||||
|
args: { mode: "crash" },
|
||||||
|
headers: { authorization: `Bearer ${token}` },
|
||||||
|
sessionKey: "main",
|
||||||
|
});
|
||||||
|
expect(crashRes.status).toBe(500);
|
||||||
|
const crashBody = await crashRes.json();
|
||||||
|
expect(crashBody.ok).toBe(false);
|
||||||
|
expect(crashBody.error?.type).toBe("tool_error");
|
||||||
|
expect(crashBody.error?.message).toBe("tool execution failed");
|
||||||
|
} finally {
|
||||||
|
await writeConfigFile({
|
||||||
|
// oxlint-disable-next-line typescript/no-explicit-any
|
||||||
|
} as any);
|
||||||
|
resetTestPluginRegistry();
|
||||||
|
}
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ import {
|
|||||||
resolveToolProfilePolicy,
|
resolveToolProfilePolicy,
|
||||||
stripPluginOnlyAllowlist,
|
stripPluginOnlyAllowlist,
|
||||||
} from "../agents/tool-policy.js";
|
} from "../agents/tool-policy.js";
|
||||||
|
import { ToolInputError } from "../agents/tools/common.js";
|
||||||
import { loadConfig } from "../config/config.js";
|
import { loadConfig } from "../config/config.js";
|
||||||
import { resolveMainSessionKey } from "../config/sessions.js";
|
import { resolveMainSessionKey } from "../config/sessions.js";
|
||||||
import { logWarn } from "../logger.js";
|
import { logWarn } from "../logger.js";
|
||||||
@@ -116,6 +117,28 @@ function mergeActionIntoArgsIfSupported(params: {
|
|||||||
return { ...args, action };
|
return { ...args, action };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function getErrorMessage(err: unknown): string {
|
||||||
|
if (err instanceof Error) {
|
||||||
|
return err.message || String(err);
|
||||||
|
}
|
||||||
|
if (typeof err === "string") {
|
||||||
|
return err;
|
||||||
|
}
|
||||||
|
return String(err);
|
||||||
|
}
|
||||||
|
|
||||||
|
function isToolInputError(err: unknown): boolean {
|
||||||
|
if (err instanceof ToolInputError) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
return (
|
||||||
|
typeof err === "object" &&
|
||||||
|
err !== null &&
|
||||||
|
"name" in err &&
|
||||||
|
(err as { name?: unknown }).name === "ToolInputError"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
export async function handleToolsInvokeHttpRequest(
|
export async function handleToolsInvokeHttpRequest(
|
||||||
req: IncomingMessage,
|
req: IncomingMessage,
|
||||||
res: ServerResponse,
|
res: ServerResponse,
|
||||||
@@ -348,6 +371,13 @@ export async function handleToolsInvokeHttpRequest(
|
|||||||
const result = await (tool as any).execute?.(`http-${Date.now()}`, toolArgs);
|
const result = await (tool as any).execute?.(`http-${Date.now()}`, toolArgs);
|
||||||
sendJson(res, 200, { ok: true, result });
|
sendJson(res, 200, { ok: true, result });
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
|
if (isToolInputError(err)) {
|
||||||
|
sendJson(res, 400, {
|
||||||
|
ok: false,
|
||||||
|
error: { type: "tool_error", message: getErrorMessage(err) || "invalid tool arguments" },
|
||||||
|
});
|
||||||
|
return true;
|
||||||
|
}
|
||||||
logWarn(`tools-invoke: tool execution failed: ${String(err)}`);
|
logWarn(`tools-invoke: tool execution failed: ${String(err)}`);
|
||||||
sendJson(res, 500, {
|
sendJson(res, 500, {
|
||||||
ok: false,
|
ok: false,
|
||||||
|
|||||||
Reference in New Issue
Block a user