diff --git a/src/config/env-preserve-io.test.ts b/src/config/env-preserve-io.test.ts index 5773218c83..5e2d29a31b 100644 --- a/src/config/env-preserve-io.test.ts +++ b/src/config/env-preserve-io.test.ts @@ -22,6 +22,35 @@ async function withTempConfig( } } +async function withEnvOverrides( + updates: Record, + run: () => Promise, +): Promise { + const previous = new Map(); + for (const key of Object.keys(updates)) { + previous.set(key, process.env[key]); + } + + try { + for (const [key, value] of Object.entries(updates)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + await run(); + } finally { + for (const [key, value] of previous.entries()) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + } +} + describe("env snapshot TOCTOU via createConfigIO", () => { it("restores env refs using read-time env even after env mutation", async () => { const env: Record = { @@ -33,20 +62,17 @@ describe("env snapshot TOCTOU via createConfigIO", () => { 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"); + const firstRead = await ioA.readConfigFileSnapshotForWrite(); + expect(firstRead.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 + // Instance B: write config using explicit read context 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); + await ioB.writeConfigFile(firstRead.snapshot.config, firstRead.writeOptions); // Verify the written file still has ${MY_API_KEY}, not the resolved value const written = await fs.readFile(configPath, "utf-8"); @@ -72,7 +98,7 @@ describe("env snapshot TOCTOU via createConfigIO", () => { // 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 + // No explicit writeOptions — ioB uses live env await ioB.writeConfigFile(snapshot.config); @@ -89,90 +115,58 @@ describe("env snapshot TOCTOU via createConfigIO", () => { 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); + await withTempConfig(configJson, async (configPath) => { + await withEnvOverrides( + { + OPENCLAW_CONFIG_PATH: configPath, + OPENCLAW_DISABLE_CONFIG_CACHE: "1", + MY_API_KEY: "original-key-123", + }, + async () => { + const firstRead = await readConfigFileSnapshotForWrite(); + expect(firstRead.snapshot.config.gateway?.remote?.token).toBe("original-key-123"); - 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"); - // 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; - } - } + // 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}"); + }, + ); + }); }); 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); + await withTempConfig(configJson, async (configPath) => { + await withEnvOverrides( + { + OPENCLAW_CONFIG_PATH: configPath, + OPENCLAW_DISABLE_CONFIG_CACHE: "1", + MY_API_KEY: "original-key-123", + }, + async () => { + const firstRead = await readConfigFileSnapshotForWrite(); + expect(firstRead.snapshot.config.gateway?.remote?.token).toBe("original-key-123"); + expect(firstRead.writeOptions.expectedConfigPath).toBe(configPath); - 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`, + }); - 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; - } - } + const written = await fs.readFile(configPath, "utf-8"); + const parsed = JSON.parse(written); + expect(parsed.gateway.remote.token).toBe("original-key-123"); + }, + ); + }); }); }); diff --git a/src/config/io.ts b/src/config/io.ts index f04fca853b..abfd3124c3 100644 --- a/src/config/io.ts +++ b/src/config/io.ts @@ -408,6 +408,43 @@ export function parseConfigJson5( } } +type ConfigReadResolution = { + resolvedConfigRaw: unknown; + envSnapshotForRestore: Record; +}; + +function resolveConfigIncludesForRead( + parsed: unknown, + configPath: string, + deps: Required, +): unknown { + return resolveConfigIncludes(parsed, configPath, { + readFile: (candidate) => deps.fs.readFileSync(candidate, "utf-8"), + parseJson: (raw) => deps.json5.parse(raw), + }); +} + +function resolveConfigForRead( + resolvedIncludes: unknown, + env: NodeJS.ProcessEnv, +): ConfigReadResolution { + // Apply config.env to process.env BEFORE substitution so ${VAR} can reference config-defined vars. + if (resolvedIncludes && typeof resolvedIncludes === "object" && "env" in resolvedIncludes) { + applyConfigEnv(resolvedIncludes as OpenClawConfig, env); + } + + return { + resolvedConfigRaw: resolveConfigEnvVars(resolvedIncludes, env), + // Capture env snapshot after substitution for write-time ${VAR} restoration. + envSnapshotForRestore: { ...env } as Record, + }; +} + +type ReadConfigFileSnapshotInternalResult = { + snapshot: ConfigFileSnapshot; + envSnapshotForRestore?: Record; +}; + export function createConfigIO(overrides: ConfigIoDeps = {}) { const deps = normalizeDeps(overrides); const requestedConfigPath = resolveConfigPathForDeps(deps); @@ -417,11 +454,6 @@ 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); @@ -439,27 +471,10 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { } const raw = deps.fs.readFileSync(configPath, "utf-8"); const parsed = deps.json5.parse(raw); - - // Resolve $include directives before validation - const resolved = resolveConfigIncludes(parsed, configPath, { - readFile: (p) => deps.fs.readFileSync(p, "utf-8"), - parseJson: (raw) => deps.json5.parse(raw), - }); - - // Apply config.env to process.env BEFORE substitution so ${VAR} can reference config-defined vars - if (resolved && typeof resolved === "object" && "env" in resolved) { - applyConfigEnv(resolved as OpenClawConfig, deps.env); - } - - // 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; + const { resolvedConfigRaw: resolvedConfig } = resolveConfigForRead( + resolveConfigIncludesForRead(parsed, configPath, deps), + deps.env, + ); warnOnConfigMiskeys(resolvedConfig, deps.logger); if (typeof resolvedConfig !== "object" || resolvedConfig === null) { return {}; @@ -539,7 +554,7 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { } } - async function readConfigFileSnapshot(): Promise { + async function readConfigFileSnapshotInternal(): Promise { maybeLoadDotEnvForConfig(deps.env); const exists = deps.fs.existsSync(configPath); if (!exists) { @@ -555,17 +570,19 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { ); const legacyIssues: LegacyConfigIssue[] = []; return { - path: configPath, - exists: false, - raw: null, - parsed: {}, - resolved: {}, - valid: true, - config, - hash, - issues: [], - warnings: [], - legacyIssues, + snapshot: { + path: configPath, + exists: false, + raw: null, + parsed: {}, + resolved: {}, + valid: true, + config, + hash, + issues: [], + warnings: [], + legacyIssues, + }, }; } @@ -575,151 +592,165 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { const parsedRes = parseConfigJson5(raw, deps.json5); if (!parsedRes.ok) { return { - path: configPath, - exists: true, - raw, - parsed: {}, - resolved: {}, - valid: false, - config: {}, - hash, - issues: [{ path: "", message: `JSON5 parse failed: ${parsedRes.error}` }], - warnings: [], - legacyIssues: [], + snapshot: { + path: configPath, + exists: true, + raw, + parsed: {}, + resolved: {}, + valid: false, + config: {}, + hash, + issues: [{ path: "", message: `JSON5 parse failed: ${parsedRes.error}` }], + warnings: [], + legacyIssues: [], + }, }; } // Resolve $include directives let resolved: unknown; try { - resolved = resolveConfigIncludes(parsedRes.parsed, configPath, { - readFile: (p) => deps.fs.readFileSync(p, "utf-8"), - parseJson: (raw) => deps.json5.parse(raw), - }); + resolved = resolveConfigIncludesForRead(parsedRes.parsed, configPath, deps); } catch (err) { const message = err instanceof ConfigIncludeError ? err.message : `Include resolution failed: ${String(err)}`; return { - path: configPath, - exists: true, - raw, - parsed: parsedRes.parsed, - resolved: coerceConfig(parsedRes.parsed), - valid: false, - config: coerceConfig(parsedRes.parsed), - hash, - issues: [{ path: "", message }], - warnings: [], - legacyIssues: [], + snapshot: { + path: configPath, + exists: true, + raw, + parsed: parsedRes.parsed, + resolved: coerceConfig(parsedRes.parsed), + valid: false, + config: coerceConfig(parsedRes.parsed), + hash, + issues: [{ path: "", message }], + warnings: [], + legacyIssues: [], + }, }; } - // Apply config.env to process.env BEFORE substitution so ${VAR} can reference config-defined vars - if (resolved && typeof resolved === "object" && "env" in resolved) { - applyConfigEnv(resolved as OpenClawConfig, deps.env); - } - - // Substitute ${VAR} env var references - let substituted: unknown; + let readResolution: ConfigReadResolution; try { - substituted = resolveConfigEnvVars(resolved, deps.env); - // Capture env snapshot (same as loadConfig — see TOCTOU comment above) - envSnapshotForRestore = { ...deps.env } as Record; + readResolution = resolveConfigForRead(resolved, deps.env); } catch (err) { const message = err instanceof MissingEnvVarError ? err.message : `Env var substitution failed: ${String(err)}`; return { - path: configPath, - exists: true, - raw, - parsed: parsedRes.parsed, - resolved: coerceConfig(resolved), - valid: false, - config: coerceConfig(resolved), - hash, - issues: [{ path: "", message }], - warnings: [], - legacyIssues: [], + snapshot: { + path: configPath, + exists: true, + raw, + parsed: parsedRes.parsed, + resolved: coerceConfig(resolved), + valid: false, + config: coerceConfig(resolved), + hash, + issues: [{ path: "", message }], + warnings: [], + legacyIssues: [], + }, }; } - const resolvedConfigRaw = substituted; + const resolvedConfigRaw = readResolution.resolvedConfigRaw; const legacyIssues = findLegacyConfigIssues(resolvedConfigRaw); const validated = validateConfigObjectWithPlugins(resolvedConfigRaw); if (!validated.ok) { return { - path: configPath, - exists: true, - raw, - parsed: parsedRes.parsed, - resolved: coerceConfig(resolvedConfigRaw), - valid: false, - config: coerceConfig(resolvedConfigRaw), - hash, - issues: validated.issues, - warnings: validated.warnings, - legacyIssues, + snapshot: { + path: configPath, + exists: true, + raw, + parsed: parsedRes.parsed, + resolved: coerceConfig(resolvedConfigRaw), + valid: false, + config: coerceConfig(resolvedConfigRaw), + hash, + issues: validated.issues, + warnings: validated.warnings, + legacyIssues, + }, }; } warnIfConfigFromFuture(validated.config, deps.logger); return { - path: configPath, - exists: true, - raw, - parsed: parsedRes.parsed, - // Use resolvedConfigRaw (after $include and ${ENV} substitution but BEFORE runtime defaults) - // for config set/unset operations (issue #6070) - resolved: coerceConfig(resolvedConfigRaw), - valid: true, - config: normalizeConfigPaths( - applyTalkApiKey( - applyModelDefaults( - applyAgentDefaults( - applySessionDefaults(applyLoggingDefaults(applyMessageDefaults(validated.config))), + snapshot: { + path: configPath, + exists: true, + raw, + parsed: parsedRes.parsed, + // Use resolvedConfigRaw (after $include and ${ENV} substitution but BEFORE runtime defaults) + // for config set/unset operations (issue #6070) + resolved: coerceConfig(resolvedConfigRaw), + valid: true, + config: normalizeConfigPaths( + applyTalkApiKey( + applyModelDefaults( + applyAgentDefaults( + applySessionDefaults( + applyLoggingDefaults(applyMessageDefaults(validated.config)), + ), + ), ), ), ), - ), - hash, - issues: [], - warnings: validated.warnings, - legacyIssues, + hash, + issues: [], + warnings: validated.warnings, + legacyIssues, + }, + envSnapshotForRestore: readResolution.envSnapshotForRestore, }; } catch (err) { return { - path: configPath, - exists: true, - raw: null, - parsed: {}, - resolved: {}, - valid: false, - config: {}, - hash: hashConfigRaw(null), - issues: [{ path: "", message: `read failed: ${String(err)}` }], - warnings: [], - legacyIssues: [], + snapshot: { + path: configPath, + exists: true, + raw: null, + parsed: {}, + resolved: {}, + valid: false, + config: {}, + hash: hashConfigRaw(null), + issues: [{ path: "", message: `read failed: ${String(err)}` }], + warnings: [], + legacyIssues: [], + }, }; } } - async function writeConfigFile(cfg: OpenClawConfig) { + async function readConfigFileSnapshot(): Promise { + const result = await readConfigFileSnapshotInternal(); + return result.snapshot; + } + + async function readConfigFileSnapshotForWrite(): Promise { + const result = await readConfigFileSnapshotInternal(); + return { + snapshot: result.snapshot, + writeOptions: { + envSnapshotForRestore: result.envSnapshotForRestore, + expectedConfigPath: configPath, + }, + }; + } + + async function writeConfigFile(cfg: OpenClawConfig, options: ConfigWriteOptions = {}) { 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(); + const { snapshot } = await readConfigFileSnapshotInternal(); 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); @@ -771,7 +802,7 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { // 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; + const envForRestore = options.envSnapshotForRestore ?? deps.env; cfgToWrite = restoreEnvVarRefs( cfgToWrite, parsedRes.parsed, @@ -851,15 +882,8 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { configPath, loadConfig, readConfigFileSnapshot, + readConfigFileSnapshotForWrite, 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; - }, }; } @@ -928,27 +952,17 @@ export async function readConfigFileSnapshot(): Promise { } export async function readConfigFileSnapshotForWrite(): Promise { - const io = createConfigIO(); - const snapshot = await io.readConfigFileSnapshot(); - return { - snapshot, - writeOptions: { - envSnapshotForRestore: io.getEnvSnapshot() ?? undefined, - expectedConfigPath: io.configPath, - }, - }; + return await createConfigIO().readConfigFileSnapshotForWrite(); } 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); + await io.writeConfigFile(cfg, { + envSnapshotForRestore: sameConfigPath ? options.envSnapshotForRestore : undefined, + }); }