mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-17 20:39:13 +00:00
fix(starlark): don't crash the pixlet editor's start/stop routes, and honor an operator-set PIXLET_EDITOR_HOST
The AST-based port of #544 onto the api_v3 package split dropped `time` from starlark.py's import list. start_pixlet_editor() and stop_pixlet_editor() both call time.time()/time.sleep() directly, so every start (NameError building `state['started_at']`) and every stop that has to wait out the EXIT trap crashed with a 500. No test caught it because the route's own tests mock subprocess.Popen but never actually invoked it before now. Also carries over #544's later fix that this port branched before: env['PIXLET_EDITOR_HOST'] = '0.0.0.0' unconditionally overrode an operator who had already pinned PIXLET_EDITOR_HOST to loopback, forcing the unauthenticated `pixlet serve` process onto the LAN regardless (CodeQL CWE-1188). Switched to env.setdefault(...), same as api_v3.starlark.py's siblings already do for _pkg-owned names. Both fixes route the shared _pkg.time reference the rest of the package's route modules already use for anything a test might need to patch, rather than a bare `import time` local to this file. Ported the existing regression test from #544 (TestPixletEditorHostDefaultsButDoesNotOverride) onto this branch's module layout (web_interface.blueprints.api_v3.starlark instead of the old monolithic api_v3 module), which is what caught the NameError. Full suite: 4330 passed, 62 skipped, 2 failed -- identical on this branch and on origin/main (missing tzdata package breaks two timezone-alias tests in test_onboarding_checklist.py, unrelated to this change). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -906,3 +906,66 @@ class TestTheStoreUsesTheTokenTheUserConfigured:
|
||||
client.get('/api/v3/starlark/repository/browse')
|
||||
|
||||
repo.assert_called_once_with(github_token='ghp_configured')
|
||||
|
||||
|
||||
class TestPixletEditorHostDefaultsButDoesNotOverride:
|
||||
"""PIXLET_EDITOR_HOST must default to 0.0.0.0, never force it.
|
||||
|
||||
A browser reaching the editor is remote by definition, so a session with
|
||||
nothing configured has to bind more than loopback to be reachable at
|
||||
all -- but an operator who has deliberately pinned PIXLET_EDITOR_HOST to
|
||||
loopback (e.g. in the systemd unit's Environment=, to edit only over an
|
||||
SSH tunnel) must keep that setting. The previous unconditional
|
||||
``env['PIXLET_EDITOR_HOST'] = '0.0.0.0'`` overrode it every time,
|
||||
always exposing the unauthenticated ``pixlet serve`` dev process on the
|
||||
LAN regardless (CodeQL CWE-1188).
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def app_dir(self, tmp_path):
|
||||
d = tmp_path / "demo_app"
|
||||
d.mkdir()
|
||||
(d / "demo_app.star").write_text("def main():\n pass\n")
|
||||
return d
|
||||
|
||||
def _start(self, client, app_dir, tmp_path, operator_host):
|
||||
from web_interface.blueprints.api_v3 import starlark as mod
|
||||
|
||||
script = tmp_path / "pixlet_config_editor.sh"
|
||||
script.write_text("#!/bin/bash\n")
|
||||
state_file = tmp_path / "pixlet_editor_state.json"
|
||||
captured = {}
|
||||
|
||||
class FakeProcess:
|
||||
pid = 424242
|
||||
|
||||
def fake_popen(cmd, *args, env=None, **kwargs):
|
||||
if env is not None:
|
||||
captured['env'] = env
|
||||
return FakeProcess()
|
||||
|
||||
with patch.object(mod, '_validate_starlark_app_path',
|
||||
return_value=(app_dir, None)), \
|
||||
patch.object(mod, '_PIXLET_EDITOR_SCRIPT', script), \
|
||||
patch.object(mod, '_PIXLET_EDITOR_STATE', state_file), \
|
||||
patch.object(mod, '_find_pixlet_binary', return_value='/usr/bin/pixlet'), \
|
||||
patch.object(mod.subprocess, 'Popen', side_effect=fake_popen), \
|
||||
patch.dict(os.environ):
|
||||
if operator_host is None:
|
||||
os.environ.pop('PIXLET_EDITOR_HOST', None)
|
||||
else:
|
||||
os.environ['PIXLET_EDITOR_HOST'] = operator_host
|
||||
resp = client.post('/api/v3/starlark/editor/start',
|
||||
json={'app_id': app_dir.name})
|
||||
|
||||
assert resp.status_code == 200, resp.get_json()
|
||||
assert 'env' in captured, "subprocess.Popen was never called"
|
||||
return captured['env']
|
||||
|
||||
def test_defaults_to_0_0_0_0_when_operator_set_nothing(self, client, app_dir, tmp_path):
|
||||
env = self._start(client, app_dir, tmp_path, operator_host=None)
|
||||
assert env['PIXLET_EDITOR_HOST'] == '0.0.0.0'
|
||||
|
||||
def test_keeps_an_operator_configured_loopback_host(self, client, app_dir, tmp_path):
|
||||
env = self._start(client, app_dir, tmp_path, operator_host='127.0.0.1')
|
||||
assert env['PIXLET_EDITOR_HOST'] == '127.0.0.1'
|
||||
|
||||
@@ -814,8 +814,11 @@ def start_pixlet_editor():
|
||||
env['PIXLET_EDITOR_PORT'] = str(port)
|
||||
env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s)
|
||||
# A browser reaching this endpoint is remote by definition, so the
|
||||
# session has to listen on more than loopback to be usable at all.
|
||||
env['PIXLET_EDITOR_HOST'] = '0.0.0.0'
|
||||
# session needs to listen on more than loopback to be usable at all --
|
||||
# but only as a *default*. An operator who has already set
|
||||
# PIXLET_EDITOR_HOST (e.g. to keep it loopback-only even from the web
|
||||
# UI) must not have that overridden here.
|
||||
env.setdefault('PIXLET_EDITOR_HOST', '0.0.0.0')
|
||||
|
||||
log_path = Path(tempfile.gettempdir()) / 'ledmatrix_pixlet_editor.log'
|
||||
log_handle = open(log_path, 'w', encoding='utf-8') # noqa: SIM115 - owned by the child
|
||||
@@ -831,10 +834,10 @@ def start_pixlet_editor():
|
||||
finally:
|
||||
log_handle.close()
|
||||
|
||||
now = time.time()
|
||||
now = _pkg.time.time()
|
||||
state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port,
|
||||
'timeout': timeout_s, 'started_at': now, 'deadline': now + timeout_s,
|
||||
'host': '0.0.0.0', 'log': str(log_path)}
|
||||
'host': env['PIXLET_EDITOR_HOST'], 'log': str(log_path)}
|
||||
try:
|
||||
with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle:
|
||||
json.dump(state, handle)
|
||||
@@ -873,9 +876,9 @@ def stop_pixlet_editor():
|
||||
os.kill(pid, signal.SIGTERM)
|
||||
|
||||
# Give the trap a moment to stop pixlet and restart ledmatrix.
|
||||
deadline = time.time() + 10
|
||||
while time.time() < deadline and _pixlet_editor_alive(pid):
|
||||
time.sleep(0.25)
|
||||
deadline = _pkg.time.time() + 10
|
||||
while _pkg.time.time() < deadline and _pixlet_editor_alive(pid):
|
||||
_pkg.time.sleep(0.25)
|
||||
|
||||
if _pixlet_editor_alive(pid):
|
||||
logger.warning('Editor session %s ignored SIGTERM; sending SIGKILL. The display '
|
||||
|
||||
Reference in New Issue
Block a user