perf+security+test: deferred audit follow-ups (health growth, first-run token, orchestrator coverage)#100
Conversation
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.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesFirst-run access and lifecycle
API and orchestrator coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 92.46% 92.61% +0.14%
==========================================
Files 30 31 +1
Lines 3118 3180 +62
==========================================
+ Hits 2883 2945 +62
Misses 235 235
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/deps.py`:
- Around line 97-112: The first-run setup flow accepts the sensitive token
through a query parameter, exposing it in access logs and browser history.
Update `_require_first_run_access` and its startup/help messaging to remove or
de-emphasize `request.query_params.get("setup_token")`, directing operators to
the `X-Setup-Token` header or `setup_token` form field instead; ensure no
generated URL or log advertises the token in a query string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48045f47-2d6f-4e34-a724-0277adcad7a9
📒 Files selected for processing (10)
app/deps.pyapp/main.pyapp/routers/auth.pyapp/setup_token.pyapp/templates/auth.htmltests/conftest.pytests/test_main_routes.pytests/test_orchestrator_coverage.pytests/test_setup_token.pytests/test_worker_api_coverage.py
…r _run_vacuum + first-run token A messy multi-rebase during PR stacking silently dropped the database.py half of the health_events change (HEALTH_CHECK_RETENTION_DAYS, the check-sample purge, and vacuum_database) while keeping main.py's _run_vacuum + scheduler job — leaving main calling a database function that no longer existed. Restore the database.py logic and its tests, and add coverage for main._run_vacuum (success + error) and the lifespan first-run setup-token generation (the codecov/patch gaps). Full pytest 1081 passed; ruff check + format clean.
…odeRabbit) A ?setup_token= URL leaks the secret into reverse-proxy access logs and browser history. Drop query-param support in _require_first_run_access (form field + X-Setup-Token header only); the GET /register page now renders behind just the network check so the operator can reach it, the token is a visible form field they type from the logs, and the startup log no longer embeds the token in a URL.
|
@coderabbitai review The query-param setup-token finding is addressed in f78c323: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
gh pr view 100 --repo GeiserX/CashPilot --json commits --jq '.commits[] | select(.oid | startswith("f78c323")) | .oid' || true
echo "---"
gh api repos/GeiserX/CashPilot/commits/f78c323 --jq '.files[].filename' || trueLength of output: 533 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== deps.py ==="
cat app/deps.py
echo "=== auth.py ==="
cat app/routers/auth.py
echo "=== main.py (relevant parts) ==="
grep -n "setup_token\|_setup_token\|client_ip" app/main.pyLength of output: 13220 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '300,345p' app/main.pyLength of output: 2326
Nice fix. I'll trigger a full review of the changes. ヾ(⌐■_■)ノ♪ ✅ Action performedReview finished.
|
1 participant
Implements three items deferred in #98, each independently verified (
ruff check+ruff format --check+ fullpytest1075 passed). (Re-opened as a fresh PR — the original #99 auto-closed when its stacked base branch was deleted on merge; identical content, now rebased directly onmain.)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% 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])
Two lowest-covered, most Docker-privileged modules: orchestrator.py 38%→73%, worker_api.py 44%→74% (61 tests, Docker SDK fully mocked). Plus a
_login_attempts/setup-token autouse reset fixture removing latent order-dependent flake.Verified:
ruff(check + format) clean; fullpytest1075 passed.Summary by CodeRabbit
Security
Maintenance
Reliability