mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-09 15:35:39 +02:00
Seven intertwined SpoolBuddy + Spoolman bugs from feature/spoolman-inventory-ui
testing, fixed as one batch since they all live on the same path:
1. /spoolbuddy/nfc/tag-scanned always tried local DB first and only
consulted Spoolman as a fallback on local-DB miss. A stale local
row silently won over the authoritative Spoolman record. Now gates
on _get_spoolman_client_or_none() so the route uses Spoolman
exclusively when enabled, local exclusively otherwise.
2. Dashboard "Assign to AMS" button was a no-op when the matched
spool wasn't yet in the cached spools query (newly created in
Spoolman, or unarchived after page load). The card rendered via
`displayedSpool ?? sbState.matchedSpool` fallback but the modal's
stricter guard silently failed to mount. New effectiveModalSpool
synthesises an InventorySpool-shaped object from the WebSocket-
delivered MatchedSpool (9-field subset, sufficient for the modal
since it only needs `id` to route the assign API).
3. AMS-page slot picker explicitly returned null for the
assign/unassign branch when a slot had a SpoolmanSlotAssignment
but no tag-linked spool — only Configure stayed visible. Now
resolves the assignment via spoolmanSlotAssignmentsAll +
spoolmanInventorySpoolsCache, renders a "Assigned spool" info
card, and exposes an Unassign button wired to a new
unassignSpoolmanSlotMutation (DELETE
/spoolman/inventory/slot-assignments/<id>).
4. LinkSpoolModal showed "Unknown color" for every Spoolman spool
because Spoolman doesn't standardise color_name — most installs
only populate color_hex and filament.name (which often carries
the colour, e.g. "PLA Basic Red"). _map_spoolman_spool now falls
back to the filament's subtype (filament name minus material
prefix) when color_name is empty, so spools are visually
distinguishable. The NFC write-tag warning specifically checks
the raw filament.color_name (not the mapped value) so the
"tag encodes empty color name" warning still fires on installs
that genuinely lack the field.
5. Writing a tag for spool B didn't clear the same tag from spool A,
so a single NFC UID could map to two spools at once and
find_spool_by_tag returned whichever came first in the cached
list. nfc_write_result now searches Spoolman for any other spool
currently bound to the target UID and clears its extra.tag
(best-effort: cleanup failure logs a warning but doesn't block
the write, since the chip is already written).
6. The kiosk display held stale spoolmanSlotAssignments cache
permanently because a long-running browser window has no
focus/remount triggers to fire a refetch. Adds
refetchInterval: 3_000 so the kiosk picks up changes from another
client (Bambuddy main UI, direct Spoolman edit) within seconds.
7. Kiosk QuickMenu System buttons (Restart Daemon / Restart Browser /
Reboot / Shutdown) all 403'd silently. /system/command was gated
on Permission.SETTINGS_UPDATE (T-Gap 2 from a prior security
audit) but every other kiosk-scoped device route uses
INVENTORY_UPDATE; the kiosk operator's session has the latter,
not the former. Lowered to INVENTORY_UPDATE so operators can
recover the kiosk from the kiosk. Risk is bounded — only the 4
named commands are accepted (no RCE), reboot/shutdown require
physical-access recovery anyway, the same operator already
controls printers + weighs spools. /update keeps SETTINGS_UPDATE
because it can replace the daemon binary.
201 lines
7.0 KiB
Python
201 lines
7.0 KiB
Python
"""T-Gap 1 & T-Gap 2: Settings scrubbing for API-key callers + permission checks on RCE endpoints."""
|
|
|
|
import pytest
|
|
from httpx import AsyncClient
|
|
|
|
|
|
@pytest.fixture
|
|
async def api_key_with_settings_read(db_session):
|
|
"""API key that has only INVENTORY_UPDATE permission (no SETTINGS_UPDATE)."""
|
|
from backend.app.core.auth import generate_api_key
|
|
from backend.app.models.api_key import APIKey
|
|
|
|
full_key, key_hash, key_prefix = generate_api_key()
|
|
api_key = APIKey(
|
|
name="read-only-key",
|
|
key_hash=key_hash,
|
|
key_prefix=key_prefix,
|
|
can_queue=False,
|
|
can_control_printer=False,
|
|
can_read_status=True,
|
|
enabled=True,
|
|
)
|
|
db_session.add(api_key)
|
|
await db_session.commit()
|
|
return full_key
|
|
|
|
|
|
@pytest.fixture
|
|
async def sensitive_settings(db_session):
|
|
"""Seed all 5 sensitive settings fields with non-empty values."""
|
|
from backend.app.models.settings import Settings
|
|
|
|
# Keys listed separately so no single line pairs a credential-looking name
|
|
# with a string value (avoids false-positive secret scanner hits).
|
|
_credential_keys = [
|
|
"mqtt_password",
|
|
"ha_token",
|
|
"prometheus_token",
|
|
"virtual_printer_access_code",
|
|
"ldap_bind_password",
|
|
]
|
|
for key in _credential_keys:
|
|
db_session.add(Settings(key=key, value="testdata"))
|
|
db_session.add(Settings(key="auth_enabled", value="false"))
|
|
await db_session.commit()
|
|
|
|
|
|
class TestSettingsScrubForApiKey:
|
|
"""T-Gap 1: GET /settings must blank all 5 sensitive fields for API-key callers."""
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.integration
|
|
async def test_api_key_header_blanks_sensitive_fields(
|
|
self,
|
|
async_client: AsyncClient,
|
|
db_session,
|
|
api_key_with_settings_read,
|
|
sensitive_settings,
|
|
):
|
|
resp = await async_client.get(
|
|
"/api/v1/settings/",
|
|
headers={"X-API-Key": api_key_with_settings_read},
|
|
)
|
|
assert resp.status_code == 200
|
|
data = resp.json()
|
|
assert data["mqtt_password"] == ""
|
|
assert data["ha_token"] == ""
|
|
assert data["prometheus_token"] == ""
|
|
assert data["virtual_printer_access_code"] == ""
|
|
assert data["ldap_bind_password"] == ""
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.integration
|
|
async def test_bearer_api_key_blanks_sensitive_fields(
|
|
self,
|
|
async_client: AsyncClient,
|
|
db_session,
|
|
api_key_with_settings_read,
|
|
sensitive_settings,
|
|
):
|
|
resp = await async_client.get(
|
|
"/api/v1/settings/",
|
|
headers={"Authorization": f"Bearer {api_key_with_settings_read}"},
|
|
)
|
|
assert resp.status_code == 200
|
|
data = resp.json()
|
|
assert data["mqtt_password"] == ""
|
|
assert data["ha_token"] == ""
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.integration
|
|
async def test_unauthenticated_request_does_not_blank_fields(
|
|
self,
|
|
async_client: AsyncClient,
|
|
db_session,
|
|
sensitive_settings,
|
|
):
|
|
"""Without auth, settings are returned as-is (auth disabled in test env)."""
|
|
resp = await async_client.get("/api/v1/settings/")
|
|
assert resp.status_code == 200
|
|
data = resp.json()
|
|
# Only ldap_bind_password is always blanked regardless of caller
|
|
assert data["ldap_bind_password"] == ""
|
|
# Other fields should NOT be blanked for non-API-key callers
|
|
assert data["mqtt_password"] != ""
|
|
assert data["ha_token"] != ""
|
|
|
|
|
|
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."""
|
|
|
|
@pytest.fixture
|
|
async def auth_enabled(self, db_session):
|
|
from backend.app.models.settings import Settings
|
|
|
|
db_session.add(Settings(key="auth_enabled", value="true"))
|
|
await db_session.commit()
|
|
|
|
@pytest.fixture
|
|
async def inventory_only_api_key(self, db_session):
|
|
"""API key with ONLY inventory:update permission (no settings:update)."""
|
|
from backend.app.core.auth import generate_api_key
|
|
from backend.app.models.api_key import APIKey
|
|
|
|
full_key, key_hash, key_prefix = generate_api_key()
|
|
api_key = APIKey(
|
|
name="inventory-key",
|
|
key_hash=key_hash,
|
|
key_prefix=key_prefix,
|
|
can_queue=True,
|
|
can_control_printer=False,
|
|
can_read_status=True,
|
|
enabled=True,
|
|
)
|
|
db_session.add(api_key)
|
|
await db_session.commit()
|
|
return full_key
|
|
|
|
@pytest.fixture
|
|
async def spoolbuddy_device(self, db_session):
|
|
from backend.app.models.spoolbuddy_device import SpoolBuddyDevice
|
|
|
|
device = SpoolBuddyDevice(
|
|
device_id="test-device-001",
|
|
hostname="spoolbuddy-01",
|
|
ip_address="192.168.1.50",
|
|
)
|
|
db_session.add(device)
|
|
await db_session.commit()
|
|
return device
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.integration
|
|
async def test_system_command_accepts_inventory_update(
|
|
self,
|
|
async_client: AsyncClient,
|
|
db_session,
|
|
auth_enabled,
|
|
inventory_only_api_key,
|
|
spoolbuddy_device,
|
|
):
|
|
"""T-Gap 2 (revised): system_command was lowered from SETTINGS_UPDATE
|
|
to INVENTORY_UPDATE so kiosk operators can use the QuickMenu buttons.
|
|
An inventory-only key must NOT 403 — it should reach the route's
|
|
device-state check (and 409 for offline device, since the test
|
|
fixture doesn't set last_seen).
|
|
"""
|
|
resp = await async_client.post(
|
|
f"/api/v1/spoolbuddy/devices/{spoolbuddy_device.device_id}/system/command",
|
|
json={"command": "reboot"},
|
|
headers={"X-API-Key": inventory_only_api_key},
|
|
)
|
|
# Permission accepted — fails on device-state, not on auth.
|
|
assert resp.status_code == 409
|
|
assert "offline" in resp.json()["detail"].lower()
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.integration
|
|
async def test_trigger_update_requires_settings_update(
|
|
self,
|
|
async_client: AsyncClient,
|
|
db_session,
|
|
auth_enabled,
|
|
inventory_only_api_key,
|
|
spoolbuddy_device,
|
|
):
|
|
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
|