Skip to content

Commit

Permalink
Prevent bookmark actions through get requests
Browse files Browse the repository at this point in the history
  • Loading branch information
@sissbruecker
sissbruecker committed Mar 27, 2022
1 parent 10e5861 commit eca98a1
Show file tree
Hide file tree
Showing 11 changed files with 109 additions and 209 deletions.
1 change: 1 addition & 0 deletions bookmarks/static/shared.js
View file
Expand Up @@ -19,6 +19,7 @@
if (buttonEl.nodeName === 'BUTTON') {
confirmEl.type = buttonEl.type;
confirmEl.name = buttonEl.name;
confirmEl.value = buttonEl.value;
}
if (buttonEl.nodeName === 'A') {
confirmEl.href = buttonEl.href;
Expand Down
4 changes: 2 additions & 2 deletions bookmarks/styles/bookmarks.scss
View file
Expand Up @@ -153,13 +153,13 @@ ul.bookmark-list {
}
}

/* Bulk edit */
/* Bookmark actions / bulk edit */
$bulk-edit-toggle-width: 16px;
$bulk-edit-toggle-offset: 8px;
$bulk-edit-bar-offset: $bulk-edit-toggle-width + (2 * $bulk-edit-toggle-offset);
$bulk-edit-transition-duration: 400ms;

