mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-05 23:05:10 +00:00
fix(web): keep exception messages out of API responses (py/stack-trace-exposure)
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:
@@ -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."""
|
||||
|
||||
@@ -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"]
|
||||
@@ -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"]
|
||||
|
||||
|
||||
|
||||
@@ -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 "<redacted>" 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 "<redacted>" 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)
|
||||
|
||||
@@ -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 "<redacted>" 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'
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user