diff --git a/bookmarks/models.py b/bookmarks/models.py index 7e978c0..764c492 100644 --- a/bookmarks/models.py +++ b/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): diff --git a/bookmarks/templates/bookmarks/edit.html b/bookmarks/templates/bookmarks/edit.html index f60b82a..bb61f81 100644 --- a/bookmarks/templates/bookmarks/edit.html +++ b/bookmarks/templates/bookmarks/edit.html @@ -7,7 +7,7 @@

Edit bookmark

-
+ {% bookmark_form form return_url bookmark_id %}
diff --git a/bookmarks/templates/bookmarks/form.html b/bookmarks/templates/bookmarks/form.html index 9e4ac78..8d8a596 100644 --- a/bookmarks/templates/bookmarks/form.html +++ b/bookmarks/templates/bookmarks/form.html @@ -4,7 +4,6 @@
{% csrf_token %} {{ form.auto_close|attr:"type:hidden" }} - {{ form.return_url|attr:"type:hidden" }}
{{ form.url|add_class:"form-input"|attr:"autofocus"|attr:"placeholder: " }} diff --git a/bookmarks/tests/test_bookmark_archive_view.py b/bookmarks/tests/test_bookmark_archive_view.py index 04a86f3..b3bbd56 100644 --- a/bookmarks/tests/test_bookmark_archive_view.py +++ b/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')) diff --git a/bookmarks/tests/test_bookmark_bulk_edit_view.py b/bookmarks/tests/test_bookmark_bulk_edit_view.py index 5ab6d39..8c6cc96 100644 --- a/bookmarks/tests/test_bookmark_bulk_edit_view.py +++ b/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')) diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index c46d818..d24be6f 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/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( - ''.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): ''.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('', 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') diff --git a/bookmarks/tests/test_bookmark_new_view.py b/bookmarks/tests/test_bookmark_new_view.py index 55489a0..080fa82 100644 --- a/bookmarks/tests/test_bookmark_new_view.py +++ b/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'}) diff --git a/bookmarks/tests/test_bookmark_remove_view.py b/bookmarks/tests/test_bookmark_remove_view.py index 32449a0..1a284d8 100644 --- a/bookmarks/tests/test_bookmark_remove_view.py +++ b/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) diff --git a/bookmarks/tests/test_bookmark_unarchive_view.py b/bookmarks/tests/test_bookmark_unarchive_view.py index 25cc79a..cdfcc51 100644 --- a/bookmarks/tests/test_bookmark_unarchive_view.py +++ b/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) diff --git a/bookmarks/utils.py b/bookmarks/utils.py index a931105..39c59fd 100644 --- a/bookmarks/utils.py +++ b/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 diff --git a/bookmarks/views/bookmarks.py b/bookmarks/views/bookmarks.py index 662c29a..44d82bd 100644 --- a/bookmarks/views/bookmarks.py +++ b/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 = return_url if return_url else reverse('bookmarks:index') + return_url = get_safe_return_url(request.GET.get('return_url'), 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 = return_url if return_url else reverse('bookmarks:index') + return_url = get_safe_return_url(request.GET.get('return_url'), 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 = return_url if return_url else reverse('bookmarks:archived') + return_url = get_safe_return_url(request.GET.get('return_url'), 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 = return_url if return_url else reverse('bookmarks:index') + return_url = get_safe_return_url(request.GET.get('return_url'), reverse('bookmarks:index')) return HttpResponseRedirect(return_url)