Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails/StrongParametersExpect can break working code - this should be documented #1418

Open
JasonBarnabe opened this issue Jan 18, 2025 · 0 comments
Labels
documentation Improvements or additions to documentation

Comments

@JasonBarnabe
Copy link

JasonBarnabe commented Jan 18, 2025

The current documentation for Rails/StrongParametersExpect says that it's unsafe because the response code on invalid parameters changes from 500 to 400.

# @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.

But it's also unsafe because ActionController::Parameters#expect is pickier about the format and may change some successful requests into failures.

https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect

This should be documented in the cop.

Example

The cop turned this code:

   def discussion_params
     attrs = [:rating, :title, :discussion_category_id, :report_id, { comments_attributes: [:text, :text_markup, { attachments: [] }] }]
     attrs += [:report_id] if current_user&.moderator?
     params
       .require(:discussion)
       .permit(attrs)
   end

into this (after correction for #1417):

   def discussion_params
     attrs = [:rating, :title, :discussion_category_id, :report_id, { comments_attributes: [:text, :text_markup, { attachments: [] }] }]
     attrs += [:report_id] if current_user&.moderator?
     params.expect(discussion: [attrs])
   end

which now fails on these request parameters:

{"authenticity_token" => "[FILTERED]", "discussion" => {"comments_attributes" => {"0" => {"text_markup" => "html", "text" => "this is my comment", "attachments" => [""]}}, "rating" => "4"}, "subscribe" => "1", "locale" => "en", "script_id" => "1-a-test"

The problem seems to be the lack of double array brackets on the value for comments_attributes. Corrected code:

   def discussion_params
     attrs = [:rating, :title, :discussion_category_id, :report_id, { comments_attributes: [[:text, :text_markup, { attachments: [] }]] }]
     attrs += [:report_id] if current_user&.moderator?
     params.expect(discussion: [attrs])
   end

(The extra array also works with #require/#permit.)

Rubocop version

1.70.0 (using Parser 3.3.7.0, rubocop-ast 1.37.0, analyzing as Ruby 3.4, running on ruby 3.4.1) +server [x86_64-linux]
  - rubocop-capybara 2.21.0
  - rubocop-minitest 0.36.0
  - rubocop-performance 1.23.1
  - rubocop-rails 2.29.0
@koic koic added the documentation Improvements or additions to documentation label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants