fix(plugins): address code review feedback

- Path traversal guard: validate entryPoint stays within plugin dir
- install() now handles direct plugin directories (not just parent dirs)
- Non-null assertion replaced with explicit null check
- require efficiency: allowedModules map moved outside function
- Source wrapper: add newlines to prevent trailing comment issues
- Config validation: validate values against configSchema on save
- Dynamic import comment: clarify Node.js caching behavior

Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
This commit is contained in:
oyi77
2026-05-19 06:03:06 +07:00
committed by diegosouzapw
parent 0974b96fa2
commit 087a880bdb
5 changed files with 77 additions and 10 deletions

View File

@@ -1526,6 +1526,7 @@ export async function handleChatCore({
let tokensCompressed: number | null = null;
body = injectSystemPrompt(body);
// ── Plugin onRequest hook ──
// Dynamic import cached by Node.js after first call — minimal overhead
try {
const { runOnRequest } = await import("@/lib/plugins/index");
const pluginCtx = {
@@ -1560,7 +1561,7 @@ export async function handleChatCore({
};
}
if (pluginResult?.ctx && "body" in pluginResult.ctx) {
body = (pluginResult.ctx as Record<string, unknown>).body;
body = (pluginResult.ctx as unknown as Record<string, unknown>).body;
}
} catch (pluginErr) {
log?.debug?.(

View File

@@ -60,6 +60,35 @@ export async function PUT(request: NextRequest, { params }: { params: Promise<{
);
}
// Validate config values against configSchema if defined
const configSchema = JSON.parse(plugin.configSchema || "{}");
if (Object.keys(configSchema).length > 0) {
for (const [key, value] of Object.entries(parsed.data.config)) {
const field = configSchema[key];
if (!field) continue; // Allow extra keys
if (field.type === "number" && typeof value === "number") {
if (field.min !== undefined && value < field.min) {
return NextResponse.json(
{ error: `Config '${key}' must be >= ${field.min}` },
{ status: 400, headers: CORS_HEADERS }
);
}
if (field.max !== undefined && value > field.max) {
return NextResponse.json(
{ error: `Config '${key}' must be <= ${field.max}` },
{ status: 400, headers: CORS_HEADERS }
);
}
}
if (field.type === "select" && field.enum && !field.enum.includes(String(value))) {
return NextResponse.json(
{ error: `Config '${key}' must be one of: ${field.enum.join(", ")}` },
{ status: 400, headers: CORS_HEADERS }
);
}
}
}
updatePluginConfig(name, parsed.data.config);
return NextResponse.json(

View File

@@ -118,7 +118,11 @@ export function insertPlugin(input: PluginCreateInput): PluginRow {
);
log.info("plugin.inserted", { id: input.id, name: input.name });
return getPluginByName(input.name)!;
const plugin = getPluginByName(input.name);
if (!plugin) {
throw new Error(`Failed to retrieve plugin '${input.name}' after insertion`);
}
return plugin;
}
export function getPluginById(id: string): PluginRow | null {

View File

@@ -98,19 +98,18 @@ export async function loadPlugin(
const moduleObj = { exports: moduleExports };
sandbox.module = moduleObj;
sandbox.exports = moduleExports;
const allowedModules: Record<string, unknown> = {};
if (permissions.includes("network")) {
allowedModules.crypto = require("crypto");
}
sandbox.require = (id: string) => {
// Only allow specific safe modules
const allowed: Record<string, unknown> = {};
if (id === "crypto") {
allowed.crypto = require("crypto");
}
if (allowed[id]) return allowed[id];
if (id in allowedModules) return allowedModules[id];
throw new Error(`Module '${id}' is not allowed in plugin sandbox`);
};
try {
// Wrap source in a function to capture exports
const wrapped = `(async function(module, exports, require) { ${source} })(module, exports, require);`;
const wrapped = `(async function(module, exports, require) {\n${source}\n})(module, exports, require);`;
vm.runInContext(wrapped, context, {
filename: entryPoint,
timeout: 10000, // 10s init timeout

View File

@@ -48,7 +48,36 @@ class PluginManager {
* Copies to plugin dir, validates manifest, registers in DB.
*/
async install(sourceDir: string): Promise<PluginRow> {
const { plugins, errors } = await scanPluginDir(sourceDir);
// Check if sourceDir itself contains plugin.json (direct plugin dir)
const { safeValidateManifest } = await import("./manifest");
const { readFile: readFileFs } = await import("fs/promises");
let directPlugin: {
name: string;
manifest: any;
pluginDir: string;
entryPoint: string;
} | null = null;
try {
const manifestPath = join(sourceDir, "plugin.json");
const raw = await readFileFs(manifestPath, "utf-8");
const parsed = JSON.parse(raw);
const result = safeValidateManifest(parsed);
if (result.success) {
const entryPoint = join(sourceDir, result.data.main);
directPlugin = {
name: result.data.name,
manifest: result.data,
pluginDir: sourceDir,
entryPoint,
};
}
} catch {}
const { plugins, errors } = directPlugin
? { plugins: [directPlugin], errors: [] }
: await scanPluginDir(sourceDir);
if (plugins.length === 0) {
throw new Error(
`No valid plugin found in ${sourceDir}: ${errors.map((e) => e.error).join(", ")}`
@@ -110,7 +139,12 @@ class PluginManager {
if (row.status === "active") return;
const manifest = JSON.parse(row.manifest) as PluginManifestWithDefaults;
// Path traversal guard: entry point must stay within plugin directory
const entryPoint = join(row.pluginDir, manifest.main);
if (!entryPoint.startsWith(row.pluginDir)) {
throw new Error(`Plugin '${name}' entry point escapes plugin directory`);
}
try {
const loaded = await loadPlugin(entryPoint, manifest);