Merge branch 'sh-fix-protected-paths' into 'master'

Only enable protected paths for POST requests

See merge request gitlab-org/gitlab!19184

changelogs/unreleased/sh-fix-protected-paths.yml

+---
+title: Only enable protected paths for POST requests
+merge_request: 19184
+author:
+type: fixed

config/initializers/rack_attack_new.rb

@@ -73,7 +73,8 @@ class Rack::Attack
   end
 
   throttle('throttle_unauthenticated_protected_paths', Gitlab::Throttle.protected_paths_options) do |req|
-    if !req.should_be_skipped? &&
+    if req.post? &&
+        !req.should_be_skipped? &&
         req.protected_path? &&
         Gitlab::Throttle.protected_paths_enabled? &&
         req.unauthenticated?
@@ -82,17 +83,19 @@ class Rack::Attack
   end
 
   throttle('throttle_authenticated_protected_paths_api', Gitlab::Throttle.protected_paths_options) do |req|
-    if req.api_request? &&
-        Gitlab::Throttle.protected_paths_enabled? &&
-        req.protected_path?
+    if req.post? &&
+        req.api_request? &&
+        req.protected_path? &&
+        Gitlab::Throttle.protected_paths_enabled?
       req.authenticated_user_id([:api])
     end
   end
 
   throttle('throttle_authenticated_protected_paths_web', Gitlab::Throttle.protected_paths_options) do |req|
-    if req.web_request? &&
-        Gitlab::Throttle.protected_paths_enabled? &&
-        req.protected_path?
+    if req.post? &&
+        req.web_request? &&
+        req.protected_path? &&
+        Gitlab::Throttle.protected_paths_enabled?
       req.authenticated_user_id([:api, :rss, :ics])
     end
   end

spec/requests/rack_attack_global_spec.rb

@@ -22,6 +22,7 @@ describe 'Rack Attack global throttles' do
     }
   end
 
+  let(:request_method) { 'GET' }
   let(:requests_per_period) { 1 }
   let(:period_in_seconds) { 10000 }
   let(:period) { period_in_seconds.seconds }
@@ -143,15 +144,15 @@ describe 'Rack Attack global throttles' do
     let(:api_partial_url) { '/todos' }
 
     context 'with the token in the query string' do
-      let(:get_args) { [api(api_partial_url, personal_access_token: token)] }
-      let(:other_user_get_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
+      let(:request_args) { [api(api_partial_url, personal_access_token: token)] }
+      let(:other_user_request_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
 
     context 'with the token in the headers' do
-      let(:get_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) }
-      let(:other_user_get_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(other_user_token)) }
+      let(:request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) }
+      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(other_user_token)) }
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
@@ -170,15 +171,15 @@ describe 'Rack Attack global throttles' do
     let(:api_partial_url) { '/todos' }
 
     context 'with the token in the query string' do
-      let(:get_args) { [api(api_partial_url, oauth_access_token: token)] }
-      let(:other_user_get_args) { [api(api_partial_url, oauth_access_token: other_user_token)] }
+      let(:request_args) { [api(api_partial_url, oauth_access_token: token)] }
+      let(:other_user_request_args) { [api(api_partial_url, oauth_access_token: other_user_token)] }
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
 
     context 'with the token in the headers' do
-      let(:get_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) }
-      let(:other_user_get_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) }
+      let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) }
+      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) }
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
@@ -190,8 +191,8 @@ describe 'Rack Attack global throttles' do
     let(:throttle_setting_prefix) { 'throttle_authenticated_web' }
 
     context 'with the token in the query string' do
-      let(:get_args) { [rss_url(user), params: nil] }
-      let(:other_user_get_args) { [rss_url(other_user), params: nil] }
+      let(:request_args) { [rss_url(user), params: nil] }
+      let(:other_user_request_args) { [rss_url(other_user), params: nil] }
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
@@ -206,10 +207,13 @@ describe 'Rack Attack global throttles' do
   end
 
   describe 'protected paths' do
+    let(:request_method) { 'POST' }
+
     context 'unauthenticated requests' do
       let(:protected_path_that_does_not_require_authentication) do
-        '/users/confirmation'
+        '/users/sign_in'
       end
+      let(:post_params) { { user: { login: 'username', password: 'password' } } }
 
       before do
         settings_to_set[:throttle_protected_paths_requests_per_period] = requests_per_period # 1
@@ -224,7 +228,7 @@ describe 'Rack Attack global throttles' do
 
         it 'allows requests over the rate limit' do
           (1 + requests_per_period).times do
-            get protected_path_that_does_not_require_authentication
+            post protected_path_that_does_not_require_authentication, params: post_params
             expect(response).to have_http_status 200
           end
         end
@@ -238,11 +242,11 @@ describe 'Rack Attack global throttles' do
 
         it 'rejects requests over the rate limit' do
           requests_per_period.times do
-            get protected_path_that_does_not_require_authentication
+            post protected_path_that_does_not_require_authentication, params: post_params
             expect(response).to have_http_status 200
           end
 
-          expect_rejection { get protected_path_that_does_not_require_authentication }
+          expect_rejection { post protected_path_that_does_not_require_authentication, params: post_params }
         end
 
         context 'when Omnibus throttle is present' do
@@ -253,7 +257,7 @@ describe 'Rack Attack global throttles' do
 
           it 'allows requests over the rate limit' do
             (1 + requests_per_period).times do
-              get protected_path_that_does_not_require_authentication
+              post protected_path_that_does_not_require_authentication, params: post_params
               expect(response).to have_http_status 200
             end
           end
@@ -267,11 +271,11 @@ describe 'Rack Attack global throttles' do
       let(:other_user) { create(:user) }
       let(:other_user_token) { create(:personal_access_token, user: other_user) }
       let(:throttle_setting_prefix) { 'throttle_protected_paths' }
-      let(:api_partial_url) { '/users' }
+      let(:api_partial_url) { '/user/emails' }
 
       let(:protected_paths) do
         [
-          '/api/v4/users'
+          '/api/v4/user/emails'
         ]
       end
 
@@ -281,22 +285,22 @@ describe 'Rack Attack global throttles' do
       end
 
       context 'with the token in the query string' do
-        let(:get_args) { [api(api_partial_url, personal_access_token: token)] }
-        let(:other_user_get_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
+        let(:request_args) { [api(api_partial_url, personal_access_token: token)] }
+        let(:other_user_request_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
 
         it_behaves_like 'rate-limited token-authenticated requests'
       end
 
       context 'with the token in the headers' do
-        let(:get_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) }
-        let(:other_user_get_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(other_user_token)) }
+        let(:request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) }
+        let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(other_user_token)) }
 
         it_behaves_like 'rate-limited token-authenticated requests'
       end
 
       context 'when Omnibus throttle is present' do
