From 22b7ccedc73d509a59b93c08a37ca5511ef9ab3c Mon Sep 17 00:00:00 2001 From: Christopher Pruijsen Date: Sun, 13 Sep 2026 09:48:40 +0000 Subject: [PATCH] fix: do not render plugins with invalid configurations Plugin changes whose config failed validation (e.g. an Insert widget missing its priority) were still applied to the slot. The missing priority produced NaN in the ordering comparator, which Array.sort treats as equal, scrambling the order of every widget in the slot. validatePlugin now returns false for invalid configs and organizePlugins skips the change entirely: the misconfigured plugin is not rendered, operations targeting it become no-ops, and the logged error still indicates the misconfiguration. This also prevents invalid plugin operations from throwing a TypeError during validation. --- src/plugins/data/utils.jsx | 24 +++-- src/plugins/data/utils.test.jsx | 162 ++++++++++++++++++++------------ 2 files changed, 116 insertions(+), 70 deletions(-) diff --git a/src/plugins/data/utils.jsx b/src/plugins/data/utils.jsx index e5a769f5..415e15b6 100644 --- a/src/plugins/data/utils.jsx +++ b/src/plugins/data/utils.jsx @@ -14,29 +14,35 @@ const validateRequirements = (requiredTypes, widgetConfig) => Object.keys(requir /** * Called by organizePlugins to validate plugin configurations - * @returns {Boolean} - boolean if all types are correct and present, else throws an error + * @returns {Boolean} - true if all types are correct and present, otherwise logs an error and returns false */ export const validatePlugin = (pluginConfig) => { - let requiredTypes = {}; const { op } = pluginConfig; let config = pluginConfig; - if (!op) { logError('There is a config with an invalid PLUGIN_OPERATION. Check to make sure it is configured correctly.'); } + if (!Object.values(PLUGIN_OPERATIONS).includes(op)) { + logError('There is a config with an invalid PLUGIN_OPERATION. Check to make sure it is configured correctly.'); + return false; + } + + let requiredTypes = requiredPluginTypes[op]; if (op === PLUGIN_OPERATIONS.Insert) { config = config.widget; - if (!config) { logError('insert operation config is missing widget object'); } + if (!config) { + logError('insert operation config is missing widget object'); + return false; + } requiredTypes = { - ...requiredPluginTypes[op].base, - ...requiredPluginTypes[op][config.type?.toLowerCase()], + ...requiredTypes.base, + ...requiredTypes[config.type?.toLowerCase()], }; - } else { - requiredTypes = requiredPluginTypes[op]; } if (!validateRequirements(requiredTypes, config)) { logError(`the ${op} operation config is invalid for widget id: ${config.widgetId || config.id || 'MISSING ID'}`); + return false; } return true; @@ -52,7 +58,7 @@ export const validatePlugin = (pluginConfig) => { export const organizePlugins = (defaultContents, plugins) => { const newContents = [...defaultContents]; plugins.forEach(change => { - validatePlugin(change); + if (!validatePlugin(change)) { return; } if (change.op === PLUGIN_OPERATIONS.Insert) { newContents.push(change.widget); } else if (change.op === PLUGIN_OPERATIONS.Hide) { diff --git a/src/plugins/data/utils.test.jsx b/src/plugins/data/utils.test.jsx index 602cebd5..dbd40e70 100644 --- a/src/plugins/data/utils.test.jsx +++ b/src/plugins/data/utils.test.jsx @@ -175,6 +175,79 @@ describe('organizePlugins', () => { expect(plugins[1].id).toBe('default_contents'); expect(plugins[2].id).toBe('login'); }); + + it('should not insert a plugin that is missing a required property', () => { + const plugins = organizePlugins(mockDefaultContent, [ + { + op: PLUGIN_OPERATIONS.Insert, + widget: { + id: 'no_priority_plugin', + type: DIRECT_PLUGIN, + RenderWidget: mockRenderWidget, + }, + }, + ]); + expect(plugins.length).toEqual(1); + expect(plugins.find((w) => w.id === 'no_priority_plugin')).toBeUndefined(); + expect(logError).toHaveBeenCalledWith('the insert operation config is invalid for widget id: no_priority_plugin'); + }); + + it('should ignore operations targeting a plugin that failed validation', () => { + const plugins = organizePlugins(mockDefaultContent, [ + { + op: PLUGIN_OPERATIONS.Insert, + widget: { + id: 'no_priority_plugin', + type: DIRECT_PLUGIN, + RenderWidget: mockRenderWidget, + }, + }, + { + op: PLUGIN_OPERATIONS.Modify, + widgetId: 'no_priority_plugin', + fn: mockModifyWidget, + }, + { + op: PLUGIN_OPERATIONS.Wrap, + widgetId: 'no_priority_plugin', + wrapper: makeMockElementWrapper(), + }, + ]); + expect(plugins.length).toEqual(1); + expect(plugins[0].id).toBe('default_contents'); + }); + + it('should preserve the priority ordering of valid plugins when an insert is invalid', () => { + const plugins = organizePlugins(mockDefaultContent, [ + { + op: PLUGIN_OPERATIONS.Insert, + widget: { + id: 'second_plugin', + priority: 30, + type: DIRECT_PLUGIN, + RenderWidget: mockRenderWidget, + }, + }, + { + op: PLUGIN_OPERATIONS.Insert, + widget: { + id: 'no_priority_plugin', + type: DIRECT_PLUGIN, + RenderWidget: mockRenderWidget, + }, + }, + { + op: PLUGIN_OPERATIONS.Insert, + widget: { + id: 'first_plugin', + priority: 10, + type: DIRECT_PLUGIN, + RenderWidget: mockRenderWidget, + }, + }, + ]); + expect(plugins.map((w) => w.id)).toEqual(['first_plugin', 'second_plugin', 'default_contents']); + }); }); }); @@ -329,35 +402,20 @@ describe('validatePlugin', () => { }, }; - try { - validatePlugin(insertBrokenDirectConfig); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the insert configuration is invalid for widget id: MISSING ID'); - } - - try { - validatePlugin(insertBrokenDirectConfig2); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the insert configuration is invalid for widget id: new_plugin'); - } - - try { - validatePlugin(insertBrokenDirectConfig3); - } catch (error) { - expect(logError).toHaveBeenCalledWith('insert operation config is missing widget object'); - } - - try { - validatePlugin(insertBrokenIFrameConfig); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the insert configuration is invalid for widget id: new_iframe_plugin'); - } - - try { - validatePlugin(insertBrokenIFrameConfig2); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the insert configuration is invalid for widget id: new_iframe_plugin'); - } + expect(validatePlugin(insertBrokenDirectConfig)).toBe(false); + expect(logError).toHaveBeenCalledWith('the insert operation config is invalid for widget id: MISSING ID'); + + expect(validatePlugin(insertBrokenDirectConfig2)).toBe(false); + expect(logError).toHaveBeenCalledWith('the insert operation config is invalid for widget id: new_plugin'); + + expect(validatePlugin(insertBrokenDirectConfig3)).toBe(false); + expect(logError).toHaveBeenCalledWith('insert operation config is missing widget object'); + + expect(validatePlugin(insertBrokenIFrameConfig)).toBe(false); + expect(logError).toHaveBeenCalledWith('the insert operation config is invalid for widget id: new_iframe_plugin'); + + expect(validatePlugin(insertBrokenIFrameConfig2)).toBe(false); + expect(logError).toHaveBeenCalledWith('the insert operation config is invalid for widget id: new_iframe_plugin'); }); }); describe('hide plugin configuration', () => { @@ -373,11 +431,8 @@ describe('validatePlugin', () => { op: PLUGIN_OPERATIONS.Hide, }; - try { - validatePlugin(invalidHideConfig); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the hide operation config is invalid for widget id: MISSING ID'); - } + expect(validatePlugin(invalidHideConfig)).toBe(false); + expect(logError).toHaveBeenCalledWith('the hide operation config is invalid for widget id: MISSING ID'); }); }); describe('modify plugin configuration', () => { @@ -399,16 +454,10 @@ describe('validatePlugin', () => { fn: mockModifyWidget, }; - try { - validatePlugin(invalidModifyConfig1); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the modify operation config is invalid for widget id: random_plugin'); - } - try { - validatePlugin(invalidModifyConfig2); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the modify operation config is invalid for widget id: MISSING ID'); - } + expect(validatePlugin(invalidModifyConfig1)).toBe(false); + expect(logError).toHaveBeenCalledWith('the modify operation config is invalid for widget id: random_plugin'); + expect(validatePlugin(invalidModifyConfig2)).toBe(false); + expect(logError).toHaveBeenCalledWith('the modify operation config is invalid for widget id: MISSING ID'); }); }); describe('wrap plugin configuration', () => { @@ -430,30 +479,21 @@ describe('validatePlugin', () => { wrapper: makeMockElementWrapper(), }; - try { - validatePlugin(invalidWrapConfig1); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the wrap operation config is invalid for widget id: random_plugin'); - } - try { - validatePlugin(invalidWrapConfig2); - } catch (error) { - expect(logError).toHaveBeenCalledWith('the wrap operation config is invalid for widget id: MISSING ID'); - } + expect(validatePlugin(invalidWrapConfig1)).toBe(false); + expect(logError).toHaveBeenCalledWith('the wrap operation config is invalid for widget id: random_plugin'); + expect(validatePlugin(invalidWrapConfig2)).toBe(false); + expect(logError).toHaveBeenCalledWith('the wrap operation config is invalid for widget id: MISSING ID'); }); }); describe('an invalid plugin configuration', () => { - it('should raise an error for an operation that does not exist', () => { + it.each([undefined, 'destroy'])('should return false and raise an error for operation "%s"', (op) => { const invalidPluginConfig = { - op: PLUGIN_OPERATIONS.Destroy, + op, widgetId: 'drafts', }; - try { - validatePlugin(invalidPluginConfig); - } catch (error) { - expect(logError).toHaveBeenCalledWith('There is a config with an invalid PLUGIN_OPERATION. Check to make sure it is configured correctly.'); - } + expect(validatePlugin(invalidPluginConfig)).toBe(false); + expect(logError).toHaveBeenCalledWith('There is a config with an invalid PLUGIN_OPERATION. Check to make sure it is configured correctly.'); }); }); });