Included configs may get overwritten

writeConfigFile now restores env-var templates by parsing only the root config file and (optionally) resolving $includes, but it always writes the result back to configPath only. If a value originally lived in an included file and was referenced from the root, this logic will still restore ${VAR} into the in-memory object and then serialize the entire resolved config into the root file, potentially overwriting the user’s include-based layout (and moving values from included files into the root on the next write path). This breaks setups that rely on $include for separation. Consider skipping restoration when the current file uses $include, or preserving/includes-aware write behavior instead of writing a fully-resolved object to the root.

This is a comment left during a code review.
Path: src/config/io.ts
Line: 496:516

Comment:
**Included configs may get overwritten**

`writeConfigFile` now restores env-var templates by parsing only the *root* config file and (optionally) resolving `$include`s, but it always writes the result back to `configPath` only. If a value originally lived in an included file and was referenced from the root, this logic will still restore `${VAR}` into the in-memory object and then serialize the *entire* resolved config into the root file, potentially overwriting the user’s include-based layout (and moving values from included files into the root on the next write path). This breaks setups that rely on `$include` for separation. Consider skipping restoration when the current file uses `$include`, or preserving/includes-aware write behavior instead of writing a fully-resolved object to the root.


How can I resolve this? If you propose a fix, please make it concise.

Fixed in 1d5dcfd — now using only the root file's parsed content (no $include resolution) for env var restoration. This avoids pulling values from included files into the root config on write-back. The $include flattening on write is a pre-existing issue with writeConfigFile itself, not introduced by this change.

Silent env changes on write

restoreEnvVarRefs verifies templates using deps.env at write time, but loadConfig can mutate deps.env via applyConfigEnv(...) (config-defined env keys) and that mutation can change between the original read and a later write. If a config file contains "${FOO}" and FOO is set via config.env (or changes in the process environment), a subsequent writeConfigFile can incorrectly restore "${FOO}" (or fail to restore it) based on the current env, not the env that produced cfg. This can make write-back depend on external env state rather than the caller’s intended value.

Consider verifying against the same env-substitution rules/state used when producing cfg (or passing an explicit env snapshot into writeConfigFile).

This is a comment left during a code review.
Path: src/config/io.ts
Line: 505:511

Comment:
**Silent env changes on write**

`restoreEnvVarRefs` verifies templates using `deps.env` at write time, but `loadConfig` can mutate `deps.env` via `applyConfigEnv(...)` (config-defined `env` keys) and that mutation can change between the original read and a later write. If a config file contains `"${FOO}"` and `FOO` is set via `config.env` (or changes in the process environment), a subsequent `writeConfigFile` can incorrectly restore `"${FOO}"` (or fail to restore it) based on the *current* env, not the env that produced `cfg`. This can make write-back depend on external env state rather than the caller’s intended value.

Consider verifying against the same env-substitution rules/state used when producing `cfg` (or passing an explicit env snapshot into `writeConfigFile`).

How can I resolve this? If you propose a fix, please make it concise.

Investigated this thoroughly. Wrote 3 targeted tests for the env mutation scenario:

1. Env mutated to different value — restoreEnvVarRefs correctly does NOT restore (resolved value no longer matches incoming). ✅
2. Env stable — correctly restores. ✅
3. Env mutates to coincidentally match incoming — This is the only problematic case: caller intentionally set "xyz789", but env also changed to "xyz789", so restoration incorrectly fires. However:

Why this is not practically exploitable:

• applyConfigEnv only sets vars that are NOT already set (if (env[key]?.trim()) continue)
• writeConfigFile runs immediately after config changes, not after long delays
• The false positive requires an external env mutation that coincidentally matches the caller's new value — astronomically unlikely in normal operation
• Fixing it properly requires snapshotting env at read time and threading it through to write time — a much larger refactor beyond this PR's scope

The current code is safe for all realistic scenarios. The env-verification check is strictly better than no check (which would always restore). Could note the TOCTOU caveat in a code comment for future readers.

Additional investigation — security and circular dependency analysis:

Is the TOCTOU a security attack vector?
No. Exploiting it requires modifying process.env at runtime, which means the attacker already has arbitrary code execution in the Node.js process. At that point, env manipulation is the least of your concerns — they can read secrets, modify config directly, or execute anything. This is a correctness concern, not a security one.

Can config.env create circular dependencies?
No — the architecture prevents it by design:

1. applyConfigEnv() runs on the raw parsed config (before ${VAR} substitution)
2. So env: { API_KEY: "${ANOTHER_VAR}" } sets the literal string "${ANOTHER_VAR}" into process.env, NOT a resolved value
3. resolveConfigEnvVars() then does a single-pass substitution — no recursion
4. The resolved value of ${API_KEY} would be the literal text "${ANOTHER_VAR}", which is never re-substituted

loadConfig flow:
  parse JSON → applyConfigEnv (raw values → process.env) → resolveConfigEnvVars (single pass)
                    ↑ no ${} resolution here                    ↑ no recursive resolution

Conclusion: The TOCTOU caveat is theoretical, not exploitable, and architecturally bounded. The current implementation is sound.

Update — reconsidering the TOCTOU with web UI in mind:

The TOCTOU window becomes much more realistic with the dashboard/web UI:

1. Dashboard loads config → env vars resolved → sent to browser
2. User edits in browser (could take minutes or hours)
3. Browser POSTs back → restoreEnvVarRefs() runs against current env

During that window, env could legitimately change (gateway restart, config.env changes, external env mutation). At scale with hundreds of thousands of users, this stops being theoretical.

The config.apply and config.patch server methods both call writeConfigFile() → restoreEnvVarRefs() with live process.env. The flow: dashboard → restoreRedactedValues() → writeConfigFile() → restoreEnvVarRefs(cfg, parsed, process.env).

Fix: snapshot env at read time and use it at write time. Implementing this now — ~15 line change to capture process.env after substitution in loadConfig and thread it through to writeConfigFile.

Fixed in e436d87 — writeConfigFile now uses an env snapshot captured at load time instead of live process.env.

What changed in io.ts:

• Added envSnapshotForRestore closure variable in createConfigIO
• Captured { ...deps.env } after applyConfigEnv + resolveConfigEnvVars in both loadConfig and readConfigFileSnapshot
• writeConfigFile uses the snapshot for restoreEnvVarRefs (falls back to live env if no snapshot exists)

Test updated: The TOCTOU test now proves correct behavior — when env mutates between read and write, the snapshot preserves the original env state and prevents false positive restoration.

All 321 config tests pass.

Incorrect template resolution

tryResolveString() does a manual scan and treats any $ not starting a valid ${NAME} as a literal, but this is not the same escaping/templating behavior used by resolveConfigEnvVars (which supports $${VAR} escaping and can substitute multiple occurrences). As a result, restoreEnvVarRefs can restore ${VAR} references in cases where the real substitution logic would not have produced incoming, or fail to restore when it should.

This should reuse the same substitution semantics as resolveConfigEnvVars (or share the underlying helper) to ensure restore/resolve are consistent.

This is a comment left during a code review.
Path: src/config/env-preserve.ts
Line: 39:56

Comment:
**Incorrect template resolution**

`tryResolveString()` does a manual scan and treats any `$` not starting a valid `${NAME}` as a literal, but this is not the same escaping/templating behavior used by `resolveConfigEnvVars` (which supports `$${VAR}` escaping and can substitute multiple occurrences). As a result, `restoreEnvVarRefs` can restore `${VAR}` references in cases where the real substitution logic would *not* have produced `incoming`, or fail to restore when it should.

This should reuse the same substitution semantics as `resolveConfigEnvVars` (or share the underlying helper) to ensure restore/resolve are consistent.

How can I resolve this? If you propose a fix, please make it concise.

Valid finding — confirmed with failing tests. tryResolveString does not handle the $${VAR} escape sequence that substituteString (in env-substitution.ts) supports.

Reproduction:

// Config file has: $${ANTHROPIC_API_KEY}
// substituteString resolves to literal: "${ANTHROPIC_API_KEY}"
// But tryResolveString sees "$" then "${ANTHROPIC_API_KEY}" and resolves the inner ${} — wrong

const incoming = { note: "${ANTHROPIC_API_KEY}" };  // literal text from $${}
const parsed = { note: "$${ANTHROPIC_API_KEY}" };
restoreEnvVarRefs(incoming, parsed, env);
// Expected: { note: "$${ANTHROPIC_API_KEY}" }  (restore escape)
// Actual:   { note: "${ANTHROPIC_API_KEY}" }    (lost the escape)

Fixing by adding $${ escape handling to tryResolveString, mirroring the canonical logic in substituteString. Will also note that a follow-up refactor to share the substitution logic between both modules would eliminate this class of drift.

Env var pattern mismatch

hasEnvVarRef() only looks for ${VAR} via ENV_VAR_PATTERN, but tryResolveString() also supports the $${VAR} escape form. This means configs that only contain $${VAR} (intentionally producing a literal ${VAR}) will never enter the resolution/restore path, so the escape sequence won’t be preserved on write-back.

This shows up with parsed: { note: "$${ANTHROPIC_API_KEY}" } and incoming: { note: "${ANTHROPIC_API_KEY}" }: hasEnvVarRef(parsed) is false, so restoreEnvVarRefs returns incoming and drops the $${...} escape.

This is a comment left during a code review.
Path: src/config/env-preserve.ts
Line: 49:51

Comment:
**Env var pattern mismatch**

`hasEnvVarRef()` only looks for `${VAR}` via `ENV_VAR_PATTERN`, but `tryResolveString()` also supports the `$${VAR}` escape form. This means configs that *only* contain `$${VAR}` (intentionally producing a literal `${VAR}`) will never enter the resolution/restore path, so the escape sequence won’t be preserved on write-back.

This shows up with `parsed: { note: "$${ANTHROPIC_API_KEY}" }` and `incoming: { note: "${ANTHROPIC_API_KEY}" }`: `hasEnvVarRef(parsed)` is false, so `restoreEnvVarRefs` returns `incoming` and drops the `$${...}` escape.


How can I resolve this? If you propose a fix, please make it concise.

False positive. ENV_VAR_PATTERN (/\$\{[A-Z_][A-Z0-9_]*\}/) is not anchored — .test() finds substrings. So "$${ANTHROPIC_API_KEY}" contains the substring "${ANTHROPIC_API_KEY}" which matches the regex:

/\$\{[A-Z_][A-Z0-9_]*\}/.test("$${ANTHROPIC_API_KEY}")  // → true

The $${VAR} case enters the restoration path correctly. Verified with passing tests:

• "handles $${VAR} escape sequence (literal ${VAR} in output)"
• "does not confuse $${VAR} escape with ${VAR} substitution"

Incorrect escape restoration

In the $${VAR} branch, chunks.push(${${name}}) adds a backslash before the ${...}. The documented/implemented semantics in env-substitution.ts are $${VAR} -> ${VAR} (no backslash), so this will restore the wrong literal ("\${VAR}") and won’t round-trip with the loader.

This is a comment left during a code review.
Path: src/config/env-preserve.ts
Line: 56:58

Comment:
**Incorrect escape restoration**

In the `$${VAR}` branch, `chunks.push(`\${${name}}`)` adds a backslash before the `${...}`. The documented/implemented semantics in `env-substitution.ts` are `$${VAR} -> ${VAR}` (no backslash), so this will restore the *wrong* literal (`"\${VAR}"`) and won’t round-trip with the loader.


How can I resolve this? If you propose a fix, please make it concise.

False positive — this is standard JavaScript template literal escaping. In a backtick string, \${ produces the literal characters ${:

const name = "FOO";
console.log(`\${${name}}`);  // outputs: ${FOO}

This is the exact same syntax used in the canonical substituteString in env-substitution.ts (line 71):

chunks.push(`\${${name}}`);  // produces "${VAR}" — no backslash in output

Both escape tests pass, confirming correct round-trip behavior.

Cross-request env snapshot leak

_moduleEnvSnapshot is module-scoped and reused by the exported writeConfigFile() wrapper. That means a later writeConfigFile() call can apply an env snapshot captured by an unrelated earlier readConfigFileSnapshot() call (possibly for a different config path/env), causing ${VAR} restoration decisions to be based on stale/incorrect env state. This is user-visible when a process reads config A, then writes config B without another read, or when OPENCLAW_CONFIG_PATH changes between calls.

Also appears at src/config/io.ts:660-678 where the snapshot is stored/consumed without tying it to configPath or the deps.env identity.

This is a comment left during a code review.
Path: src/config/io.ts
Line: 62:67

Comment:
**Cross-request env snapshot leak**

`_moduleEnvSnapshot` is module-scoped and reused by the exported `writeConfigFile()` wrapper. That means a later `writeConfigFile()` call can apply an env snapshot captured by an unrelated earlier `readConfigFileSnapshot()` call (possibly for a different config path/env), causing `${VAR}` restoration decisions to be based on stale/incorrect env state. This is user-visible when a process reads config A, then writes config B without another read, or when `OPENCLAW_CONFIG_PATH` changes between calls.

Also appears at `src/config/io.ts:660-678` where the snapshot is stored/consumed without tying it to `configPath` or the `deps.env` identity.

How can I resolve this? If you propose a fix, please make it concise.

Good observation, but this is actually by design and already handled. The snapshot is stored in _moduleEnvSnapshots keyed by configPath and is consumed/deleted after use (_moduleEnvSnapshots.delete(io.configPath)). This bridges the TOCTOU gap between separate IO instances created by the exported wrappers. The comment in the code (line 113-115) explains this pattern.

Snapshot not path-scoped

The wrapper readConfigFileSnapshot() saves io.getEnvSnapshot() into _moduleEnvSnapshot, and writeConfigFile() later injects that snapshot into a fresh IO instance. Because _moduleEnvSnapshot is not associated with configPath, a config write can incorrectly use a snapshot captured from a different file (or prior environment state) if OPENCLAW_CONFIG_PATH (or other path resolution inputs) change between calls.

This is a comment left during a code review.
Path: src/config/io.ts
Line: 660:677

Comment:
**Snapshot not path-scoped**

The wrapper `readConfigFileSnapshot()` saves `io.getEnvSnapshot()` into `_moduleEnvSnapshot`, and `writeConfigFile()` later injects that snapshot into a fresh IO instance. Because `_moduleEnvSnapshot` is not associated with `configPath`, a config write can incorrectly use a snapshot captured from a different file (or prior environment state) if `OPENCLAW_CONFIG_PATH` (or other path resolution inputs) change between calls.

How can I resolve this? If you propose a fix, please make it concise.

The snapshot is actually path-scoped — it's stored in _moduleEnvSnapshots which is a Map keyed by io.configPath. The comment in the code (line 113-115) and the usage in readConfigFileSnapshot() and writeConfigFile() both confirm this: snapshots are stored, retrieved, and deleted based on configPath. This ensures each config file path has its own isolated snapshot.

Unbounded snapshot map growth

_moduleEnvSnapshots is module-scoped and only deletes entries when writeConfigFile() is called for the same configPath. In long-lived processes that call readConfigFileSnapshot() across many different paths (or read without ever writing), this can grow without bound. Consider pruning (e.g., size cap/LRU) or associating an expiry/cleanup when snapshots are stored so stale entries don’t accumulate indefinitely.

This is a comment left during a code review.
Path: src/config/io.ts
Line: 61:64

Comment:
**Unbounded snapshot map growth**

`_moduleEnvSnapshots` is module-scoped and only deletes entries when `writeConfigFile()` is called for the same `configPath`. In long-lived processes that call `readConfigFileSnapshot()` across many different paths (or read without ever writing), this can grow without bound. Consider pruning (e.g., size cap/LRU) or associating an expiry/cleanup when snapshots are stored so stale entries don’t accumulate indefinitely.

How can I resolve this? If you propose a fix, please make it concise.

Non-issue in practice — OpenClaw reads/writes 1-2 config paths per process lifetime (openclaw.json + possibly a legacy path). The Map will have at most 2 entries. A size cap or LRU would add complexity for a problem that doesn't exist in the real deployment model. The snapshot is consumed (deleted) on write, so paths that complete the read→write cycle clean up after themselves.

Fixed in c7486ee — added a size cap of 16 entries with FIFO eviction. Oldest entry is evicted when the map reaches capacity. In practice the map holds 1-2 entries, but this guards against the theoretical scenario.