Implement critical security fixes: secret management, XML parsing, auth, input validation, security headers
Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
@@ -1,13 +1,17 @@
|
||||
from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks
|
||||
from typing import Dict, Any
|
||||
from datetime import datetime
|
||||
import logging
|
||||
|
||||
from app.services.imap_client import IMAPClient
|
||||
from app.core.security import require_admin_auth
|
||||
|
||||
router = APIRouter()
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@router.post("/test-connection")
|
||||
async def test_imap_connection(
|
||||
auth: dict = Depends(require_admin_auth),
|
||||
server: str = None,
|
||||
port: int = 993,
|
||||
username: str = None,
|
||||
@@ -16,7 +20,18 @@ async def test_imap_connection(
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Test connection to an IMAP server and gather mailbox statistics
|
||||
|
||||
Security: Requires authentication (X-API-Key or Bearer token)
|
||||
Note: Credentials should be passed in request body, not query params
|
||||
"""
|
||||
# Security: Don't accept credentials in query parameters (they get logged)
|
||||
if any([server, username, password]):
|
||||
logger.warning("IMAP credentials passed as query parameters - this is insecure")
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail="Credentials should be passed in request body, not query parameters"
|
||||
)
|
||||
|
||||
imap_client = IMAPClient(
|
||||
server=server,
|
||||
port=port,
|
||||
@@ -40,12 +55,22 @@ async def test_imap_connection(
|
||||
@router.post("/fetch-reports")
|
||||
async def fetch_imap_reports(
|
||||
background_tasks: BackgroundTasks,
|
||||
auth: dict = Depends(require_admin_auth),
|
||||
days: int = 7,
|
||||
delete_emails: bool = False
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Fetch DMARC reports from the configured IMAP mailbox
|
||||
|
||||
Security: Requires authentication (X-API-Key or Bearer token)
|
||||
"""
|
||||
# Security: Validate parameters
|
||||
if days < 1 or days > 365:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail="Days parameter must be between 1 and 365"
|
||||
)
|
||||
|
||||
imap_client = IMAPClient(delete_emails=delete_emails)
|
||||
|
||||
# Run in background if it might take a while
|
||||
@@ -58,64 +83,41 @@ async def fetch_imap_reports(
|
||||
}
|
||||
|
||||
# Otherwise run immediately
|
||||
results = imap_client.fetch_reports(days=days)
|
||||
|
||||
return {
|
||||
"success": results["success"],
|
||||
"processed_emails": results["processed"],
|
||||
"reports_found": results["reports_found"],
|
||||
"new_domains": results["new_domains"],
|
||||
"errors": results["errors"] if "errors" in results and results["errors"] else None,
|
||||
"timestamp": datetime.now().isoformat()
|
||||
}
|
||||
try:
|
||||
results = imap_client.fetch_reports(days=days)
|
||||
|
||||
return {
|
||||
"success": results["success"],
|
||||
"processed_emails": results["processed"],
|
||||
"reports_found": results["reports_found"],
|
||||
"new_domains": results["new_domains"],
|
||||
"errors": results["errors"] if "errors" in results and results["errors"] else None,
|
||||
"timestamp": datetime.now().isoformat()
|
||||
}
|
||||
except Exception as e:
|
||||
logger.error(f"Error fetching IMAP reports: {str(e)}")
|
||||
raise HTTPException(
|
||||
status_code=500,
|
||||
detail="Failed to fetch reports. Check server logs for details."
|
||||
)
|
||||
|
||||
|
||||
@router.get("/status")
|
||||
async def get_imap_status() -> Dict[str, Any]:
|
||||
async def get_imap_status(auth: dict = Depends(require_admin_auth)) -> Dict[str, Any]:
|
||||
"""
|
||||
Get the current status of IMAP polling background processes
|
||||
|
||||
Security: Requires authentication (X-API-Key or Bearer token)
|
||||
"""
|
||||
# In a real implementation this would check a persistent store
|
||||
# or a global variable tracking the status of background tasks
|
||||
# For now, returning mock data as this is MVP
|
||||
# For now, returning simplified status
|
||||
|
||||
# Get the last check time if available
|
||||
last_check_time = None
|
||||
try:
|
||||
# In a production app, this would be stored in database
|
||||
# For MVP, using a simple file-based approach
|
||||
import os
|
||||
status_file = os.path.join(os.path.dirname(__file__), "../../../../../tmp/imap_last_check.txt")
|
||||
if os.path.exists(status_file):
|
||||
with open(status_file, "r") as f:
|
||||
last_check_time = f.read().strip()
|
||||
except:
|
||||
pass
|
||||
|
||||
# If status file doesn't exist, create the directory
|
||||
try:
|
||||
os.makedirs(os.path.dirname(os.path.join(os.path.dirname(__file__), "../../../../../tmp")), exist_ok=True)
|
||||
except:
|
||||
pass
|
||||
|
||||
# For demonstration purposes, update the last check time to now
|
||||
# In a real app, this would be updated by the background process
|
||||
try:
|
||||
with open(os.path.join(os.path.dirname(__file__), "../../../../../tmp/imap_last_check.txt"), "w") as f:
|
||||
now = datetime.now().isoformat()
|
||||
f.write(now)
|
||||
# If there was no previous check time, set it to now
|
||||
if not last_check_time:
|
||||
last_check_time = now
|
||||
except:
|
||||
pass
|
||||
|
||||
# Return the status
|
||||
return {
|
||||
"is_running": True, # In a real app, check if the background task is running
|
||||
"last_check": last_check_time,
|
||||
"next_check": None, # In production, this would be calculated based on polling interval
|
||||
"messages_processed": 0, # In production, this would track actual messages processed
|
||||
"reports_found": 0, # In production, this would track reports found
|
||||
"last_check": None, # In production, track actual last check time
|
||||
"next_check": None, # In production, calculate based on polling interval
|
||||
"messages_processed": 0, # In production, track actual messages processed
|
||||
"reports_found": 0, # In production, track reports found
|
||||
"timestamp": datetime.now().isoformat()
|
||||
}
|
||||
@@ -1,12 +1,38 @@
|
||||
from typing import Dict, List, Any
|
||||
from fastapi import APIRouter, File, HTTPException, UploadFile, status
|
||||
from pydantic import BaseModel
|
||||
import logging
|
||||
|
||||
from app.services.dmarc_parser import DMARCParser
|
||||
from app.services.report_store import ReportStore
|
||||
from app.utils.domain_validator import validate_domain
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Try to import python-magic for MIME type detection
|
||||
try:
|
||||
import magic
|
||||
HAS_MAGIC = True
|
||||
except ImportError:
|
||||
HAS_MAGIC = False
|
||||
logger.warning("python-magic not installed. MIME type validation will be skipped.")
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
# Security: Allowed MIME types for DMARC report uploads
|
||||
ALLOWED_MIME_TYPES = {
|
||||
'text/xml',
|
||||
'application/xml',
|
||||
'application/zip',
|
||||
'application/x-zip-compressed',
|
||||
'application/gzip',
|
||||
'application/x-gzip',
|
||||
'application/octet-stream' # Sometimes zip/gzip are detected as this
|
||||
}
|
||||
|
||||
# Security: Allowed file extensions
|
||||
ALLOWED_EXTENSIONS = {'.xml', '.zip', '.gz', '.gzip'}
|
||||
|
||||
class UploadResponse(BaseModel):
|
||||
"""Response model for report upload"""
|
||||
success: bool
|
||||
@@ -45,21 +71,80 @@ class PaginatedReportResponse(BaseModel):
|
||||
async def upload_report(file: UploadFile = File(...)):
|
||||
"""
|
||||
Upload and process a DMARC aggregate report file (XML, ZIP, or GZIP)
|
||||
|
||||
Security:
|
||||
- File type validation (extension and MIME type)
|
||||
- File size limits enforced in parser
|
||||
- Zip bomb protection
|
||||
- Sanitized error messages
|
||||
"""
|
||||
try:
|
||||
# Security: Validate filename is provided
|
||||
if not file.filename:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail="Filename is required"
|
||||
)
|
||||
|
||||
# Security: Validate file extension
|
||||
file_ext = '.' + file.filename.rsplit('.', 1)[-1].lower() if '.' in file.filename else ''
|
||||
if file_ext not in ALLOWED_EXTENSIONS:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Invalid file type. Allowed types: {', '.join(ALLOWED_EXTENSIONS)}"
|
||||
)
|
||||
|
||||
# Read the file content
|
||||
file_content = await file.read()
|
||||
filename = file.filename
|
||||
|
||||
# Security: Validate file is not empty
|
||||
if len(file_content) == 0:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail="File is empty"
|
||||
)
|
||||
|
||||
# Security: Validate MIME type using python-magic (if available)
|
||||
if HAS_MAGIC:
|
||||
try:
|
||||
mime_type = magic.from_buffer(file_content, mime=True)
|
||||
if mime_type not in ALLOWED_MIME_TYPES:
|
||||
logger.warning(f"Rejected file with MIME type: {mime_type}")
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Invalid file type. File must be XML, ZIP, or GZIP format."
|
||||
)
|
||||
except Exception as e:
|
||||
# If magic fails, log but continue (fallback to extension check)
|
||||
logger.warning(f"MIME type detection failed: {str(e)}")
|
||||
else:
|
||||
logger.debug("MIME type validation skipped (python-magic not available)")
|
||||
|
||||
# Parse the report
|
||||
parser = DMARCParser()
|
||||
report = parser.parse_file(file_content, filename)
|
||||
report = parser.parse_file(file_content, file.filename)
|
||||
|
||||
# Security: Validate domain from report
|
||||
domain = report.get("domain", "")
|
||||
if not domain:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail="Report does not contain a valid domain"
|
||||
)
|
||||
|
||||
# Validate domain format (not DNS resolution to avoid external calls)
|
||||
is_valid, error_msg = validate_domain(domain)
|
||||
if not is_valid and "could not be resolved" not in error_msg.lower():
|
||||
# Allow domains that fail DNS resolution but have valid format
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Invalid domain in report: {error_msg}"
|
||||
)
|
||||
|
||||
# Store the report
|
||||
store = ReportStore.get_instance()
|
||||
store.add_report(report)
|
||||
|
||||
domain = report.get("domain", "unknown")
|
||||
processed_records = report.get("summary", {}).get("total_count", 0)
|
||||
|
||||
return UploadResponse(
|
||||
@@ -69,10 +154,36 @@ async def upload_report(file: UploadFile = File(...)):
|
||||
processed_records=processed_records
|
||||
)
|
||||
|
||||
except HTTPException:
|
||||
# Re-raise HTTP exceptions as-is
|
||||
raise
|
||||
except ValueError as e:
|
||||
# Security: Sanitize error messages from parser
|
||||
error_message = str(e)
|
||||
# Log full error for debugging
|
||||
logger.error(f"ValueError processing report {file.filename}: {error_message}")
|
||||
# Return sanitized message
|
||||
if "too large" in error_message.lower():
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE,
|
||||
detail="File too large"
|
||||
)
|
||||
elif "zip bomb" in error_message.lower():
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail="Invalid archive file"
|
||||
)
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail="Invalid report format"
|
||||
)
|
||||
except Exception as e:
|
||||
# Security: Don't expose internal errors to client
|
||||
logger.error(f"Unexpected error processing report {file.filename}: {str(e)}")
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Error processing report: {str(e)}"
|
||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||
detail="Error processing report. Please contact support if this persists."
|
||||
)
|
||||
|
||||
@router.get("/domains", response_model=List[str])
|
||||
|
||||
Reference in New Issue
Block a user