Fixes #22285 - Raise error when strong params filters out params

app/controllers/api/base_controller.rb

@@ -44,6 +44,13 @@ class BaseController < ActionController::Base
       not_found error.message
     end
 
+    rescue_from ActionController::UnpermittedParameters do |error|
+      logger.info "#{error.message} (#{error.class})"
+      message = n_("Invalid parameter: %s. ", "Invalid parameters: %s. ", error.params.length) % error.params.to_sentence
+      message << _('Please verify that the parameter name is valid and the values are the correct type.')
+      render_error 'param_error', :status => :bad_request, :locals => { :exception => error, :message => message }
+    end
+
     rescue_from Foreman::MaintenanceException, :with => :service_unavailable
 
     def get_resource(message = "Couldn't find resource")

app/controllers/application_controller.rb

@@ -9,6 +9,7 @@ class ApplicationController < ActionController::Base
   rescue_from ScopedSearch::QueryNotSupported, :with => :invalid_search_query
   rescue_from ActiveRecord::RecordNotFound, :with => :not_found
   rescue_from ProxyAPI::ProxyException, :with => :smart_proxy_exception
+  rescue_from ActionController::UnpermittedParameters, :with => :invalid_parameter
   rescue_from Foreman::MaintenanceException, :with => :service_unavailable
 
   # standard layout to all controllers
@@ -123,6 +124,14 @@ def smart_proxy_exception(exception = nil)
     end
   end
 
+  def invalid_parameter(exception = nil)
+    logger.debug "Strong parameters filtered parameters: #{exception.params}" if exception
+    respond_to do |format|
+      format.html { render "common/400", :status => :bad_request, :locals => { :exception => exception } }
+      format.any { head :bad_request }
+    end
+  end
+
   # this method sets the Current user to be the Admin
   # its required for actions which are not authenticated by default
   # such as unattended notifications coming from an OS, or fact and reports creations

app/views/api/v2/errors/param_error.json.rabl

@@ -2,6 +2,9 @@ exception = locals[:exception]
 
 object exception => :error
 
-attributes :message
+node :message do
+  locals[:message] || exception.message
+end
 node(:class) { exception.class.to_s }
 node(:parameter_name) { exception.param } if exception.respond_to? :param
+node(:parameter_names) { exception.params } if exception.respond_to? :params

app/views/common/400.html.erb

@@ -0,0 +1,7 @@
+<div class='col-md-offset-4 col-md-4'>
+  <%= alert :header => (n_("Invalid parameter: %s", "Invalid parameters: %s", exception.params.length) % exception.params.to_sentence),
+    :text => _('Please verify that the parameter name is valid and the values are the correct type.'),
+    :actions => link_to(_('Back'), main_app.root_path, :class => 'btn btn-default'),
+    :class => 'alert-warning',
+    :close => false %>
+</div>

config/application.rb

@@ -136,7 +136,10 @@ class Application < Rails::Application
     # Enable escaping HTML in JSON.
     config.active_support.escape_html_entities_in_json = true
 
-    # Don't raise exception for common parameters
+    # Raise exception on mass assignment of unfiltered parameters
+    config.action_controller.action_on_unpermitted_parameters = :raise
+
+    # Don't raise exception for common parameters (despite the name, they aren't permitted)
     config.action_controller.always_permitted_parameters = %w(
       controller action format locale utf8 _method authenticity_token commit redirect
       page per_page paginate search order sort sort_by sort_order

config/environments/development.rb

@@ -38,9 +38,6 @@
   # Raises helpful error messages.
   config.assets.raise_runtime_errors = true
 
-  # Raise exception on mass assignment of unfiltered parameters
-  config.action_controller.action_on_unpermitted_parameters = :strict
-
   config.after_initialize do
     Bullet.enable = true
     Bullet.bullet_logger = true

config/environments/production.rb

@@ -80,7 +80,4 @@
   config.active_record.dump_schema_after_migration = false
 
   config.webpack.dev_server.enabled = false
-
-  # Log denied attributes into logger
-  config.action_controller.action_on_unpermitted_parameters = :log
 end

config/environments/test.rb

@@ -54,9 +54,6 @@
   # Print deprecation notices to the stderr.
   config.active_support.deprecation = :stderr
 
-  # Raise exception on mass assignment of unfiltered parameters
-  config.action_controller.action_on_unpermitted_parameters = :strict
-
   # Use separate cache stores for parallel_tests
   config.cache_store = :file_store, Rails.root.join("tmp", "cache", "paralleltests#{ENV['TEST_ENV_NUMBER']}")
 

test/controllers/api/v2/auth_source_ldaps_controller_test.rb

@@ -71,14 +71,20 @@ class Api::V2::AuthSourceLdapsControllerTest < ActionController::TestCase
   end
 
   test 'taxonomies can be set' do
-    put :update, params: { :id => auth_sources(:one).to_param,
-                           :organization_names => [taxonomies(:organization1).name],
-                           :location_ids => [taxonomies(:location1).id] }
+    auth_source = FactoryBot.create(:auth_source_ldap)
+    assert_empty auth_source.organizations
+    assert_empty auth_source.locations
+    org = FactoryBot.create(:organization)
+    loc = FactoryBot.create(:location)
+    put :update, params: { :id => auth_source.to_param,
+                           :organization_names => [org.name],
+                           :location_ids => [loc.id]
+                         }
     show_response = ActiveSupport::JSON.decode(@response.body)
     assert_response :success
-    assert_equal taxonomies(:location1).id,
-      show_response['locations'].first['id']
-    assert_equal taxonomies(:organization1).id,
-      show_response['organizations'].first['id']
+    assert_equal loc.id, show_response['locations'].first['id']
+    assert_equal org.id, show_response['organizations'].first['id']
+    assert_equal [org], auth_source.organizations
+    assert_equal [loc], auth_source.locations
   end
 end

test/controllers/api/v2/users_controller_test.rb

@@ -3,7 +3,7 @@
 class Api::V2::UsersControllerTest < ActionController::TestCase
   def valid_attrs
     { :mail => 'john@example.com',
-      :auth_source_id => auth_sources(:internal), :password => '123456' }
+      :password => '123456' }
   end
 
   def min_valid_attrs
@@ -72,7 +72,7 @@ def setup
 
   test "should update user" do
     user = User.create :login => "foo", :mail => "foo@bar.com", :auth_source => auth_sources(:one)
-    put :update, params: { :id => user.id, :user => valid_attrs }
+    put :update, params: { :id => user.id, :user => valid_attrs.merge(:auth_source_id => auth_sources(:internal).id)}
     assert_response :success
 
     mod_user = User.unscoped.find_by_id(user.id)

test/controllers/auth_source_ldaps_controller_test.rb

@@ -81,6 +81,7 @@ def test_destroy
     as_admin do
       put :update, params: { :commit => "Update", :id => auth_source_ldap.id, :auth_source_ldap => {:name => auth_source_ldap.name} }, session: set_session_user
     end
+    assert_response :redirect
     auth_source_ldap = AuthSourceLdap.unscoped.find(auth_source_ldap.id)
     assert_equal old_pass, auth_source_ldap.account_password
   end
@@ -90,6 +91,7 @@ def test_destroy
     as_admin do
       put :update, params: { :commit => "Update", :id => auth_source_ldap.id, :auth_source_ldap => {:account_password => '', :name => auth_source_ldap.name} }, session: set_session_user
     end
+    assert_response :redirect
     auth_source_ldap = AuthSourceLdap.find(auth_source_ldap.id)
     assert_empty auth_source_ldap.account_password
   end

test/controllers/concerns/parameters/auth_source_ldap_test.rb

@@ -7,6 +7,8 @@ class AuthSourceLdapParametersTest < ActiveSupport::TestCase
 
   test "filters STI :type field" do
     params = ActionController::Parameters.new(:auth_source_ldap => {:type => AuthSourceHidden.name})
-    refute_includes self.class.auth_source_ldap_params_filter.filter_params(params, context), 'type'
+    assert_raises ActionController::UnpermittedParameters do
+      self.class.auth_source_ldap_params_filter.filter_params(params, context)
+    end
   end
 end

test/controllers/concerns/parameters/user_test.rb

@@ -18,7 +18,9 @@ class UserParametersTest < ActiveSupport::TestCase
     test "blocks :admin if current user is not an admin" do
       params = ActionController::Parameters.new(:user => {:admin => true})
       as_user(FactoryBot.create(:user)) do
