Harden MCP server: auth, CORS, Redis, widgets, and review fixes

[RafaelPo]

Summary

Addresses all 10 findings from the Claude code review on PR #195, plus additional hardening.

Security & auth:

• Auth code expiration defense-in-depth check after deserialization
• Atomic auth code consumption via GETDEL (re-store on client mismatch)
• Revocation TTL derived from token remaining lifetime + 60s buffer
• SameSite=strict on auth state cookies
• Supabase response validation via Pydantic model_validate
• Remove exc_info=True from JWT failure debug log

Infrastructure:

• CORS: restrict origin fallback to localhost (was "*"), add Allow-Headers
• Redis: replace @lru_cache singleton with explicit getter/setter for test safety
• Prefix-sum optimization for page budget clamping
• Exception chaining in UUID validation
• Add missing esc() XSS helper to session widget
• Add MCP_SERVER_URL to .env.example
• Add .dockerignore, Sentinel support in docker-compose
• Rate limiter: handle None client IP, proxy header trust gating

Test plan

• All 222 unit tests pass
• Pre-commit hooks (format, lint, typecheck) pass
• CI checks pass on GitHub
• Code review by Claude

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[github-actions]

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

---

Code Review Summary

Reviewed all 100 changed files across the auth, Redis, HTTP transport, middleware, routes, tools, models, config, templates, deployment, and test modules. Overall this is a well-structured PR with solid security hardening. The auth flow, rate limiting, and Redis-backed state management are implemented carefully. Below are the issues I found, grouped by severity.

---

Issues

1. Refresh token GET-then-DELETE race condition (Medium)

everyrow-mcp/src/everyrow_mcp/auth.py:486-493

load_refresh_token does a GET then verifies client_id, then DELETE. Between GET and DELETE, a concurrent request could also GET the same token and both would proceed to call exchange_refresh_token. This differs from load_authorization_code which uses GETDEL for atomic consumption.

While Supabase will invalidate the old refresh token on first use (making the second refresh fail), the losing request would then hit the error path and re-store the consumed token (line 507-512), potentially creating a stale refresh token in Redis.

Consider using GETDEL here too, with the client_id check done on the retrieved data:

async def load_refresh_token(
    self, client: OAuthClientInformationFull, refresh_token: str
) -> EveryRowRefreshToken | None:
    if len(refresh_token) > 256:
        return None
    key = build_key("refresh", refresh_token)
    data = await self._redis.getdel(key)  # Atomic consume
    if data is None:
        return None
    rt = EveryRowRefreshToken.model_validate_json(data)
    if rt.client_id != client.client_id:
        # Re-store for the legitimate client
        remaining = settings.refresh_token_ttl
        await self._redis.setex(key, remaining, data)
        return None
    return rt

Fix this →

---

2. CSV cache size check uses character count, not byte count (Low)

everyrow-mcp/src/everyrow_mcp/redis_store.py:150-154

MAX_CSV_CACHE_BYTES = 50 * 1024 * 1024 is named "bytes" but len(csv_text) counts characters. For CSV data with multi-byte unicode, character count underestimates the actual byte size in Redis (which stores UTF-8). For ASCII-dominated CSV this is a non-issue, but the naming is misleading.

# Option A: rename the constant
MAX_CSV_CACHE_CHARS = 50 * 1024 * 1024

# Option B: check actual byte size
if len(csv_text.encode("utf-8")) > MAX_CSV_CACHE_BYTES:

Fix this →

---

3. Accessing private attribute across class boundary (Low)

everyrow-mcp/src/everyrow_mcp/auth.py:527

revoke_token accesses self._token_verifier._revocation_ttl — a private attribute of another class. This creates fragile coupling.

# Consider exposing as a property on SupabaseTokenVerifier:
@property
def revocation_ttl(self) -> int:
    return self._revocation_ttl

Fix this →

---

4. f-string in logger call (Nit)

everyrow-mcp/src/everyrow_mcp/app.py:38

logging.getLogger(__name__).error(f"everyrow-mcp startup failed: {e!r}")

Uses an f-string instead of lazy %s formatting. The string is evaluated even if ERROR level is disabled. Minor, but inconsistent with the %s-style used elsewhere (e.g., auth.py:296, redis_store.py:156).

logging.getLogger(__name__).error("everyrow-mcp startup failed: %r", e)

---

5. Dockerfile missing HEALTHCHECK (Nit)

everyrow-mcp/deploy/Dockerfile

The /health endpoint is registered (line 113 of http_config.py) but the Dockerfile has no HEALTHCHECK instruction. The compose file checks Redis health but not the MCP server container. Adding a healthcheck would improve orchestration reliability:

HEALTHCHECK --interval=30s --timeout=5s --retries=3 \
  CMD python -c "import urllib.request; urllib.request.urlopen('http://localhost:8000/health')"

---

What looks good

• Atomic auth code consumption via GETDEL (auth.py:419) — prevents replay attacks
• Cookie security: httponly=True, samesite="strict", secure=True, scoped path="/auth/callback" — solid
• CORS restriction to the server's own origin — prevents cross-origin attacks from arbitrary domains
• Rate limiting fails open on Redis outage — correct trade-off to avoid blocking legitimate traffic
• Redis key sanitization (redis_store.py:36-42) — prevents key injection via user-controlled values
• Token fingerprinting via SHA-256 for revocation tracking — avoids storing raw tokens
• Poll-token auth for REST endpoints — prevents unauthorized polling even without OAuth headers
• Input validation: UUID validation, schema validation with property limits, inline data size limits, path traversal prevention
• Prefix-sum optimization in clamp_page_to_budget — efficient binary search for token budget
• Clean separation between stdio and HTTP modes throughout the codebase
• Comprehensive test suite: 222 tests covering auth, middleware, routes, result store, Redis utils, and integration scenarios

---

• Read repo setup instructions (CLAUDE.md)
• Review auth module (auth.py)
• Review Redis store (redis_store.py)
• Review result store (result_store.py)
• Review HTTP config & middleware
• Review routes (routes.py)
• Review server changes (server.py, app.py)
• Review tools & helpers (tools.py, tool_helpers.py)
• Review models changes (models.py)
• Review config (config.py)
• Review templates (templates.py)
• Review deployment config
• Review tests for coverage gaps
• Review CI/workflow changes
• Write final summary
  |

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 3m 43s —— View job

---

Code Review: Harden MCP server

• Review auth changes (auth.py, models.py)
• Review middleware changes (middleware.py, http_config.py)
• Review Redis store changes (redis_store.py)
• Review routes and tools changes (routes.py, tools.py)
• Review templates, result store, config, and deployment changes
• Review test changes

Overall this is a solid hardening PR. The changes are well-structured, defensive, and thoroughly tested. I have a few suggestions below but nothing blocking.

---

Findings

1. Auth code re-store on client mismatch — nice pattern (auth.py:425-429)

The GETDEL → re-store pattern when client_id doesn't match is a good defense against cross-client DoS. One thing to note: if the auth code is expired and has a client mismatch, the expiry check at line 423 runs first and returns None without re-storing. This is correct (no point re-storing an expired code), but worth documenting in a comment for future readers.

2. Refresh token TOCTOU window — acceptable tradeoff (auth.py:486-493)

load_refresh_token uses GET → verify → DELETE instead of the old GETDEL. This introduces a small TOCTOU window where two concurrent requests could both GET the same token and pass the client_id check. In practice this is mitigated by:

• MCP SDK serializing token endpoint requests per session
• Supabase's own refresh token rotation (second exchange would fail)

The tradeoff (preventing cross-client DoS) is worth it, but if you wanted belt-and-suspenders, you could use the same GETDEL + re-store-on-mismatch pattern from load_authorization_code.

3. Revocation TTL boundary condition — cosmetic (auth.py:526-527)

remaining = max(0, (token.expires_at or 0) - int(time.time())) + 60
ttl = remaining if remaining > 60 else self._token_verifier._revocation_ttl

When remaining == 60 (token just expired, 0 seconds left + 60 buffer), the > 60 check is False and falls back to _revocation_ttl (3600s). This is harmless but slightly inconsistent — a token with 1 second remaining gets TTL = 61s, while a token with 0 seconds gets TTL = 3600s. Consider >= 60 if you want the buffer to always apply when there's a known expiry, or this is fine as-is since it's conservative.

4. _validate_supabase_code catches broad Exception (auth.py:295)

This catches everything including programming errors. Since this is an auth callback where a clean user-facing error matters, this is pragmatic. The error class name is logged via type(exc).__name__, which is a good compromise. No change needed.

5. Prefix-sum optimization — clean (result_store.py:66-70)

The prefix-sum approach for clamp_page_to_budget is a clear improvement over re-serializing subsets on each binary search iteration. Minor note: the old code's json.dumps(candidate) included array bracket overhead (~2 tokens for []), while the prefix sum doesn't. The difference is negligible.

---

What looks good

• CORS restriction (routes.py:22-28): Replacing "*" with the configured origin + adding Allow-Headers is an important security fix.
• Proxy header trust gating (middleware.py:27, config.py:30-34): Defaulting to False and requiring explicit opt-in via TRUST_PROXY_HEADERS prevents IP spoofing in non-proxied deployments.
• __unknown__ fallback bucket (middleware.py:63-67): The old behavior of skipping rate limiting entirely when IP is None was a bypass vector. Sharing a fallback bucket is the right fix.
• Redis key sanitization (redis_store.py:36-42): The regex [^a-zA-Z0-9._\-] is more thorough than the old replace(":", "_").
• set_redis_client for test injection (redis_store.py:113-116): Replacing @lru_cache with explicit getter/setter is important for test isolation.
• Session widget XSS fix (templates.py:779, 809, 829): Adding esc() and using it on d.status closes an XSS vector.
• Error message sanitization (tools.py): Replacing e!r with structured logging + generic user messages avoids leaking internals.
• Input validation limits (models.py:29-31): MAX_INLINE_ROWS, MAX_INLINE_DATA_BYTES, MAX_SCHEMA_PROPERTIES are sensible bounds.
• UUID validation at model level (models.py:379-385): Catches malformed task IDs before they hit Redis or the API.
• SameSite=strict (auth.py:365, 404): Stricter than lax and appropriate since the cookie is only needed on same-site /auth/callback.
• Supabase response validation (auth.py:547-554): model_validate instead of blind dict indexing prevents confusing KeyError on malformed responses.
• Algorithm restriction (auth.py:82): Removing ES256 reduces attack surface when Supabase only uses RS256.
• Health endpoint Redis ping (http_config.py:105-109): Returns 503 when Redis is unreachable — useful for load balancer health checks.
• _clear_task_state HTTP guard (app.py:19-20): Prevents the server from touching the filesystem in HTTP mode.
• Content-Disposition sanitization (routes.py:121): Filtering the task_id prefix prevents header injection.
• Refresh token re-store on Supabase failure (auth.py:506-513): Prevents users from being locked out if a transient Supabase error occurs.
• Dockerfile: Non-root user, two-stage build, STOPSIGNAL SIGTERM.
• docker-compose.yaml: Redis password required, no exposed Redis ports, memory limits, health checks.

Test coverage

The new tests in test_auth.py cover the key new behaviors well: atomic auth code consumption, client_id mismatch re-store, scope narrowing, expiration defense-in-depth, revocation TTL, and Supabase response validation. The middleware tests cover the None client IP fallback. The route tests validate the new CORS headers.

---

Verdict: Looks good. The changes are well-motivated, defensive, and properly tested. The issues noted above are all low-severity. Ship it.