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']