Conversation
Why are these changes being introduced: * Actual users were being blocked when switching tabs rapidly * We don't want to increase the throttle limit for everyone, so we will bypass the throttle for verified users Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-647 How does this address that need: * Abstracts list of throttles that allow turnstile bypass * Adds a check for turnstile verified users to bypass the req/ip and previous throttles they could bypass updated to use new abstraction * Adjusts which routes bypass the req/ip throttle using new abstraction (static pages don't need throttling) Document any side effects to this change: * Bots or absusive users that can pass the turnstile check now have more leeway to make requests
❌ 5 blocking issues (5 total)
@qltysh one-click actions:
|
| test 'turnstile_grace_cookie_valid? validates signed future expiration cookie' do | ||
| refute Rack::Attack.turnstile_grace_cookie_valid?(rack_request_with_turnstile_cookie) | ||
| refute Rack::Attack.turnstile_grace_cookie_valid?(rack_request_with_turnstile_cookie('tampered-value')) | ||
| refute Rack::Attack.turnstile_grace_cookie_valid?(rack_request_with_turnstile_cookie(turnstile_cookie(Time.current - 1.minute))) |
| refute Rack::Attack.turnstile_grace_cookie_valid?(rack_request_with_turnstile_cookie('tampered-value')) | ||
| refute Rack::Attack.turnstile_grace_cookie_valid?(rack_request_with_turnstile_cookie(turnstile_cookie(Time.current - 1.minute))) | ||
|
|
||
| assert Rack::Attack.turnstile_grace_cookie_valid?(rack_request_with_turnstile_cookie(turnstile_cookie(Time.current + 15.minutes))) |
| test 'free paths do not count against general req/ip throttle' do | ||
| limit = ENV.fetch('REQUESTS_PER_PERIOD', 100).to_i | ||
|
|
||
| [ '/', '/about', '/about-natural-language-search' ].each do |path| |
Coverage Report for CI Build 29520214549Coverage remained the same at 98.384%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR adjusts Rack::Attack throttling behavior so that users who have successfully passed the Cloudflare Turnstile challenge (and have a valid grace-period cookie) can bypass additional throttles—specifically the general per-IP request throttle—reducing false-positive blocking when rapidly switching tabs.
Changes:
- Centralizes “Turnstile-challenge eligible” throttles and “free/cheap” paths in constants, with helper methods for cookie validation and path exclusion.
- Extends the general
req/ipthrottle to honor the Turnstile grace cookie and to exclude additional cheap/static routes from counting against the per-IP limit. - Updates integration tests to cover the new helper methods and updated redirect/429 behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| config/initializers/rack_attack.rb | Adds constants/helpers and updates throttles/responder to allow Turnstile-verified bypass and free-path exclusions. |
| test/integration/rack_attack_throttle_429_test.rb | Updates responder behavior tests (redirect vs 429) to match new throttle routing. |
| test/integration/rack_attack_cookie_bypass_test.rb | Adds/extends integration coverage for cookie validation, free-path detection, and req/ip bypass behavior. |
Comments suppressed due to low confidence (1)
config/initializers/rack_attack.rb:211
throttled_responderredirects for any request that hits a throttle inTURNSTILE_CHALLENGE_THROTTLES(includingreq/ip), regardless of request type. This can return the full Turnstile HTML page to non-browser/XHR callers (e.g.,/libkeyand/browzineare fetched viafetch()), which can break those clients. Consider only redirecting for interactive HTML GET navigations and returning 429 for XHR/API requests.
if TURNSTILE_CHALLENGE_THROTTLES.include?(matched_throttle)
# Redirect to Turnstile challenge
return_to = "#{request.path_info}?#{request.query_string}".gsub(/\?$/, '')
[ 302,
{ 'Location' => "/turnstile?return_to=#{ERB::Util.url_encode(return_to)}" },
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test 'req/ip throttle redirects to Turnstile' do | ||
| env = build_mock_env(path: '/about', matched_throttle: 'req/ip') | ||
|
|
jazairi
left a comment
There was a problem hiding this comment.
This feels like a reasonable compromise. The extra leeway is worth keeping an eye on, but I think Turnstile is effective enough that it shouldn't matter too much.
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Document any side effects to this change:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing