Add GitlabSecurity cops from rubocop-gitlab-security
What does this MR do and why?
This MR moves all cops related to GitlabSecurity from
rubocop-gitlab-security into gitlab-styles.
It also adds missing specs and modernizes the implementation where possible.
GitlabSecurity/JsonSerialization is an exception and it still inherits
from a deprecated RuboCop class
(see InternalAffairs/InheritDeprecatedCopClass). See .rubocop_todo.yml and the follow-up issue Let GitlabSecurity/JsonSerialization inherit fr... (#48).
Contributes to gitlab-org/rubocop-gitlab-security#5 (closed)
Testing on gitlab-org/gitlab
See gitlab-org/gitlab!108701 (closed)
How to verify locally
rubocop --show-cops diff
Diff for rubocop --show-cops on master and this MR:
Click to expand
--- before 2023-01-11 14:07:16.728389332 +0100
+++ after 2023-01-11 14:12:14.048160372 +0100
@@ -215,20 +215,20 @@
GitlabSecurity/DeepMunge:
Description: Checks for disabling the deep munge security control.
Enabled: true
- StyleGuide: http://www.rubydoc.info/gems/rubocop-gitlab-security/RuboCop/Cop/GitlabSecurity/DeepMunge
+ StyleGuide: https://www.rubydoc.info/gems/gitlab-styles/RuboCop/Cop/GitlabSecurity/DeepMunge
Exclude:
- "/home/peter/devel/gitlab/gitlab-styles/lib/**/*.rake"
- "/home/peter/devel/gitlab/gitlab-styles/spec/**/*"
GitlabSecurity/JsonSerialization:
- Description: Checks for `to_json` / `as_json` without whitelisting via `only`.
+ Description: Checks for `to_json` / `as_json` without allowing via `only`.
Enabled: false
- StyleGuide: http://www.rubydoc.info/gems/rubocop-gitlab-security/RuboCop/Cop/GitlabSecurity/JsonSerialization
+ StyleGuide: https://www.rubydoc.info/gems/gitlab-styles/RuboCop/Cop/GitlabSecurity/JsonSerialization
GitlabSecurity/PublicSend:
Description: Checks for the use of `public_send`, `send`, and `__send__` methods.
Enabled: true
- StyleGuide: http://www.rubydoc.info/gems/rubocop-gitlab-security/RuboCop/Cop/GitlabSecurity/PublicSend
+ StyleGuide: https://www.rubydoc.info/gems/gitlab-styles/RuboCop/Cop/GitlabSecurity/PublicSend
Exclude:
- "/home/peter/devel/gitlab/gitlab-styles/config/**/*"
- "/home/peter/devel/gitlab/gitlab-styles/db/**/*"
@@ -421,6 +421,8 @@
Enabled: true
InternalAffairs/InheritDeprecatedCopClass:
+ Exclude:
+ - "/home/peter/devel/gitlab/gitlab-styles/lib/rubocop/cop/gitlab_security/json_serialization.rb"
Enabled: true
# Supports --autocorrect
Diff between rubocop-gitlab-security and gitlab-styles
Done via diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security lib/rubocop/cop/gitlab_security.
The cops
Only in ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security: cop.rb
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/deep_munge.rb lib/rubocop/cop/gitlab_security/deep_munge.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/deep_munge.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/deep_munge.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
@@ -12,9 +14,10 @@
# config.action_dispatch.perform_deep_munge = false
#
# See CVE-2012-2660, CVE-2012-2694, and CVE-2013-0155.
- class DeepMunge < RuboCop::Cop::Cop
- MSG = 'Never disable the deep munge security option.'.freeze
+ class DeepMunge < RuboCop::Cop::Base
+ MSG = 'Never disable the deep munge security option.'
+ # @!method disable_deep_munge?(node)
def_node_matcher :disable_deep_munge?, <<-PATTERN
(send (send (send nil? :config) :action_dispatch) :perform_deep_munge= (false))
PATTERN
@@ -22,7 +25,7 @@
def on_send(node)
return unless disable_deep_munge?(node)
- add_offense(node, location: :selector)
+ add_offense(node.loc.selector)
end
end
end
Only in ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security: .gitkeep
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/json_serialization.rb lib/rubocop/cop/gitlab_security/json_serialization.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/json_serialization.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/json_serialization.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,7 +1,9 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
- # Checks for `to_json` / `as_json` without whitelisting via `only`.
+ # Checks for `to_json` / `as_json` without allowing via `only`.
#
# Either method called on an instance of a `Serializer` class will be
# ignored. Associations included via `include` are subject to the same
@@ -29,30 +31,35 @@
#
# See https://gitlab.com/gitlab-org/gitlab-ce/issues/29661
class JsonSerialization < RuboCop::Cop::Cop
- MSG = "Don't use `%s` without specifying `only`".freeze
+ MSG = "Don't use `%s` without specifying `only`"
# Check for `to_json` sent to any object that's not a Hash literal or
# Serializer instance
+ # @!method json_serialization?(node)
def_node_matcher :json_serialization?, <<~PATTERN
(send !{nil? hash #serializer?} ${:to_json :as_json} $...)
PATTERN
# Check if node is a `only: ...` pair
+ # @!method only_pair?(node)
def_node_matcher :only_pair?, <<~PATTERN
(pair (sym :only) ...)
PATTERN
# Check if node is a `include: {...}` pair
+ # @!method include_pair?(node)
def_node_matcher :include_pair?, <<~PATTERN
(pair (sym :include) (hash $...))
PATTERN
# Check for a `only: [...]` pair anywhere in the node
+ # @!method contains_only?(node)
def_node_search :contains_only?, <<~PATTERN
(pair (sym :only) (array ...))
PATTERN
# Check for `SomeConstant.new`
+ # @!method constant_init(node)
def_node_search :constant_init, <<~PATTERN
(send (const nil? $_) :new ...)
PATTERN
@@ -98,7 +105,7 @@
# Add a top-level offense for the entire argument list, but only if
# we haven't yet added any offenses to the child Hash values (such
# as `include`)
- add_offense(node.children.last, location: :expression, message: format_message)
+ add_offense(node.children.last, message: format_message)
end
def check_pair(pair)
@@ -110,7 +117,7 @@
includes.each_child_node do |child_node|
next if contains_only?(child_node)
- add_offense(child_node, location: :expression, message: format_message)
+ add_offense(child_node, message: format_message)
end
end
end
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/public_send.rb lib/rubocop/cop/gitlab_security/public_send.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/public_send.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/public_send.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
@@ -20,9 +22,10 @@
# when 'choice3'
# items.choice3
# end
- class PublicSend < RuboCop::Cop::Cop
- MSG = 'Avoid using `%s`.'.freeze
+ class PublicSend < RuboCop::Cop::Base
+ MSG = 'Avoid using `%s`.'
+ # @!method send?(node)
def_node_matcher :send?, <<-PATTERN
(send _ ${:send :public_send :__send__} ...)
PATTERN
@@ -31,7 +34,7 @@
send?(node) do |match|
next unless node.arguments?
- add_offense(node, location: :selector, message: format(MSG, match))
+ add_offense(node.loc.selector, message: format(MSG, match))
end
end
end
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/redirect_to_params_update.rb lib/rubocop/cop/gitlab_security/redirect_to_params_update.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/redirect_to_params_update.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/redirect_to_params_update.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
@@ -8,22 +10,27 @@
# @example
#
# # bad
- # redirect_to(params.update(action:'main'))
+ # redirect_to(params.update(action: 'main'))
#
# # good
- # redirect_to(whitelist(params))
+ # redirect_to(allowed(params))
#
- class RedirectToParamsUpdate < RuboCop::Cop::Cop
- MSG = 'Avoid using redirect_to(params.update()). Only pass whitelisted arguments into redirect_to() (e.g. not including `host`)'.freeze
+ class RedirectToParamsUpdate < RuboCop::Cop::Base
+ MSG = 'Avoid using `redirect_to(params.%<name>s(...))`. ' \
+ 'Only pass allowed arguments into redirect_to() (e.g. not including `host`)'
+ # @!method redirect_to_params_update_node(node)
def_node_matcher :redirect_to_params_update_node, <<-PATTERN
- (send nil :redirect_to (send (send nil? :params) ${:update :merge} ...))
+ (send nil? :redirect_to $(send (send nil? :params) ${:update :merge} ...))
PATTERN
def on_send(node)
- return unless redirect_to_params_update_node(node)
+ selected, name = redirect_to_params_update_node(node)
+ return unless name
+
+ message = format(MSG, name: name)
- add_offense(node, location: :selector)
+ add_offense(selected, message: message)
end
end
end
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/send_file_params.rb lib/rubocop/cop/gitlab_security/send_file_params.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/send_file_params.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/send_file_params.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
@@ -17,11 +19,11 @@
# raise if basename != filename
# send_file filename, disposition: 'inline'
#
- class SendFileParams < RuboCop::Cop::Cop
- MSG = 'Do not pass user provided params directly to send_file(), verify
- the path with file.expand_path() first. If the path has already been verified
- this warning can be disabled using `#rubocop:disable GitlabSecurity/SendFileParams`'.freeze
+ class SendFileParams < RuboCop::Cop::Base
+ MSG = 'Do not pass user provided params directly to send_file(), ' \
+ 'verify the path with file.expand_path() first.'
+ # @!method params_node?(node)
def_node_search :params_node?, <<-PATTERN
(send (send nil? :params) ... )
PATTERN
@@ -30,7 +32,7 @@
return unless node.command?(:send_file)
return unless node.arguments.any? { |e| params_node?(e) }
- add_offense(node, location: :selector)
+ add_offense(node.loc.selector)
end
end
end
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/sql_injection.rb lib/rubocop/cop/gitlab_security/sql_injection.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/sql_injection.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/sql_injection.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
@@ -14,14 +16,15 @@
# u = User.where("name = ? AND id = ?", params[:name], params[:id])
# u = User.where(name: params[:name], id: params[:id])
#
- class SqlInjection < RuboCop::Cop::Cop
- MSG = 'Parameterize all user-input passed to where(), do not directly embed user input in SQL queries.
- If this warning is in error you can white-list the line with `#rubocop:disable GitlabSecurity/SqlInjection`'.freeze
+ class SqlInjection < RuboCop::Cop::Base
+ MSG = 'Parameterize all user-input passed to where(), do not directly embed user input in SQL queries.'
+ # @!method where_user_input?(node)
def_node_matcher :where_user_input?, <<-PATTERN
(send _ :where ...)
PATTERN
+ # @!method string_var_string?(node)
def_node_matcher :string_var_string?, <<-PATTERN
(dstr (str ...) (begin ...) (str ...) ...)
PATTERN
@@ -30,7 +33,7 @@
return unless where_user_input?(node)
return unless node.arguments.any? { |e| string_var_string?(e) }
- add_offense(node, location: :selector)
+ add_offense(node.loc.selector)
end
end
end
diff -ru ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/system_command_injection.rb lib/rubocop/cop/gitlab_security/system_command_injection.rb
--- ../rubocop-gitlab-security/lib/rubocop/cop/gitlab-security/system_command_injection.rb 2023-01-10 20:39:32.037125914 +0100
+++ lib/rubocop/cop/gitlab_security/system_command_injection.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module RuboCop
module Cop
module GitlabSecurity
@@ -15,10 +17,11 @@
# # even better
# exec("/bin/ls", shell_escape(filename))
#
- class SystemCommandInjection < RuboCop::Cop::Cop
- MSG = 'Do not include variables in the command name for system(). Use parameters "system(cmd, params)" or exec() instead.
- If this warning is in error you can white-list the line with `#rubocop:disable GitlabSecurity/SystemCommandInjection`'.freeze
+ class SystemCommandInjection < RuboCop::Cop::Base
+ MSG = 'Do not include variables in the command name for system(). ' \
+ 'Use parameters "system(cmd, params)" or exec() instead.'
+ # @!method system_var?(node)
def_node_matcher :system_var?, <<-PATTERN
(dstr (str ...) (begin ...) ...)
PATTERN
@@ -27,7 +30,7 @@
return unless node.command?(:system)
return unless node.arguments.any? { |e| system_var?(e) }
- add_offense(node, location: :selector)
+ add_offense(node.loc.selector)
end
end
endDone via diff -ru ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security spec/rubocop/cop/gitlab_security:
The specs
diff -ru ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security/deep_munge_spec.rb spec/rubocop/cop/gitlab_security/deep_munge_spec.rb
--- ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security/deep_munge_spec.rb 2023-01-10 20:39:32.041125855 +0100
+++ spec/rubocop/cop/gitlab_security/deep_munge_spec.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,14 +1,18 @@
-RSpec.describe RuboCop::Cop::GitlabSecurity::DeepMunge do
- subject(:cop) { described_class.new }
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../../../../lib/rubocop/cop/gitlab_security/deep_munge'
+
+RSpec.describe RuboCop::Cop::GitlabSecurity::DeepMunge do
it 'ignores setting to true' do
- expect_no_offenses(<<-RUBY)
+ expect_no_offenses(<<~RUBY)
config.action_dispatch.perform_deep_munge = true
RUBY
end
it 'adds an offense for setting it to `false`' do
- expect_offense(<<-RUBY)
+ expect_offense(<<~RUBY)
config.action_dispatch.perform_deep_munge = false
^^^^^^^^^^^^^^^^^^ Never disable the deep munge security option.
RUBY
Only in spec/rubocop/cop/gitlab_security: .deep_munge_spec.rb.swp
diff -ru ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security/json_serialization_spec.rb spec/rubocop/cop/gitlab_security/json_serialization_spec.rb
--- ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security/json_serialization_spec.rb 2023-01-10 20:39:32.041125855 +0100
+++ spec/rubocop/cop/gitlab_security/json_serialization_spec.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,12 +1,12 @@
-RSpec.describe RuboCop::Cop::GitlabSecurity::JsonSerialization do
- subject(:cop) { described_class.new }
+# frozen_string_literal: true
- def highlight_for(method)
- "#{'^' * method.to_s.length} #{message_for(method)}"
- end
+require 'spec_helper'
+require_relative '../../../../lib/rubocop/cop/gitlab_security/json_serialization'
+
+RSpec.describe RuboCop::Cop::GitlabSecurity::JsonSerialization do
def message_for(method)
- format(described_class::MSG, method)
+ format("Don't use `%s` without specifying `only`", method)
end
shared_examples 'an upstanding constable' do |method|
@@ -25,11 +25,11 @@
it 'does nothing when sent to a Serializer instance' do
aggregate_failures do
- expect_no_offenses(<<-RUBY)
+ expect_no_offenses(<<~RUBY)
IssueSerializer.new.represent(issuable).#{method}
RUBY
- expect_no_offenses(<<-RUBY)
+ expect_no_offenses(<<~RUBY)
MergeRequestSerializer
.new(current_user: current_user, project: issuable.project)
.represent(issuable)
@@ -39,22 +39,22 @@
end
it 'adds an offense when sent to any other receiver' do
- expect_offense(<<-RUBY)
- foo.#{method}
- #{highlight_for(method)}
+ expect_offense(<<-'RUBY', method: method, message: message_for(method))
+ foo.%{method}
+ ^{method} %{message}
RUBY
end
end
context "`#{method}` with options" do
it 'does nothing when provided `only`' do
- expect_no_offenses(<<-RUBY)
+ expect_no_offenses(<<~RUBY)
render json: @issue.#{method}(only: [:name, :username])
RUBY
end
it 'does nothing when provided `only` and `methods`' do
- expect_no_offenses(<<-RUBY)
+ expect_no_offenses(<<~RUBY)
render json: @issue.#{method}(
only: [:name, :username],
methods: [:avatar_url]
@@ -63,7 +63,7 @@
end
it 'adds an offense to `include`d attributes without `only` option' do
- expect_offense(<<-RUBY)
+ expect_offense(<<~RUBY)
render json: @issue.#{method}(
include: {
milestone: {},
@@ -77,7 +77,7 @@
end
it 'handles a top-level `only` with child `include`s' do
- expect_offense(<<-RUBY)
+ expect_offense(<<~RUBY)
render json: @issue.#{method}(
only: [:foo, :bar],
include: {
@@ -91,7 +91,7 @@
it 'adds an offense for `include: [...]`' do
# Spacing is off on the highlight due to interpolation
- expect_offense(<<-RUBY)
+ expect_offense(<<~RUBY)
render json: @issue.#{method}(include: %i[foo bar baz])
^^^^^^^^^^^^^^^^^^^^^^^^ #{message_for(method)}
RUBY
@@ -99,7 +99,7 @@
lib/rubocop/cop/gitlab_security
it 'adds an offense for `except`' do
# Spacing is off on the highlight due to interpolation
- expect_offense(<<-RUBY)
+ expect_offense(<<~RUBY)
render json: @issue.#{method}(except: [:private_token])
^^^^^^^^^^^^^^^^^^^^^^^^ #{message_for(method)}
RUBY
diff -ru ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security/public_send_spec.rb spec/rubocop/cop/gitlab_security/public_send_spec.rb
--- ../rubocop-gitlab-security/spec/rubocop/cop/gitlab-security/public_send_spec.rb 2023-01-10 20:39:32.041125855 +0100
+++ spec/rubocop/cop/gitlab_security/public_send_spec.rb 2023-01-11 14:07:52.135886598 +0100
@@ -1,29 +1,29 @@
-RSpec.describe RuboCop::Cop::GitlabSecurity::PublicSend do
- subject(:cop) { described_class.new }
+# frozen_string_literal: true
- shared_examples 'an upstanding constable' do |method|
- def highlight_for(method)
- "#{'^' * method.to_s.length} Avoid using `#{method}`."
- end
+require 'spec_helper'
+require_relative '../../../../lib/rubocop/cop/gitlab_security/public_send'
+
+RSpec.describe RuboCop::Cop::GitlabSecurity::PublicSend do
+ shared_examples 'an upstanding constable' do |method|
it "adds an offense for `#{method}`" do
- expect_offense(<<-RUBY)
- basic.#{method}(:bar)
- #{highlight_for(method)}
+ expect_offense(<<~RUBY, method: method)
+ basic.%{method}(:bar)
+ ^{method} Avoid using `%{method}`.
RUBY
end
it "adds an offense for `#{method}` with arguments" do
- expect_offense(<<-RUBY)
- args.#{method}(:bar, baz: true)
- #{highlight_for(method)}
+ expect_offense(<<~RUBY, method: method)
+ args.%{method}(:bar, baz: true)
+ ^{method} Avoid using `%{method}`.
RUBY
end
it "adds offense for `#{method}` on `nil` receiver" do
- expect_offense(<<-RUBY)
- #{method}(:foo)
- #{highlight_for(method)}
+ expect_offense(<<~RUBY, method: method)
+ %{method}(:foo)
+ ^{method} Avoid using `%{method}`.
RUBY
end
Only in spec/rubocop/cop/gitlab_security: redirect_to_params_update_spec.rb
Only in spec/rubocop/cop/gitlab_security: send_file_params_spec.rb
Only in spec/rubocop/cop/gitlab_security: sql_injection_spec.rb
Only in spec/rubocop/cop/gitlab_security: system_command_injection_spec.rbEdited by Peter Leitzen