corvid/SECURITY_REVIEW.md

# Security Review — corvid

Generated: 2026-07-13 (automated authorized audit)

Supersedes the prior manual review dated 2026-06-02.

## Scope

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.

## Methodology

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.

## Summary

| Severity | Count |
|----------|-------|
| Critical | 0 |
| High     | 0 |
| Medium   | 1 |
| Low      | 1 |
| Info     | 1 |

## Findings

### M1 — Auth fails open when `TICKETS_API_KEY` is unset (Medium)

- **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`.