feat: harden filter service

VERSION_CW

@@ -1 +1 @@
-4.11.1
+4.11.2

app/helpers/filters/filter_helper.rb

@@ -47,11 +47,15 @@ module Filters::FilterHelper
 
   def handle_additional_attributes(query_hash, filter_operator_value, data_type)
     if data_type == 'text_case_insensitive'
-      "LOWER(#{filter_config[:table_name]}.additional_attributes ->> '#{query_hash[:attribute_key]}') " \
+      ActiveRecord::Base.sanitize_sql_array(
-        "#{filter_operator_value} #{query_hash[:query_operator]}"
+        ["LOWER(#{filter_config[:table_name]}.additional_attributes ->> ?) #{filter_operator_value} #{query_hash[:query_operator]}",
+         query_hash[:attribute_key]]
+      )
     else
-      "#{filter_config[:table_name]}.additional_attributes ->> '#{query_hash[:attribute_key]}' " \
+      ActiveRecord::Base.sanitize_sql_array(
-        "#{filter_operator_value} #{query_hash[:query_operator]} "
+        ["#{filter_config[:table_name]}.additional_attributes ->> ? #{filter_operator_value} #{query_hash[:query_operator]} ",
+         query_hash[:attribute_key]]
+      )
     end
   end
 
@@ -70,7 +74,7 @@ module Filters::FilterHelper
 
   def date_filter(current_filter, query_hash, filter_operator_value)
     "(#{filter_config[:table_name]}.#{query_hash[:attribute_key]})::#{current_filter['data_type']} " \
-      "#{filter_operator_value}#{current_filter['data_type']} #{query_hash[:query_operator]}"
+      "#{filter_operator_value} #{query_hash[:query_operator]}"
   end
 
   def text_case_insensitive_filter(query_hash, filter_operator_value)

app/models/custom_attribute_definition.rb

@@ -30,10 +30,12 @@ class CustomAttributeDefinition < ApplicationRecord
 
   scope :with_attribute_model, ->(attribute_model) { attribute_model.presence && where(attribute_model: attribute_model) }
   validates :attribute_display_name, presence: true
+  before_validation :normalize_attribute_fields
 
   validates :attribute_key,
             presence: true,
-            uniqueness: { scope: [:account_id, :attribute_model] }
+            uniqueness: { scope: [:account_id, :attribute_model] },
+            format: { with: /\A[\p{L}\p{N}_.\-]+\z/, message: I18n.t('errors.custom_attribute_definition.attribute_key_format') }
 
   validates :attribute_display_type, presence: true
   validates :attribute_model, presence: true
@@ -48,6 +50,11 @@ class CustomAttributeDefinition < ApplicationRecord
 
   private
 
+  def normalize_attribute_fields
+    self.attribute_key = attribute_key.strip if attribute_key.present?
+    self.attribute_display_name = attribute_display_name.strip if attribute_display_name.present?
+  end
+
   def sync_widget_pre_chat_custom_fields
     ::Inboxes::SyncWidgetPreChatCustomFieldsJob.perform_later(account, attribute_key)
   end

app/services/filter_service.rb

@@ -33,9 +33,9 @@ class FilterService
     when 'is_not_present'
       @filter_values["value_#{current_index}"] = 'IS NULL'
     when 'is_greater_than', 'is_less_than'
-      @filter_values["value_#{current_index}"] = lt_gt_filter_values(query_hash)
+      lt_gt_filter_query(query_hash, current_index)
     when 'days_before'
-      @filter_values["value_#{current_index}"] = days_before_filter_values(query_hash)
+      days_before_filter_query(query_hash, current_index)
     else
       @filter_values["value_#{current_index}"] = filter_values(query_hash).to_s
       "= :value_#{current_index}"
@@ -81,21 +81,29 @@ class FilterService
     query_hash['values'].downcase
   end
 
-  def lt_gt_filter_values(query_hash)
+  def lt_gt_filter_query(query_hash, current_index)
     attribute_key = query_hash[:attribute_key]
     attribute_model = query_hash['custom_attribute_type'].presence || self.class::ATTRIBUTE_MODEL
     attribute_type = custom_attribute(attribute_key, @account, attribute_model).try(:attribute_display_type)
