From 7900159568e63bc636735c3225ed1ac324922e4f Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 05:53:50 +0100 Subject: [PATCH 1/3] feat(admin): show "requires newer Etherpad" when installing incompatible plugin (#7763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old admins on out-of-date Etherpad installations get no feedback when they click Install on a plugin that needs a newer core. live-plugin-manager doesn't honor engines.node, and the admin UI dropped the error payload that adminplugins.ts already emits. This wires up an end-to-end signal: - pluginEngineCheck.ts: pure helper comparing a plugin's engines.node range against process.version, with a stable error code (PLUGIN_REQUIRES_NEWER_ETHERPAD) and a message that avoids leaking the Node-version implementation detail. Unparseable ranges fall through as compatible so the preflight is opportunistic, not a gate. 8 unit tests cover the happy + edge paths. - installer.ts: install() now best-effort fetches the published plugin's engines.node from npmjs.org, runs the preflight, and short-circuits with the typed error before invoking live-plugin-manager. Also wraps the body in try/catch so the error reaches the socket callback (it was silently dropped on every install failure today, including network errors). - HomePage.tsx: surfaces finished:install.error as a toast, using a dedicated i18n key when the code is PLUGIN_REQUIRES_NEWER_ETHERPAD and a generic fallback otherwise. - en.json: two new strings, parameterized by {{plugin}} and {{error}}. The message admins see is intentionally about Etherpad, not Node — upgrading Etherpad pulls the Node requirement along with it. Co-Authored-By: Claude Opus 4.7 (1M context) --- admin/src/pages/HomePage.tsx | 12 +++- src/locales/en.json | 2 + src/static/js/pluginfw/installer.ts | 41 ++++++++++-- src/static/js/pluginfw/pluginEngineCheck.ts | 47 ++++++++++++++ .../specs/pluginEngineCheck.test.ts | 65 +++++++++++++++++++ 5 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 src/static/js/pluginfw/pluginEngineCheck.ts create mode 100644 src/tests/backend-new/specs/pluginEngineCheck.test.ts diff --git a/admin/src/pages/HomePage.tsx b/admin/src/pages/HomePage.tsx index 9d14c9d0dee..9f7917684c0 100644 --- a/admin/src/pages/HomePage.tsx +++ b/admin/src/pages/HomePage.tsx @@ -68,7 +68,17 @@ export const HomePage = () => { ) setInstalledPlugins(updated) } - const onFinishedInstall = () => { + const onFinishedInstall = (data: {plugin: string; code?: string | null; error?: string | null}) => { + if (data?.error) { + const key = data.code === 'PLUGIN_REQUIRES_NEWER_ETHERPAD' + ? 'admin_plugins.install_error_requires_newer_etherpad' + : 'admin_plugins.install_error' + useStore.getState().setToastState({ + open: true, + title: t(key, {plugin: data.plugin, error: data.error}), + success: false, + }) + } pluginsSocket.emit('getInstalled') } const onFinishedUninstall = () => { diff --git a/src/locales/en.json b/src/locales/en.json index 5ae5bfa81ba..ac1ed02fc6c 100644 --- a/src/locales/en.json +++ b/src/locales/en.json @@ -66,6 +66,8 @@ "admin_plugins.disables.label": "Disables:", "admin_plugins.disables.warning_title": "This plugin intentionally removes the listed Etherpad features.", "admin_plugins.error_retrieving": "Error retrieving plugins", + "admin_plugins.install_error": "Failed to install {{plugin}}: {{error}}", + "admin_plugins.install_error_requires_newer_etherpad": "Cannot install {{plugin}}: it requires a newer version of Etherpad. Please upgrade Etherpad and try again.", "admin_plugins.installed": "Installed plugins", "admin_plugins.installed_fetching": "Fetching installed plugins…", "admin_plugins.installed_nothing": "You haven't installed any plugins yet.", diff --git a/src/static/js/pluginfw/installer.ts b/src/static/js/pluginfw/installer.ts index 8f8f937e701..4f9b7b2df86 100644 --- a/src/static/js/pluginfw/installer.ts +++ b/src/static/js/pluginfw/installer.ts @@ -17,9 +17,14 @@ import settings, { reloadSettings } from '../../../node/utils/Settings'; import {LinkInstaller} from "./LinkInstaller"; +import { + checkEngineCompatibility, + EngineIncompatibleError, +} from './pluginEngineCheck'; import {findEtherpadRoot} from '../../../node/utils/AbsolutePaths'; const logger = log4js.getLogger('plugins'); +const npmRegistry = 'https://registry.npmjs.org'; export const pluginInstallPath = path.join(settings.root, 'src','plugin_packages'); export const node_modules = path.join(findEtherpadRoot(),'src', 'node_modules'); @@ -156,13 +161,41 @@ export const uninstall = async (pluginName: string, cb:Function|null = null) => cb(null); }; +// Best-effort lookup of the published plugin's engines.node range. Returns +// undefined on any failure (network, 404, parse error) so the caller falls +// through to the existing install path rather than blocking on a flaky +// registry call. +const fetchPluginEnginesNode = async (pluginName: string): Promise => { + try { + const res = await fetch( + `${npmRegistry}/${encodeURIComponent(pluginName)}/latest`, + {headers}, + ); + if (!res.ok) return undefined; + const data = await res.json() as {engines?: {node?: string}}; + return data.engines?.node; + } catch { + return undefined; + } +}; + export const install = async (pluginName: string, cb:Function|null = null) => { cb = wrapTaskCb(cb); logger.info(`Installing plugin ${pluginName}...`); - await linkInstaller.installPlugin(pluginName); - logger.info(`Successfully installed plugin ${pluginName}`); - await hooks.aCallAll('pluginInstall', {pluginName}); - cb(null); + try { + const enginesNode = await fetchPluginEnginesNode(pluginName); + const compat = checkEngineCompatibility(enginesNode, process.version); + if (!compat.compatible) { + throw new EngineIncompatibleError(pluginName, compat.required, compat.current); + } + await linkInstaller.installPlugin(pluginName); + logger.info(`Successfully installed plugin ${pluginName}`); + await hooks.aCallAll('pluginInstall', {pluginName}); + cb(null); + } catch (err) { + logger.warn(`Failed to install plugin ${pluginName}: ${err}`); + cb(err); + } }; export let availablePlugins:MapArrayType|null = null; diff --git a/src/static/js/pluginfw/pluginEngineCheck.ts b/src/static/js/pluginfw/pluginEngineCheck.ts new file mode 100644 index 00000000000..35c524334f5 --- /dev/null +++ b/src/static/js/pluginfw/pluginEngineCheck.ts @@ -0,0 +1,47 @@ +'use strict'; + +import semver from 'semver'; + +export type EngineCheckResult = + | {compatible: true} + | {compatible: false; required: string; current: string}; + +/** + * Compares a plugin's `engines.node` range against the runtime Node version. + * + * Returned compatibility is used at plugin install time to short-circuit the + * install with a user-facing "requires a newer version of Etherpad" message + * instead of letting live-plugin-manager unpack a plugin that will fail at + * load time on the running Node. + * + * Unparseable ranges return `compatible: true` deliberately — this preflight + * is opportunistic, not a gatekeeper. If we can't make sense of the range + * we fall through to the existing install path. + */ +export const checkEngineCompatibility = ( + pluginEnginesNode: string | undefined, + currentNodeVersion: string, +): EngineCheckResult => { + if (!pluginEnginesNode) return {compatible: true}; + const current = currentNodeVersion.replace(/^v/, ''); + if (!semver.validRange(pluginEnginesNode)) return {compatible: true}; + if (semver.satisfies(current, pluginEnginesNode, {includePrerelease: true})) { + return {compatible: true}; + } + return {compatible: false, required: pluginEnginesNode, current}; +}; + +export class EngineIncompatibleError extends Error { + public readonly code = 'PLUGIN_REQUIRES_NEWER_ETHERPAD'; + constructor( + public readonly pluginName: string, + public readonly required: string, + public readonly current: string, + ) { + super( + `Plugin ${pluginName} requires a newer version of Etherpad. ` + + 'Please upgrade Etherpad and try again.', + ); + this.name = 'EngineIncompatibleError'; + } +} diff --git a/src/tests/backend-new/specs/pluginEngineCheck.test.ts b/src/tests/backend-new/specs/pluginEngineCheck.test.ts new file mode 100644 index 00000000000..e2f6519d0dd --- /dev/null +++ b/src/tests/backend-new/specs/pluginEngineCheck.test.ts @@ -0,0 +1,65 @@ +'use strict'; + +import {describe, it, expect} from 'vitest'; +import { + checkEngineCompatibility, + EngineIncompatibleError, +} from '../../../static/js/pluginfw/pluginEngineCheck'; + +describe('pluginEngineCheck', () => { + describe('checkEngineCompatibility', () => { + it('returns compatible when plugin declares no engines.node', () => { + expect(checkEngineCompatibility(undefined, 'v22.13.0').compatible).toBe(true); + }); + + it('returns compatible when the current Node satisfies the range', () => { + expect(checkEngineCompatibility('>=22.13.0', 'v22.13.0').compatible).toBe(true); + expect(checkEngineCompatibility('>=22.13.0', 'v25.1.0').compatible).toBe(true); + }); + + it('returns incompatible when the current Node is below the required range', () => { + const result = checkEngineCompatibility('>=25.0.0', 'v22.13.0'); + expect(result.compatible).toBe(false); + if (!result.compatible) { + expect(result.required).toBe('>=25.0.0'); + expect(result.current).toBe('22.13.0'); + } + }); + + it('strips the leading "v" from process.version-style strings', () => { + const result = checkEngineCompatibility('>=25.0.0', 'v22.13.0'); + expect(result.compatible).toBe(false); + if (!result.compatible) expect(result.current).not.toMatch(/^v/); + }); + + it('treats malformed engines strings as compatible (do not block install)', () => { + // If the plugin's engines field is garbage we should not block — let + // live-plugin-manager handle it. The whole point of this preflight is + // to give a useful message; if we can't parse the range, fall through. + expect(checkEngineCompatibility('not-a-range', 'v22.13.0').compatible).toBe(true); + }); + }); + + describe('EngineIncompatibleError', () => { + it('carries a stable code and the plugin name', () => { + const err = new EngineIncompatibleError('ep_test', '>=25.0.0', '22.13.0'); + expect(err.code).toBe('PLUGIN_REQUIRES_NEWER_ETHERPAD'); + expect(err.pluginName).toBe('ep_test'); + expect(err.required).toBe('>=25.0.0'); + expect(err.current).toBe('22.13.0'); + }); + + it('produces a user-facing message that does not mention Node', () => { + // Message reaches the admin UI. Per design, the admin is told to + // upgrade Etherpad — Node is an implementation detail. + const err = new EngineIncompatibleError('ep_test', '>=25.0.0', '22.13.0'); + expect(err.message).toMatch(/newer version of Etherpad/); + expect(err.message.toLowerCase()).not.toMatch(/node/); + }); + + it('is an instance of Error', () => { + const err = new EngineIncompatibleError('ep_test', '>=25.0.0', '22.13.0'); + expect(err).toBeInstanceOf(Error); + }); + }); +}); From e54f8da2323740ce7c0d00c33a008a27528d8397 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 06:09:39 +0100 Subject: [PATCH 2/3] fix(plugins): don't restart server when every install in the batch failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo PR review on #7771 caught a side effect introduced earlier in this PR. Before this PR, install() never invoked its callback on error, so a failed install left the task counter inflated and onAllTasksFinished() never ran — masking, but not fixing, the bug. Once install() correctly propagates errors to its cb, the counter hits zero on failure too, and onAllTasksFinished() fires hooks.aCallAll('restartServer'). A no-op preflight rejection (EngineIncompatibleError) would then disconnect every connected pad. Fix: extract wrapTaskCb + task state into InstallerTaskQueue and track whether at least one task in the current batch succeeded. Only fire the "all finished" side effect when something actually changed. Failed-only batches do nothing. Seven unit tests cover the matrix: single success, single failure (the regression), mixed batch (still restarts), all-failed batch, batch reset, null cb, two-task drain. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/static/js/pluginfw/installer.ts | 14 +-- src/static/js/pluginfw/installerTasks.ts | 31 +++++++ .../backend-new/specs/installerTasks.test.ts | 92 +++++++++++++++++++ 3 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 src/static/js/pluginfw/installerTasks.ts create mode 100644 src/tests/backend-new/specs/installerTasks.test.ts diff --git a/src/static/js/pluginfw/installer.ts b/src/static/js/pluginfw/installer.ts index 4f9b7b2df86..ea85a062b4d 100644 --- a/src/static/js/pluginfw/installer.ts +++ b/src/static/js/pluginfw/installer.ts @@ -21,6 +21,7 @@ import { checkEngineCompatibility, EngineIncompatibleError, } from './pluginEngineCheck'; +import {InstallerTaskQueue} from './installerTasks'; import {findEtherpadRoot} from '../../../node/utils/AbsolutePaths'; const logger = log4js.getLogger('plugins'); @@ -43,19 +44,10 @@ const headers = { 'User-Agent': `Etherpad/${getEpVersion()}`, }; -let tasks = 0; - export const linkInstaller = new LinkInstaller(); -const wrapTaskCb = (cb:Function|null) => { - tasks++; - - return (...args: any) => { - cb && cb(...args); - tasks--; - if (tasks === 0) onAllTasksFinished(); - }; -}; +const taskQueue = new InstallerTaskQueue(onAllTasksFinished); +const wrapTaskCb = (cb: Function | null) => taskQueue.wrap(cb); const migratePluginsFromNodeModules = async () => { logger.info('start migration of plugins in node_modules'); diff --git a/src/static/js/pluginfw/installerTasks.ts b/src/static/js/pluginfw/installerTasks.ts new file mode 100644 index 00000000000..8aaa3ca4bc1 --- /dev/null +++ b/src/static/js/pluginfw/installerTasks.ts @@ -0,0 +1,31 @@ +'use strict'; + +// Tracks the in-flight batch of install/uninstall operations and decides when +// to fire the "all tasks finished" side effect (reload settings, restart +// server). Extracted so it can be unit-tested without dragging in the +// LinkInstaller circular import. +// +// Crucial invariant: onFinished MUST NOT run when every task in the batch +// failed — that path is reachable now that install() correctly propagates +// errors to its callback. Restarting the server on a failed install would +// disconnect every connected pad for no reason. +export class InstallerTaskQueue { + private tasks = 0; + private anyTaskSucceeded = false; + + constructor(private readonly onFinished: () => unknown) {} + + wrap(cb: Function | null): (...args: unknown[]) => void { + this.tasks++; + return (...args: unknown[]) => { + if (!args[0]) this.anyTaskSucceeded = true; + if (cb) cb(...args); + this.tasks--; + if (this.tasks === 0) { + const shouldFinish = this.anyTaskSucceeded; + this.anyTaskSucceeded = false; + if (shouldFinish) this.onFinished(); + } + }; + } +} diff --git a/src/tests/backend-new/specs/installerTasks.test.ts b/src/tests/backend-new/specs/installerTasks.test.ts new file mode 100644 index 00000000000..dd582cd5099 --- /dev/null +++ b/src/tests/backend-new/specs/installerTasks.test.ts @@ -0,0 +1,92 @@ +'use strict'; + +import {describe, it, expect, vi} from 'vitest'; +import {InstallerTaskQueue} from '../../../static/js/pluginfw/installerTasks'; + +describe('InstallerTaskQueue', () => { + it('fires onFinished after a single successful task', () => { + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + const cb = vi.fn(); + const wrapped = q.wrap(cb); + + wrapped(null); + + expect(cb).toHaveBeenCalledWith(null); + expect(onFinished).toHaveBeenCalledTimes(1); + }); + + it('does NOT fire onFinished when the only task in the batch failed', () => { + // Regression: before this fix, a failed install(EngineIncompatibleError) + // would still trigger restartServer via onAllTasksFinished, kicking every + // connected pad off the server for no benefit. Reported by Qodo on + // PR #7771. + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + const wrapped = q.wrap(vi.fn()); + + wrapped(new Error('plugin requires a newer version of Etherpad')); + + expect(onFinished).not.toHaveBeenCalled(); + }); + + it('fires onFinished when at least one task in a mixed batch succeeded', () => { + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + const ok = q.wrap(vi.fn()); + const bad = q.wrap(vi.fn()); + + bad(new Error('boom')); + expect(onFinished).not.toHaveBeenCalled(); + ok(null); + + expect(onFinished).toHaveBeenCalledTimes(1); + }); + + it('does NOT fire onFinished when every task in a multi-task batch failed', () => { + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + const a = q.wrap(vi.fn()); + const b = q.wrap(vi.fn()); + + a(new Error('one')); + b(new Error('two')); + + expect(onFinished).not.toHaveBeenCalled(); + }); + + it('resets the success flag between batches', () => { + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + + const ok = q.wrap(vi.fn()); + ok(null); + expect(onFinished).toHaveBeenCalledTimes(1); + + const bad = q.wrap(vi.fn()); + bad(new Error('next batch all failed')); + expect(onFinished).toHaveBeenCalledTimes(1); + }); + + it('tolerates a null callback', () => { + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + const wrapped = q.wrap(null); + + expect(() => wrapped(null)).not.toThrow(); + expect(onFinished).toHaveBeenCalledTimes(1); + }); + + it('only fires onFinished once all wrapped tasks have completed', () => { + const onFinished = vi.fn(); + const q = new InstallerTaskQueue(onFinished); + const a = q.wrap(vi.fn()); + const b = q.wrap(vi.fn()); + + a(null); + expect(onFinished).not.toHaveBeenCalled(); + b(null); + + expect(onFinished).toHaveBeenCalledTimes(1); + }); +}); From 912da05bb1605cae8a672481602fefbee349e922 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 06:24:06 +0100 Subject: [PATCH 3/3] fix(plugins): time-bound the engines preflight registry fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo PR review on #7771 flagged that fetchPluginEnginesNode awaits fetch() with no timeout. A stalled DNS lookup or hung connection to registry.npmjs.org would block install() forever — the finished:install socket event would never fire and the admin UI would stay spinning with no error to surface. Wrap the fetch in AbortSignal.timeout(5000). On any failure (network, HTTP error, abort) fetchPluginEnginesNode returns undefined, which the preflight then treats as "no engines info → compatible," so a slow registry never blocks an install that would otherwise succeed. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/static/js/pluginfw/installer.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/static/js/pluginfw/installer.ts b/src/static/js/pluginfw/installer.ts index ea85a062b4d..eb65a503d5f 100644 --- a/src/static/js/pluginfw/installer.ts +++ b/src/static/js/pluginfw/installer.ts @@ -154,19 +154,24 @@ export const uninstall = async (pluginName: string, cb:Function|null = null) => }; // Best-effort lookup of the published plugin's engines.node range. Returns -// undefined on any failure (network, 404, parse error) so the caller falls -// through to the existing install path rather than blocking on a flaky -// registry call. +// undefined on any failure (network, 404, parse error, timeout) so the +// caller falls through to the existing install path rather than blocking on +// a flaky registry call. A 5s AbortSignal.timeout guards against a stalled +// registry hanging the install promise forever — without it the +// finished:install socket event would never fire and the admin UI would +// stay spinning indefinitely. +const ENGINES_PREFLIGHT_TIMEOUT_MS = 5000; const fetchPluginEnginesNode = async (pluginName: string): Promise => { try { const res = await fetch( `${npmRegistry}/${encodeURIComponent(pluginName)}/latest`, - {headers}, + {headers, signal: AbortSignal.timeout(ENGINES_PREFLIGHT_TIMEOUT_MS)}, ); if (!res.ok) return undefined; const data = await res.json() as {engines?: {node?: string}}; return data.engines?.node; - } catch { + } catch (err) { + logger.debug(`engines preflight for ${pluginName} fell through: ${err}`); return undefined; } };