feat(plugins): add the bundled plugin core - #285
Conversation
…udit, runner) Ships the plugin layer for bundled-only plugins (plugins/<name>/, code we ship, never user-added or remote): manifest validation, a bounded read/fetch context, a point-of-need consent gate that resolves through the existing automation mayRun() predicate, an activity-log audit row per run, and a timeout-guarded runner that never lets a plugin throw/hang/leak into an unhandled rejection. Adds an allowedHosts option to fetchPublicHttpText (checked on the initial target and every redirect hop) so the plugin context's fetch() can scope a plugin to its manifest's fetchHosts without weakening the existing SSRF guard. Includes example-echo, a test-only template plugin, and tests/plugins-core.test.mjs.
|
@coderabbitai full review |
|
Deployment failed for project careerrat-website with the following error: Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
…adline, keep audit rows in DB mode Five gaps from the Codex adversarial pass on the plugin core: - runPlugin validates the plugin name against the manifest name grammar before any path join, requires manifest.name to match, and checks realpath containment of the plugin dir, manifest and entry under the plugins root, so neither "../x" nor a symlinked directory can load code from outside plugins/ - the deadline now starts before the dynamic import and drives one AbortController shared by import, run(ctx) and ctx.fetch, so an unresolved top-level await times out and in-flight fetches abort; a synchronous busy loop still cannot be preempted in-process, noted inline - recordPluginRun writes through the canonical activityAppend verb in DB-mode workspaces and only appends the legacy JSONL otherwise, so the next export no longer drops plugin events; audit detail gains a status - allowedHosts is checked against the URL hostname before DNS resolution for the initial request and every redirect hop - plugins/** is covered by Biome and Knip
…rrat into feat/plugin-core
…layer does The ctx.fetch pre-check compared a lowercased URL hostname against fetchHosts, so a bracketed IPv6 literal could never match its unbracketed manifest entry. It failed closed, but disagreed with hostAllowed in public-http-fetch.mjs. Reuse that file's normalizedHostname so both checks see the same string.
|
CodeRabbit is rate limited, so this went through two adversarial passes. Codex, first pass, five findings, all fixed in 1817a15: plugin names could escape plugins/ via ../ or a symlink (now grammar-checked before any path join, manifest name must match, realpath containment on dir, manifest and entry); the timeout started after the dynamic import and left in-flight work running (one AbortController now starts before import and reaches run(ctx) and ctx.fetch); audit rows were written to the legacy JSONL even in DB-mode workspaces, where the next export dropped them (now via the activityAppend verb in DB mode); denied hosts were DNS-resolved before the allowlist check (allowlist now runs on the URL hostname first, initial and redirects); plugins/** was outside the Biome and Knip globs. Codex's backend was down for the re-verify, so the second pass ran on the Claude-side reviewer against 1817a15. Verdict ship, two low nits: the ctx.fetch host pre-check didn't strip IPv6 brackets the way the fetch layer does (fail-closed, fixed in e9b42ff by reusing normalizedHostname), and there's no separate symlink test for the manifest and entry positions (same containment function as the directory test, left as is). Worker isolation for plugins was raised and declined: bundled plugins are reviewed code, so a synchronous busy loop is accepted and noted inline. |
biggest-littlest
left a comment
biggest-littlest
left a comment
There was a problem hiding this comment.
Codex plus Claude adversarial passes, five findings fixed and two nits handled with the trace on the PR, CI green on the final head.
ALARGECOMPANY
left a comment
ALARGECOMPANY
left a comment
There was a problem hiding this comment.
Codex plus Claude adversarial passes, five findings fixed and two nits handled with the trace on the PR, CI green on the final head.
3 participants
First slice of the plugin layer (roadmap CR26, Port B). Bundled plugins only: they ship in repo-root plugins/, there is no user-added plugin, no remote code, no lock file. What this adds is the contract around a plugin: what it may read, what it may fetch, what it records, and a point-of-need consent gate.
31 new tests. No CHANGELOG entry: nothing user-visible until h1b-sponsor lands.