From e8deed874b889bf3f27e2829d56e3d8c00d65bcf Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 3 Jan 2024 16:55:28 +1000
Subject: [PATCH] FIX: Do not allow setting admin and staff for
 TrustLevelSetting (#25107)

This fixes an issue where any string for an enum site setting
(such as TrustLevelSetting) would be converted to an integer
if the default value for the enum was an integer. This is an
issue because things like "admin" and "staff" would get silently
converted to 0 which is "valid" because it's TrustLevel[0],
but it's unexpected behaviour. It's best to just let the site
setting validator catch this broken value.
---
 lib/site_settings/type_supervisor.rb    |  9 +++++++-
 spec/models/trust_level_setting_spec.rb | 29 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/lib/site_settings/type_supervisor.rb b/lib/site_settings/type_supervisor.rb
index e2550df9f34..f1fc1a493f5 100644
--- a/lib/site_settings/type_supervisor.rb
+++ b/lib/site_settings/type_supervisor.rb
@@ -231,7 +231,14 @@ class SiteSettings::TypeSupervisor
     elsif type == self.class.types[:null] && val != ""
       type = get_data_type(name, val)
     elsif type == self.class.types[:enum]
-      val = @defaults_provider[name].is_a?(Integer) ? val.to_i : val.to_s
+      val =
+        (
+          if @defaults_provider[name].is_a?(Integer) && Integer(val, exception: false)
+            val.to_i
+          else
+            val.to_s
+          end
+        )
     elsif type == self.class.types[:uploaded_image_list] && val.present?
       val = val.is_a?(String) ? val : val.map(&:id).join("|")
     elsif type == self.class.types[:upload] && val.present?
diff --git a/spec/models/trust_level_setting_spec.rb b/spec/models/trust_level_setting_spec.rb
index 6a91368ce86..160eb8ca53b 100644
--- a/spec/models/trust_level_setting_spec.rb
+++ b/spec/models/trust_level_setting_spec.rb
@@ -14,4 +14,33 @@ RSpec.describe TrustLevelSetting do
       expect(value[:value]).to eq(0)
     end
   end
+
+  describe ".valid_value?" do
+    let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" }
+
+    before { SiteSetting.load_settings(deprecated_test) }
+
+    it "allows all trust levels as valid values" do
+      expect(TrustLevelSetting.valid_value?(TrustLevel[0])).to eq(true)
+      expect(TrustLevelSetting.valid_value?(TrustLevel[1])).to eq(true)
+      expect(TrustLevelSetting.valid_value?(TrustLevel[2])).to eq(true)
+      expect(TrustLevelSetting.valid_value?(TrustLevel[3])).to eq(true)
+      expect(TrustLevelSetting.valid_value?(TrustLevel[4])).to eq(true)
+      expect(TrustLevelSetting.valid_value?(20)).to eq(false)
+    end
+
+    it "does not allow 'admin' or 'staff' as valid values" do
+      expect(TrustLevelSetting.valid_value?("admin")).to eq(false)
+      expect(TrustLevelSetting.valid_value?("staff")).to eq(false)
+    end
+
+    it "does not allow setting 'admin' or 'staff' as valid values" do
+      expect { SiteSetting.min_trust_level_to_allow_invite = "admin" }.to raise_error(
+        Discourse::InvalidParameters,
+      )
+      expect { SiteSetting.min_trust_level_to_allow_invite = "staff" }.to raise_error(
+        Discourse::InvalidParameters,
+      )
+    end
+  end
 end