Security hardening: auth replay, encryption guards, rate limits, CORS

[RafaelPo]

Summary

Addresses 18 security audit findings (7 HIGH, 8 MEDIUM, 3 LOW) across the MCP HTTP server, plus test and observability improvements. 24 files changed, 518 insertions, 177 deletions.

HIGH

• H1 — Fail-closed encryption: encrypt_value/decrypt_value raise RuntimeError in HTTP mode when UPLOAD_SECRET is missing. Required regardless of --no-auth.
• H2 — ALLOW_NO_AUTH strict check: only "1" disables auth.
• H3 — Redis password required in authenticated HTTP mode (hard error).
• H4 — HTTPS-only URL validation on mcp_server_url and supabase_url (localhost exempt).
• H5 — Referrer-Policy: no-referrer meta tag in both widget HTML templates.
• H6 — Auth code and refresh token re-storage uses SET ... NX to prevent concurrent overwrites.
• H7 — OAuth state consumed atomically in handle_start, re-stored to narrow replay window.

MEDIUM

• M1 — Security events (revoked tokens, failed polls, rate limits) logged at WARNING.
• M2 — Stack traces suppressed in error responses; generic messages returned.
• M3 — app.openLink() calls guarded with https?:// scheme check.
• M4 — rel="noopener noreferrer" on linkified anchors in results widget.
• M5 — Removed in-memory rate limit fallback — Redis is a hard dependency in HTTP mode.
• M6 — Rate limit TTL always refreshed (removed nx=True on EXPIRE).
• M7 — CORS wildcard origin for widget endpoints (auth is via Bearer tokens, not cookies).
• M8 — Content-Type allowlist on upload endpoint (415 for unsupported types).

LOW

• L1 — Per-user upload rate limiting via atomic Redis INCR+EXPIRE pipeline.
• L4 — Remote Redis without SSL fails in HTTP mode (warns in stdio).

Additional

• Upload tool fails early when client has no API token (instead of generating a signed URL that fails later).
• Direct path_params["state"] lookup in handle_start (was .get("state", "")).
• E2e integration test fixed: _http_state fixture now swaps results tool to HTTP variant.
• _get_fernet.cache_clear() in test override_settings for correct encryption behavior.
• Removed dead Sentinel password+no-ssl warning (config validator catches this earlier).

Test plan

• 298 unit tests pass, 28 skipped (integration tests gated by RUN_INTEGRATION_TESTS=1)
• 9/9 e2e tests pass (including 2 real-API integration tests)
• New test_security_hardening.py: HTTPS URL validation, encryption HTTP guard, Redis SSL enforcement (16 tests)
• New TestCorsOriginValidation in test_routes.py (3 tests)
• New TestContentTypeValidation in test_uploads.py
• Pre-commit hooks pass (ruff format, lint, basedpyright typecheck)
• Manual: start with --http --no-auth without UPLOAD_SECRET → startup error
• Manual: set ALLOW_NO_AUTH=false → startup error
• Manual: set MCP_SERVER_URL=http://remote.example.com → validation error
• Manual: set REDIS_HOST=remote.example.com without REDIS_SSL=true → validation error

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[RafaelPo]

@claude code review

[RafaelPo]

@claude code review. focus on everyrow-mcp/src/everyrow_mcp/uploads.py

[github-actions]

Claude finished @RafaelPo's task in 1m 59s —— View job

---

Code Review: uploads.py

Overall the changes are well-structured. The peek-then-consume pattern (get_upload_meta → validate → pop_upload_meta) is the right approach for one-time-use URLs, and the per-user rate limiting is a solid addition. A few items below.

---

1. Duplicate comment (nit) — uploads.py:245

The comment # Retrieve and decrypt the user's API token appears twice: once before the Content-Type check (line 245) and once before the actual decryption (line 257). The first one is a leftover from the diff and should be removed since lines 246–255 are checking Content-Type, not decrypting anything.

Fix this →

---

2. Missing Retry-After header on upload 429 — uploads.py:277-280

The middleware rate limiter (middleware.py:83-88) returns a Retry-After header on 429 responses. The upload rate limiter does not:

# uploads.py:277-280 — no Retry-After
return JSONResponse(
    {"error": "Upload rate limit exceeded. Try again later."},
    status_code=429,
)

For consistency and to help clients back off correctly, consider adding the header. You can get the TTL from Redis:

ttl = await redis_client.ttl(rl_key)
return JSONResponse(
    {"error": "Upload rate limit exceeded. Try again later."},
    status_code=429,
    headers={"Retry-After": str(max(ttl, 1))},
)

Fix this →

---

3. Sentry's pipeline race-condition finding — already addressed

Sentry flagged the incr/expire as non-atomic. The current code (uploads.py:272-275) already wraps both in a pipeline, which is the same pattern used in middleware.py:76-79. A Redis pipeline batches commands into a single round-trip, so both execute together. This is the established pattern in the codebase and is fine as-is.

---

4. Sliding vs. fixed window — design note

The middleware rate limiter uses fixed windows (key includes window_id = int(time.time()) // window_seconds), while the upload rate limiter uses a sliding window (plain key with TTL refreshed on each hit). The sliding approach is more restrictive — the counter never resets until the user is quiet for the full window — so there's no security concern. Just noting the difference in case you want them consistent.

---

5. Content-Type bypass when header is omitted — uploads.py:249

if content_type and content_type not in _ALLOWED_CONTENT_TYPES:

An empty Content-Type header (or no header) passes the check. This is probably intentional — curl with -T may omit it — but worth confirming this is the desired behavior. If you want to enforce a Content-Type, the condition would need to flip to an allowlist-only check.

---

Summary

The security improvements are sound. The peek-then-pop pattern, encryption guard, early token check, and per-user rate limiting all look correct. The two actionable items are the duplicate comment (nit) and the missing Retry-After header (minor consistency issue).
| Branch: security/mcp-http-audit