diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f729e9e..ddbcbb60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +- Web API error responses no longer carry an exception's message (CodeQL + `py/stack-trace-exposure`). `describe_exception()` now returns a reason + code -- the exception type, plus the errno for an `OSError` + (`OSError:EIO`, `PermissionError:EACCES`) -- and logs the message + instead, so `details` still names the fault without quoting paths, URLs + or library internals. The display service status and the on-demand + start/stop `service` results keep `active`, `returncode` and `started` + but drop systemctl's `stdout`/`stderr`; WiFi, unit-refresh and + config-save failures say what failed and point at the log. + ## 3.8.2 The display hands freed memory back to the OS (#774), and sports consolidation diff --git a/src/web_interface/error_handler.py b/src/web_interface/error_handler.py index c3360def..5f5913b1 100644 --- a/src/web_interface/error_handler.py +++ b/src/web_interface/error_handler.py @@ -4,6 +4,7 @@ Centralized error handling for web interface. Provides helpers for consistent error responses across API endpoints. """ +import errno from typing import Any, Optional from flask import jsonify @@ -20,10 +21,9 @@ logger = get_logger(__name__) _MAX_DETAIL_LENGTH = 400 -def describe_exception(exc: BaseException, - max_length: int = _MAX_DETAIL_LENGTH) -> str: +def describe_exception(exc: BaseException) -> str: """ - One-line, safe-to-return description of an exception. + Machine-readable reason code for an exception, safe to return over HTTP. The generic "an error occurred; see logs for details" tells a user nothing and, when the failure is bad enough, the logs are unreachable too: a device @@ -31,20 +31,24 @@ def describe_exception(exc: BaseException, *including* the log viewer, because journalctl could not be executed. The underlying `[Errno 5] Input/output error` named the fault immediately. - Returns "TypeName: message", credentials redacted and length capped. The - type alone is worth carrying -- a bare PermissionError says more than any - generic sentence. + So the type and errno still go back -- "OSError:EIO", "PermissionError: + EACCES", "TimeoutExpired" -- but never the exception's message, which can + quote paths, URLs, credentials or a library's internals (CodeQL + py/stack-trace-exposure). The message is logged here instead, so every + reason code a client sees has its full text in the log. Args: exc: The exception to describe - max_length: Truncate beyond this many characters Returns: - A single-line description, never empty + "TypeName" or "TypeName:ERRNO", never empty """ - message = str(exc).strip() - text = f"{type(exc).__name__}: {message}" if message else type(exc).__name__ - return redact_text(text, max_length) + code = type(exc).__name__ + exc_errno = getattr(exc, 'errno', None) + if isinstance(exc_errno, int) and exc_errno in errno.errorcode: + code = f"{code}:{errno.errorcode[exc_errno]}" + logger.warning("Error reported to the client as %s: %s", code, redact_text(str(exc))) + return code def redact_text(text: str, max_length: int = _MAX_DETAIL_LENGTH) -> str: diff --git a/src/wifi_manager.py b/src/wifi_manager.py index 12d98797..0221a2d1 100644 --- a/src/wifi_manager.py +++ b/src/wifi_manager.py @@ -1369,7 +1369,7 @@ class WiFiManager: self.enable_ap_mode(force=True) except Exception as ap_error: # nosec B110 - last-resort; do not re-raise, but log for debugging logger.error("Last-resort AP mode enable failed in recovery path: %s", ap_error, exc_info=True) - return False, str(e) + return False, f"Connection failed ({type(e).__name__}); see logs for details" def _failsafe_ap(self, enabled_msg: str, failed_msg: str) -> Tuple[bool, str]: """Force the setup AP up after a connect that left no working network, @@ -1585,7 +1585,7 @@ class WiFiManager: except Exception as e: logger.error(f"Error connecting with nmcli: {e}") self._show_led_message("Connection error", duration=5) - return False, str(e) + return False, f"Connection failed ({type(e).__name__}); see logs for details" # 802.11 caps an SSID at 32 octets. Control characters cannot appear in a # real one, and a leading "-" would be read by nmcli as an option rather @@ -1725,7 +1725,7 @@ class WiFiManager: return False, "nmcli is required to disconnect from WiFi" except Exception as e: logger.error(f"Error disconnecting from WiFi: {e}") - return False, str(e) + return False, f"Disconnect failed ({type(e).__name__}); see logs for details" def _ensure_wifi_radio_enabled(self, max_retries: int = 3) -> bool: """ @@ -2004,7 +2004,7 @@ class WiFiManager: return False, "No WiFi tools available (nmcli, hostapd, or dnsmasq required)" except Exception as e: logger.error(f"Error in enable_ap_mode: {e}") - return False, str(e) + return False, f"Could not enable AP mode ({type(e).__name__}); see logs for details" def _mark_forced(self) -> None: """Record that AP mode was forced on, so the periodic check leaves it @@ -2099,10 +2099,10 @@ class WiFiManager: return True, "AP mode enabled" except Exception as e: logger.error(f"Error starting AP services: {e}") - return False, str(e) + return False, f"Could not enable AP mode ({type(e).__name__}); see logs for details" except Exception as e: logger.error(f"Error enabling AP mode: {e}") - return False, str(e) + return False, f"Could not enable AP mode ({type(e).__name__}); see logs for details" def _enable_ap_mode_nmcli_hotspot(self) -> Tuple[bool, str]: """ @@ -2227,7 +2227,7 @@ class WiFiManager: logger.error(f"Error starting AP mode with nmcli: {e}") self._remove_nm_dnsmasq_captive_conf() self._show_led_message("Setup mode error", duration=5) - return False, str(e) + return False, f"Could not enable AP mode ({type(e).__name__}); see logs for details" def _get_ap_status_nmcli(self) -> Dict: """ @@ -2409,10 +2409,10 @@ class WiFiManager: return True, "AP mode disabled" except Exception as e: logger.error(f"Error stopping AP services: {e}") - return False, str(e) + return False, f"Could not disable AP mode ({type(e).__name__}); see logs for details" except Exception as e: logger.error(f"Error disabling AP mode: {e}") - return False, str(e) + return False, f"Could not disable AP mode ({type(e).__name__}); see logs for details" def _create_hostapd_config(self): """Create hostapd configuration file""" diff --git a/test/test_api_v3_display_modes.py b/test/test_api_v3_display_modes.py index a59913f0..323894d3 100644 --- a/test/test_api_v3_display_modes.py +++ b/test/test_api_v3_display_modes.py @@ -147,7 +147,8 @@ class TestOneBadConfigSectionDoesNotBlankTheList: side_effect=RuntimeError("disk is gone")) resp = api_v3_client.get('/api/v3/display/modes') assert resp.status_code == 500 - assert 'disk is gone' in resp.get_json()['details'] + assert resp.get_json()['details'] == 'RuntimeError' + assert 'disk is gone' not in json.dumps(resp.get_json()) def test_credentials_in_the_exception_are_redacted(self, api_v3_module, api_v3_client): """describe_exception is what makes returning detail safe.""" diff --git a/test/test_api_v3_no_exception_text.py b/test/test_api_v3_no_exception_text.py new file mode 100644 index 00000000..5f930576 --- /dev/null +++ b/test/test_api_v3_no_exception_text.py @@ -0,0 +1,147 @@ +"""No API response carries an exception's message (CodeQL py/stack-trace-exposure). + +One representative route per file that had open alerts. Each forces a failure +whose message holds a marker and asserts the marker is nowhere in the body: +the message goes to the log, the client gets a fixed message plus a reason +code (describe_exception: the type, and the errno for an OSError). +""" + +import json +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 + +LEAK = "LEAKED-/home/pi/secret token=abc123" +API = "web_interface.blueprints.api_v3" + + +def _assert_no_leak(response): + body = response.get_data(as_text=True) + assert "LEAKED" not in body, body + assert "abc123" not in body, body + return json.loads(body) + + +def test_display_service_status_drops_systemctl_output(api_v3_module, api_v3_client, + monkeypatch): + """display.py: the on-demand routes return the service status verbatim.""" + api_v3_module.api_v3.cache_manager.get.return_value = None + monkeypatch.setattr(f"{API}.display.display_state.read_state", lambda: None) + with patch(f"{API}.subprocess.run", side_effect=OSError(13, LEAK)): + body = _assert_no_leak(api_v3_client.get("/api/v3/display/on-demand/status")) + assert body["data"]["service"] == {"active": False, "returncode": -1} + + +@pytest.mark.parametrize("helper", ["_ensure_display_service_running", + "_stop_display_service"]) +def test_service_results_keep_returncode_but_not_output(api_v3_module, helper): + """display.py start/stop: returncode/active/started stay, stdout/stderr go.""" + failed = MagicMock(returncode=1, stdout=LEAK, stderr=LEAK) + with patch(f"{API}.subprocess.run", return_value=failed): + result = getattr(api_v3_module, helper)() + assert "LEAKED" not in json.dumps(result) + assert result["returncode"] == 1 and result["active"] is False + assert "stdout" not in result and "stderr" not in result + + +def test_wifi_connect_failure(api_v3_client): + """wifi.py: a raising connect, and the attempt /wifi/status reports after.""" + with patch("src.wifi_manager.WiFiManager") as cls: + cls.return_value._is_ap_mode_active.return_value = False + cls.return_value.connect_to_network.side_effect = RuntimeError(LEAK) + body = _assert_no_leak(api_v3_client.post( + "/api/v3/wifi/connect", json={"ssid": "HomeNet", "password": "pw"})) + assert body["details"] == "RuntimeError" + cls.return_value.get_wifi_status.return_value = MagicMock( + connected=False, ssid=None, ip_address=None, signal=0, ap_mode_active=False) + cls.return_value.config = {} + status = _assert_no_leak(api_v3_client.get("/api/v3/wifi/status")) + assert status["data"]["last_connect_attempt"]["message"] == ( + "Failed to connect to network (RuntimeError)") + + +def test_wifi_manager_messages_carry_no_exception_text(): + """src/wifi_manager.py: its (success, message) is what the wifi routes return.""" + from src.wifi_manager import WiFiManager + manager = WiFiManager.__new__(WiFiManager) # no __init__: no host access + manager.get_wifi_status = MagicMock(side_effect=OSError(5, LEAK)) + success, message = manager.disconnect_from_network() + assert success is False + assert "LEAKED" not in message and "OSError" in message + + +def test_system_action_exception(api_v3_client): + """system.py: execute_system_action's catch-all.""" + with patch("subprocess.run", side_effect=OSError(5, LEAK)): + body = _assert_no_leak(api_v3_client.post( + "/api/v3/system/action", json={"action": "stop_display"})) + assert body["details"] == "OSError:EIO" + + +def test_calendar_registration_failure(api_v3_client, tmp_path, monkeypatch): + """plugin_calendar.py: the auth script could not be run.""" + plugin_dir = tmp_path / "calendar" + plugin_dir.mkdir() + (plugin_dir / "credentials.json").write_text("{}", encoding="utf-8") + (plugin_dir / "calendar_registration.py").write_text("", encoding="utf-8") + monkeypatch.setattr(f"{API}._calendar_plugin_dir", lambda: plugin_dir) + with patch(f"{API}.subprocess.run", side_effect=OSError(13, LEAK)): + body = _assert_no_leak(api_v3_client.post( + "/api/v3/plugins/calendar/authenticate", json={"code": "x"})) + assert "EACCES" in body["message"] + + +def test_health_failure(api_v3_client, monkeypatch): + """misc.py: get_health's catch-all.""" + def boom(): + raise RuntimeError(LEAK) + monkeypatch.setattr(f"{API}.misc._get_display_service_status", boom) + body = _assert_no_leak(api_v3_client.get("/api/v3/health")) + assert body["details"] == "RuntimeError" + + +def test_config_route_failure(api_v3_module, api_v3_client): + """error_handler.py: create_error_response, as config.py's routes use it.""" + api_v3_module.api_v3.config_manager.load_config.side_effect = RuntimeError(LEAK) + body = _assert_no_leak(api_v3_client.get("/api/v3/config/schedule")) + assert body["details"] == "RuntimeError" + + +def test_plugin_route_failure(api_v3_module, api_v3_client): + """plugins.py: an unhandled error in a plugin route.""" + api_v3_module.api_v3.plugin_catalog.get_all_plugin_info.side_effect = RuntimeError(LEAK) + body = _assert_no_leak(api_v3_client.get("/api/v3/plugins/installed")) + assert body["details"] == "RuntimeError" + + +def test_starlark_route_failure(api_v3_client): + """starlark.py: one of its catch-alls.""" + with patch(f"{API}._get_starlark_plugin", side_effect=RuntimeError(LEAK)): + body = _assert_no_leak(api_v3_client.get("/api/v3/starlark/status")) + assert body["details"] == "RuntimeError" + + +def test_unit_refresh_failure(monkeypatch): + """system.py git_pull: perform_core_update appends unit_refresh's message.""" + from web_interface import unit_refresh + + def boom(*_a, **_k): + raise RuntimeError(LEAK) + monkeypatch.setattr(unit_refresh, "stale_units", boom) + result = unit_refresh.refresh_after_update() + assert result["status"] == unit_refresh.FAILED + assert "LEAKED" not in result["message"] + + +def test_install_base_requirements_failure(api_v3_client): + """system.py: a pip install that could not start, in the action's output.""" + with patch(f"{API}.system._pip_install_requirements", side_effect=OSError(5, LEAK)): + body = _assert_no_leak(api_v3_client.post( + "/api/v3/system/action", json={"action": "install_base_requirements"})) + assert "Failed: OSError:EIO" in body["output"] diff --git a/test/test_api_v3_registry_endpoints.py b/test/test_api_v3_registry_endpoints.py index b4e6dd1f..ba0e5f96 100644 --- a/test/test_api_v3_registry_endpoints.py +++ b/test/test_api_v3_registry_endpoints.py @@ -95,9 +95,10 @@ class TestRefreshPluginStore: RuntimeError("failed at /home/user/LEDMatrix/src/secret.py line 42")) body = api_v3_client.post(self.URL, json={}).get_json() assert "Traceback" not in str(body) - # `details` is describe_exception output: one line, type-named, - # credential-redacted. It may quote the message, but never a stack. - assert body["details"].startswith("RuntimeError:") + # `details` is describe_exception output: the type, never the + # message or a stack. + assert body["details"] == "RuntimeError" + assert "secret.py" not in str(body) assert "\n" not in body["details"] diff --git a/test/test_web_error_detail.py b/test/test_web_error_detail.py index 2fb3a2f7..f0e376fd 100644 --- a/test/test_web_error_detail.py +++ b/test/test_web_error_detail.py @@ -11,26 +11,32 @@ than even logging it. import pytest -from src.web_interface.error_handler import describe_exception +from src.web_interface.error_handler import describe_exception, redact_text class TestDescribeException: - def test_names_the_type_and_message(self): - detail = describe_exception(OSError(5, "Input/output error", "systemctl")) - assert detail == "OSError: [Errno 5] Input/output error: 'systemctl'" + """describe_exception is a reason code: type and errno, never the message. - def test_the_reported_failure_is_legible(self): - # The whole point: this string is the diagnosis. - assert "Input/output error" in describe_exception( - OSError(5, "Input/output error", "systemctl")) + The message can quote paths, URLs or credentials (CodeQL + py/stack-trace-exposure), so it goes to the log; the code still names the + fault, as "[Errno 5]" did. + """ + + def test_an_oserror_names_its_errno(self): + assert describe_exception( + OSError(5, "Input/output error", "systemctl")) == "OSError:EIO" def test_a_bare_exception_still_names_its_type(self): - # A PermissionError with no message still says more than "unknown". assert describe_exception(PermissionError()) == "PermissionError" assert describe_exception(Exception()) == "Exception" - def test_message_is_kept_when_present(self): - assert describe_exception(ValueError("bad port")) == "ValueError: bad port" + def test_the_message_never_reaches_the_code(self): + assert describe_exception(ValueError("bad port /etc/secret")) == "ValueError" + + def test_the_message_is_logged_instead(self, caplog): + describe_exception(RuntimeError("disk on fire token=abc123")) + assert "disk on fire" in caplog.text + assert "abc123" not in caplog.text class TestCredentialRedaction: @@ -56,47 +62,44 @@ class TestCredentialRedaction: ("authorization: barecredential", "barecredential"), ]) def test_credentials_never_reach_the_response(self, secret_text, leaked): - detail = describe_exception(RuntimeError(secret_text)) + detail = redact_text(secret_text) assert leaked not in detail assert "" in detail def test_the_parameter_name_survives_redaction(self): # Knowing *which* credential was involved is part of the diagnosis. - detail = describe_exception(RuntimeError("https://x/y?api_key=SEC123")) + detail = redact_text("https://x/y?api_key=SEC123") assert "api_key" in detail def test_unknown_schemes_keep_their_name(self): for scheme in ("ApiKey", "Negotiate", "NTLM", "AWS4-HMAC-SHA256"): - detail = describe_exception( - RuntimeError("Authorization: %s SECRETVALUE" % scheme)) + detail = redact_text("Authorization: %s SECRETVALUE" % scheme) assert scheme in detail, detail assert "SECRETVALUE" not in detail, detail def test_auth_scheme_and_username_survive(self): # Which kind of credential, and whose, without the credential itself. - assert "Bearer" in describe_exception( - RuntimeError("Authorization: Bearer eyJ.SECRET.sig")) - assert "user" in describe_exception( - RuntimeError("https://user:hunter2@example.com")) + assert "Bearer" in redact_text("Authorization: Bearer eyJ.SECRET.sig") + assert "user" in redact_text("https://user:hunter2@example.com") def test_non_secret_context_is_preserved(self): - detail = describe_exception(RuntimeError("https://api.x.com/v1?city=Tampa")) + detail = redact_text("https://api.x.com/v1?city=Tampa") assert "city=Tampa" in detail assert "" not in detail class TestBounds: def test_long_messages_are_truncated(self): - detail = describe_exception(ValueError("x" * 5000)) + detail = redact_text("x" * 5000) assert len(detail) <= 400 def test_newlines_are_collapsed_to_one_line(self): - detail = describe_exception(ValueError("line one\nline two\tthree")) + detail = redact_text("line one\nline two\tthree") assert "\n" not in detail and "\t" not in detail - assert detail == "ValueError: line one line two three" + assert detail == "line one line two three" def test_custom_length_is_honoured(self): - assert len(describe_exception(ValueError("y" * 500), max_length=50)) <= 50 + assert len(redact_text("y" * 500, max_length=50)) <= 50 class TestHandlersCarryDetail: @@ -319,10 +322,10 @@ class TestHandlersCarryDetail: assert resp.status_code == 405, "a wrong method must stay a 405" assert resp.get_json()["error_code"] == "METHOD_NOT_ALLOWED" - # A genuine server fault still reports as one, with its detail. + # A genuine server fault still reports as one, with its reason code. resp = client.get("/boom") assert resp.status_code == 500 - assert "Input/output error" in resp.get_json()["details"] + assert resp.get_json()["details"] == "OSError:EIO" def test_global_handler_reports_the_underlying_error(self): from flask import Flask, jsonify @@ -345,4 +348,5 @@ class TestHandlersCarryDetail: client = app.test_client() body = client.get("/boom").get_json() assert body["error_code"] == "UNKNOWN_ERROR" - assert "Input/output error" in body["details"] + assert body["details"] == "OSError:EIO" + assert "Input/output error" not in str(body) diff --git a/test/web_interface/test_api_v3_unhandled_errors.py b/test/web_interface/test_api_v3_unhandled_errors.py index 49ab0dbc..d8c9652a 100644 --- a/test/web_interface/test_api_v3_unhandled_errors.py +++ b/test/web_interface/test_api_v3_unhandled_errors.py @@ -114,12 +114,11 @@ def test_the_answer_is_what_the_catch_all_returned(client, caplog, method, url, assert records[-1].exc_info[1] is FORCED -def test_credentials_are_redacted_from_the_detail(client): +def test_the_exception_message_never_reaches_the_detail(client): body = client.get("/api/v3/plugins/installed").get_json() - for secret in ("SECRET123", "pw1", "K1"): - assert secret not in body["details"] - assert "" in body["details"] - assert body["details"].startswith("RuntimeError: forced failure") + for secret in ("SECRET123", "pw1", "K1", "forced failure"): + assert secret not in str(body) + assert body["details"] == "RuntimeError" def _raise_415(): @@ -218,7 +217,7 @@ class TestPluginActionStep1: encoding="utf-8") return d - def test_the_script_error_reaches_the_response(self, plugin_dir, monkeypatch): + def test_the_script_error_is_reported_by_type(self, plugin_dir, monkeypatch): from unittest.mock import MagicMock manager = MagicMock() manager.get_plugin_directory.return_value = str(plugin_dir) @@ -232,5 +231,6 @@ class TestPluginActionStep1: assert resp.status_code == 500 body = resp.get_json() - assert body["details"] == "RuntimeError: the auth script failed" + assert body["details"] == "RuntimeError" + assert "the auth script failed" not in str(body) assert body["message"] == 'An error occurred; see logs for details' diff --git a/test/web_interface/test_starlark_pixlet_routes.py b/test/web_interface/test_starlark_pixlet_routes.py index 92102c48..55c55928 100644 --- a/test/web_interface/test_starlark_pixlet_routes.py +++ b/test/web_interface/test_starlark_pixlet_routes.py @@ -946,21 +946,27 @@ class TestTheStoreReportsWhyItIsEmpty: class TestACrashCarriesItsDetail: - """Seventeen Starlark handlers answered 5xx with no detail at all.""" + """Seventeen Starlark handlers answered 5xx with no detail at all. - def test_browse_returns_the_exception_detail(self, client): + The detail is a reason code (the exception type), not the exception's + message, which stays in the log (CodeQL py/stack-trace-exposure). + """ + + def test_browse_returns_the_reason_code(self, client): with patch('web_interface.blueprints.api_v3._get_tronbyte_repository_class', side_effect=ImportError("No module named 'yaml'")): body = client.get('/api/v3/starlark/repository/browse').get_json() - assert 'yaml' in body.get('details', ''), body + assert body.get('details') == 'ImportError', body + assert 'yaml' not in str(body), body - def test_status_returns_the_exception_detail(self, client): + def test_status_returns_the_reason_code(self, client): with patch('web_interface.blueprints.api_v3._get_starlark_plugin', side_effect=RuntimeError("plugin manager is not attached")): body = client.get('/api/v3/starlark/status').get_json() - assert 'plugin manager is not attached' in body.get('details', ''), body + assert body.get('details') == 'RuntimeError', body + assert 'plugin manager is not attached' not in str(body), body class TestTheListingIsNotCappedAtOneThousand: diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index b40105be..f20d223d 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -222,7 +222,7 @@ def _save_config_atomic(config_manager, config_data, create_backup=True): config_manager.save_config(config_data) return True, None except Exception as e: - return False, str(e) + return False, f"Failed to save configuration ({describe_exception(e)})" def _coerce_to_bool(value): """ Coerce a form value to a proper Python boolean. @@ -246,7 +246,11 @@ def _coerce_to_bool(value): return value.lower() in ('true', 'on', '1', 'yes') return False def _get_display_service_status(): - """Return status information about the ledmatrix service.""" + """Return status information about the ledmatrix service. + + active/returncode only: this goes back in API responses, and systemctl's + output (or an exception's text) is logged rather than returned. + """ try: result = subprocess.run( ['systemctl', 'is-active', 'ledmatrix'], @@ -254,26 +258,18 @@ def _get_display_service_status(): text=True, timeout=3 ) + if result.stderr.strip(): + logger.debug('systemctl is-active ledmatrix: %s', result.stderr.strip()) return { 'active': result.stdout.strip() == 'active', 'returncode': result.returncode, - 'stdout': result.stdout.strip(), - 'stderr': result.stderr.strip() } except subprocess.TimeoutExpired: - return { - 'active': False, - 'returncode': -1, - 'stdout': '', - 'stderr': 'timeout' - } - except Exception as err: - return { - 'active': False, - 'returncode': -1, - 'stdout': '', - 'stderr': str(err) - } + logger.warning('systemctl is-active ledmatrix timed out') + return {'active': False, 'returncode': -1} + except Exception: + logger.warning('Could not query ledmatrix.service status', exc_info=True) + return {'active': False, 'returncode': -1} def _run_systemctl_command(args): """Run a systemctl command safely.""" try: @@ -295,18 +291,26 @@ def _run_systemctl_command(args): 'stderr': 'timeout' } except Exception as err: + logger.warning('%s failed', ' '.join(args), exc_info=True) return { 'returncode': -1, 'stdout': '', - 'stderr': str(err) + 'stderr': describe_exception(err) } +def _public_service_result(result): + """A _run_systemctl_command result fit for a response: no stdout/stderr.""" + if result.get('returncode') != 0: + logger.error('systemctl exited %s: %s', result.get('returncode'), + (result.get('stderr') or '').strip()) + return {k: v for k, v in result.items() if k not in ('stdout', 'stderr')} def _ensure_display_service_running(): """Ensure the ledmatrix display service is running.""" status = _get_display_service_status() if status.get('active'): status['started'] = False return status - result = _run_systemctl_command(['sudo', 'systemctl', 'start', 'ledmatrix.service']) + result = _public_service_result( + _run_systemctl_command(['sudo', 'systemctl', 'start', 'ledmatrix.service'])) service_status = _get_display_service_status() result['started'] = result.get('returncode') == 0 result['active'] = service_status.get('active') @@ -314,7 +318,8 @@ def _ensure_display_service_running(): return result def _stop_display_service(): """Stop the ledmatrix display service.""" - result = _run_systemctl_command(['sudo', 'systemctl', 'stop', 'ledmatrix.service']) + result = _public_service_result( + _run_systemctl_command(['sudo', 'systemctl', 'stop', 'ledmatrix.service'])) status = _get_display_service_status() result['active'] = status.get('active') result['status'] = status @@ -712,7 +717,7 @@ def _do_transactional_uninstall(plugin_id, preserve_config): success = api_v3.plugin_store_manager.uninstall_plugin(plugin_id) except Exception as remove_err: _rollback() - return False, f"Failed to remove plugin {plugin_id}: {remove_err}" + return False, f"Failed to remove plugin {plugin_id} ({describe_exception(remove_err)})" if not success: _rollback() diff --git a/web_interface/blueprints/api_v3/starlark.py b/web_interface/blueprints/api_v3/starlark.py index c6d679bf..1ea32d6a 100644 --- a/web_interface/blueprints/api_v3/starlark.py +++ b/web_interface/blueprints/api_v3/starlark.py @@ -983,7 +983,6 @@ def stop_pixlet_editor(): 'status': 'error', 'message': 'Editor force-stopped, but the display could not be ' 'restarted automatically - start it manually.', - 'details': (result.get('stderr') or '').strip(), 'data': {'running': False}}), 500 return jsonify({'status': 'success', 'message': 'Editor force-stopped; the display has been ' diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index 0b22e20d..35375332 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/web_interface/blueprints/api_v3/system.py @@ -689,7 +689,7 @@ def execute_system_action(): logger.warning("install_base_requirements timed out for %s", label) except OSError as install_err: all_ok = False - outputs.append(f"== {label} ==\nFailed: {install_err}") + outputs.append(f"== {label} ==\nFailed: {describe_exception(install_err)}") logger.warning("install_base_requirements errored for %s: %s", label, install_err) return jsonify({ 'status': 'success' if all_ok else 'error', @@ -784,11 +784,10 @@ def execute_system_action(): return jsonify({'status': 'error', 'message': 'Command timed out', 'returncode': -1, 'stderr': 'timeout'}) except Exception as e: logger.error("execute_system_action failed: %s", e, exc_info=True) - detail = describe_exception(e) resp = { 'status': 'error', - 'message': _sudo_hint_for(detail) or 'Action failed; see logs for details', - 'details': detail, + 'message': _sudo_hint_for(str(e)) or 'Action failed; see logs for details', + 'details': describe_exception(e), } return jsonify(resp), 500 @api_v3.route('/system/git-info', methods=['GET']) diff --git a/web_interface/blueprints/api_v3/wifi.py b/web_interface/blueprints/api_v3/wifi.py index c38f0462..8d26f989 100644 --- a/web_interface/blueprints/api_v3/wifi.py +++ b/web_interface/blueprints/api_v3/wifi.py @@ -67,7 +67,7 @@ def _run_background_connect(ssid, password): payload = _connect_result_payload(ssid, success, message) except Exception as e: logger.error("Background WiFi connect failed", exc_info=True) - payload = {'status': 'error', 'message': describe_exception(e)} + payload = {'status': 'error', 'message': f'Failed to connect to network ({describe_exception(e)})'} _record_connect_result(ssid, payload) @@ -276,7 +276,7 @@ def connect_wifi(): try: success, message = wifi_manager.connect_to_network(ssid, password) except Exception as e: - _record_connect_result(ssid, {'status': 'error', 'message': describe_exception(e)}) + _record_connect_result(ssid, {'status': 'error', 'message': f'Failed to connect to network ({describe_exception(e)})'}) raise payload = _connect_result_payload(ssid, success, message) _record_connect_result(ssid, payload) diff --git a/web_interface/unit_refresh.py b/web_interface/unit_refresh.py index 611a69a9..52580404 100644 --- a/web_interface/unit_refresh.py +++ b/web_interface/unit_refresh.py @@ -86,7 +86,7 @@ def refresh_after_update(run=None, systemd_dir=None, helper_source=None, helper_ except Exception as e: # a broken template must not fail the update itself if type(e).__name__ != 'UnitsUnreadable': logger.warning("Could not compare the installed systemd units with the new templates: %s", e) - return _result(FAILED, f'The service settings could not be checked: {e}.') + return _result(FAILED, 'The service settings could not be checked; see logs for details.') # Units installed mode 0600 (install_service.sh run on its own, before # it set 0644): only root can compare them, so let the helper decide. stale = [] @@ -110,7 +110,7 @@ def refresh_after_update(run=None, systemd_dir=None, helper_source=None, helper_ timeout=TIMEOUT_SECONDS) except (subprocess.SubprocessError, OSError) as e: logger.warning("Refreshing the systemd units failed: %s", e) - return _result(FAILED, f'Updating the service settings ({names}) failed: {e}.', stale) + return _result(FAILED, f'Updating the service settings ({names}) failed; see logs for details.', stale) if result.returncode == 0: # The helper says what it did: "units refreshed: a b" or "units: up to date". done = next((line.split(':', 1)[1].split() for line in (result.stdout or '').splitlines()