fix: unblock event loop, thread-safe tokens, Cloudflare WAF guide, CI - #23
Conversation
FastMCP (mcp 1.27) runs sync tool functions inline in the event loop with no thread offload. 27 of 33 Proxmox tools are sync wrappers around blocking proxmoxer calls (5s timeout x 2 retries), so a single unreachable node froze the whole server -- every other client request stalled behind it. - server._metric_tool now builds one always-async wrapper: coroutine tools are awaited as before, sync tools run via anyio.to_thread.run_sync so their blocking body executes on a worker thread, off the loop. - iscoroutine-ness is resolved once per tool; @wraps still exposes the original signature so FastMCP derives the input schema correctly. Metrics stay in the finally on the loop side. Dropped the dead sync_wrapper and trailing-whitespace lines; moved inspect to module top, added anyio and functools imports. - ProxmoxClient._connections is now hit from worker threads, so guard the cache with a threading.Lock around the get-or-create in _get_connection and the pop() in api_call -- held only across the cheap dict access and connection construction, never across the network call. Tests: connection cache survives concurrent multi-thread access (single and many nodes), one connection built per node. Full suite 249 passed.
With sync tools now running on a worker-thread pool, TokenStore mutated its in-memory dict unlocked -- concurrent issue/validate could raise "dictionary changed size during iteration". Separately, verify_totp had no replay protection: a 6-digit code stayed valid for its whole 30s step, so the same code could be redeemed more than once. - TokenStore grows a threading.RLock (re-entrant so issue -> _cleanup and revoke_named -> revoke nest without deadlock); every in-memory dict read/mutation in issue/validate/revoke/revoke_named/count_named/list_named /_cleanup is now guarded. The lock is never held across a blocking call. - ClientStore.verify_totp records the last accepted 30s timestep per SEED OWNER (owner_client_id for delegated clients, else client_id) and rejects any code whose matched step is not strictly newer -- so two derived clients sharing one owner's seed cannot each spend the same code. The matched step is found by per-offset pyotp verify over -1/0/+1. - Moved _logger below the imports in auth.py to clear ruff E402. Tests: replay rejected on reuse, fresh later-step code accepted, replay keyed across the owner; TokenStore single-thread contract unchanged plus a concurrent issue+validate storm runs clean. Full suite 249 passed.
MCP was unusable behind cloudflared + Cloudflare WAF: headless MCP clients trip Bot Fight Mode / Browser Integrity Check / Managed Rules (403 or a challenge page), and Cloudflare Access owns the Authorization header and strips the client's bearer, so /mcp saw no token and returned a bare 401. The failure looked identical to a client misconfig, with nothing pointing at the edge. - auth_middleware: when a 401 fires but the request carries Cloudflare's cf-ray edge header (no usable bearer), enrich the JSON body with a hint pointing at docs/cloudflare.md and log a warning. The 401 status and the WWW-Authenticate header are unchanged -- OAuth discovery still works; a direct unauth request (no cf-ray) keeps the minimal body. - docs/cloudflare.md (new): copy-pasteable WAF skip rule for /mcp + /oauth/ + /.well-known/ (skip Super Bot Fight Mode, Managed Rules, Browser Integrity Check), Access bypass/passthrough guidance, a no-cache rule for /mcp, the allowed_hosts/trusted_proxies reminder, and a curl verify flow. - Link the guide from docs/troubleshooting.md and README (Expose publicly). - Remove the unused `start` local in MetricsMiddleware (ruff F841). Tests: cf-ray 401 carries the hint + keeps WWW-Authenticate; no-cf-ray 401 stays minimal; helper unit checks. Suite 237 passed (bmc_registry ignored, broken on main).
The BMC backends were consolidated onto a single RedfishBackend, but tests/test_bmc_registry.py still imported the deleted idrac/supermicro stub modules, which broke collection for the whole suite. This restores a runnable suite and adds baseline lint/CI scaffolding. - tests/test_bmc_registry.py: drop stale IDRACStubBackend / SupermicroStubBackend imports, import RedfishBackend instead; assert idrac/supermicro device types build to RedfishBackend; rewrite the former stub-error test to assert power_status() returns an error dict for an unreachable host (no real network). - bmc/redfish.py: remove unused `import asyncio`. - proxmox/system.py: remove unused `from ..utils import filter_fields`. - pyproject.toml: add [tool.ruff] (py311, line-length 100, select E/F, ignore E501), [tool.pytest.ini_options] (testpaths, asyncio_mode auto), make `anyio` an explicit dependency, add a `dev` extra. - .github/workflows/ci.yml: push/PR matrix (3.11, 3.12) running ruff + pytest. Note: repo-wide `ruff check src/ tests/` is not fully clean on this branch alone -- pre-existing errors in auth.py (E402) and __main__.py (F841), plus F401/E401/F541 in several test files, are owned by sibling branches (feat/batch-a-mechanical-fixes, feat/ci-and-tests). ruff is clean on all files in this change's scope. Tests: 236 passed.
The new `ruff check src/ tests/` CI step tripped on lint debt that predates it, which would have made the very first CI run red even though the production code is clean. - test_utils: drop unused `import time`; test_snapshots: drop unused `import pytest`. - test_dashboard_chat: split a multi-import line (E401) and drop an unused import. - test_integration: drop the `f` prefix on f-strings with no placeholders (F541). Tests: 297 passed.
Verifying the "passing a URL blocks us" report against a live deployment showed it is not URLs as such -- a plain URL (even an SSRF-style metadata IP) passes. Cloudflare's OWASP Managed Ruleset blocks any request whose tool argument matches an attack signature (path traversal, /etc/passwd, RFI/LFI URLs, SQL quotes) -- exactly what legitimate admin commands look like. The block is a full "Sorry, you have been blocked" HTML page at the edge, so there is no server-side remedy; the Skip -> Managed rules box in section 1 is the fix. - Symptom: document the edge-block HTML page, distinct from the JSON 401. - Why-it-happens: name OWASP CRS and give legit-command examples.
Review follow-ups on the thread-pool migration. A ProxmoxAPI wraps a requests.Session, which is not thread-safe. Locking the connection *cache* still handed the same Session to several workers at once -- previously safe only because the event loop serialised every sync tool. The cache is now thread-local: no shared mutable state, no lock, and a transient error evicts only the failing thread's socket instead of yanking a connection out from under a thread mid-request on it. TOTP replay rejection counted towards the 5-strike lockout. The dashboard prompts for 2FA on several actions, so logging in and then minting a token inside the same 30 s step meant re-submitting the code the authenticator still displays -- five of those locked a legitimate operator out of their own panel. check_totp() now reports OK / INVALID / REPLAY; only INVALID is a failed auth attempt, and a replay says so instead of sending people clock-hunting. Also: throttle the Cloudflare 401 warning (one line per 5 min, with a suppressed count) so an anonymous scanner can't flood the journal; anchor the WAF skip expression with starts_with and document what the skip gives up; scope CI push to main, cache pip, use the [dev] extra it already declared; anyio>=4; drop the dead line-length setting. Tests: per-thread connection identity and thread-scoped eviction; replayed codes never accumulate lockout strikes; the 401 log does not grow per request. Dropped the Starlette harness in test_cloudflare_diagnostic that reimplemented auth_middleware's 401 branch -- it would have passed even if the middleware stopped calling the helper.
|
Solid work — the three problems are real and the Cloudflare diagnosis (OWASP CRS false-positiving on tool arguments) is spot on. I pushed a follow-up commit rather than blocking, because two things in the thread-pool migration needed fixing before merge.
|
|
Both catches are right, and the Session one is the more important of the two -- I read that lock as sufficient and never went one level down to The TOTP one I'd have filed as cosmetic and it isn't. Logging in and then minting a token inside one 30 s step is the ordinary path through the dashboard, not an edge case, so the lockout would have fired on normal use -- and the "check that your device clock is in sync" message would have sent people hunting a problem they don't have. I've rebased #24 (the security audit) onto e77225f -- clean, no conflicts, One follow-up in the same family as your Session find, if you want it in scope: |
Why
Three independent reliability/operability gaps surfaced while reviewing the server:
/mcphard-requiresAuthorization: Bearer. Cloudflare's Bot Fight Mode / Browser Integrity Check / Managed Rules challenge or block non-browser MCP clients, and Cloudflare Access strips the bearer for its own JWT -- producing 401/403 only through the public URL, with nothing in the response to explain why. (docs/dashboard.mdalready noted Cloudflare Tunnel dropping the header.)rufffailed onmain,test_bmc_registry.pystill imported BMC backends removed in the Redfish migration (collection error -> whole suite un-runnable), and nothing ran on push.Changes
Reliability
perf(server): every tool is now async from FastMCP's view; sync bodies run viaanyio.to_thread.run_sync, keeping blocking proxmoxer calls off the event loop. Metrics + audit logging preserved in the wrapper.fix:ProxmoxClient's connection cache is lock-guarded (now reached from worker threads).TokenStoreuses a re-entrant lock around every in-memory mutation that previously raced with the SQLite-only lock.verify_totpgains replay protection keyed by the seed owner, so a 6-digit code can't be spent twice within its window (and two derived clients can't each replay one code).Cloudflare
docs/cloudflare.md: the exact WAF Skip rule for/mcp,/oauth/,/.well-known/; Cloudflare Access bypass/passthrough; a no-cache rule for the SSE stream; and theserver.allowed_hosts/server.trusted_proxiesreminder. Linked fromdocs/troubleshooting.mdand the README.auth_middlewarenow detects acf-rayedge header on an unauthorized request and adds ahintto the 401 body plus a server log line pointing at the guide. The 401 status andWWW-Authenticateheader are unchanged, so OAuth discovery still works.CI & hygiene
.github/workflows/ci.yml: ruff + pytest on Python 3.11 and 3.12.[tool.ruff]/[tool.pytest.ini_options]added to pyproject;anyiopromoted to an explicit dependency.test_bmc_registry.py(idrac/supermicro now assertRedfishBackend); removed unused imports across src and tests so the lint gate passes.Tests
297 passed;ruff check src/ tests/clean.