CVE-2022-24804: Incorrect Default Permissions (CWE-276)
value: Discourse is an open source platform for community discussion. In stable versions prior to 2.8.3 and beta versions prior 2.9.0.beta4 erroneously expose groups. When a group with restricted visibility has been used to set the permissions of a category, the name of the group is leaked to any user that is able to see the category. To workaround the problem, a site administrator can remove groups with restricted visibility from any category's permissions setting.
CWE description: During installation, installed file permissions are set to allow anyone to modify those files.
CVSS v3 base score: 5.3
Published: 2022-04-11T20:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/assets/javascripts/discourse/app/controllers/share-topic.js, app/assets/javascripts/discourse/app/models/category.js, app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js, app/assets/javascripts/discourse/tests/acceptance/topic-test.js, app/controllers/categories_controller.rb, app/serializers/category_serializer.rb, config/routes.rb, spec/requests/categories_controller_spec.rb, spec/serializers/category_serializer_spec.rb
Commit date: 2022-04-08 13:46:20 +0800
Commit message: SECURITY: Category group permissions leaked to normal users.

After this commit, category group permissions can only be seen by users
that are allowed to manage a category. In the past, we inadvertently
included a category's group permissions settings in `CategoriesController#show`
and `CategoriesController#find_by_slug` endpoints for normal users when
those settings are only a concern to users that can manage a category.

