Prevent external redirects

bookmarks/tests/test_bookmark_action_view.py

@@ -74,7 +74,6 @@ def test_delete_should_delete_bookmark(self):
 
         self.assertEqual(Bookmark.objects.count(), 0)
 
-
     def test_delete_can_only_delete_own_bookmarks(self):
         other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123')
         bookmark = self.setup_bookmark(user=other_user)
@@ -305,10 +304,19 @@ def test_should_not_redirect_to_external_url(self):
         bookmark2 = self.setup_bookmark()
         bookmark3 = self.setup_bookmark()
 
-        url = reverse('bookmarks:action') + '?return_url=https://example.com'
-        response = self.client.post(url, {
-            'bulk_archive': [''],
-            'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
-        })
+        def post_with(return_url, follow=None):
+            url = reverse('bookmarks:action') + f'?return_url={return_url}'
+            return self.client.post(url, {
+                'bulk_archive': [''],
+                'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
+            }, follow=follow)
 
+        response = post_with('https://example.com')
         self.assertRedirects(response, reverse('bookmarks:index'))
+        response = post_with('//example.com')
+        self.assertRedirects(response, reverse('bookmarks:index'))
+        response = post_with('://example.com')
+        self.assertRedirects(response, reverse('bookmarks:index'))
+
+        response = post_with('/foo//example.com', follow=True)
+        self.assertEqual(response.status_code, 404)

bookmarks/tests/test_bookmark_edit_view.py

@@ -88,12 +88,22 @@ def test_should_redirect_to_bookmark_index_by_default(self):
 
     def test_should_not_redirect_to_external_url(self):
         bookmark = self.setup_bookmark()
-        form_data = self.create_form_data({'return_url': 'https://example.com'})
 
-        response = self.client.post(reverse('bookmarks:edit', args=[bookmark.id]), form_data)
+        def post_with(return_url, follow=None):
+            form_data = self.create_form_data()
+            url = reverse('bookmarks:edit', args=[bookmark.id]) + f'?return_url={return_url}'
+            return self.client.post(url, form_data, follow=follow)
 
+        response = post_with('https://example.com')
+        self.assertRedirects(response, reverse('bookmarks:index'))
+        response = post_with('//example.com')
+        self.assertRedirects(response, reverse('bookmarks:index'))
+        response = post_with('://example.com')
         self.assertRedirects(response, reverse('bookmarks:index'))
 
+        response = post_with('/foo//example.com', follow=True)
+        self.assertEqual(response.status_code, 404)
+
     def test_can_only_edit_own_bookmarks(self):
         other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123')
         bookmark = self.setup_bookmark(user=other_user)

bookmarks/utils.py

@@ -1,3 +1,4 @@
+import re
 from datetime import datetime
 from typing import Optional
 
@@ -99,6 +100,6 @@ def parse_timestamp(value: str):
 
 def get_safe_return_url(return_url: str, fallback_url: str):
     # Use fallback if URL is none or URL is not on same domain
-    if not return_url or not return_url.startswith('/'):
+    if not return_url or not re.match(r'^/[a-z]+', return_url):
         return fallback_url
     return return_url