From f7e1bcf87f4fe4b5c7206e2764be1c1e252dc2a6 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 1 Apr 2026 11:37:27 +0200 Subject: [PATCH] :bug: Handle plugin errors gracefully without crashing the UI (#8810) * :bug: Handle plugin errors gracefully without crashing the UI Plugin errors (like 'Set is not a constructor') were propagating to the global error handler and showing the exception page. This fix: - Uses a WeakMap to track plugin errors (works in SES hardened environment) - Wraps setTimeout/setInterval handlers to mark errors and re-throw them - Frontend global handler checks isPluginError and logs to console Plugin errors are now logged to console with 'Plugin Error' prefix but don't crash the main application or show the exception page. Signed-off-by: AI Agent * :sparkles: Improved handling of plugin errors on initialization * :sparkles: Fix test and linter --------- Signed-off-by: AI Agent Co-authored-by: alonso.torres --- frontend/src/app/main/data/plugins.cljs | 68 ++++++++------- frontend/src/app/main/errors.cljs | 87 ++++++++++++++----- frontend/src/app/plugins.cljs | 3 + .../poc-state-plugin/src/app/app.component.ts | 2 +- plugins/libs/plugins-runtime/src/index.ts | 3 + .../src/lib/create-plugin.spec.ts | 7 +- .../plugins-runtime/src/lib/create-plugin.ts | 8 +- .../plugins-runtime/src/lib/create-sandbox.ts | 79 ++++++++++++++++- .../src/lib/load-plugin.spec.ts | 13 ++- .../plugins-runtime/src/lib/load-plugin.ts | 7 +- .../plugins-runtime/src/lib/plugin-manager.ts | 7 ++ 11 files changed, 215 insertions(+), 69 deletions(-) diff --git a/frontend/src/app/main/data/plugins.cljs b/frontend/src/app/main/data/plugins.cljs index e04aecbf7c..b091518b67 100644 --- a/frontend/src/app/main/data/plugins.cljs +++ b/frontend/src/app/main/data/plugins.cljs @@ -7,12 +7,14 @@ (ns app.main.data.plugins (:require [app.common.data.macros :as dm] + [app.common.exceptions :as ex] [app.common.files.changes-builder :as pcb] [app.common.time :as ct] [app.main.data.changes :as dch] [app.main.data.event :as ev] [app.main.data.modal :as modal] [app.main.data.notifications :as ntf] + [app.main.errors :as errors] [app.main.store :as st] [app.plugins.flags :as pflag] [app.plugins.register :as preg] @@ -20,7 +22,8 @@ [app.util.http :as http] [app.util.i18n :as i18n :refer [tr]] [beicon.v2.core :as rx] - [potok.v2.core :as ptk])) + [potok.v2.core :as ptk] + [promesa.core :as p])) (defn save-plugin-permissions-peek [id permissions] @@ -54,40 +57,45 @@ (defn start-plugin! [{:keys [plugin-id name version description host code permissions allow-background]} ^js extensions] - (.ɵloadPlugin - ^js ug/global - #js {:pluginId plugin-id - :name name - :version version - :description description - :host host - :code code - :allowBackground (boolean allow-background) - :permissions (apply array permissions)} - nil - extensions)) + (-> (.ɵloadPlugin + ^js ug/global + #js {:pluginId plugin-id + :name name + :version version + :description description + :host host + :code code + :allowBackground (boolean allow-background) + :permissions (apply array permissions)} + nil + extensions) + + (p/catch (fn [cause] + (ex/print-throwable cause :prefix "Plugin Error") + (errors/flash :cause cause :type :handled))))) (defn- load-plugin! [{:keys [plugin-id name version description host code icon permissions]}] - (try - (st/emit! (pflag/clear plugin-id) - (save-current-plugin plugin-id)) + (st/emit! (pflag/clear plugin-id) + (save-current-plugin plugin-id)) - (.ɵloadPlugin ^js ug/global - #js {:pluginId plugin-id - :name name - :description description - :version version - :host host - :code code - :icon icon - :permissions (apply array permissions)} - (fn [] - (st/emit! (remove-current-plugin plugin-id)))) + (-> (.ɵloadPlugin + ^js ug/global + #js {:pluginId plugin-id + :name name + :description description + :version version + :host host + :code code + :icon icon + :permissions (apply array permissions)} + (fn [] + (st/emit! (remove-current-plugin plugin-id)))) - (catch :default e - (st/emit! (remove-current-plugin plugin-id)) - (.error js/console "Error" e)))) + (p/catch (fn [cause] + (st/emit! (remove-current-plugin plugin-id)) + (ex/print-throwable cause :prefix "Plugin Error") + (errors/flash :cause cause :type :handled))))) (defn open-plugin! [{:keys [url] :as manifest} user-can-edit?] diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 4eb52b5b35..49f4080abb 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -33,6 +33,16 @@ ;; Will contain last uncaught exception (def last-exception nil) +(defn is-plugin-error? + "This is a placeholder that always return false. It will be + overwritten when plugin system is initialized. This works this way + because we can't import plugins here because plugins requries full + DOM. + + This placeholder is set on app.plugins/initialize event" + [_] + false) + ;; --- Stale-asset error detection and auto-reload ;; ;; When the browser loads JS modules from different builds (e.g. shared.js from @@ -387,6 +397,15 @@ (and (string? stack) (str/includes? stack "posthog")))) + ;; Check if the error is marked as originating from plugin code. + ;; The plugin runtime tracks plugin errors in a WeakMap, which works + ;; even in SES hardened environments where error objects may be frozen. + (from-plugin? [cause] + (try + (is-plugin-error? cause) + (catch :default _ + false))) + (is-ignorable-exception? [cause] (let [message (ex-message cause)] (or (from-extension? cause) @@ -405,32 +424,56 @@ (on-unhandled-error [event] (.preventDefault ^js event) (when-let [cause (unchecked-get event "error")] - (when-not (is-ignorable-exception? cause) - (if (stale-asset-error? cause) - (cf/throttled-reload :reason (ex-message cause)) - (let [data (ex-data cause) - type (get data :type)] - (set! last-exception cause) - (if (= :wasm-error type) - (on-error cause) - (do - (ex/print-throwable cause :prefix "Uncaught Exception") - (ts/asap #(flash :cause cause :type :unhandled))))))))) + (cond + (stale-asset-error? cause) + (cf/throttled-reload :reason (ex-message cause)) + + ;; Plugin errors: log to console and ignore + (from-plugin? cause) + (ex/print-throwable cause :prefix "Plugin Error") + + ;; Other ignorable exceptions: ignore silently + (is-ignorable-exception? cause) + nil + + ;; All other errors: show exception page + :else + + (let [data (ex-data cause) + type (get data :type)] + (set! last-exception cause) + (if (= :wasm-error type) + (on-error cause) + (do + (ex/print-throwable cause :prefix "Uncaught Exception") + (ts/asap #(flash :cause cause :type :unhandled)))))))) (on-unhandled-rejection [event] (.preventDefault ^js event) (when-let [cause (unchecked-get event "reason")] - (when-not (is-ignorable-exception? cause) - (if (stale-asset-error? cause) - (cf/throttled-reload :reason (ex-message cause)) - (let [data (ex-data cause) - type (get data :type)] - (set! last-exception cause) - (if (= :wasm-error type) - (on-error cause) - (do - (ex/print-throwable cause :prefix "Uncaught Rejection") - (ts/asap #(flash :cause cause :type :unhandled)))))))))] + (cond + (stale-asset-error? cause) + (cf/throttled-reload :reason (ex-message cause)) + + ;; Plugin errors: log to console and ignore + (from-plugin? cause) + (ex/print-throwable cause :prefix "Plugin Error") + + ;; Other ignorable exceptions: ignore silently + (is-ignorable-exception? cause) + nil + + ;; All other errors: show exception page + :else + (let [data (ex-data cause) + type (get data :type)] + (set! last-exception cause) + (if (= :wasm-error type) + (on-error cause) + (do + (ex/print-throwable cause :prefix "Uncaught Rejection") + (ts/asap #(flash :cause cause :type :unhandled))))))))] + (.addEventListener g/window "error" on-unhandled-error) (.addEventListener g/window "unhandledrejection" on-unhandled-rejection) diff --git a/frontend/src/app/plugins.cljs b/frontend/src/app/plugins.cljs index 36f31ff110..3ce022753e 100644 --- a/frontend/src/app/plugins.cljs +++ b/frontend/src/app/plugins.cljs @@ -8,6 +8,7 @@ "RPC for plugins runtime." (:require ["@penpot/plugins-runtime" :as runtime] + [app.main.errors :as errors] [app.main.features :as features] [app.main.store :as st] [app.plugins.api :as api] @@ -30,6 +31,8 @@ (ptk/reify ::initialize ptk/WatchEvent (watch [_ _ stream] + (set! errors/is-plugin-error? runtime/isPluginError) + (->> stream (rx/filter (ptk/type? ::features/initialize)) (rx/observe-on :async) diff --git a/plugins/apps/poc-state-plugin/src/app/app.component.ts b/plugins/apps/poc-state-plugin/src/app/app.component.ts index 8996e1eb83..0eae67b448 100644 --- a/plugins/apps/poc-state-plugin/src/app/app.component.ts +++ b/plugins/apps/poc-state-plugin/src/app/app.component.ts @@ -393,7 +393,7 @@ export class AppComponent { } #startDownload(name: string, data: Uint8Array) { - const blob = new Blob([data], { type: 'application/octet-stream' }); + const blob = new Blob([data as any], { type: 'application/octet-stream' }); // We need to start a download with this URL const downloadURL = URL.createObjectURL(blob); diff --git a/plugins/libs/plugins-runtime/src/index.ts b/plugins/libs/plugins-runtime/src/index.ts index 2516cfb2f2..a38203471d 100644 --- a/plugins/libs/plugins-runtime/src/index.ts +++ b/plugins/libs/plugins-runtime/src/index.ts @@ -8,6 +8,9 @@ import { ɵunloadPlugin, } from './lib/load-plugin.js'; +// Export the plugin error checker so the frontend can identify plugin errors +export { isPluginError } from './lib/create-sandbox.js'; + import type { Context } from '@penpot/plugin-types'; console.log('%c[PLUGINS] Loading plugin system', 'color: #008d7c'); diff --git a/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts b/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts index 70d9e74ad4..29d0b6d691 100644 --- a/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts @@ -11,6 +11,7 @@ vi.mock('./plugin-manager.js', () => ({ vi.mock('./create-sandbox.js', () => ({ createSandbox: vi.fn(), + markPluginError: vi.fn(), })); describe('createPlugin', () => { @@ -116,7 +117,11 @@ describe('createPlugin', () => { throw new Error('Evaluation error'); }); - await createPlugin(mockContext, manifest, onCloseCallback); + try { + await createPlugin(mockContext, manifest, onCloseCallback); + } catch (err) { + expect.assert(err); + } expect(mockPluginManager.close).toHaveBeenCalled(); }); diff --git a/plugins/libs/plugins-runtime/src/lib/create-plugin.ts b/plugins/libs/plugins-runtime/src/lib/create-plugin.ts index f30977d5aa..fd7bc6a0a8 100644 --- a/plugins/libs/plugins-runtime/src/lib/create-plugin.ts +++ b/plugins/libs/plugins-runtime/src/lib/create-plugin.ts @@ -1,7 +1,7 @@ import type { Context } from '@penpot/plugin-types'; import type { Manifest } from './models/manifest.model.js'; import { createPluginManager } from './plugin-manager.js'; -import { createSandbox } from './create-sandbox.js'; +import { createSandbox, markPluginError } from './create-sandbox.js'; export async function createPlugin( context: Context, @@ -13,9 +13,9 @@ export async function createPlugin( try { sandbox.evaluate(); } catch (error) { - console.error(error); - + markPluginError(error); plugin.close(); + throw error; } }; @@ -33,7 +33,7 @@ export async function createPlugin( const sandbox = createSandbox(plugin, apiExtensions); - evaluateSandbox(); + await evaluateSandbox(); return { plugin, diff --git a/plugins/libs/plugins-runtime/src/lib/create-sandbox.ts b/plugins/libs/plugins-runtime/src/lib/create-sandbox.ts index 01f132548b..292ae49183 100644 --- a/plugins/libs/plugins-runtime/src/lib/create-sandbox.ts +++ b/plugins/libs/plugins-runtime/src/lib/create-sandbox.ts @@ -3,6 +3,61 @@ import type { createPluginManager } from './plugin-manager'; import { createApi } from './api'; import { ses } from './ses.js'; +/** + * WeakMap used to track errors originating from plugin code. + * Using a WeakMap is safer than extending error objects because: + * 1. It works even if the error object is frozen (SES hardened environment) + * 2. It doesn't require modifying the error object + * 3. It allows garbage collection of error objects when no longer referenced + */ +const pluginErrors = new WeakMap(); + +/** + * Checks if an error originated from plugin code. + */ +export function isPluginError(error: unknown): boolean { + if (error !== null && typeof error === 'object') { + return pluginErrors.has(error as object); + } + return false; +} + +/** + * Marks an error as originating from plugin code. + * Uses a WeakMap so it works even if the error object is frozen. + */ +export function markPluginError(error: unknown): void { + if (error !== null && typeof error === 'object') { + pluginErrors.set(error as object, true); + } +} + +/** + * Wraps a handler function to mark any thrown errors as plugin errors. + * Errors are marked and re-thrown so they propagate to the global error handler, + * where they can be identified and handled appropriately. + */ +function wrapHandler unknown>( + handler: T, +): (...args: Parameters) => ReturnType { + return function (...args: Parameters) { + try { + const result = handler(...args); + // Handle async functions - mark errors in the returned promise + if (result instanceof Promise) { + return result.catch((error: unknown) => { + markPluginError(error); + throw error; + }) as ReturnType; + } + return result as ReturnType; + } catch (error) { + markPluginError(error); + throw error; + } + }; +} + export function createSandbox( plugin: Awaited>, apiExtensions?: object, @@ -58,9 +113,10 @@ export function createSandbox( fetch: ses.harden(safeFetch), setTimeout: ses.harden( (...[handler, timeout]: Parameters) => { - const timeoutId = setTimeout(() => { - handler(); - }, timeout); + const wrappedHandler = wrapHandler( + typeof handler === 'function' ? handler : () => {}, + ); + const timeoutId = setTimeout(wrappedHandler, timeout); plugin.timeouts.add(timeoutId); @@ -72,6 +128,23 @@ export function createSandbox( plugin.timeouts.delete(id); }), + setInterval: ses.harden( + (...[handler, interval]: Parameters) => { + const wrappedHandler = wrapHandler( + typeof handler === 'function' ? handler : () => {}, + ); + const intervalId = setInterval(wrappedHandler, interval); + + plugin.intervals.add(intervalId); + + return ses.safeReturn(intervalId); + }, + ) as typeof setInterval, + clearInterval: ses.harden((id: ReturnType) => { + clearInterval(id); + + plugin.intervals.delete(id); + }), /** * GLOBAL FUNCTIONS ACCESIBLE TO PLUGINS diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts index a46bbc8573..cff0f5e57a 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts @@ -19,6 +19,10 @@ vi.mock('./create-plugin', () => ({ createPlugin: vi.fn(), })); +vi.mock('./create-sandbox.js', () => ({ + markPluginError: vi.fn(), +})); + vi.mock('./ses.js', () => ({ ses: { harden: vi.fn().mockImplementation((obj) => obj), @@ -102,16 +106,17 @@ describe('plugin-loader', () => { }); it('should handle errors and close all plugins', async () => { - const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - vi.mocked(createPlugin).mockRejectedValue( new Error('Plugin creation failed'), ); - await loadPlugin(manifest); + try { + await loadPlugin(manifest); + } catch (err) { + expect.assert(err); + } expect(getPlugins()).toHaveLength(0); - expect(consoleSpy).toHaveBeenCalled(); }); it('should handle messages sent to plugins', async () => { diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin.ts index 990a9148a1..8a88f23050 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin.ts @@ -64,11 +64,10 @@ export const loadPlugin = async function ( }, apiExtensions, ); - plugins.push(plugin); } catch (error) { closeAllPlugins(); - console.error(error); + throw error; } }; @@ -77,12 +76,12 @@ export const ɵloadPlugin = async function ( closeCallback?: () => void, apiExtensions?: object, ) { - loadPlugin(manifest, closeCallback, apiExtensions); + await loadPlugin(manifest, closeCallback, apiExtensions); }; export const ɵloadPluginByUrl = async function (manifestUrl: string) { const manifest = await loadManifest(manifestUrl); - ɵloadPlugin(manifest); + await ɵloadPlugin(manifest); }; export const ɵunloadPlugin = function (id: Manifest['pluginId']) { diff --git a/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts b/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts index 5b0b1bddd0..0b4035794a 100644 --- a/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts +++ b/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts @@ -21,6 +21,7 @@ export async function createPluginManager( let modal: PluginModalElement | null = null; let uiMessagesCallbacks: ((message: unknown) => void)[] = []; const timeouts = new Set>(); + const intervals = new Set>(); const allowDownloads = !!manifest.permissions.find( (s) => s === 'allow:downloads', @@ -55,6 +56,9 @@ export async function createPluginManager( timeouts.forEach(clearTimeout); timeouts.clear(); + intervals.forEach(clearInterval); + intervals.clear(); + if (modal) { modal.removeEventListener('close', closePlugin); modal.remove(); @@ -151,6 +155,9 @@ export async function createPluginManager( get timeouts() { return timeouts; }, + get intervals() { + return intervals; + }, get code() { return code; },