summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--changelogs/unreleased/sh-fix-protected-paths.yml5
-rw-r--r--config/initializers/rack_attack_new.rb17
-rw-r--r--spec/requests/rack_attack_global_spec.rb60
-rw-r--r--spec/support/shared_examples/requests/rack_attack_shared_examples.rb114
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
View Lorry Controller Status