Action based rate limiting.

[russ]

Purpose

Adds a way to set per action rate limits.
Fixes #1865

Description

Include the module and write a rate_limit method to define the limit.
The rate_limit_key method can be overridden if you want differeny key logic.

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[russ]

I'm not a huge fan of this but I'm not sure how best to configure the storage at the individual spec level.

[jwoertink]

This is where you'd use temp_config and put the spec inside of that block. Here's an example:

lucky/spec/tasks/exec_spec.cr

Lines 16 to 20 in bddb10b

	private def with_test_template(&)
	Lucky::Exec.temp_config(template_path: "spec/support/exec_template.cr.template") do
	yield
	end
	end

It could just be added to some helper method.

[russ]

This code is a copy of the RemoteIP code. You can override the method in your action, but the default needs something and I think the IP makes sense. The specs pass in a HTTP::Request so I don't get context.remote_ip.

[jwoertink]

I think you should have access to request.remote_ip

lucky/src/charms/request_extensions.cr

Lines 1 to 8 in bddb10b

	class HTTP::Request
	# This is an alternative to `remote_address`
	# since that casts to `Socket::Address`, and not all
	# subclasses have an `address` method to give you the value.
	# ```
	# request.remote_address.as?(Socket::IPAddress).try(&.address)
	# ```
	property remote_ip : String = ""

This is actually patched in Lucky. context doesn't have a remote_ip method, but you may be thinking of context.request_id which is something different.

Now, with that said, it brings up the point that if you don't have the RemoteIpHandler in your middleware stack, that could cause issues... I wonder if there's a way that we could say you must have that handler in your stack in order to use this module? 🤔

[jwoertink]

This is amazing 🚀 Thanks for tackling this. I think we should definitely add LuckyCache as a dependency here to solve the spec failure. There's stuff in LuckyCache for doing view caching anyway, so it makes sense. I left a few other comments too.

[jwoertink]

This is where you'd use temp_config and put the spec inside of that block. Here's an example:

lucky/spec/tasks/exec_spec.cr

Lines 16 to 20 in bddb10b

	private def with_test_template(&)
	Lucky::Exec.temp_config(template_path: "spec/support/exec_template.cr.template") do
	yield
	end
	end

It could just be added to some helper method.

[jwoertink]

Maybe this can pull the header name from Lucky::RemoteIpHandler.settings.ip_header_name ?

[jwoertink]

I think you should have access to request.remote_ip

lucky/src/charms/request_extensions.cr

Lines 1 to 8 in bddb10b

	class HTTP::Request
	# This is an alternative to `remote_address`
	# since that casts to `Socket::Address`, and not all
	# subclasses have an `address` method to give you the value.
	# ```
	# request.remote_address.as?(Socket::IPAddress).try(&.address)
	# ```
	property remote_ip : String = ""

This is actually patched in Lucky. context doesn't have a remote_ip method, but you may be thinking of context.request_id which is something different.

Now, with that said, it brings up the point that if you don't have the RemoteIpHandler in your middleware stack, that could cause issues... I wonder if there's a way that we could say you must have that handler in your stack in order to use this module? 🤔

[jwoertink]

Suggested change

	klass = self.class.to_s.downcase.gsub("::", ":")
	klass = {{ @type.stringify.downcase.gsub(/::/, ":") }}

I'm not sure if this matters here, but if we build it at compile time, then it won't need to compute on each request.

[jwoertink]

This is actually a pretty slick interface. It feels consistent with some other Lucky modules.

[rmarronnier]

Thanks so much for this !

I'd love to get a macro one liner with keeping the possibility to have a method coputing values at runtime like you proposed.

Example :

class RateLimitRoutes::Index < TestAction
  include Lucky::RateLimit
  rate_limit to: 1, within: 1.minute

  get "/rate_limit" do
    plain_text "hello"
  end
end

The dynamic setup still works as before :

class ComputedRateLimitRoutes::Index < TestAction
  include Lucky::RateLimit

  get "/computed_rate_limit" do
    plain_text "hello"
  end

  private def computed_rate_limit : NamedTuple(to: Int32, within: Time::Span)
    {to: 1, within: 1.minute}
  end
end

Here's what I got working locally :

module Lucky::RateLimit
  macro included
    before enforce_rate_limit
  end

  macro rate_limit(**tuple)
    private def computed_rate_limit : NamedTuple(to: Int32, within: Time::Span)
      {{tuple}}
    end
  end

  abstract def computed_rate_limit : NamedTuple(to: Int32, within: Time::Span)

  private def enforce_rate_limit
    cache = LuckyCache.settings.storage
    count = cache.fetch(rate_limit_key, as: Int32, expires_in: computed_rate_limit["within"]) { 0 }
    cache.write(rate_limit_key, expires_in: computed_rate_limit["within"]) { count + 1 }

    if count > computed_rate_limit["to"]
      context.response.status = HTTP::Status::TOO_MANY_REQUESTS
      context.response.headers["Retry-After"] = computed_rate_limit["within"].to_s
      plain_text("Rate limit exceeded")
    else
      continue
    end
  end

  private def rate_limit_key : String
    klass = self.class.to_s.downcase.gsub("::", ":")
    "ratelimit:#{klass}:#{rate_limit_identifier}"
  end

  private def rate_limit_identifier : Socket::Address | Nil
    request = context.request

    if x_forwarded = request.headers["X_FORWARDED_FOR"]?.try(&.split(',').first?).presence
      begin
        Socket::IPAddress.new(x_forwarded, 0)
      rescue Socket::Error
        # if the x_forwarded is not a valid ip address we fallback to request.remote_address
        request.remote_address
      end
    else
      request.remote_address
    end
  end
end

I'm sure there are still improvements macro wise but that's the idea.