Skip to content

Conversation

@amirejaz
Copy link
Contributor

@amirejaz amirejaz commented Sep 19, 2025

Summary

This PR fixes the audit middleware's transport detection mechanism to use the actual transport type passed from the command line instead of relying on hardcoded path patterns like /sse.

Problem

The audit middleware was using hardcoded path checking (strings.Contains(path, "/sse")) to determine if a request was using SSE transport. This approach was problematic because:

  1. Inflexible: Different MCP servers use different SSE endpoint paths or may not use any path (e.g., /v1/sse, /sse, /api/sse)
  2. Unreliable: The middleware couldn't correctly identify transport type for varying endpoint structures
  3. Hardcoded: Required manual updates for each new endpoint pattern

Solution

Refactored the audit middleware to:

  1. Accept transport type directly: The middleware now receives the actual transport type (sse, streamable-http) from the run configuration
  2. Remove hardcoded path checks: Eliminated all strings.Contains(path, "/sse") logic
  3. Detect sticky MCP streams on the request: classify as sticky only for GET with Accept containing text/event-stream (covers SSE + Streamable HTTP stream).
  4. Update all constructors: Modified NewAuditorWithTransport to require transport type parameter

Changes

Core Changes

  • pkg/audit/auditor.go:

    • Added transportType field to Auditor struct
    • Updated NewAuditorWithTransport to require transport type
    • Simplified isSSETransport() to use direct transport type comparison
    • Removed all hardcoded path checking in determineEventType and addMetadata
  • pkg/audit/config.go:

    • Updated CreateMiddlewareWithTransport to require transport type
    • Modified GetMiddlewareFromFile to accept transport type parameter
  • pkg/audit/middleware.go:

    • Updated MiddlewareParams to include TransportType field
    • Modified CreateMiddleware to always use transport-aware constructor
    • Add isMCPStreamOpenRequest (GET + Accept: text/event-stream) and log connection-open immediately for those; remove path checks and isMCPMessagePost; no ResponseWriter changes.
  • pkg/runner/config_builder.go & pkg/runner/middleware.go:

    • Updated middleware configuration to pass actual transport type

Test Updates

  • pkg/audit/auditor_test.go & pkg/audit/config_test.go:
    • Updated all test calls to use NewAuditorWithTransport with appropriate transport types
    • Fixed TestDetermineEventType to use correct transport types for each test case

Benefits

  1. Accurate transport detection: Works with any SSE endpoint path structure
  2. Maintainable: No more hardcoded path patterns to maintain
  3. Flexible: Supports any transport type passed from command line
  4. Reliable: Uses the actual transport configuration instead of guessing

Testing

  • All existing tests updated and passing
  • Transport detection now works correctly for different endpoint patterns
  • No breaking changes to existing functionality

Original identified issue: #1957 (comment)

@amirejaz amirejaz requested review from JAORMX and dmjb September 19, 2025 10:43
@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 67.56757% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.19%. Comparing base (6929a52) to head (168b930).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/runner/config_builder.go 0.00% 4 Missing ⚠️
pkg/runner/middleware.go 0.00% 4 Missing ⚠️
pkg/audit/auditor.go 87.50% 2 Missing and 1 partial ⚠️
pkg/audit/config.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1974      +/-   ##
==========================================
+ Coverage   48.13%   48.19%   +0.06%     
==========================================
  Files         233      233              
  Lines       29229    29240      +11     
==========================================
+ Hits        14069    14093      +24     
+ Misses      14129    14111      -18     
- Partials     1031     1036       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amirejaz amirejaz requested a review from blkt September 19, 2025 11:20
dmjb
dmjb previously approved these changes Sep 19, 2025
@amirejaz amirejaz requested a review from dmjb September 30, 2025 17:13
@amirejaz amirejaz merged commit be1e1a5 into main Oct 1, 2025
25 checks passed
@amirejaz amirejaz deleted the fix-auditor-middleware-transport branch October 1, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants