From 892a842ce4d3b9111bd32d8c64a4c165aaa2ef1b Mon Sep 17 00:00:00 2001 From: John McLear Date: Thu, 23 Apr 2026 10:13:35 +0100 Subject: [PATCH] fix(clientVars): stop mutating the shared plugin registry during sanitization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PadMessageHandler built the `pluginsSanitized` payload for clientVars by aliasing `plugins.plugins` and then mutating each entry's `package` field in place: let pluginsSanitized: any = plugins.plugins; Object.keys(plugins.plugins).forEach(function(element) { const p: any = plugins.plugins[element].package; pluginsSanitized[element].package = {name: p.name, version: p.version}; }); Because `pluginsSanitized` is a reference to `plugins.plugins`, the assignment clobbered the server-side plugin registry. After the first pad connection, every plugin's `package` object held only `{name, version}` — `realPath`, `path`, and `location` were gone. Minify.ts resolves `/static/plugins/ep_*/...` URLs via `plugin.package.realPath`. Once the field disappeared, every subsequent static asset request for a bundled plugin 500'd with: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined at Object.join (node:path:1354:7) at _minify (src/node/utils/Minify.ts:181:23) Symptoms on Chromium: plugin CSS/JS assets fail to load (e.g. /static/plugins/ep_font_size/static/css/size.css returns 500), so plugins partially render or don't work at all. Firefox swallows the resulting console errors quietly. Fix: extract the sanitization into a pure helper `sanitizePluginsForWire` that returns a fresh object graph and never touches the input. The helper is covered by a new backend spec that: * verifies the sanitized output has only {name, version} in `package` * asserts the input registry's realPath/path/location survive the call * runs the call repeatedly and confirms non-destructiveness * mutates the returned copy and asserts the input is independent Verified live with the dev server: before the fix, `/static/plugins/ ep_font_size/static/css/size.css` 500'd after visiting any pad; after the fix it returns 200 both before and after pad connections. --- src/node/handler/PadMessageHandler.ts | 34 ++++++++-- .../backend/specs/sanitizePluginsForWire.ts | 65 +++++++++++++++++++ 2 files changed, 94 insertions(+), 5 deletions(-) create mode 100644 src/tests/backend/specs/sanitizePluginsForWire.ts diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index d0f34600ca0..006831f768d 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -109,6 +109,34 @@ function getActivePadCountFromSessionInfos() { } exports.getActivePadCountFromSessionInfos = getActivePadCountFromSessionInfos; +/** + * Build a sanitized copy of the plugins registry suitable for sending to the + * client as part of clientVars. The shape is preserved but each plugin's + * `package` field is reduced to `{name, version}` so internal paths (realPath, + * path, location) are not leaked to the browser. + * + * CRITICAL: this function MUST NOT mutate the shared server-side registry. + * Other components — notably `src/node/utils/Minify.ts` — read + * `plugins.plugins[x].package.realPath` on every static asset request to + * resolve `/static/plugins/ep_/...` URLs to disk. Mutating the shared object + * in place would clobber `realPath` and cause every such request to 500 with + * `ERR_INVALID_ARG_TYPE: The "path" argument must be of type string`. + */ +const sanitizePluginsForWire = ( + pluginsRegistry: MapArrayType, +): MapArrayType => { + const out: MapArrayType = {}; + for (const [name, plugin] of Object.entries(pluginsRegistry)) { + const p: any = plugin.package; + out[name] = { + ...plugin, + package: {name: p.name, version: p.version}, + }; + } + return out; +}; +exports.sanitizePluginsForWire = sanitizePluginsForWire; + stats.gauge('totalUsers', () => getTotalActiveUsers()); stats.gauge('activePads', () => { return getActivePadCountFromSessionInfos(); @@ -1068,11 +1096,7 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => { throw new Error('corrupt pad'); } - let pluginsSanitized: any = plugins.plugins - Object.keys(plugins.plugins).forEach(function(element) { - const p: any = plugins.plugins[element].package - pluginsSanitized[element].package = {name: p.name, version: p.version}; - }); + const pluginsSanitized = sanitizePluginsForWire(plugins.plugins); // Warning: never ever send sessionInfo.padId to the client. If the client is read only you // would open a security hole 1 swedish mile wide... const canEditPadSettings = settings.enablePadWideSettings && diff --git a/src/tests/backend/specs/sanitizePluginsForWire.ts b/src/tests/backend/specs/sanitizePluginsForWire.ts new file mode 100644 index 00000000000..052a35be409 --- /dev/null +++ b/src/tests/backend/specs/sanitizePluginsForWire.ts @@ -0,0 +1,65 @@ +'use strict'; + +import {strict as assert} from 'assert'; +const {sanitizePluginsForWire} = require('../../../node/handler/PadMessageHandler'); + +describe(__filename, function () { + const makeRegistry = () => ({ + ep_example: { + parts: [{name: 'ep_example', plugin: 'ep_example'}], + package: { + name: 'ep_example', + version: '1.2.3', + realPath: '/real/path/to/ep_example', + path: '/node_modules/ep_example', + location: '/real/path/to/ep_example', + }, + }, + ep_other: { + parts: [{name: 'ep_other', plugin: 'ep_other'}], + package: { + name: 'ep_other', + version: '0.1.0', + realPath: '/real/path/to/ep_other', + path: '/node_modules/ep_other', + location: '/real/path/to/ep_other', + }, + }, + }); + + it('returns a sanitized registry with only name and version in package', function () { + const registry = makeRegistry(); + const sanitized = sanitizePluginsForWire(registry); + assert.deepEqual(Object.keys(sanitized).sort(), ['ep_example', 'ep_other']); + assert.deepEqual(sanitized.ep_example.package, {name: 'ep_example', version: '1.2.3'}); + assert.deepEqual(sanitized.ep_other.package, {name: 'ep_other', version: '0.1.0'}); + }); + + it('does not mutate the input registry (issue: realPath clobbered on pad join)', function () { + const registry = makeRegistry(); + sanitizePluginsForWire(registry); + // The original objects MUST still carry realPath and the other internal + // path fields — Minify.ts relies on them for every /static/plugins/... + // asset request. Before the fix, the sanitization mutated these in place + // and caused every such request to 500 after the first pad connection. + assert.equal(registry.ep_example.package.realPath, '/real/path/to/ep_example'); + assert.equal(registry.ep_example.package.path, '/node_modules/ep_example'); + assert.equal(registry.ep_other.package.realPath, '/real/path/to/ep_other'); + assert.equal(registry.ep_other.package.path, '/node_modules/ep_other'); + }); + + it('repeated calls remain non-destructive', function () { + const registry = makeRegistry(); + for (let i = 0; i < 5; i++) sanitizePluginsForWire(registry); + assert.equal(registry.ep_example.package.realPath, '/real/path/to/ep_example'); + assert.equal(registry.ep_other.package.realPath, '/real/path/to/ep_other'); + }); + + it('returned copy and input are independent (mutation of result does not affect input)', function () { + const registry = makeRegistry(); + const sanitized = sanitizePluginsForWire(registry); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (sanitized.ep_example as any).package.name = 'tampered'; + assert.equal(registry.ep_example.package.name, 'ep_example'); + }); +});