Merge branch '48717-rate-limit-raw-controller-show' into 'master'

Add RateLimiter to RawController

See merge request gitlab-org/gitlab-ce!30635

app/controllers/admin/application_settings_controller.rb

@@ -106,6 +106,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :lets_encrypt_notification_email,
       :lets_encrypt_terms_of_service_accepted,
       :domain_blacklist_file,
+      :raw_blob_request_limit,
       disabled_oauth_sign_in_sources: [],
       import_sources: [],
       repository_storages: [],

app/controllers/projects/raw_controller.rb

@@ -8,10 +8,30 @@ class Projects::RawController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :assign_ref_vars
   before_action :authorize_download_code!
+  before_action :show_rate_limit, only: [:show]
 
   def show
     @blob = @repository.blob_at(@commit.id, @path)
 
     send_blob(@repository, @blob, inline: (params[:inline] != 'false'))
   end
+
+  private
+
+  def show_rate_limit
+    limiter = ::Gitlab::ActionRateLimiter.new(action: :show_raw_controller)
+
+    return unless limiter.throttled?([@project, @commit, @path], raw_blob_request_limit)
+
+    limiter.log_request(request, :raw_blob_request_limit, current_user)
+
+    flash[:alert] = _('You cannot access the raw file. Please wait a minute.')
+    redirect_to project_blob_path(@project, File.join(@ref, @path))
+  end
+
+  def raw_blob_request_limit
+    Gitlab::CurrentSettings
+      .current_application_settings
+      .raw_blob_request_limit
+  end
 end

app/models/application_setting_implementation.rb

@@ -98,7 +98,8 @@ module ApplicationSettingImplementation
         commit_email_hostname: default_commit_email_hostname,
         protected_ci_variables: false,
         local_markdown_version: 0,
-        outbound_local_requests_whitelist: []
+        outbound_local_requests_whitelist: [],
+        raw_blob_request_limit: 300
       }
     end
 

app/views/admin/application_settings/_performance.html.haml

@@ -15,4 +15,10 @@
           AuthorizedKeysCommand. Click on the help icon for more details.
           = link_to icon('question-circle'), help_page_path('administration/operations/fast_ssh_key_lookup')
 
+    .form-group
+      = f.label :raw_blob_request_limit, _('Raw blob request rate limit per minute'), class: 'label-bold'
+      = f.number_field :raw_blob_request_limit, class: 'form-control'
+      .form-text.text-muted
+        = _('Highest number of requests per minute for each raw path, default to 300. To disable throttling set to 0.')
+
   = f.submit 'Save changes', class: "btn btn-success"

changelogs/unreleased/48717-rate-limit-raw-controller-show.yml

+---
+title: Add Rate Request Limiter to RawController#show endpoint
+merge_request: 30635
+author:
+type: added

db/migrate/20190715142138_add_raw_blob_request_limit_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddRawBlobRequestLimitToApplicationSettings < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :raw_blob_request_limit, :integer, default: 300, null: false
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_07_15_114644) do
+ActiveRecord::Schema.define(version: 2019_07_15_142138) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -229,6 +229,7 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do
     t.boolean "time_tracking_limit_to_hours", default: false, null: false
     t.string "grafana_url", default: "/-/grafana", null: false
     t.string "outbound_local_requests_whitelist", limit: 255, array: true
+    t.integer "raw_blob_request_limit", default: 300, null: false
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id"

lib/gitlab/action_rate_limiter.rb

@@ -33,16 +33,48 @@ module Gitlab
     # Increments the given key and returns true if the action should
     # be throttled.
     #
-    # key - An array of ActiveRecord instances
-    # threshold_value - The maximum number of times this action should occur in the given time interval
+    # key - An array of ActiveRecord instances or strings
+    # threshold_value - The maximum number of times this action should occur in the given time interval. If number is zero is considered disabled.
     def throttled?(key, threshold_value)
+      threshold_value > 0 &&
         self.increment(key) > threshold_value
     end
 
+    # Logs request into auth.log
+    #
+    # request - Web request to be logged
+    # type - A symbol key that represents the request.
+    # current_user - Current user of the request, it can be nil.
+    def log_request(request, type, current_user)
+      request_information = {
+        message: 'Action_Rate_Limiter_Request',
+        env: type,
+        ip: request.ip,
+        request_method: request.request_method,
+        fullpath: request.fullpath
+      }
+
+      if current_user
+        request_information.merge!({
+          user_id: current_user.id,
+          username: current_user.username
+        })
+      end
+
+      Gitlab::AuthLogger.error(request_information)
+    end
+
     private
 
     def action_key(key)
-      serialized = key.map { |obj| "#{obj.class.model_name.to_s.underscore}:#{obj.id}" }.join(":")
+      serialized = key.map do |obj|
+        if obj.is_a?(String)
+          "#{obj}"
+        else
+          "#{obj.class.model_name.to_s.underscore}:#{obj.id}"
+        end
+      end.join(":")
+
       "action_rate_limiter:#{action}:#{serialized}"
     end
   end

locale/gitlab.pot

@@ -5298,6 +5298,9 @@ msgstr[1] ""
 msgid "Hide values"
 msgstr ""
 
+msgid "Highest number of requests per minute for each raw path, default to 300. To disable throttling set to 0."
+msgstr ""
+
 msgid "Highest role:"
 msgstr ""
 
@@ -8717,6 +8720,9 @@ msgstr ""
 msgid "Rake Tasks Help"
 msgstr ""
 
+msgid "Raw blob request rate limit per minute"
+msgstr ""
+
 msgid "Read more"
 msgstr ""
 
@@ -12442,6 +12448,9 @@ msgstr ""
 msgid "You can test your .gitlab-ci.yml in %{linkStart}CI Lint%{linkEnd}."
 msgstr ""
 
+msgid "You cannot access the raw file. Please wait a minute."
+msgstr ""
+
 msgid "You cannot impersonate a blocked user"
 msgstr ""
 

spec/controllers/projects/raw_controller_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Projects::RawController do
+  include RepoHelpers
+
   let(:project) { create(:project, :public, :repository) }
 
   describe 'GET #show' do
@@ -46,5 +48,98 @@ describe Projects::RawController do
       let(:filename) { 'lfs_object.iso' }
       let(:filepath) { "be93687/files/lfs/#{filename}" }
     end
+
+    context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+      let(:file_path) { 'master/README.md' }
+
+      before do
+        stub_application_setting(raw_blob_request_limit: 5)
+      end
+
+      it 'prevents from accessing the raw file' do
+        execute_raw_requests(requests: 6, project: project, file_path: file_path)
+
+        expect(flash[:alert]).to eq('You cannot access the raw file. Please wait a minute.')
+        expect(response).to redirect_to(project_blob_path(project, file_path))
+      end
+
+      it 'logs the event on auth.log' do
+        attributes = {
+          message: 'Action_Rate_Limiter_Request',
+          env: :raw_blob_request_limit,
+          ip: '0.0.0.0',
+          request_method: 'GET',
+          fullpath: "/#{project.full_path}/raw/#{file_path}"
+        }
+
+        expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+
+        execute_raw_requests(requests: 6, project: project, file_path: file_path)
+      end
+
+      context 'when the request uses a different version of a commit' do
+        it 'prevents from accessing the raw file' do
+          # 3 times with the normal sha
+          commit_sha = project.repository.commit.sha
+          file_path = "#{commit_sha}/README.md"
+
+          execute_raw_requests(requests: 3, project: project, file_path: file_path)
+
+          # 3 times with the modified version
+          modified_sha = commit_sha.gsub(commit_sha[0..5], commit_sha[0..5].upcase)
+          modified_path = "#{modified_sha}/README.md"
+
+          execute_raw_requests(requests: 3, project: project, file_path: modified_path)
+
+          expect(flash[:alert]).to eq('You cannot access the raw file. Please wait a minute.')
+          expect(response).to redirect_to(project_blob_path(project, modified_path))
+        end
+      end
+
+      context 'when the throttling has been disabled' do
+        before do
+          stub_application_setting(raw_blob_request_limit: 0)
+        end
+
+        it 'does not prevent from accessing the raw file' do
+          execute_raw_requests(requests: 10, project: project, file_path: file_path)
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+
+      context 'with case-sensitive files' do
+        it 'prevents from accessing the specific file' do
+          create_file_in_repo(project, 'master', 'master', 'readme.md', 'Add readme.md')
+          create_file_in_repo(project, 'master', 'master', 'README.md', 'Add README.md')
+
+          commit_sha = project.repository.commit.sha
+          file_path = "#{commit_sha}/readme.md"
+
+          # Accessing downcase version of readme
+          execute_raw_requests(requests: 6, project: project, file_path: file_path)
+
+          expect(flash[:alert]).to eq('You cannot access the raw file. Please wait a minute.')
+          expect(response).to redirect_to(project_blob_path(project, file_path))
+
+          # Accessing upcase version of readme
+          file_path = "#{commit_sha}/README.md"
+
+          execute_raw_requests(requests: 1, project: project, file_path: file_path)
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+    end
+  end
+
+  def execute_raw_requests(requests:, project:, file_path:)
+    requests.times do
+      get :show, params: {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: file_path
+      }
+    end
   end
 end

spec/lib/gitlab/action_rate_limiter_spec.rb

 require 'spec_helper'
 
-describe Gitlab::ActionRateLimiter do
+describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
   let(:redis) { double('redis') }
   let(:user) { create(:user) }
   let(:project) { create(:project) }
-  let(:key) { [user, project] }
-  let(:cache_key) { "action_rate_limiter:test_action:user:#{user.id}:project:#{project.id}" }
 
   subject { described_class.new(action: :test_action, expiry_time: 100) }
 
@@ -13,17 +11,98 @@ describe Gitlab::ActionRateLimiter do
     allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis)
   end
 
-  it 'increases the throttle count and sets the expire time' do
+  shared_examples 'action rate limiter' do
+    it 'increases the throttle count and sets the expiration time' do
       expect(redis).to receive(:incr).with(cache_key).and_return(1)
       expect(redis).to receive(:expire).with(cache_key, 100)
 
-    expect(subject.throttled?(key, 1)).to be false
+      expect(subject.throttled?(key, 1)).to be_falsy
     end
 
     it 'returns true if the key is throttled' do
       expect(redis).to receive(:incr).with(cache_key).and_return(2)
       expect(redis).not_to receive(:expire)
 
-    expect(subject.throttled?(key, 1)).to be true
+      expect(subject.throttled?(key, 1)).to be_truthy
+    end
+
+    context 'when throttling is disabled' do
+      it 'returns false and does not set expiration time' do
+        expect(redis).not_to receive(:incr)
+        expect(redis).not_to receive(:expire)
+
+        expect(subject.throttled?(key, 0)).to be_falsy
+      end
+    end
+  end
+
+  context 'when the key is an array of only ActiveRecord models' do
+    let(:key) { [user, project] }
+
+    let(:cache_key) do
+      "action_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
+    end
+
+    it_behaves_like 'action rate limiter'
+  end
+
+  context 'when they key a combination of ActiveRecord models and strings' do
+    let(:project) { create(:project, :public, :repository) }
+    let(:commit) { project.repository.commit }
+    let(:path) { 'app/controllers/groups_controller.rb' }
+    let(:key) { [project, commit, path] }
+
+    let(:cache_key) do
+      "action_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
+    end
+
+    it_behaves_like 'action rate limiter'
+  end
+
+  describe '#log_request' do
+    let(:file_path) { 'master/README.md' }
+    let(:type) { :raw_blob_request_limit }
+    let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
+
+    let(:request) do
+      double('request', ip: '127.0.0.1', request_method: 'GET', fullpath: fullpath)
+    end
+
+    let(:base_attributes) do
+      {
+        message: 'Action_Rate_Limiter_Request',
+        env: type,
+        ip: '127.0.0.1',
+        request_method: 'GET',
+        fullpath: fullpath
+      }
+    end
+
+    context 'without a current user' do
+      let(:current_user) { nil }
+
+      it 'logs information to auth.log' do
+        expect(Gitlab::AuthLogger).to receive(:error).with(base_attributes).once
+
+        subject.log_request(request, type, current_user)
+      end
+    end
+
+    context 'with a current_user' do
+      let(:current_user) { create(:user) }
+
+      let(:attributes) do
+        base_attributes.merge({
+          user_id: current_user.id,
+          username: current_user.username
+        })
+      end
+
+      it 'logs information to auth.log' do
+        expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+
+        subject.log_request(request, type, current_user)
+      end
+    end
   end
 end

spec/services/application_settings/update_service_spec.rb

@@ -180,4 +180,20 @@ describe ApplicationSettings::UpdateService do
       described_class.new(application_settings, admin, { home_page_url: 'http://foo.bar' }).execute
     end
   end
+
+  context 'when raw_blob_request_limit is passsed' do
+    let(:params) do
+      {
+        raw_blob_request_limit: 600
+      }
+    end
+
+    it 'updates raw_blob_request_limit value' do
+      subject.execute
+
+      application_settings.reload
+
+      expect(application_settings.raw_blob_request_limit).to eq(600)
+    end
+  end
 end