-
Notifications
You must be signed in to change notification settings - Fork 301
Ensure allowed tool filter for mcp-servers section #19801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d5ce370
8aca4f8
98d2568
3296539
62ed5cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,6 +187,184 @@ func TestHasMCPConfigDetection(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestMCPServersAllowedToolFilterCompilation verifies that the allowed tool filter in | ||
| // mcp-servers section is properly compiled into the "tools" field in the output. | ||
| func TestMCPServersAllowedToolFilterCompilation(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| workflowContent string | ||
| serverName string | ||
| expectedContent []string | ||
| unexpectedInServer []string | ||
| }{ | ||
| { | ||
| name: "copilot - http mcp server with specific allowed tools", | ||
| workflowContent: `--- | ||
| on: | ||
| workflow_dispatch: | ||
| strict: false | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| mcp-servers: | ||
| my-api: | ||
| type: http | ||
| url: https://api.example.com/mcp | ||
| allowed: | ||
| - get_data | ||
| - list_items | ||
| --- | ||
|
|
||
| Test workflow. | ||
| `, | ||
| serverName: `"my-api"`, | ||
| expectedContent: []string{`"get_data"`, `"list_items"`}, | ||
| unexpectedInServer: []string{`"*"`}, | ||
| }, | ||
| { | ||
| name: "copilot - stdio mcp server with specific allowed tools", | ||
| workflowContent: `--- | ||
| on: | ||
| workflow_dispatch: | ||
| strict: false | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| mcp-servers: | ||
| my-tool: | ||
| container: example/tool:latest | ||
| allowed: | ||
| - run_query | ||
| - fetch_results | ||
| --- | ||
|
|
||
| Test workflow. | ||
| `, | ||
| serverName: `"my-tool"`, | ||
| expectedContent: []string{`"run_query"`, `"fetch_results"`}, | ||
| unexpectedInServer: []string{`"*"`}, | ||
| }, | ||
| { | ||
| name: "copilot - mcp server with no allowed field defaults to wildcard", | ||
| workflowContent: `--- | ||
| on: | ||
| workflow_dispatch: | ||
| strict: false | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| mcp-servers: | ||
| my-api: | ||
| type: http | ||
| url: https://api.example.com/mcp | ||
| --- | ||
|
|
||
| Test workflow. | ||
| `, | ||
| serverName: `"my-api"`, | ||
| expectedContent: []string{`"*"`}, | ||
| unexpectedInServer: []string{}, | ||
| }, | ||
| { | ||
| name: "claude - http mcp server with specific allowed tools passes through", | ||
| workflowContent: `--- | ||
| on: | ||
| workflow_dispatch: | ||
| strict: false | ||
| permissions: | ||
| contents: read | ||
| engine: claude | ||
| mcp-servers: | ||
| my-api: | ||
| type: http | ||
| url: https://api.example.com/mcp | ||
| allowed: | ||
| - get_data | ||
| - list_items | ||
| --- | ||
|
|
||
| Test workflow. | ||
| `, | ||
| serverName: `"my-api"`, | ||
| expectedContent: []string{`"get_data"`, `"list_items"`}, | ||
| unexpectedInServer: []string{`"*"`}, | ||
| }, | ||
| { | ||
| name: "claude - http mcp server with no allowed field has no tools filter", | ||
| workflowContent: `--- | ||
| on: | ||
| workflow_dispatch: | ||
| strict: false | ||
| permissions: | ||
| contents: read | ||
| engine: claude | ||
| mcp-servers: | ||
| my-api: | ||
| type: http | ||
| url: https://api.example.com/mcp | ||
| --- | ||
|
|
||
| Test workflow. | ||
| `, | ||
| serverName: `"my-api"`, | ||
| expectedContent: []string{`"url":`}, | ||
| unexpectedInServer: []string{`"tools":`}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| tmpFile, err := os.CreateTemp("", "test-allowed-filter-*.md") | ||
| if err != nil { | ||
| t.Fatalf("Failed to create temp file: %v", err) | ||
| } | ||
| defer os.Remove(tmpFile.Name()) | ||
|
|
||
| if _, err := tmpFile.WriteString(tt.workflowContent); err != nil { | ||
| t.Fatalf("Failed to write to temp file: %v", err) | ||
| } | ||
| tmpFile.Close() | ||
|
|
||
| compiler := NewCompiler() | ||
| compiler.SetSkipValidation(true) | ||
|
|
||
| workflowData, err := compiler.ParseWorkflowFile(tmpFile.Name()) | ||
| if err != nil { | ||
| t.Fatalf("Failed to parse workflow file: %v", err) | ||
| } | ||
|
|
||
| yamlContent, err := compiler.generateYAML(workflowData, tmpFile.Name()) | ||
| if err != nil { | ||
| t.Fatalf("Failed to generate YAML: %v", err) | ||
| } | ||
|
|
||
| // Find the server-specific block in the YAML | ||
| serverIndex := strings.Index(yamlContent, tt.serverName) | ||
| if serverIndex == -1 { | ||
| t.Fatalf("Could not find server %s in generated YAML", tt.serverName) | ||
| } | ||
|
|
||
| // Extract the server block (next 500 chars should be sufficient) | ||
| endIdx := min(serverIndex+500, len(yamlContent)) | ||
| serverBlock := yamlContent[serverIndex:endIdx] | ||
|
|
||
| for _, content := range tt.expectedContent { | ||
| if !strings.Contains(serverBlock, content) { | ||
| t.Errorf("Expected %q in server block for %s, but not found.\nServer block:\n%s", | ||
| content, tt.serverName, serverBlock) | ||
| } | ||
| } | ||
|
|
||
| for _, content := range tt.unexpectedInServer { | ||
| if strings.Contains(serverBlock, content) { | ||
| t.Errorf("Unexpected %q found in server block for %s.\nServer block:\n%s", | ||
| content, tt.serverName, serverBlock) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestDevModeAgenticWorkflowsContainer verifies that the agentic-workflows MCP server | ||
| // uses the locally built Docker image in dev mode instead of alpine:latest | ||
| func TestDevModeAgenticWorkflowsContainer(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comprehensive test coverage with 5 test cases covering HTTP, stdio, no-allowed-field defaults, and both Claude and Copilot engines. The test for the Claude case without |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,16 +122,12 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma | |
| // TOML format for HTTP MCP servers uses url and http_headers | ||
| propertyOrder = []string{"url", "http_headers"} | ||
| } else { | ||
| // JSON format - include copilot fields if required | ||
| if renderer.RequiresCopilotFields { | ||
| // For HTTP MCP with secrets in headers, env passthrough is needed | ||
| if len(headerSecrets) > 0 { | ||
| propertyOrder = []string{"type", "url", "headers", "tools", "env"} | ||
| } else { | ||
| propertyOrder = []string{"type", "url", "headers", "tools"} | ||
| } | ||
| // JSON format - include tools field for MCP gateway tool filtering (all engines) | ||
| // For HTTP MCP with secrets in headers, env passthrough is needed | ||
| if len(headerSecrets) > 0 { | ||
| propertyOrder = []string{"type", "url", "headers", "tools", "env"} | ||
| } else { | ||
|
Comment on lines
+125
to
129
|
||
| propertyOrder = []string{"type", "url", "headers"} | ||
| propertyOrder = []string{"type", "url", "headers", "tools"} | ||
| } | ||
| } | ||
| default: | ||
|
|
@@ -147,8 +143,11 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma | |
| // Include type field only for engines that require copilot fields | ||
| existingProperties = append(existingProperties, prop) | ||
| case "tools": | ||
| // Include tools field only for engines that require copilot fields | ||
| if renderer.RequiresCopilotFields { | ||
| // Include tools field for JSON format when: | ||
| // - RequiresCopilotFields (Copilot always renders it; when Allowed is empty, the | ||
| // rendering code below defaults to the "*" wildcard) | ||
| // - OR allowed tools are explicitly specified (pass the filter to the MCP gateway) | ||
| if renderer.RequiresCopilotFields || len(mcpConfig.Allowed) > 0 { | ||
| existingProperties = append(existingProperties, prop) | ||
| } | ||
| case "container": | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The condition |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scenarios here cover allowed tool compilation for Copilot (http/stdio) and Claude (http), but there isn’t a case for a non-Copilot stdio MCP server with an explicit allowed list. Adding a Claude/Gemini/Codex stdio case would better lock in the behavior from the updated tools-field inclusion gate.