Mitigate Server-Side Request Forgery across URL-consuming components

[luis5tb]

Add startup-time URL validation to block dangerous schemes (file://, ftp://, gopher://) and cloud metadata IPs on MCP_SERVER_URL, RED_HAT_SSO_ISSUER, GMA_API_BASE_URL, OTEL exporter endpoints, and RATE_LIMIT_REDIS_URL. Replace the weak startswith-based redirect URI check in GMAClient with proper urlparse validation to prevent bypass via localhost subdomains (e.g. http://localhost.attacker.com). Includes 30 new tests covering all validators.

[IlonaShishov]

please also checkout Claude code review

[IlonaShishov]

why not use here if parsed.scheme not in ("redis", "rediss"): as well?

[IlonaShishov]

Code Review: SSRF Mitigations

Overview

Comprehensive SSRF (CWE-918) protections across all URL-consuming components. Key achievement: fixes critical redirect URI validator vulnerability where http://localhost.attacker.com could bypass the old startswith check. Adds 30 new tests with excellent coverage.

Changes:

• 4 new Pydantic validators in Settings (MCP, SSO, GMA, OTEL URLs)
• Fixed redirect URI validation in GMAClient (proper URL parsing)
• Added Redis URL scheme validation
• Blocks dangerous schemes (file://, ftp://, gopher://) and cloud metadata IP

---

Strengths

✅ Proper URL parsing - Uses urlparse() instead of string manipulation
✅ Critical vulnerability fixed - http://localhost.attacker.com bypass now blocked
✅ Comprehensive test coverage - Tests specific attack vectors and edge cases
✅ Dev-friendly - Respects dev mode flags (skip_jwt_validation, dcr_enabled)
✅ Scheme restrictions - Blocks file://, ftp://, gopher:// across components
✅ Cloud metadata protection - Explicitly blocks 169.254.169.254

---

Security Gaps

🔴 Critical: IPv6 Metadata Endpoint Not Blocked

Code only checks IPv4 169.254.169.254, but cloud metadata is also accessible via IPv6:

• AWS: fd00:ec2::254
• Azure: fe80:: link-local addresses
• GCP: May add IPv6 support

Recommendation:

BLOCKED_METADATA_IPS = {
    ipaddress.ip_address("169.254.169.254"),  # IPv4
    ipaddress.ip_address("fd00:ec2::254"),     # AWS IPv6
}

# In validator:
if ip in BLOCKED_METADATA_IPS or ip.is_link_local:
    raise ValueError("Cannot target cloud metadata endpoints")

🟡 Medium: Private IP Ranges Not Blocked

Validators allow connections to private networks (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), enabling internal network scanning.

Recommendation:

if ip.is_private and not ip.is_loopback:
    raise ValueError(f"Cannot target private IP ranges: {ip}")

Consider: if internal services (mcp-sidecar:8080) need access, allow hostname-based but block direct IP references.

🟡 Medium: DNS Rebinding Window

Validation only at startup. Malicious DNS could return safe IPs during validation, then unsafe IPs when URLs are used.

Mitigation:

• Document this limitation
• Consider request-time validation for high-risk URLs (GMA, SSO)
• Use IP literals for trusted internal services

---

Code Quality Issues

1. Inconsistent startswith Usage in Redis Validator

src/lightspeed_agent/ratelimit/middleware.py:100, 108

# Current code - inconsistent with PR intent
if not settings.rate_limit_redis_url.startswith(("redis://", "rediss://")):
    raise ValueError(...)
uses_tls = settings.rate_limit_redis_url.startswith("rediss://")

This PR's purpose is replacing startswith with proper URL parsing. Should be:

from urllib.parse import urlparse

parsed = urlparse(settings.rate_limit_redis_url)
if parsed.scheme not in ("redis", "rediss"):
    raise ValueError(
        "RATE_LIMIT_REDIS_URL must use redis:// or rediss:// scheme. "
        f"Got: {parsed.scheme}://"
    )

uses_tls = parsed.scheme == "rediss"

Benefits: Consistent with PR approach, clearer intent, more maintainable, single parse reused.

2. Convoluted Exception Handling

src/lightspeed_agent/config/settings.py:419-427

# Current - fragile string matching
try:
    ip = ipaddress.ip_address(parsed.hostname)
    if ip == ipaddress.ip_address("169.254.169.254"):
        raise ValueError("...")
except ValueError as e:
    if "metadata" in str(e):
        raise
    # Not an IP address — hostname is fine

Recommendation:

if parsed.hostname:
    try:
        ip = ipaddress.ip_address(parsed.hostname)
    except ValueError:
        # Not an IP address, hostname is fine
        pass
    else:
        # It's an IP, validate it
        if ip == ipaddress.ip_address("169.254.169.254"):
            raise ValueError("Cannot target metadata endpoint")

3. Code Duplication

URL scheme validation repeated across 4 validators. Consider extracting:

def _validate_url_scheme(url: str, var_name: str, allowed_schemes: set[str]) -> None:
    parsed = urlparse(url)
    if parsed.scheme not in allowed_schemes:
        raise ValueError(
            f"{var_name} must use {'/'.join(allowed_schemes)} "
            f"(got '{parsed.scheme}://')"
        )

---

Testing Gaps

Strengths: 30 tests, excellent coverage, tests bypass attempts

Missing:

• IPv6 metadata endpoints
• Private IP ranges
• URL encoding tricks (hex: 0x7f.0.0.1, decimal: 2130706433)
• Integration test: does httpx actually block these at request time?

Recommendation: Add integration test

@pytest.mark.asyncio
async def test_httpx_blocks_metadata_endpoint():
    async with httpx.AsyncClient() as client:
        with pytest.raises(httpx.ConnectError):
            await client.get("http://169.254.169.254/")

---

Recommendations

Must Fix Before Merge

1. Add IPv6 metadata endpoint blocking (see Critical section)
2. Replace startswith in Redis validator with urlparse()
3. Fix exception handling in _validate_mcp_server_url

Should Fix

4. Block private IP ranges or document why internal access is needed
5. Extract common validation logic to reduce duplication

Nice to Have

6. Add URL normalization checks
7. Add httpx integration test
8. Document DNS rebinding limitation
9. Use pytest parametrize to reduce test duplication

---

Decision

⚠️ REQUEST CHANGES - Security gaps (#1, #2) and consistency issue (#2) should be addressed before merge. The core approach is sound and the redirect URI fix is excellent, but IPv6 metadata endpoint is a blind spot in cloud environments.

Once critical issues are fixed, this PR will significantly improve security posture. Great work on comprehensive test coverage and the urlparse fix!