Harden MCP server and fix HTTP integration tests

[RafaelPo]

Summary

• Harden MCP server for public deployment (input validation, rate limiting fixes, auth hardening)
• Fix auth rate limiter crash when client IP is None
• Fix HTTP results tool re-registration to properly replace schema
• Test real Redis results flow in HTTP integration tests — remove mocked try_cached_result/try_store_result and exercise the full pipeline: submit → poll → results → CSV download with pagination verification

Test plan

• Unit tests pass (215 passed, 15 skipped)
• HTTP integration tests pass with real Redis + API key (3/3 passed)
• Screen pipeline: submit → poll → results → CSV download verified
• Agent pipeline: pagination (page_size=1), cache hit on page 2, full CSV download with column verification
• Dual polling mode (REST + MCP tool) verified

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[github-actions]

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

---

Code Review: Harden MCP server and fix HTTP integration tests

• Read repo guidelines
• Analyze PR diff and changed files (41 files, +6418/-1112)
• Review security hardening changes
• Review auth and rate limiting changes
• Review new modules (middleware, routes, redis_store, result_store, tool_helpers)
• Review test coverage
• Post review summary

---

Overall this is a well-structured PR that significantly improves the codebase: the monolithic state.py and redis_utils.py are decomposed into focused modules (redis_store.py, result_store.py, routes.py, middleware.py, tool_helpers.py), input validation is much more thorough, and there's a solid deployment story with Docker. The security hardening (rate limiting, key sanitization, token validation, proxy header trust gating) is sound.

Below are the issues I found, ordered by severity.

---

Issues

1. _UNSAFE_decode_server_jwt skips signature verification
auth.py:186-196 — The _UNSAFE_decode_server_jwt method decodes JWTs with verify_signature=False. The comment says it's safe because the token comes from Supabase's token endpoint over HTTPS, which is a reasonable trust model for a server-to-server flow. However, the method is @staticmethod and reachable from any code that has a reference to the auth provider. Consider making it a module-level private function (harder to discover) or adding runtime assertions (e.g., that it's only called from _issue_token_response).

Fix this →

2. CORS origin defaults to "*" when mcp_server_url is empty
routes.py:23-24 — _cors_headers() falls back to "*" when settings.mcp_server_url is empty (which happens in no-auth dev mode). This means the download and progress endpoints would accept cross-origin requests from any domain in that mode. While no-auth mode is dev-only and gated by ALLOW_NO_AUTH, consider explicitly setting the origin to the local URL (e.g., http://localhost:{port}) instead of "*".

Fix this →

3. Singleton Redis client via @lru_cache is not test-safe
redis_store.py:97-106 — get_redis_client() uses @lru_cache to create a module-level singleton. Tests patch it per-test in conftest, but if any import path bypasses the patch (e.g., calling the function before the patch is applied), it silently uses the production-configured client. Consider using a module-level variable with an explicit set_redis_client() for testing, or clear the cache in the fixture.

4. Missing Access-Control-Allow-Headers on CORS preflight
routes.py:52-56 — The OPTIONS handler returns 204 with Allow-Origin and Allow-Methods, but does not include Access-Control-Allow-Headers. Browsers that send preflight with Access-Control-Request-Headers: Authorization would be rejected. This affects the download endpoint which requires a ?token= param (so it works), but if any future change moves the token to a header, CORS will break.

5. load_authorization_code — GET-then-DELETE is not atomic
auth.py:408-426 — The pattern of GET followed by client_id check followed by DELETE is subject to a race: two concurrent requests could both GET the code successfully before either DELETEs it. Under load, this means a code could be used twice. Consider using a Lua script or Redis transaction with WATCH to make this atomic, or accept the risk given the short TTL and one-time nature of auth codes.

6. clamp_page_to_budget binary search uses O(n log n) JSON serialization
result_store.py:41-61 — Each iteration of the binary search calls json.dumps() on a slice of records to estimate tokens. For large pages (up to 10k rows), this means multiple large JSON serializations. Consider caching the per-row sizes or using a running total instead.

Fix this →

---

Minor / Nits

7. _validate_task_id raises bare ValueError from except ValueError
models.py:379-385 — The re-raise without from loses the original traceback. Use raise ValueError(...) from exc.

8. Docker STOPSIGNAL SIGTERM is the default
deploy/Dockerfile:30 — STOPSIGNAL SIGTERM is the Docker default. Not harmful but redundant.

9. .env.example shows MCP_SERVER_URL is required but not listed
deploy/.env.example — The compose file requires MCP_SERVER_URL but the example env file doesn't include it. Users will get a confusing error on first run.

Fix this →

10. esc() function in session widget HTML is undefined
templates.py (SESSION_HTML, line ~737) — The session widget uses esc(cls) and esc(d.status) but never defines the esc() helper (unlike the results widget which defines it). This will throw a runtime error when the session status renders.

Fix this →

---

Looks Good

• Redis key sanitization (redis_store.py:37-43): _KEY_UNSAFE regex + build_key() properly prevents key injection.
• Rate limiting fail-open (middleware.py:85-86): Redis outage doesn't block legitimate traffic.
• Proxy header trust gating (middleware.py:20-34): trust_proxy_headers must be explicitly enabled.
• Token length limits (auth.py:413,476): Authorization code and refresh token lengths are bounded at 256 chars before Redis lookup, preventing abuse.
• No-auth deployment guard (server.py:58-64): Requires explicit ALLOW_NO_AUTH=1 env var.
• Input validation (models.py): Pydantic models with extra="forbid", min_length, ge/le, and custom validators for schemas and paths.
• Poll token auth on REST endpoints (routes.py:41-49): Uses secrets.compare_digest for timing-safe comparison.
• Test infrastructure (conftest.py): Real Redis instance per test with proper flush isolation.
• Decomposition: Breaking state.py into tool_helpers.py + redis_store.py + result_store.py + routes.py is a clear improvement in modularity.

---

|

[sentry]

Bug: For CSV results over 50MB, store_result_csv fails silently, leading to a response with a download link that results in a 404 error upon access.
_{Severity: MEDIUM}

Suggested Fix

Modify store_result_csv to either raise an exception or return a status indicator (e.g., a boolean) when it skips storing a large CSV. The calling function, try_store_result, should then check for this failure and return None to trigger the fallback mechanism, which correctly serves inline results without a broken download link.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: everyrow-mcp/src/everyrow_mcp/redis_store.py#L154-L160

Potential issue: When a task's CSV result exceeds 50MB, the `store_result_csv` function
silently skips storing the file in Redis without raising an exception. The calling
function, `try_store_result`, proceeds as if storage was successful, generating a
response that includes a download URL for the CSV. However, when the user attempts to
use this URL, the download endpoint cannot find the CSV in Redis and returns a 404
error. The user is presented with a broken download link while the inline preview works
correctly.