fix(tools): validate user-provided regex patterns in MCP tools#40752
fix(tools): validate user-provided regex patterns in MCP tools#40752pavelfeldman merged 4 commits intomicrosoft:mainfrom
Conversation
MCP tools that accept grep/filter parameters pass them directly to new RegExp() without validation. An invalid pattern crashes the tool with an unhandled exception. Wrap in try-catch and throw a clear error message for invalid patterns. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Preserves the RegExp constructor's error message (e.g. 'Invalid regular expression: /[a-/: Unterminated character class') so users can understand why their pattern is invalid. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
pavelfeldman
left a comment
pavelfeldman
left a comment
There was a problem hiding this comment.
I'd move it to where we validate params using zod.
Move regex validation from inline try-catch in handlers to Zod .refine() on the inputSchema. This validates the pattern during parameter parsing before the handler runs. Revert inline validation in trace CLI tools (traceActions, traceRequests) since those use commander, not Zod. Also add the same validation to the CLI daemon's network command schema. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
Moved the validation to Zod Reverted the inline try-catch in trace CLI tools ( |
| inputSchema: z.object({ | ||
| static: z.boolean().default(false).describe('Whether to include successful static resources like images, fonts, scripts, etc. Defaults to false.'), | ||
| filter: z.string().optional().describe('Only return requests whose URL matches this regexp (e.g. "/api/.*user").'), | ||
| filter: z.string().optional().refine(v => { if (!v) return true; try { new RegExp(v); return true; } catch { return false; } }, { message: 'Invalid regular expression' }).describe('Only return requests whose URL matches this regexp (e.g. "/api/.*user").'), |
There was a problem hiding this comment.
I'm surprised linter is not yelling at you. Let's kick it into isRegexString next to isRegExp?
There was a problem hiding this comment.
Also, this typically would warrant a test.
There was a problem hiding this comment.
Extracted isRegexString into @isomorphic/rtti next to isRegExp. The Zod schema now uses .refine(v => !v || isRegexString(v)).
Also reverted the cli-daemon and trace CLI changes since validation is handled at the tool schema level.
There was a problem hiding this comment.
Added a test in tests/mcp/network.spec.ts that verifies invalid regex filter ([invalid() returns an error. All 14 network tests pass.
Add isRegexString to @isomorphic/rtti next to isRegExp. Use it in the network tool's Zod schema to validate regex filter strings. Revert cli-daemon commands.ts and trace CLI tools to original since validation is handled at the tool schema level. Add test that verifies invalid regex filter is rejected. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Test results for "MCP"5 failed 7052 passed, 1068 skipped Merge workflow run. |
Test results for "tests 1"3 flaky41703 passed, 850 skipped Merge workflow run. |
2 participants
Summary
new RegExp()without validation[a-) crashes the tool with an unhandled exceptionAffected tools: trace actions grep, trace requests grep, network filter.
Failure scenario: An MCP client sends a grep/filter parameter with invalid regex syntax (e.g., unclosed bracket
[a-). Thenew RegExp()constructor throws aSyntaxErrorthat propagates unhandled, crashing the tool handler instead of returning a user-friendly error.Origin: The grep/filter parameters were introduced in #39771 (2026-03-20) by Pavel Feldman when trace CLI handlers were extracted into separate files.