diff --git a/.gitignore b/.gitignore index 3c83decb2..a33d1b645 100644 --- a/.gitignore +++ b/.gitignore @@ -62,6 +62,11 @@ node_modules/ data/ +# Local-dev runtime caches (matplotlib MPLCONFIGDIR lands here when DATA_DIR +# is unset, so base_dir resolves to the repo root). In Docker this sits +# inside the /app/data volume and is already covered. +.cache/ + # JWT secret file (should be in data dir, but protect project root too) .jwt_secret diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ac59abfd..6ebcf6224 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,11 @@ All notable changes to Bambuddy will be documented in this file. - **VP `access_code` no longer leaked in DEBUG logs** — Pre-fix: `PUT /virtual-printers/{id}` logged `body.model_dump(exclude_unset=True)` at DEBUG, which dumped the plaintext access code whenever the user saved a new one. Now the field is redacted (`***`) before the log emission. Violation surfaced by no-secrets-in-logs audit; not exploitable in the field (DEBUG is off by default) but is exactly the kind of leak the rule exists to prevent. - **VP FTP upload capped at 4 GiB (DoS guard)** — `cmd_STOR` now rejects an upload that crosses `MAX_UPLOAD_BYTES = 4 GiB`, deletes the partial file, and replies 426. Without the cap a runaway or malicious client could drive RSS or disk to exhaustion; 4 GiB is well above any realistic multi-plate `.gcode.3mf`. Same code path adds the streaming rewrite (see Changed section for details). +### 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 +- **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. +- **Bulk-upload ZIPs of stub / empty STL files no longer spam the log with thousands of warnings (reported by @maziggy)** — Uploading a ZIP containing many minimal STL stubs (e.g. the 24-byte `solid test\nendsolid test` shape) emitted one `WARNING [backend.app.services.stl_thumbnail] Failed to load STL or empty mesh: ` per file. The warnings were technically correct — `trimesh.load(...)` returned a valid Mesh with zero vertices, the safeguard at `stl_thumbnail.py:54` matched, and the function returned None so the library entry got created without a thumbnail — but the volume turned a successful ZIP upload into a journal full of WARNING lines. **Two-step fix**: (1) the per-file message at `stl_thumbnail.py:55` demoted from `logger.warning` to `logger.debug`; this is a per-file content observation, not an actionable error, and the caller already handles None correctly. The branch now catches only the rare "large enough but trimesh still can't parse it" case, still visible in debug logs without spamming production. (2) New module constant `MIN_USABLE_STL_BYTES = 200` (binary STL with one triangle = 80B header + 4B count + 50B triangle = 134B; ASCII STL with one triangle ≈ 150B; 200 is a safe floor below any real STL). Three thumbnail call sites in `library.py` (extract_zip_file ZIP entry path, single-file upload, `_backfill_external_stl_thumbnails`) pre-skip files below this size BEFORE calling `generate_stl_thumbnail`, so stubs / placeholders / corrupted files never enter the trimesh pipeline at all. **What this does NOT change**: behaviour is identical for any real STL — generation still runs, MAX_VERTICES still triggers simplification at 100k vertices for the 256×256 thumbnail render, large files still get thumbnails. **Tests**: 2 new in `test_stl_thumbnail.py` — one verifies `MIN_USABLE_STL_BYTES` sits above the smallest binary (134B), the smallest ASCII (150B), and the reporter's 24-byte stub case; the other writes the verbatim 24-byte stub from the bug report, calls `generate_stl_thumbnail`, and asserts no `WARNING`-level "empty mesh" record appears in `caplog`. Full backend suite green; ruff clean. - **Bambu Cloud sign-in failures caused by an upstream Cloudflare challenge now surface an actionable message instead of "Invalid response from Bambu Cloud" (#1575, reported by @cliveflint)** — Reporter hit "Invalid response from bambulabs when trying to sign in with authenticator pass code" on a Pi (UK network). Log showed three back-to-back `POST /api/sign-in/tfa` calls all returning Cloudflare's "Just a moment..." HTML interstitial instead of JSON; `backend/app/services/bambu_cloud.py::verify_totp` caught the `json.JSONDecodeError` and returned the opaque "Invalid response from Bambu Cloud" message. **Root cause is Cloudflare-side, not Bambuddy**: a curl from this machine with the same honest `Bambuddy/1.0 (+https://github.com/maziggy/bambuddy)` UA at 2026-06-02 returned a clean `HTTP/2 400 {"code":5,"error":"Login failed"}` JSON — same UA, same headers, different network. CF's bot management appears to flag conditions (per-IP / TLS-fingerprint / rate / transient mitigation window) that don't reproduce from us. No reliable way to *prevent* the challenge from our side without browser impersonation, which is explicitly off the table per the 2026-05-12 compliance audit. **Fix is diagnostic, not bypass**: new `_detect_cloudflare_challenge(response) -> str | None` helper inspects the failed-parse response for CF markers (`"Just a moment..."` in body, `"challenges.cloudflare.com"` in body, HTTP 403 with `cf-mitigated` header, HTTP 503 with `cf-ray` header) and returns a message that attributes the block to Bambu Lab's Cloudflare protection, suggests waiting a few minutes, and tells the user that signing in to bambulab.com from a browser on the same network usually clears the challenge. Wired into all three JSON-parse sites: `login_request`, `verify_code`, and `verify_totp` — previously only `verify_totp` had a defensive catch; `login_request` and `verify_code` let the parse error bubble to `BambuCloudAuthError` with `"Expecting value..."` as the detail, which surfaced as a generic 401 in the UI. **Tests**: 8 new in `TestCloudflareChallengeDetection` (`backend/tests/unit/services/test_bambu_cloud.py`) — direct helper tests for each of the four CF markers, a negative case (real JSON 400 with `cf-ray` header from the actual successful curl response above is NOT misclassified as a challenge so the application-level "Login failed" still surfaces), an attribution check (message must name "Cloudflare" and "bambulab.com" so users can act on it), and full-stack tests covering all three call sites with the verbatim interstitial fragment from the reporter's log. The existing `test_verify_totp_cloudflare_blocked` updated to assert the new actionable message. Full 5486-test backend suite green; backend ruff clean. - **OIDC auto-provisioning now reads the standard `email` claim for `User.email` when `Email Claim` is set to a non-email identity claim (#1569, reported by @anderl1969)** — Reporter configured Authentik with `Email Claim = preferred_username` to drive username from the preferred_username claim and expected the standard `email` claim (which the ID token also carries) to populate the user's email field. Result: username was correctly set from `preferred_username`, but `User.email` came out empty. Cause: `backend/app/api/routes/mfa.py::_resolve_provider_email` reads only `claims[provider.email_claim]`. With `email_claim="preferred_username"` and `preferred_username="jdoe"`, the value fails the SEC-2 email shape check (no `@`) and returns `None`. The auto-create-users branch then constructs `User(email=None, …)` and stores `UserOIDCLink(provider_email=None)` even though `claims["email"]` carries a perfectly valid `jdoe@example.com`. **Fix**: new helper `_resolve_standard_email_for_user_record(provider, claims, provider_sub)` reads the standard `email` claim independently and applies the same Fall A/B logic (shape check, `require_email_verified` strict / permissive split, explicit `email_verified=False` drop). The auto-create-users branch in `oidc_callback` now resolves `user_email_for_storage = provider_email or _resolve_standard_email_for_user_record(...)` and uses that for both `new_user.email` and the `UserOIDCLink.provider_email` record. **Scope is deliberately narrow**: the fallback is invoked only when `provider.email_claim != "email"` AND the primary resolver returned `None` AND the auto-create-users branch is taken. The auto-link-existing-accounts gate above remains on the primary `provider_email` — it does NOT consult the fallback. This preserves every existing GHSA-shape guard: Fall-B (`email_claim='email'` + `require_email_verified=False`) is still rejected at schema level when paired with auto-link; Fall-C (custom claim) auto-link still depends on the custom claim's shape, never on the standard `email` claim. New email fallback path runs the same shape + `email_verified` enforcement as Fall-A/B for the standard `email` claim, so an attacker-controlled IdP that sets `email_verified=False` or sends a malformed value gets dropped exactly like it would on the primary path. **Tests**: 4 in `TestOIDCStandardEmailFallback` (`backend/tests/integration/test_mfa_api.py`) — `email_claim=preferred_username` with both claims present → username from `preferred_username`, email from standard `email`; `email_claim=preferred_username` with no standard `email` claim → email stays `None` (behaviour unchanged); standard `email` with `email_verified=False` → fallback drops, email stays `None`; `email_claim="email"` (default) with `email_verified` absent → fallback path does NOT fire (Fall-A semantics preserved). Full 5478-test backend suite green. Backend ruff clean. - **Sliced `.gcode.3mf` files now render in the 3D preview and expose a Preview-3D action in the file row (#1543, reported by @Vlado-Tarakan)** — Reporter exported a multi-plate `.gcode.3mf` from Bambu Studio to the shared folder Bambuddy watches and the 3D preview tab came up empty; if he re-uploaded the same file via the file manager, the preview worked. Root cause: two paths classify `file_type` differently. `backend/app/api/routes/library.py:1343-1348` (the shared-folder scan path) does a compound-extension check and tags the file `gcode.3mf`; the upload path at the same file's `1588` does a single `ext[1:]` and tags it `3mf`. Then `frontend/src/components/ModelViewerModal.tsx:71-73` had `hasModel = normalizedType === '3mf' || 'stl'` and `hasGcode = normalizedType === 'gcode' || '3mf'` — neither matched `gcode.3mf`, so the capabilities object landed with both flags false and the modal rendered an empty bed. `FileManagerPage.tsx:858` also gated the Preview-3D context action on `file_type === '3mf' || 'gcode' || 'stl'`, so for shared-folder files the entry didn't even appear, and the type pill at `765-770` had no colour case for `gcode.3mf` so it fell through to the generic gray. **Fix** (frontend-only, no backend churn): `ModelViewerModal.tsx` introduces an `isThreeMfFamily = normalizedType === '3mf' || normalizedType === 'gcode.3mf'` predicate used in two places — the capabilities branch (`hasModel = isThreeMfFamily || 'stl'`, `hasGcode = isThreeMfFamily || 'gcode'`) and the plates-loading branch that previously hard-gated on `!== '3mf'` and would have returned `setPlatesData(null)` for the shared-folder file. `FileManagerPage.tsx` adds `gcode.3mf` to the Preview-3D action gate and shares the gcode blue type-pill colour so sliced-output files are visually distinguishable from source 3MFs. The compound `gcode.3mf` classification on the backend is intentionally preserved — it carries useful "this is a sliced output" semantics that other UI surfaces could use later. The `canOpenInSlicer` and `sliceableType` checks at `ModelViewerModal.tsx:269, 277-280` are deliberately left alone — a sliced output isn't openable in the slicer, and `sliceableType` already explicitly excludes `.gcode` and `.gcode.3mf` per the comment "the file type can't be sliced". **Out of scope** (separate Bambu-Studio format limitation, not a Bambuddy bug): Vlado's secondary observation that the upload-path 3D preview "shows only one plate" even though his project has 5 plates — Bambu Studio's `.gcode.3mf` export contains the g-code and model data for the active plate only, not the entire multi-plate project. The print picker enumerates plates via `gcode_*.gcode` entries inside the zip (a separate code path), which is why the user can still pick the plate at print time. The empty-bed fix is the data point that closes the user-visible bug. **Tests**: existing full 2043-test frontend suite green; no test asserted on the unsupported `gcode.3mf` capabilities branch (the change is additive — `3mf` and `stl` and `gcode` behaviours are unchanged). Frontend build clean. diff --git a/SECURITY.md b/SECURITY.md index de646b640..d5f4cc850 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -147,6 +147,28 @@ The failure modes are where the vulnerabilities live. The structural backstops above catch *categories* of regression; the negative-path tests catch *specific* regressions in the new code. +### 5. Path joins under a trusted parent use the safe-join helper + +Anywhere a Bambuddy code path joins a string from outside the function's +scope (request body, query/path param, `UploadFile.filename`, ZIP +`namelist()` entry, tarfile member, **printer FTP-listing entry**) under +a trusted directory, the join must route through +`backend.app.utils.safe_path.safe_join_under(parent, *parts)`. The helper +resolves the joined path and asserts it is a descendant of the parent — +defeating both absolute-path collapse (`Path("/a") / "/b"` → `Path("/b")`) +and `..` traversal. + +Sites that have an inline guard (an explicit resolve + `is_relative_to`, +a basename-stripping helper like `_safe_filename`, or a pre-validated +alphanumeric filter) carry a `# SEC-PATH-OK: ` marker on the +same line. CI walks **both** `backend/app/api/routes/` and +`backend/app/services/` and fails the build on any +`` / `` join without either the helper or the +marker. The services layer is in scope because it receives values from +the routes verbatim and from external sources Bambuddy has no control +over (the compromised-printer threat model: a malicious printer can +serve crafted FTP-listing entries that flow straight into a path join). + ### Where these rules live in the codebase | Rule | Enforcement | Location | @@ -156,6 +178,7 @@ tests catch *specific* regressions in the new code. | 2. Fail-closed in auth code | `test_no_fail_open_in_auth_modules` | `backend/tests/unit/test_no_fail_open_in_auth.py` | | 3. No hardcoded fallback secrets | `test_no_hardcoded_secrets` | `backend/tests/unit/test_no_hardcoded_secrets.py` | | 4. Negative-path tests required | Reviewer responsibility (no automated CI gate yet) | PR review | +| 5. Safe-join under trusted parent | `test_route_path_arithmetic_is_safe_joined_or_marked` | `backend/tests/unit/test_no_unsafe_path_joins.py` | If you are adding a CI rule, update this table. If you are removing a CI rule, you are removing a security backstop and the PR description diff --git a/backend/app/api/routes/archives.py b/backend/app/api/routes/archives.py index 26d245f60..21914bf33 100644 --- a/backend/app/api/routes/archives.py +++ b/backend/app/api/routes/archives.py @@ -30,6 +30,7 @@ from backend.app.schemas.print_log import PrintLogResponse from backend.app.schemas.slicer import SliceRequest from backend.app.services.archive import ArchiveService from backend.app.utils.http import build_content_disposition +from backend.app.utils.safe_path import safe_join_under from backend.app.utils.threemf_tools import ( extract_embedded_presets_from_3mf, extract_nozzle_mapping_from_3mf, @@ -2373,7 +2374,7 @@ async def process_timelapse( filename = f"timelapse_{archive_id}_edited" if not filename.endswith(".mp4"): filename += ".mp4" - output_path = archive_dir / filename + output_path = archive_dir / filename # SEC-PATH-OK: filename alnum-filtered + .. rejected above success = await processor.process( output_path=output_path, @@ -2444,7 +2445,7 @@ async def upload_photo( ext = Path(file.filename).suffix.lower() photo_filename = f"{uuid.uuid4().hex[:8]}{ext}" - photo_path = photos_dir / photo_filename + photo_path = photos_dir / photo_filename # SEC-PATH-OK: photo_filename = uuid.uuid4().hex[:8] + ext # Save file content = await file.read() @@ -2477,8 +2478,20 @@ async def get_photo( if not archive: raise HTTPException(404, "Archive not found") + # Membership check first — UUID-generated names on upload mean any URL + # filename that doesn't appear here is by definition not a real photo. + # Mirrors the delete handler below; previously this endpoint had no + # membership check at all and joined `filename` straight to disk. + if not archive.photos or filename not in archive.photos: + raise HTTPException(404, "Photo not found") + archive_dir = settings.base_dir / Path(archive.file_path).parent - photo_path = archive_dir / "photos" / filename + photos_dir = archive_dir / "photos" + # Defence-in-depth: even though the membership check above already + # constrains `filename` to UUID-generated names from upload, the + # resolve + containment check guards against future code paths that + # might populate `archive.photos` from a less-trusted source. + photo_path = safe_join_under(photos_dir, filename) if not photo_path.exists(): raise HTTPException(404, "Photo not found") @@ -2512,9 +2525,10 @@ async def delete_photo( if not archive.photos or filename not in archive.photos: raise HTTPException(404, "Photo not found") - # Delete file + # Delete file — same defence-in-depth as get_photo above. archive_dir = settings.base_dir / Path(archive.file_path).parent - photo_path = archive_dir / "photos" / filename + photos_dir = archive_dir / "photos" + photo_path = safe_join_under(photos_dir, filename) if photo_path.exists(): photo_path.unlink() @@ -2960,7 +2974,9 @@ async def upload_archive( # Save uploaded file temporarily — strip directory components to prevent path traversal safe_filename = _safe_filename(file.filename) - temp_path = settings.archive_dir / "temp" / safe_filename + temp_path = ( + settings.archive_dir / "temp" / safe_filename + ) # SEC-PATH-OK: safe_filename = _safe_filename(...) basename-stripped above temp_path.parent.mkdir(parents=True, exist_ok=True) try: @@ -3008,7 +3024,9 @@ async def upload_archives_bulk( continue safe_filename = _safe_filename(file.filename) - temp_path = settings.archive_dir / "temp" / safe_filename + temp_path = ( + settings.archive_dir / "temp" / safe_filename + ) # SEC-PATH-OK: safe_filename = _safe_filename(...) basename-stripped above temp_path.parent.mkdir(parents=True, exist_ok=True) try: @@ -3639,7 +3657,9 @@ async def slice_archive( detail="Archive has no source file to slice", ) - src_path = Path(settings.base_dir) / src_relative + src_path = ( + Path(settings.base_dir) / src_relative + ) # SEC-PATH-OK: src_relative is archive.source_3mf_path from DB, set by _resolve_source_3mf_path which already does resolve+relative_to containment if not src_path.exists(): raise HTTPException(status_code=404, detail="Archive source file missing on disk") @@ -3947,7 +3967,9 @@ def _resolve_source_3mf_path(archive: PrintArchive, source_filename: str) -> Pat ) from exc source_dir.mkdir(parents=True, exist_ok=True) - return source_dir / source_filename + return ( + source_dir / source_filename + ) # SEC-PATH-OK: callers pass _safe_filename(...) basename-stripped; source_dir resolve+relative_to checked above @router.post("/{archive_id}/source") @@ -4262,7 +4284,7 @@ async def upload_f3d( # Save the F3D file - preserve original filename, strip directory components f3d_filename = _safe_filename(file.filename) - f3d_path = f3d_dir / f3d_filename + f3d_path = f3d_dir / f3d_filename # SEC-PATH-OK: f3d_filename = _safe_filename(...) basename-stripped above content = await file.read() f3d_path.write_bytes(content) diff --git a/backend/app/api/routes/library.py b/backend/app/api/routes/library.py index dde0368bd..a2dd60a65 100644 --- a/backend/app/api/routes/library.py +++ b/backend/app/api/routes/library.py @@ -63,7 +63,7 @@ from backend.app.schemas.library import ( ) from backend.app.schemas.slicer import SliceRequest, SliceResponse from backend.app.services.archive import ThreeMFParser -from backend.app.services.stl_thumbnail import generate_stl_thumbnail +from backend.app.services.stl_thumbnail import MIN_USABLE_STL_BYTES, generate_stl_thumbnail from backend.app.utils.filename import InvalidFilenameError, validate_print_filename from backend.app.utils.threemf_tools import ( extract_embedded_presets_from_3mf, @@ -219,7 +219,7 @@ def _resolve_upload_destination(target_folder: LibraryFolder | None, filename: s ) # 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() + dest = (ext_dir / filename).resolve() # SEC-PATH-OK: resolve + relative_to containment check on next line try: dest.relative_to(ext_dir.resolve()) except ValueError: @@ -302,7 +302,7 @@ def _move_file_bytes(file: LibraryFile, target_folder: LibraryFolder | None) -> raise _MoveSkip("target_inaccessible", f"target path not accessible: {ext_dir}") if not os.access(ext_dir, os.W_OK): raise _MoveSkip("target_unwritable", f"target path not writable: {ext_dir}") - dest = (ext_dir / file.filename).resolve() + dest = (ext_dir / file.filename).resolve() # SEC-PATH-OK: resolve + relative_to containment check on next line try: dest.relative_to(ext_dir.resolve()) except ValueError: @@ -433,7 +433,9 @@ async def save_3mf_bytes_to_library( # extension so downstream logic (ThreeMFParser, thumbnail viewer) works. ext = os.path.splitext(filename)[1].lower() or ".3mf" unique_filename = f"{uuid.uuid4().hex}{ext}" - file_path = get_library_files_dir() / unique_filename + file_path = ( + get_library_files_dir() / unique_filename + ) # SEC-PATH-OK: unique_filename = uuid.uuid4().hex + ext, generated on the previous line with open(file_path, "wb") as fh: fh.write(file_bytes) @@ -451,7 +453,7 @@ async def save_3mf_bytes_to_library( if thumb_data: thumbs_dir = get_library_thumbnails_dir() thumb_filename = f"{uuid.uuid4().hex}{thumb_ext}" - thumb_path = thumbs_dir / thumb_filename + thumb_path = thumbs_dir / thumb_filename # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + thumb_ext with open(thumb_path, "wb") as fh: fh.write(thumb_data) thumbnail_path = str(thumb_path) @@ -554,7 +556,7 @@ def create_image_thumbnail(file_path: Path, thumbnails_dir: Path, max_size: int from PIL import Image thumb_filename = f"{uuid.uuid4().hex}.png" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = thumbnails_dir / thumb_filename # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + ".png" with Image.open(file_path) as img: # Convert to RGB if necessary (for PNG with transparency, etc.) @@ -582,7 +584,9 @@ def create_image_thumbnail(file_path: Path, thumbnails_dir: Path, max_size: int file_size = file_path.stat().st_size if file_size < 500000: # Less than 500KB thumb_filename = f"{uuid.uuid4().hex}{file_path.suffix}" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = ( + thumbnails_dir / thumb_filename + ) # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + file_path.suffix shutil.copy2(file_path, thumb_path) return str(thumb_path) except OSError: @@ -636,6 +640,14 @@ async def _backfill_external_stl_thumbnails(folder_ids: list[int]) -> None: abs_path = to_absolute_path(stl_file.file_path) if not abs_path or not abs_path.exists(): continue + # Pre-skip files too small to contain even a single triangle. + # Bulk-uploaded ZIPs of stub STLs would otherwise trigger one + # trimesh.load() call + one debug log line per stub. + try: + if abs_path.stat().st_size < MIN_USABLE_STL_BYTES: + continue + except OSError: + continue try: thumb_path = generate_stl_thumbnail(abs_path, thumbnails_dir) except Exception as exc: # noqa: BLE001 — never let one bad STL kill the rest @@ -1393,7 +1405,9 @@ async def scan_external_folder( name=part, parent_id=current_parent, is_external=True, - external_path=str(ext_path / current_path), + external_path=str( + ext_path / current_path + ), # SEC-PATH-OK: current_path built from Path(rel_dir).parts of an os.walk descent under ext_path external_readonly=folder.external_readonly, external_show_hidden=folder.external_show_hidden, ) @@ -1409,7 +1423,9 @@ async def scan_external_folder( if not folder.external_show_hidden and filename.startswith("."): continue - filepath = Path(dirpath) / filename + filepath = ( + Path(dirpath) / filename + ) # SEC-PATH-OK: dirpath + filename from os.walk(ext_path); filesystem-discovered, not user input ext = filepath.suffix.lower() # Check for compound extensions like .gcode.3mf @@ -1459,7 +1475,9 @@ async def scan_external_folder( if thumb_data: thumb_dir = get_library_thumbnails_dir() thumb_filename = f"{uuid.uuid4().hex}{thumbnail_ext}" - thumb_full = thumb_dir / thumb_filename + thumb_full = ( + thumb_dir / thumb_filename + ) # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + thumbnail_ext thumb_full.write_bytes(thumb_data) thumbnail_path = to_relative_path(thumb_full) @@ -1492,7 +1510,7 @@ async def scan_external_folder( if thumb_data: thumb_dir = get_library_thumbnails_dir() thumb_filename = f"{uuid.uuid4().hex}.png" - thumb_full = thumb_dir / thumb_filename + thumb_full = thumb_dir / thumb_filename # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + ".png" thumb_full.write_bytes(thumb_data) thumbnail_path = to_relative_path(thumb_full) @@ -1737,7 +1755,9 @@ async def upload_file( # Save thumbnail if extracted if thumbnail_data: thumb_filename = f"{uuid.uuid4().hex}{thumbnail_ext}" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = ( + thumbnails_dir / thumb_filename + ) # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + thumbnail_ext with open(thumb_path, "wb") as f: f.write(thumbnail_data) thumbnail_path = str(thumb_path) @@ -1766,7 +1786,9 @@ async def upload_file( thumbnail_data = extract_gcode_thumbnail(file_path) if thumbnail_data: thumb_filename = f"{uuid.uuid4().hex}.png" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = ( + thumbnails_dir / thumb_filename + ) # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + ".png" with open(thumb_path, "wb") as f: f.write(thumbnail_data) thumbnail_path = str(thumb_path) @@ -1778,9 +1800,16 @@ async def upload_file( thumbnail_path = create_image_thumbnail(file_path, thumbnails_dir) elif ext == ".stl": - # Generate STL thumbnail if enabled + # Generate STL thumbnail if enabled. Same MIN_USABLE_STL_BYTES + # pre-skip as extract_zip_file — stubs / placeholders below this + # size can't contain a triangle so trimesh would return an empty + # mesh anyway. if generate_stl_thumbnails: - thumbnail_path = generate_stl_thumbnail(file_path, thumbnails_dir) + try: + if file_path.stat().st_size >= MIN_USABLE_STL_BYTES: + thumbnail_path = generate_stl_thumbnail(file_path, thumbnails_dir) + except OSError: + pass # Create database entry (managed files store relative paths for portability; # external files store the absolute mount path — same shape as scan produces) @@ -1970,7 +1999,9 @@ async def extract_zip_file( # Generate unique filename for storage unique_filename = f"{uuid.uuid4().hex}{ext}" - file_path = get_library_files_dir() / unique_filename + file_path = ( + get_library_files_dir() / unique_filename + ) # SEC-PATH-OK: unique_filename = uuid.uuid4().hex + ext # Extract and save file file_content = zf.read(zip_path) @@ -1995,7 +2026,9 @@ async def extract_zip_file( if thumbnail_data: thumb_filename = f"{uuid.uuid4().hex}{thumbnail_ext}" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = ( + thumbnails_dir / thumb_filename + ) # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + thumbnail_ext with open(thumb_path, "wb") as f: f.write(thumbnail_data) thumbnail_path = str(thumb_path) @@ -2022,7 +2055,9 @@ async def extract_zip_file( thumbnail_data = extract_gcode_thumbnail(file_path) if thumbnail_data: thumb_filename = f"{uuid.uuid4().hex}.png" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = ( + thumbnails_dir / thumb_filename + ) # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + ".png" with open(thumb_path, "wb") as f: f.write(thumbnail_data) thumbnail_path = str(thumb_path) @@ -2033,8 +2068,12 @@ async def extract_zip_file( thumbnail_path = create_image_thumbnail(file_path, thumbnails_dir) elif ext == ".stl": - # Generate STL thumbnail if enabled - if generate_stl_thumbnails: + # Generate STL thumbnail if enabled. Pre-skip files + # below MIN_USABLE_STL_BYTES — they can't contain + # even a single triangle, and bulk-uploaded ZIPs of + # stub STLs would otherwise log one debug line per + # file via the empty-mesh branch in trimesh.load. + if generate_stl_thumbnails and len(file_content) >= MIN_USABLE_STL_BYTES: thumbnail_path = generate_stl_thumbnail(file_path, thumbnails_dir) # Create database entry (store relative paths for portability) @@ -3540,7 +3579,7 @@ async def slice_and_persist( base_name = model_filename.rsplit(".", 1)[0] out_filename = f"{base_name}.gcode.3mf" unique_name = f"{uuid.uuid4().hex}.gcode.3mf" - out_path = get_library_files_dir() / unique_name + out_path = get_library_files_dir() / unique_name # SEC-PATH-OK: unique_name = uuid.uuid4().hex + ".gcode.3mf" out_path.write_bytes(result.content) # Extract thumbnail from the produced 3MF so the library card shows a @@ -3657,9 +3696,13 @@ async def slice_and_persist_as_archive( timestamp = datetime.now(timezone.utc).strftime("%Y%m%d_%H%M%S") printer_folder = str(source_archive.printer_id) if source_archive.printer_id is not None else "unassigned" archive_subdir = f"{timestamp}_{base_name}_sliced" - archive_dir = app_settings.archive_dir / printer_folder / archive_subdir + archive_dir = ( + app_settings.archive_dir / printer_folder / archive_subdir + ) # SEC-PATH-OK: printer_folder = str(int|None), archive_subdir = f"{timestamp}_{base_name}_sliced" where base_name went through _safe_filename archive_dir.mkdir(parents=True, exist_ok=True) - out_path = archive_dir / out_filename + out_path = ( + archive_dir / out_filename + ) # SEC-PATH-OK: out_filename = f"{base_name}.gcode.3mf" where base_name went through _safe_filename out_path.write_bytes(result.content) # Extract a thumbnail for the new archive card. Priority order: diff --git a/backend/app/api/routes/projects.py b/backend/app/api/routes/projects.py index 07d66c53a..6ecd2e628 100644 --- a/backend/app/api/routes/projects.py +++ b/backend/app/api/routes/projects.py @@ -41,6 +41,7 @@ from backend.app.schemas.project import ( TimelineEvent, ) from backend.app.utils.http import build_content_disposition +from backend.app.utils.safe_path import safe_join_under logger = logging.getLogger(__name__) @@ -892,7 +893,7 @@ async def upload_attachment( # Generate unique filename unique_filename = f"{uuid.uuid4().hex}{ext}" - file_path = attachments_dir / unique_filename + file_path = attachments_dir / unique_filename # SEC-PATH-OK: unique_filename = uuid.uuid4().hex + ext # Save file try: @@ -964,7 +965,9 @@ async def download_attachment( raise HTTPException(status_code=404, detail="Attachment not found") # Check file exists - file_path = get_project_attachments_dir(project_id) / filename + file_path = ( + get_project_attachments_dir(project_id) / filename + ) # SEC-PATH-OK: filename validated above (no /, \\, .., empty) + attachment membership check if not file_path.exists(): raise HTTPException(status_code=404, detail="Attachment file not found") @@ -1004,7 +1007,9 @@ async def delete_attachment( project.attachments = attachments if attachments else None # Delete file - file_path = get_project_attachments_dir(project_id) / filename + file_path = ( + get_project_attachments_dir(project_id) / filename + ) # SEC-PATH-OK: filename validated above (no /, \\, .., empty) + attachment membership check if file_path.exists(): try: os.remove(file_path) @@ -1066,7 +1071,7 @@ async def upload_project_cover_image( logger.warning("Failed to delete old cover image %s: %s", old_path, e) unique_filename = f"cover_{uuid.uuid4().hex}{ext}" - file_path = attachments_dir / unique_filename + file_path = attachments_dir / unique_filename # SEC-PATH-OK: unique_filename = f"cover_{uuid.uuid4().hex}{ext}" try: with open(file_path, "wb") as f: content = await file.read() @@ -1827,6 +1832,13 @@ async def import_project_file( if not folder_name: continue + # Containment check on the folder name — refuses absolute paths and + # ``..`` traversal in ``project.json[linked_folders[*].name]``. The + # previous code did ``library_dir / folder_name`` directly, which + # collapses to ``Path(folder_name)`` when folder_name is absolute + # and lets ``..`` escape after mkdir. + folder_path = safe_join_under(library_dir, folder_name) + # Check if folder exists existing_result = await db.execute( select(LibraryFolder).where( @@ -1853,7 +1865,6 @@ async def import_project_file( await db.flush() # Create folder on disk - folder_path = library_dir / folder_name folder_path.mkdir(parents=True, exist_ok=True) # Import files for this folder from ZIP @@ -1868,8 +1879,18 @@ async def import_project_file( if not relative_path: continue - # Write file to disk - file_disk_path = library_dir / folder_name / relative_path + # Containment check on the per-entry relative path. ZIP names + # can carry ``..`` segments by spec; without resolve + parent + # containment, ``files//../../../etc/x`` escapes + # ``library_dir`` entirely. ``relative_path`` is split into + # parts because ``safe_join_under`` rejects parts that start + # with ``/``, and a single combined string would hide an + # embedded ``..`` segment behind a forward slash. + file_disk_path = safe_join_under( + library_dir, + folder_name, + *Path(relative_path).parts, + ) file_disk_path.parent.mkdir(parents=True, exist_ok=True) file_disk_path.write_bytes(file_content) diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index 3613f625c..80505c2ce 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -573,7 +573,9 @@ async def create_backup_zip(output_path: Path | None = None) -> tuple[Path, str] for name, src_dir in dirs_to_backup: if src_dir.exists() and any(src_dir.iterdir()): try: - shutil.copytree(src_dir, temp_path / name) + shutil.copytree( + src_dir, temp_path / name + ) # SEC-PATH-OK: name iterates the dirs_to_backup tuple of constant strings ("archive", "virtual_printer", ...) except shutil.Error as e: logger.warning("Some files in %s could not be copied: %s", name, e) except PermissionError as e: @@ -599,7 +601,9 @@ async def create_backup_zip(output_path: Path | None = None) -> tuple[Path, str] # Create ZIP if output_path is not None: - zip_file = output_path / filename + zip_file = ( + output_path / filename + ) # SEC-PATH-OK: filename = f"bambuddy-backup-{datetime.now()...}.zip" generated in create_backup_zip itself else: fd, tmp = tempfile.mkstemp(suffix=".zip") os.close(fd) @@ -869,7 +873,9 @@ async def restore_backup( # Reject path-traversal payloads: any entry whose resolved # path escapes temp_path would allow writing arbitrary files # on the host (ZipSlip / CVE-2006-5456). - dest = (temp_path / name).resolve() + dest = ( + temp_path / name + ).resolve() # SEC-PATH-OK: is_relative_to containment check below before extractall # is_relative_to (Python 3.9+) covers both relative # path-traversal (../etc/passwd) and absolute-path overrides # (/etc/passwd) — str.startswith was vulnerable to @@ -1003,7 +1009,9 @@ async def restore_backup( skipped_dirs = [] for name, dest_dir in dirs_to_restore: - src_dir = temp_path / name + src_dir = ( + temp_path / name + ) # SEC-PATH-OK: name iterates the dirs_to_restore tuple of constant strings ("archive", "virtual_printer", ...) if src_dir.exists(): logger.info("Restoring %s directory...", name) try: diff --git a/backend/app/services/archive.py b/backend/app/services/archive.py index cae8c5939..fc80741d8 100644 --- a/backend/app/services/archive.py +++ b/backend/app/services/archive.py @@ -16,6 +16,7 @@ from backend.app.core.config import settings from backend.app.models.archive import PrintArchive from backend.app.models.filament import Filament from backend.app.models.printer import Printer +from backend.app.utils.safe_path import PathTraversalError, safe_join_under logger = logging.getLogger(__name__) @@ -1084,7 +1085,9 @@ class ArchiveService: archive_name = f"{timestamp}_{display_stem}" # Use "unassigned" folder for archives without a printer printer_folder = str(printer_id) if printer_id is not None else "unassigned" - archive_dir = settings.archive_dir / printer_folder / archive_name + archive_dir = ( + settings.archive_dir / printer_folder / archive_name + ) # SEC-PATH-OK: printer_folder = str(int|None) → digits or "unassigned"; archive_name = f"{timestamp}_{display_stem}" where resolve_display_stem strips path components via Path(filename).name archive_dir.mkdir(parents=True, exist_ok=True) # Copy 3MF file with an explicit fsync'd loop (avoids a sendfile @@ -1448,12 +1451,29 @@ class ArchiveService: return False # Get archive directory - file_path = settings.base_dir / archive.file_path + file_path = ( + settings.base_dir / archive.file_path + ) # SEC-PATH-OK: archive.file_path is DB-stored, set by archive_print() under settings.archive_dir archive_dir = file_path.parent # Save timelapse - use thread pool to avoid blocking event loop - # (timelapse files can be 100MB+, sync write blocks for seconds) - timelapse_file = archive_dir / filename + # (timelapse files can be 100MB+, sync write blocks for seconds). + # `filename` ultimately comes from a printer's FTP listing (compromised- + # printer threat model) or a query param on /archives/{id}/timelapse/select; + # the safe-join helper rejects ``..`` segments and absolute paths so a + # crafted name can't escape the archive directory. Use http=False so a + # service-layer reject surfaces as a return False (matching the existing + # not-found contract) rather than a 400 raised from inside a background + # task. + try: + timelapse_file = safe_join_under(archive_dir, filename, http=False) + except PathTraversalError: + logger.warning( + "Refusing to attach timelapse with unsafe filename %r to archive %s", + filename, + archive_id, + ) + return False await asyncio.to_thread(timelapse_file.write_bytes, timelapse_data) # Update archive record diff --git a/backend/app/services/camera.py b/backend/app/services/camera.py index a47828081..427b608c5 100644 --- a/backend/app/services/camera.py +++ b/backend/app/services/camera.py @@ -651,7 +651,9 @@ async def capture_finish_photo( # Generate filename with timestamp timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") filename = f"finish_{timestamp}_{uuid.uuid4().hex[:8]}.jpg" - output_path = photos_dir / filename + output_path = ( + photos_dir / filename + ) # SEC-PATH-OK: filename = f"finish_{timestamp}_{uuid.uuid4().hex[:8]}.jpg" generated above success = await capture_camera_frame( ip_address=ip_address, diff --git a/backend/app/services/library_trash.py b/backend/app/services/library_trash.py index 791978eaa..15d528fb6 100644 --- a/backend/app/services/library_trash.py +++ b/backend/app/services/library_trash.py @@ -64,7 +64,9 @@ def _to_absolute_path(relative_path: str | None) -> Path | None: path = Path(relative_path) if path.is_absolute(): return path - return Path(app_settings.base_dir) / path + return ( + Path(app_settings.base_dir) / path + ) # SEC-PATH-OK: relative_path is LibraryFile.file_path / LibraryFile.thumbnail_path — DB-stored, internally generated by the upload pipeline def _age_cutoff(now: datetime, older_than_days: int) -> datetime: diff --git a/backend/app/services/local_backup.py b/backend/app/services/local_backup.py index a06f16f9d..878f80ea0 100644 --- a/backend/app/services/local_backup.py +++ b/backend/app/services/local_backup.py @@ -223,7 +223,9 @@ class LocalBackupService: if not filename.startswith("bambuddy-backup-") or not filename.endswith(".zip"): return None backup_dir = self._resolve_backup_dir(path_setting) - target = backup_dir / filename + target = ( + backup_dir / filename + ) # SEC-PATH-OK: filename rejected above on /, \\, .., plus startswith "bambuddy-backup-" + endswith ".zip" gate if not target.exists(): return None return target @@ -253,7 +255,9 @@ class LocalBackupService: return {"success": False, "message": "Invalid filename"} backup_dir = self._resolve_backup_dir(path_setting) - target = backup_dir / filename + target = ( + backup_dir / filename + ) # SEC-PATH-OK: filename rejected above on /, \\, .., plus startswith "bambuddy-backup-" + endswith ".zip" gate below if not target.exists(): return {"success": False, "message": "Backup not found"} diff --git a/backend/app/services/spoolman_tracking.py b/backend/app/services/spoolman_tracking.py index a3db52b13..fa80a1413 100644 --- a/backend/app/services/spoolman_tracking.py +++ b/backend/app/services/spoolman_tracking.py @@ -212,7 +212,9 @@ async def store_print_data( return # Get 3MF file path - full_path = app_settings.base_dir / file_path + full_path = ( + app_settings.base_dir / file_path + ) # SEC-PATH-OK: file_path is archive.file_path / library_file.file_path — DB-stored, internally generated if not full_path.exists(): logger.debug("[SPOOLMAN] 3MF file not found: %s", full_path) return diff --git a/backend/app/services/stl_thumbnail.py b/backend/app/services/stl_thumbnail.py index d383f1aa0..846e96512 100644 --- a/backend/app/services/stl_thumbnail.py +++ b/backend/app/services/stl_thumbnail.py @@ -4,11 +4,52 @@ Generates thumbnail images from STL files using trimesh and matplotlib. """ import logging +import os import uuid from pathlib import Path logger = logging.getLogger(__name__) +# Matplotlib's font_manager emits one INFO line per font on first import +# while it builds its cache, including a noisy "Failed to extract font +# properties from NotoColorEmoji.ttf" for the COLR/COLR1 emoji format it +# doesn't support. These are not actionable — demote to WARNING so real +# font issues still surface but the first STL upload doesn't produce a +# multi-line matplotlib preamble in the journal. +logging.getLogger("matplotlib.font_manager").setLevel(logging.WARNING) + + +def _configure_matplotlib_cache() -> None: + """Point matplotlib's config/cache directory at a writable persistent path. + + Without this, matplotlib falls back to ``/tmp/matplotlib-XXXXXX`` whenever + ``$HOME/.config/matplotlib`` isn't writable — which is the case under + Bambuddy's container / systemd-service deployments where ``$HOME`` is set + to a non-writable path. The fallback emits a WARNING on every cold start + AND loses the font cache on host reboot, so font_manager rebuilds it + every time → another batch of INFO lines. + + Setting ``MPLCONFIGDIR`` to ``settings.base_dir / .cache / matplotlib`` + eliminates both: the warning never fires, and the cache survives across + restarts so the per-font scan only runs once per deployment. + Idempotent — respects an externally-set ``MPLCONFIGDIR`` if the operator + chose their own path. + """ + if os.environ.get("MPLCONFIGDIR"): + return + try: + from backend.app.core.config import settings + + cache_dir = Path(settings.base_dir) / ".cache" / "matplotlib" + cache_dir.mkdir(parents=True, exist_ok=True) + os.environ["MPLCONFIGDIR"] = str(cache_dir) + except Exception as exc: + # Best-effort. If settings isn't importable or the mkdir fails (read-only + # FS, permission denied), let matplotlib fall back to /tmp with its + # built-in warning — same as today's behaviour, no worse. + logger.debug("Could not configure MPLCONFIGDIR: %s", exc) + + # Bambu green color for rendering BAMBU_GREEN = "#00AE42" BACKGROUND_COLOR = "#1a1a1a" @@ -16,6 +57,14 @@ BACKGROUND_COLOR = "#1a1a1a" # Maximum vertices before simplification MAX_VERTICES = 100000 +# Minimum STL file size that could possibly contain a usable mesh: +# - Binary STL with one triangle: 80B header + 4B count + 50B triangle = 134B +# - ASCII STL with one triangle: header + "facet ... endfacet" + footer ≈ 150B +# Files below this are stubs / placeholders / corrupted; trimesh would return an +# empty mesh anyway. Pre-skipping at the call sites suppresses the warning storm +# bulk-uploaded ZIPs of small test STLs used to produce. +MIN_USABLE_STL_BYTES = 200 + def generate_stl_thumbnail( stl_path: Path, @@ -39,6 +88,10 @@ def generate_stl_thumbnail( thumbnails_dir = Path(thumbnails_dir) try: + # Must precede the matplotlib import — MPLCONFIGDIR is read at + # matplotlib import time, not on subsequent attribute access. + _configure_matplotlib_cache() + import matplotlib import trimesh @@ -52,7 +105,14 @@ def generate_stl_thumbnail( mesh = trimesh.load(str(stl_path), force="mesh") if mesh is None or not hasattr(mesh, "vertices") or len(mesh.vertices) == 0: - logger.warning("Failed to load STL or empty mesh: %s", stl_path) + # Demoted from warning to debug: this is a per-file content + # observation (the STL is empty / stub / corrupted), not an + # actionable error. The caller proceeds correctly with no + # thumbnail. The call sites also pre-skip files below + # MIN_USABLE_STL_BYTES so the common stub-STL case never gets + # this far — this branch now catches only the rare "large + # enough but trimesh still can't parse it" case. + logger.debug("Failed to load STL or empty mesh: %s", stl_path) return None # Simplify large meshes for performance @@ -122,7 +182,7 @@ def generate_stl_thumbnail( # Save thumbnail thumb_filename = f"{uuid.uuid4().hex}.png" - thumb_path = thumbnails_dir / thumb_filename + thumb_path = thumbnails_dir / thumb_filename # SEC-PATH-OK: thumb_filename = uuid.uuid4().hex + ".png" fig.savefig( thumb_path, diff --git a/backend/app/services/virtual_printer/ftp_server.py b/backend/app/services/virtual_printer/ftp_server.py index bc6e468ec..55cefdfe6 100644 --- a/backend/app/services/virtual_printer/ftp_server.py +++ b/backend/app/services/virtual_printer/ftp_server.py @@ -414,7 +414,9 @@ class FTPSession: return filename = Path(arg).name # Sanitize filename - file_path = self.upload_dir / filename + file_path = ( + self.upload_dir / filename + ) # SEC-PATH-OK: filename = Path(arg).name strips every path component above logger.info("FTP receiving file: %s from %s", filename, self.remote_ip) diff --git a/backend/app/utils/safe_path.py b/backend/app/utils/safe_path.py new file mode 100644 index 000000000..5afd83c71 --- /dev/null +++ b/backend/app/utils/safe_path.py @@ -0,0 +1,114 @@ +"""Containment-checked path joining. + +Single source of truth for joining a user-controlled string under a trusted +parent directory. The two-vector arbitrary-file-write reported against +``backend/app/api/routes/projects.py::import_project_file`` traced to plain +``Path / user_string`` arithmetic with no resolve + containment check — +attacker passed an absolute path, ``Path("/lib") / "/etc"`` collapsed to +``Path("/etc")``, and the next ``write_bytes`` landed wherever the attacker +chose. This module is the answer. + +Every site that joins a path component coming from a request body, a ZIP +``namelist()``, an ``UploadFile.filename``, or any other attacker-controlled +source MUST route through ``safe_join_under``. Sites that join trusted +constants (settings paths, hardcoded subdirs) are not in scope — those should +carry a ``# SEC-PATH-OK: `` marker so the CI backstop knows. +""" + +from __future__ import annotations + +from pathlib import Path + +from fastapi import HTTPException + + +class PathTraversalError(ValueError): + """Raised when a join attempt would escape the trusted parent. + + Callers in API-route context catch this and translate to ``HTTPException`` + via ``safe_join_under`` (which already raises HTTPException directly when + invoked with ``http=True``). Non-route callers can catch the + ``PathTraversalError`` and decide their own response shape. + """ + + +def safe_join_under(parent: Path, *parts: str, http: bool = True) -> Path: + """Join *parts* under *parent* and assert the result stays under it. + + Rejects: + - empty / None / non-str parts; + - parts containing NUL (``\\x00``); + - parts starting with ``/`` or ``\\`` (absolute paths; + ``Path("/lib") / "/etc"`` discards ``/lib``); + - any sequence whose resolved form is not a descendant of *parent*'s + resolved form (defeats ``..`` traversal even when the literal join + doesn't look suspicious). + + Returns the resolved absolute path on success. + + When ``http=True`` (default; suitable for FastAPI routes), failures raise + ``HTTPException(400, "Invalid path in upload")``. Set ``http=False`` to + raise ``PathTraversalError`` instead — for non-route callers that need + finer control over the response. + """ + if not parts: + _fail("safe_join_under called with no parts", http) + + for part in parts: + if not isinstance(part, str): + _fail(f"Path part has type {type(part).__name__}, expected str", http) + if not part: + _fail("Empty path part", http) + if "\x00" in part: + _fail("NUL byte in path part", http) + # Reject literal absolute markers: pathlib collapses ``Path("/a") / + # "/b"`` to ``Path("/b")`` so the catch-after-resolve below would also + # fire, but rejecting up-front gives a clearer error and avoids + # touching the filesystem. + if part.startswith("/") or part.startswith("\\"): + _fail("Absolute path part not allowed", http) + + parent_resolved = parent.resolve() + candidate = parent + for part in parts: + candidate = candidate / part + candidate_resolved = candidate.resolve() + + if not _is_relative_to(candidate_resolved, parent_resolved): + _fail("Path escapes the parent directory", http) + + return candidate_resolved + + +def assert_under(parent: Path, candidate: Path, *, http: bool = True) -> Path: + """Assert that an already-joined *candidate* path is under *parent*. + + Use when you have an existing ``Path`` (e.g. from another helper that + builds the path itself) and need a containment check before writing or + deleting. Equivalent to ``safe_join_under`` minus the per-part input + validation. + """ + parent_resolved = parent.resolve() + candidate_resolved = candidate.resolve() + if not _is_relative_to(candidate_resolved, parent_resolved): + _fail("Path escapes the parent directory", http) + return candidate_resolved + + +def _is_relative_to(child: Path, parent: Path) -> bool: + # ``Path.is_relative_to`` exists in Python 3.9+. Bambuddy targets 3.11+ + # (per pyproject and the bug-report system info) so this is safe. + try: + return child.is_relative_to(parent) + except AttributeError: # pragma: no cover - defensive + try: + child.relative_to(parent) + return True + except ValueError: + return False + + +def _fail(reason: str, http: bool) -> None: + if http: + raise HTTPException(status_code=400, detail="Invalid path in upload") + raise PathTraversalError(reason) diff --git a/backend/tests/integration/test_projects_api.py b/backend/tests/integration/test_projects_api.py index fd5921172..fb58b3d14 100644 --- a/backend/tests/integration/test_projects_api.py +++ b/backend/tests/integration/test_projects_api.py @@ -953,3 +953,122 @@ class TestProjectExportImport: response = await async_client.post("/api/v1/projects/import/file", files=files) assert response.status_code == 400 assert "Invalid JSON" in response.json()["detail"] + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_import_rejects_absolute_path_in_folder_name(self, async_client: AsyncClient, tmp_path): + """Absolute paths in `linked_folders[*].name` must not escape library_dir. + + Verbatim shape from the upstream advisory: attacker sets folder name to + an absolute path, expecting Python's ``Path("/lib") / "/anywhere"`` to + collapse to ``Path("/anywhere")`` and let the next file write land + outside the library directory. + """ + import io + import json + import zipfile + + target_outside = tmp_path / "outside" / "owned" + # Build a ZIP whose folder name points outside library_dir entirely. + zip_buffer = io.BytesIO() + with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr( + "project.json", + json.dumps( + { + "name": "innocent", + "linked_folders": [{"name": str(target_outside)}], + } + ), + ) + zf.writestr(f"files/{target_outside}/evil.pth", b"import os; os.system('echo pwned > /tmp/owned')\n") + + zip_buffer.seek(0) + files = {"file": ("evil.zip", zip_buffer, "application/zip")} + response = await async_client.post("/api/v1/projects/import/file", files=files) + assert response.status_code == 400, response.text + assert not target_outside.exists(), "Attacker payload landed outside library_dir" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_import_rejects_dotdot_in_folder_name(self, async_client: AsyncClient): + """`..` segments in folder name must be rejected.""" + import io + import json + import zipfile + + zip_buffer = io.BytesIO() + with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr( + "project.json", + json.dumps( + { + "name": "innocent", + "linked_folders": [{"name": "../../../etc"}], + } + ), + ) + zf.writestr("files/../../../etc/x.txt", b"x") + + zip_buffer.seek(0) + files = {"file": ("evil.zip", zip_buffer, "application/zip")} + response = await async_client.post("/api/v1/projects/import/file", files=files) + assert response.status_code == 400, response.text + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_import_rejects_dotdot_in_relative_path(self, async_client: AsyncClient): + """`..` segments in the per-entry path (Vector B in the advisory) must + be rejected even when the folder name itself is fine.""" + import io + import json + import zipfile + + zip_buffer = io.BytesIO() + with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr( + "project.json", + json.dumps( + { + "name": "innocent", + "linked_folders": [{"name": "ok"}], + } + ), + ) + # Folder name is benign, but the file path inside attempts to + # escape via ``..``. + zf.writestr("files/ok/../../../etc/x.txt", b"x") + + zip_buffer.seek(0) + files = {"file": ("evil.zip", zip_buffer, "application/zip")} + response = await async_client.post("/api/v1/projects/import/file", files=files) + assert response.status_code == 400, response.text + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_import_legit_nested_zip_still_works(self, async_client: AsyncClient): + """A legitimate ZIP with a nested file path inside the folder must + continue to import cleanly. Guards against the fix being over-strict.""" + import io + import json + import zipfile + + zip_buffer = io.BytesIO() + with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr( + "project.json", + json.dumps( + { + "name": "nested-ok", + "linked_folders": [{"name": "OkFolder"}], + } + ), + ) + zf.writestr("files/OkFolder/sub/dir/inside.txt", b"hello") + + zip_buffer.seek(0) + files = {"file": ("nested.zip", zip_buffer, "application/zip")} + response = await async_client.post("/api/v1/projects/import/file", files=files) + assert response.status_code == 200, response.text + data = response.json() + assert data["name"] == "nested-ok" diff --git a/backend/tests/unit/services/test_attach_timelapse_safe_path.py b/backend/tests/unit/services/test_attach_timelapse_safe_path.py new file mode 100644 index 000000000..fc10e7224 --- /dev/null +++ b/backend/tests/unit/services/test_attach_timelapse_safe_path.py @@ -0,0 +1,116 @@ +"""Regression tests for ArchiveService.attach_timelapse path-traversal guard. + +``filename`` ultimately comes from a printer's FTP listing or a query +parameter on ``POST /archives/{id}/timelapse/select``. A compromised printer +that returns a malicious filename (e.g. ``"../../etc/passwd"``) used to land +the write outside the archive directory. The safe-join helper now rejects +such names; this test locks the behaviour in. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from backend.app.services.archive import ArchiveService + + +@pytest.mark.asyncio +async def test_attach_timelapse_rejects_dotdot_filename(tmp_path: Path, monkeypatch): + """A ``..`` traversal in filename must not land bytes outside archive_dir.""" + # Stage an archive directory that the service thinks is owned. + archive_dir = tmp_path / "archive" / "1" / "20260101_test" + archive_dir.mkdir(parents=True) + # Repoint settings.base_dir so attach_timelapse's archive_dir = file_path.parent + # resolves to our tmp directory. + monkeypatch.setattr( + "backend.app.services.archive.settings", + MagicMock(base_dir=tmp_path), + ) + + db = MagicMock() + db.commit = AsyncMock() + service = ArchiveService(db) + + # Mock the archive lookup to return a row whose file_path resolves under tmp_path. + fake_archive = MagicMock() + fake_archive.file_path = "archive/1/20260101_test/file.3mf" + service.get_archive = AsyncMock(return_value=fake_archive) + + # The attacker-controlled filename in the threat model. + malicious = "../../etc/passwd_pwned" + + result = await service.attach_timelapse( + archive_id=1, + timelapse_data=b"would-be-attacker-payload", + filename=malicious, + ) + + # The helper rejected the join → service returns False. + assert result is False + # And no payload landed at the target outside archive_dir. + target_outside = tmp_path / "etc" / "passwd_pwned" + assert not target_outside.exists(), "Attacker payload landed outside archive_dir" + # And no payload landed under archive_dir either (since we rejected before write). + assert not list(archive_dir.glob("*")) + + +@pytest.mark.asyncio +async def test_attach_timelapse_rejects_absolute_filename(tmp_path: Path, monkeypatch): + """An absolute path in filename must not collapse the join.""" + archive_dir = tmp_path / "archive" / "1" / "20260101_test" + archive_dir.mkdir(parents=True) + monkeypatch.setattr( + "backend.app.services.archive.settings", + MagicMock(base_dir=tmp_path), + ) + + db = MagicMock() + db.commit = AsyncMock() + service = ArchiveService(db) + + fake_archive = MagicMock() + fake_archive.file_path = "archive/1/20260101_test/file.3mf" + service.get_archive = AsyncMock(return_value=fake_archive) + + result = await service.attach_timelapse( + archive_id=1, + timelapse_data=b"x", + filename="/tmp/owned_via_absolute", + ) + + assert result is False + assert not Path("/tmp/owned_via_absolute").exists() + + +@pytest.mark.asyncio +async def test_attach_timelapse_accepts_legit_filename(tmp_path: Path, monkeypatch): + """The legitimate happy path must still work — the fix isn't over-strict.""" + archive_dir = tmp_path / "archive" / "1" / "20260101_test" + archive_dir.mkdir(parents=True) + monkeypatch.setattr( + "backend.app.services.archive.settings", + MagicMock(base_dir=tmp_path), + ) + + db = MagicMock() + db.commit = AsyncMock() + service = ArchiveService(db) + + fake_archive = MagicMock() + fake_archive.file_path = "archive/1/20260101_test/file.3mf" + fake_archive.timelapse_path = None + service.get_archive = AsyncMock(return_value=fake_archive) + + result = await service.attach_timelapse( + archive_id=1, + timelapse_data=b"hello-timelapse", + filename="timelapse_2026-01-01_12-00-00.mp4", + ) + + assert result is True + landed = archive_dir / "timelapse_2026-01-01_12-00-00.mp4" + assert landed.exists() + assert landed.read_bytes() == b"hello-timelapse" diff --git a/backend/tests/unit/services/test_stl_thumbnail.py b/backend/tests/unit/services/test_stl_thumbnail.py index 59ab3c473..a2e43934e 100644 --- a/backend/tests/unit/services/test_stl_thumbnail.py +++ b/backend/tests/unit/services/test_stl_thumbnail.py @@ -1,5 +1,6 @@ """Unit tests for the STL thumbnail service.""" +import os import tempfile from pathlib import Path @@ -230,3 +231,97 @@ class TestStlThumbnailConstants: from backend.app.services.stl_thumbnail import MAX_VERTICES assert MAX_VERTICES == 100000 + + def test_min_usable_stl_bytes_threshold(self): + """MIN_USABLE_STL_BYTES is the call-site pre-skip floor. + + Binary STL with one triangle = 80B header + 4B count + 50B triangle + = 134B. ASCII STL with one triangle ≈ 150B. Anything below this size + cannot contain a usable mesh. + """ + from backend.app.services.stl_thumbnail import MIN_USABLE_STL_BYTES + + assert MIN_USABLE_STL_BYTES == 200 + # Verify it sits between "smaller than smallest real STL" and + # "common stub size" — the 24-byte ``solid test\nendsolid test`` + # stubs that triggered the warning storm. + assert MIN_USABLE_STL_BYTES > 134 # smallest binary STL with one triangle + assert MIN_USABLE_STL_BYTES > 150 # smallest ASCII STL with one triangle + assert MIN_USABLE_STL_BYTES > 24 # the ZIP-stub case in the bug report + + def test_font_manager_logger_demoted_to_warning(self): + """matplotlib.font_manager's per-font INFO scan is demoted at module + import so the first STL upload doesn't surface a multi-line preamble + of matplotlib internals in the journal.""" + import logging + + # Importing the module sets the level as a side effect. + import backend.app.services.stl_thumbnail # noqa: F401 + + assert logging.getLogger("matplotlib.font_manager").level >= logging.WARNING + + def test_configure_matplotlib_cache_sets_mplconfigdir(self, tmp_path, monkeypatch): + """``_configure_matplotlib_cache`` points matplotlib at a writable + persistent path so it doesn't fall back to ``/tmp/matplotlib-XXX`` + on every cold start.""" + from backend.app.services.stl_thumbnail import _configure_matplotlib_cache + + # Ensure we start with no value so the helper actually runs. + monkeypatch.delenv("MPLCONFIGDIR", raising=False) + monkeypatch.setattr( + "backend.app.services.stl_thumbnail.Path", + __import__("pathlib").Path, + ) + + # Stub settings.base_dir to point inside tmp_path. + from backend.app.core import config as core_config + + monkeypatch.setattr(core_config.settings, "base_dir", tmp_path, raising=False) + + _configure_matplotlib_cache() + + assert "MPLCONFIGDIR" in os.environ + configured = Path(os.environ["MPLCONFIGDIR"]) + assert configured.exists() + assert configured.is_dir() + # And the directory sits under base_dir, not /tmp/matplotlib-XXX. + assert tmp_path in configured.parents + + def test_configure_matplotlib_cache_respects_externally_set_value(self, tmp_path, monkeypatch): + """If the operator (or container init) has set MPLCONFIGDIR already, + the helper must leave it alone — they made a deliberate choice.""" + from backend.app.services.stl_thumbnail import _configure_matplotlib_cache + + external = str(tmp_path / "external-mpl-cache") + monkeypatch.setenv("MPLCONFIGDIR", external) + _configure_matplotlib_cache() + assert os.environ["MPLCONFIGDIR"] == external + + def test_empty_mesh_logged_at_debug_not_warning(self, caplog): + """An empty STL (header present, no triangles) must log at DEBUG, not + WARNING — bulk uploads used to log thousands of WARNING lines per + ZIP. Per-file content observations stay observable in debug logs + but don't spam production journals.""" + import logging + import tempfile + from pathlib import Path + + from backend.app.services.stl_thumbnail import generate_stl_thumbnail + + # The exact 24-byte stub from the bug report + stub_content = b"solid test\nendsolid test" + + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir_path = Path(tmpdir) + stl_path = tmpdir_path / "stub.stl" + stl_path.write_bytes(stub_content) + + with caplog.at_level(logging.DEBUG, logger="backend.app.services.stl_thumbnail"): + result = generate_stl_thumbnail(stl_path, tmpdir_path) + + assert result is None + # The empty-mesh message must NOT appear at WARNING level. + warning_records = [r for r in caplog.records if r.levelno >= logging.WARNING and "empty mesh" in r.getMessage()] + assert warning_records == [], ( + f"Empty-mesh path still logs at WARNING: {[r.getMessage() for r in warning_records]}" + ) diff --git a/backend/tests/unit/test_no_unsafe_path_joins.py b/backend/tests/unit/test_no_unsafe_path_joins.py new file mode 100644 index 000000000..dd3fb5cea --- /dev/null +++ b/backend/tests/unit/test_no_unsafe_path_joins.py @@ -0,0 +1,203 @@ +"""Backstop: every Path-arithmetic site in the API routes that joins a +variable to a directory-like parent must either use ``safe_join_under`` or +carry a ``# SEC-PATH-OK: `` marker. + +A critical advisory traced to plain ``Path / user_string`` arithmetic in +``import_project_file`` — the join had no resolve + containment check, and an +attacker-supplied absolute path collapsed the left side. This test catches the +same shape in any new route added later: it AST-walks every Python file under +``backend/app/api/routes/`` and flags every ``a / b`` where ``a`` looks like a +directory variable and ``b`` is a non-constant (i.e. variable / call result). + +False positives are intentionally cheap to silence (add a one-line +``# SEC-PATH-OK: `` justifying the existing guard) so that *future* +unsafe joins are noisy by default. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +# The route surface receives external input directly; the services layer is +# called by the routes and routinely receives values that originated from a +# request (filenames, query params) or from an untrusted external source +# (printer FTP listings — the printer is part of the threat surface in the +# compromised-printer model). Both layers need the strictest gate. +_BACKEND_APP = Path(__file__).resolve().parents[2] / "app" +SCAN_DIRS = [ + _BACKEND_APP / "api" / "routes", + _BACKEND_APP / "services", +] + +# Identifier substrings that suggest the LHS is a filesystem directory. Heuristic +# but tuned to Bambuddy's conventions — every actual directory variable in the +# routes hits one of these. +_DIR_NAME_HINTS = ( + "_dir", + "_path", + "dir_", + "path_", + "temp_path", + "library_dir", + "archive_dir", + "photos_dir", + "base_dir", + "ext_dir", + "attachments_dir", + "static_dir", + "log_dir", + "data_dir", + "folder_path", + "file_disk_path", + "photo_path", + "dest", + "output_path", +) + +# Function calls whose return value is a Path under our control. Hits to these +# don't need scrutiny — they're constructed by Bambuddy code, not by the request. +_KNOWN_PATH_FACTORIES = ( + "Path", + "get_library_dir", + "get_library_files_dir", + "get_archive_dir", + "get_project_attachments_dir", + "get_project_cover_dir", + "resolve", +) + +_MARKER = "# SEC-PATH-OK:" + + +def _looks_path_like(node: ast.AST) -> bool: + """Heuristic for whether *node* evaluates to a ``pathlib.Path``.""" + if isinstance(node, ast.Call): + func = node.func + if isinstance(func, ast.Name) and func.id in _KNOWN_PATH_FACTORIES: + return True + return bool(isinstance(func, ast.Attribute) and func.attr in _KNOWN_PATH_FACTORIES) + if isinstance(node, ast.Name): + return any(hint in node.id for hint in _DIR_NAME_HINTS) + if isinstance(node, ast.Attribute): + # `settings.base_dir`, `cls.archive_dir`, etc. + return any(hint in node.attr for hint in _DIR_NAME_HINTS) + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Div): + # Chains like ``base_dir / "x" / variable`` — keep looking left. + return _looks_path_like(node.left) + return False + + +def _is_constant_string(node: ast.AST) -> bool: + return isinstance(node, ast.Constant) and isinstance(node.value, str) + + +def _rhs_is_attacker_shape(node: ast.AST) -> bool: + """The high-risk shape is ``path / Name`` — RHS is a bare variable that + came from somewhere outside this scope (a function parameter, request + body field, ZIP namelist entry). + + Attribute (``lib_file.file_path``), Subscript (``photos[i]``), Call + (``str(vp_id)``), and JoinedStr (f-strings) all have *some* structure + that the audit can reason about — those are caught by the broader audit + sweep, not the regression backstop. This narrows the noise to the exact + shape that produced the path-traversal class so the backstop only fires + when something that *looks* like that bug appears. + """ + return isinstance(node, ast.Name) + + +def _line_has_marker(source_lines: list[str], lineno: int, end_lineno: int | None) -> bool: + # Walk every line spanned by the BinOp — the marker can sit anywhere in the + # expression (start, end, or any continuation line of a multi-line join). + start = max(1, lineno) + end = max(start, end_lineno or lineno) + for i in range(start, end + 1): + if i - 1 >= len(source_lines): + continue + if _MARKER in source_lines[i - 1]: + return True + return False + + +def _enclosing_call_is_safe_join(stack: list[ast.AST]) -> bool: + """True if the BinOp is being passed directly into ``safe_join_under(...)``. + + Tracking parent links keeps the test conservative — a ``base_dir / x`` + expression that's already inside ``safe_join_under(base_dir / x, ...)`` + is fine because the helper does its own containment check. This rarely + happens in practice but keeps the test from yelling about an idiomatic + arrangement. + """ + for ancestor in reversed(stack): + if isinstance(ancestor, ast.Call): + func = ancestor.func + if isinstance(func, ast.Name) and func.id == "safe_join_under": + return True + if isinstance(func, ast.Attribute) and func.attr == "safe_join_under": + return True + return False + + +def _scan_file(py_file: Path) -> list[str]: + source = py_file.read_text() + source_lines = source.splitlines() + tree = ast.parse(source, filename=str(py_file)) + findings: list[str] = [] + + # Walk with parent stack so we can detect "inside safe_join_under" and + # skip such nodes. + stack: list[ast.AST] = [] + + def visit(node: ast.AST) -> None: + stack.append(node) + try: + if ( + isinstance(node, ast.BinOp) + and isinstance(node.op, ast.Div) + and _looks_path_like(node.left) + and _rhs_is_attacker_shape(node.right) + and not _is_constant_string(node.right) + and not _enclosing_call_is_safe_join(stack) + and not _line_has_marker(source_lines, node.lineno, node.end_lineno) + ): + line = source_lines[node.lineno - 1].strip() if node.lineno - 1 < len(source_lines) else "" + findings.append(f"{py_file.name}:{node.lineno} {line}") + for child in ast.iter_child_nodes(node): + visit(child) + finally: + stack.pop() + + visit(tree) + return findings + + +def test_route_path_arithmetic_is_safe_joined_or_marked(): + """Every `` / `` join in a route handler must + either route through ``safe_join_under(...)`` or carry a + ``# SEC-PATH-OK: `` marker on one of its source lines. + + Adding ``# SEC-PATH-OK: `` is the escape hatch for sites where + the input has already been validated (e.g. a denylist + membership + check, a pre-sanitised alphanumeric filter, or an explicit resolve + + ``relative_to`` containment check inline). The marker MUST explain the + existing guard — silent suppression defeats the backstop's purpose. + """ + findings: list[str] = [] + for scan_dir in SCAN_DIRS: + for py_file in sorted(scan_dir.rglob("*.py")): + if py_file.name == "__init__.py": + continue + findings.extend(_scan_file(py_file)) + + if findings: + pytest.fail( + "Found Path-arithmetic sites in api/routes/ or services/ that " + "join a non-constant value to a directory-like parent without " + "using safe_join_under() or carrying a # SEC-PATH-OK: marker. " + "Each site must either be refactored to " + "safe_join_under(parent, *parts) or tagged with the marker " + "explaining why the existing guard is sufficient.\n\nFindings:\n" + "\n".join(findings) + ) diff --git a/backend/tests/unit/test_safe_path.py b/backend/tests/unit/test_safe_path.py new file mode 100644 index 000000000..2b731c506 --- /dev/null +++ b/backend/tests/unit/test_safe_path.py @@ -0,0 +1,124 @@ +"""Tests for ``backend.app.utils.safe_path.safe_join_under``. + +Cover every escape vector documented in the helper plus the legitimate +nested-path use case so the helper's behaviour is locked in. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from fastapi import HTTPException + +from backend.app.utils.safe_path import ( + PathTraversalError, + assert_under, + safe_join_under, +) + + +@pytest.fixture() +def library(tmp_path: Path) -> Path: + """A real on-disk directory that mimics the "trusted parent" role.""" + lib = tmp_path / "library" + lib.mkdir() + return lib + + +class TestSafeJoinUnder: + def test_simple_filename_is_joined(self, library: Path): + result = safe_join_under(library, "model.3mf") + assert result == (library / "model.3mf").resolve() + + def test_nested_path_components_are_joined(self, library: Path): + result = safe_join_under(library, "myfolder", "sub", "file.3mf") + assert result == (library / "myfolder" / "sub" / "file.3mf").resolve() + + def test_absolute_path_rejected(self, library: Path): + # The exact shape that produced the original CVE — ``Path("/lib") / "/etc/passwd"`` + # collapses to ``Path("/etc/passwd")`` in Python's pathlib. + with pytest.raises(HTTPException) as exc: + safe_join_under(library, "/etc/passwd") + assert exc.value.status_code == 400 + + def test_absolute_windows_path_rejected(self, library: Path): + with pytest.raises(HTTPException): + safe_join_under(library, "\\\\evil\\share\\x") + + def test_parent_traversal_rejected(self, library: Path): + with pytest.raises(HTTPException): + safe_join_under(library, "..", "etc", "passwd") + + def test_embedded_parent_traversal_rejected(self, library: Path): + # ``library/foo/../../etc/passwd`` resolves outside ``library``. + with pytest.raises(HTTPException): + safe_join_under(library, "foo", "..", "..", "etc", "passwd") + + def test_null_byte_rejected(self, library: Path): + with pytest.raises(HTTPException): + safe_join_under(library, "evil\x00.3mf") + + def test_empty_string_part_rejected(self, library: Path): + with pytest.raises(HTTPException): + safe_join_under(library, "") + + def test_no_parts_rejected(self, library: Path): + with pytest.raises(HTTPException): + safe_join_under(library) + + def test_non_string_part_rejected(self, library: Path): + with pytest.raises(HTTPException): + safe_join_under(library, 42) # type: ignore[arg-type] + + def test_http_false_raises_path_traversal_error(self, library: Path): + with pytest.raises(PathTraversalError): + safe_join_under(library, "/etc/passwd", http=False) + + def test_http_false_allows_clean_join(self, library: Path): + result = safe_join_under(library, "ok.txt", http=False) + assert result == (library / "ok.txt").resolve() + + def test_returned_path_is_resolved(self, library: Path): + # The helper returns a resolved path so callers don't need to do it + # themselves — every downstream is_relative_to/parent check assumes + # a canonical form. + result = safe_join_under(library, "x.txt") + assert result == result.resolve() + + +class TestAssertUnder: + def test_inside_passes(self, library: Path): + candidate = library / "x" / "y" / "z.txt" + out = assert_under(library, candidate) + assert out == candidate.resolve() + + def test_outside_rejects(self, library: Path, tmp_path: Path): + outside = tmp_path / "elsewhere" / "evil.txt" + with pytest.raises(HTTPException): + assert_under(library, outside) + + def test_outside_raises_path_traversal_error_with_http_false(self, library: Path, tmp_path: Path): + outside = tmp_path / "elsewhere" / "evil.txt" + with pytest.raises(PathTraversalError): + assert_under(library, outside, http=False) + + +class TestPocReproducer: + """The exact attacker payload from the advisory. + + A directly attacker-controlled folder name pointing at a venv's + site-packages directory used to land a ``.pth`` file on disk. With the + helper in place the join now raises before any write. + """ + + def test_advisory_poc_target_dir_rejected(self, library: Path): + # Verbatim shape from the advisory POC. + target_dir = "BAMBUDDY_BASE_DIR/bambuddy/venv/lib/python3.14/site-packages" + # Leading slash → absolute → rejected up-front. + with pytest.raises(HTTPException): + safe_join_under(library, "/" + target_dir) + # No leading slash but with ``..`` traversal embedded in the + # follow-up file path — also rejected. + with pytest.raises(HTTPException): + safe_join_under(library, "innocent", "..", "..", "evil.pth")