perf+security+test: deferred audit follow-ups (health growth, first-run token, orchestrator coverage)#99
Closed
GeiserX wants to merge 6 commits into
Closed
perf+security+test: deferred audit follow-ups (health growth, first-run token, orchestrator coverage)#99GeiserX wants to merge 6 commits into
GeiserX wants to merge 6 commits into
Conversation
Ported from improvements made in CashPilot-Desktop (the Go port), applied where the web app didn't already have them: - dashboard: surface crashes_7d + an "unstable" flag (>=3 crashes in the 7-day health window) on each deployed service row, and render a distinct "unstable" badge (worst tone + explicit label + restarts/crashes in the tooltip) so a crash-looping earner is legible at a glance, not just via a lower score number. Reuses the existing get_health_scores data (already computes crashes/restarts). - metrics: log a startup WARNING when Prometheus /metrics is enabled — it is served unauthenticated and exposes earnings/health, so keep it behind a proxy and off untrusted networks. Tests: two new deployed-row tests (unstable at/above vs below the threshold). ruff clean; full suite 997 passed.
…op stalls, authz, error surfacing
From a full-codebase 8-lens audit. Security + correctness fixes:
Security:
- worker_api: replace the bypassable deploy-spec volume DENYLIST with a
realpath-normalized check that blocks /var/run + /run parents (closing the
/var/run:/x -> docker.sock -> host-root escape) and adds a network_mode
allowlist (permits host for mysterium, blocks container:<id>). Authenticate the
previously-unauthenticated worker mini-UI status page.
- main: deploy via /api/workers/{id}/command is now owner-gated (was writer — an
owner-gate bypass of /api/deploy). Single-source the secret-config-key set from
database.SECRET_CONFIG_KEYS (the shorter local list left remember_web/xsrf_token
unmasked in /api/collectors/meta).
- auth/routers: invalidate outstanding sessions on user role-change and deletion
(a demoted/deleted user kept access until the token expired) via the existing
per-user epoch mechanism.
- app.js: sanitizeHint rejects non-http(s)/mailto hrefs (blocks javascript:).
Correctness (event-loop stalls under async):
- main: _validate_worker_url's DNS resolution now runs via asyncio.to_thread at
its sole call site (a slow-DNS worker no longer stalls the whole UI event loop);
the validator stays sync so its existing tests are unaffected.
- worker_api: 4 unthreaded docker_available() calls now use asyncio.to_thread
(they blocked exactly when Docker was degraded).
- main: fire-and-forget collection tasks keep a reference + log exceptions
(_spawn); record_collection_start moved inside the try; the hourly collector
gather is bounded by a semaphore (anti-storm as the catalog grows).
Consistency / error-surfacing:
- main: both container-action route families now record BOTH the health event and
the metrics lifecycle (they had diverged).
- exchange_rates: wire the previously-dead rates_stale() into get_all() so the UI
can flag stale FX.
- app.js: stop fabricating $0.00 / eternal spinners / a false-healthy bell on
fetch errors — surface the error, keep last values; remove dead code.
Tests: negative-role test for deploy-via-command (owner-only); updated the route
side-effect + deploy-gate tests to the new behavior; added a _login_attempts
autouse reset fixture (removes a latent order-dependent flake).
Verified: ruff clean; full pytest 998 passed.
health_events is dominated by check_ok/check_down uptime samples (one per service every 5 minutes), yet get_health_scores only reads a bounded window (/api/health/scores caps it at 90 days) while retention kept everything 400 days. Trim samples to HEALTH_CHECK_RETENTION_DAYS=95 (just past the max query window, so no allowed query can out-range its own samples) while keeping the rare lifecycle events (start/stop/restart/crash) the full 400 days. Add a weekly VACUUM job (commit-first, WAL-checkpoint TRUNCATE) since SQLite never shrinks the file on DELETE alone — without it the DB keeps its high-water-mark size forever. Uptime% is unchanged for every allowed query. Tests cover the split-retention boundary + a clean VACUUM run.
Add 61 tests (Docker SDK fully mocked) for the two lowest-covered, most privileged modules: orchestrator.py 38%->73%, worker_api.py 44%->74%. Covers _collect_stats (zero-delta CPU, missing keys, APIError), _find_container name/ label/ValueError fallback, get_status[_light] docker-unavailable + corrupted- container-skip paths, _verify_api_key negative paths, _validate_deploy_spec rejections (privileged, cap_add, network_mode, blocked volume roots incl. subpaths; host/bridge + named volumes allowed), and every command endpoint's success + ValueError->404 / RuntimeError->503 branches. Also update the lifespan-scheduler test to expect the new db_vacuum interval job (added in the previous commit).
The "private network only" guard on first-run /register trusts request.client.host, which behind a reverse proxy is the proxy (a loopback/ private address) — so the check always passes and the first public visitor could seize the owner account. Add a proxy-independent gate: while no users exist, startup generates a one-time setup token, persists it (encrypted config), and logs it; /register then requires it (query param, X-Setup-Token header, or form field) and it is retired the moment the owner account is created. Also resolve the real client IP behind a trusted proxy (opt-in CASHPILOT_TRUSTED_PROXY, right-most X-Forwarded-For), used by both the first-run network check and the login rate limiter — which previously collapsed every proxied client into one bucket keyed by the proxy IP. Tests: token module + client_ip (trusted/untrusted) + the first-run guard (missing/wrong/query/header/form token, public-IP-still-blocked, network-only fallback once retired) + register integration (403 without token, 303 + token cleared with it); conftest resets the token global between tests.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements three of the items deferred in #98, each independently verified (
ruff check+ruff format --check+ fullpytest). Stacked on #98 (fix/web-audit-security-correctness) — review/merge that first; GitHub will retarget this tomainonce it lands.Two larger deferred items — per-worker fleet keys and the
main.pygod-module split — are still coming as their own follow-up PRs.1. Bound
health_eventsgrowth ([PERF])check_ok/check_downuptime samples (one per service every 5 min) dominate the table, butget_health_scoresonly reads a bounded window (/api/health/scorescaps at 90 days) while retention kept everything 400 days.HEALTH_CHECK_RETENTION_DAYS=95(just past the 90-day max query, so no allowed query can out-range its own samples — uptime% is unchanged) while keeping rare lifecycle events (start/stop/restart/crash) the full 400 days.VACUUMjob (commit-first,wal_checkpoint(TRUNCATE)) — SQLite never shrinks the file onDELETEalone.2. First-run setup token + trusted-proxy client IP ([SEC])
The "private network only" guard on first-run
/registertrustsrequest.client.host, which behind a reverse proxy is the proxy (loopback/private) — so it always passes and the first public visitor could seize the owner account./registernow requires it (query param /X-Setup-Tokenheader / form field) and it is retired the instant the owner is created.CASHPILOT_TRUSTED_PROXY, right-most XFF) — used by the first-run network check and the login rate limiter, which previously collapsed every proxied client into one bucket keyed by the proxy IP.3. orchestrator/worker_api coverage ([TEST])
The two lowest-covered, most Docker-privileged modules: orchestrator.py 38%→73%, worker_api.py 44%→74% (61 tests, Docker SDK fully mocked). Covers stats parsing edge cases, container-lookup fallbacks, docker-unavailable paths,
_verify_api_keynegative paths,_validate_deploy_specrejections, and every command endpoint's success + error branches. Plus a_login_attempts/setup-token autouse reset fixture removing latent order-dependent flake.Verified:
ruff(check + format) clean; fullpytest1075 passed.