Add CIDR range functionality for SS_TRUSTED_PROXY_IPS

[brettt89]

To do before merge

• Refactor logic into ip_in_any_range($ipAddress, $rangeList)
• Add a unit test for ip_in_any_range() and/or ip_in_range()
• Ensure exact-matches on ipv6 addresses (e.g. ::1) will work.

Nice to have

• Parsing of IPv6 subnet ranges

[helpfulrobot]

@brettt89, thanks for your PR! By analyzing the blame information on this pull request, I identified @hafriedlander, @chillu and @halkyon to be potential reviewers

[hafriedlander]

I think we need to reverse this and have an "is this IP in this CIDR range" function - otherwise a big enough range (like 10.0.0.0/8) would end up creating an enormous list.

[tractorcow]

Can you please update the documentation? Please see https://github.com/silverstripe/silverstripe-framework/blob/master/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md#request-hostname-forgery for the current docs links.

Some phpdoc on the new method would be great too thanks. :)

[tractorcow]

Also, +1 on what @hafriedlander said; Make it a check rather than an exhaustive allowed value generation. :P

[dhensby]

Lot's of examples online how to check IP is in a CIDR declaration (here's one: https://gist.github.com/tott/7684443)

[brettt89]

Have made changes as per comments.

[dhensby]

If there's no slash, can't we do a basic comparison?

[brettt89]

Hi @dhensby , What do you mean by Basic Comparison? If there is no slash, it adds /32 onto the end (converts it to CIDR).

[dhensby]

If there's no slash, then it's not a range, it's a single IP, right? so we can do something like: $ip == $range

[sminnee]

Doing a straight comparison will mean that ::1 can be added.

[dhensby]

My only question now is how we deal with ipv6? Can we get some unit tests, too?

[tractorcow]

According to googling, there is an option for ipv6.

http://stackoverflow.com/questions/7951061/matching-ipv6-address-to-a-cidr-subnet

[brettt89]

Yeah, now I just need to figure out how to tie them both together.... Nicely.

[brettt89]

@dhensby Where abouts would you put tests for constants?
@tractorcow I have been failing to try and combine the ipv4 and ipv6 check into a single function, do you have any suggestions as to how we could achieve this?

[dhensby]

@brettt89 put them in seperate functions and then call the relevant one depending on whether the IP has : in it?

As for tests - I don't think we have any method for it :)

[sminnee]

I think you should refactor this to an ip_in_any_range() function, then you can add tests of that function.

if(SS_TRUSTED_PROXY_IPS === '*') {
  $trusted = true;
} elseif(isset($_SERVER['REMOTE_ADDR'])) {
  $trusted = ip_in_any_range($_SERVER['REMOTE_ADDR'], SS_TRUSTED_PROXY_IPS);
}

Ensure there is a test for exact match on ::1. Handling of IPv6 subnets wouldn't block merge but it might be nice.

[sminnee]

I've added some tasks to the top of the PR listing merge blockers and nice to haves.

[sminnee]

I believe this is superceded by #6062. If @andrewandante and @dhensby can confirm that is the case, can you close this?