Merge branch...

Merge branch '31840-add-a-rubocop-that-forbids-redirect_to-inside-a-controller-destroy-action-without-an-explicit-status' into 'master'

Resolve "Add a rubocop that forbids `redirect_to` inside a controller `destroy` action without an explicit `status`"

Closes #31840

See merge request !11749

app/controllers/admin/applications_controller.rb

@@ -39,7 +39,7 @@ class Admin::ApplicationsController < Admin::ApplicationController
 
   def destroy
     @application.destroy
-    redirect_to admin_applications_url, notice: 'Application was successfully destroyed.'
+    redirect_to admin_applications_url, status: 302, notice: 'Application was successfully destroyed.'
   end
 
   private

app/controllers/admin/deploy_keys_controller.rb

@@ -23,7 +23,7 @@ class Admin::DeployKeysController < Admin::ApplicationController
     deploy_key.destroy
 
     respond_to do |format|
-      format.html { redirect_to admin_deploy_keys_path }
+      format.html { redirect_to admin_deploy_keys_path, status: 302 }
       format.json { head :ok }
     end
   end

app/controllers/admin/groups_controller.rb

@@ -56,7 +56,9 @@ class Admin::GroupsController < Admin::ApplicationController
   def destroy
     Groups::DestroyService.new(@group, current_user).async_execute
 
-    redirect_to admin_groups_path, alert: "Group '#{@group.name}' was scheduled for deletion."
+    redirect_to admin_groups_path,
+                status: 302,
+                alert: "Group '#{@group.name}' was scheduled for deletion."
   end
 
   private

app/controllers/admin/hooks_controller.rb

@@ -34,7 +34,7 @@ class Admin::HooksController < Admin::ApplicationController
   def destroy
     hook.destroy
 
-    redirect_to admin_hooks_path
+    redirect_to admin_hooks_path, status: 302
   end
 
   def test

app/controllers/admin/identities_controller.rb

@@ -36,9 +36,9 @@ class Admin::IdentitiesController < Admin::ApplicationController
   def destroy
     if @identity.destroy
       RepairLdapBlockedUserService.new(@user).execute
-      redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.'
+      redirect_to admin_user_identities_path(@user), status: 302, notice: 'User identity was successfully removed.'
     else
-      redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.'
+      redirect_to admin_user_identities_path(@user), status: 302, alert: 'Failed to remove user identity.'
     end
   end
 

app/controllers/admin/impersonations_controller.rb

@@ -11,7 +11,7 @@ class Admin::ImpersonationsController < Admin::ApplicationController
 
     session[:impersonator_id] = nil
 
-    redirect_to admin_user_path(original_user)
+    redirect_to admin_user_path(original_user), status: 302
   end
 
   private

app/controllers/admin/keys_controller.rb

@@ -15,9 +15,9 @@ class Admin::KeysController < Admin::ApplicationController
 
     respond_to do |format|
       if key.destroy
-        format.html { redirect_to keys_admin_user_path(user), notice: 'User key was successfully removed.' }
+        format.html { redirect_to keys_admin_user_path(user), status: 302, notice: 'User key was successfully removed.' }
       else
-        format.html { redirect_to keys_admin_user_path(user), alert: 'Failed to remove user key.' }
+        format.html { redirect_to keys_admin_user_path(user), status: 302, alert: 'Failed to remove user key.' }
       end
     end
   end

app/controllers/admin/labels_controller.rb

@@ -41,7 +41,7 @@ class Admin::LabelsController < Admin::ApplicationController
 
     respond_to do |format|
       format.html do
-        redirect_to(admin_labels_path, notice: 'Label was removed')
+        redirect_to admin_labels_path, status: 302, notice: 'Label was removed'
       end
       format.js
     end

app/controllers/admin/runner_projects_controller.rb

@@ -18,7 +18,7 @@ class Admin::RunnerProjectsController < Admin::ApplicationController
     runner = rp.runner
     rp.destroy
 
-    redirect_to admin_runner_path(runner)
+    redirect_to admin_runner_path(runner), status: 302
   end
 
   private

app/controllers/admin/runners_controller.rb

@@ -27,7 +27,7 @@ class Admin::RunnersController < Admin::ApplicationController
   def destroy
     @runner.destroy
 
-    redirect_to admin_runners_path
+    redirect_to admin_runners_path, status: 302
   end
 
   def resume

app/controllers/admin/spam_logs_controller.rb

@@ -8,7 +8,9 @@ class Admin::SpamLogsController < Admin::ApplicationController
 
     if params[:remove_user]
       spam_log.remove_user(deleted_by: current_user)
-      redirect_to admin_spam_logs_path, notice: "User #{spam_log.user.username} was successfully removed."
+      redirect_to admin_spam_logs_path,
+                  status: 302,
+                  notice: "User #{spam_log.user.username} was successfully removed."
     else
       spam_log.destroy
       head :ok

app/controllers/admin/users_controller.rb

@@ -141,7 +141,7 @@ class Admin::UsersController < Admin::ApplicationController
     user.delete_async(deleted_by: current_user, params: params.permit(:hard_delete))
 
     respond_to do |format|
-      format.html { redirect_to admin_users_path, notice: "The user is being deleted." }
+      format.html { redirect_to admin_users_path, status: 302, notice: "The user is being deleted." }
       format.json { head :ok }
     end
   end

app/controllers/dashboard/todos_controller.rb

@@ -15,7 +15,11 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     TodoService.new.mark_todos_as_done_by_ids([params[:id]], current_user)
 
     respond_to do |format|
-      format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' }
+      format.html do
+        redirect_to dashboard_todos_path,
+                    status: 302,
+                    notice: 'Todo was successfully marked as done.'
+      end
       format.js { head :ok }
       format.json { render json: todos_counts }
     end
@@ -25,7 +29,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     updated_ids = TodoService.new.mark_todos_as_done(@todos, current_user)
 
     respond_to do |format|
-      format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' }
+      format.html { redirect_to dashboard_todos_path, status: 302, notice: 'All todos were marked as done.' }
       format.js { head :ok }
       format.json { render json: todos_counts.merge(updated_ids: updated_ids) }
     end

app/controllers/groups/avatars_controller.rb

@@ -5,6 +5,6 @@ class Groups::AvatarsController < Groups::ApplicationController
     @group.remove_avatar!
     @group.save
 
-    redirect_to edit_group_path(@group)
+    redirect_to edit_group_path(@group), status: 302
   end
 end

app/controllers/groups/labels_controller.rb

@@ -54,7 +54,7 @@ class Groups::LabelsController < Groups::ApplicationController
 
     respond_to do |format|
       format.html do
-        redirect_to group_labels_path(@group), notice: 'Label was removed'
+        redirect_to group_labels_path(@group), status: 302, notice: 'Label was removed'
       end
       format.js
     end

app/controllers/groups_controller.rb

@@ -101,7 +101,7 @@ class GroupsController < Groups::ApplicationController
   def destroy
     Groups::DestroyService.new(@group, current_user).async_execute
 
-    redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion."
+    redirect_to root_path, status: 302, alert: "Group '#{@group.name}' was scheduled for deletion."
   end
 
   protected

app/controllers/oauth/authorized_applications_controller.rb

@@ -10,6 +10,8 @@ class Oauth::AuthorizedApplicationsController < Doorkeeper::AuthorizedApplicatio
       Doorkeeper::AccessToken.revoke_all_for(params[:id], current_resource_owner)
     end
 
-    redirect_to applications_profile_url, notice: I18n.t(:notice, scope: [:doorkeeper, :flash, :authorized_applications, :destroy])
+    redirect_to applications_profile_url,
+                status: 302,
+                notice: I18n.t(:notice, scope: [:doorkeeper, :flash, :authorized_applications, :destroy])
   end
 end

app/controllers/profiles/avatars_controller.rb

@@ -5,6 +5,6 @@ class Profiles::AvatarsController < Profiles::ApplicationController
 
     @user.save
 
-    redirect_to profile_path
+    redirect_to profile_path, status: 302
   end
 end

app/controllers/profiles/chat_names_controller.rb

@@ -39,7 +39,7 @@ class Profiles::ChatNamesController < Profiles::ApplicationController
       flash[:alert] = "Could not delete chat nickname #{@chat_name.chat_name}."
     end
 
-    redirect_to profile_chat_names_path
+    redirect_to profile_chat_names_path, status: 302
   end
 
   private

app/controllers/profiles/emails_controller.rb

@@ -23,7 +23,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
     current_user.update_secondary_emails!
 
     respond_to do |format|
