Skip to content

fix(content_feedback): prevent anonymous rate-limit bypass via spoofed X-Forwarded-For - #3751

Merged
zamanafzal merged 2 commits into
mainfrom
zafzal/12775-anon-throttle-num-proxies
Aug 12, 2026
Merged

fix(content_feedback): prevent anonymous rate-limit bypass via spoofed X-Forwarded-For#3751
zamanafzal merged 2 commits into
mainfrom
zafzal/12775-anon-throttle-num-proxies

Conversation

@zamanafzal

@zamanafzal zamanafzal commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What are the relevant tickets?

Fixes mitodl/hq#12775. Follow-up to #3738 (anonymous content-feedback submissions), part of mitodl/hq#11629.

Description (What does it do?)

The content_feedback rate throttle keys anonymous requests by client IP via DRF's get_ident. With NUM_PROXIES unset, DRF keys on the entire X-Forwarded-For header — and our nginx appends to that header (config/nginx.conf.erb), so a client can prepend arbitrary values and rotate them to bypass the limit.

This sets NUM_PROXIES = 2 in REST_FRAMEWORK (the trusted hops in front of the app: APISIX + nginx, confirmed with DevOps). DRF then reads the real client from the trusted, infra-appended entry (2nd from the right) instead of the spoofable left side. Kept env-overridable (get_int("NUM_PROXIES", 2)) in case the topology changes.

Now the sole guard on the open anonymous write endpoint actually holds.

Screenshot?

Screenshot 2026-08-11 at 3 14 58 PM

How can this be tested?

1. Automated

docker compose run --rm web uv run pytest content_feedback/views_test.py -v

test_anonymous_throttle_ignores_spoofed_xff rotates the client-controlled left entry of X-Forwarded-For while keeping the trusted suffix fixed and asserts the third request over a 2/min limit is 429. It fails on main (all 201 — bypass) and passes with this change.

2. Manual (mirrors the local repro from #3738 review)

Drop the limit so it's easy to trip, then restart web:

echo 'CONTENT_FEEDBACK_THROTTLE_RATE=2/min' >> env/backend.local.env
docker compose up -d web

Post straight to the web container (no proxy locally, so curl controls the whole header). Emulate the prod chain <spoofed>, <real-client>, <apisix> and rotate only the spoofable left entry:

for i in 1 2 3; do
  curl -s -o /dev/null -w "spoof #$i -> %{http_code}\n" \
    -X POST http://localhost:8061/api/v0/content_feedback/ \
    -H 'Content-Type: application/json' \
    -H "X-Forwarded-For: 9.9.9.$i, 203.0.113.5, 10.0.0.1" \
    -d '{"course_id":"course-v1:Test+XFF+2026","block_usage_key":"block-v1:demo+type@video+block@x","sentiment":"positive","comment":"xff test"}'
done
  • This branch → 201, 201, 429. Rotating the spoof no longer mints a new bucket; the trusted 203.0.113.5 keys all three.
  • See the old bypass → set NUM_PROXIES=99 in env/backend.local.env (trusts the spoofable left entry) and re-run → 201, 201, 201, matching Matt's reproduction on main.

Re-running within the same minute reuses the bucket — bump the real-client IP (203.0.113.6) or wait 60s for a clean pass.

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
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

OpenAPI Changes

No changes detected

View full changelog

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

@zamanafzal
zamanafzal marked this pull request as ready for review August 11, 2026 10:19
Copilot AI balanced review requested due to automatic review settings August 11, 2026 10:19
@zamanafzal zamanafzal added the Needs Review An open Pull Request that is ready for review label Aug 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Secures anonymous content-feedback throttling against spoofed X-Forwarded-For values.

Changes:

  • Configures DRF to trust two proxy hops.
  • Adds a regression test for spoofed forwarding headers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
main/settings.py Configures trusted proxy count.
content_feedback/views_test.py Tests anonymous IP throttling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread content_feedback/views_test.py Outdated
…nt 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.
@zamanafzal zamanafzal changed the title fix(content_feedback): key anonymous throttle on trusted client IP fix(content_feedback): prevent anonymous rate-limit bypass via spoofed X-Forwarded-For Aug 11, 2026
@mbertrand mbertrand self-assigned this Aug 11, 2026

@mbertrand mbertrand left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mbertrand mbertrand added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Aug 11, 2026
@zamanafzal
zamanafzal merged commit bba3ba8 into main Aug 12, 2026
13 checks passed
@zamanafzal
zamanafzal deleted the zafzal/12775-anon-throttle-num-proxies branch August 12, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants