mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 06:15:09 +00:00
* fix(security): refuse unsafe plugin ids, keep secrets private, validate bodies - install_from_url and the registry install's manifest rename refuse a plugin id that is not a single safe name (no ../ out of plugins_dir). - Uninstall and config reset refuse core config sections and ids with path parts; uninstall of a plugin whose directory is gone still works. - separate_secrets checks a field's own x-secret marker before recursing, so object/array secrets no longer land in config.json. - Backup restore creates missing secrets/wifi/ytm files with mode 640; export skips non-object manifests and no longer collides on same-second exports. - SYSTEM_FONTS includes every bundled font from BUNDLED_FONTS. - Raw config/secrets saves and validate_request_json require a JSON object. - A blank max_dynamic_duration_seconds keeps the stored value; other values are validated to 30-1800 instead of raising a 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(security): validate the id before install_plugin moves anything; claim backup names atomically - install_plugin set aside plugins_dir / plugin_id before any id check, so "../x" moved a directory outside the plugins dir (the rollback moved it back, but only if the install path got that far) - two exports finishing in the same second could both see a free name and the later os.replace destroyed the first archive; the name is now claimed with O_EXCL before the archive is swapped in Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
446 lines
20 KiB
Python
446 lines
20 KiB
Python
"""Regression tests for the transactional uninstall helper and the
|
|
``/plugins/state/reconcile`` endpoint's payload handling.
|
|
|
|
Bug 1: the original uninstall flow caught
|
|
``cleanup_plugin_config`` exceptions and only logged a warning before
|
|
proceeding to file deletion. A failure there would leave the plugin
|
|
files on disk with no config entry (orphan). The fix is a
|
|
``_do_transactional_uninstall`` helper that (a) aborts before touching
|
|
the filesystem if cleanup fails, and (b) restores the config+secrets
|
|
snapshot if file removal fails after cleanup succeeded.
|
|
|
|
Bug 2: the reconcile endpoint did ``payload.get('force', False)`` after
|
|
``request.get_json(silent=True) or {}``, which raises AttributeError if
|
|
the client sent a non-object JSON body (e.g. a bare string or array).
|
|
Additionally, ``bool("false")`` is ``True``, so string-encoded booleans
|
|
were mis-handled. The fix is an ``isinstance(payload, dict)`` guard plus
|
|
routing the value through ``_coerce_to_bool``.
|
|
"""
|
|
|
|
import json
|
|
import sys
|
|
import unittest
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
project_root = Path(__file__).parent.parent
|
|
sys.path.insert(0, str(project_root))
|
|
|
|
from flask import Flask
|
|
|
|
|
|
_API_V3_MOCKED_ATTRS = (
|
|
'config_manager', 'plugin_manager', 'plugin_store_manager',
|
|
'plugin_state_manager', 'saved_repositories_manager', 'schema_manager',
|
|
'operation_queue', 'operation_history', 'cache_manager',
|
|
)
|
|
|
|
|
|
def _make_client():
|
|
"""Minimal Flask app + mocked deps that exercises the api_v3 blueprint.
|
|
|
|
Returns ``(client, module, cleanup_fn)``. Callers (test ``setUp``
|
|
methods) must register ``cleanup_fn`` with ``self.addCleanup(...)``
|
|
so the original api_v3 singleton attributes are restored at the end
|
|
of the test — otherwise the MagicMocks leak into later tests that
|
|
import api_v3 expecting fresh state.
|
|
"""
|
|
from web_interface.blueprints import api_v3 as api_v3_module
|
|
from web_interface.blueprints.api_v3 import api_v3
|
|
|
|
# Snapshot the originals so we can restore them.
|
|
_SENTINEL = object()
|
|
originals = {
|
|
name: getattr(api_v3, name, _SENTINEL) for name in _API_V3_MOCKED_ATTRS
|
|
}
|
|
|
|
# Mocks for all the bits the reconcile / uninstall endpoints touch.
|
|
api_v3.config_manager = MagicMock()
|
|
api_v3.config_manager.get_raw_file_content.return_value = {}
|
|
api_v3.config_manager.secrets_path = "/tmp/nonexistent_secrets.json"
|
|
api_v3.plugin_manager = MagicMock()
|
|
api_v3.plugin_manager.plugins = {}
|
|
api_v3.plugin_manager.plugins_dir = "/tmp"
|
|
api_v3.plugin_store_manager = MagicMock()
|
|
api_v3.plugin_state_manager = MagicMock()
|
|
api_v3.plugin_state_manager.get_all_states.return_value = {}
|
|
api_v3.saved_repositories_manager = MagicMock()
|
|
api_v3.schema_manager = MagicMock()
|
|
api_v3.operation_queue = None # force the direct (non-queue) path
|
|
api_v3.operation_history = MagicMock()
|
|
api_v3.cache_manager = MagicMock()
|
|
|
|
def _cleanup():
|
|
for name, original in originals.items():
|
|
if original is _SENTINEL:
|
|
# Attribute didn't exist before — remove it to match.
|
|
if hasattr(api_v3, name):
|
|
try:
|
|
delattr(api_v3, name)
|
|
except AttributeError:
|
|
pass
|
|
else:
|
|
setattr(api_v3, name, original)
|
|
|
|
app = Flask(__name__)
|
|
app.config['TESTING'] = True
|
|
app.config['SECRET_KEY'] = 'test'
|
|
app.register_blueprint(api_v3, url_prefix='/api/v3')
|
|
return app.test_client(), api_v3_module, _cleanup
|
|
|
|
|
|
class TestTransactionalUninstall(unittest.TestCase):
|
|
"""Exercises ``_do_transactional_uninstall`` directly.
|
|
|
|
Using the direct (non-queue) code path via the Flask client gives us
|
|
the full uninstall endpoint behavior end-to-end, including the
|
|
rollback on mid-flight failures.
|
|
"""
|
|
|
|
def setUp(self):
|
|
self.client, self.mod, _cleanup = _make_client()
|
|
self.addCleanup(_cleanup)
|
|
self.api_v3 = self.mod.api_v3
|
|
|
|
def test_cleanup_failure_aborts_before_file_removal(self):
|
|
"""If cleanup_plugin_config raises, uninstall_plugin must NOT run."""
|
|
self.api_v3.config_manager.cleanup_plugin_config.side_effect = RuntimeError("disk full")
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
self.assertEqual(response.status_code, 500)
|
|
# File removal must NOT have been attempted — otherwise we'd have
|
|
# deleted the plugin after failing to clean its config, leaving
|
|
# the reconciler to potentially resurrect it later.
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.assert_not_called()
|
|
|
|
def test_file_removal_failure_restores_snapshot(self):
|
|
"""If uninstall_plugin returns False after cleanup, snapshot must be restored."""
|
|
# Start with the plugin in main config and in secrets.
|
|
stored_main = {'thing': {'enabled': True, 'custom': 'stuff'}}
|
|
stored_secrets = {'thing': {'api_key': 'secret'}}
|
|
|
|
# get_raw_file_content is called twice during snapshot (main +
|
|
# secrets) and then again during restore. We track writes through
|
|
# save_raw_file_content so we can assert the restore happened.
|
|
def raw_get(file_type):
|
|
if file_type == 'main':
|
|
return dict(stored_main)
|
|
if file_type == 'secrets':
|
|
return dict(stored_secrets)
|
|
return {}
|
|
|
|
self.api_v3.config_manager.get_raw_file_content.side_effect = raw_get
|
|
self.api_v3.config_manager.secrets_path = __file__ # any existing file
|
|
self.api_v3.config_manager.cleanup_plugin_config.return_value = None
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.return_value = False
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
self.assertEqual(response.status_code, 500)
|
|
# After the file removal returned False, the helper must have
|
|
# written the snapshot back. Inspect save_raw_file_content calls.
|
|
calls = self.api_v3.config_manager.save_raw_file_content.call_args_list
|
|
file_types_written = [c.args[0] for c in calls]
|
|
self.assertIn('main', file_types_written,
|
|
f"main config was not restored after uninstall failure; calls={calls}")
|
|
# Find the main restore call and confirm our snapshot entry is present.
|
|
for c in calls:
|
|
if c.args[0] == 'main':
|
|
written = c.args[1]
|
|
self.assertIn('thing', written,
|
|
"main config was written without the restored snapshot entry")
|
|
self.assertEqual(written['thing'], stored_main['thing'])
|
|
break
|
|
|
|
def test_file_removal_raising_also_restores_snapshot(self):
|
|
"""Same restore path, but triggered by an exception instead of False."""
|
|
stored_main = {'thing': {'enabled': False}}
|
|
|
|
def raw_get(file_type):
|
|
if file_type == 'main':
|
|
return dict(stored_main)
|
|
return {}
|
|
|
|
self.api_v3.config_manager.get_raw_file_content.side_effect = raw_get
|
|
self.api_v3.config_manager.cleanup_plugin_config.return_value = None
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.side_effect = OSError("rm failed")
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
self.assertEqual(response.status_code, 500)
|
|
calls = self.api_v3.config_manager.save_raw_file_content.call_args_list
|
|
self.assertTrue(
|
|
any(c.args[0] == 'main' for c in calls),
|
|
"main config was not restored after uninstall raised",
|
|
)
|
|
|
|
def test_happy_path_succeeds(self):
|
|
"""Sanity: the transactional rework did not break the happy path."""
|
|
self.api_v3.config_manager.get_raw_file_content.return_value = {}
|
|
self.api_v3.config_manager.cleanup_plugin_config.return_value = None
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.return_value = True
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
self.assertEqual(response.status_code, 200)
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.assert_called_once_with('thing')
|
|
|
|
def test_file_removal_failure_reloads_previously_loaded_plugin(self):
|
|
"""Regression: rollback must restore BOTH config AND runtime state.
|
|
|
|
If the plugin was loaded at runtime before the uninstall
|
|
request, and file removal fails after unload has already
|
|
succeeded, the rollback must call ``load_plugin`` so the user
|
|
doesn't end up in a state where the files exist and the config
|
|
exists but the plugin is no longer loaded.
|
|
"""
|
|
# Plugin is currently loaded.
|
|
self.api_v3.plugin_manager.plugins = {'thing': MagicMock()}
|
|
self.api_v3.config_manager.get_raw_file_content.return_value = {
|
|
'thing': {'enabled': True}
|
|
}
|
|
self.api_v3.config_manager.cleanup_plugin_config.return_value = None
|
|
self.api_v3.plugin_manager.unload_plugin.return_value = None
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.return_value = False
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
self.assertEqual(response.status_code, 500)
|
|
# Unload did happen (it's part of the uninstall sequence)...
|
|
self.api_v3.plugin_manager.unload_plugin.assert_called_once_with('thing')
|
|
# ...and because file removal failed, the rollback must have
|
|
# called load_plugin to restore runtime state.
|
|
self.api_v3.plugin_manager.load_plugin.assert_called_once_with('thing')
|
|
|
|
def test_snapshot_survives_config_read_error(self):
|
|
"""Regression: if get_raw_file_content raises an expected error
|
|
(OSError / ConfigError) during snapshot, the uninstall should
|
|
still proceed — we just won't have a rollback snapshot. Narrow
|
|
exception list must still cover the realistic failure modes.
|
|
"""
|
|
from src.exceptions import ConfigError
|
|
self.api_v3.config_manager.get_raw_file_content.side_effect = ConfigError(
|
|
"file missing", config_path="/tmp/missing"
|
|
)
|
|
self.api_v3.config_manager.cleanup_plugin_config.return_value = None
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.return_value = True
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
# Uninstall should still succeed — snapshot failure is logged
|
|
# but doesn't block the uninstall.
|
|
self.assertEqual(response.status_code, 200)
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.assert_called_once_with('thing')
|
|
|
|
def test_snapshot_does_not_swallow_programmer_errors(self):
|
|
"""Regression: unexpected exceptions (TypeError, AttributeError)
|
|
must propagate out of the snapshot helper so bugs surface
|
|
during development instead of being silently logged and
|
|
ignored. Narrowing from ``except Exception`` to
|
|
``(OSError, ConfigError)`` is what makes this work.
|
|
"""
|
|
# Raise an exception that is NOT in the narrow catch list.
|
|
self.api_v3.config_manager.get_raw_file_content.side_effect = TypeError(
|
|
"unexpected kwarg"
|
|
)
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
# The TypeError should propagate up to the endpoint's outer
|
|
# try/except and produce a 500, NOT be silently swallowed like
|
|
# the previous ``except Exception`` did.
|
|
self.assertEqual(response.status_code, 500)
|
|
# uninstall_plugin must NOT have been called — the snapshot
|
|
# exception bubbled up before we got that far.
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.assert_not_called()
|
|
|
|
def test_unload_failure_restores_config_and_does_not_call_uninstall(self):
|
|
"""If unload_plugin itself raises, config must be restored and
|
|
uninstall_plugin must NOT be called."""
|
|
self.api_v3.plugin_manager.plugins = {'thing': MagicMock()}
|
|
self.api_v3.config_manager.get_raw_file_content.return_value = {
|
|
'thing': {'enabled': True}
|
|
}
|
|
self.api_v3.config_manager.cleanup_plugin_config.return_value = None
|
|
self.api_v3.plugin_manager.unload_plugin.side_effect = RuntimeError("unload boom")
|
|
|
|
response = self.client.post(
|
|
'/api/v3/plugins/uninstall',
|
|
data=json.dumps({'plugin_id': 'thing'}),
|
|
content_type='application/json',
|
|
)
|
|
|
|
self.assertEqual(response.status_code, 500)
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.assert_not_called()
|
|
# Config should have been restored.
|
|
calls = self.api_v3.config_manager.save_raw_file_content.call_args_list
|
|
self.assertTrue(
|
|
any(c.args[0] == 'main' for c in calls),
|
|
"main config was not restored after unload_plugin raised",
|
|
)
|
|
# load_plugin must NOT have been called — unload didn't succeed,
|
|
# so runtime state is still what it was.
|
|
self.api_v3.plugin_manager.load_plugin.assert_not_called()
|
|
|
|
|
|
class TestReconcileEndpointPayload(unittest.TestCase):
|
|
"""``/plugins/state/reconcile`` must handle weird JSON payloads without
|
|
crashing, and must accept string booleans for ``force``.
|
|
"""
|
|
|
|
def setUp(self):
|
|
self.client, self.mod, _cleanup = _make_client()
|
|
self.addCleanup(_cleanup)
|
|
self.api_v3 = self.mod.api_v3
|
|
# Stub the reconciler so we only test the payload plumbing, not
|
|
# the full reconciliation. We patch StateReconciliation at the
|
|
# module level where the endpoint imports it lazily.
|
|
self._reconciler_instance = MagicMock()
|
|
self._reconciler_instance.reconcile_state.return_value = MagicMock(
|
|
inconsistencies_found=[],
|
|
inconsistencies_fixed=[],
|
|
inconsistencies_manual=[],
|
|
message="ok",
|
|
)
|
|
# Patch the StateReconciliation class where it's imported inside
|
|
# the reconcile endpoint.
|
|
self._patcher = patch(
|
|
'src.plugin_system.state_reconciliation.StateReconciliation',
|
|
return_value=self._reconciler_instance,
|
|
)
|
|
self._patcher.start()
|
|
self.addCleanup(self._patcher.stop)
|
|
|
|
def _post(self, body, content_type='application/json'):
|
|
return self.client.post(
|
|
'/api/v3/plugins/state/reconcile',
|
|
data=body,
|
|
content_type=content_type,
|
|
)
|
|
|
|
def test_non_object_json_body_does_not_crash(self):
|
|
"""A bare string JSON body must not raise AttributeError."""
|
|
response = self._post('"just a string"')
|
|
self.assertEqual(response.status_code, 200)
|
|
# force must default to False.
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=False)
|
|
|
|
def test_array_json_body_does_not_crash(self):
|
|
response = self._post('[1, 2, 3]')
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=False)
|
|
|
|
def test_null_json_body_does_not_crash(self):
|
|
response = self._post('null')
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=False)
|
|
|
|
def test_missing_force_key_defaults_to_false(self):
|
|
response = self._post('{}')
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=False)
|
|
|
|
def test_force_true_boolean(self):
|
|
response = self._post(json.dumps({'force': True}))
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=True)
|
|
|
|
def test_force_false_boolean(self):
|
|
response = self._post(json.dumps({'force': False}))
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=False)
|
|
|
|
def test_force_string_false_coerced_correctly(self):
|
|
"""``bool("false")`` is ``True`` — _coerce_to_bool must fix that."""
|
|
response = self._post(json.dumps({'force': 'false'}))
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=False)
|
|
|
|
def test_force_string_true_coerced_correctly(self):
|
|
response = self._post(json.dumps({'force': 'true'}))
|
|
self.assertEqual(response.status_code, 200)
|
|
self._reconciler_instance.reconcile_state.assert_called_once_with(force=True)
|
|
|
|
|
|
class TestNonPluginIdsAreRefused(unittest.TestCase):
|
|
"""Uninstall and config reset key config.json by the request's plugin_id.
|
|
|
|
``{"plugin_id": "display"}`` deleted the core display section and
|
|
answered success; a reset overwrote it with a plugin schema's defaults.
|
|
"""
|
|
|
|
def setUp(self):
|
|
self.client, self.mod, _cleanup = _make_client()
|
|
self.addCleanup(_cleanup)
|
|
self.api_v3 = self.mod.api_v3
|
|
self.api_v3.plugin_manager.plugin_manifests = {'thing': {'id': 'thing'}}
|
|
|
|
def _post(self, url, body):
|
|
return self.client.post(url, data=json.dumps(body),
|
|
content_type='application/json')
|
|
|
|
def test_uninstall_refuses_core_sections_and_traversal(self):
|
|
for bad in ('display', 'schedule', 'plugin_system', 'github', '../x', 'a/b', 7):
|
|
with self.subTest(plugin_id=bad):
|
|
response = self._post('/api/v3/plugins/uninstall', {'plugin_id': bad})
|
|
self.assertEqual(response.status_code, 400)
|
|
self.api_v3.config_manager.cleanup_plugin_config.assert_not_called()
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.assert_not_called()
|
|
|
|
def test_uninstall_still_cleans_a_plugin_whose_directory_is_gone(self):
|
|
# Not among the discovered manifests, but a plugin-shaped id: the
|
|
# config cleanup must still run.
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.return_value = True
|
|
response = self._post('/api/v3/plugins/uninstall', {'plugin_id': 'gone-plugin'})
|
|
|
|
self.assertEqual(response.status_code, 200, response.get_json())
|
|
self.api_v3.config_manager.cleanup_plugin_config.assert_called_once_with(
|
|
'gone-plugin', remove_secrets=True)
|
|
|
|
def test_secrets_only_core_key_is_allowed_when_a_plugin_has_that_id(self):
|
|
self.api_v3.plugin_manager.plugin_manifests = {'youtube': {'id': 'youtube'}}
|
|
self.api_v3.plugin_store_manager.uninstall_plugin.return_value = True
|
|
response = self._post('/api/v3/plugins/uninstall', {'plugin_id': 'youtube'})
|
|
|
|
self.assertEqual(response.status_code, 200, response.get_json())
|
|
|
|
def test_reset_refuses_core_sections(self):
|
|
response = self._post('/api/v3/plugins/config/reset', {'plugin_id': 'display'})
|
|
|
|
self.assertEqual(response.status_code, 400)
|
|
self.api_v3.schema_manager.generate_default_config.assert_not_called()
|
|
self.api_v3.config_manager.save_raw_file_content.assert_not_called()
|
|
|
|
|
|
if __name__ == '__main__':
|
|
unittest.main()
|