Merge pull request #10866 from mfo/US/fix-filter-ilike

amelioration : ETQ instructeur, je souhaite pouvoir filtrer les dossiers sur mon tableau de bord fonction des choix simple/multiple/etc.. sans conflit parmis les autres valeurs de ces même choix simple/multiple/etc...
2024-10-03 13:08:49 +00:00 · 2024-10-03 13:08:49 +00:00 · 5879d54145
commit 5879d54145
parent ecb3133cb9 c02e4fff57
5 changed files with 63 additions and 21 deletions
--- a/app/models/concerns/columns_concern.rb
+++ b/app/models/concerns/columns_concern.rb
@ -53,7 +53,7 @@ module ColumnsConcern
        Column.new(table: 'user', column: 'email'),
        Column.new(table: 'followers_instructeurs', column: 'email'),
        Column.new(table: 'groupe_instructeur', column: 'id', type: :enum),
-        Column.new(table: 'avis', column: 'question_answer', filterable: false)
+        Column.new(table: 'avis', column: 'question_answer', filterable: false) # not filterable ?
      ]
    end

--- a/app/models/concerns/dossier_filtering_concern.rb
+++ b/app/models/concerns/dossier_filtering_concern.rb
@ -33,5 +33,11 @@ module DossierFilteringConcern
      q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ')
      where(q, *(values.map { |value| "%#{value}%" }))
    }
+
+    scope :filter_enum, lambda { |table, column, values|
+      table_column = ProcedurePresentation.sanitized_column(table, column)
+      q = Array.new(values.count, "(#{table_column} = ?)").join(' OR ')
+      where(q, *(values))
+    }
  end
 end
--- a/app/models/procedure_presentation.rb
+++ b/app/models/procedure_presentation.rb
@ -220,8 +220,13 @@ class ProcedurePresentation < ApplicationRecord
            dossiers.where("dossiers.#{column} IN (?)", values)
          end
        when TYPE_DE_CHAMP
-          dossiers.with_type_de_champ(column)
-            .filter_ilike(:champs, value_column, values)
+          if dossier_column.type == :enum
+            dossiers.with_type_de_champ(column)
+              .filter_enum(:champs, value_column, values)
+          else
+            dossiers.with_type_de_champ(column)
+              .filter_ilike(:champs, value_column, values)
+          end
        when 'etablissement'
          if column == 'entreprise_date_creation'
            dates = values
@ -240,22 +245,17 @@ class ProcedurePresentation < ApplicationRecord
          dossiers
            .includes(:followers_instructeurs)
            .joins('INNER JOIN users instructeurs_users ON instructeurs_users.id = instructeurs.user_id')
-            .filter_ilike('instructeurs_users', :email, values)
-        when 'user', 'individual', 'avis'
+            .filter_ilike('instructeurs_users', :email, values) # ilike OK, user may want to search by *@domain
+        when 'user', 'individual' # user_columns: [email], individual_columns: ['nom', 'prenom', 'gender']
          dossiers
            .includes(table)
-            .filter_ilike(table, column, values)
+            .filter_ilike(table, column, values) # ilike or where column == 'value' are both valid, we opted for ilike
        when 'groupe_instructeur'
          assert_supported_column(table, column)
-          if column == 'label'
-            dossiers
-              .joins(:groupe_instructeur)
-              .filter_ilike(table, column, values)
-          else
-            dossiers
-              .joins(:groupe_instructeur)
-              .where(groupe_instructeur_id: values)
-          end
+
+          dossiers
+            .joins(:groupe_instructeur)
+            .where(groupe_instructeur_id: values)
        end.pluck(:id)
      end
    end.reduce(:&)
--- a/config/brakeman.ignore
+++ b/config/brakeman.ignore
@ -15,7 +15,7 @@
          "type": "controller",
          "class": "Users::DossiersController",
          "method": "merci",
-          "line": 329,
+          "line": 323,
          "file": "app/controllers/users/dossiers_controller.rb",
          "rendered": {
            "name": "users/dossiers/merci",
@ -44,6 +44,29 @@
      ],
      "note": ""
    },
+    {
+      "warning_type": "SQL Injection",
+      "warning_code": 0,
+      "fingerprint": "31693060072e27c02ca4f884f2a07f4f1c1247b7a6f5cc5c724e88e6ca9b4873",
+      "check_name": "SQL",
+      "message": "Possible SQL injection",
+      "file": "app/models/concerns/dossier_filtering_concern.rb",
+      "line": 40,
+      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
+      "code": "where(\"#{values.count} OR #{\"(#{ProcedurePresentation.sanitized_column(table, column)} = ?)\"}\", *values)",
+      "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,
@ -51,7 +74,7 @@
      "check_name": "SQL",
      "message": "Possible SQL injection",
      "file": "app/models/columns/json_path_column.rb",
-      "line": 10,
+      "line": 11,
      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
      "code": "dossiers.with_type_de_champ(stable_id).where(\"#{search_occurences.count} OR #{\"(#{json_path_query_part} ILIKE ?)\"}\", *search_occurences.map do\n \"%#{value}%\"\n end)",
      "render_path": null,
@ -74,7 +97,7 @@
      "check_name": "SQL",
      "message": "Possible SQL injection",
      "file": "app/graphql/connections/cursor_connection.rb",
-      "line": 150,
+      "line": 152,
      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
      "code": "items.order(order_column => ((:desc or :asc)), :id => ((:desc or :asc))).limit(limit).where(\"(#{order_table}.#{order_column}, #{order_table}.id) < (?, ?)\", timestamp, id)",
      "render_path": null,
@ -131,7 +154,7 @@
      "check_name": "SQL",
      "message": "Possible SQL injection",
      "file": "app/graphql/connections/cursor_connection.rb",
-      "line": 153,
+      "line": 155,
      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
      "code": "items.order(order_column => ((:desc or :asc)), :id => ((:desc or :asc))).limit(limit).where(\"(#{order_table}.#{order_column}, #{order_table}.id) > (?, ?)\", timestamp, id)",
      "render_path": null,
@ -154,7 +177,7 @@
      "check_name": "SQL",
      "message": "Possible SQL injection",
      "file": "app/models/concerns/dossier_filtering_concern.rb",
-      "line": 32,
+      "line": 34,
      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
      "code": "where(\"#{values.count} OR #{\"(#{ProcedurePresentation.sanitized_column(table, column)} ILIKE ?)\"}\", *values.map do\n \"%#{value}%\"\n end)",
      "render_path": null,
@ -226,6 +249,6 @@
      "note": "Current is not a model"
    }
  ],
-  "updated": "2024-08-20 14:34:27 +0200",
+  "updated": "2024-09-24 20:56:24 +0200",
  "brakeman_version": "6.1.2"
 }
--- a/spec/models/procedure_presentation_spec.rb
+++ b/spec/models/procedure_presentation_spec.rb
@ -530,6 +530,19 @@ describe ProcedurePresentation do

        it { is_expected.to contain_exactly(kept_dossier.id) }
      end
+
+      context 'with enum type_de_champ' do
+        let(:filter_value) { 'Favorable' }
+        let(:filter) { [{ 'table' => 'type_de_champ', 'column' => type_de_champ.stable_id.to_s, 'value_column' => :value, 'value' => filter_value }] }
+        let(:types_de_champ_public) { [{ type: :drop_down_list, options: ['Favorable', 'Defavorable'] }] }
+
+        before do
+          kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id).update(value: 'Favorable')
+          discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id).update(external_id: 'Defavorable')
+        end
+
+        it { is_expected.to contain_exactly(kept_dossier.id) }
+      end
    end

    context 'for type_de_champ_private table' do