Skip to content

Commit 8595a89

Browse files
authored
perf+security+test: deferred audit follow-ups (health growth, first-run token, orchestrator coverage) (#100)
* perf(db): bound health_events growth — split retention + weekly VACUUM 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. * test: cover orchestrator/worker_api Docker paths; expect db_vacuum job 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). * feat(security): setup-token gate for first-run + trusted-proxy client IP 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. * fix(db): restore health-retention split + VACUUM lost in rebase; cover _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. * fix(security): don't accept first-run setup token via query string (CodeRabbit) 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.
1 parent 3e10aa4 commit 8595a89

12 files changed

Lines changed: 1118 additions & 17 deletions

app/database.py

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1073,13 +1073,27 @@ async def get_health_scores(days: int = 7) -> list[dict[str, Any]]:
10731073
# --- Data Retention ---
10741074

10751075
RETENTION_DAYS = 400
1076+
# High-frequency uptime samples (check_ok / check_down, one per service every 5
1077+
# minutes) are the dominant source of health_events growth, yet get_health_scores
1078+
# only ever reads a bounded window. /api/health/scores caps that window at 90 days,
1079+
# so we keep samples just past that (95d) — enough that no allowed query can
1080+
# out-range its own samples, while still cutting the bulk sample rows ~76% versus
1081+
# the 400-day lifecycle-event history (start/stop/restart/crash), which we keep in
1082+
# full because those rows are rare and worth the long tail.
1083+
HEALTH_CHECK_RETENTION_DAYS = 95
1084+
_HEALTH_CHECK_EVENTS = ("check_ok", "check_down")
10761085

10771086

10781087
async def purge_old_data() -> int:
1079-
"""Delete earnings and health_events older than RETENTION_DAYS. Returns rows deleted."""
1088+
"""Delete data past retention. Returns rows deleted.
1089+
1090+
Earnings and lifecycle health events are kept RETENTION_DAYS; the far more
1091+
numerous uptime-sample events are trimmed to HEALTH_CHECK_RETENTION_DAYS.
1092+
"""
10801093
db = await _get_db()
10811094
try:
10821095
cutoff = f"-{RETENTION_DAYS} days"
1096+
check_cutoff = f"-{HEALTH_CHECK_RETENTION_DAYS} days"
10831097
c1 = await db.execute(
10841098
"DELETE FROM earnings WHERE created_at < datetime('now', ?)",
10851099
(cutoff,),
@@ -1088,7 +1102,29 @@ async def purge_old_data() -> int:
10881102
"DELETE FROM health_events WHERE created_at < datetime('now', ?)",
10891103
(cutoff,),
10901104
)
1105+
c3 = await db.execute(
1106+
"DELETE FROM health_events WHERE event IN ('check_ok', 'check_down') AND created_at < datetime('now', ?)",
1107+
(check_cutoff,),
1108+
)
1109+
await db.commit()
1110+
return (c1.rowcount or 0) + (c2.rowcount or 0) + (c3.rowcount or 0)
1111+
finally:
1112+
await db.close()
1113+
1114+
1115+
async def vacuum_database() -> None:
1116+
"""Reclaim free pages left by retention deletes.
1117+
1118+
SQLite never shrinks the file on DELETE alone, so without a periodic VACUUM the
1119+
database keeps its high-water-mark size forever even as old rows are purged.
1120+
Run off-peak (weekly) — VACUUM rewrites the whole file and briefly locks it. We
1121+
commit first because VACUUM cannot run inside an open transaction, and checkpoint
1122+
the WAL afterwards so the freed space is actually returned to the filesystem.
1123+
"""
1124+
db = await _get_db()
1125+
try:
10911126
await db.commit()
1092-
return (c1.rowcount or 0) + (c2.rowcount or 0)
1127+
await db.execute("VACUUM")
1128+
await db.execute("PRAGMA wal_checkpoint(TRUNCATE)")
10931129
finally:
10941130
await db.close()

app/deps.py

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,14 @@
1111
from __future__ import annotations
1212

1313
import ipaddress
14+
import os
1415
from typing import Any
1516

1617
from fastapi import HTTPException, Request
1718
from fastapi.responses import RedirectResponse
1819
from fastapi.templating import Jinja2Templates
1920

20-
from app import auth
21+
from app import auth, setup_token
2122

2223
__all__ = [
2324
"templates",
@@ -26,8 +27,15 @@
2627
"_require_writer",
2728
"_require_owner",
2829
"_require_private_network",
30+
"_require_first_run_access",
31+
"client_ip",
2932
]
3033

34+
# Opt-in: set CASHPILOT_TRUSTED_PROXY=1 only when the app sits behind exactly one
35+
# reverse proxy you control. X-Forwarded-For is attacker-controlled, so we ignore
36+
# it unless the operator asserts a trusted proxy is stripping/appending it.
37+
_TRUST_PROXY = os.getenv("CASHPILOT_TRUSTED_PROXY", "").strip().lower() in ("1", "true", "yes", "on")
38+
3139
templates = Jinja2Templates(directory="app/templates")
3240

3341

@@ -57,13 +65,51 @@ def _require_owner(request: Request) -> dict[str, Any]:
5765
return user
5866

5967

68+
def client_ip(request: Request) -> str | None:
69+
"""Best-effort real client IP.
70+
71+
Behind a trusted reverse proxy (opt-in via ``CASHPILOT_TRUSTED_PROXY``) the
72+
real peer is the right-most ``X-Forwarded-For`` entry — the value appended by
73+
the trusted proxy, which a client cannot forge by prepending its own. Without
74+
that opt-in we never trust the header and use the direct peer.
75+
"""
76+
if _TRUST_PROXY:
77+
xff = request.headers.get("x-forwarded-for", "")
78+
parts = [p.strip() for p in xff.split(",") if p.strip()]
79+
if parts:
80+
return parts[-1]
81+
return request.client.host if request.client else None
82+
83+
6084
def _require_private_network(request: Request) -> None:
61-
"""Block requests from public IPs (for first-run setup)."""
62-
if not request.client or not request.client.host:
85+
"""Block requests whose real client IP is public (first-run defense in depth)."""
86+
ip_str = client_ip(request)
87+
if not ip_str:
6388
return
6489
try:
65-
client_ip = ipaddress.ip_address(request.client.host)
90+
ip = ipaddress.ip_address(ip_str)
6691
except ValueError:
6792
return
68-
if not (client_ip.is_loopback or client_ip.is_private):
93+
if not (ip.is_loopback or ip.is_private):
6994
raise HTTPException(status_code=403, detail="First-run setup only allowed from private networks")
95+
96+
97+
def _require_first_run_access(request: Request, setup_token_value: str | None = None) -> None:
98+
"""Gate first-run owner creation: private network AND the one-time setup token.
99+
100+
The network check alone is spoofable behind a reverse proxy (the peer is then
101+
the proxy), so the setup token — printed to the server logs, readable only
102+
with host access — is the real gate. The token is accepted from the explicit
103+
argument (the registration form field) or the ``X-Setup-Token`` header.
104+
105+
Deliberately NOT read from the query string: a ``?setup_token=`` URL leaks the
106+
secret into reverse-proxy access logs and browser history. The form field
107+
(typed into the setup page) and the header keep it out of URLs.
108+
"""
109+
_require_private_network(request)
110+
token = setup_token_value or request.headers.get("x-setup-token")
111+
if not setup_token.verify(token):
112+
raise HTTPException(
113+
status_code=403,
114+
detail="First-run setup requires the setup token printed in the server logs",
115+
)

app/main.py

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
from pydantic import BaseModel
3232
from starlette.middleware.base import BaseHTTPMiddleware
3333

34-
from app import auth, catalog, compose_generator, database, exchange_rates, fleet_key, metrics
34+
from app import auth, catalog, compose_generator, database, exchange_rates, fleet_key, metrics, setup_token
3535

