diff --git a/VERSION_CW b/VERSION_CW index d782fca8f..4f89fb960 100644 --- a/VERSION_CW +++ b/VERSION_CW @@ -1 +1 @@ -4.11.1 +4.11.2 diff --git a/app/helpers/filters/filter_helper.rb b/app/helpers/filters/filter_helper.rb index fe03dae28..4f345676e 100644 --- a/app/helpers/filters/filter_helper.rb +++ b/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]}') " \ - "#{filter_operator_value} #{query_hash[:query_operator]}" + ActiveRecord::Base.sanitize_sql_array( + ["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]}' " \ - "#{filter_operator_value} #{query_hash[:query_operator]} " + ActiveRecord::Base.sanitize_sql_array( + ["#{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) diff --git a/app/models/custom_attribute_definition.rb b/app/models/custom_attribute_definition.rb index 70956c108..a2775ebb7 100644 --- a/app/models/custom_attribute_definition.rb +++ b/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 diff --git a/app/services/filter_service.rb b/app/services/filter_service.rb index e4cef7941..33e4092c8 100644 --- a/app/services/filter_service.rb +++ b/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] - value = query_hash['values'][0] + attribute_data_type = self.class::ATTRIBUTE_TYPES[attribute_type] || standard_attribute_data_type(attribute_key) + + @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] - query_hash['filter_operator'] = 'is_less_than' - lt_gt_filter_values(query_hash) + updated_query_hash = query_hash.with_indifferent_access.merge( + values: [date.strftime], + 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) diff --git a/config/app.yml b/config/app.yml index 4a1b004ac..e4293f20f 100644 --- a/config/app.yml +++ b/config/app.yml @@ -1,5 +1,5 @@ shared: &shared - version: '4.11.1' + version: '4.11.2' development: <<: *shared diff --git a/config/locales/en.yml b/config/locales/en.yml index 07d9b0e2f..0db53492e 100644 --- a/config/locales/en.yml +++ b/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 diff --git a/package.json b/package.json index b51ad5ffa..3191894c5 100644 --- a/package.json +++ b/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}", diff --git a/spec/models/custom_attribute_definition_spec.rb b/spec/models/custom_attribute_definition_spec.rb new file mode 100644 index 000000000..648296b69 --- /dev/null +++ b/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 diff --git a/spec/services/contacts/filter_service_spec.rb b/spec/services/contacts/filter_service_spec.rb index 540f1dfd8..77a543011 100644 --- a/spec/services/contacts/filter_service_spec.rb +++ b/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 diff --git a/spec/services/conversations/filter_service_spec.rb b/spec/services/conversations/filter_service_spec.rb index 7bfa5875d..1bf5c219d 100644 --- a/spec/services/conversations/filter_service_spec.rb +++ b/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] = [ {