diff --git a/.agents/skills/nemoclaw-user-reference/references/troubleshooting.md b/.agents/skills/nemoclaw-user-reference/references/troubleshooting.md index 72e38178856..53539537f7f 100644 --- a/.agents/skills/nemoclaw-user-reference/references/troubleshooting.md +++ b/.agents/skills/nemoclaw-user-reference/references/troubleshooting.md @@ -509,6 +509,19 @@ $ nemoclaw channels remove `channels add` stores credentials under `~/.nemoclaw/credentials.json` and `channels remove` clears them; both offer to rebuild the sandbox so the image reflects the new channel set. In non-interactive mode (`NEMOCLAW_NON_INTERACTIVE=1`), the commands stage the change and leave the rebuild to a follow-up `nemoclaw rebuild`. +### `nemoclaw config set` refuses a key that does not currently exist + +This is intentional. +The host-side `config set` does not maintain a copy of OpenClaw's config schema, so it cannot tell a typo'd key path apart from a schema-valid path that has not been written yet. +To make typos visible without blocking documented first-time writes (such as `provider.compatible-endpoint.timeoutSeconds`), it asks before creating a brand-new key. + +In an interactive terminal, accept the prompt to proceed. + +In non-interactive mode (CI or `NEMOCLAW_NON_INTERACTIVE=1`), pass `--config-accept-new-path` or set `NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1`. +Modifying a key that already exists in the config never triggers this gate. + +Numeric path segments are also refused because `config set` only writes plain objects and would silently overwrite array elements; replace the array as a whole with `--value '[...]'` instead. + ### `openclaw config set` or `unset` is blocked inside the sandbox This is expected. diff --git a/docs/reference/troubleshooting.md b/docs/reference/troubleshooting.md index 6cfcb2de764..2d530f35a75 100644 --- a/docs/reference/troubleshooting.md +++ b/docs/reference/troubleshooting.md @@ -539,6 +539,19 @@ $ nemoclaw channels remove `channels add` stores credentials under `~/.nemoclaw/credentials.json` and `channels remove` clears them; both offer to rebuild the sandbox so the image reflects the new channel set. In non-interactive mode (`NEMOCLAW_NON_INTERACTIVE=1`), the commands stage the change and leave the rebuild to a follow-up `nemoclaw rebuild`. +### `nemoclaw config set` refuses a key that does not currently exist + +This is intentional. +The host-side `config set` does not maintain a copy of OpenClaw's config schema, so it cannot tell a typo'd key path apart from a schema-valid path that has not been written yet. +To make typos visible without blocking documented first-time writes (such as `provider.compatible-endpoint.timeoutSeconds`), it asks before creating a brand-new key. + +In an interactive terminal, accept the prompt to proceed. + +In non-interactive mode (CI or `NEMOCLAW_NON_INTERACTIVE=1`), pass `--config-accept-new-path` or set `NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1`. +Modifying a key that already exists in the config never triggers this gate. + +Numeric path segments are also refused because `config set` only writes plain objects and would silently overwrite array elements; replace the array as a whole with `--value '[...]'` instead. + ### `openclaw config set` or `unset` is blocked inside the sandbox This is expected. diff --git a/src/lib/sandbox-config.ts b/src/lib/sandbox-config.ts index 0944bdff9ab..e80d95888ad 100644 --- a/src/lib/sandbox-config.ts +++ b/src/lib/sandbox-config.ts @@ -12,6 +12,7 @@ // config set: Host-initiated config mutation with validation. // config rotate-token: Credential rotation via stdin or env var. +const readline = require("readline"); const fs = require("fs"); const os = require("os"); const path = require("path"); @@ -137,21 +138,125 @@ function setDotpath(obj: ConfigObject, dotpath: string, value: ConfigValue): voi } /** - * Return true when every segment in a dotpath is an own property on the - * current config object, which keeps config set constrained to recognized keys. + * Key segments that must never appear in a dotpath — blocking these prevents + * prototype-pollution and accidental traversal into inherited members. */ -function isRecognizedConfigPath(obj: ConfigValue, dotpath: string): boolean { - if (!dotpath || typeof dotpath !== "string") return false; +const UNSAFE_KEY_SEGMENTS: ReadonlySet = new Set([ + "__proto__", + "constructor", + "prototype", + "toString", + "hasOwnProperty", +]); + +type DotpathValidation = { ok: true } | { ok: false; reason: string }; + +/** + * Validate the syntax of a config dotpath: non-empty, no empty segments, no + * prototype-pollution / inherited-member segments. Schema validity is not + * checked here — `configSet` handles unknown paths via an interactive + * confirm or a `--config-accept-new-path` opt-in so first-time writes + * under unset namespaces stay possible (see #2400). + */ +function validateConfigDotpath(dotpath: string): DotpathValidation { + if (!dotpath || typeof dotpath !== "string") { + return { ok: false, reason: "key is empty" }; + } + const keys = dotpath.split("."); + for (const key of keys) { + if (!key) return { ok: false, reason: "key contains an empty segment" }; + if (UNSAFE_KEY_SEGMENTS.has(key)) { + return { ok: false, reason: `segment '${key}' is reserved` }; + } + } + return { ok: true }; +} + +/** + * Walk a dotpath and report the first reason `configSet` should refuse it: + * + * - Numeric segment: would target an array index, but `setDotpath` always + * materialises plain objects, so allowing this would either clobber an + * existing array or create a confusingly object-shaped "array". + * - Non-object ancestor: an existing intermediate value (string, number, + * null, array, …) would be silently overwritten by `setDotpath` on its + * way to the leaf. + * + * Missing ancestors are fine — they get materialised on write. Returns + * `null` when no refusal reason applies. + */ +function findClobberingAncestor( + obj: ConfigValue, + dotpath: string, +): { segment: string; reason: string } | null { const keys = dotpath.split("."); - if (keys.some((key) => !key)) return false; + + for (let i = 0; i < keys.length; i++) { + if (/^\d+$/.test(keys[i])) { + return { + segment: keys.slice(0, i + 1).join("."), + reason: "is a numeric segment, but 'config set' does not support array editing", + }; + } + } + + if (keys.length <= 1) return null; let current: ConfigValue = obj; - for (const key of keys) { - if (!isConfigObject(current)) return false; - if (!Object.prototype.hasOwnProperty.call(current, key)) return false; - current = current[key]; + for (let i = 0; i < keys.length - 1; i++) { + if (!isConfigObject(current)) { + return { + segment: keys.slice(0, i).join(".") || "(root)", + reason: `is ${describeNonConfigValue(current)}, not a config object`, + }; + } + const key = keys[i]; + if (!Object.prototype.hasOwnProperty.call(current, key)) { + return null; + } + const next = current[key]; + if (!isConfigObject(next)) { + return { + segment: keys.slice(0, i + 1).join("."), + reason: `is ${describeNonConfigValue(next)}, not a config object`, + }; + } + current = next; + } + return null; +} + +function describeNonConfigValue(value: ConfigValue): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "an array"; + return `a ${typeof value}`; +} + +/** + * Decide what to do when `config set` targets a key that does not yet exist. + * Returns `accept` if an explicit override (CLI flag or env) is in effect, + * `prompt` if the caller should ask the user interactively, and `refuse` + * otherwise. Inputs are passed in so the gate can be tested without + * touching `process.env` or `process.stdin`. + */ +type NewKeyGate = { mode: "accept" } | { mode: "prompt" } | { mode: "refuse" }; + +interface NewKeyGateInputs { + acceptNewPath?: boolean; + acceptEnv?: string; + isTTY?: boolean; + nonInteractiveEnv?: string; +} + +function classifyNewKeyGate(inputs: NewKeyGateInputs): NewKeyGate { + if (inputs.acceptNewPath === true || inputs.acceptEnv === "1") { + return { mode: "accept" }; } - return true; + const interactive = !!inputs.isTTY && inputs.nonInteractiveEnv !== "1"; + if (!interactive) { + return { mode: "refuse" }; + } + return { mode: "prompt" }; } /** @@ -299,9 +404,10 @@ interface ConfigSetOpts { key?: string | null; value?: string | null; restart?: boolean; + acceptNewPath?: boolean; } -function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { +async function configSet(sandboxName: string, opts: ConfigSetOpts = {}): Promise { validateName(sandboxName, "sandbox name"); if (!opts.key) { @@ -316,16 +422,22 @@ function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { process.exit(1); } + const dotpathCheck = validateConfigDotpath(opts.key); + if (!dotpathCheck.ok) { + console.error(` Invalid config key '${opts.key}': ${dotpathCheck.reason}.`); + process.exit(1); + } + const target = resolveAgentConfig(sandboxName); - // 1. Read current config + // Read current config console.log(` Reading ${target.agentName} config...`); const config = readSandboxConfig(sandboxName, target); - // 2. Parse and validate value + // Parse and validate value const parsedValue = parseCliConfigValue(opts.value); - // 3. Validate URLs for SSRF. validateUrlValue no-ops on non-URL input, + // Validate URLs for SSRF. validateUrlValue no-ops on non-URL input, // so run it for every string to avoid bypasses via mixed-case schemes // ("HTTP://127.0.0.1") or leading whitespace. if (typeof parsedValue === "string") { @@ -338,36 +450,70 @@ function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { } } - // 4. Check that we're not modifying the gateway section (contains auth tokens) + // Check that we're not modifying the gateway section (contains auth tokens) if (opts.key.startsWith("gateway.") || opts.key === "gateway") { console.error(" Cannot modify the gateway section directly."); console.error(" Use `nemoclaw config rotate-token` for credential changes."); process.exit(1); } - if (!isRecognizedConfigPath(config, opts.key)) { - console.error( - ` Key validation failed: "${opts.key}" is not a recognized ${target.agentName} config path.`, - ); - process.exit(1); - } - - // 5. Show what will change + // Show what will change const oldValue = extractDotpath(config, opts.key); console.log(` Agent: ${target.agentName}`); console.log(` Key: ${opts.key}`); console.log(` Old value: ${oldValue !== undefined ? JSON.stringify(oldValue) : "(not set)"}`); console.log(` New value: ${JSON.stringify(parsedValue)}`); - // 6. Apply change + // Refuse outright if writing this path would silently overwrite an + // existing scalar ancestor or target an array index — setDotpath would + // either replace the scalar with a fresh empty object or clobber the + // array on its way to the leaf. + const refusal = findClobberingAncestor(config, opts.key); + if (refusal) { + console.error( + ` Cannot set '${opts.key}' in ${target.agentName} config: '${refusal.segment}' ${refusal.reason}.`, + ); + process.exit(1); + } + + // First-time writes go through a confirmation gate so users get a + // signal when they are creating a brand-new key (which may be a typo) + // without coupling the validator to OpenClaw's evolving config schema + // (see #2400). + if (oldValue === undefined) { + const gate = classifyNewKeyGate({ + acceptNewPath: opts.acceptNewPath, + acceptEnv: process.env.NEMOCLAW_CONFIG_ACCEPT_NEW_PATH, + isTTY: process.stdin.isTTY, + nonInteractiveEnv: process.env.NEMOCLAW_NON_INTERACTIVE, + }); + if (gate.mode === "refuse") { + console.error( + ` Key '${opts.key}' does not currently exist in the ${target.agentName} config.`, + ); + console.error( + " Re-run interactively, pass --config-accept-new-path, or set NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1.", + ); + process.exit(1); + } + if (gate.mode === "prompt") { + const confirmed = await confirmYesNo(" Write this new key? [y/N] "); + if (!confirmed) { + console.error(" Aborted."); + process.exit(1); + } + } + } + + // Apply change setDotpath(config, opts.key, parsedValue); - // 7. Write to temp file in the agent's native format + // Write to temp file in the agent's native format const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-config-")); const tmpFile = path.join(tmpDir, target.configFile); fs.writeFileSync(tmpFile, serializeConfig(config, target.format), { mode: 0o600 }); - // 8. Write config to sandbox via kubectl exec (bypasses Landlock) + // Write config to sandbox via kubectl exec (bypasses Landlock) console.log(` Writing config to sandbox (${target.configPath})...`); const content = fs.readFileSync(tmpFile, "utf-8"); execFileSync( @@ -392,7 +538,7 @@ function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { { input: content, stdio: ["pipe", "pipe", "pipe"], timeout: 15000 }, ); - // 9. Fix ownership via kubectl exec (bypasses Landlock) + // Fix ownership via kubectl exec (bypasses Landlock) try { execFileSync( "docker", @@ -417,7 +563,7 @@ function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { // Best effort — chown failure is non-fatal } - // 10. Cleanup temp + // Cleanup temp try { fs.unlinkSync(tmpFile); fs.rmdirSync(tmpDir); @@ -425,7 +571,7 @@ function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { // Best effort } - // 11. Audit log + // Audit log appendAuditEntry({ action: "shields_down", sandbox: sandboxName, @@ -435,7 +581,7 @@ function configSet(sandboxName: string, opts: ConfigSetOpts = {}): void { console.log(` ${target.agentName} config updated.`); - // 12. Restart if requested + // Restart if requested if (opts.restart) { console.log(" Restarting sandbox agent process..."); const restartBinary = getOpenshellBinary(); @@ -601,6 +747,20 @@ function readStdin(): Promise { }); } +/** + * Ask a yes/no question on stderr. Returns true only when the answer matches + * /^y(es)?$/i — empty, "no", or unparseable input is treated as no. + */ +function confirmYesNo(prompt: string): Promise { + return new Promise((resolve) => { + const rl = readline.createInterface({ input: process.stdin, output: process.stderr }); + rl.question(prompt, (answer: string) => { + rl.close(); + resolve(/^y(es)?$/i.test(answer.trim())); + }); + }); +} + // --------------------------------------------------------------------------- // Exports // --------------------------------------------------------------------------- @@ -612,7 +772,9 @@ export { resolveAgentConfig, extractDotpath, setDotpath, - isRecognizedConfigPath, + validateConfigDotpath, + findClobberingAncestor, + classifyNewKeyGate, validateUrlValue, readStdin, }; diff --git a/src/nemoclaw.ts b/src/nemoclaw.ts index 162f0b75f32..fdceef0579f 100644 --- a/src/nemoclaw.ts +++ b/src/nemoclaw.ts @@ -3806,17 +3806,24 @@ const [cmd, ...args] = process.argv.slice(2); break; } case "set": { - const setOpts: { key: string | null; value: string | null; restart: boolean } = { + const setOpts: { + key: string | null; + value: string | null; + restart: boolean; + acceptNewPath: boolean; + } = { key: null, value: null, restart: false, + acceptNewPath: false, }; for (let i = 1; i < actionArgs.length; i++) { if (actionArgs[i] === "--key") setOpts.key = actionArgs[++i]; else if (actionArgs[i] === "--value") setOpts.value = actionArgs[++i]; else if (actionArgs[i] === "--restart") setOpts.restart = true; + else if (actionArgs[i] === "--config-accept-new-path") setOpts.acceptNewPath = true; } - sandboxConfig.configSet(cmd, setOpts); + await sandboxConfig.configSet(cmd, setOpts); break; } case "rotate-token": { @@ -3834,7 +3841,9 @@ const [cmd, ...args] = process.argv.slice(2); default: console.error(" Usage: nemoclaw config "); console.error(" get [--key dotpath] [--format json|yaml]"); - console.error(" set --key --value [--restart]"); + console.error( + " set --key --value [--restart] [--config-accept-new-path]", + ); console.error(" rotate-token [--from-env ] [--from-stdin]"); process.exit(1); } diff --git a/test/config-set.test.ts b/test/config-set.test.ts index 14e6e8817aa..382326d617b 100644 --- a/test/config-set.test.ts +++ b/test/config-set.test.ts @@ -8,7 +8,9 @@ import { describe, it, expect } from "vitest"; const require = createRequire(import.meta.url); const { extractDotpath, - isRecognizedConfigPath, + validateConfigDotpath, + findClobberingAncestor, + classifyNewKeyGate, setDotpath, validateUrlValue, resolveAgentConfig, @@ -93,46 +95,151 @@ describe("config set helpers", () => { }); }); - describe("isRecognizedConfigPath", () => { - it("accepts an existing top-level key", () => { - expect(isRecognizedConfigPath({ version: 1 }, "version")).toBe(true); + describe("validateConfigDotpath", () => { + it("accepts a top-level key", () => { + expect(validateConfigDotpath("version")).toEqual({ ok: true }); }); - it("accepts an existing nested key path", () => { - expect( - isRecognizedConfigPath( - { agents: { defaults: { model: { primary: "gpt-5.4" } } } }, - "agents.defaults.model.primary", - ), - ).toBe(true); + it("accepts a deeply nested path", () => { + expect(validateConfigDotpath("provider.compatible-endpoint.timeoutSeconds")).toEqual({ + ok: true, + }); + }); + + it("rejects empty input", () => { + expect(validateConfigDotpath("").ok).toBe(false); + }); + + it("rejects an empty segment in the middle", () => { + expect(validateConfigDotpath("agents..defaults").ok).toBe(false); + }); + + it("rejects a leading or trailing dot", () => { + expect(validateConfigDotpath(".agents").ok).toBe(false); + expect(validateConfigDotpath("agents.").ok).toBe(false); + }); + + it("rejects prototype-pollution segments anywhere in the path", () => { + expect(validateConfigDotpath("__proto__").ok).toBe(false); + expect(validateConfigDotpath("agents.constructor").ok).toBe(false); + expect(validateConfigDotpath("agents.prototype.config").ok).toBe(false); + expect(validateConfigDotpath("provider.__proto__.polluted").ok).toBe(false); + expect(validateConfigDotpath("tools.hasOwnProperty").ok).toBe(false); + expect(validateConfigDotpath("toString").ok).toBe(false); + }); + + it("returns a reason describing the failure", () => { + const result = validateConfigDotpath("agents..defaults"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/empty segment/); + }); + }); + + describe("findClobberingAncestor", () => { + it("returns null for a top-level path (no ancestors to clobber)", () => { + expect(findClobberingAncestor({ a: 1 }, "a")).toBeNull(); + expect(findClobberingAncestor({}, "newKey")).toBeNull(); + }); + + it("returns null when every existing ancestor is a config object", () => { + expect(findClobberingAncestor({ a: { b: { c: 1 } } }, "a.b.c")).toBeNull(); + expect(findClobberingAncestor({ a: { b: {} } }, "a.b.newLeaf")).toBeNull(); + }); + + it("returns null when an ancestor segment is missing entirely", () => { + expect(findClobberingAncestor({}, "a.b.c")).toBeNull(); + expect(findClobberingAncestor({ a: { b: {} } }, "a.b.c.d.e")).toBeNull(); + }); + + it("refuses numeric segments anywhere in the path", () => { + const top = findClobberingAncestor({}, "0"); + expect(top).not.toBeNull(); + expect(top?.segment).toBe("0"); + expect(top?.reason).toMatch(/numeric/i); + + const mid = findClobberingAncestor({}, "tools.0.name"); + expect(mid).not.toBeNull(); + expect(mid?.segment).toBe("tools.0"); + expect(mid?.reason).toMatch(/array editing/i); + }); + + it("describes a string ancestor as 'a string'", () => { + const result = findClobberingAncestor({ a: "scalar" }, "a.b"); + expect(result).toEqual({ segment: "a", reason: "is a string, not a config object" }); }); - it("accepts existing keys whose value is null", () => { - expect(isRecognizedConfigPath({ provider: { endpoint: null } }, "provider.endpoint")).toBe( - true, + it("describes a number or boolean ancestor by typeof", () => { + expect(findClobberingAncestor({ a: 42 }, "a.b")?.reason).toBe( + "is a number, not a config object", + ); + expect(findClobberingAncestor({ a: { b: true } }, "a.b.c")?.reason).toBe( + "is a boolean, not a config object", ); }); - it("rejects an unknown top-level key", () => { - expect(isRecognizedConfigPath({ version: 1 }, "inference.endpoint")).toBe(false); + it("describes a null ancestor as 'null'", () => { + const result = findClobberingAncestor({ a: null }, "a.b"); + expect(result).toEqual({ segment: "a", reason: "is null, not a config object" }); }); - it("rejects an unknown nested key", () => { - expect( - isRecognizedConfigPath( - { agents: { defaults: { model: { primary: "gpt-5.4" } } } }, - "agents.defaults.model.secondary", - ), - ).toBe(false); + it("describes an array ancestor as 'an array'", () => { + const result = findClobberingAncestor({ a: [1, 2, 3] }, "a.b"); + expect(result).toEqual({ segment: "a", reason: "is an array, not a config object" }); + }); + + it("identifies the deepest blocking ancestor along the path", () => { + const result = findClobberingAncestor({ a: { b: { c: "leaf" } } }, "a.b.c.d"); + expect(result?.segment).toBe("a.b.c"); + expect(result?.reason).toMatch(/string/); + }); + }); + + describe("classifyNewKeyGate", () => { + it("accepts when --config-accept-new-path is set, even without a TTY", () => { + expect(classifyNewKeyGate({ acceptNewPath: true, isTTY: false })).toEqual({ + mode: "accept", + }); }); - it("rejects malformed dotpaths", () => { - expect(isRecognizedConfigPath({ version: 1 }, "agents..defaults")).toBe(false); + it("accepts when NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1, even without a TTY", () => { + expect(classifyNewKeyGate({ acceptEnv: "1", isTTY: false })).toEqual({ + mode: "accept", + }); }); - it("rejects prototype-inherited keys", () => { - expect(isRecognizedConfigPath({}, "toString")).toBe(false); - expect(isRecognizedConfigPath({ safe: {} }, "safe.constructor")).toBe(false); + it("treats env values other than '1' as not accepted", () => { + expect(classifyNewKeyGate({ acceptEnv: "true", isTTY: false })).toEqual({ + mode: "refuse", + }); + expect(classifyNewKeyGate({ acceptEnv: "yes", isTTY: false })).toEqual({ + mode: "refuse", + }); + expect(classifyNewKeyGate({ acceptEnv: "", isTTY: false })).toEqual({ + mode: "refuse", + }); + }); + + it("refuses when stdin is not a TTY and no override is in effect", () => { + expect(classifyNewKeyGate({ isTTY: false })).toEqual({ mode: "refuse" }); + }); + + it("refuses when NEMOCLAW_NON_INTERACTIVE=1, even on a TTY", () => { + expect(classifyNewKeyGate({ isTTY: true, nonInteractiveEnv: "1" })).toEqual({ + mode: "refuse", + }); + }); + + it("prompts on a TTY when no override is in effect", () => { + expect(classifyNewKeyGate({ isTTY: true })).toEqual({ mode: "prompt" }); + }); + + it("override beats NEMOCLAW_NON_INTERACTIVE", () => { + expect( + classifyNewKeyGate({ acceptNewPath: true, isTTY: true, nonInteractiveEnv: "1" }), + ).toEqual({ mode: "accept" }); + expect( + classifyNewKeyGate({ acceptEnv: "1", isTTY: false, nonInteractiveEnv: "1" }), + ).toEqual({ mode: "accept" }); }); });