Prevent external redirects

bookmarks/models.py

@@ -99,12 +99,10 @@ class BookmarkForm(forms.ModelForm):
                                   widget=forms.Textarea())
     # Hidden field that determines whether to close window/tab after saving the bookmark
     auto_close = forms.CharField(required=False)
-    # Hidden field that determines where to redirect after saving the form
-    return_url = forms.CharField(required=False)
 
     class Meta:
         model = Bookmark
-        fields = ['url', 'tag_string', 'title', 'description', 'auto_close', 'return_url']
+        fields = ['url', 'tag_string', 'title', 'description', 'auto_close']
 
 
 class UserProfile(models.Model):

bookmarks/templates/bookmarks/edit.html

@@ -7,7 +7,7 @@
             <div class="content-area-header">
                 <h2>Edit bookmark</h2>
             </div>
-            <form action="{% url 'bookmarks:edit' bookmark_id %}" method="post" class="col-6 col-md-12" novalidate>
+            <form action="{% url 'bookmarks:edit' bookmark_id %}?return_url={{ return_url|urlencode }}" method="post" class="col-6 col-md-12" novalidate>
                 {% bookmark_form form return_url bookmark_id %}
             </form>
         </section>

bookmarks/templates/bookmarks/form.html

@@ -4,7 +4,6 @@
 <div class="bookmarks-form">
     {% csrf_token %}
     {{ form.auto_close|attr:"type:hidden" }}
-    {{ form.return_url|attr:"type:hidden" }}
     <div class="form-group {% if form.url.errors %}has-error{% endif %}">
         <label for="{{ form.url.id_for_label }}" class="form-label">URL</label>
         {{ form.url|add_class:"form-input"|attr:"autofocus"|attr:"placeholder: " }}

bookmarks/tests/test_bookmark_archive_view.py

@@ -44,3 +44,12 @@ class BookmarkArchiveViewTestCase(TestCase, BookmarkFactoryMixin):
 
         self.assertEqual(response.status_code, 404)
         self.assertFalse(bookmark.is_archived)
+
+    def test_should_not_redirect_to_external_url(self):
+        bookmark = self.setup_bookmark()
+
+        response = self.client.get(
+            reverse('bookmarks:archive', args=[bookmark.id]) + '?return_url=https://example.com'
+        )
+
+        self.assertRedirects(response, reverse('bookmarks:index'))

bookmarks/tests/test_bookmark_bulk_edit_view.py

@@ -220,3 +220,29 @@ class BookmarkBulkEditViewTestCase(TestCase, BookmarkFactoryMixin):
         })
 
         self.assertBookmarksAreUnmodified([bookmark1, bookmark2, bookmark3])
+
+    def test_bulk_edit_should_redirect_to_return_url(self):
+        bookmark1 = self.setup_bookmark()
+        bookmark2 = self.setup_bookmark()
+        bookmark3 = self.setup_bookmark()
+
+        url = reverse('bookmarks:bulk_edit') + '?return_url=' + reverse('bookmarks:settings.index')
+        response = self.client.post(url, {
+            'bulk_archive': [''],
+            'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
+        })
+
+        self.assertRedirects(response, reverse('bookmarks:settings.index'))
+
+    def test_bulk_edit_should_not_redirect_to_external_url(self):
+        bookmark1 = self.setup_bookmark()
+        bookmark2 = self.setup_bookmark()
+        bookmark3 = self.setup_bookmark()
+
+        url = reverse('bookmarks:bulk_edit') + '?return_url=https://example.com'
+        response = self.client.post(url, {
+            'bulk_archive': [''],
+            'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
+        })
+
+        self.assertRedirects(response, reverse('bookmarks:index'))

bookmarks/tests/test_bookmark_edit_view.py

@@ -20,7 +20,6 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin):
             'tag_string': 'editedtag1 editedtag2',
             'title': 'edited title',
             'description': 'edited description',
-            'return_url': reverse('bookmarks:index'),
         }
         return {**form_data, **overrides}
 
@@ -40,17 +39,6 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin):
         self.assertEqual(bookmark.tags.all()[0].name, 'editedtag1')
         self.assertEqual(bookmark.tags.all()[1].name, 'editedtag2')
 
-    def test_should_use_bookmark_index_as_default_return_url(self):
-        bookmark = self.setup_bookmark()
-
-        response = self.client.get(reverse('bookmarks:edit', args=[bookmark.id]))
-        html = response.content.decode()
-
-        self.assertInHTML(
-            '<input type="hidden" name="return_url" value="{0}" '
-            'id="id_return_url">'.format(reverse('bookmarks:index')),
-            html)
-
     def test_should_prefill_bookmark_form_fields(self):
         tag1 = self.setup_tag()
         tag2 = self.setup_tag()
@@ -81,21 +69,30 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin):
                           '</textarea>'.format(bookmark.description),
                           html)
 
-    def test_should_prefill_return_url_from_url_parameter(self):
-        bookmark = self.setup_bookmark()
-
-        response = self.client.get(reverse('bookmarks:edit', args=[bookmark.id]) + '?return_url=/test-return-url')
-        html = response.content.decode()
-
-        self.assertInHTML('<input type="hidden" name="return_url" value="/test-return-url" id="id_return_url">', html)
-
     def test_should_redirect_to_return_url(self):
         bookmark = self.setup_bookmark()
-        form_data = self.create_form_data({'return_url': reverse('bookmarks:close')})
+        form_data = self.create_form_data()
+
+        url = reverse('bookmarks:edit', args=[bookmark.id]) + '?return_url=' + reverse('bookmarks:close')
+        response = self.client.post(url, form_data)
+
+        self.assertRedirects(response, reverse('bookmarks:close'))
+
+    def test_should_redirect_to_bookmark_index_by_default(self):
+        bookmark = self.setup_bookmark()
+        form_data = self.create_form_data()
 
         response = self.client.post(reverse('bookmarks:edit', args=[bookmark.id]), form_data)
 
-        self.assertRedirects(response, form_data['return_url'])
+        self.assertRedirects(response, reverse('bookmarks:index'))
+
+    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)
+
+        self.assertRedirects(response, reverse('bookmarks:index'))
 
     def test_can_only_edit_own_bookmarks(self):
         other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123')

bookmarks/tests/test_bookmark_new_view.py

@@ -73,6 +73,13 @@ class BookmarkNewViewTestCase(TestCase, BookmarkFactoryMixin):
 
         self.assertRedirects(response, reverse('bookmarks:index'))
 
+    def test_should_not_redirect_to_external_url(self):
+        form_data = self.create_form_data()
+
+        response = self.client.post(reverse('bookmarks:new') + '?return_url=https://example.com', form_data)
+
+        self.assertRedirects(response, reverse('bookmarks:index'))
+
     def test_auto_close_should_redirect_to_close_view(self):
         form_data = self.create_form_data({'auto_close': 'true'})
 

bookmarks/tests/test_bookmark_remove_view.py

@@ -35,6 +35,15 @@ class BookmarkRemoveViewTestCase(TestCase, BookmarkFactoryMixin):
 
         self.assertRedirects(response, reverse('bookmarks:close'))
 
+    def test_should_not_redirect_to_external_url(self):
+        bookmark = self.setup_bookmark()
+
+        response = self.client.get(
+            reverse('bookmarks:remove', args=[bookmark.id]) + '?return_url=https://example.com'
+        )
+
+        self.assertRedirects(response, reverse('bookmarks:index'))
+
     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/tests/test_bookmark_unarchive_view.py

@@ -35,6 +35,15 @@ class BookmarkUnarchiveViewTestCase(TestCase, BookmarkFactoryMixin):
 
         self.assertRedirects(response, reverse('bookmarks:close'))
 
