test(plugins): unit tests for manifest, hooks, and DB - [deferred to v4.0.0-rc1] #2388
test(plugins): unit tests for manifest, hooks, and DB - [deferred to v4.0.0-rc1] #2388oyi77 wants to merge 2 commits into
Conversation
![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 implements a comprehensive plugin system featuring lifecycle management, sandboxed execution via the Node.js vm module, and an event-driven hook registry integrated into the request pipeline. It adds new database migrations, management API routes, and MCP tools for plugin administration. Feedback identifies critical security vulnerabilities, specifically potential path traversal during plugin installation and a lack of authentication on management endpoints. Additionally, the review suggests technical improvements for handling asynchronous initialization in the loader, implementing defensive type checks for plugin results, and enhancing filesystem error handling in the scanner.
| * Install a plugin from a source directory. | ||
| * Copies to plugin dir, validates manifest, registers in DB. | ||
| */ | ||
| async install(sourceDir: string): Promise<PluginRow> { |
There was a problem hiding this comment.
The install method accepts an arbitrary filesystem path from the user (via the API or MCP tool) and copies its contents into the plugin directory. This presents a significant security risk, including path traversal and arbitrary file read, as an attacker could point this to sensitive system directories if they can place a plugin.json there. Access to this functionality should be strictly restricted or the input path should be validated against a safe allowlist.
| /** | ||
| * POST /api/plugins — Install a plugin from a local path | ||
| */ | ||
| export async function POST(request: NextRequest) { |
There was a problem hiding this comment.
This API route (and other plugin management routes added in this PR) lacks authentication and authorization checks. Since plugins can execute arbitrary code on the host via the vm module, exposing these endpoints without protection allows for Remote Code Execution (RCE) by any user with network access to the gateway.
| filename: entryPoint, | ||
| timeout: 10000, // 10s init timeout | ||
| }); | ||
| } catch (err: any) { |
There was a problem hiding this comment.
The vm.runInContext call executes an async function wrapper, but the resulting promise is not awaited. This creates a race condition where moduleObj.exports might be accessed before the plugin has finished its synchronous initialization or before any top-level code has run if it contains asynchronous operations. Additionally, the timeout option in vm only applies to synchronous execution; it will not protect against infinite loops or long-running tasks that occur after the first await in the plugin code.
await vm.runInContext(wrapped, context, {
filename: entryPoint,
timeout: 10000, // 10s init timeout
});| ), | ||
| }; | ||
| } | ||
| if (pluginResult?.ctx && "body" in pluginResult.ctx) { |
There was a problem hiding this comment.
The in operator will throw a TypeError if pluginResult.ctx is not an object (e.g., if a plugin incorrectly returns a string or number as the context). A more defensive check should be used to ensure ctx is an object before using the in operator.
| if (pluginResult?.ctx && "body" in pluginResult.ctx) { | |
| if (pluginResult?.ctx && typeof pluginResult.ctx === "object" && "body" in pluginResult.ctx) { |
| 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 plugin_executions tool accepts a name argument for filtering, but it is currently ignored in the implementation. The name should be passed to the skillExecutor.listExecutions method.
| const executions = skillExecutor.listExecutions(undefined, args.limit || 20); | |
| const executions = skillExecutor.listExecutions(args.name, args.limit || 20); |
| entries = dirEntries | ||
| .filter((e) => e.isDirectory() && !e.name.startsWith(".")) | ||
| .map((e) => e.name); | ||
| } catch (err: any) { |
There was a problem hiding this comment.
The readdir call will throw an ENOTDIR error if the provided dir path points to a file instead of a directory. The current catch block only handles ENOENT. It should be updated to handle ENOTDIR gracefully to avoid crashing when an invalid path is provided.
| } catch (err: any) { | |
| } catch (err: any) { | |
| if (err.code === "ENOENT" || err.code === "ENOTDIR") { | |
| log.info("scanner.dir_not_found", { dir }); | |
| return { plugins: [], errors: [] }; | |
| } |
diegosouzapw
changed the title
2 participants
Summary
Unit tests for the plugin system (Wave 6 of #2060).
Depends on: #2385 (plugin backend core)
What's included:
tests/unit/plugins/manifest.test.ts— 12 tests: valid/invalid manifests, defaults, rejection casestests/unit/plugins/hooks.test.ts— 15 tests: register, unregister, emit, priority, error handling, asynctests/unit/plugins/db.test.ts— CRUD tests: insert, get, list, update, delete, existsTest results: 27/27 pass
Test plan
node --import tsx/esm --test tests/unit/plugins/manifest.test.ts— all passnode --import tsx/esm --test tests/unit/plugins/hooks.test.ts— all passnode --import tsx/esm --test tests/unit/plugins/db.test.ts— all pass