mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-18 12:59:12 +00:00
Fixes to the api_v3 package split (PR #553), one per finding verified against the actual code: - __init__.py: _redact_credentials only blanked scalar values under a credential-named key; a bare list of secrets under such a key (e.g. tokens: ["a", "b"]) passed through untouched, since the list branch recursed with no memory that its key looked like a credential. Nested dicts still walk normally (a documented, tested behaviour -- a container like secrets: {api_key: ..., note: ...} is a section name, not a value to blank outright), but any value reached under a credential-shaped key is now actually blanked. - __init__.py: the OAuth helper script's raw stderr/stdout went to logger.error unredacted (CWE-532) right next to a comment claiming this was deliberate; the HTTP response already used the existing redact_text helper. Routed the log line through the same helper. - __init__.py / starlark.py: the standalone Starlark manifest fallback (used when the plugin instance isn't loaded) read-modified-wrote manifest.json with no lock, unlike StarlarkAppsPlugin._update_manifest_safe (plugin-repos/starlark-apps/manager.py), which already holds an flock for the same file when the plugin is loaded. Added _starlark_manifest_lock, mirroring that pattern, and wrapped every standalone read-modify-write call site in it. The app-config update route also wrote config.json and the manifest as two separate, non-transactional writes (a second, distinct finding at the same call site); config.json is now rolled back if the manifest write that follows it fails. - backup.py: restore options used bare bool() on values from the request, so {"restore_secrets": "false"} restored secrets anyway (bool("false") is True). Switched to the existing _coerce_to_bool helper already used for this exact purpose elsewhere in the package. - config.py: an automated import-rewrite mangled four user-facing validation strings and their neighbouring comments -- "Invalid start time" had become "Invalid start _pkg.time" (and likewise for "end time") in both the schedule and dim-schedule per-day validation paths. - display.py: `import _pkg.time as time_module` -- _pkg is a local alias for the package, not a real importable module, so this raised ModuleNotFoundError whenever a caller restarted an already-running display service via /display/on-demand/start, after the on-demand request was already written to cache. Fixed to `import time`. Audited the rest of the package for the same `_pkg.<module>` import mistake; every other `_pkg.` reference is a legitimate attribute read-through (`_pkg.time.time()`, `_pkg._get_starlark_plugin()`, ...), not a broken import statement. - fonts.py: validate_file_upload's max_size_mb parameter is silently unused by that helper (it only checks filename/extension) -- the font upload route saved arbitrarily large files as a result. Added the same seek-and-check pattern already used for the sibling .star upload. - wifi.py: two ad hoc, inconsistent bool coercions. POST /wifi/ap/auto-enable used bare bool(), so a JSON string "false" enabled it. POST /wifi/radio's enabled/force parsing recognized real bool and some strings but not int 1/0 (1 is True is False in Python). Factored one small _parse_bool_ish helper local to this file and used it at all three sites. Not changed: the "unknown/misspelled restore option keys default to True" half of the backup.py finding -- the file's own comment documents that a missing key deliberately means "restore everything," matching the already-existing JSON-parse-failure guard a few lines above it; only the bool-coercion defect was a real bug. Added or extended regression tests for every fix, following each area's existing test conventions. Full suite: 4328 passed, 62 skipped, 2 failed on both this branch and origin/main (missing tzdata package breaks two timezone-alias tests in test_onboarding_checklist.py, unrelated to this change) -- no new failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5
94 lines
4.2 KiB
Python
94 lines
4.2 KiB
Python
"""Regression test: POST /display/on-demand/start restarting a running
|
|
service must not import a name that does not exist.
|
|
|
|
display.py has `import web_interface.blueprints.api_v3 as _pkg` and reads
|
|
mutable, test-patched attributes back through it (`_pkg.time.time()`,
|
|
`_pkg._get_starlark_plugin()`, ...) rather than binding them by value, per
|
|
the package's own docstring. One spot went further and wrote a genuine
|
|
`import` *statement* against that alias --
|
|
|
|
import _pkg.time as time_module
|
|
|
|
-- but `_pkg` is a local name bound by `import ... as _pkg` in this module,
|
|
not a real top-level package, so `import _pkg.time` is not something Python
|
|
can resolve; it raises ModuleNotFoundError. That line only runs when the
|
|
display service is already running and the caller also asked to (re)start
|
|
it, so this endpoint failed on exactly the restart path -- the one where a
|
|
cache write recording the new on-demand request had already happened.
|
|
|
|
The route wraps its body in `except Exception`, so the failure reached the
|
|
caller as a handled 500 with a generic message, not an unhandled crash --
|
|
but a 500 all the same on a request that should have restarted the service
|
|
and reported success.
|
|
"""
|
|
|
|
import sys
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
sys.path.insert(0, str(Path(__file__).parent.parent))
|
|
|
|
from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402
|
|
|
|
URL = "/api/v3/display/on-demand/start"
|
|
|
|
|
|
@pytest.fixture
|
|
def restart_path(api_v3_module):
|
|
"""Force the `service_was_running and start_service` branch.
|
|
|
|
plugin_manager and config_manager are set to None so the route takes
|
|
the simplest path to that branch rather than tripping over unrelated
|
|
MagicMock plumbing; _ensure_cache_manager, _get_display_service_status,
|
|
_stop_display_service and _ensure_display_service_running are bound by
|
|
value in display.py (see its own docstring), so they are patched on
|
|
that submodule rather than on the package.
|
|
"""
|
|
api_v3_module.api_v3.plugin_manager = None
|
|
api_v3_module.api_v3.config_manager = None
|
|
|
|
with patch("web_interface.blueprints.api_v3.display._ensure_cache_manager") as ensure_cache, \
|
|
patch("web_interface.blueprints.api_v3.display._get_display_service_status") as get_status, \
|
|
patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service, \
|
|
patch("web_interface.blueprints.api_v3.display._ensure_display_service_running") as ensure_running:
|
|
ensure_cache.return_value = MagicMock()
|
|
# Active before the request: service_was_running becomes True.
|
|
get_status.return_value = {"active": True}
|
|
ensure_running.return_value = {"active": True}
|
|
yield {
|
|
"ensure_cache": ensure_cache,
|
|
"get_status": get_status,
|
|
"stop_service": stop_service,
|
|
"ensure_running": ensure_running,
|
|
}
|
|
|
|
|
|
class TestRestartingARunningService:
|
|
def test_it_does_not_500(self, api_v3_client, restart_path):
|
|
response = api_v3_client.post(
|
|
URL, json={"plugin_id": "weather", "start_service": True})
|
|
body = response.get_json()
|
|
assert response.status_code == 200, body
|
|
assert body["status"] == "success", body
|
|
|
|
def test_the_service_is_actually_stopped_and_restarted(
|
|
self, api_v3_client, restart_path):
|
|
api_v3_client.post(
|
|
URL, json={"plugin_id": "weather", "start_service": True})
|
|
restart_path["stop_service"].assert_called_once()
|
|
restart_path["ensure_running"].assert_called_once()
|
|
|
|
def test_a_service_that_was_not_running_is_not_stopped_first(
|
|
self, api_v3_client, restart_path):
|
|
# The buggy import sits inside `if service_was_running and
|
|
# start_service`, so it only ever fired on the restart path --
|
|
# this is the other side of that branch, unaffected either way,
|
|
# kept here so the branch condition itself stays covered.
|
|
restart_path["get_status"].return_value = {"active": False}
|
|
response = api_v3_client.post(
|
|
URL, json={"plugin_id": "weather", "start_service": True})
|
|
assert response.status_code == 200, response.get_json()
|
|
restart_path["stop_service"].assert_not_called()
|