diff --git a/src/web_interface/error_handler.py b/src/web_interface/error_handler.py index 0af53732..d22fed45 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 re from typing import Any, Optional from flask import jsonify @@ -16,6 +17,54 @@ from src.logging_config import get_logger logger = get_logger(__name__) +# Credentials that turn up inside exception text. A requests error quotes the +# URL it failed on, and plugins that authenticate by query string put their key +# there, so echoing an exception verbatim can hand out an API key. Redact the +# value, keep the parameter name -- knowing *which* credential was involved is +# part of the diagnosis. +_REDACT_CREDENTIAL = re.compile( + r'((?:api[_-]?key|access[_-]?token|auth|apikey|key|passwd|password|pwd|' + r'secret|sig|signature|token)["\']?\s*[=:]\s*["\']?)([^\s&"\'<>,}]+)', + re.IGNORECASE, +) + +# 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: + """ + One-line, safe-to-return description of an exception. + + 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 + whose storage was failing returned that message from every endpoint + *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. + + Args: + exc: The exception to describe + max_length: Truncate beyond this many characters + + Returns: + A single-line description, never empty + """ + message = str(exc).strip() + text = f"{type(exc).__name__}: {message}" if message else type(exc).__name__ + text = _REDACT_CREDENTIAL.sub(r'\1', text) + # Collapse newlines/tabs so the detail stays one line in a JSON field. + text = ' '.join(text.split()) + if len(text) > max_length: + text = text[:max_length - 1].rstrip() + '…' + return text + + def create_error_response( error_code: ErrorCode, message: str, diff --git a/test/test_web_error_detail.py b/test/test_web_error_detail.py new file mode 100644 index 00000000..553e2f3b --- /dev/null +++ b/test/test_web_error_detail.py @@ -0,0 +1,121 @@ +"""Tests for surfacing the underlying error in web responses. + +Regression under test: every failing endpoint returned "An error occurred; see +logs for details" and nothing else. On a device whose storage was failing that +sentence came back from the restart action, from /system/status, and from +/logs -- the log viewer itself -- because journalctl could not be executed. The +exception underneath said `[Errno 5] Input/output error: 'systemctl'`, which +names the fault outright, and nine handlers were discarding it entirely rather +than even logging it. +""" + +import pytest + +from src.web_interface.error_handler import describe_exception + + +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'" + + 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")) + + 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" + + +class TestCredentialRedaction: + """Exception text quotes URLs, and plugins authenticate by query string.""" + + @pytest.mark.parametrize("secret_text,leaked", [ + ("failed: https://api.x.com/v1?api_key=SEC123&city=Tampa", "SEC123"), + ("token=abcdef123456 was rejected", "abcdef123456"), + ("connect failed password=hunter2", "hunter2"), + ("GET /?access_token=zzz999", "zzz999"), + ('{"secret": "topsecret"}', "topsecret"), + ]) + def test_credentials_never_reach_the_response(self, secret_text, leaked): + detail = describe_exception(RuntimeError(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")) + assert "api_key" in detail + + def test_non_secret_context_is_preserved(self): + detail = describe_exception(RuntimeError("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)) + assert len(detail) <= 400 + + def test_newlines_are_collapsed_to_one_line(self): + detail = describe_exception(ValueError("line one\nline two\tthree")) + assert "\n" not in detail and "\t" not in detail + assert detail == "ValueError: line one line two three" + + def test_custom_length_is_honoured(self): + assert len(describe_exception(ValueError("y" * 500), max_length=50)) <= 50 + + +class TestHandlersCarryDetail: + """The response shape callers actually see.""" + + def test_no_api_v3_handler_discards_its_exception(self): + # Nine of them bound `e` and never used it, so the promised log entry + # was never written either. + import ast + import re + + src = open("web_interface/blueprints/api_v3.py").read() + tree = ast.parse(src) + generic = "An error occurred; see logs for details" + silent = [] + for h in [n for n in ast.walk(tree) if isinstance(n, ast.ExceptHandler)]: + seg = ast.get_source_segment(src, h) or "" + if generic not in seg: + continue + if not re.search( + r"\b(logger|logging|current_app\.logger)\s*\.\s*" + r"(error|exception|warning|critical|info)\b", seg): + silent.append(h.lineno) + assert not silent, ( + "handlers returning the generic message without logging: %r" % silent) + + def test_global_handler_reports_the_underlying_error(self): + from flask import Flask, jsonify + + app = Flask(__name__) + + @app.errorhandler(Exception) + def handle(error): + return jsonify({ + "status": "error", + "error_code": "UNKNOWN_ERROR", + "message": "An error occurred; see logs for details", + "details": describe_exception(error), + }), 500 + + @app.route("/boom") + def boom(): + raise OSError(5, "Input/output error", "systemctl") + + client = app.test_client() + body = client.get("/boom").get_json() + assert body["error_code"] == "UNKNOWN_ERROR" + assert "Input/output error" in body["details"] diff --git a/web_interface/app.py b/web_interface/app.py index 74ebcfe0..ecfb7f39 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -16,6 +16,7 @@ from datetime import datetime, timedelta sys.path.insert(0, str(Path(__file__).parent.parent)) from src.config_manager import ConfigManager +from src.web_interface.error_handler import describe_exception from src.exceptions import ConfigError from src.plugin_system.plugin_manager import PluginManager from src.plugin_system.store_manager import PluginStoreManager @@ -391,15 +392,30 @@ def internal_error(error): import logging logger = logging.getLogger('web_interface') logger.error("Internal server error", exc_info=True) - return jsonify({ + payload = { 'status': 'error', 'error_code': 'INTERNAL_ERROR', 'message': 'An internal error occurred; see logs for details', - }), 500 + } + # Flask hands the original exception over as `error.original_exception` + # when propagation is off; without it there is nothing to describe. + original = getattr(error, 'original_exception', None) or ( + error if isinstance(error, BaseException) else None) + if original is not None: + payload['details'] = describe_exception(original) + return jsonify(payload), 500 @app.errorhandler(Exception) def handle_exception(error): - """Handle all unhandled exceptions.""" + """Handle all unhandled exceptions. + + Returning only "see logs for details" is fine until the logs are exactly + what you cannot reach. A device with failing storage answered every + endpoint with that sentence -- including the log viewer, because journalctl + could not be executed -- while the exception underneath said + `[Errno 5] Input/output error`. Naming the error costs nothing here and is + frequently the whole diagnosis, so include it alongside the log pointer. + """ import logging logger = logging.getLogger('web_interface') logger.error("Unhandled exception", exc_info=True) @@ -407,6 +423,7 @@ def handle_exception(error): 'status': 'error', 'error_code': 'UNKNOWN_ERROR', 'message': 'An error occurred; see logs for details', + 'details': describe_exception(error), }), 500 # Captive portal redirect middleware diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 4c80ed07..8b6a2a71 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -21,6 +21,7 @@ logger = logging.getLogger(__name__) # Import new infrastructure from src.web_interface.api_helpers import success_response, error_response, validate_request_json from src.web_interface.errors import ErrorCode +from src.web_interface.error_handler import describe_exception from src.plugin_system.operation_types import OperationType from src.web_interface.validators import ( validate_file_upload @@ -289,9 +290,11 @@ def get_schedule_config(): return success_response(data=schedule_config) except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return error_response( ErrorCode.CONFIG_LOAD_FAILED, "An error occurred; see logs for details", + details=describe_exception(e), status_code=500 ) @@ -1625,9 +1628,11 @@ def get_health(): return jsonify({'status': 'success', 'data': health_status}) except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e), 'data': {'status': 'unhealthy'} }), 500 @@ -6563,7 +6568,10 @@ def get_fonts_catalog(): return jsonify({'status': 'success', 'data': {'catalog': catalog}}) except Exception as e: - return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details'}), 500 + logger.error("%s failed", request.path, exc_info=True) + return jsonify({'status': 'error', + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e)}), 500 @api_v3.route('/fonts/tokens', methods=['GET']) def get_font_tokens(): @@ -7522,9 +7530,11 @@ def get_logs(): 'message': 'Timeout while fetching logs' }), 500 except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', - 'message': 'An error occurred; see logs for details' + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e) }), 500 # Multi-Display Sync Endpoints @@ -7589,9 +7599,11 @@ def get_wifi_status(): } }) except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', - 'message': 'An error occurred; see logs for details' + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e) }), 500 @api_v3.route('/wifi/scan', methods=['GET']) @@ -7764,9 +7776,11 @@ def enable_ap_mode(): 'message': message }), 400 except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', - 'message': 'An error occurred; see logs for details' + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e) }), 500 @api_v3.route('/wifi/ap/disable', methods=['POST']) @@ -7789,9 +7803,11 @@ def disable_ap_mode(): 'message': message }), 400 except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', - 'message': 'An error occurred; see logs for details' + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e) }), 500 @api_v3.route('/wifi/ap/auto-enable', methods=['GET']) @@ -7810,9 +7826,11 @@ def get_auto_enable_ap_mode(): } }) except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', - 'message': 'An error occurred; see logs for details' + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e) }), 500 @api_v3.route('/wifi/ap/auto-enable', methods=['POST']) @@ -7842,9 +7860,11 @@ def set_auto_enable_ap_mode(): } }) except Exception as e: + logger.error("%s failed", request.path, exc_info=True) return jsonify({ 'status': 'error', - 'message': 'An error occurred; see logs for details' + 'message': 'An error occurred; see logs for details', + 'details': describe_exception(e) }), 500 @api_v3.route('/wifi/radio', methods=['GET'])