diff --git a/changelog/new_add_new_rails_strong_parameters_expect_cop.md b/changelog/new_add_new_rails_strong_parameters_expect_cop.md new file mode 100644 index 0000000000..01d7ab8561 --- /dev/null +++ b/changelog/new_add_new_rails_strong_parameters_expect_cop.md @@ -0,0 +1 @@ +* [#1358](https://github.com/rubocop/rubocop-rails/issues/1358): Add new `Rails/StrongParametersExpect` cop. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 71a9a0ebaf..8aba7bcca1 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1081,6 +1081,15 @@ Rails/StripHeredoc: Enabled: pending VersionAdded: '2.15' +Rails/StrongParametersExpect: + Description: 'Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling.' + Reference: 'https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect' + Enabled: pending + Include: + - app/controllers/**/*.rb + SafeAutoCorrect: false + VersionAdded: '<>' + Rails/TableNameAssignment: Description: >- Do not use `self.table_name =`. Use Inflections or `table_name_prefix` instead. diff --git a/lib/rubocop/cop/rails/strong_parameters_expect.rb b/lib/rubocop/cop/rails/strong_parameters_expect.rb new file mode 100644 index 0000000000..842523f629 --- /dev/null +++ b/lib/rubocop/cop/rails/strong_parameters_expect.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling. + # + # @safety + # This cop's autocorrection is considered unsafe because there are cases where the HTTP status may change + # from 500 to 400 when handling invalid parameters. This change, however, reflects an intentional + # incompatibility introduced for valid reasons by the `expect` method, which aligns better with + # strong parameter conventions. + # + # @example + # + # # bad + # params.require(:user).permit(:name, :age) + # params.permit(user: [:name, :age]).require(:user) + # + # # good + # params.expect(user: [:name, :age]) + # + class StrongParametersExpect < Base + extend AutoCorrector + extend TargetRailsVersion + + MSG = 'Use `%s` instead.' + RESTRICT_ON_SEND = %i[require permit].freeze + + minimum_target_rails_version 8.0 + + def_node_matcher :params_require_permit, <<~PATTERN + $(call + $(call + (send nil? :params) :require _) :permit ...) + PATTERN + + def_node_matcher :params_permit_require, <<~PATTERN + $(call + $(call + (send nil? :params) :permit (hash (pair _require_param_name _ ))) + :require _require_param_name) + PATTERN + + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def on_send(node) + return if part_of_ignored_node?(node) + + if (permit_method, require_method = params_require_permit(node)) + range = offense_range(require_method, node) + prefer = expect_method(require_method, permit_method) + replace_argument = true + elsif (require_method, permit_method = params_permit_require(node)) + range = offense_range(permit_method, node) + prefer = "expect(#{permit_method.arguments.map(&:source).join(', ')})" + replace_argument = false + else + return + end + + add_offense(range, message: format(MSG, prefer: prefer)) do |corrector| + corrector.remove(require_method.loc.dot.join(require_method.source_range.end)) + corrector.replace(permit_method.loc.selector, 'expect') + if replace_argument + corrector.insert_before(permit_method.first_argument, "#{require_key(require_method)}[") + corrector.insert_after(permit_method.last_argument, ']') + end + end + + ignore_node(node) + end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + alias on_csend on_send + + private + + def offense_range(method_node, node) + method_node.loc.selector.join(node.source_range.end) + end + + def expect_method(require_method, permit_method) + require_key = require_key(require_method) + permit_args = permit_method.arguments.map(&:source).join(', ') + + arguments = "#{require_key}[#{permit_args}]" + + "expect(#{arguments})" + end + + def require_key(require_method) + if (first_argument = require_method.first_argument).respond_to?(:value) + require_arg = first_argument.value + separator = ': ' + else + require_arg = first_argument.source + separator = ' => ' + end + + "#{require_arg}#{separator}" + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 047fd87bde..d3b24ec9e9 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -11,6 +11,7 @@ require_relative 'mixin/target_rails_version' require_relative 'rails/action_controller_flash_before_render' +require_relative 'rails/strong_parameters_expect' require_relative 'rails/action_controller_test_case' require_relative 'rails/action_filter' require_relative 'rails/action_order' diff --git a/spec/rubocop/cop/rails/strong_parameters_expect_spec.rb b/spec/rubocop/cop/rails/strong_parameters_expect_spec.rb new file mode 100644 index 0000000000..f618b9dd78 --- /dev/null +++ b/spec/rubocop/cop/rails/strong_parameters_expect_spec.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::StrongParametersExpect, :config do + context 'Rails >= 8.0', :rails80 do + it 'registers an offense when using `params.require(:user).permit(:name, :age)`' do + expect_offense(<<~RUBY) + params.require(:user).permit(:name, :age) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead. + RUBY + + expect_correction(<<~RUBY) + params.expect(user: [:name, :age]) + RUBY + end + + it 'registers an offense when using `params&.require(:user)&.permit(:name, :age)`' do + expect_offense(<<~RUBY) + params&.require(:user)&.permit(:name, :age) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead. + RUBY + + expect_correction(<<~RUBY) + params&.expect(user: [:name, :age]) + RUBY + end + + it 'registers an offense when using `params.permit(user: [:name, :age]).require(:user)`' do + expect_offense(<<~RUBY) + params.permit(user: [:name, :age]).require(:user) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead. + RUBY + + expect_correction(<<~RUBY) + params.expect(user: [:name, :age]) + RUBY + end + + it 'registers an offense when using `params&.permit(user: [:name, :age])&.require(:user)`' do + expect_offense(<<~RUBY) + params&.permit(user: [:name, :age])&.require(:user) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead. + RUBY + + expect_correction(<<~RUBY) + params&.expect(user: [:name, :age]) + RUBY + end + + it 'registers an offense when using `params.require(:user).permit(:name, some_ids: [])`' do + expect_offense(<<~RUBY) + params.require(:user).permit(:name, some_ids: []) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, some_ids: []])` instead. + RUBY + + expect_correction(<<~RUBY) + params.expect(user: [:name, some_ids: []]) + RUBY + end + + it 'registers an offense when using `params.require(:user).permit(*parameters, some_ids: [])`' do + expect_offense(<<~RUBY) + params.require(:user).permit(*parameters, some_ids: []) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [*parameters, some_ids: []])` instead. + RUBY + + expect_correction(<<~RUBY) + params.expect(user: [*parameters, some_ids: []]) + RUBY + end + + it 'registers an offense when using `params.require(var).permit(:name, some_ids: [])`' do + expect_offense(<<~RUBY) + var = :user + params.require(var).permit(:name, some_ids: []) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(var => [:name, some_ids: []])` instead. + RUBY + + expect_correction(<<~RUBY) + var = :user + params.expect(var => [:name, some_ids: []]) + RUBY + end + + it "registers an offense when using `params.require(:user).permit(:name, :age)` and `permit`'s args has comment" do + expect_offense(<<~RUBY) + params.require(:user).permit( + ^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead. + :name, # comment + :age # comment + ) + RUBY + + expect_correction(<<~RUBY) + params.expect( + user: [:name, # comment + :age] # comment + ) + RUBY + end + + it 'does not register an offense when using `params.expect(user: [:name, :age])`' do + expect_no_offenses(<<~RUBY) + params.expect(user: [:name, :age]) + RUBY + end + + it 'does not register an offense when using `params.permit(unmatch_require_param: [:name, :age]).require(:user)`' do + expect_no_offenses(<<~RUBY) + params.permit(unmatch_require_param: [:name, :age]).require(:user) + RUBY + end + + it 'does not register an offense when using `params.require(:name)`' do + expect_no_offenses(<<~RUBY) + params.require(:name) + RUBY + end + + it 'does not register an offense when using `params.permit(:name)`' do + expect_no_offenses(<<~RUBY) + params.permit(:name) + RUBY + end + + it 'does not register an offense when using `params[:name]`' do + expect_no_offenses(<<~RUBY) + params[:name] + RUBY + end + + it 'does not register an offense when using `params.fetch(:name)`' do + expect_no_offenses(<<~RUBY) + params.fertch(:name) + RUBY + end + + it 'does not register an offense when using `params[:user][:name]`' do + expect_no_offenses(<<~RUBY) + params[:user][:name] + RUBY + end + end + + context 'Rails <= 7.2', :rails72 do + it 'does not register an offense when using `params.require(:user).permit(:name, :age)`' do + expect_no_offenses(<<~RUBY) + params.require(:user).permit(:name, :age) + RUBY + end + end +end