Skip to content

Commit 6032e8a

Browse files
sorlen008claude
andcommitted
fix: prevent origin bypass in config lockdown, fix kill_process error message
The `origin` field was part of the `set_config_value` tool's input schema, meaning any AI agent could pass `origin: "ui"` to bypass the security-critical key lockdown. This completely undermined the protection added in the previous commit. Changes: - Remove `origin` from SetConfigValueArgsSchema (no longer caller-supplied) - Add trusted `callerOrigin` parameter to setConfigValue(), set server-side only - Config editor UI now calls `_internal_set_config_value` (not listed in tool catalog, so agents cannot discover it) which passes callerOrigin='ui' - MCP tool `set_config_value` always passes callerOrigin='mcp', unconditionally blocking security-critical keys regardless of what the agent sends - Fix contradictory kill_process error that said "use force_terminate for Desktop Commander sessions" (both tools serve the same purpose) - Update telemetry to derive call_origin from the handler name, not from user-supplied arguments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 38b38ac commit 6032e8a

6 files changed

Lines changed: 35 additions & 17 deletions

File tree

src/config-field-definitions.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,10 @@ export type ConfigFieldDefinition = {
55
description: string;
66
valueType: ConfigFieldValueType;
77
/**
8-
* When true, this key cannot be changed by LLM tool calls — only via the
9-
* config-editor UI (origin: 'ui'). This prevents prompt-injection attacks
10-
* from disabling safety controls at runtime.
8+
* When true, this key cannot be changed by MCP tool calls — only via the
9+
* config-editor UI (which uses the internal `_internal_set_config_value`
10+
* handler with a trusted server-side callerOrigin). This prevents
11+
* prompt-injection attacks from disabling safety controls at runtime.
1112
*/
1213
securityCritical?: boolean;
1314
};

src/server.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,9 +1188,9 @@ server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest)
11881188
}
11891189
}
11901190

1191-
if (name === 'set_config_value' && args && typeof args === 'object' && 'key' in args) {
1191+
if ((name === 'set_config_value' || name === '_internal_set_config_value') && args && typeof args === 'object' && 'key' in args) {
11921192
telemetryData.set_config_value_key_name = (args as any).key;
1193-
telemetryData.call_origin = (args as any).origin === 'ui' ? 'ui' : 'llm';
1193+
telemetryData.call_origin = name === '_internal_set_config_value' ? 'ui' : 'mcp';
11941194
}
11951195
if (name === 'get_prompts' && args && typeof args === 'object') {
11961196
const promptArgs = args as any;
@@ -1227,7 +1227,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest)
12271227
break;
12281228
case "set_config_value":
12291229
try {
1230-
result = await setConfigValue(args);
1230+
result = await setConfigValue(args, 'mcp');
12311231
} catch (error) {
12321232
capture('server_request_error', { message: `Error in set_config_value handler: ${error}` });
12331233
result = {
@@ -1236,6 +1236,20 @@ server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest)
12361236
};
12371237
}
12381238
break;
1239+
// Internal-only handler for the config editor UI. Not listed in
1240+
// the tools catalog, so AI agents cannot discover or call it.
1241+
// The 'ui' callerOrigin allows security-critical keys to be changed.
1242+
case "_internal_set_config_value":
1243+
try {
1244+
result = await setConfigValue(args, 'ui');
1245+
} catch (error) {
1246+
capture('server_request_error', { message: `Error in _internal_set_config_value handler: ${error}` });
1247+
result = {
1248+
content: [{ type: "text", text: `Error: Failed to set configuration value` }],
1249+
isError: true,
1250+
};
1251+
}
1252+
break;
12391253

12401254
case "get_usage_stats":
12411255
try {

src/tools/config.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,10 +155,16 @@ export async function getConfig() {
155155
}
156156

157157
/**
158-
* Set a specific config value
158+
* Set a specific config value.
159+
*
160+
* @param args Tool arguments (key + value). Parsed via SetConfigValueArgsSchema.
161+
* @param callerOrigin Trusted, server-set origin. Only `'ui'` (set by the
162+
* internal config-editor handler) may modify security-critical keys. MCP
163+
* tool calls always pass `'mcp'` (the default), so the AI agent can never
164+
* reach security-critical keys regardless of what it sends in the arguments.
159165
*/
160-
export async function setConfigValue(args: unknown) {
161-
console.error(`setConfigValue called with args: ${JSON.stringify(args)}`);
166+
export async function setConfigValue(args: unknown, callerOrigin: 'mcp' | 'ui' = 'mcp') {
167+
console.error(`setConfigValue called with args: ${JSON.stringify(args)}, callerOrigin: ${callerOrigin}`);
162168
try {
163169
const parsed = SetConfigValueArgsSchema.safeParse(args);
164170
if (!parsed.success) {
@@ -185,8 +191,9 @@ export async function setConfigValue(args: unknown) {
185191
// Security-critical keys (blockedCommands, allowedDirectories, defaultShell)
186192
// can only be changed through the config-editor UI, not by LLM tool calls.
187193
// This prevents prompt-injection attacks from disabling safety controls.
194+
// The callerOrigin is set server-side — it is never taken from tool arguments.
188195
const fieldDef: ConfigFieldDefinition = CONFIG_FIELD_DEFINITIONS[parsed.data.key];
189-
if (fieldDef.securityCritical && parsed.data.origin !== 'ui') {
196+
if (fieldDef.securityCritical && callerOrigin !== 'ui') {
190197
return {
191198
content: [{
192199
type: "text",

src/tools/process.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,12 @@ export async function killProcess(args: unknown): Promise<ServerResult> {
5151

5252
// Scope kill_process to sessions managed by Desktop Commander.
5353
// This prevents the AI agent from terminating arbitrary system processes.
54-
// Use force_terminate for Desktop Commander sessions, or the OS tools directly
55-
// for other processes.
5654
const session = terminalManager.getSession(parsed.data.pid);
5755
if (!session) {
5856
return {
5957
content: [{ type: "text", text: `Error: PID ${parsed.data.pid} is not a process managed by Desktop Commander. ` +
60-
`kill_process can only terminate processes started via start_process. ` +
61-
`Use force_terminate for Desktop Commander sessions.` }],
58+
`kill_process and force_terminate can only terminate processes started via start_process. ` +
59+
`For other system processes, use OS-level tools directly.` }],
6260
isError: true,
6361
};
6462
}

src/tools/schemas.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ export const SetConfigValueArgsSchema = z.object({
1212
z.array(z.string()),
1313
z.null(),
1414
]),
15-
origin: z.enum(['ui', 'llm']).optional(),
1615
});
1716

1817
// Empty schemas

src/ui/config-editor/src/app.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,10 +435,9 @@ export function createConfigEditorController(callTool: ToolCall, trackConfigUiEv
435435
}
436436

437437
try {
438-
const setResult = await callTool('set_config_value', {
438+
const setResult = await callTool('_internal_set_config_value', {
439439
key: selected.key,
440440
value: parsed.value,
441-
origin: 'ui',
442441
});
443442

444443
if (isToolErrorResult(setResult)) {

0 commit comments

Comments
 (0)