-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(cli): harden config file permission handling #1370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
38424c6
da477ba
6d68a7d
1fa5d60
45f3d93
a63e957
e500519
d03d091
4156a14
dab07f5
2419725
064fab3
50007fb
6a96f67
147ab9e
76460b3
a5f34fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } | ||
| } | ||
cv marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| 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"); | ||
|
Comment on lines
+10
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remediation still assumes Issue 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /** | ||
| * 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, | ||
| ); | ||
| } | ||
cv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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<T>(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; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.