Harden config backup restore permissions by drobison00 · Pull Request #77488 · openclaw/openclaw

drobison00 · 2026-05-04T19:23:03Z

Harden config backup restore permissions

Summary

Problem: Suspicious-read config backup restore paths copied a backup over openclaw.json without forcing the final file mode back to owner-only permissions.
Why it matters: openclaw.json can contain live tokens and API keys, so preserving a world-readable destination mode could expose credentials to other local users or processes.
What changed: Added best-effort async and sync chmod(..., 0o600) calls after backup restore copies, plus regression tests for insecure destination modes.
What did NOT change (scope boundary): Normal config writes, last-known-good recovery policy, audit record schema, CI, release, workflow, and infrastructure files were not changed.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes Auto-Reply: relax regex for reply tags to allow whitespace #560
Related #
This PR fixes a bug or regression

Root Cause (if applicable)

Root cause: The suspicious-read recovery helpers restored from .bak with copyFile / copyFileSync but did not normalize the destination mode afterward, so an existing broad destination mode could remain in place.
Missing detection / guardrail: Existing recovery tests asserted restored content and audit records, but not the final file mode after restoring over an insecure destination.
Contributing context (if known): Other config write and last-known-good restore paths already harden the destination mode.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/config/io.observe-recovery.test.ts
Scenario the test should lock in: Async and sync suspicious-read backup restores over an existing 0644 config leave openclaw.json at 0600.
Why this is the smallest reliable guardrail: The affected helpers are pure filesystem recovery helpers and can be exercised directly with the existing fixture setup.
Existing test that already covers this (if any): Existing observe-recovery tests covered restore success and copy failures, but not file permissions.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Restored openclaw.json files are best-effort hardened to owner-only permissions after suspicious-read backup recovery.

Diagram (if applicable)

N/A

Security Impact (required)

New permissions/capabilities? (Yes/No) No
Secrets/tokens handling changed? (Yes/No) Yes
New/changed network calls? (Yes/No) No
Command/tool execution surface changed? (Yes/No) No
Data access scope changed? (Yes/No) Yes
If any Yes, explain risk + mitigation: The change narrows local filesystem exposure for credential-bearing config restores by forcing owner-only permissions after recovery copy operations. The chmod is best-effort to avoid making restore unavailable on filesystems that cannot apply POSIX modes.

Repro + Verification

Environment

OS: Linux 6.8.0-110-generic x86_64
Runtime/container: Node v22.14.0, pnpm 10.33.2
Model/provider: N/A
Integration/channel (if any): N/A
Relevant config (redacted): Test fixture config with token-like placeholder values only.

Steps

Seed a recoverable config backup.
Replace the live config with a recoverable suspicious read state and chmod the live config to 0644.
Run async and sync suspicious-read backup recovery.
Assert the final live config mode is 0600.

Expected

Backup recovery restores the config content and leaves openclaw.json owner-readable/writable only.

Actual

pnpm exec vitest run --config test/vitest/vitest.runtime-config.config.ts src/config/io.observe-recovery.test.ts passed with 18 tests.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Targeted test output:

Test Files  1 passed (1)
Tests  18 passed (18)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: Async and sync suspicious-read backup restore permission hardening through the targeted Vitest file.
Edge cases checked: Restore copy failure tests still pass; chmod is best-effort and does not replace existing copy failure behavior.
What you did not verify: Full repository test suite, Windows file-mode behavior, or live installed-package recovery.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

Backward compatible? (Yes/No) Yes
Config/env changes? (Yes/No) No
Migration needed? (Yes/No) No
If yes, exact upgrade steps: N/A

Risks and Mitigations

Risk: Filesystems that do not support POSIX chmod may ignore or reject the permission update.
- Mitigation: The chmod remains best-effort, matching adjacent config hardening paths.

clawsweeper · 2026-05-04T19:26:32Z

Codex review: needs real behavior proof before merge.

Summary
The PR adds best-effort chmod(0o600) after async and sync suspicious-read config backup restores, POSIX regression tests, and an Unreleased changelog entry.

Reproducibility: yes. Source inspection on current main shows both suspicious-read backup restore paths copy .bak over openclaw.json without the adjacent chmod used by other config recovery paths.

Real behavior proof
Needs real behavior proof before merge: The PR body includes targeted Vitest output, but no after-fix real behavior proof outside tests; copied live output or redacted logs showing a restored 0644 config becoming 0600 would satisfy the gate.

Next step before merge
Protected-label handling, unrelated closing metadata, and missing contributor real behavior proof require maintainer/contributor follow-up rather than a repair-lane code change.

Security
Cleared: The diff narrows local credential-file exposure and does not add dependency, workflow, network, or command-execution surface.

Review details

Best possible solution:

Land the narrow chmod hardening after maintainer review, corrected closing metadata, after-fix real behavior proof, and current CI/changed-gate validation.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection on current main shows both suspicious-read backup restore paths copy .bak over openclaw.json without the adjacent chmod used by other config recovery paths.

Is this the best way to solve the issue?

Yes. Best-effort chmod immediately after successful restore copies is the narrowest maintainable fix and matches existing config write and last-known-good recovery hardening patterns.

Acceptance criteria:

pnpm test src/config/io.observe-recovery.test.ts
Testbox: pnpm check:changed

What I checked:

Protected label: The provided PR context and timeline show the open PR has the maintainer label, so this cleanup workflow must not close it. (39b977fd3fb1)
Current async restore gap: Current main copies the backup over openclaw.json in the async suspicious-read restore path and then marks the restore successful without normalizing file mode. (src/config/io.observe-recovery.ts:642, a3870686947b)
Current sync restore gap: Current main has the same copy-without-chmod pattern in the sync suspicious-read restore path. (src/config/io.observe-recovery.ts:749, a3870686947b)
Adjacent hardening pattern: The adjacent last-known-good recovery path already performs a best-effort chmod to 0600 after copying the recovery file into place. (src/config/io.observe-recovery.ts:900, a3870686947b)
Security permission contract: The security CLI docs describe tightening state/config permissions with POSIX chmod as expected security-audit behavior. Public docs: docs/cli/security.md. (docs/cli/security.md:77, e6f5f5693d22)
Latest PR patch shape: The provided latest PR diff adds chmod after both suspicious-read restore copies, adds async and sync POSIX permission regression tests, and includes a changelog entry. (src/config/io.observe-recovery.ts:643, 39b977fd3fb1)

Likely related people:

steipete: Peter introduced the critical config clobber recovery path and earlier invalid gateway config recovery work that this PR extends. (role: introduced behavior and config-recovery owner; confidence: high; commits: 7b2c9a6fa3d3, ffb1628727fd, 83801c49f7a3; files: src/config/io.observe-recovery.ts, src/config/io.observe-recovery.test.ts, src/config/io.ts)
vincentkoc: Vincent recently maintained the same observe-recovery and clobber snapshot area, including bounded clobber recovery snapshots. (role: recent maintainer; confidence: high; commits: 3c5169254362; files: src/config/io.observe-recovery.ts, src/config/io.observe-recovery.test.ts, src/config/io.clobber-snapshot.ts)
davidangularme: Recent merged work added audit and log handling for backup restore copy failures in the same async and sync recovery paths. (role: adjacent maintainer; confidence: medium; commits: f8ffc3ec4f97; files: src/config/io.observe-recovery.ts, src/config/io.observe-recovery.test.ts, src/config/io.audit.ts)
YUJIE2002: Earlier config backup permission hardening introduced the owner-only backup permission pattern that this PR extends to restored live configs. (role: adjacent permissions owner; confidence: medium; commits: 3c0ec76e8ede; files: src/config/backup-rotation.ts, src/config/config.backup-rotation.test.ts, src/config/io.ts)

Remaining risk / open question:

The PR body still links Closes #560, which points to an unrelated already-merged Auto-Reply PR and should be removed or corrected before merge.
The PR body includes only targeted test output; external contributor real behavior proof is still needed before merge under the review gate.
This read-only review did not run the targeted test or changed gate; CI/Testbox should prove the latest head before landing.

Codex review notes: model gpt-5.5, reasoning high; reviewed against a3870686947b.

Adds the user-facing Unreleased Fixes entry for the suspicious-read backup restore chmod hardening shipped in this PR.

* Harden config backup restore permissions * docs(changelog): credit config restore mode hardening Adds the user-facing Unreleased Fixes entry for the suspicious-read backup restore chmod hardening shipped in this PR.

openclaw-barnacle Bot added size: XS maintainer Maintainer-authored PR labels May 4, 2026

drobison00 force-pushed the config-restore-mode branch 3 times, most recently from bba849a to 400b330 Compare May 4, 2026 21:18

drobison00 added 2 commits May 5, 2026 08:58

Harden config backup restore permissions

a38fc48

docs(changelog): credit config restore mode hardening

39b977f

Adds the user-facing Unreleased Fixes entry for the suspicious-read backup restore chmod hardening shipped in this PR.

drobison00 force-pushed the config-restore-mode branch from 400b330 to 39b977f Compare May 5, 2026 15:02

drobison00 merged commit 91879ac into openclaw:main May 5, 2026
87 of 88 checks passed

github-actions Bot mentioned this pull request May 5, 2026

📡 Upstream Digest — 2026-05-05 15:47 UTC curtismercier/openclaw-mods#774

Open

clawsweeper Bot mentioned this pull request Jun 20, 2026

fix config-chmod-warning #95348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Harden config backup restore permissions#77488

Harden config backup restore permissions#77488
drobison00 merged 2 commits into
openclaw:mainfrom
drobison00:config-restore-mode

drobison00 commented May 4, 2026

Uh oh!

clawsweeper Bot commented May 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

drobison00 commented May 4, 2026

Harden config backup restore permissions

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

Uh oh!

clawsweeper Bot commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented May 4, 2026 •

edited

Loading