From 725b9459d878b11d8d45fa12c99a06b400171574 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curoverse.com>
Date: Wed, 23 Apr 2014 21:02:07 -0400
Subject: [PATCH] Check sanity when applying filters, and provide useful error
 messages.

---
 .../app/controllers/application_controller.rb | 15 ++++++--
 .../test/integration/collections_api_test.rb  | 36 +++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 4b13fca1de..fcc7618fb6 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -144,9 +144,14 @@ class ApplicationController < ActionController::Base
     if @filters.is_a? Array and @filters.any?
       cond_out = []
       param_out = []
-      @filters.each do |attr, operator, operand|
-        if !model_class.searchable_columns(operator).index attr.to_s
-          raise ArgumentError.new("Invalid attribute '#{attr}' in condition")
+      @filters.each do |filter|
+        attr, operator, operand = filter
+        if !filter.is_a? Array
+          raise ArgumentError.new("Invalid element in filters array: #{filter.inspect} is not an array")
+        elsif !operator.is_a? String
+          raise ArgumentError.new("Invalid operator '#{operator}' (#{operator.class}) in filter")
+        elsif !model_class.searchable_columns(operator).index attr.to_s
+          raise ArgumentError.new("Invalid attribute '#{attr}' in filter")
         end
         case operator.downcase
         when '=', '<', '<=', '>', '>=', 'like'
@@ -159,11 +164,15 @@ class ApplicationController < ActionController::Base
               operand = Time.parse operand
             end
             param_out << operand
+          else
+            raise ArgumentError.new("Invalid operand type '#{operand.class}' for '#{operator}' operator in filter")
           end
         when 'in'
           if operand.is_a? Array
             cond_out << "#{table_name}.#{attr} IN (?)"
             param_out << operand
+          else
+            raise ArgumentError.new("Invalid argument '#{operand}' for 'in' operator in filter")
           end
         when 'is_a'
           operand = [operand] unless operand.is_a? Array
diff --git a/services/api/test/integration/collections_api_test.rb b/services/api/test/integration/collections_api_test.rb
index 5fa77c3db9..e13d336505 100644
--- a/services/api/test/integration/collections_api_test.rb
+++ b/services/api/test/integration/collections_api_test.rb
@@ -15,6 +15,42 @@ class CollectionsApiTest < ActionDispatch::IntegrationTest
     assert_equal "arvados#collectionList", jresponse['kind']
   end
 
+  test "get index with invalid filters (array of strings) responds 422" do
+    get "/arvados/v1/collections", {
+      :format => :json,
+      :filters => ['uuid', '=', 'ad02e37b6a7f45bbe2ead3c29a109b8a+54'].to_json
+    }, auth(:active)
+    assert_response 422
+    assert_match /nvalid element.*not an array/, jresponse['errors'].join(' ')
+  end
+
+  test "get index with invalid filters (unsearchable column) responds 422" do
+    get "/arvados/v1/collections", {
+      :format => :json,
+      :filters => [['this_column_does_not_exist', '=', 'bogus']].to_json
+    }, auth(:active)
+    assert_response 422
+    assert_match /nvalid attribute/, jresponse['errors'].join(' ')
+  end
+
+  test "get index with invalid filters (invalid operator) responds 422" do
+    get "/arvados/v1/collections", {
+      :format => :json,
+      :filters => [['uuid', ':-(', 'displeased']].to_json
+    }, auth(:active)
+    assert_response 422
+    assert_match /nvalid operator/, jresponse['errors'].join(' ')
+  end
+
+  test "get index with invalid filters (invalid operand type) responds 422" do
+    get "/arvados/v1/collections", {
+      :format => :json,
+      :filters => [['uuid', '=', {foo: 'bar'}]].to_json
+    }, auth(:active)
+    assert_response 422
+    assert_match /nvalid operand type/, jresponse['errors'].join(' ')
+  end
+
   test "get index with where= (empty string)" do
     get "/arvados/v1/collections", {:format => :json, :where => ''}, auth(:active)
     assert_response :success
-- 
2.30.2