mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-22 03:49:18 +02:00
fix(#1112): write uploads to external folders through to the mount
POST /library/files only rejected the read-only external branch and then unconditionally wrote to get_library_files_dir() with a UUID filename. The resulting LibraryFile row pointed at the external folder via folder_id, so the file showed up in Bambuddy's UI, but the bytes physically lived in archive/library/files/ and never touched the mount -- invisible from any other machine accessing the NAS/SMB share. Writable external uploads now write through to <external_path>/<filename> with the original filename preserved, and the DB row matches what scan produces (is_external=True, file_path=<absolute mount path>). Collisions return 409 instead of silently overwriting; inaccessible or non-writable mount returns 400; path-traversal filenames are rejected via resolve + relative_to. Extract-zip is now rejected against any external folder (not just read-only) with a clear "extract on the mount and run Scan" message -- the nested-subfolder creation path would need mkdir on the mount plus matching is_external LibraryFolder rows, which is a separate design. Scan already handles that shape.
This commit is contained in:
maziggy
@@ -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 `<external_path>/<filename>` (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=<absolute external 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.
|
||||
|
||||
@@ -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 ``(<library_files_dir>/<uuid><ext>, False)``.
|
||||
Writable external target: writes to ``<external_path>/<filename>``
|
||||
(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:
|
||||
|
||||
@@ -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 ``<external_path>/<filename>``, 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("/")
|
||||
|
||||
Reference in New Issue
Block a user