Skip to content

Commit 432cba1

Browse files
Copilothotlong
Copilot
and
hotlong
committed
Fix security issues and improve code quality based on review
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent 9843066 commit 432cba1

5 files changed

Lines changed: 113 additions & 40 deletions

File tree

‎PLUGIN_SYSTEM.md‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,13 @@ const myPlugin: PluginDefinition = {
5252
The `PluginSystem` class manages plugin loading, dependency resolution, and lifecycle:
5353

5454
```typescript
55-
import { PluginSystem } from '@object-ui/core';
55+
import { PluginSystem, ComponentRegistry } from '@object-ui/core';
5656

5757
const pluginSystem = new PluginSystem();
5858

59-
// Load plugins in dependency order
60-
await pluginSystem.loadPlugin(basePlugin);
61-
await pluginSystem.loadPlugin(dependentPlugin); // Requires basePlugin
59+
// Load plugins in dependency order (pass registry for component registration)
60+
await pluginSystem.loadPlugin(basePlugin, ComponentRegistry);
61+
await pluginSystem.loadPlugin(dependentPlugin, ComponentRegistry); // Requires basePlugin
6262

6363
// Check plugin status
6464
if (pluginSystem.isLoaded('my-plugin')) {
@@ -84,19 +84,19 @@ await pluginSystem.loadPlugin({
8484
version: '1.0.0',
8585
dependencies: ['missing-plugin'], // Not loaded yet
8686
register: () => {}
87-
});
87+
}, ComponentRegistry);
8888
// Error: Missing dependency: missing-plugin required by dependent-plugin
8989

9090
// ✅ Load in correct order
91-
await pluginSystem.loadPlugin(basePlugin);
92-
await pluginSystem.loadPlugin(dependentPlugin);
91+
await pluginSystem.loadPlugin(basePlugin, ComponentRegistry);
92+
await pluginSystem.loadPlugin(dependentPlugin, ComponentRegistry);
9393
```
9494

9595
The system also prevents unloading plugins that other plugins depend on:
9696

9797
```typescript
98-
await pluginSystem.loadPlugin(basePlugin);
99-
await pluginSystem.loadPlugin(dependentPlugin); // depends on basePlugin
98+
await pluginSystem.loadPlugin(basePlugin, ComponentRegistry);
99+
await pluginSystem.loadPlugin(dependentPlugin, ComponentRegistry); // depends on basePlugin
100100

101101
// ❌ This will throw an error
102102
await pluginSystem.unloadPlugin('base-plugin');
@@ -357,8 +357,8 @@ const chartPluginDef = {
357357
const pluginSystem = new PluginSystem();
358358

359359
async function initPlugins() {
360-
await pluginSystem.loadPlugin(gridPluginDef);
361-
await pluginSystem.loadPlugin(chartPluginDef);
360+
await pluginSystem.loadPlugin(gridPluginDef, ComponentRegistry);
361+
await pluginSystem.loadPlugin(chartPluginDef, ComponentRegistry);
362362

363363
// All plugins are now registered and ready to use
364364
console.log('Loaded plugins:', pluginSystem.getLoadedPlugins());
@@ -387,7 +387,7 @@ interface PluginDefinition {
387387

388388
```typescript
389389
class PluginSystem {
390-
loadPlugin(plugin: PluginDefinition): Promise<void>;
390+
loadPlugin(plugin: PluginDefinition, registry: Registry): Promise<void>;
391391
unloadPlugin(name: string): Promise<void>;
392392
isLoaded(name: string): boolean;
393393
getPlugin(name: string): PluginDefinition | undefined;
@@ -452,9 +452,11 @@ If you're maintaining an existing plugin, here's how to migrate:
452452

453453
2. **Use PluginSystem for dependency management:**
454454
```typescript
455+
import { ComponentRegistry } from '@object-ui/core';
456+
455457
const pluginSystem = new PluginSystem();
456-
await pluginSystem.loadPlugin(basePlugin);
457-
await pluginSystem.loadPlugin(featurePlugin);
458+
await pluginSystem.loadPlugin(basePlugin, ComponentRegistry);
459+
await pluginSystem.loadPlugin(featurePlugin, ComponentRegistry);
458460
```
459461

460462
## Troubleshooting

‎packages/core/src/registry/PluginSystem.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,10 @@ export class PluginSystem {
2525
/**
2626
* Load a plugin into the system
2727
* @param plugin The plugin definition to load
28+
* @param registry The component registry to use for registration
2829
* @throws Error if dependencies are missing
2930
*/
30-
async loadPlugin(plugin: PluginDefinition): Promise<void> {
31+
async loadPlugin(plugin: PluginDefinition, registry: Registry): Promise<void> {
3132
// Check if already loaded
3233
if (this.loaded.has(plugin.name)) {
3334
console.warn(`Plugin "${plugin.name}" is already loaded. Skipping.`);
@@ -41,14 +42,23 @@ export class PluginSystem {
4142
}
4243
}
4344

44-
// Store plugin definition
45-
this.plugins.set(plugin.name, plugin);
45+
try {
46+
// Execute registration
47+
plugin.register(registry);
4648

47-
// Execute lifecycle hook
48-
await plugin.onLoad?.();
49+
// Store plugin definition
50+
this.plugins.set(plugin.name, plugin);
51+
52+
// Execute lifecycle hook
53+
await plugin.onLoad?.();
4954

50-
// Mark as loaded
51-
this.loaded.add(plugin.name);
55+
// Mark as loaded
56+
this.loaded.add(plugin.name);
57+
} catch (error) {
58+
// Clean up on failure
59+
this.plugins.delete(plugin.name);
60+
throw error;
61+
}
5262
}
5363

5464
/**

‎packages/core/src/registry/__tests__/PluginSystem.test.ts‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,11 @@ describe('PluginSystem', () => {
2828
}
2929
};
3030

31-
await pluginSystem.loadPlugin(plugin);
31+
await pluginSystem.loadPlugin(plugin, registry);
3232

3333
expect(pluginSystem.isLoaded('test-plugin')).toBe(true);
3434
expect(pluginSystem.getLoadedPlugins()).toContain('test-plugin');
35+
expect(registry.has('test')).toBe(true);
3536
});
3637

3738
it('should execute onLoad lifecycle hook', async () => {
@@ -43,7 +44,7 @@ describe('PluginSystem', () => {
4344
onLoad
4445
};
4546

46-
await pluginSystem.loadPlugin(plugin);
47+
await pluginSystem.loadPlugin(plugin, registry);
4748

4849
expect(onLoad).toHaveBeenCalledTimes(1);
4950
});
@@ -57,7 +58,7 @@ describe('PluginSystem', () => {
5758
onLoad
5859
};
5960

60-
await pluginSystem.loadPlugin(plugin);
61+
await pluginSystem.loadPlugin(plugin, registry);
6162

6263
expect(onLoad).toHaveBeenCalledTimes(1);
6364
});
@@ -71,8 +72,8 @@ describe('PluginSystem', () => {
7172
onLoad
7273
};
7374

74-
await pluginSystem.loadPlugin(plugin);
75-
await pluginSystem.loadPlugin(plugin);
75+
await pluginSystem.loadPlugin(plugin, registry);
76+
await pluginSystem.loadPlugin(plugin, registry);
7677

7778
expect(onLoad).toHaveBeenCalledTimes(1);
7879
});
@@ -85,7 +86,7 @@ describe('PluginSystem', () => {
8586
register: () => {}
8687
};
8788

88-
await expect(pluginSystem.loadPlugin(plugin)).rejects.toThrow(
89+
await expect(pluginSystem.loadPlugin(plugin, registry)).rejects.toThrow(
8990
'Missing dependency: base-plugin required by dependent-plugin'
9091
);
9192
});
@@ -104,8 +105,8 @@ describe('PluginSystem', () => {
104105
register: () => {}
105106
};
106107

107-
await pluginSystem.loadPlugin(basePlugin);
108-
await pluginSystem.loadPlugin(dependentPlugin);
108+
await pluginSystem.loadPlugin(basePlugin, registry);
109+
await pluginSystem.loadPlugin(dependentPlugin, registry);
109110

110111
expect(pluginSystem.isLoaded('base-plugin')).toBe(true);
111112
expect(pluginSystem.isLoaded('dependent-plugin')).toBe(true);
@@ -120,7 +121,7 @@ describe('PluginSystem', () => {
120121
onUnload
121122
};
122123

123-
await pluginSystem.loadPlugin(plugin);
124+
await pluginSystem.loadPlugin(plugin, registry);
124125
expect(pluginSystem.isLoaded('test-plugin')).toBe(true);
125126

126127
await pluginSystem.unloadPlugin('test-plugin');
@@ -143,8 +144,8 @@ describe('PluginSystem', () => {
143144
register: () => {}
144145
};
145146

146-
await pluginSystem.loadPlugin(basePlugin);
147-
await pluginSystem.loadPlugin(dependentPlugin);
147+
await pluginSystem.loadPlugin(basePlugin, registry);
148+
await pluginSystem.loadPlugin(dependentPlugin, registry);
148149

149150
await expect(pluginSystem.unloadPlugin('base-plugin')).rejects.toThrow(
150151
'Cannot unload plugin "base-plugin" - plugin "dependent-plugin" depends on it'
@@ -164,7 +165,7 @@ describe('PluginSystem', () => {
164165
register: () => {}
165166
};
166167

167-
await pluginSystem.loadPlugin(plugin);
168+
await pluginSystem.loadPlugin(plugin, registry);
168169

169170
const retrieved = pluginSystem.getPlugin('test-plugin');
170171
expect(retrieved).toBe(plugin);
@@ -183,12 +184,41 @@ describe('PluginSystem', () => {
183184
register: () => {}
184185
};
185186

186-
await pluginSystem.loadPlugin(plugin1);
187-
await pluginSystem.loadPlugin(plugin2);
187+
await pluginSystem.loadPlugin(plugin1, registry);
188+
await pluginSystem.loadPlugin(plugin2, registry);
188189

189190
const allPlugins = pluginSystem.getAllPlugins();
190191
expect(allPlugins).toHaveLength(2);
191192
expect(allPlugins).toContain(plugin1);
192193
expect(allPlugins).toContain(plugin2);
193194
});
195+
196+
it('should call register function with registry', async () => {
197+
const registerFn = vi.fn();
198+
const plugin: PluginDefinition = {
199+
name: 'test-plugin',
200+
version: '1.0.0',
201+
register: registerFn
202+
};
203+
204+
await pluginSystem.loadPlugin(plugin, registry);
205+
206+
expect(registerFn).toHaveBeenCalledWith(registry);
207+
expect(registerFn).toHaveBeenCalledTimes(1);
208+
});
209+
210+
it('should cleanup on registration failure', async () => {
211+
const plugin: PluginDefinition = {
212+
name: 'failing-plugin',
213+
version: '1.0.0',
214+
register: () => {
215+
throw new Error('Registration failed');
216+
}
217+
};
218+
219+
await expect(pluginSystem.loadPlugin(plugin, registry)).rejects.toThrow('Registration failed');
220+
221+
expect(pluginSystem.isLoaded('failing-plugin')).toBe(false);
222+
expect(pluginSystem.getPlugin('failing-plugin')).toBeUndefined();
223+
});
194224
});

‎packages/create-plugin/src/index.ts‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,18 @@ async function createPlugin(pluginName?: string, options: PluginOptions = {}) {
3434
type: 'text',
3535
name: 'name',
3636
message: 'Plugin name (without prefix):',
37-
validate: (value) => value.length > 0 || 'Plugin name is required'
37+
validate: (value) => {
38+
if (value.length === 0) return 'Plugin name is required';
39+
// Validate package name format
40+
if (!/^[a-z0-9-]+$/.test(value)) {
41+
return 'Plugin name must contain only lowercase letters, numbers, and hyphens';
42+
}
43+
// Prevent path traversal
44+
if (value.includes('..') || value.includes('/') || value.includes('\\')) {
45+
return 'Plugin name cannot contain path separators or ".."';
46+
}
47+
return true;
48+
}
3849
});
3950
pluginName = response.name;
4051

@@ -44,11 +55,29 @@ async function createPlugin(pluginName?: string, options: PluginOptions = {}) {
4455
}
4556
}
4657

58+
// Validate plugin name format and security
59+
if (!/^[a-z0-9-]+$/.test(pluginName)) {
60+
console.log(chalk.red('\n❌ Plugin name must contain only lowercase letters, numbers, and hyphens'));
61+
process.exit(1);
62+
}
63+
64+
if (pluginName.includes('..') || pluginName.includes('/') || pluginName.includes('\\') || path.isAbsolute(pluginName)) {
65+
console.log(chalk.red('\n❌ Invalid plugin name: path traversal detected'));
66+
process.exit(1);
67+
}
68+
4769
// Ensure plugin name doesn't include the plugin- prefix
48-
const cleanName = pluginName.replace(/^plugin-/, '');
70+
const cleanName = pluginName.replace(/^plugin-/, '').replace(/^-+|-+$/g, '').replace(/-{2,}/g, '-');
71+
72+
if (cleanName.length === 0) {
73+
console.log(chalk.red('\n❌ Plugin name cannot be empty after sanitization'));
74+
process.exit(1);
75+
}
76+
4977
const fullPackageName = `plugin-${cleanName}`;
5078
const pascalCaseName = cleanName
5179
.split('-')
80+
.filter(part => part.length > 0)
5281
.map(part => part.charAt(0).toUpperCase() + part.slice(1))
5382
.join('');
5483

‎packages/react/src/LazyPluginLoader.tsx‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,17 @@ export interface LazyPluginOptions {
3636
* );
3737
* ```
3838
*/
39-
export function createLazyPlugin<P = any>(
39+
export function createLazyPlugin<P extends object = any>(
4040
importFn: () => Promise<{ default: React.ComponentType<P> }>,
4141
options?: LazyPluginOptions
4242
): React.ComponentType<P> {
4343
const LazyComponent = lazy(importFn);
4444

45-
return (props: P) => (
45+
const PluginWrapper: React.FC<P> = (props) => (
4646
<Suspense fallback={options?.fallback || null}>
47-
<LazyComponent {...(props as any)} />
47+
<LazyComponent {...props} />
4848
</Suspense>
4949
);
50+
51+
return PluginWrapper;
5052
}

0 commit comments

Comments
 (0)