From 7c7792f92a1d6dafe45744a567dc50074a22c1cb Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Thu, 3 Feb 2022 15:00:23 +0000 Subject: [PATCH] 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) --- .../core/djangoapps/safe_sessions/middleware.py | 14 +++++++------- .../safe_sessions/tests/test_middleware.py | 10 ++++++---- openedx/core/djangoapps/user_authn/cookies.py | 2 +- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 9000977f27..467fcbb225 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -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'): diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py index 37260d3804..970818758c 100644 --- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py +++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py @@ -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' diff --git a/openedx/core/djangoapps/user_authn/cookies.py b/openedx/core/djangoapps/user_authn/cookies.py index 2c85fac76e..bfff9bcf1b 100644 --- a/openedx/core/djangoapps/user_authn/cookies.py +++ b/openedx/core/djangoapps/user_authn/cookies.py @@ -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: