mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-10 00:56:35 +00:00
fix(web): keep exception messages out of API responses (py/stack-trace-exposure) (#778)
CodeQL had ~40 open py/stack-trace-exposure alerts on main. Almost all
flowed through describe_exception(), which returned "TypeName: message"
(redacted, capped); the rest through _run_systemctl_command's str(err),
WiFiManager's `return False, str(e)`, unit_refresh's f-strings and two
str(e)/f"{err}" messages in api_v3/__init__.py.
describe_exception() now returns a reason code -- the type, plus the
errno symbol for an OSError ("OSError:EIO", "PermissionError:EACCES") --
and logs the redacted message itself. That keeps what #538 wanted (a
failing disk still says EIO in the response) without quoting paths,
URLs or library internals, and fixes every call site at once; the
test_no_api_v3_handler_discards_its_exception policy still holds.
Service results: _get_display_service_status returns active/returncode
only, and the on-demand start/stop `service` result keeps
returncode/active/started/status but drops systemctl stdout/stderr
(logged on failure). Nothing in web_interface/static, the templates or
the MQTT bridge reads those fields. The Starlark SIGKILL-restart error
no longer returns systemctl stderr as `details`.
WiFi, unit-refresh, config-save and plugin-removal failures now say
what failed with the reason code and point at the log. display.py is
untouched (draft #773 edits it).
Tests: test_api_v3_no_exception_text.py drives one route per affected
file with a marker in the exception message and asserts it never
reaches the body; all 13 fail on origin/main, and targeted mutations
(drop the service filter, put stderr back, str(e) in WiFiManager,
{e} in unit_refresh, {install_err} in system.py, message back in
describe_exception) each fail at least one. Tests that asserted the old
message-in-details contract now assert the reason code.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -5,18 +5,22 @@ Safe exception descriptions and the bodies for exceptions no route handled.
|
||||
The standard success/error responses are in api_helpers.
|
||||
"""
|
||||
|
||||
import errno
|
||||
|
||||
from src.logging_config import get_logger
|
||||
from src.redaction import redact_credentials
|
||||
|
||||
logger = get_logger(__name__)
|
||||
|
||||
|
||||
# Long enough for an errno string with a path, short enough not to dump a
|
||||
# parser's worth of context into a JSON field.
|
||||
_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
|
||||
@@ -24,20 +28,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:
|
||||
|
||||
+9
-9
@@ -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"""
|
||||
|
||||
Reference in New Issue
Block a user