Merge pull request #2742 from betagouv/frederic/fix_2579-injection_sql_filtrage_instructeur

Injection sql filtrage instructeur

app/models/dossier.rb

@@ -229,10 +229,6 @@ class Dossier < ApplicationRecord
     DossierFieldService.get_value(self, table, column)
   end
 
-  def self.sanitize_for_order(order)
-    sanitize_sql_for_order(order)
-  end
-
   def owner_name
     if etablissement.present?
       etablissement.entreprise_raison_sociale

app/models/procedure_presentation.rb

@@ -1,3 +1,45 @@
 class ProcedurePresentation < ApplicationRecord
   belongs_to :assign_to
+
+  delegate :procedure, to: :assign_to
+
+  validate :check_allowed_displayed_fields
+  validate :check_allowed_sort_column
+  validate :check_allowed_filter_columns
+
+  def check_allowed_displayed_fields
+    displayed_fields.each do |field|
+      table = field['table']
+      column = field['column']
+      if !DossierFieldService.valid_column?(procedure, table, column)
+        errors.add(:filters, "#{table}.#{column} n’est pas une colonne permise")
+      end
+    end
+  end
+
+  def check_allowed_sort_column
+    table = sort['table']
+    column = sort['column']
+    if !valid_sort_column?(procedure, table, column)
+      errors.add(:sort, "#{table}.#{column} n’est pas une colonne permise")
+    end
+  end
+
+  def check_allowed_filter_columns
+    filters.each do |_, columns|
+      columns.each do |column|
+        table = column['table']
+        column = column['column']
+        if !DossierFieldService.valid_column?(procedure, table, column)
+          errors.add(:filters, "#{table}.#{column} n’est pas une colonne permise")
+        end
+      end
+    end
+  end
+
+  private
+
+  def valid_sort_column?(procedure, table, column)
+    DossierFieldService.valid_column?(procedure, table, column) || (table == 'notifications' && column == 'notifications')
+  end
 end

app/services/dossier_field_service.rb

@@ -1,4 +1,6 @@
 class DossierFieldService
+  @@column_whitelist = {}
+
   class << self
     def fields(procedure)
       fields = [
@@ -44,6 +46,8 @@ class DossierFieldService
     end
 
     def get_value(dossier, table, column)
+      assert_valid_column(dossier.procedure, table, column)
+
       case table
       when 'self'
         dossier.send(column)
@@ -60,9 +64,21 @@ class DossierFieldService
       end
     end
 
+    def assert_valid_column(procedure, table, column)
+      if !valid_column?(procedure, table, column)
+        raise "Invalid column #{table}.#{column}"
+      end
+    end
+
+    def valid_column?(procedure, table, column)
+      valid_columns_for_table(procedure, table).include?(column)
+    end
+
     def filtered_ids(dossiers, filters)
       filters.map do |filter|
-        case filter['table']
+        table = filter['table']
+        column = sanitized_column(filter)
+        case table
         when 'self'
           dossiers.where("? ILIKE ?", filter['column'], "%#{filter['value']}%")
 
@@ -72,72 +88,97 @@ class DossierFieldService
             .where("? ILIKE ?", "france_connect_informations.#{filter['column']}", "%#{filter['value']}%")
 
         when 'type_de_champ', 'type_de_champ_private'
-          relation = filter['table'] == 'type_de_champ' ? :champs : :champs_private
+          relation = table == 'type_de_champ' ? :champs : :champs_private
           dossiers
             .includes(relation)
             .where("champs.type_de_champ_id = ?", filter['column'].to_i)
             .where("champs.value ILIKE ?", "%#{filter['value']}%")
         when 'etablissement'
-          table = filter['table']
           if filter['column'] == 'entreprise_date_creation'
             date = filter['value'].to_date rescue nil
             dossiers
               .includes(table)
-              .where("#{table.pluralize}.#{filter['column']} = ?", date)
+              .where("#{column} = ?", date)
           else
             dossiers
               .includes(table)
-              .where("#{table.pluralize}.#{filter['column']} ILIKE ?", "%#{filter['value']}%")
+              .where("#{column} ILIKE ?", "%#{filter['value']}%")
           end
         when 'user'
           dossiers
-            .includes(filter['table'])
-            .where("#{filter['table'].pluralize}.#{filter['column']} ILIKE ?", "%#{filter['value']}%")
+            .includes(table)
+            .where("#{column} ILIKE ?", "%#{filter['value']}%")
         end.pluck(:id)
       end.reduce(:&)
     end
 
     def sorted_ids(dossiers, procedure_presentation, gestionnaire)
       table = procedure_presentation.sort['table']
-      column = procedure_presentation.sort['column']
+      column = sanitized_column(procedure_presentation.sort)
       order = procedure_presentation.sort['order']
-      includes = ''
-      where = ''
-
-      sorted_ids = nil
+      assert_valid_order(order)
 
       case table
       when 'notifications'
         procedure = procedure_presentation.assign_to.procedure
         dossiers_id_with_notification = gestionnaire.notifications_for_procedure(procedure)
         if order == 'desc'
-          sorted_ids = dossiers_id_with_notification + (dossiers.order('dossiers.updated_at desc').ids - dossiers_id_with_notification)
+          return dossiers_id_with_notification +
+              (dossiers.order('dossiers.updated_at desc').ids - dossiers_id_with_notification)
         else
-          sorted_ids = (dossiers.order('dossiers.updated_at asc').ids - dossiers_id_with_notification) + dossiers_id_with_notification
+          return (dossiers.order('dossiers.updated_at asc').ids - dossiers_id_with_notification) +
+              dossiers_id_with_notification
         end
       when 'self'
-        order = "dossiers.#{column} #{order}"
+        return dossiers
+            .order("#{column} #{order}")
+            .pluck(:id)
       when 'france_connect_information'
-        includes = { user: :france_connect_information }
-        order = "france_connect_informations.#{column} #{order}"
+        return dossiers
+            .includes(user: :france_connect_information)
+            .order("#{column} #{order}")
+            .pluck(:id)
       when 'type_de_champ', 'type_de_champ_private'
-        includes = table == 'type_de_champ' ? :champs : :champs_private
-        where = "champs.type_de_champ_id = #{column.to_i}"
-        order = "champs.value #{order}"
+        return dossiers
+            .includes(table == 'type_de_champ' ? :champs : :champs_private)
+            .where("champs.type_de_champ_id = #{procedure_presentation.sort['column'].to_i}")
+            .order("champs.value #{order}")
+            .pluck(:id)
       else
-        includes = table
-        order = "#{table.pluralize}.#{column} #{order}"
+        return dossiers
+            .includes(table)
+            .order("#{column} #{order}")
+            .pluck(:id)
       end
-
-      if sorted_ids.nil?
-        sorted_ids = dossiers.includes(includes).where(where).order(Dossier.sanitize_for_order(order)).pluck(:id)
-      end
-
-      sorted_ids
     end
 
     private
 
+    def valid_columns_for_table(procedure, table)
+      if !@@column_whitelist.key?(procedure.id)
+        @@column_whitelist[procedure.id] = fields(procedure)
+          .group_by { |field| field['table'] }
+          .map { |table, fields| [table, Set.new(fields.map { |field| field['column'] }) ] }
+          .to_h
+      end
+
+      @@column_whitelist[procedure.id][table] || []
+    end
+
+    def sanitized_column(field)
+      table = field['table']
+      table = ActiveRecord::Base.connection.quote_column_name((table == 'self' ? 'dossier' : table).pluralize)
+      column = ActiveRecord::Base.connection.quote_column_name(field['column'])
+
+      table + '.' + column
+    end
+
+    def assert_valid_order(order)
+      if !%w[asc desc].include?(order)
+        raise "Invalid order #{order}"
+      end
+    end
+
     def field_hash(label, table, column)
       {
         'label' => label,

spec/factories/procedure_presentation.rb

@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :procedure_presentation do
+    assign_to { create(:assign_to, procedure: create(:procedure, :with_type_de_champ)) }
+    sort { { "table" => "user", "column" => "email", "order" => "asc" } }
+  end
+end

spec/models/assign_to_spec.rb

@@ -7,7 +7,8 @@ describe AssignTo, type: :model do
     end
 
     context "with a procedure_presentation" do
-      let!(:assign_to) { AssignTo.create }
+      let(:procedure) { create(:procedure) }
+      let!(:assign_to) { AssignTo.create(procedure: procedure) }
       let!(:procedure_presentation) { ProcedurePresentation.create(assign_to: assign_to) }
 
       it { expect(assign_to.procedure_presentation_or_default).to eq(procedure_presentation) }

spec/models/procedure_presentation_spec.rb

@@ -1,20 +1,23 @@
 require 'spec_helper'
 
 describe ProcedurePresentation do
+  let(:assign_to) { create(:assign_to, procedure: create(:procedure, :with_type_de_champ)) }
+  let(:first_type_de_champ_id) { assign_to.procedure.types_de_champ.first.id.to_s }
   let (:procedure_presentation_id) {
     ProcedurePresentation.create(
+      assign_to: assign_to,
       displayed_fields: [
-        { "label" => "test1", "table" => "user" },
-        { "label" => "test2", "table" => "champs" }
+        { "label" => "test1", "table" => "user", "column" => "email" },
+        { "label" => "test2", "table" => "type_de_champ", "column" => first_type_de_champ_id }
       ],
       sort: { "table" => "user","column" => "email","order" => "asc" },
-      filters: { "a-suivre" => [], "suivis" => [{ "label" => "label1", "table" => "table1", "column" => "column1" }] }
+      filters: { "a-suivre" => [], "suivis" => [{ "label" => "label1", "table" => "self", "column" => "created_at" }] }
     ).id
   }
   let (:procedure_presentation) { ProcedurePresentation.find(procedure_presentation_id) }
 
   describe "#displayed_fields" do
-    it { expect(procedure_presentation.displayed_fields).to eq([{ "label" => "test1", "table" => "user" }, { "label" => "test2", "table" => "champs" }]) }
+    it { expect(procedure_presentation.displayed_fields).to eq([{ "label" => "test1", "table" => "user", "column" => "email" }, { "label" => "test2", "table" => "type_de_champ", "column" => first_type_de_champ_id }]) }
   end
 
   describe "#sort" do
@@ -22,6 +25,23 @@ describe ProcedurePresentation do
   end
 
   describe "#filters" do
-    it { expect(procedure_presentation.filters).to eq({ "a-suivre" => [], "suivis" => [{ "label" => "label1", "table" => "table1", "column" => "column1" }] }) }
+    it { expect(procedure_presentation.filters).to eq({ "a-suivre" => [], "suivis" => [{ "label" => "label1", "table" => "self", "column" => "created_at" }] }) }
+  end
+
+  describe 'validation' do
+    it { expect(build(:procedure_presentation)).to be_valid }
+
+    context 'of displayed fields' do
+      it { expect(build(:procedure_presentation, displayed_fields: [{ "table" => "user", "column" => "reset_password_token", "order" => "asc" }])).to be_invalid }
+    end
+
+    context 'of sort' do
+      it { expect(build(:procedure_presentation, sort: { "table" => "notifications", "column" => "notifications", "order" => "asc" })).to be_valid }
+      it { expect(build(:procedure_presentation, sort: { "table" => "user", "column" => "reset_password_token", "order" => "asc" })).to be_invalid }
+    end
+
+    context 'of filters' do
+      it { expect(build(:procedure_presentation, filters: { "suivis" => [{ "table" => "user", "column" => "reset_password_token", "order" => "asc" }] })).to be_invalid }
+    end
   end
 end

spec/services/dossier_field_service_spec.rb

@@ -1,8 +1,38 @@
 require 'spec_helper'
 
 describe DossierFieldService do
+  let(:procedure) { create(:procedure, :with_type_de_champ, :with_type_de_champ_private) }
+
   describe '#filtered_ids' do
-    let(:procedure) { create(:procedure) }
+    context 'for type_de_champ table' do
+      let(:kept_dossier) { create(:dossier, procedure: procedure) }
+      let(:discarded_dossier) { create(:dossier, procedure: procedure) }
+      let(:type_de_champ) { procedure.types_de_champ.first }
+
+      before do
+        type_de_champ.champ.create(dossier: kept_dossier, value: 'keep me')
+        type_de_champ.champ.create(dossier: discarded_dossier, value: 'discard me')
+      end
+
+      subject { described_class.filtered_ids(procedure.dossiers, [{ 'table' => 'type_de_champ', 'column' => type_de_champ.id, 'value' => 'keep' }]) }
+
+      it { is_expected.to contain_exactly(kept_dossier.id) }
+    end
+
+    context 'for type_de_champ_private table' do
+      let(:kept_dossier) { create(:dossier, procedure: procedure) }
+      let(:discarded_dossier) { create(:dossier, procedure: procedure) }
+      let(:type_de_champ_private) { procedure.types_de_champ_private.first }
+
+      before do
+        type_de_champ_private.champ.create(dossier: kept_dossier, value: 'keep me')
+        type_de_champ_private.champ.create(dossier: discarded_dossier, value: 'discard me')
+      end
+
+      subject { described_class.filtered_ids(procedure.dossiers, [{ 'table' => 'type_de_champ_private', 'column' => type_de_champ_private.id, 'value' => 'keep' }]) }
+
+      it { is_expected.to contain_exactly(kept_dossier.id) }
+    end
 
     context 'for etablissement table' do
       context 'for entreprise_date_creation column' do
@@ -25,5 +55,193 @@ describe DossierFieldService do
         it { is_expected.to contain_exactly(kept_dossier.id) }
       end
     end
+
+    context 'for user table' do
+      let!(:kept_dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'me@keepmail.com')) }
+      let!(:discarded_dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'me@discard.com')) }
+
+      subject { described_class.filtered_ids(procedure.dossiers, [{ 'table' => 'user', 'column' => 'email', 'value' => 'keepmail' }]) }
+
+      it { is_expected.to contain_exactly(kept_dossier.id) }
+    end
+  end
+
+  describe '#sorted_ids' do
+    let(:gestionnaire) { create(:gestionnaire) }
+    let(:assign_to) { create(:assign_to, procedure: procedure, gestionnaire: gestionnaire) }
+    let(:sort) { { 'table' => table, 'column' => column, 'order' => order } }
+    let(:procedure_presentation) { ProcedurePresentation.create(assign_to: assign_to, sort: sort) }
+
+    subject { DossierFieldService.sorted_ids(procedure.dossiers, procedure_presentation, gestionnaire) }
+
+    context 'for notifications table' do
+      let(:table) { 'notifications' }
+      let(:column) { 'notifications' }
+
+      let!(:notified_dossier) { create(:dossier, :en_construction, procedure: procedure) }
+      let!(:recent_dossier) { create(:dossier, :en_construction, procedure: procedure) }
+      let!(:older_dossier) { create(:dossier, :en_construction, procedure: procedure) }
+
+      before do
+        notified_dossier.champs.first.touch(time: DateTime.new(2018, 9, 20))
+        create(:follow, gestionnaire: gestionnaire, dossier: notified_dossier, demande_seen_at: DateTime.new(2018, 9, 10))
+        recent_dossier.touch(time: DateTime.new(2018, 9, 25))
+        older_dossier.touch(time: DateTime.new(2018, 5, 13))
+      end
+
+      context 'in ascending order' do
+        let(:order) { 'asc' }
+
+        it { is_expected.to eq([older_dossier, recent_dossier, notified_dossier].map(&:id)) }
+      end
+
+      context 'in descending order' do
+        let(:order) { 'desc' }
+
+        it { is_expected.to eq([notified_dossier, recent_dossier, older_dossier].map(&:id)) }
+      end
+    end
+
+    context 'for self table' do
+      let(:table) { 'self' }
+      let(:column) { 'updated_at' } # All other columns work the same, no extra test required
+      let(:order) { 'asc' } # Desc works the same, no extra test required
+
+      let(:recent_dossier) { create(:dossier, procedure: procedure) }
+      let(:older_dossier) { create(:dossier, procedure: procedure) }
+
+      before do
+        recent_dossier.touch(time: DateTime.new(2018, 9, 25))
+        older_dossier.touch(time: DateTime.new(2018, 5, 13))
+      end
+
+      it { is_expected.to eq([older_dossier, recent_dossier].map(&:id)) }
+    end
+
+    context 'for france_connect_information table' do
+      let(:table) { 'france_connect_information' }
+      let(:column) { 'given_name' } # All other columns work the same, no extra test required
+      let(:order) { 'asc' } # Desc works the same, no extra test required
+
+      let(:anna_dossier) { create(:dossier, procedure: procedure) }
+      let(:zacharie_dossier) { create(:dossier, procedure: procedure) }
+
+      before do
+        create(:france_connect_information, given_name: 'Anna', user: anna_dossier.user)
+        create(:france_connect_information, given_name: 'Zacharie', user: zacharie_dossier.user)
+      end
+
+      it { is_expected.to eq([anna_dossier, zacharie_dossier].map(&:id)) }
+    end
+
+    context 'for type_de_champ table' do
+      let(:table) { 'type_de_champ' }
+      let(:column) { procedure.types_de_champ.first.id.to_s }
+      let(:order) { 'desc' } # Asc works the same, no extra test required
+
+      let(:beurre_dossier) { create(:dossier, procedure: procedure) }
+      let(:tartine_dossier) { create(:dossier, procedure: procedure) }
+
+      before do
+        beurre_dossier.champs.first.update(value: 'beurre')
+        tartine_dossier.champs.first.update(value: 'tartine')
+      end
+
+      it { is_expected.to eq([tartine_dossier, beurre_dossier].map(&:id)) }
+    end
+
+    context 'for type_de_champ_private table' do
+      let(:table) { 'type_de_champ_private' }
+      let(:column) { procedure.types_de_champ_private.first.id.to_s }
+      let(:order) { 'asc' } # Desc works the same, no extra test required
+
+      let(:biere_dossier) { create(:dossier, procedure: procedure) }
+      let(:vin_dossier) { create(:dossier, procedure: procedure) }
+
+      before do
+        biere_dossier.champs_private.first.update(value: 'biere')
+        vin_dossier.champs_private.first.update(value: 'vin')
+      end
+
+      it { is_expected.to eq([biere_dossier, vin_dossier].map(&:id)) }
+    end
+
+    context 'for other tables' do
+      # All other columns and tables work the same so it’s ok to test only one
+      let(:table) { 'etablissement' }
+      let(:column) { 'code_postal' }
+      let(:order) { 'asc' } # Desc works the same, no extra test required
+
+      let!(:huitieme_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '75008')) }
+      let!(:vingtieme_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '75020')) }
+
+      it { is_expected.to eq([huitieme_dossier, vingtieme_dossier].map(&:id)) }
+    end
+  end
+
+  describe '#get_value' do
+    subject { DossierFieldService.get_value(dossier, table, column) }
+
+    context 'for self table' do
+      let(:table) { 'self' }
+      let(:column) { 'updated_at' } # All other columns work the same, no extra test required
+
+      let(:dossier) { create(:dossier, procedure: procedure) }
+
+      before { dossier.touch(time: DateTime.new(2018, 9, 25)) }
+
+      it { is_expected.to eq(DateTime.new(2018, 9, 25)) }
+    end
+
+    context 'for user table' do
+      let(:table) { 'user' }
+      let(:column) { 'email' }
+
+      let(:dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'bla@yopmail.com')) }
+
+      it { is_expected.to eq('bla@yopmail.com') }
+    end
+
+    context 'for france_connect_information table' do
+      let(:table) { 'france_connect_information' }
+      let(:column) { 'given_name' } # All other columns work the same, no extra test required
+
+      let(:dossier) { create(:dossier, procedure: procedure) }
+
+      before { create(:france_connect_information, given_name: 'Anna', user: dossier.user) }
+
+      it { is_expected.to eq('Anna') }
+    end
+
+    context 'for etablissement table' do
+      let(:table) { 'etablissement' }
+      let(:column) { 'code_postal' } # All other columns work the same, no extra test required
+
+      let!(:dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '75008')) }
+
+      it { is_expected.to eq('75008') }
+    end
+
+    context 'for type_de_champ table' do
+      let(:table) { 'type_de_champ' }
+      let(:column) { procedure.types_de_champ.first.id.to_s }
+
+      let(:dossier) { create(:dossier, procedure: procedure) }
+
+      before { dossier.champs.first.update(value: 'kale') }
+
+      it { is_expected.to eq('kale') }
+    end
+
+    context 'for type_de_champ_private table' do
+      let(:table) { 'type_de_champ_private' }
+      let(:column) { procedure.types_de_champ_private.first.id.to_s }
+
+      let(:dossier) { create(:dossier, procedure: procedure) }
+
+      before { dossier.champs_private.first.update(value: 'quinoa') }
+
+      it { is_expected.to eq('quinoa') }
+    end
   end
 end