Fix frequent Retry later responses in production - #4402
Draft
ferblape wants to merge 7 commits into
Draft
Conversation
Visitors were seeing rack-attack's default plain text "Retry later" page often. The limits were far below real usage: 20 requests per 10 seconds and 1000 per hour, per IP, with no exemptions, while a town hall office shares a single NAT address and a portal page fires several requests. Rework the policy into RackAttackRules, with a budget per area and every limit and period readable from the environment, so a limit can be fixed in production without a deploy: requests by ip burst 240 / minute requests by ip sustained 4000 / hour agendas by ip 90 / minute, GET and HEAD agendas with bogus dates by ip 10 / hour budgets execution by ip 60 / minute, GET and HEAD Safelist static files, private addresses and a configurable list of ranges. Key the rules on the address ActionDispatch resolves, the one the audit trail records, instead of Rack's, rescuing the spoof check so that crafted headers cannot turn into a 500 from a middleware. Replace the Fail2Ban rule on out of window agenda dates. Fail2Ban#fail! returns true unconditionally, so the first such request was already answered with 403 and five of them banned the whole agenda section for the address for an hour, which a visitor reaches by clicking through the calendar. The date parsing also called request.params without rescuing: "/agendas?date=1&date[x]=2" raised Rack::QueryParser::ParameterTypeError out of the middleware. It is a throttle now, it counts only the bogus requests, and it parses with the window and the parser the controller already uses. Answer with a localized 429 carrying Retry-After, in HTML or JSON depending on what the client asked for, instead of a text/plain body that breaks the calendar's fetch. Name the matched rule in the log and count it in AppSignal: the five rules used to write the same line, so there was no way to tell which limit a client had hit. Fill the address fields lograge already reads and adds nothing to today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… send The search box sent one request per keystroke: devbridge-autocomplete defaults deferRequestBy to 0 and the settings never set it, so typing a single query was about twenty requests in a few seconds. Debounce them. The calendar let the visitor navigate outside the window the server accepts, which answers 422, and its fetch called r.json() on any response, so an error left the calendar empty with no explanation. Bound navigation with the window the controller enforces, passed in from the view, and report a failed request through FullCalendar's failure callback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ferblape
marked this pull request as ready for review
September 1, 2026 05:47
append_info_to_payload resolves the client address for every action, and ActionDispatch raises IpSpoofAttackError when Client-IP and X-Forwarded-For contradict each other. That error is not mapped in rescue_responses, and the hook runs in the ensure of process_action, so a response that had already been rendered was discarded and returned as a 500. The other address fields still record the raw headers, so the spoofing attempt remains visible in the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FullCalendar writes event source failures to the console when no eventSourceFailure handler is set, so the too_many_requests translation never reached the visitor: a throttled calendar just stayed empty. The message is now rendered next to the calendar and cleared when a fetch starts, so a successful retry removes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The source was assigned first, so a concurrent caller could take the early return and read the ranges while they were still nil. safelisted_ip? then raised NoMethodError, which its rescue clause does not cover, and the safelist blew up as a 500 out of the middleware. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The integration test could not pass: with lograge disabled, as it is outside production and staging, Rails::Rack::Logger builds its "Started GET" line by calling request.remote_ip, so the spoofing check fires before the app runs and no controller change can prevent it. Driving append_info_to_payload against a request annotated the way ActionDispatch::RemoteIp annotates it exercises the call site this concern actually owns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ferblape
marked this pull request as draft
September 3, 2026 13:20
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
1 participant
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.
✌️ What does this PR do?
Visitors were often getting rack-attack's default plain text
Retry laterpage: the limits were 20 requests per 10 seconds and 1000 per hour per IP, with no exemptions, while a town hall office shares one NAT address and the search box sent one request per keystroke. The policy moves tolib/middlewares/rack_attack_rules.rbwith a budget per area (240/min burst, 4000/h sustained, 90/min agendas, 60/min budgets execution) and every limit readable from the environment, so a limit can be fixed in production without a deploy; requests are keyed on the addressActionDispatchresolves and static files, private addresses andRACK_ATTACK_SAFELIST_IPSare exempt. It also fixes two bugs found on the way:Fail2Ban#fail!returns true unconditionally, so the first out-of-window agenda date already answered 403 and five of them banned the whole section for an hour (now a 10/h throttle on the bogus requests only), and the date filter calledrequest.paramswithout rescuing, so/agendas?date=1&date[x]=2raised out of the middleware as a 500. Throttled clients now get a localized 429 withRetry-After, in HTML or JSON, and the log names which rule fired — the five rules used to write identical lines — with an AppSignal counter alongside. On the front end the autocomplete is debounced, the calendar is bounded to the window the server accepts and a failed fetch no longer leaves it blank.🔍 How should this be manually tested?
On staging: type a full query in the search box and navigate months in a person's agenda (
/agendas/<slug>), both with zero 429 in the network tab;curl -si "$STAGING/agendas?start_date=1900-01-01"repeated ~15 times should end in a localized 429 carryingRetry-After; andcurl -s -o /dev/null -w '%{http_code}' "$STAGING/agendas?date=1&date%5Bx%5D=2"must not be a 500. After deploying, withENABLE_LOG_RAGEon,grep '\[rack_attack\]'shows which rule fires andgrep -o 'x_forwarded_for=[^ ]*'tells whether client addresses are the visitors' own or a shared proxy's.👀 Screenshots
Before this PR
Retry laterastext/plain, noRetry-Afterheader.After this PR
Localized 429 page (es/en/ca) with
Retry-After, or a JSON body for XHR and.jsonrequests..env.example?config/application.yml?config/secrets.yml?RACK_ATTACK_ENABLED,RACK_ATTACK_SAFELIST_IPSand oneRACK_ATTACK_<RULE>_LIMIT/_PERIODper rule. All optional — the defaults inRackAttackRules::DEFAULTSapply when unset — but the Ansible role may want them so limits can be tuned without a deploy.📖 Does this PR require updating the documentation?
Rate limiting is now enabled in production and staging only; it was previously on in development too.