fix(cli): harden config file permission handling#1370
Conversation
…lidation 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) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a permission-aware config I/O module with atomic write semantics and a dedicated permission error type; replaced inline fs/JSON logic in registry and credentials with the new helpers; added preset validation in policies; added tests and a CommonJS shim for the compiled config-io output. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CIO as config-io
participant FS as Filesystem
App->>CIO: writeConfigFile(targetPath, data)
CIO->>CIO: ensureConfigDir(parentDir)
CIO->>FS: mkdir -p parentDir (mode 0o700)
FS-->>CIO: success / EACCES
alt EACCES
CIO->>App: throw ConfigPermissionError (with remediation)
else Success
CIO->>FS: write temp file (0o600, PID-suffix)
FS-->>CIO: write success / error
CIO->>FS: rename(temp, target)
FS-->>CIO: rename success / EACCES
alt Rename EACCES
CIO->>FS: unlink(temp) [best-effort]
CIO->>App: throw ConfigPermissionError
else Success
CIO->>App: return
end
end
sequenceDiagram
participant App as Application
participant CIO as config-io
participant FS as Filesystem
App->>CIO: readConfigFile(path, default)
CIO->>FS: stat/access path
FS-->>CIO: exists / missing / EACCES
alt EACCES
CIO->>App: throw ConfigPermissionError
else Missing
CIO->>App: return default
else Exists
CIO->>FS: readFile(path)
FS-->>CIO: contents
CIO->>CIO: JSON.parse(contents)
alt Parse error
CIO->>App: return default
else Success
CIO->>App: return parsed data
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/policies.js`:
- Around line 65-73: The current check in validatePreset using
presetContent.includes("binaries:") can be fooled by comments or scalars;
instead parse the YAML and detect a real top-level key. Replace the substring
check in validatePreset with a YAML parse (e.g., yaml.parse / js-yaml.safeLoad)
of presetContent and then check for Object.prototype.hasOwnProperty.call(parsed,
"binaries") (and that parsed.binaries is not undefined/null); if parsing fails,
log a warning and treat the preset as missing the binaries section so the
original warning is emitted for presetName.
In `@src/lib/config-io.ts`:
- Around line 29-41: The current buildRemediation() message assumes sudo is
available; update it to include non-sudo fallback actions so environments
without sudo get actionable guidance. In the buildRemediation() function
(reference: nemoclawDir and HOME usage) add alternative suggestions such as
recreating the config under the current user's home (e.g., remove or move the
directory if writable), instructing to remove the directory without sudo when
the user owns it (e.g., rm -rf $HOME/.nemoclaw), and advising to relocate or
initialize config in a user-writable path (for example creating a new config
under $HOME or specifying an alternative CONFIG_HOME), so the error message
covers both sudo and non-sudo environments. Ensure the text clearly
distinguishes when sudo is required vs when the non-sudo command applies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b236f71c-5208-4de3-9a9b-e2b0d59af3a1
📒 Files selected for processing (7)
bin/lib/config-io.jsbin/lib/credentials.jsbin/lib/policies.jsbin/lib/registry.jssrc/lib/config-io.test.tssrc/lib/config-io.tstest/registry.test.js
| 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"); |
There was a problem hiding this comment.
Remediation still assumes sudo exists.
Issue #692 explicitly includes environments where sudo is unavailable, but both suggested fixes here still require it. In those installs the new ConfigPermissionError is still not actionable. Please add a non-sudo fallback, e.g. recreating config under a user-writable HOME or config directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/config-io.ts` around lines 29 - 41, The current buildRemediation()
message assumes sudo is available; update it to include non-sudo fallback
actions so environments without sudo get actionable guidance. In the
buildRemediation() function (reference: nemoclawDir and HOME usage) add
alternative suggestions such as recreating the config under the current user's
home (e.g., remove or move the directory if writable), instructing to remove the
directory without sudo when the user owns it (e.g., rm -rf $HOME/.nemoclaw), and
advising to relocate or initialize config in a user-writable path (for example
creating a new config under $HOME or specifying an alternative CONFIG_HOME), so
the error message covers both sudo and non-sudo environments. Ensure the text
clearly distinguishes when sudo is required vs when the non-sudo command
applies.
Convert the last 3 blocked-by-#782 CJS modules to TypeScript: - credentials.js → src/lib/credentials.ts - registry.js → src/lib/registry.ts - policies.js → src/lib/policies.ts 716 CLI tests pass. Coverage ratchet passes. Depends on #1370 (config-io module). Relates to #924. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Nice consolidation — the deduplication of atomic-write logic from credentials.js and registry.js into a shared module is clean, and the ConfigPermissionError with remediation hints is a real UX win for #692.
A few items worth considering before merge (inline comments below).
wscurran
added
NemoClaw CLI
- 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) <noreply@anthropic.com>
|
All four items addressed in a63e957:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/policies.js`:
- Around line 253-256: The calls to applyPreset (used in the resume flow,
interactive flow, and single selection flow) are ignoring its boolean return and
thus silently continuing on failure; update each call site that currently just
calls applyPreset (notably the resume, interactive and single-selection code
paths) to check the return value and handle failure the same way as the other
sites: either throw an error when applyPreset returns false or wrap the call in
the same try/catch + retry logic used at the other locations (the handlers
around applyPreset that perform retries at lines noted in the review), ensuring
failures are logged and cause the flow to abort or retry rather than silently
proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c59564c8-12c4-4072-badd-41205052bfa1
📒 Files selected for processing (3)
bin/lib/policies.jsbin/lib/registry.jssrc/lib/config-io.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/config-io.ts
The selectFromList function was added to policies.js on main (via #1370) after our TS migration branched. Add the typed implementation to keep the TS module in sync. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
changed the title
|
Reviewed the latest diff and the existing review threads/comments. The config-dir hardening looks good overall: tightening pre-existing dir perms back to One thing still looks unresolved to me in |
|
Addressed in 2322fe3. Added non-
Kept the existing Validation:
|
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Nice work on the permission hardening — the TOCTOU fix, ensureConfigDir tightening, and actionable remediation messages are all solid improvements. A few things to address:
1. shellQuote import may not work from runner.ts
config-io.ts does import { shellQuote } from "./runner", but runner.ts is CommonJS (module.exports). Every other TS file in the repo uses const { shellQuote } = require("./runner") or imports via the skill-install.ts re-export. This named ESM import from a CJS module may fail at build or runtime depending on tsconfig settings.
Suggest: use const { shellQuote } = require("./runner") to match the existing pattern, or import from skill-install.ts.
2. Constructor overload is fragile
The dual-signature ConfigPermissionError constructor sniffs the second argument to decide which overload was called:
const action =
configPathOrAction === "read" ||
configPathOrAction === "write" ||
configPathOrAction === "create directory"
? configPathOrAction
: null;If someone passes a path that happens to be "read" or "write", it silently picks the wrong branch. Since all call sites are updated to the new (message, configPath, cause?) form in this PR, the legacy overload isn't needed. Consider dropping it and the legacy constructor test.
3. readConfigFile — dead-code ENOENT check
if ((error as NodeJS.ErrnoException).code === "ENOENT") {
return fallback;
}
return fallback;The explicit ENOENT check is dead code since the final return fallback handles it too. If the intent is to only tolerate ENOENT (and crash on corrupt JSON), the last line should be throw error. If the intent is to tolerate all non-permission errors (matching the old behavior), the ENOENT check can be removed. Either way, one of these lines should go.
4. Minor: process.getuid?.() fallback
const recoveryHome = path.join(os.tmpdir(), `nemoclaw-home-${process.getuid?.() ?? "user"}`);On Windows, this falls back to the literal string "user", so multiple users would collide on the same recovery path. Low priority since NemoClaw targets Linux — a comment noting the assumption would suffice.
|
Thanks for the config I/O hardening — the TOCTOU fix and explicit writability check in |
wscurran
added
the
status: rebase
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — all review feedback addressed. Clean config-io hardening: TOCTOU fix, 0o700 enforcement on pre-existing dirs, rich remediation messages, consistent permission errors through acquireLock. Good test coverage.
…resolution
config-io.ts importing shellQuote from runner.ts caused vitest to load
runner.ts from source, where the CJS require("./platform") fails because
Node cannot resolve .ts extensions. Extract shellQuote into shell-quote.ts
so config-io.ts no longer pulls in runner.ts and its platform dependency.
Fixes the MODULE_NOT_FOUND error in sandbox-version.test.ts and
secret-redaction.test.ts introduced by #1370.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…#1900) ## Summary - **main is broken** since commit `57ea768f` (PR #1370) — the `checks` workflow fails with `Cannot find module './platform'` in `sandbox-version.test.ts` and `secret-redaction.test.ts` - Root cause: #1370 added `import { shellQuote } from "./runner"` to `config-io.ts`, which causes vitest to load `runner.ts` from source. `runner.ts` uses CJS `require("./platform")` which fails because Node's native `require()` cannot resolve `.ts` extensions - Fix: extract `shellQuote` into a standalone `shell-quote.ts` module with no heavy dependencies, and point `config-io.ts` at it. `runner.ts` keeps its own copy for CJS consumers ### Files changed | File | Change | |------|--------| | `src/lib/shell-quote.ts` | New standalone ESM module with `shellQuote` | | `src/lib/config-io.ts` | Import from `./shell-quote` instead of `./runner` | | `test/runner.test.ts` | Update regression guard to expect both `runner.ts` and `shell-quote.ts` | ## Test plan - [x] `sandbox-version.test.ts` passes (was failing) - [x] `secret-redaction.test.ts` passes (was failing) - [x] `runner.test.ts` regression guard passes (updated) - [x] Full CLI test suite: 71 files, 1259 tests, 0 failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Internal code reorganization and test updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 participants
Summary
This PR was originally broader, but most of that work has since landed on
mainindependently.After merging
maininto this branch, the remaining diff is now narrowly focused on config I/O hardening:ConfigPermissionErrormessages with actionable remediation for wrong-owner / sudo-created~/.nemoclawstate0700existsSync()/ read TOCTOU pattern in config readsensureConfigDir()Files still changed in this PR:
src/lib/config-io.tssrc/lib/config-io.test.tssrc/lib/registry.tsWhy keep this PR
The main TypeScript migration and initial
config-ioextraction already landed onmain.This PR now carries only the remaining permission-hardening and follow-up fixes that were not absorbed upstream.
Testing
npm run build:clinpx vitest run src/lib/config-io.test.ts test/registry.test.js test/policies.test.jsRelated issues