Merge remote-tracking branch 'origin/0.2.4.4' into resolve-pr-1566

# Conflicts:
#	.github/workflows/ci.yml
#	Dockerfile.test

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 All notable changes to Bambuddy will be documented in this file.
 
+## [0.2.4.4] - 2026-05-30
+
+### Security
+
+- **Fail-open authentication bypass on database errors — unauthenticated access to every protected endpoint during a forced DB-exception window ([GHSA-6mf4-q26m-47pv](https://github.com/maziggy/bambuddy/security/advisories/GHSA-6mf4-q26m-47pv), CVSS 9.8 critical, reported by @wondercrash)** — Two functions in the auth path caught every exception and returned the "allow" answer instead of denying the request: `is_auth_enabled` in `backend/app/core/auth.py:473` (returned `False`, treating "DB query raised" as "auth is disabled") and the global `auth_middleware` in `backend/app/main.py:5590` (caught everything and called `await call_next(request)` with a comment that explicitly said "fail open for DB issues"). An attacker who could trigger any exception during the auth probe — the reporter's documented PoC floods `/api/v1/auth/login` until the process exhausts its file-descriptor budget and the next SQLite `connect()` raises — could then hit any protected endpoint during that fail-open window with no token. CWE-636 (Failing Open) / CWE-755 (Improper Handling of Exceptional Conditions). Affected versions `>= 0.1.6`. **Impact during the window**: create a persistent admin account or API key, download the database backup (hashed passwords + encryption keys + printer access codes + MFA secrets), read/modify settings, control printers. **Fix**: `is_auth_enabled` now only returns `False` for the legitimate "settings row absent" case (`scalar_one_or_none()` returns `None` → system was never configured for auth); any actual exception propagates so the caller can deny the request. `auth_middleware` returns `503 Service Unavailable` on any probe failure instead of letting the request through. The principle applied throughout: a failure to verify the auth state means the request is denied, not granted. **Regression tests** in `backend/tests/unit/test_auth_fail_closed.py` pin the four contracts: `is_auth_enabled` propagates DB exceptions, returns `False` for the no-row case, returns `True` for `value=true`, returns `False` for `value=false`. An existing security test (`test_security.py::test_status_returns_500_on_db_error`) was renamed to `test_status_returns_503_on_db_error` and updated to accept either 500 or 503 (both fail-closed) while explicitly verifying the SQLAlchemy detail string doesn't leak in the response body. **Codebase audit**: grepped every `except Exception` in `backend/app/core/auth.py` and `backend/app/core/permissions.py` for the same shape; `_validate_api_key` catches but returns `None` which leads to a 401 downstream (fail-closed), `is_advanced_auth_enabled` in `backend/app/api/routes/auth.py` already propagates correctly, `permissions.py` has no catch-alls — no other auth-decision predicate carries this anti-pattern.
+
 ## [0.2.4.3] - 2026-05-24
 
 ### Added

backend/app/core/auth.py

@@ -471,16 +471,27 @@ async def authenticate_user_by_email(db: AsyncSession, email: str, password: str
 
 
 async def is_auth_enabled(db: AsyncSession) -> bool:
-    """Check if authentication is enabled."""
-    try:
-        result = await db.execute(select(Settings).where(Settings.key == "auth_enabled"))
-        setting = result.scalar_one_or_none()
-        if setting is None:
-            return False
-        return setting.value.lower() == "true"
-    except Exception:
-        # If settings table doesn't exist or query fails, assume auth is disabled
+    """Check if authentication is enabled.
+
+    Fails CLOSED on database errors. A previous version of this function
+    caught every exception and returned False — silently treating an
+    unavailable database as "auth is disabled" and granting unauthenticated
+    access to every endpoint that called it (GHSA-6mf4-q26m-47pv, CVSS 9.8).
+    An attacker could trigger that fail-open by flooding /api/v1/auth/login
+    to exhaust the process's file-descriptor budget, then hit a protected
+    endpoint during the window where the next DB op raised.
+
+    Legitimate "auth was never configured" still returns False — the
+    settings row is simply absent, ``scalar_one_or_none`` returns None,
+    no exception. Any OTHER failure (connection error, fd exhaustion,
+    schema mismatch, …) propagates so the caller can deny the request
+    (503 / 500). Fail-closed is the only safe default for an auth probe.
+    """
+    result = await db.execute(select(Settings).where(Settings.key == "auth_enabled"))
+    setting = result.scalar_one_or_none()
+    if setting is None:
         return False
+    return setting.value.lower() == "true"
 
 
 async def _user_from_api_key(db: AsyncSession, api_key: APIKey) -> User | None:

backend/app/core/config.py

@@ -6,7 +6,7 @@ from pathlib import Path
 from pydantic_settings import BaseSettings
 
 # Application version - single source of truth
-APP_VERSION = "0.2.4.3"
+APP_VERSION = "0.2.4.4"
 GITHUB_REPO = "maziggy/bambuddy"
 BUG_REPORT_RELAY_URL = os.environ.get("BUG_REPORT_RELAY_URL", "https://bambuddy.cool/api/bug-report")
 

backend/app/main.py

@@ -5301,7 +5301,10 @@ async def auth_middleware(request, call_next):
         if pattern in path:
             return await call_next(request)
 
-    # Check if auth is enabled
+    # Check if auth is enabled. Fail CLOSED on any exception during the
+    # probe — GHSA-6mf4-q26m-47pv: the previous fail-open path here let
+    # an attacker who could force a DB exception (e.g. file-descriptor
+    # exhaustion via login flood) bypass auth on every protected endpoint.
     try:
         async with async_session() as db:
             from backend.app.core.auth import is_auth_enabled
@@ -5312,8 +5315,11 @@ async def auth_middleware(request, call_next):
             # 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)
+        logging.getLogger(__name__).exception("auth_middleware: failing closed on auth-probe error from %s", path)
+        return JSONResponse(
+            status_code=503,
+            content={"detail": "Authentication service temporarily unavailable"},
+        )
 
     # Auth is enabled - require valid token
     auth_header = request.headers.get("Authorization")

backend/tests/integration/test_security.py

@@ -1594,8 +1594,19 @@ class TestEncryptionStatusEndpoint:
 
     @pytest.mark.asyncio
     @pytest.mark.integration
-    async def test_status_returns_500_on_db_error(self, async_client, monkeypatch):
-        """A8: SQLAlchemyError during count queries → 500 with static message."""
+    async def test_status_returns_503_on_db_error(self, async_client, monkeypatch):
+        """A8: a DB failure during the request must NOT leak internal detail
+        and must NOT silently succeed.
+
+        Post-GHSA-6mf4-q26m-47pv: the auth middleware's ``is_auth_enabled``
+        probe runs its own DB query before the route is dispatched. Patching
+        ``AsyncSession.execute`` to raise now trips the middleware first and
+        the request fails closed with 503 — a stronger guarantee than the
+        previous route-level 500, because under the old fail-open the
+        middleware would have proceeded to dispatch the route unauthenticated.
+        Either status would be acceptable; the assertion here pins the
+        defense-in-depth posture (request denied, no leak).
+        """
         from unittest.mock import AsyncMock
 
         from sqlalchemy.exc import SQLAlchemyError
@@ -1608,8 +1619,11 @@ class TestEncryptionStatusEndpoint:
         monkeypatch.setattr("sqlalchemy.ext.asyncio.AsyncSession.execute", AsyncMock(side_effect=_raise))
 
         resp = await async_client.get(self.STATUS_URL, headers={"Authorization": f"Bearer {token}"})
-        assert resp.status_code == 500
-        assert "encryption status" in resp.json().get("detail", "").lower()
+        assert resp.status_code in (500, 503)
+        # Either layer's error message must not leak the SQLAlchemy details.
+        body = resp.json().get("detail", "").lower()
+        assert "simulated" not in body
+        assert "sqlalchemy" not in body
 
     @pytest.mark.asyncio
     @pytest.mark.integration

backend/tests/unit/test_auth_fail_closed.py

@@ -0,0 +1,81 @@
+"""Regression tests for the GHSA-6mf4-q26m-47pv fail-open auth bypass.
+
+The previous version of ``is_auth_enabled`` caught every exception and
+returned False (auth disabled). An attacker could trigger a DB-side
+exception — the documented PoC exhausts file descriptors via a flood on
+``/api/v1/auth/login`` until the next SQLite ``connect`` raises — and then
+hit any protected endpoint during that fail-open window with no token at
+all. Severity CVSS 9.8.
+
+These tests pin the fail-closed contract:
+
+1. ``is_auth_enabled`` propagates any DB exception (instead of swallowing
+   it and returning False).
+2. The "no settings row" path still returns False (auth was legitimately
+   never configured).
+3. ``setting.value == "true"`` still returns True.
+"""
+
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+
+from backend.app.core.auth import is_auth_enabled
+
+
+@pytest.mark.asyncio
+async def test_is_auth_enabled_propagates_db_exception_instead_of_failing_open():
+    """The core regression for GHSA-6mf4-q26m-47pv. A DB error during the
+    auth-enabled probe must propagate — fail closed — instead of returning
+    False and treating the system as auth-disabled."""
+
+    db = AsyncMock()
+    db.execute = AsyncMock(side_effect=OSError("simulated file-descriptor exhaustion"))
+
+    with pytest.raises(OSError, match="simulated file-descriptor exhaustion"):
+        await is_auth_enabled(db)
+
+
+@pytest.mark.asyncio
+async def test_is_auth_enabled_returns_false_when_settings_row_absent():
+    """Legitimate 'auth was never configured' path: the settings row simply
+    does not exist. ``scalar_one_or_none`` returns None, no exception, and
+    the function returns False — system is auth-disabled by configuration,
+    not because the DB blew up."""
+
+    result = MagicMock()
+    result.scalar_one_or_none = MagicMock(return_value=None)
+    db = AsyncMock()
+    db.execute = AsyncMock(return_value=result)
+
+    assert await is_auth_enabled(db) is False
+
+
+@pytest.mark.asyncio
+async def test_is_auth_enabled_returns_true_when_setting_value_is_true():
+    """Happy path: the settings row exists and its value is "true" → auth
+    is enabled and the caller must require credentials."""
+
+    setting = MagicMock()
+    setting.value = "true"
+    result = MagicMock()
+    result.scalar_one_or_none = MagicMock(return_value=setting)
+    db = AsyncMock()
+    db.execute = AsyncMock(return_value=result)
+
+    assert await is_auth_enabled(db) is True
+
+
+@pytest.mark.asyncio
+async def test_is_auth_enabled_returns_false_when_setting_value_is_false():
+    """Happy path: the settings row exists and its value is "false" → auth
+    is disabled by configuration (legitimate, not exception)."""
+
+    setting = MagicMock()
+    setting.value = "false"
+    result = MagicMock()
+    result.scalar_one_or_none = MagicMock(return_value=setting)
+    db = AsyncMock()
+    db.execute = AsyncMock(return_value=result)
+
+    assert await is_auth_enabled(db) is False