-
Notifications
You must be signed in to change notification settings - Fork 15
Closed
Description
🔍 Duplicate Code Pattern: Authorization Header Parsing
Part of duplicate code analysis: #351
Summary
Authorization header parsing logic is duplicated across routed mode and unified mode HTTP handlers, with nearly identical 15-line code blocks in both internal/server/routed.go and internal/server/transport.go. This creates a security risk as changes to authorization logic must be synchronized across both files.
Duplication Details
Pattern: Authorization Header Extraction and Session ID Parsing
- Severity: High (Security-Critical)
- Occurrences: 2 instances
- Locations:
internal/server/routed.go(lines 55-68)internal/server/transport.go(lines 101-114)
Code Sample from routed.go:
// Extract session ID from Authorization header
// Per spec 7.1: When API key is configured, Authorization contains plain API key
// When API key is not configured, supports Bearer token for backward compatibility
authHeader := r.Header.Get("Authorization")
var sessionID string
if strings.HasPrefix(authHeader, "Bearer ") {
// Bearer token format (for backward compatibility when no API key)
sessionID = strings.TrimPrefix(authHeader, "Bearer ")
sessionID = strings.TrimSpace(sessionID)
} else if authHeader != "" {
// Plain format (per spec 7.1 - API key is session ID)
sessionID = authHeader
}Code Sample from transport.go:
// Extract session ID from Authorization header
// Per spec 7.1: When API key is configured, Authorization contains plain API key
// When API key is not configured, supports Bearer token for backward compatibility
authHeader := r.Header.Get("Authorization")
var sessionID string
if strings.HasPrefix(authHeader, "Bearer ") {
// Bearer token format (for backward compatibility when no API key)
sessionID = strings.TrimPrefix(authHeader, "Bearer ")
sessionID = strings.TrimSpace(sessionID)
} else if authHeader != "" {
// Plain format (per spec 7.1 - API key is session ID)
sessionID = authHeader
}These are nearly identical blocks - 15 lines duplicated verbatim.
Impact Analysis
- Maintainability: Any change to authorization parsing requires updates in 2 locations, increasing risk of inconsistency
- Bug Risk: High - if authorization logic is updated in one place but not the other, security vulnerabilities could be introduced
- Code Bloat: 15 lines × 2 = 30 lines of duplicated authorization code
- Security Risk: Authorization is security-critical - duplication increases the attack surface for inconsistencies
Refactoring Recommendations
1. Extract to internal/auth Package (Recommended)
- Create utility function:
auth.ExtractSessionID(r *http.Request) (string, error) - Location:
internal/auth/header.go(package already exists!) - Estimated effort: 1-2 hours
- Benefits:
- Single source of truth for authorization parsing
- Consistent error handling across routed and unified modes
- Easier to add tests for authorization logic
- Reduces security risk from inconsistent updates
Example Implementation:
// In internal/auth/header.go
func ExtractSessionID(r *http.Request) (string, error) {
authHeader := r.Header.Get("Authorization")
if authHeader == "" {
return "", ErrMissingAuthHeader
}
// Bearer token format (for backward compatibility)
if strings.HasPrefix(authHeader, "Bearer ") {
sessionID := strings.TrimPrefix(authHeader, "Bearer ")
return strings.TrimSpace(sessionID), nil
}
// Plain format (per spec 7.1 - API key is session ID)
return authHeader, nil
}Usage in both files:
sessionID, err := auth.ExtractSessionID(r)
if err != nil {
logger.LogError("client", "Rejected MCP client connection: %v, remote=%s, path=%s", err, r.RemoteAddr, r.URL.Path)
return nil
}2. Alternative: Use Existing auth.ParseAuthHeader
- The
internal/auth/header.goalready hasParseAuthHeaderfunction - Consider adapting it for this use case
- May need slight modification to handle Bearer tokens
Implementation Checklist
- Review
internal/auth/header.gopackage structure - Add
ExtractSessionIDfunction (or adapt existingParseAuthHeader) - Add unit tests for session ID extraction
- Refactor
internal/server/routed.goto use new function - Refactor
internal/server/transport.goto use new function - Run full test suite to verify no functionality broken
- Update documentation if needed
Parent Issue
See parent analysis report: #351
Related to #351
AI generated by Duplicate Code Detector
Reactions are currently unavailable
Metadata
Metadata
Assignees
Type
Fields
Give feedbackNo fields configured for issues without a type.