From dfaf9831f7e6f545b25b7ce00db5e0816a1414fb Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 31 Mar 2022 14:39:01 +0800
Subject: [PATCH] SECURITY: Avoid leaking private group name when viewing
 category. (#16337)

In certain instances when viewing a category, the name of a group with
restricted visilbity may be revealed to users which do not have the
required permission.
---
 app/serializers/category_serializer.rb       | 19 ++++--
 spec/serializers/category_serializer_spec.rb | 67 ++++++++++++++++++--
 2 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb
index 6d753562ac3..44382b08349 100644
--- a/app/serializers/category_serializer.rb
+++ b/app/serializers/category_serializer.rb
@@ -33,15 +33,22 @@ class CategorySerializer < SiteCategorySerializer
 
   def group_permissions
     @group_permissions ||= begin
-      perms = object.category_groups.joins(:group).includes(:group).order("groups.name").map do |cg|
-        {
-          permission_type: cg.permission_type,
-          group_name: cg.group.name
-        }
-      end
+      perms = object
+        .category_groups
+        .joins(:group)
+        .includes(:group)
+        .merge(Group.visible_groups(scope&.user))
+        .order("groups.name").map do |cg|
+          {
+            permission_type: cg.permission_type,
+            group_name: cg.group.name
+          }
+        end
+
       if perms.length == 0 && !object.read_restricted
         perms << { permission_type: CategoryGroup.permission_types[:full], group_name: Group[:everyone]&.name.presence || :everyone }
       end
+
       perms
     end
   end
diff --git a/spec/serializers/category_serializer_spec.rb b/spec/serializers/category_serializer_spec.rb
index 2a1196ec463..f54be956dd0 100644
--- a/spec/serializers/category_serializer_spec.rb
+++ b/spec/serializers/category_serializer_spec.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 describe CategorySerializer do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:admin) { Fabricate(:admin) }
   fab!(:group) { Fabricate(:group) }
   fab!(:category) { Fabricate(:category, reviewable_by_group_id: group.id) }
 
@@ -33,8 +35,6 @@ describe CategorySerializer do
   end
 
   describe "user notification level" do
-    fab!(:user) { Fabricate(:user) }
-
     it "includes the user's notification level" do
       CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:watching], category.id)
       json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
@@ -42,10 +42,67 @@ describe CategorySerializer do
     end
   end
 
-  describe "available groups" do
-    fab!(:user) { Fabricate(:user) }
-    fab!(:admin) { Fabricate(:admin) }
+  describe '#group_permissions' do
+    context "category without group permissions configured" do
+      it "returns the right category group permissions for an anon user" do
+        json = described_class.new(category, scope: Guardian.new, root: false).as_json
 
+        expect(json[:group_permissions]).to eq([
+          { permission_type: CategoryGroup.permission_types[:full], group_name: Group[:everyone]&.name }
+        ])
+      end
+    end
+
+    context "category with group permissions configured" do
+      fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff]) }
+
+      fab!(:user_group) do
+        Fabricate(:group, visibility_level: Group.visibility_levels[:members]).tap do |g|
+          g.add(user)
+        end
+      end
+
+      before do
+        category.set_permissions(
+          :everyone => :readonly,
+          group.name => :readonly,
+          user_group.name => :full,
+          private_group.name => :full,
+        )
+
+        category.save!
+      end
+
+      it "returns the right category group permissions for an anon user" do
+        json = described_class.new(category, scope: Guardian.new, root: false).as_json
+
+        expect(json[:group_permissions]).to eq([
+          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
+        ])
+      end
+
+      it "returns the right category group permissions for a regular user" do
+        json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
+
+        expect(json[:group_permissions]).to eq([
+          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
+          { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
+        ])
+      end
+
+      it "returns the right category group permission for a staff user" do
+        json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json
+
+        expect(json[:group_permissions]).to eq([
+          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
+          { permission_type: CategoryGroup.permission_types[:full], group_name: private_group.name },
+          { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name }
+        ])
+      end
+    end
+  end
+
+  describe "available groups" do
     it "not included for a regular user" do
       json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
       expect(json[:available_groups]).to eq(nil)