mirror of
https://github.com/openclaw/openclaw.git
synced 2026-02-19 18:39:20 -05:00
fix: preserve ${VAR} env var references when writing config back to disk (#11560)
* fix: preserve ${VAR} env var references when writing config back to disk
Fixes #11466
When config is loaded, ${VAR} references are resolved to their plaintext
values. Previously, writeConfigFile would serialize the resolved values,
silently replacing "${ANTHROPIC_API_KEY}" with "sk-ant-api03-..." in the
config file.
Now writeConfigFile reads the current file pre-substitution, and for each
value that matches what a ${VAR} reference would resolve to, restores the
original reference. Values the caller intentionally changed are kept as-is.
This fixes all 50+ writeConfigFile call sites (doctor, configure wizard,
gateway config.set/apply/patch, plugins, hooks, etc.) without requiring
any caller changes.
New files:
- src/config/env-preserve.ts — restoreEnvVarRefs() utility
- src/config/env-preserve.test.ts — 11 unit tests
* fix: remove global config env snapshot race
* docs(changelog): note config env snapshot race fix
---------
Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -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/<guildId>` 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.
|
||||
|
||||
@@ -4,6 +4,7 @@ export {
|
||||
loadConfig,
|
||||
parseConfigJson5,
|
||||
readConfigFileSnapshot,
|
||||
readConfigFileSnapshotForWrite,
|
||||
resolveConfigSnapshotHash,
|
||||
writeConfigFile,
|
||||
} from "./io.js";
|
||||
|
||||
178
src/config/env-preserve-io.test.ts
Normal file
178
src/config/env-preserve-io.test.ts
Normal file
@@ -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<void>,
|
||||
): Promise<void> {
|
||||
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<string, string> = {
|
||||
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<string, string> = {
|
||||
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;
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
182
src/config/env-preserve.test.ts
Normal file
182
src/config/env-preserve.test.ts
Normal file
@@ -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
|
||||
});
|
||||
});
|
||||
});
|
||||
141
src/config/env-preserve.ts
Normal file
141
src/config/env-preserve.ts
Normal file
@@ -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<string, unknown> {
|
||||
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<string, unknown> = {};
|
||||
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;
|
||||
}
|
||||
106
src/config/io.ts
106
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<string>();
|
||||
|
||||
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<string, string | undefined>;
|
||||
/**
|
||||
* 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<string, string | undefined> | 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<string, string | undefined>;
|
||||
|
||||
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<string, string | undefined>;
|
||||
} 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<string, string> | null = null;
|
||||
let changedPaths: Set<string> | 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<string, string | undefined> | null {
|
||||
return envSnapshotForRestore;
|
||||
},
|
||||
/** Inject an env snapshot (e.g. from a prior IO instance) for use by writeConfigFile. */
|
||||
setEnvSnapshot(snapshot: Record<string, string | undefined>): void {
|
||||
envSnapshotForRestore = snapshot;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -852,7 +927,28 @@ export async function readConfigFileSnapshot(): Promise<ConfigFileSnapshot> {
|
||||
return await createConfigIO().readConfigFileSnapshot();
|
||||
}
|
||||
|
||||
export async function writeConfigFile(cfg: OpenClawConfig): Promise<void> {
|
||||
clearConfigCache();
|
||||
await createConfigIO().writeConfigFile(cfg);
|
||||
export async function readConfigFileSnapshotForWrite(): Promise<ReadConfigFileSnapshotForWriteResult> {
|
||||
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<void> {
|
||||
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);
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user