From 2da7a6ec51d454192c5686a50f553305bfa7ce71 Mon Sep 17 00:00:00 2001 From: caoimhinr Date: Tue, 14 Jul 2026 05:52:50 +0200 Subject: [PATCH] docs: add automated security review findings Co-Authored-By: Claude Opus 4.8 --- SECURITY_REVIEW.md | 88 ++++++++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 34 deletions(-) diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md index 3fb47d0..7bb13a3 100644 --- a/SECURITY_REVIEW.md +++ b/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 | -|----|------|---------|-------| -| 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) | +## Scope -## 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 | -|----|------|---------|-------| -| 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 | +## Methodology -## 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 | -|----|------|---------|-------| -| 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) | +## Summary -## Low +| Severity | Count | +|----------|-------| +| Critical | 0 | +| High | 0 | +| Medium | 1 | +| Low | 1 | +| Info | 1 | -| ID | File | Line(s) | Issue | -|----|------|---------|-------| -| 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 | +## Findings -## 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` -- C2: Startup check — refuse to start if `TICKETS_API_KEY` is empty -- H4: `escHtml()` applied to `child.human_id` in `tickets_base.html` -- H4: `prefix` validated against alphanumeric-only regex in `router.py` -- M5: Full cycle detection for parent chains +- **Location:** `corvid/router.py:164-165`, `corvid/config.py:4` +- **Description:** `TICKETS_API_KEY` defaults to an empty string (`config.py:4`). In + `require_ticket_auth`, when no API key is configured the handler returns a synthetic + `{"username": "local", "email": "local@localhost"}` identity instead of raising 401 + (`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`.