diff --git a/content_feedback/views_test.py b/content_feedback/views_test.py index a647a06166..fc54011a23 100644 --- a/content_feedback/views_test.py +++ b/content_feedback/views_test.py @@ -155,8 +155,50 @@ def test_submit_rate_limited(user_client, mocker): # The limit is keyed per authenticated user: a different user still gets # through even after the first user is throttled. (The user_client fixture # shares one APIClient, so build a distinct client for the second user.) - # NB: anonymous requests instead key on client IP, which is spoofable via - # X-Forwarded-For -- tracked as a follow-up (mitodl/hq#12775). other_client = APIClient() other_client.force_login(UserFactory.create()) assert other_client.post(url, _payload()).status_code == 201 + + +def test_anonymous_throttle_ignores_spoofed_xff(mocker): + """The anon throttle keys on the trusted client IP, not the spoofable header. + + Our infrastructure (APISIX + nginx = NUM_PROXIES hops) appends the real + client IP to X-Forwarded-For; a client can prepend anything to the left. So + the throttle must key on the trusted entry (2nd from the right), which means + both: rotating the spoofable left side cannot bypass the limit, and a + genuinely different client still gets its own bucket (mitodl/hq#12775). + """ + from django.core.cache.backends.locmem import LocMemCache + + from main.throttles import RedisScopedRateThrottle + + mocker.patch.object( + RedisScopedRateThrottle, "cache", LocMemCache("throttle-test", {}) + ) + mocker.patch.object( + RedisScopedRateThrottle, "THROTTLE_RATES", {"content_feedback": "2/min"} + ) + + url = reverse("content_feedback:v0:content_feedback") + client = APIClient(enforce_csrf_checks=True) + + # XFF shape mirrors prod: ", , ". NUM_PROXIES=2 keys on the real client (2nd from the right). + def post(spoofed_ip, real_client="203.0.113.5"): + return client.post( + url, + _payload(), + HTTP_X_FORWARDED_FOR=f"{spoofed_ip}, {real_client}, 10.0.0.1", + ) + + # Rotating only the spoofable left entry does not mint new buckets: the one + # trusted client keys all three, so the third request is throttled. + assert post("9.9.9.1").status_code == 201 + assert post("9.9.9.2").status_code == 201 + assert post("9.9.9.3").status_code == 429 + + # A genuinely different client (different 2nd-from-right entry) gets a fresh + # bucket. This distinguishes correct keying from a constant peer/proxy + # address, which would instead collapse every client into the bucket above. + assert post("9.9.9.9", real_client="203.0.113.9").status_code == 201 diff --git a/main/settings.py b/main/settings.py index 2d16eab3e6..e581055089 100644 --- a/main/settings.py +++ b/main/settings.py @@ -649,6 +649,12 @@ def get_all_config_keys(): "DEFAULT_VERSIONING_CLASS": "rest_framework.versioning.NamespaceVersioning", "ALLOWED_VERSIONS": ["v0", "v1"], "ORDERING_PARAM": "sortby", + # Trusted reverse-proxy hops in front of the app (APISIX + nginx). Anonymous + # throttles identify the client via the Nth-from-last X-Forwarded-For entry; + # without this DRF keys on the whole header, which a client can spoof to + # bypass the limit (mitodl/hq#12775). nginx appends, so only the rightmost + # NUM_PROXIES entries are added by our infrastructure and can be trusted. + "NUM_PROXIES": get_int("NUM_PROXIES", 2), "DEFAULT_THROTTLE_RATES": { # Rate format is "/" (e.g. "10/min", "30/hour"); a blank # env value -> "" or None -> None -> throttle is a no-op (kill switch).