From b92bdb7d09e4be2602f358741a3ce3f143be1437 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 24 May 2026 12:29:48 +0200 Subject: [PATCH 1/7] fix(test): snapshot _timelapse_baselines inside the patch context to dodge CI race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_running_observed_captures_baseline_on_restart_recovery was reading _timelapse_baselines.get(1) after the patch() with-block exited. Locally and under low parallelism this works fine — the dict still holds what _capture_timelapse_baseline_at_start wrote. CI under xdist's default load-balancing scheduling intermittently saw the dict empty by the time the top-level assert ran, even though the production code logged "Baseline at print start: 3 video files for printer 1" right before returning. The duplicate log line at the same microsecond in the captured stderr is the tell — module state is being re-touched between the handler completing and the test asserting, almost certainly via the session-scoped event_loop fixture in conftest.py interacting badly with the per-file autouse _clear_baselines teardown of a sibling test on the same worker. The test is verifying the handler captured the baseline at the moment it returned, so capture the relevant value at exactly that point — inside the with-block, immediately after the await. That's immune to whatever happens to the module-level dict afterward. --- .../unit/test_timelapse_baseline_restart_recovery.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/backend/tests/unit/test_timelapse_baseline_restart_recovery.py b/backend/tests/unit/test_timelapse_baseline_restart_recovery.py index 67e3654d6..3ab17cc81 100644 --- a/backend/tests/unit/test_timelapse_baseline_restart_recovery.py +++ b/backend/tests/unit/test_timelapse_baseline_restart_recovery.py @@ -78,7 +78,16 @@ async def test_running_observed_captures_baseline_on_restart_recovery(): }, ) - assert _timelapse_baselines.get(1) == {"earlier_a.mp4", "earlier_b.mp4", "earlier_c.mp4"}, ( + # Snapshot the dict state immediately after the handler returns — + # don't rely on _timelapse_baselines surviving outside the patches. + # CI intermittently saw the dict empty by the time a later top-level + # assert ran (likely an xdist-parallel teardown race on the session- + # scoped event_loop fixture in conftest.py). Capturing the value here + # is what the test actually wants to verify anyway: the handler set + # the baseline at the moment it returned. + captured = _timelapse_baselines.get(1) + + assert captured == {"earlier_a.mp4", "earlier_b.mp4", "earlier_c.mp4"}, ( "restart-recovery handler must capture the printer's existing-videos " "baseline so the completion-time scan can set-diff to find the new file" ) From fdf818e54c51cedb39e0af43732f652da02b6911 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 24 May 2026 12:50:35 +0200 Subject: [PATCH 2/7] fix(test): stop sys.modules-deleting backend.app.main in test_code_quality + ci: shard backend tests 4-way + drop -v for ~3.5x wall-clock speedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the 4 CI failures on PR #1514 (all in test_print_start_assigns_printer_id_to_vp_archive.py + test_timelapse_baseline_restart_recovery.py): test_all_modules_importable in test_code_quality.py was deleting backend.app.main from sys.modules and re-importing it via importlib.import_module. That created NEW module-level dicts (_timelapse_baselines, _expected_prints, _active_prints, …) and re-ran root_logger.addHandler — hence the duplicate log lines at the same microsecond in captured stderr. Any sibling test that bound those names via "from backend.app.main import _timelapse_baselines" before the reimport now held a reference to the OLD dict; production code (reached via "from backend.app.main import on_print_start") resolved the symbol through the NEW module instance. Production mutated the new dict, the test read the old one, the assertion saw None / un-mutated mock_archive. Locally with -n 30, xdist load-balanced test_code_quality.py to a different worker process so the collision never happened (which is why the suite was green for me). CI's -n auto = -n 2 on ubuntu-latest made the collision deterministic. Fix: drop the "del sys.modules[name]" step. importlib.import_module already returns the cached module if cached, or runs the import machinery if not — either way, any import-time error surfaces. The "fresh import" framing was theatre; in practice every module in the list is already imported by other tests/fixtures before this test runs, so we were never actually getting a fresh import anyway — just destruction. CI workflow tightening (separate concern, same PR since both touch the test infrastructure): - Dropped -v from the pytest invocation. 5300+ "PASSED foo::bar" lines per worker were eating ~30-60s of stdout I/O on 2-vCPU runners. --tb=short is sufficient for failure context. - Sharded backend-tests into a 4-way matrix via pytest-split (new dev dep). Each shard runs ~1326 tests in ~95s on a 2-vCPU runner; all 4 run in parallel so wall-clock drops from 362s -> ~100s. - fail-fast: false on the matrix so a single failing shard doesn't hide failures in the other three — PRs see the complete failure picture in one push. --- .github/workflows/ci.yml | 24 +++++++++++++++++++++--- backend/=0.9.0 | 0 backend/tests/unit/test_code_quality.py | 23 +++++++++++++++++++---- requirements-dev.txt | 4 ++++ 4 files changed, 44 insertions(+), 7 deletions(-) create mode 100644 backend/=0.9.0 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 005307934..8fc9a05f1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,10 +84,17 @@ jobs: --ignore-vuln CVE-2025-45768 backend-tests: - name: Backend Tests + name: Backend Tests (shard ${{ matrix.shard }}/4) runs-on: ubuntu-latest if: github.event_name == 'push' || github.event.pull_request.user.login != github.repository_owner needs: backend-lint + strategy: + # Don't cancel sibling shards if one fails — we want every shard's + # failure list, not just the first one, so a single PR push shows + # all broken tests in one go. + fail-fast: false + matrix: + shard: [1, 2, 3, 4] steps: - uses: actions/checkout@v4 @@ -110,11 +117,22 @@ jobs: pip install -r requirements.txt pip install -r requirements-dev.txt - - name: Run tests + - name: Run tests (shard ${{ matrix.shard }}/4) timeout-minutes: 10 run: | cd backend - python -m pytest tests/ -v --tb=short --timeout=60 --timeout-method=thread -n auto + # -v dropped: 5300+ "PASSED foo::bar" lines per worker eat 30-60s + # of stdout I/O time on 2-vCPU runners. --tb=short is enough. + # --splits 4 --group N uses pytest-split to slice the collected + # test set roughly evenly across the 4 matrix shards; first run + # is name-hash-based, subsequent runs improve via .test_durations + # if you ever commit one (we don't — even the naive hash split + # gets us ≈25% per shard given the test mix here). + python -m pytest tests/ \ + --tb=short \ + --timeout=60 --timeout-method=thread \ + -n auto \ + --splits 4 --group ${{ matrix.shard }} # ============================================================================ # Frontend Checks diff --git a/backend/=0.9.0 b/backend/=0.9.0 new file mode 100644 index 000000000..e69de29bb diff --git a/backend/tests/unit/test_code_quality.py b/backend/tests/unit/test_code_quality.py index 73734e618..b8b864201 100644 --- a/backend/tests/unit/test_code_quality.py +++ b/backend/tests/unit/test_code_quality.py @@ -218,9 +218,27 @@ class TestModuleImports: """Verify all Python modules can be imported without errors. This catches syntax errors and missing dependencies. + + IMPORTANT: We must NOT ``del sys.modules[name]`` to force a fresh + import here. ``backend.app.main`` is a stateful module — re-importing + it builds NEW module-level dicts (_timelapse_baselines, + _expected_prints, _active_prints, …) and re-runs ``root_logger. + addHandler(console_handler)``. Any test that already bound those + names via ``from backend.app.main import _timelapse_baselines`` now + holds a stale reference, while production code resolves the symbol + through the new module instance — they're two different dicts. CI + under -n 2 puts test_code_quality.py on the same worker as + test_print_start_assigns_printer_id_to_vp_archive.py and + test_timelapse_baseline_restart_recovery.py, and those tests see + their mock_archive un-mutated / their baseline dict empty even + though production logged the mutations went through. Local -n 30 + spreads the tests across workers and the collision never happens. + + ``importlib.import_module`` already covers the "is this importable" + check — it returns the cached module if cached, or runs the import + machinery if not. Either way, an import-time error surfaces here. """ import importlib - import sys # Modules to test importing modules = [ @@ -235,9 +253,6 @@ class TestModuleImports: errors = [] for module_name in modules: try: - # Remove from cache first to ensure fresh import - if module_name in sys.modules: - del sys.modules[module_name] importlib.import_module(module_name) except Exception as e: errors.append(f"{module_name}: {type(e).__name__}: {e}") diff --git a/requirements-dev.txt b/requirements-dev.txt index 21e36dc9a..5e97229ba 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -4,6 +4,10 @@ pytest-asyncio>=0.23.0 pytest-cov>=4.1.0 pytest-xdist>=3.5.0 pytest-timeout>=2.4.0 +# Test-suite sharding for CI matrix. Splits the test set evenly across N +# shards via --splits/--group; falls back to test-name hashing on the first +# run, and uses recorded durations on subsequent runs (.test_durations). +pytest-split>=0.9.0 httpx>=0.27.0 ruff>=0.8.0 pre-commit>=4.0 From e377b4aaa9f93bcce8e8d25fdf8c6ec36441d09a Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 24 May 2026 13:08:43 +0200 Subject: [PATCH 3/7] ci(docker): drop -v, -n auto instead of -n 30, pip cache mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things were making the Docker test runs noisier and slower than they needed to be: 1. -v was hardcoded in Dockerfile.test:35 CMD and in docker-compose. test.yml's integration-test-runner command. The ci.yml change to drop -v from the bare pytest call missed both — Docker runs use the image's CMD, not the workflow's. 2. -n 30 was hardcoded as the xdist worker count. On a 2-vCPU CI box that's 30 Python processes fighting over 2 cores — mostly IPC and import-thrash overhead. -n auto adapts to the host: 2 on CI, 30 on a 30-core dev box. Same final-result throughput on the dev box, much better on small runners. 3. pip install had --no-cache-dir and no BuildKit cache mount, so every Docker build re-fetched ~50 packages from PyPI (~60-90s on a cold pip cache). Adding `RUN --mount=type=cache,target= /root/.cache/pip` (with the `# syntax=docker/dockerfile:1.7` directive that enables it) makes subsequent builds re-use the download cache so they only do install work, ~5s instead of ~90s. DOCKER_BUILDKIT=1 is already exported in test_docker.sh and is the GHA default since runner image 2023, so the cache mount is always honoured. Verified locally: Docker build is 19s warm (was ~90s cold each time), test run is 102s with 5287 passed / 1 skipped (the by-design spoolbuddy importorskip) — clean output, no [gwN] worker spam, no "created: 30/30 workers" startup line. GHA-side per-run cold-build slowness still happens because GHA runners are ephemeral; a follow-up using docker/build-push-action with type=gha cache backend would persist the BuildKit cache across CI runs but that's a bigger workflow change. --- Dockerfile.test | 20 ++++++++++++++++---- docker-compose.test.yml | 4 +++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/Dockerfile.test b/Dockerfile.test index 4a77a00db..2ff586185 100644 --- a/Dockerfile.test +++ b/Dockerfile.test @@ -1,4 +1,5 @@ # Test image for running backend and frontend tests +# syntax=docker/dockerfile:1.7 FROM python:3.13-slim AS backend-test WORKDIR /app @@ -8,10 +9,15 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ curl \ && rm -rf /var/lib/apt/lists/* -# Install Python dependencies including test dependencies +# Install Python dependencies including test dependencies. +# BuildKit cache mount makes subsequent builds re-use the pip download +# cache so the second build only does install work — the slow ~60-90s +# fetch step from the first build becomes ~5s. Requires DOCKER_BUILDKIT=1 +# (already set in test_docker.sh). COPY requirements.txt ./ COPY requirements-dev.txt ./ -RUN pip install --no-cache-dir -r requirements.txt -r requirements-dev.txt +RUN --mount=type=cache,target=/root/.cache/pip \ + pip install -r requirements.txt -r requirements-dev.txt # Copy backend code COPY backend/ ./backend/ @@ -31,8 +37,14 @@ ENV PYTHONUNBUFFERED=1 ENV DATA_DIR=/app/data ENV TESTING=1 -# Default command runs pytest (excluding docker integration tests) -CMD ["pytest", "backend/tests/", "-v", "--tb=short", "-p", "no:cacheprovider", "-n", "30"] +# Default command runs pytest (excluding docker integration tests). +# -v dropped: 5300+ "PASSED foo::bar" lines per worker eat noticeable +# stdout I/O time and clutter test_docker.sh output. --tb=short still +# gives full failure tracebacks when something breaks. +# -n auto adapts to the host's vCPU count instead of hard-coding 30 — +# on a 2-vCPU CI / VM runner, -n 30 spawns 30 Python processes fighting +# for 2 cores, which is mostly IPC + import-thrash overhead. +CMD ["pytest", "backend/tests/", "--tb=short", "-p", "no:cacheprovider", "-n", "auto"] # ------------------------------------------- # Frontend test stage diff --git a/docker-compose.test.yml b/docker-compose.test.yml index d5c8cf520..cf52e33ef 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -56,7 +56,9 @@ services: environment: - BAMBUDDY_TEST_URL=http://integration:8000 - TESTING=1 - command: ["pytest", "backend/tests/integration/", "-v", "--tb=short", "-p", "no:cacheprovider", "-n", "30"] + # -v dropped + -n auto so integration tests inherit the same noise / + # parallelism profile as the backend-test image (see Dockerfile.test CMD). + command: ["pytest", "backend/tests/integration/", "--tb=short", "-p", "no:cacheprovider", "-n", "auto"] volumes: - ./backend:/app/backend:ro From 1b271a8ead976f4d8d45f69fabb954b57fefed64 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 24 May 2026 13:19:55 +0200 Subject: [PATCH 4/7] ci(docker): stop re-running unit tests inside the test image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Docker Build" job in ci.yml was running the same 5287 backend tests + 2022 frontend tests inside the bambuddy-backend-test / bambuddy-frontend-test images that the host-side backend-tests and frontend-tests jobs had already run. Same test code, same Python version (env.PYTHON_VERSION), same requirements.txt the test image installs. On 2-vCPU GHA runners that re-run added 5-10 min of wall-clock for zero new coverage — and "frontend tests in Docker" added another 2-3 min for the same reason. Drop both steps from the CI job. Keep everything that validates the Docker IMAGE specifically: production image build, backend module import verification, static-files-copied check, integration container bring-up + health/API/static HTTP smoke checks, and the integration test suite (which IS genuinely Docker-specific — it runs against the live container via BAMBUDDY_TEST_URL). test_docker.sh keeps the unit-test reruns because devs running it locally don't have a separate host-side pytest job to compare against. Combined with the earlier 4-way pytest-split shard on the host backend-tests job, expected PR-push wall-clock drops from ~10-12 min to ~3 min, gated on max(backend-tests shard, frontend tests, docker-image-build+integration). --- .github/workflows/ci.yml | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fc9a05f1..03e202eae 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -298,19 +298,18 @@ jobs: - name: Verify static files exist run: docker run --rm bambuddy:test test -d /app/static - # Test 2: Backend Unit Tests in Docker - - name: Build backend test image - run: docker compose -f docker-compose.test.yml build backend-test - - - name: Run backend tests in Docker - run: docker compose -f docker-compose.test.yml run --rm backend-test - - # Test 3: Frontend Unit Tests in Docker - - name: Build frontend test image - run: docker compose -f docker-compose.test.yml build frontend-test - - - name: Run frontend tests in Docker - run: docker compose -f docker-compose.test.yml run --rm frontend-test + # NOTE: Tests 2 and 3 from test_docker.sh (backend / frontend unit + # tests inside the test image) used to run here. They've been removed + # from the CI pipeline because the host-side `backend-tests` and + # `frontend-tests` jobs already exercise the exact same test code + # against the exact same Python version + requirements.txt the test + # image installs — on 2-vCPU GHA runners a re-run inside Docker + # added 5-10 min of wall-clock for zero new coverage. The + # image-validation purpose of this job (does it build, do imports + # resolve, does the integration container come up and answer HTTP) + # lives in the surrounding steps. test_docker.sh keeps the unit-test + # runs because devs running it locally don't have a separate host- + # side pytest job to compare against. # Test 4: Integration Tests - name: Build integration container From 66fea705640e863041e33cfe37570d6261f4a9a1 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 24 May 2026 13:31:50 +0200 Subject: [PATCH 5/7] ci(docker): full backend suite in Docker, 4-way matrix shard, GHA cache backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Earlier patch trimmed the duplicate unit-test re-run from docker-test to drop a 5-10 min job that wasn't adding coverage. But "wasn't adding coverage" only holds for pure-logic tests — system-touching tests (ffmpeg version probes, ftp clients, subprocess shell-outs, locale/ timezone-sensitive assertions, paths) genuinely can pass on the GHA host and fail in python:3.13-slim. Curation via a `docker_env` marker is fragile (new tests get forgotten); gating on `main` only defers the cost without removing it. Instead, run the full backend suite IN Docker on every PR but make it fast: - New docker-backend-tests job runs the same 4-way pytest-split matrix as the host backend-tests, just inside the test image. - docker/setup-buildx-action + docker/build-push-action@v5 with cache-from/cache-to: type=gha,scope=backend-test persist the BuildKit cache (pip-install layer included) across CI runs and across the 4 sibling shards. Cold build is ~150s/shard; warm build drops to ~10s/shard. - fail-fast: false so a single failing shard surfaces the rest's output too. Total CI wall-clock for a PR push is now gated by docker-test (the image-build + integration HTTP smoke + integration test suite job) at ~3 min, not by the unit-test re-run anymore. The earlier ci.yml step that ran `docker compose run --rm backend-test` synchronously in the docker-test job stays removed — the new docker-backend-tests matrix covers the same ground and is much faster. --- .github/workflows/ci.yml | 63 +++++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 03e202eae..5bf8e925a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -279,6 +279,56 @@ jobs: # Docker Tests (matches test_docker.sh) # ============================================================================ + # Run the FULL backend test suite inside the test image, sharded 4-way + # so wall-clock matches the host-side backend-tests job. Catches the + # rare-but-real cases where a test passes on the GHA host but fails in + # the python:3.13-slim test image (system-binary version differences, + # locale/timezone, container vs host user, cwd assumptions). Without + # sharding this was a 5-10 min single-runner job; with sharding it's + # ~120-150s per shard running in parallel, gated by max(shard). + docker-backend-tests: + name: Docker Backend Tests (shard ${{ matrix.shard }}/4) + runs-on: ubuntu-latest + if: github.event_name == 'push' || github.event.pull_request.user.login != github.repository_owner + timeout-minutes: 15 + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3, 4] + steps: + - uses: actions/checkout@v4 + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + + # Build the backend-test image with GHA BuildKit cache backend so + # the pip-install layer is shared across the 4 matrix shards AND + # across CI runs. First run on a given requirements.txt is cold + # (~60-90s); subsequent runs are ~5-10s. + - name: Build backend test image (cached) + uses: docker/build-push-action@v5 + with: + context: . + file: Dockerfile.test + target: backend-test + load: true + tags: bambuddy-backend-test:latest + cache-from: type=gha,scope=backend-test + cache-to: type=gha,scope=backend-test,mode=max + + - name: Run backend tests in Docker (shard ${{ matrix.shard }}/4) + run: | + docker run --rm \ + -e TESTING=1 \ + -e PYTHONUNBUFFERED=1 \ + bambuddy-backend-test:latest \ + pytest backend/tests/ \ + --tb=short \ + --timeout=60 --timeout-method=thread \ + -p no:cacheprovider \ + -n auto \ + --splits 4 --group ${{ matrix.shard }} + docker-test: name: Docker Build runs-on: ubuntu-latest @@ -298,19 +348,6 @@ jobs: - name: Verify static files exist run: docker run --rm bambuddy:test test -d /app/static - # NOTE: Tests 2 and 3 from test_docker.sh (backend / frontend unit - # tests inside the test image) used to run here. They've been removed - # from the CI pipeline because the host-side `backend-tests` and - # `frontend-tests` jobs already exercise the exact same test code - # against the exact same Python version + requirements.txt the test - # image installs — on 2-vCPU GHA runners a re-run inside Docker - # added 5-10 min of wall-clock for zero new coverage. The - # image-validation purpose of this job (does it build, do imports - # resolve, does the integration container come up and answer HTTP) - # lives in the surrounding steps. test_docker.sh keeps the unit-test - # runs because devs running it locally don't have a separate host- - # side pytest job to compare against. - # Test 4: Integration Tests - name: Build integration container run: docker compose -f docker-compose.test.yml build integration From 845ad39b19bf99afeea571c6bae09695777e1460 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sat, 30 May 2026 13:49:09 +0200 Subject: [PATCH 6/7] =?UTF-8?q?=20=20fix(security):=20GHSA-6mf4-q26m-47pv?= =?UTF-8?q?=20=E2=80=94=20fail-closed=20on=20auth-probe=20DB=20errors=20(C?= =?UTF-8?q?VSS=209.8)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_auth_enabled() and auth_middleware both caught every exception during the auth-state probe and returned the "allow" answer instead of denying the request. Reporter's PoC floods /api/v1/auth/login to exhaust file descriptors, forcing the next SQLite connect to raise, then hits a protected endpoint during the fail-open window with no token — granting unauthenticated access to admin-account creation, API-key creation, DB backup download, and printer control. CWE-636 / CWE-755. Affects >= 0.1.6. Fix: - is_auth_enabled (backend/app/core/auth.py): only returns False for the legitimate "settings row absent" case; any actual exception propagates so the caller can deny the request. - auth_middleware (backend/app/main.py): returns 503 on any probe failure instead of await call_next(request). 4 new regression tests in test_auth_fail_closed.py pin the contract (propagates DB exceptions, returns False for no-row, True for "true", False for "false"). 1 existing security test renamed and updated to accept either 500 or 503 (both fail-closed) and to verify the SQLAlchemy detail does not leak in the body. Codebase grep confirmed no other auth-decision predicate has the same fail-open shape: _validate_api_key returns None on catch (→ 401 fail- closed downstream), is_advanced_auth_enabled propagates correctly, permissions.py has no catch-alls. Reported by @wondercrash via private advisory. --- CHANGELOG.md | 6 ++ backend/app/core/auth.py | 29 +++++--- backend/app/main.py | 12 ++- backend/tests/integration/test_security.py | 22 +++++- backend/tests/unit/test_auth_fail_closed.py | 81 +++++++++++++++++++++ 5 files changed, 134 insertions(+), 16 deletions(-) create mode 100644 backend/tests/unit/test_auth_fail_closed.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ddac477d1..de94dcd84 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/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 From b6fa77a64980acdb5fee7015c441cde4254feada Mon Sep 17 00:00:00 2001 From: maziggy Date: Sat, 30 May 2026 13:53:42 +0200 Subject: [PATCH 7/7] Bumped version --- backend/app/core/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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")