From 055243683cc428a26780e255677d5d75820ea171 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sat, 21 Feb 2026 11:35:18 +0100 Subject: [PATCH] Fix archive endpoints crash with "Is a directory" for BambuStudio prints (#475) When FTP download of a 3MF fails (e.g. BambuStudio-initiated prints), the fallback archive has file_path="". Path.exists() on settings.base_dir / "" resolves to the base directory itself and returns True, so ZipFile() then fails with [Errno 21] Is a directory. Replace .exists() with .is_file() across all 15 archive route checks and 1 in main.py. Add file_path truthiness guard for finish photo capture to prevent saving photos under the base directory. --- CHANGELOG.md | 1 + backend/app/api/routes/archives.py | 30 +++--- backend/app/main.py | 4 +- .../unit/test_archive_file_path_guard.py | 92 +++++++++++++++++++ 4 files changed, 110 insertions(+), 17 deletions(-) create mode 100644 backend/tests/unit/test_archive_file_path_guard.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 427c2f7c1..8f7ac2b4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to Bambuddy will be documented in this file. - **Queue "Any Model" Jobs Stuck in "Waiting" After Plate Clear** ([#435](https://github.com/maziggy/bambuddy/issues/435)) — After the queue visibility fix above, "Any Model" jobs were correctly assigned to an idle printer but immediately crashed with `'>=' not supported between instances of 'str' and 'int'` when computing AMS filament mapping. MQTT raw data returns AMS unit and tray IDs as strings, but `_build_loaded_filaments()` compared them to integers without casting. The crash prevented the assignment from committing, so the scheduler retried every 30 seconds in an infinite loop. Cast `ams_id` and `tray_id` to `int()` to match the pattern already used for external spool IDs. - **SD Card Cleanup After Print Never Runs** ([#374](https://github.com/maziggy/bambuddy/issues/374)) — The post-print SD card cleanup (which deletes uploaded gcode from the printer root to prevent phantom prints on power cycle) used `printer_manager.get_printer()`, which returns a `PrinterInfo` with only `name` and `serial_number`. Accessing `.ip_address`, `.access_code`, and `.model` raised `AttributeError`, silently caught by the outer exception handler. Replaced with a DB query for the `Printer` model, matching the pattern used everywhere else in `on_print_complete()`. - **Finish Photo Not Shown on Archives for BambuStudio Prints** ([#474](https://github.com/maziggy/bambuddy/issues/474)) — When a print was started from BambuStudio (not Bambuddy), the auto-archive had an empty `file_path`. The finish photo was saved correctly to `data/photos/`, but the photo serving endpoint resolved the path as `(base_dir / "").parent / "photos/"` which evaluates to `base_dir.parent/photos/` — one directory level too high. The photo existed on disk but the API returned 404. Fixed the path resolution in `get_photo`, `upload_photo`, and `delete_photo` to use `base_dir / Path(file_path).parent` (same pattern as the save code), which correctly resolves to `base_dir/photos/` when `file_path` is empty. +- **Archive Endpoints Crash With "Is a directory" for BambuStudio Prints** ([#475](https://github.com/maziggy/bambuddy/issues/475)) — When a print was started from BambuStudio (not Bambuddy), the 3MF file is transient on the printer and FTP download fails, creating a fallback archive with `file_path=""`. The archive endpoints used `Path.exists()` to check if the 3MF file was available, but `settings.base_dir / ""` resolves to the base directory itself — which `exists()` reports as True. Subsequent `ZipFile()` calls then failed with `[Errno 21] Is a directory`. Replaced all `.exists()` checks on archive file paths with `.is_file()` across 15 locations in the archive routes and 1 in the main module. Also added a `file_path` truthiness guard for finish photo capture to prevent saving photos under the base directory when the archive has no file path. - **ntfy Notifications Fail With "Illegal header value"** ([#466](https://github.com/maziggy/bambuddy/issues/466)) — When sending ntfy notifications with image attachments (progress, error events), the message body was placed in an HTTP `Message` header. Multi-line messages (e.g., printer name + remaining time) contain newline characters, which are illegal in HTTP headers. Test notifications worked because they are single-line with no image. Now escapes newlines to literal `\n` in the header, which ntfy interprets and renders as actual line breaks. Additionally, ntfy servers with attachments disabled rejected thumbnail uploads with "attachments not allowed" (HTTP 400 / code 40014), causing the entire notification to fail. Now automatically retries without the image when the server doesn't support attachments. - **Inventory Date Format Ignores Settings** ([#463](https://github.com/maziggy/bambuddy/issues/463)) — The inventory page used a local `formatDate()` that hardcoded the `en-GB` locale, always displaying dates in a fixed format regardless of the date format setting. Now fetches the `date_format` setting and uses the shared `formatDateInput()` utility which formats as MM/DD/YYYY, DD/MM/YYYY, YYYY-MM-DD, or browser locale based on the user's choice. - **Inventory Location Shows Garbled Characters for AMS-HT Slots** ([#463](https://github.com/maziggy/bambuddy/issues/463)) — The inventory location column computed slot letters via `String.fromCharCode(65 + ams_id)`, which produced accented characters (e.g., `Á`) for AMS-HT units (ams_id ≥ 128). Now uses the shared `formatSlotLabel()` utility which correctly handles AMS-HT and external spool slots. diff --git a/backend/app/api/routes/archives.py b/backend/app/api/routes/archives.py index 2529c9884..ce77f3eb5 100644 --- a/backend/app/api/routes/archives.py +++ b/backend/app/api/routes/archives.py @@ -829,7 +829,7 @@ async def rescan_archive( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") # Parse the 3MF file @@ -966,7 +966,7 @@ async def rescan_all_archives( for archive in archives: try: file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): errors.append({"id": archive.id, "error": "File not found"}) continue @@ -1036,7 +1036,7 @@ async def backfill_content_hashes( for archive in archives: try: file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): errors.append({"id": archive.id, "error": "File not found"}) continue @@ -1095,7 +1095,7 @@ async def download_archive( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "File not found") # Use inline disposition to let browser/OS handle file association @@ -1123,7 +1123,7 @@ async def download_archive_with_filename( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "File not found") return FileResponse( @@ -1179,7 +1179,7 @@ async def download_archive_for_slicer( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "File not found") return FileResponse( @@ -1990,7 +1990,7 @@ async def get_archive_capabilities( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "File not found") has_model = False @@ -2208,7 +2208,7 @@ async def get_gcode( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "File not found") try: @@ -2250,7 +2250,7 @@ async def get_plate_preview( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "File not found") try: @@ -2411,7 +2411,7 @@ async def get_archive_plates( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") plates = [] @@ -2679,7 +2679,7 @@ async def get_plate_thumbnail( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") try: @@ -2718,7 +2718,7 @@ async def get_filament_requirements( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") filaments = [] @@ -2938,7 +2938,7 @@ async def get_project_page( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") parser = ProjectPageParser(file_path) @@ -2963,7 +2963,7 @@ async def update_project_page( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") parser = ProjectPageParser(file_path) @@ -2995,7 +2995,7 @@ async def get_project_image( raise HTTPException(404, "Archive not found") file_path = settings.base_dir / archive.file_path - if not file_path.exists(): + if not file_path.is_file(): raise HTTPException(404, "Archive file not found") parser = ProjectPageParser(file_path) diff --git a/backend/app/main.py b/backend/app/main.py index f7553f8f4..7cb51ca8e 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1011,7 +1011,7 @@ def _load_objects_from_archive(archive, printer_id: int, logger) -> None: from backend.app.services.archive import extract_printable_objects_from_3mf file_path = app_settings.base_dir / archive.file_path - if file_path.exists() and str(file_path).endswith(".3mf"): + if file_path.is_file() and str(file_path).endswith(".3mf"): with open(file_path, "rb") as f: threemf_data = f.read() # Extract with positions for UI overlay @@ -2499,7 +2499,7 @@ async def on_print_complete(printer_id: int, data: dict): result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) archive = result.scalar_one_or_none() - if archive: + if archive and archive.file_path: import uuid from datetime import datetime from pathlib import Path diff --git a/backend/tests/unit/test_archive_file_path_guard.py b/backend/tests/unit/test_archive_file_path_guard.py new file mode 100644 index 000000000..59b61b668 --- /dev/null +++ b/backend/tests/unit/test_archive_file_path_guard.py @@ -0,0 +1,92 @@ +"""Tests for archive file_path guard against empty paths and directories (#475). + +When a 3mf download fails (e.g. BambuStudio-initiated prints), the fallback +archive is created with file_path="". Previously, `settings.base_dir / ""` +resolved to the base directory itself, which passed `exists()` but caused +`[Errno 21] Is a directory` when opened as a ZipFile. + +The fix replaces `.exists()` with `.is_file()` across all archive endpoints, +and adds an `archive.file_path` truthiness check for photo capture. +""" + +from pathlib import Path + +import pytest + + +class TestIsFileGuard: + """Verify that is_file() correctly rejects directories and empty paths.""" + + def test_empty_path_resolves_to_parent(self, tmp_path: Path): + """Path('') / '' resolves to the parent directory (which exists but is not a file).""" + base_dir = tmp_path / "data" + base_dir.mkdir() + + file_path = base_dir / "" + # exists() returns True (the directory exists) — this was the old broken check + assert file_path.exists() + # is_file() returns False (it's a directory, not a file) + assert not file_path.is_file() + + def test_real_file_passes_is_file(self, tmp_path: Path): + """A real 3mf file passes is_file().""" + fake_3mf = tmp_path / "archive" / "test.3mf" + fake_3mf.parent.mkdir(parents=True) + fake_3mf.write_bytes(b"PK\x03\x04") # ZIP magic bytes + + assert fake_3mf.is_file() + + def test_nonexistent_file_fails_is_file(self, tmp_path: Path): + """A nonexistent path fails is_file().""" + missing = tmp_path / "archive" / "missing.3mf" + assert not missing.is_file() + + def test_directory_fails_is_file(self, tmp_path: Path): + """A directory path fails is_file().""" + dir_path = tmp_path / "archive" + dir_path.mkdir() + assert not dir_path.is_file() + + +class TestFallbackArchiveFilePath: + """Verify that a fallback archive (file_path='') is handled safely.""" + + def test_base_dir_slash_empty_string_is_base_dir(self, tmp_path: Path): + """Joining base_dir with empty string produces base_dir (a directory).""" + base_dir = tmp_path / "data" + base_dir.mkdir() + + # Simulate: file_path = settings.base_dir / archive.file_path + # where archive.file_path = "" + file_path = base_dir / "" + + # The resolved path IS the directory itself + assert file_path.resolve() == base_dir.resolve() + # exists() says True (this caused the old bug) + assert file_path.exists() + # is_file() says False (this is the fix) + assert not file_path.is_file() + + def test_archive_file_path_empty_string_is_falsy(self): + """Empty string file_path is falsy (used for photo capture guard).""" + file_path = "" + assert not file_path + + def test_archive_file_path_real_is_truthy(self): + """Real file_path is truthy.""" + file_path = "archive/2026/02/test.3mf" + assert file_path + + +class TestPhotoPathDerivation: + """Verify that photo directory derivation is safe with empty file_path.""" + + def test_empty_file_path_parent_is_dot(self): + """Path('').parent is '.' — would resolve to base_dir instead of archive dir.""" + parent = Path("").parent + assert str(parent) == "." + + def test_real_file_path_parent_is_archive_dir(self): + """Real file_path parent gives the correct archive directory.""" + parent = Path("archive/2026/02/test.3mf").parent + assert str(parent) == "archive/2026/02"