skip hijack if request or request.session is None #191
Conversation
|
@khpeterson With your fork I was still getting "AttributeError: 'WSGIRequest' object has no attribute 'session'" so instead of using |
|
@randlet Thank you for taking a look, you're right, using getattr() to check for session is better. |
|
Any idea when this will be merged? |
|
Can this be merged and released? It will fix errors I'm getting. |
|
Also a unit test to prevent this regressing would be good |
|
@philippeowagner It would be great if this could be merged and released, thanks! |
There was a problem hiding this comment.
Hi @khpeterson,
Thank you for your contribution. It seems there are waiting for this problem to be fixed.
Still, I would prefer to take a little more time, to make sure we get this right. Adding a test, would also go a long way here.
Best,
Joe
| @@ -30,7 +30,7 @@ def _render_hijack_notification(request, template_name=None): | |||
| else: | |||
| template_name = 'hijack/notifications.html' | |||
| ans = '' | |||
| if request is not None and all([ | |||
| if request is not None and getattr(request, "session", None) is not None and all([ | |||
There was a problem hiding this comment.
Quick question, in what case is the session actually None? If you are trying to avoid an AttributeError a simple hasattr should suffice.
| if request is not None and getattr(request, "session", None) is not None and all([ | |
| if request is not None and hasattr(request, "session") and all([ |
| def is_hijacked(request): | ||
| return request.session.get('is_hijacked_user', False) | ||
| return (request is not None and | ||
| getattr(request, "session", None) is not None and | ||
| request.session.get('is_hijacked_user', False)) |
There was a problem hiding this comment.
Why would there be not request, or rather, why would it be None? I feel raising an error in case someone passes something that isn't a request to the filter is a good thing, to avoid potential security vulnerabilities.
There was a problem hiding this comment.
@codingjoe - I went back through the old emails from when we first bumped into this. We ended up with a cascading chain of 500s triggered by an exception in one of the views. The original exception would normally render a "something went wrong template" but because of this bug in django-hijack, attempting to render the something went wrong template also caused an exception, and on it went. The definitive work around at the time was to add this snippet to the template:
{% if request and request.session and request|is_hijacked %}
We needed both the checks of request and request.session to prevent the chain of errors and that's why I added it to both places. With the workaround in place we moved on to other things and didn't dig deeper. And I thought it would be helpful to do the pull-request since it seems so straightforward to check/fix. Sorry I can't be of more help.
|
@codingjoe I am happy to rework this with your suggestions + a test. Will wait for #221 to be merged. |
|
Apologies for the slow response... I need to refresh my memory on how we ended up with request.session == None. For sure it happened and I thought this change would be a safe workaround, but of course, a test is needed to complete the picture.
…Sent from my iPhone
On Feb 8, 2021, at 8:45 PM, LincolnPuzey <notifications@github.com> wrote:
@codingjoe<https://github.com/codingjoe> I am happy to rework this with your suggestions + a test.
Will wait for #221<#221> to be merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#191 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AI4S5NO2GZVE5C5COYFUGSDS6C4XLANCNFSM4HWKP2MQ>.
|
|
@LincolnPuzey great, I will try to get #221 merged asap. You are welcome to review it yourself. You are also more than welcome to work on this patch. I would keep it based on master for now. If you diff isn't too large, I shouldn't be a big problem to rebase it and even if, I can help you with that. |
|
I just took a look and MIDDLEWARE = [
....
'django.middleware.common.CommonMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
...
]and get a request with an invalid host header (i.e. when the request host is not present in the File "...lib/python3.6/site-packages/django/core/handlers/exception.py", line 34, in inner [60/9818]
response = get_response(request)
File "...lib/python3.6/site-packages/django/utils/deprecation.py", line 90, in __call__
response = self.process_request(request)
File "...lib/python3.6/site-packages/django/middleware/common.py", line 47, in process_request
host = request.get_host()
File "...lib/python3.6/site-packages/django/http/request.py", line 106, in get_host
raise DisallowedHost(msg)
django.core.exceptions.DisallowedHost: Invalid HTTP_HOST header: '127.0.0.1:8000'. You may need to add '127.0.0.1' to ALLOWED_HOSTS.
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "...lib/python3.6/site-packages/django/core/handlers/exception.py", line 108, in get_exception_response
response = callback(request, **{**param_dict, 'exception': exception})
File ".../views.py", line 185, in handle_400
return handle_error(request, 400, _("Bad Request"), _("Please check your ALLOWED_HOST setting."), exception)
File ".../views.py", line 181, in handle_error
return render(request, 'site_error.html', context, status=code)
File "...lib/python3.6/site-packages/django/shortcuts.py", line 36, in render
content = loader.render_to_string(template_name, context, request, using=using)
File "...lib/python3.6/site-packages/django/template/loader.py", line 62, in render_to_string
return template.render(context, request)
File "...lib/python3.6/site-packages/django/template/backends/django.py", line 61, in render
return self.template.render(context)
File "...lib/python3.6/site-packages/django/template/base.py", line 171, in render
return self._render(context)
<snip many lines>
return self.render(context)
File "...lib/python3.6/site-packages/django/template/library.py", line 192, in render
output = self.func(*resolved_args, **resolved_kwargs)
File "...lib/python3.6/site-packages/hijack/templatetags/hijack_tags.py", line 23, in hijack_notification
return _render_hijack_notification(request)
File "...lib/python3.6/site-packages/hijack/templatetags/hijack_tags.py", line 35, in _render_hijack_notification
request.session.get('is_hijacked_user', False),
AttributeError: 'WSGIRequest' object has no attribute 'session'Reversing the order of the CommonMiddleware and SessionMiddleware resolves the error in this particular case, but it can occur for other middlewares too, and I don't think you can always count on SessionMiddleware coming first in the middleware stack. I think this likely means that all middleware should account for the possibility that during an exception occurring farther up the middleware stack any given request may not have a session or user attached to it. |
|
I updated the pull request to the latest master and added a test (a bit contrived but it covers the code change).
I found it hard to create the right context in the django.test setup. When I bumped into this before the original exception was occurring in one of the views (timed in such a way that request.session == None) but the template using the hijack_notification tag and is_hijacked filter was still being rendered. It’s been a long time and that app was fairly big (and deployed on AWS) so hard to narrow down the exact circumstances.
I also tried simply adjusting the middleware sequence the way @randlet suggested, but that didn’t produce the expected result using the test.django.Client.
Anyway, I hope this test is enough to move this one forward. The change seems pretty safe to me.
Kevin
|
|
@randlet to be exact, your example stacktrace has
this is different to
|
|
|
||
| def test_missing_session(self): | ||
| with SettingsOverride(settings, MIDDLEWARE=()): | ||
| response = self.client.get('/hello/') |
There was a problem hiding this comment.
@khpeterson You could also test these changes more directly by calling the is_hijacked and hijack_notification functions directly in your test. Use the django request factory to make the request objects to pass to them.
Yep you're absolutely right and I misspoke above. I only ever ran into request not having a session rather than a null session so I'm not sure when/if that occurred. |
|
Most likely your request is no longer relevant after the release of version 3.0 (currently in pre-release status). If it is not, please reopen your ticket. |
|
Got it, thank you for the clarification.
On Mar 20, 2021, at 10:22 AM, Alexandr Artemyev ***@***.******@***.***>> wrote:
Most likely your request is no longer relevant after the release of version 3.0 (currently in pre-release status). If it is not, please reopen your ticket.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#191 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AI4S5NOGTLHDCXJA4ULG6HDTETKUNANCNFSM4HWKP2MQ>.
|
Bumped into a case where request is None when trying to show a render a template error message after an uncaught exception elsewhere in the application, resulting in another exception in hijack_tags.py. The changes in this pull request handle this by checking for request==None and request.session==None before trying to get the session attributes.