Commit a3e5bcc9 by James Henstridge

Fix the sanitise_redirect_url function to handle an empty string properly.

Revision 60 changed the behaviour so that empty URLs would be returned 
unchanged rather than rewriting to settings.LOGIN_REDIRECT_URL.

This meant that login without a "next" parameter would end up 
redirecting back to the login_complete() view.  Since the OpenID 
response had already been handled, this would look like a replay attack 
and the user would be presented with an error.
parent 85de7762
...@@ -381,6 +381,9 @@ class HelperFunctionsTest(TestCase): ...@@ -381,6 +381,9 @@ class HelperFunctionsTest(TestCase):
("http://example.net/foo/bar?baz=quux", False), ("http://example.net/foo/bar?baz=quux", False),
("/somewhere/local", True), ("/somewhere/local", True),
("/somewhere/local?url=http://fail.com/bar", True), ("/somewhere/local?url=http://fail.com/bar", True),
# An empty path, as seen when no "next" parameter is passed.
("", False),
("/path with spaces", False),
] ]
for url, returns_self in urls: for url, returns_self in urls:
sanitised = sanitise_redirect_url(url) sanitised = sanitise_redirect_url(url)
......
...@@ -64,7 +64,10 @@ def is_valid_next_url(next): ...@@ -64,7 +64,10 @@ def is_valid_next_url(next):
def sanitise_redirect_url(redirect_to): def sanitise_redirect_url(redirect_to):
"""Sanitise the redirection URL.""" """Sanitise the redirection URL."""
# Light security check -- make sure redirect_to isn't garbage. # Light security check -- make sure redirect_to isn't garbage.
if not redirect_to or '//' in redirect_to or ' ' in redirect_to: is_valid = True
if not redirect_to or ' ' in redirect_to:
is_valid = False
elif '//' in redirect_to:
# Allow the redirect URL to be external if it's a permitted domain # Allow the redirect URL to be external if it's a permitted domain
allowed_domains = getattr(settings, allowed_domains = getattr(settings,
"ALLOWED_EXTERNAL_OPENID_REDIRECT_DOMAINS", []) "ALLOWED_EXTERNAL_OPENID_REDIRECT_DOMAINS", [])
...@@ -75,11 +78,12 @@ def sanitise_redirect_url(redirect_to): ...@@ -75,11 +78,12 @@ def sanitise_redirect_url(redirect_to):
if netloc.find(":") != -1: if netloc.find(":") != -1:
netloc, _ = netloc.split(":", 1) netloc, _ = netloc.split(":", 1)
if netloc not in allowed_domains: if netloc not in allowed_domains:
redirect_to = settings.LOGIN_REDIRECT_URL is_valid = False
else:
# netloc is blank, so it's a local URL (possibly with another URL # If the return_to URL is not valid, use the default.
# passed in the querystring. Allow it.) if not is_valid:
pass redirect_to = settings.LOGIN_REDIRECT_URL
return redirect_to return redirect_to
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment