Combine turnstile and rack-attack#410
Open
JPrevost wants to merge 1 commit into
Open
Conversation
Coverage Report for CI Build 28801419369Coverage increased (+0.004%) to 98.384%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 3 blocking issues (3 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds Rack::Attack-based protections for the expensive /results and /record endpoints and ties throttling into the existing Turnstile flow so legitimate users can recover from throttling via challenge verification.
Changes:
- Introduces a global RPS throttle plus a stricter per-IP throttle for
/resultsand/record. - Overrides Rack::Attack throttled responses on those endpoints to redirect to the Turnstile challenge instead of returning 429.
- After successful Turnstile verification, writes a short-lived “verified” cache key intended to suppress re-challenges for that IP.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| README.md | Documents new ENV knobs for the results/record throttles and Turnstile grace period. |
| config/initializers/rack_attack.rb | Adds new throttles and custom throttled response logic for /results and /record. |
| app/controllers/turnstile_controller.rb | Writes a cache-based grace-period marker after successful Turnstile verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jazairi
requested changes
Jun 22, 2026
jazairi
left a comment
jazairi
left a comment
Contributor
There was a problem hiding this comment.
I think all of Copilot's suggested feedback is worth implementing, particularly the fix for the potential feedback loop. Once that is in place, this is ready to merge from my perspective.
I was pondering adding unit tests for the grace period and throttle_response logic, but I couldn't figure a good way to do so that wouldn't just be testing implementation details. In this case, I think manual testing is sufficient. Anything more would likely have to be a system test, which would be new to us and well out of scope of this urgent change.
| require 'test_helper' | ||
|
|
||
| class RackAttackBlocklistTest < ActionDispatch::IntegrationTest | ||
| def test_blocked_user_agent_returns_403 |
JPrevost
force-pushed
the
turnstile-plus-rackattack
branch
3 times, most recently
from
4a17ed5 to
83f8ac3
Compare
JPrevost
force-pushed
the
turnstile-plus-rackattack
branch
from
83f8ac3 to
82fe6e0
Compare
| secure: Rails.env.production? | ||
| } | ||
|
|
||
| redirect_to safe_return_path |
Comment on lines
+4
to
+7
| def test_blocked_user_agent_returns_403 | ||
| get '/results', params: { q: 'test' }, headers: { 'HTTP_USER_AGENT' => 'Sogou web spider/4.0' } | ||
| assert_equal 403, status | ||
| end |
Comment on lines
+9
to
+15
| def test_blocklist_with_partial_user_agent_match | ||
| # 'Sogou web spider' should match 'Sogou web spider/4.0 (compatible; like Gecko)' | ||
| # via include? partial string match | ||
| get '/results', params: { q: 'test' }, | ||
| headers: { 'HTTP_USER_AGENT' => 'Sogou web spider/4.0 (compatible; like Gecko)' } | ||
| assert_equal 403, status | ||
| end |
Comment on lines
+17
to
+21
| def test_blocked_user_agent_substring_match | ||
| # Verify that partial matches work | ||
| get '/results', params: { q: 'test' }, headers: { 'HTTP_USER_AGENT' => 'Sogou web spider' } | ||
| assert_equal 403, status | ||
| end |
| end | ||
| end | ||
|
|
||
| test 'verify sets turnstile_verified_at encrypted cookie for grace period' do |
Comment on lines
+45
to
+50
| cookies[:turnstile_verified_at] = { | ||
| value: signed_value, | ||
| expires: expiration_time, | ||
| httponly: true, | ||
| secure: Rails.env.production? | ||
| } |
Comment on lines
+71
to
+76
| cookie_value = req.cookies['turnstile_verified_at'] | ||
| if cookie_value.present? | ||
| begin | ||
| expiration_timestamp = Rails.application.message_verifier(:turnstile_grace).verify(cookie_value) | ||
| next nil if expiration_timestamp > Time.current.to_i | ||
| rescue ActiveSupport::MessageVerifier::InvalidSignature |
Comment on lines
+53
to
+56
| # Global rate limit for /results and /record endpoints (excluding any Rack::Attack safelisted IPs) | ||
| # to protect against distributed volume attacks. Per-IP throttling can be bypassed by rotating | ||
| # through many IPs; this shared counter caps total throughput for all non-safelisted traffic. | ||
| # |
Comment on lines
+35
to
+39
| Rack::Attack.blocklist('user_agent/blocked') do |req| | ||
| is_blocked = blocked_agents.any? { |agent| req.user_agent&.include?(agent) } | ||
| if is_blocked | ||
| Rails.logger.warn("BLOCKED_USER_AGENT: #{req.user_agent} | IP: #{req.ip} | Path: #{req.path}") | ||
| end |
Comment on lines
+187
to
+189
| # Log all throttled requests to understand traffic patterns | ||
| Rails.logger.warn("THROTTLED_REQUEST: UA=#{request.user_agent} | IP=#{request.ip} | Path=#{request.path} | Throttle=#{matched_throttle}") | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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