Skip to content

Commit 9a4d0e1

Browse files
MuriloFPMuriloFP
committed
fix(mcp): address critical review feedback for resource auto-approval
- Fix UI pattern inconsistency in McpResourceRow to match McpToolRow - Secure URI pattern matching with validation and timeout protection - Add comprehensive error handling to toggleResourceAlwaysAllow - Update tests to match secure implementation patterns - Add alwaysAllowResources field to BaseConfigSchema validation Addresses review feedback from PR #5709
1 parent aa6bad8 commit 9a4d0e1

File tree

5 files changed

+135
-68
lines changed

5 files changed

+135
-68
lines changed

‎src/core/webview/webviewMessageHandler.ts‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -883,14 +883,31 @@ export const webviewMessageHandler = async (
883883
}
884884
case "toggleResourceAlwaysAllow": {
885885
try {
886-
await provider
887-
.getMcpHub()
888-
?.toggleResourceAlwaysAllow(
889-
message.serverName!,
890-
message.source as "global" | "project",
891-
message.resourceUri!,
892-
Boolean(message.alwaysAllow),
893-
)
886+
// Validate required parameters
887+
if (!message.serverName) {
888+
throw new Error("Server name is required")
889+
}
890+
if (!message.resourceUri) {
891+
throw new Error("Resource URI is required")
892+
}
893+
if (!message.source || !["global", "project"].includes(message.source)) {
894+
throw new Error("Valid source (global or project) is required")
895+
}
896+
if (message.alwaysAllow === undefined || message.alwaysAllow === null) {
897+
throw new Error("alwaysAllow flag is required")
898+
}
899+
900+
const mcpHub = provider.getMcpHub()
901+
if (!mcpHub) {
902+
throw new Error("MCP Hub is not available")
903+
}
904+
905+
await mcpHub.toggleResourceAlwaysAllow(
906+
message.serverName,
907+
message.source as "global" | "project",
908+
message.resourceUri,
909+
Boolean(message.alwaysAllow),
910+
)
894911
} catch (error) {
895912
provider.log(
896913
`Failed to toggle auto-approve for resource ${message.resourceUri}: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,

‎src/services/mcp/McpHub.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ const BaseConfigSchema = z.object({
4444
disabled: z.boolean().optional(),
4545
timeout: z.number().min(1).max(3600).optional().default(60),
4646
alwaysAllow: z.array(z.string()).default([]),
47+
alwaysAllowResources: z.array(z.string()).default([]), // URIs of resources that are always allowed
4748
watchPaths: z.array(z.string()).optional(), // paths to watch for changes and restart server
4849
disabledTools: z.array(z.string()).default([]),
4950
})

‎webview-ui/src/components/chat/ChatView.tsx‎

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,32 @@ export const MAX_IMAGES_PER_MESSAGE = 20 // Anthropic limits to 20 images
6868

6969
const isMac = navigator.platform.toUpperCase().indexOf("MAC") >= 0
7070

71+
// Helper function to validate URI format for security
72+
const isValidUriFormat = (uri: string): boolean => {
73+
// Basic URI validation - must be reasonable length and contain valid characters
74+
if (!uri || uri.length > 2048) {
75+
return false
76+
}
77+
78+
// Must contain only safe characters (alphanumeric, common URI chars)
79+
const validUriPattern = /^[a-zA-Z][a-zA-Z0-9+.-]*:[a-zA-Z0-9._~:/?#[\]@!$&'()*+,;=-]+$/
80+
if (!validUriPattern.test(uri)) {
81+
return false
82+
}
83+
84+
// Prevent common attack patterns
85+
const dangerousPatterns = [
86+
/javascript:/i,
87+
/data:/i,
88+
/vbscript:/i,
89+
/file:/i,
90+
/\.\.\//, // Path traversal
91+
/%2e%2e%2f/i, // URL encoded path traversal
92+
]
93+
94+
return !dangerousPatterns.some((pattern) => pattern.test(uri))
95+
}
96+
7197
const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewProps> = (
7298
{ isHidden, showAnnouncement, hideAnnouncement },
7399
ref,
@@ -986,14 +1012,35 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
9861012

9871013
// Check resource templates
9881014
if (server?.resourceTemplates && mcpServerUse.uri) {
1015+
// Validate URI format first
1016+
if (!isValidUriFormat(mcpServerUse.uri)) {
1017+
return false
1018+
}
1019+
9891020
for (const template of server.resourceTemplates) {
990-
// Convert template pattern to regex with proper escaping
991-
const pattern = template.uriTemplate
992-
.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") // Escape special regex chars
993-
.replace(/\\\{[^}]+\\\}/g, "[^/]+") // Match path segments, not everything
994-
const regex = new RegExp(`^${pattern}$`)
995-
if (regex.test(mcpServerUse.uri) && template.alwaysAllow) {
996-
return true
1021+
try {
1022+
// Convert template pattern to regex with more restrictive matching
1023+
const pattern = template.uriTemplate
1024+
.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") // Escape special regex chars
1025+
.replace(/\\\{[^}]+\\\}/g, "[a-zA-Z0-9._-]+") // More restrictive: alphanumeric, dots, underscores, hyphens only
1026+
1027+
// Add timeout protection for regex execution
1028+
const regex = new RegExp(`^${pattern}$`)
1029+
const startTime = Date.now()
1030+
const matches = regex.test(mcpServerUse.uri)
1031+
1032+
// Check for regex timeout (prevent ReDoS attacks)
1033+
if (Date.now() - startTime > 100) {
1034+
console.warn("URI pattern matching timeout, rejecting for security")
1035+
return false
1036+
}
1037+
1038+
if (matches && template.alwaysAllow) {
1039+
return true
1040+
}
1041+
} catch (error) {
1042+
console.warn("Invalid regex pattern in resource template:", error)
1043+
continue
9971044
}
9981045
}
9991046
}

‎webview-ui/src/components/chat/__tests__/ChatView.auto-approve.spec.tsx‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1033,15 +1033,31 @@ describe("ChatView - Auto Approval Tests", () => {
10331033
// Check resource templates
10341034
if (server?.resourceTemplates && mcpServerUse.uri) {
10351035
for (const template of server.resourceTemplates) {
1036-
// Convert template pattern to regex with proper escaping
1037-
const pattern = template.uriTemplate
1038-
.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") // Escape special regex chars
1039-
.replace(/\\\{[^}]+\\\}/g, "[^/]+") // Match path segments, not everything
1040-
const regex = new RegExp(`^${pattern}$`)
1041-
if (regex.test(mcpServerUse.uri) && template.alwaysAllow) {
1042-
// Auto-approve the request
1043-
vscode.postMessage({ type: "askResponse", askResponse: "yesButtonClicked" })
1044-
return
1036+
try {
1037+
// Convert template pattern to regex with more restrictive matching
1038+
const pattern = template.uriTemplate
1039+
.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") // Escape special regex chars
1040+
.replace(/\\\{[^}]+\\\}/g, "[a-zA-Z0-9._-]+") // More restrictive: alphanumeric, dots, underscores, hyphens only
1041+
1042+
// Add timeout protection for regex execution
1043+
const regex = new RegExp(`^${pattern}$`)
1044+
const startTime = Date.now()
1045+
const matches = regex.test(mcpServerUse.uri)
1046+
1047+
// Check for regex timeout (prevent ReDoS attacks)
1048+
if (Date.now() - startTime > 100) {
1049+
console.warn("URI pattern matching timeout, rejecting for security")
1050+
return
1051+
}
1052+
1053+
if (matches && template.alwaysAllow) {
1054+
// Auto-approve the request
1055+
vscode.postMessage({ type: "askResponse", askResponse: "yesButtonClicked" })
1056+
return
1057+
}
1058+
} catch (error) {
1059+
console.warn("Invalid regex pattern in resource template:", error)
1060+
continue
10451061
}
10461062
}
10471063
}

‎webview-ui/src/components/mcp/McpResourceRow.tsx‎

Lines changed: 30 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,32 @@ const McpResourceRow = ({
3434
}
3535

3636
return (
37-
<div
38-
key={uri}
39-
style={{
40-
padding: "3px 0",
41-
}}>
42-
<div
43-
style={{
44-
display: "flex",
45-
alignItems: "center",
46-
marginBottom: "4px",
47-
}}>
48-
<span className={`codicon codicon-symbol-file`} style={{ marginRight: "6px" }} />
49-
<span style={{ fontWeight: 500, wordBreak: "break-all" }}>{uri}</span>
37+
<div key={uri} className="py-2 border-b border-vscode-panel-border last:border-b-0">
38+
<div className="flex items-center gap-4" onClick={(e) => e.stopPropagation()}>
39+
{/* Resource URI section */}
40+
<div className="flex items-center min-w-0 flex-1">
41+
<span className="codicon codicon-symbol-file mr-2 flex-shrink-0 text-vscode-symbolIcon-fileForeground" />
42+
<span className="font-medium break-all text-vscode-foreground">{uri}</span>
43+
</div>
44+
45+
{/* Controls section */}
46+
{serverName && alwaysAllowMcp && !isInChatContext && (
47+
<div className="flex items-center flex-shrink-0">
48+
<VSCodeCheckbox
49+
checked={item.alwaysAllow}
50+
onChange={handleAlwaysAllowChange}
51+
data-resource={uri}
52+
className="text-xs">
53+
<span className="text-vscode-descriptionForeground whitespace-nowrap">
54+
{t("mcp:resource.alwaysAllow")}
55+
</span>
56+
</VSCodeCheckbox>
57+
</div>
58+
)}
5059
</div>
51-
<div
52-
style={{
53-
fontSize: "12px",
54-
opacity: 0.8,
55-
margin: "4px 0",
56-
}}>
60+
61+
{/* Description section */}
62+
<div className="mt-1 text-xs text-vscode-descriptionForeground opacity-80">
5763
{item.name && item.description
5864
? `${item.name}: ${item.description}`
5965
: !item.name && item.description
@@ -62,34 +68,14 @@ const McpResourceRow = ({
6268
? item.name
6369
: "No description"}
6470
</div>
65-
<div
66-
style={{
67-
fontSize: "12px",
68-
}}>
69-
<span style={{ opacity: 0.8 }}>Returns </span>
70-
<code
71-
style={{
72-
color: "var(--vscode-textPreformat-foreground)",
73-
background: "var(--vscode-textPreformat-background)",
74-
padding: "1px 4px",
75-
borderRadius: "3px",
76-
}}>
71+
72+
{/* MIME type section */}
73+
<div className="mt-2 text-xs">
74+
<span className="text-vscode-descriptionForeground opacity-80">Returns </span>
75+
<code className="text-vscode-textPreformat-foreground bg-vscode-textPreformat-background px-1 py-0.5 rounded">
7776
{item.mimeType || "Unknown"}
7877
</code>
7978
</div>
80-
{serverName && alwaysAllowMcp && !isInChatContext && (
81-
<div style={{ marginTop: "8px" }}>
82-
<VSCodeCheckbox
83-
checked={item.alwaysAllow}
84-
onChange={handleAlwaysAllowChange}
85-
data-resource={uri}
86-
className="text-xs">
87-
<span className="text-vscode-descriptionForeground whitespace-nowrap">
88-
{t("mcp:resource.alwaysAllow")}
89-
</span>
90-
</VSCodeCheckbox>
91-
</div>
92-
)}
9379
</div>
9480
)
9581
}

0 commit comments

Comments
 (0)