From f35b5c2f424d1db2203114a59a8211e2606d0ebe Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 06:27:24 +0000 Subject: [PATCH] fix(logging): stop a hostile metaclass costing the record (#1576) #1515 closed #1525's three residual holes and is on main. One path in the same function is still reachable, and it is the one its docstring asserts is safe: "The type name is a plain attribute lookup and is always safe." It is not. `__name__` on a class is looked up on its *metaclass*, so a metaclass defining `__name__` as a raising property defeats the `except` branch. That second raise happens outside any guard, so it propagates past the `_JSON_UNSERIALIZABLE_RECORD` tier and out of `_format_json` entirely, and `Handler.handleError` drops the record. The constant-record tier does not catch it. Measured on 5473bcc: 2 of 3 records reach the sink. `object.__getattribute__(type(exc), "__name__")` does not fix this -- it still routes through the metaclass descriptor: type(exc).__name__ -> RAISES object.__getattribute__(type(exc),"__name__") -> RAISES type.__dict__["__name__"].__get__(type(exc)) -> OK Binding the descriptor from `type.__dict__` bypasses an override and returns the ordinary name for ordinary classes, with a constant as the final floor. The docstring is corrected to state the guarantee the code provides. This is the same failure shape #1525 was filed about, one level down: the recovery step for a failure is itself able to fail. Verification on this head: 36 passed in tests/unit/test_logging_config_crlf.py (33 pre-existing, unchanged). Reverting only logging_config.py: 2 failed, 34 passed. The third new test, test_describe_exception_is_unchanged_for_ordinary_exceptions, passes on 5473bcc by design -- it guards the normal path against regression rather than pinning the fix, so it is excluded from the non-vacuity claim. ruff clean; mypy reports the same 17 pre-existing errors on both heads. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Msg6kqkhiuW1ZiDv66sr4N --- .../backend/config/logging_config.py | 21 ++++++- tests/unit/test_logging_config_crlf.py | 55 +++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/youtube_extension/backend/config/logging_config.py b/src/youtube_extension/backend/config/logging_config.py index 6a033e419..959f31bc6 100644 --- a/src/youtube_extension/backend/config/logging_config.py +++ b/src/youtube_extension/backend/config/logging_config.py @@ -145,13 +145,28 @@ def _describe_exception(exc: BaseException) -> str: Used only by the serialization fallback, which exists precisely because a hostile ``__str__`` can raise. The exception caught there may *be* one raised from a call site's own ``__str__``, so interpolating ``exc`` can - raise a second time — inside the handler for the first. The type name is a - plain attribute lookup and is always safe. + raise a second time — inside the handler for the first. + + ``type(exc).__name__`` is the natural retreat from that, and is *not* + sufficient on its own: ``__name__`` on a class is looked up on its + **metaclass**, so a metaclass defining ``__name__`` as a raising property + defeats it too. That second raise would leave this function entirely, + past the ``_JSON_UNSERIALIZABLE_RECORD`` tier, and cost the record (#1576). + + ``object.__getattribute__(type(exc), "__name__")`` does not help — it still + routes through the metaclass descriptor. Fetching the descriptor from + ``type.__dict__`` and binding it directly is what bypasses an override, + and it returns the ordinary name for ordinary classes. """ try: return f"{type(exc).__name__}: {exc}" + except Exception: # noqa: BLE001 - fall through to a narrower attempt + pass + try: + name: str = type.__dict__["__name__"].__get__(type(exc)) + return name except Exception: # noqa: BLE001 - the fallback must not need a fallback - return type(exc).__name__ + return "UnrenderableException" class StructuredFormatter(logging.Formatter): diff --git a/tests/unit/test_logging_config_crlf.py b/tests/unit/test_logging_config_crlf.py index 12f7611bb..bd180a2b7 100644 --- a/tests/unit/test_logging_config_crlf.py +++ b/tests/unit/test_logging_config_crlf.py @@ -522,6 +522,61 @@ def test_describe_exception_survives_an_exception_that_cannot_be_stringified(): assert _describe_exception(_NastyError()) == "_NastyError" +# --------------------------------------------------------------------------- +# #1576: the retreat from an unstringifiable exception is itself defeatable. +# +# `_NastyError` above has an ordinary metaclass, so it only exercises one of +# the two ways naming an exception can raise. `__name__` on a class resolves +# through its *metaclass*, so a metaclass can make the `except` branch raise +# too — outside any guard, so it escapes even the constant-record tier. +# +# The tests below fail on 5473bcc. +# --------------------------------------------------------------------------- + + +class _HostileMeta(type): + """Makes `type(exc).__name__` raise — the usual retreat from a bad `__str__`.""" + + @property + def __name__(cls): # noqa: ANN204 - the override is the point + raise RuntimeError("__name__ exploded") + + +class _NamelessError(Exception, metaclass=_HostileMeta): + """Neither `str(exc)` nor `type(exc).__name__` can be rendered.""" + + def __str__(self) -> str: + raise RuntimeError("str() exploded too") + + +class _RaisesNameless: + def __str__(self) -> str: + raise _NamelessError() + + +def test_describe_exception_survives_a_hostile_metaclass(): + # `object.__getattribute__(type(exc), "__name__")` also raises here; only + # binding the descriptor from `type.__dict__` bypasses the override. + assert _describe_exception(_NamelessError()) == "_NamelessError" + + +def test_hostile_metaclass_exception_does_not_cost_the_record(): + records = _emit_three("json-hostile-metaclass", _RaisesNameless()) + + assert len(records) == 3 + poisoned = records[1] + assert poisoned["level"] == "INFO" + assert poisoned["message"] == "poisoned" + assert "correlation_id" not in poisoned + assert poisoned["serialization_error"] == "_NamelessError" + + +def test_describe_exception_is_unchanged_for_ordinary_exceptions(): + # The added tier must not alter the normal path, which the pre-existing + # fallback tests assert exact strings against. + assert _describe_exception(ValueError("msg")) == "ValueError: msg" + + @pytest.fixture def _restore_root_logging(): """`setup_logging` calls dictConfig, which mutates global logging state."""