Commit dfbe22ffc6ee6aceb585f757c186a4bf3fb010ca
1 parent
19c4165d
Exists in
master
and in
2 other branches
Added basic protection from unfiltered parameters to where option [skip ci]
Showing
3 changed files
with
43 additions
and
13 deletions
Show diff stats
CHANGELOG.md
| 1 | ## 5.0.0 (unreleased) | 1 | ## 5.0.0 (unreleased) |
| 2 | 2 | ||
| 3 | - Searches now use lazy loading (similar to Active Record) | 3 | - Searches now use lazy loading (similar to Active Record) |
| 4 | +- Added basic protection from unfiltered parameters to `where` option | ||
| 4 | - Anchor regular expressions by default | 5 | - Anchor regular expressions by default |
| 5 | - Raise error when `search` called on relations | 6 | - Raise error when `search` called on relations |
| 6 | - Raise `ArgumentError` (instead of warning) for invalid regular expression modifiers | 7 | - Raise `ArgumentError` (instead of warning) for invalid regular expression modifiers |
lib/searchkick/query.rb
| @@ -435,7 +435,7 @@ module Searchkick | @@ -435,7 +435,7 @@ module Searchkick | ||
| 435 | payload = {} | 435 | payload = {} |
| 436 | 436 | ||
| 437 | # type when inheritance | 437 | # type when inheritance |
| 438 | - where = (options[:where] || {}).dup | 438 | + where = ensure_permitted(options[:where] || {}).dup |
| 439 | if searchkick_options[:inheritance] && (options[:type] || (klass != searchkick_klass && searchkick_index)) | 439 | if searchkick_options[:inheritance] && (options[:type] || (klass != searchkick_klass && searchkick_index)) |
| 440 | where[:type] = [options[:type] || klass].flatten.map { |v| searchkick_index.klass_document_type(v, true) } | 440 | where[:type] = [options[:type] || klass].flatten.map { |v| searchkick_index.klass_document_type(v, true) } |
| 441 | end | 441 | end |
| @@ -832,8 +832,9 @@ module Searchkick | @@ -832,8 +832,9 @@ module Searchkick | ||
| 832 | end | 832 | end |
| 833 | 833 | ||
| 834 | where = {} | 834 | where = {} |
| 835 | - where = (options[:where] || {}).reject { |k| k == field } unless options[:smart_aggs] == false | ||
| 836 | - agg_filters = where_filters(where.merge(agg_options[:where] || {})) | 835 | + where = ensure_permitted(options[:where] || {}).reject { |k| k == field } unless options[:smart_aggs] == false |
| 836 | + agg_where = ensure_permitted(agg_options[:where] || {}) | ||
| 837 | + agg_filters = where_filters(where.merge(agg_where)) | ||
| 837 | 838 | ||
| 838 | # only do one level comparison for simplicity | 839 | # only do one level comparison for simplicity |
| 839 | filters.select! do |filter| | 840 | filters.select! do |filter| |
| @@ -872,12 +873,13 @@ module Searchkick | @@ -872,12 +873,13 @@ module Searchkick | ||
| 872 | payload[:sort] = options[:order].is_a?(Enumerable) ? options[:order] : {options[:order] => :asc} | 873 | payload[:sort] = options[:order].is_a?(Enumerable) ? options[:order] : {options[:order] => :asc} |
| 873 | end | 874 | end |
| 874 | 875 | ||
| 875 | - def where_filters(where) | ||
| 876 | - # if where.respond_to?(:permitted?) && !where.permitted? | ||
| 877 | - # # TODO check in more places | ||
| 878 | - # Searchkick.warn("Passing unpermitted parameters will raise an exception in Searchkick 5") | ||
| 879 | - # end | 876 | + # provides *very* basic protection from unfiltered parameters |
| 877 | + # this is not meant to be comprehensive and may be expanded in the future | ||
| 878 | + def ensure_permitted(obj) | ||
| 879 | + obj.to_h | ||
| 880 | + end | ||
| 880 | 881 | ||
| 882 | + def where_filters(where) | ||
| 881 | filters = [] | 883 | filters = [] |
| 882 | (where || {}).each do |field, value| | 884 | (where || {}).each do |field, value| |
| 883 | field = :_id if field.to_s == "id" | 885 | field = :_id if field.to_s == "id" |
test/parameters_test.rb
| @@ -6,17 +6,30 @@ class ParametersTest < Minitest::Test | @@ -6,17 +6,30 @@ class ParametersTest < Minitest::Test | ||
| 6 | super | 6 | super |
| 7 | end | 7 | end |
| 8 | 8 | ||
| 9 | - def test_where_unpermitted | ||
| 10 | - # TODO raise error in Searchkick 6 | ||
| 11 | - store [{name: "Product A", store_id: 1}, {name: "Product B", store_id: 2}] | 9 | + def test_options |
| 12 | params = ActionController::Parameters.new({store_id: 1}) | 10 | params = ActionController::Parameters.new({store_id: 1}) |
| 13 | - assert_search "product", ["Product A"], where: params | 11 | + assert_raises(ActionController::UnfilteredParameters) do |
| 12 | + Product.search("*", **params) | ||
| 13 | + end | ||
| 14 | + end | ||
| 15 | + | ||
| 16 | + def test_where | ||
| 17 | + params = ActionController::Parameters.new({store_id: 1}) | ||
| 18 | + assert_raises(ActionController::UnfilteredParameters) do | ||
| 19 | + Product.search("*", where: params) | ||
| 20 | + end | ||
| 14 | end | 21 | end |
| 15 | 22 | ||
| 16 | def test_where_permitted | 23 | def test_where_permitted |
| 17 | store [{name: "Product A", store_id: 1}, {name: "Product B", store_id: 2}] | 24 | store [{name: "Product A", store_id: 1}, {name: "Product B", store_id: 2}] |
| 18 | params = ActionController::Parameters.new({store_id: 1}) | 25 | params = ActionController::Parameters.new({store_id: 1}) |
| 19 | - assert_search "product", ["Product A"], where: params.permit! | 26 | + assert_search "product", ["Product A"], where: params.permit(:store_id) |
| 27 | + end | ||
| 28 | + | ||
| 29 | + def test_where_value | ||
| 30 | + store [{name: "Product A", store_id: 1}, {name: "Product B", store_id: 2}] | ||
| 31 | + params = ActionController::Parameters.new({store_id: 1}) | ||
| 32 | + assert_search "product", ["Product A"], where: {store_id: params[:store_id]} | ||
| 20 | end | 33 | end |
| 21 | 34 | ||
| 22 | def test_where_hash | 35 | def test_where_hash |
| @@ -26,4 +39,18 @@ class ParametersTest < Minitest::Test | @@ -26,4 +39,18 @@ class ParametersTest < Minitest::Test | ||
| 26 | end | 39 | end |
| 27 | assert_equal error.message, "can't cast ActionController::Parameters" | 40 | assert_equal error.message, "can't cast ActionController::Parameters" |
| 28 | end | 41 | end |
| 42 | + | ||
| 43 | + def test_aggs_where | ||
| 44 | + params = ActionController::Parameters.new({store_id: 1}) | ||
| 45 | + assert_raises(ActionController::UnfilteredParameters) do | ||
| 46 | + Product.search("*", aggs: {size: {where: params}}) | ||
| 47 | + end | ||
| 48 | + end | ||
| 49 | + | ||
| 50 | + def test_aggs_where_smart_aggs_false | ||
| 51 | + params = ActionController::Parameters.new({store_id: 1}) | ||
| 52 | + assert_raises(ActionController::UnfilteredParameters) do | ||
| 53 | + Product.search("*", aggs: {size: {where: params}}, smart_aggs: false) | ||
| 54 | + end | ||
| 55 | + end | ||
| 29 | end | 56 | end |