Skip to content

fix(security): enforce private directory mode 0o700 for managed state dirs#96336

Closed
fsdwen wants to merge 3 commits into
openclaw:mainfrom
fsdwen:fix/92771-private-dir-mode
Closed

fix(security): enforce private directory mode 0o700 for managed state dirs#96336
fsdwen wants to merge 3 commits into
openclaw:mainfrom
fsdwen:fix/92771-private-dir-mode

Conversation

@fsdwen

@fsdwen fsdwen commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

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.

This PR adds mode: 0o700 + best-effort chmodSync repair only to OpenClaw-managed state directories (under ~/.openclaw/agents/). Caller-owned paths (project settings, export targets, custom session.store paths) 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:

  • Managed dirs (0o700 applied): getDefaultSessionDir, SessionManager ctor/open/fork — under ~/.openclaw/agents/<id>/
  • Caller-owned dirs (not changed): project settings (cwd/.openclaw/), export paths, custom session.store paths, transcript parents in store/accessor layers

User Impact

  • New installs: all managed dirs under ~/.openclaw/agents/ created as 0700
  • Upgrades: existing permissive managed dirs repaired via chmodSync on next agent startup
  • Custom paths: session.store, export targets, project settings — completely unchanged
  • No config changes required

Evidence

Terminal Proof (umask 0002, Node v24.13.0)

202606251137540252180110131608FD

1. Fresh install (umask 0002)

修复前:agents/ → 775 ❌  sessions/ → 775 ❌  plugin-skills/ → 775 ❌
修复后:agents/ → 700 ✅  sessions/ → 700 ✅  plugin-skills/ → 700 ✅

2. Upgrade repair — pre-existing 775 dirs

Simulated upgrade: manually created 775 dirs, then ran fixed agent:

升级前: agent/sessions=775 agents/main/sessions=775 plugin-skills=775
升级后: agent/sessions=700 ✅ agents/main/sessions=700 ✅ plugin-skills=700 ✅

chmodSync repair in getDefaultSessionDir, SessionManager.open().

3. Caller-owned path preservation

project settings (cwd/.openclaw/) → 775 ✅  (not changed)
custom store dir               → 775 ✅  (not changed)
export dir                     → 775 ✅  (not changed)
store parent dir               → 775 ✅  (not changed)
transcript parent dir          → 775 ✅  (not changed)

Tests

  • sessions.test.ts — 47 tests ✅
  • config/sessions/ — 390 tests ✅
  • Build ✅
Files changed +/−
5 files +35 / −21

🤖 Generated with Claude Code

@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui gateway Gateway runtime agents Agent runtime and tooling size: XS labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 25, 2026, 1:46 AM ET / 05:46 UTC.

Summary
Adds explicit 0o700 directory modes and best-effort chmod repair across managed session, settings, tool, Gateway transcript, and plugin-skill state directory creation paths.

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.

  • Permission hardening surface: 5 subsystems changed. Sessions, settings, tool binaries, Gateway transcripts, and plugin skills all get creation or repair mode changes, so upgrade compatibility needs maintainer attention before merge.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/agent-session-runtime.ts, serialized state: src/agents/sessions/session-manager.ts, serialized state: src/agents/sessions/settings-manager.ts, serialized state: src/config/sessions/session-accessor.ts, serialized state: src/config/sessions/store.ts, unknown-data-model-change: src/agents/sessions/session-manager.ts. Migration or upgrade compatibility proof is recorded; maintainers should verify it before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #92771
Summary: The canonical issue is the private state/session directory permission report; this PR is an open candidate fix, while the older private-session-dir PR overlaps but is dirty and the path-encoding issue/PR are adjacent session-state work.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Guard chmod repair to recognized managed state/session dirs in SessionManager.open and Gateway transcript creation.
  • [P2] Add focused tests that prove custom session.store/imported transcript parents keep their original modes.

Risk before merge

  • [P1] The current PR can still chmod caller-owned custom session.store or imported-session parent directories to 0700 despite promising custom paths are unchanged.
  • [P1] Upgrade-time chmod repair intentionally tightens existing managed-state directory modes, which may break shared or ACL-dependent setups unless maintainers accept that security boundary change.
  • [P1] The PR does not add focused regression tests for the exact SessionManager.open runner path or Gateway custom-store transcript path where source inspection shows the boundary leak.

Maintainer options:

  1. Guard chmod to managed directories (recommended)
    Restrict permission repair to recognized OpenClaw-owned state/session/plugin-skill directories and add tests proving custom store, imported session, and Gateway transcript parents keep their existing modes.
  2. Accept managed-dir upgrade tightening
    Maintainers may accept 0700 repair for OpenClaw-owned managed dirs after the caller-owned path leaks are fixed and the PR body captures the user-visible upgrade behavior.
  3. Pause for a shared helper
    If scattered mkdir/chmod blocks remain error-prone, replace them with a small helper that encodes the managed-directory check once before continuing this PR.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Restrict permission repair to recognized OpenClaw-managed state/session/plugin-skill directories only. Do not chmod parents derived from caller-supplied session files or custom session.store paths in SessionManager.open or Gateway transcript creation. Add focused tests for managed upgrade repair and custom session parent preservation across session-manager/runner and Gateway transcript paths. Do not edit CHANGELOG.md.

Next step before merge

  • [P2] The remaining blockers are narrow mechanical repairs plus tests, though final merge still needs maintainer acceptance of the managed-dir upgrade permission tightening.

Security
Needs attention: The PR improves private defaults but still crosses the caller-owned path boundary in session and Gateway transcript runtime paths.

Review findings

  • [P1] Preserve custom parents in SessionManager.open — src/agents/sessions/session-manager.ts:2928
  • [P1] Do not chmod custom Gateway transcript dirs — src/gateway/server-methods/chat.ts:1732-1734
Review details

Best 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:

  • [P1] Preserve custom parents in SessionManager.open — src/agents/sessions/session-manager.ts:2928
    SessionManager.open() can be called with only a session file, including files resolved from configured session.store locations. With no sessionDir, this new chmod acts on resolve(path, ".."), so a caller-owned custom store or imported-session parent can be changed to 0700 despite the PR promising custom paths are unchanged.
    Confidence: 0.91
  • [P1] Do not chmod custom Gateway transcript dirs — src/gateway/server-methods/chat.ts:1732-1734
    Gateway transcript paths derive sessionsDir from dirname(storePath) for any configured session.store. When a missing transcript is created, this new mkdir/chmod forces that custom store parent to 0700, so Gateway can still alter caller-owned storage outside the managed state tree.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 56d95b18f4bc.

Label changes

Label justifications:

  • P2: This is bounded local filesystem security hardening with real multi-user impact but limited blast radius.
  • merge-risk: 🚨 compatibility: The diff tightens existing directory modes on upgrade and currently can affect caller-owned custom session parents.
  • merge-risk: 🚨 session-state: The diff changes creation and repair behavior for persistent session and transcript directories.
  • merge-risk: 🚨 security-boundary: The PR modifies filesystem permission boundaries around sensitive OpenClaw state, transcripts, settings, tools, and plugin skills.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • status: 🛠️ actively grinding: The PR author has acted after the latest ClawSweeper review and work remains. Sufficient (terminal): The PR body includes an inspected terminal screenshot/text proof for fresh managed dirs, simulated upgrade repair, caller-owned preservation cases, and settings scope under umask 0002; source findings still require code changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes an inspected terminal screenshot/text proof for fresh managed dirs, simulated upgrade repair, caller-owned preservation cases, and settings scope under umask 0002; source findings still require code changes.
Evidence reviewed

PR surface:

Source +64. Total +64 across 8 files.

View PR surface stats
Area Files Added Removed Net
Source 8 83 19 +64
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 8 83 19 +64

Security concerns:

  • [medium] SessionManager.open can chmod caller-owned parents — src/agents/sessions/session-manager.ts:2928
    Opening a session file without an explicit sessionDir now chmods the file's parent, which can be a custom session.store or imported-session directory rather than OpenClaw-managed state.
    Confidence: 0.91
  • [medium] Gateway transcript creation can chmod custom store parents — src/gateway/server-methods/chat.ts:1734
    When Gateway creates a missing transcript for a custom session.store, the transcript directory is derived from the custom store parent and is chmodded to 0700.
    Confidence: 0.88

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/agents/sessions/session-manager.test.ts src/config/sessions/sessions.test.ts src/config/sessions/paths.test.ts src/skills/loading/plugin-skills.test.ts src/agents/utils/tools-manager.test.ts src/gateway/server-methods/server-methods.test.ts.
  • [P1] node scripts/crabbox-wrapper.mjs run -- env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed -- src/agents/sessions/session-manager.ts src/agents/sessions/agent-session-runtime.ts src/agents/sessions/settings-manager.ts src/agents/utils/tools-manager.ts src/config/sessions/session-accessor.ts src/config/sessions/store.ts src/gateway/server-methods/chat.ts src/skills/loading/plugin-skills.ts.

What I checked:

  • Repository policy applied: Root and scoped AGENTS.md guidance was read; session state, persistent settings, upgrade-time filesystem behavior, and Gateway/session paths are compatibility- and security-sensitive review surfaces. (AGENTS.md:14, 56d95b18f4bc)
  • Current main still has the central permission gap: Current main creates the default managed session directory with recursive mkdir and no explicit private mode, so the hardening request is still useful and not already implemented on main. (src/agents/sessions/session-manager.ts:443, 56d95b18f4bc)
  • Custom session.store paths are caller-owned: resolveStorePath returns configured store paths as arbitrary resolved paths; only the absent-store default is under the managed agents sessions tree. (src/config/sessions/paths.ts:297, 56d95b18f4bc)
  • PR head chmods arbitrary SessionManager.open parents: At PR head, SessionManager.open() derives dir from the parent of any session file when sessionDir is omitted, then chmods that directory to 0o700. (src/agents/sessions/session-manager.ts:2928, 21d708ea4c93)
  • Runtime caller reaches SessionManager.open without sessionDir: The embedded runner calls SessionManager.open(params.sessionFile) with no sessionDir, making the PR's chmod path reachable for session files resolved from custom stores. (src/agents/embedded-agent-runner/run/attempt.ts:2183, 56d95b18f4bc)
  • PR head chmods custom Gateway transcript parents: Gateway resolves sessionsDir from dirname(storePath) when a custom store is configured, then the PR creates and chmods the derived transcript directory. (src/gateway/server-methods/chat.ts:1732, 21d708ea4c93)

Likely related people:

  • Shakker: Current local blame for the central session, Gateway, settings, and plugin-skill lines maps to the same recent commit in this checkout. (role: recent line-history owner; confidence: medium; commits: a49816ffbb52; files: src/agents/sessions/session-manager.ts, src/gateway/server-methods/chat.ts, src/config/sessions/paths.ts)
  • jalehman: GitHub path history shows recent transcript writer and lifecycle refactors through the same session-manager, session path, and Gateway transcript surfaces. (role: recent session lifecycle contributor; confidence: high; commits: 00a75db4280b, d216f7c876dd, 8a7b3c755a42; files: src/agents/sessions/session-manager.ts, src/config/sessions/paths.ts, src/gateway/server-methods/chat.ts)
  • vincentkoc: Recent path history includes session serialization, session path, and embedded runner changes adjacent to the custom-store and managed-state boundaries reviewed here. (role: adjacent session-state contributor; confidence: medium; commits: 568f2d563144, 7f1d82ab2518, 7f6a93eb8efd; files: src/agents/sessions/session-manager.ts, src/config/sessions/paths.ts, src/agents/embedded-agent-runner/run/attempt.ts)
  • stevenepalmer: Recent history touched the plugin skill symlink publisher that this PR also hardens. (role: recent plugin-skills contributor; confidence: medium; commits: 39328ed69285; files: src/skills/loading/plugin-skills.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 24, 2026
@fsdwen
fsdwen force-pushed the fix/92771-private-dir-mode branch from 18e079f to ef1adec Compare June 24, 2026 09:04
@fsdwen

fsdwen commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@fsdwen

fsdwen commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

… 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]>
@fsdwen
fsdwen force-pushed the fix/92771-private-dir-mode branch from bc055aa to daa4aa7 Compare June 25, 2026 01:20
@clawsweeper

clawsweeper Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 25, 2026
@fsdwen

fsdwen commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@fsdwen

fsdwen commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@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 mode: 0o700 + chmodSync from caller-owned paths (project settings, export targets, custom session.store), keeps them on managed state dirs (getDefaultSessionDir, SessionManager).

CI: ✅ prod-types, test-types, lint, guards all pass. (1 infra failure on compact-small-whole-3 is fork checkout flake)

Proof: Terminal evidence and screenshots in PR body.

@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 16, 2026
@fsdwen

fsdwen commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

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.

@fsdwen fsdwen closed this Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling app: web-ui App: web-ui gateway Gateway runtime merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openclaw agent startup resets ~/.openclaw to 755 via default-umask mkdirSync

1 participant