.bulk-edit-form {
.bookmarks-page form.bookmark-actions {

.bulk-edit-bar {
margin-top: -17px;
Expand Down
2 changes: 1 addition & 1 deletion bookmarks/templates/bookmarks/archive.html
View file
Expand Up @@ -18,7 +18,7 @@ <h2>Archived bookmarks</h2>
{% include 'bookmarks/bulk_edit/toggle.html' %}
</div>

<form class="bulk-edit-form" action="{% url 'bookmarks:bulk_edit' %}?return_url={{ return_url }}"
<form class="bookmark-actions" action="{% url 'bookmarks:action' %}?return_url={{ return_url }}"
method="post">
{% csrf_token %}
{% include 'bookmarks/bulk_edit/bar.html' with mode='archive' %}
Expand Down
12 changes: 6 additions & 6 deletions bookmarks/templates/bookmarks/bookmark_list.html
View file
Expand Up @@ -57,14 +57,14 @@
<a href="{% url 'bookmarks:edit' bookmark.id %}?return_url={{ return_url }}"
class="btn btn-link btn-sm">Edit</a>
{% if bookmark.is_archived %}
<a href="{% url 'bookmarks:unarchive' bookmark.id %}?return_url={{ return_url }}"
class="btn btn-link btn-sm">Unarchive</a>
<button type="submit" name="unarchive" value="{{ bookmark.id }}"
class="btn btn-link btn-sm">Unarchive</button>
{% else %}
<a href="{% url 'bookmarks:archive' bookmark.id %}?return_url={{ return_url }}"
class="btn btn-link btn-sm">Archive</a>
<button type="submit" name="archive" value="{{ bookmark.id }}"
class="btn btn-link btn-sm">Archive</button>
{% endif %}
<a href="{% url 'bookmarks:remove' bookmark.id %}?return_url={{ return_url }}"
class="btn btn-link btn-sm btn-confirmation">Remove</a>
<button type="submit" name="remove" value="{{ bookmark.id }}"
class="btn btn-link btn-sm btn-confirmation">Remove</button>
</div>
</li>
{% endfor %}
Expand Down
2 changes: 1 addition & 1 deletion bookmarks/templates/bookmarks/index.html
View file
Expand Up @@ -18,7 +18,7 @@ <h2>Bookmarks</h2>
{% include 'bookmarks/bulk_edit/toggle.html' %}
</div>

<form class="bulk-edit-form" action="{% url 'bookmarks:bulk_edit' %}?return_url={{ return_url }}"
<form class="bookmark-actions" action="{% url 'bookmarks:action' %}?return_url={{ return_url }}"
method="post">
{% csrf_token %}
{% include 'bookmarks/bulk_edit/bar.html' with mode='default' %}
Expand Down
104 changes: 85 additions & 19 deletions ...rks/tests/test_bookmark_bulk_edit_view.py → bookmarks/tests/test_bookmark_action_view.py
View file
Expand Up @@ -7,7 +7,7 @@
from bookmarks.tests.helpers import BookmarkFactoryMixin


class BookmarkBulkEditViewTestCase(TestCase, BookmarkFactoryMixin):
class BookmarkActionViewTestCase(TestCase, BookmarkFactoryMixin):

def setUp(self) -> None:
user = self.get_or_create_test_user()
Expand All @@ -19,12 +19,78 @@ def assertBookmarksAreUnmodified(self, bookmarks: [Bookmark]):
for bookmark in bookmarks:
self.assertEqual(model_to_dict(bookmark), model_to_dict(Bookmark.objects.get(id=bookmark.id)))

def test_archive_should_archive_bookmark(self):
bookmark = self.setup_bookmark()

self.client.post(reverse('bookmarks:action'), {
'archive': [bookmark.id],
})

bookmark.refresh_from_db()

self.assertTrue(bookmark.is_archived)

def test_can_only_archive_own_bookmarks(self):
other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123')
bookmark = self.setup_bookmark(user=other_user)

response = self.client.post(reverse('bookmarks:action'), {
'archive': [bookmark.id],
})

bookmark.refresh_from_db()

self.assertEqual(response.status_code, 404)
self.assertFalse(bookmark.is_archived)

def test_unarchive_should_unarchive_bookmark(self):
bookmark = self.setup_bookmark(is_archived=True)

self.client.post(reverse('bookmarks:action'), {
'unarchive': [bookmark.id],
})
bookmark.refresh_from_db()

self.assertFalse(bookmark.is_archived)

def test_unarchive_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)

response = self.client.post(reverse('bookmarks:action'), {
'unarchive': [bookmark.id],
})
bookmark.refresh_from_db()

self.assertEqual(response.status_code, 404)
self.assertTrue(bookmark.is_archived)

def test_delete_should_delete_bookmark(self):
bookmark = self.setup_bookmark()

self.client.post(reverse('bookmarks:action'), {
'remove': [bookmark.id],
})

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)

response = self.client.post(reverse('bookmarks:action'), {
'remove': [bookmark.id],
})
self.assertEqual(response.status_code, 404)
self.assertTrue(Bookmark.objects.filter(id=bookmark.id).exists())

def test_bulk_archive(self):
bookmark1 = self.setup_bookmark()
bookmark2 = self.setup_bookmark()
bookmark3 = self.setup_bookmark()

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_archive': [''],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})
Expand All @@ -39,7 +105,7 @@ def test_can_only_bulk_archive_own_bookmarks(self):
bookmark2 = self.setup_bookmark(user=other_user)
bookmark3 = self.setup_bookmark(user=other_user)

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_archive': [''],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})
Expand All @@ -53,7 +119,7 @@ def test_bulk_unarchive(self):
bookmark2 = self.setup_bookmark(is_archived=True)
bookmark3 = self.setup_bookmark(is_archived=True)

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_unarchive': [''],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})
Expand All @@ -68,7 +134,7 @@ def test_can_only_bulk_unarchive_own_bookmarks(self):
bookmark2 = self.setup_bookmark(is_archived=True, user=other_user)
bookmark3 = self.setup_bookmark(is_archived=True, user=other_user)

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_unarchive': [''],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})
Expand All @@ -82,7 +148,7 @@ def test_bulk_delete(self):
bookmark2 = self.setup_bookmark()
bookmark3 = self.setup_bookmark()

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_delete': [''],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})
Expand All @@ -97,7 +163,7 @@ def test_can_only_bulk_delete_own_bookmarks(self):
bookmark2 = self.setup_bookmark(user=other_user)
bookmark3 = self.setup_bookmark(user=other_user)

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_delete': [''],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})
Expand All @@ -113,7 +179,7 @@ def test_bulk_tag(self):
tag1 = self.setup_tag()
tag2 = self.setup_tag()

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_tag': [''],
'bulk_tag_string': [f'{tag1.name} {tag2.name}'],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
Expand All @@ -135,7 +201,7 @@ def test_can_only_bulk_tag_own_bookmarks(self):
tag1 = self.setup_tag()
tag2 = self.setup_tag()

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_tag': [''],
'bulk_tag_string': [f'{tag1.name} {tag2.name}'],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
Expand All @@ -156,7 +222,7 @@ def test_bulk_untag(self):
bookmark2 = self.setup_bookmark(tags=[tag1, tag2])
bookmark3 = self.setup_bookmark(tags=[tag1, tag2])

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_untag': [''],
'bulk_tag_string': [f'{tag1.name} {tag2.name}'],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
Expand All @@ -178,7 +244,7 @@ def test_can_only_bulk_untag_own_bookmarks(self):
bookmark2 = self.setup_bookmark(tags=[tag1, tag2], user=other_user)
bookmark3 = self.setup_bookmark(tags=[tag1, tag2], user=other_user)

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bulk_untag': [''],
'bulk_tag_string': [f'{tag1.name} {tag2.name}'],
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
Expand All @@ -192,17 +258,17 @@ def test_can_only_bulk_untag_own_bookmarks(self):
self.assertCountEqual(bookmark2.tags.all(), [tag1, tag2])
self.assertCountEqual(bookmark3.tags.all(), [tag1, tag2])

def test_bulk_edit_handles_empty_bookmark_id(self):
def test_handles_empty_bookmark_id(self):
bookmark1 = self.setup_bookmark()
bookmark2 = self.setup_bookmark()
bookmark3 = self.setup_bookmark()

response = self.client.post(reverse('bookmarks:bulk_edit'), {
response = self.client.post(reverse('bookmarks:action'), {
'bulk_archive': [''],
})
self.assertEqual(response.status_code, 302)

response = self.client.post(reverse('bookmarks:bulk_edit'), {
response = self.client.post(reverse('bookmarks:action'), {
'bulk_archive': [''],
'bookmark_id': [],
})
Expand All @@ -215,31 +281,31 @@ def test_empty_action_does_not_modify_bookmarks(self):
bookmark2 = self.setup_bookmark()
bookmark3 = self.setup_bookmark()

self.client.post(reverse('bookmarks:bulk_edit'), {
self.client.post(reverse('bookmarks:action'), {
'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)],
})

self.assertBookmarksAreUnmodified([bookmark1, bookmark2, bookmark3])

def test_bulk_edit_should_redirect_to_return_url(self):
def test_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')
url = reverse('bookmarks:action') + '?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):
def test_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'
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)],
Expand Down
55 changes: 0 additions & 55 deletions bookmarks/tests/test_bookmark_archive_view.py
View file

This file was deleted.

0 comments on commit eca98a1

Please sign in to comment.