fix: resolve comprehensive review findings (security, concurrency, perf, Android, UI)
Multi-dimension review of v0.8.2. Excludes the deliberately deferred default_config.yaml weak-default-key item (C1). Backend: - calibration: create_default_calibration no longer exceeds led_count for small odd counts (bounded trim + regression test) - game-integration: generic webhook now requires auth_token; constant-time compare_digest in all adapters; per-IP failed-auth rate limit on the ingest route; auth_token encrypted at rest via secret_box (migration-safe) - playlist engine: serialize _state/_task under the lifecycle lock to close a delete-mid-play race (+ concurrency tests) - main: stop the calibration session on shutdown (restore prior target) - home_assistant: validate HA host via the LAN classifier on create/update - perf: drop slow preview-WS clients instead of blocking the send loop; cache composite full-strip resize linspaces; effect_stream lava reuses scratch Frontend: - setup/auto-calibration wizard: guard _state after awaits (cancel-safe), await session teardown before output start, busy-gate skip-calibration, manual display input keeps focus, move focus on step change - calibration: destroy EntitySelect on modal close - color-strips test: dirty-flag-gated render + cached ctx/ImageData - a11y/TV: focus-visible for new wizard/auto-cal/corner controls, aria-labels on the spatial corner/edge picker; theme-aware syntax tokens; dead/undefined CSS tokens removed; .modal-error styled; i18n titles (en/ru/zh) Android: - ApiKeyManager: EncryptedSharedPreferences with verified, data-safe legacy migration that never rotates an existing key - CaptureService: validate MediaProjection token before promoting; satisfy the startForeground 5s contract on the bail path - NotificationListener: connection-scoped executor with lazy fallback - BLE: request BLUETOOTH_SCAN/CONNECT at runtime + guard handler-thread SecurityExceptions - Root: cancellation-aware su grant probe Adds 14 tests. Gate: ruff + tsc 5.9.3 + esbuild + pytest (2185 passed) + compileDebugKotlin all green.
This commit is contained in:
@@ -102,9 +102,20 @@ class TestGenericWebhookParsing:
|
||||
|
||||
|
||||
class TestGenericWebhookAuth:
|
||||
def test_no_auth_configured(self) -> None:
|
||||
def test_no_auth_configured_rejects(self) -> None:
|
||||
# Secure-by-default: a network-facing webhook adapter with no token
|
||||
# configured must REJECT, not accept (otherwise it is open to the LAN).
|
||||
result = GenericWebhookAdapter.validate_auth({}, {}, {})
|
||||
assert result is True
|
||||
assert result is False
|
||||
|
||||
def test_empty_token_rejects(self) -> None:
|
||||
# An explicitly empty token is still "no token" → reject.
|
||||
result = GenericWebhookAdapter.validate_auth(
|
||||
{"Authorization": "Bearer "},
|
||||
{},
|
||||
{"auth_token": ""},
|
||||
)
|
||||
assert result is False
|
||||
|
||||
def test_bearer_auth_valid(self) -> None:
|
||||
result = GenericWebhookAdapter.validate_auth(
|
||||
@@ -156,6 +167,8 @@ class TestGenericWebhookMetadata:
|
||||
assert "auth_token" in schema["properties"]
|
||||
assert "mappings" in schema["properties"]
|
||||
assert "auth_header" in schema["properties"]
|
||||
# auth_token is mandatory (secure-by-default).
|
||||
assert "auth_token" in schema.get("required", [])
|
||||
|
||||
def test_setup_instructions(self) -> None:
|
||||
instructions = GenericWebhookAdapter.get_setup_instructions()
|
||||
|
||||
@@ -315,3 +315,175 @@ class TestEvents:
|
||||
async def test_stop_when_idle_fires_nothing(self, engine):
|
||||
await engine.stop()
|
||||
assert _fired_actions(engine) == []
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Concurrency — delete-mid-play race (H5). The engine reads/mutates _task and
|
||||
# _state across await boundaries; a deletion firing stop_if_running() while the
|
||||
# _run loop is between its get_playlist re-read and the natural-end clear must
|
||||
# still produce exactly ONE terminal 'stopped' transition (no duplicate /
|
||||
# contradictory WS events), and get_state() must never raise nor return a
|
||||
# half-cleared snapshot during the window.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class _DeletableStore:
|
||||
"""Wraps a real playlist store; ``delete_playlist`` records a tombstone so
|
||||
a later ``get_playlist`` re-read raises (like the real store after a delete)
|
||||
and the engine's ``_run`` loop breaks out at the cycle boundary.
|
||||
"""
|
||||
|
||||
def __init__(self, real):
|
||||
self._real = real
|
||||
self._deleted: set[str] = set()
|
||||
|
||||
def get_playlist(self, playlist_id):
|
||||
if playlist_id in self._deleted:
|
||||
raise KeyError(playlist_id)
|
||||
return self._real.get_playlist(playlist_id)
|
||||
|
||||
def delete_playlist(self, playlist_id):
|
||||
self._deleted.add(playlist_id)
|
||||
|
||||
|
||||
class TestConcurrencyDeleteRace:
|
||||
async def test_delete_and_stop_if_running_emit_single_stopped(
|
||||
self, engine, playlist_store, applied
|
||||
):
|
||||
# A looping playlist so _run re-reads get_playlist every cycle, giving
|
||||
# us a deterministic parking point to interleave the delete + stop.
|
||||
pl = _make_playlist(playlist_store, "Racy", [("scene_a", 50), ("scene_b", 50)], loop=True)
|
||||
|
||||
gated = _DeletableStore(playlist_store)
|
||||
engine._playlist_store = gated
|
||||
|
||||
# The dwell sleep is the engine's only await between the get_playlist
|
||||
# re-read and the natural-end clear. We gate the FIRST dwell: when _run
|
||||
# parks there we set `entered`, then await `release`. While _run is
|
||||
# suspended the test deletes the playlist and kicks stop_if_running, so
|
||||
# they race _run's resumed natural-end clear under the lifecycle lock.
|
||||
entered = asyncio.Event()
|
||||
release = asyncio.Event()
|
||||
first_dwell = {"done": False}
|
||||
|
||||
async def _interleaving_sleep(_duration):
|
||||
if not first_dwell["done"]:
|
||||
first_dwell["done"] = True
|
||||
entered.set()
|
||||
await release.wait()
|
||||
await _REAL_SLEEP(0.005)
|
||||
|
||||
with patch("asyncio.sleep", _interleaving_sleep):
|
||||
await engine.start_playlist(pl.id)
|
||||
|
||||
# Wait until _run is parked inside the first dwell (mid-cycle).
|
||||
await asyncio.wait_for(entered.wait(), timeout=2.0)
|
||||
|
||||
# get_state during the window must not raise and must be coherent:
|
||||
# either fully running or fully idle — never half-cleared.
|
||||
snap = engine.get_state()
|
||||
assert isinstance(snap, dict)
|
||||
assert "is_running" in snap
|
||||
|
||||
# Concurrently: delete from the store AND call stop_if_running for
|
||||
# the same id. Kick stop_if_running first as a task so it contends
|
||||
# for the lifecycle lock with the (soon to resume) _run natural-end.
|
||||
gated.delete_playlist(pl.id)
|
||||
stop_task = asyncio.create_task(engine.stop_if_running(pl.id))
|
||||
|
||||
# Release _run: it finishes the dwell, re-reads (KeyError -> break),
|
||||
# and races stop_task into the natural-end clear under the lock.
|
||||
release.set()
|
||||
|
||||
await asyncio.wait_for(stop_task, timeout=2.0)
|
||||
# Drain whatever remains of the run task.
|
||||
run_task = engine._task
|
||||
if run_task is not None:
|
||||
try:
|
||||
await asyncio.wait_for(asyncio.shield(run_task), timeout=2.0)
|
||||
except (asyncio.CancelledError, KeyError):
|
||||
pass
|
||||
|
||||
# Exactly one terminal 'stopped' transition (no duplicate/contradictory).
|
||||
stopped = [e for e in _fired_events(engine) if e.get("action") == "stopped"]
|
||||
assert len(stopped) == 1, f"expected exactly one stopped, got {len(stopped)}: {stopped}"
|
||||
assert stopped[0]["playlist_id"] == pl.id
|
||||
|
||||
# Engine fully idle and get_state coherent afterwards.
|
||||
assert engine.is_running() is False
|
||||
final = engine.get_state()
|
||||
assert final["is_running"] is False
|
||||
assert final["playlist_id"] is None
|
||||
|
||||
async def test_natural_end_after_delete_emits_single_stopped(
|
||||
self, engine, playlist_store, applied
|
||||
):
|
||||
# Drive the OTHER ordering: the delete causes _run to break and run its
|
||||
# (now lock-wrapped) natural-end clear+'stopped', while a stop_if_running
|
||||
# for the same id is fired AFTER the dwell is released but races into the
|
||||
# same lock. Whoever wins, exactly one 'stopped' must surface and the
|
||||
# late stop_if_running must be a clean no-op (state already cleared).
|
||||
pl = _make_playlist(playlist_store, "Ending", [("scene_a", 50)], loop=False)
|
||||
|
||||
gated = _DeletableStore(playlist_store)
|
||||
engine._playlist_store = gated
|
||||
|
||||
entered = asyncio.Event()
|
||||
release = asyncio.Event()
|
||||
first_dwell = {"done": False}
|
||||
|
||||
async def _interleaving_sleep(_duration):
|
||||
if not first_dwell["done"]:
|
||||
first_dwell["done"] = True
|
||||
entered.set()
|
||||
await release.wait()
|
||||
await _REAL_SLEEP(0.005)
|
||||
|
||||
with patch("asyncio.sleep", _interleaving_sleep):
|
||||
await engine.start_playlist(pl.id)
|
||||
await asyncio.wait_for(entered.wait(), timeout=2.0)
|
||||
|
||||
# Delete and release so _run resumes -> non-loop break -> natural end.
|
||||
gated.delete_playlist(pl.id)
|
||||
release.set()
|
||||
|
||||
# Let the run task drive its natural-end clear to completion.
|
||||
run_task = engine._task
|
||||
if run_task is not None:
|
||||
await asyncio.wait_for(asyncio.shield(run_task), timeout=2.0)
|
||||
|
||||
# A late stop_if_running for the same id is now a clean no-op.
|
||||
await engine.stop_if_running(pl.id)
|
||||
|
||||
stopped = [e for e in _fired_events(engine) if e.get("action") == "stopped"]
|
||||
assert len(stopped) == 1, f"expected exactly one stopped, got {len(stopped)}: {stopped}"
|
||||
assert stopped[0]["playlist_id"] == pl.id
|
||||
assert engine.is_running() is False
|
||||
assert engine.get_state()["playlist_id"] is None
|
||||
|
||||
async def test_get_state_never_half_cleared_under_concurrent_stop(self, engine, playlist_store):
|
||||
# Hammer get_state() while start/stop churn the lifecycle, asserting it
|
||||
# never raises and never reports running with a None playlist_id.
|
||||
pl = _make_playlist(playlist_store, "Churn", [("scene_a", 50)], loop=True)
|
||||
|
||||
async def _churn():
|
||||
for _ in range(20):
|
||||
await engine.start_playlist(pl.id)
|
||||
await engine.stop()
|
||||
|
||||
async def _poll():
|
||||
for _ in range(500):
|
||||
s = engine.get_state()
|
||||
# Coherence invariant: running implies a concrete playlist_id.
|
||||
if s["is_running"]:
|
||||
assert s["playlist_id"] is not None
|
||||
await _REAL_SLEEP(0)
|
||||
|
||||
async def _fast(_duration):
|
||||
await _REAL_SLEEP(0)
|
||||
|
||||
with patch("asyncio.sleep", _fast):
|
||||
await asyncio.gather(_churn(), _poll())
|
||||
|
||||
await engine.stop()
|
||||
assert engine.is_running() is False
|
||||
|
||||
Reference in New Issue
Block a user