LEDMatrix/test/test_web_error_detail.py

"""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"),
        # requests quotes the URL it failed on, and both of these forms turn
        # up in real client exceptions.
        ("401 for https://user:hunter2@example.com/api", "hunter2"),
        ("headers: {'Authorization': 'Bearer eyJ.SECRET.sig'}", "eyJ.SECRET.sig"),
        ("Authorization: Basic dXNlcjpwYXNzd29yZA==", "dXNlcjpwYXNzd29yZA=="),
        ("Proxy-Authorization: Bearer ptok999", "ptok999"),
        # Any scheme, not a fixed list -- a list silently leaks whatever it
        # does not name, and plugin APIs invent their own.
        ("Authorization: ApiKey SECRET123", "SECRET123"),
        ("Authorization: Negotiate YIIZnegotiateblob", "YIIZnegotiateblob"),
        ("Authorization: NTLM TlRMTVNTUAAB", "TlRMTVNTUAAB"),
        ("authorization: barecredential", "barecredential"),
    ])
    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_unknown_schemes_keep_their_name(self):
        for scheme in ("ApiKey", "Negotiate", "NTLM", "AWS4-HMAC-SHA256"):
            detail = describe_exception(
                RuntimeError("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"))

    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):
        """Every generic-message handler must log a traceback and return detail.

        Nine of them bound `e` and never used it, so the promised log entry was
        never written either. Checking merely that *something* was logged is
        too weak -- a `logger.info("failed")` would satisfy it while throwing
        the exception away just as completely, so this asserts the two things
        that actually make the failure diagnosable: an error-level record with
        the traceback, and the sanitized detail in the response.
        """
        import ast

        # api_v3 is a package; the routes are spread across its modules.
        import pathlib
        src = "\n".join(
            p.read_text() for p in
            sorted(pathlib.Path("web_interface/blueprints/api_v3").glob("*.py")))
        tree = ast.parse(src)

        # This used to match one exact message string, so a handler that wrote
        # its own wording was never checked. All thirteen Starlark routes did
        # -- "Failed to browse repository" and friends -- and every one of them
        # answered a 500 with no detail at all, which is how the app store
        # spent three releases failing for reasons nobody could read. The rule
        # is now the shape that matters: if it returns 5xx, it says why.
        PRE_EXISTING = {
            # Not part of this change. This set may shrink, never grow.
            'backup_delete', 'backup_export', 'backup_list', 'backup_preview',
            'backup_restore', 'backup_validate', 'checkout_branch',
            'execute_system_action', 'get_git_branches', 'get_git_info',
            'get_hardware_status', 'get_logs', 'get_system_status',
            'get_system_version', 'scan_wifi_networks',
        }

        def enclosing_function(handler):
            """Innermost function containing `handler`."""
            best = None
            for fn in [n for n in ast.walk(tree)
                       if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))]:
                if any(h is handler for h in ast.walk(fn)):
                    if best is None or fn.lineno > best.lineno:
                        best = fn
            return best.name if best else '<module>'

        def only_catches_importerror(handler):
            """An `except ImportError` arm and nothing else.

            A missing optional dependency is a configuration fact, not a
            crash: the module name is the whole diagnosis and it is already
            in the response, so a stack trace would be noise. Detail is still
            required -- only the traceback log is excused.
            """
            t = handler.type
            names = ([t] if isinstance(t, ast.Name)
                     else list(t.elts) if isinstance(t, ast.Tuple) else [])
            return bool(names) and all(
                isinstance(n, ast.Name) and n.id == 'ImportError' for n in names)

        def returns_5xx(handler):
            for r in [n for n in ast.walk(handler) if isinstance(n, ast.Return)]:
                v = r.value
                if isinstance(v, ast.Tuple) and len(v.elts) == 2:
                    code = v.elts[1]
                    if (isinstance(code, ast.Constant)
                            and isinstance(code.value, int)
                            and 500 <= code.value < 600):
                        return True
            return False

        def logs_a_traceback(handler):
            """An error/exception-level log call carrying exc_info."""
            for call in [n for n in ast.walk(handler) if isinstance(n, ast.Call)]:
                func = call.func
                if not isinstance(func, ast.Attribute):
                    continue
                if func.attr == "exception":       # implies exc_info
                    return True
                if func.attr not in ("error", "critical"):
                    continue
                if any(kw.arg == "exc_info" and getattr(kw.value, "value", False) is True
                       for kw in call.keywords):
                    return True
            return False

        def describes_this_exception(node, bound):
            """A describe_exception(<bound>) call anywhere under `node`."""
            for call in [n for n in ast.walk(node) if isinstance(n, ast.Call)]:
                if not (isinstance(call.func, ast.Name)
                        and call.func.id == "describe_exception"):
                    continue
                if bound is None:
                    return True     # bare `except:` cannot name it; accept
                if any(isinstance(a, ast.Name) and a.id == bound
                       for a in call.args):
                    return True
            return False

        def returns_the_detail(handler):
            """The detail must be inside what the handler actually returns.

            Looking anywhere in the handler is too weak: a handler could
            compute describe_exception(e), drop it on the floor, and return the
            generic message with no details field, while still passing. So the
            call has to appear within a `return` expression.
            """
            returns = [n for n in ast.walk(handler) if isinstance(n, ast.Return)]
            if not returns:
                return False
            return all(describes_this_exception(r, handler.name) for r in returns)

        offenders = []
        for h in [n for n in ast.walk(tree) if isinstance(n, ast.ExceptHandler)]:
            if not returns_5xx(h):
                continue
            if enclosing_function(h) in PRE_EXISTING:
                continue
            missing = []
            if not logs_a_traceback(h) and not only_catches_importerror(h):
                missing.append("error-level log with exc_info")
            if not returns_the_detail(h):
                missing.append("describe_exception(e) in the response")
            if missing:
                offenders.append((h.lineno, missing))

        assert not offenders, (
            "handlers returning the generic message without %s: %r"
            % ("both a traceback log and the detail", offenders))

    def test_no_api_v3_route_copies_the_blueprint_handler(self):
        """The generic catch-all lives once, on the blueprint.

        Fifty-three routes carried their own copy of it -- log with exc_info,
        return {status, "An error occurred; see logs for details",
        describe_exception(e)}, 500 -- until they were folded into
        `_api_v3_unhandled_exception`. A new copy changes nothing a caller
        sees, so nothing else would notice it; this does. A handler that says
        something *different* (its own message, extra keys, cleanup) is fine.
        """
        import ast
        import pathlib

        generic = "An error occurred; see logs for details"
        copies = []
        for path in sorted(pathlib.Path("web_interface/blueprints/api_v3").glob("*.py")):
            tree = ast.parse(path.read_text(encoding="utf-8"))
            for fn in [n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef)]:
                for h in ast.walk(fn):
                    if not (isinstance(h, ast.ExceptHandler)
                            and isinstance(h.type, ast.Name)
                            and h.type.id == "Exception"):
                        continue
                    for r in [n for n in h.body if isinstance(n, ast.Return)]:
                        v = r.value
                        if not (isinstance(v, ast.Tuple) and len(v.elts) == 2
                                and isinstance(v.elts[0], ast.Call)
                                and getattr(v.elts[0].func, "id", None) == "jsonify"
                                and v.elts[0].args
                                and isinstance(v.elts[0].args[0], ast.Dict)):
                            continue
                        d = v.elts[0].args[0]
                        keys = {k.value for k in d.keys if isinstance(k, ast.Constant)}
                        message = [val.value for k, val in zip(d.keys, d.values)
                                   if isinstance(k, ast.Constant) and k.value == "message"
                                   and isinstance(val, ast.Constant)]
                        if keys == {"status", "message", "details"} and message == [generic]:
                            copies.append((path.name, fn.name))

        # One is not a copy: execute_plugin_action's step-1 handler sits
        # inside the route's `except subprocess.TimeoutExpired` arm, which
        # would turn a plugin's own timeout into a 408 if this let it through.
        assert copies == [("plugins.py", "execute_plugin_action")], (
            "these handlers duplicate the api_v3 blueprint's errorhandler; "
            "delete them and let the exception propagate: %r" % copies)

    def test_client_errors_keep_their_own_status(self):
        """A 405 must not be reported as a server-side UNKNOWN_ERROR.

        Werkzeug's HTTPExceptions subclass Exception, so the catch-all saw them
        too: a GET on a POST-only route came back 500 "an error occurred",
        which tells the caller nothing and blames the wrong side. Found while
        probing a device whose POST-only config endpoints answered every GET
        with UNKNOWN_ERROR.
        """
        from flask import Flask, jsonify
        from werkzeug.exceptions import HTTPException

        app = Flask(__name__)

        @app.errorhandler(Exception)
        def handle(error):
            if isinstance(error, HTTPException):
                return jsonify({
                    "status": "error",
                    "error_code": (error.name or "HTTP_ERROR").upper().replace(" ", "_"),
                    "message": error.description,
                }), error.code or 500
            return jsonify({
                "status": "error",
                "error_code": "UNKNOWN_ERROR",
                "message": "An error occurred; see logs for details",
                "details": describe_exception(error),
            }), 500

        @app.route("/only-post", methods=["POST"])
        def only_post():
            return jsonify({"ok": True})

        @app.route("/boom")
        def boom():
            raise OSError(5, "Input/output error", "systemctl")

        client = app.test_client()

        resp = client.get("/only-post")
        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.
        resp = client.get("/boom")
        assert resp.status_code == 500
        assert "Input/output error" in resp.get_json()["details"]

    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"]