From a96681c2ae72f9e2b56632aca20b9285238cd217 Mon Sep 17 00:00:00 2001 From: zamanafzal Date: Tue, 11 Aug 2026 11:59:14 +0500 Subject: [PATCH 1/2] fix(content_feedback): key anonymous throttle on trusted client IP Set REST_FRAMEWORK NUM_PROXIES=2 (APISIX + nginx) so the anonymous rate throttle identifies the client from the trusted, infra-appended entry of X-Forwarded-For instead of the whole spoofable header. Without it a client could rotate X-Forwarded-For to bypass the limit entirely. Fixes mitodl/hq#12775 --- content_feedback/views_test.py | 38 ++++++++++++++++++++++++++++++++-- main/settings.py | 6 ++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/content_feedback/views_test.py b/content_feedback/views_test.py index a647a06166..570b06ddb7 100644 --- a/content_feedback/views_test.py +++ b/content_feedback/views_test.py @@ -155,8 +155,42 @@ 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): + """A rotating client-supplied X-Forwarded-For cannot bypass the anon limit. + + Anonymous requests key on the real client IP, which our infrastructure + (APISIX + nginx = NUM_PROXIES hops) appends to X-Forwarded-For. A client can + prepend anything to that header, so the throttle must ignore the spoofable + left side and key on the trusted entry (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) + + # Same real client (203.0.113.5) and trusted proxy (10.0.0.1); only the + # spoofable, client-controlled left entry rotates. All three share a bucket. + def post(spoofed_ip): + return client.post( + url, + _payload(), + HTTP_X_FORWARDED_FOR=f"{spoofed_ip}, 203.0.113.5, 10.0.0.1", + ) + + 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 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). From 71e4c9e9cd5365caee8c21a8e2d12073789a089f Mon Sep 17 00:00:00 2001 From: zamanafzal Date: Tue, 11 Aug 2026 15:31:18 +0500 Subject: [PATCH 2/2] test(content_feedback): assert anon throttle keys on the trusted client IP The regression only proved rotating the spoofable left entry shares a bucket -- which also holds if every request were keyed to one constant proxy/peer address. Add an assertion that a genuinely different client (different 2nd-from-right entry) gets a fresh 201, verifying the throttle selects the trusted address rather than merely ignoring the spoof. --- content_feedback/views_test.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/content_feedback/views_test.py b/content_feedback/views_test.py index 570b06ddb7..fc54011a23 100644 --- a/content_feedback/views_test.py +++ b/content_feedback/views_test.py @@ -161,12 +161,13 @@ def test_submit_rate_limited(user_client, mocker): def test_anonymous_throttle_ignores_spoofed_xff(mocker): - """A rotating client-supplied X-Forwarded-For cannot bypass the anon limit. + """The anon throttle keys on the trusted client IP, not the spoofable header. - Anonymous requests key on the real client IP, which our infrastructure - (APISIX + nginx = NUM_PROXIES hops) appends to X-Forwarded-For. A client can - prepend anything to that header, so the throttle must ignore the spoofable - left side and key on the trusted entry (mitodl/hq#12775). + 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 @@ -182,15 +183,22 @@ def test_anonymous_throttle_ignores_spoofed_xff(mocker): url = reverse("content_feedback:v0:content_feedback") client = APIClient(enforce_csrf_checks=True) - # Same real client (203.0.113.5) and trusted proxy (10.0.0.1); only the - # spoofable, client-controlled left entry rotates. All three share a bucket. - def post(spoofed_ip): + # 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}, 203.0.113.5, 10.0.0.1", + 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