Merge pull request #2749 from bookwyrm-social/redirects

Add helper to refer views back to http referers safely

bookwyrm/tests/test_utils.py

@@ -22,8 +22,8 @@ class TestUtils(TestCase):
 
     def test_invalid_url_domain(self):
         """Check with an invalid URL"""
-        self.assertEqual(
-            validate_url_domain("https://up-to-no-good.tld/bad-actor.exe"), "/"
+        self.assertIsNone(
+            validate_url_domain("https://up-to-no-good.tld/bad-actor.exe")
         )
 
     def test_default_url_domain(self):

bookwyrm/tests/views/test_helpers.py

@@ -8,7 +8,7 @@ from django.test.client import RequestFactory
 import responses
 
 from bookwyrm import models, views
-from bookwyrm.settings import USER_AGENT
+from bookwyrm.settings import USER_AGENT, DOMAIN
 
 
 @patch("bookwyrm.activitystreams.add_status_task.delay")
@@ -18,6 +18,7 @@ from bookwyrm.settings import USER_AGENT
 class ViewsHelpers(TestCase):
     """viewing and creating statuses"""
 
+    # pylint: disable=invalid-name
     def setUp(self):
         """we need basic test data and mocks"""
         self.factory = RequestFactory()
@@ -260,3 +261,33 @@ class ViewsHelpers(TestCase):
                 self.local_user, self.shelf, self.book, "public"
             )
         self.assertFalse(models.GeneratedNote.objects.exists())
+
+    def test_redirect_to_referer_outside_domain(self, *_):
+        """safely send people on their way"""
+        request = self.factory.get("/path")
+        request.META = {"HTTP_REFERER": "http://outside.domain/name"}
+        result = views.helpers.redirect_to_referer(
+            request, "user-feed", self.local_user.localname
+        )
+        self.assertEqual(result.url, f"/user/{self.local_user.localname}")
+
+    def test_redirect_to_referer_outside_domain_with_fallback(self, *_):
+        """invalid domain with regular params for the redirect function"""
+        request = self.factory.get("/path")
+        request.META = {"HTTP_REFERER": "https://outside.domain/name"}
+        result = views.helpers.redirect_to_referer(request)
+        self.assertEqual(result.url, "/")
+
+    def test_redirect_to_referer_valid_domain(self, *_):
+        """redirect to within the app"""
+        request = self.factory.get("/path")
+        request.META = {"HTTP_REFERER": f"https://{DOMAIN}/and/a/path"}
+        result = views.helpers.redirect_to_referer(request)
+        self.assertEqual(result.url, f"https://{DOMAIN}/and/a/path")
+
+    def test_redirect_to_referer_with_get_args(self, *_):
+        """if the path has get params (like sort) they are preserved"""
+        request = self.factory.get("/path")
+        request.META = {"HTTP_REFERER": f"https://{DOMAIN}/and/a/path?sort=hello"}
+        result = views.helpers.redirect_to_referer(request)
+        self.assertEqual(result.url, f"https://{DOMAIN}/and/a/path?sort=hello")

bookwyrm/utils/validate.py

@@ -2,12 +2,12 @@
 from bookwyrm.settings import DOMAIN, USE_HTTPS
 
 
-def validate_url_domain(url, default="/"):
+def validate_url_domain(url):
     """Basic check that the URL starts with the instance domain name"""
     if not url:
-        return default
+        return None
 
-    if url in ("/", default):
+    if url == "/":
         return url
 
     protocol = "https://" if USE_HTTPS else "http://"
@@ -16,4 +16,4 @@ def validate_url_domain(url, default="/"):
     if url.startswith(origin):
         return url
 
-    return default
+    return None

bookwyrm/views/helpers.py

@@ -16,6 +16,7 @@ from bookwyrm import activitypub, models, settings
 from bookwyrm.connectors import ConnectorException, get_data
 from bookwyrm.status import create_generated_note
 from bookwyrm.utils import regex
+from bookwyrm.utils.validate import validate_url_domain
 
 
 # pylint: disable=unnecessary-pass
@@ -219,3 +220,15 @@ def maybe_redirect_local_path(request, model):
         new_path = f"{model.local_path}?{request.GET.urlencode()}"
 
     return redirect(new_path, permanent=True)
+
+
+def redirect_to_referer(request, *args):
+    """Redirect to the referrer, if it's in our domain, with get params"""
+    # make sure the refer is part of this instance
+    validated = validate_url_domain(request.META.get("HTTP_REFERER"))
+
+    if validated:
+        return redirect(validated)
+
+    # if not, use the args passed you'd normally pass to redirect()
+    return redirect(*args or "/")

bookwyrm/views/list/list.py

@@ -8,7 +8,7 @@ from django.db import transaction
 from django.db.models import Avg, DecimalField, Q, Max
 from django.db.models.functions import Coalesce
 from django.http import HttpResponseBadRequest, HttpResponse
-from django.shortcuts import get_object_or_404, redirect
+from django.shortcuts import get_object_or_404
 from django.template.response import TemplateResponse
 from django.urls import reverse
 from django.utils.decorators import method_decorator
@@ -18,7 +18,11 @@ from django.views.decorators.http import require_POST
 from bookwyrm import book_search, forms, models
 from bookwyrm.activitypub import ActivitypubResponse
 from bookwyrm.settings import PAGE_LENGTH
-from bookwyrm.views.helpers import is_api_request, maybe_redirect_local_path
+from bookwyrm.views.helpers import (
+    is_api_request,
+    maybe_redirect_local_path,
+    redirect_to_referer,
+)
 
 
 # pylint: disable=no-self-use
@@ -91,7 +95,7 @@ class List(View):
             book_list.group = None
             book_list.save(broadcast=False)
 
-        return redirect(book_list.local_path)
+        return redirect_to_referer(request, book_list.local_path)
 
 
 def get_list_suggestions(book_list, user, query=None, num_suggestions=5):
@@ -157,7 +161,7 @@ def save_list(request, list_id):
     """save a list"""
     book_list = get_object_or_404(models.List, id=list_id)
     request.user.saved_lists.add(book_list)
-    return redirect("list", list_id)
+    return redirect_to_referer(request, "list", list_id)
 
 
 @require_POST
@@ -166,7 +170,7 @@ def unsave_list(request, list_id):
     """unsave a list"""
     book_list = get_object_or_404(models.List, id=list_id)
     request.user.saved_lists.remove(book_list)
-    return redirect("list", list_id)
+    return redirect_to_referer(request, "list", list_id)
 
 
 @require_POST
@@ -179,7 +183,7 @@ def delete_list(request, list_id):
     book_list.raise_not_deletable(request.user)
 
     book_list.delete()
-    return redirect("lists")
+    return redirect_to_referer(request, "lists")
 
 
 @require_POST
@@ -236,7 +240,7 @@ def remove_book(request, list_id):
         item.delete()
         normalize_book_list_ordering(book_list.id, start=deleted_order)
 
-    return redirect("list", list_id)
+    return redirect_to_referer(request, "list", list_id)
 
 
 @require_POST
@@ -283,7 +287,7 @@ def set_book_position(request, list_item_id):
         list_item.order = int_position
         list_item.save()
 
-    return redirect("list", book_list.id)
+    return redirect_to_referer(request, book_list.local_path)
 
 
 @transaction.atomic

bookwyrm/views/reading.py

@@ -12,10 +12,9 @@ from django.views.decorators.http import require_POST
 
 from bookwyrm import forms, models
 from bookwyrm.views.shelf.shelf_actions import unshelve
-from bookwyrm.utils.validate import validate_url_domain
 from .status import CreateStatus
 from .helpers import get_edition, handle_reading_status, is_api_request
-from .helpers import load_date_in_user_tz_as_utc
+from .helpers import load_date_in_user_tz_as_utc, redirect_to_referer
 
 logger = logging.getLogger(__name__)
 
@@ -43,8 +42,6 @@ class ReadingStatus(View):
     @transaction.atomic
     def post(self, request, status, book_id):
         """Change the state of a book by shelving it and adding reading dates"""
