Combine turnstile and rack-attack by JPrevost · Pull Request #410 · MITLibraries/timdex-ui

JPrevost · 2026-06-22T16:45:14Z

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

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

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

coveralls · 2026-06-22T16:47:12Z

Coverage Report for CI Build 28886842527

Coverage increased (+0.004%) to 98.384%

Details

Coverage increased (+0.004%) from the base build.
Patch coverage: 4 of 4 lines across 1 file are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	1547
Covered Lines:	1522
Line Coverage:	98.38%
Coverage Strength:	78.33 hits per line

💛 - Coveralls

qltysh · 2026-06-22T17:32:31Z

❌ 4 blocking issues (4 total)

Tool	Category	Rule	Count
rubocop	Lint	Assignment Branch Condition size for `verify` is too high. [<5, 18, 0> 18.68/17]	2	❌
rubocop	Lint	Method has too many lines. [12/10]	1	❌
rubocop	Lint	Use normalcase for method name numbers.	1	❌

Copilot

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 /results and /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

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.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

JPrevost · 2026-07-06T15:36:59Z

+  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


I'm not sure what hallucination this is, but this test passes and it seems like it should error if this was correct 🤷🏻

JPrevost · 2026-07-06T15:36:51Z

+  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


I'm not sure what hallucination this is, but this test passes and it seems like it should error if this was correct 🤷🏻

JPrevost · 2026-07-06T15:37:09Z

+  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


I'm not sure what hallucination this is, but this test passes and it seems like it should error if this was correct 🤷🏻

JPrevost · 2026-07-06T15:44:44Z

+    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


Not worth it

Yeah, I don't know how likely this is to occur in reality.

https://mitlibraries.atlassian.net/browse/USE-634

qltysh · 2026-07-06T15:50:02Z

+      same_site: :lax
+    }
+
    redirect_to safe_return_path


Found 2 issues:

1. Assignment Branch Condition size for verify is too high. [<5, 18, 0> 18.68/17] [rubocop:Metrics/AbcSize]

2. Method has too many lines. [12/10] [rubocop:Metrics/MethodLength]

jazairi

This is looking really good! I think we need a bit more test coverage, though. Let me know if you disagree.

jazairi · 2026-07-06T18:26:38Z

+    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


Yeah, I don't know how likely this is to occur in reality.

jazairi · 2026-07-06T18:55:31Z

+      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


I'm not sure this condition is covered in the test suite. If not, could we add something to ensure that requests with tampered cookies redirect to Turnstile?

Related to this, I feel like we should confirm that expired cookies are handled as expected. (I.e., when the grace period expires, the request is referred to Turnstile.)

jazairi · 2026-07-06T19:37:42Z

+        { 'Location' => "/turnstile?return_to=#{ERB::Util.url_encode(return_to)}" },
+        [''] ]
+    else
+      # Default 429 for other throttled paths or throttles without grace period support


We should probably confirm this condition in the test suite. If something goes wrong with it, I think the user agent could end up in an infinite loop.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

qltysh · 2026-07-07T14:12:10Z

+    mock_response.errors.stubs(:details).returns(Object.new)
+    mock_response.errors.details.stubs(:to_h).returns({})
+
+    TimdexBase::Client.stubs(:query).returns(mock_response)


Assignment Branch Condition size for stub_timdex_client is too high. [<1, 20, 0> 20.02/17] [rubocop:Metrics/AbcSize]

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

+- `RESULTS_THROTTLE_LIMIT` - number of requests to `/results` and `/record` endpoints per `RESULTS_THROTTLE_PERIOD` (default 10). This is much stricter than the general throttle to defend against distributed botnet attacks. When this limit is exceeded, requests are redirected to Turnstile for challenge verification rather than returning a hard 429 error, allowing legitimate users to prove they're human.
+- `RESULTS_THROTTLE_PERIOD` - time in minutes for `/results` and `/record` endpoint throttle (default 1 minute). Throttled requests are redirected to Turnstile for verification.
+- `TURNSTILE_GRACE_PERIOD` - time in minutes that an IP is whitelisted from throttling after successfully passing Turnstile verification (default 15 minutes). This prevents users from being re-challenged repeatedly.
+- `RACK_ATTACK_VERBOSE_LOGGING` - Set to `false` to disable detailed Rack::Attack throttle event logging to stdout (default `true`). Useful for reducing test output noise while still maintaining throttle functionality.


+  def setup
+    # Clear cache before each test
+    Rails.cache.clear
+
+    # Stub PrimoSearch to avoid external API calls
+    stub_primo_search
+
+    # Stub TimdexBase::Client to avoid external GraphQL calls
+    stub_timdex_client
+  end


+  test 'valid grace period cookie bypasses throttle when throttle is active' do
+    # Get the throttle limit from environment or use default
+    limit = ENV.fetch('RESULTS_GLOBAL_LIMIT_PER_SEC', 30).to_i
+


jazairi

Looks good! Appreciate the logging tweak, as that was a minor concern I had. 🙂

- Add comprehensive integration tests for grace period cookie bypass behavior, throttle response codes (429 vs redirects), and edge cases like expired/invalid/malformed cookies. - Updated the `throttled_responder` to handle both Hash env and Rack::Request objects (for easier testing) - Add `RACK_ATTACK_VERBOSE_LOGGING` environment variable (default `true`) to allow disabling detailed throttle event logging to stdout, useful for reducing test output noise while maintaining throttle functionality.

mitlib temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 16:48 Inactive

JPrevost temporarily deployed to timdex-ui-gdt June 22, 2026 17:12 Inactive

JPrevost temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 17:24 Inactive

JPrevost temporarily deployed to geodata-stage June 22, 2026 17:26 Inactive

JPrevost temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 17:30 Inactive

JPrevost temporarily deployed to geodata-stage June 22, 2026 17:30 Inactive

JPrevost deployed to timdex-ui-gdt June 22, 2026 17:31 View deployment

qltysh Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread app/controllers/turnstile_controller.rb Outdated

JPrevost temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 18:06 Inactive

JPrevost temporarily deployed to geodata-stage June 22, 2026 18:07 Inactive

jazairi self-assigned this Jun 22, 2026

JPrevost requested a review from Copilot June 22, 2026 18:28

Copilot started reviewing on behalf of JPrevost June 22, 2026 18:29 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

jazairi requested changes Jun 22, 2026

View reviewed changes

JPrevost temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 20:12 Inactive

JPrevost requested a review from Copilot June 22, 2026 20:13

Copilot started reviewing on behalf of JPrevost June 22, 2026 20:13 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread config/initializers/rack_attack.rb Outdated

Comment thread app/controllers/turnstile_controller.rb Outdated

Comment thread test/controllers/turnstile_controller_test.rb

Comment thread test/controllers/turnstile_controller_test.rb

JPrevost temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 20:34 Inactive

JPrevost requested a review from Copilot June 22, 2026 20:35

Copilot started reviewing on behalf of JPrevost June 22, 2026 20:36 View session

JPrevost temporarily deployed to geodata-stage June 22, 2026 20:37 Inactive

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread config/initializers/rack_attack.rb

JPrevost temporarily deployed to timdex-ui-pi-turnstile--p4o43h June 22, 2026 20:41 Inactive

JPrevost requested a review from Copilot June 22, 2026 20:42

Copilot started reviewing on behalf of JPrevost June 22, 2026 20:42 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread config/initializers/rack_attack.rb

Comment thread config/initializers/rack_attack.rb Outdated

Comment thread app/controllers/turnstile_controller.rb Outdated

JPrevost force-pushed the turnstile-plus-rackattack branch from 4a17ed5 to 83f8ac3 Compare July 6, 2026 14:29

JPrevost requested a review from Copilot July 6, 2026 14:48

Copilot started reviewing on behalf of JPrevost July 6, 2026 14:48 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

JPrevost force-pushed the turnstile-plus-rackattack branch from 83f8ac3 to 82fe6e0 Compare July 6, 2026 15:03

JPrevost requested a review from Copilot July 6, 2026 15:04

Copilot started reviewing on behalf of JPrevost July 6, 2026 15:05 View session

qltysh Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread app/controllers/turnstile_controller.rb

Copilot AI reviewed Jul 6, 2026

View reviewed changes

Global throttle and improved turnstile

1ff05c3

https://mitlibraries.atlassian.net/browse/USE-634

JPrevost force-pushed the turnstile-plus-rackattack branch from 82fe6e0 to 1ff05c3 Compare July 6, 2026 15:48

JPrevost requested a review from jazairi July 6, 2026 15:49

qltysh Bot reviewed Jul 6, 2026

View reviewed changes

jazairi requested changes Jul 6, 2026

View reviewed changes

qltysh Bot reviewed Jul 7, 2026

View reviewed changes

JPrevost force-pushed the turnstile-plus-rackattack branch from 58dfcd9 to 849e9f8 Compare July 7, 2026 13:40

qltysh Bot reviewed Jul 7, 2026

View reviewed changes

JPrevost requested a review from Copilot July 7, 2026 13:42

Copilot started reviewing on behalf of JPrevost July 7, 2026 13:43 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

Comment thread test/integration/rack_attack_cookie_bypass_test.rb

Comment thread test/integration/rack_attack_cookie_bypass_test.rb Outdated

Comment thread test/integration/rack_attack_cookie_bypass_test.rb

Comment thread test/integration/rack_attack_throttle_429_test.rb Outdated

qltysh Bot reviewed Jul 7, 2026

View reviewed changes

JPrevost requested a review from Copilot July 7, 2026 14:17

Copilot started reviewing on behalf of JPrevost July 7, 2026 14:18 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

JPrevost requested a review from jazairi July 7, 2026 15:04

jazairi approved these changes Jul 7, 2026

View reviewed changes

JPrevost force-pushed the turnstile-plus-rackattack branch from 6fb3835 to 7f8956d Compare July 7, 2026 17:46

JPrevost merged commit b475a97 into main Jul 7, 2026
6 checks passed

JPrevost deleted the turnstile-plus-rackattack branch July 7, 2026 17:48

Uh oh!

Conversation

JPrevost commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 28886842527

Coverage increased (+0.004%) to 98.384%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

qltysh Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jazairi left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

JPrevost commented Jun 22, 2026 •

edited

Loading

coveralls commented Jun 22, 2026 •

edited

Loading

qltysh Bot commented Jun 22, 2026 •

edited

Loading