Merge branch...

Merge branch '320792-add-an-api-rate-limit-for-new-notes-using-application-rate-limits' into 'master'

Add application rate limit for Notes creation

See merge request gitlab-org/gitlab!53637

app/controllers/admin/application_settings_controller.rb

@@ -243,6 +243,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :domain_denylist_file,
       :raw_blob_request_limit,
       :issues_create_limit,
+      :notes_create_limit,
       :default_branch_name,
       disabled_oauth_sign_in_sources: [],
       import_sources: [],

app/controllers/projects/notes_controller.rb

@@ -10,6 +10,7 @@ class Projects::NotesController < Projects::ApplicationController
   before_action :authorize_read_note!
   before_action :authorize_create_note!, only: [:create]
   before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
+  before_action :create_rate_limit, only: [:create]
 
   feature_category :issue_tracking
 
@@ -90,4 +91,17 @@ class Projects::NotesController < Projects::ApplicationController
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42383')
   end
+
+  def create_rate_limit
+    key = :notes_create
+
+    return unless rate_limiter.throttled?(key, scope: [current_user])
+
+    rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
+    render plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests
+  end
+
+  def rate_limiter
+    ::Gitlab::ApplicationRateLimiter
+  end
 end

app/graphql/mutations/notes/create/base.rb

@@ -25,6 +25,7 @@ module Mutations
 
         def resolve(args)
           noteable = authorized_find!(id: args[:noteable_id])
+          verify_rate_limit!(current_user)
 
           note = ::Notes::CreateService.new(
             noteable.project,
@@ -54,6 +55,14 @@ module Mutations
             confidential: args[:confidential]
           }
         end
+
+        def verify_rate_limit!(current_user)
+          rate_limiter, key = ::Gitlab::ApplicationRateLimiter, :notes_create
+          return unless rate_limiter.throttled?(key, scope: [current_user])
+
+          raise Gitlab::Graphql::Errors::ResourceNotAvailable,
+            'This endpoint has been requested too many times. Try again later.'
+        end
       end
     end
   end

app/helpers/application_settings_helper.rb

@@ -328,6 +328,7 @@ module ApplicationSettingsHelper
       :email_restrictions_enabled,
       :email_restrictions,
       :issues_create_limit,
+      :notes_create_limit,
       :raw_blob_request_limit,
       :project_import_limit,
       :project_export_limit,

app/models/application_setting.rb

@@ -444,6 +444,9 @@ class ApplicationSetting < ApplicationRecord
             presence: true,
             numericality: { only_integer: true, greater_than: 0 }
 
+  validates :notes_create_limit,
+            numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+
   attr_encrypted :asset_proxy_secret_key,
                  mode: :per_attribute_iv,
                  key: Settings.attr_encrypted_db_key_base_truncated,

app/models/application_setting_implementation.rb

@@ -93,6 +93,7 @@ module ApplicationSettingImplementation
         import_sources: Settings.gitlab['import_sources'],
         invisible_captcha_enabled: false,
         issues_create_limit: 300,
+        notes_create_limit: 300,
         local_markdown_version: 0,
         login_recaptcha_protection_enabled: false,
         max_artifacts_size: Settings.artifacts['max_size'],

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

+= form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-note-limits-settings'), html: { class: 'fieldset-form' } do |f|
+  = form_errors(@application_setting)
+
+  %fieldset
+    .form-group
+      = f.label :notes_create_limit, _('Max requests per minute per user'), class: 'label-bold'
+      = f.number_field :notes_create_limit, class: 'form-control gl-form-input'
+
+  = f.submit _('Save changes'), class: "gl-button btn btn-success", data: { qa_selector: 'save_changes_button' }

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

@@ -61,6 +61,17 @@
   .settings-content
     = render 'issue_limits'
 
