Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions SECURITY-REVIEW.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

---

Expand Down
51 changes: 50 additions & 1 deletion app/middleware/logging_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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}" '
Expand Down
5 changes: 5 additions & 0 deletions docs/usage/configuration/environment.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
46 changes: 46 additions & 0 deletions tests/unit/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down