From 6ac2f55130bd40515dce379d84890e1f24d63e1b Mon Sep 17 00:00:00 2001 From: magi Date: Sun, 6 Sep 2026 22:49:33 +0530 Subject: [PATCH] fix(security): SecurityMiddleware no longer retries self.app() on exception Fixes #17. __call__ caught any exception from _on_request and responded by calling self.app(request) a second time -- meant to keep a buggy security check from crashing the pipeline. But every real subclass calls self.app(request) itself partway through _on_request, and since #16, that call 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 what this actually caught was a bug in a middleware's OWN post-processing after a successful self.app(request) call -- e.g. mutating headers on the response it got back -- and silently re-ran the entire downstream chain, handler included, for one incoming request. Worse, the retry's response is what the client sees, so a real bug there reads as a plain success with no indication anything ran twice. Removed the catch. Whatever's left after #16 (a bug in this middleware's own code) should surface as a real error via the existing exception handling, not get silently retried. Added a regression test: confirmed it fails against the old code (a handler with a side effect runs twice, client sees 200) and passes now (runs once, client sees 500). 262/262 tests, mypy clean, ruff clean -- verified in a fresh venv matching CI's install steps. --- tests/test_security_middleware_base.py | 52 ++++++++++++++++++++++++++ velocix/security/base.py | 23 +++++++++--- 2 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 tests/test_security_middleware_base.py 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.