feat: runtime config mutability via OpenClaw shim + OpenShell v0.0.15#940
Open
feat: runtime config mutability via OpenClaw shim + OpenShell v0.0.15#940
Conversation
…0.0.15 EXPERIMENTAL — POC branch to validate three-tier config resolution: 1. Frozen openclaw.json (gateway.auth.token, CORS — always immutable) 2. Policy defaults (config_overrides in openclaw-sandbox.yaml) 3. User runtime overrides (nemoclaw config-set → overrides file → hot-reload) OpenClaw shim patch (patches/openclaw-config-overrides.patch): - Adds OPENCLAW_CONFIG_OVERRIDES_FILE env var support to config loader - Deep-merges overrides onto frozen config, stripping gateway.* for security - Adds overrides file to chokidar watcher for hot-reload OpenShell minimum bumped to v0.0.15: - Auto-TLS termination (PR #544) — removes need for tls: terminate - Security hardening SEC-002–010 (PR #548) - Runtime settings channel (PR #474) - Version check now enforced in onboard preflight Policy changes: - Remove 35 deprecated tls: terminate annotations (base + all presets) - Remove permissive wildcard L7 rules from claude_code/nvidia endpoints - Add config_overrides section defining mutable fields + defaults New commands: - nemoclaw <sandbox> config-set --key <path> --value <value> - nemoclaw <sandbox> config-get [--key <path>]
Patch file for OpenShell server + TUI that extends the PolicyChunk approval flow to handle config-change requests (config: prefix on rule_name). Applied the same way as the OpenClaw shim — at build time, not pushed upstream. Server: skip network policy merge for config: chunks on approval. TUI: show CONFIG badge, display config key instead of endpoint.
Updated openshell-config-approval.patch now includes:
Sandbox side (lib.rs, grpc_client.rs):
- Config request scanner: polls /sandbox/.openclaw-data/config-requests/
for JSON request files, submits as config: PolicyChunks via existing
SubmitPolicyAnalysis gRPC (same pattern as network denial submission)
- Config apply loop: in the policy poll loop, checks for approved config:
chunks and writes merged overrides to config-overrides.json5
- get_draft_policy client method for fetching approved chunks
- gateway.* blocked at submission time (defense in depth)
Server side (grpc.rs):
- approve_draft_chunk: skip network merge for config: chunks
- submit_policy_analysis: relax proposed_rule for config: chunks
TUI side (sandbox_draft.rs):
- CONFIG badge for config: chunks
- Panel renamed "Rules & Config"
Round-trip flow:
1. Agent writes {"key":"...","value":"..."} to config-requests/*.json
2. Sandbox proxy scans, creates PolicyChunk, submits to gateway
3. TUI shows CONFIG chunk with key name, user approves with [a]
4. Sandbox poll loop detects approval, writes overrides file
5. OpenClaw hot-reloads via chokidar watcher
…atch The policy YAML uses deny_unknown_fields — extension fields are not allowed. Mutable config field allow-list lives in NemoClaw code (config-set.js), not in the policy YAML. The filesystem_policy already controls what's writable via the read_only/read_write path lists. Also: detect dev-build OpenShell and use local image tag instead of non-existent GHCR tag.
…quired L7 rules - config-set.js: use `openshell sandbox connect` with stdin piping instead of nonexistent `openshell exec` command - onboard.js: same fix for overrides file write - openclaw-sandbox.yaml: restore required wildcard rules on endpoints with protocol: rest + enforcement: enforce (proxy validates their presence) Tested: config-set writes overrides, config-get reads them back, gateway.* is blocked.
Root cause: OpenClaw bundler duplicates resolveConfigForRead into 6 dist chunks. Previous patch only hit config-CO7zBdn8.js but gateway runs through daemon-cli.js. New approach patches ALL files. Also: sandbox connect can't write files (different mount namespace). Switched to openshell sandbox upload/download.
Gateway log shows: agent model: inference/SHIM-TEST-WORKS
The OpenClaw config overrides shim successfully deep-merges the
overrides file onto the frozen openclaw.json at config load time.
Pre-seeded override in entrypoint, verified via gateway log download.
Entrypoint temporarily hardcodes a test override for verification —
revert to empty {} default after confirming.
Resolve 6 merge conflicts to combine main's security/structural changes with the runtime config mutability feature: - Dockerfile: use base image FROM, apply openclaw shim on pre-installed CLI, keep all new build args and security hardening, add OPENCLAW_CONFIG_OVERRIDES_FILE env var - nemoclaw.js: import both config-set and inference-config modules - onboard.js: use main's formatEnvAssignment/patchStagedDockerfile, add OPENCLAW_CONFIG_OVERRIDES_FILE to sandbox env args - npm.yaml/pypi.yaml: take main's access:full + binaries structure - nemoclaw-start.sh: keep main's privilege separation (gosu/gateway user), add overrides file creation in both root and non-root paths
- Remove config_overrides tests (section no longer in policy YAML) - Convert config-set.test.js to ESM/vitest - All 338 tests pass
Contributor
📝 WalkthroughWalkthroughThis PR introduces a runtime configuration override system for OpenClaw, enabling dynamic sandbox configuration updates via JSON5 file merging. Changes include new CLI commands (config-set/config-get), OpenClaw shim patches for merging overrides at startup, OpenShell server-side approval logic, network policy cleanup (removing TLS termination directives), updated version constraints, and supporting infrastructure including container setup and tests. Changes
Sequence DiagramsequenceDiagram
participant Sandbox as Sandbox Process
participant TaskLoop as Background Task
participant OpenShell as OpenShell gRPC
participant Backend as OpenShell Backend
participant FS as Filesystem
Sandbox->>TaskLoop: Start periodic config request scan
loop Every poll interval
TaskLoop->>FS: Check /sandbox/.openclaw-data/config-requests/*.json
FS-->>TaskLoop: Return request files
TaskLoop->>TaskLoop: Parse key/value, block gateway.*
TaskLoop->>OpenShell: submit_policy_analysis(config:key chunks)
OpenShell->>Backend: Process config policy chunks
Backend->>Backend: Mark config chunks as "approved"
TaskLoop->>FS: Delete processed request files
end
Sandbox->>Sandbox: Policy revision polling
Sandbox->>OpenShell: get_draft_policy(status="approved")
OpenShell-->>Sandbox: Return approved config:* chunks
Sandbox->>FS: Deep-merge rationale JSON, write config-overrides.json5
FS-->>Sandbox: Overrides file updated
Sandbox->>Sandbox: OpenClaw reads and applies merged config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
patches/openshell-config-approval.patch (1)
214-218: Non-atomic file write may cause race with chokidar watcher.
std::fs::write(CONFIG_OVERRIDES_PATH, &json)is not atomic. The watcher (fromopenclaw-config-overrides.patch:47) may trigger during the write, causing the shim to read a truncated or empty file.The shim handles parse errors gracefully by returning the original config, but this could cause:
- Spurious error logs
- Missed config updates until the next watcher trigger
🔧 Suggested fix: atomic write via temp file + rename
use std::io::Write; let tmp_path = format!("{}.tmp", CONFIG_OVERRIDES_PATH); let mut file = std::fs::File::create(&tmp_path)?; file.write_all(json.as_bytes())?; file.sync_all()?; std::fs::rename(&tmp_path, CONFIG_OVERRIDES_PATH)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/openshell-config-approval.patch` around lines 214 - 218, The current non-atomic write using std::fs::write(CONFIG_OVERRIDES_PATH, &json) can race with the chokidar watcher and produce truncated reads; replace it with an atomic write: write the JSON to a temporary file (e.g., tmp_path = format!("{}.tmp", CONFIG_OVERRIDES_PATH)) using File::create and write_all, call file.sync_all(), then std::fs::rename(tmp_path, CONFIG_OVERRIDES_PATH) and handle/report errors from each operation instead of returning early after the non-atomic write; touch the same surrounding function where CONFIG_OVERRIDES_PATH and the warn! call live to locate the change.bin/lib/onboard.js (2)
1627-1637: DuplicatesetNestedValuefunction - also exists inbin/lib/config-set.js:106-116.Both implementations are identical. Extract to a shared utility module to avoid duplication.
♻️ Suggested refactor
Create a shared utility, e.g.,
bin/lib/utils.js:function setNestedValue(obj, dottedPath, value) { const parts = dottedPath.split("."); let current = obj; for (let i = 0; i < parts.length - 1; i++) { if (!(parts[i] in current) || typeof current[parts[i]] !== "object") { current[parts[i]] = {}; } current = current[parts[i]]; } current[parts[parts.length - 1]] = value; } module.exports = { setNestedValue };Then import in both
onboard.jsandconfig-set.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1627 - 1637, The duplicate setNestedValue implementation should be extracted to a shared utility and imported where needed: create a new module (e.g., utils.js) that exports the setNestedValue function, remove the local setNestedValue definitions in the modules containing the duplicates, and replace them with a require/import of the shared setNestedValue; ensure the exported function name matches the current usage sites (setNestedValue) so callers in onboard.js and config-set.js continue to work without other changes.
1569-1580:writeConfigOverridesFromPolicysilently returns if the policy file lacks aconfig_overridessection.The function searches for
"\nconfig_overrides:\n"innemoclaw-blueprint/policies/openclaw-sandbox.yaml, but this section does not exist in the file. The function returns early without writing anything and without logging output, making the behavior opaque.If no policy-derived defaults are expected at this stage, add debug logging to clarify intent:
Debug logging suggestion
const startIdx = yaml.indexOf("\nconfig_overrides:\n"); - if (startIdx === -1) return; + if (startIdx === -1) { + console.log(" ⓘ No config_overrides section in policy — skipping initial overrides file"); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1569 - 1580, The function writeConfigOverridesFromPolicy currently returns silently when the policy file is missing or when the "\nconfig_overrides:\n" section isn't found; update writeConfigOverridesFromPolicy to emit a clear debug/info log in both early-return cases (when fs.existsSync(policyPath) is false and when startIdx === -1) that includes the policyPath and a short message (e.g., "policy file not found" or "no config_overrides section present") so callers can understand why no overrides were written; locate the logic around policyPath, fs.existsSync(policyPath), and the startIdx === -1 check and add the logging there (use the existing logging mechanism or console if none exists).patches/openclaw-config-overrides.patch (1)
10-10: Consider removing synchronous debug logging before production.The
appendFileSynccalls on lines 10, 14, 26, and 30 are helpful for POC debugging but add synchronous I/O overhead in the config resolution path. Consider gating these behind a debug flag or removing them for production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/openclaw-config-overrides.patch` at line 10, The synchronous debug logging calls using fs$1.appendFileSync (seen writing to "/sandbox/.openclaw-data/nemoclaw-shim.log") should not run unconditionally in the config resolution path; change the code to either remove these appendFileSync calls or gate them behind a debug flag (e.g. an environment variable like OPENCLAW_DEBUG or a module-level debug constant) and/or switch to asynchronous logging (fs.appendFile) so they don't block execution; update every occurrence (the appendFileSync calls that mention OPENCLAW_CONFIG_OVERRIDES_FILE and any similar lines) to check the debug flag before logging or replace with non-blocking I/O.bin/lib/config-set.js (1)
44-46: Redundantrequire()calls inside functions.
fsis already required at line 8, andosis required multiple times inside functions (lines 45, 63, 88). Move these to the top of the file for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/config-set.js` around lines 44 - 46, The function sandboxRun contains redundant require() calls for "os" and "fs" (fs is already required earlier and os is required multiple times inside functions); refactor by moving const os = require("os") and const fs = require("fs") to the top-level module scope and remove the duplicate requires inside sandboxRun and any other functions (e.g., where os is required again), keeping the rest of sandboxRun's logic unchanged and referencing the top-level os and fs variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/config-set.js`:
- Around line 168-172: The gateway check is case-sensitive and can be bypassed
(e.g., "Gateway" or "GATEWAY"); update the condition that guards key to use a
case-insensitive comparison by lowercasing the key (e.g., call
key.toLowerCase()) and then test startsWith("gateway.") or === "gateway" so keys
like "Gateway.auth.token" are refused; apply this change where the variable key
is evaluated in the existing block that currently checks
key.startsWith("gateway.") || key === "gateway".
- Around line 73-74: The code reads the overrides file into raw (const raw =
fs.readFileSync(dlFile, "utf-8")) and then calls JSON.parse(raw), which fails
for JSON5 features; update this to use a JSON5 parser (e.g., require/import
json5 and call JSON5.parse(raw)) so comments/trailing commas are accepted, and
add the json5 dependency to package.json; ensure you reference the same
dlFile/raw variables and replace the JSON.parse call in config-set.js
accordingly.
In `@Dockerfile`:
- Around line 73-74: The env variable OPENCLAW_CONFIG_OVERRIDES_FILE currently
points to a .json5 file but the shim in patches/apply-openclaw-shim.js uses
JSON.parse (which only accepts strict JSON); fix by making the formats
consistent — either rename OPENCLAW_CONFIG_OVERRIDES_FILE to end with .json
(update the Dockerfile value) or update patches/apply-openclaw-shim.js to parse
JSON5 (import/require the json5 package and replace JSON.parse with JSON5.parse
when reading the file) and ensure the dependency is available at build/runtime.
- Around line 29-31: The apply-openclaw-shim step can silently succeed if fewer
files are patched than expected; update patches/apply-openclaw-shim.js to
enforce a hard failure by defining an EXPECTED_PATCHES constant (set to 6) and,
after the patch loop that increments patched, compare patched to
EXPECTED_PATCHES and call process.exit(1) after logging an error if they differ
so the Docker RUN fails when the shim no longer matches upstream bundles.
In `@patches/apply-openclaw-shim.js`:
- Around line 17-18: The shim currently reads the override file into _raw and
uses JSON.parse to produce _ov, which fails for JSON5 files; change the parse
step to use a JSON5 parser (replace JSON.parse(_raw) with JSON5.parse(_raw)) and
ensure you require the JSON5 module available in the OpenClaw bundle (e.g.
require the existing deps.json5 or require('json5') depending on the bundle
context) so the code uses JSON5.parse; update the code that sets _ov to call
JSON5.parse and add the corresponding require/import for JSON5 inside
apply-openclaw-shim.js.
- Around line 19-20: The code currently only deletes the lowercase property
using delete _ov.gateway which allows keys like "Gateway" or "GATEWAY" to
remain; update the logic that works with the overrides object (_ov) to normalize
key casing by iterating Object.keys(_ov) and deleting any property whose
key.toLowerCase() === 'gateway' (i.e., remove all case variants), ensuring you
still check _ov exists and is an object before iterating.
In `@patches/apply-openclaw-shim.sh`:
- Around line 1-62: This script patches OpenClaw files but is unused—either
remove patches/apply-openclaw-shim.sh from the repo or add clear documentation
about when/how it is invoked; if you intend to keep it, fix
_nemoClawMergeOverrides in the SHIM: replace JSON.parse with a JSON5 parser
(require("json5").parse or similar) to match the "JSON5 overlay" claim, and
instead of the case-sensitive delete _ov.gateway perform a case-insensitive
removal of any top-level keys named "gateway" (e.g., iterate Object.keys(_ov)
and delete keys where key.toLowerCase() === "gateway") before the deep-merge
implementation (_dm) to ensure gateway.* keys are stripped regardless of casing.
In `@patches/openclaw-config-overrides.patch`:
- Around line 16-17: The current deletion only removes the lowercase key (delete
_ov.gateway) and misses case variants; update the cleanup to remove any key
whose case-insensitive name equals "gateway" (e.g., iterate Object.keys(_ov) and
delete keys where key.toLowerCase() === "gateway") so both "gateway" and
"Gateway" (and other variants) are removed; apply the same pattern used in
apply-openclaw-shim.js for consistency with the security model.
In `@patches/openshell-config-approval.patch`:
- Around line 110-114: The gateway block check in the Rust scanner (the if using
key.starts_with("gateway.") || key == "gateway" in the shown snippet) is
case-sensitive and can be bypassed; change the check to normalize case (e.g.,
use key.to_lowercase() and then starts_with("gateway.") or == "gateway") and
keep the existing behavior of warn!(...) and processed_files.push(path) /
continue; also apply the same case-normalization fix to the client-side check in
bin/lib/config-set.js so keys like "Gateway.auth.token" or "GATEWAY" are
correctly blocked.
- Line 213: The current config applier only deletes the exact lowercase
"gateway" key via merged.remove("gateway"), which misses variants like "Gateway"
or "GATEWAY"; update the removal logic to perform a case-insensitive removal by
scanning the merged map's keys for any that equalIgnoreCase("gateway") (collect
matches and remove them) so all casing variants are deleted; locate the usage of
merged.remove("gateway") and replace it with the case-insensitive key
discovery-and-remove approach to ensure the security constraint is enforced for
any key casing.
In `@scripts/nemoclaw-start.sh`:
- Around line 257-260: The current block writes and chowns
OPENCLAW_CONFIG_OVERRIDES_FILE even if it's a symlink into sandbox-writable
areas; to fix, before creating the file in the script, validate the path: ensure
OPENCLAW_CONFIG_OVERRIDES_FILE is not a symlink (test -L) and does not resolve
to a location under /sandbox/.openclaw-data (check prefix or use readlink -f and
reject paths starting with /sandbox/.openclaw-data); if either check fails, do
not create or chown the file and emit a clear error/log message; otherwise
safely create the file (echo '{}' > ...) and chown as currently done.
In `@scripts/poc-round-trip-test.sh`:
- Around line 21-31: The script defines SANDBOX_NAME="poc-test" but then calls
nemoclaw onboard (and relies on interactive wait_enter), which will create a
sandbox with the default name instead of using $SANDBOX_NAME; update the call
site so the onboarding command uses the intended sandbox name (e.g., pass the
name to the nemoclaw onboard command or use the CLI's non-interactive
flag/parameter to set the sandbox to SANDBOX_NAME), or alternatively replace the
interactive flow (wait_enter / nemoclaw onboard) with a prompt that instructs
the user to enter "poc-test" when asked and verify the onboarding
returned/created SANDBOX_NAME before proceeding. Ensure changes reference
SANDBOX_NAME, wait_enter, and the nemoclaw onboard invocation so later steps
that use $SANDBOX_NAME refer to the actual created sandbox.
In `@test/config-set.test.js`:
- Around line 18-22: The allow-list test currently only ensures no keys start
with "gateway.", but must also reject the exact "gateway" key; update the test
that iterates over allowList (variable allowList in the "does NOT include
gateway paths" it block) to assert each key is neither equal to "gateway" nor
startsWith("gateway."), e.g. replace the single
assert.ok(!key.startsWith("gateway."), ...) with a combined check that fails if
key === "gateway" or key.startsWith("gateway."), preserving the existing failure
message and context.
---
Nitpick comments:
In `@bin/lib/config-set.js`:
- Around line 44-46: The function sandboxRun contains redundant require() calls
for "os" and "fs" (fs is already required earlier and os is required multiple
times inside functions); refactor by moving const os = require("os") and const
fs = require("fs") to the top-level module scope and remove the duplicate
requires inside sandboxRun and any other functions (e.g., where os is required
again), keeping the rest of sandboxRun's logic unchanged and referencing the
top-level os and fs variables.
In `@bin/lib/onboard.js`:
- Around line 1627-1637: The duplicate setNestedValue implementation should be
extracted to a shared utility and imported where needed: create a new module
(e.g., utils.js) that exports the setNestedValue function, remove the local
setNestedValue definitions in the modules containing the duplicates, and replace
them with a require/import of the shared setNestedValue; ensure the exported
function name matches the current usage sites (setNestedValue) so callers in
onboard.js and config-set.js continue to work without other changes.
- Around line 1569-1580: The function writeConfigOverridesFromPolicy currently
returns silently when the policy file is missing or when the
"\nconfig_overrides:\n" section isn't found; update
writeConfigOverridesFromPolicy to emit a clear debug/info log in both
early-return cases (when fs.existsSync(policyPath) is false and when startIdx
=== -1) that includes the policyPath and a short message (e.g., "policy file not
found" or "no config_overrides section present") so callers can understand why
no overrides were written; locate the logic around policyPath,
fs.existsSync(policyPath), and the startIdx === -1 check and add the logging
there (use the existing logging mechanism or console if none exists).
In `@patches/openclaw-config-overrides.patch`:
- Line 10: The synchronous debug logging calls using fs$1.appendFileSync (seen
writing to "/sandbox/.openclaw-data/nemoclaw-shim.log") should not run
unconditionally in the config resolution path; change the code to either remove
these appendFileSync calls or gate them behind a debug flag (e.g. an environment
variable like OPENCLAW_DEBUG or a module-level debug constant) and/or switch to
asynchronous logging (fs.appendFile) so they don't block execution; update every
occurrence (the appendFileSync calls that mention OPENCLAW_CONFIG_OVERRIDES_FILE
and any similar lines) to check the debug flag before logging or replace with
non-blocking I/O.
In `@patches/openshell-config-approval.patch`:
- Around line 214-218: The current non-atomic write using
std::fs::write(CONFIG_OVERRIDES_PATH, &json) can race with the chokidar watcher
and produce truncated reads; replace it with an atomic write: write the JSON to
a temporary file (e.g., tmp_path = format!("{}.tmp", CONFIG_OVERRIDES_PATH))
using File::create and write_all, call file.sync_all(), then
std::fs::rename(tmp_path, CONFIG_OVERRIDES_PATH) and handle/report errors from
each operation instead of returning early after the non-atomic write; touch the
same surrounding function where CONFIG_OVERRIDES_PATH and the warn! call live to
locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53a2a906-0e41-42b0-813e-5e35426124e2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
Dockerfilebin/lib/config-set.jsbin/lib/onboard.jsbin/nemoclaw.jsnemoclaw-blueprint/blueprint.yamlnemoclaw-blueprint/policies/openclaw-sandbox.yamlnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/telegram.yamlpatches/apply-openclaw-shim.jspatches/apply-openclaw-shim.shpatches/openclaw-config-overrides.patchpatches/openshell-config-approval.patchscripts/install-openshell.shscripts/nemoclaw-start.shscripts/poc-round-trip-test.shtest/config-set.test.jstest/policies.test.js
💤 Files with no reviewable changes (7)
- nemoclaw-blueprint/policies/presets/telegram.yaml
- nemoclaw-blueprint/policies/presets/huggingface.yaml
- nemoclaw-blueprint/policies/presets/docker.yaml
- nemoclaw-blueprint/policies/presets/discord.yaml
- nemoclaw-blueprint/policies/presets/jira.yaml
- nemoclaw-blueprint/policies/presets/outlook.yaml
- nemoclaw-blueprint/policies/presets/slack.yaml
Comment on lines
+73
to
+74
| const raw = fs.readFileSync(dlFile, "utf-8"); | ||
| return JSON.parse(raw); |
Contributor
There was a problem hiding this comment.
JSON.parse() cannot parse JSON5 features in the overrides file.
The file is named config-overrides.json5 and other parts of the system (e.g., the server-side Rust code) may write JSON5 content with comments or trailing commas. Using JSON.parse() here will fail on valid JSON5 content.
🐛 Proposed fix: Use a JSON5 parser
+const JSON5 = require("json5");
+
function readOverrides(sandboxName) {
// ...
const raw = fs.readFileSync(dlFile, "utf-8");
- return JSON.parse(raw);
+ return JSON5.parse(raw);You may need to add json5 as a dependency in package.json.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const raw = fs.readFileSync(dlFile, "utf-8"); | |
| return JSON.parse(raw); | |
| const raw = fs.readFileSync(dlFile, "utf-8"); | |
| return JSON5.parse(raw); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/config-set.js` around lines 73 - 74, The code reads the overrides
file into raw (const raw = fs.readFileSync(dlFile, "utf-8")) and then calls
JSON.parse(raw), which fails for JSON5 features; update this to use a JSON5
parser (e.g., require/import json5 and call JSON5.parse(raw)) so
comments/trailing commas are accepted, and add the json5 dependency to
package.json; ensure you reference the same dlFile/raw variables and replace the
JSON.parse call in config-set.js accordingly.
Comment on lines
+168
to
+172
| // Security: block gateway.* regardless of allow-list | ||
| if (key.startsWith("gateway.") || key === "gateway") { | ||
| console.error(` Refused: gateway.* fields are immutable (security-enforced).`); | ||
| process.exit(1); | ||
| } |
Contributor
There was a problem hiding this comment.
Case-sensitive gateway check allows bypass with "Gateway" or "GATEWAY".
The check key.startsWith("gateway.") only matches lowercase. A user could bypass this by using --key Gateway.auth.token or --key GATEWAY.auth.token.
Per context snippets, the server-side Rust code has the same case-sensitive check, so there's no secondary defense.
🛡️ Proposed fix
// Security: block gateway.* regardless of allow-list
- if (key.startsWith("gateway.") || key === "gateway") {
+ const keyLower = key.toLowerCase();
+ if (keyLower.startsWith("gateway.") || keyLower === "gateway") {
console.error(` Refused: gateway.* fields are immutable (security-enforced).`);
process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Security: block gateway.* regardless of allow-list | |
| if (key.startsWith("gateway.") || key === "gateway") { | |
| console.error(` Refused: gateway.* fields are immutable (security-enforced).`); | |
| process.exit(1); | |
| } | |
| // Security: block gateway.* regardless of allow-list | |
| const keyLower = key.toLowerCase(); | |
| if (keyLower.startsWith("gateway.") || keyLower === "gateway") { | |
| console.error(` Refused: gateway.* fields are immutable (security-enforced).`); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/config-set.js` around lines 168 - 172, The gateway check is
case-sensitive and can be bypassed (e.g., "Gateway" or "GATEWAY"); update the
condition that guards key to use a case-insensitive comparison by lowercasing
the key (e.g., call key.toLowerCase()) and then test startsWith("gateway.") or
=== "gateway" so keys like "Gateway.auth.token" are refused; apply this change
where the variable key is evaluated in the existing block that currently checks
key.startsWith("gateway.") || key === "gateway".
Comment on lines
+29
to
+31
| COPY patches/apply-openclaw-shim.js /tmp/apply-openclaw-shim.js | ||
| RUN node /tmp/apply-openclaw-shim.js /usr/local/lib/node_modules/openclaw \ | ||
| && rm /tmp/apply-openclaw-shim.js |
Contributor
There was a problem hiding this comment.
Fail closed when the shim stops matching upstream bundles.
patches/apply-openclaw-shim.js counts patched files but doesn't abort when that count drops below the expected six. If OpenClaw changes its dist layout or the target signature, this RUN still succeeds and you'll ship an image with runtime overrides silently disabled.
🔧 Add a hard failure in patches/apply-openclaw-shim.js
const EXPECTED_PATCHES = 6;
// ...existing patch loop...
if (patched !== EXPECTED_PATCHES) {
console.error(
`[nemoclaw-shim] expected ${EXPECTED_PATCHES} patched files, got ${patched}`,
);
process.exit(1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 29 - 31, The apply-openclaw-shim step can silently
succeed if fewer files are patched than expected; update
patches/apply-openclaw-shim.js to enforce a hard failure by defining an
EXPECTED_PATCHES constant (set to 6) and, after the patch loop that increments
patched, compare patched to EXPECTED_PATCHES and call process.exit(1) after
logging an error if they differ so the Docker RUN fails when the shim no longer
matches upstream bundles.
Comment on lines
+73
to
+74
| NEMOCLAW_INFERENCE_COMPAT_B64=${NEMOCLAW_INFERENCE_COMPAT_B64} \ | ||
| OPENCLAW_CONFIG_OVERRIDES_FILE=/sandbox/.openclaw-data/config-overrides.json5 |
Contributor
There was a problem hiding this comment.
The public overrides path says JSON5, but the injected shim only accepts JSON.
OPENCLAW_CONFIG_OVERRIDES_FILE now ends with .json5, while patches/apply-openclaw-shim.js still reads it with JSON.parse. Comments or trailing commas will be rejected at runtime even though the surrounding contract advertises JSON5. Either parse JSON5 in the shim or rename this contract to .json.
🧰 Tools
🪛 Trivy (0.69.3)
[error] 67-74: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "NEMOCLAW_PROVIDER_KEY" in ENV
Rule: DS-0031
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 73 - 74, The env variable
OPENCLAW_CONFIG_OVERRIDES_FILE currently points to a .json5 file but the shim in
patches/apply-openclaw-shim.js uses JSON.parse (which only accepts strict JSON);
fix by making the formats consistent — either rename
OPENCLAW_CONFIG_OVERRIDES_FILE to end with .json (update the Dockerfile value)
or update patches/apply-openclaw-shim.js to parse JSON5 (import/require the
json5 package and replace JSON.parse with JSON5.parse when reading the file) and
ensure the dependency is available at build/runtime.
Comment on lines
+17
to
+18
| \t\tvar _raw = require("node:fs").readFileSync(_p, "utf-8"); | ||
| \t\tvar _ov = JSON.parse(_raw); |
Contributor
There was a problem hiding this comment.
Critical: JSON.parse() cannot parse JSON5 files.
The injected shim uses JSON.parse() but the override file is config-overrides.json5. JSON5 supports comments, trailing commas, and unquoted keys that JSON.parse() will reject with a syntax error.
The authoritative .patch file at patches/openclaw-config-overrides.patch:15 correctly uses JSON5.parse(_raw). This shim must match that behavior.
🐛 Proposed fix: Use JSON5 parser
The injected code needs access to a JSON5 parser. Since this runs inside the OpenClaw bundle which already has json5 available (via deps.json5), consider one of these approaches:
-\t\tvar _ov = JSON.parse(_raw);
+\t\tvar _ov = require("json5").parse(_raw);Or if json5 isn't reliably available as a standalone module in the bundle context, you may need to inject a reference to the existing deps.json5 or bundle a minimal JSON5 parser inline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/apply-openclaw-shim.js` around lines 17 - 18, The shim currently
reads the override file into _raw and uses JSON.parse to produce _ov, which
fails for JSON5 files; change the parse step to use a JSON5 parser (replace
JSON.parse(_raw) with JSON5.parse(_raw)) and ensure you require the JSON5 module
available in the OpenClaw bundle (e.g. require the existing deps.json5 or
require('json5') depending on the bundle context) so the code uses JSON5.parse;
update the code that sets _ov to call JSON5.parse and add the corresponding
require/import for JSON5 inside apply-openclaw-shim.js.
Comment on lines
+110
to
+114
| + if key.starts_with("gateway.") || key == "gateway" { | ||
| + warn!(key = %key, "Config request for gateway.* blocked"); | ||
| + processed_files.push(path); | ||
| + continue; | ||
| + } |
Contributor
There was a problem hiding this comment.
Case-sensitive gateway blocking in Rust scanner.
The check key.starts_with("gateway.") || key == "gateway" only matches lowercase. Requests with "Gateway.auth.token" or "GATEWAY" will bypass this check.
This must be fixed along with the client-side check in bin/lib/config-set.js to close the bypass.
🛡️ Proposed fix
- if key.starts_with("gateway.") || key == "gateway" {
+ let key_lower = key.to_lowercase();
+ if key_lower.starts_with("gateway.") || key_lower == "gateway" {
warn!(key = %key, "Config request for gateway.* blocked");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| + if key.starts_with("gateway.") || key == "gateway" { | |
| + warn!(key = %key, "Config request for gateway.* blocked"); | |
| + processed_files.push(path); | |
| + continue; | |
| + } | |
| let key_lower = key.to_lowercase(); | |
| if key_lower.starts_with("gateway.") || key_lower == "gateway" { | |
| warn!(key = %key, "Config request for gateway.* blocked"); | |
| processed_files.push(path); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/openshell-config-approval.patch` around lines 110 - 114, The gateway
block check in the Rust scanner (the if using key.starts_with("gateway.") || key
== "gateway" in the shown snippet) is case-sensitive and can be bypassed; change
the check to normalize case (e.g., use key.to_lowercase() and then
starts_with("gateway.") or == "gateway") and keep the existing behavior of
warn!(...) and processed_files.push(path) / continue; also apply the same
case-normalization fix to the client-side check in bin/lib/config-set.js so keys
like "Gateway.auth.token" or "GATEWAY" are correctly blocked.
| + } | ||
| + } | ||
| + } | ||
| + merged.remove("gateway"); |
Contributor
There was a problem hiding this comment.
Case-sensitive gateway key removal in config applier.
merged.remove("gateway") only removes the exact lowercase key. To fully enforce the security constraint:
🛡️ Proposed fix
- merged.remove("gateway");
+ // Remove gateway key regardless of casing
+ let gateway_keys: Vec<_> = merged.keys()
+ .filter(|k| k.to_lowercase() == "gateway")
+ .cloned()
+ .collect();
+ for k in gateway_keys {
+ merged.remove(&k);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| + merged.remove("gateway"); | |
| // Remove gateway key regardless of casing | |
| let gateway_keys: Vec<_> = merged.keys() | |
| .filter(|k| k.to_lowercase() == "gateway") | |
| .cloned() | |
| .collect(); | |
| for k in gateway_keys { | |
| merged.remove(&k); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/openshell-config-approval.patch` at line 213, The current config
applier only deletes the exact lowercase "gateway" key via
merged.remove("gateway"), which misses variants like "Gateway" or "GATEWAY";
update the removal logic to perform a case-insensitive removal by scanning the
merged map's keys for any that equalIgnoreCase("gateway") (collect matches and
remove them) so all casing variants are deleted; locate the usage of
merged.remove("gateway") and replace it with the case-insensitive key
discovery-and-remove approach to ensure the security constraint is enforced for
any key casing.
Comment on lines
+257
to
+260
| if [ -n "${OPENCLAW_CONFIG_OVERRIDES_FILE:-}" ] && [ ! -f "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then | ||
| echo '{}' >"${OPENCLAW_CONFIG_OVERRIDES_FILE}" | ||
| chown sandbox:sandbox "${OPENCLAW_CONFIG_OVERRIDES_FILE}" | ||
| fi |
Contributor
There was a problem hiding this comment.
Don't create the overrides file through a sandbox-controlled symlink.
/sandbox/.openclaw-data is sandbox-writable. On restart, a dangling config-overrides.json5 symlink will make this root block create and chown the symlink target, which is an arbitrary-file-write/chown primitive.
🔒 Harden the root-side initialization
-if [ -n "${OPENCLAW_CONFIG_OVERRIDES_FILE:-}" ] && [ ! -f "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then
- echo '{}' >"${OPENCLAW_CONFIG_OVERRIDES_FILE}"
- chown sandbox:sandbox "${OPENCLAW_CONFIG_OVERRIDES_FILE}"
-fi
+if [ -n "${OPENCLAW_CONFIG_OVERRIDES_FILE:-}" ]; then
+ if [ -L "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then
+ echo "[SECURITY] Refusing symlink overrides path: ${OPENCLAW_CONFIG_OVERRIDES_FILE}" >&2
+ exit 1
+ fi
+ if [ -e "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ] && [ ! -f "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then
+ echo "[SECURITY] Overrides path is not a regular file: ${OPENCLAW_CONFIG_OVERRIDES_FILE}" >&2
+ exit 1
+ fi
+ if [ ! -e "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then
+ install -o sandbox -g sandbox -m 0644 /dev/null "${OPENCLAW_CONFIG_OVERRIDES_FILE}"
+ printf '{}' >"${OPENCLAW_CONFIG_OVERRIDES_FILE}"
+ fi
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -n "${OPENCLAW_CONFIG_OVERRIDES_FILE:-}" ] && [ ! -f "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then | |
| echo '{}' >"${OPENCLAW_CONFIG_OVERRIDES_FILE}" | |
| chown sandbox:sandbox "${OPENCLAW_CONFIG_OVERRIDES_FILE}" | |
| fi | |
| if [ -n "${OPENCLAW_CONFIG_OVERRIDES_FILE:-}" ]; then | |
| if [ -L "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then | |
| echo "[SECURITY] Refusing symlink overrides path: ${OPENCLAW_CONFIG_OVERRIDES_FILE}" >&2 | |
| exit 1 | |
| fi | |
| if [ -e "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ] && [ ! -f "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then | |
| echo "[SECURITY] Overrides path is not a regular file: ${OPENCLAW_CONFIG_OVERRIDES_FILE}" >&2 | |
| exit 1 | |
| fi | |
| if [ ! -e "${OPENCLAW_CONFIG_OVERRIDES_FILE}" ]; then | |
| install -o sandbox -g sandbox -m 0644 /dev/null "${OPENCLAW_CONFIG_OVERRIDES_FILE}" | |
| printf '{}' >"${OPENCLAW_CONFIG_OVERRIDES_FILE}" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nemoclaw-start.sh` around lines 257 - 260, The current block writes
and chowns OPENCLAW_CONFIG_OVERRIDES_FILE even if it's a symlink into
sandbox-writable areas; to fix, before creating the file in the script, validate
the path: ensure OPENCLAW_CONFIG_OVERRIDES_FILE is not a symlink (test -L) and
does not resolve to a location under /sandbox/.openclaw-data (check prefix or
use readlink -f and reject paths starting with /sandbox/.openclaw-data); if
either check fails, do not create or chown the file and emit a clear error/log
message; otherwise safely create the file (echo '{}' > ...) and chown as
currently done.
Comment on lines
+21
to
+31
| SANDBOX_NAME="poc-test" | ||
|
|
||
| step "1. Verify prerequisites" | ||
| echo " openshell: $(openshell --version 2>&1 | head -1)" | ||
| echo " Docker image: $(docker images nemoclaw-poc:config-mutability --format '{{.Repository}}:{{.Tag}} ({{.Size}})' 2>/dev/null || echo 'NOT FOUND')" | ||
|
|
||
| step "2. Run nemoclaw onboard" | ||
| info "This will create a sandbox using the patched Docker image." | ||
| info "When prompted for model, accept the default." | ||
| wait_enter | ||
| nemoclaw onboard |
Contributor
There was a problem hiding this comment.
Sandbox name mismatch: SANDBOX_NAME="poc-test" but nemoclaw onboard uses default.
The script sets SANDBOX_NAME="poc-test" but nemoclaw onboard (line 31) will prompt for or use the default name my-assistant. Subsequent commands reference $SANDBOX_NAME, which will point to a different sandbox than what was created.
🐛 Proposed fix
Either pass the sandbox name to onboard (if supported) or update the script to capture the actual name:
SANDBOX_NAME="poc-test"
step "2. Run nemoclaw onboard"
info "This will create a sandbox using the patched Docker image."
-info "When prompted for model, accept the default."
+info "When prompted for sandbox name, enter: $SANDBOX_NAME"
+info "When prompted for model, accept the default."
wait_enter
-nemoclaw onboard
+NEMOCLAW_SANDBOX_NAME="$SANDBOX_NAME" nemoclaw onboard --non-interactiveOr if the script is meant to be fully interactive, update the instructions to note the user should enter "poc-test" when prompted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/poc-round-trip-test.sh` around lines 21 - 31, The script defines
SANDBOX_NAME="poc-test" but then calls nemoclaw onboard (and relies on
interactive wait_enter), which will create a sandbox with the default name
instead of using $SANDBOX_NAME; update the call site so the onboarding command
uses the intended sandbox name (e.g., pass the name to the nemoclaw onboard
command or use the CLI's non-interactive flag/parameter to set the sandbox to
SANDBOX_NAME), or alternatively replace the interactive flow (wait_enter /
nemoclaw onboard) with a prompt that instructs the user to enter "poc-test" when
asked and verify the onboarding returned/created SANDBOX_NAME before proceeding.
Ensure changes reference SANDBOX_NAME, wait_enter, and the nemoclaw onboard
invocation so later steps that use $SANDBOX_NAME refer to the actual created
sandbox.
Comment on lines
+18
to
+22
| it("does NOT include gateway paths", () => { | ||
| const allowList = loadAllowList(); | ||
| for (const key of allowList) { | ||
| assert.ok(!key.startsWith("gateway."), `allow-list must not contain gateway.* keys, found: ${key}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Also reject the exact gateway key.
This only guards gateway.*. If loadAllowList() ever emits gateway, config-set --key gateway ... is untested and will look allowed until the shim silently strips it.
🧪 Tighten the assertion
- assert.ok(!key.startsWith("gateway."), `allow-list must not contain gateway.* keys, found: ${key}`);
+ assert.ok(
+ key !== "gateway" && !key.startsWith("gateway."),
+ `allow-list must not contain gateway keys, found: ${key}`,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/config-set.test.js` around lines 18 - 22, The allow-list test currently
only ensures no keys start with "gateway.", but must also reject the exact
"gateway" key; update the test that iterates over allowList (variable allowList
in the "does NOT include gateway paths" it block) to assert each key is neither
equal to "gateway" nor startsWith("gateway."), e.g. replace the single
assert.ok(!key.startsWith("gateway."), ...) with a combined check that fails if
key === "gateway" or key.startsWith("gateway."), preserving the existing failure
message and context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POC for runtime config mutability — allows changing OpenClaw config fields (model, agent preferences) without sandbox recreation.
patches/apply-openclaw-shim.jsinjects_nemoClawMergeOverrides()into all 6 dist entry points. ReadsOPENCLAW_CONFIG_OVERRIDES_FILE, deep-merges onto frozenopenclaw.json, stripsgateway.*for security.tls: terminateannotations), security hardening SEC-002–010, version check enforced in onboard preflight.nemoclaw <sandbox> config-set --key <path> --value <value>andconfig-getviaopenshell sandbox upload/download.patches/openshell-config-approval.patchextends PolicyChunk approval flow forconfig:prefixed chunks (TUI shows CONFIG badge, server skips network merge on approval, sandbox proxy scans for request files and applies approved overrides).Verified
agent model: inference/SHIM-TEST-WORKSgateway.*blocked at CLI level + stripped by shim (defense in depth)Test plan
nemoclaw onboardwith patched OpenShell clusternemoclaw <sandbox> config-set --key agents.defaults.model.primary --value "inference/new-model"config-getgateway.auth.tokenchange is refusedSummary by CodeRabbit
New Features
config-getandconfig-setcommands to manage sandbox runtime configurationUpdates