From 7660c55640e31c8ad36284df0d2926ec048686aa 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 | 16 ++-------------- 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(+), 39 deletions(-) diff --git a/src/authentic2/middleware.py b/src/authentic2/middleware.py index 296b05c7..4b207356 100644 --- a/src/authentic2/middleware.py +++ b/src/authentic2/middleware.py @@ -160,21 +160,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 d886d5c7..6f633d63 100644 --- a/src/authentic2/views.py +++ b/src/authentic2/views.py @@ -1349,3 +1349,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 9243bfc9..4501c33a 100644 --- a/tests/test_all.py +++ b/tests/test_all.py @@ -494,7 +494,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) @@ -608,7 +608,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) @@ -714,7 +714,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 de785b25..dabd65b3 100644 --- a/tests/test_registration.py +++ b/tests/test_registration.py @@ -74,13 +74,13 @@ def test_registration(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 @@ -132,7 +132,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 @@ -570,7 +570,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 @@ -616,7 +616,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