Skip to content

Optional API rate limiter introduced - #511

Merged
pnabutovsky merged 3 commits into
primeroIMS:developfrom
fphgov:optional-api-rate-limiter
Aug 25, 2026
Merged

Optional API rate limiter introduced#511
pnabutovsky merged 3 commits into
primeroIMS:developfrom
fphgov:optional-api-rate-limiter

Conversation

@tkardos

@tkardos tkardos commented Aug 14, 2026

Copy link
Copy Markdown

No description provided.

@jbetala7

Copy link
Copy Markdown

The four failing specs are currently exercising the disabled path. rack_attack.rb reads PRIMERO_API_RATE_LIMIT_ENABLED, while the new spec's environment_variables list and enable_rate_limit helper use PRIMERO_RATE_LIMIT_ENABLED. The checked variable is never set, so the requests remain 200 instead of 429. Renaming those two test references to PRIMERO_API_RATE_LIMIT_ENABLED and rerunning bundle exec rspec spec/requests/api/v2/tokens_controller_spec.rb should exercise the limiter.

Comment thread config/initializers/rack_attack.rb Outdated

authorization = request.get_header('HTTP_AUTHORIZATION').presence
session = request.cookies['_app_session'].presence
identifier = authorization || session || request.remote_ip

Copy link
Copy Markdown
Contributor

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_ip that comes from TCP/IP and is thus harder to mess with. Can you get rid of authorization and session?

Copy link
Copy Markdown
Author

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.

Comment thread config/locales/hu.yml
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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/

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping out on this one

end
end

describe 'Rack::Attack API rate limit' do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this new code to a separate new file spec/middleware/rack_attack_spec.rb?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to a new file as advised.

@tkardos
tkardos requested a review from pnabutovsky August 24, 2026 22:32

@pnabutovsky pnabutovsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff

@pnabutovsky
pnabutovsky merged commit 57581f6 into primeroIMS:develop Aug 25, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants