Download as
Browse Code »

Commit 406255f3879cce9388cd488badb074701dc15d28

Authored by Evandro Junior
2 parents 3a14efd3 16147043
Exists in theme-brasil-digital-from-staging and in 9 other branches angular_poc, api-article-archived, backup-7-jan-2016, captcha_serpro_plugin, login-captcha, production, resend_confirmation_email, staging, staging_rails3

Merge branch 'api' of gitlab.com:participa/noosfero into api 

* 'api' of gitlab.com:participa/noosfero:
  added random as a possible value for make_order_with_parameters
  added sql injection protection to method make_order_with_parameters
  fix to sql injections vulnerabilities identified using brakeman
Showing 7 changed files with 98 additions and 13 deletions   Show diff stats
app/controllers/my_profile/manage_products_controller.rb
  Diff comments   View file @406255f
@@ -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 @406255f
@@ -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 @406255f
@@ -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 @406255f
@@ -288,9 +288,34 @@ class Task < ActiveRecord::Base @@ -288,9 +288,34 @@ class Task < ActiveRecord::Base
288 scope :canceled, :conditions => { :status => Task::Status::CANCELLED } 288 scope :canceled, :conditions => { :status => Task::Status::CANCELLED }
289 scope :closed, :conditions => { :status => [Task::Status::CANCELLED, Task::Status::FINISHED] } 289 scope :closed, :conditions => { :status => [Task::Status::CANCELLED, Task::Status::FINISHED] }
290 scope :opened, :conditions => { :status => [Task::Status::ACTIVE, Task::Status::HIDDEN] } 290 scope :opened, :conditions => { :status => [Task::Status::ACTIVE, Task::Status::HIDDEN] }
291 - scope :of, lambda { |type| conditions = type ? "type LIKE '#{type}'" : "1=1"; {:conditions => [conditions]} }  
292 - scope :order_by, lambda { |attribute, ord| {:order => "#{attribute} #{ord}"} }  
293 - scope :like, lambda { |field, value| where("LOWER(#{field}) LIKE ?", "%#{value.downcase}%") if value} 291 +
  292 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  293 + def self.of type
  294 + if type
  295 + where "type LIKE ?", type
  296 + else
  297 + all
  298 + end
  299 + end
  300 +
  301 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  302 + def self.order_by attribute_name, sort_order
  303 + if Task.column_names.include? attribute_name
  304 + # TODO future versions of rails accepts a hash as param to order method
  305 + # which helps to prevent sql injection in an shorter way
  306 + sort_order_filtered = ("ASC".eql? "#{sort_order}".upcase) ? 'asc' : 'desc'
  307 + sort_expression = Task.column_names.collect {|column_name| "#{column_name} #{sort_order_filtered}" if column_name.eql? attribute_name}
  308 + order(sort_expression.join) unless sort_expression.join.empty?
  309 + end
  310 + end
  311 +
  312 + # updated scope method to avoid sql injection vunerabillity (http://brakemanscanner.org/docs/warning_types/sql_injection/)
  313 + def self.like field, value
  314 + if value and Tasks.column_names.include? field
  315 + where("LOWER(?) LIKE ?", "#{field}", "%#{value.downcase}%")
  316 + end
  317 + end
  318 +
294 scope :pending_all, lambda { |profile, filter_type, filter_text| 319 scope :pending_all, lambda { |profile, filter_type, filter_text|
295 self.to(profile).without_spam.pending.of(filter_type).like('data', filter_text) 320 self.to(profile).without_spam.pending.of(filter_type).like('data', filter_text)
296 } 321 }
lib/activities_counter_cache_job.rb
  Diff comments   View file @406255f
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
lib/noosfero/api/helpers.rb
  Diff comments   View file @406255f
@@ -160,8 +160,27 @@ require 'grape' @@ -160,8 +160,27 @@ require 'grape'
160 conditions 160 conditions
161 end 161 end
162 162
163 - def make_order_with_parameters(params)  
164 - params[:order] || "created_at DESC" 163 + # changing make_order_with_parameters to avoid sql injection
  164 + def make_order_with_parameters(object, method, params)
  165 + order = "created_at DESC"
  166 + unless params[:order].blank?
  167 + if params[:order].include? '\'' or params[:order].include? '"'
  168 + order = "created_at DESC"
  169 + elsif ['RANDOM()', 'RANDOM'].include? params[:order].upcase
  170 + order = 'RANDOM()'
  171 + else
  172 + field_name, direction = params[:order].split(' ')
  173 + assoc = object.class.reflect_on_association(method.to_sym)
  174 + if !field_name.blank? and assoc
  175 + if assoc.klass.attribute_names.include? field_name
  176 + if direction.present? and ['ASC','DESC'].include? direction.upcase
  177 + order = "#{field_name} #{direction.upcase}"
  178 + end
  179 + end
  180 + end
  181 + end
  182 + end
  183 + return order
165 end 184 end
166 185
167 def by_reference(scope, params) 186 def by_reference(scope, params)
@@ -176,7 +195,7 @@ require 'grape' @@ -176,7 +195,7 @@ require 'grape'
176 195
177 def select_filtered_collection_of(object, method, params) 196 def select_filtered_collection_of(object, method, params)
178 conditions = make_conditions_with_parameter(params) 197 conditions = make_conditions_with_parameter(params)
179 - order = make_order_with_parameters(params) 198 + order = make_order_with_parameters(object,method,params)
180 199
181 objects = object.send(method) 200 objects = object.send(method)
182 objects = by_reference(objects, params) 201 objects = by_reference(objects, params)
test/unit/api/helpers_test.rb
  Diff comments   View file @406255f
@@ -161,6 +161,38 @@ class APIHelpersTest < ActiveSupport::TestCase @@ -161,6 +161,38 @@ class APIHelpersTest < ActiveSupport::TestCase
161 assert_nil make_conditions_with_parameter[:type] 161 assert_nil make_conditions_with_parameter[:type]
162 end 162 end
163 163
  164 + #test_should_make_order_with_parameters_return_order_if attribute_is_found_at_object_association
  165 + should 'make_order_with_parameters return order if attribute is found at object association' do
  166 + environment = Environment.new
  167 + params = {:order => "name ASC"}
  168 + assert_equal "name ASC", make_order_with_parameters(environment, "articles", params)
  169 + end
  170 +
  171 + # test added to check for eventual sql injection vunerabillity
  172 + #test_should_make_order_with_parameters_return_default_order_if_attributes_not_exists
  173 + should 'make_order_with_parameters return default order if attributes not exists' do
  174 + environment = Environment.new
  175 + params = {:order => "CRAZY_FIELD ASC"} # quote used to check sql injection vunerabillity
  176 + assert_equal "created_at DESC", make_order_with_parameters(environment, "articles", params)
  177 + end
  178 +
  179 + should 'make_order_with_parameters return default order if sql injection detected' do
  180 + environment = Environment.new
  181 + params = {:order => "name' ASC"} # quote used to check sql injection vunerabillity
  182 + assert_equal "created_at DESC", make_order_with_parameters(environment, "articles", params)
  183 + end
  184 +
  185 + should 'make_order_with_parameters return RANDOM() if random is passed' do
  186 + environment = Environment.new
  187 + params = {:order => "random"} # quote used to check sql injection vunerabillity
  188 + assert_equal "RANDOM()", make_order_with_parameters(environment, "articles", params)
  189 + end
  190 +
  191 + should 'make_order_with_parameters return RANDOM() if random function is passed' do
  192 + environment = Environment.new
  193 + params = {:order => "random()"} # quote used to check sql injection vunerabillity
  194 + assert_equal "RANDOM()", make_order_with_parameters(environment, "articles", params)
  195 + end
164 196
165 should 'render not_found if endpoint is unavailable' do 197 should 'render not_found if endpoint is unavailable' do
166 Noosfero::API::API.stubs(:endpoint_unavailable?).returns(true) 198 Noosfero::API::API.stubs(:endpoint_unavailable?).returns(true)