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

Currently we silently filter out parameters (or log in production),
causing unexpected results when passing an invalid param (either
incorrect name or incorrect type). This can lead to unexpected results,
since the user, seeing no error, assumes the request was successful when
in fact some of the parameters were filtered out.

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

@@ -7,6 +7,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
@@ -122,6 +123,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,6 +136,9 @@ class Application < Rails::Application
     # Enable escaping HTML in JSON.
     config.active_support.escape_html_entities_in_json = true
 
+    # Raise exception on mass assignment of unfiltered parameters
+    config.action_controller.action_on_unpermitted_parameters = :raise
+
     # Use SQL instead of Active Record's schema dumper when creating the database.
     # This is necessary if your schema can't be completely dumped by the schema dumper,
     # like if you have constraints or database-specific column types

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']}")