Plugin system (8/8): filesystem/httpClient capabilities + hardening#3732
Plugin system (8/8): filesystem/httpClient capabilities + hardening#3732ccdwyer wants to merge 9 commits into
Conversation
Threads carry an owner (user or plugin:<id>); plugin-owned threads are hidden from user-facing projections, activity relays, and client thread state. Includes migration 033 and the decider/projector/read-model wiring. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } | ||
|
|
||
| function requestQuery(url: URL): Readonly<Record<string, string | ReadonlyArray<string>>> { | ||
| const query: Record<string, string | Array<string>> = {}; |
There was a problem hiding this comment.
🟠 High plugins/PluginHttpRoutes.ts:56
requestQuery builds the query map with a plain object literal, so Object.prototype keys like toString, constructor, and __proto__ are treated as already present. For a request like ?toString=x, const existing = query[key] reads the inherited toString function instead of undefined, so line 64 stores [function toString(), "x"] instead of the string "x". These keys reach plugins with the wrong value shape, and __proto__ can even mutate the accumulator's prototype. Consider initializing the map with Object.create(null) so inherited prototype properties are absent.
| const query: Record<string, string | Array<string>> = {}; | |
| const query: Record<string, string | Array<string>> = Object.create(null); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHttpRoutes.ts around line 56:
`requestQuery` builds the query map with a plain object literal, so `Object.prototype` keys like `toString`, `constructor`, and `__proto__` are treated as already present. For a request like `?toString=x`, `const existing = query[key]` reads the inherited `toString` function instead of `undefined`, so line 64 stores `[function toString(), "x"]` instead of the string `"x"`. These keys reach plugins with the wrong value shape, and `__proto__` can even mutate the accumulator's prototype. Consider initializing the map with `Object.create(null)` so inherited prototype properties are absent.
| ); | ||
| orphanedVersionDir = null; | ||
| yield* dropStage(stageToken); | ||
| yield* host |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:821
confirmInstall calls host.activatePlugin(stage.pluginId) and maps its errors to activation-failed, but PluginHost.activatePlugin does not always surface activation failures — it swallows ordinary load errors after flipping the lockfile entry away from active. As a result, a plugin whose server entry crashes during activation makes confirmInstall return success with a plugin payload even though the plugin never actually activated and its lockfile state is no longer active. Consider having PluginHost.activatePlugin propagate load errors, or having confirmInstall re-read the lockfile entry after activation to verify state === "active" before returning success.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 821:
`confirmInstall` calls `host.activatePlugin(stage.pluginId)` and maps its errors to `activation-failed`, but `PluginHost.activatePlugin` does not always surface activation failures — it swallows ordinary load errors after flipping the lockfile entry away from `active`. As a result, a plugin whose server entry crashes during activation makes `confirmInstall` return success with a `plugin` payload even though the plugin never actually activated and its lockfile state is no longer `active`. Consider having `PluginHost.activatePlugin` propagate load errors, or having `confirmInstall` re-read the lockfile entry after activation to verify `state === "active"` before returning success.
| return yield* new OutboundUrlError({ reason: "Only https:// targets are allowed" }); | ||
| } | ||
|
|
||
| const addresses = yield* deps.lookup(parsed.hostname).pipe( |
There was a problem hiding this comment.
🟡 Medium plugins/OutboundUrlValidator.ts:231
OutboundUrlValidator.resolve passes parsed.hostname directly to deps.lookup, but WHATWG URL.hostname includes the surrounding brackets for IPv6-literal URLs (e.g. [2606:4700:4700::1111]). Node's dns.lookup expects the raw literal without brackets, so every IPv6-literal URL fails resolution and is rejected — even valid public HTTPS targets. Consider stripping the brackets before calling deps.lookup.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/OutboundUrlValidator.ts around line 231:
`OutboundUrlValidator.resolve` passes `parsed.hostname` directly to `deps.lookup`, but WHATWG `URL.hostname` includes the surrounding brackets for IPv6-literal URLs (e.g. `[2606:4700:4700::1111]`). Node's `dns.lookup` expects the raw literal without brackets, so every IPv6-literal URL fails resolution and is rejected — even valid public HTTPS targets. Consider stripping the brackets before calling `deps.lookup`.
| if (patternSegments.length !== requestSegments.length) return null; | ||
|
|
||
| const params: Record<string, string> = {}; | ||
| for (let index = 0; index < patternSegments.length; index++) { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHttpRegistry.ts:47
matchPath compares literal path segments against the raw percent-encoded request path but only decodeURIComponents the :param segments. Since match is called with URL.pathname, which preserves percent-encoding, a literal descriptor segment like café or hello world will never equal the incoming %C3%A9 or %20 form, so valid routes always 404. Decode both segments before comparing so literal and parameter segments match consistently.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHttpRegistry.ts around line 47:
`matchPath` compares literal path segments against the raw percent-encoded request path but only `decodeURIComponent`s the `:param` segments. Since `match` is called with `URL.pathname`, which preserves percent-encoding, a literal descriptor segment like `café` or `hello world` will never equal the incoming `%C3%A9` or `%20` form, so valid routes always 404. Decode both segments before comparing so literal and parameter segments match consistently.
| entryPath === "assets" || | ||
| entryPath.startsWith("assets/"); | ||
|
|
||
| const validateRelativeArchivePath = (entryPath: string) => { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:181
validateRelativeArchivePath rejects any archive entry outside manifest.json, server/, web/, and assets/. macOS tar (libarchive) emits PaxGlobal and PaxLocal extended-header records (typeflag g/x) and AppleDouble ._ metadata files by default, and standard bsdtar may use Pax headers as well. Such entries are neither in the allowlist nor among the accepted typeflags (\0/0/5), so a plugin archive created with the platform-default tar fails extraction with extract-failed before manifest.json is even read. Consider allowing Pax extended-header records (typeFlag === "g" || typeFlag === "x") and skipping AppleDouble ._ metadata entries (or documenting that plugin archives must be produced with a specific tar format and --no-mac-metadata).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 181:
`validateRelativeArchivePath` rejects any archive entry outside `manifest.json`, `server/`, `web/`, and `assets/`. macOS `tar` (libarchive) emits PaxGlobal and PaxLocal extended-header records (typeflag `g`/`x`) and AppleDouble `._` metadata files by default, and standard `bsdtar` may use Pax headers as well. Such entries are neither in the allowlist nor among the accepted typeflags (`\0`/`0`/`5`), so a plugin archive created with the platform-default `tar` fails extraction with `extract-failed` before `manifest.json` is even read. Consider allowing Pax extended-header records (`typeFlag === "g" || typeFlag === "x"`) and skipping AppleDouble `._` metadata entries (or documenting that plugin archives must be produced with a specific `tar` format and `--no-mac-metadata`).
| readonly name: string; | ||
| }): Effect.Effect<DirEntry | null, FilesystemError> { | ||
| return Effect.gen(function* () { | ||
| const realEntry = yield* Effect.tryPromise({ |
There was a problem hiding this comment.
🟡 Medium capabilities/FilesystemCapability.ts:517
dirEntryFor uses Effect.orElseSucceed(() => null) on both the realpath and stat calls, so any I/O failure (e.g. EACCES, EIO, ENOTDIR) causes the entry to be silently dropped. listDir and listDirRecursive then return incomplete directory listings with no error, hiding real failures from callers. Only ENOENT (the entry disappeared mid-listing) should map to null; other errors should propagate.
Also found in 1 other location(s)
apps/server/src/ws.ts:150
toThreadUpsertedShellStreamEventadds a second database lookup viagetThreadOwnerByIdbefore fetching the shell, then swallows any failure withEffect.orElseSucceed(() => Option.none()). A transient error in the new owner query now drops a normal user-threadthread-upsertedevent even whengetThreadShellByIdwould have succeeded, so thread/sidebar updates can silently disappear under repository/query failures that did not block them before this change.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/FilesystemCapability.ts around line 517:
`dirEntryFor` uses `Effect.orElseSucceed(() => null)` on both the `realpath` and `stat` calls, so any I/O failure (e.g. `EACCES`, `EIO`, `ENOTDIR`) causes the entry to be silently dropped. `listDir` and `listDirRecursive` then return incomplete directory listings with no error, hiding real failures from callers. Only `ENOENT` (the entry disappeared mid-listing) should map to `null`; other errors should propagate.
Also found in 1 other location(s):
- apps/server/src/ws.ts:150 -- `toThreadUpsertedShellStreamEvent` adds a second database lookup via `getThreadOwnerById` before fetching the shell, then swallows any failure with `Effect.orElseSucceed(() => Option.none())`. A transient error in the new owner query now drops a normal user-thread `thread-upserted` event even when `getThreadShellById` would have succeeded, so thread/sidebar updates can silently disappear under repository/query failures that did not block them before this change.
| } | ||
| }); | ||
|
|
||
| const uninstall: PluginInstaller["Service"]["uninstall"] = (input) => |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:878
uninstall creates the .preserve-data-on-remove marker before verifying that input.pluginId is installed. When the plugin is missing, store.updatePlugin() fails with plugin-not-found, but the marker directory remains under config.pluginsDir/<pluginId>. A later real install of the same id inherits that stale marker, so uninstall({ removeData: true }) will preserve data because PluginHost.reconcilePendingState only checks for marker existence. Consider moving the marker creation after the lockfile entry is confirmed to exist.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 878:
`uninstall` creates the `.preserve-data-on-remove` marker before verifying that `input.pluginId` is installed. When the plugin is missing, `store.updatePlugin()` fails with `plugin-not-found`, but the marker directory remains under `config.pluginsDir/<pluginId>`. A later real install of the same id inherits that stale marker, so `uninstall({ removeData: true })` will preserve data because `PluginHost.reconcilePendingState` only checks for marker existence. Consider moving the marker creation after the lockfile entry is confirmed to exist.
| // server-side importers. | ||
| export { compareSemver }; | ||
|
|
||
| const installedEntry = (entry: PluginLockfilePlugin | undefined): PluginLockfilePlugin => { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:206
installedEntry throws a PluginManagementError synchronously inside Effect.gen blocks (e.g. beginUpgrade, setEnabled, uninstall, and the updatePlugin callback). A synchronous throw inside Effect.gen or the lockfile writer callback escapes as an uncaught defect rather than a typed Effect failure, so a missing-plugin request crashes with a 500 instead of returning the intended plugin-not-found PluginManagementError. Consider yielding the error as an Effect.fail so it propagates through the typed error channel.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 206:
`installedEntry` throws a `PluginManagementError` synchronously inside `Effect.gen` blocks (e.g. `beginUpgrade`, `setEnabled`, `uninstall`, and the `updatePlugin` callback). A synchronous `throw` inside `Effect.gen` or the lockfile writer callback escapes as an uncaught defect rather than a typed `Effect` failure, so a missing-plugin request crashes with a 500 instead of returning the intended `plugin-not-found` `PluginManagementError`. Consider yielding the error as an `Effect.fail` so it propagates through the typed error channel.
| export class PluginLockfileCorruptError extends Schema.TaggedErrorClass<PluginLockfileCorruptError>()( | ||
| "PluginLockfileCorruptError", | ||
| { path: Schema.String, cause: Schema.Defect() }, | ||
| ) { | ||
| // Derive the message from the stable `path` only — never from the stringified | ||
| // decode error — so the wrapper cannot leak corrupt lockfile contents. The | ||
| // underlying failure is preserved on `cause` (Schema.Defect) for diagnostics. | ||
| override get message(): string { | ||
| return `Plugin lockfile at ${this.path} is corrupt.`; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 High plugins/PluginLockfileStore.ts:37
PluginLockfileCorruptError stores the decode failure in cause: Schema.Defect(), and Schema.Defect() encodes Error values to JSON including their message and nested cause. When this error reaches PluginInstaller.lockfileError(), the decode failure — whose message contains the raw lockfile payload — is placed under PluginManagementError.data.cause and sent to clients. The sanitized message getter does not prevent the leak because the original corrupt lockfile contents still travel inside cause. Consider omitting cause from this error or replacing it with a non-leaking placeholder so corrupt lockfile contents are not serialized into the RPC response.
| export class PluginLockfileCorruptError extends Schema.TaggedErrorClass<PluginLockfileCorruptError>()( | |
| "PluginLockfileCorruptError", | |
| { path: Schema.String, cause: Schema.Defect() }, | |
| ) { | |
| // Derive the message from the stable `path` only — never from the stringified | |
| // decode error — so the wrapper cannot leak corrupt lockfile contents. The | |
| // underlying failure is preserved on `cause` (Schema.Defect) for diagnostics. | |
| override get message(): string { | |
| return `Plugin lockfile at ${this.path} is corrupt.`; | |
| } | |
| } | |
| export class PluginLockfileCorruptError extends Schema.TaggedErrorClass<PluginLockfileCorruptError>()( | |
| "PluginLockfileCorruptError", | |
| { path: Schema.String }, | |
| ) { | |
| // Derive the message from the stable `path` only — never from the stringified | |
| // decode error — so the wrapper cannot leak corrupt lockfile contents. | |
| override get message(): string { | |
| return `Plugin lockfile at ${this.path} is corrupt.`; | |
| } | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around lines 37-47:
`PluginLockfileCorruptError` stores the decode failure in `cause: Schema.Defect()`, and `Schema.Defect()` encodes `Error` values to JSON including their `message` and nested `cause`. When this error reaches `PluginInstaller.lockfileError()`, the decode failure — whose message contains the raw lockfile payload — is placed under `PluginManagementError.data.cause` and sent to clients. The sanitized `message` getter does not prevent the leak because the original corrupt lockfile contents still travel inside `cause`. Consider omitting `cause` from this error or replacing it with a non-leaking placeholder so corrupt lockfile contents are not serialized into the RPC response.
| ), | ||
| ); | ||
|
|
||
| function validateRegistration( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:205
validateRegistration dereferences registration.rpc and registration.streams without first checking that registration is an object. Plugin modules are untrusted JS, so register() can return null, a string, or any other non-object. When it does, this function throws a TypeError instead of returning a typed PluginRegistrationError, so the invalid registration crashes through the defect path rather than being rejected as intended. Add a guard that fails with PluginRegistrationError when registration is not a non-null object.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 205:
`validateRegistration` dereferences `registration.rpc` and `registration.streams` without first checking that `registration` is an object. Plugin modules are untrusted JS, so `register()` can return `null`, a string, or any other non-object. When it does, this function throws a `TypeError` instead of returning a typed `PluginRegistrationError`, so the invalid registration crashes through the defect path rather than being rejected as intended. Add a guard that fails with `PluginRegistrationError` when `registration` is not a non-null object.
Clients could send thread.create with owner=plugin:* via the client wire union and forge a hidden thread. Add an owner-less ClientThreadCreateCommand and use it in the client-facing unions; owner stays server-injected. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
PluginHost lifecycle (discover, activate, deactivate), lockfile store, per-plugin migrations (034), module loader + resolve hooks, plugin manifest contract, and the @t3tools/plugin-sdk server package. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Plugin RPC dispatcher + catalog wiring over ws, plugin-aware auth scope model (plugin:<id>:read|operate), session/pairing-grant scope plumbing, and the client-runtime authorization surface for plugin scopes. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Database, secrets, terminals, environments-read, projections-read, source-control, text-generation, and plugin HTTP route facades gated by per-plugin scopes, plus the projection read-model support they sit on. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Durable agent turn dispatch/observe surface and vcs (git) capability facade for plugins, wired into the plugin host API and server registry. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Serve plugin web bundles + import-map host shims from the server, load and render plugin UI (routes, sidebar sections, settings pages) in the web app via PluginUiHost, and ship the @t3tools/plugin-sdk-web package. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Marketplace catalog/source management, staged install/upgrade/uninstall flows over ws RPC, the plugins settings UI, the hello-board fixture plugin, and plugin authoring docs. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Filesystem (workspace-grant-scoped) and httpClient (SSRF-guarded egress) capabilities with palette wiring, plus the capability interface expansions they unblock (sourceControl/vcs PR-loop parity, projections getMessageById, textGeneration board proposals) and the review-tribunal hardening packets: marketplace/install ingestion, web host surfaces, capability facades, and git/gh stderr handling. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
ccdwyer
force-pushed
the
wb/08-fs-http-and-hardening
branch
from
79264b6 to
4d1a78b
Compare
| detail: `object name must start with ${input.prefix}`, | ||
| }); | ||
| } | ||
| if (entry.type !== "trigger" && entry.type !== "view") continue; |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:157
validateMigrationObjects only checks trigger/view SQL bodies for references to core tables. An index on a core table, e.g. CREATE INDEX p_my_plugin_idx ON core_items(id), passes the prefix check at line 149 and then entry.type !== "trigger" && entry.type !== "view" skips it entirely, so the migration is accepted even though it modifies a core table's schema. Consider extracting the referenced table name from index/trigger/view DDL and verifying it belongs to the plugin's namespace.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 157:
`validateMigrationObjects` only checks trigger/view SQL bodies for references to core tables. An index on a core table, e.g. `CREATE INDEX p_my_plugin_idx ON core_items(id)`, passes the prefix check at line 149 and then `entry.type !== "trigger" && entry.type !== "view"` skips it entirely, so the migration is accepted even though it modifies a core table's schema. Consider extracting the referenced table name from index/trigger/view DDL and verifying it belongs to the plugin's namespace.
| Effect.suspend(() => { | ||
| const value = definition.register(hostApi); | ||
| if (Effect.isEffect(value)) return value; | ||
| if (isPromiseLike(value)) return Effect.promise(() => value as Promise<PluginRegistration>); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:187
resolveRegistration wraps a promise-based definition.register() in Effect.promise(() => value), but the arrow function ignores the AbortSignal that Effect.promise passes to its callback. When Effect.timeout(registrationTimeout()) interrupts activation after 30s, the host marks the plugin failed and moves on, but the underlying register() promise continues running in the background with no signal — it can keep mutating host state or starting work outside the plugin scope, and a retry can duplicate those side effects. Pass the signal through so the plugin can observe interruption and abort.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 187:
`resolveRegistration` wraps a promise-based `definition.register()` in `Effect.promise(() => value)`, but the arrow function ignores the `AbortSignal` that `Effect.promise` passes to its callback. When `Effect.timeout(registrationTimeout())` interrupts activation after 30s, the host marks the plugin failed and moves on, but the underlying `register()` promise continues running in the background with no signal — it can keep mutating host state or starting work outside the plugin scope, and a retry can duplicate those side effects. Pass the signal through so the plugin can observe interruption and abort.
| } | ||
|
|
||
| function shouldResolveFromHost(specifier: string, parentURL: string | undefined): boolean { | ||
| if (!parentURL || !pluginsRootUrl || !parentURL.startsWith(pluginsRootUrl)) return false; |
There was a problem hiding this comment.
🟡 Medium plugins/pluginResolveHooks.ts:18
shouldResolveFromHost compares parentURL against pluginsRootUrl as a literal string prefix, but plugin modules are loaded via fs.realPath(pluginDir), so a symlinked plugins directory produces a parentURL that no longer starts with pluginsRootUrl. The prefix check then returns false, so effect/effect//@t3tools/plugin-sdk imports from those plugins are never re-anchored to the host — they resolve their own copies or fail instead of sharing host singletons. Consider resolving pluginsRootUrl to its realpath (or comparing realpaths) so the prefix check matches the URLs Node actually uses.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/pluginResolveHooks.ts around line 18:
`shouldResolveFromHost` compares `parentURL` against `pluginsRootUrl` as a literal string prefix, but plugin modules are loaded via `fs.realPath(pluginDir)`, so a symlinked plugins directory produces a `parentURL` that no longer starts with `pluginsRootUrl`. The prefix check then returns `false`, so `effect`/`effect/`/`@t3tools/plugin-sdk` imports from those plugins are never re-anchored to the host — they resolve their own copies or fail instead of sharing host singletons. Consider resolving `pluginsRootUrl` to its realpath (or comparing realpaths) so the prefix check matches the URLs Node actually uses.
|
|
||
| const isZeroBlock = (block: Uint8Array) => block.every((byte) => byte === 0); | ||
|
|
||
| const cString = (bytes: Uint8Array) => { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:161
cString calls .trim() on the extracted string, stripping leading and trailing spaces that are part of NUL-terminated tar header fields. A tar member named manifest.json (with a trailing space) is decoded as manifest.json, bypassing validateRelativeArchivePath's exact-string allowlist — the entry matches the allowlist and gets written even though the true pathname is not allowed. Remove the .trim() call so the full stored pathname is preserved.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 161:
`cString` calls `.trim()` on the extracted string, stripping leading and trailing spaces that are part of NUL-terminated tar header fields. A tar member named `manifest.json ` (with a trailing space) is decoded as `manifest.json`, bypassing `validateRelativeArchivePath`'s exact-string allowlist — the entry matches the allowlist and gets written even though the true pathname is not allowed. Remove the `.trim()` call so the full stored pathname is preserved.
| readonly service: PluginServiceDescriptor; | ||
| }) => | ||
| input.service.run({ pluginId: input.pluginId, logger: input.logger }).pipe( | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:442
startService catches every cause from service.run, including the interrupt that Scope.close sends to stop the forkScoped fiber. Instead of letting the interruption terminate the fiber, Effect.catchCause swallows it, logs it as a failure, and Effect.repeat restarts the service. During deactivatePlugin or host teardown, closing the plugin scope interrupts the service fiber, which then loops back and starts again, preventing the fiber from ever finishing — so Scope.close waits indefinitely and disable/shutdown hangs. Consider re-raising the cause when it contains an interrupt (e.g. Cause.hasInterrupts(cause)) so the fiber terminates instead of retrying.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 442:
`startService` catches every cause from `service.run`, including the interrupt that `Scope.close` sends to stop the `forkScoped` fiber. Instead of letting the interruption terminate the fiber, `Effect.catchCause` swallows it, logs it as a failure, and `Effect.repeat` restarts the service. During `deactivatePlugin` or host teardown, closing the plugin scope interrupts the service fiber, which then loops back and starts again, preventing the fiber from ever finishing — so `Scope.close` waits indefinitely and disable/shutdown hangs. Consider re-raising the cause when it contains an interrupt (e.g. `Cause.hasInterrupts(cause)`) so the fiber terminates instead of retrying.
| createdAt: nowIso(), | ||
| }) | ||
| .pipe( | ||
| Effect.tapError(() => |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:599
The Effect.tapError cleanup at line 605 deletes the turnAliases entry for every thread.turn.start dispatch failure. OrchestrationEngine.dispatch can reject after the command was already committed (e.g. a late transport or projection error), so the turn and its pendingMessageId still exist. On a retry with the same commandId, the deleted alias is gone, and if the retry omits request.messageId the code rebuilds it with messageIdForCommand(commandId) instead of the id that was actually persisted. A later awaitTurn(turnId) correlates via the alias's messageId against the projection's pendingMessageId, so it never matches and polls until timeout. Consider removing the unconditional alias deletion (or only deleting when the dispatch was definitively rejected before commit).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 599:
The `Effect.tapError` cleanup at line 605 deletes the `turnAliases` entry for *every* `thread.turn.start` dispatch failure. `OrchestrationEngine.dispatch` can reject after the command was already committed (e.g. a late transport or projection error), so the turn and its `pendingMessageId` still exist. On a retry with the same `commandId`, the deleted alias is gone, and if the retry omits `request.messageId` the code rebuilds it with `messageIdForCommand(commandId)` instead of the id that was actually persisted. A later `awaitTurn(turnId)` correlates via the alias's `messageId` against the projection's `pendingMessageId`, so it never matches and polls until timeout. Consider removing the unconditional alias deletion (or only deleting when the dispatch was definitively rejected before commit).
| const closeHandle = (handle: TerminalSessionHandle, deleteHistory?: boolean) => | ||
| input.manager | ||
| .close({ | ||
| threadId: handle.threadId, | ||
| terminalId: handle.terminalId, | ||
| ...(deleteHistory === undefined ? {} : { deleteHistory }), | ||
| }) | ||
| .pipe(Effect.ensuring(Effect.sync(() => live.delete(handle.terminalId)))); |
There was a problem hiding this comment.
🟡 Medium capabilities/TerminalsCapability.ts:66
closeHandle runs live.delete(handle.terminalId) inside Effect.ensuring unconditionally, so when manager.close fails the entry is still removed from live. A plugin can call kill with a forged handle like { threadId: "plugin:<me>:bogus", terminalId: "run-1" } — requireOwnedHandle accepts it because the prefix matches, manager.close fails to find that session, but the ensuring block deletes run-1 from live anyway. The real terminal keeps running and shutdown skips it, leaking the PTY/process. Consider only removing from live when manager.close succeeds.
const closeHandle = (handle: TerminalSessionHandle, deleteHistory?: boolean) =>
input.manager
.close({
threadId: handle.threadId,
terminalId: handle.terminalId,
...(deleteHistory === undefined ? {} : { deleteHistory }),
})
- .pipe(Effect.ensuring(Effect.sync(() => live.delete(handle.terminalId))));
+ .pipe(Effect.tap(() => Effect.sync(() => live.delete(handle.terminalId))));🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/TerminalsCapability.ts around lines 66-73:
`closeHandle` runs `live.delete(handle.terminalId)` inside `Effect.ensuring` unconditionally, so when `manager.close` fails the entry is still removed from `live`. A plugin can call `kill` with a forged handle like `{ threadId: "plugin:<me>:bogus", terminalId: "run-1" }` — `requireOwnedHandle` accepts it because the prefix matches, `manager.close` fails to find that session, but the `ensuring` block deletes `run-1` from `live` anyway. The real terminal keeps running and `shutdown` skips it, leaking the PTY/process. Consider only removing from `live` when `manager.close` succeeds.
| // outright rather than pretend they are covered. | ||
| // database_list always reports "main" (and "temp" once the temp | ||
| // schema exists); anything else is an ATTACHed database. | ||
| const databases = yield* sql<{ readonly name: string }>`PRAGMA database_list`; |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:245
The ATTACH and TEMP guards only inspect database state after migration.up finishes. A migration can ATTACH DATABASE, modify the attached database, then DETACH before returning, and PRAGMA database_list at line 245 will show only main/temp. Similarly, a migration can create and drop a TEMP object within the same migration, leaving sqlite_temp_master clean at line 262. Both cases pass the guards despite the operations being explicitly forbidden, silently bypassing the migration hardening. Consider intercepting ATTACH and CREATE TEMP at the SQL statement level rather than diffing post-migration snapshots.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 245:
The `ATTACH` and `TEMP` guards only inspect database state after `migration.up` finishes. A migration can `ATTACH DATABASE`, modify the attached database, then `DETACH` before returning, and `PRAGMA database_list` at line 245 will show only `main`/`temp`. Similarly, a migration can create and drop a TEMP object within the same migration, leaving `sqlite_temp_master` clean at line 262. Both cases pass the guards despite the operations being explicitly forbidden, silently bypassing the migration hardening. Consider intercepting `ATTACH` and `CREATE TEMP` at the SQL statement level rather than diffing post-migration snapshots.
| const clampPositiveInteger = (value: number | undefined, fallback: number, hardMax: number) => { | ||
| if (value === undefined) return fallback; | ||
| if (!Number.isFinite(value) || value <= 0) return fallback; | ||
| return Math.min(Math.floor(value), hardMax); | ||
| }; |
There was a problem hiding this comment.
🟡 Medium capabilities/HttpClientCapability.ts:91
clampPositiveInteger checks value <= 0 before flooring, so a fractional positive value like 0.5 passes the check and Math.floor turns it into 0. With requestInput.timeoutMs = 0.5, timeoutMs becomes 0 and Effect.timeoutOrElse(Duration.millis(0)) times out immediately instead of honoring a positive timeout. Likewise, maxResponseBytes = 0.5 becomes a zero-byte cap, so every non-empty response fails. Consider treating values below 1 as fallback as well.
| const clampPositiveInteger = (value: number | undefined, fallback: number, hardMax: number) => { | |
| if (value === undefined) return fallback; | |
| if (!Number.isFinite(value) || value <= 0) return fallback; | |
| return Math.min(Math.floor(value), hardMax); | |
| }; | |
| const clampPositiveInteger = (value: number | undefined, fallback: number, hardMax: number) => { | |
| if (value === undefined) return fallback; | |
| if (!Number.isFinite(value) || value < 1) return fallback; | |
| return Math.min(Math.floor(value), hardMax); | |
| }; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/HttpClientCapability.ts around lines 91-95:
`clampPositiveInteger` checks `value <= 0` before flooring, so a fractional positive value like `0.5` passes the check and `Math.floor` turns it into `0`. With `requestInput.timeoutMs = 0.5`, `timeoutMs` becomes `0` and `Effect.timeoutOrElse(Duration.millis(0))` times out immediately instead of honoring a positive timeout. Likewise, `maxResponseBytes = 0.5` becomes a zero-byte cap, so every non-empty response fails. Consider treating values below `1` as `fallback` as well.
| const embeddedV4 = (bytes: ReadonlyArray<number>): ReadonlyArray<number> | null => { | ||
| const zerosThrough = (length: number): boolean => | ||
| bytes.slice(0, length).every((byte) => byte === 0); | ||
| if (zerosThrough(10) && bytes[10] === 0xff && bytes[11] === 0xff) { | ||
| return bytes.slice(12, 16); | ||
| } | ||
| if ( | ||
| bytes[0] === 0x00 && | ||
| bytes[1] === 0x64 && | ||
| bytes[2] === 0xff && | ||
| bytes[3] === 0x9b && | ||
| bytes.slice(4, 12).every((byte) => byte === 0) | ||
| ) { | ||
| return bytes.slice(12, 16); | ||
| } |
There was a problem hiding this comment.
🟠 High plugins/OutboundUrlValidator.ts:147
embeddedV4 recognizes the well-known NAT64 prefix 64:ff9b::/96 but misses the local-use translation prefix 64:ff9b:1::/48, which IANA marks as not globally reachable. An address like 64:ff9b:1::0a00:0001 embeds 10.0.0.1, yet isPrivateV6 treats it as public and the validator allows outbound requests to that private IPv4 host — an SSRF bypass on networks using local NAT64. Consider matching 64:ff9b:1::/48 (bytes 00:64:ff:9b:00:01) in embeddedV4 so embedded private IPv4 addresses are blocked.
const embeddedV4 = (bytes: ReadonlyArray<number>): ReadonlyArray<number> | null => {
const zerosThrough = (length: number): boolean =>
bytes.slice(0, length).every((byte) => byte === 0);
if (zerosThrough(10) && bytes[10] === 0xff && bytes[11] === 0xff) {
return bytes.slice(12, 16);
}
if (
bytes[0] === 0x00 &&
bytes[1] === 0x64 &&
bytes[2] === 0xff &&
bytes[3] === 0x9b &&
- bytes.slice(4, 12).every((byte) => byte === 0)
+ bytes.slice(6, 12).every((byte) => byte === 0) &&
+ bytes[4] === 0x00 &&
+ (bytes[5] === 0x00 || bytes[5] === 0x01)
) {
return bytes.slice(12, 16);
}
if (zerosThrough(12)) return bytes.slice(12, 16);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/OutboundUrlValidator.ts around lines 147-161:
`embeddedV4` recognizes the well-known NAT64 prefix `64:ff9b::/96` but misses the local-use translation prefix `64:ff9b:1::/48`, which IANA marks as not globally reachable. An address like `64:ff9b:1::0a00:0001` embeds `10.0.0.1`, yet `isPrivateV6` treats it as public and the validator allows outbound requests to that private IPv4 host — an SSRF bypass on networks using local NAT64. Consider matching `64:ff9b:1::/48` (bytes `00:64:ff:9b:00:01`) in `embeddedV4` so embedded private IPv4 addresses are blocked.
1 participant
What Changed
The most security-sensitive egress capabilities —
filesystem(workspace-scoped, realpath containment,O_NOFOLLOW) andhttpClient(HTTPS-only, all-address DNS lookup + special-use/metadata blocks, DNS-address pinning to defeat rebinding, response/body/timeout caps) — plus a few generic-additive capability methods that serve the example plugin (vcs.diffRefToWorkingTree,textGeneration.generateBoardProposalunder a provably no-tool posture, GitHub PR ops). This is the top of the stack — merged after parts 1–7, it completes the plugin system.Why
Part 8 of 8 of a runtime plugin system (see #3725 for the overview). Builds on #3731. Filesystem and outbound HTTP are the capabilities most likely to be abused by hostile data, so they ship last, most-hardened, and behind explicit manifest consent. The example plugin (a 46-RPC workflow engine) that proves the whole API set lives at https://github.com/ccdwyer/workflow-boards-plugin.
Review — four-model tribunal (GPT-5.5 · Grok · GLM-5.2 · Fable)
Fable/Grok verified the SSRF blocklist, DNS pin, and filesystem containment have no outright bypass.
git/ghstderr leaked credentials to the web client + Sentry (MUST, 3 reviewers): cappedstderron the RPC error types wasn't redacted → moved off the wire into a private getter (in-process readable, invisible to Schema/JSON/Sentry).lookupoverride returned a scalar while Node ≥20'sautoSelectFamilyexpects an array — so the anti-rebinding pin silently didn't apply to hostnames. Fixed + a real-transport regression test.gh pr checksexit 1 read as "no checks" (MUST): an auth failure looked like passing CI → fails loudly. No wall-clockhttpClientdeadline → end-to-endEffect.timeout+ DNS deadline. Codexno-toolposture empirically re-verified and hardened (--disable shell_tool+web_search=false). Filesystem parent-chain TOCTOU documented as a known full-trust limitation.UI Changes
N/A — server only.
Checklist
https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk