diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eb8c3fa3..d72e46175 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ All notable changes to Bambuddy will be documented in this file. ### Security - **Path-traversal hardening across the upload / import / file-write surface (routes + services); fifth CI backstop ships alongside** — A private path-traversal report against `POST /api/v1/projects/import/file` traced two attacker-controlled strings being joined to `library_dir` with no resolve + containment check: (a) `linked_folders[*].name` from the request's `project.json` ("Vector A" — an absolute path in this field collapsed `library_dir / "/anywhere"` to `Path("/anywhere")` because pathlib discards the left side when the right is absolute, letting the next `write_bytes` land anywhere the backend could write), and (b) per-entry `zf.namelist()` paths from the ZIP itself ("Vector B" — ZIP filenames carry `..` segments by spec and the join `library_dir / folder_name / relative_path` had no per-component check). Concrete escalation: drop a `.pth` file into the venv's `site-packages` directory for code execution on next service restart; overwrite the JWT signing-secret file to forge an admin token; overwrite `~/.ssh/authorized_keys` or `~/.bashrc` on native installs. **Fix is structural, not just patch the diff** (per [[feedback_dont_dismiss_preexisting]]). New `backend/app/utils/safe_path.py::safe_join_under(parent, *parts)` helper joins under a trusted parent, resolves both sides, asserts `is_relative_to(parent.resolve())`, and rejects up-front empty / null-byte / absolute path components. Wired into `import_project_file` at both vectors. **Adjacent fix from the routes audit**: `GET /api/v1/archives/{id}/photos/{filename}` had NO validation on `filename` and FileResponse-served arbitrary paths — the existing DELETE endpoint at least had a membership check against `archive.photos` (which is UUID-generated on upload), but GET shared neither the check nor any traversal guard. Both GET and DELETE now route through `safe_join_under` for defence-in-depth on top of the membership check. **Second adjacent fix from the services audit**: `ArchiveService.attach_timelapse(archive_id, data, filename)` in `backend/app/services/archive.py:1456` wrote `archive_dir / filename` where `filename` ultimately comes from either a printer's FTP listing (compromised-printer threat model — the printer is part of the trust surface) or the `?filename=...` query param on `POST /api/v1/archives/{id}/timelapse/select`. A malicious printer that returns a directory listing entry with `..` segments could write the timelapse bytes outside the archive directory; the `f.get("name") == filename` gate in the route did not prevent it because the gate is satisfied by whatever the printer claims is on disk. `attach_timelapse` now routes through `safe_join_under(..., http=False)` and returns `False` (logging the rejection) when the join would escape — matching the existing not-found contract of the function rather than raising 400 from inside a background task. **Audit sweep methodology**: AST-walked every Python file under `backend/app/api/routes/` AND `backend/app/services/` for `Path / Name` shapes (the exact shape that produced the original report). 25 additional route-layer sites and 8 additional service-layer sites confirmed safe case-by-case (UUID-generated filenames written by Bambuddy itself, `_safe_filename(...)` / `Path(arg).name` basename-stripped inputs, `os.walk`-discovered names, denylist + format-validated backup names, hardcoded constants iterated through a tuple, DB-stored paths whose write origin already goes through a resolved-and-containment-checked helper). Each safe site got a `# SEC-PATH-OK: ` marker so future audits can trust the inline guard at a glance. Six pre-existing safe-with-marker sites (`library.py` external upload, `archives.py` timelapse output, `projects.py` attachment download/delete, `settings.py` backup extractall) carry the same marker shape. **Fifth CI backstop** `test_route_path_arithmetic_is_safe_joined_or_marked` (`backend/tests/unit/test_no_unsafe_path_joins.py`) AST-walks every Python file in `backend/app/api/routes/` AND `backend/app/services/` and fails the build on any ` / ` join that doesn't either route through `safe_join_under` or carry the marker on the join line. Joins matching the higher-structure shapes (Attribute access, Subscript, f-string, `str(...)` call) are categorically different and out of scope — those are caught by the broader audit sweep, not the regression backstop. The services layer is in scope because it receives values from the routes verbatim AND from external sources Bambuddy has no control over (the printer FTP-listing case above). **Tests**: 17 unit tests for `safe_join_under` covering every escape vector (absolute path, Windows abs path, `..` segments, embedded `..`, null byte, empty string, no parts, non-str, plus legitimate nested-path round-trip); 4 integration tests against `POST /api/v1/projects/import/file` exercising the full FastAPI stack with the verbatim shape from the report (absolute path in `folder_name` → 400 + filesystem assertion that the target file doesn't exist; `..` in `folder_name` → 400; `..` in `relative_path` → 400; legitimate nested ZIP still imports cleanly to guard against the fix being over-strict); 3 unit tests against `ArchiveService.attach_timelapse` exercising the compromised-printer threat model (filename with `..` segments → returns False + no file at the escape target; absolute filename → returns False + no file at `/tmp`; legitimate `timelapse_YYYY-MM-DD_HH-MM-SS.mp4` → returns True + file lands inside archive_dir, guarding against the fix being over-strict). **SECURITY.md** gains a fifth rule + a fifth row in the CI-test mapping table; the rule explicitly names the printer FTP-listing case as in-scope to set the expectation for future services-layer audits. Full 5500+ test backend suite green; ruff clean. ### Fixed +- **Webhook printer-status / stop / cancel routes 500'd on every connected printer because the route treated the PrinterState dataclass as a dict (#1584, reported via in-app bug report)** — Reporter saw `GET /api/v1/webhook/printer/{id}/status` return `500 Internal Server Error` with a valid API key carrying the `read_status` scope, while `GET /api/v1/system/info` returned 200 with the same key — so auth and routing were fine, the handler itself was crashing. Cause: `printer_manager.get_status(printer_id)` returns a `PrinterState` dataclass (`backend/app/services/bambu_mqtt.py`), not a dict. The route at `webhook.py:266-270` called `status.get("connected", False)`, `status.get("state")`, `status.get("current_print")`, `status.get("progress")`, `status.get("remaining_time")` — every one raised `AttributeError`, which Starlette surfaced as a generic 500. Reporter's id-1 (printer exists) returned 500; non-existent ids returned 404 — exactly because the early `Printer not found` branch fired before reaching the crash. Same shape in two adjacent routes: `webhook_stop_print` (`POST /printer/{id}/stop`) and `webhook_cancel_print` (`POST /printer/{id}/cancel`) checked `status.get("connected")` / `status.get("state")` for their precondition gates. 8 crash sites total across the three routes. **Fix**: every `status.get("X", default)` replaced with attribute access (`status.X if status else default`); Pydantic response schema unchanged. `PrinterState`'s dataclass defaults cleanly cover the `status is None` branch (printer registered but never connected — the route now returns 200 with `connected=false, state=null, …` rather than crashing). **Tests** (`backend/tests/integration/test_webhook_printer_status.py`): 7 new — status route returns 200 with the dataclass attributes mapped into the response (regression for the exact #1584 shape); status route returns 200 with sensible defaults when `get_status()` returns None; status route returns 404 for a non-existent printer (control case proving the auth path is unaffected); stop route returns 503 when disconnected (pre-fix would have 500'd here); stop route returns 409 when state is not `RUNNING`; cancel route returns 503 when disconnected; cancel route returns 409 when state is not `RUNNING`/`PAUSE`. Runtime-verified end-to-end against a live PG-backed instance before and after: same key + same printer id, 500 before the patch and 200 with the correct payload after. Full backend suite + ruff clean. - **Path-traversal CI backstop now recognises markers on the closing-paren line (project-wide convention)** — `test_no_unsafe_path_joins.py::test_route_path_arithmetic_is_safe_joined_or_marked` AST-walks every Path-arithmetic site in `api/routes/` + `services/` and demands either `safe_join_under(...)` or a `# SEC-PATH-OK: ` marker. The marker-detection helper only scanned the BinOp's own line range (`lineno..end_lineno`), but the project's convention puts the marker on the line of the wrapping closing paren — one past `end_lineno`. The backstop flagged 30 already-marked, already-safe sites as findings, masking the fact that the post-GHSA marker work is complete. The helper now peeks one line past `end_lineno` IF that line begins with a continuation token (`)`, `]`, `}`, `,`), capturing exactly this convention without giving a free pass to a marker on a wholly unrelated next statement. 5 new tests in `TestMarkerDetection` pin the contract: marker on the BinOp line recognised; marker on the closing-paren line recognised; an unrelated marker on a later statement does NOT silence; a marker on a non-continuation line right after the BinOp does NOT silence; no marker anywhere is still flagged. Integration test now passes against the existing tree — 30 findings → 0 — with no changes to any guard / sanitisation in routes or services. - **Deleted local profiles no longer linger in the SliceModal preset dropdown; new manual "Refresh" button surfaces cloud-side deletions without waiting for the 5-minute cache (#1581, reported by @lloydjohnson)** — Reporter saw deleted local AND cloud profiles still appearing in the slice menu after removing them. Two distinct causes wired together. **Local half (real bug)**: `LocalProfilesView`'s import and delete mutations invalidated `['localPresets']` (the Local Profiles management view's own query) but not `['slicerPresets']` — the SliceModal reads from the unified `/slicer/presets` endpoint via a separate React Query key (`SliceModal.tsx:425`, `staleTime: 60_000`), so a freshly-deleted preset kept rendering in the dropdown until the modal's 60 s staleTime elapsed plus a refocus / remount. The backend was correct end-to-end (`delete_local_preset` removes the DB row, `get_db()` auto-commits, `_fetch_local_presets` reads fresh from DB with no backend cache). Both mutations now also invalidate `['slicerPresets']` so the next modal open shows the current set. **Cloud half (by-design backend cache + new opt-in bypass)**: `_fetch_cloud_presets` keeps a 5-minute per-(user, token) in-process cache balancing "users see their freshly-saved presets quickly" against "a busy install doesn't hit Bambu Cloud once per modal open" (`slicer_presets.py:69`). The user deletes cloud presets in Bambu Studio / Bambu Handy, not in Bambuddy, so there's no event hook to invalidate on — the cache only refreshes when the TTL expires. Rather than shorten the TTL (which would effectively rate-limit the cloud for every user), the listing endpoint gains an opt-in `?refresh=true` query param that bypasses BOTH the cloud cache and the 1-hour bundled-preset cache for that one call; the fresh result is still written back so subsequent normal callers still hit cached responses. **New SliceModal "Refresh" button**: lives in the preset section header next to the cloud-status banner, calls `getSlicerPresets({refresh: true})` and writes the fresh slots into the `['slicerPresets']` cache via `queryClient.setQueryData` (so the spinner disappears immediately rather than triggering a second refetch). Spins the `RefreshCw` icon while in-flight; disabled during a slice enqueue so users can't fire it twice. **i18n**: real translations for `slice.refreshPresets` + `slice.refreshPresetsTitle` (action label + tooltip) across all 9 locales per the [[feedback_translate_dont_fallback]] HARD RULE; parity script green at 5007 leaves × 9 locales. **Tests**: 2 new backend in `test_slicer_presets.py` (`refresh=True` re-hits Bambu Cloud even with a warm cache + still writes the fresh result back for the next normal call; same shape for `_fetch_bundled_presets`); 1 new frontend in `LocalProfilesView.test.tsx` asserts the delete flow invalidates `['slicerPresets']` in addition to `['localPresets']` via a spied QueryClient. Full backend suite + frontend vitest + ruff + eslint + i18n parity green. - **STL thumbnail noise on first generation: matplotlib cache + font_manager scan (reported by @maziggy)** — On first STL upload, three matplotlib-internal log lines surfaced: `WARNING [matplotlib] /opt/claude/.config/matplotlib is not a writable directory` (Bambuddy's `$HOME` isn't writable for the default config path so matplotlib fell back to `/tmp/matplotlib-XXXXXX`), `INFO [matplotlib.font_manager] Failed to extract font properties from NotoColorEmoji.ttf` (matplotlib doesn't support the COLR/COLR1 emoji format; this is per-font), and `INFO [matplotlib.font_manager] generated new fontManager` (the cache was rebuilt). Because the fallback was `/tmp`, every host reboot lost the cache and the font scan ran again. **Fix is in `stl_thumbnail.py` before the matplotlib import**: (a) `_configure_matplotlib_cache()` sets `MPLCONFIGDIR` to `settings.base_dir / .cache / matplotlib` (mkdir'd if missing) so the cache persists across container restarts and the writable-dir warning never fires; respects an externally-set value so operators who chose their own path aren't overridden; best-effort with a debug fallback if settings can't be imported or the mkdir fails. (b) `logging.getLogger("matplotlib.font_manager").setLevel(WARNING)` at module import demotes the per-font INFO scan so the first cold start (before the cache is populated) doesn't surface a multi-line matplotlib preamble. **Tests**: 3 new in `test_stl_thumbnail.py` — the font_manager logger is at WARNING after module import; `_configure_matplotlib_cache` creates the directory under `base_dir` and sets `MPLCONFIGDIR` to point at it; an externally-set `MPLCONFIGDIR` is preserved verbatim. diff --git a/backend/app/api/routes/webhook.py b/backend/app/api/routes/webhook.py index ad60f8769..8912b77f0 100644 --- a/backend/app/api/routes/webhook.py +++ b/backend/app/api/routes/webhook.py @@ -196,10 +196,13 @@ async def webhook_stop_print( check_printer_access(api_key, printer_id) status = printer_manager.get_status(printer_id) - if not status or not status.get("connected"): + # `printer_manager.get_status(...)` returns a ``PrinterState`` dataclass + # (see backend/app/services/bambu_mqtt.py), not a dict — `.get(...)` on it + # raises AttributeError and surfaces as a generic 500 (#1584). + if not status or not status.connected: raise HTTPException(status_code=503, detail="Printer not connected") - if status.get("state") != "RUNNING": + if status.state != "RUNNING": raise HTTPException(status_code=409, detail="No print in progress") try: @@ -224,10 +227,11 @@ async def webhook_cancel_print( check_printer_access(api_key, printer_id) status = printer_manager.get_status(printer_id) - if not status or not status.get("connected"): + # Same dataclass-not-dict shape as stop_print above (#1584). + if not status or not status.connected: raise HTTPException(status_code=503, detail="Printer not connected") - if status.get("state") not in ["RUNNING", "PAUSE"]: + if status.state not in ["RUNNING", "PAUSE"]: raise HTTPException(status_code=409, detail="No print to cancel") try: @@ -260,14 +264,18 @@ async def webhook_get_printer_status( status = printer_manager.get_status(printer_id) + # `printer_manager.get_status(...)` returns a ``PrinterState`` dataclass — + # attribute access, not dict lookup. The previous `.get(...)` calls raised + # AttributeError and surfaced as a generic 500 for any printer that + # actually had a status row (#1584). return PrinterStatusResponse( id=printer.id, name=printer.name, - connected=status.get("connected", False) if status else False, - state=status.get("state") if status else None, - current_print=status.get("current_print") if status else None, - progress=status.get("progress") if status else None, - remaining_time=status.get("remaining_time") if status else None, + connected=status.connected if status else False, + state=status.state if status else None, + current_print=status.current_print if status else None, + progress=status.progress if status else None, + remaining_time=status.remaining_time if status else None, ) diff --git a/backend/tests/integration/test_webhook_printer_status.py b/backend/tests/integration/test_webhook_printer_status.py new file mode 100644 index 000000000..e784b5c49 --- /dev/null +++ b/backend/tests/integration/test_webhook_printer_status.py @@ -0,0 +1,225 @@ +"""Regression tests for the webhook printer-status / stop / cancel routes. + +Pre-fix the routes treated ``printer_manager.get_status(...)``'s return value +as a dict and called ``.get(...)`` on it. The return is a ``PrinterState`` +dataclass (``backend/app/services/bambu_mqtt.py``), so the call raised +``AttributeError`` and surfaced as a generic 500 for any printer that +actually had a status row. See #1584. +""" + +from unittest.mock import MagicMock, patch + +import pytest +from httpx import AsyncClient + +from backend.app.services.bambu_mqtt import PrinterState + + +@pytest.fixture +async def api_key_data(async_client: AsyncClient, db_session): + """API key with read_status + control_printer scopes — covers status, + stop, and cancel in a single fixture.""" + from backend.app.core.auth import generate_api_key + from backend.app.models.api_key import APIKey + + full_key, key_hash, key_prefix = generate_api_key() + api_key = APIKey( + name="webhook-status-test-key", + key_hash=key_hash, + key_prefix=key_prefix, + can_read_status=True, + can_control_printer=True, + enabled=True, + ) + db_session.add(api_key) + await db_session.commit() + return full_key + + +@pytest.fixture +async def printer_row(db_session): + from backend.app.models.printer import Printer + + printer = Printer( + name="StatusTest", + ip_address="192.168.1.44", + access_code="12345678", + serial_number="00M00A000000010", + model="P1S", + ) + db_session.add(printer) + await db_session.commit() + return printer + + +class TestWebhookGetPrinterStatus: + """``GET /api/v1/webhook/printer/{id}/status`` — the route reads the + dataclass via attribute access, not ``.get(...)``. Pre-fix the call + raised AttributeError → 500 for every printer with a status row. + """ + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_200_with_connected_dataclass_status( + self, + async_client: AsyncClient, + api_key_data, + printer_row, + ): + """A live PrinterState dataclass must yield a 200 with the + attributes mapped into the response — this is the exact regression + from #1584 where the dataclass crashed the ``.get(...)`` calls.""" + state = PrinterState( + connected=True, + state="RUNNING", + current_print="bench.3mf", + progress=42.0, + remaining_time=1234, + ) + with patch( + "backend.app.api.routes.webhook.printer_manager.get_status", + MagicMock(return_value=state), + ): + resp = await async_client.get( + f"/api/v1/webhook/printer/{printer_row.id}/status", + headers={"X-API-Key": api_key_data}, + ) + + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["id"] == printer_row.id + assert body["name"] == "StatusTest" + assert body["connected"] is True + assert body["state"] == "RUNNING" + assert body["current_print"] == "bench.3mf" + assert body["progress"] == 42.0 + assert body["remaining_time"] == 1234 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_200_when_status_is_none( + self, + async_client: AsyncClient, + api_key_data, + printer_row, + ): + """A registered printer the manager hasn't seen yet returns None from + ``get_status``; the response must still be 200 with sensible + defaults rather than 500.""" + with patch( + "backend.app.api.routes.webhook.printer_manager.get_status", + MagicMock(return_value=None), + ): + resp = await async_client.get( + f"/api/v1/webhook/printer/{printer_row.id}/status", + headers={"X-API-Key": api_key_data}, + ) + + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["id"] == printer_row.id + assert body["connected"] is False + assert body["state"] is None + assert body["current_print"] is None + assert body["progress"] is None + assert body["remaining_time"] is None + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_404_when_printer_does_not_exist( + self, + async_client: AsyncClient, + api_key_data, + ): + resp = await async_client.get( + "/api/v1/webhook/printer/99999/status", + headers={"X-API-Key": api_key_data}, + ) + assert resp.status_code == 404 + + +class TestWebhookStopPrint: + """``POST /api/v1/webhook/printer/{id}/stop`` — same dataclass-shape + fix applies to the connection / state precondition checks (#1584).""" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_503_when_disconnected( + self, + async_client: AsyncClient, + api_key_data, + printer_row, + ): + state = PrinterState(connected=False, state="unknown") + with patch( + "backend.app.api.routes.webhook.printer_manager.get_status", + MagicMock(return_value=state), + ): + resp = await async_client.post( + f"/api/v1/webhook/printer/{printer_row.id}/stop", + headers={"X-API-Key": api_key_data}, + ) + # Pre-fix this would have 500'd on `status.get(...)`. Now it + # cleanly returns the documented 503. + assert resp.status_code == 503 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_409_when_not_running( + self, + async_client: AsyncClient, + api_key_data, + printer_row, + ): + state = PrinterState(connected=True, state="FINISH") + with patch( + "backend.app.api.routes.webhook.printer_manager.get_status", + MagicMock(return_value=state), + ): + resp = await async_client.post( + f"/api/v1/webhook/printer/{printer_row.id}/stop", + headers={"X-API-Key": api_key_data}, + ) + assert resp.status_code == 409 + + +class TestWebhookCancelPrint: + """``POST /api/v1/webhook/printer/{id}/cancel`` — same fix shape.""" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_503_when_disconnected( + self, + async_client: AsyncClient, + api_key_data, + printer_row, + ): + state = PrinterState(connected=False, state="unknown") + with patch( + "backend.app.api.routes.webhook.printer_manager.get_status", + MagicMock(return_value=state), + ): + resp = await async_client.post( + f"/api/v1/webhook/printer/{printer_row.id}/cancel", + headers={"X-API-Key": api_key_data}, + ) + assert resp.status_code == 503 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_returns_409_when_not_running_or_paused( + self, + async_client: AsyncClient, + api_key_data, + printer_row, + ): + state = PrinterState(connected=True, state="IDLE") + with patch( + "backend.app.api.routes.webhook.printer_manager.get_status", + MagicMock(return_value=state), + ): + resp = await async_client.post( + f"/api/v1/webhook/printer/{printer_row.id}/cancel", + headers={"X-API-Key": api_key_data}, + ) + assert resp.status_code == 409