Fix shadow-mode rate throttle to key on real client IP

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)

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