-    attribute_data_type = self.class::ATTRIBUTE_TYPES[attribute_type]
+    attribute_data_type = self.class::ATTRIBUTE_TYPES[attribute_type] || standard_attribute_data_type(attribute_key)
-    value = query_hash['values'][0]
+
+    @filter_values["value_#{current_index}"] = coerce_lt_gt_value(
+      query_hash['values'][0],
+      attribute_data_type,
+      attribute_key
+    )
     operator = query_hash['filter_operator'] == 'is_less_than' ? '<' : '>'
-    "#{operator} '#{value}'::#{attribute_data_type}"
+    "#{operator} :value_#{current_index}"
   end
 
-  def days_before_filter_values(query_hash)
+  def days_before_filter_query(query_hash, current_index)
     date = Time.zone.today - query_hash['values'][0].to_i.days
-    query_hash['values'] = [date.strftime]
+    updated_query_hash = query_hash.with_indifferent_access.merge(
-    query_hash['filter_operator'] = 'is_less_than'
+      values: [date.strftime],
-    lt_gt_filter_values(query_hash)
+      filter_operator: 'is_less_than'
+    )
+
+    lt_gt_filter_query(updated_query_hash, current_index)
   end
 
   def set_count_for_all_conversations
@@ -149,15 +157,39 @@ class FilterService
     @attribute_data_type = self.class::ATTRIBUTE_TYPES[attribute_type]
   end
 
+  def standard_attribute_data_type(attribute_key)
+    @filters.each_value do |section|
+      return section.dig(attribute_key, 'data_type') if section.is_a?(Hash) && section.key?(attribute_key)
+    end
+    nil
+  end
+
+  def coerce_lt_gt_value(raw_value, attribute_data_type, attribute_key)
+    case attribute_data_type
+    when 'date'
+      Date.iso8601(raw_value.to_s)
+    when 'numeric'
+      BigDecimal(raw_value.to_s)
+    else
+      raise CustomExceptions::CustomFilter::InvalidValue.new(attribute_name: attribute_key)
+    end
+  rescue Date::Error, ArgumentError, FloatDomainError, TypeError
+    raise CustomExceptions::CustomFilter::InvalidValue.new(attribute_name: attribute_key)
+  end
+
   def build_custom_attr_query(query_hash, current_index)
     filter_operator_value = filter_operation(query_hash, current_index)
     query_operator = query_hash[:query_operator]
     table_name = attribute_model == 'conversation_attribute' ? 'conversations' : 'contacts'
 
     query = if attribute_data_type == 'text'
-              "LOWER(#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} #{filter_operator_value} #{query_operator} "
+              ActiveRecord::Base.sanitize_sql_array(
+                ["LOWER(#{table_name}.custom_attributes ->> ?)::#{attribute_data_type} #{filter_operator_value} #{query_operator} ", @attribute_key]
+              )
             else
-              "(#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} #{filter_operator_value} #{query_operator} "
+              ActiveRecord::Base.sanitize_sql_array(
+                ["(#{table_name}.custom_attributes ->> ?)::#{attribute_data_type} #{filter_operator_value} #{query_operator} ", @attribute_key]
+              )
             end
 
     query + not_in_custom_attr_query(table_name, query_hash, attribute_data_type)
@@ -174,7 +206,9 @@ class FilterService
   def not_in_custom_attr_query(table_name, query_hash, attribute_data_type)
     return '' unless query_hash[:filter_operator] == 'not_equal_to'
 
-    " OR (#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} IS NULL "
+    ActiveRecord::Base.sanitize_sql_array(
+      [" OR (#{table_name}.custom_attributes ->> ?)::#{attribute_data_type} IS NULL ", @attribute_key]
+    )
   end
 
   def equals_to_filter_string(filter_operator, current_index)

config/app.yml

@@ -1,5 +1,5 @@
 shared: &shared
-  version: '4.11.1'
+  version: '4.11.2'
 
 development:
   <<: *shared

config/locales/en.yml

@@ -117,6 +117,7 @@ en:
       invalid_query_operator: Query operator must be either "AND" or "OR".
       invalid_value: Invalid value. The values provided for %{attribute_name} are invalid
     custom_attribute_definition:
