Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions assistant/src/__tests__/host-shell-tool.test.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Test coverage does not exercise targetClientId with unavailable proxy

The test file at assistant/src/__tests__/host-shell-tool.test.ts covers proxy delegation when proxy is available (line 743+), input validation before proxying (lines 770+, 789+), and local fallback when proxy is unavailable (line 810+). However, there is no test for the scenario where target_client_id is specified but the proxy is not available — which is the gap that enables the bug. Adding such a test would catch the silent local execution.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ mock.module("../util/logger.js", () => ({
// Mock the host-bash-proxy singleton so proxy delegation tests can control it.
let mockProxyAvailable = false;
let mockProxyRequestFn: (
input: { command: string; working_dir?: string; timeout_seconds?: number; env?: Record<string, string> },
input: { command: string; working_dir?: string; timeout_seconds?: number; env?: Record<string, string>; targetClientId?: string },
conversationId: string,
signal?: AbortSignal,
) => Promise<ToolExecutionResult> = () => Promise.resolve({ content: "", isError: false });
Expand Down Expand Up @@ -276,12 +276,13 @@ describe("host_bash — regression: no proxied-mode additions", () => {
expect(schemaProps).not.toHaveProperty("credential_ids");
});

test("schema only contains the expected properties (command, working_dir, timeout_seconds, activity, background)", () => {
test("schema only contains the expected properties (command, working_dir, timeout_seconds, activity, background, target_client_id)", () => {
const propertyNames = Object.keys(schemaProps).sort();
expect(propertyNames).toEqual([
"activity",
"background",
"command",
"target_client_id",
"timeout_seconds",
"working_dir",
]);
Expand Down Expand Up @@ -727,6 +728,7 @@ describe("host_bash — proxy delegation", () => {
working_dir?: string;
timeout_seconds?: number;
env?: Record<string, string>;
targetClientId?: string;
};
conversationId: string;
}> = [];
Expand Down
43 changes: 43 additions & 0 deletions assistant/src/tools/host-terminal/host-shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ import { existsSync } from "node:fs";
import { homedir } from "node:os";
import { isAbsolute } from "node:path";

import { supportsHostProxy } from "../../channels/types.js";
import { getConfig } from "../../config/loader.js";
import { isCesShellLockdownEnabled } from "../../credential-execution/feature-gates.js";
import { HostBashProxy } from "../../daemon/host-bash-proxy.js";
import { RiskLevel } from "../../permissions/types.js";
import type { ToolDefinition } from "../../providers/types.js";
import { isUntrustedTrustClass } from "../../runtime/actor-trust-resolver.js";
import { wakeAgentForOpportunity } from "../../runtime/agent-wake.js";
import { assistantEventHub } from "../../runtime/assistant-event-hub.js";
import { redactSecrets } from "../../security/secret-scanner.js";
import { getLogger } from "../../util/logger.js";
import {
Expand Down Expand Up @@ -131,6 +133,11 @@ class HostShellTool implements Tool {
description:
"Run the command in the background on the host machine. The tool returns immediately with a background tool ID. When the process exits, its output is delivered to the conversation as a wake.",
},
target_client_id: {
type: "string",
description:
"ID of the specific client to execute this command on. Required when multiple clients support host_bash; omit when only one client is connected. Obtain IDs from `assistant clients list --capability host_bash`.",
},
},
required: ["command", "activity"],
},
Expand Down Expand Up @@ -173,6 +180,11 @@ class HostShellTool implements Tool {
}
const background = input.background === true;

const targetClientId =
typeof input.target_client_id === "string"
? input.target_client_id
: undefined;

const config = getConfig();
const { shellDefaultTimeoutSec, shellMaxTimeoutSec } = config.timeouts;

Expand All @@ -190,6 +202,35 @@ class HostShellTool implements Tool {
isCesShellLockdownEnabled(config) &&
isUntrustedTrustClass(context.trustClass);

// Guard: non-host-proxy interfaces need an explicit target when multiple
// capable clients are connected to avoid ambiguous untargeted broadcasts.
const transportInterface = context.transportInterface;
if (
targetClientId == null &&
transportInterface != null &&
!supportsHostProxy(transportInterface) &&
assistantEventHub.listClientsByCapability("host_bash").length > 1
) {
return {
content: `Error: multiple clients support host_bash. Specify which client to use with \`target_client_id\`. Run \`assistant clients list --capability host_bash\` to see client IDs and labels.`,
isError: true,
};
}

// Guard: non-host-proxy interfaces with no capable clients connected.
if (
targetClientId == null &&
transportInterface != null &&
!supportsHostProxy(transportInterface) &&
!HostBashProxy.instance.isAvailable()
Comment on lines +222 to +225
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject targeted host_bash without a connected proxy client

The new non-desktop guard only fires when target_client_id is absent, so a request from a non-host-proxy interface that does include target_client_id skips this check; if no host-bash client is connected, HostBashProxy.instance.isAvailable() is false and execution falls through to the local spawn path instead of returning the intended “no client connected” error. In practice this means a caller can ask for a specific remote client and still run the command on the daemon host/container, which is a wrong-target execution regression introduced by exposing target_client_id in the schema.

Useful? React with 👍 / 👎.

) {
return {
content:
"Error: no client with host_bash capability is connected. Connect a macOS client to use host_bash from a non-desktop interface.",
isError: true,
};
}

// Proxy to connected client for execution on the user's machine
// when a capable client is available (managed/cloud-hosted mode).
if (HostBashProxy.instance.isAvailable()) {
Comment on lines 234 to 236
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Explicit target_client_id silently falls through to local execution when no proxy client is connected

When a user specifies target_client_id but no host_bash-capable proxy client is connected, the command silently runs on the local/daemon machine instead of returning an error. Both guard checks at assistant/src/tools/host-terminal/host-shell.ts:208-218 and assistant/src/tools/host-terminal/host-shell.ts:221-232 require targetClientId == null to fire, so they're skipped when a target is provided. Then the HostBashProxy.instance.isAvailable() check at line 236 returns false (no clients), and execution falls through to the local spawn("bash", ...) path at line 369, where targetClientId is completely ignored. This means a command explicitly intended for a specific remote client runs unsandboxed on the daemon host — potentially the wrong machine with different permissions and data.

Prompt for agents
In assistant/src/tools/host-terminal/host-shell.ts, after the two existing guards (lines 208-232) and before the HostBashProxy.instance.isAvailable() check at line 236, add a third guard that catches the case where targetClientId is explicitly set but the proxy is not available (or the proxy is available but the specific target client isn't connected). When targetClientId is non-null and the proxy is unavailable, the function should return an error result like: 'Error: target client "<id>" is not connected or does not support host_bash.' rather than falling through to local execution. This ensures commands with an explicit target_client_id never silently run on the wrong machine. The HostBashProxy.request() method at host-bash-proxy.ts:87-94 already validates the target when the proxy IS available, so the new guard only needs to cover the case where isAvailable() returns false.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down Expand Up @@ -227,6 +268,7 @@ class HostShellTool implements Tool {
working_dir: rawWorkingDir as string | undefined,
timeout_seconds: normalizedTimeout,
env: proxyEnv,
targetClientId,
},
context.conversationId,
abortController.signal,
Expand Down Expand Up @@ -273,6 +315,7 @@ class HostShellTool implements Tool {
working_dir: rawWorkingDir as string | undefined,
timeout_seconds: normalizedTimeout,
env: proxyEnv,
targetClientId,
},
context.conversationId,
context.signal,
Expand Down
Loading