-
Notifications
You must be signed in to change notification settings - Fork 11
Generalize ASGI Middleware used in Quart to a function #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
intercepts status codes
| context = Context(req=scope, source=self.source) | ||
|
|
||
| process_cache = get_cache() | ||
| if process_cache and process_cache.is_bypassed_ip(context.remote_address): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process_cache.is_bypassed_ip(...) short-circuits request handling and skips context setup; avoid silent bypasses or gate them behind explicit config/audit logging.
Details
β¨ AI Reasoning
βThe change introduces a bypass: when process_cache.is_bypassed_ip(...) is true the middleware returns early and skips setting a context or running any further request handling. This is an intentional short-circuit of request processing introduced in this PR and can silently disable security/validation logic for those IPs. It reduces visibility and may leave requests unobserved by later instrumentation.
π§ How do I fix it?
Remove debugging statements like console.log, debugger, dd(), or logic bypasses like || true. Keep legitimate logging for monitoring and error handling.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| await InternalASGIMiddleware(return_value, "quart")(scope, receive, send) | ||
|
|
||
| # Modify return_value | ||
| return_value = application_instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter 'return_value' is reassigned to application_instance. Avoid reassigning function parameters; use a new local variable (e.g., new_return_value) and return that instead.
Details
β¨ AI Reasoning
βAn after-advice wrapper function receives the original function's return value as a parameter and then assigns a new value to that parameter before returning it. Reassigning an input parameter can confuse readers about the original return value and make reasoning about the wrapper harder. This change introduced the reassignment where the wrapper replaced the incoming return value with a new callable wrapping the original behavior.
π§ How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
|
|
||
| @before | ||
| def _call(func, instance, args, kwargs): | ||
| async def _call_coroutine(func, instance, args, kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename _call_coroutine to a descriptive name (e.g., wrap_quart_call_coroutine) or add a docstring explaining it wraps Quart.call and delegates to InternalASGIMiddleware.
Details
β¨ AI Reasoning
βA newly introduced function named _call_coroutine is intended to wrap/replace Quart.call for coroutine-style ASGI apps, but the name doesn't describe its role (middleware wrapper, context creation, delegation). A clearer name or docstring would make intent obvious and aid maintenance.
π§ How do I fix it?
Use descriptive verb-noun function names, add docstrings explaining the function's purpose, or provide meaningful return type hints.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| """ | ||
| patch_function(m, "Quart.__call__", _call) | ||
|
|
||
| if inspect.iscoroutine(m.Quart.__call__): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using inspect.iscoroutine on Quart.call will not detect coroutine functions, making the coroutine path unreachable and always treating apps as legacy. Use inspect.iscoroutinefunction (or equivalent) for this check.
Details
β¨ AI Reasoning
βThe conditional that decides whether Quart.call is treated as a coroutine application uses an API that checks coroutine instances instead of coroutine functions. Since the target is a function, this branch will not be taken in normal operation, so the logic for non-legacy ASGI apps will never run and everything will be treated as legacy. This is a clear control-flow bug where the intended branch is unreachable.
π§ How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Summary by Aikido
π New Features
β‘ Enhancements
π§ Refactors
More info