CVE-2024-36113: Insufficient Information (NVD-CWE-noinfo)
value: Discourse is an open-source discussion platform. Prior to version 3.2.3 on the `stable` branch, version 3.3.0.beta3 on the `beta` branch, and version 3.3.0.beta4-dev on the `tests-passed` branch, a rogue staff user could suspend other staff users preventing them from logging in to the site. The issue is patched in version 3.2.3 on the `stable` branch, version 3.3.0.beta3 on the `beta` branch, and version 3.3.0.beta4-dev on the `tests-passed` branch. No known workarounds are available.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-07-03T19:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/admin/users_controller.rb, app/services/user_suspender.rb, spec/requests/admin/users_controller_spec.rb, spec/services/user_suspender_spec.rb
Commit date: 2024-07-03 20:49:29 +0800
Commit message: SECURITY: Don't allow suspending staff users via other_user_ids param

Diff: diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 78bbddf9fb2bd..a8e49aaee8656 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -44,11 +44,15 @@ class Admin::UsersController < Admin::StaffController
 
   def show
     @user = User.find_by(id: params[:id])
     raise Discourse::NotFound unless @user
 
-    similar_users = User.real.where.not(id: @user.id).where(ip_address: @user.ip_address)
+    similar_users =
+      User
+        .real
+        .where.not(id: @user.id)
+        .where(ip_address: @user.ip_address, admin: false, moderator: false)
 
     render_serialized(
       @user,
       AdminDetailedUserSerializer,
       root: false,
@@ -142,56 +146,32 @@ class Admin::UsersController < Admin::StaffController
       all_users.uniq!
     end
 
     user_history = nil
 
-    all_users.each do |user|
-      user.suspended_till = params[:suspend_until]
-      user.suspended_at = DateTime.now
-
-      message = params[:message]
-
-      User.transaction do
-        user.save!
-
-        user_history =
-          StaffActionLogger.new(current_user).log_user_suspend(
-            user,
-            params[:reason],
-            message: message,
-            post_id: params[:post_id],
-          )
-      end
-      user.logged_out
+    all_users.each { |user| raise Discourse::InvalidAccess.new if !guardian.can_suspend?(user) }
 
-      if message.present?
-        Jobs.enqueue(
-          :critical_user_email,
-          type: "account_suspended",
-          user_id: user.id,
-          user_history_id: user_history.id,
+    all_users.each do |user|
+      suspender =
+        UserSuspender.new(
+          user,
+          suspended_till: params[:suspend_until],
+          reason: params[:reason],
+          by_user: current_user,
+          message: params[:message],
+          post_id: params[:post_id],
         )
-      end
-
-      DiscourseEvent.trigger(
-        :user_suspended,
-        user: user,
-        reason: params[:reason],
-        message: message,
-        user_history: user_history,
-        post_id: params[:post_id],
-        suspended_till: params[:suspend_until],
-        suspended_at: DateTime.now,
-      )
+      suspender.suspend
+      user_history = suspender.user_history
     end
 
     perform_post_action
 
     render_json_dump(
       suspension: {
         suspend_reason: params[:reason],
-        full_suspend_reason: user_history.try(:details),
+        full_suspend_reason: user_history&.details,
         suspended_till: @user.suspended_till,
         suspended_at: @user.suspended_at,
         suspended_by: BasicUserSerializer.new(current_user, root: false).as_json,
       },
     )
@@ -367,11 +347,10 @@ class Admin::UsersController < Admin::StaffController
     refresh_browser @user
     render json: success_json
   end
 
   def silence
-    guardian.ensure_can_silence_user! @user
     reason = params[:reason]
 
     if reason && (!reason.is_a?(String) || reason.size > 300)
       raise Discourse::InvalidParameters.new(:reason)
     end
@@ -402,10 +381,14 @@ class Admin::UsersController < Admin::StaffController
       all_users.uniq!
     end
 
     user_history = nil
 
+    all_users.each do |user|
+      raise Discourse::InvalidAccess.new if !guardian.can_silence_user?(user)
+    end
+
     all_users.each do |user|
       silencer =
         UserSilencer.new(
           user,
           current_user,
diff --git a/app/services/user_suspender.rb b/app/services/user_suspender.rb
new file mode 100644
index 0000000000000..8488b0dadb731
--- /dev/null
+++ b/app/services/user_suspender.rb
@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+class UserSuspender
+  attr_reader :user_history
+
+  def initialize(user, suspended_till:, reason:, by_user:, message: nil, post_id: nil)
+    @user = user
+    @suspended_till = suspended_till
+    @reason = reason
+    @by_user = by_user
+    @message = message
+    @post_id = post_id
+  end
+
+  def suspend
+    suspended_at = DateTime.now
+
+    @user.suspended_till = @suspended_till
+    @user.suspended_at = suspended_at
+
+    @user.transaction do
+      @user.save!
+
+      @user_history =
+        StaffActionLogger.new(@by_user).log_user_suspend(
+          @user,
+          @reason,
+          message: @message,
+          post_id: @post_id,
+        )
+    end
+    @user.logged_out
+
+    if @message.present?
+      Jobs.enqueue(
+        Jobs::CriticalUserEmail,
+        type: "account_suspended",
+        user_id: @user.id,
+        user_history_id: @user_history.id,
+      )
+    end
+
+    DiscourseEvent.trigger(
+      :user_suspended,
+      user: @user,
+      reason: @reason,
+      message: @message,
+      user_history: @user_history,
+      post_id: @post_id,
+      suspended_till: @suspended_till,
+      suspended_at: suspended_at,
+    )
+    nil
+  end
+end
diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb
index 8114fe9791e0a..ea73e76036f81 100644
--- a/spec/requests/admin/users_controller_spec.rb
+++ b/spec/requests/admin/users_controller_spec.rb
@@ -3,10 +3,11 @@
 require "discourse_ip_info"
 require "rotp"
 
 RSpec.describe Admin::UsersController do
   fab!(:admin)
+  fab!(:another_admin) { Fabricate(:admin) }
   fab!(:moderator)
   fab!(:user)
   fab!(:coding_horror)
 
   describe "#index" do
@@ -106,12 +107,14 @@ RSpec.describe Admin::UsersController do
 
         expect(response.status).to eq(200)
         expect(response.parsed_body["id"]).to eq(user.id)
       end
 
-      it "returns similar users" do
+      it "includes similar users who aren't admin or mods" do
         Fabricate(:user, ip_address: "88.88.88.88")
+        Fabricate(:admin, ip_address: user.ip_address)
+        Fabricate(:moderator, ip_address: user.ip_address)
         similar_user = Fabricate(:user, ip_address: user.ip_address)
 
         get "/admin/users/#{user.id}.json"
 
         expect(response.status).to eq(200)
@@ -276,14 +279,41 @@ RSpec.describe Admin::UsersController do
         log = UserHistory.where(target_user_id: user.id).order("id desc").first
         expect(log.details).to match(/because I said so/)
       end
     end
 
+    shared_examples "suspension of staff users" do
+      it "doesn't allow suspending a staff user" do
+        put "/admin/users/#{another_admin.id}/suspend.json",
+            params: {
+              suspend_until: 5.hours.from_now,
+              reason: "naughty boy",
+            }
+
+        expect(response.status).to eq(403)
+        expect(another_admin.reload).not_to be_suspended
+      end
+
+      it "doesn't allow suspending a staff user via other_user_ids" do
+        put "/admin/users/#{user.id}/suspend.json",
+            params: {
+              suspend_until: 5.hours.from_now,
+              reason: "naughty boy",
+              other_user_ids: [another_admin.id],
+            }
+
+        expect(response.status).to eq(403)
+        expect(user.reload).not_to be_suspended
+        expect(another_admin.reload).not_to be_suspended
+      end
+    end
+
     context "when logged in as an admin" do
       before { sign_in(admin) }
 
       include_examples "suspension of active user possible"
+      include_examples "suspension of staff users"
 
       it "checks if user is suspended" do
         put "/admin/users/#{user.id}/suspend.json",
             params: {
               suspend_until: 5.hours.from_now,
@@ -503,10 +533,11 @@ RSpec.describe Admin::UsersController do
 
     context "when logged in as a moderator" do
       before { sign_in(moderator) }
 
       include_examples "suspension of active user possible"
+      include_examples "suspension of staff users"
     end
 
     context "when logged in as a non-staff user" do
       before { sign_in(user) }
 
@@ -1515,10 +1546,26 @@ RSpec.describe Admin::UsersController do
       it "returns a 404 if the user doesn't exist" do
         put "/admin/users/123123/silence.json"
         expect(response.status).to eq(404)
       end
 
+      it "doesn't allow silencing another admin" do
+        put "/admin/users/#{another_admin.id}/silence.json"
+        expect(response.status).to eq(403)
+        expect(another_admin.reload).to_not be_silenced
+      end
+
+      it "doesn't allow silencing another admin via other_user_ids" do
+        put "/admin/users/#{reg_user.id}/silence.json",
+            params: {
+              other_user_ids: [another_admin.id],
+            }
+        expect(response.status).to eq(403)
+        expect(another_admin.reload).to_not be_silenced
+        expect(reg_user.reload).to_not be_silenced
+      end
+
       it "punishes the user for spamming" do
         put "/admin/users/#{reg_user.id}/silence.json"
         expect(response.status).to eq(200)
         reg_user.reload
         expect(reg_user).to be_silenced
@@ -1631,10 +1678,26 @@ RSpec.describe Admin::UsersController do
         expect(response.status).to eq(200)
         reg_user.reload
         expect(reg_user).to be_silenced
         expect(reg_user.silenced_record).to be_present
       end
+
+      it "doesn't allow silencing another admin" do
+        put "/admin/users/#{another_admin.id}/silence.json"
+        expect(response.status).to eq(403)
+        expect(another_admin.reload).to_not be_silenced
+      end
+
+      it "doesn't allow silencing another admin via other_user_ids" do
+        put "/admin/users/#{reg_user.id}/silence.json",
+            params: {
+              other_user_ids: [another_admin.id],
+            }
+        expect(response.status).to eq(403)
+        expect(another_admin.reload).to_not be_silenced
+        expect(reg_user.reload).to_not be_silenced
+      end
     end
 
     context "when logged in as a non-staff user" do
       before { sign_in(user) }
 
diff --git a/spec/services/user_suspender_spec.rb b/spec/services/user_suspender_spec.rb
new file mode 100644
index 0000000000000..1ca558702150d
--- /dev/null
+++ b/spec/services/user_suspender_spec.rb
@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+RSpec.describe UserSuspender do
+  fab!(:user) { Fabricate(:user, trust_level: 0) }
+  fab!(:post) { Fabricate(:post, user: user) }
+  fab!(:admin)
+
+  describe "suspend" do
+    subject(:suspend_user) { suspender.suspend }
+
+    let(:suspender) do
+      UserSuspender.new(
+        user,
+        suspended_till: 5.hours.from_now,
+        reason: "because",
+        by_user: admin,
+        post_id: post.id,
+        message: "you have been suspended",
+      )
+    end
+
+    it "suspends the user correctly" do
+      freeze_time
+      suspend_user
+      expect(user.reload).to be_suspended
+      expect(user.suspended_till).to be_within_one_second_of(5.hours.from_now)
+      expect(user.suspended_at).to be_within_one_second_of(Time.zone.now)
+    end
+
+    it "creates a staff action log" do
+      expect do suspend_user end.to change {
+        UserHistory.where(
+          action: UserHistory.actions[:suspend_user],
+          acting_user_id: admin.id,
+          target_user_id: user.id,
+        ).count
+      }.from(0).to(1)
+    end
+
+    it "logs the user out" do
+      messages = MessageBus.track_publish("/logout/#{user.id}") { suspend_user }
+      expect(messages.size).to eq(1)
+      expect(messages[0].user_ids).to eq([user.id])
+      expect(messages[0].data).to eq(user.id)
+    end
+
+    it "fires a user_suspended event" do
+      freeze_time
+      events = DiscourseEvent.track_events(:user_suspended) { suspend_user }
+      expect(events.size).to eq(1)
+
+      params = events[0][:params].first
+      expect(params[:user].id).to eq(user.id)
+      expect(params[:reason]).to eq("because")
+      expect(params[:message]).to eq("you have been suspended")
+      expect(params[:suspended_till]).to be_within_one_second_of(5.hours.from_now)
+      expect(params[:suspended_at]).to eq(Time.zone.now)
+    end
+
+    context "when a message is provided" do
+      it "enqueues a critical user email job" do
+        expect do suspend_user end.to change { Jobs::CriticalUserEmail.jobs.size }.from(0).to(1)
+
+        job = Jobs::CriticalUserEmail.jobs.first
+        expect(job["args"].first["user_id"]).to eq(user.id)
+        expect(job["args"].first["user_history_id"]).to eq(suspender.user_history.id)
+      end
+    end
+
+    context "when a message is not provided" do
+      let(:suspender) do
+        UserSuspender.new(
+          user,
+          suspended_till: 5.hours.from_now,
+          reason: "because",
+          by_user: admin,
+          post_id: post.id,
+          message: nil,
+        )
+      end
+
+      it "doesn't enqueue a critical user email job" do
+        expect do suspend_user end.not_to change { Jobs::CriticalUserEmail.jobs.size }.from(0)
+      end
+    end
+  end
+end

Vulnerable code:     similar_users = User.real.where.not(id: @user.id).where(ip_address: @user.ip_address)
    all_users.each do |user|
      user.suspended_till = params[:suspend_until]
      user.suspended_at = DateTime.now

      message = params[:message]

      User.transaction do
        user.save!

        user_history =
          StaffActionLogger.new(current_user).log_user_suspend(
            user,
            params[:reason],
            message: message,
            post_id: params[:post_id],
          )
      end
      user.logged_out
      if message.present?
        Jobs.enqueue(
          :critical_user_email,
          type: "account_suspended",
          user_id: user.id,
          user_history_id: user_history.id,
      end

      DiscourseEvent.trigger(
        :user_suspended,
        user: user,
        reason: params[:reason],
        message: message,
        user_history: user_history,
        post_id: params[:post_id],
        suspended_till: params[:suspend_until],
        suspended_at: DateTime.now,
      )
        full_suspend_reason: user_history.try(:details),
    guardian.ensure_can_silence_user! @user
      it "returns similar users" do
Fixed code:     similar_users =
      User
        .real
        .where.not(id: @user.id)
        .where(ip_address: @user.ip_address, admin: false, moderator: false)
    all_users.each { |user| raise Discourse::InvalidAccess.new if !guardian.can_suspend?(user) }
    all_users.each do |user|
      suspender =
        UserSuspender.new(
          user,
          suspended_till: params[:suspend_until],
          reason: params[:reason],
          by_user: current_user,
          message: params[:message],
          post_id: params[:post_id],
      suspender.suspend
      user_history = suspender.user_history
        full_suspend_reason: user_history&.details,
    all_users.each do |user|
      raise Discourse::InvalidAccess.new if !guardian.can_silence_user?(user)
    end

# frozen_string_literal: true

class UserSuspender
  attr_reader :user_history

  def initialize(user, suspended_till:, reason:, by_user:, message: nil, post_id: nil)
    @user = user
    @suspended_till = suspended_till
    @reason = reason
    @by_user = by_user
    @message = message
    @post_id = post_id
  end

  def suspend
    suspended_at = DateTime.now

    @user.suspended_till = @suspended_till
    @user.suspended_at = suspended_at

    @user.transaction do
      @user.save!

      @user_history =
        StaffActionLogger.new(@by_user).log_user_suspend(
          @user,
          @reason,
          message: @message,
          post_id: @post_id,
        )
    end
    @user.logged_out

    if @message.present?
      Jobs.enqueue(
        Jobs::CriticalUserEmail,
        type: "account_suspended",
        user_id: @user.id,
        user_history_id: @user_history.id,
      )
    end

    DiscourseEvent.trigger(
      :user_suspended,
      user: @user,
      reason: @reason,
      message: @message,
      user_history: @user_history,
      post_id: @post_id,
      suspended_till: @suspended_till,
      suspended_at: suspended_at,
    )
    nil
  end
end
  fab!(:another_admin) { Fabricate(:admin) }
      it "includes similar users who aren't admin or mods" do
        Fabricate(:admin, ip_address: user.ip_address)
        Fabricate(:moderator, ip_address: user.ip_address)
    shared_examples "suspension of staff users" do
      it "doesn't allow suspending a staff user" do
        put "/admin/users/#{another_admin.id}/suspend.json",
            params: {
              suspend_until: 5.hours.from_now,
              reason: "naughty boy",
            }

        expect(response.status).to eq(403)
        expect(another_admin.reload).not_to be_suspended
      end

      it "doesn't allow suspending a staff user via other_user_ids" do
        put "/admin/users/#{user.id}/suspend.json",
            params: {
              suspend_until: 5.hours.from_now,
              reason: "naughty boy",
              other_user_ids: [another_admin.id],
            }

        expect(response.status).to eq(403)
        expect(user.reload).not_to be_suspended
        expect(another_admin.reload).not_to be_suspended
      end
    end

      include_examples "suspension of staff users"
      include_examples "suspension of staff users"
      it "doesn't allow silencing another admin" do
        put "/admin/users/#{another_admin.id}/silence.json"
        expect(response.status).to eq(403)
        expect(another_admin.reload).to_not be_silenced
      end

      it "doesn't allow silencing another admin via other_user_ids" do
        put "/admin/users/#{reg_user.id}/silence.json",
            params: {
              other_user_ids: [another_admin.id],
            }
        expect(response.status).to eq(403)
        expect(another_admin.reload).to_not be_silenced
        expect(reg_user.reload).to_not be_silenced
      end


      it "doesn't allow silencing another admin" do
        put "/admin/users/#{another_admin.id}/silence.json"
        expect(response.status).to eq(403)
        expect(another_admin.reload).to_not be_silenced
      end

      it "doesn't allow silencing another admin via other_user_ids" do
        put "/admin/users/#{reg_user.id}/silence.json",
            params: {
              other_user_ids: [another_admin.id],
            }
        expect(response.status).to eq(403)
        expect(another_admin.reload).to_not be_silenced
        expect(reg_user.reload).to_not be_silenced
      end
# frozen_string_literal: true

RSpec.describe UserSuspender do
  fab!(:user) { Fabricate(:user, trust_level: 0) }
  fab!(:post) { Fabricate(:post, user: user) }
  fab!(:admin)

  describe "suspend" do
    subject(:suspend_user) { suspender.suspend }

    let(:suspender) do
      UserSuspender.new(
        user,
        suspended_till: 5.hours.from_now,
        reason: "because",
        by_user: admin,
        post_id: post.id,
        message: "you have been suspended",
      )
    end

    it "suspends the user correctly" do
      freeze_time
      suspend_user
      expect(user.reload).to be_suspended
      expect(user.suspended_till).to be_within_one_second_of(5.hours.from_now)
      expect(user.suspended_at).to be_within_one_second_of(Time.zone.now)
    end

    it "creates a staff action log" do
      expect do suspend_user end.to change {
        UserHistory.where(
          action: UserHistory.actions[:suspend_user],
          acting_user_id: admin.id,
          target_user_id: user.id,
        ).count
      }.from(0).to(1)
    end

    it "logs the user out" do
      messages = MessageBus.track_publish("/logout/#{user.id}") { suspend_user }
      expect(messages.size).to eq(1)
      expect(messages[0].user_ids).to eq([user.id])
      expect(messages[0].data).to eq(user.id)
    end

    it "fires a user_suspended event" do
      freeze_time
      events = DiscourseEvent.track_events(:user_suspended) { suspend_user }
      expect(events.size).to eq(1)

      params = events[0][:params].first
      expect(params[:user].id).to eq(user.id)
      expect(params[:reason]).to eq("because")
      expect(params[:message]).to eq("you have been suspended")
      expect(params[:suspended_till]).to be_within_one_second_of(5.hours.from_now)
      expect(params[:suspended_at]).to eq(Time.zone.now)
    end

    context "when a message is provided" do
      it "enqueues a critical user email job" do
        expect do suspend_user end.to change { Jobs::CriticalUserEmail.jobs.size }.from(0).to(1)

        job = Jobs::CriticalUserEmail.jobs.first
        expect(job["args"].first["user_id"]).to eq(user.id)
        expect(job["args"].first["user_history_id"]).to eq(suspender.user_history.id)
      end
    end

    context "when a message is not provided" do
      let(:suspender) do
        UserSuspender.new(
          user,
          suspended_till: 5.hours.from_now,
          reason: "because",
          by_user: admin,
          post_id: post.id,
          message: nil,
        )
      end

      it "doesn't enqueue a critical user email job" do
        expect do suspend_user end.not_to change { Jobs::CriticalUserEmail.jobs.size }.from(0)
      end
    end
  end
end