From 385eab6b96bd4478a805f40c5d7b51f635bd5b9c Mon Sep 17 00:00:00 2001
From: Sojan Jose <sojan@pepalo.com>
Date: Fri, 12 May 2023 22:12:21 +0530
Subject: [PATCH] chore: Add max length validation to text fields (#7073)

Introduces a default max length validation for all string and text fields to prevent processing large payloads.
---
 app/models/account.rb            |  2 +-
 app/models/application_record.rb | 27 +++++++++++++++++++++++++++
 app/models/contact.rb            |  1 -
 spec/models/account_spec.rb      | 17 +++++++++++++++++
 spec/models/article_spec.rb      | 31 +++++++++++++++++++++++++++----
 spec/models/message_spec.rb      | 17 +++++++++++++++++
 6 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/app/models/account.rb b/app/models/account.rb
index 331343de1..e5453bfa3 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -34,7 +34,7 @@ class Account < ApplicationRecord
 
   validates :name, presence: true
   validates :auto_resolve_duration, numericality: { greater_than_or_equal_to: 1, less_than_or_equal_to: 999, allow_nil: true }
-  validates :name, length: { maximum: 255 }
+  validates :domain, length: { maximum: 100 }
 
   has_many :account_users, dependent: :destroy_async
   has_many :agent_bot_inboxes, dependent: :destroy_async
diff --git a/app/models/application_record.rb b/app/models/application_record.rb
index ee91205eb..117950e35 100644
--- a/app/models/application_record.rb
+++ b/app/models/application_record.rb
@@ -2,6 +2,8 @@ class ApplicationRecord < ActiveRecord::Base
   include Events::Types
   self.abstract_class = true
 
+  before_validation :validates_column_content_length
+
   # the models that exposed in email templates through liquid
   DROPPABLES = %w[Account Channel Conversation Inbox User Message].freeze
 
@@ -14,6 +16,31 @@ class ApplicationRecord < ActiveRecord::Base
 
   private
 
+  # Generic validation for all columns of type string and text
+  # Validates the length of the column to prevent DOS via large payloads
+  # if a custom length validation is already present, skip the validation
+  def validates_column_content_length
+    self.class.columns.each do |column|
+      check_and_validate_content_length(column) if column_of_type_string_or_text?(column)
+    end
+  end
+
+  def column_of_type_string_or_text?(column)
+    %i[string text].include?(column.type)
+  end
+
+  def check_and_validate_content_length(column)
+    length_validator = self.class.validators_on(column.name).find { |v| v.kind == :length }
+    validate_content_length(column) if length_validator.blank?
+  end
+
+  def validate_content_length(column)
+    max_length = column.type == :text ? 20_000 : 255
+    return if self[column.name].nil? || self[column.name].length <= max_length
+
+    errors.add(column.name.to_sym, "is too long (maximum is #{max_length} characters)")
+  end
+
   def normalize_empty_string_to_nil(attrs = [])
     attrs.each do |attr|
       self[attr] = nil if self[attr].blank?
diff --git a/app/models/contact.rb b/app/models/contact.rb
index 934fbd387..96457996f 100644
--- a/app/models/contact.rb
+++ b/app/models/contact.rb
@@ -36,7 +36,6 @@ class Contact < ApplicationRecord
   validates :phone_number,
             allow_blank: true, uniqueness: { scope: [:account_id] },
             format: { with: /\+[1-9]\d{1,14}\z/, message: I18n.t('errors.contacts.phone_number.invalid') }
-  validates :name, length: { maximum: 255 }
 
   belongs_to :account
   has_many :conversations, dependent: :destroy_async
diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
index 9cd5721ac..86a7e6a5d 100644
--- a/spec/models/account_spec.rb
+++ b/spec/models/account_spec.rb
@@ -23,6 +23,23 @@ RSpec.describe Account do
   it { is_expected.to have_many(:categories).dependent(:destroy_async) }
   it { is_expected.to have_many(:teams).dependent(:destroy_async) }
 
+  # This validation happens in ApplicationRecord
+  describe 'length validations' do
+    let(:account) { create(:account) }
+
+    it 'validates name length' do
+      account.name = 'a' * 256
+      account.valid?
+      expect(account.errors[:name]).to include('is too long (maximum is 255 characters)')
+    end
+
+    it 'validates domain length' do
+      account.domain = 'a' * 150
+      account.valid?
+      expect(account.errors[:domain]).to include('is too long (maximum is 100 characters)')
+    end
+  end
+
   describe 'usage_limits' do
     let(:account) { create(:account) }
 
diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb
index 1520b86a7..11837a1d8 100644
--- a/spec/models/article_spec.rb
+++ b/spec/models/article_spec.rb
@@ -1,6 +1,11 @@
 require 'rails_helper'
 
 RSpec.describe Article, type: :model do
+  let!(:account) { create(:account) }
+  let(:user) { create(:user, account_ids: [account.id], role: :agent) }
+  let!(:portal_1) { create(:portal, account_id: account.id, config: { allowed_locales: %w[en es] }) }
+  let!(:category_1) { create(:category, slug: 'category_1', locale: 'en', portal_id: portal_1.id) }
+
   context 'with validations' do
     it { is_expected.to validate_presence_of(:account_id) }
     it { is_expected.to validate_presence_of(:author_id) }
@@ -13,12 +18,30 @@ RSpec.describe Article, type: :model do
     it { is_expected.to belong_to(:author) }
   end
 
+  # This validation happens in ApplicationRecord
+  describe 'length validations' do
+    let(:article) do
+      create(:article, category_id: category_1.id, content: 'This is the content', description: 'this is the description',
+                       slug: 'this-is-title', title: 'this is title',
+                       portal_id: portal_1.id, author_id: user.id)
+    end
+
+    context 'when it validates content length' do
+      it 'valid when within limit' do
+        article.content = 'a' * 1000
+        expect(article.valid?).to be true
+      end
+
+      it 'invalid when crossed the limit' do
+        article.content = 'a' * 25_001
+        article.valid?
+        expect(article.errors[:content]).to include('is too long (maximum is 20000 characters)')
+      end
+    end
+  end
+
   describe 'search' do
-    let!(:account) { create(:account) }
-    let(:user) { create(:user, account_ids: [account.id], role: :agent) }
-    let!(:portal_1) { create(:portal, account_id: account.id, config: { allowed_locales: %w[en es] }) }
     let!(:portal_2) { create(:portal, account_id: account.id, config: { allowed_locales: %w[en es] }) }
-    let!(:category_1) { create(:category, slug: 'category_1', locale: 'en', portal_id: portal_1.id) }
     let!(:category_2) { create(:category, slug: 'category_2', locale: 'es', portal_id: portal_1.id) }
     let!(:category_3) { create(:category, slug: 'category_3', locale: 'es', portal_id: portal_2.id) }
 
diff --git a/spec/models/message_spec.rb b/spec/models/message_spec.rb
index e3ee1959e..e6dddbe4b 100644
--- a/spec/models/message_spec.rb
+++ b/spec/models/message_spec.rb
@@ -10,6 +10,23 @@ RSpec.describe Message, type: :model do
     it { is_expected.to validate_presence_of(:account_id) }
   end
 
+  describe 'length validations' do
+    let(:message) { create(:message) }
+
+    context 'when it validates name length' do
+      it 'valid when within limit' do
+        message.content = 'a' * 120_000
+        expect(message.valid?).to be true
+      end
+
+      it 'invalid when crossed the limit' do
+        message.content = 'a' * 150_001
+        message.valid?
+        expect(message.errors[:content]).to include('is too long (maximum is 150000 characters)')
+      end
+    end
+  end
+
   describe 'concerns' do
     it_behaves_like 'liqudable'
   end