diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index e2cc9be27a..bdd49ea34b 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -1,10 +1,10 @@ """ Views related to logout. """ +import urllib from urlparse import parse_qs, urlsplit, urlunsplit import edx_oauth2_provider from django.conf import settings from django.contrib.auth import logout -from django.urls import reverse_lazy from django.shortcuts import redirect from django.utils.http import urlencode from django.views.generic import TemplateView @@ -43,6 +43,18 @@ class LogoutView(TemplateView): """ target_url = self.request.GET.get('redirect_url') or self.request.GET.get('next') + # Some third party apps do not build URLs correctly and send next query param without URL-encoding, resulting + # all plus('+') signs interpreted as space(' ') in the process of URL-decoding + # for example if we hit on: + # >> http://example.com/logout?next=/courses/course-v1:ARTS+D1+2018_T/course/ + # we will receive in request.GET['next'] + # >> /courses/course-v1:ARTS D1 2018_T/course/ + # instead of + # >> /courses/course-v1:ARTS+D1+2018_T/course/ + # to handle this scenario we need to encode our URL using quote_plus and then unquote it again. + if target_url: + target_url = urllib.unquote(urllib.quote_plus(target_url)) + if target_url and is_safe_login_or_logout_redirect(self.request, target_url): return target_url else: diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py index f64a9c68b2..0a0fecf693 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py @@ -2,6 +2,7 @@ Tests for logout """ import unittest +import urllib import ddt from django.conf import settings @@ -61,9 +62,15 @@ class LogoutTests(TestCase): self.assertListEqual(self.client.session[AUTHORIZED_CLIENTS_SESSION_KEY], [oauth_client.client_id]) @ddt.data( - ('/courses', 'testserver'), - ('https://edx.org/courses', 'edx.org'), - ('https://test.edx.org/courses', 'edx.org'), + ('%2Fcourses', 'testserver'), + ('https%3A%2F%2Fedx.org%2Fcourses', 'edx.org'), + ('https%3A%2F%2Ftest.edx.org%2Fcourses', 'edx.org'), + ('/courses/course-v1:ARTS+D1+2018_T/course/', 'edx.org'), + ('%2Fcourses%2Fcourse-v1%3AARTS%2BD1%2B2018_T%2Fcourse%2F', 'edx.org'), + ('/courses/course-v1:ARTS+D1+2018_T/course/?q=computer+science', 'edx.org'), + ('%2Fcourses%2Fcourse-v1%3AARTS%2BD1%2B2018_T%2Fcourse%2F%3Fq%3Dcomputer+science', 'edx.org'), + ('/enterprise/c5dad9a7-741c-4841-868f-850aca3ff848/course/Microsoft+DAT206x/enroll/', 'edx.org'), + ('%2Fenterprise%2Fc5dad9a7-741c-4841-868f-850aca3ff848%2Fcourse%2FMicrosoft%2BDAT206x%2Fenroll%2F', 'edx.org'), ) @ddt.unpack @override_settings(LOGIN_REDIRECT_WHITELIST=['test.edx.org']) @@ -74,7 +81,7 @@ class LogoutTests(TestCase): ) response = self.client.get(url, HTTP_HOST=host) expected = { - 'target': redirect_url, + 'target': urllib.unquote(redirect_url), } self.assertDictContainsSubset(expected, response.context_data)