From 38424c6738ac081ef650befebf61bfbfe341f17b Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Thu, 2 Apr 2026 14:17:14 -0700 Subject: [PATCH 1/3] fix(cli): add config-io module for safe config file I/O and preset validation Add src/lib/config-io.ts with atomic JSON read/write (temp + rename), EACCES error handling with user-facing remediation hints, and directory permission enforcement. - Refactor credentials.js to use readConfigFile/writeConfigFile - Refactor registry.js to use readConfigFile/writeConfigFile - Add validatePreset() to policies.js (warns on missing binaries section) - ConfigPermissionError with actionable remediation (sudo chown / rm) - Co-located tests for config-io module Fixes #692, #606. Supersedes the config-io and preset validation parts of #782 (without the runner.js redaction, which landed separately in #1246). Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/lib/config-io.js | 7 +++ bin/lib/credentials.js | 18 +----- bin/lib/policies.js | 19 ++++++ bin/lib/registry.js | 27 +-------- src/lib/config-io.test.ts | 107 +++++++++++++++++++++++++++++++++ src/lib/config-io.ts | 123 ++++++++++++++++++++++++++++++++++++++ test/registry.test.js | 4 +- 7 files changed, 265 insertions(+), 40 deletions(-) create mode 100644 bin/lib/config-io.js create mode 100644 src/lib/config-io.test.ts create mode 100644 src/lib/config-io.ts diff --git a/bin/lib/config-io.js b/bin/lib/config-io.js new file mode 100644 index 000000000..e2361d3cf --- /dev/null +++ b/bin/lib/config-io.js @@ -0,0 +1,7 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Thin re-export shim — the implementation lives in src/lib/config-io.ts, +// compiled to dist/lib/config-io.js. + +module.exports = require("../../dist/lib/config-io"); diff --git a/bin/lib/credentials.js b/bin/lib/credentials.js index 683ba80b8..6a73cbde4 100644 --- a/bin/lib/credentials.js +++ b/bin/lib/credentials.js @@ -6,6 +6,7 @@ const path = require("path"); const os = require("os"); const readline = require("readline"); const { execFileSync } = require("child_process"); +const { readConfigFile, writeConfigFile } = require("./config-io"); const UNSAFE_HOME_PATHS = new Set(["/tmp", "/var/tmp", "/dev/shm", "/"]); @@ -56,15 +57,7 @@ function getCredsFile() { } function loadCredentials() { - try { - const file = getCredsFile(); - if (fs.existsSync(file)) { - return JSON.parse(fs.readFileSync(file, "utf-8")); - } - } catch { - /* ignored */ - } - return {}; + return readConfigFile(getCredsFile(), {}); } function normalizeCredentialValue(value) { @@ -73,14 +66,9 @@ function normalizeCredentialValue(value) { } function saveCredential(key, value) { - const dir = getCredsDir(); - const file = getCredsFile(); - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); - fs.chmodSync(dir, 0o700); const creds = loadCredentials(); creds[key] = normalizeCredentialValue(value); - fs.writeFileSync(file, JSON.stringify(creds, null, 2), { mode: 0o600 }); - fs.chmodSync(file, 0o600); + writeConfigFile(getCredsFile(), creds); } function getCredential(key) { diff --git a/bin/lib/policies.js b/bin/lib/policies.js index 1e3bb8f34..e3eebe9af 100644 --- a/bin/lib/policies.js +++ b/bin/lib/policies.js @@ -57,6 +57,22 @@ function getPresetEndpoints(content) { return hosts; } +/** + * Validate that a preset contains a binaries section. + * Presets without binaries cause 403 errors because the egress proxy + * has no approved binary list and denies all traffic (ref: #676). + */ +function validatePreset(presetContent, presetName) { + if (!presetContent.includes("binaries:")) { + console.warn( + ` Warning: preset '${presetName}' has no binaries section — ` + + `this will cause 403 errors in the sandbox (ref: #676)`, + ); + return false; + } + return true; +} + /** * Extract just the network_policies entries (indented content under * the `network_policies:` key) from a preset file, stripping the @@ -233,6 +249,8 @@ function applyPreset(sandboxName, presetName) { return false; } + validatePreset(presetContent, presetName); + const presetEntries = extractPresetEntries(presetContent); if (!presetEntries) { console.error(` Preset ${presetName} has no network_policies section.`); @@ -294,6 +312,7 @@ module.exports = { loadPreset, getPresetEndpoints, extractPresetEntries, + validatePreset, parseCurrentPolicy, buildPolicySetCommand, buildPolicyGetCommand, diff --git a/bin/lib/registry.js b/bin/lib/registry.js index 885ea8c2e..822ba1128 100644 --- a/bin/lib/registry.js +++ b/bin/lib/registry.js @@ -5,6 +5,7 @@ const fs = require("fs"); const path = require("path"); +const { readConfigFile, writeConfigFile } = require("./config-io"); const REGISTRY_FILE = path.join(process.env.HOME || "/tmp", ".nemoclaw", "sandboxes.json"); const LOCK_DIR = REGISTRY_FILE + ".lock"; @@ -121,33 +122,11 @@ function withLock(fn) { } function load() { - try { - if (fs.existsSync(REGISTRY_FILE)) { - return JSON.parse(fs.readFileSync(REGISTRY_FILE, "utf-8")); - } - } catch { - /* ignored */ - } - return { sandboxes: {}, defaultSandbox: null }; + return readConfigFile(REGISTRY_FILE, { sandboxes: {}, defaultSandbox: null }); } -/** Atomic write: tmp file + rename on the same filesystem. */ function save(data) { - const dir = path.dirname(REGISTRY_FILE); - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); - const tmp = REGISTRY_FILE + ".tmp." + process.pid; - try { - fs.writeFileSync(tmp, JSON.stringify(data, null, 2), { mode: 0o600 }); - fs.renameSync(tmp, REGISTRY_FILE); - } catch (err) { - // Clean up partial temp file on failure - try { - fs.unlinkSync(tmp); - } catch { - /* best effort */ - } - throw err; - } + writeConfigFile(REGISTRY_FILE, data); } function getSandbox(name) { diff --git a/src/lib/config-io.test.ts b/src/lib/config-io.test.ts new file mode 100644 index 000000000..4fa47ea6c --- /dev/null +++ b/src/lib/config-io.test.ts @@ -0,0 +1,107 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// Import from compiled dist/ for coverage attribution. +import { + ensureConfigDir, + readConfigFile, + writeConfigFile, + ConfigPermissionError, +} from "../../dist/lib/config-io"; + +describe("config-io", () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-config-io-")); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + describe("ensureConfigDir", () => { + it("creates a directory with mode 0o700", () => { + const dir = path.join(tmpDir, "nested", "config"); + ensureConfigDir(dir); + expect(fs.existsSync(dir)).toBe(true); + const stat = fs.statSync(dir); + expect(stat.mode & 0o777).toBe(0o700); + }); + + it("succeeds if directory already exists", () => { + const dir = path.join(tmpDir, "existing"); + fs.mkdirSync(dir, { mode: 0o700 }); + expect(() => ensureConfigDir(dir)).not.toThrow(); + }); + }); + + describe("writeConfigFile + readConfigFile", () => { + it("round-trips JSON data with atomic write", () => { + const file = path.join(tmpDir, "test.json"); + const data = { key: "value", nested: { a: 1 } }; + writeConfigFile(file, data); + + expect(fs.existsSync(file)).toBe(true); + const stat = fs.statSync(file); + expect(stat.mode & 0o777).toBe(0o600); + + const loaded = readConfigFile(file, {}); + expect(loaded).toEqual(data); + }); + + it("creates parent directories", () => { + const file = path.join(tmpDir, "deep", "nested", "config.json"); + writeConfigFile(file, { ok: true }); + expect(readConfigFile(file, null)).toEqual({ ok: true }); + }); + + it("returns default for missing files", () => { + const file = path.join(tmpDir, "nonexistent.json"); + expect(readConfigFile(file, { fallback: true })).toEqual({ fallback: true }); + }); + + it("returns default for corrupt JSON", () => { + const file = path.join(tmpDir, "corrupt.json"); + fs.writeFileSync(file, "not-json"); + expect(readConfigFile(file, "default")).toBe("default"); + }); + + it("cleans up temp file on write failure", () => { + // Write to a read-only directory to trigger failure + const readonlyDir = path.join(tmpDir, "readonly"); + fs.mkdirSync(readonlyDir, { mode: 0o700 }); + const file = path.join(readonlyDir, "test.json"); + // Write once successfully, then make dir read-only + writeConfigFile(file, { first: true }); + fs.chmodSync(readonlyDir, 0o500); + + try { + expect(() => writeConfigFile(file, { second: true })).toThrow(); + } finally { + fs.chmodSync(readonlyDir, 0o700); + } + + // No temp files left behind + const files = fs.readdirSync(readonlyDir); + expect(files.filter((f) => f.includes(".tmp."))).toHaveLength(0); + }); + }); + + describe("ConfigPermissionError", () => { + it("includes remediation message and config path", () => { + const err = new ConfigPermissionError("test error", "/some/path"); + expect(err.name).toBe("ConfigPermissionError"); + expect(err.code).toBe("EACCES"); + expect(err.configPath).toBe("/some/path"); + expect(err.message).toContain("test error"); + expect(err.remediation).toContain("sudo chown"); + expect(err.remediation).toContain(".nemoclaw"); + }); + }); +}); diff --git a/src/lib/config-io.ts b/src/lib/config-io.ts new file mode 100644 index 000000000..281b7d36b --- /dev/null +++ b/src/lib/config-io.ts @@ -0,0 +1,123 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Safe config file I/O with EACCES error handling (#692, #606, #719). + +import fs from "node:fs"; +import path from "node:path"; +import os from "node:os"; + +/** + * Custom error for config permission problems. Carries the path and + * a user-facing remediation message so callers can display it cleanly. + */ +export class ConfigPermissionError extends Error { + code = "EACCES"; + configPath: string; + remediation: string; + + constructor(message: string, configPath: string, cause?: Error) { + const remediation = buildRemediation(); + super(`${message}\n\n${remediation}`); + this.name = "ConfigPermissionError"; + this.configPath = configPath; + this.remediation = remediation; + if (cause) this.cause = cause; + } +} + +function buildRemediation(): string { + const home = process.env.HOME || os.homedir(); + const nemoclawDir = path.join(home, ".nemoclaw"); + return [ + " To fix, run one of:", + "", + ` sudo chown -R $(whoami) ${nemoclawDir}`, + ` # or, if the directory was created by another user:`, + ` sudo rm -rf ${nemoclawDir} && nemoclaw onboard`, + "", + " This usually happens when NemoClaw was first run with sudo", + " or the config directory was created by a different user.", + ].join("\n"); +} + +/** + * Ensure a directory exists with mode 0o700. On EACCES, throws + * ConfigPermissionError with remediation hints. + */ +export function ensureConfigDir(dir: string): void { + try { + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === "EACCES") { + throw new ConfigPermissionError(`Cannot create config directory: ${dir}`, dir, err as Error); + } + throw err; + } + + try { + fs.accessSync(dir, fs.constants.W_OK); + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === "EACCES") { + throw new ConfigPermissionError( + `Config directory exists but is not writable: ${dir}`, + dir, + err as Error, + ); + } + throw err; + } +} + +/** + * Write a JSON config file atomically with mode 0o600. + * Uses write-to-temp + rename to avoid partial writes on crash. + */ +export function writeConfigFile(filePath: string, data: unknown): void { + const dir = path.dirname(filePath); + ensureConfigDir(dir); + + const content = JSON.stringify(data, null, 2); + const tmpFile = filePath + ".tmp." + process.pid; + + try { + fs.writeFileSync(tmpFile, content, { mode: 0o600 }); + fs.renameSync(tmpFile, filePath); + } catch (err: unknown) { + try { + fs.unlinkSync(tmpFile); + } catch { + /* best effort cleanup */ + } + if ((err as NodeJS.ErrnoException).code === "EACCES") { + throw new ConfigPermissionError( + `Cannot write config file: ${filePath}`, + filePath, + err as Error, + ); + } + throw err; + } +} + +/** + * Read and parse a JSON config file. Returns defaultValue on missing + * or corrupt files. On EACCES, throws ConfigPermissionError. + */ +export function readConfigFile(filePath: string, defaultValue: T): T { + try { + if (fs.existsSync(filePath)) { + return JSON.parse(fs.readFileSync(filePath, "utf-8")) as T; + } + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === "EACCES") { + throw new ConfigPermissionError( + `Cannot read config file: ${filePath}`, + filePath, + err as Error, + ); + } + // Corrupt JSON or other non-permission error — return default + } + return defaultValue; +} diff --git a/test/registry.test.js b/test/registry.test.js index 80fd0dded..2318d2743 100644 --- a/test/registry.test.js +++ b/test/registry.test.js @@ -153,7 +153,9 @@ describe("atomic writes", () => { throw Object.assign(new Error("EACCES"), { code: "EACCES" }); }; try { - expect(() => registry.save({ sandboxes: {}, defaultSandbox: null })).toThrow("EACCES"); + expect(() => registry.save({ sandboxes: {}, defaultSandbox: null })).toThrow( + "Cannot write config file", + ); } finally { fs.renameSync = original; } From a63e95775944c0f1f91ba6c175842cc99b4255eb Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Fri, 3 Apr 2026 01:46:21 -0700 Subject: [PATCH 2/3] fix: address review feedback on config-io module - ensureConfigDir: chmod 0o700 on pre-existing dirs with weaker modes (preserves old credentials.js hardening behavior) - readConfigFile: remove TOCTOU existsSync, catch ENOENT directly - acquireLock: use ensureConfigDir for consistent permission errors - applyPreset: bail early when validatePreset returns false Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/lib/policies.js | 4 +++- bin/lib/registry.js | 3 ++- src/lib/config-io.ts | 15 +++++++++++---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/bin/lib/policies.js b/bin/lib/policies.js index 6f55bd9b6..a6e95577a 100644 --- a/bin/lib/policies.js +++ b/bin/lib/policies.js @@ -250,7 +250,9 @@ function applyPreset(sandboxName, presetName) { return false; } - validatePreset(presetContent, presetName); + if (!validatePreset(presetContent, presetName)) { + return false; + } const presetEntries = extractPresetEntries(presetContent); if (!presetEntries) { diff --git a/bin/lib/registry.js b/bin/lib/registry.js index 822ba1128..110599f83 100644 --- a/bin/lib/registry.js +++ b/bin/lib/registry.js @@ -19,7 +19,8 @@ const LOCK_MAX_RETRIES = 120; * Writes an owner file with PID for stale-lock detection via process liveness. */ function acquireLock() { - fs.mkdirSync(path.dirname(REGISTRY_FILE), { recursive: true, mode: 0o700 }); + const { ensureConfigDir } = require("./config-io"); + ensureConfigDir(path.dirname(REGISTRY_FILE)); const sleepBuf = new Int32Array(new SharedArrayBuffer(4)); for (let i = 0; i < LOCK_MAX_RETRIES; i++) { try { diff --git a/src/lib/config-io.ts b/src/lib/config-io.ts index 281b7d36b..9b4c52bfe 100644 --- a/src/lib/config-io.ts +++ b/src/lib/config-io.ts @@ -48,6 +48,12 @@ function buildRemediation(): string { export function ensureConfigDir(dir: string): void { try { fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + // Enforce 0o700 even if the directory already existed at a weaker mode. + // Only tighten permissions — never loosen a more restrictive mode. + const stat = fs.statSync(dir); + if ((stat.mode & 0o777) !== 0o700 && (stat.mode & 0o077) !== 0) { + fs.chmodSync(dir, 0o700); + } } catch (err: unknown) { if ((err as NodeJS.ErrnoException).code === "EACCES") { throw new ConfigPermissionError(`Cannot create config directory: ${dir}`, dir, err as Error); @@ -106,17 +112,18 @@ export function writeConfigFile(filePath: string, data: unknown): void { */ export function readConfigFile(filePath: string, defaultValue: T): T { try { - if (fs.existsSync(filePath)) { - return JSON.parse(fs.readFileSync(filePath, "utf-8")) as T; - } + return JSON.parse(fs.readFileSync(filePath, "utf-8")) as T; } catch (err: unknown) { - if ((err as NodeJS.ErrnoException).code === "EACCES") { + const code = (err as NodeJS.ErrnoException).code; + if (code === "EACCES") { throw new ConfigPermissionError( `Cannot read config file: ${filePath}`, filePath, err as Error, ); } + // ENOENT (missing file) or corrupt JSON — return default + if (code === "ENOENT") return defaultValue; // Corrupt JSON or other non-permission error — return default } return defaultValue; From 50007fbcd74b9e4f07adbd4a93bd8516ab7bc070 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 8 Apr 2026 16:34:56 -0700 Subject: [PATCH 3/3] fix(cli): narrow salvage PR to config-io hardening --- bin/lib/policies.js | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/bin/lib/policies.js b/bin/lib/policies.js index 0187eedf5..64602bd39 100644 --- a/bin/lib/policies.js +++ b/bin/lib/policies.js @@ -58,22 +58,6 @@ function getPresetEndpoints(content) { return hosts; } -/** - * Validate that a preset contains a binaries section. - * Presets without binaries cause 403 errors because the egress proxy - * has no approved binary list and denies all traffic (ref: #676). - */ -function validatePreset(presetContent, presetName) { - if (!presetContent.includes("binaries:")) { - console.warn( - ` Warning: preset '${presetName}' has no binaries section — ` + - `this will cause 403 errors in the sandbox (ref: #676)`, - ); - return false; - } - return true; -} - /** * Extract just the network_policies entries (indented content under * the `network_policies:` key) from a preset file, stripping the @@ -250,10 +234,6 @@ function applyPreset(sandboxName, presetName) { return false; } - if (!validatePreset(presetContent, presetName)) { - return false; - } - const presetEntries = extractPresetEntries(presetContent); if (!presetEntries) { console.error(` Preset ${presetName} has no network_policies section.`); @@ -367,7 +347,6 @@ module.exports = { loadPreset, getPresetEndpoints, extractPresetEntries, - validatePreset, parseCurrentPolicy, buildPolicySetCommand, buildPolicyGetCommand,