fix: prevent tools invocation without valid session initialization #607
Conversation
- Modified InsecureStatefulSessionIdManager to track active sessions using sync.Map - Added session existence validation in addition to format validation - Implemented proper session termination tracking - Added comprehensive regression tests for session validation scenarios - Updated sampling tests to use stateless mode for compatibility This fixes a security issue where tools could be invoked with any well-formatted session ID without going through proper initialization, allowing unauthorized access.
WalkthroughAdds stateful session tracking to the streamable HTTP server by giving InsecureStatefulSessionIdManager internal maps for active and terminated sessions, updates Generate/Validate/Terminate behavior, adds a WithStateLess option to NewStreamableHTTPServer, and expands tests to exercise session lifecycle and stateless construction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/streamable_http.go(1 hunks)server/streamable_http_sampling_test.go(3 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/streamable_http_test.go (4)
mcp/tools.go (3)
NewTool(679-701)CallToolRequest(54-58)CallToolResult(40-51)mcp/utils.go (1)
NewToolResultText(271-280)server/streamable_http.go (2)
NewTestStreamableHTTPServer(1060-1064)InsecureStatefulSessionIdManager(1021-1024)server/constants.go (1)
HeaderKeySessionID(5-5)
server/streamable_http_sampling_test.go (1)
server/streamable_http.go (2)
NewStreamableHTTPServer(152-167)WithStateLess(45-51)
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/streamable_http.go (1)
1050-1060: Termination is now idempotent—addresses past review.The implementation correctly makes termination idempotent by:
- Returning success (false, nil) if the session is already terminated
- Returning success (false, nil) if the session doesn't exist
This ensures that retry requests won't fail with 500 errors.
Consider adding terminated session cleanup.
The
terminatedmap grows indefinitely as sessions are terminated, which could cause memory issues in long-running servers. Consider implementing periodic cleanup or a TTL-based eviction strategy.For example, you could add a background cleanup goroutine:
// In NewStreamableHTTPServer or a dedicated initialization function go func() { ticker := time.NewTicker(1 * time.Hour) defer ticker.Stop() for range ticker.C { // Clean up terminated sessions older than 24 hours // Would require storing timestamps with terminated entries } }()Alternatively, consider using a TTL cache library or limiting the terminated map size with an LRU eviction policy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(1 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/streamable_http_test.go (5)
server/server.go (1)
NewMCPServer(335-363)mcp/tools.go (4)
NewTool(679-701)WithDescription(722-726)CallToolRequest(54-58)CallToolResult(40-51)server/streamable_http.go (2)
NewTestStreamableHTTPServer(1063-1067)InsecureStatefulSessionIdManager(1021-1024)client/transport/constants.go (1)
HeaderKeySessionID(5-5)server/constants.go (1)
HeaderKeySessionID(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
server/streamable_http.go (3)
1018-1024: LGTM! Session tracking structure is well-designed.The use of
sync.Mapfor bothsessionsandterminatedappropriately handles concurrent access patterns. The updated comment accurately describes the manager's behavior.
1028-1032: LGTM! Generation and registration logic is correct.The method correctly generates a session ID with the expected format and registers it in the active sessions map.
1034-1048: LGTM! Validation logic correctly enforces both format and existence.The method properly validates session IDs through multiple checks:
- Format validation (prefix and UUID)
- Termination status (returns
isTerminated=true)- Existence in active sessions (returns error if not found)
This addresses the security vulnerability by preventing the use of well-formatted but non-existent session IDs.
server/streamable_http_test.go (2)
1019-1187: LGTM! Comprehensive session validation testing.The test suite thoroughly validates the security fix by covering all critical scenarios:
- Rejection of fake but well-formatted session IDs (400)
- Rejection of malformed session IDs (400)
- Acceptance of valid session IDs from initialization (200)
- Rejection of terminated session IDs (404)
Each subtest properly verifies both status codes and error messages, ensuring the fix works as intended.
1189-1316: LGTM! Excellent unit test coverage for the session manager.The test suite provides comprehensive coverage of the
InsecureStatefulSessionIdManager:
- Generation and validation of session IDs
- Rejection of malformed and non-existent IDs
- Termination marking and validation
- Idempotent termination for both non-existent and already-terminated sessions
- Thread-safety with concurrent generation and validation (100 goroutines)
The idempotency tests (lines 1257-1289) are particularly valuable as they verify the fix for the past review comment.
Description
This PR fixes a security vulnerability where tools could be invoked without proper session initialization. The
InsecureStatefulSessionIdManagerwas only validating session ID format but not checking if the session actually existed, allowing any well-formatted fake session ID to bypass authentication.Fixes #579
Type of Change
Changes Made
Core Implementation
InsecureStatefulSessionIdManagerto track active sessions usingsync.Mapsessionsmap to store generated session IDsterminatedmap to track terminated sessionsGenerate()to store session IDs when createdValidate()to check session existence in addition to format validationTerminate()to properly mark sessions as terminated and remove from active sessionsTesting
TestStreamableHTTP_SessionValidationwith comprehensive test cases:TestInsecureStatefulSessionIdManagerfor unit testing the session manager:TestStreamableHTTPServer_SamplingErrorHandlingto use stateless mode for compatibilityChecklist
Additional Information
Security Impact
Before this fix, an attacker could invoke tools using any well-formatted session ID like
mcp-session-ffffffff-ffff-ffff-ffff-ffffffffffffwithout going through the initialization flow. This fix ensures that only session IDs that were actually generated by the server are accepted.Verification
The original exploit from issue #579 now properly returns a 400 Bad Request with "Invalid session ID" error message instead of executing the tool.
All existing tests pass, demonstrating backward compatibility with legitimate use cases.
Summary by CodeRabbit
New Features
Behavior Changes
Tests