diff --git a/admin/src/pages/HomePage.tsx b/admin/src/pages/HomePage.tsx index 9d14c9d0d..9f7917684 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 55f4159af..f9ca67282 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 8f8f937e7..eb65a503d 100644 --- a/src/static/js/pluginfw/installer.ts +++ b/src/static/js/pluginfw/installer.ts @@ -17,9 +17,15 @@ import settings, { reloadSettings } from '../../../node/utils/Settings'; import {LinkInstaller} from "./LinkInstaller"; +import { + checkEngineCompatibility, + EngineIncompatibleError, +} from './pluginEngineCheck'; +import {InstallerTaskQueue} from './installerTasks'; 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'); @@ -38,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'); @@ -156,13 +153,46 @@ 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, 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, 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 (err) { + logger.debug(`engines preflight for ${pluginName} fell through: ${err}`); + 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/installerTasks.ts b/src/static/js/pluginfw/installerTasks.ts new file mode 100644 index 000000000..8aaa3ca4b --- /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/static/js/pluginfw/pluginEngineCheck.ts b/src/static/js/pluginfw/pluginEngineCheck.ts new file mode 100644 index 000000000..35c524334 --- /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/installerTasks.test.ts b/src/tests/backend-new/specs/installerTasks.test.ts new file mode 100644 index 000000000..dd582cd50 --- /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); + }); +}); 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 000000000..e2f6519d0 --- /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); + }); + }); +});