SECURITY: Stop exposing hidden tag names through category serializers

app/controllers/categories_controller.rb

@@ -546,8 +546,6 @@ class CategoriesController < ApplicationController
           :uploaded_logo_dark,
           :uploaded_background,
           :uploaded_background_dark,
-          :tags,
-          :tag_groups,
           :form_templates,
           category_required_tag_groups: :tag_group,
         )

app/models/site.rb

@@ -120,10 +120,8 @@ class Site
                   :uploaded_logo_dark,
                   :uploaded_background,
                   :uploaded_background_dark,
-                  :tags,
-                  :tag_groups,
+                  :category_required_tag_groups,
                   :form_templates,
-                  category_required_tag_groups: :tag_group,
                 )
                 .joins("LEFT JOIN topics t on t.id = categories.topic_id")
                 .select("categories.*, t.slug topic_slug")

app/serializers/category_serializer.rb

@@ -20,6 +20,8 @@ class CategorySerializer < SiteCategorySerializer
   attributes :locale,
              :read_restricted,
              :available_groups,
+             :allowed_tags,
+             :allowed_tag_groups,
              :auto_close_hours,
              :auto_close_based_on_last_post,
              :group_permissions,
@@ -182,4 +184,20 @@ class CategorySerializer < SiteCategorySerializer
   def available_category_types
     Categories::TypeRegistry.list(only_visible: true, guardian: scope)
   end
+
+  def include_allowed_tags?
+    can_edit_tags?
+  end
+
+  def allowed_tags
+    object.tags.map { |tag| { id: tag.id, name: tag.name, slug: tag.slug } }
+  end
+
+  def include_allowed_tag_groups?
+    can_edit_tags?
+  end
+
+  def allowed_tag_groups
+    object.tag_groups.map(&:name)
+  end
 end

app/serializers/site_category_serializer.rb

@@ -1,15 +1,12 @@
 # frozen_string_literal: true
 
 class SiteCategorySerializer < BasicCategorySerializer
-  attributes :allowed_tags,
-             :allowed_tag_groups,
-             :allow_global_tags,
+  attributes :allow_global_tags,
              :read_only_banner,
              :form_template_ids,
+             :required_tag_groups,
              :category_types
 
-  has_many :category_required_tag_groups, key: :required_tag_groups, embed: :objects
-
   def form_template_ids
     object.form_template_ids.sort
   end
@@ -19,22 +16,6 @@ class SiteCategorySerializer < BasicCategorySerializer
     object.category_types
   end
 
-  def include_allowed_tags?
-    SiteSetting.tagging_enabled
-  end
-
-  def allowed_tags
-    object.tags.pluck(:id, :name, :slug).map { |id, name, slug| { id: id, name: name, slug: slug } }
-  end
-
-  def include_allowed_tag_groups?
-    SiteSetting.tagging_enabled
-  end
-
-  def allowed_tag_groups
-    object.tag_groups.pluck(:name)
-  end
-
   def include_allow_global_tags?
     SiteSetting.tagging_enabled
   end
@@ -43,6 +24,14 @@ class SiteCategorySerializer < BasicCategorySerializer
     SiteSetting.tagging_enabled
   end
 
+  def required_tag_groups
+    object.category_required_tag_groups.map do |crtg|
+      entry = { min_count: crtg.min_count }
+      entry[:name] = crtg.tag_group&.name if can_edit_tags?
+      entry
+    end
+  end
+
   def name
     return I18n.t("uncategorized_category_name") if object.uncategorized?
 
@@ -67,6 +56,11 @@ class SiteCategorySerializer < BasicCategorySerializer
 
   private
 
+  def can_edit_tags?
+    return @can_edit_tags if defined?(@can_edit_tags)
+    @can_edit_tags = SiteSetting.tagging_enabled && scope&.can_edit?(object)
+  end
+
   def localization
     return @localization if defined?(@localization)
     @localization =

app/services/category/action/eager_load_associations.rb

@@ -9,8 +9,6 @@ class Category::Action::EagerLoadAssociations < Service::ActionBase
     :uploaded_logo_dark,
     :uploaded_background,
     :uploaded_background_dark,
-    :tags,
-    :tag_groups,
     :form_templates,
     { category_required_tag_groups: :tag_group },
   ].freeze

spec/requests/api/schemas/json/site_response.json

@@ -792,14 +792,6 @@
           "navigate_to_first_post_after_read": {
             "type": "boolean"
           },
-          "allowed_tags": {
-            "type": "array",
-            "items": {}
-          },
-          "allowed_tag_groups": {
-            "type": "array",
-            "items": {}
-          },
           "allow_global_tags": {
             "type": "boolean"
           },
@@ -809,15 +801,11 @@
               "type": "object",
               "additionalProperties": false,
               "properties": {
-                "name": {
-                  "type": "string"
-                },
                 "min_count": {
                   "type": "integer"
                 }
               },
               "required": [
-                "name",
                 "min_count"
               ]
             }
@@ -901,8 +889,6 @@
           "default_list_filter",
           "minimum_required_tags",
           "navigate_to_first_post_after_read",
-          "allowed_tags",
-          "allowed_tag_groups",
           "allow_global_tags",
           "required_tag_groups",
           "read_only_banner",

spec/requests/categories_controller_spec.rb

@@ -1954,7 +1954,7 @@ RSpec.describe CategoriesController do
 
       queries = track_sql_queries { post "/categories/search.json", params: { term: "Notfoo" } }
 
-      expect(queries.length).to eq(8)
+      expect(queries.length).to eq(6)
 
       expect(response.parsed_body["categories"].length).to eq(1)
       expect(response.parsed_body["categories"][0]["custom_fields"]).to eq("bob" => "marley")

spec/serializers/category_serializer_spec.rb

@@ -358,6 +358,116 @@ RSpec.describe CategorySerializer do
     end
   end
 
+  describe "#allowed_tags" do
+    subject(:json) { described_class.new(category, scope: scope, root: false).as_json }
+
+    fab!(:attached_tag) { Fabricate(:tag, name: "category-allowed-tag") }
+
+    before { category.tags << attached_tag }
+
+    context "for a non-editor" do
+      let(:scope) { user.guardian }
+
+      it "is not included" do
+        expect(json).not_to have_key(:allowed_tags)
+      end
+    end
+
+    context "for an editor" do
+      let(:scope) { admin.guardian }
+
+      it "is included with all tag entries" do
+        expect(json[:allowed_tags]).to contain_exactly(
+          { id: attached_tag.id, name: attached_tag.name, slug: attached_tag.slug },
+        )
+      end
+    end
+
+    context "when tagging is disabled" do
+      let(:scope) { admin.guardian }
+
+      before { SiteSetting.tagging_enabled = false }
+
+      it "is not included" do
+        expect(json).not_to have_key(:allowed_tags)
+      end
+    end
+  end
+
+  describe "#allowed_tag_groups" do
+    subject(:json) { described_class.new(category, scope: scope, root: false).as_json }
+
+    fab!(:attached_tag_group) { Fabricate(:tag_group, name: "category-allowed-group") }
+
+    before { category.tag_groups << attached_tag_group }
+
+    context "for a non-editor" do
+      let(:scope) { user.guardian }
+
+      it "is not included" do
+        expect(json).not_to have_key(:allowed_tag_groups)
+      end
+    end
+
+    context "for an editor" do
+      let(:scope) { admin.guardian }
+
+      it "is included with all tag-group names" do
+        expect(json[:allowed_tag_groups]).to contain_exactly(attached_tag_group.name)
+      end
+    end
+
+    context "when tagging is disabled" do
+      let(:scope) { admin.guardian }
+
+      before { SiteSetting.tagging_enabled = false }
+
+      it "is not included" do
+        expect(json).not_to have_key(:allowed_tag_groups)
+      end
+    end
+  end
+
+  describe "#required_tag_groups" do
+    subject(:json) { described_class.new(category, scope: scope, root: false).as_json }
+
+    fab!(:required_tag_group) { Fabricate(:tag_group, name: "category-required-group") }
+
+    fab!(:category_required_tag_group) do
+      CategoryRequiredTagGroup.create!(
+        category: category,
+        tag_group: required_tag_group,
+        min_count: 2,
+      )
+    end
+
+    context "for a non-editor" do
+      let(:scope) { user.guardian }
+
+      it "omits the tag-group name from each entry" do
+        expect(json[:required_tag_groups]).to eq([{ min_count: 2 }])
+      end
+    end
+
+    context "for an editor" do
+      let(:scope) { admin.guardian }
+
+      it "includes the tag-group name in each entry" do
+        expect(json[:required_tag_groups]).to eq([{ name: required_tag_group.name, min_count: 2 }])
+      end
+    end
+
+    context "when tagging is disabled" do
+      let(:scope) { admin.guardian }
+
+      before { SiteSetting.tagging_enabled = false }
+
+      it "is not included" do
+        expect(json).not_to have_key(:required_tag_groups)
+      end
+    end
+  end
+
   describe "#category_type_settings" do
     let(:type_a) do
       Class.new(Categories::Types::Base) do

spec/serializers/site_category_serializer_spec.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+RSpec.describe SiteCategorySerializer do
+  subject(:json) { described_class.new(category, scope: scope, root: false).as_json }
+
+  fab!(:user)
+  fab!(:admin)
+  fab!(:category)
+
+  describe "#required_tag_groups" do
+    fab!(:required_tag_group) { Fabricate(:tag_group, name: "category-required-group") }
+
+    fab!(:category_required_tag_group) do
+      CategoryRequiredTagGroup.create!(
+        category: category,
+        tag_group: required_tag_group,
+        min_count: 1,
+      )
+    end
+
+    after { Site.clear_cache }
+
+    context "for a non-editor" do
+      let(:scope) { user.guardian }
+
+      it "omits the tag-group name from each entry" do
+        expect(json[:required_tag_groups]).to eq([{ min_count: 1 }])
+      end
+    end
+
+    context "for an editor" do
+      let(:scope) { admin.guardian }
+
+      it "includes the tag-group name in each entry" do
+        expect(json[:required_tag_groups]).to eq([{ name: required_tag_group.name, min_count: 1 }])
+      end
+    end
+
+    context "when tagging is disabled" do
+      let(:scope) { admin.guardian }
+
+      before { SiteSetting.tagging_enabled = false }
+
+      it "is not included" do
+        expect(json).not_to have_key(:required_tag_groups)
+      end
+    end
+  end
+end

spec/serializers/site_serializer_spec.rb

@@ -126,29 +126,7 @@ RSpec.describe SiteSerializer do
     Site.reset_preloaded_category_custom_fields
   end
 
-  it "includes category tags" do
-    tag = Fabricate(:tag)
-    tag_group = Fabricate(:tag_group)
-    tag_group_2 = Fabricate(:tag_group)
-
-    category.tags << tag
-    category.tag_groups << tag_group
-    category.update!(
-      category_required_tag_groups: [
-        CategoryRequiredTagGroup.new(tag_group: tag_group_2, min_count: 1),
-      ],
-    )
-
-    serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json
-    c1 = serialized[:categories].find { |c| c[:id] == category.id }
-
-    expect(c1[:allowed_tags]).to contain_exactly({ id: tag.id, name: tag.name, slug: tag.slug })
-    expect(c1[:allowed_tag_groups]).to contain_exactly(tag_group.name)
-    expect(c1[:required_tag_groups]).to eq([{ name: tag_group_2.name, min_count: 1 }])
-  end
-
   it "doesn't explode when category_required_tag_group is missing" do
-    tag = Fabricate(:tag)
     tag_group = Fabricate(:tag_group)
     crtg = CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)
     category.update!(category_required_tag_groups: [crtg])
@@ -156,9 +134,9 @@ RSpec.describe SiteSerializer do
     tag_group.delete # Bypassing hooks like this should never happen in the app
 
     serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json
-    c1 = serialized[:categories].find { |c| c[:id] == category.id }
+    serialized_category = serialized[:categories].find { |c| c[:id] == category.id }
 
-    expect(c1[:required_tag_groups]).to eq([{ name: nil, min_count: 1 }])
+    expect(serialized_category[:required_tag_groups]).to eq([{ min_count: 1 }])
   end
 
   it "returns correct notification level for categories" do