Security fix (#327) (#353)

* prevent sql injection

* ruboxcop

* simplify orderable override

* escape collation search

* fix location detection

* use filter-map

CHANGELOG.md

@@ -24,7 +24,8 @@ Features:
   - Added GraphQL types for custom fields in the API
   - Adds parsed information about custom fields in the Proposals export
   - Adds parsed information bout private custom fields when admins exports private data
-  - Adds a maintenance menu with tools to remove old private data
+  - Adds a maintenance menu with tools to remove old private 
+  - SQL vulnerability fix for admin accountability
 
 v0.10.2
 ------

app/controllers/concerns/decidim/decidim_awesome/admin_accountability/admin/filterable_helper.rb

@@ -13,10 +13,11 @@ def extra_dropdown_submenu_options_items(_filter, _i18n_scope)
           end
 
           def applied_filters_tags(i18n_ctx)
-            if global? && params[:admin_role_type].present?
+            admin_role_type = PaperTrailVersion.safe_admin_role_type(params[:admin_role_type])
+            if global? && admin_role_type.present?
               content_tag(:span, class: "label secondary") do
                 concat "#{i18n_filter_label(:admin_role_type, filterable_i18n_scope_from_ctx(i18n_ctx))}: "
-                concat t("decidim.decidim_awesome.admin.admin_accountability.admin_roles.#{params[:admin_role_type]}", default: params[:admin_role_type])
+                concat t("decidim.decidim_awesome.admin.admin_accountability.admin_roles.#{admin_role_type}", default: admin_role_type)
                 concat icon_link_to(
                   "circle-x",
                   url_for(export_params.except(:admin_role_type)),

app/controllers/concerns/decidim/decidim_awesome/proposals/orderable_override.rb

@@ -14,6 +14,8 @@ module OrderableOverride
 
           private
 
+          alias_method :decidim_original_reorder, :reorder
+
           # read order from session if available
           def order
             @order ||= detect_order(session[:order]) || default_order
@@ -31,38 +33,23 @@ def possible_orders
             end
           end
 
-          # rubocop:disable Metrics/CyclomaticComplexity
           def reorder(proposals)
+            title_by_locale = Arel.sql(proposals.sanitize_sql(["decidim_proposals_proposals.title->>? #{collation}", locale]))
+            title_by_machine_translation = Arel.sql(proposals.sanitize_sql(["decidim_proposals_proposals.title->'machine_translations'->>? #{collation}", locale]))
+            title_by_default_locale = Arel.sql(proposals.sanitize_sql(["decidim_proposals_proposals.title->>? #{collation}", default_locale]))
             case order
             when "az"
-              proposals.order(Arel.sql("CONCAT(decidim_proposals_proposals.title->>'#{locale}',
-                                        decidim_proposals_proposals.title->'machine_translations'->>'#{locale}',
-                                        decidim_proposals_proposals.title->>'#{default_locale}') #{collation} ASC"))
+              proposals.order(title_by_locale => :asc, title_by_machine_translation => :asc, title_by_default_locale => :asc)
             when "za"
-              proposals.order(Arel.sql("CONCAT(decidim_proposals_proposals.title->>'#{locale}',
-                                        decidim_proposals_proposals.title->'machine_translations'->>'#{locale}',
-                                        decidim_proposals_proposals.title->>'#{default_locale}') #{collation} DESC"))
+              proposals.order(title_by_locale => :desc, title_by_machine_translation => :desc, title_by_default_locale => :desc)
             when "supported_first"
               proposals.joins(my_votes_join).group(:id).order(Arel.sql("COUNT(decidim_proposals_proposal_votes.id) DESC"))
             when "supported_last"
               proposals.joins(my_votes_join).group(:id).order(Arel.sql("COUNT(decidim_proposals_proposal_votes.id) ASC"))
-            when "most_commented"
-              proposals.left_joins(:comments).group(:id).order(Arel.sql("COUNT(decidim_comments_comments.id) DESC"))
-            when "most_endorsed"
-              proposals.order(endorsements_count: :desc)
-            when "most_followed"
-              proposals.left_joins(:follows).group(:id).order(Arel.sql("COUNT(decidim_follows.id) DESC"))
-            when "most_voted"
-              proposals.order(proposal_votes_count: :desc)
-            when "random"
-              proposals.order_randomly(random_seed)
-            when "recent"
-              proposals.order(published_at: :desc)
-            when "with_more_authors"
-              proposals.order(coauthorships_count: :desc)
+            else
+              decidim_original_reorder(proposals)
             end
           end
-          # rubocop:enable Metrics/CyclomaticComplexity
 
           def collation
             @collation ||= begin

app/controllers/decidim/decidim_awesome/admin/admin_accountability_controller.rb

@@ -5,7 +5,7 @@ module DecidimAwesome
     module Admin
       class AdminAccountabilityController < DecidimAwesome::Admin::ApplicationController
         include NeedsAwesomeConfig
-        include Decidim::DecidimAwesome::AdminAccountability::Admin::Filterable
+        include AdminAccountability::Admin::Filterable
 
         helper_method :admin_actions, :collection, :export_params, :global?, :global_users_missing_date
 
@@ -20,9 +20,9 @@ def index; end
         def export
           filters = export_params[:q]
 
-          Decidim::DecidimAwesome::ExportAdminActionsJob.perform_later(current_user,
-                                                                       params[:format].to_s,
-                                                                       admin_actions.ransack(filters).result.ids)
+          ExportAdminActionsJob.perform_later(current_user,
+                                              params[:format].to_s,
+                                              admin_actions.ransack(filters).result.ids)
 
           redirect_back fallback_location: decidim_admin_decidim_awesome.admin_accountability_path,
                         notice: t("decidim.decidim_awesome.admin.admin_accountability.exports.notice")
@@ -39,11 +39,11 @@ def collection
         end
 
         def space_role_actions
-          @space_role_actions ||= Decidim::DecidimAwesome::PaperTrailVersion.space_role_actions(current_organization)
+          @space_role_actions ||= PaperTrailVersion.space_role_actions(current_organization)
         end
 
         def admin_role_actions
-          @admin_role_actions ||= Decidim::DecidimAwesome::PaperTrailVersion.in_organization(current_organization).admin_role_actions(params[:admin_role_type])
+          @admin_role_actions ||= PaperTrailVersion.in_organization(current_organization).admin_role_actions(PaperTrailVersion.safe_admin_role_type(params[:admin_role_type]))
         end
 
         def export_params
@@ -61,7 +61,7 @@ def global_users_missing_date
           return unless global?
 
           @global_users_missing_date ||= begin
-            first_version = Decidim::DecidimAwesome::PaperTrailVersion.where(item_type: "Decidim::UserBaseEntity").last
+            first_version = PaperTrailVersion.where(item_type: "Decidim::UserBaseEntity").last
             first_user = Decidim::User.first
             first_version.created_at if first_user && first_version && (first_version.created_at > first_user.created_at + 1.second)
           end

app/models/decidim/decidim_awesome/paper_trail_version.rb

@@ -9,6 +9,10 @@ def self.safe_user_roles
         DecidimAwesome.participatory_space_roles.filter(&:safe_constantize)
       end
 
+      def self.safe_admin_role_type(admin_role)
+        Decidim.user_roles.find { |role| role == admin_role }
+      end
+
       scope :space_role_actions, lambda { |organization|
         role_changes = where(item_type: PaperTrailVersion.safe_user_roles, event: "create")
         user_ids_from_object_changes = role_changes.pluck(:object_changes).map { |change| change.match(/decidim_user_id:\n- ?\n- (\d+)/)[1].to_i }
@@ -33,7 +37,7 @@ def self.admin_role_actions(filter = nil)
         when "admin"
           base.where("object_changes LIKE '%\nadmin:\n- false\n- true%'")
         else
-          base.where(Arel.sql("object_changes LIKE '%\nroles:\n- []\n- - #{filter}\n%'"))
+          base.where("object_changes LIKE ?", "%\nroles:\n- []\n- - #{filter}\n%")
         end
       end
 

lib/decidim/decidim_awesome/awesome.rb

@@ -356,11 +356,11 @@ def self.possible_additional_proposal_sortings
 
     def self.collation_for(locale)
       @collation_for ||= {}
-      @collation_for[locale] ||= begin
-        res = ActiveRecord::Base.connection.execute(Arel.sql("SELECT collname FROM pg_collation WHERE collname LIKE '#{locale}-x-icu' LIMIT 1")).first
-        res ||= ActiveRecord::Base.connection.execute(Arel.sql("SELECT collname FROM pg_collation WHERE collname LIKE '#{locale[0..1]}%' LIMIT 1")).first
+      @collation_for[locale] ||= ["#{locale}-x-icu", "#{locale[0..1]}%"].filter_map do |loc|
+        sql = ApplicationRecord.sanitize_sql(["SELECT collname FROM pg_collation WHERE collname LIKE ? LIMIT 1", loc])
+        res = ActiveRecord::Base.connection.execute(sql).first
         res["collname"] if res
-      end
+      end.first
     end
 
     def self.enabled?(*config_vars)

spec/models/paper_trail_version_spec.rb

@@ -106,6 +106,10 @@ module Decidim::DecidimAwesome
         expect(PaperTrailVersion.in_organization(external_organization).admin_role_actions("valuator")).to include(external_paper_trail_version)
         expect(PaperTrailVersion.in_organization(external_organization).admin_role_actions("admin")).to eq([])
       end
+
+      it "ignores invalid filters" do
+        expect(PaperTrailVersion.in_organization(organization).admin_role_actions("%')")).to eq([])
+      end
     end
   end
 end