From 5257c80064a68656fbdcc0fc2a26dcebda7e5feb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Mon, 5 Jun 2023 17:38:50 +0200 Subject: [PATCH] DEV: Set limits on custom fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch sets some limits on custom fields: - an entity can’t have more than 100 custom fields defined on it - a custom field can’t hold a value greater than 10,000,000 characters The current implementation of custom fields is relatively complex and does an upsert in SQL at some point, thus preventing to simply add an `ActiveRecord` validation on the custom field model without having to rewrite a part of the existing logic. That’s one of the reasons this patch is implementing validations in the `HasCustomField` module adding them to the model including the module. --- app/models/concerns/has_custom_fields.rb | 35 +++++++++++++++++-- config/locales/server.en.yml | 4 +++ spec/models/category_spec.rb | 2 ++ spec/models/group_spec.rb | 2 ++ spec/models/post_spec.rb | 2 ++ spec/models/topic_spec.rb | 2 ++ spec/models/user_spec.rb | 4 ++- .../shared_examples_for_custom_fields.rb | 27 ++++++++++++++ 8 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 spec/support/shared_examples_for_custom_fields.rb diff --git a/app/models/concerns/has_custom_fields.rb b/app/models/concerns/has_custom_fields.rb index 5330abdd694..1b592733528 100644 --- a/app/models/concerns/has_custom_fields.rb +++ b/app/models/concerns/has_custom_fields.rb @@ -60,12 +60,19 @@ module HasCustomFields end end - included do - has_many :_custom_fields, dependent: :destroy, class_name: "#{name}CustomField" - after_save :save_custom_fields + CUSTOM_FIELDS_MAX_ITEMS = 100 + CUSTOM_FIELDS_MAX_VALUE_LENGTH = 10_000_000 + included do attr_reader :preloaded_custom_fields + has_many :_custom_fields, dependent: :destroy, class_name: "#{name}CustomField" + + validate :custom_fields_max_items, unless: :custom_fields_clean? + validate :custom_fields_value_length, unless: :custom_fields_clean? + + after_save :save_custom_fields + def custom_fields_fk @custom_fields_fk ||= "#{_custom_fields.reflect_on_all_associations(:belongs_to)[0].name}_id" end @@ -133,6 +140,28 @@ module HasCustomFields end end end + + private + + def custom_fields_max_items + if custom_fields.size > CUSTOM_FIELDS_MAX_ITEMS + errors.add( + :base, + I18n.t("custom_fields.validations.max_items", max_items_number: CUSTOM_FIELDS_MAX_ITEMS), + ) + end + end + + def custom_fields_value_length + return if custom_fields.values.all? { _1.to_s.size <= CUSTOM_FIELDS_MAX_VALUE_LENGTH } + errors.add( + :base, + I18n.t( + "custom_fields.validations.max_value_length", + max_value_length: CUSTOM_FIELDS_MAX_VALUE_LENGTH, + ), + ) + end end def reload(options = nil) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 366981d9536..edc1f98d33e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -245,6 +245,10 @@ en: errors: <<: *errors + custom_fields: + validations: + max_items: "Maximum number of custom fields for this entity has been reached (%{max_items_number})" + max_value_length: "Maximum length for a custom field value has been reached (%{max_value_length})" invite: expired: "Your invite token has expired. Please contact staff." not_found: "Your invite token is invalid. Please contact staff." diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index d434bb886a7..d59d7abb984 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -4,6 +4,8 @@ RSpec.describe Category do fab!(:user) { Fabricate(:user) } + it_behaves_like "it has custom fields" + it { is_expected.to validate_presence_of :user_id } it { is_expected.to validate_presence_of :name } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index cc0a09b3922..0db630cd921 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -5,6 +5,8 @@ RSpec.describe Group do let(:user) { Fabricate(:user) } let(:group) { Fabricate(:group) } + it_behaves_like "it has custom fields" + describe "Validations" do it { is_expected.to allow_value("#{"a" * 996}.com").for(:automatic_membership_email_domains) } it do diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 6cbf7e4f19b..66ece7915df 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -7,6 +7,8 @@ RSpec.describe Post do before { Oneboxer.stubs :onebox } + it_behaves_like "it has custom fields" + it { is_expected.to have_many(:reviewables).dependent(:destroy) } describe "#hidden_reasons" do diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 1bc71fa6334..d5af3d60ec6 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -16,6 +16,8 @@ RSpec.describe Topic do Fabricate(:user, trust_level: SiteSetting.min_trust_level_to_allow_invite) end + it_behaves_like "it has custom fields" + describe "Validations" do let(:topic) { Fabricate.build(:topic) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bbb216ad541..fd993516cf8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1,9 +1,11 @@ # frozen_string_literal: true RSpec.describe User do + subject(:user) { Fabricate(:user, last_seen_at: 1.day.ago) } + fab!(:group) { Fabricate(:group) } - subject(:user) { Fabricate(:user, last_seen_at: 1.day.ago) } + it_behaves_like "it has custom fields" def user_error_message(*keys) I18n.t(:"activerecord.errors.models.user.attributes.#{keys.join(".")}") diff --git a/spec/support/shared_examples_for_custom_fields.rb b/spec/support/shared_examples_for_custom_fields.rb new file mode 100644 index 00000000000..c3f98f72d74 --- /dev/null +++ b/spec/support/shared_examples_for_custom_fields.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +RSpec.shared_examples_for "it has custom fields" do + let(:record) { described_class.new } + + describe "Max number of custom fields" do + let(:custom_fields) { (1..101).to_a.product(["value"]).to_h } + + before { record.custom_fields = custom_fields } + + it "can't have more than 100 custom fields" do + expect(record).to be_invalid + expect(record.errors[:base]).to include(/Maximum number.*\(100\)/) + end + end + + describe "Max length of a custom field" do + let(:bad_value) { "a" * 10_000_001 } + + before { record.custom_fields[:my_custom_field] = bad_value } + + it "can't have more than 10,000,000 characters" do + expect(record).to be_invalid + expect(record.errors[:base]).to include(/Maximum length.*\(10000000\)/) + end + end +end