-        refute_includes self.class.user_params_filter.filter_params(params, context), 'admin'
+        assert_raises ActionController::UnpermittedParameters do
+          self.class.user_params_filter.filter_params(params, context)
+        end
       end
     end
 
@@ -37,7 +39,9 @@ class UserParametersTest < ActiveSupport::TestCase
     test "blocks role attributes" do
       params = ActionController::Parameters.new(:user => {:roles => ['a'], :role_ids => [1], :role_names => ['a']})
       as_user(FactoryBot.create(:user)) do
-        assert_empty self.class.user_params_filter.filter_params(params, context)
+        assert_raises ActionController::UnpermittedParameters do
+          self.class.user_params_filter.filter_params(params, context)
+        end
       end
     end
   end

test/unit/parameter_filter_test.rb

@@ -11,31 +11,41 @@ class ParameterFilterTest < ActiveSupport::TestCase
 
   test "permitting second-level attributes via permit(Symbol)" do
     filter.permit(:test)
-    assert_equal({'test' => 'a'}, filter.filter_params(params(:example => {:test => 'a', :denied => 'b'}), ui_context).to_h)
+    assert_equal({'test' => 'a'}, filter.filter_params(params(:example => {:test => 'a'}), ui_context).to_h)
+  end
+
+  test "block second-level attributes" do
+    filter.permit(:test)
+    assert_raises ActionController::UnpermittedParameters do
+      filter.filter_params(params(:example => {:test => 'a', :denied => 'b'}), ui_context)
+    end
   end
 
   test "permitting second-level attributes via block" do
     filter.permit { |ctx| ctx.permit(:test) }
-    assert_equal({'test' => 'a'}, filter.filter_params(params(:example => {:test => 'a', :denied => 'b'}), ui_context).to_h)
+    assert_equal({'test' => 'a'}, filter.filter_params(params(:example => {:test => 'a'}), ui_context).to_h)
   end
 
   test "block contains controller/action names" do
     filter.permit do |ctx|
       ctx.controller_name == 'examples' || raise('controller is not "examples"')
       ctx.action == 'create' || raise('action is not "create"')
+      ctx.permit(:test)
     end
     filter.filter_params(params(:example => {:test => 'a'}), ui_context)
   end
 
   test "permitting second-level arrays via permit(Symbol => Array)" do
     filter.permit(:test => [])
-    assert_equal({}, filter.filter_params(params(:example => {:test => 'a'}), ui_context).to_h)
+    assert_raises ActionController::UnpermittedParameters do
+      filter.filter_params(params(:example => {:test => 'a'}), ui_context)
+    end
     assert_equal({'test' => ['a']}, filter.filter_params(params(:example => {:test => ['a']}), ui_context).to_h)
   end
 
   test "permitting third-level attributes via permit(Symbol => Array[Symbol])" do
     filter.permit(:test => [:inner])
-    assert_equal({'test' => {'inner' => 'a'}}, filter.filter_params(params(:example => {:test => {:inner => 'a', :denied => 'b'}}), ui_context).to_h)
+    assert_equal({'test' => {'inner' => 'a'}}, filter.filter_params(params(:example => {:test => {:inner => 'a'}}), ui_context).to_h)
   end
 
   test "constructs permit() args for second-level attribute" do
@@ -45,7 +55,9 @@ class ParameterFilterTest < ActiveSupport::TestCase
 
   test "blocks second-level attributes for UI when :ui => false" do
     filter.permit_by_context(:test, :ui => false)
-    assert_equal({}, filter.filter_params(params(:example => {:test => 'a'}), ui_context).to_h)
+    assert_raises ActionController::UnpermittedParameters do
+      filter.filter_params(params(:example => {:test => 'a'}), ui_context)
+    end
   end
 
   test "#permit_by_context raises error for unknown context types" do
@@ -76,14 +88,22 @@ class ParameterFilterTest < ActiveSupport::TestCase
       filter2.permit_by_context(:inner, :nested => true)
       filter2.permit(:ui_only)
       filter.permit(:test, :nested => [filter2])
-      assert_equal({'test' => 'a', 'nested' => [{'inner' => 'b'}]}, filter.filter_params(params(:example => {:test => 'a', :nested => [{:inner => 'b', :ui_only => 'b'}]}), ui_context).to_h)
-      assert_equal({'test' => 'a', 'nested' => {'123' => {'inner' => 'b'}}}, filter.filter_params(params(:example => {:test => 'a', :nested => {'123' => {:inner => 'b', :ui_only => 'b'}}}), ui_context).to_h)
+      assert_equal({'test' => 'a', 'nested' => [{'inner' => 'b'}]}, filter.filter_params(params(:example => {:test => 'a', :nested => [{:inner => 'b'}]}), ui_context).to_h)
+      assert_equal({'test' => 'a', 'nested' => {'123' => {'inner' => 'b'}}}, filter.filter_params(params(:example => {:test => 'a', :nested => {'123' => {:inner => 'b'}}}), ui_context).to_h)
+
+      assert_raises ActionController::UnpermittedParameters do
+        filter.filter_params(params(:example => {:test => 'a', :nested => {'123' => {:inner => 'b', :ui_only => 'b'}}}), ui_context)
+      end
+      assert_raises ActionController::UnpermittedParameters do
+        filter.filter_params(params(:example => {:test => 'a', :nested => [{:inner => 'b', :ui_only => 'b'}]}), ui_context)
+      end
     end
 
     test "second filter block has access to original controller/action" do
       filter2.permit do |ctx|
         ctx.controller_name == 'examples' || raise('controller is not "examples"')
         ctx.action == 'create' || raise('action is not "create"')
+        ctx.permit(:inner)
       end
       filter.permit(:test, :nested => [filter2])
       filter.filter_params(params(:example => {:test => 'a', :nested => [{:inner => 'b'}]}), ui_context)
@@ -95,30 +115,44 @@ class ParameterFilterTest < ActiveSupport::TestCase
       plugin = mock('plugin')
       plugin.expects(:parameter_filters).with(klass).returns([[:plugin_ext, :another]])
       Foreman::Plugin.expects(:all).returns([plugin])
-      assert_equal({'plugin_ext' => 'b'}, filter.filter_params(params(:example => {:test => 'a', :plugin_ext => 'b'}), ui_context).to_h)
+      assert_equal({'plugin_ext' => 'b'}, filter.filter_params(params(:example => {:plugin_ext => 'b'}), ui_context).to_h)
     end
 
     test "permits plugin-added attributes from blocks" do
       plugin = mock('plugin')
       rule = [Proc.new { |ctx| ctx.permit(:plugin_ext) }]
       plugin.expects(:parameter_filters).with(klass).returns([rule])
       Foreman::Plugin.expects(:all).returns([plugin])
-      assert_equal({'plugin_ext' => 'b'}, filter.filter_params(params(:example => {:test => 'a', :plugin_ext => 'b'}), ui_context).to_h)
+      assert_equal({'plugin_ext' => 'b'}, filter.filter_params(params(:example => {:plugin_ext => 'b'}), ui_context).to_h)
       refute_empty(rule)
     end
   end
 
   context "with top_level_hash" do
     test "applies filters without top-level hash" do
       filter.permit(:test)
-      assert_equal({'test' => 'a'}, filter.filter_params(params(:changed => {:test => 'a', :denied => 'b'}), ui_context, :changed).to_h)
+      assert_equal({'test' => 'a'}, filter.filter_params(params(:changed => {:test => 'a'}), ui_context, :changed).to_h)
+    end
+
+    test "block params without top-level hash" do
+      filter.permit(:test)
+      assert_raises ActionController::UnpermittedParameters do
+        filter.filter_params(params(:changed => {:test => 'a', :denied => 'b'}), ui_context, :changed)
+      end
     end
   end
 
   context "with top_level_hash => :none" do
     test "applies filters without top-level hash" do
       filter.permit(:test)
-      assert_equal({'test' => 'a'}, filter.filter_params(params(:test => 'a', :denied => 'b'), ui_context, :none).to_h)
+      assert_equal({'test' => 'a'}, filter.filter_params(params(:test => 'a'), ui_context, :none).to_h)
+    end
+
+    test "block params without top-level hash" do
+      filter.permit(:test)
+      assert_raises ActionController::UnpermittedParameters do
+        filter.filter_params(params(:test => 'a', :denied => 'b'), ui_context, :none)
+      end
     end
   end