Skip to content

Conversation

@bitterpanda63
Copy link
Member

@bitterpanda63 bitterpanda63 commented Dec 30, 2025

Summary by Aikido

Security Issues: 0 πŸ” Quality Issues: 4 Resolved Issues: 0

πŸš€ New Features

  • Introduced InternalASGIMiddleware class and integrated it into Quart.

⚑ Enhancements

  • Added send_interceptor and run_with_intercepts to capture response statuses.

πŸ”§ Refactors

  • Moved response-writing and pre-response handling out of quart into middleware.
  • Updated Quart patching to support legacy and non-legacy ASGI callables.

More info

context = Context(req=scope, source=self.source)

process_cache = get_cache()
if process_cache and process_cache.is_bypassed_ip(context.remote_address):

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

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):

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__):

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants