-
Notifications
You must be signed in to change notification settings - Fork 21.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor ActionDispatch::RemoteIp #16604
Conversation
"10.0.0.0/8", # private IPv4 range 10.x.x.x | ||
"172.16.0.0/12", # private IPv4 range 172.16.0.0 .. 172.31.255.255 | ||
"192.168.0.0/16", # private IPv4 range 192.168.x.x | ||
].map(&IPAddr.method(:new)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing out the block will help ppl understand what this does ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will do.
Good idea! 👍 |
c13a7fb
to
dcedf81
Compare
@jeremy implemented your suggestions, just waiting on the build to pass. Looks like something is wrong with the tests, but it could be me, still looking into it. |
elsif custom_proxies.respond_to?(:each) | ||
custom_proxies | ||
else | ||
Array.wrap(custom_proxies).concat(TRUSTED_PROXIES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may mutate the custom_proxies
argument. Can also use Array()
directly in this case: Array(custom_proxies) + TRUSTED_PROXIES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that about Array.wrap mutating the argument, I'll put in the line you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.wrap itself doesn't mutate. It passes through an array argument directly, exposing it to mutating by #concat
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
>> a = [1,2]
=> [1, 2]
>> Array.wrap(a).concat [3]
=> [1, 2, 3]
>> a
=> [1, 2, 3]
There's a libmysql + ruby segfault unfortunately - messing up the builds. Looks like it's an issue with signal handling. |
Refactored IP address checking in ActionDispatch::RemoteIp to rely on the IPAddr class instead of the unwieldly regular expression to match IP addresses. This commit keeps the same api but allows users to pass IPAddr objects to config.action_dispatch.trusted_proxies in addition to passing strings and regular expressions. Example: # config/environments/production.rb config.action_dispatch.trusted_proxies = IPAddr.new('4.8.15.0/16')
dcedf81
to
f9a84bb
Compare
@jeremy Showing all green now. |
ActionDispatch::RemoteIp accept IPAddr matches for trusted proxies
Refactored IP address checking in
ActionDispatch::RemoteIp
to rely onthe
IPAddr
class instead of the unwieldly regular expression to matchIP addresses. This commit keeps the same api but allows users to pass
IPAddr
objects toconfig.action_dispatch.trusted_proxies
in additionto passing strings and regular expressions.
Example: