diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d033713e..99cf33ebe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ All notable changes to Bambuddy will be documented in this file. - **Settings page: permission-gated instead of admin-only** — the Settings sidebar entry has always been visible to any user holding `settings:read`, but the route guard required admin role, so a non-admin with `settings:read` would see the entry, click it, and get silently redirected back to the dashboard. The route guard now matches the sidebar: any user with `settings:read` can open the page, and the individual tabs / cards continue to enforce their own per-feature permissions (`users:read`, `groups:update`, `oidc:*`, etc. — many of them admin-only, some not). Group editor routes moved to permission-based guards too (`groups:create` for `/groups/new`, `groups:update` for `/groups/:id/edit`), so permission delegation works end-to-end. Admins retain full access since admins implicitly hold every permission. ### Fixed +- **Uploads to writable external folders silently landed in internal storage** ([#1112](https://github.com/maziggy/bambuddy/issues/1112)) — `LibraryFolder` has an `external_readonly` flag, so the model already distinguishes writable from read-only external mounts, but `POST /library/files` rejected only the read-only branch and then unconditionally wrote to `get_library_files_dir()` with a UUID-scoped filename. The resulting `LibraryFile` row linked back to the external folder via `folder_id`, so the file showed up in the Bambuddy UI and could be printed, but the bytes physically lived in `archive/library/files/` and never touched the mount — invisible from any other machine accessing the same NAS/SMB share. New `_resolve_upload_destination()` helper detects writable external targets and writes through to `/` (keeping the original filename so the file is recognisable on the mount), with guards for missing/inaccessible path (400), non-writable mount (400), pre-existing filename on the mount (409 — no silent overwrite; the user is expected to rename and retry, matching how scan treats external files as externally-owned bytes), and a `resolve + relative_to` path-traversal guard on the joined destination. DB row now matches what scan produces: `is_external=True`, `file_path=`, so the existing download / delete / dedupe paths work unchanged (`to_absolute_path` already fast-paths `is_absolute()` inputs, and external-file deletion already bypasses trash and only drops the DB row + internal thumbnail). `POST /library/files/extract-zip` is now rejected against *any* external folder (not just read-only) with a clear "extract the ZIP on the external mount and run Scan" message — the nested-subfolder creation path would need to `mkdir` on the mount and create matching `is_external=True` `LibraryFolder` rows, which is a separate design round, and the Scan flow already handles that shape. 7 new integration tests cover: bytes land on the mount; DB row has `is_external=True` + absolute `file_path`; filename collision → 409 with prior bytes preserved; vanished external path → 400; path-traversal filename never escapes the external dir; extract-zip into writable external rejected with the Scan hint; root uploads unchanged. - **Queue item stuck at "printing" when print failed before reaching RUNNING** ([#1111](https://github.com/maziggy/bambuddy/issues/1111)) — Dispatching a file sliced for the wrong nozzle size (or any other pre-print error: AMS fault, wrong plate, nozzle not installed, etc.) left the queue item stuck at `status="printing"` forever, blocking every subsequent pending item for that printer (`check_queue` seeds `busy_printers` from any row in `'printing'` state and skips further dispatches for those printer IDs). Completion detection in `BambuMQTTClient._process_message` required the print to have reached `RUNNING` — either via `_previous_gcode_state == "RUNNING"` or the `_was_running` fallback — but a nozzle-mismatch failure transitions the printer `IDLE → PREPARE → FAILED` without ever entering `RUNNING`, so neither branch matched and `on_print_complete` never fired. The diagnostic log line at `bambu_mqtt.py:2690` ("State is FAILED but completion NOT triggered: prev=PREPARE, was_running=False") confirmed the path. Completion now also fires on `FAILED` from a pre-print state (`PREPARE` or `SLICING`) — restricted to those two so a stale `FAILED` on first connection (prev=None) still can't accidentally advance an unrelated queue item. Additionally, when a queue item transitions to `failed` the handler in `main.py` now populates `error_message` from the printer's current HMS error list, rendered via the existing `backend/app/services/hms_errors.py` lookup table (e.g. `[0500_4038] The nozzle diameter in sliced file is not consistent with the current nozzle setting. This file can't be printed.`) — previously `error_message` was left `NULL`, so users saw "failed" with no hint at the cause. 5 new unit tests in `TestPrePrintFailureCompletion` cover PREPARE→FAILED and SLICING→FAILED firing, IDLE→FAILED and initial-FAILED *not* firing (boot-time safety), and HMS errors being passed through in the callback payload; 6 new tests in `test_hms_error_summary.py` cover the error-message formatter (known-code lookup, unknown-code fallback, multi-error join, malformed-entry tolerance, all-malformed → None, empty → None). Thanks to @MartinNYHC for the report. - **Tailscale cert-renewal restart silently failed mid-way** (follow-up to [#1070](https://github.com/maziggy/bambuddy/issues/1070)) — The daily renewal path creates an `asyncio.Task` to restart VP services with the new cert. Inside that task, `stop_server()` / `stop_proxy()` call `_cancel_restart_task()`, which cancelled+awaited the currently-running task (itself). The self-await raised `RuntimeError`, got caught by the broad exception handler, but the cancel flag was still set — so the next `await` in `stop_server` raised `CancelledError` and aborted the restart partway through. The VP kept running the OLD expired cert until the process was manually restarted, silently defeating the feature. `_cancel_restart_task` now checks `asyncio.current_task()` and skips the cancel+await when the caller IS the restart task itself. Two new regression tests cover the self-cancel and outside-cancel paths. - **Settings table filled with duplicate rows on legacy SQLite installs** — pre-UNIQUE-constraint databases stored the `settings.key` column without a unique index, so the seed loop's `INSERT OR IGNORE` silently degraded to a plain INSERT and every `systemctl restart bambuddy` added another row of `advanced_auth_enabled` / `smtp_auth_enabled`. After a handful of restarts, `scalar_one_or_none()` in `is_advanced_auth_enabled` and similar sites blew up with `MultipleResultsFound`, 500'ing the login flow. `run_migrations` now dedupes (keeps MIN(id) per key) and creates the missing `ix_settings_key` unique index before the seed loop runs. Postgres installs were unaffected. 4 new regression tests cover legacy-with-dupes, legacy-already-clean (idempotent), and fresh-install (no-op) paths. diff --git a/backend/app/api/routes/library.py b/backend/app/api/routes/library.py index 762dfccf4..4f6f923a3 100644 --- a/backend/app/api/routes/library.py +++ b/backend/app/api/routes/library.py @@ -130,6 +130,61 @@ def calculate_file_hash(file_path: Path) -> str: return sha256_hash.hexdigest() +def _resolve_upload_destination(target_folder: LibraryFolder | None, filename: str) -> tuple[Path, bool]: + """Resolve the on-disk destination for an uploaded file. + + Non-external target: returns ``(/, False)``. + Writable external target: writes to ``/`` + (preserves the real filename so the file is recognisable on the mount); + returns ``(dest, True)``. Raises ``HTTPException`` for read-only external + folders (403), missing/inaccessible/non-writable external paths (400), and + filename collisions on the external mount (409). See #1112 — previously + uploads to writable external folders were silently misrouted to the + internal library dir. + """ + if target_folder is not None and target_folder.is_external: + if target_folder.external_readonly: + raise HTTPException(status_code=403, detail="Cannot upload to a read-only external folder") + if not target_folder.external_path: + raise HTTPException(status_code=400, detail="External folder has no configured path") + ext_dir = Path(target_folder.external_path) + if not ext_dir.exists() or not ext_dir.is_dir(): + raise HTTPException( + status_code=400, + detail=f"External path is not accessible: {target_folder.external_path}", + ) + if not os.access(ext_dir, os.W_OK): + raise HTTPException( + status_code=400, + detail=f"External path is not writable: {target_folder.external_path}", + ) + # Guard against path-traversal via a pathological filename — join then + # verify the resolved destination is still inside the external dir. + dest = (ext_dir / filename).resolve() + try: + dest.relative_to(ext_dir.resolve()) + except ValueError: + raise HTTPException(status_code=400, detail="Invalid filename") + if dest.exists(): + raise HTTPException( + status_code=409, + detail=f"A file named {filename!r} already exists in the external folder", + ) + return dest, True + ext = os.path.splitext(filename)[1].lower() + return get_library_files_dir() / f"{uuid.uuid4().hex}{ext}", False + + +def _stored_file_path(abs_path: Path, is_external: bool) -> str: + """Produce the value to persist in ``LibraryFile.file_path``. + + External files store the absolute mount path directly (same as scan does), + so ``to_absolute_path`` round-trips through its ``is_absolute()`` fast + path. Managed files store a path relative to ``base_dir`` for portability. + """ + return str(abs_path) if is_external else to_relative_path(abs_path) + + def _clean_3mf_metadata(obj): """Strip bytes and thumbnail-carrier keys so the payload is JSON-storable. @@ -1278,17 +1333,17 @@ async def upload_file( file_type = ext[1:] if ext else "unknown" # Verify folder exists if specified + target_folder = None if folder_id is not None: folder_result = await db.execute(select(LibraryFolder).where(LibraryFolder.id == folder_id)) target_folder = folder_result.scalar_one_or_none() if not target_folder: raise HTTPException(status_code=404, detail="Folder not found") - if target_folder.is_external and target_folder.external_readonly: - raise HTTPException(status_code=403, detail="Cannot upload to a read-only external folder") - # Generate unique filename for storage - unique_filename = f"{uuid.uuid4().hex}{ext}" - file_path = get_library_files_dir() / unique_filename + # Writable external folders write through to the mount so the file is + # visible outside Bambuddy (#1112); everything else lands under the + # internal library dir with a UUID-scoped filename. + file_path, is_external_upload = _resolve_upload_destination(target_folder, filename) # Save file content = await file.read() @@ -1366,11 +1421,13 @@ async def upload_file( if generate_stl_thumbnails: thumbnail_path = generate_stl_thumbnail(file_path, thumbnails_dir) - # Create database entry (store relative paths for portability) + # Create database entry (managed files store relative paths for portability; + # external files store the absolute mount path — same shape as scan produces) library_file = LibraryFile( folder_id=folder_id, + is_external=is_external_upload, filename=filename, - file_path=to_relative_path(file_path), + file_path=_stored_file_path(file_path, is_external_upload), file_type=file_type, file_size=len(content), file_hash=file_hash, @@ -1430,6 +1487,19 @@ async def extract_zip_file( raise HTTPException(status_code=404, detail="Target folder not found") if target_folder.is_external and target_folder.external_readonly: raise HTTPException(status_code=403, detail="Cannot extract ZIP to a read-only external folder") + if target_folder.is_external: + # Writable external folders aren't supported by extract-zip because the + # nested-subfolder creation path would need to mkdir on the mount and + # create matching is_external=True LibraryFolder rows — a separate + # design. Direct the user at Scan, which already handles that shape + # (#1112). + raise HTTPException( + status_code=400, + detail=( + "Cannot extract ZIP directly into an external folder. " + "Extract the ZIP on the external mount and run 'Scan External Folder' instead." + ), + ) # Save ZIP to temp file try: diff --git a/backend/tests/integration/test_external_folders_api.py b/backend/tests/integration/test_external_folders_api.py index 855d11bf0..74bc500b9 100644 --- a/backend/tests/integration/test_external_folders_api.py +++ b/backend/tests/integration/test_external_folders_api.py @@ -539,3 +539,174 @@ class TestExternalFolderProtections: ) assert response.status_code == 403 assert "read-only" in response.json()["detail"].lower() + + +class TestExternalFolderWritableUpload: + """Tests for upload write-through to writable external folders (#1112). + + Before the fix, uploads to writable external folders silently landed in the + internal library dir while the DB row pointed at the external folder — + files were invisible when the mount was viewed from another machine. + """ + + @pytest.fixture + def external_dir(self, tmp_path): + ext_dir = tmp_path / "writable_share" + ext_dir.mkdir() + return ext_dir + + @pytest.fixture + async def writable_folder(self, async_client, db_session, external_dir): + data = { + "name": "Writable NAS", + "external_path": str(external_dir), + "readonly": False, + } + response = await async_client.post("/api/v1/library/folders/external", json=data) + assert response.status_code == 200 + return response.json() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_upload_lands_on_external_mount( + self, async_client: AsyncClient, db_session, writable_folder, external_dir + ): + """Bytes are written to ``/``, not the internal library dir.""" + import io + + content = b"hello-external-world" + response = await async_client.post( + f"/api/v1/library/files?folder_id={writable_folder['id']}", + files={"file": ("upload.stl", io.BytesIO(content), "application/octet-stream")}, + ) + assert response.status_code == 200, response.text + + on_disk = external_dir / "upload.stl" + assert on_disk.exists(), "file must be written to the external mount" + assert on_disk.read_bytes() == content + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_upload_persists_correct_db_shape( + self, async_client: AsyncClient, db_session, writable_folder, external_dir + ): + """DB row must have ``is_external=True`` and ``file_path`` = absolute external path, + so scan-dedupe and deletion behaviour match scanned files.""" + import io + + from backend.app.models.library import LibraryFile + + response = await async_client.post( + f"/api/v1/library/files?folder_id={writable_folder['id']}", + files={"file": ("model.3mf", io.BytesIO(b"x"), "application/octet-stream")}, + ) + assert response.status_code == 200 + file_id = response.json()["id"] + + row = await db_session.get(LibraryFile, file_id) + await db_session.refresh(row) + assert row.is_external is True + assert row.file_path == str((external_dir / "model.3mf").resolve()) + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_upload_filename_collision_returns_409( + self, async_client: AsyncClient, db_session, writable_folder, external_dir + ): + """Re-uploading a filename that already exists on the mount must 409, + not silently overwrite — matches scan's treatment of external files as + externally-owned bytes.""" + import io + + (external_dir / "already.stl").write_bytes(b"prior") + response = await async_client.post( + f"/api/v1/library/files?folder_id={writable_folder['id']}", + files={"file": ("already.stl", io.BytesIO(b"new"), "application/octet-stream")}, + ) + assert response.status_code == 409 + assert (external_dir / "already.stl").read_bytes() == b"prior" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_upload_to_missing_external_path_returns_400( + self, async_client: AsyncClient, db_session, writable_folder, external_dir + ): + """If the external mount has gone away between folder-create and + upload, fail loud rather than silently misroute to internal storage.""" + import io + import shutil + + shutil.rmtree(external_dir) + + response = await async_client.post( + f"/api/v1/library/files?folder_id={writable_folder['id']}", + files={"file": ("x.stl", io.BytesIO(b"x"), "application/octet-stream")}, + ) + assert response.status_code == 400 + assert "not accessible" in response.json()["detail"].lower() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_upload_rejects_path_traversal_filename( + self, async_client: AsyncClient, db_session, writable_folder, external_dir + ): + """A malicious filename like ``../escape.stl`` must not write outside + the external folder. Defence-in-depth — FastAPI already strips these + on parse, but the resolve-and-relative_to guard is the final gate.""" + import io + + response = await async_client.post( + f"/api/v1/library/files?folder_id={writable_folder['id']}", + files={"file": ("../escape.stl", io.BytesIO(b"x"), "application/octet-stream")}, + ) + # Either a 400 from our traversal guard or a 200 with basename-stripped + # filename inside the external dir — both prove nothing escaped. + if response.status_code == 200: + assert not (external_dir.parent / "escape.stl").exists() + assert (external_dir / "escape.stl").exists() or (external_dir / "..escape.stl").exists() + else: + assert response.status_code in (400, 422) + assert not (external_dir.parent / "escape.stl").exists() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_zip_to_writable_external_folder_rejected( + self, async_client: AsyncClient, db_session, writable_folder + ): + """Extract-zip into writable external folders isn't supported (nested + subfolder creation on the mount is a separate design). Users are + pointed at the Scan flow instead.""" + import io + import zipfile + + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr("a/b/c.stl", b"x") + buf.seek(0) + + response = await async_client.post( + f"/api/v1/library/files/extract-zip?folder_id={writable_folder['id']}", + files={"file": ("test.zip", buf, "application/zip")}, + ) + assert response.status_code == 400 + assert "scan" in response.json()["detail"].lower() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_non_external_upload_unchanged(self, async_client: AsyncClient, db_session): + """Uploads with no folder_id (root) keep the existing internal-storage behaviour.""" + import io + + from backend.app.models.library import LibraryFile + + response = await async_client.post( + "/api/v1/library/files", + files={"file": ("root.stl", io.BytesIO(b"x"), "application/octet-stream")}, + ) + assert response.status_code == 200 + file_id = response.json()["id"] + row = await db_session.get(LibraryFile, file_id) + await db_session.refresh(row) + assert row.is_external is False + # Internal storage: file_path is UUID-scoped, stored as a relative path. + assert not row.file_path.startswith("/")