Diff: diff --git a/app/assets/javascripts/discourse/app/controllers/share-topic.js b/app/assets/javascripts/discourse/app/controllers/share-topic.js
index 46a09006ea98..5f9766af2c4d 100644
--- a/app/assets/javascripts/discourse/app/controllers/share-topic.js
+++ b/app/assets/javascripts/discourse/app/controllers/share-topic.js
@@ -36,17 +36,16 @@ export default Controller.extend(
     _showRestrictedGroupWarning() {
       if (!this.model) {
         return;
       }
 
-      Category.reloadBySlugPath(this.model.slug).then((result) => {
-        const groups = result.category.group_permissions.mapBy("group_name");
-        if (groups && !groups.any((group) => group === "everyone")) {
+      Category.fetchVisibleGroups(this.model.id).then((result) => {
+        if (result.groups.length > 0) {
           this.flash(
             I18n.t("topic.share.restricted_groups", {
-              count: groups.length,
-              groupNames: groups.join(", "),
+              count: result.groups.length,
+              groupNames: result.groups.join(", "),
             }),
             "warning"
           );
         }
       });
diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js
index cab4fea6421b..329077f284ba 100644
--- a/app/assets/javascripts/discourse/app/models/category.js
+++ b/app/assets/javascripts/discourse/app/models/category.js
@@ -513,10 +513,14 @@ Category.reopenClass({
     }
 
     return category;
   },
 
+  fetchVisibleGroups(id) {
+    return ajax(`/c/${id}/visible_groups.json`);
+  },
+
   reloadById(id) {
     return ajax(`/c/${id}/show.json`);
   },
 
   reloadBySlugPath(slugPath) {
diff --git a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js
index e028b05ab12d..a22ec248ca85 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js
@@ -12,12 +12,20 @@ import { test } from "qunit";
 
 acceptance("Share and Invite modal", function (needs) {
   needs.user();
 
   needs.pretender((server, helper) => {
-    server.get("/c/feature/find_by_slug.json", () =>
-      helper.response(200, CategoryFixtures["/c/1/show.json"])
+    server.get(`/c/2481/visible_groups.json`, () =>
+      helper.response(200, {
+        groups: ["group_name_1", "group_name_2"],
+      })
+    );
+
+    server.get(`/c/2/visible_groups.json`, () =>
+      helper.response(200, {
+        groups: [],
+      })
     );
   });
 
   test("Topic footer button", async function (assert) {
     await visit("/t/internationalization-localization/280");
@@ -28,10 +36,11 @@ acceptance("Share and Invite modal", function (needs) {
     );
 
     await click("#topic-footer-button-share-and-invite");
 
     assert.ok(exists(".share-topic-modal"), "it shows the modal");
+
     assert.notOk(
       exists("#modal-alert.alert-warning"),
       "it does not show the alert with restricted groups"
     );
 
@@ -71,28 +80,22 @@ acceptance("Share and Invite modal", function (needs) {
       "it shows restricted warning"
     );
     assert.strictEqual(
       query("#modal-alert.alert-warning").innerText,
       I18n.t("topic.share.restricted_groups", {
-        count: 1,
-        groupNames: "moderators",
+        count: 2,
+        groupNames: "group_name_1, group_name_2",
       }),
       "it shows correct restricted group name"
     );
   });
 });
 
 acceptance("Share and Invite modal - mobile", function (needs) {
   needs.user();
   needs.mobileView();
 
-  needs.pretender((server, helper) => {
-    server.get("/c/feature/find_by_slug.json", () =>
-      helper.response(200, CategoryFixtures["/c/1/show.json"])
-    );
-  });
-
   test("Topic footer mobile button", async function (assert) {
     await visit("/t/internationalization-localization/280");
 
     assert.ok(
       !exists("#topic-footer-button-share-and-invite"),
diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js
index d73ebf1fc31e..80970d7a5967 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js
@@ -24,10 +24,16 @@ import { cloneJSON } from "discourse-common/lib/object";
 import CategoryFixtures from "discourse/tests/fixtures/category-fixtures";
 
 acceptance("Topic", function (needs) {
   needs.user();
   needs.pretender((server, helper) => {
+    server.get("/c/2/visible_groups.json", () =>
+      helper.response(200, {
+        groups: [],
+      })
+    );
+
     server.get("/c/feature/find_by_slug.json", () => {
       return helper.response(200, CategoryFixtures["/c/1/show.json"]);
     });
     server.put("/posts/398/wiki", () => {
       return helper.response({});
diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb
index 204c6193d477..163734cf8124 100644
--- a/app/controllers/categories_controller.rb
+++ b/app/controllers/categories_controller.rb
@@ -1,12 +1,12 @@
 # frozen_string_literal: true
 
 class CategoriesController < ApplicationController
 
-  requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug]
+  requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug, :visible_groups]
 
-  before_action :fetch_category, only: [:show, :update, :destroy]
+  before_action :fetch_category, only: [:show, :update, :destroy, :visible_groups]
   before_action :initialize_staff_action_logger, only: [:create, :update, :destroy]
   skip_before_action :check_xhr, only: [:index, :categories_and_latest, :categories_and_top, :redirect]
 
   SYMMETRICAL_CATEGORIES_TO_TOPICS_FACTOR = 1.5
   MIN_CATEGORIES_TOPICS = 5
@@ -118,10 +118,11 @@ class CategoriesController < ApplicationController
     guardian.ensure_can_see!(@category)
 
     if Category.topic_create_allowed(guardian).where(id: @category.id).exists?
       @category.permission = CategoryGroup.permission_types[:full]
     end
+
     render_serialized(@category, CategorySerializer)
   end
 
   def create
     guardian.ensure_can_create!(Category)
@@ -250,10 +251,15 @@ class CategoriesController < ApplicationController
 
     @category.permission = CategoryGroup.permission_types[:full] if Category.topic_create_allowed(guardian).where(id: @category.id).exists?
     render_serialized(@category, CategorySerializer)
   end
 
+  def visible_groups
+    @guardian.ensure_can_see!(@category)
+    render json: success_json.merge(groups: @category.groups.merge(Group.visible_groups(current_user)).pluck("name"))
+  end
+
   private
 
   def self.topics_per_page
     return SiteSetting.categories_topics if SiteSetting.categories_topics > 0
 
@@ -369,10 +375,11 @@ class CategoriesController < ApplicationController
     end
   end
 
   def fetch_category
     @category = Category.find_by_slug(params[:id]) || Category.find_by(id: params[:id].to_i)
+    raise Discourse::NotFound if @category.blank?
   end
 
   def initialize_staff_action_logger
     @staff_action_logger = StaffActionLogger.new(current_user)
   end
diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb
index 7ff76772d4c8..b7ab43b58063 100644
--- a/app/serializers/category_serializer.rb
+++ b/app/serializers/category_serializer.rb
@@ -51,10 +51,14 @@ class CategorySerializer < SiteCategorySerializer
 
       perms
     end
   end
 
+  def include_group_permissions?
+    scope&.can_edit?(object)
+  end
+
   def include_available_groups?
     scope && scope.can_edit?(object)
   end
 
   def available_groups
diff --git a/config/routes.rb b/config/routes.rb
index 75066b99c9b4..acbb0c700cb2 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -720,10 +720,11 @@ Discourse::Application.routes.draw do
 
     get "categories_and_latest" => "categories#categories_and_latest"
     get "categories_and_top" => "categories#categories_and_top"
 
     get "c/:id/show" => "categories#show"
+    get "c/:id/visible_groups" => "categories#visible_groups"
 
     get "c/*category_slug/find_by_slug" => "categories#find_by_slug"
     get "c/*category_slug/edit(/:tab)" => "categories#find_by_slug", constraints: { format: 'html' }
     get "/new-category" => "categories#show", constraints: { format: 'html' }
 
diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb
index 790a7ab27418..f64858e4e486 100644
--- a/spec/requests/categories_controller_spec.rb
+++ b/spec/requests/categories_controller_spec.rb
@@ -731,6 +731,63 @@ describe CategoriesController do
 
         expect(parsed_topic).to be_present
       end
     end
   end
+
+  describe '#visible_groups' do
+    fab!(:public_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:public], name: 'aaa') }
+    fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
+    fab!(:user_only_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc') }
+
+    it 'responds with 404 when id param is invalid' do
+      get "/c/-9999/visible_groups.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    it "responds with 403 when category is restricted to the current user" do
+      category.set_permissions(private_group.name => :full)
+      category.save!
+
+      get "/c/#{category.id}/visible_groups.json"
+
+      expect(response.status).to eq(403)
+    end
+
+    it "returns the names of the groups that are visible to an admin" do
+      sign_in(admin)
+
+      category.set_permissions(
+        "everyone" => :readonly,
+        private_group.name => :full,
+        public_group.name => :full,
+        user_only_group.name => :full,
+      )
+
+      category.save!
+
+      get "/c/#{category.id}/visible_groups.json"
+
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["groups"]).to eq([public_group.name, private_group.name, user_only_group.name])
+    end
+
+    it "returns the names of the groups that are visible to a user and excludes the everyone group" do
+      sign_in(user)
+
+      category.set_permissions(
+        "everyone" => :readonly,
+        private_group.name => :full,
+        public_group.name => :full,
+        user_only_group.name => :full,
+      )
+
+      category.save!
+
+      get "/c/#{category.id}/visible_groups.json"
+
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["groups"]).to eq([public_group.name])
+    end
+  end
 end
diff --git a/spec/serializers/category_serializer_spec.rb b/spec/serializers/category_serializer_spec.rb
index 9d34d31f2cf5..024ae88b58ea 100644
--- a/spec/serializers/category_serializer_spec.rb
+++ b/spec/serializers/category_serializer_spec.rb
@@ -41,80 +41,55 @@ describe CategorySerializer do
       expect(json[:notification_level]).to eq(NotificationLevels.all[:watching])
     end
   end
 
   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
+    fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
 
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:full], group_name: Group[:everyone]&.name }
-        ])
+    fab!(:user_group) do
+      Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g|
+        g.add(user)
       end
     end
 
-    context "category with group permissions configured" do
-      fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
+    before do
+      group.update!(name: 'aaa')
 
-      fab!(:user_group) do
-        Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g|
-          g.add(user)
-        end
-      end
-
-      before do
-        group.update!(name: 'aaa')
-
-        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
+      category.set_permissions(
+        :everyone => :readonly,
+        group.name => :readonly,
+        user_group.name => :full,
+        private_group.name => :full,
+      )
 
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
-        ])
-      end
+      category.save!
+    end
 
-      it "returns the right category group permissions for a regular user ordered by ascending group name" do
-        json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
+    it "does not include the attribute 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 },
-          { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
-        ])
-      end
+      expect(json[:group_permissions]).to eq(nil)
+    end
 
-      it "returns the right category group permission for a staff user ordered by ascending group name" do
-        json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json
+    it "does not include the attribute 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: private_group.name },
-          { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
-        ])
-      end
+      expect(json[:group_permissions]).to eq(nil)
+    end
 
-      it "returns the group permissions for everyone group too" do
-        category.set_permissions(everyone: :readonly)
-        category.save!
+    it "returns the right category group permissions for a user that can edit the category" do
+      SiteSetting.moderators_manage_categories_and_groups = true
+      user.update!(moderator: true)
 
-        json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json
+      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: 'everyone' },
-        ])
-      end
+      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 },
+        { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
+      ])
     end
   end
 
   describe "available groups" do
     it "not included for a regular user" do

Vulnerable code:       Category.reloadBySlugPath(this.model.slug).then((result) => {
        const groups = result.category.group_permissions.mapBy("group_name");
        if (groups && !groups.any((group) => group === "everyone")) {
              count: groups.length,
              groupNames: groups.join(", "),
    server.get("/c/feature/find_by_slug.json", () =>
      helper.response(200, CategoryFixtures["/c/1/show.json"])
        count: 1,
        groupNames: "moderators",
  needs.pretender((server, helper) => {
    server.get("/c/feature/find_by_slug.json", () =>
      helper.response(200, CategoryFixtures["/c/1/show.json"])
    );
  });

  requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug]
  before_action :fetch_category, only: [:show, :update, :destroy]
    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 }
        ])
    context "category with group permissions configured" do
      fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
      fab!(:user_group) do
        Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g|
          g.add(user)
        end
      end

      before do
        group.update!(name: 'aaa')

        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 ordered by ascending group name" 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 ordered by ascending group name" 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 },
          { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
        ])
      end
      it "returns the group permissions for everyone group too" do
        category.set_permissions(everyone: :readonly)
        category.save!
        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: 'everyone' },
        ])
      end
