From d03eca8450dc493b198a88b105fd180895238e57 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Feb 2026 02:04:50 -0800 Subject: [PATCH] fix: harden plugin and hook install paths --- src/agents/pi-embedded-runner/compact.ts | 2 +- src/agents/pi-embedded-runner/run/attempt.ts | 4 +- .../pi-embedded-runner/system-prompt.ts | 7 +- src/cli/hooks-cli.ts | 8 +- src/hooks/install.test.ts | 94 +++++++++++++++++++ src/hooks/install.ts | 64 ++++++++++++- src/plugins/install.test.ts | 72 ++++++++++++++ src/plugins/install.ts | 58 +++++++++++- src/plugins/update.ts | 12 ++- 9 files changed, 307 insertions(+), 14 deletions(-) diff --git a/src/agents/pi-embedded-runner/compact.ts b/src/agents/pi-embedded-runner/compact.ts index d4d2a5555..57a30fbdf 100644 --- a/src/agents/pi-embedded-runner/compact.ts +++ b/src/agents/pi-embedded-runner/compact.ts @@ -400,7 +400,7 @@ export async function compactEmbeddedPiSessionDirect( sessionManager, settingsManager, }); - applySystemPromptOverrideToSession(session, systemPromptOverride); + applySystemPromptOverrideToSession(session, systemPromptOverride()); try { const prior = await sanitizeSessionHistory({ diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index 945d11eb2..03dfd11d2 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -390,7 +390,7 @@ export async function runEmbeddedAttempt( tools, }); const systemPromptOverride = createSystemPromptOverride(appendPrompt); - const systemPromptText = systemPromptOverride; + const systemPromptText = systemPromptOverride(); const sessionLock = await acquireSessionWriteLock({ sessionFile: params.sessionFile, @@ -475,7 +475,7 @@ export async function runEmbeddedAttempt( sessionManager, settingsManager, })); - applySystemPromptOverrideToSession(session, systemPromptOverride); + applySystemPromptOverrideToSession(session, systemPromptText); if (!session) { throw new Error("Embedded agent session missing"); } diff --git a/src/agents/pi-embedded-runner/system-prompt.ts b/src/agents/pi-embedded-runner/system-prompt.ts index 6384bc7e4..f1caa98eb 100644 --- a/src/agents/pi-embedded-runner/system-prompt.ts +++ b/src/agents/pi-embedded-runner/system-prompt.ts @@ -78,8 +78,11 @@ export function createSystemPromptOverride(systemPrompt: string): string { return systemPrompt.trim(); } -export function applySystemPromptOverrideToSession(session: AgentSession, override: string) { - const prompt = override.trim(); +export function applySystemPromptOverrideToSession( + session: AgentSession, + override: string | ((defaultPrompt?: string) => string), +) { + const prompt = typeof override === "function" ? override() : override.trim(); session.agent.setSystemPrompt(prompt); const mutableSession = session as unknown as { _baseSystemPrompt?: string; diff --git a/src/cli/hooks-cli.ts b/src/cli/hooks-cli.ts index e75dff0ea..9b3ab0100 100644 --- a/src/cli/hooks-cli.ts +++ b/src/cli/hooks-cli.ts @@ -771,7 +771,13 @@ export function registerHooksCli(program: Command): void { continue; } - const installPath = record.installPath ?? resolveHookInstallDir(hookId); + let installPath: string; + try { + installPath = record.installPath ?? resolveHookInstallDir(hookId); + } catch (err) { + defaultRuntime.log(theme.error(`Invalid install path for "${hookId}": ${String(err)}`)); + continue; + } const currentVersion = await readInstalledPackageVersion(installPath); if (opts.dryRun) { diff --git a/src/hooks/install.test.ts b/src/hooks/install.test.ts index 97bc7682a..d9cc3b16a 100644 --- a/src/hooks/install.test.ts +++ b/src/hooks/install.test.ts @@ -118,6 +118,100 @@ describe("installHooksFromArchive", () => { expect(result.hooks).toContain("tar-hook"); expect(result.targetDir).toBe(path.join(stateDir, "hooks", "tar-hooks")); }); + + it("rejects hook packs with traversal-like ids", async () => { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const archivePath = path.join(workDir, "hooks.tar"); + const pkgDir = path.join(workDir, "package"); + + fs.mkdirSync(path.join(pkgDir, "hooks", "evil-hook"), { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ + name: "@evil/..", + version: "0.0.1", + openclaw: { hooks: ["./hooks/evil-hook"] }, + }), + "utf-8", + ); + fs.writeFileSync( + path.join(pkgDir, "hooks", "evil-hook", "HOOK.md"), + [ + "---", + "name: evil-hook", + "description: Evil hook", + 'metadata: {"openclaw":{"events":["command:new"]}}', + "---", + "", + "# Evil Hook", + ].join("\n"), + "utf-8", + ); + fs.writeFileSync( + path.join(pkgDir, "hooks", "evil-hook", "handler.ts"), + "export default async () => {};\n", + "utf-8", + ); + await tar.c({ cwd: workDir, file: archivePath }, ["package"]); + + const hooksDir = path.join(stateDir, "hooks"); + const { installHooksFromArchive } = await import("./install.js"); + const result = await installHooksFromArchive({ archivePath, hooksDir }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("reserved path segment"); + }); + + it("rejects hook packs with reserved ids", async () => { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const archivePath = path.join(workDir, "hooks.tar"); + const pkgDir = path.join(workDir, "package"); + + fs.mkdirSync(path.join(pkgDir, "hooks", "reserved-hook"), { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ + name: "@evil/.", + version: "0.0.1", + openclaw: { hooks: ["./hooks/reserved-hook"] }, + }), + "utf-8", + ); + fs.writeFileSync( + path.join(pkgDir, "hooks", "reserved-hook", "HOOK.md"), + [ + "---", + "name: reserved-hook", + "description: Reserved hook", + 'metadata: {"openclaw":{"events":["command:new"]}}', + "---", + "", + "# Reserved Hook", + ].join("\n"), + "utf-8", + ); + fs.writeFileSync( + path.join(pkgDir, "hooks", "reserved-hook", "handler.ts"), + "export default async () => {};\n", + "utf-8", + ); + await tar.c({ cwd: workDir, file: archivePath }, ["package"]); + + const hooksDir = path.join(stateDir, "hooks"); + const { installHooksFromArchive } = await import("./install.js"); + const result = await installHooksFromArchive({ archivePath, hooksDir }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("reserved path segment"); + }); }); describe("installHooksFromPath", () => { diff --git a/src/hooks/install.ts b/src/hooks/install.ts index 4594f99df..63e4be39e 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -49,12 +49,52 @@ function safeDirName(input: string): string { if (!trimmed) { return trimmed; } - return trimmed.replaceAll("/", "__"); + return trimmed.replaceAll("/", "__").replaceAll("\\", "__"); +} + +function validateHookId(hookId: string): string | null { + if (!hookId) { + return "invalid hook name: missing"; + } + if (hookId === "." || hookId === "..") { + return "invalid hook name: reserved path segment"; + } + if (hookId.includes("/") || hookId.includes("\\")) { + return "invalid hook name: path separators not allowed"; + } + return null; } export function resolveHookInstallDir(hookId: string, hooksDir?: string): string { const hooksBase = hooksDir ? resolveUserPath(hooksDir) : path.join(CONFIG_DIR, "hooks"); - return path.join(hooksBase, safeDirName(hookId)); + const hookIdError = validateHookId(hookId); + if (hookIdError) { + throw new Error(hookIdError); + } + const targetDirResult = resolveSafeInstallDir(hooksBase, hookId); + if (!targetDirResult.ok) { + throw new Error(targetDirResult.error); + } + return targetDirResult.path; +} + +function resolveSafeInstallDir( + hooksDir: string, + hookId: string, +): { ok: true; path: string } | { ok: false; error: string } { + const targetDir = path.join(hooksDir, safeDirName(hookId)); + const resolvedBase = path.resolve(hooksDir); + const resolvedTarget = path.resolve(targetDir); + const relative = path.relative(resolvedBase, resolvedTarget); + if ( + !relative || + relative === ".." || + relative.startsWith(`..${path.sep}`) || + path.isAbsolute(relative) + ) { + return { ok: false, error: "invalid hook name: path traversal detected" }; + } + return { ok: true, path: targetDir }; } async function ensureOpenClawHooks(manifest: HookPackageManifest) { @@ -130,6 +170,10 @@ async function installHookPackageFromDir(params: { const pkgName = typeof manifest.name === "string" ? manifest.name : ""; const hookPackId = pkgName ? unscopedPackageName(pkgName) : path.basename(params.packageDir); + const hookIdError = validateHookId(hookPackId); + if (hookIdError) { + return { ok: false, error: hookIdError }; + } if (params.expectedHookPackId && params.expectedHookPackId !== hookPackId) { return { ok: false, @@ -142,7 +186,11 @@ async function installHookPackageFromDir(params: { : path.join(CONFIG_DIR, "hooks"); await fs.mkdir(hooksDir, { recursive: true }); - const targetDir = resolveHookInstallDir(hookPackId, hooksDir); + const targetDirResult = resolveSafeInstallDir(hooksDir, hookPackId); + if (!targetDirResult.ok) { + return { ok: false, error: targetDirResult.error }; + } + const targetDir = targetDirResult.path; if (mode === "install" && (await fileExists(targetDir))) { return { ok: false, error: `hook pack already exists: ${targetDir} (delete it first)` }; } @@ -229,6 +277,10 @@ async function installHookFromDir(params: { await validateHookDir(params.hookDir); const hookName = await resolveHookNameFromDir(params.hookDir); + const hookIdError = validateHookId(hookName); + if (hookIdError) { + return { ok: false, error: hookIdError }; + } if (params.expectedHookPackId && params.expectedHookPackId !== hookName) { return { @@ -242,7 +294,11 @@ async function installHookFromDir(params: { : path.join(CONFIG_DIR, "hooks"); await fs.mkdir(hooksDir, { recursive: true }); - const targetDir = resolveHookInstallDir(hookName, hooksDir); + const targetDirResult = resolveSafeInstallDir(hooksDir, hookName); + if (!targetDirResult.ok) { + return { ok: false, error: targetDirResult.error }; + } + const targetDir = targetDirResult.path; if (mode === "install" && (await fileExists(targetDir))) { return { ok: false, error: `hook already exists: ${targetDir} (delete it first)` }; } diff --git a/src/plugins/install.test.ts b/src/plugins/install.test.ts index 22bd0990b..b8014257f 100644 --- a/src/plugins/install.test.ts +++ b/src/plugins/install.test.ts @@ -268,6 +268,78 @@ describe("installPluginFromArchive", () => { expect(manifest.version).toBe("0.0.2"); }); + it("rejects traversal-like plugin names", async () => { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const pkgDir = path.join(workDir, "package"); + fs.mkdirSync(path.join(pkgDir, "dist"), { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ + name: "@evil/..", + version: "0.0.1", + openclaw: { extensions: ["./dist/index.js"] }, + }), + "utf-8", + ); + fs.writeFileSync(path.join(pkgDir, "dist", "index.js"), "export {};", "utf-8"); + + const archivePath = packToArchive({ + pkgDir, + outDir: workDir, + outName: "traversal.tgz", + }); + + const extensionsDir = path.join(stateDir, "extensions"); + const { installPluginFromArchive } = await import("./install.js"); + const result = await installPluginFromArchive({ + archivePath, + extensionsDir, + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("reserved path segment"); + }); + + it("rejects reserved plugin ids", async () => { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const pkgDir = path.join(workDir, "package"); + fs.mkdirSync(path.join(pkgDir, "dist"), { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ + name: "@evil/.", + version: "0.0.1", + openclaw: { extensions: ["./dist/index.js"] }, + }), + "utf-8", + ); + fs.writeFileSync(path.join(pkgDir, "dist", "index.js"), "export {};", "utf-8"); + + const archivePath = packToArchive({ + pkgDir, + outDir: workDir, + outName: "reserved.tgz", + }); + + const extensionsDir = path.join(stateDir, "extensions"); + const { installPluginFromArchive } = await import("./install.js"); + const result = await installPluginFromArchive({ + archivePath, + extensionsDir, + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("reserved path segment"); + }); + it("rejects packages without openclaw.extensions", async () => { const stateDir = makeTempDir(); const workDir = makeTempDir(); diff --git a/src/plugins/install.ts b/src/plugins/install.ts index fa27dad2a..08f4ad29e 100644 --- a/src/plugins/install.ts +++ b/src/plugins/install.ts @@ -49,13 +49,26 @@ function safeDirName(input: string): string { if (!trimmed) { return trimmed; } - return trimmed.replaceAll("/", "__"); + return trimmed.replaceAll("/", "__").replaceAll("\\", "__"); } function safeFileName(input: string): string { return safeDirName(input); } +function validatePluginId(pluginId: string): string | null { + if (!pluginId) { + return "invalid plugin name: missing"; + } + if (pluginId === "." || pluginId === "..") { + return "invalid plugin name: reserved path segment"; + } + if (pluginId.includes("/") || pluginId.includes("\\")) { + return "invalid plugin name: path separators not allowed"; + } + return null; +} + async function ensureOpenClawExtensions(manifest: PackageManifest) { const extensions = manifest[MANIFEST_KEY]?.extensions; if (!Array.isArray(extensions)) { @@ -72,7 +85,34 @@ export function resolvePluginInstallDir(pluginId: string, extensionsDir?: string const extensionsBase = extensionsDir ? resolveUserPath(extensionsDir) : path.join(CONFIG_DIR, "extensions"); - return path.join(extensionsBase, safeDirName(pluginId)); + const pluginIdError = validatePluginId(pluginId); + if (pluginIdError) { + throw new Error(pluginIdError); + } + const targetDirResult = resolveSafeInstallDir(extensionsBase, pluginId); + if (!targetDirResult.ok) { + throw new Error(targetDirResult.error); + } + return targetDirResult.path; +} + +function resolveSafeInstallDir( + extensionsDir: string, + pluginId: string, +): { ok: true; path: string } | { ok: false; error: string } { + const targetDir = path.join(extensionsDir, safeDirName(pluginId)); + const resolvedBase = path.resolve(extensionsDir); + const resolvedTarget = path.resolve(targetDir); + const relative = path.relative(resolvedBase, resolvedTarget); + if ( + !relative || + relative === ".." || + relative.startsWith(`..${path.sep}`) || + path.isAbsolute(relative) + ) { + return { ok: false, error: "invalid plugin name: path traversal detected" }; + } + return { ok: true, path: targetDir }; } async function installPluginFromPackageDir(params: { @@ -110,6 +150,10 @@ async function installPluginFromPackageDir(params: { const pkgName = typeof manifest.name === "string" ? manifest.name : ""; const pluginId = pkgName ? unscopedPackageName(pkgName) : "plugin"; + const pluginIdError = validatePluginId(pluginId); + if (pluginIdError) { + return { ok: false, error: pluginIdError }; + } if (params.expectedPluginId && params.expectedPluginId !== pluginId) { return { ok: false, @@ -122,7 +166,11 @@ async function installPluginFromPackageDir(params: { : path.join(CONFIG_DIR, "extensions"); await fs.mkdir(extensionsDir, { recursive: true }); - const targetDir = path.join(extensionsDir, safeDirName(pluginId)); + const targetDirResult = resolveSafeInstallDir(extensionsDir, pluginId); + if (!targetDirResult.ok) { + return { ok: false, error: targetDirResult.error }; + } + const targetDir = targetDirResult.path; if (mode === "install" && (await fileExists(targetDir))) { return { @@ -307,6 +355,10 @@ export async function installPluginFromFile(params: { const base = path.basename(filePath, path.extname(filePath)); const pluginId = base || "plugin"; + const pluginIdError = validatePluginId(pluginId); + if (pluginIdError) { + return { ok: false, error: pluginIdError }; + } const targetFile = path.join(extensionsDir, `${safeFileName(pluginId)}${path.extname(filePath)}`); if (mode === "install" && (await fileExists(targetFile))) { diff --git a/src/plugins/update.ts b/src/plugins/update.ts index df312dc1e..46b8d4abe 100644 --- a/src/plugins/update.ts +++ b/src/plugins/update.ts @@ -189,7 +189,17 @@ export async function updateNpmInstalledPlugins(params: { continue; } - const installPath = record.installPath ?? resolvePluginInstallDir(pluginId); + let installPath: string; + try { + installPath = record.installPath ?? resolvePluginInstallDir(pluginId); + } catch (err) { + outcomes.push({ + pluginId, + status: "error", + message: `Invalid install path for "${pluginId}": ${String(err)}`, + }); + continue; + } const currentVersion = await readInstalledPackageVersion(installPath); if (params.dryRun) {