-        next_step = request.META.get("HTTP_REFERER")
-        next_step = validate_url_domain(next_step, "/")
         identifier = {
             "want": models.Shelf.TO_READ,
             "start": models.Shelf.READING,
@@ -86,7 +83,7 @@ class ReadingStatus(View):
             if current_status_shelfbook.shelf.identifier != desired_shelf.identifier:
                 current_status_shelfbook.delete()
             else:  # It already was on the shelf
-                return redirect(next_step)
+                return redirect_to_referer(request)
 
         models.ShelfBook.objects.create(
             book=book, shelf=desired_shelf, user=request.user
@@ -124,7 +121,7 @@ class ReadingStatus(View):
         if is_api_request(request):
             return HttpResponse()
 
-        return redirect(next_step)
+        return redirect_to_referer(request)
 
 
 @method_decorator(login_required, name="dispatch")

bookwyrm/views/shelf/shelf_actions.py

@@ -3,9 +3,9 @@ from django.db import IntegrityError, transaction
 from django.contrib.auth.decorators import login_required
 from django.shortcuts import get_object_or_404, redirect
 from django.views.decorators.http import require_POST
-from bookwyrm.utils.validate import validate_url_domain
 
 from bookwyrm import forms, models
+from bookwyrm.views.helpers import redirect_to_referer
 
 
 @login_required
@@ -36,8 +36,6 @@ def delete_shelf(request, shelf_id):
 @transaction.atomic
 def shelve(request):
     """put a book on a user's shelf"""
-    next_step = request.META.get("HTTP_REFERER")
-    next_step = validate_url_domain(next_step, "/")
     book = get_object_or_404(models.Edition, id=request.POST.get("book"))
     desired_shelf = get_object_or_404(
         request.user.shelf_set, identifier=request.POST.get("shelf")
@@ -74,7 +72,7 @@ def shelve(request):
             ):
                 current_read_status_shelfbook.delete()
             else:
-                return redirect(next_step)
+                return redirect_to_referer(request)
 
         # create the new shelf-book entry
         models.ShelfBook.objects.create(
@@ -91,15 +89,13 @@ def shelve(request):
         except IntegrityError:
             pass
 
-    return redirect(next_step)
+    return redirect_to_referer(request)
 
 
 @login_required
 @require_POST
 def unshelve(request, book_id=False):
     """remove a book from a user's shelf"""
-    next_step = request.META.get("HTTP_REFERER")
-    next_step = validate_url_domain(next_step, "/")
     identity = book_id if book_id else request.POST.get("book")
     book = get_object_or_404(models.Edition, id=identity)
     shelf_book = get_object_or_404(
@@ -107,4 +103,4 @@ def unshelve(request, book_id=False):
     )
     shelf_book.raise_not_deletable(request.user)
     shelf_book.delete()
-    return redirect(next_step)
+    return redirect_to_referer(request)

bookwyrm/views/status.py

@@ -18,9 +18,8 @@ from django.views.decorators.http import require_POST
 from markdown import markdown
 from bookwyrm import forms, models
 from bookwyrm.utils import regex, sanitizer
-from bookwyrm.utils.validate import validate_url_domain
 from .helpers import handle_remote_webfinger, is_api_request
-from .helpers import load_date_in_user_tz_as_utc
+from .helpers import load_date_in_user_tz_as_utc, redirect_to_referer
 
 logger = logging.getLogger(__name__)
 
@@ -59,8 +58,6 @@ class CreateStatus(View):
     # pylint: disable=too-many-branches
     def post(self, request, status_type, existing_status_id=None):
         """create status of whatever type"""
-        next_step = request.META.get("HTTP_REFERER")
-        next_step = validate_url_domain(next_step, "/")
         created = not existing_status_id
         existing_status = None
         if existing_status_id:
@@ -83,7 +80,7 @@ class CreateStatus(View):
             if is_api_request(request):
                 logger.exception(form.errors)
                 return HttpResponseBadRequest()
-            return redirect(next_step)
+            return redirect_to_referer(request)
 
         status = form.save(request, commit=False)
         status.ready = False
@@ -150,7 +147,7 @@ class CreateStatus(View):
 
         if is_api_request(request):
             return HttpResponse()
-        return redirect(next_step)
+        return redirect_to_referer(request)
 
 
 @method_decorator(login_required, name="dispatch")
@@ -183,8 +180,6 @@ def update_progress(request, book_id):  # pylint: disable=unused-argument
 def edit_readthrough(request):
     """can't use the form because the dates are too finnicky"""
     # TODO: remove this, it duplicates the code in the ReadThrough view
-    next_step = request.META.get("HTTP_REFERER")
-    next_step = validate_url_domain(next_step, "/")
     readthrough = get_object_or_404(models.ReadThrough, id=request.POST.get("id"))
 
     readthrough.start_date = load_date_in_user_tz_as_utc(
@@ -216,7 +211,7 @@ def edit_readthrough(request):
 
     if is_api_request(request):
         return HttpResponse()
-    return redirect(next_step)
+    return redirect_to_referer(request)
 
 
 def find_mentions(user, content):