-      format.html { redirect_to profile_emails_url }
+      format.html { redirect_to profile_emails_url, status: 302 }
       format.js { head :ok }
     end
   end

app/controllers/profiles/keys_controller.rb

@@ -26,7 +26,7 @@ class Profiles::KeysController < Profiles::ApplicationController
     @key.destroy
 
     respond_to do |format|
-      format.html { redirect_to profile_keys_url }
+      format.html { redirect_to profile_keys_url, status: 302 }
       format.js { head :ok }
     end
   end

app/controllers/profiles/two_factor_auths_controller.rb

@@ -77,7 +77,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   def destroy
     current_user.disable_two_factor!
 
-    redirect_to profile_account_path
+    redirect_to profile_account_path, status: 302
   end
 
   def skip

app/controllers/profiles/u2f_registrations_controller.rb

@@ -2,6 +2,6 @@ class Profiles::U2fRegistrationsController < Profiles::ApplicationController
   def destroy
     u2f_registration = current_user.u2f_registrations.find(params[:id])
     u2f_registration.destroy
-    redirect_to profile_two_factor_auth_path, notice: "Successfully deleted U2F device."
+    redirect_to profile_two_factor_auth_path, status: 302, notice: "Successfully deleted U2F device."
   end
 end

app/controllers/projects/avatars_controller.rb

@@ -21,6 +21,6 @@ class Projects::AvatarsController < Projects::ApplicationController
 
     @project.save
 
-    redirect_to edit_project_path(@project)
+    redirect_to edit_project_path(@project), status: 302
   end
 end

app/controllers/projects/group_links_controller.rb

@@ -36,7 +36,7 @@ class Projects::GroupLinksController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
-        redirect_to namespace_project_settings_members_path(project.namespace, project)
+        redirect_to namespace_project_settings_members_path(project.namespace, project), status: 302
       end
       format.js { head :ok }
     end

app/controllers/projects/hooks_controller.rb

@@ -47,7 +47,7 @@ class Projects::HooksController < Projects::ApplicationController
   def destroy
     hook.destroy
 
-    redirect_to namespace_project_settings_integrations_path(@project.namespace, @project)
+    redirect_to namespace_project_settings_integrations_path(@project.namespace, @project), status: 302
   end
 
   private

app/controllers/projects/labels_controller.rb

@@ -74,7 +74,9 @@ class Projects::LabelsController < Projects::ApplicationController
     @label.destroy
     @labels = find_labels
 
-    redirect_to(namespace_project_labels_path(@project.namespace, @project), notice: 'Label was removed')
+    redirect_to namespace_project_labels_path(@project.namespace, @project),
+                status: 302,
+                notice: 'Label was removed'
   end
 
   def remove_priority

app/controllers/projects/milestones_controller.rb

@@ -80,7 +80,7 @@ class Projects::MilestonesController < Projects::ApplicationController
     Milestones::DestroyService.new(project, current_user).execute(milestone)
 
     respond_to do |format|
-      format.html { redirect_to namespace_project_milestones_path }
+      format.html { redirect_to namespace_project_milestones_path, status: 302 }
       format.js { head :ok }
     end
   end

app/controllers/projects/pages_controller.rb

@@ -15,8 +15,9 @@ class Projects::PagesController < Projects::ApplicationController
 
     respond_to do |format|
       format.html  do
-        redirect_to(namespace_project_pages_path(@project.namespace, @project),
-                    notice: 'Pages were removed')
+        redirect_to namespace_project_pages_path(@project.namespace, @project),
+                    status: 302,
+                    notice: 'Pages were removed'
       end
     end
   end

app/controllers/projects/pages_domains_controller.rb

@@ -27,8 +27,9 @@ class Projects::PagesDomainsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
-        redirect_to(namespace_project_pages_path(@project.namespace, @project),
-                    notice: 'Domain was removed')
+        redirect_to namespace_project_pages_path(@project.namespace, @project),
+                    status: 302,
+                    notice: 'Domain was removed'
       end
       format.js
     end

app/controllers/projects/pipeline_schedules_controller.rb

@@ -49,9 +49,11 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
 
   def destroy
     if schedule.destroy
-      redirect_to pipeline_schedules_path(@project)
+      redirect_to pipeline_schedules_path(@project), status: 302
     else
-      redirect_to pipeline_schedules_path(@project), alert: "Failed to remove the pipeline schedule"
+      redirect_to pipeline_schedules_path(@project),
+                  status: 302,
+                  alert: "Failed to remove the pipeline schedule"
     end
   end
 

app/controllers/projects/registry/repositories_controller.rb

@@ -11,9 +11,11 @@ module Projects
       def destroy
         if image.destroy
           redirect_to project_container_registry_path(@project),
+                      status: 302,
                       notice: 'Image repository has been removed successfully!'
         else
           redirect_to project_container_registry_path(@project),
+                      status: 302,
                       alert: 'Failed to remove image repository!'
         end
       end

app/controllers/projects/registry/tags_controller.rb

@@ -6,9 +6,11 @@ module Projects
       def destroy
         if tag.delete
           redirect_to project_container_registry_path(@project),
+                      status: 302,
                       notice: 'Registry tag has been removed successfully!'
         else
           redirect_to project_container_registry_path(@project),
+                      status: 302,
                       alert: 'Failed to remove registry tag!'
         end
       end

app/controllers/projects/runner_projects_controller.rb

@@ -22,6 +22,6 @@ class Projects::RunnerProjectsController < Projects::ApplicationController
     runner_project = project.runner_projects.find(params[:id])
     runner_project.destroy
 
-    redirect_to runners_path(project)
+    redirect_to runners_path(project), status: 302
   end
 end

app/controllers/projects/runners_controller.rb

@@ -24,7 +24,7 @@ class Projects::RunnersController < Projects::ApplicationController
       @runner.destroy
     end
 
-    redirect_to runners_path(@project)
+    redirect_to runners_path(@project), status: 302
   end
 
   def resume

app/controllers/projects/snippets_controller.rb

@@ -79,7 +79,7 @@ class Projects::SnippetsController < Projects::ApplicationController
 
     @snippet.destroy
 
-    redirect_to namespace_project_snippets_path(@project.namespace, @project)
+    redirect_to namespace_project_snippets_path(@project.namespace, @project), status: 302
   end
 
   protected

app/controllers/projects/triggers_controller.rb

@@ -50,7 +50,7 @@ class Projects::TriggersController < Projects::ApplicationController
       flash[:alert] = "Could not remove the trigger."
     end
 
-    redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project)
+    redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project), status: 302
   end
 
   private

app/controllers/projects/variables_controller.rb

@@ -36,7 +36,9 @@ class Projects::VariablesController < Projects::ApplicationController
     @key = @project.variables.find(params[:id])
     @key.destroy
 
-    redirect_to namespace_project_settings_ci_cd_path(project.namespace, project), notice: 'Variable was successfully removed.'
+    redirect_to namespace_project_settings_ci_cd_path(project.namespace, project),
+                status: 302,
+                notice: 'Variable was successfully removed.'
   end
 
   private

app/controllers/projects/wikis_controller.rb

@@ -85,10 +85,9 @@ class Projects::WikisController < Projects::ApplicationController
     @page = @project_wiki.find_page(params[:id])
     WikiPages::DestroyService.new(@project, current_user).execute(@page)
 
-    redirect_to(
-      namespace_project_wiki_path(@project.namespace, @project, :home),
+    redirect_to namespace_project_wiki_path(@project.namespace, @project, :home),
+                status: 302,
                 notice: "Page was successfully deleted"
-    )
   end
 
   def git_access

app/controllers/projects_controller.rb

@@ -119,9 +119,9 @@ class ProjectsController < Projects::ApplicationController
     ::Projects::DestroyService.new(@project, current_user, {}).async_execute
     flash[:alert] = "Project '#{@project.name_with_namespace}' will be deleted."
 
-    redirect_to dashboard_projects_path
+    redirect_to dashboard_projects_path, status: 302
   rescue Projects::DestroyService::DestroyError => ex
-    redirect_to edit_project_path(@project), alert: ex.message
+    redirect_to edit_project_path(@project), status: 302, alert: ex.message
   end
 
   def new_issue_address

app/controllers/registrations_controller.rb

@@ -30,7 +30,7 @@ class RegistrationsController < Devise::RegistrationsController
     respond_to do |format|
       format.html do
         session.try(:destroy)
-        redirect_to new_user_session_path, notice: "Account scheduled for removal."
+        redirect_to new_user_session_path, status: 302, notice: "Account scheduled for removal."
       end
     end
   end

app/controllers/sherlock/transactions_controller.rb

@@ -13,7 +13,7 @@ module Sherlock
     def destroy_all
       Gitlab::Sherlock.collection.clear
 
-      redirect_to(:back)
+      redirect_to :back, status: 302
     end
   end
 end

app/controllers/snippets_controller.rb

@@ -82,7 +82,7 @@ class SnippetsController < ApplicationController
 
     @snippet.destroy
 
-    redirect_to snippets_path
+    redirect_to snippets_path, status: 302
   end
 
   def preview_markdown

changelogs/unreleased/31840-add-a-rubocop-that-forbids-redirect_to-inside-a-controller-destroy-action-without-an-explicit-status.yml

+---
+title: Add a rubocop rule to check if a method 'redirect_to' is used without explicitly set 'status' in 'destroy' actions of controllers
+merge_request: 11749
+author: @blackst0ne

rubocop/cop/redirect_with_status.rb

+module RuboCop
+  module Cop
+    # This cop prevents usage of 'redirect_to' in actions 'destroy' without specifying 'status'.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/issues/31840
+    class RedirectWithStatus < RuboCop::Cop::Cop
+      MSG = 'Do not use "redirect_to" without "status" in "destroy" action'.freeze
+
+      def on_def(node)
+        return unless in_controller?(node)
+        return unless destroy?(node) || destroy_all?(node)
+
+        node.each_descendant(:send) do |def_node|
+          next unless redirect_to?(def_node)
+
+          methods = []
+
+          def_node.children.last.each_node(:pair) do |pair|
+            methods << pair.children.first.children.first
+          end
+
+          add_offense(def_node, :selector) unless methods.include?(:status)
+        end
+      end
+
+      private
+
+      def in_controller?(node)
+        node.location.expression.source_buffer.name.end_with?('_controller.rb')
+      end
+
+      def destroy?(node)
+        node.children.first == :destroy
+      end
+
+      def destroy_all?(node)
+        node.children.first == :destroy_all
+      end
+
+      def redirect_to?(node)
+        node.children[1] == :redirect_to
+      end
+    end
+  end
+end

rubocop/rubocop.rb

 require_relative 'cop/custom_error_class'
 require_relative 'cop/gem_fetcher'
 require_relative 'cop/activerecord_serialize'
+require_relative 'cop/redirect_with_status'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_column_with_default_to_large_table'
 require_relative 'cop/migration/add_concurrent_foreign_key'

spec/rubocop/cop/redirect_with_status_spec.rb

+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../rubocop/cop/redirect_with_status'
+
+describe RuboCop::Cop::RedirectWithStatus do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+  let(:controller_fixture_without_status) do
+    %q(
+      class UserController < ApplicationController
+        def show
+          user = User.find(params[:id])
+          redirect_to user_path if user.name == 'John Wick'
+        end
+
+        def destroy
+          user = User.find(params[:id])
+
+          if user.destroy
+            redirect_to root_path
+          else
+            render :show
+          end
+        end
+      end
+    )
+  end
+
+  let(:controller_fixture_with_status) do
+    %q(
+      class UserController < ApplicationController
+        def show
+          user = User.find(params[:id])
+          redirect_to user_path if user.name == 'John Wick'
+        end
+
+        def destroy
+          user = User.find(params[:id])
+
+          if user.destroy
+            redirect_to root_path, status: 302
+          else
+            render :show
+          end
+        end
+      end
+    )
+  end
+
+  context 'in controller' do
+    before do
+      allow(cop).to receive(:in_controller?).and_return(true)
+    end
+
+    it 'registers an offense when a "destroy" action uses "redirect_to" without "status"' do
+      inspect_source(cop, controller_fixture_without_status)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([12]) # 'redirect_to' is located on 12th line in controller_fixture.
+        expect(cop.highlights).to eq(['redirect_to'])
+      end
+    end
+
+    it 'does not register an offense when a "destroy" action uses "redirect_to" with "status"' do
+      inspect_source(cop, controller_fixture_with_status)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+  end
+
+  context 'outside of controller' do
+    it 'registers no offense' do
+      inspect_source(cop, controller_fixture_without_status)
+      inspect_source(cop, controller_fixture_with_status)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end