Commit 101a2617c4d5254b228d7d2abac5c887889a8388
1 parent
39eb02cb
Exists in
theme-brasil-digital-from-staging
and in
9 other branches
added sql injection protection to method make_order_with_parameters
Showing
2 changed files
with
37 additions
and
3 deletions
Show diff stats
lib/noosfero/api/helpers.rb
| @@ -160,8 +160,21 @@ require 'grape' | @@ -160,8 +160,21 @@ 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 | + field_name, direction = params[:order].split(' ') | ||
| 168 | + assoc = object.class.reflect_on_association(method.to_sym) | ||
| 169 | + if !field_name.blank? and assoc | ||
| 170 | + if assoc.klass.attribute_names.include? field_name | ||
| 171 | + if direction.present? and ['ASC','DESC'].include? direction.upcase | ||
| 172 | + order = "#{field_name} #{direction.upcase}" | ||
| 173 | + end | ||
| 174 | + end | ||
| 175 | + end | ||
| 176 | + end | ||
| 177 | + return order | ||
| 165 | end | 178 | end |
| 166 | 179 | ||
| 167 | def by_reference(scope, params) | 180 | def by_reference(scope, params) |
| @@ -176,7 +189,7 @@ require 'grape' | @@ -176,7 +189,7 @@ require 'grape' | ||
| 176 | 189 | ||
| 177 | def select_filtered_collection_of(object, method, params) | 190 | def select_filtered_collection_of(object, method, params) |
| 178 | conditions = make_conditions_with_parameter(params) | 191 | conditions = make_conditions_with_parameter(params) |
| 179 | - order = make_order_with_parameters(params) | 192 | + order = make_order_with_parameters(object,method,params) |
| 180 | 193 | ||
| 181 | objects = object.send(method) | 194 | objects = object.send(method) |
| 182 | objects = by_reference(objects, params) | 195 | objects = by_reference(objects, params) |
test/unit/api/helpers_test.rb
| @@ -161,6 +161,27 @@ class APIHelpersTest < ActiveSupport::TestCase | @@ -161,6 +161,27 @@ 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 | + | ||
| 164 | 185 | ||
| 165 | should 'render not_found if endpoint is unavailable' do | 186 | should 'render not_found if endpoint is unavailable' do |
| 166 | Noosfero::API::API.stubs(:endpoint_unavailable?).returns(true) | 187 | Noosfero::API::API.stubs(:endpoint_unavailable?).returns(true) |