Skip to content
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

Merged
merged 1 commit into from Aug 22, 2014

Conversation

saarons
Copy link
Contributor

@saarons saarons commented Aug 21, 2014

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')

"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))
Copy link
Member

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 ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will do.

@jeremy
Copy link
Member

jeremy commented Aug 21, 2014

Good idea! 👍

@saarons saarons force-pushed the clean-remote-ip-middleware branch 3 times, most recently from c13a7fb to dcedf81 Compare August 21, 2014 23:04
@saarons
Copy link
Contributor Author

saarons commented Aug 21, 2014

@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)
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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]

@jeremy
Copy link
Member

jeremy commented Aug 21, 2014

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')
@saarons
Copy link
Contributor Author

saarons commented Aug 22, 2014

@jeremy Showing all green now.

jeremy added a commit that referenced this pull request Aug 22, 2014
ActionDispatch::RemoteIp accept IPAddr matches for trusted proxies
@jeremy jeremy merged commit 7fce216 into rails:master Aug 22, 2014
@saarons saarons deleted the clean-remote-ip-middleware branch August 22, 2014 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants