Skip to content

Commit 2d6daa3

Browse files
committed
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.
1 parent 4e4b1e6 commit 2d6daa3

File tree

5 files changed

+266
-0
lines changed

5 files changed

+266
-0
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#1358](https://github.com/rubocop/rubocop-rails/issues/1358): Add new `Rails/StrongParametersExpect` cop. ([@koic][])

config/default.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,6 +1081,15 @@ Rails/StripHeredoc:
10811081
Enabled: pending
10821082
VersionAdded: '2.15'
10831083

1084+
Rails/StrongParametersExpect:
1085+
Description: 'Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling.'
1086+
Reference: 'https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect'
1087+
Enabled: pending
1088+
Include:
1089+
- app/controllers/**/*.rb
1090+
SafeAutoCorrect: false
1091+
VersionAdded: '<<next>>'
1092+
10841093
Rails/TableNameAssignment:
10851094
Description: >-
10861095
Do not use `self.table_name =`. Use Inflections or `table_name_prefix` instead.
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling.
7+
#
8+
# @safety
9+
# This cop's autocorrection is considered unsafe because there are cases where the HTTP status may change
10+
# from 500 to 400 when handling invalid parameters. This change, however, reflects an intentional
11+
# incompatibility introduced for valid reasons by the `expect` method, which aligns better with
12+
# strong parameter conventions.
13+
#
14+
# @example
15+
#
16+
# # bad
17+
# params.require(:user).permit(:name, :age)
18+
# params.permit(user: [:name, :age]).require(:user)
19+
#
20+
# # good
21+
# params.expect(user: [:name, :age])
22+
#
23+
class StrongParametersExpect < Base
24+
extend AutoCorrector
25+
extend TargetRailsVersion
26+
27+
MSG = 'Use `%<prefer>s` instead.'
28+
RESTRICT_ON_SEND = %i[require permit].freeze
29+
30+
minimum_target_rails_version 8.0
31+
32+
def_node_matcher :params_require_permit, <<~PATTERN
33+
$(call
34+
$(call
35+
(send nil? :params) :require _) :permit ...)
36+
PATTERN
37+
38+
def_node_matcher :params_permit_require, <<~PATTERN
39+
$(call
40+
$(call
41+
(send nil? :params) :permit (hash (pair _require_param_name _ )))
42+
:require _require_param_name)
43+
PATTERN
44+
45+
# rubocop:disable Metrics/AbcSize, Metrics/MethodLength
46+
def on_send(node)
47+
return if part_of_ignored_node?(node)
48+
49+
if (permit_method, require_method = params_require_permit(node))
50+
range = offense_range(require_method, node)
51+
prefer = expect_method(require_method, permit_method)
52+
replace_argument = true
53+
elsif (require_method, permit_method = params_permit_require(node))
54+
range = offense_range(permit_method, node)
55+
prefer = "expect(#{permit_method.arguments.map(&:source).join(', ')})"
56+
replace_argument = false
57+
else
58+
return
59+
end
60+
61+
add_offense(range, message: format(MSG, prefer: prefer)) do |corrector|
62+
corrector.remove(require_method.loc.dot.join(require_method.source_range.end))
63+
corrector.replace(permit_method.loc.selector, 'expect')
64+
if replace_argument
65+
corrector.insert_before(permit_method.first_argument, "#{require_key(require_method)}[")
66+
corrector.insert_after(permit_method.last_argument, ']')
67+
end
68+
end
69+
70+
ignore_node(node)
71+
end
72+
# rubocop:enable Metrics/AbcSize, Metrics/MethodLength
73+
alias on_csend on_send
74+
75+
private
76+
77+
def offense_range(method_node, node)
78+
method_node.loc.selector.join(node.source_range.end)
79+
end
80+
81+
def expect_method(require_method, permit_method)
82+
require_key = require_key(require_method)
83+
permit_args = permit_method.arguments.map(&:source).join(', ')
84+
85+
arguments = "#{require_key}[#{permit_args}]"
86+
87+
"expect(#{arguments})"
88+
end
89+
90+
def require_key(require_method)
91+
if (first_argument = require_method.first_argument).respond_to?(:value)
92+
require_arg = first_argument.value
93+
separator = ': '
94+
else
95+
require_arg = first_argument.source
96+
separator = ' => '
97+
end
98+
99+
"#{require_arg}#{separator}"
100+
end
101+
end
102+
end
103+
end
104+
end

lib/rubocop/cop/rails_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
require_relative 'mixin/target_rails_version'
1212

