diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md new file mode 100644 index 0000000..3fb47d0 --- /dev/null +++ b/SECURITY_REVIEW.md @@ -0,0 +1,46 @@ +# Security Review — corvid + +_Reviewed: 2026-06-02_ + +## Critical + +| 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) | + +## High + +| 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 | + +## Medium + +| 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) | + +## Low + +| 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 | + +## Fixed in this review + +- 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 diff --git a/corvid/router.py b/corvid/router.py index 8fc1e53..2ea2b07 100644 --- a/corvid/router.py +++ b/corvid/router.py @@ -1,4 +1,5 @@ import hmac +import re import uuid from fastapi import APIRouter, Depends, Header, HTTPException, Query, Request @@ -182,6 +183,8 @@ def make_router(api_key: str, get_db, require_user=None) -> APIRouter: prefix = body.prefix.strip().upper() if not prefix or len(prefix) > 16: raise HTTPException(400, "prefix must be 1–16 characters") + if not re.match(r'^[A-Z0-9_-]+$', prefix): + raise HTTPException(400, "prefix must contain only letters, digits, underscores, and hyphens") existing = (await db.execute( select(Environment).where(Environment.prefix == prefix) )).scalar_one_or_none() @@ -333,6 +336,19 @@ def make_router(api_key: str, get_db, require_user=None) -> APIRouter: if body.parent_id == ticket_id: raise HTTPException(400, "ticket cannot be its own parent") await _get_ticket_or_404(db, body.parent_id) + # Detect cycles of any depth before setting parent + ancestor_id = body.parent_id + visited: set[int] = set() + while ancestor_id is not None: + if ancestor_id == ticket_id: + raise HTTPException(400, "setting this parent would create a cycle") + if ancestor_id in visited: + break + visited.add(ancestor_id) + ancestor = (await db.execute( + select(Ticket).where(Ticket.id == ancestor_id) + )).scalar_one_or_none() + ancestor_id = ancestor.parent_id if ancestor else None t.parent_id = body.parent_id if "effort" in body.model_fields_set: if body.effort is not None and not (1 <= body.effort <= 10): diff --git a/mcp_server.py b/mcp_server.py index 692c47f..589675e 100644 --- a/mcp_server.py +++ b/mcp_server.py @@ -16,6 +16,7 @@ Configure in ~/.claude/mcp.json: """ import os +import shlex import httpx from mcp.server.fastmcp import FastMCP @@ -68,7 +69,7 @@ def _generate_startup_script(ticket_id: int, human_id: str, title: str, f"Start by calling mcp__homelab-tickets__get_ticket({ticket_id}) to read the full " f"ticket details, then complete the work and mark the ticket as completed." ) - return f'claude "{prompt}" --allowedTools "{tools_str}"' + return f'claude {shlex.quote(prompt)} --allowedTools {shlex.quote(tools_str)}' def _client() -> httpx.Client: