fix: surface the real client IP from nginx's X-Real-IP (#274) - #341
Open
mithro wants to merge 7 commits into
Open
fix: surface the real client IP from nginx's X-Real-IP (#274)#341mithro wants to merge 7 commits into
mithro wants to merge 7 commits into
Conversation
Design for issue #274: Django reads REMOTE_ADDR, which behind gunicorn on a unix socket is empty/local, so the real visitor IP never reaches the application. Approach: django-ipware behind a thin RealClientIPMiddleware that reads the authoritative X-Real-IP set by nginx and rewrites REMOTE_ADDR, so all present and future consumers read the correct IP. The four existing X-Forwarded-For[0] extraction sites (spoofable) are refactored onto a single shared helper. Cloudflare range validation stays at nginx (set_real_ip_from), the only layer that can see the connecting peer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ci2kPzRxhvaHXAz7MjrTjC
Spec review noted gunicorn on a unix socket leaves REMOTE_ADDR as an empty string, not absent. The three consumers store into GenericIPAddressField over a Postgres inet column, where "" raises DataError but None is valid. Normalise falsy to None in the shared helper and cover it with a test. Also sharpen the django-ipware assumption to require verification against the installed package, and require re-establishing the test baseline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ci2kPzRxhvaHXAz7MjrTjC
Failing tests for the design in docs/superpowers/specs/2026-07-16-real-client-ip-design.md: the middleware rewrites REMOTE_ADDR from a trusted X-Real-IP only when TRUST_X_REAL_IP is on and never consults X-Forwarded-For; the shared helper returns REMOTE_ADDR normalised so "" (gunicorn on a unix socket) becomes None. Adds django-ipware==7.0.1 for the header parsing. Part of #274 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4hgdz4RAYqZVLWHZRYrd2
Behind gunicorn on a unix socket REMOTE_ADDR is empty, so audit logs, TOS acceptance records and anything else reading it recorded the wrong value. nginx now resolves the true visitor (Cloudflare-scoped real_ip, hetzner-ansible#132/#133) and forwards it in X-Real-IP; this middleware copies that into REMOTE_ADDR, registered first so every later consumer sees it. - Only X-Real-IP is consulted (django-ipware with a fixed header order); X-Forwarded-For is never read -- its first entry is client-controlled. - Gated by TRUST_X_REAL_IP: False in base (dev/test), True in stage/prod where nginx is the sole ingress and gunicorn is socket-only. - Absent/malformed header -> REMOTE_ADDR untouched; no spoofable fallback. - New wafer_space.core.utils.get_client_ip() is the single source of truth for consumers and normalises "" to None so the value can be stored in a GenericIPAddressField (Postgres inet rejects ""). Design: docs/superpowers/specs/2026-07-16-real-client-ip-design.md Part of #274 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4hgdz4RAYqZVLWHZRYrd2
Rewrite the tests that encoded the old, spoofable behaviour (first X-Forwarded-For entry wins) to the new model: REMOTE_ADDR -- as resolved from the trusted X-Real-IP by RealClientIPMiddleware -- is recorded, and a client-supplied X-Forwarded-For never overrides it. Covers the compliance helper, project access log, and both TOS-acceptance paths. Part of #274 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4hgdz4RAYqZVLWHZRYrd2
Delete the X-Forwarded-For-first extraction in the compliance view, the two project access-log sites and the TOS acceptance view. Its first entry is client-controlled (nginx appends the real IP after it), so a request carrying a forged X-Forwarded-For was recorded as that forged value. Everything now reads REMOTE_ADDR -- rewritten from the nginx-set X-Real-IP by RealClientIPMiddleware -- through the one shared helper, which also normalises "" to None for the inet-backed GenericIPAddressField columns. Closes #274 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4hgdz4RAYqZVLWHZRYrd2
The four pre-deploy assumptions (nginx overwrites X-Real-IP at server level, hetzner-ansible#132/#133 merged and rendered live, django-ipware 7.0.1 API semantics probed against the installed package, fresh test baseline) were all checked during implementation; note that and the new branch name so the spec matches the PR. Part of #274 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4hgdz4RAYqZVLWHZRYrd2
|
Warning Review limit reached
Next review available in: 46 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #274
Problem
Behind gunicorn on a unix socket
REMOTE_ADDRis empty, so audit logs, TOS-acceptance records and the compliance certification recorded the wrong client IP. Worse, four call sites preferred the firstX-Forwarded-Forentry — which is client-controlled (nginx appends the real IP after it) — so a request carrying a forgedX-Forwarded-Forwas recorded as the forged value. FixingREMOTE_ADDRalone would have been bypassed by that path; both are fixed together.The infra side is done: hetzner-ansible#132/#133 give the platform nginx the true visitor (Cloudflare-scoped
real_ip), and since #144 it setsX-Real-IP $remote_addrat server level so it is overwritten on every proxied location. This PR is the Django side that consumes it.Design
Implements
docs/superpowers/specs/2026-07-16-real-client-ip-design.md(user-approved; carried over from the spec-onlyissue/274-real-client-ipbranch, with its "assumptions to verify" now recorded as verified).wafer_space/core/middleware.py—RealClientIPMiddleware, registered first inMIDDLEWARE. WhenTRUST_X_REAL_IPis on, it copies theX-Real-IPvalue intoREMOTE_ADDRviadjango-ipwarewithrequest_header_order=("HTTP_X_REAL_IP",), soX-Forwarded-Foris never consulted. Absent/malformed header →REMOTE_ADDRuntouched (no spoofable fallback).TRUST_X_REAL_IP:Falseinbase.py(dev/test),Trueinstage.py/prod.py. Trust is safe there because nginx is the sole ingress and gunicorn is socket-only — a client-suppliedX-Real-IPis overwritten by nginx before it can reach Django.wafer_space/core/utils.get_client_ip()— single source of truth for consumers; returnsREMOTE_ADDRnormalised so""(the unix-socket case) becomesNone, which theGenericIPAddressField/Postgresinetcolumns accept and""does not (a latentDataErrorin the old code).projects/views_compliance.py, both blocks inprojects/mixins.py,legal/views.py) drop their inlineX-Forwarded-Forlogic and use the helper; the old localget_client_ipinviews_complianceis deleted.django-ipware==7.0.1(latest; sole deppython-ipware). Its semantics were probed against the installed package before use.Testing (TDD — every test watched failing first)
wafer_space/core/tests/test_middleware.py: rewrite fromX-Real-IP(v4 + v6), header absent, header malformed,X-Forwarded-For-only ignored,TRUST_X_REAL_IP=FalseignoresX-Real-IP, and a guard that the middleware is first inMIDDLEWARE.wafer_space/core/tests/test_utils.py: helper returnsREMOTE_ADDR;Nonewhen absent;None(not"") when empty.legal/tests/test_views.py,legal/tests/test_oauth_tos_flow.py,projects/tests/test_views_compliance.py) and added an access-log XFF-ignore test inprojects/tests/test_mixins.py.make lint-fix && make lint && make type-checkclean;make test: 1538 passed, 3 skipped, 0 failed (main: 1530).Rollout note
Nothing to configure at deploy time: stage/prod already set the flag in settings, and nginx already sends
X-Real-IP(re-rendered live on both on 2026-08-17). Verify after deploy by accepting the TOS / viewing a project as staff and checking the recordedip_addressis the real visitor rather than empty or192.168.27.1.🤖 Generated with Claude Code
https://claude.ai/code/session_01G4hgdz4RAYqZVLWHZRYrd2