diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f22ef04477..bc30d60df92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ Docs: https://docs.openclaw.ai - Discord/Gateway: handle close code 4014 (missing privileged gateway intents) without crashing the gateway. Thanks @thewilloftheshadow. - Security/Net: strip sensitive headers (`Authorization`, `Proxy-Authorization`, `Cookie`, `Cookie2`) on cross-origin redirects in `fetchWithSsrFGuard` to prevent credential forwarding across origin boundaries. (#20313) Thanks @afurm. +- Skills/Security: sanitize skill env overrides to block unsafe runtime injection variables and only allow sensitive keys when declared in skill metadata, with warnings for suspicious values. Thanks @thewilloftheshadow. - Auto-reply/Runner: emit `onAgentRunStart` only after agent lifecycle or tool activity begins (and only once per run), so fallback preflight errors no longer mark runs as started. (#21165) Thanks @shakkernerd. - Agents/Failover: treat non-default override runs as direct fallback-to-configured-primary (skip configured fallback chain), normalize default-model detection for provider casing/whitespace, and add regression coverage for override/auth error paths. (#18820) Thanks @Glucksberg. - Auto-reply/Tool results: serialize tool-result delivery and keep the delivery chain progressing after individual failures so concurrent tool outputs preserve user-visible ordering. (#21231) thanks @ahdernasr. diff --git a/src/agents/sandbox/sanitize-env-vars.ts b/src/agents/sandbox/sanitize-env-vars.ts index df61aeb7697..254c802e08c 100644 --- a/src/agents/sandbox/sanitize-env-vars.ts +++ b/src/agents/sandbox/sanitize-env-vars.ts @@ -42,7 +42,7 @@ export type EnvSanitizationOptions = { customAllowedPatterns?: ReadonlyArray; }; -function validateEnvVarValue(value: string): string | undefined { +export function validateEnvVarValue(value: string): string | undefined { if (value.includes("\0")) { return "Contains null bytes"; } diff --git a/src/agents/skills.e2e.test.ts b/src/agents/skills.e2e.test.ts index a174da332a9..72dbc84af79 100644 --- a/src/agents/skills.e2e.test.ts +++ b/src/agents/skills.e2e.test.ts @@ -296,4 +296,102 @@ describe("applySkillEnvOverrides", () => { } } }); + + it("blocks unsafe env overrides but allows declared secrets", async () => { + const workspaceDir = await makeWorkspace(); + const skillDir = path.join(workspaceDir, "skills", "unsafe-env-skill"); + await writeSkill({ + dir: skillDir, + name: "unsafe-env-skill", + description: "Needs env", + metadata: + '{"openclaw":{"requires":{"env":["OPENAI_API_KEY","NODE_OPTIONS"]},"primaryEnv":"OPENAI_API_KEY"}}', + }); + + const entries = loadWorkspaceSkillEntries(workspaceDir, { + managedSkillsDir: path.join(workspaceDir, ".managed"), + }); + + const originalApiKey = process.env.OPENAI_API_KEY; + const originalNodeOptions = process.env.NODE_OPTIONS; + delete process.env.OPENAI_API_KEY; + delete process.env.NODE_OPTIONS; + + const restore = applySkillEnvOverrides({ + skills: entries, + config: { + skills: { + entries: { + "unsafe-env-skill": { + env: { + OPENAI_API_KEY: "sk-test", + NODE_OPTIONS: "--require /tmp/evil.js", + }, + }, + }, + }, + }, + }); + + try { + expect(process.env.OPENAI_API_KEY).toBe("sk-test"); + expect(process.env.NODE_OPTIONS).toBeUndefined(); + } finally { + restore(); + if (originalApiKey === undefined) { + expect(process.env.OPENAI_API_KEY).toBeUndefined(); + } else { + expect(process.env.OPENAI_API_KEY).toBe(originalApiKey); + } + if (originalNodeOptions === undefined) { + expect(process.env.NODE_OPTIONS).toBeUndefined(); + } else { + expect(process.env.NODE_OPTIONS).toBe(originalNodeOptions); + } + } + }); + + it("allows required env overrides from snapshots", async () => { + const workspaceDir = await makeWorkspace(); + const skillDir = path.join(workspaceDir, "skills", "snapshot-env-skill"); + await writeSkill({ + dir: skillDir, + name: "snapshot-env-skill", + description: "Needs env", + metadata: '{"openclaw":{"requires":{"env":["OPENAI_API_KEY"]}}}', + }); + + const snapshot = buildWorkspaceSkillSnapshot(workspaceDir, { + managedSkillsDir: path.join(workspaceDir, ".managed"), + }); + + const originalApiKey = process.env.OPENAI_API_KEY; + delete process.env.OPENAI_API_KEY; + + const restore = applySkillEnvOverridesFromSnapshot({ + snapshot, + config: { + skills: { + entries: { + "snapshot-env-skill": { + env: { + OPENAI_API_KEY: "snap-secret", + }, + }, + }, + }, + }, + }); + + try { + expect(process.env.OPENAI_API_KEY).toBe("snap-secret"); + } finally { + restore(); + if (originalApiKey === undefined) { + expect(process.env.OPENAI_API_KEY).toBeUndefined(); + } else { + expect(process.env.OPENAI_API_KEY).toBe(originalApiKey); + } + } + }); }); diff --git a/src/agents/skills/env-overrides.ts b/src/agents/skills/env-overrides.ts index 0f5061a0da4..df19f79ef09 100644 --- a/src/agents/skills/env-overrides.ts +++ b/src/agents/skills/env-overrides.ts @@ -1,4 +1,5 @@ import type { OpenClawConfig } from "../../config/config.js"; +import { sanitizeEnvVars, validateEnvVarValue } from "../sandbox/sanitize-env-vars.js"; import { resolveSkillConfig } from "./config.js"; import { resolveSkillKey } from "./frontmatter.js"; import type { SkillEntry, SkillSnapshot } from "./types.js"; @@ -6,25 +7,123 @@ import type { SkillEntry, SkillSnapshot } from "./types.js"; type EnvUpdate = { key: string; prev: string | undefined }; type SkillConfig = NonNullable>; +type SanitizedSkillEnvOverrides = { + allowed: Record; + blocked: string[]; + warnings: string[]; +}; + +// Never allow skill env overrides that can alter runtime loader flags. +const HARD_BLOCKED_SKILL_ENV_PATTERNS: ReadonlyArray = [ + /^NODE_OPTIONS$/i, + /^OPENSSL_CONF$/i, + /^LD_PRELOAD$/i, + /^DYLD_INSERT_LIBRARIES$/i, +]; + +function matchesAnyPattern(value: string, patterns: readonly RegExp[]): boolean { + return patterns.some((pattern) => pattern.test(value)); +} + +function sanitizeSkillEnvOverrides(params: { + overrides: Record; + allowedSensitiveKeys: Set; +}): SanitizedSkillEnvOverrides { + if (Object.keys(params.overrides).length === 0) { + return { allowed: {}, blocked: [], warnings: [] }; + } + + const result = sanitizeEnvVars(params.overrides, { + customBlockedPatterns: HARD_BLOCKED_SKILL_ENV_PATTERNS, + }); + const allowed = { ...result.allowed }; + const blocked: string[] = []; + const warnings = [...result.warnings]; + + for (const key of result.blocked) { + if ( + matchesAnyPattern(key, HARD_BLOCKED_SKILL_ENV_PATTERNS) || + !params.allowedSensitiveKeys.has(key) + ) { + blocked.push(key); + continue; + } + const value = params.overrides[key]; + if (!value) { + continue; + } + const warning = validateEnvVarValue(value); + if (warning) { + if (warning === "Contains null bytes") { + blocked.push(key); + continue; + } + warnings.push(`${key}: ${warning}`); + } + allowed[key] = value; + } + + return { allowed, blocked, warnings }; +} + function applySkillConfigEnvOverrides(params: { updates: EnvUpdate[]; skillConfig: SkillConfig; primaryEnv?: string | null; + requiredEnv?: string[] | null; + skillKey: string; }) { - const { updates, skillConfig, primaryEnv } = params; - if (skillConfig.env) { - for (const [envKey, envValue] of Object.entries(skillConfig.env)) { - if (!envValue || process.env[envKey]) { - continue; - } - updates.push({ key: envKey, prev: process.env[envKey] }); - process.env[envKey] = envValue; + const { updates, skillConfig, primaryEnv, requiredEnv, skillKey } = params; + const allowedSensitiveKeys = new Set(); + const normalizedPrimaryEnv = primaryEnv?.trim(); + if (normalizedPrimaryEnv) { + allowedSensitiveKeys.add(normalizedPrimaryEnv); + } + for (const envName of requiredEnv ?? []) { + const trimmedEnv = envName.trim(); + if (trimmedEnv) { + allowedSensitiveKeys.add(trimmedEnv); } } - if (primaryEnv && skillConfig.apiKey && !process.env[primaryEnv]) { - updates.push({ key: primaryEnv, prev: process.env[primaryEnv] }); - process.env[primaryEnv] = skillConfig.apiKey; + const pendingOverrides: Record = {}; + if (skillConfig.env) { + for (const [rawKey, envValue] of Object.entries(skillConfig.env)) { + const envKey = rawKey.trim(); + if (!envKey || !envValue || process.env[envKey]) { + continue; + } + pendingOverrides[envKey] = envValue; + } + } + + if (normalizedPrimaryEnv && skillConfig.apiKey && !process.env[normalizedPrimaryEnv]) { + if (!pendingOverrides[normalizedPrimaryEnv]) { + pendingOverrides[normalizedPrimaryEnv] = skillConfig.apiKey; + } + } + + const sanitized = sanitizeSkillEnvOverrides({ + overrides: pendingOverrides, + allowedSensitiveKeys, + }); + + if (sanitized.blocked.length > 0) { + console.warn( + `[Security] Blocked skill env overrides for ${skillKey}:`, + sanitized.blocked.join(", "), + ); + } + if (sanitized.warnings.length > 0) { + console.warn(`[Security] Suspicious skill env overrides for ${skillKey}:`, sanitized.warnings); + } + + for (const [envKey, envValue] of Object.entries(sanitized.allowed)) { + if (process.env[envKey]) { + continue; + } + updates.push({ key: envKey, prev: process.env[envKey] }); + process.env[envKey] = envValue; } } @@ -55,6 +154,8 @@ export function applySkillEnvOverrides(params: { skills: SkillEntry[]; config?: updates, skillConfig, primaryEnv: entry.metadata?.primaryEnv, + requiredEnv: entry.metadata?.requires?.env, + skillKey, }); } @@ -81,6 +182,8 @@ export function applySkillEnvOverridesFromSnapshot(params: { updates, skillConfig, primaryEnv: skill.primaryEnv, + requiredEnv: skill.requiredEnv, + skillKey: skill.name, }); } diff --git a/src/agents/skills/types.ts b/src/agents/skills/types.ts index abfb8743dd7..e3eef67a2fd 100644 --- a/src/agents/skills/types.ts +++ b/src/agents/skills/types.ts @@ -81,7 +81,7 @@ export type SkillEligibilityContext = { export type SkillSnapshot = { prompt: string; - skills: Array<{ name: string; primaryEnv?: string }>; + skills: Array<{ name: string; primaryEnv?: string; requiredEnv?: string[] }>; /** Normalized agent-level filter used to build this snapshot; undefined means unrestricted. */ skillFilter?: string[]; resolvedSkills?: Skill[]; diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index 5e2123941af..98c9a679488 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -490,6 +490,7 @@ export function buildWorkspaceSkillSnapshot( skills: eligible.map((entry) => ({ name: entry.skill.name, primaryEnv: entry.metadata?.primaryEnv, + requiredEnv: entry.metadata?.requires?.env?.slice(), })), ...(skillFilter === undefined ? {} : { skillFilter }), resolvedSkills, diff --git a/src/config/sessions/types.ts b/src/config/sessions/types.ts index f103d61d57c..10d1d3bc54b 100644 --- a/src/config/sessions/types.ts +++ b/src/config/sessions/types.ts @@ -152,7 +152,7 @@ export type GroupKeyResolution = { export type SessionSkillSnapshot = { prompt: string; - skills: Array<{ name: string; primaryEnv?: string }>; + skills: Array<{ name: string; primaryEnv?: string; requiredEnv?: string[] }>; /** Normalized agent-level filter used to build this snapshot; undefined means unrestricted. */ skillFilter?: string[]; resolvedSkills?: Skill[];