Security hardening: SSRF, headers, Redis TLS, container lockdown

[RafaelPo]

Summary

• Add transport-aware server instructions and SSRF protection (redirect validation, hostname re-check transport)
• Security headers middleware, rate limiter fail-open with in-memory fallback, body size limits
• Shell injection prevention via shlex.quote, Redis TLS support, container lockdown (non-root, read-only FS)
• User isolation: per-user Redis key namespacing, token encryption at rest
• Fix Docker build (--frozen removal), Redis healthcheck, and .dockerignore

Depends on PR #207 (unified input API).

Test plan

• All 283 unit tests pass (26 new tests for security features)
• SSRF protection blocks private IPs, metadata endpoints, unresolvable hosts
• Rate limiter falls back to in-memory when Redis unavailable
• Body size middleware rejects oversized uploads
• Upload handler returns generic error messages (no info leaks)
• Manual: Docker Compose build and healthcheck verification

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 2m 17s —— View job

---

Code Review: Security Hardening PR

• Read CLAUDE.md and repo guidelines
• Review SSRF protection (utils.py)
• Review security headers middleware (middleware.py)
• Review Redis TLS & key namespacing (redis_store.py)
• Review token encryption at rest (auth.py)
• Review upload handler changes (uploads.py)
• Review route changes (routes.py, app.py, http_config.py)
• Review tool/server changes (tools.py, tool_helpers.py, server.py)
• Review config changes (config.py, models.py, templates.py)
• Review Docker/compose changes
• Review test coverage
• Post final review summary

---

This is a well-structured security hardening PR. The defense-in-depth approach is solid, with multiple layers of protection across SSRF, authentication, encryption, and container security. I reviewed all 26 changed files; here is my analysis.

What looks good

SSRF Protection (utils.py) — The multi-layer approach is thorough: pre-flight DNS validation, redirect validation via httpx event hooks, and a custom _SSRFSafeTransport that re-validates hostnames at request time to narrow the TOCTOU window. IPv4-mapped IPv6 unwrapping (::ffff:127.0.0.1 → 127.0.0.1) is correctly handled. Streaming response size enforcement is a nice touch.

Rate Limiter (middleware.py) — Redis-primary with in-memory fallback is the right pattern. Thread-safe in-memory counters with periodic cleanup and a 50k hard cap prevent unbounded memory growth. The BodySizeLimitMiddleware is a clean pure-ASGI implementation that handles chunked transfers correctly.

Token Encryption (redis_store.py) — HKDF → Fernet for at-rest encryption of tokens in Redis is solid. The build_key sanitizer prevents Redis key injection. Sentinel support and TLS options are well-configured.

Auth (auth.py) — Atomic consume via GETDEL, client-ID mismatch re-storage, input length caps (256 chars), __Host- cookie prefix, rate limiting on auth endpoints, and JWKS lock for concurrent access are all well-implemented.

Upload System (uploads.py) — shlex.quote for shell injection prevention, hmac.compare_digest for timing-safe comparison, consume-on-use metadata, and generic error messages (no info leaks) are all correct.

Container Lockdown (Dockerfile/compose) — Non-root user, read-only filesystem, CAP_DROP ALL, no-new-privileges, resource limits, private Redis network, and 127.0.0.1 port binding are all best practices.

Test Coverage — 26 new tests covering SSRF blocking, rate limiter fallback, body size enforcement, upload validation, and generic error messages. Coverage is comprehensive.

---

Issues found

1. Incomplete cloud metadata hostname blocklist

utils.py:33-38 — _BLOCKED_HOSTNAMES only covers Google Cloud. While _BLOCKED_NETWORKS blocks 169.254.0.0/16 (covering metadata IPs for all providers), adding AWS and Azure metadata hostnames provides defense-in-depth against DNS edge cases:

_BLOCKED_HOSTNAMES = frozenset(
    {
        "metadata.google.internal",
        "metadata.google.internal.",
+       "metadata.aws.internal",
+       "metadata.aws.internal.",
    }
)

Severity: Low (IPs are already blocked by _BLOCKED_NETWORKS; this is defense-in-depth only).

Fix this →

2. _decode_trusted_server_jwt should restrict algorithms

auth.py:159-161 — This function disables signature verification (acceptable for trusted server-to-server tokens from Supabase) but hardcodes algorithms=["RS256"]. Since verification is off, PyJWT ignores the algorithms param anyway. Consider adding an "alg" claim check or a comment clarifying that algorithm confusion doesn't apply here because verification is disabled.

def _decode_trusted_server_jwt(token: str) -> dict[str, Any]:
    return pyjwt.decode(
        token, options={"verify_signature": False}, algorithms=["RS256"]
    )

Severity: Low (signature verification is intentionally off; algorithm confusion isn't a risk when you don't verify).

3. Misleading exception type in redirect validation

utils.py:157-160 — _check_redirect raises httpx.TooManyRedirects for SSRF blocks. While this works to abort the request, it could confuse debugging. A comment explaining the choice (or a dedicated exception) would improve clarity:

async def _check_redirect(response: httpx.Response) -> None:
    if response.is_redirect:
        location = response.headers.get("location", "")
        if location:
            try:
                _validate_url_target(location)
            except ValueError:
+               # Use TooManyRedirects to abort the redirect chain — httpx
+               # does not expose a more specific "redirect rejected" error.
                raise httpx.TooManyRedirects(
                    f"Redirect to blocked address: {location}",
                    request=response.request,
                )

Severity: Nit.

4. HKDF derivation uses salt=None

redis_store.py:61-66 — The HKDF key derivation uses salt=None. While the info parameter provides domain separation, HKDF's security properties are stronger with a salt. Consider using a static salt:

key = HKDF(
    algorithm=hashes.SHA256(),
    length=32,
-   salt=None,
+   salt=b"everyrow-mcp-fernet-v1",
    info=b"everyrow-mcp-fernet",
).derive(settings.upload_secret.encode())

Severity: Low (current approach is cryptographically acceptable for single-key derivation from a high-entropy secret, but a salt is recommended by RFC 5869).

5. Redis warning should fire for auth-mode HTTP only

http_config.py:78-82 — The Redis password warning fires for all HTTP mode (settings.is_http), but it's arguably only critical for authenticated deployments. In --no-auth dev mode, Redis on localhost without a password may be fine.

-   if settings.is_http and not settings.redis_password:
+   if settings.is_http and not no_auth and not settings.redis_password:

Severity: Nit (cosmetic; a warning in dev mode is noisy but harmless).

---

Summary

This is a strong security PR with no high-severity issues. The SSRF protection, middleware stack, encryption-at-rest, and container hardening are all well-implemented with proper test coverage. The issues noted above are low-severity improvements that can be addressed in follow-up if desired.
|