From 56b74e60423da67bdb571e422c2b855448f9fc1f Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Tue, 13 Jun 2023 11:07:47 -0600
Subject: [PATCH] SECURITY: Limit ThemeField value length to prevent DoS
 (#22087)

Co-authored-by: Daniel Waterworth <me@danielwaterworth.com>
---
 app/models/theme.rb                           |  9 ++++-
 app/models/theme_field.rb                     |  2 ++
 spec/requests/admin/themes_controller_spec.rb | 36 +++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/app/models/theme.rb b/app/models/theme.rb
index 862f6c71878..a5b2c24bfbb 100644
--- a/app/models/theme.rb
+++ b/app/models/theme.rb
@@ -14,7 +14,7 @@ class Theme < ActiveRecord::Base
 
   belongs_to :user
   belongs_to :color_scheme
-  has_many :theme_fields, dependent: :destroy
+  has_many :theme_fields, dependent: :destroy, validate: false
   has_many :theme_settings, dependent: :destroy
   has_many :theme_translation_overrides, dependent: :destroy
   has_many :child_theme_relation,
@@ -59,6 +59,7 @@ class Theme < ActiveRecord::Base
            class_name: "ThemeField"
 
   validate :component_validations
+  validate :validate_theme_fields
 
   after_create :update_child_components
 
@@ -300,6 +301,12 @@ class Theme < ActiveRecord::Base
     errors.add(:base, I18n.t("themes.errors.component_no_default")) if default?
   end
 
+  def validate_theme_fields
+    theme_fields.each do |field|
+      field.errors.full_messages.each { |message| errors.add(:base, message) } unless field.valid?
+    end
+  end
+
   def switch_to_component!
     return if component
 
diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb
index a9b22b2fb16..1972d2e3734 100644
--- a/app/models/theme_field.rb
+++ b/app/models/theme_field.rb
@@ -5,6 +5,8 @@ class ThemeField < ActiveRecord::Base
   has_one :javascript_cache, dependent: :destroy
   has_one :upload_reference, as: :target, dependent: :destroy
 
+  validates :value, { length: { maximum: 1024**2 } }
+
   after_save do
     if self.type_id == ThemeField.types[:theme_upload_var] && saved_change_to_upload_id?
       UploadReference.ensure_exist!(upload_ids: [self.upload_id], target: self)
diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb
index 7d6e8574610..c2874670b61 100644
--- a/spec/requests/admin/themes_controller_spec.rb
+++ b/spec/requests/admin/themes_controller_spec.rb
@@ -668,6 +668,42 @@ RSpec.describe Admin::ThemesController do
         expect(response.status).to eq(403)
       end
 
+      it "creates new theme fields" do
+        expect(theme.theme_fields.count).to eq(0)
+
+        put "/admin/themes/#{theme.id}.json",
+            params: {
+              theme: {
+                theme_fields: [{ name: "scss", target: "common", value: "test" }],
+              },
+            }
+
+        expect(response.status).to eq(200)
+        theme.reload
+        expect(theme.theme_fields.count).to eq(1)
+        theme_field = theme.theme_fields.first
+        expect(theme_field.name).to eq("scss")
+        expect(theme_field.target_id).to eq(Theme.targets[:common])
+        expect(theme_field.value).to eq("test")
+      end
+
+      it "doesn't create theme fields when they don't pass validation" do
+        expect(theme.theme_fields.count).to eq(0)
+
+        put "/admin/themes/#{theme.id}.json",
+            params: {
+              theme: {
+                theme_fields: [
+                  { name: "scss", target: "common", value: "Na " * 1024**2 + "Batman!" },
+                ],
+              },
+            }
+
+        expect(response.status).to eq(422)
+        json = JSON.parse(response.body)
+        expect(json["errors"].first).to include("Value is too long")
+      end
+
       it "allows zip-imported theme fields to be locally edited" do
         r = RemoteTheme.create!(remote_url: "")
         theme.update!(remote_theme_id: r.id)