From 751888ece3c970e208d9d77f21a35464c9122835 Mon Sep 17 00:00:00 2001 From: Jannis Leidel Date: Wed, 2 Mar 2011 12:47:44 +0000 Subject: [PATCH] Fixed #11223 -- Fixed logout view to use the 'next' GET parameter correctly as described in the docs, while only allowing redirection to the same host. git-svn-id: http://code.djangoproject.com/svn/django/trunk@15706 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/auth/tests/views.py | 55 ++++++++++++++++++++++ django/contrib/auth/views.py | 75 +++++++++++++++++------------- 2 files changed, 97 insertions(+), 33 deletions(-) diff --git a/django/contrib/auth/tests/views.py b/django/contrib/auth/tests/views.py index 530956ae45..baea1fc95f 100644 --- a/django/contrib/auth/tests/views.py +++ b/django/contrib/auth/tests/views.py @@ -333,6 +333,19 @@ class LogoutTest(AuthViewsTestCase): response = self.client.get('/logout/') self.assertTrue('site' in response.context) + def test_logout_with_overridden_redirect_url(self): + # Bug 11223 + self.login() + response = self.client.get('/logout/next_page/') + self.assertEqual(response.status_code, 302) + self.assert_(response['Location'].endswith('/somewhere/')) + + response = self.client.get('/logout/next_page/?next=/login/') + self.assertEqual(response.status_code, 302) + self.assert_(response['Location'].endswith('/login/')) + + self.confirm_logged_out() + def test_logout_with_next_page_specified(self): "Logout with next_page option given redirects to specified resource" self.login() @@ -356,3 +369,45 @@ class LogoutTest(AuthViewsTestCase): self.assertEqual(response.status_code, 302) self.assert_(response['Location'].endswith('/somewhere/')) self.confirm_logged_out() + + def test_security_check(self, password='password'): + logout_url = reverse('django.contrib.auth.views.logout') + + # Those URLs should not pass the security check + for bad_url in ('http://example.com', + 'https://example.com', + 'ftp://exampel.com', + '//example.com' + ): + nasty_url = '%(url)s?%(next)s=%(bad_url)s' % { + 'url': logout_url, + 'next': REDIRECT_FIELD_NAME, + 'bad_url': urllib.quote(bad_url) + } + self.login() + response = self.client.get(nasty_url) + self.assertEquals(response.status_code, 302) + self.assertFalse(bad_url in response['Location'], + "%s should be blocked" % bad_url) + self.confirm_logged_out() + + # These URLs *should* still pass the security check + for good_url in ('/view/?param=http://example.com', + '/view/?param=https://example.com', + '/view?param=ftp://exampel.com', + 'view/?param=//example.com', + 'https:///', + '//testserver/', + '/url%20with%20spaces/', # see ticket #12534 + ): + safe_url = '%(url)s?%(next)s=%(good_url)s' % { + 'url': logout_url, + 'next': REDIRECT_FIELD_NAME, + 'good_url': urllib.quote(good_url) + } + self.login() + response = self.client.get(safe_url) + self.assertEquals(response.status_code, 302) + self.assertTrue(good_url in response['Location'], + "%s should be allowed" % good_url) + self.confirm_logged_out() diff --git a/django/contrib/auth/views.py b/django/contrib/auth/views.py index c34a4a377e..eba83a269e 100644 --- a/django/contrib/auth/views.py +++ b/django/contrib/auth/views.py @@ -1,23 +1,23 @@ -import re import urlparse + from django.conf import settings -from django.contrib.auth import REDIRECT_FIELD_NAME -# Avoid shadowing the login() view below. -from django.contrib.auth import login as auth_login -from django.contrib.auth.decorators import login_required -from django.contrib.auth.forms import AuthenticationForm -from django.contrib.auth.forms import PasswordResetForm, SetPasswordForm, PasswordChangeForm -from django.contrib.auth.tokens import default_token_generator -from django.views.decorators.csrf import csrf_protect from django.core.urlresolvers import reverse -from django.shortcuts import render_to_response, get_object_or_404 -from django.contrib.sites.models import get_current_site -from django.http import HttpResponseRedirect, Http404, QueryDict +from django.http import HttpResponseRedirect, QueryDict +from django.shortcuts import render_to_response from django.template import RequestContext from django.utils.http import base36_to_int from django.utils.translation import ugettext as _ -from django.contrib.auth.models import User from django.views.decorators.cache import never_cache +from django.views.decorators.csrf import csrf_protect + +# Avoid shadowing the login() and logout() views below. +from django.contrib.auth import REDIRECT_FIELD_NAME, login as auth_login, logout as auth_logout +from django.contrib.auth.decorators import login_required +from django.contrib.auth.forms import AuthenticationForm, PasswordResetForm, SetPasswordForm, PasswordChangeForm +from django.contrib.auth.models import User +from django.contrib.auth.tokens import default_token_generator +from django.contrib.sites.models import get_current_site + @csrf_protect @never_cache @@ -25,8 +25,9 @@ def login(request, template_name='registration/login.html', redirect_field_name=REDIRECT_FIELD_NAME, authentication_form=AuthenticationForm, current_app=None, extra_context=None): - """Displays the login form and handles the login action.""" - + """ + Displays the login form and handles the login action. + """ redirect_to = request.REQUEST.get(redirect_field_name, '') if request.method == "POST": @@ -71,36 +72,44 @@ def logout(request, next_page=None, template_name='registration/logged_out.html', redirect_field_name=REDIRECT_FIELD_NAME, current_app=None, extra_context=None): - "Logs out the user and displays 'You are logged out' message." - from django.contrib.auth import logout - logout(request) - if next_page is None: - redirect_to = request.REQUEST.get(redirect_field_name, '') - if redirect_to: + """ + Logs out the user and displays 'You are logged out' message. + """ + auth_logout(request) + redirect_to = request.REQUEST.get(redirect_field_name, '') + if redirect_to: + netloc = urlparse.urlparse(redirect_to)[1] + # Security check -- don't allow redirection to a different host. + if not (netloc and netloc != request.get_host()): return HttpResponseRedirect(redirect_to) - else: - current_site = get_current_site(request) - context = { - 'site': current_site, - 'site_name': current_site.name, - 'title': _('Logged out') - } - context.update(extra_context or {}) - return render_to_response(template_name, context, - context_instance=RequestContext(request, current_app=current_app)) + + if next_page is None: + current_site = get_current_site(request) + context = { + 'site': current_site, + 'site_name': current_site.name, + 'title': _('Logged out') + } + context.update(extra_context or {}) + return render_to_response(template_name, context, + context_instance=RequestContext(request, current_app=current_app)) else: # Redirect to this page until the session has been cleared. return HttpResponseRedirect(next_page or request.path) def logout_then_login(request, login_url=None, current_app=None, extra_context=None): - "Logs out the user if he is logged in. Then redirects to the log-in page." + """ + Logs out the user if he is logged in. Then redirects to the log-in page. + """ if not login_url: login_url = settings.LOGIN_URL return logout(request, login_url, current_app=current_app, extra_context=extra_context) def redirect_to_login(next, login_url=None, redirect_field_name=REDIRECT_FIELD_NAME): - "Redirects the user to the login page, passing the given 'next' page" + """ + Redirects the user to the login page, passing the given 'next' page + """ if not login_url: login_url = settings.LOGIN_URL