diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 779ac6ea54c..01ef6bb0728 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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 diff --git a/app/controllers/export_csv_controller.rb b/app/controllers/export_csv_controller.rb index 833bd53b303..a053de20b30 100644 --- a/app/controllers/export_csv_controller.rb +++ b/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") diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 796dcd48bcb..5b6c6f006cc 100644 --- a/app/models/user_history.rb +++ b/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) } diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index 64a6d283a35..039b3eddf22 100644 --- a/app/services/staff_action_logger.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 434dbcd9d7a..3a815f4d86d 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index cf8f68c48c0..8114fe9791e 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/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 diff --git a/spec/requests/export_csv_controller_spec.rb b/spec/requests/export_csv_controller_spec.rb index 861d5274b1b..d55106cb60f 100644 --- a/spec/requests/export_csv_controller_spec.rb +++ b/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 diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index 4a60944f802..a6a2d282500 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/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