Skip to content

Commit 339576f

Browse files
fix: security & auth hardening — 11 bug-hunt findings (batch:p3-security-auth) (#464)
* fix(auth): decouple reset-request timing from account existence (discogsography-0lof) Defer the reset-token mint, Redis setex, and outbound notification-send to a FastAPI background task scheduled unconditionally on both branches, so the HTTP response for /api/auth/reset-request returns after nothing but the initial SELECT regardless of whether the account exists. Previously the Redis write + Resend HTTP round-trip only fired on the existing-account branch, giving a reliable timing oracle that defeated the endpoint's declared anti-enumeration contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(auth): stop logging registrant/admin email addresses at INFO (discogsography-1385) Fix-one-fix-all sweep of every logger.*(... email=...) structlog binding in api/routers/auth.py, api/routers/admin.py, and api/notifications.py: registration and login success logs now bind user_id instead of the raw email; admin login logs admin user_id; DLQ-purge audit log binds admin_id instead of admin_email; the LogNotificationChannel fallback and ResendNotificationChannel drop the email argument from all log calls entirely (no user_id available in that scope). No PII-bearing structlog event remains in these files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(api): app-token defense-in-depth compare against the real stored hash (discogsography-osoc) _lookup_active_token now projects the row's own persisted token_hash (and joins users to filter is_active — laying groundwork for discogsography-ci4a) instead of only id/user_id/name/scope. Both app-token entry points — app_tokens.require_app_token and dependencies.require_user_or_app_token — now compare_digest the row's stored token_hash against the lookup hash, replacing a vacuous hash_token(plaintext) == hash_token(plaintext) self-comparison that could never fail. dependencies.require_user_or_app_token previously had no such check at all; the two app-token paths were diverging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(auth): bulk-revoke app tokens on password reset/change (discogsography-ci4a) reset_confirm and change_password now also execute UPDATE app_tokens SET revoked_at = NOW() WHERE user_id = ... AND revoked_at IS NULL in the same round trip as the password update. The password_changed:{user_id} Redis marker only gates JWT validation and is sized to jwt_expire_minutes, so it was structurally incapable of revoking app tokens (which carry no expiry by design) — an attacker who minted a dscg_ app token from a stolen access JWT kept read access to the victim's collection data indefinitely after the victim 'remediated' by resetting their password. Bulk-revoking the rows themselves closes the gap permanently, unlike a Redis-key check that would itself expire. Builds on discogsography-osoc's _lookup_active_token(is_active join), which already closes the sibling 'deactivated user keeps an authenticating app token' gap the same finding raised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(api): stop accepting the admin-setup password via argv (discogsography-dir0) Remove --password entirely from the admin-setup CLI. Command-line arguments are world-readable via /proc/*/cmdline for the life of the process (ps aux, docker container top) and land verbatim in shell history — an unconditional leak of the highest-privilege credential in the system on every invocation, with no non-leaking path previously available. The password now resolves via ADMIN_PASSWORD / ADMIN_PASSWORD_FILE (the repo's existing get_secret Docker-secrets convention, already used one line above for POSTGRES_PASSWORD in the same file) for scripted use, falling back to an interactive getpass.getpass() prompt (never echoed, never in argv or history). Updated docs/admin-guide.md and docs/monitoring.md to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(api): key request metrics on the matched route template (discogsography-jlei) metrics_middleware now reads request.scope['route'] AFTER call_next and records normalize_path(route.path) — the route's template (e.g. /api/artists/{id}) — instead of the raw, attacker-controlled URL. Requests that never matched a route collapse into a single '<unmatched>' bucket. Previously an unauthenticated flood of distinct nonexistent paths (each one individually 404ing) had unbounded cardinality against the 10k-entry MetricsBuffer, evicting every real endpoint's latency/error samples in a single burst and bloating the persisted endpoint_stats JSONB with bogus keys. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(auth): make the 2FA verify lockout gate atomic with the increment (discogsography-vjod) twofa_verify's lockout check now runs inside an explicit transaction that takes a row lock (SELECT ... FOR UPDATE) on the user row, held across both the lockout read and the failed-attempt increment. Concurrent verify requests for the same account now serialize on that lock instead of all reading a stale pre-increment snapshot — previously N requests whose SELECT landed before the first lock UPDATE committed each got a free TOTP guess, letting a distributed burst exceed the intended 5-attempt-per-window cap. Branch ordering (not-configured -> locked -> encryption-not-configured -> invalid-code) is preserved exactly; only the lockout gate's concurrency semantics change. tests/api/conftest.py: mock_conn now stubs set_autocommit and a no-op transaction() async context manager so mocked tests can exercise code that opens an explicit transaction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(auth): reset TOTP lockout state on successful 2FA recovery (discogsography-cflq) twofa_recovery's code-redemption UPDATE now also sets totp_failed_attempts = 0, totp_locked_until = NULL, mirroring the reset twofa_verify's success path already performs. Recovery (password + one-time recovery code) is an equally strong proof of account control as a correct TOTP code, but its success path previously left stale lockout state untouched — a user who fat-fingered TOTP into a 15-minute lock, then recovered successfully via a recovery code, would still get 429 'Account temporarily locked' on their very next CORRECT TOTP login for the remainder of that window, burning another scarce recovery code to work around it. Folded into the existing guarded UPDATE so it only fires when the code actually matched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(auth): check the atomic getdel result in 2FA recovery (discogsography-kqw4) twofa_recovery now captures the return value of the challenge-consuming getdel and raises 401 'Challenge expired or already used' when it's falsy — mirroring twofa_verify's identical, already-tested check. Previously the result was discarded, so two concurrent requests carrying the SAME challenge token but two DIFFERENT recovery codes could both redeem a code and both mint an access token, silently voiding the challenge's one-time contract that twofa_verify enforces. Ordering is unchanged (challenge consumed only AFTER the recovery code is redeemed), so a mistyped code still never burns the challenge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(auth): guard the 2FA-confirm enable UPDATE on the verified secret (discogsography-8vlp) twofa_confirm's enable UPDATE now binds AND totp_secret = %s using the exact encrypted secret read (and whose code was verified) earlier in the handler, and treats rowcount 0 as 409 'setup state changed — restart 2FA setup'. Previously the UPDATE was an unconditional blind write: if a concurrent twofa_disable committed between confirm's SELECT and this UPDATE, the row would land as totp_enabled=TRUE with totp_secret/totp_recovery_codes NULLed — login demands 2FA but neither twofa_verify nor twofa_recovery can ever satisfy it, a permanent lockout with no self-service recovery path. Mirrors the guarded-UPDATE treatment twofa_setup already has. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU * fix(api): self-heal snapshot quota counter TTL and refund on failed save (discogsography-7639) SnapshotStore.save now calls EXPIRE ... NX on every save (not just when count == 1), so a counter left TTL-less by a prior lost EXPIRE call (crash/ timeout right after INCR) self-heals on the next save instead of becoming permanent — previously the TTL had exactly one arming opportunity per key generation, silently converting a 28-day sliding window into an unbounded lifetime counter and eventually locking the user out of snapshots forever. The final Redis set() is now wrapped so a failure decrements the just-reserved quota slot before re-raising, mirroring the existing quota-exceeded decrement path — a transient Redis/network failure on that call no longer permanently burns quota with zero live snapshots to show for it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BnH7JWqGK1b8EuYRxYJtU --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 8871b9b commit 339576f

20 files changed

Lines changed: 1006 additions & 139 deletions

api/admin_setup.py

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,21 @@
11
"""CLI tool for managing admin accounts.
22
33
Usage:
4-
admin-setup --email admin@example.com --password mysecretpw
4+
admin-setup --email admin@example.com # prompts for the password
55
admin-setup --list
6+
7+
The password may also come from the ADMIN_PASSWORD (or ADMIN_PASSWORD_FILE)
8+
environment variable for non-interactive/scripted use. A bare --password
9+
CLI argument is intentionally NOT offered: command-line arguments are
10+
world-readable via /proc/*/cmdline for the life of the process and land
11+
verbatim in shell history, leaking the highest-privilege credential in the
12+
system (discogsography-dir0).
613
"""
714

815
from __future__ import annotations
916

1017
import argparse
18+
import getpass
1119
from os import getenv
1220
import sys
1321

@@ -64,32 +72,48 @@ def list_admins(conninfo: str) -> None:
6472
print(f"{email:<40} {status:<8} {created_at}")
6573

6674

75+
def _resolve_password() -> str:
76+
"""Resolve the admin password WITHOUT ever accepting it as a CLI argument.
77+
78+
Command-line arguments are world-readable via /proc/*/cmdline for the life
79+
of the process (`ps aux`, `docker container top`) and land verbatim in
80+
shell history — for the highest-privilege account in the system, that is
81+
an unconditional credential leak on every invocation (discogsography-dir0).
82+
ADMIN_PASSWORD / ADMIN_PASSWORD_FILE (via the repo's standard `get_secret`
83+
Docker-secrets convention) covers scripted/non-interactive use; an
84+
interactive getpass prompt (never echoed, never in argv or history)
85+
covers everything else.
86+
"""
87+
return get_secret("ADMIN_PASSWORD") or getpass.getpass("Admin password (min 8 chars): ")
88+
89+
6790
def main() -> None:
6891
"""Entry point for the admin-setup CLI tool."""
6992
parser = argparse.ArgumentParser(
7093
prog="admin-setup",
7194
description="Manage admin accounts for the dashboard.",
7295
)
7396
parser.add_argument("--email", metavar="EMAIL", help="Admin email address")
74-
parser.add_argument("--password", metavar="PW", help="Admin password (min 8 chars)")
7597
parser.add_argument("--list", action="store_true", help="List existing admin accounts")
7698

7799
args = parser.parse_args()
78100

79-
if not args.list and not (args.email and args.password):
101+
if not args.list and not args.email:
80102
parser.print_help()
81103
sys.exit(1)
82104

83-
if args.password and len(args.password) < 8:
84-
print("❌ Password must be at least 8 characters.")
85-
sys.exit(1)
86-
87105
conninfo = _build_conninfo()
88106

89107
if args.list:
90108
list_admins(conninfo)
91-
else:
92-
add_admin(conninfo, args.email, args.password)
109+
return
110+
111+
password = _resolve_password()
112+
if len(password) < 8:
113+
print("❌ Password must be at least 8 characters.")
114+
sys.exit(1)
115+
116+
add_admin(conninfo, args.email, password)
93117

94118

95119
if __name__ == "__main__":

api/api.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -375,13 +375,25 @@ async def security_headers(request: Request, call_next: Any) -> Any:
375375

376376
@app.middleware("http")
377377
async def metrics_middleware(request: Request, call_next: Any) -> Any:
378-
"""Record per-request timing for endpoint performance metrics."""
378+
"""Record per-request timing for endpoint performance metrics.
379+
380+
Keys on the matched route's path TEMPLATE (e.g. ``/api/artists/{id}``),
381+
not the raw request URL. Requests that never matched a route (unknown
382+
paths — attacker-controlled cardinality, wrong HTTP method) collapse into
383+
a single ``<unmatched>`` bucket instead of each becoming a distinct key.
384+
Previously this keyed on ``normalize_path(request.url.path)``, a regex
385+
reconstruction that only collapses pure-integer/UUID segments — an
386+
unauthenticated flood of distinct junk paths (``/a1``, ``/a2``, ...) could
387+
fill the whole 10k-entry MetricsBuffer and evict every real endpoint's
388+
samples (discogsography-jlei).
389+
"""
379390
import time as _time # noqa: PLC0415
380391

381-
path = normalize_path(request.url.path)
382392
start = _time.monotonic()
383393
response = await call_next(request)
384394
elapsed_ms = (_time.monotonic() - start) * 1000
395+
route = request.scope.get("route")
396+
path = normalize_path(route.path) if route is not None else "<unmatched>"
385397
if hasattr(app.state, "metrics_buffer"):
386398
app.state.metrics_buffer.record(path, response.status_code, elapsed_ms)
387399
return response

api/app_tokens.py

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,15 @@ def _parse_bearer(credentials: HTTPAuthorizationCredentials | None) -> str:
172172

173173

174174
async def _lookup_active_token(token_hash: str) -> dict[str, Any] | None:
175-
"""Fetch the active row for a token_hash, or None if missing/revoked."""
175+
"""Fetch the active row for a token_hash, or None if missing/revoked/for a deactivated user.
176+
177+
Joins `users` so a deactivated account's app tokens stop authenticating —
178+
mirroring the `is_active` check every JWT-based admin/user path already
179+
enforces (discogsography-ci4a). Also projects the row's own `token_hash`
180+
so callers can do a real defense-in-depth comparison against it, instead
181+
of recomputing the same value they already used for the WHERE clause and
182+
comparing it to itself (discogsography-osoc).
183+
"""
176184
if _pool is None:
177185
raise HTTPException(
178186
status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
@@ -181,9 +189,10 @@ async def _lookup_active_token(token_hash: str) -> dict[str, Any] | None:
181189
async with _pool.connection() as conn, conn.cursor(row_factory=dict_row) as cur:
182190
await cur.execute(
183191
"""
184-
SELECT id, user_id, name, scope
185-
FROM app_tokens
186-
WHERE token_hash = %s AND revoked_at IS NULL
192+
SELECT t.id, t.user_id, t.name, t.scope, t.token_hash
193+
FROM app_tokens t
194+
JOIN users u ON u.id = t.user_id
195+
WHERE t.token_hash = %s AND t.revoked_at IS NULL AND u.is_active = TRUE
187196
""",
188197
(token_hash,),
189198
)
@@ -222,11 +231,14 @@ async def dependency(
222231
headers={"WWW-Authenticate": "Bearer"},
223232
)
224233

225-
# Defense in depth: hmac.compare_digest on the canonical hash even though
226-
# the WHERE clause already filtered. Guards against any future fast-path
227-
# that bypasses the index (e.g. column rename, query restructure).
228-
stored_hash = hash_token(plaintext)
229-
if not hmac.compare_digest(stored_hash, token_hash):
234+
# Defense in depth: compare the row's OWN persisted token_hash (now
235+
# projected by _lookup_active_token) against the hash we looked up
236+
# with. Previously this recomputed hash_token(plaintext) on both
237+
# sides of compare_digest — comparing a value to itself, an
238+
# unreachable guard (discogsography-osoc). Comparing against the
239+
# stored column actually catches a future fast-path query that
240+
# bypasses the WHERE clause (e.g. column rename, query restructure).
241+
if not hmac.compare_digest(row["token_hash"], token_hash):
230242
raise HTTPException(
231243
status_code=status.HTTP_401_UNAUTHORIZED,
232244
detail="Invalid app token",

api/dependencies.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import asyncio
44
from dataclasses import dataclass
5+
import hmac
56
from typing import Annotated, Any, Literal
67

78
from fastapi import Depends, HTTPException, status
@@ -132,13 +133,24 @@ async def dependency(
132133
# so the JWT path's 503/401 ordering and revocation checks are preserved
133134
# byte-for-byte for existing clients.
134135
if credentials is not None and credentials.credentials.startswith(_APP_TOKEN_PREFIX):
135-
row = await _lookup_active_token(hash_token(credentials.credentials))
136+
token_hash = hash_token(credentials.credentials)
137+
row = await _lookup_active_token(token_hash)
136138
if row is None:
137139
raise HTTPException(
138140
status_code=status.HTTP_401_UNAUTHORIZED,
139141
detail="Invalid or revoked app token",
140142
headers={"WWW-Authenticate": "Bearer"},
141143
)
144+
# Defense in depth: mirrors require_app_token's check against the
145+
# row's own persisted token_hash, so the two app-token entry
146+
# points stay in lockstep instead of silently diverging
147+
# (discogsography-osoc).
148+
if not hmac.compare_digest(row["token_hash"], token_hash):
149+
raise HTTPException(
150+
status_code=status.HTTP_401_UNAUTHORIZED,
151+
detail="Invalid app token",
152+
headers={"WWW-Authenticate": "Bearer"},
153+
)
142154
granted = list(row.get("scope") or [])
143155
missing = [s for s in required_scopes if s not in granted]
144156
if missing:

api/notifications.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ async def send_password_reset(self, email: str, reset_url: str) -> None:
2424
class LogNotificationChannel:
2525
"""Notification channel that logs messages (development/MVP use)."""
2626

27-
async def send_password_reset(self, email: str, reset_url: str) -> None: # noqa: ARG002 # reset_url unused in log channel
28-
"""Log a password reset link (URL intentionally not logged for security)."""
29-
logger.info("🔑 Password reset link generated", email=email)
27+
async def send_password_reset(self, email: str, reset_url: str) -> None: # noqa: ARG002 # email/reset_url unused in log channel
28+
"""Log a password reset link (email and URL intentionally not logged — PII)."""
29+
logger.info("🔑 Password reset link generated")
3030

3131

3232
class ResendNotificationChannel:
@@ -58,7 +58,7 @@ async def send_password_reset(self, email: str, reset_url: str) -> None:
5858
"</body></html>"
5959
)
6060

61-
logger.debug("🔑 Sending password reset email", email=email)
61+
logger.debug("🔑 Sending password reset email")
6262
try:
6363
async with httpx.AsyncClient(timeout=10.0) as client:
6464
response = await client.post(
@@ -72,6 +72,6 @@ async def send_password_reset(self, email: str, reset_url: str) -> None:
7272
},
7373
)
7474
response.raise_for_status()
75-
logger.info("📧 Password reset email sent", email=email)
75+
logger.info("📧 Password reset email sent")
7676
except Exception:
77-
logger.exception("❌ Failed to send password reset email", email=email)
77+
logger.exception("❌ Failed to send password reset email")

api/routers/admin.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ async def admin_login(request: Request, body: AdminLoginRequest) -> JSONResponse
118118
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required")
119119

120120
access_token, expires_in = create_admin_token(str(admin["id"]), admin["email"], _config.jwt_secret_key)
121-
logger.info("✅ Admin logged in", email=body.email)
121+
logger.info("✅ Admin logged in", user_id=str(admin["id"]))
122122
await record_audit_entry(pool=_pool, admin_id=str(admin["id"]), action="admin.login", target=body.email, details={"success": True})
123123

124124
return JSONResponse(
@@ -593,8 +593,8 @@ async def purge_dlq(
593593
detail="RabbitMQ management API unreachable",
594594
) from exc
595595

596-
admin_email = current_admin.get("email", "unknown")
597-
logger.info("🗑️ DLQ purged", queue=queue, messages_purged=messages_purged, admin_email=admin_email)
596+
admin_id = current_admin.get("sub", "unknown")
597+
logger.info("🗑️ DLQ purged", queue=queue, messages_purged=messages_purged, admin_id=admin_id)
598598
if _pool is not None:
599599
await record_audit_entry(
600600
pool=_pool, admin_id=current_admin["sub"], action="dlq.purge", target=queue, details={"purged_count": messages_purged}

0 commit comments

Comments
 (0)