Avoid reviewer privilege escalation vulnerability

chrstinalin · chrstinalin · commit bdbb9e744d9e · 2026-07-03T10:50:00.000-04:00
diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py
@@ -43,10 +43,10 @@
     AllowAddonAuthor,
     AllowAddonOwner,
     AllowIfNotMozillaDisabled,
-    AllowListedViewerOrReviewer,
+    AllowListedViewerOrReviewerReadOnly,
     AllowReadOnlyIfPublic,
     AllowRelatedObjectPermissions,
-    AllowUnlistedViewerOrReviewer,
+    AllowUnlistedViewerOrReviewerReadOnly,
     AnyOf,
     APIGatePermission,
     GroupPermission,
@@ -253,8 +253,8 @@ class AddonViewSet(
         AnyOf(
             AllowReadOnlyIfPublic,
             AllowAddonAuthor,
-            AllowListedViewerOrReviewer,
-            AllowUnlistedViewerOrReviewer,
+            AllowListedViewerOrReviewerReadOnly,
+            AllowUnlistedViewerOrReviewerReadOnly,
         )
     ]
     authentication_classes = [
@@ -578,8 +578,8 @@ def check_permissions(self, request):
             # be add-on author or reviewer.
             self.permission_classes = [
                 AnyOf(
-                    AllowListedViewerOrReviewer,
-                    AllowUnlistedViewerOrReviewer,
+                    AllowListedViewerOrReviewerReadOnly,
+                    AllowUnlistedViewerOrReviewerReadOnly,
                     AllowAddonAuthor,
                 )
             ]
@@ -607,14 +607,16 @@ def check_object_permissions(self, request, obj):
             # authors..
             self.permission_classes = [
                 AllowRelatedObjectPermissions(
-                    'addon', [AnyOf(AllowUnlistedViewerOrReviewer, AllowAddonAuthor)]
+                    'addon',
+                    [AnyOf(AllowUnlistedViewerOrReviewerReadOnly, AllowAddonAuthor)],
                 )
             ]
         elif not obj.is_public():
             # If the instance is disabled, only allow reviewers and authors.
             self.permission_classes = [
                 AllowRelatedObjectPermissions(
-                    'addon', [AnyOf(AllowListedViewerOrReviewer, AllowAddonAuthor)]
+                    'addon',
+                    [AnyOf(AllowListedViewerOrReviewerReadOnly, AllowAddonAuthor)],
                 )
             ]
 
@@ -879,8 +881,8 @@ def check_permissions(self, request):
                     AllowAddonOwner
                     if self.action in self.owner_actions
                     else AllowAddonAuthor,
-                    AllowListedViewerOrReviewer,
-                    AllowUnlistedViewerOrReviewer,
+                    AllowListedViewerOrReviewerReadOnly,
+                    AllowUnlistedViewerOrReviewerReadOnly,
                 ),
             ]
         )
diff --git a/src/olympia/api/permissions.py b/src/olympia/api/permissions.py
@@ -170,14 +170,16 @@ class AllowListedViewerOrReviewer(BasePermission):
     """Allow reviewers to access add-ons with listed versions.
 
     The user logged in must either be making a read-only request and have the
-    'ReviewerTools:View' permission, or simply be a reviewer or admin.
+    'ReviewerTools:View' permission, or simply be a reviewer.
 
     The definition of an add-on reviewer depends on the object:
     - For static themes, it's someone with 'Addons:ThemeReview'
     - For the rest of the add-ons, is someone who has either
       'Addons:Review' or 'Addons:ContentReview' permission.
     """
 
+    disallow_unsafe = False
+
     def has_permission(self, request, view):
         return request.user.is_authenticated
 
@@ -186,13 +188,18 @@ def has_object_permission(self, request, view, obj):
             request.method in SAFE_METHODS
             and acl.action_allowed_for(request.user, permissions.REVIEWER_TOOLS_VIEW)
         )
-        can_access_because_listed_reviewer = obj.has_listed_versions(
-            include_deleted=True
-        ) and acl.is_reviewer(request.user, obj)
-
+        can_access_because_listed_reviewer = (
+            obj.has_listed_versions(include_deleted=True)
+            and acl.is_reviewer(request.user, obj)
+            and (not self.disallow_unsafe or request.method in SAFE_METHODS)
+        )
         return can_access_because_viewer or can_access_because_listed_reviewer
 
 
+class AllowListedViewerOrReviewerReadOnly(AllowListedViewerOrReviewer):
+    disallow_unsafe = True
+
+
 class AllowUnlistedViewerOrReviewer(AllowListedViewerOrReviewer):
     """Allow unlisted reviewers to access add-ons with unlisted versions, or
     add-ons with no listed versions at all.
@@ -219,7 +226,7 @@ def has_object_permission(self, request, view, obj):
         )
         can_access_because_unlisted_reviewer = acl.is_unlisted_addons_reviewer(
             request.user
-        )
+        ) and (not self.disallow_unsafe or request.method in SAFE_METHODS)
         has_unlisted_or_no_listed = obj.has_unlisted_versions(
             include_deleted=True
         ) or not obj.has_listed_versions(include_deleted=True)
@@ -229,6 +236,10 @@ def has_object_permission(self, request, view, obj):
         )
 
 
+class AllowUnlistedViewerOrReviewerReadOnly(AllowUnlistedViewerOrReviewer):
+    disallow_unsafe = True
+
+
 class AllowAnyKindOfReviewer(BasePermission):
     """Allow access to any kind of reviewer. Use only for views that don't
     alter add-on data.
diff --git a/src/olympia/api/tests/test_permissions.py b/src/olympia/api/tests/test_permissions.py
@@ -25,11 +25,13 @@
     AllowIfNotMozillaDisabled,
     AllowIfPublic,
     AllowListedViewerOrReviewer,
+    AllowListedViewerOrReviewerReadOnly,
     AllowNone,
     AllowOwner,
     AllowReadOnlyIfPublic,
     AllowRelatedObjectPermissions,
     AllowUnlistedViewerOrReviewer,
+    AllowUnlistedViewerOrReviewerReadOnly,
     AnyOf,
     ByHttpMethod,
     GroupPermission,
@@ -415,6 +417,67 @@ def test_no_listed_version_reviewer(self):
             assert not self.permission.has_object_permission(request, myview, obj)
 
 
+class TestAllowListedViewerOrReviewerReadOnly(TestAllowListedViewerOrReviewer):
+    """Similar to AllowListedViewerOrReviewer, but restricted to safe methods."""
+
+    def setUp(self):
+        super().setUp()
+        self.permission = AllowListedViewerOrReviewerReadOnly()
+
+    def test_admin(self):
+        user = user_factory()
+        self.grant_permission(user, '*:*')
+        obj = Mock(spec=[])
+        obj.type = amo.ADDON_EXTENSION
+        obj.has_listed_versions = lambda include_deleted=False: True
+
+        for method in self.safe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_permission(request, myview)
+            assert self.permission.has_object_permission(request, myview, obj)
+
+        for method in self.unsafe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_permission(request, myview)
+            assert not self.permission.has_object_permission(request, myview, obj)
+
+    def test_addon_reviewer(self):
+        user = user_factory()
+        self.grant_permission(user, 'Addons:Review')
+        obj = Mock(spec=[])
+        obj.type = amo.ADDON_EXTENSION
+        obj.has_listed_versions = lambda include_deleted=False: True
+
+        for method in self.safe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_object_permission(request, myview, obj)
+
+        for method in self.unsafe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert not self.permission.has_object_permission(request, myview, obj)
+
+    def test_theme_reviewer(self):
+        user = user_factory()
+        self.grant_permission(user, 'Addons:ThemeReview')
+        obj = Mock(spec=[])
+        obj.type = amo.ADDON_STATICTHEME
+        obj.has_listed_versions = lambda include_deleted=False: True
+
+        for method in self.safe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_object_permission(request, myview, obj)
+
+        for method in self.unsafe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert not self.permission.has_object_permission(request, myview, obj)
+
+
 class TestAllowAnyKindOfReviewer(TestCase):
     # Note: be careful when testing, under the hood we're using a method that
     # relies on UserProfile.groups_list, which is cached on the UserProfile
@@ -587,6 +650,47 @@ def test_object_with_no_unlisted_versions_and_no_listed_versions_viewer(self):
         assert self.permission.has_object_permission(self.request, myview, obj)
 
 
+class TestAllowUnlistedViewerOrReviewerReadOnly(TestAllowUnlistedViewerOrReviewer):
+    """Similar to AllowUnlistedViewerOrReviewer, but restricted to safe methods."""
+
+    def setUp(self):
+        super().setUp()
+        self.permission = AllowUnlistedViewerOrReviewerReadOnly()
+
+    def test_admin(self):
+        self.request.user = user_factory()
+        self.grant_permission(self.request.user, '*:*')
+        obj = Mock(spec=[])
+        obj.has_unlisted_versions = lambda include_deleted=False: True
+
+        assert self.permission.has_permission(self.request, myview)
+        assert not self.permission.has_object_permission(self.request, myview, obj)
+        self.request.method = 'GET'
+        assert self.permission.has_object_permission(self.request, myview, obj)
+
+    def test_unlisted_reviewer(self):
+        self.request.user = user_factory()
+        self.grant_permission(self.request.user, 'Addons:ReviewUnlisted')
+        obj = Mock(spec=[])
+        obj.has_unlisted_versions = lambda include_deleted=False: True
+
+        assert self.permission.has_permission(self.request, myview)
+        assert not self.permission.has_object_permission(self.request, myview, obj)
+        self.request.method = 'GET'
+        assert self.permission.has_object_permission(self.request, myview, obj)
+
+    def test_object_with_no_unlisted_versions_and_no_listed_versions(self):
+        self.request.user = user_factory()
+        self.grant_permission(self.request.user, 'Addons:ReviewUnlisted')
+        obj = Mock(spec=[])
+        obj.has_unlisted_versions = lambda include_deleted=False: False
+        obj.has_listed_versions = lambda include_deleted=False: False
+        assert self.permission.has_permission(self.request, myview)
+        assert not self.permission.has_object_permission(self.request, myview, obj)
+        self.request.method = 'GET'
+        assert self.permission.has_object_permission(self.request, myview, obj)
+
+
 class TestAllowIfPublic(TestCase):
     def setUp(self):
         self.permission = AllowIfPublic()
diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py
@@ -46,7 +46,7 @@
 )
 from olympia.api.permissions import (
     AllowAnyKindOfReviewer,
-    AllowUnlistedViewerOrReviewer,
+    AllowUnlistedViewerOrReviewerReadOnly,
     GroupPermission,
 )
 from olympia.constants.abuse import DECISION_ACTIONS
@@ -1057,7 +1057,7 @@ def unsubscribe_listed(self, request, **kwargs):
     @drf_action(
         detail=True,
         methods=['post'],
-        permission_classes=[AllowUnlistedViewerOrReviewer],
+        permission_classes=[AllowUnlistedViewerOrReviewerReadOnly],
     )
     def subscribe_unlisted(self, request, **kwargs):
         addon = get_object_or_404(Addon, pk=kwargs['pk'])
@@ -1069,7 +1069,7 @@ def subscribe_unlisted(self, request, **kwargs):
     @drf_action(
         detail=True,
         methods=['post'],
-        permission_classes=[AllowUnlistedViewerOrReviewer],
+        permission_classes=[AllowUnlistedViewerOrReviewerReadOnly],
     )
     def unsubscribe_unlisted(self, request, **kwargs):
         addon = get_object_or_404(Addon, pk=kwargs['pk'])
