diff options
author | Brian Neel <brian@gitlab.com> | 2017-08-03 22:20:34 -0400 |
---|---|---|
committer | Brian Neel <brian@gitlab.com> | 2017-08-08 10:50:54 -0400 |
commit | 9770c57fab0315865a33c8b6df269eded0d57b5c (patch) | |
tree | 5a7c7a9fccbce5ef3ccf6b02b1297aace41101fd | |
parent | b612a47da0e0225332a59ab961206f84602ad629 (diff) | |
download | gitlab-ce-9770c57fab0315865a33c8b6df269eded0d57b5c.tar.gz |
Re-enable SqlInjection and CommandInjection
34 files changed, 90 insertions, 48 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f55bdc19eec..e10958b3bee 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -514,8 +514,11 @@ codeclimate: services: - docker:dind script: + - cp .rubocop.yml .rubocop.yml.bak + - grep -v "rubocop-gitlab-security" .rubocop.yml.bak > .rubocop.yml - docker run --env CODECLIMATE_CODE="$PWD" --volume "$PWD":/code --volume /var/run/docker.sock:/var/run/docker.sock --volume /tmp/cc:/tmp/cc codeclimate/codeclimate analyze -f json > raw_codeclimate.json - cat raw_codeclimate.json | docker run -i stedolan/jq -c 'map({check_name,fingerprint,location})' > codeclimate.json + - mv .rubocop.yml.bak .rubocop.yml artifacts: paths: [codeclimate.json] diff --git a/.rubocop.yml b/.rubocop.yml index a5ccec0437b..876828f68f1 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,5 +1,6 @@ require: - rubocop-rspec + - rubocop-gitlab-security - ./rubocop/rubocop inherit_from: .rubocop_todo.yml @@ -1156,3 +1157,35 @@ RSpec/SubjectStub: # Prefer using verifying doubles over normal doubles. RSpec/VerifiedDoubles: Enabled: false + +# GitlabSecurity ############################################################## + +GitlabSecurity/DeepMunge: + Enabled: true + Exclude: + - 'spec/**/*' + - 'lib/**/*.rake' + +GitlabSecurity/PublicSend: + Enabled: true + Exclude: + - 'spec/**/*' + - 'lib/**/*.rake' + +GitlabSecurity/RedirectToParamsUpdate: + Enabled: true + Exclude: + - 'spec/**/*' + - 'lib/**/*.rake' + +GitlabSecurity/SqlInjection: + Enabled: true + Exclude: + - 'spec/**/*' + - 'lib/**/*.rake' + +GitlabSecurity/SystemCommandInjection: + Enabled: true + Exclude: + - 'spec/**/*' + - 'lib/**/*.rake' @@ -341,6 +341,7 @@ group :development, :test do gem 'rubocop', '~> 0.49.1', require: false gem 'rubocop-rspec', '~> 1.15.1', require: false + gem 'rubocop-gitlab-security', '~> 0.0.6', require: false gem 'scss_lint', '~> 0.54.0', require: false gem 'haml_lint', '~> 0.26.0', require: false gem 'simplecov', '~> 0.14.0', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 948ba02a72c..6c2fcc377ec 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -742,7 +742,8 @@ GEM ruby-progressbar (~> 1.7) unicode-display_width (~> 1.0, >= 1.0.1) rubocop-rspec (1.15.1) - rubocop (>= 0.42.0) + rubocop-gitlab-security (0.0.6) + rubocop (>= 0.47.0) ruby-fogbugz (0.2.1) crack (~> 0.4) ruby-prof (0.16.2) @@ -1089,6 +1090,7 @@ DEPENDENCIES rspec_profiling (~> 0.0.5) rubocop (~> 0.49.1) rubocop-rspec (~> 1.15.1) + rubocop-gitlab-security (~> 0.0.6) ruby-fogbugz (~> 0.2.1) ruby-prof (~> 0.16.2) ruby_parser (~> 3.8) diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 53a5981e564..baa6645e5ce 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -68,15 +68,15 @@ class Import::GithubController < Import::BaseController end def new_import_url - public_send("new_import_#{provider}_url") + public_send("new_import_#{provider}_url") # rubocop:disable GitlabSecurity/PublicSend end def status_import_url - public_send("status_import_#{provider}_url") + public_send("status_import_#{provider}_url") # rubocop:disable GitlabSecurity/PublicSend end def callback_import_url - public_send("callback_import_#{provider}_url") + public_send("callback_import_#{provider}_url") # rubocop:disable GitlabSecurity/PublicSend end def provider_unauthorized diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index f4fad7150e8..ce6754564aa 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -234,7 +234,7 @@ module IssuablesHelper end def issuables_count_for_state(issuable_type, state, finder: nil) - finder ||= public_send("#{issuable_type}_finder") + finder ||= public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend cache_key = finder.state_counter_cache_key @counts ||= {} diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 4b99de1b6a5..e60513b35c7 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -43,11 +43,11 @@ module LabelsHelper def label_filter_path(subject, label, type: :issue) case subject when Group - send("#{type.to_s.pluralize}_group_path", + send("#{type.to_s.pluralize}_group_path", # rubocop:disable GitlabSecurity/PublicSend subject, label_name: [label.name]) when Project - send("namespace_project_#{type.to_s.pluralize}_path", + send("namespace_project_#{type.to_s.pluralize}_path", # rubocop:disable GitlabSecurity/PublicSend subject.namespace, subject, label_name: [label.name]) diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index bd75f25a210..f2707022a4b 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -58,7 +58,7 @@ module Spammable options.fetch(:spam_title, false) end - public_send(attr.first) if attr && respond_to?(attr.first.to_sym) + public_send(attr.first) if attr && respond_to?(attr.first.to_sym) # rubocop:disable GitlabSecurity/PublicSend end def spam_description @@ -66,12 +66,12 @@ module Spammable options.fetch(:spam_description, false) end - public_send(attr.first) if attr && respond_to?(attr.first.to_sym) + public_send(attr.first) if attr && respond_to?(attr.first.to_sym) # rubocop:disable GitlabSecurity/PublicSend end def spammable_text result = self.class.spammable_attrs.map do |attr| - public_send(attr.first) + public_send(attr.first) # rubocop:disable GitlabSecurity/PublicSend end result.reject(&:blank?).join("\n") diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 1ca7f91dc03..a7d5de48c66 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -44,7 +44,8 @@ module TokenAuthenticatable end define_method("ensure_#{token_field}!") do - send("reset_#{token_field}!") if read_attribute(token_field).blank? + send("reset_#{token_field}!") if read_attribute(token_field).blank? # rubocop:disable GitlabSecurity/PublicSend + read_attribute(token_field) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e83b11f7668..f90194041b1 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -162,7 +162,7 @@ class MergeRequest < ActiveRecord::Base target = unscoped.where(target_project_id: relation).select(:id) union = Gitlab::SQL::Union.new([source, target]) - where("merge_requests.id IN (#{union.to_sql})") + where("merge_requests.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end WIP_REGEX = /\A\s*(\[WIP\]\s*|WIP:\s*|WIP\s+)+\s*/i.freeze diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb index cafdbe11849..670b26d4ca3 100644 --- a/app/models/merge_request_diff_commit.rb +++ b/app/models/merge_request_diff_commit.rb @@ -26,7 +26,7 @@ class MergeRequestDiffCommit < ActiveRecord::Base def to_hash Gitlab::Git::Commit::SERIALIZE_KEYS.each_with_object({}) do |key, hash| - hash[key] = public_send(key) + hash[key] = public_send(key) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 9b1cac64c44..245f8dddcf9 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -66,6 +66,6 @@ class NotificationSetting < ActiveRecord::Base alias_method :failed_pipeline?, :failed_pipeline def event_enabled?(event) - respond_to?(event) && !!public_send(event) + respond_to?(event) && !!public_send(event) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/models/project.rb b/app/models/project.rb index e7baba2ef08..58f01f2b8d5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -415,7 +415,7 @@ class Project < ActiveRecord::Base union = Gitlab::SQL::Union.new([projects, namespaces]) - where("projects.id IN (#{union.to_sql})") + where("projects.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end def search_by_title(query) @@ -825,7 +825,7 @@ class Project < ActiveRecord::Base if template.nil? # If no template, we should create an instance. Ex `build_gitlab_ci_service` - public_send("build_#{service_name}_service") + public_send("build_#{service_name}_service") # rubocop:disable GitlabSecurity/PublicSend else Service.build_from_template(id, template) end @@ -1326,7 +1326,7 @@ class Project < ActiveRecord::Base end def append_or_update_attribute(name, value) - old_values = public_send(name.to_s) + old_values = public_send(name.to_s) # rubocop:disable GitlabSecurity/PublicSend if Project.reflect_on_association(name).try(:macro) == :has_many && old_values.any? update_attribute(name, old_values + value) diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index c8fabb16dc1..fb1db0255aa 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -55,7 +55,7 @@ class ProjectFeature < ActiveRecord::Base end def access_level(feature) - public_send(ProjectFeature.access_level_attribute(feature)) + public_send(ProjectFeature.access_level_attribute(feature)) # rubocop:disable GitlabSecurity/PublicSend end def builds_enabled? @@ -80,7 +80,7 @@ class ProjectFeature < ActiveRecord::Base # which cannot be higher than repository access level def repository_children_level validator = lambda do |field| - level = public_send(field) || ProjectFeature::ENABLED + level = public_send(field) || ProjectFeature::ENABLED # rubocop:disable GitlabSecurity/PublicSend not_allowed = level > repository_access_level self.errors.add(field, "cannot have higher visibility level than repository access level") if not_allowed end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index aeaf63abab9..715b215d1db 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -14,7 +14,7 @@ class ProjectStatistics < ActiveRecord::Base def refresh!(only: nil) STATISTICS_COLUMNS.each do |column, generator| if only.blank? || only.include?(column) - public_send("update_#{column}") + public_send("update_#{column}") # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/models/repository.rb b/app/models/repository.rb index ff82b958255..049bebdbe42 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -300,7 +300,7 @@ class Repository expire_method_caches(to_refresh) - to_refresh.each { |method| send(method) } + to_refresh.each { |method| send(method) } # rubocop:disable GitlabSecurity/PublicSend end def expire_branch_cache(branch_name = nil) diff --git a/app/models/user.rb b/app/models/user.rb index 5148886eed7..43973425a4b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -528,7 +528,7 @@ class User < ActiveRecord::Base union = Gitlab::SQL::Union .new([groups.select(:id), authorized_projects.select(:namespace_id)]) - Group.where("namespaces.id IN (#{union.to_sql})") + Group.where("namespaces.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end # Returns a relation of groups the user has access to, including their parent @@ -719,8 +719,8 @@ class User < ActiveRecord::Base def sanitize_attrs %w[username skype linkedin twitter].each do |attr| - value = public_send(attr) - public_send("#{attr}=", Sanitize.clean(value)) if value.present? + value = public_send(attr) # rubocop:disable GitlabSecurity/PublicSend + public_send("#{attr}=", Sanitize.clean(value)) if value.present? # rubocop:disable GitlabSecurity/PublicSend end end @@ -779,7 +779,7 @@ class User < ActiveRecord::Base def with_defaults User.defaults.each do |k, v| - public_send("#{k}=", v) + public_send("#{k}=", v) # rubocop:disable GitlabSecurity/PublicSend end self @@ -919,7 +919,7 @@ class User < ActiveRecord::Base def ci_authorized_runners @ci_authorized_runners ||= begin runner_ids = Ci::RunnerProject - .where("ci_runner_projects.project_id IN (#{ci_projects_union.to_sql})") + .where("ci_runner_projects.project_id IN (#{ci_projects_union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection .select(:runner_id) Ci::Runner.specific.where(id: runner_ids) end diff --git a/app/serializers/analytics_build_entity.rb b/app/serializers/analytics_build_entity.rb index ad7ad020b03..bdc22d71202 100644 --- a/app/serializers/analytics_build_entity.rb +++ b/app/serializers/analytics_build_entity.rb @@ -35,6 +35,6 @@ class AnalyticsBuildEntity < Grape::Entity private def url_to(route, build, id = nil) - public_send("#{route}_url", build.project.namespace, build.project, id || build) + public_send("#{route}_url", build.project.namespace, build.project, id || build) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/serializers/analytics_issue_entity.rb b/app/serializers/analytics_issue_entity.rb index 44c50f18613..b7d95ea020f 100644 --- a/app/serializers/analytics_issue_entity.rb +++ b/app/serializers/analytics_issue_entity.rb @@ -24,6 +24,6 @@ class AnalyticsIssueEntity < Grape::Entity private def url_to(route, id) - public_send("#{route}_url", request.project.namespace, request.project, id) + public_send("#{route}_url", request.project.namespace, request.project, id) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/serializers/job_entity.rb b/app/serializers/job_entity.rb index d6de43bcbcb..72e56a2c77f 100644 --- a/app/serializers/job_entity.rb +++ b/app/serializers/job_entity.rb @@ -46,6 +46,6 @@ class JobEntity < Grape::Entity end def path_to(route, build) - send("#{route}_path", build.project.namespace, build.project, build) + send("#{route}_path", build.project.namespace, build.project, build) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/services/labels/transfer_service.rb b/app/services/labels/transfer_service.rb index d2ece354efc..775efed48eb 100644 --- a/app/services/labels/transfer_service.rb +++ b/app/services/labels/transfer_service.rb @@ -37,7 +37,7 @@ module Labels union = Gitlab::SQL::Union.new(label_ids) - Label.where("labels.id IN (#{union.to_sql})").reorder(nil).uniq + Label.where("labels.id IN (#{union.to_sql})").reorder(nil).uniq # rubocop:disable GitlabSecurity/SqlInjection end def group_labels_applied_to_issues diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index 4eeb9666bb0..64788da7299 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -4,7 +4,7 @@ class PagesWorker sidekiq_options queue: :pages, retry: false def perform(action, *arg) - send(action, *arg) + send(action, *arg) # rubocop:disable GitlabSecurity/PublicSend end def deploy(build_id) diff --git a/config/application.rb b/config/application.rb index 47887bf8596..f69dab4de39 100644 --- a/config/application.rb +++ b/config/application.rb @@ -176,7 +176,7 @@ module Gitlab next unless name.include?('namespace_project') define_method(name.sub('namespace_project', 'project')) do |project, *args| - send(name, project&.namespace, project, *args) + send(name, project&.namespace, project, *args) # rubocop:disable GitlabSecurity/PublicSend end end end diff --git a/config/initializers/active_record_locking.rb b/config/initializers/active_record_locking.rb index 9266ff0f615..150aaa2a8c2 100644 --- a/config/initializers/active_record_locking.rb +++ b/config/initializers/active_record_locking.rb @@ -18,7 +18,7 @@ module ActiveRecord lock_col = self.class.locking_column - previous_lock_value = send(lock_col).to_i + previous_lock_value = send(lock_col).to_i # rubocop:disable GitlabSecurity/PublicSend # This line is added as a patch previous_lock_value = nil if previous_lock_value == '0' || previous_lock_value == 0 @@ -48,7 +48,7 @@ module ActiveRecord # If something went wrong, revert the version. rescue Exception - send(lock_col + '=', previous_lock_value) + send(lock_col + '=', previous_lock_value) # rubocop:disable GitlabSecurity/PublicSend raise end end diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index d9cae1501f8..a50ea0b52aa 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -1,8 +1,10 @@ +# rubocop:disable GitlabSecurity/PublicSend + module API module Helpers module MembersHelpers def find_source(source_type, id) - public_send("find_#{source_type}!", id) + public_send("find_#{source_type}!", id) # rubocop:disable GitlabSecurity/PublicSend end def authorize_admin_source!(source_type, source) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 65ff89edf65..4e4e473994b 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -139,7 +139,7 @@ module API helpers do def find_project_noteable(noteables_str, noteable_id) - public_send("find_project_#{noteables_str.singularize}", noteable_id) + public_send("find_project_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend end def noteable_read_ability_name(noteable) diff --git a/lib/ci/ansi2html.rb b/lib/ci/ansi2html.rb index 55402101e43..8354fc8d595 100644 --- a/lib/ci/ansi2html.rb +++ b/lib/ci/ansi2html.rb @@ -254,7 +254,7 @@ module Ci def state state = STATE_PARAMS.inject({}) do |h, param| - h[param] = send(param) + h[param] = send(param) # rubocop:disable GitlabSecurity/PublicSend h end Base64.urlsafe_encode64(state.to_json) @@ -266,7 +266,7 @@ module Ci return if state[:offset].to_i > stream.size STATE_PARAMS.each do |param| - send("#{param}=".to_sym, state[param]) + send("#{param}=".to_sym, state[param]) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/lib/ci/charts.rb b/lib/ci/charts.rb index 872e418c788..76a69bf8a83 100644 --- a/lib/ci/charts.rb +++ b/lib/ci/charts.rb @@ -47,7 +47,7 @@ module Ci def collect query = project.pipelines - .where("? > #{Ci::Pipeline.table_name}.created_at AND #{Ci::Pipeline.table_name}.created_at > ?", @to, @from) + .where("? > #{Ci::Pipeline.table_name}.created_at AND #{Ci::Pipeline.table_name}.created_at > ?", @to, @from) # rubocop:disable GitlabSecurity/SqlInjection totals_count = grouped_count(query) success_count = grouped_count(query.success) diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 2d89ccfc354..0603141e441 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -21,7 +21,7 @@ module Gitlab def to_hash hash = {} - serialize_keys.each { |key| hash[key] = send(key) } + serialize_keys.each { |key| hash[key] = send(key) } # rubocop:disable GitlabSecurity/PublicSend hash end diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 9256663f454..fd4dfdb09a2 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -319,7 +319,7 @@ module Gitlab def to_hash serialize_keys.map.with_object({}) do |key, hash| - hash[key] = send(key) + hash[key] = send(key) # rubocop:disable GitlabSecurity/PublicSend end end @@ -412,7 +412,7 @@ module Gitlab raw_commit = hash.symbolize_keys serialize_keys.each do |key| - send("#{key}=", raw_commit[key]) + send("#{key}=", raw_commit[key]) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 9e00abefd02..ce3d65062e8 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -143,7 +143,7 @@ module Gitlab hash = {} SERIALIZE_KEYS.each do |key| - hash[key] = send(key) + hash[key] = send(key) # rubocop:disable GitlabSecurity/PublicSend end hash @@ -221,7 +221,7 @@ module Gitlab raw_diff = hash.symbolize_keys SERIALIZE_KEYS.each do |key| - send(:"#{key}=", raw_diff[key.to_sym]) + send(:"#{key}=", raw_diff[key.to_sym]) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/lib/gitlab/gitaly_client/diff.rb b/lib/gitlab/gitaly_client/diff.rb index d459c9a88fb..54df6304865 100644 --- a/lib/gitlab/gitaly_client/diff.rb +++ b/lib/gitlab/gitaly_client/diff.rb @@ -7,13 +7,13 @@ module Gitlab def initialize(params) params.each do |key, val| - public_send(:"#{key}=", val) + public_send(:"#{key}=", val) # rubocop:disable GitlabSecurity/PublicSend end end def ==(other) FIELDS.all? do |field| - public_send(field) == other.public_send(field) + public_send(field) == other.public_send(field) # rubocop:disable GitlabSecurity/PublicSend end end end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index f5b757ace77..bc836dcc08d 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -45,7 +45,7 @@ module Gitlab end def all - REFERABLES.each { |referable| send(referable.to_s.pluralize) } + REFERABLES.each { |referable| send(referable.to_s.pluralize) } # rubocop:disable GitlabSecurity/PublicSend @references.values.flatten end diff --git a/lib/static_model.rb b/lib/static_model.rb index 185921d8fbe..60e2dd82e4e 100644 --- a/lib/static_model.rb +++ b/lib/static_model.rb @@ -18,7 +18,7 @@ module StaticModel # # Pass it along if we respond to it. def [](key) - send(key) if respond_to?(key) + send(key) if respond_to?(key) # rubocop:disable GitlabSecurity/PublicSend end def to_param |