From 75e40baa64f3b71dd9629d08708a640edfc1bfc8 Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Fri, 8 Jul 2022 12:00:47 -0300 Subject: [PATCH] FIX: min/max username length limits weren't validated (#17382) * FIX: min/max username length limits weren't validated The custom validators introduced in e0d7cda made so we ignored the mix and max values set on site_settings.yml. That change allowed admins to set values outside of the range defined on the yaml file. Related to https://meta.discourse.org/t/group-names-with-more-than-60-characters-broken/232115?u=falco Co-authored-by: Alan Guo Xiang Tan --- config/site_settings.yml | 4 ---- lib/site_settings/yaml_loader.rb | 4 ++++ .../max_username_length_validator.rb | 10 +++++++- .../min_username_length_validator.rb | 10 +++++++- .../max_username_length_validator_spec.rb | 23 +++++++++++++++++-- .../min_username_length_validator_spec.rb | 18 +++++++++++++++ 6 files changed, 61 insertions(+), 8 deletions(-) diff --git a/config/site_settings.yml b/config/site_settings.yml index 0f307b4e77b..a8bea1b00a0 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -542,14 +542,10 @@ users: min_username_length: client: true default: 3 - min: 1 - max: 60 validator: "MinUsernameLengthValidator" max_username_length: client: true default: 20 - min: 8 - max: 60 validator: "MaxUsernameLengthValidator" unicode_usernames: default: false diff --git a/lib/site_settings/yaml_loader.rb b/lib/site_settings/yaml_loader.rb index 9248a407d4b..91603d05cf0 100644 --- a/lib/site_settings/yaml_loader.rb +++ b/lib/site_settings/yaml_loader.rb @@ -19,6 +19,10 @@ class SiteSettings::YamlLoader raise StandardError, "The site setting `#{setting_name}` in '#{@file}' is missing default value." end + if hash.values_at('min', 'max').any? && hash['validator'].present? + raise StandardError, "The site setting `#{setting_name}` in '#{@file}' will have it's min/max validation ignored because there is a validator also specified." + end + yield category, setting_name, value, hash.deep_symbolize_keys! else # Simplest case. site_setting_name: 'default value' diff --git a/lib/validators/max_username_length_validator.rb b/lib/validators/max_username_length_validator.rb index bce5a648d13..0ee1990fe1c 100644 --- a/lib/validators/max_username_length_validator.rb +++ b/lib/validators/max_username_length_validator.rb @@ -1,18 +1,26 @@ # frozen_string_literal: true class MaxUsernameLengthValidator + MAX_USERNAME_LENGTH_RANGE = 8..60 + def initialize(opts = {}) @opts = opts end def valid_value?(value) + if !MAX_USERNAME_LENGTH_RANGE.cover?(value) + @max_range_violation = true + return false + end return false if value < SiteSetting.min_username_length @username = User.where('length(username) > ?', value).pluck_first(:username) @username.blank? end def error_message - if @username.blank? + if @max_range_violation + I18n.t('site_settings.errors.invalid_integer_min_max', min: MAX_USERNAME_LENGTH_RANGE.begin, max: MAX_USERNAME_LENGTH_RANGE.end) + elsif @username.blank? I18n.t("site_settings.errors.max_username_length_range") else I18n.t("site_settings.errors.max_username_length_exists", username: @username) diff --git a/lib/validators/min_username_length_validator.rb b/lib/validators/min_username_length_validator.rb index 0e1fe936a85..cee43b8a4f6 100644 --- a/lib/validators/min_username_length_validator.rb +++ b/lib/validators/min_username_length_validator.rb @@ -1,18 +1,26 @@ # frozen_string_literal: true class MinUsernameLengthValidator + MIN_USERNAME_LENGTH_RANGE = 1..60 + def initialize(opts = {}) @opts = opts end def valid_value?(value) + if !MIN_USERNAME_LENGTH_RANGE.cover?(value) + @min_range_violation = true + return false + end return false if value > SiteSetting.max_username_length @username = User.where('length(username) < ?', value).pluck_first(:username) @username.blank? end def error_message - if @username.blank? + if @min_length_violation + I18n.t('site_settings.errors.invalid_integer_min_max', min: MIN_USERNAME_LENGTH_RANGE.begin, max: MIN_USERNAME_LENGTH_RANGE.end) + elsif @username.blank? I18n.t("site_settings.errors.min_username_length_range") else I18n.t("site_settings.errors.min_username_length_exists", username: @username) diff --git a/spec/lib/validators/max_username_length_validator_spec.rb b/spec/lib/validators/max_username_length_validator_spec.rb index d304fa46616..e0e8065a355 100644 --- a/spec/lib/validators/max_username_length_validator_spec.rb +++ b/spec/lib/validators/max_username_length_validator_spec.rb @@ -2,13 +2,32 @@ describe MaxUsernameLengthValidator do it "checks for minimum range" do - SiteSetting.min_username_length = 6 + User.update_all('username = username || username') + SiteSetting.min_username_length = 9 validator = described_class.new - expect(validator.valid_value?(5)).to eq(false) + expect(validator.valid_value?(8)).to eq(false) expect(validator.error_message).to eq(I18n.t("site_settings.errors.max_username_length_range")) end + context "checks for valid ranges" do + it "fails for values below the valid range" do + expect do + SiteSetting.max_username_length = 5 + end.to raise_error(Discourse::InvalidParameters) + end + it "fails for values above the valid range" do + expect do + SiteSetting.max_username_length = 61 + end.to raise_error(Discourse::InvalidParameters) + end + it "works for values within the valid range" do + expect do + SiteSetting.max_username_length = 42 + end.not_to raise_error + end + end + it "checks for users with short usernames" do user = Fabricate(:user, username: 'jackjackjack') diff --git a/spec/lib/validators/min_username_length_validator_spec.rb b/spec/lib/validators/min_username_length_validator_spec.rb index 489ade75ad7..daae5e7c05d 100644 --- a/spec/lib/validators/min_username_length_validator_spec.rb +++ b/spec/lib/validators/min_username_length_validator_spec.rb @@ -9,6 +9,24 @@ describe MinUsernameLengthValidator do expect(validator.error_message).to eq(I18n.t("site_settings.errors.min_username_length_range")) end + context "checks for valid ranges" do + it "fails for values below the valid range" do + expect do + SiteSetting.min_username_length = 0 + end.to raise_error(Discourse::InvalidParameters) + end + it "fails for values above the valid range" do + expect do + SiteSetting.min_username_length = 61 + end.to raise_error(Discourse::InvalidParameters) + end + it "works for values within the valid range" do + expect do + SiteSetting.min_username_length = 4 + end.not_to raise_error + end + end + it "checks for users with short usernames" do user = Fabricate(:user, username: 'jack')