diff --git a/changelogs/unreleased/fix-leading-slash-in-redirects-plus-rubocop.yml b/changelogs/unreleased/fix-leading-slash-in-redirects-plus-rubocop.yml new file mode 100644 index 00000000000..38b2486a475 --- /dev/null +++ b/changelogs/unreleased/fix-leading-slash-in-redirects-plus-rubocop.yml @@ -0,0 +1,5 @@ +--- +title: Fix leading slash in redirects and add rubocop cop +merge_request: 21828 +author: Sanad Liaquat +type: fixed diff --git a/config/routes/instance_statistics.rb b/config/routes/instance_statistics.rb index 824ef47cda3..1102ef6b017 100644 --- a/config/routes/instance_statistics.rb +++ b/config/routes/instance_statistics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true namespace :instance_statistics do - root to: redirect('/-/instance_statistics/conversational_development_index') + root to: redirect('-/instance_statistics/conversational_development_index') resources :cohorts, only: :index resources :conversational_development_index, only: :index diff --git a/config/routes/wiki.rb b/config/routes/wiki.rb index cd3828b743c..1a07b1c206b 100644 --- a/config/routes/wiki.rb +++ b/config/routes/wiki.rb @@ -2,7 +2,7 @@ scope(controller: :wikis) do scope(path: 'wikis', as: :wikis) do get :git_access get :pages - get '/', to: redirect('/%{namespace_id}/%{project_id}/wikis/home') + get '/', to: redirect('%{namespace_id}/%{project_id}/wikis/home') post '/', to: 'wikis#create' end diff --git a/rubocop/cop/avoid_route_redirect_leading_slash.rb b/rubocop/cop/avoid_route_redirect_leading_slash.rb new file mode 100644 index 00000000000..7ac1c881269 --- /dev/null +++ b/rubocop/cop/avoid_route_redirect_leading_slash.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Checks for a leading '/' in route redirects + # For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/50645 + # + # @example + # # bad + # root to: redirect('/-/instance/statistics/conversational_development_index') + # + # # good + # root to: redirect('-/instance/statistics/conversational_development_index') + # + + class AvoidRouteRedirectLeadingSlash < RuboCop::Cop::Cop + MSG = 'Do not use a leading "/" in route redirects' + + def_node_matcher :leading_slash_in_redirect?, <<~PATTERN + (send nil? :redirect (str #has_leading_slash?)) + PATTERN + + def on_send(node) + return unless in_routes?(node) + return unless leading_slash_in_redirect?(node) + + add_offense(node) + end + + def has_leading_slash?(str) + str.start_with?("/") + end + + def in_routes?(node) + path = node.location.expression.source_buffer.name + dirname = File.dirname(path) + filename = File.basename(path) + dirname.end_with?('config/routes') || filename.end_with?('routes.rb') + end + + def autocorrect(node) + lambda do |corrector| + corrector.replace(node.loc.expression, remove_leading_slash(node)) + end + end + + def remove_leading_slash(node) + node.source.sub('/', '') + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 9d6cc73fc3b..ff929c7b6ce 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -7,6 +7,7 @@ require_relative 'cop/gitlab/union' require_relative 'cop/include_sidekiq_worker' require_relative 'cop/avoid_return_from_blocks' require_relative 'cop/avoid_break_from_strong_memoize' +require_relative 'cop/avoid_route_redirect_leading_slash' require_relative 'cop/line_break_around_conditional_block' require_relative 'cop/prefer_class_methods_over_module' require_relative 'cop/migration/add_column' diff --git a/spec/rubocop/cop/avoid_route_redirect_leading_slash_spec.rb b/spec/rubocop/cop/avoid_route_redirect_leading_slash_spec.rb new file mode 100644 index 00000000000..c9eb61ccc72 --- /dev/null +++ b/spec/rubocop/cop/avoid_route_redirect_leading_slash_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require_relative '../../../rubocop/cop/avoid_route_redirect_leading_slash' + +describe RuboCop::Cop::AvoidRouteRedirectLeadingSlash do + include CopHelper + + subject(:cop) { described_class.new } + + before do + allow(cop).to receive(:in_routes?).and_return(true) + end + + it 'registers an offense when redirect has a leading slash' do + expect_offense(<<~PATTERN.strip_indent) + root to: redirect("/-/route") + ^^^^^^^^^^^^^^^^^^^^ Do not use a leading "/" in route redirects + PATTERN + end + + it 'does not register an offense when redirect does not have a leading slash' do + expect_no_offenses(<<~PATTERN.strip_indent) + root to: redirect("-/route") + PATTERN + end + + it 'autocorrect `/-/route` to `-/route`' do + expect(autocorrect_source('redirect("/-/route")')).to eq('redirect("-/route")') + end +end