feat(security): add comprehensive input validation and sanitization (#172)

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

SECURITY_AUDIT.md

@@ -278,7 +278,17 @@ ftp = ftplib.FTP()  # nosec B321 - Plaintext FTP intentional when configured
 - ✅ Streaming file reads in upload endpoint to prevent memory exhaustion
 - ⏳ **TODO:** Implement rate limiting on API endpoints
 - ⏳ **TODO:** Add CSRF protection for state-changing operations
-- ⏳ **TODO:** Add comprehensive input sanitization for all user inputs ([#172](https://github.com/christianlouis/DocuElevate/issues/172))
+- ✅ **COMPLETED:** Add comprehensive input sanitization for all user inputs ([#172](https://github.com/christianlouis/DocuElevate/issues/172))
+  - `app/utils/input_validation.py` — centralized validation module with:
+    - `validate_setting_key()`: allow-lists setting keys against `SETTING_METADATA` (prevents attribute enumeration / Python object sniffing via `getattr`)
+    - `validate_sort_field()`: enforces sort field against an explicit allow-list
+    - `validate_sort_order()`: ensures sort direction is exactly `asc` or `desc`
+    - `validate_search_query()`: strips whitespace, enforces 255-character maximum
+    - `validate_task_id()`: validates Celery task IDs against UUID v4 format
+  - Applied to `app/api/settings.py` (GET/POST/DELETE `/{key}` endpoints)
+  - Applied to `app/api/files.py` (file list sort + search query parameters)
+  - Applied to `app/api/logs.py` (task_id query filter and path parameter)
+  - 30 unit tests added in `tests/test_input_validation.py`
 - ⏳ **TODO:** Implement proper API key rotation mechanisms ([#168](https://github.com/christianlouis/DocuElevate/issues/168))
 
 ### Infrastructure Security
@@ -303,7 +313,7 @@ ftp = ftplib.FTP()  # nosec B321 - Plaintext FTP intentional when configured
 ### High Priority
 1. ~~**Enable CodeQL scanning**~~ ✅ Already implemented - Two CodeQL workflows active
 2. **Implement rate limiting** - Prevent abuse and DoS attacks (consider slowapi or fastapi-limiter)
-3. **Add comprehensive input validation** - Prevent injection attacks ([#172](https://github.com/christianlouis/DocuElevate/issues/172))
+3. ~~**Add comprehensive input validation**~~ ✅ Implemented — centralized `app/utils/input_validation.py` module with allow-list validators for sort fields, sort order, search queries, task IDs, and setting keys; applied across `files.py`, `logs.py`, and `settings.py` endpoints ([#172](https://github.com/christianlouis/DocuElevate/issues/172))
 4. ~~**Add request size limits**~~ ✅ Implemented - `RequestSizeLimitMiddleware` enforces `MAX_REQUEST_BODY_SIZE` (default 1 MB) for non-file requests and `MAX_UPLOAD_SIZE` (default 1 GB) for multipart uploads; file uploads also use streaming reads to bound memory usage ([#173](https://github.com/christianlouis/DocuElevate/issues/173))
 5. **Implement CSRF protection** - Protect state-changing operations
 

app/api/files.py

@@ -21,6 +21,7 @@ from app.tasks.process_document import process_document
 from app.utils.file_queries import apply_status_filter
 from app.utils.file_status import get_files_processing_status
 from app.utils.filename_utils import sanitize_filename
+from app.utils.input_validation import validate_search_query, validate_sort_field, validate_sort_order
 
 # Set up logging
 logger = logging.getLogger(__name__)
@@ -77,6 +78,11 @@ def list_files_api(
       }
     }
     """
+    # Validate and sanitize query parameters
+    validate_sort_field(sort_by)
+    validate_sort_order(sort_order)
+    search = validate_search_query(search)
+
     # Start with base query
     query = db.query(FileRecord)
 

app/api/logs.py

@@ -12,6 +12,7 @@ from sqlalchemy.orm import Session
 from app.auth import require_login
 from app.database import get_db
 from app.models import FileRecord, ProcessingLog
+from app.utils.input_validation import validate_task_id
 
 # Set up logging
 logger = logging.getLogger(__name__)
@@ -59,6 +60,7 @@ def list_processing_logs(
     if file_id is not None:
         query = query.filter(ProcessingLog.file_id == file_id)
     if task_id is not None:
+        validate_task_id(task_id)
         query = query.filter(ProcessingLog.task_id == task_id)
 
     # Order by timestamp descending and limit
@@ -132,6 +134,7 @@ def get_task_processing_logs(request: Request, task_id: str, db: DbSession):
     Get all processing logs for a specific task.
     Returns logs ordered by timestamp (oldest first to show processing flow).
     """
+    validate_task_id(task_id)
     # Get all logs for this task
     logs = db.query(ProcessingLog).filter(ProcessingLog.task_id == task_id).order_by(ProcessingLog.timestamp).all()
 

app/api/settings.py

@@ -11,6 +11,7 @@ from sqlalchemy.orm import Session
 
 from app.config import settings
 from app.database import get_db
+from app.utils.input_validation import validate_setting_key
 from app.utils.settings_service import (
     SETTING_METADATA,
     delete_setting_from_db,
@@ -98,6 +99,7 @@ async def get_setting(key: str, request: Request, db: DbSession, admin: AdminUse
     Get a specific setting by key.
     Admin only.
     """
+    validate_setting_key(key)
     try:
         # Get current value
         value = getattr(settings, key, None)
@@ -125,6 +127,7 @@ async def update_setting(
     Update a specific setting.
     Admin only.
     """
+    validate_setting_key(key)
     try:
         # Validate the setting value
         if setting.value is not None:
@@ -165,6 +168,7 @@ async def delete_setting(key: str, request: Request, db: DbSession, admin: Admin
     Delete a setting from the database (reverts to environment variable or default).
     Admin only.
     """
+    validate_setting_key(key)
     try:
         success = delete_setting_from_db(db, key)
         if not success:

app/utils/input_validation.py

@@ -0,0 +1,168 @@
+"""
+Centralized input validation and sanitization utilities.
+
+Provides reusable validators used across API endpoints to prevent injection attacks,
+path traversal, and malformed payloads. See SECURITY_AUDIT.md (Code Security section)
+for context.
+"""
+
+import logging
+import re
+from typing import Optional
+
+from fastapi import HTTPException, status
+
+logger = logging.getLogger(__name__)
+
+# Allowed sort fields for the files list endpoint
+ALLOWED_SORT_FIELDS = frozenset({"id", "original_filename", "file_size", "mime_type", "created_at"})
+
+# Allowed sort orders
+ALLOWED_SORT_ORDERS = frozenset({"asc", "desc"})
+
+# Maximum length for free-text search queries
+MAX_SEARCH_QUERY_LENGTH = 255
+
+# Pattern for valid Celery task IDs (UUID v4 format: version=4, variant=[89ab])
+_TASK_ID_RE = re.compile(r"^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$", re.IGNORECASE)
+
+# Pattern for valid setting keys (alphanumeric + underscore, non-empty, max 128 chars)
+_SETTING_KEY_RE = re.compile(r"^[a-zA-Z][a-zA-Z0-9_]{0,127}$")
+
+
+def validate_sort_field(sort_by: str) -> str:
+    """
+    Validate that *sort_by* is one of the allowed sort fields.
+
+    Args:
+        sort_by: The sort field name supplied by the client.
+
+    Returns:
+        The validated sort field name (unchanged).
+
+    Raises:
+        HTTPException 422: If *sort_by* is not in the allow-list.
+    """
+    if sort_by not in ALLOWED_SORT_FIELDS:
+        allowed = ", ".join(sorted(ALLOWED_SORT_FIELDS))
+        logger.warning(f"Invalid sort_by value rejected: {sort_by!r}")
+        raise HTTPException(
+            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+            detail=f"Invalid sort_by value '{sort_by}'. Allowed values: {allowed}",
+        )
+    return sort_by
+
+
+def validate_sort_order(sort_order: str) -> str:
+    """
+    Validate that *sort_order* is either 'asc' or 'desc'.
+
+    Args:
+        sort_order: The sort order supplied by the client.
+
+    Returns:
+        The validated sort order (unchanged).
+
+    Raises:
+        HTTPException 422: If *sort_order* is not 'asc' or 'desc'.
+    """
+    if sort_order not in ALLOWED_SORT_ORDERS:
+        logger.warning(f"Invalid sort_order value rejected: {sort_order!r}")
+        raise HTTPException(
+            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+            detail=f"Invalid sort_order value '{sort_order}'. Allowed values: asc, desc",
+        )
+    return sort_order
+
+
+def validate_search_query(search: Optional[str]) -> Optional[str]:
+    """
+    Sanitize and validate a free-text search query.
+
+    Strips leading/trailing whitespace and enforces a maximum length to prevent
+    overly long inputs from reaching the database layer.
+
+    Args:
+        search: The search string supplied by the client, or None.
+
+    Returns:
+        The sanitized search string, or None if the input was None or empty.
+
+    Raises:
+        HTTPException 422: If the search query exceeds MAX_SEARCH_QUERY_LENGTH.
+    """
+    if search is None:
+        return None
+    search = search.strip()
+    if not search:
+        return None
+    if len(search) > MAX_SEARCH_QUERY_LENGTH:
+        raise HTTPException(
+            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+            detail=f"Search query too long (max {MAX_SEARCH_QUERY_LENGTH} characters)",
+        )
+    return search
+
+
+def validate_task_id(task_id: str) -> str:
+    """
+    Validate that *task_id* matches the expected Celery UUID format.
+
+    Args:
+        task_id: The task ID supplied by the client.
+
+    Returns:
+        The validated task ID (unchanged).
+
+    Raises:
+        HTTPException 422: If *task_id* does not match the expected UUID format.
+    """
+    if not _TASK_ID_RE.match(task_id):
+        logger.warning(f"Invalid task_id format rejected: {task_id!r}")
+        raise HTTPException(
+            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+            detail="Invalid task_id format. Expected a UUID (e.g. 550e8400-e29b-41d4-a716-446655440000)",
+        )
+    return task_id
+
+
+def validate_setting_key(key: str) -> str:
+    """
+    Validate that *key* is a syntactically valid setting key.
+
+    Performs two checks:
+    1. The key matches the allowed character pattern (alphanumeric + underscore,
+       starting with a letter) to prevent attribute injection or enumeration of
+       internal Python object attributes.
+    2. The key exists in the ``SETTING_METADATA`` registry, so callers cannot
+       read or write arbitrary attributes of the ``Settings`` object.
+
+    Args:
+        key: The setting key supplied by the client.
+
+    Returns:
+        The validated setting key (unchanged).
+
+    Raises:
+        HTTPException 400: If the key contains invalid characters.
+        HTTPException 404: If the key is not a known setting.
+    """
+    # Structural check first to avoid importing settings_service unnecessarily
+    if not _SETTING_KEY_RE.match(key):
+        logger.warning(f"Invalid setting key format rejected: {key!r}")
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail="Invalid setting key format",
+        )
+
+    # Allow-list check: only expose keys that are declared in SETTING_METADATA
+    from app.utils.settings_service import SETTING_METADATA  # local import to avoid circular deps
+
+    if key not in SETTING_METADATA:
+        logger.warning(f"Unknown setting key rejected: {key!r}")
+        raise HTTPException(
+            status_code=status.HTTP_404_NOT_FOUND,
+            detail=f"Setting '{key}' not found",
+        )
+
+    return key

tests/test_api_logs.py

@@ -37,15 +37,25 @@ class TestListProcessingLogs:
 
     def test_list_logs_filter_by_task_id(self, client, db_session):
         """Test filtering logs by task_id."""
-        log1 = ProcessingLog(task_id="task-a", step_name="step1", status="success", message="Log A")
-        log2 = ProcessingLog(task_id="task-b", step_name="step2", status="success", message="Log B")
+        log1 = ProcessingLog(
+            task_id="aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa",
+            step_name="step1",
+            status="success",
+            message="Log A",
+        )
+        log2 = ProcessingLog(
+            task_id="bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb",
+            step_name="step2",
+            status="success",
+            message="Log B",
+        )
         db_session.add_all([log1, log2])
         db_session.commit()
 
-        response = client.get("/api/logs?task_id=task-a")
+        response = client.get("/api/logs?task_id=aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa")
         assert response.status_code == 200
         data = response.json()
-        assert all(log["task_id"] == "task-a" for log in data)
+        assert all(log["task_id"] == "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" for log in data)
 
     def test_list_logs_with_limit(self, client, db_session):
         """Test limiting number of returned logs."""
@@ -106,8 +116,9 @@ class TestGetTaskProcessingLogs:
 
     def test_get_logs_for_existing_task(self, client, db_session):
         """Test getting logs for an existing task."""
+        task_uuid = "cccccccc-cccc-4ccc-8ccc-cccccccccccc"
         log = ProcessingLog(
-            task_id="test-task-abc",
+            task_id=task_uuid,
             step_name="process_document",
             status="success",
             message="Done",
@@ -115,13 +126,13 @@ class TestGetTaskProcessingLogs:
         db_session.add(log)
         db_session.commit()
 
-        response = client.get("/api/logs/task/test-task-abc")
+        response = client.get(f"/api/logs/task/{task_uuid}")
         assert response.status_code == 200
         data = response.json()
-        assert data["task_id"] == "test-task-abc"
+        assert data["task_id"] == task_uuid
         assert len(data["logs"]) == 1
 
     def test_get_logs_for_nonexistent_task(self, client):
         """Test getting logs for a task that doesn't exist."""
-        response = client.get("/api/logs/task/nonexistent-task")
+        response = client.get("/api/logs/task/dddddddd-dddd-4ddd-8ddd-dddddddddddd")
         assert response.status_code == 404

tests/test_input_validation.py

@@ -0,0 +1,277 @@
+"""
+Unit and integration tests for app/utils/input_validation.py.
+
+Covers all validators: sort field/order, search query, task ID, and setting key.
+"""
+
+import pytest
+from fastapi import HTTPException
+
+
+# ---------------------------------------------------------------------------
+# validate_sort_field
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+class TestValidateSortField:
+    """Tests for validate_sort_field."""
+
+    def test_accepts_valid_sort_fields(self):
+        """All declared sort fields should be accepted."""
+        from app.utils.input_validation import ALLOWED_SORT_FIELDS, validate_sort_field
+
+        for field in ALLOWED_SORT_FIELDS:
+            assert validate_sort_field(field) == field
+
+    def test_rejects_unknown_field(self):
+        """An unknown sort field should raise 422."""
+        from app.utils.input_validation import validate_sort_field
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_field("nonexistent_field")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_sql_injection_attempt(self):
+        """A SQL-injection-style field name should raise 422."""
+        from app.utils.input_validation import validate_sort_field
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_field("id; DROP TABLE files; --")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_empty_string(self):
+        """An empty string should raise 422."""
+        from app.utils.input_validation import validate_sort_field
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_field("")
+        assert exc_info.value.status_code == 422
+
+    def test_error_message_lists_allowed_values(self):
+        """Error detail should mention the allowed values."""
+        from app.utils.input_validation import validate_sort_field
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_field("bad_field")
+        assert "created_at" in exc_info.value.detail
+
+
+# ---------------------------------------------------------------------------
+# validate_sort_order
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+class TestValidateSortOrder:
+    """Tests for validate_sort_order."""
+
+    def test_accepts_asc(self):
+        from app.utils.input_validation import validate_sort_order
+
+        assert validate_sort_order("asc") == "asc"
+
+    def test_accepts_desc(self):
+        from app.utils.input_validation import validate_sort_order
+
+        assert validate_sort_order("desc") == "desc"
+
+    def test_rejects_uppercase_asc(self):
+        """Validation is case-sensitive; 'ASC' should be rejected."""
+        from app.utils.input_validation import validate_sort_order
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_order("ASC")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_arbitrary_string(self):
+        from app.utils.input_validation import validate_sort_order
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_order("random")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_empty_string(self):
+        from app.utils.input_validation import validate_sort_order
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_sort_order("")
+        assert exc_info.value.status_code == 422
+
+
+# ---------------------------------------------------------------------------
+# validate_search_query
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+class TestValidateSearchQuery:
+    """Tests for validate_search_query."""
+
+    def test_returns_none_for_none_input(self):
+        from app.utils.input_validation import validate_search_query
+
+        assert validate_search_query(None) is None
+
+    def test_returns_none_for_blank_string(self):
+        from app.utils.input_validation import validate_search_query
+
+        assert validate_search_query("   ") is None
+
+    def test_strips_whitespace(self):
+        from app.utils.input_validation import validate_search_query
+
+        assert validate_search_query("  hello  ") == "hello"
+
+    def test_accepts_normal_query(self):
+        from app.utils.input_validation import validate_search_query
+
+        assert validate_search_query("invoice 2024") == "invoice 2024"
+
+    def test_rejects_too_long_query(self):
+        """A query longer than 255 characters should raise 422."""
+        from app.utils.input_validation import MAX_SEARCH_QUERY_LENGTH, validate_search_query
+
+        long_query = "a" * (MAX_SEARCH_QUERY_LENGTH + 1)
+        with pytest.raises(HTTPException) as exc_info:
+            validate_search_query(long_query)
+        assert exc_info.value.status_code == 422
+
+    def test_accepts_query_at_max_length(self):
+        """A query exactly at the maximum length should be accepted."""
+        from app.utils.input_validation import MAX_SEARCH_QUERY_LENGTH, validate_search_query
+
+        exact_query = "a" * MAX_SEARCH_QUERY_LENGTH
+        assert validate_search_query(exact_query) == exact_query
+
+
+# ---------------------------------------------------------------------------
+# validate_task_id
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+class TestValidateTaskId:
+    """Tests for validate_task_id."""
+
+    def test_accepts_valid_uuid(self):
+        from app.utils.input_validation import validate_task_id
+
+        valid_uuid = "550e8400-e29b-41d4-a716-446655440000"
+        assert validate_task_id(valid_uuid) == valid_uuid
+
+    def test_accepts_uppercase_uuid(self):
+        """UUID validation should be case-insensitive."""
+        from app.utils.input_validation import validate_task_id
+
+        upper_uuid = "550E8400-E29B-41D4-A716-446655440000"
+        assert validate_task_id(upper_uuid) == upper_uuid
+
+    def test_rejects_non_v4_uuid(self):
+        """A syntactically valid UUID with a version other than 4 should be rejected."""
+        from app.utils.input_validation import validate_task_id
+
+        # Version 1 UUID (version digit is '1', not '4')
+        with pytest.raises(HTTPException) as exc_info:
+            validate_task_id("550e8400-e29b-11d4-a716-446655440000")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_short_string(self):
+        from app.utils.input_validation import validate_task_id
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_task_id("abc-123")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_sql_injection(self):
+        from app.utils.input_validation import validate_task_id
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_task_id("'; DROP TABLE processing_logs; --")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_path_traversal(self):
+        from app.utils.input_validation import validate_task_id
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_task_id("../../etc/passwd")
+        assert exc_info.value.status_code == 422
+
+    def test_rejects_empty_string(self):
+        from app.utils.input_validation import validate_task_id
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_task_id("")
+        assert exc_info.value.status_code == 422
+
+    def test_error_message_mentions_uuid(self):
+        from app.utils.input_validation import validate_task_id
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_task_id("not-a-uuid")
+        assert "UUID" in exc_info.value.detail
+
+
+# ---------------------------------------------------------------------------
+# validate_setting_key
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+class TestValidateSettingKey:
+    """Tests for validate_setting_key."""
+
+    def test_accepts_known_setting_key(self):
+        """A key that exists in SETTING_METADATA should be accepted."""
+        from app.utils.input_validation import validate_setting_key
+
+        # 'workdir' is always defined in SETTING_METADATA
+        assert validate_setting_key("workdir") == "workdir"
+
+    def test_rejects_unknown_key_with_404(self):
+        """An unknown but syntactically valid key should raise 404."""
+        from app.utils.input_validation import validate_setting_key
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_setting_key("totally_unknown_key_xyz")
+        assert exc_info.value.status_code == 404
+
+    def test_rejects_key_with_special_characters(self):
+        """A key with special characters (e.g., injection attempt) should raise 400."""
+        from app.utils.input_validation import validate_setting_key
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_setting_key("__class__")
+        assert exc_info.value.status_code == 400
+
+    def test_rejects_key_with_dot_notation(self):
+        """Dot-separated attribute traversal should be rejected."""
+        from app.utils.input_validation import validate_setting_key
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_setting_key("model_fields")
+        # model_fields is a Pydantic internal but not in SETTING_METADATA -> 404
+        assert exc_info.value.status_code == 404
+
+    def test_rejects_dunder_attributes(self):
+        """Double-underscore attributes should be rejected (bad format)."""
+        from app.utils.input_validation import validate_setting_key
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_setting_key("__dict__")
+        assert exc_info.value.status_code == 400
+
+    def test_rejects_empty_key(self):
+        from app.utils.input_validation import validate_setting_key
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_setting_key("")
+        assert exc_info.value.status_code == 400
+
+    def test_rejects_key_starting_with_digit(self):
+        """Keys starting with a digit should be rejected."""
+        from app.utils.input_validation import validate_setting_key
+
+        with pytest.raises(HTTPException) as exc_info:
+            validate_setting_key("1nvalid_key")
+        assert exc_info.value.status_code == 400