Repository navigation
feat(plugins): backend core + security/ESM fixes + tests + discovery stub [deferred to v4.0.0-rc1] - #2912
Conversation
- 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>
Addresses all remaining code review feedback: 1. **Loader rewrite**: Replaced Node.js vm module with child_process.fork() for proper process-level isolation. Complies with Rule 3 (no eval). Each plugin runs in a separate Node.js process with IPC communication. 2. **Auth on all routes**: Added requireManagementAuth to all 6 plugin API route files (list, install, scan, details, activate, deactivate, config). 3. **Env filtering**: Only safe env vars passed to plugin processes unless "env" permission is granted. Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
loader.ts:
- Fix IPC: use process.send()/process.on("message") instead of worker_threads.parentPort
- Fix ESM: write host script as .mjs (not .js) to force ESM execution
- Add timeout: 10s default on callHook() with Promise.race
- Add SIGKILL escalation: SIGTERM first, then SIGKILL after 3s grace
- Fix env filtering: use allowlist (safeKeys) instead of passing all env vars
- Clear timeout on successful IPC response (no timer leak)
manager.ts:
- Fix path traversal: use fs.realpath() instead of startsWith()
- Fix imports: use registerHook/unregisterHooks from hooks.ts
- Register hooks individually via registerHook(event, name, handler)
hooks.ts:
- Copied from feat/plugin-custom-hooks (canonical registry)
Phase 1 scaffold for automated provider discovery: - DiscoveryConfig, DiscoveryResult types - probeEndpoint() for URL availability checking - scanProvider() stub (Phase 2 will implement real scanning) - getDiscoveryResults() stub - Default config: disabled (opt-in)
- index.ts: replace console.log/error with pino structured logging - hooks.ts: remove redundant .sort() in emitHookBlocking/runOnResponse (already sorted on registration) - manager.ts: add readFile import
- scanner: 9 tests (discovery, hidden dirs, validation, entry point, multiple) - loader: 5 tests (type contracts, Plugin/PluginContext/PluginResult interfaces) - manager: 6 tests (singleton, lifecycle methods, error on unknown) - Total: 20 tests, all passing
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive plugin system to OmniRoute, adding database migrations, CRUD operations, lifecycle management (scanning, loading, activation, configuration), API endpoints, and integration into the chat core request pipeline and MCP server tools. The feedback highlights several critical issues: a lack of proper context chaining in blocking hooks, a non-cross-platform path traversal guard, potential crashes in the plugin host script on non-Error exceptions, and an ignored filter argument in the plugin executions tool. Additionally, there is a potential temporary file leak on synchronous fork failures, duplicate migration files, and violations of style guide Rule 2.8 due to locally defined Zod schemas in the API routes.
| for (const reg of list) { | ||
| try { | ||
| const result = await reg.handler(payload); |
There was a problem hiding this comment.
In emitHookBlocking, the original payload is passed to each hook handler in the loop. This prevents proper chaining because subsequent plugins will not receive the modified body or metadata returned by previous plugins. To enable proper chaining, pass the updated context containing mergedBody and mergedMetadata to the handler.
| for (const reg of list) { | |
| try { | |
| const result = await reg.handler(payload); | |
| for (const reg of list) { | |
| try { | |
| const result = await reg.handler({ ...ctx, body: mergedBody, metadata: mergedMetadata }); |
| const resolvedEntry = await realpath(entryPoint).catch(() => null); | ||
| if ( | ||
| !resolvedEntry || | ||
| (!resolvedEntry.startsWith(resolvedPluginDir + "/") && resolvedEntry !== resolvedPluginDir) | ||
| ) { | ||
| throw new Error(`Plugin '${name}' entry point escapes plugin directory`); | ||
| } |
There was a problem hiding this comment.
The path traversal guard uses string concatenation with a forward slash (resolvedPluginDir + "/") to verify if the entry point is within the plugin directory. This is not cross-platform and will fail on Windows where backslashes are used. Use relative and isAbsolute to securely and portably verify the directory containment.
| const resolvedEntry = await realpath(entryPoint).catch(() => null); | |
| if ( | |
| !resolvedEntry || | |
| (!resolvedEntry.startsWith(resolvedPluginDir + "/") && resolvedEntry !== resolvedPluginDir) | |
| ) { | |
| throw new Error(`Plugin '${name}' entry point escapes plugin directory`); | |
| } | |
| const resolvedEntry = await realpath(entryPoint).catch(() => null); | |
| if (!resolvedEntry) { | |
| throw new Error(`Plugin '${name}' entry point escapes plugin directory`); | |
| } | |
| const rel = relative(resolvedPluginDir, resolvedEntry); | |
| if (rel.startsWith("..") || isAbsolute(rel)) { | |
| throw new Error(`Plugin '${name}' entry point escapes plugin directory`); | |
| } |
| } catch (err) { | ||
| process.send({ type: "result", id: msg.id, error: err.message }); | ||
| } |
There was a problem hiding this comment.
In the plugin host script, err.message is accessed directly in the catch block. If a plugin throws a non-Error value (such as a string or null), accessing err.message will throw a TypeError and crash the host process. Use a safe fallback like err instanceof Error ? err.message : String(err).
| } catch (err) { | |
| process.send({ type: "result", id: msg.id, error: err.message }); | |
| } | |
| } catch (err) { | |
| const errMsg = err instanceof Error ? err.message : String(err); | |
| process.send({ type: "result", id: msg.id, error: errMsg }); | |
| } |
| handler: async (args: { name?: string; limit?: number }) => { | ||
| // Plugin executions are tracked via the skills system | ||
| const { skillExecutor } = await import("../../../src/lib/skills/executor"); | ||
| const executions = skillExecutor.listExecutions(undefined, args.limit || 20); |
There was a problem hiding this comment.
The args.name filter is defined in the input schema but is completely ignored in the handler implementation. It should be passed to skillExecutor.listExecutions instead of undefined.
| const executions = skillExecutor.listExecutions(undefined, args.limit || 20); | |
| const executions = skillExecutor.listExecutions(args.name, args.limit || 20); |
| */ | ||
|
|
||
| import { mkdir, cp, rm, realpath, readFile } from "fs/promises"; | ||
| import { join, dirname } from "path"; |
There was a problem hiding this comment.
Import relative and isAbsolute from "path" to support a cross-platform path traversal check.
| import { join, dirname } from "path"; | |
| import { join, dirname, relative, isAbsolute } from "path"; |
| const child = fork(hostScriptPath, [entryPoint], { | ||
| env, | ||
| stdio: ["pipe", "pipe", "pipe", "ipc"], | ||
| execArgv: ["--no-warnings"], | ||
| }); |
There was a problem hiding this comment.
If fork() throws an error synchronously (e.g., due to resource limits like EMFILE or invalid arguments), the temporary host script file hostScriptPath will leak in the temp directory because the cleanup handlers are registered on the child process which was never successfully created. Wrap the fork call in a try/catch block to clean up the temporary file on failure.
let child;
try {
child = fork(hostScriptPath, [entryPoint], {
env,
stdio: ["pipe", "pipe", "pipe", "ipc"],
execArgv: ["--no-warnings"],
});
} catch (err) {
await rm(hostScriptPath, { force: true }).catch(() => {});
throw err;
}| const schema = z.object({ | ||
| config: z.record(z.string(), z.unknown()), | ||
| }); |
There was a problem hiding this comment.
According to the repository style guide (Rule 2.8), all inputs must be validated using Zod schemas imported from src/shared/validation/schemas.ts. Defining the schema locally in the route violates this rule. Please move this schema to src/shared/validation/schemas.ts and import it here.
References
- Always validate inputs with Zod schemas from src/shared/validation/schemas.ts (Repository Style Guide, Rule 2.8) (link)
| const schema = z.object({ | ||
| path: z.string().min(1), | ||
| }); |
There was a problem hiding this comment.
According to the repository style guide (Rule 2.8), all inputs must be validated using Zod schemas imported from src/shared/validation/schemas.ts. Defining the schema locally in the route violates this rule. Please move this schema to src/shared/validation/schemas.ts and import it here.
References
- Always validate inputs with Zod schemas from src/shared/validation/schemas.ts (Repository Style Guide, Rule 2.8) (link)
| -- 059: Plugin system tables | ||
| -- WordPress-style plugin management with lifecycle tracking | ||
|
|
||
| CREATE TABLE IF NOT EXISTS plugins ( | ||
| id TEXT PRIMARY KEY, | ||
| name TEXT NOT NULL UNIQUE, | ||
| version TEXT NOT NULL DEFAULT '1.0.0', | ||
| description TEXT, | ||
| author TEXT, | ||
| license TEXT DEFAULT 'MIT', | ||
| main TEXT NOT NULL DEFAULT 'index.js', | ||
| source TEXT NOT NULL DEFAULT 'local', | ||
| tags TEXT DEFAULT '[]', | ||
| status TEXT NOT NULL DEFAULT 'installed' | ||
| CHECK (status IN ('installed', 'active', 'inactive', 'error')), | ||
| enabled INTEGER NOT NULL DEFAULT 0, | ||
| manifest TEXT NOT NULL, | ||
| config TEXT DEFAULT '{}', | ||
| config_schema TEXT DEFAULT '{}', | ||
| hooks TEXT DEFAULT '[]', | ||
| permissions TEXT DEFAULT '[]', | ||
| plugin_dir TEXT NOT NULL, | ||
| error_message TEXT, | ||
| installed_at TEXT NOT NULL DEFAULT (datetime('now')), | ||
| updated_at TEXT NOT NULL DEFAULT (datetime('now')), | ||
| activated_at TEXT | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_plugins_status ON plugins(status); | ||
| CREATE INDEX IF NOT EXISTS idx_plugins_enabled ON plugins(enabled); | ||
| CREATE INDEX IF NOT EXISTS idx_plugins_name ON plugins(name); |
There was a problem hiding this comment.
This migration file is identical to 059_create_plugins.sql. Having duplicate migration files is redundant and can lead to confusion or maintenance issues. Please remove one of the duplicate migration files.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code or existing comments that cannot receive inline comments:
Files Reviewed (12 files)
Positive notes:
Reviewed by nemotron-3-super-120b-a12b-20230311:free · 855,599 tokens |
diegosouzapw
changed the title
feat(plugins): backend core + security/ESM fixes + tests + discovery stub [deferred to v4.0.0-rc1]
2 participants
Summary
Plugin system backend fixes and extensions for v4.0.0-rc1.
Security + ESM Fixes
process.send()/process.on("message")instead ofworker_threads.parentPort.mjshost script (forces ESM regardless of package.json)callHook()withPromise.racefs.realpath()instead ofstartsWith()Code Review Feedback
vmmodule withchild_process.fork()for process-level isolationDiscovery Tool
src/lib/discovery/index.tsDiscoveryConfig,DiscoveryResulttypesprobeEndpoint(),scanProvider(),getDiscoveryResults()stubsTests
Migration
Test plan
npm run typecheck:core— 0 errorsnode --import tsx/esm --test tests/unit/plugins-scanner.test.ts— 9/9 passnode --import tsx/esm --test tests/unit/plugins-loader.test.ts— 5/5 passnode --import tsx/esm --test tests/unit/plugins-manager.test.ts— 6/6 pass