From 801a63f8bb49555a2c6d3275f9ca5c200ac5fad5 Mon Sep 17 00:00:00 2001 From: hzhang Date: Sat, 16 May 2026 16:53:14 +0100 Subject: [PATCH] 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) --- app/api/deps.py | 7 ++++++ app/api/rbac.py | 28 ++++++++++++++++++----- app/api/routers/misc.py | 45 +++++++++++++++++++++++++------------ app/api/routers/tasks.py | 8 +++++-- app/api/routers/webhooks.py | 4 +++- app/core/config.py | 16 +++++++++++++ app/services/webhook.py | 33 ++++++++++++++++++++++++++- 7 files changed, 117 insertions(+), 24 deletions(-) diff --git a/app/api/deps.py b/app/api/deps.py index 5a10a47..bf70393 100644 --- 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 diff --git a/app/api/rbac.py b/app/api/rbac.py index 39127a7..578d789 100644 --- 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 diff --git a/app/api/routers/misc.py b/app/api/routers/misc.py index 0040d3e..938bd9f 100644 --- 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) diff --git a/app/api/routers/tasks.py b/app/api/routers/tasks.py index 6eeb36b..bdc606a 100644 --- 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() diff --git a/app/api/routers/webhooks.py b/app/api/routers/webhooks.py index daabeea..d89ea0d 100644 --- 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) diff --git a/app/core/config.py b/app/core/config.py index 8200002..e011199 100644 --- 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) diff --git a/app/services/webhook.py b/app/services/webhook.py index cf28ea7..886a76e 100644 --- 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]