fix(security): code-audit findings — container escape, event-loop stalls, authz, error surfacing#98
Merged
Merged
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates background collection, service health and lifecycle reporting, worker security, authentication invalidation, metrics warnings, frontend resilience, input sanitization, and related test coverage. ChangesCollection and service operations
Worker security and proxy execution
Authentication and observability safeguards
Frontend resilience and dashboard rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
+ Coverage 92.09% 92.11% +0.01%
==========================================
Files 30 30
Lines 3085 3118 +33
==========================================
+ Hits 2841 2872 +31
- Misses 244 246 +2
🚀 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/main.py`:
- Around line 1746-1758: api_worker_command lacks the health and container
lifecycle bookkeeping performed by other management routes. In
api_worker_command, mirror the corresponding record_health_event and
metrics.record_container_lifecycle calls used by /api/services handlers,
covering the proxied command’s success and failure/lifecycle outcomes;
alternatively, explicitly document this endpoint as a raw proxy if that behavior
is intentional.
🪄 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: b1bfcd97-5229-4ef8-83fa-0bbf3fc9ba4b
📒 Files selected for processing (11)
app/database.pyapp/exchange_rates.pyapp/main.pyapp/metrics.pyapp/routers/users.pyapp/static/js/app.jsapp/worker_api.pytests/conftest.pytests/test_coverage_gaps.pytests/test_main_deploy_routes.pytests/test_main_routes.py
Comment on lines
1746
to
1758
| @app.post("/api/workers/{worker_id}/command") | ||
| async def api_worker_command(request: Request, worker_id: int, body: WorkerCommand) -> dict[str, Any]: | ||
| """Send a command to a worker by proxying to its REST API.""" | ||
| _require_writer(request) | ||
| # Deploy is owner-gated everywhere else (see /api/deploy/{slug}); a writer | ||
| # must not be able to bypass that gate by sending command="deploy" here. | ||
| if body.command == "deploy": | ||
| _require_owner(request) | ||
| else: | ||
| _require_writer(request) | ||
|
|
||
| worker = await database.get_worker(worker_id) | ||
| url, headers = _get_verified_worker_url(worker) | ||
| url, headers = await _get_verified_worker_url(worker) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the UI frontend calls /api/workers/{id}/command or only slug-specific routes
rg -n "workers/.*command\|/api/services/.*\(restart\|stop\|start\|remove\)" app/static/js/ --type=jsRepository: GeiserX/CashPilot
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate api_worker_command and related lifecycle recording paths
rg -n "api_worker_command|record_health_event|record_container_lifecycle|unstable" app/main.py app -g '!app/static/**'
# Show the relevant section around api_worker_command
sed -n '1720,1795p' app/main.py
# Find frontend / API usages of worker command versus slug-specific routes
rg -n "/api/workers/.*/command|api/workers/.*/command|/api/services/.*/(restart|stop|start|remove)|api/services/.*/(restart|stop|start|remove)" app -g '!app/static/**'Repository: GeiserX/CashPilot
Length of output: 6914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether worker-side container endpoints already record lifecycle/health data
rg -n "api/containers/.*/(deploy|stop|restart|start)|record_health_event|record_container_lifecycle|unstable" app/worker_api.py app/main.py app -g '!app/static/**'
# Show the relevant worker API section
sed -n '1,260p' app/worker_api.py
# Show the slug-specific routes for comparison
sed -n '760,830p' app/main.py
sed -n '1038,1098p' app/main.pyRepository: GeiserX/CashPilot
Length of output: 19873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any in-repo callers/docs of the raw worker command route
rg -n "/api/workers/\{worker_id\}/command|api_worker_command|workers/.*/command" .
# Show the frontend container-action calls, if any, for comparison
rg -n "/api/services/\{slug\}/(restart|stop|start)|/api/(restart|stop|remove)/\{slug\}|worker_id=" app/static app/templates app/main.pyRepository: GeiserX/CashPilot
Length of output: 2446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tests for /api/workers/{id}/command to see intended behavior
sed -n '500,670p' tests/test_main_deploy_routes.py
sed -n '660,730p' tests/test_coverage_gaps.py
# Show the route helper it uses, if any
rg -n "def _proxy_worker_command|_proxy_worker_deploy|record_health_event|record_container_lifecycle" app/main.pyRepository: GeiserX/CashPilot
Length of output: 10839
api_worker_command still skips health/lifecycle bookkeeping. The UI uses /api/services/*, so this only affects direct callers of /api/workers/{id}/command; if it should behave like the other management routes, mirror their record_health_event and metrics.record_container_lifecycle calls, or document it as a raw proxy.
🤖 Prompt for 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.
In `@app/main.py` around lines 1746 - 1758, api_worker_command lacks the health
and container lifecycle bookkeeping performed by other management routes. In
api_worker_command, mirror the corresponding record_health_event and
metrics.record_container_lifecycle calls used by /api/services handlers,
covering the proxied command’s success and failure/lifecycle outcomes;
alternatively, explicitly document this endpoint as a raw proxy if that behavior
is intentional.
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.
Fixes from a full-codebase 8-lens
/review-code!audit (architecture, security, complexity, dead-code, error-handling, test-health, consistency, performance). Overall health 7/10 — strong bones (clean collector ABC, uniform DB layer, Fernet+bcrypt, DNS-rebinding-aware SSRF guard, parameterized SQL, CodeQL+Dependabot, 88% coverage); the risk concentrated in the UI↔worker Docker channel + a few real correctness bugs. This closes the high-value, safe findings; the large refactors are deferred (below).Security
/var/run/docker.sockbut not the parent/var/run→ mount it → socket → host root) and never validatednetwork_mode. Now:realpath-normalized checks that block/var/run+/run+all sensitive roots, and anetwork_modeallowlist (hostkept for mysterium,container:blocked). Verified against the catalog (only mysterium needshost; no service mounts a host path)./api/deploy/{slug}is owner-only but the same deploy via/api/workers/{id}/commandwas writer-only. Now owner-gated everywhere (+ atest_command_deploy_writer_deniedregression test).main.pys 9-key secret list vsdatabase.SECRET_CONFIG_KEYSs 11 leftremember_web/xsrf_tokenshown unmasked by/api/collectors/meta; now single-sourced from the DB list.sanitizeHintblocksjavascript:hrefs.Correctness (event-loop stalls)
_validate_worker_urldid a blockingsocket.getaddrinfoin async context — one slow-DNS worker stalled the entire UI event loop. Now threaded viaasyncio.to_threadat the sole call site (validator kept sync → no test churn).docker_available()unthreaded in 4 spots (blocked when Docker was degraded) →to_thread._spawn);record_collection_startmoved inside thetry; the hourly collectorgatheris bounded by a semaphore.Consistency / error-surfacing
rates_stale()is wired intoget_all()so the UI can flag stale FX.$0.00/ eternal spinners / a false-healthy bell on fetch errors — surfaces the error, keeps last values; removes dead code (_breakdownCache, the********sentinel); consistent slug escaping.Verification
ruffclean; fullpytest998 passed (updated the route-side-effect + deploy-gate tests to the new secure behavior, added the deploy-writer-denied test + a_login_attemptsautouse reset fixture that removes a latent order-dependent flake).Deferred — with rationale (need their own PR / your call)
main.pygod-module extraction (fleet-proxy / dashboard / earnings-view) + build-enforcing the UI/worker Docker boundary + a sharedcontracts.py— structural refactors.orchestrator.py/worker_api.pytest coverage (38%/44% — the Docker-socket code) + true end-to-end (real-DB) tests — a focused test effort.health_eventsgrowth — needs an uptime-aware redesign (a naive dedup would break the uptime% metric), not a simple change.Summary by CodeRabbit
New Features
Security
Improvements
/metricsis unauthenticated.