Repository navigation
fix(typescript): fail closed and add null-safety validation to McpSecurityScanner - #3440
Henry Su (hsusul) wants to merge 1 commit into
Conversation
…urityScanner Signed-off-by: Henry Su <henrysu4707@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your thoughtful contribution! You did a great job clearly documenting the problem and providing comprehensive regression tests. Before merging, please ensure:
Refer to CONTRIBUTING.md for details. |
🤖 AI Agent: test-generator — `agent-governance-typescript/src/mcp.ts`
|
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. Robust fix with minor follow-up needed.
Action items: None (no blockers). Warnings:
|
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
There was a problem hiding this comment.
Pull request overview
Hardens the TypeScript SDK’s McpSecurityScanner against malformed/untrusted MCP tool definitions by adding fail-closed behavior and null-safety so scans don’t crash host processes.
TL;DR: 2 blockers, 0 warnings. Fix #1 and #2 and this ships.
| # | Sev | Issue | Where |
|---|---|---|---|
| 1 | Block | tool_name isn’t guaranteed to be a string for untrusted inputs |
agent-governance-typescript/src/mcp.ts scan() |
| 2 | Block | Current “sanitization” doesn’t handle non-string name/description, so detectors can still throw (and force fail-closed) |
agent-governance-typescript/src/mcp.ts detector methods |
#1: Coerce/validate toolDefinition.name to a trimmed string before returning it as tool_name.
#2: Use typeof === 'string' guards (or explicit String(...)) before calling string methods/regex ops so malformed types don’t raise avoidable exceptions.
Changes:
- Made
McpToolDefinition.name/.descriptionoptional and added a fail-closedtry/catchboundary inscan(). - Added additional tests for missing fields, fail-closed behavior, and typosquatting deduplication.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
agent-governance-typescript/src/mcp.ts |
Fail-closed scan boundary; null-safety updates; stop after first typosquat match |
agent-governance-typescript/tests/mcp.test.ts |
Regression tests for null-safety, fail-closed path, and typosquat dedupe |
Comments suppressed due to low confidence (3)
agent-governance-typescript/src/mcp.ts:250
tool?.description ?? ''doesn’t protect against non-stringdescriptionvalues; ifdescriptionis an object/number,text.includes(...)will still throw. Normalizedescriptionto a string before scanning.
private detectHiddenInstructions(tool: McpToolDefinition, threats: McpThreat[]): void {
const text = tool?.description ?? '';
agent-governance-typescript/src/mcp.ts:280
tool?.description ?? ''can still yield a non-string ifdescriptionis present but not a string (untrusted input), causingtext.length/ regex tests to throw. Normalize to a string using atypeofcheck.
private detectRugPull(tool: McpToolDefinition, threats: McpThreat[]): void {
const text = tool?.description ?? '';
if (text.length <= RUG_PULL_DESCRIPTION_LENGTH) return;
agent-governance-typescript/src/mcp.ts:232
(tool?.name ?? '').toLowerCase()still throws ifnameis present but not a string (e.g.,name: 123from an untrusted server). Also, the threat message interpolatestool.namedirectly. Validate/coercenameonce and use the sanitized value for both the distance calculation and the message.
private detectTyposquatting(tool: McpToolDefinition, threats: McpThreat[]): void {
const name = (tool?.name ?? '').toLowerCase();
if (!name) return;
| scan(toolDefinition: McpToolDefinition): McpScanResult { | ||
| const threats: McpThreat[] = []; | ||
|
|
||
| this.detectToolPoisoning(toolDefinition, threats); | ||
| this.detectTyposquatting(toolDefinition, threats); | ||
| this.detectHiddenInstructions(toolDefinition, threats); | ||
| this.detectRugPull(toolDefinition, threats); | ||
|
|
||
| const risk_score = Math.min( | ||
| 100, | ||
| threats.reduce((sum, t) => sum + SEVERITY_WEIGHT[t.severity], 0), | ||
| ); | ||
|
|
||
| return { | ||
| tool_name: toolDefinition.name, | ||
| threats, | ||
| risk_score, | ||
| safe: threats.length === 0, | ||
| }; | ||
| const toolName = toolDefinition?.name ?? 'unknown'; | ||
| try { |
| private detectToolPoisoning(tool: McpToolDefinition, threats: McpThreat[]): void { | ||
| const text = tool.description; | ||
| const text = tool?.description ?? ''; | ||
| for (const pattern of POISONING_PATTERNS) { |
| description: `Tool name "${tool.name}" is suspiciously similar to known tool "${known}" (edit distance ${dist})`, | ||
| evidence: known, | ||
| }); | ||
| break; // stop after first typosquatting match to prevent duplicate threat inflation |
There was a problem hiding this comment.
the new break in detectTyposquatting stops at the first match in KNOWN_TOOL_NAMES list order, not the closest. Concrete under-report: creat_file matches read_file at distance 2 (medium, listed first) and never reaches create_file at distance 1 (high), dropping risk 75 -> 25. Python emits one finding per similar name with no dedup (mcp_security.py:956-980). Select the minimum-distance match if dedup is wanted, or drop the break.
| name?: string; | ||
| description?: string; |
There was a problem hiding this comment.
making name/description optional on the exported McpToolDefinition (re-exported from src/index.ts:27) is a type-level break for consumers reading these fields under strict mode; the runtime coalescing alone handles untrusted input, so the PR body's backward-compatible claim is inaccurate. Keep the fields required or acknowledge the typing break.
| threats: [ | ||
| { | ||
| type: McpThreatType.ToolPoisoning, | ||
| severity: 'critical', |
There was a problem hiding this comment.
nit: the catch embeds raw err.message into the returned threat description; Python logs and returns a generic message. Mirror that to avoid leaking internal error detail.
| it('fails closed when scan method encounters an unexpected exception', () => { | ||
| const tool: McpToolDefinition = { | ||
| name: 'test_tool', | ||
| description: 'Test description', | ||
| }; | ||
| // Force an error in internal detector | ||
| jest.spyOn(scanner as any, 'detectToolPoisoning').mockImplementation(() => { | ||
| throw new Error('Unexpected memory error'); | ||
| }); | ||
|
|
||
| const result = scanner.scan(tool); | ||
| expect(result.safe).toBe(false); | ||
| expect(result.risk_score).toBe(100); | ||
| expect(result.threats).toHaveLength(1); | ||
| expect(result.threats[0].severity).toBe('critical'); | ||
| expect(result.threats[0].description).toContain('Scan error — fail closed'); | ||
| }); |
There was a problem hiding this comment.
nit: the fail-closed test only forces the error via a jest mock of a private method; add one organic malformed-input case (e.g. description: 123 as any) so the catch is exercised end-to-end.
| description: `Tool name "${tool.name}" is suspiciously similar to known tool "${known}" (edit distance ${dist})`, | ||
| evidence: known, | ||
| }); | ||
| break; // stop after first typosquatting match to prevent duplicate threat inflation |
There was a problem hiding this comment.
the new break in detectTyposquatting stops at the first match in KNOWN_TOOL_NAMES list order, not the closest. Concrete under-report: creat_file matches read_file at distance 2 (medium, listed first) and never reaches create_file at distance 1 (high), dropping risk 75 -> 25. Python emits one finding per similar name with no dedup (mcp_security.py:956-980). Select the minimum-distance match if dedup is wanted, or drop the break.
| name?: string; | ||
| description?: string; |
There was a problem hiding this comment.
making name/description optional on the exported McpToolDefinition (re-exported from src/index.ts:27) is a type-level break for consumers reading these fields under strict mode; the runtime coalescing alone handles untrusted input, so the PR body's backward-compatible claim is inaccurate. Keep the fields required or acknowledge the typing break.
| threats: [ | ||
| { | ||
| type: McpThreatType.ToolPoisoning, | ||
| severity: 'critical', |
There was a problem hiding this comment.
nit: the catch embeds raw err.message into the returned threat description; Python logs and returns a generic message. Mirror that to avoid leaking internal error detail.
| it('fails closed when scan method encounters an unexpected exception', () => { | ||
| const tool: McpToolDefinition = { | ||
| name: 'test_tool', | ||
| description: 'Test description', | ||
| }; | ||
| // Force an error in internal detector | ||
| jest.spyOn(scanner as any, 'detectToolPoisoning').mockImplementation(() => { | ||
| throw new Error('Unexpected memory error'); | ||
| }); | ||
|
|
||
| const result = scanner.scan(tool); | ||
| expect(result.safe).toBe(false); | ||
| expect(result.risk_score).toBe(100); | ||
| expect(result.threats).toHaveLength(1); | ||
| expect(result.threats[0].severity).toBe('critical'); | ||
| expect(result.threats[0].description).toContain('Scan error — fail closed'); | ||
| }); |
There was a problem hiding this comment.
nit: the fail-closed test only forces the error via a jest mock of a private method; add one organic malformed-input case (e.g. description: 123 as any) so the catch is exercised end-to-end.
| description: `Tool name "${tool.name}" is suspiciously similar to known tool "${known}" (edit distance ${dist})`, | ||
| evidence: known, | ||
| }); | ||
| break; // stop after first typosquatting match to prevent duplicate threat inflation |
There was a problem hiding this comment.
the new break in detectTyposquatting stops at the first match in KNOWN_TOOL_NAMES list order, not the closest. Concrete under-report: creat_file matches read_file at distance 2 (medium, listed first) and never reaches create_file at distance 1 (high), dropping risk 75 -> 25. Python emits one finding per similar name with no dedup (mcp_security.py:956-980). Select the minimum-distance match if dedup is wanted, or drop the break.
| name?: string; | ||
| description?: string; |
There was a problem hiding this comment.
making name/description optional on the exported McpToolDefinition (re-exported from src/index.ts:27) is a type-level break for consumers reading these fields under strict mode; the runtime coalescing alone handles untrusted input, so the PR body's backward-compatible claim is inaccurate. Keep the fields required or acknowledge the typing break.
| threats: [ | ||
| { | ||
| type: McpThreatType.ToolPoisoning, | ||
| severity: 'critical', |
There was a problem hiding this comment.
nit: the catch embeds raw err.message into the returned threat description; Python logs and returns a generic message. Mirror that to avoid leaking internal error detail.
| it('fails closed when scan method encounters an unexpected exception', () => { | ||
| const tool: McpToolDefinition = { | ||
| name: 'test_tool', | ||
| description: 'Test description', | ||
| }; | ||
| // Force an error in internal detector | ||
| jest.spyOn(scanner as any, 'detectToolPoisoning').mockImplementation(() => { | ||
| throw new Error('Unexpected memory error'); | ||
| }); | ||
|
|
||
| const result = scanner.scan(tool); | ||
| expect(result.safe).toBe(false); | ||
| expect(result.risk_score).toBe(100); | ||
| expect(result.threats).toHaveLength(1); | ||
| expect(result.threats[0].severity).toBe('critical'); | ||
| expect(result.threats[0].description).toContain('Scan error — fail closed'); | ||
| }); |
There was a problem hiding this comment.
nit: the fail-closed test only forces the error via a jest mock of a private method; add one organic malformed-input case (e.g. description: 123 as any) so the catch is exercised end-to-end.
| risk_score, | ||
| safe: threats.length === 0, | ||
| }; | ||
| const toolName = toolDefinition?.name ?? 'unknown'; |
There was a problem hiding this comment.
On main, scan({}) and scan(undefined) raise a TypeError. On this branch both return safe true with risk_score 0, so the catch block never runs for the malformed definitions this change targets. A tool definition with no description to scan should return unsafe.
|
Several requests remain unaddressed for over a week, so I'm closing the PR. Feel free to reopen once they're addressed. |
Problem and Affected Boundary
In
@microsoft/agent-governance-sdk(agent-governance-typescript/),McpSecurityScannerscreens Model Context Protocol (MCP) tool definitions for security threats (tool poisoning, typosquatting, hidden instructions, rug pulls). When processing untrusted tool definitions from external MCP tool servers, definitions missing expected string properties (e.g.descriptionornameomitted ornull) or malformed structures causedMcpSecurityScanner.scan()to crash with unhandledTypeErrorexceptions (such asCannot read properties of undefined (reading 'includes')orCannot read properties of undefined (reading 'toLowerCase')).Furthermore, unlike the Python
MCPSecurityScanner(which catches unexpected scan exceptions and fails closed with aCRITICALthreat finding), the TypeScript implementation lacked a fail-closed error boundary. Additionally,detectTyposquattingdid not break after finding a match, causing a tool name matching multiple known tools within edit distance 2 to emit duplicate threat findings and artificially inflate the risk score.Reproduction
descriptionornametoMcpSecurityScanner.scan():Expected vs Actual Behavior
McpSecurityScanner.scan()safely handles missing/null property fields without throwing runtime exceptions. If an unexpected error occurs during scanning, it fails closed returningsafe = false,risk_score = 100, and a critical threat finding.McpSecurityScanner.scan()threw an unhandledTypeError, crashing host process / agent gateway routines.Root Cause
McpToolDefinitionrequired non-optionalnameanddescriptionstrings, but internal private detection methods (detectToolPoisoning,detectTyposquatting,detectHiddenInstructions,detectRugPull) directly accessedtool.descriptionandtool.namewithout null-coalescing or fallback checks.scan()executed detection steps without a try-catch error boundary.detectTyposquattingdid not exit the loop after recording a typosquatting match.Implementation
McpToolDefinitioninterface to makenameanddescriptionoptional (name?: string; description?: string;).McpSecurityScanner.scan()in a try-catch fail-closed block that catches unexpected errors and returns a fail-closed result (safe: false,risk_score: 100, criticalToolPoisoningthreat).tool?.name ?? '',tool?.description ?? '') in all private detection methods.breakafter the first typosquatting match indetectTyposquatting.Regression Coverage
Added 4 unit tests in
agent-governance-typescript/tests/mcp.test.ts:handles tool definition with missing description without throwinghandles tool definition with missing name without throwingfails closed when scan method encounters an unexpected exceptionemits at most one typosquatting threat per toolExact Validation Commands and Results
Compatibility Considerations
Backward compatible. Non-breaking bug fix for
McpSecurityScannerin TypeScript SDK.Security Disclosure Assessment
Ordinary correctness and robustness defense-in-depth bug fix in scanner exception handling and null validation. Does not expose or exploit a remote vulnerability.
Confirmation
No unrelated files, formatting changes, or lockfile churn are included.