fix: Delete JWTs and other cookies when SafeSessions deletes session cookie (#29857)

This is more correct and may reduce the likelihood of perpetuating a bad
mixed-auth state.

In general, we should probably be modifying session and JWT cookies in
sync at all times, never individually. This specific code probably won't
make anything worse, but a clean reset might improve user experience in
the rare cases where someone somehow gets their browser into a weird
state.

- Switch from `response.set_cookie` with past expiry to just using the
  `response.delete_cookie` method.
- Docstring improvements.

ref: ARCHBOM-2030 (internal)
This commit is contained in:
Tim McCormack
2022-02-03 15:00:23 +00:00
committed by GitHub
parent 52db919b6c
commit 7c7792f92a
3 changed files with 14 additions and 12 deletions

View File

@@ -95,6 +95,7 @@ from edx_django_utils.monitoring import set_custom_attribute
from edx_toggles.toggles import SettingToggle
from common.djangoapps.util.log_sensitive import encrypt_for_log
from openedx.core.djangoapps.user_authn.cookies import delete_logged_in_cookies
from openedx.core.lib.mobile_utils import is_request_from_mobile_app
# .. toggle_name: LOG_REQUEST_USER_CHANGES
@@ -822,17 +823,16 @@ def _is_cookie_present(response):
def _delete_cookie(request, response):
"""
Delete the cookie by setting the expiration to a date in the past,
while maintaining the domain, secure, and httponly settings.
Delete session cookie, as well as related login cookies.
"""
response.set_cookie(
response.delete_cookie(
settings.SESSION_COOKIE_NAME,
max_age=0,
expires='Thu, 01-Jan-1970 00:00:00 GMT',
path='/',
domain=settings.SESSION_COOKIE_DOMAIN,
secure=settings.SESSION_COOKIE_SECURE or None,
httponly=settings.SESSION_COOKIE_HTTPONLY or None,
)
# Keep JWT cookies and others in sync with session cookie
# (meaning, in this case, delete them too).
delete_logged_in_cookies(response)
# Note, there is no request.user attribute at this point.
if hasattr(request, 'session') and hasattr(request.session, 'session_key'):

View File

@@ -191,9 +191,10 @@ class TestSafeSessionProcessResponse(TestSafeSessionsLogMixin, TestCase):
See assert_response for information on the other
parameters.
"""
with patch('django.http.HttpResponse.set_cookie') as mock_delete_cookie:
with patch('django.http.HttpResponse.delete_cookie') as mock_delete_cookie:
self.assert_response(set_request_user=set_request_user, set_session_cookie=set_session_cookie)
assert mock_delete_cookie.called == expect_delete_called
assert {'sessionid', 'edx-jwt-cookie-header-payload'} \
<= {call.args[0] for call in mock_delete_cookie.call_args_list}
def test_success(self):
with self.assert_not_logged():
@@ -321,9 +322,10 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
assert self.request.need_to_delete_cookie
self.cookies_from_request_to_response()
with patch('django.http.HttpResponse.set_cookie') as mock_delete_cookie:
with patch('django.http.HttpResponse.delete_cookie') as mock_delete_cookie:
SafeSessionMiddleware().process_response(self.request, self.client.response)
assert mock_delete_cookie.called
assert {'sessionid', 'edx-jwt-cookie-header-payload'} \
<= {call.args[0] for call in mock_delete_cookie.call_args_list}
def test_error(self):
self.request.META['HTTP_ACCEPT'] = 'text/html'

View File

@@ -68,7 +68,7 @@ def are_logged_in_cookies_set(request):
def delete_logged_in_cookies(response):
"""
Delete cookies indicating that the user is logged in.
Delete cookies indicating that the user is logged in (except for session cookie.)
Arguments:
response (HttpResponse): The response sent to the client.
Returns: