Skip to content

Commit d3cbe35

Browse files
authored
Ensure allowed tool filter for mcp-servers section (#19801)
1 parent b00407b commit d3cbe35

File tree

3 files changed

+193
-11
lines changed

3 files changed

+193
-11
lines changed

.changeset/patch-propagate-mcp-allowed-filter.md

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/workflow/mcp_config_compilation_test.go

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,184 @@ func TestHasMCPConfigDetection(t *testing.T) {
187187
}
188188
}
189189

190+
// TestMCPServersAllowedToolFilterCompilation verifies that the allowed tool filter in
191+
// mcp-servers section is properly compiled into the "tools" field in the output.
192+
func TestMCPServersAllowedToolFilterCompilation(t *testing.T) {
193+
tests := []struct {
194+
name string
195+
workflowContent string
196+
serverName string
197+
expectedContent []string
198+
unexpectedInServer []string
199+
}{
200+
{
201+
name: "copilot - http mcp server with specific allowed tools",
202+
workflowContent: `---
203+
on:
204+
workflow_dispatch:
205+
strict: false
206+
permissions:
207+
contents: read
208+
engine: copilot
209+
mcp-servers:
210+
my-api:
211+
type: http
212+
url: https://api.example.com/mcp
213+
allowed:
214+
- get_data
215+
- list_items
216+
---
217+
218+
Test workflow.
219+
`,
220+
serverName: `"my-api"`,
221+
expectedContent: []string{`"get_data"`, `"list_items"`},
222+
unexpectedInServer: []string{`"*"`},
223+
},
224+
{
225+
name: "copilot - stdio mcp server with specific allowed tools",
226+
workflowContent: `---
227+
on:
228+
workflow_dispatch:
229+
strict: false
230+
permissions:
231+
contents: read
232+
engine: copilot
233+
mcp-servers:
234+
my-tool:
235+
container: example/tool:latest
236+
allowed:
237+
- run_query
238+
- fetch_results
239+
---
240+
241+
Test workflow.
242+
`,
243+
serverName: `"my-tool"`,
244+
expectedContent: []string{`"run_query"`, `"fetch_results"`},
245+
unexpectedInServer: []string{`"*"`},
246+
},
247+
{
248+
name: "copilot - mcp server with no allowed field defaults to wildcard",
249+
workflowContent: `---
250+
on:
251+
workflow_dispatch:
252+
strict: false
253+
permissions:
254+
contents: read
255+
engine: copilot
256+
mcp-servers:
257+
my-api:
258+
type: http
259+
url: https://api.example.com/mcp
260+
---
261+
262+
Test workflow.
263+
`,
264+
serverName: `"my-api"`,
265+
expectedContent: []string{`"*"`},
266+
unexpectedInServer: []string{},
267+
},
268+
{
269+
name: "claude - http mcp server with specific allowed tools passes through",
270+
workflowContent: `---
271+
on:
272+
workflow_dispatch:
273+
strict: false
274+
permissions:
275+
contents: read
276+
engine: claude
277+
mcp-servers:
278+
my-api:
279+
type: http
280+
url: https://api.example.com/mcp
281+
allowed:
282+
- get_data
283+
- list_items
284+
---
285+
286+
Test workflow.
287+
`,
288+
serverName: `"my-api"`,
289+
expectedContent: []string{`"get_data"`, `"list_items"`},
290+
unexpectedInServer: []string{`"*"`},
291+
},
292+
{
293+
name: "claude - http mcp server with no allowed field has no tools filter",
294+
workflowContent: `---
295+
on:
296+
workflow_dispatch:
297+
strict: false
298+
permissions:
299+
contents: read
300+
engine: claude
301+
mcp-servers:
302+
my-api:
303+
type: http
304+
url: https://api.example.com/mcp
305+
---
306+
307+
Test workflow.
308+
`,
309+
serverName: `"my-api"`,
310+
expectedContent: []string{`"url":`},
311+
unexpectedInServer: []string{`"tools":`},
312+
},
313+
}
314+
315+
for _, tt := range tests {
316+
t.Run(tt.name, func(t *testing.T) {
317+
tmpFile, err := os.CreateTemp("", "test-allowed-filter-*.md")
318+
if err != nil {
319+
t.Fatalf("Failed to create temp file: %v", err)
320+
}
321+
defer os.Remove(tmpFile.Name())
322+
323+
if _, err := tmpFile.WriteString(tt.workflowContent); err != nil {
324+
t.Fatalf("Failed to write to temp file: %v", err)
325+
}
326+
tmpFile.Close()
327+
328+
compiler := NewCompiler()
329+
compiler.SetSkipValidation(true)
330+
331+
workflowData, err := compiler.ParseWorkflowFile(tmpFile.Name())
332+
if err != nil {
333+
t.Fatalf("Failed to parse workflow file: %v", err)
334+
}
335+
336+
yamlContent, err := compiler.generateYAML(workflowData, tmpFile.Name())
337+
if err != nil {
338+
t.Fatalf("Failed to generate YAML: %v", err)
339+
}
340+
341+
// Find the server-specific block in the YAML
342+
serverIndex := strings.Index(yamlContent, tt.serverName)
343+
if serverIndex == -1 {
344+
t.Fatalf("Could not find server %s in generated YAML", tt.serverName)
345+
}
346+
347+
// Extract the server block (next 500 chars should be sufficient)
348+
endIdx := min(serverIndex+500, len(yamlContent))
349+
serverBlock := yamlContent[serverIndex:endIdx]
350+
351+
for _, content := range tt.expectedContent {
352+
if !strings.Contains(serverBlock, content) {
353+
t.Errorf("Expected %q in server block for %s, but not found.\nServer block:\n%s",
354+
content, tt.serverName, serverBlock)
355+
}
356+
}
357+
358+
for _, content := range tt.unexpectedInServer {
359+
if strings.Contains(serverBlock, content) {
360+
t.Errorf("Unexpected %q found in server block for %s.\nServer block:\n%s",
361+
content, tt.serverName, serverBlock)
362+
}
363+
}
364+
})
365+
}
366+
}
367+
190368
// TestDevModeAgenticWorkflowsContainer verifies that the agentic-workflows MCP server
191369
// uses the locally built Docker image in dev mode instead of alpine:latest
192370
func TestDevModeAgenticWorkflowsContainer(t *testing.T) {

pkg/workflow/mcp_config_custom.go

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -122,16 +122,12 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma
122122
// TOML format for HTTP MCP servers uses url and http_headers
123123
propertyOrder = []string{"url", "http_headers"}
124124
} else {
125-
// JSON format - include copilot fields if required
126-
if renderer.RequiresCopilotFields {
127-
// For HTTP MCP with secrets in headers, env passthrough is needed
128-
if len(headerSecrets) > 0 {
129-
propertyOrder = []string{"type", "url", "headers", "tools", "env"}
130-
} else {
131-
propertyOrder = []string{"type", "url", "headers", "tools"}
132-
}
125+
// JSON format - include tools field for MCP gateway tool filtering (all engines)
126+
// For HTTP MCP with secrets in headers, env passthrough is needed
127+
if len(headerSecrets) > 0 {
128+
propertyOrder = []string{"type", "url", "headers", "tools", "env"}
133129
} else {
134-
propertyOrder = []string{"type", "url", "headers"}
130+
propertyOrder = []string{"type", "url", "headers", "tools"}
135131
}
136132
}
137133
default:
@@ -147,8 +143,11 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma
147143
// Include type field only for engines that require copilot fields
148144
existingProperties = append(existingProperties, prop)
149145
case "tools":
150-
// Include tools field only for engines that require copilot fields
151-
if renderer.RequiresCopilotFields {
146+
// Include tools field for JSON format when:
147+
// - RequiresCopilotFields (Copilot always renders it; when Allowed is empty, the
148+
// rendering code below defaults to the "*" wildcard)
149+
// - OR allowed tools are explicitly specified (pass the filter to the MCP gateway)
150+
if renderer.RequiresCopilotFields || len(mcpConfig.Allowed) > 0 {
152151
existingProperties = append(existingProperties, prop)
153152
}
154153
case "container":

0 commit comments

Comments
 (0)