+%section.settings.as-note-limits.no-animate#js-note-limits-settings{ class: ('expanded' if expanded_by_default?) }
+  .settings-header
+    %h4
+      = _('Notes Rate Limits')
+    %button.btn.gl-button.btn-default.js-settings-toggle{ type: 'button' }
+      = expanded_by_default? ? _('Collapse') : _('Expand')
+    %p
+      = _('Configure limit for notes created per minute by web and API requests.')
+  .settings-content
+    = render 'note_limits'
+
 %section.settings.as-import-export-limits.no-animate#js-import-export-limits-settings{ class: ('expanded' if expanded_by_default?) }
   .settings-header
     %h4

changelogs/unreleased/320792-add-an-api-rate-limit-for-new-notes-using-application-rate-limits.yml

+---
+title: Add application rate limit for Notes creation
+merge_request: 53637
+author:
+type: added

db/migrate/20210208161207_add_notes_create_limit_to_application_settings.rb

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

db/schema_migrations/20210208161207

+818fcf0f0fec9d2833b091ef380005a2d485486522fb63e2a7b2fd01dbf1ff79
\ No newline at end of file

db/structure.sql

@@ -9413,6 +9413,7 @@ CREATE TABLE application_settings (
     git_two_factor_session_expiry integer DEFAULT 15 NOT NULL,
     asset_proxy_allowlist text,
     keep_latest_artifact boolean DEFAULT true NOT NULL,
+    notes_create_limit integer DEFAULT 300 NOT NULL,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),
     CONSTRAINT check_17d9558205 CHECK ((char_length((kroki_url)::text) <= 1024)),

doc/api/notes.md

@@ -36,6 +36,11 @@ are paginated.
 
 Read more on [pagination](README.md#pagination).
 
+## Rate limits
+
+To help avoid abuse, you can limit your users to a specific number of `Create` request per minute.
+See [Notes rate limits](../user/admin_area/settings/rate_limit_on_notes_creation.md).
+
 ## Issues
 
 ### List project issue notes

doc/security/rate_limits.md

@@ -25,11 +25,14 @@ similarly mitigated by a rate limit.
 
 ## Admin Area settings
 
-- [Issues rate limits](../user/admin_area/settings/rate_limit_on_issues_creation.md).
-- [User and IP rate limits](../user/admin_area/settings/user_and_ip_rate_limits.md).
-- [Raw endpoints rate limits](../user/admin_area/settings/rate_limits_on_raw_endpoints.md).
-- [Protected paths](../user/admin_area/settings/protected_paths.md).
-- [Import/Export rate limits](../user/admin_area/settings/import_export_rate_limits.md).
+These are rate limits you can set in the Admin Area of your instance:
+
+- [Import/Export rate limits](../user/admin_area/settings/import_export_rate_limits.md)
+- [Issues rate limits](../user/admin_area/settings/rate_limit_on_issues_creation.md)
+- [Notes rate limits](../user/admin_area/settings/rate_limit_on_notes_creation.md)
+- [Protected paths](../user/admin_area/settings/protected_paths.md)
+- [Raw endpoints rate limits](../user/admin_area/settings/rate_limits_on_raw_endpoints.md)
+- [User and IP rate limits](../user/admin_area/settings/user_and_ip_rate_limits.md)
 
 ## Non-configurable limits
 

doc/user/admin_area/settings/index.md

@@ -94,6 +94,7 @@ Access the default page for admin area settings by navigating to **Admin Area >
 | [Outbound requests](../../../security/webhooks.md) | Allow requests to the local network from hooks and services. |
 | [Protected Paths](protected_paths.md) | Configure paths to be protected by Rack Attack. |
 | [Incident Management](../../../operations/incident_management/index.md) Limits | Configure limits on the number of inbound alerts able to be sent to a project. |
+| [Notes creation limit](rate_limit_on_notes_creation.md)| Set a rate limit on the note creation requests. |
 
 ## Geo
 

doc/user/admin_area/settings/rate_limit_on_notes_creation.md

+---
+type: reference
+stage: Plan
+group: Project Management
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments
+---
+
+# Rate limits on note creation
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53637) in GitLab 13.9.
+
+This setting allows you to rate limit the requests to the note creation endpoint.
+
+To change the note creation rate limit:
+
+1. Go to **Admin Area > Settings > Network**.
+1. Expand the **Notes Rate Limits** section.
+1. Enter the new value.
+1. Select **Save changes**.
+
+This limit is:
+
+- Applied independently per user.
+- Not applied per IP address.
+
+The default value is `300`.
+
+Requests over the rate limit are logged into the `auth.log` file.
+
+For example, if you set a limit of 300, requests using the
+[Projects::NotesController#create](https://gitlab.com/gitlab-org/gitlab/blob/master/app/controllers/projects/notes_controller.rb)
+action exceeding a rate of 300 per minute are blocked. Access to the endpoint is allowed after one minute.

lib/api/issues.rb

@@ -231,7 +231,7 @@ module API
       post ':id/issues' do
         Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42320')
 
-        check_rate_limit! :issues_create, [current_user, :issues_create]
+        check_rate_limit! :issues_create, [current_user]
 
         authorize! :create_issue, user_project
 

lib/api/notes.rb

@@ -4,6 +4,7 @@ module API
   class Notes < ::API::Base
     include PaginationParams
     helpers ::API::Helpers::NotesHelpers
+    helpers Helpers::RateLimiter
 
     before { authenticate! }
 
@@ -72,6 +73,7 @@ module API
           optional :created_at, type: String, desc: 'The creation date of the note'
         end
         post ":id/#{noteables_str}/:noteable_id/notes", feature_category: feature_category do
+          check_rate_limit! :notes_create, [current_user]
           noteable = find_noteable(noteable_type, params[:noteable_id])
 
           opts = {

lib/gitlab/application_rate_limiter.rb

@@ -20,6 +20,7 @@ module Gitlab
       def rate_limits
         {
           issues_create:                { threshold: -> { application_settings.issues_create_limit }, interval: 1.minute },
+          notes_create:                 { threshold: -> { application_settings.notes_create_limit }, interval: 1.minute },
           project_export:               { threshold: -> { application_settings.project_export_limit }, interval: 1.minute },
           project_download_export:      { threshold: -> { application_settings.project_download_export_limit }, interval: 1.minute },
           project_repositories_archive: { threshold: 5, interval: 1.minute },

locale/gitlab.pot

@@ -7504,6 +7504,9 @@ msgstr ""
 msgid "Configure limit for issues created per minute by web and API requests."
 msgstr ""
 
+msgid "Configure limit for notes created per minute by web and API requests."
+msgstr ""
+
 msgid "Configure limits for Project/Group Import/Export."
 msgstr ""
 
@@ -17964,6 +17967,9 @@ msgstr ""
 msgid "Max file size is 200 KB."
 msgstr ""
 
+msgid "Max requests per minute per user"
+msgstr ""
+
 msgid "Max role"
 msgstr ""
 
@@ -20143,6 +20149,9 @@ msgstr ""
 msgid "NoteForm|Note"
 msgstr ""
 
+msgid "Notes Rate Limits"
+msgstr ""
+
 msgid "Notes|Are you sure you want to cancel creating this comment?"
 msgstr ""
 

spec/controllers/projects/notes_controller_spec.rb

@@ -727,6 +727,42 @@ RSpec.describe Projects::NotesController do
         end
       end
     end
+
+    context 'when the endpoint receives requests above the limit' do
+      before do
+        stub_application_setting(notes_create_limit: 5)
+      end
+
+      it 'prevents from creating more notes', :request_store do
+        5.times { create! }
+
+        expect { create! }
+          .to change { Gitlab::GitalyClient.get_request_count }.by(0)
+
+        create!
+        expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
+        expect(response).to have_gitlab_http_status(:too_many_requests)
+      end
+
+      it 'logs the event in auth.log' do
+        attributes = {
+          message: 'Application_Rate_Limiter_Request',
+          env: :notes_create_request_limit,
+          remote_ip: '0.0.0.0',
+          request_method: 'POST',
+          path: "/#{project.full_path}/notes",
+          user_id: user.id,
+          username: user.username
+        }
+
+        expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+
+        project.add_developer(user)
+        sign_in(user)
+
+        6.times { create! }
+      end
+    end
   end
 
   describe 'PUT update' do

spec/models/application_setting_spec.rb

@@ -114,6 +114,12 @@ RSpec.describe ApplicationSetting do
     it { is_expected.to allow_value(nil).for(:repository_storages_weighted_default) }
     it { is_expected.not_to allow_value({ default: 100, shouldntexist: 50 }).for(:repository_storages_weighted) }
 
+    it { is_expected.to allow_value(400).for(:notes_create_limit) }
+    it { is_expected.not_to allow_value('two').for(:notes_create_limit) }
+    it { is_expected.not_to allow_value(nil).for(:notes_create_limit) }
+    it { is_expected.not_to allow_value(5.5).for(:notes_create_limit) }
+    it { is_expected.not_to allow_value(-2).for(:notes_create_limit) }
+
     context 'help_page_documentation_base_url validations' do
       it { is_expected.to allow_value(nil).for(:help_page_documentation_base_url) }
       it { is_expected.to allow_value('https://docs.gitlab.com').for(:help_page_documentation_base_url) }

spec/requests/api/graphql/mutations/notes/create/diff_note_spec.rb

@@ -43,6 +43,8 @@ RSpec.describe 'Adding a DiffNote' do
 
     it_behaves_like 'a Note mutation when there are active record validation errors', model: DiffNote
 
+    it_behaves_like 'a Note mutation when there are rate limit validation errors'
+
     context do
       let(:diff_refs) { build(:commit).diff_refs } # Allow fake diff refs so arguments are valid
 

spec/requests/api/graphql/mutations/notes/create/image_diff_note_spec.rb

@@ -46,6 +46,8 @@ RSpec.describe 'Adding an image DiffNote' do
 
     it_behaves_like 'a Note mutation when there are active record validation errors', model: DiffNote
 
+    it_behaves_like 'a Note mutation when there are rate limit validation errors'
+
     context do
       let(:diff_refs) { build(:commit).diff_refs } # Allow fake diff refs so arguments are valid
 

spec/requests/api/graphql/mutations/notes/create/note_spec.rb

@@ -37,6 +37,8 @@ RSpec.describe 'Adding a Note' do
 
     it_behaves_like 'a Note mutation when the given resource id is not for a Noteable'
 
+    it_behaves_like 'a Note mutation when there are rate limit validation errors'
+
     it 'returns the note' do
       post_graphql_mutation(mutation, current_user: current_user)
 

spec/support/shared_examples/graphql/notes_creation_shared_examples.rb

@@ -64,3 +64,14 @@ RSpec.shared_examples 'a Note mutation when the given resource id is not for a N
     let(:match_errors) { include(/does not represent an instance of Note/) }
   end
 end
+
+RSpec.shared_examples 'a Note mutation when there are rate limit validation errors' do
+  before do
+    stub_application_setting(notes_create_limit: 3)
+    3.times { post_graphql_mutation(mutation, current_user: current_user) }
+  end
+
+  it_behaves_like 'a Note mutation that does not create a Note'
+  it_behaves_like 'a mutation that returns top-level errors',
+                  errors: ['This endpoint has been requested too many times. Try again later.']
+end

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -274,6 +274,19 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
+
+    context 'when request exceeds the rate limit' do
+      before do
+        allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true)
+      end
+
+      it 'prevents users from creating more notes' do
+        post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!' }
+
+        expect(response).to have_gitlab_http_status(:too_many_requests)
+        expect(json_response['message']['error']).to eq('This endpoint has been requested too many times. Try again later.')
+      end
+    end
   end
 
   describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/notes/:note_id" do