From 5dad5701a9bd47ffca0e2b8a9e05293dc24b44a4 Mon Sep 17 00:00:00 2001 From: Brian Glass Date: Fri, 21 Aug 2026 11:35:13 -0400 Subject: [PATCH] Fix shadow-mode rate throttle to key on real client IP, not raw XFF 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 Claude-Session: https://claude.ai/code/session_01Hf6j2xXQXywHVh3HAVRxB3 --- calendarium/api.py | 25 ++++++++++++++++++++++--- calendarium/tests/test_api.py | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/calendarium/api.py b/calendarium/api.py index 976ecd1..3d3f584 100644 --- a/calendarium/api.py +++ b/calendarium/api.py @@ -26,15 +26,34 @@ BURST_RATE = settings.ORTHOCAL_API_RATELIMIT -class ShadowAnonRateThrottle(AnonRateThrottle): +class ClientIpMixin: + """ + ninja's own get_ident() only extracts a single client IP out of + X-Forwarded-For when NINJA_NUM_PROXIES is set; left unset (as it is + here), it falls back to keying on the *entire* raw XFF string. Cloud + Run's front end puts the real client IP first in that header and + appends its own hop(s) after it, and those appended hops aren't + guaranteed to stay the same across requests from the same client -- + so the unpatched behavior can fragment one client's requests across + many different throttle-cache keys instead of accumulating against a + single one, undercounting how often the real limit is actually hit. + """ + def get_ident(self, request): + xff = request.META.get('HTTP_X_FORWARDED_FOR') + if xff: + return xff.split(',')[0].strip() + return request.META.get('REMOTE_ADDR') + +class ShadowAnonRateThrottle(ClientIpMixin, AnonRateThrottle): """ Runs the real per-IP rate check and logs what WOULD be throttled, but always allows the request through -- no client sees a 429 yet. This is a deliberate rollout step: BURST_RATE was defined but never wired up to anything, so there's no data on how real (including third-party, non- browser) traffic would be affected by actually enforcing it. Watch the - logs this produces for a while, then swap this for AnonRateThrottle - directly (same rate, same identity-by-IP behavior) once it looks safe. + logs this produces for a while, then swap this for a plain + ClientIpMixin + AnonRateThrottle combo (same rate, same identity-by-IP + behavior, minus the shadow logging) once it looks safe. """ def allow_request(self, request): allowed = super().allow_request(request) diff --git a/calendarium/tests/test_api.py b/calendarium/tests/test_api.py index 726ce23..6254689 100644 --- a/calendarium/tests/test_api.py +++ b/calendarium/tests/test_api.py @@ -8,10 +8,31 @@ from django.urls import reverse from django.utils import timezone +from .. import api + # Build paths inside the project like this: BASE_DIR / 'subdir'. BASE_DIR = Path(__file__).resolve().parent +class ClientIpMixinTestCase(TestCase): + def setUp(self): + self.throttle = api.ShadowAnonRateThrottle('5/s') + self.factory = RequestFactory() + + def test_uses_first_xff_entry(self): + """Cloud Run puts the real client IP first in X-Forwarded-For and + appends its own hop(s) after it; those appended hops can vary + between requests from the same client, so only the first entry is + a stable per-client identity.""" + request = self.factory.get('/', HTTP_X_FORWARDED_FOR='203.0.113.5, 66.249.82.68') + self.assertEqual('203.0.113.5', self.throttle.get_ident(request)) + + def test_falls_back_to_remote_addr(self): + request = self.factory.get('/') + request.META['REMOTE_ADDR'] = '203.0.113.5' + self.assertEqual('203.0.113.5', self.throttle.get_ident(request)) + + class DayAPITestCase(TestCase): fixtures = ['calendarium.json', 'commemorations.json']