diff --git a/djangocms_redirect/admin.py b/djangocms_redirect/admin.py index 2e2ef35..da7028e 100644 --- a/djangocms_redirect/admin.py +++ b/djangocms_redirect/admin.py @@ -4,6 +4,7 @@ from django.utils.translation import get_language from .models import Redirect +from .utils import normalize_url class RedirectForm(ModelForm): @@ -18,6 +19,9 @@ def __init__(self, *args, **kwargs): self.fields["old_path"].widget = widget self.fields["new_path"].widget = widget + def clean_old_path(self): + return normalize_url(self.cleaned_data.get("old_path")) + @admin.register(Redirect) class RedirectAdmin(admin.ModelAdmin): diff --git a/djangocms_redirect/middleware.py b/djangocms_redirect/middleware.py index e970a2b..7e2463d 100644 --- a/djangocms_redirect/middleware.py +++ b/djangocms_redirect/middleware.py @@ -8,7 +8,7 @@ from django.core.exceptions import ImproperlyConfigured from django.db.models import Q from django.utils.deprecation import MiddlewareMixin -from django.utils.http import urlunquote_plus +from django.utils.encoding import escape_uri_path, iri_to_uri from .models import Redirect from .utils import get_key_from_path_and_site @@ -32,22 +32,30 @@ def do_redirect(self, request, response=None): if getattr(settings, "DJANGOCMS_REDIRECT_404_ONLY", True) and response and response.status_code != 404: return response - full_path_quoted, part, querystring = request.get_full_path().partition("?") - possible_paths = [full_path_quoted] - full_path_unquoted = urlunquote_plus(full_path_quoted) - if full_path_unquoted != full_path_quoted: - possible_paths.append(urlunquote_plus(full_path_unquoted)) - if not settings.APPEND_SLASH and not request.path.endswith("/"): - full_path_slash, __, __ = request.get_full_path(force_append_slash=True).partition("?") - possible_paths.append(full_path_slash) - full_path_slash_unquoted = urlunquote_plus(full_path_slash) - if full_path_slash_unquoted != full_path_slash: - possible_paths.append(full_path_slash_unquoted) - querystring = "{}{}".format(part, querystring) + req_path = request.path + # get the query string + querystring = request.META.get("QUERY_STRING", "") + if querystring: + querystring = "?%s" % iri_to_uri(querystring) + # start with the path as is + possible_paths = [req_path] + # add the unquoted path if it differs + req_path_quoted = escape_uri_path(req_path) + if req_path_quoted != req_path: + possible_paths.append(req_path_quoted) + # if a slash is missing, try to append it + if not req_path.endswith("/"): + req_path_slash = req_path + "/" + possible_paths.append(req_path_slash) + req_path_slash_quoted = escape_uri_path(req_path_slash) + if req_path_slash_quoted != req_path_slash: + possible_paths.append(req_path_slash_quoted) + current_site = get_current_site(request) r = None - key = get_key_from_path_and_site(full_path_quoted, settings.SITE_ID) + key = get_key_from_path_and_site(req_path, settings.SITE_ID) cached_redirect = cache.get(key) + if not cached_redirect: for path in possible_paths: filters = dict(site=current_site, old_path=path) @@ -58,6 +66,7 @@ def do_redirect(self, request, response=None): r = self._match_substring(path) if r: break + cached_redirect = { "site": settings.SITE_ID, "redirect": r.new_path if r else None, diff --git a/djangocms_redirect/models.py b/djangocms_redirect/models.py index a5f9d2c..8fb51db 100644 --- a/djangocms_redirect/models.py +++ b/djangocms_redirect/models.py @@ -1,3 +1,5 @@ +from urllib.parse import unquote_plus + from django.contrib.sites.models import Site from django.core.cache import cache from django.db import models @@ -5,6 +7,8 @@ from django.dispatch import receiver from django.utils.translation import ugettext_lazy as _ +from .utils import normalize_url + RESPONSE_CODES = ( ("301", _("301 - Permanent redirection")), ( @@ -59,6 +63,10 @@ class Meta: unique_together = (("site", "old_path"),) ordering = ("old_path",) + def clean(self): + self.old_path = normalize_url(self.old_path) + super().clean() + def __str__(self): return "{} ---> {}".format(self.old_path, self.new_path) @@ -68,5 +76,6 @@ def __str__(self): def clear_redirect_cache(**kwargs): from .utils import get_key_from_path_and_site - key = get_key_from_path_and_site(kwargs["instance"].old_path, kwargs["instance"].site_id) + path = unquote_plus(kwargs["instance"].old_path) + key = get_key_from_path_and_site(path, kwargs["instance"].site_id) cache.delete(key) diff --git a/djangocms_redirect/utils.py b/djangocms_redirect/utils.py index 5572cc3..543d9b4 100644 --- a/djangocms_redirect/utils.py +++ b/djangocms_redirect/utils.py @@ -1,5 +1,7 @@ import hashlib +from django.conf import settings + def get_key_from_path_and_site(path, site_id): """ @@ -12,3 +14,11 @@ def get_key_from_path_and_site(path, site_id): hashed_path = hashlib.sha224(path.encode("utf-8")).hexdigest() key = "CMSREDIRECT:{}:{}".format(hashed_path, site_id) return key + + +def normalize_url(path): + if settings.APPEND_SLASH and not path.endswith("/"): + path = "%s/" % path + if not path.startswith("/"): + path = "/%s" % path + return path diff --git a/docs/usage.rst b/docs/usage.rst index 9a44299..7b42868 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -12,6 +12,17 @@ For each redirect you must provide: * **Redirect to**: The path to which the request will be redirected: you can type any URL or select an existing django CMS page. * **Response code**: You can select 3 types of status_code header: 301 (permanent redirect), 302 (temporary redirect) or 410 (permanent unavailable resource). +Each **redirect from** URL must be unique and start with a slash. If you leave out the +leading slash when creating a redirect, it is added automatically. + +If the user requests a page without a trailing slash and there is no redirect for that +URL but there is one for the URL with a trailing slash, that redirect is used. For +backwards-compatibility, this is also true when ``APPEND_SLASH=False``. + +If ``APPEND_SLASH=True`` (the default), a trailing slash is added automatically when +creating a redirect. That way, there is only ever a single relevant redirect, +whether there is a trailing slash or not. + **************** Subpath matching **************** diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 9449951..8e898ad 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -4,6 +4,7 @@ from django.utils.encoding import force_text from django.utils.http import urlunquote_plus +from djangocms_redirect.admin import RedirectForm from djangocms_redirect.middleware import RedirectMiddleware from djangocms_redirect.models import Redirect @@ -208,6 +209,22 @@ def test_redirect_no_append_slash(self): response = self.client.get(pages[1].get_absolute_url().rstrip("/")) self.assertRedirects(response, redirect.new_path, status_code=301) + def test_redirect_no_append_slash_unquoted(self): + pages = self.get_pages() + + original_path = "/path (escaped)/" + with override_settings(APPEND_SLASH=False): + redirect = Redirect.objects.create( + site=self.site_1, + old_path=original_path, + new_path=pages[0].get_absolute_url(), + response_code="301", + ) + + with self.assertNumQueries(5): + response = self.client.get(original_path.rstrip("/")) + self.assertRedirects(response, redirect.new_path, status_code=301) + def test_redirect_no_append_slash_quoted(self): pages = self.get_pages() @@ -220,7 +237,7 @@ def test_redirect_no_append_slash_quoted(self): response_code="301", ) - with self.assertNumQueries(5): + with self.assertNumQueries(7): response = self.client.get(original_path.rstrip("/")) self.assertRedirects(response, redirect.new_path, status_code=301) @@ -319,6 +336,43 @@ def test_slash_append_slash(self): self.assertRedirects(response, "/en/b/", status_code=301) +class TestClean(BaseRedirectTest): + + _pages_data = ( + {"en": {"title": "home page", "template": "page.html", "publish": True}}, + ) + + def _make_form(self, old_path): + pages = self.get_pages() + return RedirectForm({ + "site": self.site_1.pk, + "old_path": old_path, + "new_path": pages[0].get_absolute_url(), + "response_code": "301", + }) + + def _assert_clean(self, old_path, old_path_cleaned): + redirect_form = self._make_form(old_path) + redirect = redirect_form.save() + self.assertEqual(redirect.old_path, old_path_cleaned) + + def test_clean_keep_slash(self): + self._assert_clean("/slash-bar/", "/slash-bar/") + + def test_clean_prepend_slash(self): + self._assert_clean("slash-test/", "/slash-test/") + + def test_clean_single_slash_root(self): + self._assert_clean("/", "/") + + def test_clean_append_slash(self): + with override_settings(APPEND_SLASH=True): + self._assert_clean("/slash-foo", "/slash-foo/") + + def test_clean_no_append_slash(self): + with override_settings(APPEND_SLASH=False): + self._assert_clean("/slash-baz", "/slash-baz") + class TestPartialMatch(BaseRedirectTest): _pages_data = ( {"en": {"title": "home page", "template": "page.html", "publish": True}},