1
Download as
Browse Code »

Commit 86d0f595d01d5d46dad5fd46cd3aae7eee7425c6

Authored by Ábner Silva de Oliveira
1 parent 7e5512ff
Exists in staging and in 4 other branches captcha_serpro_plugin, login-captcha, production, production-vendorized

fix to sql injections vulnerabilities identified using brakeman

Showing 5 changed files with 44 additions and 10 deletions   Show diff stats
app/controllers/my_profile/manage_products_controller.rb
  Diff comments   View file @86d0f59
@@ -206,7 +206,8 @@ class ManageProductsController < ApplicationController @@ -206,7 +206,8 @@ class ManageProductsController < ApplicationController
206 end 206 end
207 207
208 def certifiers_for_selection 208 def certifiers_for_selection
209 - @qualifier = Qualifier.exists?(params[:id]) ? Qualifier.find(params[:id]) : nil 209 + # updated to use hash as argument to exists? to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  210 + @qualifier = Qualifier.exists?(:id => params[:id]) ? Qualifier.find(params[:id]) : nil
210 render :update do |page| 211 render :update do |page|
211 page.replace_html params[:certifier_area], :partial => 'certifiers_for_selection' 212 page.replace_html params[:certifier_area], :partial => 'certifiers_for_selection'
212 end 213 end
app/controllers/public/contact_controller.rb
  Diff comments   View file @86d0f59
@@ -6,8 +6,9 @@ class ContactController < PublicController @@ -6,8 +6,9 @@ class ContactController < PublicController
6 def new 6 def new
7 @contact = build_contact 7 @contact = build_contact
8 if request.post? && params[:confirm] == 'true' 8 if request.post? && params[:confirm] == 'true'
9 - @contact.city = (!params[:city].blank? && City.exists?(params[:city])) ? City.find(params[:city]).name : nil  
10 - @contact.state = (!params[:state].blank? && State.exists?(params[:state])) ? State.find(params[:state]).name : nil 9 + # updated to use hash as argument to exists? to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  10 + @contact.city = (!params[:city].blank? && City.exists?(:id => params[:city])) ? City.find(params[:city]).name : nil
  11 + @contact.state = (!params[:state].blank? && State.exists?(:id => params[:state])) ? State.find(params[:state]).name : nil
11 if @contact.deliver 12 if @contact.deliver
12 session[:notice] = _('Contact successfully sent') 13 session[:notice] = _('Contact successfully sent')
13 redirect_to :action => 'new' 14 redirect_to :action => 'new'
app/models/product_category.rb
  Diff comments   View file @86d0f59
@@ -13,8 +13,11 @@ class ProductCategory < Category @@ -13,8 +13,11 @@ class ProductCategory < Category
13 scope :by_environment, lambda { |environment| { 13 scope :by_environment, lambda { |environment| {
14 :conditions => ['environment_id = ?', environment.id] 14 :conditions => ['environment_id = ?', environment.id]
15 }} 15 }}
  16 +
  17 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  18 + # explicited to_i on level argument
16 scope :unique_by_level, lambda { |level| { 19 scope :unique_by_level, lambda { |level| {
17 - :select => "DISTINCT ON (filtered_category) split_part(path, '/', #{level}) AS filtered_category, categories.*" 20 + :select => "DISTINCT ON (filtered_category) split_part(path, '/', #{level.to_i}) AS filtered_category, categories.*"
18 }} 21 }}
19 22
20 def all_products 23 def all_products
app/models/task.rb
  Diff comments   View file @86d0f59
@@ -242,9 +242,34 @@ class Task < ActiveRecord::Base @@ -242,9 +242,34 @@ class Task < ActiveRecord::Base
242 scope :canceled, :conditions => { :status => Task::Status::CANCELLED } 242 scope :canceled, :conditions => { :status => Task::Status::CANCELLED }
243 scope :closed, :conditions => { :status => [Task::Status::CANCELLED, Task::Status::FINISHED] } 243 scope :closed, :conditions => { :status => [Task::Status::CANCELLED, Task::Status::FINISHED] }
244 scope :opened, :conditions => { :status => [Task::Status::ACTIVE, Task::Status::HIDDEN] } 244 scope :opened, :conditions => { :status => [Task::Status::ACTIVE, Task::Status::HIDDEN] }
245 - scope :of, lambda { |type| conditions = type ? "type LIKE '#{type}'" : "1=1"; {:conditions => [conditions]} }  
246 - scope :order_by, lambda { |attribute, ord| {:order => "#{attribute} #{ord}"} }  
247 - scope :like, lambda { |field, value| where("LOWER(#{field}) LIKE ?", "%#{value.downcase}%") if value} 245 +
  246 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  247 + def self.of type
  248 + if type
  249 + where "type LIKE ?", type
  250 + else
  251 + all
  252 + end
  253 + end
  254 +
  255 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  256 + def self.order_by attribute_name, sort_order
  257 + if Task.column_names.include? attribute_name
  258 + # TODO future versions of rails accepts a hash as param to order method
  259 + # which helps to prevent sql injection in an shorter way
  260 + sort_order_filtered = ("ASC".eql? "#{sort_order}".upcase) ? 'asc' : 'desc'
  261 + sort_expression = Task.column_names.collect {|column_name| "#{column_name} #{sort_order_filtered}" if column_name.eql? attribute_name}
  262 + order(sort_expression.join) unless sort_expression.join.empty?
  263 + end
  264 + end
  265 +
  266 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  267 + def self.like field, value
  268 + if value and Tasks.column_names.include? field
  269 + where("LOWER(?) LIKE ?", "#{field}", "%#{value.downcase}%")
  270 + end
  271 + end
  272 +
248 scope :pending_all, lambda { |profile, filter_type, filter_text| 273 scope :pending_all, lambda { |profile, filter_type, filter_text|
249 self.to(profile).without_spam.pending.of(filter_type).like('data', filter_text) 274 self.to(profile).without_spam.pending.of(filter_type).like('data', filter_text)
250 } 275 }
lib/activities_counter_cache_job.rb
  Diff comments   View file @86d0f59
1 class ActivitiesCounterCacheJob 1 class ActivitiesCounterCacheJob
  2 +
  3 + # Changed to prevent sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
2 def perform 4 def perform
3 - person_activities_counts = ActiveRecord::Base.connection.execute("SELECT profiles.id, count(action_tracker.id) as count FROM profiles LEFT OUTER JOIN action_tracker ON profiles.id = action_tracker.user_id WHERE (action_tracker.created_at >= '#{ActionTracker::Record::RECENT_DELAY.days.ago.to_s(:db)}') AND ( (profiles.type = 'Person' ) ) GROUP BY profiles.id;")  
4 - organization_activities_counts = ActiveRecord::Base.connection.execute("SELECT profiles.id, count(action_tracker.id) as count FROM profiles LEFT OUTER JOIN action_tracker ON profiles.id = action_tracker.target_id WHERE (action_tracker.created_at >= '#{ActionTracker::Record::RECENT_DELAY.days.ago.to_s(:db)}') AND ( (profiles.type = 'Community' OR profiles.type = 'Enterprise' OR profiles.type = 'Organization' ) ) GROUP BY profiles.id;") 5 + person_activities_counts = ActiveRecord::Base.connection.execute("SELECT profiles.id, count(action_tracker.id) as count FROM profiles LEFT OUTER JOIN action_tracker ON profiles.id = action_tracker.user_id WHERE (action_tracker.created_at >= #{ActiveRecord::Base.connection.quote(ActionTracker::Record::RECENT_DELAY.days.ago.to_s(:db))}) AND ( (profiles.type = 'Person' ) ) GROUP BY profiles.id;")
  6 + organization_activities_counts = ActiveRecord::Base.connection.execute("SELECT profiles.id, count(action_tracker.id) as count FROM profiles LEFT OUTER JOIN action_tracker ON profiles.id = action_tracker.target_id WHERE (action_tracker.created_at >= #{ActiveRecord::Base.connection.quote(ActionTracker::Record::RECENT_DELAY.days.ago.to_s(:db))}) AND ( (profiles.type = 'Community' OR profiles.type = 'Enterprise' OR profiles.type = 'Organization' ) ) GROUP BY profiles.id;")
5 activities_counts = person_activities_counts.entries + organization_activities_counts.entries 7 activities_counts = person_activities_counts.entries + organization_activities_counts.entries
6 activities_counts.each do |count| 8 activities_counts.each do |count|
7 - ActiveRecord::Base.connection.execute("UPDATE profiles SET activities_count=#{count['count'].to_i} WHERE profiles.id=#{count['id']};") 9 + update_sql = ActiveRecord::Base.__send__(:sanitize_sql, ["UPDATE profiles SET activities_count=? WHERE profiles.id=?;", count['count'].to_i, count['id'] ], '')
  10 + ActiveRecord::Base.connection.execute(update_sql)
8 end 11 end
9 Delayed::Job.enqueue(ActivitiesCounterCacheJob.new, {:priority => -3, :run_at => 1.day.from_now}) 12 Delayed::Job.enqueue(ActivitiesCounterCacheJob.new, {:priority => -3, :run_at => 1.day.from_now})
10 end 13 end
  14 +
11 end 15 end