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

Fix flash before render and add custom cop #3201

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .rubocop.yml
Expand Up @@ -4,6 +4,7 @@ require:
- rubocop-performance
- rubocop-rails
- rubocop-minitest
- ./test/action_controller_flash_before_render.rb

AllCops:
Exclude:
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/api_keys_controller.rb
Expand Up @@ -21,7 +21,7 @@ def create
@api_key = ApiKey.new(build_params)

if @api_key.errors.present?
flash[:error] = @api_key.errors.full_messages.to_sentence
flash.now[:error] = @api_key.errors.full_messages.to_sentence
@api_key = current_user.api_keys.build(api_key_params.merge(rubygem_id: nil))
return render :new
end
Expand All @@ -32,7 +32,7 @@ def create
session[:api_key] = key
redirect_to profile_api_keys_path, flash: { notice: t(".success") }
else
flash[:error] = @api_key.errors.full_messages.to_sentence
flash.now[:error] = @api_key.errors.full_messages.to_sentence
render :new
end
end
Expand All @@ -50,14 +50,14 @@ def update
@api_key.assign_attributes(api_key_params)

if @api_key.errors.present?
flash[:error] = @api_key.errors.full_messages.to_sentence
flash.now[:error] = @api_key.errors.full_messages.to_sentence
return render :edit
end

if @api_key.save
redirect_to profile_api_keys_path, flash: { notice: t(".success") }
else
flash[:error] = @api_key.errors.full_messages.to_sentence
flash.now[:error] = @api_key.errors.full_messages.to_sentence
render :edit
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/multifactor_auths_controller.rb
Expand Up @@ -19,7 +19,7 @@ def create
flash[:error] = current_user.errors[:base].join
redirect_to edit_settings_url
else
flash[:success] = t(".success")
flash.now[:success] = t(".success")
render :recovery
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/owners_controller.rb
Expand Up @@ -77,7 +77,7 @@ def notify_owner_added(ownership)

def index_with_error(msg, status)
@ownerships = @rubygem.ownerships_including_unconfirmed.includes(:user, :authorizer)
flash[:alert] = msg
flash.now[:alert] = msg
render :index, status: status
end

Expand Down
48 changes: 48 additions & 0 deletions test/action_controller_flash_before_render.rb
@@ -0,0 +1,48 @@
# frozen_string_literal: true

# Using `flash` assignment before `render` will persist the message for too long.
# Check https://guides.rubyonrails.org/action_controller_overview.html#flash-now
#
# @safety
# This cop's autocorrection is unsafe because it replaces `flash` by `flash.now`.
# Even though it is usually a mistake, it might be used intentionally.
#
# @example
# # bad
# flash[:alert] = "Warning!"
# render :index
#
# # good
# flash.now[:alert] = "Warning!"
# render :index
#
class RuboCop::Cop::Rails::ActionControllerFlashBeforeRender < RuboCop::Cop::Cop
extend RuboCop::Cop::AutoCorrector

MSG = "Use `flash.now` before `render`."

def_node_search :flash_assignment?, <<~PATTERN
^(send (send nil? :flash) :[]= ...)
PATTERN

def_node_search :render?, <<~PATTERN
(send nil? :render ...)
PATTERN

RESTRICT_ON_SEND = [:flash].freeze

def on_send(node)
expression = flash_assignment?(node)
return unless expression

context = node.parent.parent
return unless context

siblings = context.descendants
return unless siblings.any? { |sibling| render?(sibling) }

add_offense(node) do |corrector|
corrector.replace(node, "flash.now")
end
end
end