fix: harden plugin and hook install paths
This commit is contained in:
@@ -400,7 +400,7 @@ export async function compactEmbeddedPiSessionDirect(
|
||||
sessionManager,
|
||||
settingsManager,
|
||||
});
|
||||
applySystemPromptOverrideToSession(session, systemPromptOverride);
|
||||
applySystemPromptOverrideToSession(session, systemPromptOverride());
|
||||
|
||||
try {
|
||||
const prior = await sanitizeSessionHistory({
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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)` };
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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))) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user