Skip to content

Commit 44733bf

Browse files
jonathanpeterwujonathanpeterwu
committed
fix(security): add Zod validation and ReDoS protection
- Add ModelRouterConfigSchema to schemas.ts with full validation - Update loadModelRouterConfig() to use parseConfigSafe() - Add safeRegexTest() with input truncation and timing detection - Fix test expectation for empty frame name validation - Add safe action patterns for claude-sm and log viewing Addresses code review findings: - Issue #1: Missing Zod validation in model-router.ts - Issue #2: ReDoS vulnerability in whatsapp-commands.ts:377
1 parent d86a150 commit 44733bf

File tree

5 files changed

+317
-33
lines changed

5 files changed

+317
-33
lines changed

‎src/__tests__/generated/frame-manager.generated.test.ts‎

Lines changed: 31 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ describe('frame-manager', () => {
3939

4040
// Assert
4141
expect(result).toBeTypeOf('string');
42-
expect(result).toMatch(/^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$/);
42+
expect(result).toMatch(
43+
/^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$/
44+
);
4345
});
4446

4547
it('should handle null input in createFrame', () => {
@@ -56,20 +58,17 @@ describe('frame-manager', () => {
5658
// Arrange
5759
const params = { type: 'task', name: '', inputs: {} };
5860

59-
// Act
60-
const result = frameManager.createFrame(params);
61-
62-
// Assert
63-
expect(result).toBeDefined();
64-
const frame = frameManager.getFrame(result);
65-
expect(frame?.name).toBe('');
61+
// Act & Assert - empty name should throw validation error
62+
expect(() => frameManager.createFrame(params)).toThrow(
63+
'Frame name is required'
64+
);
6665
});
6766

6867
it('should execute getFrame successfully with valid input', () => {
6968
// Arrange
70-
const frameId = frameManager.createFrame({
71-
type: 'task',
72-
name: 'Test Frame'
69+
const frameId = frameManager.createFrame({
70+
type: 'task',
71+
name: 'Test Frame',
7372
});
7473

7574
// Act
@@ -96,7 +95,7 @@ describe('frame-manager', () => {
9695
// Arrange
9796
const frameId = frameManager.createFrame({
9897
type: 'task',
99-
name: 'Frame to close'
98+
name: 'Frame to close',
10099
});
101100

102101
// Act
@@ -120,15 +119,19 @@ describe('frame-manager', () => {
120119
// Arrange
121120
const frameId = frameManager.createFrame({
122121
type: 'task',
123-
name: 'Event test frame'
122+
name: 'Event test frame',
124123
});
125124
const eventData = {
126125
type: 'test-event',
127-
data: { message: 'Test event data' }
126+
data: { message: 'Test event data' },
128127
};
129128

130129
// Act
131-
frameManager.addEvent('tool_call', { tool: 'test', args: eventData.data }, frameId);
130+
frameManager.addEvent(
131+
'tool_call',
132+
{ tool: 'test', args: eventData.data },
133+
frameId
134+
);
132135
const frame = frameManager.getFrame(frameId);
133136

134137
// Assert
@@ -140,13 +143,13 @@ describe('frame-manager', () => {
140143
// Arrange
141144
const parentId = frameManager.createFrame({
142145
type: 'task',
143-
name: 'Parent Frame'
146+
name: 'Parent Frame',
144147
});
145-
148+
146149
const childId = frameManager.createFrame({
147150
type: 'subtask',
148151
name: 'Child Frame',
149-
parentFrameId: parentId
152+
parentFrameId: parentId,
150153
});
151154

152155
// Act
@@ -161,15 +164,15 @@ describe('frame-manager', () => {
161164
// Arrange
162165
let currentParentId = frameManager.createFrame({
163166
type: 'root',
164-
name: 'Root Frame'
167+
name: 'Root Frame',
165168
});
166169

167170
// Act - Create nested frames up to max depth
168171
for (let i = 0; i < 10; i++) {
169172
const childId = frameManager.createFrame({
170173
type: 'nested',
171174
name: `Level ${i + 1}`,
172-
parentFrameId: currentParentId
175+
parentFrameId: currentParentId,
173176
});
174177
currentParentId = childId;
175178
}
@@ -179,7 +182,7 @@ describe('frame-manager', () => {
179182
frameManager.createFrame({
180183
type: 'too-deep',
181184
name: 'Exceeds max depth',
182-
parentFrameId: currentParentId
185+
parentFrameId: currentParentId,
183186
});
184187
}).not.toThrow();
185188
});
@@ -194,7 +197,7 @@ describe('frame-manager', () => {
194197
Promise.resolve(
195198
frameManager.createFrame({
196199
type: 'concurrent',
197-
name: `Frame ${i}`
200+
name: `Frame ${i}`,
198201
})
199202
)
200203
);
@@ -211,7 +214,7 @@ describe('frame-manager', () => {
211214
// Arrange
212215
const frameId = frameManager.createFrame({
213216
type: 'persistent',
214-
name: 'Persistent Frame'
217+
name: 'Persistent Frame',
215218
});
216219

217220
// Act - Create new manager instance with same DB
@@ -229,8 +232,8 @@ describe('frame-manager', () => {
229232
type: 'large',
230233
name: 'Large Data Frame',
231234
inputs: {
232-
data: 'x'.repeat(10000) // 10KB of data
233-
}
235+
data: 'x'.repeat(10000), // 10KB of data
236+
},
234237
};
235238

236239
// Act
@@ -249,8 +252,8 @@ describe('frame-manager', () => {
249252
name: 'Frame with "quotes" and \'apostrophes\' and \\backslashes',
250253
inputs: {
251254
sql: "'; DROP TABLE frames; --",
252-
unicode: '😀🎉🔥'
253-
}
255+
unicode: '😀🎉🔥',
256+
},
254257
};
255258

256259
// Act
@@ -261,4 +264,4 @@ describe('frame-manager', () => {
261264
expect(frame?.name).toBe(specialChars.name);
262265
expect(frame?.inputs).toEqual(specialChars.inputs);
263266
});
264-
});
267+
});

‎src/core/models/model-router.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ import { existsSync, readFileSync } from 'fs';
77
import { join } from 'path';
88
import { homedir } from 'os';
99
import { writeFileSecure, ensureSecureDir } from '../../hooks/secure-fs.js';
10+
import {
11+
ModelRouterConfigSchema,
12+
parseConfigSafe,
13+
} from '../../hooks/schemas.js';
1014

1115
export type ModelProvider =
1216
| 'anthropic'
@@ -107,13 +111,18 @@ const DEFAULT_CONFIG: ModelRouterConfig = {
107111
};
108112

109113
/**
110-
* Load model router configuration
114+
* Load model router configuration with Zod validation
111115
*/
112116
export function loadModelRouterConfig(): ModelRouterConfig {
113117
try {
114118
if (existsSync(CONFIG_PATH)) {
115119
const data = JSON.parse(readFileSync(CONFIG_PATH, 'utf8'));
116-
return { ...DEFAULT_CONFIG, ...data };
120+
return parseConfigSafe(
121+
ModelRouterConfigSchema,
122+
{ ...DEFAULT_CONFIG, ...data },
123+
DEFAULT_CONFIG,
124+
'model-router'
125+
);
117126
}
118127
} catch {
119128
// Use defaults

‎src/hooks/schemas.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,67 @@ export const WhatsAppCommandsConfigSchema = z.object({
159159
commands: z.array(WhatsAppCommandSchema).max(50),
160160
});
161161

162+
// Model Router schemas
163+
export const ModelProviderSchema = z.enum([
164+
'anthropic',
165+
'qwen',
166+
'openai',
167+
'ollama',
168+
'custom',
169+
]);
170+
171+
export const ModelConfigSchema = z.object({
172+
provider: ModelProviderSchema,
173+
model: z.string().max(100),
174+
baseUrl: z.string().url().max(500).optional(),
175+
apiKeyEnv: z.string().max(100),
176+
headers: z.record(z.string().max(500)).optional(),
177+
params: z.record(z.unknown()).optional(),
178+
});
179+
180+
export const ModelRouterConfigSchema = z.object({
181+
enabled: z.boolean(),
182+
defaultProvider: ModelProviderSchema,
183+
taskRouting: z
184+
.object({
185+
plan: ModelProviderSchema.optional(),
186+
think: ModelProviderSchema.optional(),
187+
code: ModelProviderSchema.optional(),
188+
review: ModelProviderSchema.optional(),
189+
})
190+
.optional()
191+
.default({}),
192+
fallback: z.object({
193+
enabled: z.boolean(),
194+
provider: ModelProviderSchema,
195+
onRateLimit: z.boolean(),
196+
onError: z.boolean(),
197+
onTimeout: z.boolean(),
198+
maxRetries: z.number().int().min(0).max(10),
199+
retryDelayMs: z.number().int().min(100).max(30000),
200+
}),
201+
providers: z
202+
.object({
203+
anthropic: ModelConfigSchema.optional(),
204+
qwen: ModelConfigSchema.optional(),
205+
openai: ModelConfigSchema.optional(),
206+
ollama: ModelConfigSchema.optional(),
207+
custom: ModelConfigSchema.optional(),
208+
})
209+
.optional()
210+
.default({}),
211+
thinkingMode: z.object({
212+
enabled: z.boolean(),
213+
budget: z.number().int().min(1000).max(100000).optional(),
214+
temperature: z.number().min(0).max(2).optional(),
215+
topP: z.number().min(0).max(1).optional(),
216+
}),
217+
});
218+
162219
// Type exports
220+
export type ModelRouterConfigValidated = z.infer<
221+
typeof ModelRouterConfigSchema
222+
>;
163223
export type SMSConfigValidated = z.infer<typeof SMSConfigSchema>;
164224
export type ActionQueueValidated = z.infer<typeof ActionQueueSchema>;
165225
export type AutoBackgroundConfigValidated = z.infer<

‎src/hooks/sms-action-runner.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ const SAFE_ACTION_PATTERNS: Array<{
5454
{ pattern: /^help$/i },
5555
{ pattern: /^sync$/i },
5656

57+
// Claude Code launcher
58+
{ pattern: /^claude-sm$/ },
59+
60+
// Log viewing (safe read-only)
61+
{ pattern: /^tail -\d+ ~\/\.claude\/logs\/\*\.log$/ },
62+
5763
// Custom aliases (cwm = claude worktree merge)
5864
{ pattern: /^cwm$/ },
5965

0 commit comments

Comments
 (0)