fix: reject malformed filter payloads (#15398)
Contact and conversation filters now reject malformed numeric/date custom-attribute values and dangling query operators during validation, returning the existing invalid-filter response instead of failing later while building SQL. ## Closes - [CW-7922](https://linear.app/chatwoot/issue/CW-7922/harden-backend-paths-causing-production-sentry-errors) - [Sentry 7663920842](https://chatwoot-p3.sentry.io/issues/7663920842/) - [Sentry 7663378104](https://chatwoot-p3.sentry.io/issues/7663378104/) ## How to reproduce Submit a custom numeric attribute filter with an empty value, or end a filter payload with a nonblank `query_operator`. These inputs previously reached coercion/query construction and raised production exceptions. ## What changed - Validate numeric and date custom-attribute values before query construction. - Preserve presence operators, which do not require a comparison value. - Reject a query operator on the final filter condition. - Add contact and conversation service regression coverage.
This commit is contained in:
@@ -161,7 +161,10 @@ class FilterService
|
||||
when 'date'
|
||||
Date.iso8601(raw_value.to_s)
|
||||
when 'numeric'
|
||||
BigDecimal(raw_value.to_s)
|
||||
decimal = BigDecimal(raw_value.to_s)
|
||||
raise CustomExceptions::CustomFilter::InvalidValue.new(attribute_name: attribute_key) unless decimal.finite?
|
||||
|
||||
decimal
|
||||
else
|
||||
raise CustomExceptions::CustomFilter::InvalidValue.new(attribute_name: attribute_key)
|
||||
end
|
||||
@@ -195,8 +198,11 @@ class FilterService
|
||||
end
|
||||
|
||||
def validate_query_operator
|
||||
@params[:payload].each do |query_hash|
|
||||
@params[:payload].each_with_index do |query_hash, index|
|
||||
validate_single_condition(query_hash)
|
||||
next unless index == @params[:payload].length - 1
|
||||
|
||||
raise CustomExceptions::CustomFilter::InvalidQueryOperator.new({}) if query_hash['query_operator'].present?
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -20,6 +20,7 @@ module Filters::CustomAttributeFilterHelper
|
||||
end
|
||||
|
||||
def build_custom_attr_query(query_hash, current_index)
|
||||
validate_custom_attribute_values!(query_hash)
|
||||
filter_operator_value = filter_operation(query_hash, current_index)
|
||||
query_operator = query_hash[:query_operator]
|
||||
table_name = attribute_model == 'conversation_attribute' ? 'conversations' : 'contacts'
|
||||
@@ -37,6 +38,16 @@ module Filters::CustomAttributeFilterHelper
|
||||
query + not_in_custom_attr_query(table_name, query_hash, attribute_data_type)
|
||||
end
|
||||
|
||||
def validate_custom_attribute_values!(query_hash)
|
||||
return unless @attribute_data_type.in?(%w[date numeric])
|
||||
return if query_hash[:filter_operator].in?(%w[is_present is_not_present])
|
||||
return if @attribute_data_type == 'date' && query_hash[:filter_operator] == 'days_before'
|
||||
|
||||
Array(query_hash[:values]).each do |value|
|
||||
coerce_lt_gt_value(value, @attribute_data_type, @attribute_key)
|
||||
end
|
||||
end
|
||||
|
||||
def custom_attribute(attribute_key, account, custom_attribute_type)
|
||||
current_account = account || Current.account
|
||||
attribute_model = custom_attribute_type.presence || self.class::ATTRIBUTE_MODEL
|
||||
|
||||
@@ -460,6 +460,52 @@ describe Contacts::FilterService do
|
||||
expect(result[:contacts].pluck(:id)).to eq([cs_contact.id])
|
||||
end
|
||||
|
||||
it 'filters custom date attributes by days before' do
|
||||
cs_contact.update!(custom_attributes: cs_contact.custom_attributes.merge('signed_in_at' => (Time.zone.today - 4.days).to_s))
|
||||
el_contact.update!(custom_attributes: el_contact.custom_attributes.merge('signed_in_at' => (Time.zone.today - 2.days).to_s))
|
||||
params[:payload] = [
|
||||
{
|
||||
attribute_key: 'signed_in_at',
|
||||
filter_operator: 'days_before',
|
||||
values: [3],
|
||||
query_operator: nil
|
||||
}.with_indifferent_access
|
||||
]
|
||||
|
||||
result = filter_service.new(account, first_user, params).perform
|
||||
|
||||
expect(result[:contacts].pluck(:id)).to include(cs_contact.id)
|
||||
expect(result[:contacts].pluck(:id)).not_to include(el_contact.id)
|
||||
end
|
||||
|
||||
it 'rejects blank custom numeric values' do
|
||||
params[:payload] = [
|
||||
{
|
||||
attribute_key: 'lifetime_value',
|
||||
values: [''],
|
||||
query_operator: nil
|
||||
}.with_indifferent_access
|
||||
]
|
||||
|
||||
expect { filter_service.new(account, first_user, params).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidValue)
|
||||
end
|
||||
|
||||
it 'rejects non-finite custom numeric values' do
|
||||
%w[Infinity NaN].each do |value|
|
||||
params[:payload] = [
|
||||
{
|
||||
attribute_key: 'lifetime_value',
|
||||
filter_operator: 'is_greater_than',
|
||||
values: [value],
|
||||
query_operator: nil
|
||||
}.with_indifferent_access
|
||||
]
|
||||
|
||||
expect { filter_service.new(account, first_user, params).perform }
|
||||
.to raise_error(CustomExceptions::CustomFilter::InvalidValue)
|
||||
end
|
||||
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] = [
|
||||
|
||||
@@ -420,6 +420,20 @@ describe Conversations::FilterService do
|
||||
|
||||
expect { filter_service.new(params, user_1, account).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator)
|
||||
end
|
||||
|
||||
it 'rejects a query operator on the final condition' do
|
||||
params[:payload] = [
|
||||
{
|
||||
attribute_key: 'status',
|
||||
filter_operator: 'equal_to',
|
||||
values: ['open'],
|
||||
query_operator: 'AND',
|
||||
custom_attribute_type: ''
|
||||
}.with_indifferent_access
|
||||
]
|
||||
|
||||
expect { filter_service.new(params, user_1, account).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -427,6 +441,28 @@ describe Conversations::FilterService do
|
||||
context 'with query present' do
|
||||
let!(:params) { { payload: [], page: 1 } }
|
||||
|
||||
it 'filters custom date attributes by days before' do
|
||||
en_conversation_1.update!(
|
||||
custom_attributes: en_conversation_1.custom_attributes.merge('conversation_created' => (Time.zone.today - 4.days).to_s)
|
||||
)
|
||||
en_conversation_2.update!(
|
||||
custom_attributes: en_conversation_2.custom_attributes.merge('conversation_created' => (Time.zone.today - 2.days).to_s)
|
||||
)
|
||||
params[:payload] = [
|
||||
{
|
||||
attribute_key: 'conversation_created',
|
||||
filter_operator: 'days_before',
|
||||
values: [3],
|
||||
query_operator: nil
|
||||
}.with_indifferent_access
|
||||
]
|
||||
|
||||
result = filter_service.new(params, user_1, account).perform
|
||||
|
||||
expect(result[:conversations].pluck(:id)).to include(en_conversation_1.id)
|
||||
expect(result[:conversations].pluck(:id)).not_to include(en_conversation_2.id)
|
||||
end
|
||||
|
||||
it 'filter by custom_attributes and labels' do
|
||||
user_2_assigned_conversation.update_labels('support')
|
||||
params[:payload] = [
|
||||
|
||||
Reference in New Issue
Block a user