mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 11:12:35 +02:00
Fix critical security vulnerabilities (GHSA-gc24-px2r-5qmf)
## Summary
Address two critical security issues reported via GitHub Security Advisory:
1. Hardcoded JWT secret key allowing token forgery
2. Missing authentication on 77+ API endpoints
## Changes
### JWT Secret Key (backend/app/core/auth.py)
- Remove hardcoded secret "bambuddy-secret-key-change-in-production"
- Load secret from JWT_SECRET_KEY environment variable (recommended)
- Fall back to .jwt_secret file in data directory (auto-generated)
- Generate cryptographically secure 64-byte random secret if neither exists
- File is created with 0600 permissions for security
### API Authentication Middleware (backend/app/main.py)
- Add HTTP middleware that enforces auth on ALL /api/ routes
- When auth is enabled, every API request requires valid JWT or API key
- Only exempt routes that must be public:
- /api/v1/auth/status (check if auth enabled)
- /api/v1/auth/login (login endpoint)
- /api/v1/updates/version (version check)
- /api/v1/ws/* (WebSockets handle own auth)
### Test Updates
- backend/tests/conftest.py: Patch middleware's async_session for tests
- backend/tests/integration/test_ownership_permissions.py: Add missing
auth headers to requests that now require authentication
## Migration Notes
- Existing JWT tokens will be invalidated (users must re-login)
- Set JWT_SECRET_KEY env var in production for token persistence across restarts
- No database changes required
Fixes: GHSA-gc24-px2r-5qmf
Security: CWE-306 (Missing Authentication), CWE-321 (Hardcoded Crypto Key)
Closes GHSA-gc24-px2r-5qmf
This commit is contained in:
@@ -1,7 +1,10 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import os
|
||||
import secrets
|
||||
from datetime import datetime, timedelta
|
||||
from pathlib import Path
|
||||
from typing import Annotated
|
||||
|
||||
import jwt
|
||||
@@ -19,13 +22,67 @@ from backend.app.models.api_key import APIKey
|
||||
from backend.app.models.settings import Settings
|
||||
from backend.app.models.user import User
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Password hashing
|
||||
# Use pbkdf2_sha256 instead of bcrypt to avoid 72-byte limit and passlib initialization issues
|
||||
# pbkdf2_sha256 is a secure password hashing algorithm without bcrypt's limitations
|
||||
pwd_context = CryptContext(schemes=["pbkdf2_sha256"], deprecated="auto")
|
||||
|
||||
|
||||
def _get_jwt_secret() -> str:
|
||||
"""Get the JWT secret key from environment, file, or generate a new one.
|
||||
|
||||
Priority:
|
||||
1. JWT_SECRET_KEY environment variable
|
||||
2. .jwt_secret file in data directory
|
||||
3. Generate new random secret and save to file
|
||||
|
||||
Returns:
|
||||
The JWT secret key
|
||||
"""
|
||||
# 1. Check environment variable first
|
||||
env_secret = os.environ.get("JWT_SECRET_KEY")
|
||||
if env_secret:
|
||||
logger.info("Using JWT secret from JWT_SECRET_KEY environment variable")
|
||||
return env_secret
|
||||
|
||||
# 2. Check for secret file in data directory
|
||||
data_dir = Path(os.environ.get("BAMBUDDY_DATA_DIR", "/app/data"))
|
||||
secret_file = data_dir / ".jwt_secret"
|
||||
|
||||
if secret_file.exists():
|
||||
try:
|
||||
secret = secret_file.read_text().strip()
|
||||
if secret and len(secret) >= 32:
|
||||
logger.info("Using JWT secret from %s", secret_file)
|
||||
return secret
|
||||
except Exception as e:
|
||||
logger.warning("Failed to read JWT secret file: %s", e)
|
||||
|
||||
# 3. Generate new random secret
|
||||
new_secret = secrets.token_urlsafe(64)
|
||||
|
||||
# Try to save it
|
||||
try:
|
||||
data_dir.mkdir(parents=True, exist_ok=True)
|
||||
secret_file.write_text(new_secret)
|
||||
# Restrict permissions (owner read/write only)
|
||||
secret_file.chmod(0o600)
|
||||
logger.info("Generated new JWT secret and saved to %s", secret_file)
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"Could not save JWT secret to file (%s). "
|
||||
"Secret will be regenerated on restart, invalidating existing tokens. "
|
||||
"Set JWT_SECRET_KEY environment variable for persistence.",
|
||||
e,
|
||||
)
|
||||
|
||||
return new_secret
|
||||
|
||||
|
||||
# JWT settings
|
||||
SECRET_KEY = "bambuddy-secret-key-change-in-production" # TODO: Move to settings/env
|
||||
SECRET_KEY = _get_jwt_secret()
|
||||
ALGORITHM = "HS256"
|
||||
ACCESS_TOKEN_EXPIRE_MINUTES = 60 * 24 * 7 # 7 days
|
||||
|
||||
|
||||
@@ -2537,6 +2537,121 @@ app = FastAPI(
|
||||
lifespan=lifespan,
|
||||
)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Authentication Middleware - Secures ALL API routes by default
|
||||
# =============================================================================
|
||||
# Public routes that don't require authentication even when auth is enabled
|
||||
PUBLIC_API_ROUTES = {
|
||||
# Auth routes needed before login
|
||||
"/api/v1/auth/status",
|
||||
"/api/v1/auth/login",
|
||||
# Version check for updates (no sensitive data)
|
||||
"/api/v1/updates/version",
|
||||
}
|
||||
|
||||
# Route prefixes that are public (for routes with dynamic segments)
|
||||
PUBLIC_API_PREFIXES = [
|
||||
# WebSocket connections handle their own auth
|
||||
"/api/v1/ws",
|
||||
]
|
||||
|
||||
|
||||
@app.middleware("http")
|
||||
async def auth_middleware(request, call_next):
|
||||
"""Enforce authentication on all API routes when auth is enabled.
|
||||
|
||||
This middleware provides defense-in-depth by checking auth at the API gateway level,
|
||||
regardless of whether individual routes have auth dependencies.
|
||||
"""
|
||||
from starlette.responses import JSONResponse
|
||||
|
||||
path = request.url.path
|
||||
|
||||
# Only apply to API routes
|
||||
if not path.startswith("/api/"):
|
||||
return await call_next(request)
|
||||
|
||||
# Allow public routes
|
||||
if path in PUBLIC_API_ROUTES:
|
||||
return await call_next(request)
|
||||
|
||||
# Allow public prefixes
|
||||
for prefix in PUBLIC_API_PREFIXES:
|
||||
if path.startswith(prefix):
|
||||
return await call_next(request)
|
||||
|
||||
# Check if auth is enabled
|
||||
try:
|
||||
async with async_session() as db:
|
||||
from backend.app.core.auth import is_auth_enabled
|
||||
|
||||
auth_enabled = await is_auth_enabled(db)
|
||||
|
||||
if not auth_enabled:
|
||||
# Auth disabled, allow all requests
|
||||
return await call_next(request)
|
||||
except Exception:
|
||||
# If we can't check auth status, allow request (fail open for DB issues)
|
||||
return await call_next(request)
|
||||
|
||||
# Auth is enabled - require valid token
|
||||
auth_header = request.headers.get("Authorization")
|
||||
x_api_key = request.headers.get("X-API-Key")
|
||||
|
||||
# Check for API key auth first
|
||||
if x_api_key or (auth_header and auth_header.startswith("Bearer bb_")):
|
||||
# API key authentication - let the request through to be validated by route handler
|
||||
# API keys are validated per-route since they have different permission levels
|
||||
return await call_next(request)
|
||||
|
||||
# Check for JWT auth
|
||||
if not auth_header or not auth_header.startswith("Bearer "):
|
||||
return JSONResponse(
|
||||
status_code=401,
|
||||
content={"detail": "Authentication required"},
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
|
||||
# Validate JWT token
|
||||
try:
|
||||
import jwt
|
||||
|
||||
from backend.app.core.auth import ALGORITHM, SECRET_KEY
|
||||
|
||||
token = auth_header.replace("Bearer ", "")
|
||||
payload = jwt.decode(token, SECRET_KEY, algorithms=[ALGORITHM])
|
||||
username = payload.get("sub")
|
||||
if not username:
|
||||
raise ValueError("No username in token")
|
||||
|
||||
# Verify user exists and is active
|
||||
async with async_session() as db:
|
||||
from backend.app.core.auth import get_user_by_username
|
||||
|
||||
user = await get_user_by_username(db, username)
|
||||
if not user or not user.is_active:
|
||||
return JSONResponse(
|
||||
status_code=401,
|
||||
content={"detail": "User not found or inactive"},
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
except jwt.ExpiredSignatureError:
|
||||
return JSONResponse(
|
||||
status_code=401,
|
||||
content={"detail": "Token has expired"},
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
except (jwt.InvalidTokenError, ValueError, Exception):
|
||||
return JSONResponse(
|
||||
status_code=401,
|
||||
content={"detail": "Invalid token"},
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
|
||||
return await call_next(request)
|
||||
|
||||
|
||||
# API routes
|
||||
app.include_router(auth.router, prefix=app_settings.api_prefix)
|
||||
app.include_router(users.router, prefix=app_settings.api_prefix)
|
||||
|
||||
@@ -117,10 +117,11 @@ async def async_client(test_engine, db_session) -> AsyncGenerator[AsyncClient, N
|
||||
async def mock_init_printer_connections(db):
|
||||
pass # No-op - don't connect to real printers
|
||||
|
||||
# Also patch the module-level async_session used by services and auth
|
||||
# Also patch the module-level async_session used by services, auth, and middleware
|
||||
with (
|
||||
patch("backend.app.core.database.async_session", test_async_session),
|
||||
patch("backend.app.core.auth.async_session", test_async_session),
|
||||
patch("backend.app.main.async_session", test_async_session),
|
||||
patch("backend.app.main.init_printer_connections", mock_init_printer_connections),
|
||||
):
|
||||
# Seed default groups for tests that need them
|
||||
|
||||
@@ -700,7 +700,10 @@ class TestUserItemsCountAndDeletion(TestOwnershipPermissionsSetup):
|
||||
assert response.status_code == 204
|
||||
|
||||
# Verify archive still exists but is now ownerless
|
||||
archive_response = await async_client.get(f"/api/v1/archives/{archive_id}")
|
||||
archive_response = await async_client.get(
|
||||
f"/api/v1/archives/{archive_id}",
|
||||
headers={"Authorization": f"Bearer {auth_setup['admin_token']}"},
|
||||
)
|
||||
assert archive_response.status_code == 200
|
||||
assert archive_response.json()["created_by_id"] is None
|
||||
|
||||
@@ -736,5 +739,8 @@ class TestUserItemsCountAndDeletion(TestOwnershipPermissionsSetup):
|
||||
assert response.status_code == 204
|
||||
|
||||
# Verify archive was deleted
|
||||
archive_response = await async_client.get(f"/api/v1/archives/{archive_id}")
|
||||
archive_response = await async_client.get(
|
||||
f"/api/v1/archives/{archive_id}",
|
||||
headers={"Authorization": f"Bearer {auth_setup['admin_token']}"},
|
||||
)
|
||||
assert archive_response.status_code == 404
|
||||
|
||||
Reference in New Issue
Block a user