mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-18 04:49:13 +00:00
fix(api-v3): address CodeRabbit findings from the blueprint-split review
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
This commit is contained in:
@@ -229,6 +229,61 @@ class TestRadio:
|
||||
assert api_v3_client.post(self.URL, json={"enabled": True}).status_code == 500
|
||||
|
||||
|
||||
class TestAutoEnableApMode:
|
||||
URL = "/api/v3/wifi/ap/auto-enable"
|
||||
|
||||
def test_requires_the_field(self, api_v3_client, wifi_manager):
|
||||
response = api_v3_client.post(self.URL, json={})
|
||||
assert response.status_code == 400
|
||||
|
||||
@pytest.mark.parametrize("raw,expected", [
|
||||
(True, True), (False, False),
|
||||
("true", True), ("True", True), ("1", True), ("yes", True),
|
||||
("false", False), ("False", False), ("0", False), ("no", False),
|
||||
(1, True), (0, False),
|
||||
])
|
||||
def test_value_is_coerced_not_just_truthy(
|
||||
self, api_v3_client, wifi_manager, raw, expected):
|
||||
# Regression: bool(data['auto_enable_ap_mode']) meant a caller who
|
||||
# sent the JSON string "false" got it stored as True — bool("false")
|
||||
# is True, since any non-empty string is truthy.
|
||||
wifi_manager.config = {}
|
||||
response = api_v3_client.post(self.URL, json={"auto_enable_ap_mode": raw})
|
||||
assert response.status_code == 200, response.get_json()
|
||||
assert response.get_json()["data"]["auto_enable_ap_mode"] is expected
|
||||
assert wifi_manager.config["auto_enable_ap_mode"] is expected
|
||||
|
||||
def test_a_string_false_does_not_enable_it(self, api_v3_client, wifi_manager):
|
||||
# The exact shape of the bug.
|
||||
wifi_manager.config = {}
|
||||
api_v3_client.post(self.URL, json={"auto_enable_ap_mode": "false"})
|
||||
assert wifi_manager.config["auto_enable_ap_mode"] is False
|
||||
|
||||
|
||||
class TestRadioEnabledAndForceAcceptIntegers:
|
||||
"""`{"enabled": 1}` / `{"enabled": 0}` used to be mishandled: the old
|
||||
coercion was `raw is True or (isinstance(raw, str) and ...)`, and
|
||||
`1 is True` is False in Python -- an int is never the `True` singleton
|
||||
even though it equals it -- so a plain integer fell through to False
|
||||
regardless of its value.
|
||||
"""
|
||||
URL = "/api/v3/wifi/radio"
|
||||
|
||||
@pytest.mark.parametrize("raw,expected", [(1, True), (0, False)])
|
||||
def test_enabled_as_an_integer(self, api_v3_client, wifi_manager, raw, expected):
|
||||
wifi_manager.set_wifi_radio.return_value = (True, "ok", None)
|
||||
wifi_manager.get_wifi_radio_state.return_value = {}
|
||||
api_v3_client.post(self.URL, json={"enabled": raw})
|
||||
wifi_manager.set_wifi_radio.assert_called_once_with(expected, force=False)
|
||||
|
||||
@pytest.mark.parametrize("raw,expected", [(1, True), (0, False)])
|
||||
def test_force_as_an_integer(self, api_v3_client, wifi_manager, raw, expected):
|
||||
wifi_manager.set_wifi_radio.return_value = (True, "ok", None)
|
||||
wifi_manager.get_wifi_radio_state.return_value = {}
|
||||
api_v3_client.post(self.URL, json={"enabled": True, "force": raw})
|
||||
wifi_manager.set_wifi_radio.assert_called_once_with(True, force=expected)
|
||||
|
||||
|
||||
class TestNoRealNetworking:
|
||||
def test_wifi_manager_is_never_constructed_for_real(self, api_v3_client):
|
||||
# Guard against a future refactor moving the import to module level,
|
||||
|
||||
Reference in New Issue
Block a user