Fix shadow-mode rate throttle to key on real client IP - #210
Merged
Conversation
NINJA_NUM_PROXIES is unset, so django-ninja's get_ident() was falling back to keying the throttle cache on the entire X-Forwarded-For string. Cloud Run's front end appends its own hop IP(s) after the real client IP, and those hops aren't guaranteed to stay the same across requests from the same client -- fragmenting one client's requests across many throttle buckets and undercounting how often the real limit is hit. Confirmed via the collected shadow-mode logs: only 10 "would throttle" events against ~32k requests in 36 hours, several of which showed the same client IP paired with a different second XFF entry per request. ClientIpMixin now explicitly takes the first XFF entry (or REMOTE_ADDR if XFF is absent), matching Cloud Run's documented client-first XFF format, so the shadow-mode data collected going forward reflects real per-client behavior before deciding whether to enforce the limit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hf6j2xXQXywHVh3HAVRxB3
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.
Summary
ShadowAnonRateThrottle, live since Wire up API rate limiting in shadow mode #205) turned up only 10 "would throttle" events against ~32k/api/requests in 36 hours -- but several of those events showed the same client IP paired with a different secondX-Forwarded-Forentry per request.NINJA_NUM_PROXIESis unset, so django-ninja'sget_ident()falls back to keying the throttle cache on the entire raw XFF string rather than just the client IP. Cloud Run's front end appends its own hop IP(s) after the real client IP, and those hops vary between requests -- fragmenting one client's requests across many different throttle buckets and undercounting how often the real limit is actually hit.ClientIpMixinfixes this by explicitly taking the first XFF entry (Cloud Run's documented client-first format), falling back toREMOTE_ADDRwhen XFF is absent.Test plan
ClientIpMixinTestCaseverifyingget_ident()extracts the first XFF entry and falls back toREMOTE_ADDRdocker compose run --rm tests)gcloud logging readthat the current unpatched behavior does produce inconsistent per-client keys🤖 Generated with Claude Code
https://claude.ai/code/session_01Hf6j2xXQXywHVh3HAVRxB3