From 3906d9e5b86c56e26e9b4cc0f1e4f2e8fcc44744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Sun, 27 Mar 2022 11:47:45 +0200 Subject: [PATCH] Prevent external redirects --- bookmarks/tests/test_bookmark_action_view.py | 20 ++++++++++++++------ bookmarks/tests/test_bookmark_edit_view.py | 14 ++++++++++++-- bookmarks/utils.py | 3 ++- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/bookmarks/tests/test_bookmark_action_view.py b/bookmarks/tests/test_bookmark_action_view.py index 0737f8b3..9d457566 100644 --- a/bookmarks/tests/test_bookmark_action_view.py +++ b/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) diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index d24be6f5..43d3b739 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/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) diff --git a/bookmarks/utils.py b/bookmarks/utils.py index 39c59fd8..0352fbe0 100644 --- a/bookmarks/utils.py +++ b/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