From f5ecc61cda949db5bcf2539fa03cbd62c64091d2 Mon Sep 17 00:00:00 2001 From: maziggy Date: Fri, 8 May 2026 14:28:41 +0200 Subject: [PATCH] fix(spoolbuddy): lower /update permission to INVENTORY_UPDATE so kiosk's own Settings -> Update button works MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kiosk's Settings -> Update Daemon button returned "API keys cannot be used for administrative operations" because POST /spoolbuddy/devices/ {id}/update was gated on Permission.SETTINGS_UPDATE, and SETTINGS_UPDATE is in the _APIKEY_DENIED_PERMISSIONS deny-list introduced by PR #1241. Every kiosk-side request tripped the deny-list before the API key's scope set (Read / Print Queue / Control / Legacy) was even consulted. Same root cause as the four QuickMenu System buttons fixed in 0.2.4b3 (Restart Daemon / Restart Browser / Reboot / Shutdown). Missed /update in that audit on the reasoning "replaces the daemon binary, different threat surface" — but that's wrong: restart_daemon already replaces the running daemon process, so daemon-replacement is not a step up in blast radius. The SSH update is also strictly scoped to the one device the operator physically controls (git fetch + pip install + systemctl restart on that host) — same threat profile as the system commands already running on INVENTORY_UPDATE. Lower /spoolbuddy/devices/{id}/update from SETTINGS_UPDATE to INVENTORY_UPDATE so it aligns with the rest of the kiosk-scoped routes (calibration/tare, display, cancel-write, system/command, system/command-result, update-status). The main Bambuddy in-app updater at POST /api/v1/updates/apply keeps SETTINGS_UPDATE — that one runs on the Bambuddy host and is correctly fenced behind the deny-list. --- CHANGELOG.md | 4 ++ backend/app/api/routes/spoolbuddy.py | 9 +++- backend/app/core/database.py | 5 +++ backend/app/models/spoolbuddy_device.py | 2 +- backend/app/services/spoolbuddy_ssh.py | 10 +++-- .../test_settings_api_key_scrubbing.py | 34 +++++++++----- backend/tests/unit/test_spoolbuddy_ssh.py | 45 +++++++++++++++++++ spoolbuddy/install/install.sh | 8 ++++ 8 files changed, 100 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa45b932b..afb55e31d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.4b4] - Unreleased ### Fixed +- **SpoolBuddy install.sh re-run failed with `Permission denied` on root-owned files in update mode** — `download_spoolbuddy()` ran `git fetch + git checkout + git reset --hard` *before* the post-install chown at the end of the function. If a previous install left stray root-owned files in the tree (e.g. `static/assets/*` written by an earlier `sudo` run, or a frontend build that wrote as root), the `git reset --hard` step aborted with EACCES on the unlink/replace step before reaching the chown. The script then exited and the kiosk's underlying ownership problem persisted, so the next attempt would fail the same way. **Fix:** pre-emptively `chown -R spoolbuddy:spoolbuddy "$INSTALL_PATH"` in the update branch *before* any git operation runs. The script already runs as root (enforced by `check_root`), so the chown is always safe. The existing post-install chown at the end stays — it now mostly catches new files created during this run that need their ownership normalised. Same root cause showed up on the kiosk's *runtime* SSH update path (Bambuddy → kiosk: `git checkout dev && git reset --hard origin/dev` running as the `spoolbuddy` user) but that path can't `chown` without sudoers expansion — the install.sh fix is the immediate recovery, and re-running the install script restores a clean ownership baseline that the runtime updater can keep healthy thereafter. +- **SpoolBuddy SSH update aborted with `TypeError: startswith first arg must be bytes or a tuple of bytes, not str` after the host-key store succeeded** — `perform_ssh_update` calls `asyncssh.import_known_hosts(...)` to materialise an `SSHKnownHosts` object for `_run_ssh_command`'s `known_hosts=` keyword arg. Both call sites (the stored-key path at line 221 and the just-stored TOFU re-parse at line 272) passed `f"{ip} {key}\n".encode()` — i.e. `bytes`. asyncssh's parser does line-based string operations (`line.startswith('#')` with a `str` literal), so any `bytes` input crashes inside its loader with `TypeError`. The two `try`/`except` clauses caught only `(ValueError, asyncssh.Error)`, missing `TypeError`, so the crash bubbled up and aborted the whole update right after the schema fix successfully persisted the host key. **Fix:** drop the `.encode()` at both call sites — pass the str directly. Widened both except clauses to `(ValueError, TypeError, asyncssh.Error)` so any future asyncssh API surprise degrades to the existing fallback (TOFU mode without host-key verification, with a logger.warning) instead of crashing the update. Existing SSH tests all mocked `asyncssh.import_known_hosts` itself so they never reached the parser — added `test_perform_ssh_update_passes_str_not_bytes_to_import_known_hosts` to capture both call sites' arguments and assert `isinstance(arg, str)` so re-introducing `.encode()` fails CI immediately. +- **SpoolBuddy SSH update crashed on Postgres with `value too long for type character varying(500)` when storing the device's RSA host key** — `spoolbuddy_devices.ssh_host_key` was declared as `String(500)`, which is fine for SQLite (ignores VARCHAR length) and for ed25519 host keys (~120 chars), but RSA host keys in OpenSSH format are typically 370 chars (2048-bit) → 544 chars (3072-bit) → ~720 chars (4096-bit). Postgres enforces the limit strictly, so any kiosk reporting an RSA-3072 or larger host key on the first SSH update aborted at the `UPDATE spoolbuddy_devices SET ssh_host_key=...` flush — the `git fetch + pip install + systemctl restart` may have run successfully but the persistence of the TOFU host key failed and the device's update_status was never written. **Fix:** widened `ssh_host_key` from `String(500)` → `Text` on the model, plus an idempotent `ALTER TABLE spoolbuddy_devices ALTER COLUMN ssh_host_key TYPE TEXT` migration gated on `not is_sqlite()` (Postgres-only; SQLite is a no-op since it doesn't enforce VARCHAR length). Existing rows are preserved — `TYPE TEXT` is a metadata-only change on Postgres for `VARCHAR(N)` → `TEXT` so it's a fast migration even on populated tables. Originally introduced in the H1 SSH-host-key TOFU security fix; the 500-char floor was a guess based on ed25519 sizes that the RSA case immediately blew past. +- **SpoolBuddy kiosk Settings → Update button returned "API keys cannot be used for administrative operations"** — Same root cause as the four QuickMenu System buttons fixed in 0.2.4b3 (Restart Daemon / Restart Browser / Reboot / Shutdown), missed in that audit. The `POST /spoolbuddy/devices/{id}/update` route (kiosk's own Settings → Update Daemon button → SSH update on the kiosk device) was gated on `Permission.SETTINGS_UPDATE`, but `SETTINGS_UPDATE` is on the API-key deny-list (`_APIKEY_DENIED_PERMISSIONS` in `backend/app/core/auth.py`, introduced in PR #1241). Every kiosk-side request to update the daemon — regardless of the API key's scope set (Read / Print Queue / Control / Legacy) — tripped the deny-list and returned a hard 403 with that message. **The 0.2.4b3 fix explicitly carved /update out** with the reasoning "replaces the daemon binary, different threat surface" — but that reasoning was wrong: `restart_daemon` already replaces the running daemon process, so daemon-replacement is *not* a step up in blast radius. The SSH update is also strictly scoped to the single device the operator physically controls (`git fetch + pip install + systemctl restart` on that one host) — same threat profile as the system commands already running on `INVENTORY_UPDATE`. **Fix:** lower `/spoolbuddy/devices/{id}/update` from `Permission.SETTINGS_UPDATE` → `Permission.INVENTORY_UPDATE`, matching the rest of the kiosk-scoped routes (`calibration/tare`, `display`, `cancel-write`, `system/command`, `system/command-result`, `update-status`). The main Bambuddy in-app updater at `POST /api/v1/updates/apply` keeps `SETTINGS_UPDATE` — that one operates on the Bambuddy host and is correctly fenced behind the deny-list. **Tests:** `test_trigger_update_requires_settings_update` (which pinned the broken behavior — 403 on inventory-only key) is renamed to `test_trigger_update_accepts_inventory_update` and now asserts the inventory-only key reaches the device-state check (409 offline) instead of 403, so a future re-tightening of the gate surfaces immediately. Class-level docstring in `test_settings_api_key_scrubbing.py` updated to reflect the corrected threat-model reasoning. - **Printer file download 500'd on non-ASCII filenames; same crash latent in three sibling endpoints** ([#1245](https://github.com/maziggy/bambuddy/issues/1245), reported by @1000Delta) — `GET /api/v1/printers/{id}/files/download?path=...` raised `UnicodeEncodeError: 'latin-1' codec can't encode characters in position …` for any path whose filename carried non-ASCII characters (Chinese, Japanese, Arabic, accented Latin), reproducible against P2S firmware on macOS but not target-specific. **Cause:** the route shoved `filename` straight into `Content-Disposition: attachment; filename="{filename}"` — Starlette/uvicorn encodes response headers as latin-1, so anything outside U+0000..U+00FF crashed at write-time. Same pattern existed in three sibling endpoints reachable with user-controlled non-ASCII input: `GET /archives/{id}/qr` (uses `archive.print_name` from 3MF metadata, often non-ASCII), `GET /projects/{id}/export` (uses `project.name` — the existing sanitiser at `projects.py:1648` uses `c.isalnum()` which **passes non-ASCII Unicode through**, so the crash propagated), and `_stream_pdf` in `labels.py` (latent — current callers pass ASCII-only template names, but the same shape would crash if a future caller passed user input). **Fix:** new helper `backend/app/utils/http.py::build_content_disposition(filename, disposition="attachment")` returns an RFC 6266-compliant header with both an ASCII-stripped legacy `filename="..."` fallback and an RFC 5987 `filename*=UTF-8''` parameter — every modern browser (Chrome / Firefox / Safari / Edge) prefers the `*=` form when present, so the original filename round-trips intact through Save-As; the ASCII fallback covers IE10-era clients. Helper wired in at all four call sites in one PR (per project rule: no deferred follow-ups). **Tests:** 20 unit tests in `test_http_utils.py` pinning ASCII-fallback rules across plain ASCII / Chinese / Japanese / Arabic / French diacritics / `.gcode.3mf` double-extension / quote-injection / backslash-injection / empty-string and `___.zip` edge cases, asserting the helper's output round-trips through latin-1 (the crash condition) for every test input. 6 new integration tests in `test_printers_api.py::TestPrintersAPI::test_download_printer_file_non_ascii_filename` parametrized over the same character classes (the original `龙泡泡石墩子_p2s_ok.gcode.3mf` case from #1245 is included) — each asserts the route returns 200 with an unmangled body, the ASCII fallback in the header matches expectations, and `unquote(filename*=)` round-trips back to the original Unicode filename. Thanks to @1000Delta for the diagnosis and the proof-of-concept patch on `printers.py` — the broader audit (three sibling endpoints, helper extraction, latin-1 round-trip assertions) was done on top of that. ## [0.2.4b3] - 2026-05-08 diff --git a/backend/app/api/routes/spoolbuddy.py b/backend/app/api/routes/spoolbuddy.py index dc69a7746..16b58a4a0 100644 --- a/backend/app/api/routes/spoolbuddy.py +++ b/backend/app/api/routes/spoolbuddy.py @@ -1319,7 +1319,14 @@ async def trigger_daemon_update( device_id: str, req: dict | None = None, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_UPDATE), + # Aligns with the rest of the kiosk-scoped device routes (calibration, + # display, cancel-write, system/command — all INVENTORY_UPDATE). + # SETTINGS_UPDATE is on the API-key deny-list, which blocks the Update + # button from the kiosk's own Settings page even when the operator has + # physical access. Update only acts on the device the operator already + # controls (git fetch + pip install + systemctl restart on that one + # host) — same blast radius as the restart_daemon command. + _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_UPDATE), ): """Trigger a SpoolBuddy update over SSH. diff --git a/backend/app/core/database.py b/backend/app/core/database.py index bf34a1096..b8aa6ca54 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -1564,6 +1564,11 @@ async def run_migrations(conn): # Migration: Add SSH host key for TOFU verification (H1 security fix) await _safe_execute(conn, "ALTER TABLE spoolbuddy_devices ADD COLUMN ssh_host_key VARCHAR(500)") + # Migration: Widen ssh_host_key from VARCHAR(500) to TEXT — RSA-3072+ host keys + # in OpenSSH format exceed 500 chars (RSA-4096 ~720 chars). PostgreSQL enforces + # the limit and rejects the UPDATE; SQLite ignores VARCHAR length so no-op there. + if not is_sqlite(): + await _safe_execute(conn, "ALTER TABLE spoolbuddy_devices ALTER COLUMN ssh_host_key TYPE TEXT") # Migration: Convert ams_labels table from (printer_id, ams_id) key to ams_serial_number key # Labels are now keyed by AMS serial number so they persist when the AMS is moved to another printer. diff --git a/backend/app/models/spoolbuddy_device.py b/backend/app/models/spoolbuddy_device.py index c6a22da03..76b2da15f 100644 --- a/backend/app/models/spoolbuddy_device.py +++ b/backend/app/models/spoolbuddy_device.py @@ -37,6 +37,6 @@ class SpoolBuddyDevice(Base): scale_ok: Mapped[bool] = mapped_column(Boolean, default=False) uptime_s: Mapped[int] = mapped_column(Integer, default=0) system_stats: Mapped[str | None] = mapped_column(Text, nullable=True) - ssh_host_key: Mapped[str | None] = mapped_column(String(500), nullable=True) + ssh_host_key: Mapped[str | None] = mapped_column(Text, nullable=True) created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now()) updated_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now(), onupdate=func.now()) diff --git a/backend/app/services/spoolbuddy_ssh.py b/backend/app/services/spoolbuddy_ssh.py index ceadd4749..7267d050a 100644 --- a/backend/app/services/spoolbuddy_ssh.py +++ b/backend/app/services/spoolbuddy_ssh.py @@ -218,8 +218,10 @@ async def perform_ssh_update(device_id: str, ip_address: str, install_path: str known_hosts: asyncssh.SSHKnownHosts | None = None if stored_host_key: try: - known_hosts = asyncssh.import_known_hosts(f"{ip_address} {stored_host_key}\n".encode()) - except (ValueError, asyncssh.Error) as exc: + # asyncssh.import_known_hosts() expects str — passing bytes crashes + # inside its line-by-line parser with a TypeError. + known_hosts = asyncssh.import_known_hosts(f"{ip_address} {stored_host_key}\n") + except (ValueError, TypeError, asyncssh.Error) as exc: logger.warning( "Could not parse stored SSH host key for %s, falling back to TOFU: %s", device_id, @@ -269,8 +271,8 @@ async def perform_ssh_update(device_id: str, ip_address: str, install_path: str await db.commit() logger.info("TOFU: stored SSH host key for SpoolBuddy %s", device_id) try: - known_hosts = asyncssh.import_known_hosts(f"{ip_address} {observed_key}\n".encode()) - except (ValueError, asyncssh.Error) as exc: + known_hosts = asyncssh.import_known_hosts(f"{ip_address} {observed_key}\n") + except (ValueError, TypeError, asyncssh.Error) as exc: logger.error( "TOFU: could not parse just-stored host key for %s; " "remaining SSH steps in this run will not verify host key: %s", diff --git a/backend/tests/integration/test_settings_api_key_scrubbing.py b/backend/tests/integration/test_settings_api_key_scrubbing.py index 53576c1f1..ed226c799 100644 --- a/backend/tests/integration/test_settings_api_key_scrubbing.py +++ b/backend/tests/integration/test_settings_api_key_scrubbing.py @@ -107,15 +107,17 @@ class TestSettingsScrubForApiKey: class TestRceEndpointPermissions: - """T-Gap 2 (revised): system_command was originally gated on SETTINGS_UPDATE - but that locked out kiosk operators (who hold INVENTORY_UPDATE-only keys) - from the QuickMenu's Restart-Daemon / Restart-Browser / Reboot / Shutdown - buttons — the only way to recover the kiosk from the kiosk itself. Risk - is bounded: only the 4 named commands are accepted (no RCE), reboot and - shutdown require physical-access recovery, and the same operator already - controls printers + weighs spools on the same device. The /update route - (full firmware upgrade) keeps SETTINGS_UPDATE because it can replace the - daemon binary, which is a different threat surface.""" + """T-Gap 2 (revised): system_command and /update were originally gated on + SETTINGS_UPDATE but that locked out kiosk operators (who hold + INVENTORY_UPDATE-only keys) from the QuickMenu's Restart-Daemon / + Restart-Browser / Reboot / Shutdown buttons and the Settings → Update + button — the only ways to recover or update the kiosk from the kiosk + itself. Risk is bounded the same way for both: actions are scoped to a + single SpoolBuddy device that the operator already physically controls, + daemon-replacement via /update has the same blast radius as + restart_daemon (which also replaces the running process), and the + same operator already controls printers + weighs spools on the same + device. Both routes are now on INVENTORY_UPDATE.""" @pytest.fixture async def auth_enabled(self, db_session): @@ -184,7 +186,7 @@ class TestRceEndpointPermissions: @pytest.mark.asyncio @pytest.mark.integration - async def test_trigger_update_requires_settings_update( + async def test_trigger_update_accepts_inventory_update( self, async_client: AsyncClient, db_session, @@ -192,9 +194,19 @@ class TestRceEndpointPermissions: inventory_only_api_key, spoolbuddy_device, ): + """T-Gap 2 (revised): /update was lowered from SETTINGS_UPDATE to + INVENTORY_UPDATE so the kiosk's own Settings → Update button works. + The deny-list (SETTINGS_UPDATE in _APIKEY_DENIED_PERMISSIONS) was + returning "API keys cannot be used for administrative operations" + for any kiosk-side request to update the daemon. An inventory-only + key must NOT 403 — it should reach the device-state check (and 409 + for offline device, since the fixture doesn't set last_seen). + """ resp = await async_client.post( f"/api/v1/spoolbuddy/devices/{spoolbuddy_device.device_id}/update", json={}, headers={"X-API-Key": inventory_only_api_key}, ) - assert resp.status_code == 403 + # Permission accepted — fails on device-state, not on auth. + assert resp.status_code == 409 + assert "offline" in resp.json()["detail"].lower() diff --git a/backend/tests/unit/test_spoolbuddy_ssh.py b/backend/tests/unit/test_spoolbuddy_ssh.py index 56597d299..896d938e2 100644 --- a/backend/tests/unit/test_spoolbuddy_ssh.py +++ b/backend/tests/unit/test_spoolbuddy_ssh.py @@ -672,3 +672,48 @@ async def test_perform_ssh_update_corrupt_stored_key_falls_back_to_tofu(tmp_path # Broadcast must show success, not error error_broadcasts = [c for c in mock_ws.broadcast.call_args_list if c[0][0].get("update_status") == "error"] assert not error_broadcasts, f"Got unexpected error broadcast: {error_broadcasts}" + + +@pytest.mark.asyncio +async def test_perform_ssh_update_passes_str_not_bytes_to_import_known_hosts(tmp_path): + """asyncssh.import_known_hosts() is a str-only API — passing bytes crashes + inside its line parser (`line.startswith('#')` against a bytes line raises + TypeError). Pin both call sites — the stored-key parse and the just-stored + TOFU re-parse — to ensure we never re-introduce the .encode() bug.""" + ssh_dir = tmp_path / "spoolbuddy" / "ssh" + ssh_dir.mkdir(parents=True) + (ssh_dir / "id_ed25519").write_text("PRIVATE") + (ssh_dir / "id_ed25519.pub").write_text("PUBLIC") + + captured_args: list[object] = [] + + def capture_import(arg): + captured_args.append(arg) + return MagicMock(name="known_hosts") + + async def mock_ssh(ip, cmd, key, *, known_hosts=None, timeout=60): + # Surface a freshly observed key on the first call so the TOFU branch + # also re-imports — exercises the second call site too. + observed = "ssh-rsa AAAAOBSERVED first-tofu" if not captured_args else None + return 0, "ok", "", observed + + mock_device, mock_ctx, mock_ws = _make_update_mocks(tmp_path) + mock_device.ssh_host_key = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5 storedkey" + + with ( + patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings, + patch("backend.app.services.spoolbuddy_ssh._run_ssh_command", side_effect=mock_ssh), + patch("backend.app.services.spoolbuddy_ssh.detect_current_branch", return_value="main"), + patch( + "backend.app.services.spoolbuddy_ssh.asyncssh.import_known_hosts", + side_effect=capture_import, + ), + patch("backend.app.core.database.async_session", return_value=mock_ctx), + patch("backend.app.api.routes.spoolbuddy.ws_manager", mock_ws), + ): + mock_settings.base_dir = tmp_path + await perform_ssh_update("sb-test", "10.0.0.1") + + assert captured_args, "import_known_hosts was never called" + for arg in captured_args: + assert isinstance(arg, str), f"asyncssh.import_known_hosts must receive str, got {type(arg).__name__}: {arg!r}" diff --git a/spoolbuddy/install/install.sh b/spoolbuddy/install/install.sh index 8e5076b55..795204d9f 100755 --- a/spoolbuddy/install/install.sh +++ b/spoolbuddy/install/install.sh @@ -528,6 +528,14 @@ SUDOERS download_spoolbuddy() { if [[ -d "$INSTALL_PATH/.git" ]]; then info "Existing installation found, updating..." + # Pre-emptively normalise tree ownership before git touches anything. + # Stray root-owned files (e.g. static/assets/* left by an earlier + # `sudo` run, or a frontend build that wrote as root) make + # `git reset --hard` fail with EACCES on the unlink/replace step, + # aborting the update before the post-install chown below ever + # runs. The script already runs as root (see check_root), so this + # is always safe. + chown -R "$SPOOLBUDDY_SERVICE_USER:$SPOOLBUDDY_SERVICE_USER" "$INSTALL_PATH" git config --global --add safe.directory "$INSTALL_PATH" 2>/dev/null || true cd "$INSTALL_PATH" git remote set-url origin "$INSTALL_REPO" 2>/dev/null || true