Merge pull request #805 from seapagan/security/redact-query-logs

fix(security): redact sensitive query params in request logs

Author: seapagan

SECURITY-REVIEW.md

@@ -93,6 +93,9 @@
 
 ### 5. Sensitive Data in Request Logs
 
+> [!NOTE]
+> ✅ **Done**: Request logging now redacts sensitive query parameters by name.
+
 **Location**: `app/middleware/logging_middleware.py:44`
 
 - **Issue**: Logs full query strings including:
@@ -526,12 +529,12 @@
 
 **Total Issues Identified: 33**
 
-| Priority | Count | Must Fix Before Production? |
-|----------|-------|-----------------------------|
-| **CRITICAL** | 5 | ✅ YES - Security vulnerabilities |
-| **High** | 9 | ✅ YES - Important security/quality |
-| **Medium** | 14 | ⚠️ Recommended - Hardening needed |
-| **Low** | 5 | 💡 Optional - Nice to have |
+| Priority     | Count         | Must Fix Before Production?         |
+|--------------|---------------|-------------------------------------|
+| **CRITICAL** | 5 (3 closed)  | ✅ YES - Security vulnerabilities   |
+| **High**     | 9 (0 closed)  | ✅ YES - Important security/quality |
+| **Medium**   | 14 (0 closed) | ⚠️ Recommended - Hardening needed   |
+| **Low**      | 5 (0 closed)  | 💡 Optional - Nice to have          |
 
 **Overall Assessment**: The codebase demonstrates solid security foundations with
 proper JWT authentication, password hashing (bcrypt), and SQL injection protection
@@ -633,19 +636,20 @@ rate limiting, token validation, and API key scope enforcement.
 
 The codebase demonstrates several excellent security practices:
 
-✅ **Strong cryptography**: bcrypt for passwords, HMAC-SHA256 for API keys, proper
-JWT handling
-✅ **SQL injection protection**: SQLAlchemy ORM throughout, no raw SQL
-✅ **Token validation**: Format checking before expensive crypto operations (DoS
-prevention)
-✅ **Secret key validation**: Strong validation at startup, prevents weak keys
-✅ **Email enumeration protection**: Password reset correctly prevents enumeration
-✅ **Self-ban prevention**: User can't ban themselves
-✅ **Last admin protection**: Check in place (though has race condition)
-✅ **Category-based logging**: Excellent separation of concerns for security
-monitoring
-✅ **Proper password hashing**: Automatic salting, modern algorithms
-✅ **Database password validation**: Prevents weak defaults in production
+- ✅ **Strong cryptography**: bcrypt for passwords, HMAC-SHA256 for API keys,
+  proper JWT handling
+- ✅ **SQL injection protection**: SQLAlchemy ORM throughout, no raw SQL
+- ✅ **Token validation**: Format checking before expensive crypto operations
+  (DoS prevention)
+- ✅ **Secret key validation**: Strong validation at startup, prevents weak keys
+- ✅ **Email enumeration protection**: Password reset correctly prevents
+  enumeration
+- ✅ **Self-ban prevention**: User can't ban themselves
+- ✅ **Last admin protection**: Check in place (though has race condition)
+- ✅ **Category-based logging**: Excellent separation of concerns for security
+  monitoring
+- ✅ **Proper password hashing**: Automatic salting, modern algorithms
+- ✅ **Database password validation**: Prevents weak defaults in production
 
 ---
 

app/middleware/logging_middleware.py

@@ -4,6 +4,7 @@
 
 import time
 from typing import TYPE_CHECKING
+from urllib.parse import parse_qsl, urlencode
 
 from loguru import logger
 from starlette.middleware.base import BaseHTTPMiddleware
@@ -20,6 +21,53 @@
 class LoggingMiddleware(BaseHTTPMiddleware):
     """Log HTTP requests and responses if REQUESTS category is enabled."""
 
+    REDACTED_VALUE = "REDACTED"
+    # Order matters: keep currently used keys early for short-circuit checks.
+    SENSITIVE_QUERY_KEYS = (
+        "code",
+        "token",
+        "reset_token",
+        "verification",
+        "verify",
+        "access_token",
+        "refresh_token",
+        "api_key",
+        "key",
+    )
+
+    @classmethod
+    def _should_redact(cls, key: str) -> bool:
+        """Check if a query parameter name should be redacted."""
+        key_lower = key.lower()
+        for sensitive_key in cls.SENSITIVE_QUERY_KEYS:
+            if key_lower == sensitive_key:
+                return True
+        return False
+
+    @classmethod
+    def _redact_query(cls, query: str) -> str:
+        """Redact sensitive query parameters from a raw query string."""
+        if not query:
+            return ""
+
+        params = parse_qsl(query, keep_blank_values=True)
+        if not params:
+            return query
+
+        redacted = False
+        redacted_params: list[tuple[str, str]] = []
+        for key, value in params:
+            if cls._should_redact(key):
+                redacted = True
+                redacted_params.append((key, cls.REDACTED_VALUE))
+                continue
+            redacted_params.append((key, value))
+
+        if not redacted:
+            return query
+
+        return urlencode(redacted_params)
+
     async def dispatch(
         self,
         request: Request,
@@ -42,7 +90,8 @@ async def dispatch(
         # Build full path including query string if present
         path = request.url.path
         if request.url.query:
-            path = f"{path}?{request.url.query}"
+            redacted_query = self._redact_query(request.url.query)
+            path = f"{path}?{redacted_query}"
 
         logger.info(
             f'{client_addr} - "{request.method} {path}" '

docs/usage/configuration/environment.md

@@ -653,6 +653,11 @@ LOG_CATEGORIES=NONE
 - Case-insensitive: `auth` = `AUTH`
 - Whitespace is trimmed: `AUTH, ERRORS` works fine
 
+!!! note "Request log redaction"
+    Request logs redact sensitive query parameters by name before writing to
+    disk (for example `code` and `token`). To customize the list, update
+    `SENSITIVE_QUERY_KEYS` in `app/middleware/logging_middleware.py`.
+
 ### Log Filename
 
 Custom filename for the log file:

tests/unit/test_logging.py

@@ -460,6 +460,7 @@ async def test_middleware_logs_when_requests_enabled(
         mock_request.client = Mock(host="127.0.0.1")
         mock_request.method = "GET"
         mock_request.url.path = "/api/users"
+        mock_request.url.query = ""
 
         mock_response = Mock(spec=Response)
         mock_response.status_code = 200
@@ -513,6 +514,51 @@ async def test_middleware_logs_query_parameters(
         assert "200" in log_message
         assert result == mock_response
 
+    async def test_middleware_redacts_sensitive_query_parameters(
+        self, mocker: MockerFixture
+    ) -> None:
+        """Test middleware redacts sensitive query parameters in logs."""
+        mock_log_config = mocker.patch(
+            "app.middleware.logging_middleware.log_config"
+        )
+        mock_log_config.is_enabled.return_value = True
+
+        mock_logger = mocker.patch("app.middleware.logging_middleware.logger")
+
+        middleware = LoggingMiddleware(app=Mock())
+
+        # Create mock request with sensitive query parameters
+        mock_request = Mock(spec=Request)
+        mock_request.client = Mock(host="127.0.0.1")
+        mock_request.method = "GET"
+        mock_request.url.path = "/api/auth"
+        mock_request.url.query = "page=2&code=secret&token=abc&API_KEY=bad"
+
+        mock_response = Mock(spec=Response)
+        mock_response.status_code = 200
+        mock_call_next = AsyncMock(return_value=mock_response)
+
+        result = await middleware.dispatch(mock_request, mock_call_next)
+
+        mock_logger.info.assert_called_once()
+        log_message = mock_logger.info.call_args[0][0]
+        assert "GET /api/auth?page=2" in log_message
+        assert "code=REDACTED" in log_message
+        assert "token=REDACTED" in log_message
+        assert "API_KEY=REDACTED" in log_message
+        assert "secret" not in log_message
+        assert "abc" not in log_message
+        assert "bad" not in log_message
+        assert result == mock_response
+
+    def test_redact_query_returns_empty_for_blank_query(self) -> None:
+        """Test redaction helper returns empty for blank query."""
+        assert LoggingMiddleware._redact_query("") == ""
+
+    def test_redact_query_returns_original_when_no_params(self) -> None:
+        """Test redaction helper keeps original when parse yields no params."""
+        assert LoggingMiddleware._redact_query("&") == "&"
+
     async def test_middleware_logs_without_query_parameters(
         self, mocker: MockerFixture
     ) -> None: