fix(security): SecurityMiddleware no longer retries self.app() on exception - #22
Merged
Merged
Conversation
…eption 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. Every real subclass calls self.app(request) itself partway through, and since #16 that call essentially never raises anymore (the compiled middleware terminal already converts routing failures and handler exceptions to real Responses). So what this actually caught was a bug in a middleware's own post-processing after a successful downstream call — and silently re-ran the whole chain, handler included. Worse, the client sees the retry's response as a plain success, no sign anything ran twice.
Removed the catch. Added a regression test — confirmed it fails against the old code (handler 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.