From 1c5a8fd3a4bc0b2c1f52dff8bd0af58db5859d49 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Fri, 2 Oct 2020 18:59:44 +0200 Subject: [PATCH 3/3] misc: prevent internal URL leak in browser history (#47302) --- src/authentic2/middleware.py | 17 ++--------------- src/authentic2/urls.py | 1 + src/authentic2/views.py | 28 ++++++++++++++++++++++++++++ tests/auth_fc/test_auth_fc.py | 33 +++++++++++++++++---------------- tests/settings.py | 1 + tests/test_all.py | 6 +++--- tests/test_registration.py | 12 ++++++------ 7 files changed, 58 insertions(+), 40 deletions(-) diff --git a/src/authentic2/middleware.py b/src/authentic2/middleware.py index 663f532b..b7392e0f 100644 --- a/src/authentic2/middleware.py +++ b/src/authentic2/middleware.py @@ -26,7 +26,6 @@ from django.utils.deprecation import MiddlewareMixin from django.utils.functional import SimpleLazyObject from django.utils.translation import ugettext as _ from django.utils.six.moves.urllib import parse as urlparse -from django.shortcuts import render from . import app_settings, utils, plugins from .utils.service import get_service_from_request, get_service_from_session @@ -162,21 +161,9 @@ class DisplayMessageBeforeRedirectMiddleware(MiddlewareMixin): return response # Check if there is some messages to show storage = messages.get_messages(request) - if not storage: + if not len(storage): return response - only_info = True - some_message = False - for message in storage: - some_message = True - if message.level != messages.INFO: - # If there are warnin or error messages, the intermediate page must not redirect - # automatically but should ask for an user confirmation - only_info = False - storage.used = False - if not some_message: - return response - return render(request, 'authentic2/display_message_and_continue.html', - {'url': url, 'only_info': only_info}) + return utils.redirect(request, 'continue', resolve=True, params={'next': url}) class ServiceAccessControlMiddleware(MiddlewareMixin): diff --git a/src/authentic2/urls.py b/src/authentic2/urls.py index 24568f49..34da9524 100644 --- a/src/authentic2/urls.py +++ b/src/authentic2/urls.py @@ -114,6 +114,7 @@ urlpatterns = [ url(r'^idp/', include('authentic2.idp.urls')), url(r'^manage/', include('authentic2.manager.urls')), url(r'^api/', include('authentic2.api_urls')), + url(r'^continue/$', views.display_message_and_continue, name='continue'), ] try: diff --git a/src/authentic2/views.py b/src/authentic2/views.py index 1827ce46..8e6ecb9d 100644 --- a/src/authentic2/views.py +++ b/src/authentic2/views.py @@ -1363,3 +1363,31 @@ def old_view_redirect(request, to, message=None): if message: messages.info(request, message) return utils.redirect(request, to=to) + + +class DisplayMessageAndContinueView(TemplateView): + template_name = 'authentic2/display_message_and_continue.html' + + def get(self, request, *args, **kwargs): + self.url = utils.select_next_url(self.request, reverse('account_management')) + self.only_info = True + + storage = messages.get_messages(request) + if not len(storage): + return utils.redirect(request, self.url, resolve=False) + + for message in storage: + if message.level != messages.INFO: + # If there are warning or error messages, the intermediate page must not redirect + # automatically but should ask for an user confirmation + self.only_info = False + storage.used = False + return super().get(request, *args, **kwargs) + + def get_context_data(self, **kwargs): + ctx = super().get_context_data(**kwargs) + ctx['url'] = self.url + ctx['only_info'] = self.only_info + return ctx + +display_message_and_continue = DisplayMessageAndContinueView.as_view() diff --git a/tests/auth_fc/test_auth_fc.py b/tests/auth_fc/test_auth_fc.py index 1dae183a..e90b652d 100644 --- a/tests/auth_fc/test_auth_fc.py +++ b/tests/auth_fc/test_auth_fc.py @@ -191,13 +191,13 @@ def test_login_simple(app, fc_settings, caplog, hooks, exp): response.form.set('new_password1', 'ikKL1234') response.form.set('new_password2', 'ikKL1234') response = response.form.submit(name='unlink') - assert 'The link with the FranceConnect account has been deleted' in response.text assert models.FcAccount.objects.count() == 0 - continue_url = response.pyquery('a#a2-continue').attr['href'] + continue_url = response.location state = urlparse.parse_qs(urlparse.urlparse(continue_url).query)['state'][0] assert app.session['fc_states'][state]['next'] == '/accounts/' - response = app.get(reverse('fc-logout') + '?state=' + state) - assert path(response['Location']) == '/accounts/' + response = app.get(reverse('fc-logout') + '?state=' + state).maybe_follow() + assert response.request.path == '/accounts/' + assert 'The link with the FranceConnect account has been deleted' in response.text def test_login_email_is_unique(app, fc_settings, caplog): @@ -435,13 +435,13 @@ def test_registration1(app, fc_settings, caplog, hooks): response.form.set('new_password1', 'ikKL1234') response.form.set('new_password2', 'ikKL1234') response = response.form.submit(name='unlink') - assert 'The link with the FranceConnect account has been deleted' in response.text assert models.FcAccount.objects.count() == 0 - continue_url = response.pyquery('a#a2-continue').attr['href'] + continue_url = response.location state = urlparse.parse_qs(urlparse.urlparse(continue_url).query)['state'][0] assert app.session['fc_states'][state]['next'] == '/accounts/' - response = app.get(reverse('fc-logout') + '?state=' + state) - assert path(response['Location']) == '/accounts/' + response = app.get(reverse('fc-logout') + '?state=' + state).maybe_follow() + assert response.request.path == '/accounts/' + assert 'The link with the FranceConnect account has been deleted' in response.text def test_registration2(app, fc_settings, caplog, hooks): @@ -529,13 +529,13 @@ def test_registration2(app, fc_settings, caplog, hooks): response.form.set('new_password1', 'ikKL1234') response.form.set('new_password2', 'ikKL1234') response = response.form.submit(name='unlink') - assert 'The link with the FranceConnect account has been deleted' in response.text assert models.FcAccount.objects.count() == 0 - continue_url = response.pyquery('a#a2-continue').attr['href'] + continue_url = response.location state = urlparse.parse_qs(urlparse.urlparse(continue_url).query)['state'][0] assert app.session['fc_states'][state]['next'] == '/accounts/' - response = app.get(reverse('fc-logout') + '?state=' + state) - assert path(response['Location']) == '/accounts/' + response = app.get(reverse('fc-logout') + '?state=' + state).maybe_follow() + assert response.request.path == '/accounts/' + assert 'The link with the FranceConnect account has been deleted' in response.text def test_can_change_password(app, fc_settings, caplog, hooks): @@ -653,12 +653,13 @@ def test_can_change_password(app, fc_settings, caplog, hooks): response.form.set('new_password1', 'ikKL1234') response.form.set('new_password2', 'ikKL1234') response = response.form.submit(name='unlink') - continue_url = response.pyquery('a#a2-continue').attr['href'] + assert models.FcAccount.objects.count() == 0 + continue_url = response.location state = urlparse.parse_qs(urlparse.urlparse(continue_url).query)['state'][0] assert app.session['fc_states'][state]['next'] == '/accounts/' - response = app.get(reverse('fc-logout') + '?state=' + state) - assert path(response['Location']) == '/accounts/' - response = response.follow() + response = app.get(reverse('fc-logout') + '?state=' + state).maybe_follow() + assert response.request.path == '/accounts/' + assert 'The link with the FranceConnect account has been deleted' in response.text assert len(response.pyquery('[href*="password/change"]')) > 0 diff --git a/tests/settings.py b/tests/settings.py index 8fb9d4f9..b3cd2fce 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -56,3 +56,4 @@ A2_MAX_EMAILS_PER_IP = None A2_MAX_EMAILS_FOR_ADDRESS = None A2_TOKEN_EXISTS_WARNING = False +A2_REDIRECT_WHITELIST = ['http://sp.org/'] diff --git a/tests/test_all.py b/tests/test_all.py index 98215100..c225e7be 100644 --- a/tests/test_all.py +++ b/tests/test_all.py @@ -497,7 +497,7 @@ class APITest(TestCase): # User side client = Client() activation_url = get_link_from_mail(mail.outbox[-1]) - response = client.get(activation_url) + response = client.get(activation_url, follow=True) self.assertEqual(response.status_code, status.HTTP_200_OK) assert utils.make_url(return_url, params={'token': token}) in force_text(response.content) self.assertEqual(User.objects.count(), user_count + 1) @@ -611,7 +611,7 @@ class APITest(TestCase): # User side - user click on email client = Client() activation_url = get_link_from_mail(mail.outbox[0]) - response = client.get(activation_url) + response = client.get(activation_url, follow=True) self.assertEqual(response.status_code, status.HTTP_200_OK) assert utils.make_url(return_url, params={'token': token}) in force_text(response.content) self.assertEqual(User.objects.count(), user_count + 1) @@ -717,7 +717,7 @@ class APITest(TestCase): # User side - user click on first email client = Client() activation_url = get_link_from_mail(activation_mail1) - response = client.get(activation_url) + response = client.get(activation_url, follow=True) self.assertEqual(response.status_code, status.HTTP_200_OK) assert utils.make_url(return_url, params={'token': token}) in force_text(response.content) self.assertEqual(User.objects.count(), user_count + 1) diff --git a/tests/test_registration.py b/tests/test_registration.py index 2963d678..13645ee2 100644 --- a/tests/test_registration.py +++ b/tests/test_registration.py @@ -75,13 +75,13 @@ def test_registration_success(app, db, settings, mailoutbox, external_redirect): # set valid password response.form.set('password1', 'T0==toto') response.form.set('password2', 'T0==toto') - response = response.form.submit() + response = response.form.submit().maybe_follow() if good_next_url: assert 'You have just created an account.' in response.text assert next_url in response.text + assert response.request.path == '/continue/' else: - assert urlparse(response['Location']).path == '/' - response = response.follow() + assert response.request.path == '/' assert 'You have just created an account.' in response.text assert User.objects.count() == 1 assert len(mailoutbox) == 2 @@ -134,7 +134,7 @@ def test_registration_realm(app, db, settings, mailoutbox): response.form.set('username', 'toto') response.form.set('password1', 'T0==toto') response.form.set('password2', 'T0==toto') - response = response.form.submit() + response = response.form.submit().follow() assert 'You have just created an account.' in response.text assert next_url in response.text assert len(mailoutbox) == 2 @@ -572,7 +572,7 @@ def test_registration_redirect(app, db, settings, mailoutbox, external_redirect) response = app.get(link) response.form.set('password1', 'T0==toto') response.form.set('password2', 'T0==toto') - response = response.form.submit() + response = response.form.submit().follow() assert 'You have just created an account.' in response.text assert new_next_url in response.text @@ -618,7 +618,7 @@ def test_registration_redirect_tuple(app, db, settings, mailoutbox, external_red response = app.get(link) response.form.set('password1', 'T0==toto') response.form.set('password2', 'T0==toto') - response = response.form.submit() + response = response.form.submit().follow() assert new_next_url in response.text -- 2.28.0