diff --git a/CHANGELOG.md b/CHANGELOG.md index 26f159656..74e4d7f34 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/backend/app/core/auth.py b/backend/app/core/auth.py index 818efe0f3..10b5840ca 100644 --- a/backend/app/core/auth.py +++ b/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: diff --git a/backend/app/core/config.py b/backend/app/core/config.py index ae5e49110..5deb530ad 100644 --- a/backend/app/core/config.py +++ b/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") diff --git a/backend/app/main.py b/backend/app/main.py index 4ee71179d..b104852aa 100644 --- a/backend/app/main.py +++ b/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") diff --git a/backend/tests/integration/test_security.py b/backend/tests/integration/test_security.py index fae82caf2..cba87af71 100644 --- a/backend/tests/integration/test_security.py +++ b/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 diff --git a/backend/tests/unit/test_auth_fail_closed.py b/backend/tests/unit/test_auth_fail_closed.py new file mode 100644 index 000000000..fa25fe74c --- /dev/null +++ b/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