-
Notifications
You must be signed in to change notification settings - Fork 13
Description
Part of duplicate code analysis: #1388
Summary
The session establishment callback inside sdk.NewStreamableHTTPHandler is duplicated nearly verbatim between unified mode (internal/server/transport.go) and routed mode (internal/server/routed.go). Both callbacks perform the same sequence of operations with only minor variations (the routed version also handles backendID). A setupSessionCallback helper previously existed to unify this logic but was removed during the recent refactor.
Duplication Details
Pattern: StreamableHTTP session setup callback
- Severity: Medium
- Occurrences: 2 instances
- Locations:
internal/server/transport.go(lines 65–87)internal/server/routed.go(lines 110–129)
Unified mode (transport.go lines 65–87):
sessionID := extractAndValidateSession(r)
if sessionID == "" {
return nil
}
logger.LogInfo("client", "MCP connection established, remote=%s, method=%s, path=%s, session=%s", r.RemoteAddr, r.Method, r.URL.Path, sessionID)
log.Printf("=== NEW STREAMABLE HTTP CONNECTION ===")
log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path)
log.Printf("Authorization (Session ID): %s", sanitize.TruncateSecret(sessionID))
log.Printf("DEBUG: About to check request body, Method=%s, Body!=nil: %v", r.Method, r.Body != nil)
logHTTPRequestBody(r, sessionID, "")
*r = *injectSessionContext(r, sessionID, "")
log.Printf("✓ Injected session ID into context")
log.Printf("==========================\n")Routed mode (routed.go lines 110–129):
sessionID := extractAndValidateSession(r)
if sessionID == "" {
return nil
}
logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s",
r.RemoteAddr, r.Method, r.URL.Path, backendID, sessionID)
log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===")
log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path)
log.Printf("Backend: %s", backendID)
log.Printf("Authorization (Session ID): %s", sessionID)
logHTTPRequestBody(r, sessionID, backendID)
*r = *injectSessionContext(r, sessionID, backendID)
log.Printf("✓ Injected session ID and backend ID into context")
log.Printf("===================================\n")The only structural differences are: the log message includes backendID in the routed version, and a log.Printf("Backend: ...") line is added.
Impact Analysis
- Maintainability: Any change to session setup logic (e.g., adding a new field to context, changing the log format, adding tracing) must be applied in both places. History shows a
setupSessionCallbackhelper previously unified this logic before being removed in the recent refactor. - Bug Risk: The
transport.goversion usessanitize.TruncateSecret(sessionID)for the Authorization log, butrouted.gologssessionIDdirectly without truncation — a security-relevant inconsistency introduced by the duplication. - Code Bloat: ~20 lines of near-duplicate callback code in two files.
Refactoring Recommendations
- Re-introduce a
setupSessionCallbackhelper ininternal/server/http_helpers.go:Estimated effort: ~1 hour. Benefits: single place to update session setup logic, eliminates the security inconsistency in Authorization logging.// setupSessionCallback performs common session establishment for StreamableHTTP handlers. // Returns the sessionID and false if the request should be rejected (empty session). func setupSessionCallback(r *http.Request, backendID string) (string, bool) { sessionID := extractAndValidateSession(r) if sessionID == "" { return "", false } if backendID != "" { logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s", r.RemoteAddr, r.Method, r.URL.Path, backendID, sessionID) log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===") } else { logger.LogInfo("client", "MCP connection established, remote=%s, method=%s, path=%s, session=%s", r.RemoteAddr, r.Method, r.URL.Path, sessionID) log.Printf("=== NEW STREAMABLE HTTP CONNECTION ===") } log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path) if backendID != "" { log.Printf("Backend: %s", backendID) } log.Printf("Authorization (Session ID): %s", sanitize.TruncateSecret(sessionID)) logHTTPRequestBody(r, sessionID, backendID) *r = *injectSessionContext(r, sessionID, backendID) log.Printf("✓ Injected session ID into context") log.Printf("===================================\n") return sessionID, true }
Implementation Checklist
- Re-introduce
setupSessionCallbackhelper ininternal/server/http_helpers.go - Update
transport.gocallback to use the helper - Update
routed.gocallback to use the helper - Fix the
sanitize.TruncateSecretinconsistency (unified vs routed) - Verify no functionality broken with existing tests
- Run
make agent-finished
Parent Issue
See parent analysis report: #1388
Related to #1388
Generated by Duplicate Code Detector
- expires on Mar 4, 2026, 3:01 AM UTC