-
Notifications
You must be signed in to change notification settings - Fork 72
Optional API rate limiter introduced #511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -879,6 +879,7 @@ hu: | |
| errors: | ||
| api: | ||
| internal_server: Szerver hiba | ||
| too_many_requests: Túl sok kérés. Kérjük, próbálja újra később. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For future reference, we maintain non-English translations for application strings in Transifex. I'll add this particular one manually, but you can review the Hungarian ones here: https://app.transifex.com/primero-v2/primero-app-v2/language/hu/
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for helping out on this one |
||
| attachments: | ||
| maximum: Elérte a rekordhoz tartozó maximum mellékletek számát. | ||
| error_loading: Hiba a rekord(ok) betöltésekor | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,3 +178,121 @@ def decode_jwt(token) | |
| Warden::JWTAuth::TokenDecoder.new.call(token) | ||
| end | ||
| end | ||
|
|
||
| describe 'Rack::Attack API rate limit' do | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you move this new code to a separate new file
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved it to a new file as advised. |
||
| let(:app) { ->(_env) { [200, {}, ['OK']] } } | ||
| let(:client) { Rack::MockRequest.new(Rack::Attack.new(app)) } | ||
| let(:environment_variables) do | ||
| %w[ | ||
| PRIMERO_API_RATE_LIMIT_ENABLED | ||
| PRIMERO_API_RATE_LIMIT_REQUESTS | ||
| PRIMERO_API_RATE_LIMIT_PERIOD | ||
| ] | ||
| end | ||
|
|
||
| around do |example| | ||
| original_values = ENV.to_h.slice(*environment_variables) | ||
| environment_variables.each { |name| ENV.delete(name) } | ||
| Rack::Attack.reset! | ||
|
|
||
| example.run | ||
| ensure | ||
| environment_variables.each { |name| ENV.delete(name) } | ||
| original_values.each { |name, value| ENV[name] = value } | ||
| Rack::Attack.reset! | ||
| end | ||
|
|
||
| it 'is disabled when the enable environment variable is missing' do | ||
| ENV['PRIMERO_API_RATE_LIMIT_REQUESTS'] = '1' | ||
|
|
||
| 2.times do | ||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(200) | ||
| end | ||
| end | ||
|
|
||
| it 'uses 300 requests and 60 seconds as defaults' do | ||
| throttle = Rack::Attack.throttles.fetch('API requests') | ||
|
|
||
| expect(throttle.limit.call(nil)).to eq(300) | ||
| expect(throttle.period.call(nil)).to eq(60) | ||
| end | ||
|
|
||
| it 'throttles API requests when enabled' do | ||
| enable_rate_limit(requests: 2) | ||
|
|
||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(200) | ||
| expect(post('/api/v2/users', session: 'session-one')).to eq(200) | ||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(429) | ||
| end | ||
|
|
||
| it 'returns a JSON error response and logs the first rejected request' do | ||
| enable_rate_limit(requests: 1) | ||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(200) | ||
| expect(Rails.logger).to receive(:warn).with( | ||
| 'Rate limit exceeded: throttle=API requests method=GET path=/api/v2/cases limit=1 period=60' | ||
| ).once | ||
|
|
||
| response = get_response('/api/v2/cases', session: 'session-one') | ||
| error = JSON.parse(response.body).fetch('errors').first | ||
|
|
||
| expect(response.status).to eq(429) | ||
| expect(response.content_type).to eq('application/json; charset=utf-8') | ||
| expect(response['retry-after'].to_i).to be_between(1, 60) | ||
| expect(error).to eq( | ||
| 'status' => 429, | ||
| 'resource' => '/api/v2/cases', | ||
| 'message' => 'errors.api.too_many_requests' | ||
| ) | ||
|
|
||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(429) | ||
| end | ||
|
|
||
| it 'keeps separate limits for separate sessions' do | ||
| enable_rate_limit(requests: 1) | ||
|
|
||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(200) | ||
| expect(get('/api/v2/cases', session: 'session-two')).to eq(200) | ||
| expect(get('/api/v2/cases', session: 'session-one')).to eq(429) | ||
| end | ||
|
|
||
| it 'keeps separate limits for separate authorization credentials' do | ||
| enable_rate_limit(requests: 1) | ||
|
|
||
| expect(get('/api/v2/cases', authorization: 'Bearer token-one')).to eq(200) | ||
| expect(get('/api/v2/cases', authorization: 'Bearer token-two')).to eq(200) | ||
| expect(get('/api/v2/cases', authorization: 'Bearer token-one')).to eq(429) | ||
| end | ||
|
|
||
| it 'does not throttle requests outside the API' do | ||
| enable_rate_limit(requests: 1) | ||
|
|
||
| 2.times do | ||
| expect(get('/v2/dashboards', session: 'session-one')).to eq(200) | ||
| end | ||
| end | ||
|
|
||
| def enable_rate_limit(requests:) | ||
| ENV['PRIMERO_API_RATE_LIMIT_ENABLED'] = 'true' | ||
| ENV['PRIMERO_API_RATE_LIMIT_REQUESTS'] = requests.to_s | ||
| ENV['PRIMERO_API_RATE_LIMIT_PERIOD'] = '60' | ||
| end | ||
|
|
||
| def get(path, session: nil, authorization: nil) | ||
| get_response(path, session:, authorization:).status | ||
| end | ||
|
|
||
| def get_response(path, session: nil, authorization: nil) | ||
| client.get(path, request_headers(session:, authorization:)) | ||
| end | ||
|
|
||
| def post(path, session: nil, authorization: nil) | ||
| client.post(path, request_headers(session:, authorization:)).status | ||
| end | ||
|
|
||
| def request_headers(session:, authorization:) | ||
| headers = { 'REMOTE_ADDR' => '127.0.0.1' } | ||
| headers['HTTP_COOKIE'] = "_app_session=#{session}" if session | ||
| headers['HTTP_AUTHORIZATION'] = authorization if authorization | ||
| headers | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular order can be spoofed. A crafted request may set both a valid session cookie and a bogus authorization header. For that matter an attack can simply rotate invalid session and auth credentials, thereby bypassing rackattack. In this scenario I would only trust the underlying IP address
request.remote_ipthat comes from TCP/IP and is thus harder to mess with. Can you get rid ofauthorizationandsession?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, now it's only IP based.