From f202e73077a2a419595f395f4bc0a6185fa8307b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 13:04:34 +0100 Subject: [PATCH] refactor(security): centralize host env policy and harden env ingestion --- .../Sources/OpenClaw/HostEnvSanitizer.swift | 2 + .../daemon-install-helpers.e2e.test.ts | 25 +++++++++ src/commands/daemon-install-helpers.ts | 4 +- src/config/config.env-vars.test.ts | 22 +++++++- src/config/env-vars.ts | 33 +++++++++-- src/discord/voice/manager.ts | 2 +- src/infra/host-env-security-policy.json | 18 ++++++ .../host-env-security.policy-parity.test.ts | 38 +++++++++++++ src/infra/host-env-security.test.ts | 32 ++++++++++- src/infra/host-env-security.ts | 56 ++++++++++++------- 10 files changed, 201 insertions(+), 31 deletions(-) create mode 100644 src/infra/host-env-security-policy.json create mode 100644 src/infra/host-env-security.policy-parity.test.ts diff --git a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift index bbef0486fad..330e5b3b6f9 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift @@ -1,6 +1,8 @@ import Foundation enum HostEnvSanitizer { + // Keep in sync with src/infra/host-env-security-policy.json. + // Parity is validated by src/infra/host-env-security.policy-parity.test.ts. private static let blockedKeys: Set = [ "NODE_OPTIONS", "NODE_PATH", diff --git a/src/commands/daemon-install-helpers.e2e.test.ts b/src/commands/daemon-install-helpers.e2e.test.ts index 2fb1645f900..cf3c6a8af86 100644 --- a/src/commands/daemon-install-helpers.e2e.test.ts +++ b/src/commands/daemon-install-helpers.e2e.test.ts @@ -140,6 +140,31 @@ describe("buildGatewayInstallPlan", () => { expect(plan.environment.HOME).toBe("/Users/me"); }); + it("drops dangerous config env vars before service merge", async () => { + mockNodeGatewayPlanFixture({ + serviceEnvironment: { + OPENCLAW_PORT: "3000", + }, + }); + + const plan = await buildGatewayInstallPlan({ + env: {}, + port: 3000, + runtime: "node", + config: { + env: { + vars: { + NODE_OPTIONS: "--require /tmp/evil.js", + SAFE_KEY: "safe-value", + }, + }, + }, + }); + + expect(plan.environment.NODE_OPTIONS).toBeUndefined(); + expect(plan.environment.SAFE_KEY).toBe("safe-value"); + }); + it("does not include empty config env values", async () => { mockNodeGatewayPlanFixture(); diff --git a/src/commands/daemon-install-helpers.ts b/src/commands/daemon-install-helpers.ts index f027d2fdc2d..8bcd717c3df 100644 --- a/src/commands/daemon-install-helpers.ts +++ b/src/commands/daemon-install-helpers.ts @@ -1,5 +1,5 @@ import { formatCliCommand } from "../cli/command-format.js"; -import { collectConfigEnvVars } from "../config/env-vars.js"; +import { collectConfigServiceEnvVars } from "../config/env-vars.js"; import type { OpenClawConfig } from "../config/types.js"; import { resolveGatewayLaunchAgentLabel } from "../daemon/constants.js"; import { resolveGatewayProgramArguments } from "../daemon/program-args.js"; @@ -67,7 +67,7 @@ export async function buildGatewayInstallPlan(params: { // Merge config env vars into the service environment (vars + inline env keys). // Config env vars are added first so service-specific vars take precedence. const environment: Record = { - ...collectConfigEnvVars(params.config), + ...collectConfigServiceEnvVars(params.config), }; Object.assign(environment, serviceEnvironment); diff --git a/src/config/config.env-vars.test.ts b/src/config/config.env-vars.test.ts index 37fa7f8fe48..9aba6f6dbea 100644 --- a/src/config/config.env-vars.test.ts +++ b/src/config/config.env-vars.test.ts @@ -3,7 +3,7 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; import { loadDotEnv } from "../infra/dotenv.js"; import { resolveConfigEnvVars } from "./env-substitution.js"; -import { applyConfigEnvVars, collectConfigEnvVars } from "./env-vars.js"; +import { applyConfigEnvVars, collectConfigRuntimeEnvVars } from "./env-vars.js"; import { withEnvOverride, withTempHome } from "./test-helpers.js"; import type { OpenClawConfig } from "./types.js"; @@ -34,7 +34,7 @@ describe("config env vars", () => { const config = { env: { vars: { BASH_ENV: "/tmp/pwn.sh", OPENROUTER_API_KEY: "config-key" } }, }; - const entries = collectConfigEnvVars(config as OpenClawConfig); + const entries = collectConfigRuntimeEnvVars(config as OpenClawConfig); expect(entries.BASH_ENV).toBeUndefined(); expect(entries.OPENROUTER_API_KEY).toBe("config-key"); @@ -44,6 +44,24 @@ describe("config env vars", () => { }); }); + it("drops non-portable env keys from config env", async () => { + await withEnvOverride({ OPENROUTER_API_KEY: undefined }, async () => { + const config = { + env: { + vars: { + " BAD KEY": "oops", + OPENROUTER_API_KEY: "config-key", + }, + "NOT-PORTABLE": "bad", + }, + }; + const entries = collectConfigRuntimeEnvVars(config as OpenClawConfig); + expect(entries.OPENROUTER_API_KEY).toBe("config-key"); + expect(entries[" BAD KEY"]).toBeUndefined(); + expect(entries["NOT-PORTABLE"]).toBeUndefined(); + }); + }); + it("loads ${VAR} substitutions from ~/.openclaw/.env on repeated runtime loads", async () => { await withTempHome(async (_home) => { await withEnvOverride({ BRAVE_API_KEY: undefined }, async () => { diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts index cff37b1efd1..a26d69a62f3 100644 --- a/src/config/env-vars.ts +++ b/src/config/env-vars.ts @@ -1,7 +1,7 @@ -import { isDangerousHostEnvVarName } from "../infra/host-env-security.js"; +import { isDangerousHostEnvVarName, normalizeEnvVarKey } from "../infra/host-env-security.js"; import type { OpenClawConfig } from "./types.js"; -export function collectConfigEnvVars(cfg?: OpenClawConfig): Record { +function collectConfigEnvVarsByTarget(cfg?: OpenClawConfig): Record { const envConfig = cfg?.env; if (!envConfig) { return {}; @@ -10,10 +10,14 @@ export function collectConfigEnvVars(cfg?: OpenClawConfig): Record = {}; if (envConfig.vars) { - for (const [key, value] of Object.entries(envConfig.vars)) { + for (const [rawKey, value] of Object.entries(envConfig.vars)) { if (!value) { continue; } + const key = normalizeEnvVarKey(rawKey, { portable: true }); + if (!key) { + continue; + } if (isDangerousHostEnvVarName(key)) { continue; } @@ -21,13 +25,17 @@ export function collectConfigEnvVars(cfg?: OpenClawConfig): Record { + return collectConfigEnvVarsByTarget(cfg); +} + +export function collectConfigServiceEnvVars(cfg?: OpenClawConfig): Record { + return collectConfigEnvVarsByTarget(cfg); +} + +/** @deprecated Use `collectConfigRuntimeEnvVars` or `collectConfigServiceEnvVars`. */ +export function collectConfigEnvVars(cfg?: OpenClawConfig): Record { + return collectConfigRuntimeEnvVars(cfg); +} + export function applyConfigEnvVars( cfg: OpenClawConfig, env: NodeJS.ProcessEnv = process.env, ): void { - const entries = collectConfigEnvVars(cfg); + const entries = collectConfigRuntimeEnvVars(cfg); for (const [key, value] of Object.entries(entries)) { if (env[key]?.trim()) { continue; diff --git a/src/discord/voice/manager.ts b/src/discord/voice/manager.ts index ff048d7ac1f..8668e57b6fc 100644 --- a/src/discord/voice/manager.ts +++ b/src/discord/voice/manager.ts @@ -37,7 +37,6 @@ import { parseTtsDirectives } from "../../tts/tts-core.js"; import { resolveTtsConfig, textToSpeech, type ResolvedTtsConfig } from "../../tts/tts.js"; const require = createRequire(import.meta.url); -const OpusScript = require("opusscript") as typeof import("opusscript"); const SAMPLE_RATE = 48_000; const CHANNELS = 2; @@ -149,6 +148,7 @@ type OpusDecoder = { function createOpusDecoder(): { decoder: OpusDecoder; name: string } | null { try { + const OpusScript = require("opusscript") as typeof import("opusscript"); const decoder = new OpusScript(SAMPLE_RATE, CHANNELS, OpusScript.Application.AUDIO); return { decoder, name: "opusscript" }; } catch (err) { diff --git a/src/infra/host-env-security-policy.json b/src/infra/host-env-security-policy.json new file mode 100644 index 00000000000..b7760800b2c --- /dev/null +++ b/src/infra/host-env-security-policy.json @@ -0,0 +1,18 @@ +{ + "blockedKeys": [ + "NODE_OPTIONS", + "NODE_PATH", + "PYTHONHOME", + "PYTHONPATH", + "PERL5LIB", + "PERL5OPT", + "RUBYLIB", + "RUBYOPT", + "BASH_ENV", + "ENV", + "GCONV_PATH", + "IFS", + "SSLKEYLOGFILE" + ], + "blockedPrefixes": ["DYLD_", "LD_", "BASH_FUNC_"] +} diff --git a/src/infra/host-env-security.policy-parity.test.ts b/src/infra/host-env-security.policy-parity.test.ts new file mode 100644 index 00000000000..1b989d52244 --- /dev/null +++ b/src/infra/host-env-security.policy-parity.test.ts @@ -0,0 +1,38 @@ +import fs from "node:fs"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; + +type HostEnvSecurityPolicy = { + blockedKeys: string[]; + blockedPrefixes: string[]; +}; + +function parseSwiftStringArray(source: string, marker: string): string[] { + const escapedMarker = marker.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const re = new RegExp(`${escapedMarker}[\\s\\S]*?=\\s*\\[([\\s\\S]*?)\\]`, "m"); + const match = source.match(re); + if (!match) { + throw new Error(`Failed to parse Swift array for marker: ${marker}`); + } + return Array.from(match[1].matchAll(/"([^"]+)"/g), (m) => m[1]); +} + +describe("host env security policy parity", () => { + it("keeps macOS HostEnvSanitizer lists in sync with shared JSON policy", () => { + const repoRoot = process.cwd(); + const policyPath = path.join(repoRoot, "src/infra/host-env-security-policy.json"); + const swiftPath = path.join(repoRoot, "apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift"); + + const policy = JSON.parse(fs.readFileSync(policyPath, "utf8")) as HostEnvSecurityPolicy; + const swiftSource = fs.readFileSync(swiftPath, "utf8"); + + const swiftBlockedKeys = parseSwiftStringArray(swiftSource, "private static let blockedKeys"); + const swiftBlockedPrefixes = parseSwiftStringArray( + swiftSource, + "private static let blockedPrefixes", + ); + + expect(swiftBlockedKeys).toEqual(policy.blockedKeys); + expect(swiftBlockedPrefixes).toEqual(policy.blockedPrefixes); + }); +}); diff --git a/src/infra/host-env-security.test.ts b/src/infra/host-env-security.test.ts index 47a860c6b7e..773b27dded7 100644 --- a/src/infra/host-env-security.test.ts +++ b/src/infra/host-env-security.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "vitest"; -import { isDangerousHostEnvVarName, sanitizeHostExecEnv } from "./host-env-security.js"; +import { + isDangerousHostEnvVarName, + normalizeEnvVarKey, + sanitizeHostExecEnv, +} from "./host-env-security.js"; describe("isDangerousHostEnvVarName", () => { it("matches dangerous keys and prefixes case-insensitively", () => { @@ -48,4 +52,30 @@ describe("sanitizeHostExecEnv", () => { expect(env.SAFE).toBe("ok"); expect(env.HOME).toBe("/tmp/home"); }); + + it("drops non-portable env key names", () => { + const env = sanitizeHostExecEnv({ + baseEnv: { + PATH: "/usr/bin:/bin", + }, + overrides: { + " BAD KEY": "x", + "NOT-PORTABLE": "x", + GOOD_KEY: "ok", + }, + }); + + expect(env.GOOD_KEY).toBe("ok"); + expect(env[" BAD KEY"]).toBeUndefined(); + expect(env["NOT-PORTABLE"]).toBeUndefined(); + }); +}); + +describe("normalizeEnvVarKey", () => { + it("normalizes and validates keys", () => { + expect(normalizeEnvVarKey(" OPENROUTER_API_KEY ")).toBe("OPENROUTER_API_KEY"); + expect(normalizeEnvVarKey("NOT-PORTABLE", { portable: true })).toBeNull(); + expect(normalizeEnvVarKey(" BASH_FUNC_echo%% ")).toBe("BASH_FUNC_echo%%"); + expect(normalizeEnvVarKey(" ")).toBeNull(); + }); }); diff --git a/src/infra/host-env-security.ts b/src/infra/host-env-security.ts index a3347c60834..f5cd775e70a 100644 --- a/src/infra/host-env-security.ts +++ b/src/infra/host-env-security.ts @@ -1,23 +1,41 @@ -const HOST_DANGEROUS_ENV_KEY_VALUES = [ - "NODE_OPTIONS", - "NODE_PATH", - "PYTHONHOME", - "PYTHONPATH", - "PERL5LIB", - "PERL5OPT", - "RUBYLIB", - "RUBYOPT", - "BASH_ENV", - "ENV", - "GCONV_PATH", - "IFS", - "SSLKEYLOGFILE", -] as const; +import HOST_ENV_SECURITY_POLICY_JSON from "./host-env-security-policy.json" with { type: "json" }; +const PORTABLE_ENV_VAR_KEY = /^[A-Za-z_][A-Za-z0-9_]*$/; + +type HostEnvSecurityPolicy = { + blockedKeys: string[]; + blockedPrefixes: string[]; +}; + +const HOST_ENV_SECURITY_POLICY = HOST_ENV_SECURITY_POLICY_JSON as HostEnvSecurityPolicy; + +export const HOST_DANGEROUS_ENV_KEY_VALUES: readonly string[] = Object.freeze( + HOST_ENV_SECURITY_POLICY.blockedKeys.map((key) => key.toUpperCase()), +); +export const HOST_DANGEROUS_ENV_PREFIXES: readonly string[] = Object.freeze( + HOST_ENV_SECURITY_POLICY.blockedPrefixes.map((prefix) => prefix.toUpperCase()), +); export const HOST_DANGEROUS_ENV_KEYS = new Set(HOST_DANGEROUS_ENV_KEY_VALUES); -export const HOST_DANGEROUS_ENV_PREFIXES = ["DYLD_", "LD_", "BASH_FUNC_"] as const; -export function isDangerousHostEnvVarName(key: string): boolean { +export function normalizeEnvVarKey( + rawKey: string, + options?: { portable?: boolean }, +): string | null { + const key = rawKey.trim(); + if (!key) { + return null; + } + if (options?.portable && !PORTABLE_ENV_VAR_KEY.test(key)) { + return null; + } + return key; +} + +export function isDangerousHostEnvVarName(rawKey: string): boolean { + const key = normalizeEnvVarKey(rawKey); + if (!key) { + return false; + } const upper = key.toUpperCase(); if (HOST_DANGEROUS_ENV_KEYS.has(upper)) { return true; @@ -39,7 +57,7 @@ export function sanitizeHostExecEnv(params?: { if (typeof value !== "string") { continue; } - const key = rawKey.trim(); + const key = normalizeEnvVarKey(rawKey, { portable: true }); if (!key || isDangerousHostEnvVarName(key)) { continue; } @@ -54,7 +72,7 @@ export function sanitizeHostExecEnv(params?: { if (typeof value !== "string") { continue; } - const key = rawKey.trim(); + const key = normalizeEnvVarKey(rawKey, { portable: true }); if (!key) { continue; }