Skip to content

Commit 9e352ca

Browse files
authored
fix(clientVars): stop mutating the shared plugin registry during sanitization (#7587)
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.
1 parent 67e542d commit 9e352ca

2 files changed

Lines changed: 94 additions & 5 deletions

File tree

src/node/handler/PadMessageHandler.ts

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,34 @@ function getActivePadCountFromSessionInfos() {
109109
}
110110
exports.getActivePadCountFromSessionInfos = getActivePadCountFromSessionInfos;
111111

112+
/**
113+
* Build a sanitized copy of the plugins registry suitable for sending to the
114+
* client as part of clientVars. The shape is preserved but each plugin's
115+
* `package` field is reduced to `{name, version}` so internal paths (realPath,
116+
* path, location) are not leaked to the browser.
117+
*
118+
* CRITICAL: this function MUST NOT mutate the shared server-side registry.
119+
* Other components — notably `src/node/utils/Minify.ts` — read
120+
* `plugins.plugins[x].package.realPath` on every static asset request to
121+
* resolve `/static/plugins/ep_<name>/...` URLs to disk. Mutating the shared object
122+
* in place would clobber `realPath` and cause every such request to 500 with
123+
* `ERR_INVALID_ARG_TYPE: The "path" argument must be of type string`.
124+
*/
125+
const sanitizePluginsForWire = (
126+
pluginsRegistry: MapArrayType<any>,
127+
): MapArrayType<any> => {
128+
const out: MapArrayType<any> = {};
129+
for (const [name, plugin] of Object.entries(pluginsRegistry)) {
130+
const p: any = plugin.package;
131+
out[name] = {
132+
...plugin,
133+
package: {name: p.name, version: p.version},
134+
};
135+
}
136+
return out;
137+
};
138+
exports.sanitizePluginsForWire = sanitizePluginsForWire;
139+
112140
stats.gauge('totalUsers', () => getTotalActiveUsers());
113141
stats.gauge('activePads', () => {
114142
return getActivePadCountFromSessionInfos();
@@ -1068,11 +1096,7 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => {
10681096
throw new Error('corrupt pad');
10691097
}
10701098

1071-
let pluginsSanitized: any = plugins.plugins
1072-
Object.keys(plugins.plugins).forEach(function(element) {
1073-
const p: any = plugins.plugins[element].package
1074-
pluginsSanitized[element].package = {name: p.name, version: p.version};
1075-
});
1099+
const pluginsSanitized = sanitizePluginsForWire(plugins.plugins);
10761100
// Warning: never ever send sessionInfo.padId to the client. If the client is read only you
10771101
// would open a security hole 1 swedish mile wide...
10781102
const canEditPadSettings = settings.enablePadWideSettings &&
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
'use strict';
2+
3+
import {strict as assert} from 'assert';
4+
const {sanitizePluginsForWire} = require('../../../node/handler/PadMessageHandler');
5+
6+
describe(__filename, function () {
7+
const makeRegistry = () => ({
8+
ep_example: {
9+
parts: [{name: 'ep_example', plugin: 'ep_example'}],
10+
package: {
11+
name: 'ep_example',
12+
version: '1.2.3',
13+
realPath: '/real/path/to/ep_example',
14+
path: '/node_modules/ep_example',
15+
location: '/real/path/to/ep_example',
16+
},
17+
},
18+
ep_other: {
19+
parts: [{name: 'ep_other', plugin: 'ep_other'}],
20+
package: {
21+
name: 'ep_other',
22+
version: '0.1.0',
23+
realPath: '/real/path/to/ep_other',
24+
path: '/node_modules/ep_other',
25+
location: '/real/path/to/ep_other',
26+
},
27+
},
28+
});
29+
30+
it('returns a sanitized registry with only name and version in package', function () {
31+
const registry = makeRegistry();
32+
const sanitized = sanitizePluginsForWire(registry);
33+
assert.deepEqual(Object.keys(sanitized).sort(), ['ep_example', 'ep_other']);
34+
assert.deepEqual(sanitized.ep_example.package, {name: 'ep_example', version: '1.2.3'});
35+
assert.deepEqual(sanitized.ep_other.package, {name: 'ep_other', version: '0.1.0'});
36+
});
37+
38+
it('does not mutate the input registry (issue: realPath clobbered on pad join)', function () {
39+
const registry = makeRegistry();
40+
sanitizePluginsForWire(registry);
41+
// The original objects MUST still carry realPath and the other internal
42+
// path fields — Minify.ts relies on them for every /static/plugins/...
43+
// asset request. Before the fix, the sanitization mutated these in place
44+
// and caused every such request to 500 after the first pad connection.
45+
assert.equal(registry.ep_example.package.realPath, '/real/path/to/ep_example');
46+
assert.equal(registry.ep_example.package.path, '/node_modules/ep_example');
47+
assert.equal(registry.ep_other.package.realPath, '/real/path/to/ep_other');
48+
assert.equal(registry.ep_other.package.path, '/node_modules/ep_other');
49+
});
50+
51+
it('repeated calls remain non-destructive', function () {
52+
const registry = makeRegistry();
53+
for (let i = 0; i < 5; i++) sanitizePluginsForWire(registry);
54+
assert.equal(registry.ep_example.package.realPath, '/real/path/to/ep_example');
55+
assert.equal(registry.ep_other.package.realPath, '/real/path/to/ep_other');
56+
});
57+
58+
it('returned copy and input are independent (mutation of result does not affect input)', function () {
59+
const registry = makeRegistry();
60+
const sanitized = sanitizePluginsForWire(registry);
61+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
62+
(sanitized.ep_example as any).package.name = 'tampered';
63+
assert.equal(registry.ep_example.package.name, 'ep_example');
64+
});
65+
});

0 commit comments

Comments
 (0)