feat: validate query conditions (#10595)

Query conditions can take in arbitrary values, this can cause SQL
errors. This PR fixes it

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

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

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

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')

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']
 

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'])
 
     {

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

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

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}

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])

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] << {

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 = [

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

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