fix: improve plugin safety -- error isolation, timeouts, retries, and redirects#517
fix: improve plugin safety -- error isolation, timeouts, retries, and redirects#517
Conversation
… redirects - Hook dependency cycles: log warning with affected plugin IDs instead of silently falling back to priority order - Hook timeout: clear timer on success to prevent resource leaks - Plugin route errors: log internal details, return generic message to client (was leaking error.message) - Cron one-shot retry: cap at 5 retries with exponential backoff (1m, 2m, 4m, 8m, 16m) then delete. Was retrying forever at 1m fixed. - Host allowlist: document that *.example.com matches bare example.com - Marketplace download: use redirect: manual to control redirect handling
Addresses adversarial review: - Marketplace downloadBundle: validate redirect target origin matches marketplace origin. Reject redirects to untrusted hosts (SSRF). Also use redirect: manual on follow-up fetch. - Cron retry count: validate _retryCount is a finite positive number. Prevents plugins from pre-seeding negative or infinite values.
🦋 Changeset detectedLatest commit: 788c113 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 788c113 | Apr 13 2026, 04:10 PM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
Pull request overview
This PR hardens the core plugin system by improving error isolation and reducing risk from untrusted behaviors (timeouts, retries, error message leakage, and redirect handling) across hooks, routes, cron, and marketplace bundle downloads.
Changes:
- Add hook dependency-cycle warnings and ensure hook timeout timers are always cleared.
- Reduce error detail leakage from plugin route handlers and improve server-side logging.
- Add bounded exponential backoff + retry limits for failing one-shot cron tasks, and validate marketplace bundle download redirects.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/plugins/routes.ts | Logs unknown route handler errors server-side and returns a generic 500 error message to clients. |
| packages/core/src/plugins/marketplace.ts | Switches to manual redirect handling and validates redirect targets stay on the marketplace origin. |
| packages/core/src/plugins/hooks.ts | Warns on hook dependency cycles/missing deps and clears timeout timers in a finally block. |
| packages/core/src/plugins/cron.ts | Adds exponential backoff and a max retry limit for one-shot cron tasks, storing retry metadata in task data. |
| packages/core/src/plugins/context.ts | Documents isHostAllowed wildcard behavior (including bare-domain matching for *.example.com). |
| .changeset/spotty-words-notice.md | Adds a patch changeset describing the plugin-safety improvements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…edirects, test update - Namespace cron retry metadata under __emdash.retryCount to avoid collisions with plugin-controlled data fields - Add Math.floor() to prevent fractional retry counts - Replace single-hop marketplace redirect with loop (max 5 hops) validating origin on each hop, matching createHttpAccess pattern - Fix [emdash] -> [hooks] log prefix inconsistency - Update route test to expect generic error message
c4e193a to
788c113
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Retry metadata is namespaced under __emdash to avoid collisions | ||
| // with plugin-controlled data fields. | ||
| const meta = | ||
| parsedData?.__emdash != null && typeof parsedData.__emdash === "object" | ||
| ? (parsedData.__emdash as Record<string, unknown>) | ||
| : undefined; | ||
| const raw = meta?.retryCount; |
There was a problem hiding this comment.
In strict TS, parsedData?.__emdash is inferred as unknown (because parsedData is Record<string, unknown>), so meta?.retryCount is a type error. Please narrow/cast __emdash to an object shape (e.g. Record<string, unknown> / { retryCount?: unknown }) before reading retryCount and building the updated JSON.
| let timer: ReturnType<typeof setTimeout>; | ||
| const timeoutPromise = new Promise<T>( | ||
| (_, reject) => | ||
| (timer = setTimeout(() => reject(new Error(`Hook timeout after ${timeout}ms`)), timeout)), | ||
| ); | ||
| try { | ||
| return await Promise.race([fn(), timeoutPromise]); | ||
| } finally { | ||
| clearTimeout(timer!); | ||
| } |
There was a problem hiding this comment.
executeWithTimeout uses timer! in clearTimeout(timer!). Consider typing timer as possibly undefined and clearing conditionally to avoid relying on a non-null assertion (and to keep the function safe if it’s refactored later).
| const marketplaceOrigin = new URL(this.baseUrl).origin; | ||
| const MAX_REDIRECTS = 5; | ||
| let response: Response; | ||
| try { | ||
| response = await fetch(bundleUrl, { | ||
| redirect: "follow", | ||
| }); | ||
| let currentUrl = bundleUrl; | ||
| response = await fetch(currentUrl, { redirect: "manual" }); | ||
|
|
||
| // Follow redirects manually, validating each target stays on the marketplace host | ||
| for (let i = 0; i < MAX_REDIRECTS; i++) { | ||
| if (response.status < 300 || response.status >= 400) break; | ||
|
|
||
| const location = response.headers.get("location"); | ||
| if (!location) break; | ||
|
|
||
| const target = new URL(location, currentUrl); | ||
| if (target.origin !== marketplaceOrigin) { | ||
| throw new MarketplaceError( | ||
| `Bundle download redirected to untrusted host: ${target.origin}`, | ||
| response.status, | ||
| "BUNDLE_REDIRECT_UNTRUSTED", | ||
| ); | ||
| } | ||
| currentUrl = target.href; | ||
| response = await fetch(currentUrl, { redirect: "manual" }); | ||
| } | ||
|
|
||
| // If still a redirect after MAX_REDIRECTS, fail explicitly | ||
| if (response.status >= 300 && response.status < 400) { | ||
| throw new MarketplaceError( | ||
| `Bundle download exceeded maximum redirects (${MAX_REDIRECTS})`, | ||
| response.status, | ||
| "BUNDLE_TOO_MANY_REDIRECTS", | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new manual redirect handling (untrusted redirect host + max redirect limit) isn’t covered by the existing marketplace client unit tests. Please add tests that (1) follow a same-origin redirect successfully, (2) reject a redirect to a different origin, and (3) fail after exceeding MAX_REDIRECTS.
| it("handles unknown errors from handler", async () => { | ||
| const plugin = createTestPlugin({ | ||
| routes: { | ||
| crash: { | ||
| handler: async () => { | ||
| throw new Error("Unexpected error"); | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| const handler = new PluginRouteHandler(plugin, createMockFactoryOptions()); | ||
|
|
||
| const result = await handler.invoke("crash", { | ||
| request: new Request("http://test.com"), | ||
| }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.status).toBe(500); | ||
| expect(result.error?.code).toBe("INTERNAL_ERROR"); | ||
| expect(result.error?.message).toContain("Unexpected error"); | ||
| expect(result.error?.message).toBe("An internal error occurred"); | ||
| }); |
There was a problem hiding this comment.
This test now triggers console.error from PluginRouteHandler when exercising the unknown-error path, which can make unit test output noisy. Consider stubbing console.error (e.g. via vi.spyOn(console, "error").mockImplementation(...)) within this test and restoring it afterwards.
… redirects (emdash-cms#517) * fix: improve plugin safety -- error isolation, timeouts, retries, and redirects - Hook dependency cycles: log warning with affected plugin IDs instead of silently falling back to priority order - Hook timeout: clear timer on success to prevent resource leaks - Plugin route errors: log internal details, return generic message to client (was leaking error.message) - Cron one-shot retry: cap at 5 retries with exponential backoff (1m, 2m, 4m, 8m, 16m) then delete. Was retrying forever at 1m fixed. - Host allowlist: document that *.example.com matches bare example.com - Marketplace download: use redirect: manual to control redirect handling * fix: validate marketplace redirect target host, clamp cron retry count Addresses adversarial review: - Marketplace downloadBundle: validate redirect target origin matches marketplace origin. Reject redirects to untrusted hosts (SSRF). Also use redirect: manual on follow-up fetch. - Cron retry count: validate _retryCount is a finite positive number. Prevents plugins from pre-seeding negative or infinite values. * chore: add changeset for plugin safety fixes * fix: address review feedback -- namespace retry metadata, multi-hop redirects, test update - Namespace cron retry metadata under __emdash.retryCount to avoid collisions with plugin-controlled data fields - Add Math.floor() to prevent fractional retry counts - Replace single-hop marketplace redirect with loop (max 5 hops) validating origin on each hop, matching createHttpAccess pattern - Fix [emdash] -> [hooks] log prefix inconsistency - Update route test to expect generic error message
What does this PR do?
Six targeted improvements to the plugin system's safety and error handling.
Hook dependency cycles: When circular or missing dependencies are detected during hook topological sort, log a warning with affected plugin IDs instead of silently falling back to priority order.
Hook timeout cleanup:
executeWithTimeoutnow clears the timer in afinallyblock when the hook completes, preventing timer resource leaks on Node.js.Plugin route error messages: Unknown errors no longer include
error.messagein the response body. Internal details are logged server-side; clients receive a generic "An internal error occurred" message.One-shot cron retry limit: Failed one-shot tasks now retry with exponential backoff (1m, 2m, 4m, 8m, 16m) and are deleted after 5 failures. Previously retried forever at a fixed 1-minute interval. Retry count is stored in the task data JSON and validated (clamped to 0+, must be finite) to prevent plugins from pre-seeding negative values.
Host allowlist documentation: Added JSDoc to
isHostAlloweddocumenting that*.example.commatches both subdomains and the bare domain.Marketplace bundle download: Changed from
redirect: "follow"toredirect: "manual"with host validation. Redirect targets must match the marketplace origin. Prevents SSRF via crafted redirect responses from compromised marketplace servers.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure