docs: add automated security review findings

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

SECURITY_REVIEW.md

@@ -1,46 +1,66 @@
 # Security Review — corvid
 
-_Reviewed: 2026-06-02_
+Generated: 2026-07-13 (automated authorized audit)
 
-## Critical
+Supersedes the prior manual review dated 2026-06-02.
 
-| ID | File | Line(s) | Issue |
+## Scope
-|----|------|---------|-------|
-| C1 | `mcp_server.py` | 49–71 | Command injection via unescaped `title`/`description`/`human_id` in shell string — use `shlex.quote()` |
-| C2 | `corvid/router.py` | 162–163 | Full auth bypass when `TICKETS_API_KEY` is empty (the default) |
 
-## High
+All tracked files (`git ls-files`) of corvid — a FastAPI ticketing service with an
+MCP server (`corvid/main.py`, `corvid/router.py`, `corvid/models.py`,
+`corvid/database.py`, `mcp_server.py`), Alembic migrations, Jinja templates, and the
+container/compose definitions.
 
-| ID | File | Line(s) | Issue |
+## Methodology
-|----|------|---------|-------|
-| H1 | `corvid/router.py` | 149–164 | No rate limiting — API key is brute-forceable |
-| H2 | `corvid/main.py`, `corvid/router.py` | — | No CSRF protection (latent for cookie-auth integration) |
-| H3 | `.env.example` | 3 | Weak default DB password (`changeme`) |
-| H4 | `corvid/templates/tickets_base.html` | 795 | Stored XSS via unescaped `human_id` in child-ticket list |
 
-## Medium
+Manual source inspection of the auth layer, all query construction, template rendering,
+and config; plus greps for committed secrets and unsafe sinks. No automated scanners
+were available.
 
-| ID | File | Line(s) | Issue |
+## Summary
-|----|------|---------|-------|
-| M1 | `mcp_server.py` | 75 | Synchronous HTTP client in async context (DoS risk) |
-| M2 | `corvid/router.py` | 30–58 | No input length limits; no pagination |
-| M3 | `corvid/main.py` | — | No Content-Security-Policy or security headers |
-| M4 | `corvid/router.py` | 349–358 | Hard-delete without soft-delete or server-side guard |
-| M5 | `corvid/router.py` | 331–334 | Incomplete cycle detection for ticket parent chains (only checks direct self-reference) |
 
-## Low
+| Severity | Count |
+|----------|-------|
+| Critical | 0 |
+| High     | 0 |
+| Medium   | 1 |
+| Low      | 1 |
+| Info     | 1 |
 
-| ID | File | Line(s) | Issue |
+## Findings
-|----|------|---------|-------|
-| L1 | `docker-compose.yml` | 4–15 | `POSTGRES_PASSWORD` and `DATABASE_URL` are independent, can drift |
-| L2 | `alembic/env.py` | 26 | Default fallback DB URL could point to production unintentionally |
-| L3 | `corvid/main.py` | 41–43 | Unauthenticated `/health` endpoint |
-| L4 | `CLAUDE.md` | 16 | Hardcoded home directory path in documentation |
 
-## Fixed in this review
+### M1 — Auth fails open when `TICKETS_API_KEY` is unset (Medium)
 
-- C1: `shlex.quote()` applied to all interpolated values in `mcp_server.py`
+- **Location:** `corvid/router.py:164-165`, `corvid/config.py:4`
-- C2: Startup check — refuse to start if `TICKETS_API_KEY` is empty
+- **Description:** `TICKETS_API_KEY` defaults to an empty string (`config.py:4`). In
-- H4: `escHtml()` applied to `child.human_id` in `tickets_base.html`
+  `require_ticket_auth`, when no API key is configured the handler returns a synthetic
-- H4: `prefix` validated against alphanumeric-only regex in `router.py`
+  `{"username": "local", "email": "local@localhost"}` identity instead of raising 401
-- M5: Full cycle detection for parent chains
+  (`router.py:164-165`) — i.e. the service runs fully open. The intent is a "local mode",
+  but it is the **default** state and there is no startup warning or refuse-to-serve guard.
+- **Impact:** A deployment that forgets to set `TICKETS_API_KEY` exposes all ticket
+  read/write endpoints unauthenticated to anything that can reach the port. The web UI
+  never sends `X-Api-Key`, so it relies on this open mode unless fronted by an auth proxy.
+- **Recommendation:** Fail closed — refuse to start (or 401 all requests) when both
+  `TICKETS_API_KEY` is empty and no `require_user` dependency is wired, unless an explicit
+  `CORVID_ALLOW_ANONYMOUS=1` opt-in is set.
+
+### L1 — No rate limiting / lockout on the API-key check (Low)
+
+- **Location:** `corvid/router.py:151-154`
+- **Description:** The key comparison itself is constant-time (`hmac.compare_digest` —
+  good), but there is no throttling on repeated failed attempts.
+- **Recommendation:** Add basic rate limiting at the proxy or app layer.
+
+### Info — Internal DB DSN via environment
+
+`corvid/config.py`/`database.py` read the database URL from the environment; `.env` is
+gitignored and `.env.example` holds only placeholders. No secrets committed.
+
+## Clean (verified, no findings)
+
+- **Secrets:** none committed — `.env` is gitignored; `.env.example` is placeholders only.
+- **SQL injection:** all DB access is via SQLAlchemy ORM / bound parameters; no string-built SQL.
+- **XSS:** Jinja templates auto-escape; no `| safe` on user-controlled data observed.
+- **Command injection / unsafe deserialization:** no `subprocess`, `os.system`, `eval`,
+  `exec`, `pickle`, or `yaml.load` in tracked code.
+- **Auth comparison:** timing-safe via `hmac.compare_digest`.