From 60ec27bce0feaa262c6d1adbf655bba6eb976c66 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 7 Apr 2026 12:58:49 +0800 Subject: [PATCH] Security: split permission target collection from apply --- src/security/fix.test.ts | 38 +++++++++++++--------- src/security/fix.ts | 70 ++++++++++++++++++++++------------------ 2 files changed, 61 insertions(+), 47 deletions(-) diff --git a/src/security/fix.test.ts b/src/security/fix.test.ts index 563710dfd3e..eb33512c097 100644 --- a/src/security/fix.test.ts +++ b/src/security/fix.test.ts @@ -4,7 +4,11 @@ import path from "node:path"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import type { ChannelPlugin } from "../channels/plugins/types.js"; import type { OpenClawConfig } from "../config/config.js"; -import { applySecurityFixConfigMutations, fixSecurityFootguns } from "./fix.js"; +import { + applySecurityFixConfigMutations, + collectSecurityPermissionTargets, + fixSecurityFootguns, +} from "./fix.js"; const isWindows = process.platform === "win32"; @@ -231,7 +235,7 @@ describe("security fix", () => { await expectTightenedStateAndConfigPerms(stateDir, configPath); }); - it("tightens perms for credentials + agent auth/sessions + include files", async () => { + it("collects permission targets for credentials + agent auth/sessions + include files", async () => { const stateDir = await createStateDir("includes"); const includesDir = path.join(stateDir, "includes"); @@ -273,25 +277,27 @@ describe("security fix", () => { await fs.writeFile(transcriptPath, '{"type":"session"}\n', "utf-8"); await fs.chmod(transcriptPath, 0o644); - const res = await fixSecurityFootguns({ + const targets = await collectSecurityPermissionTargets({ env: createFixEnv(stateDir, configPath), stateDir, configPath, + cfg: { + channels: { whatsapp: { groupPolicy: "open" } }, + } as OpenClawConfig, + includePaths: [includePath], }); - expect(res.ok).toBe(true); - const permissionChecks: Array = [ - [credsDir, 0o700], - [allowFromPath, 0o600], - [authProfilesPath, 0o600], - [sessionsStorePath, 0o600], - [transcriptPath, 0o600], - [includePath, 0o600], - ]; - await Promise.all( - permissionChecks.map(async ([targetPath, expectedMode]) => - expectPerms((await fs.stat(targetPath)).mode & 0o777, expectedMode), - ), + expect(targets).toEqual( + expect.arrayContaining([ + { path: stateDir, mode: 0o700, require: "dir" }, + { path: configPath, mode: 0o600, require: "file" }, + { path: credsDir, mode: 0o700, require: "dir" }, + { path: allowFromPath, mode: 0o600, require: "file" }, + { path: authProfilesPath, mode: 0o600, require: "file" }, + { path: sessionsStorePath, mode: 0o600, require: "file" }, + { path: transcriptPath, mode: 0o600, require: "file" }, + { path: includePath, mode: 0o600, require: "file" }, + ]), ); }); }); diff --git a/src/security/fix.ts b/src/security/fix.ts index 418bc824b22..44f663dd2f4 100644 --- a/src/security/fix.ts +++ b/src/security/fix.ts @@ -40,6 +40,12 @@ export type SecurityFixResult = { errors: string[]; }; +export type SecurityPermissionTarget = { + path: string; + mode: number; + require: "dir" | "file"; +}; + async function safeChmod(params: { path: string; mode: number; @@ -302,19 +308,24 @@ async function collectChannelSecurityConfigFixMutation(params: { return { cfg: nextCfg, changes }; } -async function chmodCredentialsAndAgentState(params: { +export async function collectSecurityPermissionTargets(params: { env: NodeJS.ProcessEnv; stateDir: string; + configPath: string; cfg: OpenClawConfig; - actions: SecurityFixAction[]; - applyPerms: (params: { - path: string; - mode: number; - require: "dir" | "file"; - }) => Promise; -}): Promise { + includePaths?: readonly string[]; +}): Promise { + const targets: SecurityPermissionTarget[] = [ + { path: params.stateDir, mode: 0o700, require: "dir" }, + { path: params.configPath, mode: 0o600, require: "file" }, + ...(params.includePaths ?? []).map((targetPath) => ({ + path: targetPath, + mode: 0o600, + require: "file" as const, + })), + ]; const credsDir = resolveOAuthDir(params.env, params.stateDir); - params.actions.push(await safeChmod({ path: credsDir, mode: 0o700, require: "dir" })); + targets.push({ path: credsDir, mode: 0o700, require: "dir" }); const credsEntries = await fs.readdir(credsDir, { withFileTypes: true }).catch(() => []); for (const entry of credsEntries) { @@ -325,12 +336,12 @@ async function chmodCredentialsAndAgentState(params: { continue; } const p = path.join(credsDir, entry.name); - params.actions.push(await safeChmod({ path: p, mode: 0o600, require: "file" })); + targets.push({ path: p, mode: 0o600, require: "file" }); } const ids = new Set(); ids.add(resolveDefaultAgentId(params.cfg)); - const list = Array.isArray(params.cfg.agents?.list) ? params.cfg.agents?.list : []; + const list = Array.isArray(params.cfg.agents?.list) ? params.cfg.agents.list : []; for (const agent of list ?? []) { if (!agent || typeof agent !== "object") { continue; @@ -348,18 +359,16 @@ async function chmodCredentialsAndAgentState(params: { const agentDir = path.join(agentRoot, "agent"); const sessionsDir = path.join(agentRoot, "sessions"); - params.actions.push(await safeChmod({ path: agentRoot, mode: 0o700, require: "dir" })); - params.actions.push(await params.applyPerms({ path: agentDir, mode: 0o700, require: "dir" })); + targets.push({ path: agentRoot, mode: 0o700, require: "dir" }); + targets.push({ path: agentDir, mode: 0o700, require: "dir" }); const authPath = path.join(agentDir, "auth-profiles.json"); - params.actions.push(await params.applyPerms({ path: authPath, mode: 0o600, require: "file" })); + targets.push({ path: authPath, mode: 0o600, require: "file" }); - params.actions.push( - await params.applyPerms({ path: sessionsDir, mode: 0o700, require: "dir" }), - ); + targets.push({ path: sessionsDir, mode: 0o700, require: "dir" }); const storePath = path.join(sessionsDir, "sessions.json"); - params.actions.push(await params.applyPerms({ path: storePath, mode: 0o600, require: "file" })); + targets.push({ path: storePath, mode: 0o600, require: "file" }); // Fix permissions on session transcript files (*.jsonl) const sessionEntries = await fs.readdir(sessionsDir, { withFileTypes: true }).catch(() => []); @@ -371,9 +380,10 @@ async function chmodCredentialsAndAgentState(params: { continue; } const p = path.join(sessionsDir, entry.name); - params.actions.push(await params.applyPerms({ path: p, mode: 0o600, require: "file" })); + targets.push({ path: p, mode: 0o600, require: "file" }); } } + return targets; } export async function fixSecurityFootguns(opts?: { @@ -423,29 +433,27 @@ export async function fixSecurityFootguns(opts?: { isWindows ? safeAclReset({ path: params.path, require: params.require, env, exec }) : safeChmod({ path: params.path, mode: params.mode, require: params.require }); - - actions.push(await applyPerms({ path: stateDir, mode: 0o700, require: "dir" })); - actions.push(await applyPerms({ path: configPath, mode: 0o600, require: "file" })); - + let includePaths: string[] = []; if (snap.exists) { - const includePaths = await collectIncludePathsRecursive({ + includePaths = await collectIncludePathsRecursive({ configPath: snap.path, parsed: snap.parsed, }).catch(() => []); - for (const p of includePaths) { - actions.push(await applyPerms({ path: p, mode: 0o600, require: "file" })); - } } - await chmodCredentialsAndAgentState({ + const permissionTargets = await collectSecurityPermissionTargets({ env, stateDir, + configPath, cfg: snap.config ?? {}, - actions, - applyPerms, + includePaths, }).catch((err) => { - errors.push(`chmodCredentialsAndAgentState failed: ${String(err)}`); + errors.push(`collectSecurityPermissionTargets failed: ${String(err)}`); + return [] as SecurityPermissionTarget[]; }); + for (const target of permissionTargets) { + actions.push(await applyPerms(target)); + } return { ok: errors.length === 0,