Skip to content

Commit 5b1f3e0

Browse files
Vishnu Kosuriclaude
andcommitted
Fix open redirect in account views and URL shortener
logoutView, loginView, and updateProfile accepted arbitrary URLs in the nextPage parameter and passed them directly to HttpResponseRedirect. Validate nextPage against the current host using Django's url_has_allowed_host_and_scheme so external redirects are rejected. The URL shortener's shorten view stored paths with leading slashes intact. When follow() concatenated reverse('browser') with a path like //evil.com, some browsers normalize the result to a protocol-relative URL pointing externally. Strip leading slashes from the stored path at write time to prevent this. Tests added for both cases. Fixes #2871 and #2872. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent d6dfc59 commit 5b1f3e0

3 files changed

Lines changed: 46 additions & 4 deletions

File tree

webapp/graphite/account/views.py

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,26 @@
1919
from django.urls import reverse
2020
from django.http import HttpResponseRedirect
2121
from django.shortcuts import render
22+
from django.utils.http import url_has_allowed_host_and_scheme
2223
from graphite.user_util import getProfile, isAuthenticated
2324

2425

26+
def _safe_next_page(request, raw):
27+
url_is_safe = url_has_allowed_host_and_scheme(
28+
url=raw,
29+
allowed_hosts={request.get_host()},
30+
require_https=request.is_secure(),
31+
)
32+
return raw if url_is_safe else reverse('browser')
33+
34+
2535
def loginView(request):
2636
username = request.POST.get('username')
2737
password = request.POST.get('password')
2838
if request.method == 'GET':
29-
nextPage = request.GET.get('nextPage', reverse('browser'))
39+
nextPage = _safe_next_page(request, request.GET.get('nextPage', reverse('browser')))
3040
else:
31-
nextPage = request.POST.get('nextPage', reverse('browser'))
41+
nextPage = _safe_next_page(request, request.POST.get('nextPage', reverse('browser')))
3242
if username and password:
3343
user = authenticate(username=username,password=password)
3444
if user is None:
@@ -43,7 +53,7 @@ def loginView(request):
4353

4454

4555
def logoutView(request):
46-
nextPage = request.GET.get('nextPage', reverse('browser'))
56+
nextPage = _safe_next_page(request, request.GET.get('nextPage', reverse('browser')))
4757
logout(request)
4858
return HttpResponseRedirect(nextPage)
4959

@@ -60,5 +70,5 @@ def updateProfile(request):
6070
if profile:
6171
profile.advancedUI = request.POST.get('advancedUI','off') == 'on'
6272
profile.save()
63-
nextPage = request.POST.get('nextPage', reverse('browser'))
73+
nextPage = _safe_next_page(request, request.POST.get('nextPage', reverse('browser')))
6474
return HttpResponseRedirect(nextPage)

webapp/graphite/url_shortener/views.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ def shorten(request, path):
2222
path += '?' + request.META['QUERY_STRING']
2323
# Remove _salt, _dc and _uniq to avoid creating many copies of the same URL
2424
path = re.sub('&_(uniq|salt|dc)=[0-9.]+', "", path)
25+
# Strip leading slashes so the stored path, when concatenated with
26+
# reverse('browser'), cannot produce a protocol-relative URL like //host.
27+
path = path.lstrip('/')
2528

2629
link, created = Link.objects.get_or_create(url=path)
2730
link_id = base62.from_decimal(link.id)

webapp/tests/test_xss.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,32 @@ def test_render_xss(self):
4040

4141
response = self.client.get(url, {'query': 'test', 'local': xssStr, 'from': xssStr, 'tz': xssStr})
4242
self.assertXSS(response, status_code=400, msg_prefix='XSS detected: ')
43+
44+
45+
class LogoutOpenRedirectTest(TestCase):
46+
def test_logout_external_redirect_blocked(self):
47+
url = reverse('account_logout')
48+
response = self.client.get(url, {'nextPage': 'http://evil.example.com'})
49+
location = response.get('Location', '')
50+
self.assertNotIn('evil.example.com', location)
51+
52+
def test_logout_local_redirect_allowed(self):
53+
url = reverse('account_logout')
54+
response = self.client.get(url, {'nextPage': '/browser/'})
55+
location = response.get('Location', '')
56+
self.assertEqual(location, '/browser/')
57+
58+
59+
class URLShortenerOpenRedirectTest(TestCase):
60+
def test_shorten_strips_leading_slashes(self):
61+
shorten_url = reverse('shorten', kwargs={'path': '//evil.example.com'})
62+
shorten_response = self.client.get(shorten_url)
63+
self.assertEqual(shorten_response.status_code, 200)
64+
link_id = shorten_response.content.decode('utf-8').rsplit('/', 1)[-1]
65+
follow_url = reverse('follow', kwargs={'link_id': link_id})
66+
follow_response = self.client.get(follow_url)
67+
location = follow_response.get('Location', '')
68+
self.assertFalse(
69+
location.startswith('//') or location.startswith('http://evil') or location.startswith('https://evil'),
70+
msg='Open redirect detected: %s' % location,
71+
)

0 commit comments

Comments
 (0)