fix(security): enforce private directory mode 0o700 for managed state dirs#96336
fix(security): enforce private directory mode 0o700 for managed state dirs#96336fsdwen wants to merge 3 commits into
Conversation
|
Codex review: needs changes before merge. Reviewed June 25, 2026, 1:46 AM ET / 05:46 UTC. Summary PR surface: Source +64. Total +64 across 8 files. Reproducibility: yes. Source inspection shows current main still creates managed session directories without explicit private modes, and the PR body includes inspected terminal proof under umask 0002; I did not run tests in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Land the private-directory hardening only after chmod repair is guarded to recognized OpenClaw-managed directories, caller-owned paths are preserved, and focused upgrade/custom-path tests cover the affected runtime paths. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main still creates managed session directories without explicit private modes, and the PR body includes inspected terminal proof under umask 0002; I did not run tests in this read-only review. Is this the best way to solve the issue? No. Managed-only hardening is the right direction, but this patch is not the best mergeable shape until it preserves caller-owned session and transcript parents in all runtime paths. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 56d95b18f4bc. Label changesLabel justifications:
Evidence reviewedPR surface: Source +64. Total +64 across 8 files. View PR surface stats
Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
18e079f to
ef1adec
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
ef1adec to
bc055aa
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
… dirs
mkdirSync({recursive: true}) without explicit mode inherits umask-derived
permissions (775/755 instead of 700). This exposes session transcripts,
tool binaries, plugin skills, and settings directories under
~/.openclaw to group/other users on multi-user systems.
- Add mode:0o700 to 12 mkdirSync calls for managed OpenClaw state dirs
- Add best-effort chmodSync repair for existing dirs after upgrade
- Preserve caller-owned custom paths (session.store, export targets)
Fixes openclaw#92771
Co-Authored-By: Claude <[email protected]>
bc055aa to
daa4aa7
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@joshavant @vincentkoc @hydro13 — requesting review per CONTRIBUTING.md maintainer areas. This PR narrows 0o700 directory hardening to OpenClaw-managed state directories only, addressing the ClawSweeper review findings about caller-owned path overreach. Changes: 5 files, +35/−21 — removes CI: ✅ prod-types, test-types, lint, guards all pass. (1 infra failure on Proof: Terminal evidence and screenshots in PR body. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing this PR as it has been stale for 3+ weeks with unresolved ClawSweeper review findings (caller-owned path boundary leaks in SessionManager.open and Gateway transcript creation). The PR's approach of managed-only hardening is directionally correct but needs a rebase, narrowed chmod guard, and targeted tests before merging. Author can reopen with refreshed branch and addressed findings. |
What Problem This Solves
mkdirSync({recursive: true})without explicitmodeinherits umask-derived permissions (775/755 instead of 700). This exposes session transcripts, tool binaries, plugin skills, and settings directories under~/.openclawto group/other users on multi-user systems.This PR adds
mode: 0o700+ best-effortchmodSyncrepair only to OpenClaw-managed state directories (under~/.openclaw/agents/). Caller-owned paths (project settings, export targets, customsession.storepaths) are NOT affected.Fixes #92771
Why This Change Was Made
The project already has ~10 locations that correctly use
mode: 0o700. Per maintainer review, this PR narrows 0o700 hardening to OpenClaw-managed state directories only:getDefaultSessionDir,SessionManagerctor/open/fork — under~/.openclaw/agents/<id>/cwd/.openclaw/), export paths, customsession.storepaths, transcript parents in store/accessor layersUser Impact
~/.openclaw/agents/created as 0700session.store, export targets, project settings — completely unchangedEvidence
Terminal Proof (umask 0002, Node v24.13.0)
1. Fresh install (umask 0002)
2. Upgrade repair — pre-existing 775 dirs
Simulated upgrade: manually created 775 dirs, then ran fixed agent:
chmodSync repair in getDefaultSessionDir, SessionManager.open().
3. Caller-owned path preservation
Tests
sessions.test.ts— 47 tests ✅config/sessions/— 390 tests ✅🤖 Generated with Claude Code