Skip to content
Snippets Groups Projects
Commit d5f38aab authored by GitLab Bot's avatar GitLab Bot
Browse files

Add latest changes from gitlab-org/gitlab@master

parent ab0f1c7a
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
changelogs/unreleased/sh-fix-protected-paths.yml 0 → 100644
+ 5
− 0
View file @ d5f38aab
---
title: Only enable protected paths for POST requests
merge_request: 19184
author:
type: fixed
config/initializers/rack_attack_new.rb
+ 10
− 7
View file @ d5f38aab
@@ -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
+ 32
− 28
View file @ d5f38aab
@@ -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
+ 66
− 48
View file @ d5f38aab
@@ -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
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment