From ec2db4dd2a9472532a5cb5b07bb0072182ba6830 Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Sun, 19 May 2019 17:56:54 +0200 Subject: [PATCH 1/8] Fixed sql injection vulnerability in users_and_enterprises_report. --- .../users_and_enterprises_report.rb | 77 ++++++++++++++----- 1 file changed, 59 insertions(+), 18 deletions(-) diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index 89dba0b73a..8aebc6ac00 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -38,29 +38,70 @@ module OpenFoodNetwork end def owners_and_enterprises - query = "SELECT enterprises.name, enterprises.sells, enterprises.visible, enterprises.is_primary_producer, enterprises.created_at AS created_at, - 'owns' AS relationship_type, owners.email as user_email FROM enterprises - LEFT JOIN spree_users AS owners ON owners.id=enterprises.owner_id - WHERE enterprises.id IS NOT NULL - #{ params[:enterprise_id_in].present? ? "AND enterprises.id IN (#{ params[:enterprise_id_in] })" : "" } - #{ params[:user_id_in].present? ? "AND owners.id IN (#{ params[:user_id_in] })" : "" } - ORDER BY enterprises.created_at DESC" + query = Enterprise.joins("LEFT JOIN spree_users AS owner ON enterprises.owner_id = owner.id") + .where("enterprises.id IS NOT NULL") - ActiveRecord::Base.connection.execute(query).to_a + if params[:enterprise_id_in].present? + query = query.where("enterprises.id IN (?)", params[:enterprise_id_in].split(',').map(&:to_i)) + end + + if params[:user_id_in].present? + query = query.where("owner.id IN (?)", params[:user_id_in].split(',').map(&:to_i)) + end + + query.order("enterprises.created_at DESC") + .select([ + "enterprises.name", + "enterprises.sells", + "enterprises.visible", + "enterprises.is_primary_producer", + "enterprises.created_at", + "owner.email AS user_email"]) + .to_a + .map {|x| { + name: x.name, + sells: x.sells, + visible: (x.visible ? 't' : 'f'), + is_primary_producer: (x.is_primary_producer ? 't' : 'f'), + created_at: x.created_at.utc.iso8601, + relationship_type: 'owns', + user_email: x.user_email + }.stringify_keys } end def managers_and_enterprises - query = "SELECT enterprises.name, enterprises.sells, enterprises.visible, enterprises.is_primary_producer, enterprises.created_at AS created_at, - 'manages' AS relationship_type, managers.email as user_email FROM enterprises - LEFT JOIN enterprise_roles ON enterprises.id=enterprise_roles.enterprise_id - LEFT JOIN spree_users AS managers ON enterprise_roles.user_id=managers.id - WHERE enterprise_id IS NOT NULL - #{ params[:enterprise_id_in].present? ? "AND enterprise_id IN (#{ params[:enterprise_id_in] })" : "" } - AND user_id IS NOT NULL - #{ params[:user_id_in].present? ? "AND user_id IN (#{ params[:user_id_in] })" : "" } - ORDER BY enterprises.created_at DESC" + query = Enterprise.joins("LEFT JOIN enterprise_roles ON enterprises.id = enterprise_roles.enterprise_id") + .joins("LEFT JOIN spree_users AS managers ON enterprise_roles.user_id = managers.id") + .where("enterprise_id IS NOT NULL") - ActiveRecord::Base.connection.execute(query).to_a + if params[:enterprise_id_in].present? + query = query.where("enterprise_id IN (?)", params[:enterprise_id_in].split(',').map(&:to_i)) + end + + query.where("user_id IS NOT NULL") + + if params[:user_id_in].present? + query = query.where("user_id IN (?)", params[:user_id_in].split(',').map(&:to_i)) + end + + query.order("enterprises.created_at DESC") + .select([ + "enterprises.name", + "enterprises.sells", + "enterprises.visible", + "enterprises.is_primary_producer", + "enterprises.created_at", + "managers.email AS user_email"]) + .to_a + .map {|x| { + name: x.name, + sells: x.sells, + visible: (x.visible ? 't' : 'f'), + is_primary_producer: (x.is_primary_producer ? 't' : 'f'), + created_at: x.created_at.utc.iso8601, + relationship_type: 'manages', + user_email: x.user_email + }.stringify_keys } end def users_and_enterprises From 1246c3b6b99999347019f1c8c395e1b1fbec48b8 Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Sun, 19 May 2019 18:57:09 +0200 Subject: [PATCH 2/8] Styling. --- .../users_and_enterprises_report.rb | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index 8aebc6ac00..14edd91205 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -1,3 +1,4 @@ +# rubocop:disable Metrics/ClassLength module OpenFoodNetwork class UsersAndEnterprisesReport attr_reader :params @@ -50,15 +51,15 @@ module OpenFoodNetwork end query.order("enterprises.created_at DESC") - .select([ - "enterprises.name", - "enterprises.sells", - "enterprises.visible", - "enterprises.is_primary_producer", - "enterprises.created_at", - "owner.email AS user_email"]) + .select(["enterprises.name", + "enterprises.sells", + "enterprises.visible", + "enterprises.is_primary_producer", + "enterprises.created_at", + "owner.email AS user_email"]) .to_a - .map {|x| { + .map { |x| + { name: x.name, sells: x.sells, visible: (x.visible ? 't' : 'f'), @@ -85,15 +86,15 @@ module OpenFoodNetwork end query.order("enterprises.created_at DESC") - .select([ - "enterprises.name", - "enterprises.sells", - "enterprises.visible", - "enterprises.is_primary_producer", - "enterprises.created_at", - "managers.email AS user_email"]) + .select(["enterprises.name", + "enterprises.sells", + "enterprises.visible", + "enterprises.is_primary_producer", + "enterprises.created_at", + "managers.email AS user_email"]) .to_a - .map {|x| { + .map { |x| + { name: x.name, sells: x.sells, visible: (x.visible ? 't' : 'f'), @@ -130,3 +131,4 @@ module OpenFoodNetwork end end end +# rubocop:enable Metrics/ClassLength From 4204943a7c5aaa967772f992fe898c3e041fcd5e Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Sun, 19 May 2019 22:04:44 +0200 Subject: [PATCH 3/8] Styling. --- .rubocop_styleguide.yml | 1 + lib/open_food_network/users_and_enterprises_report.rb | 11 ++++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml index accf9e6314..98cdd03b3a 100644 --- a/.rubocop_styleguide.yml +++ b/.rubocop_styleguide.yml @@ -196,6 +196,7 @@ Metrics/ClassLength: Max: 100 Exclude: - engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb + - lib/open_food_network/users_and_enterprises_report.rb Metrics/ModuleLength: Max: 100 diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index 14edd91205..a7dff1ebf4 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -1,4 +1,3 @@ -# rubocop:disable Metrics/ClassLength module OpenFoodNetwork class UsersAndEnterprisesReport attr_reader :params @@ -43,7 +42,8 @@ module OpenFoodNetwork .where("enterprises.id IS NOT NULL") if params[:enterprise_id_in].present? - query = query.where("enterprises.id IN (?)", params[:enterprise_id_in].split(',').map(&:to_i)) + query = query.where("enterprises.id IN (?)", + params[:enterprise_id_in].split(',').map(&:to_i)) end if params[:user_id_in].present? @@ -71,12 +71,14 @@ module OpenFoodNetwork end def managers_and_enterprises - query = Enterprise.joins("LEFT JOIN enterprise_roles ON enterprises.id = enterprise_roles.enterprise_id") + query = Enterprise + .joins("LEFT JOIN enterprise_roles ON enterprises.id = enterprise_roles.enterprise_id") .joins("LEFT JOIN spree_users AS managers ON enterprise_roles.user_id = managers.id") .where("enterprise_id IS NOT NULL") if params[:enterprise_id_in].present? - query = query.where("enterprise_id IN (?)", params[:enterprise_id_in].split(',').map(&:to_i)) + query = query.where("enterprise_id IN (?)", + params[:enterprise_id_in].split(',').map(&:to_i)) end query.where("user_id IS NOT NULL") @@ -131,4 +133,3 @@ module OpenFoodNetwork end end end -# rubocop:enable Metrics/ClassLength From 0e6a576e33a16fc572d013f5e80a1ec111d6078d Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Sun, 19 May 2019 22:05:34 +0200 Subject: [PATCH 4/8] Fix user_id IS NOT NULL check. --- lib/open_food_network/users_and_enterprises_report.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index a7dff1ebf4..aae7917392 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -75,14 +75,13 @@ module OpenFoodNetwork .joins("LEFT JOIN enterprise_roles ON enterprises.id = enterprise_roles.enterprise_id") .joins("LEFT JOIN spree_users AS managers ON enterprise_roles.user_id = managers.id") .where("enterprise_id IS NOT NULL") + .where("user_id IS NOT NULL") if params[:enterprise_id_in].present? query = query.where("enterprise_id IN (?)", params[:enterprise_id_in].split(',').map(&:to_i)) end - query.where("user_id IS NOT NULL") - if params[:user_id_in].present? query = query.where("user_id IN (?)", params[:user_id_in].split(',').map(&:to_i)) end From 2f83d0216854eda80b88c441fcac51e15f515180 Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Sun, 19 May 2019 22:36:55 +0200 Subject: [PATCH 5/8] Reduce code duplication. --- .../users_and_enterprises_report.rb | 27 +++++-------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index aae7917392..eb37d41f36 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -50,24 +50,7 @@ module OpenFoodNetwork query = query.where("owner.id IN (?)", params[:user_id_in].split(',').map(&:to_i)) end - query.order("enterprises.created_at DESC") - .select(["enterprises.name", - "enterprises.sells", - "enterprises.visible", - "enterprises.is_primary_producer", - "enterprises.created_at", - "owner.email AS user_email"]) - .to_a - .map { |x| - { - name: x.name, - sells: x.sells, - visible: (x.visible ? 't' : 'f'), - is_primary_producer: (x.is_primary_producer ? 't' : 'f'), - created_at: x.created_at.utc.iso8601, - relationship_type: 'owns', - user_email: x.user_email - }.stringify_keys } + query_helper(query, :owner, :owns) end def managers_and_enterprises @@ -86,13 +69,17 @@ module OpenFoodNetwork query = query.where("user_id IN (?)", params[:user_id_in].split(',').map(&:to_i)) end + query_helper(query, :managers, :manages) + end + + def query_helper(query, email_user, relationship_type) query.order("enterprises.created_at DESC") .select(["enterprises.name", "enterprises.sells", "enterprises.visible", "enterprises.is_primary_producer", "enterprises.created_at", - "managers.email AS user_email"]) + "#{email_user}.email AS user_email"]) .to_a .map { |x| { @@ -101,7 +88,7 @@ module OpenFoodNetwork visible: (x.visible ? 't' : 'f'), is_primary_producer: (x.is_primary_producer ? 't' : 'f'), created_at: x.created_at.utc.iso8601, - relationship_type: 'manages', + relationship_type: relationship_type, user_email: x.user_email }.stringify_keys } end From 29f68ed5d845bfdd4e53bdbf9c5e060a234443eb Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Tue, 21 May 2019 14:51:52 +0200 Subject: [PATCH 6/8] Rubocop changes. --- .rubocop_styleguide.yml | 1 - lib/open_food_network/users_and_enterprises_report.rb | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml index 98cdd03b3a..accf9e6314 100644 --- a/.rubocop_styleguide.yml +++ b/.rubocop_styleguide.yml @@ -196,7 +196,6 @@ Metrics/ClassLength: Max: 100 Exclude: - engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb - - lib/open_food_network/users_and_enterprises_report.rb Metrics/ModuleLength: Max: 100 diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index eb37d41f36..f94781ec7b 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -1,3 +1,4 @@ +# rubocop:disable Metrics/ClassLength module OpenFoodNetwork class UsersAndEnterprisesReport attr_reader :params @@ -119,3 +120,4 @@ module OpenFoodNetwork end end end +# rubocop:enable Metrics/ClassLength From c16265475703870c7edee1fd865bb27adbc533f6 Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Tue, 21 May 2019 15:21:10 +0200 Subject: [PATCH 7/8] Reduce code duplication. --- .../users_and_enterprises_report.rb | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index f94781ec7b..7795e52b87 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -42,14 +42,8 @@ module OpenFoodNetwork query = Enterprise.joins("LEFT JOIN spree_users AS owner ON enterprises.owner_id = owner.id") .where("enterprises.id IS NOT NULL") - if params[:enterprise_id_in].present? - query = query.where("enterprises.id IN (?)", - params[:enterprise_id_in].split(',').map(&:to_i)) - end - - if params[:user_id_in].present? - query = query.where("owner.id IN (?)", params[:user_id_in].split(',').map(&:to_i)) - end + query = filter_by_int_list_if_present(query, "enterprises.id", params[:enterprise_id_in]) + query = filter_by_int_list_if_present(query, "owner.id", params[:user_id_in]) query_helper(query, :owner, :owns) end @@ -61,14 +55,8 @@ module OpenFoodNetwork .where("enterprise_id IS NOT NULL") .where("user_id IS NOT NULL") - if params[:enterprise_id_in].present? - query = query.where("enterprise_id IN (?)", - params[:enterprise_id_in].split(',').map(&:to_i)) - end - - if params[:user_id_in].present? - query = query.where("user_id IN (?)", params[:user_id_in].split(',').map(&:to_i)) - end + query = filter_by_int_list_if_present(query, "enterprise_id", params[:enterprise_id_in]) + query = filter_by_int_list_if_present(query, "user_id", params[:user_id_in]) query_helper(query, :managers, :manages) end @@ -98,6 +86,17 @@ module OpenFoodNetwork sort( owners_and_enterprises.concat managers_and_enterprises ) end + def filter_by_int_list_if_present(query, filtered_field_name, int_list) + if int_list.present? + query = query.where("#{filtered_field_name} IN (?)", split_int_list(int_list)) + end + query + end + + def split_int_list(int_list) + int_list.split(',').map(&:to_i) + end + def sort(results) results.sort do |a,b| if a["created_at"].nil? || b["created_at"].nil? From 414ae58cbd51837edc1b6fea66ee650579f2d921 Mon Sep 17 00:00:00 2001 From: Drumstickx Date: Tue, 21 May 2019 16:48:02 +0200 Subject: [PATCH 8/8] Rubocop changes. --- lib/open_food_network/users_and_enterprises_report.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/open_food_network/users_and_enterprises_report.rb b/lib/open_food_network/users_and_enterprises_report.rb index 7795e52b87..3b5701d07b 100644 --- a/lib/open_food_network/users_and_enterprises_report.rb +++ b/lib/open_food_network/users_and_enterprises_report.rb @@ -1,4 +1,3 @@ -# rubocop:disable Metrics/ClassLength module OpenFoodNetwork class UsersAndEnterprisesReport attr_reader :params @@ -119,4 +118,3 @@ module OpenFoodNetwork end end end -# rubocop:enable Metrics/ClassLength