fix(security): close critical auth/SSRF/RBAC holes

Verified locally end-to-end (before: exploitable, after: blocked). - config: refuse to start on weak/default/short SECRET_KEY (was trivially forgeable JWT -> full admin) - deps: add reusable require_admin dependency (JWT or API key) - api-keys: require admin to mint/list/revoke; mask key on list (was unauthenticated -> instant admin API key) - webhooks: whole router now admin-only (was fully unauthenticated CRUD + readable logs) - webhook delivery: validate URL scheme + reject hosts resolving to private/loopback/link-local/reserved IPs; disable redirects (was a readable SSRF primitive) - rbac: implement a real project-role hierarchy in check_project_role (was a no-op: any member, even guest, passed admin/mgr gates) - misc: auth on delete_milestone (+ensure_can_edit_milestone), worklog create/delete (force caller user_id, owner-only delete), /activity and /export/tasks (were unauthenticated data exposure) - tasks: auth + ensure_can_edit_task on assign_task and batch_assign Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-16 16:53:14 +01:00
parent c3199d0cd0
commit 51fb8ca073
7 changed files with 117 additions and 24 deletions
--- a/app/api/deps.py
+++ b/app/api/deps.py
@@ -76,3 +76,10 @@ async def get_current_user_or_apikey(
    if token:
        return await get_current_user(token=token, db=db)
    raise HTTPException(status_code=401, detail="Not authenticated")
+
+
+def require_admin(current_user: models.User = Depends(get_current_user_or_apikey)) -> models.User:
+    """Dependency: caller must be a global admin (JWT or API key)."""
+    if not getattr(current_user, "is_admin", False):
+        raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin privileges required")
+    return current_user
--- a/app/api/rbac.py
+++ b/app/api/rbac.py
@@ -81,12 +81,28 @@ def check_project_role(db: Session, user_id: int, project_id: int, min_role: str
            detail="Role not found"
        )

-    # Legacy compatibility: most current routes use non-hierarchical names like dev/mgr.
-    # For now, any valid membership passes those broad checks; strict edit rules are handled
-    # by the explicit can_edit_* helpers below.
-    if min_role in {"dev", "mgr", "viewer", "member", "guest", "admin"}:
-        return True
-
+    # Enforce a real role hierarchy. Higher rank == more privilege.
+    _RANK = {
+        "guest": 0,
+        "viewer": 1,
+        "member": 2,
+        "dev": 3,
+        "mgr": 4,
+        "admin": 5,
+    }
+    role_rank = _RANK.get((role.name or "").lower())
+    required_rank = _RANK.get((min_role or "member").lower())
+    if role_rank is None or required_rank is None:
+        # Unknown role on either side -> deny by default.
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN,
+            detail=f"Insufficient project role (have '{role.name}', need '{min_role}')",
+        )
+    if role_rank < required_rank:
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN,
+            detail=f"Insufficient project role (have '{role.name}', need '{min_role}')",
+        )
    return True


--- a/app/api/routers/misc.py
+++ b/app/api/routers/misc.py
@@ -12,7 +12,7 @@ from sqlalchemy import func as sqlfunc
 from pydantic import BaseModel

 from app.core.config import get_db
-from app.api.deps import get_current_user_or_apikey
+from app.api.deps import get_current_user_or_apikey, require_admin
 from app.api.rbac import check_project_role, ensure_can_edit_milestone
 from app.models import models
 from app.models.apikey import APIKey
@@ -60,7 +60,8 @@ class APIKeyResponse(BaseModel):


@router.post("/api-keys", response_model=APIKeyResponse, status_code=status.HTTP_201_CREATED, tags=["API Keys"])
-def create_api_key(data: APIKeyCreate, db: Session = Depends(get_db)):
+def create_api_key(data: APIKeyCreate, db: Session = Depends(get_db),
+                   _: models.User = Depends(require_admin)):
    user = db.query(models.User).filter(models.User.id == data.user_id).first()
    if not user:
        raise HTTPException(status_code=404, detail="User not found")
@@ -73,15 +74,22 @@ def create_api_key(data: APIKeyCreate, db: Session = Depends(get_db)):


@router.get("/api-keys", response_model=List[APIKeyResponse], tags=["API Keys"])
-def list_api_keys(user_id: int = None, db: Session = Depends(get_db)):
+def list_api_keys(user_id: int = None, db: Session = Depends(get_db),
+                 _: models.User = Depends(require_admin)):
    query = db.query(APIKey)
    if user_id:
        query = query.filter(APIKey.user_id == user_id)
-    return query.all()
+    keys = query.all()
+    # Never expose the full secret on listing; show only a masked prefix.
+    for k in keys:
+        if k.key and len(k.key) > 8:
+            k.key = k.key[:6] + "…" + k.key[-2:]
+    return keys


@router.delete("/api-keys/{key_id}", status_code=status.HTTP_204_NO_CONTENT, tags=["API Keys"])
-def revoke_api_key(key_id: int, db: Session = Depends(get_db)):
+def revoke_api_key(key_id: int, db: Session = Depends(get_db),
+                  _: models.User = Depends(require_admin)):
    key_obj = db.query(APIKey).filter(APIKey.id == key_id).first()
    if not key_obj:
        raise HTTPException(status_code=404, detail="API key not found")
@@ -106,7 +114,8 @@ class ActivityLogResponse(BaseModel):

@router.get("/activity", response_model=List[ActivityLogResponse], tags=["Activity"])
 def list_activity(entity_type: str = None, entity_id: int = None, user_id: int = None,
-                  limit: int = 50, db: Session = Depends(get_db)):
+                  limit: int = 50, db: Session = Depends(get_db),
+                  _: models.User = Depends(get_current_user_or_apikey)):
    query = db.query(ActivityLog)
    if entity_type:
        query = query.filter(ActivityLog.entity_type == entity_type)
@@ -199,8 +208,10 @@ def update_milestone(milestone_id: str, ms_update: schemas.MilestoneUpdate, db:


@router.delete("/milestones/{milestone_id}", status_code=status.HTTP_204_NO_CONTENT, tags=["Milestones"])
-def delete_milestone(milestone_id: str, db: Session = Depends(get_db)):
+def delete_milestone(milestone_id: str, db: Session = Depends(get_db),
+                     current_user: models.User = Depends(get_current_user_or_apikey)):
    ms = _resolve_milestone(db, milestone_id)
+    ensure_can_edit_milestone(db, current_user.id, ms)
    db.delete(ms)
    db.commit()
    return None
@@ -322,16 +333,18 @@ class WorkLogResponse(BaseModel):


@router.post("/worklogs", response_model=WorkLogResponse, status_code=status.HTTP_201_CREATED, tags=["Time Tracking"])
-def create_worklog(wl: WorkLogCreate, db: Session = Depends(get_db)):
+def create_worklog(wl: WorkLogCreate, db: Session = Depends(get_db),
+                   current_user: models.User = Depends(get_current_user_or_apikey)):
    task = db.query(Task).filter(Task.id == wl.task_id).first()
    if not task:
        raise HTTPException(status_code=404, detail="Task not found")
-    user = db.query(models.User).filter(models.User.id == wl.user_id).first()
-    if not user:
-        raise HTTPException(status_code=404, detail="User not found")
    if wl.hours <= 0:
        raise HTTPException(status_code=400, detail="Hours must be positive")
-    db_wl = WorkLog(**wl.model_dump())
+    data = wl.model_dump()
+    # Worklogs are always attributed to the caller (non-admins cannot log time for others).
+    if not current_user.is_admin or not data.get("user_id"):
+        data["user_id"] = current_user.id
+    db_wl = WorkLog(**data)
    db.add(db_wl)
    db.commit()
    db.refresh(db_wl)
@@ -370,10 +383,13 @@ def task_worklog_summary(task_id: str, db: Session = Depends(get_db)):


@router.delete("/worklogs/{worklog_id}", status_code=status.HTTP_204_NO_CONTENT, tags=["Time Tracking"])
-def delete_worklog(worklog_id: int, db: Session = Depends(get_db)):
+def delete_worklog(worklog_id: int, db: Session = Depends(get_db),
+                   current_user: models.User = Depends(get_current_user_or_apikey)):
    wl = db.query(WorkLog).filter(WorkLog.id == worklog_id).first()
    if not wl:
        raise HTTPException(status_code=404, detail="Work log not found")
+    if not current_user.is_admin and wl.user_id != current_user.id:
+        raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Cannot delete another user's worklog")
    db.delete(wl)
    db.commit()
    return None
@@ -382,7 +398,8 @@ def delete_worklog(worklog_id: int, db: Session = Depends(get_db)):
 # ============ Export ============

@router.get("/export/tasks", tags=["Export"])
-def export_tasks_csv(project_id: int = None, db: Session = Depends(get_db)):
+def export_tasks_csv(project_id: int = None, db: Session = Depends(get_db),
+                     _: models.User = Depends(get_current_user_or_apikey)):
    query = db.query(Task)
    if project_id:
        query = query.filter(Task.project_id == project_id)
--- a/app/api/routers/tasks.py
+++ b/app/api/routers/tasks.py
@@ -576,8 +576,10 @@ def take_task(
 # ---- Assignment ----

@router.post("/tasks/{task_code}/assign")
-def assign_task(task_code: str, assignee_id: int, db: Session = Depends(get_db)):
+def assign_task(task_code: str, assignee_id: int, db: Session = Depends(get_db),
+                current_user: models.User = Depends(get_current_user_or_apikey)):
    task = _resolve_task(db, task_code)
+    ensure_can_edit_task(db, current_user.id, task)
    user = db.query(models.User).filter(models.User.id == assignee_id).first()
    if not user:
        raise HTTPException(status_code=404, detail="User not found")
@@ -765,7 +767,8 @@ def batch_transition(


@router.post("/tasks/batch/assign")
-def batch_assign(data: BatchAssign, db: Session = Depends(get_db)):
+def batch_assign(data: BatchAssign, db: Session = Depends(get_db),
+                 current_user: models.User = Depends(get_current_user_or_apikey)):
    user = db.query(models.User).filter(models.User.id == data.assignee_id).first()
    if not user:
        raise HTTPException(status_code=404, detail="Assignee not found")
@@ -773,6 +776,7 @@ def batch_assign(data: BatchAssign, db: Session = Depends(get_db)):
    for task_code in data.task_codes:
        task = db.query(Task).filter(Task.task_code == task_code).first()
        if task:
+            ensure_can_edit_task(db, current_user.id, task)
            task.assignee_id = data.assignee_id
            updated.append(task.task_code)
    db.commit()
--- a/app/api/routers/webhooks.py
+++ b/app/api/routers/webhooks.py
@@ -5,11 +5,13 @@ from fastapi import APIRouter, Depends, HTTPException, status, BackgroundTasks
 from sqlalchemy.orm import Session

 from app.core.config import get_db
+from app.api.deps import require_admin
 from app.models.webhook import Webhook, WebhookLog
 from app.schemas.webhook import WebhookCreate, WebhookUpdate, WebhookResponse, WebhookLogResponse
 from app.services.webhook import fire_webhooks_sync

-router = APIRouter(prefix="/webhooks", tags=["Webhooks"])
+# Webhook management is admin-only (registration, inspection, retry, logs).
+router = APIRouter(prefix="/webhooks", tags=["Webhooks"], dependencies=[Depends(require_admin)])


@router.post("", response_model=WebhookResponse, status_code=status.HTTP_201_CREATED)
--- a/app/core/config.py
+++ b/app/core/config.py
@@ -44,6 +44,22 @@ class Settings(BaseSettings):

 settings = Settings()

+# Fail fast on a weak/default JWT signing key (prevents token forgery).
+_WEAK_SECRETS = {
+    "change-me-in-production",
+    "change_me_in_production",
+    "change-me-use-openssl-rand-hex-32",
+    "secret",
+    "changeme",
+    "",
+}
+if settings.SECRET_KEY in _WEAK_SECRETS or len(settings.SECRET_KEY) < 32:
+    raise RuntimeError(
+        "Insecure SECRET_KEY: set a strong random value "
+        "(e.g. `openssl rand -hex 32`) via the SECRET_KEY env var. "
+        "Refusing to start with a default/short key."
+    )
+
 # Resolve DB URL: wizard config volume > env > default
 _db_url = _resolve_db_url(settings.DATABASE_URL)
 engine = create_engine(_db_url, pool_pre_ping=True)
--- a/app/services/webhook.py
+++ b/app/services/webhook.py
@@ -2,12 +2,41 @@ import json
 import hmac
 import hashlib
 import logging
+import socket
+import ipaddress
+from urllib.parse import urlparse
 from sqlalchemy.orm import Session
 from app.models.webhook import Webhook, WebhookLog

 logger = logging.getLogger(__name__)


+def _validate_webhook_url(url: str) -> None:
+    """Raise ValueError if the URL would target a non-public address (SSRF guard)."""
+    parsed = urlparse(url)
+    if parsed.scheme not in ("http", "https"):
+        raise ValueError(f"unsupported scheme: {parsed.scheme!r}")
+    host = parsed.hostname
+    if not host:
+        raise ValueError("missing host")
+    # Resolve every address the host maps to and reject non-global ranges.
+    try:
+        infos = socket.getaddrinfo(host, parsed.port or (443 if parsed.scheme == "https" else 80))
+    except socket.gaierror as e:
+        raise ValueError(f"DNS resolution failed: {e}")
+    for info in infos:
+        ip = ipaddress.ip_address(info[4][0])
+        if (
+            ip.is_private
+            or ip.is_loopback
+            or ip.is_link_local
+            or ip.is_multicast
+            or ip.is_reserved
+            or ip.is_unspecified
+        ):
+            raise ValueError(f"host resolves to non-public address {ip}")
+
+
 def fire_webhooks_sync(event: str, payload: dict, project_id: int, db: Session):
    """Find matching webhooks and send payloads (sync version)."""
    import httpx
@@ -35,6 +64,8 @@ def fire_webhooks_sync(event: str, payload: dict, project_id: int, db: Session):
            payload=payload_json,
        )
        try:
+            _validate_webhook_url(wh.url)
+
            headers = {"Content-Type": "application/json"}
            if wh.secret:
                sig = hmac.new(
@@ -42,7 +73,7 @@ def fire_webhooks_sync(event: str, payload: dict, project_id: int, db: Session):
                ).hexdigest()
                headers["X-Webhook-Signature"] = sig

-            with httpx.Client(timeout=10.0) as client:
+            with httpx.Client(timeout=10.0, follow_redirects=False) as client:
                resp = client.post(wh.url, content=payload_json, headers=headers)
                log.response_status = resp.status_code
                log.response_body = resp.text[:1000]