Fixed code:       Category.fetchVisibleGroups(this.model.id).then((result) => {
        if (result.groups.length > 0) {
              count: result.groups.length,
              groupNames: result.groups.join(", "),
  fetchVisibleGroups(id) {
    return ajax(`/c/${id}/visible_groups.json`);
  },

    server.get(`/c/2481/visible_groups.json`, () =>
      helper.response(200, {
        groups: ["group_name_1", "group_name_2"],
      })
    );

    server.get(`/c/2/visible_groups.json`, () =>
      helper.response(200, {
        groups: [],
      })

        count: 2,
        groupNames: "group_name_1, group_name_2",
    server.get("/c/2/visible_groups.json", () =>
      helper.response(200, {
        groups: [],
      })
    );

  requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug, :visible_groups]
  before_action :fetch_category, only: [:show, :update, :destroy, :visible_groups]

  def visible_groups
    @guardian.ensure_can_see!(@category)
    render json: success_json.merge(groups: @category.groups.merge(Group.visible_groups(current_user)).pluck("name"))
  end

    raise Discourse::NotFound if @category.blank?
  def include_group_permissions?
    scope&.can_edit?(object)
  end

    get "c/:id/visible_groups" => "categories#visible_groups"

  describe '#visible_groups' do
    fab!(:public_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:public], name: 'aaa') }
    fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
    fab!(:user_only_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc') }

    it 'responds with 404 when id param is invalid' do
      get "/c/-9999/visible_groups.json"

      expect(response.status).to eq(404)
    end

    it "responds with 403 when category is restricted to the current user" do
      category.set_permissions(private_group.name => :full)
      category.save!

      get "/c/#{category.id}/visible_groups.json"

      expect(response.status).to eq(403)
    end

    it "returns the names of the groups that are visible to an admin" do
      sign_in(admin)

      category.set_permissions(
        "everyone" => :readonly,
        private_group.name => :full,
        public_group.name => :full,
        user_only_group.name => :full,
      )

      category.save!

      get "/c/#{category.id}/visible_groups.json"

      expect(response.status).to eq(200)
      expect(response.parsed_body["groups"]).to eq([public_group.name, private_group.name, user_only_group.name])
    end

    it "returns the names of the groups that are visible to a user and excludes the everyone group" do
      sign_in(user)

      category.set_permissions(
        "everyone" => :readonly,
        private_group.name => :full,
        public_group.name => :full,
        user_only_group.name => :full,
      )

      category.save!

      get "/c/#{category.id}/visible_groups.json"

      expect(response.status).to eq(200)
      expect(response.parsed_body["groups"]).to eq([public_group.name])
    end
  end
    fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
    fab!(:user_group) do
      Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g|
        g.add(user)
    before do
      group.update!(name: 'aaa')
      category.set_permissions(
        :everyone => :readonly,
        group.name => :readonly,
        user_group.name => :full,
        private_group.name => :full,
      )
      category.save!
    end
    it "does not include the attribute for an anon user" do
      json = described_class.new(category, scope: Guardian.new, root: false).as_json
      expect(json[:group_permissions]).to eq(nil)
    end
    it "does not include the attribute for a regular user" do
      json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
      expect(json[:group_permissions]).to eq(nil)
    end
    it "returns the right category group permissions for a user that can edit the category" do
      SiteSetting.moderators_manage_categories_and_groups = true
      user.update!(moderator: true)
      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: private_group.name },
        { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
        { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
      ])