Remove OAuth paths from protected paths rate limit

These paths were added in the 13.3.3 security release [1] so they would
get throttled by the "protected paths" rate limit, as a protection
against brute-force attacks.

This rate limit turned out to be to strict for normal OAuth usage [2],
so we're removing the paths again and instead let them get throttled by
the general rate limit for unauthenticated requests, which didn't exist
yet when we made the original change, and should still offer sufficient
protection.

This configuration change was already applied manually on gitlab.com
and verified to be working as expected. [3]

[1] https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/757
[2] https://gitlab.com/gitlab-org/gitlab/-/issues/345554
[3] https://gitlab.com/gitlab-com/gl-infra/production/-/issues/6000

Changelog: changed

db/migrate/20211215182006_update_application_settings_protected_paths.rb

+# frozen_string_literal: true
+
+class UpdateApplicationSettingsProtectedPaths < Gitlab::Database::Migration[1.0]
+  REMOVE_PROTECTED_PATHS = [
+    '/oauth/authorize',
+    '/oauth/token'
+  ].freeze
+
+  NEW_DEFAULT_PROTECTED_PATHS = [
+    '/users/password',
+    '/users/sign_in',
+    '/api/v3/session.json',
+    '/api/v3/session',
+    '/api/v4/session.json',
+    '/api/v4/session',
+    '/users',
+    '/users/confirmation',
+    '/unsubscribes/',
+    '/import/github/personal_access_token',
+    '/admin/session'
+  ].freeze
+
+  OLD_DEFAULT_PROTECTED_PATHS = (NEW_DEFAULT_PROTECTED_PATHS + REMOVE_PROTECTED_PATHS).freeze
+
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+  end
+
+  def up
+    change_column_default(:application_settings, :protected_paths, NEW_DEFAULT_PROTECTED_PATHS)
+
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.where.not(protected_paths: nil).each do |application_setting|
+      paths_to_remove = application_setting.protected_paths & REMOVE_PROTECTED_PATHS
+
+      next if paths_to_remove.empty?
+
+      updated_protected_paths = application_setting.protected_paths - paths_to_remove
+      application_setting.update!(protected_paths: updated_protected_paths)
+    end
+  end
+
+  def down
+    change_column_default(:application_settings, :protected_paths, OLD_DEFAULT_PROTECTED_PATHS)
+
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.where.not(protected_paths: nil).each do |application_setting|
+      paths_to_add = REMOVE_PROTECTED_PATHS - application_setting.protected_paths
+
+      next if paths_to_add.empty?
+
+      updated_protected_paths = application_setting.protected_paths + paths_to_add
+      application_setting.update!(protected_paths: updated_protected_paths)
+    end
+  end
+end

db/schema_migrations/20211215182006

+ead2a1b13438514bb97bea3f1656f9bac352a8c733d9f808b2405685bce91e00
\ No newline at end of file

db/structure.sql

@@ -10298,7 +10298,7 @@ CREATE TABLE application_settings (
     throttle_protected_paths_enabled boolean DEFAULT false NOT NULL,
     throttle_protected_paths_requests_per_period integer DEFAULT 10 NOT NULL,
     throttle_protected_paths_period_in_seconds integer DEFAULT 60 NOT NULL,
-    protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session,/oauth/authorize,/oauth/token}'::character varying[],
+    protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session}'::character varying[],
     throttle_incident_management_notification_enabled boolean DEFAULT false NOT NULL,
     throttle_incident_management_notification_period_in_seconds integer DEFAULT 3600,
     throttle_incident_management_notification_per_period integer DEFAULT 3600,

spec/migrations/update_application_settings_protected_paths_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe UpdateApplicationSettingsProtectedPaths, :aggregate_failures do
+  subject(:migration) { described_class.new }
+
+  let_it_be(:application_settings) { table(:application_settings) }
+  let_it_be(:oauth_paths) { %w[/oauth/authorize /oauth/token] }
+  let_it_be(:custom_paths) { %w[/foo /bar] }
+
+  let(:default_paths) { application_settings.column_defaults.fetch('protected_paths') }
+
+  before do
+    application_settings.create!(protected_paths: custom_paths)
+    application_settings.create!(protected_paths: custom_paths + oauth_paths)
+    application_settings.create!(protected_paths: custom_paths + oauth_paths.take(1))
+  end
+
+  describe '#up' do
+    before do
+      migrate!
+      application_settings.reset_column_information
+    end
+
+    it 'removes the OAuth paths from the default value and persisted records' do
+      expect(default_paths).not_to include(*oauth_paths)
+      expect(default_paths).to eq(described_class::NEW_DEFAULT_PROTECTED_PATHS)
+      expect(application_settings.all).to all(have_attributes(protected_paths: custom_paths))
+    end
+  end
+
+  describe '#down' do
+    before do
+      migrate!
+      schema_migrate_down!
+    end
+
+    it 'adds the OAuth paths to the default value and persisted records' do
+      expect(default_paths).to include(*oauth_paths)
+      expect(default_paths).to eq(described_class::OLD_DEFAULT_PROTECTED_PATHS)
+      expect(application_settings.all).to all(have_attributes(protected_paths: custom_paths + oauth_paths))
+    end
+  end
+end