+    def test_should_not_redirect_to_external_url(self):
+        bookmark = self.setup_bookmark()
+
+        response = self.client.get(
+            reverse('bookmarks:unarchive', args=[bookmark.id]) + '?return_url=https://example.com'
+        )
+
+        self.assertRedirects(response, reverse('bookmarks:archived'))
+
     def test_can_only_archive_own_bookmarks(self):
         other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123')
         bookmark = self.setup_bookmark(is_archived=True, user=other_user)

bookmarks/utils.py

@@ -95,3 +95,10 @@ def parse_timestamp(value: str):
 
     # Timestamp is out of range
     raise ValueError(f'{value} exceeds maximum value for a timestamp')
+
+
+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('/'):
+        return fallback_url
+    return return_url

bookmarks/views/bookmarks.py

@@ -10,6 +10,7 @@ from bookmarks import queries
 from bookmarks.models import Bookmark, BookmarkForm, build_tag_string
 from bookmarks.services.bookmarks import create_bookmark, update_bookmark, archive_bookmark, archive_bookmarks, \
     unarchive_bookmark, unarchive_bookmarks, delete_bookmarks, tag_bookmarks, untag_bookmarks
+from bookmarks.utils import get_safe_return_url
 
 _default_page_size = 30
 
@@ -112,22 +113,18 @@ def edit(request, bookmark_id: int):
         bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
     except Bookmark.DoesNotExist:
         raise Http404('Bookmark does not exist')
+    return_url = get_safe_return_url(request.GET.get('return_url'), reverse('bookmarks:index'))
 
     if request.method == 'POST':
         form = BookmarkForm(request.POST, instance=bookmark)
-        return_url = form.data['return_url']
         if form.is_valid():
             tag_string = convert_tag_string(form.data['tag_string'])
             update_bookmark(form.save(commit=False), tag_string, request.user)
             return HttpResponseRedirect(return_url)
     else:
-        return_url = request.GET.get('return_url')
         form = BookmarkForm(instance=bookmark)
 
-    return_url = return_url if return_url else reverse('bookmarks:index')
-
     form.initial['tag_string'] = build_tag_string(bookmark.tag_names, ' ')
-    form.initial['return_url'] = return_url
 
     context = {
         'form': form,
@@ -146,8 +143,7 @@ def remove(request, bookmark_id: int):
         raise Http404('Bookmark does not exist')
 
     bookmark.delete()
-    return_url = request.GET.get('return_url')
+    return_url = get_safe_return_url(request.GET.get('return_url'), reverse('bookmarks:index'))
-    return_url = return_url if return_url else reverse('bookmarks:index')
     return HttpResponseRedirect(return_url)
 
 
@@ -159,8 +155,7 @@ def archive(request, bookmark_id: int):
         raise Http404('Bookmark does not exist')
 
     archive_bookmark(bookmark)
-    return_url = request.GET.get('return_url')
+    return_url = get_safe_return_url(request.GET.get('return_url'), reverse('bookmarks:index'))
-    return_url = return_url if return_url else reverse('bookmarks:index')
     return HttpResponseRedirect(return_url)
 
 
@@ -172,8 +167,7 @@ def unarchive(request, bookmark_id: int):
         raise Http404('Bookmark does not exist')
 
     unarchive_bookmark(bookmark)
-    return_url = request.GET.get('return_url')
+    return_url = get_safe_return_url(request.GET.get('return_url'), reverse('bookmarks:archived'))
-    return_url = return_url if return_url else reverse('bookmarks:archived')
     return HttpResponseRedirect(return_url)
 
 
@@ -195,8 +189,7 @@ def bulk_edit(request):
         tag_string = convert_tag_string(request.POST['bulk_tag_string'])
         untag_bookmarks(bookmark_ids, tag_string, request.user)
 
-    return_url = request.GET.get('return_url')
+    return_url = get_safe_return_url(request.GET.get('return_url'), reverse('bookmarks:index'))
-    return_url = return_url if return_url else reverse('bookmarks:index')
     return HttpResponseRedirect(return_url)