openedx / openedx/openedx-platform
Clean up XForwardedForMiddleware
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 8.2k
- Forks
- 4.4k
- Avg merge
- 6d 18h
- Merged PRs (30d)
- 42
Description
XForwardedForMiddleware in edxapp contains several pieces of code relating to IP addresses.
The most recent part of the code (closest to the top, in that permalink) uses edx-django-utils to make various information about the client IP chain available to the application. This is currently working as desired, although it might not be the best place for it. However, the oldest part assumes there is only one true client IP, and then uses that to overwrite request.META['REMOTE_ADDR']. We'd like to delete this part, since it creates an incorrect and inconsistent view of the IP chain. This needs to be removed before the newer code can properly be extracted to a utility module somewhere—it overwrites information that the newer block needs to read. That order dependence is fragile.
In between, there's also some code that copies some values in request.META to new names, for a past Gunicorn upgrade. This will need to be carefully evaluated to determine whether it is still needed, and what to do with it. (Caution: The field and header variable names might be reversed here.) At the very least we'll want to expand the comments.
Acceptance criteria:
- DEPRs filed and accepted as needed
- Sweep for any remaining uses of
REMOTE_ADDRor other code that thinks it has the "real client IP" and isn't already using theedx_django_utils.iputils - No longer overwrite
request.META['REMOTE_ADDR'] - Move newer code (call to
ip.init_client_ips, instrumentation) into a edx-django-utils middleware, and use that middleware instead- Also recommend to other teams that they use this
- Do something with the
request.METAfield copying/renaming (delete? document better?)
Notes:
- Background reading: https://www.brainonfire.net/blog/2022/03/04/understanding-using-xff/
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with openedx/core/lib/x_forwarded_for/middleware.py at the linked XForwardedForMiddleware implementation and inspect the edx_django_utils.ip usage plus request.META field copying. Then sweep the repository for REMOTE_ADDR and other real-client-IP handling, and review the DEPR requirements. Done means the overwrite is removed, the newer middleware behavior is moved to edx-django-utils, and the field-copying decision and team guidance are documented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, networking
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100