WealthySmart/codebase-review/04-security.md

# Security Review — WealthySmart

> Hat: Senior Application Security Engineer. Defensive review of the owner's own app (wealth.cescalante.dev) for hardening. All findings verified against actual code; key claims re-checked by the synthesizer (see README corrections).

## Executive Summary

The most important finding is configuration, not code: `backend/app/config.py` ships **working fallback credentials** (`admin`/`admin`, a known `SECRET_KEY`) that take effect silently if a deploy ever runs without its env file — and the deploy pipeline regenerates `.env.prod` on every deploy, making that scenario plausible. Beyond that: login endpoints use non-constant-time `!=` comparison with **no rate limiting**, CORS is `allow_origins=["*"]` with credentials enabled, JWTs live for 30 days, and PDF upload endpoints accept files of any size and type. On the positive side, the app uses parameterized queries throughout, safe subprocess invocation (no `shell=True`, controlled args, timeouts), hashed API tokens with expiry checks, and httpOnly + SameSite=Lax cookies. All critical fixes are low-effort.

**Note (good news, verified):** `.env` and `.env.prod` are properly gitignored and have never been committed — no secret exposure via the repository.

## Threat Model

**Assets:** personal financial data (accounts, transactions, balances, pensions, water usage), admin credentials, JWT signing key, n8n API tokens, OpenAI API key, VAPID keys.

**Actors:**
- Internet attacker reaching wealth.cescalante.dev (it is publicly exposed)
- Malicious website visited by the logged-in user (CORS/CSRF surface)
- Credential brute-forcer (no rate limit, fixed username)
- Holder of a stolen token (30-day validity window)
- **Indirect prompt injector** — transaction descriptions originate from emails parsed by n8n; an attacker who controls a merchant name or email content gets text into the LLM agent's context

**Entry points:** `/api/auth/login` (×2 implementations), `/api/v1/pensions/upload`, `/api/v1/municipal-receipts/upload`, `/api/copilotkit/*` (Hono), `/api/v1/agent/agui`, n8n token-authenticated POSTs.

## Findings

### SEC-01: Working Fallback Credentials in Default Configuration — Critical
**Location:** `backend/app/config.py:6, 9, 10` (verified)
**Evidence:**
```python
class Settings(BaseSettings):
    DATABASE_URL: str = "postgresql://wealthy_user:wealthy_pass@db:5432/wealthysmart"
    SECRET_KEY: str = "change-me-in-production"
    ADMIN_USERNAME: str = "admin"
    ADMIN_PASSWORD: str = "admin"
```
**Exploit scenario:** Deploy pipeline regenerates `.env.prod` from Gitea secrets each deploy (see `reference_gitea_deploy`). A pipeline regression or missing secret silently boots the app accepting `admin:admin`, and the known `SECRET_KEY` lets anyone forge valid JWTs even *without* logging in.
**Remediation:** Remove defaults; validate at startup:
```python
SECRET_KEY: str  # no default
@model_validator(mode="after")
def _no_weak_defaults(self):
    if self.SECRET_KEY in ("", "change-me-in-production"):
        raise ValueError("SECRET_KEY must be set")
    if self.ADMIN_PASSWORD in ("", "admin"):
        raise ValueError("ADMIN_PASSWORD must be set")
    return self
```
A failed boot is loud (healthcheck fails, deploy aborts) — exactly what you want.

### SEC-02: Non-Constant-Time Credential Comparison — High
**Location:** `backend/app/main.py:165–169`, `backend/app/api/v1/endpoints/auth.py:12–19`
**Evidence:** Both login implementations:
```python
if (body.username != settings.ADMIN_USERNAME
        or body.password != settings.ADMIN_PASSWORD):
    raise HTTPException(status_code=401, ...)
```
**Exploit scenario:** Short-circuit `or` plus string `!=` leaks timing information distinguishing username-fail from password-fail and partially leaking prefix matches under many measurements. Marginal for a remote attacker, but the fix is one line and the password is also stored in plaintext config.
**Remediation:** `hmac.compare_digest()` on both fields, both endpoints. Better: store a bcrypt/argon2 hash of the admin password (`passlib`) instead of the plaintext in env.

### SEC-03: No Rate Limiting on Login — High
**Location:** Both login endpoints (`main.py:163–179`, `endpoints/auth.py:10–21`)
**Evidence:** No limiter, lockout, or backoff anywhere in the codebase.
**Exploit scenario:** Username is effectively fixed ("admin"); an attacker brute-forces the password at full line speed against a public endpoint.
**Remediation:** `slowapi` with `5/minute` per IP on both login routes, or a simple in-memory failure counter with exponential backoff (single-process app, in-memory is fine). Also consider consolidating to **one** login endpoint — two implementations means two places to harden forever.

### SEC-04: CORS `["*"]` with Credentials Enabled — Medium
**Location:** `backend/app/main.py:79–85` (verified line 81)
**Evidence:**
```python
app.add_middleware(CORSMiddleware,
    allow_origins=["*"], allow_credentials=True,
    allow_methods=["*"], allow_headers=["*"])
```
**Exploit scenario:** Starlette reflects the request Origin when `allow_credentials=True`, so any website gets credentialed CORS approval. Mitigating factor: the `ws_token` cookie is `SameSite=Lax`, so browsers won't attach it to cross-site XHR/fetch — which currently blunts this from Critical to Medium. But the protection is one cookie-attribute change away from vanishing, and `Authorization`-header flows aren't SameSite-protected.
**Remediation:** `allow_origins=["https://wealth.cescalante.dev", "http://localhost:3000"]`, explicit methods and headers.

### SEC-05: 30-Day JWT Expiry — Medium
**Location:** `backend/app/config.py:7` (verified), `backend/app/auth.py:18–20`
**Evidence:** `ACCESS_TOKEN_EXPIRE_MINUTES: int = 60 * 24 * 30  # 30 days`
**Exploit scenario:** A stolen cookie/token works for a month; there is no revocation mechanism for JWTs (only API tokens are revocable).
**Remediation:** For a single-user personal app, 24h–7d is a reasonable convenience/security balance (8h would force daily logins). Pick e.g. 7 days, or implement short access + refresh tokens if login friction matters.

### SEC-06: No Upload Size/Type Validation — Medium
**Location:** `backend/app/api/v1/endpoints/pensions.py:104–162`, `municipal_receipts.py:197–228`
**Evidence:** `files: list[UploadFile]` then `await file.read()` — unbounded read, no content-type or magic-byte check (endpoints *are* authenticated).
**Exploit scenario:** Memory-exhaustion DoS via huge upload (requires auth — the n8n token or the user's own session — so primarily a robustness issue); junk files reach `pdftotext`.
**Remediation:** 10 MB cap via bounded read, require `pdf_bytes.startswith(b"%PDF")`, record per-file rejections in the existing `errors[]` response field.

### SEC-07: Missing Security Headers — Low
**Location:** `backend/app/main.py` (absent middleware); more importantly `frontend/server.ts` since it serves the HTML
**Evidence:** No CSP, `X-Content-Type-Options`, `X-Frame-Options`, or HSTS set by either layer.
**Remediation:** Set headers at the Hono layer (the user-facing one): `X-Frame-Options: DENY`, `X-Content-Type-Options: nosniff`, `Referrer-Policy: strict-origin-when-cross-origin`, and a CSP once inline-script usage is audited. HSTS can be set at nginx-proxy.

### SEC-08: Cookie `secure=False` Relies on a Comment — Low
**Location:** `backend/app/main.py:177`
**Evidence:** `secure=False,  # set True behind TLS in production via nginx` — but nothing actually sets it to True in production; nginx terminating TLS does not add the Secure attribute to an app-issued cookie.
**Exploit scenario:** The cookie would be sent over plain HTTP if the user ever hits an http:// URL for the domain (pre-HSTS first visit, or a downgrade).
**Remediation:** Drive it from config: `secure=settings.COOKIE_SECURE` (True in prod env). Pair with HSTS.

### SEC-09: LLM Agent — Indirect Prompt Injection Surface — Medium (added by synthesis)
**Location:** `backend/app/agent/tools.py` (tool outputs include merchant names, notes), data path: email → n8n → `POST /transactions/` → agent context
**Evidence:** Transaction descriptions originate from external emails. When the assistant lists recent transactions, attacker-influenceable text (a crafted merchant name) enters the model's context. Mitigating factors: all agent tools are **read-only queries** (verified — no tool mutates data), and the agent endpoint requires authentication, so impact is bounded to misleading the user, not data modification or exfiltration to third parties.
**Remediation:** Keep tools read-only as a hard rule (document it); if write-tools are ever added, require explicit user confirmation in the UI (`renderAndWaitForResponse`) for every mutation. Consider delimiting untrusted strings in tool outputs.

### SEC-10: Positive Findings (No Issue) — Informational
- **API token handling is correct** (`auth.py:42–54`): SHA-256 hash lookup, `is_active` flag, expiry checked against now. Tokens are revocable.
- **Subprocess usage is safe** (`pension_pdf.py:46–53`, `municipal_receipt_pdf.py:79–86`): list-args, no `shell=True`, timeouts, Python-generated temp filenames — no command-injection path.
- **No raw SQL string interpolation found** — ORM/parameterized throughout (the ad-hoc migrations in `db.py` are static strings).
- **httpOnly + SameSite=Lax cookie** — correct baseline against XSS token theft and CSRF.
- **`.env` files properly gitignored, never committed** (verified via `git ls-files` and history).

## Hardening Checklist

**Priority 1 — do before anything else (≈half a day total):**
- [ ] SEC-01: remove config defaults; fail-fast startup validation
- [ ] SEC-02: `hmac.compare_digest` (or hashed admin password) in both login endpoints
- [ ] SEC-03: rate-limit login (5/min/IP); consolidate the duplicate login endpoints
- [ ] SEC-04: pin CORS origins

**Priority 2 — same week:**
- [ ] SEC-05: reduce token expiry (7 days suggested)
- [ ] SEC-06: upload size cap + `%PDF` magic check
- [ ] SEC-07: security headers at the Hono layer; HSTS at nginx
- [ ] SEC-08: `secure=True` cookie in production via config

**Priority 3 — ongoing posture:**
- [ ] Keep agent tools read-only by policy; confirm-before-write if that ever changes (SEC-09)
- [ ] Audit logging for login attempts and token creation
- [ ] Scope n8n API tokens to the endpoints they need (currently any valid token hits any endpoint)
- [ ] Database backup with encryption; test restore
- [ ] Dependency update cadence (Dependabot/Renovate on `requirements.txt` and `package.json`)