+      attribute_key_format: must only contain letters, numbers, underscores, hyphens, and dots
       key_conflict: The provided key is not allowed as it might conflict with default attributes.
     mfa:
       already_enabled: MFA is already enabled

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@chatwoot/chatwoot",
-  "version": "4.11.1",
+  "version": "4.11.2",
   "license": "MIT",
   "scripts": {
     "eslint": "eslint app/**/*.{js,vue}",

spec/models/custom_attribute_definition_spec.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe CustomAttributeDefinition do
+  let(:account) { create(:account) }
+
+  describe 'validations' do
+    describe 'attribute_key format' do
+      it 'allows alphanumeric keys with underscores' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: 'order_date_1')
+        expect(cad).to be_valid
+      end
+
+      it 'allows hyphens and dots' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: 'order-date.v2')
+        expect(cad).to be_valid
+      end
+
+      it 'allows Unicode letters' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: '客户类型')
+        expect(cad).to be_valid
+      end
+
+      it 'rejects keys with single quotes' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: "x'||(SELECT 1)||'")
+        expect(cad).not_to be_valid
+        expect(cad.errors[:attribute_key]).to be_present
+      end
+
+      it 'rejects keys with spaces' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: 'order date')
+        expect(cad).not_to be_valid
+      end
+
+      it 'rejects keys with semicolons' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: 'key; DROP TABLE users--')
+        expect(cad).not_to be_valid
+      end
+
+      it 'rejects keys with parentheses' do
+        cad = build(:custom_attribute_definition, account: account, attribute_key: 'key()')
+        expect(cad).not_to be_valid
+      end
+    end
+  end
+
+  describe 'callbacks' do
+    describe '#strip_attribute_key' do
+      it 'strips leading and trailing whitespace from attribute_key' do
+        cad = create(:custom_attribute_definition, account: account, attribute_key: '  order_date  ')
+        expect(cad.attribute_key).to eq('order_date')
+      end
+
+      it 'strips leading and trailing whitespace from attribute_display_name' do
+        cad = create(:custom_attribute_definition, account: account, attribute_display_name: '  Order Date  ')
+        expect(cad.attribute_display_name).to eq('Order Date')
+      end
+    end
+  end
+end

spec/services/contacts/filter_service_spec.rb

@@ -49,6 +49,11 @@ describe Contacts::FilterService do
            account: account,
            attribute_model: 'contact_attribute',
            attribute_display_type: 'date')
+    create(:custom_attribute_definition,
+           attribute_key: 'lifetime_value',
+           account: account,
+           attribute_model: 'contact_attribute',
+           attribute_display_type: 'number')
   end
 
   describe '#perform' do
@@ -60,7 +65,7 @@ describe Contacts::FilterService do
 
       en_contact.update!(custom_attributes: { contact_additional_information: 'test custom data' })
       el_contact.update!(custom_attributes: { contact_additional_information: 'test custom data', customer_type: 'platinum' })
-      cs_contact.update!(custom_attributes: { customer_type: 'platinum', signed_in_at: '2022-01-19' })
+      cs_contact.update!(custom_attributes: { customer_type: 'platinum', signed_in_at: '2022-01-19', lifetime_value: '120.50' })
     end
 
     context 'with standard attributes - name' do
@@ -272,6 +277,39 @@ describe Contacts::FilterService do
         expect(result[:contacts].pluck(:id)).to include(cs_contact.id)
         expect(result[:contacts].pluck(:id)).not_to include(en_contact.id)
       end
+
+      it 'binds last_activity_at comparison values as dates' do
+        date_value = '2024-01-01'
+        params[:payload] = [
+          {
+            attribute_key: 'last_activity_at',
+            filter_operator: 'is_greater_than',
+            values: [date_value],
+            query_operator: nil
+          }.with_indifferent_access
+        ]
+
+        service = filter_service.new(account, first_user, params)
+        filters = service.instance_variable_get(:@filters)['contacts']
+        condition_query = service.send(:build_condition_query, filters, params[:payload].first, 0)
+
+        expect(condition_query).to include('(contacts.last_activity_at)::date > :value_0')
+        expect(service.instance_variable_get(:@filter_values)['value_0']).to eq(Date.iso8601(date_value))
+      end
+
+      it 'rejects invalid last_activity_at comparison values' do
+        malicious_value = "2024-01-01'::date OR (SELECT pg_sleep(5)) IS NOT NULL --"
+        params[:payload] = [
+          {
+            attribute_key: 'last_activity_at',
+            filter_operator: 'is_greater_than',
+            values: [malicious_value],
+            query_operator: nil
+          }.with_indifferent_access
+        ]
+
+        expect { filter_service.new(account, first_user, params).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidValue)
+      end
     end
 
     context 'with additional attributes' do
@@ -369,6 +407,72 @@ describe Contacts::FilterService do
         expect(result[:contacts].length).to be expected_count
         expect(result[:contacts].pluck(:id)).to include(el_contact.id)
       end
+
+      it 'binds custom date comparison values as dates' do
+        date_value = '2024-01-01'
+        params[:payload] = [
+          {
+            attribute_key: 'signed_in_at',
+            filter_operator: 'is_less_than',
+            values: [date_value],
+            query_operator: nil
+          }.with_indifferent_access
+        ]
+
+        service = filter_service.new(account, first_user, params)
+        filters = service.instance_variable_get(:@filters)['contacts']
+        condition_query = service.send(:build_condition_query, filters, params[:payload].first, 0)
+
+        expect(condition_query).to include("(contacts.custom_attributes ->> 'signed_in_at')::date < :value_0")
+        expect(service.instance_variable_get(:@filter_values)['value_0']).to eq(Date.iso8601(date_value))
+      end
+
+      it 'binds custom numeric comparison values as decimals' do
+        params[:payload] = [
+          {
+            attribute_key: 'lifetime_value',
+            filter_operator: 'is_greater_than',
+            values: ['100.25'],
+            query_operator: nil
+          }.with_indifferent_access
+        ]
+
+        service = filter_service.new(account, first_user, params)
+        filters = service.instance_variable_get(:@filters)['contacts']
+        condition_query = service.send(:build_condition_query, filters, params[:payload].first, 0)
+
+        expect(condition_query).to include("(contacts.custom_attributes ->> 'lifetime_value')::numeric > :value_0")
+        expect(service.instance_variable_get(:@filter_values)['value_0']).to eq(BigDecimal('100.25'))
+      end
+
+      it 'filters by custom numeric attributes' do
+        params[:payload] = [
+          {
+            attribute_key: 'lifetime_value',
+            filter_operator: 'is_greater_than',
+            values: ['100.25'],
+            query_operator: nil
+          }.with_indifferent_access
+        ]
+
+        result = filter_service.new(account, first_user, params).perform
+
+        expect(result[:contacts].pluck(:id)).to eq([cs_contact.id])
+      end
+
+      it 'rejects invalid custom date comparison values' do
+        malicious_value = "2024-01-01'::date OR (SELECT pg_sleep(5)) IS NOT NULL --"
+        params[:payload] = [
+          {
+            attribute_key: 'signed_in_at',
+            filter_operator: 'is_less_than',
+            values: [malicious_value],
+            query_operator: nil
+          }.with_indifferent_access
+        ]
+
+        expect { filter_service.new(account, first_user, params).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidValue)
+      end
     end
   end
 end

spec/services/conversations/filter_service_spec.rb

@@ -417,6 +417,41 @@ describe Conversations::FilterService do
         expect(result[:conversations].length).to be expected_count
       end
 
+      it 'binds created_at comparison values as dates' do
+        date_value = '2024-01-01'
+        params[:payload] = [
+          {
+            attribute_key: 'created_at',
+            filter_operator: 'is_greater_than',
+            values: [date_value],
+            query_operator: nil,
+            custom_attribute_type: ''
+          }.with_indifferent_access
+        ]
+
+        service = filter_service.new(params, user_1, account)
+        filters = service.instance_variable_get(:@filters)['conversations']
+        condition_query = service.send(:build_condition_query, filters, params[:payload].first, 0)
+
+        expect(condition_query).to include('(conversations.created_at)::date > :value_0')
+        expect(service.instance_variable_get(:@filter_values)['value_0']).to eq(Date.iso8601(date_value))
+      end
+
+      it 'rejects invalid created_at comparison values' do
+        malicious_value = "2024-01-01'::date OR (SELECT pg_sleep(5)) IS NOT NULL --"
+        params[:payload] = [
+          {
+            attribute_key: 'created_at',
+            filter_operator: 'is_greater_than',
+            values: [malicious_value],
+            query_operator: nil,
+            custom_attribute_type: ''
+          }.with_indifferent_access
+        ]
+
+        expect { filter_service.new(params, user_1, account).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidValue)
+      end
+
       it 'filter by created_at and conversation_type' do
         params[:payload] = [
           {