From ecdbd8aa523d25f5da41d3984bfca72612628a95 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:12:22 +0000 Subject: [PATCH 01/24] fix(security): restrict leaf subagent control scope --- CHANGELOG.md | 1 + .../openclaw-tools.subagents.scope.test.ts | 152 ++++++++++++++++++ ...agents.sessions-spawn-depth-limits.test.ts | 5 + .../openclaw-tools.subagents.test-harness.ts | 5 + src/agents/pi-tools.policy.test.ts | 4 +- src/agents/pi-tools.policy.ts | 11 +- src/agents/tools/subagents-tool.ts | 70 +++++--- 7 files changed, 217 insertions(+), 31 deletions(-) create mode 100644 src/agents/openclaw-tools.subagents.scope.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index af544e1f6ff..b1b827ae6db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,7 @@ Docs: https://docs.openclaw.ai - Docs/Changelog: correct the contributor credit for the bundled Control UI global-install fix to @LarytheLord. (#40420) Thanks @velvet-shark. - Telegram/media downloads: time out only stalled body reads so polling recovers from hung file downloads without aborting slow downloads that are still streaming data. (#40098) thanks @tysoncung. - Docker/runtime image: prune dev dependencies, strip build-only dist metadata for smaller Docker images. (#40307) Thanks @vincentkoc. +- Subagents/sandboxing: restrict leaf subagents to their own spawned runs and remove leaf `subagents` control access so sandboxed leaf workers can no longer steer sibling sessions. Thanks @tdjackey. - Gateway/restart timeout recovery: exit non-zero when restart-triggered shutdown drains time out so launchd/systemd restart the gateway instead of treating the failed restart as a clean stop. Landed from contributor PR #40380 by @dsantoreis. Thanks @dsantoreis. - Gateway/config restart guard: validate config before service start/restart and keep post-SIGUSR1 startup failures from crashing the gateway process, reducing invalid-config restart loops and macOS permission loss. Landed from contributor PR #38699 by @lml2468. Thanks @lml2468. - Gateway/launchd respawn detection: treat `XPC_SERVICE_NAME` as a launchd supervision hint so macOS restarts exit cleanly under launchd instead of attempting detached self-respawn. Landed from contributor PR #20555 by @dimat. Thanks @dimat. diff --git a/src/agents/openclaw-tools.subagents.scope.test.ts b/src/agents/openclaw-tools.subagents.scope.test.ts new file mode 100644 index 00000000000..c58708ee6f4 --- /dev/null +++ b/src/agents/openclaw-tools.subagents.scope.test.ts @@ -0,0 +1,152 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { beforeEach, describe, expect, it } from "vitest"; +import { + callGatewayMock, + resetSubagentsConfigOverride, + setSubagentsConfigOverride, +} from "./openclaw-tools.subagents.test-harness.js"; +import { addSubagentRunForTests, resetSubagentRegistryForTests } from "./subagent-registry.js"; +import "./test-helpers/fast-core-tools.js"; +import { createPerSenderSessionConfig } from "./test-helpers/session-config.js"; +import { createSubagentsTool } from "./tools/subagents-tool.js"; + +function writeStore(storePath: string, store: Record) { + fs.mkdirSync(path.dirname(storePath), { recursive: true }); + fs.writeFileSync(storePath, JSON.stringify(store, null, 2), "utf-8"); +} + +describe("openclaw-tools: subagents scope isolation", () => { + let storePath = ""; + + beforeEach(() => { + resetSubagentRegistryForTests(); + resetSubagentsConfigOverride(); + callGatewayMock.mockReset(); + storePath = path.join( + os.tmpdir(), + `openclaw-subagents-scope-${Date.now()}-${Math.random().toString(16).slice(2)}.json`, + ); + setSubagentsConfigOverride({ + session: createPerSenderSessionConfig({ store: storePath }), + }); + writeStore(storePath, {}); + }); + + it("leaf subagents do not inherit parent sibling control scope", async () => { + const leafKey = "agent:main:subagent:leaf"; + const siblingKey = "agent:main:subagent:unsandboxed"; + + writeStore(storePath, { + [leafKey]: { + sessionId: "leaf-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + [siblingKey]: { + sessionId: "sibling-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + }); + + addSubagentRunForTests({ + runId: "run-leaf", + childSessionKey: leafKey, + requesterSessionKey: "agent:main:main", + requesterDisplayKey: "main", + task: "sandboxed leaf", + cleanup: "keep", + createdAt: Date.now() - 30_000, + startedAt: Date.now() - 30_000, + }); + addSubagentRunForTests({ + runId: "run-sibling", + childSessionKey: siblingKey, + requesterSessionKey: "agent:main:main", + requesterDisplayKey: "main", + task: "unsandboxed sibling", + cleanup: "keep", + createdAt: Date.now() - 20_000, + startedAt: Date.now() - 20_000, + }); + + const tool = createSubagentsTool({ agentSessionKey: leafKey }); + const result = await tool.execute("call-leaf-list", { action: "list" }); + + expect(result.details).toMatchObject({ + status: "ok", + requesterSessionKey: leafKey, + callerSessionKey: leafKey, + callerIsSubagent: true, + total: 0, + active: [], + recent: [], + }); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("orchestrator subagents still see children they spawned", async () => { + const orchestratorKey = "agent:main:subagent:orchestrator"; + const workerKey = `${orchestratorKey}:subagent:worker`; + const siblingKey = "agent:main:subagent:sibling"; + + writeStore(storePath, { + [orchestratorKey]: { + sessionId: "orchestrator-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + [workerKey]: { + sessionId: "worker-session", + updatedAt: Date.now(), + spawnedBy: orchestratorKey, + }, + [siblingKey]: { + sessionId: "sibling-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + }); + + addSubagentRunForTests({ + runId: "run-worker", + childSessionKey: workerKey, + requesterSessionKey: orchestratorKey, + requesterDisplayKey: orchestratorKey, + task: "worker child", + cleanup: "keep", + createdAt: Date.now() - 30_000, + startedAt: Date.now() - 30_000, + }); + addSubagentRunForTests({ + runId: "run-sibling", + childSessionKey: siblingKey, + requesterSessionKey: "agent:main:main", + requesterDisplayKey: "main", + task: "sibling of orchestrator", + cleanup: "keep", + createdAt: Date.now() - 20_000, + startedAt: Date.now() - 20_000, + }); + + const tool = createSubagentsTool({ agentSessionKey: orchestratorKey }); + const result = await tool.execute("call-orchestrator-list", { action: "list" }); + const details = result.details as { + status?: string; + requesterSessionKey?: string; + total?: number; + active?: Array<{ sessionKey?: string }>; + }; + + expect(details.status).toBe("ok"); + expect(details.requesterSessionKey).toBe(orchestratorKey); + expect(details.total).toBe(1); + expect(details.active).toEqual([ + expect.objectContaining({ + sessionKey: workerKey, + }), + ]); + }); +}); diff --git a/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts b/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts index 7a5b93d7ae1..69a1a913b2c 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts @@ -6,6 +6,11 @@ import { addSubagentRunForTests, resetSubagentRegistryForTests } from "./subagen import { createPerSenderSessionConfig } from "./test-helpers/session-config.js"; import { createSessionsSpawnTool } from "./tools/sessions-spawn-tool.js"; +vi.mock("@mariozechner/pi-ai/oauth", () => ({ + getOAuthApiKey: () => undefined, + getOAuthProviders: () => [], +})); + const callGatewayMock = vi.fn(); vi.mock("../gateway/call.js", () => ({ diff --git a/src/agents/openclaw-tools.subagents.test-harness.ts b/src/agents/openclaw-tools.subagents.test-harness.ts index 44b6ea79118..36f00e22961 100644 --- a/src/agents/openclaw-tools.subagents.test-harness.ts +++ b/src/agents/openclaw-tools.subagents.test-harness.ts @@ -5,6 +5,11 @@ export type LoadedConfig = ReturnType<(typeof import("../config/config.js"))["lo export const callGatewayMock: MockFn = vi.fn(); +vi.mock("@mariozechner/pi-ai/oauth", () => ({ + getOAuthApiKey: () => undefined, + getOAuthProviders: () => [], +})); + const defaultConfig: LoadedConfig = { session: { mainKey: "main", diff --git a/src/agents/pi-tools.policy.test.ts b/src/agents/pi-tools.policy.test.ts index 0cdc572c448..ac31ca18694 100644 --- a/src/agents/pi-tools.policy.test.ts +++ b/src/agents/pi-tools.policy.test.ts @@ -144,9 +144,9 @@ describe("resolveSubagentToolPolicy depth awareness", () => { expect(isToolAllowedByPolicyName("sessions_spawn", policy)).toBe(false); }); - it("depth-2 leaf allows subagents (for visibility)", () => { + it("depth-2 leaf denies subagents", () => { const policy = resolveSubagentToolPolicy(baseCfg, 2); - expect(isToolAllowedByPolicyName("subagents", policy)).toBe(true); + expect(isToolAllowedByPolicyName("subagents", policy)).toBe(false); }); it("depth-2 leaf denies sessions_list and sessions_history", () => { diff --git a/src/agents/pi-tools.policy.ts b/src/agents/pi-tools.policy.ts index 61d037dd9f3..d5cf592428e 100644 --- a/src/agents/pi-tools.policy.ts +++ b/src/agents/pi-tools.policy.ts @@ -64,15 +64,20 @@ const SUBAGENT_TOOL_DENY_ALWAYS = [ * Additional tools denied for leaf sub-agents (depth >= maxSpawnDepth). * These are tools that only make sense for orchestrator sub-agents that can spawn children. */ -const SUBAGENT_TOOL_DENY_LEAF = ["sessions_list", "sessions_history", "sessions_spawn"]; +const SUBAGENT_TOOL_DENY_LEAF = [ + "subagents", + "sessions_list", + "sessions_history", + "sessions_spawn", +]; /** * Build the deny list for a sub-agent at a given depth. * * - Depth 1 with maxSpawnDepth >= 2 (orchestrator): allowed to use sessions_spawn, * subagents, sessions_list, sessions_history so it can manage its children. - * - Depth >= maxSpawnDepth (leaf): denied sessions_spawn and - * session management tools. Still allowed subagents (for list/status visibility). + * - Depth >= maxSpawnDepth (leaf): denied subagents, sessions_spawn, and + * session management tools. */ function resolveSubagentDenyList(depth: number, maxSpawnDepth: number): string[] { const isLeaf = depth >= Math.max(1, Math.floor(maxSpawnDepth)); diff --git a/src/agents/tools/subagents-tool.ts b/src/agents/tools/subagents-tool.ts index f2b073934ab..8735bc8809c 100644 --- a/src/agents/tools/subagents-tool.ts +++ b/src/agents/tools/subagents-tool.ts @@ -7,7 +7,6 @@ import { sortSubagentRuns, type SubagentTargetResolution, } from "../../auto-reply/reply/subagents-utils.js"; -import { DEFAULT_SUBAGENT_MAX_SPAWN_DEPTH } from "../../config/agent-limits.js"; import { loadConfig } from "../../config/config.js"; import type { SessionEntry } from "../../config/sessions.js"; import { loadSessionStore, resolveStorePath, updateSessionStore } from "../../config/sessions.js"; @@ -28,7 +27,6 @@ import { INTERNAL_MESSAGE_CHANNEL } from "../../utils/message-channel.js"; import { AGENT_LANE_SUBAGENT } from "../lanes.js"; import { abortEmbeddedPiRun } from "../pi-embedded.js"; import { optionalStringEnum } from "../schema/typebox.js"; -import { getSubagentDepthFromSessionStore } from "../subagent-depth.js"; import { clearSubagentRunSteerRestart, countPendingDescendantRuns, @@ -204,36 +202,28 @@ function resolveRequesterKey(params: { }; } - // Check if this sub-agent can spawn children (orchestrator). - // If so, it should see its own children, not its parent's children. - const callerDepth = getSubagentDepthFromSessionStore(callerSessionKey, { cfg: params.cfg }); - const maxSpawnDepth = - params.cfg.agents?.defaults?.subagents?.maxSpawnDepth ?? DEFAULT_SUBAGENT_MAX_SPAWN_DEPTH; - if (callerDepth < maxSpawnDepth) { - // Orchestrator sub-agent: use its own session key as requester - // so it sees children it spawned. - return { - requesterSessionKey: callerSessionKey, - callerSessionKey, - callerIsSubagent: true, - }; - } - - // Leaf sub-agent: walk up to its parent so it can see sibling runs. - const cache = new Map>(); - const callerEntry = resolveSessionEntryForKey({ - cfg: params.cfg, - key: callerSessionKey, - cache, - }).entry; - const spawnedBy = typeof callerEntry?.spawnedBy === "string" ? callerEntry.spawnedBy.trim() : ""; return { - requesterSessionKey: spawnedBy || callerSessionKey, + // Subagents can only control runs spawned from their own session key. + // Announce routing still uses SubagentRunRecord.requesterSessionKey elsewhere. + requesterSessionKey: callerSessionKey, callerSessionKey, callerIsSubagent: true, }; } +function ensureSubagentControlsOwnDescendants(params: { + requester: ResolvedRequesterKey; + entry: SubagentRunRecord; +}) { + if (!params.requester.callerIsSubagent) { + return undefined; + } + if (params.entry.requesterSessionKey === params.requester.callerSessionKey) { + return undefined; + } + return "Subagents can only control runs spawned from their own session."; +} + async function killSubagentRun(params: { cfg: ReturnType; entry: SubagentRunRecord; @@ -499,6 +489,20 @@ export function createSubagentsTool(opts?: { agentSessionKey?: string }): AnyAge error: resolved.error ?? "Unknown subagent target.", }); } + const ownershipError = ensureSubagentControlsOwnDescendants({ + requester, + entry: resolved.entry, + }); + if (ownershipError) { + return jsonResult({ + status: "forbidden", + action: "kill", + target, + runId: resolved.entry.runId, + sessionKey: resolved.entry.childSessionKey, + error: ownershipError, + }); + } const killCache = new Map>(); const stopResult = await killSubagentRun({ cfg, @@ -568,6 +572,20 @@ export function createSubagentsTool(opts?: { agentSessionKey?: string }): AnyAge error: resolved.error ?? "Unknown subagent target.", }); } + const ownershipError = ensureSubagentControlsOwnDescendants({ + requester, + entry: resolved.entry, + }); + if (ownershipError) { + return jsonResult({ + status: "forbidden", + action: "steer", + target, + runId: resolved.entry.runId, + sessionKey: resolved.entry.childSessionKey, + error: ownershipError, + }); + } if (resolved.entry.endedAt) { return jsonResult({ status: "done", From 702f6f3305653922548ed2f5c78228ef3c3573e7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:13:43 +0000 Subject: [PATCH 02/24] fix: fail closed for unresolved local gateway auth refs --- CHANGELOG.md | 1 + .../daemon-cli/gateway-token-drift.test.ts | 25 ++++++++ src/cli/daemon-cli/lifecycle.test.ts | 21 ++++--- src/gateway/credentials.test.ts | 52 +++++++++++++++ src/gateway/credentials.ts | 63 ++++++++++++++----- src/gateway/probe-auth.test.ts | 28 +++++++++ 6 files changed, 163 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1b827ae6db..2e0274c5a38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,7 @@ Docs: https://docs.openclaw.ai - Browser/Browserbase 429 handling: surface stable no-retry rate-limit guidance without buffering discarded HTTP 429 response bodies from remote browser services. (#40491) thanks @mvanhorn. - Gateway/auth: allow one trusted device-token retry on shared-token mismatch with recovery hints to prevent reconnect churn during token drift. (#42507) Thanks @joshavant. - Channels/allowlists: remove stale matcher caching so same-array allowlist edits and wildcard replacements take effect immediately, with regression coverage for in-place mutation cases. +- Gateway/auth: fail closed when local `gateway.auth.*` SecretRefs are configured but unavailable, instead of silently falling back to `gateway.remote.*` credentials in local mode. Thanks @tdjackey. ## 2026.3.8 diff --git a/src/cli/daemon-cli/gateway-token-drift.test.ts b/src/cli/daemon-cli/gateway-token-drift.test.ts index ff221b24e44..0b9d0cfb308 100644 --- a/src/cli/daemon-cli/gateway-token-drift.test.ts +++ b/src/cli/daemon-cli/gateway-token-drift.test.ts @@ -43,4 +43,29 @@ describe("resolveGatewayTokenForDriftCheck", () => { }), ).toThrow(/gateway\.auth\.token/i); }); + + it("does not fall back to gateway.remote token for unresolved local token refs", () => { + expect(() => + resolveGatewayTokenForDriftCheck({ + cfg: { + secrets: { + providers: { + default: { source: "env" }, + }, + }, + gateway: { + mode: "local", + auth: { + mode: "token", + token: { source: "env", provider: "default", id: "MISSING_LOCAL_TOKEN" }, + }, + remote: { + token: "remote-token", + }, + }, + } as OpenClawConfig, + env: {} as NodeJS.ProcessEnv, + }), + ).toThrow(/gateway\.auth\.token/i); + }); }); diff --git a/src/cli/daemon-cli/lifecycle.test.ts b/src/cli/daemon-cli/lifecycle.test.ts index f1e87fc4938..3f0ed6d531c 100644 --- a/src/cli/daemon-cli/lifecycle.test.ts +++ b/src/cli/daemon-cli/lifecycle.test.ts @@ -36,16 +36,17 @@ const renderGatewayPortHealthDiagnostics = vi.fn(() => ["diag: unhealthy port"]) const renderRestartDiagnostics = vi.fn(() => ["diag: unhealthy runtime"]); const resolveGatewayPort = vi.fn(() => 18789); const findGatewayPidsOnPortSync = vi.fn<(port: number) => number[]>(() => []); -const probeGateway = vi.fn< - (opts: { - url: string; - auth?: { token?: string; password?: string }; - timeoutMs: number; - }) => Promise<{ - ok: boolean; - configSnapshot: unknown; - }> ->(); +const probeGateway = + vi.fn< + (opts: { + url: string; + auth?: { token?: string; password?: string }; + timeoutMs: number; + }) => Promise<{ + ok: boolean; + configSnapshot: unknown; + }> + >(); const isRestartEnabled = vi.fn<(config?: { commands?: unknown }) => boolean>(() => true); const loadConfig = vi.fn(() => ({})); diff --git a/src/gateway/credentials.test.ts b/src/gateway/credentials.test.ts index 5a6ea041c92..2ec45139d09 100644 --- a/src/gateway/credentials.test.ts +++ b/src/gateway/credentials.test.ts @@ -222,6 +222,58 @@ describe("resolveGatewayCredentialsFromConfig", () => { ).toThrow("gateway.auth.token"); }); + it("throws when unresolved local token SecretRef would otherwise fall back to remote token", () => { + expect(() => + resolveGatewayCredentialsFromConfig({ + cfg: { + gateway: { + mode: "local", + auth: { + mode: "token", + token: { source: "env", provider: "default", id: "MISSING_LOCAL_TOKEN" }, + }, + remote: { + token: "remote-token", + }, + }, + secrets: { + providers: { + default: { source: "env" }, + }, + }, + } as unknown as OpenClawConfig, + env: {} as NodeJS.ProcessEnv, + includeLegacyEnv: false, + }), + ).toThrow("gateway.auth.token"); + }); + + it("throws when unresolved local password SecretRef would otherwise fall back to remote password", () => { + expect(() => + resolveGatewayCredentialsFromConfig({ + cfg: { + gateway: { + mode: "local", + auth: { + mode: "password", + password: { source: "env", provider: "default", id: "MISSING_LOCAL_PASSWORD" }, + }, + remote: { + password: "remote-password", // pragma: allowlist secret + }, + }, + secrets: { + providers: { + default: { source: "env" }, + }, + }, + } as unknown as OpenClawConfig, + env: {} as NodeJS.ProcessEnv, + includeLegacyEnv: false, + }), + ).toThrow("gateway.auth.password"); + }); + it("ignores unresolved local password ref when local auth mode is none", () => { const resolved = resolveLocalModeWithUnresolvedPassword("none"); expect(resolved).toEqual({ diff --git a/src/gateway/credentials.ts b/src/gateway/credentials.ts index 0e9a7c1e07d..d5c3c6037a2 100644 --- a/src/gateway/credentials.ts +++ b/src/gateway/credentials.ts @@ -1,6 +1,6 @@ import type { OpenClawConfig } from "../config/config.js"; import { containsEnvVarReference } from "../config/env-substitution.js"; -import { resolveSecretInputRef } from "../config/types.secrets.js"; +import { hasConfiguredSecretInput, resolveSecretInputRef } from "../config/types.secrets.js"; export type ExplicitGatewayAuth = { token?: string; @@ -16,6 +16,13 @@ export type GatewayCredentialMode = "local" | "remote"; export type GatewayCredentialPrecedence = "env-first" | "config-first"; export type GatewayRemoteCredentialPrecedence = "remote-first" | "env-first"; export type GatewayRemoteCredentialFallback = "remote-env-local" | "remote-only"; +type GatewaySecretDefaults = NonNullable["defaults"]; + +type GatewayConfiguredCredentialInput = { + configured: boolean; + value?: string; + refPath?: string; +}; const GATEWAY_SECRET_REF_UNAVAILABLE_ERROR_CODE = "GATEWAY_SECRET_REF_UNAVAILABLE"; // pragma: allowlist secret @@ -85,6 +92,22 @@ function throwUnresolvedGatewaySecretInput(path: string): never { throw new GatewaySecretRefUnavailableError(path); } +function resolveConfiguredGatewayCredentialInput(params: { + value: unknown; + defaults?: GatewaySecretDefaults; + path: string; +}): GatewayConfiguredCredentialInput { + const ref = resolveSecretInputRef({ + value: params.value, + defaults: params.defaults, + }).ref; + return { + configured: hasConfiguredSecretInput(params.value, params.defaults), + value: ref ? undefined : trimToUndefined(params.value), + refPath: ref ? params.path : undefined, + }; +} + export function readGatewayTokenEnv( env: NodeJS.ProcessEnv = process.env, includeLegacyEnv = true, @@ -200,28 +223,34 @@ export function resolveGatewayCredentialsFromConfig(params: { const envToken = readGatewayTokenEnv(env, includeLegacyEnv); const envPassword = readGatewayPasswordEnv(env, includeLegacyEnv); - const localTokenRef = resolveSecretInputRef({ + const localTokenInput = resolveConfiguredGatewayCredentialInput({ value: params.cfg.gateway?.auth?.token, defaults, - }).ref; - const localPasswordRef = resolveSecretInputRef({ + path: "gateway.auth.token", + }); + const localPasswordInput = resolveConfiguredGatewayCredentialInput({ value: params.cfg.gateway?.auth?.password, defaults, - }).ref; - const remoteTokenRef = resolveSecretInputRef({ + path: "gateway.auth.password", + }); + const remoteTokenInput = resolveConfiguredGatewayCredentialInput({ value: remote?.token, defaults, - }).ref; - const remotePasswordRef = resolveSecretInputRef({ + path: "gateway.remote.token", + }); + const remotePasswordInput = resolveConfiguredGatewayCredentialInput({ value: remote?.password, defaults, - }).ref; - const remoteToken = remoteTokenRef ? undefined : trimToUndefined(remote?.token); - const remotePassword = remotePasswordRef ? undefined : trimToUndefined(remote?.password); - const localToken = localTokenRef ? undefined : trimToUndefined(params.cfg.gateway?.auth?.token); - const localPassword = localPasswordRef - ? undefined - : trimToUndefined(params.cfg.gateway?.auth?.password); + path: "gateway.remote.password", + }); + const localTokenRef = localTokenInput.refPath; + const localPasswordRef = localPasswordInput.refPath; + const remoteTokenRef = remoteTokenInput.refPath; + const remotePasswordRef = remotePasswordInput.refPath; + const remoteToken = remoteTokenInput.value; + const remotePassword = remotePasswordInput.value; + const localToken = localTokenInput.value; + const localPassword = localPasswordInput.value; const localTokenPrecedence = params.localTokenPrecedence ?? @@ -232,8 +261,8 @@ export function resolveGatewayCredentialsFromConfig(params: { // In local mode, prefer gateway.auth.token, but also accept gateway.remote.token // as a fallback for cron commands and other local gateway clients. // This allows users in remote mode to use a single token for all operations. - const fallbackToken = localToken ?? remoteToken; - const fallbackPassword = localPassword ?? remotePassword; + const fallbackToken = localTokenInput.configured ? localToken : remoteToken; + const fallbackPassword = localPasswordInput.configured ? localPassword : remotePassword; const localResolved = resolveGatewayCredentialsFromValues({ configToken: fallbackToken, configPassword: fallbackPassword, diff --git a/src/gateway/probe-auth.test.ts b/src/gateway/probe-auth.test.ts index e31dd4856ad..7a6d639e10a 100644 --- a/src/gateway/probe-auth.test.ts +++ b/src/gateway/probe-auth.test.ts @@ -51,6 +51,34 @@ describe("resolveGatewayProbeAuthSafe", () => { expect(result.warning).toContain("unresolved"); }); + it("does not fall through to remote token when local token SecretRef is unresolved", () => { + const result = resolveGatewayProbeAuthSafe({ + cfg: { + gateway: { + mode: "local", + auth: { + mode: "token", + token: { source: "env", provider: "default", id: "MISSING_GATEWAY_TOKEN" }, + }, + remote: { + token: "remote-token", + }, + }, + secrets: { + providers: { + default: { source: "env" }, + }, + }, + } as OpenClawConfig, + mode: "local", + env: {} as NodeJS.ProcessEnv, + }); + + expect(result.auth).toEqual({}); + expect(result.warning).toContain("gateway.auth.token"); + expect(result.warning).toContain("unresolved"); + }); + it("ignores unresolved local token SecretRef in remote mode when remote-only auth is requested", () => { const result = resolveGatewayProbeAuthSafe({ cfg: { From 11924a70264f235b2b1d47e3d32a5f0dae111bb1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:15:22 +0000 Subject: [PATCH 03/24] fix(sandbox): pin fs-bridge staged writes --- CHANGELOG.md | 1 + src/agents/sandbox/fs-bridge-path-safety.ts | 26 +++++ .../sandbox/fs-bridge-shell-command-plans.ts | 12 -- .../sandbox/fs-bridge-write-helper.test.ts | 86 ++++++++++++++ src/agents/sandbox/fs-bridge-write-helper.ts | 109 ++++++++++++++++++ src/agents/sandbox/fs-bridge.shell.test.ts | 10 +- src/agents/sandbox/fs-bridge.ts | 83 +++---------- 7 files changed, 240 insertions(+), 87 deletions(-) create mode 100644 src/agents/sandbox/fs-bridge-write-helper.test.ts create mode 100644 src/agents/sandbox/fs-bridge-write-helper.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e0274c5a38..f051950d860 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,7 @@ Docs: https://docs.openclaw.ai - Gateway/auth: allow one trusted device-token retry on shared-token mismatch with recovery hints to prevent reconnect churn during token drift. (#42507) Thanks @joshavant. - Channels/allowlists: remove stale matcher caching so same-array allowlist edits and wildcard replacements take effect immediately, with regression coverage for in-place mutation cases. - Gateway/auth: fail closed when local `gateway.auth.*` SecretRefs are configured but unavailable, instead of silently falling back to `gateway.remote.*` credentials in local mode. Thanks @tdjackey. +- Sandbox/fs bridge: pin staged writes to verified parent directories so temporary write files cannot materialize outside the allowed mount before atomic replace. Thanks @tdjackey. ## 2026.3.8 diff --git a/src/agents/sandbox/fs-bridge-path-safety.ts b/src/agents/sandbox/fs-bridge-path-safety.ts index a18ed500287..dabba7319b6 100644 --- a/src/agents/sandbox/fs-bridge-path-safety.ts +++ b/src/agents/sandbox/fs-bridge-path-safety.ts @@ -23,6 +23,12 @@ export type AnchoredSandboxEntry = { basename: string; }; +export type PinnedSandboxWriteEntry = { + mountRootPath: string; + relativeParentPath: string; + basename: string; +}; + type RunCommand = ( script: string, options?: { @@ -144,6 +150,26 @@ export class SandboxFsPathGuard { }; } + resolvePinnedWriteEntry(target: SandboxResolvedFsPath, action: string): PinnedSandboxWriteEntry { + const basename = path.posix.basename(target.containerPath); + if (!basename || basename === "." || basename === "/") { + throw new Error(`Invalid sandbox entry target: ${target.containerPath}`); + } + const parentPath = normalizeContainerPath(path.posix.dirname(target.containerPath)); + const mount = this.resolveRequiredMount(parentPath, action); + const relativeParentPath = path.posix.relative(mount.containerRoot, parentPath); + if (relativeParentPath.startsWith("..") || path.posix.isAbsolute(relativeParentPath)) { + throw new Error( + `Sandbox path escapes allowed mounts; cannot ${action}: ${target.containerPath}`, + ); + } + return { + mountRootPath: mount.containerRoot, + relativeParentPath: relativeParentPath === "." ? "" : relativeParentPath, + basename, + }; + } + private pathIsExistingDirectory(hostPath: string): boolean { try { return fs.statSync(hostPath).isDirectory(); diff --git a/src/agents/sandbox/fs-bridge-shell-command-plans.ts b/src/agents/sandbox/fs-bridge-shell-command-plans.ts index 4c1a9b8d64f..e821f6cea7a 100644 --- a/src/agents/sandbox/fs-bridge-shell-command-plans.ts +++ b/src/agents/sandbox/fs-bridge-shell-command-plans.ts @@ -10,18 +10,6 @@ export type SandboxFsCommandPlan = { allowFailure?: boolean; }; -export function buildWriteCommitPlan( - target: SandboxResolvedFsPath, - tempPath: string, -): SandboxFsCommandPlan { - return { - checks: [{ target, options: { action: "write files", requireWritable: true } }], - recheckBeforeCommand: true, - script: 'set -eu; mv -f -- "$1" "$2"', - args: [tempPath, target.containerPath], - }; -} - export function buildMkdirpPlan( target: SandboxResolvedFsPath, anchoredTarget: AnchoredSandboxEntry, diff --git a/src/agents/sandbox/fs-bridge-write-helper.test.ts b/src/agents/sandbox/fs-bridge-write-helper.test.ts new file mode 100644 index 00000000000..75da7046573 --- /dev/null +++ b/src/agents/sandbox/fs-bridge-write-helper.test.ts @@ -0,0 +1,86 @@ +import { spawnSync } from "node:child_process"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { SANDBOX_PINNED_WRITE_PYTHON } from "./fs-bridge-write-helper.js"; + +async function withTempRoot(prefix: string, run: (root: string) => Promise): Promise { + const root = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + try { + return await run(root); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } +} + +function runPinnedWrite(params: { + mountRoot: string; + relativeParentPath: string; + basename: string; + mkdir: boolean; + input: string; +}) { + return spawnSync( + "python3", + [ + "-c", + SANDBOX_PINNED_WRITE_PYTHON, + params.mountRoot, + params.relativeParentPath, + params.basename, + params.mkdir ? "1" : "0", + ], + { + input: params.input, + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + }, + ); +} + +describe("sandbox pinned write helper", () => { + it("creates missing parents and writes through a pinned directory fd", async () => { + await withTempRoot("openclaw-write-helper-", async (root) => { + const workspace = path.join(root, "workspace"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runPinnedWrite({ + mountRoot: workspace, + relativeParentPath: "nested/deeper", + basename: "note.txt", + mkdir: true, + input: "hello", + }); + + expect(result.status).toBe(0); + await expect( + fs.readFile(path.join(workspace, "nested", "deeper", "note.txt"), "utf8"), + ).resolves.toBe("hello"); + }); + }); + + it.runIf(process.platform !== "win32")( + "rejects symlink-parent writes instead of materializing a temp file outside the mount", + async () => { + await withTempRoot("openclaw-write-helper-", async (root) => { + const workspace = path.join(root, "workspace"); + const outside = path.join(root, "outside"); + await fs.mkdir(workspace, { recursive: true }); + await fs.mkdir(outside, { recursive: true }); + await fs.symlink(outside, path.join(workspace, "alias")); + + const result = runPinnedWrite({ + mountRoot: workspace, + relativeParentPath: "alias", + basename: "escape.txt", + mkdir: false, + input: "owned", + }); + + expect(result.status).not.toBe(0); + await expect(fs.readFile(path.join(outside, "escape.txt"), "utf8")).rejects.toThrow(); + }); + }, + ); +}); diff --git a/src/agents/sandbox/fs-bridge-write-helper.ts b/src/agents/sandbox/fs-bridge-write-helper.ts new file mode 100644 index 00000000000..a8a30388799 --- /dev/null +++ b/src/agents/sandbox/fs-bridge-write-helper.ts @@ -0,0 +1,109 @@ +import type { PathSafetyCheck, PinnedSandboxWriteEntry } from "./fs-bridge-path-safety.js"; +import type { SandboxFsCommandPlan } from "./fs-bridge-shell-command-plans.js"; + +export const SANDBOX_PINNED_WRITE_PYTHON = [ + "import errno", + "import os", + "import secrets", + "import sys", + "", + "mount_root = sys.argv[1]", + "relative_parent = sys.argv[2]", + "basename = sys.argv[3]", + 'mkdir_enabled = sys.argv[4] == "1"', + "", + "DIR_FLAGS = os.O_RDONLY", + "if hasattr(os, 'O_DIRECTORY'):", + " DIR_FLAGS |= os.O_DIRECTORY", + "if hasattr(os, 'O_NOFOLLOW'):", + " DIR_FLAGS |= os.O_NOFOLLOW", + "", + "WRITE_FLAGS = os.O_WRONLY | os.O_CREAT | os.O_EXCL", + "if hasattr(os, 'O_NOFOLLOW'):", + " WRITE_FLAGS |= os.O_NOFOLLOW", + "", + "def open_dir(path, dir_fd=None):", + " return os.open(path, DIR_FLAGS, dir_fd=dir_fd)", + "", + "def walk_parent(root_fd, rel_parent, mkdir_enabled):", + " current_fd = os.dup(root_fd)", + " try:", + " segments = [segment for segment in rel_parent.split('/') if segment and segment != '.']", + " for segment in segments:", + " if segment == '..':", + " raise OSError(errno.EPERM, 'path traversal is not allowed', segment)", + " try:", + " next_fd = open_dir(segment, dir_fd=current_fd)", + " except FileNotFoundError:", + " if not mkdir_enabled:", + " raise", + " os.mkdir(segment, 0o777, dir_fd=current_fd)", + " next_fd = open_dir(segment, dir_fd=current_fd)", + " os.close(current_fd)", + " current_fd = next_fd", + " return current_fd", + " except Exception:", + " os.close(current_fd)", + " raise", + "", + "def create_temp_file(parent_fd, basename):", + " prefix = '.openclaw-write-' + basename + '.'", + " for _ in range(128):", + " candidate = prefix + secrets.token_hex(6)", + " try:", + " fd = os.open(candidate, WRITE_FLAGS, 0o600, dir_fd=parent_fd)", + " return candidate, fd", + " except FileExistsError:", + " continue", + " raise RuntimeError('failed to allocate sandbox temp file')", + "", + "root_fd = open_dir(mount_root)", + "parent_fd = None", + "temp_fd = None", + "temp_name = None", + "try:", + " parent_fd = walk_parent(root_fd, relative_parent, mkdir_enabled)", + " temp_name, temp_fd = create_temp_file(parent_fd, basename)", + " while True:", + " chunk = sys.stdin.buffer.read(65536)", + " if not chunk:", + " break", + " os.write(temp_fd, chunk)", + " os.fsync(temp_fd)", + " os.close(temp_fd)", + " temp_fd = None", + " os.replace(temp_name, basename, src_dir_fd=parent_fd, dst_dir_fd=parent_fd)", + " os.fsync(parent_fd)", + "except Exception:", + " if temp_fd is not None:", + " os.close(temp_fd)", + " temp_fd = None", + " if temp_name is not None and parent_fd is not None:", + " try:", + " os.unlink(temp_name, dir_fd=parent_fd)", + " except FileNotFoundError:", + " pass", + " raise", + "finally:", + " if parent_fd is not None:", + " os.close(parent_fd)", + " os.close(root_fd)", +].join("\n"); + +export function buildPinnedWritePlan(params: { + check: PathSafetyCheck; + pinned: PinnedSandboxWriteEntry; + mkdir: boolean; +}): SandboxFsCommandPlan & { stdin?: Buffer | string } { + return { + checks: [params.check], + recheckBeforeCommand: true, + script: ["set -eu", "python3 - \"$@\" <<'PY'", SANDBOX_PINNED_WRITE_PYTHON, "PY"].join("\n"), + args: [ + params.pinned.mountRootPath, + params.pinned.relativeParentPath, + params.pinned.basename, + params.mkdir ? "1" : "0", + ], + }; +} diff --git a/src/agents/sandbox/fs-bridge.shell.test.ts b/src/agents/sandbox/fs-bridge.shell.test.ts index d8b29c0f5d5..5b27f210333 100644 --- a/src/agents/sandbox/fs-bridge.shell.test.ts +++ b/src/agents/sandbox/fs-bridge.shell.test.ts @@ -130,11 +130,11 @@ describe("sandbox fs bridge shell compatibility", () => { const scripts = getScriptsFromCalls(); expect(scripts.some((script) => script.includes('cat >"$1"'))).toBe(false); - expect(scripts.some((script) => script.includes('cat >"$tmp"'))).toBe(true); - expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(true); + expect(scripts.some((script) => script.includes('cat >"$tmp"'))).toBe(false); + expect(scripts.some((script) => script.includes("os.replace("))).toBe(true); }); - it("re-validates target before final rename and cleans temp file on failure", async () => { + it("re-validates target before the pinned write helper runs", async () => { const { mockedOpenBoundaryFile } = await import("./fs-bridge.test-helpers.js"); mockedOpenBoundaryFile .mockImplementationOnce(async () => ({ ok: false, reason: "path" })) @@ -150,8 +150,6 @@ describe("sandbox fs bridge shell compatibility", () => { ); const scripts = getScriptsFromCalls(); - expect(scripts.some((script) => script.includes("mktemp"))).toBe(true); - expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(false); - expect(scripts.some((script) => script.includes('rm -f -- "$1"'))).toBe(true); + expect(scripts.some((script) => script.includes("os.replace("))).toBe(false); }); }); diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index f937ad2c702..67fedf3b515 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -6,15 +6,14 @@ import { buildRemovePlan, buildRenamePlan, buildStatPlan, - buildWriteCommitPlan, type SandboxFsCommandPlan, } from "./fs-bridge-shell-command-plans.js"; +import { buildPinnedWritePlan } from "./fs-bridge-write-helper.js"; import { buildSandboxFsMounts, resolveSandboxFsPathWithMounts, type SandboxResolvedFsPath, } from "./fs-paths.js"; -import { normalizeContainerPath } from "./path-utils.js"; import type { SandboxContext, SandboxWorkspaceAccess } from "./types.js"; type RunCommandOptions = { @@ -112,26 +111,24 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "write files"); - await this.pathGuard.assertPathSafety(target, { action: "write files", requireWritable: true }); + const writeCheck = { + target, + options: { action: "write files", requireWritable: true } as const, + }; + await this.pathGuard.assertPathSafety(target, writeCheck.options); const buffer = Buffer.isBuffer(params.data) ? params.data : Buffer.from(params.data, params.encoding ?? "utf8"); - const tempPath = await this.writeFileToTempPath({ - targetContainerPath: target.containerPath, - mkdir: params.mkdir !== false, - data: buffer, + const pinnedWriteTarget = this.pathGuard.resolvePinnedWriteEntry(target, "write files"); + await this.runCheckedCommand({ + ...buildPinnedWritePlan({ + check: writeCheck, + pinned: pinnedWriteTarget, + mkdir: params.mkdir !== false, + }), + stdin: buffer, signal: params.signal, }); - - try { - await this.runCheckedCommand({ - ...buildWriteCommitPlan(target, tempPath), - signal: params.signal, - }); - } catch (error) { - await this.cleanupTempPath(tempPath, params.signal); - throw error; - } } async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { @@ -265,58 +262,6 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { return await this.runCheckedCommand({ ...plan, signal }); } - private async writeFileToTempPath(params: { - targetContainerPath: string; - mkdir: boolean; - data: Buffer; - signal?: AbortSignal; - }): Promise { - const script = params.mkdir - ? [ - "set -eu", - 'target="$1"', - 'dir=$(dirname -- "$target")', - 'if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi', - 'base=$(basename -- "$target")', - 'tmp=$(mktemp "$dir/.openclaw-write-$base.XXXXXX")', - 'cat >"$tmp"', - 'printf "%s\\n" "$tmp"', - ].join("\n") - : [ - "set -eu", - 'target="$1"', - 'dir=$(dirname -- "$target")', - 'base=$(basename -- "$target")', - 'tmp=$(mktemp "$dir/.openclaw-write-$base.XXXXXX")', - 'cat >"$tmp"', - 'printf "%s\\n" "$tmp"', - ].join("\n"); - const result = await this.runCommand(script, { - args: [params.targetContainerPath], - stdin: params.data, - signal: params.signal, - }); - const tempPath = result.stdout.toString("utf8").trim().split(/\r?\n/).at(-1)?.trim(); - if (!tempPath || !tempPath.startsWith("/")) { - throw new Error( - `Failed to create temporary sandbox write path for ${params.targetContainerPath}`, - ); - } - return normalizeContainerPath(tempPath); - } - - private async cleanupTempPath(tempPath: string, signal?: AbortSignal): Promise { - try { - await this.runCommand('set -eu; rm -f -- "$1"', { - args: [tempPath], - signal, - allowFailure: true, - }); - } catch { - // Best-effort cleanup only. - } - } - private ensureWriteAccess(target: SandboxResolvedFsPath, action: string) { if (!allowsWrites(this.sandbox.workspaceAccess) || !target.writable) { throw new Error(`Sandbox path is read-only; cannot ${action}: ${target.containerPath}`); From 8eac9394170e2218a55fab774c310ff9bcaad19f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:24:17 +0000 Subject: [PATCH 04/24] fix(security): enforce target account configWrites --- CHANGELOG.md | 1 + docs/gateway/configuration-reference.md | 1 + docs/tools/slash-commands.md | 1 + src/auto-reply/reply/commands-allowlist.ts | 24 +++-- src/auto-reply/reply/commands-config.ts | 65 +++++++----- src/auto-reply/reply/commands.test.ts | 114 +++++++++++++++++++++ src/channels/plugins/config-writes.ts | 96 +++++++++++++++++ src/channels/plugins/plugins-core.test.ts | 34 +++++- 8 files changed, 303 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f051950d860..8dcfe701e11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,7 @@ Docs: https://docs.openclaw.ai - Channels/allowlists: remove stale matcher caching so same-array allowlist edits and wildcard replacements take effect immediately, with regression coverage for in-place mutation cases. - Gateway/auth: fail closed when local `gateway.auth.*` SecretRefs are configured but unavailable, instead of silently falling back to `gateway.remote.*` credentials in local mode. Thanks @tdjackey. - Sandbox/fs bridge: pin staged writes to verified parent directories so temporary write files cannot materialize outside the allowed mount before atomic replace. Thanks @tdjackey. +- Commands/config writes: enforce `configWrites` against both the originating account and the targeted account scope for `/config` and config-backed `/allowlist` edits, blocking sibling-account mutations while preserving gateway `operator.admin` flows. Thanks @tdjackey for reporting. ## 2026.3.8 diff --git a/docs/gateway/configuration-reference.md b/docs/gateway/configuration-reference.md index 2a27470fd36..ae958788e2f 100644 --- a/docs/gateway/configuration-reference.md +++ b/docs/gateway/configuration-reference.md @@ -748,6 +748,7 @@ Include your own number in `allowFrom` to enable self-chat mode (ignores native - `bash: true` enables `! ` for host shell. Requires `tools.elevated.enabled` and sender in `tools.elevated.allowFrom.`. - `config: true` enables `/config` (reads/writes `openclaw.json`). For gateway `chat.send` clients, persistent `/config set|unset` writes also require `operator.admin`; read-only `/config show` stays available to normal write-scoped operator clients. - `channels..configWrites` gates config mutations per channel (default: true). +- For multi-account channels, `channels..accounts..configWrites` also gates writes that target that account (for example `/allowlist --config --account ` or `/config set channels..accounts....`). - `allowFrom` is per-provider. When set, it is the **only** authorization source (channel allowlists/pairing and `useAccessGroups` are ignored). - `useAccessGroups: false` allows commands to bypass access-group policies when `allowFrom` is not set. diff --git a/docs/tools/slash-commands.md b/docs/tools/slash-commands.md index dea4fb0d30f..d792398f1fa 100644 --- a/docs/tools/slash-commands.md +++ b/docs/tools/slash-commands.md @@ -123,6 +123,7 @@ Notes: - `/new ` accepts a model alias, `provider/model`, or a provider name (fuzzy match); if no match, the text is treated as the message body. - For full provider usage breakdown, use `openclaw status --usage`. - `/allowlist add|remove` requires `commands.config=true` and honors channel `configWrites`. +- In multi-account channels, config-targeted `/allowlist --account ` and `/config set channels..accounts....` also honor the target account's `configWrites`. - `/usage` controls the per-response usage footer; `/usage cost` prints a local cost summary from OpenClaw session logs. - `/restart` is enabled by default; set `commands.restart: false` to disable it. - Discord-only native command: `/vc join|leave|status` controls voice channels (requires `channels.discord.voice` and native commands; not available as text). diff --git a/src/auto-reply/reply/commands-allowlist.ts b/src/auto-reply/reply/commands-allowlist.ts index 766bb5f41b3..643d20143c6 100644 --- a/src/auto-reply/reply/commands-allowlist.ts +++ b/src/auto-reply/reply/commands-allowlist.ts @@ -1,5 +1,5 @@ import { getChannelDock } from "../../channels/dock.js"; -import { resolveChannelConfigWrites } from "../../channels/plugins/config-writes.js"; +import { authorizeConfigWrite } from "../../channels/plugins/config-writes.js"; import { listPairingChannels } from "../../channels/plugins/pairing.js"; import type { ChannelId } from "../../channels/plugins/types.js"; import { normalizeChannelId } from "../../channels/registry.js"; @@ -28,6 +28,7 @@ import { resolveSignalAccount } from "../../signal/accounts.js"; import { resolveSlackAccount } from "../../slack/accounts.js"; import { resolveSlackUserAllowlist } from "../../slack/resolve-users.js"; import { resolveTelegramAccount } from "../../telegram/accounts.js"; +import { isInternalMessageChannel } from "../../utils/message-channel.js"; import { resolveWhatsAppAccount } from "../../web/accounts.js"; import { rejectUnauthorizedCommand, requireCommandFlagEnabled } from "./command-gates.js"; import type { CommandHandler } from "./commands-types.js"; @@ -585,16 +586,25 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo const shouldTouchStore = parsed.target !== "config" && listPairingChannels().includes(channelId); if (shouldUpdateConfig) { - const allowWrites = resolveChannelConfigWrites({ + const writeAuth = authorizeConfigWrite({ cfg: params.cfg, - channelId, - accountId: params.ctx.AccountId, + origin: { channelId, accountId: params.ctx.AccountId }, + targets: [{ channelId, accountId }], + allowBypass: + isInternalMessageChannel(params.command.channel) && + params.ctx.GatewayClientScopes?.includes("operator.admin") === true, }); - if (!allowWrites) { - const hint = `channels.${channelId}.configWrites=true`; + if (!writeAuth.allowed) { + const blocked = writeAuth.blockedScope?.scope; + const hint = + blocked?.channelId && blocked.accountId + ? `channels.${blocked.channelId}.accounts.${blocked.accountId}.configWrites=true` + : `channels.${blocked?.channelId ?? channelId}.configWrites=true`; return { shouldContinue: false, - reply: { text: `⚠️ Config writes are disabled for ${channelId}. Set ${hint} to enable.` }, + reply: { + text: `⚠️ Config writes are disabled for ${blocked?.channelId ?? channelId}. Set ${hint} to enable.`, + }, }; } diff --git a/src/auto-reply/reply/commands-config.ts b/src/auto-reply/reply/commands-config.ts index 00ef8048efe..edc0c145526 100644 --- a/src/auto-reply/reply/commands-config.ts +++ b/src/auto-reply/reply/commands-config.ts @@ -1,4 +1,7 @@ -import { resolveChannelConfigWrites } from "../../channels/plugins/config-writes.js"; +import { + authorizeConfigWrite, + resolveConfigWriteScopesFromPath, +} from "../../channels/plugins/config-writes.js"; import { normalizeChannelId } from "../../channels/registry.js"; import { getConfigValueAtPath, @@ -17,6 +20,7 @@ import { setConfigOverride, unsetConfigOverride, } from "../../config/runtime-overrides.js"; +import { isInternalMessageChannel } from "../../utils/message-channel.js"; import { rejectUnauthorizedCommand, requireCommandFlagEnabled, @@ -52,6 +56,7 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma }; } + let parsedWritePath: string[] | undefined; if (configCommand.action === "set" || configCommand.action === "unset") { const missingAdminScope = requireGatewayClientScopeForInternalChannel(params, { label: "/config write", @@ -61,17 +66,41 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma if (missingAdminScope) { return missingAdminScope; } + const parsedPath = parseConfigPath(configCommand.path); + if (!parsedPath.ok || !parsedPath.path) { + return { + shouldContinue: false, + reply: { text: `⚠️ ${parsedPath.error ?? "Invalid path."}` }, + }; + } + parsedWritePath = parsedPath.path; const channelId = params.command.channelId ?? normalizeChannelId(params.command.channel); - const allowWrites = resolveChannelConfigWrites({ + const writeAuth = authorizeConfigWrite({ cfg: params.cfg, - channelId, - accountId: params.ctx.AccountId, + origin: { channelId, accountId: params.ctx.AccountId }, + ...resolveConfigWriteScopesFromPath(parsedWritePath), + allowBypass: + isInternalMessageChannel(params.command.channel) && + params.ctx.GatewayClientScopes?.includes("operator.admin") === true, }); - if (!allowWrites) { - const channelLabel = channelId ?? "this channel"; - const hint = channelId - ? `channels.${channelId}.configWrites=true` - : "channels..configWrites=true"; + if (!writeAuth.allowed) { + if (writeAuth.reason === "ambiguous-target") { + return { + shouldContinue: false, + reply: { + text: "⚠️ Channel-initiated /config writes cannot replace channels, channel roots, or accounts collections. Use a more specific path or gateway operator.admin.", + }, + }; + } + const blocked = writeAuth.blockedScope?.scope; + const channelLabel = blocked?.channelId ?? channelId ?? "this channel"; + const hint = blocked?.channelId + ? blocked?.accountId + ? `channels.${blocked.channelId}.accounts.${blocked.accountId}.configWrites=true` + : `channels.${blocked.channelId}.configWrites=true` + : channelId + ? `channels.${channelId}.configWrites=true` + : "channels..configWrites=true"; return { shouldContinue: false, reply: { @@ -119,14 +148,7 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma } if (configCommand.action === "unset") { - const parsedPath = parseConfigPath(configCommand.path); - if (!parsedPath.ok || !parsedPath.path) { - return { - shouldContinue: false, - reply: { text: `⚠️ ${parsedPath.error ?? "Invalid path."}` }, - }; - } - const removed = unsetConfigValueAtPath(parsedBase, parsedPath.path); + const removed = unsetConfigValueAtPath(parsedBase, parsedWritePath ?? []); if (!removed) { return { shouldContinue: false, @@ -151,14 +173,7 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma } if (configCommand.action === "set") { - const parsedPath = parseConfigPath(configCommand.path); - if (!parsedPath.ok || !parsedPath.path) { - return { - shouldContinue: false, - reply: { text: `⚠️ ${parsedPath.error ?? "Invalid path."}` }, - }; - } - setConfigValueAtPath(parsedBase, parsedPath.path, configCommand.value); + setConfigValueAtPath(parsedBase, parsedWritePath ?? [], configCommand.value); const validated = validateConfigObjectWithPlugins(parsedBase); if (!validated.ok) { const issue = validated.issues[0]; diff --git a/src/auto-reply/reply/commands.test.ts b/src/auto-reply/reply/commands.test.ts index 0f526d6edaa..01e2c2238dd 100644 --- a/src/auto-reply/reply/commands.test.ts +++ b/src/auto-reply/reply/commands.test.ts @@ -682,6 +682,52 @@ describe("handleCommands /config configWrites gating", () => { expect(result.reply?.text).toContain("Config writes are disabled"); }); + it("blocks /config set when the target account disables writes", async () => { + const previousWriteCount = writeConfigFileMock.mock.calls.length; + const cfg = { + commands: { config: true, text: true }, + channels: { + telegram: { + configWrites: true, + accounts: { + work: { configWrites: false, enabled: true }, + }, + }, + }, + } as OpenClawConfig; + const params = buildPolicyParams( + "/config set channels.telegram.accounts.work.enabled=false", + cfg, + { + AccountId: "default", + Provider: "telegram", + Surface: "telegram", + }, + ); + const result = await handleCommands(params); + expect(result.shouldContinue).toBe(false); + expect(result.reply?.text).toContain("channels.telegram.accounts.work.configWrites=true"); + expect(writeConfigFileMock.mock.calls.length).toBe(previousWriteCount); + }); + + it("blocks ambiguous channel-root /config writes from channel commands", async () => { + const previousWriteCount = writeConfigFileMock.mock.calls.length; + const cfg = { + commands: { config: true, text: true }, + channels: { telegram: { configWrites: true } }, + } as OpenClawConfig; + const params = buildPolicyParams('/config set channels.telegram={"enabled":false}', cfg, { + Provider: "telegram", + Surface: "telegram", + }); + const result = await handleCommands(params); + expect(result.shouldContinue).toBe(false); + expect(result.reply?.text).toContain( + "cannot replace channels, channel roots, or accounts collections", + ); + expect(writeConfigFileMock.mock.calls.length).toBe(previousWriteCount); + }); + it("blocks /config set from gateway clients without operator.admin", async () => { const cfg = { commands: { config: true, text: true }, @@ -739,6 +785,49 @@ describe("handleCommands /config configWrites gating", () => { expect(writeConfigFileMock).toHaveBeenCalledOnce(); expect(result.reply?.text).toContain("Config updated"); }); + + it("keeps /config set working for gateway operator.admin on protected account paths", async () => { + readConfigFileSnapshotMock.mockResolvedValueOnce({ + valid: true, + parsed: { + channels: { + telegram: { + accounts: { + work: { enabled: true, configWrites: false }, + }, + }, + }, + }, + }); + validateConfigObjectWithPluginsMock.mockImplementation((config: unknown) => ({ + ok: true, + config, + })); + const params = buildParams( + "/config set channels.telegram.accounts.work.enabled=false", + { + commands: { config: true, text: true }, + channels: { + telegram: { + accounts: { + work: { enabled: true, configWrites: false }, + }, + }, + }, + } as OpenClawConfig, + { + Provider: INTERNAL_MESSAGE_CHANNEL, + Surface: INTERNAL_MESSAGE_CHANNEL, + GatewayClientScopes: ["operator.write", "operator.admin"], + }, + ); + params.command.channel = INTERNAL_MESSAGE_CHANNEL; + const result = await handleCommands(params); + expect(result.shouldContinue).toBe(false); + expect(result.reply?.text).toContain("Config updated"); + const written = writeConfigFileMock.mock.calls.at(-1)?.[0] as OpenClawConfig; + expect(written.channels?.telegram?.accounts?.work?.enabled).toBe(false); + }); }); describe("handleCommands bash alias", () => { @@ -891,6 +980,31 @@ describe("handleCommands /allowlist", () => { }); }); + it("blocks config-targeted /allowlist edits when the target account disables writes", async () => { + const previousWriteCount = writeConfigFileMock.mock.calls.length; + const cfg = { + commands: { text: true, config: true }, + channels: { + telegram: { + configWrites: true, + accounts: { + work: { configWrites: false, allowFrom: ["123"] }, + }, + }, + }, + } as OpenClawConfig; + const params = buildPolicyParams("/allowlist add dm --account work --config 789", cfg, { + AccountId: "default", + Provider: "telegram", + Surface: "telegram", + }); + const result = await handleCommands(params); + + expect(result.shouldContinue).toBe(false); + expect(result.reply?.text).toContain("channels.telegram.accounts.work.configWrites=true"); + expect(writeConfigFileMock.mock.calls.length).toBe(previousWriteCount); + }); + it("removes default-account entries from scoped and legacy pairing stores", async () => { removeChannelAllowFromStoreEntryMock .mockResolvedValueOnce({ diff --git a/src/channels/plugins/config-writes.ts b/src/channels/plugins/config-writes.ts index 87e220d7029..440e95dcfe9 100644 --- a/src/channels/plugins/config-writes.ts +++ b/src/channels/plugins/config-writes.ts @@ -12,6 +12,19 @@ function resolveAccountConfig(accounts: ChannelConfigWithAccounts["accounts"], a return resolveAccountEntry(accounts, accountId); } +export type ConfigWriteScope = { + channelId?: ChannelId | null; + accountId?: string | null; +}; + +export type ConfigWriteAuthorizationResult = + | { allowed: true } + | { + allowed: false; + reason: "ambiguous-target" | "origin-disabled" | "target-disabled"; + blockedScope?: { kind: "origin" | "target"; scope: ConfigWriteScope }; + }; + export function resolveChannelConfigWrites(params: { cfg: OpenClawConfig; channelId?: ChannelId | null; @@ -30,3 +43,86 @@ export function resolveChannelConfigWrites(params: { const value = accountConfig?.configWrites ?? channelConfig.configWrites; return value !== false; } + +export function authorizeConfigWrite(params: { + cfg: OpenClawConfig; + origin?: ConfigWriteScope; + targets?: ConfigWriteScope[]; + allowBypass?: boolean; + hasAmbiguousTarget?: boolean; +}): ConfigWriteAuthorizationResult { + if (params.allowBypass) { + return { allowed: true }; + } + if (params.hasAmbiguousTarget) { + return { allowed: false, reason: "ambiguous-target" }; + } + if ( + params.origin?.channelId && + !resolveChannelConfigWrites({ + cfg: params.cfg, + channelId: params.origin.channelId, + accountId: params.origin.accountId, + }) + ) { + return { + allowed: false, + reason: "origin-disabled", + blockedScope: { kind: "origin", scope: params.origin }, + }; + } + const seen = new Set(); + for (const target of params.targets ?? []) { + if (!target.channelId) { + continue; + } + const key = `${target.channelId}:${normalizeAccountId(target.accountId)}`; + if (seen.has(key)) { + continue; + } + seen.add(key); + if ( + !resolveChannelConfigWrites({ + cfg: params.cfg, + channelId: target.channelId, + accountId: target.accountId, + }) + ) { + return { + allowed: false, + reason: "target-disabled", + blockedScope: { kind: "target", scope: target }, + }; + } + } + return { allowed: true }; +} + +export function resolveConfigWriteScopesFromPath(path: string[]): { + targets: ConfigWriteScope[]; + hasAmbiguousTarget: boolean; +} { + if (path[0] !== "channels") { + return { targets: [], hasAmbiguousTarget: false }; + } + if (path.length < 2) { + return { targets: [], hasAmbiguousTarget: true }; + } + const channelId = path[1].trim().toLowerCase() as ChannelId; + if (!channelId) { + return { targets: [], hasAmbiguousTarget: true }; + } + if (path.length === 2) { + return { targets: [{ channelId }], hasAmbiguousTarget: true }; + } + if (path[2] !== "accounts") { + return { targets: [{ channelId }], hasAmbiguousTarget: false }; + } + if (path.length < 4) { + return { targets: [{ channelId }], hasAmbiguousTarget: true }; + } + return { + targets: [{ channelId, accountId: normalizeAccountId(path[3]) }], + hasAmbiguousTarget: false, + }; +} diff --git a/src/channels/plugins/plugins-core.test.ts b/src/channels/plugins/plugins-core.test.ts index 49012222982..8d574e472b4 100644 --- a/src/channels/plugins/plugins-core.test.ts +++ b/src/channels/plugins/plugins-core.test.ts @@ -20,7 +20,11 @@ import { } from "../../test-utils/channel-plugins.js"; import { withEnvAsync } from "../../test-utils/env.js"; import { getChannelPluginCatalogEntry, listChannelPluginCatalogEntries } from "./catalog.js"; -import { resolveChannelConfigWrites } from "./config-writes.js"; +import { + authorizeConfigWrite, + resolveChannelConfigWrites, + resolveConfigWriteScopesFromPath, +} from "./config-writes.js"; import { listDiscordDirectoryGroupsFromConfig, listDiscordDirectoryPeersFromConfig, @@ -325,6 +329,34 @@ describe("resolveChannelConfigWrites", () => { }); }); +describe("authorizeConfigWrite", () => { + it("blocks when a target account disables writes", () => { + const cfg = makeSlackConfigWritesCfg("work"); + expect( + authorizeConfigWrite({ + cfg, + origin: { channelId: "slack", accountId: "default" }, + targets: [{ channelId: "slack", accountId: "work" }], + }), + ).toEqual({ + allowed: false, + reason: "target-disabled", + blockedScope: { kind: "target", scope: { channelId: "slack", accountId: "work" } }, + }); + }); + + it("rejects ambiguous channel collection writes", () => { + expect(resolveConfigWriteScopesFromPath(["channels", "telegram"])).toEqual({ + targets: [{ channelId: "telegram" }], + hasAmbiguousTarget: true, + }); + expect(resolveConfigWriteScopesFromPath(["channels", "telegram", "accounts"])).toEqual({ + targets: [{ channelId: "telegram" }], + hasAmbiguousTarget: true, + }); + }); +}); + describe("directory (config-backed)", () => { it("lists Slack peers/groups from config", async () => { const cfg = { From 7289c19f1a35fe82f8047c11de9d7cc0429ae112 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:25:19 +0000 Subject: [PATCH 05/24] fix(security): bind system.run approvals to exact argv text --- CHANGELOG.md | 1 + src/cli/nodes-cli.coverage.test.ts | 10 ++-- src/discord/monitor/exec-approvals.ts | 31 ++++++++++++ .../node-invoke-system-run-approval.test.ts | 10 +++- src/gateway/protocol/schema/exec-approvals.ts | 1 + src/gateway/server-methods/exec-approval.ts | 2 + .../server-methods/server-methods.test.ts | 29 +++++++++++ src/infra/exec-approvals.ts | 2 + src/infra/system-run-approval-binding.ts | 1 + src/infra/system-run-approval-context.test.ts | 40 +++++++++++++++ src/infra/system-run-approval-context.ts | 18 ++++++- src/infra/system-run-command.test.ts | 38 ++++++++++++++ src/infra/system-run-command.ts | 50 ++++++++++--------- src/node-host/invoke-system-run-plan.test.ts | 15 ++++++ src/node-host/invoke-system-run-plan.ts | 11 ++-- src/test-utils/system-run-prepare-payload.ts | 7 ++- .../fixtures/system-run-command-contract.json | 9 ++++ 17 files changed, 241 insertions(+), 34 deletions(-) create mode 100644 src/infra/system-run-approval-context.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dcfe701e11..609d16bed3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -222,6 +222,7 @@ Docs: https://docs.openclaw.ai - Onboarding/API key input hardening: strip non-Latin1 Unicode artifacts from normalized secret input (while preserving Latin-1 content and internal spaces) so malformed copied API keys cannot trigger HTTP header `ByteString` construction crashes; adds regression coverage for shared normalization and MiniMax auth header usage. (#24496) Thanks @fa6maalassaf. - Kimi Coding/Anthropic tools compatibility: normalize `anthropic-messages` tool payloads to OpenAI-style `tools[].function` + compatible `tool_choice` when targeting Kimi Coding endpoints, restoring tool-call workflows that regressed after v2026.3.2. (#37038) Thanks @mochimochimochi-hub. - Heartbeat/workspace-path guardrails: append explicit workspace `HEARTBEAT.md` path guidance (and `docs/heartbeat.md` avoidance) to heartbeat prompts so heartbeat runs target workspace checklists reliably across packaged install layouts. (#37037) Thanks @stofancy. +- Node/system.run approvals: bind approval prompts to the exact executed argv text and show shell payload only as a secondary preview, closing basename-spoofed wrapper approval mismatches. Thanks @tdjackey. - Subagents/kill-complete announce race: when a late `subagent-complete` lifecycle event arrives after an earlier kill marker, clear stale kill suppression/cleanup flags and re-run announce cleanup so finished runs no longer get silently swallowed. (#37024) Thanks @cmfinlan. - Agents/tool-result cleanup timeout hardening: on embedded runner teardown idle timeouts, clear pending tool-call state without persisting synthetic `missing tool result` entries, preventing timeout cleanups from poisoning follow-up turns; adds regression coverage for timeout clear-vs-flush behavior. (#37081) Thanks @Coyote-Den. - Agents/openai-completions stream timeout hardening: ensure runtime undici global dispatchers use extended streaming body/header timeouts (including env-proxy dispatcher mode) before embedded runs, reducing forced mid-stream `terminated` failures on long generations; adds regression coverage for dispatcher selection and idempotent reconfiguration. (#9708) Thanks @scottchguard. diff --git a/src/cli/nodes-cli.coverage.test.ts b/src/cli/nodes-cli.coverage.test.ts index 04bdfb39bf8..cba8a8de7fb 100644 --- a/src/cli/nodes-cli.coverage.test.ts +++ b/src/cli/nodes-cli.coverage.test.ts @@ -174,7 +174,7 @@ describe("nodes-cli coverage", () => { expect(invoke?.params?.command).toBe("system.run"); expect(invoke?.params?.params).toEqual({ command: ["echo", "hi"], - rawCommand: null, + rawCommand: "echo hi", cwd: "/tmp", env: { FOO: "bar" }, timeoutMs: 1200, @@ -190,7 +190,8 @@ describe("nodes-cli coverage", () => { expect(approval?.params?.["systemRunPlan"]).toEqual({ argv: ["echo", "hi"], cwd: "/tmp", - rawCommand: null, + rawCommand: "echo hi", + commandPreview: null, agentId: "main", sessionKey: null, }); @@ -213,7 +214,7 @@ describe("nodes-cli coverage", () => { expect(invoke?.params?.command).toBe("system.run"); expect(invoke?.params?.params).toMatchObject({ command: ["/bin/sh", "-lc", "echo hi"], - rawCommand: "echo hi", + rawCommand: '/bin/sh -lc "echo hi"', agentId: "main", approved: true, approvalDecision: "allow-once", @@ -224,7 +225,8 @@ describe("nodes-cli coverage", () => { expect(approval?.params?.["systemRunPlan"]).toEqual({ argv: ["/bin/sh", "-lc", "echo hi"], cwd: null, - rawCommand: "echo hi", + rawCommand: '/bin/sh -lc "echo hi"', + commandPreview: "echo hi", agentId: "main", sessionKey: null, }); diff --git a/src/discord/monitor/exec-approvals.ts b/src/discord/monitor/exec-approvals.ts index e8583475e30..79635bd5ebe 100644 --- a/src/discord/monitor/exec-approvals.ts +++ b/src/discord/monitor/exec-approvals.ts @@ -105,6 +105,7 @@ type ExecApprovalContainerParams = { title: string; description?: string; commandPreview: string; + commandSecondaryPreview?: string | null; metadataLines?: string[]; actionRow?: Row