-
-
Notifications
You must be signed in to change notification settings - Fork 266
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add new
Rails/StrongParametersExpect
cop
## Summary This PR adds new `Rails/StrongParametersExpect` cop that enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling. As a starting point for this cop, the implementation in this PR will detect the following cases. ```ruby # bad params.require(:user).permit(:name, :age) params.permit(user: [:name, :age]).require(:user) # good params.expect(user: [:name, :age]) ``` ## 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. ## Additional Information This cop does not detect the following cases for the reasons outlined below. Consideration will be given to whether these should be provided as separate options. ### `params.permit` Incompatibilities occur with the returned object. ```ruby params = ActionController::Parameters.new(id: 42) # => #<ActionController::Parameters {"id"=>42} permitted: false> params.permit(:id) # => #<ActionController::Parameters {"id"=>42} permitted: true> params.expect(:id) # => 42 ``` ### `params.require` It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter. `ids` is `42`: ```ruby params = ActionController::Parameters.new(ids: 42) # => #<ActionController::Parameters {"ids"=>42} permitted: false> params.require(:ids) # => 42 params.expect(:ids) # => 42 params.expect(ids: []) # => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing) ``` `ids` is `[42, 43]`: ```ruby params = ActionController::Parameters.new(ids: [42, 43]) # => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false> params.require(:ids) # => [42, 43] params.expect(:ids) # => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing) params.expect(ids: []) # => [42, 43] ``` ### `params[]` and `params.fetch` Incompatibilities occur when the value is an array. ```ruby params = ActionController::Parameters.new(ids: [42, 43]) # => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false> params[:ids] # => [42, 43] params.fetch(:ids) # => [42, 43] params.expect(:ids) # => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing) ``` These may be designed and provided separately in the future. Closes #1358.
- Loading branch information
Showing
5 changed files
with
266 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#1358](https://github.com/rubocop/rubocop-rails/issues/1358): Add new `Rails/StrongParametersExpect` cop. ([@koic][]) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 `%<prefer>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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
151 changes: 151 additions & 0 deletions
151
spec/rubocop/cop/rails/strong_parameters_expect_spec.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |