mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-11 05:38:07 +00:00
fix(web): say what actually went wrong instead of "unknown"
Every failing endpoint returned "An error occurred; see logs for
details" and nothing else. That is survivable until the logs are the
thing you cannot reach: a device whose SD card was failing answered the
restart action, /system/status and /logs with that same sentence -- the
log viewer included, because journalctl could not be executed -- while
the exception underneath said
[Errno 5] Input/output error: 'systemctl'
which names the fault outright. The only endpoint that helped was
/health, and only because it happens to pass a subprocess's stderr
through. Diagnosis came down to guessing which endpoint leaked something.
Add describe_exception(), returning "TypeName: message" on one line, and
populate the `details` field that the response schema has always had and
nothing ever filled. The type alone carries information -- a bare
PermissionError says more than any generic sentence.
Exception text is not automatically safe to echo: a requests error
quotes the URL it failed on, and plugins that authenticate by query
string put their key there. Credential values are redacted while the
parameter name is kept, since knowing which credential was involved is
part of the diagnosis. Length is capped and newlines collapsed so a
parser's context cannot flood a JSON field.
Nine handlers in api_v3 bound the exception and never used it, so the
promised log entry was never written either -- "see logs for details"
was false, not merely unhelpful. Those now log with a traceback and
carry the detail. The other 60 already logged and are unchanged; they
can adopt the helper as they are touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This commit is contained in:
co-authored by
Claude Opus 5
parent
fc25a70d75
commit
8c1171444c
@@ -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<redacted>', 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,
|
||||
|
||||
@@ -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 "<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"))
|
||||
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 "<redacted>" 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"]
|
||||
+20
-3
@@ -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
|
||||
|
||||
@@ -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'])
|
||||
|
||||
Reference in New Issue
Block a user