Skip to content

fix(clientVars): stop mutating the shared plugin registry during sanitization#7587

Merged
JohnMcLear merged 1 commit intoether:developfrom
JohnMcLear:fix-plugin-package-mutation
Apr 23, 2026
Merged

fix(clientVars): stop mutating the shared plugin registry during sanitization#7587
JohnMcLear merged 1 commit intoether:developfrom
JohnMcLear:fix-plugin-package-mutation

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

PadMessageHandler was building the pluginsSanitized payload for clientVars by aliasing plugins.plugins and mutating each entry's package field in place:

let pluginsSanitized: any = plugins.plugins;       // alias, not a copy
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 was a reference to plugins.plugins, the assignment clobbered the shared server-side plugin registry. After the first pad connection every plugin's package object held only {name, version} — the realPath, path, and location fields were gone.

Impact

Minify.ts resolves /static/plugins/ep_*/... URLs via plugin.package.realPath. Once the field disappeared, every subsequent static asset request for a bundled plugin responded with HTTP 500:

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)

Observable symptoms:

  • Chromium: plugin CSS/JS 500s on first asset request after any pad join. For example /static/plugins/ep_font_size/static/css/size.css returns 500. Plugins partially or completely fail to render.
  • Firefox: swallows the console/network errors quietly but the plugin assets are still broken.
  • Server log: a cascade of the same ERR_INVALID_ARG_TYPE stack every time a static asset request hits a bundled plugin.

Fix

Extract sanitization into a pure helper sanitizePluginsForWire that returns a fresh object graph and never touches the input. Replaces the aliasing + mutation pattern with a spread-based copy.

Tests

New backend spec sanitizePluginsForWire.ts covering:

  • Sanitized output contains only {name, version} in package.
  • Input registry's realPath, path, location survive the call (regression of this bug).
  • Repeated invocations remain non-destructive.
  • Mutations to the returned copy don't leak back to the input.

Test plan

  • New spec passes (4/4).
  • Full backend suite passes (799 total, +4 from this PR).
  • tsc --noEmit clean in our code.
  • Manual verification with dev server: /static/plugins/ep_font_size/static/css/size.css returns 200 both before and after pad connections. Before the fix, the same request 500'd after the first pad visit.

🤖 Generated with Claude Code

…tization

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.
@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Fix plugin registry mutation during clientVars sanitization

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Extract plugin sanitization into pure helper function
• Prevent mutation of shared server-side plugin registry
• Fix 500 errors on static plugin asset requests after pad connections
• Add comprehensive test coverage for sanitization logic
Diagram
flowchart LR
  A["Shared Plugin Registry"] -->|sanitizePluginsForWire| B["Fresh Sanitized Copy"]
  B -->|{name, version}| C["Client Vars"]
  A -->|realPath preserved| D["Minify.ts Asset Resolution"]
  D -->|/static/plugins/...| E["200 OK Response"]
Loading

Grey Divider

File Changes

1. src/node/handler/PadMessageHandler.ts 🐞 Bug fix +29/-5

Extract sanitization into pure helper function

• Added sanitizePluginsForWire helper function that creates fresh object graph
• Function spreads plugin properties and reduces package to {name, version}
• Replaced inline mutation pattern with call to new helper in handleClientReady
• Includes detailed JSDoc explaining critical non-mutation requirement

src/node/handler/PadMessageHandler.ts


2. src/tests/backend/specs/sanitizePluginsForWire.ts 🧪 Tests +65/-0

Add comprehensive tests for plugin sanitization

• New test suite with 4 test cases covering sanitization behavior
• Verifies sanitized output contains only {name, version} in package field
• Asserts input registry fields (realPath, path, location) survive sanitization
• Tests repeated invocations remain non-destructive
• Validates returned copy is independent from input (mutations don't leak back)

src/tests/backend/specs/sanitizePluginsForWire.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented Apr 23, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Qodo Logo

@JohnMcLear JohnMcLear merged commit 9e352ca into ether:develop Apr 23, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant