SECURITY: Prevent large staff actions causing DoS

This commit operates at three levels of abstraction:

 1. We want to prevent user history rows from being unbounded in size.
    This commit adds rails validations to limit the sizes of columns on
    user_histories,

 2. However, we don't want to prevent certain actions from being
    completed if these columns are too long. In those cases, we truncate
    the values that are given and store the truncated versions,

 3. For endpoints that perform staff actions, we can further control
    what is permitted by explicitly validating the params that are given
    before attempting the action,

app/controllers/admin/users_controller.rb

@@ -107,6 +107,11 @@ class Admin::UsersController < Admin::StaffController
 
   def suspend
     guardian.ensure_can_suspend!(@user)
+    reason = params[:reason]
+
+    if reason && (!reason.is_a?(String) || reason.size > 300)
+      raise Discourse::InvalidParameters.new(:reason)
+    end
 
     if @user.suspended?
       suspend_record = @user.suspend_record
@@ -128,6 +133,10 @@ class Admin::UsersController < Admin::StaffController
 
     all_users = [@user]
     if Array === params[:other_user_ids]
+      if params[:other_user_ids].size > MAX_SIMILAR_USERS
+        raise Discourse::InvalidParameters.new(:other_user_ids)
+      end
+
       all_users.concat(User.where(id: params[:other_user_ids]).to_a)
       all_users.uniq!
     end
@@ -360,6 +369,11 @@ class Admin::UsersController < Admin::StaffController
 
   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
 
     if @user.silenced?
       silenced_record = @user.silenced_record
@@ -379,6 +393,10 @@ class Admin::UsersController < Admin::StaffController
 
     all_users = [@user]
     if Array === params[:other_user_ids]
+      if params[:other_user_ids].size > MAX_SIMILAR_USERS
+        raise Discourse::InvalidParameters.new(:other_user_ids)
+      end
+
       all_users.concat(User.where(id: params[:other_user_ids]).to_a)
       all_users.uniq!
     end

app/controllers/export_csv_controller.rb

@@ -5,18 +5,26 @@ class ExportCsvController < ApplicationController
 
   def export_entity
     guardian.ensure_can_export_entity!(export_params[:entity])
+    entity = export_params[:entity]
+    raise Discourse::InvalidParameters.new(:entity) unless entity.is_a?(String) && entity.size < 100
 
-    if export_params[:entity] == "user_archive"
+    (export_params[:args] || {}).each do |key, value|
+      unless value.is_a?(String) && value.size < 100
+        raise Discourse::InvalidParameters.new("args.#{key}")
+      end
+    end
+
+    if entity == "user_archive"
       Jobs.enqueue(:export_user_archive, user_id: current_user.id, args: export_params[:args])
     else
       Jobs.enqueue(
         :export_csv_file,
-        entity: export_params[:entity],
+        entity: entity,
         user_id: current_user.id,
         args: export_params[:args],
       )
     end
-    StaffActionLogger.new(current_user).log_entity_export(export_params[:entity])
+    StaffActionLogger.new(current_user).log_entity_export(entity)
     render json: success_json
   rescue Discourse::InvalidAccess
     render_json_error I18n.t("csv_export.rate_limit_error")

app/models/user_history.rb

@@ -11,6 +11,23 @@ class UserHistory < ActiveRecord::Base
   belongs_to :topic
   belongs_to :category
 
+  # Each value in the context should be shorter than this
+  MAX_CONTEXT_LENGTH = 50_000
+
+  # We often store multiple values in details, particularly during post edits
+  # Let's allow space for 2 values + a little extra for padding
+  MAX_DETAILS_LENGTH = 110_000
+
+  MAX_JSON_LENGTH = 300_000
+
+  validates :details, length: { maximum: MAX_DETAILS_LENGTH }
+  validates :context, length: { maximum: MAX_CONTEXT_LENGTH }
+  validates :subject, length: { maximum: MAX_CONTEXT_LENGTH }
+  validates :ip_address, length: { maximum: MAX_CONTEXT_LENGTH }
+  validates :email, length: { maximum: MAX_CONTEXT_LENGTH }
+  validates :previous_value, length: { maximum: MAX_JSON_LENGTH }
+  validates :new_value, length: { maximum: MAX_JSON_LENGTH }
+
   validates_presence_of :action
 
   scope :only_staff_actions, -> { where("action IN (?)", UserHistory.staff_action_ids) }

app/services/staff_action_logger.rb

@@ -38,7 +38,7 @@ class StaffActionLogger
     StaffActionLogger.base_attrs.each do |attr|
       attrs[attr] = details.delete(attr) if details.has_key?(attr)
     end
-    attrs[:details] = details.map { |r| "#{r[0]}: #{r[1]}" }.join("\n")
+    attrs[:details] = details.map { |r| "#{r[0]}: #{truncate(r[1].to_s)}" }.join("\n")
     attrs[:acting_user_id] = @admin.id
     attrs[:action] = UserHistory.actions[:custom_staff]
     attrs[:custom_type] = custom_type
@@ -80,7 +80,7 @@ class StaffActionLogger
       "user: #{username} (#{name})",
       "topic: #{topic_title}",
       "post_number: #{deleted_post.post_number}",
-      "raw: #{deleted_post.raw}",
+      "raw: #{truncate(deleted_post.raw)}",
     ]
 
     UserHistory.create!(
@@ -105,7 +105,7 @@ class StaffActionLogger
     ]
 
     if first_post = topic.ordered_posts.with_deleted.first
-      details << "raw: #{first_post.raw}"
+      details << "raw: #{truncate(first_post.raw)}"
     end
 
     UserHistory.create!(
@@ -179,7 +179,7 @@ class StaffActionLogger
       params(opts).merge(
         action: UserHistory.actions[:post_edit],
         post_id: post.id,
-        details: "#{opts[:old_raw]}\n\n---\n\n#{post.raw}",
+        details: "#{truncate(opts[:old_raw])}\n\n---\n\n#{truncate(post.raw)}",
       ),
     )
   end
@@ -261,15 +261,12 @@ class StaffActionLogger
     raise Discourse::InvalidParameters.new(:new_theme) unless new_theme
 
     new_json = theme_json(new_theme)
-
     old_json, new_json = strip_duplicates(old_json, new_json)
 
     UserHistory.create!(
-      params(opts).merge(
+      params(opts).merge(json_params(old_json, new_json)).merge(
         action: UserHistory.actions[:change_theme],
         subject: new_theme.name,
-        previous_value: old_json,
-        new_value: new_json,
       ),
     )
   end
@@ -277,10 +274,9 @@ class StaffActionLogger
   def log_theme_destroy(theme, opts = {})
     raise Discourse::InvalidParameters.new(:theme) unless theme
     UserHistory.create!(
-      params(opts).merge(
+      params(opts).merge(json_params(theme_json(theme), nil)).merge(
         action: UserHistory.actions[:delete_theme],
         subject: theme.name,
-        previous_value: theme_json(theme),
       ),
     )
   end
@@ -804,7 +800,7 @@ class StaffActionLogger
       "rejected_at: #{rejected_at}",
       "user: #{username} (#{name})",
       "topic: #{topic_title}",
-      "raw: #{reviewable.payload["raw"]}",
+      "raw: #{truncate(reviewable.payload["raw"])}",
     ]
 
     UserHistory.create!(
@@ -1017,6 +1013,15 @@ class StaffActionLogger
 
   private
 
+  def json_params(previous_value, new_value)
+    if (previous_value && previous_value.length > UserHistory::MAX_JSON_LENGTH) ||
+         (new_value && new_value.length > UserHistory::MAX_JSON_LENGTH)
+      { context: I18n.t("staff_action_logs.json_too_long") }
+    else
+      { previous_value: previous_value, new_value: new_value }
+    end
+  end
+
   def get_changes(changes)
     return unless changes
 
@@ -1046,4 +1051,12 @@ class StaffActionLogger
     urls = section.sidebar_urls.map { |url| "#{url.name} - #{url.value}" }
     "links: #{urls.join(", ")}"
   end
+
+  def truncate(s)
+    if s.size > UserHistory::MAX_CONTEXT_LENGTH
+      "#{s.slice(..UserHistory::MAX_CONTEXT_LENGTH)}..."
+    else
+      s
+    end
+  end
 end

config/locales/server.en.yml

@@ -5320,6 +5320,7 @@ en:
       custom: "Notification from %{username} on %{site_title}"
 
   staff_action_logs:
+    json_too_long: "Values not logged because they exceed the column length limits"
     not_found: "not found"
     unknown: "unknown"
     user_merged: "%{username} was merged into this account"

spec/requests/admin/users_controller_spec.rb

@@ -334,6 +334,18 @@ RSpec.describe Admin::UsersController do
         end
       end
 
+      it "fails the request if the reason is too long" do
+        expect(user).not_to be_suspended
+        put "/admin/users/#{user.id}/suspend.json",
+            params: {
+              reason: "x" * 301,
+              suspend_until: 5.hours.from_now,
+            }
+        expect(response.status).to eq(400)
+        user.reload
+        expect(user).not_to be_suspended
+      end
+
       it "requires suspend_until and reason" do
         expect(user).not_to be_suspended
         put "/admin/users/#{user.id}/suspend.json", params: {}
@@ -1596,6 +1608,18 @@ RSpec.describe Admin::UsersController do
         expect(reg_user.reload).to be_silenced
         expect(other_user.reload).to be_silenced
       end
+
+      it "fails the request if the reason is too long" do
+        expect(user).not_to be_silenced
+        put "/admin/users/#{user.id}/silence.json",
+            params: {
+              reason: "x" * 301,
+              silenced_till: 5.hours.from_now,
+            }
+        expect(response.status).to eq(400)
+        user.reload
+        expect(user).not_to be_suspended
+      end
     end
 
     context "when logged in as a moderator" do

spec/requests/export_csv_controller_spec.rb

@@ -73,6 +73,16 @@ RSpec.describe ExportCsvController do
         expect(log_entry.acting_user_id).to eq(admin.id)
         expect(log_entry.subject).to eq("user_list")
       end
+
+      it "fails requests where the entity is too long" do
+        post "/export_csv/export_entity.json", params: { entity: "x" * 200 }
+        expect(response.status).to eq(400)
+      end
+
+      it "fails requests where the name arg is too long" do
+        post "/export_csv/export_entity.json", params: { entity: "foo", args: { name: "x" * 200 } }
+        expect(response.status).to eq(400)
+      end
     end
   end
 

spec/services/staff_action_logger_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 RSpec.describe StaffActionLogger do
+  let(:long_string) { "Na " * 100_000 + "Batman!" }
   fab!(:admin)
   let(:logger) { described_class.new(admin) }
 
@@ -69,6 +70,13 @@ RSpec.describe StaffActionLogger do
         log_post_deletion
       }.to change { UserHistory.count }.by(1)
     end
+
+    it "truncates overly long values" do
+      deleted_post.update!(raw: long_string, skip_validation: true)
+      expect { log_post_deletion }.to change { UserHistory.count }.by(1)
+      log = UserHistory.last
+      expect(log.details.size).to be_between(50_000, 110_000)
+    end
   end
 
   describe "log_topic_delete_recover" do
@@ -90,6 +98,13 @@ RSpec.describe StaffActionLogger do
       it "creates a new UserHistory record" do
         expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1)
       end
+
+      it "truncates overly long values" do
+        Fabricate(:post, topic: topic, skip_validation: true, raw: long_string)
+        expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1)
+        log = UserHistory.last
+        expect(log.details.size).to be_between(50_000, 110_000)
+      end
     end
 
     context "when recovering topic" do
@@ -112,6 +127,13 @@ RSpec.describe StaffActionLogger do
       it "creates a new UserHistory record" do
         expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1)
       end
+
+      it "truncates overly long values" do
+        Fabricate(:post, topic: topic, skip_validation: true, raw: long_string)
+        expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1)
+        log = UserHistory.last
+        expect(log.details.size).to be_between(50_000, 110_000)
+      end
     end
   end
 
@@ -187,14 +209,12 @@ RSpec.describe StaffActionLogger do
   end
 
   describe "log_theme_change" do
+    fab!(:theme)
+
     it "raises an error when params are invalid" do
       expect { logger.log_theme_change(nil, nil) }.to raise_error(Discourse::InvalidParameters)
     end
 
-    let! :theme do
-      Fabricate(:theme)
-    end
-
     it "logs new site customizations" do
       log_record = logger.log_theme_change(nil, theme)
       expect(log_record.subject).to eq(theme.name)
@@ -226,15 +246,28 @@ RSpec.describe StaffActionLogger do
         ],
       )
     end
+
+    it "doesn't log values when the json is too large" do
+      old_json = ThemeSerializer.new(theme, root: false).to_json
+
+      theme.set_field(target: :common, name: :scss, value: long_string)
+
+      log_record = logger.log_theme_change(old_json, theme)
+
+      expect(log_record.previous_value).not_to be_present
+      expect(log_record.new_value).not_to be_present
+      expect(log_record.context).to be_present
+    end
   end
 
   describe "log_theme_destroy" do
+    fab!(:theme)
+
     it "raises an error when params are invalid" do
       expect { logger.log_theme_destroy(nil) }.to raise_error(Discourse::InvalidParameters)
     end
 
     it "creates a new UserHistory record" do
-      theme = Fabricate(:theme)
       theme.set_field(target: :common, name: :scss, value: "body{margin: 10px;}")
 
       log_record = logger.log_theme_destroy(theme)
@@ -253,6 +286,15 @@ RSpec.describe StaffActionLogger do
         ],
       )
     end
+
+    it "doesn't log values when the json is too large" do
+      theme.set_field(target: :common, name: :scss, value: long_string)
+      log_record = logger.log_theme_destroy(theme)
+
+      expect(log_record.previous_value).not_to be_present
+      expect(log_record.new_value).not_to be_present
+      expect(log_record.context).to be_present
+    end
   end
 
   describe "log_theme_setting_change" do
@@ -398,6 +440,12 @@ RSpec.describe StaffActionLogger do
       expect(logged.custom_type).to eq("clicked_something")
       expect(logged.topic_id).to be === 1234
     end
+
+    it "truncates overly long values" do
+      logged = logger.log_custom(:shower_thought, lyrics: long_string)
+      expect(logged).to be_valid
+      expect(logged.details.size).to be_between(50_000, 110_000)
+    end
   end
 
   describe "log_category_settings_change" do
@@ -654,6 +702,15 @@ RSpec.describe StaffActionLogger do
       expect(user_history.action).to eq(UserHistory.actions[:post_rejected])
       expect(user_history.details).to include(reviewable.payload["raw"])
     end
+
+    it "truncates overly long values" do
+      reviewable.payload["raw"] = long_string
+      reviewable.save!
+
+      expect { log_post_rejected }.to change { UserHistory.count }.by(1)
+      log = UserHistory.last
+      expect(log.details.size).to be_between(50_000, 110_000)
+    end
   end
 
   describe "log_topic_closed" do