3636
logging.basicConfig(
3737
level=logging.INFO,
@@ -75,16 +75,16 @@ def _on_done(t: asyncio.Task) -> None:
7575
_LOGIN_WINDOW_SECONDS = 300
7676

7777

78-
def _check_login_rate(client_ip: str) -> None:
78+
def _check_login_rate(ip: str) -> None:
7979
now = monotonic()
80-
attempts = _login_attempts[client_ip]
81-
_login_attempts[client_ip] = [t for t in attempts if now - t < _LOGIN_WINDOW_SECONDS]
82-
if len(_login_attempts[client_ip]) >= _LOGIN_MAX_ATTEMPTS:
80+
attempts = _login_attempts[ip]
81+
_login_attempts[ip] = [t for t in attempts if now - t < _LOGIN_WINDOW_SECONDS]
82+
if len(_login_attempts[ip]) >= _LOGIN_MAX_ATTEMPTS:
8383
raise HTTPException(status_code=429, detail="Too many login attempts. Try again in a few minutes.")
8484

8585

86-
def _record_failed_login(client_ip: str) -> None:
87-
_login_attempts[client_ip].append(monotonic())
86+
def _record_failed_login(ip: str) -> None:
87+
_login_attempts[ip].append(monotonic())
8888

8989

9090
def _safe_json(raw: str, fallback: Any = None) -> Any:
@@ -273,6 +273,15 @@ async def _run_data_retention() -> None:
273273
logger.warning("Data retention error: %s", exc)
274274

275275

276+
async def _run_vacuum() -> None:
277+
"""Reclaim disk left by retention deletes (SQLite does not auto-shrink)."""
278+
try:
279+
await database.vacuum_database()
280+
logger.info("Database VACUUM complete")
281+
except Exception as exc:
282+
logger.warning("Database VACUUM error: %s", exc)
283+
284+
276285
async def _check_stale_workers() -> None:
277286
"""Mark workers as offline if stale, and purge workers offline > 1 hour."""
278287
try:
@@ -312,6 +321,22 @@ async def lifespan(app: FastAPI):
312321
_changed = _u.get("password_changed_at") or 0.0
313322
if _changed:
314323
auth.set_user_pwd_epoch(_u["id"], _changed)
324+
# First-run setup token: while no users exist, require a one-time token
325+
# (printed below) for /register so a proxy-exposed instance cannot be seized
326+
# by the first public visitor. Persisted in config so it survives restarts;
327+
# cleared once the owner account is created.
328+
if not await database.has_any_users():
329+
_tok = await database.get_config("_setup_token")
330+
if not _tok:
331+
_tok = setup_token.generate()
332+
await database.set_config("_setup_token", _tok)
333+
setup_token.set_active(_tok)
334+
logger.warning(
335+
"FIRST-RUN SETUP: no account exists yet. Open /register and enter this "
336+
"one-time setup token to create the owner account: %s (shown only here; "
337+
"not embedded in any URL so it stays out of proxy logs and browser history)",
338+
_tok,
339+
)
315340
catalog.load_services()
316341
catalog.register_sighup()
317342

@@ -355,6 +380,15 @@ def _on_job_event(event):
355380
coalesce=True,
356381
misfire_grace_time=300,
357382
)
383+
scheduler.add_job(
384+
_run_vacuum,
385+
"interval",
386+
weeks=1,
387+
id="db_vacuum",
388+
max_instances=1,
389+
coalesce=True,
390+
misfire_grace_time=300,
391+
)
358392
scheduler.add_job(
359393
exchange_rates.refresh,
360394
"interval",
@@ -424,9 +458,11 @@ async def dispatch(self, request, call_next):
424458
from app.deps import ( # noqa: E402
425459
_login_redirect, # noqa: F401 (re-exported for app.main.* test/router surface)
426460
_require_auth_api,
461+
_require_first_run_access, # noqa: F401 (re-exported for app.main.* router surface)
427462
_require_owner,
428463
_require_private_network, # noqa: F401 (re-exported for app.main.* router surface)
429464
_require_writer,
465+
client_ip, # noqa: F401 (re-exported for app.main.* router surface)
430466
templates, # noqa: F401 (re-exported for app.main.* router/test surface)
431467
)
432468

app/routers/auth.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ async def do_login(
4545
username: str = Form(...),
4646
password: str = Form(...),
4747
):
48-
client_ip = request.client.host if request.client else "unknown"
48+
client_ip = main.client_ip(request) or "unknown"
4949
try:
5050
main._check_login_rate(client_ip)
5151
except HTTPException:
@@ -85,6 +85,9 @@ async def page_register(request: Request, error: str = ""):
8585
user = main.auth.get_current_user(request)
8686
if not user or user.get("r") != "owner":
8787
return RedirectResponse("/login", status_code=303)
88+
# The GET page is gated only by the network check so the operator can reach the
89+
# form; the setup token is entered into a form field and verified on POST (never
90+
# via URL, which would leak it into access logs / browser history).
8891
if is_first:
8992
main._require_private_network(request)
9093

@@ -109,6 +112,7 @@ async def do_register(
109112
username: str = Form(...),
110113
password: str = Form(...),
111114
password_confirm: str = Form(...),
115+
setup_token: str = Form(""),
112116
):
113117
is_first = not await main.database.has_any_users()
114118

@@ -119,7 +123,7 @@ async def do_register(
119123
raise HTTPException(status_code=403, detail="Only owners can add users")
120124

121125
if is_first:
122-
main._require_private_network(request)
126+
main._require_first_run_access(request, setup_token)
123127

124128
if not re.match(r"^[a-zA-Z0-9_-]{3,32}$", username):
125129
return main.templates.TemplateResponse(
@@ -133,6 +137,7 @@ async def do_register(
133137
"button_text": "Create Account",
134138
"error": "Username must be 3-32 alphanumeric characters (a-z, 0-9, _ -)",
135139
"is_first": is_first,
140+
"setup_token": setup_token if is_first else "",
136141
},
137142
status_code=400,
138143
)
@@ -149,6 +154,7 @@ async def do_register(
149154
"button_text": "Create Account",
150155
"error": "Passwords do not match",
151156
"is_first": is_first,
157+
"setup_token": setup_token if is_first else "",
152158
},
153159
status_code=400,
154160
)
@@ -165,6 +171,7 @@ async def do_register(
165171
"button_text": "Create Account",
166172
"error": "Password must be at least 10 characters",
167173
"is_first": is_first,
174+
"setup_token": setup_token if is_first else "",
168175
},
169176
status_code=400,
170177
)
@@ -182,6 +189,7 @@ async def do_register(
182189
"button_text": "Create Account",
183190
"error": "Username already taken",
184191
"is_first": is_first,
192+
"setup_token": setup_token if is_first else "",
185193
},
186194
status_code=400,
187195
)
@@ -191,6 +199,11 @@ async def do_register(
191199
hashed = main.auth.hash_password(password)
192200
user_id = await main.database.create_user(username, hashed, role)
193201

202+
if is_first:
203+
# Owner now exists — retire the one-time setup token permanently.
204+
await main.database.delete_config_keys(["_setup_token"])
205+
main.setup_token.clear()
206+
194207
token = main.auth.create_session_token(user_id, username, role)
195208
dest = "/setup" if is_first else "/"
196209
response = RedirectResponse(dest, status_code=303)

app/setup_token.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
"""First-run setup-token gate.
2+
3+
The very first account created on a fresh install becomes the ``owner``. The
4+
private-network check in :mod:`app.deps` is not enough on its own: behind a
5+
reverse proxy ``request.client`` is the *proxy* (a loopback/private address), so
6+
the network check always passes and the first person to reach ``/register`` from
7+
the public internet could seize the owner account.
8+
9+
This module adds a proxy-independent second factor: a one-time token generated at
10+
startup while no users exist, printed to the container logs. Only someone who can
11+
read the server logs (i.e. already has host access) can complete first-run setup.
12+
Once the owner account is created the token is cleared and never required again
13+
(further users are added by an authenticated owner).
14+
15+
The active token is held in a module global so the synchronous request guard can
16+
check it without a DB round-trip; it is also persisted in the ``config`` table so
17+
it survives restarts until consumed.
18+
"""
19+
20+
from __future__ import annotations
21+
22+
import hmac
23+
import secrets
24+
25+
# The token currently required for first-run registration, or ``None`` when no
26+
# first-run gate is active (owner already exists, or not yet initialised).
27+
_active: str | None = None
28+
29+
30+
def generate() -> str:
31+
"""Return a fresh, URL-safe setup token."""
32+
return secrets.token_urlsafe(24)
33+
34+
35+
def set_active(token: str | None) -> None:
36+
"""Install (or clear, with ``None``) the token required for first-run setup."""
37+
global _active
38+
_active = token or None
39+
40+
41+
def clear() -> None:
42+
"""Drop the first-run gate — called once the owner account exists."""
43+
set_active(None)
44+
45+
46+
def active() -> str | None:
47+
"""Return the currently required setup token, or ``None`` if none is active."""
48+
return _active
49+
50+
51+
def verify(provided: str | None) -> bool:
52+
"""Check a caller-supplied token against the active one.
53+
54+
Returns ``True`` when no token is active (nothing to enforce) or when the
55+
supplied value matches in constant time; ``False`` otherwise.
56+
"""
57+
current = _active
58+
if current is None:
59+
return True
60+
if not provided:
61+
return False
62+
return hmac.compare_digest(current, provided)

0 commit comments

Comments
 (0)