From 088efbf0259f19aefd3fafd37f6842344900c339 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 10 Apr 2026 16:59:16 +0200 Subject: [PATCH] Don't allow developers to download attachments on private comments --- src/olympia/activity/tests/test_views.py | 30 ++++++++++++++++++------ src/olympia/activity/urls.py | 2 +- src/olympia/activity/views.py | 16 ++++++++----- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/olympia/activity/tests/test_views.py b/src/olympia/activity/tests/test_views.py index 7a235cf1045c..eb88523887ff 100644 --- a/src/olympia/activity/tests/test_views.py +++ b/src/olympia/activity/tests/test_views.py @@ -700,24 +700,40 @@ def setUp(self): file=ContentFile('Pseudo File', name='attachment.txt'), ) AddonLog.objects.create(addon=self.addon, activity_log=self.log) + self.url = reverse('activity.attachment', args=[self.log.pk]) def test_download_attachment_developer(self): self.client.force_login(self.user) - url = reverse('activity.attachment', args=[self.log.pk]) - response = self.client.get(url, follow=True) + response = self.client.get(self.url, follow=True) self.assertEqual(response.status_code, 404) - response = self.client.get(url, follow=True) + response = self.client.get(self.url, follow=True) self.addon.authors.add(self.user) - response = self.client.get(url, follow=True) + response = self.client.get(self.url, follow=True) self.assertEqual(response.status_code, 200) self.assertIn('.txt', response['Content-Disposition']) def test_download_attachment_reviewer(self): self.client.force_login(self.user) - url = reverse('activity.attachment', args=[self.log.pk]) - response = self.client.get(url, follow=True) + response = self.client.get(self.url, follow=True) self.assertEqual(response.status_code, 404) self.grant_permission(self.user, 'Addons:Review', 'Addon Reviewers') - response = self.client.get(url, follow=True) + response = self.client.get(self.url, follow=True) self.assertEqual(response.status_code, 200) self.assertIn('.txt', response['Content-Disposition']) + + def test_download_attachment_reviewer_private_comment(self): + self.log.update(action=amo.LOG.REVIEWER_PRIVATE_COMMENT.id) + self.client.force_login(self.user) + response = self.client.get(self.url, follow=True) + self.assertEqual(response.status_code, 404) + self.grant_permission(self.user, 'Addons:Review', 'Addon Reviewers') + response = self.client.get(self.url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertIn('.txt', response['Content-Disposition']) + + def test_download_attachment_developer_private_comment(self): + self.log.update(action=amo.LOG.REVIEWER_PRIVATE_COMMENT.id) + self.client.force_login(self.user) + self.addon.authors.add(self.user) + response = self.client.get(self.url, follow=True) + self.assertEqual(response.status_code, 404) diff --git a/src/olympia/activity/urls.py b/src/olympia/activity/urls.py index 8191bd22ab51..dfe78eae5215 100644 --- a/src/olympia/activity/urls.py +++ b/src/olympia/activity/urls.py @@ -5,7 +5,7 @@ urlpatterns = [ re_path( - r'^attachment/(?P\d+)', + r'^attachment/(?P\d+)', views.download_attachment, name='activity.attachment', ) diff --git a/src/olympia/activity/views.py b/src/olympia/activity/views.py index dd880b34f8fc..99043b6e129d 100644 --- a/src/olympia/activity/views.py +++ b/src/olympia/activity/views.py @@ -177,13 +177,13 @@ def check_content_length(request): @non_atomic_requests -def download_attachment(request, log_id): +def download_attachment(request, activity_log_id): """ Download attachment for a given activity log. """ - log = get_object_or_404(ActivityLog, pk=log_id) - addon = get_object_or_404(AddonLog, activity_log=log).addon - attachmentlog = log.attachmentlog + activity = get_object_or_404(ActivityLog, pk=activity_log_id) + addon = get_object_or_404(AddonLog, activity_log=activity).addon + attachmentlog = activity.attachmentlog is_reviewer = acl.is_user_any_kind_of_reviewer(request.user, allow_viewers=True) is_developer = acl.check_addon_ownership( @@ -191,8 +191,12 @@ def download_attachment(request, log_id): addon, allow_developer=True, ) - - if not (is_reviewer or is_developer): + permission_required = ( + is_reviewer + if getattr(activity.log, 'hide_developer', False) + else is_reviewer or is_developer + ) + if not permission_required: raise http.Http404() response = HttpResponseXSendFile(request, attachmentlog.file.path)