← Back to PRs

#18954: fix(security): secure cron, browser, settings dirs in doctor --fix

by BinHPdev open 2026-02-17 06:33 View on GitHub →
channel: mattermost size: S
## Summary `fixSecurityFootguns` already tightened permissions on the state directory, config file, credentials, and agent session paths. However, the `cron/`, `browser/`, and `settings/` subdirectories were left with default permissions (755/644), leaving `cron/jobs.json` (scheduled task payloads), browser session state/cookies, and settings world-readable. Closes #18866 ## Changes - `src/security/fix.ts` – after the existing credential/agent hardening, iterate over `cron/`, `browser/`, `settings/` subdirectories and apply 700 permissions; also chmod all files inside `cron/` to 600 - `src/security/fix.test.ts` – new test case that creates all three subdirectories with 755/644 permissions and verifies they get tightened to 700/600 ## Test plan - [x] New test passes: `pnpm vitest run src/security/fix.test.ts` (6/6) - [x] All existing security fix tests still pass - [ ] Manual: run `openclaw doctor --fix` and verify `ls -la ~/.openclaw/cron/` shows 700/600 ## QA - [x] Missing directories are safely skipped (ENOENT handling in `safeChmod`) - [x] Symlinks are skipped (existing `safeChmod` guard) - [x] Windows path handled via existing `applyPerms` abstraction (icacls) - [x] No type errors 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR extends `fixSecurityFootguns` to tighten permissions on three previously unsecured state subdirectories (`cron/`, `browser/`, `settings/`), closing a gap where sensitive data like cron job payloads, browser session state, and settings were world-readable. - Applies `0o700` to `cron/`, `browser/`, and `settings/` directories and `0o600` to files inside `cron/` using the existing cross-platform `applyPerms` abstraction (handles both Unix chmod and Windows icacls). - Adds a comprehensive test verifying all three directories and cron files get tightened from `755`/`644` to `700`/`600`. - Changelog entry correctly placed under Fixes with issue reference. - Minor gap: `cron/runs/` subdirectory (containing per-job `.jsonl` run logs) is not recursively secured, though parent `cron/` at `700` provides adequate first-layer protection. <h3>Confidence Score: 4/5</h3> - This PR is safe to merge — it extends existing permission-hardening patterns with proper error handling and cross-platform support. - Score of 4 reflects a clean, well-tested security improvement that follows existing patterns. The only gap is the `cron/runs/` subdirectory not being recursively secured, which is a defense-in-depth improvement rather than a vulnerability since the parent directory permissions already prevent unauthorized access. - `src/security/fix.ts` — the `cron/runs/` subdirectory and its JSONL files are not secured by the new code, though parent directory permissions provide adequate protection. <sub>Last reviewed commit: 38df423</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs