[Bug]: Multiple Fixes and improved security for HTTP Header Passthrough Feature

[crivetimihai]

Multiple Fixes and improved security for HTTP Header Passthrough Feature

Summary

The recently merged HTTP Header Passthrough feature (#208, commit b6072ad) contains several code quality issues that need immediate attention.

Critical Issues

1. 🔴 Environment Variable Parsing Bug

Location: mcpgateway/config.py:553-559, 565-571

Current Code:

default_passthrough_headers: Any = os.environ.get("DEFAULT_PASSTHROUGH_HEADERS", ["Authorization", "X-Tenant-Id", "X-Trace-Id"])
if not isinstance(default_passthrough_headers, list):
    try:
        default_passthrough_headers = list(default_passthrough_headers)  # BUG: Converts string to char array!
    except Exception as e:
        logger.warning(f"Invalid DEFAULT_PASSTHROUGH_HEADERS format...")

Problem:

• os.environ.get() returns a string, not a list
• list("ABC") returns ['A', 'B', 'C'] instead of parsing as JSON
• This creates single-character "headers" that could bypass security controls

Fix Required:

import json

default_value = os.environ.get("DEFAULT_PASSTHROUGH_HEADERS")
if default_value:
    try:
        # Try JSON parsing first
        default_passthrough_headers = json.loads(default_value)
    except json.JSONDecodeError:
        # Fallback to comma-separated parsing
        default_passthrough_headers = [h.strip() for h in default_value.split(',') if h.strip()]
else:
    default_passthrough_headers = ["X-Tenant-Id", "X-Trace-Id"]  # Safer defaults without Authorization

2. 🔴 Duplicate Code Block

Location: mcpgateway/config.py:553-571

Problem: Lines 553-559 and 565-571 are identical duplicates

Fix Required: Remove the second occurrence (lines 565-571)

3. 🔴 Missing Header Value Sanitization

Location: mcpgateway/utils/passthrough_headers.py:1414

Problem: Headers are passed through without sanitization, allowing:

• Header injection via newlines (\r\n)
• Excessively long header values
• Control characters

Fix Required:

def sanitize_header_value(value: str, max_length: int = 4096) -> str:
    """Sanitize header value for security."""
    # Remove newlines and carriage returns
    value = value.replace('\r', '').replace('\n', '')
    # Trim to max length
    value = value[:max_length]
    # Remove control characters except tab
    value = ''.join(c for c in value if ord(c) >= 32 or c == '\t')
    return value.strip()

4. 🟡 Client-Side Header Parsing Without Validation

Location: mcpgateway/static/admin.js:3987-4008

Problem: No validation for malicious header names/values

Fix Required:

const HEADER_NAME_REGEX = /^[A-Za-z0-9\-]+$/;
const MAX_HEADER_VALUE_LENGTH = 4096;

function validateHeader(name, value) {
    if (!HEADER_NAME_REGEX.test(name)) {
        throw new Error(`Invalid header name: ${name}`);
    }
    if (value.includes('\n') || value.includes('\r')) {
        throw new Error('Header values cannot contain newlines');
    }
    if (value.length > MAX_HEADER_VALUE_LENGTH) {
        throw new Error(`Header value too long (max ${MAX_HEADER_VALUE_LENGTH} chars)`);
    }
    return true;
}

5. 🟡 Authorization Header in Default Passthrough

Location: .env.example:125, mcpgateway/config.py:553,565

Problem: Including Authorization in defaults could lead to token leakage

Fix Required: Remove Authorization from default headers

6. 🟡 Case Sensitivity Issues

Location: mcpgateway/utils/passthrough_headers.py:1396

Problem: Inconsistent case handling could cause headers to be missed

Fix Required: Normalize to lowercase for matching while preserving original case

7. 🔴 Missing Feature Flag (Security by Default)

Location: Feature flag not implemented

Problem: Header passthrough is enabled by default, violating security-by-default principles

Fix Required: Add ENABLE_HEADER_PASSTHROUGH environment variable (default: false)

# In config.py
enable_header_passthrough: bool = Field(
    default=False,
    description="Enable HTTP header passthrough feature (default: disabled for security)"
)

# In utils/passthrough_headers.py
def get_passthrough_headers(...):
    """Get headers that should be passed through to the target gateway."""
    # Early return if feature is disabled
    if not settings.enable_header_passthrough:
        return base_headers.copy()
    
    # Rest of existing logic...

Environment Variable:

# .env.example
# WARNING: Only enable if you understand the security implications
# ENABLE_HEADER_PASSTHROUGH=false  # Default: false for security

---

Implementation Checklist

Phase 1: Critical Fixes (Immediate)

• Add Feature Flag (mcpgateway/config.py)

  • Add enable_header_passthrough: bool = False setting
  • Check flag in get_passthrough_headers() - return early if disabled
  • Update .env.example with ENABLE_HEADER_PASSTHROUGH=false
  • Add warning comment about security implications
  • Update documentation to explain feature flag

• Fix Environment Variable Parsing (mcpgateway/config.py)

  • Import json module (already imported)
  • Replace list() call with proper JSON parsing
  • Add fallback to comma-separated parsing
  • Add unit tests for various input formats

• Remove Duplicate Code Block (mcpgateway/config.py)

  • Delete lines 565-571
  • Verify no other duplicates exist

• Add Header Sanitization (mcpgateway/utils/passthrough_headers.py)

  • Create sanitize_header_value() function
  • Apply sanitization in get_passthrough_headers()
  • Add regex validation for header names: ^[A-Za-z0-9\-]+$
  • Enforce max header value length (4KB)
  • Strip newlines and control characters

Phase 2: Security Hardening (High Priority)

• Update Default Headers

  • Remove Authorization from defaults in config.py
  • Update .env.example to exclude Authorization
  • Add documentation warning about Authorization header risks

• Fix Case Sensitivity (mcpgateway/utils/passthrough_headers.py)

  • Create case-insensitive lookup dict for request headers
  • Preserve original case when forwarding
  • Test with mixed-case headers

• Add Client-Side Validation (mcpgateway/static/admin.js)

  • Add header name regex validation
  • Add header value sanitization
  • Display validation errors in UI
  • Limit header value length client-side

Phase 3: Additional Security (Medium Priority)

• Add Rate Limiting

  • Implement rate limiting for /admin/config/passthrough-headers endpoints
  • Use existing validation_max_requests_per_minute setting
  • Add tests for rate limit behavior

• Improve Logging

  • Add audit logging for config changes
  • Log when headers are blocked due to conflicts
  • Add metrics for passthrough header usage
  • Log when feature is disabled via flag

Phase 4: Testing & Documentation

• Unit Tests

  • Test feature flag (disabled by default)
  • Test that headers are NOT passed when flag is false
  • Test environment variable parsing (JSON, CSV, invalid)
  • Test header sanitization with malicious input
  • Test case-insensitive matching
  • Test authorization conflict detection

• Integration Tests

  • Test end-to-end header passthrough
  • Test with multiple gateways
  • Test config API endpoints
  • Test admin UI header configuration

• Documentation Updates

  • Update security best practices
  • Add examples of safe configurations
  • Document header validation rules
  • Add troubleshooting for common issues

---

Testing Scenarios

Security Test Cases

1. Feature Flag Test

   # Test with feature disabled (default)
   unset ENABLE_HEADER_PASSTHROUGH
   # Headers should NOT be passed through
   
   # Test with feature explicitly disabled
   export ENABLE_HEADER_PASSTHROUGH=false
   # Headers should NOT be passed through
   
   # Test with feature enabled
   export ENABLE_HEADER_PASSTHROUGH=true
   # Headers should be passed through

2. Header Injection Test

   # Try to inject headers with newlines
   curl -H "X-Test: value\r\nX-Injected: malicious" ...

3. Environment Variable Parsing

   # Test various formats
   export DEFAULT_PASSTHROUGH_HEADERS='["X-Tenant-Id", "X-Trace-Id"]'
   export DEFAULT_PASSTHROUGH_HEADERS='X-Tenant-Id,X-Trace-Id'
   export DEFAULT_PASSTHROUGH_HEADERS='invalid json ['

4. Case Sensitivity

   # Headers with different cases
   curl -H "x-tenant-id: lower" -H "X-TENANT-ID: upper" ...

5. Long Header Values

   # Test with 10KB header value
   curl -H "X-Test: $(python -c 'print("A"*10000)')" ...

---

Rollback Plan

If issues are discovered after deployment:

1. Immediate Disable: Set ENABLE_HEADER_PASSTHROUGH=false in environment (this is the default)
2. Alternative Disable: Set DEFAULT_PASSTHROUGH_HEADERS=[] in environment
3. Database Cleanup: Clear passthrough_headers from all gateways
4. Verify: Check logs to confirm no headers are being passed through
5. Feature Flag: Add ENABLE_HEADER_PASSTHROUGH=false option

---

Priority Matrix

Issue	Severity	Effort	Priority
Feature Flag (Disabled by Default)	Critical	Low	P0 - Immediate
Environment Variable Parsing	Critical	Low	P0 - Immediate
Duplicate Code Block	High	Low	P0 - Immediate
Header Sanitization	Critical	Medium	P0 - Immediate
Remove Authorization Default	High	Low	P1 - High
Case Sensitivity	Medium	Low	P1 - High
Client Validation	Medium	Medium	P2 - Medium
Rate Limiting	Low	Medium	P3 - Low

Review Checklist

Before merging fixes:

• Feature flag defaults to disabled (false)
• All unit tests passing
• Security review completed
• No duplicate code blocks
• Environment variable parsing tested
• Header sanitization verified
• Documentation updated
• Admin UI tested
• Rate limiting functional
• Deployment guide updated
• Feature can be completely disabled via flag

[crivetimihai]

Closed by #687