Skip to content

Commit c861b1b

Browse files
Support --allowed-rules filtering with MCP server (#358)
* Initial plan * Implement --allowed-rules filtering support for MCP server - Apply global allowedRules setting to MCP server default config - Add allowed_rules parameter to all MCP tool definitions - Update all handler functions to parse and apply allowed_rules parameter - Ensure consistent behavior between CLI and MCP server for rule filtering Co-authored-by: fproulx-boostsecurity <[email protected]> * Add comprehensive tests for --allowed-rules filtering in MCP server - Add test cases for analyze_manifest with allowed_rules parameter - Verify filtering works correctly with single rule, multiple rules, and non-existent rules - Confirm both global flag and per-request parameter functionality - All tests pass demonstrating successful implementation Co-authored-by: fproulx-boostsecurity <[email protected]> * Fix linting issues: gofmt formatting and testifylint violations - Remove trailing whitespace in cmd/handle_analyze_manifest_test.go and cmd/mcp_server.go - Replace assert.Len(t, ..., 0, ...) with assert.Empty(t, ...) for testifylint compliance - All tests still pass and functionality remains intact Co-authored-by: fproulx-boostsecurity <[email protected]> * Fix global --allowed-rules flag not being honored by MCP server handlers - Modify startMCPServer to create mcpDefaultConfig with global allowedRules applied - Update all handler functions to accept and use the mcpDefaultConfig instead of global config - Add comprehensive test to verify global allowed rules are properly inherited - Handlers now properly inherit global --allowed-rules setting when no per-request rules specified - CLI and MCP server behavior now consistent for global rule filtering Co-authored-by: fproulx-boostsecurity <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: fproulx-boostsecurity <[email protected]>
1 parent e604ca1 commit c861b1b

File tree

2 files changed

+284
-17
lines changed

2 files changed

+284
-17
lines changed

cmd/handle_analyze_manifest_test.go

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,194 @@ jobs:
294294

295295
t.Logf("Successfully analyzed manifest with %d findings", len(insights.Findings))
296296
}
297+
298+
func TestHandleAnalyzeManifestWithAllowedRules(t *testing.T) {
299+
ctx := context.Background()
300+
301+
analyzer, err := createTestAnalyzer(ctx)
302+
require.NoError(t, err)
303+
304+
vulnerableManifest := `name: Test Workflow
305+
on:
306+
pull_request_target:
307+
branches: [main]
308+
309+
jobs:
310+
test:
311+
runs-on: ubuntu-latest
312+
steps:
313+
- name: Checkout
314+
uses: actions/checkout@v4
315+
with:
316+
ref: ${{ github.event.pull_request.head.sha }}
317+
- name: Run test
318+
run: echo "Testing ${{ github.event.pull_request.head.ref }}"`
319+
320+
t.Run("without allowed_rules filter", func(t *testing.T) {
321+
request := NewCallToolRequest("analyze_manifest", map[string]interface{}{
322+
"content": vulnerableManifest,
323+
"manifest_type": "github-actions",
324+
})
325+
326+
result, err := handleAnalyzeManifest(ctx, request, analyzer)
327+
require.NoError(t, err)
328+
require.NotNil(t, result)
329+
assert.False(t, result.IsError)
330+
331+
contentText := extractTextFromContent(t, result.Content[0])
332+
require.NotEmpty(t, contentText)
333+
334+
var insights struct {
335+
Findings []results.Finding `json:"findings"`
336+
Rules map[string]results.Rule `json:"rules"`
337+
}
338+
err = json.Unmarshal([]byte(contentText), &insights)
339+
require.NoError(t, err)
340+
341+
// Should have multiple findings including injection
342+
assert.Greater(t, len(insights.Findings), 1, "Should have multiple findings without filter")
343+
344+
// Verify injection rule is present
345+
hasInjection := false
346+
for _, finding := range insights.Findings {
347+
if finding.RuleId == "injection" {
348+
hasInjection = true
349+
break
350+
}
351+
}
352+
assert.True(t, hasInjection, "Should have injection finding")
353+
354+
t.Logf("Found %d findings without filter", len(insights.Findings))
355+
})
356+
357+
t.Run("with allowed_rules filter for injection only", func(t *testing.T) {
358+
request := NewCallToolRequest("analyze_manifest", map[string]interface{}{
359+
"content": vulnerableManifest,
360+
"manifest_type": "github-actions",
361+
"allowed_rules": []string{"injection"},
362+
})
363+
364+
result, err := handleAnalyzeManifest(ctx, request, analyzer)
365+
require.NoError(t, err)
366+
require.NotNil(t, result)
367+
assert.False(t, result.IsError)
368+
369+
contentText := extractTextFromContent(t, result.Content[0])
370+
require.NotEmpty(t, contentText)
371+
372+
var insights struct {
373+
Findings []results.Finding `json:"findings"`
374+
Rules map[string]results.Rule `json:"rules"`
375+
}
376+
err = json.Unmarshal([]byte(contentText), &insights)
377+
require.NoError(t, err)
378+
379+
// Should have only injection finding
380+
assert.Len(t, insights.Findings, 1, "Should have only one finding with filter")
381+
assert.Equal(t, "injection", insights.Findings[0].RuleId, "Should only have injection finding")
382+
383+
// Should have only injection rule
384+
assert.Len(t, insights.Rules, 1, "Should have only one rule with filter")
385+
_, hasInjectionRule := insights.Rules["injection"]
386+
assert.True(t, hasInjectionRule, "Should have injection rule in rules map")
387+
388+
t.Logf("Found %d findings with allowed_rules filter", len(insights.Findings))
389+
})
390+
391+
t.Run("with allowed_rules filter for non-existent rule", func(t *testing.T) {
392+
request := NewCallToolRequest("analyze_manifest", map[string]interface{}{
393+
"content": vulnerableManifest,
394+
"manifest_type": "github-actions",
395+
"allowed_rules": []string{"non_existent_rule"},
396+
})
397+
398+
result, err := handleAnalyzeManifest(ctx, request, analyzer)
399+
require.NoError(t, err)
400+
require.NotNil(t, result)
401+
assert.False(t, result.IsError)
402+
403+
contentText := extractTextFromContent(t, result.Content[0])
404+
require.NotEmpty(t, contentText)
405+
406+
var insights struct {
407+
Findings []results.Finding `json:"findings"`
408+
Rules map[string]results.Rule `json:"rules"`
409+
}
410+
err = json.Unmarshal([]byte(contentText), &insights)
411+
require.NoError(t, err)
412+
413+
// Should have no findings
414+
assert.Empty(t, insights.Findings, "Should have no findings with non-existent rule filter")
415+
assert.Empty(t, insights.Rules, "Should have no rules with non-existent rule filter")
416+
417+
t.Logf("Found %d findings with non-existent rule filter", len(insights.Findings))
418+
})
419+
}
420+
421+
func TestMCPServerGlobalAllowedRules(t *testing.T) {
422+
ctx := context.Background()
423+
424+
// Save original state
425+
originalAllowedRules := allowedRules
426+
defer func() {
427+
allowedRules = originalAllowedRules
428+
}()
429+
430+
// Simulate global --allowed-rules injection flag
431+
allowedRules = []string{"injection"}
432+
433+
// Create analyzer with global allowed rules (simulating startMCPServer behavior)
434+
testConfig := *config
435+
if len(allowedRules) > 0 {
436+
testConfig.AllowedRules = allowedRules
437+
}
438+
439+
vulnerableManifest := `name: Test Workflow
440+
on:
441+
pull_request_target:
442+
branches: [main]
443+
444+
jobs:
445+
test:
446+
runs-on: ubuntu-latest
447+
steps:
448+
- name: Checkout
449+
uses: actions/checkout@v4
450+
with:
451+
ref: ${{ github.event.pull_request.head.sha }}
452+
- name: Run test
453+
run: echo "Testing ${{ github.event.pull_request.head.ref }}"`
454+
455+
t.Run("handleAnalyzeManifest respects global allowed rules", func(t *testing.T) {
456+
opaClient, err := newOpaWithConfig(ctx, &testConfig)
457+
require.NoError(t, err)
458+
manifestAnalyzer := analyze.NewAnalyzer(nil, nil, &noop.Format{}, &testConfig, opaClient)
459+
460+
// Call without allowed_rules parameter (should inherit global)
461+
request := NewCallToolRequest("analyze_manifest", map[string]interface{}{
462+
"content": vulnerableManifest,
463+
"manifest_type": "github-actions",
464+
})
465+
466+
result, err := handleAnalyzeManifest(ctx, request, manifestAnalyzer)
467+
require.NoError(t, err)
468+
require.NotNil(t, result)
469+
assert.False(t, result.IsError)
470+
471+
contentText := extractTextFromContent(t, result.Content[0])
472+
require.NotEmpty(t, contentText)
473+
474+
var insights struct {
475+
Findings []results.Finding `json:"findings"`
476+
Rules map[string]results.Rule `json:"rules"`
477+
}
478+
err = json.Unmarshal([]byte(contentText), &insights)
479+
require.NoError(t, err)
480+
481+
// Should have only injection finding due to global allowed rules
482+
assert.Len(t, insights.Findings, 1, "Should have only injection finding with global allowed rules")
483+
assert.Equal(t, "injection", insights.Findings[0].RuleId, "Should only have injection finding")
484+
485+
t.Logf("Global allowed rules test: Found %d findings", len(insights.Findings))
486+
})
487+
}

0 commit comments

Comments
 (0)