-        let(:get_args) { [api(api_partial_url, personal_access_token: token)] }
-        let(:other_user_get_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
+        let(:request_args) { [api(api_partial_url, personal_access_token: token)] }
+        let(:other_user_request_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
 
         before do
           settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period
@@ -310,8 +314,8 @@ describe 'Rack Attack global throttles' do
 
         it 'allows requests over the rate limit' do
           (1 + requests_per_period).times do
-            get(*get_args)
-            expect(response).to have_http_status 200
+            post(*request_args)
+            expect(response).not_to have_http_status 429
           end
         end
       end
@@ -320,7 +324,7 @@ describe 'Rack Attack global throttles' do
     describe 'web requests authenticated with regular login' do
       let(:throttle_setting_prefix) { 'throttle_protected_paths' }
       let(:user) { create(:user) }
-      let(:url_that_requires_authentication) { '/dashboard/snippets' }
+      let(:url_that_requires_authentication) { '/users/confirmation' }
 
       let(:protected_paths) do
         [
@@ -350,8 +354,8 @@ describe 'Rack Attack global throttles' do
 
         it 'allows requests over the rate limit' do
           (1 + requests_per_period).times do
-            get url_that_requires_authentication
-            expect(response).to have_http_status 200
+            post url_that_requires_authentication
+            expect(response).not_to have_http_status 429
           end
         end
       end

spec/support/shared_examples/requests/rack_attack_shared_examples.rb

@@ -2,8 +2,9 @@
 #
 # Requires let variables:
 # * throttle_setting_prefix: "throttle_authenticated_api", "throttle_authenticated_web", "throttle_protected_paths"
-# * get_args
-# * other_user_get_args
+# * request_method
+# * request_args
+# * other_user_request_args
 # * requests_per_period
 # * period_in_seconds
 # * period
@@ -31,66 +32,66 @@ shared_examples_for 'rate-limited token-authenticated requests' do
     it 'rejects requests over the rate limit' do
       # At first, allow requests under the rate limit.
       requests_per_period.times do
-        get(*get_args)
-        expect(response).to have_http_status 200
+        make_request(request_args)
+        expect(response).not_to have_http_status 429
       end
 
       # the last straw
-      expect_rejection { get(*get_args) }
+      expect_rejection { make_request(request_args) }
     end
 
     it 'allows requests after throttling and then waiting for the next period' do
       requests_per_period.times do
-        get(*get_args)
-        expect(response).to have_http_status 200
+        make_request(request_args)
+        expect(response).not_to have_http_status 429
       end
 
-      expect_rejection { get(*get_args) }
+      expect_rejection { make_request(request_args) }
 
       Timecop.travel(period.from_now) do
         requests_per_period.times do
-          get(*get_args)
-          expect(response).to have_http_status 200
+          make_request(request_args)
+          expect(response).not_to have_http_status 429
         end
 
-        expect_rejection { get(*get_args) }
+        expect_rejection { make_request(request_args) }
       end
     end
 
     it 'counts requests from different users separately, even from the same IP' do
       requests_per_period.times do
-        get(*get_args)
-        expect(response).to have_http_status 200
+        make_request(request_args)
+        expect(response).not_to have_http_status 429
       end
 
       # would be over the limit if this wasn't a different user
-      get(*other_user_get_args)
-      expect(response).to have_http_status 200
+      make_request(other_user_request_args)
+      expect(response).not_to have_http_status 429
     end
 
     it 'counts all requests from the same user, even via different IPs' do
       requests_per_period.times do
-        get(*get_args)
-        expect(response).to have_http_status 200
+        make_request(request_args)
+        expect(response).not_to have_http_status 429
       end
 
       expect_any_instance_of(Rack::Attack::Request).to receive(:ip).at_least(:once).and_return('1.2.3.4')
 
-      expect_rejection { get(*get_args) }
+      expect_rejection { make_request(request_args) }
     end
 
     it 'logs RackAttack info into structured logs' do
       requests_per_period.times do
-        get(*get_args)
-        expect(response).to have_http_status 200
+        make_request(request_args)
+        expect(response).not_to have_http_status 429
       end
 
       arguments = {
         message: 'Rack_Attack',
         env: :throttle,
         remote_ip: '127.0.0.1',
-        request_method: 'GET',
-        path: get_args.first,
+        request_method: request_method,
+        path: request_args.first,
         user_id: user.id,
         username: user.username,
         throttle_type: throttle_types[throttle_setting_prefix]
@@ -98,7 +99,7 @@ shared_examples_for 'rate-limited token-authenticated requests' do
 
       expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once
 
-      expect_rejection { get(*get_args) }
+      expect_rejection { make_request(request_args) }
     end
   end
 
@@ -110,17 +111,26 @@ shared_examples_for 'rate-limited token-authenticated requests' do
 
     it 'allows requests over the rate limit' do
       (1 + requests_per_period).times do
-        get(*get_args)
-        expect(response).to have_http_status 200
+        make_request(request_args)
+        expect(response).not_to have_http_status 429
       end
     end
   end
+
+  def make_request(args)
+    if request_method == 'POST'
+      post(*args)
+    else
+      get(*args)
+    end
+  end
 end
 
 # Requires let variables:
 # * throttle_setting_prefix: "throttle_authenticated_web" or "throttle_protected_paths"
 # * user
 # * url_that_requires_authentication
+# * request_method
 # * requests_per_period
 # * period_in_seconds
 # * period
@@ -149,68 +159,68 @@ shared_examples_for 'rate-limited web authenticated requests' do
     it 'rejects requests over the rate limit' do
       # At first, allow requests under the rate limit.
       requests_per_period.times do
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
+        request_authenticated_web_url
+        expect(response).not_to have_http_status 429
       end
 
       # the last straw
-      expect_rejection { get url_that_requires_authentication }
+      expect_rejection { request_authenticated_web_url }
     end
 
     it 'allows requests after throttling and then waiting for the next period' do
       requests_per_period.times do
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
+        request_authenticated_web_url
+        expect(response).not_to have_http_status 429
       end
 
-      expect_rejection { get url_that_requires_authentication }
+      expect_rejection { request_authenticated_web_url }
 
       Timecop.travel(period.from_now) do
         requests_per_period.times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
+          request_authenticated_web_url
+          expect(response).not_to have_http_status 429
         end
 
-        expect_rejection { get url_that_requires_authentication }
+        expect_rejection { request_authenticated_web_url }
       end
     end
 
     it 'counts requests from different users separately, even from the same IP' do
       requests_per_period.times do
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
+        request_authenticated_web_url
+        expect(response).not_to have_http_status 429
       end
 
       # would be over the limit if this wasn't a different user
       login_as(create(:user))
 
-      get url_that_requires_authentication
-      expect(response).to have_http_status 200
+      request_authenticated_web_url
+      expect(response).not_to have_http_status 429
     end
 
     it 'counts all requests from the same user, even via different IPs' do
       requests_per_period.times do
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
+        request_authenticated_web_url
+        expect(response).not_to have_http_status 429
       end
 
       expect_any_instance_of(Rack::Attack::Request).to receive(:ip).at_least(:once).and_return('1.2.3.4')
 
-      expect_rejection { get url_that_requires_authentication }
+      expect_rejection { request_authenticated_web_url }
     end
 
     it 'logs RackAttack info into structured logs' do
       requests_per_period.times do
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
+        request_authenticated_web_url
+        expect(response).not_to have_http_status 429
       end
 
       arguments = {
         message: 'Rack_Attack',
         env: :throttle,
         remote_ip: '127.0.0.1',
-        request_method: 'GET',
-        path: '/dashboard/snippets',
+        request_method: request_method,
+        path: url_that_requires_authentication,
         user_id: user.id,
         username: user.username,
         throttle_type: throttle_types[throttle_setting_prefix]
@@ -218,7 +228,7 @@ shared_examples_for 'rate-limited web authenticated requests' do
 
       expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once
 
-      get url_that_requires_authentication
+      request_authenticated_web_url
     end
   end
 
@@ -230,9 +240,17 @@ shared_examples_for 'rate-limited web authenticated requests' do
 
     it 'allows requests over the rate limit' do
       (1 + requests_per_period).times do
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
+        request_authenticated_web_url
+        expect(response).not_to have_http_status 429
       end
     end
   end
+
+  def request_authenticated_web_url
+    if request_method == 'POST'
+      post url_that_requires_authentication
+    else
+      get url_that_requires_authentication
+    end
+  end
 end