diff --git a/app/controllers/api/v1/accounts/contacts_controller.rb b/app/controllers/api/v1/accounts/contacts_controller.rb index 0d8e0ed93..0e024b3d8 100644 --- a/app/controllers/api/v1/accounts/contacts_controller.rb +++ b/app/controllers/api/v1/accounts/contacts_controller.rb @@ -68,6 +68,7 @@ class Api::V1::Accounts::ContactsController < Api::V1::Accounts::BaseController @contacts = fetch_contacts(contacts) rescue CustomExceptions::CustomFilter::InvalidAttribute, CustomExceptions::CustomFilter::InvalidOperator, + CustomExceptions::CustomFilter::InvalidQueryOperator, CustomExceptions::CustomFilter::InvalidValue => e render_could_not_create_error(e.message) end diff --git a/app/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index 2aedf1928..2cd5281ff 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/conversations_controller.rb @@ -46,6 +46,7 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro @conversations_count = result[:count] rescue CustomExceptions::CustomFilter::InvalidAttribute, CustomExceptions::CustomFilter::InvalidOperator, + CustomExceptions::CustomFilter::InvalidQueryOperator, CustomExceptions::CustomFilter::InvalidValue => e render_could_not_create_error(e.message) end diff --git a/app/helpers/filter_helper.rb b/app/helpers/filter_helper.rb index bce2de5ea..9b5cac684 100644 --- a/app/helpers/filter_helper.rb +++ b/app/helpers/filter_helper.rb @@ -81,4 +81,12 @@ module FilterHelper def default_filter(query_hash, filter_operator_value) "#{filter_config[:table_name]}.#{query_hash[:attribute_key]} #{filter_operator_value} #{query_hash[:query_operator]}" end + + def validate_single_condition(condition) + return if condition['query_operator'].nil? + return if condition['query_operator'].empty? + + operator = condition['query_operator'].upcase + raise CustomExceptions::CustomFilter::InvalidQueryOperator.new({}) unless %w[AND OR].include?(operator) + end end diff --git a/app/models/automation_rule.rb b/app/models/automation_rule.rb index 9f7da28a0..258c5c0a3 100644 --- a/app/models/automation_rule.rb +++ b/app/models/automation_rule.rb @@ -27,6 +27,7 @@ class AutomationRule < ApplicationRecord validate :json_conditions_format validate :json_actions_format validate :query_operator_presence + validate :query_operator_value validates :account_id, presence: true after_update_commit :reauthorized!, if: -> { saved_change_to_conditions? } @@ -83,6 +84,24 @@ class AutomationRule < ApplicationRecord operators = conditions.select { |obj, _| obj['query_operator'].nil? } errors.add(:conditions, 'Automation conditions should have query operator.') if operators.length > 1 end + + # This validation ensures logical operators are being used correctly in automation conditions. + # And we don't push any unsanitized query operators to the database. + def query_operator_value + conditions.each do |obj| + validate_single_condition(obj) + end + end + + def validate_single_condition(condition) + query_operator = condition['query_operator'] + + return if query_operator.nil? + return if query_operator.empty? + + operator = query_operator.upcase + errors.add(:conditions, 'Query operator must be either "AND" or "OR"') unless %w[AND OR].include?(operator) + end end AutomationRule.include_mod_with('Audit::AutomationRule') diff --git a/app/services/automation_rules/condition_validation_service.rb b/app/services/automation_rules/condition_validation_service.rb index 63e58b5bc..f1462e667 100644 --- a/app/services/automation_rules/condition_validation_service.rb +++ b/app/services/automation_rules/condition_validation_service.rb @@ -15,7 +15,7 @@ class AutomationRules::ConditionValidationService def perform @rule.conditions.each do |condition| - return false unless valid_condition?(condition) + return false unless valid_condition?(condition) && valid_query_operator?(condition) end true @@ -23,6 +23,15 @@ class AutomationRules::ConditionValidationService private + def valid_query_operator?(condition) + query_operator = condition['query_operator'] + + return true if query_operator.nil? + return true if query_operator.empty? + + %w[AND OR].include?(query_operator.upcase) + end + def valid_condition?(condition) key = condition['attribute_key'] diff --git a/app/services/contacts/filter_service.rb b/app/services/contacts/filter_service.rb index 973dc4b40..7f2d6a0b8 100644 --- a/app/services/contacts/filter_service.rb +++ b/app/services/contacts/filter_service.rb @@ -9,6 +9,7 @@ class Contacts::FilterService < FilterService end def perform + validate_query_operator @contacts = query_builder(@filters['contacts']) { diff --git a/app/services/conversations/filter_service.rb b/app/services/conversations/filter_service.rb index 3a06959b8..d5156b531 100644 --- a/app/services/conversations/filter_service.rb +++ b/app/services/conversations/filter_service.rb @@ -7,6 +7,7 @@ class Conversations::FilterService < FilterService end def perform + validate_query_operator @conversations = query_builder(@filters['conversations']) mine_count, unassigned_count, all_count, = set_count_for_all_conversations assigned_count = all_count - unassigned_count diff --git a/app/services/filter_service.rb b/app/services/filter_service.rb index c545ac7d8..23cc0e188 100644 --- a/app/services/filter_service.rb +++ b/app/services/filter_service.rb @@ -204,4 +204,10 @@ class FilterService end base_relation.where(@query_string, @filter_values.with_indifferent_access) end + + def validate_query_operator + @params[:payload].each do |query_hash| + validate_single_condition(query_hash) + end + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index dd7b07bcf..9ff3fa320 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -80,6 +80,7 @@ en: number_of_records: Limit reached. The maximum number of allowed custom filters for a user per account is 50. invalid_attribute: Invalid attribute key - [%{key}]. The key should be one of [%{allowed_keys}] or a custom attribute defined in the account. invalid_operator: Invalid operator. The allowed operators for %{attribute_name} are [%{allowed_keys}]. + invalid_query_operator: Query operator must be either "AND" or "OR". invalid_value: Invalid value. The values provided for %{attribute_name} are invalid reports: period: Reporting period %{since} to %{until} diff --git a/lib/custom_exceptions/custom_filter.rb b/lib/custom_exceptions/custom_filter.rb index 03ff9ec7a..8093b3612 100644 --- a/lib/custom_exceptions/custom_filter.rb +++ b/lib/custom_exceptions/custom_filter.rb @@ -11,6 +11,12 @@ module CustomExceptions::CustomFilter end end + class InvalidQueryOperator < CustomExceptions::Base + def message + I18n.t('errors.custom_filters.invalid_query_operator') + end + end + class InvalidValue < CustomExceptions::Base def message I18n.t('errors.custom_filters.invalid_value', attribute_name: @data[:attribute_name]) diff --git a/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb b/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb index e9858844a..35ff53b12 100644 --- a/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb @@ -76,6 +76,23 @@ RSpec.describe 'Api::V1::Accounts::AutomationRulesController', type: :request do } end + it 'processes invalid query operator' do + expect(account.automation_rules.count).to eq(0) + params[:conditions] << { + 'attribute_key': 'browser_language', + 'filter_operator': 'equal_to', + 'values': ['en'], + 'query_operator': 'invalid' + } + + post "/api/v1/accounts/#{account.id}/automation_rules", + headers: administrator.create_new_auth_token, + params: params + + expect(response).to have_http_status(:unprocessable_entity) + expect(account.automation_rules.count).to eq(0) + end + it 'throws an error for unknown attributes in condtions' do expect(account.automation_rules.count).to eq(0) params[:conditions] << { diff --git a/spec/services/automation_rules/condition_validation_service_spec.rb b/spec/services/automation_rules/condition_validation_service_spec.rb index c1c380780..36387754a 100644 --- a/spec/services/automation_rules/condition_validation_service_spec.rb +++ b/spec/services/automation_rules/condition_validation_service_spec.rb @@ -46,6 +46,17 @@ RSpec.describe AutomationRules::ConditionValidationService do end end + context 'with wrong query operator' do + before do + rule.conditions = [{ 'values': ['open'], 'attribute_key': 'status', 'query_operator': 'invalid', 'filter_operator': 'attribute_changed' }] + rule.save + end + + it 'returns false' do + expect(described_class.new(rule).perform).to be(false) + end + end + context 'with "attribute_changed" filter operator' do before do rule.conditions = [ diff --git a/spec/services/contacts/filter_service_spec.rb b/spec/services/contacts/filter_service_spec.rb index 483555a0c..22882ae81 100644 --- a/spec/services/contacts/filter_service_spec.rb +++ b/spec/services/contacts/filter_service_spec.rb @@ -146,6 +146,19 @@ describe Contacts::FilterService do expect(result[:contacts].length).to be 1 expect(result[:contacts].first.id).to eq el_contact.id end + + it 'handles invalid query conditions' do + params[:payload] = [ + { + attribute_key: 'labels', + filter_operator: 'is_not_present', + values: [], + query_operator: 'INVALID' + }.with_indifferent_access + ] + + expect { filter_service.new(account, first_user, params).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator) + end end context 'with standard attributes - last_activity_at' do diff --git a/spec/services/conversations/filter_service_spec.rb b/spec/services/conversations/filter_service_spec.rb index 2fbaf5f61..45e5be143 100644 --- a/spec/services/conversations/filter_service_spec.rb +++ b/spec/services/conversations/filter_service_spec.rb @@ -185,6 +185,30 @@ describe Conversations::FilterService do expect(result[:count][:all_count]).to be 2 expect(result[:conversations].pluck(:campaign_id).sort).to eq [campaign_2.id, campaign_1.id].sort end + + it 'handles invalid query conditions' do + params[:payload] = [ + { + attribute_key: 'assignee_id', + filter_operator: 'equal_to', + values: [ + user_1.id, + user_2.id + ], + query_operator: 'INVALID', + custom_attribute_type: '' + }.with_indifferent_access, + { + attribute_key: 'campaign_id', + filter_operator: 'is_present', + values: [], + query_operator: nil, + custom_attribute_type: '' + }.with_indifferent_access + ] + + expect { filter_service.new(params, user_1).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator) + end end end