Add MCP tool trust verifier hook#6437
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a trust verifier mechanism for Model Context Protocol (MCP) tool calls, allowing policy hooks to allow, warn, or deny tool calls before execution. It also includes comprehensive unit tests verifying this behavior. A review comment identifies a potential runtime crash if the trust verifier returns null or undefined due to JavaScript's typeof null evaluating to 'object', and suggests adding a nullish check to ensure fail-secure behavior.
| const action = | ||
| typeof decision === 'boolean' ? (decision ? 'allow' : 'deny') : decision.action ?? (decision.allowed === false ? 'deny' : 'allow') | ||
|
|
||
| if (action === 'warn') { | ||
| console.warn( | ||
| `MCP trust verifier warning for tool "${toolName}"${ | ||
| typeof decision === 'object' && decision.reason ? `: ${decision.reason}` : '' | ||
| }` | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| if (action === 'deny') { | ||
| const reason = typeof decision === 'object' && decision.reason ? `: ${decision.reason}` : '' | ||
| throw new Error(`MCP tool call blocked by trust verifier for "${toolName}"${reason}`) | ||
| } |
There was a problem hiding this comment.
If the trustVerifier hook returns null or undefined, the current implementation will throw a TypeError when attempting to access properties like decision.action or decision.reason (since typeof null === 'object' evaluates to true in JavaScript). To prevent runtime crashes and ensure a fail-secure behavior, we should add a nullish check (== null) before processing the decision.
if (decision == null) {
throw new Error('MCP tool call blocked by trust verifier for "' + toolName + '": decision is null or undefined')
}
const action =
typeof decision === 'boolean' ? (decision ? 'allow' : 'deny') : decision.action ?? (decision.allowed === false ? 'deny' : 'allow')
if (action === 'warn') {
console.warn(
'MCP trust verifier warning for tool "' + toolName + '"' +
(typeof decision === 'object' && decision.reason ? ': ' + decision.reason : '')
)
return
}
if (action === 'deny') {
const reason = typeof decision === 'object' && decision.reason ? ': ' + decision.reason : ''
throw new Error('MCP tool call blocked by trust verifier for "' + toolName + '"' + reason)
}References
- In JavaScript/TypeScript, use loose equality (
== null) as a standard idiom for a 'nullish' check that covers bothnullandundefined.
|
Addressed the Gemini review in 4a59a6e: trust verifier decisions now fail closed for null/undefined, reason handling guards null objects, and a regression test verifies no MCP client is created when the verifier returns null. |
Summary
Why
This addresses the trust-boundary gap described in #6433 with an opt-in core hook and no behavior change when the verifier is not configured.
Tests