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
This commit is contained in:
@@ -283,9 +283,8 @@ async def save_preferences(request: Request, current_user=Depends(auth.get_curre
|
|||||||
except HTTPException:
|
except HTTPException:
|
||||||
raise
|
raise
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
print(f"Error saving preferences: {e}")
|
logger.exception("Error saving preferences for user %s", current_user.username)
|
||||||
traceback.print_exc()
|
raise HTTPException(status_code=500, detail="Failed to save preferences")
|
||||||
raise HTTPException(status_code=500, detail=str(e))
|
|
||||||
|
|
||||||
|
|
||||||
@router.delete("/rbac/overrides/{username}")
|
@router.delete("/rbac/overrides/{username}")
|
||||||
|
|||||||
@@ -1,6 +1,9 @@
|
|||||||
import aiosqlite
|
import os
|
||||||
from fastapi import APIRouter, HTTPException
|
|
||||||
|
|
||||||
|
import aiosqlite
|
||||||
|
from fastapi import APIRouter, Depends, HTTPException
|
||||||
|
|
||||||
|
import auth_service as auth
|
||||||
from config import CHAT_SUMMARY_DB
|
from config import CHAT_SUMMARY_DB
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
@@ -38,10 +41,20 @@ async def get_messages(date: str):
|
|||||||
|
|
||||||
|
|
||||||
@router.post("/trigger")
|
@router.post("/trigger")
|
||||||
async def trigger_summary():
|
async def trigger_summary(current_user=Depends(auth.get_current_user)):
|
||||||
import os
|
"""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")
|
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)
|
os.makedirs(os.path.dirname(trigger_path), exist_ok=True)
|
||||||
with open(trigger_path, "w") as f:
|
with open(trigger_path, "w") as f:
|
||||||
f.write("1")
|
f.write("1")
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import logging
|
||||||
import os
|
import os
|
||||||
import re
|
import re
|
||||||
import shutil
|
import shutil
|
||||||
@@ -10,6 +11,7 @@ from config import VOLUME_ROOT
|
|||||||
from rbac import require_admin
|
from rbac import require_admin
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
BASE = Path(VOLUME_ROOT)
|
BASE = Path(VOLUME_ROOT)
|
||||||
|
|
||||||
MAX_UPLOAD_SIZE = 100 * 1024 * 1024 # 100 MB
|
MAX_UPLOAD_SIZE = 100 * 1024 * 1024 # 100 MB
|
||||||
@@ -131,5 +133,6 @@ def delete(path: str, recursive: bool = False):
|
|||||||
except PermissionError:
|
except PermissionError:
|
||||||
raise HTTPException(status_code=403, detail="Permission denied")
|
raise HTTPException(status_code=403, detail="Permission denied")
|
||||||
except OSError as exc:
|
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}
|
return {"ok": True}
|
||||||
|
|||||||
@@ -2,6 +2,8 @@
|
|||||||
OPC Agents Router
|
OPC Agents Router
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import logging
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, Request
|
from fastapi import APIRouter, Depends, HTTPException, Request
|
||||||
from pydantic import BaseModel
|
from pydantic import BaseModel
|
||||||
|
|
||||||
@@ -9,6 +11,7 @@ from db import opc_db
|
|||||||
from rbac import User, require_admin
|
from rbac import User, require_admin
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|
||||||
class AgentUpdate(BaseModel):
|
class AgentUpdate(BaseModel):
|
||||||
@@ -65,7 +68,8 @@ async def update_agent(
|
|||||||
)
|
)
|
||||||
return agent
|
return agent
|
||||||
except ValueError as e:
|
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")
|
@router.post("/agents/{agent_id}/execute")
|
||||||
@@ -121,4 +125,5 @@ async def approve_execution(
|
|||||||
)
|
)
|
||||||
return execution
|
return execution
|
||||||
except ValueError as e:
|
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")
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
"""WebAuthn Passkey authentication endpoints."""
|
"""WebAuthn Passkey authentication endpoints."""
|
||||||
|
|
||||||
import json
|
import json
|
||||||
|
import logging
|
||||||
import time
|
import time
|
||||||
from datetime import timedelta
|
from datetime import timedelta
|
||||||
|
|
||||||
@@ -22,6 +23,7 @@ import config
|
|||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
limiter = Limiter(key_func=get_remote_address)
|
limiter = Limiter(key_func=get_remote_address)
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
# In-memory challenge store with TTL
|
# In-memory challenge store with TTL
|
||||||
_challenges = {}
|
_challenges = {}
|
||||||
@@ -83,7 +85,8 @@ async def passkey_register_verify(request: Request, current_user=Depends(auth.ge
|
|||||||
expected_origin=config.WEBAUTHN_ORIGINS,
|
expected_origin=config.WEBAUTHN_ORIGINS,
|
||||||
)
|
)
|
||||||
except Exception as e:
|
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(
|
auth.save_passkey_credential(
|
||||||
{
|
{
|
||||||
@@ -137,7 +140,8 @@ async def passkey_login_verify(request: Request, response: Response):
|
|||||||
credential_current_sign_count=matched["sign_count"],
|
credential_current_sign_count=matched["sign_count"],
|
||||||
)
|
)
|
||||||
except Exception as e:
|
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
|
# Update sign count
|
||||||
matched["sign_count"] = verification.new_sign_count
|
matched["sign_count"] = verification.new_sign_count
|
||||||
|
|||||||
@@ -170,7 +170,7 @@ class TestChatSummaryTrigger:
|
|||||||
"""Test trigger summary endpoint."""
|
"""Test trigger summary endpoint."""
|
||||||
|
|
||||||
def test_trigger_summary_success(self, test_app, mock_config, admin_headers, tmp_path):
|
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"
|
trigger_path = tmp_path / "trigger"
|
||||||
|
|
||||||
with patch.dict(os.environ, {"CHAT_SUMMARY_TRIGGER": str(trigger_path)}):
|
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.parent.exists()
|
||||||
assert trigger_path.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):
|
def test_trigger_summary_without_auth(self, test_app, mock_config):
|
||||||
"""Test triggering summary without authentication."""
|
"""Test triggering summary without authentication."""
|
||||||
response = test_app.post("/api/chat-summary/trigger")
|
response = test_app.post("/api/chat-summary/trigger")
|
||||||
|
|||||||
Reference in New Issue
Block a user