From 13db0b88f5b82ab10b80d339f0b7ca5b0ecedbe4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Feb 2026 23:12:31 +0100 Subject: [PATCH] refactor(gateway): share safe avatar file open checks --- src/gateway/control-ui.ts | 71 +++++++---------------------------- src/gateway/session-utils.ts | 38 +++++++------------ src/infra/safe-open-sync.ts | 72 ++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 82 deletions(-) create mode 100644 src/infra/safe-open-sync.ts diff --git a/src/gateway/control-ui.ts b/src/gateway/control-ui.ts index cd152720c..aa5f3b90e 100644 --- a/src/gateway/control-ui.ts +++ b/src/gateway/control-ui.ts @@ -4,6 +4,7 @@ import path from "node:path"; import type { OpenClawConfig } from "../config/config.js"; import { resolveControlUiRootSync } from "../infra/control-ui-assets.js"; import { isWithinDir } from "../infra/path-safety.js"; +import { openVerifiedFileSync } from "../infra/safe-open-sync.js"; import { AVATAR_MAX_BYTES } from "../shared/avatar-policy.js"; import { DEFAULT_ASSISTANT_IDENTITY, resolveAssistantIdentity } from "./assistant-identity.js"; import { @@ -220,84 +221,40 @@ function isExpectedSafePathError(error: unknown): boolean { return code === "ENOENT" || code === "ENOTDIR" || code === "ELOOP"; } -function areSameFileIdentity(preOpen: fs.Stats, opened: fs.Stats): boolean { - return preOpen.dev === opened.dev && preOpen.ino === opened.ino; -} - function resolveSafeAvatarFile(filePath: string): { path: string; fd: number } | null { - let fd: number | null = null; - try { - const candidateStat = fs.lstatSync(filePath); - if (candidateStat.isSymbolicLink()) { - return null; - } - const fileReal = fs.realpathSync(filePath); - const preOpenStat = fs.lstatSync(fileReal); - if (!preOpenStat.isFile() || preOpenStat.size > AVATAR_MAX_BYTES) { - return null; - } - const openFlags = - fs.constants.O_RDONLY | - (typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0); - fd = fs.openSync(fileReal, openFlags); - const openedStat = fs.fstatSync(fd); - if ( - !openedStat.isFile() || - openedStat.size > AVATAR_MAX_BYTES || - !areSameFileIdentity(preOpenStat, openedStat) - ) { - return null; - } - const safeFile = { path: fileReal, fd }; - fd = null; - return safeFile; - } catch { + const opened = openVerifiedFileSync({ + filePath, + rejectPathSymlink: true, + maxBytes: AVATAR_MAX_BYTES, + }); + if (!opened.ok) { return null; - } finally { - if (fd !== null) { - fs.closeSync(fd); - } } + return { path: opened.path, fd: opened.fd }; } function resolveSafeControlUiFile( rootReal: string, filePath: string, ): { path: string; fd: number } | null { - let fd: number | null = null; try { const fileReal = fs.realpathSync(filePath); if (!isContainedPath(rootReal, fileReal)) { return null; } - - const preOpenStat = fs.lstatSync(fileReal); - if (!preOpenStat.isFile()) { + const opened = openVerifiedFileSync({ filePath: fileReal, resolvedPath: fileReal }); + if (!opened.ok) { + if (opened.reason === "io") { + throw opened.error; + } return null; } - - const openFlags = - fs.constants.O_RDONLY | - (typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0); - fd = fs.openSync(fileReal, openFlags); - const openedStat = fs.fstatSync(fd); - // Compare inode identity so swaps between validation and open are rejected. - if (!openedStat.isFile() || !areSameFileIdentity(preOpenStat, openedStat)) { - return null; - } - - const resolved = { path: fileReal, fd }; - fd = null; - return resolved; + return { path: opened.path, fd: opened.fd }; } catch (error) { if (isExpectedSafePathError(error)) { return null; } throw error; - } finally { - if (fd !== null) { - fs.closeSync(fd); - } } } diff --git a/src/gateway/session-utils.ts b/src/gateway/session-utils.ts index 4876bb956..73dbd9c71 100644 --- a/src/gateway/session-utils.ts +++ b/src/gateway/session-utils.ts @@ -21,6 +21,7 @@ import { type SessionEntry, type SessionScope, } from "../config/sessions.js"; +import { openVerifiedFileSync } from "../infra/safe-open-sync.js"; import { normalizeAgentId, normalizeMainKey, @@ -75,10 +76,6 @@ function tryResolveExistingPath(value: string): string | null { } } -function areSameFileIdentity(preOpen: fs.Stats, opened: fs.Stats): boolean { - return preOpen.dev === opened.dev && preOpen.ino === opened.ino; -} - function resolveIdentityAvatarUrl( cfg: OpenClawConfig, agentId: string, @@ -103,37 +100,28 @@ function resolveIdentityAvatarUrl( if (!isPathWithinRoot(workspaceRoot, resolvedCandidate)) { return undefined; } - let fd: number | null = null; try { const resolvedReal = fs.realpathSync(resolvedCandidate); if (!isPathWithinRoot(workspaceRoot, resolvedReal)) { return undefined; } - const preOpenStat = fs.lstatSync(resolvedReal); - if (!preOpenStat.isFile() || preOpenStat.size > AVATAR_MAX_BYTES) { + const opened = openVerifiedFileSync({ + filePath: resolvedReal, + resolvedPath: resolvedReal, + maxBytes: AVATAR_MAX_BYTES, + }); + if (!opened.ok) { return undefined; } - const openFlags = - fs.constants.O_RDONLY | - (typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0); - fd = fs.openSync(resolvedReal, openFlags); - const openedStat = fs.fstatSync(fd); - if ( - !openedStat.isFile() || - openedStat.size > AVATAR_MAX_BYTES || - !areSameFileIdentity(preOpenStat, openedStat) - ) { - return undefined; + try { + const buffer = fs.readFileSync(opened.fd); + const mime = resolveAvatarMime(resolvedCandidate); + return `data:${mime};base64,${buffer.toString("base64")}`; + } finally { + fs.closeSync(opened.fd); } - const buffer = fs.readFileSync(fd); - const mime = resolveAvatarMime(resolvedCandidate); - return `data:${mime};base64,${buffer.toString("base64")}`; } catch { return undefined; - } finally { - if (fd !== null) { - fs.closeSync(fd); - } } } diff --git a/src/infra/safe-open-sync.ts b/src/infra/safe-open-sync.ts new file mode 100644 index 000000000..ac4638483 --- /dev/null +++ b/src/infra/safe-open-sync.ts @@ -0,0 +1,72 @@ +import fs from "node:fs"; + +export type SafeOpenSyncFailureReason = "path" | "validation" | "io"; + +export type SafeOpenSyncResult = + | { ok: true; path: string; fd: number; stat: fs.Stats } + | { ok: false; reason: SafeOpenSyncFailureReason; error?: unknown }; + +const OPEN_READ_FLAGS = + fs.constants.O_RDONLY | + (typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0); + +function isExpectedPathError(error: unknown): boolean { + const code = + typeof error === "object" && error !== null && "code" in error ? String(error.code) : ""; + return code === "ENOENT" || code === "ENOTDIR" || code === "ELOOP"; +} + +export function sameFileIdentity(left: fs.Stats, right: fs.Stats): boolean { + return left.dev === right.dev && left.ino === right.ino; +} + +export function openVerifiedFileSync(params: { + filePath: string; + resolvedPath?: string; + rejectPathSymlink?: boolean; + maxBytes?: number; +}): SafeOpenSyncResult { + let fd: number | null = null; + try { + if (params.rejectPathSymlink) { + const candidateStat = fs.lstatSync(params.filePath); + if (candidateStat.isSymbolicLink()) { + return { ok: false, reason: "validation" }; + } + } + + const realPath = params.resolvedPath ?? fs.realpathSync(params.filePath); + const preOpenStat = fs.lstatSync(realPath); + if (!preOpenStat.isFile()) { + return { ok: false, reason: "validation" }; + } + if (params.maxBytes !== undefined && preOpenStat.size > params.maxBytes) { + return { ok: false, reason: "validation" }; + } + + fd = fs.openSync(realPath, OPEN_READ_FLAGS); + const openedStat = fs.fstatSync(fd); + if (!openedStat.isFile()) { + return { ok: false, reason: "validation" }; + } + if (params.maxBytes !== undefined && openedStat.size > params.maxBytes) { + return { ok: false, reason: "validation" }; + } + if (!sameFileIdentity(preOpenStat, openedStat)) { + return { ok: false, reason: "validation" }; + } + + const opened = { ok: true as const, path: realPath, fd, stat: openedStat }; + fd = null; + return opened; + } catch (error) { + if (isExpectedPathError(error)) { + return { ok: false, reason: "path", error }; + } + return { ok: false, reason: "io", error }; + } finally { + if (fd !== null) { + fs.closeSync(fd); + } + } +}