From 1bc9bada656241eba928081e468ff14552dd91d7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 26 Apr 2026 02:51:13 +0100 Subject: [PATCH] test: speed up security audit tests --- src/security/audit-extra.async.ts | 206 ------------------ src/security/audit-plugins-trust.test.ts | 5 +- src/security/audit-plugins-trust.ts | 2 +- .../audit-workspace-skill-escape.test.ts | 2 +- src/security/audit-workspace-skills.ts | 205 +++++++++++++++++ src/security/audit.nondeep.runtime.ts | 2 +- 6 files changed, 211 insertions(+), 211 deletions(-) create mode 100644 src/security/audit-workspace-skills.ts diff --git a/src/security/audit-extra.async.ts b/src/security/audit-extra.async.ts index f8c611ed3f7..64818306748 100644 --- a/src/security/audit-extra.async.ts +++ b/src/security/audit-extra.async.ts @@ -39,48 +39,6 @@ type ExecDockerRawFn = ( ) => Promise; type CodeSafetySummaryCache = Map>; -type WorkspaceSkillScanLimits = { - maxFiles?: number; - maxDirVisits?: number; -}; -const MAX_WORKSPACE_SKILL_SCAN_FILES_PER_WORKSPACE = 2_000; -const MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS = 12; - -/** - * Resolves the realpath of `p` with a 2 s timeout. - * - * Returns the realpath string on success, or `null` if realpath fails or the - * timeout fires first. Note: fs.realpath cannot be cancelled once submitted to - * libuv — the underlying OS call continues running in the background after the - * timeout resolves. Callers make sequential (not concurrent) calls so at most - * one libuv thread is occupied at a time; the OS will eventually time out the - * stuck NFS/SMB call independently. - * - * Timer cleanup: when realpath resolves before the deadline the timer is - * cleared immediately so it does not linger across the rest of the audit run. - * The timer is also unref'd so it cannot prevent process exit even if it fires - * late (e.g. the process finishes while a hang is still in-flight). - */ -function realpathWithTimeout(p: string, timeoutMs = 2000): Promise { - let timerHandle: ReturnType | undefined; - - const realpathPromise = fs - .realpath(p) - .catch(() => null) - .then((result) => { - clearTimeout(timerHandle); - return result; - }); - - const timeoutPromise = new Promise((resolve) => { - timerHandle = setTimeout(() => resolve(null), timeoutMs); - // Prevent the timer from keeping the process alive while waiting on a - // potentially hanging NFS/SMB path during a large audit run. - timerHandle.unref?.(); - }); - - return Promise.race([realpathPromise, timeoutPromise]); -} let skillsModulePromise: Promise | undefined; let configModulePromise: Promise | undefined; let agentScopeModulePromise: Promise | undefined; @@ -302,66 +260,6 @@ async function getCodeSafetySummary(params: { }); } -async function listWorkspaceSkillMarkdownFiles( - workspaceDir: string, - limits: WorkspaceSkillScanLimits = {}, -): Promise<{ skillFilePaths: string[]; truncated: boolean }> { - const skillsRoot = path.join(workspaceDir, "skills"); - const rootStat = await safeStat(skillsRoot); - if (!rootStat.ok || !rootStat.isDir) { - return { skillFilePaths: [], truncated: false }; - } - - const maxFiles = limits.maxFiles ?? MAX_WORKSPACE_SKILL_SCAN_FILES_PER_WORKSPACE; - const maxTotalDirVisits = limits.maxDirVisits ?? maxFiles * 20; - const skillFiles: string[] = []; - const queue: string[] = [skillsRoot]; - const visitedDirs = new Set(); - let totalDirVisits = 0; - - while (queue.length > 0 && skillFiles.length < maxFiles && totalDirVisits++ < maxTotalDirVisits) { - const dir = queue.shift()!; - // Use the module-level realpathWithTimeout so a hanging network FS doesn't - // block the BFS indefinitely (same 2 s guard as the outer escape-detection loop). - const dirRealPath = (await realpathWithTimeout(dir)) ?? path.resolve(dir); - if (visitedDirs.has(dirRealPath)) { - continue; - } - visitedDirs.add(dirRealPath); - - const entries = await fs.readdir(dir, { withFileTypes: true }).catch(() => []); - for (const entry of entries) { - if (entry.name.startsWith(".") || entry.name === "node_modules") { - continue; - } - const fullPath = path.join(dir, entry.name); - if (entry.isDirectory()) { - queue.push(fullPath); - continue; - } - if (entry.isSymbolicLink()) { - const stat = await fs.stat(fullPath).catch(() => null); - if (!stat) { - continue; - } - if (stat.isDirectory()) { - queue.push(fullPath); - continue; - } - if (stat.isFile() && entry.name === "SKILL.md") { - skillFiles.push(fullPath); - } - continue; - } - if (entry.isFile() && entry.name === "SKILL.md") { - skillFiles.push(fullPath); - } - } - } - - return { skillFilePaths: skillFiles, truncated: queue.length > 0 }; -} - // -------------------------------------------------------------------------- // Exported collectors // -------------------------------------------------------------------------- @@ -552,110 +450,6 @@ export async function collectSandboxBrowserHashLabelFindings(params?: { return findings; } -export async function collectWorkspaceSkillSymlinkEscapeFindings(params: { - cfg: OpenClawConfig; - skillScanLimits?: WorkspaceSkillScanLimits; -}): Promise { - const findings: SecurityAuditFinding[] = []; - const { listAgentWorkspaceDirs } = await loadAgentWorkspaceDirsModule(); - const workspaceDirs = listAgentWorkspaceDirs(params.cfg); - if (workspaceDirs.length === 0) { - return findings; - } - - const escapedSkillFiles: Array<{ - workspaceDir: string; - skillFilePath: string; - skillRealPath: string; - }> = []; - const seenSkillPaths = new Set(); - - for (const workspaceDir of workspaceDirs) { - const workspacePath = path.resolve(workspaceDir); - const workspaceRealPath = (await realpathWithTimeout(workspacePath)) ?? workspacePath; - const { skillFilePaths, truncated } = await listWorkspaceSkillMarkdownFiles( - workspacePath, - params.skillScanLimits, - ); - - if (truncated) { - // The BFS visit cap was hit before the full skills/ tree was scanned. - // Escaped SKILL.md symlinks in the unvisited portion will not be detected. - // Surface this as a warning so the user knows coverage was incomplete. - findings.push({ - checkId: "skills.workspace.scan_truncated", - severity: "warn", - title: "Workspace skill scan reached the directory visit limit", - detail: - `The skills/ directory scan in ${workspacePath} stopped early after reaching the ` + - `BFS visit cap. Skill files in the unscanned portion of the tree were not checked ` + - "for symlink escapes.", - remediation: - "Flatten or simplify the skills/ directory hierarchy to stay within the scan budget, " + - "or move deeply-nested skill collections to a managed skill location.", - }); - } - - for (const skillFilePath of skillFilePaths) { - const canonicalSkillPath = path.resolve(skillFilePath); - if (seenSkillPaths.has(canonicalSkillPath)) { - continue; - } - seenSkillPaths.add(canonicalSkillPath); - - const skillRealPath = await realpathWithTimeout(canonicalSkillPath); - if (!skillRealPath) { - // realpath timed out or failed — cannot verify the symlink target. - // Treat as a potential escape rather than silently bypassing the check. - // An attacker on a slow/network FS could otherwise hang realpath to - // prevent escape detection. - escapedSkillFiles.push({ - workspaceDir: workspacePath, - skillFilePath: canonicalSkillPath, - skillRealPath: "(realpath timed out \u2014 symlink target unverifiable)", - }); - continue; - } - if (isPathInside(workspaceRealPath, skillRealPath)) { - continue; - } - escapedSkillFiles.push({ - workspaceDir: workspacePath, - skillFilePath: canonicalSkillPath, - skillRealPath, - }); - } - } - - if (escapedSkillFiles.length === 0) { - return findings; - } - - findings.push({ - checkId: "skills.workspace.symlink_escape", - severity: "warn", - title: "Workspace skill files resolve outside the workspace root", - detail: - "Detected workspace `skills/**/SKILL.md` paths whose realpath escapes their workspace root:\n" + - escapedSkillFiles - .slice(0, MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS) - .map( - (entry) => - `- workspace=${entry.workspaceDir}\n` + - ` skill=${entry.skillFilePath}\n` + - ` realpath=${entry.skillRealPath}`, - ) - .join("\n") + - (escapedSkillFiles.length > MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS - ? `\n- +${escapedSkillFiles.length - MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS} more` - : ""), - remediation: - "Keep workspace skills inside the workspace root (replace symlinked escapes with real in-workspace files), or move trusted shared skills to managed/bundled skill locations.", - }); - - return findings; -} - export async function collectIncludeFilePermFindings(params: { configSnapshot: ConfigFileSnapshot; env?: NodeJS.ProcessEnv; diff --git a/src/security/audit-plugins-trust.test.ts b/src/security/audit-plugins-trust.test.ts index 332f159606c..fda78e2c9ae 100644 --- a/src/security/audit-plugins-trust.test.ts +++ b/src/security/audit-plugins-trust.test.ts @@ -4,7 +4,6 @@ import path from "node:path"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import type { PluginInstallRecord } from "../config/types.plugins.js"; -import { writePersistedInstalledPluginIndex } from "../plugins/installed-plugin-index-store.js"; import type { InstalledPluginIndex } from "../plugins/installed-plugin-index.js"; import { createPathResolutionEnv, withEnvAsync } from "../test-utils/env.js"; import { collectPluginsTrustFindings } from "./audit-plugins-trust.js"; @@ -132,7 +131,9 @@ describe("security audit install metadata findings", () => { })), diagnostics: [], }; - await writePersistedInstalledPluginIndex(index, { stateDir }); + const filePath = path.join(stateDir, "plugins", "installs.json"); + await fs.mkdir(path.dirname(filePath), { recursive: true, mode: 0o700 }); + await fs.writeFile(filePath, `${JSON.stringify(index, null, 2)}\n`, { mode: 0o600 }); }; beforeAll(async () => { diff --git a/src/security/audit-plugins-trust.ts b/src/security/audit-plugins-trust.ts index 085b38347d2..8e3de3a990a 100644 --- a/src/security/audit-plugins-trust.ts +++ b/src/security/audit-plugins-trust.ts @@ -7,7 +7,7 @@ import type { OpenClawConfig } from "../config/config.js"; import type { AgentToolsConfig } from "../config/types.tools.js"; import { readInstalledPackageVersion } from "../infra/package-update-utils.js"; import { normalizePluginId, normalizePluginsConfig } from "../plugins/config-state.js"; -import { loadInstalledPluginIndexInstallRecords } from "../plugins/installed-plugin-index-records.js"; +import { loadInstalledPluginIndexInstallRecords } from "../plugins/installed-plugin-index-record-reader.js"; import { normalizeOptionalLowercaseString } from "../shared/string-coerce.js"; import type { SecurityAuditFinding } from "./audit.types.js"; diff --git a/src/security/audit-workspace-skill-escape.test.ts b/src/security/audit-workspace-skill-escape.test.ts index b47130231de..94cfa1ddf6a 100644 --- a/src/security/audit-workspace-skill-escape.test.ts +++ b/src/security/audit-workspace-skill-escape.test.ts @@ -2,7 +2,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; -import { collectWorkspaceSkillSymlinkEscapeFindings } from "./audit-extra.async.js"; +import { collectWorkspaceSkillSymlinkEscapeFindings } from "./audit-workspace-skills.js"; import { AsyncTempCaseFactory } from "./test-temp-cases.js"; const isWindows = process.platform === "win32"; diff --git a/src/security/audit-workspace-skills.ts b/src/security/audit-workspace-skills.ts new file mode 100644 index 00000000000..9e785d7e42f --- /dev/null +++ b/src/security/audit-workspace-skills.ts @@ -0,0 +1,205 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { listAgentWorkspaceDirs } from "../agents/workspace-dirs.js"; +import type { OpenClawConfig } from "../config/config.js"; +import type { SecurityAuditFinding } from "./audit.types.js"; +import { isPathInside } from "./scan-paths.js"; + +type WorkspaceSkillScanLimits = { + maxFiles?: number; + maxDirVisits?: number; +}; + +const MAX_WORKSPACE_SKILL_SCAN_FILES_PER_WORKSPACE = 2_000; +const MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS = 12; + +async function safeStat(targetPath: string): Promise<{ + ok: boolean; + isDir: boolean; +}> { + try { + const lst = await fs.lstat(targetPath); + return { + ok: true, + isDir: lst.isDirectory(), + }; + } catch { + return { + ok: false, + isDir: false, + }; + } +} + +function realpathWithTimeout(p: string, timeoutMs = 2000): Promise { + let timerHandle: ReturnType | undefined; + + const realpathPromise = fs + .realpath(p) + .catch(() => null) + .then((result) => { + clearTimeout(timerHandle); + return result; + }); + + const timeoutPromise = new Promise((resolve) => { + timerHandle = setTimeout(() => resolve(null), timeoutMs); + timerHandle.unref?.(); + }); + + return Promise.race([realpathPromise, timeoutPromise]); +} + +async function listWorkspaceSkillMarkdownFiles( + workspaceDir: string, + limits: WorkspaceSkillScanLimits = {}, +): Promise<{ skillFilePaths: string[]; truncated: boolean }> { + const skillsRoot = path.join(workspaceDir, "skills"); + const rootStat = await safeStat(skillsRoot); + if (!rootStat.ok || !rootStat.isDir) { + return { skillFilePaths: [], truncated: false }; + } + + const maxFiles = limits.maxFiles ?? MAX_WORKSPACE_SKILL_SCAN_FILES_PER_WORKSPACE; + const maxTotalDirVisits = limits.maxDirVisits ?? maxFiles * 20; + const skillFiles: string[] = []; + const queue: string[] = [skillsRoot]; + const visitedDirs = new Set(); + let totalDirVisits = 0; + + while (queue.length > 0 && skillFiles.length < maxFiles && totalDirVisits++ < maxTotalDirVisits) { + const dir = queue.shift()!; + const dirRealPath = (await realpathWithTimeout(dir)) ?? path.resolve(dir); + if (visitedDirs.has(dirRealPath)) { + continue; + } + visitedDirs.add(dirRealPath); + + const entries = await fs.readdir(dir, { withFileTypes: true }).catch(() => []); + for (const entry of entries) { + if (entry.name.startsWith(".") || entry.name === "node_modules") { + continue; + } + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + queue.push(fullPath); + continue; + } + if (entry.isSymbolicLink()) { + const stat = await fs.stat(fullPath).catch(() => null); + if (!stat) { + continue; + } + if (stat.isDirectory()) { + queue.push(fullPath); + continue; + } + if (stat.isFile() && entry.name === "SKILL.md") { + skillFiles.push(fullPath); + } + continue; + } + if (entry.isFile() && entry.name === "SKILL.md") { + skillFiles.push(fullPath); + } + } + } + + return { skillFilePaths: skillFiles, truncated: queue.length > 0 }; +} + +export async function collectWorkspaceSkillSymlinkEscapeFindings(params: { + cfg: OpenClawConfig; + skillScanLimits?: WorkspaceSkillScanLimits; +}): Promise { + const findings: SecurityAuditFinding[] = []; + const workspaceDirs = listAgentWorkspaceDirs(params.cfg); + if (workspaceDirs.length === 0) { + return findings; + } + + const escapedSkillFiles: Array<{ + workspaceDir: string; + skillFilePath: string; + skillRealPath: string; + }> = []; + const seenSkillPaths = new Set(); + + for (const workspaceDir of workspaceDirs) { + const workspacePath = path.resolve(workspaceDir); + const workspaceRealPath = (await realpathWithTimeout(workspacePath)) ?? workspacePath; + const { skillFilePaths, truncated } = await listWorkspaceSkillMarkdownFiles( + workspacePath, + params.skillScanLimits, + ); + + if (truncated) { + findings.push({ + checkId: "skills.workspace.scan_truncated", + severity: "warn", + title: "Workspace skill scan reached the directory visit limit", + detail: + `The skills/ directory scan in ${workspacePath} stopped early after reaching the ` + + `BFS visit cap. Skill files in the unscanned portion of the tree were not checked ` + + "for symlink escapes.", + remediation: + "Flatten or simplify the skills/ directory hierarchy to stay within the scan budget, " + + "or move deeply-nested skill collections to a managed skill location.", + }); + } + + for (const skillFilePath of skillFilePaths) { + const canonicalSkillPath = path.resolve(skillFilePath); + if (seenSkillPaths.has(canonicalSkillPath)) { + continue; + } + seenSkillPaths.add(canonicalSkillPath); + + const skillRealPath = await realpathWithTimeout(canonicalSkillPath); + if (!skillRealPath) { + escapedSkillFiles.push({ + workspaceDir: workspacePath, + skillFilePath: canonicalSkillPath, + skillRealPath: "(realpath timed out - symlink target unverifiable)", + }); + continue; + } + if (isPathInside(workspaceRealPath, skillRealPath)) { + continue; + } + escapedSkillFiles.push({ + workspaceDir: workspacePath, + skillFilePath: canonicalSkillPath, + skillRealPath, + }); + } + } + + if (escapedSkillFiles.length === 0) { + return findings; + } + + findings.push({ + checkId: "skills.workspace.symlink_escape", + severity: "warn", + title: "Workspace skill files resolve outside the workspace root", + detail: + "Detected workspace `skills/**/SKILL.md` paths whose realpath escapes their workspace root:\n" + + escapedSkillFiles + .slice(0, MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS) + .map( + (entry) => + `- workspace=${entry.workspaceDir}\n` + + ` skill=${entry.skillFilePath}\n` + + ` realpath=${entry.skillRealPath}`, + ) + .join("\n") + + (escapedSkillFiles.length > MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS + ? `\n- +${escapedSkillFiles.length - MAX_WORKSPACE_SKILL_ESCAPE_DETAIL_ROWS} more` + : ""), + remediation: + "Keep workspace skills inside the workspace root (replace symlinked escapes with real in-workspace files), or move trusted shared skills to managed/bundled skill locations.", + }); + + return findings; +} diff --git a/src/security/audit.nondeep.runtime.ts b/src/security/audit.nondeep.runtime.ts index b6de420d8e6..ec3aba95c18 100644 --- a/src/security/audit.nondeep.runtime.ts +++ b/src/security/audit.nondeep.runtime.ts @@ -23,7 +23,7 @@ export { collectSandboxBrowserHashLabelFindings, collectIncludeFilePermFindings, collectStateDeepFilesystemFindings, - collectWorkspaceSkillSymlinkEscapeFindings, readConfigSnapshotForAudit, } from "./audit-extra.async.js"; +export { collectWorkspaceSkillSymlinkEscapeFindings } from "./audit-workspace-skills.js"; export { collectPluginsTrustFindings } from "./audit-plugins-trust.js";