Skip to content

fix: validate the trusted X-Forwarded-For entry as client_ip - #7880

Open
MaiAditya wants to merge 3 commits into
reflex-dev:mainfrom
MaiAditya:fix/trusted-proxy-client-ip
Open

MaiAditya wants to merge 3 commits into
reflex-dev:mainfrom
MaiAditya:fix/trusted-proxy-client-ip

Conversation

@MaiAditya

@MaiAditya MaiAditya commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Closes #7854

With trusted_proxy_hops set, the selected X-Forwarded-For entry became router.session.client_ip as is, so 203.0.113.7:51234, [2001:db8::1]:443 or text that isn't an address at all ended up there.

  • New _forwarded_client_ip() drops a numeric :port suffix or IPv6 brackets and returns str(ipaddress.ip_address(...)), or None if the entry isn't an address. Malformed forms (an unclosed bracket, text after ], a non-numeric or empty port) and IPv6 zone indexes (fe80::1%eth0, which ipaddress keeps without validating) are rejected rather than trimmed. It's only used at the client_ip site; _trusted_forwarded_value() and the X-Forwarded-Host / X-Forwarded-Proto handling are unchanged.
  • An invalid entry falls back to the connection's peer address and is logged at debug level (through the existing _clip), since the value is client-controlled when a deployment is misconfigured.
  • Valid IPv6 entries are now canonical (2001:DB8:0:0:0:0:0:1 becomes 2001:db8::1), and leading-zero IPv4 such as 010.0.0.1 falls back to the peer address, which ipaddress rejects.
  • The trusted_proxy_hops docs mention the normalisation, and the bugfix news fragment notes the change so other client_ip consumers can follow.

Tests: new cases in test_static_router_data_resolves_client_ip_from_trusted_proxy_hops (port, bracketed IPv6 with and without port, expanded IPv6, a non-address entry, malformed brackets and ports, zone indexes) and test_static_router_data_logs_ignored_forwarded_client_ip; 13 of them fail on main. pytest tests/units/test_app.py tests/units/test_config.py: 500 passed; ruff check/format clean.

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • Bug fix (non-breaking change which fixes an issue)

View guided diff

With trusted_proxy_hops set, the selected X-Forwarded-For entry became
router.session.client_ip verbatim, so values with a port or that were not
addresses at all were passed through. Normalize the entry with
ipaddress.ip_address after dropping a port and IPv6 brackets, and fall back
to the peer address (logging at debug level) when it is not a valid address.

Fixes reflex-dev#7854
@MaiAditya
MaiAditya requested review from a team and Alek99 as code owners November 13, 2026 03:40
@greptile-apps

greptile-apps Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] This PR appears safe to merge; no actionable issues remain in the reviewed changes.

Summary

This PR makes router.session.client_ip use a valid, normalized address from the trusted proxy entry, or the connection's peer address when that entry is invalid.

  • Socket sessions now use a validated IP from the trusted forwarded entry.

The earlier malformed-entry finding is fixed. No new actionable issues were found.

Reviews (3) · Last reviewed commit: "fix: reject IPv6 zone indexes in X-Forwa..." · Reviewed by Greptile

Comment thread reflex/app.py Resolved

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread reflex/app.py Resolved
Comment thread tests/units/test_app.py Resolved
@codspeed

codspeed Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 5.9%

2 warnings

⚠️ 10 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ 3 benchmarks with a significant change ran on different hardware

Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.

Learn more about Macro Runners

See affected benchmarks

❌ 3 regressed benchmarks
✅ 148 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_route_matching[1000] 47.5 µs 50.8 µs -6.44%
❌ test_state_update_wire_serialization[model_1000] 25 ms 26.5 ms -5.69%
❌ test_route_matching[10] 50.2 µs 53.2 µs -5.56%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing MaiAditya:fix/trusted-proxy-client-ip (1b0af00) with main (cb9ee91)

Open in CodSpeed

@YusefSyed

Copy link
Copy Markdown

ipaddress.ip_address() doesn't validate an IPv6 zone index, the text after a %, so with trusted_proxy_hops=1 a client can still pick its own client_ip on this branch (b3a32ee):

X-Forwarded-For: fe80::1%not an ip <script>   ->  client_ip 'fe80::1%not an ip <script>'
X-Forwarded-For: [fe80::1%eth0]:443           ->  client_ip 'fe80::1%eth0'

That's a websocket client connecting straight to the backend. A zone only means something on the proxy's own host, so rejecting it should be safe: if "%" in entry: return None after the bracket and port stripping in _forwarded_client_ip closes it. With those two entries added as cases to test_static_router_data_resolves_client_ip_from_trusted_proxy_hops, both fail on this branch and pass with that change.

An unclosed bracket, text after the closing bracket or a non-numeric
port now falls back to the peer address instead of being trimmed away.
The log test only looks at the reflex.app debug records.

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 2 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread reflex/app.py Resolved
ipaddress keeps the text after % without validating it, so a client could
still choose its own client_ip through it. A zone only means something on
the proxy's own host, so such entries now fall back to the peer address.
@MaiAditya

Copy link
Copy Markdown
Contributor Author

Good catch, thanks. Fixed in 1b0af00: an entry with a % zone index now falls back to the peer address, and both of your examples are in the parametrized test (they fail on the previous head and pass now).

@FarhanAliRaza FarhanAliRaza added the bug Something isn't working label Nov 13, 2026 — with ChatGPT Codex Connector

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trusted_proxy_hops: the selected X-Forwarded-For entry becomes client_ip verbatim, with no IP validation

3 participants