diff --git a/tests/test_security_middleware_base.py b/tests/test_security_middleware_base.py new file mode 100644 index 0000000..1db88d8 --- /dev/null +++ b/tests/test_security_middleware_base.py @@ -0,0 +1,52 @@ +"""Regression test for issue #17: SecurityMiddleware.__call__ used to catch +ANY exception from _on_request. Since PR #16, self.app(request) itself +essentially never raises anymore -- the compiled middleware terminal +already converts routing failures and handler-raised HTTPExceptions into +real Responses before they get back here. So the exception this catch +block actually swallowed was from a middleware's own code that runs AFTER +a successful self.app(request) call (e.g. CSRFMiddleware setting a cookie +on the response it got back). Catching that and calling self.app(request) +again meant the downstream handler -- and any side effect it has -- ran a +second time for one incoming request. +""" + +import asyncio + +from velocix import TestClient, Velocix +from velocix.security.base import SecurityMiddleware + + +class _BuggyPostProcessSecurity(SecurityMiddleware): + """Calls through to self.app like every real subclass, then does its + own post-processing on the response -- which has a bug.""" + + async def _on_request(self, request): + _response = await self.app(request) + raise RuntimeError("bug in this middleware's own post-processing") + + +def _run(coro): + return asyncio.run(coro) + + +def test_downstream_handler_not_invoked_twice_on_post_processing_bug(): + call_count = 0 + + app = Velocix() + + @app.get("/side-effect") + async def side_effect(): + nonlocal call_count + call_count += 1 + return {"ok": True} + + app.add_middleware(_BuggyPostProcessSecurity) + + async def scenario(): + async with TestClient(app) as client: + resp = await client.get("/side-effect") + assert resp.status_code == 500 + + _run(scenario()) + + assert call_count == 1 diff --git a/velocix/security/base.py b/velocix/security/base.py index d8908b5..613da87 100644 --- a/velocix/security/base.py +++ b/velocix/security/base.py @@ -209,11 +209,24 @@ def emit(self, event_type: str, detail: str = "", **kwargs: Any) -> SecurityEven return event async def __call__(self, request: Request) -> Response: - try: - return await self._on_request(request) - except Exception: - # Security middleware must never crash the request pipeline. - return await self.app(request) + # No try/except here on purpose: _on_request implementations call + # self.app(request) themselves partway through their own checks, and + # a bare `except Exception: return await self.app(request)` around + # that can't tell "our own check logic raised" from "the call to + # self.app(request) we already made raised" -- the latter meant a + # single incoming request invoked the downstream handler chain + # TWICE, risking real double side effects (e.g. a double DB write) + # for any handler that raises after doing one. + # + # This used to be needed because an exception here would otherwise + # propagate past every middleware, bypassing all of it. Velocix's + # compiled middleware terminal now catches every exception (routing + # failures and handler-raised HTTPExceptions alike) and converts it + # to a Response before it ever reaches here, so self.app(request) + # returning normally is already the common case; whatever's left + # (a bug in this middleware's own pre-dispatch logic) is exactly + # what should surface as a real error, not be silently retried. + return await self._on_request(request) async def _on_request(self, request: Request) -> Response: """Override this to implement security logic.