diff --git a/app/models/columns/champ_column.rb b/app/models/columns/champ_column.rb index fa83db9c6..c032b3a35 100644 --- a/app/models/columns/champ_column.rb +++ b/app/models/columns/champ_column.rb @@ -32,15 +32,16 @@ class Columns::ChampColumn < Column end def filtered_ids(dossiers, search_terms) + relation = dossiers.with_type_de_champ(stable_id) + if type == :enum - dossiers.with_type_de_champ(stable_id) - .filter_enum(:champs, column, search_terms).ids + relation.where(champs: { column => search_terms }).ids elsif type == :enums - dossiers.with_type_de_champ(stable_id) - .filter_array_enum(:champs, column, search_terms).ids + # in a multiple drop down list, the value are stored as '["v1", "v2"]' + quoted_search_terms = search_terms.map { %{"#{_1}"} } + relation.filter_ilike(:champs, column, quoted_search_terms).ids else - dossiers.with_type_de_champ(stable_id) - .filter_ilike(:champs, column, search_terms).ids + relation.filter_ilike(:champs, column, search_terms).ids end end diff --git a/app/models/columns/linked_drop_down_column.rb b/app/models/columns/linked_drop_down_column.rb index f750c1fa8..1d25d3eea 100644 --- a/app/models/columns/linked_drop_down_column.rb +++ b/app/models/columns/linked_drop_down_column.rb @@ -17,10 +17,28 @@ class Columns::LinkedDropDownColumn < Columns::ChampColumn ) end - def filtered_ids(dossiers, values) - dossiers.with_type_de_champ(@column) - .filter_ilike(:champs, :value, values) - .ids + def filtered_ids(dossiers, search_terms) + relation = dossiers.with_type_de_champ(@stable_id) + + case path + when :value + search_terms.flat_map do |search_term| + # when looking for "section 1 / option A", + # the value must contain both "section 1" and "option A" + primary, *secondary = search_term.split(%r{[[:space:]]*/[[:space:]]*}) + safe_terms = [primary, *secondary].map { "%#{safe_like(_1)}%" } + + relation.where("champs.value ILIKE ALL (ARRAY[?])", safe_terms).ids + end.uniq + when :primary + primary_terms = search_terms.map { |term| %{["#{safe_like(term)}","%"]} } + + relation.where("champs.value ILIKE ANY (array[?])", primary_terms).ids + when :secondary + secondary_terms = search_terms.map { |term| %{["%","#{safe_like(term)}"]} } + + relation.where("champs.value ILIKE ANY (array[?])", secondary_terms).ids + end end private @@ -44,4 +62,6 @@ class Columns::LinkedDropDownColumn < Columns::ChampColumn rescue JSON::ParserError [] end + + def safe_like(q) = ActiveRecord::Base.sanitize_sql_like(q) end diff --git a/app/models/concerns/dossier_filtering_concern.rb b/app/models/concerns/dossier_filtering_concern.rb index ac040f3f5..7883ae700 100644 --- a/app/models/concerns/dossier_filtering_concern.rb +++ b/app/models/concerns/dossier_filtering_concern.rb @@ -28,22 +28,13 @@ module DossierFilteringConcern end } - scope :filter_ilike, lambda { |table, column, values| + scope :filter_ilike, lambda { |table, column, search_terms| + safe_quoted_terms = search_terms.map { "%#{sanitize_sql_like(_1)}%" } table_column = DossierFilterService.sanitized_column(table, column) - q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') - where(q, *(values.map { |value| "%#{value}%" })) + + where("#{table_column} LIKE ANY (ARRAY[?])", safe_quoted_terms) } - scope :filter_enum, lambda { |table, column, values| - table_column = DossierFilterService.sanitized_column(table, column) - q = Array.new(values.count, "(#{table_column} = ?)").join(' OR ') - where(q, *(values)) - } - - scope :filter_array_enum, lambda { |table, column, values| - table_column = DossierFilterService.sanitized_column(table, column) - q = Array.new(values.count, "(#{table_column} = ?)").join(' OR ') - where(q, *(values. map { |value| "[\"#{value}\"]" })) - } + def sanitize_sql_like(q) = ActiveRecord::Base.sanitize_sql_like(q) end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 78ea26d4c..d064270c1 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -44,29 +44,6 @@ ], "note": "" }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "5092b33433aef8fe42b688a780325f3791a77b39e55131256c78cebc3c14c0a3", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/concerns/dossier_filtering_concern.rb", - "line": 46, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "where(\"#{values.count} OR #{\"(#{DossierFilterService.sanitized_column(table, column)} = ?)\"}\", *values.map do\n \"[\\\"#{value}\\\"]\"\n end)", - "render_path": null, - "location": { - "type": "method", - "class": "DossierFilteringConcern", - "method": null - }, - "user_input": "values.count", - "confidence": "Medium", - "cwe_id": [ - 89 - ], - "note": "filtered by rails query params where(something: ?, values)" - }, { "warning_type": "SQL Injection", "warning_code": 0, @@ -120,7 +97,7 @@ "check_name": "SQL", "message": "Possible SQL injection", "file": "app/models/columns/json_path_column.rb", - "line": 24, + "line": 26, "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", "code": "dossiers.with_type_de_champ(stable_id).where(\"champs.value_json @? '#{jsonpath} ? (@ like_regex \\\"#{quote_string(search_terms.join(\"|\"))}\\\" flag \\\"i\\\")'\")", "render_path": null, @@ -136,29 +113,6 @@ ], "note": "escaped by hand" }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "91ff8031e7c639c95fe6c244867349a72078ef456d8b3507deaf2bdb9bf62fe2", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/concerns/dossier_filtering_concern.rb", - "line": 34, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "where(\"#{values.count} OR #{\"(#{DossierFilterService.sanitized_column(table, column)} ILIKE ?)\"}\", *values.map do\n \"%#{value}%\"\n end)", - "render_path": null, - "location": { - "type": "method", - "class": "DossierFilteringConcern", - "method": null - }, - "user_input": "values.count", - "confidence": "Medium", - "cwe_id": [ - 89 - ], - "note": "filtered by rails query params where(something: ?, values)" - }, { "warning_type": "Cross-Site Scripting", "warning_code": 2, @@ -219,20 +173,20 @@ { "warning_type": "SQL Injection", "warning_code": 0, - "fingerprint": "aaff41afa7bd5a551cd2e3a385071090cb53c95caa40fad3785cd3d68c9b939c", + "fingerprint": "afd2a1a41bd87fa62e065671670bd9bd8cc503ca4cbd3cfdb74a38a794146926", "check_name": "SQL", "message": "Possible SQL injection", "file": "app/models/concerns/dossier_filtering_concern.rb", - "line": 40, + "line": 35, "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "where(\"#{values.count} OR #{\"(#{DossierFilterService.sanitized_column(table, column)} = ?)\"}\", *values)", + "code": "where(\"#{DossierFilterService.sanitized_column(table, column)} LIKE ANY (ARRAY[?])\", search_terms.map do\n \"%#{sanitize_sql_like(_1)}%\"\n end)", "render_path": null, "location": { "type": "method", "class": "DossierFilteringConcern", "method": null }, - "user_input": "values.count", + "user_input": "DossierFilterService.sanitized_column(table, column)", "confidence": "Medium", "cwe_id": [ 89 @@ -295,6 +249,6 @@ "note": "Current is not a model" } ], - "updated": "2024-11-04 09:56:55 +0100", + "updated": "2024-11-12 17:33:07 +0100", "brakeman_version": "6.1.2" } diff --git a/spec/models/columns/linked_drop_down_column_spec.rb b/spec/models/columns/linked_drop_down_column_spec.rb new file mode 100644 index 000000000..0929090bb --- /dev/null +++ b/spec/models/columns/linked_drop_down_column_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +describe Columns::LinkedDropDownColumn do + describe '#filtered_ids' do + let(:procedure) { create(:procedure, types_de_champ_public: [{ type: :linked_drop_down_list, libelle: 'linked' }]) } + let(:type_de_champ) { procedure.active_revision.types_de_champ_public.first } + let(:kept_dossier) { create(:dossier, procedure: procedure) } + let(:discarded_dossier) { create(:dossier, procedure: procedure) } + + subject { column.filtered_ids(Dossier.all, search_terms) } + + context 'when path is :value' do + let(:column) { procedure.find_column(label: 'linked') } + + before do + kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id) + .update(value: %{["section 1","option A"]}) + + discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id) + .update(value: %{["section 1","option B"]}) + end + + describe 'when looking for a part' do + let(:search_terms) { ['option A'] } + + it { is_expected.to eq([kept_dossier.id]) } + end + + describe 'when looking for the aggregated value' do + let(:search_terms) { ['section 1 / option A'] } + + it { is_expected.to match_array([kept_dossier.id]) } + end + + describe 'when looking for the aggregated value or a common value' do + let(:search_terms) { ['section 1 / option A', 'section'] } + + it { is_expected.to match_array([kept_dossier.id, discarded_dossier.id]) } + end + + describe 'when looking for a shared string' do + let(:search_terms) { ['option'] } + + it { is_expected.to match_array([kept_dossier.id, discarded_dossier.id]) } + end + end + + context 'when path is not :value' do + before do + kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id) + .update(value: %{["1","2"]}) + + discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id) + .update(value: %{["2","1"]}) + end + + context 'when path is :primary' do + let(:column) { procedure.find_column(label: 'linked (Primaire)') } + + describe 'when looking kept part' do + let(:search_terms) { ['1'] } + + it { is_expected.to eq([kept_dossier.id]) } + end + end + + context 'when path is :secondary' do + let(:column) { procedure.find_column(label: 'linked (Secondaire)') } + + describe 'when looking kept part' do + let(:search_terms) { ['2'] } + + it { is_expected.to eq([kept_dossier.id]) } + end + end + end + end +end diff --git a/spec/services/dossier_filter_service_spec.rb b/spec/services/dossier_filter_service_spec.rb index d00ae6371..2290aad8c 100644 --- a/spec/services/dossier_filter_service_spec.rb +++ b/spec/services/dossier_filter_service_spec.rb @@ -510,20 +510,42 @@ describe DossierFilterService do end context 'with enums type_de_champ' do - let(:filter) { [type_de_champ.libelle, 'Favorable'] } - let(:types_de_champ_public) { [{ type: :multiple_drop_down_list, options: ['Favorable', 'Defavorable'] }] } + let(:filter) { [type_de_champ.libelle, search_term] } + let(:types_de_champ_public) { [{ type: :multiple_drop_down_list, options: ['champ', 'champignon'] }] } before do kept_champ = kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id) - kept_champ.value = ['Favorable'] + kept_champ.value = ['champ', 'champignon'] kept_champ.save! discarded_champ = discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id) - discarded_champ.value = ['Defavorable'] + discarded_champ.value = ['champignon'] discarded_champ.save! end - it { is_expected.to contain_exactly(kept_dossier.id) } + context 'with single value' do + let(:search_term) { 'champ' } + + it { is_expected.to contain_exactly(kept_dossier.id) } + end + + context 'with multiple search values' do + let(:search_term) { 'champignon' } + + it { is_expected.to contain_exactly(kept_dossier.id, discarded_dossier.id) } + end + + context 'test if I could break a regex with %' do + let(:search_term) { '%' } + + it { is_expected.to be_empty } + end + + context 'test if I could break a regex with .' do + let(:search_term) { '.*' } + + it { is_expected.to be_empty } + end end end