diff --git a/src/tollgate/api/errors.py b/src/tollgate/api/errors.py index 804f145..08e3e97 100644 --- a/src/tollgate/api/errors.py +++ b/src/tollgate/api/errors.py @@ -9,6 +9,7 @@ from __future__ import annotations +import logging from typing import Final from fastapi import FastAPI, Request @@ -32,18 +33,22 @@ UnknownModel, ) +logger = logging.getLogger(__name__) + #: Datastore connectivity/timeout failures that mean "no enforcement decision was made". #: They are translated to :class:`EnforcementUnavailable` so a client SDK sees the documented -#: fail-closed 503 envelope (ADR 0031) instead of an off-contract 500. On the request path the -#: only I/O is the datastore, so a raw ``OSError`` (connection refused/reset, host unreachable, -#: DNS failure, and the ``ETIMEDOUT`` builtin ``TimeoutError``, which subclasses ``OSError``) -#: is a connect-time outage. ``OperationalError``/``InterfaceError`` cover failures on an -#: established connection (server-side statement timeout, dropped socket); the SQLAlchemy -#: ``TimeoutError`` covers pool-checkout exhaustion. Statement/constraint errors -#: (``IntegrityError``, ``ProgrammingError``, ...) are deliberately excluded: those are bugs, -#: not outages, and must keep failing loudly as 500s. +#: fail-closed 503 envelope (ADR 0031) instead of an off-contract 500. A connect-time outage +#: surfaces as ``ConnectionError`` (refused/reset/aborted) or the builtin ``TimeoutError`` +#: (``ETIMEDOUT``); ``OperationalError``/``InterfaceError`` cover failures on an established or +#: attempted connection through the driver (server-side statement timeout, dropped socket, and the +#: connect/DNS failures SQLAlchemy wraps); the SQLAlchemy ``TimeoutError`` covers pool-checkout +#: exhaustion. A *bare* ``OSError`` is deliberately excluded (#102): it is too broad — an incidental +#: filesystem/socket error or a plain bug would otherwise be mislabeled a retryable datastore +#: outage and invite fail-closed-with-grace, masking a defect that must surface loudly as a 500. +#: Statement/constraint errors (``IntegrityError``, ``ProgrammingError``) are likewise excluded. _UNAVAILABLE_ERRORS: Final = ( - OSError, + ConnectionError, + TimeoutError, OperationalError, InterfaceError, SQLAlchemyTimeoutError, @@ -74,18 +79,33 @@ } +def _envelope( + status: int, code: str, message: str, *, headers: dict[str, str] | None = None +) -> JSONResponse: + body = ErrorEnvelope(error=ErrorBody(code=code, message=message)) + return JSONResponse(status_code=status, content=body.model_dump(), headers=headers) + + def _error_response(exc: TollgateError) -> JSONResponse: - status, code, default_message = _MAPPING.get(type(exc), _FALLBACK) - envelope = ErrorEnvelope(error=ErrorBody(code=code, message=str(exc) or default_message)) + mapped = _MAPPING.get(type(exc)) + if mapped is None: + # An unmapped TollgateError is an internal invariant breach, not a client-facing outcome; + # surface the generic 500 message so an internal invariant string (e.g. "idempotency replay + # is missing its stored response") never leaks into the response body (#101). + status, code, message = _FALLBACK + else: + status, code, default_message = mapped + message = str(exc) or default_message headers = {"WWW-Authenticate": "Bearer"} if status == 401 else None - return JSONResponse(status_code=status, content=envelope.model_dump(), headers=headers) + return _envelope(status, code, message, headers=headers) def register_error_handlers(app: FastAPI) -> None: - """Install the domain-error and fail-closed datastore handlers on ``app``. + """Install the domain-error, fail-closed datastore, and catch-all handlers on ``app``. The ``TollgateError`` handler covers every domain subtype; the connectivity handler - translates a datastore outage into the 503 ``EnforcementUnavailable`` envelope (#62). + translates a datastore outage into the 503 ``EnforcementUnavailable`` envelope (#62); the + catch-all keeps every unexpected exception on the ADR 0031 envelope (#101). """ async def handle(_: Request, exc: Exception) -> JSONResponse: @@ -98,6 +118,15 @@ async def handle_unavailable(_: Request, exc: Exception) -> JSONResponse: # the driver's message (it can carry the DSN); use the envelope's default text. return _error_response(EnforcementUnavailable()) + async def handle_unexpected(_: Request, exc: Exception) -> JSONResponse: + # A genuine bug (any exception not otherwise mapped) still returns the ADR 0031 envelope + # rather than Starlette's plain-text "Internal Server Error"; the generic message never + # leaks internals, and the traceback is logged so the defect is not lost (#101). + logger.exception("unhandled exception on the request path") + status, code, message = _FALLBACK + return _envelope(status, code, message) + app.add_exception_handler(TollgateError, handle) for exc_type in _UNAVAILABLE_ERRORS: app.add_exception_handler(exc_type, handle_unavailable) + app.add_exception_handler(Exception, handle_unexpected) diff --git a/tests/unit/test_api_errors.py b/tests/unit/test_api_errors.py index e923de3..31b236a 100644 --- a/tests/unit/test_api_errors.py +++ b/tests/unit/test_api_errors.py @@ -105,3 +105,33 @@ class _Novel(TollgateError): response = _client_raising(_Novel()).get("/boom") assert response.status_code == 500 assert response.json()["error"]["code"] == "internal_error" + + +def test_unmapped_error_does_not_echo_its_internal_message() -> None: + # An unmapped TollgateError is an internal invariant breach; the 500 body must carry the + # generic message, never the raw internal string (#101). + leak = "idempotency replay is missing its stored response" + response = _client_raising(TollgateError(leak)).get("/boom") + assert response.status_code == 500 + body = response.json() + assert body["error"]["code"] == "internal_error" + assert body["error"]["message"] == "internal error" + assert leak not in body["error"]["message"] + + +def test_unexpected_exception_is_enveloped_not_plain_text() -> None: + # A non-Tollgate, non-datastore exception (a genuine bug) must still return the ADR 0031 + # envelope, not Starlette's plain-text "Internal Server Error" (#101). + response = _client_raising(ValueError("some internal detail")).get("/boom") + assert response.status_code == 500 + body = response.json() + assert body["error"]["code"] == "internal_error" + assert body["error"]["message"] == "internal error" + + +def test_incidental_oserror_is_not_a_datastore_outage() -> None: + # A bare OSError that is not a connection/timeout is not a datastore outage: it must surface as + # a 500 (a bug to fix), not a retryable 503 that tells the SDK to fail-closed-with-grace (#102). + response = _client_raising(OSError("some incidental os error")).get("/boom") + assert response.status_code == 500 + assert response.json()["error"]["code"] == "internal_error"