diff options
-rw-r--r-- | changelogs/unreleased/sh-fix-protected-paths.yml | 5 | ||||
-rw-r--r-- | config/initializers/rack_attack_new.rb | 17 | ||||
-rw-r--r-- | spec/requests/rack_attack_global_spec.rb | 60 | ||||
-rw-r--r-- | spec/support/shared_examples/requests/rack_attack_shared_examples.rb | 114 |
4 files changed, 113 insertions, 83 deletions
diff --git a/changelogs/unreleased/sh-fix-protected-paths.yml b/changelogs/unreleased/sh-fix-protected-paths.yml new file mode 100644 index 00000000000..1cc0e704506 --- /dev/null +++ b/changelogs/unreleased/sh-fix-protected-paths.yml @@ -0,0 +1,5 @@ +--- +title: Only enable protected paths for POST requests +merge_request: 19184 +author: +type: fixed diff --git a/config/initializers/rack_attack_new.rb b/config/initializers/rack_attack_new.rb index 5efff0579ba..92a8bf79432 100644 --- a/config/initializers/rack_attack_new.rb +++ b/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 diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index 0c148102e79..4d5055a7e27 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/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 diff --git a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb index 0897f643796..c078e982e87 100644 --- a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb +++ b/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 |