mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-30 13:40:20 +00:00
fix(config): harden backup file permissions and clean orphan .bak files
Addresses #31699 — config .bak files persist with sensitive data. Changes: - Explicitly chmod 0o600 on all .bak files after creation, instead of relying on copyFile to preserve source permissions (not guaranteed on all platforms, e.g. Windows, NFS mounts). - Clean up orphan .bak files that fall outside the managed 5-deep rotation ring (e.g. PID-stamped leftovers from interrupted writes, manual backups like .bak.before-marketing). - Add tests for permission hardening and orphan cleanup. The backup ring itself is preserved — it's a valuable recovery mechanism. This PR hardens the security surface by ensuring backup files are always owner-only and stale copies don't accumulate indefinitely.
This commit is contained in:
committed by
Peter Steinberger
parent
d80144f572
commit
3c0ec76e8e
@@ -1,11 +1,17 @@
|
||||
import path from "node:path";
|
||||
|
||||
export const CONFIG_BACKUP_COUNT = 5;
|
||||
|
||||
export interface BackupRotationFs {
|
||||
unlink: (path: string) => Promise<void>;
|
||||
rename: (from: string, to: string) => Promise<void>;
|
||||
chmod?: (path: string, mode: number) => Promise<void>;
|
||||
readdir?: (path: string) => Promise<string[]>;
|
||||
}
|
||||
|
||||
export async function rotateConfigBackups(
|
||||
configPath: string,
|
||||
ioFs: {
|
||||
unlink: (path: string) => Promise<void>;
|
||||
rename: (from: string, to: string) => Promise<void>;
|
||||
},
|
||||
ioFs: BackupRotationFs,
|
||||
): Promise<void> {
|
||||
if (CONFIG_BACKUP_COUNT <= 1) {
|
||||
return;
|
||||
@@ -24,3 +30,76 @@ export async function rotateConfigBackups(
|
||||
// best-effort
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Harden file permissions on all .bak files in the rotation ring.
|
||||
* copyFile does not guarantee permission preservation on all platforms
|
||||
* (e.g. Windows, some NFS mounts), so we explicitly chmod each backup
|
||||
* to owner-only (0o600) to match the main config file.
|
||||
*/
|
||||
export async function hardenBackupPermissions(
|
||||
configPath: string,
|
||||
ioFs: BackupRotationFs,
|
||||
): Promise<void> {
|
||||
if (!ioFs.chmod) {
|
||||
return;
|
||||
}
|
||||
const backupBase = `${configPath}.bak`;
|
||||
// Harden the primary .bak
|
||||
await ioFs.chmod(backupBase, 0o600).catch(() => {
|
||||
// best-effort
|
||||
});
|
||||
// Harden numbered backups
|
||||
for (let i = 1; i < CONFIG_BACKUP_COUNT; i++) {
|
||||
await ioFs.chmod(`${backupBase}.${i}`, 0o600).catch(() => {
|
||||
// best-effort
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Remove orphan .bak files that fall outside the managed rotation ring.
|
||||
* These can accumulate from interrupted writes, manual copies, or PID-stamped
|
||||
* backups (e.g. openclaw.json.bak.1772352289, openclaw.json.bak.before-marketing).
|
||||
*
|
||||
* Only files matching `<configBasename>.bak.*` are considered; the primary
|
||||
* `.bak` and numbered `.bak.1` through `.bak.{N-1}` are preserved.
|
||||
*/
|
||||
export async function cleanOrphanBackups(
|
||||
configPath: string,
|
||||
ioFs: BackupRotationFs,
|
||||
): Promise<void> {
|
||||
if (!ioFs.readdir) {
|
||||
return;
|
||||
}
|
||||
const dir = path.dirname(configPath);
|
||||
const base = path.basename(configPath);
|
||||
const bakPrefix = `${base}.bak.`;
|
||||
|
||||
// Build the set of valid numbered suffixes: "1", "2", ..., "{N-1}"
|
||||
const validSuffixes = new Set<string>();
|
||||
for (let i = 1; i < CONFIG_BACKUP_COUNT; i++) {
|
||||
validSuffixes.add(String(i));
|
||||
}
|
||||
|
||||
let entries: string[];
|
||||
try {
|
||||
entries = await ioFs.readdir(dir);
|
||||
} catch {
|
||||
return; // best-effort
|
||||
}
|
||||
|
||||
for (const entry of entries) {
|
||||
if (!entry.startsWith(bakPrefix)) {
|
||||
continue;
|
||||
}
|
||||
const suffix = entry.slice(bakPrefix.length);
|
||||
if (validSuffixes.has(suffix)) {
|
||||
continue;
|
||||
}
|
||||
// This is an orphan — remove it
|
||||
await ioFs.unlink(path.join(dir, entry)).catch(() => {
|
||||
// best-effort
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,7 +1,11 @@
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { rotateConfigBackups } from "./backup-rotation.js";
|
||||
import {
|
||||
rotateConfigBackups,
|
||||
hardenBackupPermissions,
|
||||
cleanOrphanBackups,
|
||||
} from "./backup-rotation.js";
|
||||
import { withTempHome } from "./test-helpers.js";
|
||||
import type { OpenClawConfig } from "./types.js";
|
||||
|
||||
@@ -49,4 +53,63 @@ describe("config backup rotation", () => {
|
||||
await expect(fs.stat(`${configPath}.bak.5`)).rejects.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
it("hardenBackupPermissions sets 0o600 on all backup files", async () => {
|
||||
await withTempHome(async () => {
|
||||
const stateDir = process.env.OPENCLAW_STATE_DIR?.trim();
|
||||
if (!stateDir) {
|
||||
throw new Error("Expected OPENCLAW_STATE_DIR to be set by withTempHome");
|
||||
}
|
||||
const configPath = path.join(stateDir, "openclaw.json");
|
||||
|
||||
// Create .bak and .bak.1 with permissive mode
|
||||
await fs.writeFile(`${configPath}.bak`, "secret", { mode: 0o644 });
|
||||
await fs.writeFile(`${configPath}.bak.1`, "secret", { mode: 0o644 });
|
||||
|
||||
await hardenBackupPermissions(configPath, fs);
|
||||
|
||||
const bakStat = await fs.stat(`${configPath}.bak`);
|
||||
const bak1Stat = await fs.stat(`${configPath}.bak.1`);
|
||||
|
||||
// Owner-only permissions (0o600)
|
||||
expect(bakStat.mode & 0o777).toBe(0o600);
|
||||
expect(bak1Stat.mode & 0o777).toBe(0o600);
|
||||
});
|
||||
});
|
||||
|
||||
it("cleanOrphanBackups removes stale files outside the rotation ring", async () => {
|
||||
await withTempHome(async () => {
|
||||
const stateDir = process.env.OPENCLAW_STATE_DIR?.trim();
|
||||
if (!stateDir) {
|
||||
throw new Error("Expected OPENCLAW_STATE_DIR to be set by withTempHome");
|
||||
}
|
||||
const configPath = path.join(stateDir, "openclaw.json");
|
||||
|
||||
// Create valid backups
|
||||
await fs.writeFile(configPath, "current");
|
||||
await fs.writeFile(`${configPath}.bak`, "backup-0");
|
||||
await fs.writeFile(`${configPath}.bak.1`, "backup-1");
|
||||
await fs.writeFile(`${configPath}.bak.2`, "backup-2");
|
||||
|
||||
// Create orphans
|
||||
await fs.writeFile(`${configPath}.bak.1772352289`, "orphan-pid");
|
||||
await fs.writeFile(`${configPath}.bak.before-marketing`, "orphan-manual");
|
||||
await fs.writeFile(`${configPath}.bak.99`, "orphan-overflow");
|
||||
|
||||
await cleanOrphanBackups(configPath, fs);
|
||||
|
||||
// Valid backups preserved
|
||||
await expect(fs.stat(`${configPath}.bak`)).resolves.toBeDefined();
|
||||
await expect(fs.stat(`${configPath}.bak.1`)).resolves.toBeDefined();
|
||||
await expect(fs.stat(`${configPath}.bak.2`)).resolves.toBeDefined();
|
||||
|
||||
// Orphans removed
|
||||
await expect(fs.stat(`${configPath}.bak.1772352289`)).rejects.toThrow();
|
||||
await expect(fs.stat(`${configPath}.bak.before-marketing`)).rejects.toThrow();
|
||||
await expect(fs.stat(`${configPath}.bak.99`)).rejects.toThrow();
|
||||
|
||||
// Main config untouched
|
||||
await expect(fs.readFile(configPath, "utf-8")).resolves.toBe("current");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -15,7 +15,11 @@ import {
|
||||
} from "../infra/shell-env.js";
|
||||
import { VERSION } from "../version.js";
|
||||
import { DuplicateAgentDirError, findDuplicateAgentDirs } from "./agent-dirs.js";
|
||||
import { rotateConfigBackups } from "./backup-rotation.js";
|
||||
import {
|
||||
rotateConfigBackups,
|
||||
hardenBackupPermissions,
|
||||
cleanOrphanBackups,
|
||||
} from "./backup-rotation.js";
|
||||
import {
|
||||
applyCompactionDefaults,
|
||||
applyContextPruningDefaults,
|
||||
@@ -1245,6 +1249,8 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) {
|
||||
await deps.fs.promises.copyFile(configPath, `${configPath}.bak`).catch(() => {
|
||||
// best-effort
|
||||
});
|
||||
await hardenBackupPermissions(configPath, deps.fs.promises);
|
||||
await cleanOrphanBackups(configPath, deps.fs.promises);
|
||||
}
|
||||
|
||||
try {
|
||||
|
||||
Reference in New Issue
Block a user