mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-17 20:39:13 +00:00
fix(tools): don't force PIXLET_EDITOR_HOST, and stop hard-assuming the MQTT bridge is uninstalled in tests
CodeRabbit review on #544: - start_pixlet_editor() unconditionally set PIXLET_EDITOR_HOST=0.0.0.0, overriding any operator-configured loopback-only value and always exposing the unauthenticated `pixlet serve` dev process on the LAN (CWE-1188). Use env.setdefault() so it's only a default; also stop hardcoding 'host' in the recorded session state so status reporting matches what was actually used. - test_tools_sections.js asserted an Install button always renders, which assumes the bridge service is never installed on the test/dev host. Branch on the same service.installed flag the template itself renders from. Adds regression coverage in test_starlark_pixlet_routes.py for the setdefault-vs-override behavior (both the "nothing configured" and "operator pinned loopback" cases). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5
This commit is contained in:
@@ -63,7 +63,16 @@ const get = p => new Promise((res, rej) =>
|
|||||||
!/s3cret|mqtt_password"\s*:\s*"/.test(window.document.body.innerHTML));
|
!/s3cret|mqtt_password"\s*:\s*"/.test(window.document.body.innerHTML));
|
||||||
ok('state badge rendered', ($('mqtt-bridge-state').textContent || '').trim().length > 0,
|
ok('state badge rendered', ($('mqtt-bridge-state').textContent || '').trim().length > 0,
|
||||||
$('mqtt-bridge-state').textContent);
|
$('mqtt-bridge-state').textContent);
|
||||||
ok('not-installed shows an Install button', !!$('btn-mqtt-install'));
|
// The dev/test machine may or may not actually have the bridge service
|
||||||
|
// installed -- branch on the same `service.installed` flag the template
|
||||||
|
// itself renders from, rather than hard-assuming "not installed" (which
|
||||||
|
// fails on any host where the service happens to be present).
|
||||||
|
if (bridge.data.service.installed) {
|
||||||
|
ok('installed shows Restart/Start-Stop buttons, not Install',
|
||||||
|
!$('btn-mqtt-install') && !!$('btn-mqtt-restart') && !!$('btn-mqtt-startstop'));
|
||||||
|
} else {
|
||||||
|
ok('not-installed shows an Install button', !!$('btn-mqtt-install'));
|
||||||
|
}
|
||||||
ok('config path shown', $('mqtt-bridge-body').textContent.includes('bridge_config.json'));
|
ok('config path shown', $('mqtt-bridge-body').textContent.includes('bridge_config.json'));
|
||||||
ok('env override hint shown', $('mqtt-bridge-body').textContent.includes('LEDMATRIX_MQTT_'));
|
ok('env override hint shown', $('mqtt-bridge-body').textContent.includes('LEDMATRIX_MQTT_'));
|
||||||
|
|
||||||
|
|||||||
@@ -748,3 +748,66 @@ class TestTheStoreUsesTheTokenTheUserConfigured:
|
|||||||
client.get('/api/v3/starlark/repository/browse')
|
client.get('/api/v3/starlark/repository/browse')
|
||||||
|
|
||||||
repo.assert_called_once_with(github_token='ghp_configured')
|
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 import api_v3 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, cwd=None, env=None, stdout=None, stderr=None,
|
||||||
|
start_new_session=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'
|
||||||
|
|||||||
@@ -10087,8 +10087,11 @@ def start_pixlet_editor():
|
|||||||
env['PIXLET_EDITOR_PORT'] = str(port)
|
env['PIXLET_EDITOR_PORT'] = str(port)
|
||||||
env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s)
|
env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s)
|
||||||
# A browser reaching this endpoint is remote by definition, so the
|
# A browser reaching this endpoint is remote by definition, so the
|
||||||
# session has to listen on more than loopback to be usable at all.
|
# session needs to listen on more than loopback to be usable at all --
|
||||||
env['PIXLET_EDITOR_HOST'] = '0.0.0.0'
|
# 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_path = Path(tempfile.gettempdir()) / 'ledmatrix_pixlet_editor.log'
|
||||||
log_handle = open(log_path, 'w', encoding='utf-8') # noqa: SIM115 - owned by the child
|
log_handle = open(log_path, 'w', encoding='utf-8') # noqa: SIM115 - owned by the child
|
||||||
@@ -10107,7 +10110,7 @@ def start_pixlet_editor():
|
|||||||
now = time.time()
|
now = time.time()
|
||||||
state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port,
|
state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port,
|
||||||
'timeout': timeout_s, 'started_at': now, 'deadline': now + timeout_s,
|
'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:
|
try:
|
||||||
with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle:
|
with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle:
|
||||||
json.dump(state, handle)
|
json.dump(state, handle)
|
||||||
|
|||||||
Reference in New Issue
Block a user