Commit 718255ad authored by Peter Leitzen's avatar Peter Leitzen Committed by Dmytro Zaporozhets

Check HTTP methods in Cop/Put{Project,Group}RoutesUnderScope

Previously, Cop/PutProjectRoutesUnderScope and
Cop/PutGroupRoutesUnderScope were only checking `resource` and
`resources` and skipped checking trivial routes like `get`, `post` etc.

This commit unifies both cops and adds the ability to check HTTP
methods.
parent fe433bd7
......@@ -481,3 +481,13 @@ Rails/SaveBang:
- 'ee/spec/**/*.rb'
- 'qa/spec/**/*.rb'
- 'qa/qa/specs/**/*.rb'
Cop/PutProjectRoutesUnderScope:
Include:
- 'config/routes/project.rb'
- 'ee/config/routes/project.rb'
Cop/PutGroupRoutesUnderScope:
Include:
- 'config/routes/group.rb'
- 'ee/config/routes/group.rb'
......@@ -5,23 +5,27 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
controller: :groups,
constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom|ics)/ }) do
scope(path: '-') do
get :edit, as: :edit_group
get :issues, as: :issues_group_calendar, action: :issues_calendar, constraints: lambda { |req| req.format == :ics }
get :issues, as: :issues_group
get :merge_requests, as: :merge_requests_group
get :projects, as: :projects_group
get :details, as: :details_group
get :activity, as: :activity_group
put :transfer, as: :transfer_group
post :export, as: :export_group
get :download_export, as: :download_export_group
# These routes are legit and the cop rule will be improved in
# https://gitlab.com/gitlab-org/gitlab/-/issues/230703
get :edit, as: :edit_group # rubocop:disable Cop/PutGroupRoutesUnderScope
get :issues, as: :issues_group_calendar, action: :issues_calendar, constraints: lambda { |req| req.format == :ics } # rubocop:disable Cop/PutGroupRoutesUnderScope
get :issues, as: :issues_group # rubocop:disable Cop/PutGroupRoutesUnderScope
get :merge_requests, as: :merge_requests_group # rubocop:disable Cop/PutGroupRoutesUnderScope
get :projects, as: :projects_group # rubocop:disable Cop/PutGroupRoutesUnderScope
get :details, as: :details_group # rubocop:disable Cop/PutGroupRoutesUnderScope
get :activity, as: :activity_group # rubocop:disable Cop/PutGroupRoutesUnderScope
put :transfer, as: :transfer_group # rubocop:disable Cop/PutGroupRoutesUnderScope
post :export, as: :export_group # rubocop:disable Cop/PutGroupRoutesUnderScope
get :download_export, as: :download_export_group # rubocop:disable Cop/PutGroupRoutesUnderScope
# TODO: Remove as part of refactor in https://gitlab.com/gitlab-org/gitlab-foss/issues/49693
get 'shared', action: :show, as: :group_shared
get 'archived', action: :show, as: :group_archived
get 'shared', action: :show, as: :group_shared # rubocop:disable Cop/PutGroupRoutesUnderScope
get 'archived', action: :show, as: :group_archived # rubocop:disable Cop/PutGroupRoutesUnderScope
end
get '/', action: :show, as: :group_canonical
# These routes are legit and the cop rule will be improved in
# https://gitlab.com/gitlab-org/gitlab/-/issues/230703
get '/', action: :show, as: :group_canonical # rubocop:disable Cop/PutGroupRoutesUnderScope
end
scope(path: 'groups/*group_id/-',
......@@ -106,9 +110,11 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
as: :group,
constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ },
controller: :groups) do
get '/', action: :show
patch '/', action: :update
put '/', action: :update
delete '/', action: :destroy
# These routes are legit and the cop rule will be improved in
# https://gitlab.com/gitlab-org/gitlab/-/issues/230703
get '/', action: :show # rubocop:disable Cop/PutGroupRoutesUnderScope
patch '/', action: :update # rubocop:disable Cop/PutGroupRoutesUnderScope
put '/', action: :update # rubocop:disable Cop/PutGroupRoutesUnderScope
delete '/', action: :destroy # rubocop:disable Cop/PutGroupRoutesUnderScope
end
end
......@@ -362,18 +362,18 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
#
# Service Desk
#
get '/service_desk' => 'service_desk#show', as: :service_desk
put '/service_desk' => 'service_desk#update', as: :service_desk_refresh
get '/service_desk' => 'service_desk#show', as: :service_desk # rubocop:todo Cop/PutProjectRoutesUnderScope
put '/service_desk' => 'service_desk#update', as: :service_desk_refresh # rubocop:todo Cop/PutProjectRoutesUnderScope
#
# Templates
#
get '/templates/:template_type/:key' => 'templates#show',
get '/templates/:template_type/:key' => 'templates#show', # rubocop:todo Cop/PutProjectRoutesUnderScope
as: :template,
defaults: { format: 'json' },
constraints: { key: %r{[^/]+}, template_type: %r{issue|merge_request}, format: 'json' }
get '/description_templates/names/:template_type',
get '/description_templates/names/:template_type', # rubocop:todo Cop/PutProjectRoutesUnderScope
to: 'templates#names',
as: :template_names,
defaults: { format: 'json' },
......@@ -382,39 +382,39 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
resource :pages, only: [:show, :update, :destroy] do # rubocop: disable Cop/PutProjectRoutesUnderScope
resources :domains, except: :index, controller: 'pages_domains', constraints: { id: %r{[^/]+} } do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
post :verify
post :retry_auto_ssl
delete :clean_certificate
post :verify # rubocop:todo Cop/PutProjectRoutesUnderScope
post :retry_auto_ssl # rubocop:todo Cop/PutProjectRoutesUnderScope
delete :clean_certificate # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
end
namespace :prometheus do
resources :alerts, constraints: { id: /\d+/ }, only: [:index, :create, :show, :update, :destroy] do # rubocop: disable Cop/PutProjectRoutesUnderScope
post :notify, on: :collection
post :notify, on: :collection # rubocop:todo Cop/PutProjectRoutesUnderScope
member do
get :metrics_dashboard
get :metrics_dashboard # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
resources :metrics, constraints: { id: %r{[^\/]+} }, only: [:index, :new, :create, :edit, :update, :destroy] do # rubocop: disable Cop/PutProjectRoutesUnderScope
get :active_common, on: :collection
post :validate_query, on: :collection
get :active_common, on: :collection # rubocop:todo Cop/PutProjectRoutesUnderScope
post :validate_query, on: :collection # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
post 'alerts/notify', to: 'alerting/notifications#create'
post 'alerts/notify', to: 'alerting/notifications#create' # rubocop:todo Cop/PutProjectRoutesUnderScope
draw :legacy_builds
resources :hooks, only: [:index, :create, :edit, :update, :destroy], constraints: { id: /\d+/ } do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
post :test
post :test # rubocop:todo Cop/PutProjectRoutesUnderScope
end
resources :hook_logs, only: [:show] do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
post :retry
post :retry # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
end
......@@ -431,7 +431,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
resources :tags, only: [:index, :destroy], # rubocop: disable Cop/PutProjectRoutesUnderScope
constraints: { id: Gitlab::Regex.container_registry_tag_regex } do
collection do
delete :bulk_destroy
delete :bulk_destroy # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
end
......@@ -440,32 +440,32 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
resources :notes, only: [:create, :destroy, :update], concerns: :awardable, constraints: { id: /\d+/ } do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
delete :delete_attachment
post :resolve
delete :resolve, action: :unresolve
delete :delete_attachment # rubocop:todo Cop/PutProjectRoutesUnderScope
post :resolve # rubocop:todo Cop/PutProjectRoutesUnderScope
delete :resolve, action: :unresolve # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
get 'noteable/:target_type/:target_id/notes' => 'notes#index', as: 'noteable_notes'
get 'noteable/:target_type/:target_id/notes' => 'notes#index', as: 'noteable_notes' # rubocop:todo Cop/PutProjectRoutesUnderScope
resources :todos, only: [:create] # rubocop: disable Cop/PutProjectRoutesUnderScope
resources :uploads, only: [:create] do # rubocop: disable Cop/PutProjectRoutesUnderScope
collection do
get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil }
post :authorize
get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil } # rubocop:todo Cop/PutProjectRoutesUnderScope
post :authorize # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
resources :runners, only: [:index, :edit, :update, :destroy, :show] do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
post :resume
post :pause
post :resume # rubocop:todo Cop/PutProjectRoutesUnderScope
post :pause # rubocop:todo Cop/PutProjectRoutesUnderScope
end
collection do
post :toggle_shared_runners
post :toggle_group_runners
post :toggle_shared_runners # rubocop:todo Cop/PutProjectRoutesUnderScope
post :toggle_group_runners # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
......@@ -474,26 +474,26 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
collection do
scope '*ref', constraints: { ref: Gitlab::PathRegex.git_reference_regex } do
constraints format: /svg/ do
get :pipeline
get :coverage
get :pipeline # rubocop:todo Cop/PutProjectRoutesUnderScope
get :coverage # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
end
end
scope :usage_ping, controller: :usage_ping do
post :web_ide_clientside_preview
post :web_ide_pipelines_count
post :web_ide_clientside_preview # rubocop:todo Cop/PutProjectRoutesUnderScope
post :web_ide_pipelines_count # rubocop:todo Cop/PutProjectRoutesUnderScope
end
resources :web_ide_terminals, path: :ide_terminals, only: [:create, :show], constraints: { id: /\d+/, format: :json } do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
post :cancel
post :retry
post :cancel # rubocop:todo Cop/PutProjectRoutesUnderScope
post :retry # rubocop:todo Cop/PutProjectRoutesUnderScope
end
collection do
post :check_config
post :check_config # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
......@@ -506,8 +506,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
# Issue https://gitlab.com/gitlab-org/gitlab/-/issues/29572
resources :snippets, concerns: :awardable, constraints: { id: /\d+/ } do # rubocop: disable Cop/PutProjectRoutesUnderScope
member do
get :raw
post :mark_as_spam
get :raw # rubocop:todo Cop/PutProjectRoutesUnderScope
post :mark_as_spam # rubocop:todo Cop/PutProjectRoutesUnderScope
end
end
end
......
......@@ -5,7 +5,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
controller: :groups,
constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom|ics)/ }) do
scope(path: '-') do
get :subgroups, as: :subgroups_group
get :subgroups, as: :subgroups_group # rubocop:todo Cop/PutGroupRoutesUnderScope
end
end
......@@ -184,10 +184,10 @@ end
# Dependency proxy for containers
# Because docker adds v2 prefix to URI this need to be outside of usual group routes
scope format: false do
get 'v2', to: proc { [200, {}, ['']] }
get 'v2', to: proc { [200, {}, ['']] } # rubocop:disable Cop/PutGroupRoutesUnderScope
constraints image: Gitlab::PathRegex.container_image_regex, sha: Gitlab::PathRegex.container_image_blob_sha_regex do
get 'v2/*group_id/dependency_proxy/containers/*image/manifests/*tag' => 'groups/dependency_proxy_for_containers#manifest'
get 'v2/*group_id/dependency_proxy/containers/*image/blobs/:sha' => 'groups/dependency_proxy_for_containers#blob'
get 'v2/*group_id/dependency_proxy/containers/*image/manifests/*tag' => 'groups/dependency_proxy_for_containers#manifest' # rubocop:todo Cop/PutGroupRoutesUnderScope
get 'v2/*group_id/dependency_proxy/containers/*image/blobs/:sha' => 'groups/dependency_proxy_for_containers#blob' # rubocop:todo Cop/PutGroupRoutesUnderScope
end
end
......@@ -141,6 +141,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
end
end
# It's under /-/jira scope but cop is only checking /-/
# rubocop: disable Cop/PutProjectRoutesUnderScope
scope path: '(/-/jira)', constraints: ::Constraints::JiraEncodedUrlConstrainer.new, as: :jira do
scope path: '*namespace_id/:project_id',
namespace_id: Gitlab::Jira::Dvcs::ENCODED_ROUTE_REGEX,
......@@ -171,3 +173,4 @@ scope path: '(/-/jira)', constraints: ::Constraints::JiraEncodedUrlConstrainer.n
}
end
end
# rubocop: enable Cop/PutProjectRoutesUnderScope
# frozen_string_literal: true
require_relative '../routes_under_scope'
module RuboCop
module Cop
# Checks for a group routes outside '/-/' scope.
# For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572
class PutGroupRoutesUnderScope < RuboCop::Cop::Cop
include RoutesUnderScope
MSG = 'Put new group routes under /-/ scope'
def_node_matcher :dash_scope?, <<~PATTERN
(:send nil? :scope (hash <(pair (sym :path)(str "groups/*group_id/-")) ...>))
PATTERN
def on_send(node)
return unless in_group_routes?(node)
return unless resource?(node)
return unless outside_scope?(node)
add_offense(node)
end
def outside_scope?(node)
node.each_ancestor(:block).none? do |parent|
dash_scope?(parent.to_a.first)
end
end
def in_group_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?('group.rb')
end
def resource?(node)
node.method_name == :resource ||
node.method_name == :resources
end
end
end
end
# frozen_string_literal: true
require_relative '../routes_under_scope'
module RuboCop
module Cop
# Checks for a project routes outside '/-/' scope.
# For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572
class PutProjectRoutesUnderScope < RuboCop::Cop::Cop
include RoutesUnderScope
MSG = 'Put new project routes under /-/ scope'
def_node_matcher :dash_scope?, <<~PATTERN
(:send nil? :scope (:str "-"))
PATTERN
def on_send(node)
return unless in_project_routes?(node)
return unless resource?(node)
return unless outside_scope?(node)
add_offense(node)
end
def outside_scope?(node)
node.each_ancestor(:block).none? do |parent|
dash_scope?(parent.to_a.first)
end
end
def in_project_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?('project.rb')
end
def resource?(node)
node.method_name == :resource ||
node.method_name == :resources
end
end
end
end
# frozen_string_literal: true
module RuboCop
# Common code used to implement cops checking routes outside of /-/ scope.
#
# Examples:
# * RuboCop::Cop::PutProjectRoutesUnderScope
# * RuboCop::Cop::PutGroupRoutesUnderScope
module RoutesUnderScope
ROUTE_METHODS = Set.new(%i[resource resources get post put patch delete]).freeze
def on_send(node)
return unless route_method?(node)
return unless outside_scope?(node)
add_offense(node)
end
def outside_scope?(node)
node.each_ancestor(:block).none? do |parent|
dash_scope?(parent.to_a.first)
end
end
def route_method?(node)
ROUTE_METHODS.include?(node.method_name)
end
end
end
......@@ -9,19 +9,20 @@ RSpec.describe RuboCop::Cop::PutGroupRoutesUnderScope, type: :rubocop do
subject(:cop) { described_class.new }
before do
allow(cop).to receive(:in_group_routes?).and_return(true)
end
%w[resource resources get post put patch delete].each do |route_method|
it "registers an offense when route is outside scope for `#{route_method}`" do
offense = "#{route_method} :notes"
marker = '^' * offense.size
it 'registers an offense when route is outside scope' do
expect_offense(<<~PATTERN)
expect_offense(<<~PATTERN)
scope(path: 'groups/*group_id/-', module: :groups) do
resource :issues
end
resource :notes
^^^^^^^^^^^^^^^ Put new group routes under /-/ scope
PATTERN
#{offense}
#{marker} Put new group routes under /-/ scope
PATTERN
end
end
it 'does not register an offense when resource inside the scope' do
......
......@@ -9,19 +9,20 @@ RSpec.describe RuboCop::Cop::PutProjectRoutesUnderScope, type: :rubocop do
subject(:cop) { described_class.new }
before do
allow(cop).to receive(:in_project_routes?).and_return(true)
end
%w[resource resources get post put patch delete].each do |route_method|
it "registers an offense when route is outside scope for `#{route_method}`" do
offense = "#{route_method} :notes"
marker = '^' * offense.size
it 'registers an offense when route is outside scope' do
expect_offense(<<~PATTERN)
expect_offense(<<~PATTERN)
scope '-' do
resource :issues
end
resource :notes
^^^^^^^^^^^^^^^ Put new project routes under /-/ scope
PATTERN
#{offense}
#{marker} Put new project routes under /-/ scope
PATTERN
end
end
it 'does not register an offense when resource inside the scope' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment