Discards optional port information from X-Forwarded-For header's IP parsing #1251
Conversation
This file contains 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
|
@tenderlove hopefully you can squash this into Thanks!
|
dpritchett
force-pushed
the
djp/x-forwarded-for-with-ports
branch
4 times, most recently
from
0a5958d
to
ce7f9e1
Compare
|
Please don't merge me yet - I've found a busted merge in trying to preserve @wuhanyumsft's earlier PR. |
dpritchett
force-pushed
the
djp/x-forwarded-for-with-ports
branch
2 times, most recently
from
e64a43b
to
e9eae8b
Compare
|
Ok it's clean again, but I lost the @wuhanyumsft diff 😢 |
dpritchett
force-pushed
the
djp/x-forwarded-for-with-ports
branch
from
e9eae8b
to
a3dbe54
Compare
|
Great! Thank you! |
eileencodes
pushed a commit
to eileencodes/rack
that referenced
this pull request
Apr 16, 2019
…-ports Discards optional port information from X-Forwarded-For header's IP parsing
duncanjbrown
added a commit
to duncanjbrown/rails
that referenced
this pull request
May 5, 2020
Rack decided to tolerate proxies which choose to attach ports to X-Forwarded-For IPs by stripping the port: rack/rack#1251. Attaching a port is rare in the wild but some proxies (notably Microsoft Azure's App Service) do it. Without this patch, remote_ip will ignore X-Forwarded-For IPs with ports attached and the return value is less likely to be useful. Rails should do the same thing. The stripping logic is already available in Rack::Request::Helpers, so change the X-Forwarded-For retrieval method from ActionDispatch::Request#x_forwarded_for (which returns the raw header) to #forwarded_for, which returns a stripped array of IP addresses, or nil. There may be other benefits hiding in Rack's implementation. We can't call ips_from with an array (and legislating for that inside ips_from doesn't appeal), so refactor out the bit we need to apply in both cases (verifying the IP is acceptable to IPAddr and that it's not a range) to a separate method called #sanitize_ips which reduces an array of maybe-ips to an array of acceptable ones.
georgeclaghorn
pushed a commit
to rails/rails
that referenced
this pull request
May 10, 2020
Rack decided to tolerate proxies which choose to attach ports to X-Forwarded-For IPs by stripping the port: rack/rack#1251. Attaching a port is rare in the wild but some proxies (notably Microsoft Azure's App Service) do it. Without this patch, remote_ip will ignore X-Forwarded-For IPs with ports attached and the return value is less likely to be useful. Rails should do the same thing. The stripping logic is already available in Rack::Request::Helpers, so change the X-Forwarded-For retrieval method from ActionDispatch::Request#x_forwarded_for (which returns the raw header) to #forwarded_for, which returns a stripped array of IP addresses, or nil. There may be other benefits hiding in Rack's implementation. We can't call ips_from with an array (and legislating for that inside ips_from doesn't appeal), so refactor out the bit we need to apply in both cases (verifying the IP is acceptable to IPAddr and that it's not a range) to a separate method called #sanitize_ips which reduces an array of maybe-ips to an array of acceptable ones.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tests and documents the work from #1229 which should close #1227.
RFC 7239 describes an optional port specification in
X-Forwarded-Forthat is apparently implemented in Microsoft Azure Application Gateway.The relevant examples from the RFC are in section 6:
This PR attempts to gently discard the port information from
X-Forwarded-For.Should also close #1229.