fix(security): enforce secure permissions on openclaw.json during migration#187
Conversation
wscurran
added
security
|
Hey @dumko2001, enforcing secure permissions on openclaw.json during migration is important — good catch. The repo has been moving fast though. A rebase onto main would let us review this properly. |
dumko2001
force-pushed
the
fix/h17-openclaw-json-perms
branch
from
9f9dc6d to
cdd2a17
Compare
📝 WalkthroughWalkthroughThe changes add file permission restrictions ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoclaw/src/commands/migration-state.ts (1)
681-690:⚠️ Potential issue | 🟡 MinorApply explicit permission enforcement to internal config during restore.
When
hasExternalConfigis false, the internalopenclaw.jsoninsidestateDiris restored viacopyDirectory()at line 682 without explicit permission enforcement. For consistency with external config handling (which receiveschmodSync(0o600)at line 688), the internal config should also be explicitly chmod'd to 0o600 if it exists in the restored state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 681 - 690, The restore path uses copyDirectory(snapshotStateDir, manifest.stateDir) to restore internal config when manifest.hasExternalConfig is false, but it doesn't explicitly set secure permissions; after the directory copy, check for the internal config file at path.join(manifest.stateDir, "config", "openclaw.json") and, if it exists, call chmodSync(..., 0o600) (mirroring the external config branch that uses copyFileSync and chmodSync) and log the restoration (use logger.info) so internal config permissions are enforced consistently; update the code near the copyDirectory call and reuse manifest.stateDir, hasExternalConfig, copyDirectory, chmodSync, and logger identifiers to locate where to add this.
🧹 Nitpick comments (1)
nemoclaw/src/commands/migration-state.ts (1)
563-565: Consider using atomic permission setting to eliminate race window.The current sequence (write then chmod) leaves a brief window where the file exists with default permissions. For sensitive files, you can pass the
modeoption directly towriteFileSyncto set permissions atomically on file creation:✨ Suggested improvement for atomic permission setting
const configPath = path.join(preparedStateDir, "openclaw.json"); - writeFileSync(configPath, JSON.stringify(config, null, 2)); - chmodSync(configPath, 0o600); + writeFileSync(configPath, JSON.stringify(config, null, 2), { mode: 0o600 });Note: Keep
chmodSyncas a fallback if the file might already exist (though in this case, the directory is freshly created viarmSync+mkdirSync).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 563 - 565, The current sequence writes the config file then calls chmodSync, leaving a small race window; update the write to pass the mode option (e.g., writeFileSync(configPath, JSON.stringify(config, null, 2), { mode: 0o600 })) so the file is created with correct permissions atomically, and keep the existing chmodSync(configPath, 0o600) as a fallback in case the file already existed; apply this change around the code that constructs configPath using preparedStateDir and uses writeFileSync/chmodSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 681-690: The restore path uses copyDirectory(snapshotStateDir,
manifest.stateDir) to restore internal config when manifest.hasExternalConfig is
false, but it doesn't explicitly set secure permissions; after the directory
copy, check for the internal config file at path.join(manifest.stateDir,
"config", "openclaw.json") and, if it exists, call chmodSync(..., 0o600)
(mirroring the external config branch that uses copyFileSync and chmodSync) and
log the restoration (use logger.info) so internal config permissions are
enforced consistently; update the code near the copyDirectory call and reuse
manifest.stateDir, hasExternalConfig, copyDirectory, chmodSync, and logger
identifiers to locate where to add this.
---
Nitpick comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 563-565: The current sequence writes the config file then calls
chmodSync, leaving a small race window; update the write to pass the mode option
(e.g., writeFileSync(configPath, JSON.stringify(config, null, 2), { mode: 0o600
})) so the file is created with correct permissions atomically, and keep the
existing chmodSync(configPath, 0o600) as a fallback in case the file already
existed; apply this change around the code that constructs configPath using
preparedStateDir and uses writeFileSync/chmodSync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42e45e9d-ab02-42b4-9574-0da423d804c4
📒 Files selected for processing (1)
nemoclaw/src/commands/migration-state.ts
4 participants
Rationale
openclaw.json contains sensitive session and routing data that should never be world-readable.
Changes
Added chmodSync(0o600) to all migration-state file write/copy operations.
Verification Results
Summary by CodeRabbit