Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix open redirect vulnerability
  • Loading branch information
amarsahinovic authored and cyface committed Jun 23, 2022
1 parent c131a1b commit 03396a1
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 18 deletions.
27 changes: 18 additions & 9 deletions termsandconditions/tests.py
Expand Up @@ -242,7 +242,7 @@ def test_accept(self):
)
self.assertContains(accept_version_post_response, "Secure")

def test_accept_redirect_safe(self):
def _post_accept(self, return_to):
# Pre-accept terms 2 and 3
UserTermsAndConditions.objects.create(user=self.user1, terms=self.terms2)
UserTermsAndConditions.objects.create(user=self.user1, terms=self.terms3)
Expand All @@ -253,19 +253,28 @@ def test_accept_redirect_safe(self):

LOGGER.debug("Test /terms/accept/site-terms/1/ post")
accept_response = self.client.post(
"/terms/accept/", {"terms": 1, "returnTo": "/secure/"}, follow=True
"/terms/accept/", {"terms": 1, "returnTo": return_to}, follow=True
)
return accept_response

def test_accept_redirect_safe(self):
accept_response = self._post_accept("/secure/")
self.assertRedirects(accept_response, "/secure/")

def test_accept_redirect_unsafe(self):
# Pre-accept terms 2 and 3
UserTermsAndConditions.objects.create(user=self.user1, terms=self.terms2)
UserTermsAndConditions.objects.create(user=self.user1, terms=self.terms3)
accept_response = self._post_accept("http://attacker/")
self.assertRedirects(accept_response, "/")

LOGGER.debug("Test /terms/accept/contrib-terms/3/ post")
accept_response = self.client.post(
"/terms/accept/", {"terms": 3, "returnTo": "http://attacker/"}, follow=False
)
def test_accept_redirect_unsafe_2(self):
accept_response = self._post_accept("//attacker.com")
self.assertRedirects(accept_response, "/")

def test_accept_redirect_unsafe_3(self):
accept_response = self._post_accept("///attacker.com")
self.assertRedirects(accept_response, "/")

def test_accept_redirect_unsafe_4(self):
accept_response = self._post_accept("////attacker.com")
self.assertRedirects(accept_response, "/")

def test_accept_store_ip_address(self):
Expand Down
36 changes: 27 additions & 9 deletions termsandconditions/views.py
Expand Up @@ -14,6 +14,7 @@
from django.utils.translation import gettext as _
from django.views.generic import DetailView, CreateView, FormView
from django.template.loader import get_template
from django.utils.encoding import iri_to_uri
import logging
from smtplib import SMTPException

Expand Down Expand Up @@ -44,6 +45,28 @@ def get_terms(self, kwargs):
terms = TermsAndConditions.get_active_terms_not_agreed_to(self.request.user)
return terms

def get_return_to(self, from_dict):
return_to = from_dict.get("returnTo", "/")

if self.is_safe_url(return_to):
# Django recommends to use this together with the helper above
return iri_to_uri(return_to)

LOGGER.debug("Unsafe URL found: {}".format(return_to))
return "/"

def is_safe_url(self, url):
# In Django 3.0 is_safe_url is renamed, so we import conditionally:
# https://docs.djangoproject.com/en/3.2/releases/3.0/#id3
try:
from django.utils.http import url_has_allowed_host_and_scheme
except ImportError:
from django.utils.http import (
is_safe_url as url_has_allowed_host_and_scheme,
)

return url_has_allowed_host_and_scheme(url, settings.ALLOWED_HOSTS)


class AcceptTermsView(CreateView, GetTermsViewMixin):
"""
Expand All @@ -69,22 +92,17 @@ def get_initial(self):
LOGGER.debug("termsandconditions.views.AcceptTermsView.get_initial")

terms = self.get_terms(self.kwargs)
return_to = self.request.GET.get("returnTo", "/")
return_to = self.get_return_to(self.request.GET)

return {"terms": terms, "returnTo": return_to}

def post(self, request, *args, **kwargs):
"""
Handles POST request.
"""
return_url = request.POST.get("returnTo", "/")
return_url = self.get_return_to(self.request.POST)
terms_ids = request.POST.getlist("terms")

parsed = urlparse(return_url)
if parsed.hostname and parsed.hostname not in settings.ALLOWED_HOSTS:
# Make sure the return url is a relative path or a trusted hostname
return_url = '/'

if not terms_ids: # pragma: nocover
return HttpResponseRedirect(return_url)

Expand Down Expand Up @@ -147,7 +165,7 @@ def get_initial(self):

terms = self.get_terms(self.kwargs)

return_to = self.request.GET.get("returnTo", "/")
return_to = self.get_return_to(self.request.GET)

return {"terms": terms, "returnTo": return_to}

Expand Down Expand Up @@ -179,7 +197,7 @@ def form_valid(self, form):
_("An Error Occurred Sending Your Message."),
)

self.success_url = form.cleaned_data.get("returnTo", "/") or "/"
self.success_url = self.get_return_to(form.cleaned_data)

return super().form_valid(form)

Expand Down

0 comments on commit 03396a1

Please sign in to comment.