summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2017-06-07 07:54:33 +0000
committerRémy Coutable <remy@rymai.me>2017-06-07 07:54:33 +0000
commit5f6926f63249b3bcc6b94d27e038213aa55c9f06 (patch)
treea9b114ec12fd5f2861101481aea8053a81057d4f
parent9c2f82842b9d4d985ab8fadf47bcbac22ec76bd6 (diff)
parenta544e46bb083c27bfa2966ea67ddf97b5dc7cb08 (diff)
downloadgitlab-ce-5f6926f63249b3bcc6b94d27e038213aa55c9f06.tar.gz
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
-rw-r--r--app/controllers/admin/applications_controller.rb2
-rw-r--r--app/controllers/admin/deploy_keys_controller.rb2
-rw-r--r--app/controllers/admin/groups_controller.rb4
-rw-r--r--app/controllers/admin/hooks_controller.rb2
-rw-r--r--app/controllers/admin/identities_controller.rb4
-rw-r--r--app/controllers/admin/impersonations_controller.rb2
-rw-r--r--app/controllers/admin/keys_controller.rb4
-rw-r--r--app/controllers/admin/labels_controller.rb2
-rw-r--r--app/controllers/admin/runner_projects_controller.rb2
-rw-r--r--app/controllers/admin/runners_controller.rb2
-rw-r--r--app/controllers/admin/spam_logs_controller.rb4
-rw-r--r--app/controllers/admin/users_controller.rb2
-rw-r--r--app/controllers/dashboard/todos_controller.rb8
-rw-r--r--app/controllers/groups/avatars_controller.rb2
-rw-r--r--app/controllers/groups/labels_controller.rb2
-rw-r--r--app/controllers/groups_controller.rb4
-rw-r--r--app/controllers/oauth/authorized_applications_controller.rb4
-rw-r--r--app/controllers/profiles/avatars_controller.rb2
-rw-r--r--app/controllers/profiles/chat_names_controller.rb2
-rw-r--r--app/controllers/profiles/emails_controller.rb2
-rw-r--r--app/controllers/profiles/keys_controller.rb2
-rw-r--r--app/controllers/profiles/two_factor_auths_controller.rb2
-rw-r--r--app/controllers/profiles/u2f_registrations_controller.rb2
-rw-r--r--app/controllers/projects/avatars_controller.rb2
-rw-r--r--app/controllers/projects/group_links_controller.rb2
-rw-r--r--app/controllers/projects/hooks_controller.rb2
-rw-r--r--app/controllers/projects/labels_controller.rb4
-rw-r--r--app/controllers/projects/milestones_controller.rb2
-rw-r--r--app/controllers/projects/pages_controller.rb5
-rw-r--r--app/controllers/projects/pages_domains_controller.rb5
-rw-r--r--app/controllers/projects/pipeline_schedules_controller.rb6
-rw-r--r--app/controllers/projects/registry/repositories_controller.rb2
-rw-r--r--app/controllers/projects/registry/tags_controller.rb2
-rw-r--r--app/controllers/projects/runner_projects_controller.rb2
-rw-r--r--app/controllers/projects/runners_controller.rb2
-rw-r--r--app/controllers/projects/snippets_controller.rb2
-rw-r--r--app/controllers/projects/triggers_controller.rb2
-rw-r--r--app/controllers/projects/variables_controller.rb4
-rw-r--r--app/controllers/projects/wikis_controller.rb7
-rw-r--r--app/controllers/projects_controller.rb4
-rw-r--r--app/controllers/registrations_controller.rb2
-rw-r--r--app/controllers/sherlock/transactions_controller.rb2
-rw-r--r--app/controllers/snippets_controller.rb2
-rw-r--r--changelogs/unreleased/31840-add-a-rubocop-that-forbids-redirect_to-inside-a-controller-destroy-action-without-an-explicit-status.yml4
-rw-r--r--rubocop/cop/redirect_with_status.rb44
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--spec/rubocop/cop/redirect_with_status_spec.rb86
47 files changed, 208 insertions, 52 deletions
diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb
index 9c9f420c1e0..434ff6b2a62 100644
--- a/app/controllers/admin/applications_controller.rb
+++ b/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
diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb
index 4f6a7e9e2cb..ab212ed15d0 100644
--- a/app/controllers/admin/deploy_keys_controller.rb
+++ b/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
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 5a2a7c7f27b..2ce26de1768 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/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
diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb
index b9251e140f8..054c3500b35 100644
--- a/app/controllers/admin/hooks_controller.rb
+++ b/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
diff --git a/app/controllers/admin/identities_controller.rb b/app/controllers/admin/identities_controller.rb
index 79a53556f0a..43b4e3a2cc3 100644
--- a/app/controllers/admin/identities_controller.rb
+++ b/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
diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb
index 8e7adc06584..39dbf85f6c0 100644
--- a/app/controllers/admin/impersonations_controller.rb
+++ b/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
diff --git a/app/controllers/admin/keys_controller.rb b/app/controllers/admin/keys_controller.rb
index 299419fb509..0b76193a90e 100644
--- a/app/controllers/admin/keys_controller.rb
+++ b/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
diff --git a/app/controllers/admin/labels_controller.rb b/app/controllers/admin/labels_controller.rb
index 4531657268c..cbc7a14ae83 100644
--- a/app/controllers/admin/labels_controller.rb
+++ b/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
diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb
index 70ac6a75434..7ed2de71028 100644
--- a/app/controllers/admin/runner_projects_controller.rb
+++ b/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
diff --git a/app/controllers/admin/runners_controller.rb b/app/controllers/admin/runners_controller.rb
index 348641e5ecb..719893c0bc8 100644
--- a/app/controllers/admin/runners_controller.rb
+++ b/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
diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb
index 1d66955bb71..d52d67a67a5 100644
--- a/app/controllers/admin/spam_logs_controller.rb
+++ b/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
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index bace99dad58..b09eef17c23 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/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
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 4d7d45787fc..623392c1240 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/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
diff --git a/app/controllers/groups/avatars_controller.rb b/app/controllers/groups/avatars_controller.rb
index ad2c20b42db..735915abdaa 100644
--- a/app/controllers/groups/avatars_controller.rb
+++ b/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
diff --git a/app/controllers/groups/labels_controller.rb b/app/controllers/groups/labels_controller.rb
index 3fa0516fb0c..dda59262483 100644
--- a/app/controllers/groups/labels_controller.rb
+++ b/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
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 18a2d69db29..c08943d993a 100644
--- a/app/controllers/groups_controller.rb
+++ b/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
@@ -173,7 +173,7 @@ class GroupsController < Groups::ApplicationController
def build_canonical_path(group)
return group_path(group) if action_name == 'show' # root group path
-
+
params[:id] = group.to_param
url_for(params)
diff --git a/app/controllers/oauth/authorized_applications_controller.rb b/app/controllers/oauth/authorized_applications_controller.rb
index 4193ac11399..656107d2b26 100644
--- a/app/controllers/oauth/authorized_applications_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb
index daa51ae41df..933e0f3bceb 100644
--- a/app/controllers/profiles/avatars_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/chat_names_controller.rb b/app/controllers/profiles/chat_names_controller.rb
index 6a1f468ba5a..2353f0840d6 100644
--- a/app/controllers/profiles/chat_names_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 1c24c4db993..5655fb2ba0e 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb
index e4452f46056..88f49da555a 100644
--- a/app/controllers/profiles/keys_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index d3fa81cd623..313cdcd1c15 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/u2f_registrations_controller.rb b/app/controllers/profiles/u2f_registrations_controller.rb
index c02fe85c3cc..e3d7737f44a 100644
--- a/app/controllers/profiles/u2f_registrations_controller.rb
+++ b/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
diff --git a/app/controllers/projects/avatars_controller.rb b/app/controllers/projects/avatars_controller.rb
index 53788687076..21a403f3765 100644
--- a/app/controllers/projects/avatars_controller.rb
+++ b/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
diff --git a/app/controllers/projects/group_links_controller.rb b/app/controllers/projects/group_links_controller.rb
index 66b7bdbd988..deb33a2f0ff 100644
--- a/app/controllers/projects/group_links_controller.rb
+++ b/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
diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index 38bd82841dc..f5143280154 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/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
diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb
index 71bfb7163da..ac151839f61 100644
--- a/app/controllers/projects/labels_controller.rb
+++ b/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
diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index c56bce19eee..ae16f69955a 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/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
diff --git a/app/controllers/projects/pages_controller.rb b/app/controllers/projects/pages_controller.rb
index 93b2c180810..28b383e69eb 100644
--- a/app/controllers/projects/pages_controller.rb
+++ b/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
diff --git a/app/controllers/projects/pages_domains_controller.rb b/app/controllers/projects/pages_domains_controller.rb
index 3a93977fd27..dbd011f6c5d 100644
--- a/app/controllers/projects/pages_domains_controller.rb
+++ b/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
diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index 1616b2cb6b8..2662a146968 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/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
diff --git a/app/controllers/projects/registry/repositories_controller.rb b/app/controllers/projects/registry/repositories_controller.rb
index 17f391ba07f..98e78585be8 100644
--- a/app/controllers/projects/registry/repositories_controller.rb
+++ b/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
diff --git a/app/controllers/projects/registry/tags_controller.rb b/app/controllers/projects/registry/tags_controller.rb
index d689cade3ab..5050dba3aab 100644
--- a/app/controllers/projects/registry/tags_controller.rb
+++ b/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
diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb
index 8267b14941d..3cb01405b05 100644
--- a/app/controllers/projects/runner_projects_controller.rb
+++ b/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
diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb
index 8b50ea207a5..160e632648a 100644
--- a/app/controllers/projects/runners_controller.rb
+++ b/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
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index 3a97c1e98af..84deefb7875 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/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
diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb
index afa56de920b..e86adddd77f 100644
--- a/app/controllers/projects/triggers_controller.rb
+++ b/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
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 0953eecaeb5..50e25a00f03 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/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
diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb
index 887d18dbec3..e54b90b8d52 100644
--- a/app/controllers/projects/wikis_controller.rb
+++ b/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),
- notice: "Page was successfully deleted"
- )
+ redirect_to namespace_project_wiki_path(@project.namespace, @project, :home),
+ status: 302,
+ notice: "Page was successfully deleted"
end
def git_access
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index cc62e1fa99b..aada031f117 100644
--- a/app/controllers/projects_controller.rb
+++ b/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
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index cd2003586be..1bc6520370a 100644
--- a/app/controllers/registrations_controller.rb
+++ b/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
diff --git a/app/controllers/sherlock/transactions_controller.rb b/app/controllers/sherlock/transactions_controller.rb
index ccc739da879..cb6c3a7cd98 100644
--- a/app/controllers/sherlock/transactions_controller.rb
+++ b/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
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 5b2d143ee79..8511457b53d 100644
--- a/app/controllers/snippets_controller.rb
+++ b/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
diff --git a/changelogs/unreleased/31840-add-a-rubocop-that-forbids-redirect_to-inside-a-controller-destroy-action-without-an-explicit-status.yml b/changelogs/unreleased/31840-add-a-rubocop-that-forbids-redirect_to-inside-a-controller-destroy-action-without-an-explicit-status.yml
new file mode 100644
index 00000000000..52bfe771e2b
--- /dev/null
+++ b/changelogs/unreleased/31840-add-a-rubocop-that-forbids-redirect_to-inside-a-controller-destroy-action-without-an-explicit-status.yml
@@ -0,0 +1,4 @@
+---
+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
diff --git a/rubocop/cop/redirect_with_status.rb b/rubocop/cop/redirect_with_status.rb
new file mode 100644
index 00000000000..36810642c88
--- /dev/null
+++ b/rubocop/cop/redirect_with_status.rb
@@ -0,0 +1,44 @@
+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
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 17d2bf6aa1c..22815090508 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,6 +1,7 @@
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'
diff --git a/spec/rubocop/cop/redirect_with_status_spec.rb b/spec/rubocop/cop/redirect_with_status_spec.rb
new file mode 100644
index 00000000000..5ad63567f84
--- /dev/null
+++ b/spec/rubocop/cop/redirect_with_status_spec.rb
@@ -0,0 +1,86 @@
+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