Conversation
faaabe1 to
c171e6a
Compare
| if settings.MITOL_APIGATEWAY_DISABLE_MIDDLEWARE: | ||
| return self.get_response(request) | ||
| return |
There was a problem hiding this comment.
Bug: The process_response method sets a next cookie even when MITOL_APIGATEWAY_DISABLE_MIDDLEWARE is True because it lacks a check for this setting.
Severity: MEDIUM
Suggested Fix
Add a check at the beginning of the process_response method to see if settings.MITOL_APIGATEWAY_DISABLE_MIDDLEWARE is True. If it is, the method should return the response object immediately without any modifications.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/apigateway/mitol/apigateway/middleware.py#L28-L29
Potential issue: When `MITOL_APIGATEWAY_DISABLE_MIDDLEWARE` is set to `True`, the
`process_request` method correctly returns `None`. However, the newly introduced
`process_response` method is still invoked by Django's `MiddlewareMixin`. This new
method sets a `next` cookie on the response if a `next` query parameter is present in
the request, but it does not check if the middleware is disabled. Consequently, even
with the middleware disabled, any request containing a `?next=` parameter will have a
`next` cookie set, which is unintended behavior.
Did we get this right? 👍 / 👎 to inform future reviews.
05e03ab to
f11f279
Compare
rhysyngsun
left a comment
There was a problem hiding this comment.
Copy/pasting this comment from slack:
Prior to 5.2 they were subclassing MiddlewareMixin (link) which gives the process_request and process_response methods, but in 5.2 they rewrote it to not subclass that but still kept the process_request but instead call it directly themselves from RemoteUserMiddleware here. But process_response appears not to be called. So I think what you want to do it refactor that so that instead of overriding process_request we override call instead.
f6b67f6 to
117cece
Compare
for more information, see https://pre-commit.ci
What are the relevant tickets?
Fix https://github.com/mitodl/hq/issues/10366
Description (What does it do?)
update the default django version to 5.2.x
How can this be tested?
All tests should pass