fix(security): WebSocket auth gate + audit-driven hardening sweep

This commit is contained in:
maziggy
2026-06-02 10:02:17 +02:00
parent 9c8df1744d
commit b7d7c82501
32 changed files with 1420 additions and 234 deletions
+39 -15
View File
@@ -25,6 +25,7 @@ from backend.app.core.auth import (
authenticate_user,
authenticate_user_by_email,
create_access_token,
create_websocket_token,
get_current_active_user,
get_password_hash,
get_user_by_email,
@@ -273,7 +274,7 @@ async def setup_auth(request: SetupRequest, db: AsyncSession = Depends(get_db)):
db.add(admin_user)
logger.info("Admin user added to session: %s", request.admin_username)
admin_created = True
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); no user is created on error
await db.rollback()
logger.error("Failed to create admin user: %s", e, exc_info=True)
raise HTTPException(
@@ -294,7 +295,7 @@ async def setup_auth(request: SetupRequest, db: AsyncSession = Depends(get_db)):
return SetupResponse(auth_enabled=request.auth_enabled, admin_created=admin_created)
except HTTPException:
raise
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); setup state stays unchanged
logger.error("Setup error: %s", e, exc_info=True)
await db.rollback()
raise HTTPException(
@@ -339,7 +340,7 @@ async def disable_auth(
await db.commit()
logger.info("Authentication disabled by admin user: %s", user.username)
return {"message": "Authentication disabled successfully", "auth_enabled": False}
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); auth_enabled stays at its prior value
await db.rollback()
logger.error("Failed to disable authentication: %s", e, exc_info=True)
raise HTTPException(
@@ -408,7 +409,7 @@ async def login(raw_request: Request, request: LoginRequest, response: Response,
if user and ldap_user:
# Update email and group mappings on each login
await _sync_ldap_user(db, user, ldap_user, ldap_config)
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: LDAP failure sets ldap_user=None, downstream local-auth path runs with its own credential check (no implicit grant)
import logging
logging.getLogger(__name__).warning("LDAP authentication error, falling back to local: %s", e)
@@ -505,6 +506,29 @@ async def login(raw_request: Request, request: LoginRequest, response: Response,
)
@router.post("/ws-token")
async def mint_websocket_token(
current_user: User | None = RequirePermissionIfAuthEnabled(Permission.WEBSOCKET_CONNECT),
):
"""Mint a short-lived token for ``/api/v1/ws`` connections (GHSA-r2qv follow-up).
The WebSocket endpoint cannot read ``Authorization`` headers from
browsers (the WebSocket handshake does not let JS attach custom
headers), so we use the same opaque-token-in-query-param pattern
as ``/camera/stream`` — the token is minted here behind the standard
permission gate, then appended as ``?token=<value>`` on the
``ws://...`` URL. The WebSocket endpoint validates it *before*
calling ``websocket.accept()``.
Returns ``{"token": <opaque string>}``. The token is valid for 60
minutes; the SPA refreshes it on reconnect if expired. API keys can
mint tokens too — their scope flags decide whether ``WEBSOCKET_CONNECT``
passes via the standard allowlist (``can_read_status`` covers it).
"""
username = current_user.username if current_user is not None else None
return {"token": await create_websocket_token(username)}
@router.get("/me", response_model=UserResponse)
async def get_current_user_info(
credentials: Annotated[HTTPAuthorizationCredentials | None, Depends(security)] = None,
@@ -619,7 +643,7 @@ async def logout(
expires_at = datetime.fromtimestamp(exp, tz=timezone.utc)
try:
await revoke_jti(jti, expires_at, username)
except Exception as exc:
except Exception as exc: # SEC-AUTH-EXC: JTI-revoke failure on logout is logged only; logout removes access, never grants it (token stays valid until natural expiry — degraded but never escalation)
_logger.error("Failed to revoke JTI on logout for user %s: %s", username, exc)
except PyJWTError:
client_ip = _get_client_ip(raw_request)
@@ -664,7 +688,7 @@ async def test_smtp_connection(
logger.info(f"Test email sent successfully to {test_request.test_recipient}")
return TestSMTPResponse(success=True, message="Test email sent successfully")
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: SMTP test diagnostic returns success=False; no auth-relevant outcome (route is admin-gated by SETTINGS_UPDATE upstream)
logger.error("Failed to send test email: %s", e)
return TestSMTPResponse(success=False, message="Failed to send test email")
@@ -698,7 +722,7 @@ async def save_smtp_config(
await db.commit()
logger.info(f"SMTP settings updated by admin user: {current_user.username if current_user else 'anonymous'}")
return {"message": "SMTP settings saved successfully"}
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); SMTP settings unchanged on error
await db.rollback()
logger.error("Failed to save SMTP settings: %s", e)
raise HTTPException(
@@ -743,7 +767,7 @@ async def enable_advanced_auth(
await db.commit()
logger.info(f"Advanced authentication enabled by admin user: {user.username}")
return {"message": "Advanced authentication enabled successfully", "advanced_auth_enabled": True}
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); advanced-auth setting unchanged on error
await db.rollback()
logger.error("Failed to enable advanced authentication: %s", e)
raise HTTPException(
@@ -777,7 +801,7 @@ async def disable_advanced_auth(
await db.commit()
logger.info(f"Advanced authentication disabled by admin user: {user.username}")
return {"message": "Advanced authentication disabled successfully", "advanced_auth_enabled": False}
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); advanced-auth setting unchanged on error
await db.rollback()
logger.error("Failed to disable advanced authentication: %s", e)
raise HTTPException(
@@ -826,7 +850,7 @@ async def _send_reset_email_or_delete_token(
try:
send_email(smtp_settings, to_email, subject, text_body, html_body)
_logger.info("Password reset email sent (%s) to %s", log_label, to_email)
except Exception as exc:
except Exception as exc: # SEC-AUTH-EXC: email-send failure → defensive token cleanup so a stuck token doesn't block re-request; no access granted, just frees future workflow
_logger.error(
"Password reset email failed (%s) to %s — deleting token to unblock re-request: %s",
log_label,
@@ -842,7 +866,7 @@ async def _send_reset_email_or_delete_token(
)
)
await db.commit()
except Exception as db_exc:
except Exception as db_exc: # SEC-AUTH-EXC: nested cleanup failure logged only; no access decision made in this branch (already handling a prior failure)
_logger.error("Failed to delete reset token after send failure: %s", db_exc)
@@ -967,7 +991,7 @@ async def forgot_password(
"forgot_password",
)
_logger.info("Password reset email queued for %s", user.email)
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: forgot-password response is intentionally generic regardless of outcome (user-enumeration defence); email failure does not grant access
_logger.error("Failed to send password reset email: %s", e)
# Don't reveal error to caller for security
@@ -1114,7 +1138,7 @@ async def reset_user_password(
_logger.info("Admin password reset link queued for user '%s' by admin '%s'", user.username, admin_user.username)
return ResetPasswordResponse(message=f"Password reset link sent to {user.email}")
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: rollback + raise 500 (fail-closed); reset token state unchanged on error
await db.rollback()
_logger.error("Failed to send admin password reset for user '%s': %s", user.username, e)
raise HTTPException(
@@ -1354,7 +1378,7 @@ async def search_ldap_directory(
try:
results = search_ldap_users(config, query, limit=25)
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: raise 503 (fail-closed); route gated upstream by USERS_CREATE permission so detail leak is admin-only
_logger.exception("LDAP directory search failed")
# Admin-only endpoint — surface the underlying reason so the operator
# can fix it (auth_middleware already restricted access to USERS_CREATE).
@@ -1430,7 +1454,7 @@ async def provision_ldap_user(
# "username doesn't exist in the directory" in the UI.
try:
ldap_user = lookup_ldap_user(config, username)
except Exception as e:
except Exception as e: # SEC-AUTH-EXC: raise 503 (fail-closed); LDAP provision never succeeds on lookup failure
_logger.exception("LDAP lookup failed during provision")
raise HTTPException(
status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
+117 -19
View File
@@ -1074,20 +1074,73 @@ async def delete_folder(
# ============ External Folder Endpoints ============
# Blocked system directories that cannot be mounted
_BLOCKED_PREFIXES = (
"/proc",
"/sys",
"/dev",
"/run",
"/boot",
"/sbin",
"/bin",
"/usr/sbin",
"/usr/bin",
"/lib",
"/etc",
)
# GHSA-r2qv follow-up (audit finding I1): external-folder mount path uses an
# allowlist of operator-opted-in roots rather than the original denylist of
# system directories. The denylist shape was fail-open-on-growth — anything
# not enumerated (``/data`` containing other users' archives, ``/root``,
# arbitrary NFS/SMB mounts, the Bambuddy ``LOG_DIR``) could be mounted by any
# user with ``LIBRARY_UPLOAD``. The allowlist defaults to empty and is
# extended via the ``BAMBUDDY_EXTERNAL_ROOTS`` env var (colon-separated
# absolute paths). The route is additionally gated on ``SETTINGS_UPDATE``
# (admin scope) rather than ``LIBRARY_UPLOAD`` because mounting host paths
# is an operator-level capability that crosses user boundaries.
# Bambuddy-owned data directories. Hardcode-rejected even if the operator
# tries to add them to ``BAMBUDDY_EXTERNAL_ROOTS`` — mounting these would
# allow reading other users' archives, log files, or the static assets path.
def _bambuddy_reserved_roots() -> tuple[Path, ...]:
"""Resolved Bambuddy-owned directories that may NEVER be mounted as an
external folder regardless of the operator's allowlist.
Resolved at call time because tests patch ``settings.base_dir`` /
``settings.log_dir`` to a temp dir; resolving lazily picks up the
patched values rather than module-import-time values.
"""
from backend.app.core.config import settings as app_settings
reserved = [app_settings.base_dir, app_settings.log_dir, app_settings.static_dir, app_settings.archive_dir]
return tuple(Path(p).resolve() for p in reserved if p is not None)
def _allowed_external_roots() -> tuple[Path, ...]:
"""Parse ``BAMBUDDY_EXTERNAL_ROOTS`` into resolved allowed roots.
Empty env var (the default) means external folders are disabled.
Operators opt in explicitly: ``BAMBUDDY_EXTERNAL_ROOTS=/mnt/library:/srv/3d``
Returns a tuple of resolved ``Path`` objects; entries that don't
resolve to absolute paths are silently dropped (operator error, not
a security boundary). Resolved lazily so tests can monkeypatch.
"""
raw = os.environ.get("BAMBUDDY_EXTERNAL_ROOTS", "")
roots: list[Path] = []
for entry in raw.split(":"):
entry = entry.strip()
if not entry:
continue
try:
resolved = Path(entry).resolve()
except (OSError, RuntimeError): # noqa: BLE001 — operator config error, not a security boundary
continue
if resolved.is_absolute():
roots.append(resolved)
return tuple(roots)
def _path_within(child: Path, parent: Path) -> bool:
"""Return True if ``child`` is ``parent`` or any descendant.
Uses ``Path.relative_to`` semantics (raises ``ValueError`` on miss)
instead of string ``startswith``, which would falsely match
``/data-other`` against ``/data``. ``Path.is_relative_to`` is the
sanctioned form on Python 3.9+; both are available here.
"""
try:
child.relative_to(parent)
except ValueError:
return False
return True
# Supported file extensions for external folder scanning
_SCANNABLE_EXTENSIONS = {
@@ -1108,15 +1161,53 @@ _SCANNABLE_EXTENSIONS = {
def _validate_external_path(path_str: str) -> Path:
"""Validate an external path is safe to mount."""
"""Validate an external path is safe to mount.
Allowlist semantics:
1. Path must be absolute and resolve cleanly (symlink-escape rejected
implicitly by the resolved-startswith check below).
2. Path must fall under one of the roots enumerated in
``BAMBUDDY_EXTERNAL_ROOTS``; empty allowlist (the default)
means external folders are not available on this deployment.
3. Path must NOT fall under any Bambuddy-owned directory (``base_dir``,
``log_dir``, ``static_dir``, ``archive_dir``) — the reserved set
takes precedence over the allowlist, so an operator who accidentally
sets ``BAMBUDDY_EXTERNAL_ROOTS=/`` does not expose ``/data``.
4. Existence + directory-type + readability gates remain.
"""
path = Path(path_str).resolve()
if not path.is_absolute():
raise HTTPException(status_code=400, detail="Path must be absolute")
for prefix in _BLOCKED_PREFIXES:
if str(path).startswith(prefix):
raise HTTPException(status_code=400, detail=f"Cannot mount system directory: {prefix}")
allowed_roots = _allowed_external_roots()
if not allowed_roots:
raise HTTPException(
status_code=400,
detail=(
"External folders are not enabled on this deployment. Ask the "
"operator to set BAMBUDDY_EXTERNAL_ROOTS=<colon-separated paths>."
),
)
# Reserved (Bambuddy-owned) paths are rejected before the allowlist check
# so an over-broad allowlist (e.g. operator set "/" for testing) cannot
# expose Bambuddy's own data dir or log dir.
for reserved in _bambuddy_reserved_roots():
if _path_within(path, reserved):
raise HTTPException(
status_code=400,
detail=f"Cannot mount Bambuddy-managed directory: {reserved}",
)
if not any(_path_within(path, root) for root in allowed_roots):
raise HTTPException(
status_code=400,
detail=(
f"Path '{path}' is not within an allowed external root. "
f"Allowed roots: {', '.join(str(r) for r in allowed_roots)}"
),
)
if not path.exists():
raise HTTPException(status_code=400, detail=f"Path does not exist: {path}")
@@ -1135,7 +1226,14 @@ def _validate_external_path(path_str: str) -> Path:
async def create_external_folder(
data: ExternalFolderCreate,
db: AsyncSession = Depends(get_db),
_: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_UPLOAD)),
# GHSA-r2qv follow-up (I1): elevated from LIBRARY_UPLOAD to SETTINGS_UPDATE.
# Registering a host filesystem path as a Bambuddy library folder is an
# operator-level capability that crosses user boundaries (one user's
# registered external folder is visible to every other user via
# /api/v1/library/folders). LIBRARY_UPLOAD was always the wrong scope —
# SETTINGS_UPDATE is the admin-class gate that already protects every
# other host-affecting setting (SMTP, LDAP, cloud, smart plugs).
_: User | None = Depends(require_permission_if_auth_enabled(Permission.SETTINGS_UPDATE)),
):
"""Create an external folder that points to a host directory."""
resolved = _validate_external_path(data.external_path)
+4 -1
View File
@@ -1,6 +1,7 @@
"""Prometheus metrics endpoint for external monitoring."""
import platform
import secrets
from fastapi import APIRouter, Depends, Header, HTTPException, Response
from sqlalchemy import func, select
@@ -75,7 +76,9 @@ async def get_metrics(
if not authorization.startswith("Bearer "):
raise HTTPException(status_code=401, detail="Bearer token required")
provided_token = authorization[7:] # Remove "Bearer " prefix
if provided_token != token:
# Constant-time comparison closes the byte-by-byte timing oracle that
# plain ``!=`` opens on a LAN-attached attacker (audit finding I2).
if not secrets.compare_digest(provided_token.encode("utf-8"), token.encode("utf-8")):
raise HTTPException(status_code=401, detail="Invalid token")
lines: list[str] = []
+11 -3
View File
@@ -361,12 +361,20 @@ async def get_ui_preferences(db: AsyncSession = Depends(get_db)):
@router.get("/check-ffmpeg")
async def check_ffmpeg():
"""Check if ffmpeg is installed and available."""
async def check_ffmpeg(
_: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_READ),
):
"""Check if ffmpeg is installed and available.
Gated on ``SETTINGS_READ`` (audit finding I4 — the binary path was
leaking the host filesystem layout to unauthenticated callers).
``require_permission_if_auth_enabled`` returns ``None`` only when
auth is disabled (in which case there's no privacy boundary to
enforce); otherwise it raises 401/403 before we get here.
"""
from backend.app.services.camera import get_ffmpeg_path
ffmpeg_path = get_ffmpeg_path()
return {
"installed": ffmpeg_path is not None,
"path": ffmpeg_path,
+78 -6
View File
@@ -1,7 +1,27 @@
"""GHSA-r2qv follow-up — WebSocket auth gate.
Previously ``/api/v1/ws`` accepted *any* network client and immediately
streamed every ``printer_status`` / ``print_start`` / ``print_complete``
/ ``archive_*`` / ``inventory_changed`` broadcast back to it. That is
the GHSA-gc24 shape on a different protocol — anyone who could reach
the HTTP port could subscribe to every printer event in the system.
This endpoint now validates a short-lived token (minted by
``POST /api/v1/auth/ws-token`` behind ``Permission.WEBSOCKET_CONNECT``)
*before* ``websocket.accept()``. When auth is disabled, no token is
required (the legacy SPA-friendly path). The token is reused across
reconnects within its 60-minute window so a brief network blip does
not require a round-trip to the auth router.
"""
from __future__ import annotations
import logging
from fastapi import APIRouter, WebSocket, WebSocketDisconnect
from fastapi import APIRouter, Query, WebSocket, WebSocketDisconnect
from backend.app.core.auth import is_auth_enabled, verify_websocket_token
from backend.app.core.database import async_session
from backend.app.core.websocket import ws_manager
from backend.app.services.background_dispatch import background_dispatch
from backend.app.services.printer_manager import printer_manager, printer_state_to_dict
@@ -9,16 +29,68 @@ from backend.app.services.printer_manager import printer_manager, printer_state_
logger = logging.getLogger(__name__)
router = APIRouter()
# 4401 mirrors the WebSocket "unauthorised" application close code
# convention used by Sec-WebSocket-Protocol authors (private-use range
# is 4000-4999 per RFC 6455). The SPA distinguishes 4401 from network
# drops and refetches a token instead of retrying with the old one.
_WS_CLOSE_UNAUTHORIZED = 4401
@router.websocket("/ws")
async def websocket_endpoint(websocket: WebSocket):
"""WebSocket endpoint for real-time updates."""
logger.info("WebSocket client connecting...")
async def websocket_endpoint(websocket: WebSocket, token: str | None = Query(default=None)) -> None:
"""WebSocket endpoint for real-time updates.
Connection auth (GHSA-r2qv follow-up):
- Auth disabled → connect without a token, identical to the prior
behaviour (single-user / local-network deployments).
- Auth enabled → ``?token=<value>`` query param must hold an
unexpired token minted via ``POST /api/v1/auth/ws-token``.
Missing / invalid / expired token → ``close(code=4401)`` *before*
``accept()`` so no ``ws_manager.broadcast`` ever reaches the
caller (broadcasts walk ``active_connections`` blindly — letting
an unauthenticated socket into that list is a fan-out leak).
The auth check is fail-closed at every error path: a DB exception
while reading the ``auth_enabled`` setting closes the connection
rather than admitting the caller.
"""
# Authenticate before accept() so an unauth caller never lands in
# ws_manager.active_connections (where broadcasts blindly fan out).
try:
async with async_session() as db:
auth_required = await is_auth_enabled(db)
except Exception: # SEC-AUTH-EXC: DB failure on auth probe → fail-closed (refuse connect), matches is_auth_enabled itself which returns True on error
logger.error("WebSocket auth probe failed; refusing connection", exc_info=True)
await websocket.close(code=_WS_CLOSE_UNAUTHORIZED)
return
principal: str | None = None
if auth_required:
if not token:
logger.info("WebSocket connect refused: no token (auth enabled)")
await websocket.close(code=_WS_CLOSE_UNAUTHORIZED)
return
principal = await verify_websocket_token(token)
if principal is None:
logger.info("WebSocket connect refused: invalid or expired token")
await websocket.close(code=_WS_CLOSE_UNAUTHORIZED)
return
# Token verified (or auth disabled); now safe to admit the connection.
logger.info("WebSocket client connecting (principal=%s)", principal if principal else "<anonymous>")
await ws_manager.connect(websocket)
# Stash on connection state for any future per-message permission
# logic; today the message handlers are read-only and only respond
# to the requesting socket, so the stash is informational. The
# explicit attribute (rather than a side dict) means a future
# ``broadcast_to_principal()`` helper can filter on it without
# touching every call site.
websocket.state.bambuddy_principal = principal
logger.info("WebSocket client connected")
try:
# Send initial status of all printers
# Send initial status of all printers.
statuses = printer_manager.get_all_statuses()
for printer_id, state in statuses.items():
await websocket.send_json(
@@ -39,7 +111,7 @@ async def websocket_endpoint(websocket: WebSocket):
)
logger.info("Sent initial status for %s printers", len(statuses))
# Keep connection alive and handle incoming messages
# Keep connection alive and handle incoming messages.
while True:
data = await websocket.receive_json()