From dac163d88ca7a59fba25b9a072772b25974e5782 Mon Sep 17 00:00:00 2001 From: "Gan, Jimmy" Date: Wed, 8 Apr 2026 00:57:14 +0800 Subject: [PATCH] security: fix critical vulnerabilities (issues #2 and #3) Issue #2: Unprotected Chat Summary Trigger (CRITICAL) - Add authentication and admin authorization to /trigger endpoint - Add path validation to prevent directory traversal - Block access to system directories (/etc, /root, /sys, /proc, /boot) - Add tests for unauthorized access and invalid paths Issue #3: Exception Information Disclosure (HIGH) - Replace raw exception messages with generic errors - Log full exception details server-side with logger.exception() - Affected files: auth.py, files.py, passkey.py, opc_agents.py - Prevents information leakage about system architecture Security Impact: - Prevents unauthenticated file system writes - Reduces reconnaissance opportunities for attackers - Maintains security while preserving debugging capability Tests: 214 passing, all security tests verified --- dashboard/backend/routers/auth.py | 5 ++-- dashboard/backend/routers/chat_summary.py | 21 +++++++++++--- dashboard/backend/routers/files.py | 5 +++- dashboard/backend/routers/opc_agents.py | 9 ++++-- dashboard/backend/routers/passkey.py | 8 ++++-- .../tests/integration/test_chat_summary.py | 28 ++++++++++++++++++- 6 files changed, 63 insertions(+), 13 deletions(-) diff --git a/dashboard/backend/routers/auth.py b/dashboard/backend/routers/auth.py index 2aa5f5d..90efb49 100644 --- a/dashboard/backend/routers/auth.py +++ b/dashboard/backend/routers/auth.py @@ -283,9 +283,8 @@ async def save_preferences(request: Request, current_user=Depends(auth.get_curre except HTTPException: raise except Exception as e: - print(f"Error saving preferences: {e}") - traceback.print_exc() - raise HTTPException(status_code=500, detail=str(e)) + logger.exception("Error saving preferences for user %s", current_user.username) + raise HTTPException(status_code=500, detail="Failed to save preferences") @router.delete("/rbac/overrides/{username}") diff --git a/dashboard/backend/routers/chat_summary.py b/dashboard/backend/routers/chat_summary.py index 79f1f93..4febf8d 100644 --- a/dashboard/backend/routers/chat_summary.py +++ b/dashboard/backend/routers/chat_summary.py @@ -1,6 +1,9 @@ -import aiosqlite -from fastapi import APIRouter, HTTPException +import os +import aiosqlite +from fastapi import APIRouter, Depends, HTTPException + +import auth_service as auth from config import CHAT_SUMMARY_DB router = APIRouter() @@ -38,10 +41,20 @@ async def get_messages(date: str): @router.post("/trigger") -async def trigger_summary(): - import os +async def trigger_summary(current_user=Depends(auth.get_current_user)): + """Trigger chat summary generation. Admin only.""" + if current_user.role != "admin": + raise HTTPException(status_code=403, detail="Admin access required") trigger_path = os.environ.get("CHAT_SUMMARY_TRIGGER", "/app/data/chat-summarizer/trigger") + + # Validate path to prevent directory traversal to sensitive locations + trigger_path = os.path.abspath(trigger_path) + # Block access to system directories + forbidden_prefixes = ("/etc/", "/root/", "/sys/", "/proc/", "/boot/") + if any(trigger_path.startswith(prefix) for prefix in forbidden_prefixes): + raise HTTPException(status_code=400, detail="Invalid trigger path") + os.makedirs(os.path.dirname(trigger_path), exist_ok=True) with open(trigger_path, "w") as f: f.write("1") diff --git a/dashboard/backend/routers/files.py b/dashboard/backend/routers/files.py index c9b76d1..dcbe15f 100644 --- a/dashboard/backend/routers/files.py +++ b/dashboard/backend/routers/files.py @@ -1,3 +1,4 @@ +import logging import os import re import shutil @@ -10,6 +11,7 @@ from config import VOLUME_ROOT from rbac import require_admin router = APIRouter() +logger = logging.getLogger(__name__) BASE = Path(VOLUME_ROOT) MAX_UPLOAD_SIZE = 100 * 1024 * 1024 # 100 MB @@ -131,5 +133,6 @@ def delete(path: str, recursive: bool = False): except PermissionError: raise HTTPException(status_code=403, detail="Permission denied") except OSError as exc: - raise HTTPException(status_code=400, detail=str(exc)) + logger.exception("OS error during file deletion: %s", path) + raise HTTPException(status_code=400, detail="Failed to delete file") return {"ok": True} diff --git a/dashboard/backend/routers/opc_agents.py b/dashboard/backend/routers/opc_agents.py index fb93309..08ebebb 100644 --- a/dashboard/backend/routers/opc_agents.py +++ b/dashboard/backend/routers/opc_agents.py @@ -2,6 +2,8 @@ OPC Agents Router """ +import logging + from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel @@ -9,6 +11,7 @@ from db import opc_db from rbac import User, require_admin router = APIRouter() +logger = logging.getLogger(__name__) class AgentUpdate(BaseModel): @@ -65,7 +68,8 @@ async def update_agent( ) return agent except ValueError as e: - raise HTTPException(status_code=404, detail=str(e)) + logger.exception("Failed to update agent %s", agent_id) + raise HTTPException(status_code=404, detail="Agent not found") @router.post("/agents/{agent_id}/execute") @@ -121,4 +125,5 @@ async def approve_execution( ) return execution except ValueError as e: - raise HTTPException(status_code=404, detail=str(e)) + logger.exception("Failed to approve agent execution %s", execution_id) + raise HTTPException(status_code=404, detail="Execution not found") diff --git a/dashboard/backend/routers/passkey.py b/dashboard/backend/routers/passkey.py index a2f6db4..f49d5a6 100644 --- a/dashboard/backend/routers/passkey.py +++ b/dashboard/backend/routers/passkey.py @@ -1,6 +1,7 @@ """WebAuthn Passkey authentication endpoints.""" import json +import logging import time from datetime import timedelta @@ -22,6 +23,7 @@ import config router = APIRouter() limiter = Limiter(key_func=get_remote_address) +logger = logging.getLogger(__name__) # In-memory challenge store with TTL _challenges = {} @@ -83,7 +85,8 @@ async def passkey_register_verify(request: Request, current_user=Depends(auth.ge expected_origin=config.WEBAUTHN_ORIGINS, ) except Exception as e: - raise HTTPException(status_code=400, detail=str(e)) + logger.exception("Passkey registration verification failed") + raise HTTPException(status_code=400, detail="Invalid passkey registration") auth.save_passkey_credential( { @@ -137,7 +140,8 @@ async def passkey_login_verify(request: Request, response: Response): credential_current_sign_count=matched["sign_count"], ) except Exception as e: - raise HTTPException(status_code=400, detail=str(e)) + logger.exception("Passkey authentication verification failed") + raise HTTPException(status_code=400, detail="Invalid passkey authentication") # Update sign count matched["sign_count"] = verification.new_sign_count diff --git a/dashboard/backend/tests/integration/test_chat_summary.py b/dashboard/backend/tests/integration/test_chat_summary.py index 0487e66..39a46b3 100644 --- a/dashboard/backend/tests/integration/test_chat_summary.py +++ b/dashboard/backend/tests/integration/test_chat_summary.py @@ -170,7 +170,7 @@ class TestChatSummaryTrigger: """Test trigger summary endpoint.""" def test_trigger_summary_success(self, test_app, mock_config, admin_headers, tmp_path): - """Test triggering a summary generation.""" + """Test triggering a summary generation as admin.""" trigger_path = tmp_path / "trigger" with patch.dict(os.environ, {"CHAT_SUMMARY_TRIGGER": str(trigger_path)}): @@ -193,6 +193,32 @@ class TestChatSummaryTrigger: assert trigger_path.parent.exists() assert trigger_path.exists() + def test_trigger_summary_as_member(self, test_app, mock_config, temp_auth_file, temp_rbac_file, tmp_path): + """Test that non-admin cannot trigger summary.""" + from datetime import timedelta + + from auth_service import create_access_token + + member_token = create_access_token(data={"sub": "memberuser", "role": "member"}, expires_delta=timedelta(minutes=30)) + headers = {"Authorization": f"Bearer {member_token}"} + + trigger_path = tmp_path / "trigger" + with patch.dict(os.environ, {"CHAT_SUMMARY_TRIGGER": str(trigger_path)}): + response = test_app.post("/api/chat-summary/trigger", headers=headers) + + # Page-level RBAC blocks access before endpoint-level check + assert response.status_code == 403 + assert "denied" in response.json()["detail"].lower() + + def test_trigger_summary_invalid_path(self, test_app, mock_config, admin_headers): + """Test that invalid trigger paths are rejected.""" + # Try to write outside /app/data/ + with patch.dict(os.environ, {"CHAT_SUMMARY_TRIGGER": "/etc/passwd"}): + response = test_app.post("/api/chat-summary/trigger", headers=admin_headers) + + assert response.status_code == 400 + assert "Invalid trigger path" in response.json()["detail"] + def test_trigger_summary_without_auth(self, test_app, mock_config): """Test triggering summary without authentication.""" response = test_app.post("/api/chat-summary/trigger")