diff --git a/CHANGELOG.md b/CHANGELOG.md index f5dd4304a5..9e7cf641f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ Docs: https://docs.openclaw.ai - Signal/Install: auto-install `signal-cli` via Homebrew on non-x64 Linux architectures, avoiding x86_64 native binary `Exec format error` failures on arm64/arm hosts. (#15443) Thanks @jogvan-k. - Discord: avoid misrouting numeric guild allowlist entries to `/channels/` by prefixing guild-only inputs with `guild:` during resolution. (#12326) Thanks @headswim. - Config: preserve `${VAR}` env references when writing config files so `openclaw config set/apply/patch` does not persist secrets to disk. Thanks @thewilloftheshadow. +- Config: remove a cross-request env-snapshot race in config writes by carrying read-time env context into write calls per request, preserving `${VAR}` refs safely under concurrent gateway config mutations. (#11560) Thanks @akoscz. - Config: log overwrite audit entries (path, backup target, and hash transition) whenever an existing config file is replaced, improving traceability for unexpected config clobbers. - Process/Exec: avoid shell execution for `.exe` commands on Windows so env overrides work reliably in `runCommandWithTimeout`. Thanks @thewilloftheshadow. - Web tools/web_fetch: prefer `text/markdown` responses for Cloudflare Markdown for Agents, add `cf-markdown` extraction for markdown bodies, and redact fetched URLs in `x-markdown-tokens` debug logs to avoid leaking raw paths/query params. (#15376) Thanks @Yaxuan42. diff --git a/src/config/config.ts b/src/config/config.ts index db3091c5f0..a20d9495b0 100644 --- a/src/config/config.ts +++ b/src/config/config.ts @@ -4,6 +4,7 @@ export { loadConfig, parseConfigJson5, readConfigFileSnapshot, + readConfigFileSnapshotForWrite, resolveConfigSnapshotHash, writeConfigFile, } from "./io.js"; diff --git a/src/config/env-preserve-io.test.ts b/src/config/env-preserve-io.test.ts new file mode 100644 index 0000000000..5773218c83 --- /dev/null +++ b/src/config/env-preserve-io.test.ts @@ -0,0 +1,178 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { describe, it, expect } from "vitest"; +import { + createConfigIO, + readConfigFileSnapshotForWrite, + writeConfigFile as writeConfigFileViaWrapper, +} from "./io.js"; + +async function withTempConfig( + configContent: string, + run: (configPath: string) => Promise, +): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-env-io-")); + const configPath = path.join(dir, "openclaw.json"); + await fs.writeFile(configPath, configContent); + try { + await run(configPath); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } +} + +describe("env snapshot TOCTOU via createConfigIO", () => { + it("restores env refs using read-time env even after env mutation", async () => { + const env: Record = { + MY_API_KEY: "original-key-123", + }; + + const configJson = JSON.stringify({ gateway: { remote: { token: "${MY_API_KEY}" } } }, null, 2); + + await withTempConfig(configJson, async (configPath) => { + // Instance A: read config (captures env snapshot) + const ioA = createConfigIO({ configPath, env: env as unknown as NodeJS.ProcessEnv }); + const snapshot = await ioA.readConfigFileSnapshot(); + expect(snapshot.config.gateway?.remote?.token).toBe("original-key-123"); + + // Mutate env between read and write + env.MY_API_KEY = "mutated-key-456"; + + // Instance B: write config, but inject snapshot from A + const ioB = createConfigIO({ configPath, env: env as unknown as NodeJS.ProcessEnv }); + const envSnapshot = ioA.getEnvSnapshot(); + expect(envSnapshot).not.toBeNull(); + ioB.setEnvSnapshot(envSnapshot!); + + // Write the resolved config back — should restore ${MY_API_KEY} + await ioB.writeConfigFile(snapshot.config); + + // Verify the written file still has ${MY_API_KEY}, not the resolved value + const written = await fs.readFile(configPath, "utf-8"); + const parsed = JSON.parse(written); + expect(parsed.gateway.remote.token).toBe("${MY_API_KEY}"); + }); + }); + + it("without snapshot bridging, mutated env causes incorrect restoration", async () => { + const env: Record = { + MY_API_KEY: "original-key-123", + }; + + const configJson = JSON.stringify({ gateway: { remote: { token: "${MY_API_KEY}" } } }, null, 2); + + await withTempConfig(configJson, async (configPath) => { + // Instance A: read config + const ioA = createConfigIO({ configPath, env: env as unknown as NodeJS.ProcessEnv }); + const snapshot = await ioA.readConfigFileSnapshot(); + + // Mutate env + env.MY_API_KEY = "mutated-key-456"; + + // Instance B: write WITHOUT snapshot bridging (simulates the old bug) + const ioB = createConfigIO({ configPath, env: env as unknown as NodeJS.ProcessEnv }); + // No setEnvSnapshot — ioB uses live env + + await ioB.writeConfigFile(snapshot.config); + + // The written file should have the raw value because the live env + // no longer matches — restoreEnvVarRefs won't find a match + const written = await fs.readFile(configPath, "utf-8"); + const parsed = JSON.parse(written); + // Without snapshot, the resolved value "original-key-123" doesn't match + // live env "mutated-key-456", so restoration fails — value is written as-is + expect(parsed.gateway.remote.token).toBe("original-key-123"); + }); + }); +}); + +describe("env snapshot TOCTOU via wrapper APIs", () => { + it("uses explicit read context even if another read interleaves", async () => { + const prevConfigPath = process.env.OPENCLAW_CONFIG_PATH; + const prevCacheDisabled = process.env.OPENCLAW_DISABLE_CONFIG_CACHE; + const prevToken = process.env.MY_API_KEY; + const configJson = JSON.stringify({ gateway: { remote: { token: "${MY_API_KEY}" } } }, null, 2); + + try { + await withTempConfig(configJson, async (configPath) => { + process.env.OPENCLAW_CONFIG_PATH = configPath; + process.env.OPENCLAW_DISABLE_CONFIG_CACHE = "1"; + process.env.MY_API_KEY = "original-key-123"; + const firstRead = await readConfigFileSnapshotForWrite(); + expect(firstRead.snapshot.config.gateway?.remote?.token).toBe("original-key-123"); + + // Interleaving read from another request context with a different env value. + process.env.MY_API_KEY = "mutated-key-456"; + const secondRead = await readConfigFileSnapshotForWrite(); + expect(secondRead.snapshot.config.gateway?.remote?.token).toBe("mutated-key-456"); + + // Write using the first read's explicit context. + await writeConfigFileViaWrapper(firstRead.snapshot.config, firstRead.writeOptions); + const written = await fs.readFile(configPath, "utf-8"); + const parsed = JSON.parse(written); + expect(parsed.gateway.remote.token).toBe("${MY_API_KEY}"); + }); + } finally { + if (prevConfigPath === undefined) { + delete process.env.OPENCLAW_CONFIG_PATH; + } else { + process.env.OPENCLAW_CONFIG_PATH = prevConfigPath; + } + if (prevCacheDisabled === undefined) { + delete process.env.OPENCLAW_DISABLE_CONFIG_CACHE; + } else { + process.env.OPENCLAW_DISABLE_CONFIG_CACHE = prevCacheDisabled; + } + if (prevToken === undefined) { + delete process.env.MY_API_KEY; + } else { + process.env.MY_API_KEY = prevToken; + } + } + }); + + it("ignores read context when expected config path does not match", async () => { + const prevConfigPath = process.env.OPENCLAW_CONFIG_PATH; + const prevCacheDisabled = process.env.OPENCLAW_DISABLE_CONFIG_CACHE; + const prevToken = process.env.MY_API_KEY; + const configJson = JSON.stringify({ gateway: { remote: { token: "${MY_API_KEY}" } } }, null, 2); + + try { + await withTempConfig(configJson, async (configPath) => { + process.env.OPENCLAW_CONFIG_PATH = configPath; + process.env.OPENCLAW_DISABLE_CONFIG_CACHE = "1"; + process.env.MY_API_KEY = "original-key-123"; + const firstRead = await readConfigFileSnapshotForWrite(); + expect(firstRead.snapshot.config.gateway?.remote?.token).toBe("original-key-123"); + expect(firstRead.writeOptions.expectedConfigPath).toBe(configPath); + + process.env.MY_API_KEY = "mutated-key-456"; + await writeConfigFileViaWrapper(firstRead.snapshot.config, { + ...firstRead.writeOptions, + expectedConfigPath: `${configPath}.different`, + }); + + const written = await fs.readFile(configPath, "utf-8"); + const parsed = JSON.parse(written); + expect(parsed.gateway.remote.token).toBe("original-key-123"); + }); + } finally { + if (prevConfigPath === undefined) { + delete process.env.OPENCLAW_CONFIG_PATH; + } else { + process.env.OPENCLAW_CONFIG_PATH = prevConfigPath; + } + if (prevCacheDisabled === undefined) { + delete process.env.OPENCLAW_DISABLE_CONFIG_CACHE; + } else { + process.env.OPENCLAW_DISABLE_CONFIG_CACHE = prevCacheDisabled; + } + if (prevToken === undefined) { + delete process.env.MY_API_KEY; + } else { + process.env.MY_API_KEY = prevToken; + } + } + }); +}); diff --git a/src/config/env-preserve.test.ts b/src/config/env-preserve.test.ts new file mode 100644 index 0000000000..a3f9d63e4a --- /dev/null +++ b/src/config/env-preserve.test.ts @@ -0,0 +1,182 @@ +import { describe, it, expect } from "vitest"; +import { restoreEnvVarRefs } from "./env-preserve.js"; + +describe("restoreEnvVarRefs", () => { + const env = { + ANTHROPIC_API_KEY: "sk-ant-api03-real-key", + OPENAI_API_KEY: "sk-openai-real-key", + MY_TOKEN: "tok-12345", + } as unknown as NodeJS.ProcessEnv; + + it("restores a simple ${VAR} reference when value matches", () => { + const incoming = { apiKey: "sk-ant-api03-real-key" }; + const parsed = { apiKey: "${ANTHROPIC_API_KEY}" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ apiKey: "${ANTHROPIC_API_KEY}" }); + }); + + it("keeps new value when caller intentionally changed it", () => { + const incoming = { apiKey: "sk-ant-new-different-key" }; + const parsed = { apiKey: "${ANTHROPIC_API_KEY}" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ apiKey: "sk-ant-new-different-key" }); + }); + + it("handles nested objects", () => { + const incoming = { + models: { + providers: { + anthropic: { apiKey: "sk-ant-api03-real-key" }, + openai: { apiKey: "sk-openai-real-key" }, + }, + }, + }; + const parsed = { + models: { + providers: { + anthropic: { apiKey: "${ANTHROPIC_API_KEY}" }, + openai: { apiKey: "${OPENAI_API_KEY}" }, + }, + }, + }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ + models: { + providers: { + anthropic: { apiKey: "${ANTHROPIC_API_KEY}" }, + openai: { apiKey: "${OPENAI_API_KEY}" }, + }, + }, + }); + }); + + it("preserves new keys not in parsed", () => { + const incoming = { apiKey: "sk-ant-api03-real-key", newField: "hello" }; + const parsed = { apiKey: "${ANTHROPIC_API_KEY}" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ apiKey: "${ANTHROPIC_API_KEY}", newField: "hello" }); + }); + + it("handles non-env-var strings (no restoration needed)", () => { + const incoming = { name: "my-config" }; + const parsed = { name: "my-config" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ name: "my-config" }); + }); + + it("handles arrays", () => { + const incoming = ["sk-ant-api03-real-key", "literal"]; + const parsed = ["${ANTHROPIC_API_KEY}", "literal"]; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual(["${ANTHROPIC_API_KEY}", "literal"]); + }); + + it("handles null/undefined parsed gracefully", () => { + const incoming = { apiKey: "sk-ant-api03-real-key" }; + expect(restoreEnvVarRefs(incoming, null, env)).toEqual(incoming); + expect(restoreEnvVarRefs(incoming, undefined, env)).toEqual(incoming); + }); + + it("handles missing env var (cannot verify match)", () => { + const envMissing = {} as unknown as NodeJS.ProcessEnv; + const incoming = { apiKey: "some-value" }; + const parsed = { apiKey: "${MISSING_VAR}" }; + // Can't resolve the template, so keep incoming as-is + const result = restoreEnvVarRefs(incoming, parsed, envMissing); + expect(result).toEqual({ apiKey: "some-value" }); + }); + + it("handles composite template strings like prefix-${VAR}-suffix", () => { + const incoming = { url: "https://tok-12345.example.com" }; + const parsed = { url: "https://${MY_TOKEN}.example.com" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ url: "https://${MY_TOKEN}.example.com" }); + }); + + it("handles type mismatches between incoming and parsed", () => { + // Caller changed type from string to number + const incoming = { port: 8080 }; + const parsed = { port: "8080" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ port: 8080 }); + }); + + it("does not restore when parsed value has no env var pattern", () => { + const incoming = { apiKey: "sk-ant-api03-real-key" }; + const parsed = { apiKey: "sk-ant-api03-real-key" }; + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ apiKey: "sk-ant-api03-real-key" }); + }); + + // Edge case: env mutation between read and write (Greptile comment #1) + // Scenario: config.env sets FOO=bar, which gets applied to process.env during loadConfig. + // Later writeConfigFile runs — the env has changed since the original read. + it("does not incorrectly restore when env var value changed between read and write", () => { + // At read time, MY_VAR was "original-value" and resolved ${MY_VAR} → "original-value" + // Then config.env or external mutation changed MY_VAR to "mutated-value" + // Caller is writing back "original-value" (the value they got from the read) + const mutatedEnv = { MY_VAR: "mutated-value" } as unknown as NodeJS.ProcessEnv; + const incoming = { key: "original-value" }; + const parsed = { key: "${MY_VAR}" }; + + const result = restoreEnvVarRefs(incoming, parsed, mutatedEnv); + // Should NOT restore ${MY_VAR} because resolving it now gives "mutated-value", + // which doesn't match "original-value" — the caller's value should be kept + expect(result).toEqual({ key: "original-value" }); + }); + + it("correctly restores when env var value hasn't changed", () => { + const stableEnv = { MY_VAR: "stable-value" } as unknown as NodeJS.ProcessEnv; + const incoming = { key: "stable-value" }; + const parsed = { key: "${MY_VAR}" }; + + const result = restoreEnvVarRefs(incoming, parsed, stableEnv); + // Env value matches incoming — safe to restore + expect(result).toEqual({ key: "${MY_VAR}" }); + }); + + it("does not restore when env snapshot differs from live env (TOCTOU fix)", () => { + // With env snapshots: at read time MY_VAR was "old-value", so incoming is "old-value". + // Caller changed it to "new-value". Live env also changed to "new-value". + // But using the READ-TIME snapshot ("old-value"), we correctly see mismatch and keep incoming. + const readTimeEnv = { MY_VAR: "old-value" } as unknown as NodeJS.ProcessEnv; + const incoming = { key: "new-value" }; // caller intentionally changed this + const parsed = { key: "${MY_VAR}" }; + + const result = restoreEnvVarRefs(incoming, parsed, readTimeEnv); + // Using read-time snapshot: ${MY_VAR} resolves to "old-value", doesn't match "new-value" + // → correctly keeps caller's new value + expect(result).toEqual({ key: "new-value" }); + }); + + // Edge case: $${VAR} escape sequence (Greptile comment #2) + it("handles $${VAR} escape sequence (literal ${VAR} in output)", () => { + // In the config file: $${ANTHROPIC_API_KEY} + // substituteString resolves this to literal "${ANTHROPIC_API_KEY}" + // So incoming would be "${ANTHROPIC_API_KEY}" (the literal text) + const incoming = { note: "${ANTHROPIC_API_KEY}" }; + const parsed = { note: "$${ANTHROPIC_API_KEY}" }; + + const result = restoreEnvVarRefs(incoming, parsed, env); + // Should restore the $${} escape, not try to resolve ${} inside it + expect(result).toEqual({ note: "$${ANTHROPIC_API_KEY}" }); + }); + + it("does not confuse $${VAR} escape with ${VAR} substitution", () => { + // Config has both: an escaped ref and a real ref + const incoming = { + literal: "${MY_TOKEN}", // from $${MY_TOKEN} → literal "${MY_TOKEN}" + resolved: "tok-12345", // from ${MY_TOKEN} → "tok-12345" + }; + const parsed = { + literal: "$${MY_TOKEN}", // escape sequence + resolved: "${MY_TOKEN}", // real env var ref + }; + + const result = restoreEnvVarRefs(incoming, parsed, env); + expect(result).toEqual({ + literal: "$${MY_TOKEN}", // should restore escape + resolved: "${MY_TOKEN}", // should restore ref + }); + }); +}); diff --git a/src/config/env-preserve.ts b/src/config/env-preserve.ts new file mode 100644 index 0000000000..a882357b9e --- /dev/null +++ b/src/config/env-preserve.ts @@ -0,0 +1,141 @@ +/** + * Preserves `${VAR}` environment variable references during config write-back. + * + * When config is read, `${VAR}` references are resolved to their values. + * When writing back, callers pass the resolved config. This module detects + * values that match what a `${VAR}` reference would resolve to and restores + * the original reference, so env var references survive config round-trips. + * + * A value is restored only if: + * 1. The pre-substitution value contained a `${VAR}` pattern + * 2. Resolving that pattern with current env vars produces the incoming value + * + * If a caller intentionally set a new value (different from what the env var + * resolves to), the new value is kept as-is. + */ + +const ENV_VAR_PATTERN = /\$\{[A-Z_][A-Z0-9_]*\}/; + +function isPlainObject(value: unknown): value is Record { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + Object.prototype.toString.call(value) === "[object Object]" + ); +} + +/** + * Check if a string contains any `${VAR}` env var references. + */ +function hasEnvVarRef(value: string): boolean { + return ENV_VAR_PATTERN.test(value); +} + +/** + * Resolve `${VAR}` references in a single string using the given env. + * Returns null if any referenced var is missing (instead of throwing). + * + * Mirrors the substitution semantics of `substituteString` in env-substitution.ts: + * - `${VAR}` → env value (returns null if missing) + * - `$${VAR}` → literal `${VAR}` (escape sequence) + */ +function tryResolveString(template: string, env: NodeJS.ProcessEnv): string | null { + const ENV_VAR_NAME = /^[A-Z_][A-Z0-9_]*$/; + const chunks: string[] = []; + + for (let i = 0; i < template.length; i++) { + if (template[i] === "$") { + // Escaped: $${VAR} -> literal ${VAR} + if (template[i + 1] === "$" && template[i + 2] === "{") { + const start = i + 3; + const end = template.indexOf("}", start); + if (end !== -1) { + const name = template.slice(start, end); + if (ENV_VAR_NAME.test(name)) { + chunks.push(`\${${name}}`); + i = end; + continue; + } + } + } + + // Substitution: ${VAR} -> env value + if (template[i + 1] === "{") { + const start = i + 2; + const end = template.indexOf("}", start); + if (end !== -1) { + const name = template.slice(start, end); + if (ENV_VAR_NAME.test(name)) { + const val = env[name]; + if (val === undefined || val === "") { + return null; + } + chunks.push(val); + i = end; + continue; + } + } + } + } + chunks.push(template[i]); + } + + return chunks.join(""); +} + +/** + * Deep-walk the incoming config and restore `${VAR}` references from the + * pre-substitution parsed config wherever the resolved value matches. + * + * @param incoming - The resolved config about to be written + * @param parsed - The pre-substitution parsed config (from the current file on disk) + * @param env - Environment variables for verification + * @returns A new config object with env var references restored where appropriate + */ +export function restoreEnvVarRefs( + incoming: unknown, + parsed: unknown, + env: NodeJS.ProcessEnv = process.env, +): unknown { + // If parsed has no env var refs at this level, return incoming as-is + if (parsed === null || parsed === undefined) { + return incoming; + } + + // String leaf: check if parsed was a ${VAR} template that resolves to incoming + if (typeof incoming === "string" && typeof parsed === "string") { + if (hasEnvVarRef(parsed)) { + const resolved = tryResolveString(parsed, env); + if (resolved === incoming) { + // The incoming value matches what the env var resolves to — restore the reference + return parsed; + } + } + return incoming; + } + + // Arrays: walk element by element + if (Array.isArray(incoming) && Array.isArray(parsed)) { + return incoming.map((item, i) => + i < parsed.length ? restoreEnvVarRefs(item, parsed[i], env) : item, + ); + } + + // Objects: walk key by key + if (isPlainObject(incoming) && isPlainObject(parsed)) { + const result: Record = {}; + for (const [key, value] of Object.entries(incoming)) { + if (key in parsed) { + result[key] = restoreEnvVarRefs(value, parsed[key], env); + } else { + // New key added by caller — keep as-is + result[key] = value; + } + } + return result; + } + + // Mismatched types or primitives — keep incoming + return incoming; +} diff --git a/src/config/io.ts b/src/config/io.ts index 64434a5a11..f04fca853b 100644 --- a/src/config/io.ts +++ b/src/config/io.ts @@ -25,6 +25,7 @@ import { applySessionDefaults, applyTalkApiKey, } from "./defaults.js"; +import { restoreEnvVarRefs } from "./env-preserve.js"; import { MissingEnvVarError, containsEnvVarReference, @@ -70,6 +71,23 @@ const CONFIG_BACKUP_COUNT = 5; const loggedInvalidConfigs = new Set(); export type ParseConfigJson5Result = { ok: true; parsed: unknown } | { ok: false; error: string }; +export type ConfigWriteOptions = { + /** + * Read-time env snapshot used to validate `${VAR}` restoration decisions. + * If omitted, write falls back to current process env. + */ + envSnapshotForRestore?: Record; + /** + * Optional safety check: only use envSnapshotForRestore when writing the + * same config file path that produced the snapshot. + */ + expectedConfigPath?: string; +}; + +export type ReadConfigFileSnapshotForWriteResult = { + snapshot: ConfigFileSnapshot; + writeOptions: ConfigWriteOptions; +}; function hashConfigRaw(raw: string | null): string { return crypto @@ -399,6 +417,11 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { const configPath = candidatePaths.find((candidate) => deps.fs.existsSync(candidate)) ?? requestedConfigPath; + // Snapshot of env vars captured after applyConfigEnv + resolveConfigEnvVars. + // Used by writeConfigFile to verify ${VAR} restoration against the env state + // that produced the resolved config, not the (possibly mutated) live env. + let envSnapshotForRestore: Record | null = null; + function loadConfig(): OpenClawConfig { try { maybeLoadDotEnvForConfig(deps.env); @@ -431,6 +454,11 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { // Substitute ${VAR} env var references const substituted = resolveConfigEnvVars(resolved, deps.env); + // Capture env snapshot after substitution for use by writeConfigFile. + // This ensures restoreEnvVarRefs verifies against the env that produced + // the resolved values, not a potentially mutated live env (TOCTOU fix). + envSnapshotForRestore = { ...deps.env } as Record; + const resolvedConfig = substituted; warnOnConfigMiskeys(resolvedConfig, deps.logger); if (typeof resolvedConfig !== "object" || resolvedConfig === null) { @@ -597,6 +625,8 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { let substituted: unknown; try { substituted = resolveConfigEnvVars(resolved, deps.env); + // Capture env snapshot (same as loadConfig — see TOCTOU comment above) + envSnapshotForRestore = { ...deps.env } as Record; } catch (err) { const message = err instanceof MissingEnvVarError @@ -681,9 +711,15 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { async function writeConfigFile(cfg: OpenClawConfig) { clearConfigCache(); let persistCandidate: unknown = cfg; + // Save the injected env snapshot before readConfigFileSnapshot() overwrites it + // with a new snapshot based on the (possibly mutated) live env. + const savedEnvSnapshot = envSnapshotForRestore; const snapshot = await readConfigFileSnapshot(); let envRefMap: Map | null = null; let changedPaths: Set | null = null; + if (savedEnvSnapshot) { + envSnapshotForRestore = savedEnvSnapshot; + } if (snapshot.valid && snapshot.exists) { const patch = createMergePatch(snapshot.config, cfg); persistCandidate = applyMergePatch(snapshot.resolved, patch); @@ -716,12 +752,43 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { .join("\n"); deps.logger.warn(`Config warnings:\n${details}`); } + + // Restore ${VAR} env var references that were resolved during config loading. + // Read the current file (pre-substitution) and restore any references whose + // resolved values match the incoming config — so we don't overwrite + // "${ANTHROPIC_API_KEY}" with "sk-ant-..." when the caller didn't change it. + // + // We use only the root file's parsed content (no $include resolution) to avoid + // pulling values from included files into the root config on write-back. + // Apply env restoration to validated.config (which has runtime defaults stripped + // per issue #6070) rather than the raw caller input. + let cfgToWrite = validated.config; + try { + if (deps.fs.existsSync(configPath)) { + const currentRaw = await deps.fs.promises.readFile(configPath, "utf-8"); + const parsedRes = parseConfigJson5(currentRaw, deps.json5); + if (parsedRes.ok) { + // Use env snapshot from when config was loaded (if available) to avoid + // TOCTOU issues where env changes between load and write. Falls back to + // live env if no snapshot exists (e.g., first write before any load). + const envForRestore = envSnapshotForRestore ?? deps.env; + cfgToWrite = restoreEnvVarRefs( + cfgToWrite, + parsedRes.parsed, + envForRestore, + ) as OpenClawConfig; + } + } + } catch { + // If reading the current file fails, write cfg as-is (no env restoration) + } + const dir = path.dirname(configPath); await deps.fs.promises.mkdir(dir, { recursive: true, mode: 0o700 }); const outputConfig = envRefMap && changedPaths - ? (restoreEnvRefsFromMap(validated.config, "", envRefMap, changedPaths) as OpenClawConfig) - : validated.config; + ? (restoreEnvRefsFromMap(cfgToWrite, "", envRefMap, changedPaths) as OpenClawConfig) + : cfgToWrite; // Do NOT apply runtime defaults when writing — user config should only contain // explicitly set values. Runtime defaults are applied when loading (issue #6070). const json = JSON.stringify(stampConfigVersion(outputConfig), null, 2).trimEnd().concat("\n"); @@ -785,6 +852,14 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { loadConfig, readConfigFileSnapshot, writeConfigFile, + /** Return the env snapshot captured during the last loadConfig/readConfigFileSnapshot, or null. */ + getEnvSnapshot(): Record | null { + return envSnapshotForRestore; + }, + /** Inject an env snapshot (e.g. from a prior IO instance) for use by writeConfigFile. */ + setEnvSnapshot(snapshot: Record): void { + envSnapshotForRestore = snapshot; + }, }; } @@ -852,7 +927,28 @@ export async function readConfigFileSnapshot(): Promise { return await createConfigIO().readConfigFileSnapshot(); } -export async function writeConfigFile(cfg: OpenClawConfig): Promise { - clearConfigCache(); - await createConfigIO().writeConfigFile(cfg); +export async function readConfigFileSnapshotForWrite(): Promise { + const io = createConfigIO(); + const snapshot = await io.readConfigFileSnapshot(); + return { + snapshot, + writeOptions: { + envSnapshotForRestore: io.getEnvSnapshot() ?? undefined, + expectedConfigPath: io.configPath, + }, + }; +} + +export async function writeConfigFile( + cfg: OpenClawConfig, + options: ConfigWriteOptions = {}, +): Promise { + clearConfigCache(); + const io = createConfigIO(); + const sameConfigPath = + options.expectedConfigPath === undefined || options.expectedConfigPath === io.configPath; + if (sameConfigPath && options.envSnapshotForRestore) { + io.setEnvSnapshot(options.envSnapshotForRestore); + } + await io.writeConfigFile(cfg); } diff --git a/src/gateway/server-methods/config.ts b/src/gateway/server-methods/config.ts index 2e397728c6..30ba6bc983 100644 --- a/src/gateway/server-methods/config.ts +++ b/src/gateway/server-methods/config.ts @@ -6,6 +6,7 @@ import { loadConfig, parseConfigJson5, readConfigFileSnapshot, + readConfigFileSnapshotForWrite, resolveConfigSnapshotHash, validateConfigObjectWithPlugins, writeConfigFile, @@ -170,7 +171,7 @@ export const configHandlers: GatewayRequestHandlers = { ); return; } - const snapshot = await readConfigFileSnapshot(); + const { snapshot, writeOptions } = await readConfigFileSnapshotForWrite(); if (!requireConfigBaseHash(params, snapshot, respond)) { return; } @@ -209,7 +210,7 @@ export const configHandlers: GatewayRequestHandlers = { ); return; } - await writeConfigFile(validated.config); + await writeConfigFile(validated.config, writeOptions); respond( true, { @@ -232,7 +233,7 @@ export const configHandlers: GatewayRequestHandlers = { ); return; } - const snapshot = await readConfigFileSnapshot(); + const { snapshot, writeOptions } = await readConfigFileSnapshotForWrite(); if (!requireConfigBaseHash(params, snapshot, respond)) { return; } @@ -300,7 +301,7 @@ export const configHandlers: GatewayRequestHandlers = { ); return; } - await writeConfigFile(validated.config); + await writeConfigFile(validated.config, writeOptions); const sessionKey = typeof (params as { sessionKey?: unknown }).sessionKey === "string" @@ -371,7 +372,7 @@ export const configHandlers: GatewayRequestHandlers = { ); return; } - const snapshot = await readConfigFileSnapshot(); + const { snapshot, writeOptions } = await readConfigFileSnapshotForWrite(); if (!requireConfigBaseHash(params, snapshot, respond)) { return; } @@ -413,7 +414,7 @@ export const configHandlers: GatewayRequestHandlers = { ); return; } - await writeConfigFile(validated.config); + await writeConfigFile(validated.config, writeOptions); const sessionKey = typeof (params as { sessionKey?: unknown }).sessionKey === "string"