1313
require_relative 'rails/action_controller_flash_before_render'
14+
require_relative 'rails/strong_parameters_expect'
1415
require_relative 'rails/action_controller_test_case'
1516
require_relative 'rails/action_filter'
1617
require_relative 'rails/action_order'
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Rails::StrongParametersExpect, :config do
4+
context 'Rails >= 8.0', :rails80 do
5+
it 'registers an offense when using `params.require(:user).permit(:name, :age)`' do
6+
expect_offense(<<~RUBY)
7+
params.require(:user).permit(:name, :age)
8+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
9+
RUBY
10+
11+
expect_correction(<<~RUBY)
12+
params.expect(user: [:name, :age])
13+
RUBY
14+
end
15+
16+
it 'registers an offense when using `params&.require(:user)&.permit(:name, :age)`' do
17+
expect_offense(<<~RUBY)
18+
params&.require(:user)&.permit(:name, :age)
19+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
20+
RUBY
21+
22+
expect_correction(<<~RUBY)
23+
params&.expect(user: [:name, :age])
24+
RUBY
25+
end
26+
27+
it 'registers an offense when using `params.permit(user: [:name, :age]).require(:user)`' do
28+
expect_offense(<<~RUBY)
29+
params.permit(user: [:name, :age]).require(:user)
30+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
31+
RUBY
32+
33+
expect_correction(<<~RUBY)
34+
params.expect(user: [:name, :age])
35+
RUBY
36+
end
37+
38+
it 'registers an offense when using `params&.permit(user: [:name, :age])&.require(:user)`' do
39+
expect_offense(<<~RUBY)
40+
params&.permit(user: [:name, :age])&.require(:user)
41+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
42+
RUBY
43+
44+
expect_correction(<<~RUBY)
45+
params&.expect(user: [:name, :age])
46+
RUBY
47+
end
48+
49+
it 'registers an offense when using `params.require(:user).permit(:name, some_ids: [])`' do
50+
expect_offense(<<~RUBY)
51+
params.require(:user).permit(:name, some_ids: [])
52+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, some_ids: []])` instead.
53+
RUBY
54+
55+
expect_correction(<<~RUBY)
56+
params.expect(user: [:name, some_ids: []])
57+
RUBY
58+
end
59+
60+
it 'registers an offense when using `params.require(:user).permit(*parameters, some_ids: [])`' do
61+
expect_offense(<<~RUBY)
62+
params.require(:user).permit(*parameters, some_ids: [])
63+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [*parameters, some_ids: []])` instead.
64+
RUBY
65+
66+
expect_correction(<<~RUBY)
67+
params.expect(user: [*parameters, some_ids: []])
68+
RUBY
69+
end
70+
71+
it 'registers an offense when using `params.require(var).permit(:name, some_ids: [])`' do
72+
expect_offense(<<~RUBY)
73+
var = :user
74+
params.require(var).permit(:name, some_ids: [])
75+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(var => [:name, some_ids: []])` instead.
76+
RUBY
77+
78+
expect_correction(<<~RUBY)
79+
var = :user
80+
params.expect(var => [:name, some_ids: []])
81+
RUBY
82+
end
83+
84+
it "registers an offense when using `params.require(:user).permit(:name, :age)` and `permit`'s args has comment" do
85+
expect_offense(<<~RUBY)
86+
params.require(:user).permit(
87+
^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
88+
:name, # comment
89+
:age # comment
90+
)
91+
RUBY
92+
93+
expect_correction(<<~RUBY)
94+
params.expect(
95+
user: [:name, # comment
96+
:age] # comment
97+
)
98+
RUBY
99+
end
100+
101+
it 'does not register an offense when using `params.expect(user: [:name, :age])`' do
102+
expect_no_offenses(<<~RUBY)
103+
params.expect(user: [:name, :age])
104+
RUBY
105+
end
106+
107+
it 'does not register an offense when using `params.permit(unmatch_require_param: [:name, :age]).require(:user)`' do
108+
expect_no_offenses(<<~RUBY)
109+
params.permit(unmatch_require_param: [:name, :age]).require(:user)
110+
RUBY
111+
end
112+
113+
it 'does not register an offense when using `params.require(:name)`' do
114+
expect_no_offenses(<<~RUBY)
115+
params.require(:name)
116+
RUBY
117+
end
118+
119+
it 'does not register an offense when using `params.permit(:name)`' do
120+
expect_no_offenses(<<~RUBY)
121+
params.permit(:name)
122+
RUBY
123+
end
124+
125+
it 'does not register an offense when using `params[:name]`' do
126+
expect_no_offenses(<<~RUBY)
127+
params[:name]
128+
RUBY
129+
end
130+
131+
it 'does not register an offense when using `params.fetch(:name)`' do
132+
expect_no_offenses(<<~RUBY)
133+
params.fetch(:name)
134+
RUBY
135+
end
136+
137+
it 'does not register an offense when using `params[:user][:name]`' do
138+
expect_no_offenses(<<~RUBY)
139+
params[:user][:name]
140+
RUBY
141+
end
142+
end
143+
144+
context 'Rails <= 7.2', :rails72 do
145+
it 'does not register an offense when using `params.require(:user).permit(:name, :age)`' do
146+
expect_no_offenses(<<~RUBY)
147+
params.require(:user).permit(:name, :age)
148+
RUBY
149+
end
150+
end
151+
end

0 commit comments

Comments
 (0)