hapi-rate-limit icon indicating copy to clipboard operation
hapi-rate-limit copied to clipboard

Enable ip ranges when whitelisting and ip whitelisting also for path checks

Open cur3n4 opened this issue 6 years ago • 3 comments

This PR adds the ability to use CIDR to specify IP ranges when whitelisting IP addresses. It also considers IP whitelisting for path checks. Was there any reason that they IP whitelists are currently not taking into account for path checks?

This PR adds a dependency on https://github.com/keverw/range_check, which might or might not be acceptable.

cur3n4 avatar Mar 25 '19 02:03 cur3n4

Wow, two pull requests! Thank you for taking this time to write these.

The reason IP whitelisting was not added to the path checks is because I had only been thinking of IPs as "unauthenticated users" which meant that combining both IP and path considerations fell under the umbrella of the "userPath" checks.

This would be my only reason for hesitating implementing this, up until now that separation was pretty distinct. Allowing the ipWhitelist to affect also the path checks would muddy that distinction.

I do see this as a very valid request though. Being able to blanket whitelist an IP or IP range for path checks feels very proper. The question I have to answer for myself (and would ask you) is should we allow the whitelist to cross over like this, or should the idea of a "blanket" whitelist that overarches other "IP as a user" concerns and exists separately be considered?

I see that you built this PR off of the code in #65 so of course I would like to get that one merged first.

wraithgar avatar Mar 25 '19 20:03 wraithgar

Thanks for having a look at the PR and for maintaining this library!

I just realised that the same sort of ambiguity exists around userWhitelist.

I am not sure what is the best way forward, I am not terribly familiar with how rate limiting is usually configured. But here go a few ideas:

  • The configuration could allow more granularity around the whitelisting, at the expense of additional complexity, allowing to configure the user and ip whitelist for each of the three limiters (path, user and user path). There could also be a blanket whitelist which applies for all the limits.
  • I think I would find it clearer if there was a distinction between authenticated and unauthenticated limits (user is an authenticated entity, ip is a remote address). So user, userPath, ip and ipPath cache entries; the first two ones applying only to authenticated requests and the last two to every request.
  • The configuration could also allow to define dynamic limits and whitelist. For example something like: ['user', 'ip', 'path'] would create a limit based on a user coming from the same ip on the same path.

cur3n4 avatar Mar 25 '19 22:03 cur3n4

I agree it would be clearer if there was a distinction between IP and Authenticated user. In hindsight making the IP be the "fallback" to user was a poor choice on my part. Your second bullet point makes the most sense to me.

Making that change would involve quite a refactor, and one that introduced breaking changes. I'm not really terribly averse to breaking changes when they make sense (and again w/ the benefit of hindsight think this one in particular would be good). I am however pretty limited lately w/ the amount of attention I can give to some of the libraries I maintain.

So while I don't want to just throw this in someone else's lap and say "hey go do a bunch of free work", that would likely be the only way this kind of a refactor would happen any time soon.

I really do like the idea of allowing for CIDR strings on the existing ip lists, and think that if this PR were pared down to just introduce that, it would be a quick an easy decision to merge and release it. We can leave the broader question for another push then.

wraithgar avatar Mar 25 '19 23:03 wraithgar