0e3ae78de7
Closes the issues surfaced by the pre-merge code review of the expand-device-support branch. CRITICAL #2 -- update_device double-encrypts secrets in memory. storage/device_store.py round-tripped through device.to_dict() which encrypts hue_username / hue_client_key / ble_govee_key / nanoleaf_token via _enc(), but Device.__init__ does not decrypt. The cached self._items[device_id] thus held ciphertext where plaintext belonged, breaking runtime auth for paired devices on any update -- even an innocuous rename. Sourcing kwargs from vars(device) directly avoids the round-trip. Regression tests cover Nanoleaf and Hue. HIGH #3 -- secrets leaked in GET /api/v1/devices response. DeviceResponse previously returned nanoleaf_token / hue_username / hue_client_key in plaintext (decrypted server-side from storage), defeating the encryption-at-rest. Replaced with nanoleaf_paired and hue_paired booleans. ble_govee_key intentionally stays -- it's a user-managed value pasted from a third-party tool, must remain visible for edit. Frontend types.ts + the one nanoleaf_token reader updated to the boolean. HIGH #4 -- SSRF surface. validate_lan_host() added to net_classify.py; called from each new driver's validate_device (DDP / Yeelight / WiZ / LIFX / Govee / OPC / Nanoleaf) and from pair_device. Rejects literal public IPs with a descriptive ValueError; non-IP hostnames pass through (mDNS labels, bare hostnames). RFC6890 ranges (documentation, former class E) are accepted as LAN-like since Python's ipaddress.is_private treats them so -- correct policy for LedGrab. HIGH #5 -- decrypt failure deletes the device row. _dec() now catches the exception, logs an error, and returns "" instead of propagating. Without the fix, a regenerated data/.secret_key would silently make every Hue / Nanoleaf / BLE-Govee device disappear from the device list on next startup. Regression test asserts a corrupt envelope leaves the device hydratable. HIGH #6 -- update_device route does not rstrip("/") for non-WLED. Moved the trim before the WLED-specific scheme inference so every device type gets consistent URL normalization between create and update. MEDIUM #7 -- Govee discovery port 4002 collision. Added a lazily- initialized module-level asyncio.Lock that serializes concurrent discover_govee_devices() calls; the previous behavior had the second parallel scan silently return [] when the first still held port 4002. Error message also clarified to mention another Govee tool. MEDIUM #8 -- Nanoleaf discover() leaked browser tasks on cancellation. Moved the browser cancel loop into the finally block so an interrupted mDNS scan still tears them down. MEDIUM #9 -- pair endpoint logged user-supplied URL with exc_info=True. Added _sanitize_url_for_log() that strips userinfo + fragment, and demoted the log from exc_info to type(exc).__name__ + str(exc) so a hostile receiver's response body can't end up in the log file. LOW -- Nanoleaf was the only client without a .port property. Added one (returns NANOLEAF_PORT, fixed) for cross-driver symmetry. LOW -- no end-to-end pair-then-create coverage. Added TestPairThenCreateFlow.test_pair_then_create_persists_encrypted_token which exercises the full path: POST /api/v1/devices/pair returned fields, store.create_device, then asserts (a) in-memory plaintext, (b) to_config() plaintext, (c) persisted ciphertext, (d) API response strip + paired-boolean. Tests: 1379 pass (was 1358 -- 21 new regression tests added). ruff clean. TypeScript clean.
351 lines
10 KiB
Python
351 lines
10 KiB
Python
"""Tests for device storage."""
|
|
|
|
import pytest
|
|
|
|
from ledgrab.storage.database import Database
|
|
from ledgrab.storage.device_store import Device, DeviceStore
|
|
|
|
|
|
@pytest.fixture
|
|
def tmp_db(tmp_path):
|
|
"""Provide a temporary SQLite Database instance."""
|
|
db = Database(tmp_path / "test.db")
|
|
yield db
|
|
db.close()
|
|
|
|
|
|
@pytest.fixture
|
|
def device_store(tmp_db):
|
|
"""Provide device store instance."""
|
|
return DeviceStore(tmp_db)
|
|
|
|
|
|
def test_device_creation():
|
|
"""Test creating a device."""
|
|
device = Device(
|
|
device_id="test_001",
|
|
name="Test Device",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
assert device.id == "test_001"
|
|
assert device.name == "Test Device"
|
|
assert device.url == "http://192.168.1.100"
|
|
assert device.led_count == 150
|
|
assert device.enabled is True
|
|
|
|
|
|
def test_device_to_dict():
|
|
"""Test converting device to dictionary."""
|
|
device = Device(
|
|
device_id="test_001",
|
|
name="Test Device",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
data = device.to_dict()
|
|
|
|
assert data["id"] == "test_001"
|
|
assert data["name"] == "Test Device"
|
|
assert data["url"] == "http://192.168.1.100"
|
|
assert data["led_count"] == 150
|
|
|
|
|
|
def test_device_from_dict():
|
|
"""Test creating device from dictionary."""
|
|
data = {
|
|
"id": "test_001",
|
|
"name": "Test Device",
|
|
"url": "http://192.168.1.100",
|
|
"led_count": 150,
|
|
"enabled": True,
|
|
}
|
|
|
|
device = Device.from_dict(data)
|
|
|
|
assert device.id == "test_001"
|
|
assert device.name == "Test Device"
|
|
assert device.led_count == 150
|
|
|
|
|
|
def test_device_round_trip():
|
|
"""Test converting device to dict and back."""
|
|
original = Device(
|
|
device_id="test_001",
|
|
name="Test Device",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
data = original.to_dict()
|
|
restored = Device.from_dict(data)
|
|
|
|
assert restored.id == original.id
|
|
assert restored.name == original.name
|
|
assert restored.url == original.url
|
|
assert restored.led_count == original.led_count
|
|
|
|
|
|
def test_device_store_init(device_store):
|
|
"""Test device store initialization."""
|
|
assert device_store is not None
|
|
assert device_store.count() == 0
|
|
|
|
|
|
def test_create_device(device_store):
|
|
"""Test creating a device in store."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
assert device.id is not None
|
|
assert device.name == "Test WLED"
|
|
assert device_store.count() == 1
|
|
|
|
|
|
def test_get_device(device_store):
|
|
"""Test retrieving a device."""
|
|
created = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
retrieved = device_store.get_device(created.id)
|
|
|
|
assert retrieved is not None
|
|
assert retrieved.id == created.id
|
|
assert retrieved.name == "Test WLED"
|
|
|
|
|
|
def test_get_device_not_found(device_store):
|
|
"""Test retrieving non-existent device raises ValueError."""
|
|
with pytest.raises(ValueError, match="not found"):
|
|
device_store.get_device("nonexistent")
|
|
|
|
|
|
def test_get_all_devices(device_store):
|
|
"""Test getting all devices."""
|
|
device_store.create_device("Device 1", "http://192.168.1.100", 150)
|
|
device_store.create_device("Device 2", "http://192.168.1.101", 200)
|
|
|
|
devices = device_store.get_all_devices()
|
|
|
|
assert len(devices) == 2
|
|
assert any(d.name == "Device 1" for d in devices)
|
|
assert any(d.name == "Device 2" for d in devices)
|
|
|
|
|
|
def test_update_device(device_store):
|
|
"""Test updating a device."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
updated = device_store.update_device(
|
|
device.id,
|
|
name="Updated WLED",
|
|
enabled=False,
|
|
)
|
|
|
|
assert updated.name == "Updated WLED"
|
|
assert updated.enabled is False
|
|
|
|
|
|
def test_update_device_led_count(device_store):
|
|
"""Test updating device LED count."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
updated = device_store.update_device(device.id, led_count=200)
|
|
|
|
assert updated.led_count == 200
|
|
|
|
|
|
def test_update_device_not_found(device_store):
|
|
"""Test updating non-existent device."""
|
|
with pytest.raises(ValueError, match="not found"):
|
|
device_store.update_device("nonexistent", name="New Name")
|
|
|
|
|
|
def test_delete_device(device_store):
|
|
"""Test deleting a device."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
device_store.delete_device(device.id)
|
|
|
|
assert device_store.count() == 0
|
|
with pytest.raises(ValueError, match="not found"):
|
|
device_store.get_device(device.id)
|
|
|
|
|
|
def test_delete_device_not_found(device_store):
|
|
"""Test deleting non-existent device."""
|
|
with pytest.raises(ValueError, match="not found"):
|
|
device_store.delete_device("nonexistent")
|
|
|
|
|
|
def test_device_exists(device_store):
|
|
"""Test checking if device exists."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
assert device_store.device_exists(device.id) is True
|
|
assert device_store.device_exists("nonexistent") is False
|
|
|
|
|
|
def test_persistence(tmp_path):
|
|
"""Test device persistence across store instances."""
|
|
db = Database(tmp_path / "persist.db")
|
|
# Create store and add device
|
|
store1 = DeviceStore(db)
|
|
device = store1.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
device_id = device.id
|
|
|
|
# Create new store instance (loads from database)
|
|
store2 = DeviceStore(db)
|
|
|
|
# Verify device persisted
|
|
loaded_device = store2.get_device(device_id)
|
|
assert loaded_device is not None
|
|
assert loaded_device.name == "Test WLED"
|
|
assert loaded_device.led_count == 150
|
|
db.close()
|
|
|
|
|
|
def test_clear(device_store):
|
|
"""Test clearing all devices."""
|
|
device_store.create_device("Device 1", "http://192.168.1.100", 150)
|
|
device_store.create_device("Device 2", "http://192.168.1.101", 200)
|
|
|
|
assert device_store.count() == 2
|
|
|
|
device_store.clear()
|
|
|
|
assert device_store.count() == 0
|
|
|
|
|
|
def test_update_device_ignores_none_values(device_store):
|
|
"""Test that update_device ignores None kwargs."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
updated = device_store.update_device(device.id, name=None, led_count=200)
|
|
|
|
assert updated.name == "Test WLED" # unchanged
|
|
assert updated.led_count == 200
|
|
|
|
|
|
def test_update_device_ignores_unknown_fields(device_store):
|
|
"""Test that update_device silently ignores unknown kwargs."""
|
|
device = device_store.create_device(
|
|
name="Test WLED",
|
|
url="http://192.168.1.100",
|
|
led_count=150,
|
|
)
|
|
|
|
# Should not raise even with an unknown kwarg
|
|
updated = device_store.update_device(device.id, nonexistent_field="foo")
|
|
assert updated.name == "Test WLED"
|
|
|
|
|
|
def test_update_device_preserves_plaintext_secrets(device_store):
|
|
"""update_device must not corrupt encrypted-at-rest secrets in memory.
|
|
|
|
Regression test for the to_dict() round-trip bug: to_dict() encrypts
|
|
hue_username / hue_client_key / ble_govee_key / nanoleaf_token via
|
|
_enc(), but Device.__init__ does not decrypt -- so the old
|
|
update_device path would leave the cached Device holding ciphertext
|
|
where plaintext belongs, breaking runtime auth for paired devices
|
|
until server restart. See pre-merge review CRITICAL #2.
|
|
"""
|
|
device = device_store.create_device(
|
|
name="Office panels",
|
|
url="nanoleaf://192.168.1.99",
|
|
led_count=9,
|
|
device_type="nanoleaf",
|
|
nanoleaf_token="plaintext-secret-token",
|
|
)
|
|
assert device.nanoleaf_token == "plaintext-secret-token"
|
|
|
|
# An innocuous rename must not touch the secret.
|
|
updated = device_store.update_device(device.id, name="Renamed panels")
|
|
assert (
|
|
updated.nanoleaf_token == "plaintext-secret-token"
|
|
), "rename round-tripped the secret through to_dict() and corrupted it"
|
|
|
|
# The cached _items entry must also hold plaintext (it is what
|
|
# provider.create_client() reads).
|
|
cached = device_store.get_device(device.id)
|
|
assert cached.nanoleaf_token == "plaintext-secret-token"
|
|
|
|
# And to_config() must surface the plaintext token to the provider.
|
|
config = cached.to_config()
|
|
assert config.nanoleaf_token == "plaintext-secret-token"
|
|
|
|
|
|
def test_update_device_preserves_hue_secrets(device_store):
|
|
"""Same bug applies to hue_username / hue_client_key / ble_govee_key."""
|
|
device = device_store.create_device(
|
|
name="Hue bridge",
|
|
url="http://192.168.1.50",
|
|
led_count=10,
|
|
device_type="hue",
|
|
hue_username="plain-hue-user",
|
|
hue_client_key="plain-client-key",
|
|
)
|
|
|
|
updated = device_store.update_device(device.id, name="Renamed bridge")
|
|
|
|
assert updated.hue_username == "plain-hue-user"
|
|
assert updated.hue_client_key == "plain-client-key"
|
|
|
|
|
|
def test_from_dict_clears_corrupt_secret_envelope(device_store):
|
|
"""A corrupt nanoleaf_token must not delete the device row.
|
|
|
|
Regression test for HIGH #5: previously _dec() would raise on a bad
|
|
envelope, propagate up through BaseSqliteStore._load, and the entire
|
|
device row would silently disappear. The fix clears the offending
|
|
field and logs an error instead.
|
|
"""
|
|
payload = {
|
|
"id": "device_abc12345",
|
|
"name": "Will survive",
|
|
"url": "nanoleaf://192.168.1.50",
|
|
"led_count": 9,
|
|
"device_type": "nanoleaf",
|
|
# Looks like an envelope (ENC:v1: prefix) but the base64 body is
|
|
# truncated / corrupt -- the AES-GCM auth tag will fail to verify.
|
|
"nanoleaf_token": "ENC:v1:not-real-base64-cipher-text",
|
|
}
|
|
|
|
restored = Device.from_dict(payload)
|
|
|
|
# The device hydrates; the broken secret is cleared, not crashed.
|
|
assert restored.id == "device_abc12345"
|
|
assert restored.nanoleaf_token == ""
|