Rate limiter whitelist of IP address #997
Conversation
config/app.php
Outdated
| 'whitelisted_ip_addresses' => env('RATE_LIMITER_WHITELISTED_IP_ADDRESSES') | ||
| ? explode(',', env('RATE_LIMITER_WHITELISTED_IP_ADDRESSES')) | ||
| : [], |
There was a problem hiding this comment.
Is there an actual need for the configurability? Could we just list the IPs here?
There was a problem hiding this comment.
It would be handy to be able to add our own IP addresses.
There was a problem hiding this comment.
Do you mean for development? Maybe we could disable it based on app environment then?
There was a problem hiding this comment.
I don't have any issues with the implementation, but I'm a little concerned that the whitelisting is potentially needed for development. In my opinion, the rate limiter should be a safety net, not a mode of operation.
My guess is that either
- the rate limiter is too strict, or
- the client implementation is not optimal, or
- the API needs a design adjustment
In any case, from an architecture perspective, I don't think the assumption that "nobody should call this so often, except for us" is 100% sound. The "whitelisted" calls are ultimately being triggered by visitors, so still outside of our control. In fact, for those apps -- that might potentially generate lots of requests -- we're losing that safety mechanism altogether.
My suggestion would be to increase the rate limit for everyone to a level we're comfortable with.
|
When using SSR frontends the requests will come from our own server, that's why we need to make a whitelist here. And if high rates are an issue we should implement rate limiter on the caller side. |
As suggested in #792