From 3f5f813c65f31e00292962f75f447b9f8ed77c7c Mon Sep 17 00:00:00 2001 From: hzhang Date: Sun, 31 May 2026 20:16:11 +0100 Subject: [PATCH] fix(security): RBAC on legacy create endpoints, hashed API keys, hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses findings from the security audit: - H1: add check_project_role to the legacy misc.py create endpoints (milestones=mgr, tasks/supports/meetings=dev) that previously required only authentication — closing a cross-project write bypass available to any logged-in user or agent API key. - M2: comments are always attributed to the authenticated caller; the client-supplied author_id is dropped (no author spoofing). - M3: API keys are stored as SHA-256 hashes (key_hash) plus a short key_prefix for display — never plaintext. Lookup hashes the presented key; listings never expose the secret. Includes an idempotent migration for existing deployments. - M5: the OIDC session cookie's Secure flag is env-driven via SESSION_COOKIE_SECURE (default True; set false for plain-HTTP dev). Co-Authored-By: Claude Opus 4.8 (1M context) --- app/api/deps.py | 9 ++++++- app/api/routers/comments.py | 8 +++++-- app/api/routers/misc.py | 47 ++++++++++++++++++++++++------------- app/api/routers/users.py | 7 +++--- app/core/config.py | 4 ++++ app/main.py | 15 +++++++++++- app/models/apikey.py | 5 +++- app/schemas/schemas.py | 4 +++- 8 files changed, 74 insertions(+), 25 deletions(-) diff --git a/app/api/deps.py b/app/api/deps.py index 9f6983d..0231320 100644 --- a/app/api/deps.py +++ b/app/api/deps.py @@ -1,4 +1,5 @@ """Shared auth dependencies.""" +import hashlib from datetime import datetime, timedelta from fastapi import Depends, HTTPException, status from fastapi.security import OAuth2PasswordBearer, APIKeyHeader @@ -59,11 +60,17 @@ async def get_current_user(token: str = Depends(oauth2_scheme), db: Session = De return user +def hash_api_key(raw: str) -> str: + """SHA-256 of a raw API key. Keys are high-entropy random tokens, so a + fast hash (not bcrypt) is appropriate and allows O(1) lookup by hash.""" + return hashlib.sha256(raw.encode()).hexdigest() + + def _lookup_api_key(db: Session, key: str) -> models.User | None: """Resolve an API key string to a User; mark last_used_at on hit.""" if not key: return None - key_obj = db.query(APIKey).filter(APIKey.key == key, APIKey.is_active == True).first() # noqa: E712 + key_obj = db.query(APIKey).filter(APIKey.key_hash == hash_api_key(key), APIKey.is_active == True).first() # noqa: E712 if not key_obj: return None key_obj.last_used_at = datetime.utcnow() diff --git a/app/api/routers/comments.py b/app/api/routers/comments.py index d754658..e27e6a8 100644 --- a/app/api/routers/comments.py +++ b/app/api/routers/comments.py @@ -32,8 +32,12 @@ def create_comment(comment: schemas.CommentCreate, db: Session = Depends(get_db) if not task: raise HTTPException(status_code=404, detail="Task not found") check_project_role(db, current_user.id, task.project_id, min_role="viewer") - - db_comment = models.Comment(**comment.model_dump()) + + # Always attribute the comment to the authenticated caller — never trust + # a client-supplied author_id (prevents author spoofing). + data = comment.model_dump() + data.pop("author_id", None) + db_comment = models.Comment(**data, author_id=current_user.id) db.add(db_comment) db.commit() db.refresh(db_comment) diff --git a/app/api/routers/misc.py b/app/api/routers/misc.py index 938bd9f..95ef149 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, require_admin +from app.api.deps import get_current_user_or_apikey, require_admin, hash_api_key from app.api.rbac import check_project_role, ensure_can_edit_milestone from app.models import models from app.models.apikey import APIKey @@ -49,7 +49,8 @@ class APIKeyCreate(BaseModel): class APIKeyResponse(BaseModel): id: int - key: str + key: str | None = None # full secret — only populated on create/reset + key_prefix: str | None = None # masked display for listings name: str user_id: int is_active: bool @@ -66,11 +67,16 @@ def create_api_key(data: APIKeyCreate, db: Session = Depends(get_db), if not user: raise HTTPException(status_code=404, detail="User not found") key = secrets.token_hex(32) - db_key = APIKey(key=key, name=data.name, user_id=data.user_id) + db_key = APIKey(key_hash=hash_api_key(key), key_prefix=key[:8], name=data.name, user_id=data.user_id) db.add(db_key) db.commit() db.refresh(db_key) - return db_key + # Return the raw key once (it is never stored or shown again). + return { + "id": db_key.id, "key": key, "key_prefix": db_key.key_prefix, + "name": db_key.name, "user_id": db_key.user_id, "is_active": db_key.is_active, + "created_at": db_key.created_at, "last_used_at": db_key.last_used_at, + } @router.get("/api-keys", response_model=List[APIKeyResponse], tags=["API Keys"]) @@ -80,11 +86,14 @@ def list_api_keys(user_id: int = None, db: Session = Depends(get_db), if user_id: query = query.filter(APIKey.user_id == user_id) 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 + # Never expose the secret on listing — the raw key isn't stored. Show only + # the masked prefix. + return [{ + "id": k.id, "key": None, + "key_prefix": (k.key_prefix + "…") if k.key_prefix else None, + "name": k.name, "user_id": k.user_id, "is_active": k.is_active, + "created_at": k.created_at, "last_used_at": k.last_used_at, + } for k in keys] @router.delete("/api-keys/{key_id}", status_code=status.HTTP_204_NO_CONTENT, tags=["API Keys"]) @@ -132,7 +141,10 @@ def list_activity(entity_type: str = None, entity_id: int = None, user_id: int = def create_milestone(ms: schemas.MilestoneCreate, db: Session = Depends(get_db), current_user: models.User = Depends(get_current_user_or_apikey)): import json project = db.query(models.Project).filter(models.Project.id == ms.project_id).first() - project_code = project.project_code if project and project.project_code else f"P{ms.project_id}" + if not project: + raise HTTPException(status_code=404, detail="Project not found") + check_project_role(db, current_user.id, project.id, min_role="mgr") + project_code = project.project_code if project.project_code else f"P{ms.project_id}" max_ms = db.query(MilestoneModel).filter(MilestoneModel.project_id == ms.project_id).order_by(MilestoneModel.id.desc()).first() next_num = (max_ms.id + 1) if max_ms else 1 @@ -488,14 +500,15 @@ def create_milestone_task(project_code: str, milestone_id: str, task_data: dict, project = db.query(models.Project).filter(models.Project.project_code == project_code).first() if not project: raise HTTPException(status_code=404, detail="Project not found") - + check_project_role(db, current_user.id, project.id, min_role="dev") + ms = db.query(MilestoneModel).filter(MilestoneModel.milestone_code == milestone_id, MilestoneModel.project_id == project.id).first() if not ms: raise HTTPException(status_code=404, detail="Milestone not found") - + if ms.status and hasattr(ms.status, "value") and ms.status.value == "undergoing": raise HTTPException(status_code=400, detail="Cannot add items to a milestone that is undergoing") - + milestone_code = ms.milestone_code or f"m{ms.id}" max_task = db.query(Task).filter(Task.milestone_id == ms.id).order_by(Task.id.desc()).first() next_num = (max_task.id + 1) if max_task else 1 @@ -622,6 +635,7 @@ def create_support(project_code: str, milestone_id: str, support_data: dict, db: project = db.query(models.Project).filter(models.Project.project_code == project_code).first() if not project: raise HTTPException(status_code=404, detail="Project not found") + check_project_role(db, current_user.id, project.id, min_role="dev") ms = db.query(MilestoneModel).filter(MilestoneModel.milestone_code == milestone_id, MilestoneModel.project_id == project.id).first() if not ms: @@ -768,14 +782,15 @@ def create_meeting(project_code: str, milestone_id: str, meeting_data: dict, db: project = db.query(models.Project).filter(models.Project.project_code == project_code).first() if not project: raise HTTPException(status_code=404, detail="Project not found") - + check_project_role(db, current_user.id, project.id, min_role="dev") + ms = db.query(MilestoneModel).filter(MilestoneModel.milestone_code == milestone_id, MilestoneModel.project_id == project.id).first() if not ms: raise HTTPException(status_code=404, detail="Milestone not found") - + if ms.status and hasattr(ms.status, "value") and ms.status.value == "undergoing": raise HTTPException(status_code=400, detail="Cannot add items to a milestone that is undergoing") - + milestone_code = ms.milestone_code or f"m{ms.id}" max_meeting = db.query(Meeting).filter(Meeting.milestone_id == ms.id).order_by(Meeting.id.desc()).first() next_num = (max_meeting.id + 1) if max_meeting else 1 diff --git a/app/api/routers/users.py b/app/api/routers/users.py index 7c9d128..c5d4a2e 100644 --- a/app/api/routers/users.py +++ b/app/api/routers/users.py @@ -7,7 +7,7 @@ from pydantic import BaseModel from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session -from app.api.deps import get_current_user, get_current_user_or_apikey, get_password_hash +from app.api.deps import get_current_user, get_current_user_or_apikey, get_password_hash, hash_api_key from app.core.config import get_db, settings from app.init_bootstrap import DELETED_USER_USERNAME from app.models import models @@ -464,9 +464,10 @@ def reset_user_apikey( existing_key.is_active = False db.flush() - # Create new key + # Create new key (store only the hash + a display prefix) new_key = APIKey( - key=new_key_value, + key_hash=hash_api_key(new_key_value), + key_prefix=new_key_value[:8], name=f"{target_user.username}-key", user_id=target_user.id, is_active=True, diff --git a/app/core/config.py b/app/core/config.py index 5b57fbc..64fe71c 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -22,6 +22,10 @@ class Settings(BaseSettings): # in via a bound OIDC identity / API keys), frontend hides password UI. HARBORFORGE_OIDC_ONLY: bool = False + # Mark the OIDC state/session cookie Secure (HTTPS-only). Defaults to True + # for production; set SESSION_COOKIE_SECURE=false for plain-HTTP local dev. + SESSION_COOKIE_SECURE: bool = True + class Config: env_file = ".env" diff --git a/app/main.py b/app/main.py index 86173ae..1179831 100644 --- a/app/main.py +++ b/app/main.py @@ -27,7 +27,7 @@ app.add_middleware( secret_key=settings.SECRET_KEY, session_cookie="hf_oidc", same_site="lax", - https_only=False, + https_only=settings.SESSION_COOKIE_SECURE, max_age=600, ) @@ -449,6 +449,19 @@ def _migrate_schema(): "CREATE INDEX idx_time_slots_special_slot_id ON time_slots (special_slot_id)" )) + # --- api_keys: migrate legacy plaintext `key` -> hashed `key_hash` --- + # Only runs on deployments that still have the old plaintext column; + # fresh installs get key_hash/key_prefix directly from create_all. + if _has_table(db, "api_keys") and _has_column(db, "api_keys", "key"): + if not _has_column(db, "api_keys", "key_hash"): + db.execute(text("ALTER TABLE api_keys ADD COLUMN key_hash VARCHAR(64) NULL")) + if not _has_column(db, "api_keys", "key_prefix"): + db.execute(text("ALTER TABLE api_keys ADD COLUMN key_prefix VARCHAR(16) NULL")) + db.execute(text("ALTER TABLE api_keys MODIFY COLUMN `key` VARCHAR(64) NULL")) + db.execute(text("UPDATE api_keys SET key_hash = SHA2(`key`, 256), key_prefix = LEFT(`key`, 8) WHERE key_hash IS NULL AND `key` IS NOT NULL")) + db.execute(text("UPDATE api_keys SET `key` = NULL WHERE `key` IS NOT NULL")) + _ensure_unique_index(db, "api_keys", "idx_api_keys_key_hash", "key_hash") + # --- schedule_type_special_slots: create-table is handled by # Base.metadata.create_all on first boot; no migration needed here # because there is no legacy table to evolve. Future schema bumps diff --git a/app/models/apikey.py b/app/models/apikey.py index 3bad394..62e31fe 100644 --- a/app/models/apikey.py +++ b/app/models/apikey.py @@ -7,7 +7,10 @@ class APIKey(Base): __tablename__ = "api_keys" id = Column(Integer, primary_key=True, index=True) - key = Column(String(64), unique=True, nullable=False, index=True) + # The raw key is never stored — only its SHA-256 hash. `key_prefix` holds + # the first few chars for human-readable display/masking in listings. + key_hash = Column(String(64), unique=True, nullable=False, index=True) + key_prefix = Column(String(16), nullable=True) name = Column(String(100), nullable=False) # e.g. "agent-zhi", "agent-lyn" user_id = Column(Integer, ForeignKey("users.id"), nullable=False) is_active = Column(Boolean, default=True) diff --git a/app/schemas/schemas.py b/app/schemas/schemas.py index 8fc2beb..5116855 100644 --- a/app/schemas/schemas.py +++ b/app/schemas/schemas.py @@ -105,7 +105,9 @@ class CommentBase(BaseModel): class CommentCreate(CommentBase): task_id: int - author_id: int + # author_id is NOT accepted from the client — the comment is always + # attributed to the authenticated caller (server-side) to prevent + # author spoofing. class CommentUpdate(BaseModel):